Re: [alsa-devel] [PATCH 1/2 v2] ASoC: sh: fsi: Add runtime PM support

Re: [alsa-devel] [PATCH 1/2 v2] ASoC: sh: fsi: Add runtime PM support

[Mark Brown]

On Mon, Nov 09, 2009 at 11:54:47AM +0900, Kuninori Morimoto wrote:
> This patch add support runtime PM.
> Driver callbacks for Runtime PM are empty because
> the device registers are always re-initialized after
> pm_runtime_get_sync(). The Runtime PM functions replaces the
> clock framework module stop bit handling in this driver.

> Signed-off-by: Kuninori Morimoto <morimoto.kuninori@renesas.com>

Hrm.  I'll have to see how this plays with ASoC core pm_runtime support
when that appears.  It should be OK as-is, I think.

> +	pm_suspend_ignore_children(&pdev->dev, true);
> +	pm_runtime_enable(&pdev->dev);
> +	pm_runtime_resume(&pdev->dev);

Why pm_suspend_ignore_all_children()?  I'd not expect the device to have
any children and if it did it doesn't seem like an entirely safe
assumption.

> +static int fsi_runtime_nop(struct device *dev)
> +{
> +	/* Runtime PM callback shared between ->runtime_suspend()
> +	 * and ->runtime_resume(). Simply returns success.
> +	 *
> +	 * This driver re-initializes all registers after
> +	 * pm_runtime_get_sync() anyway so there is no need
> +	 * to save and restore registers here.
> +	 */
> +	return 0;
> +}

This sets off alarm bells but it's perfectly reasonable, especially with
platforms able to put things into a low power state with no explicit
driver code now they can do power domain style things like SH.  I've
CCed in the PM folks since this seems like a perfectly reasonable use
case which ought to be handled more nicely.

Re: [alsa-devel] [PATCH 1/2 v2] ASoC: sh: fsi: Add runtime PM support

[Mark Brown]

On Mon, Nov 09, 2009 at 10:58:58AM +0000, Mark Brown wrote:

Adding in an address for Magnus which hopefully won't bounce and also
the linux-pm list which get_maintainers didn't find.

> On Mon, Nov 09, 2009 at 11:54:47AM +0900, Kuninori Morimoto wrote:
> > This patch add support runtime PM.
> > Driver callbacks for Runtime PM are empty because
> > the device registers are always re-initialized after
> > pm_runtime_get_sync(). The Runtime PM functions replaces the
> > clock framework module stop bit handling in this driver.
> 
> > Signed-off-by: Kuninori Morimoto <morimoto.kuninori@renesas.com>
> 
> Hrm.  I'll have to see how this plays with ASoC core pm_runtime support
> when that appears.  It should be OK as-is, I think.
> 
> > +	pm_suspend_ignore_children(&pdev->dev, true);
> > +	pm_runtime_enable(&pdev->dev);
> > +	pm_runtime_resume(&pdev->dev);
> 
> Why pm_suspend_ignore_all_children()?  I'd not expect the device to have
> any children and if it did it doesn't seem like an entirely safe
> assumption.
> 
> > +static int fsi_runtime_nop(struct device *dev)
> > +{
> > +	/* Runtime PM callback shared between ->runtime_suspend()
> > +	 * and ->runtime_resume(). Simply returns success.
> > +	 *
> > +	 * This driver re-initializes all registers after
> > +	 * pm_runtime_get_sync() anyway so there is no need
> > +	 * to save and restore registers here.
> > +	 */
> > +	return 0;
> > +}
> 
> This sets off alarm bells but it's perfectly reasonable, especially with
> platforms able to put things into a low power state with no explicit
> driver code now they can do power domain style things like SH.  I've
> CCed in the PM folks since this seems like a perfectly reasonable use
> case which ought to be handled more nicely.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

-- 
"You grabbed my hand and we fell into it, like a daydream - or a fever."

Null suspend/resume functions

[Mark Brown]

On Mon, Nov 09, 2009 at 01:31:36PM +0000, Mark Brown wrote:
> On Mon, Nov 09, 2009 at 10:58:58AM +0000, Mark Brown wrote:
> > On Mon, Nov 09, 2009 at 11:54:47AM +0900, Kuninori Morimoto wrote:

Any chance someone from the PM side could comment on the issue below?

> > > +static int fsi_runtime_nop(struct device *dev)
> > > +{
> > > +	/* Runtime PM callback shared between ->runtime_suspend()
> > > +	 * and ->runtime_resume(). Simply returns success.
> > > +	 *
> > > +	 * This driver re-initializes all registers after
> > > +	 * pm_runtime_get_sync() anyway so there is no need
> > > +	 * to save and restore registers here.
> > > +	 */
> > > +	return 0;
> > > +}
> > 
> > This sets off alarm bells but it's perfectly reasonable, especially with
> > platforms able to put things into a low power state with no explicit
> > driver code now they can do power domain style things like SH.  I've
> > CCed in the PM folks since this seems like a perfectly reasonable use
> > case which ought to be handled more nicely.

Re: Null suspend/resume functions

[Rafael J. Wysocki]

On Monday 16 November 2009, Mark Brown wrote:
> On Mon, Nov 09, 2009 at 01:31:36PM +0000, Mark Brown wrote:
> > On Mon, Nov 09, 2009 at 10:58:58AM +0000, Mark Brown wrote:
> > > On Mon, Nov 09, 2009 at 11:54:47AM +0900, Kuninori Morimoto wrote:
> 
> Any chance someone from the PM side could comment on the issue below?

There is.  I haven't had a chance to look at it yet, will do shortly.

> > > > +static int fsi_runtime_nop(struct device *dev)
> > > > +{
> > > > +	/* Runtime PM callback shared between ->runtime_suspend()
> > > > +	 * and ->runtime_resume(). Simply returns success.
> > > > +	 *
> > > > +	 * This driver re-initializes all registers after
> > > > +	 * pm_runtime_get_sync() anyway so there is no need
> > > > +	 * to save and restore registers here.
> > > > +	 */
> > > > +	return 0;
> > > > +}
> > > 
> > > This sets off alarm bells but it's perfectly reasonable, especially with
> > > platforms able to put things into a low power state with no explicit
> > > driver code now they can do power domain style things like SH.  I've
> > > CCed in the PM folks since this seems like a perfectly reasonable use
> > > case which ought to be handled more nicely.

Thanks,
Rafael

Re: [linux-pm] Null suspend/resume functions

[Pavel Machek]

On Mon 2009-11-16 15:30:00, Mark Brown wrote:
> On Mon, Nov 09, 2009 at 01:31:36PM +0000, Mark Brown wrote:
> > On Mon, Nov 09, 2009 at 10:58:58AM +0000, Mark Brown wrote:
> > > On Mon, Nov 09, 2009 at 11:54:47AM +0900, Kuninori Morimoto wrote:
> 
> Any chance someone from the PM side could comment on the issue below?
> 
> > > > +static int fsi_runtime_nop(struct device *dev)
> > > > +{
> > > > +	/* Runtime PM callback shared between ->runtime_suspend()
> > > > +	 * and ->runtime_resume(). Simply returns success.
> > > > +	 *
> > > > +	 * This driver re-initializes all registers after
> > > > +	 * pm_runtime_get_sync() anyway so there is no need
> > > > +	 * to save and restore registers here.
> > > > +	 */
> > > > +	return 0;
> > > > +}
> > > 
> > > This sets off alarm bells but it's perfectly reasonable, especially with
> > > platforms able to put things into a low power state with no explicit
> > > driver code now they can do power domain style things like SH.  I've
> > > CCed in the PM folks since this seems like a perfectly reasonable use
> > > case which ought to be handled more nicely.

I believe that having few nop functions around the tree should not be
huge problem. If it is, you can introduce one shared top function into
the core...
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [linux-pm] Null suspend/resume functions

[Mark Brown]

On Tue, Nov 17, 2009 at 12:52:36PM +0100, Pavel Machek wrote:

> I believe that having few nop functions around the tree should not be
> huge problem. If it is, you can introduce one shared top function into
> the core...

The problem I have with that is that for most APIs noop functions are a
big fat warning sign that something is going wrong and the API is being
abused.  This then creates noise and code review problems in the driver
code since you've got something that normally suggests a problem.

Re: [linux-pm] Null suspend/resume functions

[Pavel Machek]

On Tue 2009-11-17 12:41:25, Mark Brown wrote:
> On Tue, Nov 17, 2009 at 12:52:36PM +0100, Pavel Machek wrote:
> 
> > I believe that having few nop functions around the tree should not be
> > huge problem. If it is, you can introduce one shared top function into
> > the core...
> 
> The problem I have with that is that for most APIs noop functions are a
> big fat warning sign that something is going wrong and the API is being
> abused.  This then creates noise and code review problems in the driver
> code since you've got something that normally suggests a problem.

