[PATCH] mmc: meson-gx: check for scatterlist size alignment in block mode

[PATCH] mmc: meson-gx: check for scatterlist size alignment in block mode

[Dmitry Lebed]

Enable SGDMA support for SD_IO_RW_EXTENDED and add proper check
for scatterlist size alignment in block mode.

According to documentation, in SDIO block mode meson-gx DMA could
only handle buffers with sizes that are multiples of SDIO block size.

Some SDIO drivers like brcmfmac use scatterlist API, but do not enforce
proper scatterlist buffer size alignemnt, this looks like a root cause
of non-working CMD53.

Some minor style fixes.

Signed-off-by: Dmitry Lebed <lebed.dmitry@gmail.com>
---
 drivers/mmc/host/meson-gx-mmc.c | 37 ++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c
index 13f6a2c0ed04..eb6c02bc4a02 100644
--- a/drivers/mmc/host/meson-gx-mmc.c
+++ b/drivers/mmc/host/meson-gx-mmc.c
@@ -227,7 +227,6 @@ static void meson_mmc_get_transfer_mode(struct mmc_host *mmc,
 	struct mmc_data *data = mrq->data;
 	struct scatterlist *sg;
 	int i;
-	bool use_desc_chain_mode = true;
 
 	/*
 	 * When Controller DMA cannot directly access DDR memory, disable
@@ -237,25 +236,33 @@ static void meson_mmc_get_transfer_mode(struct mmc_host *mmc,
 	if (host->dram_access_quirk)
 		return;
 
-	/*
-	 * Broken SDIO with AP6255-based WiFi on Khadas VIM Pro has been
-	 * reported. For some strange reason this occurs in descriptor
-	 * chain mode only. So let's fall back to bounce buffer mode
-	 * for command SD_IO_RW_EXTENDED.
-	 */
-	if (mrq->cmd->opcode == SD_IO_RW_EXTENDED)
-		return;
+	if (data->blocks > 1) {
+		/*
+		 * In block mode DMA descriptor format, "length" field indicates
+		 * number of blocks and there is no way to pass DMA size that
+		 * is not multiple of SDIO block size, making it impossible to
+		 * tie more than one memory buffer with single SDIO block.
+		 * Block mode sg buffer size should be aligned with SDIO block
+		 * size, otherwise chain mode could not be used.
+		 */
+		for_each_sg(data->sg, sg, data->sg_len, i) {
+			if (sg->length % data->blksz) {
+				WARN_ONCE(1, "unaligned sg len %u blksize %u\n",
+					  sg->length, data->blksz);
+				return;
+			}
+		}
+	}
 
-	for_each_sg(data->sg, sg, data->sg_len, i)
+	for_each_sg(data->sg, sg, data->sg_len, i) {
 		/* check for 8 byte alignment */
-		if (sg->offset & 7) {
+		if (sg->offset % 8) {
 			WARN_ONCE(1, "unaligned scatterlist buffer\n");
-			use_desc_chain_mode = false;
-			break;
+			return;
 		}
+	}
 
-	if (use_desc_chain_mode)
-		data->host_cookie |= SD_EMMC_DESC_CHAIN_MODE;
+	data->host_cookie |= SD_EMMC_DESC_CHAIN_MODE;
 }
 
 static inline bool meson_mmc_desc_chain_mode(const struct mmc_data *data)
-- 
2.24.3 (Apple Git-128)


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH] mmc: meson-gx: check for scatterlist size alignment in block mode

[Heiner Kallweit]

Am 18.12.2020 um 08:53 schrieb Dmitry Lebed:
> Enable SGDMA support for SD_IO_RW_EXTENDED and add proper check
> for scatterlist size alignment in block mode.
> 
> According to documentation, in SDIO block mode meson-gx DMA could
> only handle buffers with sizes that are multiples of SDIO block size.
> 
> Some SDIO drivers like brcmfmac use scatterlist API, but do not enforce
> proper scatterlist buffer size alignemnt, this looks like a root cause
> of non-working CMD53.
> 
It's been too long ago that I worked on this to provide real feedback.
Just one comment: Your commit description sounds like there's a problem
in drivers like brcmfmac. Wouldn't it be better then to first fix these
drivers?

