diff mbox series

dt-bindings: net: renesas,etheravb: Add MDIO bus reset properties

Message ID 20240309012538.719518-1-niklas.soderlund+renesas@ragnatech.se (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series dt-bindings: net: renesas,etheravb: Add MDIO bus reset properties | expand

Commit Message

Niklas Söderlund March 9, 2024, 1:25 a.m. UTC
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(+)

Comments

Krzysztof Kozlowski March 9, 2024, 11:45 a.m. UTC | #1
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
Niklas Söderlund March 9, 2024, 2:41 p.m. UTC | #2
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.
Andrew Lunn March 12, 2024, 9:54 p.m. UTC | #3
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 mbox series

Patch

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: