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

[Krzysztof Kozlowski <k.kozlowski@samsung.com>]

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