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

[PATCH net-next v3 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.
Link for v2:
https://lore.kernel.org/all/20230511071214.18611-1-pranavi.somisetty@amd.com/

Changes v3:
1. Fixed DT schema error: "scalar properties shouldn't have array keywords"
2. Modified description of rx-watermark in to include units of the watermark value
3. Modified the DT property name corresponding to rx_watermark in pbuf_rxcutthru to
"cdns,rx-watermark".
4. Followed reverse christmas tree pattern in declaring variables.
5. Return -EINVAL when an invalid watermark value is set.
6. Removed netdev_info when partial store and forward is not enabled.
7. Validating the rx-watermark value in probe itself and only write to the register
in init.
8. Writing a reset value to the pbuf_cuthru register before disabing partial store
and forward is redundant. So removing it.
9. Removed the platform caps flag.
10. Instead of reading rx-watermark from DT in macb_configure_caps,
reading it in probe.
11. Changed Signed-Off-By and author names on the macb driver patch.

Maulik Jodhani (1):
  net: macb: Add support for partial store and forward

Pranavi Somisetty (1):
  dt-bindings: net: cdns,macb: Add rx-watermark property

 .../devicetree/bindings/net/cdns,macb.yaml    |  9 ++++++
 drivers/net/ethernet/cadence/macb.h           | 14 +++++++++
 drivers/net/ethernet/cadence/macb_main.c      | 29 +++++++++++++++++++
 3 files changed, 52 insertions(+)

-- 
2.36.1

[PATCH net-next v3 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>
---
Changes v2:
None (patch added in v2)

Changes v3:
1. Fixed DT schema error: "scalar properties shouldn't have array keywords".
2. Modified description of rx-watermark to include units of the watermark value.
3. Modified the DT property name corresponding to rx_watermark in
pbuf_rxcutthru to "cdns,rx-watermark".
4. Modified commit description to remove references to Xilinx platforms,
since the changes aren't platform specific.
--- 
 Documentation/devicetree/bindings/net/cdns,macb.yaml | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/cdns,macb.yaml b/Documentation/devicetree/bindings/net/cdns,macb.yaml
index bef5e0f895be..2c733c061dce 100644
--- a/Documentation/devicetree/bindings/net/cdns,macb.yaml
+++ b/Documentation/devicetree/bindings/net/cdns,macb.yaml
@@ -109,6 +109,14 @@ properties:
   power-domains:
     maxItems: 1
 
+  cdns,rx-watermark:
+    $ref: /schemas/types.yaml#/definitions/uint16
+    description:
+      Set watermark value for pbuf_rxcutthru reg and enable
+      rx partial store and forward. Watermark value here
+      corresponds to number of SRAM locations. The width of SRAM is
+      system dependent and can be 4,8 or 16 bytes.
+
   '#address-cells':
     const: 1
 
@@ -166,6 +174,7 @@ examples:
             compatible = "cdns,macb";
             reg = <0xfffc4000 0x4000>;
             interrupts = <21>;
+            cdns,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

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

[Krzysztof Kozlowski]

On Tue, 30 May 2023 03:51:37 -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>
> ---
> Changes v2:
> None (patch added in v2)
> 
> Changes v3:
> 1. Fixed DT schema error: "scalar properties shouldn't have array keywords".
> 2. Modified description of rx-watermark to include units of the watermark value.
> 3. Modified the DT property name corresponding to rx_watermark in
> pbuf_rxcutthru to "cdns,rx-watermark".
> 4. Modified commit description to remove references to Xilinx platforms,
> since the changes aren't platform specific.
> ---
>  Documentation/devicetree/bindings/net/cdns,macb.yaml | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 

Running 'make dtbs_check' with the schema in this patch gives the
following warnings. Consider if they are expected or the schema is
incorrect. These may not be new warnings.

Note that it is not yet a requirement to have 0 warnings for dtbs_check.
This will change in the future.

Full log is available here: https://patchwork.ozlabs.org/patch/1787378


ethernet@e000b000: ethernet-phy@0: Unevaluated properties are not allowed ('device_type', 'marvell,reg-init' were unexpected)
	arch/arm/boot/dts/zynq-parallella.dtb

ethernet@e000b000: ethernet-phy@0: Unevaluated properties are not allowed ('device_type' was unexpected)
	arch/arm/boot/dts/zynq-zed.dtb
	arch/arm/boot/dts/zynq-zybo.dtb
	arch/arm/boot/dts/zynq-zybo-z7.dtb

ethernet@e000b000: ethernet-phy@1: Unevaluated properties are not allowed ('device_type' was unexpected)
	arch/arm/boot/dts/zynq-cc108.dtb

ethernet@e000b000: ethernet-phy@7: Unevaluated properties are not allowed ('device_type' was unexpected)
	arch/arm/boot/dts/zynq-zc702.dtb
	arch/arm/boot/dts/zynq-zc706.dtb
	arch/arm/boot/dts/zynq-zc770-xm010.dtb

ethernet@e000c000: ethernet-phy@7: Unevaluated properties are not allowed ('device_type' was unexpected)
	arch/arm/boot/dts/zynq-zc770-xm013.dtb

ethernet@f0028000: ethernet-phy@1: Unevaluated properties are not allowed ('rxc-skew-ps', 'rxd0-skew-ps', 'rxd1-skew-ps', 'rxd2-skew-ps', 'rxd3-skew-ps', 'rxdv-skew-ps', 'txc-skew-ps', 'txen-skew-ps' were unexpected)
	arch/arm/boot/dts/sama5d33ek.dtb
	arch/arm/boot/dts/sama5d34ek.dtb
	arch/arm/boot/dts/sama5d35ek.dtb
	arch/arm/boot/dts/sama5d36ek_cmp.dtb
	arch/arm/boot/dts/sama5d36ek.dtb

ethernet@f0028000: ethernet-phy@7: Unevaluated properties are not allowed ('rxc-skew-ps', 'rxd0-skew-ps', 'rxd1-skew-ps', 'rxd2-skew-ps', 'rxd3-skew-ps', 'rxdv-skew-ps', 'txc-skew-ps', 'txen-skew-ps' were unexpected)
	arch/arm/boot/dts/at91-dvk_som60.dtb
	arch/arm/boot/dts/sama5d33ek.dtb
	arch/arm/boot/dts/sama5d34ek.dtb
	arch/arm/boot/dts/sama5d35ek.dtb
	arch/arm/boot/dts/sama5d36ek_cmp.dtb
	arch/arm/boot/dts/sama5d36ek.dtb

ethernet@ff0d0000: ethernet-phy@5: Unevaluated properties are not allowed ('ti,dp83867-rxctrl-strap-quirk', 'ti,fifo-depth', 'ti,rx-internal-delay', 'ti,tx-internal-delay' were unexpected)
	arch/arm64/boot/dts/xilinx/zynqmp-zc1751-xm016-dc2.dtb

ethernet@ff0e0000: ethernet-phy@c: Unevaluated properties are not allowed ('ti,dp83867-rxctrl-strap-quirk', 'ti,fifo-depth', 'ti,rx-internal-delay', 'ti,tx-internal-delay' were unexpected)
	arch/arm64/boot/dts/xilinx/zynqmp-zcu102-rev1.0.dtb
	arch/arm64/boot/dts/xilinx/zynqmp-zcu102-rev1.1.dtb
	arch/arm64/boot/dts/xilinx/zynqmp-zcu102-revB.dtb
	arch/arm64/boot/dts/xilinx/zynqmp-zcu104-revA.dtb
	arch/arm64/boot/dts/xilinx/zynqmp-zcu104-revC.dtb
	arch/arm64/boot/dts/xilinx/zynqmp-zcu106-revA.dtb
	arch/arm64/boot/dts/xilinx/zynqmp-zcu111-revA.dtb

ethernet@ff0e0000: Unevaluated properties are not allowed ('ethernet-phy@21' was unexpected)
	arch/arm64/boot/dts/xilinx/zynqmp-zcu102-revA.dtb

ethernet@fffbc000: Unevaluated properties are not allowed ('ethernet-phy' was unexpected)
	arch/arm/boot/dts/at91rm9200ek.dtb

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

[Krzysztof Kozlowski]

On 30/05/2023 14:25, Krzysztof Kozlowski wrote:
> On Tue, 30 May 2023 03:51:37 -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>
>> ---
>> Changes v2:
>> None (patch added in v2)
>>
>> Changes v3:
>> 1. Fixed DT schema error: "scalar properties shouldn't have array keywords".
>> 2. Modified description of rx-watermark to include units of the watermark value.
>> 3. Modified the DT property name corresponding to rx_watermark in
>> pbuf_rxcutthru to "cdns,rx-watermark".
>> 4. Modified commit description to remove references to Xilinx platforms,
>> since the changes aren't platform specific.
>> ---
>>  Documentation/devicetree/bindings/net/cdns,macb.yaml | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
> 
> Running 'make dtbs_check' with the schema in this patch gives the
> following warnings. Consider if they are expected or the schema is
> incorrect. These may not be new warnings.
> 
> Note that it is not yet a requirement to have 0 warnings for dtbs_check.
> This will change in the future.
> 
> Full log is available here: https://patchwork.ozlabs.org/patch/1787378
> 
> 
> ethernet@e000b000: ethernet-phy@0: Unevaluated properties are not allowed ('device_type', 'marvell,reg-init' were unexpected)
> 	arch/arm/boot/dts/zynq-parallella.dtb
> 

Unrelated warnings, can be ignored.

Best regards,
Krzysztof

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

[Claudiu.Beznea]

On 30.05.2023 12:51, Pranavi Somisetty wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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>
> ---
> Changes v2:
> None (patch added in v2)
> 
> Changes v3:
> 1. Fixed DT schema error: "scalar properties shouldn't have array keywords".
> 2. Modified description of rx-watermark to include units of the watermark value.
> 3. Modified the DT property name corresponding to rx_watermark in
> pbuf_rxcutthru to "cdns,rx-watermark".
> 4. Modified commit description to remove references to Xilinx platforms,
> since the changes aren't platform specific.
> ---
>  Documentation/devicetree/bindings/net/cdns,macb.yaml | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/cdns,macb.yaml b/Documentation/devicetree/bindings/net/cdns,macb.yaml
> index bef5e0f895be..2c733c061dce 100644
> --- a/Documentation/devicetree/bindings/net/cdns,macb.yaml
> +++ b/Documentation/devicetree/bindings/net/cdns,macb.yaml
> @@ -109,6 +109,14 @@ properties:
>    power-domains:
>      maxItems: 1
> 
> +  cdns,rx-watermark:
> +    $ref: /schemas/types.yaml#/definitions/uint16

I still think keeping this on 32 bits is still a better idea to have this
DT property useable on future hardware implementation that may expand it.

> +    description:
> +      Set watermark value for pbuf_rxcutthru reg and enable
> +      rx partial store and forward. Watermark value here
> +      corresponds to number of SRAM locations. The width of SRAM is
> +      system dependent and can be 4,8 or 16 bytes.

s/4,8/4, 8

> +
>    '#address-cells':
>      const: 1
> 
> @@ -166,6 +174,7 @@ examples:
>              compatible = "cdns,macb";
>              reg = <0xfffc4000 0x4000>;
>              interrupts = <21>;
> +            cdns,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
> 

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

[Krzysztof Kozlowski]

On 30/05/2023 11:51, 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>
> ---
> Changes v2:
> None (patch added in v2)
> 
> Changes v3:
> 1. Fixed DT schema error: "scalar properties shouldn't have array keywords".
> 2. Modified description of rx-watermark to include units of the watermark value.
> 3. Modified the DT property name corresponding to rx_watermark in
> pbuf_rxcutthru to "cdns,rx-watermark".
> 4. Modified commit description to remove references to Xilinx platforms,
> since the changes aren't platform specific.
> --- 
>  Documentation/devicetree/bindings/net/cdns,macb.yaml | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/cdns,macb.yaml b/Documentation/devicetree/bindings/net/cdns,macb.yaml
> index bef5e0f895be..2c733c061dce 100644
> --- a/Documentation/devicetree/bindings/net/cdns,macb.yaml
> +++ b/Documentation/devicetree/bindings/net/cdns,macb.yaml
> @@ -109,6 +109,14 @@ properties:
>    power-domains:
>      maxItems: 1
>  
> +  cdns,rx-watermark:
> +    $ref: /schemas/types.yaml#/definitions/uint16
> +    description:
> +      Set watermark value for pbuf_rxcutthru reg and enable
> +      rx partial store and forward. Watermark value here
> +      corresponds to number of SRAM locations. The width of SRAM is
> +      system dependent and can be 4,8 or 16 bytes.

You described device programming model - registers - not the actual
hardware.

Best regards,
Krzysztof

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

[Pranavi Somisetty]

From: Maulik Jodhani <maulik.jodhani@xilinx.com>

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: 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.

Changes v3:
1. Followed reverse christmas tree pattern in declaring variables.
2. Return -EINVAL when an invalid watermark value is set.
3. Removed netdev_info when partial store and forward is not enabled.
4. Validating the rx-watermark value in probe itself and only write to the register
in init.
5. Writing a reset value to the pbuf_cuthru register before disabing partial store
and forward is redundant. So removing it.
6. Removed the platform caps flag.
7. Instead of reading rx-watermark from DT in macb_configure_caps,
reading it in probe.
8. Changed Signed-Off-By and author names on this patch.
---
 drivers/net/ethernet/cadence/macb.h      | 14 ++++++++++++
 drivers/net/ethernet/cadence/macb_main.c | 29 ++++++++++++++++++++++++
 2 files changed, 43 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 14dfec4db8f9..416e6070e4ec 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			25
+#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..7a31e6673e15 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -2617,6 +2617,9 @@ 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 */
+	gem_writel(bp, PBUFRXCUT, 0);
+
 	/* Disable all interrupts */
 	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
 		queue_writel(queue, IDR, -1);
@@ -2770,6 +2773,10 @@ 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->rx_watermark)
+		gem_writel(bp, PBUFRXCUT, (bp->rx_watermark | GEM_BIT(ENCUTTHRU)));
 }
 
 /* The hash address register is 64 bits long and takes up two
@@ -4923,6 +4930,7 @@ static int macb_probe(struct platform_device *pdev)
 	phy_interface_t interface;
 	struct net_device *dev;
 	struct resource *regs;
+	u32 wtrmrk_rst_val;
 	void __iomem *mem;
 	struct macb *bp;
 	int err, val;
@@ -4995,6 +5003,27 @@ static int macb_probe(struct platform_device *pdev)
 
 	bp->usrio = macb_config->usrio;
 
+	/* 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))) {
+		err = of_property_read_u16(bp->pdev->dev.of_node,
+					   "cdns,rx-watermark",
+					   &bp->rx_watermark);
+
+		if (!err) {
+			/* 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 (bp->rx_watermark > wtrmrk_rst_val || !bp->rx_watermark) {
+				dev_info(&bp->pdev->dev, "Invalid watermark value\n");
+				bp->rx_watermark = 0;
+				return -EINVAL;
+			}
+			bp->rx_watermark &= wtrmrk_rst_val;
+		}
+	}
 	spin_lock_init(&bp->lock);
 
 	/* setup capabilities */
-- 
2.36.1

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

[Simon Horman]

On Tue, May 30, 2023 at 03:51:38AM -0600, Pranavi Somisetty wrote:
> From: Maulik Jodhani <maulik.jodhani@xilinx.com>
> 
> 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: Pranavi Somisetty <pranavi.somisetty@amd.com>

...

> @@ -4995,6 +5003,27 @@ static int macb_probe(struct platform_device *pdev)
>  
>  	bp->usrio = macb_config->usrio;
>  
> +	/* 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))) {
> +		err = of_property_read_u16(bp->pdev->dev.of_node,
> +					   "cdns,rx-watermark",
> +					   &bp->rx_watermark);
> +
> +		if (!err) {
> +			/* 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 (bp->rx_watermark > wtrmrk_rst_val || !bp->rx_watermark) {
> +				dev_info(&bp->pdev->dev, "Invalid watermark value\n");
> +				bp->rx_watermark = 0;
> +				return -EINVAL;

Hi Pranavi,

This appears to leak resources.
Perhaps:

				err = -EINVAL;
				goto err_out_free_netdev;

> +			}
> +			bp->rx_watermark &= wtrmrk_rst_val;
> +		}
> +	}
>  	spin_lock_init(&bp->lock);
>  
>  	/* setup capabilities */

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

[Claudiu.Beznea]

On 30.05.2023 12:51, Pranavi Somisetty wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> From: Maulik Jodhani <maulik.jodhani@xilinx.com>
> 
> 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: 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.
> 
> Changes v3:
> 1. Followed reverse christmas tree pattern in declaring variables.
> 2. Return -EINVAL when an invalid watermark value is set.
> 3. Removed netdev_info when partial store and forward is not enabled.
> 4. Validating the rx-watermark value in probe itself and only write to the register
> in init.
> 5. Writing a reset value to the pbuf_cuthru register before disabing partial store
> and forward is redundant. So removing it.
> 6. Removed the platform caps flag.
> 7. Instead of reading rx-watermark from DT in macb_configure_caps,
> reading it in probe.
> 8. Changed Signed-Off-By and author names on this patch.
> ---
>  drivers/net/ethernet/cadence/macb.h      | 14 ++++++++++++
>  drivers/net/ethernet/cadence/macb_main.c | 29 ++++++++++++++++++++++++
>  2 files changed, 43 insertions(+)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 14dfec4db8f9..416e6070e4ec 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 */

This is not used

> +#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                        25
> +#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

This is not used

>  #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..7a31e6673e15 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -2617,6 +2617,9 @@ 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 */
> +       gem_writel(bp, PBUFRXCUT, 0);
> +
>         /* Disable all interrupts */
>         for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
>                 queue_writel(queue, IDR, -1);
> @@ -2770,6 +2773,10 @@ 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->rx_watermark)
> +               gem_writel(bp, PBUFRXCUT, (bp->rx_watermark | GEM_BIT(ENCUTTHRU)));
>  }
> 
>  /* The hash address register is 64 bits long and takes up two
> @@ -4923,6 +4930,7 @@ static int macb_probe(struct platform_device *pdev)
>         phy_interface_t interface;
>         struct net_device *dev;
>         struct resource *regs;
> +       u32 wtrmrk_rst_val;
>         void __iomem *mem;
>         struct macb *bp;
>         int err, val;
> @@ -4995,6 +5003,27 @@ static int macb_probe(struct platform_device *pdev)
> 
>         bp->usrio = macb_config->usrio;
> 
> +       /* 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))) {
> +               err = of_property_read_u16(bp->pdev->dev.of_node,
> +                                          "cdns,rx-watermark",
> +                                          &bp->rx_watermark);
> +
> +               if (!err) {
> +                       /* 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 (bp->rx_watermark > wtrmrk_rst_val || !bp->rx_watermark) {
> +                               dev_info(&bp->pdev->dev, "Invalid watermark value\n");
> +                               bp->rx_watermark = 0;> +                               return -EINVAL;

Don't need to return here as you already set bp->rx_watermark = 0 and it
will not be taken into account by configuration path.

> +                       }
> +                       bp->rx_watermark &= wtrmrk_rst_val;

Is this needed? You already checked above
bp->rx_watermark > wtrmrk_rst_val

> +               }
> +       }
>         spin_lock_init(&bp->lock);
> 
>         /* setup capabilities */
> --
> 2.36.1
>