[PATCH 00/17] amba: PM fixups for amba bus and some amba drivers

[PATCH 00/17] amba: PM fixups for amba bus and some amba drivers

[Ulf Hansson]

This patchset fixes the PM problems you yet when combining CONFIG_PM_SLEEP and
CONFIG_PM_RUNTIME. In principle these drivers did not manage to put it's devices
into low power state at system suspend, which then this patchset intend to fix.

Both the drivers and the amba bus converts to the new SET_PM_RUNTIME_PM_OPS
macro while setting up the runtime PM callbacks, which is intended to be used
for solving these kind of issues.

The fixes for the amba bus needs to be merged prior to the other, thus I think
it could make sense to merge this complete patchset through Russell's tree,
if he and the other maintainers think this is okay.

Ulf Hansson (17):
  amba: Let runtime PM callbacks be available for CONFIG_PM
  amba: Add late and early PM callbacks
  mmc: mmci: Mask IRQs for all variants during runtime suspend
  mmc: mmci: Let runtime PM callbacks be available for CONFIG_PM
  mmc: mmci: Put the device into low power state at system suspend
  spi: pl022: Let runtime PM callbacks be available for CONFIG_PM
  spi: pl022: Don't ignore power domain and amba bus at system suspend
  spi: pl022: Fully gate clocks at request inactivity
  spi: pl022: Simplify clock handling
  spi: pl022: Remove redundant pinctrl to default state in probe
  i2c: nomadik: Convert to devm functions
  i2c: nomadik: Remove redundant call to pm_runtime_disable
  i2c: nomadik: Leave probe with the device in active state
  i2c: nomadik: Fixup deployment of runtime PM
  i2c: nomadik: Convert to late and early system PM callbacks
  i2c: nomadik: Remove busy check for transfers at suspend late
  i2c: nomadik: Fixup system suspend

 drivers/amba/bus.c               |   10 ++-
 drivers/i2c/busses/i2c-nomadik.c |  146 ++++++++++++++++++--------------------
 drivers/mmc/host/mmci.c          |   72 ++++++++++---------
 drivers/spi/spi-pl022.c          |   95 +++++++++++--------------
 4 files changed, 159 insertions(+), 164 deletions(-)

-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 01/17] amba: Let runtime PM callbacks be available for CONFIG_PM

[Ulf Hansson]

Convert to the SET_PM_RUNTIME_PM macro while defining the runtime PM
callbacks. This means the callbacks becomes available for both
CONFIG_PM_SLEEP and CONFIG_PM_RUNTIME, which is needed by drivers and
power domains.

Cc: Russell King <linux@arm.linux.org.uk>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/amba/bus.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 9e60291..3cf61a1 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -83,7 +83,7 @@ static struct device_attribute amba_dev_attrs[] = {
 	__ATTR_NULL,
 };
 
