| Message ID | 1368807872-2601-4-git-send-email-t.figa@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Tomasz, On Fri, May 17, 2013 at 9:24 AM, Tomasz Figa <t.figa@samsung.com> wrote: > This patch makes legacy code on suspend/resume path being executed > conditionally, on non-DT platforms only, to fix suspend/resume of > DT-enabled systems, for which the code is inappropriate. > > Signed-off-by: Tomasz Figa <t.figa@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > --- > arch/arm/plat-samsung/pm.c | 17 ++++++++++++----- > 1 file changed, 12 insertions(+), 5 deletions(-) > > diff --git a/arch/arm/plat-samsung/pm.c b/arch/arm/plat-samsung/pm.c > index 53210ec..8ac2b2d 100644 > --- a/arch/arm/plat-samsung/pm.c > +++ b/arch/arm/plat-samsung/pm.c > @@ -261,7 +261,8 @@ static int s3c_pm_enter(suspend_state_t state) > * require a full power-cycle) > */ > > - if (!any_allowed(s3c_irqwake_intmask, s3c_irqwake_intallow) && > + if (!of_have_populated_dt() && > + !any_allowed(s3c_irqwake_intmask, s3c_irqwake_intallow) && I'd rather see you modify patch set #2 to provide some function to retrieve just the eint mask and then use it here. Your patch removes this test and doesn't replace it with anything. If you moved this test to pinctrl directly you'd lose the test against intallow. ...or do you think this test is no longer useful for some reason? > !any_allowed(s3c_irqwake_eintmask, s3c_irqwake_eintallow)) { > printk(KERN_ERR "%s: No wake-up sources!\n", __func__); > printk(KERN_ERR "%s: Aborting sleep\n", __func__); > @@ -270,8 +271,11 @@ static int s3c_pm_enter(suspend_state_t state) > > /* save all necessary core registers not covered by the drivers */ > > - samsung_pm_save_gpios(); > - samsung_pm_saved_gpios(); > + if (!of_have_populated_dt()) { > + samsung_pm_save_gpios(); > + samsung_pm_saved_gpios(); > + } > + Ah, the important part here is the "saved", not the "save"! The "save" should get a NULL chip for all GPIOs and effectively be a no-op. Skipping the "saved" is important of s3c64xx and s5p64x0 where the "saved" seems to transition things over to powerdown mode. Hopefully a future patch of yours adds that back for those platforms in the pinmux world. ...same for restore. Summary: I've tested this on exynos5250-snow and it's reasonable but I'd love a response about the missing test before adding Reviewed-by. -Doug
On Friday 17 of May 2013 12:24:04 Doug Anderson wrote: > Tomasz, > > On Fri, May 17, 2013 at 9:24 AM, Tomasz Figa <t.figa@samsung.com> wrote: > > This patch makes legacy code on suspend/resume path being executed > > conditionally, on non-DT platforms only, to fix suspend/resume of > > DT-enabled systems, for which the code is inappropriate. > > > > Signed-off-by: Tomasz Figa <t.figa@samsung.com> > > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > > --- > > > > arch/arm/plat-samsung/pm.c | 17 ++++++++++++----- > > 1 file changed, 12 insertions(+), 5 deletions(-) > > > > diff --git a/arch/arm/plat-samsung/pm.c b/arch/arm/plat-samsung/pm.c > > index 53210ec..8ac2b2d 100644 > > --- a/arch/arm/plat-samsung/pm.c > > +++ b/arch/arm/plat-samsung/pm.c > > @@ -261,7 +261,8 @@ static int s3c_pm_enter(suspend_state_t state) > > > > * require a full power-cycle) > > > > */ > > > > - if (!any_allowed(s3c_irqwake_intmask, s3c_irqwake_intallow) && > > + if (!of_have_populated_dt() && > > + !any_allowed(s3c_irqwake_intmask, s3c_irqwake_intallow) && > > I'd rather see you modify patch set #2 to provide some function to > retrieve just the eint mask and then use it here. Your patch removes > this test and doesn't replace it with anything. If you moved this > test to pinctrl directly you'd lose the test against intallow. Well, looking from the perspective of status before my patch, it just bypasses a test that is incorrect on DT-enabled platforms. I agree that this test is rather reasonable to have, but it would require defining a new interface and moving all platforms to it, which for now would be a bit of overkill. IMHO a separate series that sanitizes the whole PM handling in plat- samsung, including a rework of this check to make it cover all platforms in a generic and multiplatform-friendly way. What do you think? > ...or do you think this test is no longer useful for some reason? > > > !any_allowed(s3c_irqwake_eintmask, s3c_irqwake_eintallow)) > > { > > > > printk(KERN_ERR "%s: No wake-up sources!\n", > > __func__); > > printk(KERN_ERR "%s: Aborting sleep\n", __func__); > > > > @@ -270,8 +271,11 @@ static int s3c_pm_enter(suspend_state_t state) > > > > /* save all necessary core registers not covered by the > > drivers */ > > > > - samsung_pm_save_gpios(); > > - samsung_pm_saved_gpios(); > > + if (!of_have_populated_dt()) { > > + samsung_pm_save_gpios(); > > + samsung_pm_saved_gpios(); > > + } > > + > > Ah, the important part here is the "saved", not the "save"! The > "save" should get a NULL chip for all GPIOs and effectively be a > no-op. > > Skipping the "saved" is important of s3c64xx and s5p64x0 where the > "saved" seems to transition things over to powerdown mode. Hopefully > a future patch of yours adds that back for those platforms in the > pinmux world. ...same for restore. S3C64xx can be switched to power down pin configuration manually, but if you don't do it, it will activate it automatically after entering sleep mode. > Summary: I've tested this on exynos5250-snow and it's reasonable but > I'd love a response about the missing test before adding Reviewed-by. Thanks. Best regards, Tomasz
Tomasz, On Fri, May 17, 2013 at 1:23 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote: >> I'd rather see you modify patch set #2 to provide some function to >> retrieve just the eint mask and then use it here. Your patch removes >> this test and doesn't replace it with anything. If you moved this >> test to pinctrl directly you'd lose the test against intallow. > > Well, looking from the perspective of status before my patch, it just > bypasses a test that is incorrect on DT-enabled platforms. True. What was there before was broken and this avoids the broken code. > I agree that this test is rather reasonable to have, but it would require > defining a new interface and moving all platforms to it, which for now > would be a bit of overkill. It seems like you could use the same type of solution as patch set #2? ...oh, but that's only for exynos! I guess we would need something similar for other platforms. ...and until we do those other platforms (including S3C64xx, I think) are still using the old s3c_irqwake_eintmask, right? ...so overall your patch series only fully fixes exynos, though it does make other platforms less broken which is a plus. :) > IMHO a separate series that sanitizes the whole PM handling in plat- > samsung, including a rework of this check to make it cover all platforms > in a generic and multiplatform-friendly way. What do you think? Sure, I think that would be OK. >> Ah, the important part here is the "saved", not the "save"! The >> "save" should get a NULL chip for all GPIOs and effectively be a >> no-op. >> >> Skipping the "saved" is important of s3c64xx and s5p64x0 where the >> "saved" seems to transition things over to powerdown mode. Hopefully >> a future patch of yours adds that back for those platforms in the >> pinmux world. ...same for restore. > > S3C64xx can be switched to power down pin configuration manually, but if > you don't do it, it will activate it automatically after entering sleep > mode. How would restore work in that case? I'd imagine that it would automatically switch out of the powerdown config at wakeup before running software? That doesn't seem like a great idea. I think that to make S3C64xx work properly we'd need to solve. I wouldn't be opposed to a re-spin to address some of the above, but I also won't object to this landing and remaining issues being addressed in future patches. This patch definitely does make things better and no worse. :) On exynos5250-snow (pinmux backported to 3.8): Tested-by: Doug Anderson <dianders@chromium.org> Reviewed-by: Doug Anderson <dianders@chromium.org>
On Friday 17 of May 2013 13:56:39 Doug Anderson wrote: > Tomasz, > > On Fri, May 17, 2013 at 1:23 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote: > >> I'd rather see you modify patch set #2 to provide some function to > >> retrieve just the eint mask and then use it here. Your patch removes > >> this test and doesn't replace it with anything. If you moved this > >> test to pinctrl directly you'd lose the test against intallow. > > > > Well, looking from the perspective of status before my patch, it just > > bypasses a test that is incorrect on DT-enabled platforms. > > True. What was there before was broken and this avoids the broken code. > > I agree that this test is rather reasonable to have, but it would > > require defining a new interface and moving all platforms to it, > > which for now would be a bit of overkill. > > It seems like you could use the same type of solution as patch set #2? > > ...oh, but that's only for exynos! I guess we would need something > similar for other platforms. ...and until we do those other platforms > (including S3C64xx, I think) are still using the old > s3c_irqwake_eintmask, right? Correct. Also as an extra side note, intallow is used here as a mask of valid eintmask bits on given platform. On Exynos SoCs there are 32 EINTs, so this extra mask is not needed. > ...so overall your patch series only fully fixes exynos, though it > does make other platforms less broken which is a plus. :) > > > IMHO a separate series that sanitizes the whole PM handling in plat- > > samsung, including a rework of this check to make it cover all > > platforms in a generic and multiplatform-friendly way. What do you > > think? > Sure, I think that would be OK. > > >> Ah, the important part here is the "saved", not the "save"! The > >> "save" should get a NULL chip for all GPIOs and effectively be a > >> no-op. > >> > >> Skipping the "saved" is important of s3c64xx and s5p64x0 where the > >> "saved" seems to transition things over to powerdown mode. Hopefully > >> a future patch of yours adds that back for those platforms in the > >> pinmux world. ...same for restore. > > > > S3C64xx can be switched to power down pin configuration manually, but > > if you don't do it, it will activate it automatically after entering > > sleep mode. > > How would restore work in that case? I'd imagine that it would > automatically switch out of the powerdown config at wakeup before > running software? That doesn't seem like a great idea. This is configurable. There is a bit that determines whether it should switch back to normal config automatically or not. What's interesting is that AFAIR current code uses automatic switch, which even with the glitch prevention in resume can glitch things, due to the period of time between wake-up and resume when pins are misconfigured. I think we should just configure this bit for manual switch back and trigger the switch from .resumed() callback of the pin controller in pinctrl-s3c64xx driver. I will post a patch for this some day. > I think that to make S3C64xx work properly we'd need to solve. > > > I wouldn't be opposed to a re-spin to address some of the above, but I > also won't object to this landing and remaining issues being addressed > in future patches. This patch definitely does make things better and > no worse. :) Let's see. If nobody takes this until Monday, which is very likely, I will send new version. > On exynos5250-snow (pinmux backported to 3.8): > > Tested-by: Doug Anderson <dianders@chromium.org> > > Reviewed-by: Doug Anderson <dianders@chromium.org> Thanks. Best regards, Tomasz
On Fri, May 17, 2013 at 11:07 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote: > On Friday 17 of May 2013 13:56:39 Doug Anderson wrote: >> I wouldn't be opposed to a re-spin to address some of the above, but I >> also won't object to this landing and remaining issues being addressed >> in future patches. This patch definitely does make things better and >> no worse. :) > > Let's see. If nobody takes this until Monday, which is very likely, I will > send new version. I applied 1 & 2 so pls repost your updated patches 3,4,5,6 on monday and let's continue fixing. Yours, Linus Walleij
Hi Linus, On Tuesday 21 of May 2013 13:29:23 Linus Walleij wrote: > On Fri, May 17, 2013 at 11:07 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote: > > On Friday 17 of May 2013 13:56:39 Doug Anderson wrote: > >> I wouldn't be opposed to a re-spin to address some of the above, but I > >> also won't object to this landing and remaining issues being addressed > >> in future patches. This patch definitely does make things better and > >> no worse. :) > > > > Let's see. If nobody takes this until Monday, which is very likely, I will > > send new version. > > I applied 1 & 2 so pls repost your updated patches 3,4,5,6 on > monday and let's continue fixing. Thanks. Feel free to apply current version of patches 3, 4 and 5 as well. I will post updated patch 6 in a while. Best regards,
Just posted updated version of patch 6/6. Best regards,
Hi, On Friday 17 of May 2013 18:24:29 Tomasz Figa wrote: > This patch makes legacy code on suspend/resume path being executed > conditionally, on non-DT platforms only, to fix suspend/resume of > DT-enabled systems, for which the code is inappropriate. > > Signed-off-by: Tomasz Figa <t.figa@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > --- > arch/arm/plat-samsung/pm.c | 17 ++++++++++++----- > 1 file changed, 12 insertions(+), 5 deletions(-) It seems like this patch did not make it into 3.10-rc5, while rest of patches did, which ended up with suspend/resume still being broken. What should we do in this case? Best regards, Tomasz > diff --git a/arch/arm/plat-samsung/pm.c b/arch/arm/plat-samsung/pm.c > index 53210ec..8ac2b2d 100644 > --- a/arch/arm/plat-samsung/pm.c > +++ b/arch/arm/plat-samsung/pm.c > @@ -261,7 +261,8 @@ static int s3c_pm_enter(suspend_state_t state) > * require a full power-cycle) > */ > > - if (!any_allowed(s3c_irqwake_intmask, s3c_irqwake_intallow) && > + if (!of_have_populated_dt() && > + !any_allowed(s3c_irqwake_intmask, s3c_irqwake_intallow) && > !any_allowed(s3c_irqwake_eintmask, s3c_irqwake_eintallow)) { > printk(KERN_ERR "%s: No wake-up sources!\n", __func__); > printk(KERN_ERR "%s: Aborting sleep\n", __func__); > @@ -270,8 +271,11 @@ static int s3c_pm_enter(suspend_state_t state) > > /* save all necessary core registers not covered by the drivers */ > > - samsung_pm_save_gpios(); > - samsung_pm_saved_gpios(); > + if (!of_have_populated_dt()) { > + samsung_pm_save_gpios(); > + samsung_pm_saved_gpios(); > + } > + > s3c_pm_save_uarts(); > s3c_pm_save_core(); > > @@ -310,8 +314,11 @@ static int s3c_pm_enter(suspend_state_t state) > > s3c_pm_restore_core(); > s3c_pm_restore_uarts(); > - samsung_pm_restore_gpios(); > - s3c_pm_restored_gpios(); > + > + if (!of_have_populated_dt()) { > + samsung_pm_restore_gpios(); > + s3c_pm_restored_gpios(); > + } > > s3c_pm_debug_init();
On Mon, Jun 10, 2013 at 4:45 PM, Tomasz Figa <t.figa@samsung.com> wrote: > On Friday 17 of May 2013 18:24:29 Tomasz Figa wrote: >> This patch makes legacy code on suspend/resume path being executed >> conditionally, on non-DT platforms only, to fix suspend/resume of >> DT-enabled systems, for which the code is inappropriate. >> >> Signed-off-by: Tomasz Figa <t.figa@samsung.com> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> >> --- >> arch/arm/plat-samsung/pm.c | 17 ++++++++++++----- >> 1 file changed, 12 insertions(+), 5 deletions(-) > > It seems like this patch did not make it into 3.10-rc5, while rest of patches > did, which ended up with suspend/resume still being broken. > > What should we do in this case? I did not apply this patch to the pinctrl tree because it begins with ARM: but it appears I promised to carry it anyway... I need an ACK from the ARM SoC people to do that though, or you can take it through ARM SoC fixes directly. I would just ask Olof/Arnd to apply it directly as a separate patch. Yours, Linus Walleij
On Mon, Jun 10, 2013 at 06:13:10PM +0200, Linus Walleij wrote: > On Mon, Jun 10, 2013 at 4:45 PM, Tomasz Figa <t.figa@samsung.com> wrote: > > On Friday 17 of May 2013 18:24:29 Tomasz Figa wrote: > >> This patch makes legacy code on suspend/resume path being executed > >> conditionally, on non-DT platforms only, to fix suspend/resume of > >> DT-enabled systems, for which the code is inappropriate. > >> > >> Signed-off-by: Tomasz Figa <t.figa@samsung.com> > >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > >> --- > >> arch/arm/plat-samsung/pm.c | 17 ++++++++++++----- > >> 1 file changed, 12 insertions(+), 5 deletions(-) > > > > It seems like this patch did not make it into 3.10-rc5, while rest of patches > > did, which ended up with suspend/resume still being broken. > > > > What should we do in this case? > > I did not apply this patch to the pinctrl tree because it begins > with ARM: but it appears I promised to carry it anyway... > > I need an ACK from the ARM SoC people to do that though, > or you can take it through ARM SoC fixes directly. > > I would just ask Olof/Arnd to apply it directly as a separate > patch. It makes sense to take this through arm-soc since it's no longer dependent on anything else. Applied to fixes. Please do follow through with the promised cleanups at some point, Tomasz. :-) -Olof
On Tue, Jun 11, 2013 at 12:45 AM, Olof Johansson <olof@lixom.net> wrote: > On Mon, Jun 10, 2013 at 06:13:10PM +0200, Linus Walleij wrote: >> On Mon, Jun 10, 2013 at 4:45 PM, Tomasz Figa <t.figa@samsung.com> wrote: >> > On Friday 17 of May 2013 18:24:29 Tomasz Figa wrote: >> >> This patch makes legacy code on suspend/resume path being executed >> >> conditionally, on non-DT platforms only, to fix suspend/resume of >> >> DT-enabled systems, for which the code is inappropriate. >> >> >> >> Signed-off-by: Tomasz Figa <t.figa@samsung.com> >> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> >> >> --- >> >> arch/arm/plat-samsung/pm.c | 17 ++++++++++++----- >> >> 1 file changed, 12 insertions(+), 5 deletions(-) >> > >> > It seems like this patch did not make it into 3.10-rc5, while rest of patches >> > did, which ended up with suspend/resume still being broken. >> > >> > What should we do in this case? >> >> I did not apply this patch to the pinctrl tree because it begins >> with ARM: but it appears I promised to carry it anyway... >> >> I need an ACK from the ARM SoC people to do that though, >> or you can take it through ARM SoC fixes directly. >> >> I would just ask Olof/Arnd to apply it directly as a separate >> patch. > > It makes sense to take this through arm-soc since it's no longer > dependent on anything else. > > Applied to fixes. Please do follow through with the promised cleanups at some > point, Tomasz. :-) Btw, I had to add an include of <linux/of.h> or else all non-DT builds broke... -Olof
Hi Olof, On Tuesday 11 of June 2013 01:21:59 Olof Johansson wrote: > On Tue, Jun 11, 2013 at 12:45 AM, Olof Johansson <olof@lixom.net> wrote: > > On Mon, Jun 10, 2013 at 06:13:10PM +0200, Linus Walleij wrote: > >> On Mon, Jun 10, 2013 at 4:45 PM, Tomasz Figa <t.figa@samsung.com> wrote: > >> > On Friday 17 of May 2013 18:24:29 Tomasz Figa wrote: > >> >> This patch makes legacy code on suspend/resume path being executed > >> >> conditionally, on non-DT platforms only, to fix suspend/resume of > >> >> DT-enabled systems, for which the code is inappropriate. > >> >> > >> >> Signed-off-by: Tomasz Figa <t.figa@samsung.com> > >> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > >> >> --- > >> >> > >> >> arch/arm/plat-samsung/pm.c | 17 ++++++++++++----- > >> >> 1 file changed, 12 insertions(+), 5 deletions(-) > >> > > >> > It seems like this patch did not make it into 3.10-rc5, while rest > >> > of patches did, which ended up with suspend/resume still being > >> > broken. > >> > > >> > What should we do in this case? > >> > >> I did not apply this patch to the pinctrl tree because it begins > >> with ARM: but it appears I promised to carry it anyway... > >> > >> I need an ACK from the ARM SoC people to do that though, > >> or you can take it through ARM SoC fixes directly. > >> > >> I would just ask Olof/Arnd to apply it directly as a separate > >> patch. > > > > It makes sense to take this through arm-soc since it's no longer > > dependent on anything else. > > > > Applied to fixes. Thanks. > > Please do follow through with the promised cleanups > > at some point, Tomasz. :-) Yeah, just managed to reserve some time for it, so stay tuned ;) . > > Btw, I had to add an include of <linux/of.h> or else all non-DT builds > broke... Sorry, I must have missed it. I guess it's the right time to set up some autobuild framework to check all patches with several most interesting configs. I guess some git hooks would be enough... Best regards, Tomasz > > -Olof > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Jun 11, 2013 at 5:15 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote: >> > Please do follow through with the promised cleanups >> > at some point, Tomasz. :-) > > Yeah, just managed to reserve some time for it, so stay tuned ;) . Cool. >> Btw, I had to add an include of <linux/of.h> or else all non-DT builds >> broke... > > Sorry, I must have missed it. > > I guess it's the right time to set up some autobuild framework to check > all patches with several most interesting configs. I guess some git hooks > would be enough... No worries. I tend to build all defconfigs after doing a batch of pulls, and that's when we find these issues. I also boot on the few boards that I have to make sure nothing catastrophic is broken, but there's obviously much less coverage on that side. -Olof
diff --git a/arch/arm/plat-samsung/pm.c b/arch/arm/plat-samsung/pm.c index 53210ec..8ac2b2d 100644 --- a/arch/arm/plat-samsung/pm.c +++ b/arch/arm/plat-samsung/pm.c @@ -261,7 +261,8 @@ static int s3c_pm_enter(suspend_state_t state) * require a full power-cycle) */ - if (!any_allowed(s3c_irqwake_intmask, s3c_irqwake_intallow) && + if (!of_have_populated_dt() && + !any_allowed(s3c_irqwake_intmask, s3c_irqwake_intallow) && !any_allowed(s3c_irqwake_eintmask, s3c_irqwake_eintallow)) { printk(KERN_ERR "%s: No wake-up sources!\n", __func__); printk(KERN_ERR "%s: Aborting sleep\n", __func__); @@ -270,8 +271,11 @@ static int s3c_pm_enter(suspend_state_t state) /* save all necessary core registers not covered by the drivers */ - samsung_pm_save_gpios(); - samsung_pm_saved_gpios(); + if (!of_have_populated_dt()) { + samsung_pm_save_gpios(); + samsung_pm_saved_gpios(); + } + s3c_pm_save_uarts(); s3c_pm_save_core(); @@ -310,8 +314,11 @@ static int s3c_pm_enter(suspend_state_t state) s3c_pm_restore_core(); s3c_pm_restore_uarts(); - samsung_pm_restore_gpios(); - s3c_pm_restored_gpios(); + + if (!of_have_populated_dt()) { + samsung_pm_restore_gpios(); + s3c_pm_restored_gpios(); + } s3c_pm_debug_init();