drivers/mtd_mapper: inherit physical properties

[benpicco]

Contribution description

A MTD mapper region must adhere to the same physical constraints as the underlying MTD device, so inherit it's physical properties instead of relying on the user to set them correctly.

Testing procedure

tests/mtd_mapper still works without the redundant information

RIOT native interrupts/signals initialized.
RIOT native board initialized.
RIOT native hardware initialization complete.

Help: Press s to start test, r to print it is ready
s
START
main(): This is RIOT! (Version: 2022.07-devel-448-ge28040-drivers/mtd_mapper-inherit)
......
OK (6 tests)
{ "threads": [{ "name": "idle", "stack_size": 8192, "stack_used": 436 }]}
{ "threads": [{ "name": "main", "stack_size": 12288, "stack_used": 2508 }]}

Issues/PRs references

[chrysn]

Might this have been assert-only so far to allow the maps to be static const? It's currently not tested for that this is allowed, but it might have been the original intention.

[edit] I looked around a bit more in the code -- the _lock() looks like it's writing, but that write goes through indirections, so at least from there statidc const should have been possible.

[benpicco]

mtd_dev_t is usually writable to

• allocate the .work_area buffer
• set .pages_per_sector / .sector_count at run-time for SPI Flash and SD cards where this information can be queried from the device

[fjmolinas]

Might this have been assert-only so far to allow the maps to be static const? It's currently not tested for that this is allowed, but it might have been the original intention.

[edit] I looked around a bit more in the code -- the _lock() looks like it's writing, but that write goes through indirections, so at least from there statidc const should have been possible.

Lets summon @bergzand then :)

[Laczen]

@benpicco are all these properties really physical properties? The write_size and sector_size are, the page_size and pages_per_sector can be changed as long as the sector size is kept. Allowing changing the page_size allows easier subsystem definition (e.g. Littlefs that IIRC uses the page_size as the block_size).

[benpicco]

Huh? page_size is the largest block of data that can be written at once (when aligned to the page boundary).

It could be changed to a lower fraction, but to what gain?

LittleFS does

fs->config.block_size = fs->dev->page_size * fs->dev->pages_per_sector;
fs->config.prog_size = fs->dev->page_size;
fs->config.read_size = fs->dev->page_size;

where they use block instead of sector to name the erase unit.

[Laczen]

Huh? page_size is the largest block of data that can be written at once (when aligned to the page boundary).

It could be changed to a lower fraction, but to what gain?

LittleFS does

fs->config.block_size = fs->dev->page_size * fs->dev->pages_per_sector;
fs->config.prog_size = fs->dev->page_size;
fs->config.read_size = fs->dev->page_size;

where they use block instead of sector to name the erase unit.

I indeed missed in the definition, I meant the page_size is used as the prog_size in LittleFS. That the page_size is the largest block that can be written at once is new to me. The gain of defining a smaller block_size is better flash utilization.

[fabian18]

The gain of defining a smaller block_size is better flash utilization.

If people find a reason not to inherit the physical properties, we could only inherit them in _init(), if they are 0.
static memory is 0 if uninitialized. So if they have been set before _init() we can assume that someone does not want to inherit parent properties.

[benpicco]

But we already have

assert(backing_mtd->page_size == region->mtd.page_size);
assert(backing_mtd->pages_per_sector == region->mtd.pages_per_sector);

so setting this to anything else would currently fail in master.

[Laczen]

But we already have

assert(backing_mtd->page_size == region->mtd.page_size);
assert(backing_mtd->pages_per_sector == region->mtd.pages_per_sector);

so setting this to anything else would currently fail in master.

Maybe this was too limiting from the start ?

If the page_size would be defined as the largest block that can be written at once (needs to be documented) the checks should be:
a. region page_size <= backing page_size,
b. region page_size >= backing write_size,
c. region page_size * region pages_per_sector == backing page_size * backing pages_per_sector

[bergzand]

Lets summon @bergzand then :)

There might have been some intention to make it const at some point. But that's not going to happen at this point. I'm fine with this PR as it is.

[fabian18]

I am also happy with this. I would also add some kind of initializer macro but that´s not the topic here.