Sorting out the RTL9300 dt-bindings

Sorting out the RTL9300 dt-bindings

[Chris Packham]

Hi,

As Krzysztof points out in [1] I seem to have made a bit of a mess with 
the mfd binding for the RTL9300 Ethernet switch with integrated CPU. I'm 
spinning up this email thread separately so as not to unnecessarily spam 
the netdev folks and to maybe appease google so it doesn't automatically 
get flagged as junk.

First off sorry for not providing a more complete binding initially, 
Krzysztof suggested doing so a few times but I was concentrating on 
landing the drivers.

The RTL9300 has these basic blocks:
- rtl9300
   |- cpu@0 - mips34kc
   |- soc@18000000
     |- intc
     |- spi-nor
     |- spi-nand
     |- timer
     |- gpio
     `- uart
   `- switch@1b000000
      |- ethernet-ports
      |- mdio
      |- i2c
      |- reset
      `- led/gpio

The CPU/soc can be disabled and the switch managed by an external CPU 
(register access over SPI I think, the docs are a bit vague).

I think I probably inferred too much from mfd/mscc,ocelot.yaml when I 
created mfd/realtek,rtl9301-switch.yaml.

I still think the switch@1b000000 needs to be "syscon", "simple-mfd" as 
that's how the reset and i2c blocks work and they're pretty independent 
of everything else.

I've currently described the mdio interface as a device on a simple bus 
although it could just be handled as a descendant of the switch block 
that a driver looks up as a child node (I've tried to keep the mdio 
driver independent for now but that's an implementation detail). It does 
need to reach out to the ethernet-ports to figure out the mapping of 
port to phy so it isn't independent.

I see a couple of paths forward
- keep adding the switch stuff to the mfd/realtek,rtl9301-switch.yaml
- move mfd/realtek,rtl9301-switch.yaml to net/realtek,rtl9301-switch.yaml
- keep mfd/realtek,rtl9301-switch.yaml with the i2c and reboot but have 
a $ref to a new binding under net/ (not sure what that'd look like).

There's only one in-tree user of this so I don't think we need to be too 
concerned about backwards compatibility. Downstream openwrt handles 
these devices way differently already.

Thanks,
Chris

--

[1] - 
https://lore.kernel.org/lkml/20250204-eccentric-deer-of-felicity-02b7ee@krzk-bin/

Re: Sorting out the RTL9300 dt-bindings

[Krzysztof Kozlowski]

On 04/02/2025 23:39, Chris Packham wrote:
> Hi,
> 
> As Krzysztof points out in [1] I seem to have made a bit of a mess with 
> the mfd binding for the RTL9300 Ethernet switch with integrated CPU. I'm 
> spinning up this email thread separately so as not to unnecessarily spam 
> the netdev folks and to maybe appease google so it doesn't automatically 
> get flagged as junk.
> 
> First off sorry for not providing a more complete binding initially, 
> Krzysztof suggested doing so a few times but I was concentrating on 
> landing the drivers.
> 
> The RTL9300 has these basic blocks:
> - rtl9300
>    |- cpu@0 - mips34kc
>    |- soc@18000000
>      |- intc
>      |- spi-nor
>      |- spi-nand
>      |- timer
>      |- gpio
>      `- uart
>    `- switch@1b000000
>       |- ethernet-ports
>       |- mdio
>       |- i2c
>       |- reset
>       `- led/gpio
> 
> The CPU/soc can be disabled and the switch managed by an external CPU 
> (register access over SPI I think, the docs are a bit vague).
> 
> I think I probably inferred too much from mfd/mscc,ocelot.yaml when I 
> created mfd/realtek,rtl9301-switch.yaml.


... and to recap to others for context, the problem is that switch is
simple-mfd and most of the switch children have bus addresses (MMIO of
the switch region) but ethernet-ports does not.

This will work fine, but is discouraged style.

Considering also that some of the children - like syscon-reboot or i2c -
take one or few registers as address space, maybe adding MMIO for
children was not necessary at all.

From implementation point of view, this MMIO is not really used, because
children anyway get and use regmap from the parent, right?

> 
> I still think the switch@1b000000 needs to be "syscon", "simple-mfd" as 
> that's how the reset and i2c blocks work and they're pretty independent 
> of everything else.
> 
> I've currently described the mdio interface as a device on a simple bus 
> although it could just be handled as a descendant of the switch block 
> that a driver looks up as a child node (I've tried to keep the mdio 
> driver independent for now but that's an implementation detail). It does 
> need to reach out to the ethernet-ports to figure out the mapping of 
> port to phy so it isn't independent.
> 
> I see a couple of paths forward
> - keep adding the switch stuff to the mfd/realtek,rtl9301-switch.yaml

I think that's the way to go.

> - move mfd/realtek,rtl9301-switch.yaml to net/realtek,rtl9301-switch.yaml

This you can do anyway. MFD in bindings is rather placeholder for
complex devices where we cannot assign one, common function. In your
case you call it switch, so it could be placed in net in the first place.


> - keep mfd/realtek,rtl9301-switch.yaml with the i2c and reboot but have 
> a $ref to a new binding under net/ (not sure what that'd look like).
> 
> There's only one in-tree user of this so I don't think we need to be too 
> concerned about backwards compatibility. Downstream openwrt handles 
> these devices way differently already.


Best regards,
Krzysztof

Re: Sorting out the RTL9300 dt-bindings

[Chris Packham]

On 10/02/2025 03:40, Krzysztof Kozlowski wrote:
> On 04/02/2025 23:39, Chris Packham wrote:
>> Hi,
>>
>> As Krzysztof points out in [1] I seem to have made a bit of a mess with
>> the mfd binding for the RTL9300 Ethernet switch with integrated CPU. I'm
>> spinning up this email thread separately so as not to unnecessarily spam
>> the netdev folks and to maybe appease google so it doesn't automatically
>> get flagged as junk.
>>
>> First off sorry for not providing a more complete binding initially,
>> Krzysztof suggested doing so a few times but I was concentrating on
>> landing the drivers.
>>
>> The RTL9300 has these basic blocks:
>> - rtl9300
>>     |- cpu@0 - mips34kc
>>     |- soc@18000000
>>       |- intc
>>       |- spi-nor
>>       |- spi-nand
>>       |- timer
>>       |- gpio
>>       `- uart
>>     `- switch@1b000000
>>        |- ethernet-ports
>>        |- mdio
>>        |- i2c
>>        |- reset
>>        `- led/gpio
>>
>> The CPU/soc can be disabled and the switch managed by an external CPU
>> (register access over SPI I think, the docs are a bit vague).
>>
>> I think I probably inferred too much from mfd/mscc,ocelot.yaml when I
>> created mfd/realtek,rtl9301-switch.yaml.
>
> ... and to recap to others for context, the problem is that switch is
> simple-mfd and most of the switch children have bus addresses (MMIO of
> the switch region) but ethernet-ports does not.
>
> This will work fine, but is discouraged style.
>
> Considering also that some of the children - like syscon-reboot or i2c -
> take one or few registers as address space, maybe adding MMIO for
> children was not necessary at all.
>
>  From implementation point of view, this MMIO is not really used, because
> children anyway get and use regmap from the parent, right?

The "reg" property (which I assume is what makes these MMIO children) on 
some of these children is used to work out the offset withing the 
parent. This does lead to a few odd constructs like `regmap_read(map, 
priv->base + REG, &val)` instead of just defining REG as the offset in 
the parent regmap. But it does let us re-use the same driver when 
Realtek decides to arbitrarily move the block around in the next chip 
(e.g. the i2c block on the RTL9310 is now at offset 0x1004).

Some syscon drivers accept an "offset" property but if I understand 
correctly has been deprecated in favor of using "reg" to allow 
supporting syscon children or true MMIO. One of the first patches for 
this platform was to update syscon-reboot.c to take a reg property[1] 
but perhaps I had misunderstood what was MMIO vs syscon and when I 
should use "reg" vs "offset".

Things get a little more complicated as I move into the switch 
functionality. The MDIO stuff is grouped together so I kept the reg 
property but decided to use full register addresses instead of 
priv->base + REG in the driver because the mental math required when 
referring to the datasheet was getting a little bit tiresome (and a 
reviewer said that it looked odd).

When we get to the ethernet-ports that's where any resemblance to MMIO 
goes out the window. There are port related registers scattered 
throughout the switch regmap.

[1] - 
https://lore.kernel.org/r/20241015225948.3971924-3-chris.packham@alliedtelesis.co.nz

>
>> I still think the switch@1b000000 needs to be "syscon", "simple-mfd" as
>> that's how the reset and i2c blocks work and they're pretty independent
>> of everything else.
>>
>> I've currently described the mdio interface as a device on a simple bus
>> although it could just be handled as a descendant of the switch block
>> that a driver looks up as a child node (I've tried to keep the mdio
>> driver independent for now but that's an implementation detail). It does
>> need to reach out to the ethernet-ports to figure out the mapping of
>> port to phy so it isn't independent.
>>
>> I see a couple of paths forward
>> - keep adding the switch stuff to the mfd/realtek,rtl9301-switch.yaml
> I think that's the way to go.
>
>> - move mfd/realtek,rtl9301-switch.yaml to net/realtek,rtl9301-switch.yaml
> This you can do anyway. MFD in bindings is rather placeholder for
> complex devices where we cannot assign one, common function. In your
> case you call it switch, so it could be placed in net in the first place.

Does that mean I should move mfd/realtek,rtl9301-switch.yaml to 
net/realtek,rtl9301-switch.yaml as-is? I'd at least do it as a series of 
doing the move then adding the missing switch bits. But does that 
address the concerns of mixing MMIO-like children with things that 
definitely aren't?

>> - keep mfd/realtek,rtl9301-switch.yaml with the i2c and reboot but have
>> a $ref to a new binding under net/ (not sure what that'd look like).
>>
>> There's only one in-tree user of this so I don't think we need to be too
>> concerned about backwards compatibility. Downstream openwrt handles
>> these devices way differently already.
>
> Best regards,
> Krzysztof