[LEDE-DEV] [PATCH 1/2] generic: ar8216: fix ar8xxx_is_possible check

[Christian Lamparter]

Hello,

On Monday, October 3, 2016 9:12:32 PM CEST John Crispin wrote:
> On 01/10/2016 18:33, Christian Lamparter wrote:
> > The commit "generic: ar8216: add sanity check to ar8216_probe"
> > (774da6c7a40320a320b28d71291c0e61fcf7bc8a) stated that PHY IDs
> > should be checked at address 0-4. However, the PHY 4 was
> > never check by the for loop... And I can't find any documents
> > about why this check should be performed the way it is
> > currently?!
>
> the SDK driver checks for the phy ids if i am not mistaken.
Thanks. I looked at the phy driver again and found the check right here [0].
(ATHRS16 and ATHRS17 uses similar code).

|    for (phyUnit=0; phyUnit < S27_PHY_MAX; phyUnit++) {
|
|        foundPhy = TRUE;
|        phyBase = S27_PHYBASE(phyUnit);
|        phyAddr = S27_PHYADDR(phyUnit);
|
|        if (!S27_IS_ETHUNIT(phyUnit, ethUnit)) {
|            continue;
|        }
|        [...]
|    }
|
|    if (!foundPhy) {
|        return FALSE; /* No PHY's configured for this ethUnit */
|    }

S27_PHY_MAX is 5 (line #151). The vendor driver checks PHY 0 - (including) 4.
But unlike ar8xxx_is_possible, it just skips unavailable PHYs (It looks like
it foundPhy was set to TRUE even though the PHY wasn't actually tested yet, 
this looks like a bug). I would be happy to add the skip to ar8xxx_is_possible
and make it to check all 5 PHYs.

For example:
	unsigned int found_phys = 0;

	for (i = 0; i < 5; i++) {
		u32 phy_id;

		phy_id = mdiobus_read(bus, i, MII_PHYSID1) << 16;
		phy_id |= mdiobus_read(bus, i, MII_PHYSID2);
		if (ar8xxx_phy_match(phy_id)) {
			found_phys++;
		} else if (phy_id) {
			pr_debug("ar8xxx: unknown PHY at %s:%02x id:%08x\n",
					  dev_name(&bus->dev), i, phy_id);
		}
	}

	if (found_phys == 0)
		return false;

This would work fine with the C-60. What do you think?

> doing the check does make sense to ensure that the switch is
> actually present.
The PHYID is used for looking up the correct driver, so at least
one phy port needs to have a valid ID for phy_connect() to work.

> why do you want to remove this check ? imho it does no harm
The C-60 doesn't have a PHY at 3. This caused the check in ar8xxx_is_possible
to fail and the ethernet ports on the C-60. Also, it doesn't look like the
qca8k.c (DSA) driver checks for it. So I removed it.

Regards,
Christian

[0] <https://github.com/riptidewave93/meraki-linux/blob/linux-3.4-r23-20150601/drivers/net/ethernet/atheros/ag7240/phys/athrs27_phy.c#L754>

> > Signed-off-by: Christian Lamparter <chunkeey at gmail.com>
> > ---
> >  .../linux/generic/files/drivers/net/phy/ar8216.c   | 23 ----------------------
> >  1 file changed, 23 deletions(-)
> > 
> > diff --git a/target/linux/generic/files/drivers/net/phy/ar8216.c b/target/linux/generic/files/drivers/net/phy/ar8216.c
> > index 70f4774..57130b1 100644
> > --- a/target/linux/generic/files/drivers/net/phy/ar8216.c
> > +++ b/target/linux/generic/files/drivers/net/phy/ar8216.c
> > @@ -2110,26 +2110,6 @@ ar8xxx_phy_match(u32 phy_id)
> >  	return false;
> >  }
> >  
> > -static bool
> > -ar8xxx_is_possible(struct mii_bus *bus)
> > -{
> > -	unsigned i;
> > -
> > -	for (i = 0; i < 4; i++) {
> > -		u32 phy_id;
> > -
> > -		phy_id = mdiobus_read(bus, i, MII_PHYSID1) << 16;
> > -		phy_id |= mdiobus_read(bus, i, MII_PHYSID2);
> > -		if (!ar8xxx_phy_match(phy_id)) {
> > -			pr_debug("ar8xxx: unknown PHY at %s:%02x id:%08x\n",
> > -				 dev_name(&bus->dev), i, phy_id);
> > -			return false;
> > -		}
> > -	}
> > -
> > -	return true;
> > -}
> > -
> >  static int
> >  ar8xxx_phy_probe(struct phy_device *phydev)
> >  {
> > @@ -2141,9 +2121,6 @@ ar8xxx_phy_probe(struct phy_device *phydev)
> >  	if (phydev->addr != 0 && phydev->addr != 4)
> >  		return -ENODEV;
> >  
> > -	if (!ar8xxx_is_possible(phydev->bus))
> > -		return -ENODEV;
> > -
> >  	mutex_lock(&ar8xxx_dev_list_lock);
> >  	list_for_each_entry(priv, &ar8xxx_dev_list, list)
> >  		if (priv->mii_bus == phydev->bus)
> > 
>