[PATCH v1 3/3] dt-bindings: net: sun8i-emac: Add AC300 EMAC1 nvmem phy selection

[PATCH v1 3/3] dt-bindings: net: sun8i-emac: Add AC300 EMAC1 nvmem phy selection

[James Hilliard]

The Allwinner H616 EMAC1 can be connected to an on-die AC200 or AC300
PHY depending upon the silicon variant.

Add a new allwinner,sun50i-h616-emac1 compatible and example, support
for the allwinner,sun50i-h616-emac1 will be added later on.

Add nvmem-cells and nvmem-cell-names properties for the ac300 efuse
based phy selection.

Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 .../net/allwinner,sun8i-a83t-emac.yaml        | 42 +++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml b/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml
index 7fe0352dff0f..b6bf1718dba1 100644
--- a/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml
+++ b/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml
@@ -18,6 +18,7 @@ properties:
       - const: allwinner,sun8i-r40-gmac
       - const: allwinner,sun8i-v3s-emac
       - const: allwinner,sun50i-a64-emac
+      - const: allwinner,sun50i-h616-emac1
       - items:
           - enum:
               - allwinner,sun20i-d1-emac
@@ -28,6 +29,14 @@ properties:
   reg:
     maxItems: 1
 
+  nvmem-cells:
+    maxItems: 1
+    description: NVMEM cell with the ac300 efuse.
+
+  nvmem-cell-names:
+    items:
+      - const: ac300
+
   interrupts:
     maxItems: 1
 
@@ -321,4 +330,37 @@ examples:
         };
     };
 
+  - |
+    ethernet@5030000 {
+        compatible = "allwinner,sun50i-h616-emac1";
+        reg = <0x05030000 0x10000>;
+        interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
+        interrupt-names = "macirq";
+        resets = <&ccu RST_BUS_EMAC1>;
+        reset-names = "stmmaceth";
+        clocks = <&ccu CLK_BUS_EMAC1>;
+        clock-names = "stmmaceth";
+        phys = <&ac200_rmii_phy>, <&ac300_rmii_phy>;
+        phy-names = "ac200", "ac300";
+        phy-mode = "rgmii";
+        nvmem-cells = <&ephy_acx00>;
+        nvmem-cell-names = "ac300";
+
+        mdio {
+            compatible = "snps,dwmac-mdio";
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            ac300_rmii_phy: ac300-ethernet-phy@0 {
+              compatible = "ethernet-phy-ieee802.3-c22";
+              reg = <0>;
+            };
+
+            ac200_rmii_phy: ac200-ethernet-phy@1 {
+              compatible = "ethernet-phy-ieee802.3-c22";
+              reg = <1>;
+            };
+        };
+    };
+
 ...
-- 
2.34.1

Re: [PATCH v1 3/3] dt-bindings: net: sun8i-emac: Add AC300 EMAC1 nvmem phy selection

[Rob Herring (Arm)]

On Mon, 26 May 2025 12:29:36 -0600, James Hilliard wrote:
> The Allwinner H616 EMAC1 can be connected to an on-die AC200 or AC300
> PHY depending upon the silicon variant.
> 
> Add a new allwinner,sun50i-h616-emac1 compatible and example, support
> for the allwinner,sun50i-h616-emac1 will be added later on.
> 
> Add nvmem-cells and nvmem-cell-names properties for the ac300 efuse
> based phy selection.
> 
> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> ---
>  .../net/allwinner,sun8i-a83t-emac.yaml        | 42 +++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 

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

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.example.dts:219.27-28 syntax error
FATAL ERROR: Unable to parse input tree
make[2]: *** [scripts/Makefile.dtbs:131: Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.example.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1519: dt_binding_check] Error 2
make: *** [Makefile:248: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250526182939.2593553-3-james.hilliard1@gmail.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 v1 3/3] dt-bindings: net: sun8i-emac: Add AC300 EMAC1 nvmem phy selection

[Andrew Lunn]

> +        phy-mode = "rgmii";