> Some minor style fixes.
> 
> Signed-off-by: Dmitry Lebed <lebed.dmitry@gmail.com>
> ---
>  drivers/mmc/host/meson-gx-mmc.c | 37 ++++++++++++++++++++-------------
>  1 file changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c
> index 13f6a2c0ed04..eb6c02bc4a02 100644
> --- a/drivers/mmc/host/meson-gx-mmc.c
> +++ b/drivers/mmc/host/meson-gx-mmc.c
> @@ -227,7 +227,6 @@ static void meson_mmc_get_transfer_mode(struct mmc_host *mmc,
>  	struct mmc_data *data = mrq->data;
>  	struct scatterlist *sg;
>  	int i;
> -	bool use_desc_chain_mode = true;
>  
>  	/*
>  	 * When Controller DMA cannot directly access DDR memory, disable
> @@ -237,25 +236,33 @@ static void meson_mmc_get_transfer_mode(struct mmc_host *mmc,
>  	if (host->dram_access_quirk)
>  		return;
>  
> -	/*
> -	 * Broken SDIO with AP6255-based WiFi on Khadas VIM Pro has been
> -	 * reported. For some strange reason this occurs in descriptor
> -	 * chain mode only. So let's fall back to bounce buffer mode
> -	 * for command SD_IO_RW_EXTENDED.
> -	 */
> -	if (mrq->cmd->opcode == SD_IO_RW_EXTENDED)
> -		return;
> +	if (data->blocks > 1) {
> +		/*
> +		 * In block mode DMA descriptor format, "length" field indicates
> +		 * number of blocks and there is no way to pass DMA size that
> +		 * is not multiple of SDIO block size, making it impossible to
> +		 * tie more than one memory buffer with single SDIO block.
> +		 * Block mode sg buffer size should be aligned with SDIO block
> +		 * size, otherwise chain mode could not be used.
> +		 */
> +		for_each_sg(data->sg, sg, data->sg_len, i) {
> +			if (sg->length % data->blksz) {
> +				WARN_ONCE(1, "unaligned sg len %u blksize %u\n",
> +					  sg->length, data->blksz);
> +				return;
> +			}
> +		}
> +	}
>  
> -	for_each_sg(data->sg, sg, data->sg_len, i)
> +	for_each_sg(data->sg, sg, data->sg_len, i) {
>  		/* check for 8 byte alignment */
> -		if (sg->offset & 7) {
> +		if (sg->offset % 8) {
>  			WARN_ONCE(1, "unaligned scatterlist buffer\n");
> -			use_desc_chain_mode = false;
> -			break;
> +			return;
>  		}
> +	}
>  
> -	if (use_desc_chain_mode)
> -		data->host_cookie |= SD_EMMC_DESC_CHAIN_MODE;
> +	data->host_cookie |= SD_EMMC_DESC_CHAIN_MODE;
>  }
>  
>  static inline bool meson_mmc_desc_chain_mode(const struct mmc_data *data)
> 


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH] mmc: meson-gx: check for scatterlist size alignment in block mode

[Jerome Brunet]

On Fri 18 Dec 2020 at 10:08, Heiner Kallweit <hkallweit1@gmail.com> wrote:

> Am 18.12.2020 um 08:53 schrieb Dmitry Lebed:
>> Enable SGDMA support for SD_IO_RW_EXTENDED and add proper check
>> for scatterlist size alignment in block mode.
>> 
>> According to documentation, in SDIO block mode meson-gx DMA could
>> only handle buffers with sizes that are multiples of SDIO block size.
>> 
>> Some SDIO drivers like brcmfmac use scatterlist API, but do not enforce
>> proper scatterlist buffer size alignemnt, this looks like a root cause
>> of non-working CMD53.
>> 
> It's been too long ago that I worked on this to provide real feedback.
> Just one comment: Your commit description sounds like there's a problem
> in drivers like brcmfmac. Wouldn't it be better then to first fix these
> drivers?

While this may be true, I think the proposed change is better than the
device/command specific work around we had so far.

>
>> Some minor style fixes.
>> 
>> Signed-off-by: Dmitry Lebed <lebed.dmitry@gmail.com>

Thx.

Reviewed-by: Jerome Brunet <jbrunet@baylibre.com>

>> ---
>>  drivers/mmc/host/meson-gx-mmc.c | 37 ++++++++++++++++++++-------------
>>  1 file changed, 22 insertions(+), 15 deletions(-)
>> 
>> diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c
>> index 13f6a2c0ed04..eb6c02bc4a02 100644
>> --- a/drivers/mmc/host/meson-gx-mmc.c
>> +++ b/drivers/mmc/host/meson-gx-mmc.c
>> @@ -227,7 +227,6 @@ static void meson_mmc_get_transfer_mode(struct mmc_host *mmc,
>>  	struct mmc_data *data = mrq->data;
>>  	struct scatterlist *sg;
>>  	int i;
>> -	bool use_desc_chain_mode = true;
>>  
>>  	/*
>>  	 * When Controller DMA cannot directly access DDR memory, disable
>> @@ -237,25 +236,33 @@ static void meson_mmc_get_transfer_mode(struct mmc_host *mmc,
>>  	if (host->dram_access_quirk)
>>  		return;
>>  
>> -	/*
>> -	 * Broken SDIO with AP6255-based WiFi on Khadas VIM Pro has been
>> -	 * reported. For some strange reason this occurs in descriptor
>> -	 * chain mode only. So let's fall back to bounce buffer mode
>> -	 * for command SD_IO_RW_EXTENDED.
>> -	 */
>> -	if (mrq->cmd->opcode == SD_IO_RW_EXTENDED)
>> -		return;
>> +	if (data->blocks > 1) {
>> +		/*
>> +		 * In block mode DMA descriptor format, "length" field indicates
>> +		 * number of blocks and there is no way to pass DMA size that
>> +		 * is not multiple of SDIO block size, making it impossible to
>> +		 * tie more than one memory buffer with single SDIO block.
>> +		 * Block mode sg buffer size should be aligned with SDIO block
>> +		 * size, otherwise chain mode could not be used.
>> +		 */
>> +		for_each_sg(data->sg, sg, data->sg_len, i) {
>> +			if (sg->length % data->blksz) {
>> +				WARN_ONCE(1, "unaligned sg len %u blksize %u\n",
>> +					  sg->length, data->blksz);
>> +				return;
>> +			}
>> +		}
>> +	}
>>  
>> -	for_each_sg(data->sg, sg, data->sg_len, i)
>> +	for_each_sg(data->sg, sg, data->sg_len, i) {
>>  		/* check for 8 byte alignment */
>> -		if (sg->offset & 7) {
>> +		if (sg->offset % 8) {
>>  			WARN_ONCE(1, "unaligned scatterlist buffer\n");
>> -			use_desc_chain_mode = false;
>> -			break;
>> +			return;
>>  		}
>> +	}
>>  
>> -	if (use_desc_chain_mode)
>> -		data->host_cookie |= SD_EMMC_DESC_CHAIN_MODE;
>> +	data->host_cookie |= SD_EMMC_DESC_CHAIN_MODE;
>>  }
>>  
>>  static inline bool meson_mmc_desc_chain_mode(const struct mmc_data *data)
>> 
>
>
> _______________________________________________
> linux-amlogic mailing list
> linux-amlogic@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-amlogic


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH] mmc: meson-gx: check for scatterlist size alignment in block mode

