drivers: disk: stm32_sdmmc: add PM runtime support #100123
Conversation
Added PM device runtime actions so driver now honors device runtime actions when PM_DEVICE and/or PM_DEVICE_RUNTIME is used. Signed-off-by: Praneeth <[email protected]>
|
Hello @Praneethsvch, and thank you very much for your first pull request to the Zephyr project! |
zephyrbot
requested review from
FRASTM,
GeorgeCGV,
danieldegrasse,
decsny,
djiatsaf-st,
erwango,
etienne-lms,
gautierg-st,
jfischer-no,
juickar and
mathieuchopstm
|
bearsh
left a comment
bearsh
left a comment
There was a problem hiding this comment.
pm_device_runtime_get() / pm_device_runtime_put() is not called in stm32_sdmmc_access_read() stm32_sdmmc_access_write()
|
|
||
| #ifdef CONFIG_PM_DEVICE_RUNTIME | ||
| pm_device_runtime_get(dev); | ||
| #endif |
There was a problem hiding this comment.
if CONFIG_PM_DEVICE_RUNTIME is defined, the following code is being called twice, once in stm32_sdmmc_activate() and then here
|
|
||
| #ifdef CONFIG_PM_DEVICE_RUNTIME | ||
| pm_device_runtime_put(dev); | ||
| #endif |
There was a problem hiding this comment.
here the same, stm32_sdmmc_suspend() repeats code already executed in stm32_sdmmc_access_deinit()
| #ifdef CONFIG_PM_DEVICE_RUNTIME | ||
| pm_device_runtime_get(dev); | ||
| #endif |
There was a problem hiding this comment.
pm_device_runtime_*() are already stubbed when CONFIG_PM_DEVICE_RUNTIME is disabled.
| #ifdef CONFIG_PM_DEVICE_RUNTIME | |
| pm_device_runtime_get(dev); | |
| #endif | |
| pm_device_runtime_get(dev); |
Ditto below.
That said, device the device for runtime PM would make stm32_sdmmc_activate() to be called, so all the clock/pinctrl/SD/MMC initialization already proceeded, no?
| return 0; | ||
| } | ||
|
|
||
| #ifdef CONFIG_PM_DEVICE |
There was a problem hiding this comment.
Condition can be remove.
|
|
||
| switch (action) { | ||
| case PM_DEVICE_ACTION_RESUME: | ||
| err = stm32_sdmmc_activate(dev); |
There was a problem hiding this comment.
Nitpicking: can remove err and use return stm32_sdmmc_xxx(dev); straight.
| /* Apply sleep/low-leakage pin state before removing power */ | ||
| int ret = pinctrl_apply_state(priv->pcfg, PINCTRL_STATE_SLEEP); | ||
| if (ret && ret != -ENOTSUP) { | ||
| LOG_WRN("sleep pinctrl failed: %d", ret); |
There was a problem hiding this comment.
LOG_WARN_ONCE() should be enough.
That said stm32_sdmmc_pwr_off(), called right above, already does the job.
| /* Gate SDMMC clocks and reset peripheral to kill any latent state */ | ||
| (void)stm32_sdmmc_clock_disable(priv); | ||
| __HAL_RCC_SDMMC1_FORCE_RESET(); | ||
| __HAL_RCC_SDMMC1_RELEASE_RESET(); |
There was a problem hiding this comment.
Prefer reset_line_toggle_dt(&priv->reset); over use of HAL functions.
I wonder if reset should be done at deinit, rather at init, which is already done.
Note: reset operation needs clock to be enabled.
|
|
||
| static int stm32_sdmmc_activate(const struct device *dev) | ||
| { | ||
| pm_policy_state_lock_get(PM_STATE_SUSPEND_TO_IDLE, PM_ALL_SUBSTATES); |
There was a problem hiding this comment.
I think so, SDMMC, at least on a U5 is only working in run and sleep but not stop. I don't thinks this is different in other families...
| stm32_sdmmc_pwr_off(priv); | ||
|
|
||
| /* If HSI48 was used only for SDMMC, system PM may turn it off later. | ||
| If you want to force it off right here (safe if USB not used): */ |
There was a problem hiding this comment.
As you said, this may be unsafe. I would leave it running for now until there are some refcount on clock gating requests.
| memcpy(csd, &priv->hsd.CSD, sizeof(priv->hsd.CSD)); | ||
| } | ||
|
|
||
| static int stm32_sdmmc_activate(const struct device *dev) |
There was a problem hiding this comment.
Most of the sequence is already done in in stm32_sdmmc_access_init(). Could you factorize their common code?
5 participants
Added PM device runtime actions so driver now honors device runtime actions when PM_DEVICE and/or PM_DEVICE_RUNTIME is used. This allows dynamic suspend and resume actions.
stm32_sdmmc_pm_action()handling for PM_DEVICE_ACTION_RESUME/SUSPENDThis ensures the SDMMC peripheral enters low-power states when idle and
restores correctly when accessed again via disk or FS layers.
Tested on: STM32U575 (nucleo_u575zi_q)