Re: [PATCH RFC 4/7] net: phy: add support for probing MMDs >= 8 for devices-in-package

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up