[RESEND PATCH 0/2] Macb PTP enhancements

[RESEND PATCH 0/2] Macb PTP enhancements

[Harini Katakam]

This series is a follow up for patches 2 and 3 from a previous series:
https://lore.kernel.org/all/ca4c97c9-1117-a465-5202-e1bf276fe75b@microchip.com/
https://lore.kernel.org/all/20220517135525.GC3344@hoboy.vegasvil.org/
Sorry for the delay.

ACK is added only to patch 3 (now patch 2).
Patch 1 is updated with check for gem_has_ptp as per Claudiu's comments.

Resending as net-next was closed when this series was sent a few weeks ago.

Harini Katakam (2):
  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 | 13 ++++++++++++-
 drivers/net/ethernet/cadence/macb_ptp.c  |  8 ++++++--
 3 files changed, 22 insertions(+), 3 deletions(-)

-- 
2.17.1

[RESEND PATCH 1/2] net: macb: Enable PTP unicast

[Harini Katakam]

From: Harini Katakam <harini.katakam@xilinx.com>

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>
---
Added check for gem_has_ptp as per Claudiu's comments.

 drivers/net/ethernet/cadence/macb.h      |  4 ++++
 drivers/net/ethernet/cadence/macb_main.c | 13 ++++++++++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 9c410f93a103..1aa578c1ca4a 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 494fe961a49d..4699699a1593 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -287,6 +287,13 @@ 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);
 
+#ifdef CONFIG_MACB_USE_HWSTAMP
+	if (gem_has_ptp(bp)) {
+		gem_writel(bp, RXPTPUNI, bottom);
+		gem_writel(bp, TXPTPUNI, bottom);
+	}
+#endif
+
 	/* Clear unused address register sets */
 	macb_or_gem_writel(bp, SA2B, 0);
 	macb_or_gem_writel(bp, SA2T, 0);
@@ -720,7 +727,11 @@ static void macb_mac_link_up(struct phylink_config *config,
 
 	spin_unlock_irqrestore(&bp->lock, flags);
 
-	/* Enable Rx and Tx */
+	/* Enable Rx and Tx; Enable PTP unicast */
+#ifdef CONFIG_MACB_USE_HWSTAMP
+	if (gem_has_ptp(bp))
+		macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(PTPUNI));
+#endif
 	macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(RE) | MACB_BIT(TE));
 
 	netif_tx_wake_all_queues(ndev);
-- 
2.17.1

[RESEND PATCH 2/2] net: macb: Optimize reading HW timestamp

[Harini Katakam]

From: Harini Katakam <harini.katakam@xilinx.com>

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>
Acked-by: Richard Cochran <richardcochran@gmail.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 e6cb20aaa76a..674002661366 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: [RESEND PATCH 2/2] net: macb: Optimize reading HW timestamp

[Claudiu.Beznea]

On 16.08.2022 14:55, Harini Katakam wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> From: Harini Katakam <harini.katakam@xilinx.com>
> 
> 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>
> Acked-by: Richard Cochran <richardcochran@gmail.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 e6cb20aaa76a..674002661366 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);

It looks to me that this instruction could be moved before

>         if ((ts->tv_sec & (GEM_DMA_SEC_TOP >> 1)) &&
>             !(tsu.tv_sec & (GEM_DMA_SEC_TOP >> 1)))

and get rid of sec_rolover extra variable thus, in the end, the diff could
look like this:

diff --git a/drivers/net/ethernet/cadence/macb_ptp.c
b/drivers/net/ethernet/cadence/macb_ptp.c
index e6cb20aaa76a..f9db4501b995 100644
--- a/drivers/net/ethernet/cadence/macb_ptp.c
+++ b/drivers/net/ethernet/cadence/macb_ptp.c
@@ -258,6 +258,8 @@ static int gem_hw_timestamp(struct macb *bp, u32
dma_desc_ts_1,
         */
        gem_tsu_get_time(&bp->ptp_clock_info, &tsu, NULL);

+       ts->tv_sec |= ((~GEM_DMA_SEC_MASK) & tsu.tv_sec);
+
        /* If the top bit is set in the timestamp,
         * but not in 1588 timer, it has rolled over,
         * so subtract max size
@@ -266,8 +268,6 @@ static int gem_hw_timestamp(struct macb *bp, u32
dma_desc_ts_1,
            !(tsu.tv_sec & (GEM_DMA_SEC_TOP >> 1)))
                ts->tv_sec -= GEM_DMA_SEC_TOP;

-       ts->tv_sec += ((~GEM_DMA_SEC_MASK) & tsu.tv_sec);
-
        return 0;
 }

> 
> -       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: [RESEND PATCH 1/2] net: macb: Enable PTP unicast

[Claudiu.Beznea]

On 16.08.2022 14:54, Harini Katakam wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> From: Harini Katakam <harini.katakam@xilinx.com>
> 
> 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>
> ---
> Added check for gem_has_ptp as per Claudiu's comments.
> 
>  drivers/net/ethernet/cadence/macb.h      |  4 ++++
>  drivers/net/ethernet/cadence/macb_main.c | 13 ++++++++++++-
>  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 9c410f93a103..1aa578c1ca4a 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 494fe961a49d..4699699a1593 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -287,6 +287,13 @@ 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);
> 
> +#ifdef CONFIG_MACB_USE_HWSTAMP
> +       if (gem_has_ptp(bp)) {
> +               gem_writel(bp, RXPTPUNI, bottom);
> +               gem_writel(bp, TXPTPUNI, bottom);
> +       }
> +#endif
> +
>         /* Clear unused address register sets */
>         macb_or_gem_writel(bp, SA2B, 0);
>         macb_or_gem_writel(bp, SA2T, 0);
> @@ -720,7 +727,11 @@ static void macb_mac_link_up(struct phylink_config *config,
> 
>         spin_unlock_irqrestore(&bp->lock, flags);
> 
> -       /* Enable Rx and Tx */
> +       /* Enable Rx and Tx; Enable PTP unicast */
> +#ifdef CONFIG_MACB_USE_HWSTAMP
> +       if (gem_has_ptp(bp))
> +               macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(PTPUNI));
> +#endif
>         macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(RE) | MACB_BIT(TE));

Can you use a single read and single write here? something like:

	/* Enable Rx and Tx; Enable PTP unicast */
	ctrl = macb_readl(bp, NCR);
	if (IS_ENABLED(CONFIG_MACB_USE_HWSTAMP) && gem_has_ptp(bp))
		ctrl |= MACB_BIT(PTPUNI);
	macb_writel(bp, NCR, ctrl | MACB_BIT(RE) | MACB_BIT(TE));

Thank you,
Claudiu Beznea

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