Skip to content

Conversation

@JordanYates
Copy link
Contributor

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).

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.

@Laczen
Copy link
Contributor

Laczen commented May 24, 2024

@JordanYates, that is a very nice solution.

@de-nordic
Copy link
Contributor

@krbvroc1 @lxbrz you have been profiling your designs for power usage recently and are fresh on experience.

@JordanYates JordanYates force-pushed the 240524_spi_nor_remove_idpd branch 2 times, most recently from c9d17d7 to aa4e140 Compare May 24, 2024 12:42
@zephyrbot zephyrbot added the platform: STM32 ST Micro STM32 label May 24, 2024
@JordanYates
Copy link
Contributor Author

@krish2718 I had to revert #72519 as the flash driver fails to compile on main due to a missing interface driver (nordic,nrf-exmif), unless I am missing something obvious:

west build -p -b nrf54h20dk/nrf54h20/cpuapp samples/drivers/spi_flash -T sample.drivers.spi.flash
...
zephyr/drivers/flash/libdrivers__flash.a(spi_nor.c.obj):(.rodata.spi_nor_0_config+0x0): undefined reference to `__device_dts_ord_162'

@JordanYates JordanYates force-pushed the 240524_spi_nor_remove_idpd branch from aa4e140 to 59e9a3d Compare May 24, 2024 12:47
@krish2718
Copy link
Contributor

@krish2718 I had to revert #72519 as the flash driver fails to compile on main due to a missing interface driver (nordic,nrf-exmif), unless I am missing something obvious:

west build -p -b nrf54h20dk/nrf54h20/cpuapp samples/drivers/spi_flash -T sample.drivers.spi.flash
...
zephyr/drivers/flash/libdrivers__flash.a(spi_nor.c.obj):(.rodata.spi_nor_0_config+0x0): undefined reference to `__device_dts_ord_162'

This means that the driver which defines the device isn't included in the build, let me check and fix this.

@krish2718
Copy link
Contributor

west build -p -b nrf54h20dk/nrf54h20/cpuapp samples/drivers/spi_flash -T sample.drivers.spi.flash

Please go ahead with the revert, this needs more changes from downstream, I had tested this downstream and submitted the PR, haven't verified upstream, thanks.

@krish2718
Copy link
Contributor

krish2718 commented May 24, 2024

you also need to revert b1de9a6 same issue as earlier sample, exmif peripheral cannot be used in Zephyr now till NCS downstream changes are merged. Do you want to do it in this PR itself? or I can raise a new PR with both reverts, but then you would have to wait till it gets merged and then rebase.

@krbvroc1
Copy link

krbvroc1 commented May 24, 2024

@krbvroc1 you have been profiling your designs for power usage recently and are fresh on experience.

In my case, I found

  • The IDLE_IN_DPD was causing the back to back DPD commands. They are sent too fast (14us) and thus violates the tDPDD specification (30us) for the part. That caused the chip to never enter DPD mode.
  • I found the SPI bus was not disabled which does not tri-state the pins and caused another 10uA or so.

At this point I settled on setting IDLE_IN_DPD=n and in my application I created the following routing which I current call before and after various calls to write the flash.

void dfu_task_enable_flash_spi_bus(bool enable)
{
  const struct device *spi_dev = DEVICE_DT_GET(DT_NODELABEL(spi2));
  const struct device *flash_dev = DEVICE_DT_GET(DT_NODELABEL(mx25r16));

  if (enable)
  {
    pm_device_action_run(spi_dev, PM_DEVICE_ACTION_RESUME);
    pm_device_action_run(flash_dev, PM_DEVICE_ACTION_RESUME);
  }
  else
  {
    pm_device_action_run(flash_dev, PM_DEVICE_ACTION_SUSPEND);
    pm_device_action_run(spi_dev, PM_DEVICE_ACTION_SUSPEND);
  }
}

Would using the PM_DEVICE_RUNTIME take care of the SPI bus too? I am thinking not.

How would I continue to enable / disable the SPI bus, which is a significant power consumer, when PM_DEVICE_RUNTIME feature is used? I am having trouble finding it in the documentation now, but I thought I read somewhere not to use PM_DEVICE_RUNTIME while trying to manually change things. Especially if you are introducing a CONFIG_SPI_NOR_DEEP_POWER_DOWN_INACTIVITY_TIMEOUT_MS.

How can you opt in/out devices from PM_DEVICE_RUNTIME. Doesn't this imply all devices in the system support it but maybe I do not want it enabled on all devices.

@JordanYates JordanYates force-pushed the 240524_spi_nor_remove_idpd branch from 59e9a3d to 16626fd Compare May 25, 2024 11:01
@JordanYates
Copy link
Contributor Author

Would using the PM_DEVICE_RUNTIME take care of the SPI bus too? I am thinking not.

It does not automatically, but that has been added to this PR.

but I thought I read somewhere not to use PM_DEVICE_RUNTIME while trying to manually change things.

Correct, we recommend to never use pm_device_action_run in application code as it will inevitably break as soon as there are multiple users (a second device on the SPI bus in your example).

How can you opt in/out devices from PM_DEVICE_RUNTIME. Doesn't this imply all devices in the system support it but maybe I do not want it enabled on all devices.

Device runtime PM has no effect unless it is explicitly enabled for a device. There is no implication about all devices in the application.

@JordanYates
Copy link
Contributor Author

you also need to revert b1de9a6 same issue as earlier sample, exmif peripheral cannot be used in Zephyr now till NCS downstream changes are merged. Do you want to do it in this PR itself? or I can raise a new PR with both reverts, but then you would have to wait till it gets merged and then rebase.

Happy for you to do a more complete revert in an alternate PR, I don't expect this one will move quickly.

Copy link
Contributor

Choose a reason for hiding this comment

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

If DPD_INACTIVITY_TIMEOUT_MS is 0, should this call pm_device_runtime_put() instead of put_async()? Otherwise it puts the work on the system workqueue without delay, which seems like needless extra work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This sort of optimization is better suited to occur in the PM implementation so that all users get the benefits and code isn't copy pasted everywhere.

@carlescufi carlescufi requested a review from anangl June 20, 2024 11:57
@carlescufi
Copy link
Member

@anangl PTAL

@JordanYates JordanYates force-pushed the 240524_spi_nor_remove_idpd branch from 16626fd to 7f9c52a Compare July 15, 2024 01:44
anangl
anangl previously approved these changes Jul 15, 2024
@de-nordic de-nordic removed the platform: STM32 ST Micro STM32 label Jul 16, 2024
de-nordic
de-nordic previously approved these changes Jul 16, 2024
@de-nordic de-nordic added this to the v4.0.0 milestone Jul 16, 2024
@JordanYates JordanYates dismissed stale reviews from de-nordic and anangl via c0011c2 July 29, 2024 09:37
@JordanYates JordanYates force-pushed the 240524_spi_nor_remove_idpd branch from 7f9c52a to c0011c2 Compare July 29, 2024 09:37
@JordanYates
Copy link
Contributor Author

Rebased on 3.7.0 and migration notes added

@de-nordic de-nordic self-requested a review August 2, 2024 10:49
@de-nordic
Copy link
Contributor

@JordanYates This needs rebase as it fails on some unrelated issues.

@JordanYates JordanYates force-pushed the 240524_spi_nor_remove_idpd branch from c0011c2 to e535b32 Compare August 3, 2024 01:05
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]>
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]>
Automatically request and release the SPI bus before talking to the
flash chip.

Signed-off-by: Jordan Yates <[email protected]>
@JordanYates JordanYates force-pushed the 240524_spi_nor_remove_idpd branch from e535b32 to 6b7a852 Compare August 3, 2024 01:07
@JordanYates
Copy link
Contributor Author

ping @de-nordic

@nashif nashif merged commit 7b4e7ff into zephyrproject-rtos:main Aug 27, 2024
@JordanYates JordanYates deleted the 240524_spi_nor_remove_idpd branch August 27, 2024 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Flash area: Samples Samples Release Notes To be mentioned in the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.