Re: [RFC PATCH 2/2] macb: Enable 1588 support in SAMA5D2 platform.

[Andrei Pistirica]

Hi Richard,

I will take your indications into account in next version of the patch.

Regards,
Andrei

On 06.09.2016 18:37, Richard Cochran wrote:

On Fri, Sep 02, 2016 at 02:53:37PM +0200, Andrei Pistirica wrote:

Hardware time stamp on the PTP Ethernet packets are received using the
SO_TIMESTAMPING API. Timers are obtained from the PTP event/peer
gem registers.

Signed-off-by: Andrei Pistirica 
---
Integration with SAMA5D2 only. This feature wasn't tested on any
other platform that might use cadence/gem.


What does that mean?  I didn't see any references to SAMA5D2 anywhere
in your patch.

The driver needs to positively identify the HW that supports this
feature.


diff --git a/drivers/net/ethernet/cadence/macb.c 
b/drivers/net/ethernet/cadence/macb.c
index 8d54e7b..18f0715 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -697,6 +697,11 @@ static void macb_tx_interrupt(struct macb_queue *queue)

/* First, update TX stats if needed */
if (skb) {
+/* guard the hot-path */
+#ifdef CONFIG_MACB_USE_HWSTAMP
+   if (bp->hwts_tx_en)
+   macb_ptp_do_txstamp(bp, skb);
+#endif


Pull the test into the helper function, and then you can drop the
ifdef and the funny comment.


netdev_vdbg(bp->dev, "skb %u (data %p) TX 
complete\n",
macb_tx_ring_wrap(tail), skb->data);
bp->stats.tx_packets++;
@@ -853,6 +858,11 @@ static int gem_rx(struct macb *bp, int budget)
GEM_BFEXT(RX_CSUM, ctrl) & GEM_RX_CSUM_CHECKED_MASK)
skb->ip_summed = CHECKSUM_UNNECESSARY;

+/* guard the hot-path */
+#ifdef CONFIG_MACB_USE_HWSTAMP
+   if (bp->hwts_rx_en)
+   macb_ptp_do_rxstamp(bp, skb);
+#endif


Same here.


bp->stats.rx_packets++;
bp->stats.rx_bytes += skb->len;

@@ -1723,6 +1733,11 @@ static void macb_init_hw(struct macb *bp)

/* Enable TX and RX */
macb_writel(bp, NCR, MACB_BIT(RE) | MACB_BIT(TE) | MACB_BIT(MPE));
+
+#ifdef CONFIG_MACB_USE_HWSTAMP
+   bp->hwts_tx_en = 0;
+   bp->hwts_rx_en = 0;
+#endif


