[PATCH 0/7] Port am335 and am437 devices to sdhci-omap

[PATCH 0/7] Port am335 and am437 devices to sdhci-omap

[Faiz Abbas]

The following add driver patches for porting TI's am335x and am437x
devices to the sdhci-omap driver.

This involves adding external DMA support to sdhci (first 3 patches from
Chunyan) plus some miscellaneous patches to take care of deviations of
the controllers from the sdhci model.

DT changes will be posted in a separate series.

Untested versions of Chunyan's patches were posted before[1].

Tested on: am335x-evm, am335x-boneblack, am335x-sk, am437x-gpevm,
am43xx-gpevm, am437x-idk, dra7xx-evm, dra72x-evm

[1] https://patchwork.kernel.org/project/linux-mmc/list/?series=54897

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

Faiz Abbas (4):
  mmc: sdhci: Add quirk for disabling DTO during erase command
  mmc: sdhci_omap: Add DISABLE_DTO_FOR_ERASE Quirk
  dt-bindings: sdhci-omap: Add am335x and am437x specific bindings
  mmc: sdhci-omap: Add am335x and am437x specific compatibles

 .../devicetree/bindings/mmc/sdhci-omap.txt    |   9 +
 drivers/mmc/host/Kconfig                      |   4 +
 drivers/mmc/host/sdhci-omap.c                 |  27 +-
 drivers/mmc/host/sdhci.c                      | 274 +++++++++++++++++-
 drivers/mmc/host/sdhci.h                      |  10 +
 5 files changed, 319 insertions(+), 5 deletions(-)

-- 
2.19.2

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

[Faiz Abbas]

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
synchronize once at the start of each request.

Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 drivers/mmc/host/Kconfig |   3 +
 drivers/mmc/host/sdhci.c | 266 ++++++++++++++++++++++++++++++++++++++-
 drivers/mmc/host/sdhci.h |   8 ++
 3 files changed, 273 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index e26b8145efb3..333292e8ecdd 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -999,3 +999,6 @@ config MMC_SDHCI_AM654
 	  If you have a controller with this interface, say Y or M here.
 
 	  If unsure, say N.
+
+config MMC_SDHCI_EXTERNAL_DMA
+        bool
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index a22e11a65658..4a9044c06e21 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>
@@ -1118,6 +1119,226 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
 	}
 }
 
+#if IS_ENABLED(CONFIG_MMC_SDHCI_EXTERNAL_DMA)
+static int sdhci_external_dma_init(struct sdhci_host *host)
+{
+	int ret = 0;
+	struct mmc_host *mmc = host->mmc;
+
+	host->tx_chan = dma_request_chan(mmc->parent, "tx");
+	if (IS_ERR(host->tx_chan)) {
+		ret = PTR_ERR(host->tx_chan);
+		if (ret != -EPROBE_DEFER)
+			pr_warn("Failed to request TX DMA channel.\n");
+		host->tx_chan = NULL;
+		return ret;
+	}
+
+	host->rx_chan = dma_request_chan(mmc->parent, "rx");
+	if (IS_ERR(host->rx_chan)) {
+		if (host->tx_chan) {
+			dma_release_channel(host->tx_chan);
+			host->tx_chan = NULL;
+		}
+
+		ret = PTR_ERR(host->rx_chan);
+		if (ret != -EPROBE_DEFER)
+			pr_warn("Failed to request RX DMA channel.\n");
+		host->rx_chan = NULL;
+	}
+
+	return ret;
+}
+
+static inline struct dma_chan *
+sdhci_external_dma_channel(struct sdhci_host *host, struct mmc_data *data)
+{
+	return data->flags & MMC_DATA_WRITE ? host->tx_chan : host->rx_chan;
+}
+
+static int sdhci_external_dma_setup(struct sdhci_host *host,
+				    struct mmc_command *cmd)
+{
+	int ret, i;
+	struct dma_async_tx_descriptor *desc;
+	struct mmc_data *data = cmd->data;
+	struct dma_chan *chan;
+	struct dma_slave_config cfg;
+	dma_cookie_t cookie;
+	int sg_cnt;
+
+	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_external_dma_channel(host, data);
+
+	ret = dmaengine_slave_config(chan, &cfg);
+	if (ret)
+		return ret;
+
+	sg_cnt = sdhci_pre_dma_transfer(host, data, COOKIE_MAPPED);
+	if (sg_cnt <= 0)
+		return -EINVAL;
+
+	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;
+
+	cookie = dmaengine_submit(desc);
+	if (cookie < 0)
+		ret = cookie;
+
+	return ret;
+}
+
+static void sdhci_external_dma_release(struct sdhci_host *host)
+{
+	if (host->tx_chan) {
+		dma_release_channel(host->tx_chan);
+		host->tx_chan = NULL;
+	}
+
+	if (host->rx_chan) {
+		dma_release_channel(host->rx_chan);
+		host->rx_chan = NULL;
+	}
+
+	sdhci_switch_external_dma(host, false);
+}
+
+static int __sdhci_external_dma_prepare_data(struct sdhci_host *host,
+					     struct mmc_command *cmd)
+{
+	struct mmc_data *data = cmd->data;
+
+	host->data_timeout = 0;
+
+	if (sdhci_data_line_cmd(cmd))
+		sdhci_set_timeout(host, cmd);
+
+	WARN_ON(host->data);
+
+	/* Sanity checks */
+	WARN_ON(data->blksz * data->blocks > 524288);
+	WARN_ON(data->blksz > host->mmc->max_blk_size);
+	WARN_ON(data->blocks > 65535);
+
+	host->flags |= SDHCI_REQ_USE_DMA;
+	host->data = data;
+	host->data_early = 0;
+	host->data->bytes_xfered = 0;
+
+	sdhci_set_transfer_irqs(host);
+
+	/*
+	 * For Version 4.10 onwards, if v4 mode is enabled, 32-bit Block Count
+	 * can be supported, in that case 16-bit block count register must be 0.
+	 */
+	if (host->version >= SDHCI_SPEC_410 && host->v4_mode &&
+	    (host->quirks2 & SDHCI_QUIRK2_USE_32BIT_BLK_CNT)) {
+		if (sdhci_readw(host, SDHCI_BLOCK_COUNT))
+			sdhci_writew(host, 0, SDHCI_BLOCK_COUNT);
+		sdhci_writew(host, data->blocks, SDHCI_32BIT_BLK_CNT);
+	} else {
+		sdhci_writew(host, data->blocks, SDHCI_BLOCK_COUNT);
+	}
+
+	return 0;
+}
+
+static void sdhci_external_dma_prepare_data(struct sdhci_host *host,
+					    struct mmc_command *cmd)
+{
+	struct mmc_data *data = cmd->data;
+
+	if (!data)
+		return;
+
+	if (sdhci_external_dma_setup(host, cmd) ||
+	    __sdhci_external_dma_prepare_data(host, cmd)) {
+		sdhci_external_dma_release(host);
+		pr_err("%s: Cannot use external DMA, switch to the DMA/PIO which standard SDHCI provides.\n",
+		       mmc_hostname(host->mmc));
+		sdhci_prepare_data(host, cmd);
+	}
+}
+
+static void sdhci_external_dma_pre_transfer(struct sdhci_host *host,
+					    struct mmc_command *cmd)
+{
+	struct dma_chan *chan;
+
+	if (!cmd->data || cmd->opcode == MMC_SET_BLOCK_COUNT)
+		return;
+
+	sdhci_writew(host, cmd->data->blksz, SDHCI_BLOCK_SIZE);
+	chan = sdhci_external_dma_channel(host, cmd->data);
+	if (chan)
+		dma_async_issue_pending(chan);
+}
+
+static int sdhci_external_dma_cleanup(struct sdhci_host *host,
+				    struct mmc_data *data)
+{
+	struct dma_chan *chan = sdhci_external_dma_channel(host, data);
+	int ret = 0;
+
+	if (chan)
+		ret = dmaengine_terminate_async(chan);
+
+	return ret;
+}
+#else
+static int sdhci_external_dma_init(struct sdhci_host *host)
+{
+	return -EOPNOTSUPP;
+}
+
+static void sdhci_external_dma_release(struct sdhci_host *host)
+{}
+
+static void sdhci_external_dma_prepare_data(struct sdhci_host *host,
+					    struct mmc_command *cmd)
+{
+	/* If MMC_SDHCI_EXTERNAL_DMA not supported, PIO will be used */
+	sdhci_prepare_data(host, cmd);
+}
+
+static void sdhci_external_dma_pre_transfer(struct sdhci_host *host,
+					    struct mmc_command *cmd)
+{}
+
+static int sdhci_external_dma_cleanup(struct sdhci_host *host,
+				    struct mmc_data *data)
+{
+	return 0;
+}
+#endif
+
+void sdhci_switch_external_dma(struct sdhci_host *host, bool en)
+{
+	host->use_external_dma = en;
+}
+EXPORT_SYMBOL_GPL(sdhci_switch_external_dma);
+
 static inline bool sdhci_auto_cmd12(struct sdhci_host *host,
 				    struct mmc_request *mrq)
 {
@@ -1374,7 +1595,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_external_dma)
+		sdhci_external_dma_prepare_data(host, cmd);
+	else
+		sdhci_prepare_data(host, cmd);
 
 	sdhci_writel(host, cmd->arg, SDHCI_ARGUMENT);
 
@@ -1416,6 +1640,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_external_dma)
+		sdhci_external_dma_pre_transfer(host, cmd);
+
 	sdhci_writew(host, SDHCI_MAKE_CMD(cmd->opcode, flags), SDHCI_COMMAND);
 }
 EXPORT_SYMBOL_GPL(sdhci_send_command);
@@ -1781,6 +2008,11 @@ void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 
 	sdhci_led_activate(host);
 
+	if (host->use_external_dma && mrq->data) {
+		struct dma_chan *chan = sdhci_external_dma_channel(host,
+								   mrq->data);
+		dmaengine_synchronize(chan);
+	}
 	/*
 	 * Ensure we don't send the STOP for non-SET_BLOCK_COUNTED
 	 * requests if Auto-CMD12 is enabled.
@@ -2658,6 +2890,8 @@ static bool sdhci_request_done(struct sdhci_host *host)
 				dma_unmap_sg(mmc_dev(host->mmc), data->sg,
 					     data->sg_len,
 					     mmc_get_dma_dir(data));
+				if (host->use_external_dma)
+					sdhci_external_dma_cleanup(host, data);
 			}
 			data->host_cookie = COOKIE_UNMAPPED;
 		}
@@ -3692,12 +3926,15 @@ int sdhci_setup_host(struct sdhci_host *host)
 		       mmc_hostname(mmc), host->version);
 	}
 
-	if (host->quirks & SDHCI_QUIRK_FORCE_DMA)
+	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 */
+	} else {
 		host->flags |= SDHCI_USE_SDMA;
+	}
 
 	if ((host->quirks & SDHCI_QUIRK_BROKEN_DMA) &&
 		(host->flags & SDHCI_USE_SDMA)) {
@@ -3785,6 +4022,19 @@ int sdhci_setup_host(struct sdhci_host *host)
 		}
 	}
 
+	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 we use DMA, then it's up to the caller to set the DMA
 	 * mask, but PIO does not need the hw shim so we set a new
@@ -4201,6 +4451,10 @@ void sdhci_cleanup_host(struct sdhci_host *host)
 		dma_free_coherent(mmc_dev(mmc), host->align_buffer_sz +
 				  host->adma_table_sz, host->align_buffer,
 				  host->align_addr);
+
+	if (host->use_external_dma)
+		sdhci_external_dma_release(host);
+
 	host->adma_table = NULL;
 	host->align_buffer = NULL;
 }
@@ -4247,6 +4501,7 @@ int __sdhci_add_host(struct sdhci_host *host)
 
 	pr_info("%s: SDHCI controller on %s [%s] using %s\n",
 		mmc_hostname(mmc), host->hw_name, dev_name(mmc_dev(mmc)),
+		host->use_external_dma ? "External DMA" :
 		(host->flags & SDHCI_USE_ADMA) ?
 		(host->flags & SDHCI_USE_64_BIT_DMA) ? "ADMA 64-bit" : "ADMA" :
 		(host->flags & SDHCI_USE_SDMA) ? "DMA" : "PIO");
@@ -4335,6 +4590,9 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
 				  host->adma_table_sz, host->align_buffer,
 				  host->align_addr);
 
+	if (host->use_external_dma)
+		sdhci_external_dma_release(host);
+
 	host->adma_table = NULL;
 	host->align_buffer = NULL;
 }
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 6cc9a3c2ac66..7a52823ebef4 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -482,6 +482,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;
@@ -531,6 +532,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_external_dma; /* Host selects to use external DMA */
 
 	struct mmc_request *mrqs_done[SDHCI_MAX_MRQS];	/* Requests done */
 	struct mmc_command *cmd;	/* Current command */
@@ -559,6 +561,11 @@ 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_EXTERNAL_DMA)
+	struct dma_chan	*rx_chan;
+	struct dma_chan	*tx_chan;
+#endif
+
 	u32 caps;		/* CAPABILITY_0 */
 	u32 caps1;		/* CAPABILITY_1 */
 	bool read_caps;		/* Capability flags have been read */
@@ -792,5 +799,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_external_dma(struct sdhci_host *host, bool en);
 
 #endif /* __SDHCI_HW_H */
-- 
2.19.2

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

[Adrian Hunter]

On 11/01/19 1:08 PM, 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
> synchronize once at the start of each request.
> 
> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/mmc/host/Kconfig |   3 +
>  drivers/mmc/host/sdhci.c | 266 ++++++++++++++++++++++++++++++++++++++-
>  drivers/mmc/host/sdhci.h |   8 ++
>  3 files changed, 273 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index e26b8145efb3..333292e8ecdd 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -999,3 +999,6 @@ config MMC_SDHCI_AM654
>  	  If you have a controller with this interface, say Y or M here.
>  
>  	  If unsure, say N.
> +
> +config MMC_SDHCI_EXTERNAL_DMA
> +        bool
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index a22e11a65658..4a9044c06e21 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>
> @@ -1118,6 +1119,226 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
>  	}
>  }
>  
> +#if IS_ENABLED(CONFIG_MMC_SDHCI_EXTERNAL_DMA)
> +static int sdhci_external_dma_init(struct sdhci_host *host)
> +{
> +	int ret = 0;
> +	struct mmc_host *mmc = host->mmc;
> +
> +	host->tx_chan = dma_request_chan(mmc->parent, "tx");
> +	if (IS_ERR(host->tx_chan)) {
> +		ret = PTR_ERR(host->tx_chan);
> +		if (ret != -EPROBE_DEFER)
> +			pr_warn("Failed to request TX DMA channel.\n");
> +		host->tx_chan = NULL;
> +		return ret;
> +	}
> +
> +	host->rx_chan = dma_request_chan(mmc->parent, "rx");
> +	if (IS_ERR(host->rx_chan)) {
> +		if (host->tx_chan) {
> +			dma_release_channel(host->tx_chan);
> +			host->tx_chan = NULL;
> +		}
> +
> +		ret = PTR_ERR(host->rx_chan);
> +		if (ret != -EPROBE_DEFER)
> +			pr_warn("Failed to request RX DMA channel.\n");
> +		host->rx_chan = NULL;
> +	}
> +
> +	return ret;
> +}
> +
> +static inline struct dma_chan *
> +sdhci_external_dma_channel(struct sdhci_host *host, struct mmc_data *data)
> +{
> +	return data->flags & MMC_DATA_WRITE ? host->tx_chan : host->rx_chan;
> +}
> +
> +static int sdhci_external_dma_setup(struct sdhci_host *host,
> +				    struct mmc_command *cmd)
> +{
> +	int ret, i;
> +	struct dma_async_tx_descriptor *desc;
> +	struct mmc_data *data = cmd->data;
> +	struct dma_chan *chan;
> +	struct dma_slave_config cfg;
> +	dma_cookie_t cookie;
> +	int sg_cnt;
> +
> +	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_external_dma_channel(host, data);
> +
> +	ret = dmaengine_slave_config(chan, &cfg);
> +	if (ret)
> +		return ret;
> +
> +	sg_cnt = sdhci_pre_dma_transfer(host, data, COOKIE_MAPPED);
> +	if (sg_cnt <= 0)
> +		return -EINVAL;
> +
> +	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;
> +
> +	cookie = dmaengine_submit(desc);
> +	if (cookie < 0)
> +		ret = cookie;
> +
> +	return ret;
> +}
> +
> +static void sdhci_external_dma_release(struct sdhci_host *host)
> +{
> +	if (host->tx_chan) {
> +		dma_release_channel(host->tx_chan);
> +		host->tx_chan = NULL;
> +	}
> +
> +	if (host->rx_chan) {
> +		dma_release_channel(host->rx_chan);
> +		host->rx_chan = NULL;
> +	}
> +
> +	sdhci_switch_external_dma(host, false);
> +}
> +
> +static int __sdhci_external_dma_prepare_data(struct sdhci_host *host,
> +					     struct mmc_command *cmd)
> +{
> +	struct mmc_data *data = cmd->data;
> +
> +	host->data_timeout = 0;
> +
> +	if (sdhci_data_line_cmd(cmd))
> +		sdhci_set_timeout(host, cmd);
> +
> +	WARN_ON(host->data);
> +
> +	/* Sanity checks */
> +	WARN_ON(data->blksz * data->blocks > 524288);
> +	WARN_ON(data->blksz > host->mmc->max_blk_size);
> +	WARN_ON(data->blocks > 65535);
> +
> +	host->flags |= SDHCI_REQ_USE_DMA;
> +	host->data = data;
> +	host->data_early = 0;
> +	host->data->bytes_xfered = 0;
> +
> +	sdhci_set_transfer_irqs(host);
> +
> +	/*
> +	 * For Version 4.10 onwards, if v4 mode is enabled, 32-bit Block Count
> +	 * can be supported, in that case 16-bit block count register must be 0.
> +	 */
> +	if (host->version >= SDHCI_SPEC_410 && host->v4_mode &&
> +	    (host->quirks2 & SDHCI_QUIRK2_USE_32BIT_BLK_CNT)) {
> +		if (sdhci_readw(host, SDHCI_BLOCK_COUNT))
> +			sdhci_writew(host, 0, SDHCI_BLOCK_COUNT);
> +		sdhci_writew(host, data->blocks, SDHCI_32BIT_BLK_CNT);
> +	} else {
> +		sdhci_writew(host, data->blocks, SDHCI_BLOCK_COUNT);
> +	}
> +
> +	return 0;
> +}
> +
> +static void sdhci_external_dma_prepare_data(struct sdhci_host *host,
> +					    struct mmc_command *cmd)
> +{
> +	struct mmc_data *data = cmd->data;
> +
> +	if (!data)
> +		return;
> +
> +	if (sdhci_external_dma_setup(host, cmd) ||
> +	    __sdhci_external_dma_prepare_data(host, cmd)) {
> +		sdhci_external_dma_release(host);
> +		pr_err("%s: Cannot use external DMA, switch to the DMA/PIO which standard SDHCI provides.\n",
> +		       mmc_hostname(host->mmc));
> +		sdhci_prepare_data(host, cmd);
> +	}
> +}
> +
> +static void sdhci_external_dma_pre_transfer(struct sdhci_host *host,
> +					    struct mmc_command *cmd)
> +{
> +	struct dma_chan *chan;
> +
> +	if (!cmd->data || cmd->opcode == MMC_SET_BLOCK_COUNT)
> +		return;
> +
> +	sdhci_writew(host, cmd->data->blksz, SDHCI_BLOCK_SIZE);
> +	chan = sdhci_external_dma_channel(host, cmd->data);
> +	if (chan)
> +		dma_async_issue_pending(chan);
> +}
> +
> +static int sdhci_external_dma_cleanup(struct sdhci_host *host,
> +				    struct mmc_data *data)
> +{
> +	struct dma_chan *chan = sdhci_external_dma_channel(host, data);
> +	int ret = 0;
> +
> +	if (chan)
> +		ret = dmaengine_terminate_async(chan);
> +
> +	return ret;
> +}
> +#else
> +static int sdhci_external_dma_init(struct sdhci_host *host)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static void sdhci_external_dma_release(struct sdhci_host *host)
> +{}
> +
> +static void sdhci_external_dma_prepare_data(struct sdhci_host *host,
> +					    struct mmc_command *cmd)
> +{
> +	/* If MMC_SDHCI_EXTERNAL_DMA not supported, PIO will be used */
> +	sdhci_prepare_data(host, cmd);
> +}
> +
> +static void sdhci_external_dma_pre_transfer(struct sdhci_host *host,
> +					    struct mmc_command *cmd)
> +{}
> +
> +static int sdhci_external_dma_cleanup(struct sdhci_host *host,
> +				    struct mmc_data *data)
> +{
> +	return 0;
> +}
> +#endif
> +
> +void sdhci_switch_external_dma(struct sdhci_host *host, bool en)
> +{
> +	host->use_external_dma = en;
> +}
> +EXPORT_SYMBOL_GPL(sdhci_switch_external_dma);
> +
>  static inline bool sdhci_auto_cmd12(struct sdhci_host *host,
>  				    struct mmc_request *mrq)
>  {
> @@ -1374,7 +1595,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_external_dma)
> +		sdhci_external_dma_prepare_data(host, cmd);
> +	else
> +		sdhci_prepare_data(host, cmd);
>  
>  	sdhci_writel(host, cmd->arg, SDHCI_ARGUMENT);
>  
> @@ -1416,6 +1640,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_external_dma)
> +		sdhci_external_dma_pre_transfer(host, cmd);
> +
>  	sdhci_writew(host, SDHCI_MAKE_CMD(cmd->opcode, flags), SDHCI_COMMAND);
>  }
>  EXPORT_SYMBOL_GPL(sdhci_send_command);
> @@ -1781,6 +2008,11 @@ void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>  
>  	sdhci_led_activate(host);
>  
> +	if (host->use_external_dma && mrq->data) {
> +		struct dma_chan *chan = sdhci_external_dma_channel(host,
> +								   mrq->data);
> +		dmaengine_synchronize(chan);
> +	}
>  	/*
>  	 * Ensure we don't send the STOP for non-SET_BLOCK_COUNTED
>  	 * requests if Auto-CMD12 is enabled.
> @@ -2658,6 +2890,8 @@ static bool sdhci_request_done(struct sdhci_host *host)
>  				dma_unmap_sg(mmc_dev(host->mmc), data->sg,
>  					     data->sg_len,
>  					     mmc_get_dma_dir(data));
> +				if (host->use_external_dma)
> +					sdhci_external_dma_cleanup(host, data);
>  			}
>  			data->host_cookie = COOKIE_UNMAPPED;
>  		}
> @@ -3692,12 +3926,15 @@ int sdhci_setup_host(struct sdhci_host *host)
>  		       mmc_hostname(mmc), host->version);
>  	}
>  
> -	if (host->quirks & SDHCI_QUIRK_FORCE_DMA)
> +	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 */
> +	} else {
>  		host->flags |= SDHCI_USE_SDMA;
> +	}
>  
>  	if ((host->quirks & SDHCI_QUIRK_BROKEN_DMA) &&
>  		(host->flags & SDHCI_USE_SDMA)) {
> @@ -3785,6 +4022,19 @@ int sdhci_setup_host(struct sdhci_host *host)
>  		}
>  	}
>  
> +	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 we use DMA, then it's up to the caller to set the DMA
>  	 * mask, but PIO does not need the hw shim so we set a new
> @@ -4201,6 +4451,10 @@ void sdhci_cleanup_host(struct sdhci_host *host)
>  		dma_free_coherent(mmc_dev(mmc), host->align_buffer_sz +
>  				  host->adma_table_sz, host->align_buffer,
>  				  host->align_addr);
> +
> +	if (host->use_external_dma)
> +		sdhci_external_dma_release(host);
> +
>  	host->adma_table = NULL;
>  	host->align_buffer = NULL;
>  }
> @@ -4247,6 +4501,7 @@ int __sdhci_add_host(struct sdhci_host *host)
>  
>  	pr_info("%s: SDHCI controller on %s [%s] using %s\n",
>  		mmc_hostname(mmc), host->hw_name, dev_name(mmc_dev(mmc)),
> +		host->use_external_dma ? "External DMA" :
>  		(host->flags & SDHCI_USE_ADMA) ?
>  		(host->flags & SDHCI_USE_64_BIT_DMA) ? "ADMA 64-bit" : "ADMA" :
>  		(host->flags & SDHCI_USE_SDMA) ? "DMA" : "PIO");
> @@ -4335,6 +4590,9 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
>  				  host->adma_table_sz, host->align_buffer,
>  				  host->align_addr);
>  
> +	if (host->use_external_dma)
> +		sdhci_external_dma_release(host);
> +
>  	host->adma_table = NULL;
>  	host->align_buffer = NULL;
>  }
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 6cc9a3c2ac66..7a52823ebef4 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -482,6 +482,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;
> @@ -531,6 +532,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_external_dma; /* Host selects to use external DMA */
>  
>  	struct mmc_request *mrqs_done[SDHCI_MAX_MRQS];	/* Requests done */
>  	struct mmc_command *cmd;	/* Current command */
> @@ -559,6 +561,11 @@ 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_EXTERNAL_DMA)
> +	struct dma_chan	*rx_chan;
> +	struct dma_chan	*tx_chan;
> +#endif
> +
>  	u32 caps;		/* CAPABILITY_0 */
>  	u32 caps1;		/* CAPABILITY_1 */
>  	bool read_caps;		/* Capability flags have been read */
> @@ -792,5 +799,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_external_dma(struct sdhci_host *host, bool en);
>  
>  #endif /* __SDHCI_HW_H */
> 

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

