Message-ID: <20230103153134.utalc6kw3l34dp4s@skbuf>
Date:   Tue, 3 Jan 2023 17:31:34 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Michael Walle <michael@...le.cc>
Cc:     Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Jose Abreu <Jose.Abreu@...opsys.com>,
        Sergey Shtylyov <s.shtylyov@....ru>,
        Wei Fang <wei.fang@....com>,
        Shenwei Wang <shenwei.wang@....com>,
        Clark Wang <xiaoning.wang@....com>,
        NXP Linux Team <linux-imx@....com>,
        Sean Wang <sean.wang@...iatek.com>,
        Landen Chao <Landen.Chao@...iatek.com>,
        DENG Qingfang <dqfext@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-renesas-soc@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, Andrew Lunn <andrew@...n.ch>,
        Geert Uytterhoeven <geert+renesas@...der.be>
Subject: Re: [PATCH RFC net-next v2 11/12] net: dsa: Separate C22 and C45
 MDIO bus transaction methods

On Wed, Dec 28, 2022 at 12:07:27AM +0100, Michael Walle wrote:
> From: Andrew Lunn <andrew@...n.ch>
> 
> By adding _c45 function pointers to the dsa_switch_op structure, the
> dsa core can register an MDIO bus with C45 accessors.
> 
> The dsa-loop driver could in theory provide such accessors, since it
> just passed requests to the MDIO bus it is on, but it seems unlikely
> to be useful at the moment. It can however be added later.
> 
> mt7530 does support C45, but its uses a mix of registering its MDIO
> bus and using the DSA core provided bus. This makes the change a bit
> more complex.

"using the DSA core provided bus" is a misrepresentation AFAICS.
Rather said, "providing its private MDIO bus to the DSA core too".

> 
> Signed-off-by: Andrew Lunn <andrew@...n.ch>
> Signed-off-by: Michael Walle <michael@...le.cc>
> ---
> v2:
>  - [al] Remove conditional c45, since all switches support c45
>  - [al] Remove dsa core changes, they are not needed
>  - [al] Add comment that DSA provided MDIO bus is C22 only.
> ---
>  drivers/net/dsa/mt7530.c | 87 ++++++++++++++++++++++++------------------------
>  drivers/net/dsa/mt7530.h | 15 ++++++---
>  include/net/dsa.h        |  2 +-
>  3 files changed, 56 insertions(+), 48 deletions(-)

This patch is not very coherent after the changes in v2.

There are really 2 distinct pieces:

1. a comment in include/net/dsa.h, which provides a justification for
why dsa_switch_ops :: {phy_read(), phy_write()} weren't split into
{phy_read(), phy_write()} and {phy_read_c45() and phy_write_c45()}.

2. a conversion of the mt7530 MDIO bus driver.

I would expect these to be distinct patches.

> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 96086289aa9b..732c7bc261a9 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -858,7 +858,7 @@ struct dsa_switch_ops {
>  	u32	(*get_phy_flags)(struct dsa_switch *ds, int port);
>  
>  	/*
> -	 * Access to the switch's PHY registers.
> +	 * Access to the switch's PHY registers. C22 only.
>  	 */
>  	int	(*phy_read)(struct dsa_switch *ds, int port, int regnum);
>  	int	(*phy_write)(struct dsa_switch *ds, int port,

Let me try to untangle for you what these operations really do.
When they are present, DSA will allocate ds->slave_mii_bus on behalf of
the driver, and use these methods for MDIO access of internal PHYs.

The purpose of ds->slave_mii_bus is to offer a non-OF based
phy_connect() for old-style device trees where there is no phy-handle
between the user port fwnode and the internal PHY fwnode (normally
because the ethernet-phy isn't described in the device tree at all).

Like this:

	ethernet-switch {
		ethernet-ports {
			port@1 {
				reg = <1>;
			};
		};
	};

So ds->slave_mii_bus is useful with or without the ds->ops->phy_read()
and ds->ops->phy_write() pointers, which is for example why mt7530
allocates its own MDIO bus with its own private methods (so it doesn't
populate ds->ops->phy_read()), but it also populates ds->slave_mii_bus
with its own bus.

Since clause 45 PHYs are identified by the "ethernet-phy-ieee802.3-c45"
compatible string (otherwise they are C22), then a PHY which is not
described in the device tree can only be C22. So this is why
ds->slave_mii_bus only deals with clause 22 methods, and the true reason
behind the comment above.

But actually this premise is no longer true since Luiz' commit
fe7324b93222 ("net: dsa: OF-ware slave_mii_bus"), which introduced the
strange concept of an "OF-aware helper for internal PHYs which are not
described in the device tree". After his patch, it is possible to have
something like this:

	ethernet-switch {
		ethernet-ports {
			port@1 {
				reg = <1>;
			};
		};

		mdio {
			ethernet-phy@1 {
				compatible = "ethernet-phy-ieee802.3-c45"
				reg = <1>;
			};
		};
	};

As you can see, this is a clause 45 internal PHY which lacks a
phy-handle, so its bus must be put in ds->slave_mii_bus in order for
dsa_slave_phy_connect() to see it without that phy-handle (based on the
port number matching with the PHY number). After Luiz' patch, this kind
of device tree is possible, and it invalidates the assumption about
ds->slave_mii_bus only driving C22 PHYs.

> 
> -- 
> 2.30.2

Powered by blists - more mailing lists