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

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

Hello,

On 5/18/26 2:16 PM, Sascha Hauer wrote:
> 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.

I don't like that, it's not standards-compliant and will just makes it
harder for static analysis.. :/

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

Okay.

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

Fair enough.

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

I somehow missed that this is error handling.
That's fine then.

Thanks,
Ahmad

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