[Adrian Hunter]

On 11/01/19 1:08 PM, 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
> synchronize once at the start of each request.

Sorry for the slow reply, but I do have some concerns.  Please see the comments.

> 
> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
> ---
>  drivers/mmc/host/Kconfig |   3 +
>  drivers/mmc/host/sdhci.c | 266 ++++++++++++++++++++++++++++++++++++++-
>  drivers/mmc/host/sdhci.h |   8 ++
>  3 files changed, 273 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index e26b8145efb3..333292e8ecdd 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -999,3 +999,6 @@ config MMC_SDHCI_AM654
>  	  If you have a controller with this interface, say Y or M here.
>  
>  	  If unsure, say N.
> +
> +config MMC_SDHCI_EXTERNAL_DMA
> +        bool
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index a22e11a65658..4a9044c06e21 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>
> @@ -1118,6 +1119,226 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
>  	}
>  }
>  
> +#if IS_ENABLED(CONFIG_MMC_SDHCI_EXTERNAL_DMA)
> +static int sdhci_external_dma_init(struct sdhci_host *host)
> +{
> +	int ret = 0;
> +	struct mmc_host *mmc = host->mmc;
> +
> +	host->tx_chan = dma_request_chan(mmc->parent, "tx");
> +	if (IS_ERR(host->tx_chan)) {
> +		ret = PTR_ERR(host->tx_chan);
> +		if (ret != -EPROBE_DEFER)
> +			pr_warn("Failed to request TX DMA channel.\n");
> +		host->tx_chan = NULL;
> +		return ret;
> +	}
> +
> +	host->rx_chan = dma_request_chan(mmc->parent, "rx");
> +	if (IS_ERR(host->rx_chan)) {
> +		if (host->tx_chan) {
> +			dma_release_channel(host->tx_chan);
> +			host->tx_chan = NULL;
> +		}
> +
> +		ret = PTR_ERR(host->rx_chan);
> +		if (ret != -EPROBE_DEFER)
> +			pr_warn("Failed to request RX DMA channel.\n");
> +		host->rx_chan = NULL;
> +	}
> +
> +	return ret;
> +}
> +
> +static inline struct dma_chan *
> +sdhci_external_dma_channel(struct sdhci_host *host, struct mmc_data *data)
> +{
> +	return data->flags & MMC_DATA_WRITE ? host->tx_chan : host->rx_chan;
> +}
> +
> +static int sdhci_external_dma_setup(struct sdhci_host *host,
> +				    struct mmc_command *cmd)
> +{
> +	int ret, i;
> +	struct dma_async_tx_descriptor *desc;
> +	struct mmc_data *data = cmd->data;
> +	struct dma_chan *chan;
> +	struct dma_slave_config cfg;
> +	dma_cookie_t cookie;
> +	int sg_cnt;
> +
> +	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_external_dma_channel(host, data);
> +
> +	ret = dmaengine_slave_config(chan, &cfg);
> +	if (ret)
> +		return ret;
> +
> +	sg_cnt = sdhci_pre_dma_transfer(host, data, COOKIE_MAPPED);
> +	if (sg_cnt <= 0)
> +		return -EINVAL;
> +
> +	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;
> +
> +	cookie = dmaengine_submit(desc);
> +	if (cookie < 0)
> +		ret = cookie;
> +
> +	return ret;
> +}
> +
> +static void sdhci_external_dma_release(struct sdhci_host *host)
> +{
> +	if (host->tx_chan) {
> +		dma_release_channel(host->tx_chan);
> +		host->tx_chan = NULL;
> +	}
> +
> +	if (host->rx_chan) {
> +		dma_release_channel(host->rx_chan);
> +		host->rx_chan = NULL;
> +	}
> +
> +	sdhci_switch_external_dma(host, false);
> +}
> +
> +static int __sdhci_external_dma_prepare_data(struct sdhci_host *host,
> +					     struct mmc_command *cmd)
> +{
> +	struct mmc_data *data = cmd->data;
> +
> +	host->data_timeout = 0;
> +
> +	if (sdhci_data_line_cmd(cmd))
> +		sdhci_set_timeout(host, cmd);
> +
> +	WARN_ON(host->data);
> +
> +	/* Sanity checks */
> +	WARN_ON(data->blksz * data->blocks > 524288);
> +	WARN_ON(data->blksz > host->mmc->max_blk_size);
> +	WARN_ON(data->blocks > 65535);
> +
> +	host->flags |= SDHCI_REQ_USE_DMA;
> +	host->data = data;
> +	host->data_early = 0;
> +	host->data->bytes_xfered = 0;
> +
> +	sdhci_set_transfer_irqs(host);
> +
> +	/*
> +	 * For Version 4.10 onwards, if v4 mode is enabled, 32-bit Block Count
> +	 * can be supported, in that case 16-bit block count register must be 0.
> +	 */
> +	if (host->version >= SDHCI_SPEC_410 && host->v4_mode &&
> +	    (host->quirks2 & SDHCI_QUIRK2_USE_32BIT_BLK_CNT)) {
> +		if (sdhci_readw(host, SDHCI_BLOCK_COUNT))
> +			sdhci_writew(host, 0, SDHCI_BLOCK_COUNT);
> +		sdhci_writew(host, data->blocks, SDHCI_32BIT_BLK_CNT);
> +	} else {
> +		sdhci_writew(host, data->blocks, SDHCI_BLOCK_COUNT);
> +	}

It is probably worth factoring out the code that is shared with
sdhci_prepare_data() where possible.

> +
> +	return 0;
> +}
> +
> +static void sdhci_external_dma_prepare_data(struct sdhci_host *host,
> +					    struct mmc_command *cmd)
> +{
> +	struct mmc_data *data = cmd->data;
> +
> +	if (!data)
> +		return;

Even in the !data case, we still need to set up a timeout for commands with
busy waiting.  I suggest checking the !data case before calling
sdhci_external_dma_prepare_data()

> +
> +	if (sdhci_external_dma_setup(host, cmd) ||
> +	    __sdhci_external_dma_prepare_data(host, cmd)) {
> +		sdhci_external_dma_release(host);
> +		pr_err("%s: Cannot use external DMA, switch to the DMA/PIO which standard SDHCI provides.\n",
> +		       mmc_hostname(host->mmc));
> +		sdhci_prepare_data(host, cmd);
> +	}
> +}
> +
> +static void sdhci_external_dma_pre_transfer(struct sdhci_host *host,
> +					    struct mmc_command *cmd)
> +{
> +	struct dma_chan *chan;
> +
> +	if (!cmd->data || cmd->opcode == MMC_SET_BLOCK_COUNT)

MMC_SET_BLOCK_COUNT never has cmd->data and so does not need to be checked.

> +		return;
> +
> +	sdhci_writew(host, cmd->data->blksz, SDHCI_BLOCK_SIZE);

Block size is set in __sdhci_external_dma_prepare_data() so does it need to
be set here also.

> +	chan = sdhci_external_dma_channel(host, cmd->data);
> +	if (chan)
> +		dma_async_issue_pending(chan);
> +}
> +
> +static int sdhci_external_dma_cleanup(struct sdhci_host *host,
> +				    struct mmc_data *data)

Please align parameters with open parenthesis

> +{
> +	struct dma_chan *chan = sdhci_external_dma_channel(host, data);
> +	int ret = 0;
> +
> +	if (chan)
> +		ret = dmaengine_terminate_async(chan);
> +
> +	return ret;
> +}
> +#else
> +static int sdhci_external_dma_init(struct sdhci_host *host)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static void sdhci_external_dma_release(struct sdhci_host *host)
> +{}
> +
> +static void sdhci_external_dma_prepare_data(struct sdhci_host *host,
> +					    struct mmc_command *cmd)
> +{
> +	/* If MMC_SDHCI_EXTERNAL_DMA not supported, PIO will be used */
> +	sdhci_prepare_data(host, cmd);
> +}
> +
> +static void sdhci_external_dma_pre_transfer(struct sdhci_host *host,
> +					    struct mmc_command *cmd)
> +{}
> +
> +static int sdhci_external_dma_cleanup(struct sdhci_host *host,
> +				    struct mmc_data *data)

Please align parameters with open parenthesis

> +{
> +	return 0;
> +}
> +#endif
> +
> +void sdhci_switch_external_dma(struct sdhci_host *host, bool en)
> +{
> +	host->use_external_dma = en;
> +}
> +EXPORT_SYMBOL_GPL(sdhci_switch_external_dma);
> +
>  static inline bool sdhci_auto_cmd12(struct sdhci_host *host,
>  				    struct mmc_request *mrq)
>  {
> @@ -1374,7 +1595,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_external_dma)

As mentioned above wrt sdhci_external_dma_prepare_data():

	if (host->use_external_dma && cmd->data)

> +		sdhci_external_dma_prepare_data(host, cmd);
> +	else
> +		sdhci_prepare_data(host, cmd);
>  
>  	sdhci_writel(host, cmd->arg, SDHCI_ARGUMENT);
>  
> @@ -1416,6 +1640,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_external_dma)
> +		sdhci_external_dma_pre_transfer(host, cmd);

Why is sdhci_external_dma_pre_transfer() needed here - couldn't it be done
in sdhci_external_dma_prepare_data()?

> +
>  	sdhci_writew(host, SDHCI_MAKE_CMD(cmd->opcode, flags), SDHCI_COMMAND);
>  }
>  EXPORT_SYMBOL_GPL(sdhci_send_command);
> @@ -1781,6 +2008,11 @@ void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>  
>  	sdhci_led_activate(host);
>  
> +	if (host->use_external_dma && mrq->data) {
> +		struct dma_chan *chan = sdhci_external_dma_channel(host,
> +								   mrq->data);

sdhci_external_dma_channel is not declared if
!IS_ENABLED(CONFIG_MMC_SDHCI_EXTERNAL_DMA)

> +		dmaengine_synchronize(chan);

So this is to cover for using dmaengine_terminate_async()?

> +	}
>  	/*
>  	 * Ensure we don't send the STOP for non-SET_BLOCK_COUNTED
>  	 * requests if Auto-CMD12 is enabled.
> @@ -2658,6 +2890,8 @@ static bool sdhci_request_done(struct sdhci_host *host)
>  				dma_unmap_sg(mmc_dev(host->mmc), data->sg,
>  					     data->sg_len,
>  					     mmc_get_dma_dir(data));
> +				if (host->use_external_dma)
> +					sdhci_external_dma_cleanup(host, data);

Is sdhci_external_dma_cleanup() only needed in the error case?

The DMA must be stopped before the memory is unmapped and potentially freed.

Isn't the DMA cleanup also needed in the bounce buffer case?

Isn't the DMA cleanup also needed in the COOKIE_PRE_MAPPED case?

dmaengine_terminate_async() doesn't stop the DMA but
dmaengine_terminate_sync() is not atomic, which looks like a problem.

Perhaps you look at scheduling some work for the external dma error case
instead of calling __sdhci_finish_mrq()?  Then the work can do the
dmaengine_terminate_sync() and call __sdhci_finish_mrq().

>  			}
>  			data->host_cookie = COOKIE_UNMAPPED;
>  		}
> @@ -3692,12 +3926,15 @@ int sdhci_setup_host(struct sdhci_host *host)
>  		       mmc_hostname(mmc), host->version);
>  	}
>  
> -	if (host->quirks & SDHCI_QUIRK_FORCE_DMA)
> +	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 */
> +	} else {
>  		host->flags |= SDHCI_USE_SDMA;
> +	}

These if-statements are about setting SDHCI_USE_SDMA but why is a change
needed for the host->use_external_dma case?

>  
>  	if ((host->quirks & SDHCI_QUIRK_BROKEN_DMA) &&
>  		(host->flags & SDHCI_USE_SDMA)) {
> @@ -3785,6 +4022,19 @@ int sdhci_setup_host(struct sdhci_host *host)
>  		}
>  	}
>  
> +	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 we use DMA, then it's up to the caller to set the DMA
>  	 * mask, but PIO does not need the hw shim so we set a new
> @@ -4201,6 +4451,10 @@ void sdhci_cleanup_host(struct sdhci_host *host)
>  		dma_free_coherent(mmc_dev(mmc), host->align_buffer_sz +
>  				  host->adma_table_sz, host->align_buffer,
>  				  host->align_addr);
> +
> +	if (host->use_external_dma)
> +		sdhci_external_dma_release(host);
> +
>  	host->adma_table = NULL;
>  	host->align_buffer = NULL;
>  }
> @@ -4247,6 +4501,7 @@ int __sdhci_add_host(struct sdhci_host *host)
>  
>  	pr_info("%s: SDHCI controller on %s [%s] using %s\n",
>  		mmc_hostname(mmc), host->hw_name, dev_name(mmc_dev(mmc)),
> +		host->use_external_dma ? "External DMA" :
>  		(host->flags & SDHCI_USE_ADMA) ?
>  		(host->flags & SDHCI_USE_64_BIT_DMA) ? "ADMA 64-bit" : "ADMA" :
>  		(host->flags & SDHCI_USE_SDMA) ? "DMA" : "PIO");
> @@ -4335,6 +4590,9 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
>  				  host->adma_table_sz, host->align_buffer,
>  				  host->align_addr);
>  
> +	if (host->use_external_dma)
> +		sdhci_external_dma_release(host);
> +
>  	host->adma_table = NULL;
>  	host->align_buffer = NULL;
>  }
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 6cc9a3c2ac66..7a52823ebef4 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -482,6 +482,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;
> @@ -531,6 +532,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_external_dma; /* Host selects to use external DMA */

Please align /**/ with above i.e. use tab

>  
>  	struct mmc_request *mrqs_done[SDHCI_MAX_MRQS];	/* Requests done */
>  	struct mmc_command *cmd;	/* Current command */
> @@ -559,6 +561,11 @@ 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_EXTERNAL_DMA)
> +	struct dma_chan	*rx_chan;
> +	struct dma_chan	*tx_chan;
> +#endif
> +
>  	u32 caps;		/* CAPABILITY_0 */
>  	u32 caps1;		/* CAPABILITY_1 */
>  	bool read_caps;		/* Capability flags have been read */
> @@ -792,5 +799,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_external_dma(struct sdhci_host *host, bool en);
>  
>  #endif /* __SDHCI_HW_H */
> 

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

[Chunyan Zhang]

On Thu, 24 Jan 2019 at 19:41, Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 11/01/19 1:08 PM, 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
> > synchronize once at the start of each request.
>
> Sorry for the slow reply, but I do have some concerns.  Please see the comments.
>
> >
> > Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
> > Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
> > ---
> >  drivers/mmc/host/Kconfig |   3 +
> >  drivers/mmc/host/sdhci.c | 266 ++++++++++++++++++++++++++++++++++++++-
> >  drivers/mmc/host/sdhci.h |   8 ++
> >  3 files changed, 273 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> > index e26b8145efb3..333292e8ecdd 100644
> > --- a/drivers/mmc/host/Kconfig
> > +++ b/drivers/mmc/host/Kconfig
> > @@ -999,3 +999,6 @@ config MMC_SDHCI_AM654
> >         If you have a controller with this interface, say Y or M here.
> >
> >         If unsure, say N.
> > +
> > +config MMC_SDHCI_EXTERNAL_DMA
> > +        bool
> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > index a22e11a65658..4a9044c06e21 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>
> > @@ -1118,6 +1119,226 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
> >       }
> >  }
> >
> > +#if IS_ENABLED(CONFIG_MMC_SDHCI_EXTERNAL_DMA)
> > +static int sdhci_external_dma_init(struct sdhci_host *host)
> > +{
> > +     int ret = 0;
> > +     struct mmc_host *mmc = host->mmc;
> > +
> > +     host->tx_chan = dma_request_chan(mmc->parent, "tx");
> > +     if (IS_ERR(host->tx_chan)) {
> > +             ret = PTR_ERR(host->tx_chan);
> > +             if (ret != -EPROBE_DEFER)
> > +                     pr_warn("Failed to request TX DMA channel.\n");
> > +             host->tx_chan = NULL;
> > +             return ret;
> > +     }
> > +
> > +     host->rx_chan = dma_request_chan(mmc->parent, "rx");
> > +     if (IS_ERR(host->rx_chan)) {
> > +             if (host->tx_chan) {
> > +                     dma_release_channel(host->tx_chan);
> > +                     host->tx_chan = NULL;
> > +             }
> > +
> > +             ret = PTR_ERR(host->rx_chan);
> > +             if (ret != -EPROBE_DEFER)
> > +                     pr_warn("Failed to request RX DMA channel.\n");
> > +             host->rx_chan = NULL;
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> > +static inline struct dma_chan *
> > +sdhci_external_dma_channel(struct sdhci_host *host, struct mmc_data *data)
> > +{
> > +     return data->flags & MMC_DATA_WRITE ? host->tx_chan : host->rx_chan;
> > +}
> > +
> > +static int sdhci_external_dma_setup(struct sdhci_host *host,
> > +                                 struct mmc_command *cmd)
> > +{
> > +     int ret, i;
> > +     struct dma_async_tx_descriptor *desc;
> > +     struct mmc_data *data = cmd->data;
> > +     struct dma_chan *chan;
> > +     struct dma_slave_config cfg;
> > +     dma_cookie_t cookie;
> > +     int sg_cnt;
> > +
> > +     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_external_dma_channel(host, data);
> > +
> > +     ret = dmaengine_slave_config(chan, &cfg);
> > +     if (ret)
> > +             return ret;
> > +
> > +     sg_cnt = sdhci_pre_dma_transfer(host, data, COOKIE_MAPPED);
> > +     if (sg_cnt <= 0)
> > +             return -EINVAL;
> > +
> > +     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;
> > +
> > +     cookie = dmaengine_submit(desc);
> > +     if (cookie < 0)
> > +             ret = cookie;
> > +
> > +     return ret;
> > +}
> > +
> > +static void sdhci_external_dma_release(struct sdhci_host *host)
> > +{
> > +     if (host->tx_chan) {
> > +             dma_release_channel(host->tx_chan);
> > +             host->tx_chan = NULL;
> > +     }
> > +
> > +     if (host->rx_chan) {
> > +             dma_release_channel(host->rx_chan);
> > +             host->rx_chan = NULL;
> > +     }
> > +
> > +     sdhci_switch_external_dma(host, false);
> > +}
> > +
> > +static int __sdhci_external_dma_prepare_data(struct sdhci_host *host,
> > +                                          struct mmc_command *cmd)
> > +{
> > +     struct mmc_data *data = cmd->data;
> > +
> > +     host->data_timeout = 0;
> > +
> > +     if (sdhci_data_line_cmd(cmd))
> > +             sdhci_set_timeout(host, cmd);
> > +
> > +     WARN_ON(host->data);
> > +
> > +     /* Sanity checks */
> > +     WARN_ON(data->blksz * data->blocks > 524288);
> > +     WARN_ON(data->blksz > host->mmc->max_blk_size);
> > +     WARN_ON(data->blocks > 65535);
> > +
> > +     host->flags |= SDHCI_REQ_USE_DMA;
> > +     host->data = data;
> > +     host->data_early = 0;
> > +     host->data->bytes_xfered = 0;
> > +
> > +     sdhci_set_transfer_irqs(host);
> > +
> > +     /*
> > +      * For Version 4.10 onwards, if v4 mode is enabled, 32-bit Block Count
> > +      * can be supported, in that case 16-bit block count register must be 0.
> > +      */
> > +     if (host->version >= SDHCI_SPEC_410 && host->v4_mode &&
> > +         (host->quirks2 & SDHCI_QUIRK2_USE_32BIT_BLK_CNT)) {
> > +             if (sdhci_readw(host, SDHCI_BLOCK_COUNT))
> > +                     sdhci_writew(host, 0, SDHCI_BLOCK_COUNT);
> > +             sdhci_writew(host, data->blocks, SDHCI_32BIT_BLK_CNT);
> > +     } else {
> > +             sdhci_writew(host, data->blocks, SDHCI_BLOCK_COUNT);
> > +     }
>
> It is probably worth factoring out the code that is shared with
> sdhci_prepare_data() where possible.
>
> > +
> > +     return 0;
> > +}
> > +
> > +static void sdhci_external_dma_prepare_data(struct sdhci_host *host,
> > +                                         struct mmc_command *cmd)
> > +{
> > +     struct mmc_data *data = cmd->data;
> > +
> > +     if (!data)
> > +             return;
>
> Even in the !data case, we still need to set up a timeout for commands with
> busy waiting.  I suggest checking the !data case before calling
> sdhci_external_dma_prepare_data()

Ok.

>
> > +
> > +     if (sdhci_external_dma_setup(host, cmd) ||
> > +         __sdhci_external_dma_prepare_data(host, cmd)) {
> > +             sdhci_external_dma_release(host);
> > +             pr_err("%s: Cannot use external DMA, switch to the DMA/PIO which standard SDHCI provides.\n",
> > +                    mmc_hostname(host->mmc));
> > +             sdhci_prepare_data(host, cmd);
> > +     }
> > +}
> > +
> > +static void sdhci_external_dma_pre_transfer(struct sdhci_host *host,
> > +                                         struct mmc_command *cmd)
> > +{
> > +     struct dma_chan *chan;
> > +
> > +     if (!cmd->data || cmd->opcode == MMC_SET_BLOCK_COUNT)
>
> MMC_SET_BLOCK_COUNT never has cmd->data and so does not need to be checked.

Ok.

>
> > +             return;
> > +
> > +     sdhci_writew(host, cmd->data->blksz, SDHCI_BLOCK_SIZE);
>
> Block size is set in __sdhci_external_dma_prepare_data() so does it need to
> be set here also.

Ok.

>
> > +     chan = sdhci_external_dma_channel(host, cmd->data);
> > +     if (chan)
> > +             dma_async_issue_pending(chan);
> > +}
> > +
> > +static int sdhci_external_dma_cleanup(struct sdhci_host *host,
> > +                                 struct mmc_data *data)
>
> Please align parameters with open parenthesis
>
> > +{
> > +     struct dma_chan *chan = sdhci_external_dma_channel(host, data);
> > +     int ret = 0;
> > +
> > +     if (chan)
> > +             ret = dmaengine_terminate_async(chan);
> > +
> > +     return ret;
> > +}
> > +#else
> > +static int sdhci_external_dma_init(struct sdhci_host *host)
> > +{
> > +     return -EOPNOTSUPP;
> > +}
> > +
> > +static void sdhci_external_dma_release(struct sdhci_host *host)
> > +{}
> > +
> > +static void sdhci_external_dma_prepare_data(struct sdhci_host *host,
> > +                                         struct mmc_command *cmd)
> > +{
> > +     /* If MMC_SDHCI_EXTERNAL_DMA not supported, PIO will be used */
> > +     sdhci_prepare_data(host, cmd);
> > +}
> > +
> > +static void sdhci_external_dma_pre_transfer(struct sdhci_host *host,
> > +                                         struct mmc_command *cmd)
> > +{}
> > +
> > +static int sdhci_external_dma_cleanup(struct sdhci_host *host,
> > +                                 struct mmc_data *data)
>
> Please align parameters with open parenthesis
>
> > +{
> > +     return 0;
> > +}
> > +#endif
> > +
> > +void sdhci_switch_external_dma(struct sdhci_host *host, bool en)
> > +{
> > +     host->use_external_dma = en;
> > +}
> > +EXPORT_SYMBOL_GPL(sdhci_switch_external_dma);
> > +
> >  static inline bool sdhci_auto_cmd12(struct sdhci_host *host,
> >                                   struct mmc_request *mrq)
> >  {
> > @@ -1374,7 +1595,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_external_dma)
>
> As mentioned above wrt sdhci_external_dma_prepare_data():
>
>         if (host->use_external_dma && cmd->data)
>

Maybe we should move checking the !data case out of both
_prepare_data, and add it to sdhci_send_command() before calling
_prepare_data(), that's saying we can do set up a timeout in
sdhci_send_command().

> > +             sdhci_external_dma_prepare_data(host, cmd);
> > +     else
> > +             sdhci_prepare_data(host, cmd);
> >
> >       sdhci_writel(host, cmd->arg, SDHCI_ARGUMENT);
> >
> > @@ -1416,6 +1640,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_external_dma)
> > +             sdhci_external_dma_pre_transfer(host, cmd);
>
> Why is sdhci_external_dma_pre_transfer() needed here - couldn't it be done
> in sdhci_external_dma_prepare_data()?
>

I'm not sure dma_async_issue_pending() can be done so early in
sdhci_external_dma_prepare_data().

> > +
> >       sdhci_writew(host, SDHCI_MAKE_CMD(cmd->opcode, flags), SDHCI_COMMAND);
> >  }
> >  EXPORT_SYMBOL_GPL(sdhci_send_command);
> > @@ -1781,6 +2008,11 @@ void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
> >
> >       sdhci_led_activate(host);
> >
> > +     if (host->use_external_dma && mrq->data) {
> > +             struct dma_chan *chan = sdhci_external_dma_channel(host,
> > +                                                                mrq->data);
>
> sdhci_external_dma_channel is not declared if
> !IS_ENABLED(CONFIG_MMC_SDHCI_EXTERNAL_DMA)

Ok, I guess this can be moved to sdhci_external_dma_prepare_data().

>
> > +             dmaengine_synchronize(chan);
>
> So this is to cover for using dmaengine_terminate_async()?

Ok.

>
> > +     }
> >       /*
> >        * Ensure we don't send the STOP for non-SET_BLOCK_COUNTED
> >        * requests if Auto-CMD12 is enabled.
> > @@ -2658,6 +2890,8 @@ static bool sdhci_request_done(struct sdhci_host *host)
> >                               dma_unmap_sg(mmc_dev(host->mmc), data->sg,
> >                                            data->sg_len,
> >                                            mmc_get_dma_dir(data));
> > +                             if (host->use_external_dma)
> > +                                     sdhci_external_dma_cleanup(host, data);
>
> Is sdhci_external_dma_cleanup() only needed in the error case?
>
> The DMA must be stopped before the memory is unmapped and potentially freed.
>
> Isn't the DMA cleanup also needed in the bounce buffer case?
>
> Isn't the DMA cleanup also needed in the COOKIE_PRE_MAPPED case?
>
> dmaengine_terminate_async() doesn't stop the DMA but
> dmaengine_terminate_sync() is not atomic, which looks like a problem.
>
> Perhaps you look at scheduling some work for the external dma error case
> instead of calling __sdhci_finish_mrq()?  Then the work can do the
> dmaengine_terminate_sync() and call __sdhci_finish_mrq().

Ok, I will look at these issues.

>
> >                       }
> >                       data->host_cookie = COOKIE_UNMAPPED;
> >               }
> > @@ -3692,12 +3926,15 @@ int sdhci_setup_host(struct sdhci_host *host)
> >                      mmc_hostname(mmc), host->version);
> >       }
> >
> > -     if (host->quirks & SDHCI_QUIRK_FORCE_DMA)
> > +     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 */
> > +     } else {
> >               host->flags |= SDHCI_USE_SDMA;
> > +     }
>
> These if-statements are about setting SDHCI_USE_SDMA but why is a change
> needed for the host->use_external_dma case?

Yes, this is not needed, otherwise the controller cannot switch back
to SDMA if it is supported in the controller.

>
> >
> >       if ((host->quirks & SDHCI_QUIRK_BROKEN_DMA) &&
> >               (host->flags & SDHCI_USE_SDMA)) {
> > @@ -3785,6 +4022,19 @@ int sdhci_setup_host(struct sdhci_host *host)
> >               }
> >       }
> >
> > +     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 we use DMA, then it's up to the caller to set the DMA
> >        * mask, but PIO does not need the hw shim so we set a new
> > @@ -4201,6 +4451,10 @@ void sdhci_cleanup_host(struct sdhci_host *host)
> >               dma_free_coherent(mmc_dev(mmc), host->align_buffer_sz +
> >                                 host->adma_table_sz, host->align_buffer,
> >                                 host->align_addr);
> > +
> > +     if (host->use_external_dma)
> > +             sdhci_external_dma_release(host);
> > +
> >       host->adma_table = NULL;
> >       host->align_buffer = NULL;
> >  }
> > @@ -4247,6 +4501,7 @@ int __sdhci_add_host(struct sdhci_host *host)
> >
> >       pr_info("%s: SDHCI controller on %s [%s] using %s\n",
> >               mmc_hostname(mmc), host->hw_name, dev_name(mmc_dev(mmc)),
> > +             host->use_external_dma ? "External DMA" :
> >               (host->flags & SDHCI_USE_ADMA) ?
> >               (host->flags & SDHCI_USE_64_BIT_DMA) ? "ADMA 64-bit" : "ADMA" :
> >               (host->flags & SDHCI_USE_SDMA) ? "DMA" : "PIO");
> > @@ -4335,6 +4590,9 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
> >                                 host->adma_table_sz, host->align_buffer,
> >                                 host->align_addr);
> >
> > +     if (host->use_external_dma)
> > +             sdhci_external_dma_release(host);
> > +
> >       host->adma_table = NULL;
> >       host->align_buffer = NULL;
> >  }
> > diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> > index 6cc9a3c2ac66..7a52823ebef4 100644
> > --- a/drivers/mmc/host/sdhci.h
> > +++ b/drivers/mmc/host/sdhci.h
> > @@ -482,6 +482,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;
> > @@ -531,6 +532,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_external_dma; /* Host selects to use external DMA */
>
> Please align /**/ with above i.e. use tab
>
> >
> >       struct mmc_request *mrqs_done[SDHCI_MAX_MRQS];  /* Requests done */
> >       struct mmc_command *cmd;        /* Current command */
> > @@ -559,6 +561,11 @@ 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_EXTERNAL_DMA)
> > +     struct dma_chan *rx_chan;
> > +     struct dma_chan *tx_chan;
> > +#endif
> > +
> >       u32 caps;               /* CAPABILITY_0 */
> >       u32 caps1;              /* CAPABILITY_1 */
> >       bool read_caps;         /* Capability flags have been read */
> > @@ -792,5 +799,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_external_dma(struct sdhci_host *host, bool en);
> >
> >  #endif /* __SDHCI_HW_H */
> >
>

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

[Faiz Abbas]

Hi,

On 28/01/19 3:50 PM, Chunyan Zhang wrote:
> On Thu, 24 Jan 2019 at 19:41, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>
>> On 11/01/19 1:08 PM, 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
>>> synchronize once at the start of each request.
>>
>> Sorry for the slow reply, but I do have some concerns.  Please see the comments.
>>
>>>
>>> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
>>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
>>> ---
>>>  drivers/mmc/host/Kconfig |   3 +
>>>  drivers/mmc/host/sdhci.c | 266 ++++++++++++++++++++++++++++++++++++++-
>>>  drivers/mmc/host/sdhci.h |   8 ++
>>>  3 files changed, 273 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
>>> index e26b8145efb3..333292e8ecdd 100644
>>> --- a/drivers/mmc/host/Kconfig
>>> +++ b/drivers/mmc/host/Kconfig
>>> @@ -999,3 +999,6 @@ config MMC_SDHCI_AM654
>>>         If you have a controller with this interface, say Y or M here.
>>>
>>>         If unsure, say N.
>>> +
>>> +config MMC_SDHCI_EXTERNAL_DMA
>>> +        bool
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index a22e11a65658..4a9044c06e21 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>
>>> @@ -1118,6 +1119,226 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
>>>       }
>>>  }
>>>
>>> +#if IS_ENABLED(CONFIG_MMC_SDHCI_EXTERNAL_DMA)
>>> +static int sdhci_external_dma_init(struct sdhci_host *host)
>>> +{
>>> +     int ret = 0;
>>> +     struct mmc_host *mmc = host->mmc;
>>> +
>>> +     host->tx_chan = dma_request_chan(mmc->parent, "tx");
>>> +     if (IS_ERR(host->tx_chan)) {
>>> +             ret = PTR_ERR(host->tx_chan);
>>> +             if (ret != -EPROBE_DEFER)
>>> +                     pr_warn("Failed to request TX DMA channel.\n");
>>> +             host->tx_chan = NULL;
>>> +             return ret;
>>> +     }
>>> +
>>> +     host->rx_chan = dma_request_chan(mmc->parent, "rx");
>>> +     if (IS_ERR(host->rx_chan)) {
>>> +             if (host->tx_chan) {
>>> +                     dma_release_channel(host->tx_chan);
>>> +                     host->tx_chan = NULL;
>>> +             }
>>> +
>>> +             ret = PTR_ERR(host->rx_chan);
>>> +             if (ret != -EPROBE_DEFER)
>>> +                     pr_warn("Failed to request RX DMA channel.\n");
>>> +             host->rx_chan = NULL;
>>> +     }
>>> +
>>> +     return ret;
>>> +}
>>> +
>>> +static inline struct dma_chan *
>>> +sdhci_external_dma_channel(struct sdhci_host *host, struct mmc_data *data)
>>> +{
>>> +     return data->flags & MMC_DATA_WRITE ? host->tx_chan : host->rx_chan;
>>> +}
>>> +
>>> +static int sdhci_external_dma_setup(struct sdhci_host *host,
>>> +                                 struct mmc_command *cmd)
>>> +{
>>> +     int ret, i;
>>> +     struct dma_async_tx_descriptor *desc;
>>> +     struct mmc_data *data = cmd->data;
>>> +     struct dma_chan *chan;
>>> +     struct dma_slave_config cfg;
>>> +     dma_cookie_t cookie;
>>> +     int sg_cnt;
>>> +
>>> +     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_external_dma_channel(host, data);
>>> +
>>> +     ret = dmaengine_slave_config(chan, &cfg);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     sg_cnt = sdhci_pre_dma_transfer(host, data, COOKIE_MAPPED);
>>> +     if (sg_cnt <= 0)
>>> +             return -EINVAL;
>>> +
>>> +     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;
>>> +
>>> +     cookie = dmaengine_submit(desc);
>>> +     if (cookie < 0)
>>> +             ret = cookie;
>>> +
>>> +     return ret;
>>> +}
>>> +
>>> +static void sdhci_external_dma_release(struct sdhci_host *host)
>>> +{
>>> +     if (host->tx_chan) {
>>> +             dma_release_channel(host->tx_chan);
>>> +             host->tx_chan = NULL;
>>> +     }
>>> +
>>> +     if (host->rx_chan) {
>>> +             dma_release_channel(host->rx_chan);
>>> +             host->rx_chan = NULL;
>>> +     }
>>> +
>>> +     sdhci_switch_external_dma(host, false);
>>> +}
>>> +
>>> +static int __sdhci_external_dma_prepare_data(struct sdhci_host *host,
>>> +                                          struct mmc_command *cmd)
>>> +{
>>> +     struct mmc_data *data = cmd->data;
>>> +
>>> +     host->data_timeout = 0;
>>> +
>>> +     if (sdhci_data_line_cmd(cmd))
>>> +             sdhci_set_timeout(host, cmd);
>>> +
>>> +     WARN_ON(host->data);
>>> +
>>> +     /* Sanity checks */
>>> +     WARN_ON(data->blksz * data->blocks > 524288);
>>> +     WARN_ON(data->blksz > host->mmc->max_blk_size);
>>> +     WARN_ON(data->blocks > 65535);
>>> +
>>> +     host->flags |= SDHCI_REQ_USE_DMA;
>>> +     host->data = data;
>>> +     host->data_early = 0;
>>> +     host->data->bytes_xfered = 0;
>>> +
>>> +     sdhci_set_transfer_irqs(host);
>>> +
>>> +     /*
>>> +      * For Version 4.10 onwards, if v4 mode is enabled, 32-bit Block Count
>>> +      * can be supported, in that case 16-bit block count register must be 0.
>>> +      */
>>> +     if (host->version >= SDHCI_SPEC_410 && host->v4_mode &&
>>> +         (host->quirks2 & SDHCI_QUIRK2_USE_32BIT_BLK_CNT)) {
>>> +             if (sdhci_readw(host, SDHCI_BLOCK_COUNT))
>>> +                     sdhci_writew(host, 0, SDHCI_BLOCK_COUNT);
>>> +             sdhci_writew(host, data->blocks, SDHCI_32BIT_BLK_CNT);
>>> +     } else {
>>> +             sdhci_writew(host, data->blocks, SDHCI_BLOCK_COUNT);
>>> +     }
>>
>> It is probably worth factoring out the code that is shared with
>> sdhci_prepare_data() where possible.
>>
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static void sdhci_external_dma_prepare_data(struct sdhci_host *host,
>>> +                                         struct mmc_command *cmd)
>>> +{
>>> +     struct mmc_data *data = cmd->data;
>>> +
>>> +     if (!data)
>>> +             return;
>>
>> Even in the !data case, we still need to set up a timeout for commands with
>> busy waiting.  I suggest checking the !data case before calling
>> sdhci_external_dma_prepare_data()
> 
> Ok.
> 
>>
>>> +
>>> +     if (sdhci_external_dma_setup(host, cmd) ||
>>> +         __sdhci_external_dma_prepare_data(host, cmd)) {
>>> +             sdhci_external_dma_release(host);
>>> +             pr_err("%s: Cannot use external DMA, switch to the DMA/PIO which standard SDHCI provides.\n",
>>> +                    mmc_hostname(host->mmc));
>>> +             sdhci_prepare_data(host, cmd);
>>> +     }
>>> +}
>>> +
>>> +static void sdhci_external_dma_pre_transfer(struct sdhci_host *host,
>>> +                                         struct mmc_command *cmd)
>>> +{
>>> +     struct dma_chan *chan;
>>> +
>>> +     if (!cmd->data || cmd->opcode == MMC_SET_BLOCK_COUNT)
>>
>> MMC_SET_BLOCK_COUNT never has cmd->data and so does not need to be checked.
> 
> Ok.
> 
>>
>>> +             return;
>>> +
>>> +     sdhci_writew(host, cmd->data->blksz, SDHCI_BLOCK_SIZE);
>>
>> Block size is set in __sdhci_external_dma_prepare_data() so does it need to
>> be set here also.
> 
> Ok.

That is BLOCK_COUNT. This is BLOCK_SIZE.

> 
>>
>>> +     chan = sdhci_external_dma_channel(host, cmd->data);
>>> +     if (chan)
>>> +             dma_async_issue_pending(chan);
>>> +}
>>> +
>>> +static int sdhci_external_dma_cleanup(struct sdhci_host *host,
>>> +                                 struct mmc_data *data)
>>
>> Please align parameters with open parenthesis
>>
>>> +{
>>> +     struct dma_chan *chan = sdhci_external_dma_channel(host, data);
>>> +     int ret = 0;
>>> +
>>> +     if (chan)
>>> +             ret = dmaengine_terminate_async(chan);
>>> +
>>> +     return ret;
>>> +}
>>> +#else
>>> +static int sdhci_external_dma_init(struct sdhci_host *host)
>>> +{
>>> +     return -EOPNOTSUPP;
>>> +}
>>> +
>>> +static void sdhci_external_dma_release(struct sdhci_host *host)
>>> +{}
>>> +
>>> +static void sdhci_external_dma_prepare_data(struct sdhci_host *host,
>>> +                                         struct mmc_command *cmd)
>>> +{
>>> +     /* If MMC_SDHCI_EXTERNAL_DMA not supported, PIO will be used */
>>> +     sdhci_prepare_data(host, cmd);
>>> +}
>>> +
>>> +static void sdhci_external_dma_pre_transfer(struct sdhci_host *host,
>>> +                                         struct mmc_command *cmd)
>>> +{}
>>> +
>>> +static int sdhci_external_dma_cleanup(struct sdhci_host *host,
>>> +                                 struct mmc_data *data)
>>
>> Please align parameters with open parenthesis
>>
>>> +{
>>> +     return 0;
>>> +}
>>> +#endif
>>> +
>>> +void sdhci_switch_external_dma(struct sdhci_host *host, bool en)
>>> +{
>>> +     host->use_external_dma = en;
>>> +}
>>> +EXPORT_SYMBOL_GPL(sdhci_switch_external_dma);
>>> +
>>>  static inline bool sdhci_auto_cmd12(struct sdhci_host *host,
>>>                                   struct mmc_request *mrq)
>>>  {
>>> @@ -1374,7 +1595,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_external_dma)
>>
>> As mentioned above wrt sdhci_external_dma_prepare_data():
>>
>>         if (host->use_external_dma && cmd->data)
>>
> 
> Maybe we should move checking the !data case out of both
> _prepare_data, and add it to sdhci_send_command() before calling
> _prepare_data(), that's saying we can do set up a timeout in
> sdhci_send_command().

Lets just factor out everything in common as Adrian said. The !data,
BLOCK_COUNT writes and busy waiting.

> 
>>> +             sdhci_external_dma_prepare_data(host, cmd);
>>> +     else
>>> +             sdhci_prepare_data(host, cmd);
>>>
>>>       sdhci_writel(host, cmd->arg, SDHCI_ARGUMENT);
>>>
>>> @@ -1416,6 +1640,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_external_dma)
>>> +             sdhci_external_dma_pre_transfer(host, cmd);
>>
>> Why is sdhci_external_dma_pre_transfer() needed here - couldn't it be done
>> in sdhci_external_dma_prepare_data()?
>>
> 
> I'm not sure dma_async_issue_pending() can be done so early in
> sdhci_external_dma_prepare_data().
> 
>>> +
>>>       sdhci_writew(host, SDHCI_MAKE_CMD(cmd->opcode, flags), SDHCI_COMMAND);
>>>  }
>>>  EXPORT_SYMBOL_GPL(sdhci_send_command);
>>> @@ -1781,6 +2008,11 @@ void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>>>
>>>       sdhci_led_activate(host);
>>>
>>> +     if (host->use_external_dma && mrq->data) {
>>> +             struct dma_chan *chan = sdhci_external_dma_channel(host,
>>> +                                                                mrq->data);
>>
>> sdhci_external_dma_channel is not declared if
>> !IS_ENABLED(CONFIG_MMC_SDHCI_EXTERNAL_DMA)
> 
> Ok, I guess this can be moved to sdhci_external_dma_prepare_data().

No. We have to synchronize from a guaranteed non-atomic context. Maybe
add another empty sdhci_external_dma_channel definition in the #else part?

> 
>>
>>> +             dmaengine_synchronize(chan);
>>
>> So this is to cover for using dmaengine_terminate_async()?

Yes.

> 
> Ok.
> 
>>
>>> +     }
>>>       /*
>>>        * Ensure we don't send the STOP for non-SET_BLOCK_COUNTED
>>>        * requests if Auto-CMD12 is enabled.
>>> @@ -2658,6 +2890,8 @@ static bool sdhci_request_done(struct sdhci_host *host)
>>>                               dma_unmap_sg(mmc_dev(host->mmc), data->sg,
>>>                                            data->sg_len,
>>>                                            mmc_get_dma_dir(data));
>>> +                             if (host->use_external_dma)
>>> +                                     sdhci_external_dma_cleanup(host, data);
>>
>> Is sdhci_external_dma_cleanup() only needed in the error case?
>>
>> The DMA must be stopped before the memory is unmapped and potentially freed.
>>
>> Isn't the DMA cleanup also needed in the bounce buffer case?
>>
>> Isn't the DMA cleanup also needed in the COOKIE_PRE_MAPPED case?
>>
>> dmaengine_terminate_async() doesn't stop the DMA but
>> dmaengine_terminate_sync() is not atomic, which looks like a problem.
>>
>> Perhaps you look at scheduling some work for the external dma error case
>> instead of calling __sdhci_finish_mrq()?  Then the work can do the
>> dmaengine_terminate_sync() and call __sdhci_finish_mrq().
> 
> Ok, I will look at these issues.
> 
>>
>>>                       }
>>>                       data->host_cookie = COOKIE_UNMAPPED;
>>>               }
>>> @@ -3692,12 +3926,15 @@ int sdhci_setup_host(struct sdhci_host *host)
>>>                      mmc_hostname(mmc), host->version);
>>>       }
>>>
>>> -     if (host->quirks & SDHCI_QUIRK_FORCE_DMA)
>>> +     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 */
>>> +     } else {
>>>               host->flags |= SDHCI_USE_SDMA;
>>> +     }
>>
>> These if-statements are about setting SDHCI_USE_SDMA but why is a change
>> needed for the host->use_external_dma case?
> 
> Yes, this is not needed, otherwise the controller cannot switch back
> to SDMA if it is supported in the controller.

The else case means that the host always uses SDMA. Needed to make sure
this doesn't happen in the external_dma case. Its not SDHCI_CAN_DO_SDMA,
its SDHCI_USE_SDMA.

> 
>>
>>>
>>>       if ((host->quirks & SDHCI_QUIRK_BROKEN_DMA) &&
>>>               (host->flags & SDHCI_USE_SDMA)) {
>>> @@ -3785,6 +4022,19 @@ int sdhci_setup_host(struct sdhci_host *host)
>>>               }
>>>       }
>>>
>>> +     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 we use DMA, then it's up to the caller to set the DMA
>>>        * mask, but PIO does not need the hw shim so we set a new
>>> @@ -4201,6 +4451,10 @@ void sdhci_cleanup_host(struct sdhci_host *host)
>>>               dma_free_coherent(mmc_dev(mmc), host->align_buffer_sz +
>>>                                 host->adma_table_sz, host->align_buffer,
>>>                                 host->align_addr);
>>> +
>>> +     if (host->use_external_dma)
>>> +             sdhci_external_dma_release(host);
>>> +
>>>       host->adma_table = NULL;
>>>       host->align_buffer = NULL;
>>>  }
>>> @@ -4247,6 +4501,7 @@ int __sdhci_add_host(struct sdhci_host *host)
>>>
>>>       pr_info("%s: SDHCI controller on %s [%s] using %s\n",
>>>               mmc_hostname(mmc), host->hw_name, dev_name(mmc_dev(mmc)),
>>> +             host->use_external_dma ? "External DMA" :
>>>               (host->flags & SDHCI_USE_ADMA) ?
>>>               (host->flags & SDHCI_USE_64_BIT_DMA) ? "ADMA 64-bit" : "ADMA" :
>>>               (host->flags & SDHCI_USE_SDMA) ? "DMA" : "PIO");
>>> @@ -4335,6 +4590,9 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
>>>                                 host->adma_table_sz, host->align_buffer,
>>>                                 host->align_addr);
>>>
>>> +     if (host->use_external_dma)
>>> +             sdhci_external_dma_release(host);
>>> +
>>>       host->adma_table = NULL;
>>>       host->align_buffer = NULL;
>>>  }
>>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>>> index 6cc9a3c2ac66..7a52823ebef4 100644
>>> --- a/drivers/mmc/host/sdhci.h
>>> +++ b/drivers/mmc/host/sdhci.h
>>> @@ -482,6 +482,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;
>>> @@ -531,6 +532,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_external_dma; /* Host selects to use external DMA */
>>
>> Please align /**/ with above i.e. use tab

Will fix.

Thanks,
Faiz

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

[Chunyan Zhang]

On Mon, 28 Jan 2019 at 18:40, Faiz Abbas <faiz_abbas@ti.com> wrote:
>
> Hi,
>
> On 28/01/19 3:50 PM, Chunyan Zhang wrote:
> > On Thu, 24 Jan 2019 at 19:41, Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>
> >> On 11/01/19 1:08 PM, 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
> >>> synchronize once at the start of each request.
> >>
> >> Sorry for the slow reply, but I do have some concerns.  Please see the comments.
> >>
> >>>
> >>> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
> >>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
> >>> ---
> >>>  drivers/mmc/host/Kconfig |   3 +
> >>>  drivers/mmc/host/sdhci.c | 266 ++++++++++++++++++++++++++++++++++++++-
> >>>  drivers/mmc/host/sdhci.h |   8 ++
> >>>  3 files changed, 273 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> >>> index e26b8145efb3..333292e8ecdd 100644
> >>> --- a/drivers/mmc/host/Kconfig
> >>> +++ b/drivers/mmc/host/Kconfig
> >>> @@ -999,3 +999,6 @@ config MMC_SDHCI_AM654
> >>>         If you have a controller with this interface, say Y or M here.
> >>>
> >>>         If unsure, say N.
> >>> +
> >>> +config MMC_SDHCI_EXTERNAL_DMA
> >>> +        bool
> >>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> >>> index a22e11a65658..4a9044c06e21 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>
> >>> @@ -1118,6 +1119,226 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
> >>>       }
> >>>  }
> >>>
> >>> +#if IS_ENABLED(CONFIG_MMC_SDHCI_EXTERNAL_DMA)
> >>> +static int sdhci_external_dma_init(struct sdhci_host *host)
> >>> +{
> >>> +     int ret = 0;
> >>> +     struct mmc_host *mmc = host->mmc;
> >>> +
> >>> +     host->tx_chan = dma_request_chan(mmc->parent, "tx");
> >>> +     if (IS_ERR(host->tx_chan)) {
> >>> +             ret = PTR_ERR(host->tx_chan);
> >>> +             if (ret != -EPROBE_DEFER)
> >>> +                     pr_warn("Failed to request TX DMA channel.\n");
> >>> +             host->tx_chan = NULL;
> >>> +             return ret;
> >>> +     }
> >>> +
> >>> +     host->rx_chan = dma_request_chan(mmc->parent, "rx");
> >>> +     if (IS_ERR(host->rx_chan)) {
> >>> +             if (host->tx_chan) {
> >>> +                     dma_release_channel(host->tx_chan);
> >>> +                     host->tx_chan = NULL;
> >>> +             }
> >>> +
> >>> +             ret = PTR_ERR(host->rx_chan);
> >>> +             if (ret != -EPROBE_DEFER)
> >>> +                     pr_warn("Failed to request RX DMA channel.\n");
> >>> +             host->rx_chan = NULL;
> >>> +     }
> >>> +
> >>> +     return ret;
> >>> +}
> >>> +
> >>> +static inline struct dma_chan *
> >>> +sdhci_external_dma_channel(struct sdhci_host *host, struct mmc_data *data)
> >>> +{
> >>> +     return data->flags & MMC_DATA_WRITE ? host->tx_chan : host->rx_chan;
> >>> +}
> >>> +
> >>> +static int sdhci_external_dma_setup(struct sdhci_host *host,
> >>> +                                 struct mmc_command *cmd)
> >>> +{
> >>> +     int ret, i;
> >>> +     struct dma_async_tx_descriptor *desc;
> >>> +     struct mmc_data *data = cmd->data;
> >>> +     struct dma_chan *chan;
> >>> +     struct dma_slave_config cfg;
> >>> +     dma_cookie_t cookie;
> >>> +     int sg_cnt;
> >>> +
> >>> +     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_external_dma_channel(host, data);
> >>> +
> >>> +     ret = dmaengine_slave_config(chan, &cfg);
> >>> +     if (ret)
> >>> +             return ret;
> >>> +
> >>> +     sg_cnt = sdhci_pre_dma_transfer(host, data, COOKIE_MAPPED);
> >>> +     if (sg_cnt <= 0)
> >>> +             return -EINVAL;
> >>> +
> >>> +     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;
> >>> +
> >>> +     cookie = dmaengine_submit(desc);
> >>> +     if (cookie < 0)
> >>> +             ret = cookie;
> >>> +
> >>> +     return ret;
> >>> +}
> >>> +
> >>> +static void sdhci_external_dma_release(struct sdhci_host *host)
> >>> +{
> >>> +     if (host->tx_chan) {
> >>> +             dma_release_channel(host->tx_chan);
> >>> +             host->tx_chan = NULL;
> >>> +     }
> >>> +
> >>> +     if (host->rx_chan) {
> >>> +             dma_release_channel(host->rx_chan);
> >>> +             host->rx_chan = NULL;
> >>> +     }
> >>> +
> >>> +     sdhci_switch_external_dma(host, false);
> >>> +}
> >>> +
> >>> +static int __sdhci_external_dma_prepare_data(struct sdhci_host *host,
> >>> +                                          struct mmc_command *cmd)
> >>> +{
> >>> +     struct mmc_data *data = cmd->data;
> >>> +
> >>> +     host->data_timeout = 0;
> >>> +
> >>> +     if (sdhci_data_line_cmd(cmd))
> >>> +             sdhci_set_timeout(host, cmd);
> >>> +
> >>> +     WARN_ON(host->data);
> >>> +
> >>> +     /* Sanity checks */
> >>> +     WARN_ON(data->blksz * data->blocks > 524288);
> >>> +     WARN_ON(data->blksz > host->mmc->max_blk_size);
> >>> +     WARN_ON(data->blocks > 65535);
> >>> +
> >>> +     host->flags |= SDHCI_REQ_USE_DMA;
> >>> +     host->data = data;
> >>> +     host->data_early = 0;
> >>> +     host->data->bytes_xfered = 0;
> >>> +
> >>> +     sdhci_set_transfer_irqs(host);
> >>> +
> >>> +     /*
> >>> +      * For Version 4.10 onwards, if v4 mode is enabled, 32-bit Block Count
> >>> +      * can be supported, in that case 16-bit block count register must be 0.
> >>> +      */
> >>> +     if (host->version >= SDHCI_SPEC_410 && host->v4_mode &&
> >>> +         (host->quirks2 & SDHCI_QUIRK2_USE_32BIT_BLK_CNT)) {
> >>> +             if (sdhci_readw(host, SDHCI_BLOCK_COUNT))
> >>> +                     sdhci_writew(host, 0, SDHCI_BLOCK_COUNT);
> >>> +             sdhci_writew(host, data->blocks, SDHCI_32BIT_BLK_CNT);
> >>> +     } else {
> >>> +             sdhci_writew(host, data->blocks, SDHCI_BLOCK_COUNT);
> >>> +     }
> >>
> >> It is probably worth factoring out the code that is shared with
> >> sdhci_prepare_data() where possible.
> >>
> >>> +
> >>> +     return 0;
> >>> +}
> >>> +
> >>> +static void sdhci_external_dma_prepare_data(struct sdhci_host *host,
> >>> +                                         struct mmc_command *cmd)
> >>> +{
> >>> +     struct mmc_data *data = cmd->data;
> >>> +
> >>> +     if (!data)
> >>> +             return;
> >>
> >> Even in the !data case, we still need to set up a timeout for commands with
> >> busy waiting.  I suggest checking the !data case before calling
> >> sdhci_external_dma_prepare_data()
> >
> > Ok.
> >
> >>
> >>> +
> >>> +     if (sdhci_external_dma_setup(host, cmd) ||
> >>> +         __sdhci_external_dma_prepare_data(host, cmd)) {
> >>> +             sdhci_external_dma_release(host);
> >>> +             pr_err("%s: Cannot use external DMA, switch to the DMA/PIO which standard SDHCI provides.\n",
> >>> +                    mmc_hostname(host->mmc));
> >>> +             sdhci_prepare_data(host, cmd);
> >>> +     }
> >>> +}
> >>> +
> >>> +static void sdhci_external_dma_pre_transfer(struct sdhci_host *host,
> >>> +                                         struct mmc_command *cmd)
> >>> +{
> >>> +     struct dma_chan *chan;
> >>> +
> >>> +     if (!cmd->data || cmd->opcode == MMC_SET_BLOCK_COUNT)
> >>
> >> MMC_SET_BLOCK_COUNT never has cmd->data and so does not need to be checked.
> >
> > Ok.
> >
> >>
> >>> +             return;
> >>> +
> >>> +     sdhci_writew(host, cmd->data->blksz, SDHCI_BLOCK_SIZE);
> >>
> >> Block size is set in __sdhci_external_dma_prepare_data() so does it need to
> >> be set here also.
> >
> > Ok.
>
> That is BLOCK_COUNT. This is BLOCK_SIZE.
>
> >
> >>
> >>> +     chan = sdhci_external_dma_channel(host, cmd->data);
> >>> +     if (chan)
> >>> +             dma_async_issue_pending(chan);
> >>> +}
> >>> +
> >>> +static int sdhci_external_dma_cleanup(struct sdhci_host *host,
> >>> +                                 struct mmc_data *data)
> >>
> >> Please align parameters with open parenthesis
> >>
> >>> +{
> >>> +     struct dma_chan *chan = sdhci_external_dma_channel(host, data);
> >>> +     int ret = 0;
> >>> +
> >>> +     if (chan)
> >>> +             ret = dmaengine_terminate_async(chan);
> >>> +
> >>> +     return ret;
> >>> +}
> >>> +#else
> >>> +static int sdhci_external_dma_init(struct sdhci_host *host)
> >>> +{
> >>> +     return -EOPNOTSUPP;
> >>> +}
> >>> +
> >>> +static void sdhci_external_dma_release(struct sdhci_host *host)
> >>> +{}
> >>> +
> >>> +static void sdhci_external_dma_prepare_data(struct sdhci_host *host,
> >>> +                                         struct mmc_command *cmd)
> >>> +{
> >>> +     /* If MMC_SDHCI_EXTERNAL_DMA not supported, PIO will be used */
> >>> +     sdhci_prepare_data(host, cmd);
> >>> +}
> >>> +
> >>> +static void sdhci_external_dma_pre_transfer(struct sdhci_host *host,
> >>> +                                         struct mmc_command *cmd)
> >>> +{}
> >>> +
> >>> +static int sdhci_external_dma_cleanup(struct sdhci_host *host,
> >>> +                                 struct mmc_data *data)
> >>
> >> Please align parameters with open parenthesis
> >>
> >>> +{
> >>> +     return 0;
> >>> +}
> >>> +#endif
> >>> +
> >>> +void sdhci_switch_external_dma(struct sdhci_host *host, bool en)
> >>> +{
> >>> +     host->use_external_dma = en;
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(sdhci_switch_external_dma);
> >>> +
> >>>  static inline bool sdhci_auto_cmd12(struct sdhci_host *host,
> >>>                                   struct mmc_request *mrq)
> >>>  {
> >>> @@ -1374,7 +1595,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_external_dma)
> >>
> >> As mentioned above wrt sdhci_external_dma_prepare_data():
> >>
> >>         if (host->use_external_dma && cmd->data)
> >>
> >
> > Maybe we should move checking the !data case out of both
> > _prepare_data, and add it to sdhci_send_command() before calling
> > _prepare_data(), that's saying we can do set up a timeout in
> > sdhci_send_command().
>
> Lets just factor out everything in common as Adrian said. The !data,
> BLOCK_COUNT writes and busy waiting.

Ok, and also BLOCK_SIZE which can be set in _prepare_data().

>
> >
> >>> +             sdhci_external_dma_prepare_data(host, cmd);
> >>> +     else
> >>> +             sdhci_prepare_data(host, cmd);
> >>>
> >>>       sdhci_writel(host, cmd->arg, SDHCI_ARGUMENT);
> >>>
> >>> @@ -1416,6 +1640,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_external_dma)
> >>> +             sdhci_external_dma_pre_transfer(host, cmd);
> >>
> >> Why is sdhci_external_dma_pre_transfer() needed here - couldn't it be done
> >> in sdhci_external_dma_prepare_data()?
> >>
> >
> > I'm not sure dma_async_issue_pending() can be done so early in
> > sdhci_external_dma_prepare_data().
> >
> >>> +
> >>>       sdhci_writew(host, SDHCI_MAKE_CMD(cmd->opcode, flags), SDHCI_COMMAND);
> >>>  }
> >>>  EXPORT_SYMBOL_GPL(sdhci_send_command);
> >>> @@ -1781,6 +2008,11 @@ void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
> >>>
> >>>       sdhci_led_activate(host);
> >>>
> >>> +     if (host->use_external_dma && mrq->data) {
> >>> +             struct dma_chan *chan = sdhci_external_dma_channel(host,
> >>> +                                                                mrq->data);
> >>
> >> sdhci_external_dma_channel is not declared if
> >> !IS_ENABLED(CONFIG_MMC_SDHCI_EXTERNAL_DMA)
> >
> > Ok, I guess this can be moved to sdhci_external_dma_prepare_data().
>
> No. We have to synchronize from a guaranteed non-atomic context. Maybe
> add another empty sdhci_external_dma_channel definition in the #else part?
>
> >
> >>
> >>> +             dmaengine_synchronize(chan);
> >>
> >> So this is to cover for using dmaengine_terminate_async()?
>
> Yes.
>
> >
> > Ok.
> >
> >>
> >>> +     }
> >>>       /*
> >>>        * Ensure we don't send the STOP for non-SET_BLOCK_COUNTED
> >>>        * requests if Auto-CMD12 is enabled.
> >>> @@ -2658,6 +2890,8 @@ static bool sdhci_request_done(struct sdhci_host *host)
> >>>                               dma_unmap_sg(mmc_dev(host->mmc), data->sg,
> >>>                                            data->sg_len,
> >>>                                            mmc_get_dma_dir(data));
> >>> +                             if (host->use_external_dma)
> >>> +                                     sdhci_external_dma_cleanup(host, data);
> >>
> >> Is sdhci_external_dma_cleanup() only needed in the error case?
> >>
> >> The DMA must be stopped before the memory is unmapped and potentially freed.
> >>
> >> Isn't the DMA cleanup also needed in the bounce buffer case?
> >>
> >> Isn't the DMA cleanup also needed in the COOKIE_PRE_MAPPED case?
> >>
> >> dmaengine_terminate_async() doesn't stop the DMA but
> >> dmaengine_terminate_sync() is not atomic, which looks like a problem.
> >>
> >> Perhaps you look at scheduling some work for the external dma error case
> >> instead of calling __sdhci_finish_mrq()?  Then the work can do the
> >> dmaengine_terminate_sync() and call __sdhci_finish_mrq().
> >
> > Ok, I will look at these issues.
> >
> >>
> >>>                       }
> >>>                       data->host_cookie = COOKIE_UNMAPPED;
> >>>               }
> >>> @@ -3692,12 +3926,15 @@ int sdhci_setup_host(struct sdhci_host *host)
> >>>                      mmc_hostname(mmc), host->version);
> >>>       }
> >>>
> >>> -     if (host->quirks & SDHCI_QUIRK_FORCE_DMA)
> >>> +     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 */
> >>> +     } else {
> >>>               host->flags |= SDHCI_USE_SDMA;
> >>> +     }
> >>
> >> These if-statements are about setting SDHCI_USE_SDMA but why is a change
> >> needed for the host->use_external_dma case?
> >
> > Yes, this is not needed, otherwise the controller cannot switch back
> > to SDMA if it is supported in the controller.
>
> The else case means that the host always uses SDMA. Needed to make sure
> this doesn't happen in the external_dma case. Its not SDHCI_CAN_DO_SDMA,
> its SDHCI_USE_SDMA.
>
> >
> >>
> >>>
> >>>       if ((host->quirks & SDHCI_QUIRK_BROKEN_DMA) &&
> >>>               (host->flags & SDHCI_USE_SDMA)) {
> >>> @@ -3785,6 +4022,19 @@ int sdhci_setup_host(struct sdhci_host *host)
> >>>               }
> >>>       }
> >>>
> >>> +     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 we use DMA, then it's up to the caller to set the DMA
> >>>        * mask, but PIO does not need the hw shim so we set a new
> >>> @@ -4201,6 +4451,10 @@ void sdhci_cleanup_host(struct sdhci_host *host)
> >>>               dma_free_coherent(mmc_dev(mmc), host->align_buffer_sz +
> >>>                                 host->adma_table_sz, host->align_buffer,
> >>>                                 host->align_addr);
> >>> +
> >>> +     if (host->use_external_dma)
> >>> +             sdhci_external_dma_release(host);
> >>> +
> >>>       host->adma_table = NULL;
> >>>       host->align_buffer = NULL;
> >>>  }
> >>> @@ -4247,6 +4501,7 @@ int __sdhci_add_host(struct sdhci_host *host)
> >>>
> >>>       pr_info("%s: SDHCI controller on %s [%s] using %s\n",
> >>>               mmc_hostname(mmc), host->hw_name, dev_name(mmc_dev(mmc)),
> >>> +             host->use_external_dma ? "External DMA" :
> >>>               (host->flags & SDHCI_USE_ADMA) ?
> >>>               (host->flags & SDHCI_USE_64_BIT_DMA) ? "ADMA 64-bit" : "ADMA" :
> >>>               (host->flags & SDHCI_USE_SDMA) ? "DMA" : "PIO");
> >>> @@ -4335,6 +4590,9 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
> >>>                                 host->adma_table_sz, host->align_buffer,
> >>>                                 host->align_addr);
> >>>
> >>> +     if (host->use_external_dma)
> >>> +             sdhci_external_dma_release(host);
> >>> +
> >>>       host->adma_table = NULL;
> >>>       host->align_buffer = NULL;
> >>>  }
> >>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> >>> index 6cc9a3c2ac66..7a52823ebef4 100644
> >>> --- a/drivers/mmc/host/sdhci.h
> >>> +++ b/drivers/mmc/host/sdhci.h
> >>> @@ -482,6 +482,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;
> >>> @@ -531,6 +532,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_external_dma; /* Host selects to use external DMA */
> >>
> >> Please align /**/ with above i.e. use tab
>
> Will fix.

