[PATCH 0/4] mmc: sdhci: Fix recovery from tuning timeout

[PATCH 0/4] mmc: sdhci: Fix recovery from tuning timeout

[Adrian Hunter]

Hi

Here are some small SDHCI tuning-related fixes.


Adrian Hunter (4):
      Revert "mmc: sdhci: Reset cmd and data circuits after tuning failure"
      mmc: sdhci: Fix recovery from tuning timeout
      mmc: sdhci: Fix tuning reset after exhausting the maximum number of loops
      mmc: sdhci: Always allow tuning to fall back to fixed sampling

 drivers/mmc/host/sdhci.c | 43 ++++++++++++++++++++++---------------------
 1 file changed, 22 insertions(+), 21 deletions(-)


Regards
Adrian

[PATCH 1/4] Revert "mmc: sdhci: Reset cmd and data circuits after tuning failure"

[Adrian Hunter]

This reverts commit fe5fb2e3b58f ("mmc: sdhci: Reset cmd and data circuits
after tuning failure").

A better fix is available, and it will be applied to older stable releases,
so get this out of the way by reverting it.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Cc: stable@vger.kernel.org # v4.9+
---
 drivers/mmc/host/sdhci.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 7f2526c9572f..e761fe2aa99e 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2090,10 +2090,6 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
 
 		if (!host->tuning_done) {
 			pr_info(DRIVER_NAME ": Timeout waiting for Buffer Read Ready interrupt during tuning procedure, falling back to fixed sampling clock\n");
-
-			sdhci_do_reset(host, SDHCI_RESET_CMD);
-			sdhci_do_reset(host, SDHCI_RESET_DATA);
-
 			ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
 			ctrl &= ~SDHCI_CTRL_TUNED_CLK;
 			ctrl &= ~SDHCI_CTRL_EXEC_TUNING;
-- 
1.9.1

[PATCH 2/4] mmc: sdhci: Fix recovery from tuning timeout

[Adrian Hunter]

Clearing the tuning bits should reset the tuning circuit. However there is
more to do. Reset the command and data lines for good measure, and then
for eMMC ensure the card is not still trying to process a tuning command by
sending a stop command.

Note the JEDEC eMMC specification says the stop command (CMD12) can be used
to stop a tuning command (CMD21) whereas the SD specification is silent on
the subject with respect to the SD tuning command (CMD19). Considering that
CMD12 is not a valid SDIO command, the stop command is sent only when the
tuning command is CMD21 i.e. for eMMC. That addresses cases seen so far
which have been on eMMC.

Note that this replaces the commit fe5fb2e3b58f ("mmc: sdhci: Reset cmd and
data circuits after tuning failure") which is being reverted for v4.9+.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Tested-by: Dan O'Donovan <dan@emutex.com>
Cc: stable@vger.kernel.org
---
 drivers/mmc/host/sdhci.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index e761fe2aa99e..1d72a51287d4 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2095,7 +2095,27 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
 			ctrl &= ~SDHCI_CTRL_EXEC_TUNING;
 			sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
 
+			sdhci_do_reset(host, SDHCI_RESET_CMD);
+			sdhci_do_reset(host, SDHCI_RESET_DATA);
+
 			err = -EIO;
+
+			if (cmd.opcode != MMC_SEND_TUNING_BLOCK_HS200)
+				goto out;
+
+			sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
+			sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE);
+
+			spin_unlock_irqrestore(&host->lock, flags);
+
+			memset(&cmd, 0, sizeof(cmd));
+			cmd.opcode = MMC_STOP_TRANSMISSION;
+			cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
+			cmd.busy_timeout = 50;
+			mmc_wait_for_cmd(mmc, &cmd, 0);
+
+			spin_lock_irqsave(&host->lock, flags);
+
 			goto out;
 		}
 
-- 
1.9.1

Re: [PATCH 2/4] mmc: sdhci: Fix recovery from tuning timeout

[Ulf Hansson]

On 30 November 2016 at 10:20, Adrian Hunter <adrian.hunter@intel.com> wrote:
> Clearing the tuning bits should reset the tuning circuit. However there is
> more to do. Reset the command and data lines for good measure, and then
> for eMMC ensure the card is not still trying to process a tuning command by
> sending a stop command.
>
> Note the JEDEC eMMC specification says the stop command (CMD12) can be used
> to stop a tuning command (CMD21) whereas the SD specification is silent on
> the subject with respect to the SD tuning command (CMD19). Considering that
> CMD12 is not a valid SDIO command, the stop command is sent only when the
> tuning command is CMD21 i.e. for eMMC. That addresses cases seen so far
> which have been on eMMC.
>
> Note that this replaces the commit fe5fb2e3b58f ("mmc: sdhci: Reset cmd and
> data circuits after tuning failure") which is being reverted for v4.9+.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> Tested-by: Dan O'Donovan <dan@emutex.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/mmc/host/sdhci.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index e761fe2aa99e..1d72a51287d4 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2095,7 +2095,27 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
>                         ctrl &= ~SDHCI_CTRL_EXEC_TUNING;
>                         sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>
> +                       sdhci_do_reset(host, SDHCI_RESET_CMD);
> +                       sdhci_do_reset(host, SDHCI_RESET_DATA);
> +
>                         err = -EIO;
> +
> +                       if (cmd.opcode != MMC_SEND_TUNING_BLOCK_HS200)
> +                               goto out;
> +
> +                       sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
> +                       sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE);
> +
> +                       spin_unlock_irqrestore(&host->lock, flags);
> +
> +                       memset(&cmd, 0, sizeof(cmd));
> +                       cmd.opcode = MMC_STOP_TRANSMISSION;
> +                       cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
> +                       cmd.busy_timeout = 50;
> +                       mmc_wait_for_cmd(mmc, &cmd, 0);

No, please don't add more hacks to send commands internally from sdhci.

Maybe even before you start fix the problems for tuning, perhaps you
try to clean up the current code when sending CMD21/19 in
sdhci_execute_tuning()?

Moreover, according to the change log above, it seems like a generic
thing to send CMD12 to abort tuning. In such case, we could either
make the core deal with it in the error path - or we could implement a
"mmc_abort_tuning()" function, host drivers may call when needed.

> +
> +                       spin_lock_irqsave(&host->lock, flags);
> +
>                         goto out;
>                 }
>
> --
> 1.9.1
>