-#ifdef CONFIG_PM_RUNTIME
+#ifdef CONFIG_PM
 /*
  * Hooks to provide runtime PM of the pclk (bus clock).  It is safe to
  * enable/disable the bus clock at runtime PM suspend/resume as this
@@ -123,7 +123,7 @@ static const struct dev_pm_ops amba_pm = {
 	.thaw		= pm_generic_thaw,
 	.poweroff	= pm_generic_poweroff,
 	.restore	= pm_generic_restore,
-	SET_RUNTIME_PM_OPS(
+	SET_PM_RUNTIME_PM_OPS(
 		amba_pm_runtime_suspend,
 		amba_pm_runtime_resume,
 		NULL
-- 
1.7.9.5

Re: [PATCH 01/17] amba: Let runtime PM callbacks be available for CONFIG_PM

[Russell King - ARM Linux]

On Tue, Feb 04, 2014 at 04:58:42PM +0100, Ulf Hansson wrote:
> Convert to the SET_PM_RUNTIME_PM macro while defining the runtime PM
> callbacks. This means the callbacks becomes available for both
> CONFIG_PM_SLEEP and CONFIG_PM_RUNTIME, which is needed by drivers and
> power domains.

This patch is wrong, because it causes build errors:

drivers/amba/bus.c:126:18: error: 'pm_generic_suspend_late' undeclared here (not in a function)
drivers/amba/bus.c:127:18: error: 'pm_generic_resume_early' undeclared here (not in a function)
drivers/amba/bus.c:128:17: error: 'pm_generic_freeze_late' undeclared here (not in a function)
drivers/amba/bus.c:129:16: error: 'pm_generic_thaw_early' undeclared here (not in a function)
drivers/amba/bus.c:130:19: error: 'pm_generic_poweroff_late' undeclared here (not in a function)
drivers/amba/bus.c:131:19: error: 'pm_generic_restore_early' undeclared here (not in a function)

The failing configuration is:

# CONFIG_SUSPEND is not set
CONFIG_PM_RUNTIME=y
CONFIG_PM=y

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

Re: [PATCH 01/17] amba: Let runtime PM callbacks be available for CONFIG_PM

[Ulf Hansson]

On 19 February 2014 10:40, Russell King - ARM Linux
<linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org> wrote:
> On Tue, Feb 04, 2014 at 04:58:42PM +0100, Ulf Hansson wrote:
>> Convert to the SET_PM_RUNTIME_PM macro while defining the runtime PM
>> callbacks. This means the callbacks becomes available for both
>> CONFIG_PM_SLEEP and CONFIG_PM_RUNTIME, which is needed by drivers and
>> power domains.
>
> This patch is wrong, because it causes build errors:
>
> drivers/amba/bus.c:126:18: error: 'pm_generic_suspend_late' undeclared here (not in a function)
> drivers/amba/bus.c:127:18: error: 'pm_generic_resume_early' undeclared here (not in a function)
> drivers/amba/bus.c:128:17: error: 'pm_generic_freeze_late' undeclared here (not in a function)
> drivers/amba/bus.c:129:16: error: 'pm_generic_thaw_early' undeclared here (not in a function)
> drivers/amba/bus.c:130:19: error: 'pm_generic_poweroff_late' undeclared here (not in a function)
> drivers/amba/bus.c:131:19: error: 'pm_generic_restore_early' undeclared here (not in a function)
>
> The failing configuration is:
>
> # CONFIG_SUSPEND is not set
> CONFIG_PM_RUNTIME=y
> CONFIG_PM=y
>

Thanks for spotting this! I thought I had tested all the various
combinations, but obvisouly not.

Anyway to add some more clarify; it is not this patch that causes the
issue. It's patch 2, which introduces the late/early callbacks.

Though the problem need to be fixed in the PM core,
pm_generic_suspend_late etc is not set to NULL for !CONFIG_PM_SLEEP,
they should. I guess we should put patch 2 on hold until, right?

Kind regards
Ulf Hansson

> --
> FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
> in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
> Estimate before purchase was "up to 13.2Mbit".
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 02/17] amba: Add late and early PM callbacks

[Ulf Hansson]

To give provision for amba drivers to make use of the late|early PM
callbacks, we point the amba bus PM callbacks to the generic versions
of these.

Cc: Russell King <linux@arm.linux.org.uk>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/amba/bus.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 3cf61a1..c255d62 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -123,6 +123,12 @@ static const struct dev_pm_ops amba_pm = {
 	.thaw		= pm_generic_thaw,
 	.poweroff	= pm_generic_poweroff,
 	.restore	= pm_generic_restore,
+	.suspend_late	= pm_generic_suspend_late,
+	.resume_early	= pm_generic_resume_early,
+	.freeze_late	= pm_generic_freeze_late,
+	.thaw_early	= pm_generic_thaw_early,
+	.poweroff_late	= pm_generic_poweroff_late,
+	.restore_early	= pm_generic_restore_early,
 	SET_PM_RUNTIME_PM_OPS(
 		amba_pm_runtime_suspend,
 		amba_pm_runtime_resume,
-- 
1.7.9.5

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

[Ulf Hansson]

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.

Cc: Russell King <linux@arm.linux.org.uk>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/mmci.c |   23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index b931226..178868a 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -1758,35 +1758,34 @@ static void mmci_save(struct mmci_host *host)
 {
 	unsigned long flags;
 
-	if (host->variant->pwrreg_nopower) {
-		spin_lock_irqsave(&host->lock, flags);
+	spin_lock_irqsave(&host->lock, flags);
 
-		writel(0, host->base + MMCIMASK0);
+	writel(0, host->base + MMCIMASK0);
+	if (host->variant->pwrreg_nopower) {
 		writel(0, host->base + MMCIDATACTRL);
 		writel(0, host->base + MMCIPOWER);
 		writel(0, host->base + MMCICLOCK);
-		mmci_reg_delay(host);
-
-		spin_unlock_irqrestore(&host->lock, flags);
 	}
+	mmci_reg_delay(host);
 
+	spin_unlock_irqrestore(&host->lock, flags);
 }
 
 static void mmci_restore(struct mmci_host *host)
 {
 	unsigned long flags;
 
-	if (host->variant->pwrreg_nopower) {
-		spin_lock_irqsave(&host->lock, flags);
+	spin_lock_irqsave(&host->lock, flags);
 
+	if (host->variant->pwrreg_nopower) {
 		writel(host->clk_reg, host->base + MMCICLOCK);
 		writel(host->datactrl_reg, host->base + MMCIDATACTRL);
 		writel(host->pwr_reg, host->base + MMCIPOWER);
-		writel(MCI_IRQENABLE, host->base + MMCIMASK0);
-		mmci_reg_delay(host);
-
-		spin_unlock_irqrestore(&host->lock, flags);
 	}
+	writel(MCI_IRQENABLE, host->base + MMCIMASK0);
+	mmci_reg_delay(host);
+
+	spin_unlock_irqrestore(&host->lock, flags);
 }
 
 static int mmci_runtime_suspend(struct device *dev)
-- 
1.7.9.5

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

[Russell King - ARM Linux]

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?

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.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

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

[Ulf Hansson]

On 18 February 2014 17:05, Russell King - ARM Linux
<linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org> 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.

Kind regards
Ulf Hansson

>
> --
> FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
> in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
> Estimate before purchase was "up to 13.2Mbit".
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Russell King - ARM Linux]

On Tue, Feb 18, 2014 at 05:36:52PM +0100, Ulf Hansson wrote:
> 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.

Note that the "irq thread" in sdio_irq is scheduled for destruction -
it's buggy when the system is under high load and a driver claims a
SDIO irq.  Sched people hate it too...

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Ulf Hansson]

On 18 February 2014 17:46, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Feb 18, 2014 at 05:36:52PM +0100, Ulf Hansson wrote:
>> 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.
>
> Note that the "irq thread" in sdio_irq is scheduled for destruction -
> it's buggy when the system is under high load and a driver claims a
> SDIO irq.  Sched people hate it too...

We kind of realized that too, in the early days for ux500. At that
point we converted it to a use a dedicated work queue, which improved
the behaviour, don't remember the details why, but maybe I should
collect the patches and send them out. :-)

Finally we moved to use a separate GPIO irq as a dedicated SDIO irq
line for our cw1200 wlan chip, mostly due to gain a bit of
performance. That happened to simplified the mmci part as well, which
was welcome.

>
> --
> FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
> in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
> Estimate before purchase was "up to 13.2Mbit".

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

[Linus Walleij]

On Tue, Feb 18, 2014 at 5:36 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 18 February 2014 17:05, Russell King - ARM Linux
> <linux@arm.linux.org.uk> 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

[PATCH 04/17] mmc: mmci: Let runtime PM callbacks be available for CONFIG_PM

[Ulf Hansson]

Convert to the SET_PM_RUNTIME_PM macro while defining the runtime PM
callbacks. This means the callbacks becomes available for both
CONFIG_PM_SLEEP and CONFIG_PM_RUNTIME, which is needed to handle the
combinations of these scenarios.

Cc: Russell King <linux@arm.linux.org.uk>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/mmci.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 178868a..c88da1c 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -1753,7 +1753,7 @@ static int mmci_resume(struct device *dev)
 }
 #endif
 
-#ifdef CONFIG_PM_RUNTIME
+#ifdef CONFIG_PM
 static void mmci_save(struct mmci_host *host)
 {
 	unsigned long flags;
@@ -1821,7 +1821,7 @@ static int mmci_runtime_resume(struct device *dev)
 
 static const struct dev_pm_ops mmci_dev_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(mmci_suspend, mmci_resume)
-	SET_RUNTIME_PM_OPS(mmci_runtime_suspend, mmci_runtime_resume, NULL)
+	SET_PM_RUNTIME_PM_OPS(mmci_runtime_suspend, mmci_runtime_resume, NULL)
 };
 
 static struct amba_id mmci_ids[] = {
-- 
1.7.9.5

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

[Ulf Hansson]

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 <linux@arm.linux.org.uk>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 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);
+
+	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;
 }
 #endif
 
@@ -1820,7 +1825,7 @@ static int mmci_runtime_resume(struct device *dev)
 #endif
 
 static const struct dev_pm_ops mmci_dev_pm_ops = {
-	SET_SYSTEM_SLEEP_PM_OPS(mmci_suspend, mmci_resume)
+	SET_LATE_SYSTEM_SLEEP_PM_OPS(mmci_suspend_late, mmci_resume_early)
 	SET_PM_RUNTIME_PM_OPS(mmci_runtime_suspend, mmci_runtime_resume, NULL)
 };
 
-- 
1.7.9.5

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

[Kevin Hilman]

Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> 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 <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
> Signed-off-by: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  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

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Linus Walleij]

On Tue, Feb 4, 2014 at 8:22 PM, Kevin Hilman <khilman@linaro.org> wrote:
> Ulf Hansson <ulf.hansson@linaro.org> 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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 657 bytes --]

On Wed, Feb 05, 2014 at 01:49:49PM +0100, Linus Walleij wrote:
> On Tue, Feb 4, 2014 at 8:22 PM, Kevin Hilman <khilman-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> > 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?

No, not at all.  Android does make aggressive use of sleep but it's also
highly desirable to use runtime PM - for example you don't want to have
to power up the entire SoC simply because the system is getting a new
e-mail pushed to it or location updates, most of the hardware is doing
nothing.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Kevin Hilman]

Linus Walleij <linus.walleij@linaro.org> writes:

> On Tue, Feb 4, 2014 at 8:22 PM, Kevin Hilman <khilman@linaro.org> wrote:
>> Ulf Hansson <ulf.hansson@linaro.org> 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?

No, most Android platforms that I'm aware of use both extensively.

Kevin

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

[Ulf Hansson]

On 4 February 2014 20:22, Kevin Hilman <khilman@linaro.org> wrote:
> Ulf Hansson <ulf.hansson@linaro.org> 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 <linux@arm.linux.org.uk>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>  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()...

It is similar, but with once big difference.

Since the PM core prevents pm_runtime_suspend() from invoking our
->runtime_suspend callback during system suspend (it does so by
invoking pm_runtime_get_sync() before starting the suspend sequence),
we then need to make the driver handle that by itself.

>
>> +     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.)

Correct, but again with a big difference. See comment above.

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

I have send a patch a while ago for the PM core, that tried to
implement something similar like this, I wasn't accepted. I will
follow up on that asap.

Still, do you think we could go ahead with this patch? If/when we can
get an acceptance for a PM runtime helper function in the PM core, we
can easily convert to use it later on.

> 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.

This wont help. The PM core will still prevent the runtime_suspend
callback from being invoked during system suspend.


Kind regards
Ulf Hansson

>
> Kevin
>

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

[Kevin Hilman]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> On 4 February 2014 20:22, Kevin Hilman <khilman@linaro.org> wrote:
>> Ulf Hansson <ulf.hansson@linaro.org> 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 <linux@arm.linux.org.uk>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> ---
>>>  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()...
>
> It is similar, but with once big difference.
>
> Since the PM core prevents pm_runtime_suspend() from invoking our
> ->runtime_suspend callback during system suspend (it does so by
> invoking pm_runtime_get_sync() before starting the suspend sequence),
> we then need to make the driver handle that by itself.

Yeah, I still think we need to allow a bus/pm_domain to override that
behavior.


Kevin

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

[Ulf Hansson]

Convert to the SET_PM_RUNTIME_PM macro while defining the runtime PM
callbacks. This means the callbacks becomes available for both
CONFIG_PM_SLEEP and CONFIG_PM_RUNTIME, which is needed to handle the
combinations of these scenarios.

Cc: Mark Brown <broonie@kernel.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/spi/spi-pl022.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
index 2789b45..70fa907 100644
--- a/drivers/spi/spi-pl022.c
+++ b/drivers/spi/spi-pl022.c
@@ -2288,7 +2288,7 @@ pl022_remove(struct amba_device *adev)
 	return 0;
 }
 
-#if defined(CONFIG_SUSPEND) || defined(CONFIG_PM_RUNTIME)
+#ifdef CONFIG_PM
 /*
  * These two functions are used from both suspend/resume and
  * the runtime counterparts to handle external resources like
@@ -2354,7 +2354,7 @@ static int pl022_resume(struct device *dev)
 }
 #endif	/* CONFIG_PM */
 
-#ifdef CONFIG_PM_RUNTIME
+#ifdef CONFIG_PM
 static int pl022_runtime_suspend(struct device *dev)
 {
 	struct pl022 *pl022 = dev_get_drvdata(dev);
@@ -2374,7 +2374,7 @@ static int pl022_runtime_resume(struct device *dev)
 
 static const struct dev_pm_ops pl022_dev_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(pl022_suspend, pl022_resume)
-	SET_RUNTIME_PM_OPS(pl022_runtime_suspend, pl022_runtime_resume, NULL)
+	SET_PM_RUNTIME_PM_OPS(pl022_runtime_suspend, pl022_runtime_resume, NULL)
 };
 
 static struct vendor_data vendor_arm = {
-- 
1.7.9.5

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

[Ulf Hansson]

On 4 February 2014 16:58, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> Convert to the SET_PM_RUNTIME_PM macro while defining the runtime PM
> callbacks. This means the callbacks becomes available for both
> CONFIG_PM_SLEEP and CONFIG_PM_RUNTIME, which is needed to handle the
> combinations of these scenarios.
>
> Cc: Mark Brown <broonie@kernel.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Mark, any thoughts of this one?

This will be needed no matter of patch 7.

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

Kind regards
Uffe

> ---
>  drivers/spi/spi-pl022.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
> index 2789b45..70fa907 100644
> --- a/drivers/spi/spi-pl022.c
> +++ b/drivers/spi/spi-pl022.c
> @@ -2288,7 +2288,7 @@ pl022_remove(struct amba_device *adev)
>         return 0;
>  }
>
> -#if defined(CONFIG_SUSPEND) || defined(CONFIG_PM_RUNTIME)
> +#ifdef CONFIG_PM
>  /*
>   * These two functions are used from both suspend/resume and
>   * the runtime counterparts to handle external resources like
> @@ -2354,7 +2354,7 @@ static int pl022_resume(struct device *dev)
>  }
>  #endif /* CONFIG_PM */
>
> -#ifdef CONFIG_PM_RUNTIME
> +#ifdef CONFIG_PM
>  static int pl022_runtime_suspend(struct device *dev)
>  {
>         struct pl022 *pl022 = dev_get_drvdata(dev);
> @@ -2374,7 +2374,7 @@ static int pl022_runtime_resume(struct device *dev)
>
>  static const struct dev_pm_ops pl022_dev_pm_ops = {
>         SET_SYSTEM_SLEEP_PM_OPS(pl022_suspend, pl022_resume)
> -       SET_RUNTIME_PM_OPS(pl022_runtime_suspend, pl022_runtime_resume, NULL)
> +       SET_PM_RUNTIME_PM_OPS(pl022_runtime_suspend, pl022_runtime_resume, NULL)
>  };
>
>  static struct vendor_data vendor_arm = {
> --
> 1.7.9.5
>

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 265 bytes --]

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?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Ulf Hansson]

On 19 February 2014 16:10, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 07/17] spi: pl022: Don't ignore power domain and amba bus at system suspend

[Ulf Hansson]

Due to the available runtime PM callbacks for CONFIG_PM, we are now
able to put the device into complete low power state at system suspend.

Previously only the resources controlled by the driver were put into
low power state at system suspend. Both the amba bus and a potential
power domain were ignored, which now isn't the case any more.

Moreover, putting the device into low power state couldn't be done
without first bringing it back to full power. This constraint don't
exists any more.

Cc: Mark Brown <broonie@kernel.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/spi/spi-pl022.c |   74 +++++++++++++++++++++++------------------------
 1 file changed, 37 insertions(+), 37 deletions(-)

diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
index 70fa907..db829a1 100644
--- a/drivers/spi/spi-pl022.c
+++ b/drivers/spi/spi-pl022.c
@@ -2288,35 +2288,7 @@ pl022_remove(struct amba_device *adev)
 	return 0;
 }
 
-#ifdef CONFIG_PM
-/*
- * These two functions are used from both suspend/resume and
- * the runtime counterparts to handle external resources like
- * clocks, pins and regulators when going to sleep.
- */
-static void pl022_suspend_resources(struct pl022 *pl022, bool runtime)
-{
-	clk_disable(pl022->clk);
-
-	if (runtime)
-		pinctrl_pm_select_idle_state(&pl022->adev->dev);
-	else
-		pinctrl_pm_select_sleep_state(&pl022->adev->dev);
-}
-
-static void pl022_resume_resources(struct pl022 *pl022, bool runtime)
-{
-	/* First go to the default state */
-	pinctrl_pm_select_default_state(&pl022->adev->dev);
-	if (!runtime)
-		/* Then let's idle the pins until the next transfer happens */
-		pinctrl_pm_select_idle_state(&pl022->adev->dev);
-
-	clk_enable(pl022->clk);
-}
-#endif
-
-#ifdef CONFIG_SUSPEND
+#ifdef CONFIG_PM_SLEEP
 static int pl022_suspend(struct device *dev)
 {
 	struct pl022 *pl022 = dev_get_drvdata(dev);
@@ -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);
+	}
+
+	pinctrl_pm_select_sleep_state(dev);
 
 	dev_dbg(dev, "suspended\n");
 	return 0;
@@ -2338,10 +2325,19 @@ static int pl022_suspend(struct device *dev)
 static int pl022_resume(struct device *dev)
 {
 	struct pl022 *pl022 = dev_get_drvdata(dev);
-	int ret;
+	int ret = 0;
 
-	pl022_resume_resources(pl022, false);
-	pm_runtime_put(dev);
+	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);
+
+	if (ret)
+		dev_err(dev, "problem resuming\n");
+	else
+		pm_runtime_set_active(dev);
+
+	pm_runtime_enable(dev);
 
 	/* Start the queue running */
 	ret = spi_master_resume(pl022->master);
@@ -2352,14 +2348,16 @@ static int pl022_resume(struct device *dev)
 
 	return ret;
 }
-#endif	/* CONFIG_PM */
+#endif
 
 #ifdef CONFIG_PM
 static int pl022_runtime_suspend(struct device *dev)
 {
 	struct pl022 *pl022 = dev_get_drvdata(dev);
 
-	pl022_suspend_resources(pl022, true);
+	clk_disable(pl022->clk);
+	pinctrl_pm_select_idle_state(dev);
+
 	return 0;
 }
 
@@ -2367,7 +2365,9 @@ static int pl022_runtime_resume(struct device *dev)
 {
 	struct pl022 *pl022 = dev_get_drvdata(dev);
 
-	pl022_resume_resources(pl022, true);
+	pinctrl_pm_select_default_state(dev);
+	clk_enable(pl022->clk);
+
 	return 0;
 }
 #endif
-- 
1.7.9.5

Re: [PATCH 07/17] spi: pl022: Don't ignore power domain and amba bus at system suspend

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1316 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 07/17] spi: pl022: Don't ignore power domain and amba bus at system suspend

[Ulf Hansson]

On 4 February 2014 20:16, Mark Brown <broonie@kernel.org> 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

Re: [PATCH 07/17] spi: pl022: Don't ignore power domain and amba bus at system suspend

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1240 bytes --]

On Mon, Feb 10, 2014 at 01:33:50PM +0100, Ulf Hansson wrote:
> On 4 February 2014 20:16, Mark Brown <broonie@kernel.org> wrote:

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

...

> > 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?

It's definitely *a* way forward, but I'm not convinced it's a good way
forward.  Since it's something that I'd expect us to be doing in all
drivers we'd want to replicate it all over the place which is obviously
not good, or conversely if there are issues that prevented the code
being added to the PM core then presumably we're just adding problematic
code to the driver (you've not mentioned what the problems were with
doing this in the PM core).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 07/17] spi: pl022: Don't ignore power domain and amba bus at system suspend

[Ulf Hansson]

On 10 February 2014 13:51, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Feb 10, 2014 at 01:33:50PM +0100, Ulf Hansson wrote:
>> On 4 February 2014 20:16, Mark Brown <broonie@kernel.org> wrote:
>
>> > 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
>
> ...
>
>> > 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?
>
> It's definitely *a* way forward, but I'm not convinced it's a good way
> forward.  Since it's something that I'd expect us to be doing in all
> drivers we'd want to replicate it all over the place which is obviously
> not good, or conversely if there are issues that prevented the code
> being added to the PM core then presumably we're just adding problematic
> code to the driver (you've not mentioned what the problems were with
> doing this in the PM core).

I have posted a patch which adds a runtime PM helper function to the
PM core, I am hoping to get some comments soon.
http://marc.info/?l=linux-pm&m=139228211505423&w=2

So I agree, let's put this patch on hold until we sorted out how to
proceed. Though I will rebase and send a v2 of it, just to keep it as
a reference for later use.

Kind regards
Uffe

[PATCH 08/17] spi: pl022: Fully gate clocks at request inactivity

[Ulf Hansson]

Use clk_disable_unprepare and clk_prepare_enable from the runtime PM
callbacks, to fully gate|ungate clocks. Potentially this will save more
power, depending on the clock tree for the current SOC.

Cc: Mark Brown <broonie@kernel.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/spi/spi-pl022.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
index db829a1..2bb238f 100644
--- a/drivers/spi/spi-pl022.c
+++ b/drivers/spi/spi-pl022.c
@@ -2355,7 +2355,7 @@ static int pl022_runtime_suspend(struct device *dev)
 {
 	struct pl022 *pl022 = dev_get_drvdata(dev);
 
-	clk_disable(pl022->clk);
+	clk_disable_unprepare(pl022->clk);
 	pinctrl_pm_select_idle_state(dev);
 
 	return 0;
@@ -2366,7 +2366,7 @@ static int pl022_runtime_resume(struct device *dev)
 	struct pl022 *pl022 = dev_get_drvdata(dev);
 
 	pinctrl_pm_select_default_state(dev);
-	clk_enable(pl022->clk);
+	clk_prepare_enable(pl022->clk);
 
 	return 0;
 }
-- 
1.7.9.5

Re: [PATCH 08/17] spi: pl022: Fully gate clocks at request inactivity

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 526 bytes --]

On Tue, Feb 04, 2014 at 04:58:49PM +0100, Ulf Hansson wrote:
> Use clk_disable_unprepare and clk_prepare_enable from the runtime PM
> callbacks, to fully gate|ungate clocks. Potentially this will save more
> power, depending on the clock tree for the current SOC.

The same patch has already been applied (I noticed and fixed it while
doing unrelated code review).

>  	pinctrl_pm_select_default_state(dev);
> -	clk_enable(pl022->clk);
> +	clk_prepare_enable(pl022->clk);

Someone should really make this check errors though!

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 08/17] spi: pl022: Fully gate clocks at request inactivity

[Ulf Hansson]

On 4 February 2014 18:07, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Feb 04, 2014 at 04:58:49PM +0100, Ulf Hansson wrote:
>> Use clk_disable_unprepare and clk_prepare_enable from the runtime PM
>> callbacks, to fully gate|ungate clocks. Potentially this will save more
>> power, depending on the clock tree for the current SOC.
>
> The same patch has already been applied (I noticed and fixed it while
> doing unrelated code review).

Sorry, didn't notice that.

Thanks!

Kind regards
Ulf Hansson

>
>>       pinctrl_pm_select_default_state(dev);
>> -     clk_enable(pl022->clk);
>> +     clk_prepare_enable(pl022->clk);
>
> Someone should really make this check errors though!

[PATCH 12/17] i2c: nomadik: Remove redundant call to pm_runtime_disable

[Ulf Hansson]

The amba bus are responsible for pm_runtime_enable|disable, remove the
redundant pm_runtime_disable at driver removal.

Cc: Alessandro Rubini <rubini@unipv.it>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Wolfram Sang <wsa@the-dreams.de>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/i2c/busses/i2c-nomadik.c |    1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index 840bcf9..42c9e51 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -1105,7 +1105,6 @@ static int nmk_i2c_remove(struct amba_device *adev)
 	i2c_clr_bit(dev->virtbase + I2C_CR, I2C_CR_PE);
 	if (res)
 		release_mem_region(res->start, resource_size(res));
-	pm_runtime_disable(&adev->dev);
 
 	return 0;
 }
-- 
1.7.9.5

[PATCH 13/17] i2c: nomadik: Leave probe with the device in active state

[Ulf Hansson]

Since the runtime PM state is expected to be active according to the
amba bus, we must align our behaviour while probing to it.

Moreover, this is needed to be able to have the driver fully functional
without depending on CONFIG_RUNTIME_PM.

Cc: Alessandro Rubini <rubini@unipv.it>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Wolfram Sang <wsa@the-dreams.de>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/i2c/busses/i2c-nomadik.c |   18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index 42c9e51..0caa8ea 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -1026,11 +1026,6 @@ static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
 	dev->adev = adev;
 	amba_set_drvdata(adev, dev);
 
-	/* Select default pin state */
-	pinctrl_pm_select_default_state(&adev->dev);
-	/* If possible, let's go to idle until the first transfer */
-	pinctrl_pm_select_idle_state(&adev->dev);
-
 	dev->virtbase = devm_ioremap(&adev->dev, adev->res.start,
 				resource_size(&adev->res));
 	if (!dev->virtbase) {
@@ -1055,6 +1050,14 @@ static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
 		goto err_no_mem;
 	}
 
+	ret = clk_prepare_enable(dev->clk);
+	if (ret) {
+		dev_err(&adev->dev, "can't prepare_enable clock\n");
+		goto err_no_mem;
+	}
+
+	init_hw(dev);
+
 	adap = &dev->adap;
 	adap->dev.of_node = np;
 	adap->dev.parent = &adev->dev;
@@ -1080,13 +1083,15 @@ static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
 	ret = i2c_add_adapter(adap);
 	if (ret) {
 		dev_err(&adev->dev, "failed to add adapter\n");
-		goto err_no_mem;
+		goto err_no_adap;
 	}
 
 	pm_runtime_put(&adev->dev);
 
 	return 0;
 
+ err_no_adap:
+	clk_disable_unprepare(dev->clk);
  err_no_mem:
 
 	return ret;
@@ -1103,6 +1108,7 @@ static int nmk_i2c_remove(struct amba_device *adev)
 	clear_all_interrupts(dev);
 	/* disable the controller */
 	i2c_clr_bit(dev->virtbase + I2C_CR, I2C_CR_PE);
+	clk_disable_unprepare(dev->clk);
 	if (res)
 		release_mem_region(res->start, resource_size(res));
 
-- 
1.7.9.5

[PATCH 14/17] i2c: nomadik: Fixup deployment of runtime PM

[Ulf Hansson]

Since the device is active while a successful probe has been completed,
the reference counting for the clock will be screwed up and never reach
zero.

The issue is resolved by implementing runtime PM callbacks and let them
handle the resources accordingly, including the clock.

Cc: Alessandro Rubini <rubini@unipv.it>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Wolfram Sang <wsa@the-dreams.de>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/i2c/busses/i2c-nomadik.c |   47 ++++++++++++++++++++++----------------
 1 file changed, 27 insertions(+), 20 deletions(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index 0caa8ea..2d7dbc9 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -674,7 +674,7 @@ static int nmk_i2c_xfer_one(struct nmk_i2c_dev *dev, u16 flags)
 static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
 		struct i2c_msg msgs[], int num_msgs)
 {
-	int status;
+	int status = 0;
 	int i;
 	struct nmk_i2c_dev *dev = i2c_get_adapdata(i2c_adap);
 	int j;
@@ -683,19 +683,6 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
 
 	pm_runtime_get_sync(&dev->adev->dev);
 
-	status = clk_prepare_enable(dev->clk);
-	if (status) {
-		dev_err(&dev->adev->dev, "can't prepare_enable clock\n");
-		goto out_clk;
-	}
-
-	/* Optionaly enable pins to be muxed in and configured */
-	pinctrl_pm_select_default_state(&dev->adev->dev);
-
-	status = init_hw(dev);
-	if (status)
-		goto out;
-
 	/* Attempt three times to send the message queue */
 	for (j = 0; j < 3; j++) {
 		/* setup the i2c controller */
@@ -716,12 +703,6 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
 			break;
 	}
 
-out:
-	clk_disable_unprepare(dev->clk);
-out_clk:
-	/* Optionally let pins go into idle state */
-	pinctrl_pm_select_idle_state(&dev->adev->dev);
-
 	pm_runtime_put_sync(&dev->adev->dev);
 
 	dev->busy = false;
@@ -938,6 +919,29 @@ static int nmk_i2c_resume(struct device *dev)
 #define nmk_i2c_resume	NULL
 #endif
 
+#if CONFIG_PM
+static int nmk_i2c_runtime_suspend(struct device *dev)
+{
+	struct amba_device *adev = to_amba_device(dev);
+	struct nmk_i2c_dev *nmk_i2c = amba_get_drvdata(adev);
+
+	clk_disable_unprepare(nmk_i2c->clk);
+	pinctrl_pm_select_idle_state(dev);
+	return 0;
+}
+
+static int nmk_i2c_runtime_resume(struct device *dev)
+{
+	struct amba_device *adev = to_amba_device(dev);
+	struct nmk_i2c_dev *nmk_i2c = amba_get_drvdata(adev);
+
+	clk_prepare_enable(nmk_i2c->clk);
+	pinctrl_pm_select_default_state(dev);
+	init_hw(nmk_i2c);
+	return 0;
+}
+#endif
+
 /*
  * We use noirq so that we suspend late and resume before the wakeup interrupt
  * to ensure that we do the !pm_runtime_suspended() check in resume before
@@ -946,6 +950,9 @@ static int nmk_i2c_resume(struct device *dev)
 static const struct dev_pm_ops nmk_i2c_pm = {
 	.suspend_noirq	= nmk_i2c_suspend,
 	.resume_noirq	= nmk_i2c_resume,
+	SET_PM_RUNTIME_PM_OPS(nmk_i2c_runtime_suspend,
+			nmk_i2c_runtime_resume,
+			NULL)
 };
 
 static unsigned int nmk_i2c_functionality(struct i2c_adapter *adap)
-- 
1.7.9.5

Re: [PATCH 14/17] i2c: nomadik: Fixup deployment of runtime PM

[Linus Walleij]

On Tue, Feb 4, 2014 at 4:58 PM, Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> Since the device is active while a successful probe has been completed,
> the reference counting for the clock will be screwed up and never reach
> zero.
>
> The issue is resolved by implementing runtime PM callbacks and let them
> handle the resources accordingly, including the clock.
>
> Cc: Alessandro Rubini <rubini-9wsNiZum9E8@public.gmane.org>
> Cc: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
> Signed-off-by: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Hm do I read it right as patch 13 breaks runtime PM by leaving
the device active after probe() and this patch
14 fixes it again? Maybe these two patches should be squashed
then.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 14/17] i2c: nomadik: Fixup deployment of runtime PM

[Ulf Hansson]

On 5 February 2014 15:34, Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Tue, Feb 4, 2014 at 4:58 PM, Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>
>> Since the device is active while a successful probe has been completed,
>> the reference counting for the clock will be screwed up and never reach
>> zero.
>>
>> The issue is resolved by implementing runtime PM callbacks and let them
>> handle the resources accordingly, including the clock.
>>
>> Cc: Alessandro Rubini <rubini-9wsNiZum9E8@public.gmane.org>
>> Cc: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> Cc: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
>> Signed-off-by: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>
> Hm do I read it right as patch 13 breaks runtime PM by leaving
> the device active after probe() and this patch
> 14 fixes it again? Maybe these two patches should be squashed
> then.

You are right; but the driver will still be working, you just don't
get the benefit from inactivating the device at request inactivity -
as you pointed out.

The reason for why I wanted to do this as separate steps was to make
it easier for reviewing, otherwise the patch(es) would have been quite
big and messy. I am for sure open to  adopt to your proposal, but just
wanted to give you some more background, before I go ahead and send a
v2.

Kind regards
Ulf Hansson

>
> Yours,
> Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 14/17] i2c: nomadik: Fixup deployment of runtime PM

[Ulf Hansson]

On 10 February 2014 11:14, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 5 February 2014 15:34, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Tue, Feb 4, 2014 at 4:58 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>
>>> Since the device is active while a successful probe has been completed,
>>> the reference counting for the clock will be screwed up and never reach
>>> zero.
>>>
>>> The issue is resolved by implementing runtime PM callbacks and let them
>>> handle the resources accordingly, including the clock.
>>>
>>> Cc: Alessandro Rubini <rubini@unipv.it>
>>> Cc: Linus Walleij <linus.walleij@linaro.org>
>>> Cc: Wolfram Sang <wsa@the-dreams.de>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>
>> Hm do I read it right as patch 13 breaks runtime PM by leaving
>> the device active after probe() and this patch
>> 14 fixes it again? Maybe these two patches should be squashed
>> then.

In v2 I have now squashed patch 13 into this patch 14.

That means patch13 shall be dropped from this patchset.

Kind regards
Uffe

>
> You are right; but the driver will still be working, you just don't
> get the benefit from inactivating the device at request inactivity -
> as you pointed out.
>
> The reason for why I wanted to do this as separate steps was to make
> it easier for reviewing, otherwise the patch(es) would have been quite
> big and messy. I am for sure open to  adopt to your proposal, but just
> wanted to give you some more background, before I go ahead and send a
> v2.
>
> Kind regards
> Ulf Hansson
>
>>
>> Yours,
>> Linus Walleij

Re: [PATCH 14/17] i2c: nomadik: Fixup deployment of runtime PM

[Fabio Estevam]

On Tue, Feb 4, 2014 at 1:58 PM, Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> +static int nmk_i2c_runtime_resume(struct device *dev)
> +{
> +       struct amba_device *adev = to_amba_device(dev);,
> +       struct nmk_i2c_dev *nmk_i2c = amba_get_drvdata(adev);
> +
> +       clk_prepare_enable(nmk_i2c->clk);

Previously the code was checking the return value from
clk_prepare_enable(). You should keep the check here.

Regards,

Fabio Estevam
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 14/17] i2c: nomadik: Fixup deployment of runtime PM

[Ulf Hansson]

On 5 February 2014 15:45, Fabio Estevam <festevam-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Tue, Feb 4, 2014 at 1:58 PM, Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>
>> +static int nmk_i2c_runtime_resume(struct device *dev)
>> +{
>> +       struct amba_device *adev = to_amba_device(dev);,
>> +       struct nmk_i2c_dev *nmk_i2c = amba_get_drvdata(adev);
>> +
>> +       clk_prepare_enable(nmk_i2c->clk);
>
> Previously the code was checking the return value from
> clk_prepare_enable(). You should keep the check here.

Will fix in v2! Thanks for reviewing!

Kind regards
Ulf Hansson

>
> Regards,
>
> Fabio Estevam

[PATCH 09/17] spi: pl022: Simplify clock handling

[Ulf Hansson]

Make use of clk_prepare_enable and clk_disable_unprepare to simplify
code. No functional change.

Cc: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Signed-off-by: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/spi/spi-pl022.c |   15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
index 2bb238f..2f44c81 100644
--- a/drivers/spi/spi-pl022.c
+++ b/drivers/spi/spi-pl022.c
@@ -2183,13 +2183,7 @@ static int pl022_probe(struct amba_device *adev, const struct amba_id *id)
 		goto err_no_clk;
 	}
 
-	status = clk_prepare(pl022->clk);
-	if (status) {
-		dev_err(&adev->dev, "could not prepare SSP/SPI bus clock\n");
-		goto  err_clk_prep;
-	}
-
-	status = clk_enable(pl022->clk);
+	status = clk_prepare_enable(pl022->clk);
 	if (status) {
 		dev_err(&adev->dev, "could not enable SSP/SPI bus clock\n");
 		goto err_no_clk_en;
@@ -2250,10 +2244,8 @@ static int pl022_probe(struct amba_device *adev, const struct amba_id *id)
 	if (platform_info->enable_dma)
 		pl022_dma_remove(pl022);
  err_no_irq:
-	clk_disable(pl022->clk);
+	clk_disable_unprepare(pl022->clk);
  err_no_clk_en:
-	clk_unprepare(pl022->clk);
- err_clk_prep:
  err_no_clk:
  err_no_ioremap:
 	amba_release_regions(adev);
@@ -2281,8 +2273,7 @@ pl022_remove(struct amba_device *adev)
 	if (pl022->master_info->enable_dma)
 		pl022_dma_remove(pl022);
 
-	clk_disable(pl022->clk);
-	clk_unprepare(pl022->clk);
+	clk_disable_unprepare(pl022->clk);
 	amba_release_regions(adev);
 	tasklet_disable(&pl022->pump_transfers);
 	return 0;
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/17] spi: pl022: Simplify clock handling

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 477 bytes --]

On Tue, Feb 04, 2014 at 04:58:50PM +0100, Ulf Hansson wrote:
> Make use of clk_prepare_enable and clk_disable_unprepare to simplify
> code. No functional change.

I went ahead and applied this since it looks good and seems like an
unrelated cleanup to the runtime PM stuff which seems to be where the
interdependencies are - thanks!  It's on a separate branch with the
already applied change so if it does need to be cross merged we can do
that easily or even drop the branch.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 09/17] spi: pl022: Simplify clock handling

[Ulf Hansson]

On 4 February 2014 20:19, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Tue, Feb 04, 2014 at 04:58:50PM +0100, Ulf Hansson wrote:
>> Make use of clk_prepare_enable and clk_disable_unprepare to simplify
>> code. No functional change.
>
> I went ahead and applied this since it looks good and seems like an
> unrelated cleanup to the runtime PM stuff which seems to be where the
> interdependencies are - thanks!  It's on a separate branch with the
> already applied change so if it does need to be cross merged we can do
> that easily or even drop the branch.

Thanks Mark, it seems like a good approach.

Kind regards
Ulf Hansson

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

[Ulf Hansson]

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 <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Signed-off-by: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/spi/spi-pl022.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
index 2f44c81..3ddd8da 100644
--- a/drivers/spi/spi-pl022.c
+++ b/drivers/spi/spi-pl022.c
@@ -2109,8 +2109,6 @@ static int pl022_probe(struct amba_device *adev, const struct amba_id *id)
 	pl022->chipselects = devm_kzalloc(dev, num_cs * sizeof(int),
 					  GFP_KERNEL);
 
-	pinctrl_pm_select_default_state(dev);
-
 	/*
 	 * Bus Number Which has been Assigned to this SSP controller
 	 * on this board
-- 
1.7.9.5

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 298 bytes --]

On Tue, Feb 04, 2014 at 04:58:51PM +0100, Ulf Hansson 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.

I applied this one too, as with the last patch it looks independant of
the runtime PM stuff.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Linus Walleij]

On Tue, Feb 4, 2014 at 4:58 PM, Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> 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 <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Signed-off-by: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Acked-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 11/17] i2c: nomadik: Convert to devm functions

[Ulf Hansson]

Use devm_* functions to simplify code and error handling.

Cc: Alessandro Rubini <rubini-9wsNiZum9E8@public.gmane.org>
Cc: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
Signed-off-by: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/i2c/busses/i2c-nomadik.c |   29 +++++++++--------------------
 1 file changed, 9 insertions(+), 20 deletions(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index 4443613..840bcf9 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -1015,7 +1015,7 @@ static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
 		pdata->rft = max_fifo_threshold;
 	}
 
-	dev = kzalloc(sizeof(struct nmk_i2c_dev), GFP_KERNEL);
+	dev = devm_kzalloc(&adev->dev, sizeof(struct nmk_i2c_dev), GFP_KERNEL);
 	if (!dev) {
 		dev_err(&adev->dev, "cannot allocate memory\n");
 		ret = -ENOMEM;
@@ -1031,27 +1031,28 @@ static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
 	/* If possible, let's go to idle until the first transfer */
 	pinctrl_pm_select_idle_state(&adev->dev);
 
-	dev->virtbase = ioremap(adev->res.start, resource_size(&adev->res));
+	dev->virtbase = devm_ioremap(&adev->dev, adev->res.start,
+				resource_size(&adev->res));
 	if (!dev->virtbase) {
 		ret = -ENOMEM;
-		goto err_no_ioremap;
+		goto err_no_mem;
 	}
 
 	dev->irq = adev->irq[0];
-	ret = request_irq(dev->irq, i2c_irq_handler, 0,
+	ret = devm_request_irq(&adev->dev, dev->irq, i2c_irq_handler, 0,
 				DRIVER_NAME, dev);
 	if (ret) {
 		dev_err(&adev->dev, "cannot claim the irq %d\n", dev->irq);
-		goto err_irq;
+		goto err_no_mem;
 	}
 
 	pm_suspend_ignore_children(&adev->dev, true);
 
-	dev->clk = clk_get(&adev->dev, NULL);
+	dev->clk = devm_clk_get(&adev->dev, NULL);
 	if (IS_ERR(dev->clk)) {
 		dev_err(&adev->dev, "could not get i2c clock\n");
 		ret = PTR_ERR(dev->clk);
-		goto err_no_clk;
+		goto err_no_mem;
 	}
 
 	adap = &dev->adap;
@@ -1079,21 +1080,13 @@ static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
 	ret = i2c_add_adapter(adap);
 	if (ret) {
 		dev_err(&adev->dev, "failed to add adapter\n");
-		goto err_add_adap;
+		goto err_no_mem;
 	}
 
 	pm_runtime_put(&adev->dev);
 
 	return 0;
 
- err_add_adap:
-	clk_put(dev->clk);
- err_no_clk:
-	free_irq(dev->irq, dev);
- err_irq:
-	iounmap(dev->virtbase);
- err_no_ioremap:
-	kfree(dev);
  err_no_mem:
 
 	return ret;
@@ -1110,13 +1103,9 @@ static int nmk_i2c_remove(struct amba_device *adev)
 	clear_all_interrupts(dev);
 	/* disable the controller */
 	i2c_clr_bit(dev->virtbase + I2C_CR, I2C_CR_PE);
-	free_irq(dev->irq, dev);
-	iounmap(dev->virtbase);
 	if (res)
 		release_mem_region(res->start, resource_size(res));
-	clk_put(dev->clk);
 	pm_runtime_disable(&adev->dev);
-	kfree(dev);
 
 	return 0;
 }
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/17] i2c: nomadik: Convert to devm functions

[Linus Walleij]

On Tue, Feb 4, 2014 at 4:58 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:

> Use devm_* functions to simplify code and error handling.
>
> Cc: Alessandro Rubini <rubini@unipv.it>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

However make sure this (and the rest) applies on top of this patch:
http://marc.info/?l=linux-i2c&m=139142325809973&w=2

Because I expect that to be applied first.

Yours,
Linus Walleij

[PATCH 15/17] i2c: nomadik: Convert to late and early system PM callbacks

[Ulf Hansson]

At system suspend_late, runtime PM has been disabled by the PM core
which means we can safely operate on these resources. Consequentially
we no longer have to wait until the noirq phase of the system suspend.

Cc: Alessandro Rubini <rubini-9wsNiZum9E8@public.gmane.org>
Cc: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
Signed-off-by: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/i2c/busses/i2c-nomadik.c |   18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index 2d7dbc9..eda4f0d 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -890,9 +890,8 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
 	return IRQ_HANDLED;
 }
 
-
-#ifdef CONFIG_PM
-static int nmk_i2c_suspend(struct device *dev)
+#ifdef CONFIG_PM_SLEEP
+static int nmk_i2c_suspend_late(struct device *dev)
 {
 	struct amba_device *adev = to_amba_device(dev);
 	struct nmk_i2c_dev *nmk_i2c = amba_get_drvdata(adev);
@@ -905,7 +904,7 @@ static int nmk_i2c_suspend(struct device *dev)
 	return 0;
 }
 
