[PATCH v4 0/4] RTL9300 MDIO driver

[PATCH v4 0/4] RTL9300 MDIO driver

[Chris Packham]

This series adds a driver for the MDIO controller on the RTL9300 family
of devices. The controller is a little unique in that we can't access the SMI
interfaces directly. This means we need to use the hardware description from
the DTS to compute a mapping of switch port to mdio bus/address.

Chris Packham (4):
  dt-bindings: net: Add Realtek MDIO controller
  dt-bindings: mfd: Add MDIO interface to rtl9301-switch
  mips: dts: realtek: Add MDIO controller
  net: mdio: Add RTL9300 MDIO driver

 .../bindings/mfd/realtek,rtl9301-switch.yaml  |  24 +
 .../bindings/net/realtek,rtl9301-mdio.yaml    |  93 ++++
 arch/mips/boot/dts/realtek/rtl930x.dtsi       |  32 ++
 drivers/net/mdio/Kconfig                      |   7 +
 drivers/net/mdio/Makefile                     |   1 +
 drivers/net/mdio/mdio-realtek-rtl9300.c       | 417 ++++++++++++++++++
 6 files changed, 574 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/realtek,rtl9301-mdio.yaml
 create mode 100644 drivers/net/mdio/mdio-realtek-rtl9300.c

-- 
2.47.1

[PATCH v4 1/4] dt-bindings: net: Add Realtek MDIO controller

[Chris Packham]

Add dtschema for the MDIO controller found in the RTL9300 SoCs. The
controller is slightly unusual in that direct MDIO communication is not
possible. We model the MDIO controller with the MDIO buses as child
nodes and the PHYs as children of the buses. Because we do need the
switch port number to actually communicate over the MDIO bus this needs
to be supplied via the "realtek,port" property.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Notes:
    Changes in v4:
    - Model the MDIO controller with the buses as child nodes. We still need
      to deal with the switch port number so this is represented with the
      "realtek,port" property which needs to be added to the MDIO bus
      children (i.e. the PHYs)
    - Because the above is quite a departure from earlier I've dropped the
      r-by
    Changes in v3:
    - Add r-by from Connor
    Changes in v2:
    - None

 .../bindings/net/realtek,rtl9301-mdio.yaml    | 93 +++++++++++++++++++
 1 file changed, 93 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/realtek,rtl9301-mdio.yaml

diff --git a/Documentation/devicetree/bindings/net/realtek,rtl9301-mdio.yaml b/Documentation/devicetree/bindings/net/realtek,rtl9301-mdio.yaml
new file mode 100644
index 000000000000..e3ecb1b4afd3
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/realtek,rtl9301-mdio.yaml
@@ -0,0 +1,93 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/realtek,rtl9301-mdio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Realtek RTL9300 MDIO Controller
+
+maintainers:
+  - Chris Packham <chris.packham@alliedtelesis.co.nz>
+
+properties:
+  compatible:
+    oneOf:
+      - items:
+          - enum:
+              - realtek,rtl9302b-mdio
+              - realtek,rtl9302c-mdio
+              - realtek,rtl9303-mdio
+          - const: realtek,rtl9301-mdio
+      - const: realtek,rtl9301-mdio
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+patternProperties:
+  '^mdio-bus@[0-4]$':
+    $ref: mdio.yaml#
+
+    properties:
+      reg:
+        maxItems: 1
+
+    required:
+      - reg
+
+    patternProperties:
+      '^ethernet-phy(@[a-f0-9]+)?':
+        type: object
+        $ref: ethernet-phy.yaml#
+
+        properties:
+          realtek,port:
+            $ref: /schemas/types.yaml#/definitions/uint32
+            description:
+              The MDIO communication on the RTL9300 is abstracted by the switch. At
+              the software level communication uses the switch port to address the
+              PHY with the actual MDIO bus and address having been setup via the
+              parent mdio-bus and reg property.
+
+        unevaluatedProperties: false
+
+    unevaluatedProperties: false
+
+required:
+  - compatible
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    mdio-controller {
+      compatible = "realtek,rtl9301-mdio";
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      mdio-bus@0 {
+        reg = <0>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ethernet-phy@0 {
+          compatible = "ethernet-phy-ieee802.3-c45";
+          reg = <0>;
+          realtek,port = <0>;
+        };
+      };
+
+      mdio-bus@1 {
+        reg = <1>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ethernet-phy@0 {
+          compatible = "ethernet-phy-ieee802.3-c45";
+          reg = <0>;
+          realtek,port = <8>;
+        };
+      };
+    };
-- 
2.47.1

Re: [PATCH v4 1/4] dt-bindings: net: Add Realtek MDIO controller

[Krzysztof Kozlowski]

On Mon, Jan 20, 2025 at 05:02:11PM +1300, Chris Packham wrote:
> Add dtschema for the MDIO controller found in the RTL9300 SoCs. The
> controller is slightly unusual in that direct MDIO communication is not
> possible. We model the MDIO controller with the MDIO buses as child
> nodes and the PHYs as children of the buses. Because we do need the
> switch port number to actually communicate over the MDIO bus this needs
> to be supplied via the "realtek,port" property.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> 
> Notes:
>     Changes in v4:
>     - Model the MDIO controller with the buses as child nodes. We still need
>       to deal with the switch port number so this is represented with the
>       "realtek,port" property which needs to be added to the MDIO bus
>       children (i.e. the PHYs)
>     - Because the above is quite a departure from earlier I've dropped the
>       r-by
>     Changes in v3:
>     - Add r-by from Connor
>     Changes in v2:
>     - None
> 
>  .../bindings/net/realtek,rtl9301-mdio.yaml    | 93 +++++++++++++++++++
>  1 file changed, 93 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/realtek,rtl9301-mdio.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/realtek,rtl9301-mdio.yaml b/Documentation/devicetree/bindings/net/realtek,rtl9301-mdio.yaml
> new file mode 100644
> index 000000000000..e3ecb1b4afd3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/realtek,rtl9301-mdio.yaml
> @@ -0,0 +1,93 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/realtek,rtl9301-mdio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Realtek RTL9300 MDIO Controller
> +
> +maintainers:
> +  - Chris Packham <chris.packham@alliedtelesis.co.nz>
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - items:
> +          - enum:
> +              - realtek,rtl9302b-mdio
> +              - realtek,rtl9302c-mdio
> +              - realtek,rtl9303-mdio
> +          - const: realtek,rtl9301-mdio
> +      - const: realtek,rtl9301-mdio
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +patternProperties:
> +  '^mdio-bus@[0-4]$':
> +    $ref: mdio.yaml#
> +
> +    properties:
> +      reg:
> +        maxItems: 1
> +
> +    required:
> +      - reg
> +
> +    patternProperties:
> +      '^ethernet-phy(@[a-f0-9]+)?':

Why is the unit address optional?

> +        type: object
> +        $ref: ethernet-phy.yaml#
> +
> +        properties:
> +          realtek,port:
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +            description:
> +              The MDIO communication on the RTL9300 is abstracted by the switch. At
> +              the software level communication uses the switch port to address the
> +              PHY with the actual MDIO bus and address having been setup via the
> +              parent mdio-bus and reg property.

I don't quite get why this cannot be the 'reg' property. I understood that
'reg' of this node is not really used? Or you meant here this 'reg', not
parent's 'reg'?

Best regards,
Krzysztof

Re: [PATCH v4 1/4] dt-bindings: net: Add Realtek MDIO controller

[Chris Packham]

Hi Krzyztof,

Sorry meant to reply to this yesterday but ran out of time

On 22/01/2025 21:12, Krzysztof Kozlowski wrote:
> On Mon, Jan 20, 2025 at 05:02:11PM +1300, Chris Packham wrote:
>> Add dtschema for the MDIO controller found in the RTL9300 SoCs. The
>> controller is slightly unusual in that direct MDIO communication is not
>> possible. We model the MDIO controller with the MDIO buses as child
>> nodes and the PHYs as children of the buses. Because we do need the
>> switch port number to actually communicate over the MDIO bus this needs
>> to be supplied via the "realtek,port" property.
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> ---
>>
>> Notes:
>>      Changes in v4:
>>      - Model the MDIO controller with the buses as child nodes. We still need
>>        to deal with the switch port number so this is represented with the
>>        "realtek,port" property which needs to be added to the MDIO bus
>>        children (i.e. the PHYs)
>>      - Because the above is quite a departure from earlier I've dropped the
>>        r-by
>>      Changes in v3:
>>      - Add r-by from Connor
>>      Changes in v2:
>>      - None
>>
>>   .../bindings/net/realtek,rtl9301-mdio.yaml    | 93 +++++++++++++++++++
>>   1 file changed, 93 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/net/realtek,rtl9301-mdio.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/net/realtek,rtl9301-mdio.yaml b/Documentation/devicetree/bindings/net/realtek,rtl9301-mdio.yaml
>> new file mode 100644
>> index 000000000000..e3ecb1b4afd3
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/realtek,rtl9301-mdio.yaml
>> @@ -0,0 +1,93 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/net/realtek,rtl9301-mdio.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Realtek RTL9300 MDIO Controller
>> +
>> +maintainers:
>> +  - Chris Packham <chris.packham@alliedtelesis.co.nz>
>> +
>> +properties:
>> +  compatible:
>> +    oneOf:
>> +      - items:
>> +          - enum:
>> +              - realtek,rtl9302b-mdio
>> +              - realtek,rtl9302c-mdio
>> +              - realtek,rtl9303-mdio
>> +          - const: realtek,rtl9301-mdio
>> +      - const: realtek,rtl9301-mdio
>> +
>> +  '#address-cells':
>> +    const: 1
>> +
>> +  '#size-cells':
>> +    const: 0
>> +
>> +patternProperties:
>> +  '^mdio-bus@[0-4]$':
>> +    $ref: mdio.yaml#
>> +
>> +    properties:
>> +      reg:
>> +        maxItems: 1
>> +
>> +    required:
>> +      - reg
>> +
>> +    patternProperties:
>> +      '^ethernet-phy(@[a-f0-9]+)?':
> Why is the unit address optional?

No specific reason. I can make it mandatory, in all likelihood any board 
using this chip will have more than one PHY connected so it would have 
been defacto required anyway.

>> +        type: object
>> +        $ref: ethernet-phy.yaml#
>> +
>> +        properties:
>> +          realtek,port:
>> +            $ref: /schemas/types.yaml#/definitions/uint32
>> +            description:
>> +              The MDIO communication on the RTL9300 is abstracted by the switch. At
>> +              the software level communication uses the switch port to address the
>> +              PHY with the actual MDIO bus and address having been setup via the
>> +              parent mdio-bus and reg property.
> I don't quite get why this cannot be the 'reg' property. I understood that
> 'reg' of this node is not really used? Or you meant here this 'reg', not
> parent's 'reg'?

It's is a bit confusing (any suggestions for improving the description 
and/or commit message are welcome).

The 'reg' property here is the MDIO address of the PHY, the parent 'reg' 
is the MDIO bus number. That's the information the "normal" bindings for 
Ethernet PHYs use. The MDIO (a.k.a. SMI) accesses via the RTL9300 use 
the switch port so we need to know the switch port number.

An earlier incarnation of this patchset had the switch port number 
masquerading as the MDIO address but that would have allowed nonsensical 
addresses >0x1f and meant that aspects of the hardware design were 
tucked away in special vendor specific properties. The MDIO bus/address 
is still used it's just setup once at probe time and now my driver maps 
the MDIO bus/address to the switch port number that the hardware 
ultimately uses.

Re: [PATCH v4 1/4] dt-bindings: net: Add Realtek MDIO controller

[Andrew Lunn]

> >> +        properties:
> >> +          realtek,port:
> >> +            $ref: /schemas/types.yaml#/definitions/uint32
> >> +            description:
> >> +              The MDIO communication on the RTL9300 is abstracted by the switch. At
> >> +              the software level communication uses the switch port to address the
> >> +              PHY with the actual MDIO bus and address having been setup via the
> >> +              parent mdio-bus and reg property.
> > I don't quite get why this cannot be the 'reg' property. I understood that
> > 'reg' of this node is not really used? Or you meant here this 'reg', not
> > parent's 'reg'?
> 
> It's is a bit confusing (any suggestions for improving the description 
> and/or commit message are welcome).

I don't know if it will actually help, but....

We have two entangled configurations here.

1) You have 4 MDIO busses which you need to describe using mdio.yaml
   In this binding, reg is the address of the device on the bus, in
   the range 0-31.

2) The hardware was a pool of PHYs which you can map to address on the
   MDIO busses.

Rather than combining them, maybe it would be better to keep them
separate. It is probably more error prone, but simpler to
understand. And hopefully errors result in PHYs not being found during
probe, so the problems are obvious.

Maybe you can actually use phandles. You have the usual MDIO bus
nodes:

    mdio@5c030000 {
        #address-cells = <1>;
        #size-cells = <0>;

        ethphy0: ethernet-phy@1 {
            reg = <1>;
        };

        ethphy1: ethernet-phy@3 {
            reg = <3>;
        };
    };

    mdio@5c040000 {
        #address-cells = <1>;
        #size-cells = <0>;

        ethphy2: ethernet-phy@1 {
            reg = <1>;
        };

        ethphy3: ethernet-phy@3 {
            reg = <3>;
        };
    };

    mdio@5c050000 {
       ...
    }

    mdio@5c060000 {
       ...
    }

    And then a node which is a list of PHY phandles:

    [&ethphy0, &ethphy1, &ethphy2, &ethphy3, ....]

The 0th entry in the list tells you have to map the 0th PHY in the
pool to an MDIO bus and address. Follow the phandle to get the MDIO
bus and the address on the bus.

	Andrew

Re: [PATCH v4 1/4] dt-bindings: net: Add Realtek MDIO controller

[Chris Packham]

On 24/01/2025 11:08, Andrew Lunn wrote:
>>>> +        properties:
>>>> +          realtek,port:
>>>> +            $ref: /schemas/types.yaml#/definitions/uint32
>>>> +            description:
>>>> +              The MDIO communication on the RTL9300 is abstracted by the switch. At
>>>> +              the software level communication uses the switch port to address the
>>>> +              PHY with the actual MDIO bus and address having been setup via the
>>>> +              parent mdio-bus and reg property.
>>> I don't quite get why this cannot be the 'reg' property. I understood that
>>> 'reg' of this node is not really used? Or you meant here this 'reg', not
>>> parent's 'reg'?
>> It's is a bit confusing (any suggestions for improving the description
>> and/or commit message are welcome).
> I don't know if it will actually help, but....
>
> We have two entangled configurations here.
>
> 1) You have 4 MDIO busses which you need to describe using mdio.yaml
>     In this binding, reg is the address of the device on the bus, in
>     the range 0-31.
>
> 2) The hardware was a pool of PHYs which you can map to address on the
>     MDIO busses.
>
> Rather than combining them, maybe it would be better to keep them
> separate. It is probably more error prone, but simpler to
> understand. And hopefully errors result in PHYs not being found during
> probe, so the problems are obvious.
>
> Maybe you can actually use phandles. You have the usual MDIO bus
> nodes:
>
>      mdio@5c030000 {
>          #address-cells = <1>;
>          #size-cells = <0>;
>
>          ethphy0: ethernet-phy@1 {
>              reg = <1>;
>          };
>
>          ethphy1: ethernet-phy@3 {
>              reg = <3>;
>          };
>      };
>
>      mdio@5c040000 {
>          #address-cells = <1>;
>          #size-cells = <0>;
>
>          ethphy2: ethernet-phy@1 {
>              reg = <1>;
>          };
>
>          ethphy3: ethernet-phy@3 {
>              reg = <3>;
>          };
>      };
>
>      mdio@5c050000 {
>         ...
>      }
>
>      mdio@5c060000 {
>         ...
>      }
>
>      And then a node which is a list of PHY phandles:
>
>      [&ethphy0, &ethphy1, &ethphy2, &ethphy3, ....]
>
> The 0th entry in the list tells you have to map the 0th PHY in the
> pool to an MDIO bus and address. Follow the phandle to get the MDIO
> bus and the address on the bus.

A fuller dts would be something like this (for the 8-port board I have 
in front of me)

                 mdio-controller@ca00 {
                         compatible = "realtek,rtl9301-mdio";
                         reg = <0xca00>;

                         mdio-bus@0 {
                                 reg = <0x00>;
                                 ethphy0: ethernet-phy@0 {
                                         reg = <0x00>;
                                         compatible = 
"ethernet-phy-ieee802.3-c45";
                                 };
                                 ethphy1: ethernet-phy@1 {
                                         reg = <0x01>;
                                         compatible = 
"ethernet-phy-ieee802.3-c45";
                                 };
                                 ethphy2: ethernet-phy@2 {
                                         reg = <0x02>;
                                         compatible = 
"ethernet-phy-ieee802.3-c45";
                                 };
                                 ethphy3: ethernet-phy@3 {
                                         reg = <0x03>;
                                         compatible = 
"ethernet-phy-ieee802.3-c45";
                                 };
                         };
                         mdio-bus@1 {
                                 reg = <0x01>;
                                 ethphy4: ethernet-phy@8 {
                                         reg = <0x00>;
                                         compatible = 
"ethernet-phy-ieee802.3-c45";
                                 };
                                 ethphy5: ethernet-phy@9 {
                                         reg = <0x01>;
                                         compatible = 
"ethernet-phy-ieee802.3-c45";
                                 };
                                 ethphy6: ethernet-phy@10 {
                                         reg = <0x02>;
                                         compatible = 
"ethernet-phy-ieee802.3-c45";
                                 };
                                 ethphy7: ethernet-phy@11 {
                                         reg = <0x03>;
                                         compatible = 
"ethernet-phy-ieee802.3-c45";
                                 };
                         };
                         realtek,ports = [&ethphy0, &ethphy1, &ethphy2, 
&ethphy3,
                                                    /* need a gap here 
as there are 4 unused ports*/
                                                    0, 0, 0, 0,
                                                    &ethphy4, &ethphy5, 
&ethphy6, &ethphy7];
                 };