Kind regards
Uffe

Re: [PATCH 2/4] mmc: sdhci: Fix recovery from tuning timeout

[Adrian Hunter]

On 30/11/16 12:06, Ulf Hansson wrote:
> On 30 November 2016 at 10:20, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> Clearing the tuning bits should reset the tuning circuit. However there is
>> more to do. Reset the command and data lines for good measure, and then
>> for eMMC ensure the card is not still trying to process a tuning command by
>> sending a stop command.
>>
>> Note the JEDEC eMMC specification says the stop command (CMD12) can be used
>> to stop a tuning command (CMD21) whereas the SD specification is silent on
>> the subject with respect to the SD tuning command (CMD19). Considering that
>> CMD12 is not a valid SDIO command, the stop command is sent only when the
>> tuning command is CMD21 i.e. for eMMC. That addresses cases seen so far
>> which have been on eMMC.
>>
>> Note that this replaces the commit fe5fb2e3b58f ("mmc: sdhci: Reset cmd and
>> data circuits after tuning failure") which is being reverted for v4.9+.
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> Tested-by: Dan O'Donovan <dan@emutex.com>
>> Cc: stable@vger.kernel.org
>> ---
>>  drivers/mmc/host/sdhci.c | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index e761fe2aa99e..1d72a51287d4 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -2095,7 +2095,27 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
>>                         ctrl &= ~SDHCI_CTRL_EXEC_TUNING;
>>                         sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>>
>> +                       sdhci_do_reset(host, SDHCI_RESET_CMD);
>> +                       sdhci_do_reset(host, SDHCI_RESET_DATA);
>> +
>>                         err = -EIO;
>> +
>> +                       if (cmd.opcode != MMC_SEND_TUNING_BLOCK_HS200)
>> +                               goto out;
>> +
>> +                       sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
>> +                       sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE);
>> +
>> +                       spin_unlock_irqrestore(&host->lock, flags);
>> +
>> +                       memset(&cmd, 0, sizeof(cmd));
>> +                       cmd.opcode = MMC_STOP_TRANSMISSION;
>> +                       cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
>> +                       cmd.busy_timeout = 50;
>> +                       mmc_wait_for_cmd(mmc, &cmd, 0);
> 
> No, please don't add more hacks to send commands internally from sdhci.
> 
> Maybe even before you start fix the problems for tuning, perhaps you
> try to clean up the current code when sending CMD21/19 in
> sdhci_execute_tuning()?
> 
> Moreover, according to the change log above, it seems like a generic
> thing to send CMD12 to abort tuning. In such case, we could either
> make the core deal with it in the error path - or we could implement a
> "mmc_abort_tuning()" function, host drivers may call when needed.

I am not sure a cleanup would apply cleanly to stable trees.  It would be
nicer to have these patches for stable and then a cleanup on top.  Would
that be acceptable?

Re: [PATCH 2/4] mmc: sdhci: Fix recovery from tuning timeout

[Ulf Hansson]

On 30 November 2016 at 11:17, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 30/11/16 12:06, Ulf Hansson wrote:
>> On 30 November 2016 at 10:20, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> Clearing the tuning bits should reset the tuning circuit. However there is
>>> more to do. Reset the command and data lines for good measure, and then
>>> for eMMC ensure the card is not still trying to process a tuning command by
>>> sending a stop command.
>>>
>>> Note the JEDEC eMMC specification says the stop command (CMD12) can be used
>>> to stop a tuning command (CMD21) whereas the SD specification is silent on
>>> the subject with respect to the SD tuning command (CMD19). Considering that
>>> CMD12 is not a valid SDIO command, the stop command is sent only when the
>>> tuning command is CMD21 i.e. for eMMC. That addresses cases seen so far
>>> which have been on eMMC.
>>>
>>> Note that this replaces the commit fe5fb2e3b58f ("mmc: sdhci: Reset cmd and
>>> data circuits after tuning failure") which is being reverted for v4.9+.
>>>
>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>>> Tested-by: Dan O'Donovan <dan@emutex.com>
>>> Cc: stable@vger.kernel.org
>>> ---
>>>  drivers/mmc/host/sdhci.c | 20 ++++++++++++++++++++
>>>  1 file changed, 20 insertions(+)
>>>
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index e761fe2aa99e..1d72a51287d4 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -2095,7 +2095,27 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
>>>                         ctrl &= ~SDHCI_CTRL_EXEC_TUNING;
>>>                         sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>>>
>>> +                       sdhci_do_reset(host, SDHCI_RESET_CMD);
>>> +                       sdhci_do_reset(host, SDHCI_RESET_DATA);
>>> +
>>>                         err = -EIO;
>>> +
>>> +                       if (cmd.opcode != MMC_SEND_TUNING_BLOCK_HS200)
>>> +                               goto out;
>>> +
>>> +                       sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
>>> +                       sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE);
>>> +
>>> +                       spin_unlock_irqrestore(&host->lock, flags);
>>> +
>>> +                       memset(&cmd, 0, sizeof(cmd));
>>> +                       cmd.opcode = MMC_STOP_TRANSMISSION;
>>> +                       cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
>>> +                       cmd.busy_timeout = 50;
>>> +                       mmc_wait_for_cmd(mmc, &cmd, 0);
>>
>> No, please don't add more hacks to send commands internally from sdhci.
>>
>> Maybe even before you start fix the problems for tuning, perhaps you
>> try to clean up the current code when sending CMD21/19 in
>> sdhci_execute_tuning()?
>>
>> Moreover, according to the change log above, it seems like a generic
>> thing to send CMD12 to abort tuning. In such case, we could either
>> make the core deal with it in the error path - or we could implement a
>> "mmc_abort_tuning()" function, host drivers may call when needed.
>
> I am not sure a cleanup would apply cleanly to stable trees.  It would be
> nicer to have these patches for stable and then a cleanup on top.  Would
> that be acceptable?
>

Yes. That's ok. Can you please re-spin with cleanups on top then.

Kind regards
Uffe

[PATCH 3/4] mmc: sdhci: Fix tuning reset after exhausting the maximum number of loops

[Adrian Hunter]

If the driver has exhausted the maximum number of tuning loops, then fixed
sampling is used. To do that both SDHCI_CTRL_TUNED_CLK and
SDHCI_CTRL_EXEC_TUNING must be reset to 0, but only SDHCI_CTRL_TUNED_CLK
was being reset. Reset SDHCI_CTRL_EXEC_TUNING to 0 also.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/host/sdhci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 1d72a51287d4..ad2f2c6113e4 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2134,6 +2134,7 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
 	 */
 	if (tuning_loop_counter < 0) {
 		ctrl &= ~SDHCI_CTRL_TUNED_CLK;
+		ctrl &= ~SDHCI_CTRL_EXEC_TUNING;
 		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
 	}
 	if (!(ctrl & SDHCI_CTRL_TUNED_CLK)) {
-- 
1.9.1

[PATCH 4/4] mmc: sdhci: Always allow tuning to fall back to fixed sampling

[Adrian Hunter]

SDHCI falls back to fixed sampling if there is an error during tuning.
However it also reports an error unless there is periodic re-tuning.
That is not the best option because:
a) there is a reasonable chance that fixed sampling will work, especially
at room temperature.
b) re-tuning will be done again anyway if there are CRC errors.
Change to return no error always when falling back to fixed sampling.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/host/sdhci.c | 20 ++------------------
 1 file changed, 2 insertions(+), 18 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index ad2f2c6113e4..a23887799f43 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2098,8 +2098,6 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
 			sdhci_do_reset(host, SDHCI_RESET_CMD);
 			sdhci_do_reset(host, SDHCI_RESET_DATA);
 
-			err = -EIO;
-
 			if (cmd.opcode != MMC_SEND_TUNING_BLOCK_HS200)
 				goto out;
 
@@ -2137,24 +2135,10 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
 		ctrl &= ~SDHCI_CTRL_EXEC_TUNING;
 		sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
 	}
-	if (!(ctrl & SDHCI_CTRL_TUNED_CLK)) {
+	if (!(ctrl & SDHCI_CTRL_TUNED_CLK))
 		pr_info(DRIVER_NAME ": Tuning procedure failed, falling back to fixed sampling clock\n");
-		err = -EIO;
-	}
-
 out:
-	if (tuning_count) {
-		/*
-		 * In case tuning fails, host controllers which support
-		 * re-tuning can try tuning again at a later time, when the
-		 * re-tuning timer expires.  So for these controllers, we
-		 * return 0. Since there might be other controllers who do not
-		 * have this capability, we return error for them.
-		 */
-		err = 0;
-	}
-
-	host->mmc->retune_period = err ? 0 : tuning_count;
+	host->mmc->retune_period = tuning_count;
 
 	sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
 	sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE);
-- 
1.9.1