From: Faiz Abbas <faiz_abbas@ti.com>
To: Adrian Hunter <adrian.hunter@intel.com>,
<linux-kernel@vger.kernel.org>, <devicetree@vger.kernel.org>,
<linux-mmc@vger.kernel.org>
Cc: <kishon@ti.com>, <mark.rutland@arm.com>, <robh+dt@kernel.org>,
<ulf.hansson@linaro.org>, <zhang.chunyan@linaro.org>,
<tony@atomide.com>
Subject: Re: [PATCH v3 2/7] mmc: sdhci: add support for using external DMA devices
Date: Mon, 16 Dec 2019 13:57:21 +0530 [thread overview]
Message-ID: <fdf1334a-39bc-9247-9934-df6e1562f4b8@ti.com> (raw)
In-Reply-To: <92fd22bf-3024-928d-ebf5-e7382988a36b@intel.com>
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@linaro.org>
>>
>> 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@ti.com>:
>> 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@linaro.org>
>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
>> ---
...
>> {
>> @@ -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
next prev parent reply other threads:[~2019-12-16 8:26 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-10 9:51 [PATCH v3 0/7] Port am335 and am437 devices to sdhci-omap Faiz Abbas
2019-12-10 9:51 ` [PATCH v3 1/7] dt-bindings: sdhci-omap: Add properties for using external dma Faiz Abbas
2019-12-18 21:39 ` Rob Herring
2019-12-10 9:51 ` [PATCH v3 2/7] mmc: sdhci: add support for using external DMA devices Faiz Abbas
2019-12-12 12:55 ` Adrian Hunter
2019-12-16 8:27 ` Faiz Abbas [this message]
2019-12-16 13:45 ` Adrian Hunter
2019-12-23 14:25 ` Faiz Abbas
2019-12-10 9:51 ` [PATCH v3 3/7] mmc: sdhci-omap: Add using external dma Faiz Abbas
2019-12-10 9:51 ` [PATCH v3 4/7] mmc: sdhci: Add quirk for disabling DTO during erase command Faiz Abbas
2019-12-13 9:40 ` Adrian Hunter
2019-12-16 8:42 ` Faiz Abbas
2019-12-10 9:51 ` [PATCH v3 5/7] mmc: sdhci-omap: Add DISABLE_DTO_FOR_ERASE Quirk Faiz Abbas
2019-12-10 9:51 ` [PATCH v3 6/7] dt-bindings: sdhci-omap: Add am335x and am437x specific bindings Faiz Abbas
2019-12-10 9:51 ` [PATCH v3 7/7] mmc: sdhci-omap: Add am335x and am437x specific compatibles Faiz Abbas
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=fdf1334a-39bc-9247-9934-df6e1562f4b8@ti.com \
--to=faiz_abbas@ti.com \
--cc=adrian.hunter@intel.com \
--cc=devicetree@vger.kernel.org \
--cc=kishon@ti.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mmc@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=robh+dt@kernel.org \
--cc=tony@atomide.com \
--cc=ulf.hansson@linaro.org \
--cc=zhang.chunyan@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).