Do you need me to address these comments and then send you a patch or
you'd like to do instead?

Thanks,
Chunyan

>
> Thanks,
> Faiz

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

[Faiz Abbas]

Chunyan,

On 28/01/19 5:16 PM, Chunyan Zhang wrote:
> On Mon, 28 Jan 2019 at 18:40, Faiz Abbas <faiz_abbas@ti.com> wrote:
>>
>> Hi,
>>
>> On 28/01/19 3:50 PM, Chunyan Zhang wrote:
>>> On Thu, 24 Jan 2019 at 19:41, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>
>>>> On 11/01/19 1:08 PM, 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
>>>>> synchronize once at the start of each request.
>>>>
>>>> Sorry for the slow reply, but I do have some concerns.  Please see the comments.
>>>>
>>>>>
>>>>> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
>>>>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
>>>>> ---
>>>>>  drivers/mmc/host/Kconfig |   3 +
>>>>>  drivers/mmc/host/sdhci.c | 266 ++++++++++++++++++++++++++++++++++++++-
>>>>>  drivers/mmc/host/sdhci.h |   8 ++
>>>>>  3 files changed, 273 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
>>>>> index e26b8145efb3..333292e8ecdd 100644
>>>>> --- a/drivers/mmc/host/Kconfig
>>>>> +++ b/drivers/mmc/host/Kconfig
>>>>> @@ -999,3 +999,6 @@ config MMC_SDHCI_AM654
>>>>>         If you have a controller with this interface, say Y or M here.
>>>>>
>>>>>         If unsure, say N.
>>>>> +
>>>>> +config MMC_SDHCI_EXTERNAL_DMA
>>>>> +        bool
>>>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>>>> index a22e11a65658..4a9044c06e21 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>
>>>>> @@ -1118,6 +1119,226 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
>>>>>       }
>>>>>  }
>>>>>
>>>>> +#if IS_ENABLED(CONFIG_MMC_SDHCI_EXTERNAL_DMA)
>>>>> +static int sdhci_external_dma_init(struct sdhci_host *host)
>>>>> +{
>>>>> +     int ret = 0;
>>>>> +     struct mmc_host *mmc = host->mmc;
>>>>> +
>>>>> +     host->tx_chan = dma_request_chan(mmc->parent, "tx");
>>>>> +     if (IS_ERR(host->tx_chan)) {
>>>>> +             ret = PTR_ERR(host->tx_chan);
>>>>> +             if (ret != -EPROBE_DEFER)
>>>>> +                     pr_warn("Failed to request TX DMA channel.\n");
>>>>> +             host->tx_chan = NULL;
>>>>> +             return ret;
>>>>> +     }
>>>>> +
>>>>> +     host->rx_chan = dma_request_chan(mmc->parent, "rx");
>>>>> +     if (IS_ERR(host->rx_chan)) {
>>>>> +             if (host->tx_chan) {
>>>>> +                     dma_release_channel(host->tx_chan);
>>>>> +                     host->tx_chan = NULL;
>>>>> +             }
>>>>> +
>>>>> +             ret = PTR_ERR(host->rx_chan);
>>>>> +             if (ret != -EPROBE_DEFER)
>>>>> +                     pr_warn("Failed to request RX DMA channel.\n");
>>>>> +             host->rx_chan = NULL;
>>>>> +     }
>>>>> +
>>>>> +     return ret;
>>>>> +}
>>>>> +
>>>>> +static inline struct dma_chan *
>>>>> +sdhci_external_dma_channel(struct sdhci_host *host, struct mmc_data *data)
>>>>> +{
>>>>> +     return data->flags & MMC_DATA_WRITE ? host->tx_chan : host->rx_chan;
>>>>> +}
>>>>> +
>>>>> +static int sdhci_external_dma_setup(struct sdhci_host *host,
>>>>> +                                 struct mmc_command *cmd)
>>>>> +{
>>>>> +     int ret, i;
>>>>> +     struct dma_async_tx_descriptor *desc;
>>>>> +     struct mmc_data *data = cmd->data;
>>>>> +     struct dma_chan *chan;
>>>>> +     struct dma_slave_config cfg;
>>>>> +     dma_cookie_t cookie;
>>>>> +     int sg_cnt;
>>>>> +
>>>>> +     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_external_dma_channel(host, data);
>>>>> +
>>>>> +     ret = dmaengine_slave_config(chan, &cfg);
>>>>> +     if (ret)
>>>>> +             return ret;
>>>>> +
>>>>> +     sg_cnt = sdhci_pre_dma_transfer(host, data, COOKIE_MAPPED);
>>>>> +     if (sg_cnt <= 0)
>>>>> +             return -EINVAL;
>>>>> +
>>>>> +     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;
>>>>> +
>>>>> +     cookie = dmaengine_submit(desc);
>>>>> +     if (cookie < 0)
>>>>> +             ret = cookie;
>>>>> +
>>>>> +     return ret;
>>>>> +}
>>>>> +
>>>>> +static void sdhci_external_dma_release(struct sdhci_host *host)
>>>>> +{
>>>>> +     if (host->tx_chan) {
>>>>> +             dma_release_channel(host->tx_chan);
>>>>> +             host->tx_chan = NULL;
>>>>> +     }
>>>>> +
>>>>> +     if (host->rx_chan) {
>>>>> +             dma_release_channel(host->rx_chan);
>>>>> +             host->rx_chan = NULL;
>>>>> +     }
>>>>> +
>>>>> +     sdhci_switch_external_dma(host, false);
>>>>> +}
>>>>> +
>>>>> +static int __sdhci_external_dma_prepare_data(struct sdhci_host *host,
>>>>> +                                          struct mmc_command *cmd)
>>>>> +{
>>>>> +     struct mmc_data *data = cmd->data;
>>>>> +
>>>>> +     host->data_timeout = 0;
>>>>> +
>>>>> +     if (sdhci_data_line_cmd(cmd))
>>>>> +             sdhci_set_timeout(host, cmd);
>>>>> +
>>>>> +     WARN_ON(host->data);
>>>>> +
>>>>> +     /* Sanity checks */
>>>>> +     WARN_ON(data->blksz * data->blocks > 524288);
>>>>> +     WARN_ON(data->blksz > host->mmc->max_blk_size);
>>>>> +     WARN_ON(data->blocks > 65535);
>>>>> +
>>>>> +     host->flags |= SDHCI_REQ_USE_DMA;
>>>>> +     host->data = data;
>>>>> +     host->data_early = 0;
>>>>> +     host->data->bytes_xfered = 0;
>>>>> +
>>>>> +     sdhci_set_transfer_irqs(host);
>>>>> +
>>>>> +     /*
>>>>> +      * For Version 4.10 onwards, if v4 mode is enabled, 32-bit Block Count
>>>>> +      * can be supported, in that case 16-bit block count register must be 0.
>>>>> +      */
>>>>> +     if (host->version >= SDHCI_SPEC_410 && host->v4_mode &&
>>>>> +         (host->quirks2 & SDHCI_QUIRK2_USE_32BIT_BLK_CNT)) {
>>>>> +             if (sdhci_readw(host, SDHCI_BLOCK_COUNT))
>>>>> +                     sdhci_writew(host, 0, SDHCI_BLOCK_COUNT);
>>>>> +             sdhci_writew(host, data->blocks, SDHCI_32BIT_BLK_CNT);
>>>>> +     } else {
>>>>> +             sdhci_writew(host, data->blocks, SDHCI_BLOCK_COUNT);
>>>>> +     }
>>>>
>>>> It is probably worth factoring out the code that is shared with
>>>> sdhci_prepare_data() where possible.
>>>>
>>>>> +
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +static void sdhci_external_dma_prepare_data(struct sdhci_host *host,
>>>>> +                                         struct mmc_command *cmd)
>>>>> +{
>>>>> +     struct mmc_data *data = cmd->data;
>>>>> +
>>>>> +     if (!data)
>>>>> +             return;
>>>>
>>>> Even in the !data case, we still need to set up a timeout for commands with
>>>> busy waiting.  I suggest checking the !data case before calling
>>>> sdhci_external_dma_prepare_data()
>>>
>>> Ok.
>>>
>>>>
>>>>> +
>>>>> +     if (sdhci_external_dma_setup(host, cmd) ||
>>>>> +         __sdhci_external_dma_prepare_data(host, cmd)) {
>>>>> +             sdhci_external_dma_release(host);
>>>>> +             pr_err("%s: Cannot use external DMA, switch to the DMA/PIO which standard SDHCI provides.\n",
>>>>> +                    mmc_hostname(host->mmc));
>>>>> +             sdhci_prepare_data(host, cmd);
>>>>> +     }
>>>>> +}
>>>>> +
>>>>> +static void sdhci_external_dma_pre_transfer(struct sdhci_host *host,
>>>>> +                                         struct mmc_command *cmd)
>>>>> +{
>>>>> +     struct dma_chan *chan;
>>>>> +
>>>>> +     if (!cmd->data || cmd->opcode == MMC_SET_BLOCK_COUNT)
>>>>
>>>> MMC_SET_BLOCK_COUNT never has cmd->data and so does not need to be checked.
>>>
>>> Ok.
>>>
>>>>
>>>>> +             return;
>>>>> +
>>>>> +     sdhci_writew(host, cmd->data->blksz, SDHCI_BLOCK_SIZE);
>>>>
>>>> Block size is set in __sdhci_external_dma_prepare_data() so does it need to
>>>> be set here also.
>>>
>>> Ok.
>>
>> That is BLOCK_COUNT. This is BLOCK_SIZE.
>>
>>>
>>>>
>>>>> +     chan = sdhci_external_dma_channel(host, cmd->data);
>>>>> +     if (chan)
>>>>> +             dma_async_issue_pending(chan);
>>>>> +}
>>>>> +
>>>>> +static int sdhci_external_dma_cleanup(struct sdhci_host *host,
>>>>> +                                 struct mmc_data *data)
>>>>
>>>> Please align parameters with open parenthesis
>>>>
>>>>> +{
>>>>> +     struct dma_chan *chan = sdhci_external_dma_channel(host, data);
>>>>> +     int ret = 0;
>>>>> +
>>>>> +     if (chan)
>>>>> +             ret = dmaengine_terminate_async(chan);
>>>>> +
>>>>> +     return ret;
>>>>> +}
>>>>> +#else
>>>>> +static int sdhci_external_dma_init(struct sdhci_host *host)
>>>>> +{
>>>>> +     return -EOPNOTSUPP;
>>>>> +}
>>>>> +
>>>>> +static void sdhci_external_dma_release(struct sdhci_host *host)
>>>>> +{}
>>>>> +
>>>>> +static void sdhci_external_dma_prepare_data(struct sdhci_host *host,
>>>>> +                                         struct mmc_command *cmd)
>>>>> +{
>>>>> +     /* If MMC_SDHCI_EXTERNAL_DMA not supported, PIO will be used */
>>>>> +     sdhci_prepare_data(host, cmd);
>>>>> +}
>>>>> +
>>>>> +static void sdhci_external_dma_pre_transfer(struct sdhci_host *host,
>>>>> +                                         struct mmc_command *cmd)
>>>>> +{}
>>>>> +
>>>>> +static int sdhci_external_dma_cleanup(struct sdhci_host *host,
>>>>> +                                 struct mmc_data *data)
>>>>
>>>> Please align parameters with open parenthesis
>>>>
>>>>> +{
>>>>> +     return 0;
>>>>> +}
>>>>> +#endif
>>>>> +
>>>>> +void sdhci_switch_external_dma(struct sdhci_host *host, bool en)
>>>>> +{
>>>>> +     host->use_external_dma = en;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(sdhci_switch_external_dma);
>>>>> +
>>>>>  static inline bool sdhci_auto_cmd12(struct sdhci_host *host,
>>>>>                                   struct mmc_request *mrq)
>>>>>  {
>>>>> @@ -1374,7 +1595,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_external_dma)
>>>>
>>>> As mentioned above wrt sdhci_external_dma_prepare_data():
>>>>
>>>>         if (host->use_external_dma && cmd->data)
>>>>
>>>
>>> Maybe we should move checking the !data case out of both
>>> _prepare_data, and add it to sdhci_send_command() before calling
>>> _prepare_data(), that's saying we can do set up a timeout in
>>> sdhci_send_command().
>>
>> Lets just factor out everything in common as Adrian said. The !data,
>> BLOCK_COUNT writes and busy waiting.
> 
> Ok, and also BLOCK_SIZE which can be set in _prepare_data().
> 
>>
>>>
>>>>> +             sdhci_external_dma_prepare_data(host, cmd);
>>>>> +     else
>>>>> +             sdhci_prepare_data(host, cmd);
>>>>>
>>>>>       sdhci_writel(host, cmd->arg, SDHCI_ARGUMENT);
>>>>>
>>>>> @@ -1416,6 +1640,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_external_dma)
>>>>> +             sdhci_external_dma_pre_transfer(host, cmd);
>>>>
>>>> Why is sdhci_external_dma_pre_transfer() needed here - couldn't it be done
>>>> in sdhci_external_dma_prepare_data()?
>>>>
>>>
>>> I'm not sure dma_async_issue_pending() can be done so early in
>>> sdhci_external_dma_prepare_data().
>>>
>>>>> +
>>>>>       sdhci_writew(host, SDHCI_MAKE_CMD(cmd->opcode, flags), SDHCI_COMMAND);
>>>>>  }
>>>>>  EXPORT_SYMBOL_GPL(sdhci_send_command);
>>>>> @@ -1781,6 +2008,11 @@ void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>>>>>
>>>>>       sdhci_led_activate(host);
>>>>>
>>>>> +     if (host->use_external_dma && mrq->data) {
>>>>> +             struct dma_chan *chan = sdhci_external_dma_channel(host,
>>>>> +                                                                mrq->data);
>>>>
>>>> sdhci_external_dma_channel is not declared if
>>>> !IS_ENABLED(CONFIG_MMC_SDHCI_EXTERNAL_DMA)
>>>
>>> Ok, I guess this can be moved to sdhci_external_dma_prepare_data().
>>
>> No. We have to synchronize from a guaranteed non-atomic context. Maybe
>> add another empty sdhci_external_dma_channel definition in the #else part?
>>
>>>
>>>>
>>>>> +             dmaengine_synchronize(chan);
>>>>
>>>> So this is to cover for using dmaengine_terminate_async()?
>>
>> Yes.
>>
>>>
>>> Ok.
>>>
>>>>
>>>>> +     }
>>>>>       /*
>>>>>        * Ensure we don't send the STOP for non-SET_BLOCK_COUNTED
>>>>>        * requests if Auto-CMD12 is enabled.
>>>>> @@ -2658,6 +2890,8 @@ static bool sdhci_request_done(struct sdhci_host *host)
>>>>>                               dma_unmap_sg(mmc_dev(host->mmc), data->sg,
>>>>>                                            data->sg_len,
>>>>>                                            mmc_get_dma_dir(data));
>>>>> +                             if (host->use_external_dma)
>>>>> +                                     sdhci_external_dma_cleanup(host, data);
>>>>
>>>> Is sdhci_external_dma_cleanup() only needed in the error case?
>>>>
>>>> The DMA must be stopped before the memory is unmapped and potentially freed.
>>>>
>>>> Isn't the DMA cleanup also needed in the bounce buffer case?
>>>>
>>>> Isn't the DMA cleanup also needed in the COOKIE_PRE_MAPPED case?
>>>>
>>>> dmaengine_terminate_async() doesn't stop the DMA but
>>>> dmaengine_terminate_sync() is not atomic, which looks like a problem.
>>>>
>>>> Perhaps you look at scheduling some work for the external dma error case
>>>> instead of calling __sdhci_finish_mrq()?  Then the work can do the
>>>> dmaengine_terminate_sync() and call __sdhci_finish_mrq().
>>>
>>> Ok, I will look at these issues.
>>>
>>>>
>>>>>                       }
>>>>>                       data->host_cookie = COOKIE_UNMAPPED;
>>>>>               }
>>>>> @@ -3692,12 +3926,15 @@ int sdhci_setup_host(struct sdhci_host *host)
>>>>>                      mmc_hostname(mmc), host->version);
>>>>>       }
>>>>>
>>>>> -     if (host->quirks & SDHCI_QUIRK_FORCE_DMA)
>>>>> +     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 */
>>>>> +     } else {
>>>>>               host->flags |= SDHCI_USE_SDMA;
>>>>> +     }
>>>>
>>>> These if-statements are about setting SDHCI_USE_SDMA but why is a change
>>>> needed for the host->use_external_dma case?
>>>
>>> Yes, this is not needed, otherwise the controller cannot switch back
>>> to SDMA if it is supported in the controller.
>>
>> The else case means that the host always uses SDMA. Needed to make sure
>> this doesn't happen in the external_dma case. Its not SDHCI_CAN_DO_SDMA,
>> its SDHCI_USE_SDMA.
>>
>>>
>>>>
>>>>>
>>>>>       if ((host->quirks & SDHCI_QUIRK_BROKEN_DMA) &&
>>>>>               (host->flags & SDHCI_USE_SDMA)) {
>>>>> @@ -3785,6 +4022,19 @@ int sdhci_setup_host(struct sdhci_host *host)
>>>>>               }
>>>>>       }
>>>>>
>>>>> +     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 we use DMA, then it's up to the caller to set the DMA
>>>>>        * mask, but PIO does not need the hw shim so we set a new
>>>>> @@ -4201,6 +4451,10 @@ void sdhci_cleanup_host(struct sdhci_host *host)
>>>>>               dma_free_coherent(mmc_dev(mmc), host->align_buffer_sz +
>>>>>                                 host->adma_table_sz, host->align_buffer,
>>>>>                                 host->align_addr);
>>>>> +
>>>>> +     if (host->use_external_dma)
>>>>> +             sdhci_external_dma_release(host);
>>>>> +
>>>>>       host->adma_table = NULL;
>>>>>       host->align_buffer = NULL;
>>>>>  }
>>>>> @@ -4247,6 +4501,7 @@ int __sdhci_add_host(struct sdhci_host *host)
>>>>>
>>>>>       pr_info("%s: SDHCI controller on %s [%s] using %s\n",
>>>>>               mmc_hostname(mmc), host->hw_name, dev_name(mmc_dev(mmc)),
>>>>> +             host->use_external_dma ? "External DMA" :
>>>>>               (host->flags & SDHCI_USE_ADMA) ?
>>>>>               (host->flags & SDHCI_USE_64_BIT_DMA) ? "ADMA 64-bit" : "ADMA" :
>>>>>               (host->flags & SDHCI_USE_SDMA) ? "DMA" : "PIO");
>>>>> @@ -4335,6 +4590,9 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
>>>>>                                 host->adma_table_sz, host->align_buffer,
>>>>>                                 host->align_addr);
>>>>>
>>>>> +     if (host->use_external_dma)
>>>>> +             sdhci_external_dma_release(host);
>>>>> +
>>>>>       host->adma_table = NULL;
>>>>>       host->align_buffer = NULL;
>>>>>  }
>>>>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>>>>> index 6cc9a3c2ac66..7a52823ebef4 100644
>>>>> --- a/drivers/mmc/host/sdhci.h
>>>>> +++ b/drivers/mmc/host/sdhci.h
>>>>> @@ -482,6 +482,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;
>>>>> @@ -531,6 +532,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_external_dma; /* Host selects to use external DMA */
>>>>
>>>> Please align /**/ with above i.e. use tab
>>
>> Will fix.
> 
> Do you need me to address these comments and then send you a patch or
> you'd like to do instead?
> 

