[PATCH 0/3] Macb PTP updates

[PATCH 0/3] Macb PTP updates

[Harini Katakam]

Macb PTP updates to handle PTP one step sync support and other minor
enhancements.

Harini Katakam (3):
  net: macb: Fix PTP one step sync support
  net: macb: Enable PTP unicast
  net: macb: Optimize reading HW timestamp

 drivers/net/ethernet/cadence/macb.h      |  4 ++
 drivers/net/ethernet/cadence/macb_main.c | 61 +++++++++++++++++++-----
 drivers/net/ethernet/cadence/macb_ptp.c  | 12 +++--
 3 files changed, 63 insertions(+), 14 deletions(-)

-- 
2.17.1

[PATCH 1/3] net: macb: Fix PTP one step sync support

[Harini Katakam]

PTP one step sync packets cannot have CSUM padding and insertion in
SW since time stamp is inserted on the fly by HW.
In addition, ptp4l version 3.0 and above report an error when skb
timestamps are reported for packets that not processed for TX TS
after transmission.
Add a helper to identify PTP one step sync and fix the above two
errors.
Also reset ptp OSS bit when one step is not selected.

Fixes: ab91f0a9b5f4 ("net: macb: Add hardware PTP support")
Fixes: 653e92a9175e ("net: macb: add support for padding and fcs computation")
Signed-off-by: Harini Katakam <harini.katakam@xilinx.com>
Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
---
Notes:
-> Added the macb pad and fcs fixes tag because strictly speaking the PTP support
patch precedes the fcs patch in timeline.
-> FYI, the error observed with setting HW TX timestamp for one step sync packets:
ptp4l[405.292]: port 1: unexpected socket error

 drivers/net/ethernet/cadence/macb_main.c | 54 ++++++++++++++++++++----
 drivers/net/ethernet/cadence/macb_ptp.c  |  4 +-
 2 files changed, 48 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index e993616308f8..e23a03e8badf 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -37,6 +37,7 @@
 #include <linux/phy/phy.h>
 #include <linux/pm_runtime.h>
 #include <linux/reset.h>
+#include <linux/ptp_classify.h>
 #include "macb.h"
 
 /* This structure is only used for MACB on SiFive FU540 devices */
