| Message ID | 20240309012538.719518-1-niklas.soderlund+renesas@ragnatech.se |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | dt-bindings: net: renesas,etheravb: Add MDIO bus reset properties | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On 09/03/2024 02:25, Niklas Söderlund wrote: > The bindings for Renesas Ethernet AVB are from 2015 and contain some > oddities that are impossible to get right without breaking existing > bindings. One such thing is that the MDIO bus properties that should be > its own node are mixed with the node for the IP for Ethernet AVB. > > Instead of a separate node for the MDIO bus, > > avb: ethernet@e6800000 { > compatible = "renesas,etheravb-r8a7795", > "renesas,etheravb-rcar-gen3"; > reg = <0xe6800000 0x800>, <0xe6a00000 0x10000>; > > ... > > phy-handle = <&phy0>; > > mdio { > #address-cells = <1>; > #size-cells = <0>; > > phy0: ethernet-phy@0 { > ... > }; > }; > }; > > The Ethernet AVB mix it in one, > > avb: ethernet@e6800000 { > compatible = "renesas,etheravb-r8a7795", > "renesas,etheravb-rcar-gen3"; > reg = <0xe6800000 0x800>, <0xe6a00000 0x10000>; > > ... > > phy-handle = <&phy0>; > > #address-cells = <1>; > #size-cells = <0>; > > phy0: ethernet-phy@0 { > ... > }; > }; > > This forces to all MDIO bus properties needed to be described in the > Ethernet AVB bindings directly. However not all MDIO bus properties are > described as they were not needed. This change adds the MDIO bus > properties to reset the MDIO bus in preparation for them being used. That is not exactly what you wrote in the binding. Binding suggests you have per device GPIO, which in your design is shared but the commit msg says it is some sort of bus reset. These are two different things, because in first case you could have a design using two GPIOs, not one. Then your binding is completely wrong. Plus, where is the user of all this? I think you should rather correct the binding to use mdio node and add appropriate handling in the driver, keeping backward compatibility. Best regards, Krzysztof
Hi Krzysztof, Thanks for your reply. On 2024-03-09 12:45:06 +0100, Krzysztof Kozlowski wrote: > On 09/03/2024 02:25, Niklas Söderlund wrote: > > The bindings for Renesas Ethernet AVB are from 2015 and contain some > > oddities that are impossible to get right without breaking existing > > bindings. One such thing is that the MDIO bus properties that should be > > its own node are mixed with the node for the IP for Ethernet AVB. > > > > Instead of a separate node for the MDIO bus, > > > > avb: ethernet@e6800000 { > > compatible = "renesas,etheravb-r8a7795", > > "renesas,etheravb-rcar-gen3"; > > reg = <0xe6800000 0x800>, <0xe6a00000 0x10000>; > > > > ... > > > > phy-handle = <&phy0>; > > > > mdio { > > #address-cells = <1>; > > #size-cells = <0>; > > > > phy0: ethernet-phy@0 { > > ... > > }; > > }; > > }; > > > > The Ethernet AVB mix it in one, > > > > avb: ethernet@e6800000 { > > compatible = "renesas,etheravb-r8a7795", > > "renesas,etheravb-rcar-gen3"; > > reg = <0xe6800000 0x800>, <0xe6a00000 0x10000>; > > > > ... > > > > phy-handle = <&phy0>; > > > > #address-cells = <1>; > > #size-cells = <0>; > > > > phy0: ethernet-phy@0 { > > ... > > }; > > }; > > > > This forces to all MDIO bus properties needed to be described in the > > Ethernet AVB bindings directly. However not all MDIO bus properties are > > described as they were not needed. This change adds the MDIO bus > > properties to reset the MDIO bus in preparation for them being used. > > That is not exactly what you wrote in the binding. Binding suggests you > have per device GPIO, which in your design is shared but the commit msg > says it is some sort of bus reset. > > These are two different things, because in first case you could have a > design using two GPIOs, not one. Then your binding is completely wrong. I copied the description from mdio.yaml as the intention is the two are the same. But on a second read I agree it's confusing. > > Plus, where is the user of all this? I think you found it in, for posterity it's in '[PATCH] arm64: dts: renesas: white-hawk: ethernet: Describe adv1 and avb2'. > > I think you should rather correct the binding to use mdio node and add > appropriate handling in the driver, keeping backward compatibility. I had not consider that, the driver can indeed support both modes and only allow MDIO bus properties if the mdio sub-node is present. I will do so, thanks for the suggestion.
On Sat, Mar 09, 2024 at 02:25:38AM +0100, Niklas Söderlund wrote: > The bindings for Renesas Ethernet AVB are from 2015 and contain some > oddities that are impossible to get right without breaking existing > bindings. One such thing is that the MDIO bus properties that should be > its own node are mixed with the node for the IP for Ethernet AVB. > > Instead of a separate node for the MDIO bus, > > avb: ethernet@e6800000 { > compatible = "renesas,etheravb-r8a7795", > "renesas,etheravb-rcar-gen3"; > reg = <0xe6800000 0x800>, <0xe6a00000 0x10000>; > > ... > > phy-handle = <&phy0>; > > mdio { > #address-cells = <1>; > #size-cells = <0>; > > phy0: ethernet-phy@0 { > ... > }; > }; > }; > > The Ethernet AVB mix it in one, > > avb: ethernet@e6800000 { > compatible = "renesas,etheravb-r8a7795", > "renesas,etheravb-rcar-gen3"; > reg = <0xe6800000 0x800>, <0xe6a00000 0x10000>; > > ... > > phy-handle = <&phy0>; > > #address-cells = <1>; > #size-cells = <0>; > > phy0: ethernet-phy@0 { > ... > }; > }; This was at one time O.K, and since we don't remove things because that would break backwards compatibility, its still technically O.K. It does however lead to problems, so newer drivers have used a dedicated MDIO node. The yaml i think only documents the use of an MDIO node. If you want older DT blobs to pass validation, you might want to add the extra properties to the renesas,etheravb-r8a7795 binding. You also see some older drivers which have migrated to first look for an MDIO node and use it if found, and then fallback to the old scheme. So that is what i would suggest here. You could then convert all in tree dts files to having an MDIO node, which will make the yaml validator happy. Andrew
diff --git a/Documentation/devicetree/bindings/net/renesas,etheravb.yaml b/Documentation/devicetree/bindings/net/renesas,etheravb.yaml index de7ba7f345a9..d87f00a25877 100644 --- a/Documentation/devicetree/bindings/net/renesas,etheravb.yaml +++ b/Documentation/devicetree/bindings/net/renesas,etheravb.yaml @@ -93,6 +93,25 @@ properties: description: Number of size cells on the MDIO bus. const: 0 + reset-gpios: + maxItems: 1 + description: + The phandle and specifier for the GPIO that controls the RESET + lines of all devices on that MDIO bus. + + reset-delay-us: + description: + RESET pulse width in microseconds. It applies to all MDIO devices + and must therefore be appropriately determined based on all devices + requirements (maximum value of all per-device RESET pulse widths). + + reset-post-delay-us: + description: + Delay after reset deassert in microseconds. It applies to all MDIO + devices and it's determined by how fast all devices are ready for + communication. This delay happens just before e.g. Ethernet PHY + type ID auto detection. + renesas,no-ether-link: type: boolean description:
The bindings for Renesas Ethernet AVB are from 2015 and contain some oddities that are impossible to get right without breaking existing bindings. One such thing is that the MDIO bus properties that should be its own node are mixed with the node for the IP for Ethernet AVB. Instead of a separate node for the MDIO bus, avb: ethernet@e6800000 { compatible = "renesas,etheravb-r8a7795", "renesas,etheravb-rcar-gen3"; reg = <0xe6800000 0x800>, <0xe6a00000 0x10000>; ... phy-handle = <&phy0>; mdio { #address-cells = <1>; #size-cells = <0>; phy0: ethernet-phy@0 { ... }; }; }; The Ethernet AVB mix it in one, avb: ethernet@e6800000 { compatible = "renesas,etheravb-r8a7795", "renesas,etheravb-rcar-gen3"; reg = <0xe6800000 0x800>, <0xe6a00000 0x10000>; ... phy-handle = <&phy0>; #address-cells = <1>; #size-cells = <0>; phy0: ethernet-phy@0 { ... }; }; This forces to all MDIO bus properties needed to be described in the Ethernet AVB bindings directly. However not all MDIO bus properties are described as they were not needed. This change adds the MDIO bus properties to reset the MDIO bus in preparation for them being used. Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> --- .../bindings/net/renesas,etheravb.yaml | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)