| Message ID | 1395396913-24354-1-git-send-email-ben.dooks@codethink.co.uk (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
Hi Ben, 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. rpm_suspend(): callback = rpm_get_suspend_cb(dev); retval = rpm_callback(callback, dev); if (retval) goto fail; (rpm_callback() returns -ENOSYS if !callback). pm_runtime_force_suspend(): callback = rpm_get_suspend_cb(dev); if (!callback) { ret = -ENOSYS; goto err; } Same for resume. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 21/03/14 11:30, Geert Uytterhoeven wrote: > Hi Ben, > > 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. 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. 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?
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@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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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...
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@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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Ben Dooks <ben.dooks@codethink.co.uk> Date: Fri, 21 Mar 2014 11:57:23 +0100 > On 21/03/14 11:30, Geert Uytterhoeven wrote: >> Hi Ben, >> >> 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. This discussion has stalled so I'm marking this patch as "deferred" in patchwork. Please resubmit once things are resolved, thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c index b908507..bb93333e 100644 --- a/drivers/net/ethernet/renesas/sh_eth.c +++ b/drivers/net/ethernet/renesas/sh_eth.c @@ -2998,28 +2998,6 @@ static int sh_eth_drv_remove(struct platform_device *pdev) return 0; } -#ifdef CONFIG_PM -static int sh_eth_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; -} - -static const struct dev_pm_ops sh_eth_dev_pm_ops = { - .runtime_suspend = sh_eth_runtime_nop, - .runtime_resume = sh_eth_runtime_nop, -}; -#define SH_ETH_PM_OPS (&sh_eth_dev_pm_ops) -#else -#define SH_ETH_PM_OPS NULL -#endif - static struct platform_device_id sh_eth_id_table[] = { { "sh7619-ether", (kernel_ulong_t)&sh7619_data }, { "sh771x-ether", (kernel_ulong_t)&sh771x_data }, @@ -3043,7 +3021,6 @@ static struct platform_driver sh_eth_driver = { .id_table = sh_eth_id_table, .driver = { .name = CARDNAME, - .pm = SH_ETH_PM_OPS, .of_match_table = of_match_ptr(sh_eth_match_table), }, };
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. Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> --- drivers/net/ethernet/renesas/sh_eth.c | 23 ----------------------- 1 file changed, 23 deletions(-)