diff mbox

[v9,2/4] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM

Message ID 1415105570-7871-3-git-send-email-k.kozlowski@samsung.com (mailing list archive)
State Superseded
Headers show

Commit Message

Krzysztof Kozlowski Nov. 4, 2014, 12:52 p.m. UTC
The AMBA bus driver defines runtime Power Management functions which
disable and unprepare AMBA bus clock. This is problematic for runtime PM
because unpreparing a clock might sleep so it is not interrupt safe.

However some drivers may want to implement runtime PM functions in
interrupt-safe way (see pm_runtime_irq_safe()). In such case the AMBA
bus driver should only disable/enable the clock in runtime suspend and
resume callbacks.

Detect the device driver behavior during runtime suspend. During runtime
resume deal with clocks according to stored value.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/amba/bus.c       | 24 ++++++++++++++++++++----
 include/linux/amba/bus.h |  1 +
 2 files changed, 21 insertions(+), 4 deletions(-)

Comments

Ulf Hansson Nov. 4, 2014, 7:06 p.m. UTC | #1
On 4 November 2014 13:52, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote:
> The AMBA bus driver defines runtime Power Management functions which
> disable and unprepare AMBA bus clock. This is problematic for runtime PM
> because unpreparing a clock might sleep so it is not interrupt safe.
>
> However some drivers may want to implement runtime PM functions in
> interrupt-safe way (see pm_runtime_irq_safe()). In such case the AMBA
> bus driver should only disable/enable the clock in runtime suspend and
> resume callbacks.
>
> Detect the device driver behavior during runtime suspend. During runtime
> resume deal with clocks according to stored value.
>
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> ---
>  drivers/amba/bus.c       | 24 ++++++++++++++++++++----
>  include/linux/amba/bus.h |  1 +
>  2 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 47bbdc1b5be3..27ec8882ec8e 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -85,6 +85,13 @@ static struct device_attribute amba_dev_attrs[] = {
>  };
>
>  #ifdef CONFIG_PM
> +
> +#ifdef CONFIG_PM_RUNTIME
> +#define get_pm_runtime_irq_safe(dev)   ((dev)->power.irq_safe)
> +#else
> +#define get_pm_runtime_irq_safe(dev)   1

No, this doesn't work for those drivers that isn't configured as
irq_safe. The pm_runtime_force_suspend() invoked from the driver's
system PM suspend callback, will expect clocks to be unprepared when
CONFIG_PM_RUNTIME is unset.

To address this issue, in principle we need to know whether the driver
has invoked pm_runtime_irq_safe(), even when CONFIG_PM_RUNTIME is
unset.

This may be solved by the help of PM core:
1) Move the irq_safe member in the struct dev_pm_info, outside the
#ifdef CONFIG_PM_RUNTIME.
2) Let the pm_runtime_irq_safe() function be implemented for CONFIG_PM
instead of CONFIG_PM_RUNTIME.

If the PM core maintainers don't like that approach we can always add
an amba specific wrapper function for pm_runtime_irq_safe() and store
the information needed in the struct amba_device.

Kind regards
Uffe