[Ulf Hansson]

On Fri, 18 Dec 2020 at 08:54, Dmitry Lebed <lebed.dmitry@gmail.com> wrote:
>
> Enable SGDMA support for SD_IO_RW_EXTENDED and add proper check
> for scatterlist size alignment in block mode.
>
> According to documentation, in SDIO block mode meson-gx DMA could
> only handle buffers with sizes that are multiples of SDIO block size.
>
> Some SDIO drivers like brcmfmac use scatterlist API, but do not enforce
> proper scatterlist buffer size alignemnt, this looks like a root cause
> of non-working CMD53.
>
> Some minor style fixes.
>
> Signed-off-by: Dmitry Lebed <lebed.dmitry@gmail.com>

Applied for next, thanks!

Kind regards
Uffe


> ---
>  drivers/mmc/host/meson-gx-mmc.c | 37 ++++++++++++++++++++-------------
>  1 file changed, 22 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c
> index 13f6a2c0ed04..eb6c02bc4a02 100644
> --- a/drivers/mmc/host/meson-gx-mmc.c
> +++ b/drivers/mmc/host/meson-gx-mmc.c
> @@ -227,7 +227,6 @@ static void meson_mmc_get_transfer_mode(struct mmc_host *mmc,
>         struct mmc_data *data = mrq->data;
>         struct scatterlist *sg;
>         int i;
> -       bool use_desc_chain_mode = true;
>
>         /*
>          * When Controller DMA cannot directly access DDR memory, disable
> @@ -237,25 +236,33 @@ static void meson_mmc_get_transfer_mode(struct mmc_host *mmc,
>         if (host->dram_access_quirk)
>                 return;
>
> -       /*
> -        * Broken SDIO with AP6255-based WiFi on Khadas VIM Pro has been
> -        * reported. For some strange reason this occurs in descriptor
> -        * chain mode only. So let's fall back to bounce buffer mode
> -        * for command SD_IO_RW_EXTENDED.
> -        */
> -       if (mrq->cmd->opcode == SD_IO_RW_EXTENDED)
> -               return;
> +       if (data->blocks > 1) {
> +               /*
> +                * In block mode DMA descriptor format, "length" field indicates
> +                * number of blocks and there is no way to pass DMA size that
> +                * is not multiple of SDIO block size, making it impossible to
> +                * tie more than one memory buffer with single SDIO block.
> +                * Block mode sg buffer size should be aligned with SDIO block
> +                * size, otherwise chain mode could not be used.
> +                */
> +               for_each_sg(data->sg, sg, data->sg_len, i) {
> +                       if (sg->length % data->blksz) {
> +                               WARN_ONCE(1, "unaligned sg len %u blksize %u\n",
> +                                         sg->length, data->blksz);
> +                               return;
> +                       }
> +               }
> +       }
>
> -       for_each_sg(data->sg, sg, data->sg_len, i)
> +       for_each_sg(data->sg, sg, data->sg_len, i) {
>                 /* check for 8 byte alignment */
> -               if (sg->offset & 7) {
> +               if (sg->offset % 8) {
>                         WARN_ONCE(1, "unaligned scatterlist buffer\n");
> -                       use_desc_chain_mode = false;
> -                       break;
> +                       return;
>                 }
> +       }
>
> -       if (use_desc_chain_mode)
> -               data->host_cookie |= SD_EMMC_DESC_CHAIN_MODE;
> +       data->host_cookie |= SD_EMMC_DESC_CHAIN_MODE;
>  }
>
>  static inline bool meson_mmc_desc_chain_mode(const struct mmc_data *data)
> --
> 2.24.3 (Apple Git-128)
>

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic