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

[PATCH v3 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.

I need some help with testing all the other beaglebone variants and SDIO
Wifi cards.

v3:
1. Dropped patch 1 because the tasklet was removed by Adrian in an
   earlier series.
2. Added dma bindings in sdhci-omap as optional properties.
3. Rebased on top of latest mainline.

v2:
1. sdhci is using two bottom halves. One threaded_rq for card detect and a
   tasklet for finishing mmc requests. Patch 1 removes the tasklet and
   moves its function to the threaded_irq. This enables me to
   terminate_sync() in sdhci_request_done()

2. Factored out common code for between the normal adn external dma case

3. Using existing API sdhci_data_timeout_irq for disabling DTO during
   erase commands.

4. Fixed subject line for dt-bindings patch.

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


Chunyan Zhang (3):
  dt-bindings: sdhci-omap: Add properties for using external dma
  mmc: sdhci: add support for using external DMA devices
  mmc: sdhci-omap: Add 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    |  11 +
 drivers/mmc/host/Kconfig                      |   4 +
 drivers/mmc/host/sdhci-omap.c                 |  27 +-
 drivers/mmc/host/sdhci.c                      | 290 ++++++++++++++++--
 drivers/mmc/host/sdhci.h                      |  10 +
 5 files changed, 313 insertions(+), 29 deletions(-)

-- 
2.19.2

[PATCH v3 1/7] dt-bindings: sdhci-omap: Add properties 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. Add binding documentation for these external dma properties.

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 | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/sdhci-omap.txt b/Documentation/devicetree/bindings/mmc/sdhci-omap.txt
index 72c4dec7e1db..97efb01617dd 100644
--- a/Documentation/devicetree/bindings/mmc/sdhci-omap.txt
+++ b/Documentation/devicetree/bindings/mmc/sdhci-omap.txt
@@ -15,6 +15,13 @@ Required properties:
 		 "hs200_1_8v",
 - pinctrl-<n> : Pinctrl states as described in bindings/pinctrl/pinctrl-bindings.txt
 
+Optional properties:
+- 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 {
 		compatible = "ti,dra7-sdhci";
@@ -22,4 +29,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 v3 1/7] dt-bindings: sdhci-omap: Add properties for using external dma

[Rob Herring]

On Tue, 10 Dec 2019 15:21:45 +0530, Faiz Abbas wrote:
> 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. Add binding documentation for these external dma properties.
> 
> 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 | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 

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

[PATCH v3 2/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 call
terminate_sync() in non-atomic context in case of an error.

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

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 49ea02c467bf..66ba8daa1cbe 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -1019,3 +1019,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 b056400e34b1..6f3d4991bee1 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -10,6 +10,7 @@
  */
 
 #include <linux/delay.h>
+#include <linux/dmaengine.h>
 #include <linux/ktime.h>
 #include <linux/highmem.h>
 #include <linux/io.h>
@@ -30,6 +31,7 @@
 #include <linux/mmc/card.h>
 #include <linux/mmc/sdio.h>
 #include <linux/mmc/slot-gpio.h>
+#include <linux/workqueue.h>
 
 #include "sdhci.h"
 
@@ -1014,18 +1016,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 */
@@ -1036,6 +1029,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;
@@ -1122,24 +1143,186 @@ 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)
+{
+	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);
+}
+
+#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 inline struct dma_chan *
+sdhci_external_dma_channel(struct sdhci_host *host, struct mmc_data *data)
+{
+	return NULL;
+}
+#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)
 {
@@ -1379,12 +1562,19 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 	}
 
 	host->cmd = cmd;
+	host->data_timeout = 0;
 	if (sdhci_data_line_cmd(cmd)) {
 		WARN_ON(host->data_cmd);
 		host->data_cmd = cmd;
+		sdhci_set_timeout(host, cmd);
 	}
 
-	sdhci_prepare_data(host, cmd);
+	if (cmd->data) {
+		if (host->use_external_dma)
+			sdhci_external_dma_prepare_data(host, cmd);
+		else
+			sdhci_prepare_data(host, cmd);
+	}
 
 	sdhci_writel(host, cmd->arg, SDHCI_ARGUMENT);
 
