Skip to content

drivers: disk: stm32_sdmmc: add PM runtime support #100123

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+173 −4
Open

drivers: disk: stm32_sdmmc: add PM runtime support #100123

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
177 changes: 173 additions & 4 deletions drivers/disk/sdmmc_stm32.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
#include <zephyr/drivers/gpio.h>
#include <zephyr/drivers/reset.h>
#include <zephyr/pm/policy.h>
#include <zephyr/pm/device.h>
#include <zephyr/pm/device_runtime.h>
#include <zephyr/logging/log.h>
#include <zephyr/irq.h>
#include <soc.h>
Expand Down Expand Up @@ -372,6 +374,9 @@
HAL_StatusTypeDef hal_ret;
int err;

#ifdef CONFIG_PM_DEVICE_RUNTIME
pm_device_runtime_get(dev);
#endif
Comment on lines +377 to +379
Copy link
Contributor

@etienne-lms etienne-lms Nov 27, 2025

Choose a reason for hiding this comment

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

pm_device_runtime_*() are already stubbed when CONFIG_PM_DEVICE_RUNTIME is disabled.

Suggested change
#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?

Copy link
Contributor

@bearsh bearsh Nov 27, 2025

Choose a reason for hiding this comment

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

if CONFIG_PM_DEVICE_RUNTIME is defined, the following code is being called twice, once in stm32_sdmmc_activate() and then here

err = stm32_sdmmc_pwr_on(priv);
if (err) {
return -EIO;
Expand Down Expand Up @@ -460,9 +465,10 @@
return err;
}

static int stm32_sdmmc_access_deinit(struct stm32_sdmmc_priv *priv)
static int stm32_sdmmc_access_deinit(const struct device *dev)
{
HAL_StatusTypeDef hal_ret;
struct stm32_sdmmc_priv *priv = dev->data;

#if STM32_SDMMC_USE_DMA
int err;
Expand Down Expand Up @@ -491,6 +497,10 @@
stm32_sdmmc_pwr_off(priv);

priv->status = DISK_STATUS_UNINIT;

#ifdef CONFIG_PM_DEVICE_RUNTIME
pm_device_runtime_put(dev);
#endif
Copy link
Contributor

@bearsh bearsh Nov 27, 2025

Choose a reason for hiding this comment

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

here the same, stm32_sdmmc_suspend() repeats code already executed in stm32_sdmmc_access_deinit()

return 0;
}

Expand Down Expand Up @@ -713,7 +723,7 @@
case DISK_IOCTL_CTRL_INIT:
return stm32_sdmmc_access_init(disk);
case DISK_IOCTL_CTRL_DEINIT:
return stm32_sdmmc_access_deinit(priv);
return stm32_sdmmc_access_deinit(dev);
default:
return -EINVAL;
}
Expand Down Expand Up @@ -772,7 +782,7 @@
priv->status = DISK_STATUS_UNINIT;
} else {
LOG_DBG("card removed");
stm32_sdmmc_access_deinit(priv);
stm32_sdmmc_access_deinit(stm32_sdmmc_info.dev);
priv->status = DISK_STATUS_NOMEDIA;
}
}
Expand Down Expand Up @@ -923,6 +933,163 @@
memcpy(csd, &priv->hsd.CSD, sizeof(priv->hsd.CSD));
}

static int stm32_sdmmc_activate(const struct device *dev)
Copy link
Contributor

@etienne-lms etienne-lms Nov 27, 2025

Choose a reason for hiding this comment

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

Most of the sequence is already done in in stm32_sdmmc_access_init(). Could you factorize their common code?

{
pm_policy_state_lock_get(PM_STATE_SUSPEND_TO_IDLE, PM_ALL_SUBSTATES);
Copy link
Contributor

@etienne-lms etienne-lms Nov 27, 2025

Choose a reason for hiding this comment

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

Is this needed?

Copy link
Contributor

@bearsh bearsh Nov 27, 2025

Choose a reason for hiding this comment

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

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


struct stm32_sdmmc_priv *priv = dev->data;
int err = 0;

/* serialize against read/write */
k_sem_take(&priv->thread_lock, K_FOREVER);

/* Power rail first (if present) */
err = stm32_sdmmc_pwr_on(priv);
if (err) {
goto out;
}

/* Default (active) pins */
err = pinctrl_apply_state(priv->pcfg, PINCTRL_STATE_DEFAULT);
if (err) {
LOG_ERR("pinctrl default failed: %d", err);
goto out_pwr_off;
}

/* Enable SDMMC clocks (APB + 48-MHz domain) */
err = stm32_sdmmc_clock_enable(priv);
if (err) {
LOG_ERR("clock enable failed: %d", err);
goto out_sleep_pins;
}

/* Clean peripheral state */
err = reset_line_toggle_dt(&priv->reset);
if (err) {
LOG_ERR("peripheral reset failed: %d", err);
goto out_clk_off;
}

#if STM32_SDMMC_USE_DMA
err = stm32_sdmmc_dma_init(priv);
if (err) {
LOG_ERR("DMA init failed: %d", err);
goto out_clk_off;
}
#endif

#ifdef CONFIG_SDMMC_STM32_EMMC
err = HAL_MMC_Init(&priv->hsd);
#else
err = HAL_SD_Init(&priv->hsd);
#endif
if (err != HAL_OK) {
LOG_ERR("HAL *_Init failed (ErrorCode 0x%X)", priv->hsd.ErrorCode);
err = -EIO;
goto out_dma_deinit;
}

/* IRQ last */
priv->irq_config(dev);
irq_enable(DT_INST_IRQN(0));

#ifdef CONFIG_SDMMC_STM32_HWFC
stm32_sdmmc_fc_enable(priv);
#endif

goto out; /* success */

out_dma_deinit:
#if STM32_SDMMC_USE_DMA
(void)stm32_sdmmc_dma_deinit(priv);
#endif
out_clk_off:
(void)stm32_sdmmc_clock_disable(priv);
out_sleep_pins:
(void)pinctrl_apply_state(priv->pcfg, PINCTRL_STATE_SLEEP);
out_pwr_off:
stm32_sdmmc_pwr_off(priv);
out:
k_sem_give(&priv->thread_lock);
pm_policy_state_lock_put(PM_STATE_SUSPEND_TO_IDLE, PM_ALL_SUBSTATES);
return err;
}

static int stm32_sdmmc_suspend(const struct device *dev)
{
pm_policy_state_lock_get(PM_STATE_SUSPEND_TO_IDLE, PM_ALL_SUBSTATES);

struct stm32_sdmmc_priv *priv = dev->data;

/* serialize against read/write */
k_sem_take(&priv->thread_lock, K_FOREVER);

/* Stop IRQs first so nothing wakes us/keeps us busy */
irq_disable(DT_INST_IRQN(0));
NVIC_ClearPendingIRQ(DT_INST_IRQN(0));

/* DeInit HAL (if it was ever init’d) */
#ifdef CONFIG_SDMMC_STM32_EMMC
(void)HAL_MMC_DeInit(&priv->hsd);
#else
(void)HAL_SD_DeInit(&priv->hsd);
#endif

/* DMA fully idle (and detach from HAL) */
#if STM32_SDMMC_USE_DMA
(void)stm32_sdmmc_dma_deinit(priv);
#endif

/* 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();
Copy link
Contributor

@etienne-lms etienne-lms Nov 27, 2025

Choose a reason for hiding this comment

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

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.


/* Apply sleep/low-leakage pin state before removing power */
int ret = pinctrl_apply_state(priv->pcfg, PINCTRL_STATE_SLEEP);
if (ret && ret != -ENOTSUP) {

Check warning on line 1050 in drivers/disk/sdmmc_stm32.c

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

LINE_SPACING 

drivers/disk/sdmmc_stm32.c:1050  Missing a blank line after declarations
LOG_WRN("sleep pinctrl failed: %d", ret);
Copy link
Contributor

@etienne-lms etienne-lms Nov 27, 2025

Choose a reason for hiding this comment

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

LOG_WARN_ONCE() should be enough.
That said stm32_sdmmc_pwr_off(), called right above, already does the job.

}

/* Optionally cut card power (if you wired pwr_gpios) */
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): */

Check warning on line 1058 in drivers/disk/sdmmc_stm32.c

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

BLOCK_COMMENT_STYLE 

drivers/disk/sdmmc_stm32.c:1058  Block comments use a trailing */ on a separate line

Check warning on line 1058 in drivers/disk/sdmmc_stm32.c

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

BLOCK_COMMENT_STYLE 

drivers/disk/sdmmc_stm32.c:1058  Block comments use * on subsequent lines
Copy link
Contributor

@etienne-lms etienne-lms Nov 27, 2025

Choose a reason for hiding this comment

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

As you said, this may be unsafe. I would leave it running for now until there are some refcount on clock gating requests.

#if defined(LL_RCC_HSI48_IsReady)
if (LL_RCC_HSI48_IsReady()) {
LL_RCC_HSI48_Disable();
while (LL_RCC_HSI48_IsReady()) {
;

Check warning on line 1063 in drivers/disk/sdmmc_stm32.c

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Remove this empty statement. 


See more on https://sonarcloud.io/project/issues?id=zephyrproject-rtos_zephyr&issues=AZrCBKqJzAF9f2sW26Hw&open=AZrCBKqJzAF9f2sW26Hw&pullRequest=100123
}
}
#endif

k_sem_give(&priv->thread_lock);
pm_policy_state_lock_put(PM_STATE_SUSPEND_TO_IDLE, PM_ALL_SUBSTATES);
return 0;
}

#ifdef CONFIG_PM_DEVICE
Copy link
Contributor

@etienne-lms etienne-lms Nov 27, 2025

Choose a reason for hiding this comment

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

Condition can be remove.

int stm32_sdmmc_pm_action(const struct device *dev, enum pm_device_action action)
{
int err;

switch (action) {
case PM_DEVICE_ACTION_RESUME:
err = stm32_sdmmc_activate(dev);
Copy link
Contributor

@etienne-lms etienne-lms Nov 27, 2025

Choose a reason for hiding this comment

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

Nitpicking: can remove err and use return stm32_sdmmc_xxx(dev); straight.

break;
case PM_DEVICE_ACTION_SUSPEND:
err = stm32_sdmmc_suspend(dev);
break;
default:
return -ENOTSUP;
}

return err;
}
#endif

#if DT_NODE_HAS_STATUS_OKAY(DT_DRV_INST(0))

#if STM32_SDMMC_USE_DMA
Expand Down Expand Up @@ -1002,7 +1169,9 @@
#endif
};

DEVICE_DT_INST_DEFINE(0, disk_stm32_sdmmc_init, NULL,
PM_DEVICE_DT_INST_DEFINE(0, stm32_sdmmc_pm_action);

DEVICE_DT_INST_DEFINE(0, disk_stm32_sdmmc_init, PM_DEVICE_DT_INST_GET(0),
&stm32_sdmmc_priv_1, NULL, POST_KERNEL,
CONFIG_SD_INIT_PRIORITY,
NULL);
Expand Down
Loading