[PATCH V1 0/2] Introduce new quirk to use reserved timeout

[PATCH V1 0/2] Introduce new quirk to use reserved timeout

[Sarthak Garg]

Introduce a new quirk for letting vendor drivers to use reserved timeout
value (0xF) in timeout control register.

Sarthak Garg (2):
  mmc: sdhci: Introduce new quirk to use reserved timeout
  mmc: sdhci-msm: Use maximum possible data timeout value

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

-- 
2.7.4

[PATCH V1 1/2] mmc: sdhci: Introduce new quirk to use reserved timeout

[Sarthak Garg]

Introduce a new quirk for letting vendor drivers to use reserved
timeout value (0xF) in timeout control register.

Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
Signed-off-by: Sarthak Garg <sartgarg@codeaurora.org>
---
 drivers/mmc/host/sdhci.c | 3 ++-
 drivers/mmc/host/sdhci.h | 5 +++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 1bb6b67..07528a9 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -967,7 +967,8 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd,
 	}
 
 	if (count >= 0xF) {
-		if (!(host->quirks2 & SDHCI_QUIRK2_DISABLE_HW_TIMEOUT))
+		if (!(host->quirks2 & SDHCI_QUIRK2_DISABLE_HW_TIMEOUT) ||
+		!(host->quirks2 & SDHCI_QUIRK2_USE_RESERVED_MAX_TIMEOUT))
 			DBG("Too large timeout 0x%x requested for CMD%d!\n",
 			    count, cmd->opcode);
 		count = 0xE;
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 8d2a096..02f8779 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -476,6 +476,11 @@ struct sdhci_host {
  * block count.
  */
 #define SDHCI_QUIRK2_USE_32BIT_BLK_CNT			(1<<18)
+/*
+ * Some controllers define the usage of 0xF in data timeout counter
+ * register (0x2E) which is actually a reserved bit as per specification.
+ */
+#define SDHCI_QUIRK2_USE_RESERVED_MAX_TIMEOUT		(1<<19)
 
 	int irq;		/* Device IRQ */
 	void __iomem *ioaddr;	/* Mapped address */
-- 
2.7.4

[PATCH V1 2/2] mmc: sdhci-msm: Use maximum possible data timeout value

[Sarthak Garg]

The MSM SD controller defines the usage of 0xF in data
timeout counter register (0x2E) which is actually a reserved
bit as per specification. This would result in maximum of 21.26 secs
timeout value.

Some SDcard taking more time than 2.67secs (timeout value corresponding
to 0xE) and with that observed data timeout errors.
So increasing the timeout value to max possible timeout.

Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
Signed-off-by: Sarthak Garg <sartgarg@codeaurora.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 8a055dd..909855b 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -1888,7 +1888,8 @@ static const struct sdhci_pltfm_data sdhci_msm_pdata = {
 		  SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN |
 		  SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12,
 
-	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
+	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
+		SDHCI_QUIRK2_USE_RESERVED_MAX_TIMEOUT,
 	.ops = &sdhci_msm_ops,
 };
 
-- 
2.7.4

Re: [PATCH V1 1/2] mmc: sdhci: Introduce new quirk to use reserved timeout

[Ulf Hansson]

On Wed, 6 May 2020 at 15:53, Sarthak Garg <sartgarg@codeaurora.org> wrote:
>
> Introduce a new quirk for letting vendor drivers to use reserved
> timeout value (0xF) in timeout control register.
>
> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> Signed-off-by: Sarthak Garg <sartgarg@codeaurora.org>
> ---
>  drivers/mmc/host/sdhci.c | 3 ++-
>  drivers/mmc/host/sdhci.h | 5 +++++
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 1bb6b67..07528a9 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -967,7 +967,8 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd,
>         }
>
>         if (count >= 0xF) {
> -               if (!(host->quirks2 & SDHCI_QUIRK2_DISABLE_HW_TIMEOUT))
> +               if (!(host->quirks2 & SDHCI_QUIRK2_DISABLE_HW_TIMEOUT) ||
> +               !(host->quirks2 & SDHCI_QUIRK2_USE_RESERVED_MAX_TIMEOUT))

I don't quite get how this can make your variant use 0xF rather than 0xE?

To me it looks like an updated conditional check to print a debug message, no?

>                         DBG("Too large timeout 0x%x requested for CMD%d!\n",
>                             count, cmd->opcode);
>                 count = 0xE;
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 8d2a096..02f8779 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -476,6 +476,11 @@ struct sdhci_host {
>   * block count.
>   */
>  #define SDHCI_QUIRK2_USE_32BIT_BLK_CNT                 (1<<18)
> +/*
> + * Some controllers define the usage of 0xF in data timeout counter
> + * register (0x2E) which is actually a reserved bit as per specification.
> + */
> +#define SDHCI_QUIRK2_USE_RESERVED_MAX_TIMEOUT          (1<<19)
>
>         int irq;                /* Device IRQ */
>         void __iomem *ioaddr;   /* Mapped address */
> --
> 2.7.4
>

Kind regards
Uffe

Re: [PATCH V1 1/2] mmc: sdhci: Introduce new quirk to use reserved timeout

[Adrian Hunter]

On 18/05/20 12:39 pm, Ulf Hansson wrote:
> On Wed, 6 May 2020 at 15:53, Sarthak Garg <sartgarg@codeaurora.org> wrote:
>>
>> Introduce a new quirk for letting vendor drivers to use reserved
>> timeout value (0xF) in timeout control register.
>>
>> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
>> Signed-off-by: Sarthak Garg <sartgarg@codeaurora.org>
>> ---
>>  drivers/mmc/host/sdhci.c | 3 ++-
>>  drivers/mmc/host/sdhci.h | 5 +++++
>>  2 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 1bb6b67..07528a9 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -967,7 +967,8 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd,
>>         }
>>
>>         if (count >= 0xF) {
>> -               if (!(host->quirks2 & SDHCI_QUIRK2_DISABLE_HW_TIMEOUT))
>> +               if (!(host->quirks2 & SDHCI_QUIRK2_DISABLE_HW_TIMEOUT) ||
>> +               !(host->quirks2 & SDHCI_QUIRK2_USE_RESERVED_MAX_TIMEOUT))
> 
> I don't quite get how this can make your variant use 0xF rather than 0xE?
> 
> To me it looks like an updated conditional check to print a debug message, no?

Probably need to introduce host->max_timeout_count, set it to 0xE in
sdhci_alloc_host(), and change sdhci_calc_timeout() to use it in place of
all the 0xE and 0xF constants.

> 
>>                         DBG("Too large timeout 0x%x requested for CMD%d!\n",
>>                             count, cmd->opcode);
>>                 count = 0xE;
>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>> index 8d2a096..02f8779 100644
>> --- a/drivers/mmc/host/sdhci.h
>> +++ b/drivers/mmc/host/sdhci.h
>> @@ -476,6 +476,11 @@ struct sdhci_host {
>>   * block count.
>>   */
>>  #define SDHCI_QUIRK2_USE_32BIT_BLK_CNT                 (1<<18)
>> +/*
>> + * Some controllers define the usage of 0xF in data timeout counter
>> + * register (0x2E) which is actually a reserved bit as per specification.
>> + */
>> +#define SDHCI_QUIRK2_USE_RESERVED_MAX_TIMEOUT          (1<<19)
>>
>>         int irq;                /* Device IRQ */
>>         void __iomem *ioaddr;   /* Mapped address */
>> --
>> 2.7.4
>>
> 
> Kind regards
> Uffe
> 

Re: [PATCH V1 1/2] mmc: sdhci: Introduce new quirk to use reserved timeout

[Ulf Hansson]

On Mon, 18 May 2020 at 13:45, Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 18/05/20 12:39 pm, Ulf Hansson wrote:
> > On Wed, 6 May 2020 at 15:53, Sarthak Garg <sartgarg@codeaurora.org> wrote:
> >>
> >> Introduce a new quirk for letting vendor drivers to use reserved
> >> timeout value (0xF) in timeout control register.
> >>
> >> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
> >> Signed-off-by: Sarthak Garg <sartgarg@codeaurora.org>
> >> ---
> >>  drivers/mmc/host/sdhci.c | 3 ++-
> >>  drivers/mmc/host/sdhci.h | 5 +++++
> >>  2 files changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> >> index 1bb6b67..07528a9 100644
> >> --- a/drivers/mmc/host/sdhci.c
> >> +++ b/drivers/mmc/host/sdhci.c
> >> @@ -967,7 +967,8 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd,
> >>         }
> >>
> >>         if (count >= 0xF) {
> >> -               if (!(host->quirks2 & SDHCI_QUIRK2_DISABLE_HW_TIMEOUT))
> >> +               if (!(host->quirks2 & SDHCI_QUIRK2_DISABLE_HW_TIMEOUT) ||
> >> +               !(host->quirks2 & SDHCI_QUIRK2_USE_RESERVED_MAX_TIMEOUT))
> >
> > I don't quite get how this can make your variant use 0xF rather than 0xE?
> >
> > To me it looks like an updated conditional check to print a debug message, no?
>
> Probably need to introduce host->max_timeout_count, set it to 0xE in
> sdhci_alloc_host(), and change sdhci_calc_timeout() to use it in place of
> all the 0xE and 0xF constants.

Yep, that seems like a reasonable approach to me as well.

[...]

Kind regards
Uffe

Re: [PATCH V1 1/2] mmc: sdhci: Introduce new quirk to use reserved timeout

[sartgarg]

On 2020-05-18 18:20, Ulf Hansson wrote:
> On Mon, 18 May 2020 at 13:45, Adrian Hunter <adrian.hunter@intel.com> 
> wrote:
>> 
>> On 18/05/20 12:39 pm, Ulf Hansson wrote:
>> > On Wed, 6 May 2020 at 15:53, Sarthak Garg <sartgarg@codeaurora.org> wrote:
>> >>
>> >> Introduce a new quirk for letting vendor drivers to use reserved
>> >> timeout value (0xF) in timeout control register.
>> >>
>> >> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
>> >> Signed-off-by: Sarthak Garg <sartgarg@codeaurora.org>
>> >> ---
>> >>  drivers/mmc/host/sdhci.c | 3 ++-
>> >>  drivers/mmc/host/sdhci.h | 5 +++++
>> >>  2 files changed, 7 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> >> index 1bb6b67..07528a9 100644
>> >> --- a/drivers/mmc/host/sdhci.c
>> >> +++ b/drivers/mmc/host/sdhci.c
>> >> @@ -967,7 +967,8 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_command *cmd,
>> >>         }
>> >>
>> >>         if (count >= 0xF) {
>> >> -               if (!(host->quirks2 & SDHCI_QUIRK2_DISABLE_HW_TIMEOUT))
>> >> +               if (!(host->quirks2 & SDHCI_QUIRK2_DISABLE_HW_TIMEOUT) ||
>> >> +               !(host->quirks2 & SDHCI_QUIRK2_USE_RESERVED_MAX_TIMEOUT))
>> >
>> > I don't quite get how this can make your variant use 0xF rather than 0xE?
>> >
>> > To me it looks like an updated conditional check to print a debug message, no?
>> 
>> Probably need to introduce host->max_timeout_count, set it to 0xE in
>> sdhci_alloc_host(), and change sdhci_calc_timeout() to use it in place 
>> of
>> all the 0xE and 0xF constants.
> 
> Yep, that seems like a reasonable approach to me as well.
> 
> [...]
> 
> Kind regards
> Uffe

Resending the mail again as can't see my comment on the 
https://patchwork.kernel.org/ page.
Sorry for the mistake just want to update the logic as below.
-               count = 0xE;
+               if(!(host->quirks2 & 
SDHCI_QUIRK2_USE_RESERVED_MAX_TIMEOUT))
+                       count = 0xE;

Re: [PATCH V1 1/2] mmc: sdhci: Introduce new quirk to use reserved timeout

[Adrian Hunter]

On 19/05/20 5:08 pm, sartgarg@codeaurora.org wrote:
> On 2020-05-18 18:20, Ulf Hansson wrote:
>> On Mon, 18 May 2020 at 13:45, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>
>>> On 18/05/20 12:39 pm, Ulf Hansson wrote:
>>> > On Wed, 6 May 2020 at 15:53, Sarthak Garg <sartgarg@codeaurora.org> wrote:
>>> >>
>>> >> Introduce a new quirk for letting vendor drivers to use reserved
>>> >> timeout value (0xF) in timeout control register.
>>> >>
>>> >> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
>>> >> Signed-off-by: Sarthak Garg <sartgarg@codeaurora.org>
>>> >> ---
>>> >>  drivers/mmc/host/sdhci.c | 3 ++-
>>> >>  drivers/mmc/host/sdhci.h | 5 +++++
>>> >>  2 files changed, 7 insertions(+), 1 deletion(-)
>>> >>
>>> >> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> >> index 1bb6b67..07528a9 100644
>>> >> --- a/drivers/mmc/host/sdhci.c
>>> >> +++ b/drivers/mmc/host/sdhci.c
>>> >> @@ -967,7 +967,8 @@ static u8 sdhci_calc_timeout(struct sdhci_host
>>> *host, struct mmc_command *cmd,
>>> >>         }
>>> >>
>>> >>         if (count >= 0xF) {
>>> >> -               if (!(host->quirks2 & SDHCI_QUIRK2_DISABLE_HW_TIMEOUT))
>>> >> +               if (!(host->quirks2 & SDHCI_QUIRK2_DISABLE_HW_TIMEOUT) ||
>>> >> +               !(host->quirks2 & SDHCI_QUIRK2_USE_RESERVED_MAX_TIMEOUT))
>>> >
>>> > I don't quite get how this can make your variant use 0xF rather than 0xE?
>>> >
>>> > To me it looks like an updated conditional check to print a debug
>>> message, no?
>>>
>>> Probably need to introduce host->max_timeout_count, set it to 0xE in
>>> sdhci_alloc_host(), and change sdhci_calc_timeout() to use it in place of
>>> all the 0xE and 0xF constants.
>>
>> Yep, that seems like a reasonable approach to me as well.
>>
>> [...]
>>
>> Kind regards
>> Uffe
> 
> Resending the mail again as can't see my comment on the
> https://patchwork.kernel.org/ page.
> Sorry for the mistake just want to update the logic as below.
> -               count = 0xE;
> +               if(!(host->quirks2 & SDHCI_QUIRK2_USE_RESERVED_MAX_TIMEOUT))
> +                       count = 0xE;

I think it is conceptually simpler to define SDHCI constants as variables
and let vendor drivers change them if need be.  In other words, what I wrote
above.  It changes more code, but the overall result is more consistent.

Re: [PATCH V1 2/2] mmc: sdhci-msm: Use maximum possible data timeout value

[Bjorn Andersson]

On Fri 16 Jul 09:03 CDT 2021, Sarthak Garg wrote:

> The Qcom SD controller defines the usage of 0xF in data
> timeout counter register (0x2E) which is actually a reserved
> bit as per specification. This would result in maximum of 21.26 secs
> timeout value.
> 
> Some SDcard taking more time than 2.67secs (timeout value corresponding
> to 0xE) and with that observed data timeout errors.
> So increasing the timeout value to max possible timeout.
> 
> Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>

The From: of the email says you wrote the patch, but the first person to
certify its origin is Sahitya. Did you perhaps write this together? If
so you should have a:

Co-developed-by: Sahitya

If Sahitya wrote the patch, certifies its origin and then you picks it
up and certify that you got it from Sahitya, then you should write it as
you did - but you should have retained Sahitya as author...

Regards,
Bjorn

> Signed-off-by: Sarthak Garg <sartgarg@codeaurora.org>
> ---
>  drivers/mmc/host/sdhci-msm.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index e44b7a6..19e4673 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -2696,6 +2696,9 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>  
>  	msm_host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY | MMC_CAP_NEED_RSP_BUSY;
>  
> +	/* Set the timeout value to max possible */
> +	host->max_timeout_count = 0xF;
> +
>  	pm_runtime_get_noresume(&pdev->dev);
>  	pm_runtime_set_active(&pdev->dev);
>  	pm_runtime_enable(&pdev->dev);
> -- 
> 2.7.4
> 

[PATCH V1 2/2] mmc: sdhci-msm: Use maximum possible data timeout value

[Sarthak Garg]

The Qcom SD controller defines the usage of 0xF in data
timeout counter register (0x2E) which is actually a reserved
bit as per specification. This would result in maximum of 21.26 secs
timeout value.

Some SDcard taking more time than 2.67secs (timeout value corresponding
to 0xE) and with that observed data timeout errors.
So increasing the timeout value to max possible timeout.

Signed-off-by: Sahitya Tummala <stummala@codeaurora.org>
Signed-off-by: Sarthak Garg <sartgarg@codeaurora.org>
---
 drivers/mmc/host/sdhci-msm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index e44b7a6..19e4673 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -2696,6 +2696,9 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 
 	msm_host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY | MMC_CAP_NEED_RSP_BUSY;
 
+	/* Set the timeout value to max possible */
+	host->max_timeout_count = 0xF;
+
 	pm_runtime_get_noresume(&pdev->dev);
 	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
-- 
2.7.4