I could probably make it work but I'm not sure that it is any more 
understandable.

[PATCH v4 2/4] dt-bindings: mfd: Add MDIO interface to rtl9301-switch

[Chris Packham]

The MDIO controller is part of the switch on the RTL9300 family of
devices. Add a $ref to the mfd binding for these devices.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Notes:
    Changes in v4:
    - There is a single MDIO controller that has MDIO buses as children
    Changes in v3:
    - None
    Changes in v2:
    - None

 .../bindings/mfd/realtek,rtl9301-switch.yaml  | 24 +++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/realtek,rtl9301-switch.yaml b/Documentation/devicetree/bindings/mfd/realtek,rtl9301-switch.yaml
index f053303ab1e6..c19d2c209434 100644
--- a/Documentation/devicetree/bindings/mfd/realtek,rtl9301-switch.yaml
+++ b/Documentation/devicetree/bindings/mfd/realtek,rtl9301-switch.yaml
@@ -28,6 +28,9 @@ properties:
   reg:
     maxItems: 1
 
+  mdio-controller:
+    $ref: /schemas/net/realtek,rtl9301-mdio.yaml#
+
   '#address-cells':
     const: 1
 
@@ -110,5 +113,26 @@ examples:
           };
         };
       };
+
+      mdio-controller {
+        compatible = "realtek,rtl9301-mdio";
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        mdio-bus@0 {
+          reg = <0>;
+          #address-cells = <1>;
+          #size-cells = <0>;
+
+          ethernet-phy@0 {
+            reg = <0>;
+            realtek,port = <1>;
+          };
+          ethernet-phy@1 {
+            reg = <1>;
+            realtek,port = <0>;
+          };
+        };
+      };
     };
 
-- 
2.47.1

Re: [PATCH v4 2/4] dt-bindings: mfd: Add MDIO interface to rtl9301-switch

[Krzysztof Kozlowski]

On Mon, Jan 20, 2025 at 05:02:12PM +1300, Chris Packham wrote:
> The MDIO controller is part of the switch on the RTL9300 family of
> devices. Add a $ref to the mfd binding for these devices.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> 

You need to explain merging dependencies. Nothing in cover letter,
nothing here, but this *CANNOT* be merged independently.

> Notes:
>     Changes in v4:
>     - There is a single MDIO controller that has MDIO buses as children
>     Changes in v3:
>     - None
>     Changes in v2:
>     - None
> 
>  .../bindings/mfd/realtek,rtl9301-switch.yaml  | 24 +++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/realtek,rtl9301-switch.yaml b/Documentation/devicetree/bindings/mfd/realtek,rtl9301-switch.yaml
> index f053303ab1e6..c19d2c209434 100644
> --- a/Documentation/devicetree/bindings/mfd/realtek,rtl9301-switch.yaml
> +++ b/Documentation/devicetree/bindings/mfd/realtek,rtl9301-switch.yaml
> @@ -28,6 +28,9 @@ properties:
>    reg:
>      maxItems: 1
>  
> +  mdio-controller:
> +    $ref: /schemas/net/realtek,rtl9301-mdio.yaml#
> +
>    '#address-cells':
>      const: 1
>  
> @@ -110,5 +113,26 @@ examples:
>            };
>          };
>        };
> +
> +      mdio-controller {

No, no resources here, no unit address. Look at other nodes - they have
the resource, the address. Mixing such nodes is clear indication this is
not correct hardware description and you do this only for Linux.

Fold child device into parent.

Best regards,
Krzysztof

Re: [PATCH v4 2/4] dt-bindings: mfd: Add MDIO interface to rtl9301-switch

[Chris Packham]

On 22/01/2025 21:14, Krzysztof Kozlowski wrote:
> On Mon, Jan 20, 2025 at 05:02:12PM +1300, Chris Packham wrote:
>> The MDIO controller is part of the switch on the RTL9300 family of
>> devices. Add a $ref to the mfd binding for these devices.
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> ---
>>
> You need to explain merging dependencies. Nothing in cover letter,
> nothing here, but this *CANNOT* be merged independently.
OK. I'll make sure to add a note here and to the series cover letter.
>> Notes:
>>      Changes in v4:
>>      - There is a single MDIO controller that has MDIO buses as children
>>      Changes in v3:
>>      - None
>>      Changes in v2:
>>      - None
>>
>>   .../bindings/mfd/realtek,rtl9301-switch.yaml  | 24 +++++++++++++++++++
>>   1 file changed, 24 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/realtek,rtl9301-switch.yaml b/Documentation/devicetree/bindings/mfd/realtek,rtl9301-switch.yaml
>> index f053303ab1e6..c19d2c209434 100644
>> --- a/Documentation/devicetree/bindings/mfd/realtek,rtl9301-switch.yaml
>> +++ b/Documentation/devicetree/bindings/mfd/realtek,rtl9301-switch.yaml
>> @@ -28,6 +28,9 @@ properties:
>>     reg:
>>       maxItems: 1
>>   
>> +  mdio-controller:
>> +    $ref: /schemas/net/realtek,rtl9301-mdio.yaml#
>> +
>>     '#address-cells':
>>       const: 1
>>   
>> @@ -110,5 +113,26 @@ examples:
>>             };
>>           };
>>         };
>> +
>> +      mdio-controller {
> No, no resources here, no unit address. Look at other nodes - they have
> the resource, the address. Mixing such nodes is clear indication this is
> not correct hardware description and you do this only for Linux.
>
> Fold child device into parent.

In this particular case all the mdio stuff is actually contained to a 
range starting at offset 0xca00. I dropped it because it was simpler in 
the driver to use the full 16-bit address rather than trying to use 
offsets from the base address that didn't correspond to the datasheet. 
As you've highlighted that's making the dt-binding impose driver 
specifics so would adding back `mdio-controller@ca00` and `reg = <0xca00 
0x200>;` be OK even if the driver doesn't actually use them?

>
> Best regards,
> Krzysztof
>

Re: [PATCH v4 2/4] dt-bindings: mfd: Add MDIO interface to rtl9301-switch

[Krzysztof Kozlowski]

On 22/01/2025 21:53, Chris Packham wrote:
>>>         };
>>> +
>>> +      mdio-controller {
>> No, no resources here, no unit address. Look at other nodes - they have
>> the resource, the address. Mixing such nodes is clear indication this is
>> not correct hardware description and you do this only for Linux.
>>
>> Fold child device into parent.
> 
> In this particular case all the mdio stuff is actually contained to a 
> range starting at offset 0xca00. I dropped it because it was simpler in 
> the driver to use the full 16-bit address rather than trying to use 
> offsets from the base address that didn't correspond to the datasheet. 
> As you've highlighted that's making the dt-binding impose driver 
> specifics so would adding back `mdio-controller@ca00` and `reg = <0xca00 
> 0x200>;` be OK even if the driver doesn't actually use them?


If this matches the hardware, then yes.

Best regards,
Krzysztof

[PATCH v4 3/4] mips: dts: realtek: Add MDIO controller

[Chris Packham]

Add a device tree node for the MDIO controller on the RTL9300 chips.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Notes:
    Changes in v4:
    - Have a single mdio-controller with the individual buses as child
      nodes
    Changes in v3:
    - None
    Changes in v2:
    - None

 arch/mips/boot/dts/realtek/rtl930x.dtsi | 32 +++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/arch/mips/boot/dts/realtek/rtl930x.dtsi b/arch/mips/boot/dts/realtek/rtl930x.dtsi
index f2e57ea3a60c..8410411fbba6 100644
--- a/arch/mips/boot/dts/realtek/rtl930x.dtsi
+++ b/arch/mips/boot/dts/realtek/rtl930x.dtsi
@@ -69,6 +69,38 @@ i2c1: i2c@388 {
 			#size-cells = <0>;
 			status = "disabled";
 		};
+
+		mdio_controller: mdio-controller {
+			compatible = "realtek,rtl9301-mdio";
+			#address-cells = <1>;
+			#size-cells = <0>;
+			status = "disabled";
+
+			mdio0: mdio-bus@0 {
+				reg = <0>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+				status = "disabled";
+			};
+			mdio1: mdio-bus@1 {
+				reg = <1>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+				status = "disabled";
+			};
+			mdio2: mdio-bus@2 {
+				reg = <2>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+				status = "disabled";
+			};
+			mdio3: mdio-bus@3 {
+				reg = <3>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+				status = "disabled";
+			};
+		};
 	};
 
 	soc: soc@18000000 {
-- 
2.47.1

[PATCH v4 4/4] net: mdio: Add RTL9300 MDIO driver

[Chris Packham]

Add a driver for the MDIO controller on the RTL9300 family of Ethernet
switches with integrated SoC. There are 4 physical SMI interfaces on the
RTL9300 however access is done using the switch ports. The driver takes
the MDIO bus hierarchy from the DTS and uses this to configure the
switch ports so they are associated with the correct PHY. This mapping
is also used when dealing with software requests from phylib.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Notes:
    Changes in v4:
    - rename to realtek-rtl9300
    - s/realtek_/rtl9300_/
    - add locking to support concurrent access
    - The dtbinding now represents the MDIO bus hierarchy so we consume this
      information and use it to configure the switch port to MDIO bus+addr.
    Changes in v3:
    - Fix (another) off-by-one error
    Changes in v2:
    - Add clause 22 support
    - Remove commented out code
    - Formatting cleanup
    - Set MAX_PORTS correctly for MDIO interface
    - Fix off-by-one error in pn check

 drivers/net/mdio/Kconfig                |   7 +
 drivers/net/mdio/Makefile               |   1 +
 drivers/net/mdio/mdio-realtek-rtl9300.c | 417 ++++++++++++++++++++++++
 3 files changed, 425 insertions(+)
 create mode 100644 drivers/net/mdio/mdio-realtek-rtl9300.c

diff --git a/drivers/net/mdio/Kconfig b/drivers/net/mdio/Kconfig
index 4a7a303be2f7..058fcdaf6c18 100644
--- a/drivers/net/mdio/Kconfig
+++ b/drivers/net/mdio/Kconfig
@@ -185,6 +185,13 @@ config MDIO_IPQ8064
 	  This driver supports the MDIO interface found in the network
 	  interface units of the IPQ8064 SoC
 
+config MDIO_REALTEK_RTL9300
+	tristate "Realtek RTL9300 MDIO interface support"
+	depends on MACH_REALTEK_RTL || COMPILE_TEST
+	help
+	  This driver supports the MDIO interface found in the Realtek
+	  RTL9300 family of Ethernet switches with integrated SoC.
+
 config MDIO_REGMAP
 	tristate
 	help
diff --git a/drivers/net/mdio/Makefile b/drivers/net/mdio/Makefile
index 1015f0db4531..c23778e73890 100644
--- a/drivers/net/mdio/Makefile
+++ b/drivers/net/mdio/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_MDIO_MOXART)		+= mdio-moxart.o
 obj-$(CONFIG_MDIO_MSCC_MIIM)		+= mdio-mscc-miim.o
 obj-$(CONFIG_MDIO_MVUSB)		+= mdio-mvusb.o
 obj-$(CONFIG_MDIO_OCTEON)		+= mdio-octeon.o
