[PATCH net-next] net: stmmac: xgmac3+: add FPE handshaking support

[PATCH net-next] net: stmmac: xgmac3+: add FPE handshaking support

[Jianheng Zhang]

Adds the HW specific support for Frame Preemption handshaking on XGMAC3+
cores.

Signed-off-by: Jianheng Zhang <Jianheng.Zhang@synopsys.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h     |  6 ++
 .../net/ethernet/stmicro/stmmac/dwxgmac2_core.c    | 65 ++++++++++++++++++----
 2 files changed, 60 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
index 207ff17..306d15b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
+++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
@@ -194,6 +194,12 @@
 #define XGMAC_MDIO_DATA			0x00000204
 #define XGMAC_MDIO_C22P			0x00000220
 #define XGMAC_FPE_CTRL_STS		0x00000280
+#define XGMAC_TRSP			BIT(19)
+#define XGMAC_TVER			BIT(18)
+#define XGMAC_RRSP			BIT(17)
+#define XGMAC_RVER			BIT(16)
+#define XGMAC_SRSP			BIT(2)
+#define XGMAC_SVER			BIT(1)
 #define XGMAC_EFPE			BIT(0)
 #define XGMAC_ADDRx_HIGH(x)		(0x00000300 + (x) * 0x8)
 #define XGMAC_ADDR_MAX			32
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
index eb48211..091d932 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
@@ -1439,22 +1439,63 @@ static void dwxgmac3_fpe_configure(void __iomem *ioaddr, struct stmmac_fpe_cfg *
 {
 	u32 value;
 
-	if (!enable) {
-		value = readl(ioaddr + XGMAC_FPE_CTRL_STS);
+	if (enable) {
+		cfg->fpe_csr = XGMAC_EFPE;
+		value = readl(ioaddr + XGMAC_RXQ_CTRL1);
+		value &= ~XGMAC_RQ;
+		value |= (num_rxq - 1) << XGMAC_RQ_SHIFT;
+		writel(value, ioaddr + XGMAC_RXQ_CTRL1);
+	} else {
+		cfg->fpe_csr = 0;
+	}
+	writel(cfg->fpe_csr, ioaddr + XGMAC_FPE_CTRL_STS);
+}
 
-		value &= ~XGMAC_EFPE;
+static int dwxgmac3_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev)
+{
+	u32 value;
+	int status;
 
-		writel(value, ioaddr + XGMAC_FPE_CTRL_STS);
-		return;
+	status = FPE_EVENT_UNKNOWN;
+
+	/* Reads from the XGMAC_FPE_CTRL_STS register should only be performed
+	 * here, since the status flags of MAC_FPE_CTRL_STS are "clear on read"
+	 */
+	value = readl(ioaddr + XGMAC_FPE_CTRL_STS);
+
+	if (value & XGMAC_TRSP) {
+		status |= FPE_EVENT_TRSP;
+		netdev_info(dev, "FPE: Respond mPacket is transmitted\n");
 	}
 
-	value = readl(ioaddr + XGMAC_RXQ_CTRL1);
-	value &= ~XGMAC_RQ;
-	value |= (num_rxq - 1) << XGMAC_RQ_SHIFT;
-	writel(value, ioaddr + XGMAC_RXQ_CTRL1);
+	if (value & XGMAC_TVER) {
+		status |= FPE_EVENT_TVER;
+		netdev_info(dev, "FPE: Verify mPacket is transmitted\n");
+	}
+
+	if (value & XGMAC_RRSP) {
+		status |= FPE_EVENT_RRSP;
+		netdev_info(dev, "FPE: Respond mPacket is received\n");
+	}
+
+	if (value & XGMAC_RVER) {
+		status |= FPE_EVENT_RVER;
+		netdev_info(dev, "FPE: Verify mPacket is received\n");
+	}
+
+	return status;
+}
+
+static void dwxgmac3_fpe_send_mpacket(void __iomem *ioaddr, struct stmmac_fpe_cfg *cfg,
+				      enum stmmac_mpacket_type type)
+{
+	u32 value = cfg->fpe_csr;
+
+	if (type == MPACKET_VERIFY)
+		value |= XGMAC_SVER;
+	else if (type == MPACKET_RESPONSE)
+		value |= XGMAC_SRSP;
 
-	value = readl(ioaddr + XGMAC_FPE_CTRL_STS);
-	value |= XGMAC_EFPE;
 	writel(value, ioaddr + XGMAC_FPE_CTRL_STS);
 }
 
@@ -1503,6 +1544,8 @@ static void dwxgmac3_fpe_configure(void __iomem *ioaddr, struct stmmac_fpe_cfg *
 	.config_l4_filter = dwxgmac2_config_l4_filter,
 	.set_arp_offload = dwxgmac2_set_arp_offload,
 	.fpe_configure = dwxgmac3_fpe_configure,
+	.fpe_send_mpacket = dwxgmac3_fpe_send_mpacket,
+	.fpe_irq_status = dwxgmac3_fpe_irq_status,
 };
 
 static void dwxlgmac2_rx_queue_enable(struct mac_device_info *hw, u8 mode,
-- 
1.8.3.1

[PATCH net-next] net: stmmac: xgmac3+: add FPE handshaking support

[Jianheng Zhang]

Adds the HW specific support for Frame Preemption handshaking on XGMAC3+
cores.

Signed-off-by: Jianheng Zhang <Jianheng.Zhang@synopsys.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h     |  6 ++
 .../net/ethernet/stmicro/stmmac/dwxgmac2_core.c    | 65 ++++++++++++++++++----
 2 files changed, 60 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
index 207ff17..306d15b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
+++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
@@ -194,6 +194,12 @@
 #define XGMAC_MDIO_DATA			0x00000204
 #define XGMAC_MDIO_C22P			0x00000220
 #define XGMAC_FPE_CTRL_STS		0x00000280
+#define XGMAC_TRSP			BIT(19)
+#define XGMAC_TVER			BIT(18)
+#define XGMAC_RRSP			BIT(17)
+#define XGMAC_RVER			BIT(16)
+#define XGMAC_SRSP			BIT(2)
+#define XGMAC_SVER			BIT(1)
 #define XGMAC_EFPE			BIT(0)
 #define XGMAC_ADDRx_HIGH(x)		(0x00000300 + (x) * 0x8)
 #define XGMAC_ADDR_MAX			32
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
index eb48211..091d932 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
@@ -1439,22 +1439,63 @@ static void dwxgmac3_fpe_configure(void __iomem *ioaddr, struct stmmac_fpe_cfg *
 {
 	u32 value;
 
-	if (!enable) {
-		value = readl(ioaddr + XGMAC_FPE_CTRL_STS);
+	if (enable) {
+		cfg->fpe_csr = XGMAC_EFPE;
+		value = readl(ioaddr + XGMAC_RXQ_CTRL1);
+		value &= ~XGMAC_RQ;
+		value |= (num_rxq - 1) << XGMAC_RQ_SHIFT;
+		writel(value, ioaddr + XGMAC_RXQ_CTRL1);
+	} else {
+		cfg->fpe_csr = 0;
+	}
+	writel(cfg->fpe_csr, ioaddr + XGMAC_FPE_CTRL_STS);
+}
 
-		value &= ~XGMAC_EFPE;
+static int dwxgmac3_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev)
+{
+	u32 value;
+	int status;
 
-		writel(value, ioaddr + XGMAC_FPE_CTRL_STS);
-		return;
+	status = FPE_EVENT_UNKNOWN;
+
+	/* Reads from the XGMAC_FPE_CTRL_STS register should only be performed
+	 * here, since the status flags of MAC_FPE_CTRL_STS are "clear on read"
+	 */
+	value = readl(ioaddr + XGMAC_FPE_CTRL_STS);
+
+	if (value & XGMAC_TRSP) {
+		status |= FPE_EVENT_TRSP;
+		netdev_info(dev, "FPE: Respond mPacket is transmitted\n");
 	}
 
-	value = readl(ioaddr + XGMAC_RXQ_CTRL1);
-	value &= ~XGMAC_RQ;
-	value |= (num_rxq - 1) << XGMAC_RQ_SHIFT;
-	writel(value, ioaddr + XGMAC_RXQ_CTRL1);
+	if (value & XGMAC_TVER) {
+		status |= FPE_EVENT_TVER;
+		netdev_info(dev, "FPE: Verify mPacket is transmitted\n");
+	}
+
+	if (value & XGMAC_RRSP) {
+		status |= FPE_EVENT_RRSP;
+		netdev_info(dev, "FPE: Respond mPacket is received\n");
+	}
+
+	if (value & XGMAC_RVER) {
+		status |= FPE_EVENT_RVER;
+		netdev_info(dev, "FPE: Verify mPacket is received\n");
+	}
+
+	return status;
+}
+
+static void dwxgmac3_fpe_send_mpacket(void __iomem *ioaddr, struct stmmac_fpe_cfg *cfg,
+				      enum stmmac_mpacket_type type)
+{
+	u32 value = cfg->fpe_csr;
+
+	if (type == MPACKET_VERIFY)
+		value |= XGMAC_SVER;
+	else if (type == MPACKET_RESPONSE)
+		value |= XGMAC_SRSP;
 
-	value = readl(ioaddr + XGMAC_FPE_CTRL_STS);
-	value |= XGMAC_EFPE;
 	writel(value, ioaddr + XGMAC_FPE_CTRL_STS);
 }
 
@@ -1503,6 +1544,8 @@ static void dwxgmac3_fpe_configure(void __iomem *ioaddr, struct stmmac_fpe_cfg *
 	.config_l4_filter = dwxgmac2_config_l4_filter,
 	.set_arp_offload = dwxgmac2_set_arp_offload,
 	.fpe_configure = dwxgmac3_fpe_configure,
+	.fpe_send_mpacket = dwxgmac3_fpe_send_mpacket,
+	.fpe_irq_status = dwxgmac3_fpe_irq_status,
 };
 
 static void dwxlgmac2_rx_queue_enable(struct mac_device_info *hw, u8 mode,
-- 
1.8.3.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: stmmac: xgmac3+: add FPE handshaking support

[Serge Semin]

[-- Attachment #1: Type: text/plain, Size: 4855 bytes --]

Hi Jianheng, Jakub

On Mon, Dec 11, 2023 at 06:13:21AM +0000, Jianheng Zhang wrote:
> Adds the HW specific support for Frame Preemption handshaking on XGMAC3+
> cores.

Thanks for the patch. No objection about the change:
Reviewed-by: Serge Semin <fancer.lancer@gmail.com>

The only note is that the DW XGMAC v3.x and DW QoS Eth v5.x FPE
implementations are now identical (see the attached diff). What about
factoring out the common parts to a separate file - stmmac_fpe.c
(perhaps together with the handshaking algo from the stmmac_main.c)
and send it out as an additional patch on top of this one? A similar
thing has been recently done for EST:
https://lore.kernel.org/netdev/20231201055252.1302-1-rohan.g.thomas@intel.com/
Although in this case AFAICS the implementation is simpler and the
only difference is in the CSR offset and the frame preemption residue
queue ID setting. All of that can be easily solved in the same way as
it was done for EST (see the link above).

Jakub, what do you think?

-Serge(y)

> 
> Signed-off-by: Jianheng Zhang <Jianheng.Zhang@synopsys.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h     |  6 ++
>  .../net/ethernet/stmicro/stmmac/dwxgmac2_core.c    | 65 ++++++++++++++++++----
>  2 files changed, 60 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> index 207ff17..306d15b 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> @@ -194,6 +194,12 @@
>  #define XGMAC_MDIO_DATA			0x00000204
>  #define XGMAC_MDIO_C22P			0x00000220
>  #define XGMAC_FPE_CTRL_STS		0x00000280
> +#define XGMAC_TRSP			BIT(19)
> +#define XGMAC_TVER			BIT(18)
> +#define XGMAC_RRSP			BIT(17)
> +#define XGMAC_RVER			BIT(16)
> +#define XGMAC_SRSP			BIT(2)
> +#define XGMAC_SVER			BIT(1)
>  #define XGMAC_EFPE			BIT(0)
>  #define XGMAC_ADDRx_HIGH(x)		(0x00000300 + (x) * 0x8)
>  #define XGMAC_ADDR_MAX			32
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> index eb48211..091d932 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> @@ -1439,22 +1439,63 @@ static void dwxgmac3_fpe_configure(void __iomem *ioaddr, struct stmmac_fpe_cfg *
>  {
>  	u32 value;
>  
> -	if (!enable) {
> -		value = readl(ioaddr + XGMAC_FPE_CTRL_STS);
> +	if (enable) {
> +		cfg->fpe_csr = XGMAC_EFPE;
> +		value = readl(ioaddr + XGMAC_RXQ_CTRL1);
> +		value &= ~XGMAC_RQ;
> +		value |= (num_rxq - 1) << XGMAC_RQ_SHIFT;
> +		writel(value, ioaddr + XGMAC_RXQ_CTRL1);
> +	} else {
> +		cfg->fpe_csr = 0;
> +	}
> +	writel(cfg->fpe_csr, ioaddr + XGMAC_FPE_CTRL_STS);
> +}
>  
> -		value &= ~XGMAC_EFPE;
> +static int dwxgmac3_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev)
> +{
> +	u32 value;
> +	int status;
>  
> -		writel(value, ioaddr + XGMAC_FPE_CTRL_STS);
> -		return;
> +	status = FPE_EVENT_UNKNOWN;
> +
> +	/* Reads from the XGMAC_FPE_CTRL_STS register should only be performed
> +	 * here, since the status flags of MAC_FPE_CTRL_STS are "clear on read"
> +	 */
> +	value = readl(ioaddr + XGMAC_FPE_CTRL_STS);
> +
> +	if (value & XGMAC_TRSP) {
> +		status |= FPE_EVENT_TRSP;
> +		netdev_info(dev, "FPE: Respond mPacket is transmitted\n");
>  	}
>  
> -	value = readl(ioaddr + XGMAC_RXQ_CTRL1);
> -	value &= ~XGMAC_RQ;
> -	value |= (num_rxq - 1) << XGMAC_RQ_SHIFT;
> -	writel(value, ioaddr + XGMAC_RXQ_CTRL1);
> +	if (value & XGMAC_TVER) {
> +		status |= FPE_EVENT_TVER;
> +		netdev_info(dev, "FPE: Verify mPacket is transmitted\n");
> +	}
> +
> +	if (value & XGMAC_RRSP) {
> +		status |= FPE_EVENT_RRSP;
> +		netdev_info(dev, "FPE: Respond mPacket is received\n");
> +	}
> +
> +	if (value & XGMAC_RVER) {
> +		status |= FPE_EVENT_RVER;
> +		netdev_info(dev, "FPE: Verify mPacket is received\n");
> +	}
> +
> +	return status;
> +}
> +
> +static void dwxgmac3_fpe_send_mpacket(void __iomem *ioaddr, struct stmmac_fpe_cfg *cfg,
> +				      enum stmmac_mpacket_type type)
> +{
> +	u32 value = cfg->fpe_csr;
> +
> +	if (type == MPACKET_VERIFY)
> +		value |= XGMAC_SVER;
> +	else if (type == MPACKET_RESPONSE)
> +		value |= XGMAC_SRSP;
>  
> -	value = readl(ioaddr + XGMAC_FPE_CTRL_STS);
> -	value |= XGMAC_EFPE;
>  	writel(value, ioaddr + XGMAC_FPE_CTRL_STS);
>  }
>  
> @@ -1503,6 +1544,8 @@ static void dwxgmac3_fpe_configure(void __iomem *ioaddr, struct stmmac_fpe_cfg *
>  	.config_l4_filter = dwxgmac2_config_l4_filter,
>  	.set_arp_offload = dwxgmac2_set_arp_offload,
>  	.fpe_configure = dwxgmac3_fpe_configure,
> +	.fpe_send_mpacket = dwxgmac3_fpe_send_mpacket,
> +	.fpe_irq_status = dwxgmac3_fpe_irq_status,
>  };
>  
>  static void dwxlgmac2_rx_queue_enable(struct mac_device_info *hw, u8 mode,
> -- 
> 1.8.3.1
> 
> 

[-- Attachment #2: stmmac_fpe.diff --]
[-- Type: text/x-patch, Size: 3544 bytes --]

--- drivers/net/ethernet/stmicro/stmmac/dwmac5_fpe.c	2023-12-11 14:01:26.888400348 +0300
+++ drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core_fpe.c	2023-12-11 14:01:49.538644889 +0300
@@ -1,64 +1,64 @@
-#define MAC_FPE_CTRL_STS		0x00000234
-#define TRSP				BIT(19)
-#define TVER				BIT(18)
-#define RRSP				BIT(17)
-#define RVER				BIT(16)
-#define SRSP				BIT(2)
-#define SVER				BIT(1)
-#define EFPE				BIT(0)
-
-#define GMAC_RXQ_CTRL0			0x000000a0
-#define GMAC_RXQ_CTRL1			0x000000a4
-
-#define GMAC_RXQCTRL_FPRQ		GENMASK(26, 24)
-#define GMAC_RXQCTRL_FPRQ_SHIFT		24
-
-void dwmac5_fpe_configure(void __iomem *ioaddr, struct stmmac_fpe_cfg *cfg,
-			  u32 num_txq, u32 num_rxq,
-			  bool enable)
+#define XGMAC_FPE_CTRL_STS		0x00000280
+#define XGMAC_TRSP			BIT(19)
+#define XGMAC_TVER			BIT(18)
+#define XGMAC_RRSP			BIT(17)
+#define XGMAC_RVER			BIT(16)
+#define XGMAC_SRSP			BIT(2)
+#define XGMAC_SVER			BIT(1)
+#define XGMAC_EFPE			BIT(0)
+
+#define XGMAC_RXQ_CTRL0                 0x000000a0
+#define XGMAC_RXQ_CTRL1                 0x000000a4
+
+#define XGMAC_RQ                        GENMASK(7, 4)
+#define XGMAC_RQ_SHIFT                  4
+
+static void dwxgmac3_fpe_configure(void __iomem *ioaddr, struct stmmac_fpe_cfg *cfg,
+				   u32 num_txq,
+				   u32 num_rxq, bool enable)
 {
 	u32 value;
 
 	if (enable) {
-		cfg->fpe_csr = EFPE;
-		value = readl(ioaddr + GMAC_RXQ_CTRL1);
-		value &= ~GMAC_RXQCTRL_FPRQ;
-		value |= (num_rxq - 1) << GMAC_RXQCTRL_FPRQ_SHIFT;
-		writel(value, ioaddr + GMAC_RXQ_CTRL1);
+		cfg->fpe_csr = XGMAC_EFPE;
+		value = readl(ioaddr + XGMAC_RXQ_CTRL1);
+		value &= ~XGMAC_RQ;
+		value |= (num_rxq - 1) << XGMAC_RQ_SHIFT;
+		writel(value, ioaddr + XGMAC_RXQ_CTRL1);
 	} else {
 		cfg->fpe_csr = 0;
 	}
-	writel(cfg->fpe_csr, ioaddr + MAC_FPE_CTRL_STS);
+	writel(cfg->fpe_csr, ioaddr + XGMAC_FPE_CTRL_STS);
 }
 
-int dwmac5_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev)
+static int dwxgmac3_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev)
 {
 	u32 value;
 	int status;
 
 	status = FPE_EVENT_UNKNOWN;
 
-	/* Reads from the MAC_FPE_CTRL_STS register should only be performed
+	/* Reads from the XGMAC_FPE_CTRL_STS register should only be performed
 	 * here, since the status flags of MAC_FPE_CTRL_STS are "clear on read"
 	 */
-	value = readl(ioaddr + MAC_FPE_CTRL_STS);
+	value = readl(ioaddr + XGMAC_FPE_CTRL_STS);
 
-	if (value & TRSP) {
+	if (value & XGMAC_TRSP) {
 		status |= FPE_EVENT_TRSP;
 		netdev_info(dev, "FPE: Respond mPacket is transmitted\n");
 	}
 
-	if (value & TVER) {
+	if (value & XGMAC_TVER) {
 		status |= FPE_EVENT_TVER;
 		netdev_info(dev, "FPE: Verify mPacket is transmitted\n");
 	}
 
-	if (value & RRSP) {
+	if (value & XGMAC_RRSP) {
 		status |= FPE_EVENT_RRSP;
 		netdev_info(dev, "FPE: Respond mPacket is received\n");
 	}
 
-	if (value & RVER) {
+	if (value & XGMAC_RVER) {
 		status |= FPE_EVENT_RVER;
 		netdev_info(dev, "FPE: Verify mPacket is received\n");
 	}
@@ -66,15 +66,15 @@
 	return status;
 }
 
-void dwmac5_fpe_send_mpacket(void __iomem *ioaddr, struct stmmac_fpe_cfg *cfg,
-			     enum stmmac_mpacket_type type)
+static void dwxgmac3_fpe_send_mpacket(void __iomem *ioaddr, struct stmmac_fpe_cfg *cfg,
+				      enum stmmac_mpacket_type type)
 {
 	u32 value = cfg->fpe_csr;
 
 	if (type == MPACKET_VERIFY)
-		value |= SVER;
+		value |= XGMAC_SVER;
 	else if (type == MPACKET_RESPONSE)
-		value |= SRSP;
+		value |= XGMAC_SRSP;
 
-	writel(value, ioaddr + MAC_FPE_CTRL_STS);
+	writel(value, ioaddr + XGMAC_FPE_CTRL_STS);
 }

Re: [PATCH net-next] net: stmmac: xgmac3+: add FPE handshaking support

[Serge Semin]

[-- Attachment #1: Type: text/plain, Size: 4855 bytes --]

Hi Jianheng, Jakub

On Mon, Dec 11, 2023 at 06:13:21AM +0000, Jianheng Zhang wrote:
> Adds the HW specific support for Frame Preemption handshaking on XGMAC3+
> cores.

Thanks for the patch. No objection about the change:
Reviewed-by: Serge Semin <fancer.lancer@gmail.com>

The only note is that the DW XGMAC v3.x and DW QoS Eth v5.x FPE
implementations are now identical (see the attached diff). What about
factoring out the common parts to a separate file - stmmac_fpe.c
(perhaps together with the handshaking algo from the stmmac_main.c)
and send it out as an additional patch on top of this one? A similar
thing has been recently done for EST:
https://lore.kernel.org/netdev/20231201055252.1302-1-rohan.g.thomas@intel.com/
Although in this case AFAICS the implementation is simpler and the
only difference is in the CSR offset and the frame preemption residue
queue ID setting. All of that can be easily solved in the same way as
it was done for EST (see the link above).

Jakub, what do you think?

-Serge(y)

> 
> Signed-off-by: Jianheng Zhang <Jianheng.Zhang@synopsys.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h     |  6 ++
>  .../net/ethernet/stmicro/stmmac/dwxgmac2_core.c    | 65 ++++++++++++++++++----
>  2 files changed, 60 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> index 207ff17..306d15b 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> @@ -194,6 +194,12 @@
>  #define XGMAC_MDIO_DATA			0x00000204
>  #define XGMAC_MDIO_C22P			0x00000220
>  #define XGMAC_FPE_CTRL_STS		0x00000280
> +#define XGMAC_TRSP			BIT(19)
> +#define XGMAC_TVER			BIT(18)
> +#define XGMAC_RRSP			BIT(17)
> +#define XGMAC_RVER			BIT(16)
> +#define XGMAC_SRSP			BIT(2)
> +#define XGMAC_SVER			BIT(1)
>  #define XGMAC_EFPE			BIT(0)
>  #define XGMAC_ADDRx_HIGH(x)		(0x00000300 + (x) * 0x8)
>  #define XGMAC_ADDR_MAX			32
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> index eb48211..091d932 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> @@ -1439,22 +1439,63 @@ static void dwxgmac3_fpe_configure(void __iomem *ioaddr, struct stmmac_fpe_cfg *
>  {
>  	u32 value;
>  
> -	if (!enable) {
> -		value = readl(ioaddr + XGMAC_FPE_CTRL_STS);
> +	if (enable) {
> +		cfg->fpe_csr = XGMAC_EFPE;
> +		value = readl(ioaddr + XGMAC_RXQ_CTRL1);
> +		value &= ~XGMAC_RQ;
> +		value |= (num_rxq - 1) << XGMAC_RQ_SHIFT;
> +		writel(value, ioaddr + XGMAC_RXQ_CTRL1);
> +	} else {
> +		cfg->fpe_csr = 0;
> +	}
> +	writel(cfg->fpe_csr, ioaddr + XGMAC_FPE_CTRL_STS);
> +}
>  
> -		value &= ~XGMAC_EFPE;
> +static int dwxgmac3_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev)
> +{
> +	u32 value;
> +	int status;
>  
> -		writel(value, ioaddr + XGMAC_FPE_CTRL_STS);
> -		return;
> +	status = FPE_EVENT_UNKNOWN;
> +
> +	/* Reads from the XGMAC_FPE_CTRL_STS register should only be performed
> +	 * here, since the status flags of MAC_FPE_CTRL_STS are "clear on read"
> +	 */
> +	value = readl(ioaddr + XGMAC_FPE_CTRL_STS);
> +
> +	if (value & XGMAC_TRSP) {
> +		status |= FPE_EVENT_TRSP;
> +		netdev_info(dev, "FPE: Respond mPacket is transmitted\n");
>  	}
>  
> -	value = readl(ioaddr + XGMAC_RXQ_CTRL1);
> -	value &= ~XGMAC_RQ;
> -	value |= (num_rxq - 1) << XGMAC_RQ_SHIFT;
> -	writel(value, ioaddr + XGMAC_RXQ_CTRL1);
> +	if (value & XGMAC_TVER) {
> +		status |= FPE_EVENT_TVER;
> +		netdev_info(dev, "FPE: Verify mPacket is transmitted\n");
> +	}
> +
> +	if (value & XGMAC_RRSP) {
> +		status |= FPE_EVENT_RRSP;
> +		netdev_info(dev, "FPE: Respond mPacket is received\n");
> +	}
> +
> +	if (value & XGMAC_RVER) {
> +		status |= FPE_EVENT_RVER;
> +		netdev_info(dev, "FPE: Verify mPacket is received\n");
> +	}
> +
> +	return status;
> +}
> +
> +static void dwxgmac3_fpe_send_mpacket(void __iomem *ioaddr, struct stmmac_fpe_cfg *cfg,
> +				      enum stmmac_mpacket_type type)
> +{
> +	u32 value = cfg->fpe_csr;
> +
> +	if (type == MPACKET_VERIFY)
> +		value |= XGMAC_SVER;
> +	else if (type == MPACKET_RESPONSE)
> +		value |= XGMAC_SRSP;
>  
> -	value = readl(ioaddr + XGMAC_FPE_CTRL_STS);
> -	value |= XGMAC_EFPE;
>  	writel(value, ioaddr + XGMAC_FPE_CTRL_STS);
>  }
>  
> @@ -1503,6 +1544,8 @@ static void dwxgmac3_fpe_configure(void __iomem *ioaddr, struct stmmac_fpe_cfg *
>  	.config_l4_filter = dwxgmac2_config_l4_filter,
>  	.set_arp_offload = dwxgmac2_set_arp_offload,
>  	.fpe_configure = dwxgmac3_fpe_configure,
> +	.fpe_send_mpacket = dwxgmac3_fpe_send_mpacket,
> +	.fpe_irq_status = dwxgmac3_fpe_irq_status,
>  };
>  
>  static void dwxlgmac2_rx_queue_enable(struct mac_device_info *hw, u8 mode,
> -- 
> 1.8.3.1
> 
> 

[-- Attachment #2: stmmac_fpe.diff --]
[-- Type: text/x-patch, Size: 3544 bytes --]

--- drivers/net/ethernet/stmicro/stmmac/dwmac5_fpe.c	2023-12-11 14:01:26.888400348 +0300
+++ drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core_fpe.c	2023-12-11 14:01:49.538644889 +0300
@@ -1,64 +1,64 @@
-#define MAC_FPE_CTRL_STS		0x00000234
-#define TRSP				BIT(19)
-#define TVER				BIT(18)
-#define RRSP				BIT(17)
-#define RVER				BIT(16)
-#define SRSP				BIT(2)
-#define SVER				BIT(1)
-#define EFPE				BIT(0)
-
-#define GMAC_RXQ_CTRL0			0x000000a0
-#define GMAC_RXQ_CTRL1			0x000000a4
-
-#define GMAC_RXQCTRL_FPRQ		GENMASK(26, 24)
-#define GMAC_RXQCTRL_FPRQ_SHIFT		24
-
-void dwmac5_fpe_configure(void __iomem *ioaddr, struct stmmac_fpe_cfg *cfg,
-			  u32 num_txq, u32 num_rxq,
-			  bool enable)
+#define XGMAC_FPE_CTRL_STS		0x00000280
+#define XGMAC_TRSP			BIT(19)
+#define XGMAC_TVER			BIT(18)
+#define XGMAC_RRSP			BIT(17)
+#define XGMAC_RVER			BIT(16)
+#define XGMAC_SRSP			BIT(2)
+#define XGMAC_SVER			BIT(1)
+#define XGMAC_EFPE			BIT(0)
+
+#define XGMAC_RXQ_CTRL0                 0x000000a0
+#define XGMAC_RXQ_CTRL1                 0x000000a4
+
+#define XGMAC_RQ                        GENMASK(7, 4)
+#define XGMAC_RQ_SHIFT                  4
+
+static void dwxgmac3_fpe_configure(void __iomem *ioaddr, struct stmmac_fpe_cfg *cfg,
+				   u32 num_txq,
+				   u32 num_rxq, bool enable)
 {
 	u32 value;
 
 	if (enable) {
-		cfg->fpe_csr = EFPE;
-		value = readl(ioaddr + GMAC_RXQ_CTRL1);
-		value &= ~GMAC_RXQCTRL_FPRQ;
-		value |= (num_rxq - 1) << GMAC_RXQCTRL_FPRQ_SHIFT;
-		writel(value, ioaddr + GMAC_RXQ_CTRL1);
+		cfg->fpe_csr = XGMAC_EFPE;
+		value = readl(ioaddr + XGMAC_RXQ_CTRL1);
+		value &= ~XGMAC_RQ;
+		value |= (num_rxq - 1) << XGMAC_RQ_SHIFT;
+		writel(value, ioaddr + XGMAC_RXQ_CTRL1);
 	} else {
 		cfg->fpe_csr = 0;
 	}
-	writel(cfg->fpe_csr, ioaddr + MAC_FPE_CTRL_STS);
+	writel(cfg->fpe_csr, ioaddr + XGMAC_FPE_CTRL_STS);
 }
 
-int dwmac5_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev)
+static int dwxgmac3_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev)
 {
 	u32 value;
 	int status;
 
 	status = FPE_EVENT_UNKNOWN;
 
-	/* Reads from the MAC_FPE_CTRL_STS register should only be performed
+	/* Reads from the XGMAC_FPE_CTRL_STS register should only be performed
 	 * here, since the status flags of MAC_FPE_CTRL_STS are "clear on read"
 	 */
-	value = readl(ioaddr + MAC_FPE_CTRL_STS);
+	value = readl(ioaddr + XGMAC_FPE_CTRL_STS);
 
-	if (value & TRSP) {
+	if (value & XGMAC_TRSP) {
 		status |= FPE_EVENT_TRSP;
 		netdev_info(dev, "FPE: Respond mPacket is transmitted\n");
 	}
 
-	if (value & TVER) {
+	if (value & XGMAC_TVER) {
 		status |= FPE_EVENT_TVER;
 		netdev_info(dev, "FPE: Verify mPacket is transmitted\n");
 	}
 
-	if (value & RRSP) {
+	if (value & XGMAC_RRSP) {
 		status |= FPE_EVENT_RRSP;
 		netdev_info(dev, "FPE: Respond mPacket is received\n");
 	}
 
-	if (value & RVER) {
+	if (value & XGMAC_RVER) {
 		status |= FPE_EVENT_RVER;
 		netdev_info(dev, "FPE: Verify mPacket is received\n");
 	}
@@ -66,15 +66,15 @@
 	return status;
 }
 
-void dwmac5_fpe_send_mpacket(void __iomem *ioaddr, struct stmmac_fpe_cfg *cfg,
-			     enum stmmac_mpacket_type type)
+static void dwxgmac3_fpe_send_mpacket(void __iomem *ioaddr, struct stmmac_fpe_cfg *cfg,
+				      enum stmmac_mpacket_type type)
 {
 	u32 value = cfg->fpe_csr;
 
 	if (type == MPACKET_VERIFY)
-		value |= SVER;
+		value |= XGMAC_SVER;
 	else if (type == MPACKET_RESPONSE)
-		value |= SRSP;
+		value |= XGMAC_SRSP;
 
-	writel(value, ioaddr + MAC_FPE_CTRL_STS);
+	writel(value, ioaddr + XGMAC_FPE_CTRL_STS);
 }

[-- Attachment #3: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: stmmac: xgmac3+: add FPE handshaking support

[Vladimir Oltean]

Hi Jianheng,

On Mon, Dec 11, 2023 at 06:13:21AM +0000, Jianheng Zhang wrote:
> Adds the HW specific support for Frame Preemption handshaking on XGMAC3+
> cores.
> 
> Signed-off-by: Jianheng Zhang <Jianheng.Zhang@synopsys.com>
> ---

It's nice to see contributions from Synopsys!

Have you seen the (relatively newly introduced) common framework for
Frame Preemption and the MAC Merge layer?
https://docs.kernel.org/networking/ethtool-netlink.html#mm-get
https://man7.org/linux/man-pages/man8/ethtool.8.html
https://man7.org/linux/man-pages/man8/tc-mqprio.8.html # "fp" option
https://man7.org/linux/man-pages/man8/tc-taprio.8.html # "fp" option

I think it would be valuable if the stmmac driver would also use it, so
it could support openlldp and pass the selftest at
https://github.com/torvalds/linux/blob/master/tools/testing/selftests/net/forwarding/ethtool_mm.sh

Re: [PATCH net-next] net: stmmac: xgmac3+: add FPE handshaking support

[Vladimir Oltean]

Hi Jianheng,

On Mon, Dec 11, 2023 at 06:13:21AM +0000, Jianheng Zhang wrote:
> Adds the HW specific support for Frame Preemption handshaking on XGMAC3+
> cores.
> 
> Signed-off-by: Jianheng Zhang <Jianheng.Zhang@synopsys.com>
> ---

It's nice to see contributions from Synopsys!

Have you seen the (relatively newly introduced) common framework for
Frame Preemption and the MAC Merge layer?
https://docs.kernel.org/networking/ethtool-netlink.html#mm-get
https://man7.org/linux/man-pages/man8/ethtool.8.html
https://man7.org/linux/man-pages/man8/tc-mqprio.8.html # "fp" option
https://man7.org/linux/man-pages/man8/tc-taprio.8.html # "fp" option

I think it would be valuable if the stmmac driver would also use it, so
it could support openlldp and pass the selftest at
https://github.com/torvalds/linux/blob/master/tools/testing/selftests/net/forwarding/ethtool_mm.sh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: stmmac: xgmac3+: add FPE handshaking support

[Andrew Lunn]

On Mon, Dec 11, 2023 at 06:13:21AM +0000, Jianheng Zhang wrote:
> Adds the HW specific support for Frame Preemption handshaking on XGMAC3+
> cores.
> 
> Signed-off-by: Jianheng Zhang <Jianheng.Zhang@synopsys.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h     |  6 ++
>  .../net/ethernet/stmicro/stmmac/dwxgmac2_core.c    | 65 ++++++++++++++++++----
>  2 files changed, 60 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> index 207ff17..306d15b 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> @@ -194,6 +194,12 @@
>  #define XGMAC_MDIO_DATA			0x00000204
>  #define XGMAC_MDIO_C22P			0x00000220
>  #define XGMAC_FPE_CTRL_STS		0x00000280
> +#define XGMAC_TRSP			BIT(19)
> +#define XGMAC_TVER			BIT(18)
> +#define XGMAC_RRSP			BIT(17)
> +#define XGMAC_RVER			BIT(16)
> +#define XGMAC_SRSP			BIT(2)
> +#define XGMAC_SVER			BIT(1)
>  #define XGMAC_EFPE			BIT(0)
>  #define XGMAC_ADDRx_HIGH(x)		(0x00000300 + (x) * 0x8)
>  #define XGMAC_ADDR_MAX			32
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> index eb48211..091d932 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> @@ -1439,22 +1439,63 @@ static void dwxgmac3_fpe_configure(void __iomem *ioaddr, struct stmmac_fpe_cfg *
>  {
>  	u32 value;
>  
> -	if (!enable) {
> -		value = readl(ioaddr + XGMAC_FPE_CTRL_STS);
> +	if (enable) {
> +		cfg->fpe_csr = XGMAC_EFPE;
> +		value = readl(ioaddr + XGMAC_RXQ_CTRL1);
> +		value &= ~XGMAC_RQ;
> +		value |= (num_rxq - 1) << XGMAC_RQ_SHIFT;
> +		writel(value, ioaddr + XGMAC_RXQ_CTRL1);
> +	} else {
> +		cfg->fpe_csr = 0;
> +	}
> +	writel(cfg->fpe_csr, ioaddr + XGMAC_FPE_CTRL_STS);
> +}
>  
> -		value &= ~XGMAC_EFPE;
> +static int dwxgmac3_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev)
> +{
> +	u32 value;
> +	int status;
>  
> -		writel(value, ioaddr + XGMAC_FPE_CTRL_STS);
> -		return;
> +	status = FPE_EVENT_UNKNOWN;
> +
> +	/* Reads from the XGMAC_FPE_CTRL_STS register should only be performed
> +	 * here, since the status flags of MAC_FPE_CTRL_STS are "clear on read"
> +	 */
> +	value = readl(ioaddr + XGMAC_FPE_CTRL_STS);
> +
> +	if (value & XGMAC_TRSP) {
> +		status |= FPE_EVENT_TRSP;
> +		netdev_info(dev, "FPE: Respond mPacket is transmitted\n");

netdev_info()?  Is this going to spam the logs? Should it be netdev_dbg()

	Andrew

Re: [PATCH net-next] net: stmmac: xgmac3+: add FPE handshaking support

[Andrew Lunn]

On Mon, Dec 11, 2023 at 06:13:21AM +0000, Jianheng Zhang wrote:
> Adds the HW specific support for Frame Preemption handshaking on XGMAC3+
> cores.
> 
> Signed-off-by: Jianheng Zhang <Jianheng.Zhang@synopsys.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h     |  6 ++
>  .../net/ethernet/stmicro/stmmac/dwxgmac2_core.c    | 65 ++++++++++++++++++----
>  2 files changed, 60 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> index 207ff17..306d15b 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> @@ -194,6 +194,12 @@
>  #define XGMAC_MDIO_DATA			0x00000204
>  #define XGMAC_MDIO_C22P			0x00000220
>  #define XGMAC_FPE_CTRL_STS		0x00000280
> +#define XGMAC_TRSP			BIT(19)
> +#define XGMAC_TVER			BIT(18)
> +#define XGMAC_RRSP			BIT(17)
> +#define XGMAC_RVER			BIT(16)
> +#define XGMAC_SRSP			BIT(2)
> +#define XGMAC_SVER			BIT(1)
>  #define XGMAC_EFPE			BIT(0)
>  #define XGMAC_ADDRx_HIGH(x)		(0x00000300 + (x) * 0x8)
>  #define XGMAC_ADDR_MAX			32
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> index eb48211..091d932 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> @@ -1439,22 +1439,63 @@ static void dwxgmac3_fpe_configure(void __iomem *ioaddr, struct stmmac_fpe_cfg *
>  {
>  	u32 value;
>  
> -	if (!enable) {
> -		value = readl(ioaddr + XGMAC_FPE_CTRL_STS);
> +	if (enable) {
> +		cfg->fpe_csr = XGMAC_EFPE;
> +		value = readl(ioaddr + XGMAC_RXQ_CTRL1);
> +		value &= ~XGMAC_RQ;
> +		value |= (num_rxq - 1) << XGMAC_RQ_SHIFT;
> +		writel(value, ioaddr + XGMAC_RXQ_CTRL1);
> +	} else {
> +		cfg->fpe_csr = 0;
> +	}
> +	writel(cfg->fpe_csr, ioaddr + XGMAC_FPE_CTRL_STS);
> +}
>  
> -		value &= ~XGMAC_EFPE;
> +static int dwxgmac3_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev)
> +{
> +	u32 value;
> +	int status;
>  
> -		writel(value, ioaddr + XGMAC_FPE_CTRL_STS);
> -		return;
> +	status = FPE_EVENT_UNKNOWN;
> +
> +	/* Reads from the XGMAC_FPE_CTRL_STS register should only be performed
> +	 * here, since the status flags of MAC_FPE_CTRL_STS are "clear on read"
> +	 */
> +	value = readl(ioaddr + XGMAC_FPE_CTRL_STS);
> +
> +	if (value & XGMAC_TRSP) {
> +		status |= FPE_EVENT_TRSP;
> +		netdev_info(dev, "FPE: Respond mPacket is transmitted\n");

netdev_info()?  Is this going to spam the logs? Should it be netdev_dbg()

	Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: stmmac: xgmac3+: add FPE handshaking support

[Jakub Kicinski]

On Mon, 11 Dec 2023 14:14:00 +0300 Serge Semin wrote:
> Although in this case AFAICS the implementation is simpler and the
> only difference is in the CSR offset and the frame preemption residue
> queue ID setting. All of that can be easily solved in the same way as
> it was done for EST (see the link above).
> 
> Jakub, what do you think?

Yup, less code duplication would be great. Highest prio, tho, is to
focus on Vladimir's comment around this driver seemingly implementing
FPE but not using the common ethtool APIs to configure it, yet :(

Re: [PATCH net-next] net: stmmac: xgmac3+: add FPE handshaking support

[Jakub Kicinski]

On Mon, 11 Dec 2023 14:14:00 +0300 Serge Semin wrote:
> Although in this case AFAICS the implementation is simpler and the
> only difference is in the CSR offset and the frame preemption residue
> queue ID setting. All of that can be easily solved in the same way as
> it was done for EST (see the link above).
> 
> Jakub, what do you think?

Yup, less code duplication would be great. Highest prio, tho, is to
focus on Vladimir's comment around this driver seemingly implementing
FPE but not using the common ethtool APIs to configure it, yet :(

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH net-next] net: stmmac: xgmac3+: add FPE handshaking support

[Jianheng Zhang]

Hi Andrew,  

> > +static int dwxgmac3_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev)
> > +{
> > +	u32 value;
> > +	int status;
> >
> > -		writel(value, ioaddr + XGMAC_FPE_CTRL_STS);
> > -		return;
> > +	status = FPE_EVENT_UNKNOWN;
> > +
> > +	/* Reads from the XGMAC_FPE_CTRL_STS register should only be performed
> > +	 * here, since the status flags of MAC_FPE_CTRL_STS are "clear on read"
> > +	 */
> > +	value = readl(ioaddr + XGMAC_FPE_CTRL_STS);
> > +
> > +	if (value & XGMAC_TRSP) {
> > +		status |= FPE_EVENT_TRSP;
> > +		netdev_info(dev, "FPE: Respond mPacket is transmitted\n");
> 
> netdev_info()?  Is this going to spam the logs? Should it be netdev_dbg()

Yes, netdev_dbg() should be better, let me fix it in the next patch.

Jianheng
> 
> 	Andrew

RE: [PATCH net-next] net: stmmac: xgmac3+: add FPE handshaking support

[Jianheng Zhang]

Hi Andrew,  

> > +static int dwxgmac3_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev)
> > +{
> > +	u32 value;
> > +	int status;
> >
> > -		writel(value, ioaddr + XGMAC_FPE_CTRL_STS);
> > -		return;
> > +	status = FPE_EVENT_UNKNOWN;
> > +
> > +	/* Reads from the XGMAC_FPE_CTRL_STS register should only be performed
> > +	 * here, since the status flags of MAC_FPE_CTRL_STS are "clear on read"
> > +	 */
> > +	value = readl(ioaddr + XGMAC_FPE_CTRL_STS);
> > +
> > +	if (value & XGMAC_TRSP) {
> > +		status |= FPE_EVENT_TRSP;
> > +		netdev_info(dev, "FPE: Respond mPacket is transmitted\n");
> 
> netdev_info()?  Is this going to spam the logs? Should it be netdev_dbg()

Yes, netdev_dbg() should be better, let me fix it in the next patch.

Jianheng
> 
> 	Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH net-next] net: stmmac: xgmac3+: add FPE handshaking support

[Jianheng Zhang]

Hi Vladimir,

> -----Original Message-----
> From: Vladimir Oltean <olteanv@gmail.com>
> Sent: Monday, December 11, 2023 9:40 PM
> To: Jianheng Zhang <jianheng@synopsys.com>
> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>; David S.
> Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub Kicinski
> <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Maxime Coquelin
> <mcoquelin.stm32@gmail.com>; open list:STMMAC ETHERNET DRIVER <netdev@vger.kernel.org>;
> moderated list:ARM/STM32 ARCHITECTURE <linux-stm32@st-md-mailman.stormreply.com>;
> moderated list:ARM/STM32 ARCHITECTURE <linux-arm-kernel@lists.infradead.org>; open list
> <linux-kernel@vger.kernel.org>; James Li <lijames@synopsys.com>; Martin McKenny
> <mmckenny@synopsys.com>
> Subject: Re: [PATCH net-next] net: stmmac: xgmac3+: add FPE handshaking support
> 
> Hi Jianheng,
> 
> On Mon, Dec 11, 2023 at 06:13:21AM +0000, Jianheng Zhang wrote:
> > Adds the HW specific support for Frame Preemption handshaking on XGMAC3+
> > cores.
> >
> > Signed-off-by: Jianheng Zhang <Jianheng.Zhang@synopsys.com>
> > ---
> 
> It's nice to see contributions from Synopsys!
> 
> Have you seen the (relatively newly introduced) common framework for
> Frame Preemption and the MAC Merge layer?
> https://urldefense.com/v3/__https://docs.kernel.org/networking/ethtool-netlink.html*mm-get__;Iw!!A
> 4F2R9G_pg!Z461fiVMBqXVlpgdD8t9ey1qGp6_hZg9jNlY__TljPgVHZcYbqtzqQhbI9IpjDoHBoOCX14vZOf2J
> hZgsW_fnQ$
> https://urldefense.com/v3/__https://man7.org/linux/man-pages/man8/ethtool.8.html__;!!A4F2R9G_pg
> !Z461fiVMBqXVlpgdD8t9ey1qGp6_hZg9jNlY__TljPgVHZcYbqtzqQhbI9IpjDoHBoOCX14vZOf2JhbkJXuqTA$
> https://urldefense.com/v3/__https://man7.org/linux/man-pages/man8/tc-mqprio.8.html__;!!A4F2R9G_
> pg!Z461fiVMBqXVlpgdD8t9ey1qGp6_hZg9jNlY__TljPgVHZcYbqtzqQhbI9IpjDoHBoOCX14vZOf2JhbEBQbq
> ZQ$  # "fp" option
> https://urldefense.com/v3/__https://man7.org/linux/man-pages/man8/tc-taprio.8.html__;!!A4F2R9G_p
> g!Z461fiVMBqXVlpgdD8t9ey1qGp6_hZg9jNlY__TljPgVHZcYbqtzqQhbI9IpjDoHBoOCX14vZOf2JhbOMeXO
> UQ$  # "fp" option
> 
> I think it would be valuable if the stmmac driver would also use it, so
> it could support openlldp and pass the selftest at
> https://urldefense.com/v3/__https://github.com/torvalds/linux/blob/master/tools/testing/selftests/net/f
> orwarding/ethtool_mm.sh__;!!A4F2R9G_pg!Z461fiVMBqXVlpgdD8t9ey1qGp6_hZg9jNlY__TljPgVHZcYb
> qtzqQhbI9IpjDoHBoOCX14vZOf2JhasMiyt2w$

Thanks for mentioning the common framework for Frame Preemption and the MAC 
Merge layer. I think it is essential to let the stmmac driver support it next.
And it is also needed to avoid the code duplication mentioned by Serge.

Jianheng

RE: [PATCH net-next] net: stmmac: xgmac3+: add FPE handshaking support

[Jianheng Zhang]

Hi Vladimir,

> -----Original Message-----
> From: Vladimir Oltean <olteanv@gmail.com>
> Sent: Monday, December 11, 2023 9:40 PM
> To: Jianheng Zhang <jianheng@synopsys.com>
> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>; David S.
> Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub Kicinski
> <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Maxime Coquelin
> <mcoquelin.stm32@gmail.com>; open list:STMMAC ETHERNET DRIVER <netdev@vger.kernel.org>;
> moderated list:ARM/STM32 ARCHITECTURE <linux-stm32@st-md-mailman.stormreply.com>;
> moderated list:ARM/STM32 ARCHITECTURE <linux-arm-kernel@lists.infradead.org>; open list
> <linux-kernel@vger.kernel.org>; James Li <lijames@synopsys.com>; Martin McKenny
> <mmckenny@synopsys.com>
> Subject: Re: [PATCH net-next] net: stmmac: xgmac3+: add FPE handshaking support
> 
> Hi Jianheng,
> 
> On Mon, Dec 11, 2023 at 06:13:21AM +0000, Jianheng Zhang wrote:
> > Adds the HW specific support for Frame Preemption handshaking on XGMAC3+
> > cores.
> >
> > Signed-off-by: Jianheng Zhang <Jianheng.Zhang@synopsys.com>
> > ---
> 
> It's nice to see contributions from Synopsys!
> 
> Have you seen the (relatively newly introduced) common framework for
> Frame Preemption and the MAC Merge layer?
> https://urldefense.com/v3/__https://docs.kernel.org/networking/ethtool-netlink.html*mm-get__;Iw!!A
> 4F2R9G_pg!Z461fiVMBqXVlpgdD8t9ey1qGp6_hZg9jNlY__TljPgVHZcYbqtzqQhbI9IpjDoHBoOCX14vZOf2J
> hZgsW_fnQ$
> https://urldefense.com/v3/__https://man7.org/linux/man-pages/man8/ethtool.8.html__;!!A4F2R9G_pg
> !Z461fiVMBqXVlpgdD8t9ey1qGp6_hZg9jNlY__TljPgVHZcYbqtzqQhbI9IpjDoHBoOCX14vZOf2JhbkJXuqTA$
> https://urldefense.com/v3/__https://man7.org/linux/man-pages/man8/tc-mqprio.8.html__;!!A4F2R9G_
> pg!Z461fiVMBqXVlpgdD8t9ey1qGp6_hZg9jNlY__TljPgVHZcYbqtzqQhbI9IpjDoHBoOCX14vZOf2JhbEBQbq
> ZQ$  # "fp" option
> https://urldefense.com/v3/__https://man7.org/linux/man-pages/man8/tc-taprio.8.html__;!!A4F2R9G_p
> g!Z461fiVMBqXVlpgdD8t9ey1qGp6_hZg9jNlY__TljPgVHZcYbqtzqQhbI9IpjDoHBoOCX14vZOf2JhbOMeXO
> UQ$  # "fp" option
> 
> I think it would be valuable if the stmmac driver would also use it, so
> it could support openlldp and pass the selftest at
> https://urldefense.com/v3/__https://github.com/torvalds/linux/blob/master/tools/testing/selftests/net/f
> orwarding/ethtool_mm.sh__;!!A4F2R9G_pg!Z461fiVMBqXVlpgdD8t9ey1qGp6_hZg9jNlY__TljPgVHZcYb
> qtzqQhbI9IpjDoHBoOCX14vZOf2JhasMiyt2w$

Thanks for mentioning the common framework for Frame Preemption and the MAC 
Merge layer. I think it is essential to let the stmmac driver support it next.
And it is also needed to avoid the code duplication mentioned by Serge.

Jianheng

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: stmmac: xgmac3+: add FPE handshaking support

[Serge Semin]

On Tue, Dec 12, 2023 at 07:22:24AM +0000, Jianheng Zhang wrote:
> Hi Andrew,  
> 
> > > +static int dwxgmac3_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev)
> > > +{
> > > +	u32 value;
> > > +	int status;
> > >
> > > -		writel(value, ioaddr + XGMAC_FPE_CTRL_STS);
> > > -		return;
> > > +	status = FPE_EVENT_UNKNOWN;
> > > +
> > > +	/* Reads from the XGMAC_FPE_CTRL_STS register should only be performed
> > > +	 * here, since the status flags of MAC_FPE_CTRL_STS are "clear on read"
> > > +	 */
> > > +	value = readl(ioaddr + XGMAC_FPE_CTRL_STS);
> > > +
> > > +	if (value & XGMAC_TRSP) {
> > > +		status |= FPE_EVENT_TRSP;
> > > +		netdev_info(dev, "FPE: Respond mPacket is transmitted\n");
> > 
> > netdev_info()?  Is this going to spam the logs? Should it be netdev_dbg()
> 
> Yes, netdev_dbg() should be better, let me fix it in the next patch.

Please do not forget to keep this change in the refactoring patch (if
one will be introduced). So instead of preserving the DW QoS Eth FPE
code snipped with netdev_info() utilized, the common FPE
implementation would have the netdev_dbg() calls.

-Serge(y)

> 
> Jianheng
> > 
> > 	Andrew
> 

Re: [PATCH net-next] net: stmmac: xgmac3+: add FPE handshaking support

[Serge Semin]

On Tue, Dec 12, 2023 at 07:22:24AM +0000, Jianheng Zhang wrote:
> Hi Andrew,  
> 
> > > +static int dwxgmac3_fpe_irq_status(void __iomem *ioaddr, struct net_device *dev)
> > > +{
> > > +	u32 value;
> > > +	int status;
> > >
> > > -		writel(value, ioaddr + XGMAC_FPE_CTRL_STS);
> > > -		return;
> > > +	status = FPE_EVENT_UNKNOWN;
> > > +
> > > +	/* Reads from the XGMAC_FPE_CTRL_STS register should only be performed
> > > +	 * here, since the status flags of MAC_FPE_CTRL_STS are "clear on read"
> > > +	 */
> > > +	value = readl(ioaddr + XGMAC_FPE_CTRL_STS);
> > > +
> > > +	if (value & XGMAC_TRSP) {
> > > +		status |= FPE_EVENT_TRSP;
> > > +		netdev_info(dev, "FPE: Respond mPacket is transmitted\n");
> > 
> > netdev_info()?  Is this going to spam the logs? Should it be netdev_dbg()
> 
> Yes, netdev_dbg() should be better, let me fix it in the next patch.

Please do not forget to keep this change in the refactoring patch (if
one will be introduced). So instead of preserving the DW QoS Eth FPE
code snipped with netdev_info() utilized, the common FPE
implementation would have the netdev_dbg() calls.

-Serge(y)

> 
> Jianheng
> > 
> > 	Andrew
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: stmmac: xgmac3+: add FPE handshaking support

[Serge Semin]

On Mon, Dec 11, 2023 at 02:59:44PM -0800, Jakub Kicinski wrote:
> On Mon, 11 Dec 2023 14:14:00 +0300 Serge Semin wrote:
> > Although in this case AFAICS the implementation is simpler and the
> > only difference is in the CSR offset and the frame preemption residue
> > queue ID setting. All of that can be easily solved in the same way as
> > it was done for EST (see the link above).
> > 
> > Jakub, what do you think?
> 
> Yup, less code duplication would be great. Highest prio, tho, is to
> focus on Vladimir's comment around this driver seemingly implementing
> FPE but not using the common ethtool APIs to configure it, yet :(

Fully agree. If that required to refactor the currently implemented
handshaking algo (looking not that safe and clumsy a bit), it would
have been even better. Although I guess refactoring the code
duplication could be done as a preparation patch anyway or as a patch
which would convert the feature-specific callbacks to be suitable for
the 802.3 MAC Merge layer.

-Serge(y)

Re: [PATCH net-next] net: stmmac: xgmac3+: add FPE handshaking support

[Serge Semin]

On Mon, Dec 11, 2023 at 02:59:44PM -0800, Jakub Kicinski wrote:
> On Mon, 11 Dec 2023 14:14:00 +0300 Serge Semin wrote:
> > Although in this case AFAICS the implementation is simpler and the
> > only difference is in the CSR offset and the frame preemption residue
> > queue ID setting. All of that can be easily solved in the same way as
> > it was done for EST (see the link above).
> > 
> > Jakub, what do you think?
> 
> Yup, less code duplication would be great. Highest prio, tho, is to
> focus on Vladimir's comment around this driver seemingly implementing
> FPE but not using the common ethtool APIs to configure it, yet :(

Fully agree. If that required to refactor the currently implemented
handshaking algo (looking not that safe and clumsy a bit), it would
have been even better. Although I guess refactoring the code
duplication could be done as a preparation patch anyway or as a patch
which would convert the feature-specific callbacks to be suitable for
the 802.3 MAC Merge layer.

-Serge(y)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel