[PATCH v2] mmc: sdhci: make sure SDHCI_CLOCK_CARD_EN bit sticks

[PATCH v2] mmc: sdhci: make sure SDHCI_CLOCK_CARD_EN bit sticks

[Jean-Francois Dagenais]

Regardless of the broken-cd quirk, when it silently doesn't stick,
no clock is applied to the bus lines, yet the code continues to
try to make CMDs and times out after 10 seconds for each. This
process can take up to a minute as mmc_rescan_try_freq tries the
different commands to discover the card.

Short of changing sdhci_enable_clk's signature chain in all
dependent drivers, at least provide a hint that this might be the
problem. This will save tons of time for system integrators.

Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
---
Changes in v2:
 * removed redundant wmb()
---
 drivers/mmc/host/sdhci.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 63db84481dff..42a02d034fda 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1894,6 +1894,20 @@ void sdhci_enable_clk(struct sdhci_host *host, u16 clk)
 
 	clk |= SDHCI_CLOCK_CARD_EN;
 	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
+
+	clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+	if (clk & SDHCI_CLOCK_CARD_EN)
+		return;
+
+	/* The controller will clear this bit if card absent condition is
+	 * detected. If card is indeed present, check platform configuration for
+	 * how CD is reported to the SDHCI host controller. There may be an
+	 * "assume present" mechanism in the platform registers, or your pin mux
+	 * may be incorrect.
+	 */
+	pr_err("%s: SDHCI_CLOCK_CARD_EN bit did not stick. Card absent?\n",
+		mmc_hostname(host->mmc));
+	sdhci_dumpregs(host);
 }
 EXPORT_SYMBOL_GPL(sdhci_enable_clk);
 
-- 
2.25.0

Re: [PATCH v2] mmc: sdhci: make sure SDHCI_CLOCK_CARD_EN bit sticks

[Ulf Hansson]

On Wed, 19 Feb 2020 at 22:07, Jean-Francois Dagenais
<jeff.dagenais@gmail.com> wrote:
>
> Regardless of the broken-cd quirk, when it silently doesn't stick,
> no clock is applied to the bus lines, yet the code continues to
> try to make CMDs and times out after 10 seconds for each. This
> process can take up to a minute as mmc_rescan_try_freq tries the
> different commands to discover the card.
>
> Short of changing sdhci_enable_clk's signature chain in all
> dependent drivers, at least provide a hint that this might be the
> problem. This will save tons of time for system integrators.
>
> Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com>

The change looks reasonable to me. However I would like to get an ack
by Adrian before applying, as I may not have thought of all the
consequences this change may have.

Kind regards
Uffe


> ---
> Changes in v2:
>  * removed redundant wmb()
> ---
>  drivers/mmc/host/sdhci.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 63db84481dff..42a02d034fda 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1894,6 +1894,20 @@ void sdhci_enable_clk(struct sdhci_host *host, u16 clk)
>
>         clk |= SDHCI_CLOCK_CARD_EN;
>         sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
> +
> +       clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> +       if (clk & SDHCI_CLOCK_CARD_EN)
> +               return;
> +
> +       /* The controller will clear this bit if card absent condition is
> +        * detected. If card is indeed present, check platform configuration for
> +        * how CD is reported to the SDHCI host controller. There may be an
> +        * "assume present" mechanism in the platform registers, or your pin mux
> +        * may be incorrect.
> +        */
> +       pr_err("%s: SDHCI_CLOCK_CARD_EN bit did not stick. Card absent?\n",
> +               mmc_hostname(host->mmc));
> +       sdhci_dumpregs(host);
>  }
>  EXPORT_SYMBOL_GPL(sdhci_enable_clk);
>
> --
> 2.25.0
>

Re: [PATCH v2] mmc: sdhci: make sure SDHCI_CLOCK_CARD_EN bit sticks

[Adrian Hunter]

On 4/03/20 5:34 pm, Ulf Hansson wrote:
> On Wed, 19 Feb 2020 at 22:07, Jean-Francois Dagenais
> <jeff.dagenais@gmail.com> wrote:
>>
>> Regardless of the broken-cd quirk, when it silently doesn't stick,
>> no clock is applied to the bus lines, yet the code continues to
>> try to make CMDs and times out after 10 seconds for each. This
>> process can take up to a minute as mmc_rescan_try_freq tries the
>> different commands to discover the card.
>>
>> Short of changing sdhci_enable_clk's signature chain in all
>> dependent drivers, at least provide a hint that this might be the
>> problem. This will save tons of time for system integrators.
>>
>> Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
> 
> The change looks reasonable to me. However I would like to get an ack
> by Adrian before applying, as I may not have thought of all the
> consequences this change may have.
> 
> Kind regards
> Uffe
> 
> 
>> ---
>> Changes in v2:
>>  * removed redundant wmb()
>> ---
>>  drivers/mmc/host/sdhci.c | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 63db84481dff..42a02d034fda 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -1894,6 +1894,20 @@ void sdhci_enable_clk(struct sdhci_host *host, u16 clk)
>>
>>         clk |= SDHCI_CLOCK_CARD_EN;
>>         sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>> +
>> +       clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
>> +       if (clk & SDHCI_CLOCK_CARD_EN)
>> +               return;
>> +
>> +       /* The controller will clear this bit if card absent condition is

Please make the comment style consistent with the rest of sdhci.c e.g.

	/*
	 * The controller blah...
	 * blah ...
	 */

>> +        * detected. If card is indeed present, check platform configuration for
>> +        * how CD is reported to the SDHCI host controller. There may be an
>> +        * "assume present" mechanism in the platform registers, or your pin mux
>> +        * may be incorrect.
>> +        */
>> +       pr_err("%s: SDHCI_CLOCK_CARD_EN bit did not stick. Card absent?\n",
>> +               mmc_hostname(host->mmc));
>> +       sdhci_dumpregs(host);

The error message is ok, but for some controllers this could happen normally
if the card is removed suddenly, so let's not also do a register dump.

>>  }
>>  EXPORT_SYMBOL_GPL(sdhci_enable_clk);
>>
>> --
>> 2.25.0
>>