[RFC PATCH 0/2] Clause-22/Clause-45 MDIO regmap support

[RFC PATCH 0/2] Clause-22/Clause-45 MDIO regmap support

[Sander Vanheule]

The initial MDIO regmap implementation only supported (or claimed to
only support) clause-22 register access, with 5 bit register addresses.
However, this was not enforced sufficiently, and regnum values were
passed verbatim to the mdio bus subsystem.

These patches aim to enforce the register address width, and also add
clause-45 support for extended address spaces. A clause-45 address is
defined here as the composite of the device type (see MDIO_MMD_* in
include/uapi/linux/mdio.h) and the register number, for a total width of
21 bit.

I have zero experience with clause-45 devices, and no such devices
available for testing. As such, clause-45 code in the second patch is
entirely untested, although it isn't very complex.

Although these patches should eventually make it into regmap-mdio, I
would like to resolve some questions first.

1. I've opted to just ignore any bits that lie beyond the allowed address
   width. Would it be cleaner to raise an error instead?

2. Packing of the clause-45 register addresses (16 bit) and adressed device
   type (5 bit) is the same as in the mdio subsystem, resulting in a 21 bit
   address. Is this an appropriate way to pack this information into one
   address for the regmap interface?

The reasoning behind (1) is to allow the regmap user to use extra bits
as a way to virtually extend the address space. Note that this actually
results in aliasing. This can be useful if the data read from to a
register doesn't have the same meaning as the data written to it
(e.g. GPIO pin input and output data). An alternative solution to this
would be some concept of "aliased registers" in regmap -- like volatile or
precious registers.

Sander Vanheule (2):
  regmap: mdio: Clean up invalid clause-22 addresses
  regmap: mdio: Add clause-45 support

 drivers/base/regmap/regmap-mdio.c | 73 +++++++++++++++++++++++++------
 1 file changed, 59 insertions(+), 14 deletions(-)

-- 
2.31.1

[RFC PATCH 1/2] regmap: mdio: Clean up invalid clause-22 addresses

[Sander Vanheule]

Currently a regmap configuration for regmap-mdio must have a register
address width of 5 bits (cf. clause-22 register access). This is not
enforced on the provided register addresses, which would enable
clause-45 MDIO bus access, if the right bit packing is used.

Prevent clause-45 access, and other invalid addresses, by masking the
provided register address.

Signed-off-by: Sander Vanheule <sander@svanheule.net>
---
 drivers/base/regmap/regmap-mdio.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/base/regmap/regmap-mdio.c b/drivers/base/regmap/regmap-mdio.c
index 5ec208279913..aee34bf2400e 100644
--- a/drivers/base/regmap/regmap-mdio.c
+++ b/drivers/base/regmap/regmap-mdio.c
@@ -5,16 +5,19 @@
 #include <linux/module.h>
 #include <linux/regmap.h>
 
+#define REGVAL_MASK		GENMASK(15, 0)
+#define REGNUM_C22_MASK		GENMASK(4, 0)
+
 static int regmap_mdio_read(void *context, unsigned int reg, unsigned int *val)
 {
 	struct mdio_device *mdio_dev = context;
 	int ret;
 
-	ret = mdiobus_read(mdio_dev->bus, mdio_dev->addr, reg);
+	ret = mdiobus_read(mdio_dev->bus, mdio_dev->addr, reg & REGNUM_C22_MASK);
 	if (ret < 0)
 		return ret;
 
-	*val = ret & 0xffff;
+	*val = ret & REGVAL_MASK;
 	return 0;
 }
 
