| Message ID | 1459236673-5639-7-git-send-email-ulf.hansson@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 1 April 2016 at 09:46, Adrian Hunter <adrian.hunter@intel.com> wrote: > On 29/03/16 10:31, Ulf Hansson wrote: >> Commit 9250aea76bfc ("mmc: core: Enable runtime PM management of host >> devices"), made some calls to the runtime PM API from the driver >> redundant. Especially those which deals with runtime PM reference >> counting, so let's remove them. >> >> Moreover as SDHCI have its own wrapper functions for runtime PM these >> becomes superfluous, so let's remove them as well. >> >> Cc: Adrian Hunter <adrian.hunter@intel.com> >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > Looks good, but I have one comment below. > [...] >> >> static void sdhci_enable_sdio_irq_nolock(struct sdhci_host *host, int enable) >> @@ -1668,7 +1646,7 @@ static void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable) >> struct sdhci_host *host = mmc_priv(mmc); >> unsigned long flags; >> >> - sdhci_runtime_pm_get(host); >> + pm_runtime_get_sync(mmc_dev(mmc)); > > Why can't this be moved into the core as well? At the least, there should > be a comment here and explanation in the commit message. That's indeed a very good question! :-) SDHCI is a special case for how a host driver deals with SDIO IRQs as it's the only user of MMC_CAP2_SDIO_IRQ_NOTHREAD. It means it has its own SDIO IRQ thread (threaded IRQ) which will invoke sdio_run_irqs() to consume a received SDIO IRQ. Other host drivers are using the mmc_signal_sdio_irq() API to deal with this, which behaves a bit different. The sdio_run_irqs() API claims the host (mmc_claim|release_host()) before it starts to consume the SDIO IRQ. This triggers the host to be runtime resumed before the host ops ->enable_sdio_irq() gets called. So, actually it means it *should* be safe to remove pm_runtime_get|put() in it's current form from within sdhci_enable_sdio_irq()... Although, I wonder how SDHCI can allow to have SDIO IRQs enabled without *keeping* the host runtime resumed, because I fail to see that any wakeups are being configured when the host becomes runtime suspended. I may be missing something, but what do you think? [...] Kind regards Uffe -- 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
[...] >> >>>> >>>> static void sdhci_enable_sdio_irq_nolock(struct sdhci_host *host, int enable) >>>> @@ -1668,7 +1646,7 @@ static void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable) >>>> struct sdhci_host *host = mmc_priv(mmc); >>>> unsigned long flags; >>>> >>>> - sdhci_runtime_pm_get(host); >>>> + pm_runtime_get_sync(mmc_dev(mmc)); >>> >>> Why can't this be moved into the core as well? At the least, there should >>> be a comment here and explanation in the commit message. >> >> That's indeed a very good question! :-) >> >> SDHCI is a special case for how a host driver deals with SDIO IRQs as >> it's the only user of MMC_CAP2_SDIO_IRQ_NOTHREAD. >> >> It means it has its own SDIO IRQ thread (threaded IRQ) which will >> invoke sdio_run_irqs() to consume a received SDIO IRQ. Other host >> drivers are using the mmc_signal_sdio_irq() API to deal with this, >> which behaves a bit different. >> >> The sdio_run_irqs() API claims the host (mmc_claim|release_host()) >> before it starts to consume the SDIO IRQ. This triggers the host to be >> runtime resumed before the host ops ->enable_sdio_irq() gets called. >> So, actually it means it *should* be safe to remove >> pm_runtime_get|put() in it's current form from within >> sdhci_enable_sdio_irq()... > > Sorry for slow reply. > > Yes it looks safe. Please remove it. Okay, v2 is on its way. Thanks! > >> >> Although, I wonder how SDHCI can allow to have SDIO IRQs enabled >> without *keeping* the host runtime resumed, because I fail to see that >> any wakeups are being configured when the host becomes runtime >> suspended. I may be missing something, but what do you think? > > As you probably know, it was done by Russell King, for the sdhci_esdhc-imx > driver by the look of it. The clocks are left on during runtime suspend iff > the SDIO IRQ is enabled, and IIRC it says the registers are accessible. I > would not expect any wakeups would need to be configured because the system > is not asleep. I don't know exactly what Russell was aiming for, but one > advantage is that the host controller will go to a lower power state (clocks > off) when the SDIO IRQ is not enabled. My interpretation of how runtime PM should be deployed, it's s that a device shall not process data in the runtime suspend state. To do that, it first needs to be runtime resumed. I think this conforms to what the runtime PM documentation also recommends in section 2, Device Runtime PM Callbacks. Moreover I think it's an SoC specific detail whether a driver *can* access a device's registers in the runtime suspend state (thus it shouldn't do it). Especially, devices may reside in a PM domain, which could enter a retention state, because all its devices are runtime suspended. Typically for an ARM SoC is that devices looses their register's context in such retention state. Regarding remote wake-ups, if the device supports it, this could be configured to trigger the device to be runtime resumed to allow it to start process data again. Typically it means the wake-up IRQ handler should call pm_runtime_resume() (or similar) to wake up the device (and its potential PM domain). In the MMC/SD/SDIO case, the relevant wake-ups settings concerns card detect IRQ and the SDIO IRQ. For runtime PM deployment; if SDIO IRQ is enabled but no remote wake-up is supported, I think runtime PM suspend must be prevented. Either via returning -EBUSY from the ->runtime_suspend() callback or by increasing the runtime PM usage count. The similar applies to card detect IRQ regarding runtime PM deployment. For system PM, there is a few additional parameters to consider. In some cases we might not care about wake-ups but in some other we do, MMC_PM_WAKE_SDIO_IRQ is one of these cases. I guess I should spend some time documenting this for MMC. :-) Kind regards Uffe -- 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
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index 8670f16..4bb9bfd 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -56,19 +56,9 @@ static void sdhci_enable_preset_value(struct sdhci_host *host, bool enable); static int sdhci_do_get_cd(struct sdhci_host *host); #ifdef CONFIG_PM -static int sdhci_runtime_pm_get(struct sdhci_host *host); -static int sdhci_runtime_pm_put(struct sdhci_host *host); static void sdhci_runtime_pm_bus_on(struct sdhci_host *host); static void sdhci_runtime_pm_bus_off(struct sdhci_host *host); #else -static inline int sdhci_runtime_pm_get(struct sdhci_host *host) -{ - return 0; -} -static inline int sdhci_runtime_pm_put(struct sdhci_host *host) -{ - return 0; -} static void sdhci_runtime_pm_bus_on(struct sdhci_host *host) { } @@ -1298,8 +1288,6 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq) host = mmc_priv(mmc); - sdhci_runtime_pm_get(host); - /* Firstly check card presence */ present = mmc->ops->get_cd(mmc); @@ -1546,9 +1534,7 @@ static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) { struct sdhci_host *host = mmc_priv(mmc); - sdhci_runtime_pm_get(host); sdhci_do_set_ios(host, ios); - sdhci_runtime_pm_put(host); } static int sdhci_do_get_cd(struct sdhci_host *host) @@ -1580,12 +1566,8 @@ static int sdhci_do_get_cd(struct sdhci_host *host) static int sdhci_get_cd(struct mmc_host *mmc) { struct sdhci_host *host = mmc_priv(mmc); - int ret; - sdhci_runtime_pm_get(host); - ret = sdhci_do_get_cd(host); - sdhci_runtime_pm_put(host); - return ret; + return sdhci_do_get_cd(host); } static int sdhci_check_ro(struct sdhci_host *host) @@ -1641,12 +1623,8 @@ static void sdhci_hw_reset(struct mmc_host *mmc) static int sdhci_get_ro(struct mmc_host *mmc) { struct sdhci_host *host = mmc_priv(mmc); - int ret; - sdhci_runtime_pm_get(host); - ret = sdhci_do_get_ro(host); - sdhci_runtime_pm_put(host); - return ret; + return sdhci_do_get_ro(host); } static void sdhci_enable_sdio_irq_nolock(struct sdhci_host *host, int enable) @@ -1668,7 +1646,7 @@ static void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable) struct sdhci_host *host = mmc_priv(mmc); unsigned long flags; - sdhci_runtime_pm_get(host); + pm_runtime_get_sync(mmc_dev(mmc)); spin_lock_irqsave(&host->lock, flags); if (enable) @@ -1679,7 +1657,8 @@ static void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable) sdhci_enable_sdio_irq_nolock(host, enable); spin_unlock_irqrestore(&host->lock, flags); - sdhci_runtime_pm_put(host); + pm_runtime_mark_last_busy(mmc_dev(mmc)); + pm_runtime_put_autosuspend(mmc_dev(mmc)); } static int sdhci_do_start_signal_voltage_switch(struct sdhci_host *host, @@ -1777,14 +1756,11 @@ static int sdhci_start_signal_voltage_switch(struct mmc_host *mmc, struct mmc_ios *ios) { struct sdhci_host *host = mmc_priv(mmc); - int err; if (host->version < SDHCI_SPEC_300) return 0; - sdhci_runtime_pm_get(host); - err = sdhci_do_start_signal_voltage_switch(host, ios); - sdhci_runtime_pm_put(host); - return err; + + return sdhci_do_start_signal_voltage_switch(host, ios); } static int sdhci_card_busy(struct mmc_host *mmc) @@ -1792,10 +1768,8 @@ static int sdhci_card_busy(struct mmc_host *mmc) struct sdhci_host *host = mmc_priv(mmc); u32 present_state; - sdhci_runtime_pm_get(host); /* Check whether DAT[3:0] is 0000 */ present_state = sdhci_readl(host, SDHCI_PRESENT_STATE); - sdhci_runtime_pm_put(host); return !(present_state & SDHCI_DATA_LVL_MASK); } @@ -1822,7 +1796,6 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode) unsigned int tuning_count = 0; bool hs400_tuning; - sdhci_runtime_pm_get(host); spin_lock_irqsave(&host->lock, flags); hs400_tuning = host->flags & SDHCI_HS400_TUNING; @@ -1870,7 +1843,6 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode) if (host->ops->platform_execute_tuning) { spin_unlock_irqrestore(&host->lock, flags); err = host->ops->platform_execute_tuning(host, opcode); - sdhci_runtime_pm_put(host); return err; } @@ -2002,8 +1974,6 @@ out: sdhci_writel(host, host->ier, SDHCI_SIGNAL_ENABLE); out_unlock: spin_unlock_irqrestore(&host->lock, flags); - sdhci_runtime_pm_put(host); - return err; } @@ -2201,7 +2171,6 @@ static void sdhci_tasklet_finish(unsigned long param) spin_unlock_irqrestore(&host->lock, flags); mmc_request_done(host->mmc, mrq); - sdhci_runtime_pm_put(host); } static void sdhci_timeout_timer(unsigned long data) @@ -2682,17 +2651,6 @@ int sdhci_resume_host(struct sdhci_host *host) EXPORT_SYMBOL_GPL(sdhci_resume_host); -static int sdhci_runtime_pm_get(struct sdhci_host *host) -{ - return pm_runtime_get_sync(host->mmc->parent); -} - -static int sdhci_runtime_pm_put(struct sdhci_host *host) -{ - pm_runtime_mark_last_busy(host->mmc->parent); - return pm_runtime_put_autosuspend(host->mmc->parent); -} - static void sdhci_runtime_pm_bus_on(struct sdhci_host *host) { if (host->bus_on)
Commit 9250aea76bfc ("mmc: core: Enable runtime PM management of host devices"), made some calls to the runtime PM API from the driver redundant. Especially those which deals with runtime PM reference counting, so let's remove them. Moreover as SDHCI have its own wrapper functions for runtime PM these becomes superfluous, so let's remove them as well. Cc: Adrian Hunter <adrian.hunter@intel.com> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- drivers/mmc/host/sdhci.c | 56 ++++++------------------------------------------ 1 file changed, 7 insertions(+), 49 deletions(-) -- 1.9.1