Question about regmap_range_cfg and regmap_mmio

Question about regmap_range_cfg and regmap_mmio

[Lars Möllendorf]

Hi,

this mail is copied from internal issue written in markdown - I hope
this is still readable as mail.

I am referring to kernel sources v4.9.87 but I think all my assumptions
do still apply to current kernel versions.

[regmap.h](https://elixir.bootlin.com/linux/v4.9.87/source/include/linux/regmap.h#L334)
contains:

```c
/**
 * Configuration for indirectly accessed or paged registers.
 * Registers, mapped to this virtual range, are accessed in two steps:
 *     1. page selector register update;
 *     2. access through data window registers.
 *
 * @name: Descriptive name for diagnostics
 *
 * @range_min: Address of the lowest register address in virtual range.
 * @range_max: Address of the highest register in virtual range.
 *
 * @page_sel_reg: Register with selector field.
 * @page_sel_mask: Bit shift for selector value.
 * @page_sel_shift: Bit mask for selector value.
 *
 * @window_start: Address of first (lowest) register in data window.
 * @window_len: Number of registers in data window.
 */
struct regmap_range_cfg {
	const char *name;

	/* Registers of virtual address range */
	unsigned int range_min;
	unsigned int range_max;

	/* Page selector for indirect addressing */
	unsigned int selector_reg;
	unsigned int selector_mask;
	int selector_shift;

	/* Data window (per each page) */
	unsigned int window_start;
	unsigned int window_len;
};
```

Unfortunately this seems not to work for MMIO devices.

In
[`__regmap_init()`](https://elixir.bootlin.com/linux/v4.9.87/source/drivers/base/regmap/regmap.c#L711)
[`_regmap_bus_reg_read()`](https://elixir.bootlin.com/linux/v4.9.87/source/drivers/base/regmap/regmap.c#L2330)
is assigned to
[`regmap.reg_read()`](https://elixir.bootlin.com/linux/v4.9.87/source/drivers/base/regmap/internal.h#L101)
if `!bus->read || !bus->write`, else
[`_regmap_bus_read()`](https://elixir.bootlin.com/linux/v4.9.87/source/drivers/base/regmap/regmap.c#L2338)
is assigned:

```c
	if (!bus) {
		map->reg_read  = config->reg_read;
		map->reg_write = config->reg_write;

		map->defer_caching = false;
		goto skip_format_initialization;
	} else if (!bus->read || !bus->write) {
		map->reg_read = _regmap_bus_reg_read;
		map->reg_write = _regmap_bus_reg_write;

		map->defer_caching = false;
		goto skip_format_initialization;
	} else {
		map->reg_read  = _regmap_bus_read;
		map->reg_update_bits = bus->reg_update_bits;
	}
```
[`_regmap_bus_reg_read()`](https://elixir.bootlin.com/linux/v4.9.87/source/drivers/base/regmap/regmap.c#L2330)
calls the `reg_read` function of the bus directly,
[`_regmap_bus_read()`](https://elixir.bootlin.com/linux/v4.9.87/source/drivers/base/regmap/regmap.c#L2338)
instead calls `_regmap_raw_read()`:

```c
static int _regmap_bus_reg_read(void *context, unsigned int reg,
				unsigned int *val)
{
	struct regmap *map = context;

	return map->bus->reg_read(map->bus_context, reg, val);
}

static int _regmap_bus_read(void *context, unsigned int reg,
			    unsigned int *val)
{
	int ret;
	struct regmap *map = context;

	if (!map->format.parse_val)
		return -EINVAL;

	ret = _regmap_raw_read(map, reg, map->work_buf, map->format.val_bytes);
	if (ret == 0)
		*val = map->format.parse_val(map->work_buf);

	return ret;
}
```

[`_regmap_raw_read()`](https://elixir.bootlin.com/linux/v4.9.87/source/drivers/base/regmap/regmap.c#L2297)
in turn calls
[`_regmap_range_lookup()`](https://elixir.bootlin.com/linux/v4.9.87/source/drivers/base/regmap/regmap-mmio.c#L479)
and
[`_regmap_select_page()`](https://elixir.bootlin.com/linux/v4.9.87/source/drivers/base/regmap/regmap-mmio.c#L1283)
which do the paging.

-
[`regmap_mmio`](https://elixir.bootlin.com/linux/v4.9.87/source/drivers/base/regmap/regmap-mmio.c#L212)
does neither contain `.read` nor `.write`.
-
[`regmap_i2c`](https://elixir.bootlin.com/linux/v4.9.87/source/drivers/base/regmap/regmap-i2c.c#L204)
does contain both.

My assumption is that paging is not a common use case for Memory-mapped
I/O and thus has not been implemented for this case.

- Are my assumptions correct?
- If so, what would you recommend me to do:
  - Continue using `regmap-mmio` and implement my custom paging
functions on top of that?
  - Enhance the current `regmap-mmio` implementation so it does paging
and submit a patch?
  - Write my own `better-regmap-mmio` implementation?

Thank you,
Lars

-- 

Lars Möllendorf, B. Eng.


Tel.:    +49 (0) 7641 93500-425
Fax:     +49 (0) 7641 93500-999
E-Mail:  lars.moellendorf@plating.de <mailto:lars.moellendorf@plating.de>
Website: www.plating.de <http://www.plating.de>

--------------------------------
plating electronic GmbH - Amtsgericht Freiburg - HRB Nr. 260 592 /
Geschäftsführer Karl Rieder / Rheinstraße 4 – 79350 Sexau – Tel.:+49 (0)
7641 – 93500-0

--------------------------------
Der Inhalt dieser E-Mail ist vertraulich und ausschließlich für den
bezeichneten Adressaten bestimmt. Wenn Sie nicht der vorgesehene
Adressat dieser E-Mail oder dessen Vertreter sein sollten, so beachten
Sie bitte, dass jede Form der Kenntnisnahme, Veröffentlichung,
Vervielfältigung oder Weitergabe des Inhalts dieser E-Mail unzulässig
ist. Wir bitten Sie, sich in diesem Fall mit dem Absender der E-Mail in
Verbindung zu setzen. Aussagen gegenüber  dem Adressaten unterliegen den
Regelungen des zugrundeliegenden Angebotes bzw. Auftrags, insbesondere
den Allgemeinen Geschäftsbedingungen und der individuellen
Haftungsvereinbarung. Der Inhalt der E-Mail ist nur rechtsverbindlich,
wenn er unsererseits durch einen Brief oder ein Telefax entsprechend
bestaetigt wird.

The information contained in this email is confidential. It is intended
solely for the addressee. Access to this email by anyone else is
unauthorized. If you are not the intended recipient, any form of
disclosure, reproduction, distribution or any action taken or refrained
from in reliance on it, is prohibited and may be unlawful. Please notify
the sender immediately. All statements of opinion or advice directed via
this email to our clients are subject to the terms and conditions
expressed in the governing client engagement letter. The content of this
email is not legally binding unless confirmed by letter or fax.

Although plating electronic GmbH attempts to sweep e-mail and
attachments for viruses, it does not guarantee that either are
virus-free and accepts no liability for any damage sustained as a result
of viruses.

Re: Question about regmap_range_cfg and regmap_mmio

[Mark Brown]

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

On Wed, Mar 04, 2020 at 12:25:09PM +0100, Lars Möllendorf wrote:

> this mail is copied from internal issue written in markdown - I hope
> this is still readable as mail.

Not really frankly.  I *think* you are saying that paging doesn't work
due to relying on having register read and write operations?

> My assumption is that paging is not a common use case for Memory-mapped
> I/O and thus has not been implemented for this case.

> - Are my assumptions correct?
> - If so, what would you recommend me to do:
>   - Continue using `regmap-mmio` and implement my custom paging
> functions on top of that?

This will obviously work.

>   - Enhance the current `regmap-mmio` implementation so it does paging
> and submit a patch?

That's not really possible since MMIO never writes the register address
to the bus.

>   - Write my own `better-regmap-mmio` implementation?

It's not clear what that would mean.

You could also look into making the paging code not rely on explicit
register read and write operations.

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

Re: Question about regmap_range_cfg and regmap_mmio

[Lars Möllendorf]

On 04.03.20 13:03, Mark Brown wrote:
> On Wed, Mar 04, 2020 at 12:25:09PM +0100, Lars Möllendorf wrote:
> 
>> this mail is copied from internal issue written in markdown - I hope
>> this is still readable as mail.
> 
> Not really frankly.  
Ok, sorry. Here the same text without code snippets and links:

In `__regmap_init()` `_regmap_bus_reg_read()`is assigned to
`regmap.reg_read()` if there are no `bus->read/write` functions, else
`_regmap_bus_read()` is assigned.

`_regmap_bus_reg_read()` calls the `reg_read` function of the bus
directly, `_regmap_bus_read()` instead calls `_regmap_raw_read()`.

`_regmap_raw_read()` in turn calls `_regmap_range_lookup()` and
`_regmap_select_page()` which do the paging.

`regmap_mmio` does not contain `bus->read/write`, but does contain
`bus->reg_read/reg_write` only.

>> `regmap_i2c` does -contain both-.

Sorry, I have to correct myself here. It does only contain the
`bus->read/write` functions.

> I *think* you are saying that paging doesn't work
> due to relying on having register read and write operations?

If I understand correctly it is relying on having plain
`bus->read/write` operations. MMIO *has* `bus->reg_read/reg_write` but
is missing the former. But maybe I just mix up the wording here.


>> My assumption is that paging is not a common use case for Memory-mapped
>> I/O and thus has not been implemented for this case.
> 
>> - Are my assumptions correct?
>> - If so, what would you recommend me to do:
>>   - Continue using `regmap-mmio` and implement my custom paging
>> functions on top of that?
> 
> This will obviously work.

Yes, this works. But it comes at the cost of implementing (and
maintaining) something which already exists. And the existing
implementation which uses virtual memory addresses is much smarter than
my current implementation.

>>   - Enhance the current `regmap-mmio` implementation so it does paging
>> and submit a patch?
> 
> That's not really possible since MMIO never writes the register address
> to the bus

Sorry, but I do not get why this shouldn't work with MMIO? If I
understood the code correctly in  `_regmap_raw_read` the address is
checked before it is used anywhere. If
`_regmap_range_lookup()` returns a range it does the paging, i.e.
translates the virtual address into the real address
(`_regmap_select_page`). If so the real address is passed to
`bus->read/write`, else the given address is used directly. Do I miss
something here?

>>   - Write my own `better-regmap-mmio` implementation?
> 
> It's not clear what that would mean.

Maybe for some reason the current MMIO implementation should not be
touched, or paging for MMIO is not wanted?

> You could also look into making the paging code not rely on explicit
> register read and write operations.

Maybe it is sufficient to implement `bus->read/write` as a wrapper of
`bus->reg_read/reg_write` in regmap-mmio.c?

Re: Question about regmap_range_cfg and regmap_mmio

[Mark Brown]

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

On Wed, Mar 04, 2020 at 02:28:25PM +0100, Lars Möllendorf wrote:
> On 04.03.20 13:03, Mark Brown wrote:
> > On Wed, Mar 04, 2020 at 12:25:09PM +0100, Lars Möllendorf wrote:

> In `__regmap_init()` `_regmap_bus_reg_read()`is assigned to
> `regmap.reg_read()` if there are no `bus->read/write` functions, else
> `_regmap_bus_read()` is assigned.
> 
> `_regmap_bus_reg_read()` calls the `reg_read` function of the bus
> directly, `_regmap_bus_read()` instead calls `_regmap_raw_read()`.
> 
> `_regmap_raw_read()` in turn calls `_regmap_range_lookup()` and
> `_regmap_select_page()` which do the paging.
> 
> `regmap_mmio` does not contain `bus->read/write`, but does contain
> `bus->reg_read/reg_write` only.

This is basically just saying that the paging is done in the bulk I/O
code only (partly for performance, partly since we may need to page in
the middle of a bulk operation), not in the per-register I/O paths.

> >>   - Enhance the current `regmap-mmio` implementation so it does paging
> >> and submit a patch?

> > That's not really possible since MMIO never writes the register address
> > to the bus

> Sorry, but I do not get why this shouldn't work with MMIO? If I
> understood the code correctly in  `_regmap_raw_read` the address is
> checked before it is used anywhere. If
> `_regmap_range_lookup()` returns a range it does the paging, i.e.
> translates the virtual address into the real address
> (`_regmap_select_page`). If so the real address is passed to
> `bus->read/write`, else the given address is used directly. Do I miss
> something here?

The plain read and write operations sit at the bottom of a stack that
(especially in the write path) deals with byte streams rather than
parsed data, they're only differentiated for read since there's a mix of
read and write I/O in a read operation.  Since for MMIO the register is
never part of a byte stream you'd need to contort things to parse the
address back out of the byte stream which is not great for abstraction
and likely to lead to bugs as different parts of the stack get confused
about what is handling endinaness and register size issues.

> >>   - Write my own `better-regmap-mmio` implementation?

> > It's not clear what that would mean.

> Maybe for some reason the current MMIO implementation should not be
> touched, or paging for MMIO is not wanted?

That doesn't really answer the question - what such an implementation
would look like?

> > You could also look into making the paging code not rely on explicit
> > register read and write operations.

> Maybe it is sufficient to implement `bus->read/write` as a wrapper of
> `bus->reg_read/reg_write` in regmap-mmio.c?

For the reasons outlined above that's an abstraction problem.  This is
not something that should be being done in the physical I/O code, that's
not the right layer of abstraction.  Like I say push it up into the
core.

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