[PATCH 0/3] Fix issues with command queuing in arasan controllers

[PATCH 0/3] Fix issues with command queuing in arasan controllers

[Faiz Abbas]

In some Arasan SDHCI controllers, after tuning, the tuning pattern data
is leftover in the sdhci buffer. This leads to issues with future data
commands, especially when command queuing is enabled. The following
patches help fix this issue by resetting data lines after tuning is
finished. The first two patches have been tested with TI's am65x and
j721e SoCs using the sdhci_am654 driver.

I have a strong suspicion that this is the same issue with
the sdhci-of-arasan driver where they are forced to dump data from the
buffer before enabling command queuing. I need help from someone with a
compatible platform to test this.

Faiz Abbas (3):
  mmc: sdhci: Add Quirk to reset data lines after tuning
  mmc: sdhci_am654: Enable Quirk to reset data after tuning
  mmc: sdhci-of-arasan: Fix Command Queuing enable handling

 drivers/mmc/host/sdhci-of-arasan.c | 21 ++++-----------------
 drivers/mmc/host/sdhci.c           |  3 +++
 drivers/mmc/host/sdhci.h           |  4 ++++
 drivers/mmc/host/sdhci_am654.c     |  9 ++++++---
 4 files changed, 17 insertions(+), 20 deletions(-)

-- 
2.19.2

[PATCH 1/3] mmc: sdhci: Add Quirk to reset data lines after tuning

[Faiz Abbas]

Some arasan controllers have data leftover in the buffer after tuning
procedure is complete which interferes with future data commands. Add a
quirk to reset the data after tuning is finished.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 drivers/mmc/host/sdhci.c | 3 +++
 drivers/mmc/host/sdhci.h | 4 ++++
 2 files changed, 7 insertions(+)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 1b1c26da3fe0..e4b478efb560 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2316,6 +2316,9 @@ EXPORT_SYMBOL_GPL(sdhci_start_tuning);
 
 void sdhci_end_tuning(struct sdhci_host *host)
 {
+	if (host->quirks2 & SDHCI_QUIRK2_RESET_DATA_POST_TUNING)
+		sdhci_reset(host, SDHCI_RESET_DATA);
+
 	sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
 	sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE);
 }
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index fe83ece6965b..28826124f7c3 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -484,6 +484,10 @@ struct sdhci_host {
  * block count.
  */
 #define SDHCI_QUIRK2_USE_32BIT_BLK_CNT			(1<<18)
+/*
+ * Controller needs to reset data lines once tuning is complete
+ */
+#define SDHCI_QUIRK2_RESET_DATA_POST_TUNING		(1<<19)
 
 	int irq;		/* Device IRQ */
 	void __iomem *ioaddr;	/* Mapped address */
-- 
2.19.2

[PATCH 2/3] mmc: sdhci_am654: Enable Quirk to reset data after tuning

[Faiz Abbas]

Enable SDHCI_QUIRK2_RESET_DATA_POST_TUNING for all controller variants.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 drivers/mmc/host/sdhci_am654.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
index b8e897e31e2e..1ac1caa2bd0c 100644
--- a/drivers/mmc/host/sdhci_am654.c
+++ b/drivers/mmc/host/sdhci_am654.c
@@ -255,7 +255,8 @@ static const struct sdhci_pltfm_data sdhci_am654_pdata = {
 	.ops = &sdhci_am654_ops,
 	.quirks = SDHCI_QUIRK_INVERTED_WRITE_PROTECT |
 		  SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12,
-	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
+	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
+		   SDHCI_QUIRK2_RESET_DATA_POST_TUNING,
 };
 
 static const struct sdhci_am654_driver_data sdhci_am654_drvdata = {
@@ -292,7 +293,8 @@ static const struct sdhci_pltfm_data sdhci_j721e_8bit_pdata = {
 	.ops = &sdhci_j721e_8bit_ops,
 	.quirks = SDHCI_QUIRK_INVERTED_WRITE_PROTECT |
 		  SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12,
-	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
+	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
+		   SDHCI_QUIRK2_RESET_DATA_POST_TUNING,
 };
 
 static const struct sdhci_am654_driver_data sdhci_j721e_8bit_drvdata = {
@@ -316,7 +318,8 @@ static const struct sdhci_pltfm_data sdhci_j721e_4bit_pdata = {
 	.ops = &sdhci_j721e_4bit_ops,
 	.quirks = SDHCI_QUIRK_INVERTED_WRITE_PROTECT |
 		  SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12,
-	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
+	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
+		   SDHCI_QUIRK2_RESET_DATA_POST_TUNING,
 };
 
 static const struct sdhci_am654_driver_data sdhci_j721e_4bit_drvdata = {
-- 
2.19.2

[RFT PATCH 3/3] mmc: sdhci-of-arasan: Fix Command Queuing enable handling

[Faiz Abbas]

There is a need to dump data from the buffer before enabling command
queuing because of leftover data from tuning. Reset the data lines to
fix this at the source.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 drivers/mmc/host/sdhci-of-arasan.c | 21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
index e49b44b4d82e..1495ae72b902 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -376,22 +376,8 @@ static void sdhci_arasan_dumpregs(struct mmc_host *mmc)
 	sdhci_dumpregs(mmc_priv(mmc));
 }
 
-static void sdhci_arasan_cqe_enable(struct mmc_host *mmc)
-{
-	struct sdhci_host *host = mmc_priv(mmc);
-	u32 reg;
-
-	reg = sdhci_readl(host, SDHCI_PRESENT_STATE);
-	while (reg & SDHCI_DATA_AVAILABLE) {
-		sdhci_readl(host, SDHCI_BUFFER);
-		reg = sdhci_readl(host, SDHCI_PRESENT_STATE);
-	}
-
-	sdhci_cqe_enable(mmc);
-}
-
 static const struct cqhci_host_ops sdhci_arasan_cqhci_ops = {
-	.enable         = sdhci_arasan_cqe_enable,
+	.enable         = sdhci_cqe_enable,
 	.disable        = sdhci_cqe_disable,
 	.dumpregs       = sdhci_arasan_dumpregs,
 };
@@ -410,8 +396,9 @@ static const struct sdhci_ops sdhci_arasan_cqe_ops = {
 static const struct sdhci_pltfm_data sdhci_arasan_cqe_pdata = {
 	.ops = &sdhci_arasan_cqe_ops,
 	.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
-	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
-			SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN,
+	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN	|
+		   SDHCI_QUIRK2_RESET_DATA_POST_TUNING	|
+		   SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN,
 };
 
 static struct sdhci_arasan_of_data sdhci_arasan_rk3399_data = {
-- 
2.19.2

Re: [RFT PATCH 3/3] mmc: sdhci-of-arasan: Fix Command Queuing enable handling

[Shawn Lin]

On 2019/12/30 17:23, Faiz Abbas wrote:
> There is a need to dump data from the buffer before enabling command
> queuing because of leftover data from tuning. Reset the data lines to
> fix this at the source.
> 

It seems to work for my platform by porting it to 4.19 LTS.

Tested-by: Shawn Lin <shawn.lin@rock-chips.com>

> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
> ---
>   drivers/mmc/host/sdhci-of-arasan.c | 21 ++++-----------------
>   1 file changed, 4 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> index e49b44b4d82e..1495ae72b902 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> @@ -376,22 +376,8 @@ static void sdhci_arasan_dumpregs(struct mmc_host *mmc)
>   	sdhci_dumpregs(mmc_priv(mmc));
>   }
>   
> -static void sdhci_arasan_cqe_enable(struct mmc_host *mmc)
> -{
> -	struct sdhci_host *host = mmc_priv(mmc);
> -	u32 reg;
> -
> -	reg = sdhci_readl(host, SDHCI_PRESENT_STATE);
> -	while (reg & SDHCI_DATA_AVAILABLE) {
> -		sdhci_readl(host, SDHCI_BUFFER);
> -		reg = sdhci_readl(host, SDHCI_PRESENT_STATE);
> -	}
> -
> -	sdhci_cqe_enable(mmc);
> -}
> -
>   static const struct cqhci_host_ops sdhci_arasan_cqhci_ops = {
> -	.enable         = sdhci_arasan_cqe_enable,
> +	.enable         = sdhci_cqe_enable,
>   	.disable        = sdhci_cqe_disable,
>   	.dumpregs       = sdhci_arasan_dumpregs,
>   };
> @@ -410,8 +396,9 @@ static const struct sdhci_ops sdhci_arasan_cqe_ops = {
>   static const struct sdhci_pltfm_data sdhci_arasan_cqe_pdata = {
>   	.ops = &sdhci_arasan_cqe_ops,
>   	.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
> -	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
> -			SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN,
> +	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN	|
> +		   SDHCI_QUIRK2_RESET_DATA_POST_TUNING	|
> +		   SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN,
>   };
>   
>   static struct sdhci_arasan_of_data sdhci_arasan_rk3399_data = {
> 

Re: [PATCH 0/3] Fix issues with command queuing in arasan controllers

[Faiz Abbas]

Hi,

On 30/12/19 2:53 pm, Faiz Abbas wrote:
> In some Arasan SDHCI controllers, after tuning, the tuning pattern data
> is leftover in the sdhci buffer. This leads to issues with future data
> commands, especially when command queuing is enabled. The following
> patches help fix this issue by resetting data lines after tuning is
> finished. The first two patches have been tested with TI's am65x and
> j721e SoCs using the sdhci_am654 driver.
> 
> I have a strong suspicion that this is the same issue with
> the sdhci-of-arasan driver where they are forced to dump data from the
> buffer before enabling command queuing. I need help from someone with a
> compatible platform to test this.
> 

I had some discussions with our hardware team and they say we should be
asserting both SRC and SRD reset after tuning to start from a clean
state. Will update the patches to do that in v2.

Thanks,
Faiz

Re: [PATCH 0/3] Fix issues with command queuing in arasan controllers

[Adrian Hunter]

On 8/01/20 1:30 pm, Faiz Abbas wrote:
> Hi,
> 
> On 30/12/19 2:53 pm, Faiz Abbas wrote:
>> In some Arasan SDHCI controllers, after tuning, the tuning pattern data
>> is leftover in the sdhci buffer. This leads to issues with future data
>> commands, especially when command queuing is enabled. The following
>> patches help fix this issue by resetting data lines after tuning is
>> finished. The first two patches have been tested with TI's am65x and
>> j721e SoCs using the sdhci_am654 driver.
>>
>> I have a strong suspicion that this is the same issue with
>> the sdhci-of-arasan driver where they are forced to dump data from the
>> buffer before enabling command queuing. I need help from someone with a
>> compatible platform to test this.
>>
> 
> I had some discussions with our hardware team and they say we should be
> asserting both SRC and SRD reset after tuning to start from a clean
> state. Will update the patches to do that in v2.

Can you use the ->execute_tuning() for that instead of a quirk?

Re: [PATCH 0/3] Fix issues with command queuing in arasan controllers

[Faiz Abbas]

Adrian,

On 08/01/20 5:12 pm, Adrian Hunter wrote:
> On 8/01/20 1:30 pm, Faiz Abbas wrote:
>> Hi,
>>
>> On 30/12/19 2:53 pm, Faiz Abbas wrote:
>>> In some Arasan SDHCI controllers, after tuning, the tuning pattern data
>>> is leftover in the sdhci buffer. This leads to issues with future data
>>> commands, especially when command queuing is enabled. The following
>>> patches help fix this issue by resetting data lines after tuning is
>>> finished. The first two patches have been tested with TI's am65x and
>>> j721e SoCs using the sdhci_am654 driver.
>>>
>>> I have a strong suspicion that this is the same issue with
>>> the sdhci-of-arasan driver where they are forced to dump data from the
>>> buffer before enabling command queuing. I need help from someone with a
>>> compatible platform to test this.
>>>
>>
>> I had some discussions with our hardware team and they say we should be
>> asserting both SRC and SRD reset after tuning to start from a clean
>> state. Will update the patches to do that in v2.
> 
> Can you use the ->execute_tuning() for that instead of a quirk?
> 

->platform_execute_tuning() is called before __sdhci_execute_tuning(). I
need this to be done after that. Should I add a post_tuning() callback?

Thanks,
Faiz

Re: [PATCH 0/3] Fix issues with command queuing in arasan controllers

[Adrian Hunter]

On 8/01/20 1:49 pm, Faiz Abbas wrote:
> Adrian,
> 
> On 08/01/20 5:12 pm, Adrian Hunter wrote:
>> On 8/01/20 1:30 pm, Faiz Abbas wrote:
>>> Hi,
>>>
>>> On 30/12/19 2:53 pm, Faiz Abbas wrote:
>>>> In some Arasan SDHCI controllers, after tuning, the tuning pattern data
>>>> is leftover in the sdhci buffer. This leads to issues with future data
>>>> commands, especially when command queuing is enabled. The following
>>>> patches help fix this issue by resetting data lines after tuning is
>>>> finished. The first two patches have been tested with TI's am65x and
>>>> j721e SoCs using the sdhci_am654 driver.
>>>>
>>>> I have a strong suspicion that this is the same issue with
>>>> the sdhci-of-arasan driver where they are forced to dump data from the
>>>> buffer before enabling command queuing. I need help from someone with a
>>>> compatible platform to test this.
>>>>
>>>
>>> I had some discussions with our hardware team and they say we should be
>>> asserting both SRC and SRD reset after tuning to start from a clean
>>> state. Will update the patches to do that in v2.
>>
>> Can you use the ->execute_tuning() for that instead of a quirk?
>>
> 
> ->platform_execute_tuning() is called before __sdhci_execute_tuning(). I
> need this to be done after that. Should I add a post_tuning() callback?

I meant hook host->mmc_host_ops.execute_tuning and call
sdhci_execute_tuning() and then sdhci_reset(), like in intel_execute_tuning().

Re: [PATCH 0/3] Fix issues with command queuing in arasan controllers

[Faiz Abbas]

Hi Adrian,

On 08/01/20 5:29 pm, Adrian Hunter wrote:
> On 8/01/20 1:49 pm, Faiz Abbas wrote:
>> Adrian,
>>
>> On 08/01/20 5:12 pm, Adrian Hunter wrote:
>>> On 8/01/20 1:30 pm, Faiz Abbas wrote:
>>>> Hi,
>>>>
>>>> On 30/12/19 2:53 pm, Faiz Abbas wrote:
>>>>> In some Arasan SDHCI controllers, after tuning, the tuning pattern data
>>>>> is leftover in the sdhci buffer. This leads to issues with future data
>>>>> commands, especially when command queuing is enabled. The following
>>>>> patches help fix this issue by resetting data lines after tuning is
>>>>> finished. The first two patches have been tested with TI's am65x and
>>>>> j721e SoCs using the sdhci_am654 driver.
>>>>>
>>>>> I have a strong suspicion that this is the same issue with
>>>>> the sdhci-of-arasan driver where they are forced to dump data from the
>>>>> buffer before enabling command queuing. I need help from someone with a
>>>>> compatible platform to test this.
>>>>>
>>>>
>>>> I had some discussions with our hardware team and they say we should be
>>>> asserting both SRC and SRD reset after tuning to start from a clean
>>>> state. Will update the patches to do that in v2.
>>>
>>> Can you use the ->execute_tuning() for that instead of a quirk?
>>>
>>
>> ->platform_execute_tuning() is called before __sdhci_execute_tuning(). I
>> need this to be done after that. Should I add a post_tuning() callback?
> 
> I meant hook host->mmc_host_ops.execute_tuning and call
> sdhci_execute_tuning() and then sdhci_reset(), like in intel_execute_tuning().
> 

Ok. Makes sense.

Thanks,
Faiz