linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] mmc: mmci: stm32: manage unaligned DMA req for SDIO
@ 2022-03-17 11:19 Yann Gautier
  2022-03-17 11:19 ` [PATCH 1/2] mmc: mmci: stm32: correctly check all elements of sg list Yann Gautier
  2022-03-17 11:19 ` [PATCH 2/2] mmc: mmci: stm32: use a buffer for unaligned DMA requests Yann Gautier
  0 siblings, 2 replies; 8+ messages in thread
From: Yann Gautier @ 2022-03-17 11:19 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Christophe Kerello, Ludovic Barre, Maxime Coquelin,
	Alexandre Torgue, Philipp Zabel, Linus Walleij, linux-mmc,
	linux-stm32, linux-arm-kernel, linux-kernel, Yann Gautier

The first patch corrects an issue when parsing the sg list to check DMA
constraints.
The second patch manages those DMA alignment by copying from or to a
bounce buffer, the way it is done in meson-gx-mmc.

Yann Gautier (2):
  mmc: mmci: stm32: correctly check all elements of sg list
  mmc: mmci: stm32: use a buffer for unaligned DMA requests

 drivers/mmc/host/mmci_stm32_sdmmc.c | 86 ++++++++++++++++++++++-------
 1 file changed, 66 insertions(+), 20 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/2] mmc: mmci: stm32: correctly check all elements of sg list
  2022-03-17 11:19 [PATCH 0/2] mmc: mmci: stm32: manage unaligned DMA req for SDIO Yann Gautier
@ 2022-03-17 11:19 ` Yann Gautier
  2022-03-28 14:04   ` Ulf Hansson
  2022-03-17 11:19 ` [PATCH 2/2] mmc: mmci: stm32: use a buffer for unaligned DMA requests Yann Gautier
  1 sibling, 1 reply; 8+ messages in thread
From: Yann Gautier @ 2022-03-17 11:19 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Christophe Kerello, Ludovic Barre, Maxime Coquelin,
	Alexandre Torgue, Philipp Zabel, Linus Walleij, linux-mmc,
	linux-stm32, linux-arm-kernel, linux-kernel, Yann Gautier

Use sg and not data->sg when checking sg list elements. Else only the
first element alignment is checked.
The last element should be checked the same way, for_each_sg already set
sg to sg_next(sg).

Fixes: 46b723dd867d ("mmc: mmci: add stm32 sdmmc variant")

Signed-off-by: Yann Gautier <yann.gautier@foss.st.com>
---
 drivers/mmc/host/mmci_stm32_sdmmc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/mmci_stm32_sdmmc.c b/drivers/mmc/host/mmci_stm32_sdmmc.c
index 9c13f2c31365..4566d7fc9055 100644
--- a/drivers/mmc/host/mmci_stm32_sdmmc.c
+++ b/drivers/mmc/host/mmci_stm32_sdmmc.c
@@ -62,8 +62,8 @@ static int sdmmc_idma_validate_data(struct mmci_host *host,
 	 * excepted the last element which has no constraint on idmasize
 	 */
 	for_each_sg(data->sg, sg, data->sg_len - 1, i) {
-		if (!IS_ALIGNED(data->sg->offset, sizeof(u32)) ||
-		    !IS_ALIGNED(data->sg->length, SDMMC_IDMA_BURST)) {
+		if (!IS_ALIGNED(sg->offset, sizeof(u32)) ||
+		    !IS_ALIGNED(sg->length, SDMMC_IDMA_BURST)) {
 			dev_err(mmc_dev(host->mmc),
 				"unaligned scatterlist: ofst:%x length:%d\n",
 				data->sg->offset, data->sg->length);
@@ -71,7 +71,7 @@ static int sdmmc_idma_validate_data(struct mmci_host *host,
 		}
 	}
 
-	if (!IS_ALIGNED(data->sg->offset, sizeof(u32))) {
+	if (!IS_ALIGNED(sg->offset, sizeof(u32))) {
 		dev_err(mmc_dev(host->mmc),
 			"unaligned last scatterlist: ofst:%x length:%d\n",
 			data->sg->offset, data->sg->length);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/2] mmc: mmci: stm32: use a buffer for unaligned DMA requests
  2022-03-17 11:19 [PATCH 0/2] mmc: mmci: stm32: manage unaligned DMA req for SDIO Yann Gautier
  2022-03-17 11:19 ` [PATCH 1/2] mmc: mmci: stm32: correctly check all elements of sg list Yann Gautier
@ 2022-03-17 11:19 ` Yann Gautier
  2022-03-24 11:55   ` Ulf Hansson
  1 sibling, 1 reply; 8+ messages in thread
From: Yann Gautier @ 2022-03-17 11:19 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Christophe Kerello, Ludovic Barre, Maxime Coquelin,
	Alexandre Torgue, Philipp Zabel, Linus Walleij, linux-mmc,
	linux-stm32, linux-arm-kernel, linux-kernel, Yann Gautier

In SDIO mode, the sg list for requests can be unaligned with what the
STM32 SDMMC internal DMA can support. In that case, instead of failing,
use a temporary bounce buffer to copy from/to the sg list.
This buffer is limited to 1MB. But for that we need to also limit
max_req_size to 1MB. It has not shown any throughput penalties for
SD-cards or eMMC.

Signed-off-by: Yann Gautier <yann.gautier@foss.st.com>
---
 drivers/mmc/host/mmci_stm32_sdmmc.c | 80 +++++++++++++++++++++++------
 1 file changed, 63 insertions(+), 17 deletions(-)

diff --git a/drivers/mmc/host/mmci_stm32_sdmmc.c b/drivers/mmc/host/mmci_stm32_sdmmc.c
index 4566d7fc9055..a4414e32800f 100644
--- a/drivers/mmc/host/mmci_stm32_sdmmc.c
+++ b/drivers/mmc/host/mmci_stm32_sdmmc.c
@@ -43,6 +43,9 @@ struct sdmmc_lli_desc {
 struct sdmmc_idma {
 	dma_addr_t sg_dma;
 	void *sg_cpu;
+	dma_addr_t bounce_dma_addr;
+	void *bounce_buf;
+	bool use_bounce_buffer;
 };
 
 struct sdmmc_dlyb {
@@ -54,6 +57,7 @@ struct sdmmc_dlyb {
 static int sdmmc_idma_validate_data(struct mmci_host *host,
 				    struct mmc_data *data)
 {
+	struct sdmmc_idma *idma = host->dma_priv;
 	struct scatterlist *sg;
 	int i;
 
@@ -61,21 +65,23 @@ static int sdmmc_idma_validate_data(struct mmci_host *host,
 	 * idma has constraints on idmabase & idmasize for each element
 	 * excepted the last element which has no constraint on idmasize
 	 */
+	idma->use_bounce_buffer = false;
 	for_each_sg(data->sg, sg, data->sg_len - 1, i) {
 		if (!IS_ALIGNED(sg->offset, sizeof(u32)) ||
 		    !IS_ALIGNED(sg->length, SDMMC_IDMA_BURST)) {
-			dev_err(mmc_dev(host->mmc),
+			dev_dbg(mmc_dev(host->mmc),
 				"unaligned scatterlist: ofst:%x length:%d\n",
 				data->sg->offset, data->sg->length);
-			return -EINVAL;
+			idma->use_bounce_buffer = true;
+			return 0;
 		}
 	}
 
 	if (!IS_ALIGNED(sg->offset, sizeof(u32))) {
-		dev_err(mmc_dev(host->mmc),
+		dev_dbg(mmc_dev(host->mmc),
 			"unaligned last scatterlist: ofst:%x length:%d\n",
 			data->sg->offset, data->sg->length);
-		return -EINVAL;
+		idma->use_bounce_buffer = true;
 	}
 
 	return 0;
@@ -84,18 +90,29 @@ static int sdmmc_idma_validate_data(struct mmci_host *host,
 static int _sdmmc_idma_prep_data(struct mmci_host *host,
 				 struct mmc_data *data)
 {
-	int n_elem;
+	struct sdmmc_idma *idma = host->dma_priv;
 
-	n_elem = dma_map_sg(mmc_dev(host->mmc),
-			    data->sg,
-			    data->sg_len,
-			    mmc_get_dma_dir(data));
+	if (idma->use_bounce_buffer) {
+		if (data->flags & MMC_DATA_WRITE) {
+			unsigned int xfer_bytes = data->blksz * data->blocks;
 
-	if (!n_elem) {
-		dev_err(mmc_dev(host->mmc), "dma_map_sg failed\n");
-		return -EINVAL;
-	}
+			sg_copy_to_buffer(data->sg, data->sg_len,
+					  idma->bounce_buf, xfer_bytes);
+			dma_wmb();
+		}
+	} else {
+		int n_elem;
+
+		n_elem = dma_map_sg(mmc_dev(host->mmc),
+				    data->sg,
+				    data->sg_len,
+				    mmc_get_dma_dir(data));
 
+		if (!n_elem) {
+			dev_err(mmc_dev(host->mmc), "dma_map_sg failed\n");
+			return -EINVAL;
+		}
+	}
 	return 0;
 }
 
@@ -112,8 +129,19 @@ static int sdmmc_idma_prep_data(struct mmci_host *host,
 static void sdmmc_idma_unprep_data(struct mmci_host *host,
 				   struct mmc_data *data, int err)
 {
-	dma_unmap_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
-		     mmc_get_dma_dir(data));
+	struct sdmmc_idma *idma = host->dma_priv;
+
+	if (idma->use_bounce_buffer) {
+		if (data->flags & MMC_DATA_READ) {
+			unsigned int xfer_bytes = data->blksz * data->blocks;
+
+			sg_copy_from_buffer(data->sg, data->sg_len,
+					    idma->bounce_buf, xfer_bytes);
+		}
+	} else {
+		dma_unmap_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
+			     mmc_get_dma_dir(data));
+	}
 }
 
 static int sdmmc_idma_setup(struct mmci_host *host)
@@ -137,6 +165,16 @@ static int sdmmc_idma_setup(struct mmci_host *host)
 		host->mmc->max_segs = SDMMC_LLI_BUF_LEN /
 			sizeof(struct sdmmc_lli_desc);
 		host->mmc->max_seg_size = host->variant->stm32_idmabsize_mask;
+
+		host->mmc->max_req_size = SZ_1M;
+		idma->bounce_buf = dmam_alloc_coherent(dev,
+						       host->mmc->max_req_size,
+						       &idma->bounce_dma_addr,
+						       GFP_KERNEL);
+		if (!idma->bounce_buf) {
+			dev_err(dev, "Unable to map allocate DMA bounce buffer.\n");
+			return -ENOMEM;
+		}
 	} else {
 		host->mmc->max_segs = 1;
 		host->mmc->max_seg_size = host->mmc->max_req_size;
@@ -154,8 +192,16 @@ static int sdmmc_idma_start(struct mmci_host *host, unsigned int *datactrl)
 	struct scatterlist *sg;
 	int i;
 
-	if (!host->variant->dma_lli || data->sg_len == 1) {
-		writel_relaxed(sg_dma_address(data->sg),
+	if (!host->variant->dma_lli || data->sg_len == 1 ||
+	    idma->use_bounce_buffer) {
+		u32 dma_addr;
+
+		if (idma->use_bounce_buffer)
+			dma_addr = idma->bounce_dma_addr;
+		else
+			dma_addr = sg_dma_address(data->sg);
+
+		writel_relaxed(dma_addr,
 			       host->base + MMCI_STM32_IDMABASE0R);
 		writel_relaxed(MMCI_STM32_IDMAEN,
 			       host->base + MMCI_STM32_IDMACTRLR);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] mmc: mmci: stm32: use a buffer for unaligned DMA requests
  2022-03-17 11:19 ` [PATCH 2/2] mmc: mmci: stm32: use a buffer for unaligned DMA requests Yann Gautier
@ 2022-03-24 11:55   ` Ulf Hansson
  2022-03-24 16:23     ` Yann Gautier
  0 siblings, 1 reply; 8+ messages in thread
From: Ulf Hansson @ 2022-03-24 11:55 UTC (permalink / raw)
  To: Yann Gautier
  Cc: Christophe Kerello, Ludovic Barre, Maxime Coquelin,
	Alexandre Torgue, Philipp Zabel, Linus Walleij, linux-mmc,
	linux-stm32, linux-arm-kernel, linux-kernel

On Thu, 17 Mar 2022 at 12:19, Yann Gautier <yann.gautier@foss.st.com> wrote:
>
> In SDIO mode, the sg list for requests can be unaligned with what the
> STM32 SDMMC internal DMA can support. In that case, instead of failing,
> use a temporary bounce buffer to copy from/to the sg list.
> This buffer is limited to 1MB. But for that we need to also limit
> max_req_size to 1MB. It has not shown any throughput penalties for
> SD-cards or eMMC.
>
> Signed-off-by: Yann Gautier <yann.gautier@foss.st.com>
> ---
>  drivers/mmc/host/mmci_stm32_sdmmc.c | 80 +++++++++++++++++++++++------
>  1 file changed, 63 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/mmc/host/mmci_stm32_sdmmc.c b/drivers/mmc/host/mmci_stm32_sdmmc.c
> index 4566d7fc9055..a4414e32800f 100644
> --- a/drivers/mmc/host/mmci_stm32_sdmmc.c
> +++ b/drivers/mmc/host/mmci_stm32_sdmmc.c
> @@ -43,6 +43,9 @@ struct sdmmc_lli_desc {
>  struct sdmmc_idma {
>         dma_addr_t sg_dma;
>         void *sg_cpu;
> +       dma_addr_t bounce_dma_addr;
> +       void *bounce_buf;
> +       bool use_bounce_buffer;
>  };
>
>  struct sdmmc_dlyb {
> @@ -54,6 +57,7 @@ struct sdmmc_dlyb {
>  static int sdmmc_idma_validate_data(struct mmci_host *host,
>                                     struct mmc_data *data)
>  {
> +       struct sdmmc_idma *idma = host->dma_priv;
>         struct scatterlist *sg;
>         int i;
>
> @@ -61,21 +65,23 @@ static int sdmmc_idma_validate_data(struct mmci_host *host,
>          * idma has constraints on idmabase & idmasize for each element
>          * excepted the last element which has no constraint on idmasize
>          */
> +       idma->use_bounce_buffer = false;
>         for_each_sg(data->sg, sg, data->sg_len - 1, i) {
>                 if (!IS_ALIGNED(sg->offset, sizeof(u32)) ||
>                     !IS_ALIGNED(sg->length, SDMMC_IDMA_BURST)) {
> -                       dev_err(mmc_dev(host->mmc),
> +                       dev_dbg(mmc_dev(host->mmc),
>                                 "unaligned scatterlist: ofst:%x length:%d\n",
>                                 data->sg->offset, data->sg->length);
> -                       return -EINVAL;
> +                       idma->use_bounce_buffer = true;
> +                       return 0;
>                 }
>         }
>
>         if (!IS_ALIGNED(sg->offset, sizeof(u32))) {
> -               dev_err(mmc_dev(host->mmc),
> +               dev_dbg(mmc_dev(host->mmc),
>                         "unaligned last scatterlist: ofst:%x length:%d\n",
>                         data->sg->offset, data->sg->length);
> -               return -EINVAL;
> +               idma->use_bounce_buffer = true;
>         }
>
>         return 0;
> @@ -84,18 +90,29 @@ static int sdmmc_idma_validate_data(struct mmci_host *host,
>  static int _sdmmc_idma_prep_data(struct mmci_host *host,
>                                  struct mmc_data *data)
>  {
> -       int n_elem;
> +       struct sdmmc_idma *idma = host->dma_priv;
>
> -       n_elem = dma_map_sg(mmc_dev(host->mmc),
> -                           data->sg,
> -                           data->sg_len,
> -                           mmc_get_dma_dir(data));
> +       if (idma->use_bounce_buffer) {
> +               if (data->flags & MMC_DATA_WRITE) {
> +                       unsigned int xfer_bytes = data->blksz * data->blocks;
>
> -       if (!n_elem) {
> -               dev_err(mmc_dev(host->mmc), "dma_map_sg failed\n");
> -               return -EINVAL;
> -       }
> +                       sg_copy_to_buffer(data->sg, data->sg_len,
> +                                         idma->bounce_buf, xfer_bytes);
> +                       dma_wmb();
> +               }
> +       } else {
> +               int n_elem;
> +
> +               n_elem = dma_map_sg(mmc_dev(host->mmc),
> +                                   data->sg,
> +                                   data->sg_len,
> +                                   mmc_get_dma_dir(data));
>
> +               if (!n_elem) {
> +                       dev_err(mmc_dev(host->mmc), "dma_map_sg failed\n");
> +                       return -EINVAL;
> +               }
> +       }
>         return 0;
>  }
>
> @@ -112,8 +129,19 @@ static int sdmmc_idma_prep_data(struct mmci_host *host,
>  static void sdmmc_idma_unprep_data(struct mmci_host *host,
>                                    struct mmc_data *data, int err)
>  {
> -       dma_unmap_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
> -                    mmc_get_dma_dir(data));
> +       struct sdmmc_idma *idma = host->dma_priv;
> +
> +       if (idma->use_bounce_buffer) {
> +               if (data->flags & MMC_DATA_READ) {
> +                       unsigned int xfer_bytes = data->blksz * data->blocks;
> +
> +                       sg_copy_from_buffer(data->sg, data->sg_len,
> +                                           idma->bounce_buf, xfer_bytes);
> +               }
> +       } else {
> +               dma_unmap_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
> +                            mmc_get_dma_dir(data));
> +       }
>  }
>
>  static int sdmmc_idma_setup(struct mmci_host *host)
> @@ -137,6 +165,16 @@ static int sdmmc_idma_setup(struct mmci_host *host)
>                 host->mmc->max_segs = SDMMC_LLI_BUF_LEN /
>                         sizeof(struct sdmmc_lli_desc);
>                 host->mmc->max_seg_size = host->variant->stm32_idmabsize_mask;
> +
> +               host->mmc->max_req_size = SZ_1M;
> +               idma->bounce_buf = dmam_alloc_coherent(dev,
> +                                                      host->mmc->max_req_size,
> +                                                      &idma->bounce_dma_addr,
> +                                                      GFP_KERNEL);
> +               if (!idma->bounce_buf) {
> +                       dev_err(dev, "Unable to map allocate DMA bounce buffer.\n");
> +                       return -ENOMEM;

If we fail to allocate the 1M bounce buffer, then we end up always
using a PIO based mode, right?

Perhaps we can allow the above allocation to fail, but then limit us
to use DMA only when the buffers are properly aligned? Would that
work?

> +               }
>         } else {
>                 host->mmc->max_segs = 1;
>                 host->mmc->max_seg_size = host->mmc->max_req_size;
> @@ -154,8 +192,16 @@ static int sdmmc_idma_start(struct mmci_host *host, unsigned int *datactrl)
>         struct scatterlist *sg;
>         int i;
>
> -       if (!host->variant->dma_lli || data->sg_len == 1) {
> -               writel_relaxed(sg_dma_address(data->sg),
> +       if (!host->variant->dma_lli || data->sg_len == 1 ||
> +           idma->use_bounce_buffer) {
> +               u32 dma_addr;
> +
> +               if (idma->use_bounce_buffer)
> +                       dma_addr = idma->bounce_dma_addr;
> +               else
> +                       dma_addr = sg_dma_address(data->sg);
> +
> +               writel_relaxed(dma_addr,
>                                host->base + MMCI_STM32_IDMABASE0R);
>                 writel_relaxed(MMCI_STM32_IDMAEN,
>                                host->base + MMCI_STM32_IDMACTRLR);

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] mmc: mmci: stm32: use a buffer for unaligned DMA requests
  2022-03-24 11:55   ` Ulf Hansson
@ 2022-03-24 16:23     ` Yann Gautier
  2022-03-25 13:43       ` Ulf Hansson
  0 siblings, 1 reply; 8+ messages in thread
From: Yann Gautier @ 2022-03-24 16:23 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Christophe Kerello, Ludovic Barre, Maxime Coquelin,
	Alexandre Torgue, Philipp Zabel, Linus Walleij, linux-mmc,
	linux-stm32, linux-arm-kernel, linux-kernel

On 3/24/22 12:55, Ulf Hansson wrote:
> On Thu, 17 Mar 2022 at 12:19, Yann Gautier <yann.gautier@foss.st.com> wrote:
>>
>> In SDIO mode, the sg list for requests can be unaligned with what the
>> STM32 SDMMC internal DMA can support. In that case, instead of failing,
>> use a temporary bounce buffer to copy from/to the sg list.
>> This buffer is limited to 1MB. But for that we need to also limit
>> max_req_size to 1MB. It has not shown any throughput penalties for
>> SD-cards or eMMC.
>>
>> Signed-off-by: Yann Gautier <yann.gautier@foss.st.com>
>> ---
>>   drivers/mmc/host/mmci_stm32_sdmmc.c | 80 +++++++++++++++++++++++------
>>   1 file changed, 63 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/mmc/host/mmci_stm32_sdmmc.c b/drivers/mmc/host/mmci_stm32_sdmmc.c
>> index 4566d7fc9055..a4414e32800f 100644
>> --- a/drivers/mmc/host/mmci_stm32_sdmmc.c
>> +++ b/drivers/mmc/host/mmci_stm32_sdmmc.c
>> @@ -43,6 +43,9 @@ struct sdmmc_lli_desc {
>>   struct sdmmc_idma {
>>          dma_addr_t sg_dma;
>>          void *sg_cpu;
>> +       dma_addr_t bounce_dma_addr;
>> +       void *bounce_buf;
>> +       bool use_bounce_buffer;
>>   };
>>
>>   struct sdmmc_dlyb {
>> @@ -54,6 +57,7 @@ struct sdmmc_dlyb {
>>   static int sdmmc_idma_validate_data(struct mmci_host *host,
>>                                      struct mmc_data *data)
>>   {
>> +       struct sdmmc_idma *idma = host->dma_priv;
>>          struct scatterlist *sg;
>>          int i;
>>
>> @@ -61,21 +65,23 @@ static int sdmmc_idma_validate_data(struct mmci_host *host,
>>           * idma has constraints on idmabase & idmasize for each element
>>           * excepted the last element which has no constraint on idmasize
>>           */
>> +       idma->use_bounce_buffer = false;
>>          for_each_sg(data->sg, sg, data->sg_len - 1, i) {
>>                  if (!IS_ALIGNED(sg->offset, sizeof(u32)) ||
>>                      !IS_ALIGNED(sg->length, SDMMC_IDMA_BURST)) {
>> -                       dev_err(mmc_dev(host->mmc),
>> +                       dev_dbg(mmc_dev(host->mmc),
>>                                  "unaligned scatterlist: ofst:%x length:%d\n",
>>                                  data->sg->offset, data->sg->length);
>> -                       return -EINVAL;
>> +                       idma->use_bounce_buffer = true;
>> +                       return 0;
>>                  }
>>          }
>>
>>          if (!IS_ALIGNED(sg->offset, sizeof(u32))) {
>> -               dev_err(mmc_dev(host->mmc),
>> +               dev_dbg(mmc_dev(host->mmc),
>>                          "unaligned last scatterlist: ofst:%x length:%d\n",
>>                          data->sg->offset, data->sg->length);
>> -               return -EINVAL;
>> +               idma->use_bounce_buffer = true;
>>          }
>>
>>          return 0;
>> @@ -84,18 +90,29 @@ static int sdmmc_idma_validate_data(struct mmci_host *host,
>>   static int _sdmmc_idma_prep_data(struct mmci_host *host,
>>                                   struct mmc_data *data)
>>   {
>> -       int n_elem;
>> +       struct sdmmc_idma *idma = host->dma_priv;
>>
>> -       n_elem = dma_map_sg(mmc_dev(host->mmc),
>> -                           data->sg,
>> -                           data->sg_len,
>> -                           mmc_get_dma_dir(data));
>> +       if (idma->use_bounce_buffer) {
>> +               if (data->flags & MMC_DATA_WRITE) {
>> +                       unsigned int xfer_bytes = data->blksz * data->blocks;
>>
>> -       if (!n_elem) {
>> -               dev_err(mmc_dev(host->mmc), "dma_map_sg failed\n");
>> -               return -EINVAL;
>> -       }
>> +                       sg_copy_to_buffer(data->sg, data->sg_len,
>> +                                         idma->bounce_buf, xfer_bytes);
>> +                       dma_wmb();
>> +               }
>> +       } else {
>> +               int n_elem;
>> +
>> +               n_elem = dma_map_sg(mmc_dev(host->mmc),
>> +                                   data->sg,
>> +                                   data->sg_len,
>> +                                   mmc_get_dma_dir(data));
>>
>> +               if (!n_elem) {
>> +                       dev_err(mmc_dev(host->mmc), "dma_map_sg failed\n");
>> +                       return -EINVAL;
>> +               }
>> +       }
>>          return 0;
>>   }
>>
>> @@ -112,8 +129,19 @@ static int sdmmc_idma_prep_data(struct mmci_host *host,
>>   static void sdmmc_idma_unprep_data(struct mmci_host *host,
>>                                     struct mmc_data *data, int err)
>>   {
>> -       dma_unmap_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
>> -                    mmc_get_dma_dir(data));
>> +       struct sdmmc_idma *idma = host->dma_priv;
>> +
>> +       if (idma->use_bounce_buffer) {
>> +               if (data->flags & MMC_DATA_READ) {
>> +                       unsigned int xfer_bytes = data->blksz * data->blocks;
>> +
>> +                       sg_copy_from_buffer(data->sg, data->sg_len,
>> +                                           idma->bounce_buf, xfer_bytes);
>> +               }
>> +       } else {
>> +               dma_unmap_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
>> +                            mmc_get_dma_dir(data));
>> +       }
>>   }
>>
>>   static int sdmmc_idma_setup(struct mmci_host *host)
>> @@ -137,6 +165,16 @@ static int sdmmc_idma_setup(struct mmci_host *host)
>>                  host->mmc->max_segs = SDMMC_LLI_BUF_LEN /
>>                          sizeof(struct sdmmc_lli_desc);
>>                  host->mmc->max_seg_size = host->variant->stm32_idmabsize_mask;
>> +
>> +               host->mmc->max_req_size = SZ_1M;
>> +               idma->bounce_buf = dmam_alloc_coherent(dev,
>> +                                                      host->mmc->max_req_size,
>> +                                                      &idma->bounce_dma_addr,
>> +                                                      GFP_KERNEL);
>> +               if (!idma->bounce_buf) {
>> +                       dev_err(dev, "Unable to map allocate DMA bounce buffer.\n");
>> +                       return -ENOMEM;
> 
Hi Ulf,

> If we fail to allocate the 1M bounce buffer, then we end up always
> using a PIO based mode, right?
> 
> Perhaps we can allow the above allocation to fail, but then limit us
> to use DMA only when the buffers are properly aligned? Would that
> work?
> 
We have never supported PIO mode with STM32 variant.
We only support DMA single buffer or DMA LLI.
As we cannot have DMA LLI for unaligned access, we'll default to single 
mode.
If allocation fails, it then won't work.
Maybe we shouldn't fail here, and just check idma->bounce_buf in 
validate data function. If buffer is not allocated, we just return 
-EINVAL as it was done before.

Best regards,
Yann

>> +               }
>>          } else {
>>                  host->mmc->max_segs = 1;
>>                  host->mmc->max_seg_size = host->mmc->max_req_size;
>> @@ -154,8 +192,16 @@ static int sdmmc_idma_start(struct mmci_host *host, unsigned int *datactrl)
>>          struct scatterlist *sg;
>>          int i;
>>
>> -       if (!host->variant->dma_lli || data->sg_len == 1) {
>> -               writel_relaxed(sg_dma_address(data->sg),
>> +       if (!host->variant->dma_lli || data->sg_len == 1 ||
>> +           idma->use_bounce_buffer) {
>> +               u32 dma_addr;
>> +
>> +               if (idma->use_bounce_buffer)
>> +                       dma_addr = idma->bounce_dma_addr;
>> +               else
>> +                       dma_addr = sg_dma_address(data->sg);
>> +
>> +               writel_relaxed(dma_addr,
>>                                 host->base + MMCI_STM32_IDMABASE0R);
>>                  writel_relaxed(MMCI_STM32_IDMAEN,
>>                                 host->base + MMCI_STM32_IDMACTRLR);
> 
> Kind regards
> Uffe


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] mmc: mmci: stm32: use a buffer for unaligned DMA requests
  2022-03-24 16:23     ` Yann Gautier
@ 2022-03-25 13:43       ` Ulf Hansson
  2022-03-25 14:01         ` Yann Gautier
  0 siblings, 1 reply; 8+ messages in thread
From: Ulf Hansson @ 2022-03-25 13:43 UTC (permalink / raw)
  To: Yann Gautier
  Cc: Christophe Kerello, Ludovic Barre, Maxime Coquelin,
	Alexandre Torgue, Philipp Zabel, Linus Walleij, linux-mmc,
	linux-stm32, linux-arm-kernel, linux-kernel

On Thu, 24 Mar 2022 at 17:23, Yann Gautier <yann.gautier@foss.st.com> wrote:
>
> On 3/24/22 12:55, Ulf Hansson wrote:
> > On Thu, 17 Mar 2022 at 12:19, Yann Gautier <yann.gautier@foss.st.com> wrote:
> >>
> >> In SDIO mode, the sg list for requests can be unaligned with what the
> >> STM32 SDMMC internal DMA can support. In that case, instead of failing,
> >> use a temporary bounce buffer to copy from/to the sg list.
> >> This buffer is limited to 1MB. But for that we need to also limit
> >> max_req_size to 1MB. It has not shown any throughput penalties for
> >> SD-cards or eMMC.
> >>
> >> Signed-off-by: Yann Gautier <yann.gautier@foss.st.com>
> >> ---
> >>   drivers/mmc/host/mmci_stm32_sdmmc.c | 80 +++++++++++++++++++++++------
> >>   1 file changed, 63 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/drivers/mmc/host/mmci_stm32_sdmmc.c b/drivers/mmc/host/mmci_stm32_sdmmc.c
> >> index 4566d7fc9055..a4414e32800f 100644
> >> --- a/drivers/mmc/host/mmci_stm32_sdmmc.c
> >> +++ b/drivers/mmc/host/mmci_stm32_sdmmc.c
> >> @@ -43,6 +43,9 @@ struct sdmmc_lli_desc {
> >>   struct sdmmc_idma {
> >>          dma_addr_t sg_dma;
> >>          void *sg_cpu;
> >> +       dma_addr_t bounce_dma_addr;
> >> +       void *bounce_buf;
> >> +       bool use_bounce_buffer;
> >>   };
> >>
> >>   struct sdmmc_dlyb {
> >> @@ -54,6 +57,7 @@ struct sdmmc_dlyb {
> >>   static int sdmmc_idma_validate_data(struct mmci_host *host,
> >>                                      struct mmc_data *data)
> >>   {
> >> +       struct sdmmc_idma *idma = host->dma_priv;
> >>          struct scatterlist *sg;
> >>          int i;
> >>
> >> @@ -61,21 +65,23 @@ static int sdmmc_idma_validate_data(struct mmci_host *host,
> >>           * idma has constraints on idmabase & idmasize for each element
> >>           * excepted the last element which has no constraint on idmasize
> >>           */
> >> +       idma->use_bounce_buffer = false;
> >>          for_each_sg(data->sg, sg, data->sg_len - 1, i) {
> >>                  if (!IS_ALIGNED(sg->offset, sizeof(u32)) ||
> >>                      !IS_ALIGNED(sg->length, SDMMC_IDMA_BURST)) {
> >> -                       dev_err(mmc_dev(host->mmc),
> >> +                       dev_dbg(mmc_dev(host->mmc),
> >>                                  "unaligned scatterlist: ofst:%x length:%d\n",
> >>                                  data->sg->offset, data->sg->length);
> >> -                       return -EINVAL;
> >> +                       idma->use_bounce_buffer = true;
> >> +                       return 0;
> >>                  }
> >>          }
> >>
> >>          if (!IS_ALIGNED(sg->offset, sizeof(u32))) {
> >> -               dev_err(mmc_dev(host->mmc),
> >> +               dev_dbg(mmc_dev(host->mmc),
> >>                          "unaligned last scatterlist: ofst:%x length:%d\n",
> >>                          data->sg->offset, data->sg->length);
> >> -               return -EINVAL;
> >> +               idma->use_bounce_buffer = true;
> >>          }
> >>
> >>          return 0;
> >> @@ -84,18 +90,29 @@ static int sdmmc_idma_validate_data(struct mmci_host *host,
> >>   static int _sdmmc_idma_prep_data(struct mmci_host *host,
> >>                                   struct mmc_data *data)
> >>   {
> >> -       int n_elem;
> >> +       struct sdmmc_idma *idma = host->dma_priv;
> >>
> >> -       n_elem = dma_map_sg(mmc_dev(host->mmc),
> >> -                           data->sg,
> >> -                           data->sg_len,
> >> -                           mmc_get_dma_dir(data));
> >> +       if (idma->use_bounce_buffer) {
> >> +               if (data->flags & MMC_DATA_WRITE) {
> >> +                       unsigned int xfer_bytes = data->blksz * data->blocks;
> >>
> >> -       if (!n_elem) {
> >> -               dev_err(mmc_dev(host->mmc), "dma_map_sg failed\n");
> >> -               return -EINVAL;
> >> -       }
> >> +                       sg_copy_to_buffer(data->sg, data->sg_len,
> >> +                                         idma->bounce_buf, xfer_bytes);
> >> +                       dma_wmb();
> >> +               }
> >> +       } else {
> >> +               int n_elem;
> >> +
> >> +               n_elem = dma_map_sg(mmc_dev(host->mmc),
> >> +                                   data->sg,
> >> +                                   data->sg_len,
> >> +                                   mmc_get_dma_dir(data));
> >>
> >> +               if (!n_elem) {
> >> +                       dev_err(mmc_dev(host->mmc), "dma_map_sg failed\n");
> >> +                       return -EINVAL;
> >> +               }
> >> +       }
> >>          return 0;
> >>   }
> >>
> >> @@ -112,8 +129,19 @@ static int sdmmc_idma_prep_data(struct mmci_host *host,
> >>   static void sdmmc_idma_unprep_data(struct mmci_host *host,
> >>                                     struct mmc_data *data, int err)
> >>   {
> >> -       dma_unmap_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
> >> -                    mmc_get_dma_dir(data));
> >> +       struct sdmmc_idma *idma = host->dma_priv;
> >> +
> >> +       if (idma->use_bounce_buffer) {
> >> +               if (data->flags & MMC_DATA_READ) {
> >> +                       unsigned int xfer_bytes = data->blksz * data->blocks;
> >> +
> >> +                       sg_copy_from_buffer(data->sg, data->sg_len,
> >> +                                           idma->bounce_buf, xfer_bytes);
> >> +               }
> >> +       } else {
> >> +               dma_unmap_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
> >> +                            mmc_get_dma_dir(data));
> >> +       }
> >>   }
> >>
> >>   static int sdmmc_idma_setup(struct mmci_host *host)
> >> @@ -137,6 +165,16 @@ static int sdmmc_idma_setup(struct mmci_host *host)
> >>                  host->mmc->max_segs = SDMMC_LLI_BUF_LEN /
> >>                          sizeof(struct sdmmc_lli_desc);
> >>                  host->mmc->max_seg_size = host->variant->stm32_idmabsize_mask;
> >> +
> >> +               host->mmc->max_req_size = SZ_1M;
> >> +               idma->bounce_buf = dmam_alloc_coherent(dev,
> >> +                                                      host->mmc->max_req_size,
> >> +                                                      &idma->bounce_dma_addr,
> >> +                                                      GFP_KERNEL);
> >> +               if (!idma->bounce_buf) {
> >> +                       dev_err(dev, "Unable to map allocate DMA bounce buffer.\n");
> >> +                       return -ENOMEM;
> >
> Hi Ulf,
>
> > If we fail to allocate the 1M bounce buffer, then we end up always
> > using a PIO based mode, right?
> >
> > Perhaps we can allow the above allocation to fail, but then limit us
> > to use DMA only when the buffers are properly aligned? Would that
> > work?
> >
> We have never supported PIO mode with STM32 variant.
> We only support DMA single buffer or DMA LLI.
> As we cannot have DMA LLI for unaligned access, we'll default to single
> mode.

Right, I was looking at the legacy variant, which uses PIO as
fallback. Sorry for my ignorance.

> If allocation fails, it then won't work.

Right, but that's only part of the issue, I think.

> Maybe we shouldn't fail here, and just check idma->bounce_buf in
> validate data function. If buffer is not allocated, we just return
> -EINVAL as it was done before.

Yes, something along those lines. However, there is another problem
too, which is that the allocation will be done for each instance of
the host that is probed. In all cases but the SDIO case, this would be
a waste, right?

Perhaps we should manage the allocation in the validate function too
(de-allocation should be handled at ->remove()). In this way, the
buffer will only be allocated when it's actually needed. Yes, it would
add a latency while serving the *first* request that has unaligned
buffers, but I guess we can live with that?

>
> Best regards,
> Yann

Kind regards
Uffe

>
> >> +               }
> >>          } else {
> >>                  host->mmc->max_segs = 1;
> >>                  host->mmc->max_seg_size = host->mmc->max_req_size;
> >> @@ -154,8 +192,16 @@ static int sdmmc_idma_start(struct mmci_host *host, unsigned int *datactrl)
> >>          struct scatterlist *sg;
> >>          int i;
> >>
> >> -       if (!host->variant->dma_lli || data->sg_len == 1) {
> >> -               writel_relaxed(sg_dma_address(data->sg),
> >> +       if (!host->variant->dma_lli || data->sg_len == 1 ||
> >> +           idma->use_bounce_buffer) {
> >> +               u32 dma_addr;
> >> +
> >> +               if (idma->use_bounce_buffer)
> >> +                       dma_addr = idma->bounce_dma_addr;
> >> +               else
> >> +                       dma_addr = sg_dma_address(data->sg);
> >> +
> >> +               writel_relaxed(dma_addr,
> >>                                 host->base + MMCI_STM32_IDMABASE0R);
> >>                  writel_relaxed(MMCI_STM32_IDMAEN,
> >>                                 host->base + MMCI_STM32_IDMACTRLR);
> >
> > Kind regards
> > Uffe
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] mmc: mmci: stm32: use a buffer for unaligned DMA requests
  2022-03-25 13:43       ` Ulf Hansson
@ 2022-03-25 14:01         ` Yann Gautier
  0 siblings, 0 replies; 8+ messages in thread
From: Yann Gautier @ 2022-03-25 14:01 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Christophe Kerello, Ludovic Barre, Maxime Coquelin,
	Alexandre Torgue, Philipp Zabel, Linus Walleij, linux-mmc,
	linux-stm32, linux-arm-kernel, linux-kernel

On 3/25/22 14:43, Ulf Hansson wrote:
> On Thu, 24 Mar 2022 at 17:23, Yann Gautier <yann.gautier@foss.st.com> wrote:
>>
>> On 3/24/22 12:55, Ulf Hansson wrote:
>>> On Thu, 17 Mar 2022 at 12:19, Yann Gautier <yann.gautier@foss.st.com> wrote:
>>>>
>>>> In SDIO mode, the sg list for requests can be unaligned with what the
>>>> STM32 SDMMC internal DMA can support. In that case, instead of failing,
>>>> use a temporary bounce buffer to copy from/to the sg list.
>>>> This buffer is limited to 1MB. But for that we need to also limit
>>>> max_req_size to 1MB. It has not shown any throughput penalties for
>>>> SD-cards or eMMC.
>>>>
>>>> Signed-off-by: Yann Gautier <yann.gautier@foss.st.com>
>>>> ---
>>>>    drivers/mmc/host/mmci_stm32_sdmmc.c | 80 +++++++++++++++++++++++------
>>>>    1 file changed, 63 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/host/mmci_stm32_sdmmc.c b/drivers/mmc/host/mmci_stm32_sdmmc.c
>>>> index 4566d7fc9055..a4414e32800f 100644
>>>> --- a/drivers/mmc/host/mmci_stm32_sdmmc.c
>>>> +++ b/drivers/mmc/host/mmci_stm32_sdmmc.c
>>>> @@ -43,6 +43,9 @@ struct sdmmc_lli_desc {
>>>>    struct sdmmc_idma {
>>>>           dma_addr_t sg_dma;
>>>>           void *sg_cpu;
>>>> +       dma_addr_t bounce_dma_addr;
>>>> +       void *bounce_buf;
>>>> +       bool use_bounce_buffer;
>>>>    };
>>>>
>>>>    struct sdmmc_dlyb {
>>>> @@ -54,6 +57,7 @@ struct sdmmc_dlyb {
>>>>    static int sdmmc_idma_validate_data(struct mmci_host *host,
>>>>                                       struct mmc_data *data)
>>>>    {
>>>> +       struct sdmmc_idma *idma = host->dma_priv;
>>>>           struct scatterlist *sg;
>>>>           int i;
>>>>
>>>> @@ -61,21 +65,23 @@ static int sdmmc_idma_validate_data(struct mmci_host *host,
>>>>            * idma has constraints on idmabase & idmasize for each element
>>>>            * excepted the last element which has no constraint on idmasize
>>>>            */
>>>> +       idma->use_bounce_buffer = false;
>>>>           for_each_sg(data->sg, sg, data->sg_len - 1, i) {
>>>>                   if (!IS_ALIGNED(sg->offset, sizeof(u32)) ||
>>>>                       !IS_ALIGNED(sg->length, SDMMC_IDMA_BURST)) {
>>>> -                       dev_err(mmc_dev(host->mmc),
>>>> +                       dev_dbg(mmc_dev(host->mmc),
>>>>                                   "unaligned scatterlist: ofst:%x length:%d\n",
>>>>                                   data->sg->offset, data->sg->length);
>>>> -                       return -EINVAL;
>>>> +                       idma->use_bounce_buffer = true;
>>>> +                       return 0;
>>>>                   }
>>>>           }
>>>>
>>>>           if (!IS_ALIGNED(sg->offset, sizeof(u32))) {
>>>> -               dev_err(mmc_dev(host->mmc),
>>>> +               dev_dbg(mmc_dev(host->mmc),
>>>>                           "unaligned last scatterlist: ofst:%x length:%d\n",
>>>>                           data->sg->offset, data->sg->length);
>>>> -               return -EINVAL;
>>>> +               idma->use_bounce_buffer = true;
>>>>           }
>>>>
>>>>           return 0;
>>>> @@ -84,18 +90,29 @@ static int sdmmc_idma_validate_data(struct mmci_host *host,
>>>>    static int _sdmmc_idma_prep_data(struct mmci_host *host,
>>>>                                    struct mmc_data *data)
>>>>    {
>>>> -       int n_elem;
>>>> +       struct sdmmc_idma *idma = host->dma_priv;
>>>>
>>>> -       n_elem = dma_map_sg(mmc_dev(host->mmc),
>>>> -                           data->sg,
>>>> -                           data->sg_len,
>>>> -                           mmc_get_dma_dir(data));
>>>> +       if (idma->use_bounce_buffer) {
>>>> +               if (data->flags & MMC_DATA_WRITE) {
>>>> +                       unsigned int xfer_bytes = data->blksz * data->blocks;
>>>>
>>>> -       if (!n_elem) {
>>>> -               dev_err(mmc_dev(host->mmc), "dma_map_sg failed\n");
>>>> -               return -EINVAL;
>>>> -       }
>>>> +                       sg_copy_to_buffer(data->sg, data->sg_len,
>>>> +                                         idma->bounce_buf, xfer_bytes);
>>>> +                       dma_wmb();
>>>> +               }
>>>> +       } else {
>>>> +               int n_elem;
>>>> +
>>>> +               n_elem = dma_map_sg(mmc_dev(host->mmc),
>>>> +                                   data->sg,
>>>> +                                   data->sg_len,
>>>> +                                   mmc_get_dma_dir(data));
>>>>
>>>> +               if (!n_elem) {
>>>> +                       dev_err(mmc_dev(host->mmc), "dma_map_sg failed\n");
>>>> +                       return -EINVAL;
>>>> +               }
>>>> +       }
>>>>           return 0;
>>>>    }
>>>>
>>>> @@ -112,8 +129,19 @@ static int sdmmc_idma_prep_data(struct mmci_host *host,
>>>>    static void sdmmc_idma_unprep_data(struct mmci_host *host,
>>>>                                      struct mmc_data *data, int err)
>>>>    {
>>>> -       dma_unmap_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
>>>> -                    mmc_get_dma_dir(data));
>>>> +       struct sdmmc_idma *idma = host->dma_priv;
>>>> +
>>>> +       if (idma->use_bounce_buffer) {
>>>> +               if (data->flags & MMC_DATA_READ) {
>>>> +                       unsigned int xfer_bytes = data->blksz * data->blocks;
>>>> +
>>>> +                       sg_copy_from_buffer(data->sg, data->sg_len,
>>>> +                                           idma->bounce_buf, xfer_bytes);
>>>> +               }
>>>> +       } else {
>>>> +               dma_unmap_sg(mmc_dev(host->mmc), data->sg, data->sg_len,
>>>> +                            mmc_get_dma_dir(data));
>>>> +       }
>>>>    }
>>>>
>>>>    static int sdmmc_idma_setup(struct mmci_host *host)
>>>> @@ -137,6 +165,16 @@ static int sdmmc_idma_setup(struct mmci_host *host)
>>>>                   host->mmc->max_segs = SDMMC_LLI_BUF_LEN /
>>>>                           sizeof(struct sdmmc_lli_desc);
>>>>                   host->mmc->max_seg_size = host->variant->stm32_idmabsize_mask;
>>>> +
>>>> +               host->mmc->max_req_size = SZ_1M;
>>>> +               idma->bounce_buf = dmam_alloc_coherent(dev,
>>>> +                                                      host->mmc->max_req_size,
>>>> +                                                      &idma->bounce_dma_addr,
>>>> +                                                      GFP_KERNEL);
>>>> +               if (!idma->bounce_buf) {
>>>> +                       dev_err(dev, "Unable to map allocate DMA bounce buffer.\n");
>>>> +                       return -ENOMEM;
>>>
>> Hi Ulf,
>>
>>> If we fail to allocate the 1M bounce buffer, then we end up always
>>> using a PIO based mode, right?
>>>
>>> Perhaps we can allow the above allocation to fail, but then limit us
>>> to use DMA only when the buffers are properly aligned? Would that
>>> work?
>>>
>> We have never supported PIO mode with STM32 variant.
>> We only support DMA single buffer or DMA LLI.
>> As we cannot have DMA LLI for unaligned access, we'll default to single
>> mode.
> 
> Right, I was looking at the legacy variant, which uses PIO as
> fallback. Sorry for my ignorance.
> 
>> If allocation fails, it then won't work.
> 
> Right, but that's only part of the issue, I think.
> 
>> Maybe we shouldn't fail here, and just check idma->bounce_buf in
>> validate data function. If buffer is not allocated, we just return
>> -EINVAL as it was done before.
> 
> Yes, something along those lines. However, there is another problem
> too, which is that the allocation will be done for each instance of
> the host that is probed. In all cases but the SDIO case, this would be
> a waste, right?
> 
> Perhaps we should manage the allocation in the validate function too
> (de-allocation should be handled at ->remove()). In this way, the
> buffer will only be allocated when it's actually needed. Yes, it would
> add a latency while serving the *first* request that has unaligned
> buffers, but I guess we can live with that?
> 
Hi Ulf,

That makes sense, I'll rework the validate data function with this.
I'll push a new version soon.

Thanks,
Yann
>>
>> Best regards,
>> Yann
> 
> Kind regards
> Uffe
> 
>>
>>>> +               }
>>>>           } else {
>>>>                   host->mmc->max_segs = 1;
>>>>                   host->mmc->max_seg_size = host->mmc->max_req_size;
>>>> @@ -154,8 +192,16 @@ static int sdmmc_idma_start(struct mmci_host *host, unsigned int *datactrl)
>>>>           struct scatterlist *sg;
>>>>           int i;
>>>>
>>>> -       if (!host->variant->dma_lli || data->sg_len == 1) {
>>>> -               writel_relaxed(sg_dma_address(data->sg),
>>>> +       if (!host->variant->dma_lli || data->sg_len == 1 ||
>>>> +           idma->use_bounce_buffer) {
>>>> +               u32 dma_addr;
>>>> +
>>>> +               if (idma->use_bounce_buffer)
>>>> +                       dma_addr = idma->bounce_dma_addr;
>>>> +               else
>>>> +                       dma_addr = sg_dma_address(data->sg);
>>>> +
>>>> +               writel_relaxed(dma_addr,
>>>>                                  host->base + MMCI_STM32_IDMABASE0R);
>>>>                   writel_relaxed(MMCI_STM32_IDMAEN,
>>>>                                  host->base + MMCI_STM32_IDMACTRLR);
>>>
>>> Kind regards
>>> Uffe
>>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] mmc: mmci: stm32: correctly check all elements of sg list
  2022-03-17 11:19 ` [PATCH 1/2] mmc: mmci: stm32: correctly check all elements of sg list Yann Gautier
@ 2022-03-28 14:04   ` Ulf Hansson
  0 siblings, 0 replies; 8+ messages in thread
From: Ulf Hansson @ 2022-03-28 14:04 UTC (permalink / raw)
  To: Yann Gautier
  Cc: Christophe Kerello, Ludovic Barre, Maxime Coquelin,
	Alexandre Torgue, Philipp Zabel, Linus Walleij, linux-mmc,
	linux-stm32, linux-arm-kernel, linux-kernel

On Thu, 17 Mar 2022 at 12:19, Yann Gautier <yann.gautier@foss.st.com> wrote:
>
> Use sg and not data->sg when checking sg list elements. Else only the
> first element alignment is checked.
> The last element should be checked the same way, for_each_sg already set
> sg to sg_next(sg).
>
> Fixes: 46b723dd867d ("mmc: mmci: add stm32 sdmmc variant")
>
> Signed-off-by: Yann Gautier <yann.gautier@foss.st.com>

This one, applied for fixes, thanks!

Kind regards
Uffe


> ---
>  drivers/mmc/host/mmci_stm32_sdmmc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/host/mmci_stm32_sdmmc.c b/drivers/mmc/host/mmci_stm32_sdmmc.c
> index 9c13f2c31365..4566d7fc9055 100644
> --- a/drivers/mmc/host/mmci_stm32_sdmmc.c
> +++ b/drivers/mmc/host/mmci_stm32_sdmmc.c
> @@ -62,8 +62,8 @@ static int sdmmc_idma_validate_data(struct mmci_host *host,
>          * excepted the last element which has no constraint on idmasize
>          */
>         for_each_sg(data->sg, sg, data->sg_len - 1, i) {
> -               if (!IS_ALIGNED(data->sg->offset, sizeof(u32)) ||
> -                   !IS_ALIGNED(data->sg->length, SDMMC_IDMA_BURST)) {
> +               if (!IS_ALIGNED(sg->offset, sizeof(u32)) ||
> +                   !IS_ALIGNED(sg->length, SDMMC_IDMA_BURST)) {
>                         dev_err(mmc_dev(host->mmc),
>                                 "unaligned scatterlist: ofst:%x length:%d\n",
>                                 data->sg->offset, data->sg->length);
> @@ -71,7 +71,7 @@ static int sdmmc_idma_validate_data(struct mmci_host *host,
>                 }
>         }
>
> -       if (!IS_ALIGNED(data->sg->offset, sizeof(u32))) {
> +       if (!IS_ALIGNED(sg->offset, sizeof(u32))) {
>                 dev_err(mmc_dev(host->mmc),
>                         "unaligned last scatterlist: ofst:%x length:%d\n",
>                         data->sg->offset, data->sg->length);
> --
> 2.25.1
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-03-28 14:05 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-17 11:19 [PATCH 0/2] mmc: mmci: stm32: manage unaligned DMA req for SDIO Yann Gautier
2022-03-17 11:19 ` [PATCH 1/2] mmc: mmci: stm32: correctly check all elements of sg list Yann Gautier
2022-03-28 14:04   ` Ulf Hansson
2022-03-17 11:19 ` [PATCH 2/2] mmc: mmci: stm32: use a buffer for unaligned DMA requests Yann Gautier
2022-03-24 11:55   ` Ulf Hansson
2022-03-24 16:23     ` Yann Gautier
2022-03-25 13:43       ` Ulf Hansson
2022-03-25 14:01         ` Yann Gautier

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).