On Wed, Dec 28, 2022 at 12:07:27AM +0100, Michael Walle wrote:
> From: Andrew Lunn <andrew@xxxxxxx>
>
> 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@xxxxxxx>
> Signed-off-by: Michael Walle <michael@xxxxxxxx>
> ---
> 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
