ST HAL: Ethernet PHY address resolution

[AlexFabre]

Introduction

The objective of this discussion is to find a way to simplify ethernet PHY address resolution inside ST's HAL.

The problem

Currently there are two distinct settings that define the PHY address that the HAL has to use:

• KConfig definition CONFIG_ETH_STM32_HAL_PHY_ADDRESS.
• Devicetree node ethernet-phy and its reg field.

&mdio {
	status = "okay";
	pinctrl-0 = <&eth_mdio_pa2 &eth_mdc_pc1>;
	pinctrl-names = "default";

	ethernet-phy@0 {
		compatible = "ethernet-phy";
		reg = <0x00>;
		status = "okay";
	};
};

drivers/ethernet/eth_stm32_hal.c

#define PHY_ADDR	CONFIG_ETH_STM32_HAL_PHY_ADDRESS

#if defined(CONFIG_MDIO)

#define DEVICE_PHY_BY_NAME(n) \
	    DEVICE_DT_GET(DT_CHILD(DT_INST_CHILD(n, mdio), __CONCAT(ethernet_phy_, PHY_ADDR)))

static const struct device *eth_stm32_phy_dev = DEVICE_PHY_BY_NAME(0);

#endif

Propositions

Resolve with node label

Adding a node label that indicates in the device tree the physical line used by the HAL.

This solution allows a board to have multiple PHY connected, and the one used by the HAL is clearly identified.

&mdio {
	status = "okay";
	pinctrl-0 = <&eth_mdio_pa2 &eth_mdc_pc1>;
	pinctrl-names = "default";

	hal_ethernet_phy: ethernet-phy@1 {
		compatible = "ethernet-phy";
		reg = <1>;
		status = "okay";
	};
};

drivers/ethernet/eth_stm32_hal.c

#define PHY_ADDR	DT_REG_ADDR(DT_NODELABEL(hal_ethernet_phy))

#if defined(CONFIG_MDIO)

static const struct device *eth_stm32_phy_dev = DEVICE_DT_GET(DT_NODELABEL(hal_ethernet_phy));

#endif

[erwango]

Thanks for trying to address this issue.
Actually you're not supposed to make reference to sw implementation in device tree. So making use of hal_ethernet_phy is not a valid reference.

Likely, what we miss is a way to bind the node with its phy.
As a matter of fact, as I've just found ethernet-controller.yaml binding declare a phy-handle which would allow to bind eth node with phy node.

Using this new property eth node, we could do something like (quick attempt, to be verified):

#define PHY_ADDR DT_REG_ADDR(DT_INST_PHANDLE(0, phy_handle)))

#if defined(CONFIG_MDIO)

static const struct device *eth_stm32_phy_dev = DEVICE_DT_GET(DT_INST_PHANDLE(0, phy_handle));

#endif

Note that it would also require some protection for the parts that don't declare this property, but it should be the correct way to go.

[AlexFabre]

Indeed, you're right @erwango, the phy-handle is the way to go.

Following the numerous examples that can be found, I've followed this implementation:

Unfortunately I have a device tree node token error at compile time...

To reproduce:

Board: stm32h573i_dk
Project: samples/net/ipv4_autoconf/

boards/st/stm32h573i_dk/stm32h573i_dk.dts

&mac {
	status = "okay";
	pinctrl-0 = <&eth_rxd0_pc4
		     &eth_rxd1_pc5
		     &eth_ref_clk_pa1
		     &eth_crs_dv_pa7
		     &eth_tx_en_pg11
		     &eth_txd0_pg13
		     &eth_txd1_pg12>;
	pinctrl-names = "default";
+	phy-handle = <&phy>;
};