Does the PCB have extra long clock lines?

https://elixir.bootlin.com/linux/v6.15/source/Documentation/devicetree/bindings/net/ethernet-controller.yaml#L287

	Andrew

Re: [PATCH v1 3/3] dt-bindings: net: sun8i-emac: Add AC300 EMAC1 nvmem phy selection

[James Hilliard]

On Mon, May 26, 2025 at 1:36 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > +        phy-mode = "rgmii";
>
> Does the PCB have extra long clock lines?

I'm not sure, it's a copackaged(maybe on-die is the wrong terminology)
PHY I think so I assume the clock lines are internal, in the device specific
dts we set something like this on the emac1 node:
allwinner,rx-delay-ps = <3100>;
allwinner,tx-delay-ps = <700>;

There's some more info here on the AC300:
https://lore.kernel.org/lkml/20200416185758.1388148-1-jernej.skrabec@siol.net/

>
> https://elixir.bootlin.com/linux/v6.15/source/Documentation/devicetree/bindings/net/ethernet-controller.yaml#L287
>
>         Andrew

Re: [PATCH v1 3/3] dt-bindings: net: sun8i-emac: Add AC300 EMAC1 nvmem phy selection

[Andrew Lunn]

On Mon, May 26, 2025 at 03:32:03PM -0600, James Hilliard wrote:
> On Mon, May 26, 2025 at 1:36 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > > +        phy-mode = "rgmii";
> >
> > Does the PCB have extra long clock lines?
> 
> I'm not sure, it's a copackaged(maybe on-die is the wrong terminology)
> PHY I think so I assume the clock lines are internal, in the device specific
> dts we set something like this on the emac1 node:
> allwinner,rx-delay-ps = <3100>;
> allwinner,tx-delay-ps = <700>;

Those values are just weird. The RGMII delay should be 2000ps. 3100 is
way too big, and 700 is way too small.

I think phy-mode = "internal" would be better, and just hard code the
delays either in the MAC or PHY driver.

Thanks for the link to the old thread, which was 5 years
ago. Hopefully since then, a bit more has been learnt. Quickly reading
through that thread, i don't think an MFD is not the correct solution.
In the last 5 years we have had to deal with more chicken/egg problems
with PHYs. It has now become pretty much standard practice to put the
ID values in DT, to get the driver probed when the device does not
respond on the bus. The DT node can then use phandles to the reset and
clock controller to configure them as needed, the core will probably
do that. I2C is a bit messier, you probably want a phandle pointing to
the i2c_adapter, so you can use i2c_transfer() on it in the probe()
function.

	Andrew

Re: [PATCH v1 3/3] dt-bindings: net: sun8i-emac: Add AC300 EMAC1 nvmem phy selection

[James Hilliard]

On Mon, May 26, 2025 at 4:38 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Mon, May 26, 2025 at 03:32:03PM -0600, James Hilliard wrote:
> > On Mon, May 26, 2025 at 1:36 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > > +        phy-mode = "rgmii";
> > >
> > > Does the PCB have extra long clock lines?
> >
> > I'm not sure, it's a copackaged(maybe on-die is the wrong terminology)
> > PHY I think so I assume the clock lines are internal, in the device specific
> > dts we set something like this on the emac1 node:
> > allwinner,rx-delay-ps = <3100>;
> > allwinner,tx-delay-ps = <700>;
>
> Those values are just weird. The RGMII delay should be 2000ps. 3100 is
> way too big, and 700 is way too small.

I think these may not actually be required when using the internal
EPHY's now that I think about it again.

> I think phy-mode = "internal" would be better, and just hard code the
> delays either in the MAC or PHY driver.

Hmm, would that make sense even though the MAC driver also
supports external PHY's?

> Thanks for the link to the old thread, which was 5 years
> ago. Hopefully since then, a bit more has been learnt. Quickly reading
> through that thread, i don't think an MFD is not the correct solution.

Well the current patches I've been playing around with for the AC200
phy are pretty similar to those patches still.