@@ -22,7 +25,7 @@ static int regmap_mdio_write(void *context, unsigned int reg, unsigned int val)
 {
 	struct mdio_device *mdio_dev = context;
 
-	return mdiobus_write(mdio_dev->bus, mdio_dev->addr, reg, val);
+	return mdiobus_write(mdio_dev->bus, mdio_dev->addr, reg & REGNUM_C22_MASK, val);
 }
 
 static const struct regmap_bus regmap_mdio_bus = {
-- 
2.31.1

[RFC PATCH 2/2] regmap: mdio: Add clause-45 support

[Sander Vanheule]

Modern ethernet phys support the so-called clause-45 register access
mode, which allows for register address widths of 16 bit.

Also allow for 16-bit register address widths, and return a regmap for
clause-45 access in that case.

Signed-off-by: Sander Vanheule <sander@svanheule.net>
---
 drivers/base/regmap/regmap-mdio.c | 70 ++++++++++++++++++++++++-------
 1 file changed, 56 insertions(+), 14 deletions(-)

diff --git a/drivers/base/regmap/regmap-mdio.c b/drivers/base/regmap/regmap-mdio.c
index aee34bf2400e..cfb23afb19eb 100644
--- a/drivers/base/regmap/regmap-mdio.c
+++ b/drivers/base/regmap/regmap-mdio.c
@@ -7,13 +7,14 @@
 
 #define REGVAL_MASK		GENMASK(15, 0)
 #define REGNUM_C22_MASK		GENMASK(4, 0)
+/* Clause-45 mask includes the device type (5 bit) and actual register number (16 bit) */
+#define REGNUM_C45_MASK		GENMASK(20, 0)
 
-static int regmap_mdio_read(void *context, unsigned int reg, unsigned int *val)
+static int regmap_mdio_read(struct mdio_device *mdio_dev, u32 reg, unsigned int *val)
 {
-	struct mdio_device *mdio_dev = context;
 	int ret;
 
-	ret = mdiobus_read(mdio_dev->bus, mdio_dev->addr, reg & REGNUM_C22_MASK);
+	ret = mdiobus_read(mdio_dev->bus, mdio_dev->addr, reg);
 	if (ret < 0)
 		return ret;
 
@@ -21,27 +22,63 @@ static int regmap_mdio_read(void *context, unsigned int reg, unsigned int *val)
 	return 0;
 }
 
-static int regmap_mdio_write(void *context, unsigned int reg, unsigned int val)
+static int regmap_mdio_write(struct mdio_device *mdio_dev, u32 reg, unsigned int val)
+{
+	return mdiobus_write(mdio_dev->bus, mdio_dev->addr, reg, val);
+}
+
+static int regmap_mdio_c22_read(void *context, unsigned int reg, unsigned int *val)
+{
+	struct mdio_device *mdio_dev = context;
+
+	return regmap_mdio_read(mdio_dev, reg & REGNUM_C22_MASK, val);
+}
+
+static int regmap_mdio_c22_write(void *context, unsigned int reg, unsigned int val)
 {
 	struct mdio_device *mdio_dev = context;
 
-	return mdiobus_write(mdio_dev->bus, mdio_dev->addr, reg & REGNUM_C22_MASK, val);
+	return regmap_mdio_write(mdio_dev, reg & REGNUM_C22_MASK, val);
 }
 
-static const struct regmap_bus regmap_mdio_bus = {
-	.reg_write = regmap_mdio_write,
-	.reg_read = regmap_mdio_read,
+static const struct regmap_bus regmap_mdio_c22_bus = {
+	.reg_write = regmap_mdio_c22_write,
+	.reg_read = regmap_mdio_c22_read,
+};
+
+static int regmap_mdio_c45_read(void *context, unsigned int reg, unsigned int *val)
+{
+	struct mdio_device *mdio_dev = context;
+
+	return regmap_mdio_read(mdio_dev, MII_ADDR_C45 | (reg & REGNUM_C45_MASK), val);
+}
+
+static int regmap_mdio_c45_write(void *context, unsigned int reg, unsigned int val)
+{
+	struct mdio_device *mdio_dev = context;
+
+	return regmap_mdio_write(mdio_dev, MII_ADDR_C45 | (reg & REGNUM_C45_MASK), val);
+}
+
+static const struct regmap_bus regmap_mdio_c45_bus = {
+	.reg_write = regmap_mdio_c45_write,
+	.reg_read = regmap_mdio_c45_read,
 };
 
 struct regmap *__regmap_init_mdio(struct mdio_device *mdio_dev,
 	const struct regmap_config *config, struct lock_class_key *lock_key,
 	const char *lock_name)
 {
-	if (config->reg_bits != 5 || config->val_bits != 16)
+	struct regmap_bus *bus;
+
+	if (config->reg_bits == 5 && config->val_bits == 16)
+		bus = &regmap_mdio_c22_bus;
+	else if (config->reg_bits == 21 && config->val_bits == 16)
+		bus = &regmap_mdio_c45_bus;
+	else
 		return ERR_PTR(-EOPNOTSUPP);
 
-	return __regmap_init(&mdio_dev->dev, &regmap_mdio_bus, mdio_dev, config,
-		lock_key, lock_name);
+	return __regmap_init(&mdio_dev->dev, bus, mdio_dev, config, lock_key, lock_name);
 }
 EXPORT_SYMBOL_GPL(__regmap_init_mdio);
 
@@ -49,11 +86,16 @@ struct regmap *__devm_regmap_init_mdio(struct mdio_device *mdio_dev,
 	const struct regmap_config *config, struct lock_class_key *lock_key,
 	const char *lock_name)
 {
-	if (config->reg_bits != 5 || config->val_bits != 16)
+	const struct regmap_bus *bus;
+
+	if (config->reg_bits == 5 && config->val_bits == 16)
+		bus = &regmap_mdio_c22_bus;
+	else if (config->reg_bits == 21 && config->val_bits == 16)
+		bus = &regmap_mdio_c45_bus;
+	else
 		return ERR_PTR(-EOPNOTSUPP);
 
-	return __devm_regmap_init(&mdio_dev->dev, &regmap_mdio_bus, mdio_dev,
-		config, lock_key, lock_name);
+	return __devm_regmap_init(&mdio_dev->dev, bus, mdio_dev, config, lock_key, lock_name);
 }
 EXPORT_SYMBOL_GPL(__devm_regmap_init_mdio);
 
-- 
2.31.1

Re: [RFC PATCH 0/2] Clause-22/Clause-45 MDIO regmap support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1473 bytes --]

On Thu, Jun 03, 2021 at 08:25:08PM +0200, Sander Vanheule wrote:

> 1. I've opted to just ignore any bits that lie beyond the allowed address
>    width. Would it be cleaner to raise an error instead?

It doesn't *really* matter, enforcement is probably a bit better as it
might catch bugs.

> 2. Packing of the clause-45 register addresses (16 bit) and adressed device
>    type (5 bit) is the same as in the mdio subsystem, resulting in a 21 bit
>    address. Is this an appropriate way to pack this information into one
>    address for the regmap interface?

Either that or pass the type in when instantiating the regmap (it sounds
like it should be the same for all registers on the device?).

> The reasoning behind (1) is to allow the regmap user to use extra bits
> as a way to virtually extend the address space. Note that this actually
> results in aliasing. This can be useful if the data read from to a
> register doesn't have the same meaning as the data written to it
> (e.g. GPIO pin input and output data). An alternative solution to this
> would be some concept of "aliased registers" in regmap -- like volatile or
> precious registers.

I think these registers are in practice going to either need to be
volatile (how most of them work at the minute) or otherwise handled in
regmap (eg, the page support we've got).  Having two different names for
the same register feels like it's asking for bugs if any of the higher
level functions of regmap get used.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 0/2] Clause-22/Clause-45 MDIO regmap support

[Sander Vanheule]

Hi Mark,

On Fri, 2021-06-04 at 18:25 +0100, Mark Brown wrote:
> On Thu, Jun 03, 2021 at 08:25:08PM +0200, Sander Vanheule wrote:
> 
> > 1. I've opted to just ignore any bits that lie beyond the allowed address
> >    width. Would it be cleaner to raise an error instead?
> 
> It doesn't *really* matter, enforcement is probably a bit better as it
> might catch bugs.

Agreed.

> > 2. Packing of the clause-45 register addresses (16 bit) and adressed device
> >    type (5 bit) is the same as in the mdio subsystem, resulting in a 21 bit
> >    address. Is this an appropriate way to pack this information into one
> >    address for the regmap interface?
> 
> Either that or pass the type in when instantiating the regmap (it sounds
> like it should be the same for all registers on the device?).

I think the 'device type' field should be viewed more as a paging index. A phy
will have PMA/PMD ("generic phy") features, but will likely also have status and
settings in the AN (auto-negotiation) page. I'm sure Andrew knows a lot more
about this than I do.

> 
> > The reasoning behind (1) is to allow the regmap user to use extra bits
> > as a way to virtually extend the address space. Note that this actually
> > results in aliasing. This can be useful if the data read from to a
> > register doesn't have the same meaning as the data written to it
> > (e.g. GPIO pin input and output data). An alternative solution to this
> > would be some concept of "aliased registers" in regmap -- like volatile or
> > precious registers.
> 
> I think these registers are in practice going to either need to be
> volatile (how most of them work at the minute) or otherwise handled in
> regmap (eg, the page support we've got).  Having two different names for
> the same register feels like it's asking for bugs if any of the higher
> level functions of regmap get used.

This is actually an issue with a GPIO chip that I'm trying to implement [1]. To
set an output, data is written to the register. To get an input value, data is
read from the register. Since a register contains data for 16 GPIO lines, a
regular read-modify-write could erroneously overwrite output values. A pin
outside of the RMW mask could've changed to an input, and may now be reading a
different value. The issue I was running into, had to do with a RMW not being
written because the pin value apparently hadn't changed.

To work around the issue, I created a "virtual page" by adding an extra bit [2].
On reads and writes, they are aliased to the same actual register. However, by
having two different addresses, one can be marked as "volatile and read-only",
while the other is "non-volatile and write-only". The latter allows for caching,
ensuring that a RMW will use the (correct) cached value to calculate the updated
register value.

I didn't use the existing paging mechanism for this, since (I think) then I
would need to specify a register that contains the page index. But as I don't
have an actual page register, I would have to specify another existing register
with an empty mask. This could lead to useless bus activity if I accidentally
chose a volatile register.

[1] https://lore.kernel.org/lkml/84352c93f27d7c8b7afea54f3932020e9cd97d02.camel@svanheule.net/
[2] https://lore.kernel.org/lkml/56fb027587fa067a249237ecaf40828cd508cdcc.1622713678.git.sander@svanheule.net/


Best,
Sander

Re: [RFC PATCH 0/2] Clause-22/Clause-45 MDIO regmap support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1808 bytes --]

On Fri, Jun 04, 2021 at 08:16:53PM +0200, Sander Vanheule wrote:
> On Fri, 2021-06-04 at 18:25 +0100, Mark Brown wrote:

> > I think these registers are in practice going to either need to be
> > volatile (how most of them work at the minute) or otherwise handled in
> > regmap (eg, the page support we've got).  Having two different names for
> > the same register feels like it's asking for bugs if any of the higher
> > level functions of regmap get used.

> This is actually an issue with a GPIO chip that I'm trying to implement [1]. To
> set an output, data is written to the register. To get an input value, data is
> read from the register. Since a register contains data for 16 GPIO lines, a
> regular read-modify-write could erroneously overwrite output values. A pin
> outside of the RMW mask could've changed to an input, and may now be reading a
> different value. The issue I was running into, had to do with a RMW not being
> written because the pin value apparently hadn't changed.

If the hardware isn't able to read back the status of the pins in output
mode (even if it's always reading back from the input circuit where is
it getting other inputs from?) you're probably better off with just
having an open coded cache separately than trying to make up fake
registers that rely on current implementation details to work.

> I didn't use the existing paging mechanism for this, since (I think) then I
> would need to specify a register that contains the page index. But as I don't
> have an actual page register, I would have to specify another existing register
> with an empty mask. This could lead to useless bus activity if I accidentally
> chose a volatile register.

This is clearly not paging, it would be totally inappropraite to use
paging for this.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 0/2] Clause-22/Clause-45 MDIO regmap support

[Sander Vanheule]

Hi Mark,

On Mon, 2021-06-07 at 12:54 +0100, Mark Brown wrote:
> On Fri, Jun 04, 2021 at 08:16:53PM +0200, Sander Vanheule wrote:
> > On Fri, 2021-06-04 at 18:25 +0100, Mark Brown wrote:
> 
> > > I think these registers are in practice going to either need to be
> > > volatile (how most of them work at the minute) or otherwise handled in
> > > regmap (eg, the page support we've got).  Having two different names for
> > > the same register feels like it's asking for bugs if any of the higher
> > > level functions of regmap get used.
> 
> > This is actually an issue with a GPIO chip that I'm trying to implement [1].
> > To
> > set an output, data is written to the register. To get an input value, data
> > is
> > read from the register. Since a register contains data for 16 GPIO lines, a
> > regular read-modify-write could erroneously overwrite output values. A pin
> > outside of the RMW mask could've changed to an input, and may now be reading
> > a
> > different value. The issue I was running into, had to do with a RMW not
> > being
> > written because the pin value apparently hadn't changed.
> 
> If the hardware isn't able to read back the status of the pins in output
> mode (even if it's always reading back from the input circuit where is
> it getting other inputs from?) you're probably better off with just
> having an open coded cache separately than trying to make up fake
> registers that rely on current implementation details to work.
> 
> > I didn't use the existing paging mechanism for this, since (I think) then I
> > would need to specify a register that contains the page index. But as I
> > don't
> > have an actual page register, I would have to specify another existing
> > register
> > with an empty mask. This could lead to useless bus activity if I
> > accidentally
> > chose a volatile register.
> 
> This is clearly not paging, it would be totally inappropraite to use
> paging for this.

Thank you for the input. I'll take this to the RTL8231 thread, to see what I can
come up with as a cleaner solution, without abusing the regmap interface.


Best regards,
Sander

Re: [RFC PATCH 0/2] Clause-22/Clause-45 MDIO regmap support

[Andy Shevchenko]

On Mon, Jun 7, 2021 at 2:55 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Jun 04, 2021 at 08:16:53PM +0200, Sander Vanheule wrote:
> > On Fri, 2021-06-04 at 18:25 +0100, Mark Brown wrote:
>
> > > I think these registers are in practice going to either need to be
> > > volatile (how most of them work at the minute) or otherwise handled in
> > > regmap (eg, the page support we've got).  Having two different names for
> > > the same register feels like it's asking for bugs if any of the higher
> > > level functions of regmap get used.
>
> > This is actually an issue with a GPIO chip that I'm trying to implement [1]. To
> > set an output, data is written to the register. To get an input value, data is
> > read from the register. Since a register contains data for 16 GPIO lines, a
> > regular read-modify-write could erroneously overwrite output values. A pin
> > outside of the RMW mask could've changed to an input, and may now be reading a
> > different value. The issue I was running into, had to do with a RMW not being
> > written because the pin value apparently hadn't changed.
>
> If the hardware isn't able to read back the status of the pins in output
> mode (even if it's always reading back from the input circuit where is
> it getting other inputs from?) you're probably better off with just
> having an open coded cache separately than trying to make up fake
> registers that rely on current implementation details to work.

Isn't it a disadvantage of regmap APIs? The hardware that uses the
same offset for R and W with different semantics is quite normal. I
think it is a good exercise to implement regmap-8250 as an example of
how to deal with such hardware.


-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 0/2] Clause-22/Clause-45 MDIO regmap support

[Andrew Lunn]

On Fri, Jun 04, 2021 at 06:25:15PM +0100, Mark Brown wrote:
> On Thu, Jun 03, 2021 at 08:25:08PM +0200, Sander Vanheule wrote:
> 
> > 1. I've opted to just ignore any bits that lie beyond the allowed address
> >    width. Would it be cleaner to raise an error instead?
> 
> It doesn't *really* matter, enforcement is probably a bit better as it
> might catch bugs.

I would agree with that. The mostly likely problem is that somebody
misses the difference between C22 and C45 and instantiates the wrong
sort of regmap. You can quickly detect a C22 regmap being used for C45
due to the range difference.

> > 2. Packing of the clause-45 register addresses (16 bit) and adressed device
> >    type (5 bit) is the same as in the mdio subsystem, resulting in a 21 bit
> >    address. Is this an appropriate way to pack this information into one
> >    address for the regmap interface?
> 
> Either that or pass the type in when instantiating the regmap (it sounds
> like it should be the same for all registers on the device?).

A device can implement both C22 and C45. There is also a standardized
way to perform C45 access over C22, using two registers in C22 space.
But this assumes the device is an Ethernet PHY, or is at least making
use of the Ethernet PHY way of doing this tunnelling. But i doubt any
Ethernet PHY driver will ever use regmap.

Where i see regmap-mdio being used it for non-Ethernet PHY
devices. That mostly means Ethernet Switches and oddball devices like
this like LED controller. Broadcom also have a generic PHY driver
talking over MDIO to hardware.

     Andrew

Re: [RFC PATCH 0/2] Clause-22/Clause-45 MDIO regmap support

[Andrew Lunn]

> I have zero experience with clause-45 devices, and no such devices
> available for testing. As such, clause-45 code in the second patch is
> entirely untested, although it isn't very complex.

Normal kernel policy is not to merge something if it has no users. As
i said in another reply, the only users of regmap-mdio are going to be
Ethernet switch and funky mdio devices. So far, i've not seen a C45
Ethernet switch, nor a funky c45 mdio device. So i suggest we leave it
on the mainline list until some hardware which could use it comes
along.

	Andrew

[PATCH] regmap: mdio: Reject invalid clause-22 addresses

[Sander Vanheule]

Currently a regmap configuration for regmap-mdio must have a register
address width of 5 bits (cf. clause-22 register access). This is not
enforced on the provided register addresses, which would enable
clause-45 MDIO bus access, if the right bit packing is used.

Prevent clause-45 access, and other invalid addresses, by verifying the
provided register address.

Cc: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Sander Vanheule <sander@svanheule.net>
---
 drivers/base/regmap/regmap-mdio.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/base/regmap/regmap-mdio.c b/drivers/base/regmap/regmap-mdio.c
index 5ec208279913..3b842cf1eef9 100644
--- a/drivers/base/regmap/regmap-mdio.c
+++ b/drivers/base/regmap/regmap-mdio.c
@@ -5,16 +5,22 @@
 #include <linux/module.h>
 #include <linux/regmap.h>
 
+#define REGVAL_MASK		GENMASK(15, 0)
+#define REGNUM_C22_MASK		GENMASK(4, 0)
+
 static int regmap_mdio_read(void *context, unsigned int reg, unsigned int *val)
 {
 	struct mdio_device *mdio_dev = context;
 	int ret;
 
-	ret = mdiobus_read(mdio_dev->bus, mdio_dev->addr, reg);
+	if (unlikely(reg & ~REGNUM_C22_MASK))
+		return -ENXIO;
+
+	ret = mdiobus_read(mdio_dev->bus, mdio_dev->addr, reg & REGNUM_C22_MASK);
 	if (ret < 0)
 		return ret;
 
-	*val = ret & 0xffff;
+	*val = ret & REGVAL_MASK;
 	return 0;
 }
 
@@ -22,7 +28,10 @@ static int regmap_mdio_write(void *context, unsigned int reg, unsigned int val)
 {
 	struct mdio_device *mdio_dev = context;
 
-	return mdiobus_write(mdio_dev->bus, mdio_dev->addr, reg, val);
+	if (unlikely(reg & ~REGNUM_C22_MASK))
+		return -ENXIO;
+
+	return mdiobus_write(mdio_dev->bus, mdio_dev->addr, reg & REGNUM_C22_MASK, val);
 }
 
 static const struct regmap_bus regmap_mdio_bus = {
-- 
2.31.1

Re: [PATCH] regmap: mdio: Reject invalid clause-22 addresses

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 556 bytes --]

On Sat, Jun 05, 2021 at 10:31:18AM +0200, Sander Vanheule wrote:
> Currently a regmap configuration for regmap-mdio must have a register
> address width of 5 bits (cf. clause-22 register access). This is not
> enforced on the provided register addresses, which would enable
> clause-45 MDIO bus access, if the right bit packing is used.

Please don't send new patches in reply to old patch serieses, it makes
it hard to follow what's going on and what the current state of things
is and makes it easy for things to get missed when threads get cleaned
out.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regmap: mdio: Reject invalid clause-22 addresses

[Sander Vanheule]

On Mon, 2021-06-07 at 12:03 +0100, Mark Brown wrote:
> On Sat, Jun 05, 2021 at 10:31:18AM +0200, Sander Vanheule wrote:
> > Currently a regmap configuration for regmap-mdio must have a register
> > address width of 5 bits (cf. clause-22 register access). This is not
> > enforced on the provided register addresses, which would enable
> > clause-45 MDIO bus access, if the right bit packing is used.
> 
> Please don't send new patches in reply to old patch serieses, it makes
> it hard to follow what's going on and what the current state of things
> is and makes it easy for things to get missed when threads get cleaned
> out.

Sorry, my bad. I usually have an updated cover letter in between updated
patches, but this was only one patch. I'll include a LKML-link to the previous
version(s) in the future, to keep an easy reference around.

Best,
Sander

Re: [PATCH] regmap: mdio: Reject invalid clause-22 addresses

[Sander Vanheule]

Hi Mark,

On Mon, 2021-06-07 at 12:03 +0100, Mark Brown wrote:
> On Sat, Jun 05, 2021 at 10:31:18AM +0200, Sander Vanheule wrote:
> > Currently a regmap configuration for regmap-mdio must have a register
> > address width of 5 bits (cf. clause-22 register access). This is not
> > enforced on the provided register addresses, which would enable
> > clause-45 MDIO bus access, if the right bit packing is used.
> 
> Please don't send new patches in reply to old patch serieses, it makes
> it hard to follow what's going on and what the current state of things
> is and makes it easy for things to get missed when threads get cleaned
> out.

It appears that this has caused you to merge the RFC patches instead. I've
posted two new patches, to bring the code in line with the discussion on the
original RFC patches. See:
https://lore.kernel.org/lkml/cover.1623238313.git.sander@svanheule.net/

My apologies for the extra work this has caused.

Best,
Sander

Re: [RFC PATCH 0/2] Clause-22/Clause-45 MDIO regmap support

[Mark Brown]

On Thu, 3 Jun 2021 20:25:08 +0200, Sander Vanheule wrote:
> The initial MDIO regmap implementation only supported (or claimed to
> only support) clause-22 register access, with 5 bit register addresses.
> However, this was not enforced sufficiently, and regnum values were
> passed verbatim to the mdio bus subsystem.
> 
> These patches aim to enforce the register address width, and also add
> clause-45 support for extended address spaces. A clause-45 address is
> defined here as the composite of the device type (see MDIO_MMD_* in
> include/uapi/linux/mdio.h) and the register number, for a total width of
> 21 bit.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next

Thanks!

[1/2] regmap: mdio: Clean up invalid clause-22 addresses
      commit: dff404deb8493e6154ad75a62ce7c4e37ff8fccd
[2/2] regmap: mdio: Add clause-45 support
      commit: f083be9db060fbac09123d80bdffb2c001ac0e2b

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark