[RFC] regmap: clairify max_register meaning

[RFC] regmap: clairify max_register meaning

[Stefan Agner]

The exact meaning of max_register is not entirely clear. Define
it to be the maximum address offset a register in a regmap can
actually have. This seems to be the interpretation most commonly
used.

Fix regcache-flat to follow this definition.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
Hi Mark,

I stumbled upon this while switching to regmap cache FLAT for the
FSL DCU and PWM FTM drivers.

Maybe I see something completely wrong here, but to me it seems
that the regcache-flat.c is the only code interpreting max_register
as register indexes rather then the maximum relative address...
At least regmap.c compares with range_max, which seems to use
addresses.

If this is right, we should probably check the max_register values
of the drivers using REGCACHE_FLAT before fixing this... Some
drivers I checked seem to pass the maximum register address already.

--
Stefan

 drivers/base/regmap/regcache-flat.c | 3 +--
 include/linux/regmap.h              | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/base/regmap/regcache-flat.c b/drivers/base/regmap/regcache-flat.c
index 686c9e0..fe997c1 100644
--- a/drivers/base/regmap/regcache-flat.c
+++ b/drivers/base/regmap/regcache-flat.c
@@ -21,8 +21,7 @@ static int regcache_flat_init(struct regmap *map)
 	int i;
 	unsigned int *cache;
 
-	map->cache = kcalloc(map->max_register + 1, sizeof(unsigned int),
-			     GFP_KERNEL);
+	map->cache = kzalloc(map->max_register + map->reg_stride, GFP_KERNEL);
 	if (!map->cache)
 		return -ENOMEM;
 
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 1839434..27aaac9 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -162,7 +162,7 @@ typedef void (*regmap_unlock)(void *);
  *		  This field is a duplicate of a similar file in
  *		  'struct regmap_bus' and serves exact same purpose.
  *		   Use it only for "no-bus" cases.
- * @max_register: Optional, specifies the maximum valid register index.
+ * @max_register: Optional, specifies the maximum valid register address.
  * @wr_table:     Optional, points to a struct regmap_access_table specifying
  *                valid ranges for write access.
  * @rd_table:     As above, for read access.
-- 
2.7.0

Re: [RFC] regmap: clairify max_register meaning

[Mark Brown]

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

On Sun, Jan 17, 2016 at 10:52:56PM -0800, Stefan Agner wrote:

Please submit one change per patch.  You've mixed a change to the
documentation with a code here and it's really unclear what either of
them are supposed to do.

> Maybe I see something completely wrong here, but to me it seems
> that the regcache-flat.c is the only code interpreting max_register
> as register indexes rather then the maximum relative address...

I don't think I understand what the above means, sorry.  What is a
"relative address" in the context of a register map?

> At least regmap.c compares with range_max, which seems to use
> addresses.

That's a fairly large source file, can you be more specific please?

> -	map->cache = kcalloc(map->max_register + 1, sizeof(unsigned int),
> -			     GFP_KERNEL);
> +	map->cache = kzalloc(map->max_register + map->reg_stride, GFP_KERNEL);

This isn't very obvious and appears broken for a lot of devices.  It
does two things, it converts from allocating an array of unsigned
integers to an array of bytes and it replaces the handling of address 0
by adding an extra element to the array with adding stride bytes on the
end of the array.  This means that for a regmap with 16 bit values and a
stride of 1 we'll end up allocating nowhere the space we need as we'll
only allocate one byte per register plus an extra byte at the end but
the implementation of the flat cache is treating the cache as an array
of unsigned ints so needs at least four bytes per register.  It should
wash out as the same thing if the stride is equal to the size of an
unsigned int but most other cases will be broken.

Your changelog doesn't actually say this but I think what this is trying
to do is make the cache more memory efficient by not allocating space
for the registers that don't exist due to striding (ie, if we have a 4
byte stride then currently we'll allocate space for 4 times as many
registers as actually exist).  That's a reasonable goal but I don't
immediately have a good idea for doing it bearing in mind that we are
very performance sensitive here.

> - * @max_register: Optional, specifies the maximum valid register index.
> + * @max_register: Optional, specifies the maximum valid register address.

This is reasonable (index and address are synonyms here).

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

Re: [RFC] regmap: clairify max_register meaning

[Stefan Agner]

On 2016-01-18 08:05, Mark Brown wrote:
> On Sun, Jan 17, 2016 at 10:52:56PM -0800, Stefan Agner wrote:
> 
> Please submit one change per patch.  You've mixed a change to the
> documentation with a code here and it's really unclear what either of
> them are supposed to do.

Sure, one change per patch. I interpreted this issue as "one change"
since the interpretation of the doc lead to this implementation issue...

But I see, the issue in regcache-flat.c could be a completely
independent issue.

> 
>> Maybe I see something completely wrong here, but to me it seems
>> that the regcache-flat.c is the only code interpreting max_register
>> as register indexes rather then the maximum relative address...
> 
> I don't think I understand what the above means, sorry.  What is a
> "relative address" in the context of a register map?

