Message ID | 8e08cb676011aefb69a3fbf9ccdd5a529e289c09.1475339604.git.chunkeey@gmail.com |
---|---|
State | Changes Requested |
Headers | show |
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?! Hi the SDK driver checks for the phy ids if i am not mistaken. doing the cech does make sense to ensure that the switch is actually present. why do you want to remove this check ? imho it does no harm John > > Signed-off-by: Christian Lamparter <chunkeey@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) >
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@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) > > >
On 03/10/2016 22:57, Christian Lamparter wrote: >> > 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. your patch failed to mention this. remving the code because of a buggy switch seems odd. please modify the code to just skip missing phy ids. John
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)
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?! Signed-off-by: Christian Lamparter <chunkeey@gmail.com> --- .../linux/generic/files/drivers/net/phy/ar8216.c | 23 ---------------------- 1 file changed, 23 deletions(-)