[PATCH 0/2] sdhci-acpi initialization performance regression

[PATCH 0/2] sdhci-acpi initialization performance regression

[Adrian Hunter]

Hi

When I submitted the sdhci-acpi driver, it's initialization performance
was satisfactory.  Now it is not.  Here are 2 fixes to restore that
regression.

Adrian Hunter (2):
      Revert "mmc: core: wait while adding MMC host to ensure root mounts successfully"
      mmc: core: fix performance regression initializing MMC host controllers

 drivers/mmc/core/core.c       | 4 ++--
 drivers/mmc/host/sdhci-acpi.c | 2 ++
 include/linux/mmc/host.h      | 1 +
 3 files changed, 5 insertions(+), 2 deletions(-)


Regards
Adrian

[PATCH 1/2] Revert "mmc: core: wait while adding MMC host to ensure root mounts successfully"

[Adrian Hunter]

This reverts commit 3500ed90b26a9935b943b5e2e4cd3226600d6b58.

The reverted patch caused a significant performance regression when
booting with the root file system on eMMC.

Before the patch:

[    1.625623] VFS: Mounted root (ext4 filesystem) readonly on device 179:2.

After the patch:

[    1.935851] VFS: Mounted root (ext4 filesystem) readonly on device 179:2.

That was an addition of 310 ms which is a 19% performance degradation.

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

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index ad7decc..3bf1c46 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2418,7 +2418,6 @@ void mmc_start_host(struct mmc_host *host)
 	host->rescan_disable = 0;
 	mmc_power_up(host);
 	mmc_detect_change(host, 0);
-	mmc_flush_scheduled_work();
 }
 
 void mmc_stop_host(struct mmc_host *host)
-- 
1.7.11.7

[PATCH 2/2] mmc: core: fix performance regression initializing MMC host controllers

[Adrian Hunter]

Commit fa5501890d8974301042e0202d342a6cbe8609f4 introduced a performance
regression by adding mmc_power_up() to mmc_start_host().  mmc_power_up()
is not necessary to host controller initialization, it is part of card
initialization and is performed anyway asynchronously.

This patch allows a driver to leave the power up in asynchronous code
(as it was before).

On my current target platform this reduces driver initialization from:

[    1.313220] initcall sdhci_acpi_driver_init+0x0/0x12 returned 0 after 102008 usecs

to this:

[    1.217209] initcall sdhci_acpi_driver_init+0x0/0x12 returned 0 after 8331 usecs

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/core/core.c       | 3 ++-
 drivers/mmc/host/sdhci-acpi.c | 2 ++
 include/linux/mmc/host.h      | 1 +
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 3bf1c46..c1893c9 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2416,7 +2416,8 @@ void mmc_start_host(struct mmc_host *host)
 {
 	host->f_init = max(freqs[0], host->f_min);
 	host->rescan_disable = 0;
-	mmc_power_up(host);
+	if (!(host->caps2 & MMC_CAP2_NO_PRESCAN_POWERUP))
+		mmc_power_up(host);
 	mmc_detect_change(host, 0);
 }
 
diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
index 2592ddd..7bcf74b 100644
--- a/drivers/mmc/host/sdhci-acpi.c
+++ b/drivers/mmc/host/sdhci-acpi.c
@@ -195,6 +195,8 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
 		host->mmc->pm_caps  |= c->slot->pm_caps;
 	}
 
+	host->mmc->caps2 |= MMC_CAP2_NO_PRESCAN_POWERUP;
+
 	err = sdhci_add_host(host);
 	if (err)
 		goto err_free;
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 17d7148..8873e83 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -280,6 +280,7 @@ struct mmc_host {
 #define MMC_CAP2_PACKED_WR	(1 << 13)	/* Allow packed write */
 #define MMC_CAP2_PACKED_CMD	(MMC_CAP2_PACKED_RD | \
 				 MMC_CAP2_PACKED_WR)
+#define MMC_CAP2_NO_PRESCAN_POWERUP (1 << 14)	/* Don't power up before scan */
 
 	mmc_pm_flag_t		pm_caps;	/* supported pm features */
 
-- 
1.7.11.7

Re: [PATCH 1/2] Revert "mmc: core: wait while adding MMC host to ensure root mounts successfully"

[Sergey Yanovich]

On Thu, 2013-04-04 at 16:41 +0300, Adrian Hunter wrote:
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index ad7decc..3bf1c46 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -2418,7 +2418,6 @@ void mmc_start_host(struct mmc_host *host)
>  	host->rescan_disable = 0;
>  	mmc_power_up(host);
>  	mmc_detect_change(host, 0);
> -	mmc_flush_scheduled_work();
>  }
>  
>  void mmc_stop_host(struct mmc_host *host)

Without some kind of synchronization between end of card probing and
start of root mounting, it is not safe to have root on an mmc card.

In other words, 'root_wait' will be mandatory to ensure successful mount
of root filesystem on an mmc card. It may be worth documenting
somewhere.

It is not a big deal. I could live with a patched kernel or pass the
parameter in the systems I work with. However, other people will fall in
this trap later, and it would be nice to let them know what to do.

Re: [PATCH 2/2] mmc: core: fix performance regression initializing MMC host controllers

[Ulf Hansson]

On 4 April 2013 15:41, Adrian Hunter <adrian.hunter@intel.com> wrote:
> Commit fa5501890d8974301042e0202d342a6cbe8609f4 introduced a performance
> regression by adding mmc_power_up() to mmc_start_host().  mmc_power_up()
> is not necessary to host controller initialization, it is part of card
> initialization and is performed anyway asynchronously.

This commit message is a bit miss-leading. In eMMC case, when the host
driver has no possibility of power cycle the VCCQ power supply but
only the VCC, this is the only proper way to prevent violation of the
eMMC spec.

>From SD-card point of view, it is not needed, so this can be optimized
to be done as before in an asynchronous mode.

>
> This patch allows a driver to leave the power up in asynchronous code
> (as it was before).
>
> On my current target platform this reduces driver initialization from:
>
> [    1.313220] initcall sdhci_acpi_driver_init+0x0/0x12 returned 0 after 102008 usecs
>
> to this:
>
> [    1.217209] initcall sdhci_acpi_driver_init+0x0/0x12 returned 0 after 8331 usecs
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  drivers/mmc/core/core.c       | 3 ++-
>  drivers/mmc/host/sdhci-acpi.c | 2 ++
>  include/linux/mmc/host.h      | 1 +
>  3 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 3bf1c46..c1893c9 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -2416,7 +2416,8 @@ void mmc_start_host(struct mmc_host *host)
>  {
>         host->f_init = max(freqs[0], host->f_min);
>         host->rescan_disable = 0;
> -       mmc_power_up(host);
> +       if (!(host->caps2 & MMC_CAP2_NO_PRESCAN_POWERUP))
> +               mmc_power_up(host);
>         mmc_detect_change(host, 0);
>  }
>
> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
> index 2592ddd..7bcf74b 100644
> --- a/drivers/mmc/host/sdhci-acpi.c
> +++ b/drivers/mmc/host/sdhci-acpi.c
> @@ -195,6 +195,8 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
>                 host->mmc->pm_caps  |= c->slot->pm_caps;
>         }
>
> +       host->mmc->caps2 |= MMC_CAP2_NO_PRESCAN_POWERUP;
> +
>         err = sdhci_add_host(host);
>         if (err)
>                 goto err_free;
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 17d7148..8873e83 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -280,6 +280,7 @@ struct mmc_host {
>  #define MMC_CAP2_PACKED_WR     (1 << 13)       /* Allow packed write */
>  #define MMC_CAP2_PACKED_CMD    (MMC_CAP2_PACKED_RD | \
>                                  MMC_CAP2_PACKED_WR)
> +#define MMC_CAP2_NO_PRESCAN_POWERUP (1 << 14)  /* Don't power up before scan */
>
>         mmc_pm_flag_t           pm_caps;        /* supported pm features */
>
> --
> 1.7.11.7
>


Some update to the commit msg, then I am happy.

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

Re: [PATCH 2/2] mmc: core: fix performance regression initializing MMC host controllers

[Adrian Hunter]

On 05/04/13 00:02, Ulf Hansson wrote:
> On 4 April 2013 15:41, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> Commit fa5501890d8974301042e0202d342a6cbe8609f4 introduced a performance
>> regression by adding mmc_power_up() to mmc_start_host().  mmc_power_up()
>> is not necessary to host controller initialization, it is part of card
>> initialization and is performed anyway asynchronously.
> 
> This commit message is a bit miss-leading. In eMMC case, when the host
> driver has no possibility of power cycle the VCCQ power supply but
> only the VCC, this is the only proper way to prevent violation of the
> eMMC spec.

But that is not true.  The host controller driver can manage the voltages.
The patch is a workaround for the regulator_init_complete late initcall.

I deliberately did not paraphrase the original commit so that people who
wanted to know would have to look at it for themselves.  I really cannot add
things to my commit message that do not make sense to me.

> 
>>From SD-card point of view, it is not needed, so this can be optimized
> to be done as before in an asynchronous mode.
> 
>>
>> This patch allows a driver to leave the power up in asynchronous code
>> (as it was before).
>>
>> On my current target platform this reduces driver initialization from:
>>
>> [    1.313220] initcall sdhci_acpi_driver_init+0x0/0x12 returned 0 after 102008 usecs
>>
>> to this:
>>
>> [    1.217209] initcall sdhci_acpi_driver_init+0x0/0x12 returned 0 after 8331 usecs
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> ---
>>  drivers/mmc/core/core.c       | 3 ++-
>>  drivers/mmc/host/sdhci-acpi.c | 2 ++
>>  include/linux/mmc/host.h      | 1 +
>>  3 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index 3bf1c46..c1893c9 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -2416,7 +2416,8 @@ void mmc_start_host(struct mmc_host *host)
>>  {
>>         host->f_init = max(freqs[0], host->f_min);
>>         host->rescan_disable = 0;
>> -       mmc_power_up(host);
>> +       if (!(host->caps2 & MMC_CAP2_NO_PRESCAN_POWERUP))
>> +               mmc_power_up(host);
>>         mmc_detect_change(host, 0);
>>  }
>>
>> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
>> index 2592ddd..7bcf74b 100644
>> --- a/drivers/mmc/host/sdhci-acpi.c
>> +++ b/drivers/mmc/host/sdhci-acpi.c
>> @@ -195,6 +195,8 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
>>                 host->mmc->pm_caps  |= c->slot->pm_caps;
>>         }
>>
>> +       host->mmc->caps2 |= MMC_CAP2_NO_PRESCAN_POWERUP;
>> +
>>         err = sdhci_add_host(host);
>>         if (err)
>>                 goto err_free;
>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>> index 17d7148..8873e83 100644
>> --- a/include/linux/mmc/host.h
>> +++ b/include/linux/mmc/host.h
>> @@ -280,6 +280,7 @@ struct mmc_host {
>>  #define MMC_CAP2_PACKED_WR     (1 << 13)       /* Allow packed write */
>>  #define MMC_CAP2_PACKED_CMD    (MMC_CAP2_PACKED_RD | \
>>                                  MMC_CAP2_PACKED_WR)
>> +#define MMC_CAP2_NO_PRESCAN_POWERUP (1 << 14)  /* Don't power up before scan */
>>
>>         mmc_pm_flag_t           pm_caps;        /* supported pm features */
>>
>> --
>> 1.7.11.7
>>
> 
> 
> Some update to the commit msg, then I am happy.
> 
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> 
> 

[PATCH V2 2/2] mmc: core: fix performance regression initializing MMC, host controllers

[Adrian Hunter]

Commit fa5501890d8974301042e0202d342a6cbe8609f4 introduced a performance
regression by adding mmc_power_up() to mmc_start_host().  mmc_power_up()
is not necessary to host controller initialization, it is part of card
initialization and is performed anyway asynchronously.  Please see the
original commit for an explanation of the use of mmc_power_up() in
mmc_start_host().

This patch allows a driver to leave the power up in asynchronous code
(as it was before).

On my current target platform this reduces driver initialization from:

[    1.313220] initcall sdhci_acpi_driver_init+0x0/0x12 returned 0 after 102008 usecs

to this:

[    1.217209] initcall sdhci_acpi_driver_init+0x0/0x12 returned 0 after 8331 usecs

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/core/core.c       | 3 ++-
 drivers/mmc/host/sdhci-acpi.c | 2 ++
 include/linux/mmc/host.h      | 1 +
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 3bf1c46..c1893c9 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2416,7 +2416,8 @@ void mmc_start_host(struct mmc_host *host)
 {
 	host->f_init = max(freqs[0], host->f_min);
 	host->rescan_disable = 0;
-	mmc_power_up(host);
+	if (!(host->caps2 & MMC_CAP2_NO_PRESCAN_POWERUP))
+		mmc_power_up(host);
 	mmc_detect_change(host, 0);
 }
 
diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
index 2592ddd..7bcf74b 100644
--- a/drivers/mmc/host/sdhci-acpi.c
+++ b/drivers/mmc/host/sdhci-acpi.c
@@ -195,6 +195,8 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
 		host->mmc->pm_caps  |= c->slot->pm_caps;
 	}
 
+	host->mmc->caps2 |= MMC_CAP2_NO_PRESCAN_POWERUP;
+
 	err = sdhci_add_host(host);
 	if (err)
 		goto err_free;
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 17d7148..8873e83 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -280,6 +280,7 @@ struct mmc_host {
 #define MMC_CAP2_PACKED_WR	(1 << 13)	/* Allow packed write */
 #define MMC_CAP2_PACKED_CMD	(MMC_CAP2_PACKED_RD | \
 				 MMC_CAP2_PACKED_WR)
+#define MMC_CAP2_NO_PRESCAN_POWERUP (1 << 14)	/* Don't power up before scan */
 
 	mmc_pm_flag_t		pm_caps;	/* supported pm features */
 
-- 
1.7.11.7

Re: [PATCH 2/2] mmc: core: fix performance regression initializing MMC host controllers

[Ulf Hansson]

On 5 April 2013 09:26, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 05/04/13 00:02, Ulf Hansson wrote:
>> On 4 April 2013 15:41, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> Commit fa5501890d8974301042e0202d342a6cbe8609f4 introduced a performance
>>> regression by adding mmc_power_up() to mmc_start_host().  mmc_power_up()
>>> is not necessary to host controller initialization, it is part of card
>>> initialization and is performed anyway asynchronously.
>>
>> This commit message is a bit miss-leading. In eMMC case, when the host
>> driver has no possibility of power cycle the VCCQ power supply but
>> only the VCC, this is the only proper way to prevent violation of the
>> eMMC spec.
>
> But that is not true.  The host controller driver can manage the voltages.
> The patch is a workaround for the regulator_init_complete late initcall.

In my view, the host driver is not responsible for implementing the
mmc/sd/sdio protocol as such, that is the core layer responsibility.
Moreover I don't think you should consider this as a workaround, it is
a proper fix to make sure we follow eMMC spec.

I noticed you V2 patch, but would still like some more clear
information in there and maybe mention "boot time performance" instead
of just "performance".

Kind regards
Ulf Hansson

>
> I deliberately did not paraphrase the original commit so that people who
> wanted to know would have to look at it for themselves.  I really cannot add
> things to my commit message that do not make sense to me.
>
>>
>>>From SD-card point of view, it is not needed, so this can be optimized
>> to be done as before in an asynchronous mode.
>>
>>>
>>> This patch allows a driver to leave the power up in asynchronous code
>>> (as it was before).
>>>
>>> On my current target platform this reduces driver initialization from:
>>>
>>> [    1.313220] initcall sdhci_acpi_driver_init+0x0/0x12 returned 0 after 102008 usecs
>>>
>>> to this:
>>>
>>> [    1.217209] initcall sdhci_acpi_driver_init+0x0/0x12 returned 0 after 8331 usecs
>>>
>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>>> ---
>>>  drivers/mmc/core/core.c       | 3 ++-
>>>  drivers/mmc/host/sdhci-acpi.c | 2 ++
>>>  include/linux/mmc/host.h      | 1 +
>>>  3 files changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>> index 3bf1c46..c1893c9 100644
>>> --- a/drivers/mmc/core/core.c
>>> +++ b/drivers/mmc/core/core.c
>>> @@ -2416,7 +2416,8 @@ void mmc_start_host(struct mmc_host *host)
>>>  {
>>>         host->f_init = max(freqs[0], host->f_min);
>>>         host->rescan_disable = 0;
>>> -       mmc_power_up(host);
>>> +       if (!(host->caps2 & MMC_CAP2_NO_PRESCAN_POWERUP))
>>> +               mmc_power_up(host);
>>>         mmc_detect_change(host, 0);
>>>  }
>>>
>>> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
>>> index 2592ddd..7bcf74b 100644
>>> --- a/drivers/mmc/host/sdhci-acpi.c
>>> +++ b/drivers/mmc/host/sdhci-acpi.c
>>> @@ -195,6 +195,8 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
>>>                 host->mmc->pm_caps  |= c->slot->pm_caps;
>>>         }
>>>
>>> +       host->mmc->caps2 |= MMC_CAP2_NO_PRESCAN_POWERUP;
>>> +
>>>         err = sdhci_add_host(host);
>>>         if (err)
>>>                 goto err_free;
>>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>>> index 17d7148..8873e83 100644
>>> --- a/include/linux/mmc/host.h
>>> +++ b/include/linux/mmc/host.h
>>> @@ -280,6 +280,7 @@ struct mmc_host {
>>>  #define MMC_CAP2_PACKED_WR     (1 << 13)       /* Allow packed write */
>>>  #define MMC_CAP2_PACKED_CMD    (MMC_CAP2_PACKED_RD | \
>>>                                  MMC_CAP2_PACKED_WR)
>>> +#define MMC_CAP2_NO_PRESCAN_POWERUP (1 << 14)  /* Don't power up before scan */
>>>
>>>         mmc_pm_flag_t           pm_caps;        /* supported pm features */
>>>
>>> --
>>> 1.7.11.7
>>>
>>
>>
>> Some update to the commit msg, then I am happy.
>>
>> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
>>
>>
>

Re: [PATCH 2/2] mmc: core: fix performance regression initializing MMC host controllers

[Adrian Hunter]

On 05/04/13 12:49, Ulf Hansson wrote:
> On 5 April 2013 09:26, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 05/04/13 00:02, Ulf Hansson wrote:
>>> On 4 April 2013 15:41, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>> Commit fa5501890d8974301042e0202d342a6cbe8609f4 introduced a performance
>>>> regression by adding mmc_power_up() to mmc_start_host().  mmc_power_up()
>>>> is not necessary to host controller initialization, it is part of card
>>>> initialization and is performed anyway asynchronously.
>>>
>>> This commit message is a bit miss-leading. In eMMC case, when the host
>>> driver has no possibility of power cycle the VCCQ power supply but
>>> only the VCC, this is the only proper way to prevent violation of the
>>> eMMC spec.
>>
>> But that is not true.  The host controller driver can manage the voltages.
>> The patch is a workaround for the regulator_init_complete late initcall.
> 
> In my view, the host driver is not responsible for implementing the
> mmc/sd/sdio protocol as such, that is the core layer responsibility.

Having VCC always on is not part of the MMC protocol.  For example, you
can't look up EXT_CSD and find out if VCC is always on.  Consequently,
the core does not support it.  If a platform requires it, it should be
added as a new feature and flagged as such, not introduced as a "fix".

> Moreover I don't think you should consider this as a workaround, it is
> a proper fix to make sure we follow eMMC spec.

It won't work if the host controller driver is loaded as a module.

Also it causes attempts to initialize cards without first powering off
thereby ensuring they are in a known state.

> 
> I noticed you V2 patch, but would still like some more clear
> information in there and maybe mention "boot time performance" instead
> of just "performance".



