On 02/05/13 14:35, Ulf Hansson wrote: > On 2 May 2013 12:38, Adrian Hunter <adrian.hunter@xxxxxxxxx> wrote: >> On 16/04/13 13:00, Ulf Hansson wrote: >>> Aggressive power management is suitable when saving power is >>> essential. At request inactivity timeout, aka pm runtime >>> autosuspend timeout, the card will be suspended. >>> >>> Once a new request arrives, the card will be re-initalized and >>> thus the first request will suffer from a latency. This latency >>> is card-specific, experiments has shown in general that SD-cards >>> has quite poor initialization time, around 300ms-1100ms. eMMC is >>> not surprisingly far better but still a couple of hundreds of ms >>> has been observed. >>> >>> Except for the request latency, it is important to know that >>> suspending the card will also prevent the card from executing >>> internal house-keeping operations in idle mode. This could mean >>> degradation in performance. >>> >>> To use this feature make sure the request inactivity timeout is >>> chosen carefully. This has not been done as a part of this patch. >>> >>> Enable this feature by using host cap MMC_CAP_AGGRESSIVE_PM and >>> by setting CONFIG_MMC_UNSAFE_RESUME. >>> >>> Signed-off-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx> >>> Cc: Maya Erez <merez@xxxxxxxxxxxxxx> >>> Cc: Subhash Jadavani <subhashj@xxxxxxxxxxxxxx> >>> Cc: Arnd Bergmann <arnd@xxxxxxxx> >>> Cc: Kevin Liu <kliu5@xxxxxxxxxxx> >>> Cc: Adrian Hunter <adrian.hunter@xxxxxxxxx> >>> Cc: Daniel Drake <dsd@xxxxxxxxxx> >>> Cc: Ohad Ben-Cohen <ohad@xxxxxxxxxx> >>> --- >>> drivers/mmc/core/mmc.c | 43 +++++++++++++++++++++++++++++++++++++++++++ >>> drivers/mmc/core/sd.c | 42 ++++++++++++++++++++++++++++++++++++++++++ >>> include/linux/mmc/host.h | 2 +- >>> 3 files changed, 86 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c >>> index bf19058..8dfbc84 100644 >>> --- a/drivers/mmc/core/mmc.c >>> +++ b/drivers/mmc/core/mmc.c >>> @@ -1454,6 +1454,47 @@ static int mmc_resume(struct mmc_host *host) >>> return err; >>> } >>> >>> + >>> +/* >>> + * Callback for runtime_suspend. >>> + */ >>> +static int mmc_runtime_suspend(struct mmc_host *host) >>> +{ >>> + int err; >>> + >>> + if (!(host->caps & MMC_CAP_AGGRESSIVE_PM)) >>> + return 0; >>> + >> >> mmc_power_off() needs to be within mmc_claim_host() / mmc_release_host(). >> >> Claiming is nested, so you can out put mmc_claim_host() here: >> >> mmc_claim_host(host); > > OK > >> >> >>> + err = mmc_suspend(host); >>> + if (err) { >>> + pr_err("%s: error %d doing aggessive suspend\n", >>> + mmc_hostname(host), err); >>> + return err; >> >> goto out; >> >>> + } >>> + >>> + mmc_power_off(host); >> >> out: >> mmc_release_host(host); >> > > OK > >>> + return err; >>> +} >>> + >>> +/* >>> + * Callback for runtime_resume. >>> + */ >>> +static int mmc_runtime_resume(struct mmc_host *host) >>> +{ >>> + int err; >>> + >>> + if (!(host->caps & MMC_CAP_AGGRESSIVE_PM)) >>> + return 0; >>> + >>> + mmc_power_up(host); >> >> As above >> > > OK > >>> + err = mmc_resume(host); >>> + if (err) >>> + pr_err("%s: error %d doing aggessive resume\n", >>> + mmc_hostname(host), err); >>> + >>> + return err; >> >> The power is on - leaving the device in a RPM_SUSPENDED state does not seem >> useful so better to return zero here. > > OK > >> >>> +} >>> + >>> static int mmc_power_restore(struct mmc_host *host) >>> { >>> int ret; >>> @@ -1514,6 +1555,8 @@ static const struct mmc_bus_ops mmc_ops_unsafe = { >>> .detect = mmc_detect, >>> .suspend = mmc_suspend, >>> .resume = mmc_resume, >>> + .runtime_suspend = mmc_runtime_suspend, >>> + .runtime_resume = mmc_runtime_resume, >>> .power_restore = mmc_power_restore, >>> .alive = mmc_alive, >>> }; >>> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c >>> index 30387d6..e0458f9 100644 >>> --- a/drivers/mmc/core/sd.c >>> +++ b/drivers/mmc/core/sd.c >>> @@ -1095,6 +1095,46 @@ static int mmc_sd_resume(struct mmc_host *host) >>> return err; >>> } >>> >>> +/* >>> + * Callback for runtime_suspend. >>> + */ >>> +static int mmc_sd_runtime_suspend(struct mmc_host *host) >>> +{ >>> + int err; >>> + >>> + if (!(host->caps & MMC_CAP_AGGRESSIVE_PM)) >>> + return 0; >>> + >>> + err = mmc_sd_suspend(host); >>> + if (err) { >>> + pr_err("%s: error %d doing aggessive suspend\n", >>> + mmc_hostname(host), err); >>> + return err; >>> + } >>> + >>> + mmc_power_off(host); >> >> As above > > OK > >> >>> + return err; >>> +} >>> + >>> +/* >>> + * Callback for runtime_resume. >>> + */ >>> +static int mmc_sd_runtime_resume(struct mmc_host *host) >>> +{ >>> + int err; >>> + >>> + if (!(host->caps & MMC_CAP_AGGRESSIVE_PM)) >>> + return 0; >>> + >>> + mmc_power_up(host); >> >> As above > > OK > >> >>> + err = mmc_sd_resume(host); >>> + if (err) >>> + pr_err("%s: error %d doing aggessive resume\n", >>> + mmc_hostname(host), err); >>> + >>> + return err; >> >> As above > > OK > >> >> return 0 >> >>> +} >>> + >>> static int mmc_sd_power_restore(struct mmc_host *host) >>> { >>> int ret; >>> @@ -1119,6 +1159,8 @@ static const struct mmc_bus_ops mmc_sd_ops = { >>> static const struct mmc_bus_ops mmc_sd_ops_unsafe = { >>> .remove = mmc_sd_remove, >>> .detect = mmc_sd_detect, >>> + .runtime_suspend = mmc_sd_runtime_suspend, >>> + .runtime_resume = mmc_sd_runtime_resume, >>> .suspend = mmc_sd_suspend, >>> .resume = mmc_sd_resume, >>> .power_restore = mmc_sd_power_restore, >>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h >>> index 17d7148..cec6684 100644 >>> --- a/include/linux/mmc/host.h >>> +++ b/include/linux/mmc/host.h >>> @@ -239,7 +239,7 @@ struct mmc_host { >>> #define MMC_CAP_SPI (1 << 4) /* Talks only SPI protocols */ >>> #define MMC_CAP_NEEDS_POLL (1 << 5) /* Needs polling for card-detection */ >>> #define MMC_CAP_8_BIT_DATA (1 << 6) /* Can the host do 8 bit transfers */ >>> - >>> +#define MMC_CAP_AGGRESSIVE_PM (1 << 7) /* Suspend (e)MMC/SD at idle */ >> >> Using a "cap" is not ideal here - it should really be under the control of >> user space. > > Several other caps could be debated whether these should exist here as > well, but let's leave that to a separate discussion. > Until there are a solution for how to add new mmc features per host, > which should be runtime configurable, I believe this is the only way > we can do it. Yes, I guess it can be improved later. > > Do note, that runtime pm can be disabled from user space, which > indirectly will prevent this feature from being used. > >> >>> #define MMC_CAP_NONREMOVABLE (1 << 8) /* Nonremovable e.g. eMMC */ >>> #define MMC_CAP_WAIT_WHILE_BUSY (1 << 9) /* Waits while card is busy */ >>> #define MMC_CAP_ERASE (1 << 10) /* Allow erase/trim commands */ >>> >> > > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html