That still sounds like poor reason to add tests to core. But return 0
function for that purpose should be ok (and should make code easy to
review, too).

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [linux-pm] Null suspend/resume functions

[Mark Brown]

On Wed, Nov 18, 2009 at 05:09:08PM +0100, Pavel Machek wrote:
> On Tue 2009-11-17 12:41:25, Mark Brown wrote:

> > The problem I have with that is that for most APIs noop functions are a
> > big fat warning sign that something is going wrong and the API is being
> > abused.  This then creates noise and code review problems in the driver
> > code since you've got something that normally suggests a problem.

> That still sounds like poor reason to add tests to core. But return 0
> function for that purpose should be ok (and should make code easy to
> review, too).

What makes you believe that this is a poor reason?  The issue isn't that
the driver code is complex, the issue is that it's noise in the driver
which suggests that the driver isn't doing something it's supposed to
do.

Re: [linux-pm] Null suspend/resume functions

[Pavel Machek]

On Thu 2009-11-19 11:21:03, Mark Brown wrote:
> On Wed, Nov 18, 2009 at 05:09:08PM +0100, Pavel Machek wrote:
> > On Tue 2009-11-17 12:41:25, Mark Brown wrote:
> 
> > > The problem I have with that is that for most APIs noop functions are a
> > > big fat warning sign that something is going wrong and the API is being
> > > abused.  This then creates noise and code review problems in the driver
> > > code since you've got something that normally suggests a problem.
> 
> > That still sounds like poor reason to add tests to core. But return 0
> > function for that purpose should be ok (and should make code easy to
> > review, too).
> 
> What makes you believe that this is a poor reason?  The issue isn't that
> the driver code is complex, the issue is that it's noise in the driver
> which suggests that the driver isn't doing something it's supposed to
> do.

So you place a comment there; it should be there anyway. Having nop
during suspend/resume *is* unusual, and it should raise red flags.

Plus, if we allowed NULLs there, we'd not know if the driver does not
implement it because it is not neccessary, or because they don't care.
 
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [linux-pm] Null suspend/resume functions

[Mark Brown]

On Sun, Nov 22, 2009 at 12:45:10AM +0100, Pavel Machek wrote:

> So you place a comment there; it should be there anyway. Having nop
> during suspend/resume *is* unusual, and it should raise red flags.

This isn't a good assumption here.  Remember that this is for runtime PM
so if we're getting as far as these calls then the driver has already
told the core that it is idle, which probably means that the hardware is
already quieseced.  For a lot of hardware that will mean that the only
thing left to do in order to suspend is to (possibly) remove power and
in many cases (as with SH) that's going to be done by the bus rather
than the device.  It's a bit more likely that some activity will be
needed on resume but in many cases the device gets fully reprogrammed on
when it becomes active anyway.

Looking at it another way a key goal of runtime PM is to get the bus
involved in the process - if we can do things at device level to reduce
power consumption then there's no need to go through runtime PM to do
them.

> Plus, if we allowed NULLs there, we'd not know if the driver does not
> implement it because it is not neccessary, or because they don't care.

Again, in order to use runtime PM the driver must already be doing
explicit calls so it should be fairly clear that the driver is trying to
support suspend.

Re: [linux-pm] Null suspend/resume functions

[Pavel Machek]

Hi!

> > So you place a comment there; it should be there anyway. Having nop
> > during suspend/resume *is* unusual, and it should raise red flags.
> 
> This isn't a good assumption here.  Remember that this is for runtime PM
> so if we're getting as far as these calls then the driver has already
> told the core that it is idle, which probably means that the hardware is
> already quieseced.  For a lot of hardware that will mean that the

Ok, I did not realize we were talking runtime pm here.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: Null suspend/resume functions

[Magnus Damm]

Hi Mark,

On Tue, Nov 17, 2009 at 12:30 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Mon, Nov 09, 2009 at 01:31:36PM +0000, Mark Brown wrote:
>> On Mon, Nov 09, 2009 at 10:58:58AM +0000, Mark Brown wrote:
>> > On Mon, Nov 09, 2009 at 11:54:47AM +0900, Kuninori Morimoto wrote:
>
> Any chance someone from the PM side could comment on the issue below?
>
>> > > +static int fsi_runtime_nop(struct device *dev)
>> > > +{
>> > > + /* Runtime PM callback shared between ->runtime_suspend()
>> > > +  * and ->runtime_resume(). Simply returns success.
>> > > +  *
>> > > +  * This driver re-initializes all registers after
>> > > +  * pm_runtime_get_sync() anyway so there is no need
>> > > +  * to save and restore registers here.
>> > > +  */
>> > > + return 0;
>> > > +}
>> >
>> > This sets off alarm bells but it's perfectly reasonable, especially with
>> > platforms able to put things into a low power state with no explicit
>> > driver code now they can do power domain style things like SH.  I've
>> > CCed in the PM folks since this seems like a perfectly reasonable use
>> > case which ought to be handled more nicely.