&mdio {
	status = "okay";
	pinctrl-0 = <&eth_mdio_pa2 &eth_mdc_pc1>;
	pinctrl-names = "default";

-       ethernet-phy@0 {
+	phy: ethernet-phy@0 {
		compatible = "ethernet-phy";
		reg = <0x00>;
		status = "okay";
	};
};

drivers/ethernet/eth_stm32_hal.c

- #define PHY_ADDR	CONFIG_ETH_STM32_HAL_PHY_ADDRESS
+ #define PHY_ADDR	DT_REG_ADDR(DT_INST_PHANDLE(0, phy_handle))

#if defined(CONFIG_MDIO)

- #define DEVICE_PHY_BY_NAME(n) \
-	    DEVICE_DT_GET(DT_CHILD(DT_INST_CHILD(n, mdio), _CONCAT(ethernet_phy_, PHY_ADDR)))

- static const struct device *eth_stm32_phy_dev = DEVICE_PHY_BY_NAME(0);
+ static const struct device *eth_stm32_phy_dev = DEVICE_DT_GET(DT_INST_PHANDLE(0, phy_handle));

west build -b stm32h573i_dk samples/net/ipv4_autoconf/

[erwango]

Thanks for providing the way to reproduce, but can you also provide the error returned ? (so I can have a quick idea of the issue)

[AlexFabre]

The error is quite explosive, it triggers hundreds of:

/Users/alex/Workspace/walle/zephyr/include/zephyr/device.h:102:45: error: pasting "dts_ord_" and "-" does not give a valid preprocessing token
  102 | #define Z_DEVICE_DT_DEV_ID(node_id) _CONCAT(dts_ord_, DT_DEP_ORD(node_id))
      |                                             ^~~~~~~~

and

/Users/alex/Workspace/walle/zephyr/build/zephyr/include/generated/zephyr/devicetree_generated.h:1037:67: error: expected '=', ',', ';', 'asm' or '__attribute__' before '-' token
 1037 | #define DT_N_S_soc_S_pin_controller_42020000_S_eth_tx_en_pg11_ORD -1
      |                                                                   ^

[erwango]

Ok, so I'm not able to dig much into this right now, indeed the actual issue would be the negative value provided as ordinal (_ORD -1)
For some reason, the node wasn't parsed correctly by scripts/dts/python-devicetree/src/devicetree/edtlib.py
Giving -4 in

zephyr/scripts/dts/python-devicetree/src/devicetree/edtlib.py

Line 1107 in 98e0e65

self.dep_ordinal: int = -1

will output in -4 as ordinal for the node and this is due to phy-handle = <&phy>; introduction, for some reason

[AlexFabre]

What would be the best way to ask for some help on this point ? Do you think I should open an issue ?

[erwango]

Don't raise an issue as there is no issue on the code available in the tree. You can try to get some help in discord though.

[erwango]

On alternate idea is to try to reproduce the issue using tests/lib/devicetree/api, updating app.overlay to reproduce the dt configuration you're using.
If the overlay modification is legit, then the issue would be valid.

[mathieuchopstm]

After investigation, this issue seems to be caused by the "dependency loop" between the ethernet controller and the ethernet-phy.

I haven't dived too deep in EDTlib, but as far as I understand, the ordinals generation is done by walking up the tree from all roots - that is, nodes with no incoming edge. Given the following device tree:

flowchart BT
    eth-phy[phy: ethernet-phy@0] --> mdio[mdio: mdio] --> eth-ctrl[mac: ethernet@40028000] --> soc --> /

    pinctrl(pinctrl) --> soc

    mdio --> mdio-pins(*MDIO pinctrl nodes*) --> pinctrl
    eth-ctrl --> eth-pins(*Ethernet pinctrl nodes*) --> pinctrl

Loading

ethernet-phy@0 has no incoming edge, so it is treated as a root. It will get assigned an ordinal, then all its parents nodes will get one as the tree is walked up until / is reached.

By applying the following patch to zephyrscripts/dts/python-devicetree/src/devicetree/grutils.py, we can display all edges and roots:

diff --git a/scripts/dts/python-devicetree/src/devicetree/grutils.py b/scripts/dts/python-devicetree/src/devicetree/grutils.py
index 452ee4aaaae..b8bf5875408 100644
--- a/scripts/dts/python-devicetree/src/devicetree/grutils.py
+++ b/scripts/dts/python-devicetree/src/devicetree/grutils.py
@@ -38,6 +38,7 @@ class Graph:
 
         The nodes are added to the graph if necessary.
         """
+        print(f"New edge: {source.label if source.label else source.name} -> {target.label if target.label else target.name}")
         self.__edge_map[source].add(target)
         if source != target:
             self.__reverse_map[target].add(source)
@@ -84,6 +85,10 @@ class Graph:
         if self.__nodes and not roots:
             raise Exception('TARJAN: No roots found in graph with {} nodes'.format(len(self.__nodes)))
 
+        print(roots)
+        import sys
+        sys.exit(1)
+
         for r in roots:
             self._tarjan_root(r)

When phy-handle = <&phy> is not present, the edges are displayed as seen earlier, and the PHY is visible in the list of roots:

	New edge: ethernet-phy@0 -> mdio
	New edge: mdio -> ethernet@40028000
[and in the roots list:]
	<Node /soc/ethernet@40028000/mdio/ethernet-phy@0 in ...>

By adding phy-handle = <&phy>;, we create an a circular loop: the PHY now has an incoming edge from a node to which it (indirectly) has an outgoing edge. As such, it is no longer visible in the roots list, and no ordinal is generated for it, not any node "above" it (e.g., the MDIO, MAC, or ETH pinctrl nodes):

	New edge: ethernet-phy@0 -> mdio
	New edge: mdio -> ethernet@40028000
	New edge: ethernet@40028000 -> ethernet-phy@0
[and in the roots list: none of the nodes we care about!]

flowchart BT
    eth-phy[ethernet-phy@0] --> mdio[mdio: mdio] --> eth-ctrl[mac: ethernet@40028000] --> soc --> /

    pinctrl(pinctrl) --> soc

    mdio --> mdio-pins(*MDIO pinctrl nodes*) --> pinctrl
    eth-ctrl --> eth-pins(*Ethernet pinctrl nodes*) --> pinctrl

    eth-ctrl -->|&phy| eth-phy

Loading

This sounds like a bug in EDTlib: I would expect cycles like this to be caught.
However, I don't whether cycles should be supported by EDTlib, or simply detected to abort cleanly.

What's being done in this SoC could be a fitting (and guaranteed to work) solution for this problem:

zephyr/dts/arm64/fvp/fvp-aemv8r.dtsi

Lines 107 to 130 in a5e3a33

	ethernet@9a000000 {
	reg = <0x9a000000 0x1000>;
	eth: ethernet {
	compatible = "smsc,lan91c111";
	interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL IRQ_DEFAULT_PRIORITY>;
	status = "disabled";
	phy-handle = <&phy>;
	};
	mdio: mdio {
	compatible = "smsc,lan91c111-mdio";
	status = "disabled";
	#address-cells = <1>;
	#size-cells = <0>;
	phy: ethernet-phy@0 {
	compatible = "ethernet-phy";
	status = "disabled";
	reg = <0>;
	};
	};
	};