My interpretation:

relative address = index * stride
absolute address = base address + relative address

For the driver at  hand I was about to set max_register to (max relative
address / 4), since that matches my interpretation of index and at first
sight seem to mach what is done in regcache-flat.c (it even works fine,
at least for 4 byte registers)....

> 
>> At least regmap.c compares with range_max, which seems to use
>> addresses.
> 
> That's a fairly large source file, can you be more specific please?
> 

e.g. in __regmap_init, just after the label skip_format_initialization.
(more on that see below)

>> -	map->cache = kcalloc(map->max_register + 1, sizeof(unsigned int),
>> -			     GFP_KERNEL);
>> +	map->cache = kzalloc(map->max_register + map->reg_stride, GFP_KERNEL);
> 
> This isn't very obvious and appears broken for a lot of devices.  It
> does two things, it converts from allocating an array of unsigned
> integers to an array of bytes and it replaces the handling of address 0
> by adding an extra element to the array with adding stride bytes on the
> end of the array.  This means that for a regmap with 16 bit values and a
> stride of 1 we'll end up allocating nowhere the space we need as we'll
> only allocate one byte per register plus an extra byte at the end but
> the implementation of the flat cache is treating the cache as an array
> of unsigned ints so needs at least four bytes per register.  It should
> wash out as the same thing if the stride is equal to the size of an
> unsigned int but most other cases will be broken.

I see, agreed, my code is certainly broken if strides <
sizeof(*reg_value).

> 
> Your changelog doesn't actually say this but I think what this is trying
> to do is make the cache more memory efficient by not allocating space
> for the registers that don't exist due to striding (ie, if we have a 4
> byte stride then currently we'll allocate space for 4 times as many
> registers as actually exist).  That's a reasonable goal but I don't
> immediately have a good idea for doing it bearing in mind that we are
> very performance sensitive here.

Yeah I saw a bug in that, but I see that this is actually just a not
very space efficient implementation detail.

> 
>> - * @max_register: Optional, specifies the maximum valid register index.
>> + * @max_register: Optional, specifies the maximum valid register address.
> 
> This is reasonable (index and address are synonyms here).

Ok, in that case I clearly interpreted index wrong.

Btw, range_min and range_max in struct regmap_range_cfg are defined as
memory addresses:
...
 * @range_min: Address of the lowest register address in virtual range. 
        
 * @range_max: Address of the highest register in virtual range.
...

Will send out a patch for the documentation change only, if you think
that this is a reasonable change too?

--
Stefan

Re: [RFC] regmap: clairify max_register meaning

[Mark Brown]

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

On Mon, Jan 18, 2016 at 09:19:52AM -0800, Stefan Agner wrote:
> On 2016-01-18 08:05, Mark Brown wrote:

> >> Maybe I see something completely wrong here, but to me it seems
> >> that the regcache-flat.c is the only code interpreting max_register
> >> as register indexes rather then the maximum relative address...

> > I don't think I understand what the above means, sorry.  What is a
> > "relative address" in the context of a register map?

> My interpretation:

> relative address = index * stride
> absolute address = base address + relative address

Oh, OK.  I would have got that for absolute address for MMIO but
definitely not relative address.  For relative addresses I'd interpret
index and address as synonyms.

> > Your changelog doesn't actually say this but I think what this is trying
> > to do is make the cache more memory efficient by not allocating space
> > for the registers that don't exist due to striding (ie, if we have a 4
> > byte stride then currently we'll allocate space for 4 times as many
> > registers as actually exist).  That's a reasonable goal but I don't
> > immediately have a good idea for doing it bearing in mind that we are
> > very performance sensitive here.

> Yeah I saw a bug in that, but I see that this is actually just a not
> very space efficient implementation detail.

Indeed.  It is true that having that 300% overhead in a common case is a
bit annoying so if you do have any ideas for fixing that without taking
a performance hit that'd be really good.

> >> - * @max_register: Optional, specifies the maximum valid register index.
> >> + * @max_register: Optional, specifies the maximum valid register address.

> > This is reasonable (index and address are synonyms here).
> 
> Ok, in that case I clearly interpreted index wrong.

> Btw, range_min and range_max in struct regmap_range_cfg are defined as
> memory addresses:
> ...
>  * @range_min: Address of the lowest register address in virtual range. 
>         
>  * @range_max: Address of the highest register in virtual range.
> ...

Not memory addresses, register addresses.  Remember that most regmap
register maps are not memory mapped at all but rather are completely
separate ranges on some external device (especially things using ranges,
you're normally not short enough on addresses on a memory mapped device
to bother with a window) so when looking at memory mapped devices it's
best to think only of the area that's actually mapped into the regmap.

> Will send out a patch for the documentation change only, if you think
> that this is a reasonable change too?

Yes, please do.

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