On SuperH we have Runtime PM enabled on a few platforms together with
a few updated drivers. The latest driver to become more power aware is
this FSI driver.

It's all quite simple, in the FSI driver we enable the clock to the
FSI hardware using Runtime PM functions, and whenever the clock is
turned off the SuperH specific bus code _may_ decide to call
->runtime_suspend() and turn off the power domain. The "_may_" comes
from the fact that multiple hardware blocks may be grouped together
into a single power domain, and all devices in the power domain must
support Runtime PM and have their clocks disabled before we can turn
off the power.

At this point the SuperH specific platform bus code requires the
callbacks ->runtime_suspend() and ->runtime_resume() to be present. It
may be a good idea to allow them to be NULL in the future or maybe
having some shared functions, but before starting to break out stuff
I'd like to see how other Runtime PM implementations deal with this.
So unless people object I prefer to keep it as-is for now.

/ magnus

Re: Null suspend/resume functions

[Mark Brown]

On Tue, Nov 17, 2009 at 09:46:35PM +0900, Magnus Damm wrote:

> On SuperH we have Runtime PM enabled on a few platforms together with
> a few updated drivers. The latest driver to become more power aware is
> this FSI driver.

I understand exactly what the runtime PM stuff and the driver are doing
here, the issue is the mandatory suspend and resume functions.

> At this point the SuperH specific platform bus code requires the
> callbacks ->runtime_suspend() and ->runtime_resume() to be present. It
> may be a good idea to allow them to be NULL in the future or maybe
> having some shared functions, but before starting to break out stuff
> I'd like to see how other Runtime PM implementations deal with this.
> So unless people object I prefer to keep it as-is for now.

What is the reason for requiring that the driver provide stub functions?
For me the issue is that if it's mandatory for the driver to provide the
functions then having stub functions in there makes the driver look like
it is abusing the API by not implementing mandatory functionality.

Given that the arch is now dealing with clocking and power for the
device using the runtime PM system it seems fairly clear that there are
going to be drivers like this one that can at least skip the suspend
part and may not need to do anything at resume time either.

Re: Null suspend/resume functions

[Rafael J. Wysocki]

On Tuesday 17 November 2009, Mark Brown wrote:
> On Tue, Nov 17, 2009 at 09:46:35PM +0900, Magnus Damm wrote:
> 
> > On SuperH we have Runtime PM enabled on a few platforms together with
> > a few updated drivers. The latest driver to become more power aware is
> > this FSI driver.
> 
> I understand exactly what the runtime PM stuff and the driver are doing
> here, the issue is the mandatory suspend and resume functions.
> 
> > At this point the SuperH specific platform bus code requires the
> > callbacks ->runtime_suspend() and ->runtime_resume() to be present. It
> > may be a good idea to allow them to be NULL in the future or maybe
> > having some shared functions, but before starting to break out stuff
> > I'd like to see how other Runtime PM implementations deal with this.
> > So unless people object I prefer to keep it as-is for now.
> 
> What is the reason for requiring that the driver provide stub functions?
> For me the issue is that if it's mandatory for the driver to provide the
> functions then having stub functions in there makes the driver look like
> it is abusing the API by not implementing mandatory functionality.

In fact, it's not mandatory for bus types, not for drivers.  IMO bus types
really have to know how to suspend a device and how to resume it,
otherwise the core framework won't be useful anyway.  What the bus type does
about drivers not implementing ->runtime_suspend() or ->runtime_resume(), it's
up to the bus type.  That's even documented IIRC.

Thanks,
Rafael

Re: Null suspend/resume functions

[Mark Brown]

On Tue, Nov 17, 2009 at 11:14:04PM +0100, Rafael J. Wysocki wrote:

> In fact, it's not mandatory for bus types, not for drivers.  IMO bus types
> really have to know how to suspend a device and how to resume it,
> otherwise the core framework won't be useful anyway.  What the bus type does
> about drivers not implementing ->runtime_suspend() or ->runtime_resume(), it's
> up to the bus type.  That's even documented IIRC.

OK, thanks - I hadn't realised that these were being called directly
from the bus type code rather than by the core.

Re: Null suspend/resume functions

[Magnus Damm]

On Tue, Nov 17, 2009 at 9:59 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Tue, Nov 17, 2009 at 09:46:35PM +0900, Magnus Damm wrote:
>
>> On SuperH we have Runtime PM enabled on a few platforms together with
>> a few updated drivers. The latest driver to become more power aware is
>> this FSI driver.
>
> I understand exactly what the runtime PM stuff and the driver are doing
> here, the issue is the mandatory suspend and resume functions.

Cool, but don't you think it makes sense to see how other
architectures will deal with this first?

>> At this point the SuperH specific platform bus code requires the
>> callbacks ->runtime_suspend() and ->runtime_resume() to be present. It
>> may be a good idea to allow them to be NULL in the future or maybe
>> having some shared functions, but before starting to break out stuff
>> I'd like to see how other Runtime PM implementations deal with this.
>> So unless people object I prefer to keep it as-is for now.
>
> What is the reason for requiring that the driver provide stub functions?
> For me the issue is that if it's mandatory for the driver to provide the
> functions then having stub functions in there makes the driver look like
> it is abusing the API by not implementing mandatory functionality.

I see your point, but there is another side to it as well. Having the
stubs there is a way of showing that these functions may be called as
part of the Runtime PM management. Not having them would confuse
people even more IMO.

> Given that the arch is now dealing with clocking and power for the
> device using the runtime PM system it seems fairly clear that there are
> going to be drivers like this one that can at least skip the suspend
> part and may not need to do anything at resume time either.

Right. We just need to figure out what the arch-independent solution
would be, and we're unfortunately not there yet.

I can spend some time on updating the drivers and removing the
nop-stubs if you'd like, but I'd rather not since I suspect that I'll
have to update things again when we get Runtime PM for ARM or other
non-sh architectures.

Let me know what you think!

/ magnus

Re: Null suspend/resume functions

[Mark Brown]

On Wed, Nov 18, 2009 at 07:09:14PM +0900, Magnus Damm wrote:
> On Tue, Nov 17, 2009 at 9:59 PM, Mark Brown

> > I understand exactly what the runtime PM stuff and the driver are doing
> > here, the issue is the mandatory suspend and resume functions.

> Cool, but don't you think it makes sense to see how other
> architectures will deal with this first?

Of course, the other way of looking at it would be that it'd be good to
provide the best possible implementation in the first architecture so
that others can draw inspiration from it.

Looking at what you've got it seems clearly sane as-is modulo this issue
and I can see how it maps onto the Samsung/OMAP style hardware so it
doesn't set off any alarm bells for me on that front.  The only thing
I'm a bit unsure about with your implementation is mixing in the clock
gating but to be honest that's something that could be changed on a per
device basis anyway.

> > What is the reason for requiring that the driver provide stub functions?
> > For me the issue is that if it's mandatory for the driver to provide the
> > functions then having stub functions in there makes the driver look like
> > it is abusing the API by not implementing mandatory functionality.

> I see your point, but there is another side to it as well. Having the
> stubs there is a way of showing that these functions may be called as
> part of the Runtime PM management. Not having them would confuse
> people even more IMO.

Given that they're part of dev_pm_ops which already does such things as
routine I'm not sure that it's particularly confusing.  To me it's
confusing that one might be required to do something at runtime suspend
time - since the driver has to explicitly tell the PM core that the
device is idle so it seems reasonable to expect that the hardware would
already be quiesced and all that'd be needed would be to power down the
block.

> I can spend some time on updating the drivers and removing the
> nop-stubs if you'd like, but I'd rather not since I suspect that I'll
> have to update things again when we get Runtime PM for ARM or other
> non-sh architectures.

Like I say, the SH doesn't sound fundamentally different to the ARMs
here.

Re: [alsa-devel] [PATCH 1/2 v2] ASoC: sh: fsi: Add runtime PM support

[Mark Brown]

On Mon, Nov 09, 2009 at 10:58:58AM +0000, Mark Brown wrote:
> On Mon, Nov 09, 2009 at 11:54:47AM +0900, Kuninori Morimoto wrote:

> > +	pm_suspend_ignore_children(&pdev->dev, true);
> > +	pm_runtime_enable(&pdev->dev);
> > +	pm_runtime_resume(&pdev->dev);

> Why pm_suspend_ignore_all_children()?  I'd not expect the device to have
> any children and if it did it doesn't seem like an entirely safe
> assumption.

I've not yet seen any response to this query?