[RFC net-next] net: phy: introduce phy_reg_field interface

[RFC net-next] net: phy: introduce phy_reg_field interface

[Radu Pirea (OSS)]

Some PHYs can be heavily modified between revisions, and the addresses of
the registers are changed and the register fields are moved from one
register to another.

To integrate more PHYs in the same driver with the same register fields,
but these register fields were located in different registers at
different offsets, I introduced the phy_reg_fied structure.

phy_reg_fied structure abstracts the register fields differences.

Signed-off-by: Radu Pirea (OSS) <radu-nicolae.pirea@oss.nxp.com>
---
 drivers/net/phy/phy-core.c | 77 ++++++++++++++++++++++++++++++++++++++
 include/linux/phy.h        | 73 ++++++++++++++++++++++++++++++++++++
 2 files changed, 150 insertions(+)

diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index a64186dc53f8..e9c16e78dc72 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -593,6 +593,45 @@ int phy_read_mmd(struct phy_device *phydev, int devad, u32 regnum)
 }
 EXPORT_SYMBOL(phy_read_mmd);
 
+/**
+ * phy_read_reg_field - Convenience function for reading a register field
+ * on a given PHY.
+ * @phydev: the phy_device struct
+ * @reg_field: the phy_reg_field structure to be read
+ *
+ * Return: the reg field value on success, -errno on failure
+ */
+int phy_read_reg_field(struct phy_device *phydev,
+		       const struct phy_reg_field *reg_field)
+{
+	u16 mask;
+	int ret;
+
+	if (reg_field->size == 0) {
+		phydev_warn(phydev, "Trying to read a reg field of size 0.");
+		return -EINVAL;
+	}
+
+	phy_lock_mdio_bus(phydev);
+	if (reg_field->mmd)
+		ret = __phy_read_mmd(phydev, reg_field->devad,
+				     reg_field->reg);
+	else
+		ret = __phy_read(phydev, reg_field->reg);
+	phy_unlock_mdio_bus(phydev);
+
+	if (ret < 0)
+		return ret;
+
+	mask = reg_field->size == 1 ? BIT(reg_field->offset) :
+		GENMASK(reg_field->offset + reg_field->size - 1, reg_field->offset);
+	ret &= mask;
+	ret >>= reg_field->offset;
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(phy_read_reg_field);
+
 /**
  * __phy_write_mmd - Convenience function for writing a register
  * on an MMD on a given PHY.
@@ -652,6 +691,44 @@ int phy_write_mmd(struct phy_device *phydev, int devad, u32 regnum, u16 val)
 }
 EXPORT_SYMBOL(phy_write_mmd);
 
+/**
+ * phy_write_reg_field - Convenience function for writing a register field
+ * on a given PHY.
+ * @phydev: the phy_device struct
+ * @reg_field: the phy_reg_field structure to be written
+ * @val: value to write to @reg_field
+ *
+ * Return: 0 on success, -errno on failure.
+ */
+int phy_write_reg_field(struct phy_device *phydev,
+			const struct phy_reg_field *reg_field, u16 val)
+{
+	u16 mask;
+	u16 set;
+	int ret;
+
+	if (reg_field->size == 0) {
+		phydev_warn(phydev, "Trying to write a reg field of size 0.");
+		return -EINVAL;
+	}
+
+	mask = reg_field->size == 1 ? BIT(reg_field->offset) :
+		GENMASK(reg_field->offset + reg_field->size - 1, reg_field->offset);
+	set = val << reg_field->offset;
+
+	phy_lock_mdio_bus(phydev);
+	if (reg_field->mmd)
+		ret = __phy_modify_mmd_changed(phydev, reg_field->devad,
+					       reg_field->reg, mask, set);
+	else
+		ret = __phy_modify_changed(phydev, reg_field->reg,
+					   mask, set);
+	phy_unlock_mdio_bus(phydev);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(phy_write_reg_field);
+
 /**
  * phy_modify_changed - Function for modifying a PHY register
  * @phydev: the phy_device struct
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 2f83cfc206e5..f8bf90895340 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1113,6 +1113,39 @@ void phy_resolve_aneg_pause(struct phy_device *phydev);
 void phy_resolve_aneg_linkmode(struct phy_device *phydev);
 void phy_check_downshift(struct phy_device *phydev);
 
+/**
+ * struct phy_reg_field - The generic register field structure for a PHY
+ * @reg: register address
+ * @devad: MMD where the register is present
+ * @offset: offset of the field inside the register
+ * @size: size of the register filed
+ * @mmd: if true, then the register field will be read using phy_read_mmd
+ */
+struct phy_reg_field {
+	u16 reg;
+	u8 devad;
+	u8 offset;
+	u8 size;
+	bool mmd;
+};
+
+#define PHY_REG_FIELD_INIT(_reg, _offset, _size) \
+	((struct phy_reg_field) {		 \
+		.reg = _reg,			 \
+		.offset = _offset,		 \
+		.size = _size,			 \
+		.mmd = false			 \
+	})
+
+#define PHY_REG_FIELD_MMD_INIT(_reg, _mmd, _offset, _size) \
+	((struct phy_reg_field) {			   \
+		.reg = _reg,				   \
+		.devad =  _mmd,				   \
+		.offset = _offset,			   \
+		.size = _size,				   \
+		.mmd = true				   \
+	})
+
 /**
  * phy_read - Convenience function for reading a given PHY register
  * @phydev: the phy_device struct
@@ -1205,6 +1238,13 @@ static inline int __phy_modify_changed(struct phy_device *phydev, u32 regnum,
  */
 int phy_read_mmd(struct phy_device *phydev, int devad, u32 regnum);
 
+/*
+ * phy_read_reg_field - Convenience function for reading a register field on a
+ * given PHY.
+ */
+int phy_read_reg_field(struct phy_device *phydev,
+		       const struct phy_reg_field *reg_field);
+
 /**
  * phy_read_mmd_poll_timeout - Periodically poll a PHY register until a
  *                             condition is met or a timeout occurs
@@ -1248,6 +1288,39 @@ int __phy_read_mmd(struct phy_device *phydev, int devad, u32 regnum);
  */
 int phy_write_mmd(struct phy_device *phydev, int devad, u32 regnum, u16 val);
 
+/*
+ * phy_write_reg_field - Convenience function for writing a register field on a
+ * given PHY.
+ */
+int phy_write_reg_field(struct phy_device *phydev,
+			const struct phy_reg_field *reg_field, u16 val);
+
+/*
+ * phy_set_reg_field - Convenience function for setting a register field
+ * of one bit on a given PHY.
+ */
+static inline int phy_set_reg_field(struct phy_device *phydev,
+				    const struct phy_reg_field *reg_field)
+{
+	if (reg_field->size != 1)
+		return -EINVAL;
+
+	return phy_write_reg_field(phydev, reg_field, 1);
+}
+
+/*
+ * phy_clear_reg_field - Convenience function for clearing a register field
+ * of one bit on a given PHY.
+ */
+static inline int phy_clear_reg_field(struct phy_device *phydev,
+				      const struct phy_reg_field *reg_field)
+{
+	if (reg_field->size != 1)
+		return -EINVAL;
+
+	return phy_write_reg_field(phydev, reg_field, 0);
+}
+
 /*
  * __phy_write_mmd - Convenience function for writing a register
  * on an MMD on a given PHY.
-- 
2.34.1

Re: [RFC net-next] net: phy: introduce phy_reg_field interface

[Russell King (Oracle)]

On Fri, Mar 31, 2023 at 03:32:59PM +0300, Radu Pirea (OSS) wrote:
> Some PHYs can be heavily modified between revisions, and the addresses of
> the registers are changed and the register fields are moved from one
> register to another.
> 
> To integrate more PHYs in the same driver with the same register fields,
> but these register fields were located in different registers at
> different offsets, I introduced the phy_reg_fied structure.
> 
> phy_reg_fied structure abstracts the register fields differences.

Oh no, not more perliferation of different accessors...

> +int phy_read_reg_field(struct phy_device *phydev,
> +		       const struct phy_reg_field *reg_field)
> +{
> +	u16 mask;
> +	int ret;
> +
> +	if (reg_field->size == 0) {
> +		phydev_warn(phydev, "Trying to read a reg field of size 0.");
> +		return -EINVAL;
> +	}
> +
> +	phy_lock_mdio_bus(phydev);
> +	if (reg_field->mmd)
> +		ret = __phy_read_mmd(phydev, reg_field->devad,
> +				     reg_field->reg);
> +	else
> +		ret = __phy_read(phydev, reg_field->reg);
> +	phy_unlock_mdio_bus(phydev);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	mask = reg_field->size == 1 ? BIT(reg_field->offset) :
> +		GENMASK(reg_field->offset + reg_field->size - 1, reg_field->offset);
> +	ret &= mask;
> +	ret >>= reg_field->offset;
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(phy_read_reg_field);

I guess next we'll eventually see that we need __phy_read_reg_field
which doesn't take the lock, so that several accesses can be done
together. E.g. to access some form of paging mechanism.

> +/**
> + * phy_write_reg_field - Convenience function for writing a register field
> + * on a given PHY.
> + * @phydev: the phy_device struct
> + * @reg_field: the phy_reg_field structure to be written
> + * @val: value to write to @reg_field
> + *
> + * Return: 0 on success, -errno on failure.
> + */
> +int phy_write_reg_field(struct phy_device *phydev,
> +			const struct phy_reg_field *reg_field, u16 val)
> +{
> +	u16 mask;
> +	u16 set;
> +	int ret;
> +
> +	if (reg_field->size == 0) {
> +		phydev_warn(phydev, "Trying to write a reg field of size 0.");
> +		return -EINVAL;
> +	}
> +
> +	mask = reg_field->size == 1 ? BIT(reg_field->offset) :
> +		GENMASK(reg_field->offset + reg_field->size - 1, reg_field->offset);
> +	set = val << reg_field->offset;
> +
> +	phy_lock_mdio_bus(phydev);
> +	if (reg_field->mmd)
> +		ret = __phy_modify_mmd_changed(phydev, reg_field->devad,
> +					       reg_field->reg, mask, set);
> +	else
> +		ret = __phy_modify_changed(phydev, reg_field->reg,
> +					   mask, set);
> +	phy_unlock_mdio_bus(phydev);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(phy_write_reg_field);

More or less the same for this too.

In order to properly review this, we need the patch which has the use
case for these new accessors.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [RFC net-next] net: phy: introduce phy_reg_field interface

[Florian Fainelli]

On 3/31/2023 5:32 AM, Radu Pirea (OSS) wrote:
> Some PHYs can be heavily modified between revisions, and the addresses of
> the registers are changed and the register fields are moved from one
> register to another.
> 
> To integrate more PHYs in the same driver with the same register fields,
> but these register fields were located in different registers at
> different offsets, I introduced the phy_reg_fied structure.
> 
> phy_reg_fied structure abstracts the register fields differences.
> 
> Signed-off-by: Radu Pirea (OSS) <radu-nicolae.pirea@oss.nxp.com>

You know how it goes: a framework without its user will not be accepted 
unless an user of that framework also shows up. Can you post both?
-- 
Florian

Re: [RFC net-next] net: phy: introduce phy_reg_field interface

[Andrew Lunn]

On Fri, Mar 31, 2023 at 03:32:59PM +0300, Radu Pirea (OSS) wrote:
> Some PHYs can be heavily modified between revisions, and the addresses of
> the registers are changed and the register fields are moved from one
> register to another.
> 
> To integrate more PHYs in the same driver with the same register fields,
> but these register fields were located in different registers at
> different offsets, I introduced the phy_reg_fied structure.
> 
> phy_reg_fied structure abstracts the register fields differences.

Hi Radu

You should always include a user of a new API. It makes it easier to
understand and review if you see both sides of an API.

Please turn this into a patchset, and make use of this new functions
in a driver.

	Andrew

Re: [RFC net-next] net: phy: introduce phy_reg_field interface

[Andrew Lunn]

On Fri, Mar 31, 2023 at 03:32:59PM +0300, Radu Pirea (OSS) wrote:
> Some PHYs can be heavily modified between revisions, and the addresses of
> the registers are changed and the register fields are moved from one
> register to another.
> 
> To integrate more PHYs in the same driver with the same register fields,
> but these register fields were located in different registers at
> different offsets, I introduced the phy_reg_fied structure.

Maybe you are solving the wrong problem. Maybe you should be telling
the hardware/firmware engineers not to do this!

How many drivers can actually use this?	I don't	really want to
encourage vendors to make such a mess of their hardware, so i'm
wondering if this should be hidden away in the driver, if there	is
only one driver which needs it.	If there are multiple drivers which
can use this, please do	modify at least	one other driver to use	it,
hence showing it is generic.

> +int phy_read_reg_field(struct phy_device *phydev,
> +		       const struct phy_reg_field *reg_field)
> +{
> +	u16 mask;
> +	int ret;
> +
> +	if (reg_field->size == 0) {
> +		phydev_warn(phydev, "Trying to read a reg field of size 0.");
> +		return -EINVAL;
> +	}
> +
> +	phy_lock_mdio_bus(phydev);
> +	if (reg_field->mmd)
> +		ret = __phy_read_mmd(phydev, reg_field->devad,
> +				     reg_field->reg);
> +	else
> +		ret = __phy_read(phydev, reg_field->reg);
> +	phy_unlock_mdio_bus(phydev);
> +

Could you please explain the locking. It appears you are trying to
protect reg_field->mmd? Does that really change? Especially since you
have _const_ struct phy_reg_field *

      Andrew

Re: [RFC net-next] net: phy: introduce phy_reg_field interface

[Radu Nicolae Pirea (OSS)]

On Fri, 2023-03-31 at 15:07 +0200, Andrew Lunn wrote:
> On Fri, Mar 31, 2023 at 03:32:59PM +0300, Radu Pirea (OSS) wrote:
> > Some PHYs can be heavily modified between revisions, and the
> > addresses of
> > the registers are changed and the register fields are moved from
> > one
> > register to another.
> > 
> > To integrate more PHYs in the same driver with the same register
> > fields,
> > but these register fields were located in different registers at
> > different offsets, I introduced the phy_reg_fied structure.
> 
> Maybe you are solving the wrong problem. Maybe you should be telling
> the hardware/firmware engineers not to do this!
I agree with this. I am trying to solve the wrong problem.

> 
> How many drivers can actually use this? I don't really want to
> encourage vendors to make such a mess of their hardware, so i'm
> wondering if this should be hidden away in the driver, if there is
> only one driver which needs it. If there are multiple drivers which
> can use this, please do modify at least one other driver to use it,
> hence showing it is generic.
The nxp-c45-tja11xx driver will be the user of this kind of abstraction
layer. I was looking to get a quick review on this, before sending it
integrated into a patch series.

> 
> > +int phy_read_reg_field(struct phy_device *phydev,
> > +                      const struct phy_reg_field *reg_field)
> > +{
> > +       u16 mask;
> > +       int ret;
> > +
> > +       if (reg_field->size == 0) {
> > +               phydev_warn(phydev, "Trying to read a reg field of
> > size 0.");
> > +               return -EINVAL;
> > +       }
> > +
> > +       phy_lock_mdio_bus(phydev);
> > +       if (reg_field->mmd)
> > +               ret = __phy_read_mmd(phydev, reg_field->devad,
> > +                                    reg_field->reg);
> > +       else
> > +               ret = __phy_read(phydev, reg_field->reg);
> > +       phy_unlock_mdio_bus(phydev);
> > +
> 
> Could you please explain the locking. It appears you are trying to
> protect reg_field->mmd? Does that really change? Especially since you
> have _const_ struct phy_reg_field *
I am trying to protect the __phy_read_mmd and __phy_read calls, not the
reg_field->mmd.

Radu P.
> 
>       Andrew

Re: [RFC net-next] net: phy: introduce phy_reg_field interface

[Andrew Lunn]

> I am trying to protect the __phy_read_mmd and __phy_read calls, not the
> reg_field->mmd.

You are?

Then why not use phy_read_mmd() and phy_read()?

    Andrew