[PATCH RFC 0/4] mmc: host: renesas_sdhi_sys_dmac: change dma_buswidth

[PATCH RFC 0/4] mmc: host: renesas_sdhi_sys_dmac: change dma_buswidth

[Yoshihiro Shimoda]

This patch can improve performance when a sd card transfer size
is multiples of 32 like a sd memory card. However, this may
disimprove performance when a sd card transfer size is not
multiples of 32 because this patch uses PIO instead of SYS-DMAC.
So, I marked as "RFC" and sent linux-renesas-soc ML only
for the reference.

Yoshihiro Shimoda (4):
  mmc: host: renesas_sdhi_sys_dmac: Use dma_buswidth instead of
    hardcoded value
  mmc: host: renesas_sdhi_sys_dmac: Do not fall back to PIO
  mmc: host: renesas_sdhi_sys_dmac: add DMACR setting
  mmc: host: renesas_sdhi_sys_dmac: Set dma_buswidth value to 32 byte

 drivers/mmc/host/renesas_sdhi_core.c     | 35 +++++++++++++++++++++++
 drivers/mmc/host/renesas_sdhi_sys_dmac.c | 49 +++++++++-----------------------
 2 files changed, 49 insertions(+), 35 deletions(-)

-- 
2.7.4

[PATCH RFC 1/4] mmc: host: renesas_sdhi_sys_dmac: Use dma_buswidth instead of hardcoded value

[Yoshihiro Shimoda]

This patch uses dma_buswidth instread of hardcoded value of
TMIO_MMC_MIN_DMA_LEN (8) for increasing the buswidth in the future.
Note that, since the dma_buswidth is 4 and the align is 2,
when the sg_tmp->length is 6, it cannot transfer correcly.
So, this patch changes the conditions.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/mmc/host/renesas_sdhi_sys_dmac.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi_sys_dmac.c b/drivers/mmc/host/renesas_sdhi_sys_dmac.c
index 13ff023..30f34a3 100644
--- a/drivers/mmc/host/renesas_sdhi_sys_dmac.c
+++ b/drivers/mmc/host/renesas_sdhi_sys_dmac.c
@@ -23,8 +23,6 @@
 #include "renesas_sdhi.h"
 #include "tmio_mmc.h"
 
-#define TMIO_MMC_MIN_DMA_LEN 8
-
 static const struct renesas_sdhi_of_data of_default_cfg = {
 	.tmio_flags = TMIO_MMC_HAS_IDLE_WAIT,
 };
@@ -159,11 +157,12 @@ static void renesas_sdhi_sys_dmac_start_dma_rx(struct tmio_mmc_host *host)
 	int ret, i;
 	bool aligned = true, multiple = true;
 	unsigned int align = (1 << host->pdata->alignment_shift) - 1;
