On 24/04/2018 09:37:09-0700, Florian Fainelli wrote: > > > On 04/24/2018 09:09 AM, Alexandre Belloni wrote: > > Some MDIO busses will error out when trying to read a phy address with no > > phy present at that address. In that case, probing the bus will fail > > because __mdiobus_register() is scanning the bus for all possible phys > > addresses. > > > > In case MII_PHYSID1 returns -EIO or -ENODEV, consider there is no phy at > > this address and set the phy ID to 0xffffffff which is then properly > > handled in get_phy_device(). > > Humm, why not have your MDIO bus implementation do the scanning itself > in a reset() callback, which happens before probing the bus, and based > on the results, set phy_mask accordingly such that only PHYs present are > populated? > > My only concern with your change is that we are having a special > treatment for EIO and ENODEV, so we must make sure MDIO bus drivers are > all conforming to that. >
That was what I was doing in [1] but it seems that Andrew preferred this way. The third solution I was seeing was to return phy_reg instead of -EIO so the MDIO driver can return -ENODEV and that would be passed to get_phy_device(). __mdiobus_register() seems to handle -ENODEV properly. My coccinelle-fu is not great but the following drivers can return -ENODEV from their read callback: drivers/net/ethernet/marvell/mvmdio.c drivers/net/ethernet/hisilicon/hix5hd2_gmac.c (seeing the error message, this has probably been copy pasted) [1] https://marc.info/?l=linux-netdev&m=152183609927933&w=2 > > > > Suggested-by: Andrew Lunn <and...@lunn.ch> > > Signed-off-by: Alexandre Belloni <alexandre.bell...@bootlin.com> > > --- > > drivers/net/phy/phy_device.c | 11 ++++++++++- > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > > index ac23322a32e1..9e4ba8e80a18 100644 > > --- a/drivers/net/phy/phy_device.c > > +++ b/drivers/net/phy/phy_device.c > > @@ -535,8 +535,17 @@ static int get_phy_id(struct mii_bus *bus, int addr, > > u32 *phy_id, > > > > /* Grab the bits from PHYIR1, and put them in the upper half */ > > phy_reg = mdiobus_read(bus, addr, MII_PHYSID1); > > - if (phy_reg < 0) > > + if (phy_reg < 0) { > > + /* if there is no device, return without an error so scanning > > + * the bus works properly > > + */ > > + if (phy_reg == -EIO || phy_reg == -ENODEV) { > > + *phy_id = 0xffffffff; > > + return 0; > > + } > > + > > return -EIO; > > + } > > > > *phy_id = (phy_reg & 0xffff) << 16; > > > > > > -- > Florian -- Alexandre Belloni, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com
