[PATCH 03/10] mci: sdhci: add ADMA2 descriptor helpers
Ahmad Fatoum
a.fatoum at pengutronix.de
Mon May 18 02:18:57 PDT 2026
Hi,
On 5/11/26 2:07 PM, Sascha Hauer wrote:
> Add reusable ADMA2 (32-bit and 64-bit) support to the SDHCI core so
> drivers can opt in to ADMA without each having to reimplement descriptor
> table management.
>
> A driver enables ADMA by calling sdhci_setup_adma() after
> sdhci_setup_host(). The helper allocates a DMA-coherent descriptor
> table sized for SDHCI_DEFAULT_ADMA_DESCS entries (drivers can override
> adma_table_cnt before calling), picks the descriptor format based on
> SDHCI_USE_64_BIT_DMA, sets SDHCI_USE_ADMA in host->flags and caps
> mci->max_req_size so the MCI core splits requests to fit. From there,
> sdhci_setup_data_dma() builds an ADMA2 descriptor chain for the
> contiguous transfer buffer (one descriptor per up-to-64 KiB chunk plus
> a terminating nop/end entry) and programs SDHCI_ADMA_ADDRESS instead
> of the SDMA address. sdhci_config_dma() now selects ADMA32/ADMA64 in
> HOST_CONTROL accordingly.
>
> If sdhci_setup_adma() fails (no SDHCI_CAN_DO_ADMA2, no memory, or
> unaligned table), the host transparently falls back to the existing
> SDMA path.
See below for feedback.
>
> Assisted-by: Claude Opus 4.7 <noreply at anthropic.com>
> Signed-off-by: Sascha Hauer <s.hauer at pengutronix.de>
> ---
> drivers/mci/sdhci.c | 154 +++++++++++++++++++++++++++++++++++++++++++++++++++-
> drivers/mci/sdhci.h | 59 ++++++++++++++++++++
> 2 files changed, 212 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mci/sdhci.c b/drivers/mci/sdhci.c
> index f7172347e1..0c3ca69e9a 100644
> --- a/drivers/mci/sdhci.c
> +++ b/drivers/mci/sdhci.c
> @@ -585,6 +585,13 @@ static void sdhci_config_dma(struct sdhci *host)
> ctrl = sdhci_read8(host, SDHCI_HOST_CONTROL);
> /* Note if DMA Select is zero then SDMA is selected */
> ctrl &= ~SDHCI_CTRL_DMA_MASK;
> +
> + if (host->flags & SDHCI_USE_ADMA) {
> + ctrl |= SDHCI_CTRL_ADMA32;
> + if (host->flags & SDHCI_USE_64_BIT_DMA && !host->v4_mode)
> + ctrl |= SDHCI_CTRL_ADMA64;
> + }
> +
> sdhci_write8(host, SDHCI_HOST_CONTROL, ctrl);
>
> if (host->flags & SDHCI_USE_64_BIT_DMA) {
> @@ -601,11 +608,67 @@ static void sdhci_config_dma(struct sdhci *host)
> }
> }
>
> +static void sdhci_adma_write_desc(struct sdhci *host, void **desc,
> + dma_addr_t addr, int len, unsigned int cmd)
> +{
> + struct sdhci_adma2_64_desc *dma_desc = *desc;
This should be a union between sdhci_adma2_32_desc and sdhci_adma2_64_desc.
> +
> + /* 32-bit and 64-bit descriptors share these fields. */
> + dma_desc->cmd = cpu_to_le16(cmd);
> + dma_desc->len = cpu_to_le16(len);
> + dma_desc->addr_lo = cpu_to_le32(lower_32_bits(addr));
> +
> + if (host->flags & SDHCI_USE_64_BIT_DMA)
> + dma_desc->addr_hi = cpu_to_le32(upper_32_bits(addr));
On a 64-bit system without SDHCI_USE_64_BIT_DMA, but with a 64-bit addr,
we will end up with memory corruption here.
Please add at least a BUG, so it fails reliably or propagate an error.
> +
> + *desc += host->desc_sz;
> +}
> +EXPORT_SYMBOL_GPL(sdhci_adma_write_desc);
Why export a static symbol?
> +
> +/*
> + * Build the ADMA2 descriptor table for a single contiguous DMA buffer.
> + * Each entry of the table covers up to SDHCI_ADMA2_MAX_LEN bytes; longer
> + * transfers are split across multiple descriptors.
> + */
> +static int sdhci_adma_build_table(struct sdhci *host, dma_addr_t addr,
> + unsigned int len)
> +{
> + void *desc = host->adma_table;
> + void *desc_end = host->adma_table + host->adma_table_sz;
> +
> + while (len) {
> + unsigned int chunk = min_t(unsigned int, len,
> + SDHCI_ADMA2_MAX_LEN);
I think the min_t is unneeded and if it is, just give len the same type
as SDHCI_ADMA2_MAX_LEN?
> +
> + if (desc + host->desc_sz > desc_end)
> + return -ENOSPC;
> +
> + /*
> + * The length field is 16-bit; a length of 0 encodes
> + * SDHCI_ADMA2_MAX_LEN bytes per the SD Host Controller
> + * specification.
> + */
> + sdhci_adma_write_desc(host, &desc, addr, chunk & 0xffff,
> + ADMA2_TRAN_VALID);
> + addr += chunk;
> + len -= chunk;
> + }
> +
> + if (desc + host->desc_sz > desc_end)
> + return -ENOSPC;
Nitpick: I think it would be cleaner to move this check into
sdhci_adma_write_desc(), so it returns an error on out-of-bound write
and just propagate the error instead of redoing the same if clause here
and in the loop.
> +
> + /* Append a terminating descriptor (nop, end, valid). */
> + sdhci_adma_write_desc(host, &desc, 0, 0, ADMA2_NOP_END_VALID);
> +
> + return 0;
> +}
> +
> void sdhci_setup_data_dma(struct sdhci *sdhci, struct mci_data *data,
> dma_addr_t *dma)
> {
> struct device *dev = sdhci_dev(sdhci);
> int nbytes;
> + int ret;
>
> if (!data) {
> if (dma)
> @@ -633,7 +696,22 @@ void sdhci_setup_data_dma(struct sdhci *sdhci, struct mci_data *data,
> }
>
> sdhci_config_dma(sdhci);
> - sdhci_set_sdma_addr(sdhci, *dma);
> +
> + if (sdhci->flags & SDHCI_USE_ADMA) {
> + ret = sdhci_adma_build_table(sdhci, *dma, nbytes);
> + if (ret) {
> + dev_err(dev, "ADMA table build failed: %pe\n",
> + ERR_PTR(ret));
> + dma_unmap_single(dev, *dma, nbytes,
> + (data->flags & MMC_DATA_READ) ?
> + DMA_FROM_DEVICE : DMA_TO_DEVICE);
Why is the data umapped here?!
> + *dma = SDHCI_NO_DMA;
> + return;
> + }
> + sdhci_set_adma_addr(sdhci, sdhci->adma_addr);
> + } else {
> + sdhci_set_sdma_addr(sdhci, *dma);
> + }
> }
>
> void sdhci_teardown_data(struct sdhci *sdhci,
> @@ -1213,3 +1291,77 @@ int sdhci_setup_host(struct sdhci *host)
>
> return 0;
> }
> +
> +/**
> + * sdhci_setup_adma() - allocate ADMA descriptor table and enable ADMA
> + * @host: sdhci host
> + *
> + * Allocate a DMA-coherent ADMA2 descriptor table and mark the host as
> + * ADMA-capable so subsequent calls to sdhci_setup_data_dma() use ADMA
> + * instead of SDMA. Drivers must call this after sdhci_setup_host() since
> + * it relies on the SDHCI_USE_64_BIT_DMA flag established there.
> + *
> + * The descriptor count defaults to SDHCI_DEFAULT_ADMA_DESCS, which caps
> + * the largest single transfer at SDHCI_DEFAULT_ADMA_DESCS *
> + * SDHCI_ADMA2_MAX_LEN bytes. Drivers can override host->adma_table_cnt
> + * before calling to allocate a different size.
> + *
> + * Returns 0 on success or a negative error code on failure. On failure
> + * the host falls back to SDMA.
> + */
> +int sdhci_setup_adma(struct sdhci *host)
> +{
> + struct device *dev = sdhci_dev(host);
> + struct mci_host *mci = host->mci;
> + dma_addr_t dma;
> + void *buf;
> +
> + BUG_ON(!mci);
> +
> + /*
> + * Without a controller capability bit ADMA2 cannot be used. Don't
> + * fail loudly: the driver may have called us speculatively, just
> + * leave SDMA as the fallback.
> + */
> + if (!(host->caps & SDHCI_CAN_DO_ADMA2))
> + return -ENOTSUPP;
> +
> + if (host->flags & SDHCI_USE_64_BIT_DMA)
> + host->desc_sz = SDHCI_ADMA2_64_DESC_SZ(host);
> + else
> + host->desc_sz = SDHCI_ADMA2_32_DESC_SZ;
> +
> + if (!host->adma_table_cnt)
> + host->adma_table_cnt = SDHCI_DEFAULT_ADMA_DESCS;
> +
> + host->adma_table_sz = host->adma_table_cnt * host->desc_sz;
> +
> + buf = dma_alloc_coherent(dev, host->adma_table_sz, &dma);
> + if (!buf)
> + return -ENOMEM;
> +
> + host->adma_table = buf;
> + host->adma_addr = dma;
> + host->flags |= SDHCI_USE_ADMA;
> +
> + /*
> + * One descriptor handles up to SDHCI_ADMA2_MAX_LEN bytes; the last
> + * one is reserved for the terminating entry.
> + */
> + mci->max_req_size = (host->adma_table_cnt - 1) * SDHCI_ADMA2_MAX_LEN;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(sdhci_setup_adma);
> +
> +void sdhci_release_adma(struct sdhci *host)
> +{
> + if (!(host->flags & SDHCI_USE_ADMA))
> + return;
> +
> + dma_free_coherent(sdhci_dev(host), host->adma_table, host->adma_addr,
> + host->adma_table_sz);
> + host->adma_table = NULL;
Why zero host->adma_table, but not host->adma_addr?
> + host->flags &= ~SDHCI_USE_ADMA;
> +}
> +EXPORT_SYMBOL_GPL(sdhci_release_adma);
Cheers,
Ahmad
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
More information about the barebox
mailing list