[PATCH 0/5] mmc: renesas_sdhi_internal_dmac: DMA handling fixes

[PATCH 0/5] mmc: renesas_sdhi_internal_dmac: DMA handling fixes

[Wolfram Sang]

Ulf,

I have collected the patches floating around for renesas_sdhi_internal_dmac
(also previously internal ones) and grouped them to avoid dependency issues. I
think it would be good to give Shimoda-san some time to look at the new patches
(2,3,5), too.

Regarding stable: I think patch 1 is clearly for stable. Patch 3 maybe, but it
needs patch 2 which is not. Not sure if it is worth the hazzle. Maybe
Shimoda-san has an opinion, too? Patches 4 & 5 are not for stable.

Changes since last time:
	* patch 1: added tags, don't initialize static var to 0
	* patches 2,3,5: new
	* patch 4: added tags

Kind regards,

   Wolfram


Masaharu Hayakawa (1):
  mmc: renesas_sdhi: Fix alignment check of sg buffer

Niklas Söderlund (1):
  mmc: renesas_sdhi: use helpers to access struct scatterlist members

Wolfram Sang (3):
  mmc: renesas_sdhi_internal_dmac: limit DMA RX for old SoCs
  mmc: renesas_sdhi_internal_dmac: use more generic whitelisting
  mmc: renesas_sdhi_internal_dmac: remove superfluous WARN

 drivers/mmc/host/renesas_sdhi_internal_dmac.c | 64 +++++++++++++++++++--------
 1 file changed, 46 insertions(+), 18 deletions(-)

-- 
2.11.0

[PATCH 1/5] mmc: renesas_sdhi_internal_dmac: limit DMA RX for old SoCs

[Wolfram Sang]

Early revisions of certain SoCs cannot do multiple DMA RX streams in
parallel. To avoid data corruption, only allow one DMA RX channel and
fall back to PIO, if needed.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Tested-by: Nguyen Viet Dung <dung.nguyen.aj@renesas.com>
---
 drivers/mmc/host/renesas_sdhi_internal_dmac.c | 35 ++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
index 8e0acd197c43..9c50d64cd10c 100644
--- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
+++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
@@ -9,6 +9,7 @@
  * published by the Free Software Foundation.
  */
 
+#include <linux/bitops.h>
 #include <linux/device.h>
 #include <linux/dma-mapping.h>
 #include <linux/io-64-nonatomic-hi-lo.h>
@@ -62,6 +63,17 @@
  *   need a custom accessor.
  */
 