@@ -1426,6 +1616,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);
@@ -2652,6 +2845,18 @@ static bool sdhci_request_done(struct sdhci_host *host)
 	if (host->flags & SDHCI_REQ_USE_DMA) {
 		struct mmc_data *data = mrq->data;
 
+		spin_unlock_irqrestore(&host->lock, flags);
+
+		/* Terminate and synchronize dma in case of an error */
+		if (data && (mrq->cmd->error || data->error) &&
+		    host->use_external_dma) {
+			struct dma_chan *chan = sdhci_external_dma_channel(host,
+									  data);
+			dmaengine_terminate_sync(chan);
+		}
+
+		spin_lock_irqsave(&host->lock, flags);
+
 		if (data && data->host_cookie == COOKIE_MAPPED) {
 			if (host->bounce_buffer) {
 				/*
@@ -3758,12 +3963,28 @@ int sdhci_setup_host(struct sdhci_host *host)
 		       mmc_hostname(mmc), host->version);
 	}
 
-	if (host->quirks & SDHCI_QUIRK_FORCE_DMA)
+	if (host->use_external_dma) {
+		ret = sdhci_external_dma_init(host);
+		if (ret == -EPROBE_DEFER)
+			goto unreg;
+
+		/*
+		 * Fall back to use the DMA/PIO integrated in standard SDHCI
+		 * instead of external DMA devices.
+		 */
+		if (ret)
+			sdhci_switch_external_dma(host, false);
+	}
+
+	if (host->quirks & SDHCI_QUIRK_FORCE_DMA) {
 		host->flags |= SDHCI_USE_SDMA;
-	else if (!(host->caps & SDHCI_CAN_DO_SDMA))
+	} else if (!(host->caps & SDHCI_CAN_DO_SDMA)) {
 		DBG("Controller doesn't have SDMA capability\n");
-	else
+	} else if (host->use_external_dma) {
+		/* Using dma-names to detect external dma capability */
+	} else {
 		host->flags |= SDHCI_USE_SDMA;
+	}
 
 	if ((host->quirks & SDHCI_QUIRK_BROKEN_DMA) &&
 		(host->flags & SDHCI_USE_SDMA)) {
@@ -4264,6 +4485,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;
 }
@@ -4309,6 +4534,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");
@@ -4397,6 +4623,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 0ed3e0eaef5f..b28706a1bc6f 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -485,6 +485,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;
@@ -533,6 +534,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 */
@@ -562,6 +564,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 */
@@ -793,5 +800,6 @@ 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_abort_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 v3 2/7] mmc: sdhci: add support for using external DMA devices

[Adrian Hunter]

On 10/12/19 11:51 am, Faiz Abbas wrote:
> From: Chunyan Zhang <zhang.chunyan@linaro.org>
> 
> Some standard SD host controllers can support both external dma
> controllers as well as ADMA/SDMA in which the SD host controller
> acts as DMA master. TI's omap controller is the case as an example.
> 
> Currently the generic SDHCI code supports ADMA/SDMA integrated in
> the host controller but does not have any support for external DMA
> controllers implemented using dmaengine, meaning that custom code is
> needed for any systems that use an external DMA controller with SDHCI.
> 
> Fixes by Faiz Abbas <faiz_abbas@ti.com>:
> 1. Map scatterlists before dmaengine_prep_slave_sg()
> 2. Use dma_async() functions inside of the send_command() path and call
> terminate_sync() in non-atomic context in case of an error.
> 
> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
> ---
>  drivers/mmc/host/Kconfig |   3 +
>  drivers/mmc/host/sdhci.c | 285 +++++++++++++++++++++++++++++++++++----
>  drivers/mmc/host/sdhci.h |   8 ++
>  3 files changed, 268 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 49ea02c467bf..66ba8daa1cbe 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -1019,3 +1019,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 b056400e34b1..6f3d4991bee1 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -10,6 +10,7 @@
>   */
>  
>  #include <linux/delay.h>
> +#include <linux/dmaengine.h>
>  #include <linux/ktime.h>
>  #include <linux/highmem.h>
>  #include <linux/io.h>
> @@ -30,6 +31,7 @@
>  #include <linux/mmc/card.h>
>  #include <linux/mmc/sdio.h>
>  #include <linux/mmc/slot-gpio.h>
> +#include <linux/workqueue.h>

This is unrelated and should be a separate patch.

>  
>  #include "sdhci.h"
>  
> @@ -1014,18 +1016,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,

'inline' is not necessary

'sdhci_reset_data' is too much like SDHCI_RESET_DATA.  Maybe 'sdhci_set_data'

Please make the factoring out of sdhci_reset_data and sdhci_set_block_info a
separate patch

> +				    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 */
> @@ -1036,6 +1029,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)

