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

Message ID 1541387810-24867-3-git-send-email-zhang.chunyan@linaro.org
State Superseded
Headers show
Series
  • [RFC,1/3] mmc: sdhci: add support for using external DMA devices
Related show

Commit Message

Chunyan Zhang Nov. 5, 2018, 3:16 a.m.
sdhci-omap can support both external dma controllers via dmaengine
framework as well as ADMA in which the controller acts as DMA master.

Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>

---
 drivers/mmc/host/sdhci-omap.c | 7 +++++++
 1 file changed, 7 insertions(+)

-- 
2.7.4

Comments

Kishon Vijay Abraham I Nov. 5, 2018, 4:25 a.m. | #1
Hi,

On 05/11/18 8:46 AM, Chunyan Zhang wrote:
> sdhci-omap can support both external dma controllers via dmaengine

> framework as well as ADMA in which the controller acts as DMA master.

> 

> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>

> ---

>  drivers/mmc/host/sdhci-omap.c | 7 +++++++

>  1 file changed, 7 insertions(+)

> 

> diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c

> index 88347ce..0a8162c 100644

> --- a/drivers/mmc/host/sdhci-omap.c

> +++ b/drivers/mmc/host/sdhci-omap.c

> @@ -896,6 +896,7 @@ static int sdhci_omap_probe(struct platform_device *pdev)

>  	const struct of_device_id *match;

>  	struct sdhci_omap_data *data;

>  	const struct soc_device_attribute *soc;

> +	struct resource *regs;

>  

>  	match = of_match_device(omap_sdhci_match, dev);

>  	if (!match)

> @@ -908,6 +909,10 @@ static int sdhci_omap_probe(struct platform_device *pdev)

>  	}

>  	offset = data->offset;

>  

> +	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);

> +	if (!regs)

> +		return -ENXIO;

> +

>  	host = sdhci_pltfm_init(pdev, &sdhci_omap_pdata,

>  				sizeof(*omap_host));

>  	if (IS_ERR(host)) {

> @@ -924,6 +929,7 @@ static int sdhci_omap_probe(struct platform_device *pdev)

>  	omap_host->timing = MMC_TIMING_LEGACY;

>  	omap_host->flags = data->flags;

>  	host->ioaddr += offset;

> +	host->mapbase = regs->start;

>  

>  	mmc = host->mmc;

>  	sdhci_get_of_property(pdev);

> @@ -991,6 +997,7 @@ static int sdhci_omap_probe(struct platform_device *pdev)

>  	host->mmc_host_ops.execute_tuning = sdhci_omap_execute_tuning;

>  	host->mmc_host_ops.enable_sdio_irq = sdhci_omap_enable_sdio_irq;

>  

> +	sdhci_switch_extdma(host, true);


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

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

Thanks
Kishon
Chunyan Zhang Nov. 7, 2018, 3 a.m. | #2
On Tue, 6 Nov 2018 at 20:51, Arnd Bergmann <arnd@arndb.de> wrote:
>

> On 11/5/18, Kishon Vijay Abraham I <kishon@ti.com> wrote:

> > On 05/11/18 8:46 AM, Chunyan Zhang wrote:

> >>

> >> +    sdhci_switch_extdma(host, true);

> >

> > A number of devices using sdhci-omap supports ADMA. So switching to

> > external

> > DMA shouldn't be unconditional.

> >

> > IMHO sdhci.c should see if the device supports ADMA or SDMA. If not it

> > should

> > try switching to external DMA and if external DMA too is not supported, it

> > should use PIO.

>

> What's the reasoning for preferring ADMA/SDMA over external DMA if

> both are supported?

>

> I'd expect that if the external DMA for some reason is worse than

> ADMA, we just wouldn't list it in the DT at all, but if it's listed and

> ADMA also works, then the driver should try to use it before falling

> back to ADMA.


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

Thanks again,
Chunyan

>

>        Arnd
Kishon Vijay Abraham I Nov. 9, 2018, 5:27 a.m. | #3
Hi Arnd,

On 06/11/18 6:21 PM, Arnd Bergmann wrote:
> On 11/5/18, Kishon Vijay Abraham I <kishon@ti.com> wrote:

>> On 05/11/18 8:46 AM, Chunyan Zhang wrote:

>>>

>>> +	sdhci_switch_extdma(host, true);

>>

>> A number of devices using sdhci-omap supports ADMA. So switching to

>> external

>> DMA shouldn't be unconditional.

>>

>> IMHO sdhci.c should see if the device supports ADMA or SDMA. If not it

>> should

>> try switching to external DMA and if external DMA too is not supported, it

>> should use PIO.

> 

> What's the reasoning for preferring ADMA/SDMA over external DMA if

> both are supported?


Generally from our experiments we've found ADMA gives better throughput than
DMA. I have to ascertain the actual reasons for that.
> 

> I'd expect that if the external DMA for some reason is worse than

> ADMA, we just wouldn't list it in the DT at all, but if it's listed and

> ADMA also works, then the driver should try to use it before falling

> back to ADMA.


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

Thanks
Kishon

Patch

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