Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nvmp: api for non volatile memory #62175

Closed
wants to merge 1 commit into from

Conversation

Laczen
Copy link
Collaborator

@Laczen Laczen commented Aug 31, 2023

Renamed the proposal from mtd to nvmp.

The nvmp system enables support for working with non volatile memory and partitions on non volatile memory. The PR enables support for using non volatile memory on FLASH, EEPROM or RETAINED_MEM devices. Other types (e.g. disks or even files can be added as future extensions).

The nvmp api provides unified read, write and erase methods. When a "backend" has no erase method a write with fixed values is used for erase. A device is declared as non volatile memory by adding the compatible "zephyr, nvmp_flash", "zephyr, nvmp-eeprom", "zephyr,nvmp-retained_mem". All non volatile memories can be partitioned using a "zephyr,nvmp-partition" compatible that is equal to the present "fixed-partition" compatible.

The non volatile memory type can be retrieved and used when needed (e.g. to avoid an erase when it is not needed by the non volatile memory type).

The storage solution (e.g. the device) can be retrieved and all methods available to the storage solution can then be used (e.g. getting the flash "page" info for a flash device).

The proposed api can be used for:
a. Working with images as an alternative to flash_map. This allows images to reside on the different types of non volatile memory. It is extendable to disks an files,
b. Simplify the enabling of fs solutions on different types of non volatile memory,
c. Simplify the coredump backends,
d. Simplify the logging backends,
e. Simplify the "emulated" disks,
f. Share information between a bootloader and an application without forcing the bootloader to use the subsystem.

Fixes #61979,
Could be used to fix #52395,

At the moment a separate compatible is used to define partitions, the proposed solution can be modified to reuse the "fixed-partition" compatible.

Rationale behind this proposal:
All types of non volatile memory share properties like read and write methods and some require an erase method. There are several types available but they don't share a method to partition and each have their own method to write/read/(erase). Also they differ on getting basic info like getting the size at build time.
Developing for each type of non volatile memory is tedious and requires knowledge of the type of non volatile memory and the driver. By making a unified solution the users can keep their focus on the problem at hand.
The difficulty of the driver interaction is moved to the nvmp subsystem and this can be done once.
The subsystem also provides support to achieve minimal compile size as is illustrated by NVMP_HAS_COMPATIBLE(node, compatible) that allows the code to select the (only) used type of non volatile memory and to remove any unused types.

@Laczen Laczen added the DNM This PR should not be merged (Do Not Merge) label Aug 31, 2023
@Laczen Laczen added the Architecture Review Discussion in the Architecture WG required label Aug 31, 2023
@zephyrbot zephyrbot added area: EEPROM area: Devicetree Binding PR modifies or adds a Device Tree binding labels Aug 31, 2023
@Laczen Laczen force-pushed the mtd_api branch 5 times, most recently from ac5ae90 to e83094b Compare September 1, 2023 08:47
@zephyrbot zephyrbot added the area: Storage Storage subsystem label Sep 1, 2023
@Laczen Laczen force-pushed the mtd_api branch 10 times, most recently from a205dd9 to 02b86e7 Compare September 5, 2023 07:16
@nordicjm
Copy link
Collaborator

nordicjm commented Sep 6, 2023

One extra remark:

Q: Does the use of the subsystem oblige developers to support all non volatile memory? A: No it does not, the developer can decide based upon the non volatile memory type go exclude a memory from use.

I don't really see the point in this then, so if this became the normal for interacting with devices, let's say nvs or lfs support moves over to it but actually only really supports the flash backend because it relies upon flash writing tricks, all this would do is add an additional access layer, consume more space, and be unable to prevent it from being configured on backend devices that it actually cannot function on.

@Laczen
Copy link
Collaborator Author

Laczen commented Sep 6, 2023

One extra remark:
Q: Does the use of the subsystem oblige developers to support all non volatile memory? A: No it does not, the developer can decide based upon the non volatile memory type go exclude a memory from use.

I don't really see the point in this then, so if this became the normal for interacting with devices, let's say nvs or lfs support moves over to it but actually only really supports the flash backend because it relies upon flash writing tricks, all this would do is add an additional access layer, consume more space, and be unable to prevent it from being configured on backend devices that it actually cannot function on.

Neither nvs or lfs rely on any flash writing tricks. nvs relies on the fact that the area is erased before writing to it in order to discover where it has left off. lfs relies on nothing illustrated by the fact that it can use disks as well as flash.

Now consider the situation where also a disk would be covered by nvmp (it is not in the PR at the moment but adding it is very easy). nvs on disk does not really make sense because of how it is working (writing the same sector to add data), the nvs library could exclude any setup that uses disk support (this is also possible at build time). The present littlefs code that supports disks and flash could be simplified a lot by using nvmp. All configuration parameters that are related to the setup on flash or disk could be removed and retrieved from the nvmp layer. The possibility to retrieve the information about the backend at build time also allows to remove any unneeded code.

The proposal does indeed add an extra layer and in some cases it is possible to avoid this layer. However to avoid this it is needed to write extra code for each of the backends that could be used. This results in more maintenance and fewer possibilities.

Copy link
Member

@henrikbrixandersen henrikbrixandersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How could this PR be used:
a. Working with images as a replacement of flash_(map)partitions,
b. Support nvs on many backends with minor changes to nvs,
c. Combine ram_disk and flash_disk in one "internal" disk solution,
d. With a backend created for a disk it would allow to combine littlefs on disk and littlefs on flash/ram/eeprom,
e. With a backend created for a disk it would allow working with images directly from disk,
f. With a backend created for a file on disk it would allow working with images directly from file,

