| Message ID | 20220323183419.2278676-3-michael@walle.cc (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: C45-over-C22 access | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 78 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
> +static int mdiobus_probe_mmd_read(struct mii_bus *bus, int prtad, int devad, > + u16 regnum) > +{ > + int ret; > + > + /* For backwards compatibility, treat MDIOBUS_NO_CAP as c45 capable */ > + if (bus->probe_capabilities == MDIOBUS_NO_CAP || > + bus->probe_capabilities >= MDIOBUS_C45) Maybe we should do the work and mark up those that are C45 capable. At a quick count, see 16 of them. > + return mdiobus_c45_read(bus, prtad, devad, regnum); > + > + mutex_lock(&bus->mdio_lock); > + > + /* Write the desired MMD Devad */ > + ret = __mdiobus_write(bus, prtad, MII_MMD_CTRL, devad); > + if (ret) > + goto out; > + > + /* Write the desired MMD register address */ > + ret = __mdiobus_write(bus, prtad, MII_MMD_DATA, regnum); > + if (ret) > + goto out; > + > + /* Select the Function : DATA with no post increment */ > + ret = __mdiobus_write(bus, prtad, MII_MMD_CTRL, > + devad | MII_MMD_CTRL_NOINCR); > + if (ret) > + goto out; Make mmd_phy_indirect() usable, rather then repeat it. Andrew
Am 2022-03-23 20:39, schrieb Andrew Lunn: >> +static int mdiobus_probe_mmd_read(struct mii_bus *bus, int prtad, int >> devad, >> + u16 regnum) >> +{ >> + int ret; >> + >> + /* For backwards compatibility, treat MDIOBUS_NO_CAP as c45 capable >> */ >> + if (bus->probe_capabilities == MDIOBUS_NO_CAP || >> + bus->probe_capabilities >= MDIOBUS_C45) > > Maybe we should do the work and mark up those that are C45 capable. At > a quick count, see 16 of them. You mean look at they are MDIOBUS_C45, MDIOBUS_C22_C45 or MDIOBUS_C22 and drop MDIOBUS_NO_CAP? > >> + return mdiobus_c45_read(bus, prtad, devad, regnum); >> + >> + mutex_lock(&bus->mdio_lock); >> + >> + /* Write the desired MMD Devad */ >> + ret = __mdiobus_write(bus, prtad, MII_MMD_CTRL, devad); >> + if (ret) >> + goto out; >> + >> + /* Write the desired MMD register address */ >> + ret = __mdiobus_write(bus, prtad, MII_MMD_DATA, regnum); >> + if (ret) >> + goto out; >> + >> + /* Select the Function : DATA with no post increment */ >> + ret = __mdiobus_write(bus, prtad, MII_MMD_CTRL, >> + devad | MII_MMD_CTRL_NOINCR); >> + if (ret) >> + goto out; > > Make mmd_phy_indirect() usable, rather then repeat it. I actually had that. But mmd_phy_indirect() doesn't check the return code and neither does the __phy_write_mmd() it actually deliberatly sets "ret = 0". So I wasn't sure. If you are fine with a changed code flow in the error case, then sure. I.e. mmd_phy_indirect() always (try to) do three accesses; with error checks it might end after the first. If you are fine with the error checks, should __phy_write_mmd() also check the last mdiobus_write()? -michael
Am 2022-03-23 20:39, schrieb Andrew Lunn: >> +static int mdiobus_probe_mmd_read(struct mii_bus *bus, int prtad, int >> devad, >> + u16 regnum) >> +{ >> + int ret; >> + >> + /* For backwards compatibility, treat MDIOBUS_NO_CAP as c45 capable >> */ >> + if (bus->probe_capabilities == MDIOBUS_NO_CAP || >> + bus->probe_capabilities >= MDIOBUS_C45) > > Maybe we should do the work and mark up those that are C45 capable. At > a quick count, see 16 of them. I guess you grepped for MII_ADDR_C45 and had a look who actually handled it correctly. Correct? Let's say we mark these as either MDIOBUS_C45 or MDIOBUS_C45_C22, can we then drop MDIOBUS_NO_CAP and make MDIOBUS_C22 the default value (i.e. value 0) or do we have to go through all the mdio drivers and add bus->probe_capabilities = MDIOBUS_C22 ? Grepping for {of_,}mdiobus_register lists quite a few of them. -michael
On Thu, Mar 24, 2022 at 03:28:43PM +0100, Michael Walle wrote: > Am 2022-03-23 20:39, schrieb Andrew Lunn: > > > +static int mdiobus_probe_mmd_read(struct mii_bus *bus, int prtad, > > > int devad, > > > + u16 regnum) > > > +{ > > > + int ret; > > > + > > > + /* For backwards compatibility, treat MDIOBUS_NO_CAP as c45 > > > capable */ > > > + if (bus->probe_capabilities == MDIOBUS_NO_CAP || > > > + bus->probe_capabilities >= MDIOBUS_C45) > > > > Maybe we should do the work and mark up those that are C45 capable. At > > a quick count, see 16 of them. > > I guess you grepped for MII_ADDR_C45 and had a look who > actually handled it correctly. Correct? Yes. > Let's say we mark these as either MDIOBUS_C45 or MDIOBUS_C45_C22, > can we then drop MDIOBUS_NO_CAP and make MDIOBUS_C22 the default > value (i.e. value 0) or do we have to go through all the mdio drivers > and add bus->probe_capabilities = MDIOBUS_C22 ? Grepping for > {of_,}mdiobus_register lists quite a few of them. The minimum is marking those that support C45 with MDIOBUS_C45 or MDIOBUS_C45_C22. We can then really trust it does C45. Those that don't set probe_capabilities we assume are C22 only. That should be enough for this problem. FYI: Yesterday i started actually adding probe_capabilities values to drivers. I did everything in driver/net/mdio. I will work on the rest over the next few days and then post an RFC patchset. Andrew
On Wed, Mar 23, 2022 at 11:14:11PM +0100, Michael Walle wrote: > I actually had that. But mmd_phy_indirect() doesn't check > the return code and neither does the __phy_write_mmd() it > actually deliberatly sets "ret = 0". So I wasn't sure. If you > are fine with a changed code flow in the error case, then sure. > I.e. mmd_phy_indirect() always (try to) do three accesses; with > error checks it might end after the first. If you are fine > with the error checks, should __phy_write_mmd() also check the > last mdiobus_write()? The reason for that goes back to commit a59a4d1921664da63d801ba477950114c71c88c9 phy: add the EEE support and the way to access to the MMD registers. and to maintain compatibility with that; if we start checking for errors now, we might trigger a kernel regression sadly.
Am 2022-03-30 18:18, schrieb Russell King (Oracle): > On Wed, Mar 23, 2022 at 11:14:11PM +0100, Michael Walle wrote: >> I actually had that. But mmd_phy_indirect() doesn't check >> the return code and neither does the __phy_write_mmd() it >> actually deliberatly sets "ret = 0". So I wasn't sure. If you >> are fine with a changed code flow in the error case, then sure. >> I.e. mmd_phy_indirect() always (try to) do three accesses; with >> error checks it might end after the first. If you are fine >> with the error checks, should __phy_write_mmd() also check the >> last mdiobus_write()? > > The reason for that goes back to > commit a59a4d1921664da63d801ba477950114c71c88c9 > phy: add the EEE support and the way to access to the MMD > registers. > > and to maintain compatibility with that; if we start checking for > errors now, we might trigger a kernel regression sadly. I see that this is the commit which introduced the mmd_phy_indirect() function, but I don't see why there is no return code checking. Unlike now, there is a check for the last read (the one who reads MII_MMD_DATA). That read which might return garbage if any write has failed before - or if the bus is completely dead, return an error. Current code will just return 0. In any case, I don't have a strong opinion here. I just don't see how that function could be reused while adding error checks and without making it ugly, so I've just duplicated it. Maybe something like this: static int __phy_mmd_indirect_common(struct mii_bus *bus, int prtad, int devad, int addr, bool check_rc) { int ret; /* Write the desired MMD Devad */ ret = __mdiobus_write(bus, phy_addr, MII_MMD_CTRL, devad); if (check_rc && ret) return ret; /* Write the desired MMD register address */ ret = __mdiobus_write(bus, phy_addr, MII_MMD_DATA, regnum); if (check_rc && ret) return ret; /* Select the Function : DATA with no post increment */ ret = __mdiobus_write(bus, phy_addr, MII_MMD_CTRL, devad | MII_MMD_CTRL_NOINCR); if (check_rc && ret) return ret; return 0; } int __phy_mmd_indirect(struct mii_bus *bus, int prtad, int devad, int addr) { return __phy_mmd_indirect_common(bus, prtad, devad, addr, true); } /* some function doc about deliberatly no error checking.. */ void __phy_mmd_indirect_legacy(struct mii_bus *bus, int prtad, int devad, int addr) { __phy_mmd_indirect_common(bus, prtad, devad, addr, false); } should the last two functions be static inline? -michael
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 8406ac739def..c766f5bb421a 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -649,6 +649,42 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id, } EXPORT_SYMBOL(phy_device_create); +static int mdiobus_probe_mmd_read(struct mii_bus *bus, int prtad, int devad, + u16 regnum) +{ + int ret; + + /* For backwards compatibility, treat MDIOBUS_NO_CAP as c45 capable */ + if (bus->probe_capabilities == MDIOBUS_NO_CAP || + bus->probe_capabilities >= MDIOBUS_C45) + return mdiobus_c45_read(bus, prtad, devad, regnum); + + mutex_lock(&bus->mdio_lock); + + /* Write the desired MMD Devad */ + ret = __mdiobus_write(bus, prtad, MII_MMD_CTRL, devad); + if (ret) + goto out; + + /* Write the desired MMD register address */ + ret = __mdiobus_write(bus, prtad, MII_MMD_DATA, regnum); + if (ret) + goto out; + + /* Select the Function : DATA with no post increment */ + ret = __mdiobus_write(bus, prtad, MII_MMD_CTRL, + devad | MII_MMD_CTRL_NOINCR); + if (ret) + goto out; + + ret = __mdiobus_read(bus, prtad, MII_MMD_DATA); + +out: + mutex_unlock(&bus->mdio_lock); + + return ret; +} + /* 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 @@ -664,7 +700,7 @@ 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); + stat2 = mdiobus_probe_mmd_read(bus, prtad, devad, MDIO_STAT2); if (stat2 < 0) return stat2; @@ -687,12 +723,12 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus *bus, int addr, int dev_addr, { int phy_reg; - phy_reg = mdiobus_c45_read(bus, addr, dev_addr, MDIO_DEVS2); + phy_reg = mdiobus_probe_mmd_read(bus, addr, dev_addr, MDIO_DEVS2); if (phy_reg < 0) return -EIO; *devices_in_package = phy_reg << 16; - phy_reg = mdiobus_c45_read(bus, addr, dev_addr, MDIO_DEVS1); + phy_reg = mdiobus_probe_mmd_read(bus, addr, dev_addr, MDIO_DEVS1); if (phy_reg < 0) return -EIO; *devices_in_package |= phy_reg; @@ -776,12 +812,12 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, continue; } - phy_reg = mdiobus_c45_read(bus, addr, i, MII_PHYSID1); + phy_reg = mdiobus_probe_mmd_read(bus, addr, i, MII_PHYSID1); if (phy_reg < 0) return -EIO; c45_ids->device_ids[i] = phy_reg << 16; - phy_reg = mdiobus_c45_read(bus, addr, i, MII_PHYSID2); + phy_reg = mdiobus_probe_mmd_read(bus, addr, i, MII_PHYSID2); if (phy_reg < 0) return -EIO; c45_ids->device_ids[i] |= phy_reg;
There are some PHYs, namely the Maxlinear GPY215, whose driver is explicitly supporting C45-over-C22 access. At least that was the intention. In practice, it cannot work because get_phy_c45_ids() will always issue c45 register accesses. There is another issue at hand: the Microchip LAN8814, which is a c22 only quad PHY, has issues with c45 accesses on the same bus and its address decoder will find a match in the middle of another c45 transaction. This will lead to spurious reads and writes. The reads will corrupt the c45 in flight. The write will lead to random writes to the LAN8814 registers. As a workaround for PHYs which support C45-over-C22 register accesses, we can make the MDIO bus c22-only. For both reasons, extend the register accesses in get_phy_c45_ids() to allow indirect accesses, indicated by the bus->probe_capabilities bits. The probe_capabilites can then be degraded by a device tree property, for example. Or it will just work when the MDIO driver is c22-only and set the capabilities accordingly. Signed-off-by: Michael Walle <michael@walle.cc> --- drivers/net/phy/phy_device.c | 46 ++++++++++++++++++++++++++++++++---- 1 file changed, 41 insertions(+), 5 deletions(-)