storage: flash_map: PM in flash_area_open/close

[JordanYates]

Run power management actions in flash_area_open and
flash_area_close. If the flash area is open it should be in the active
state, otherwise it should be left in its lowest power state.

[Laczen]

@JordanYates, is it required for the flash_area_open and ...close to modify the pm state. As all flash area routines are using flash calls anyway to do something isn't the pm of the flash driver sufficient ?

[JordanYates]

@JordanYates, is it required for the flash_area_open and ...close to modify the pm state. As all flash area routines are using flash calls anyway to do something isn't the pm of the flash driver sufficient ?

It is required that the backing device be in the ACTIVE state for any of the other operations to make sense.
It is also required to call flash_area_open before running any of the other operations.

This change lets the flash_map API always work, regardless of the PM state of the backing device. Without this change, every call to flash_area_open should be followed with pm_device_runtime_get, as otherwise there are no guarantees.

Do you have a specific concern about adding this?

[Laczen]

@JordanYates, my concern is that things will be done double (and this is a risk for all subsystems building on top of a driver that has pm): the pm support is already in the driver so any call to the driver will wake it up.

The requirement to be in an ACTIVE state can be postponed to the real driver call. And any call to the driver that allows the device to keep asleep can be done without waking it up.

I don't know if there are other parts of zephyr that are already using a similar pattern.

[JordanYates]

@JordanYates, my concern is that things will be done double (and this is a risk for all subsystems building on top of a driver that has pm): the pm support is already in the driver so any call to the driver will wake it up.

Multiple calls to pm_device_runtime_get/put are expected and designed around, multiple users are a core part of the API behaviour. SPI_NOR_IDLE_IN_DPD may be fine for simple use cases, but it is unsuitable for more complex setups (Flash device on a power domain for example).

The requirement to be in an ACTIVE state can be postponed to the real driver call. And any call to the driver that allows the device to keep asleep can be done without waking it up.

The ACTIVE PM state cannot be postponed to another driver call, that is not how the PM API is designed. pm_device_runtime_get/close should be called when you are interested in using a device, which IMO maps very cleanly onto the concept of opening and closing the flash area.

[Laczen]

The ACTIVE PM state cannot be postponed to another driver call, that is not how the PM API is designed. pm_device_runtime_get/close should be called when you are interested in using a device, which IMO maps very cleanly onto the concept of opening and closing the flash area.

My concern might have resulted from a incomplete understanding from how the PM API is to be used. I agree that the mapping is clean (I also don't see any objection to the PR). I am trying to understand the "full" implications: a direct flash api call does not call a pm_device_runtime_get() before calling the flash api, but a subsystem using this api does a call to pm_device_runtime_get(): this seems counter intuitive.

Suppose the change isn't made, would it no longer operate or operate in a limited fashion ? We shouldn't just add the pm calls because we can, but because we need.

This of course raises the question, why is there a flash_area_open/close: the open is to set up the limits, the close does nothing. What should they do ? IMO the intention for the open/close is to allow the flash backend to modify the allowed read/write erase limits and to set them back when done working with the area.

[JordanYates]

Suppose the change isn't made, would it no longer operate or operate in a limited fashion ? We shouldn't just add the pm calls because we can, but because we need.

Suppose a scenario where SPI_NOR_IDLE_IN_DPD isn't used because the user thinks cycling to DPD on every call to flash_read or flash_write is silly. Instead they choose to use device runtime PM on the flash chip, so they can manually decide when an "operation" starts and stops. Now introduce MCUmgr, where the secondary slot is also on that external flash.

MCUmgr (and other libraries) are written using the flash map API. They are relatively high level, in that they shouldn't really be caring about the power states of flash chips unless there is no alternative. As long as flash_area_open succeeds, they should be good to go. Without this change, all operations on that external flash chip will currently fail, as the flash remains in DPD.

And from the users perspective, if a library is holding the device in ACTIVE state for too long, then that is a signal that flash_area_open/close in that library could potentially be reduced in scope (For example NVS currently opens the area once on init and never closes it).

This of course raises the question, why is there a flash_area_open/close: the open is to set up the limits, the close does nothing. What should they do ? IMO the intention for the open/close is to allow the flash backend to modify the allowed read/write erase limits and to set them back when done working with the area.

flash_area_open should be doing ANY work required to make the flash area usable. flash_area_close should be doing the inverse. If that includes PM control, MMU setup, whatever, it should be doing that work IMO.

[Laczen]

I am getting really scared about pm now:

1. It seems that there is a possibility to introduce a system failure by improper use of the pm system (overriding the automatic working of pm),
2. Every system (like nvs, but also littlefs, flashdisk, ...) that is using the flash area API will need to consider the pm to allow power saving when the system is not currently used. There is no automatic power saving by not accessing the area for a longer time.

[JordanYates]

I am getting really scared about pm now:

CONFIG_DEVICE_PM=n is always an option

1. It seems that there is a possibility to introduce a system failure by improper use of the pm system (overriding the automatic working of pm),

This is embedded, every line of code has the possibility to introduce a system failure. The question is why are you overriding the automatic behaviour of PM? I have used the runtime PM subsystem extensively across multiple applications/boards/years and have never had a problem that wasn't readily apparent within a short duration after boot.

2. Every system (like nvs, but also littlefs, flashdisk, ...) that is using the flash area API will need to consider the pm to allow power saving when the system is not currently used. There is no automatic power saving by not accessing the area for a longer time.

Sure, but by rolling the PM behaviour into the flash map implementation, the only effect on those users is asking them to limit the duration they open an area for. And that is no different from any other API. You don't leave a socket open indefinitely if you're not using it, you don't claim an SPI bus forever. Say when you need it, say when you're done.

[Laczen]

Sure, but by rolling the PM behaviour into the flash map implementation, the only effect on those users is asking them to limit the duration they open an area for. And that is no different from any other API. You don't leave a socket open indefinitely if you're not using it, you don't claim an SPI bus forever. Say when you need it, say when you're done.

flash map users are saying when they need the device, when there they make a call to flash_area_read(), flash_area_write(), flash_area_erase(). flash_area_open should only have the meaning "make this area available to me", it should not mean "make this area available to me and keep it alive". The calls to pm routines should be inside the flash_area_read(), flash_area_write(), flash_area_erase(), to avoid pushing it down to the subsystems. But this would be rather strange (double work) as the first thing the device does is acquire it for these operations.

So maybe it is just the naming/meaning of the routine ("open") that creates the misunderstanding.

[de-nordic]

No.
Subsystems may hold flash area indefinitely open and never release it, for example application images and subsystems like settings or file systems.
That will be major headache for low power applications that try to sleep most of a time but keep flash area access enabled.
The pm_device_runtime_get should be only attempted when the device will really be used. So when operation is attempted.

[JordanYates]

No. Subsystems may hold flash area indefinitely open and never release it, for example application images and subsystems like settings or file systems. That will be major headache for low power applications that try to sleep most of a time but keep flash area access enabled. The pm_device_runtime_get should be only attempted when the device will really be used. So when operation is attempted.

So your preference is for an implementation more like this? 34322fd

[de-nordic]

No. Subsystems may hold flash area indefinitely open and never release it, for example application images and subsystems like settings or file systems. That will be major headache for low power applications that try to sleep most of a time but keep flash area access enabled. The pm_device_runtime_get should be only attempted when the device will really be used. So when operation is attempted.

So your preference is for an implementation more like this? 34322fd

IMHO only functions that directly use a device should be tasked with waking it up, with some exception I will mention in a moment. There is no reason to manipulate power level of a device by function that just passes pointer to another function because that other function may actually do nothing with the dev and/or just pass it to another function and so on, and finally not action o a device may happen. So basically no flash_area_ API call functions (except for below case) should set pm state to device, that should be done by flash_ API calls as they really working with the device.

There is case for aggressive power management, and I was even considering flash_area_ioctl type function and identifiers to aid it. I do not remember details now, because it was discussed long time ago, but somebody wanted to be able to power up device for entire length of operation line file upload or app image upload, so for example when MCUmgr starts upload you set higher pm level on a device till the final flash_write happens.

[JordanYates]

So basically no flash_area_ API call functions (except for below case) should set pm state to device, that should be done by flash_ API calls as they really working with the device.

I can appreciate the reasoning, but that is not how any of the device PM API is designed to work AFAIK. The expected flow is:

1. Manually ensure the device is active (via pm_device_runtime_get or pm_device_state_set)
2. Run any and all operations you need to run on the device through the API it implements
3. Manually let the device transition to suspend (via pm_device_runtime_put or pm_device_state_set)

Are you suggesting that PM should be done in flash_readetc implementations in individual drivers, in the thin API wrappers in zephyr/drivers/flash.h, or something else?

There is case for aggressive power management, and I was even considering flash_area_ioctl type function and identifiers to aid it.

I'm not sure of the practical difference between doing the PM calls in open/close vs a dedicated flash_area_ioctl function, beyond the later requiring a large amount of existing code additions to handle PM properly. At least rolling it into the existing functions will make it work out of the box, if perhaps sub optimally (Which can be incrementally improved in subsequent PRs).

but somebody wanted to be able to power up device for entire length of operation line file upload or app image upload, so for example when MCUmgr starts upload you set higher pm level on a device till the final flash_write happens.

That is exactly the type of behaviour that I am trying to enable. An optional step beyond that is "A higher PM level until X seconds after the last call", given that with remote procedures you don't necessarily know which command is the last one, and in many cases multiple flash accesses are tightly grouped in time.

[de-nordic]

So basically no flash_area_ API call functions (except for below case) should set pm state to device, that should be done by flash_ API calls as they really working with the device.

I can appreciate the reasoning, but that is not how any of the device PM API is designed to work AFAIK. The expected flow is:

1. Manually ensure the device is active (via `pm_device_runtime_get` or `pm_device_state_set`)

2. Run any and all operations you need to run on the device through the API it implements

3. Manually let the device transition to suspend (via `pm_device_runtime_put` or `pm_device_state_set`)

Are you suggesting that PM should be done in flash_readetc implementations in individual drivers, in the thin API wrappers in zephyr/drivers/flash.h, or something else?

As far as I understand the https://docs.zephyrproject.org/latest/services/pm/device_runtime.html, the driver is responsible for notifying PM system on device use.

There is case for aggressive power management, and I was even considering flash_area_ioctl type function and identifiers to aid it.

I'm not sure of the practical difference between doing the PM calls in open/close vs a dedicated flash_area_ioctl function, beyond the later requiring a large amount of existing code additions to handle PM properly. At least rolling it into the existing functions will make it work out of the box, if perhaps sub optimally (Which can be incrementally improved in subsequent PRs).

but somebody wanted to be able to power up device for entire length of operation line file upload or app image upload, so for example when MCUmgr starts upload you set higher pm level on a device till the final flash_write happens.

flash_area_open may be used by file system in the moment the file system is mounted and flash_area_close when it is unmounted, which will happen when device (MCU) is powered down; this means elevated power state remains on everything between flash device and flash area API (SPI bus, etc).
open operation may give you exclusive or non-exclusive access to a resource that may be a single- or a multi-instance; it only gives you information whether system allows you access to a resource or not, and possibly creates/allocates or initializes system objects that serve that resource; you may hold a resource indefinitely, with no requirement for any operations, on your side, to follow with assurance that there is a control object, assigned to you, dedicated to the resource you have requested.

That is exactly the type of behaviour that I am trying to enable. An optional step beyond that is "A higher PM level until X seconds after the last call", given that with remote procedures you don't necessarily know which command is the last one, and in many cases multiple flash accesses are tightly grouped in time.

Consider having a device that is woken up once an hour to read several sensors hanging from SPI or other buses. All the readings will happen in q sequence, after which device suspends again. It may be more efficient to elevate the PM level on the path to all devices prior to doing all readings to keep the path active through entire read sequence.

Using open/close operations for waking/suspending device works as long as this happens every time you try to access device, but what if you have an API that reserves the device and allows access to it only through itself, it may never release device via close, but it may use read/write operations from time to time, due to API calls.

In my ideal world, device operation like read works as a software construct till the actual device is needed, which means that no device, like bus, should be powered up till the final device is required to do any work, in which case the final device requests everything on path to itself to be awake, to complete requested task.

[JordanYates]

As far as I understand the https://docs.zephyrproject.org/latest/services/pm/device_runtime.html, the driver is responsible for notifying PM system on device use.

I will admit that I haven't previously read through that page closely. While there is a lot of talk about devices requesting themselves, I would like to note two things:

First is that the only concrete example given, "For example, if a bus device is used by multiple sensors, we can keep the bus active until the last sensor has finished using it." is talking about running PM on another device that the driver depends on.

Second is that no driver that I have seen in-tree is written to run PM operations on itself. And that sort of makes sense, because if that was the case what is the point of having the API in the first place? Just do the work you know you need to do at the start and end of every function.

@gmarull wrote the bulk of the current doc version, I would be interested in his opinion.

In my ideal world, device operation like read works as a software construct till the actual device is needed, which means that no device, like bus, should be powered up till the final device is required to do any work, in which case the final device requests everything on path to itself to be awake, to complete requested task.

While this may be fine for internal resources like SPI buses (even if they're not written that way currently) it sort of falls apart IMO as soon as you start considering external devices. Consider a (poorly thought out) power domain that has a GPS modem (slow to initialize, high initial power usage) and a flash chip on it, and we want to run 4 calls to flash_read for example. Are we expecting to cycle the power domain on each call to flash_read? The operation will take 4x longer than it should to complete, drawing huge power the whole while.

My ideal world is the inverse of yours from what I can tell. In my opinion, higher layers always (ignoring resources automatically controlled by hardware) have a better idea of when a device should be enabled/disabled. A SPI driver is by definition incapable of knowing how higher levels are planning to use it, and unaware of the larger hardware design at play. The same holds for a flash driver, it has 0 idea whether someone is planning to read the status register or write 10MB of data, and hence whether it should be turning itself off now or not. The buck should be passed up the stack until you have a software user that has some idea about the bigger usage picture.

I accept that the result will be slightly less efficient power efficient in trivial cases (single flash chip, always powered, single SPI user), but it is actually usable in non-trivial cases with multiple power domains. And when designing for lowest power consumption, the cheapest part to run is the one that has no power applied.

[de-nordic]

First is that the only concrete example given, "For example, if a bus device is used by multiple sensors, we can keep the bus active until the last sensor has finished using it." is talking about running PM on another device that the driver depends on.

OK, have another example: you have a file system that opens device at mount and keeps it opened till umount; file system does read/writes in bulks, calling op in a loop. The file system may, before entering loop, elevate PM level of device it is going to do ops on, and lower it when loop exits; PM functions in ops like read/write, for a device, will just increase/decrease counter without actually affecting the PM operation - as the file system driver itself has +1 on it anyway.

Second is that no driver that I have seen in-tree is written to run PM operations on itself. And that sort of makes sense, because if that was the case what is the point of having the API in the first place? Just do the work you know you need to do at the start and end of every function.

For the drivers to call the API, it is for that.

While this may be fine for internal resources like SPI buses (even if they're not written that way currently) it sort of falls apart IMO as soon as you start considering external devices. Consider a (poorly thought out) power domain that has a GPS modem (slow to initialize, high initial power usage) and a flash chip on it, and we want to run 4 calls to flash_read for example. Are we expecting to cycle the power domain on each call to flash_read? The operation will take 4x longer than it should to complete, drawing huge power the whole while.

If these are completely separate flash_read ops, that are not executed under other subsys that executes in in sequence, then yes; and that will happen right now, right? Because when you bypass Flash Map API and go directly for the flash, this is what you are going to get: a need to either control PM by yourself or flash_read to do that or both.

My ideal world is the inverse of yours from what I can tell. In my opinion, higher layers always (ignoring resources automatically controlled by hardware) have a better idea of when a device should be enabled/disabled.

Exactly, that is why it should not happen when you gain access to an object representing a device, or reserve it, but rather when you start to use a device. That is why an API call that allow you to notify lower API call too keep device powered, and open/close is nor a right for that.

A SPI driver is by definition incapable of knowing how higher levels are planning to use it, and unaware of the larger hardware design at play.

SPI driver is used by lower levels: when you ask flash API to read something the flash API takes care of finding the right path to device, whether it is SoC flash or SPI flash, does not matter; you do not know the path and you do not care: you access device by the final node, wherever it is.

The same holds for a flash driver, it has 0 idea whether someone is planning to read the status register or write 10MB of data, and hence whether it should be turning itself off now or not. The buck should be passed up the stack until you have a software user that has some idea about the bigger usage picture.

That is correct, that is why the flash driver should attempt to control PM level in operations and there should be API to allow to elevate it for unspecified sequence of ops happening in organized manner from single source of upper level. In such case, when the upper level increases device PM level, the calls within device driver will just increase/decrease counters without affecting PM much more; but if you have some random unorganized calls, they still will be covered by PM, without need to directly engage with Power Management.

Flash Maps open/close and all other non-pm dedicated ops should not affect PM; there should be specific calls to do that.
User may completely bypass Flash Map API and use FIXED_PARTITION macros directly with Flash API, in which case PM would be completely dropped on user even for a single, random instances of flash operation, that could be covered by Flash API.

I accept that the result will be slightly less efficient power efficient in trivial cases (single flash chip, always powered, single SPI user), but it is actually usable in non-trivial cases with multiple power domains. And when designing for lowest power consumption, the cheapest part to run is the one that has no power applied.

The problem with "result will be slightly less efficient power efficient in trivial cases" is that these might be the cases that users care a lot, because these may be devices with coin batteries or other limited power sources and locking something into higher power state because it makes other things easier may not be acceptable solution.

We will not have efficient power management, in Zephyr, without relying on developers knowledge of device and how user/subsystem uses the device, and user needs to be given a way to control the PM of the device. We can not at the same time cover, without user involvement, without subsystem involvement, and additional PM calls here and there, devices that will be powered by cr2032 and these powered by 1800mAh Li-On, devices that have rough startup and then very small current drawn afterwards and these that have some moderate current drawn through entire time; we can not easily cover operations that happen in bulk and these that happen randomly once a while.

[Laczen]

I am completely inline with the position of @de-nordic: it should be the device (driver) that takes control of the pm. A subsystem that uses a driver could decide to take over the pm by doing a pm_device_get() if it knows it will need many accesses (it is in that case also responsible for the release). A subsystem should be able to rely on the driver pm to do the correct actions without it needing to do any pm actions. Isn't the spi_nor driver an illustration of this ?

To allow the subsystem to take over the pm management some routines would need to be defined e.g. flash_area_pm_runtime_get() and flash_area_pm_runtime_put(), but their use (and availability) should be in no way obliged.

[JordanYates]

A subsystem should be able to rely on the driver pm to do the correct actions without it needing to do any pm actions. Isn't the spi_nor driver an illustration of this ?

The spi_nor driver (until the introduction of #59647 a month ago) was an illustration of a driver that managed PM operations itself (via SPI_NOR_IDLE_IN_DPD), by completely ignoring the PM API, at the cost of being unusable in anything other than trivial setups. In my opinion it is a perfect example of how it should not be done.

[JordanYates]

Taking a step back from the meta discussion around PM.

The file system may, before entering loop, elevate PM level of device it is going to do ops on, and lower it when loop exits;

Great, we both agree that a feature that elevates the PM for a number of sequential operations is desirable, and that is what this PR is attempting to introduce. I see three potential mechanisms for achieving this (with my thoughts in line).

1. flash_area_open/close can automatically call pm_device_runtime_get/put (This PR)

Ensures that "out-of-the-box", any operations using the flash area API will be operating on a device in a valid power state.
Suboptimal usage (open at boot, never close) can be slowly migrated through independent PR's.
Flash area parameters (size, offset, backend device) can only be queried by physically powering up the device, which is not technically required.

2. A new flash_area_ioctl function can be introduced that has operations for pm_device_runtime_get/put

Explicit operations that clearly define what they are doing and operates on the native flash area handle.
Enables existing usage (open at boot, never close) to remain unchanged.
Does not "fix" any API users by default, every user must be updated to call the new functions as required.
Several users (mcumgr for example) are already calling flash_area_open/close around each operation.

3. Users of flash area can manually call pm_device_runtime_get/put(fa->fa_dev);

Workable, but not ideal. The "handle" for the entire flash area API is the flash area itself. Users of flash area currently need no knowledge about devices, and the fact that the backend is a device is an implementation detail that only happens to leak out through a struct member. Has the same downsides as number 2.

[Laczen]

@JordanYates my first reaction is that option 2 is the way to go:

1. It maximizes power save without any changes to existing flash_area users,
2. It allows for maximum performance if required,
3. It's existence is optional,

This could be done by either introducing a generic iotcl method or specific routines for doing pm control (flash_area_pm_get and flash_area_pm_put), this is to be decided.
BUT To properly operate this needs a pm state in the flash area structure.

Option 1 is not good because it will disable any power saving when an area is to be used as done by littlefs or nvs. Even when the open/close is done for every flash_area_write, ..._read or ..._erase it results in (unneeded) code to setup the flash area structure and just results in an extra semaphore to be acquired and released. Calling an early close (could be done by nvs) is also not ideal as it could disable any allow to write/read by flash_area_close().

Option 3 should be avoided.

I see another possibility but I don't know if pm can handle such a scenario: We take option 1, but we add the posibility to "release" the pm from flash_area to the device. Could this be done without creating a state in flash_area that the pm has been "released" ?

[gmarull]

As far as I understand the https://docs.zephyrproject.org/latest/services/pm/device_runtime.html, the driver is responsible for notifying PM system on device use.

I will admit that I haven't previously read through that page closely. While there is a lot of talk about devices requesting themselves, I would like to note two things:

First is that the only concrete example given, "For example, if a bus device is used by multiple sensors, we can keep the bus active until the last sensor has finished using it." is talking about running PM on another device that the driver depends on.

Second is that no driver that I have seen in-tree is written to run PM operations on itself. And that sort of makes sense, because if that was the case what is the point of having the API in the first place? Just do the work you know you need to do at the start and end of every function.

@gmarull wrote the bulk of the current doc version, I would be interested in his opinion.

With device runtime PM it is expected that drivers manage themselves (as in Linux). However, it can be used externally if an upper layer wants the device to stay awake between successive calls (to avoid unnecessary sleeps/wakeups during a short sequence). Note that if we had the autosuspend feature (ie, only suspend after a certain inactivity period, say time since last get() call), the last point would probably be mitigated for most cases.

[de-nordic]

@JordanYates my first reaction is that option 2 is the way to go:

1. It maximizes power save without any changes to existing flash_area users,

2. It allows for maximum performance if required,

3. It's existence is optional,

This could be done by either introducing a generic iotcl method or specific routines for doing pm control (flash_area_pm_get and flash_area_pm_put), this is to be decided. BUT To properly operate this needs a pm state in the flash area structure.

No it does not. The `Flash Map API is just passing calls to flash driver, the driver does hold PM state by itself, flash area has no interest in current state as it is not going to manipulate it in any of API calls.
All the required information, for the PM management, is already there - this is the device pointer.

Any open/close functions should be only used to obtain access to device in form of system provided object, they should not be used for power management as obtaining object does not mean it will be used, it may just be reserved for potential use.

[JordanYates]

@de-nordic can you comment on which of the three proposed approaches you find least objectionable?

[de-nordic]

@de-nordic can you comment on which of the three proposed approaches you find least objectionable?

I am not that grumpy, I do not find them all objectionable: at least one is OK, I mean the second one with special API calls and open/close untouched.

[JordanYates]

I am not that grumpy, I do not find them all objectionable: at least one is OK, I mean the second one with special API calls and open/close untouched.

Sorry I wasn't implying you're grumpy, I was implying that all 3 have their own drawbacks

[de-nordic]

I am not that grumpy, I do not find them all objectionable: at least one is OK, I mean the second one with special API calls and open/close untouched.

Sorry I wasn't implying you're grumpy, I was implying that all 3 have their own drawbacks

Over 20 years of experience with internet and I still have not figured out there is no voice modulation in a typed text that would suggest that I am trying to be funny-sarcastic.

[de-nordic]

DNM till changes applied.

no longer reviewing PM related code

[ceolin]

Adding two cents regarding pm subsystem. I completely agree with @gmarull, it is up to devices to manage themselves, from user perspective is expected that when I need to perform an operation in the device it is active and when it is not being used it is saving energy. Obviously the device itself has limited knowledge of how it will be used, so the upper layer can optimize this getting/releasing the device.

I think the general (obviously there are exceptions) assumption should be, if the device is not doing a hw operation at the moment it is suspended. This kind of transfer part of the responsibility to the upper layer (avoid unnecessary suspend/resume), but that will always be the case, since as higher you are more knowledge of the application you have.