sdhci-acpi initialization performance regression

PATCH In review Adrian Hunter 4 participants 16 messages Apr 4, 2013 – Apr 17, 2013

Subsystems (2)

MMC, SD and SDIO Secure digital host controller interface (SDHCI) driver

lore

Share & tools

lore

Adrian Hunter

author maintainer

2013-04-04 13:36 UTC #1 lore raw

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

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 <[email protected]>

---
 drivers/mmc/core/core.c | 1 -
 1 file changed, 1 deletion(-)

drivers/mmc/core/core.c +0 -1

index ad7decc..3bf1c46 100644
@@ -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)

Signature · 1 line

1.7.11.7

Sergey Yanovich

2013-04-04 13:52 UTC #2 lore raw

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

drivers/mmc/core/core.c void mmc_start_host(struct mmc_host *host) · line 2423

Quoted 12 lines · Adrian Hunter

drivers/mmc/core/core.c +0 -1

index ad7decc..3bf1c46 100644
@@ -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.

Adrian Hunter

author maintainer

2013-04-04 13:36 UTC #3 lore raw

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

In reply to 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 <[email protected]>

---
 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(-)

drivers/mmc/core/core.c +2 -1

index 3bf1c46..c1893c9 100644
@@ -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);
 }

drivers/mmc/host/sdhci-acpi.c +2 -0

index 2592ddd..7bcf74b 100644
@@ -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;

include/linux/mmc/host.h +1 -0

index 17d7148..8873e83 100644
@@ -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 */

Signature · 1 line

1.7.11.7

Ulf Hansson

Reviewed-by on v1 2/2

2013-04-04 21:02 UTC #4 lore raw

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

Quoted 4 lines · 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 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.

Quoted 1 line

From SD-card point of view, it is not needed, so this can be optimized

to be done as before in an asynchronous mode.

Quoted 59 lines

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 <[email protected]>

---
 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(-)

drivers/mmc/core/core.c +2 -1

index 3bf1c46..c1893c9 100644
@@ -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);
 }

drivers/mmc/host/sdhci-acpi.c +2 -0

index 2592ddd..7bcf74b 100644
@@ -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;

include/linux/mmc/host.h +1 -1

index 17d7148..8873e83 100644
@@ -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 */
 
--

Quoted 2 lines

1.7.11.7

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

Reviewed-by Ulf Hansson <[email protected]>

Adrian Hunter

author maintainer

2013-04-05 07:21 UTC #5 lore raw

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

Quoted 10 lines · Ulf Hansson

Quoted 4 lines · 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 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.

Quoted 72 lines

Quoted 2 lines

From SD-card point of view, it is not needed, so this can be optimized

to be done as before in an asynchronous mode.

Quoted 59 lines

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 <[email protected]>

---
 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(-)

drivers/mmc/core/core.c +2 -1

index 3bf1c46..c1893c9 100644
@@ -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);
 }

drivers/mmc/host/sdhci-acpi.c +2 -0

index 2592ddd..7bcf74b 100644
@@ -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;

include/linux/mmc/host.h +1 -1

index 17d7148..8873e83 100644
@@ -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 */
 
--

Quoted 2 lines

1.7.11.7

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

Reviewed-by Ulf Hansson <[email protected]>

Adrian Hunter

author maintainer

2013-04-05 07:35 UTC #6 lore raw

PATCH v2 2/2 mmc: core: fix performance regression initializing MMC, host controllers

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 <[email protected]>

---
 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(-)

drivers/mmc/core/core.c +2 -1

index 3bf1c46..c1893c9 100644
@@ -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);
 }

drivers/mmc/host/sdhci-acpi.c +2 -0

index 2592ddd..7bcf74b 100644
@@ -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;

include/linux/mmc/host.h +1 -0

index 17d7148..8873e83 100644
@@ -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 */

Signature · 1 line

1.7.11.7

Ulf Hansson

2013-04-05 09:49 UTC #7 lore raw

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

In reply to Adrian Hunter

Quoted 7 lines · Adrian Hunter

Quoted 6 lines · Ulf Hansson
Quoted 4 lines · 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.

Quoted 7 lines

Quoted 4 lines
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

Quoted 78 lines

Quoted 72 lines

Quoted 2 lines

From SD-card point of view, it is not needed, so this can be optimized

to be done as before in an asynchronous mode.

Quoted 59 lines

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 <[email protected]>

---
 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(-)

drivers/mmc/core/core.c +2 -1

index 3bf1c46..c1893c9 100644
@@ -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);
 }

drivers/mmc/host/sdhci-acpi.c +2 -0

index 2592ddd..7bcf74b 100644
@@ -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;

include/linux/mmc/host.h +1 -1

index 17d7148..8873e83 100644
@@ -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 */
 
--

Quoted 2 lines

1.7.11.7

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

Reviewed-by Ulf Hansson <[email protected]>

Adrian Hunter

author maintainer

2013-04-05 13:24 UTC #8 lore raw

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

Quoted 13 lines · Ulf Hansson

Quoted 12 lines · Adrian Hunter
Quoted 10 lines · Ulf Hansson
Quoted 4 lines · 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 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.

Quoted 5 lines

Quoted 2 lines
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".

Quoted 2 lines

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.

Quoted 91 lines

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
Quoted 78 lines
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.
Quoted 72 lines
Quoted 2 lines
From SD-card point of view, it is not needed, so this can be optimized
to be done as before in an asynchronous mode.
Quoted 59 lines
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 <[email protected]>
---
 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(-)
drivers/mmc/core/core.c +2 -1
index 3bf1c46..c1893c9 100644
@@ -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);
 }
 
drivers/mmc/host/sdhci-acpi.c +2 -0
index 2592ddd..7bcf74b 100644
@@ -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;
include/linux/mmc/host.h +1 -1
index 17d7148..8873e83 100644
@@ -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 */
 
--
Quoted 2 lines
1.7.11.7
Some update to the commit msg, then I am happy.
Reviewed-by Ulf Hansson <[email protected]>

Adrian Hunter

author maintainer

2013-04-05 13:25 UTC #9 lore raw

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

In reply to Ulf Hansson

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 <[email protected]>

---
 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(-)

drivers/mmc/core/core.c +2 -1

index 3bf1c46..c1893c9 100644
@@ -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);
 }

drivers/mmc/host/sdhci-acpi.c +2 -0

index 2592ddd..7bcf74b 100644
@@ -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;

include/linux/mmc/host.h +1 -0

index 17d7148..8873e83 100644
@@ -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 */

Signature · 1 line

1.7.11.7

Ulf Hansson

Acked-by on v3 2/2

2013-04-08 09:41 UTC #10 lore raw

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

Quoted 65 lines · Adrian Hunter

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 <[email protected]>

---
 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(-)

drivers/mmc/core/core.c +2 -1

index 3bf1c46..c1893c9 100644
@@ -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);
 }

drivers/mmc/host/sdhci-acpi.c +2 -0

index 2592ddd..7bcf74b 100644
@@ -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;

include/linux/mmc/host.h +1 -1

index 17d7148..8873e83 100644
@@ -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 */
 
--

Quoted 5 lines

1.7.11.7
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Acked-by Ulf Hansson <[email protected]>

Adrian Hunter

author maintainer

2013-04-08 10:44 UTC #11 lore raw

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

In reply to Adrian Hunter

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 <[email protected]>

---

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(-)

drivers/mmc/core/core.c +4 -1

index 3bf1c46..65f9ca7 100644
@@ -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);
 }

drivers/mmc/host/sdhci-acpi.c +2 -0

index 2592ddd..7bcf74b 100644
@@ -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;

drivers/mmc/host/sdhci-pci.c +1 -0

index c1f0372..0012d3f 100644
@@ -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)

include/linux/mmc/host.h +1 -0

index 17d7148..8873e83 100644
@@ -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 */

Signature · 1 line

1.7.11.7

Chris Ball

2013-04-12 18:09 UTC #12 lore raw

In reply to Adrian Hunter

Hi Adrian,

Quoted 4 lines · Adrian Hunter

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.

Quoted 8 lines

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.

Signature · 2 lines

Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child

Adrian Hunter

author maintainer

2013-04-15 06:42 UTC #13 lore raw

Quoted 11 lines · Chris Ball

Hi Adrian,
Quoted 8 lines · Adrian Hunter
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

Quoted 6 lines

Quoted 4 lines
 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?

Chris Ball

2013-04-15 15:42 UTC #14 lore raw

Hi,

Quoted 17 lines · Adrian Hunter

Quoted 16 lines · Chris Ball
Hi Adrian,
Quoted 12 lines · Adrian Hunter
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(-)

Quoted 6 lines

Quoted 1 line
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 <[email protected]>
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 <[email protected]>

[cjb: previously applied v1 of this patch instead of v4]

Signed-off-by Chris Ball <[email protected]>

---
 drivers/mmc/core/core.c      | 4 +++-
 drivers/mmc/host/sdhci-pci.c | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

drivers/mmc/core/core.c +3 -1

index c1893c9..65f9ca7 100644
@@ -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);
 }

drivers/mmc/host/sdhci-pci.c +1 -0

index c1f0372..0012d3f 100644
@@ -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)

Signature · 2 lines

Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child

Adrian Hunter

author maintainer

2013-04-17 05:50 UTC #15 lore raw

Quoted 19 lines · Chris Ball

Hi,
Quoted 16 lines · Adrian Hunter
Quoted 15 lines · Chris Ball
Hi Adrian,
Quoted 12 lines · Adrian Hunter
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(-)

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
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?

Quoted 44 lines

From: Adrian Hunter <[email protected]>
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 <[email protected]>
[cjb: previously applied v1 of this patch instead of v4]
Signed-off-by Chris Ball <[email protected]>
---
 drivers/mmc/core/core.c      | 4 +++-
 drivers/mmc/host/sdhci-pci.c | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)
drivers/mmc/core/core.c +3 -1
index c1893c9..65f9ca7 100644
@@ -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);
 }
drivers/mmc/host/sdhci-pci.c +1 -0
index c1f0372..0012d3f 100644
@@ -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)