| Message ID | CAPDyKFoxSiCrfbWRGpSxpwYuL_5okARjqum25nZVe0KyOg_QYg@mail.gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hi Ulf, On Tue, Oct 25, 2016 at 11:31 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 25 October 2016 at 15:59, Geert Uytterhoeven <geert@linux-m68k.org> wrote: >> On Mon, Oct 17, 2016 at 8:17 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>> When resuming a device in __pm_runtime_set_status(), the prerequisite is >>> that its parent must already be active, else an error code is returned and >>> the device's status remains suspended. >>> >>> When suspending a device there is no similar constraints being validated. >>> Let's change this to make the behaviour consistent, by not allowing to >>> suspend a device with an active child, unless it has been explicitly set to >>> ignore its children. >>> >>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >> >> This is now commit 8b1107b85efd78c in pm/linux-next. >> >> This breaks resume from s2ram on r8a73a4/ape6evm and sh73a0/kzm9g, where the >> smsc911x Ethernet is connected to an external bus, whose clock is controlled >> through Runtime PM and the simple-pm-bus driver. >> >> Reverting this commit fixes the issue. > > Geert, thanks again for testing and reporting. I believe this problem > is directly related to what you reported for another patch [1] as > well. Indeed. At first I thought that patch got applied, but I couldn't find it ;-) > Can you please try out this rather trivial patch to the Ethernet > driver (smsc911x), to see if that solves the problem(s). Thanks, it does! Difference between before and after is: PM: Suspending system (mem) cpg_div6_clock_disable: sdhi1ck cpg_div6_clock_disable: sdhi0ck -PM: suspend of devices complete after 22.932 msecs -PM: late suspend of devices complete after 7.311 msecs +PM: suspend of devices complete after 23.131 msecs +PM: late suspend of devices complete after 7.300 msecs cpg_div6_clock_disable: mmc0 cpg_div6_clock_disable: zb -simple-pm-bus fec10000.bus: runtime PM trying to suspend device but active child rmobile_pd_power_down: a3sp -PM: noirq suspend of devices complete after 26.937 msecs +PM: noirq suspend of devices complete after 18.467 msecs Disabling non-boot CPUs ... -Suspended for 2.579 seconds +Suspended for 2.949 seconds rmobile_pd_power_up: a3sp +cpg_div6_clock_enable: zb cpg_div6_clock_enable: sdhi0ck cpg_div6_clock_enable: sdhi1ck cpg_div6_clock_enable: mmc0 -PM: noirq resume of devices complete after 128.014 msecs -PM: early resume of devices complete after 6.121 msecs -Unhandled fault: imprecise external abort (0x1406) at 0x00000000 The zb clock is enabled again. BTW, shouldn't "runtime PM trying to suspend device but active child" become a WARN()? If this happens, something is really wrong, and the device will be left in half-suspended state (clock disabled). > From: Ulf Hansson <ulf.hansson@linaro.org> > Date: Tue, 25 Oct 2016 23:11:51 +0200 > Subject: [PATCH] net: smsc911x: Synchronize the runtime PM status during > system suspend > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> 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
On 26 October 2016 at 11:47, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Ulf, > > On Tue, Oct 25, 2016 at 11:31 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> On 25 October 2016 at 15:59, Geert Uytterhoeven <geert@linux-m68k.org> wrote: >>> On Mon, Oct 17, 2016 at 8:17 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>>> When resuming a device in __pm_runtime_set_status(), the prerequisite is >>>> that its parent must already be active, else an error code is returned and >>>> the device's status remains suspended. >>>> >>>> When suspending a device there is no similar constraints being validated. >>>> Let's change this to make the behaviour consistent, by not allowing to >>>> suspend a device with an active child, unless it has been explicitly set to >>>> ignore its children. >>>> >>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >>> >>> This is now commit 8b1107b85efd78c in pm/linux-next. >>> >>> This breaks resume from s2ram on r8a73a4/ape6evm and sh73a0/kzm9g, where the >>> smsc911x Ethernet is connected to an external bus, whose clock is controlled >>> through Runtime PM and the simple-pm-bus driver. >>> >>> Reverting this commit fixes the issue. >> >> Geert, thanks again for testing and reporting. I believe this problem >> is directly related to what you reported for another patch [1] as >> well. > > Indeed. At first I thought that patch got applied, but I couldn't find it ;-) > >> Can you please try out this rather trivial patch to the Ethernet >> driver (smsc911x), to see if that solves the problem(s). > > Thanks, it does! Great, thanks for testing! I will re-post the patch with a proper change-log. > > Difference between before and after is: > > PM: Suspending system (mem) > cpg_div6_clock_disable: sdhi1ck > cpg_div6_clock_disable: sdhi0ck > -PM: suspend of devices complete after 22.932 msecs > -PM: late suspend of devices complete after 7.311 msecs > +PM: suspend of devices complete after 23.131 msecs > +PM: late suspend of devices complete after 7.300 msecs > cpg_div6_clock_disable: mmc0 > cpg_div6_clock_disable: zb > -simple-pm-bus fec10000.bus: runtime PM trying to suspend device but > active child > rmobile_pd_power_down: a3sp > -PM: noirq suspend of devices complete after 26.937 msecs > +PM: noirq suspend of devices complete after 18.467 msecs > Disabling non-boot CPUs ... > -Suspended for 2.579 seconds > +Suspended for 2.949 seconds > rmobile_pd_power_up: a3sp > +cpg_div6_clock_enable: zb > cpg_div6_clock_enable: sdhi0ck > cpg_div6_clock_enable: sdhi1ck > cpg_div6_clock_enable: mmc0 > -PM: noirq resume of devices complete after 128.014 msecs > -PM: early resume of devices complete after 6.121 msecs > -Unhandled fault: imprecise external abort (0x1406) at 0x00000000 > > The zb clock is enabled again. > > BTW, shouldn't "runtime PM trying to suspend device but active child" > become a WARN()? If this happens, something is really wrong, and > the device will be left in half-suspended state (clock disabled). That would make sense to me! Although if we decide to change that, we should also convert from dev_err() to WARN() when failing to set RPM_ACTIVE in __pm_runtime_set_status(), as to be consistent. Do you want to send a patch? > >> From: Ulf Hansson <ulf.hansson@linaro.org> >> Date: Tue, 25 Oct 2016 23:11:51 +0200 >> Subject: [PATCH] net: smsc911x: Synchronize the runtime PM status during >> system suspend >> >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Gr{oetje,eeting}s, > > Geert > Kind regards Uffe
diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c index e9b8579..65fca9c 100644 --- a/drivers/net/ethernet/smsc/smsc911x.c +++ b/drivers/net/ethernet/smsc/smsc911x.c @@ -2584,6 +2584,9 @@ static int smsc911x_suspend(struct device *dev) PMT_CTRL_PM_MODE_D1_ | PMT_CTRL_WOL_EN_ | PMT_CTRL_ED_EN_ | PMT_CTRL_PME_EN_); + pm_runtime_disable(dev); + pm_runtime_set_suspended(dev); + return 0; } @@ -2593,6 +2596,9 @@ static int smsc911x_resume(struct device *dev) struct smsc911x_data *pdata = netdev_priv(ndev); unsigned int to = 100; + pm_runtime_enable(dev); + pm_runtime_resume(dev); + /* Note 3.11 from the datasheet: * "When the LAN9220 is in a power saving state, a write of any * data to the BYTE_TEST register will wake-up the device."