I understand how this proposed subsystem could be used, but I still don't think this is a good idea. Being able to use e.g. NVS "with minor changes" on an EEPROM does not mean that NVS is suitable for use on an EEPROM with just these minor changes. The same goes for file systems.

I think a better approach would be to add dedicated backend (i.e. flash, EEPROM, BBRAM, ...) support to these other subsystems without having to go through an adaption layer as proposed here. This will ensure that proper thought has gone into adapting/designing the given subsystem for use on the various backends instead of just using an adaption layer for shoehorning in the support.

@Laczen
Copy link
Collaborator Author

Laczen commented Sep 7, 2023

@brix, I understand your objection to the proposal. Your proposal of course also has disadvantages:

  1. There would be a lot of code duplication to add support for each of the backends,
  2. The adaptation/design for some of the backends is not going to happen in the near future,

So we would be leaving zephyr users in a situation where they would be perfectly happy with a non-optimal solution but this solution is being deprived from them.

As an example of item 2. I have in the past done quite some effort into providing a direct settings backend solution using an eeprom. The biggest problem in this is guaranteeing that no data is lost when it is needed to "pack" the storage info. It was not possible (without replicating what is done in the flash based backends) to get this correct, and I really tried hard. As a result up to today there is no solution for storing settings directly on an eeprom which has been requested numerous times. I don't see this changing in the near future but the requests will increase given the recent addition of fujitsu mb85rcxx device.

Providing a solution that could simplify development, enable partitioning on non volatile memory in no way forces the developers to use this provided solution. If it is decided not to support a specific kind of storage this should be possible (and it is).

Regarding the "shoehorning" of support: if it would be needed to add eeprom support for nvs it would be using exactly the support method that is proposed: call the eeprom read/write routines and simulate an erase by writing a constant value to it. When enabling littlefs or fatfs on eeprom it would use the block based approach as for disks, no erase would be called but just a block write/read. Both nvs or littlefs/fat would need to keep track of the backend type in order to call the correct routines. All of this added code is provided by the proposal directly without the need to duplicate.

@nordicjm
Copy link
Collaborator

nordicjm commented Sep 7, 2023

By having an "erase" feature for EEPROMs, you are halving the lifespan of that because instead of just doing a program for each location, you are doing an "erase" by writing 0xff to it then writing the real data you want to write to it, that's not really solving the problem of lack of settings on an EEPROM device, that just introduces a new issue of, and I quote from @Laczen's own words on another PR: "a user might result in premature end of flash life"

@Laczen
Copy link
Collaborator Author

Laczen commented Sep 7, 2023

By having an "erase" feature for EEPROMs, you are halving the lifespan of that because instead of just doing a program for each location, you are doing an "erase" by writing 0xff to it then writing the real data you want to write to it, that's not really solving the problem of lack of settings on an EEPROM device, that just introduces a new issue of, and I quote from @Laczen's own words on another PR: "a user might result in premature end of flash life"

As said: if users are happy with a sub-optimal solution why deprive it from them? And with the words of @carlescufi :"support of fatfs on eeprom could be done, ... because eeprom, well never dies.".

@nordicjm
Copy link
Collaborator

nordicjm commented Sep 7, 2023

By having an "erase" feature for EEPROMs, you are halving the lifespan of that because instead of just doing a program for each location, you are doing an "erase" by writing 0xff to it then writing the real data you want to write to it, that's not really solving the problem of lack of settings on an EEPROM device, that just introduces a new issue of, and I quote from @Laczen's own words on another PR: "a user might result in premature end of flash life"

As said: if users are happy with a sub-optimal solution why deprive it from them? And with the words of @carlescufi :"support of fatfs on eeprom could be done, ... because eeprom, well never dies.".

I was just curious as you thought it was a big enough issue to NACK a whole PR previously but now seem to think it's okay.

@Laczen
Copy link
Collaborator Author

Laczen commented Sep 7, 2023

I was just curious as you thought it was a big enough issue to NACK a whole PR previously but now seem to think it's okay.

I have no idea what PR you are referring to, but yes I can change my mind. No decision is ever final for me, it is taken at the specific time and situation.

@Laczen Laczen changed the title mtd: api for mtd devices nvmp: api for non volatile memory Sep 11, 2023
@Laczen Laczen force-pushed the mtd_api branch 9 times, most recently from 164edf9 to 54f3e15 Compare September 16, 2023 06:57
@Laczen Laczen removed the DNM This PR should not be merged (Do Not Merge) label Sep 16, 2023
@Laczen Laczen force-pushed the mtd_api branch 3 times, most recently from 54a42f2 to e6281f7 Compare September 23, 2023 11:47
Provide a unified way of working with non volatile memory and partitions
of non volatile memory. Non volatile memory should be considered broadly:
devices but also e.g. files could be considered as non volatile memory.

Enable support of non volatile memory and partitions api on flash,
eeprom and retained_mem. Disks and files could be added.

The subsystem is modular to allow easy addition of new storage solutions

Signed-off-by: Laczen JMS <laczenjms@gmail.com>
@Laczen
Copy link
Collaborator Author

Laczen commented Sep 29, 2023

I'm stopping my pursuit to try and unify working with different memories as this could be seen alternatives to existing solutions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Architecture Review Discussion in the Architecture WG required area: Devicetree Binding PR modifies or adds a Device Tree binding area: EEPROM area: Flash area: Retained Memory area: Storage Storage subsystem
Projects
Status: Done
5 participants