@@ -98,6 +99,9 @@ struct sifive_fu540_macb_mgmt {
 
 #define MACB_MDIO_TIMEOUT	1000000 /* in usecs */
 
+/* IEEE1588 PTP flag field values  */
+#define PTP_FLAG_TWOSTEP	0x2
+
 /* DMA buffer descriptor might be different size
  * depends on hardware configuration:
  *
@@ -1122,6 +1126,36 @@ static void macb_tx_error_task(struct work_struct *work)
 	napi_enable(&queue->napi_tx);
 }
 
+static inline bool ptp_oss(struct sk_buff *skb)
+{
+	struct ptp_header *hdr;
+	unsigned int ptp_class;
+	u8 msgtype;
+
+	/* No need to parse packet if PTP TS is not involved */
+	if (!(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP))
+		goto not_oss;
+
+	/* Identify and return whether PTP one step sync is being processed */
+	ptp_class = ptp_classify_raw(skb);
+	if (ptp_class == PTP_CLASS_NONE)
+		goto not_oss;
+
+	hdr = ptp_parse_header(skb, ptp_class);
+	if (!hdr)
+		goto not_oss;
+
+	if (hdr->flag_field[0] & PTP_FLAG_TWOSTEP)
+		goto not_oss;
+
+	msgtype = ptp_get_msgtype(hdr, ptp_class);
+	if (msgtype == PTP_MSGTYPE_SYNC)
+		return true;
+
+not_oss:
+	return false;
+}
+
 static int macb_tx_complete(struct macb_queue *queue, int budget)
 {
 	struct macb *bp = queue->bp;
@@ -1158,13 +1192,14 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
 
 			/* First, update TX stats if needed */
 			if (skb) {
-				if (unlikely(skb_shinfo(skb)->tx_flags &
-					     SKBTX_HW_TSTAMP) &&
-				    gem_ptp_do_txstamp(queue, skb, desc) == 0) {
-					/* skb now belongs to timestamp buffer
-					 * and will be removed later
-					 */
-					tx_skb->skb = NULL;
+				if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
+				    !ptp_oss(skb)) {
+					if (gem_ptp_do_txstamp(queue, skb, desc) == 0) {
+						/* skb now belongs to timestamp buffer
+						 * and will be removed later
+						 */
+						tx_skb->skb = NULL;
+					}
 				}
 				netdev_vdbg(bp->dev, "skb %u (data %p) TX complete\n",
 					    macb_tx_ring_wrap(bp, tail),
@@ -2063,7 +2098,7 @@ static unsigned int macb_tx_map(struct macb *bp,
 			ctrl |= MACB_BF(TX_LSO, lso_ctrl);
 			ctrl |= MACB_BF(TX_TCP_SEQ_SRC, seq_ctrl);
 			if ((bp->dev->features & NETIF_F_HW_CSUM) &&
-			    skb->ip_summed != CHECKSUM_PARTIAL && !lso_ctrl)
+			    skb->ip_summed != CHECKSUM_PARTIAL && !lso_ctrl && !ptp_oss(skb))
 				ctrl |= MACB_BIT(TX_NOCRC);
 		} else
 			/* Only set MSS/MFS on payload descriptors
@@ -2159,9 +2194,10 @@ static int macb_pad_and_fcs(struct sk_buff **skb, struct net_device *ndev)
 	struct sk_buff *nskb;
 	u32 fcs;
 
+	/* Not available for GSO and PTP one step sync */
 	if (!(ndev->features & NETIF_F_HW_CSUM) ||
 	    !((*skb)->ip_summed != CHECKSUM_PARTIAL) ||
-	    skb_shinfo(*skb)->gso_size)	/* Not available for GSO */
+	    skb_shinfo(*skb)->gso_size || ptp_oss(*skb))
 		return 0;
 
 	if (padlen <= 0) {
diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c
index fb6b27f46b15..9559c16078f9 100644
--- a/drivers/net/ethernet/cadence/macb_ptp.c
+++ b/drivers/net/ethernet/cadence/macb_ptp.c
@@ -470,8 +470,10 @@ int gem_set_hwtst(struct net_device *dev, struct ifreq *ifr, int cmd)
 	case HWTSTAMP_TX_ONESTEP_SYNC:
 		if (gem_ptp_set_one_step_sync(bp, 1) != 0)
 			return -ERANGE;
-		fallthrough;
+		tx_bd_control = TSTAMP_ALL_FRAMES;
+		break;
 	case HWTSTAMP_TX_ON:
+		gem_ptp_set_one_step_sync(bp, 0);
 		tx_bd_control = TSTAMP_ALL_FRAMES;
 		break;
 	default:
-- 
2.17.1

[PATCH 2/3] net: macb: Enable PTP unicast

[Harini Katakam]

Enable transmission and reception of PTP unicast packets by
updating PTP unicast config bit and setting current HW mac
address as allowed address in PTP unicast filter registers.

Signed-off-by: Harini Katakam <harini.katakam@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
---
 drivers/net/ethernet/cadence/macb.h      | 4 ++++
 drivers/net/ethernet/cadence/macb_main.c | 7 +++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 7ca077b65eaa..d245fd78ec51 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -95,6 +95,8 @@
 #define GEM_SA4B		0x00A0 /* Specific4 Bottom */
 #define GEM_SA4T		0x00A4 /* Specific4 Top */
 #define GEM_WOL			0x00b8 /* Wake on LAN */
+#define GEM_RXPTPUNI		0x00D4 /* PTP RX Unicast address */
+#define GEM_TXPTPUNI		0x00D8 /* PTP TX Unicast address */
 #define GEM_EFTSH		0x00e8 /* PTP Event Frame Transmitted Seconds Register 47:32 */
 #define GEM_EFRSH		0x00ec /* PTP Event Frame Received Seconds Register 47:32 */
 #define GEM_PEFTSH		0x00f0 /* PTP Peer Event Frame Transmitted Seconds Register 47:32 */
@@ -245,6 +247,8 @@
 #define MACB_TZQ_OFFSET		12 /* Transmit zero quantum pause frame */
 #define MACB_TZQ_SIZE		1
 #define MACB_SRTSM_OFFSET	15 /* Store Receive Timestamp to Memory */
+#define MACB_PTPUNI_OFFSET	20 /* PTP Unicast packet enable */
+#define MACB_PTPUNI_SIZE	1
 #define MACB_OSSMODE_OFFSET	24 /* Enable One Step Synchro Mode */
 #define MACB_OSSMODE_SIZE	1
 #define MACB_MIIONRGMII_OFFSET	28 /* MII Usage on RGMII Interface */
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index e23a03e8badf..19276583811e 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -290,6 +290,9 @@ static void macb_set_hwaddr(struct macb *bp)
 	top = cpu_to_le16(*((u16 *)(bp->dev->dev_addr + 4)));
 	macb_or_gem_writel(bp, SA1T, top);
 
+	gem_writel(bp, RXPTPUNI, bottom);
+	gem_writel(bp, TXPTPUNI, bottom);
+
 	/* Clear unused address register sets */
 	macb_or_gem_writel(bp, SA2B, 0);
 	macb_or_gem_writel(bp, SA2T, 0);
@@ -723,8 +726,8 @@ static void macb_mac_link_up(struct phylink_config *config,
 
 	spin_unlock_irqrestore(&bp->lock, flags);
 
-	/* Enable Rx and Tx */
-	macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(RE) | MACB_BIT(TE));
+	/* Enable Rx and Tx; Enable PTP unicast */
+	macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(RE) | MACB_BIT(TE) | MACB_BIT(PTPUNI));
 
 	netif_tx_wake_all_queues(ndev);
 }