+	unsigned int min_len = priv->dma_priv.dma_buswidth ? : align + 1;
 
 	for_each_sg(sg, sg_tmp, host->sg_len, i) {
 		if (sg_tmp->offset & align)
 			aligned = false;
-		if (sg_tmp->length & align) {
+		if (sg_tmp->length % min_len) {
 			multiple = false;
 			break;
 		}
@@ -175,7 +174,7 @@ static void renesas_sdhi_sys_dmac_start_dma_rx(struct tmio_mmc_host *host)
 		goto pio;
 	}
 
-	if (sg->length < TMIO_MMC_MIN_DMA_LEN)
+	if (sg->length < min_len)
 		return;
 
 	/* The only sg element can be unaligned, use our bounce buffer then */
@@ -231,11 +230,12 @@ static void renesas_sdhi_sys_dmac_start_dma_tx(struct tmio_mmc_host *host)
 	int ret, i;
 	bool aligned = true, multiple = true;
 	unsigned int align = (1 << host->pdata->alignment_shift) - 1;
+	unsigned int min_len = priv->dma_priv.dma_buswidth ? : align + 1;
 
 	for_each_sg(sg, sg_tmp, host->sg_len, i) {
 		if (sg_tmp->offset & align)
 			aligned = false;
-		if (sg_tmp->length & align) {
+		if (sg_tmp->length % min_len) {
 			multiple = false;
 			break;
 		}
@@ -247,7 +247,7 @@ static void renesas_sdhi_sys_dmac_start_dma_tx(struct tmio_mmc_host *host)
 		goto pio;
 	}
 
-	if (sg->length < TMIO_MMC_MIN_DMA_LEN)
+	if (sg->length < min_len)
 		return;
 
 	/* The only sg element can be unaligned, use our bounce buffer then */
-- 
2.7.4

[PATCH RFC 2/4] mmc: host: renesas_sdhi_sys_dmac: Do not fall back to PIO

[Yoshihiro Shimoda]

Falling back to PIO forever is not convenience if a buffer condition
is not match with the hardware once. So, this patch removes it.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/mmc/host/renesas_sdhi_sys_dmac.c | 35 +++++++-------------------------
 1 file changed, 7 insertions(+), 28 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi_sys_dmac.c b/drivers/mmc/host/renesas_sdhi_sys_dmac.c
index 30f34a3..09137cc 100644
--- a/drivers/mmc/host/renesas_sdhi_sys_dmac.c
+++ b/drivers/mmc/host/renesas_sdhi_sys_dmac.c
@@ -175,7 +175,7 @@ static void renesas_sdhi_sys_dmac_start_dma_rx(struct tmio_mmc_host *host)
 	}
 
 	if (sg->length < min_len)
-		return;
+		goto pio;
 
 	/* The only sg element can be unaligned, use our bounce buffer then */
 	if (!aligned) {
@@ -200,23 +200,13 @@ static void renesas_sdhi_sys_dmac_start_dma_rx(struct tmio_mmc_host *host)
 			ret = cookie;
 		}
 		host->dma_on = true;
+		renesas_sdhi_sys_dmac_enable_dma(host, true);
 	}
 pio:
 	if (!desc) {
-		/* DMA failed, fall back to PIO */
 		renesas_sdhi_sys_dmac_enable_dma(host, false);
-		if (ret >= 0)
-			ret = -EIO;
-		host->chan_rx = NULL;
-		dma_release_channel(chan);
-		/* Free the Tx channel too */
-		chan = host->chan_tx;
-		if (chan) {
-			host->chan_tx = NULL;
-			dma_release_channel(chan);
-		}
-		dev_warn(&host->pdev->dev,
-			 "DMA failed: %d, falling back to PIO\n", ret);
+		host->dma_on = false;
+		dev_dbg(&host->pdev->dev, "DMA failed: %d\n", ret);
 	}
 }
 
@@ -248,7 +238,7 @@ static void renesas_sdhi_sys_dmac_start_dma_tx(struct tmio_mmc_host *host)
 	}
 
 	if (sg->length < min_len)
-		return;
+		goto pio;
 
 	/* The only sg element can be unaligned, use our bounce buffer then */
 	if (!aligned) {
@@ -281,20 +271,9 @@ static void renesas_sdhi_sys_dmac_start_dma_tx(struct tmio_mmc_host *host)
 	}
 pio:
 	if (!desc) {
-		/* DMA failed, fall back to PIO */
 		renesas_sdhi_sys_dmac_enable_dma(host, false);
-		if (ret >= 0)
-			ret = -EIO;
-		host->chan_tx = NULL;
-		dma_release_channel(chan);
-		/* Free the Rx channel too */
-		chan = host->chan_rx;
-		if (chan) {
-			host->chan_rx = NULL;
-			dma_release_channel(chan);
-		}
-		dev_warn(&host->pdev->dev,
-			 "DMA failed: %d, falling back to PIO\n", ret);
+		host->dma_on = false;
+		dev_dbg(&host->pdev->dev, "DMA failed: %d\n", ret);
 	}
 }
 
-- 
2.7.4

[PATCH RFC 3/4] mmc: host: renesas_sdhi_sys_dmac: add DMACR setting

[Yoshihiro Shimoda]

This patch adds DMACR setting which needs to use the 32 bytes
transfer mode of SYS-DMAC.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/mmc/host/renesas_sdhi_core.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index 234551a..d9a69f6 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -20,6 +20,7 @@
 
 #include <linux/kernel.h>
 #include <linux/clk.h>
+#include <linux/dmaengine.h>
 #include <linux/slab.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
@@ -46,6 +47,12 @@
 #define SDHI_VER_GEN3_SD	0xcc10
 #define SDHI_VER_GEN3_SDMMC	0xcd10
 
+#define DMACR_SDR104		0x192
+#define DMACR_SDR104_32BYTE	0x0004
+#define DMACR_SDR50		0xe4
+#define DMACR_SDR50_32BYTE	0x000a
+#define DMACR_2_OR_4BYTE	0x0000
+
 struct renesas_sdhi_quirks {
 	bool hs400_disabled;
 	bool hs400_4taps;
@@ -604,6 +611,32 @@ static int renesas_sdhi_multi_io_quirk(struct mmc_card *card,
 	return blk_size;
 }
 
+static void renesas_sdhi_set_dmacr(struct tmio_mmc_host *host)
+{
+	struct renesas_sdhi *priv = host_to_priv(host);
+	u16 val = DMACR_2_OR_4BYTE;
+	u16 reg;
+	enum dma_slave_buswidth width = priv->dma_priv.dma_buswidth;
+
+	switch (sd_ctrl_read16(host, CTL_VERSION)) {
+	case SDHI_VER_GEN2_SDR50:
+		if (width == DMA_SLAVE_BUSWIDTH_32_BYTES)
+			val = DMACR_SDR50_32BYTE;
+		reg = DMACR_SDR50;
+		break;
+	case SDHI_VER_GEN2_SDR104:
+		if (width == DMA_SLAVE_BUSWIDTH_32_BYTES)
+			val = DMACR_SDR104_32BYTE;
+		reg = DMACR_SDR104;
+		break;
+	default:
+		/* nothing to do */
+		return;
+	}
+
+	sd_ctrl_write16(host, reg, val);
+}
+
 static void renesas_sdhi_enable_dma(struct tmio_mmc_host *host, bool enable)
 {
 	/* Iff regs are 8 byte apart, sdbuf is 64 bit. Otherwise always 32. */
@@ -611,6 +644,8 @@ static void renesas_sdhi_enable_dma(struct tmio_mmc_host *host, bool enable)
 
 	sd_ctrl_write16(host, CTL_DMA_ENABLE, enable ? DMA_ENABLE_DMASDRW : 0);
 	renesas_sdhi_sdbuf_width(host, enable ? width : 16);
+
+	renesas_sdhi_set_dmacr(host);
 }
 
 static const struct renesas_sdhi_quirks sdhi_quirks_4tap_nohs400 = {
-- 
2.7.4

[PATCH RFC 4/4] mmc: host: renesas_sdhi_sys_dmac: Set dma_buswidth value to 32 byte

[Yoshihiro Shimoda]

To improve performance, this patch sets dma_buswidth value to 32
when transfer size is multiples of 32. In other words, a sd card
transfer's size if not multiples of 32, this driver uses PIO
and then the performance will be down.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/mmc/host/renesas_sdhi_sys_dmac.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/host/renesas_sdhi_sys_dmac.c b/drivers/mmc/host/renesas_sdhi_sys_dmac.c
index 09137cc..65e71b6 100644
--- a/drivers/mmc/host/renesas_sdhi_sys_dmac.c
+++ b/drivers/mmc/host/renesas_sdhi_sys_dmac.c
@@ -58,7 +58,7 @@ static const struct renesas_sdhi_of_data of_rcar_gen2_compatible = {
 	.capabilities	= MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ |
 			  MMC_CAP_CMD23,
 	.capabilities2	= MMC_CAP2_NO_WRITE_PROTECT,
-	.dma_buswidth	= DMA_SLAVE_BUSWIDTH_4_BYTES,
+	.dma_buswidth	= DMA_SLAVE_BUSWIDTH_32_BYTES,
 	.dma_rx_offset	= 0x2000,
 	.scc_offset	= 0x0300,
 	.taps		= rcar_gen2_scc_taps,
-- 
2.7.4

Re: [PATCH RFC 0/4] mmc: host: renesas_sdhi_sys_dmac: change dma_buswidth

[Ulrich Hecht]

> On November 22, 2019 at 7:13 AM Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote:
> 
> 
> This patch can improve performance when a sd card transfer size
> is multiples of 32 like a sd memory card. However, this may
> disimprove performance when a sd card transfer size is not
> multiples of 32 because this patch uses PIO instead of SYS-DMAC.

I have logged the DMA transfer sizes of a Marvell SD8897 WiFi/BT chip, and there transfers that are not multiples of 32 are infrequent (every second or so) and small (184 bytes); all large transfers are multiples of 32.

Would it be practical to switch the DMA bus width down if odd sizes are encountered, and back up again otherwise?
 
CU
Uli

Re: [PATCH RFC 1/4] mmc: host: renesas_sdhi_sys_dmac: Use dma_buswidth instead of hardcoded value

[Ulrich Hecht]

> On November 22, 2019 at 7:13 AM Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote:
> 
> 
> This patch uses dma_buswidth instread of hardcoded value of
> TMIO_MMC_MIN_DMA_LEN (8) for increasing the buswidth in the future.
> Note that, since the dma_buswidth is 4 and the align is 2,
> when the sg_tmp->length is 6, it cannot transfer correcly.
> So, this patch changes the conditions.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/mmc/host/renesas_sdhi_sys_dmac.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mmc/host/renesas_sdhi_sys_dmac.c b/drivers/mmc/host/renesas_sdhi_sys_dmac.c
> index 13ff023..30f34a3 100644
> --- a/drivers/mmc/host/renesas_sdhi_sys_dmac.c
> +++ b/drivers/mmc/host/renesas_sdhi_sys_dmac.c
> @@ -23,8 +23,6 @@
>  #include "renesas_sdhi.h"
>  #include "tmio_mmc.h"
>  
> -#define TMIO_MMC_MIN_DMA_LEN 8
> -
>  static const struct renesas_sdhi_of_data of_default_cfg = {
>  	.tmio_flags = TMIO_MMC_HAS_IDLE_WAIT,
>  };
> @@ -159,11 +157,12 @@ static void renesas_sdhi_sys_dmac_start_dma_rx(struct tmio_mmc_host *host)
>  	int ret, i;
>  	bool aligned = true, multiple = true;
>  	unsigned int align = (1 << host->pdata->alignment_shift) - 1;
> +	unsigned int min_len = priv->dma_priv.dma_buswidth ? : align + 1;
>  
>  	for_each_sg(sg, sg_tmp, host->sg_len, i) {
>  		if (sg_tmp->offset & align)
>  			aligned = false;
> -		if (sg_tmp->length & align) {
> +		if (sg_tmp->length % min_len) {
>  			multiple = false;
>  			break;
>  		}
> @@ -175,7 +174,7 @@ static void renesas_sdhi_sys_dmac_start_dma_rx(struct tmio_mmc_host *host)
>  		goto pio;
>  	}
>  
> -	if (sg->length < TMIO_MMC_MIN_DMA_LEN)
> +	if (sg->length < min_len)
>  		return;
>  
>  	/* The only sg element can be unaligned, use our bounce buffer then */
> @@ -231,11 +230,12 @@ static void renesas_sdhi_sys_dmac_start_dma_tx(struct tmio_mmc_host *host)
>  	int ret, i;
>  	bool aligned = true, multiple = true;
>  	unsigned int align = (1 << host->pdata->alignment_shift) - 1;
> +	unsigned int min_len = priv->dma_priv.dma_buswidth ? : align + 1;
>  
>  	for_each_sg(sg, sg_tmp, host->sg_len, i) {
>  		if (sg_tmp->offset & align)
>  			aligned = false;
> -		if (sg_tmp->length & align) {
> +		if (sg_tmp->length % min_len) {
>  			multiple = false;
>  			break;
>  		}
> @@ -247,7 +247,7 @@ static void renesas_sdhi_sys_dmac_start_dma_tx(struct tmio_mmc_host *host)
>  		goto pio;
>  	}
>  
> -	if (sg->length < TMIO_MMC_MIN_DMA_LEN)
> +	if (sg->length < min_len)
>  		return;
>  
>  	/* The only sg element can be unaligned, use our bounce buffer then */
> -- 
> 2.7.4

Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>

CU
Uli

Re: [PATCH RFC 2/4] mmc: host: renesas_sdhi_sys_dmac: Do not fall back to PIO

[Ulrich Hecht]

> On November 22, 2019 at 7:13 AM Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote:
> 
> 
> Falling back to PIO forever is not convenience if a buffer condition
> is not match with the hardware once. So, this patch removes it.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/mmc/host/renesas_sdhi_sys_dmac.c | 35 +++++++-------------------------
>  1 file changed, 7 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/mmc/host/renesas_sdhi_sys_dmac.c b/drivers/mmc/host/renesas_sdhi_sys_dmac.c
> index 30f34a3..09137cc 100644
> --- a/drivers/mmc/host/renesas_sdhi_sys_dmac.c
> +++ b/drivers/mmc/host/renesas_sdhi_sys_dmac.c
> @@ -175,7 +175,7 @@ static void renesas_sdhi_sys_dmac_start_dma_rx(struct tmio_mmc_host *host)
>  	}
>  
>  	if (sg->length < min_len)
> -		return;
> +		goto pio;
>  
>  	/* The only sg element can be unaligned, use our bounce buffer then */
>  	if (!aligned) {
> @@ -200,23 +200,13 @@ static void renesas_sdhi_sys_dmac_start_dma_rx(struct tmio_mmc_host *host)
>  			ret = cookie;
>  		}
>  		host->dma_on = true;
> +		renesas_sdhi_sys_dmac_enable_dma(host, true);
>  	}
>  pio:
>  	if (!desc) {
> -		/* DMA failed, fall back to PIO */
>  		renesas_sdhi_sys_dmac_enable_dma(host, false);
> -		if (ret >= 0)
> -			ret = -EIO;
> -		host->chan_rx = NULL;
> -		dma_release_channel(chan);
> -		/* Free the Tx channel too */
> -		chan = host->chan_tx;
> -		if (chan) {
> -			host->chan_tx = NULL;
> -			dma_release_channel(chan);
> -		}
> -		dev_warn(&host->pdev->dev,
> -			 "DMA failed: %d, falling back to PIO\n", ret);
> +		host->dma_on = false;
> +		dev_dbg(&host->pdev->dev, "DMA failed: %d\n", ret);
>  	}
>  }
>  
> @@ -248,7 +238,7 @@ static void renesas_sdhi_sys_dmac_start_dma_tx(struct tmio_mmc_host *host)
>  	}
>  
>  	if (sg->length < min_len)
> -		return;
> +		goto pio;
>  
>  	/* The only sg element can be unaligned, use our bounce buffer then */
>  	if (!aligned) {
> @@ -281,20 +271,9 @@ static void renesas_sdhi_sys_dmac_start_dma_tx(struct tmio_mmc_host *host)
>  	}
>  pio:
>  	if (!desc) {
> -		/* DMA failed, fall back to PIO */
>  		renesas_sdhi_sys_dmac_enable_dma(host, false);
> -		if (ret >= 0)
> -			ret = -EIO;
> -		host->chan_tx = NULL;
> -		dma_release_channel(chan);
> -		/* Free the Rx channel too */
> -		chan = host->chan_rx;
> -		if (chan) {
> -			host->chan_rx = NULL;
> -			dma_release_channel(chan);
> -		}
> -		dev_warn(&host->pdev->dev,
> -			 "DMA failed: %d, falling back to PIO\n", ret);
> +		host->dma_on = false;
> +		dev_dbg(&host->pdev->dev, "DMA failed: %d\n", ret);
>  	}
>  }
>  
> -- 
> 2.7.4

Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>

CU
Uli

Re: [PATCH RFC 3/4] mmc: host: renesas_sdhi_sys_dmac: add DMACR setting

[Ulrich Hecht]

> On November 22, 2019 at 7:13 AM Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote:
> 
> 
> This patch adds DMACR setting which needs to use the 32 bytes
> transfer mode of SYS-DMAC.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/mmc/host/renesas_sdhi_core.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
> index 234551a..d9a69f6 100644
> --- a/drivers/mmc/host/renesas_sdhi_core.c
> +++ b/drivers/mmc/host/renesas_sdhi_core.c
> @@ -20,6 +20,7 @@
>  
>  #include <linux/kernel.h>
>  #include <linux/clk.h>
> +#include <linux/dmaengine.h>
>  #include <linux/slab.h>
>  #include <linux/module.h>
>  #include <linux/of_device.h>
> @@ -46,6 +47,12 @@
>  #define SDHI_VER_GEN3_SD	0xcc10
>  #define SDHI_VER_GEN3_SDMMC	0xcd10
>  
> +#define DMACR_SDR104		0x192
> +#define DMACR_SDR104_32BYTE	0x0004
> +#define DMACR_SDR50		0xe4
> +#define DMACR_SDR50_32BYTE	0x000a
> +#define DMACR_2_OR_4BYTE	0x0000
> +

Could you give me a pointer to where these magic numbers are documented? In my (rather old) SDHI docs the register addresses don't match.

>  struct renesas_sdhi_quirks {
>  	bool hs400_disabled;
>  	bool hs400_4taps;
> @@ -604,6 +611,32 @@ static int renesas_sdhi_multi_io_quirk(struct mmc_card *card,
>  	return blk_size;
>  }
>  
> +static void renesas_sdhi_set_dmacr(struct tmio_mmc_host *host)
> +{
> +	struct renesas_sdhi *priv = host_to_priv(host);
> +	u16 val = DMACR_2_OR_4BYTE;
> +	u16 reg;
> +	enum dma_slave_buswidth width = priv->dma_priv.dma_buswidth;
> +
> +	switch (sd_ctrl_read16(host, CTL_VERSION)) {
> +	case SDHI_VER_GEN2_SDR50:
> +		if (width == DMA_SLAVE_BUSWIDTH_32_BYTES)
> +			val = DMACR_SDR50_32BYTE;
> +		reg = DMACR_SDR50;
> +		break;
> +	case SDHI_VER_GEN2_SDR104:
> +		if (width == DMA_SLAVE_BUSWIDTH_32_BYTES)
> +			val = DMACR_SDR104_32BYTE;
> +		reg = DMACR_SDR104;
> +		break;
> +	default:
> +		/* nothing to do */
> +		return;
> +	}
> +
> +	sd_ctrl_write16(host, reg, val);
> +}
> +
>  static void renesas_sdhi_enable_dma(struct tmio_mmc_host *host, bool enable)
>  {
>  	/* Iff regs are 8 byte apart, sdbuf is 64 bit. Otherwise always 32. */
> @@ -611,6 +644,8 @@ static void renesas_sdhi_enable_dma(struct tmio_mmc_host *host, bool enable)
>  
>  	sd_ctrl_write16(host, CTL_DMA_ENABLE, enable ? DMA_ENABLE_DMASDRW : 0);
>  	renesas_sdhi_sdbuf_width(host, enable ? width : 16);
> +
> +	renesas_sdhi_set_dmacr(host);
>  }
>  
>  static const struct renesas_sdhi_quirks sdhi_quirks_4tap_nohs400 = {
> -- 
> 2.7.4
>

Assuming that the register addresses are correct,
Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>

CU
Uli

Re: [PATCH RFC 4/4] mmc: host: renesas_sdhi_sys_dmac: Set dma_buswidth value to 32 byte

[Ulrich Hecht]

> On November 22, 2019 at 7:13 AM Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote:
> 
> 
> To improve performance, this patch sets dma_buswidth value to 32
> when transfer size is multiples of 32. In other words, a sd card
> transfer's size if not multiples of 32, this driver uses PIO
> and then the performance will be down.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/mmc/host/renesas_sdhi_sys_dmac.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/renesas_sdhi_sys_dmac.c b/drivers/mmc/host/renesas_sdhi_sys_dmac.c
> index 09137cc..65e71b6 100644
> --- a/drivers/mmc/host/renesas_sdhi_sys_dmac.c
> +++ b/drivers/mmc/host/renesas_sdhi_sys_dmac.c
> @@ -58,7 +58,7 @@ static const struct renesas_sdhi_of_data of_rcar_gen2_compatible = {
>  	.capabilities	= MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ |
>  			  MMC_CAP_CMD23,
>  	.capabilities2	= MMC_CAP2_NO_WRITE_PROTECT,
> -	.dma_buswidth	= DMA_SLAVE_BUSWIDTH_4_BYTES,
> +	.dma_buswidth	= DMA_SLAVE_BUSWIDTH_32_BYTES,
>  	.dma_rx_offset	= 0x2000,
>  	.scc_offset	= 0x0300,
>  	.taps		= rcar_gen2_scc_taps,
> -- 
> 2.7.4
>

Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>

CU
Uli

Re: [PATCH RFC 4/4] mmc: host: renesas_sdhi_sys_dmac: Set dma_buswidth value to 32 byte

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 684 bytes --]

Hi Shimoda-san,

Interesting series!

> -	.dma_buswidth	= DMA_SLAVE_BUSWIDTH_4_BYTES,
> +	.dma_buswidth	= DMA_SLAVE_BUSWIDTH_32_BYTES,

Two very high level questions:

1) can't we set dma_priv->dma_buswidth at runtime when we know what the
card is capable of? Either DMA_SLAVE_BUSWIDTH_32_BYTES or
DMA_SLAVE_BUSWIDTH_4_BYTES? Then we don't need to fallback to PIO.
AFAIS, we only Gen2 sets .dma_buswidth in of_data, so we could even
remove it from of_data entirely?

2) Just by grepping in mmc/hosts, I see that no driver uses
DMA_SLAVE_BUSWIDTH_32_BYTES. Do you have an idea why? Because it is the
convenient setting which works for all cards?

