[PATCH] regmap: apply reg_base and reg_downshift for single register ops

[PATCH] regmap: apply reg_base and reg_downshift for single register ops

[Daniel Golle]

reg_base and reg_downshift currently don't have any effect if used with
simple single register operations.

Fix that by taking them into account also for _reg_read, _reg_write and
_reg_update_bits (they may still be missing also in other place, eg.
page selection code).

Fixes: 0074f3f2b1e43d ("regmap: allow a defined reg_base to be added to every address")
Fixes: 86fc59ef818beb ("regmap: add configurable downshift for addresses")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/base/regmap/regmap.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index d12d669157f24..7b8386ec21b8c 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -1986,6 +1986,8 @@ int _regmap_write(struct regmap *map, unsigned int reg,
 		}
 	}
 
+	reg += map->reg_base;
+	reg >>= map->format.reg_downshift;
 	ret = map->reg_write(context, reg, val);
 	if (ret == 0) {
 		if (regmap_should_log(map))
@@ -2879,6 +2881,8 @@ static int _regmap_read(struct regmap *map, unsigned int reg,
 	if (!regmap_readable(map, reg))
 		return -EIO;
 
+	reg += map->reg_base;
+	reg >>= map->format.reg_downshift;
 	ret = map->reg_read(context, reg, val);
 	if (ret == 0) {
 		if (regmap_should_log(map))
@@ -3231,6 +3235,8 @@ static int _regmap_update_bits(struct regmap *map, unsigned int reg,
 		*change = false;
 
 	if (regmap_volatile(map, reg) && map->reg_update_bits) {
+		reg += map->reg_base;
+		reg >>= map->format.reg_downshift;
 		ret = map->reg_update_bits(map->bus_context, reg, mask, val);
 		if (ret == 0 && change)
 			*change = true;

base-commit: e2f86c02fdc96ca29ced53221a3cbf50aa6f8b49
-- 
2.39.1

Re: [PATCH] regmap: apply reg_base and reg_downshift for single register ops

[Colin Foster]

Hi Daniel,

On Sat, Jan 28, 2023 at 03:10:01AM +0000, Daniel Golle wrote:
> reg_base and reg_downshift currently don't have any effect if used with
> simple single register operations.
> 
> Fix that by taking them into account also for _reg_read, _reg_write and
> _reg_update_bits (they may still be missing also in other place, eg.
> page selection code).
> 
> Fixes: 0074f3f2b1e43d ("regmap: allow a defined reg_base to be added to every address")
> Fixes: 86fc59ef818beb ("regmap: add configurable downshift for addresses")
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
>  drivers/base/regmap/regmap.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
> index d12d669157f24..7b8386ec21b8c 100644
> --- a/drivers/base/regmap/regmap.c
> +++ b/drivers/base/regmap/regmap.c
> @@ -1986,6 +1986,8 @@ int _regmap_write(struct regmap *map, unsigned int reg,
>  		}
>  	}
>  
> +	reg += map->reg_base;
> +	reg >>= map->format.reg_downshift;
>  	ret = map->reg_write(context, reg, val);
>  	if (ret == 0) {
>  		if (regmap_should_log(map))
> @@ -2879,6 +2881,8 @@ static int _regmap_read(struct regmap *map, unsigned int reg,
>  	if (!regmap_readable(map, reg))
>  		return -EIO;
>  
> +	reg += map->reg_base;
> +	reg >>= map->format.reg_downshift;

Something more subtle is going on.

Here's a stack dump from a write and a read:

[    3.238249]  dump_backtrace from show_stack+0x20/0x24
[    3.243349]  r7:c3a27c00 r6:00000000 r5:c17aa2d8 r4:60000113
[    3.249034]  show_stack from dump_stack_lvl+0x60/0x78
[    3.254121]  dump_stack_lvl from dump_stack+0x18/0x1c
[    3.259208]  r7:c3a27c00 r6:c3a5a400 r5:00000007 r4:c1e64d5c
[    3.264892]  dump_stack from ocelot_spi_regmap_bus_write+0x9c/0xa8
[    3.271113]  ocelot_spi_regmap_bus_write from _regmap_raw_write_impl+0x60c/0x8b8
[    3.278555]  r7:00000000 r6:c3a5a403 r5:c1dfe160 r4:c3a1ba00
[    3.284239]  _regmap_raw_write_impl from _regmap_bus_raw_write+0x7c/0xa0
[    3.290982]  r10:df9bd164 r9:df9bd164 r8:c3a1ba00 r7:00000000 r6:00000000 r5:00000000
[    3.298847]  r4:c3a1ba00
[    3.301391]  _regmap_bus_raw_write from _regmap_write+0x64/0x174
[    3.307448]  r6:00000000 r5:00000000 r4:c3a1ba00
[    3.312085]  _regmap_write from regmap_write+0x4c/0x6c
[    3.317263]  r9:df9bd164 r8:c1d9e1e0 r7:00000000 r6:00000000 r5:00000000 r4:c3a1ba00
[    3.325040]  regmap_write from ocelot_spi_initialize+0x44/0xc0
[    3.330910]  r7:00000000 r6:c1da1904 r5:c3a58d40 r4:c3a58d40
[    3.336595]  ocelot_spi_initialize from ocelot_spi_probe+0x9c/0x178


[    3.753685]  dump_backtrace from show_stack+0x20/0x24
[    3.758777]  r7:00000004 r6:00000000 r5:c17aa2d8 r4:60000113
[    3.764462]  show_stack from dump_stack_lvl+0x60/0x78
[    3.769547]  dump_stack_lvl from dump_stack+0x18/0x1c
[    3.774633]  r7:00000004 r6:c3a5a403 r5:c1e64d5c r4:c3a27c00
[    3.780317]  dump_stack from ocelot_spi_regmap_bus_read+0x144/0x150
[    3.786623]  ocelot_spi_regmap_bus_read from _regmap_raw_read+0x114/0x2d4
[    3.793455]  r9:df9bd164 r8:c1dfe160 r7:c3a5a403 r6:00000004 r5:c3a1ba00 r4:c0a84140
[    3.801232]  _regmap_raw_read from _regmap_bus_read+0x54/0x80
[    3.807016]  r10:df9bd164 r9:df9bd164 r8:c1d9e1e0 r7:e0055ab8 r6:c3a5a403 r5:00000004
[    3.814881]  r4:c3a1ba00
[    3.817425]  _regmap_bus_read from _regmap_read+0x70/0x190
[    3.822952]  r7:e0055ab8 r6:c3a1ba00 r5:00000004 r4:c3a1ba00
[    3.828635]  _regmap_read from regmap_read+0x4c/0x6c
[    3.833642]  r10:df9bd164 r9:df9bd164 r8:c1d9e1e0 r7:00000000 r6:e0055ab8 r5:00000004
[    3.841508]  r4:c3a1ba00 r3:c21fc700
[    3.845098]  regmap_read from ocelot_spi_initialize+0xa0/0xc0
[    3.850886]  r7:00000000 r6:c1da1904 r5:c3a58d40 r4:00000001
[    3.856570]  ocelot_spi_initialize from ocelot_spi_probe+0x9c/0x178


So applying this in both _regmap_write and _regmap_raw_write_impl cause
the operations to happen twice. Similarly with _regmap_read and
_regmap_raw_read.

Rewinding my brain back a year - I also didn't want to tamper with the
reg value before any cache checks. Those operations are at the very end
of the processing chain. In my scenario, I'm getting a 4-byte register
at address 0x4. The fact that it needs to go out a SPI bus as 0x1
shouldn't change any other logic in the system.

And my other _main_ motivation was to use bus reads, so that bulk
transfers were possible. My initial implementations didn't use the bus
interface, so I did all address manipulation in my custom read / write
functions. Bulk transfers offered an order of magnitude improvement in
access time.


Perhaps there is some confusion due to my field description in
include/linux/regmap.h, and it should reference "any bus operation"? Or
something similar...


>  	ret = map->reg_read(context, reg, val);
>  	if (ret == 0) {
>  		if (regmap_should_log(map))
> @@ -3231,6 +3235,8 @@ static int _regmap_update_bits(struct regmap *map, unsigned int reg,
>  		*change = false;
>  
>  	if (regmap_volatile(map, reg) && map->reg_update_bits) {
> +		reg += map->reg_base;
> +		reg >>= map->format.reg_downshift;
>  		ret = map->reg_update_bits(map->bus_context, reg, mask, val);
>  		if (ret == 0 && change)
>  			*change = true;
> 
> base-commit: e2f86c02fdc96ca29ced53221a3cbf50aa6f8b49
> -- 
> 2.39.1
> 

Colin Foster

Re: [PATCH] regmap: apply reg_base and reg_downshift for single register ops

[Daniel Golle]

Hi Colin,

On Sat, Jan 28, 2023 at 12:06:20PM -0800, Colin Foster wrote:
> Hi Daniel,
> 
> On Sat, Jan 28, 2023 at 03:10:01AM +0000, Daniel Golle wrote:
> > reg_base and reg_downshift currently don't have any effect if used with
> > simple single register operations.
> > 
> > Fix that by taking them into account also for _reg_read, _reg_write and
> > _reg_update_bits (they may still be missing also in other place, eg.
> > page selection code).
> > 
> > Fixes: 0074f3f2b1e43d ("regmap: allow a defined reg_base to be added to every address")
> > Fixes: 86fc59ef818beb ("regmap: add configurable downshift for addresses")
> > Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> > ---
> >  drivers/base/regmap/regmap.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
> > index d12d669157f24..7b8386ec21b8c 100644
> > --- a/drivers/base/regmap/regmap.c
> > +++ b/drivers/base/regmap/regmap.c
> > @@ -1986,6 +1986,8 @@ int _regmap_write(struct regmap *map, unsigned int reg,
> >  		}
> >  	}
> >  
> > +	reg += map->reg_base;
> > +	reg >>= map->format.reg_downshift;
> >  	ret = map->reg_write(context, reg, val);
> >  	if (ret == 0) {
> >  		if (regmap_should_log(map))
> > @@ -2879,6 +2881,8 @@ static int _regmap_read(struct regmap *map, unsigned int reg,
> >  	if (!regmap_readable(map, reg))
> >  		return -EIO;
> >  
> > +	reg += map->reg_base;
> > +	reg >>= map->format.reg_downshift;
> 
> Something more subtle is going on.
> 
> Here's a stack dump from a write and a read:
> 
> [    3.238249]  dump_backtrace from show_stack+0x20/0x24
> [    3.243349]  r7:c3a27c00 r6:00000000 r5:c17aa2d8 r4:60000113
> [    3.249034]  show_stack from dump_stack_lvl+0x60/0x78
> [    3.254121]  dump_stack_lvl from dump_stack+0x18/0x1c
> [    3.259208]  r7:c3a27c00 r6:c3a5a400 r5:00000007 r4:c1e64d5c
> [    3.264892]  dump_stack from ocelot_spi_regmap_bus_write+0x9c/0xa8
> [    3.271113]  ocelot_spi_regmap_bus_write from _regmap_raw_write_impl+0x60c/0x8b8
> [    3.278555]  r7:00000000 r6:c3a5a403 r5:c1dfe160 r4:c3a1ba00
> [    3.284239]  _regmap_raw_write_impl from _regmap_bus_raw_write+0x7c/0xa0
> [    3.290982]  r10:df9bd164 r9:df9bd164 r8:c3a1ba00 r7:00000000 r6:00000000 r5:00000000
> [    3.298847]  r4:c3a1ba00
> [    3.301391]  _regmap_bus_raw_write from _regmap_write+0x64/0x174
> [    3.307448]  r6:00000000 r5:00000000 r4:c3a1ba00
> [    3.312085]  _regmap_write from regmap_write+0x4c/0x6c
> [    3.317263]  r9:df9bd164 r8:c1d9e1e0 r7:00000000 r6:00000000 r5:00000000 r4:c3a1ba00
> [    3.325040]  regmap_write from ocelot_spi_initialize+0x44/0xc0
> [    3.330910]  r7:00000000 r6:c1da1904 r5:c3a58d40 r4:c3a58d40
> [    3.336595]  ocelot_spi_initialize from ocelot_spi_probe+0x9c/0x178
> 
> 
> [    3.753685]  dump_backtrace from show_stack+0x20/0x24
> [    3.758777]  r7:00000004 r6:00000000 r5:c17aa2d8 r4:60000113
> [    3.764462]  show_stack from dump_stack_lvl+0x60/0x78
> [    3.769547]  dump_stack_lvl from dump_stack+0x18/0x1c
> [    3.774633]  r7:00000004 r6:c3a5a403 r5:c1e64d5c r4:c3a27c00
> [    3.780317]  dump_stack from ocelot_spi_regmap_bus_read+0x144/0x150
> [    3.786623]  ocelot_spi_regmap_bus_read from _regmap_raw_read+0x114/0x2d4
> [    3.793455]  r9:df9bd164 r8:c1dfe160 r7:c3a5a403 r6:00000004 r5:c3a1ba00 r4:c0a84140
> [    3.801232]  _regmap_raw_read from _regmap_bus_read+0x54/0x80
> [    3.807016]  r10:df9bd164 r9:df9bd164 r8:c1d9e1e0 r7:e0055ab8 r6:c3a5a403 r5:00000004
> [    3.814881]  r4:c3a1ba00
> [    3.817425]  _regmap_bus_read from _regmap_read+0x70/0x190
> [    3.822952]  r7:e0055ab8 r6:c3a1ba00 r5:00000004 r4:c3a1ba00
> [    3.828635]  _regmap_read from regmap_read+0x4c/0x6c
> [    3.833642]  r10:df9bd164 r9:df9bd164 r8:c1d9e1e0 r7:00000000 r6:e0055ab8 r5:00000004
> [    3.841508]  r4:c3a1ba00 r3:c21fc700
> [    3.845098]  regmap_read from ocelot_spi_initialize+0xa0/0xc0
> [    3.850886]  r7:00000000 r6:c1da1904 r5:c3a58d40 r4:00000001
> [    3.856570]  ocelot_spi_initialize from ocelot_spi_probe+0x9c/0x178
> 
> 
> So applying this in both _regmap_write and _regmap_raw_write_impl cause
> the operations to happen twice. Similarly with _regmap_read and
> _regmap_raw_read.

Ok, so I'll need another way to address this. Will dig more ;)

> Rewinding my brain back a year - I also didn't want to tamper with the
> reg value before any cache checks. Those operations are at the very end
> of the processing chain. In my scenario, I'm getting a 4-byte register
> at address 0x4. The fact that it needs to go out a SPI bus as 0x1
> shouldn't change any other logic in the system.
> 
> And my other _main_ motivation was to use bus reads, so that bulk
> transfers were possible. My initial implementations didn't use the bus
> interface, so I did all address manipulation in my custom read / write
> functions. Bulk transfers offered an order of magnitude improvement in
> access time.

I'm using a regmap_bus with only reg_read and reg_write functions, in
this case the value of reg_base is currently not taken into account.

> Perhaps there is some confusion due to my field description in
> include/linux/regmap.h, and it should reference "any bus operation"? Or
> something similar...

I suppose so, as that's where my expectation regarding reg_base also
being applied on regmap_read operations on e.g. regmap-mdio stem
from. I also expected reg_base base to affect the 'registers' file in
debugfs (ie. offset being applied there as well).

To get an idea what I was doing while encountering this problem, see
the top 4 commits here:

https://github.com/dangowrt/linux/commits/wip

[RFC PATCH v2] regmap: apply reg_base and reg_downshift for single register ops

[Daniel Golle]

reg_base and reg_downshift currently don't have any effect if used with
a regmap_bus or regmap_config which only offers single register
operations (ie. reg_read, reg_write and optionally reg_update_bits).

Fix that and take them into account also for regmap_bus with only
reg_read and read_write operations by applying reg_base and
reg_downshift in _regmap_bus_reg_write, _regmap_bus_reg_read.

Also apply reg_base and reg_downshift in _regmap_update_bits, but only
in case the operation is carried out with a reg_update_bits call
defined in either regmap_bus or regmap_config.

Fixes: 0074f3f2b1e43d ("regmap: allow a defined reg_base to be added to every address")
Fixes: 86fc59ef818beb ("regmap: add configurable downshift for addresses")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
I hope that I didn't miss anything there...

@Colin Please let me know if this breaks anything with your ocelot_spi
use-case.

 drivers/base/regmap/regmap.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index d12d669157f24..d2a54eb0efd9b 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -1942,6 +1942,8 @@ static int _regmap_bus_reg_write(void *context, unsigned int reg,
 {
 	struct regmap *map = context;
 
+	reg += map->reg_base;
+	reg >>= map->format.reg_downshift;
 	return map->bus->reg_write(map->bus_context, reg, val);
 }
 
@@ -2840,6 +2842,8 @@ static int _regmap_bus_reg_read(void *context, unsigned int reg,
 {
 	struct regmap *map = context;
 
+	reg += map->reg_base;
+	reg >>= map->format.reg_downshift;
 	return map->bus->reg_read(map->bus_context, reg, val);
 }
 
@@ -3231,6 +3235,8 @@ static int _regmap_update_bits(struct regmap *map, unsigned int reg,
 		*change = false;
 
 	if (regmap_volatile(map, reg) && map->reg_update_bits) {
+		reg += map->reg_base;
+		reg >>= map->format.reg_downshift;
 		ret = map->reg_update_bits(map->bus_context, reg, mask, val);
 		if (ret == 0 && change)
 			*change = true;
-- 
2.39.1

Re: [RFC PATCH v2] regmap: apply reg_base and reg_downshift for single register ops

[Colin Foster]

Hi Daniel,

On Mon, Jan 30, 2023 at 02:04:57AM +0000, Daniel Golle wrote:
> reg_base and reg_downshift currently don't have any effect if used with
> a regmap_bus or regmap_config which only offers single register
> operations (ie. reg_read, reg_write and optionally reg_update_bits).
> 
> Fix that and take them into account also for regmap_bus with only
> reg_read and read_write operations by applying reg_base and
> reg_downshift in _regmap_bus_reg_write, _regmap_bus_reg_read.
> 
> Also apply reg_base and reg_downshift in _regmap_update_bits, but only
> in case the operation is carried out with a reg_update_bits call
> defined in either regmap_bus or regmap_config.
> 
> Fixes: 0074f3f2b1e43d ("regmap: allow a defined reg_base to be added to every address")
> Fixes: 86fc59ef818beb ("regmap: add configurable downshift for addresses")
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
> I hope that I didn't miss anything there...
> 
> @Colin Please let me know if this breaks anything with your ocelot_spi
> use-case.

I see we're working on similar things! (DSA hardware, that is)

This patch works for me. I don't konw if there's any value in
back-porting it to affected kernels, as ocelot_spi is the only user as
far as I can tell. (wishing I called it something more greppable than
'reg_base')

Tested-by: Colin Foster <colin.foster@in-advantage.com>

> 
>  drivers/base/regmap/regmap.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
> index d12d669157f24..d2a54eb0efd9b 100644
> --- a/drivers/base/regmap/regmap.c
> +++ b/drivers/base/regmap/regmap.c
> @@ -1942,6 +1942,8 @@ static int _regmap_bus_reg_write(void *context, unsigned int reg,
>  {
>  	struct regmap *map = context;
>  
> +	reg += map->reg_base;
> +	reg >>= map->format.reg_downshift;
>  	return map->bus->reg_write(map->bus_context, reg, val);
>  }
>  
> @@ -2840,6 +2842,8 @@ static int _regmap_bus_reg_read(void *context, unsigned int reg,
>  {
>  	struct regmap *map = context;
>  
> +	reg += map->reg_base;
> +	reg >>= map->format.reg_downshift;
>  	return map->bus->reg_read(map->bus_context, reg, val);
>  }
>  
> @@ -3231,6 +3235,8 @@ static int _regmap_update_bits(struct regmap *map, unsigned int reg,
>  		*change = false;
>  
>  	if (regmap_volatile(map, reg) && map->reg_update_bits) {
> +		reg += map->reg_base;
> +		reg >>= map->format.reg_downshift;
>  		ret = map->reg_update_bits(map->bus_context, reg, mask, val);
>  		if (ret == 0 && change)
>  			*change = true;
> -- 
> 2.39.1
> 

Re: [RFC PATCH v2] regmap: apply reg_base and reg_downshift for single register ops

[Mark Brown]

On Mon, 30 Jan 2023 02:04:57 +0000, Daniel Golle wrote:
> reg_base and reg_downshift currently don't have any effect if used with
> a regmap_bus or regmap_config which only offers single register
> operations (ie. reg_read, reg_write and optionally reg_update_bits).
> 
> Fix that and take them into account also for regmap_bus with only
> reg_read and read_write operations by applying reg_base and
> reg_downshift in _regmap_bus_reg_write, _regmap_bus_reg_read.
> 
> [...]

Applied to

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

Thanks!

[1/1] regmap: apply reg_base and reg_downshift for single register ops
      commit: 697c3892d825fb78f42ec8e53bed065dd728db3e

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