'inline' is not necessary

Please add 'data' as a parameter like sdhci_reset_data.

> +{
> +
> +	/* 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;
> @@ -1122,24 +1143,186 @@ 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)

Please add blank line

> +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 *

My preference is not to wrap this line

> +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)
> +{
> +	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);
> +}
> +
> +#else

Please add blank line

> +static int sdhci_external_dma_init(struct sdhci_host *host)

This and 4 below can be inline

> +{
> +	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 */

Isn't this actually unreachable?  Maybe WARN_ON_ONCE would be better.

> +	sdhci_prepare_data(host, cmd);
> +}
> +
> +static void sdhci_external_dma_pre_transfer(struct sdhci_host *host,
> +					    struct mmc_command *cmd)
> +{}

{
}

> +
> +static inline struct dma_chan *

My preference is not to wrap this line

> +sdhci_external_dma_channel(struct sdhci_host *host, struct mmc_data *data)
> +{
> +	return NULL;
> +}

Please add blank line

> +#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)
>  {
> @@ -1379,12 +1562,19 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
>  	}
>  
>  	host->cmd = cmd;
> +	host->data_timeout = 0;
>  	if (sdhci_data_line_cmd(cmd)) {
>  		WARN_ON(host->data_cmd);
>  		host->data_cmd = cmd;
> +		sdhci_set_timeout(host, cmd);
>  	}
>  
> -	sdhci_prepare_data(host, cmd);
> +	if (cmd->data) {
> +		if (host->use_external_dma)
> +			sdhci_external_dma_prepare_data(host, cmd);
> +		else
> +			sdhci_prepare_data(host, cmd);
> +	}

Please make the 3 changes above and the corresponding changes
sdhci_prepare_data into a separate patch i.e.

 	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)
		sdhci_prepare_data(host, cmd);

>  
>  	sdhci_writel(host, cmd->arg, SDHCI_ARGUMENT);
>  
> @@ -1426,6 +1616,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);
> @@ -2652,6 +2845,18 @@ static bool sdhci_request_done(struct sdhci_host *host)
>  	if (host->flags & SDHCI_REQ_USE_DMA) {
>  		struct mmc_data *data = mrq->data;
>  
> +		spin_unlock_irqrestore(&host->lock, flags);
> +
> +		/* Terminate and synchronize dma in case of an error */
> +		if (data && (mrq->cmd->error || data->error) &&
> +		    host->use_external_dma) {
> +			struct dma_chan *chan = sdhci_external_dma_channel(host,
> +									  data);
> +			dmaengine_terminate_sync(chan);
> +		}
> +
> +		spin_lock_irqsave(&host->lock, flags);
> +

Need to take the mrq out of mrqs_done[] to ensure it is not processed again,
and put it back again to be consistent with the remaining code. Also put
host->use_external_dma as the first condition i.e.

		if (host->use_external_dma && data &&
		    (mrq->cmd->error || data->error)) {
			struct dma_chan *chan = sdhci_external_dma_channel(host, data);

			host->mrqs_done[i] = NULL;
			spin_unlock_irqrestore(&host->lock, flags);
			dmaengine_terminate_sync(chan);
			spin_lock_irqsave(&host->lock, flags);
			sdhci_set_mrq_done(host, mrq);
		}

where sdhci_set_mrq_done() is factored out from __sdhci_finish_mrq() i.e.

static void sdhci_set_mrq_done(struct sdhci_host *host, struct mmc_request *mrq)
{
	int i;

	for (i = 0; i < SDHCI_MAX_MRQS; i++) {
		if (host->mrqs_done[i] == mrq) {
			WARN_ON(1);
			return;
		}
	}

	for (i = 0; i < SDHCI_MAX_MRQS; i++) {
		if (!host->mrqs_done[i]) {
			host->mrqs_done[i] = mrq;
			break;
		}
	}

	WARN_ON(i >= SDHCI_MAX_MRQS);
}

sdhci_set_mrq_done() can be made in the refactoring patch.

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

What is this change for?  Do you expect for SDHCI_USE_SDMA and
SDHCI_USE_ADMA flags to be clear?

> +	} else {
>  		host->flags |= SDHCI_USE_SDMA;
> +	}
>  
>  	if ((host->quirks & SDHCI_QUIRK_BROKEN_DMA) &&
>  		(host->flags & SDHCI_USE_SDMA)) {
> @@ -4264,6 +4485,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;
>  }
> @@ -4309,6 +4534,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");
> @@ -4397,6 +4623,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 0ed3e0eaef5f..b28706a1bc6f 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -485,6 +485,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;
> @@ -533,6 +534,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 */
> @@ -562,6 +564,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 */
> @@ -793,5 +800,6 @@ 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_abort_tuning(struct sdhci_host *host, u32 opcode);
> +void sdhci_switch_external_dma(struct sdhci_host *host, bool en);
>  
>  #endif /* __SDHCI_HW_H */
> 

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

