Re: [PATCH v3 2/7] mmc: sdhci: add support for using external DMA devices

[Faiz Abbas]

Hi Adrian,

On 12/12/19 6:25 pm, Adrian Hunter wrote:
> On 10/12/19 11:51 am, Faiz Abbas wrote:
>> From: Chunyan Zhang <zhang.chunyan@xxxxxxxxxx>
>>
>> Some standard SD host controllers can support both external dma
>> controllers as well as ADMA/SDMA in which the SD host controller
>> acts as DMA master. TI's omap controller is the case as an example.
>>
>> Currently the generic SDHCI code supports ADMA/SDMA integrated in
>> the host controller but does not have any support for external DMA
>> controllers implemented using dmaengine, meaning that custom code is
>> needed for any systems that use an external DMA controller with SDHCI.
>>
>> Fixes by Faiz Abbas <faiz_abbas@xxxxxx>:
>> 1. Map scatterlists before dmaengine_prep_slave_sg()
>> 2. Use dma_async() functions inside of the send_command() path and call
>> terminate_sync() in non-atomic context in case of an error.
>>
>> Signed-off-by: Chunyan Zhang <zhang.chunyan@xxxxxxxxxx>
>> Signed-off-by: Faiz Abbas <faiz_abbas@xxxxxx>
>> ---
...
>>  {
>> @@ -1379,12 +1562,19 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
>>  	}
>>  
>>  	host->cmd = cmd;
>> +	host->data_timeout = 0;
>>  	if (sdhci_data_line_cmd(cmd)) {
>>  		WARN_ON(host->data_cmd);
>>  		host->data_cmd = cmd;
>> +		sdhci_set_timeout(host, cmd);
>>  	}
>>  
>> -	sdhci_prepare_data(host, cmd);
>> +	if (cmd->data) {
>> +		if (host->use_external_dma)
>> +			sdhci_external_dma_prepare_data(host, cmd);
>> +		else
>> +			sdhci_prepare_data(host, cmd);
>> +	}
> 
> Please make the 3 changes above and the corresponding changes
> sdhci_prepare_data into a separate patch i.e.

Ok. And I agree with all your style change requests above this. Will fix
in v4.

>> @@ -2652,6 +2845,18 @@ static bool sdhci_request_done(struct sdhci_host *host)
>>  	if (host->flags & SDHCI_REQ_USE_DMA) {
>>  		struct mmc_data *data = mrq->data;
>>  
>> +		spin_unlock_irqrestore(&host->lock, flags);
>> +
>> +		/* Terminate and synchronize dma in case of an error */
>> +		if (data && (mrq->cmd->error || data->error) &&
>> +		    host->use_external_dma) {
>> +			struct dma_chan *chan = sdhci_external_dma_channel(host,
>> +									  data);
>> +			dmaengine_terminate_sync(chan);
>> +		}
>> +
>> +		spin_lock_irqsave(&host->lock, flags);
>> +
> 
> Need to take the mrq out of mrqs_done[] to ensure it is not processed again,
> and put it back again to be consistent with the remaining code. Also put
> host->use_external_dma as the first condition i.e.
> 
> 		if (host->use_external_dma && data &&
> 		    (mrq->cmd->error || data->error)) {
> 			struct dma_chan *chan = sdhci_external_dma_channel(host, data);
> 
> 			host->mrqs_done[i] = NULL;
> 			spin_unlock_irqrestore(&host->lock, flags);
> 			dmaengine_terminate_sync(chan);
> 			spin_lock_irqsave(&host->lock, flags);
> 			sdhci_set_mrq_done(host, mrq);
> 		}
> 
> where sdhci_set_mrq_done() is factored out from __sdhci_finish_mrq() i.e.
> 
> static void sdhci_set_mrq_done(struct sdhci_host *host, struct mmc_request *mrq)
> {
> 	int i;
> 
> 	for (i = 0; i < SDHCI_MAX_MRQS; i++) {
> 		if (host->mrqs_done[i] == mrq) {
> 			WARN_ON(1);
> 			return;
> 		}
> 	}
> 
> 	for (i = 0; i < SDHCI_MAX_MRQS; i++) {
> 		if (!host->mrqs_done[i]) {
> 			host->mrqs_done[i] = mrq;
> 			break;
> 		}
> 	}
> 
> 	WARN_ON(i >= SDHCI_MAX_MRQS);
> }
> 
> sdhci_set_mrq_done() can be made in the refactoring patch.
Haven't we already done the sdhci_set_mrq_done() part in
__sdhci_finish_mrq()?

We are picking up an already "done" mrq, looking at whether it had any
error and then sychronizing with external dma. Or at least that is my
understanding.

> 
>>  		if (data && data->host_cookie == COOKIE_MAPPED) {
>>  			if (host->bounce_buffer) {
>>  				/*
>> @@ -3758,12 +3963,28 @@ int sdhci_setup_host(struct sdhci_host *host)
>>  		       mmc_hostname(mmc), host->version);
>>  	}
>>  
>> -	if (host->quirks & SDHCI_QUIRK_FORCE_DMA)
>> +	if (host->use_external_dma) {
>> +		ret = sdhci_external_dma_init(host);
>> +		if (ret == -EPROBE_DEFER)
>> +			goto unreg;
>> +
>> +		/*
>> +		 * Fall back to use the DMA/PIO integrated in standard SDHCI
>> +		 * instead of external DMA devices.
>> +		 */
>> +		if (ret)
>> +			sdhci_switch_external_dma(host, false);
>> +	}
>> +
>> +	if (host->quirks & SDHCI_QUIRK_FORCE_DMA) {
>>  		host->flags |= SDHCI_USE_SDMA;
>> -	else if (!(host->caps & SDHCI_CAN_DO_SDMA))
>> +	} else if (!(host->caps & SDHCI_CAN_DO_SDMA)) {
>>  		DBG("Controller doesn't have SDMA capability\n");
>> -	else
>> +	} else if (host->use_external_dma) {
>> +		/* Using dma-names to detect external dma capability */
> 
> What is this change for?  Do you expect for SDHCI_USE_SDMA and
> SDHCI_USE_ADMA flags to be clear?

Yes. Today the code enables SDMA by default (in the else part below
this). I want it to not enable SDMA in the external dma case.

Thanks,
Faiz