Flash and nvs improvements #69588
Flash and nvs improvements #69588
Conversation
mjchen0
force-pushed
the
flash_nvs
branch
2 times, most recently
from
1c23cb2 to
23b5123
Compare
There was a problem hiding this comment.
This should be using https://docs.zephyrproject.org/latest/services/pm/device_runtime.html#introduction
There was a problem hiding this comment.
Split to two PRs, the changes are unrelated.
I do not think there is a need for callbacks nor internal counter, PM already handle that.
There are pm_device_runtime_get and pm_device_runtime_put functions use usage counter provided by the PM. What we probably lack here is the PM callback processing.
|
Okay, I've split into actually three PRs (third is the removal of the unneeded enter_dpd() call at end of spi_nor_configure() in spi_nor.c). Using runtime pm is cleaner, but a little awkward in that anyone previously using CONFIG_SPI_NOR_IDLE_IN_DPD to save power will actually want to disable that now in favor of using runtime pm, and make sure to enable it in their device tree. |
If the flash device supports and enables pm runtime, this will resume/suspend it as needed for nvs operations. Note that with this change, if nvs is used with the spi_nor flash driver, it's better to not enable CONFIG_SPI_NOR_IDLE_IN_DPD, otherwise the runtime PM calls do nothing. Also, spi-nor driver doesn't enable runtime pm so add the devicetree property zephyr,pm-device-runtime-auto to have enable pm device runtime for the spi-nor device automatically. Signed-off-by: Mike J. Chen <[email protected]>
|
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
I make the requested change. Please review again. |
|
Please have the stale label removed. |
|
@mjchen0 my knowledge on the PM subsystem is insufficient to confirm that the correct method is used (from the intro in PM I would think using |
|
I believe myself and @ceolin have come to an agreement that the SPI NOR driver should be automatically performing the PM operations in The async put would mean multiple requests closely grouped in time would not result in multiple PM transitions. This would resolve not only the issue here, but a collection of other flash users. I just haven't had a board using the |
|
@mjchen0 a gentle nudge... |
I just took a look and yes, I think it should resolve the issue as well, though there are differences. In my PR, if NVS is the only user of the spi_nor FLASH, the FLASH it is put to sleep as soon as possible after NVS is done. There is no additional latency for the async wait timeout and there is no extra code execution for scheduling and cancelling the work when a burst of FLASH activity occurs, etc. So I think my PR is complementary and has advantages. |
I understand that the PR has advantages but I think it is too pm specific to integrate into NVS. What do you think about adding a routine to nvs: |
I agree that this PR is more optimized for this particular use case (reading or writing from one NVS key), however I am not convinced that the performance improvement is significant. If NVS is the only user of the flash, then the delay can be set to 1ms and achieve the same functionality as this PR with minimal overhead (1ms of idle vs DPD on the flash, 1 additional SoC wakeup). I am curious how often you are reading/writing to the NVS, that these few milliseconds are a problem. |
Remove `SPI_NOR_IDLE_IN_DPD` to simplify the possible transition states in the `spi_nor` driver. This option was originally added 5 years ago when device runtime PM was in a much less mature state. I do not believe having a separate power management implementation for this one in-tree driver is in the interests of the project. The behaviour of `SPI_NOR_IDLE_IN_DPD` leads to extremly suboptimal behaviour in use cases where multiple small reads are performed sequentially, see zephyrproject-rtos#69588. Removal of this option does not break the behaviour of any existing applications, only increases current consumption in idle by ~10uA until device runtime PM is enabled. Signed-off-by: Jordan Yates <[email protected]>
Remove `SPI_NOR_IDLE_IN_DPD` to simplify the possible transition states in the `spi_nor` driver. This option was originally added 5 years ago when device runtime PM was in a much less mature state. I do not believe having a separate power management implementation for this one in-tree driver is in the interests of the project. The behaviour of `SPI_NOR_IDLE_IN_DPD` leads to extremly suboptimal behaviour in use cases where multiple small reads are performed sequentially, see zephyrproject-rtos#69588. Removal of this option does not break the behaviour of any existing applications, only increases current consumption in idle by ~10uA until device runtime PM is enabled. Signed-off-by: Jordan Yates <[email protected]>
Remove `SPI_NOR_IDLE_IN_DPD` to simplify the possible transition states in the `spi_nor` driver. This option was originally added 5 years ago when device runtime PM was in a much less mature state. I do not believe having a separate power management implementation for this one in-tree driver is in the interests of the project. The behaviour of `SPI_NOR_IDLE_IN_DPD` leads to extremly suboptimal behaviour in use cases where multiple small reads are performed sequentially, see zephyrproject-rtos#69588. Removal of this option does not break the behaviour of any existing applications, only increases current consumption in idle by ~10uA until device runtime PM is enabled. Signed-off-by: Jordan Yates <[email protected]>
Remove `SPI_NOR_IDLE_IN_DPD` to simplify the possible transition states in the `spi_nor` driver. This option was originally added 5 years ago when device runtime PM was in a much less mature state. I do not believe having a separate power management implementation for this one in-tree driver is in the interests of the project. The behaviour of `SPI_NOR_IDLE_IN_DPD` leads to extremly suboptimal behaviour in use cases where multiple small reads are performed sequentially, see #69588. Removal of this option does not break the behaviour of any existing applications, only increases current consumption in idle by ~10uA until device runtime PM is enabled. Signed-off-by: Jordan Yates <[email protected]>
|
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
Remove `SPI_NOR_IDLE_IN_DPD` to simplify the possible transition states in the `spi_nor` driver. This option was originally added 5 years ago when device runtime PM was in a much less mature state. I do not believe having a separate power management implementation for this one in-tree driver is in the interests of the project. The behaviour of `SPI_NOR_IDLE_IN_DPD` leads to extremly suboptimal behaviour in use cases where multiple small reads are performed sequentially, see zephyrproject-rtos#69588. Removal of this option does not break the behaviour of any existing applications, only increases current consumption in idle by ~10uA until device runtime PM is enabled. (cherry picked from commit 6487a2a) Original-Signed-off-by: Jordan Yates <[email protected]> GitOrigin-RevId: 6487a2a Cr-Build-Id: 8738366835818535617 Cr-Build-Url: https://cr-buildbucket.appspot.com/build/8738366835818535617 Copybot-Job-Name: zephyr-main-copybot-downstream Change-Id: Ib35665da136ef7ba33e2671f7b0c16c8f9224b96 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/zephyr/+/5822382 Commit-Queue: Ting Shen <[email protected]> Reviewed-by: Ting Shen <[email protected]> Tested-by: ChromeOS Prod (Robot) <[email protected]> Tested-by: Ting Shen <[email protected]>
Remove `SPI_NOR_IDLE_IN_DPD` to simplify the possible transition states in the `spi_nor` driver. This option was originally added 5 years ago when device runtime PM was in a much less mature state. I do not believe having a separate power management implementation for this one in-tree driver is in the interests of the project. The behaviour of `SPI_NOR_IDLE_IN_DPD` leads to extremly suboptimal behaviour in use cases where multiple small reads are performed sequentially, see zephyrproject-rtos#69588. Removal of this option does not break the behaviour of any existing applications, only increases current consumption in idle by ~10uA until device runtime PM is enabled. Signed-off-by: Jordan Yates <[email protected]> flash: spi_nor: automatically run device runtime PM Automatically handle device runtime PM for all flash API calls. Asynchronously release the device after a small delay to minimise power state transitions under multiple sequential API calls (e.g. NVS). Signed-off-by: Jordan Yates <[email protected]> flash: spi_nor: handle SPI bus power Automatically request and release the SPI bus before talking to the flash chip. Signed-off-by: Jordan Yates <[email protected]>
NVS is very slow when CONFIG_SPI_NOR_IDLE_IN_DPD is defined because every flash operation involves entering and exiting the FLASH part's deep power down mode. In addition to the SPI cmd involved, there is typically a non-trivial delay required after issuing the exit from DPD cmd. These delays of every FLASH SPI operation can dramatically slow down NVS (in my tests, mounting a small NVS of 256 sectors too over an order of magnitude longer when DPD was enabled).
The main CL in this pull request introduces a new FLASH driver API that involves two new functions called acquire and release. These are implemented for the spi nor driver to call it's already existing acquire and release functions that do the DPD management, with some modification to reference count the acquires/release calls because they can now be nested.
NVS uses the new APIs if they exist, allowing multiple FLASH operations to be called while the FLASH part is acquired and kept out of DPD, avoiding the extra delay involved in each FLASH operation, but still putting the FLASH part to deep power down once we're idle.
The second CL is a smaller one that improves a comparison function, making it faster and reducing it's stack usage.