-- 
2.17.1

[PATCH 3/3] net: macb: Optimize reading HW timestamp

[Harini Katakam]

The seconds input from BD (6 bits) just needs to be ORed with the
upper bits from timer in this function. Avoid +/- operations every
single time. Check for seconds rollover at BIT 5 and subtract the
overhead only in that case.

Signed-off-by: Harini Katakam <harini.katakam@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
---
 drivers/net/ethernet/cadence/macb_ptp.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c
index 9559c16078f9..c853518c0406 100644
--- a/drivers/net/ethernet/cadence/macb_ptp.c
+++ b/drivers/net/ethernet/cadence/macb_ptp.c
@@ -247,6 +247,7 @@ static int gem_hw_timestamp(struct macb *bp, u32 dma_desc_ts_1,
 			    u32 dma_desc_ts_2, struct timespec64 *ts)
 {
 	struct timespec64 tsu;
+	bool sec_rollover = false;
 
 	ts->tv_sec = (GEM_BFEXT(DMA_SECH, dma_desc_ts_2) << GEM_DMA_SECL_SIZE) |
 			GEM_BFEXT(DMA_SECL, dma_desc_ts_1);
@@ -264,9 +265,12 @@ static int gem_hw_timestamp(struct macb *bp, u32 dma_desc_ts_1,
 	 */
 	if ((ts->tv_sec & (GEM_DMA_SEC_TOP >> 1)) &&
 	    !(tsu.tv_sec & (GEM_DMA_SEC_TOP >> 1)))
-		ts->tv_sec -= GEM_DMA_SEC_TOP;
+		sec_rollover = true;
+
+	ts->tv_sec |= ((~GEM_DMA_SEC_MASK) & tsu.tv_sec);
 
-	ts->tv_sec += ((~GEM_DMA_SEC_MASK) & tsu.tv_sec);
+	if (sec_rollover)
+		ts->tv_sec -= GEM_DMA_SEC_TOP;
 
 	return 0;
 }
-- 
2.17.1

Re: [PATCH 0/3] Macb PTP updates

[Richard Cochran]

On Tue, May 17, 2022 at 01:02:56PM +0530, Harini Katakam wrote:
> Macb PTP updates to handle PTP one step sync support and other minor
> enhancements.
> 
> Harini Katakam (3):
>   net: macb: Fix PTP one step sync support
>   net: macb: Enable PTP unicast
>   net: macb: Optimize reading HW timestamp
> 
>  drivers/net/ethernet/cadence/macb.h      |  4 ++
>  drivers/net/ethernet/cadence/macb_main.c | 61 +++++++++++++++++++-----
>  drivers/net/ethernet/cadence/macb_ptp.c  | 12 +++--
>  3 files changed, 63 insertions(+), 14 deletions(-)

For the series:

Acked-by: Richard Cochran <richardcochran@gmail.com>

Re: [PATCH 1/3] net: macb: Fix PTP one step sync support

[Jakub Kicinski]

On Tue, 17 May 2022 13:02:57 +0530 Harini Katakam wrote:
> PTP one step sync packets cannot have CSUM padding and insertion in
> SW since time stamp is inserted on the fly by HW.
> In addition, ptp4l version 3.0 and above report an error when skb
> timestamps are reported for packets that not processed for TX TS
> after transmission.
> Add a helper to identify PTP one step sync and fix the above two
> errors.
> Also reset ptp OSS bit when one step is not selected.
> 
> Fixes: ab91f0a9b5f4 ("net: macb: Add hardware PTP support")
> Fixes: 653e92a9175e ("net: macb: add support for padding and fcs computation")

Please make sure to CC authors of the patches under fixes.
./scripts/get_maintainer should point them out.

> Signed-off-by: Harini Katakam <harini.katakam@xilinx.com>
> Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>

If this is a fix it needs to be posted as [PATCH net] separately first
so we can get it into stable as well. The trees get merged together
every Thursday, if you're quick you may still make it this week.
Then after trees get merged you should send send the remaining patches.

https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html

> Notes:
> -> Added the macb pad and fcs fixes tag because strictly speaking the PTP support  
> patch precedes the fcs patch in timeline.
> -> FYI, the error observed with setting HW TX timestamp for one step sync packets:  
> ptp4l[405.292]: port 1: unexpected socket error
> 
>  drivers/net/ethernet/cadence/macb_main.c | 54 ++++++++++++++++++++----
>  drivers/net/ethernet/cadence/macb_ptp.c  |  4 +-
>  2 files changed, 48 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index e993616308f8..e23a03e8badf 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -37,6 +37,7 @@
>  #include <linux/phy/phy.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/reset.h>
> +#include <linux/ptp_classify.h>

Please keep the includes in alphabetical order.

>  #include "macb.h"
>  
>  /* This structure is only used for MACB on SiFive FU540 devices */
> @@ -98,6 +99,9 @@ struct sifive_fu540_macb_mgmt {
>  
>  #define MACB_MDIO_TIMEOUT	1000000 /* in usecs */
>  
> +/* IEEE1588 PTP flag field values  */
> +#define PTP_FLAG_TWOSTEP	0x2

Shouldn't this go into the PTP header?

>  /* DMA buffer descriptor might be different size
>   * depends on hardware configuration:
>   *
> @@ -1122,6 +1126,36 @@ static void macb_tx_error_task(struct work_struct *work)
>  	napi_enable(&queue->napi_tx);
>  }
>  
> +static inline bool ptp_oss(struct sk_buff *skb)

Please spell out then name more oss == open source software.

No inline here, please, let the compiler decide if the function is
small enough. One step timestamp may be a rare use case so inlining
this twice is not necessarily the right choice.

> +{
> +	struct ptp_header *hdr;
> +	unsigned int ptp_class;
> +	u8 msgtype;
> +
> +	/* No need to parse packet if PTP TS is not involved */
> +	if (!(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP))
> +		goto not_oss;
> +
> +	/* Identify and return whether PTP one step sync is being processed */
> +	ptp_class = ptp_classify_raw(skb);
> +	if (ptp_class == PTP_CLASS_NONE)
> +		goto not_oss;
> +
> +	hdr = ptp_parse_header(skb, ptp_class);
> +	if (!hdr)
> +		goto not_oss;
> +
> +	if (hdr->flag_field[0] & PTP_FLAG_TWOSTEP)
> +		goto not_oss;
> +
> +	msgtype = ptp_get_msgtype(hdr, ptp_class);
> +	if (msgtype == PTP_MSGTYPE_SYNC)
> +		return true;
> +
> +not_oss:
> +	return false;
> +}
> +
>  static int macb_tx_complete(struct macb_queue *queue, int budget)
>  {
>  	struct macb *bp = queue->bp;
> @@ -1158,13 +1192,14 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
>  
>  			/* First, update TX stats if needed */
>  			if (skb) {
> -				if (unlikely(skb_shinfo(skb)->tx_flags &
> -					     SKBTX_HW_TSTAMP) &&
> -				    gem_ptp_do_txstamp(queue, skb, desc) == 0) {
> -					/* skb now belongs to timestamp buffer
> -					 * and will be removed later
> -					 */
> -					tx_skb->skb = NULL;
> +				if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&

ptp_oss already checks if HW_TSTAMP is set.

> +				    !ptp_oss(skb)) {
> +					if (gem_ptp_do_txstamp(queue, skb, desc) == 0) {

Why convert the gem_ptp_do_txstamp check from a && in the condition to
a separate if?

> +						/* skb now belongs to timestamp buffer
> +						 * and will be removed later
> +						 */
> +						tx_skb->skb = NULL;
> +					}
>  				}
>  				netdev_vdbg(bp->dev, "skb %u (data %p) TX complete\n",
>  					    macb_tx_ring_wrap(bp, tail),
> @@ -2063,7 +2098,7 @@ static unsigned int macb_tx_map(struct macb *bp,
>  			ctrl |= MACB_BF(TX_LSO, lso_ctrl);
>  			ctrl |= MACB_BF(TX_TCP_SEQ_SRC, seq_ctrl);
>  			if ((bp->dev->features & NETIF_F_HW_CSUM) &&
> -			    skb->ip_summed != CHECKSUM_PARTIAL && !lso_ctrl)
> +			    skb->ip_summed != CHECKSUM_PARTIAL && !lso_ctrl && !ptp_oss(skb))
>  				ctrl |= MACB_BIT(TX_NOCRC);
>  		} else
>  			/* Only set MSS/MFS on payload descriptors
> @@ -2159,9 +2194,10 @@ static int macb_pad_and_fcs(struct sk_buff **skb, struct net_device *ndev)
>  	struct sk_buff *nskb;
>  	u32 fcs;
>  
> +	/* Not available for GSO and PTP one step sync */

If the functions are named right this comment should not be needed.

>  	if (!(ndev->features & NETIF_F_HW_CSUM) ||
>  	    !((*skb)->ip_summed != CHECKSUM_PARTIAL) ||
> -	    skb_shinfo(*skb)->gso_size)	/* Not available for GSO */
> +	    skb_shinfo(*skb)->gso_size || ptp_oss(*skb))
>  		return 0;
>  
>  	if (padlen <= 0) {
> diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c
> index fb6b27f46b15..9559c16078f9 100644
> --- a/drivers/net/ethernet/cadence/macb_ptp.c
> +++ b/drivers/net/ethernet/cadence/macb_ptp.c
> @@ -470,8 +470,10 @@ int gem_set_hwtst(struct net_device *dev, struct ifreq *ifr, int cmd)
>  	case HWTSTAMP_TX_ONESTEP_SYNC:
>  		if (gem_ptp_set_one_step_sync(bp, 1) != 0)
>  			return -ERANGE;
> -		fallthrough;
> +		tx_bd_control = TSTAMP_ALL_FRAMES;
> +		break;
>  	case HWTSTAMP_TX_ON:
> +		gem_ptp_set_one_step_sync(bp, 0);
>  		tx_bd_control = TSTAMP_ALL_FRAMES;
>  		break;
>  	default:

Re: [PATCH 1/3] net: macb: Fix PTP one step sync support

[Harini Katakam]

Hi Jakub,

On Wed, May 18, 2022 at 8:12 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 17 May 2022 13:02:57 +0530 Harini Katakam wrote:
> > PTP one step sync packets cannot have CSUM padding and insertion in
> > SW since time stamp is inserted on the fly by HW.
> > In addition, ptp4l version 3.0 and above report an error when skb
> > timestamps are reported for packets that not processed for TX TS
> > after transmission.
> > Add a helper to identify PTP one step sync and fix the above two
> > errors.
> > Also reset ptp OSS bit when one step is not selected.
> >
> > Fixes: ab91f0a9b5f4 ("net: macb: Add hardware PTP support")
> > Fixes: 653e92a9175e ("net: macb: add support for padding and fcs computation")
>
> Please make sure to CC authors of the patches under fixes.
> ./scripts/get_maintainer should point them out.

Thanks for the review.
Rafal Ozieblo <rafalo@cadence.com> is the author of the first Fixes
patch but that
address hasn't worked in the last ~4 yrs.
I have cced Claudiu and everyone else from the maintainers
(Eric Dumazet <edumazet@google.com> also doesn't work)

<snip>
> > +/* IEEE1588 PTP flag field values  */
> > +#define PTP_FLAG_TWOSTEP     0x2
>
> Shouldn't this go into the PTP header?

Let me add it to ptp_classify where the relevant helpers are present.

<snip>
> > +static inline bool ptp_oss(struct sk_buff *skb)
>
> Please spell out then name more oss == open source software.

Will change to ptp_one_step_sync

>
> No inline here, please, let the compiler decide if the function is
> small enough. One step timestamp may be a rare use case so inlining
> this twice is not necessarily the right choice.

One step is a rare case but the check happens on every PTP packet in the
transmit data path and hence I wanted to explicitly inline it.

<snip>
> > @@ -1158,13 +1192,14 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
> >
> >                       /* First, update TX stats if needed */
> >                       if (skb) {
> > -                             if (unlikely(skb_shinfo(skb)->tx_flags &
> > -                                          SKBTX_HW_TSTAMP) &&
> > -                                 gem_ptp_do_txstamp(queue, skb, desc) == 0) {
> > -                                     /* skb now belongs to timestamp buffer
> > -                                      * and will be removed later
> > -                                      */
> > -                                     tx_skb->skb = NULL;
> > +                             if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
>
> ptp_oss already checks if HW_TSTAMP is set.

The check for SKBTX_HW_TSTAMP is required here universally and not
just inside ptp_oss.
I will remove the redundant check in ptp_oss instead. Please see the
reply below.

>
> > +                                 !ptp_oss(skb)) {
> > +                                     if (gem_ptp_do_txstamp(queue, skb, desc) == 0) {
>
> Why convert the gem_ptp_do_txstamp check from a && in the condition to
> a separate if?

The intention is that ptp_oss should only be evaluated when
SKBTX_HW_TSTAMP is set and
gem_ptp_do_txstamp should only be called if ptp_oss is false. Since
compiler follows the order
of evaluation, I'll simplify this to:

if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) && !ptp_oss(skb) &&
    gem_ptp_do_txstamp(queue, skb, desc) == 0) {
...
}

Regards,
Harini

Re: [PATCH 1/3] net: macb: Fix PTP one step sync support

[Jakub Kicinski]

On Wed, 18 May 2022 09:53:29 +0530 Harini Katakam wrote:
> On Wed, May 18, 2022 at 8:12 AM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Tue, 17 May 2022 13:02:57 +0530 Harini Katakam wrote:  
> > > PTP one step sync packets cannot have CSUM padding and insertion in
> > > SW since time stamp is inserted on the fly by HW.
> > > In addition, ptp4l version 3.0 and above report an error when skb
> > > timestamps are reported for packets that not processed for TX TS
> > > after transmission.
> > > Add a helper to identify PTP one step sync and fix the above two
> > > errors.
> > > Also reset ptp OSS bit when one step is not selected.
> > >
> > > Fixes: ab91f0a9b5f4 ("net: macb: Add hardware PTP support")
> > > Fixes: 653e92a9175e ("net: macb: add support for padding and fcs computation")  
> >
> > Please make sure to CC authors of the patches under fixes.
> > ./scripts/get_maintainer should point them out.  
> 
> Thanks for the review.
> Rafal Ozieblo <rafalo@cadence.com> is the author of the first Fixes
> patch but that
> address hasn't worked in the last ~4 yrs.
> I have cced Claudiu and everyone else from the maintainers
> (Eric Dumazet <edumazet@google.com> also doesn't work)

I see, thanks, added Rafal's email to the ignore list, 
I'm quite sure Eric's email address works.

> > > @@ -1158,13 +1192,14 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
> > >
> > >                       /* First, update TX stats if needed */
> > >                       if (skb) {
> > > -                             if (unlikely(skb_shinfo(skb)->tx_flags &
> > > -                                          SKBTX_HW_TSTAMP) &&
> > > -                                 gem_ptp_do_txstamp(queue, skb, desc) == 0) {
> > > -                                     /* skb now belongs to timestamp buffer
> > > -                                      * and will be removed later
> > > -                                      */
> > > -                                     tx_skb->skb = NULL;
> > > +                             if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&  
> >
> > ptp_oss already checks if HW_TSTAMP is set.  
> 
> The check for SKBTX_HW_TSTAMP is required here universally and not
> just inside ptp_oss.
> I will remove the redundant check in ptp_oss instead. Please see the
> reply below.

But then you need to add this check in the padding/fcs call site and
the place where NOCRC is set. If you wrap the check for SKBTX_HW_TSTAMP
in the helper with likely() and remove the inline - will the compiler
not split the function and inline just that check? And leave the rest
as a functionname.part... thing?

Re: [PATCH 1/3] net: macb: Fix PTP one step sync support

[Harini Katakam]

Hi Jakub,

<snip>
>
> > > > @@ -1158,13 +1192,14 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
> > > >
> > > >                       /* First, update TX stats if needed */
> > > >                       if (skb) {
> > > > -                             if (unlikely(skb_shinfo(skb)->tx_flags &
> > > > -                                          SKBTX_HW_TSTAMP) &&
> > > > -                                 gem_ptp_do_txstamp(queue, skb, desc) == 0) {
> > > > -                                     /* skb now belongs to timestamp buffer
> > > > -                                      * and will be removed later
> > > > -                                      */
> > > > -                                     tx_skb->skb = NULL;
> > > > +                             if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
> > >
> > > ptp_oss already checks if HW_TSTAMP is set.
> >
> > The check for SKBTX_HW_TSTAMP is required here universally and not
> > just inside ptp_oss.
> > I will remove the redundant check in ptp_oss instead. Please see the
> > reply below.
>
> But then you need to add this check in the padding/fcs call site and
> the place where NOCRC is set. If you wrap the check for SKBTX_HW_TSTAMP
> in the helper with likely() and remove the inline - will the compiler
> not split the function and inline just that check? And leave the rest
> as a functionname.part... thing?

Yes, I checked the disassembly and this is what's happening. This
should be good for
the non-PTP packet (going to "likely" branch) and the rest of ptp_oss
is evaluated for
PTP packets.

Regards,
Harini

Re: [PATCH 2/3] net: macb: Enable PTP unicast

[Claudiu.Beznea]

Hi, Harini,

On 17.05.2022 10:32, Harini Katakam wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Enable transmission and reception of PTP unicast packets by
> updating PTP unicast config bit and setting current HW mac
> address as allowed address in PTP unicast filter registers.
> 
> Signed-off-by: Harini Katakam <harini.katakam@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
> ---
>  drivers/net/ethernet/cadence/macb.h      | 4 ++++
>  drivers/net/ethernet/cadence/macb_main.c | 7 +++++--
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 7ca077b65eaa..d245fd78ec51 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -95,6 +95,8 @@
>  #define GEM_SA4B               0x00A0 /* Specific4 Bottom */
>  #define GEM_SA4T               0x00A4 /* Specific4 Top */
>  #define GEM_WOL                        0x00b8 /* Wake on LAN */
> +#define GEM_RXPTPUNI           0x00D4 /* PTP RX Unicast address */
> +#define GEM_TXPTPUNI           0x00D8 /* PTP TX Unicast address */
>  #define GEM_EFTSH              0x00e8 /* PTP Event Frame Transmitted Seconds Register 47:32 */
>  #define GEM_EFRSH              0x00ec /* PTP Event Frame Received Seconds Register 47:32 */
>  #define GEM_PEFTSH             0x00f0 /* PTP Peer Event Frame Transmitted Seconds Register 47:32 */
> @@ -245,6 +247,8 @@
>  #define MACB_TZQ_OFFSET                12 /* Transmit zero quantum pause frame */
>  #define MACB_TZQ_SIZE          1
>  #define MACB_SRTSM_OFFSET      15 /* Store Receive Timestamp to Memory */
> +#define MACB_PTPUNI_OFFSET     20 /* PTP Unicast packet enable */
> +#define MACB_PTPUNI_SIZE       1
>  #define MACB_OSSMODE_OFFSET    24 /* Enable One Step Synchro Mode */
>  #define MACB_OSSMODE_SIZE      1
>  #define MACB_MIIONRGMII_OFFSET 28 /* MII Usage on RGMII Interface */
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index e23a03e8badf..19276583811e 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -290,6 +290,9 @@ static void macb_set_hwaddr(struct macb *bp)
>         top = cpu_to_le16(*((u16 *)(bp->dev->dev_addr + 4)));
>         macb_or_gem_writel(bp, SA1T, top);
> 
> +       gem_writel(bp, RXPTPUNI, bottom);
> +       gem_writel(bp, TXPTPUNI, bottom);

Please call these only if gem_has_ptp() returns true as macb_set_hwaddr()
is called either on emac, gem variants or gem variants w/o ptp support or
w/o ptp support enabled.

> +
>         /* Clear unused address register sets */
>         macb_or_gem_writel(bp, SA2B, 0);
>         macb_or_gem_writel(bp, SA2T, 0);
> @@ -723,8 +726,8 @@ static void macb_mac_link_up(struct phylink_config *config,
> 
>         spin_unlock_irqrestore(&bp->lock, flags);
> 
> -       /* Enable Rx and Tx */
> -       macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(RE) | MACB_BIT(TE));
> +       /* Enable Rx and Tx; Enable PTP unicast */
> +       macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(RE) | MACB_BIT(TE) | MACB_BIT(PTPUNI));

Same here.

> 
>         netif_tx_wake_all_queues(ndev);
>  }
> --
> 2.17.1
>