| Message ID | E1jdabs-0005sW-P8@rmk-PC.armlinux.org.uk |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | Clause 45 PHY probing cleanups | expand |
On Tue, May 26, 2020 at 03:31:16PM +0100, Russell King wrote: > Add support for probing MMDs above 7 for a valid devices-in-package > specifier. > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > --- > drivers/net/phy/phy_device.c | 39 ++++++++++++++++++++++++++++++++++-- > include/linux/phy.h | 2 ++ > 2 files changed, 39 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index 0d6b6ca66216..fa9164ac0f3d 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -659,6 +659,28 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id, > } > EXPORT_SYMBOL(phy_device_create); > > +/* phy_c45_probe_present - checks to see if a MMD is present in the package > + * @bus: the target MII bus > + * @prtad: PHY package address on the MII bus > + * @devad: PHY device (MMD) address > + * > + * Read the MDIO_STAT2 register, and check whether a device is responding > + * at this address. > + * > + * Returns: negative error number on bus access error, zero if no device > + * is responding, or positive if a device is present. > + */ > +static int phy_c45_probe_present(struct mii_bus *bus, int prtad, int devad) > +{ > + int stat2; > + > + stat2 = mdiobus_c45_read(bus, prtad, devad, MDIO_STAT2); > + if (stat2 < 0) > + return stat2; > + > + return (stat2 & MDIO_STAT2_DEVPRST) == MDIO_STAT2_DEVPRST_VAL; > +} > + > /* get_phy_c45_devs_in_pkg - reads a MMD's devices in package registers. > * @bus: the target MII bus > * @addr: PHY address on the MII bus > @@ -709,12 +731,25 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id, > { > const int num_ids = ARRAY_SIZE(c45_ids->device_ids); > u32 *devs = &c45_ids->devices_in_package; > - int i, phy_reg; > + int i, ret, phy_reg; > > /* 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++) { > + for (i = 1; i < MDIO_MMD_NUM && *devs == 0; i++) { > + if (i >= 8) { > + /* Only probe for the devices-in-package if there > + * is a PHY reporting as present here; this avoids > + * picking up on PHYs that implement non-IEEE802.3 > + * compliant register spaces. > + */ > + ret = phy_c45_probe_present(bus, addr, i); > + if (ret < 0) > + return -EIO; > + > + if (!ret) > + continue; > + } A second look at 802.3, this can't be done for all MMDs (which becomes visible when I look at the results from the 88x3310.) Only MMDs 1, 2, 3, 4, 5, 30 and 31 are defined to have this register with the "Device Present" bit pair.
Hi, On 5/26/20 12:14 PM, Russell King - ARM Linux admin wrote: > On Tue, May 26, 2020 at 03:31:16PM +0100, Russell King wrote: >> Add support for probing MMDs above 7 for a valid devices-in-package >> specifier. >> >> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> >> --- >> drivers/net/phy/phy_device.c | 39 ++++++++++++++++++++++++++++++++++-- >> include/linux/phy.h | 2 ++ >> 2 files changed, 39 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c >> index 0d6b6ca66216..fa9164ac0f3d 100644 >> --- a/drivers/net/phy/phy_device.c >> +++ b/drivers/net/phy/phy_device.c >> @@ -659,6 +659,28 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id, >> } >> EXPORT_SYMBOL(phy_device_create); >> >> +/* phy_c45_probe_present - checks to see if a MMD is present in the package >> + * @bus: the target MII bus >> + * @prtad: PHY package address on the MII bus >> + * @devad: PHY device (MMD) address >> + * >> + * Read the MDIO_STAT2 register, and check whether a device is responding >> + * at this address. >> + * >> + * Returns: negative error number on bus access error, zero if no device >> + * is responding, or positive if a device is present. >> + */ >> +static int phy_c45_probe_present(struct mii_bus *bus, int prtad, int devad) >> +{ >> + int stat2; >> + >> + stat2 = mdiobus_c45_read(bus, prtad, devad, MDIO_STAT2); >> + if (stat2 < 0) >> + return stat2; >> + >> + return (stat2 & MDIO_STAT2_DEVPRST) == MDIO_STAT2_DEVPRST_VAL; >> +} >> + >> /* get_phy_c45_devs_in_pkg - reads a MMD's devices in package registers. >> * @bus: the target MII bus >> * @addr: PHY address on the MII bus >> @@ -709,12 +731,25 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id, >> { >> const int num_ids = ARRAY_SIZE(c45_ids->device_ids); >> u32 *devs = &c45_ids->devices_in_package; >> - int i, phy_reg; >> + int i, ret, phy_reg; >> >> /* 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++) { >> + for (i = 1; i < MDIO_MMD_NUM && *devs == 0; i++) { >> + if (i >= 8) { >> + /* Only probe for the devices-in-package if there >> + * is a PHY reporting as present here; this avoids >> + * picking up on PHYs that implement non-IEEE802.3 >> + * compliant register spaces. >> + */ >> + ret = phy_c45_probe_present(bus, addr, i); >> + if (ret < 0) >> + return -EIO; >> + >> + if (!ret) >> + continue; >> + } > > A second look at 802.3, this can't be done for all MMDs (which becomes > visible when I look at the results from the 88x3310.) Only MMDs 1, 2, > 3, 4, 5, 30 and 31 are defined to have this register with the "Device > Present" bit pair. > I'm not sure it helps, but my thought process following some of the discussion last night was: something to the effect: for (i = 1; i < MDIO_MMD_NUM && *devs == 0; i++) { + if (i & RESERVED_MMDS) + continue; where RESERVED_MMDS was a hardcoded bitfield matching the IEEE reserved MMDs. or maybe a "IGNORE_MMDS" which also includes BIT0 and other MMDs the code doesn't understand.
On Tue, May 26, 2020 at 12:20:10PM -0500, Jeremy Linton wrote: > Hi, > > On 5/26/20 12:14 PM, Russell King - ARM Linux admin wrote: > > On Tue, May 26, 2020 at 03:31:16PM +0100, Russell King wrote: > > > Add support for probing MMDs above 7 for a valid devices-in-package > > > specifier. > > > > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > > > --- > > > drivers/net/phy/phy_device.c | 39 ++++++++++++++++++++++++++++++++++-- > > > include/linux/phy.h | 2 ++ > > > 2 files changed, 39 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > > > index 0d6b6ca66216..fa9164ac0f3d 100644 > > > --- a/drivers/net/phy/phy_device.c > > > +++ b/drivers/net/phy/phy_device.c > > > @@ -659,6 +659,28 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id, > > > } > > > EXPORT_SYMBOL(phy_device_create); > > > +/* phy_c45_probe_present - checks to see if a MMD is present in the package > > > + * @bus: the target MII bus > > > + * @prtad: PHY package address on the MII bus > > > + * @devad: PHY device (MMD) address > > > + * > > > + * Read the MDIO_STAT2 register, and check whether a device is responding > > > + * at this address. > > > + * > > > + * Returns: negative error number on bus access error, zero if no device > > > + * is responding, or positive if a device is present. > > > + */ > > > +static int phy_c45_probe_present(struct mii_bus *bus, int prtad, int devad) > > > +{ > > > + int stat2; > > > + > > > + stat2 = mdiobus_c45_read(bus, prtad, devad, MDIO_STAT2); > > > + if (stat2 < 0) > > > + return stat2; > > > + > > > + return (stat2 & MDIO_STAT2_DEVPRST) == MDIO_STAT2_DEVPRST_VAL; > > > +} > > > + > > > /* get_phy_c45_devs_in_pkg - reads a MMD's devices in package registers. > > > * @bus: the target MII bus > > > * @addr: PHY address on the MII bus > > > @@ -709,12 +731,25 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id, > > > { > > > const int num_ids = ARRAY_SIZE(c45_ids->device_ids); > > > u32 *devs = &c45_ids->devices_in_package; > > > - int i, phy_reg; > > > + int i, ret, phy_reg; > > > /* 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++) { > > > + for (i = 1; i < MDIO_MMD_NUM && *devs == 0; i++) { > > > + if (i >= 8) { > > > + /* Only probe for the devices-in-package if there > > > + * is a PHY reporting as present here; this avoids > > > + * picking up on PHYs that implement non-IEEE802.3 > > > + * compliant register spaces. > > > + */ > > > + ret = phy_c45_probe_present(bus, addr, i); > > > + if (ret < 0) > > > + return -EIO; > > > + > > > + if (!ret) > > > + continue; > > > + } > > > > A second look at 802.3, this can't be done for all MMDs (which becomes > > visible when I look at the results from the 88x3310.) Only MMDs 1, 2, > > 3, 4, 5, 30 and 31 are defined to have this register with the "Device > > Present" bit pair. > > > > I'm not sure it helps, but my thought process following some of the > discussion last night was: > > something to the effect: > > for (i = 1; i < MDIO_MMD_NUM && *devs == 0; i++) { > + if (i & RESERVED_MMDS) > + continue; > > where RESERVED_MMDS was a hardcoded bitfield matching the IEEE reserved > MMDs. or maybe a "IGNORE_MMDS" which also includes BIT0 and other MMDs the > code doesn't understand. That seems to be walking into a mine field - which MMDs are reserved depends on which revision of the IEEE 802.3 specification you look at: Spec version Reserved MMDs 2012, 2015 0, 12-28 2018 0, 14-28 and as technology progresses, it's likely more MMDs will no longer be reserved. There is another problem: 802.3 explicitly defines the devices in package registers for MMD 1, 2, 3, 4, 5, 6, 7, and 29, but then goes on to say: "Each MMD contains registers 5 and 6, as defined in Table 45-2." which seems rather contradictory. There is also the problem that some PHYs have different values in their devices-in-package register for each MMD: MMD devices-in-package 1,3,7 c000009a 4 4000001a 30 00000000 (device present = not present) 31 fffe0000 (device present = not present) What fun. Thankfully, MMD1 will be read first, so it doesn't cause us a problem.
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 0d6b6ca66216..fa9164ac0f3d 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -659,6 +659,28 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id, } EXPORT_SYMBOL(phy_device_create); +/* phy_c45_probe_present - checks to see if a MMD is present in the package + * @bus: the target MII bus + * @prtad: PHY package address on the MII bus + * @devad: PHY device (MMD) address + * + * Read the MDIO_STAT2 register, and check whether a device is responding + * at this address. + * + * Returns: negative error number on bus access error, zero if no device + * is responding, or positive if a device is present. + */ +static int phy_c45_probe_present(struct mii_bus *bus, int prtad, int devad) +{ + int stat2; + + stat2 = mdiobus_c45_read(bus, prtad, devad, MDIO_STAT2); + if (stat2 < 0) + return stat2; + + return (stat2 & MDIO_STAT2_DEVPRST) == MDIO_STAT2_DEVPRST_VAL; +} + /* get_phy_c45_devs_in_pkg - reads a MMD's devices in package registers. * @bus: the target MII bus * @addr: PHY address on the MII bus @@ -709,12 +731,25 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id, { const int num_ids = ARRAY_SIZE(c45_ids->device_ids); u32 *devs = &c45_ids->devices_in_package; - int i, phy_reg; + int i, ret, phy_reg; /* 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++) { + for (i = 1; i < MDIO_MMD_NUM && *devs == 0; i++) { + if (i >= 8) { + /* Only probe for the devices-in-package if there + * is a PHY reporting as present here; this avoids + * picking up on PHYs that implement non-IEEE802.3 + * compliant register spaces. + */ + ret = phy_c45_probe_present(bus, addr, i); + if (ret < 0) + return -EIO; + + if (!ret) + continue; + } phy_reg = get_phy_c45_devs_in_pkg(bus, addr, i, devs); if (phy_reg < 0) return -EIO; diff --git a/include/linux/phy.h b/include/linux/phy.h index 9b7c46cf14d3..41c046545354 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -350,6 +350,8 @@ enum phy_state { PHY_NOLINK, }; +#define MDIO_MMD_NUM 32 + /** * struct phy_c45_device_ids - 802.3-c45 Device Identifiers * @devices_in_package: Bit vector of devices present.
Add support for probing MMDs above 7 for a valid devices-in-package specifier. Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> --- drivers/net/phy/phy_device.c | 39 ++++++++++++++++++++++++++++++++++-- include/linux/phy.h | 2 ++ 2 files changed, 39 insertions(+), 2 deletions(-)