Thanks and kind regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH RFC 0/4] mmc: host: renesas_sdhi_sys_dmac: change dma_buswidth

[Yoshihiro Shimoda]

Hi Ulrich-san,

Thank you for your comment!

> From: Ulrich Hecht, Sent: Tuesday, November 26, 2019 7:45 PM
> 
> > On November 22, 2019 at 7:13 AM Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote:
> >
> >
> > This patch can improve performance when a sd card transfer size
> > is multiples of 32 like a sd memory card. However, this may
> > disimprove performance when a sd card transfer size is not
> > multiples of 32 because this patch uses PIO instead of SYS-DMAC.
> 
> I have logged the DMA transfer sizes of a Marvell SD8897 WiFi/BT chip, and there transfers that are not multiples of 32
> are infrequent (every second or so) and small (184 bytes); all large transfers are multiples of 32.
> 
> Would it be practical to switch the DMA bus width down if odd sizes are encountered, and back up again otherwise?

I don't know why, but according to drivers/dma/sh/rcar-dmac.c, we should not configure the channel while using it.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/dma/sh/rcar-dmac.c?h=v5.4#n1245

The comment has been described on the first commit. But, since other drivers don't mention such a limitation,
I think we can remove the comments somehow.

Best regards,
Yoshihiro Shimoda