Here's a branch that works on both AC200/AC300 but only if I do
the PHY initialization in u-boot:
https://github.com/jameshilliard/linux-h616/commits/acx00

> In the last 5 years we have had to deal with more chicken/egg problems
> with PHYs. It has now become pretty much standard practice to put the
> ID values in DT, to get the driver probed when the device does not
> respond on the bus.

This is what I'm doing right? I mean I'm putting the phy ID in the
DT for both the AC200 and AC300. When doing the reset driver
for say the AC200 I would wire that up to only the AC200 phy
node and not the AC300 node(since the AC300 reset is MDIO
based while the AC200 is i2c based).

> The DT node can then use phandles to the reset and
> clock controller to configure them as needed, the core will probably
> do that. I2C is a bit messier, you probably want a phandle pointing to
> the i2c_adapter, so you can use i2c_transfer() on it in the probe()
> function.

Without a MFD or some other node that exposes a reset I'm a bit
confused about what driver would actually issue the reset.

Yeah, we need a phandle to the i2c_adapter, but since the resets
would be under the AC200 PHY node I assume there would need
to be some sort of intermediary driver implementing the i2c reset
right?

Re: [PATCH v1 3/3] dt-bindings: net: sun8i-emac: Add AC300 EMAC1 nvmem phy selection

[Andrew Lunn]

On Mon, May 26, 2025 at 05:22:48PM -0600, James Hilliard wrote:
> On Mon, May 26, 2025 at 4:38 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > On Mon, May 26, 2025 at 03:32:03PM -0600, James Hilliard wrote:
> > > On Mon, May 26, 2025 at 1:36 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > > >
> > > > > +        phy-mode = "rgmii";
> > > >
> > > > Does the PCB have extra long clock lines?
> > >
> > > I'm not sure, it's a copackaged(maybe on-die is the wrong terminology)
> > > PHY I think so I assume the clock lines are internal, in the device specific
> > > dts we set something like this on the emac1 node:
> > > allwinner,rx-delay-ps = <3100>;
> > > allwinner,tx-delay-ps = <700>;
> >
> > Those values are just weird. The RGMII delay should be 2000ps. 3100 is
> > way too big, and 700 is way too small.
> 
> I think these may not actually be required when using the internal
> EPHY's now that I think about it again.
> 
> > I think phy-mode = "internal" would be better, and just hard code the
> > delays either in the MAC or PHY driver.
> 
> Hmm, would that make sense even though the MAC driver also
> supports external PHY's?

If an external PHY is being used, i would not expect a phy-mode of
internal.
 
> > Thanks for the link to the old thread, which was 5 years
> > ago. Hopefully since then, a bit more has been learnt. Quickly reading
> > through that thread, i don't think an MFD is not the correct solution.
> 
> Well the current patches I've been playing around with for the AC200
> phy are pretty similar to those patches still.
> 
> Here's a branch that works on both AC200/AC300 but only if I do
> the PHY initialization in u-boot:
> https://github.com/jameshilliard/linux-h616/commits/acx00

I personally don't like depending on the bootloader. I often replace
the bootloader, because it is a crippled version that does not let me
TFTP boot, does not have flash enabled for saving configuration, and i
want to use barebox etc. I think it is much better when Linux drives
the hardware, not the bootloader.

> 
> > In the last 5 years we have had to deal with more chicken/egg problems
> > with PHYs. It has now become pretty much standard practice to put the
> > ID values in DT, to get the driver probed when the device does not
> > respond on the bus.
> 
> This is what I'm doing right? I mean I'm putting the phy ID in the
> DT for both the AC200 and AC300. When doing the reset driver
> for say the AC200 I would wire that up to only the AC200 phy
> node and not the AC300 node(since the AC300 reset is MDIO
> based while the AC200 is i2c based).
> 
> > The DT node can then use phandles to the reset and
> > clock controller to configure them as needed, the core will probably
> > do that. I2C is a bit messier, you probably want a phandle pointing to
> > the i2c_adapter, so you can use i2c_transfer() on it in the probe()
> > function.
> 
> Without a MFD or some other node that exposes a reset I'm a bit
> confused about what driver would actually issue the reset.
> 
> Yeah, we need a phandle to the i2c_adapter, but since the resets
> would be under the AC200 PHY node I assume there would need
> to be some sort of intermediary driver implementing the i2c reset
> right?

You need an reset controller, yes, but is that an MFD? Or just a reset
controller? Where does this reset controller live?

Question like this show we are missing the big picture. What are all
the different parts, how are they interconnected? Once we see that, we
can give you better advice.

	Andrew

Re: [PATCH v1 3/3] dt-bindings: net: sun8i-emac: Add AC300 EMAC1 nvmem phy selection

[James Hilliard]

On Mon, May 26, 2025 at 5:45 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Mon, May 26, 2025 at 05:22:48PM -0600, James Hilliard wrote:
> > On Mon, May 26, 2025 at 4:38 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > On Mon, May 26, 2025 at 03:32:03PM -0600, James Hilliard wrote:
> > > > On Mon, May 26, 2025 at 1:36 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > > > >
> > > > > > +        phy-mode = "rgmii";
> > > > >
> > > > > Does the PCB have extra long clock lines?
> > > >
> > > > I'm not sure, it's a copackaged(maybe on-die is the wrong terminology)
> > > > PHY I think so I assume the clock lines are internal, in the device specific
> > > > dts we set something like this on the emac1 node:
> > > > allwinner,rx-delay-ps = <3100>;
> > > > allwinner,tx-delay-ps = <700>;
> > >
> > > Those values are just weird. The RGMII delay should be 2000ps. 3100 is
> > > way too big, and 700 is way too small.
> >
> > I think these may not actually be required when using the internal
> > EPHY's now that I think about it again.
> >
> > > I think phy-mode = "internal" would be better, and just hard code the
> > > delays either in the MAC or PHY driver.
> >
> > Hmm, would that make sense even though the MAC driver also
> > supports external PHY's?
>
> If an external PHY is being used, i would not expect a phy-mode of
> internal.

Ok, I guess this is a bit of a separate issue vs phy selection right?

> > > Thanks for the link to the old thread, which was 5 years
> > > ago. Hopefully since then, a bit more has been learnt. Quickly reading
> > > through that thread, i don't think an MFD is not the correct solution.
> >
> > Well the current patches I've been playing around with for the AC200
> > phy are pretty similar to those patches still.
> >
> > Here's a branch that works on both AC200/AC300 but only if I do
> > the PHY initialization in u-boot:
> > https://github.com/jameshilliard/linux-h616/commits/acx00
>
> I personally don't like depending on the bootloader. I often replace
> the bootloader, because it is a crippled version that does not let me
> TFTP boot, does not have flash enabled for saving configuration, and i
> want to use barebox etc. I think it is much better when Linux drives
> the hardware, not the bootloader.

Yeah, I'm just using that for verifying the PHY selection logic at the
moment is functional and that Linux can handle the PHY's once in
an initialized state. The initialization sequence I'm using in u-boot
is pretty far from being suitable for upstream as well.

> > > In the last 5 years we have had to deal with more chicken/egg problems
> > > with PHYs. It has now become pretty much standard practice to put the
> > > ID values in DT, to get the driver probed when the device does not
> > > respond on the bus.
> >
> > This is what I'm doing right? I mean I'm putting the phy ID in the
> > DT for both the AC200 and AC300. When doing the reset driver
> > for say the AC200 I would wire that up to only the AC200 phy
> > node and not the AC300 node(since the AC300 reset is MDIO
> > based while the AC200 is i2c based).
> >
> > > The DT node can then use phandles to the reset and
> > > clock controller to configure them as needed, the core will probably
> > > do that. I2C is a bit messier, you probably want a phandle pointing to
> > > the i2c_adapter, so you can use i2c_transfer() on it in the probe()
> > > function.
> >
> > Without a MFD or some other node that exposes a reset I'm a bit
> > confused about what driver would actually issue the reset.
> >
> > Yeah, we need a phandle to the i2c_adapter, but since the resets
> > would be under the AC200 PHY node I assume there would need
> > to be some sort of intermediary driver implementing the i2c reset
> > right?
>
> You need an reset controller, yes, but is that an MFD? Or just a reset
> controller? Where does this reset controller live?

