[PATCH 03/10] mci: sdhci: add ADMA2 descriptor helpers
Sascha Hauer
s.hauer at pengutronix.de
Mon May 18 05:16:53 PDT 2026
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 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.
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 |
More information about the barebox
mailing list