Currently the generic SDHCI code in the Linux kernel supports the SD
standard DMA integrated into the host controller but does not have any
support for external DMA controllers implemented using dmaengine meaning
that custom code is needed for any systems that use a generic DMA
controller with SDHCI which in practice means any SDHCI controller that
doesn't have an integrated DMA controller so we should have this as a
generic feature.
There are already a number of controller specific drivers that have dmaengine
code, and some could use sdhci.c actually, but needed to implement mmc_ops->request()
in their specific driver for sending command with external dma using dmaengine
framework, with this patchset, them will take advantage of the generic support.
TI's omap controller is the case as an example.
Any comments are very appreciated.
Thanks,
Chunyan
Changes from v1 (https://lkml.org/lkml/2018/11/5/110):
(The code on patch 1/3 only was revised)
* Address comments from Arnd:
- Release channel when failed to request it unconditionally;
- Skip warning message if get EPROBE_DEFER;
* Address Andrian's comments:
- Replace extdma with external_dma;
- Add release dma resources in sdhci_cleanup_host() and sdhci_remove_host();
- Release dma resources once dmaengine_submit() failed.
- Put rx/tx_chan in struct sdhci_host, and removed unused structure.
* Fall back to the DMA/PIO which standard SDHCI supports, if sdhci_external_dma_setup()
or sdhci_external_dma_init failed;
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
.../devicetree/bindings/mmc/sdhci-omap.txt | 7 +
drivers/mmc/host/Kconfig | 13 ++
drivers/mmc/host/sdhci-omap.c | 7 +
drivers/mmc/host/sdhci.c | 181 ++++++++++++++++++++-
drivers/mmc/host/sdhci.h | 8 +
5 files changed, 215 insertions(+), 1 deletion(-)
--
2.7.4
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.
Signed-off-by: Chunyan Zhang <[email protected]>
---
drivers/mmc/host/Kconfig | 13 ++++
drivers/mmc/host/sdhci.c | 181 ++++++++++++++++++++++++++++++++++++++++++++++-
drivers/mmc/host/sdhci.h | 8 +++
3 files changed, 201 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 1b58739..7bf3eff 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -977,3 +977,16 @@ config MMC_SDHCI_OMAP
If you have a controller with this interface, say Y or M here.
If unsure, say N.
+
+config MMC_SDHCI_EXTERNAL_DMA
+ bool "Support external DMA in standard SD host controller"
+ depends on MMC_SDHCI
+ depends on DMA_ENGINE
+ help
+ This is an option for using external DMA device via dmaengine
+ framework.
+
+ If you have a controller which support using external DMA device
+ for data transfer, can say Y.
+
+ If unsure, say N.
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 99bdae5..853cc98 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -14,6 +14,7 @@
*/
#include <linux/delay.h>
+#include <linux/dmaengine.h>
#include <linux/ktime.h>
#include <linux/highmem.h>
#include <linux/io.h>
@@ -1309,6 +1310,158 @@ static void sdhci_del_timer(struct sdhci_host *host, struct mmc_request *mrq)
del_timer(&host->timer);
}
+#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 = 0, 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;
+
+ 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;
+
+ 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)
+{
+ if (sdhci_external_dma_setup(host, cmd)) {
+ sdhci_external_dma_release(host);
+ pr_err("%s: Failed to setup external DMA, switch to the DMA/PIO which standard SDHCI provides.\n",
+ mmc_hostname(host->mmc));
+ } else {
+ /* Prepare for using external dma */
+ host->flags |= SDHCI_REQ_USE_DMA;
+ }
+
+ sdhci_prepare_data(host, cmd);
+}
+
+static void sdhci_external_dma_pre_transfer(struct sdhci_host *host,
+ struct mmc_command *cmd)
+{
+ struct dma_chan *chan = sdhci_external_dma_channel(host, cmd->data);
+
+ if (cmd->opcode != MMC_SET_BLOCK_COUNT) {
+ sdhci_set_timeout(host, cmd);
+ dma_async_issue_pending(chan);
+ }
+}
+#else
+static int sdhci_external_dma_init(struct sdhci_host *host)
+{
+ return 0;
+}
+
+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 SDHCI_EXTDMA 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)
+{}
+#endif
+
+void sdhci_switch_external_dma(struct sdhci_host *host, bool en)
+{
+ host->use_external_dma = en;
+}
+EXPORT_SYMBOL_GPL(sdhci_switch_external_dma);
+
void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
{
int flags;
@@ -1355,7 +1508,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);
@@ -1397,6 +1553,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);
@@ -4133,6 +4292,19 @@ int sdhci_setup_host(struct sdhci_host *host)
return ret;
}
+ 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);
+ }
+
return 0;
unreg:
@@ -4161,6 +4333,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;
}
@@ -4295,6 +4471,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 b001cf4..8e50a97 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -475,6 +475,7 @@ struct sdhci_host {
int irq; /* Device IRQ */
void __iomem *ioaddr; /* Mapped address */
+ phys_addr_t mapbase; /* physical address base */
char *bounce_buffer; /* For packing SDMA reads/writes */
dma_addr_t bounce_addr;
unsigned int bounce_buffer_size;
@@ -524,6 +525,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;
struct mmc_request *mrqs_done[SDHCI_MAX_MRQS]; /* Requests done */
struct mmc_command *cmd; /* Current command */
@@ -552,6 +554,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 */
@@ -785,5 +792,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.7.4
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 <[email protected]>
---
drivers/mmc/host/sdhci-omap.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c
index 88347ce..ccc79f2 100644
--- a/drivers/mmc/host/sdhci-omap.c
+++ b/drivers/mmc/host/sdhci-omap.c
@@ -896,6 +896,7 @@ static int sdhci_omap_probe(struct platform_device *pdev)
const struct of_device_id *match;
struct sdhci_omap_data *data;
const struct soc_device_attribute *soc;
+ struct resource *regs;
match = of_match_device(omap_sdhci_match, dev);
if (!match)
@@ -908,6 +909,10 @@ static int sdhci_omap_probe(struct platform_device *pdev)
}
offset = data->offset;
+ regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!regs)
+ return -ENXIO;
+
host = sdhci_pltfm_init(pdev, &sdhci_omap_pdata,
sizeof(*omap_host));
if (IS_ERR(host)) {
@@ -924,6 +929,7 @@ static int sdhci_omap_probe(struct platform_device *pdev)
omap_host->timing = MMC_TIMING_LEGACY;
omap_host->flags = data->flags;
host->ioaddr += offset;
+ host->mapbase = regs->start;
mmc = host->mmc;
sdhci_get_of_property(pdev);
@@ -991,6 +997,7 @@ static int sdhci_omap_probe(struct platform_device *pdev)
host->mmc_host_ops.execute_tuning = sdhci_omap_execute_tuning;
host->mmc_host_ops.enable_sdio_irq = sdhci_omap_enable_sdio_irq;
+ sdhci_switch_external_dma(host, true);
ret = sdhci_setup_host(host);
if (ret)
goto err_put_sync;
--
2.7.4
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 <[email protected]>
---
Documentation/devicetree/bindings/mmc/sdhci-omap.txt | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/Documentation/devicetree/bindings/mmc/sdhci-omap.txt b/Documentation/devicetree/bindings/mmc/sdhci-omap.txt
index 393848c..c73fd47 100644
--- a/Documentation/devicetree/bindings/mmc/sdhci-omap.txt
+++ b/Documentation/devicetree/bindings/mmc/sdhci-omap.txt
@@ -12,6 +12,11 @@ Required properties:
"ddr_1_8v-rev11", "ddr_1_8v" or "ddr_3_3v", "hs200_1_8v-rev11",
"hs200_1_8v",
- pinctrl-<n> : Pinctrl states as described in bindings/pinctrl/pinctrl-bindings.txt
+- dmas: List of DMA specifiers with the controller specific format as described
+ in the generic DMA client binding. A tx and rx specifier is required.
+- dma-names: List of DMA request names. These strings correspond 1:1 with the
+ DMA specifiers listed in dmas. The string naming is to be "rx"
+ and "tx" for RX and TX DMA requests, respectively.
Example:
mmc1: mmc@4809c000 {
@@ -20,4 +25,6 @@ Example:
ti,hwmods = "mmc1";
bus-width = <4>;
vmmc-supply = <&vmmc>; /* phandle to regulator node */
+ dmas = <&sdma 61 &sdma 62>;
+ dma-names = "tx", "rx";
};
--
2.7.4
On 12/11/18 9:26 AM, 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.
I still think you probably need to reset the DMA if there are transfer
errors - perhaps you could comment on that. Also there are some comments below.
>
> Signed-off-by: Chunyan Zhang <[email protected]>
> ---
> drivers/mmc/host/Kconfig | 13 ++++
> drivers/mmc/host/sdhci.c | 181 ++++++++++++++++++++++++++++++++++++++++++++++-
> drivers/mmc/host/sdhci.h | 8 +++
> 3 files changed, 201 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 1b58739..7bf3eff 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -977,3 +977,16 @@ config MMC_SDHCI_OMAP
> If you have a controller with this interface, say Y or M here.
>
> If unsure, say N.
> +
> +config MMC_SDHCI_EXTERNAL_DMA
> + bool "Support external DMA in standard SD host controller"
It would be simpler if the drivers that needed it just selected it e.g.
config MMC_SDHCI_OMAP
tristate "TI SDHCI Controller Support"
depends on MMC_SDHCI_PLTFM && OF
select MMC_SDHCI_EXTERNAL_DMA if DMA_ENGINE
> + depends on MMC_SDHCI
> + depends on DMA_ENGINE
> + help
> + This is an option for using external DMA device via dmaengine
> + framework.
> +
> + If you have a controller which support using external DMA device
> + for data transfer, can say Y.
> +
> + If unsure, say N.
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 99bdae5..853cc98 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -14,6 +14,7 @@
> */
>
> #include <linux/delay.h>
> +#include <linux/dmaengine.h>
> #include <linux/ktime.h>
> #include <linux/highmem.h>
> #include <linux/io.h>
> @@ -1309,6 +1310,158 @@ static void sdhci_del_timer(struct sdhci_host *host, struct mmc_request *mrq)
> del_timer(&host->timer);
> }
>
> +#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 = 0, i;
> + struct dma_async_tx_descriptor *desc;
> + struct mmc_data *data = cmd->data;
cmd->data might be NULL? Have you tested this?
> + struct dma_chan *chan;
> + struct dma_slave_config cfg;
> + dma_cookie_t cookie;
> +
> + 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;
> +
> + 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)
> +{
> + if (sdhci_external_dma_setup(host, cmd)) {
> + sdhci_external_dma_release(host);
> + pr_err("%s: Failed to setup external DMA, switch to the DMA/PIO which standard SDHCI provides.\n",
> + mmc_hostname(host->mmc));
> + } else {
> + /* Prepare for using external dma */
> + host->flags |= SDHCI_REQ_USE_DMA;
> + }
> +
> + sdhci_prepare_data(host, cmd);
> +}
> +
> +static void sdhci_external_dma_pre_transfer(struct sdhci_host *host,
> + struct mmc_command *cmd)
> +{
> + struct dma_chan *chan = sdhci_external_dma_channel(host, cmd->data);
cmd->data might be NULL? Have you tested this?
> +
> + if (cmd->opcode != MMC_SET_BLOCK_COUNT) {
I would have thought:
if (cmd->data)
> + sdhci_set_timeout(host, cmd);
> + dma_async_issue_pending(chan);
> + }
> +}
> +#else
> +static int sdhci_external_dma_init(struct sdhci_host *host)
> +{
> + return 0;
That should return an error, perhaps -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 SDHCI_EXTDMA not supported, PIO will be used */
SDHCI_EXTDMA is now MMC_SDHCI_EXTERNAL_DMA
> + sdhci_prepare_data(host, cmd);
> +}
> +
> +static void sdhci_external_dma_pre_transfer(struct sdhci_host *host,
> + struct mmc_command *cmd)
> +{}
> +#endif
> +
> +void sdhci_switch_external_dma(struct sdhci_host *host, bool en)
> +{
> + host->use_external_dma = en;
> +}
> +EXPORT_SYMBOL_GPL(sdhci_switch_external_dma);
> +
> void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
> {
> int flags;
> @@ -1355,7 +1508,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);
>
> @@ -1397,6 +1553,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);
> @@ -4133,6 +4292,19 @@ int sdhci_setup_host(struct sdhci_host *host)
> return ret;
> }
>
> + 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);
> + }
> +
> return 0;
>
> unreg:
> @@ -4161,6 +4333,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;
> }
> @@ -4295,6 +4471,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 b001cf4..8e50a97 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -475,6 +475,7 @@ struct sdhci_host {
>
> int irq; /* Device IRQ */
> void __iomem *ioaddr; /* Mapped address */
> + phys_addr_t mapbase; /* physical address base */
> char *bounce_buffer; /* For packing SDMA reads/writes */
> dma_addr_t bounce_addr;
> unsigned int bounce_buffer_size;
> @@ -524,6 +525,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;
>
> struct mmc_request *mrqs_done[SDHCI_MAX_MRQS]; /* Requests done */
> struct mmc_command *cmd; /* Current command */
> @@ -552,6 +554,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 */
> @@ -785,5 +792,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 */
>
Hi Chunyan,
On 12/11/18 12:56 PM, Chunyan Zhang wrote:
> Currently the generic SDHCI code in the Linux kernel supports the SD
> standard DMA integrated into the host controller but does not have any
> support for external DMA controllers implemented using dmaengine meaning
> that custom code is needed for any systems that use a generic DMA
> controller with SDHCI which in practice means any SDHCI controller that
> doesn't have an integrated DMA controller so we should have this as a
> generic feature.
>
> There are already a number of controller specific drivers that have dmaengine
> code, and some could use sdhci.c actually, but needed to implement mmc_ops->request()
> in their specific driver for sending command with external dma using dmaengine
> framework, with this patchset, them will take advantage of the generic support.
> TI's omap controller is the case as an example.
>
> Any comments are very appreciated.
>
This is great. It helps us move am335x and am43xx platforms to
sdhci-omap. What platforms have you tested this on?
Thanks,
Faiz
+ Mark Brown
Chunyan,
On 11/21/2018 5:17 PM, Faiz Abbas wrote:
> Hi Chunyan,
>
> On 12/11/18 12:56 PM, Chunyan Zhang wrote:
>> Currently the generic SDHCI code in the Linux kernel supports the SD
>> standard DMA integrated into the host controller but does not have any
>> support for external DMA controllers implemented using dmaengine meaning
>> that custom code is needed for any systems that use a generic DMA
>> controller with SDHCI which in practice means any SDHCI controller that
>> doesn't have an integrated DMA controller so we should have this as a
>> generic feature.
>>
>> There are already a number of controller specific drivers that have dmaengine
>> code, and some could use sdhci.c actually, but needed to implement mmc_ops->request()
>> in their specific driver for sending command with external dma using dmaengine
>> framework, with this patchset, them will take advantage of the generic support.
>> TI's omap controller is the case as an example.
>>
>> Any comments are very appreciated.
>>
>
> This is great. It helps us move am335x and am43xx platforms to
> sdhci-omap. What platforms have you tested this on?
>
Gentle ping on this. I tried testing these with an am335x-evm board. In
their current condition, the card fails to enumerate altogether. The
changes suggested by Adrian should fix this. Let me know when you post a v3.
Thanks,
Faiz
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.
Signed-off-by: Chunyan Zhang <[email protected]>
---
drivers/mmc/host/Kconfig | 14 ++++
drivers/mmc/host/sdhci.c | 185 ++++++++++++++++++++++++++++++++++++++++++++++-
drivers/mmc/host/sdhci.h | 8 ++
3 files changed, 206 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 1b58739..4183f43 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -969,6 +969,7 @@ config MMC_SDHCI_XENON
config MMC_SDHCI_OMAP
tristate "TI SDHCI Controller Support"
depends on MMC_SDHCI_PLTFM && OF
+ 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
@@ -977,3 +978,16 @@ config MMC_SDHCI_OMAP
If you have a controller with this interface, say Y or M here.
If unsure, say N.
+
+config MMC_SDHCI_EXTERNAL_DMA
+ bool "Support external DMA in standard SD host controller"
+ depends on MMC_SDHCI
+ depends on DMA_ENGINE
+ help
+ This is an option for using external DMA device via dmaengine
+ framework.
+
+ If you have a controller which support using external DMA device
+ for data transfer, can say Y.
+
+ If unsure, say N.
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 99bdae5..ad7cc80 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -14,6 +14,7 @@
*/
#include <linux/delay.h>
+#include <linux/dmaengine.h>
#include <linux/ktime.h>
#include <linux/highmem.h>
#include <linux/io.h>
@@ -1309,6 +1310,162 @@ static void sdhci_del_timer(struct sdhci_host *host, struct mmc_request *mrq)
del_timer(&host->timer);
}
+#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;
+
+ if (!host->mapbase)
+ return -EINVAL;
+
+ if (!data)
+ return 0;
+
+ 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;
+
+ 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)
+{
+ if (sdhci_external_dma_setup(host, cmd)) {
+ sdhci_external_dma_release(host);
+ pr_err("%s: Failed to setup external DMA, switch to the DMA/PIO which standard SDHCI provides.\n",
+ mmc_hostname(host->mmc));
+ } else {
+ /* Prepare for using external dma */
+ host->flags |= SDHCI_REQ_USE_DMA;
+ }
+
+ 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->opcode != MMC_SET_BLOCK_COUNT && cmd->data) {
+ sdhci_set_timeout(host, cmd);
+ chan = sdhci_external_dma_channel(host, cmd->data);
+ 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)
+{}
+#endif
+
+void sdhci_switch_external_dma(struct sdhci_host *host, bool en)
+{
+ host->use_external_dma = en;
+}
+EXPORT_SYMBOL_GPL(sdhci_switch_external_dma);
+
void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
{
int flags;
@@ -1355,7 +1512,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);
@@ -1397,6 +1557,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);
@@ -4133,6 +4296,19 @@ int sdhci_setup_host(struct sdhci_host *host)
return ret;
}
+ 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);
+ }
+
return 0;
unreg:
@@ -4161,6 +4337,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;
}
@@ -4295,6 +4475,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 b001cf4..8e50a97 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -475,6 +475,7 @@ struct sdhci_host {
int irq; /* Device IRQ */
void __iomem *ioaddr; /* Mapped address */
+ phys_addr_t mapbase; /* physical address base */
char *bounce_buffer; /* For packing SDMA reads/writes */
dma_addr_t bounce_addr;
unsigned int bounce_buffer_size;
@@ -524,6 +525,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;
struct mmc_request *mrqs_done[SDHCI_MAX_MRQS]; /* Requests done */
struct mmc_command *cmd; /* Current command */
@@ -552,6 +554,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 */
@@ -785,5 +792,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.7.4
Hi Faiz,
Many thanks for testing this.
On Thu, 29 Nov 2018 at 00:59, Rizvi, Mohammad Faiz Abbas
<[email protected]> wrote:
>
> + Mark Brown
>
> Chunyan,
>
> On 11/21/2018 5:17 PM, Faiz Abbas wrote:
> > Hi Chunyan,
> >
> > On 12/11/18 12:56 PM, Chunyan Zhang wrote:
> >> Currently the generic SDHCI code in the Linux kernel supports the SD
> >> standard DMA integrated into the host controller but does not have any
> >> support for external DMA controllers implemented using dmaengine meaning
> >> that custom code is needed for any systems that use a generic DMA
> >> controller with SDHCI which in practice means any SDHCI controller that
> >> doesn't have an integrated DMA controller so we should have this as a
> >> generic feature.
> >>
> >> There are already a number of controller specific drivers that have dmaengine
> >> code, and some could use sdhci.c actually, but needed to implement mmc_ops->request()
> >> in their specific driver for sending command with external dma using dmaengine
> >> framework, with this patchset, them will take advantage of the generic support.
> >> TI's omap controller is the case as an example.
> >>
> >> Any comments are very appreciated.
> >>
> >
> > This is great. It helps us move am335x and am43xx platforms to
> > sdhci-omap. What platforms have you tested this on?
> >
>
> Gentle ping on this. I tried testing these with an am335x-evm board. In
> their current condition, the card fails to enumerate altogether. The
> changes suggested by Adrian should fix this. Let me know when you post a v3.
I addressed Adrian's comments, and posted a new patch under 1/3 of the
last patch series, if this patch can be verified workable, I will send
another new patchset also add your tested-by.
Thanks,
Chunyan
>
> Thanks,
> Faiz
On Tue, 20 Nov 2018 at 21:41, Adrian Hunter <[email protected]> wrote:
>
> On 12/11/18 9:26 AM, 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.
>
> I still think you probably need to reset the DMA if there are transfer
> errors - perhaps you could comment on that. Also there are some comments below.
With regard to "transfer error", do you mean if
sdhci_external_dma_setup() failed?
Thanks,
Chunyan
>
> >
> > Signed-off-by: Chunyan Zhang <[email protected]>
> > ---
> > drivers/mmc/host/Kconfig | 13 ++++
> > drivers/mmc/host/sdhci.c | 181 ++++++++++++++++++++++++++++++++++++++++++++++-
> > drivers/mmc/host/sdhci.h | 8 +++
> > 3 files changed, 201 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> > index 1b58739..7bf3eff 100644
> > --- a/drivers/mmc/host/Kconfig
> > +++ b/drivers/mmc/host/Kconfig
> > @@ -977,3 +977,16 @@ config MMC_SDHCI_OMAP
> > If you have a controller with this interface, say Y or M here.
> >
> > If unsure, say N.
> > +
> > +config MMC_SDHCI_EXTERNAL_DMA
> > + bool "Support external DMA in standard SD host controller"
>
> It would be simpler if the drivers that needed it just selected it e.g.
>
> config MMC_SDHCI_OMAP
> tristate "TI SDHCI Controller Support"
> depends on MMC_SDHCI_PLTFM && OF
> select MMC_SDHCI_EXTERNAL_DMA if DMA_ENGINE
>
> > + depends on MMC_SDHCI
> > + depends on DMA_ENGINE
> > + help
> > + This is an option for using external DMA device via dmaengine
> > + framework.
> > +
> > + If you have a controller which support using external DMA device
> > + for data transfer, can say Y.
> > +
> > + If unsure, say N.
> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > index 99bdae5..853cc98 100644
> > --- a/drivers/mmc/host/sdhci.c
> > +++ b/drivers/mmc/host/sdhci.c
> > @@ -14,6 +14,7 @@
> > */
> >
> > #include <linux/delay.h>
> > +#include <linux/dmaengine.h>
> > #include <linux/ktime.h>
> > #include <linux/highmem.h>
> > #include <linux/io.h>
> > @@ -1309,6 +1310,158 @@ static void sdhci_del_timer(struct sdhci_host *host, struct mmc_request *mrq)
> > del_timer(&host->timer);
> > }
> >
> > +#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 = 0, i;
> > + struct dma_async_tx_descriptor *desc;
> > + struct mmc_data *data = cmd->data;
>
> cmd->data might be NULL? Have you tested this?
>
> > + struct dma_chan *chan;
> > + struct dma_slave_config cfg;
> > + dma_cookie_t cookie;
> > +
> > + 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;
> > +
> > + 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)
> > +{
> > + if (sdhci_external_dma_setup(host, cmd)) {
> > + sdhci_external_dma_release(host);
> > + pr_err("%s: Failed to setup external DMA, switch to the DMA/PIO which standard SDHCI provides.\n",
> > + mmc_hostname(host->mmc));
> > + } else {
> > + /* Prepare for using external dma */
> > + host->flags |= SDHCI_REQ_USE_DMA;
> > + }
> > +
> > + sdhci_prepare_data(host, cmd);
> > +}
> > +
> > +static void sdhci_external_dma_pre_transfer(struct sdhci_host *host,
> > + struct mmc_command *cmd)
> > +{
> > + struct dma_chan *chan = sdhci_external_dma_channel(host, cmd->data);
>
> cmd->data might be NULL? Have you tested this?
>
> > +
> > + if (cmd->opcode != MMC_SET_BLOCK_COUNT) {
>
> I would have thought:
>
> if (cmd->data)
>
> > + sdhci_set_timeout(host, cmd);
> > + dma_async_issue_pending(chan);
> > + }
> > +}
> > +#else
> > +static int sdhci_external_dma_init(struct sdhci_host *host)
> > +{
> > + return 0;
>
> That should return an error, perhaps -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 SDHCI_EXTDMA not supported, PIO will be used */
>
> SDHCI_EXTDMA is now MMC_SDHCI_EXTERNAL_DMA
>
> > + sdhci_prepare_data(host, cmd);
> > +}
> > +
> > +static void sdhci_external_dma_pre_transfer(struct sdhci_host *host,
> > + struct mmc_command *cmd)
> > +{}
> > +#endif
> > +
> > +void sdhci_switch_external_dma(struct sdhci_host *host, bool en)
> > +{
> > + host->use_external_dma = en;
> > +}
> > +EXPORT_SYMBOL_GPL(sdhci_switch_external_dma);
> > +
> > void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
> > {
> > int flags;
> > @@ -1355,7 +1508,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);
> >
> > @@ -1397,6 +1553,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);
> > @@ -4133,6 +4292,19 @@ int sdhci_setup_host(struct sdhci_host *host)
> > return ret;
> > }
> >
> > + 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);
> > + }
> > +
> > return 0;
> >
> > unreg:
> > @@ -4161,6 +4333,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;
> > }
> > @@ -4295,6 +4471,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 b001cf4..8e50a97 100644
> > --- a/drivers/mmc/host/sdhci.h
> > +++ b/drivers/mmc/host/sdhci.h
> > @@ -475,6 +475,7 @@ struct sdhci_host {
> >
> > int irq; /* Device IRQ */
> > void __iomem *ioaddr; /* Mapped address */
> > + phys_addr_t mapbase; /* physical address base */
> > char *bounce_buffer; /* For packing SDMA reads/writes */
> > dma_addr_t bounce_addr;
> > unsigned int bounce_buffer_size;
> > @@ -524,6 +525,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;
> >
> > struct mmc_request *mrqs_done[SDHCI_MAX_MRQS]; /* Requests done */
> > struct mmc_command *cmd; /* Current command */
> > @@ -552,6 +554,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 */
> > @@ -785,5 +792,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 */
> >
>
On 29/11/18 8:22 AM, Chunyan Zhang wrote:
> On Tue, 20 Nov 2018 at 21:41, Adrian Hunter <[email protected]> wrote:
>>
>> On 12/11/18 9:26 AM, 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.
>>
>> I still think you probably need to reset the DMA if there are transfer
>> errors - perhaps you could comment on that. Also there are some comments below.
>
> With regard to "transfer error", do you mean if
> sdhci_external_dma_setup() failed?
No, I mean any error interrupt that can leave the DMA uncompleted. For
SDHCI, resetting the data circuit cleans that up, but presumably something
is needed for external DMA?
>
> Thanks,
> Chunyan
>
>
>>
>>>
>>> Signed-off-by: Chunyan Zhang <[email protected]>
>>> ---
>>> drivers/mmc/host/Kconfig | 13 ++++
>>> drivers/mmc/host/sdhci.c | 181 ++++++++++++++++++++++++++++++++++++++++++++++-
>>> drivers/mmc/host/sdhci.h | 8 +++
>>> 3 files changed, 201 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
>>> index 1b58739..7bf3eff 100644
>>> --- a/drivers/mmc/host/Kconfig
>>> +++ b/drivers/mmc/host/Kconfig
>>> @@ -977,3 +977,16 @@ config MMC_SDHCI_OMAP
>>> If you have a controller with this interface, say Y or M here.
>>>
>>> If unsure, say N.
>>> +
>>> +config MMC_SDHCI_EXTERNAL_DMA
>>> + bool "Support external DMA in standard SD host controller"
>>
>> It would be simpler if the drivers that needed it just selected it e.g.
>>
>> config MMC_SDHCI_OMAP
>> tristate "TI SDHCI Controller Support"
>> depends on MMC_SDHCI_PLTFM && OF
>> select MMC_SDHCI_EXTERNAL_DMA if DMA_ENGINE
>>
>>> + depends on MMC_SDHCI
>>> + depends on DMA_ENGINE
>>> + help
>>> + This is an option for using external DMA device via dmaengine
>>> + framework.
>>> +
>>> + If you have a controller which support using external DMA device
>>> + for data transfer, can say Y.
>>> +
>>> + If unsure, say N.
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index 99bdae5..853cc98 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -14,6 +14,7 @@
>>> */
>>>
>>> #include <linux/delay.h>
>>> +#include <linux/dmaengine.h>
>>> #include <linux/ktime.h>
>>> #include <linux/highmem.h>
>>> #include <linux/io.h>
>>> @@ -1309,6 +1310,158 @@ static void sdhci_del_timer(struct sdhci_host *host, struct mmc_request *mrq)
>>> del_timer(&host->timer);
>>> }
>>>
>>> +#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 = 0, i;
>>> + struct dma_async_tx_descriptor *desc;
>>> + struct mmc_data *data = cmd->data;
>>
>> cmd->data might be NULL? Have you tested this?
>>
>>> + struct dma_chan *chan;
>>> + struct dma_slave_config cfg;
>>> + dma_cookie_t cookie;
>>> +
>>> + 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;
>>> +
>>> + 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)
>>> +{
>>> + if (sdhci_external_dma_setup(host, cmd)) {
>>> + sdhci_external_dma_release(host);
>>> + pr_err("%s: Failed to setup external DMA, switch to the DMA/PIO which standard SDHCI provides.\n",
>>> + mmc_hostname(host->mmc));
>>> + } else {
>>> + /* Prepare for using external dma */
>>> + host->flags |= SDHCI_REQ_USE_DMA;
>>> + }
>>> +
>>> + sdhci_prepare_data(host, cmd);
>>> +}
>>> +
>>> +static void sdhci_external_dma_pre_transfer(struct sdhci_host *host,
>>> + struct mmc_command *cmd)
>>> +{
>>> + struct dma_chan *chan = sdhci_external_dma_channel(host, cmd->data);
>>
>> cmd->data might be NULL? Have you tested this?
>>
>>> +
>>> + if (cmd->opcode != MMC_SET_BLOCK_COUNT) {
>>
>> I would have thought:
>>
>> if (cmd->data)
>>
>>> + sdhci_set_timeout(host, cmd);
>>> + dma_async_issue_pending(chan);
>>> + }
>>> +}
>>> +#else
>>> +static int sdhci_external_dma_init(struct sdhci_host *host)
>>> +{
>>> + return 0;
>>
>> That should return an error, perhaps -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 SDHCI_EXTDMA not supported, PIO will be used */
>>
>> SDHCI_EXTDMA is now MMC_SDHCI_EXTERNAL_DMA
>>
>>> + sdhci_prepare_data(host, cmd);
>>> +}
>>> +
>>> +static void sdhci_external_dma_pre_transfer(struct sdhci_host *host,
>>> + struct mmc_command *cmd)
>>> +{}
>>> +#endif
>>> +
>>> +void sdhci_switch_external_dma(struct sdhci_host *host, bool en)
>>> +{
>>> + host->use_external_dma = en;
>>> +}
>>> +EXPORT_SYMBOL_GPL(sdhci_switch_external_dma);
>>> +
>>> void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
>>> {
>>> int flags;
>>> @@ -1355,7 +1508,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);
>>>
>>> @@ -1397,6 +1553,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);
>>> @@ -4133,6 +4292,19 @@ int sdhci_setup_host(struct sdhci_host *host)
>>> return ret;
>>> }
>>>
>>> + 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);
>>> + }
>>> +
>>> return 0;
>>>
>>> unreg:
>>> @@ -4161,6 +4333,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;
>>> }
>>> @@ -4295,6 +4471,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 b001cf4..8e50a97 100644
>>> --- a/drivers/mmc/host/sdhci.h
>>> +++ b/drivers/mmc/host/sdhci.h
>>> @@ -475,6 +475,7 @@ struct sdhci_host {
>>>
>>> int irq; /* Device IRQ */
>>> void __iomem *ioaddr; /* Mapped address */
>>> + phys_addr_t mapbase; /* physical address base */
>>> char *bounce_buffer; /* For packing SDMA reads/writes */
>>> dma_addr_t bounce_addr;
>>> unsigned int bounce_buffer_size;
>>> @@ -524,6 +525,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;
>>>
>>> struct mmc_request *mrqs_done[SDHCI_MAX_MRQS]; /* Requests done */
>>> struct mmc_command *cmd; /* Current command */
>>> @@ -552,6 +554,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 */
>>> @@ -785,5 +792,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 */
>>>
>>
>
On 29/11/18 8:07 AM, 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.
>
> Signed-off-by: Chunyan Zhang <[email protected]>
> ---
> drivers/mmc/host/Kconfig | 14 ++++
> drivers/mmc/host/sdhci.c | 185 ++++++++++++++++++++++++++++++++++++++++++++++-
> drivers/mmc/host/sdhci.h | 8 ++
> 3 files changed, 206 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 1b58739..4183f43 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -969,6 +969,7 @@ config MMC_SDHCI_XENON
> config MMC_SDHCI_OMAP
> tristate "TI SDHCI Controller Support"
> depends on MMC_SDHCI_PLTFM && OF
> + 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
> @@ -977,3 +978,16 @@ config MMC_SDHCI_OMAP
> If you have a controller with this interface, say Y or M here.
>
> If unsure, say N.
> +
> +config MMC_SDHCI_EXTERNAL_DMA
> + bool "Support external DMA in standard SD host controller"
> + depends on MMC_SDHCI
> + depends on DMA_ENGINE
> + help
> + This is an option for using external DMA device via dmaengine
> + framework.
> +
> + If you have a controller which support using external DMA device
> + for data transfer, can say Y.
> +
> + If unsure, say N.
So if you are going to select this, then you don't need the prompt or help
anymore i.e.
config MMC_SDHCI_EXTERNAL_DMA
bool
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 99bdae5..ad7cc80 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -14,6 +14,7 @@
> */
>
> #include <linux/delay.h>
> +#include <linux/dmaengine.h>
> #include <linux/ktime.h>
> #include <linux/highmem.h>
> #include <linux/io.h>
> @@ -1309,6 +1310,162 @@ static void sdhci_del_timer(struct sdhci_host *host, struct mmc_request *mrq)
> del_timer(&host->timer);
> }
>
> +#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;
> +
> + if (!host->mapbase)
> + return -EINVAL;
> +
> + if (!data)
> + return 0;
It would read better if the above 2 if-statements were the other way around i.e.
if (!data)
return 0;
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;
> +
> + 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)
> +{
> + if (sdhci_external_dma_setup(host, cmd)) {
> + sdhci_external_dma_release(host);
> + pr_err("%s: Failed to setup external DMA, switch to the DMA/PIO which standard SDHCI provides.\n",
> + mmc_hostname(host->mmc));
> + } else {
> + /* Prepare for using external dma */
> + host->flags |= SDHCI_REQ_USE_DMA;
> + }
> +
> + 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->opcode != MMC_SET_BLOCK_COUNT && cmd->data) {
MMC_SET_BLOCK_COUNT doesn't have data so that part is not needed
> + sdhci_set_timeout(host, cmd);
> + chan = sdhci_external_dma_channel(host, cmd->data);
> + 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)
> +{}
> +#endif
> +
> +void sdhci_switch_external_dma(struct sdhci_host *host, bool en)
> +{
> + host->use_external_dma = en;
> +}
> +EXPORT_SYMBOL_GPL(sdhci_switch_external_dma);
> +
> void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
> {
> int flags;
> @@ -1355,7 +1512,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);
>
> @@ -1397,6 +1557,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);
> @@ -4133,6 +4296,19 @@ int sdhci_setup_host(struct sdhci_host *host)
> return ret;
> }
>
> + 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);
> + }
> +
> return 0;
>
> unreg:
> @@ -4161,6 +4337,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;
> }
> @@ -4295,6 +4475,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 b001cf4..8e50a97 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -475,6 +475,7 @@ struct sdhci_host {
>
> int irq; /* Device IRQ */
> void __iomem *ioaddr; /* Mapped address */
> + phys_addr_t mapbase; /* physical address base */
> char *bounce_buffer; /* For packing SDMA reads/writes */
> dma_addr_t bounce_addr;
> unsigned int bounce_buffer_size;
> @@ -524,6 +525,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;
>
> struct mmc_request *mrqs_done[SDHCI_MAX_MRQS]; /* Requests done */
> struct mmc_command *cmd; /* Current command */
> @@ -552,6 +554,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 */
> @@ -785,5 +792,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 */
>
On Thu, 29 Nov 2018 at 17:25, Adrian Hunter <[email protected]> wrote:
>
> On 29/11/18 8:07 AM, 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.
> >
> > Signed-off-by: Chunyan Zhang <[email protected]>
> > ---
> > drivers/mmc/host/Kconfig | 14 ++++
> > drivers/mmc/host/sdhci.c | 185 ++++++++++++++++++++++++++++++++++++++++++++++-
> > drivers/mmc/host/sdhci.h | 8 ++
> > 3 files changed, 206 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> > index 1b58739..4183f43 100644
> > --- a/drivers/mmc/host/Kconfig
> > +++ b/drivers/mmc/host/Kconfig
> > @@ -969,6 +969,7 @@ config MMC_SDHCI_XENON
> > config MMC_SDHCI_OMAP
> > tristate "TI SDHCI Controller Support"
> > depends on MMC_SDHCI_PLTFM && OF
> > + 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
> > @@ -977,3 +978,16 @@ config MMC_SDHCI_OMAP
> > If you have a controller with this interface, say Y or M here.
> >
> > If unsure, say N.
> > +
> > +config MMC_SDHCI_EXTERNAL_DMA
> > + bool "Support external DMA in standard SD host controller"
> > + depends on MMC_SDHCI
> > + depends on DMA_ENGINE
> > + help
> > + This is an option for using external DMA device via dmaengine
> > + framework.
> > +
> > + If you have a controller which support using external DMA device
> > + for data transfer, can say Y.
> > +
> > + If unsure, say N.
>
> So if you are going to select this, then you don't need the prompt or help
> anymore i.e.
>
> config MMC_SDHCI_EXTERNAL_DMA
> bool
>
>
> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > index 99bdae5..ad7cc80 100644
> > --- a/drivers/mmc/host/sdhci.c
> > +++ b/drivers/mmc/host/sdhci.c
> > @@ -14,6 +14,7 @@
> > */
> >
> > #include <linux/delay.h>
> > +#include <linux/dmaengine.h>
> > #include <linux/ktime.h>
> > #include <linux/highmem.h>
> > #include <linux/io.h>
> > @@ -1309,6 +1310,162 @@ static void sdhci_del_timer(struct sdhci_host *host, struct mmc_request *mrq)
> > del_timer(&host->timer);
> > }
> >
> > +#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;
> > +
> > + if (!host->mapbase)
> > + return -EINVAL;
> > +
> > + if (!data)
> > + return 0;
>
> It would read better if the above 2 if-statements were the other way around i.e.
>
> if (!data)
> return 0;
>
> if (!host->mapbase)
> return -EINVAL;
>
Ok.
> > +
> > + 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;
> > +
> > + 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)
> > +{
> > + if (sdhci_external_dma_setup(host, cmd)) {
> > + sdhci_external_dma_release(host);
> > + pr_err("%s: Failed to setup external DMA, switch to the DMA/PIO which standard SDHCI provides.\n",
> > + mmc_hostname(host->mmc));
> > + } else {
> > + /* Prepare for using external dma */
> > + host->flags |= SDHCI_REQ_USE_DMA;
> > + }
> > +
> > + 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->opcode != MMC_SET_BLOCK_COUNT && cmd->data) {
>
> MMC_SET_BLOCK_COUNT doesn't have data so that part is not needed
Didn't get you here.
This is for other packets except MMC_SET_BLOCK_COUNT.
>
> > + sdhci_set_timeout(host, cmd);
> > + chan = sdhci_external_dma_channel(host, cmd->data);
> > + 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)
> > +{}
> > +#endif
> > +
> > +void sdhci_switch_external_dma(struct sdhci_host *host, bool en)
> > +{
> > + host->use_external_dma = en;
> > +}
> > +EXPORT_SYMBOL_GPL(sdhci_switch_external_dma);
> > +
> > void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
> > {
> > int flags;
> > @@ -1355,7 +1512,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);
> >
> > @@ -1397,6 +1557,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);
> > @@ -4133,6 +4296,19 @@ int sdhci_setup_host(struct sdhci_host *host)
> > return ret;
> > }
> >
> > + 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);
> > + }
> > +
> > return 0;
> >
> > unreg:
> > @@ -4161,6 +4337,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;
> > }
> > @@ -4295,6 +4475,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 b001cf4..8e50a97 100644
> > --- a/drivers/mmc/host/sdhci.h
> > +++ b/drivers/mmc/host/sdhci.h
> > @@ -475,6 +475,7 @@ struct sdhci_host {
> >
> > int irq; /* Device IRQ */
> > void __iomem *ioaddr; /* Mapped address */
> > + phys_addr_t mapbase; /* physical address base */
> > char *bounce_buffer; /* For packing SDMA reads/writes */
> > dma_addr_t bounce_addr;
> > unsigned int bounce_buffer_size;
> > @@ -524,6 +525,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;
> >
> > struct mmc_request *mrqs_done[SDHCI_MAX_MRQS]; /* Requests done */
> > struct mmc_command *cmd; /* Current command */
> > @@ -552,6 +554,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 */
> > @@ -785,5 +792,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 */
> >
>
Hi Adrian,
On Thu, 29 Nov 2018 at 15:36, Adrian Hunter <[email protected]> wrote:
>
> On 29/11/18 8:22 AM, Chunyan Zhang wrote:
> > On Tue, 20 Nov 2018 at 21:41, Adrian Hunter <[email protected]> wrote:
> >>
> >> On 12/11/18 9:26 AM, 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.
> >>
> >> I still think you probably need to reset the DMA if there are transfer
> >> errors - perhaps you could comment on that. Also there are some comments below.
> >
> > With regard to "transfer error", do you mean if
> > sdhci_external_dma_setup() failed?
>
> No, I mean any error interrupt that can leave the DMA uncompleted. For
> SDHCI, resetting the data circuit cleans that up, but presumably something
> is needed for external DMA?
Yes, it should need a dmaengine_terminate_all().
How about adding that at here (I will wrap it up of course):
https://elixir.bootlin.com/linux/v4.19.5/source/drivers/mmc/host/sdhci.c#L2553
Is there somewhere else I'm missing?
Thanks,
Chunyan
On 29/11/18 11:44 AM, Chunyan Zhang wrote:
> On Thu, 29 Nov 2018 at 17:25, Adrian Hunter <[email protected]> wrote:
>>
>> On 29/11/18 8:07 AM, 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.
>>>
>>> Signed-off-by: Chunyan Zhang <[email protected]>
>>> ---
>>> drivers/mmc/host/Kconfig | 14 ++++
>>> drivers/mmc/host/sdhci.c | 185 ++++++++++++++++++++++++++++++++++++++++++++++-
>>> drivers/mmc/host/sdhci.h | 8 ++
>>> 3 files changed, 206 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
>>> index 1b58739..4183f43 100644
>>> --- a/drivers/mmc/host/Kconfig
>>> +++ b/drivers/mmc/host/Kconfig
>>> @@ -969,6 +969,7 @@ config MMC_SDHCI_XENON
>>> config MMC_SDHCI_OMAP
>>> tristate "TI SDHCI Controller Support"
>>> depends on MMC_SDHCI_PLTFM && OF
>>> + 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
>>> @@ -977,3 +978,16 @@ config MMC_SDHCI_OMAP
>>> If you have a controller with this interface, say Y or M here.
>>>
>>> If unsure, say N.
>>> +
>>> +config MMC_SDHCI_EXTERNAL_DMA
>>> + bool "Support external DMA in standard SD host controller"
>>> + depends on MMC_SDHCI
>>> + depends on DMA_ENGINE
>>> + help
>>> + This is an option for using external DMA device via dmaengine
>>> + framework.
>>> +
>>> + If you have a controller which support using external DMA device
>>> + for data transfer, can say Y.
>>> +
>>> + If unsure, say N.
>>
>> So if you are going to select this, then you don't need the prompt or help
>> anymore i.e.
>>
>> config MMC_SDHCI_EXTERNAL_DMA
>> bool
>>
>>
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index 99bdae5..ad7cc80 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -14,6 +14,7 @@
>>> */
>>>
>>> #include <linux/delay.h>
>>> +#include <linux/dmaengine.h>
>>> #include <linux/ktime.h>
>>> #include <linux/highmem.h>
>>> #include <linux/io.h>
>>> @@ -1309,6 +1310,162 @@ static void sdhci_del_timer(struct sdhci_host *host, struct mmc_request *mrq)
>>> del_timer(&host->timer);
>>> }
>>>
>>> +#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;
>>> +
>>> + if (!host->mapbase)
>>> + return -EINVAL;
>>> +
>>> + if (!data)
>>> + return 0;
>>
>> It would read better if the above 2 if-statements were the other way around i.e.
>>
>> if (!data)
>> return 0;
>>
>> if (!host->mapbase)
>> return -EINVAL;
>>
>
> Ok.
>
>>> +
>>> + 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;
>>> +
>>> + 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)
>>> +{
>>> + if (sdhci_external_dma_setup(host, cmd)) {
>>> + sdhci_external_dma_release(host);
>>> + pr_err("%s: Failed to setup external DMA, switch to the DMA/PIO which standard SDHCI provides.\n",
>>> + mmc_hostname(host->mmc));
>>> + } else {
>>> + /* Prepare for using external dma */
>>> + host->flags |= SDHCI_REQ_USE_DMA;
>>> + }
>>> +
>>> + 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->opcode != MMC_SET_BLOCK_COUNT && cmd->data) {
>>
>> MMC_SET_BLOCK_COUNT doesn't have data so that part is not needed
>
> Didn't get you here.
> This is for other packets except MMC_SET_BLOCK_COUNT.
>
MMC_SET_BLOCK_COUNT is not a data transfer command, so cmd->data == NULL anyway.
On Thu, 29 Nov 2018 at 18:41, Adrian Hunter <[email protected]> wrote:
>
> On 29/11/18 11:44 AM, Chunyan Zhang wrote:
> > On Thu, 29 Nov 2018 at 17:25, Adrian Hunter <[email protected]> wrote:
> >>
> >> On 29/11/18 8:07 AM, 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.
> >>>
> >>> Signed-off-by: Chunyan Zhang <[email protected]>
> >>> ---
> >>> drivers/mmc/host/Kconfig | 14 ++++
> >>> drivers/mmc/host/sdhci.c | 185 ++++++++++++++++++++++++++++++++++++++++++++++-
> >>> drivers/mmc/host/sdhci.h | 8 ++
> >>> 3 files changed, 206 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> >>> index 1b58739..4183f43 100644
> >>> --- a/drivers/mmc/host/Kconfig
> >>> +++ b/drivers/mmc/host/Kconfig
> >>> @@ -969,6 +969,7 @@ config MMC_SDHCI_XENON
> >>> config MMC_SDHCI_OMAP
> >>> tristate "TI SDHCI Controller Support"
> >>> depends on MMC_SDHCI_PLTFM && OF
> >>> + 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
> >>> @@ -977,3 +978,16 @@ config MMC_SDHCI_OMAP
> >>> If you have a controller with this interface, say Y or M here.
> >>>
> >>> If unsure, say N.
> >>> +
> >>> +config MMC_SDHCI_EXTERNAL_DMA
> >>> + bool "Support external DMA in standard SD host controller"
> >>> + depends on MMC_SDHCI
> >>> + depends on DMA_ENGINE
> >>> + help
> >>> + This is an option for using external DMA device via dmaengine
> >>> + framework.
> >>> +
> >>> + If you have a controller which support using external DMA device
> >>> + for data transfer, can say Y.
> >>> +
> >>> + If unsure, say N.
> >>
> >> So if you are going to select this, then you don't need the prompt or help
> >> anymore i.e.
> >>
> >> config MMC_SDHCI_EXTERNAL_DMA
> >> bool
> >>
> >>
> >>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> >>> index 99bdae5..ad7cc80 100644
> >>> --- a/drivers/mmc/host/sdhci.c
> >>> +++ b/drivers/mmc/host/sdhci.c
> >>> @@ -14,6 +14,7 @@
> >>> */
> >>>
> >>> #include <linux/delay.h>
> >>> +#include <linux/dmaengine.h>
> >>> #include <linux/ktime.h>
> >>> #include <linux/highmem.h>
> >>> #include <linux/io.h>
> >>> @@ -1309,6 +1310,162 @@ static void sdhci_del_timer(struct sdhci_host *host, struct mmc_request *mrq)
> >>> del_timer(&host->timer);
> >>> }
> >>>
> >>> +#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;
> >>> +
> >>> + if (!host->mapbase)
> >>> + return -EINVAL;
> >>> +
> >>> + if (!data)
> >>> + return 0;
> >>
> >> It would read better if the above 2 if-statements were the other way around i.e.
> >>
> >> if (!data)
> >> return 0;
> >>
> >> if (!host->mapbase)
> >> return -EINVAL;
> >>
> >
> > Ok.
> >
> >>> +
> >>> + 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;
> >>> +
> >>> + 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)
> >>> +{
> >>> + if (sdhci_external_dma_setup(host, cmd)) {
> >>> + sdhci_external_dma_release(host);
> >>> + pr_err("%s: Failed to setup external DMA, switch to the DMA/PIO which standard SDHCI provides.\n",
> >>> + mmc_hostname(host->mmc));
> >>> + } else {
> >>> + /* Prepare for using external dma */
> >>> + host->flags |= SDHCI_REQ_USE_DMA;
> >>> + }
> >>> +
> >>> + 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->opcode != MMC_SET_BLOCK_COUNT && cmd->data) {
> >>
> >> MMC_SET_BLOCK_COUNT doesn't have data so that part is not needed
> >
> > Didn't get you here.
> > This is for other packets except MMC_SET_BLOCK_COUNT.
> >
>
> MMC_SET_BLOCK_COUNT is not a data transfer command, so cmd->data == NULL anyway.
Oh, got you.
"if (cmd->data)" is enough here :)
Thank you,
Chunyan
On 29/11/18 11:59 AM, Chunyan Zhang wrote:
> Hi Adrian,
>
> On Thu, 29 Nov 2018 at 15:36, Adrian Hunter <[email protected]> wrote:
>>
>> On 29/11/18 8:22 AM, Chunyan Zhang wrote:
>>> On Tue, 20 Nov 2018 at 21:41, Adrian Hunter <[email protected]> wrote:
>>>>
>>>> On 12/11/18 9:26 AM, 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.
>>>>
>>>> I still think you probably need to reset the DMA if there are transfer
>>>> errors - perhaps you could comment on that. Also there are some comments below.
>>>
>>> With regard to "transfer error", do you mean if
>>> sdhci_external_dma_setup() failed?
>>
>> No, I mean any error interrupt that can leave the DMA uncompleted. For
>> SDHCI, resetting the data circuit cleans that up, but presumably something
>> is needed for external DMA?
>
> Yes, it should need a dmaengine_terminate_all().
>
> How about adding that at here (I will wrap it up of course):
> https://elixir.bootlin.com/linux/v4.19.5/source/drivers/mmc/host/sdhci.c#L2553
Yes except we really need to reverse
if (host->flags & SDHCI_REQ_USE_DMA) {
}
if (sdhci_needs_reset(host, mrq)) {
}
so that we do not unmap before killing the dma
Perhaps you could send that as a separate patch.
> Is there somewhere else I'm missing?
Testing ;-)
On 2018/11/29 17:59, Chunyan Zhang wrote:
> Hi Adrian,
>
> On Thu, 29 Nov 2018 at 15:36, Adrian Hunter <[email protected]> wrote:
>>
>> On 29/11/18 8:22 AM, Chunyan Zhang wrote:
>>> On Tue, 20 Nov 2018 at 21:41, Adrian Hunter <[email protected]> wrote:
>>>>
>>>> On 12/11/18 9:26 AM, 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.
>>>>
>>>> I still think you probably need to reset the DMA if there are transfer
>>>> errors - perhaps you could comment on that. Also there are some comments below.
>>>
>>> With regard to "transfer error", do you mean if
>>> sdhci_external_dma_setup() failed?
>>
>> No, I mean any error interrupt that can leave the DMA uncompleted. For
>> SDHCI, resetting the data circuit cleans that up, but presumably something
>> is needed for external DMA?
>
> Yes, it should need a dmaengine_terminate_all().
>
No, dmaengine_terminate_all is deprecated for quite a long time.
Please use dmaengine_terminate_{async, sync}() instead.
See Documentation/dmaengine/client.txt
> How about adding that at here (I will wrap it up of course):
> https://elixir.bootlin.com/linux/v4.19.5/source/drivers/mmc/host/sdhci.c#L2553
>
> Is there somewhere else I'm missing?
>
> Thanks,
> Chunyan
>
>
Hi Chunyan,
On 29/11/18 2:53 PM, Adrian Hunter wrote:
> On 29/11/18 8:07 AM, 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.
>>
>> Signed-off-by: Chunyan Zhang <[email protected]>
>> ---
>> drivers/mmc/host/Kconfig | 14 ++++
>> drivers/mmc/host/sdhci.c | 185 ++++++++++++++++++++++++++++++++++++++++++++++-
>> drivers/mmc/host/sdhci.h | 8 ++
>> 3 files changed, 206 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
>> index 1b58739..4183f43 100644
>> --- a/drivers/mmc/host/Kconfig
>> +++ b/drivers/mmc/host/Kconfig
>> @@ -969,6 +969,7 @@ config MMC_SDHCI_XENON
>> config MMC_SDHCI_OMAP
>> tristate "TI SDHCI Controller Support">> depends on MMC_SDHCI_PLTFM && OF
>> + select MMC_SDHCI_EXTERNAL_DMA if DMA_ENGINE
This should be in patch 2.
Thanks,
Faiz
On Mon, 3 Dec 2018 at 17:01, Faiz Abbas <[email protected]> wrote:
>
> Hi Chunyan,
>
> On 29/11/18 2:53 PM, Adrian Hunter wrote:
> > On 29/11/18 8:07 AM, 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.
> >>
> >> Signed-off-by: Chunyan Zhang <[email protected]>
> >> ---
> >> drivers/mmc/host/Kconfig | 14 ++++
> >> drivers/mmc/host/sdhci.c | 185 ++++++++++++++++++++++++++++++++++++++++++++++-
> >> drivers/mmc/host/sdhci.h | 8 ++
> >> 3 files changed, 206 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> >> index 1b58739..4183f43 100644
> >> --- a/drivers/mmc/host/Kconfig
> >> +++ b/drivers/mmc/host/Kconfig
> >> @@ -969,6 +969,7 @@ config MMC_SDHCI_XENON
> >> config MMC_SDHCI_OMAP
> >> tristate "TI SDHCI Controller Support">> depends on MMC_SDHCI_PLTFM && OF
> >> + select MMC_SDHCI_EXTERNAL_DMA if DMA_ENGINE
>
>
> This should be in patch 2.
Ok, will move to.
Thanks,
Chunyan
>
> Thanks,
> Faiz
On Fri, 30 Nov 2018 at 09:15, Shawn Lin <[email protected]> wrote:
>
> On 2018/11/29 17:59, Chunyan Zhang wrote:
> > Hi Adrian,
> >
> > On Thu, 29 Nov 2018 at 15:36, Adrian Hunter <[email protected]> wrote:
> >>
> >> On 29/11/18 8:22 AM, Chunyan Zhang wrote:
> >>> On Tue, 20 Nov 2018 at 21:41, Adrian Hunter <[email protected]> wrote:
> >>>>
> >>>> On 12/11/18 9:26 AM, 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.
> >>>>
> >>>> I still think you probably need to reset the DMA if there are transfer
> >>>> errors - perhaps you could comment on that. Also there are some comments below.
> >>>
> >>> With regard to "transfer error", do you mean if
> >>> sdhci_external_dma_setup() failed?
> >>
> >> No, I mean any error interrupt that can leave the DMA uncompleted. For
> >> SDHCI, resetting the data circuit cleans that up, but presumably something
> >> is needed for external DMA?
> >
> > Yes, it should need a dmaengine_terminate_all().
> >
>
> No, dmaengine_terminate_all is deprecated for quite a long time.
> Please use dmaengine_terminate_{async, sync}() instead.
>
> See Documentation/dmaengine/client.txt
Ok, thanks for the comments.
Chunyan
>
>
> > How about adding that at here (I will wrap it up of course):
> > https://elixir.bootlin.com/linux/v4.19.5/source/drivers/mmc/host/sdhci.c#L2553
> >
> > Is there somewhere else I'm missing?
> >
> > Thanks,
> > Chunyan
> >
> >
>
Hi,
On 03/12/18 5:45 PM, Faiz Abbas wrote:
> Hi,
>
>> +static void sdhci_external_dma_prepare_data(struct sdhci_host *host,
>> + struct mmc_command *cmd)
>> +{
>
> Please add a condition for data == NULL here. This was already pointed
> out by Adrian in v2.
>
> My test with an am335x-evm failed with these patches. Looks like the
> very first SDIO commands failing.
>
> https://pastebin.ubuntu.com/p/Y2RDjSKpgd/
>
> Currently am335x-evm is using omap_hsmmc driver. I added the following
> patch to make it work with sdhci_omap.
>
> https://pastebin.ubuntu.com/p/VTGrCbJxY3/
>
> Will look deeper into this. Please ping if you need any more information.
>
So I disabled DMA in the driver altogether and still got the same
messages on am335x-evm in PIO mode. Looks like something more is
required for it to be supported.
I instead shifted to a dra71-evm which supports both ADMA and external
DMA. Here is the log:
https://pastebin.ubuntu.com/p/mcJmgcjQsp/
The interface fundamentally works but it complains with the following error:
[3.111693] Failed to request TX DMA channel.
Thanks,
Faiz
Hi Faiz,
Thanks for the review and test!
On Mon, 3 Dec 2018 at 21:47, Faiz Abbas <[email protected]> wrote:
>
> Chunyan,
>
> On 12/11/18 12:56 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 <[email protected]>
> > ---
> > drivers/mmc/host/sdhci-omap.c | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c
> > index 88347ce..ccc79f2 100644
> > --- a/drivers/mmc/host/sdhci-omap.c
> > +++ b/drivers/mmc/host/sdhci-omap.c
> > @@ -896,6 +896,7 @@ static int sdhci_omap_probe(struct platform_device *pdev)
> > const struct of_device_id *match;
> > struct sdhci_omap_data *data;
> > const struct soc_device_attribute *soc;
> > + struct resource *regs;
> >
> > match = of_match_device(omap_sdhci_match, dev);
> > if (!match)
> > @@ -908,6 +909,10 @@ static int sdhci_omap_probe(struct platform_device *pdev)
> > }
> > offset = data->offset;
> >
> > + regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (!regs)
> > + return -ENXIO;
> > +
> > host = sdhci_pltfm_init(pdev, &sdhci_omap_pdata,
> > sizeof(*omap_host));
> > if (IS_ERR(host)) {
> > @@ -924,6 +929,7 @@ static int sdhci_omap_probe(struct platform_device *pdev)
> > omap_host->timing = MMC_TIMING_LEGACY;
> > omap_host->flags = data->flags;
> > host->ioaddr += offset;
> > + host->mapbase = regs->start;
> >
> > mmc = host->mmc;
> > sdhci_get_of_property(pdev);
> > @@ -991,6 +997,7 @@ static int sdhci_omap_probe(struct platform_device *pdev)
> > host->mmc_host_ops.execute_tuning = sdhci_omap_execute_tuning;
> > host->mmc_host_ops.enable_sdio_irq = sdhci_omap_enable_sdio_irq;
> >
> > + sdhci_switch_external_dma(host, true);
>
> You should give the user a choice based on a dt property whether to use
> ADMA or use external DMA. We might still want to use external DMA for
> some platforms and ADMA for other platforms.
With the current patchset, if users want to use external DMA, they
need to add 'dmas' in device tree, like patch 3 shows:
+ dmas = <&sdma 61 &sdma 62>;
+ dma-names = "tx", "rx";
Otherwise, sdhci.c will switch back to use the DMA/PIO integrated in
standard SDHCI.
But I think your suggestion makes sense, will add the below graph in
next version:
+ /* 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);
Thanks,
Chunyan
Hi Faiz,
On Mon, 3 Dec 2018 at 21:55, Faiz Abbas <[email protected]> wrote:
>
> Hi,
>
> On 03/12/18 5:45 PM, Faiz Abbas wrote:
> > Hi,
> >
> >> +static void sdhci_external_dma_prepare_data(struct sdhci_host *host,
> >> + struct mmc_command *cmd)
> >> +{
> >
> > Please add a condition for data == NULL here. This was already pointed
> > out by Adrian in v2.
The check for data is added in sdhci_external_dma_setup() .
> >
> > My test with an am335x-evm failed with these patches. Looks like the
> > very first SDIO commands failing.
I guess you didn't add 'dmas' in device tree, like patch 3 shows.
> >
> > https://pastebin.ubuntu.com/p/Y2RDjSKpgd/
> >
> > Currently am335x-evm is using omap_hsmmc driver. I added the following
> > patch to make it work with sdhci_omap.
> >
> > https://pastebin.ubuntu.com/p/VTGrCbJxY3/
> >
> > Will look deeper into this. Please ping if you need any more information.
> >
>
> So I disabled DMA in the driver altogether and still got the same
> messages on am335x-evm in PIO mode. Looks like something more is
> required for it to be supported.
>
> I instead shifted to a dra71-evm which supports both ADMA and external
> DMA. Here is the log:
>
> https://pastebin.ubuntu.com/p/mcJmgcjQsp/
>
> The interface fundamentally works but it complains with the following error:
>
Yes, it switched back to ADMA/PIO since sdhci couldn't find 'dmas'
property in devicetree.
> [3.111693] Failed to request TX DMA channel.
Ok, I will add a check in sdhci-omap.c before switching to external
dma, that should be able to avoid this error logs.
Thanks for the review and test!
Chunyan