[RFC PATCH 0/2] mmc: sdhci: Add SDHCI_QUIRK2_BROKEN_SDMA_BOUNDARY_BUFFER

[RFC PATCH 0/2] mmc: sdhci: Add SDHCI_QUIRK2_BROKEN_SDMA_BOUNDARY_BUFFER

[srinivas.kandagatla]

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

This patchset adds quirk to support cards which have issues when sdma
boundary buffer bits are programmed in Block Size Register (0x04)
when using ADMA.

First patch adds quirk and second one uses that quirk in msm sdhci driver.

Tested on DB410c with WLAN SDIO card.

thanks,
srini

Srinivas Kandagatla (2):
  mmc: sdhci: add quirk SDHCI_QUIRK2_BROKEN_SDMA_BOUNDARY_BUFFER
  mmc: sdhci-msm: enable SDHCI_QUIRK2_BROKEN_SDMA_BOUNDARY_BUFFER

 drivers/mmc/host/sdhci-msm.c |  3 ++-
 drivers/mmc/host/sdhci.c     | 24 ++++++++++++++++++------
 drivers/mmc/host/sdhci.h     |  2 ++
 3 files changed, 22 insertions(+), 7 deletions(-)

-- 
2.11.0

[RFC PATCH 1/2] mmc: sdhci: add quirk SDHCI_QUIRK2_BROKEN_SDMA_BOUNDARY_BUFFER

[srinivas.kandagatla]

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

This patch adds quirk to sdhci controllers which are broken when
HOST SDMA Buffer Boundary is programmed in Block Size Register (0x04)
when using ADMA. Qualcomm sdhci controller is one of such type, writing
to this bits is un-supported.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/mmc/host/sdhci.c | 24 ++++++++++++++++++------
 drivers/mmc/host/sdhci.h |  2 ++
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index ecd0d4350e8a..d68ff1955761 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -765,6 +765,20 @@ static void sdhci_set_timeout(struct sdhci_host *host, struct mmc_command *cmd)
 	}
 }
 
+static void sdhci_set_blk_size_reg(struct sdhci_host *host,
+				   unsigned int sdma_boundary,
+				   unsigned int blksz)
+{
+	if ((host->quirks2 & SDHCI_QUIRK2_BROKEN_SDMA_BOUNDARY_BUFFER) &&
+			(host->flags & SDHCI_USE_ADMA)) {
+		sdhci_writew(host, SDHCI_MAKE_BLKSZ(0, blksz),
+			     SDHCI_BLOCK_SIZE);
+	} else {
+		sdhci_writew(host, SDHCI_MAKE_BLKSZ(sdma_boundary, blksz),
+			     SDHCI_BLOCK_SIZE);
+	}
+}
+
 static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
 {
 	u8 ctrl;
@@ -897,8 +911,7 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
 	sdhci_set_transfer_irqs(host);
 
 	/* Set the DMA boundary value and block size */
-	sdhci_writew(host, SDHCI_MAKE_BLKSZ(SDHCI_DEFAULT_BOUNDARY_ARG,
-		data->blksz), SDHCI_BLOCK_SIZE);
+	sdhci_set_blk_size_reg(host, SDHCI_DEFAULT_BOUNDARY_ARG, data->blksz);
 	sdhci_writew(host, data->blocks, SDHCI_BLOCK_COUNT);
 }
 
@@ -2052,9 +2065,9 @@ static void sdhci_send_tuning(struct sdhci_host *host, u32 opcode)
 	 */
 	if (cmd.opcode == MMC_SEND_TUNING_BLOCK_HS200 &&
 	    mmc->ios.bus_width == MMC_BUS_WIDTH_8)
-		sdhci_writew(host, SDHCI_MAKE_BLKSZ(7, 128), SDHCI_BLOCK_SIZE);
+		sdhci_set_blk_size_reg(host, SDHCI_DEFAULT_BOUNDARY_ARG, 128);
 	else
-		sdhci_writew(host, SDHCI_MAKE_BLKSZ(7, 64), SDHCI_BLOCK_SIZE);
+		sdhci_set_blk_size_reg(host, SDHCI_DEFAULT_BOUNDARY_ARG, 64);
 
 	/*
 	 * The tuning block is sent by the card to the host controller.
@@ -2998,8 +3011,7 @@ void sdhci_cqe_enable(struct mmc_host *mmc)
 		ctrl |= SDHCI_CTRL_ADMA32;
 	sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
 
-	sdhci_writew(host, SDHCI_MAKE_BLKSZ(SDHCI_DEFAULT_BOUNDARY_ARG, 512),
-		     SDHCI_BLOCK_SIZE);
+	sdhci_set_blk_size_reg(host, SDHCI_DEFAULT_BOUNDARY_ARG, 512);
 
 	/* Set maximum timeout */
 	sdhci_writeb(host, 0xE, SDHCI_TIMEOUT_CONTROL);
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 0469fa191493..9a1343509bb5 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -435,6 +435,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK2_ACMD23_BROKEN			(1<<14)
 /* Broken Clock divider zero in controller */
 #define SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN		(1<<15)
+/* Controller doesn't support sdma boundray buffer setup when using ADMA */
+#define SDHCI_QUIRK2_BROKEN_SDMA_BOUNDARY_BUFFER	(1<<16)
 
 	int irq;		/* Device IRQ */
 	void __iomem *ioaddr;	/* Mapped address */
-- 
2.11.0

[RFC PATCH 2/2] mmc: sdhci-msm: enable SDHCI_QUIRK2_BROKEN_SDMA_BOUNDARY_BUFFER

[srinivas.kandagatla]

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

Programming legacy HOST SDMA Buffer Boundary bits in Block Size Register
(0x04) is not supported in Qualcomm sdhci controllers. Writing to this
would cause the controller not to transfer last block in case block size
is 4 bytes or less.

This issue was noticed while testing sdio wlan card on Qcom DB410c board.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/mmc/host/sdhci-msm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 9d601dc0d646..50f650301ae6 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -1114,7 +1114,8 @@ static const struct sdhci_pltfm_data sdhci_msm_pdata = {
 		  SDHCI_QUIRK_NO_CARD_NO_RESET |
 		  SDHCI_QUIRK_SINGLE_POWER_WRITE |
 		  SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
-	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
+	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
+		   SDHCI_QUIRK2_BROKEN_SDMA_BOUNDARY_BUFFER,
 	.ops = &sdhci_msm_ops,
 };
 
-- 
2.11.0

Re: [RFC PATCH 0/2] mmc: sdhci: Add SDHCI_QUIRK2_BROKEN_SDMA_BOUNDARY_BUFFER

[Ritesh Harjani]

Hi Srinivas,

On 6/28/2017 7:05 PM, srinivas.kandagatla@linaro.org wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> 
> This patchset adds quirk to support cards which have issues when sdma
> boundary buffer bits are programmed in Block Size Register (0x04)
> when using ADMA.

Thanks for pointing out the reason of failure without this patch.
Earlier I could not find the reason.

Previous discussion link :-
https://patchwork.kernel.org/patch/9200579/


> 
> First patch adds quirk and second one uses that quirk in msm sdhci driver.

Not sure if quirk will be the right way to go about this, or whether we 
should make this functionality default since ADMA does not
uses this (as per spec) ?
Since other systems should not break (as Adrian was mentioning in the 
discussion link above).

Adrian/Ulf will know better on this.

> 
> Tested on DB410c with WLAN SDIO card.
> 
> thanks,
> srini
> 
> Srinivas Kandagatla (2):
>    mmc: sdhci: add quirk SDHCI_QUIRK2_BROKEN_SDMA_BOUNDARY_BUFFER
>    mmc: sdhci-msm: enable SDHCI_QUIRK2_BROKEN_SDMA_BOUNDARY_BUFFER
> 
>   drivers/mmc/host/sdhci-msm.c |  3 ++-
>   drivers/mmc/host/sdhci.c     | 24 ++++++++++++++++++------
>   drivers/mmc/host/sdhci.h     |  2 ++
>   3 files changed, 22 insertions(+), 7 deletions(-)
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RFC PATCH 0/2] mmc: sdhci: Add SDHCI_QUIRK2_BROKEN_SDMA_BOUNDARY_BUFFER

[Srinivas Kandagatla]

On 28/06/17 18:21, Ritesh Harjani wrote:
> Hi Srinivas,
> 
> On 6/28/2017 7:05 PM, srinivas.kandagatla@linaro.org wrote:
>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>
>> This patchset adds quirk to support cards which have issues when sdma
>> boundary buffer bits are programmed in Block Size Register (0x04)
>> when using ADMA.
> 
> Thanks for pointing out the reason of failure without this patch.
> Earlier I could not find the reason.
> 
> Previous discussion link :-
> https://patchwork.kernel.org/patch/9200579/

I did see this patch, only issue I had is making it default.

> 
> 
>>
>> First patch adds quirk and second one uses that quirk in msm sdhci 
>> driver.
> 
> Not sure if quirk will be the right way to go about this, or whether we 
> should make this functionality default since ADMA does not
> uses this (as per spec) ?
> Since other systems should not break (as Adrian was mentioning in the 
> discussion link above).
> 
That was the only reason to add this as quirk, so that other drivers 
would work as it previously.

Also, this seems to be a issue with only qcom controller, so quirk makes 
more sense for me.

> Adrian/Ulf will know better on this.
> 

--srini
>>
>> Tested on DB410c with WLAN SDIO card.
>>
>> thanks,
>> srini
>>
>> Srinivas Kandagatla (2):
>>    mmc: sdhci: add quirk SDHCI_QUIRK2_BROKEN_SDMA_BOUNDARY_BUFFER
>>    mmc: sdhci-msm: enable SDHCI_QUIRK2_BROKEN_SDMA_BOUNDARY_BUFFER
>>
>>   drivers/mmc/host/sdhci-msm.c |  3 ++-
>>   drivers/mmc/host/sdhci.c     | 24 ++++++++++++++++++------
>>   drivers/mmc/host/sdhci.h     |  2 ++
>>   3 files changed, 22 insertions(+), 7 deletions(-)
>>
> 

Re: [RFC PATCH 1/2] mmc: sdhci: add quirk SDHCI_QUIRK2_BROKEN_SDMA_BOUNDARY_BUFFER

[Ulf Hansson]

On 28 June 2017 at 15:35,  <srinivas.kandagatla@linaro.org> wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>
> This patch adds quirk to sdhci controllers which are broken when
> HOST SDMA Buffer Boundary is programmed in Block Size Register (0x04)
> when using ADMA. Qualcomm sdhci controller is one of such type, writing
> to this bits is un-supported.
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  drivers/mmc/host/sdhci.c | 24 ++++++++++++++++++------
>  drivers/mmc/host/sdhci.h |  2 ++
>  2 files changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index ecd0d4350e8a..d68ff1955761 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -765,6 +765,20 @@ static void sdhci_set_timeout(struct sdhci_host *host, struct mmc_command *cmd)
>         }
>  }
>
> +static void sdhci_set_blk_size_reg(struct sdhci_host *host,
> +                                  unsigned int sdma_boundary,
> +                                  unsigned int blksz)
> +{
> +       if ((host->quirks2 & SDHCI_QUIRK2_BROKEN_SDMA_BOUNDARY_BUFFER) &&
> +                       (host->flags & SDHCI_USE_ADMA)) {
> +               sdhci_writew(host, SDHCI_MAKE_BLKSZ(0, blksz),
> +                            SDHCI_BLOCK_SIZE);
> +       } else {
> +               sdhci_writew(host, SDHCI_MAKE_BLKSZ(sdma_boundary, blksz),
> +                            SDHCI_BLOCK_SIZE);
> +       }
> +}
> +
>  static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
>  {
>         u8 ctrl;
> @@ -897,8 +911,7 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
>         sdhci_set_transfer_irqs(host);
>
>         /* Set the DMA boundary value and block size */
> -       sdhci_writew(host, SDHCI_MAKE_BLKSZ(SDHCI_DEFAULT_BOUNDARY_ARG,
> -               data->blksz), SDHCI_BLOCK_SIZE);
> +       sdhci_set_blk_size_reg(host, SDHCI_DEFAULT_BOUNDARY_ARG, data->blksz);
>         sdhci_writew(host, data->blocks, SDHCI_BLOCK_COUNT);
>  }
>
> @@ -2052,9 +2065,9 @@ static void sdhci_send_tuning(struct sdhci_host *host, u32 opcode)
>          */
>         if (cmd.opcode == MMC_SEND_TUNING_BLOCK_HS200 &&
>             mmc->ios.bus_width == MMC_BUS_WIDTH_8)
> -               sdhci_writew(host, SDHCI_MAKE_BLKSZ(7, 128), SDHCI_BLOCK_SIZE);
> +               sdhci_set_blk_size_reg(host, SDHCI_DEFAULT_BOUNDARY_ARG, 128);
>         else
> -               sdhci_writew(host, SDHCI_MAKE_BLKSZ(7, 64), SDHCI_BLOCK_SIZE);
> +               sdhci_set_blk_size_reg(host, SDHCI_DEFAULT_BOUNDARY_ARG, 64);
>
>         /*
>          * The tuning block is sent by the card to the host controller.
> @@ -2998,8 +3011,7 @@ void sdhci_cqe_enable(struct mmc_host *mmc)
>                 ctrl |= SDHCI_CTRL_ADMA32;
>         sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
>
> -       sdhci_writew(host, SDHCI_MAKE_BLKSZ(SDHCI_DEFAULT_BOUNDARY_ARG, 512),
> -                    SDHCI_BLOCK_SIZE);
> +       sdhci_set_blk_size_reg(host, SDHCI_DEFAULT_BOUNDARY_ARG, 512);
>
>         /* Set maximum timeout */
>         sdhci_writeb(host, 0xE, SDHCI_TIMEOUT_CONTROL);
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 0469fa191493..9a1343509bb5 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -435,6 +435,8 @@ struct sdhci_host {
>  #define SDHCI_QUIRK2_ACMD23_BROKEN                     (1<<14)
>  /* Broken Clock divider zero in controller */
>  #define SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN             (1<<15)
> +/* Controller doesn't support sdma boundray buffer setup when using ADMA */
> +#define SDHCI_QUIRK2_BROKEN_SDMA_BOUNDARY_BUFFER       (1<<16)
>
>         int irq;                /* Device IRQ */
>         void __iomem *ioaddr;   /* Mapped address */
> --
> 2.11.0
>

This change seems like a reasonable justification for adding a new
SDHCI quirk, even if we in general wants to avoid that.

Adrian?

Kind regards
Uffe

Re: [RFC PATCH 1/2] mmc: sdhci: add quirk SDHCI_QUIRK2_BROKEN_SDMA_BOUNDARY_BUFFER

[Adrian Hunter]

On 11/07/17 16:49, Ulf Hansson wrote:
> On 28 June 2017 at 15:35,  <srinivas.kandagatla@linaro.org> wrote:
>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>
>> This patch adds quirk to sdhci controllers which are broken when
>> HOST SDMA Buffer Boundary is programmed in Block Size Register (0x04)
>> when using ADMA. Qualcomm sdhci controller is one of such type, writing
>> to this bits is un-supported.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>  drivers/mmc/host/sdhci.c | 24 ++++++++++++++++++------
>>  drivers/mmc/host/sdhci.h |  2 ++
>>  2 files changed, 20 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index ecd0d4350e8a..d68ff1955761 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -765,6 +765,20 @@ static void sdhci_set_timeout(struct sdhci_host *host, struct mmc_command *cmd)
>>         }
>>  }
>>
>> +static void sdhci_set_blk_size_reg(struct sdhci_host *host,
>> +                                  unsigned int sdma_boundary,
>> +                                  unsigned int blksz)
>> +{
>> +       if ((host->quirks2 & SDHCI_QUIRK2_BROKEN_SDMA_BOUNDARY_BUFFER) &&
>> +                       (host->flags & SDHCI_USE_ADMA)) {
>> +               sdhci_writew(host, SDHCI_MAKE_BLKSZ(0, blksz),
>> +                            SDHCI_BLOCK_SIZE);
>> +       } else {
>> +               sdhci_writew(host, SDHCI_MAKE_BLKSZ(sdma_boundary, blksz),
>> +                            SDHCI_BLOCK_SIZE);
>> +       }
>> +}
>> +
>>  static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
>>  {
>>         u8 ctrl;
>> @@ -897,8 +911,7 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd)
>>         sdhci_set_transfer_irqs(host);
>>
>>         /* Set the DMA boundary value and block size */
>> -       sdhci_writew(host, SDHCI_MAKE_BLKSZ(SDHCI_DEFAULT_BOUNDARY_ARG,
>> -               data->blksz), SDHCI_BLOCK_SIZE);
>> +       sdhci_set_blk_size_reg(host, SDHCI_DEFAULT_BOUNDARY_ARG, data->blksz);
>>         sdhci_writew(host, data->blocks, SDHCI_BLOCK_COUNT);
>>  }
>>
>> @@ -2052,9 +2065,9 @@ static void sdhci_send_tuning(struct sdhci_host *host, u32 opcode)
>>          */
>>         if (cmd.opcode == MMC_SEND_TUNING_BLOCK_HS200 &&
>>             mmc->ios.bus_width == MMC_BUS_WIDTH_8)
>> -               sdhci_writew(host, SDHCI_MAKE_BLKSZ(7, 128), SDHCI_BLOCK_SIZE);
>> +               sdhci_set_blk_size_reg(host, SDHCI_DEFAULT_BOUNDARY_ARG, 128);
>>         else
>> -               sdhci_writew(host, SDHCI_MAKE_BLKSZ(7, 64), SDHCI_BLOCK_SIZE);
>> +               sdhci_set_blk_size_reg(host, SDHCI_DEFAULT_BOUNDARY_ARG, 64);
>>
>>         /*
>>          * The tuning block is sent by the card to the host controller.
>> @@ -2998,8 +3011,7 @@ void sdhci_cqe_enable(struct mmc_host *mmc)
>>                 ctrl |= SDHCI_CTRL_ADMA32;
>>         sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
>>
>> -       sdhci_writew(host, SDHCI_MAKE_BLKSZ(SDHCI_DEFAULT_BOUNDARY_ARG, 512),
>> -                    SDHCI_BLOCK_SIZE);
>> +       sdhci_set_blk_size_reg(host, SDHCI_DEFAULT_BOUNDARY_ARG, 512);
>>
>>         /* Set maximum timeout */
>>         sdhci_writeb(host, 0xE, SDHCI_TIMEOUT_CONTROL);
>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>> index 0469fa191493..9a1343509bb5 100644
>> --- a/drivers/mmc/host/sdhci.h
>> +++ b/drivers/mmc/host/sdhci.h
>> @@ -435,6 +435,8 @@ struct sdhci_host {
>>  #define SDHCI_QUIRK2_ACMD23_BROKEN                     (1<<14)
>>  /* Broken Clock divider zero in controller */
>>  #define SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN             (1<<15)
>> +/* Controller doesn't support sdma boundray buffer setup when using ADMA */
>> +#define SDHCI_QUIRK2_BROKEN_SDMA_BOUNDARY_BUFFER       (1<<16)
>>
>>         int irq;                /* Device IRQ */
>>         void __iomem *ioaddr;   /* Mapped address */
>> --
>> 2.11.0
>>
> 
> This change seems like a reasonable justification for adding a new
> SDHCI quirk, even if we in general wants to avoid that.
> 
> Adrian?

If we add a quirk for every register we end up with a very ugly version of
CONFIG_MMC_SDHCI_IO_ACCESSORS.  So CONFIG_MMC_SDHCI_IO_ACCESSORS is a better
option.  Another possibility is to add a member to struct sdhci_host,
initialized to SDHCI_DEFAULT_BOUNDARY_ARG, and write that.  Drivers could
then change the value if necessary.

Re: [RFC PATCH 1/2] mmc: sdhci: add quirk SDHCI_QUIRK2_BROKEN_SDMA_BOUNDARY_BUFFER

[Srinivas Kandagatla]

On 17/07/17 13:47, Adrian Hunter wrote:
>> This change seems like a reasonable justification for adding a new
>> SDHCI quirk, even if we in general wants to avoid that.
>>
>> Adrian?
> If we add a quirk for every register we end up with a very ugly version of
> CONFIG_MMC_SDHCI_IO_ACCESSORS.  So CONFIG_MMC_SDHCI_IO_ACCESSORS is a better
> option.  Another possibility is to add a member to struct sdhci_host,
> initialized to SDHCI_DEFAULT_BOUNDARY_ARG, and write that.  Drivers could
> then change the value if necessary.
Second option seems to be better one. I will give it a try and see!

thanks,
srini