Well at least the AC200(the i2c one) appears to be a full MFD:
https://github.com/DeciHD/allwinner_docs/blob/main/mfd_xpowers/AC200_Datasheet_V1.1.pdf

The AC300 appears to only have EPHY related functionality:
https://github.com/DeciHD/allwinner_docs/blob/main/mfd_xpowers/AC300_User_Manual_V1.0.pdf

> Question like this show we are missing the big picture. What are all
> the different parts, how are they interconnected? Once we see that, we
> can give you better advice.

If you look at the block diagrams here it should hopefully give you
a better idea of how the AC200 and AC300 PHY's are connected
on the H616:
http://file.whycan.com/files/202304/V85x/Linux_EMAC_%e5%bc%80%e5%8f%91%e6%8c%87%e5%8d%97.pdf

Re: [PATCH v1 3/3] dt-bindings: net: sun8i-emac: Add AC300 EMAC1 nvmem phy selection

[Krzysztof Kozlowski]

On Mon, May 26, 2025 at 12:29:36PM GMT, James Hilliard wrote:
> The Allwinner H616 EMAC1 can be connected to an on-die AC200 or AC300
> PHY depending upon the silicon variant.
> 
> Add a new allwinner,sun50i-h616-emac1 compatible and example, support
> for the allwinner,sun50i-h616-emac1 will be added later on.
> 
> Add nvmem-cells and nvmem-cell-names properties for the ac300 efuse
> based phy selection.
> 
> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> ---
>  .../net/allwinner,sun8i-a83t-emac.yaml        | 42 +++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml b/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml
> index 7fe0352dff0f..b6bf1718dba1 100644
> --- a/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml
> +++ b/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml
> @@ -18,6 +18,7 @@ properties:
>        - const: allwinner,sun8i-r40-gmac
>        - const: allwinner,sun8i-v3s-emac
>        - const: allwinner,sun50i-a64-emac
> +      - const: allwinner,sun50i-h616-emac1
>        - items:
>            - enum:
>                - allwinner,sun20i-d1-emac
> @@ -28,6 +29,14 @@ properties:
>    reg:
>      maxItems: 1
>  
> +  nvmem-cells:
> +    maxItems: 1
> +    description: NVMEM cell with the ac300 efuse.
> +
> +  nvmem-cell-names:
> +    items:
> +      - const: ac300
> +
>    interrupts:
>      maxItems: 1
>  
> @@ -321,4 +330,37 @@ examples:
>          };
>      };
>  
> +  - |
> +    ethernet@5030000 {
> +        compatible = "allwinner,sun50i-h616-emac1";
> +        reg = <0x05030000 0x10000>;

No need for new example for every soc.

But if you ever decide to add new example, it must work. Please test
your patches prior to sending them.

Best regards,
Krzysztof

Re: [PATCH v1 3/3] dt-bindings: net: sun8i-emac: Add AC300 EMAC1 nvmem phy selection

[Russell King (Oracle)]

On Mon, May 26, 2025 at 12:29:36PM -0600, James Hilliard wrote:
> The Allwinner H616 EMAC1 can be connected to an on-die AC200 or AC300
> PHY depending upon the silicon variant.
> 
> Add a new allwinner,sun50i-h616-emac1 compatible and example, support
> for the allwinner,sun50i-h616-emac1 will be added later on.
> 
> Add nvmem-cells and nvmem-cell-names properties for the ac300 efuse
> based phy selection.

You also need to mention the non-standard usage of phys and phy-names,
which is the whole reason I suggested you need to patch the binding.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!