> 
> Kind regards
> Ulf Hansson
> 
>>
>> I deliberately did not paraphrase the original commit so that people who
>> wanted to know would have to look at it for themselves.  I really cannot add
>> things to my commit message that do not make sense to me.
>>
>>>
>>> >From SD-card point of view, it is not needed, so this can be optimized
>>> to be done as before in an asynchronous mode.
>>>
>>>>
>>>> This patch allows a driver to leave the power up in asynchronous code
>>>> (as it was before).
>>>>
>>>> On my current target platform this reduces driver initialization from:
>>>>
>>>> [    1.313220] initcall sdhci_acpi_driver_init+0x0/0x12 returned 0 after 102008 usecs
>>>>
>>>> to this:
>>>>
>>>> [    1.217209] initcall sdhci_acpi_driver_init+0x0/0x12 returned 0 after 8331 usecs
>>>>
>>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>>>> ---
>>>>  drivers/mmc/core/core.c       | 3 ++-
>>>>  drivers/mmc/host/sdhci-acpi.c | 2 ++
>>>>  include/linux/mmc/host.h      | 1 +
>>>>  3 files changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>>> index 3bf1c46..c1893c9 100644
>>>> --- a/drivers/mmc/core/core.c
>>>> +++ b/drivers/mmc/core/core.c
>>>> @@ -2416,7 +2416,8 @@ void mmc_start_host(struct mmc_host *host)
>>>>  {
>>>>         host->f_init = max(freqs[0], host->f_min);
>>>>         host->rescan_disable = 0;
>>>> -       mmc_power_up(host);
>>>> +       if (!(host->caps2 & MMC_CAP2_NO_PRESCAN_POWERUP))
>>>> +               mmc_power_up(host);
>>>>         mmc_detect_change(host, 0);
>>>>  }
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
>>>> index 2592ddd..7bcf74b 100644
>>>> --- a/drivers/mmc/host/sdhci-acpi.c
>>>> +++ b/drivers/mmc/host/sdhci-acpi.c
>>>> @@ -195,6 +195,8 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
>>>>                 host->mmc->pm_caps  |= c->slot->pm_caps;
>>>>         }
>>>>
>>>> +       host->mmc->caps2 |= MMC_CAP2_NO_PRESCAN_POWERUP;
>>>> +
>>>>         err = sdhci_add_host(host);
>>>>         if (err)
>>>>                 goto err_free;
>>>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>>>> index 17d7148..8873e83 100644
>>>> --- a/include/linux/mmc/host.h
>>>> +++ b/include/linux/mmc/host.h
>>>> @@ -280,6 +280,7 @@ struct mmc_host {
>>>>  #define MMC_CAP2_PACKED_WR     (1 << 13)       /* Allow packed write */
>>>>  #define MMC_CAP2_PACKED_CMD    (MMC_CAP2_PACKED_RD | \
>>>>                                  MMC_CAP2_PACKED_WR)
>>>> +#define MMC_CAP2_NO_PRESCAN_POWERUP (1 << 14)  /* Don't power up before scan */
>>>>
>>>>         mmc_pm_flag_t           pm_caps;        /* supported pm features */
>>>>
>>>> --
>>>> 1.7.11.7
>>>>
>>>
>>>
>>> Some update to the commit msg, then I am happy.
>>>
>>> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>
>>>
>>
> 
> 

[PATCH V3 2/2] mmc: core: fix performance regression initializing MMC host controllers

[Adrian Hunter]

Commit fa5501890d8974301042e0202d342a6cbe8609f4 introduced a boot time
performance regression by adding mmc_power_up() to mmc_start_host().
mmc_power_up() is not necessary to host controller initialization, it is
part of card initialization and is performed anyway asynchronously.
Please see the original commit for an explanation of the use of
mmc_power_up() in mmc_start_host().

This patch allows a driver to leave the power up in asynchronous code
(as it was before).

On my current target platform this reduces driver initialization from:

[    1.313220] initcall sdhci_acpi_driver_init+0x0/0x12 returned 0 after 102008 usecs

to this:

[    1.217209] initcall sdhci_acpi_driver_init+0x0/0x12 returned 0 after 8331 usecs

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/core/core.c       | 3 ++-
 drivers/mmc/host/sdhci-acpi.c | 2 ++
 include/linux/mmc/host.h      | 1 +
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 3bf1c46..c1893c9 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2416,7 +2416,8 @@ void mmc_start_host(struct mmc_host *host)
 {
 	host->f_init = max(freqs[0], host->f_min);
 	host->rescan_disable = 0;
-	mmc_power_up(host);
+	if (!(host->caps2 & MMC_CAP2_NO_PRESCAN_POWERUP))
+		mmc_power_up(host);
 	mmc_detect_change(host, 0);
 }
 
diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
index 2592ddd..7bcf74b 100644
--- a/drivers/mmc/host/sdhci-acpi.c
+++ b/drivers/mmc/host/sdhci-acpi.c
@@ -195,6 +195,8 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
 		host->mmc->pm_caps  |= c->slot->pm_caps;
 	}
 
+	host->mmc->caps2 |= MMC_CAP2_NO_PRESCAN_POWERUP;
+
 	err = sdhci_add_host(host);
 	if (err)
 		goto err_free;
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 17d7148..8873e83 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -280,6 +280,7 @@ struct mmc_host {
 #define MMC_CAP2_PACKED_WR	(1 << 13)	/* Allow packed write */
 #define MMC_CAP2_PACKED_CMD	(MMC_CAP2_PACKED_RD | \
 				 MMC_CAP2_PACKED_WR)
+#define MMC_CAP2_NO_PRESCAN_POWERUP (1 << 14)	/* Don't power up before scan */
 
 	mmc_pm_flag_t		pm_caps;	/* supported pm features */
 
-- 
1.7.11.7

Re: [PATCH V3 2/2] mmc: core: fix performance regression initializing MMC host controllers

[Ulf Hansson]

On 5 April 2013 15:31, Adrian Hunter <adrian.hunter@intel.com> wrote:
> Commit fa5501890d8974301042e0202d342a6cbe8609f4 introduced a boot time
> performance regression by adding mmc_power_up() to mmc_start_host().
> mmc_power_up() is not necessary to host controller initialization, it is
> part of card initialization and is performed anyway asynchronously.
> Please see the original commit for an explanation of the use of
> mmc_power_up() in mmc_start_host().
>
> This patch allows a driver to leave the power up in asynchronous code
> (as it was before).
>
> On my current target platform this reduces driver initialization from:
>
> [    1.313220] initcall sdhci_acpi_driver_init+0x0/0x12 returned 0 after 102008 usecs
>
> to this:
>
> [    1.217209] initcall sdhci_acpi_driver_init+0x0/0x12 returned 0 after 8331 usecs
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  drivers/mmc/core/core.c       | 3 ++-
>  drivers/mmc/host/sdhci-acpi.c | 2 ++
>  include/linux/mmc/host.h      | 1 +
>  3 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 3bf1c46..c1893c9 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -2416,7 +2416,8 @@ void mmc_start_host(struct mmc_host *host)
>  {
>         host->f_init = max(freqs[0], host->f_min);
>         host->rescan_disable = 0;
> -       mmc_power_up(host);
> +       if (!(host->caps2 & MMC_CAP2_NO_PRESCAN_POWERUP))
> +               mmc_power_up(host);
>         mmc_detect_change(host, 0);
>  }
>
> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
> index 2592ddd..7bcf74b 100644
> --- a/drivers/mmc/host/sdhci-acpi.c
> +++ b/drivers/mmc/host/sdhci-acpi.c
> @@ -195,6 +195,8 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
>                 host->mmc->pm_caps  |= c->slot->pm_caps;
>         }
>
> +       host->mmc->caps2 |= MMC_CAP2_NO_PRESCAN_POWERUP;
> +
>         err = sdhci_add_host(host);
>         if (err)
>                 goto err_free;
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 17d7148..8873e83 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -280,6 +280,7 @@ struct mmc_host {
>  #define MMC_CAP2_PACKED_WR     (1 << 13)       /* Allow packed write */
>  #define MMC_CAP2_PACKED_CMD    (MMC_CAP2_PACKED_RD | \
>                                  MMC_CAP2_PACKED_WR)
> +#define MMC_CAP2_NO_PRESCAN_POWERUP (1 << 14)  /* Don't power up before scan */
>
>         mmc_pm_flag_t           pm_caps;        /* supported pm features */
>
> --
> 1.7.11.7
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[PATCH V4 2/2] mmc: core: fix performance regression initializing MMC host controllers

[Adrian Hunter]

Commit fa5501890d8974301042e0202d342a6cbe8609f4 introduced a boot time
performance regression by adding mmc_power_up() to mmc_start_host().
mmc_power_up() is not necessary to host controller initialization, it is
part of card initialization and is performed anyway asynchronously.
Please see the original commit for an explanation of the use of
mmc_power_up() in mmc_start_host().

This patch allows a driver to leave the power up in asynchronous code
(as it was before).

On my current target platform this reduces driver initialization from:

[    1.313220] initcall sdhci_acpi_driver_init+0x0/0x12 returned 0 after 102008 usecs

to this:

[    1.217209] initcall sdhci_acpi_driver_init+0x0/0x12 returned 0 after 8331 usecs

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---


Changes for V4:
	- use mmc_power_off() for MMC_CAP2_NO_PRESCAN_POWERUP
	- add MMC_CAP2_NO_PRESCAN_POWERUP to sdhci-pci.c also


 drivers/mmc/core/core.c       | 5 ++++-
 drivers/mmc/host/sdhci-acpi.c | 2 ++
 drivers/mmc/host/sdhci-pci.c  | 1 +
 include/linux/mmc/host.h      | 1 +
 4 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 3bf1c46..65f9ca7 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2416,7 +2416,10 @@ void mmc_start_host(struct mmc_host *host)
 {
 	host->f_init = max(freqs[0], host->f_min);
 	host->rescan_disable = 0;
-	mmc_power_up(host);
+	if (host->caps2 & MMC_CAP2_NO_PRESCAN_POWERUP)
+		mmc_power_off(host);
+	else
+		mmc_power_up(host);
 	mmc_detect_change(host, 0);
 }
 
diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c
index 2592ddd..7bcf74b 100644
--- a/drivers/mmc/host/sdhci-acpi.c
+++ b/drivers/mmc/host/sdhci-acpi.c
@@ -195,6 +195,8 @@ static int sdhci_acpi_probe(struct platform_device *pdev)
 		host->mmc->pm_caps  |= c->slot->pm_caps;
 	}
 
+	host->mmc->caps2 |= MMC_CAP2_NO_PRESCAN_POWERUP;
+
 	err = sdhci_add_host(host);
 	if (err)
 		goto err_free;
diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
index c1f0372..0012d3f 100644
--- a/drivers/mmc/host/sdhci-pci.c
+++ b/drivers/mmc/host/sdhci-pci.c
@@ -1280,6 +1280,7 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot(
 
 	host->mmc->pm_caps = MMC_PM_KEEP_POWER | MMC_PM_WAKE_SDIO_IRQ;
 	host->mmc->slotno = slotno;
+	host->mmc->caps2 |= MMC_CAP2_NO_PRESCAN_POWERUP;
 
 	ret = sdhci_add_host(host);
 	if (ret)
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 17d7148..8873e83 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -280,6 +280,7 @@ struct mmc_host {
 #define MMC_CAP2_PACKED_WR	(1 << 13)	/* Allow packed write */
 #define MMC_CAP2_PACKED_CMD	(MMC_CAP2_PACKED_RD | \
 				 MMC_CAP2_PACKED_WR)
+#define MMC_CAP2_NO_PRESCAN_POWERUP (1 << 14)	/* Don't power up before scan */
 
 	mmc_pm_flag_t		pm_caps;	/* supported pm features */
 
-- 
1.7.11.7

Re: [PATCH 0/2] sdhci-acpi initialization performance regression

[Chris Ball]

Hi Adrian,

On Thu, Apr 04 2013, Adrian Hunter wrote:
> When I submitted the sdhci-acpi driver, it's initialization performance
> was satisfactory.  Now it is not.  Here are 2 fixes to restore that
> regression.
>
> Adrian Hunter (2):
>       Revert "mmc: core: wait while adding MMC host to ensure root mounts successfully"
>       mmc: core: fix performance regression initializing MMC host controllers
>
>  drivers/mmc/core/core.c       | 4 ++--
>  drivers/mmc/host/sdhci-acpi.c | 2 ++
>  include/linux/mmc/host.h      | 1 +
>  3 files changed, 5 insertions(+), 2 deletions(-)

Thanks, both pushed to mmc-next (using v4 of patch 2/2) for 3.10.

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH 0/2] sdhci-acpi initialization performance regression

[Adrian Hunter]

On 12/04/13 21:09, Chris Ball wrote:
> Hi Adrian,
> 
> On Thu, Apr 04 2013, Adrian Hunter wrote:
>> When I submitted the sdhci-acpi driver, it's initialization performance
>> was satisfactory.  Now it is not.  Here are 2 fixes to restore that
>> regression.
>>
>> Adrian Hunter (2):
>>       Revert "mmc: core: wait while adding MMC host to ensure root mounts successfully"
>>       mmc: core: fix performance regression initializing MMC host controllers
>>
>>  drivers/mmc/core/core.c       | 4 ++--
>>  drivers/mmc/host/sdhci-acpi.c | 2 ++
>>  include/linux/mmc/host.h      | 1 +
>>  3 files changed, 5 insertions(+), 2 deletions(-)
> 
> Thanks, both pushed to mmc-next (using v4 of patch 2/2) for 3.10.

Thanks you!

It looks like mmc-next and linux-next have the V1 version of patch 2/2.  Can
you replace it with V4?

Re: [PATCH 0/2] sdhci-acpi initialization performance regression

[Chris Ball]

Hi,

On Mon, Apr 15 2013, Adrian Hunter wrote:
> On 12/04/13 21:09, Chris Ball wrote:
>> Hi Adrian,
>> 
>> On Thu, Apr 04 2013, Adrian Hunter wrote:
>>> When I submitted the sdhci-acpi driver, it's initialization performance
>>> was satisfactory.  Now it is not.  Here are 2 fixes to restore that
>>> regression.
>>>
>>> Adrian Hunter (2):
>>>       Revert "mmc: core: wait while adding MMC host to ensure root mounts successfully"
>>>       mmc: core: fix performance regression initializing MMC host controllers
>>>
>>>  drivers/mmc/core/core.c       | 4 ++--
>>>  drivers/mmc/host/sdhci-acpi.c | 2 ++
>>>  include/linux/mmc/host.h      | 1 +
>>>  3 files changed, 5 insertions(+), 2 deletions(-)
>> 
>> Thanks, both pushed to mmc-next (using v4 of patch 2/2) for 3.10.
>
> Thanks you!
>
> It looks like mmc-next and linux-next have the V1 version of patch 2/2.  Can
> you replace it with V4?

Ouch, I'm sorry about that; thank you for catching it.  I applied the
incremental patch below to bring it up to v4 (to avoid rebasing a
public tree):


From: Adrian Hunter <adrian.hunter@intel.com>
Subject: [PATCH] mmc: core: fix init controller performance regression,
 updated patch

Add MMC_CAP2_NO_PRESCAN_POWERUP to sdhci-pci.c also, use mmc_power_off()
for MMC_CAP2_NO_PRESCAN_POWERUP.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
[cjb: previously applied v1 of this patch instead of v4]
Signed-off-by: Chris Ball <cjb@laptop.org>
---
 drivers/mmc/core/core.c      | 4 +++-
 drivers/mmc/host/sdhci-pci.c | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index c1893c9..65f9ca7 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2416,7 +2416,9 @@ void mmc_start_host(struct mmc_host *host)
 {
 	host->f_init = max(freqs[0], host->f_min);
 	host->rescan_disable = 0;
-	if (!(host->caps2 & MMC_CAP2_NO_PRESCAN_POWERUP))
+	if (host->caps2 & MMC_CAP2_NO_PRESCAN_POWERUP)
+		mmc_power_off(host);
+	else
 		mmc_power_up(host);
 	mmc_detect_change(host, 0);
 }
diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
index c1f0372..0012d3f 100644
--- a/drivers/mmc/host/sdhci-pci.c
+++ b/drivers/mmc/host/sdhci-pci.c
@@ -1280,6 +1280,7 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot(
 
 	host->mmc->pm_caps = MMC_PM_KEEP_POWER | MMC_PM_WAKE_SDIO_IRQ;
 	host->mmc->slotno = slotno;
+	host->mmc->caps2 |= MMC_CAP2_NO_PRESCAN_POWERUP;
 
 	ret = sdhci_add_host(host);
 	if (ret)
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH 0/2] sdhci-acpi initialization performance regression

[Adrian Hunter]

On 15/04/13 18:42, Chris Ball wrote:
> Hi,
>
> On Mon, Apr 15 2013, Adrian Hunter wrote:
>> On 12/04/13 21:09, Chris Ball wrote:
>>> Hi Adrian,
>>>
>>> On Thu, Apr 04 2013, Adrian Hunter wrote:
>>>> When I submitted the sdhci-acpi driver, it's initialization performance
>>>> was satisfactory.  Now it is not.  Here are 2 fixes to restore that
>>>> regression.
>>>>
>>>> Adrian Hunter (2):
>>>>       Revert "mmc: core: wait while adding MMC host to ensure root mounts successfully"
>>>>       mmc: core: fix performance regression initializing MMC host controllers
>>>>
>>>>  drivers/mmc/core/core.c       | 4 ++--
>>>>  drivers/mmc/host/sdhci-acpi.c | 2 ++
>>>>  include/linux/mmc/host.h      | 1 +
>>>>  3 files changed, 5 insertions(+), 2 deletions(-)
>>> Thanks, both pushed to mmc-next (using v4 of patch 2/2) for 3.10.
>> Thanks you!
>>
>> It looks like mmc-next and linux-next have the V1 version of patch 2/2.  Can
>> you replace it with V4?
> Ouch, I'm sorry about that; thank you for catching it.  I applied the
> incremental patch below to bring it up to v4 (to avoid rebasing a
> public tree):

That is fine with me.  Is it in any tree?

>
>
> From: Adrian Hunter <adrian.hunter@intel.com>
> Subject: [PATCH] mmc: core: fix init controller performance regression,
>  updated patch
>
> Add MMC_CAP2_NO_PRESCAN_POWERUP to sdhci-pci.c also, use mmc_power_off()
> for MMC_CAP2_NO_PRESCAN_POWERUP.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> [cjb: previously applied v1 of this patch instead of v4]
> Signed-off-by: Chris Ball <cjb@laptop.org>
> ---
>  drivers/mmc/core/core.c      | 4 +++-
>  drivers/mmc/host/sdhci-pci.c | 1 +
>  2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index c1893c9..65f9ca7 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -2416,7 +2416,9 @@ void mmc_start_host(struct mmc_host *host)
>  {
>  	host->f_init = max(freqs[0], host->f_min);
>  	host->rescan_disable = 0;
> -	if (!(host->caps2 & MMC_CAP2_NO_PRESCAN_POWERUP))
> +	if (host->caps2 & MMC_CAP2_NO_PRESCAN_POWERUP)
> +		mmc_power_off(host);
> +	else
>  		mmc_power_up(host);
>  	mmc_detect_change(host, 0);
>  }
> diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
> index c1f0372..0012d3f 100644
> --- a/drivers/mmc/host/sdhci-pci.c
> +++ b/drivers/mmc/host/sdhci-pci.c
> @@ -1280,6 +1280,7 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot(
>  
>  	host->mmc->pm_caps = MMC_PM_KEEP_POWER | MMC_PM_WAKE_SDIO_IRQ;
>  	host->mmc->slotno = slotno;
> +	host->mmc->caps2 |= MMC_CAP2_NO_PRESCAN_POWERUP;
>  
>  	ret = sdhci_add_host(host);
>  	if (ret)