> CU
> Uli

RE: [PATCH RFC 3/4] mmc: host: renesas_sdhi_sys_dmac: add DMACR setting

[Yoshihiro Shimoda]

Hi Ulrich-san,

Thank you for your review!

> From: Ulrich Hecht, Sent: Tuesday, November 26, 2019 7:46 PM
<snip>
> > +#define DMACR_SDR104		0x192
> > +#define DMACR_SDR104_32BYTE	0x0004
> > +#define DMACR_SDR50		0xe4
> > +#define DMACR_SDR50_32BYTE	0x000a
> > +#define DMACR_2_OR_4BYTE	0x0000
> > +
> 
> Could you give me a pointer to where these magic numbers are documented? In my (rather old) SDHI docs the register addresses
> don't match.

R-Car Gen2 has 2 types of SDHI controller from renesas_sdhi_core.c:
---
#define SDHI_VER_GEN2_SDR50	0x490c
<snip>
#define SDHI_VER_GEN2_SDR104	0xcb0d
---

- SDR50/104 supported controller's bus_shift will be 1, so that DMACR_SDR104 is 0x192 (actual address is 0x324).
- SDR50 supported controller's bus_shift will be 0, so that DMACR_SDR50 is 0xe6 (actual address is 0xe6).

So, I should fix the "#define DMACR_SDR50" value to 0xe6, not 0xe4 :)

Best regards,
Yoshihiro Shimoda

> >  struct renesas_sdhi_quirks {
> >  	bool hs400_disabled;
> >  	bool hs400_4taps;
> > @@ -604,6 +611,32 @@ static int renesas_sdhi_multi_io_quirk(struct mmc_card *card,
> >  	return blk_size;
> >  }
> >
> > +static void renesas_sdhi_set_dmacr(struct tmio_mmc_host *host)
> > +{
> > +	struct renesas_sdhi *priv = host_to_priv(host);
> > +	u16 val = DMACR_2_OR_4BYTE;
> > +	u16 reg;
> > +	enum dma_slave_buswidth width = priv->dma_priv.dma_buswidth;
> > +
> > +	switch (sd_ctrl_read16(host, CTL_VERSION)) {
> > +	case SDHI_VER_GEN2_SDR50:
> > +		if (width == DMA_SLAVE_BUSWIDTH_32_BYTES)
> > +			val = DMACR_SDR50_32BYTE;
> > +		reg = DMACR_SDR50;
> > +		break;
> > +	case SDHI_VER_GEN2_SDR104:
> > +		if (width == DMA_SLAVE_BUSWIDTH_32_BYTES)
> > +			val = DMACR_SDR104_32BYTE;
> > +		reg = DMACR_SDR104;
> > +		break;
> > +	default:
> > +		/* nothing to do */
> > +		return;
> > +	}
> > +
> > +	sd_ctrl_write16(host, reg, val);
> > +}
> > +
> >  static void renesas_sdhi_enable_dma(struct tmio_mmc_host *host, bool enable)
> >  {
> >  	/* Iff regs are 8 byte apart, sdbuf is 64 bit. Otherwise always 32. */
> > @@ -611,6 +644,8 @@ static void renesas_sdhi_enable_dma(struct tmio_mmc_host *host, bool enable)
> >
> >  	sd_ctrl_write16(host, CTL_DMA_ENABLE, enable ? DMA_ENABLE_DMASDRW : 0);
> >  	renesas_sdhi_sdbuf_width(host, enable ? width : 16);
> > +
> > +	renesas_sdhi_set_dmacr(host);
> >  }
> >
> >  static const struct renesas_sdhi_quirks sdhi_quirks_4tap_nohs400 = {
> > --
> > 2.7.4
> >
> 
> Assuming that the register addresses are correct,
> Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>
> 
> CU
> Uli

RE: [PATCH RFC 4/4] mmc: host: renesas_sdhi_sys_dmac: Set dma_buswidth value to 32 byte

[Yoshihiro Shimoda]

Hi Wolfram-san,

Thank you for your comments!

> From: Wolfram Sang, Sent: Friday, November 29, 2019 6:07 AM
> 
> Hi Shimoda-san,
> 
> Interesting series!
> 
> > -	.dma_buswidth	= DMA_SLAVE_BUSWIDTH_4_BYTES,
> > +	.dma_buswidth	= DMA_SLAVE_BUSWIDTH_32_BYTES,
> 
> Two very high level questions:
> 
> 1) can't we set dma_priv->dma_buswidth at runtime when we know what the
> card is capable of? Either DMA_SLAVE_BUSWIDTH_32_BYTES or
> DMA_SLAVE_BUSWIDTH_4_BYTES? Then we don't need to fallback to PIO.
> AFAIS, we only Gen2 sets .dma_buswidth in of_data, so we could even
> remove it from of_data entirely?

As I replied to Ulrich-san on other email thread, for now, rcar-dmac has a limitation
on dmaengine_slave_config(), we should not call it at runtime. But, I don't think
any sd card have such a limitation. In other words, if rcar-dmac doesn't have
the limitation, I think we can change the buswidth at runtime and then we can
remove the .dma_buswidth from of_data.

> 2) Just by grepping in mmc/hosts, I see that no driver uses
> DMA_SLAVE_BUSWIDTH_32_BYTES. Do you have an idea why? Because it is the
> convenient setting which works for all cards?

I also grepped in drivers/dma, and all dmaengine drivers except Renesas related
SoCs don't support DMA_SLAVE_BUSWIDTH_32_BYTES. So, I think no driver uses
the 32 bytes on mmc/hosts :)

Best regards,
Yoshihiro Shimoda

> Thanks and kind regards,
> 
>    Wolfram

Re: [PATCH RFC 4/4] mmc: host: renesas_sdhi_sys_dmac: Set dma_buswidth value to 32 byte

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1137 bytes --]

Hi Shimoda-san,

> > 1) can't we set dma_priv->dma_buswidth at runtime when we know what the
> > card is capable of? Either DMA_SLAVE_BUSWIDTH_32_BYTES or
> > DMA_SLAVE_BUSWIDTH_4_BYTES? Then we don't need to fallback to PIO.
> > AFAIS, we only Gen2 sets .dma_buswidth in of_data, so we could even
> > remove it from of_data entirely?
> 
> As I replied to Ulrich-san on other email thread, for now, rcar-dmac has a limitation
> on dmaengine_slave_config(), we should not call it at runtime. But, I don't think
> any sd card have such a limitation. In other words, if rcar-dmac doesn't have
> the limitation, I think we can change the buswidth at runtime and then we can
> remove the .dma_buswidth from of_data.

So, that I understand correctly: The DMAC limitation is because of the
driver and not because of the HW? If so, is it hard/planned to be fixed?

> I also grepped in drivers/dma, and all dmaengine drivers except Renesas related
> SoCs don't support DMA_SLAVE_BUSWIDTH_32_BYTES. So, I think no driver uses
> the 32 bytes on mmc/hosts :)

Wow, we are bleeding edge with this? :)

Thanks,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]