On 16/12/19 10:27 am, Faiz Abbas wrote:
> 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.
sdhci supports having 2 requests (1 data, 1 cmd) at a time, so there is an
error case where 1 request will wait for the 2nd request before doing a
reset. That logic is further down in sdhci_request_done() so you have to
put the mrq back into host->mrqs_done[] to make it work.
>
>>
>>> 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.
What about moving the "if (host->use_external_dma) {" clause and explicitly
clearing SDHCI_USE_SDMA and SDHCI_USE_ADMA?
