[PATCH v1 1/3] net: stmmac: allow drivers to explicitly select PHY device

[PATCH v1 1/3] net: stmmac: allow drivers to explicitly select PHY device

[James Hilliard]

Some devices like the Allwinner H616 need the ability to select a phy
in cases where multiple PHY's may be present in a device tree due to
needing the ability to support multiple SoC variants with runtime
PHY selection.

Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 22 +++++++++++++------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 59d07d0d3369..949c4a8a1456 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1210,17 +1210,25 @@ static int stmmac_init_phy(struct net_device *dev)
 	 */
 	if (!phy_fwnode || IS_ERR(phy_fwnode)) {
 		int addr = priv->plat->phy_addr;
-		struct phy_device *phydev;
+		struct phy_device *phydev = NULL;
 
-		if (addr < 0) {
-			netdev_err(priv->dev, "no phy found\n");
-			return -ENODEV;
+		if (priv->plat->phy_node) {
+			phy_fwnode = of_fwnode_handle(priv->plat->phy_node);
+			phydev = fwnode_phy_find_device(phy_fwnode);
+			fwnode_handle_put(phy_fwnode);
 		}
 
-		phydev = mdiobus_get_phy(priv->mii, addr);
 		if (!phydev) {
-			netdev_err(priv->dev, "no phy at addr %d\n", addr);
-			return -ENODEV;
+			if (addr < 0) {
+				netdev_err(priv->dev, "no phy found\n");
+				return -ENODEV;
+			}
+
+			phydev = mdiobus_get_phy(priv->mii, addr);
+			if (!phydev) {
+				netdev_err(priv->dev, "no phy at addr %d\n", addr);
+				return -ENODEV;
+			}
 		}
 
 		ret = phylink_connect_phy(priv->phylink, phydev);
-- 
2.34.1

[PATCH v1 2/3] net: stmmac: dwmac-sun8i: Allow runtime AC200/AC300 phy selection

[James Hilliard]

The Allwinner H616 ships with two different on-die phy variants, in
order to determine the phy being used we need to read an efuse and
then select the appropriate PHY based on the AC300 bit.

By defining an emac node without a phy-handle we can override the
default PHY selection logic in stmmac by passing a specific phy_node
selected based on the ac200 and ac300 names in a phys list.

This allows us to have a device tree that defines both PHY variants
even though only one will actually end up being used at runtime
based on the ac300 nvmem efuse bit.

Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 .../net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 36 +++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
index 6c7e8655a7eb..50d37876fabf 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
@@ -11,6 +11,7 @@
 #include <linux/mdio-mux.h>
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
+#include <linux/nvmem-consumer.h>
 #include <linux/of.h>
 #include <linux/of_mdio.h>
 #include <linux/of_net.h>
@@ -280,6 +281,8 @@ static const struct emac_variant emac_variant_h6 = {
 #define SYSCON_ETCS_EXT_GMII	0x1
 #define SYSCON_ETCS_INT_GMII	0x2
 
+#define AC300_KEY		BIT(8) /* 1: AC300 PHY, 0: AC200 PHY */
+
 /* sun8i_dwmac_dma_reset() - reset the EMAC
  * Called from stmmac via stmmac_dma_ops->reset
  */
@@ -1149,6 +1152,35 @@ static struct regmap *sun8i_dwmac_get_syscon_from_dev(struct device_node *node)
 	return regmap;
 }
 
+/* H616 SoCs can contain either an AC200 PHY (needs i2c init) or an AC300
+ * PHY (no i2c). The silicon variant is flagged by the AC300_KEY efuse.
+ */
+static int sun8i_dwmac_get_ac300_phy(struct device *dev,
+				     struct plat_stmmacenet_data *plat_dat)
+{
+	u16 val;
+
+	/* If the nvmem cell is absent, use normal phy selection. */
+	if (nvmem_cell_read_u16(dev, "ac300", &val))
+		return 0;
+
+	const char *phy_name = (val & AC300_KEY) ? "ac300" : "ac200";
+	int index = of_property_match_string(dev->of_node, "phy-names",
+					     phy_name);
+	if (index < 0) {
+		dev_err(dev, "PHY name not found in device tree\n");
+		return -EINVAL;
+	}
+
+	plat_dat->phy_node = of_parse_phandle(dev->of_node, "phys", index);
+	if (!plat_dat->phy_node) {
+		dev_err(dev, "Failed to get PHY node from phys property\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int sun8i_dwmac_probe(struct platform_device *pdev)
 {
 	struct plat_stmmacenet_data *plat_dat;
@@ -1222,6 +1254,10 @@ static int sun8i_dwmac_probe(struct platform_device *pdev)
 	if (IS_ERR(plat_dat))
 		return PTR_ERR(plat_dat);
 
+	ret = sun8i_dwmac_get_ac300_phy(dev, plat_dat);
+	if (ret)
+		return ret;
+
 	/* platform data specifying hardware features and callbacks.
 	 * hardware features were copied from Allwinner drivers.
 	 */
-- 
2.34.1

Re: [PATCH v1 2/3] net: stmmac: dwmac-sun8i: Allow runtime AC200/AC300 phy selection

[Andrew Lunn]

On Mon, May 26, 2025 at 12:29:35PM -0600, James Hilliard wrote:
> The Allwinner H616 ships with two different on-die phy variants, in
> order to determine the phy being used we need to read an efuse and
> then select the appropriate PHY based on the AC300 bit.
> 
> By defining an emac node without a phy-handle we can override the
> default PHY selection logic in stmmac by passing a specific phy_node
> selected based on the ac200 and ac300 names in a phys list.

The normal way to do this is phy_find_first().

    Andrew

Re: [PATCH v1 2/3] net: stmmac: dwmac-sun8i: Allow runtime AC200/AC300 phy selection

[James Hilliard]

On Mon, May 26, 2025 at 1:56 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Mon, May 26, 2025 at 12:29:35PM -0600, James Hilliard wrote:
> > The Allwinner H616 ships with two different on-die phy variants, in
> > order to determine the phy being used we need to read an efuse and
> > then select the appropriate PHY based on the AC300 bit.
> >
> > By defining an emac node without a phy-handle we can override the
> > default PHY selection logic in stmmac by passing a specific phy_node
> > selected based on the ac200 and ac300 names in a phys list.
>
> The normal way to do this is phy_find_first().

Sure, but there are problems with that approach here.

The initialization sequences are rather different and the devices
won't be visible on the mdio bus until after they are initialized.

The resets will be specific to either the AC200 or AC300 so we
need to choose the correct PHY based on the efuse value rather
than a mdio bus scan to avoid a circular dependency essentially.

AC200: i2c based reset/init sequence
AC300: mdio based reset/init sequence

>
>     Andrew
>

Re: [PATCH v1 2/3] net: stmmac: dwmac-sun8i: Allow runtime AC200/AC300 phy selection

[Andrew Lunn]

> > The normal way to do this is phy_find_first().
> 
> Sure, but there are problems with that approach here.
> 
> The initialization sequences are rather different and the devices
> won't be visible on the mdio bus until after they are initialized.
> 
> The resets will be specific to either the AC200 or AC300 so we
> need to choose the correct PHY based on the efuse value rather
> than a mdio bus scan to avoid a circular dependency essentially.
> 
> AC200: i2c based reset/init sequence
> AC300: mdio based reset/init sequence

O.K. so you need to post more, so we get to see the complete
problem/solution. It seems to me, AC200 and AC300 are not compatible,
so should have different compatible strings. That might be part of the
solution. But it is too early to say.

	Andrew

Re: [PATCH v1 2/3] net: stmmac: dwmac-sun8i: Allow runtime AC200/AC300 phy selection

[James Hilliard]

On Mon, May 26, 2025 at 3:22 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > > The normal way to do this is phy_find_first().
> >
> > Sure, but there are problems with that approach here.
> >
> > The initialization sequences are rather different and the devices
> > won't be visible on the mdio bus until after they are initialized.
> >
> > The resets will be specific to either the AC200 or AC300 so we
> > need to choose the correct PHY based on the efuse value rather
> > than a mdio bus scan to avoid a circular dependency essentially.
> >
> > AC200: i2c based reset/init sequence
> > AC300: mdio based reset/init sequence
>
> O.K. so you need to post more, so we get to see the complete
> problem/solution. It seems to me, AC200 and AC300 are not compatible,
> so should have different compatible strings. That might be part of the
> solution. But it is too early to say.

They will need to use different reset drivers but the mac part is
largely the same AFAIU. The mdio part is similar after initialization
as well I think.

These are the vendor docs(I only found chinese version so far) that
have some more details on the AC200 and AC300 EPHY's:
http://file.whycan.com/files/202304/V85x/Linux_EMAC_%e5%bc%80%e5%8f%91%e6%8c%87%e5%8d%97.pdf

Translated important sections:

For AC200:
ARM communicates with AC200 through TWI, initializes EPHY, and then
MAC accesses the MDIO bus.
EPHY, PWM module provides an internal 25M clock to EPHY.

For AC300:
ARM communicates with AC300 through MDIO bus, initializes EPHY, and
then MAC accesses EPHY through MDIO bus. PWM module provides an
internal
25M clock to EPHY.

So the MAC to EPHY connection is more or less the same AFAIU,
but the initialization is quite different.

>
>         Andrew
>

[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!