Re: [PATCH net-next v11] net: mdio: Add RTL9300 MDIO driver

[Andrew Lunn]

On Sun, Mar 16, 2025 at 08:11:29PM +0000, Chris Packham wrote:
> 
> On 15/03/2025 04:26, Andrew Lunn wrote:
> >> +static int rtl9300_mdiobus_probe_one(struct device *dev, struct rtl9300_mdio_priv *priv,
> >> +				     struct fwnode_handle *node)
> >> +{
> >> +	struct rtl9300_mdio_chan *chan;
> >> +	struct fwnode_handle *child;
> >> +	struct mii_bus *bus;
> >> +	u32 mdio_bus;
> >> +	int err;
> >> +
> >> +	err = fwnode_property_read_u32(node, "reg", &mdio_bus);
> >> +	if (err)
> >> +		return err;
> >> +
> >> +	/* The MDIO interfaces are either in GPHY (i.e. clause 22) or 10GPHY
> >> +	 * mode (i.e. clause 45).
> > I still need more clarification about this. Is this solely about the
> > polling? Or does an interface in C22 mode go horribly wrong when asked
> > to do a C45 bus transaction?
> 
> It's just the polling. I haven't seen any sign of the bus getting into a 
> bad state when using the wrong transaction type.

If it is just polling, the comment should say that. But i also wounder
if configuring this polling is in the correct place. It has nothing to
do with the MDIO bus driver. It is a switch thing. So logically it
should be in the switch driver. Now the address spaces might not allow
that...

I'm also don't particularly like the idea of using the compatible to
decide this. It is not a PHY property, because you said in another
email, it sets it for all PHYs on the bus, not just one.

> >> +	bus->name = "Realtek Switch MDIO Bus";
> >> +	bus->read = rtl9300_mdio_read_c22;
> >> +	bus->write = rtl9300_mdio_write_c22;
> >> +	bus->read_c45 = rtl9300_mdio_read_c45;
> >> +	bus->write_c45 =  rtl9300_mdio_write_c45;
> > You are providing C45 bus methods, independent of the interface
> > mode. So when accessing EEE registers in C45 address space, C45 bus
> > transactions are going to be used, even on an MDIO interface using C22
> > mode. Does this work? Can you actually do both C22 and C45 bus
> > transactions independent of the interface mode?
> I'm not actually sure if I can mix transactions but it doesn't seem to 
> do any harm.
> 
> Initially I planned to only supply one of the function pairs depending 
> on the mode but I left this in because of this:
> 
> https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/phy/phy.c#n337
> 
> I've written myself a little test app that uses SIOCGMIIREG/SIOCSMIIREG 
> to exercise the MDIO accesses. It uses SIOCGMIIPHY to look up the MDIO 
> address of the PHY attached to the netdev but because of that 
> fallthrough looking up the PHY address for a C45 PHY will fail with 
> -EOPNOTSUPP.

A simpler test is to not provide a DT node. Calling mdiobus_register()
not of_mdiobus_register(). It will then scan the bus looking for
devices on the bus, doing both C22 and C45 reads. You can add a
printk() and see if you get sensible values back from the reads,
e.g. 0xffff or valid looking IDs from registers 2 and 3.

	Andrew