[PATCH 0/3] mmc: sdhci: Disable re-tuning for HS400

[PATCH 0/3] mmc: sdhci: Disable re-tuning for HS400

[Adrian Hunter]

Hi

As described in patch 3, re-tuning for HS400 has to be done
in HS200 mode, but there is no support for that, so re-tuning
needs to be disabled until support is added. I have some patches
for that which I will send in the next day or two.


Adrian Hunter (3):
      mmc: sdhci: Tuning should not change max_blk_count
      mmc: sdhci: Add out_unlock to sdhci_execute_tuning
      mmc: sdhci: Disable re-tuning for HS400


Regards
Adrian

[PATCH 1/3] mmc: sdhci: Tuning should not change max_blk_count

[Adrian Hunter]

Re-tuning requires that the maximum data length
is limited to 4MiB. The code currently changes
max_blk_count in an attempt to achieve that.
This is wrong because max_blk_count is a different
limit, but it is also un-necessary because
max_req_size is 512KiB anyway. Consequently, the
changes to max_blk_count are removed and the
comment for max_req_size adjusted accordingly.
The comment is also tweaked to show that the 512KiB
limit is a SDMA limit not an ADMA limit.

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

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 73de62a..b3d68e0 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -259,8 +259,6 @@ static void sdhci_reinit(struct sdhci_host *host)
 
 		del_timer_sync(&host->tuning_timer);
 		host->flags &= ~SDHCI_NEEDS_RETUNING;
-		host->mmc->max_blk_count =
-			(host->quirks & SDHCI_QUIRK_NO_MULTIBLOCK) ? 1 : 65535;
 	}
 	sdhci_enable_card_detection(host);
 }
@@ -2048,8 +2046,6 @@ out:
 		host->flags |= SDHCI_USING_RETUNING_TIMER;
 		mod_timer(&host->tuning_timer, jiffies +
 			host->tuning_count * HZ);
-		/* Tuning mode 1 limits the maximum data length to 4MB */
-		mmc->max_blk_count = (4 * 1024 * 1024) / mmc->max_blk_size;
 	} else if (host->flags & SDHCI_USING_RETUNING_TIMER) {
 		host->flags &= ~SDHCI_NEEDS_RETUNING;
 		/* Reload the new initial value for timer */
@@ -3263,8 +3259,9 @@ int sdhci_add_host(struct sdhci_host *host)
 		mmc->max_segs = SDHCI_MAX_SEGS;
 
 	/*
-	 * Maximum number of sectors in one transfer. Limited by DMA boundary
-	 * size (512KiB).
+	 * Maximum number of sectors in one transfer. Limited by SDMA boundary
+	 * size (512KiB). Note some tuning modes impose a 4MiB limit, but this
+	 * is less anyway.
 	 */
 	mmc->max_req_size = 524288;
 
-- 
1.9.1

[PATCH 2/3] mmc: sdhci: Add out_unlock to sdhci_execute_tuning

[Adrian Hunter]

A 'goto' can be used to save duplicating unlocking
and returning.

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

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index b3d68e0..2efa7fe 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1909,9 +1909,7 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
 		/* FALLTHROUGH */
 
 	default:
-		spin_unlock_irqrestore(&host->lock, flags);
-		sdhci_runtime_pm_put(host);
-		return 0;
+		goto out_unlock;
 	}
 
 	if (host->ops->platform_execute_tuning) {
@@ -2066,6 +2064,7 @@ out:
 
 	sdhci_writel(host, host->ier, SDHCI_INT_ENABLE);
 	sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE);
+out_unlock:
 	spin_unlock_irqrestore(&host->lock, flags);
 	sdhci_runtime_pm_put(host);
 
-- 
1.9.1

[PATCH 3/3] mmc: sdhci: Disable re-tuning for HS400

[Adrian Hunter]

Re-tuning for HS400 mode must be done in HS200
mode. Currently there is no support for that.
That needs to be reflected in the code.
Specifically, if tuning is executed in HS400 mode
then return an error, and if the re-tuning timer
is running when switching to HS400 mode, then
disable the timer.

Note that periodic re-tuning is not expected
to be needed for HS400 but re-tuning is still
needed after the host controller has lost power.
In the case of suspend/resume that is not necessary
because the card is fully re-initialised. That
just leaves runtime suspend/resume with no support
for HS400 re-tuning.

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

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 2efa7fe..a7c9e67 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1476,8 +1476,18 @@ void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing)
 	else if ((timing == MMC_TIMING_UHS_DDR50) ||
 		 (timing == MMC_TIMING_MMC_DDR52))
 		ctrl_2 |= SDHCI_CTRL_UHS_DDR50;
-	else if (timing == MMC_TIMING_MMC_HS400)
+	else if (timing == MMC_TIMING_MMC_HS400) {
 		ctrl_2 |= SDHCI_CTRL_HS400; /* Non-standard */
+		/*
+		 * Periodic re-tuning for HS400 is not expected to be needed, so
+		 * disable it here.
+		 */
+		if (host->flags & SDHCI_USING_RETUNING_TIMER) {
+			host->flags &= ~SDHCI_USING_RETUNING_TIMER;
+			del_timer_sync(&host->tuning_timer);
+			host->flags &= ~SDHCI_NEEDS_RETUNING;
+		}
+	}
 	sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
 }
 EXPORT_SYMBOL_GPL(sdhci_set_uhs_signaling);
@@ -1897,7 +1907,11 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
 	 * tuning function has to be executed.
 	 */
 	switch (host->timing) {
+	/* HS400 tuning is done in HS200 mode */
 	case MMC_TIMING_MMC_HS400:
+		err = -EINVAL;
+		goto out_unlock;
+
 	case MMC_TIMING_MMC_HS200:
 	case MMC_TIMING_UHS_SDR104:
 		break;
-- 
1.9.1

Re: [PATCH 3/3] mmc: sdhci: Disable re-tuning for HS400

[Ulf Hansson]

On 1 December 2014 at 14:16, Adrian Hunter <adrian.hunter@intel.com> wrote:
> Re-tuning for HS400 mode must be done in HS200
> mode. Currently there is no support for that.
> That needs to be reflected in the code.
> Specifically, if tuning is executed in HS400 mode
> then return an error, and if the re-tuning timer
> is running when switching to HS400 mode, then
> disable the timer.
>
> Note that periodic re-tuning is not expected
> to be needed for HS400 but re-tuning is still
> needed after the host controller has lost power.

Why can't the old values be restored instead of trigger a re-tuning?

> In the case of suspend/resume that is not necessary
> because the card is fully re-initialised. That
> just leaves runtime suspend/resume with no support
> for HS400 re-tuning.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  drivers/mmc/host/sdhci.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 2efa7fe..a7c9e67 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1476,8 +1476,18 @@ void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing)
>         else if ((timing == MMC_TIMING_UHS_DDR50) ||
>                  (timing == MMC_TIMING_MMC_DDR52))
>                 ctrl_2 |= SDHCI_CTRL_UHS_DDR50;
> -       else if (timing == MMC_TIMING_MMC_HS400)
> +       else if (timing == MMC_TIMING_MMC_HS400) {
>                 ctrl_2 |= SDHCI_CTRL_HS400; /* Non-standard */
> +               /*
> +                * Periodic re-tuning for HS400 is not expected to be needed, so
> +                * disable it here.

Urgh, I don't like that the periodic tuning is handled by the host. We
should never had merged that.

How about trying to move the periodic tuning to be handled by the mmc
core instead?

> +                */
> +               if (host->flags & SDHCI_USING_RETUNING_TIMER) {
> +                       host->flags &= ~SDHCI_USING_RETUNING_TIMER;
> +                       del_timer_sync(&host->tuning_timer);
> +                       host->flags &= ~SDHCI_NEEDS_RETUNING;
> +               }
> +       }
>         sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
>  }
>  EXPORT_SYMBOL_GPL(sdhci_set_uhs_signaling);
> @@ -1897,7 +1907,11 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
>          * tuning function has to be executed.
>          */
>         switch (host->timing) {
> +       /* HS400 tuning is done in HS200 mode */
>         case MMC_TIMING_MMC_HS400:
> +               err = -EINVAL;
> +               goto out_unlock;
> +
>         case MMC_TIMING_MMC_HS200:
>         case MMC_TIMING_UHS_SDR104:
>                 break;
> --
> 1.9.1
>

Kind regards
Uffe

Re: [PATCH 3/3] mmc: sdhci: Disable re-tuning for HS400

[Adrian Hunter]

On 02/12/14 11:35, Ulf Hansson wrote:
> On 1 December 2014 at 14:16, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> Re-tuning for HS400 mode must be done in HS200
>> mode. Currently there is no support for that.
>> That needs to be reflected in the code.
>> Specifically, if tuning is executed in HS400 mode
>> then return an error, and if the re-tuning timer
>> is running when switching to HS400 mode, then
>> disable the timer.
>>
>> Note that periodic re-tuning is not expected
>> to be needed for HS400 but re-tuning is still
>> needed after the host controller has lost power.
> 
> Why can't the old values be restored instead of trigger a re-tuning?

The "values" (not sure what you mean by that) are not available to the
driver. Even if they were the operating conditions may have changed, (i.e.
temperature change) so the old "values" could still be wrong.

Jedec spec. says:

	It is recommended to perform tuning procedure while Device wakes
	up, after sleep.

SDHCI spec. says:

	If the Host System goes into power down mode, the Host Driver
	should stop the re-tuning timer and set the expiration flag
	to 1 when the Host System resumes from power down mode.

> 
>> In the case of suspend/resume that is not necessary
>> because the card is fully re-initialised. That
>> just leaves runtime suspend/resume with no support
>> for HS400 re-tuning.
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> ---
>>  drivers/mmc/host/sdhci.c | 16 +++++++++++++++-
>>  1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 2efa7fe..a7c9e67 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -1476,8 +1476,18 @@ void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing)
>>         else if ((timing == MMC_TIMING_UHS_DDR50) ||
>>                  (timing == MMC_TIMING_MMC_DDR52))
>>                 ctrl_2 |= SDHCI_CTRL_UHS_DDR50;
>> -       else if (timing == MMC_TIMING_MMC_HS400)
>> +       else if (timing == MMC_TIMING_MMC_HS400) {
>>                 ctrl_2 |= SDHCI_CTRL_HS400; /* Non-standard */
>> +               /*
>> +                * Periodic re-tuning for HS400 is not expected to be needed, so
>> +                * disable it here.
> 
> Urgh, I don't like that the periodic tuning is handled by the host. We
> should never had merged that.
> 
> How about trying to move the periodic tuning to be handled by the mmc
> core instead?

I have patches for that. I hope to send them today.

> 
>> +                */
>> +               if (host->flags & SDHCI_USING_RETUNING_TIMER) {
>> +                       host->flags &= ~SDHCI_USING_RETUNING_TIMER;
>> +                       del_timer_sync(&host->tuning_timer);
>> +                       host->flags &= ~SDHCI_NEEDS_RETUNING;
>> +               }
>> +       }
>>         sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
>>  }
>>  EXPORT_SYMBOL_GPL(sdhci_set_uhs_signaling);
>> @@ -1897,7 +1907,11 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
>>          * tuning function has to be executed.
>>          */
>>         switch (host->timing) {
>> +       /* HS400 tuning is done in HS200 mode */
>>         case MMC_TIMING_MMC_HS400:
>> +               err = -EINVAL;
>> +               goto out_unlock;
>> +
>>         case MMC_TIMING_MMC_HS200:
>>         case MMC_TIMING_UHS_SDR104:
>>                 break;
>> --
>> 1.9.1
>>
> 
> Kind regards
> Uffe
> 
> 

Re: [PATCH 3/3] mmc: sdhci: Disable re-tuning for HS400

[Ulf Hansson]

On 2 December 2014 at 11:08, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 02/12/14 11:35, Ulf Hansson wrote:
>> On 1 December 2014 at 14:16, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> Re-tuning for HS400 mode must be done in HS200
>>> mode. Currently there is no support for that.
>>> That needs to be reflected in the code.
>>> Specifically, if tuning is executed in HS400 mode
>>> then return an error, and if the re-tuning timer
>>> is running when switching to HS400 mode, then
>>> disable the timer.
>>>
>>> Note that periodic re-tuning is not expected
>>> to be needed for HS400 but re-tuning is still
>>> needed after the host controller has lost power.
>>
>> Why can't the old values be restored instead of trigger a re-tuning?
>
> The "values" (not sure what you mean by that) are not available to the
> driver. Even if they were the operating conditions may have changed, (i.e.
> temperature change) so the old "values" could still be wrong.

The "values" I refer to is those which we "calculated" during the
tuning process.

What I had in mind, was that we should save these values at runtime PM
suspend. And restore them at runtime PM resume. For some mmc
controllers the "values" are typically just a some bits in a
controller register, but that might not be true for all cases.

Regarding the temperature change, etc. I think that is what the
periodic retuning should be taken care off.

Could you elaborate on why the "values" is not available to the driver?

>
> Jedec spec. says:
>
>         It is recommended to perform tuning procedure while Device wakes
>         up, after sleep.
>
> SDHCI spec. says:
>
>         If the Host System goes into power down mode, the Host Driver
>         should stop the re-tuning timer and set the expiration flag
>         to 1 when the Host System resumes from power down mode.

I am not sure how to interpret this. Is the context about system PM or
runtime PM?

>
>>
>>> In the case of suspend/resume that is not necessary
>>> because the card is fully re-initialised. That
>>> just leaves runtime suspend/resume with no support
>>> for HS400 re-tuning.
>>>
>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>>> ---
>>>  drivers/mmc/host/sdhci.c | 16 +++++++++++++++-
>>>  1 file changed, 15 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index 2efa7fe..a7c9e67 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -1476,8 +1476,18 @@ void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing)
>>>         else if ((timing == MMC_TIMING_UHS_DDR50) ||
>>>                  (timing == MMC_TIMING_MMC_DDR52))
>>>                 ctrl_2 |= SDHCI_CTRL_UHS_DDR50;
>>> -       else if (timing == MMC_TIMING_MMC_HS400)
>>> +       else if (timing == MMC_TIMING_MMC_HS400) {
>>>                 ctrl_2 |= SDHCI_CTRL_HS400; /* Non-standard */
>>> +               /*
>>> +                * Periodic re-tuning for HS400 is not expected to be needed, so
>>> +                * disable it here.
>>
>> Urgh, I don't like that the periodic tuning is handled by the host. We
>> should never had merged that.
>>
>> How about trying to move the periodic tuning to be handled by the mmc
>> core instead?
>
> I have patches for that. I hope to send them today.

Great! Looking forward to review them!

Kind regards
Uffe

Re: [PATCH 3/3] mmc: sdhci: Disable re-tuning for HS400

[Adrian Hunter]

On 02/12/14 13:20, Ulf Hansson wrote:
> On 2 December 2014 at 11:08, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 02/12/14 11:35, Ulf Hansson wrote:
>>> On 1 December 2014 at 14:16, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>> Re-tuning for HS400 mode must be done in HS200
>>>> mode. Currently there is no support for that.
>>>> That needs to be reflected in the code.
>>>> Specifically, if tuning is executed in HS400 mode
>>>> then return an error, and if the re-tuning timer
>>>> is running when switching to HS400 mode, then
>>>> disable the timer.
>>>>
>>>> Note that periodic re-tuning is not expected
>>>> to be needed for HS400 but re-tuning is still
>>>> needed after the host controller has lost power.
>>>
>>> Why can't the old values be restored instead of trigger a re-tuning?
>>
>> The "values" (not sure what you mean by that) are not available to the
>> driver. Even if they were the operating conditions may have changed, (i.e.
>> temperature change) so the old "values" could still be wrong.
> 
> The "values" I refer to is those which we "calculated" during the
> tuning process.
> 
> What I had in mind, was that we should save these values at runtime PM
> suspend. And restore them at runtime PM resume. For some mmc
> controllers the "values" are typically just a some bits in a
> controller register, but that might not be true for all cases.
> 
> Regarding the temperature change, etc. I think that is what the
> periodic retuning should be taken care off.
> 
> Could you elaborate on why the "values" is not available to the driver?

The "optimal sampling point" has no corresponding register or value
in SDHCI.

> 
>>
>> Jedec spec. says:
>>
>>         It is recommended to perform tuning procedure while Device wakes
>>         up, after sleep.
>>
>> SDHCI spec. says:
>>
>>         If the Host System goes into power down mode, the Host Driver
>>         should stop the re-tuning timer and set the expiration flag
>>         to 1 when the Host System resumes from power down mode.
> 
> I am not sure how to interpret this. Is the context about system PM or
> runtime PM?

I interpret it to mean any situation where the host controller loses power
and therefore loses its tuning.

> 
>>
>>>
>>>> In the case of suspend/resume that is not necessary
>>>> because the card is fully re-initialised. That
>>>> just leaves runtime suspend/resume with no support
>>>> for HS400 re-tuning.
>>>>
>>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>>>> ---
>>>>  drivers/mmc/host/sdhci.c | 16 +++++++++++++++-
>>>>  1 file changed, 15 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>>> index 2efa7fe..a7c9e67 100644
>>>> --- a/drivers/mmc/host/sdhci.c
>>>> +++ b/drivers/mmc/host/sdhci.c
>>>> @@ -1476,8 +1476,18 @@ void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing)
>>>>         else if ((timing == MMC_TIMING_UHS_DDR50) ||
>>>>                  (timing == MMC_TIMING_MMC_DDR52))
>>>>                 ctrl_2 |= SDHCI_CTRL_UHS_DDR50;
>>>> -       else if (timing == MMC_TIMING_MMC_HS400)
>>>> +       else if (timing == MMC_TIMING_MMC_HS400) {
>>>>                 ctrl_2 |= SDHCI_CTRL_HS400; /* Non-standard */
>>>> +               /*
>>>> +                * Periodic re-tuning for HS400 is not expected to be needed, so
>>>> +                * disable it here.
>>>
>>> Urgh, I don't like that the periodic tuning is handled by the host. We
>>> should never had merged that.
>>>
>>> How about trying to move the periodic tuning to be handled by the mmc
>>> core instead?
>>
>> I have patches for that. I hope to send them today.
> 
> Great! Looking forward to review them!
> 
> Kind regards
> Uffe
> 
> 

Re: [PATCH 3/3] mmc: sdhci: Disable re-tuning for HS400

[Ulf Hansson]

On 2 December 2014 at 13:28, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 02/12/14 13:20, Ulf Hansson wrote:
>> On 2 December 2014 at 11:08, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> On 02/12/14 11:35, Ulf Hansson wrote:
>>>> On 1 December 2014 at 14:16, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>> Re-tuning for HS400 mode must be done in HS200
>>>>> mode. Currently there is no support for that.
>>>>> That needs to be reflected in the code.
>>>>> Specifically, if tuning is executed in HS400 mode
>>>>> then return an error, and if the re-tuning timer
>>>>> is running when switching to HS400 mode, then
>>>>> disable the timer.
>>>>>
>>>>> Note that periodic re-tuning is not expected
>>>>> to be needed for HS400 but re-tuning is still
>>>>> needed after the host controller has lost power.
>>>>
>>>> Why can't the old values be restored instead of trigger a re-tuning?
>>>
>>> The "values" (not sure what you mean by that) are not available to the
>>> driver. Even if they were the operating conditions may have changed, (i.e.
>>> temperature change) so the old "values" could still be wrong.
>>
>> The "values" I refer to is those which we "calculated" during the
>> tuning process.
>>
>> What I had in mind, was that we should save these values at runtime PM
>> suspend. And restore them at runtime PM resume. For some mmc
>> controllers the "values" are typically just a some bits in a
>> controller register, but that might not be true for all cases.
>>
>> Regarding the temperature change, etc. I think that is what the
>> periodic retuning should be taken care off.
>>
>> Could you elaborate on why the "values" is not available to the driver?
>
> The "optimal sampling point" has no corresponding register or value
> in SDHCI.

Is that really the case for all sdhci variants? For sure I am not an
sdhci expert, but I just find it to be a very poor HW design.

Especially if the sdhci controller may lose power frequently, it will
add a significant latency for each runtime PM resume cycle, right!?

Anyway, thanks for sharing the information.

Kind regards
Uffe