Skip to content

disk: sdmmc: support L4 series with shared DMA channel #91491

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 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
97 changes: 87 additions & 10 deletions drivers/disk/sdmmc_stm32.c
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ LOG_MODULE_REGISTER(stm32_sdmmc, CONFIG_SDMMC_LOG_LEVEL);
#include <zephyr/drivers/dma.h>
#include <zephyr/drivers/dma/dma_stm32.h>
#include <stm32_ll_dma.h>
/* Uses a shared DMA channel for read & write */
#define STM32_SDMMC_USE_DMA_SHARED DT_INST_DMAS_HAS_NAME(0, txrx)
#endif

#ifndef MMC_TypeDef
Expand Down Expand Up @@ -87,11 +89,16 @@ struct stm32_sdmmc_priv {
const struct reset_dt_spec reset;

#if STM32_SDMMC_USE_DMA
#if STM32_SDMMC_USE_DMA_SHARED
struct sdmmc_dma_stream dma_txrx;
DMA_HandleTypeDef dma_txrx_handle;
#else
struct sdmmc_dma_stream dma_rx;
struct sdmmc_dma_stream dma_tx;
DMA_HandleTypeDef dma_tx_handle;
DMA_HandleTypeDef dma_rx_handle;
#endif
#endif /* STM32_SDMMC_USE_DMA */
};

#ifdef CONFIG_SDMMC_STM32_HWFC
Expand Down Expand Up @@ -208,24 +215,45 @@ static int stm32_sdmmc_configure_dma(DMA_HandleTypeDef *handle, struct sdmmc_dma

dma->cfg.user_data = handle;

/* Reserve the channel in the DMA subsystem, even though we use the HAL API.
* See the usage of STM32_DMA_HAL_OVERRIDE.
*/
ret = dma_config(dma->dev, dma->channel, &dma->cfg);
if (ret != 0) {
LOG_ERR("Failed to conig");
return ret;
}

#if DT_HAS_COMPAT_STATUS_OKAY(st_stm32_dma_v1)
handle->Instance = __LL_DMA_GET_STREAM_INSTANCE(dma->reg, dma->channel_nb);
handle->Init.Channel = dma->cfg.dma_slot * DMA_CHANNEL_1;
handle->Init.PeriphInc = DMA_PINC_DISABLE;
handle->Init.MemInc = DMA_MINC_ENABLE;
handle->Init.PeriphDataAlignment = DMA_PDATAALIGN_WORD;
handle->Init.MemDataAlignment = DMA_MDATAALIGN_WORD;
handle->Init.Mode = DMA_PFCTRL;
handle->Init.Priority = table_priority[dma->cfg.channel_priority],
handle->Init.Priority = table_priority[dma->cfg.channel_priority];
handle->Init.FIFOMode = DMA_FIFOMODE_ENABLE;
handle->Init.FIFOThreshold = DMA_FIFO_THRESHOLD_FULL;
handle->Init.MemBurst = DMA_MBURST_INC4;
handle->Init.PeriphBurst = DMA_PBURST_INC4;
#else
uint32_t channel_id = dma->channel_nb - STM32_DMA_STREAM_OFFSET;

BUILD_ASSERT(STM32_SDMMC_USE_DMA_SHARED == 1, "Only txrx is supported on this family");
/* handle->Init.Direction is not initialised here on purpose.
* Since the channel is reused for both directions, the direction is
* configured before each read/write call.
*/
handle->Instance = __LL_DMA_GET_CHANNEL_INSTANCE(dma->reg, channel_id);
handle->Init.Request = dma->cfg.dma_slot;
handle->Init.PeriphInc = DMA_PINC_DISABLE;
handle->Init.MemInc = DMA_MINC_ENABLE;
handle->Init.PeriphDataAlignment = DMA_PDATAALIGN_WORD;
handle->Init.MemDataAlignment = DMA_MDATAALIGN_WORD;
handle->Init.Mode = DMA_NORMAL;
handle->Init.Priority = table_priority[dma->cfg.channel_priority];
#endif

return ret;
}
Expand All @@ -236,6 +264,15 @@ static int stm32_sdmmc_dma_init(struct stm32_sdmmc_priv *priv)

LOG_DBG("using dma");

#if STM32_SDMMC_USE_DMA_SHARED
err = stm32_sdmmc_configure_dma(&priv->dma_txrx_handle, &priv->dma_txrx);
if (err) {
LOG_ERR("failed to init shared DMA");
return err;
}
__HAL_LINKDMA(&priv->hsd, hdmatx, priv->dma_txrx_handle);
__HAL_LINKDMA(&priv->hsd, hdmarx, priv->dma_txrx_handle);
#else
err = stm32_sdmmc_configure_dma(&priv->dma_tx_handle, &priv->dma_tx);
if (err) {
LOG_ERR("failed to init tx dma");
Expand All @@ -251,29 +288,39 @@ static int stm32_sdmmc_dma_init(struct stm32_sdmmc_priv *priv)
}
__HAL_LINKDMA(&priv->hsd, hdmarx, priv->dma_rx_handle);
HAL_DMA_Init(&priv->dma_rx_handle);
#endif /* STM32_SDMMC_USE_DMA_SHARED */

return err;
}

static int stm32_sdmmc_dma_deinit(struct stm32_sdmmc_priv *priv)
{
int ret;

/* Since we use STM32_DMA_HAL_OVERRIDE, the only purpose of dma_stop
* is to notify the DMA subsystem that the channel is no longer in use.
* Calling this before or after HAL_DMA_DeInit makes no difference.
* There is no possibility of runtime failures apart from providing an
* invalid channel ID, which is already validated by the setup.
*/
#if STM32_SDMMC_USE_DMA_SHARED
struct sdmmc_dma_stream *dma_txrx = &priv->dma_txrx;

ret = dma_stop(dma_txrx->dev, dma_txrx->channel);
__ASSERT(ret == 0, "Shared DMA channel index corrupted");
#else
struct sdmmc_dma_stream *dma_tx = &priv->dma_tx;
struct sdmmc_dma_stream *dma_rx = &priv->dma_rx;

ret = dma_stop(dma_tx->dev, dma_tx->channel);
__ASSERT(ret == 0, "TX DMA channel index corrupted");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unless asserting the return value in debug build mode is always sufficient, i think it would be worth:

	int ret = 0;

	(...)

	if (dma_stop(dma_tx->dev, dma_tx->channel) != 0) {
		LOG_ERR("Failed to stop tx DMA transmission");		
		ret = -EIO;
	}
	HAL_DMA_DeInit(&priv->dma_tx_handle);

	if (dma_stop(dma_xx->dev, dma_rx->channel) != 0) {
		LOG_ERR("Failed to stop rx DMA transmission");		
		ret = -EIO;
	}
	HAL_DMA_DeInit(&priv->dma_rx_handle);
#endif

	return ret;		
}		

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is an assertion because there is no code path that results in an error through that function unless RAM is corrupted.

	if (id >= config->max_streams) {
		return -EINVAL;
	}

	if (stream->hal_override) {
		stream->busy = false;
		return 0;
	}

If ID is invalid in the normal case the setup functions will already error out with log messages.
If we're trying to protect against RAM corruption, there are many other problems...

HAL_DMA_DeInit(&priv->dma_tx_handle);
if (ret != 0) {
LOG_ERR("Failed to stop tx DMA transmission");
return ret;
}

ret = dma_stop(dma_rx->dev, dma_rx->channel);
__ASSERT(ret == 0, "RX DMA channel index corrupted");
HAL_DMA_DeInit(&priv->dma_rx_handle);
if (ret != 0) {
LOG_ERR("Failed to stop rx DMA transmission");
return ret;
}
return ret;
#endif
return 0;
}

#endif
Expand Down Expand Up @@ -401,6 +448,15 @@ static int stm32_sdmmc_access_read(struct disk_info *disk, uint8_t *data_buf,

k_sem_take(&priv->thread_lock, K_FOREVER);

#if STM32_SDMMC_USE_DMA_SHARED
/* Initialise the shared DMA channel for the current direction */
priv->dma_txrx_handle.Init.Direction = LL_DMA_DIRECTION_PERIPH_TO_MEMORY;
if (HAL_DMA_Init(&priv->dma_txrx_handle) != HAL_OK) {
err = -EIO;
goto end;
}
#endif

err = stm32_sdmmc_read_blocks(&priv->hsd, data_buf, start_sector, num_sector);
if (err != HAL_OK) {
LOG_ERR("sd read block failed %d", err);
Expand All @@ -410,6 +466,10 @@ static int stm32_sdmmc_access_read(struct disk_info *disk, uint8_t *data_buf,

k_sem_take(&priv->sync, K_FOREVER);

#if STM32_SDMMC_USE_DMA_SHARED
HAL_DMA_DeInit(&priv->dma_txrx_handle);
#endif

if (priv->status != DISK_STATUS_OK) {
LOG_ERR("sd read error %d", priv->status);
err = -EIO;
Expand Down Expand Up @@ -457,6 +517,15 @@ static int stm32_sdmmc_access_write(struct disk_info *disk,

k_sem_take(&priv->thread_lock, K_FOREVER);

#if STM32_SDMMC_USE_DMA_SHARED
/* Initialise the shared DMA channel for the current direction */
priv->dma_txrx_handle.Init.Direction = LL_DMA_DIRECTION_MEMORY_TO_PERIPH;
if (HAL_DMA_Init(&priv->dma_txrx_handle) != HAL_OK) {
err = -EIO;
goto end;
}
#endif

err = stm32_sdmmc_write_blocks(&priv->hsd, (uint8_t *)data_buf, start_sector, num_sector);
if (err != HAL_OK) {
LOG_ERR("sd write block failed %d", err);
Expand All @@ -466,6 +535,10 @@ static int stm32_sdmmc_access_write(struct disk_info *disk,

k_sem_take(&priv->sync, K_FOREVER);

#if STM32_SDMMC_USE_DMA_SHARED
HAL_DMA_DeInit(&priv->dma_txrx_handle);
#endif

if (priv->status != DISK_STATUS_OK) {
LOG_ERR("sd write error %d", priv->status);
err = -EIO;
Expand Down Expand Up @@ -813,8 +886,12 @@ static struct stm32_sdmmc_priv stm32_sdmmc_priv_1 = {
.pclken = pclken_sdmmc,
.pcfg = PINCTRL_DT_INST_DEV_CONFIG_GET(0),
.reset = RESET_DT_SPEC_INST_GET(0),
#if STM32_SDMMC_USE_DMA_SHARED
SDMMC_DMA_CHANNEL(txrx, TXRX)
#else
SDMMC_DMA_CHANNEL(rx, RX)
SDMMC_DMA_CHANNEL(tx, TX)
#endif
};

DEVICE_DT_INST_DEFINE(0, disk_stm32_sdmmc_init, NULL,
Expand Down
7 changes: 6 additions & 1 deletion dts/bindings/mmc/st,stm32-sdmmc.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,15 @@ properties:

For example dmas for TX/RX on SDMMC
dmas = <&dma2 4 6 0x30000 0x00>, <&dma2 4 3 0x30000 0x00>;
Alternatively, for a shared TX/RX DMA channels on a STM32L4
dmas = <&dma2 5 7 STM32_DMA_PRIORITY_HIGH>;

dma-names:
description: |
DMA channel name. If DMA should be used, expected value is "tx", "rx".
DMA channel name. If DMA should be used, expected value is either "tx", "rx"
or a single "txrx" for the shared channel configuration

For example
dma-names = "tx", "rx";
or
dma-names = "txrx";
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
CONFIG_DMA=y
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
/* SPDX-License-Identifier: Apache-2.0 */

&sdmmc1 {
/* Channel 5 request 7 on DMA2 */
dmas = <&dma2 5 7 STM32_DMA_PRIORITY_HIGH>;
dma-names = "txrx";
};