We don't initialize to zero unless we have to.


 }

 /*
@@ -1885,6 +1900,8 @@ static int macb_open(struct net_device *dev)

netif_tx_start_all_queues(dev);

+   macb_ptp_init(dev);
+
return 0;
 }

@@ -2143,7 +2160,7 @@ static const struct ethtool_ops gem_ethtool_ops = {
.get_regs_len   = macb_get_regs_len,
.get_regs   = macb_get_regs,
.get_link   = ethtool_op_get_link,
-   .get_ts_info= ethtool_op_get_ts_info,
+   .get_ts_info= macb_get_ts_info,
.get_ethtool_stats  = gem_get_ethtool_stats,
.get_strings= gem_get_ethtool_strings,
.get_sset_count = gem_get_sset_count,
@@ -2157,6 +2174,12 @@ static int macb_ioctl(struct net_device *dev, struct 
ifreq *rq, int cmd)
if (!netif_running(dev))
return -EINVAL;

+   if (cmd == SIOCSHWTSTAMP)
+   return macb_hwtst_set(dev, rq, cmd);
+
+   if (cmd == SIOCGHWTSTAMP)
+   return macb_hwtst_get(dev, rq);


switch/case?


+
if (!phydev)
return -ENODEV;

diff --git a/drivers/net/ethernet/cadence/macb.h 
b/drivers/net/ethernet/cadence/macb.h
index 8c3779d..555316a 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -920,8 +920,21 @@ struct macb {

 #ifdef CONFIG_MACB_USE_HWSTAMP
 void macb_ptp_init(struct net_device *ndev);
+void macb_ptp_do_rxstamp(struct macb *bp, struct sk_buff *skb);
+void macb_ptp_do_txstamp(struct macb *bp, struct sk_buff *skb);
+int macb_ptp_get_ts_info(struct net_device *dev, struct ethtool_ts_info *info);
+#define macb_get_ts_info macb_ptp_get_ts_info
+int macb_hwtst_set(struct net_device *netdev, struct ifreq *ifr, int cmd);
+int macb_hwtst_get(struct net_device *netdev, struct ifreq *ifr);
 #else
 void macb_ptp_init(struct net_device *ndev) { }
+void macb_ptp_do_rxstamp(struct macb *bp, struct sk_buff *skb) { }
+void macb_ptp_do_txstamp(struct macb *bp, struct sk_buff *skb) { }
+#define macb_get_ts_info ethtool_op_get_ts_info
+int macb_hwtst_set(struct net_device *netdev, struct ifreq *ifr, int cmd)
+   { return -1; }
+int macb_hwtst_get(struct net_device *netdev, struct ifreq *ifr)
+   { return -1; }


Use a proper return code please.


 #endif

 static inline bool macb_is_gem(struct macb *bp)
diff --git a/drivers/net/ethernet/cadence/macb_ptp.c 
b/drivers/net/ethernet/cadence/macb_ptp.c
index 6d6a6ec..e3f784a 100644
--- a/drivers/net/ethernet/cadence/macb_ptp.c
+++ b/drivers/net/ethernet/cadence/macb_ptp.c
@@ -5,6 +5,7 @@
  * Copyright (C) 2016 Microchip Technology
  *
  * 

Re: [RFC PATCH 2/2] macb: Enable 1588 support in SAMA5D2 platform.

[Richard Cochran]

On Fri, Sep 02, 2016 at 02:53:37PM +0200, Andrei Pistirica wrote:
> Hardware time stamp on the PTP Ethernet packets are received using the
> SO_TIMESTAMPING API. Timers are obtained from the PTP event/peer
> gem registers.
> 
> Signed-off-by: Andrei Pistirica 
> ---
> Integration with SAMA5D2 only. This feature wasn't tested on any
> other platform that might use cadence/gem.

What does that mean?  I didn't see any references to SAMA5D2 anywhere
in your patch.

The driver needs to positively identify the HW that supports this
feature.

> diff --git a/drivers/net/ethernet/cadence/macb.c 
> b/drivers/net/ethernet/cadence/macb.c
> index 8d54e7b..18f0715 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -697,6 +697,11 @@ static void macb_tx_interrupt(struct macb_queue *queue)
>  
>   /* First, update TX stats if needed */
>   if (skb) {
> +/* guard the hot-path */
> +#ifdef CONFIG_MACB_USE_HWSTAMP
> + if (bp->hwts_tx_en)
> + macb_ptp_do_txstamp(bp, skb);
> +#endif

Pull the test into the helper function, and then you can drop the
ifdef and the funny comment.

>   netdev_vdbg(bp->dev, "skb %u (data %p) TX 
> complete\n",
>   macb_tx_ring_wrap(tail), skb->data);
>   bp->stats.tx_packets++;
> @@ -853,6 +858,11 @@ static int gem_rx(struct macb *bp, int budget)
>   GEM_BFEXT(RX_CSUM, ctrl) & GEM_RX_CSUM_CHECKED_MASK)
>   skb->ip_summed = CHECKSUM_UNNECESSARY;
>  
> +/* guard the hot-path */
> +#ifdef CONFIG_MACB_USE_HWSTAMP
> + if (bp->hwts_rx_en)
> + macb_ptp_do_rxstamp(bp, skb);
> +#endif

Same here.

>   bp->stats.rx_packets++;
>   bp->stats.rx_bytes += skb->len;
>  
> @@ -1723,6 +1733,11 @@ static void macb_init_hw(struct macb *bp)
>  
>   /* Enable TX and RX */
>   macb_writel(bp, NCR, MACB_BIT(RE) | MACB_BIT(TE) | MACB_BIT(MPE));
> +
> +#ifdef CONFIG_MACB_USE_HWSTAMP
> + bp->hwts_tx_en = 0;
> + bp->hwts_rx_en = 0;
> +#endif

We don't initialize to zero unless we have to.

>  }
>  
>  /*
> @@ -1885,6 +1900,8 @@ static int macb_open(struct net_device *dev)
>  
>   netif_tx_start_all_queues(dev);
>  
> + macb_ptp_init(dev);
> +
>   return 0;
>  }
>  
> @@ -2143,7 +2160,7 @@ static const struct ethtool_ops gem_ethtool_ops = {
>   .get_regs_len   = macb_get_regs_len,
>   .get_regs   = macb_get_regs,
>   .get_link   = ethtool_op_get_link,
> - .get_ts_info= ethtool_op_get_ts_info,
> + .get_ts_info= macb_get_ts_info,
>   .get_ethtool_stats  = gem_get_ethtool_stats,
>   .get_strings= gem_get_ethtool_strings,
>   .get_sset_count = gem_get_sset_count,
> @@ -2157,6 +2174,12 @@ static int macb_ioctl(struct net_device *dev, struct 
> ifreq *rq, int cmd)
>   if (!netif_running(dev))
>   return -EINVAL;
>  
> + if (cmd == SIOCSHWTSTAMP)
> + return macb_hwtst_set(dev, rq, cmd);
> +
> + if (cmd == SIOCGHWTSTAMP)
> + return macb_hwtst_get(dev, rq);

switch/case?

> +
>   if (!phydev)
>   return -ENODEV;
>  
> diff --git a/drivers/net/ethernet/cadence/macb.h 
> b/drivers/net/ethernet/cadence/macb.h
> index 8c3779d..555316a 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -920,8 +920,21 @@ struct macb {
>  
>  #ifdef CONFIG_MACB_USE_HWSTAMP
>  void macb_ptp_init(struct net_device *ndev);
> +void macb_ptp_do_rxstamp(struct macb *bp, struct sk_buff *skb);
> +void macb_ptp_do_txstamp(struct macb *bp, struct sk_buff *skb);
> +int macb_ptp_get_ts_info(struct net_device *dev, struct ethtool_ts_info 
> *info);
> +#define macb_get_ts_info macb_ptp_get_ts_info
> +int macb_hwtst_set(struct net_device *netdev, struct ifreq *ifr, int cmd);
> +int macb_hwtst_get(struct net_device *netdev, struct ifreq *ifr);
>  #else
>  void macb_ptp_init(struct net_device *ndev) { }
> +void macb_ptp_do_rxstamp(struct macb *bp, struct sk_buff *skb) { }
> +void macb_ptp_do_txstamp(struct macb *bp, struct sk_buff *skb) { }
> +#define macb_get_ts_info ethtool_op_get_ts_info
> +int macb_hwtst_set(struct net_device *netdev, struct ifreq *ifr, int cmd)
> + { return -1; }
> +int macb_hwtst_get(struct net_device *netdev, struct ifreq *ifr)
> + { return -1; }

Use a proper return code please.

>  #endif
>  
>  static inline bool macb_is_gem(struct macb *bp)
> diff --git a/drivers/net/ethernet/cadence/macb_ptp.c 
> b/drivers/net/ethernet/cadence/macb_ptp.c
> index 6d6a6ec..e3f784a 100644
> --- a/drivers/net/ethernet/cadence/macb_ptp.c
> +++ b/drivers/net/ethernet/cadence/macb_ptp.c
> @@ -5,6 +5,7 @@
>   * Copyright (C) 2016 Microchip 

Re: [RFC PATCH 2/2] macb: Enable 1588 support in SAMA5D2 platform.

[Harini Katakam]

Hi Andrei,

+Richard Cochran

On Fri, Sep 2, 2016 at 6:23 PM, Andrei Pistirica
 wrote:
> Hardware time stamp on the PTP Ethernet packets are received using the
> SO_TIMESTAMPING API. Timers are obtained from the PTP event/peer
> gem registers.
>
> Signed-off-by: Andrei Pistirica 
> ---
> Integration with SAMA5D2 only. This feature wasn't tested on any
> other platform that might use cadence/gem.
>
> Patch is not completely ported to the very latest version of net-next,
> and it will be after review.
>

> @@ -853,6 +858,11 @@ static int gem_rx(struct macb *bp, int budget)
> GEM_BFEXT(RX_CSUM, ctrl) & GEM_RX_CSUM_CHECKED_MASK)
> skb->ip_summed = CHECKSUM_UNNECESSARY;
>
> +/* guard the hot-path */
> +#ifdef CONFIG_MACB_USE_HWSTAMP
> +   if (bp->hwts_rx_en)
> +   macb_ptp_do_rxstamp(bp, skb);
> +#endif

I'm just wondering if the same #ifdef can be used for timestamping
in all versions of this IP.
As you know, ZynqMP uses the timestamp from BD and older
versions of do not have an indication or extended BD support.
So, it might useful to add a dependency/check on the product family,
through config structure.
That way, both version can use their respective RX timestamp methods
based on the compatible string.
Please let me know if you have any other ideas.

Regards,
Harini

[RFC PATCH 2/2] macb: Enable 1588 support in SAMA5D2 platform.

[Andrei Pistirica]

Hardware time stamp on the PTP Ethernet packets are received using the
SO_TIMESTAMPING API. Timers are obtained from the PTP event/peer
gem registers.

Signed-off-by: Andrei Pistirica 
---
Integration with SAMA5D2 only. This feature wasn't tested on any
other platform that might use cadence/gem.

Patch is not completely ported to the very latest version of net-next,
and it will be after review.

 drivers/net/ethernet/cadence/macb.c |  25 +++-
 drivers/net/ethernet/cadence/macb.h |  13 ++
 drivers/net/ethernet/cadence/macb_ptp.c | 217 
 3 files changed, 254 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cadence/macb.c 
b/drivers/net/ethernet/cadence/macb.c
index 8d54e7b..18f0715 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -697,6 +697,11 @@ static void macb_tx_interrupt(struct macb_queue *queue)
 
/* First, update TX stats if needed */
if (skb) {
+/* guard the hot-path */
+#ifdef CONFIG_MACB_USE_HWSTAMP
+   if (bp->hwts_tx_en)
+   macb_ptp_do_txstamp(bp, skb);
+#endif
netdev_vdbg(bp->dev, "skb %u (data %p) TX 
complete\n",
macb_tx_ring_wrap(tail), skb->data);
bp->stats.tx_packets++;
@@ -853,6 +858,11 @@ static int gem_rx(struct macb *bp, int budget)
GEM_BFEXT(RX_CSUM, ctrl) & GEM_RX_CSUM_CHECKED_MASK)
skb->ip_summed = CHECKSUM_UNNECESSARY;
 
+/* guard the hot-path */
+#ifdef CONFIG_MACB_USE_HWSTAMP
+   if (bp->hwts_rx_en)
+   macb_ptp_do_rxstamp(bp, skb);
+#endif
bp->stats.rx_packets++;
bp->stats.rx_bytes += skb->len;
 
@@ -1723,6 +1733,11 @@ static void macb_init_hw(struct macb *bp)
 
/* Enable TX and RX */
macb_writel(bp, NCR, MACB_BIT(RE) | MACB_BIT(TE) | MACB_BIT(MPE));
+
+#ifdef CONFIG_MACB_USE_HWSTAMP
+   bp->hwts_tx_en = 0;
+   bp->hwts_rx_en = 0;
+#endif
 }
 
 /*
@@ -1885,6 +1900,8 @@ static int macb_open(struct net_device *dev)
 
netif_tx_start_all_queues(dev);
 
+   macb_ptp_init(dev);
+
return 0;
 }
 
@@ -2143,7 +2160,7 @@ static const struct ethtool_ops gem_ethtool_ops = {
.get_regs_len   = macb_get_regs_len,
.get_regs   = macb_get_regs,
.get_link   = ethtool_op_get_link,
-   .get_ts_info= ethtool_op_get_ts_info,
+   .get_ts_info= macb_get_ts_info,
.get_ethtool_stats  = gem_get_ethtool_stats,
.get_strings= gem_get_ethtool_strings,
.get_sset_count = gem_get_sset_count,
@@ -2157,6 +2174,12 @@ static int macb_ioctl(struct net_device *dev, struct 
ifreq *rq, int cmd)
if (!netif_running(dev))
return -EINVAL;
 
+   if (cmd == SIOCSHWTSTAMP)
+   return macb_hwtst_set(dev, rq, cmd);
+
+   if (cmd == SIOCGHWTSTAMP)
+   return macb_hwtst_get(dev, rq);
+
if (!phydev)
return -ENODEV;
 
diff --git a/drivers/net/ethernet/cadence/macb.h 
b/drivers/net/ethernet/cadence/macb.h
index 8c3779d..555316a 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -920,8 +920,21 @@ struct macb {
 
 #ifdef CONFIG_MACB_USE_HWSTAMP
 void macb_ptp_init(struct net_device *ndev);
+void macb_ptp_do_rxstamp(struct macb *bp, struct sk_buff *skb);
+void macb_ptp_do_txstamp(struct macb *bp, struct sk_buff *skb);
+int macb_ptp_get_ts_info(struct net_device *dev, struct ethtool_ts_info *info);
+#define macb_get_ts_info macb_ptp_get_ts_info
+int macb_hwtst_set(struct net_device *netdev, struct ifreq *ifr, int cmd);
+int macb_hwtst_get(struct net_device *netdev, struct ifreq *ifr);
 #else
 void macb_ptp_init(struct net_device *ndev) { }
+void macb_ptp_do_rxstamp(struct macb *bp, struct sk_buff *skb) { }
+void macb_ptp_do_txstamp(struct macb *bp, struct sk_buff *skb) { }
+#define macb_get_ts_info ethtool_op_get_ts_info
+int macb_hwtst_set(struct net_device *netdev, struct ifreq *ifr, int cmd)
+   { return -1; }
+int macb_hwtst_get(struct net_device *netdev, struct ifreq *ifr)
+   { return -1; }
 #endif
 
 static inline bool macb_is_gem(struct macb *bp)
diff --git a/drivers/net/ethernet/cadence/macb_ptp.c 
b/drivers/net/ethernet/cadence/macb_ptp.c
index 6d6a6ec..e3f784a 100644
--- a/drivers/net/ethernet/cadence/macb_ptp.c
+++ b/drivers/net/ethernet/cadence/macb_ptp.c
@@ -5,6 +5,7 @@
  * Copyright (C) 2016 Microchip Technology
  *
  * Authors: Harini Katakam 
+ * Andrei Pistirica 
  *
  * This file is licensed under the terms of the GNU General Public
  * License version 2. This program is licensed "as is" without any
@@