Ulf Hansson <[email protected]> writes:

> Due to the available runtime PM callbacks, we are now able to put our
> device into low power state at system suspend.
>
> Earlier we could not accomplish this without trusting a power domain
> for the device to take care of it. Now we are able to cope with
> scenarios both with and without a power domain.
>
> Cc: Russell King <[email protected]>
> Signed-off-by: Ulf Hansson <[email protected]>
> ---
> drivers/mmc/host/mmci.c | 45 +++++++++++++++++++++++++--------------------
> 1 file changed, 25 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index c88da1c..074e0cb 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -1723,33 +1723,38 @@ static int mmci_remove(struct amba_device *dev)
> return 0;
> }
>
> -#ifdef CONFIG_SUSPEND
> -static int mmci_suspend(struct device *dev)
> +#ifdef CONFIG_PM_SLEEP
> +static int mmci_suspend_late(struct device *dev)
> {
> - struct amba_device *adev = to_amba_device(dev);
> - struct mmc_host *mmc = amba_get_drvdata(adev);
> + int ret = 0;
>
> - if (mmc) {
> - struct mmci_host *host = mmc_priv(mmc);
> - pm_runtime_get_sync(dev);
> - writel(0, host->base + MMCIMASK0);
> - }
> + if (pm_runtime_status_suspended(dev))
> + return 0;
>
> - return 0;
> + if (dev->pm_domain && dev->pm_domain->ops.runtime_suspend)
> + ret = dev->pm_domain->ops.runtime_suspend(dev);
> + else
> + ret = dev->bus->pm->runtime_suspend(dev);
> +
> + if (!ret)
> + pm_runtime_set_suspended(dev);

Isn't this basically open-coding pm_runtime_suspend()...

> + return ret;
> }
>
> -static int mmci_resume(struct device *dev)
> +static int mmci_resume_early(struct device *dev)
> {
> - struct amba_device *adev = to_amba_device(dev);
> - struct mmc_host *mmc = amba_get_drvdata(adev);
> + int ret = 0;
>
> - if (mmc) {
> - struct mmci_host *host = mmc_priv(mmc);
> - writel(MCI_IRQENABLE, host->base + MMCIMASK0);
> - pm_runtime_put(dev);
> - }
> + if (pm_runtime_status_suspended(dev))
> + return 0;
>
> - return 0;
> + if (dev->pm_domain && dev->pm_domain->ops.runtime_resume)
> + ret = dev->pm_domain->ops.runtime_resume(dev);
> + else
> + ret = dev->bus->pm->runtime_resume(dev);
> +
> + return ret;

...and this is pm_runtime_resume()? (though both terribly simplified.)

This is starting to show that building with PM_SLEEP but not PM_RUNTIME
is going to force open-coding a lot of stuff that the runtime PM
framework already provides. So either we need some helper functions so
we're not sprinkling manual calls to bus/pm_domain callbacks all over
the place, or maybe where we need to go is have a way for platforms that
really are "runtime PM centric" to declare that even PM_SLEEP depends on
PM_RUNTIME.

I'm trying to thing of a good reason to not make PM_SLEEP depend on
PM_RUNTIME for platforms like this.

Kevin

2014-02-05 12:49:53

by Linus Walleij

[permalink] [raw]

Subject: Re: [PATCH 05/17] mmc: mmci: Put the device into low power state at system suspend

On Tue, Feb 4, 2014 at 8:22 PM, Kevin Hilman <[email protected]> wrote:
> Ulf Hansson <[email protected]> writes:
>
>> Due to the available runtime PM callbacks, we are now able to put our
>> device into low power state at system suspend.
(...)
> I'm trying to thing of a good reason to not make PM_SLEEP depend on
> PM_RUNTIME for platforms like this.

Isn't the typical Android platform using PM_SLEEP without using
PM_RUNTIME?

Yours,
Linus Walleij

2014-02-05 12:53:10

by Linus Walleij

[permalink] [raw]

Subject: Re: [PATCH 10/17] spi: pl022: Remove redundant pinctrl to default state in probe

On Tue, Feb 4, 2014 at 4:58 PM, Ulf Hansson <[email protected]> wrote:

> The driver core is now taking care of putting our pins into default
> state at probe. Thus we can remove the redundant call for it in probe.
>
> Cc: Mark Brown <[email protected]>
> Signed-off-by: Ulf Hansson <[email protected]>

Acked-by: Linus Walleij <[email protected]>

Yours,
Linus Walleij

2014-02-05 14:14:22

On 4 February 2014 20:16, Mark Brown <[email protected]> wrote:
> On Tue, Feb 04, 2014 at 04:58:48PM +0100, Ulf Hansson wrote:
>
>> @@ -2328,8 +2300,23 @@ static int pl022_suspend(struct device *dev)
>> return ret;
>> }
>>
>> - pm_runtime_get_sync(dev);
>> - pl022_suspend_resources(pl022, false);
>> + pm_runtime_disable(dev);
>> +
>> + if (!pm_runtime_status_suspended(dev)) {
>> + if (dev->pm_domain && dev->pm_domain->ops.runtime_suspend)
>> + ret = dev->pm_domain->ops.runtime_suspend(dev);
>> + else
>> + ret = dev->bus->pm->runtime_suspend(dev);
>> +
>> + if (ret) {
>> + pm_runtime_enable(dev);
>> + return ret;
>> + }
>> +
>> + pm_runtime_set_suspended(dev);
>> + }
>
> This seems like a fairly hideous thing to be having to open code in an
> individual driver, it all looks generic and like something that most if
> not all devices ought to be doing and it looks very vulnerable to being
> broken by changes in the core. At the very least I would expect this to
> be done in a helper function, though it would be even nicer if the
> driver core were figuring this stuff out for us based on the device
> level callback so that drivers didn't need to worry about being in power
> domains or manually calling bus level callbacks.
>
> Putting it in a helper would at least mean that it's easier for the
> mechanics to be transferred to the core proper later on.

I agree, a helper function would be nice. I have earlier sent a patch
to the PM core, that is similar to the code above, though it was
rejected in it's current form. Let me follow up on this again.

At this point, would a way forward be to still go ahead with this
patch and then convert to, when/if, the helper function from the PM
core becomes available?

Kind regards
Ulf Hansson

2014-02-10 12:51:47

On Wed, Feb 19, 2014 at 03:20:07PM +0100, Ulf Hansson wrote:

> Also note that, Russell has already applied the corresponding part in
> the amba bus (patch 1)

Why would this depend on an AMBA patch and if it does surely the two
need to be merged together somehow?

Attachments:

(No filename) (265.00 B)
signature.asc (836.00 B)
Digital signature Download all attachments

2014-02-19 18:08:09

by Ulf Hansson

[permalink] [raw]

Subject: Re: [PATCH 06/17] spi: pl022: Let runtime PM callbacks be available for CONFIG_PM

On 19 February 2014 16:10, Mark Brown <[email protected]> wrote:
> On Wed, Feb 19, 2014 at 03:20:07PM +0100, Ulf Hansson wrote:
>
>> Also note that, Russell has already applied the corresponding part in
>> the amba bus (patch 1)
>
> Why would this depend on an AMBA patch and if it does surely the two
> need to be merged together somehow?

There are no immediate dependency, sorry for confusion.

I just wanted to highlight that the amba has converted to the new
SET_PM_RUNTIME_PM_OPS. It means the amba bus' runtime PM callbacks are
available for CONFIG_PM.

Kind regards
Uffe

2014-02-24 14:54:14

by Linus Walleij

[permalink] [raw]

Subject: Re: [PATCH 03/17] mmc: mmci: Mask IRQs for all variants during runtime suspend

On Tue, Feb 18, 2014 at 5:36 PM, Ulf Hansson <[email protected]> wrote:
> On 18 February 2014 17:05, Russell King - ARM Linux
> <[email protected]> wrote:
>> On Tue, Feb 04, 2014 at 04:58:44PM +0100, Ulf Hansson wrote:
>>> In runtime suspended state, we are not expecting IRQs and thus we can
>>> safely mask them, not only for pwrreg_nopower variants but for all.
>>>
>>> Obviously we then also need to make sure we restore the IRQ mask while
>>> becoming runtime resumed.
>>
>> So, what happens when this patch is applied, and a SDIO card is attached
>> which expects to receive interrupts at any moment?
>
> Currently, no variant implements SDIO irq.
>
> The SDIO irq polling mode from the sdio core will still be functional,
> as of today. So, this patch will not break SDIO.
>
>>
>> Given that I've run into this during the last week with a SDHCI controller,
>> I'm not that thrilled with other interfaces doing the same broken thing.
>
> If we add SDIO irq support to mmci in future; parts of that
> implementation includes a re-route of DAT1 to a GPIO irq when entering
> runtime suspend state. The mmci HW will in runtime suspend state, not
> be responsible for handling irqs, which is the same as of today.

[Just smalltalk]

Switching DAT1 to "gpio mode" (which is something of a fallacy, see
explanation in Documentation/pinctrl.txt) is not at all possible in all
implementations of the PL18x, as it depends on exploiting properties
on an assumed pin controller.

Systems that don't have such wakeup features on their pins are
unlikely to support deepsleep in any capacity, and if they do they are
ill-designed from the top level as this needs to be taken into account
when devising the hardware :-/

Yours,
Linus Walleij