[Faiz Abbas]

Hi Adrian,

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

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

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

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

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

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

Thanks,
Faiz

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

[Adrian Hunter]

On 16/12/19 10:27 am, Faiz Abbas wrote:
> Hi Adrian,
> 
> On 12/12/19 6:25 pm, Adrian Hunter wrote:
>> On 10/12/19 11:51 am, Faiz Abbas wrote:
>>> From: Chunyan Zhang <zhang.chunyan@linaro.org>
>>>
>>> Some standard SD host controllers can support both external dma
>>> controllers as well as ADMA/SDMA in which the SD host controller
>>> acts as DMA master. TI's omap controller is the case as an example.
>>>
>>> Currently the generic SDHCI code supports ADMA/SDMA integrated in
>>> the host controller but does not have any support for external DMA
>>> controllers implemented using dmaengine, meaning that custom code is
>>> needed for any systems that use an external DMA controller with SDHCI.
>>>
>>> Fixes by Faiz Abbas <faiz_abbas@ti.com>:
>>> 1. Map scatterlists before dmaengine_prep_slave_sg()
>>> 2. Use dma_async() functions inside of the send_command() path and call
>>> terminate_sync() in non-atomic context in case of an error.
>>>
>>> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
>>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
>>> ---
> ...
>>>  {
>>> @@ -1379,12 +1562,19 @@ void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
>>>  	}
>>>  
>>>  	host->cmd = cmd;
>>> +	host->data_timeout = 0;
>>>  	if (sdhci_data_line_cmd(cmd)) {
>>>  		WARN_ON(host->data_cmd);
>>>  		host->data_cmd = cmd;
>>> +		sdhci_set_timeout(host, cmd);
>>>  	}
>>>  
>>> -	sdhci_prepare_data(host, cmd);
>>> +	if (cmd->data) {
>>> +		if (host->use_external_dma)
>>> +			sdhci_external_dma_prepare_data(host, cmd);
>>> +		else
>>> +			sdhci_prepare_data(host, cmd);
>>> +	}
>>
>> Please make the 3 changes above and the corresponding changes
>> sdhci_prepare_data into a separate patch i.e.
> 
> Ok. And I agree with all your style change requests above this. Will fix
> in v4.
> 
>>> @@ -2652,6 +2845,18 @@ static bool sdhci_request_done(struct sdhci_host *host)
>>>  	if (host->flags & SDHCI_REQ_USE_DMA) {
>>>  		struct mmc_data *data = mrq->data;
>>>  
>>> +		spin_unlock_irqrestore(&host->lock, flags);
>>> +
>>> +		/* Terminate and synchronize dma in case of an error */
>>> +		if (data && (mrq->cmd->error || data->error) &&
>>> +		    host->use_external_dma) {
>>> +			struct dma_chan *chan = sdhci_external_dma_channel(host,
>>> +									  data);
>>> +			dmaengine_terminate_sync(chan);
>>> +		}
>>> +
>>> +		spin_lock_irqsave(&host->lock, flags);
>>> +
>>
>> Need to take the mrq out of mrqs_done[] to ensure it is not processed again,
>> and put it back again to be consistent with the remaining code. Also put
>> host->use_external_dma as the first condition i.e.
>>
>> 		if (host->use_external_dma && data &&
>> 		    (mrq->cmd->error || data->error)) {
>> 			struct dma_chan *chan = sdhci_external_dma_channel(host, data);
>>
>> 			host->mrqs_done[i] = NULL;
>> 			spin_unlock_irqrestore(&host->lock, flags);
>> 			dmaengine_terminate_sync(chan);
>> 			spin_lock_irqsave(&host->lock, flags);
>> 			sdhci_set_mrq_done(host, mrq);
>> 		}
>>
>> where sdhci_set_mrq_done() is factored out from __sdhci_finish_mrq() i.e.
>>
>> static void sdhci_set_mrq_done(struct sdhci_host *host, struct mmc_request *mrq)
>> {
>> 	int i;
>>
>> 	for (i = 0; i < SDHCI_MAX_MRQS; i++) {
>> 		if (host->mrqs_done[i] == mrq) {
>> 			WARN_ON(1);
>> 			return;
>> 		}
>> 	}
>>
>> 	for (i = 0; i < SDHCI_MAX_MRQS; i++) {
>> 		if (!host->mrqs_done[i]) {
>> 			host->mrqs_done[i] = mrq;
>> 			break;
>> 		}
>> 	}
>>
>> 	WARN_ON(i >= SDHCI_MAX_MRQS);
>> }
>>
>> sdhci_set_mrq_done() can be made in the refactoring patch.
> Haven't we already done the sdhci_set_mrq_done() part in
> __sdhci_finish_mrq()?
> 
> We are picking up an already "done" mrq, looking at whether it had any
> error and then sychronizing with external dma. Or at least that is my
> understanding.

sdhci supports having 2 requests (1 data, 1 cmd) at a time, so there is an
error case where 1 request will wait for the 2nd request before doing a
reset.  That logic is further down in sdhci_request_done() so you have to
put the mrq back into host->mrqs_done[] to make it work.

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

What about moving the "if (host->use_external_dma) {" clause and explicitly
clearing SDHCI_USE_SDMA and SDHCI_USE_ADMA?

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

[Faiz Abbas]

Hi Adrian,

On 16/12/19 7:15 pm, Adrian Hunter wrote:
> On 16/12/19 10:27 am, Faiz Abbas wrote:
>> Hi Adrian,
>>
>> On 12/12/19 6:25 pm, Adrian Hunter wrote:
>>> On 10/12/19 11:51 am, Faiz Abbas wrote:
>>>> From: Chunyan Zhang <zhang.chunyan@linaro.org>
>>>>
>>>> Some standard SD host controllers can support both external dma
>>>> controllers as well as ADMA/SDMA in which the SD host controller
>>>> acts as DMA master. TI's omap controller is the case as an example.
>>>>
>>>> Currently the generic SDHCI code supports ADMA/SDMA integrated in
>>>> the host controller but does not have any support for external DMA
>>>> controllers implemented using dmaengine, meaning that custom code is
>>>> needed for any systems that use an external DMA controller with SDHCI.
>>>>
>>>> Fixes by Faiz Abbas <faiz_abbas@ti.com>:
>>>> 1. Map scatterlists before dmaengine_prep_slave_sg()
>>>> 2. Use dma_async() functions inside of the send_command() path and call
>>>> terminate_sync() in non-atomic context in case of an error.
>>>>
>>>> @@ -2652,6 +2845,18 @@ static bool sdhci_request_done(struct sdhci_host *host)
>>>>  	if (host->flags & SDHCI_REQ_USE_DMA) {
>>>>  		struct mmc_data *data = mrq->data;
>>>>  
>>>> +		spin_unlock_irqrestore(&host->lock, flags);
>>>> +
>>>> +		/* Terminate and synchronize dma in case of an error */
>>>> +		if (data && (mrq->cmd->error || data->error) &&
>>>> +		    host->use_external_dma) {
>>>> +			struct dma_chan *chan = sdhci_external_dma_channel(host,
>>>> +									  data);
>>>> +			dmaengine_terminate_sync(chan);
>>>> +		}
>>>> +
>>>> +		spin_lock_irqsave(&host->lock, flags);
>>>> +
>>>
>>> Need to take the mrq out of mrqs_done[] to ensure it is not processed again,
>>> and put it back again to be consistent with the remaining code. Also put
>>> host->use_external_dma as the first condition i.e.
>>>
>>> 		if (host->use_external_dma && data &&
>>> 		    (mrq->cmd->error || data->error)) {
>>> 			struct dma_chan *chan = sdhci_external_dma_channel(host, data);
>>>
>>> 			host->mrqs_done[i] = NULL;
>>> 			spin_unlock_irqrestore(&host->lock, flags);
>>> 			dmaengine_terminate_sync(chan);
>>> 			spin_lock_irqsave(&host->lock, flags);
>>> 			sdhci_set_mrq_done(host, mrq);
>>> 		}
>>>
>>> where sdhci_set_mrq_done() is factored out from __sdhci_finish_mrq() i.e.
>>>
>>> static void sdhci_set_mrq_done(struct sdhci_host *host, struct mmc_request *mrq)
>>> {
>>> 	int i;
>>>
>>> 	for (i = 0; i < SDHCI_MAX_MRQS; i++) {
>>> 		if (host->mrqs_done[i] == mrq) {
>>> 			WARN_ON(1);
>>> 			return;
>>> 		}
>>> 	}
>>>
>>> 	for (i = 0; i < SDHCI_MAX_MRQS; i++) {
>>> 		if (!host->mrqs_done[i]) {
>>> 			host->mrqs_done[i] = mrq;
>>> 			break;
>>> 		}
>>> 	}
>>>
>>> 	WARN_ON(i >= SDHCI_MAX_MRQS);
>>> }
>>>
>>> sdhci_set_mrq_done() can be made in the refactoring patch.
>> Haven't we already done the sdhci_set_mrq_done() part in
>> __sdhci_finish_mrq()?
>>
>> We are picking up an already "done" mrq, looking at whether it had any
>> error and then sychronizing with external dma. Or at least that is my
>> understanding.
> 
> sdhci supports having 2 requests (1 data, 1 cmd) at a time, so there is an
> error case where 1 request will wait for the 2nd request before doing a
> reset.  That logic is further down in sdhci_request_done() so you have to
> put the mrq back into host->mrqs_done[] to make it work.

Sorry for the late response. I had to spend some time figuring out how
the mrqs_done handling works. Will add the new function above.

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

I am ok with this as well. Sending a new version.

Thanks,
Faiz

[PATCH v3 3/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 66ba8daa1cbe..89ce19e5755f 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -998,6 +998,7 @@ config MMC_SDHCI_OMAP
 	depends on MMC_SDHCI_PLTFM && OF
 	select THERMAL
 	imply 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 083e7e053c95..84d85aa743da 100644
--- a/drivers/mmc/host/sdhci-omap.c
+++ b/drivers/mmc/host/sdhci-omap.c
@@ -685,7 +685,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;
@@ -1037,6 +1041,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)
@@ -1049,6 +1054,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)) {
@@ -1065,6 +1074,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);
@@ -1134,6 +1144,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 v3 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 | 5 +++++
 drivers/mmc/host/sdhci.h | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 6f3d4991bee1..b8934c50b9c4 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1532,6 +1532,11 @@ 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)) {
+		sdhci_set_data_timeout_irq(host, false);
+	}
+
 	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 b28706a1bc6f..beda9afba3a6 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -482,6 +482,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 v3 4/7] mmc: sdhci: Add quirk for disabling DTO during erase command

