On Tue, May 26, 2020 at 04:47:54PM +0100, Russell King - ARM Linux admin wrote:
> On Tue, May 26, 2020 at 10:39:08AM -0500, Jeremy Linton wrote:
> > Hi,
> >
> > On 5/26/20 9:31 AM, Russell King wrote:
> > > We have two competing requirements for the devices_in_package field.
> > > We want to use it as a bit array indicating which MMDs are present, but
> > > we also want to know if the Clause 22 registers are present.
> > >
> > > Since "devices in package" is a term used in the 802.3 specification,
> > > keep this as the as-specified values read from the PHY, and introduce
> > > a new member "mmds_present" to indicate which MMDs are actually
> > > present in the PHY, derived from the "devices in package" value.
> > >
> > > Signed-off-by: Russell King <rmk+ker...@armlinux.org.uk>
> > > ---
> > > drivers/net/phy/phy-c45.c | 4 ++--
> > > drivers/net/phy/phy_device.c | 6 +++---
> > > drivers/net/phy/phylink.c | 8 ++++----
> > > include/linux/phy.h | 4 +++-
> > > 4 files changed, 12 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
> > > index 8cd952950a75..4b5805e2bd0e 100644
> > > --- a/drivers/net/phy/phy-c45.c
> > > +++ b/drivers/net/phy/phy-c45.c
> > > @@ -219,7 +219,7 @@ int genphy_c45_read_link(struct phy_device *phydev)
> > > int val, devad;
> > > bool link = true;
> > > - if (phydev->c45_ids.devices_in_package & MDIO_DEVS_AN) {
> > > + if (phydev->c45_ids.mmds_present & MDIO_DEVS_AN) {
> > > val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_CTRL1);
> > > if (val < 0)
> > > return val;
> > > @@ -397,7 +397,7 @@ int genphy_c45_pma_read_abilities(struct phy_device
> > > *phydev)
> > > int val;
> > > linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
> > > phydev->supported);
> > > - if (phydev->c45_ids.devices_in_package & MDIO_DEVS_AN) {
> > > + if (phydev->c45_ids.mmds_present & MDIO_DEVS_AN) {
> > > val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
> > > if (val < 0)
> > > return val;
> > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > > index a483d79cfc87..1c948bbf4fa0 100644
> > > --- a/drivers/net/phy/phy_device.c
> > > +++ b/drivers/net/phy/phy_device.c
> > > @@ -707,9 +707,6 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus
> > > *bus, int addr, int dev_addr,
> > > return -EIO;
> > > *devices_in_package |= phy_reg;
> > > - /* Bit 0 doesn't represent a device, it indicates c22 regs presence */
> > > - *devices_in_package &= ~BIT(0);
> > > -
> > > return 0;
> > > }
> > > @@ -788,6 +785,8 @@ static int get_phy_c45_ids(struct mii_bus *bus, int
> > > addr, u32 *phy_id,
> > > }
> > > c45_ids->devices_in_package = devs_in_pkg;
> > > + /* Bit 0 doesn't represent a device, it indicates c22 regs presence */
> > > + c45_ids->mmds_present = devs_in_pkg & ~BIT(0);
> > > *phy_id = 0;
> > > return 0;
> > > @@ -842,6 +841,7 @@ struct phy_device *get_phy_device(struct mii_bus
> > > *bus, int addr, bool is_c45)
> > > int r;
> > > c45_ids.devices_in_package = 0;
> > > + c45_ids.mmds_present = 0;
> > > memset(c45_ids.device_ids, 0xff, sizeof(c45_ids.device_ids));
> > > if (is_c45)
> > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > > index 6defd5eddd58..b548e0418694 100644
> > > --- a/drivers/net/phy/phylink.c
> > > +++ b/drivers/net/phy/phylink.c
> > > @@ -1709,11 +1709,11 @@ static int phylink_phy_read(struct phylink *pl,
> > > unsigned int phy_id,
> > > case MII_BMSR:
> > > case MII_PHYSID1:
> > > case MII_PHYSID2:
> > > - devad = __ffs(phydev->c45_ids.devices_in_package);
> > > + devad = __ffs(phydev->c45_ids.mmds_present);
> > > break;
> > > case MII_ADVERTISE:
> > > case MII_LPA:
> > > - if (!(phydev->c45_ids.devices_in_package &
> > > MDIO_DEVS_AN))
> > > + if (!(phydev->c45_ids.mmds_present & MDIO_DEVS_AN))
> > > return -EINVAL;
> > > devad = MDIO_MMD_AN;
> > > if (reg == MII_ADVERTISE)
> > > @@ -1749,11 +1749,11 @@ static int phylink_phy_write(struct phylink *pl,
> > > unsigned int phy_id,
> > > case MII_BMSR:
> > > case MII_PHYSID1:
> > > case MII_PHYSID2:
> > > - devad = __ffs(phydev->c45_ids.devices_in_package);
> > > + devad = __ffs(phydev->c45_ids.mmds_present);
> > > break;
> > > case MII_ADVERTISE:
> > > case MII_LPA:
> > > - if (!(phydev->c45_ids.devices_in_package &
> > > MDIO_DEVS_AN))
> > > + if (!(phydev->c45_ids.mmds_present & MDIO_DEVS_AN))
> > > return -EINVAL;
> > > devad = MDIO_MMD_AN;
> > > if (reg == MII_ADVERTISE)
> > > diff --git a/include/linux/phy.h b/include/linux/phy.h
> > > index 41c046545354..0d41c710339a 100644
> > > --- a/include/linux/phy.h
> > > +++ b/include/linux/phy.h
> > > @@ -354,11 +354,13 @@ enum phy_state {
> > > /**
> > > * struct phy_c45_device_ids - 802.3-c45 Device Identifiers
> > > - * @devices_in_package: Bit vector of devices present.
> > > + * @devices_in_package: IEEE 802.3 devices in package register value.
> > > + * @mmds_present: bit vector of MMDs present.
> > > * @device_ids: The device identifer for each present device.
> > > */
> > > struct phy_c45_device_ids {
> > > u32 devices_in_package;
> > > + u32 mmds_present;
> > > u32 device_ids[8];
> > > };
> >
> > It seems like the majority of the devices_in_package accessors are just bit
> > masking for a given MMD/field. The case that has the problem is the __ffs()
> > calls which failed to account for this. So why not just fix those two cases
> > instead of creating a duplicate field with exactly the same data in it minus
> > a bit.
>
> I think I explained that in the commit message...
I should also point out that we may wish to clear bits in mmds_present
when we scan the IDs and discover STAT2 registers that indicate that
the MMD is not implemented, while leaving the devices_in_package
field unaltered.
To do that would be a behavioural change, which is something that
this series is trying to avoid.
--
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