+obj-$(CONFIG_MDIO_REALTEK_RTL9300)	+= mdio-realtek-rtl9300.o
 obj-$(CONFIG_MDIO_REGMAP)		+= mdio-regmap.o
 obj-$(CONFIG_MDIO_SUN4I)		+= mdio-sun4i.o
 obj-$(CONFIG_MDIO_THUNDER)		+= mdio-thunder.o
diff --git a/drivers/net/mdio/mdio-realtek-rtl9300.c b/drivers/net/mdio/mdio-realtek-rtl9300.c
new file mode 100644
index 000000000000..a9b894eff407
--- /dev/null
+++ b/drivers/net/mdio/mdio-realtek-rtl9300.c
@@ -0,0 +1,417 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * MDIO controller for RTL9300 switches with integrated SoC.
+ *
+ * The MDIO communication is abstracted by the switch. At the software level
+ * communication uses the switch port to address the PHY with the actual MDIO
+ * bus and address having been setup via the realtek,smi-address property.
+ */
+
+#include <linux/cleanup.h>
+#include <linux/mdio.h>
+#include <linux/mfd/syscon.h>
+#include <linux/mod_devicetable.h>
+#include <linux/mutex.h>
+#include <linux/of_mdio.h>
+#include <linux/phy.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+
+#define SMI_GLB_CTRL			0xca00
+#define   GLB_CTRL_INTF_SEL(intf)	BIT(16 + (intf))
+#define SMI_PORT0_15_POLLING_SEL	0xca08
+#define SMI_ACCESS_PHY_CTRL_0		0xcb70
+#define SMI_ACCESS_PHY_CTRL_1		0xcb74
+#define   PHY_CTRL_RWOP			BIT(2)
+#define   PHY_CTRL_TYPE			BIT(1)
+#define   PHY_CTRL_CMD			BIT(0)
+#define   PHY_CTRL_FAIL			BIT(25)
+#define SMI_ACCESS_PHY_CTRL_2		0xcb78
+#define SMI_ACCESS_PHY_CTRL_3		0xcb7c
+#define SMI_PORT0_5_ADDR_CTRL		0xcb80
+
+#define MAX_PORTS       28
+#define MAX_SMI_BUSSES  4
+#define MAX_SMI_ADDR	0x1f
+
+struct rtl9300_mdio_priv;
+
+struct rtl9300_mdio_chan {
+	struct rtl9300_mdio_priv *priv;
+	u8 smi_bus;
+};
+
+struct rtl9300_mdio_priv {
+	struct regmap *regmap;
+	struct mutex lock; /* protect HW access */
+	u8 smi_bus[MAX_PORTS];
+	u8 smi_addr[MAX_PORTS];
+	bool smi_bus_isc45[MAX_SMI_BUSSES];
+	struct mii_bus *bus[MAX_SMI_BUSSES];
+};
+
+static int rtl9300_mdio_phy_to_port(struct mii_bus *bus, int phy_id)
+{
+	struct rtl9300_mdio_chan *chan = bus->priv;
+	struct rtl9300_mdio_priv *priv = chan->priv;
+	int i;
+
+	for (i = 0; i < MAX_PORTS; i++)
+		if (priv->smi_bus[i] == chan->smi_bus &&
+		    priv->smi_addr[i] == phy_id)
+			return i;
+
+	return -ENOENT;
+}
+
+static int rtl9300_mdio_wait_ready(struct rtl9300_mdio_priv *priv)
+{
+	struct regmap *regmap = priv->regmap;
+	u32 val;
+
+	lockdep_assert_held(&priv->lock);
+
+	return regmap_read_poll_timeout(regmap, SMI_ACCESS_PHY_CTRL_1,
+					val, !(val & PHY_CTRL_CMD), 10, 1000);
+}
+
+static int rtl9300_mdio_read_c22(struct mii_bus *bus, int phy_id, int regnum)
+{
+	struct rtl9300_mdio_chan *chan = bus->priv;
+	struct rtl9300_mdio_priv *priv = chan->priv;
+	struct regmap *regmap = priv->regmap;
+	int port;
+	u32 val;
+	int err;
+
+	guard(mutex)(&priv->lock);
+
+	port = rtl9300_mdio_phy_to_port(bus, phy_id);
+	if (port < 0)
+		return port;
+
+	err = rtl9300_mdio_wait_ready(priv);
+	if (err)
+		return err;
+
+	err = regmap_write(regmap, SMI_ACCESS_PHY_CTRL_2, port << 16);
+	if (err)
+		return err;
+
+	err = regmap_write(regmap, SMI_ACCESS_PHY_CTRL_1,
+			   regnum << 20 |  0x1f << 15 | 0xfff << 3 | PHY_CTRL_CMD);
+	if (err)
+		return err;
+
+	err = rtl9300_mdio_wait_ready(priv);
+	if (err)
+		return err;
+
+	err = regmap_read(regmap, SMI_ACCESS_PHY_CTRL_2, &val);
+	if (err)
+		return err;
+
+	return val & 0xffff;
+}
+
+static int rtl9300_mdio_write_c22(struct mii_bus *bus, int phy_id, int regnum, u16 value)
+{
+	struct rtl9300_mdio_chan *chan = bus->priv;
+	struct rtl9300_mdio_priv *priv = chan->priv;
+	struct regmap *regmap = priv->regmap;
+	int port;
+	u32 val;
+	int err;
+
+	guard(mutex)(&priv->lock);
+
+	port = rtl9300_mdio_phy_to_port(bus, phy_id);
+	if (port < 0)
+		return port;
+
+	err = rtl9300_mdio_wait_ready(priv);
+	if (err)
+		return err;
+
+	err = regmap_write(regmap, SMI_ACCESS_PHY_CTRL_0, BIT(port));
+	if (err)
+		return err;
+
+	err = regmap_write(regmap, SMI_ACCESS_PHY_CTRL_2, value << 16);
+	if (err)
+		return err;
+
+	err = regmap_write(regmap, SMI_ACCESS_PHY_CTRL_1,
+			   regnum << 20 |  0x1f << 15 | 0xfff << 3 | PHY_CTRL_RWOP | PHY_CTRL_CMD);
+	if (err)
+		return err;
+
+	err = regmap_read_poll_timeout(regmap, SMI_ACCESS_PHY_CTRL_1,
+				       val, !(val & PHY_CTRL_CMD), 10, 100);
+	if (err)
+		return err;
+
+	if (val & PHY_CTRL_FAIL)
+		return -ENXIO;
+
+	return 0;
+}
+
+static int rtl9300_mdio_read_c45(struct mii_bus *bus, int phy_id, int dev_addr, int regnum)
+{
+	struct rtl9300_mdio_chan *chan = bus->priv;
+	struct rtl9300_mdio_priv *priv = chan->priv;
+	struct regmap *regmap = priv->regmap;
+	int port;
+	u32 val;
+	int err;
+
+	guard(mutex)(&priv->lock);
+
+	port = rtl9300_mdio_phy_to_port(bus, phy_id);
+	if (port < 0)
+		return port;
+
+	err = rtl9300_mdio_wait_ready(priv);
+	if (err)
+		return err;
+
+	err = regmap_write(regmap, SMI_ACCESS_PHY_CTRL_2, port << 16);
+	if (err)
+		return err;
+
+	err = regmap_write(regmap, SMI_ACCESS_PHY_CTRL_3,
+			   dev_addr << 16 | (regnum & 0xffff));
+	if (err)
+		return err;
+
+	err = regmap_write(regmap, SMI_ACCESS_PHY_CTRL_1,
+			   PHY_CTRL_TYPE | PHY_CTRL_CMD);
+	if (err)
+		return err;
+
+	err = rtl9300_mdio_wait_ready(priv);
+	if (err)
+		return err;
+
+	err = regmap_read(regmap, SMI_ACCESS_PHY_CTRL_2, &val);
+	if (err)
+		return err;
+
+	return val & 0xffff;
+}
+
+static int rtl9300_mdio_write_c45(struct mii_bus *bus, int phy_id, int dev_addr,
+				  int regnum, u16 value)
+{
+	struct rtl9300_mdio_chan *chan = bus->priv;
+	struct rtl9300_mdio_priv *priv = chan->priv;
+	struct regmap *regmap = priv->regmap;
+	int port;
+	u32 val;
+	int err;
+
+	guard(mutex)(&priv->lock);
+
+	port = rtl9300_mdio_phy_to_port(bus, phy_id);
+	if (port < 0)
+		return port;
+
+	err = rtl9300_mdio_wait_ready(priv);
+	if (err)
+		return err;
+
+	err = regmap_write(regmap, SMI_ACCESS_PHY_CTRL_0, BIT(port));
+	if (err)
+		return err;
+
+	err = regmap_write(regmap, SMI_ACCESS_PHY_CTRL_2, value << 16);
+	if (err)
+		return err;
+
+	err = regmap_write(regmap, SMI_ACCESS_PHY_CTRL_3,
+			   dev_addr << 16 | (regnum & 0xffff));
+	if (err)
+		return err;
+
+	err = regmap_write(regmap, SMI_ACCESS_PHY_CTRL_1,
+			   PHY_CTRL_RWOP | PHY_CTRL_TYPE | PHY_CTRL_CMD);
+	if (err)
+		return err;
+
+	err = regmap_read_poll_timeout(regmap, SMI_ACCESS_PHY_CTRL_1,
+				       val, !(val & PHY_CTRL_CMD), 10, 100);
+	if (err)
+		return err;
+
+	if (val & PHY_CTRL_FAIL)
+		return -ENXIO;
+
+	return 0;
+}
+
+static int rtl9300_mdiobus_init(struct rtl9300_mdio_priv *priv)
+{
+	u32 glb_ctrl_mask = 0, glb_ctrl_val = 0;
+	struct regmap *regmap = priv->regmap;
+	u32 port_addr[5] = { 0 };
+	u32 poll_sel[2] = { 0 };
+	int i, err;
+
+	/* Associate the port with the SMI interface and PHY */
+	for (i = 0; i < MAX_PORTS; i++) {
+		int pos;
+
+		if (priv->smi_bus[i] > 3)
+			continue;
+
+		pos = (i % 6) * 5;
+		port_addr[i / 6] |= priv->smi_addr[i] << pos;
+
+		pos = (i % 16) * 2;
+		poll_sel[i / 16] |= priv->smi_bus[i] << pos;
+	}
+
+	/* Put the interfaces into C45 mode if required */
+	for (i = 0; i < MAX_SMI_BUSSES; i++) {
+		if (priv->smi_bus_isc45[i]) {
+			glb_ctrl_mask |= GLB_CTRL_INTF_SEL(i);
+			glb_ctrl_val |= GLB_CTRL_INTF_SEL(i);
+		}
+	}
+
+	err = regmap_bulk_write(regmap, SMI_PORT0_5_ADDR_CTRL,
+				port_addr, 5);
+	if (err)
+		return err;
+
+	err = regmap_bulk_write(regmap, SMI_PORT0_15_POLLING_SEL,
+				poll_sel, 2);
+	if (err)
+		return err;
+
+	err = regmap_update_bits(regmap, SMI_GLB_CTRL,
+				 glb_ctrl_mask, glb_ctrl_val);
+	if (err)
+		return err;
+
+	return 0;
+}
+
+static int rtl9300_mdiobus_probe_one(struct device *dev, struct rtl9300_mdio_priv *priv,
+				     struct fwnode_handle *node)
+{
+	struct rtl9300_mdio_chan *chan;
+	struct fwnode_handle *child;
+	struct mii_bus *bus;
+	u32 smi_bus;
+	int err;
+
+	err = fwnode_property_read_u32(node, "reg", &smi_bus);
+	if (err)
+		return err;
+
+	if (smi_bus >= MAX_SMI_BUSSES)
+		return dev_err_probe(dev, -EINVAL, "illegal smi bus number %d\n", smi_bus);
+
+	fwnode_for_each_child_node(node, child) {
+		u32 smi_addr;
+		u32 pn;
+
+		err = fwnode_property_read_u32(child, "reg", &smi_addr);
+		if (err)
+			return err;
+
+		err = fwnode_property_read_u32(child, "realtek,port", &pn);
+		if (err)
+			return err;
+
+		if (pn >= MAX_PORTS)
+			return dev_err_probe(dev, -EINVAL, "illegal port number %d\n", pn);
+
+		if (fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45"))
+			priv->smi_bus_isc45[smi_bus] = true;
+
+		priv->smi_bus[pn] = smi_bus;
+		priv->smi_addr[pn] = smi_addr;
+	}
+
+	bus = devm_mdiobus_alloc_size(dev, sizeof(*chan));
+	if (!bus)
+		return -ENOMEM;
+
+	bus->name = "Reaktek Switch MDIO Bus";
+	bus->read = rtl9300_mdio_read_c22;
+	bus->write = rtl9300_mdio_write_c22;
+	bus->read_c45 = rtl9300_mdio_read_c45;
+	bus->write_c45 =  rtl9300_mdio_write_c45;
+	bus->parent = dev;
+	chan = bus->priv;
+	chan->smi_bus = smi_bus;
+	chan->priv = priv;
+
+	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-%d", dev_name(dev), smi_bus);
+
+	err = devm_of_mdiobus_register(dev, bus, to_of_node(node));
+	if (err)
+		return dev_err_probe(dev, err, "cannot register MDIO bus\n");
+
+	return 0;
+}
+
+static int rtl9300_mdiobus_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct rtl9300_mdio_priv *priv;
+	struct fwnode_handle *child;
+	int err;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	err = devm_mutex_init(dev, &priv->lock);
+	if (err)
+		return err;
+
+	priv->regmap = syscon_node_to_regmap(dev->parent->of_node);
+	if (IS_ERR(priv->regmap))
+		return PTR_ERR(priv->regmap);
+
+	platform_set_drvdata(pdev, priv);
+
+	if (device_get_child_node_count(dev) >= MAX_SMI_BUSSES)
+		return dev_err_probe(dev, -EINVAL, "Too many SMI busses\n");
+
+	device_for_each_child_node(dev, child) {
+		err = rtl9300_mdiobus_probe_one(dev, priv, child);
+		if (err)
+			return err;
+	}
+
+	err = rtl9300_mdiobus_init(priv);
+	if (err)
+		return dev_err_probe(dev, err, "failed to initialise MDIO bus controller\n");
+
+	return 0;
+}
+
+static const struct of_device_id rtl9300_mdio_ids[] = {
+	{ .compatible = "realtek,rtl9301-mdio" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, rtl9300_mdio_ids);
+
+static struct platform_driver rtl9300_mdio_driver = {
+	.probe = rtl9300_mdiobus_probe,
+	.driver = {
+		.name = "mdio-rtl9300",
+		.of_match_table = rtl9300_mdio_ids,
+	},
+};
+
+module_platform_driver(rtl9300_mdio_driver);
+
+MODULE_DESCRIPTION("RTL9300 MDIO driver");
+MODULE_LICENSE("GPL");
-- 
2.47.1

Re: [PATCH v4 4/4] net: mdio: Add RTL9300 MDIO driver

[Sander Vanheule]

Hi Chris,

On Mon, 2025-01-20 at 17:02 +1300, Chris Packham wrote:
> Add a driver for the MDIO controller on the RTL9300 family of Ethernet
> switches with integrated SoC. There are 4 physical SMI interfaces on the
> RTL9300 however access is done using the switch ports. The driver takes
> the MDIO bus hierarchy from the DTS and uses this to configure the
> switch ports so they are associated with the correct PHY. This mapping
> is also used when dealing with software requests from phylib.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---

[...]


> diff --git a/drivers/net/mdio/mdio-realtek-rtl9300.c b/drivers/net/mdio/mdio-realtek-rtl9300.c
> new file mode 100644
> index 000000000000..a9b894eff407
> --- /dev/null
> +++ b/drivers/net/mdio/mdio-realtek-rtl9300.c
> @@ -0,0 +1,417 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * MDIO controller for RTL9300 switches with integrated SoC.
> + *
> + * The MDIO communication is abstracted by the switch. At the software level
> + * communication uses the switch port to address the PHY with the actual MDIO
> + * bus and address having been setup via the realtek,smi-address property.

realtek,smi-address is a leftover from a previous spin?

> + */
> +
> +#include <linux/cleanup.h>
> +#include <linux/mdio.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/mutex.h>
> +#include <linux/of_mdio.h>
> +#include <linux/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +
> +#define SMI_GLB_CTRL			0xca00
> +#define   GLB_CTRL_INTF_SEL(intf)	BIT(16 + (intf))
> +#define SMI_PORT0_15_POLLING_SEL	0xca08
> +#define SMI_ACCESS_PHY_CTRL_0		0xcb70
> +#define SMI_ACCESS_PHY_CTRL_1		0xcb74
> +#define   PHY_CTRL_RWOP			BIT(2)

With

#define PHY_CTRL_WRITE		BIT(2)
#define PHY_CTRL_READ		0

you could use both macros in the write/read functions. Now I have to go and parse the write/read
functions to see what it means when this bit is set.

> +#define   PHY_CTRL_TYPE			BIT(1)

Similar here:
#define	PHY_CTRL_TYPE_C22	0
#define PHY_CTRL_TYPE_C45	BIT(1)

> +#define   PHY_CTRL_CMD			BIT(0)
> +#define   PHY_CTRL_FAIL			BIT(25)
> +#define SMI_ACCESS_PHY_CTRL_2		0xcb78
> +#define SMI_ACCESS_PHY_CTRL_3		0xcb7c
> +#define SMI_PORT0_5_ADDR_CTRL		0xcb80
> +
> +#define MAX_PORTS       28
> +#define MAX_SMI_BUSSES  4
> +#define MAX_SMI_ADDR	0x1f
> +
> +struct rtl9300_mdio_priv;
> +
> +struct rtl9300_mdio_chan {
> +	struct rtl9300_mdio_priv *priv;
> +	u8 smi_bus;
> +};
> +
> +struct rtl9300_mdio_priv {
> +	struct regmap *regmap;
> +	struct mutex lock; /* protect HW access */
> +	u8 smi_bus[MAX_PORTS];
> +	u8 smi_addr[MAX_PORTS];
> +	bool smi_bus_isc45[MAX_SMI_BUSSES];
Nit: add an underscore: smi_bus_is_c45

> +	struct mii_bus *bus[MAX_SMI_BUSSES];
> +};
> +
> +static int rtl9300_mdio_phy_to_port(struct mii_bus *bus, int phy_id)
> +{
> +	struct rtl9300_mdio_chan *chan = bus->priv;
> +	struct rtl9300_mdio_priv *priv = chan->priv;
> +	int i;
> +
> +	for (i = 0; i < MAX_PORTS; i++)
> +		if (priv->smi_bus[i] == chan->smi_bus &&
> +		    priv->smi_addr[i] == phy_id)
> +			return i;

This may break if some lower port numbers are not configured by the user, e.g. phy 0-7 on bus 0 are
mapped to ports 8-15 and ports 0-7 are unused.
When looking up the port number of phy 0 on bus 0, you would get a match on an unconfigured port
(port 0) since smi_bus/smi_addr are zero-initialized. This could be fixed by adding a bitmap
indicating which ports are actually configured.

> +
> +	return -ENOENT;
> +}

[...]

> +static int rtl9300_mdio_read_c22(struct mii_bus *bus, int phy_id, int regnum)
> +{

[...]

> +	err = regmap_write(regmap, SMI_ACCESS_PHY_CTRL_1,
> +			   regnum << 20 |  0x1f << 15 | 0xfff << 3 | PHY_CTRL_CMD);

You could use FIELD_PREP() to pack the bitfields.

> +	if (err)
> +		return err;
> +
> +	err = rtl9300_mdio_wait_ready(priv);
> +	if (err)
> +		return err;
> +
> +	err = regmap_read(regmap, SMI_ACCESS_PHY_CTRL_2, &val);
> +	if (err)
> +		return err;
> +
> +	return val & 0xffff;

... and FIELD_GET() to unpack.

> +}
> +

[...]

> +
> +static int rtl9300_mdiobus_init(struct rtl9300_mdio_priv *priv)
> +{
> +	u32 glb_ctrl_mask = 0, glb_ctrl_val = 0;
> +	struct regmap *regmap = priv->regmap;
> +	u32 port_addr[5] = { 0 };
> +	u32 poll_sel[2] = { 0 };
> +	int i, err;
> +
> +	/* Associate the port with the SMI interface and PHY */
> +	for (i = 0; i < MAX_PORTS; i++) {
> +		int pos;
> +
> +		if (priv->smi_bus[i] > 3)
> +			continue;
> +
> +		pos = (i % 6) * 5;
> +		port_addr[i / 6] |= priv->smi_addr[i] << pos;
> +
> +		pos = (i % 16) * 2;
> +		poll_sel[i / 16] |= priv->smi_bus[i] << pos;

I've read the discussion on v1-v3 and had a quick look at the available documentation. Thinking out
loud here, so you can correct me if I'm making any false assumptions.

As I understand, the SoC has four physical MDIO/MDC pin pairs. Using the DT description, phy-s are
matched with to specific MDIO bus. PORT_ADDR tells the switch which phy address a port maps to.
POLL_SEL then tells the MDIO controller which bus this port (phy) is assigned to. I have the
impression this [port] <-> [bus, phy] mapping is completely arbitrary. If configured correctly, it
can probably serve as a convenience to match a front panel port number to a specific phy.

If the port numbers are in fact arbitrary, I think they could be hidden from the user, removing the
need for a custom DT property. You could probably populate the port numbers one by one as phy-s are
enumerated, as you are already storing the assigned port number in the MDIO controller's private
data.

One complication this might have, is that I suspect these port numbers are not used exclusively by
the MDIO controller, but also by the switch itself. So then there may have to be a way to resolve
(auto-assigned) port numbers outside of this driver too.

> +	}
> +
> +	/* Put the interfaces into C45 mode if required */
> +	for (i = 0; i < MAX_SMI_BUSSES; i++) {
> +		if (priv->smi_bus_isc45[i]) {
> +			glb_ctrl_mask |= GLB_CTRL_INTF_SEL(i);

Can't glb_ctrl_mask be fixed to GENMASK(19, 16)?

> +			glb_ctrl_val |= GLB_CTRL_INTF_SEL(i);
> +		}
> +	}

[...]

> +static int rtl9300_mdiobus_probe_one(struct device *dev, struct rtl9300_mdio_priv *priv,
> +				     struct fwnode_handle *node)
> +{
> +	struct rtl9300_mdio_chan *chan;
> +	struct fwnode_handle *child;
> +	struct mii_bus *bus;
> +	u32 smi_bus;
> +	int err;
> +
> +	err = fwnode_property_read_u32(node, "reg", &smi_bus);
> +	if (err)
> +		return err;
> +
> +	if (smi_bus >= MAX_SMI_BUSSES)
> +		return dev_err_probe(dev, -EINVAL, "illegal smi bus number %d\n", smi_bus);
> +
> +	fwnode_for_each_child_node(node, child) {
> +		u32 smi_addr;
> +		u32 pn;
> +
> +		err = fwnode_property_read_u32(child, "reg", &smi_addr);
> +		if (err)
> +			return err;

[...]

> +
> +		priv->smi_bus[pn] = smi_bus;
> +		priv->smi_addr[pn] = smi_addr;

Nitpicking a bit here, but perhaps the code might be a tad bit easier to read for the non-Realtek
initiated by renaming:
  - smi_bus -> mdio_bus or just bus_id (matching the mii_bus *bus member)
  - smi_addr -> phy_addr

> +	}

[...]

> +static int rtl9300_mdiobus_probe(struct platform_device *pdev)
> +{

[...]

> +
> +	if (device_get_child_node_count(dev) >= MAX_SMI_BUSSES)
> +		return dev_err_probe(dev, -EINVAL, "Too many SMI busses\n");

This seems redundant with the MAX_SMI_BUSSES comparison in probe_one().

Best,
Sander

Re: [PATCH v4 4/4] net: mdio: Add RTL9300 MDIO driver

[Chris Packham]

Hi Sander,

On 20/01/2025 23:28, Sander Vanheule wrote:
> Hi Chris,
>
> On Mon, 2025-01-20 at 17:02 +1300, Chris Packham wrote:
>> Add a driver for the MDIO controller on the RTL9300 family of Ethernet
>> switches with integrated SoC. There are 4 physical SMI interfaces on the
>> RTL9300 however access is done using the switch ports. The driver takes
>> the MDIO bus hierarchy from the DTS and uses this to configure the
>> switch ports so they are associated with the correct PHY. This mapping
>> is also used when dealing with software requests from phylib.
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> ---
> [...]
>
>
>> diff --git a/drivers/net/mdio/mdio-realtek-rtl9300.c b/drivers/net/mdio/mdio-realtek-rtl9300.c
>> new file mode 100644
>> index 000000000000..a9b894eff407
>> --- /dev/null
>> +++ b/drivers/net/mdio/mdio-realtek-rtl9300.c
>> @@ -0,0 +1,417 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * MDIO controller for RTL9300 switches with integrated SoC.
>> + *
>> + * The MDIO communication is abstracted by the switch. At the software level
>> + * communication uses the switch port to address the PHY with the actual MDIO
>> + * bus and address having been setup via the realtek,smi-address property.
> realtek,smi-address is a leftover from a previous spin?
Oops, will fix
>
>> + */
>> +
>> +#include <linux/cleanup.h>
>> +#include <linux/mdio.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/mutex.h>
>> +#include <linux/of_mdio.h>
>> +#include <linux/phy.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/property.h>
>> +#include <linux/regmap.h>
>> +
>> +#define SMI_GLB_CTRL			0xca00
>> +#define   GLB_CTRL_INTF_SEL(intf)	BIT(16 + (intf))
>> +#define SMI_PORT0_15_POLLING_SEL	0xca08
>> +#define SMI_ACCESS_PHY_CTRL_0		0xcb70
>> +#define SMI_ACCESS_PHY_CTRL_1		0xcb74
>> +#define   PHY_CTRL_RWOP			BIT(2)
> With
>
> #define PHY_CTRL_WRITE		BIT(2)
> #define PHY_CTRL_READ		0
>
> you could use both macros in the write/read functions. Now I have to go and parse the write/read
> functions to see what it means when this bit is set.
Ack.
>> +#define   PHY_CTRL_TYPE			BIT(1)
> Similar here:
> #define	PHY_CTRL_TYPE_C22	0
> #define PHY_CTRL_TYPE_C45	BIT(1)

Ack

>> +#define   PHY_CTRL_CMD			BIT(0)
>> +#define   PHY_CTRL_FAIL			BIT(25)
>> +#define SMI_ACCESS_PHY_CTRL_2		0xcb78
>> +#define SMI_ACCESS_PHY_CTRL_3		0xcb7c
>> +#define SMI_PORT0_5_ADDR_CTRL		0xcb80
>> +
>> +#define MAX_PORTS       28
>> +#define MAX_SMI_BUSSES  4
>> +#define MAX_SMI_ADDR	0x1f
>> +
>> +struct rtl9300_mdio_priv;
>> +
>> +struct rtl9300_mdio_chan {
>> +	struct rtl9300_mdio_priv *priv;
>> +	u8 smi_bus;
>> +};
>> +
>> +struct rtl9300_mdio_priv {
>> +	struct regmap *regmap;
>> +	struct mutex lock; /* protect HW access */
>> +	u8 smi_bus[MAX_PORTS];
>> +	u8 smi_addr[MAX_PORTS];
>> +	bool smi_bus_isc45[MAX_SMI_BUSSES];
> Nit: add an underscore: smi_bus_is_c45

Ack

>
>> +	struct mii_bus *bus[MAX_SMI_BUSSES];
>> +};
>> +
>> +static int rtl9300_mdio_phy_to_port(struct mii_bus *bus, int phy_id)
>> +{
>> +	struct rtl9300_mdio_chan *chan = bus->priv;
>> +	struct rtl9300_mdio_priv *priv = chan->priv;
>> +	int i;
>> +
>> +	for (i = 0; i < MAX_PORTS; i++)
>> +		if (priv->smi_bus[i] == chan->smi_bus &&
>> +		    priv->smi_addr[i] == phy_id)
>> +			return i;
> This may break if some lower port numbers are not configured by the user, e.g. phy 0-7 on bus 0 are
> mapped to ports 8-15 and ports 0-7 are unused.
> When looking up the port number of phy 0 on bus 0, you would get a match on an unconfigured port
> (port 0) since smi_bus/smi_addr are zero-initialized. This could be fixed by adding a bitmap
> indicating which ports are actually configured.
Yes that makes sense.
>
>> +
>> +	return -ENOENT;
>> +}
> [...]
>
>> +static int rtl9300_mdio_read_c22(struct mii_bus *bus, int phy_id, int regnum)
>> +{
> [...]
>
>> +	err = regmap_write(regmap, SMI_ACCESS_PHY_CTRL_1,
>> +			   regnum << 20 |  0x1f << 15 | 0xfff << 3 | PHY_CTRL_CMD);
> You could use FIELD_PREP() to pack the bitfields.
Ack.
>
>> +	if (err)
>> +		return err;
>> +
>> +	err = rtl9300_mdio_wait_ready(priv);
>> +	if (err)
>> +		return err;
>> +
>> +	err = regmap_read(regmap, SMI_ACCESS_PHY_CTRL_2, &val);
>> +	if (err)
>> +		return err;
>> +
>> +	return val & 0xffff;
> ... and FIELD_GET() to unpack.
Not sure it buys us much in this case since it's just the lower 16 bits 
but for symmetry and a little extra type checking may as well.
>
>> +}
>> +
> [...]
>
>> +
>> +static int rtl9300_mdiobus_init(struct rtl9300_mdio_priv *priv)
>> +{
>> +	u32 glb_ctrl_mask = 0, glb_ctrl_val = 0;
>> +	struct regmap *regmap = priv->regmap;
>> +	u32 port_addr[5] = { 0 };
>> +	u32 poll_sel[2] = { 0 };
>> +	int i, err;
>> +
>> +	/* Associate the port with the SMI interface and PHY */
>> +	for (i = 0; i < MAX_PORTS; i++) {
>> +		int pos;
>> +
>> +		if (priv->smi_bus[i] > 3)
>> +			continue;
>> +
>> +		pos = (i % 6) * 5;
>> +		port_addr[i / 6] |= priv->smi_addr[i] << pos;
>> +
>> +		pos = (i % 16) * 2;
>> +		poll_sel[i / 16] |= priv->smi_bus[i] << pos;
> I've read the discussion on v1-v3 and had a quick look at the available documentation. Thinking out
> loud here, so you can correct me if I'm making any false assumptions.
>
> As I understand, the SoC has four physical MDIO/MDC pin pairs. Using the DT description, phy-s are
> matched with to specific MDIO bus. PORT_ADDR tells the switch which phy address a port maps to.
> POLL_SEL then tells the MDIO controller which bus this port (phy) is assigned to. I have the
> impression this [port] <-> [bus, phy] mapping is completely arbitrary. If configured correctly, it
> can probably serve as a convenience to match a front panel port number to a specific phy.
>
> If the port numbers are in fact arbitrary, I think they could be hidden from the user, removing the
> need for a custom DT property. You could probably populate the port numbers one by one as phy-s are
> enumerated, as you are already storing the assigned port number in the MDIO controller's private
> data.
>
> One complication this might have, is that I suspect these port numbers are not used exclusively by
> the MDIO controller, but also by the switch itself. So then there may have to be a way to resolve
> (auto-assigned) port numbers outside of this driver too.

I believe the POLL_SEL configuration actually affects an internal port 
polling unit. From the datasheets I have it seems pretty configurable, 
you can tell it which phy registers to poll and what values indicate 
link up/down (the defaults are conveniently setup to match the Realtek 
PHYs). So I don't think they are arbitrary and I don't think it would be 
a good idea to change them on the fly. I did consider at one point 
finding an unused port and re-mapping that to the desired bus/addr on 
the fly but I'm not sure what that'd do to the PPU and there's no 
guarantee that there will be a unused port.

>> +	}
>> +
>> +	/* Put the interfaces into C45 mode if required */
>> +	for (i = 0; i < MAX_SMI_BUSSES; i++) {
>> +		if (priv->smi_bus_isc45[i]) {
>> +			glb_ctrl_mask |= GLB_CTRL_INTF_SEL(i);
> Can't glb_ctrl_mask be fixed to GENMASK(19, 16)?
I guess it could be. Doing it this way avoids undoing things that may 
have been set by an earlier boot stage but even as I type that I don't 
find it a good argument against GENMASK(19, 16).
>
>> +			glb_ctrl_val |= GLB_CTRL_INTF_SEL(i);
>> +		}
>> +	}
> [...]
>
>> +static int rtl9300_mdiobus_probe_one(struct device *dev, struct rtl9300_mdio_priv *priv,
>> +				     struct fwnode_handle *node)
>> +{
>> +	struct rtl9300_mdio_chan *chan;
>> +	struct fwnode_handle *child;
>> +	struct mii_bus *bus;
>> +	u32 smi_bus;
>> +	int err;
>> +
>> +	err = fwnode_property_read_u32(node, "reg", &smi_bus);
>> +	if (err)
>> +		return err;
>> +
>> +	if (smi_bus >= MAX_SMI_BUSSES)
>> +		return dev_err_probe(dev, -EINVAL, "illegal smi bus number %d\n", smi_bus);
>> +
>> +	fwnode_for_each_child_node(node, child) {
>> +		u32 smi_addr;
>> +		u32 pn;
>> +
>> +		err = fwnode_property_read_u32(child, "reg", &smi_addr);
>> +		if (err)
>> +			return err;
> [...]
>
>> +
>> +		priv->smi_bus[pn] = smi_bus;
>> +		priv->smi_addr[pn] = smi_addr;
> Nitpicking a bit here, but perhaps the code might be a tad bit easier to read for the non-Realtek
> initiated by renaming:
>    - smi_bus -> mdio_bus or just bus_id (matching the mii_bus *bus member)
>    - smi_addr -> phy_addr
I'll consider that. Certainly `u32 smi_bus` and `u32 smi_addr` can be 
renamed to match their usage from the dts. Not so sure about 
priv->smi_bus/priv->smi_addr as I am trying to match the usage in the 
datasheet. I guess technically they should be smi_set and port_addr but 
I find "port" here particularly confusing.
>> +	}
> [...]
>
>> +static int rtl9300_mdiobus_probe(struct platform_device *pdev)
>> +{
> [...]
>
>> +
>> +	if (device_get_child_node_count(dev) >= MAX_SMI_BUSSES)
>> +		return dev_err_probe(dev, -EINVAL, "Too many SMI busses\n");
> This seems redundant with the MAX_SMI_BUSSES comparison in probe_one().
The check in probe_one() checks that the mdio bus number is valid 
whereas this checks that there are at most 4. So not totally redundant 
but could probably be removed without doing any harm.
>
> Best,
> Sander

Re: [PATCH v4 4/4] net: mdio: Add RTL9300 MDIO driver

[Andrew Lunn]

> I believe the POLL_SEL configuration actually affects an internal port
> polling unit. From the datasheets I have it seems pretty configurable, you
> can tell it which phy registers to poll and what values indicate link
> up/down (the defaults are conveniently setup to match the Realtek PHYs).

You need to disable this. The linux PHY driver is driving the PHY, and
the hardware has no idea what Linux is doing. Say the driver has
changed the page to read a temperature sensor, when the switch does a
poll. Rather than reading the link status, it gets some random value
from the page containing the temperature sensor.

	Andrew

Re: [PATCH v4 4/4] net: mdio: Add RTL9300 MDIO driver

[Chris Packham]

Hi Andrew,

On 23/01/2025 10:47, Andrew Lunn wrote:
>> I believe the POLL_SEL configuration actually affects an internal port
>> polling unit. From the datasheets I have it seems pretty configurable, you
>> can tell it which phy registers to poll and what values indicate link
>> up/down (the defaults are conveniently setup to match the Realtek PHYs).
> You need to disable this. The linux PHY driver is driving the PHY, and
> the hardware has no idea what Linux is doing. Say the driver has
> changed the page to read a temperature sensor, when the switch does a
> poll. Rather than reading the link status, it gets some random value
> from the page containing the temperature sensor.

There's a mask that can be set via a register that can disable polling 
for a port. The trick will be deciding when to do so.

For C45 PHYs I think it's fine as the register space is so large that 
paging isn't really used (the only time I've seen it is in the vendor 
MMD). The PPU does seem to have some knowledge of paging for C22 but as 
far as I understand the page select register varies vendor to vendor and 
I can't see any way of telling it so it probably just uses whatever page 
select register that realtek use in their PHYs.

So I _think_ the PPU is OK. What might be a bit more tricky is the other 
way round where Linux is doing something involving changing pages and 
reading registers, there would need to be some kind mechanism that masks 
the port out of the PPU for the duration of the whole transaction.


>
> 	Andrew

Re: [PATCH v4 4/4] net: mdio: Add RTL9300 MDIO driver

[Andrew Lunn]

On Wed, Jan 22, 2025 at 11:02:14PM +0000, Chris Packham wrote:
> Hi Andrew,
> 
> On 23/01/2025 10:47, Andrew Lunn wrote:
> >> I believe the POLL_SEL configuration actually affects an internal port
> >> polling unit. From the datasheets I have it seems pretty configurable, you
> >> can tell it which phy registers to poll and what values indicate link
> >> up/down (the defaults are conveniently setup to match the Realtek PHYs).
> > You need to disable this. The linux PHY driver is driving the PHY, and
> > the hardware has no idea what Linux is doing. Say the driver has
> > changed the page to read a temperature sensor, when the switch does a
> > poll. Rather than reading the link status, it gets some random value
> > from the page containing the temperature sensor.
> 
> There's a mask that can be set via a register that can disable polling 
> for a port. The trick will be deciding when to do so.

On probe. And leave is disabled. phylink will provide you with all the
information you need about link up, what the link speed is etc. There
is no need for PPU.

	Andrew

Re: [PATCH v4 4/4] net: mdio: Add RTL9300 MDIO driver

[Chris Packham]

On 23/01/2025 16:41, Andrew Lunn wrote:
> On Wed, Jan 22, 2025 at 11:02:14PM +0000, Chris Packham wrote:
>> Hi Andrew,
>>
>> On 23/01/2025 10:47, Andrew Lunn wrote:
>>>> I believe the POLL_SEL configuration actually affects an internal port
>>>> polling unit. From the datasheets I have it seems pretty configurable, you
>>>> can tell it which phy registers to poll and what values indicate link
>>>> up/down (the defaults are conveniently setup to match the Realtek PHYs).
>>> You need to disable this. The linux PHY driver is driving the PHY, and
>>> the hardware has no idea what Linux is doing. Say the driver has
>>> changed the page to read a temperature sensor, when the switch does a
>>> poll. Rather than reading the link status, it gets some random value
>>> from the page containing the temperature sensor.
>> There's a mask that can be set via a register that can disable polling
>> for a port. The trick will be deciding when to do so.
> On probe. And leave is disabled. phylink will provide you with all the
> information you need about link up, what the link speed is etc. There
> is no need for PPU.

Works for me :P.