| Message ID | 20200522213059.1535892-4-jeremy.linton@arm.com |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | Make C45 autoprobe more robust | expand |
On Fri, May 22, 2020 at 04:30:51PM -0500, Jeremy Linton wrote: > Lets factor out the phy id logic, and make it generic > so that it can be used for c22 and c45. > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > --- > drivers/net/phy/phy_device.c | 65 +++++++++++++++++++----------------- > 1 file changed, 35 insertions(+), 30 deletions(-) > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index 7746c07b97fe..f0761fa5e40b 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -695,6 +695,29 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus *bus, int addr, int dev_addr, > return 0; > } > > +static int _get_phy_id(struct mii_bus *bus, int addr, int dev_addr, > + u32 *phy_id, bool c45) Hi Jeremy How about read_phy_id() so you can avoid the _ prefix. > static bool valid_phy_id(int val) > { > return (val > 0 && ((val & 0x1fffffff) != 0x1fffffff)); > @@ -715,17 +738,17 @@ static bool valid_phy_id(int val) > */ > static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id, > struct phy_c45_device_ids *c45_ids) { > - int phy_reg; > - int i, reg_addr; > + int ret; > + int i; > const int num_ids = ARRAY_SIZE(c45_ids->device_ids); > u32 *devs = &c45_ids->devices_in_package; > > /* Find first non-zero Devices In package. Device zero is reserved > * for 802.3 c45 complied PHYs, so don't probe it at first. > */ > - for (i = 1; i < num_ids && *devs == 0; i++) { > - phy_reg = get_phy_c45_devs_in_pkg(bus, addr, i, devs); > - if (phy_reg < 0) > + for (i = 0; i < num_ids && *devs == 0; i++) { > + ret = get_phy_c45_devs_in_pkg(bus, addr, i, devs); > + if (ret < 0) > return -EIO; Renaming reg_addr to ret does not belong in this patch. Andrew
Hi, Thanks for taking a look at this! On 5/23/20 10:28 AM, Andrew Lunn wrote: > On Fri, May 22, 2020 at 04:30:51PM -0500, Jeremy Linton wrote: >> Lets factor out the phy id logic, and make it generic >> so that it can be used for c22 and c45. >> >> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> >> --- >> drivers/net/phy/phy_device.c | 65 +++++++++++++++++++----------------- >> 1 file changed, 35 insertions(+), 30 deletions(-) >> >> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c >> index 7746c07b97fe..f0761fa5e40b 100644 >> --- a/drivers/net/phy/phy_device.c >> +++ b/drivers/net/phy/phy_device.c >> @@ -695,6 +695,29 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus *bus, int addr, int dev_addr, >> return 0; >> } >> >> +static int _get_phy_id(struct mii_bus *bus, int addr, int dev_addr, >> + u32 *phy_id, bool c45) > > Hi Jeremy > > How about read_phy_id() so you can avoid the _ prefix. Yes, that sounds good. > >> static bool valid_phy_id(int val) >> { >> return (val > 0 && ((val & 0x1fffffff) != 0x1fffffff)); >> @@ -715,17 +738,17 @@ static bool valid_phy_id(int val) >> */ >> static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id, >> struct phy_c45_device_ids *c45_ids) { >> - int phy_reg; >> - int i, reg_addr; >> + int ret; >> + int i; >> const int num_ids = ARRAY_SIZE(c45_ids->device_ids); >> u32 *devs = &c45_ids->devices_in_package; >> >> /* Find first non-zero Devices In package. Device zero is reserved >> * for 802.3 c45 complied PHYs, so don't probe it at first. >> */ >> - for (i = 1; i < num_ids && *devs == 0; i++) { >> - phy_reg = get_phy_c45_devs_in_pkg(bus, addr, i, devs); >> - if (phy_reg < 0) >> + for (i = 0; i < num_ids && *devs == 0; i++) { >> + ret = get_phy_c45_devs_in_pkg(bus, addr, i, devs); >> + if (ret < 0) >> return -EIO; > > Renaming reg_addr to ret does not belong in this patch. Sure, that makes sense. The rename was a last min change when I was shuffling the args around. Thanks,
Hi, On 5/23/20 10:28 AM, Andrew Lunn wrote: > On Fri, May 22, 2020 at 04:30:51PM -0500, Jeremy Linton wrote: >> Lets factor out the phy id logic, and make it generic >> so that it can be used for c22 and c45. >> >> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> >> --- >> drivers/net/phy/phy_device.c | 65 +++++++++++++++++++----------------- >> 1 file changed, 35 insertions(+), 30 deletions(-) >> >> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c >> index 7746c07b97fe..f0761fa5e40b 100644 >> --- a/drivers/net/phy/phy_device.c >> +++ b/drivers/net/phy/phy_device.c >> @@ -695,6 +695,29 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus *bus, int addr, int dev_addr, >> return 0; >> } >> >> +static int _get_phy_id(struct mii_bus *bus, int addr, int dev_addr, >> + u32 *phy_id, bool c45) > > Hi Jeremy > > How about read_phy_id() so you can avoid the _ prefix. > >> static bool valid_phy_id(int val) >> { >> return (val > 0 && ((val & 0x1fffffff) != 0x1fffffff)); >> @@ -715,17 +738,17 @@ static bool valid_phy_id(int val) >> */ >> static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id, >> struct phy_c45_device_ids *c45_ids) { >> - int phy_reg; >> - int i, reg_addr; >> + int ret; >> + int i; >> const int num_ids = ARRAY_SIZE(c45_ids->device_ids); >> u32 *devs = &c45_ids->devices_in_package; >> >> /* Find first non-zero Devices In package. Device zero is reserved >> * for 802.3 c45 complied PHYs, so don't probe it at first. >> */ >> - for (i = 1; i < num_ids && *devs == 0; i++) { >> - phy_reg = get_phy_c45_devs_in_pkg(bus, addr, i, devs); >> - if (phy_reg < 0) >> + for (i = 0; i < num_ids && *devs == 0; i++) { >> + ret = get_phy_c45_devs_in_pkg(bus, addr, i, devs); >> + if (ret < 0) >> return -EIO; > > Renaming reg_addr to ret does not belong in this patch. > Looks like I changed the loop index in this patch while shuffling things around yesterday too. The "for (i = 1/0.." change belongs in 5/11 as well.
On Fri, May 22, 2020 at 04:30:51PM -0500, Jeremy Linton wrote: > Lets factor out the phy id logic, and make it generic > so that it can be used for c22 and c45. > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > --- > drivers/net/phy/phy_device.c | 65 +++++++++++++++++++----------------- > 1 file changed, 35 insertions(+), 30 deletions(-) > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index 7746c07b97fe..f0761fa5e40b 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -695,6 +695,29 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus *bus, int addr, int dev_addr, > return 0; > } > > +static int _get_phy_id(struct mii_bus *bus, int addr, int dev_addr, > + u32 *phy_id, bool c45) > +{ > + int phy_reg, reg_addr; > + > + int reg_base = c45 ? (MII_ADDR_C45 | dev_addr << 16) : 0; int phy_reg, reg_addr, reg_base; reg_base = c45 ? (MII_ADDR_C45 | dev_addr << 16) : 0; I think would be preferable. > + > + reg_addr = reg_base | MII_PHYSID1; > + phy_reg = mdiobus_read(bus, addr, reg_addr); > + if (phy_reg < 0) > + return -EIO; > + > + *phy_id = phy_reg << 16; > + > + reg_addr = reg_base | MII_PHYSID2; > + phy_reg = mdiobus_read(bus, addr, reg_addr); > + if (phy_reg < 0) > + return -EIO; > + *phy_id |= phy_reg; The line spacing seems to be a little inconsistent between the above two register reads. > + > + return 0; > +} So, _get_phy_id() returns either zero or -EIO. > + > static bool valid_phy_id(int val) > { > return (val > 0 && ((val & 0x1fffffff) != 0x1fffffff)); > @@ -715,17 +738,17 @@ static bool valid_phy_id(int val) > */ > static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id, > struct phy_c45_device_ids *c45_ids) { > - int phy_reg; > - int i, reg_addr; > + int ret; > + int i; > const int num_ids = ARRAY_SIZE(c45_ids->device_ids); > u32 *devs = &c45_ids->devices_in_package; I feel a "reverse christmas tree" complaint brewing... yes, the original code didn't follow it. Maybe a tidy up while touching this code? > > /* Find first non-zero Devices In package. Device zero is reserved > * for 802.3 c45 complied PHYs, so don't probe it at first. > */ > - for (i = 1; i < num_ids && *devs == 0; i++) { > - phy_reg = get_phy_c45_devs_in_pkg(bus, addr, i, devs); > - if (phy_reg < 0) > + for (i = 0; i < num_ids && *devs == 0; i++) { > + ret = get_phy_c45_devs_in_pkg(bus, addr, i, devs); > + if (ret < 0) > return -EIO; > > if ((*devs & 0x1fffffff) == 0x1fffffff) { > @@ -752,17 +775,9 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id, > if (!(c45_ids->devices_in_package & (1 << i))) > continue; > > - reg_addr = MII_ADDR_C45 | i << 16 | MII_PHYSID1; > - phy_reg = mdiobus_read(bus, addr, reg_addr); > - if (phy_reg < 0) > - return -EIO; > - c45_ids->device_ids[i] = phy_reg << 16; > - > - reg_addr = MII_ADDR_C45 | i << 16 | MII_PHYSID2; > - phy_reg = mdiobus_read(bus, addr, reg_addr); > - if (phy_reg < 0) > - return -EIO; > - c45_ids->device_ids[i] |= phy_reg; > + ret = _get_phy_id(bus, addr, i, &c45_ids->device_ids[i], true); > + if (ret < 0) > + return ret; So here we can only propagate the -EIO, so this keeps the semantics. > } > *phy_id = 0; > return 0; > @@ -787,27 +802,17 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id, > static int get_phy_id(struct mii_bus *bus, int addr, u32 *phy_id, > bool is_c45, struct phy_c45_device_ids *c45_ids) > { > - int phy_reg; > + int ret; > > if (is_c45) > return get_phy_c45_ids(bus, addr, phy_id, c45_ids); > > - /* Grab the bits from PHYIR1, and put them in the upper half */ > - phy_reg = mdiobus_read(bus, addr, MII_PHYSID1); > - if (phy_reg < 0) { > + ret = _get_phy_id(bus, addr, 0, phy_id, false); > + if (ret < 0) { > /* returning -ENODEV doesn't stop bus scanning */ > - return (phy_reg == -EIO || phy_reg == -ENODEV) ? -ENODEV : -EIO; > + return (ret == -EIO || ret == -ENODEV) ? -ENODEV : -EIO; Since ret will only ever be -EIO here, this can only ever return -ENODEV, which is a functional change in the code (probably unintended.) Nevertheless, it's likely introducing a bug if the intention is for some other return from mdiobus_read() to be handled differently. > } > > - *phy_id = phy_reg << 16; > - > - /* Grab the bits from PHYIR2, and put them in the lower half */ > - phy_reg = mdiobus_read(bus, addr, MII_PHYSID2); > - if (phy_reg < 0) > - return -EIO; ... whereas this one always returns -EIO on any error. So, I think you have the potential in this patch to introduce a subtle change of behaviour, which may lead to problems - have you closely analysed why the code was the way it was, and whether your change of behaviour is actually valid? > - > - *phy_id |= phy_reg; > - > return 0; > } > > -- > 2.26.2 > >
On Sat, May 23, 2020 at 12:32:59PM -0500, Jeremy Linton wrote: > Hi, > > On 5/23/20 10:28 AM, Andrew Lunn wrote: > > On Fri, May 22, 2020 at 04:30:51PM -0500, Jeremy Linton wrote: > > > Lets factor out the phy id logic, and make it generic > > > so that it can be used for c22 and c45. > > > > > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > > > --- > > > drivers/net/phy/phy_device.c | 65 +++++++++++++++++++----------------- > > > 1 file changed, 35 insertions(+), 30 deletions(-) > > > > > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > > > index 7746c07b97fe..f0761fa5e40b 100644 > > > --- a/drivers/net/phy/phy_device.c > > > +++ b/drivers/net/phy/phy_device.c > > > @@ -695,6 +695,29 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus *bus, int addr, int dev_addr, > > > return 0; > > > } > > > +static int _get_phy_id(struct mii_bus *bus, int addr, int dev_addr, > > > + u32 *phy_id, bool c45) > > > > Hi Jeremy > > > > How about read_phy_id() so you can avoid the _ prefix. > > > > > static bool valid_phy_id(int val) > > > { > > > return (val > 0 && ((val & 0x1fffffff) != 0x1fffffff)); > > > @@ -715,17 +738,17 @@ static bool valid_phy_id(int val) > > > */ > > > static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id, > > > struct phy_c45_device_ids *c45_ids) { > > > - int phy_reg; > > > - int i, reg_addr; > > > + int ret; > > > + int i; > > > const int num_ids = ARRAY_SIZE(c45_ids->device_ids); > > > u32 *devs = &c45_ids->devices_in_package; > > > /* Find first non-zero Devices In package. Device zero is reserved > > > * for 802.3 c45 complied PHYs, so don't probe it at first. > > > */ > > > - for (i = 1; i < num_ids && *devs == 0; i++) { > > > - phy_reg = get_phy_c45_devs_in_pkg(bus, addr, i, devs); > > > - if (phy_reg < 0) > > > + for (i = 0; i < num_ids && *devs == 0; i++) { > > > + ret = get_phy_c45_devs_in_pkg(bus, addr, i, devs); > > > + if (ret < 0) > > > return -EIO; > > > > Renaming reg_addr to ret does not belong in this patch. > > > > Looks like I changed the loop index in this patch while shuffling things > around yesterday too. The "for (i = 1/0.." change belongs in 5/11 as well. Indeed; that's a change of behaviour - see the CS4315/CS4340 workaround. Note that MMD 0 is explicitly marked "Reserved" in 802.3, so we shouldn't be probing it in this way. Also note that bit 0 of the "devices in package" does not mean that MMD 0 is accessible; it means that the clause 22 registers are present: "Bit 5.0 is used to indicate that Clause 22 functionality has been implemented within a Clause 45 electrical interface device." which basically means Clause 22 protocol over Clause 45 electrical interface. So, we should be careful about poking in MMD 0 if 5.0 is set. Some places that will break are: - phylink_phy_read() / phylink_phy_write() - phy_restart_aneg() / phy_config_aneg() if we have an clause 45 MDIO interface that can't issue clause 22 MDIO cycles with a PHY that sets 5.0. These comments apply more to your patch 4, but you brought up the MMD 0 accesses in this patch, so I thought I'd provide it here.
> > static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id, > > struct phy_c45_device_ids *c45_ids) { > > - int phy_reg; > > - int i, reg_addr; > > + int ret; > > + int i; > > const int num_ids = ARRAY_SIZE(c45_ids->device_ids); > > u32 *devs = &c45_ids->devices_in_package; > > I feel a "reverse christmas tree" complaint brewing... yes, the original > code didn't follow it. Maybe a tidy up while touching this code? At minimum, a patch should not make it worse. ret and i should clearly be after devs. > > static int get_phy_id(struct mii_bus *bus, int addr, u32 *phy_id, > > bool is_c45, struct phy_c45_device_ids *c45_ids) > > { > > - int phy_reg; > > + int ret; > > > > if (is_c45) > > return get_phy_c45_ids(bus, addr, phy_id, c45_ids); > > > > - /* Grab the bits from PHYIR1, and put them in the upper half */ > > - phy_reg = mdiobus_read(bus, addr, MII_PHYSID1); > > - if (phy_reg < 0) { > > + ret = _get_phy_id(bus, addr, 0, phy_id, false); > > + if (ret < 0) { > > /* returning -ENODEV doesn't stop bus scanning */ > > - return (phy_reg == -EIO || phy_reg == -ENODEV) ? -ENODEV : -EIO; > > + return (ret == -EIO || ret == -ENODEV) ? -ENODEV : -EIO; > > Since ret will only ever be -EIO here, this can only ever return > -ENODEV, which is a functional change in the code (probably unintended.) > Nevertheless, it's likely introducing a bug if the intention is for > some other return from mdiobus_read() to be handled differently. > > > } > > > > - *phy_id = phy_reg << 16; > > - > > - /* Grab the bits from PHYIR2, and put them in the lower half */ > > - phy_reg = mdiobus_read(bus, addr, MII_PHYSID2); > > - if (phy_reg < 0) > > - return -EIO; > > ... whereas this one always returns -EIO on any error. > > So, I think you have the potential in this patch to introduce a subtle > change of behaviour, which may lead to problems - have you closely > analysed why the code was the way it was, and whether your change of > behaviour is actually valid? I could be remembering this wrongly, but i think this is to do with orion_mdio_xsmi_read() returning -ENODEV, not 0xffffffffff, if there is no device on the bus at the given address. -EIO is fatal to the scan, everything stops with the assumption the bus is broken. -ENODEV should not be fatal to the scan. Andrew
On Sat, May 23, 2020 at 09:51:31PM +0200, Andrew Lunn wrote: > > > static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id, > > > struct phy_c45_device_ids *c45_ids) { > > > - int phy_reg; > > > - int i, reg_addr; > > > + int ret; > > > + int i; > > > const int num_ids = ARRAY_SIZE(c45_ids->device_ids); > > > u32 *devs = &c45_ids->devices_in_package; > > > > I feel a "reverse christmas tree" complaint brewing... yes, the original > > code didn't follow it. Maybe a tidy up while touching this code? > > At minimum, a patch should not make it worse. ret and i should clearly > be after devs. > > > > static int get_phy_id(struct mii_bus *bus, int addr, u32 *phy_id, > > > bool is_c45, struct phy_c45_device_ids *c45_ids) > > > { > > > - int phy_reg; > > > + int ret; > > > > > > if (is_c45) > > > return get_phy_c45_ids(bus, addr, phy_id, c45_ids); > > > > > > - /* Grab the bits from PHYIR1, and put them in the upper half */ > > > - phy_reg = mdiobus_read(bus, addr, MII_PHYSID1); > > > - if (phy_reg < 0) { > > > + ret = _get_phy_id(bus, addr, 0, phy_id, false); > > > + if (ret < 0) { > > > /* returning -ENODEV doesn't stop bus scanning */ > > > - return (phy_reg == -EIO || phy_reg == -ENODEV) ? -ENODEV : -EIO; > > > + return (ret == -EIO || ret == -ENODEV) ? -ENODEV : -EIO; > > > > Since ret will only ever be -EIO here, this can only ever return > > -ENODEV, which is a functional change in the code (probably unintended.) > > Nevertheless, it's likely introducing a bug if the intention is for > > some other return from mdiobus_read() to be handled differently. > > > > > } > > > > > > - *phy_id = phy_reg << 16; > > > - > > > - /* Grab the bits from PHYIR2, and put them in the lower half */ > > > - phy_reg = mdiobus_read(bus, addr, MII_PHYSID2); > > > - if (phy_reg < 0) > > > - return -EIO; > > > > ... whereas this one always returns -EIO on any error. > > > > So, I think you have the potential in this patch to introduce a subtle > > change of behaviour, which may lead to problems - have you closely > > analysed why the code was the way it was, and whether your change of > > behaviour is actually valid? > > I could be remembering this wrongly, but i think this is to do with > orion_mdio_xsmi_read() returning -ENODEV, not 0xffffffffff, if there > is no device on the bus at the given address. -EIO is fatal to the > scan, everything stops with the assumption the bus is broken. -ENODEV > should not be fatal to the scan. Maybe orion_mdio_xsmi_read() should be fixed then? Also, maybe adding return code documentation for mdiobus_read() / mdiobus_write() would help MDIO driver authors have some consistency in what errors they are expected to return (does anyone know for certain?)
Hi, On 5/23/20 3:01 PM, Russell King - ARM Linux admin wrote: > On Sat, May 23, 2020 at 09:51:31PM +0200, Andrew Lunn wrote: >>>> static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id, >>>> struct phy_c45_device_ids *c45_ids) { >>>> - int phy_reg; >>>> - int i, reg_addr; >>>> + int ret; >>>> + int i; >>>> const int num_ids = ARRAY_SIZE(c45_ids->device_ids); >>>> u32 *devs = &c45_ids->devices_in_package; >>> >>> I feel a "reverse christmas tree" complaint brewing... yes, the original >>> code didn't follow it. Maybe a tidy up while touching this code? >> >> At minimum, a patch should not make it worse. ret and i should clearly >> be after devs. >> >>>> static int get_phy_id(struct mii_bus *bus, int addr, u32 *phy_id, >>>> bool is_c45, struct phy_c45_device_ids *c45_ids) >>>> { >>>> - int phy_reg; >>>> + int ret; >>>> >>>> if (is_c45) >>>> return get_phy_c45_ids(bus, addr, phy_id, c45_ids); >>>> >>>> - /* Grab the bits from PHYIR1, and put them in the upper half */ >>>> - phy_reg = mdiobus_read(bus, addr, MII_PHYSID1); >>>> - if (phy_reg < 0) { >>>> + ret = _get_phy_id(bus, addr, 0, phy_id, false); >>>> + if (ret < 0) { >>>> /* returning -ENODEV doesn't stop bus scanning */ >>>> - return (phy_reg == -EIO || phy_reg == -ENODEV) ? -ENODEV : -EIO; >>>> + return (ret == -EIO || ret == -ENODEV) ? -ENODEV : -EIO; >>> >>> Since ret will only ever be -EIO here, this can only ever return >>> -ENODEV, which is a functional change in the code (probably unintended.) >>> Nevertheless, it's likely introducing a bug if the intention is for >>> some other return from mdiobus_read() to be handled differently. >>> >>>> } >>>> >>>> - *phy_id = phy_reg << 16; >>>> - >>>> - /* Grab the bits from PHYIR2, and put them in the lower half */ >>>> - phy_reg = mdiobus_read(bus, addr, MII_PHYSID2); >>>> - if (phy_reg < 0) >>>> - return -EIO; >>> >>> ... whereas this one always returns -EIO on any error. >>> >>> So, I think you have the potential in this patch to introduce a subtle >>> change of behaviour, which may lead to problems - have you closely >>> analysed why the code was the way it was, and whether your change of >>> behaviour is actually valid? >> >> I could be remembering this wrongly, but i think this is to do with >> orion_mdio_xsmi_read() returning -ENODEV, not 0xffffffffff, if there >> is no device on the bus at the given address. -EIO is fatal to the >> scan, everything stops with the assumption the bus is broken. -ENODEV >> should not be fatal to the scan. > > Maybe orion_mdio_xsmi_read() should be fixed then? Also, maybe > adding return code documentation for mdiobus_read() / mdiobus_write() > would help MDIO driver authors have some consistency in what > errors they are expected to return (does anyone know for certain?) > My understanding at this point (which is mostly based on the xgmac here), is that 0xffffffff is equivalent to "bus responding correctly, phy failed to respond at this register location" while any -Eerror is understood as "something wrong with bus", and the mdio core then makes a choice about terminating just the current phy search (ENODEV), or terminating the entire mdio bus (basically everything else) registration. I will see about clarifying the docs. I need to see if that will end up being a bit of a rabbit hole before committing to including that in this set. Which brings up the problem that at least xgmac_mdio doesn't appear to handle being told "your bus registration failed" without OOPSing the probe routine. I think Calvin is aware of this, and I believe he has some additional xgmac/etc patches on top of this set. Although he pinged me offline the other day to say that apparently all my hunk shuffling broke some of the c45 phy detection I had working earlier in the week.
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 7746c07b97fe..f0761fa5e40b 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -695,6 +695,29 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus *bus, int addr, int dev_addr, return 0; } +static int _get_phy_id(struct mii_bus *bus, int addr, int dev_addr, + u32 *phy_id, bool c45) +{ + int phy_reg, reg_addr; + + int reg_base = c45 ? (MII_ADDR_C45 | dev_addr << 16) : 0; + + reg_addr = reg_base | MII_PHYSID1; + phy_reg = mdiobus_read(bus, addr, reg_addr); + if (phy_reg < 0) + return -EIO; + + *phy_id = phy_reg << 16; + + reg_addr = reg_base | MII_PHYSID2; + phy_reg = mdiobus_read(bus, addr, reg_addr); + if (phy_reg < 0) + return -EIO; + *phy_id |= phy_reg; + + return 0; +} + static bool valid_phy_id(int val) { return (val > 0 && ((val & 0x1fffffff) != 0x1fffffff)); @@ -715,17 +738,17 @@ static bool valid_phy_id(int val) */ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id, struct phy_c45_device_ids *c45_ids) { - int phy_reg; - int i, reg_addr; + int ret; + int i; const int num_ids = ARRAY_SIZE(c45_ids->device_ids); u32 *devs = &c45_ids->devices_in_package; /* Find first non-zero Devices In package. Device zero is reserved * for 802.3 c45 complied PHYs, so don't probe it at first. */ - for (i = 1; i < num_ids && *devs == 0; i++) { - phy_reg = get_phy_c45_devs_in_pkg(bus, addr, i, devs); - if (phy_reg < 0) + for (i = 0; i < num_ids && *devs == 0; i++) { + ret = get_phy_c45_devs_in_pkg(bus, addr, i, devs); + if (ret < 0) return -EIO; if ((*devs & 0x1fffffff) == 0x1fffffff) { @@ -752,17 +775,9 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id, if (!(c45_ids->devices_in_package & (1 << i))) continue; - reg_addr = MII_ADDR_C45 | i << 16 | MII_PHYSID1; - phy_reg = mdiobus_read(bus, addr, reg_addr); - if (phy_reg < 0) - return -EIO; - c45_ids->device_ids[i] = phy_reg << 16; - - reg_addr = MII_ADDR_C45 | i << 16 | MII_PHYSID2; - phy_reg = mdiobus_read(bus, addr, reg_addr); - if (phy_reg < 0) - return -EIO; - c45_ids->device_ids[i] |= phy_reg; + ret = _get_phy_id(bus, addr, i, &c45_ids->device_ids[i], true); + if (ret < 0) + return ret; } *phy_id = 0; return 0; @@ -787,27 +802,17 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id, static int get_phy_id(struct mii_bus *bus, int addr, u32 *phy_id, bool is_c45, struct phy_c45_device_ids *c45_ids) { - int phy_reg; + int ret; if (is_c45) return get_phy_c45_ids(bus, addr, phy_id, c45_ids); - /* Grab the bits from PHYIR1, and put them in the upper half */ - phy_reg = mdiobus_read(bus, addr, MII_PHYSID1); - if (phy_reg < 0) { + ret = _get_phy_id(bus, addr, 0, phy_id, false); + if (ret < 0) { /* returning -ENODEV doesn't stop bus scanning */ - return (phy_reg == -EIO || phy_reg == -ENODEV) ? -ENODEV : -EIO; + return (ret == -EIO || ret == -ENODEV) ? -ENODEV : -EIO; } - *phy_id = phy_reg << 16; - - /* Grab the bits from PHYIR2, and put them in the lower half */ - phy_reg = mdiobus_read(bus, addr, MII_PHYSID2); - if (phy_reg < 0) - return -EIO; - - *phy_id |= phy_reg; - return 0; }
Lets factor out the phy id logic, and make it generic so that it can be used for c22 and c45. Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> --- drivers/net/phy/phy_device.c | 65 +++++++++++++++++++----------------- 1 file changed, 35 insertions(+), 30 deletions(-)