Messages in this thread Patch in this message | | | Date | Tue, 3 Jan 2023 17:31:34 +0200 | From | Vladimir Oltean <> | 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@lunn.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@lunn.ch> > Signed-off-by: Michael Walle <michael@walle.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
| |