| Message ID | 20231126003748.9600-3-ansuelsmth@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,1/3] net: phy: extend PHY package API to support multiple global address | expand |
On 11/25/2023 4:37 PM, Christian Marangi wrote: > Some PHY in PHY package may require to read/write MMD regs to correctly > configure the PHY package. > > Add support for these additional required function in both lock and no > lock variant. > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > --- > include/linux/phy.h | 74 +++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 74 insertions(+) > > diff --git a/include/linux/phy.h b/include/linux/phy.h > index 984bca9a82f4..1799133c8387 100644 > --- a/include/linux/phy.h > +++ b/include/linux/phy.h > @@ -2067,6 +2067,80 @@ static inline int __phy_package_write(struct phy_device *phydev, > return __mdiobus_write(phydev->mdio.bus, addr, regnum, val); > } > > +static inline int phy_package_read_mmd(struct phy_device *phydev, > + unsigned int addr_offset, int devad, > + u32 regnum) > +{ > + struct phy_package_shared *shared = phydev->shared; > + struct mii_bus *bus = phydev->mdio.bus; > + int addr, val; > + > + if (!shared || shared->base_addr + addr_offset > PHY_MAX_ADDR) > + return -EIO; You might be off by one here, should not that >= PHY_MAX_ADDR here and below?
On Sat, Nov 25, 2023 at 04:52:19PM -0800, Florian Fainelli wrote: > > > On 11/25/2023 4:37 PM, Christian Marangi wrote: > > Some PHY in PHY package may require to read/write MMD regs to correctly > > configure the PHY package. > > > > Add support for these additional required function in both lock and no > > lock variant. > > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > > --- > > include/linux/phy.h | 74 +++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 74 insertions(+) > > > > diff --git a/include/linux/phy.h b/include/linux/phy.h > > index 984bca9a82f4..1799133c8387 100644 > > --- a/include/linux/phy.h > > +++ b/include/linux/phy.h > > @@ -2067,6 +2067,80 @@ static inline int __phy_package_write(struct phy_device *phydev, > > return __mdiobus_write(phydev->mdio.bus, addr, regnum, val); > > } > > +static inline int phy_package_read_mmd(struct phy_device *phydev, > > + unsigned int addr_offset, int devad, > > + u32 regnum) > > +{ > > + struct phy_package_shared *shared = phydev->shared; > > + struct mii_bus *bus = phydev->mdio.bus; > > + int addr, val; > > + > > + if (!shared || shared->base_addr + addr_offset > PHY_MAX_ADDR) > > + return -EIO; > > You might be off by one here, should not that >= PHY_MAX_ADDR here and > below? Thanks for the review. Yes PHY_MAX_ADDR is 32 so I should use >=. (interesting choice to use 32 instead of 31 as MAX, guess an old mistake)
On 11/25/2023 4:54 PM, Christian Marangi wrote: > On Sat, Nov 25, 2023 at 04:52:19PM -0800, Florian Fainelli wrote: >> >> >> On 11/25/2023 4:37 PM, Christian Marangi wrote: >>> Some PHY in PHY package may require to read/write MMD regs to correctly >>> configure the PHY package. >>> >>> Add support for these additional required function in both lock and no >>> lock variant. >>> >>> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> >>> --- >>> include/linux/phy.h | 74 +++++++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 74 insertions(+) >>> >>> diff --git a/include/linux/phy.h b/include/linux/phy.h >>> index 984bca9a82f4..1799133c8387 100644 >>> --- a/include/linux/phy.h >>> +++ b/include/linux/phy.h >>> @@ -2067,6 +2067,80 @@ static inline int __phy_package_write(struct phy_device *phydev, >>> return __mdiobus_write(phydev->mdio.bus, addr, regnum, val); >>> } >>> +static inline int phy_package_read_mmd(struct phy_device *phydev, >>> + unsigned int addr_offset, int devad, >>> + u32 regnum) >>> +{ >>> + struct phy_package_shared *shared = phydev->shared; >>> + struct mii_bus *bus = phydev->mdio.bus; >>> + int addr, val; >>> + >>> + if (!shared || shared->base_addr + addr_offset > PHY_MAX_ADDR) >>> + return -EIO; >> >> You might be off by one here, should not that >= PHY_MAX_ADDR here and >> below? > > Thanks for the review. Yes PHY_MAX_ADDR is 32 so I should use >=. > > (interesting choice to use 32 instead of 31 as MAX, guess an old mistake) It has historically been used as an iterator upper bound, hence the 32.
On Sun, Nov 26, 2023 at 01:37:48AM +0100, Christian Marangi wrote: > Some PHY in PHY package may require to read/write MMD regs to correctly > configure the PHY package. > > Add support for these additional required function in both lock and no > lock variant. You are assuming the PHY only supports C45 over C22. But what about those PHYs which have native C45? And maybe don't have C22 at all? You should refactor the code of __phy_read_mmd() into a helper and use it here. Andrew
On Sun, Nov 26, 2023 at 07:14:52PM +0100, Andrew Lunn wrote: > On Sun, Nov 26, 2023 at 01:37:48AM +0100, Christian Marangi wrote: > > Some PHY in PHY package may require to read/write MMD regs to correctly > > configure the PHY package. > > > > Add support for these additional required function in both lock and no > > lock variant. > > You are assuming the PHY only supports C45 over C22. But what about > those PHYs which have native C45? And maybe don't have C22 at all? > > You should refactor the code of __phy_read_mmd() into a helper and use > it here. > Have to be honest here and chose the ""easy"" way. Was a little scared by all the complexity with mmd with C45 and C22... The idea was to add C22 support for now and if used by others extend the function. But I guess with the generic approach we are taking it's better to handle it now. Will update this in v2.
diff --git a/include/linux/phy.h b/include/linux/phy.h index 984bca9a82f4..1799133c8387 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -2067,6 +2067,80 @@ static inline int __phy_package_write(struct phy_device *phydev, return __mdiobus_write(phydev->mdio.bus, addr, regnum, val); } +static inline int phy_package_read_mmd(struct phy_device *phydev, + unsigned int addr_offset, int devad, + u32 regnum) +{ + struct phy_package_shared *shared = phydev->shared; + struct mii_bus *bus = phydev->mdio.bus; + int addr, val; + + if (!shared || shared->base_addr + addr_offset > PHY_MAX_ADDR) + return -EIO; + + addr = shared->base_addr + addr_offset; + + phy_lock_mdio_bus(phydev); + mmd_phy_indirect(bus, addr, devad, regnum); + val = __mdiobus_read(bus, addr, MII_MMD_DATA); + phy_unlock_mdio_bus(phydev); + + return val; +} + +static inline int __phy_package_read_mmd(struct phy_device *phydev, + unsigned int addr_offset, int devad, + u32 regnum) +{ + struct phy_package_shared *shared = phydev->shared; + struct mii_bus *bus = phydev->mdio.bus; + int addr; + + if (!shared || shared->base_addr + addr_offset > PHY_MAX_ADDR) + return -EIO; + + addr = shared->base_addr + addr_offset; + mmd_phy_indirect(bus, addr, devad, regnum); + return __mdiobus_read(bus, addr, MII_MMD_DATA); +} + +static inline int phy_package_write_mmd(struct phy_device *phydev, + unsigned int addr_offset, int devad, + u32 regnum, u16 val) +{ + struct phy_package_shared *shared = phydev->shared; + struct mii_bus *bus = phydev->mdio.bus; + int addr, ret; + + if (!shared || shared->base_addr + addr_offset > PHY_MAX_ADDR) + return -EIO; + + addr = shared->base_addr + addr_offset; + + phy_lock_mdio_bus(phydev); + mmd_phy_indirect(bus, addr, devad, regnum); + ret = __mdiobus_write(bus, addr, MII_MMD_DATA, val); + phy_unlock_mdio_bus(phydev); + + return ret; +} + +static inline int __phy_package_write_mmd(struct phy_device *phydev, + unsigned int addr_offset, int devad, + u32 regnum, u16 val) +{ + struct phy_package_shared *shared = phydev->shared; + struct mii_bus *bus = phydev->mdio.bus; + int addr; + + if (!shared || shared->base_addr + addr_offset > PHY_MAX_ADDR) + return -EIO; + + addr = shared->base_addr + addr_offset; + mmd_phy_indirect(bus, addr, devad, regnum); + return __mdiobus_write(bus, addr, MII_MMD_DATA, val); +} + static inline bool __phy_package_set_once(struct phy_device *phydev, unsigned int b) {
Some PHY in PHY package may require to read/write MMD regs to correctly configure the PHY package. Add support for these additional required function in both lock and no lock variant. Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> --- include/linux/phy.h | 74 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+)