Skip to content

drivers: disk: sdmmc_stm32: power up on request #91220

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.

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 4 commits into
base: main
Choose a base branch
from

Conversation

JordanYates
Copy link
Contributor

@JordanYates JordanYates commented Jun 7, 2025

Move power configuration from the device init function into the
DISK_IOCTL_CTRL_INIT and DISK_IOCTL_CTRL_DEINIT handlers. If
pwr-gpios is defined, this ensures that the card is not powered up
until requested, and enables powering down the card when not in use.

This behaviour matches the implementation for sdhc_spi.

The updated implementation has been tested on a custom STM32L451RE board with the following devicetree definition:

&sdmmc1 {
	status = "okay";
	pinctrl-0 = <&sdmmc1_d0_pc8 &sdmmc1_d1_pc9 &sdmmc1_d2_pc10
		&sdmmc1_d3_pc11 &sdmmc1_ck_pc12 &sdmmc1_cmd_pd2>;
	pinctrl-1 = <&analog_pc8 &analog_pc9 &analog_pc10
		&analog_pc11 &analog_pc12 &analog_pd2>;
	pinctrl-names = "default", "sleep";
	pwr-gpios = <&gpiob 5 GPIO_ACTIVE_HIGH>;
	disk-name = "SD";
	bus-width = <4>;
	clk-div = <4>;

	/**
	 * The following channel configuration is correct, but does not compile for L4
	 * dmas = <&dma2 4 1 (STM32_DMA_PERIPH_TX | STM32_DMA_PRIORITY_HIGH)>,
	 *        <&dma2 5 1 (STM32_DMA_PERIPH_RX | STM32_DMA_PRIORITY_HIGH)>;
	 * dma-names = "tx", "rx";
	 */
};

The implementation remains working and low power after power cycling the card multiple times without an application reboot:
image

erwango
erwango previously approved these changes Jun 9, 2025
@JordanYates JordanYates added this to the v4.2.0 milestone Jun 14, 2025
@JordanYates JordanYates force-pushed the 250607_sdmmc_low_power branch from aae30df to 6d3d7cc Compare June 14, 2025 08:00
if ((err != 0) && (err != -ENOENT)) {
LOG_WRN("Failed to apply pin sleep states");
}
gpio_pin_configure_dt(&priv->pe, GPIO_OUTPUT_INACTIVE);
Copy link
Contributor

Choose a reason for hiding this comment

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

I missed that: return value to be tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are quite a number of calls to this function in the driver that already aren't checked.
What action would you want taken if this does fail?

Copy link
Contributor

@etienne-lms etienne-lms Jun 17, 2025

Choose a reason for hiding this comment

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

Indeed.

Suggested change
gpio_pin_configure_dt(&priv->pe, GPIO_OUTPUT_INACTIVE);
if (gpio_pin_configure_dt(&priv->pe, GPIO_OUTPUT_INACTIVE) != 0) {
LOG_WRN("Failed to apply GPIO inactive state");
}

Provide a pointer to the first element of the clock list explicitly (as
done by `stm32_sdmmc_clock_enable`), instead of a pointer to the list.

Signed-off-by: Jordan Yates <[email protected]>
Move power configuration from the device init function into the
`DISK_IOCTL_CTRL_INIT` and `DISK_IOCTL_CTRL_DEINIT` handlers. If
`pwr-gpios` is defined, this ensures that the card is not powered up
until requested, and enables power down the card when not in use.

This behaviour matches the implementation for `sdhc_spi`.

Signed-off-by: Jordan Yates <[email protected]>
Add support for configuring pins into an alternate mode when the
device is powered off.

Signed-off-by: Jordan Yates <[email protected]>
Cleanup functions that only return `0`.

Signed-off-by: Jordan Yates <[email protected]>
@JordanYates JordanYates force-pushed the 250607_sdmmc_low_power branch from 6d3d7cc to e94ffad Compare June 16, 2025 23:26
Copy link

@decsny decsny removed their request for review June 17, 2025 14:51
@erwango erwango modified the milestones: v4.2.0, v4.3.0 Jul 15, 2025
if ((err != 0) && (err != -ENOENT)) {
LOG_WRN("Failed to apply pin sleep states");
}
gpio_pin_configure_dt(&priv->pe, GPIO_OUTPUT_INACTIVE);
Copy link
Contributor

@etienne-lms etienne-lms Jun 17, 2025

Choose a reason for hiding this comment

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

Indeed.

Suggested change
gpio_pin_configure_dt(&priv->pe, GPIO_OUTPUT_INACTIVE);
if (gpio_pin_configure_dt(&priv->pe, GPIO_OUTPUT_INACTIVE) != 0) {
LOG_WRN("Failed to apply GPIO inactive state");
}

}

gpio_pin_interrupt_configure_dt(&priv->cd, GPIO_INT_MODE_DISABLED);
gpio_pin_configure_dt(&priv->cd, GPIO_DISCONNECTED);
gpio_remove_callback(priv->cd.port, &priv->cd_cb);
return 0;
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could remove this instruction.

LOG_WRN("Failed to apply pin sleep states");
}
gpio_pin_configure_dt(&priv->pe, GPIO_OUTPUT_INACTIVE);
return;
Copy link
Contributor

@etienne-lms etienne-lms Jul 18, 2025

Choose a reason for hiding this comment

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

Could remove this return instruction.

@etienne-lms etienne-lms self-requested a review July 18, 2025 15:45
@etienne-lms etienne-lms dismissed their stale review July 18, 2025 16:27

Non-blocking review comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants