[PATCH net-next v2 0/2] Add support for partial store and forward

[PATCH net-next v2 0/2] Add support for partial store and forward

[Pranavi Somisetty]

Add support for partial store and forward mode in Cadence MACB.

Link for v1:
https://lore.kernel.org/all/20221213121245.13981-1-pranavi.somisetty@amd.com/

Changes v2:
1. Removed all the changes related to validating FCS when Rx checksum
offload is disabled.
2. Instead of using a platform dependent number (0xFFF) for the reset
value of rx watermark, derive it from designcfg_debug2 register.
3. Added a check to see if partial s/f is supported, by reading the
designcfg_debug6 register.
4. Added devicetree bindings for "rx-watermark" property.

Pranavi Somisetty (2):
  dt-bindings: net: cdns,macb: Add rx-watermark property
  net: macb: Add support for partial store and forward

 .../devicetree/bindings/net/cdns,macb.yaml    |  8 +++
 drivers/net/ethernet/cadence/macb.h           | 14 ++++++
 drivers/net/ethernet/cadence/macb_main.c      | 49 ++++++++++++++++++-
 3 files changed, 69 insertions(+), 2 deletions(-)

-- 
2.36.1

[PATCH net-next v2 1/2] dt-bindings: net: cdns,macb: Add rx-watermark property

[Pranavi Somisetty]

watermark value is the minimum amount of packet data
required to activate the forwarding process. The watermark
implementation and maximum size is dependent on the device
where Cadence MACB/GEM is used.

Signed-off-by: Pranavi Somisetty <pranavi.somisetty@amd.com>
---
 Documentation/devicetree/bindings/net/cdns,macb.yaml | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/cdns,macb.yaml b/Documentation/devicetree/bindings/net/cdns,macb.yaml
index bef5e0f895be..779bc25cf005 100644
--- a/Documentation/devicetree/bindings/net/cdns,macb.yaml
+++ b/Documentation/devicetree/bindings/net/cdns,macb.yaml
@@ -109,6 +109,13 @@ properties:
   power-domains:
     maxItems: 1
 
+  rx-watermark:
+    maxItems: 1
+    $ref: /schemas/types.yaml#/definitions/uint16
+    description:
+      Set watermark value for pbuf_rxcutthru reg and enable
+      rx partial store and forward.
+
   '#address-cells':
     const: 1
 
@@ -166,6 +173,7 @@ examples:
             compatible = "cdns,macb";
             reg = <0xfffc4000 0x4000>;
             interrupts = <21>;
+            rx-watermark = /bits/ 16 <0x44>;
             phy-mode = "rmii";
             local-mac-address = [3a 0e 03 04 05 06];
             clock-names = "pclk", "hclk", "tx_clk";
-- 
2.36.1

[PATCH net-next v2 2/2] net: macb: Add support for partial store and forward

[Pranavi Somisetty]

When the receive partial store and forward mode is activated, the
receiver will only begin to forward the packet to the external AHB
or AXI slave when enough packet data is stored in the packet buffer.
The amount of packet data required to activate the forwarding process
is programmable via watermark registers which are located at the same
address as the partial store and forward enable bits. Adding support to
read this rx-watermark value from device-tree, to program the watermark
registers and enable partial store and forwarding.

Signed-off-by: Maulik Jodhani <maulik.jodhani@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Signed-off-by: Harini Katakam <harini.katakam@xilinx.com>
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
Signed-off-by: Pranavi Somisetty <pranavi.somisetty@amd.com>
---
Changes v2:

1. Removed all the changes related to validating FCS when Rx checksum offload is disabled.
2. Instead of using a platform dependent number (0xFFF) for the reset value of rx watermark,
derive it from designcfg_debug2 register.
3. Added a check to see if partial s/f is supported, by reading the
designcfg_debug6 register.
---
 drivers/net/ethernet/cadence/macb.h      | 14 +++++++
 drivers/net/ethernet/cadence/macb_main.c | 49 +++++++++++++++++++++++-
 2 files changed, 61 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 14dfec4db8f9..46833662094d 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -82,6 +82,7 @@
 #define GEM_NCFGR		0x0004 /* Network Config */
 #define GEM_USRIO		0x000c /* User IO */
 #define GEM_DMACFG		0x0010 /* DMA Configuration */
+#define GEM_PBUFRXCUT		0x0044 /* RX Partial Store and Forward */
 #define GEM_JML			0x0048 /* Jumbo Max Length */
 #define GEM_HS_MAC_CONFIG	0x0050 /* GEM high speed config */
 #define GEM_HRB			0x0080 /* Hash Bottom */
@@ -343,6 +344,11 @@
 #define GEM_ADDR64_SIZE		1
 
 
+/* Bitfields in PBUFRXCUT */
+#define GEM_WTRMRK_OFFSET	0 /* Watermark value offset */
+#define GEM_ENCUTTHRU_OFFSET	31 /* Enable RX partial store and forward */
+#define GEM_ENCUTTHRU_SIZE	1
+
 /* Bitfields in NSR */
 #define MACB_NSR_LINK_OFFSET	0 /* pcs_link_state */
 #define MACB_NSR_LINK_SIZE	1
@@ -509,6 +515,8 @@
 #define GEM_TX_PKT_BUFF_OFFSET			21
 #define GEM_TX_PKT_BUFF_SIZE			1
 
+#define GEM_RX_PBUF_ADDR_OFFSET			22
+#define GEM_RX_PBUF_ADDR_SIZE			4
 
 /* Bitfields in DCFG5. */
 #define GEM_TSU_OFFSET				8
@@ -517,6 +525,8 @@
 /* Bitfields in DCFG6. */
 #define GEM_PBUF_LSO_OFFSET			27
 #define GEM_PBUF_LSO_SIZE			1
+#define GEM_PBUF_CUTTHRU_OFFSET			26
+#define GEM_PBUF_CUTTHRU_SIZE			1
 #define GEM_DAW64_OFFSET			23
 #define GEM_DAW64_SIZE				1
 
@@ -718,6 +728,7 @@
 #define MACB_CAPS_NEEDS_RSTONUBR		0x00000100
 #define MACB_CAPS_MIIONRGMII			0x00000200
 #define MACB_CAPS_NEED_TSUCLK			0x00000400
+#define MACB_CAPS_PARTIAL_STORE_FORWARD		0x00000800
 #define MACB_CAPS_PCS				0x01000000
 #define MACB_CAPS_HIGH_SPEED			0x02000000
 #define MACB_CAPS_CLK_HW_CHG			0x04000000
