Date:   Fri, 11 Feb 2022 09:03:02 +0000
From:   Alvin Šipraga <ALSI@...g-olufsen.dk>
To:     Luiz Angelo Daros de Luca <luizluca@...il.com>
CC:     Florian Fainelli <f.fainelli@...il.com>,
        "open list:NETWORKING DRIVERS" <netdev@...r.kernel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Arınç ÜNAL <arinc.unal@...nc9.com>
Subject: Re: [PATCH net-next] net: dsa: realtek: rtl8365mb: irq with
 realtek-mdio

Hi Luiz,

Luiz Angelo Daros de Luca <luizluca@...il.com> writes:

> Thanks Alvin,
>
>> > As far as I know, for supported models, yes. I'm not sure about models
>> > rtl8363nb and rtl8364nb because they have only 2 user ports at 1 and
>> > 3.
>> > Anyway, they are not supported yet.
>>
>> I think the port number as defined in the device tree is always going to
>> be the same as its PHY address on the internal bus. I had a look at the
>> Realtek code and this seems to be the assumption there too.
>
> One of the realtek-smi.txt examples (that I also copied to
> realtek.yaml) does not respect that:
>
> phy4: phy@4 {
>    reg = <4>;
>    interrupt-parent = <&switch_intc>;
>    interrupts = <12>;
> };
>
> I don't know if 12 is a typo here.

Oh sorry, I thought we were talking about the rtl8365mb driver and the
family it supports. I did not check datasheets for RTL8366RB and
such. My statement was only regarding switches supported by the '65mb
driver, where I still believe this is the case based on my reading of
the Realtek vendor code.

>
> It would only matter if I do create a default association when the
> specific device tree-entry is missing. It wasn't supposed to
> completely remove the device-tree declaration but to make it optional.
> For now, I'll put this option aside.
>
>> >> We could also change the DSA framework's way of creating the
>> >> MDIO bus so as to be OF-aware.
>
> It worked like a charm. I'll send it in reply to this email. I still
> have some questions about it.
>
>> We are not the only ones doing this. mv88e6xxx is another example. So
>> Florian's suggestion seems like a good one, but we should be careful to
>> maintain compatibility with older device trees. In some cases it is
>> based on child node name (e.g. "mdio"), in others it is based on the
>> child node compatible string (e.g. "realtek,smi-mdio",
>> "marvell,mv88e6xxx-mdio-external").
>
> The name "mdio" seems to be the de facto name. I'll use it. However,
> it might be confusing with mdio-connected switches as you'll have an
> mdio inside a switch inside another mdio. But it is exactly what it
> is.
>
> It would not affect drivers that are already allocating slave_mii_bus
> by themselves. If the driver is fine with that, that's the end of the
> case.
>
> However, if a driver doesn't need any special properties inside the
> mdio node, it might want to migrate to the default dsa slave_mii_bus.
> For those already using "mdio" node name, they just need to drop their
> code and move phy_read/write to dsa_switch_ops. Now, those using
> different node names (like when they check the compatible strings)
> will have a little more job. I believe we cannot rename a node "on the
> fly". So, if the matched node name is not mdio, they still need to
> allocate the slave_mii_bus. They will also need a different
> dsa_switch_ops for each case because dsa_switch_ops->phy_read cannot
> coexist with an externally allocated slave_mii_bus. Each driver needs
> to plan their own migration path if they want to migrate.
>
> For realtek-smi, the code does not require the node to be named "mdio"
> but doc "realtek-smi.txt" and my new "realtek.yaml" does require it.

The fact that the required property is documented as "mdio" probably
lets us do away with the compatible string parsing and switch to a
generic implementation in the realtek drivers - although I'm no device
tree lawyer, so I could be wrong here. I agree with your analysis.

> If I assume that, I could simply drop the code and migrate to
> read/write to dsa_switch_ops.
> If not, we need to maintain both code paths and warn the user for a
> couple of releases until we drop the compatible string match.
>
>> > If possible, I would like to define safe default values (like assuming
>> > 1:1 mapping between the port number and its PHY address) for this
>> > driver when interrupt-controller is present but
>> > slave_mii_bus node is missing.
>>
>> You could just require the phy nodes to be described in the device
>> tree. Then you don't need this extra port_setup code. Seems better IMO,
>> or am I missing something?
>
> Upstream devs seem to prefer more code than more device-tree confs. I
> just wanted to reduce some device-tree copy/paste. I'm ok with using a
> device-tree node.

Do you have an example of such a statement from an upstream dev? I am
asking for my own education :-)

If we require an mdio node to begin with it also obviates the whole
discussion about 1:1 mapping between DSA port number and PHY address.

Kind regards,
Alvin

Powered by blists - more mailing lists