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

["Sascha Hauer" <s.hauer@pengutronix.de>]

On 2026-05-18 11:18, Ahmad Fatoum wrote:
> 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.

This exactly matches the Kernel. Might be cleaner to make this a union,
but having the same code as the kernel here is also nice.

> 
> > +
> > +	/* 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.

This problem is pre-existing in the tree. Yes, we should catch this, but
should be another patch.

>
> > +
> > +	*desc += host->desc_sz;
> > +}
> > +EXPORT_SYMBOL_GPL(sdhci_adma_write_desc);
> 
> Why export a static symbol?

Was exported previously, forgot to remove when making it static.

> 
> > +
> > +/*
> > + * 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?

That would be int. But should we really allow negative lengths as input
to this function?

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

Ok.

> 
> > +
> > +	/* 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?!

We have to. We are returning SDHCI_NO_DMA as dma address, so the caller
can't unmap it.

Sascha


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