@@ -1283,6 +1294,9 @@ struct macb {
 
 	u32			wol;
 
+	/* holds value of rx watermark value for pbuf_rxcutthru register */
+	u16			rx_watermark;
+
 	struct macb_ptp_info	*ptp_info;	/* macb-ptp interface */
 
 	struct phy		*sgmii_phy;	/* for ZynqMP SGMII mode */
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 41964fd02452..07b9964e7aa3 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -2600,6 +2600,7 @@ static void macb_init_rings(struct macb *bp)
 static void macb_reset_hw(struct macb *bp)
 {
 	struct macb_queue *queue;
+	u16 watermark_reset_value;
 	unsigned int q;
 	u32 ctrl = macb_readl(bp, NCR);
 
@@ -2617,6 +2618,12 @@ static void macb_reset_hw(struct macb *bp)
 	macb_writel(bp, TSR, -1);
 	macb_writel(bp, RSR, -1);
 
+	/* Disable RX partial store and forward and reset watermark value */
+	if (bp->caps & MACB_CAPS_PARTIAL_STORE_FORWARD) {
+		watermark_reset_value = (1 << (GEM_BFEXT(RX_PBUF_ADDR, gem_readl(bp, DCFG2)))) - 1;
+		gem_writel(bp, PBUFRXCUT, watermark_reset_value);
+	}
+
 	/* Disable all interrupts */
 	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
 		queue_writel(queue, IDR, -1);
@@ -2743,6 +2750,8 @@ static void macb_configure_dma(struct macb *bp)
 
 static void macb_init_hw(struct macb *bp)
 {
+	u16 watermark_reset_value;
+	u16 watermark_value;
 	u32 config;
 
 	macb_reset_hw(bp);
@@ -2770,6 +2779,14 @@ static void macb_init_hw(struct macb *bp)
 		bp->rx_frm_len_mask = MACB_RX_JFRMLEN_MASK;
 
 	macb_configure_dma(bp);
+
+	/* Enable RX partial store and forward and set watermark */
+	if ((bp->caps & MACB_CAPS_PARTIAL_STORE_FORWARD) && bp->rx_watermark) {
+		watermark_reset_value = (1 << (GEM_BFEXT(RX_PBUF_ADDR, gem_readl(bp, DCFG2)))) - 1;
+		watermark_value = bp->rx_watermark & watermark_reset_value;
+		gem_writel(bp, PBUFRXCUT,
+			   (watermark_value | GEM_BIT(ENCUTTHRU)));
+	}
 }
 
 /* The hash address register is 64 bits long and takes up two
@@ -3861,11 +3878,37 @@ static const struct net_device_ops macb_netdev_ops = {
 static void macb_configure_caps(struct macb *bp,
 				const struct macb_config *dt_conf)
 {
+	u32 wtrmrk_rst_val;
+	int retval;
 	u32 dcfg;
 
 	if (dt_conf)
 		bp->caps = dt_conf->caps;
 
+	/* By default we set to partial store and forward mode for zynqmp.
+	 * Disable if not set in devicetree.
+	 */
+	if (GEM_BFEXT(PBUF_CUTTHRU, gem_readl(bp, DCFG6))) {
+		if (bp->caps & MACB_CAPS_PARTIAL_STORE_FORWARD) {
+			retval = of_property_read_u16(bp->pdev->dev.of_node,
+						      "rx-watermark",
+						      &bp->rx_watermark);
+
+			/* Disable partial store and forward in case of error or
+			 * invalid watermark value
+			 */
+			wtrmrk_rst_val = (1 << (GEM_BFEXT(RX_PBUF_ADDR, gem_readl(bp, DCFG2)))) - 1;
+			if (retval || bp->rx_watermark > wtrmrk_rst_val || !bp->rx_watermark) {
+				if (bp->rx_watermark > wtrmrk_rst_val) {
+					dev_info(&bp->pdev->dev, "Invalid watermark value\n");
+					bp->rx_watermark = 0;
+				}
+				dev_info(&bp->pdev->dev, "Not enabling partial store and forward\n");
+				bp->caps &= ~MACB_CAPS_PARTIAL_STORE_FORWARD;
+			}
+		}
+	}
+
 	if (hw_is_gem(bp->regs, bp->native_io)) {
 		bp->caps |= MACB_CAPS_MACB_IS_GEM;
 
@@ -4813,7 +4856,8 @@ static const struct macb_config np4_config = {
 static const struct macb_config zynqmp_config = {
 	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE |
 		MACB_CAPS_JUMBO |
-		MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH,
+		MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH |
+		MACB_CAPS_PARTIAL_STORE_FORWARD,
 	.dma_burst_length = 16,
 	.clk_init = macb_clk_init,
 	.init = init_reset_optional,
@@ -4861,7 +4905,8 @@ static const struct macb_config sama7g5_emac_config = {
 
 static const struct macb_config versal_config = {
 	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO |
-		MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH | MACB_CAPS_NEED_TSUCLK,
+		MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH |
+		MACB_CAPS_NEED_TSUCLK | MACB_CAPS_PARTIAL_STORE_FORWARD,
 	.dma_burst_length = 16,
 	.clk_init = macb_clk_init,
 	.init = init_reset_optional,
-- 
2.36.1

Re: [PATCH net-next v2 1/2] dt-bindings: net: cdns,macb: Add rx-watermark property

[Conor Dooley]

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

On Thu, May 11, 2023 at 01:12:13AM -0600, Pranavi Somisetty wrote:
> watermark value is the minimum amount of packet data
> required to activate the forwarding process. The watermark
> implementation and maximum size is dependent on the device
> where Cadence MACB/GEM is used.
> 
> Signed-off-by: Pranavi Somisetty <pranavi.somisetty@amd.com>

Please send dt-binding patches to the dt-binding maintainers and list.
get_maintainer.pl should have told you to do so & without having done
so, the bindings will not get tested :/

Thanks,
Conor.

> ---
>  Documentation/devicetree/bindings/net/cdns,macb.yaml | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/cdns,macb.yaml b/Documentation/devicetree/bindings/net/cdns,macb.yaml
> index bef5e0f895be..779bc25cf005 100644
> --- a/Documentation/devicetree/bindings/net/cdns,macb.yaml
> +++ b/Documentation/devicetree/bindings/net/cdns,macb.yaml
> @@ -109,6 +109,13 @@ properties:
>    power-domains:
>      maxItems: 1
>  
> +  rx-watermark:
> +    maxItems: 1
> +    $ref: /schemas/types.yaml#/definitions/uint16
> +    description:
> +      Set watermark value for pbuf_rxcutthru reg and enable
> +      rx partial store and forward.
> +
>    '#address-cells':
>      const: 1
>  
> @@ -166,6 +173,7 @@ examples:
>              compatible = "cdns,macb";
>              reg = <0xfffc4000 0x4000>;
>              interrupts = <21>;
> +            rx-watermark = /bits/ 16 <0x44>;
>              phy-mode = "rmii";
>              local-mac-address = [3a 0e 03 04 05 06];
>              clock-names = "pclk", "hclk", "tx_clk";
> -- 
> 2.36.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net-next v2 2/2] net: macb: Add support for partial store and forward

[Conor Dooley]

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

On Thu, May 11, 2023 at 01:12:14AM -0600, Pranavi Somisetty wrote:
> When the receive partial store and forward mode is activated, the
> receiver will only begin to forward the packet to the external AHB
> or AXI slave when enough packet data is stored in the packet buffer.
> The amount of packet data required to activate the forwarding process
> is programmable via watermark registers which are located at the same
> address as the partial store and forward enable bits. Adding support to
> read this rx-watermark value from device-tree, to program the watermark
> registers and enable partial store and forwarding.
> 
> Signed-off-by: Maulik Jodhani <maulik.jodhani@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> Signed-off-by: Harini Katakam <harini.katakam@xilinx.com>
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
> Signed-off-by: Pranavi Somisetty <pranavi.somisetty@amd.com>

In passing, this looks like an odd SoB chain. You are the patch's author
and the sender, what did everyone else here do?
If these people helped with the patch, should they have Co-developed-by:
lines too?

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net-next v2 2/2] net: macb: Add support for partial store and forward

[Simon Horman]

On Thu, May 11, 2023 at 01:12:14AM -0600, Pranavi Somisetty wrote:
> When the receive partial store and forward mode is activated, the
> receiver will only begin to forward the packet to the external AHB
> or AXI slave when enough packet data is stored in the packet buffer.
> The amount of packet data required to activate the forwarding process
> is programmable via watermark registers which are located at the same
> address as the partial store and forward enable bits. Adding support to
> read this rx-watermark value from device-tree, to program the watermark
> registers and enable partial store and forwarding.
> 
> Signed-off-by: Maulik Jodhani <maulik.jodhani@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> Signed-off-by: Harini Katakam <harini.katakam@xilinx.com>
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
> Signed-off-by: Pranavi Somisetty <pranavi.somisetty@amd.com>
> ---
> Changes v2:
> 
> 1. Removed all the changes related to validating FCS when Rx checksum offload is disabled.
> 2. Instead of using a platform dependent number (0xFFF) for the reset value of rx watermark,
> derive it from designcfg_debug2 register.
> 3. Added a check to see if partial s/f is supported, by reading the
> designcfg_debug6 register.
> ---
>  drivers/net/ethernet/cadence/macb.h      | 14 +++++++
>  drivers/net/ethernet/cadence/macb_main.c | 49 +++++++++++++++++++++++-
>  2 files changed, 61 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 14dfec4db8f9..46833662094d 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -82,6 +82,7 @@
>  #define GEM_NCFGR		0x0004 /* Network Config */
>  #define GEM_USRIO		0x000c /* User IO */
>  #define GEM_DMACFG		0x0010 /* DMA Configuration */
> +#define GEM_PBUFRXCUT		0x0044 /* RX Partial Store and Forward */
>  #define GEM_JML			0x0048 /* Jumbo Max Length */
>  #define GEM_HS_MAC_CONFIG	0x0050 /* GEM high speed config */
>  #define GEM_HRB			0x0080 /* Hash Bottom */
> @@ -343,6 +344,11 @@
>  #define GEM_ADDR64_SIZE		1
>  
>  
> +/* Bitfields in PBUFRXCUT */
> +#define GEM_WTRMRK_OFFSET	0 /* Watermark value offset */
> +#define GEM_ENCUTTHRU_OFFSET	31 /* Enable RX partial store and forward */
> +#define GEM_ENCUTTHRU_SIZE	1
> +
>  /* Bitfields in NSR */
>  #define MACB_NSR_LINK_OFFSET	0 /* pcs_link_state */
>  #define MACB_NSR_LINK_SIZE	1
> @@ -509,6 +515,8 @@
>  #define GEM_TX_PKT_BUFF_OFFSET			21
>  #define GEM_TX_PKT_BUFF_SIZE			1
>  
> +#define GEM_RX_PBUF_ADDR_OFFSET			22
> +#define GEM_RX_PBUF_ADDR_SIZE			4
>  
>  /* Bitfields in DCFG5. */
>  #define GEM_TSU_OFFSET				8
> @@ -517,6 +525,8 @@
>  /* Bitfields in DCFG6. */
>  #define GEM_PBUF_LSO_OFFSET			27
>  #define GEM_PBUF_LSO_SIZE			1
> +#define GEM_PBUF_CUTTHRU_OFFSET			26
> +#define GEM_PBUF_CUTTHRU_SIZE			1
>  #define GEM_DAW64_OFFSET			23
>  #define GEM_DAW64_SIZE				1
>  
> @@ -718,6 +728,7 @@
>  #define MACB_CAPS_NEEDS_RSTONUBR		0x00000100
>  #define MACB_CAPS_MIIONRGMII			0x00000200
>  #define MACB_CAPS_NEED_TSUCLK			0x00000400
> +#define MACB_CAPS_PARTIAL_STORE_FORWARD		0x00000800
>  #define MACB_CAPS_PCS				0x01000000
>  #define MACB_CAPS_HIGH_SPEED			0x02000000
>  #define MACB_CAPS_CLK_HW_CHG			0x04000000
> @@ -1283,6 +1294,9 @@ struct macb {
>  
>  	u32			wol;
>  
> +	/* holds value of rx watermark value for pbuf_rxcutthru register */
> +	u16			rx_watermark;
> +
>  	struct macb_ptp_info	*ptp_info;	/* macb-ptp interface */
>  
>  	struct phy		*sgmii_phy;	/* for ZynqMP SGMII mode */
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 41964fd02452..07b9964e7aa3 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -2600,6 +2600,7 @@ static void macb_init_rings(struct macb *bp)
>  static void macb_reset_hw(struct macb *bp)
>  {
>  	struct macb_queue *queue;
> +	u16 watermark_reset_value;
>  	unsigned int q;
>  	u32 ctrl = macb_readl(bp, NCR);

Given the review of Conor Dooley of both patches in this series I think
there will be a v3.

Please consider adjusting the hunk above to move towards rather than
away from reverse xmas tree - longest line to shortest - for local
variable names.

That would be:

	u16 watermark_reset_value;
	struct macb_queue *queue;
	...

pw-bot: cr

Re: [PATCH net-next v2 1/2] dt-bindings: net: cdns,macb: Add rx-watermark property

[Krzysztof Kozlowski]

On 11/05/2023 09:12, Pranavi Somisetty wrote:
> watermark value is the minimum amount of packet data
> required to activate the forwarding process. The watermark
> implementation and maximum size is dependent on the device
> where Cadence MACB/GEM is used.
> 
> Signed-off-by: Pranavi Somisetty <pranavi.somisetty@amd.com>

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC.  It might happen, that command when run on an older
kernel, gives you outdated entries.  Therefore please be sure you base
your patches on recent Linux kernel.

You missed at least DT list (maybe more), so this won't be tested.
Please resend and include all necessary entries.

Best regards,
Krzysztof

Re: [PATCH net-next v2 1/2] dt-bindings: net: cdns,macb: Add rx-watermark property

[Andrew Lunn]

On Thu, May 11, 2023 at 01:12:13AM -0600, Pranavi Somisetty wrote:
> watermark value is the minimum amount of packet data
> required to activate the forwarding process. The watermark
> implementation and maximum size is dependent on the device
> where Cadence MACB/GEM is used.
> 
> Signed-off-by: Pranavi Somisetty <pranavi.somisetty@amd.com>
> ---
>  Documentation/devicetree/bindings/net/cdns,macb.yaml | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/cdns,macb.yaml b/Documentation/devicetree/bindings/net/cdns,macb.yaml
> index bef5e0f895be..779bc25cf005 100644
> --- a/Documentation/devicetree/bindings/net/cdns,macb.yaml
> +++ b/Documentation/devicetree/bindings/net/cdns,macb.yaml
> @@ -109,6 +109,13 @@ properties:
>    power-domains:
>      maxItems: 1
>  
> +  rx-watermark:
> +    maxItems: 1
> +    $ref: /schemas/types.yaml#/definitions/uint16
> +    description:
> +      Set watermark value for pbuf_rxcutthru reg and enable
> +      rx partial store and forward.

What are the units? Frames, octets, DMA scatter-gather segments, % of
maximum?

	Andrew

---
pw-bot: cr

Re: [PATCH net-next v2 2/2] net: macb: Add support for partial store and forward

[Andrew Lunn]

> +	if (GEM_BFEXT(PBUF_CUTTHRU, gem_readl(bp, DCFG6))) {
> +		if (bp->caps & MACB_CAPS_PARTIAL_STORE_FORWARD) {
> +			retval = of_property_read_u16(bp->pdev->dev.of_node,
> +						      "rx-watermark",
> +						      &bp->rx_watermark);
> +
> +			/* Disable partial store and forward in case of error or
> +			 * invalid watermark value
> +			 */
> +			wtrmrk_rst_val = (1 << (GEM_BFEXT(RX_PBUF_ADDR, gem_readl(bp, DCFG2)))) - 1;
> +			if (retval || bp->rx_watermark > wtrmrk_rst_val || !bp->rx_watermark) {
> +				if (bp->rx_watermark > wtrmrk_rst_val) {
> +					dev_info(&bp->pdev->dev, "Invalid watermark value\n");
> +					bp->rx_watermark = 0;

Please return -EINVAL. We want the DT author to fix their error.

> +				}
> +				dev_info(&bp->pdev->dev, "Not enabling partial store and forward\n");

The DT property is optional? So when it is missing, retval will be
-EINVAL. Please don't spam the logs in this case.

	 Andrew

Re: [PATCH net-next v2 1/2] dt-bindings: net: cdns,macb: Add rx-watermark property

[Conor Dooley]

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

On Thu, May 11, 2023 at 08:25:15AM +0100, Conor Dooley wrote:
> On Thu, May 11, 2023 at 01:12:13AM -0600, Pranavi Somisetty wrote:
> > watermark value is the minimum amount of packet data
> > required to activate the forwarding process. The watermark
> > implementation and maximum size is dependent on the device
> > where Cadence MACB/GEM is used.
> > 
> > Signed-off-by: Pranavi Somisetty <pranavi.somisetty@amd.com>
> 
> Please send dt-binding patches to the dt-binding maintainers and list.
> get_maintainer.pl should have told you to do so & without having done
> so, the bindings will not get tested :/

The automation we run for the linux-riscv did actually run against
this though, and even dtbs_check complains about this binding:

Documentation/devicetree/bindings/net/cdns,macb.yaml: properties:rx-watermark:maxItems: False schema does not allow 1
	hint: Scalar properties should not have array keywords
	from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#

Please test your bindings :/

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net-next v2 2/2] net: macb: Add support for partial store and forward

[Claudiu.Beznea]

On 11.05.2023 10:12, Pranavi Somisetty wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> When the receive partial store and forward mode is activated, the
> receiver will only begin to forward the packet to the external AHB
> or AXI slave when enough packet data is stored in the packet buffer.
> The amount of packet data required to activate the forwarding process
> is programmable via watermark registers which are located at the same
> address as the partial store and forward enable bits. Adding support to
> read this rx-watermark value from device-tree, to program the watermark
> registers and enable partial store and forwarding.
> 
> Signed-off-by: Maulik Jodhani <maulik.jodhani@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> Signed-off-by: Harini Katakam <harini.katakam@xilinx.com>
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
> Signed-off-by: Pranavi Somisetty <pranavi.somisetty@amd.com>
> ---
> Changes v2:
> 
> 1. Removed all the changes related to validating FCS when Rx checksum offload is disabled.
> 2. Instead of using a platform dependent number (0xFFF) for the reset value of rx watermark,
> derive it from designcfg_debug2 register.
> 3. Added a check to see if partial s/f is supported, by reading the
> designcfg_debug6 register.
> ---
>  drivers/net/ethernet/cadence/macb.h      | 14 +++++++
>  drivers/net/ethernet/cadence/macb_main.c | 49 +++++++++++++++++++++++-
>  2 files changed, 61 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 14dfec4db8f9..46833662094d 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -82,6 +82,7 @@
>  #define GEM_NCFGR              0x0004 /* Network Config */
>  #define GEM_USRIO              0x000c /* User IO */
>  #define GEM_DMACFG             0x0010 /* DMA Configuration */
> +#define GEM_PBUFRXCUT          0x0044 /* RX Partial Store and Forward */
>  #define GEM_JML                        0x0048 /* Jumbo Max Length */
>  #define GEM_HS_MAC_CONFIG      0x0050 /* GEM high speed config */
>  #define GEM_HRB                        0x0080 /* Hash Bottom */
> @@ -343,6 +344,11 @@
>  #define GEM_ADDR64_SIZE                1
> 
> 
> +/* Bitfields in PBUFRXCUT */
> +#define GEM_WTRMRK_OFFSET      0 /* Watermark value offset */
> +#define GEM_ENCUTTHRU_OFFSET   31 /* Enable RX partial store and forward */
> +#define GEM_ENCUTTHRU_SIZE     1
> +
>  /* Bitfields in NSR */
>  #define MACB_NSR_LINK_OFFSET   0 /* pcs_link_state */
>  #define MACB_NSR_LINK_SIZE     1
> @@ -509,6 +515,8 @@
>  #define GEM_TX_PKT_BUFF_OFFSET                 21
>  #define GEM_TX_PKT_BUFF_SIZE                   1
> 
> +#define GEM_RX_PBUF_ADDR_OFFSET                        22
> +#define GEM_RX_PBUF_ADDR_SIZE                  4
> 
>  /* Bitfields in DCFG5. */
>  #define GEM_TSU_OFFSET                         8
> @@ -517,6 +525,8 @@
>  /* Bitfields in DCFG6. */
>  #define GEM_PBUF_LSO_OFFSET                    27
>  #define GEM_PBUF_LSO_SIZE                      1
> +#define GEM_PBUF_CUTTHRU_OFFSET                        26
> +#define GEM_PBUF_CUTTHRU_SIZE                  1
>  #define GEM_DAW64_OFFSET                       23
>  #define GEM_DAW64_SIZE                         1
> 
> @@ -718,6 +728,7 @@
>  #define MACB_CAPS_NEEDS_RSTONUBR               0x00000100
>  #define MACB_CAPS_MIIONRGMII                   0x00000200
>  #define MACB_CAPS_NEED_TSUCLK                  0x00000400
> +#define MACB_CAPS_PARTIAL_STORE_FORWARD                0x00000800
>  #define MACB_CAPS_PCS                          0x01000000
>  #define MACB_CAPS_HIGH_SPEED                   0x02000000
>  #define MACB_CAPS_CLK_HW_CHG                   0x04000000
> @@ -1283,6 +1294,9 @@ struct macb {
> 
>         u32                     wol;
> 
> +       /* holds value of rx watermark value for pbuf_rxcutthru register */
> +       u16                     rx_watermark;
> +
>         struct macb_ptp_info    *ptp_info;      /* macb-ptp interface */
> 
>         struct phy              *sgmii_phy;     /* for ZynqMP SGMII mode */
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 41964fd02452..07b9964e7aa3 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -2600,6 +2600,7 @@ static void macb_init_rings(struct macb *bp)
>  static void macb_reset_hw(struct macb *bp)
>  {
>         struct macb_queue *queue;
> +       u16 watermark_reset_value;
>         unsigned int q;
>         u32 ctrl = macb_readl(bp, NCR);
> 
> @@ -2617,6 +2618,12 @@ static void macb_reset_hw(struct macb *bp)
>         macb_writel(bp, TSR, -1);
>         macb_writel(bp, RSR, -1);
> 
> +       /* Disable RX partial store and forward and reset watermark value */
> +       if (bp->caps & MACB_CAPS_PARTIAL_STORE_FORWARD) {
> +               watermark_reset_value = (1 << (GEM_BFEXT(RX_PBUF_ADDR, gem_readl(bp, DCFG2)))) - 1;

Is this block needed? Maybe all you need here is just to disable the rx
partial store and forward?

> +               gem_writel(bp, PBUFRXCUT, watermark_reset_value);
> +       }
> +
>         /* Disable all interrupts */
>         for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
>                 queue_writel(queue, IDR, -1);
> @@ -2743,6 +2750,8 @@ static void macb_configure_dma(struct macb *bp)
> 
>  static void macb_init_hw(struct macb *bp)
>  {
> +       u16 watermark_reset_value;
> +       u16 watermark_value;
>         u32 config;
> 
>         macb_reset_hw(bp);
> @@ -2770,6 +2779,14 @@ static void macb_init_hw(struct macb *bp)
>                 bp->rx_frm_len_mask = MACB_RX_JFRMLEN_MASK;
> 
>         macb_configure_dma(bp);
> +
> +       /* Enable RX partial store and forward and set watermark */
> +       if ((bp->caps & MACB_CAPS_PARTIAL_STORE_FORWARD) && bp->rx_watermark) {
> +               watermark_reset_value = (1 << (GEM_BFEXT(RX_PBUF_ADDR, gem_readl(bp, DCFG2)))) - 1;
> +               watermark_value = bp->rx_watermark & watermark_reset_value;

You should validate the value in bp->rx_watermark in probe and here just
update the PBUFRXCUT with that value.

> +               gem_writel(bp, PBUFRXCUT,
> +                          (watermark_value | GEM_BIT(ENCUTTHRU)));
> +       }
>  }
> 
>  /* The hash address register is 64 bits long and takes up two
> @@ -3861,11 +3878,37 @@ static const struct net_device_ops macb_netdev_ops = {
>  static void macb_configure_caps(struct macb *bp,
>                                 const struct macb_config *dt_conf)
>  {
> +       u32 wtrmrk_rst_val;
> +       int retval;
>         u32 dcfg;
> 
>         if (dt_conf)
>                 bp->caps = dt_conf->caps;
> 
> +       /* By default we set to partial store and forward mode for zynqmp.
> +        * Disable if not set in devicetree.
> +        */
> +       if (GEM_BFEXT(PBUF_CUTTHRU, gem_readl(bp, DCFG6))) {
> +               if (bp->caps & MACB_CAPS_PARTIAL_STORE_FORWARD) {

You can get rid of MACB_CAPS_PARTIAL_STORE_FORWARD and consider it enabled
or not based on device tree rx-watermark dt property. Thus you can have
here only:
	if (GEM_BFEXT(PBUF_CUTTHRU, gem_readl(bp, DCFG6)))

and based on the validity of data passed to "rx-watermark" the
bp->rx_watermark will be zero or not. You can check bp->rx_watermark all
over the code to check if rx partial store and fw is enabled.

> +                       retval = of_property_read_u16(bp->pdev->dev.of_node,
> +                                                     "rx-watermark",
> +                                                     &bp->rx_watermark);

E.g. SAMA7G5 has PBUFRXCUT.watermark on 10 bits. Is it the same on Xynqmp?
For compatibility with future implementations and stable DT interface it
would be better to just keep rx-watermark DT property on 32 bits.

> +
> +                       /* Disable partial store and forward in case of error or
> +                        * invalid watermark value
> +                        */
> +                       wtrmrk_rst_val = (1 << (GEM_BFEXT(RX_PBUF_ADDR, gem_readl(bp, DCFG2)))) - 1;
> +                       if (retval || bp->rx_watermark > wtrmrk_rst_val || !bp->rx_watermark) {
> +                               if (bp->rx_watermark > wtrmrk_rst_val) {
> +                                       dev_info(&bp->pdev->dev, "Invalid watermark value\n");
> +                                       bp->rx_watermark = 0;

Checking this in the code should be enough. There is no need to introduce a
new capability.

> +                               }
> +                               dev_info(&bp->pdev->dev, "Not enabling partial store and forward\n");
> +                               bp->caps &= ~MACB_CAPS_PARTIAL_STORE_FORWARD;
> +                       }
> +               }
> +       }
> +
>         if (hw_is_gem(bp->regs, bp->native_io)) {
>                 bp->caps |= MACB_CAPS_MACB_IS_GEM;
> 
> @@ -4813,7 +4856,8 @@ static const struct macb_config np4_config = {
>  static const struct macb_config zynqmp_config = {
>         .caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE |
>                 MACB_CAPS_JUMBO |
> -               MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH,
> +               MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH |
> +               MACB_CAPS_PARTIAL_STORE_FORWARD,
>         .dma_burst_length = 16,
>         .clk_init = macb_clk_init,
>         .init = init_reset_optional,
> @@ -4861,7 +4905,8 @@ static const struct macb_config sama7g5_emac_config = {
> 
>  static const struct macb_config versal_config = {
>         .caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO |
> -               MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH | MACB_CAPS_NEED_TSUCLK,
> +               MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH |
> +               MACB_CAPS_NEED_TSUCLK | MACB_CAPS_PARTIAL_STORE_FORWARD,
>         .dma_burst_length = 16,
>         .clk_init = macb_clk_init,
>         .init = init_reset_optional,
> --
> 2.36.1
> 

RE: [PATCH net-next v2 2/2] net: macb: Add support for partial store and forward

[Somisetty, Pranavi]

> -----Original Message-----
> From: Claudiu.Beznea@microchip.com <Claudiu.Beznea@microchip.com>
> Sent: Friday, May 12, 2023 1:28 PM
> To: Somisetty, Pranavi <pranavi.somisetty@amd.com>;
> Nicolas.Ferre@microchip.com; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> richardcochran@gmail.com; linux@armlinux.org.uk; palmer@dabbelt.com
> Cc: git (AMD-Xilinx) <git@amd.com>; Simek, Michal
> <michal.simek@amd.com>; Katakam, Harini <harini.katakam@amd.com>;
> Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> riscv@lists.infradead.org
> Subject: Re: [PATCH net-next v2 2/2] net: macb: Add support for partial store
> and forward
> 
> On 11.05.2023 10:12, Pranavi Somisetty wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know
> > the content is safe
> >
> > When the receive partial store and forward mode is activated, the
> > receiver will only begin to forward the packet to the external AHB or
> > AXI slave when enough packet data is stored in the packet buffer.
> > The amount of packet data required to activate the forwarding process
> > is programmable via watermark registers which are located at the same
> > address as the partial store and forward enable bits. Adding support
> > to read this rx-watermark value from device-tree, to program the
> > watermark registers and enable partial store and forwarding.
> >
> > Signed-off-by: Maulik Jodhani <maulik.jodhani@xilinx.com>
> > Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> > Signed-off-by: Harini Katakam <harini.katakam@xilinx.com>
> > Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
> > Signed-off-by: Pranavi Somisetty <pranavi.somisetty@amd.com>
> > ---

<snip>
> > +       /* Disable RX partial store and forward and reset watermark value */
> > +       if (bp->caps & MACB_CAPS_PARTIAL_STORE_FORWARD) {
> > +               watermark_reset_value = (1 << (GEM_BFEXT(RX_PBUF_ADDR,
> > + gem_readl(bp, DCFG2)))) - 1;
> 
> Is this block needed? Maybe all you need here is just to disable the rx partial
> store and forward?

Yes, the value doesn’t need to be written, disabling rx partial store and forward
is enough, will take care.
<snip>
> 
> > +                       retval = of_property_read_u16(bp->pdev->dev.of_node,
> > +                                                     "rx-watermark",
> > +
> > + &bp->rx_watermark);
> 
> E.g. SAMA7G5 has PBUFRXCUT.watermark on 10 bits. Is it the same on
> Xynqmp?

On ZynqMP its 12 bits.

> For compatibility with future implementations and stable DT interface it
> would be better to just keep rx-watermark DT property on 32 bits.
> 

I don’t think it can extend beyond u16, considering, the maximum pbuf depth
field in DCFG2, is only 4 bits (22:25). 

I'll do a re-spin addressing your and other reviewer's comments.

Thanks
Pranavi