Re: [PATCH 03/10] mci: sdhci: add ADMA2 descriptor helpers

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

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@anthropic.com>
> Signed-off-by: Sascha Hauer <s.hauer@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 |