-static int nmk_i2c_resume(struct device *dev)
+static int nmk_i2c_resume_early(struct device *dev)
 {
 	/* First go to the default state */
 	pinctrl_pm_select_default_state(dev);
@@ -914,9 +913,6 @@ static int nmk_i2c_resume(struct device *dev)
 
 	return 0;
 }
-#else
-#define nmk_i2c_suspend	NULL
-#define nmk_i2c_resume	NULL
 #endif
 
 #if CONFIG_PM
@@ -942,14 +938,8 @@ static int nmk_i2c_runtime_resume(struct device *dev)
 }
 #endif
 
-/*
- * We use noirq so that we suspend late and resume before the wakeup interrupt
- * to ensure that we do the !pm_runtime_suspended() check in resume before
- * there has been a regular pm runtime resume (via pm_runtime_get_sync()).
- */
 static const struct dev_pm_ops nmk_i2c_pm = {
-	.suspend_noirq	= nmk_i2c_suspend,
-	.resume_noirq	= nmk_i2c_resume,
+	SET_LATE_SYSTEM_SLEEP_PM_OPS(nmk_i2c_suspend_late, nmk_i2c_resume_early)
 	SET_PM_RUNTIME_PM_OPS(nmk_i2c_runtime_suspend,
 			nmk_i2c_runtime_resume,
 			NULL)
-- 
1.7.9.5

[PATCH 16/17] i2c: nomadik: Remove busy check for transfers at suspend late

[Ulf Hansson]

We should never be busy performing transfers at suspend late, thus
there are no reason to check for it.

Cc: Alessandro Rubini <rubini-9wsNiZum9E8@public.gmane.org>
Cc: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
Signed-off-by: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/i2c/busses/i2c-nomadik.c |   10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index eda4f0d..2004eea 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -179,7 +179,6 @@ struct i2c_nmk_client {
  * @stop: stop condition.
  * @xfer_complete: acknowledge completion for a I2C message.
  * @result: controller propogated result.
- * @busy: Busy doing transfer.
  */
 struct nmk_i2c_dev {
 	struct i2c_vendor_data		*vendor;
@@ -193,7 +192,6 @@ struct nmk_i2c_dev {
 	int				stop;
 	struct completion		xfer_complete;
 	int				result;
-	bool				busy;
 };
 
 /* controller's abort causes */
@@ -679,8 +677,6 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
 	struct nmk_i2c_dev *dev = i2c_get_adapdata(i2c_adap);
 	int j;
 
-	dev->busy = true;
-
 	pm_runtime_get_sync(&dev->adev->dev);
 
 	/* Attempt three times to send the message queue */
@@ -705,8 +701,6 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap,
 
 	pm_runtime_put_sync(&dev->adev->dev);
 
-	dev->busy = false;
-
 	/* return the no. messages processed */
 	if (status)
 		return status;
@@ -896,9 +890,6 @@ static int nmk_i2c_suspend_late(struct device *dev)
 	struct amba_device *adev = to_amba_device(dev);
 	struct nmk_i2c_dev *nmk_i2c = amba_get_drvdata(adev);
 
-	if (nmk_i2c->busy)
-		return -EBUSY;
-
 	pinctrl_pm_select_sleep_state(dev);
 
 	return 0;
@@ -1019,7 +1010,6 @@ static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
 		goto err_no_mem;
 	}
 	dev->vendor = vendor;
-	dev->busy = false;
 	dev->adev = adev;
 	amba_set_drvdata(adev, dev);
 
-- 
1.7.9.5

[PATCH 17/17] i2c: nomadik: Fixup system suspend

[Ulf Hansson]

For !CONFIG_PM_RUNTIME, the device were never put back into active
state while resuming.

For CONFIG_PM_RUNTIME, we blindly trusted the device to be inactive
while we were about to handle it at suspend late, which is just too
optimistic.

Even if the driver uses pm_runtime_put_sync() after each tranfer to
return it's runtime PM resources, there are no guarantees this will
actually mean the device will inactivated. The reason is that the PM
core will prevent runtime suspend during system suspend, and thus when
a transfer occurs during the early phases of system suspend the device
will be kept active after the transfer.

To handle both issues above, we need to re-use the runtime PM callbacks
and check the runtime PM state of the device before proceeding with our
operations for system suspend.

Cc: Alessandro Rubini <rubini-9wsNiZum9E8@public.gmane.org>
Cc: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
Signed-off-by: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/i2c/busses/i2c-nomadik.c |   29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index 2004eea..e6dbe66 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -887,21 +887,36 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg)
 #ifdef CONFIG_PM_SLEEP
 static int nmk_i2c_suspend_late(struct device *dev)
 {
-	struct amba_device *adev = to_amba_device(dev);
-	struct nmk_i2c_dev *nmk_i2c = amba_get_drvdata(adev);
+	if (!pm_runtime_status_suspended(dev)) {
+		int ret = 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)
+			return ret;
+
+		pm_runtime_set_suspended(dev);
+	}
 
 	pinctrl_pm_select_sleep_state(dev);
-
 	return 0;
 }
 
 static int nmk_i2c_resume_early(struct device *dev)
 {
-	/* First go to the default state */
-	pinctrl_pm_select_default_state(dev);
-	/* Then let's idle the pins until the next transfer happens */
-	pinctrl_pm_select_idle_state(dev);
+	int ret = 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);
+	if (ret) {
+		dev_err(dev, "problem resuming\n");
+		return ret;
+	}
 
+	pm_runtime_set_active(dev);
 	return 0;
 }
 #endif
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/17] amba: PM fixups for amba bus and some amba drivers

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 388 bytes --]

On Tue, Feb 04, 2014 at 04:58:41PM +0100, Ulf Hansson wrote:

> The fixes for the amba bus needs to be merged prior to the other, thus I think
> it could make sense to merge this complete patchset through Russell's tree,
> if he and the other maintainers think this is okay.

What are the actual dependencies for the SPI bit?  AFAICT it's probably
the first patch needs the bus updating?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 00/17] amba: PM fixups for amba bus and some amba drivers

[Ulf Hansson]

On 4 February 2014 19:49, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Feb 04, 2014 at 04:58:41PM +0100, Ulf Hansson wrote:
>
>> The fixes for the amba bus needs to be merged prior to the other, thus I think
>> it could make sense to merge this complete patchset through Russell's tree,
>> if he and the other maintainers think this is okay.
>
> What are the actual dependencies for the SPI bit?  AFAICT it's probably
> the first patch needs the bus updating?

Correct!

At this point I thought it made sense to collect all this patches,
since it adopts to use the new SET_PM_RUNTIME_PM_OPS macro from the PM
core.

In principle I wanted to visualize in one go how driver's system
suspend|resume callbacks could be implemented for three similar
scenarios.

Kind regards
Ulf Hansson