[PATCH] sh_eth: pm_runtime should not need null operations

[PATCH] sh_eth: pm_runtime should not need null operations

[Geert Uytterhoeven]

Hi Ben,

(dropping netdev and davem, adding Felipe, Kevin, linux-pm, and
linux-arm-kernel)

On Fri, Mar 21, 2014 at 11:57 AM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> On 21/03/14 11:30, Geert Uytterhoeven wrote:
>> On Fri, Mar 21, 2014 at 11:15 AM, Ben Dooks <ben.dooks@codethink.co.uk>
>> wrote:
>>> The driver has a no-op for the pm_runtime callbacks but
>>> the pm_runtime core should correctly ignore drivers without
>>> any pm_rumtime callback ops.
>>
>> The pm_runtime core doesn't ignore non-existing runtime_{suspend,resume}
>> callbacks, it turns them into a failure withv -ENOSYS.
>> Only non-existing runtime_idle callbacks are ignored.
>
> I've added Rafael Wysocki as he may be able to add better
> feedback to this issue.
>
> [snip rpm_susend code block]
>
> I got very confused here. The clock_ops sets dev->pm_domain
> which over-rides the use of the dev->driver->pm entry as the
> primary pm for the device. The code above the bit you snipped
> does a ladder looking for the pm_runtime entry it calls and
> would stop at finding dev->pm_domain as so:
>
> from drivers/base/power/runtime.c:
>     495         if (dev->pm_domain)
>     496                 callback = dev->pm_domain->ops.runtime_suspend;
>  ...
>     502                 callback = dev->bus->pm->runtime_suspend;
>     503         else
>     504                 callback = NULL;
>
>
> So for drivers on shmobile with drivers/sh/pm_runtime.c enabled
> we would never call the drivers' entry as the ops that this code
> introduces just calls the pm_clk calls and does not send the
> events on.

Yes, that's also my understanding.

Commit 4d27e9dcff00a6425d779b065ec8892e4f391661 ("PM: Make
power domain callbacks take precedence over subsystem ones") explains
the rationale behind this.

Now, this doesn't prevent your power domain from delegating to other
callbacks...

> If we send the events on, then we would use pm_generic_runtime_suspend()
> to send it. This call treats the lack of runtime_pm driver entry as a
> do nothing and return 0 which means in this case the code in sh_eth
> is not necessary to have any pm_runtime functions.

If the power domain just calls pm_generic_runtime_suspend(), it will only
consider the driver-specific callback, bypassing type, class, and
bus-specific callbacks.

So should the power domain delegate it further using a similar ladder
strategy like RPM_GET_CALLBACK() at the core pm level, i.e.
try type/class/bus/driver?
And type should delegate to class/bus/driver, class to bus/driver, bus to
driver?

> This means depending on if we have a pm_domain in the path we get
> different treatment of NULL runtime pm ops pointer. I am not sure
> how to handle this, as IIRC a number of other drivers for Renesas
> currently assume that the NULL case is going to be fine for them.
>
> Changing pm_generic_runtime_suspend() to return ENOSYS would end
> up breaking davinci and probably a number of other platforms.
>
> So questions:
>
> - Should rpm_suspend() ignore the lack of pm_runtime operations?
> - Do we need to add a generic `ignore pm runtime callback`
> - Are any other shmobile drivers similarly affected?

The code indeed looks a bit like a mix of:
  - Lack of callback means it's safe to suspend,
  - Lack of callback means it's not safe to suspend.

BTW, I also looked a bit into history, and added asterisks in front of the
most interesting parts:

 1. drivers/sh/pm_runtime.c
    Initial version by Magnus, reworked by Rafael

 2. This approach was applied to omap1, "to allow code sharing with shmobile",
    in commit 600b776eb39a13a28b090ba9efceb0c69d4508aa
    ("OMAP1 / PM: Use generic clock manipulation routines for runtime PM")
    by Rafael, acked-by Kevin Hilman.
*   This one is interesting, as it moves omap1 away from the
*   fck/ick-centric approach Felipe proposed to generalize in
*   https://lkml.org/lkml/2014/1/31/290

 3. Kevin converted omap2 in commit
    commit 638080c37ae08fd0c44cec13d7948ca5385ae851 ("OMAP2+ / PM: move
    runtime PM implementation to use device power domains")
*   and fixed it for power domain callbacks taking precedence in commit
*   345f79b3de7f6d651e4dba794af7c7303bdfd649 ("OMAP: PM: omap_device: fix
*   device power domain callbacks").
*   This is what Ben is proposing to add to drivers/sh/pm_runtime.c.

 4. Kevin converted davinci in commit
    ce9dcb8784611c50974d1c6b600c71f5c0a29308 ("ARM: davinci: add runtime PM
    support for clock management").

 5. which got copied by Santosh Shilimkar for keystone in commit
    fc20ffe1213beb09bb7fb6687b404fe48183a55e ("ARM: keystone: add PM domain
    support for clock management").

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH] sh_eth: pm_runtime should not need null operations

[Ben Dooks]

On 21/03/14 14:24, Geert Uytterhoeven wrote:
> Hi Ben,
>
> (dropping netdev and davem, adding Felipe, Kevin, linux-pm, and
> linux-arm-kernel)
>
> On Fri, Mar 21, 2014 at 11:57 AM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
>> On 21/03/14 11:30, Geert Uytterhoeven wrote:
>>> On Fri, Mar 21, 2014 at 11:15 AM, Ben Dooks <ben.dooks@codethink.co.uk>
>>> wrote:
>>>> The driver has a no-op for the pm_runtime callbacks but
>>>> the pm_runtime core should correctly ignore drivers without
>>>> any pm_rumtime callback ops.
>>>
>>> The pm_runtime core doesn't ignore non-existing runtime_{suspend,resume}
>>> callbacks, it turns them into a failure withv -ENOSYS.
>>> Only non-existing runtime_idle callbacks are ignored.
>>
>> I've added Rafael Wysocki as he may be able to add better
>> feedback to this issue.
>>
>> [snip rpm_susend code block]
>>
>> I got very confused here. The clock_ops sets dev->pm_domain
>> which over-rides the use of the dev->driver->pm entry as the
>> primary pm for the device. The code above the bit you snipped
>> does a ladder looking for the pm_runtime entry it calls and
>> would stop at finding dev->pm_domain as so:
>>
>> from drivers/base/power/runtime.c:
>>      495         if (dev->pm_domain)
>>      496                 callback = dev->pm_domain->ops.runtime_suspend;
>>   ...
>>      502                 callback = dev->bus->pm->runtime_suspend;
>>      503         else
>>      504                 callback = NULL;
>>
>>
>> So for drivers on shmobile with drivers/sh/pm_runtime.c enabled
>> we would never call the drivers' entry as the ops that this code
>> introduces just calls the pm_clk calls and does not send the
>> events on.
>
> Yes, that's also my understanding.
>
> Commit 4d27e9dcff00a6425d779b065ec8892e4f391661 ("PM: Make
> power domain callbacks take precedence over subsystem ones") explains
> the rationale behind this.
>
> Now, this doesn't prevent your power domain from delegating to other
> callbacks...
>
>> If we send the events on, then we would use pm_generic_runtime_suspend()
>> to send it. This call treats the lack of runtime_pm driver entry as a
>> do nothing and return 0 which means in this case the code in sh_eth
>> is not necessary to have any pm_runtime functions.
>
> If the power domain just calls pm_generic_runtime_suspend(), it will only
> consider the driver-specific callback, bypassing type, class, and
> bus-specific callbacks.
>
> So should the power domain delegate it further using a similar ladder
> strategy like RPM_GET_CALLBACK() at the core pm level, i.e.
> try type/class/bus/driver?
> And type should delegate to class/bus/driver, class to bus/driver, bus to
> driver?
>
>> This means depending on if we have a pm_domain in the path we get
>> different treatment of NULL runtime pm ops pointer. I am not sure
>> how to handle this, as IIRC a number of other drivers for Renesas
>> currently assume that the NULL case is going to be fine for them.
>>
>> Changing pm_generic_runtime_suspend() to return ENOSYS would end
>> up breaking davinci and probably a number of other platforms.
>>
>> So questions:
>>
>> - Should rpm_suspend() ignore the lack of pm_runtime operations?
>> - Do we need to add a generic `ignore pm runtime callback`
>> - Are any other shmobile drivers similarly affected?
>
> The code indeed looks a bit like a mix of:
>    - Lack of callback means it's safe to suspend,
>    - Lack of callback means it's not safe to suspend.

I thought historically NULL tended to mean it did not care about
this.

The whole thing is giving me a headache as I would expect the
suspend to start with device and then work down the layers and
resume to do the opposite. However currently rpm_resume will also
start at the dev->pm_domain.

Should the rpm_resume start with the dev->bus->pm and then work
its way up to the dev->driver->pm callback? If so then the current
davinci code is also going to be wrong...

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

[PATCH] sh_eth: pm_runtime should not need null operations

[Geert Uytterhoeven]

i Ben,

On Fri, Mar 21, 2014 at 2:46 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> On 21/03/14 14:24, Geert Uytterhoeven wrote:
>> On Fri, Mar 21, 2014 at 11:57 AM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
>>> On 21/03/14 11:30, Geert Uytterhoeven wrote:
>>>> On Fri, Mar 21, 2014 at 11:15 AM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
>>>>> The driver has a no-op for the pm_runtime callbacks but
>>>>> the pm_runtime core should correctly ignore drivers without
>>>>> any pm_rumtime callback ops.
>>>>
>>>> The pm_runtime core doesn't ignore non-existing runtime_{suspend,resume}
>>>> callbacks, it turns them into a failure withv -ENOSYS.
>>>> Only non-existing runtime_idle callbacks are ignored.
>>>
>>> I've added Rafael Wysocki as he may be able to add better
>>> feedback to this issue.
>>>
>>> [snip rpm_susend code block]
>>>
>>> I got very confused here. The clock_ops sets dev->pm_domain
>>> which over-rides the use of the dev->driver->pm entry as the
>>> primary pm for the device. The code above the bit you snipped
>>> does a ladder looking for the pm_runtime entry it calls and
>>> would stop at finding dev->pm_domain as so:
>>>
>>> from drivers/base/power/runtime.c:
>>>      495         if (dev->pm_domain)
>>>      496                 callback = dev->pm_domain->ops.runtime_suspend;
>>>   ...
>>>      502                 callback = dev->bus->pm->runtime_suspend;
>>>      503         else
>>>      504                 callback = NULL;
>>>
>>>
>>> So for drivers on shmobile with drivers/sh/pm_runtime.c enabled
>>> we would never call the drivers' entry as the ops that this code
>>> introduces just calls the pm_clk calls and does not send the
>>> events on.
>>
>> Yes, that's also my understanding.
>>
>> Commit 4d27e9dcff00a6425d779b065ec8892e4f391661 ("PM: Make
>> power domain callbacks take precedence over subsystem ones") explains
>> the rationale behind this.
>>
>> Now, this doesn't prevent your power domain from delegating to other
>> callbacks...
>>
>>> If we send the events on, then we would use pm_generic_runtime_suspend()
>>> to send it. This call treats the lack of runtime_pm driver entry as a
>>> do nothing and return 0 which means in this case the code in sh_eth
>>> is not necessary to have any pm_runtime functions.
>>
>> If the power domain just calls pm_generic_runtime_suspend(), it will only
>> consider the driver-specific callback, bypassing type, class, and
>> bus-specific callbacks.
>>
>> So should the power domain delegate it further using a similar ladder
>> strategy like RPM_GET_CALLBACK() at the core pm level, i.e.
>> try type/class/bus/driver?
>> And type should delegate to class/bus/driver, class to bus/driver, bus to
>> driver?
>>
>>> This means depending on if we have a pm_domain in the path we get
>>> different treatment of NULL runtime pm ops pointer. I am not sure
>>> how to handle this, as IIRC a number of other drivers for Renesas
>>> currently assume that the NULL case is going to be fine for them.
>>>
>>> Changing pm_generic_runtime_suspend() to return ENOSYS would end
>>> up breaking davinci and probably a number of other platforms.
>>>
>>> So questions:
>>>
>>> - Should rpm_suspend() ignore the lack of pm_runtime operations?
>>> - Do we need to add a generic `ignore pm runtime callback`
>>> - Are any other shmobile drivers similarly affected?

> The whole thing is giving me a headache as I would expect the
> suspend to start with device and then work down the layers and
> resume to do the opposite. However currently rpm_resume will also
> start at the dev->pm_domain.
>
> Should the rpm_resume start with the dev->bus->pm and then work
> its way up to the dev->driver->pm callback? If so then the current
> davinci code is also going to be wrong...

I think you got confused by mixing two things: (1) suspend/resume order of
devices, and (2) priority of control.

 1. Obviously a device must be suspended before its bus (its parent),
    and resumed after its bus.
 2. rpm_suspend() et al use RPM_GET_CALLBACK() to get the callback
    that's in control of power management for the device. This does not
    imply order of suspend or resume. That's left to delegation in the
callbacks.

Note that all of the above are for a single device (they take a device *).
Suspending all devices in the system (in the right order) on system suspend
is done elsewhere.

All of this is complicated because you have several different and overlapping
topologies:
  - devices connected with buses, forming a physical data flow hierarchy,
  - device classes and types, forming logical topologies,
  - power domains, grouping one or more devices and buses, not necessarily
    matching the physical data flow topology.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds