[PATCH RFC 0/3] Add support for using external dma in SDHCI

[PATCH RFC 0/3] Add support for using external dma in SDHCI

[Chunyan Zhang]

Currently the generic SDHCI code in the Linux kernel supports the SD
standard DMA integrated into 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 a generic DMA
controller with SDHCI which in practice means any SDHCI controller that
doesn't have an integrated DMA controller so we should have this as a
generic feature.

There are already a number of controller specific drivers that have dmaengine
code, and some could use sdhci.c actually, but needed to implement mmc_ops->request()
in their specific driver for sending command with external dma using dmaengine
framework, with this patchset, them will take advantage of the generic support.
TI's omap controller is the case as an example.

Any comments are very appreciated.

Thanks,
Chunyan

Chunyan Zhang (3):
  mmc: sdhci: add support for using external DMA devices
  mmc: sdhci-omap: Add using external dma
  dt-bindings: sdhci-omap: Add example for using dmaengine

 .../devicetree/bindings/mmc/sdhci-omap.txt         |   7 ++
 drivers/mmc/host/Kconfig                           |  13 ++
 drivers/mmc/host/sdhci-omap.c                      |   7 ++
 drivers/mmc/host/sdhci.c                           | 137 ++++++++++++++++++++-
 drivers/mmc/host/sdhci.h                           |  12 ++
 5 files changed, 175 insertions(+), 1 deletion(-)

-- 
2.7.4

[PATCH RFC 1/3] mmc: sdhci: add support for using external DMA devices

[Chunyan Zhang]

Some standard SD host controller can support both external dma
controllers as well as ADMA in which the controller acts as
DMA master.

Currently the generic SDHCI code supports ADMA/SDMA integrated into
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 a generic DMA controller with SDHCI.

Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
---
 drivers/mmc/host/Kconfig |  13 +++++
 drivers/mmc/host/sdhci.c | 137 ++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/mmc/host/sdhci.h |  12 +++++
 3 files changed, 161 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 1b58739..c4745d8 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -977,3 +977,16 @@ config MMC_SDHCI_OMAP
 	  If you have a controller with this interface, say Y or M here.
 
 	  If unsure, say N.
+
+config MMC_SDHCI_EXTDMA
+        bool "Support external DMA in standard SD host controller"
+	depends on MMC_SDHCI
+	depends on DMA_ENGINE
+	help
+	  This is an option for using external DMA device via dmaengine
+	  framework.
+
+	  If you have a controller which supports using external DMA device
+	  for data transfer, can say Y.
+
+	  If unsure, say N.
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 99bdae5..ffb1d2b 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -14,6 +14,7 @@
  */
 
 #include <linux/delay.h>
+#include <linux/dmaengine.h>
 #include <linux/ktime.h>
 #include <linux/highmem.h>
 #include <linux/io.h>
@@ -1309,6 +1310,128 @@ static void sdhci_del_timer(struct sdhci_host *host, struct mmc_request *mrq)
 		del_timer(&host->timer);
 }
 
+#if IS_ENABLED(CONFIG_MMC_SDHCI_EXTDMA)
+static int sdhci_extdma_init_chan(struct sdhci_host *host)
+{
+	int ret = 0;
+	struct mmc_host *mmc = host->mmc;
+	struct sdhci_extdma *dma = &host->extdma;
+
+	dma->tx_chan = dma_request_chan(mmc->parent, "tx");
+	if (IS_ERR(dma->tx_chan)) {
+		ret = PTR_ERR(dma->tx_chan);
+		dma->tx_chan = NULL;
+		pr_warn("Failed to request TX DMA channel.\n");
+		return ret;
+	}
+
+	dma->rx_chan = dma_request_chan(mmc->parent, "rx");
+	if (IS_ERR(dma->rx_chan)) {
+		ret = PTR_ERR(dma->rx_chan);
+		if (ret == -EPROBE_DEFER && dma->tx_chan)
+			dma_release_channel(dma->tx_chan);
+
+		dma->rx_chan = NULL;
+		pr_warn("Failed to request RX DMA channel.\n");
+	}
+
+	return ret;
+}
+
+static inline struct dma_chan *
+sdhci_extdma_get_chan(struct sdhci_extdma *dma, struct mmc_data *data)
+{
+	return data->flags & MMC_DATA_WRITE ? dma->tx_chan : dma->rx_chan;
+}
+
+static int sdhci_extdma_setup(struct sdhci_host *host, struct mmc_command *cmd)
+{
+	int ret = 0, i;
+	struct dma_async_tx_descriptor *desc;
+	struct mmc_data *data = cmd->data;
+	struct dma_chan *chan;
+	struct dma_slave_config cfg;
+
+	if (!host->mapbase)
+		return -EINVAL;
+
+	cfg.src_addr = host->mapbase + SDHCI_BUFFER;
+	cfg.dst_addr = host->mapbase + SDHCI_BUFFER;
+	cfg.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
+	cfg.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
+	cfg.src_maxburst = data->blksz / 4;
+	cfg.dst_maxburst = data->blksz / 4;
+
+	/* Sanity check: all the SG entries must be aligned by block size. */
+	for (i = 0; i < data->sg_len; i++) {
+		if ((data->sg + i)->length % data->blksz)
+			return -EINVAL;
+	}
+
+	chan = sdhci_extdma_get_chan(&host->extdma, data);
+
+	ret = dmaengine_slave_config(chan, &cfg);
+	if (ret)
+		return ret;
+
+	desc = dmaengine_prep_slave_sg(chan, data->sg, data->sg_len,
+				       mmc_get_dma_dir(data),
+				       DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+	if (!desc)
+		return -EINVAL;
+
+	desc->callback = NULL;
+	desc->callback_param = NULL;
+
+	dmaengine_submit(desc);
+
+	return 0;
+}
+
+static void sdhci_extdma_prepare_data(struct sdhci_host *host,
+				      struct mmc_command *cmd)
+{
+	host->flags |= SDHCI_REQ_USE_DMA;
+	sdhci_prepare_data(host, cmd);
+
+	if (sdhci_extdma_setup(host, cmd))
+		dev_err(mmc_dev(host->mmc), "MMC start dma failure\n");
+}
+
+static void sdhci_extdma_pre_transfer(struct sdhci_host *host,
+				      struct mmc_command *cmd)
+{
+	struct dma_chan *chan = sdhci_extdma_get_chan(&host->extdma, cmd->data);
+
+	if (cmd->opcode != MMC_SET_BLOCK_COUNT) {
+		sdhci_set_timeout(host, cmd);
+		dma_async_issue_pending(chan);
+	}
+}
+#else
+static int sdhci_extdma_init_chan(struct sdhci_host *host)
+{
+	return 0;
+}
+
+static void sdhci_extdma_prepare_data(struct sdhci_host *host,
+				      struct mmc_command *cmd)
+{
+	/* If SDHCI_EXTDMA not supported, PIO will be used */
+	sdhci_prepare_data(host, cmd);
+}
+
+static void sdhci_extdma_pre_transfer(struct sdhci_host *host,
+				      struct mmc_command *cmd)
+{}
+#endif
+
+void sdhci_switch_extdma(struct sdhci_host *host, bool en)
+{
+	host->use_extdma = en;
+}
+EXPORT_SYMBOL_GPL(sdhci_switch_extdma);
+
 void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 {
 	int flags;
@@ -1355,7 +1478,10 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 		host->data_cmd = cmd;
 	}
 
-	sdhci_prepare_data(host, cmd);
+	if (host->use_extdma)
+		sdhci_extdma_prepare_data(host, cmd);
+	else
+		sdhci_prepare_data(host, cmd);
 
 	sdhci_writel(host, cmd->arg, SDHCI_ARGUMENT);
 
@@ -1397,6 +1523,9 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 		timeout += 10 * HZ;
 	sdhci_mod_timer(host, cmd->mrq, timeout);
 
+	if (host->use_extdma)
+		sdhci_extdma_pre_transfer(host, cmd);
+
 	sdhci_writew(host, SDHCI_MAKE_CMD(cmd->opcode, flags), SDHCI_COMMAND);
 }
 EXPORT_SYMBOL_GPL(sdhci_send_command);
@@ -4133,6 +4262,12 @@ int sdhci_setup_host(struct sdhci_host *host)
 			return ret;
 	}
 
+	if (host->use_extdma) {
+		ret = sdhci_extdma_init_chan(host);
+		if (ret)
+			goto unreg;
+	}
+
 	return 0;
 
 unreg:
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index b001cf4..2d4a3ba 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -361,6 +361,12 @@ enum sdhci_cookie {
 	COOKIE_MAPPED,		/* mapped by sdhci_prepare_data() */
 };
 
+struct sdhci_extdma {
+	struct sdhci_host	*host;
+	struct dma_chan		*rx_chan;
+	struct dma_chan		*tx_chan;
+};
+
 struct sdhci_host {
 	/* Data set by hardware interface driver */
 	const char *hw_name;	/* Hardware bus name */
@@ -475,6 +481,7 @@ struct sdhci_host {
 
 	int irq;		/* Device IRQ */
 	void __iomem *ioaddr;	/* Mapped address */
+	phys_addr_t mapbase;	/* physical address base */
 	char *bounce_buffer;	/* For packing SDMA reads/writes */
 	dma_addr_t bounce_addr;
 	unsigned int bounce_buffer_size;
@@ -524,6 +531,7 @@ struct sdhci_host {
 	bool pending_reset;	/* Cmd/data reset is pending */
 	bool irq_wake_enabled;	/* IRQ wakeup is enabled */
 	bool v4_mode;		/* Host Version 4 Enable */
+	bool use_extdma;
 
 	struct mmc_request *mrqs_done[SDHCI_MAX_MRQS];	/* Requests done */
 	struct mmc_command *cmd;	/* Current command */
@@ -551,6 +559,9 @@ struct sdhci_host {
 
 	struct timer_list timer;	/* Timer for timeouts */
 	struct timer_list data_timer;	/* Timer for data timeouts */
+#if IS_ENABLED(CONFIG_MMC_SDHCI_EXTDMA)
+	struct sdhci_extdma	extdma;	/* support external DMA */
+#endif
 
 	u32 caps;		/* CAPABILITY_0 */
 	u32 caps1;		/* CAPABILITY_1 */
@@ -785,5 +796,6 @@ void sdhci_start_tuning(struct sdhci_host *host);
 void sdhci_end_tuning(struct sdhci_host *host);
 void sdhci_reset_tuning(struct sdhci_host *host);
 void sdhci_send_tuning(struct sdhci_host *host, u32 opcode);
+void sdhci_switch_extdma(struct sdhci_host *host, bool en);
 
 #endif /* __SDHCI_HW_H */
-- 
2.7.4

[PATCH RFC 2/3] mmc: sdhci-omap: Add using external dma

[Chunyan Zhang]

sdhci-omap can support both external dma controllers via dmaengine
framework as well as ADMA in which the controller acts as DMA master.

Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
---
 drivers/mmc/host/sdhci-omap.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c
index 88347ce..0a8162c 100644
--- a/drivers/mmc/host/sdhci-omap.c
+++ b/drivers/mmc/host/sdhci-omap.c
@@ -896,6 +896,7 @@ static int sdhci_omap_probe(struct platform_device *pdev)
 	const struct of_device_id *match;
 	struct sdhci_omap_data *data;
 	const struct soc_device_attribute *soc;
+	struct resource *regs;
 
 	match = of_match_device(omap_sdhci_match, dev);
 	if (!match)
@@ -908,6 +909,10 @@ static int sdhci_omap_probe(struct platform_device *pdev)
 	}
 	offset = data->offset;
 
+	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!regs)
+		return -ENXIO;
+
 	host = sdhci_pltfm_init(pdev, &sdhci_omap_pdata,
 				sizeof(*omap_host));
 	if (IS_ERR(host)) {
@@ -924,6 +929,7 @@ static int sdhci_omap_probe(struct platform_device *pdev)
 	omap_host->timing = MMC_TIMING_LEGACY;
 	omap_host->flags = data->flags;
 	host->ioaddr += offset;
+	host->mapbase = regs->start;
 
 	mmc = host->mmc;
 	sdhci_get_of_property(pdev);
@@ -991,6 +997,7 @@ static int sdhci_omap_probe(struct platform_device *pdev)
 	host->mmc_host_ops.execute_tuning = sdhci_omap_execute_tuning;
 	host->mmc_host_ops.enable_sdio_irq = sdhci_omap_enable_sdio_irq;
 
+	sdhci_switch_extdma(host, true);
 	ret = sdhci_setup_host(host);
 	if (ret)
 		goto err_put_sync;
-- 
2.7.4

[PATCH RFC 3/3] dt-bindings: sdhci-omap: Add example for using dmaengine

[Chunyan Zhang]

sdhci-omap can support both external dma controllers via dmaengine
framework as well as ADMA in which the controller acts as DMA master.

Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
---
 Documentation/devicetree/bindings/mmc/sdhci-omap.txt | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/sdhci-omap.txt b/Documentation/devicetree/bindings/mmc/sdhci-omap.txt
index 393848c..c73fd47 100644
--- a/Documentation/devicetree/bindings/mmc/sdhci-omap.txt
+++ b/Documentation/devicetree/bindings/mmc/sdhci-omap.txt
@@ -12,6 +12,11 @@ Required properties:
 		 "ddr_1_8v-rev11", "ddr_1_8v" or "ddr_3_3v", "hs200_1_8v-rev11",
 		 "hs200_1_8v",
 - pinctrl-<n> : Pinctrl states as described in bindings/pinctrl/pinctrl-bindings.txt
+- dmas:		List of DMA specifiers with the controller specific format as described
+		in the generic DMA client binding. A tx and rx specifier is required.
+- dma-names:	List of DMA request names. These strings correspond 1:1 with the
+		DMA specifiers listed in dmas. The string naming is to be "rx"
+		and "tx" for RX and TX DMA requests, respectively.
 
 Example:
 	mmc1: mmc@4809c000 {
@@ -20,4 +25,6 @@ Example:
 		ti,hwmods = "mmc1";
 		bus-width = <4>;
 		vmmc-supply = <&vmmc>; /* phandle to regulator node */
+		dmas = <&sdma 61 &sdma 62>;
+		dma-names = "tx", "rx";
 	};
-- 
2.7.4

Re: [PATCH RFC 2/3] mmc: sdhci-omap: Add using external dma

[Kishon Vijay Abraham I]

Hi,

On 05/11/18 8:46 AM, Chunyan Zhang wrote:
> sdhci-omap can support both external dma controllers via dmaengine
> framework as well as ADMA in which the controller acts as DMA master.
> 
> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
> ---
>  drivers/mmc/host/sdhci-omap.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c
> index 88347ce..0a8162c 100644
> --- a/drivers/mmc/host/sdhci-omap.c
> +++ b/drivers/mmc/host/sdhci-omap.c
> @@ -896,6 +896,7 @@ static int sdhci_omap_probe(struct platform_device *pdev)
>  	const struct of_device_id *match;
>  	struct sdhci_omap_data *data;
>  	const struct soc_device_attribute *soc;
> +	struct resource *regs;
>  
>  	match = of_match_device(omap_sdhci_match, dev);
>  	if (!match)
> @@ -908,6 +909,10 @@ static int sdhci_omap_probe(struct platform_device *pdev)
>  	}
>  	offset = data->offset;
>  
> +	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!regs)
> +		return -ENXIO;
> +
>  	host = sdhci_pltfm_init(pdev, &sdhci_omap_pdata,
>  				sizeof(*omap_host));
>  	if (IS_ERR(host)) {
> @@ -924,6 +929,7 @@ static int sdhci_omap_probe(struct platform_device *pdev)
>  	omap_host->timing = MMC_TIMING_LEGACY;
>  	omap_host->flags = data->flags;
>  	host->ioaddr += offset;
> +	host->mapbase = regs->start;
>  
>  	mmc = host->mmc;
>  	sdhci_get_of_property(pdev);
> @@ -991,6 +997,7 @@ static int sdhci_omap_probe(struct platform_device *pdev)
>  	host->mmc_host_ops.execute_tuning = sdhci_omap_execute_tuning;
>  	host->mmc_host_ops.enable_sdio_irq = sdhci_omap_enable_sdio_irq;
>  
> +	sdhci_switch_extdma(host, true);

A number of devices using sdhci-omap supports ADMA. So switching to external
DMA shouldn't be unconditional.

IMHO sdhci.c should see if the device supports ADMA or SDMA. If not it should
try switching to external DMA and if external DMA too is not supported, it
should use PIO.

Thanks
Kishon

Re: [PATCH RFC 1/3] mmc: sdhci: add support for using external DMA devices

[Arnd Bergmann]

On 11/5/18, Chunyan Zhang <zhang.chunyan@linaro.org> wrote:
> Some standard SD host controller can support both external dma
> controllers as well as ADMA in which the controller acts as
> DMA master.
>
> Currently the generic SDHCI code supports ADMA/SDMA integrated into
> 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 a generic DMA controller with SDHCI.
>
> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>

Looks good to me overall, but I think I found one small bug:


> +	dma->rx_chan = dma_request_chan(mmc->parent, "rx");
> +	if (IS_ERR(dma->rx_chan)) {
> +		ret = PTR_ERR(dma->rx_chan);
> +		if (ret == -EPROBE_DEFER && dma->tx_chan)
> +			dma_release_channel(dma->tx_chan);
> +
> +		dma->rx_chan = NULL;
> +		pr_warn("Failed to request RX DMA channel.\n");
> +	}

The error handling looks wrong here: if you get EPROBE_DEFER,
you want to skip the warning message. If you get any other error code,
you want the warning message and also the dma_release_channel()
which should be unconditional here.

        Arnd

Re: [PATCH RFC 2/3] mmc: sdhci-omap: Add using external dma

[Arnd Bergmann]

On 11/5/18, Kishon Vijay Abraham I <kishon@ti.com> wrote:
> On 05/11/18 8:46 AM, Chunyan Zhang wrote:
>>
>> +	sdhci_switch_extdma(host, true);
>
> A number of devices using sdhci-omap supports ADMA. So switching to
> external
> DMA shouldn't be unconditional.
>
> IMHO sdhci.c should see if the device supports ADMA or SDMA. If not it
> should
> try switching to external DMA and if external DMA too is not supported, it
> should use PIO.

What's the reasoning for preferring ADMA/SDMA over external DMA if
both are supported?

I'd expect that if the external DMA for some reason is worse than
ADMA, we just wouldn't list it in the DT at all, but if it's listed and
ADMA also works, then the driver should try to use it before falling
back to ADMA.

       Arnd

Re: [PATCH RFC 1/3] mmc: sdhci: add support for using external DMA devices

[Chunyan Zhang]

On Tue, 6 Nov 2018 at 19:16, Arnd Bergmann <arnd@arndb.de> wrote:
>
> On 11/5/18, Chunyan Zhang <zhang.chunyan@linaro.org> wrote:
> > Some standard SD host controller can support both external dma
> > controllers as well as ADMA in which the controller acts as
> > DMA master.
> >
> > Currently the generic SDHCI code supports ADMA/SDMA integrated into
> > 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 a generic DMA controller with SDHCI.
> >
> > Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
>
> Looks good to me overall, but I think I found one small bug:
>
>
> > +     dma->rx_chan = dma_request_chan(mmc->parent, "rx");
> > +     if (IS_ERR(dma->rx_chan)) {
> > +             ret = PTR_ERR(dma->rx_chan);
> > +             if (ret == -EPROBE_DEFER && dma->tx_chan)
> > +                     dma_release_channel(dma->tx_chan);
> > +
> > +             dma->rx_chan = NULL;
> > +             pr_warn("Failed to request RX DMA channel.\n");
> > +     }
>
> The error handling looks wrong here: if you get EPROBE_DEFER,
> you want to skip the warning message. If you get any other error code,

Right, will address.

> you want the warning message and also the dma_release_channel()
> which should be unconditional here.

Will address.

Thanks for the review,
Chunyan

>
>         Arnd

Re: [PATCH RFC 2/3] mmc: sdhci-omap: Add using external dma

[Chunyan Zhang]

On Tue, 6 Nov 2018 at 20:51, Arnd Bergmann <arnd@arndb.de> wrote:
>
> On 11/5/18, Kishon Vijay Abraham I <kishon@ti.com> wrote:
> > On 05/11/18 8:46 AM, Chunyan Zhang wrote:
> >>
> >> +    sdhci_switch_extdma(host, true);
> >
> > A number of devices using sdhci-omap supports ADMA. So switching to
> > external
> > DMA shouldn't be unconditional.
> >
> > IMHO sdhci.c should see if the device supports ADMA or SDMA. If not it
> > should
> > try switching to external DMA and if external DMA too is not supported, it
> > should use PIO.
>
> What's the reasoning for preferring ADMA/SDMA over external DMA if
> both are supported?
>
> I'd expect that if the external DMA for some reason is worse than
> ADMA, we just wouldn't list it in the DT at all, but if it's listed and
> ADMA also works, then the driver should try to use it before falling
> back to ADMA.

Thanks Arnd and Kishon, let's see what Andrian and Ulf would suggest,
and then I will address in next version of patchset.

Thanks again,
Chunyan

>
>        Arnd

Re: [PATCH RFC 1/3] mmc: sdhci: add support for using external DMA devices

[Adrian Hunter]

On 5/11/18 5:16 AM, Chunyan Zhang wrote:
> Some standard SD host controller can support both external dma
> controllers as well as ADMA in which the controller acts as
> DMA master.
> 
> Currently the generic SDHCI code supports ADMA/SDMA integrated into
> 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 a generic DMA controller with SDHCI.

I have no object to supporting external DMA, but there are a few comments below.

> 
> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
> ---
>  drivers/mmc/host/Kconfig |  13 +++++
>  drivers/mmc/host/sdhci.c | 137 ++++++++++++++++++++++++++++++++++++++++++++++-
>  drivers/mmc/host/sdhci.h |  12 +++++
>  3 files changed, 161 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 1b58739..c4745d8 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -977,3 +977,16 @@ config MMC_SDHCI_OMAP
>  	  If you have a controller with this interface, say Y or M here.
>  
>  	  If unsure, say N.
> +
> +config MMC_SDHCI_EXTDMA
> +        bool "Support external DMA in standard SD host controller"
> +	depends on MMC_SDHCI
> +	depends on DMA_ENGINE
> +	help
> +	  This is an option for using external DMA device via dmaengine
> +	  framework.
> +
> +	  If you have a controller which supports using external DMA device
> +	  for data transfer, can say Y.
> +
> +	  If unsure, say N.
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 99bdae5..ffb1d2b 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -14,6 +14,7 @@
>   */
>  
>  #include <linux/delay.h>
> +#include <linux/dmaengine.h>
>  #include <linux/ktime.h>
>  #include <linux/highmem.h>
>  #include <linux/io.h>
> @@ -1309,6 +1310,128 @@ static void sdhci_del_timer(struct sdhci_host *host, struct mmc_request *mrq)
>  		del_timer(&host->timer);
>  }
>  
> +#if IS_ENABLED(CONFIG_MMC_SDHCI_EXTDMA)
> +static int sdhci_extdma_init_chan(struct sdhci_host *host)

I would prefer "external_dma" to extdma e.g.
	CONFIG_MMC_SDHCI_EXTERNAL_DMA
	sdhci_external_dma_init
	sdhci_external_dma_channel
	sdhci_external_dma_setup
	etc


> +{
> +	int ret = 0;
> +	struct mmc_host *mmc = host->mmc;
> +	struct sdhci_extdma *dma = &host->extdma;
> +
> +	dma->tx_chan = dma_request_chan(mmc->parent, "tx");
> +	if (IS_ERR(dma->tx_chan)) {
> +		ret = PTR_ERR(dma->tx_chan);
> +		dma->tx_chan = NULL;
> +		pr_warn("Failed to request TX DMA channel.\n");
> +		return ret;
> +	}
> +
> +	dma->rx_chan = dma_request_chan(mmc->parent, "rx");
> +	if (IS_ERR(dma->rx_chan)) {
> +		ret = PTR_ERR(dma->rx_chan);
> +		if (ret == -EPROBE_DEFER && dma->tx_chan)
> +			dma_release_channel(dma->tx_chan);
> +
> +		dma->rx_chan = NULL;
> +		pr_warn("Failed to request RX DMA channel.\n");
> +	}

I guess the channels need to be released in sdhci_cleanup_host() and
sdhci_remove_host()

> +
> +	return ret;
> +}
> +
> +static inline struct dma_chan *
> +sdhci_extdma_get_chan(struct sdhci_extdma *dma, struct mmc_data *data)
> +{
> +	return data->flags & MMC_DATA_WRITE ? dma->tx_chan : dma->rx_chan;
> +}
> +
> +static int sdhci_extdma_setup(struct sdhci_host *host, struct mmc_command *cmd)
> +{
> +	int ret = 0, i;
> +	struct dma_async_tx_descriptor *desc;
> +	struct mmc_data *data = cmd->data;
> +	struct dma_chan *chan;
> +	struct dma_slave_config cfg;
> +
> +	if (!host->mapbase)
> +		return -EINVAL;
> +
> +	cfg.src_addr = host->mapbase + SDHCI_BUFFER;
> +	cfg.dst_addr = host->mapbase + SDHCI_BUFFER;
> +	cfg.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> +	cfg.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> +	cfg.src_maxburst = data->blksz / 4;
> +	cfg.dst_maxburst = data->blksz / 4;
> +
> +	/* Sanity check: all the SG entries must be aligned by block size. */
> +	for (i = 0; i < data->sg_len; i++) {
> +		if ((data->sg + i)->length % data->blksz)
> +			return -EINVAL;
> +	}
> +
> +	chan = sdhci_extdma_get_chan(&host->extdma, data);
> +
> +	ret = dmaengine_slave_config(chan, &cfg);
> +	if (ret)
> +		return ret;
> +
> +	desc = dmaengine_prep_slave_sg(chan, data->sg, data->sg_len,
> +				       mmc_get_dma_dir(data),
> +				       DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
> +	if (!desc)
> +		return -EINVAL;
> +
> +	desc->callback = NULL;
> +	desc->callback_param = NULL;
> +
> +	dmaengine_submit(desc);

Doesn't the DMA need to be cleaned up somewhere if there are transfer errors?

> +
> +	return 0;
> +}
> +
> +static void sdhci_extdma_prepare_data(struct sdhci_host *host,
> +				      struct mmc_command *cmd)
> +{
> +	host->flags |= SDHCI_REQ_USE_DMA;
> +	sdhci_prepare_data(host, cmd);
> +
> +	if (sdhci_extdma_setup(host, cmd))
> +		dev_err(mmc_dev(host->mmc), "MMC start dma failure\n");
> +}
> +
> +static void sdhci_extdma_pre_transfer(struct sdhci_host *host,
> +				      struct mmc_command *cmd)
> +{
> +	struct dma_chan *chan = sdhci_extdma_get_chan(&host->extdma, cmd->data);
> +
> +	if (cmd->opcode != MMC_SET_BLOCK_COUNT) {
> +		sdhci_set_timeout(host, cmd);
> +		dma_async_issue_pending(chan);
> +	}
> +}
> +#else
> +static int sdhci_extdma_init_chan(struct sdhci_host *host)
> +{
> +	return 0;
> +}
> +
> +static void sdhci_extdma_prepare_data(struct sdhci_host *host,
> +				      struct mmc_command *cmd)
> +{
> +	/* If SDHCI_EXTDMA not supported, PIO will be used */
> +	sdhci_prepare_data(host, cmd);
> +}
> +
> +static void sdhci_extdma_pre_transfer(struct sdhci_host *host,
> +				      struct mmc_command *cmd)
> +{}
> +#endif
> +
> +void sdhci_switch_extdma(struct sdhci_host *host, bool en)
> +{
> +	host->use_extdma = en;
> +}
> +EXPORT_SYMBOL_GPL(sdhci_switch_extdma);
> +
>  void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
>  {
>  	int flags;
> @@ -1355,7 +1478,10 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
>  		host->data_cmd = cmd;
>  	}
>  
> -	sdhci_prepare_data(host, cmd);
> +	if (host->use_extdma)
> +		sdhci_extdma_prepare_data(host, cmd);
> +	else
> +		sdhci_prepare_data(host, cmd);
>  
>  	sdhci_writel(host, cmd->arg, SDHCI_ARGUMENT);
>  
> @@ -1397,6 +1523,9 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
>  		timeout += 10 * HZ;
>  	sdhci_mod_timer(host, cmd->mrq, timeout);
>  
> +	if (host->use_extdma)
> +		sdhci_extdma_pre_transfer(host, cmd);
> +
>  	sdhci_writew(host, SDHCI_MAKE_CMD(cmd->opcode, flags), SDHCI_COMMAND);
>  }
>  EXPORT_SYMBOL_GPL(sdhci_send_command);
> @@ -4133,6 +4262,12 @@ int sdhci_setup_host(struct sdhci_host *host)
>  			return ret;
>  	}
>  
> +	if (host->use_extdma) {
> +		ret = sdhci_extdma_init_chan(host);
> +		if (ret)
> +			goto unreg;
> +	}
> +
>  	return 0;
>  
>  unreg:
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index b001cf4..2d4a3ba 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -361,6 +361,12 @@ enum sdhci_cookie {
>  	COOKIE_MAPPED,		/* mapped by sdhci_prepare_data() */
>  };
>  
> +struct sdhci_extdma {
> +	struct sdhci_host	*host;
> +	struct dma_chan		*rx_chan;
> +	struct dma_chan		*tx_chan;
> +};

Is this struct really needed?  Otherwise kernel style is not to nest
structures. i.e. just put rx/tx_chan in struct sdhci_host.

> +
>  struct sdhci_host {
>  	/* Data set by hardware interface driver */
>  	const char *hw_name;	/* Hardware bus name */
> @@ -475,6 +481,7 @@ struct sdhci_host {
>  
>  	int irq;		/* Device IRQ */
>  	void __iomem *ioaddr;	/* Mapped address */
> +	phys_addr_t mapbase;	/* physical address base */
>  	char *bounce_buffer;	/* For packing SDMA reads/writes */
>  	dma_addr_t bounce_addr;
>  	unsigned int bounce_buffer_size;
> @@ -524,6 +531,7 @@ struct sdhci_host {
>  	bool pending_reset;	/* Cmd/data reset is pending */
>  	bool irq_wake_enabled;	/* IRQ wakeup is enabled */
>  	bool v4_mode;		/* Host Version 4 Enable */
> +	bool use_extdma;
>  
>  	struct mmc_request *mrqs_done[SDHCI_MAX_MRQS];	/* Requests done */
>  	struct mmc_command *cmd;	/* Current command */
> @@ -551,6 +559,9 @@ struct sdhci_host {
>  
>  	struct timer_list timer;	/* Timer for timeouts */
>  	struct timer_list data_timer;	/* Timer for data timeouts */
> +#if IS_ENABLED(CONFIG_MMC_SDHCI_EXTDMA)
> +	struct sdhci_extdma	extdma;	/* support external DMA */
> +#endif
>  
>  	u32 caps;		/* CAPABILITY_0 */
>  	u32 caps1;		/* CAPABILITY_1 */
> @@ -785,5 +796,6 @@ void sdhci_start_tuning(struct sdhci_host *host);
>  void sdhci_end_tuning(struct sdhci_host *host);
>  void sdhci_reset_tuning(struct sdhci_host *host);
>  void sdhci_send_tuning(struct sdhci_host *host, u32 opcode);
> +void sdhci_switch_extdma(struct sdhci_host *host, bool en);
>  
>  #endif /* __SDHCI_HW_H */
> 

Re: [PATCH RFC 2/3] mmc: sdhci-omap: Add using external dma

[Kishon Vijay Abraham I]

Hi Arnd,

On 06/11/18 6:21 PM, Arnd Bergmann wrote:
> On 11/5/18, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>> On 05/11/18 8:46 AM, Chunyan Zhang wrote:
>>>
>>> +	sdhci_switch_extdma(host, true);
>>
>> A number of devices using sdhci-omap supports ADMA. So switching to
>> external
>> DMA shouldn't be unconditional.
>>
>> IMHO sdhci.c should see if the device supports ADMA or SDMA. If not it
>> should
>> try switching to external DMA and if external DMA too is not supported, it
>> should use PIO.
> 
> What's the reasoning for preferring ADMA/SDMA over external DMA if
> both are supported?

Generally from our experiments we've found ADMA gives better throughput than
DMA. I have to ascertain the actual reasons for that.
> 
> I'd expect that if the external DMA for some reason is worse than
> ADMA, we just wouldn't list it in the DT at all, but if it's listed and
> ADMA also works, then the driver should try to use it before falling
> back to ADMA.

I would agree that for newer dtbs. However if you consider omap_hsmmc, external
DMA bindings are already added in dt. If we try to switch to sdhci-omap with
the same bindings, systems with older dtbs will use external DMA and give
lesser throughput.

Thanks
Kishon

Re: [PATCH RFC 2/3] mmc: sdhci-omap: Add using external dma

[Ulf Hansson]

On 9 November 2018 at 06:27, Kishon Vijay Abraham I <kishon@ti.com> wrote:
> Hi Arnd,
>
> On 06/11/18 6:21 PM, Arnd Bergmann wrote:
>> On 11/5/18, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>> On 05/11/18 8:46 AM, Chunyan Zhang wrote:
>>>>
>>>> +   sdhci_switch_extdma(host, true);
>>>
>>> A number of devices using sdhci-omap supports ADMA. So switching to
>>> external
>>> DMA shouldn't be unconditional.
>>>
>>> IMHO sdhci.c should see if the device supports ADMA or SDMA. If not it
>>> should
>>> try switching to external DMA and if external DMA too is not supported, it
>>> should use PIO.
>>
>> What's the reasoning for preferring ADMA/SDMA over external DMA if
>> both are supported?
>
> Generally from our experiments we've found ADMA gives better throughput than
> DMA. I have to ascertain the actual reasons for that.
>>
>> I'd expect that if the external DMA for some reason is worse than
>> ADMA, we just wouldn't list it in the DT at all, but if it's listed and
>> ADMA also works, then the driver should try to use it before falling
>> back to ADMA.
>
> I would agree that for newer dtbs. However if you consider omap_hsmmc, external
> DMA bindings are already added in dt. If we try to switch to sdhci-omap with
> the same bindings, systems with older dtbs will use external DMA and give
> lesser throughput.

I was under the impression that the internal DMA (for old versions) is
rather poor.

However, in the end this is a software policy decision of what to use.
I have no strong opinion, so feel free to pick what you think makes
best sense.

Maybe Adrian also have some thoughts?

Kind regards
Uffe