> +#endif
> +
>  /*
>   * 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
> @@ -95,8 +102,14 @@ static int amba_pm_runtime_suspend(struct device *dev)
>         struct amba_device *pcdev = to_amba_device(dev);
>         int ret = pm_generic_runtime_suspend(dev);
>
> -       if (ret == 0 && dev->driver)
> -               clk_disable_unprepare(pcdev->pclk);
> +       if (ret == 0 && dev->driver) {
> +               pcdev->irq_safe = get_pm_runtime_irq_safe(dev);
> +
> +               if (pcdev->irq_safe)
> +                       clk_disable(pcdev->pclk);
> +               else
> +                       clk_disable_unprepare(pcdev->pclk);
> +       }
>
>         return ret;
>  }
> @@ -107,7 +120,10 @@ static int amba_pm_runtime_resume(struct device *dev)
>         int ret;
>
>         if (dev->driver) {
> -               ret = clk_prepare_enable(pcdev->pclk);
> +               if (pcdev->irq_safe)
> +                       ret = clk_enable(pcdev->pclk);
> +               else
> +                       ret = clk_prepare_enable(pcdev->pclk);
>                 /* Failure is probably fatal to the system, but... */
>                 if (ret)
>                         return ret;
> @@ -115,7 +131,7 @@ static int amba_pm_runtime_resume(struct device *dev)
>
>         return pm_generic_runtime_resume(dev);
>  }
> -#endif
> +#endif /* CONFIG_PM */
>
>  static const struct dev_pm_ops amba_pm = {
>         .suspend        = pm_generic_suspend,
> diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
> index ac02f9bd63dc..c4bae79851fb 100644
> --- a/include/linux/amba/bus.h
> +++ b/include/linux/amba/bus.h
> @@ -32,6 +32,7 @@ struct amba_device {
>         struct clk              *pclk;
>         unsigned int            periphid;
>         unsigned int            irq[AMBA_NR_IRQS];
> +       unsigned int            irq_safe:1;
>  };
>
>  struct amba_driver {
> --
> 1.9.1
>
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pavel Machek Nov. 4, 2014, 8:18 p.m. UTC | #2
On Tue 2014-11-04 13:52:48, Krzysztof Kozlowski wrote:
> The AMBA bus driver defines runtime Power Management functions which
> disable and unprepare AMBA bus clock. This is problematic for runtime PM
> because unpreparing a clock might sleep so it is not interrupt safe.
> 
> However some drivers may want to implement runtime PM functions in
> interrupt-safe way (see pm_runtime_irq_safe()). In such case the AMBA
> bus driver should only disable/enable the clock in runtime suspend and
> resume callbacks.



>  /*
>   * 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
> @@ -95,8 +102,14 @@ static int amba_pm_runtime_suspend(struct device *dev)
>  	struct amba_device *pcdev = to_amba_device(dev);
>  	int ret = pm_generic_runtime_suspend(dev);
>  
> -	if (ret == 0 && dev->driver)
> -		clk_disable_unprepare(pcdev->pclk);
> +	if (ret == 0 && dev->driver) {
> +		pcdev->irq_safe = get_pm_runtime_irq_safe(dev);
> +
> +		if (pcdev->irq_safe)
> +			clk_disable(pcdev->pclk);
> +		else
> +			clk_disable_unprepare(pcdev->pclk);
> +	}

So you can handle the case of !pcdev->irq_safe. What is the penalty
for always assuming !pcdev->irq_safe?
									Pavel
Krzysztof Kozlowski Nov. 5, 2014, 8:42 a.m. UTC | #3
On wto, 2014-11-04 at 21:18 +0100, Pavel Machek wrote:
> On Tue 2014-11-04 13:52:48, Krzysztof Kozlowski wrote:
> > The AMBA bus driver defines runtime Power Management functions which
> > disable and unprepare AMBA bus clock. This is problematic for runtime PM
> > because unpreparing a clock might sleep so it is not interrupt safe.
> > 
> > However some drivers may want to implement runtime PM functions in
> > interrupt-safe way (see pm_runtime_irq_safe()). In such case the AMBA
> > bus driver should only disable/enable the clock in runtime suspend and
> > resume callbacks.
> 
> 
> 
> >  /*
> >   * 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
> > @@ -95,8 +102,14 @@ static int amba_pm_runtime_suspend(struct device *dev)
> >  	struct amba_device *pcdev = to_amba_device(dev);
> >  	int ret = pm_generic_runtime_suspend(dev);
> >  
> > -	if (ret == 0 && dev->driver)
> > -		clk_disable_unprepare(pcdev->pclk);
> > +	if (ret == 0 && dev->driver) {
> > +		pcdev->irq_safe = get_pm_runtime_irq_safe(dev);
> > +
> > +		if (pcdev->irq_safe)
> > +			clk_disable(pcdev->pclk);
> > +		else
> > +			clk_disable_unprepare(pcdev->pclk);
> > +	}
> 
> So you can handle the case of !pcdev->irq_safe. What is the penalty
> for always assuming !pcdev->irq_safe?

The penalty (for pl330 driver) would be that the runtime resume/suspend
cannot happen from atomic context
  => pm_runtime_get_sync() cannot be called from atomic context
    => complete rework of runtime PM for pl330 DMA driver because now
       one of pm_runtime_get_sync() calls is in device_issue_pending
       callback which may not sleep. And by "rework" I also mean that
       I do not know how to do this... yet.

Best regards,
Krzysztof





--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Krzysztof Kozlowski Nov. 5, 2014, 8:48 a.m. UTC | #4
On wto, 2014-11-04 at 20:06 +0100, Ulf Hansson wrote:
> On 4 November 2014 13:52, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote:
> > The AMBA bus driver defines runtime Power Management functions which
> > disable and unprepare AMBA bus clock. This is problematic for runtime PM
> > because unpreparing a clock might sleep so it is not interrupt safe.
> >
> > However some drivers may want to implement runtime PM functions in
> > interrupt-safe way (see pm_runtime_irq_safe()). In such case the AMBA
> > bus driver should only disable/enable the clock in runtime suspend and
> > resume callbacks.
> >
> > Detect the device driver behavior during runtime suspend. During runtime
> > resume deal with clocks according to stored value.
> >
> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > ---
> >  drivers/amba/bus.c       | 24 ++++++++++++++++++++----
> >  include/linux/amba/bus.h |  1 +
> >  2 files changed, 21 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> > index 47bbdc1b5be3..27ec8882ec8e 100644
> > --- a/drivers/amba/bus.c
> > +++ b/drivers/amba/bus.c
> > @@ -85,6 +85,13 @@ static struct device_attribute amba_dev_attrs[] = {
> >  };
> >
> >  #ifdef CONFIG_PM
> > +
> > +#ifdef CONFIG_PM_RUNTIME
> > +#define get_pm_runtime_irq_safe(dev)   ((dev)->power.irq_safe)
> > +#else
> > +#define get_pm_runtime_irq_safe(dev)   1
> 
> No, this doesn't work for those drivers that isn't configured as
> irq_safe. The pm_runtime_force_suspend() invoked from the driver's
> system PM suspend callback, will expect clocks to be unprepared when
> CONFIG_PM_RUNTIME is unset.

D'oh! You're right... 

> To address this issue, in principle we need to know whether the driver
> has invoked pm_runtime_irq_safe(), even when CONFIG_PM_RUNTIME is
> unset.
> 
> This may be solved by the help of PM core:
> 1) Move the irq_safe member in the struct dev_pm_info, outside the
> #ifdef CONFIG_PM_RUNTIME.
> 2) Let the pm_runtime_irq_safe() function be implemented for CONFIG_PM
> instead of CONFIG_PM_RUNTIME.

Before I'll start implementing this - could PM maintainers share their
opinion on such change?

Best regards,
Krzysztof


> If the PM core maintainers don't like that approach we can always add
> an amba specific wrapper function for pm_runtime_irq_safe() and store
> the information needed in the struct amba_device.
> 
> Kind regards
> Uffe


--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pavel Machek Nov. 7, 2014, 12:13 p.m. UTC | #5
On Wed 2014-11-05 09:42:58, Krzysztof Kozlowski wrote:
> On wto, 2014-11-04 at 21:18 +0100, Pavel Machek wrote:
> > On Tue 2014-11-04 13:52:48, Krzysztof Kozlowski wrote:
> > > The AMBA bus driver defines runtime Power Management functions which
> > > disable and unprepare AMBA bus clock. This is problematic for runtime PM
> > > because unpreparing a clock might sleep so it is not interrupt safe.
> > > 
> > > However some drivers may want to implement runtime PM functions in
> > > interrupt-safe way (see pm_runtime_irq_safe()). In such case the AMBA
> > > bus driver should only disable/enable the clock in runtime suspend and
> > > resume callbacks.
> > 
> > 
> > 
> > >  /*
> > >   * 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
> > > @@ -95,8 +102,14 @@ static int amba_pm_runtime_suspend(struct device *dev)
> > >  	struct amba_device *pcdev = to_amba_device(dev);
> > >  	int ret = pm_generic_runtime_suspend(dev);
> > >  
> > > -	if (ret == 0 && dev->driver)
> > > -		clk_disable_unprepare(pcdev->pclk);
> > > +	if (ret == 0 && dev->driver) {
> > > +		pcdev->irq_safe = get_pm_runtime_irq_safe(dev);
> > > +
> > > +		if (pcdev->irq_safe)
> > > +			clk_disable(pcdev->pclk);
> > > +		else
> > > +			clk_disable_unprepare(pcdev->pclk);
> > > +	}
> > 
> > So you can handle the case of !pcdev->irq_safe. What is the penalty
> > for always assuming !pcdev->irq_safe?
> 
> The penalty (for pl330 driver) would be that the runtime resume/suspend
> cannot happen from atomic context
>   => pm_runtime_get_sync() cannot be called from atomic context
>     => complete rework of runtime PM for pl330 DMA driver because now
>        one of pm_runtime_get_sync() calls is in device_issue_pending
>        callback which may not sleep. And by "rework" I also mean that
>        I do not know how to do this... yet.

I still don't get it. You say that you don't know how to handle
!pcdev->irq_safe case... Yet have code above that tries to handle it.

If that case can't be sanely handled, I'd expect
BUG_ON(!pcdev->irq_safe).
									Pavel
Krzysztof Kozlowski Nov. 7, 2014, 12:18 p.m. UTC | #6
On pi?, 2014-11-07 at 13:13 +0100, Pavel Machek wrote:
> On Wed 2014-11-05 09:42:58, Krzysztof Kozlowski wrote:
> > On wto, 2014-11-04 at 21:18 +0100, Pavel Machek wrote:
> > > On Tue 2014-11-04 13:52:48, Krzysztof Kozlowski wrote:
> > > > The AMBA bus driver defines runtime Power Management functions which
> > > > disable and unprepare AMBA bus clock. This is problematic for runtime PM
> > > > because unpreparing a clock might sleep so it is not interrupt safe.
> > > > 
> > > > However some drivers may want to implement runtime PM functions in
> > > > interrupt-safe way (see pm_runtime_irq_safe()). In such case the AMBA
> > > > bus driver should only disable/enable the clock in runtime suspend and
> > > > resume callbacks.
> > > 
> > > 
> > > 
> > > >  /*
> > > >   * 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
> > > > @@ -95,8 +102,14 @@ static int amba_pm_runtime_suspend(struct device *dev)
> > > >  	struct amba_device *pcdev = to_amba_device(dev);
> > > >  	int ret = pm_generic_runtime_suspend(dev);
> > > >  
> > > > -	if (ret == 0 && dev->driver)
> > > > -		clk_disable_unprepare(pcdev->pclk);
> > > > +	if (ret == 0 && dev->driver) {
> > > > +		pcdev->irq_safe = get_pm_runtime_irq_safe(dev);
> > > > +
> > > > +		if (pcdev->irq_safe)
> > > > +			clk_disable(pcdev->pclk);
> > > > +		else
> > > > +			clk_disable_unprepare(pcdev->pclk);
> > > > +	}
> > > 
> > > So you can handle the case of !pcdev->irq_safe. What is the penalty
> > > for always assuming !pcdev->irq_safe?
> > 
> > The penalty (for pl330 driver) would be that the runtime resume/suspend
> > cannot happen from atomic context
> >   => pm_runtime_get_sync() cannot be called from atomic context
> >     => complete rework of runtime PM for pl330 DMA driver because now
> >        one of pm_runtime_get_sync() calls is in device_issue_pending
> >        callback which may not sleep. And by "rework" I also mean that
> >        I do not know how to do this... yet.
> 
> I still don't get it. You say that you don't know how to handle
> !pcdev->irq_safe case... Yet have code above that tries to handle it.
> 
> If that case can't be sanely handled, I'd expect
> BUG_ON(!pcdev->irq_safe).

Hmmm... I could misunderstand your question. The amba/bus.c driver can
handle both cases. However this varies for child drivers (which use
these runtime PM callbacks too). For pl330 cannot handle non-irq-safe.
Other drivers can.

Is it the answer for your question?

Best regards,
Krzysztof


--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pavel Machek Nov. 7, 2014, 12:28 p.m. UTC | #7
On Fri 2014-11-07 13:18:03, Krzysztof Kozlowski wrote:
> On pi?, 2014-11-07 at 13:13 +0100, Pavel Machek wrote:
> > On Wed 2014-11-05 09:42:58, Krzysztof Kozlowski wrote:
> > > On wto, 2014-11-04 at 21:18 +0100, Pavel Machek wrote:
> > > > On Tue 2014-11-04 13:52:48, Krzysztof Kozlowski wrote:
> > > > > The AMBA bus driver defines runtime Power Management functions which
> > > > > disable and unprepare AMBA bus clock. This is problematic for runtime PM
> > > > > because unpreparing a clock might sleep so it is not interrupt safe.
> > > > > 
> > > > > However some drivers may want to implement runtime PM functions in
> > > > > interrupt-safe way (see pm_runtime_irq_safe()). In such case the AMBA
> > > > > bus driver should only disable/enable the clock in runtime suspend and
> > > > > resume callbacks.
> > > > 
> > > > 
> > > > 
> > > > >  /*
> > > > >   * 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
> > > > > @@ -95,8 +102,14 @@ static int amba_pm_runtime_suspend(struct device *dev)
> > > > >  	struct amba_device *pcdev = to_amba_device(dev);
> > > > >  	int ret = pm_generic_runtime_suspend(dev);
> > > > >  
> > > > > -	if (ret == 0 && dev->driver)
> > > > > -		clk_disable_unprepare(pcdev->pclk);
> > > > > +	if (ret == 0 && dev->driver) {
> > > > > +		pcdev->irq_safe = get_pm_runtime_irq_safe(dev);
> > > > > +
> > > > > +		if (pcdev->irq_safe)
> > > > > +			clk_disable(pcdev->pclk);
> > > > > +		else
> > > > > +			clk_disable_unprepare(pcdev->pclk);
> > > > > +	}
> > > > 
> > > > So you can handle the case of !pcdev->irq_safe. What is the penalty
> > > > for always assuming !pcdev->irq_safe?
> > > 
> > > The penalty (for pl330 driver) would be that the runtime resume/suspend
> > > cannot happen from atomic context
> > >   => pm_runtime_get_sync() cannot be called from atomic context
> > >     => complete rework of runtime PM for pl330 DMA driver because now
> > >        one of pm_runtime_get_sync() calls is in device_issue_pending
> > >        callback which may not sleep. And by "rework" I also mean that
> > >        I do not know how to do this... yet.
> > 
> > I still don't get it. You say that you don't know how to handle
> > !pcdev->irq_safe case... Yet have code above that tries to handle it.
> > 
> > If that case can't be sanely handled, I'd expect
> > BUG_ON(!pcdev->irq_safe).
> 
> Hmmm... I could misunderstand your question. The amba/bus.c driver can
> handle both cases. However this varies for child drivers (which use
> these runtime PM callbacks too). For pl330 cannot handle non-irq-safe.
> Other drivers can.

Ok, so pl330 can't handle non-irq-safe callbacks. What about the other
solution preserving consistency -- can we make sure all callbacks are
irq-safe with acceptable penalty?

									Pavel
Krzysztof Kozlowski Nov. 7, 2014, 1:21 p.m. UTC | #8
On pi?, 2014-11-07 at 13:28 +0100, Pavel Machek wrote:
> On Fri 2014-11-07 13:18:03, Krzysztof Kozlowski wrote:
> > On pi?, 2014-11-07 at 13:13 +0100, Pavel Machek wrote:
> > > On Wed 2014-11-05 09:42:58, Krzysztof Kozlowski wrote:
> > > > On wto, 2014-11-04 at 21:18 +0100, Pavel Machek wrote:
> > > > > On Tue 2014-11-04 13:52:48, Krzysztof Kozlowski wrote:
> > > > > > The AMBA bus driver defines runtime Power Management functions which
> > > > > > disable and unprepare AMBA bus clock. This is problematic for runtime PM
> > > > > > because unpreparing a clock might sleep so it is not interrupt safe.
> > > > > > 
> > > > > > However some drivers may want to implement runtime PM functions in
> > > > > > interrupt-safe way (see pm_runtime_irq_safe()). In such case the AMBA
> > > > > > bus driver should only disable/enable the clock in runtime suspend and
> > > > > > resume callbacks.
> > > > > 
> > > > > 
> > > > > 
> > > > > >  /*
> > > > > >   * 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
> > > > > > @@ -95,8 +102,14 @@ static int amba_pm_runtime_suspend(struct device *dev)
> > > > > >  	struct amba_device *pcdev = to_amba_device(dev);
> > > > > >  	int ret = pm_generic_runtime_suspend(dev);
> > > > > >  
> > > > > > -	if (ret == 0 && dev->driver)
> > > > > > -		clk_disable_unprepare(pcdev->pclk);
> > > > > > +	if (ret == 0 && dev->driver) {
> > > > > > +		pcdev->irq_safe = get_pm_runtime_irq_safe(dev);
> > > > > > +
> > > > > > +		if (pcdev->irq_safe)
> > > > > > +			clk_disable(pcdev->pclk);
> > > > > > +		else
> > > > > > +			clk_disable_unprepare(pcdev->pclk);
> > > > > > +	}
> > > > > 
> > > > > So you can handle the case of !pcdev->irq_safe. What is the penalty
> > > > > for always assuming !pcdev->irq_safe?
> > > > 
> > > > The penalty (for pl330 driver) would be that the runtime resume/suspend
> > > > cannot happen from atomic context
> > > >   => pm_runtime_get_sync() cannot be called from atomic context
> > > >     => complete rework of runtime PM for pl330 DMA driver because now
> > > >        one of pm_runtime_get_sync() calls is in device_issue_pending
> > > >        callback which may not sleep. And by "rework" I also mean that
> > > >        I do not know how to do this... yet.
> > > 
> > > I still don't get it. You say that you don't know how to handle
> > > !pcdev->irq_safe case... Yet have code above that tries to handle it.
> > > 
> > > If that case can't be sanely handled, I'd expect
> > > BUG_ON(!pcdev->irq_safe).
> > 
> > Hmmm... I could misunderstand your question. The amba/bus.c driver can
> > handle both cases. However this varies for child drivers (which use
> > these runtime PM callbacks too). For pl330 cannot handle non-irq-safe.
> > Other drivers can.
> 
> Ok, so pl330 can't handle non-irq-safe callbacks. What about the other
> solution preserving consistency -- can we make sure all callbacks are
> irq-safe with acceptable penalty?

Aaahhh, I get it. We could revert 5303c0f46c87 ("ARM: 7916/1: amba: Add
clk_prepare|unprepare in runtime PM callbacks"). After quick grep
through amba drivers it seems that this should work and the only impact
would be that less energy could be saved on other drivers (clock only
disabled, not unprepared).

However someone more experienced in amba drivers should confirm this.

Best regards,
Krzysztof


--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 47bbdc1b5be3..27ec8882ec8e 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -85,6 +85,13 @@  static struct device_attribute amba_dev_attrs[] = {
 };
 
 #ifdef CONFIG_PM
+
+#ifdef CONFIG_PM_RUNTIME
+#define get_pm_runtime_irq_safe(dev)	((dev)->power.irq_safe)
+#else
+#define get_pm_runtime_irq_safe(dev)	1
+#endif
+
 /*
  * 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
@@ -95,8 +102,14 @@  static int amba_pm_runtime_suspend(struct device *dev)
 	struct amba_device *pcdev = to_amba_device(dev);
 	int ret = pm_generic_runtime_suspend(dev);
 
-	if (ret == 0 && dev->driver)
-		clk_disable_unprepare(pcdev->pclk);
+	if (ret == 0 && dev->driver) {
+		pcdev->irq_safe = get_pm_runtime_irq_safe(dev);
+
+		if (pcdev->irq_safe)
+			clk_disable(pcdev->pclk);
+		else
+			clk_disable_unprepare(pcdev->pclk);
+	}
 
 	return ret;
 }
@@ -107,7 +120,10 @@  static int amba_pm_runtime_resume(struct device *dev)
 	int ret;
 
 	if (dev->driver) {
-		ret = clk_prepare_enable(pcdev->pclk);
+		if (pcdev->irq_safe)
+			ret = clk_enable(pcdev->pclk);
+		else
+			ret = clk_prepare_enable(pcdev->pclk);
 		/* Failure is probably fatal to the system, but... */
 		if (ret)
 			return ret;
@@ -115,7 +131,7 @@  static int amba_pm_runtime_resume(struct device *dev)
 
 	return pm_generic_runtime_resume(dev);
 }
-#endif
+#endif /* CONFIG_PM */
 
 static const struct dev_pm_ops amba_pm = {
 	.suspend	= pm_generic_suspend,
diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
index ac02f9bd63dc..c4bae79851fb 100644
--- a/include/linux/amba/bus.h
+++ b/include/linux/amba/bus.h
@@ -32,6 +32,7 @@  struct amba_device {
 	struct clk		*pclk;
 	unsigned int		periphid;
 	unsigned int		irq[AMBA_NR_IRQS];
+	unsigned int		irq_safe:1;
 };
 
 struct amba_driver {