Message-ID: <c491d47f13873f6c7fc344a34d6fd141@agner.ch>
Date:	Mon, 18 Jan 2016 09:19:52 -0800
From:	Stefan Agner <stefan@...er.ch>
To:	Mark Brown <broonie@...nel.org>
Cc:	gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC] regmap: clairify max_register meaning

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

Powered by blists - more mailing lists