[PATCH] mmc: sdhci: remove mdelay in eMMC tuning

[PATCH] mmc: sdhci: remove mdelay in eMMC tuning

[Nick Sanders]

This patch removes an unneccesary 1ms mdelay in the HS200 tuning
loop, called 40 times per retuning. Currently this causes a latency
of >40ms on any emmc accesses triggering wake from runtime PM,
which can occur for a significant portion of reads on a mostly idle system.

The delay is left in place for SD Cards, which use
MMC_SEND_TUNING_BLOCK rather than MMC_SEND_TUNING_BLOCK_HS200.
I'm not able to find evidence that this is required for SD in the
specs I have access to, however this delay has been present from
initial checkin for SD so I have preserved the original behavior for
compatibility.

This has been verified to fix observed glitching on local audio
playback and recording on apps with inbuilt assumptions on storage
latency.

Signed-off-by: Nick Sanders <nsanders@chromium.org>
Reviewed-by: Grant Grundler <grundler@chromium.org>
Reviewed-by: Doug Anderson <dianders@chromium.org>
---
 drivers/mmc/host/sdhci.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index df6d85a..05ffac2 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1987,7 +1987,10 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
 		ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
 		tuning_loop_counter--;
 		timeout--;
-		mdelay(1);
+
+		/* eMMC spec does not require a delay between tuning cycles */
+		if (opcode == MMC_SEND_TUNING_BLOCK)
+			mdelay(1);
 	} while (ctrl & SDHCI_CTRL_EXEC_TUNING);
 
 	/*
-- 
1.9.1.423.g4596e3a

Re: [PATCH] mmc: sdhci: remove mdelay in eMMC tuning

[Ulf Hansson]

On 7 May 2014 03:52, Nick Sanders <nsanders@chromium.org> wrote:
> This patch removes an unneccesary 1ms mdelay in the HS200 tuning
> loop, called 40 times per retuning. Currently this causes a latency
> of >40ms on any emmc accesses triggering wake from runtime PM,
> which can occur for a significant portion of reads on a mostly idle system.

Aha, so you are actually using "MMC_CAP_RUNTIME_RESUME" here - cool :-).

I suppose those patches that enables the "cap" has not reached
mainline yet. Any plans on sending them?

>
> The delay is left in place for SD Cards, which use
> MMC_SEND_TUNING_BLOCK rather than MMC_SEND_TUNING_BLOCK_HS200.
> I'm not able to find evidence that this is required for SD in the
> specs I have access to, however this delay has been present from
> initial checkin for SD so I have preserved the original behavior for
> compatibility.
>
> This has been verified to fix observed glitching on local audio
> playback and recording on apps with inbuilt assumptions on storage
> latency.
>
> Signed-off-by: Nick Sanders <nsanders@chromium.org>
> Reviewed-by: Grant Grundler <grundler@chromium.org>
> Reviewed-by: Doug Anderson <dianders@chromium.org>

Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

> ---
>  drivers/mmc/host/sdhci.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index df6d85a..05ffac2 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1987,7 +1987,10 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
>                 ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>                 tuning_loop_counter--;
>                 timeout--;
> -               mdelay(1);
> +
> +               /* eMMC spec does not require a delay between tuning cycles */
> +               if (opcode == MMC_SEND_TUNING_BLOCK)
> +                       mdelay(1);
>         } while (ctrl & SDHCI_CTRL_EXEC_TUNING);
>
>         /*
> --
> 1.9.1.423.g4596e3a
>

Re: [PATCH] mmc: sdhci: remove mdelay in eMMC tuning

[Nick Sanders]

On Wed, May 7, 2014 at 1:35 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> Aha, so you are actually using "MMC_CAP_RUNTIME_RESUME" here - cool :-).
>
> I suppose those patches that enables the "cap" has not reached
> mainline yet. Any plans on sending them?

I am actually running 3.10, so MMC_CAP_RUNTIME_RESUME isn't used yet.
sdhci-acpi.c is enabling runtime pm for my device, and there are no
caps checks otherwise.

Re: [PATCH] mmc: sdhci: remove mdelay in eMMC tuning

[Grant Grundler]

On Wed, May 7, 2014 at 1:35 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 7 May 2014 03:52, Nick Sanders <nsanders@chromium.org> wrote:
>> This patch removes an unneccesary 1ms mdelay in the HS200 tuning
>> loop, called 40 times per retuning. Currently this causes a latency
>> of >40ms on any emmc accesses triggering wake from runtime PM,
>> which can occur for a significant portion of reads on a mostly idle system.
>
> Aha, so you are actually using "MMC_CAP_RUNTIME_RESUME" here - cool :-).
>
> I suppose those patches that enables the "cap" has not reached
> mainline yet. Any plans on sending them?
>
>>
>> The delay is left in place for SD Cards, which use
>> MMC_SEND_TUNING_BLOCK rather than MMC_SEND_TUNING_BLOCK_HS200.
>> I'm not able to find evidence that this is required for SD in the
>> specs I have access to, however this delay has been present from
>> initial checkin for SD so I have preserved the original behavior for
>> compatibility.
>>
>> This has been verified to fix observed glitching on local audio
>> playback and recording on apps with inbuilt assumptions on storage
>> latency.
>>
>> Signed-off-by: Nick Sanders <nsanders@chromium.org>
>> Reviewed-by: Grant Grundler <grundler@chromium.org>
>> Reviewed-by: Doug Anderson <dianders@chromium.org>
>
> Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

Ulf,
Is your ACK enough to get this to land upstream or does cjb need to
respond it's been accepted?

It doesn't appear to have landed in mmc-next branch yet:
    http://git.kernel.org/cgit/linux/kernel/git/cjb/mmc.git/log/?h=mmc-next

apologies if it appears I am being impatient...but it's been almost a week now.

thanks,
grant

>
>> ---
>>  drivers/mmc/host/sdhci.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index df6d85a..05ffac2 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -1987,7 +1987,10 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode)
>>                 ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>>                 tuning_loop_counter--;
>>                 timeout--;
>> -               mdelay(1);
>> +
>> +               /* eMMC spec does not require a delay between tuning cycles */
>> +               if (opcode == MMC_SEND_TUNING_BLOCK)
>> +                       mdelay(1);
>>         } while (ctrl & SDHCI_CTRL_EXEC_TUNING);
>>
>>         /*
>> --
>> 1.9.1.423.g4596e3a
>>

Re: [PATCH] mmc: sdhci: remove mdelay in eMMC tuning

[Chris Ball]

Hi,

On Wed, May 14 2014, Grant Grundler wrote:
>>> The delay is left in place for SD Cards, which use
>>> MMC_SEND_TUNING_BLOCK rather than MMC_SEND_TUNING_BLOCK_HS200.
>>> I'm not able to find evidence that this is required for SD in the
>>> specs I have access to, however this delay has been present from
>>> initial checkin for SD so I have preserved the original behavior for
>>> compatibility.
>>>
>>> This has been verified to fix observed glitching on local audio
>>> playback and recording on apps with inbuilt assumptions on storage
>>> latency.
>>>
>>> Signed-off-by: Nick Sanders <nsanders@chromium.org>
>>> Reviewed-by: Grant Grundler <grundler@chromium.org>
>>> Reviewed-by: Doug Anderson <dianders@chromium.org>
>>
>> Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
>
> Ulf,
> Is your ACK enough to get this to land upstream or does cjb need to
> respond it's been accepted?
>
> It doesn't appear to have landed in mmc-next branch yet:
>     http://git.kernel.org/cgit/linux/kernel/git/cjb/mmc.git/log/?h=mmc-next
>
> apologies if it appears I am being impatient...but it's been almost a week now.

Sorry about that.  Looks fine, pushed to mmc-next with Ulf's ACK now.

- Chris.
-- 
Chris Ball   <http://printf.net/>

Re: [PATCH] mmc: sdhci: remove mdelay in eMMC tuning

[Arnd Bergmann]

On Tuesday 13 May 2014 18:13:30 Grant Grundler wrote:
> On Wed, May 7, 2014 at 1:35 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > On 7 May 2014 03:52, Nick Sanders <nsanders@chromium.org> wrote:
> >> This patch removes an unneccesary 1ms mdelay in the HS200 tuning
> >> loop, called 40 times per retuning. Currently this causes a latency
> >> of >40ms on any emmc accesses triggering wake from runtime PM,
> >> which can occur for a significant portion of reads on a mostly idle system.
> >
> > Aha, so you are actually using "MMC_CAP_RUNTIME_RESUME" here - cool :-).
> >
> > I suppose those patches that enables the "cap" has not reached
> > mainline yet. Any plans on sending them?
> >
> >>
> >> The delay is left in place for SD Cards, which use
> >> MMC_SEND_TUNING_BLOCK rather than MMC_SEND_TUNING_BLOCK_HS200.
> >> I'm not able to find evidence that this is required for SD in the
> >> specs I have access to, however this delay has been present from
> >> initial checkin for SD so I have preserved the original behavior for
> >> compatibility.
> >>
> >> This has been verified to fix observed glitching on local audio
> >> playback and recording on apps with inbuilt assumptions on storage
> >> latency.
> >>
> >> Signed-off-by: Nick Sanders <nsanders@chromium.org>
> >> Reviewed-by: Grant Grundler <grundler@chromium.org>
> >> Reviewed-by: Doug Anderson <dianders@chromium.org>
> >
> > Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
> 
> Ulf,
> Is your ACK enough to get this to land upstream or does cjb need to
> respond it's been accepted?
> 
> It doesn't appear to have landed in mmc-next branch yet:
>     http://git.kernel.org/cgit/linux/kernel/git/cjb/mmc.git/log/?h=mmc-next
> 
> apologies if it appears I am being impatient...but it's been almost a week now.
> 

I just stumbled over this thread. Looking at the callers of the
execute_tuning() function, I see that all three come from non-atomic
context. How about an add-on patch to turn the mdelay(1) into an
msleep(1)? That would be much friendlier to other tasks running
concurrently.

	Arnd

Re: [PATCH] mmc: sdhci: remove mdelay in eMMC tuning

[Doug Anderson]

Arnd,

On Wed, May 14, 2014 at 5:18 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> I just stumbled over this thread. Looking at the callers of the
> execute_tuning() function, I see that all three come from non-atomic
> context. How about an add-on patch to turn the mdelay(1) into an
> msleep(1)? That would be much friendlier to other tasks running
> concurrently.

Even better if someone could tell us why the mdelay() was there to
begin with.  As I understand it Nick was confident that it wasn't
needed for eMMC.  He didn't _think_ it was needed for SD cards either
but it was safer to leave it there for SD cards since the delay wasn't
causing any known problems on them.

-Doug