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
Enable SDHCI_QUIRK2_RESET_DATA_POST_TUNING for all controller variants.
Signed-off-by: Faiz Abbas <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Faiz Abbas <[email protected]>
> ---
> 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 = {
>
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
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
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().
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
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?