[net-next,v6,3/3] net: phy: add support for PHY package MMD read/write
Commit Message
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.
It's assumed that the entire PHY package is either C22 or C45. We use
C22 or C45 way of writing/reading to mmd regs based on the passed phydev
whether it's C22 or C45.
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
Changes v6:
- Fix copy paste error for kdoc
Changes v5:
- Improve function description
Changes v4:
- Drop function comments in header file
Changes v3:
- Move in phy-core.c from phy.h
- Base c45 from phydev
Changes v2:
- Rework to use newly introduced helper
- Add common check for regnum and devad
drivers/net/phy/phy-core.c | 144 +++++++++++++++++++++++++++++++++++++
include/linux/phy.h | 16 +++++
2 files changed, 160 insertions(+)
Comments
On Wed, Dec 13, 2023 at 11:57:30AM +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.
>
> It's assumed that the entire PHY package is either C22 or C45. We use
> C22 or C45 way of writing/reading to mmd regs based on the passed phydev
> whether it's C22 or C45.
>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
I don't recall what has been said in previous postings of this, but
introducing new helpers without an example user is normally frowned
upon. The lack of cover message for this three patch series also
doesn't help (the cover message could explain why there's no users
being proposed with this addition of helpers.)
On Wed, Dec 13, 2023 at 03:47:19PM +0000, Russell King (Oracle) wrote:
> On Wed, Dec 13, 2023 at 11:57:30AM +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.
> >
> > It's assumed that the entire PHY package is either C22 or C45. We use
> > C22 or C45 way of writing/reading to mmd regs based on the passed phydev
> > whether it's C22 or C45.
> >
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
>
> I don't recall what has been said in previous postings of this, but
> introducing new helpers without an example user is normally frowned
> upon. The lack of cover message for this three patch series also
> doesn't help (the cover message could explain why there's no users
> being proposed with this addition of helpers.)
>
These are prereq for the qca803x PHY driver and the PHY package series.
I can move this single patch in those series, but it was suggested to
move these simple change to a separate patch to lower the patch number
since they were orthogonal to the PHY package series proposal.
On Wed, Dec 13, 2023 at 04:55:08PM +0100, Christian Marangi wrote:
> On Wed, Dec 13, 2023 at 03:47:19PM +0000, Russell King (Oracle) wrote:
> > On Wed, Dec 13, 2023 at 11:57:30AM +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.
> > >
> > > It's assumed that the entire PHY package is either C22 or C45. We use
> > > C22 or C45 way of writing/reading to mmd regs based on the passed phydev
> > > whether it's C22 or C45.
> > >
> > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> >
> > I don't recall what has been said in previous postings of this, but
> > introducing new helpers without an example user is normally frowned
> > upon. The lack of cover message for this three patch series also
> > doesn't help (the cover message could explain why there's no users
> > being proposed with this addition of helpers.)
> >
>
> These are prereq for the qca803x PHY driver and the PHY package series.
>
> I can move this single patch in those series, but it was suggested to
> move these simple change to a separate patch to lower the patch number
> since they were orthogonal to the PHY package series proposal.
... so adding a cover message (your series in general seem to lack
those) would be a good idea to explain that.
On Wed, Dec 13, 2023 at 04:13:28PM +0000, Russell King (Oracle) wrote:
> On Wed, Dec 13, 2023 at 04:55:08PM +0100, Christian Marangi wrote:
> > On Wed, Dec 13, 2023 at 03:47:19PM +0000, Russell King (Oracle) wrote:
> > > On Wed, Dec 13, 2023 at 11:57:30AM +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.
> > > >
> > > > It's assumed that the entire PHY package is either C22 or C45. We use
> > > > C22 or C45 way of writing/reading to mmd regs based on the passed phydev
> > > > whether it's C22 or C45.
> > > >
> > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > >
> > > I don't recall what has been said in previous postings of this, but
> > > introducing new helpers without an example user is normally frowned
> > > upon. The lack of cover message for this three patch series also
> > > doesn't help (the cover message could explain why there's no users
> > > being proposed with this addition of helpers.)
> > >
> >
> > These are prereq for the qca803x PHY driver and the PHY package series.
> >
> > I can move this single patch in those series, but it was suggested to
> > move these simple change to a separate patch to lower the patch number
> > since they were orthogonal to the PHY package series proposal.
>
> ... so adding a cover message (your series in general seem to lack
> those) would be a good idea to explain that.
>
I tend to use cover letters only for big series, sorry for not being
very clear about this. Soo should I detach this from here or it O.K.?
On Wed, 13 Dec 2023 17:26:43 +0100 Christian Marangi wrote:
> I tend to use cover letters only for big series, sorry for not being
> very clear about this.
FWIW in netdev the rule of thumb is "if there's more than 2 patches,
there's probably extra context". Even the 2 patch exception for cover
letters is mostly for when people post code + test, as two separate
patches. Cover letter stating that you have a series for xyz
pending, which will need this, seems like a straightforward ask..
@@ -650,6 +650,150 @@ int phy_write_mmd(struct phy_device *phydev, int devad, u32 regnum, u16 val)
}
EXPORT_SYMBOL(phy_write_mmd);
+/**
+ * __phy_package_read_mmd - read MMD reg relative to PHY package base addr
+ * @phydev: The phy_device struct
+ * @addr_offset: The offset to be added to PHY package base_addr
+ * @devad: The MMD to read from
+ * @regnum: The register on the MMD to read
+ *
+ * Convenience helper for reading a register of an MMD on a given PHY
+ * using the PHY package base address. The base address is added to
+ * the addr_offset value.
+ *
+ * Same calling rules as for __phy_read();
+ *
+ * NOTE: It's assumed that the entire PHY package is either C22 or C45.
+ */
+int __phy_package_read_mmd(struct phy_device *phydev,
+ unsigned int addr_offset, int devad,
+ u32 regnum)
+{
+ struct phy_package_shared *shared = phydev->shared;
+ int addr = shared->base_addr + addr_offset;
+
+ if (addr >= PHY_MAX_ADDR)
+ return -EIO;
+
+ if (regnum > (u16)~0 || devad > 32)
+ return -EINVAL;
+
+ return mmd_phy_read(phydev->mdio.bus, addr, phydev->is_c45, devad,
+ regnum);
+}
+EXPORT_SYMBOL(__phy_package_read_mmd);
+
+/**
+ * phy_package_read_mmd - read MMD reg relative to PHY package base addr
+ * @phydev: The phy_device struct
+ * @addr_offset: The offset to be added to PHY package base_addr
+ * @devad: The MMD to read from
+ * @regnum: The register on the MMD to read
+ *
+ * Convenience helper for reading a register of an MMD on a given PHY
+ * using the PHY package base address. The base address is added to
+ * the addr_offset value.
+ *
+ * Same calling rules as for phy_read();
+ *
+ * NOTE: It's assumed that the entire PHY package is either C22 or C45.
+ */
+int phy_package_read_mmd(struct phy_device *phydev,
+ unsigned int addr_offset, int devad,
+ u32 regnum)
+{
+ struct phy_package_shared *shared = phydev->shared;
+ int addr = shared->base_addr + addr_offset;
+ int val;
+
+ if (addr >= PHY_MAX_ADDR)
+ return -EIO;
+
+ if (regnum > (u16)~0 || devad > 32)
+ return -EINVAL;
+
+ phy_lock_mdio_bus(phydev);
+ val = mmd_phy_read(phydev->mdio.bus, addr, phydev->is_c45, devad,
+ regnum);
+ phy_unlock_mdio_bus(phydev);
+
+ return val;
+}
+EXPORT_SYMBOL(phy_package_read_mmd);
+
+/**
+ * __phy_package_write_mmd - write MMD reg relative to PHY package base addr
+ * @phydev: The phy_device struct
+ * @addr_offset: The offset to be added to PHY package base_addr
+ * @devad: The MMD to write to
+ * @regnum: The register on the MMD to write
+ * @val: value to write to @regnum
+ *
+ * Convenience helper for writing a register of an MMD on a given PHY
+ * using the PHY package base address. The base address is added to
+ * the addr_offset value.
+ *
+ * Same calling rules as for __phy_write();
+ *
+ * NOTE: It's assumed that the entire PHY package is either C22 or C45.
+ */
+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;
+ int addr = shared->base_addr + addr_offset;
+
+ if (addr >= PHY_MAX_ADDR)
+ return -EIO;
+
+ if (regnum > (u16)~0 || devad > 32)
+ return -EINVAL;
+
+ return mmd_phy_write(phydev->mdio.bus, addr, phydev->is_c45, devad,
+ regnum, val);
+}
+EXPORT_SYMBOL(__phy_package_write_mmd);
+
+/**
+ * phy_package_write_mmd - write MMD reg relative to PHY package base addr
+ * @phydev: The phy_device struct
+ * @addr_offset: The offset to be added to PHY package base_addr
+ * @devad: The MMD to write to
+ * @regnum: The register on the MMD to write
+ * @val: value to write to @regnum
+ *
+ * Convenience helper for writing a register of an MMD on a given PHY
+ * using the PHY package base address. The base address is added to
+ * the addr_offset value.
+ *
+ * Same calling rules as for phy_write();
+ *
+ * NOTE: It's assumed that the entire PHY package is either C22 or C45.
+ */
+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;
+ int addr = shared->base_addr + addr_offset;
+ int ret;
+
+ if (addr >= PHY_MAX_ADDR)
+ return -EIO;
+
+ if (regnum > (u16)~0 || devad > 32)
+ return -EINVAL;
+
+ phy_lock_mdio_bus(phydev);
+ ret = mmd_phy_write(phydev->mdio.bus, addr, phydev->is_c45, devad,
+ regnum, val);
+ phy_unlock_mdio_bus(phydev);
+
+ return ret;
+}
+EXPORT_SYMBOL(phy_package_write_mmd);
+
/**
* phy_modify_changed - Function for modifying a PHY register
* @phydev: the phy_device struct
@@ -2049,6 +2049,22 @@ static inline int __phy_package_write(struct phy_device *phydev,
return __mdiobus_write(phydev->mdio.bus, addr, regnum, val);
}
+int __phy_package_read_mmd(struct phy_device *phydev,
+ unsigned int addr_offset, int devad,
+ u32 regnum);
+
+int phy_package_read_mmd(struct phy_device *phydev,
+ unsigned int addr_offset, int devad,
+ u32 regnum);
+
+int __phy_package_write_mmd(struct phy_device *phydev,
+ unsigned int addr_offset, int devad,
+ u32 regnum, u16 val);
+
+int phy_package_write_mmd(struct phy_device *phydev,
+ unsigned int addr_offset, int devad,
+ u32 regnum, u16 val);
+
static inline bool __phy_package_set_once(struct phy_device *phydev,
unsigned int b)
{