[Adrian Hunter]

On 10/12/19 11:51 am, 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.
> 
> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
> ---
>  drivers/mmc/host/sdhci.c | 5 +++++
>  drivers/mmc/host/sdhci.h | 2 ++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 6f3d4991bee1..b8934c50b9c4 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1532,6 +1532,11 @@ 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)) {
> +		sdhci_set_data_timeout_irq(host, false);
> +	}

If you factor out __sdhci_set_timeout() like below then
you could implement ->set_timeout() to do this.

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index ad6d2f93aa0b..389e3239eadc 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1002,27 +1002,28 @@ static void sdhci_set_data_timeout_irq(struct sdhci_host *host, bool enable)
 	sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE);
 }
 
-static void sdhci_set_timeout(struct sdhci_host *host, struct mmc_command *cmd)
+void __sdhci_set_timeout(struct sdhci_host *host, struct mmc_command *cmd)
 {
-	u8 count;
-
-	if (host->ops->set_timeout) {
-		host->ops->set_timeout(host, cmd);
-	} else {
-		bool too_big = false;
-
-		count = sdhci_calc_timeout(host, cmd, &too_big);
+	bool too_big = false;
+	u8 count = sdhci_calc_timeout(host, cmd, &too_big);
+
+	if (too_big && host->quirks2 & SDHCI_QUIRK2_DISABLE_HW_TIMEOUT) {
+		sdhci_calc_sw_timeout(host, cmd);
+		sdhci_set_data_timeout_irq(host, false);
+	} else if (!(host->ier & SDHCI_INT_DATA_TIMEOUT)) {
+		sdhci_set_data_timeout_irq(host, true);
+	}
 
-		if (too_big &&
-		    host->quirks2 & SDHCI_QUIRK2_DISABLE_HW_TIMEOUT) {
-			sdhci_calc_sw_timeout(host, cmd);
-			sdhci_set_data_timeout_irq(host, false);
-		} else if (!(host->ier & SDHCI_INT_DATA_TIMEOUT)) {
-			sdhci_set_data_timeout_irq(host, true);
-		}
+	sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL);
+}
+EXPORT_SYMBOL_GPL(__sdhci_set_timeout);
 
-		sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL);
-	}
+static void sdhci_set_timeout(struct sdhci_host *host, struct mmc_command *cmd)
+{
+	if (host->ops->set_timeout)
+		host->ops->set_timeout(host, cmd);
+	else
+		__sdhci_set_timeout(host, cmd);
 }
 
 static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)



> +
>  	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 b28706a1bc6f..beda9afba3a6 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -482,6 +482,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 v3 4/7] mmc: sdhci: Add quirk for disabling DTO during erase command

[Faiz Abbas]

Hi Adrian,


On 13/12/19 3:10 pm, Adrian Hunter wrote:
> On 10/12/19 11:51 am, 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.
>>
>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
>> ---
>>  drivers/mmc/host/sdhci.c | 5 +++++
>>  drivers/mmc/host/sdhci.h | 2 ++
>>  2 files changed, 7 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 6f3d4991bee1..b8934c50b9c4 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -1532,6 +1532,11 @@ 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)) {
>> +		sdhci_set_data_timeout_irq(host, false);
>> +	}
> 
> If you factor out __sdhci_set_timeout() like below then
> you could implement ->set_timeout() to do this.
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index ad6d2f93aa0b..389e3239eadc 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1002,27 +1002,28 @@ static void sdhci_set_data_timeout_irq(struct sdhci_host *host, bool enable)
>  	sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE);
>  }
>  
> -static void sdhci_set_timeout(struct sdhci_host *host, struct mmc_command *cmd)
> +void __sdhci_set_timeout(struct sdhci_host *host, struct mmc_command *cmd)
>  {
> -	u8 count;
> -
> -	if (host->ops->set_timeout) {
> -		host->ops->set_timeout(host, cmd);
> -	} else {
> -		bool too_big = false;
> -
> -		count = sdhci_calc_timeout(host, cmd, &too_big);
> +	bool too_big = false;
> +	u8 count = sdhci_calc_timeout(host, cmd, &too_big);
> +
> +	if (too_big && host->quirks2 & SDHCI_QUIRK2_DISABLE_HW_TIMEOUT) {
> +		sdhci_calc_sw_timeout(host, cmd);
> +		sdhci_set_data_timeout_irq(host, false);
> +	} else if (!(host->ier & SDHCI_INT_DATA_TIMEOUT)) {
> +		sdhci_set_data_timeout_irq(host, true);
> +	}
>  
> -		if (too_big &&
> -		    host->quirks2 & SDHCI_QUIRK2_DISABLE_HW_TIMEOUT) {
> -			sdhci_calc_sw_timeout(host, cmd);
> -			sdhci_set_data_timeout_irq(host, false);
> -		} else if (!(host->ier & SDHCI_INT_DATA_TIMEOUT)) {
> -			sdhci_set_data_timeout_irq(host, true);
> -		}
> +	sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL);
> +}
> +EXPORT_SYMBOL_GPL(__sdhci_set_timeout);
>  
> -		sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL);
> -	}
> +static void sdhci_set_timeout(struct sdhci_host *host, struct mmc_command *cmd)
> +{
> +	if (host->ops->set_timeout)
> +		host->ops->set_timeout(host, cmd);
> +	else
> +		__sdhci_set_timeout(host, cmd);
>  }

Ok. I'll add the refactoring as a separate patch.

Thanks,
Faiz

[PATCH v3 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 84d85aa743da..c54c864fe419 100644
--- a/drivers/mmc/host/sdhci-omap.c
+++ b/drivers/mmc/host/sdhci-omap.c
@@ -879,6 +879,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 v3 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>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 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 97efb01617dd..aeb615ef672a 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

[PATCH v3 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 c54c864fe419..09ed326d150c 100644
--- a/drivers/mmc/host/sdhci-omap.c
+++ b/drivers/mmc/host/sdhci-omap.c
@@ -888,6 +888,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,
@@ -896,6 +904,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