+static unsigned long global_flags;
+/*
+ * Workaround for avoiding to use RX DMAC by multiple channels.
+ * On R-Car H3 ES1.* and M3-W ES1.0, when multiple SDHI channels use
+ * RX DMAC simultaneously, sometimes hundreds of bytes data are not
+ * stored into the system memory even if the DMAC interrupt happened.
+ * So, this driver then uses one RX DMAC channel only.
+ */
+#define SDHI_INTERNAL_DMAC_ONE_RX_ONLY	0
+#define SDHI_INTERNAL_DMAC_RX_IN_USE	1
+
 /* Definitions for sampling clocks */
 static struct renesas_sdhi_scc rcar_gen3_scc_taps[] = {
 	{
@@ -126,6 +138,10 @@ renesas_sdhi_internal_dmac_abort_dma(struct tmio_mmc_host *host) {
 	renesas_sdhi_internal_dmac_dm_write(host, DM_CM_RST,
 					    RST_RESERVED_BITS | val);
 
+
+	if (host->data && host->data->flags & MMC_DATA_READ)
+		clear_bit(SDHI_INTERNAL_DMAC_RX_IN_USE, &global_flags);
+
 	renesas_sdhi_internal_dmac_enable_dma(host, true);
 }
 
@@ -155,6 +171,9 @@ renesas_sdhi_internal_dmac_start_dma(struct tmio_mmc_host *host,
 	if (data->flags & MMC_DATA_READ) {
 		dtran_mode |= DTRAN_MODE_CH_NUM_CH1;
 		dir = DMA_FROM_DEVICE;
+		if (test_bit(SDHI_INTERNAL_DMAC_ONE_RX_ONLY, &global_flags) &&
+		    test_and_set_bit(SDHI_INTERNAL_DMAC_RX_IN_USE, &global_flags))
+			goto force_pio;
 	} else {
 		dtran_mode |= DTRAN_MODE_CH_NUM_CH0;
 		dir = DMA_TO_DEVICE;
@@ -208,6 +227,9 @@ static void renesas_sdhi_internal_dmac_complete_tasklet_fn(unsigned long arg)
 	renesas_sdhi_internal_dmac_enable_dma(host, false);
 	dma_unmap_sg(&host->pdev->dev, host->sg_ptr, host->sg_len, dir);
 
+	if (dir == DMA_FROM_DEVICE)
+		clear_bit(SDHI_INTERNAL_DMAC_RX_IN_USE, &global_flags);
+
 	tmio_mmc_do_data_irq(host);
 out:
 	spin_unlock_irq(&host->lock);
@@ -251,18 +273,25 @@ static const struct tmio_mmc_dma_ops renesas_sdhi_internal_dmac_dma_ops = {
  * implementation as others may use a different implementation.
  */
 static const struct soc_device_attribute gen3_soc_whitelist[] = {
-        { .soc_id = "r8a7795", .revision = "ES1.*" },
+        { .soc_id = "r8a7795", .revision = "ES1.*",
+	  .data = (void *)BIT(SDHI_INTERNAL_DMAC_ONE_RX_ONLY) },
         { .soc_id = "r8a7795", .revision = "ES2.0" },
-        { .soc_id = "r8a7796", .revision = "ES1.0" },
+        { .soc_id = "r8a7796", .revision = "ES1.0",
+	  .data = (void *)BIT(SDHI_INTERNAL_DMAC_ONE_RX_ONLY) },
         { .soc_id = "r8a77995", .revision = "ES1.0" },
         { /* sentinel */ }
 };
 
 static int renesas_sdhi_internal_dmac_probe(struct platform_device *pdev)
 {
-	if (!soc_device_match(gen3_soc_whitelist))
+	const struct soc_device_attribute *soc = soc_device_match(gen3_soc_whitelist);
+
+	if (!soc)
 		return -ENODEV;
 
+	if (soc->data)
+		global_flags |= (unsigned long)soc->data;
+
 	return renesas_sdhi_probe(pdev, &renesas_sdhi_internal_dmac_dma_ops);
 }
 
-- 
2.11.0

[PATCH 2/5] mmc: renesas_sdhi: use helpers to access struct scatterlist members

[Wolfram Sang]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Instead of directly accessing the members of struct scatterlist use the
helpers mmc_get_dma_dir() and sg_dma_address() in
renesas_sdhi_internal_dmac_start_dma(). Based on previous work by
Masaharu Hayakawa.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
[rebased to mmc/next]
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/host/renesas_sdhi_internal_dmac.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
index 9c50d64cd10c..6c2f3ae01de9 100644
--- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
+++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
@@ -158,7 +158,6 @@ renesas_sdhi_internal_dmac_start_dma(struct tmio_mmc_host *host,
 {
 	struct scatterlist *sg = host->sg_ptr;
 	u32 dtran_mode = DTRAN_MODE_BUS_WID_TH | DTRAN_MODE_ADDR_MODE;
-	enum dma_data_direction dir;
 	int ret;
 
 	/* This DMAC cannot handle if sg_len is not 1 */
@@ -170,16 +169,15 @@ renesas_sdhi_internal_dmac_start_dma(struct tmio_mmc_host *host,
 
 	if (data->flags & MMC_DATA_READ) {
 		dtran_mode |= DTRAN_MODE_CH_NUM_CH1;
-		dir = DMA_FROM_DEVICE;
 		if (test_bit(SDHI_INTERNAL_DMAC_ONE_RX_ONLY, &global_flags) &&
 		    test_and_set_bit(SDHI_INTERNAL_DMAC_RX_IN_USE, &global_flags))
 			goto force_pio;
 	} else {
 		dtran_mode |= DTRAN_MODE_CH_NUM_CH0;
-		dir = DMA_TO_DEVICE;
 	}
 
-	ret = dma_map_sg(&host->pdev->dev, sg, host->sg_len, dir);
+	ret = dma_map_sg(&host->pdev->dev, sg, host->sg_len,
+			 mmc_get_dma_dir(data));
 	if (ret == 0)
 		goto force_pio;
 
@@ -189,7 +187,7 @@ renesas_sdhi_internal_dmac_start_dma(struct tmio_mmc_host *host,
 	renesas_sdhi_internal_dmac_dm_write(host, DM_CM_DTRAN_MODE,
 					    dtran_mode);
 	renesas_sdhi_internal_dmac_dm_write(host, DM_DTRAN_ADDR,
-					    sg->dma_address);
+					    sg_dma_address(sg));
 
 	return;
 
-- 
2.11.0

[PATCH 3/5] mmc: renesas_sdhi: Fix alignment check of sg buffer

[Wolfram Sang]

From: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>

Sometimes sg->offset is not used for buffer addresses allocated by
dma_map_sg(), so alignment checks should be done on the allocated buffer
addresses. Delete the alignment check for sg->offset that is done before
dma_map_sg(). Instead, it performs the alignment check for
sg->dma_address after dma_map_sg().

Signed-off-by: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
[Niklas: broke this commit in two and tidied small style issue]
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
[rebased to mmc/next]
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/host/renesas_sdhi_internal_dmac.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
index 6c2f3ae01de9..32b8be4c0b81 100644
--- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
+++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
@@ -158,14 +158,20 @@ renesas_sdhi_internal_dmac_start_dma(struct tmio_mmc_host *host,
 {
 	struct scatterlist *sg = host->sg_ptr;
 	u32 dtran_mode = DTRAN_MODE_BUS_WID_TH | DTRAN_MODE_ADDR_MODE;
-	int ret;
 
 	/* This DMAC cannot handle if sg_len is not 1 */
 	WARN_ON(host->sg_len > 1);
 
+	if (!dma_map_sg(&host->pdev->dev, sg, host->sg_len,
+			mmc_get_dma_dir(data)))
+		goto force_pio;
+
 	/* This DMAC cannot handle if buffer is not 8-bytes alignment */
-	if (!IS_ALIGNED(sg->offset, 8))
+	if (!IS_ALIGNED(sg_dma_address(sg), 8)) {
+		dma_unmap_sg(&host->pdev->dev, sg, host->sg_len,
+			     mmc_get_dma_dir(data));
 		goto force_pio;
+	}
 
 	if (data->flags & MMC_DATA_READ) {
 		dtran_mode |= DTRAN_MODE_CH_NUM_CH1;
@@ -176,11 +182,6 @@ renesas_sdhi_internal_dmac_start_dma(struct tmio_mmc_host *host,
 		dtran_mode |= DTRAN_MODE_CH_NUM_CH0;
 	}
 
-	ret = dma_map_sg(&host->pdev->dev, sg, host->sg_len,
-			 mmc_get_dma_dir(data));
-	if (ret == 0)
-		goto force_pio;
-
 	renesas_sdhi_internal_dmac_enable_dma(host, true);
 
 	/* set dma parameters */
-- 
2.11.0

[PATCH 4/5] mmc: renesas_sdhi_internal_dmac: use more generic whitelisting

[Wolfram Sang]

Whitelisting every ES version does not scale. So, we whitelist whole
SoCs independent of ES version. If we need specific handling for an ES
version, we put it to the front, so it will be matched first.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
Tested-by: Nguyen Viet Dung <dung.nguyen.aj@renesas.com>
---
 drivers/mmc/host/renesas_sdhi_internal_dmac.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
index 32b8be4c0b81..06b4e0004ca4 100644
--- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
+++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
@@ -272,12 +272,15 @@ static const struct tmio_mmc_dma_ops renesas_sdhi_internal_dmac_dma_ops = {
  * implementation as others may use a different implementation.
  */
 static const struct soc_device_attribute gen3_soc_whitelist[] = {
+	/* specific ones */
         { .soc_id = "r8a7795", .revision = "ES1.*",
 	  .data = (void *)BIT(SDHI_INTERNAL_DMAC_ONE_RX_ONLY) },
-        { .soc_id = "r8a7795", .revision = "ES2.0" },
         { .soc_id = "r8a7796", .revision = "ES1.0",
 	  .data = (void *)BIT(SDHI_INTERNAL_DMAC_ONE_RX_ONLY) },
-        { .soc_id = "r8a77995", .revision = "ES1.0" },
+	/* generic ones */
+        { .soc_id = "r8a7795" },
+        { .soc_id = "r8a7796" },
+        { .soc_id = "r8a77995" },
         { /* sentinel */ }
 };
 
-- 
2.11.0

[PATCH 5/5] mmc: renesas_sdhi_internal_dmac: remove superfluous WARN

[Wolfram Sang]

The WARN can never trigger because we limited the max_seg number in
renesas_sdhi_of_data already. Remove it and update the comment.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/host/renesas_sdhi_internal_dmac.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
index 06b4e0004ca4..9be1780d9970 100644
--- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
+++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
@@ -91,7 +91,7 @@ static const struct renesas_sdhi_of_data of_rcar_gen3_compatible = {
 	.scc_offset	= 0x1000,
 	.taps		= rcar_gen3_scc_taps,
 	.taps_num	= ARRAY_SIZE(rcar_gen3_scc_taps),
-	/* Gen3 SDHI DMAC can handle 0xffffffff blk count, but seg = 1 */
+	/* DMAC can handle 0xffffffff blk count but only 1 segment */
 	.max_blk_count	= 0xffffffff,
 	.max_segs	= 1,
 };
@@ -159,9 +159,6 @@ renesas_sdhi_internal_dmac_start_dma(struct tmio_mmc_host *host,
 	struct scatterlist *sg = host->sg_ptr;
 	u32 dtran_mode = DTRAN_MODE_BUS_WID_TH | DTRAN_MODE_ADDR_MODE;
 
-	/* This DMAC cannot handle if sg_len is not 1 */
-	WARN_ON(host->sg_len > 1);
-
 	if (!dma_map_sg(&host->pdev->dev, sg, host->sg_len,
 			mmc_get_dma_dir(data)))
 		goto force_pio;
-- 
2.11.0

Re: [PATCH 1/5] mmc: renesas_sdhi_internal_dmac: limit DMA RX for old SoCs

[Geert Uytterhoeven]

Hi Wolfram,

On Tue, Apr 10, 2018 at 11:38 AM, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> Early revisions of certain SoCs cannot do multiple DMA RX streams in
> parallel. To avoid data corruption, only allow one DMA RX channel and
> fall back to PIO, if needed.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Tested-by: Nguyen Viet Dung <dung.nguyen.aj@renesas.com>

Thanks for your patch!

> --- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> +++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c

>  static int renesas_sdhi_internal_dmac_probe(struct platform_device *pdev)
>  {
> -       if (!soc_device_match(gen3_soc_whitelist))
> +       const struct soc_device_attribute *soc = soc_device_match(gen3_soc_whitelist);
> +
> +       if (!soc)
>                 return -ENODEV;
>
> +       if (soc->data)

This non-NULL check is not really needed.

> +               global_flags |= (unsigned long)soc->data;
> +
>         return renesas_sdhi_probe(pdev, &renesas_sdhi_internal_dmac_dma_ops);
>  }

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/5] mmc: renesas_sdhi_internal_dmac: limit DMA RX for old SoCs

[Simon Horman]

On Tue, Apr 10, 2018 at 11:38:27AM +0200, Wolfram Sang wrote:
> Early revisions of certain SoCs cannot do multiple DMA RX streams in
> parallel. To avoid data corruption, only allow one DMA RX channel and
> fall back to PIO, if needed.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Tested-by: Nguyen Viet Dung <dung.nguyen.aj@renesas.com>
> ---
>  drivers/mmc/host/renesas_sdhi_internal_dmac.c | 35 ++++++++++++++++++++++++---
>  1 file changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> index 8e0acd197c43..9c50d64cd10c 100644
> --- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> +++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> @@ -9,6 +9,7 @@
>   * published by the Free Software Foundation.
>   */
>  
> +#include <linux/bitops.h>
>  #include <linux/device.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/io-64-nonatomic-hi-lo.h>
> @@ -62,6 +63,17 @@
>   *   need a custom accessor.
>   */
>  
> +static unsigned long global_flags;

Is the restriction on concurrent DMA RX streams global or per-device?

> +/*
> + * Workaround for avoiding to use RX DMAC by multiple channels.
> + * On R-Car H3 ES1.* and M3-W ES1.0, when multiple SDHI channels use
> + * RX DMAC simultaneously, sometimes hundreds of bytes data are not
> + * stored into the system memory even if the DMAC interrupt happened.
> + * So, this driver then uses one RX DMAC channel only.
> + */
> +#define SDHI_INTERNAL_DMAC_ONE_RX_ONLY	0
> +#define SDHI_INTERNAL_DMAC_RX_IN_USE	1
> +
>  /* Definitions for sampling clocks */
>  static struct renesas_sdhi_scc rcar_gen3_scc_taps[] = {
>  	{
> @@ -126,6 +138,10 @@ renesas_sdhi_internal_dmac_abort_dma(struct tmio_mmc_host *host) {
>  	renesas_sdhi_internal_dmac_dm_write(host, DM_CM_RST,
>  					    RST_RESERVED_BITS | val);
>  
> +
> +	if (host->data && host->data->flags & MMC_DATA_READ)
> +		clear_bit(SDHI_INTERNAL_DMAC_RX_IN_USE, &global_flags);
>  	renesas_sdhi_internal_dmac_enable_dma(host, true);
>  }
>  
> @@ -155,6 +171,9 @@ renesas_sdhi_internal_dmac_start_dma(struct tmio_mmc_host *host,
>  	if (data->flags & MMC_DATA_READ) {
>  		dtran_mode |= DTRAN_MODE_CH_NUM_CH1;
>  		dir = DMA_FROM_DEVICE;
> +		if (test_bit(SDHI_INTERNAL_DMAC_ONE_RX_ONLY, &global_flags) &&
> +		    test_and_set_bit(SDHI_INTERNAL_DMAC_RX_IN_USE, &global_flags))
> +			goto force_pio;
>  	} else {
>  		dtran_mode |= DTRAN_MODE_CH_NUM_CH0;
>  		dir = DMA_TO_DEVICE;
> @@ -208,6 +227,9 @@ static void renesas_sdhi_internal_dmac_complete_tasklet_fn(unsigned long arg)
>  	renesas_sdhi_internal_dmac_enable_dma(host, false);
>  	dma_unmap_sg(&host->pdev->dev, host->sg_ptr, host->sg_len, dir);
>  
> +	if (dir == DMA_FROM_DEVICE)
> +		clear_bit(SDHI_INTERNAL_DMAC_RX_IN_USE, &global_flags);
> +

Is clear_bit() expensive? If so it might be worth avoiding on SoCs that
don't have the restriction covered by this patch.

> +
>  	tmio_mmc_do_data_irq(host);
>  out:
>  	spin_unlock_irq(&host->lock);
> @@ -251,18 +273,25 @@ static const struct tmio_mmc_dma_ops renesas_sdhi_internal_dmac_dma_ops = {
>   * implementation as others may use a different implementation.
>   */
>  static const struct soc_device_attribute gen3_soc_whitelist[] = {
> -        { .soc_id = "r8a7795", .revision = "ES1.*" },
> +        { .soc_id = "r8a7795", .revision = "ES1.*",
> +	  .data = (void *)BIT(SDHI_INTERNAL_DMAC_ONE_RX_ONLY) },
>          { .soc_id = "r8a7795", .revision = "ES2.0" },
> -        { .soc_id = "r8a7796", .revision = "ES1.0" },
> +        { .soc_id = "r8a7796", .revision = "ES1.0",
> +	  .data = (void *)BIT(SDHI_INTERNAL_DMAC_ONE_RX_ONLY) },
>          { .soc_id = "r8a77995", .revision = "ES1.0" },
>          { /* sentinel */ }
>  };
>  
>  static int renesas_sdhi_internal_dmac_probe(struct platform_device *pdev)
>  {
> -	if (!soc_device_match(gen3_soc_whitelist))
> +	const struct soc_device_attribute *soc = soc_device_match(gen3_soc_whitelist);
> +
> +	if (!soc)
>  		return -ENODEV;
>  
> +	if (soc->data)
> +		global_flags |= (unsigned long)soc->data;
> +
>  	return renesas_sdhi_probe(pdev, &renesas_sdhi_internal_dmac_dma_ops);
>  }
>  
> -- 
> 2.11.0
> 

Re: [PATCH 2/5] mmc: renesas_sdhi: use helpers to access struct scatterlist members

[Simon Horman]

On Tue, Apr 10, 2018 at 11:38:28AM +0200, Wolfram Sang wrote:
> From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> Instead of directly accessing the members of struct scatterlist use the
> helpers mmc_get_dma_dir() and sg_dma_address() in
> renesas_sdhi_internal_dmac_start_dma(). Based on previous work by
> Masaharu Hayakawa.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> [rebased to mmc/next]
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

Re: [PATCH 3/5] mmc: renesas_sdhi: Fix alignment check of sg buffer

[Simon Horman]

On Tue, Apr 10, 2018 at 11:38:29AM +0200, Wolfram Sang wrote:
> From: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
> 
> Sometimes sg->offset is not used for buffer addresses allocated by
> dma_map_sg(), so alignment checks should be done on the allocated buffer
> addresses. Delete the alignment check for sg->offset that is done before
> dma_map_sg(). Instead, it performs the alignment check for
> sg->dma_address after dma_map_sg().
> 
> Signed-off-by: Masaharu Hayakawa <masaharu.hayakawa.ry@renesas.com>
> [Niklas: broke this commit in two and tidied small style issue]
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> [rebased to mmc/next]
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

Re: [PATCH 5/5] mmc: renesas_sdhi_internal_dmac: remove superfluous WARN

[Simon Horman]

On Tue, Apr 10, 2018 at 11:38:31AM +0200, Wolfram Sang wrote:
> The WARN can never trigger because we limited the max_seg number in
> renesas_sdhi_of_data already. Remove it and update the comment.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

Re: [PATCH 1/5] mmc: renesas_sdhi_internal_dmac: limit DMA RX for old SoCs

[Wolfram Sang]

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


> > +       if (soc->data)
> 
> This non-NULL check is not really needed.

And if we match using the Gen3 generic compatible with a non-whitelisted
SoC?


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

Re: [PATCH 1/5] mmc: renesas_sdhi_internal_dmac: limit DMA RX for old SoCs

[Wolfram Sang]

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


> > +static unsigned long global_flags;
> 
> Is the restriction on concurrent DMA RX streams global or per-device?

? Each device has only one DMA RX channel. Hey Simon, you upstreamed
this driver :) Or did I get the question wrong?

> > +	if (dir == DMA_FROM_DEVICE)
> > +		clear_bit(SDHI_INTERNAL_DMAC_RX_IN_USE, &global_flags);
> > +
> 
> Is clear_bit() expensive? If so it might be worth avoiding on SoCs that
> don't have the restriction covered by this patch.

It's an atoimc bitop, so maybe has a memory barrier. Hmm, the above
version is better on the cache lines, though, if you don't have the
restriction.

Will think about it and make sure both clear_bit() are in sync.


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

Re: [PATCH 1/5] mmc: renesas_sdhi_internal_dmac: limit DMA RX for old SoCs

[Geert Uytterhoeven]

Hi Wolfram,

On Thu, Apr 12, 2018 at 1:11 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
>> > +       if (soc->data)
>>
>> This non-NULL check is not really needed.
>
> And if we match using the Gen3 generic compatible with a non-whitelisted
> SoC?

That should have been caught by the !soc check above, and have already
returned with -ENODEV.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/5] mmc: renesas_sdhi_internal_dmac: limit DMA RX for old SoCs

[Wolfram Sang]

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


> That should have been caught by the !soc check above, and have already
> returned with -ENODEV.

Now I get it: You mean non-0 check, not non-NULL check...


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

Re: [PATCH 1/5] mmc: renesas_sdhi_internal_dmac: limit DMA RX for old SoCs

[Geert Uytterhoeven]

Hi Wolfram,

On Thu, Apr 12, 2018 at 1:31 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
>> That should have been caught by the !soc check above, and have already
>> returned with -ENODEV.
>
> Now I get it: You mean non-0 check, not non-NULL check...

soc->data _is_ a pointer. You only cast it to an integer on the next line.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/5] mmc: renesas_sdhi_internal_dmac: limit DMA RX for old SoCs

[Wolfram Sang]

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

On Thu, Apr 12, 2018 at 01:34:41PM +0200, Geert Uytterhoeven wrote:
> Hi Wolfram,
> 
> On Thu, Apr 12, 2018 at 1:31 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> >> That should have been caught by the !soc check above, and have already
> >> returned with -ENODEV.
> >
> > Now I get it: You mean non-0 check, not non-NULL check...
> 
> soc->data _is_ a pointer. You only cast it to an integer on the next line.

Yes. I usually worked with it as an integer, so I got confused. Will
fix.


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

Re: [PATCH 1/5] mmc: renesas_sdhi_internal_dmac: limit DMA RX for old SoCs

[Simon Horman]

On Thu, Apr 12, 2018 at 01:21:42PM +0200, Wolfram Sang wrote:
> 
> > > +static unsigned long global_flags;
> > 
> > Is the restriction on concurrent DMA RX streams global or per-device?
> 
> ? Each device has only one DMA RX channel. Hey Simon, you upstreamed
> this driver :) Or did I get the question wrong?

As I understand things this patch implements a restriction on concurrent
DMA RX streams for old SoCs, corresponding with a limitation in the
hardware.

As the implementation stands it is global - only one DMA RX stream may
be in flight for the entire system. I am wondering if that is the right
granularity for the restriction. Perhaps it could be per-SDHI device,
allowing concurrent streams on different SDHI devices.

I think what you have is safe. But perhaps it could be relaxed.
I do not have any insights regarding the extent of the hardware
restriction (that I can recall at this time).

> > > +	if (dir == DMA_FROM_DEVICE)
> > > +		clear_bit(SDHI_INTERNAL_DMAC_RX_IN_USE, &global_flags);
> > > +
> > 
> > Is clear_bit() expensive? If so it might be worth avoiding on SoCs that
> > don't have the restriction covered by this patch.
> 
> It's an atoimc bitop, so maybe has a memory barrier. Hmm, the above
> version is better on the cache lines, though, if you don't have the
> restriction.
> 
> Will think about it and make sure both clear_bit() are in sync.

Thanks, I don't feel strongly about this.

Re: [PATCH 1/5] mmc: renesas_sdhi_internal_dmac: limit DMA RX for old SoCs

[Wolfram Sang]

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


> As the implementation stands it is global - only one DMA RX stream may
> be in flight for the entire system. I am wondering if that is the right
> granularity for the restriction. Perhaps it could be per-SDHI device,
> allowing concurrent streams on different SDHI devices.

As we have only one DMA RX channel per device, there is no other
concurrency than the global concurrency.


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

Re: [PATCH 1/5] mmc: renesas_sdhi_internal_dmac: limit DMA RX for old SoCs

[Simon Horman]

On Fri, Apr 13, 2018 at 10:35:48AM +0200, Wolfram Sang wrote:
> 
> > As the implementation stands it is global - only one DMA RX stream may
> > be in flight for the entire system. I am wondering if that is the right
> > granularity for the restriction. Perhaps it could be per-SDHI device,
> > allowing concurrent streams on different SDHI devices.
> 
> As we have only one DMA RX channel per device, there is no other
> concurrency than the global concurrency.

Thanks, understood.