This would be much appreciated. Would you be able to work on it this
week? I would like to send a v2 by Friday.

Thanks,
Faiz

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

[Chunyan Zhang]

Hi Faiz，

On Tue, 29 Jan 2019 at 19:50, Faiz Abbas <faiz_abbas@ti.com> wrote:
>
> Chunyan,
>
> On 28/01/19 5:16 PM, Chunyan Zhang wrote:
> > On Mon, 28 Jan 2019 at 18:40, Faiz Abbas <faiz_abbas@ti.com> wrote:
> >>
> >> Hi,
> >>
> >> On 28/01/19 3:50 PM, Chunyan Zhang wrote:
> >>> On Thu, 24 Jan 2019 at 19:41, Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>>>
> >>>> On 11/01/19 1:08 PM, 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
> >>>>> synchronize once at the start of each request.
> >>>>
> >>>> Sorry for the slow reply, but I do have some concerns.  Please see the comments.
> >>>>
> >>>>>
> >>>>> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
> >>>>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
> >>>>> ---
> >>>>>  drivers/mmc/host/Kconfig |   3 +
> >>>>>  drivers/mmc/host/sdhci.c | 266 ++++++++++++++++++++++++++++++++++++++-
> >>>>>  drivers/mmc/host/sdhci.h |   8 ++
> >>>>>  3 files changed, 273 insertions(+), 4 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> >>>>> index e26b8145efb3..333292e8ecdd 100644
> >>>>> --- a/drivers/mmc/host/Kconfig
> >>>>> +++ b/drivers/mmc/host/Kconfig
> >>>>> @@ -999,3 +999,6 @@ config MMC_SDHCI_AM654
> >>>>>         If you have a controller with this interface, say Y or M here.
> >>>>>
> >>>>>         If unsure, say N.
> >>>>> +
> >>>>> +config MMC_SDHCI_EXTERNAL_DMA
> >>>>> +        bool
> >>>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> >>>>> index a22e11a65658..4a9044c06e21 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>
> >>>>> @@ -1118,6 +1119,226 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
> >>>>>       }
> >>>>>  }
> >>>>>
> >>>>> +#if IS_ENABLED(CONFIG_MMC_SDHCI_EXTERNAL_DMA)
> >>>>> +static int sdhci_external_dma_init(struct sdhci_host *host)
> >>>>> +{
> >>>>> +     int ret = 0;
> >>>>> +     struct mmc_host *mmc = host->mmc;
> >>>>> +
> >>>>> +     host->tx_chan = dma_request_chan(mmc->parent, "tx");
> >>>>> +     if (IS_ERR(host->tx_chan)) {
> >>>>> +             ret = PTR_ERR(host->tx_chan);
> >>>>> +             if (ret != -EPROBE_DEFER)
> >>>>> +                     pr_warn("Failed to request TX DMA channel.\n");
> >>>>> +             host->tx_chan = NULL;
> >>>>> +             return ret;
> >>>>> +     }
> >>>>> +
> >>>>> +     host->rx_chan = dma_request_chan(mmc->parent, "rx");
> >>>>> +     if (IS_ERR(host->rx_chan)) {
> >>>>> +             if (host->tx_chan) {
> >>>>> +                     dma_release_channel(host->tx_chan);
> >>>>> +                     host->tx_chan = NULL;
> >>>>> +             }
> >>>>> +
> >>>>> +             ret = PTR_ERR(host->rx_chan);
> >>>>> +             if (ret != -EPROBE_DEFER)
> >>>>> +                     pr_warn("Failed to request RX DMA channel.\n");
> >>>>> +             host->rx_chan = NULL;
> >>>>> +     }
> >>>>> +
> >>>>> +     return ret;
> >>>>> +}
> >>>>> +
> >>>>> +static inline struct dma_chan *
> >>>>> +sdhci_external_dma_channel(struct sdhci_host *host, struct mmc_data *data)
> >>>>> +{
> >>>>> +     return data->flags & MMC_DATA_WRITE ? host->tx_chan : host->rx_chan;
> >>>>> +}
> >>>>> +
> >>>>> +static int sdhci_external_dma_setup(struct sdhci_host *host,
> >>>>> +                                 struct mmc_command *cmd)
> >>>>> +{
> >>>>> +     int ret, i;
> >>>>> +     struct dma_async_tx_descriptor *desc;
> >>>>> +     struct mmc_data *data = cmd->data;
> >>>>> +     struct dma_chan *chan;
> >>>>> +     struct dma_slave_config cfg;
> >>>>> +     dma_cookie_t cookie;
> >>>>> +     int sg_cnt;
> >>>>> +
> >>>>> +     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_external_dma_channel(host, data);
> >>>>> +
> >>>>> +     ret = dmaengine_slave_config(chan, &cfg);
> >>>>> +     if (ret)
> >>>>> +             return ret;
> >>>>> +
> >>>>> +     sg_cnt = sdhci_pre_dma_transfer(host, data, COOKIE_MAPPED);
> >>>>> +     if (sg_cnt <= 0)
> >>>>> +             return -EINVAL;
> >>>>> +
> >>>>> +     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;
> >>>>> +
> >>>>> +     cookie = dmaengine_submit(desc);
> >>>>> +     if (cookie < 0)
> >>>>> +             ret = cookie;
> >>>>> +
> >>>>> +     return ret;
> >>>>> +}
> >>>>> +
> >>>>> +static void sdhci_external_dma_release(struct sdhci_host *host)
> >>>>> +{
> >>>>> +     if (host->tx_chan) {
> >>>>> +             dma_release_channel(host->tx_chan);
> >>>>> +             host->tx_chan = NULL;
> >>>>> +     }
> >>>>> +
> >>>>> +     if (host->rx_chan) {
> >>>>> +             dma_release_channel(host->rx_chan);
> >>>>> +             host->rx_chan = NULL;
> >>>>> +     }
> >>>>> +
> >>>>> +     sdhci_switch_external_dma(host, false);
> >>>>> +}
> >>>>> +
> >>>>> +static int __sdhci_external_dma_prepare_data(struct sdhci_host *host,
> >>>>> +                                          struct mmc_command *cmd)
> >>>>> +{
> >>>>> +     struct mmc_data *data = cmd->data;
> >>>>> +
> >>>>> +     host->data_timeout = 0;
> >>>>> +
> >>>>> +     if (sdhci_data_line_cmd(cmd))
> >>>>> +             sdhci_set_timeout(host, cmd);
> >>>>> +
> >>>>> +     WARN_ON(host->data);
> >>>>> +
> >>>>> +     /* Sanity checks */
> >>>>> +     WARN_ON(data->blksz * data->blocks > 524288);
> >>>>> +     WARN_ON(data->blksz > host->mmc->max_blk_size);
> >>>>> +     WARN_ON(data->blocks > 65535);
> >>>>> +
> >>>>> +     host->flags |= SDHCI_REQ_USE_DMA;
> >>>>> +     host->data = data;
> >>>>> +     host->data_early = 0;
> >>>>> +     host->data->bytes_xfered = 0;
> >>>>> +
> >>>>> +     sdhci_set_transfer_irqs(host);
> >>>>> +
> >>>>> +     /*
> >>>>> +      * For Version 4.10 onwards, if v4 mode is enabled, 32-bit Block Count
> >>>>> +      * can be supported, in that case 16-bit block count register must be 0.
> >>>>> +      */
> >>>>> +     if (host->version >= SDHCI_SPEC_410 && host->v4_mode &&
> >>>>> +         (host->quirks2 & SDHCI_QUIRK2_USE_32BIT_BLK_CNT)) {
> >>>>> +             if (sdhci_readw(host, SDHCI_BLOCK_COUNT))
> >>>>> +                     sdhci_writew(host, 0, SDHCI_BLOCK_COUNT);
> >>>>> +             sdhci_writew(host, data->blocks, SDHCI_32BIT_BLK_CNT);
> >>>>> +     } else {
> >>>>> +             sdhci_writew(host, data->blocks, SDHCI_BLOCK_COUNT);
> >>>>> +     }
> >>>>
> >>>> It is probably worth factoring out the code that is shared with
> >>>> sdhci_prepare_data() where possible.
> >>>>
> >>>>> +
> >>>>> +     return 0;
> >>>>> +}
> >>>>> +
> >>>>> +static void sdhci_external_dma_prepare_data(struct sdhci_host *host,
> >>>>> +                                         struct mmc_command *cmd)
> >>>>> +{
> >>>>> +     struct mmc_data *data = cmd->data;
> >>>>> +
> >>>>> +     if (!data)
> >>>>> +             return;
> >>>>
> >>>> Even in the !data case, we still need to set up a timeout for commands with
> >>>> busy waiting.  I suggest checking the !data case before calling
> >>>> sdhci_external_dma_prepare_data()
> >>>
> >>> Ok.
> >>>
> >>>>
> >>>>> +
> >>>>> +     if (sdhci_external_dma_setup(host, cmd) ||
> >>>>> +         __sdhci_external_dma_prepare_data(host, cmd)) {
> >>>>> +             sdhci_external_dma_release(host);
> >>>>> +             pr_err("%s: Cannot use external DMA, switch to the DMA/PIO which standard SDHCI provides.\n",
> >>>>> +                    mmc_hostname(host->mmc));
> >>>>> +             sdhci_prepare_data(host, cmd);
> >>>>> +     }
> >>>>> +}
> >>>>> +
> >>>>> +static void sdhci_external_dma_pre_transfer(struct sdhci_host *host,
> >>>>> +                                         struct mmc_command *cmd)
> >>>>> +{
> >>>>> +     struct dma_chan *chan;
> >>>>> +
> >>>>> +     if (!cmd->data || cmd->opcode == MMC_SET_BLOCK_COUNT)
> >>>>
> >>>> MMC_SET_BLOCK_COUNT never has cmd->data and so does not need to be checked.
> >>>
> >>> Ok.
> >>>
> >>>>
> >>>>> +             return;
> >>>>> +
> >>>>> +     sdhci_writew(host, cmd->data->blksz, SDHCI_BLOCK_SIZE);
> >>>>
> >>>> Block size is set in __sdhci_external_dma_prepare_data() so does it need to
> >>>> be set here also.
> >>>
> >>> Ok.
> >>
> >> That is BLOCK_COUNT. This is BLOCK_SIZE.
> >>
> >>>
> >>>>
> >>>>> +     chan = sdhci_external_dma_channel(host, cmd->data);
> >>>>> +     if (chan)
> >>>>> +             dma_async_issue_pending(chan);
> >>>>> +}
> >>>>> +
> >>>>> +static int sdhci_external_dma_cleanup(struct sdhci_host *host,
> >>>>> +                                 struct mmc_data *data)
> >>>>
> >>>> Please align parameters with open parenthesis
> >>>>
> >>>>> +{
> >>>>> +     struct dma_chan *chan = sdhci_external_dma_channel(host, data);
> >>>>> +     int ret = 0;
> >>>>> +
> >>>>> +     if (chan)
> >>>>> +             ret = dmaengine_terminate_async(chan);
> >>>>> +
> >>>>> +     return ret;
> >>>>> +}
> >>>>> +#else
> >>>>> +static int sdhci_external_dma_init(struct sdhci_host *host)
> >>>>> +{
> >>>>> +     return -EOPNOTSUPP;
> >>>>> +}
> >>>>> +
> >>>>> +static void sdhci_external_dma_release(struct sdhci_host *host)
> >>>>> +{}
> >>>>> +
> >>>>> +static void sdhci_external_dma_prepare_data(struct sdhci_host *host,
> >>>>> +                                         struct mmc_command *cmd)
> >>>>> +{
> >>>>> +     /* If MMC_SDHCI_EXTERNAL_DMA not supported, PIO will be used */
> >>>>> +     sdhci_prepare_data(host, cmd);
> >>>>> +}
> >>>>> +
> >>>>> +static void sdhci_external_dma_pre_transfer(struct sdhci_host *host,
> >>>>> +                                         struct mmc_command *cmd)
> >>>>> +{}
> >>>>> +
> >>>>> +static int sdhci_external_dma_cleanup(struct sdhci_host *host,
> >>>>> +                                 struct mmc_data *data)
> >>>>
> >>>> Please align parameters with open parenthesis
> >>>>
> >>>>> +{
> >>>>> +     return 0;
> >>>>> +}
> >>>>> +#endif
> >>>>> +
> >>>>> +void sdhci_switch_external_dma(struct sdhci_host *host, bool en)
> >>>>> +{
> >>>>> +     host->use_external_dma = en;
> >>>>> +}
> >>>>> +EXPORT_SYMBOL_GPL(sdhci_switch_external_dma);
> >>>>> +
> >>>>>  static inline bool sdhci_auto_cmd12(struct sdhci_host *host,
> >>>>>                                   struct mmc_request *mrq)
> >>>>>  {
> >>>>> @@ -1374,7 +1595,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_external_dma)
> >>>>
> >>>> As mentioned above wrt sdhci_external_dma_prepare_data():
> >>>>
> >>>>         if (host->use_external_dma && cmd->data)
> >>>>
> >>>
> >>> Maybe we should move checking the !data case out of both
> >>> _prepare_data, and add it to sdhci_send_command() before calling
> >>> _prepare_data(), that's saying we can do set up a timeout in
> >>> sdhci_send_command().
> >>
> >> Lets just factor out everything in common as Adrian said. The !data,
> >> BLOCK_COUNT writes and busy waiting.
> >
> > Ok, and also BLOCK_SIZE which can be set in _prepare_data().
> >
> >>
> >>>
> >>>>> +             sdhci_external_dma_prepare_data(host, cmd);
> >>>>> +     else
> >>>>> +             sdhci_prepare_data(host, cmd);
> >>>>>
> >>>>>       sdhci_writel(host, cmd->arg, SDHCI_ARGUMENT);
> >>>>>
> >>>>> @@ -1416,6 +1640,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_external_dma)
> >>>>> +             sdhci_external_dma_pre_transfer(host, cmd);
> >>>>
> >>>> Why is sdhci_external_dma_pre_transfer() needed here - couldn't it be done
> >>>> in sdhci_external_dma_prepare_data()?
> >>>>
> >>>
> >>> I'm not sure dma_async_issue_pending() can be done so early in
> >>> sdhci_external_dma_prepare_data().
> >>>
> >>>>> +
> >>>>>       sdhci_writew(host, SDHCI_MAKE_CMD(cmd->opcode, flags), SDHCI_COMMAND);
> >>>>>  }
> >>>>>  EXPORT_SYMBOL_GPL(sdhci_send_command);
> >>>>> @@ -1781,6 +2008,11 @@ void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
> >>>>>
> >>>>>       sdhci_led_activate(host);
> >>>>>
> >>>>> +     if (host->use_external_dma && mrq->data) {
> >>>>> +             struct dma_chan *chan = sdhci_external_dma_channel(host,
> >>>>> +                                                                mrq->data);
> >>>>
> >>>> sdhci_external_dma_channel is not declared if
> >>>> !IS_ENABLED(CONFIG_MMC_SDHCI_EXTERNAL_DMA)
> >>>
> >>> Ok, I guess this can be moved to sdhci_external_dma_prepare_data().
> >>
> >> No. We have to synchronize from a guaranteed non-atomic context. Maybe
> >> add another empty sdhci_external_dma_channel definition in the #else part?
> >>
> >>>
> >>>>
> >>>>> +             dmaengine_synchronize(chan);
> >>>>
> >>>> So this is to cover for using dmaengine_terminate_async()?
> >>
> >> Yes.
> >>
> >>>
> >>> Ok.
> >>>
> >>>>
> >>>>> +     }
> >>>>>       /*
> >>>>>        * Ensure we don't send the STOP for non-SET_BLOCK_COUNTED
> >>>>>        * requests if Auto-CMD12 is enabled.
> >>>>> @@ -2658,6 +2890,8 @@ static bool sdhci_request_done(struct sdhci_host *host)
> >>>>>                               dma_unmap_sg(mmc_dev(host->mmc), data->sg,
> >>>>>                                            data->sg_len,
> >>>>>                                            mmc_get_dma_dir(data));
> >>>>> +                             if (host->use_external_dma)
> >>>>> +                                     sdhci_external_dma_cleanup(host, data);
> >>>>
> >>>> Is sdhci_external_dma_cleanup() only needed in the error case?
> >>>>
> >>>> The DMA must be stopped before the memory is unmapped and potentially freed.
> >>>>
> >>>> Isn't the DMA cleanup also needed in the bounce buffer case?
> >>>>
> >>>> Isn't the DMA cleanup also needed in the COOKIE_PRE_MAPPED case?
> >>>>
> >>>> dmaengine_terminate_async() doesn't stop the DMA but
> >>>> dmaengine_terminate_sync() is not atomic, which looks like a problem.
> >>>>
> >>>> Perhaps you look at scheduling some work for the external dma error case
> >>>> instead of calling __sdhci_finish_mrq()?  Then the work can do the
> >>>> dmaengine_terminate_sync() and call __sdhci_finish_mrq().
> >>>
> >>> Ok, I will look at these issues.
> >>>
> >>>>
> >>>>>                       }
> >>>>>                       data->host_cookie = COOKIE_UNMAPPED;
> >>>>>               }
> >>>>> @@ -3692,12 +3926,15 @@ int sdhci_setup_host(struct sdhci_host *host)
> >>>>>                      mmc_hostname(mmc), host->version);
> >>>>>       }
> >>>>>
> >>>>> -     if (host->quirks & SDHCI_QUIRK_FORCE_DMA)
> >>>>> +     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 */
> >>>>> +     } else {
> >>>>>               host->flags |= SDHCI_USE_SDMA;
> >>>>> +     }
> >>>>
> >>>> These if-statements are about setting SDHCI_USE_SDMA but why is a change
> >>>> needed for the host->use_external_dma case?
> >>>
> >>> Yes, this is not needed, otherwise the controller cannot switch back
> >>> to SDMA if it is supported in the controller.
> >>
> >> The else case means that the host always uses SDMA. Needed to make sure
> >> this doesn't happen in the external_dma case. Its not SDHCI_CAN_DO_SDMA,
> >> its SDHCI_USE_SDMA.
> >>
> >>>
> >>>>
> >>>>>
> >>>>>       if ((host->quirks & SDHCI_QUIRK_BROKEN_DMA) &&
> >>>>>               (host->flags & SDHCI_USE_SDMA)) {
> >>>>> @@ -3785,6 +4022,19 @@ int sdhci_setup_host(struct sdhci_host *host)
> >>>>>               }
> >>>>>       }
> >>>>>
> >>>>> +     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 we use DMA, then it's up to the caller to set the DMA
> >>>>>        * mask, but PIO does not need the hw shim so we set a new
> >>>>> @@ -4201,6 +4451,10 @@ void sdhci_cleanup_host(struct sdhci_host *host)
> >>>>>               dma_free_coherent(mmc_dev(mmc), host->align_buffer_sz +
> >>>>>                                 host->adma_table_sz, host->align_buffer,
> >>>>>                                 host->align_addr);
> >>>>> +
> >>>>> +     if (host->use_external_dma)
> >>>>> +             sdhci_external_dma_release(host);
> >>>>> +
> >>>>>       host->adma_table = NULL;
> >>>>>       host->align_buffer = NULL;
> >>>>>  }
> >>>>> @@ -4247,6 +4501,7 @@ int __sdhci_add_host(struct sdhci_host *host)
> >>>>>
> >>>>>       pr_info("%s: SDHCI controller on %s [%s] using %s\n",
> >>>>>               mmc_hostname(mmc), host->hw_name, dev_name(mmc_dev(mmc)),
> >>>>> +             host->use_external_dma ? "External DMA" :
> >>>>>               (host->flags & SDHCI_USE_ADMA) ?
> >>>>>               (host->flags & SDHCI_USE_64_BIT_DMA) ? "ADMA 64-bit" : "ADMA" :
> >>>>>               (host->flags & SDHCI_USE_SDMA) ? "DMA" : "PIO");
> >>>>> @@ -4335,6 +4590,9 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
> >>>>>                                 host->adma_table_sz, host->align_buffer,
> >>>>>                                 host->align_addr);
> >>>>>
> >>>>> +     if (host->use_external_dma)
> >>>>> +             sdhci_external_dma_release(host);
> >>>>> +
> >>>>>       host->adma_table = NULL;
> >>>>>       host->align_buffer = NULL;
> >>>>>  }
> >>>>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> >>>>> index 6cc9a3c2ac66..7a52823ebef4 100644
> >>>>> --- a/drivers/mmc/host/sdhci.h
> >>>>> +++ b/drivers/mmc/host/sdhci.h
> >>>>> @@ -482,6 +482,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;
> >>>>> @@ -531,6 +532,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_external_dma; /* Host selects to use external DMA */
> >>>>
> >>>> Please align /**/ with above i.e. use tab
> >>
> >> Will fix.
> >
> > Do you need me to address these comments and then send you a patch or
> > you'd like to do instead?
> >
>
> This would be much appreciated. Would you be able to work on it this
> week? I would like to send a v2 by Friday.

I will try. I just have worries about handling the external dma error case.
Anyway I'll send you a patch by tomorrow, since I will leave 10 days
from this Friday.

Thanks,
Chunyan

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

[Chunyan Zhang]

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
synchronize once at the start of each request.

Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
Changes from the last version:
* Moved sdhci_set_timeout() from _prepare_data() to its caller -
  sdhci_send_command();
* Factor out a new function sdhci_reset_data() which deal with sanity
  checks for mmc_data and reset for host->data, these processes were
  in _prepare_data();
* Factor out a new function sdhci_set_block_info() for configuring data
  blocks which were processing at bottom of _prepare_data();
* Added a new tasklet and functions for handling external dma  error case;
* Removed sdhci_external_dma_cleanup() which is not used;
* Added an empty sdhci_external_dma_channel definition for
  !IS_ENABLED(CONFIG_MMC_SDHCI_EXTERNAL_DMA);
* Addressed some other comments from Adrian:
- Removed check for MMC_SET_BLOCK_COUNT, left checking !cmd->data only
  which is enough.
---
 drivers/mmc/host/Kconfig |   3 +
 drivers/mmc/host/sdhci.c | 333 +++++++++++++++++++++++++++++++++++----
 drivers/mmc/host/sdhci.h |  10 ++
 3 files changed, 317 insertions(+), 29 deletions(-)

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index e26b8145efb3..333292e8ecdd 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -999,3 +999,6 @@ config MMC_SDHCI_AM654
 	  If you have a controller with this interface, say Y or M here.
 
 	  If unsure, say N.
+
+config MMC_SDHCI_EXTERNAL_DMA
+        bool
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index a22e11a65658..8b056bb9422d 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>
@@ -987,18 +988,9 @@ static void sdhci_set_timeout(struct sdhci_host *host, struct mmc_command *cmd)
 	}
 }
 
-static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
+static inline void sdhci_reset_data(struct sdhci_host *host,
+				    struct mmc_data *data)
 {
-	struct mmc_data *data = cmd->data;
-
-	host->data_timeout = 0;
-
-	if (sdhci_data_line_cmd(cmd))
-		sdhci_set_timeout(host, cmd);
-
-	if (!data)
-		return;
-
 	WARN_ON(host->data);
 
 	/* Sanity checks */
@@ -1009,6 +1001,34 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
 	host->data = data;
 	host->data_early = 0;
 	host->data->bytes_xfered = 0;
+}
+
+static inline void sdhci_set_block_info(struct sdhci_host *host)
+{
+
+	/* Set the DMA boundary value and block size */
+	sdhci_writew(host,
+		     SDHCI_MAKE_BLKSZ(host->sdma_boundary, host->data->blksz),
+		     SDHCI_BLOCK_SIZE);
+	/*
+	 * For Version 4.10 onwards, if v4 mode is enabled, 32-bit Block Count
+	 * can be supported, in that case 16-bit block count register must be 0.
+	 */
+	if (host->version >= SDHCI_SPEC_410 && host->v4_mode &&
+	    (host->quirks2 & SDHCI_QUIRK2_USE_32BIT_BLK_CNT)) {
+		if (sdhci_readw(host, SDHCI_BLOCK_COUNT))
+			sdhci_writew(host, 0, SDHCI_BLOCK_COUNT);
+		sdhci_writew(host, host->data->blocks, SDHCI_32BIT_BLK_CNT);
+	} else {
+		sdhci_writew(host, host->data->blocks, SDHCI_BLOCK_COUNT);
+	}
+}
+
+static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
+{
+	struct mmc_data *data = cmd->data;
+
+	sdhci_reset_data(host, data);
 
 	if (host->flags & (SDHCI_USE_SDMA | SDHCI_USE_ADMA)) {
 		struct scatterlist *sg;
@@ -1100,24 +1120,227 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
 
 	sdhci_set_transfer_irqs(host);
 
-	/* Set the DMA boundary value and block size */
-	sdhci_writew(host, SDHCI_MAKE_BLKSZ(host->sdma_boundary, data->blksz),
-		     SDHCI_BLOCK_SIZE);
+	sdhci_set_block_info(host);
+}
 
-	/*
-	 * For Version 4.10 onwards, if v4 mode is enabled, 32-bit Block Count
-	 * can be supported, in that case 16-bit block count register must be 0.
-	 */
-	if (host->version >= SDHCI_SPEC_410 && host->v4_mode &&
-	    (host->quirks2 & SDHCI_QUIRK2_USE_32BIT_BLK_CNT)) {
-		if (sdhci_readw(host, SDHCI_BLOCK_COUNT))
-			sdhci_writew(host, 0, SDHCI_BLOCK_COUNT);
-		sdhci_writew(host, data->blocks, SDHCI_32BIT_BLK_CNT);
+#if IS_ENABLED(CONFIG_MMC_SDHCI_EXTERNAL_DMA)
+static int sdhci_external_dma_init(struct sdhci_host *host)
+{
+	int ret = 0;
+	struct mmc_host *mmc = host->mmc;
+
+	host->tx_chan = dma_request_chan(mmc->parent, "tx");
+	if (IS_ERR(host->tx_chan)) {
+		ret = PTR_ERR(host->tx_chan);
+		if (ret != -EPROBE_DEFER)
+			pr_warn("Failed to request TX DMA channel.\n");
+		host->tx_chan = NULL;
+		return ret;
+	}
+
+	host->rx_chan = dma_request_chan(mmc->parent, "rx");
+	if (IS_ERR(host->rx_chan)) {
+		if (host->tx_chan) {
+			dma_release_channel(host->tx_chan);
+			host->tx_chan = NULL;
+		}
+
+		ret = PTR_ERR(host->rx_chan);
+		if (ret != -EPROBE_DEFER)
+			pr_warn("Failed to request RX DMA channel.\n");
+		host->rx_chan = NULL;
+	}
+
+	return ret;
+}
+
+static inline struct dma_chan *
+sdhci_external_dma_channel(struct sdhci_host *host, struct mmc_data *data)
+{
+	return data->flags & MMC_DATA_WRITE ? host->tx_chan : host->rx_chan;
+}
+
+static int sdhci_external_dma_setup(struct sdhci_host *host,
+				    struct mmc_command *cmd)
+{
+	int ret, i;
+	struct dma_async_tx_descriptor *desc;
+	struct mmc_data *data = cmd->data;
+	struct dma_chan *chan;
+	struct dma_slave_config cfg;
+	dma_cookie_t cookie;
+	int sg_cnt;
+
+	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_external_dma_channel(host, data);
+
+	ret = dmaengine_slave_config(chan, &cfg);
+	if (ret)
+		return ret;
+
+	sg_cnt = sdhci_pre_dma_transfer(host, data, COOKIE_MAPPED);
+	if (sg_cnt <= 0)
+		return -EINVAL;
+
+	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;
+
+	cookie = dmaengine_submit(desc);
+	if (cookie < 0)
+		ret = cookie;
+
+	return ret;
+}
+
+static void sdhci_external_dma_release(struct sdhci_host *host)
+{
+	if (host->tx_chan) {
+		dma_release_channel(host->tx_chan);
+		host->tx_chan = NULL;
+	}
+
+	if (host->rx_chan) {
+		dma_release_channel(host->rx_chan);
+		host->rx_chan = NULL;
+	}
+
+	sdhci_switch_external_dma(host, false);
+}
+
+static void __sdhci_external_dma_prepare_data(struct sdhci_host *host,
+					      struct mmc_command *cmd)
+{
+	struct mmc_data *data = cmd->data;
+
+	sdhci_reset_data(host, data);
+
+	host->flags |= SDHCI_REQ_USE_DMA;
+	sdhci_set_transfer_irqs(host);
+
+	sdhci_set_block_info(host);
+}
+
+static void sdhci_external_dma_prepare_data(struct sdhci_host *host,
+					    struct mmc_command *cmd)
+{
+	struct mmc_data *data = cmd->data;
+	struct dma_chan *chan = sdhci_external_dma_channel(host, data);
+
+	if (!sdhci_external_dma_setup(host, cmd)) {
+		__sdhci_external_dma_prepare_data(host, cmd);
 	} else {
-		sdhci_writew(host, data->blocks, SDHCI_BLOCK_COUNT);
+		sdhci_external_dma_release(host);
+		pr_err("%s: Cannot use external DMA, switch to the DMA/PIO which standard SDHCI provides.\n",
+		       mmc_hostname(host->mmc));
+		sdhci_prepare_data(host, cmd);
+	}
+}
+
+static void sdhci_external_dma_pre_transfer(struct sdhci_host *host,
+					    struct mmc_command *cmd)
+{
+	struct dma_chan *chan;
+
+	if (!cmd->data)
+		return;
+
+	chan = sdhci_external_dma_channel(host, cmd->data);
+	if (chan)
+		dma_async_issue_pending(chan);
+}
+
+static bool sdhci_external_dma_request_done(struct sdhci_host *host)
+{
+	struct mmc_request *mrq;
+	struct dma_chan *chan;
+	int i;
+
+	spin_lock_irqsave(&host->lock, flags);
+
+	for (i = 0; i < SDHCI_MAX_MRQS; i++) {
+		mrq = host->mrqs_done[i];
+		if (mrq)
+			break;
 	}
+
+	spin_unlock_irqrestore(&host->lock, flags);
+
+	if (!mrq)
+		return true;
+
+	sdhci_tasklet_finish(host);
+
+	chan = sdhci_external_dma_channel(host, mrq->data);
+	dmaengine_synchronize(chan);
+
+	return false;
 }
 
+
+static void sdhci_external_dma_tasklet_finish(unsigned long param)
+{
+	struct sdhci_host *host = (struct sdhci_host *)param;
+
+	while (!sdhci_external_dma_request_done(host))
+		;
+}
+#else
+static int sdhci_external_dma_init(struct sdhci_host *host)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline struct dma_chan *
+sdhci_external_dma_channel(struct sdhci_host *host, struct mmc_data *data)
+{
+	return NULL;
+}
+
+static void sdhci_external_dma_release(struct sdhci_host *host)
+{}
+
+static void sdhci_external_dma_prepare_data(struct sdhci_host *host,
+					    struct mmc_command *cmd)
+{
+	/* If MMC_SDHCI_EXTERNAL_DMA not supported, PIO will be used */
+	sdhci_prepare_data(host, cmd);
+}
+
+static void sdhci_external_dma_pre_transfer(struct sdhci_host *host,
+					    struct mmc_command *cmd)
+{}
+
+static void sdhci_external_dma_tasklet_finish(unsigned long param)
+{}
+#endif
+
+void sdhci_switch_external_dma(struct sdhci_host *host, bool en)
+{
+	host->use_external_dma = en;
+}
+EXPORT_SYMBOL_GPL(sdhci_switch_external_dma);
+
 static inline bool sdhci_auto_cmd12(struct sdhci_host *host,
 				    struct mmc_request *mrq)
 {
@@ -1215,6 +1438,7 @@ static bool sdhci_needs_reset(struct sdhci_host *host, struct mmc_request *mrq)
 static void __sdhci_finish_mrq(struct sdhci_host *host, struct mmc_request *mrq)
 {
 	int i;
+	struct dma_chan *chan;
 
 	for (i = 0; i < SDHCI_MAX_MRQS; i++) {
 		if (host->mrqs_done[i] == mrq) {
@@ -1232,7 +1456,13 @@ static void __sdhci_finish_mrq(struct sdhci_host *host, struct mmc_request *mrq)
 
 	WARN_ON(i >= SDHCI_MAX_MRQS);
 
-	tasklet_schedule(&host->finish_tasklet);
+	if (host->use_external_dma) {
+		chan = sdhci_external_dma_channel(host, mrq->data);
+		dmaengine_terminate_async(chan);
+		tasklet_schedule(&host->external_dma_finish_tasklet);
+	} else {
+		tasklet_schedule(&host->finish_tasklet);
+	}
 }
 
 static void sdhci_finish_mrq(struct sdhci_host *host, struct mmc_request *mrq)
@@ -1369,12 +1599,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);
+	}
 
 	sdhci_writel(host, cmd->arg, SDHCI_ARGUMENT);
 
@@ -1416,6 +1653,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_external_dma)
+		sdhci_external_dma_pre_transfer(host, cmd);
+
 	sdhci_writew(host, SDHCI_MAKE_CMD(cmd->opcode, flags), SDHCI_COMMAND);
 }
 EXPORT_SYMBOL_GPL(sdhci_send_command);
@@ -1771,6 +2011,7 @@ void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 	struct sdhci_host *host;
 	int present;
 	unsigned long flags;
+	struct dma_chan *chan;
 
 	host = mmc_priv(mmc);
 
@@ -1781,6 +2022,12 @@ void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 
 	sdhci_led_activate(host);
 
+	if (host->use_external_dma && mrq->data) {
+		chan = sdhci_external_dma_channel(host, mrq->data);
+		if (chan)
+			dmaengine_terminate_async(chan);
+	}
+
 	/*
 	 * Ensure we don't send the STOP for non-SET_BLOCK_COUNTED
 	 * requests if Auto-CMD12 is enabled.
@@ -3692,12 +3939,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 */
+	} else {
 		host->flags |= SDHCI_USE_SDMA;
+	}
 
 	if ((host->quirks & SDHCI_QUIRK_BROKEN_DMA) &&
 		(host->flags & SDHCI_USE_SDMA)) {
@@ -4201,6 +4464,10 @@ void sdhci_cleanup_host(struct sdhci_host *host)
 		dma_free_coherent(mmc_dev(mmc), host->align_buffer_sz +
 				  host->adma_table_sz, host->align_buffer,
 				  host->align_addr);
+
+	if (host->use_external_dma)
+		sdhci_external_dma_release(host);
+
 	host->adma_table = NULL;
 	host->align_buffer = NULL;
 }
@@ -4216,6 +4483,8 @@ int __sdhci_add_host(struct sdhci_host *host)
 	 */
 	tasklet_init(&host->finish_tasklet,
 		sdhci_tasklet_finish, (unsigned long)host);
+	tasklet_init(&host->external_dma_finish_tasklet,
+		     sdhci_external_dma_tasklet_finish, (unsigned long)host);
 
 	timer_setup(&host->timer, sdhci_timeout_timer, 0);
 	timer_setup(&host->data_timer, sdhci_timeout_data_timer, 0);
@@ -4247,6 +4516,7 @@ int __sdhci_add_host(struct sdhci_host *host)
 
 	pr_info("%s: SDHCI controller on %s [%s] using %s\n",
 		mmc_hostname(mmc), host->hw_name, dev_name(mmc_dev(mmc)),
+		host->use_external_dma ? "External DMA" :
 		(host->flags & SDHCI_USE_ADMA) ?
 		(host->flags & SDHCI_USE_64_BIT_DMA) ? "ADMA 64-bit" : "ADMA" :
 		(host->flags & SDHCI_USE_SDMA) ? "DMA" : "PIO");
@@ -4264,6 +4534,7 @@ int __sdhci_add_host(struct sdhci_host *host)
 	free_irq(host->irq, host);
 untasklet:
 	tasklet_kill(&host->finish_tasklet);
+	tasklet_kill(&host->external_dma_finish_tasklet);
 
 	return ret;
 }
@@ -4326,6 +4597,7 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
 	del_timer_sync(&host->data_timer);
 
 	tasklet_kill(&host->finish_tasklet);
+	tasklet_kill(&host->external_dma_finish_tasklet);
 
 	if (!IS_ERR(mmc->supply.vqmmc))
 		regulator_disable(mmc->supply.vqmmc);
@@ -4335,6 +4607,9 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)
 				  host->adma_table_sz, host->align_buffer,
 				  host->align_addr);
 
+	if (host->use_external_dma)
+		sdhci_external_dma_release(host);
+
 	host->adma_table = NULL;
 	host->align_buffer = NULL;
 }
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 6cc9a3c2ac66..43f71b479e0b 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -482,6 +482,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;
@@ -531,6 +532,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_external_dma;	/* Host selects to use external DMA */
 
 	struct mmc_request *mrqs_done[SDHCI_MAX_MRQS];	/* Requests done */
 	struct mmc_command *cmd;	/* Current command */
