[PATCH net-next 00/13] Support the Cadence MACB/GEM instances on Mobileye EyeQ5 SoCs

[PATCH net-next 00/13] Support the Cadence MACB/GEM instances on Mobileye EyeQ5 SoCs

[Théo Lebrun]

Mobileye EyeQ5 SoCs provides two GEM IP blocks. The end result is
working networking on the EyeQ5 eval board. It isn't just a new
macb_config & compatible, here are each commit with a brief note:

 - Let's get the cleanup patches out of the way first:

   [PATCH net-next 04/13] net: macb: use BIT() macro for capability definitions
   [PATCH net-next 06/13] net: macb: simplify macb_probe() code touching match data
   [PATCH net-next 08/13] net: macb: introduce DMA descriptor helpers (is 64bit? is PTP?)
   [PATCH net-next 09/13] net: macb: sort #includes

 - LSO has been observed to be buggy, even though HW reports it is
   supported. We add a capability to force-disable it:

   [PATCH net-next 05/13] net: macb: add no LSO capability (MACB_CAPS_NO_LSO)

 - The MACB driver code has an issue: the HW inserts two dummy bytes at
   the start of Rx buffers, for IP header alignment (ie skb_reserve is
   done AFTER writing the addr in DMA descriptors). But the driver
   assumes that alignment is NET_IP_ALIGN. We appear to be facing the
   first SoC where that isn't the case.

   Happy to get comments & discuss the approach proposed.

   [PATCH net-next 07/13] net: macb: move HW IP alignment value to macb_config

 - We want cache coherent memory through a CM3 IO Coherency Unit (IOCU).
   To route through that, DMA addresses must have BIT(36) enabled.

   We do that in platform-specific code and hook our dma_map_ops through
   a notifier block.

   [PATCH net-next 11/13] MIPS: mobileye: add EyeQ5 DMA IOCU support

 - dt-bindings improvements:

   [PATCH net-next 02/13] dt-bindings: net: cdns,macb: allow tsu_clk without tx_clk
   [PATCH net-next 03/13] dt-bindings: net: cdns,macb: allow dma-coherent

 - Add the hardware to:
    - dt-bindings: new compatible, new phandle property,
    - the driver: macb_config, compatible and a custom init callback
      (that needs a regmap to the system-controller),
    - the DTS: both the SoC GEM instances and the eval board PHYs.

   [PATCH net-next 01/13] dt-bindings: net: cdns,macb: add Mobileye EyeQ5 ethernet interface
   [PATCH net-next 10/13] net: macb: Add "mobileye,eyeq5-gem" compatible
   [PATCH net-next 12/13] MIPS: mobileye: eyeq5: add two Cadence GEM Ethernet controllers
   [PATCH net-next 13/13] MIPS: mobileye: eyeq5-epm: add two Cadence GEM Ethernet PHYs

Have a nice day,
Théo

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
Théo Lebrun (13):
      dt-bindings: net: cdns,macb: add Mobileye EyeQ5 ethernet interface
      dt-bindings: net: cdns,macb: allow tsu_clk without tx_clk
      dt-bindings: net: cdns,macb: allow dma-coherent
      net: macb: use BIT() macro for capability definitions
      net: macb: add no LSO capability (MACB_CAPS_NO_LSO)
      net: macb: simplify macb_probe() code touching match data
      net: macb: move HW IP alignment value to macb_config
      net: macb: introduce DMA descriptor helpers (is 64bit? is PTP?)
      net: macb: sort #includes
      net: macb: Add "mobileye,eyeq5-gem" compatible
      MIPS: mobileye: add EyeQ5 DMA IOCU support
      MIPS: mobileye: eyeq5: add two Cadence GEM Ethernet controllers
      MIPS: mobileye: eyeq5-epm: add two Cadence GEM Ethernet PHYs

 .../devicetree/bindings/net/cdns,macb.yaml         |  25 +-
 MAINTAINERS                                        |   2 +-
 arch/mips/boot/dts/mobileye/eyeq5-epm5.dts         |  26 ++
 arch/mips/boot/dts/mobileye/eyeq5.dtsi             |  34 +++
 arch/mips/mobileye/Kconfig                         |   1 +
 arch/mips/mobileye/Makefile                        |   2 +
 arch/mips/mobileye/eyeq5-iocu-dma.c                | 160 +++++++++++
 drivers/net/ethernet/cadence/macb.h                |  51 ++--
 drivers/net/ethernet/cadence/macb_main.c           | 309 +++++++++++++--------
 9 files changed, 470 insertions(+), 140 deletions(-)
---
base-commit: ddf9c6d982ae7472a4da982e0497be2a140a194b
change-id: 20250311-macb-65a7fa86af1d

Best regards,
-- 
Théo Lebrun <theo.lebrun@bootlin.com>

[PATCH net-next 01/13] dt-bindings: net: cdns,macb: add Mobileye EyeQ5 ethernet interface

[Théo Lebrun]

Add cdns,eyeq5-gem as compatible for the integrated GEM block inside
Mobileye EyeQ5 SoCs. Add a phandle (and two offset arguments) for
accessing syscon registers.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 .../devicetree/bindings/net/cdns,macb.yaml          | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/cdns,macb.yaml b/Documentation/devicetree/bindings/net/cdns,macb.yaml
index 3c30dd23cd4efa17e14b17bfb41c54de4ebadcaa..306d14958778df1a80a15e24d8ed5409704613be 100644
--- a/Documentation/devicetree/bindings/net/cdns,macb.yaml
+++ b/Documentation/devicetree/bindings/net/cdns,macb.yaml
@@ -51,6 +51,7 @@ properties:
           - atmel,sama5d2-gem         # GEM IP (10/100) on Atmel sama5d2 SoCs
           - atmel,sama5d3-gem         # Gigabit IP on Atmel sama5d3 SoCs
           - atmel,sama5d4-gem         # GEM IP (10/100) on Atmel sama5d4 SoCs
+          - mobileye,eyeq5-gem        # Mobileye EyeQ5 SoCs
           - cdns,np4-macb             # NP4 SoC devices
           - microchip,sama7g5-emac    # Microchip SAMA7G5 ethernet interface
           - microchip,sama7g5-gem     # Microchip SAMA7G5 gigabit ethernet interface
@@ -136,6 +137,14 @@ properties:
       Node containing PHY children. If this node is not present, then PHYs will
       be direct children.
 
+  mobileye,olb:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    items:
+      - items:
+          - description: phandle to OLB node
+          - description: MAC General-Purpose register offset
+          - description: MAC SGMII register offset
+
 patternProperties:
   "^ethernet-phy@[0-9a-f]$":
     type: object
@@ -174,6 +183,18 @@ allOf:
         reg:
           maxItems: 1
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: mobileye,eyeq5-gem
+    then:
+      required:
+        - mobileye,olb
+    else:
+      properties:
+        mobileye,olb: false
+
 unevaluatedProperties: false
 
 examples:

-- 
2.48.1

Re: [PATCH net-next 01/13] dt-bindings: net: cdns,macb: add Mobileye EyeQ5 ethernet interface

[Rob Herring (Arm)]

On Fri, 21 Mar 2025 20:09:32 +0100, Théo Lebrun wrote:
> Add cdns,eyeq5-gem as compatible for the integrated GEM block inside
> Mobileye EyeQ5 SoCs. Add a phandle (and two offset arguments) for
> accessing syscon registers.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  .../devicetree/bindings/net/cdns,macb.yaml          | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/cdns,macb.yaml: properties:mobileye,olb: 'anyOf' conditional failed, one must be fixed:
	'description' is a dependency of '$ref'
	'/schemas/types.yaml#/definitions/phandle-array' does not match '^#/(definitions|\\$defs)/'
		hint: A vendor property can have a $ref to a a $defs schema
	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250321-macb-v1-1-537b7e37971d@bootlin.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH net-next 01/13] dt-bindings: net: cdns,macb: add Mobileye EyeQ5 ethernet interface

[Andrew Lunn]

>            - atmel,sama5d2-gem         # GEM IP (10/100) on Atmel sama5d2 SoCs
>            - atmel,sama5d3-gem         # Gigabit IP on Atmel sama5d3 SoCs
>            - atmel,sama5d4-gem         # GEM IP (10/100) on Atmel sama5d4 SoCs
> +          - mobileye,eyeq5-gem        # Mobileye EyeQ5 SoCs
>            - cdns,np4-macb             # NP4 SoC devices
>            - microchip,sama7g5-emac    # Microchip SAMA7G5 ethernet interface
>            - microchip,sama7g5-gem     # Microchip SAMA7G5 gigabit ethernet interface

These are kind of sorted. Maybe put mobileye after microchip?

	Andrew

Re: [PATCH net-next 01/13] dt-bindings: net: cdns,macb: add Mobileye EyeQ5 ethernet interface

[Théo Lebrun]

On Fri Mar 21, 2025 at 9:49 PM CET, Andrew Lunn wrote:
>>            - atmel,sama5d2-gem         # GEM IP (10/100) on Atmel sama5d2 SoCs
>>            - atmel,sama5d3-gem         # Gigabit IP on Atmel sama5d3 SoCs
>>            - atmel,sama5d4-gem         # GEM IP (10/100) on Atmel sama5d4 SoCs
>> +          - mobileye,eyeq5-gem        # Mobileye EyeQ5 SoCs
>>            - cdns,np4-macb             # NP4 SoC devices
>>            - microchip,sama7g5-emac    # Microchip SAMA7G5 ethernet interface
>>            - microchip,sama7g5-gem     # Microchip SAMA7G5 gigabit ethernet interface
>
> These are kind of sorted. Maybe put mobileye after microchip?

I never understood how most lists end up mostly sorted. Changes for V2:
 - add a "dt-bindings: net: cdns,macb: sort compatibles" patch,
 - add the mobileye compatible below microchip's.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH net-next 02/13] dt-bindings: net: cdns,macb: allow tsu_clk without tx_clk

[Théo Lebrun]

Allow providing tsu_clk without a tx_clk as both are optional.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 Documentation/devicetree/bindings/net/cdns,macb.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/cdns,macb.yaml b/Documentation/devicetree/bindings/net/cdns,macb.yaml
index 306d14958778df1a80a15e24d8ed5409704613be..36fcae1b20d757b3ebe615a9fc66068000ded56d 100644
--- a/Documentation/devicetree/bindings/net/cdns,macb.yaml
+++ b/Documentation/devicetree/bindings/net/cdns,macb.yaml
@@ -85,7 +85,7 @@ properties:
     items:
       - enum: [ ether_clk, hclk, pclk ]
       - enum: [ hclk, pclk ]
-      - const: tx_clk
+      - enum: [ tx_clk, tsu_clk ]
       - enum: [ rx_clk, tsu_clk ]
       - const: tsu_clk
 

-- 
2.48.1

Re: [PATCH net-next 02/13] dt-bindings: net: cdns,macb: allow tsu_clk without tx_clk

[Rob Herring]

On Fri, Mar 21, 2025 at 08:09:33PM +0100, Théo Lebrun wrote:
> Allow providing tsu_clk without a tx_clk as both are optional.

Why? Is there some new h/w? Where's the compatible for it. Or this fixes 
some existing user? Which one?

> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  Documentation/devicetree/bindings/net/cdns,macb.yaml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/cdns,macb.yaml b/Documentation/devicetree/bindings/net/cdns,macb.yaml
> index 306d14958778df1a80a15e24d8ed5409704613be..36fcae1b20d757b3ebe615a9fc66068000ded56d 100644
> --- a/Documentation/devicetree/bindings/net/cdns,macb.yaml
> +++ b/Documentation/devicetree/bindings/net/cdns,macb.yaml
> @@ -85,7 +85,7 @@ properties:
>      items:
>        - enum: [ ether_clk, hclk, pclk ]
>        - enum: [ hclk, pclk ]
> -      - const: tx_clk
> +      - enum: [ tx_clk, tsu_clk ]
>        - enum: [ rx_clk, tsu_clk ]
>        - const: tsu_clk
>  
> 
> -- 
> 2.48.1
> 

Re: [PATCH net-next 02/13] dt-bindings: net: cdns,macb: allow tsu_clk without tx_clk

[Théo Lebrun]

On Mon Mar 24, 2025 at 5:30 PM CET, Rob Herring wrote:
> On Fri, Mar 21, 2025 at 08:09:33PM +0100, Théo Lebrun wrote:
>> Allow providing tsu_clk without a tx_clk as both are optional.
>
> Why? Is there some new h/w? Where's the compatible for it. Or this fixes 
> some existing user? Which one?

I encountered this while supporting a new compatible yes:
mobileye,eyeq5-gem.

But this is more about relaxing unneeded requirements: the previous
bindings said "if you provide a tsu_clk, please provide a tx_clk". That
constraint can be removed as there is no relation inbetween tx_clk and
tsu_clk.

So I'd describe that as a semantic fix of the dt-bindings, that happens
to be useful for our newly introduced compatible.

Or am I mistaken? In any case, I should expand the commit message.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH net-next 03/13] dt-bindings: net: cdns,macb: allow dma-coherent

[Théo Lebrun]

On EyeQ5, the GEM DMA controller is coherent with the CPU;
allow specifying the information.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 Documentation/devicetree/bindings/net/cdns,macb.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/cdns,macb.yaml b/Documentation/devicetree/bindings/net/cdns,macb.yaml
index 36fcae1b20d757b3ebe615a9fc66068000ded56d..8a3f0807ffce89ab17bf561722833e355a4a0238 100644
--- a/Documentation/devicetree/bindings/net/cdns,macb.yaml
+++ b/Documentation/devicetree/bindings/net/cdns,macb.yaml
@@ -112,6 +112,8 @@ properties:
   iommus:
     maxItems: 1
 
+  dma-coherent: true
+
   power-domains:
     maxItems: 1
 

-- 
2.48.1

Re: [PATCH net-next 03/13] dt-bindings: net: cdns,macb: allow dma-coherent

[Rob Herring (Arm)]

On Fri, 21 Mar 2025 20:09:34 +0100, Théo Lebrun wrote:
> On EyeQ5, the GEM DMA controller is coherent with the CPU;
> allow specifying the information.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  Documentation/devicetree/bindings/net/cdns,macb.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 

Acked-by: Rob Herring (Arm) <robh@kernel.org>

[PATCH net-next 04/13] net: macb: use BIT() macro for capability definitions

[Théo Lebrun]

Replace all capabilities values by calls to the BIT() macro.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/net/ethernet/cadence/macb.h | 40 ++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index c9a5c8beb2fa8166195d1d83f187d2d0c62668a8..3b43cb9468e3618754ff2bc6c5f360447bdeeed0 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -727,26 +727,26 @@
 #define MACB_MAN_C45_CODE			2
 
 /* Capability mask bits */
-#define MACB_CAPS_ISR_CLEAR_ON_WRITE		0x00000001
-#define MACB_CAPS_USRIO_HAS_CLKEN		0x00000002
-#define MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII	0x00000004
-#define MACB_CAPS_NO_GIGABIT_HALF		0x00000008
-#define MACB_CAPS_USRIO_DISABLED		0x00000010
-#define MACB_CAPS_JUMBO				0x00000020
-#define MACB_CAPS_GEM_HAS_PTP			0x00000040
-#define MACB_CAPS_BD_RD_PREFETCH		0x00000080
-#define MACB_CAPS_NEEDS_RSTONUBR		0x00000100
-#define MACB_CAPS_MIIONRGMII			0x00000200
-#define MACB_CAPS_NEED_TSUCLK			0x00000400
-#define MACB_CAPS_QUEUE_DISABLE			0x00000800
-#define MACB_CAPS_PCS				0x01000000
-#define MACB_CAPS_HIGH_SPEED			0x02000000
-#define MACB_CAPS_CLK_HW_CHG			0x04000000
-#define MACB_CAPS_MACB_IS_EMAC			0x08000000
-#define MACB_CAPS_FIFO_MODE			0x10000000
-#define MACB_CAPS_GIGABIT_MODE_AVAILABLE	0x20000000
-#define MACB_CAPS_SG_DISABLED			0x40000000
-#define MACB_CAPS_MACB_IS_GEM			0x80000000
+#define MACB_CAPS_ISR_CLEAR_ON_WRITE		BIT(0)
+#define MACB_CAPS_USRIO_HAS_CLKEN		BIT(1)
+#define MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII	BIT(2)
+#define MACB_CAPS_NO_GIGABIT_HALF		BIT(3)
+#define MACB_CAPS_USRIO_DISABLED		BIT(4)
+#define MACB_CAPS_JUMBO				BIT(5)
+#define MACB_CAPS_GEM_HAS_PTP			BIT(6)
+#define MACB_CAPS_BD_RD_PREFETCH		BIT(7)
+#define MACB_CAPS_NEEDS_RSTONUBR		BIT(8)
+#define MACB_CAPS_MIIONRGMII			BIT(9)
+#define MACB_CAPS_NEED_TSUCLK			BIT(10)
+#define MACB_CAPS_QUEUE_DISABLE			BIT(11)
+#define MACB_CAPS_PCS				BIT(24)
+#define MACB_CAPS_HIGH_SPEED			BIT(25)
+#define MACB_CAPS_CLK_HW_CHG			BIT(26)
+#define MACB_CAPS_MACB_IS_EMAC			BIT(27)
+#define MACB_CAPS_FIFO_MODE			BIT(28)
+#define MACB_CAPS_GIGABIT_MODE_AVAILABLE	BIT(29)
+#define MACB_CAPS_SG_DISABLED			BIT(30)
+#define MACB_CAPS_MACB_IS_GEM			BIT(31)
 
 /* LSO settings */
 #define MACB_LSO_UFO_ENABLE			0x01

-- 
2.48.1

Re: [PATCH net-next 04/13] net: macb: use BIT() macro for capability definitions

[Andrew Lunn]

On Fri, Mar 21, 2025 at 08:09:35PM +0100, Théo Lebrun wrote:
> Replace all capabilities values by calls to the BIT() macro.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net-next 05/13] net: macb: add no LSO capability (MACB_CAPS_NO_LSO)

[Théo Lebrun]

LSO is runtime-detected using the PBUF_LSO field inside register
designcfg_debug6/GEM_DCFG6. Allow disabling that feature if it is
broken by using struct macb_config->caps.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/net/ethernet/cadence/macb.h      | 1 +
 drivers/net/ethernet/cadence/macb_main.c | 5 +++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 3b43cb9468e3618754ff2bc6c5f360447bdeeed0..e9da6e3b869fc772613a0d6b86308917c9bff7fe 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -739,6 +739,7 @@
 #define MACB_CAPS_MIIONRGMII			BIT(9)
 #define MACB_CAPS_NEED_TSUCLK			BIT(10)
 #define MACB_CAPS_QUEUE_DISABLE			BIT(11)
+#define MACB_CAPS_NO_LSO			BIT(12)
 #define MACB_CAPS_PCS				BIT(24)
 #define MACB_CAPS_HIGH_SPEED			BIT(25)
 #define MACB_CAPS_CLK_HW_CHG			BIT(26)
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index b5797c1ac0a41e9472883b013c1e44a01092f257..807f7abbd9941bf624f14a5ddead68dad1c8deb2 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -4373,8 +4373,9 @@ static int macb_init(struct platform_device *pdev)
 	/* Set features */
 	dev->hw_features = NETIF_F_SG;
 
-	/* Check LSO capability */
-	if (GEM_BFEXT(PBUF_LSO, gem_readl(bp, DCFG6)))
+	/* Check LSO capability; capability is for buggy HW */
+	if (!(bp->caps & MACB_CAPS_NO_LSO) &&
+	    GEM_BFEXT(PBUF_LSO, gem_readl(bp, DCFG6)))
 		dev->hw_features |= MACB_NETIF_LSO;
 
 	/* Checksum offload is only available on gem with packet buffer */

-- 
2.48.1

Re: [PATCH net-next 05/13] net: macb: add no LSO capability (MACB_CAPS_NO_LSO)

[Andrew Lunn]

On Fri, Mar 21, 2025 at 08:09:36PM +0100, Théo Lebrun wrote:
> LSO is runtime-detected using the PBUF_LSO field inside register
> designcfg_debug6/GEM_DCFG6. Allow disabling that feature if it is
> broken by using struct macb_config->caps.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 05/13] net: macb: add no LSO capability (MACB_CAPS_NO_LSO)

[Claudiu Beznea]

Hi, Theo,

On 21.03.2025 21:09, Théo Lebrun wrote:
> LSO is runtime-detected using the PBUF_LSO field inside register
> designcfg_debug6/GEM_DCFG6. Allow disabling that feature if it is
> broken by using struct macb_config->caps.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  drivers/net/ethernet/cadence/macb.h      | 1 +
>  drivers/net/ethernet/cadence/macb_main.c | 5 +++--
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 3b43cb9468e3618754ff2bc6c5f360447bdeeed0..e9da6e3b869fc772613a0d6b86308917c9bff7fe 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -739,6 +739,7 @@
>  #define MACB_CAPS_MIIONRGMII			BIT(9)
>  #define MACB_CAPS_NEED_TSUCLK			BIT(10)
>  #define MACB_CAPS_QUEUE_DISABLE			BIT(11)
> +#define MACB_CAPS_NO_LSO			BIT(12)
>  #define MACB_CAPS_PCS				BIT(24)
>  #define MACB_CAPS_HIGH_SPEED			BIT(25)
>  #define MACB_CAPS_CLK_HW_CHG			BIT(26)
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index b5797c1ac0a41e9472883b013c1e44a01092f257..807f7abbd9941bf624f14a5ddead68dad1c8deb2 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -4373,8 +4373,9 @@ static int macb_init(struct platform_device *pdev)
>  	/* Set features */
>  	dev->hw_features = NETIF_F_SG;
>  
> -	/* Check LSO capability */
> -	if (GEM_BFEXT(PBUF_LSO, gem_readl(bp, DCFG6)))
> +	/* Check LSO capability; capability is for buggy HW */

The comment here is a bit confusing to me.

> +	if (!(bp->caps & MACB_CAPS_NO_LSO) &&
> +	    GEM_BFEXT(PBUF_LSO, gem_readl(bp, DCFG6)))
>  		dev->hw_features |= MACB_NETIF_LSO;
>  
>  	/* Checksum offload is only available on gem with packet buffer */
> 

Re: [PATCH net-next 05/13] net: macb: add no LSO capability (MACB_CAPS_NO_LSO)

[Théo Lebrun]

Hello Claudiu,

On Mon Mar 24, 2025 at 9:18 AM CET, Claudiu Beznea wrote:
> On 21.03.2025 21:09, Théo Lebrun wrote:
>> LSO is runtime-detected using the PBUF_LSO field inside register
>> designcfg_debug6/GEM_DCFG6. Allow disabling that feature if it is
>> broken by using struct macb_config->caps.
>> 
>> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
>> ---
>>  drivers/net/ethernet/cadence/macb.h      | 1 +
>>  drivers/net/ethernet/cadence/macb_main.c | 5 +++--
>>  2 files changed, 4 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
>> index 3b43cb9468e3618754ff2bc6c5f360447bdeeed0..e9da6e3b869fc772613a0d6b86308917c9bff7fe 100644
>> --- a/drivers/net/ethernet/cadence/macb.h
>> +++ b/drivers/net/ethernet/cadence/macb.h
>> @@ -739,6 +739,7 @@
>>  #define MACB_CAPS_MIIONRGMII			BIT(9)
>>  #define MACB_CAPS_NEED_TSUCLK			BIT(10)
>>  #define MACB_CAPS_QUEUE_DISABLE			BIT(11)
>> +#define MACB_CAPS_NO_LSO			BIT(12)
>>  #define MACB_CAPS_PCS				BIT(24)
>>  #define MACB_CAPS_HIGH_SPEED			BIT(25)
>>  #define MACB_CAPS_CLK_HW_CHG			BIT(26)
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
>> index b5797c1ac0a41e9472883b013c1e44a01092f257..807f7abbd9941bf624f14a5ddead68dad1c8deb2 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -4373,8 +4373,9 @@ static int macb_init(struct platform_device *pdev)
>>  	/* Set features */
>>  	dev->hw_features = NETIF_F_SG;
>>  
>> -	/* Check LSO capability */
>> -	if (GEM_BFEXT(PBUF_LSO, gem_readl(bp, DCFG6)))
>> +	/* Check LSO capability; capability is for buggy HW */
>
> The comment here is a bit confusing to me.

Proposal:

+  /* Check LSO capability; runtime detection can be overridden by a cap
+   * flag if the hardware is known to be buggy */

I'll use that in V2, or feel free to reply if it's still unclear.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH net-next 06/13] net: macb: simplify macb_probe() code touching match data

[Théo Lebrun]

Remove local variables clk_init and init. Those function pointers are
always equivalent to macb_config->clk_init and macb_config->init.

Also remove NULL checks on macb_config as it is always valid:
 - either it is its default value &default_gem_config,
 - or it got overridden using match data.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 807f7abbd9941bf624f14a5ddead68dad1c8deb2..b206966178e3d49a084c754191f77205ff6dd466 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -5177,10 +5177,6 @@ static const struct macb_config default_gem_config = {
 static int macb_probe(struct platform_device *pdev)
 {
 	const struct macb_config *macb_config = &default_gem_config;
-	int (*clk_init)(struct platform_device *, struct clk **,
-			struct clk **, struct clk **,  struct clk **,
-			struct clk **) = macb_config->clk_init;
-	int (*init)(struct platform_device *) = macb_config->init;
 	struct device_node *np = pdev->dev.of_node;
 	struct clk *pclk, *hclk = NULL, *tx_clk = NULL, *rx_clk = NULL;
 	struct clk *tsu_clk = NULL;
@@ -5202,14 +5198,11 @@ static int macb_probe(struct platform_device *pdev)
 		const struct of_device_id *match;
 
 		match = of_match_node(macb_dt_ids, np);
-		if (match && match->data) {
+		if (match && match->data)
 			macb_config = match->data;
-			clk_init = macb_config->clk_init;
-			init = macb_config->init;
-		}
 	}
 
-	err = clk_init(pdev, &pclk, &hclk, &tx_clk, &rx_clk, &tsu_clk);
+	err = macb_config->clk_init(pdev, &pclk, &hclk, &tx_clk, &rx_clk, &tsu_clk);
 	if (err)
 		return err;
 
@@ -5245,15 +5238,13 @@ static int macb_probe(struct platform_device *pdev)
 	}
 	bp->num_queues = num_queues;
 	bp->queue_mask = queue_mask;
-	if (macb_config)
-		bp->dma_burst_length = macb_config->dma_burst_length;
+	bp->dma_burst_length = macb_config->dma_burst_length;
 	bp->pclk = pclk;
 	bp->hclk = hclk;
 	bp->tx_clk = tx_clk;
 	bp->rx_clk = rx_clk;
 	bp->tsu_clk = tsu_clk;
-	if (macb_config)
-		bp->jumbo_max_len = macb_config->jumbo_max_len;
+	bp->jumbo_max_len = macb_config->jumbo_max_len;
 
 	if (!hw_is_gem(bp->regs, bp->native_io))
 		bp->max_tx_length = MACB_MAX_TX_LEN;
@@ -5343,7 +5334,7 @@ static int macb_probe(struct platform_device *pdev)
 		bp->phy_interface = interface;
 
 	/* IP specific init */
-	err = init(pdev);
+	err = macb_config->init(pdev);
 	if (err)
 		goto err_out_free_netdev;
 

-- 
2.48.1

Re: [PATCH net-next 06/13] net: macb: simplify macb_probe() code touching match data

[Andrew Lunn]

On Fri, Mar 21, 2025 at 08:09:37PM +0100, Théo Lebrun wrote:
> Remove local variables clk_init and init. Those function pointers are
> always equivalent to macb_config->clk_init and macb_config->init.
> 
> Also remove NULL checks on macb_config as it is always valid:
>  - either it is its default value &default_gem_config,
>  - or it got overridden using match data.

I probably would of made this two patches. But otherwise it looks O.K.

  Andrew

[PATCH net-next 07/13] net: macb: move HW IP alignment value to macb_config

[Théo Lebrun]

The controller does IP alignment (two bytes). However, we match that on
the software side with:

	skb_reserve(skb, NET_IP_ALIGN);

The NET_IP_ALIGN value is arch-dependent and picked based on unaligned
CPU access performance. The hardware alignment value should be
compatible-specific rather than arch-specific. Offer a path forward by
adding a hw_ip_align field inside macb_config.

Values for macb_config->hw_ip_align are picked based on upstream
devicetrees:

    Compatible             |  DTS folders              |  hw_ip_align
   ------------------------|---------------------------|----------------
   cdns,at91sam9260-macb   | arch/arm/                 | 2
   cdns,macb               | arch/{arm,riscv}/         | NET_IP_ALIGN
   cdns,np4-macb           | NULL                      | NET_IP_ALIGN
   cdns,pc302-gem          | NULL                      | NET_IP_ALIGN
   cdns,gem                | arch/{arm,arm64}/         | NET_IP_ALIGN
   cdns,sam9x60-macb       | arch/arm/                 | 2
   atmel,sama5d2-gem       | arch/arm/                 | 2
   atmel,sama5d29-gem      | arch/arm/                 | 2
   atmel,sama5d3-gem       | arch/arm/                 | 2
   atmel,sama5d3-macb      | arch/arm/                 | 2
   atmel,sama5d4-gem       | arch/arm/                 | 2
   cdns,at91rm9200-emac    | arch/arm/                 | 2
   cdns,emac               | arch/arm/                 | 2
   cdns,zynqmp-gem         | *same as xlnx,zynqmp-gem* | 0
   cdns,zynq-gem           | *same as xlnx,zynq-gem*   | 2
   sifive,fu540-c000-gem   | arch/riscv/               | 2
   microchip,mpfs-macb     | arch/riscv/               | 2
   microchip,sama7g5-gem   | arch/arm/                 | 2
   microchip,sama7g5-emac  | arch/arm/                 | 2
   xlnx,zynqmp-gem         | arch/arm64/               | 0
   xlnx,zynq-gem           | arch/arm/                 | 2
   xlnx,versal-gem         | NULL                      | NET_IP_ALIGN

Considerations:
 - cdns,macb has no match data config.
 - cdns,np4-macb, cdns,pc302-gem and xlnx,versal-gem have no upstream
   devicetrees using them.
 - cdns,gem is used on both arm and arm64 platforms. Those do not have
   the same NET_IP_ALIGN value. Also, it uses the same config as
   cdns,pc302-gem.
 - The default config (default_gem_config) behavior isn't changed:
   it uses hw_ip_align == NET_IP_ALIGN.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/net/ethernet/cadence/macb.h      |  2 ++
 drivers/net/ethernet/cadence/macb_main.c | 40 ++++++++++++++++++++++++++++++--
 2 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index e9da6e3b869fc772613a0d6b86308917c9bff7fe..5bf7e7ff70490cdb068bfdbe7cfd5bb8e1db7f86 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -1191,6 +1191,7 @@ struct macb_usrio_config {
 
 struct macb_config {
 	u32			caps;
+	int			hw_ip_align;
 	unsigned int		dma_burst_length;
 	int	(*clk_init)(struct platform_device *pdev, struct clk **pclk,
 			    struct clk **hclk, struct clk **tx_clk,
@@ -1295,6 +1296,7 @@ struct macb {
 
 	u32			caps;
 	unsigned int		dma_burst_length;
+	int			hw_ip_align;
 
 	phy_interface_t		phy_interface;
 
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index b206966178e3d49a084c754191f77205ff6dd466..b32363ba1ec3be0fc42866c8585f0b465d178220 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -1352,8 +1352,26 @@ static void gem_rx_refill(struct macb_queue *queue)
 			dma_wmb();
 			macb_set_addr(bp, desc, paddr);
 
-			/* properly align Ethernet header */
-			skb_reserve(skb, NET_IP_ALIGN);
+			/* Properly align Ethernet header.
+			 *
+			 * Here be (small-ish) dragons. 3 features intertwine:
+			 * (1) Hardware adds two dummy bytes. Notice how
+			 *     skb_reserve() is done AFTER dma_map_single().
+			 * (2) The NET_IP_ALIGN value is arch dependent.
+			 * (3) The low 2/3 bits cannot be picked.
+			 *     3 bits if HW_DMA_CAP_PTP.
+			 *
+			 * Notice how (1) and (2) are unrelated (IP-specific
+			 * versus arch-specific) but must agree for a working
+			 * system.
+			 *
+			 * (3) implies we cannot align the IP header (ie respect
+			 * NET_IP_ALIGN) if HW does not add two bytes.
+			 *
+			 * FIXME: migrate away from hw_ip_align == NET_IP_ALIGN
+			 * for all compatibles.
+			 */
+			skb_reserve(skb, bp->hw_ip_align);
 		} else {
 			desc->ctrl = 0;
 			dma_wmb();
@@ -4994,6 +5012,7 @@ static const struct macb_usrio_config sama7g5_usrio = {
 static const struct macb_config fu540_c000_config = {
 	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO |
 		MACB_CAPS_GEM_HAS_PTP,
+	.hw_ip_align = 2,
 	.dma_burst_length = 16,
 	.clk_init = fu540_c000_clk_init,
 	.init = fu540_c000_init,
@@ -5003,6 +5022,7 @@ static const struct macb_config fu540_c000_config = {
 
 static const struct macb_config at91sam9260_config = {
 	.caps = MACB_CAPS_USRIO_HAS_CLKEN | MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII,
+	.hw_ip_align = 2,
 	.clk_init = macb_clk_init,
 	.init = macb_init,
 	.usrio = &macb_default_usrio,
@@ -5011,6 +5031,7 @@ static const struct macb_config at91sam9260_config = {
 static const struct macb_config sama5d3macb_config = {
 	.caps = MACB_CAPS_SG_DISABLED |
 		MACB_CAPS_USRIO_HAS_CLKEN | MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII,
+	.hw_ip_align = 2,
 	.clk_init = macb_clk_init,
 	.init = macb_init,
 	.usrio = &macb_default_usrio,
@@ -5018,6 +5039,7 @@ static const struct macb_config sama5d3macb_config = {
 
 static const struct macb_config pc302gem_config = {
 	.caps = MACB_CAPS_SG_DISABLED | MACB_CAPS_GIGABIT_MODE_AVAILABLE,
+	.hw_ip_align = NET_IP_ALIGN,
 	.dma_burst_length = 16,
 	.clk_init = macb_clk_init,
 	.init = macb_init,
@@ -5026,6 +5048,7 @@ static const struct macb_config pc302gem_config = {
 
 static const struct macb_config sama5d2_config = {
 	.caps = MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII | MACB_CAPS_JUMBO,
+	.hw_ip_align = 2,
 	.dma_burst_length = 16,
 	.clk_init = macb_clk_init,
 	.init = macb_init,
@@ -5035,6 +5058,7 @@ static const struct macb_config sama5d2_config = {
 
 static const struct macb_config sama5d29_config = {
 	.caps = MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII | MACB_CAPS_GEM_HAS_PTP,
+	.hw_ip_align = 2,
 	.dma_burst_length = 16,
 	.clk_init = macb_clk_init,
 	.init = macb_init,
@@ -5044,6 +5068,7 @@ static const struct macb_config sama5d29_config = {
 static const struct macb_config sama5d3_config = {
 	.caps = MACB_CAPS_SG_DISABLED | MACB_CAPS_GIGABIT_MODE_AVAILABLE |
 		MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII | MACB_CAPS_JUMBO,
+	.hw_ip_align = 2,
 	.dma_burst_length = 16,
 	.clk_init = macb_clk_init,
 	.init = macb_init,
@@ -5053,6 +5078,7 @@ static const struct macb_config sama5d3_config = {
 
 static const struct macb_config sama5d4_config = {
 	.caps = MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII,
+	.hw_ip_align = 2,
 	.dma_burst_length = 4,
 	.clk_init = macb_clk_init,
 	.init = macb_init,
@@ -5061,6 +5087,7 @@ static const struct macb_config sama5d4_config = {
 
 static const struct macb_config emac_config = {
 	.caps = MACB_CAPS_NEEDS_RSTONUBR | MACB_CAPS_MACB_IS_EMAC,
+	.hw_ip_align = 2,
 	.clk_init = at91ether_clk_init,
 	.init = at91ether_init,
 	.usrio = &macb_default_usrio,
@@ -5068,6 +5095,7 @@ static const struct macb_config emac_config = {
 
 static const struct macb_config np4_config = {
 	.caps = MACB_CAPS_USRIO_DISABLED,
+	.hw_ip_align = NET_IP_ALIGN,
 	.clk_init = macb_clk_init,
 	.init = macb_init,
 	.usrio = &macb_default_usrio,
@@ -5077,6 +5105,7 @@ 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,
+	.hw_ip_align = 0,
 	.dma_burst_length = 16,
 	.clk_init = macb_clk_init,
 	.init = init_reset_optional,
@@ -5087,6 +5116,7 @@ static const struct macb_config zynqmp_config = {
 static const struct macb_config zynq_config = {
 	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_NO_GIGABIT_HALF |
 		MACB_CAPS_NEEDS_RSTONUBR,
+	.hw_ip_align = 2,
 	.dma_burst_length = 16,
 	.clk_init = macb_clk_init,
 	.init = macb_init,
@@ -5097,6 +5127,7 @@ static const struct macb_config mpfs_config = {
 	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE |
 		MACB_CAPS_JUMBO |
 		MACB_CAPS_GEM_HAS_PTP,
+	.hw_ip_align = 2,
 	.dma_burst_length = 16,
 	.clk_init = macb_clk_init,
 	.init = init_reset_optional,
@@ -5108,6 +5139,7 @@ static const struct macb_config mpfs_config = {
 static const struct macb_config sama7g5_gem_config = {
 	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_CLK_HW_CHG |
 		MACB_CAPS_MIIONRGMII | MACB_CAPS_GEM_HAS_PTP,
+	.hw_ip_align = 2,
 	.dma_burst_length = 16,
 	.clk_init = macb_clk_init,
 	.init = macb_init,
@@ -5118,6 +5150,7 @@ static const struct macb_config sama7g5_emac_config = {
 	.caps = MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII |
 		MACB_CAPS_USRIO_HAS_CLKEN | MACB_CAPS_MIIONRGMII |
 		MACB_CAPS_GEM_HAS_PTP,
+	.hw_ip_align = 2,
 	.dma_burst_length = 16,
 	.clk_init = macb_clk_init,
 	.init = macb_init,
@@ -5128,6 +5161,7 @@ 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_QUEUE_DISABLE,
+	.hw_ip_align = NET_IP_ALIGN,
 	.dma_burst_length = 16,
 	.clk_init = macb_clk_init,
 	.init = init_reset_optional,
@@ -5167,6 +5201,7 @@ static const struct macb_config default_gem_config = {
 	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE |
 		MACB_CAPS_JUMBO |
 		MACB_CAPS_GEM_HAS_PTP,
+	.hw_ip_align = NET_IP_ALIGN,
 	.dma_burst_length = 16,
 	.clk_init = macb_clk_init,
 	.init = macb_init,
@@ -5244,6 +5279,7 @@ static int macb_probe(struct platform_device *pdev)
 	bp->tx_clk = tx_clk;
 	bp->rx_clk = rx_clk;
 	bp->tsu_clk = tsu_clk;
+	bp->hw_ip_align = macb_config->hw_ip_align;
 	bp->jumbo_max_len = macb_config->jumbo_max_len;
 
 	if (!hw_is_gem(bp->regs, bp->native_io))

-- 
2.48.1

Re: [PATCH net-next 07/13] net: macb: move HW IP alignment value to macb_config

[Andrew Lunn]

On Fri, Mar 21, 2025 at 08:09:38PM +0100, Théo Lebrun wrote:
> The controller does IP alignment (two bytes).

I'm a bit confused here. Is this hard coded, baked into the silicon?
It will always do IP alignment? It cannot be turned off?

> 	skb_reserve(skb, NET_IP_ALIGN);

Why not just replace this with

        skb_reserve(skb, 2);

> The NET_IP_ALIGN value is arch-dependent and picked based on unaligned
> CPU access performance. The hardware alignment value should be
> compatible-specific rather than arch-specific. Offer a path forward by
> adding a hw_ip_align field inside macb_config.
> 
> Values for macb_config->hw_ip_align are picked based on upstream
> devicetrees:
> 
>     Compatible             |  DTS folders              |  hw_ip_align
>    ------------------------|---------------------------|----------------
>    cdns,at91sam9260-macb   | arch/arm/                 | 2
>    cdns,macb               | arch/{arm,riscv}/         | NET_IP_ALIGN
>    cdns,np4-macb           | NULL                      | NET_IP_ALIGN
>    cdns,pc302-gem          | NULL                      | NET_IP_ALIGN
>    cdns,gem                | arch/{arm,arm64}/         | NET_IP_ALIGN
>    cdns,sam9x60-macb       | arch/arm/                 | 2
>    atmel,sama5d2-gem       | arch/arm/                 | 2
>    atmel,sama5d29-gem      | arch/arm/                 | 2
>    atmel,sama5d3-gem       | arch/arm/                 | 2
>    atmel,sama5d3-macb      | arch/arm/                 | 2
>    atmel,sama5d4-gem       | arch/arm/                 | 2
>    cdns,at91rm9200-emac    | arch/arm/                 | 2
>    cdns,emac               | arch/arm/                 | 2
>    cdns,zynqmp-gem         | *same as xlnx,zynqmp-gem* | 0
>    cdns,zynq-gem           | *same as xlnx,zynq-gem*   | 2
>    sifive,fu540-c000-gem   | arch/riscv/               | 2
>    microchip,mpfs-macb     | arch/riscv/               | 2
>    microchip,sama7g5-gem   | arch/arm/                 | 2
>    microchip,sama7g5-emac  | arch/arm/                 | 2
>    xlnx,zynqmp-gem         | arch/arm64/               | 0
>    xlnx,zynq-gem           | arch/arm/                 | 2
>    xlnx,versal-gem         | NULL                      | NET_IP_ALIGN

I don't remember seeing any other driver doing anything like
this. That often means it is wrong....

      Andrew

Re: [PATCH net-next 07/13] net: macb: move HW IP alignment value to macb_config

[Théo Lebrun]

Hello Andrew,

On Fri Mar 21, 2025 at 10:06 PM CET, Andrew Lunn wrote:
> On Fri, Mar 21, 2025 at 08:09:38PM +0100, Théo Lebrun wrote:
>> The controller does IP alignment (two bytes).
>
> I'm a bit confused here. Is this hard coded, baked into the silicon?
> It will always do IP alignment? It cannot be turned off?

Yes, the alignment is baked inside the silicon.
I looked but haven't seen any register to configure the alignment.

Sorry the commit message isn't clear, it needs improvements.

>> 	skb_reserve(skb, NET_IP_ALIGN);
>
> Why not just replace this with
>
>         skb_reserve(skb, 2);

On arm64, NET_IP_ALIGN=0. I don't have HW to test, but the current code
is telling us that the silicon doesn't do alignment on those:

   skb = netdev_alloc_skb(...);
   paddr = dma_map_single(..., skb->data, ...);
   macb_set_addr(..., paddr);

   // arm   => NET_IP_ALIGN=2 => silicon does alignment
   // arm64 => NET_IP_ALIGN=0 => silicon doesn't do alignment
   skb_reserve(skb, NET_IP_ALIGN);

The platform we introduce is the first one where the silicon alignment
(0 bytes) is different from the NET_IP_ALIGN value (MIPS, 2 bytes).

Does that clarify things?

>> The NET_IP_ALIGN value is arch-dependent and picked based on unaligned
>> CPU access performance. The hardware alignment value should be
>> compatible-specific rather than arch-specific. Offer a path forward by
>> adding a hw_ip_align field inside macb_config.
>> 
>> Values for macb_config->hw_ip_align are picked based on upstream
>> devicetrees:
>> 
>>     Compatible             |  DTS folders              |  hw_ip_align
>>    ------------------------|---------------------------|----------------
>>    cdns,at91sam9260-macb   | arch/arm/                 | 2
>>    cdns,macb               | arch/{arm,riscv}/         | NET_IP_ALIGN
>>    cdns,np4-macb           | NULL                      | NET_IP_ALIGN
>>    cdns,pc302-gem          | NULL                      | NET_IP_ALIGN
>>    cdns,gem                | arch/{arm,arm64}/         | NET_IP_ALIGN
>>    cdns,sam9x60-macb       | arch/arm/                 | 2
>>    atmel,sama5d2-gem       | arch/arm/                 | 2
>>    atmel,sama5d29-gem      | arch/arm/                 | 2
>>    atmel,sama5d3-gem       | arch/arm/                 | 2
>>    atmel,sama5d3-macb      | arch/arm/                 | 2
>>    atmel,sama5d4-gem       | arch/arm/                 | 2
>>    cdns,at91rm9200-emac    | arch/arm/                 | 2
>>    cdns,emac               | arch/arm/                 | 2
>>    cdns,zynqmp-gem         | *same as xlnx,zynqmp-gem* | 0
>>    cdns,zynq-gem           | *same as xlnx,zynq-gem*   | 2
>>    sifive,fu540-c000-gem   | arch/riscv/               | 2
>>    microchip,mpfs-macb     | arch/riscv/               | 2
>>    microchip,sama7g5-gem   | arch/arm/                 | 2
>>    microchip,sama7g5-emac  | arch/arm/                 | 2
>>    xlnx,zynqmp-gem         | arch/arm64/               | 0
>>    xlnx,zynq-gem           | arch/arm/                 | 2
>>    xlnx,versal-gem         | NULL                      | NET_IP_ALIGN
>
> I don't remember seeing any other driver doing anything like
> this. That often means it is wrong....

Good question, let's look at skb_reserve() that follow dma_map_single():

   ⟩ git grep -A20 dma_map_single drivers/net/ethernet/ | \
      rg skb_reserve | grep -v macb_main
   drivers/net/ethernet/sun/sunbmac.c:          skb_reserve(copy_skb, 2);
   drivers/net/ethernet/sun/sunhme.c:           skb_reserve(skb, RX_OFFSET);
   drivers/net/ethernet/sun/sunhme.c:           skb_reserve(new_skb, RX_OFFSET);
   drivers/net/ethernet/sgi/ioc3-eth.c:         skb_reserve(new_skb, RX_OFFSET);
   drivers/net/ethernet/chelsio/cxgb/sge.c:     skb_reserve(skb, sge->rx_pkt_pad);
   drivers/net/ethernet/marvell/mv643xx_eth.c:  skb_reserve(skb, 2);
   drivers/net/ethernet/dec/tulip/de2104x.c:    skb_reserve(copy_skb, RX_OFFSET);
   drivers/net/ethernet/marvell/pxa168_eth.c:   skb_reserve(skb, ETH_HW_IP_ALIGN);
   drivers/net/ethernet/alacritech/slicoss.c:   skb_reserve(skb, offset);
   drivers/net/ethernet/toshiba/tc35815.c:      skb_reserve(skb, 2); /* make IP header 4byte aligned */
   drivers/net/ethernet/lantiq_etop.c:          skb_reserve(ch->skb[ch->dma.desc], NET_IP_ALIGN);

Out of those, two are using dynamic values:

   // In drivers/net/ethernet/chelsio/cxgb/sge.c
   // The value comes from [0]:
   sge->rx_pkt_pad = t1_is_T1B(adapter) ? 0 : 2;
   // The macro resolves to something like [1]:
   adap->params.chip_version == CHBT_TERM_T1 && adap->params.chip_revision == TERM_T1B

   // In drivers/net/ethernet/alacritech/slicoss.c
   // In slic_refill_rx_queue() [2]
   /* ensure head buffer descriptors are 256 byte aligned */
   offset = 0;
   misalign = paddr & ALIGN_MASK;
   if (misalign) {
      offset = SLIC_RX_BUFF_ALIGN - misalign;
      skb_reserve(skb, offset);
   }

Conclusion:
 - one is HW revision dependent,
 - the other knows that HW always aligns its buffer to 256.

We aren't alone, but pretty lonely.

Maybe I missed a common denominator that could be used to identify
compatibles that do or do not have hardcoded alignemnt. Without such
info, the approach taken (have alignment stored inside match data)
sounds reasonable to me.

Do you agree?

Thanks,

[0]: https://elixir.bootlin.com/linux/v6.13.7/source/drivers/net/ethernet/chelsio/cxgb/sge.c#L2106
[1]: https://elixir.bootlin.com/linux/v6.13.7/source/drivers/net/ethernet/chelsio/cxgb/common.h#L292-L299
[2]: https://elixir.bootlin.com/linux/v6.13.7/source/drivers/net/ethernet/alacritech/slicoss.c#L418-L424

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next 07/13] net: macb: move HW IP alignment value to macb_config

[Andrew Lunn]

On Mon, Mar 24, 2025 at 06:49:05PM +0100, Théo Lebrun wrote:
> Hello Andrew,
> 
> On Fri Mar 21, 2025 at 10:06 PM CET, Andrew Lunn wrote:
> > On Fri, Mar 21, 2025 at 08:09:38PM +0100, Théo Lebrun wrote:
> >> The controller does IP alignment (two bytes).
> >
> > I'm a bit confused here. Is this hard coded, baked into the silicon?
> > It will always do IP alignment? It cannot be turned off?
> 
> Yes, the alignment is baked inside the silicon.
> I looked but haven't seen any register to configure the alignment.
> 
> Sorry the commit message isn't clear, it needs improvements.
> 
> >> 	skb_reserve(skb, NET_IP_ALIGN);
> >
> > Why not just replace this with
> >
> >         skb_reserve(skb, 2);
> 
> On arm64, NET_IP_ALIGN=0. I don't have HW to test, but the current code
> is telling us that the silicon doesn't do alignment on those:

This is part of the confusion. You say the hardware does alignment,
and then say it does not....

>    skb = netdev_alloc_skb(...);
>    paddr = dma_map_single(..., skb->data, ...);
>    macb_set_addr(..., paddr);
> 
>    // arm   => NET_IP_ALIGN=2 => silicon does alignment
>    // arm64 => NET_IP_ALIGN=0 => silicon doesn't do alignment
>    skb_reserve(skb, NET_IP_ALIGN);
> 
> The platform we introduce is the first one where the silicon alignment
> (0 bytes) is different from the NET_IP_ALIGN value (MIPS, 2 bytes).

This is starting to make it clearer. So the first statement that the
controller does IP alignment (two bytes) is not the full story. I
would start there, explain the full story, otherwise readers get the
wrong idea.

> >>     Compatible             |  DTS folders              |  hw_ip_align
> >>    ------------------------|---------------------------|----------------
> >>    cdns,at91sam9260-macb   | arch/arm/                 | 2
> >>    cdns,macb               | arch/{arm,riscv}/         | NET_IP_ALIGN
> >>    cdns,np4-macb           | NULL                      | NET_IP_ALIGN
> >>    cdns,pc302-gem          | NULL                      | NET_IP_ALIGN
> >>    cdns,gem                | arch/{arm,arm64}/         | NET_IP_ALIGN
> >>    cdns,sam9x60-macb       | arch/arm/                 | 2
> >>    atmel,sama5d2-gem       | arch/arm/                 | 2
> >>    atmel,sama5d29-gem      | arch/arm/                 | 2
> >>    atmel,sama5d3-gem       | arch/arm/                 | 2
> >>    atmel,sama5d3-macb      | arch/arm/                 | 2
> >>    atmel,sama5d4-gem       | arch/arm/                 | 2
> >>    cdns,at91rm9200-emac    | arch/arm/                 | 2
> >>    cdns,emac               | arch/arm/                 | 2
> >>    cdns,zynqmp-gem         | *same as xlnx,zynqmp-gem* | 0
> >>    cdns,zynq-gem           | *same as xlnx,zynq-gem*   | 2
> >>    sifive,fu540-c000-gem   | arch/riscv/               | 2
> >>    microchip,mpfs-macb     | arch/riscv/               | 2
> >>    microchip,sama7g5-gem   | arch/arm/                 | 2
> >>    microchip,sama7g5-emac  | arch/arm/                 | 2
> >>    xlnx,zynqmp-gem         | arch/arm64/               | 0
> >>    xlnx,zynq-gem           | arch/arm/                 | 2
> >>    xlnx,versal-gem         | NULL                      | NET_IP_ALIGN

I'm not sure this table is useful. What might be more interesting is

     Compatible             |  architecture |  hw_ip_align | value of NET_IP_ALIGN

We can then see if there are cases when the 3rd and 4th column differ.

	Andrew

RE: [PATCH net-next 07/13] net: macb: move HW IP alignment value to macb_config

[Katakam, Harini]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Theo,

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Tuesday, March 25, 2025 12:06 AM
> To: Théo Lebrun <theo.lebrun@bootlin.com>
> Cc: Andrew Lunn <andrew+netdev@lunn.ch>; David S. Miller
> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub Kicinski
> <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Rob Herring
> <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor Dooley
> <conor+dt@kernel.org>; Nicolas Ferre <nicolas.ferre@microchip.com>; Claudiu
> Beznea <claudiu.beznea@tuxon.dev>; Paul Walmsley
> <paul.walmsley@sifive.com>; Palmer Dabbelt <palmer@dabbelt.com>; Albert Ou
> <aou@eecs.berkeley.edu>; Alexandre Ghiti <alex@ghiti.fr>; Samuel Holland
> <samuel.holland@sifive.com>; Richard Cochran <richardcochran@gmail.com>;
> Russell King <linux@armlinux.org.uk>; Thomas Bogendoerfer
> <tsbogend@alpha.franken.de>; Vladimir Kondratiev
> <vladimir.kondratiev@mobileye.com>; Gregory CLEMENT
> <gregory.clement@bootlin.com>; netdev@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> riscv@lists.infradead.org; linux-mips@vger.kernel.org; Thomas Petazzoni
> <thomas.petazzoni@bootlin.com>; Tawfik Bayouk <tawfik.bayouk@mobileye.com>
> Subject: Re: [PATCH net-next 07/13] net: macb: move HW IP alignment value to
> macb_config
>
> On Mon, Mar 24, 2025 at 06:49:05PM +0100, Théo Lebrun wrote:
> > Hello Andrew,
> >
> > On Fri Mar 21, 2025 at 10:06 PM CET, Andrew Lunn wrote:
> > > On Fri, Mar 21, 2025 at 08:09:38PM +0100, Théo Lebrun wrote:
> > >> The controller does IP alignment (two bytes).
> > >
> > > I'm a bit confused here. Is this hard coded, baked into the silicon?
> > > It will always do IP alignment? It cannot be turned off?
> >
> > Yes, the alignment is baked inside the silicon.
> > I looked but haven't seen any register to configure the alignment.
> >
> > Sorry the commit message isn't clear, it needs improvements.
> >
> > >>  skb_reserve(skb, NET_IP_ALIGN);
> > >
> > > Why not just replace this with
> > >
> > >         skb_reserve(skb, 2);
> >
> > On arm64, NET_IP_ALIGN=0. I don't have HW to test, but the current
> > code is telling us that the silicon doesn't do alignment on those:
>
> This is part of the confusion. You say the hardware does alignment, and then say it
> does not....
>
> >    skb = netdev_alloc_skb(...);
> >    paddr = dma_map_single(..., skb->data, ...);
> >    macb_set_addr(..., paddr);
> >
> >    // arm   => NET_IP_ALIGN=2 => silicon does alignment
> >    // arm64 => NET_IP_ALIGN=0 => silicon doesn't do alignment
> >    skb_reserve(skb, NET_IP_ALIGN);
> >
> > The platform we introduce is the first one where the silicon alignment
> > (0 bytes) is different from the NET_IP_ALIGN value (MIPS, 2 bytes).
>
> This is starting to make it clearer. So the first statement that the controller does IP
> alignment (two bytes) is not the full story. I would start there, explain the full story,
> otherwise readers get the wrong idea.
>
> > >>     Compatible             |  DTS folders              |  hw_ip_align
> > >>    ------------------------|---------------------------|----------------
> > >>    cdns,at91sam9260-macb   | arch/arm/                 | 2
> > >>    cdns,macb               | arch/{arm,riscv}/         | NET_IP_ALIGN
> > >>    cdns,np4-macb           | NULL                      | NET_IP_ALIGN
> > >>    cdns,pc302-gem          | NULL                      | NET_IP_ALIGN
> > >>    cdns,gem                | arch/{arm,arm64}/         | NET_IP_ALIGN
> > >>    cdns,sam9x60-macb       | arch/arm/                 | 2
> > >>    atmel,sama5d2-gem       | arch/arm/                 | 2
> > >>    atmel,sama5d29-gem      | arch/arm/                 | 2
> > >>    atmel,sama5d3-gem       | arch/arm/                 | 2
> > >>    atmel,sama5d3-macb      | arch/arm/                 | 2
> > >>    atmel,sama5d4-gem       | arch/arm/                 | 2
> > >>    cdns,at91rm9200-emac    | arch/arm/                 | 2
> > >>    cdns,emac               | arch/arm/                 | 2
> > >>    cdns,zynqmp-gem         | *same as xlnx,zynqmp-gem* | 0
> > >>    cdns,zynq-gem           | *same as xlnx,zynq-gem*   | 2
> > >>    sifive,fu540-c000-gem   | arch/riscv/               | 2
> > >>    microchip,mpfs-macb     | arch/riscv/               | 2
> > >>    microchip,sama7g5-gem   | arch/arm/                 | 2
> > >>    microchip,sama7g5-emac  | arch/arm/                 | 2
> > >>    xlnx,zynqmp-gem         | arch/arm64/               | 0
> > >>    xlnx,zynq-gem           | arch/arm/                 | 2
> > >>    xlnx,versal-gem         | NULL                      | NET_IP_ALIGN

Thanks for the patch. xlnx,versal-gem is arm64 and NET_IP_ALIGN is 0.

AFAIK, IP alignment is controlled by the register field " receive buffer offset "
in the NW config register. The only exception is when " gem_pbuf_rsc " i.e.
receive coalescing is enabled in the RTL in the IP. In that case, the Cadenc
specification states that these bits are ignored.
So to summarize, if RSC is not enabled (see bit 26 of designcfg_debug6),
then the current implementation works for all architectures i.e. these two
statements are in sync:
config |= MACB_BF(RBOF, NET_IP_ALIGN);  /* Make eth data aligned */
skb_reserve(skb, NET_IP_ALIGN);

Hope this helps simplify the patch (and also fill up the table that Andrew suggested)

Regards,
Harini

>
> I'm not sure this table is useful. What might be more interesting is
>
>      Compatible             |  architecture |  hw_ip_align | value of NET_IP_ALIGN
>
> We can then see if there are cases when the 3rd and 4th column differ.
>
>       Andrew

Re: [PATCH net-next 07/13] net: macb: move HW IP alignment value to macb_config

[Théo Lebrun]

Hello Harini, Andrew,

On Wed Mar 26, 2025 at 6:01 AM CET, Katakam, Harini wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
>
> Hi Theo,
>
>> -----Original Message-----
>> From: Andrew Lunn <andrew@lunn.ch>
>> Sent: Tuesday, March 25, 2025 12:06 AM
>> To: Théo Lebrun <theo.lebrun@bootlin.com>
>> Cc: Andrew Lunn <andrew+netdev@lunn.ch>; David S. Miller
>> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub Kicinski
>> <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Rob Herring
>> <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor Dooley
>> <conor+dt@kernel.org>; Nicolas Ferre <nicolas.ferre@microchip.com>; Claudiu
>> Beznea <claudiu.beznea@tuxon.dev>; Paul Walmsley
>> <paul.walmsley@sifive.com>; Palmer Dabbelt <palmer@dabbelt.com>; Albert Ou
>> <aou@eecs.berkeley.edu>; Alexandre Ghiti <alex@ghiti.fr>; Samuel Holland
>> <samuel.holland@sifive.com>; Richard Cochran <richardcochran@gmail.com>;
>> Russell King <linux@armlinux.org.uk>; Thomas Bogendoerfer
>> <tsbogend@alpha.franken.de>; Vladimir Kondratiev
>> <vladimir.kondratiev@mobileye.com>; Gregory CLEMENT
>> <gregory.clement@bootlin.com>; netdev@vger.kernel.org;
>> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
>> riscv@lists.infradead.org; linux-mips@vger.kernel.org; Thomas Petazzoni
>> <thomas.petazzoni@bootlin.com>; Tawfik Bayouk <tawfik.bayouk@mobileye.com>
>> Subject: Re: [PATCH net-next 07/13] net: macb: move HW IP alignment value to
>> macb_config
>>
>> On Mon, Mar 24, 2025 at 06:49:05PM +0100, Théo Lebrun wrote:
>> > Hello Andrew,
>> >
>> > On Fri Mar 21, 2025 at 10:06 PM CET, Andrew Lunn wrote:
>> > > On Fri, Mar 21, 2025 at 08:09:38PM +0100, Théo Lebrun wrote:
>> > >> The controller does IP alignment (two bytes).
>> > >
>> > > I'm a bit confused here. Is this hard coded, baked into the silicon?
>> > > It will always do IP alignment? It cannot be turned off?
>> >
>> > Yes, the alignment is baked inside the silicon.
>> > I looked but haven't seen any register to configure the alignment.
>> >
>> > Sorry the commit message isn't clear, it needs improvements.
>> >
>> > >>  skb_reserve(skb, NET_IP_ALIGN);
>> > >
>> > > Why not just replace this with
>> > >
>> > >         skb_reserve(skb, 2);
>> >
>> > On arm64, NET_IP_ALIGN=0. I don't have HW to test, but the current
>> > code is telling us that the silicon doesn't do alignment on those:
>>
>> This is part of the confusion. You say the hardware does alignment, and then say it
>> does not....
>>
>> >    skb = netdev_alloc_skb(...);
>> >    paddr = dma_map_single(..., skb->data, ...);
>> >    macb_set_addr(..., paddr);
>> >
>> >    // arm   => NET_IP_ALIGN=2 => silicon does alignment
>> >    // arm64 => NET_IP_ALIGN=0 => silicon doesn't do alignment
>> >    skb_reserve(skb, NET_IP_ALIGN);
>> >
>> > The platform we introduce is the first one where the silicon alignment
>> > (0 bytes) is different from the NET_IP_ALIGN value (MIPS, 2 bytes).
>>
>> This is starting to make it clearer. So the first statement that the controller does IP
>> alignment (two bytes) is not the full story. I would start there, explain the full story,
>> otherwise readers get the wrong idea.
>>
>> > >>     Compatible             |  DTS folders              |  hw_ip_align
>> > >>    ------------------------|---------------------------|----------------
>> > >>    cdns,at91sam9260-macb   | arch/arm/                 | 2
>> > >>    cdns,macb               | arch/{arm,riscv}/         | NET_IP_ALIGN
>> > >>    cdns,np4-macb           | NULL                      | NET_IP_ALIGN
>> > >>    cdns,pc302-gem          | NULL                      | NET_IP_ALIGN
>> > >>    cdns,gem                | arch/{arm,arm64}/         | NET_IP_ALIGN
>> > >>    cdns,sam9x60-macb       | arch/arm/                 | 2
>> > >>    atmel,sama5d2-gem       | arch/arm/                 | 2
>> > >>    atmel,sama5d29-gem      | arch/arm/                 | 2
>> > >>    atmel,sama5d3-gem       | arch/arm/                 | 2
>> > >>    atmel,sama5d3-macb      | arch/arm/                 | 2
>> > >>    atmel,sama5d4-gem       | arch/arm/                 | 2
>> > >>    cdns,at91rm9200-emac    | arch/arm/                 | 2
>> > >>    cdns,emac               | arch/arm/                 | 2
>> > >>    cdns,zynqmp-gem         | *same as xlnx,zynqmp-gem* | 0
>> > >>    cdns,zynq-gem           | *same as xlnx,zynq-gem*   | 2
>> > >>    sifive,fu540-c000-gem   | arch/riscv/               | 2
>> > >>    microchip,mpfs-macb     | arch/riscv/               | 2
>> > >>    microchip,sama7g5-gem   | arch/arm/                 | 2
>> > >>    microchip,sama7g5-emac  | arch/arm/                 | 2
>> > >>    xlnx,zynqmp-gem         | arch/arm64/               | 0
>> > >>    xlnx,zynq-gem           | arch/arm/                 | 2
>> > >>    xlnx,versal-gem         | NULL                      | NET_IP_ALIGN
>
> Thanks for the patch. xlnx,versal-gem is arm64 and NET_IP_ALIGN is 0.
>
> AFAIK, IP alignment is controlled by the register field " receive buffer offset "
> in the NW config register. The only exception is when " gem_pbuf_rsc " i.e.
> receive coalescing is enabled in the RTL in the IP. In that case, the Cadenc
> specification states that these bits are ignored.
> So to summarize, if RSC is not enabled (see bit 26 of designcfg_debug6),
> then the current implementation works for all architectures i.e. these two
> statements are in sync:
> config |= MACB_BF(RBOF, NET_IP_ALIGN);  /* Make eth data aligned */
> skb_reserve(skb, NET_IP_ALIGN);
>
> Hope this helps simplify the patch (and also fill up the table that Andrew suggested)

Well, big thanks! That'll make the patch much simpler. Either EyeQ5
is the first compatible with RSC enabled, or others with RSC enabled
have NET_IP_ALIGN=0.

Below is what the patch could look like for V2.
 - We detect at probe if the HW is RSC-capable.
 - If it isn't, we keep the code the same.
 - If it is, that means the alignment feature isn't available.
   We can't respect our arch alignment request.

That removes all the macb_config->hw_ip_align mess. Much better.

Note: I tried checking if the RBOF field is read-only when the "receive
buffer offset" isn't available but it isn't.

-

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index d8ee7878e144..478152f70563 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -525,6 +525,8 @@
 /* Bitfields in DCFG6. */
 #define GEM_PBUF_LSO_OFFSET        27
 #define GEM_PBUF_LSO_SIZE       1
+#define GEM_PBUF_RSC_OFFSET        26
+#define GEM_PBUF_RSC_SIZE       1
 #define GEM_PBUF_CUTTHRU_OFFSET       25
 #define GEM_PBUF_CUTTHRU_SIZE         1
 #define GEM_DAW64_OFFSET        23
@@ -736,6 +738,7 @@
 #define MACB_CAPS_NEED_TSUCLK         BIT(10)
 #define MACB_CAPS_QUEUE_DISABLE       BIT(11)
 #define MACB_CAPS_NO_LSO        BIT(12)
+#define MACB_CAPS_RSC_CAPABLE         BIT(13)
 #define MACB_CAPS_PCS           BIT(24)
 #define MACB_CAPS_HIGH_SPEED       BIT(25)
 #define MACB_CAPS_CLK_HW_CHG       BIT(26)
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index db8da8590fe0..51e82d66403b 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -1329,8 +1329,19 @@ static void gem_rx_refill(struct macb_queue *queue)
         dma_wmb();
         macb_set_addr(bp, desc, paddr);

-        /* Properly align Ethernet header. */
-        skb_reserve(skb, NET_IP_ALIGN);
+        /* Properly align Ethernet header.
+         *
+         * Hardware can add dummy bytes if asked using the RBOF
+         * field inside the NCFGR register. That feature isn't
+         * available if hardware is RSC capable.
+         *
+         * We cannot fallback to doing the 2-byte shift before
+         * DMA mapping because the address field does not allow
+         * setting the low 2/3 bits.
+         * It is 3 bits if HW_DMA_CAP_PTP.
+         */
+        if (!(bp->caps & MACB_CAPS_RSC_CAPABLE))
+           skb_reserve(skb, NET_IP_ALIGN);
      } else {
         desc->ctrl = 0;
         dma_wmb();
@@ -2788,7 +2799,9 @@ static void macb_init_hw(struct macb *bp)
   macb_set_hwaddr(bp);

   config = macb_mdc_clk_div(bp);
-  config |= MACB_BF(RBOF, NET_IP_ALIGN); /* Make eth data aligned */
+  /* Make eth data aligned. If RSC capable, that offset is ignored. */
+  if (!(bp->caps & MACB_CAPS_RSC_CAPABLE))
+     config |= MACB_BF(RBOF, NET_IP_ALIGN);
   config |= MACB_BIT(DRFCS);    /* Discard Rx FCS */
   if (bp->caps & MACB_CAPS_JUMBO)
      config |= MACB_BIT(JFRAME);   /* Enable jumbo frames */
@@ -4109,6 +4122,8 @@ static void macb_configure_caps(struct macb *bp,
      dcfg = gem_readl(bp, DCFG2);
      if ((dcfg & (GEM_BIT(RX_PKT_BUFF) | GEM_BIT(TX_PKT_BUFF))) == 0)
         bp->caps |= MACB_CAPS_FIFO_MODE;
+     if (GEM_BFEXT(PBUF_RSC, gem_readl(bp, DCFG6)))
+        bp->caps |= MACB_CAPS_RSC_CAPABLE;
      if (gem_has_ptp(bp)) {
         if (!GEM_BFEXT(TSU, gem_readl(bp, DCFG5)))
            dev_err(&bp->pdev->dev,

Thanks Andrew & Harini,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH net-next 08/13] net: macb: introduce DMA descriptor helpers (is 64bit? is PTP?)

[Théo Lebrun]

Introduce macb_dma_is_64b() and macb_dma_is_ptp() helper functions.
Many codepaths are made simpler by dropping conditional compilation.

This implies three changes:
 - Always compile related structure definitions inside <macb.h>.
 - Make the field hw_dma_cap in struct macb always present.
 - MACB_EXT_DESC can be dropped as it is useless now.

The common case is:

	#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
		struct macb_dma_desc_64 *desc_64;
		if (bp->hw_dma_cap & HW_DMA_CAP_64B) {
			desc_64 = macb_64b_desc(bp, desc);
			// ...
		}
	#endif

And replaced by:

	struct macb_dma_desc_64 *desc_64;
	if (macb_dma_is_64b(bp)) {
		desc_64 = macb_64b_desc(bp, desc);
		// ...
	}

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/net/ethernet/cadence/macb.h      |   8 ---
 drivers/net/ethernet/cadence/macb_main.c | 110 +++++++++++--------------------
 2 files changed, 38 insertions(+), 80 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 5bf7e7ff70490cdb068bfdbe7cfd5bb8e1db7f86..26e0af44a45926c782cf0f72184332ab3605a178 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -15,10 +15,6 @@
 #include <linux/phy/phy.h>
 #include <linux/workqueue.h>
 
-#if defined(CONFIG_ARCH_DMA_ADDR_T_64BIT) || defined(CONFIG_MACB_USE_HWSTAMP)
-#define MACB_EXT_DESC
-#endif
-
 #define MACB_GREGS_NBR 16
 #define MACB_GREGS_VERSION 2
 #define MACB_MAX_QUEUES 8
@@ -824,7 +820,6 @@ struct macb_dma_desc {
 	u32	ctrl;
 };
 
-#ifdef MACB_EXT_DESC
 #define HW_DMA_CAP_32B		0
 #define HW_DMA_CAP_64B		(1 << 0)
 #define HW_DMA_CAP_PTP		(1 << 1)
@@ -839,7 +834,6 @@ struct macb_dma_desc_ptp {
 	u32	ts_1;
 	u32	ts_2;
 };
-#endif
 
 /* DMA descriptor bitfields */
 #define MACB_RX_USED_OFFSET			0
@@ -1319,9 +1313,7 @@ struct macb {
 
 	struct phy		*sgmii_phy;	/* for ZynqMP SGMII mode */
 
-#ifdef MACB_EXT_DESC
 	uint8_t hw_dma_cap;
-#endif
 	spinlock_t tsu_clk_lock; /* gem tsu clock locking */
 	unsigned int tsu_rate;
 	struct ptp_clock *ptp_clock;
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index b32363ba1ec3be0fc42866c8585f0b465d178220..ad154cfe29106f642b32922fd4a03ca63112f4a7 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -98,6 +98,18 @@ struct sifive_fu540_macb_mgmt {
 
 #define MACB_MDIO_TIMEOUT	1000000 /* in usecs */
 
+static bool macb_dma_is_64b(struct macb *bp)
+{
+	return IS_ENABLED(CONFIG_ARCH_DMA_ADDR_T_64BIT) &&
+	       bp->hw_dma_cap & HW_DMA_CAP_64B;
+}
+
+static bool macb_dma_is_ptp(struct macb *bp)
+{
+	return IS_ENABLED(CONFIG_MACB_USE_HWSTAMP) &&
+	       bp->hw_dma_cap & HW_DMA_CAP_PTP;
+}
+
 /* DMA buffer descriptor might be different size
  * depends on hardware configuration:
  *
@@ -127,56 +139,31 @@ struct sifive_fu540_macb_mgmt {
  */
 static unsigned int macb_dma_desc_get_size(struct macb *bp)
 {
-#ifdef MACB_EXT_DESC
-	unsigned int desc_size;
+	unsigned int desc_size = sizeof(struct macb_dma_desc);
+
+	if (macb_dma_is_64b(bp))
+		desc_size += sizeof(struct macb_dma_desc_64);
+	if (macb_dma_is_ptp(bp))
+		desc_size += sizeof(struct macb_dma_desc_ptp);
 
-	switch (bp->hw_dma_cap) {
-	case HW_DMA_CAP_64B:
-		desc_size = sizeof(struct macb_dma_desc)
-			+ sizeof(struct macb_dma_desc_64);
-		break;
-	case HW_DMA_CAP_PTP:
-		desc_size = sizeof(struct macb_dma_desc)
-			+ sizeof(struct macb_dma_desc_ptp);
-		break;
-	case HW_DMA_CAP_64B_PTP:
-		desc_size = sizeof(struct macb_dma_desc)
-			+ sizeof(struct macb_dma_desc_64)
-			+ sizeof(struct macb_dma_desc_ptp);
-		break;
-	default:
-		desc_size = sizeof(struct macb_dma_desc);
-	}
 	return desc_size;
-#endif
-	return sizeof(struct macb_dma_desc);
 }
 
 static unsigned int macb_adj_dma_desc_idx(struct macb *bp, unsigned int desc_idx)
 {
-#ifdef MACB_EXT_DESC
-	switch (bp->hw_dma_cap) {
-	case HW_DMA_CAP_64B:
-	case HW_DMA_CAP_PTP:
-		desc_idx <<= 1;
-		break;
-	case HW_DMA_CAP_64B_PTP:
-		desc_idx *= 3;
-		break;
-	default:
-		break;
-	}
-#endif
-	return desc_idx;
+	if (macb_dma_is_64b(bp) && macb_dma_is_ptp(bp))
+		return desc_idx * 3;
+	else if (macb_dma_is_64b(bp) || macb_dma_is_ptp(bp))
+		return desc_idx << 1;
+	else
+		return desc_idx;
 }
 
-#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
 static struct macb_dma_desc_64 *macb_64b_desc(struct macb *bp, struct macb_dma_desc *desc)
 {
 	return (struct macb_dma_desc_64 *)((void *)desc
 		+ sizeof(struct macb_dma_desc));
 }
-#endif
 
 /* Ring buffer accessors */
 static unsigned int macb_tx_ring_wrap(struct macb *bp, unsigned int index)
@@ -500,17 +487,13 @@ static void macb_init_buffers(struct macb *bp)
 
 	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
 		queue_writel(queue, RBQP, lower_32_bits(queue->rx_ring_dma));
-#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
-		if (bp->hw_dma_cap & HW_DMA_CAP_64B)
+		if (macb_dma_is_64b(bp))
 			queue_writel(queue, RBQPH,
 				     upper_32_bits(queue->rx_ring_dma));
-#endif
 		queue_writel(queue, TBQP, lower_32_bits(queue->tx_ring_dma));
-#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
-		if (bp->hw_dma_cap & HW_DMA_CAP_64B)
+		if (macb_dma_is_64b(bp))
 			queue_writel(queue, TBQPH,
 				     upper_32_bits(queue->tx_ring_dma));
-#endif
 	}
 }
 
@@ -1038,10 +1021,9 @@ static void macb_tx_unmap(struct macb *bp, struct macb_tx_skb *tx_skb, int budge
 
 static void macb_set_addr(struct macb *bp, struct macb_dma_desc *desc, dma_addr_t addr)
 {
-#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
 	struct macb_dma_desc_64 *desc_64;
 
-	if (bp->hw_dma_cap & HW_DMA_CAP_64B) {
+	if (macb_dma_is_64b(bp)) {
 		desc_64 = macb_64b_desc(bp, desc);
 		desc_64->addrh = upper_32_bits(addr);
 		/* The low bits of RX address contain the RX_USED bit, clearing
@@ -1050,26 +1032,22 @@ static void macb_set_addr(struct macb *bp, struct macb_dma_desc *desc, dma_addr_
 		 */
 		dma_wmb();
 	}
-#endif
+
 	desc->addr = lower_32_bits(addr);
 }
 
 static dma_addr_t macb_get_addr(struct macb *bp, struct macb_dma_desc *desc)
 {
-	dma_addr_t addr = 0;
-#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
 	struct macb_dma_desc_64 *desc_64;
+	dma_addr_t addr = 0;
 
-	if (bp->hw_dma_cap & HW_DMA_CAP_64B) {
+	if (macb_dma_is_64b(bp)) {
 		desc_64 = macb_64b_desc(bp, desc);
 		addr = ((u64)(desc_64->addrh) << 32);
 	}
-#endif
 	addr |= MACB_BF(RX_WADDR, MACB_BFEXT(RX_WADDR, desc->addr));
-#ifdef CONFIG_MACB_USE_HWSTAMP
-	if (bp->hw_dma_cap & HW_DMA_CAP_PTP)
+	if (macb_dma_is_ptp(bp))
 		addr &= ~GEM_BIT(DMA_RXVALID);
-#endif
 	return addr;
 }
 
@@ -1176,10 +1154,8 @@ static void macb_tx_error_task(struct work_struct *work)
 
 	/* Reinitialize the TX desc queue */
 	queue_writel(queue, TBQP, lower_32_bits(queue->tx_ring_dma));
-#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
-	if (bp->hw_dma_cap & HW_DMA_CAP_64B)
+	if (macb_dma_is_64b(bp))
 		queue_writel(queue, TBQPH, upper_32_bits(queue->tx_ring_dma));
-#endif
 	/* Make TX ring reflect state of hardware */
 	queue->tx_head = 0;
 	queue->tx_tail = 0;
@@ -2349,11 +2325,9 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
 		return ret;
 	}
 
-#ifdef CONFIG_MACB_USE_HWSTAMP
-	if ((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
-	    (bp->hw_dma_cap & HW_DMA_CAP_PTP))
+	if (macb_dma_is_ptp(bp) &&
+	    (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP))
 		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
-#endif
 
 	is_lso = (skb_shinfo(skb)->gso_size != 0);
 
@@ -2813,14 +2787,10 @@ static void macb_configure_dma(struct macb *bp)
 			dmacfg &= ~GEM_BIT(TXCOEN);
 
 		dmacfg &= ~GEM_BIT(ADDR64);
-#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
-		if (bp->hw_dma_cap & HW_DMA_CAP_64B)
+		if (macb_dma_is_64b(bp))
 			dmacfg |= GEM_BIT(ADDR64);
-#endif
-#ifdef CONFIG_MACB_USE_HWSTAMP
-		if (bp->hw_dma_cap & HW_DMA_CAP_PTP)
+		if (macb_dma_is_ptp(bp))
 			dmacfg |= GEM_BIT(RXEXT) | GEM_BIT(TXEXT);
-#endif
 		netdev_dbg(bp->dev, "Cadence configure DMA with 0x%08x\n",
 			   dmacfg);
 		gem_writel(bp, DMACFG, dmacfg);
@@ -4326,12 +4296,10 @@ static int macb_init(struct platform_device *pdev)
 			queue->TBQP = GEM_TBQP(hw_q - 1);
 			queue->RBQP = GEM_RBQP(hw_q - 1);
 			queue->RBQS = GEM_RBQS(hw_q - 1);
-#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
-			if (bp->hw_dma_cap & HW_DMA_CAP_64B) {
+			if (macb_dma_is_64b(bp)) {
 				queue->TBQPH = GEM_TBQPH(hw_q - 1);
 				queue->RBQPH = GEM_RBQPH(hw_q - 1);
 			}
-#endif
 		} else {
 			/* queue0 uses legacy registers */
 			queue->ISR  = MACB_ISR;
@@ -4340,12 +4308,10 @@ static int macb_init(struct platform_device *pdev)
 			queue->IMR  = MACB_IMR;
 			queue->TBQP = MACB_TBQP;
 			queue->RBQP = MACB_RBQP;
-#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
-			if (bp->hw_dma_cap & HW_DMA_CAP_64B) {
+			if (macb_dma_is_64b(bp)) {
 				queue->TBQPH = MACB_TBQPH;
 				queue->RBQPH = MACB_RBQPH;
 			}
-#endif
 		}
 
 		/* get irq: here we use the linux queue index, not the hardware

-- 
2.48.1

Re: [PATCH net-next 08/13] net: macb: introduce DMA descriptor helpers (is 64bit? is PTP?)

[Claudiu Beznea]

I think the "(is 64bit? is PTP?)" from title could be dropped.

On 21.03.2025 21:09, Théo Lebrun wrote:
> Introduce macb_dma_is_64b() and macb_dma_is_ptp() helper functions.
> Many codepaths are made simpler by dropping conditional compilation.
> 
> This implies three changes:
>  - Always compile related structure definitions inside <macb.h>.
>  - Make the field hw_dma_cap in struct macb always present.
>  - MACB_EXT_DESC can be dropped as it is useless now.
> 
> The common case is:
> 
> 	#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> 		struct macb_dma_desc_64 *desc_64;
> 		if (bp->hw_dma_cap & HW_DMA_CAP_64B) {
> 			desc_64 = macb_64b_desc(bp, desc);
> 			// ...
> 		}
> 	#endif
> 
> And replaced by:
> 
> 	struct macb_dma_desc_64 *desc_64;
> 	if (macb_dma_is_64b(bp)) {
> 		desc_64 = macb_64b_desc(bp, desc);
> 		// ...
> 	}
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  drivers/net/ethernet/cadence/macb.h      |   8 ---
>  drivers/net/ethernet/cadence/macb_main.c | 110 +++++++++++--------------------
>  2 files changed, 38 insertions(+), 80 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 5bf7e7ff70490cdb068bfdbe7cfd5bb8e1db7f86..26e0af44a45926c782cf0f72184332ab3605a178 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -15,10 +15,6 @@
>  #include <linux/phy/phy.h>
>  #include <linux/workqueue.h>
>  
> -#if defined(CONFIG_ARCH_DMA_ADDR_T_64BIT) || defined(CONFIG_MACB_USE_HWSTAMP)
> -#define MACB_EXT_DESC
> -#endif
> -
>  #define MACB_GREGS_NBR 16
>  #define MACB_GREGS_VERSION 2
>  #define MACB_MAX_QUEUES 8
> @@ -824,7 +820,6 @@ struct macb_dma_desc {
>  	u32	ctrl;
>  };
>  
> -#ifdef MACB_EXT_DESC
>  #define HW_DMA_CAP_32B		0
>  #define HW_DMA_CAP_64B		(1 << 0)
>  #define HW_DMA_CAP_PTP		(1 << 1)
> @@ -839,7 +834,6 @@ struct macb_dma_desc_ptp {
>  	u32	ts_1;
>  	u32	ts_2;
>  };
> -#endif
>  
>  /* DMA descriptor bitfields */
>  #define MACB_RX_USED_OFFSET			0
> @@ -1319,9 +1313,7 @@ struct macb {
>  
>  	struct phy		*sgmii_phy;	/* for ZynqMP SGMII mode */
>  
> -#ifdef MACB_EXT_DESC
>  	uint8_t hw_dma_cap;
> -#endif
>  	spinlock_t tsu_clk_lock; /* gem tsu clock locking */
>  	unsigned int tsu_rate;
>  	struct ptp_clock *ptp_clock;
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index b32363ba1ec3be0fc42866c8585f0b465d178220..ad154cfe29106f642b32922fd4a03ca63112f4a7 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -98,6 +98,18 @@ struct sifive_fu540_macb_mgmt {
>  
>  #define MACB_MDIO_TIMEOUT	1000000 /* in usecs */
>  
> +static bool macb_dma_is_64b(struct macb *bp)
> +{
> +	return IS_ENABLED(CONFIG_ARCH_DMA_ADDR_T_64BIT) &&
> +	       bp->hw_dma_cap & HW_DMA_CAP_64B;
> +}
> +
> +static bool macb_dma_is_ptp(struct macb *bp)
> +{
> +	return IS_ENABLED(CONFIG_MACB_USE_HWSTAMP) &&
> +	       bp->hw_dma_cap & HW_DMA_CAP_PTP;
> +}
> +
>  /* DMA buffer descriptor might be different size
>   * depends on hardware configuration:
>   *
> @@ -127,56 +139,31 @@ struct sifive_fu540_macb_mgmt {
>   */
>  static unsigned int macb_dma_desc_get_size(struct macb *bp)
>  {
> -#ifdef MACB_EXT_DESC
> -	unsigned int desc_size;
> +	unsigned int desc_size = sizeof(struct macb_dma_desc);
> +
> +	if (macb_dma_is_64b(bp))
> +		desc_size += sizeof(struct macb_dma_desc_64);
> +	if (macb_dma_is_ptp(bp))
> +		desc_size += sizeof(struct macb_dma_desc_ptp);
>  
> -	switch (bp->hw_dma_cap) {
> -	case HW_DMA_CAP_64B:
> -		desc_size = sizeof(struct macb_dma_desc)
> -			+ sizeof(struct macb_dma_desc_64);
> -		break;
> -	case HW_DMA_CAP_PTP:
> -		desc_size = sizeof(struct macb_dma_desc)
> -			+ sizeof(struct macb_dma_desc_ptp);
> -		break;
> -	case HW_DMA_CAP_64B_PTP:
> -		desc_size = sizeof(struct macb_dma_desc)
> -			+ sizeof(struct macb_dma_desc_64)
> -			+ sizeof(struct macb_dma_desc_ptp);
> -		break;
> -	default:
> -		desc_size = sizeof(struct macb_dma_desc);
> -	}
>  	return desc_size;
> -#endif
> -	return sizeof(struct macb_dma_desc);
>  }
>  
>  static unsigned int macb_adj_dma_desc_idx(struct macb *bp, unsigned int desc_idx)
>  {
> -#ifdef MACB_EXT_DESC
> -	switch (bp->hw_dma_cap) {
> -	case HW_DMA_CAP_64B:
> -	case HW_DMA_CAP_PTP:
> -		desc_idx <<= 1;
> -		break;
> -	case HW_DMA_CAP_64B_PTP:
> -		desc_idx *= 3;
> -		break;
> -	default:
> -		break;
> -	}
> -#endif
> -	return desc_idx;
> +	if (macb_dma_is_64b(bp) && macb_dma_is_ptp(bp))
> +		return desc_idx * 3;
> +	else if (macb_dma_is_64b(bp) || macb_dma_is_ptp(bp))
> +		return desc_idx << 1;
> +	else
> +		return desc_idx;
>  }
>  
> -#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
>  static struct macb_dma_desc_64 *macb_64b_desc(struct macb *bp, struct macb_dma_desc *desc)
>  {
>  	return (struct macb_dma_desc_64 *)((void *)desc
>  		+ sizeof(struct macb_dma_desc));
>  }
> -#endif
>  
>  /* Ring buffer accessors */
>  static unsigned int macb_tx_ring_wrap(struct macb *bp, unsigned int index)
> @@ -500,17 +487,13 @@ static void macb_init_buffers(struct macb *bp)
>  
>  	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
>  		queue_writel(queue, RBQP, lower_32_bits(queue->rx_ring_dma));
> -#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> -		if (bp->hw_dma_cap & HW_DMA_CAP_64B)
> +		if (macb_dma_is_64b(bp))
>  			queue_writel(queue, RBQPH,
>  				     upper_32_bits(queue->rx_ring_dma));
> -#endif
>  		queue_writel(queue, TBQP, lower_32_bits(queue->tx_ring_dma));
> -#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> -		if (bp->hw_dma_cap & HW_DMA_CAP_64B)
> +		if (macb_dma_is_64b(bp))
>  			queue_writel(queue, TBQPH,
>  				     upper_32_bits(queue->tx_ring_dma));
> -#endif
>  	}
>  }
>  
> @@ -1038,10 +1021,9 @@ static void macb_tx_unmap(struct macb *bp, struct macb_tx_skb *tx_skb, int budge
>  
>  static void macb_set_addr(struct macb *bp, struct macb_dma_desc *desc, dma_addr_t addr)
>  {
> -#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
>  	struct macb_dma_desc_64 *desc_64;

This can be moved
>  
> -	if (bp->hw_dma_cap & HW_DMA_CAP_64B) {
> +	if (macb_dma_is_64b(bp)) {

here.

>  		desc_64 = macb_64b_desc(bp, desc);
>  		desc_64->addrh = upper_32_bits(addr);
>  		/* The low bits of RX address contain the RX_USED bit, clearing
> @@ -1050,26 +1032,22 @@ static void macb_set_addr(struct macb *bp, struct macb_dma_desc *desc, dma_addr_
>  		 */
>  		dma_wmb();
>  	}
> -#endif
> +
>  	desc->addr = lower_32_bits(addr);
>  }
>  
>  static dma_addr_t macb_get_addr(struct macb *bp, struct macb_dma_desc *desc)
>  {
> -	dma_addr_t addr = 0;
> -#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
>  	struct macb_dma_desc_64 *desc_64;

Same for this one.

> +	dma_addr_t addr = 0;
>  
> -	if (bp->hw_dma_cap & HW_DMA_CAP_64B) {
> +	if (macb_dma_is_64b(bp)) {
>  		desc_64 = macb_64b_desc(bp, desc);
>  		addr = ((u64)(desc_64->addrh) << 32);
>  	}
> -#endif
>  	addr |= MACB_BF(RX_WADDR, MACB_BFEXT(RX_WADDR, desc->addr));
> -#ifdef CONFIG_MACB_USE_HWSTAMP
> -	if (bp->hw_dma_cap & HW_DMA_CAP_PTP)
> +	if (macb_dma_is_ptp(bp))
>  		addr &= ~GEM_BIT(DMA_RXVALID);
> -#endif
>  	return addr;
>  }
>  
> @@ -1176,10 +1154,8 @@ static void macb_tx_error_task(struct work_struct *work)
>  
>  	/* Reinitialize the TX desc queue */
>  	queue_writel(queue, TBQP, lower_32_bits(queue->tx_ring_dma));
> -#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> -	if (bp->hw_dma_cap & HW_DMA_CAP_64B)
> +	if (macb_dma_is_64b(bp))
>  		queue_writel(queue, TBQPH, upper_32_bits(queue->tx_ring_dma));
> -#endif
>  	/* Make TX ring reflect state of hardware */
>  	queue->tx_head = 0;
>  	queue->tx_tail = 0;
> @@ -2349,11 +2325,9 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  		return ret;
>  	}
>  
> -#ifdef CONFIG_MACB_USE_HWSTAMP
> -	if ((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
> -	    (bp->hw_dma_cap & HW_DMA_CAP_PTP))
> +	if (macb_dma_is_ptp(bp) &&
> +	    (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP))
>  		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> -#endif
>  
>  	is_lso = (skb_shinfo(skb)->gso_size != 0);
>  
> @@ -2813,14 +2787,10 @@ static void macb_configure_dma(struct macb *bp)
>  			dmacfg &= ~GEM_BIT(TXCOEN);
>  
>  		dmacfg &= ~GEM_BIT(ADDR64);
> -#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> -		if (bp->hw_dma_cap & HW_DMA_CAP_64B)
> +		if (macb_dma_is_64b(bp))
>  			dmacfg |= GEM_BIT(ADDR64);
> -#endif
> -#ifdef CONFIG_MACB_USE_HWSTAMP
> -		if (bp->hw_dma_cap & HW_DMA_CAP_PTP)
> +		if (macb_dma_is_ptp(bp))
>  			dmacfg |= GEM_BIT(RXEXT) | GEM_BIT(TXEXT);
> -#endif
>  		netdev_dbg(bp->dev, "Cadence configure DMA with 0x%08x\n",
>  			   dmacfg);
>  		gem_writel(bp, DMACFG, dmacfg);
> @@ -4326,12 +4296,10 @@ static int macb_init(struct platform_device *pdev)
>  			queue->TBQP = GEM_TBQP(hw_q - 1);
>  			queue->RBQP = GEM_RBQP(hw_q - 1);
>  			queue->RBQS = GEM_RBQS(hw_q - 1);
> -#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> -			if (bp->hw_dma_cap & HW_DMA_CAP_64B) {
> +			if (macb_dma_is_64b(bp)) {
>  				queue->TBQPH = GEM_TBQPH(hw_q - 1);
>  				queue->RBQPH = GEM_RBQPH(hw_q - 1);
>  			}
> -#endif
>  		} else {
>  			/* queue0 uses legacy registers */
>  			queue->ISR  = MACB_ISR;
> @@ -4340,12 +4308,10 @@ static int macb_init(struct platform_device *pdev)
>  			queue->IMR  = MACB_IMR;
>  			queue->TBQP = MACB_TBQP;
>  			queue->RBQP = MACB_RBQP;
> -#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> -			if (bp->hw_dma_cap & HW_DMA_CAP_64B) {
> +			if (macb_dma_is_64b(bp)) {
>  				queue->TBQPH = MACB_TBQPH;
>  				queue->RBQPH = MACB_RBQPH;
>  			}
> -#endif
>  		}
>  
>  		/* get irq: here we use the linux queue index, not the hardware
> 

Re: [PATCH net-next 08/13] net: macb: introduce DMA descriptor helpers (is 64bit? is PTP?)

[Maxime Chevallier]

Hello Théo,

On Fri, 21 Mar 2025 20:09:39 +0100
Théo Lebrun <theo.lebrun@bootlin.com> wrote:

> Introduce macb_dma_is_64b() and macb_dma_is_ptp() helper functions.
> Many codepaths are made simpler by dropping conditional compilation.
> 
> This implies three changes:
>  - Always compile related structure definitions inside <macb.h>.
>  - Make the field hw_dma_cap in struct macb always present.
>  - MACB_EXT_DESC can be dropped as it is useless now.
> 
> The common case is:
> 
> 	#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> 		struct macb_dma_desc_64 *desc_64;
> 		if (bp->hw_dma_cap & HW_DMA_CAP_64B) {
> 			desc_64 = macb_64b_desc(bp, desc);
> 			// ...
> 		}
> 	#endif
> 
> And replaced by:
> 
> 	struct macb_dma_desc_64 *desc_64;
> 	if (macb_dma_is_64b(bp)) {
> 		desc_64 = macb_64b_desc(bp, desc);
> 		// ...
> 	}

Just a thought, but this is adding some more branches in the hotpath on
32 bits DMA setups. Did you measure any performance changes on
these platforms (if you have access to one :) )

As the caps can't be changed dynamically, maybe these helpers could be
replaced more efficiently with some static_key ? This would benefit
both 32 and 64 bits systems as the following would be more efficient

	if (bp->hw_dma_cap & HW_DMA_CAP_64B) {
		//  ...
	}

Just a thought of course, maybe this patch doesn't really hurt perfs :)

Maxime

Re: [PATCH net-next 08/13] net: macb: introduce DMA descriptor helpers (is 64bit? is PTP?)

[Théo Lebrun]

Hello Maxime,

On Mon Mar 24, 2025 at 9:55 AM CET, Maxime Chevallier wrote:
> On Fri, 21 Mar 2025 20:09:39 +0100
> Théo Lebrun <theo.lebrun@bootlin.com> wrote:
>
>> Introduce macb_dma_is_64b() and macb_dma_is_ptp() helper functions.
>> Many codepaths are made simpler by dropping conditional compilation.
>> 
>> This implies three changes:
>>  - Always compile related structure definitions inside <macb.h>.
>>  - Make the field hw_dma_cap in struct macb always present.
>>  - MACB_EXT_DESC can be dropped as it is useless now.
>> 
>> The common case is:
>> 
>> 	#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
>> 		struct macb_dma_desc_64 *desc_64;
>> 		if (bp->hw_dma_cap & HW_DMA_CAP_64B) {
>> 			desc_64 = macb_64b_desc(bp, desc);
>> 			// ...
>> 		}
>> 	#endif
>> 
>> And replaced by:
>> 
>> 	struct macb_dma_desc_64 *desc_64;
>> 	if (macb_dma_is_64b(bp)) {
>> 		desc_64 = macb_64b_desc(bp, desc);
>> 		// ...
>> 	}
>
> Just a thought, but this is adding some more branches in the hotpath on
> 32 bits DMA setups. Did you measure any performance changes on
> these platforms (if you have access to one :) )
>
> As the caps can't be changed dynamically, maybe these helpers could be
> replaced more efficiently with some static_key ? This would benefit
> both 32 and 64 bits systems as the following would be more efficient
>
> 	if (bp->hw_dma_cap & HW_DMA_CAP_64B) {
> 		//  ...
> 	}
>
> Just a thought of course, maybe this patch doesn't really hurt perfs :)

Good question! I asked myself the same thing before posting.

We go from:

	void bar(struct macb *bp) {
	#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
		if (bp->hw_dma_cap & HW_DMA_CAP_64B) {
			foo();
		}
	#endif
	}

To:

	static bool macb_dma_is_64b(struct macb *bp)
	{
		return IS_ENABLED(CONFIG_ARCH_DMA_ADDR_T_64BIT) &&
		       bp->hw_dma_cap & HW_DMA_CAP_64B;
	}

	void bar(struct macb *bp) {
		if (macb_dma_is_64b(bp)) {
			foo();
		}
	}

In the first case, we use explicit preprocessor directives to remove
code if CONFIG_ARCH_DMA_ADDR_T_64BIT isn't defined.

In the second case, our compiler optimises away the IS_ENABLED() call.
 - If false, then the branch doesn't appear.
 - If true, then only the capability check is inlined in bar().
I checked the assembly on arm/arm64/MIPS.

Conclusion: the hotpath doesn't change.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH net-next 09/13] net: macb: sort #includes

[Théo Lebrun]

Sort #include preprocessor directives.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 40 ++++++++++++++++----------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index ad154cfe29106f642b32922fd4a03ca63112f4a7..79161d559166478f85a6f8294d488ed961d9be7f 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -6,39 +6,39 @@
  */
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-#include <linux/clk.h>
-#include <linux/clk-provider.h>
-#include <linux/crc32.h>
-#include <linux/module.h>
-#include <linux/moduleparam.h>
-#include <linux/kernel.h>
-#include <linux/types.h>
 #include <linux/circ_buf.h>
-#include <linux/slab.h>
-#include <linux/init.h>
-#include <linux/io.h>
+#include <linux/clk-provider.h>
+#include <linux/clk.h>
+#include <linux/crc32.h>
+#include <linux/dma-mapping.h>
+#include <linux/etherdevice.h>
+#include <linux/firmware/xlnx-zynqmp.h>
 #include <linux/gpio.h>
 #include <linux/gpio/consumer.h>
+#include <linux/inetdevice.h>
+#include <linux/init.h>
 #include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/ip.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
 #include <linux/netdevice.h>
-#include <linux/etherdevice.h>
-#include <linux/dma-mapping.h>
-#include <linux/platform_device.h>
-#include <linux/phylink.h>
 #include <linux/of.h>
 #include <linux/of_gpio.h>
 #include <linux/of_mdio.h>
 #include <linux/of_net.h>
-#include <linux/ip.h>
-#include <linux/udp.h>
-#include <linux/tcp.h>
-#include <linux/iopoll.h>
 #include <linux/phy/phy.h>
+#include <linux/phylink.h>
+#include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/ptp_classify.h>
 #include <linux/reset.h>
-#include <linux/firmware/xlnx-zynqmp.h>
-#include <linux/inetdevice.h>
+#include <linux/slab.h>
+#include <linux/tcp.h>
+#include <linux/types.h>
+#include <linux/udp.h>
 #include "macb.h"
 
 /* This structure is only used for MACB on SiFive FU540 devices */

-- 
2.48.1

Re: [PATCH net-next 09/13] net: macb: sort #includes

[Andrew Lunn]

On Fri, Mar 21, 2025 at 08:09:40PM +0100, Théo Lebrun wrote:
> Sort #include preprocessor directives.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net-next 10/13] net: macb: Add "mobileye,eyeq5-gem" compatible

[Théo Lebrun]

Add support for the two GEM instances inside Mobileye EyeQ5 SoCs, using
compatible "mobileye,eyeq5-gem". With it, add a custom init sequence
that accesses two system-controller registers.

Noteworthy: NET_IP_ALIGN=2 on MIPS but the hardware does not align and
low bits aren't configurable, so we cannot respect the requested IP
header alignment.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 95 ++++++++++++++++++++++++++++++++
 1 file changed, 95 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 79161d559166478f85a6f8294d488ed961d9be7f..9f2a5bf9a5ebca5941229bd96091a0fb96f0607d 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -22,6 +22,7 @@
 #include <linux/iopoll.h>
 #include <linux/ip.h>
 #include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/netdevice.h>
@@ -34,6 +35,7 @@
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/ptp_classify.h>
+#include <linux/regmap.h>
 #include <linux/reset.h>
 #include <linux/slab.h>
 #include <linux/tcp.h>
@@ -4967,6 +4969,86 @@ static int init_reset_optional(struct platform_device *pdev)
 	return ret;
 }
 
+#define EYEQ5_OLB_GP_TX_SWRST_DIS	BIT(0)		// Tx SW reset
+#define EYEQ5_OLB_GP_TX_M_CLKE		BIT(1)		// Tx M clock enable
+#define EYEQ5_OLB_GP_SYS_SWRST_DIS	BIT(2)		// Sys SW reset
+#define EYEQ5_OLB_GP_SYS_M_CLKE		BIT(3)		// Sys clock enable
+#define EYEQ5_OLB_GP_SGMII_MODE		BIT(4)		// SGMII mode
+#define EYEQ5_OLB_GP_RGMII_DRV		GENMASK(8, 5)	// RGMII mode
+#define EYEQ5_OLB_GP_SMA_DRV		GENMASK(12, 9)
+#define EYEQ5_OLB_GP_RGMII_PD		BIT(13)		// RGMII pull-down
+#define EYEQ5_OLB_GP_MDIO_PU		BIT(14)		// RGMII pull-up
+#define EYEQ5_OLB_GP_RGMII_RX_ST	BIT(15)		// Schmitt trigger on RGMII Rx
+#define EYEQ5_OLB_GP_RGMII_TX_ST	BIT(16)		// Schmitt trigger on RGMII Tx
+#define EYEQ5_OLB_GP_MDIO_ST		BIT(17)
+#define EYEQ5_OLB_GP_MDC_ST		BIT(18)
+#define EYEQ5_OLB_GP_MBIST_ENABLE	BIT(19)
+
+#define EYEQ5_OLB_SGMII_PWR_EN		BIT(0)
+#define EYEQ5_OLB_SGMII_RST_DIS		BIT(1)
+#define EYEQ5_OLB_SGMII_PLL_EN		BIT(2)
+#define EYEQ5_OLB_SGMII_SIG_DET_SW	BIT(3)
+#define EYEQ5_OLB_SGMII_PWR_STATE_MASK	GENMASK(8, 4)
+#define EYEQ5_OLB_SGMII_PWR_STATE	BIT(4)
+#define EYEQ5_OLB_SGMII_TX_ELECT_IDLE	BIT(9)		// Tx elect idle
+#define EYEQ5_OLB_SGMII_POWER_ACK	BIT(16)
+#define EYEQ5_OLB_SGMII_PLL_ACK		BIT(18)
+#define EYEQ5_OLB_SGMII_SIG_DET		BIT(19)
+#define EYEQ5_OLB_SGMII_PWR_STATE_ACK	GENMASK(24, 20)
+
+static int eyeq5_init(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct net_device *netdev = platform_get_drvdata(pdev);
+	struct macb *bp = netdev_priv(netdev);
+	struct device_node *np = dev->of_node;
+	unsigned int gp, sgmii;
+	struct regmap *regmap;
+	unsigned int args[2];
+	unsigned int reg;
+	int ret;
+
+	regmap = syscon_regmap_lookup_by_phandle_args(np, "mobileye,olb", 2, args);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	gp = args[0];
+	sgmii = args[1];
+
+	/* Forced reset */
+	regmap_write(regmap, gp, 0);
+	regmap_write(regmap, sgmii, 0);
+	usleep_range(5, 20);
+
+	if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) {
+		regmap_write(regmap, gp, EYEQ5_OLB_GP_SGMII_MODE);
+
+		reg = EYEQ5_OLB_SGMII_PWR_EN | EYEQ5_OLB_SGMII_RST_DIS |
+		      EYEQ5_OLB_SGMII_PLL_EN;
+		regmap_write(regmap, sgmii, reg);
+
+		ret = regmap_read_poll_timeout(regmap, sgmii, reg,
+					       reg & EYEQ5_OLB_SGMII_PLL_ACK,
+					       1, 100);
+		if (ret)
+			return dev_err_probe(dev, ret, "PLL timeout");
+
+		regmap_read(regmap, sgmii, &reg);
+		reg |= EYEQ5_OLB_SGMII_PWR_STATE | EYEQ5_OLB_SGMII_SIG_DET_SW;
+		regmap_write(regmap, sgmii, reg);
+	}
+
+	regmap_read(regmap, gp, &reg);
+	reg &= ~EYEQ5_OLB_GP_RGMII_DRV;
+	if (phy_interface_mode_is_rgmii(bp->phy_interface))
+		reg |= FIELD_PREP(EYEQ5_OLB_GP_RGMII_DRV, 0x9);
+	reg |= EYEQ5_OLB_GP_TX_SWRST_DIS | EYEQ5_OLB_GP_TX_M_CLKE;
+	reg |= EYEQ5_OLB_GP_SYS_SWRST_DIS | EYEQ5_OLB_GP_SYS_M_CLKE;
+	regmap_write(regmap, gp, reg);
+
+	return macb_init(pdev);
+}
+
 static const struct macb_usrio_config sama7g5_usrio = {
 	.mii = 0,
 	.rmii = 1,
@@ -5135,6 +5217,18 @@ static const struct macb_config versal_config = {
 	.usrio = &macb_default_usrio,
 };
 
+static const struct macb_config eyeq5_config = {
+	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO |
+		MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_QUEUE_DISABLE |
+		MACB_CAPS_NO_LSO,
+	.hw_ip_align = 0,
+	.dma_burst_length = 16,
+	.clk_init = macb_clk_init,
+	.init = eyeq5_init,
+	.jumbo_max_len = 10240,
+	.usrio = &macb_default_usrio,
+};
+
 static const struct of_device_id macb_dt_ids[] = {
 	{ .compatible = "cdns,at91sam9260-macb", .data = &at91sam9260_config },
 	{ .compatible = "cdns,macb" },
@@ -5152,6 +5246,7 @@ static const struct of_device_id macb_dt_ids[] = {
 	{ .compatible = "cdns,zynqmp-gem", .data = &zynqmp_config}, /* deprecated */
 	{ .compatible = "cdns,zynq-gem", .data = &zynq_config }, /* deprecated */
 	{ .compatible = "sifive,fu540-c000-gem", .data = &fu540_c000_config },
+	{ .compatible = "mobileye,eyeq5-gem", .data = &eyeq5_config },
 	{ .compatible = "microchip,mpfs-macb", .data = &mpfs_config },
 	{ .compatible = "microchip,sama7g5-gem", .data = &sama7g5_gem_config },
 	{ .compatible = "microchip,sama7g5-emac", .data = &sama7g5_emac_config },

-- 
2.48.1

Re: [PATCH net-next 10/13] net: macb: Add "mobileye,eyeq5-gem" compatible

[Claudiu Beznea]

Hi, Theo,

On 21.03.2025 21:09, Théo Lebrun wrote:
> Add support for the two GEM instances inside Mobileye EyeQ5 SoCs, using
> compatible "mobileye,eyeq5-gem". With it, add a custom init sequence
> that accesses two system-controller registers.
> 
> Noteworthy: NET_IP_ALIGN=2 on MIPS but the hardware does not align and
> low bits aren't configurable, so we cannot respect the requested IP
> header alignment.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 95 ++++++++++++++++++++++++++++++++
>  1 file changed, 95 insertions(+)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 79161d559166478f85a6f8294d488ed961d9be7f..9f2a5bf9a5ebca5941229bd96091a0fb96f0607d 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -22,6 +22,7 @@
>  #include <linux/iopoll.h>
>  #include <linux/ip.h>
>  #include <linux/kernel.h>
> +#include <linux/mfd/syscon.h>
>  #include <linux/module.h>
>  #include <linux/moduleparam.h>
>  #include <linux/netdevice.h>
> @@ -34,6 +35,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/ptp_classify.h>
> +#include <linux/regmap.h>
>  #include <linux/reset.h>
>  #include <linux/slab.h>
>  #include <linux/tcp.h>
> @@ -4967,6 +4969,86 @@ static int init_reset_optional(struct platform_device *pdev)
>  	return ret;
>  }
>  
> +#define EYEQ5_OLB_GP_TX_SWRST_DIS	BIT(0)		// Tx SW reset
> +#define EYEQ5_OLB_GP_TX_M_CLKE		BIT(1)		// Tx M clock enable
> +#define EYEQ5_OLB_GP_SYS_SWRST_DIS	BIT(2)		// Sys SW reset
> +#define EYEQ5_OLB_GP_SYS_M_CLKE		BIT(3)		// Sys clock enable
> +#define EYEQ5_OLB_GP_SGMII_MODE		BIT(4)		// SGMII mode
> +#define EYEQ5_OLB_GP_RGMII_DRV		GENMASK(8, 5)	// RGMII mode
> +#define EYEQ5_OLB_GP_SMA_DRV		GENMASK(12, 9)

Defines starting here:

> +#define EYEQ5_OLB_GP_RGMII_PD		BIT(13)		// RGMII pull-down
> +#define EYEQ5_OLB_GP_MDIO_PU		BIT(14)		// RGMII pull-up
> +#define EYEQ5_OLB_GP_RGMII_RX_ST	BIT(15)		// Schmitt trigger on RGMII Rx
> +#define EYEQ5_OLB_GP_RGMII_TX_ST	BIT(16)		// Schmitt trigger on RGMII Tx
> +#define EYEQ5_OLB_GP_MDIO_ST		BIT(17)
> +#define EYEQ5_OLB_GP_MDC_ST		BIT(18)
> +#define EYEQ5_OLB_GP_MBIST_ENABLE	BIT(19)

ending here are unused.

> +
> +#define EYEQ5_OLB_SGMII_PWR_EN		BIT(0)
> +#define EYEQ5_OLB_SGMII_RST_DIS		BIT(1)
> +#define EYEQ5_OLB_SGMII_PLL_EN		BIT(2)
> +#define EYEQ5_OLB_SGMII_SIG_DET_SW	BIT(3)
> +#define EYEQ5_OLB_SGMII_PWR_STATE_MASK	GENMASK(8, 4)

Unused


> +#define EYEQ5_OLB_SGMII_PWR_STATE	BIT(4)
> +#define EYEQ5_OLB_SGMII_TX_ELECT_IDLE	BIT(9)		// Tx elect idle

Unused

> +#define EYEQ5_OLB_SGMII_POWER_ACK	BIT(16)

Unused

> +#define EYEQ5_OLB_SGMII_PLL_ACK		BIT(18)
> +#define EYEQ5_OLB_SGMII_SIG_DET		BIT(19)
> +#define EYEQ5_OLB_SGMII_PWR_STATE_ACK	GENMASK(24, 20)

Unused.

> +
> +static int eyeq5_init(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct net_device *netdev = platform_get_drvdata(pdev);
> +	struct macb *bp = netdev_priv(netdev);
> +	struct device_node *np = dev->of_node;
> +	unsigned int gp, sgmii;
> +	struct regmap *regmap;
> +	unsigned int args[2];
> +	unsigned int reg;
> +	int ret;
> +
> +	regmap = syscon_regmap_lookup_by_phandle_args(np, "mobileye,olb", 2, args);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
> +	gp = args[0];
> +	sgmii = args[1];
> +
> +	/* Forced reset */
> +	regmap_write(regmap, gp, 0);
> +	regmap_write(regmap, sgmii, 0);
> +	usleep_range(5, 20);
> +
> +	if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) {
> +		regmap_write(regmap, gp, EYEQ5_OLB_GP_SGMII_MODE);
> +
> +		reg = EYEQ5_OLB_SGMII_PWR_EN | EYEQ5_OLB_SGMII_RST_DIS |
> +		      EYEQ5_OLB_SGMII_PLL_EN;
> +		regmap_write(regmap, sgmii, reg);
> +
> +		ret = regmap_read_poll_timeout(regmap, sgmii, reg,
> +					       reg & EYEQ5_OLB_SGMII_PLL_ACK,
> +					       1, 100);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "PLL timeout");
> +
> +		regmap_read(regmap, sgmii, &reg);
> +		reg |= EYEQ5_OLB_SGMII_PWR_STATE | EYEQ5_OLB_SGMII_SIG_DET_SW;
> +		regmap_write(regmap, sgmii, reg);

You can use regmap_update_bits() here.

> +	}
> +
> +	regmap_read(regmap, gp, &reg);
> +	reg &= ~EYEQ5_OLB_GP_RGMII_DRV;
> +	if (phy_interface_mode_is_rgmii(bp->phy_interface))
> +		reg |= FIELD_PREP(EYEQ5_OLB_GP_RGMII_DRV, 0x9);
> +	reg |= EYEQ5_OLB_GP_TX_SWRST_DIS | EYEQ5_OLB_GP_TX_M_CLKE;
> +	reg |= EYEQ5_OLB_GP_SYS_SWRST_DIS | EYEQ5_OLB_GP_SYS_M_CLKE;
> +	regmap_write(regmap, gp, reg);

To me it looks like this code could be abstracted as a phy driver. E.g.,
check the init_reset_optional() and its usage on "cdns,zynqmp-gem" (phy
driver here: drivers/phy/xilinx/phy-zynqmp.c).


> +
> +	return macb_init(pdev);
> +}
> +
>  static const struct macb_usrio_config sama7g5_usrio = {
>  	.mii = 0,
>  	.rmii = 1,
> @@ -5135,6 +5217,18 @@ static const struct macb_config versal_config = {
>  	.usrio = &macb_default_usrio,
>  };
>  
> +static const struct macb_config eyeq5_config = {
> +	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO |
> +		MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_QUEUE_DISABLE |
> +		MACB_CAPS_NO_LSO,
> +	.hw_ip_align = 0,
> +	.dma_burst_length = 16,
> +	.clk_init = macb_clk_init,
> +	.init = eyeq5_init,
> +	.jumbo_max_len = 10240,
> +	.usrio = &macb_default_usrio,
> +};
> +
>  static const struct of_device_id macb_dt_ids[] = {
>  	{ .compatible = "cdns,at91sam9260-macb", .data = &at91sam9260_config },
>  	{ .compatible = "cdns,macb" },
> @@ -5152,6 +5246,7 @@ static const struct of_device_id macb_dt_ids[] = {
>  	{ .compatible = "cdns,zynqmp-gem", .data = &zynqmp_config}, /* deprecated */
>  	{ .compatible = "cdns,zynq-gem", .data = &zynq_config }, /* deprecated */
>  	{ .compatible = "sifive,fu540-c000-gem", .data = &fu540_c000_config },
> +	{ .compatible = "mobileye,eyeq5-gem", .data = &eyeq5_config },

Maybe move it after microchip to have it a bit sorted.

Thank you,
Claudiu

>  	{ .compatible = "microchip,mpfs-macb", .data = &mpfs_config },
>  	{ .compatible = "microchip,sama7g5-gem", .data = &sama7g5_gem_config },
>  	{ .compatible = "microchip,sama7g5-emac", .data = &sama7g5_emac_config },
> 

Re: [PATCH net-next 10/13] net: macb: Add "mobileye,eyeq5-gem" compatible

[Théo Lebrun]

On Mon Mar 24, 2025 at 9:18 AM CET, Claudiu Beznea wrote:
> On 21.03.2025 21:09, Théo Lebrun wrote:
>> Add support for the two GEM instances inside Mobileye EyeQ5 SoCs, using
>> compatible "mobileye,eyeq5-gem". With it, add a custom init sequence
>> that accesses two system-controller registers.
>> 
>> Noteworthy: NET_IP_ALIGN=2 on MIPS but the hardware does not align and
>> low bits aren't configurable, so we cannot respect the requested IP
>> header alignment.
>> 
>> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
>> ---
>>  drivers/net/ethernet/cadence/macb_main.c | 95 ++++++++++++++++++++++++++++++++
>>  1 file changed, 95 insertions(+)
>> 
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
>> index 79161d559166478f85a6f8294d488ed961d9be7f..9f2a5bf9a5ebca5941229bd96091a0fb96f0607d 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c

[...]

>> +static int eyeq5_init(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct net_device *netdev = platform_get_drvdata(pdev);
>> +	struct macb *bp = netdev_priv(netdev);
>> +	struct device_node *np = dev->of_node;
>> +	unsigned int gp, sgmii;
>> +	struct regmap *regmap;
>> +	unsigned int args[2];
>> +	unsigned int reg;
>> +	int ret;
>> +
>> +	regmap = syscon_regmap_lookup_by_phandle_args(np, "mobileye,olb", 2, args);
>> +	if (IS_ERR(regmap))
>> +		return PTR_ERR(regmap);
>> +
>> +	gp = args[0];
>> +	sgmii = args[1];
>> +
>> +	/* Forced reset */
>> +	regmap_write(regmap, gp, 0);
>> +	regmap_write(regmap, sgmii, 0);
>> +	usleep_range(5, 20);
>> +
>> +	if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) {
>> +		regmap_write(regmap, gp, EYEQ5_OLB_GP_SGMII_MODE);
>> +
>> +		reg = EYEQ5_OLB_SGMII_PWR_EN | EYEQ5_OLB_SGMII_RST_DIS |
>> +		      EYEQ5_OLB_SGMII_PLL_EN;
>> +		regmap_write(regmap, sgmii, reg);
>> +
>> +		ret = regmap_read_poll_timeout(regmap, sgmii, reg,
>> +					       reg & EYEQ5_OLB_SGMII_PLL_ACK,
>> +					       1, 100);
>> +		if (ret)
>> +			return dev_err_probe(dev, ret, "PLL timeout");
>> +
>> +		regmap_read(regmap, sgmii, &reg);
>> +		reg |= EYEQ5_OLB_SGMII_PWR_STATE | EYEQ5_OLB_SGMII_SIG_DET_SW;
>> +		regmap_write(regmap, sgmii, reg);
>
> You can use regmap_update_bits() here.
>
>> +	}
>> +
>> +	regmap_read(regmap, gp, &reg);
>> +	reg &= ~EYEQ5_OLB_GP_RGMII_DRV;
>> +	if (phy_interface_mode_is_rgmii(bp->phy_interface))
>> +		reg |= FIELD_PREP(EYEQ5_OLB_GP_RGMII_DRV, 0x9);
>> +	reg |= EYEQ5_OLB_GP_TX_SWRST_DIS | EYEQ5_OLB_GP_TX_M_CLKE;
>> +	reg |= EYEQ5_OLB_GP_SYS_SWRST_DIS | EYEQ5_OLB_GP_SYS_M_CLKE;
>> +	regmap_write(regmap, gp, reg);
>
> To me it looks like this code could be abstracted as a phy driver. E.g.,
> check the init_reset_optional() and its usage on "cdns,zynqmp-gem" (phy
> driver here: drivers/phy/xilinx/phy-zynqmp.c).

I thought about that question. Options to implement that sequence are:

 - (1) Implement a separate PHY driver, what you are proposing. I just
   made a prototype branch to see what it'd look like. Nothing too
   surprising; mostly the above sequence is copy-pasted inside
   phy_init|power_on(). I see two issues:

    - First, a practical one. This adds a lot of boilerplate for no
      obvious benefit compared to a raw registers read/write sequence
      inside macb_config->init().

      The main reason for that boilerplate is to allow reuse of a PHY
      across MACs; here we already know that cannot be useful because
      the EyeQ5 has two GEMs and nothing else. Those registers are
      EyeQ5-specific.

    - Second, a semantic one. The registers we are touching are *not*
      the PHY's registers. They are configuring the PHY's integration:
      its input PLL, resets, etc.

 - (2) Second, taking into account that what we are configuring isn't
   the PHY itself but its resources, we could try modeling each
   individual register+field as a reset / clock / pin control (there is
   some drive strength in here, *I think*). Issue: this would get
   messy, fast.
    - A single register would expose many resources.
    - The sequence in macb_config->init() would need to be the exact
      same order. IE we can't abstract much.

   Something like this pseudocode (which is a bad idea, we'd all agree
   here):

      reset_deassert(bp->eq5_sgmii_reset);
      reset_deassert(bp->eq5_sgmii_reset_pwr);
      reset_deassert(bp->eq5_phy_reset_tx);
      reset_deassert(bp->eq5_phy_reset_sys);

      if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) {
         pinctrl_select_state(bp->eq5_phy_input_pinctrl, bp->eq5_pins_sgmii);

         reset_deassert(bp->eq5_sgmii_reset);
         clk_prepare_enable(bp->eq5_sgmii_phy_input_pll);

         reset_deassert(bp->eq5_sgmii_reset_pwr);
      } else {
         pinctrl_select_state(bp->eq5_pinctrl, bp->eq5_pins_rgmii);
      }

      reset_deassert(bp->eq5_phy_reset_tx);
      reset_deassert(bp->eq5_phy_reset_sys);
      clk_prepare_enable(bp->eq5_phy_mclk_tx);
      clk_prepare_enable(bp->eq5_phy_mclk_sys);

 - (3) Keep the sequence in macb_config->init(). Plain and simple.
    - Issue: it is somewhat unrelated platform-specific code that's
      present inside macb_main.c.

The two serious options are (1) and (3).
(1) is what you proposed and (3) is what's in the series.

>>  static const struct of_device_id macb_dt_ids[] = {
>>  	{ .compatible = "cdns,at91sam9260-macb", .data = &at91sam9260_config },
>>  	{ .compatible = "cdns,macb" },
>> @@ -5152,6 +5246,7 @@ static const struct of_device_id macb_dt_ids[] = {
>>  	{ .compatible = "cdns,zynqmp-gem", .data = &zynqmp_config}, /* deprecated */
>>  	{ .compatible = "cdns,zynq-gem", .data = &zynq_config }, /* deprecated */
>>  	{ .compatible = "sifive,fu540-c000-gem", .data = &fu540_c000_config },
>> +	{ .compatible = "mobileye,eyeq5-gem", .data = &eyeq5_config },
>
> Maybe move it after microchip to have it a bit sorted.

Argh those semi sorted lists. I saw "cdns" then "atmel" then "cdns" so I
ignored sorting.

Thanks for the review!

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next 10/13] net: macb: Add "mobileye,eyeq5-gem" compatible

[Claudiu Beznea]

Hi, Theo,

On 25.03.2025 19:25, Théo Lebrun wrote:
>>> +	}
>>> +
>>> +	regmap_read(regmap, gp, &reg);
>>> +	reg &= ~EYEQ5_OLB_GP_RGMII_DRV;
>>> +	if (phy_interface_mode_is_rgmii(bp->phy_interface))
>>> +		reg |= FIELD_PREP(EYEQ5_OLB_GP_RGMII_DRV, 0x9);
>>> +	reg |= EYEQ5_OLB_GP_TX_SWRST_DIS | EYEQ5_OLB_GP_TX_M_CLKE;
>>> +	reg |= EYEQ5_OLB_GP_SYS_SWRST_DIS | EYEQ5_OLB_GP_SYS_M_CLKE;
>>> +	regmap_write(regmap, gp, reg);
>> To me it looks like this code could be abstracted as a phy driver. E.g.,
>> check the init_reset_optional() and its usage on "cdns,zynqmp-gem" (phy
>> driver here: drivers/phy/xilinx/phy-zynqmp.c).
> I thought about that question. Options to implement that sequence are:
> 
>  - (1) Implement a separate PHY driver, what you are proposing. I just
>    made a prototype branch to see what it'd look like. Nothing too
>    surprising; mostly the above sequence is copy-pasted inside
>    phy_init|power_on(). I see two issues:
> 
>     - First, a practical one. This adds a lot of boilerplate for no
>       obvious benefit compared to a raw registers read/write sequence
>       inside macb_config->init().

The macb is used by various platforms. If the settings proposed in this
patch (platform specific AFAICT) could be abstracted and used with generic
APIs I think would be better this way.

> 
>       The main reason for that boilerplate is to allow reuse of a PHY
>       across MACs;

And/or avoid having platform specific code in the macb driver.

> here we already know that cannot be useful because
>       the EyeQ5 has two GEMs and nothing else. Those registers are
>       EyeQ5-specific.
> 
>     - Second, a semantic one. The registers we are touching are *not*
>       the PHY's registers. They are configuring the PHY's integration:
>       its input PLL, resets, etc.
> 
>  - (2) Second, taking into account that what we are configuring isn't
>    the PHY itself but its resources, we could try modeling each
>    individual register+field as a reset / clock / pin control (there is
>    some drive strength in here, *I think*). Issue: this would get
>    messy, fast.
>     - A single register would expose many resources.
>     - The sequence in macb_config->init() would need to be the exact
>       same order. IE we can't abstract much.
> 
>    Something like this pseudocode (which is a bad idea, we'd all agree
>    here):
> 
>       reset_deassert(bp->eq5_sgmii_reset);
>       reset_deassert(bp->eq5_sgmii_reset_pwr);
>       reset_deassert(bp->eq5_phy_reset_tx);
>       reset_deassert(bp->eq5_phy_reset_sys);
> 
>       if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) {
>          pinctrl_select_state(bp->eq5_phy_input_pinctrl, bp->eq5_pins_sgmii);
> 
>          reset_deassert(bp->eq5_sgmii_reset);
>          clk_prepare_enable(bp->eq5_sgmii_phy_input_pll);
> 
>          reset_deassert(bp->eq5_sgmii_reset_pwr);
>       } else {
>          pinctrl_select_state(bp->eq5_pinctrl, bp->eq5_pins_rgmii);
>       }
> 
>       reset_deassert(bp->eq5_phy_reset_tx);
>       reset_deassert(bp->eq5_phy_reset_sys);
>       clk_prepare_enable(bp->eq5_phy_mclk_tx);
>       clk_prepare_enable(bp->eq5_phy_mclk_sys);

This looks complicated to me.

> 
>  - (3) Keep the sequence in macb_config->init(). Plain and simple.
>     - Issue: it is somewhat unrelated platform-specific code that's
>       present inside macb_main.c.

For maintainability I would prefer to avoid this.

> 
> The two serious options are (1) and (3).
> (1) is what you proposed and (3) is what's in the series.

I prefer (1) if it can be done.

Thank you,
Claudiu

[PATCH net-next 11/13] MIPS: mobileye: add EyeQ5 DMA IOCU support

[Théo Lebrun]

Both Cadence GEM Ethernet controllers on EyeQ5 are hardwired through CM3
IO Coherency Units (IOCU). For DMA coherent accesses, BIT(36) must be
set in DMA addresses.

Implement that in platform-specific dma_map_ops which get attached to
both instances of `cdns,eyeq5-gem` through a notifier block.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 MAINTAINERS                         |   2 +-
 arch/mips/mobileye/Kconfig          |   1 +
 arch/mips/mobileye/Makefile         |   2 +
 arch/mips/mobileye/eyeq5-iocu-dma.c | 160 ++++++++++++++++++++++++++++++++++++
 4 files changed, 164 insertions(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 5959513a7359f46e10d91bafd5a736b8dceeb7c5..a943f88fc8d4ffe502479e3cd5de6d1a2b1e1fc0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15975,7 +15975,7 @@ F:	Documentation/devicetree/bindings/mips/mobileye.yaml
 F:	Documentation/devicetree/bindings/soc/mobileye/
 F:	arch/mips/boot/dts/mobileye/
 F:	arch/mips/configs/eyeq5_defconfig
-F:	arch/mips/mobileye/board-epm5.its.S
+F:	arch/mips/mobileye/
 F:	drivers/clk/clk-eyeq.c
 F:	drivers/pinctrl/pinctrl-eyeq5.c
 F:	drivers/reset/reset-eyeq.c
diff --git a/arch/mips/mobileye/Kconfig b/arch/mips/mobileye/Kconfig
index f9abb2d6e1787dbc5a173db48606ed5a02088e41..b9040f3a9b3ddc7f5addcd8e5f110cb9c775b6b1 100644
--- a/arch/mips/mobileye/Kconfig
+++ b/arch/mips/mobileye/Kconfig
@@ -9,6 +9,7 @@ choice
 
 	config MACH_EYEQ5
 		bool "Mobileye EyeQ5 SoC"
+		select ARCH_HAS_DMA_OPS
 
 	config MACH_EYEQ6H
 		bool "Mobileye EyeQ6H SoC"
diff --git a/arch/mips/mobileye/Makefile b/arch/mips/mobileye/Makefile
index 315c06b689cfbb83f9f205d1140ecf5058e2aa02..50fc7d0ae167c3fb3dc8585bcd45583c6cc3f2d2 100644
--- a/arch/mips/mobileye/Makefile
+++ b/arch/mips/mobileye/Makefile
@@ -1 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0-or-later
+
+obj-$(CONFIG_MACH_EYEQ5)               += eyeq5-iocu-dma.o
diff --git a/arch/mips/mobileye/eyeq5-iocu-dma.c b/arch/mips/mobileye/eyeq5-iocu-dma.c
new file mode 100644
index 0000000000000000000000000000000000000000..71d1c35f911636db141c4467dccc405af69835ec
--- /dev/null
+++ b/arch/mips/mobileye/eyeq5-iocu-dma.c
@@ -0,0 +1,160 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bits.h>
+#include <linux/device.h>
+#include <linux/device/bus.h>
+#include <linux/dma-direct.h>
+#include <linux/dma-direction.h>
+#include <linux/dma-map-ops.h>
+#include <linux/dma-mapping.h>
+#include <linux/errno.h>
+#include <linux/export.h>
+#include <linux/gfp_types.h>
+#include <linux/init.h>
+#include <linux/mm.h>
+#include <linux/mm_types.h>
+#include <linux/notifier.h>
+#include <linux/pfn.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/scatterlist.h>
+#include <linux/types.h>
+
+static void *eyeq5_iocu_alloc(struct device *dev, size_t size,
+			      dma_addr_t *dma_handle, gfp_t gfp,
+			      unsigned long attrs)
+{
+	void *p = dma_direct_alloc(dev, size, dma_handle, gfp, attrs);
+
+	*dma_handle |= BIT_ULL(36);
+	return p;
+}
+
+static void eyeq5_iocu_free(struct device *dev, size_t size,
+			    void *vaddr, dma_addr_t dma_handle,
+			    unsigned long attrs)
+{
+	dma_handle &= ~BIT_ULL(36);
+	dma_direct_free(dev, size, vaddr, dma_handle, attrs);
+}
+
+static int eyeq5_iocu_mmap(struct device *dev, struct vm_area_struct *vma,
+			   void *cpu_addr, dma_addr_t dma_addr, size_t size,
+			   unsigned long attrs)
+{
+	unsigned long pfn = PHYS_PFN(dma_to_phys(dev, dma_addr));
+	unsigned long count = PAGE_ALIGN(size) >> PAGE_SHIFT;
+	unsigned long user_count = vma_pages(vma);
+	int ret;
+
+	vma->vm_page_prot = dma_pgprot(dev, vma->vm_page_prot, attrs);
+
+	if (dma_mmap_from_dev_coherent(dev, vma, cpu_addr, size, &ret))
+		return ret;
+
+	if (vma->vm_pgoff >= count || user_count > count - vma->vm_pgoff)
+		return -ENXIO;
+
+	return remap_pfn_range(vma, vma->vm_start, pfn + vma->vm_pgoff,
+			       user_count << PAGE_SHIFT, vma->vm_page_prot);
+}
+
+static int eyeq5_iocu_get_sgtable(struct device *dev, struct sg_table *sgt,
+				  void *cpu_addr, dma_addr_t dma_addr, size_t size,
+				  unsigned long attrs)
+{
+	struct page *page = virt_to_page(cpu_addr);
+	int ret;
+
+	ret = sg_alloc_table(sgt, 1, GFP_KERNEL);
+	if (!ret)
+		sg_set_page(sgt->sgl, page, PAGE_ALIGN(size), 0);
+	return ret;
+}
+
+static dma_addr_t eyeq5_iocu_map_page(struct device *dev, struct page *page,
+				      unsigned long offset, size_t size,
+				      enum dma_data_direction dir,
+				      unsigned long attrs)
+{
+	phys_addr_t phys = page_to_phys(page) + offset;
+
+	/* BIT(36) toggles routing through IOCU for DMA operations. */
+	return phys_to_dma(dev, phys) | BIT_ULL(36);
+}
+
+static void eyeq5_iocu_unmap_page(struct device *dev, dma_addr_t dma_handle,
+				  size_t size, enum dma_data_direction dir,
+		unsigned long attrs)
+{
+}
+
+static int eyeq5_iocu_map_sg(struct device *dev, struct scatterlist *sgl,
+			     int nents, enum dma_data_direction dir,
+			     unsigned long attrs)
+{
+	struct scatterlist *sg;
+	int i;
+
+	for_each_sg(sgl, sg, nents, i) {
+		sg->dma_address = eyeq5_iocu_map_page(dev, sg_page(sg),
+						      sg->offset, sg->length,
+						      dir, attrs);
+		if (sg->dma_address == DMA_MAPPING_ERROR)
+			return 0; /* No cleanup because ->unmap_page() is a no-op. */
+		sg_dma_len(sg) = sg->length;
+	}
+
+	return nents;
+}
+
+static void eyeq5_iocu_unmap_sg(struct device *dev, struct scatterlist *sgl,
+				int nents, enum dma_data_direction dir,
+				unsigned long attrs)
+{
+	/* We know page ->unmap_page() is a no-op. */
+}
+
+const struct dma_map_ops eyeq5_iocu_ops = {
+	.alloc			= eyeq5_iocu_alloc,
+	.free			= eyeq5_iocu_free,
+	.alloc_pages_op		= dma_direct_alloc_pages,
+	.free_pages		= dma_direct_free_pages,
+	.mmap			= eyeq5_iocu_mmap,
+	.get_sgtable		= eyeq5_iocu_get_sgtable,
+	.map_page		= eyeq5_iocu_map_page,
+	.unmap_page		= eyeq5_iocu_unmap_page,
+	.map_sg			= eyeq5_iocu_map_sg,
+	.unmap_sg		= eyeq5_iocu_unmap_sg,
+	.get_required_mask	= dma_direct_get_required_mask,
+};
+EXPORT_SYMBOL(eyeq5_iocu_ops);
+
+static int eyeq5_iocu_notifier(struct notifier_block *nb,
+			       unsigned long event,
+			       void *data)
+{
+	struct device *dev = data;
+
+	/*
+	 * IOCU routing is hardwired; we must use our above custom
+	 * routines for cache-coherent DMA on ethernet interfaces.
+	 */
+	if (event == BUS_NOTIFY_ADD_DEVICE &&
+	    device_is_compatible(dev, "mobileye,eyeq5-gem")) {
+		set_dma_ops(dev, &eyeq5_iocu_ops);
+		return NOTIFY_OK;
+	}
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block eyeq5_iocu_nb = {
+	.notifier_call = eyeq5_iocu_notifier,
+};
+
+static int __init eyeq5_iocu_init(void)
+{
+	return bus_register_notifier(&platform_bus_type, &eyeq5_iocu_nb);
+}
+postcore_initcall(eyeq5_iocu_init);

-- 
2.48.1

[PATCH net-next 12/13] MIPS: mobileye: eyeq5: add two Cadence GEM Ethernet controllers

[Théo Lebrun]

Add both MACB/GEM instances found in the Mobileye EyeQ5 SoC.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 arch/mips/boot/dts/mobileye/eyeq5.dtsi | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/arch/mips/boot/dts/mobileye/eyeq5.dtsi b/arch/mips/boot/dts/mobileye/eyeq5.dtsi
index a84e6e720619ef99e1405ae6296d8bad1aa3fa23..420cb27607bfdd8d5ea510fb668b0a1c85dd7d83 100644
--- a/arch/mips/boot/dts/mobileye/eyeq5.dtsi
+++ b/arch/mips/boot/dts/mobileye/eyeq5.dtsi
@@ -77,6 +77,8 @@ aliases {
 		serial0 = &uart0;
 		serial1 = &uart1;
 		serial2 = &uart2;
+		ethernet0 = &macb0;
+		ethernet1 = &macb1;
 	};
 
 	cpu_intc: interrupt-controller {
@@ -178,6 +180,38 @@ timer {
 				clocks = <&olb EQ5C_CPU_CORE0>;
 			};
 		};
+
+		macb0: ethernet@2a00000 {
+			compatible = "mobileye,eyeq5-gem";
+			reg = <0x0 0x02a00000 0x0 0x4000>;
+			interrupt-parent = <&gic>;
+			interrupts = <GIC_SHARED 23 IRQ_TYPE_LEVEL_HIGH>, /* queue0 */
+				     <GIC_SHARED 23 IRQ_TYPE_LEVEL_HIGH>, /* queue1 */
+				     <GIC_SHARED 23 IRQ_TYPE_LEVEL_HIGH>, /* queue2 */
+				     <GIC_SHARED 23 IRQ_TYPE_LEVEL_HIGH>; /* queue3 */
+			clock-names = "pclk", "hclk", "tsu_clk";
+			clocks = <&pclk>, <&pclk>, <&tsu_clk>;
+			dma-coherent;
+			nvmem-cells = <&eth0_mac>;
+			nvmem-cell-names = "mac-address";
+			mobileye,olb = <&olb 0x128 0x134>;
+		};
+
+		macb1: ethernet@2b00000 {
+			compatible = "mobileye,eyeq5-gem";
+			reg = <0x0 0x02b00000 0x0 0x4000>;
+			interrupt-parent = <&gic>;
+			interrupts = <GIC_SHARED 24 IRQ_TYPE_LEVEL_HIGH>, /* queue0 */
+				     <GIC_SHARED 24 IRQ_TYPE_LEVEL_HIGH>, /* queue1 */
+				     <GIC_SHARED 24 IRQ_TYPE_LEVEL_HIGH>, /* queue2 */
+				     <GIC_SHARED 24 IRQ_TYPE_LEVEL_HIGH>; /* queue3 */
+			clock-names = "pclk", "hclk", "tsu_clk";
+			clocks = <&pclk>, <&pclk>, <&tsu_clk>;
+			dma-coherent;
+			nvmem-cells = <&eth1_mac>;
+			nvmem-cell-names = "mac-address";
+			mobileye,olb = <&olb 0x12c 0x138>;
+		};
 	};
 };
 

-- 
2.48.1

[PATCH net-next 13/13] MIPS: mobileye: eyeq5-epm: add two Cadence GEM Ethernet PHYs

[Théo Lebrun]

The Mobileye EyeQ5 eval board (EPM) embeds two MDIO PHYs.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 arch/mips/boot/dts/mobileye/eyeq5-epm5.dts | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/arch/mips/boot/dts/mobileye/eyeq5-epm5.dts b/arch/mips/boot/dts/mobileye/eyeq5-epm5.dts
index 6898b2d8267dfadeea511a84d1df3f70744f17bb..20dfc85681bb03330981ca0f11b2edfff3fa57bc 100644
--- a/arch/mips/boot/dts/mobileye/eyeq5-epm5.dts
+++ b/arch/mips/boot/dts/mobileye/eyeq5-epm5.dts
@@ -21,3 +21,29 @@ memory@0 {
 		      <0x8 0x02000000 0x0 0x7E000000>;
 	};
 };
+
+&macb0 {
+	phy-mode = "sgmii";
+	phy-handle = <&macb0_phy>;
+
+	mdio {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		macb0_phy: ethernet-phy@e {
+			reg = <0xE>;
+		};
+	};
+};
+
+&macb1 {
+	phy-mode = "rgmii-id";
+	phy-handle = <&macb1_phy>;
+
+	mdio {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		macb1_phy: ethernet-phy@e {
+			reg = <0xE>;
+		};
+	};
+};

-- 
2.48.1

Re: [PATCH net-next 13/13] MIPS: mobileye: eyeq5-epm: add two Cadence GEM Ethernet PHYs

[Andrew Lunn]

On Fri, Mar 21, 2025 at 08:09:44PM +0100, Théo Lebrun wrote:
> The Mobileye EyeQ5 eval board (EPM) embeds two MDIO PHYs.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  arch/mips/boot/dts/mobileye/eyeq5-epm5.dts | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/arch/mips/boot/dts/mobileye/eyeq5-epm5.dts b/arch/mips/boot/dts/mobileye/eyeq5-epm5.dts
> index 6898b2d8267dfadeea511a84d1df3f70744f17bb..20dfc85681bb03330981ca0f11b2edfff3fa57bc 100644
> --- a/arch/mips/boot/dts/mobileye/eyeq5-epm5.dts
> +++ b/arch/mips/boot/dts/mobileye/eyeq5-epm5.dts
> @@ -21,3 +21,29 @@ memory@0 {
>  		      <0x8 0x02000000 0x0 0x7E000000>;
>  	};
>  };
> +
> +&macb0 {
> +	phy-mode = "sgmii";
> +	phy-handle = <&macb0_phy>;
> +
> +	mdio {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		macb0_phy: ethernet-phy@e {
> +			reg = <0xE>;

The DT coding style might say this should be lower case?

> +		};
> +	};
> +};
> +
> +&macb1 {
> +	phy-mode = "rgmii-id";

Hurrah.  A correct rgmii phy-mode.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew