Message-ID: <5c9aa2f3-689f-6dd7-ed26-de11e14f5ba0@rasmusvillemoes.dk>
Date:   Fri, 19 Aug 2022 13:47:51 +0200
From:   Rasmus Villemoes <linux@...musvillemoes.dk>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Vladimir Oltean <vladimir.oltean@....com>, netdev@...r.kernel.org,
        Woojung Huh <woojung.huh@...rochip.com>,
        UNGLinuxDriver@...rochip.com, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Russell King <linux@...linux.org.uk>,
        Michael Grzeschik <m.grzeschik@...gutronix.de>,
        Oleksij Rempel <linux@...pel-privat.de>,
        Thorsten Leemhuis <regressions@...mhuis.info>,
        Alvin Šipraga <alsi@...g-olufsen.dk>,
        Craig McQueen <craig@...ueen.id.au>,
        Tim Harvey <tharvey@...eworks.com>
Subject: Re: [PATCH net] net: dsa: microchip: keep compatibility with device
 tree blobs with no phy-mode

On 19/08/2022 12.57, Vladimir Oltean wrote:
> On Fri, Aug 19, 2022 at 12:19:12PM +0200, Rasmus Villemoes wrote:
>> Well, there's also e.g. arch/arm/boot/dts/at91-sama5d3_ksz9477_evb.dts
>> which sets the phy-mode but not the phy-handle:
>>
>>                         port@0 {
>>                                 reg = <0>;
>>                                 label = "lan1";
>>                                 phy-mode = "internal";
>>                         };
> 
> Yeah, it looks like there's also that variation, curious.
> 
>> And doing that in my case seems to fix things (I wouldn't know what
>> phy-handle to point at anyway), so since we're still in development, I
>> think I'll do that. But if I want to follow the new-world-order to the
>> letter, should I also figure out a way to point at a phy-handle?
> 
> So if by "new world order" you mean
> https://patchwork.kernel.org/project/netdevbpf/cover/20220818115500.2592578-1-vladimir.oltean@nxp.com/
> then no, that patch set doesn't change the requirements for *user* ports
> (what you have here) but for CPU and DSA ports, where no phy-handle/
> fixed-link/phy-mode means something entirely different than it means for
> user ports.

No, I meant the new world order as of the things merged into v5.19.
Which does seem to require an explicit phy-mode, and I'm happy to add that.

> To give you an idea of how things work for user ports. If a user port
> has a phy-handle, DSA will connect to that, irrespective of what OF-based
> MDIO bus that is on. If not, DSA looks at whether ds->slave_mii_bus is
> populated with a struct mii_bus by the driver. If it is, it connects in
> a non-OF based way to a PHY address equal to the port number. If
> ds->slave_mii_bus doesn't exist but the driver provides
> ds->ops->phy_read and ds->ops->phy_write, DSA automatically creates
> ds->slave_mii_bus where its ops are the driver provided phy_read and
> phy_write, and it then does the same thing of connecting to the PHY in
> that non-OF based way.

Thanks, that's quite useful. From quick grepping, it seems that ksz9567
currently falls into the latter category?

> So to convert a driver that currently relies on DSA allocating the
> ds->slave_mii_bus, you need to allocate/register it yourself (using the
> of_mdiobus_* variants), and populate ds->slave_mii_bus with it. Look at
> lan937x_mdio_register() for instance.
> 
> For existing device trees which connect in a non-OF based way, you still
> need to keep ds->ops->phy_read and ds->ops->phy_write, and let DSA
> create the ds->slave_mii_bus. The phy_read and phy_write can be the same
> between your MDIO bus ops and DSA's MDIO bus ops.
> 
>>> Fixes: 2c709e0bdad4 ("net: dsa: microchip: ksz8795: add phylink support")
>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216320
>>> Reported-by: Craig McQueen <craig@...ueen.id.au>
>>> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
>>
>> I've also tested this patch on top of v5.19 without altering my .dts,
>> and that also seems to fix things, so I suppose you can add
>>
>> Fixes: 65ac79e18120 ("net: dsa: microchip: add the phylink get_caps")
>> Tested-by: Rasmus Villemoes <rasmus.villemoes@...vas.dk>
> 
> So I don't intend to make you modify your device tree in this case, 

No, but it's actually easier for me to just do that rather than carry an
extra patch until the mainline fix hits 5.19.y.

but
> there is something to be said about U-Boot compatibility. In U-Boot,
> with DM_DSA, I don't intend to support any unnecessary complexity and
> alternative ways of describing the same thing, so there, phy-mode and
> one of phy-handle or fixed-link are mandatory for all ports. 

OK. I suppose that means the linux driver for the ksz9477 family  should
learn to check if there's an "mdio" subnode and if so populate
ds->slave_mii_bus, but unlike lan937x, absence of that node should not
be fatal?

And since
> U-Boot can pass its own device tree to Linux, it means Linux DSA drivers
> might need to gradually gain support for OF-based phy-handle on user
> ports as well. So see what Tim Harvey has done in drivers/net/ksz9477.c
> in the U-Boot source code, and try to work with his device tree format,
> as a starting point.

Hm. It does seem like that driver has the mdio bus optional (as in,
probe doesn't fail just because the subnode isn't present). But I'm
curious why ksz_probe_mdio() looks for a subnode called "mdios" rather
than "mdio". Tim, just a typo?

Thanks,
Rasmus

Powered by blists - more mailing lists