@@ -555,10 +557,17 @@ struct sdhci_host {
 	unsigned int desc_sz;	/* ADMA descriptor size */
 
 	struct tasklet_struct finish_tasklet;	/* Tasklet structures */
+	/* Tasklet struction for using external dma */
+	struct tasklet_struct external_dma_finish_tasklet;
 
 	struct timer_list timer;	/* Timer for timeouts */
 	struct timer_list data_timer;	/* Timer for data timeouts */
 
+#if IS_ENABLED(CONFIG_MMC_SDHCI_EXTERNAL_DMA)
+	struct dma_chan	*rx_chan;
+	struct dma_chan	*tx_chan;
+#endif
+
 	u32 caps;		/* CAPABILITY_0 */
 	u32 caps1;		/* CAPABILITY_1 */
 	bool read_caps;		/* Capability flags have been read */
@@ -792,5 +801,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_external_dma(struct sdhci_host *host, bool en);
 
 #endif /* __SDHCI_HW_H */
-- 
2.17.1

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

[Faiz Abbas]

Chunyan,

On 30/01/19 4:35 PM, Chunyan Zhang wrote:
> 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
> synchronize once at the start of each request.
> 
> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
> ---
> Changes from the last version:
> * Moved sdhci_set_timeout() from _prepare_data() to its caller -
>   sdhci_send_command();
> * Factor out a new function sdhci_reset_data() which deal with sanity
>   checks for mmc_data and reset for host->data, these processes were
>   in _prepare_data();
> * Factor out a new function sdhci_set_block_info() for configuring data
>   blocks which were processing at bottom of _prepare_data();
> * Added a new tasklet and functions for handling external dma  error case;
> * Removed sdhci_external_dma_cleanup() which is not used;
> * Added an empty sdhci_external_dma_channel definition for
>   !IS_ENABLED(CONFIG_MMC_SDHCI_EXTERNAL_DMA);
> * Addressed some other comments from Adrian:
> - Removed check for MMC_SET_BLOCK_COUNT, left checking !cmd->data only
>   which is enough.

There should have been a v2 in the patch description. It lets reviewers
know that they have seen this patch before and its not the same thing. I
would have preferred you had me test this before posting it to the
mailing list. Otherwise there should be a "not tested" in the description.

> ---
>  drivers/mmc/host/Kconfig |   3 +
>  drivers/mmc/host/sdhci.c | 333 +++++++++++++++++++++++++++++++++++----
>  drivers/mmc/host/sdhci.h |  10 ++
>  3 files changed, 317 insertions(+), 29 deletions
[snip]

> +static void sdhci_external_dma_prepare_data(struct sdhci_host *host,
> +					    struct mmc_command *cmd)
> +{
> +	struct mmc_data *data = cmd->data;
> +	struct dma_chan *chan = sdhci_external_dma_channel(host, data);
chan is not being used in this function.
> +
> +	if (!sdhci_external_dma_setup(host, cmd)) {
> +		__sdhci_external_dma_prepare_data(host, cmd);
>  	} else {
> -		sdhci_writew(host, data->blocks, SDHCI_BLOCK_COUNT);
> +		sdhci_external_dma_release(host);
> +		pr_err("%s: Cannot use external DMA, switch to the DMA/PIO which standard SDHCI provides.\n",
> +		       mmc_hostname(host->mmc));
> +		sdhci_prepare_data(host, cmd);
> +	}
> +}
> +
> +static void sdhci_external_dma_pre_transfer(struct sdhci_host *host,
> +					    struct mmc_command *cmd)
> +{
> +	struct dma_chan *chan;

I think you wanted to initialize this one.

> +
> +	if (!cmd->data)
> +		return;
> +
> +	chan = sdhci_external_dma_channel(host, cmd->data);
> +	if (chan)
> +		dma_async_issue_pending(chan);
> +}
> +
> +static bool sdhci_external_dma_request_done(struct sdhci_host *host)
> +{
> +	struct mmc_request *mrq;
> +	struct dma_chan *chan;
> +	int i;
> +
> +	spin_lock_irqsave(&host->lock, flags);

flags is not declared.

> +
> +	for (i = 0; i < SDHCI_MAX_MRQS; i++) {
> +		mrq = host->mrqs_done[i];
> +		if (mrq)
> +			break;
>  	}
> +
> +	spin_unlock_irqrestore(&host->lock, flags);
> +
> +	if (!mrq)
> +		return true;
> +
> +	sdhci_tasklet_finish(host);

This doesn't even build. sdhci_tasklet_finish is declared below this. In
the future, if you don't have a platform to test things on, at least
build and use checkpatch before posting it to the mailing list.

Should we even be calling a tasklet callback function directly?
Shouldn't we do a tasklet_schedule()?

Thanks,
Faiz

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

[Faiz Abbas]

Hi Adrian,

On 24/01/19 5:10 PM, Adrian Hunter wrote:
> On 11/01/19 1:08 PM, 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
>> synchronize once at the start of each request.
> 
> Sorry for the slow reply, but I do have some concerns.  Please see the comments.
>[snip]>>  	/*
>>  	 * Ensure we don't send the STOP for non-SET_BLOCK_COUNTED
>>  	 * requests if Auto-CMD12 is enabled.
>> @@ -2658,6 +2890,8 @@ static bool sdhci_request_done(struct sdhci_host *host)
>>  				dma_unmap_sg(mmc_dev(host->mmc), data->sg,
>>  					     data->sg_len,
>>  					     mmc_get_dma_dir(data));
>> +				if (host->use_external_dma)
>> +					sdhci_external_dma_cleanup(host, data);
> 
> Is sdhci_external_dma_cleanup() only needed in the error case?
> 
> The DMA must be stopped before the memory is unmapped and potentially freed.
> 
> Isn't the DMA cleanup also needed in the bounce buffer case?
> 
> Isn't the DMA cleanup also needed in the COOKIE_PRE_MAPPED case?
> 
> dmaengine_terminate_async() doesn't stop the DMA but
> dmaengine_terminate_sync() is not atomic, which looks like a problem.
> 
> Perhaps you look at scheduling some work for the external dma error case
> instead of calling __sdhci_finish_mrq()?  Then the work can do the
> dmaengine_terminate_sync() and call __sdhci_finish_mrq().
> 

Why don't we get rid of the finish_tasklet and use the already existing
threaded_irq for everything? I tested the following patch out and it
seems to work well. This enables us to just call
dmaengine_terminate_sync() in sdhci_request_done().

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index a22e11a65658..beff2ac2dee5 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1232,7 +1232,7 @@ static void __sdhci_finish_mrq(struct sdhci_host
*host, struct mmc_request *mrq)

 	WARN_ON(i >= SDHCI_MAX_MRQS);

-	tasklet_schedule(&host->finish_tasklet);
+	host->threaded_irq_needed = true;
 }

 static void sdhci_finish_mrq(struct sdhci_host *host, struct
mmc_request *mrq)
@@ -2705,14 +2705,6 @@ static bool sdhci_request_done(struct sdhci_host
*host)
 	return false;
 }

-static void sdhci_tasklet_finish(unsigned long param)
-{
-	struct sdhci_host *host = (struct sdhci_host *)param;
-
-	while (!sdhci_request_done(host))
-		;
-}
-
 static void sdhci_timeout_timer(struct timer_list *t)
 {
 	struct sdhci_host *host;
@@ -3000,6 +2992,8 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
 	u32 intmask, mask, unexpected = 0;
 	int max_loops = 16;

+	host->threaded_irq_needed = false;
+
 	spin_lock(&host->lock);

 	if (host->runtime_suspended && !sdhci_sdio_irq_enabled(host)) {
@@ -3077,6 +3071,9 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
 			result = IRQ_WAKE_THREAD;
 		}

+		if (host->threaded_irq_needed)
+			result = IRQ_WAKE_THREAD;
+
 		intmask &= ~(SDHCI_INT_CARD_INSERT | SDHCI_INT_CARD_REMOVE |
 			     SDHCI_INT_CMD_MASK | SDHCI_INT_DATA_MASK |
 			     SDHCI_INT_ERROR | SDHCI_INT_BUS_POWER |
@@ -3131,6 +3128,10 @@ static irqreturn_t sdhci_thread_irq(int irq, void
*dev_id)
 		spin_unlock_irqrestore(&host->lock, flags);
 	}

+	do {
+		isr = sdhci_request_done(host);
+	} while(isr);
+
 	return isr ? IRQ_HANDLED : IRQ_NONE;
 }

@@ -4211,12 +4212,6 @@ int __sdhci_add_host(struct sdhci_host *host)
 	struct mmc_host *mmc = host->mmc;
 	int ret;

-	/*
-	 * Init tasklets.
-	 */
-	tasklet_init(&host->finish_tasklet,
-		sdhci_tasklet_finish, (unsigned long)host);
-
 	timer_setup(&host->timer, sdhci_timeout_timer, 0);
 	timer_setup(&host->data_timer, sdhci_timeout_data_timer, 0);

@@ -4229,7 +4224,7 @@ int __sdhci_add_host(struct sdhci_host *host)
 	if (ret) {
 		pr_err("%s: Failed to request IRQ %d: %d\n",
 		       mmc_hostname(mmc), host->irq, ret);
-		goto untasklet;
+		return ret;
 	}

 	ret = sdhci_led_register(host);
@@ -4262,8 +4257,6 @@ int __sdhci_add_host(struct sdhci_host *host)
 	sdhci_writel(host, 0, SDHCI_INT_ENABLE);
 	sdhci_writel(host, 0, SDHCI_SIGNAL_ENABLE);
 	free_irq(host->irq, host);
-untasklet:
-	tasklet_kill(&host->finish_tasklet);

 	return ret;
 }
@@ -4325,8 +4318,6 @@ void sdhci_remove_host(struct sdhci_host *host,
int dead)
 	del_timer_sync(&host->timer);
 	del_timer_sync(&host->data_timer);

-	tasklet_kill(&host->finish_tasklet);
-
 	if (!IS_ERR(mmc->supply.vqmmc))
 		regulator_disable(mmc->supply.vqmmc);

diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 6cc9a3c2ac66..abf4f77650b5 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -554,7 +554,7 @@ struct sdhci_host {

 	unsigned int desc_sz;	/* ADMA descriptor size */

-	struct tasklet_struct finish_tasklet;	/* Tasklet structures */
+	bool threaded_irq_needed;

 	struct timer_list timer;	/* Timer for timeouts */
 	struct timer_list data_timer;	/* Timer for data timeouts */

Thanks,
Faiz

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

[Faiz Abbas]

From: Chunyan Zhang <zhang.chunyan@linaro.org>

sdhci-omap can support both external dma controller via dmaengine framework
as well as ADMA which standard SD host controller provides.

Fixes by Faiz Abbas <fazi_abbas@ti.com>:
1. Switch to DMA slave mode when using external DMA
2. Add offset to mapbase

Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 drivers/mmc/host/Kconfig      |  1 +
 drivers/mmc/host/sdhci-omap.c | 16 +++++++++++++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 333292e8ecdd..70e90b00cb37 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -979,6 +979,7 @@ config MMC_SDHCI_OMAP
 	depends on MMC_SDHCI_PLTFM && OF
 	select THERMAL
 	select TI_SOC_THERMAL
+	select MMC_SDHCI_EXTERNAL_DMA if DMA_ENGINE
 	help
 	  This selects the Secure Digital Host Controller Interface (SDHCI)
 	  support present in TI's DRA7 SOCs. The controller supports
diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c
index c11c18a9aacb..8a05e94fe612 100644
--- a/drivers/mmc/host/sdhci-omap.c
+++ b/drivers/mmc/host/sdhci-omap.c
@@ -696,7 +696,11 @@ static int sdhci_omap_enable_dma(struct sdhci_host *host)
 	struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host);
 
 	reg = sdhci_omap_readl(omap_host, SDHCI_OMAP_CON);
-	reg |= CON_DMA_MASTER;
+	reg &= ~CON_DMA_MASTER;
+	/* Switch to DMA slave mode when using external DMA */
+	if (!host->use_external_dma)
+		reg |= CON_DMA_MASTER;
+
 	sdhci_omap_writel(omap_host, SDHCI_OMAP_CON, reg);
 
 	return 0;
@@ -1010,6 +1014,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)
@@ -1022,6 +1027,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)) {
@@ -1038,6 +1047,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 + offset;
 
 	mmc = host->mmc;
 	sdhci_get_of_property(pdev);
@@ -1105,6 +1115,10 @@ 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;
 
+	/* Switch to external DMA only if there is the "dmas" property */
+	if (of_find_property(dev->of_node, "dmas", NULL))
+		sdhci_switch_external_dma(host, true);
+
 	ret = sdhci_setup_host(host);
 	if (ret)
 		goto err_put_sync;
-- 
2.19.2

[PATCH 3/7] dt-bindings: sdhci-omap: Add example for using external dma

[Faiz Abbas]

From: Chunyan Zhang <zhang.chunyan@linaro.org>

sdhci-omap can support both external dma controller via dmaengine
framework as well as ADMA which standard SD host controller
provides.

Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 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 72c4dec7e1db..f8914a2a2aec 100644
--- a/Documentation/devicetree/bindings/mmc/sdhci-omap.txt
+++ b/Documentation/devicetree/bindings/mmc/sdhci-omap.txt
@@ -14,6 +14,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 {
@@ -22,4 +27,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.19.2

Re: [PATCH 3/7] dt-bindings: sdhci-omap: Add example for using external dma

[Rob Herring]

On Fri, Jan 11, 2019 at 04:38:47PM +0530, Faiz Abbas wrote:
> From: Chunyan Zhang <zhang.chunyan@linaro.org>

The subject is misleading. You are adding new properties, not just 
changing an example.

> 
> sdhci-omap can support both external dma controller via dmaengine
> framework as well as ADMA which standard SD host controller
> provides.
> 
> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
> ---
>  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 72c4dec7e1db..f8914a2a2aec 100644
> --- a/Documentation/devicetree/bindings/mmc/sdhci-omap.txt
> +++ b/Documentation/devicetree/bindings/mmc/sdhci-omap.txt
> @@ -14,6 +14,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.

The ordering of rx and tx should be defined and you have a mixture.

>  
>  Example:
>  	mmc1: mmc@4809c000 {
> @@ -22,4 +27,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.19.2
> 

[PATCH] dt-bindings: sdhci-omap: Add properties for using external dma

[Chunyan Zhang]

sdhci-omap can support both external dma controller via dmaengine
framework as well as ADMA which standard SD host controller
provides.

Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 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 72c4dec7e1db..4485dbceb373 100644
--- a/Documentation/devicetree/bindings/mmc/sdhci-omap.txt
+++ b/Documentation/devicetree/bindings/mmc/sdhci-omap.txt
@@ -14,6 +14,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 "tx"
+		and "rx" for TX and RX DMA requests, respectively.
 
 Example:
 	mmc1: mmc@4809c000 {
@@ -22,4 +27,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.17.1

Re: [PATCH] dt-bindings: sdhci-omap: Add properties for using external dma

[Faiz Abbas]

Hi Chunyan,

+Rob Herring

On 22/01/19 2:17 PM, Chunyan Zhang wrote:
> sdhci-omap can support both external dma controller via dmaengine
> framework as well as ADMA which standard SD host controller
> provides.
> 
> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
> ---

Thanks for fixing this. However, this change should be part of the
series (with a change log and a version number). I will send this as a
part of my series after I get some Acks on the driver patches.

Thanks,
Faiz

Re: [PATCH] dt-bindings: sdhci-omap: Add properties for using external dma

[Chunyan Zhang]

On Tue, 22 Jan 2019 at 18:17, Faiz Abbas <faiz_abbas@ti.com> wrote:
>
> Hi Chunyan,
>
> +Rob Herring
>
> On 22/01/19 2:17 PM, Chunyan Zhang wrote:
> > sdhci-omap can support both external dma controller via dmaengine
> > framework as well as ADMA which standard SD host controller
> > provides.
> >
> > Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
> > Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
> > ---
>
> Thanks for fixing this. However, this change should be part of the

Right, I actually used "--in-reply-to" with the parameter of
Message-ID of Rob's mail, but it seems not work as I expected that it
was expected a reply to Rob's mail for review.

> series (with a change log and a version number). I will send this as a
> part of my series after I get some Acks on the driver patches.

Ok, thanks.

Chunyan

>
> Thanks,
> Faiz

[PATCH 4/7] mmc: sdhci: Add quirk for disabling DTO during erase command

[Faiz Abbas]

Some controllers might prematurely issue a data timeout during an erase
command. Add a quirk to disable the interrupt when an erase command is
issued.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 drivers/mmc/host/sdhci.c | 8 ++++++++
 drivers/mmc/host/sdhci.h | 2 ++
 2 files changed, 10 insertions(+)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 4a9044c06e21..cfd716aee552 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1560,6 +1560,14 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 	/* Initially, a command has no error */
 	cmd->error = 0;
 
+	if (cmd->opcode == MMC_ERASE &&
+	    (host->quirks2 & SDHCI_QUIRK2_DISABLE_DTO_FOR_ERASE)) {
+		mask = sdhci_readl(host, SDHCI_INT_ENABLE);
+		mask &= ~SDHCI_INT_DATA_TIMEOUT;
+		sdhci_writel(host, mask, SDHCI_INT_ENABLE);
+		sdhci_writel(host, mask, SDHCI_SIGNAL_ENABLE);
+	}
+
 	if ((host->quirks2 & SDHCI_QUIRK2_STOP_WITH_TC) &&
 	    cmd->opcode == MMC_STOP_TRANSMISSION)
 		cmd->flags |= MMC_RSP_BUSY;
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 7a52823ebef4..d0c6d4fe5371 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -479,6 +479,8 @@ struct sdhci_host {
  * block count.
  */
 #define SDHCI_QUIRK2_USE_32BIT_BLK_CNT			(1<<18)
+/* Controller needs to disable DTO for erase command */
+#define SDHCI_QUIRK2_DISABLE_DTO_FOR_ERASE		(1<<19)
 
 	int irq;		/* Device IRQ */
 	void __iomem *ioaddr;	/* Mapped address */
-- 
2.19.2

Re: [PATCH 4/7] mmc: sdhci: Add quirk for disabling DTO during erase command

[Adrian Hunter]

On 11/01/19 1:08 PM, Faiz Abbas wrote:
> Some controllers might prematurely issue a data timeout during an erase
> command. Add a quirk to disable the interrupt when an erase command is
> issued.

I might have already asked this, but would it be possible to use the
existing SDHCI_QUIRK2_DISABLE_HW_TIMEOUT quirk?

> 
> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
> ---
>  drivers/mmc/host/sdhci.c | 8 ++++++++
>  drivers/mmc/host/sdhci.h | 2 ++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 4a9044c06e21..cfd716aee552 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1560,6 +1560,14 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
>  	/* Initially, a command has no error */
>  	cmd->error = 0;
>  
> +	if (cmd->opcode == MMC_ERASE &&
> +	    (host->quirks2 & SDHCI_QUIRK2_DISABLE_DTO_FOR_ERASE)) {
> +		mask = sdhci_readl(host, SDHCI_INT_ENABLE);
> +		mask &= ~SDHCI_INT_DATA_TIMEOUT;
> +		sdhci_writel(host, mask, SDHCI_INT_ENABLE);
> +		sdhci_writel(host, mask, SDHCI_SIGNAL_ENABLE);
> +	}
> +
>  	if ((host->quirks2 & SDHCI_QUIRK2_STOP_WITH_TC) &&
>  	    cmd->opcode == MMC_STOP_TRANSMISSION)
>  		cmd->flags |= MMC_RSP_BUSY;
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 7a52823ebef4..d0c6d4fe5371 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -479,6 +479,8 @@ struct sdhci_host {
>   * block count.
>   */
>  #define SDHCI_QUIRK2_USE_32BIT_BLK_CNT			(1<<18)
> +/* Controller needs to disable DTO for erase command */
> +#define SDHCI_QUIRK2_DISABLE_DTO_FOR_ERASE		(1<<19)
>  
>  	int irq;		/* Device IRQ */
>  	void __iomem *ioaddr;	/* Mapped address */
> 

Re: [PATCH 4/7] mmc: sdhci: Add quirk for disabling DTO during erase command

[Faiz Abbas]

Hi Adrian,

On 24/01/19 5:38 PM, Adrian Hunter wrote:
> On 11/01/19 1:08 PM, Faiz Abbas wrote:
>> Some controllers might prematurely issue a data timeout during an erase
>> command. Add a quirk to disable the interrupt when an erase command is
>> issued.
> 
> I might have already asked this, but would it be possible to use the
> existing SDHCI_QUIRK2_DISABLE_HW_TIMEOUT quirk?

This looks promising. Let me look into this. In any case, I should
probably move this to something like sdhci_set_timeout and use
sdhci_set_data_timeout_irq().

I was following 93caf8e69eac ("omap_hsmmc: add erase capability") for
adding this.

Thanks,
Faiz

> 
>>
>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
>> ---
>>  drivers/mmc/host/sdhci.c | 8 ++++++++
>>  drivers/mmc/host/sdhci.h | 2 ++
>>  2 files changed, 10 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 4a9044c06e21..cfd716aee552 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -1560,6 +1560,14 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
>>  	/* Initially, a command has no error */
>>  	cmd->error = 0;
>>  
>> +	if (cmd->opcode == MMC_ERASE &&
>> +	    (host->quirks2 & SDHCI_QUIRK2_DISABLE_DTO_FOR_ERASE)) {
>> +		mask = sdhci_readl(host, SDHCI_INT_ENABLE);
>> +		mask &= ~SDHCI_INT_DATA_TIMEOUT;
>> +		sdhci_writel(host, mask, SDHCI_INT_ENABLE);
>> +		sdhci_writel(host, mask, SDHCI_SIGNAL_ENABLE);
>> +	}
>> +
>>  	if ((host->quirks2 & SDHCI_QUIRK2_STOP_WITH_TC) &&
>>  	    cmd->opcode == MMC_STOP_TRANSMISSION)
>>  		cmd->flags |= MMC_RSP_BUSY;
>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>> index 7a52823ebef4..d0c6d4fe5371 100644
>> --- a/drivers/mmc/host/sdhci.h
>> +++ b/drivers/mmc/host/sdhci.h
>> @@ -479,6 +479,8 @@ struct sdhci_host {
>>   * block count.
>>   */
>>  #define SDHCI_QUIRK2_USE_32BIT_BLK_CNT			(1<<18)
>> +/* Controller needs to disable DTO for erase command */
>> +#define SDHCI_QUIRK2_DISABLE_DTO_FOR_ERASE		(1<<19)
>>  
>>  	int irq;		/* Device IRQ */
>>  	void __iomem *ioaddr;	/* Mapped address */
>>
> 

[PATCH 5/7] mmc: sdhci-omap: Add DISABLE_DTO_FOR_ERASE Quirk

[Faiz Abbas]

Add a Quirk to disable DTO during an erase command.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 drivers/mmc/host/sdhci-omap.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c
index 8a05e94fe612..ae9701d198d5 100644
--- a/drivers/mmc/host/sdhci-omap.c
+++ b/drivers/mmc/host/sdhci-omap.c
@@ -852,6 +852,7 @@ static const struct sdhci_pltfm_data sdhci_omap_pdata = {
 	.quirks2 = SDHCI_QUIRK2_ACMD23_BROKEN |
 		   SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
 		   SDHCI_QUIRK2_RSP_136_HAS_CRC |
+		   SDHCI_QUIRK2_DISABLE_DTO_FOR_ERASE |
 		   SDHCI_QUIRK2_DISABLE_HW_TIMEOUT,
 	.ops = &sdhci_omap_ops,
 };
-- 
2.19.2

[PATCH 6/7] dt-bindings: sdhci-omap: Add am335x and am437x specific bindings

[Faiz Abbas]

Add binding for the TI's sdhci-omap controller present in am335x and
am437x devices.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 Documentation/devicetree/bindings/mmc/sdhci-omap.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/sdhci-omap.txt b/Documentation/devicetree/bindings/mmc/sdhci-omap.txt
index f8914a2a2aec..dd4dbda32ca0 100644
--- a/Documentation/devicetree/bindings/mmc/sdhci-omap.txt
+++ b/Documentation/devicetree/bindings/mmc/sdhci-omap.txt
@@ -7,6 +7,8 @@ For UHS devices which require tuning, the device tree should have a "cpu_thermal
 Required properties:
 - compatible: Should be "ti,dra7-sdhci" for DRA7 and DRA72 controllers
 	      Should be "ti,k2g-sdhci" for K2G
+	      Should be "ti,am335-sdhci" for am335x controllers
+	      Should be "ti,am437-sdhci" for am437x controllers
 - ti,hwmods: Must be "mmc<n>", <n> is controller instance starting 1
 	     (Not required for K2G).
 - pinctrl-names: Should be subset of "default", "hs", "sdr12", "sdr25", "sdr50",
-- 
2.19.2

Re: [PATCH 6/7] dt-bindings: sdhci-omap: Add am335x and am437x specific bindings

[Rob Herring]

On Fri, 11 Jan 2019 16:38:50 +0530, Faiz Abbas wrote:
> Add binding for the TI's sdhci-omap controller present in am335x and
> am437x devices.
> 
> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
> ---
>  Documentation/devicetree/bindings/mmc/sdhci-omap.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH 7/7] mmc: sdhci-omap: Add am335x and am437x specific compatibles

[Faiz Abbas]

Add support for new compatible for TI's am335x and am437x devices.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 drivers/mmc/host/sdhci-omap.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c
index ae9701d198d5..2e246b5b744c 100644
--- a/drivers/mmc/host/sdhci-omap.c
+++ b/drivers/mmc/host/sdhci-omap.c
@@ -861,6 +861,14 @@ static const struct sdhci_omap_data k2g_data = {
 	.offset = 0x200,
 };
 
+static const struct sdhci_omap_data am335_data = {
+	.offset = 0x200,
+};
+
+static const struct sdhci_omap_data am437_data = {
+	.offset = 0x200,
+};
+
 static const struct sdhci_omap_data dra7_data = {
 	.offset = 0x200,
 	.flags	= SDHCI_OMAP_REQUIRE_IODELAY,
@@ -869,6 +877,8 @@ static const struct sdhci_omap_data dra7_data = {
 static const struct of_device_id omap_sdhci_match[] = {
 	{ .compatible = "ti,dra7-sdhci", .data = &dra7_data },
 	{ .compatible = "ti,k2g-sdhci", .data = &k2g_data },
+	{ .compatible = "ti,am335-sdhci", .data = &am335_data },
+	{ .compatible = "ti,am437-sdhci", .data = &am437_data },
 	{},
 };
 MODULE_DEVICE_TABLE(of, omap_sdhci_match);
-- 
2.19.2

Re: [PATCH 0/7] Port am335 and am437 devices to sdhci-omap

[Faiz Abbas]

Hi,

On 11/01/19 4:38 PM, Faiz Abbas wrote:
> The following add driver patches for porting TI's am335x and am437x
> devices to the sdhci-omap driver.
> 
> This involves adding external DMA support to sdhci (first 3 patches from
> Chunyan) plus some miscellaneous patches to take care of deviations of
> the controllers from the sdhci model.
> 
> DT changes will be posted in a separate series.
> 
> Untested versions of Chunyan's patches were posted before[1].
> 
> Tested on: am335x-evm, am335x-boneblack, am335x-sk, am437x-gpevm,
> am43xx-gpevm, am437x-idk, dra7xx-evm, dra72x-evm
> 
> [1] https://patchwork.kernel.org/project/linux-mmc/list/?series=54897
> 
> 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 external dma
> 
> Faiz Abbas (4):
>   mmc: sdhci: Add quirk for disabling DTO during erase command
>   mmc: sdhci_omap: Add DISABLE_DTO_FOR_ERASE Quirk
>   dt-bindings: sdhci-omap: Add am335x and am437x specific bindings
>   mmc: sdhci-omap: Add am335x and am437x specific compatibles
> 
>  .../devicetree/bindings/mmc/sdhci-omap.txt    |   9 +
>  drivers/mmc/host/Kconfig                      |   4 +
>  drivers/mmc/host/sdhci-omap.c                 |  27 +-
>  drivers/mmc/host/sdhci.c                      | 274 +++++++++++++++++-
>  drivers/mmc/host/sdhci.h                      |  10 +
>  5 files changed, 319 insertions(+), 5 deletions(-)
> 

Gentle ping.

Thanks,
Faiz