arm: imx: suspend/resume: use outer_disable/resume

Commit Message

Dong Aisheng Dec. 26, 2017, 10:03 a.m. UTC

On Tue, Dec 26, 2017 at 09:49:01AM +0000, Peng Fan wrote:
> Hi Shawn
> 
> > -----Original Message-----
> > From: Shawn Guo [mailto:[e-mail]]
> > Sent: Tuesday, December 26, 2017 5:13 PM
> > To: Peng Fan <[e-mail]>
> > Cc: A.s. Dong <[e-mail]>; [e-mail]; Russell
> > King <[e-mail]>; Fabio Estevam <[e-mail]>;
> > Sascha Hauer <[e-mail]>; [e-mail]; linux-arm-
> > [e-mail]
> > Subject: Re: [PATCH] arm: imx: suspend/resume: use outer_disable/resume
> > 
> > On Sun, Dec 10, 2017 at 08:07:18PM +0800, Peng Fan wrote:
> > > Use outer_disable/resume for suspend/resume.
> > > With the two APIs used, code could be simplified and easy to extend to
> > > introduce l2c_write_sec for i.MX platforms when moving Linux Kernel
> > > runs in non-secure world.
> > >
> > > Signed-off-by: Peng Fan <[e-mail]>
> > > Cc: Shawn Guo <[e-mail]>
> > > Cc: Sascha Hauer <[e-mail]>
> > > Cc: Fabio Estevam <[e-mail]>
> > > Cc: Russell King <[e-mail]>
> > > Cc: Dong Aisheng <[e-mail]>
> > 
> > Changed 'arm: ' prefix to 'ARM: ', and applied patch.
> 
> I just tested it on 6sx-sdb, seems this patch breaks 6sx. Could you first drop this
> patch? I'll send out v2 fix the 6sx issue soon.
> 

Yes, i tested mx6ul/mx6sl ok but mx6sx sdb failed.

After a few debug, it seems you removed the l2c310_early_resume in v7_cpu_resume
which is shared between lower power idle and suspend.

Current only mx6sx/mx6ul supports low power idle in upstream, but mx6ul is A7 with
internal L2, that's probably why only mx6sx showed the issue.

I did the following quick try to restore L2 after exit lower power idle, but still
can meet occasional crash during booting.


As this changed the order to L2 restore and cpu resume, so i'm not quite
sure if lower power idle still requres L2 restore before CPU resume or
something else we're missing.

Regards
Dong Aisheng

> Thanks,
> Peng.
> 
> > 
> > Shawn

Comments

Peng Fan Dec. 26, 2017, 10:11 a.m. UTC | #1

Hi Aisheng,

> -----Original Message-----
> From: Dong Aisheng [mailto:[e-mail]]
> Sent: Tuesday, December 26, 2017 6:04 PM
> To: Peng Fan <[e-mail]>
> Cc: Shawn Guo <[e-mail]>; A.s. Dong <[e-mail]>;
> [e-mail]; Russell King <[e-mail]>; Fabio
> Estevam <[e-mail]>; Sascha Hauer <[e-mail]>;
> [e-mail]; [e-mail]
> Subject: Re: [PATCH] arm: imx: suspend/resume: use outer_disable/resume
> 
> On Tue, Dec 26, 2017 at 09:49:01AM +0000, Peng Fan wrote:
> > Hi Shawn
> >
> > > -----Original Message-----
> > > From: Shawn Guo [mailto:[e-mail]]
> > > Sent: Tuesday, December 26, 2017 5:13 PM
> > > To: Peng Fan <[e-mail]>
> > > Cc: A.s. Dong <[e-mail]>; [e-mail];
> > > Russell King <[e-mail]>; Fabio Estevam
> > > <[e-mail]>; Sascha Hauer <[e-mail]>;
> > > [e-mail]; linux-arm- [e-mail]
> > > Subject: Re: [PATCH] arm: imx: suspend/resume: use
> > > outer_disable/resume
> > >
> > > On Sun, Dec 10, 2017 at 08:07:18PM +0800, Peng Fan wrote:
> > > > Use outer_disable/resume for suspend/resume.
> > > > With the two APIs used, code could be simplified and easy to
> > > > extend to introduce l2c_write_sec for i.MX platforms when moving
> > > > Linux Kernel runs in non-secure world.
> > > >
> > > > Signed-off-by: Peng Fan <[e-mail]>
> > > > Cc: Shawn Guo <[e-mail]>
> > > > Cc: Sascha Hauer <[e-mail]>
> > > > Cc: Fabio Estevam <[e-mail]>
> > > > Cc: Russell King <[e-mail]>
> > > > Cc: Dong Aisheng <[e-mail]>
> > >
> > > Changed 'arm: ' prefix to 'ARM: ', and applied patch.
> >
> > I just tested it on 6sx-sdb, seems this patch breaks 6sx. Could you
> > first drop this patch? I'll send out v2 fix the 6sx issue soon.
> >
> 
> Yes, i tested mx6ul/mx6sl ok but mx6sx sdb failed.

Thanks for the test.

> 
> After a few debug, it seems you removed the l2c310_early_resume in
> v7_cpu_resume which is shared between lower power idle and suspend.
> 
> Current only mx6sx/mx6ul supports low power idle in upstream, but mx6ul is
> A7 with internal L2, that's probably why only mx6sx showed the issue.
> 
> I did the following quick try to restore L2 after exit lower power idle, but still
> can meet occasional crash during booting.
> 
> diff --git a/arch/arm/mach-imx/cpuidle-imx6sx.c b/arch/arm/mach-
> imx/cpuidle-imx6sx.c
> index c5a5c3a..edce5bd 100644
> --- a/arch/arm/mach-imx/cpuidle-imx6sx.c
> +++ b/arch/arm/mach-imx/cpuidle-imx6sx.c
> @@ -26,7 +26,7 @@ static int imx6sx_idle_finish(unsigned long val)
>          * to adding conditional code for L2 cache type,
>          * just call flush_cache_all() is fine.
>          */
> -       flush_cache_all();
> +//     flush_cache_all();

I think flush_cache_all is still needed, to flush L1 data, right?

>         cpu_do_idle();
> 
>         return 0;
> @@ -49,7 +49,9 @@ static int imx6sx_enter_wait(struct cpuidle_device *dev,
>                 cpu_pm_enter();
>                 cpu_cluster_pm_enter();
> 
> +               outer_disable();
>                 cpu_suspend(0, imx6sx_idle_finish);
> +               outer_resume();

Yes, this is in my V2 patch.

> 
>                 cpu_cluster_pm_exit();
>                 cpu_pm_exit();
> 
> As this changed the order to L2 restore and cpu resume, so i'm not quite sure if
> lower power idle still requres L2 restore before CPU resume or something else
> we're missing.

In low power idle, L2 may also lose power, so outer_disable/resume is needed, I think.

Thanks,
Peng.

> 
> Regards
> Dong Aisheng
> 
> > Thanks,
> > Peng.
> >
> > >
> > > Shawn

Dong Aisheng Dec. 26, 2017, 10:20 a.m. UTC | #2

On Tue, Dec 26, 2017 at 10:11:41AM +0000, Peng Fan wrote:
> 
> Hi Aisheng,
> 
> > -----Original Message-----
> > From: Dong Aisheng [mailto:[e-mail]]
> > Sent: Tuesday, December 26, 2017 6:04 PM
> > To: Peng Fan <[e-mail]>
> > Cc: Shawn Guo <[e-mail]>; A.s. Dong <[e-mail]>;
> > [e-mail]; Russell King <[e-mail]>; Fabio
> > Estevam <[e-mail]>; Sascha Hauer <[e-mail]>;
> > [e-mail]; [e-mail]
> > Subject: Re: [PATCH] arm: imx: suspend/resume: use outer_disable/resume
> > 
> > On Tue, Dec 26, 2017 at 09:49:01AM +0000, Peng Fan wrote:
> > > Hi Shawn
> > >
> > > > -----Original Message-----
> > > > From: Shawn Guo [mailto:[e-mail]]
> > > > Sent: Tuesday, December 26, 2017 5:13 PM
> > > > To: Peng Fan <[e-mail]>
> > > > Cc: A.s. Dong <[e-mail]>; [e-mail];
> > > > Russell King <[e-mail]>; Fabio Estevam
> > > > <[e-mail]>; Sascha Hauer <[e-mail]>;
> > > > [e-mail]; linux-arm- [e-mail]
> > > > Subject: Re: [PATCH] arm: imx: suspend/resume: use
> > > > outer_disable/resume
> > > >
> > > > On Sun, Dec 10, 2017 at 08:07:18PM +0800, Peng Fan wrote:
> > > > > Use outer_disable/resume for suspend/resume.
> > > > > With the two APIs used, code could be simplified and easy to
> > > > > extend to introduce l2c_write_sec for i.MX platforms when moving
> > > > > Linux Kernel runs in non-secure world.
> > > > >
> > > > > Signed-off-by: Peng Fan <[e-mail]>
> > > > > Cc: Shawn Guo <[e-mail]>
> > > > > Cc: Sascha Hauer <[e-mail]>
> > > > > Cc: Fabio Estevam <[e-mail]>
> > > > > Cc: Russell King <[e-mail]>
> > > > > Cc: Dong Aisheng <[e-mail]>
> > > >
> > > > Changed 'arm: ' prefix to 'ARM: ', and applied patch.
> > >
> > > I just tested it on 6sx-sdb, seems this patch breaks 6sx. Could you
> > > first drop this patch? I'll send out v2 fix the 6sx issue soon.
> > >
> > 
> > Yes, i tested mx6ul/mx6sl ok but mx6sx sdb failed.
> 
> Thanks for the test.
> 
> > 
> > After a few debug, it seems you removed the l2c310_early_resume in
> > v7_cpu_resume which is shared between lower power idle and suspend.
> > 
> > Current only mx6sx/mx6ul supports low power idle in upstream, but mx6ul is
> > A7 with internal L2, that's probably why only mx6sx showed the issue.
> > 
> > I did the following quick try to restore L2 after exit lower power idle, but still
> > can meet occasional crash during booting.
> > 
> > diff --git a/arch/arm/mach-imx/cpuidle-imx6sx.c b/arch/arm/mach-
> > imx/cpuidle-imx6sx.c
> > index c5a5c3a..edce5bd 100644
> > --- a/arch/arm/mach-imx/cpuidle-imx6sx.c
> > +++ b/arch/arm/mach-imx/cpuidle-imx6sx.c
> > @@ -26,7 +26,7 @@ static int imx6sx_idle_finish(unsigned long val)
> >          * to adding conditional code for L2 cache type,
> >          * just call flush_cache_all() is fine.
> >          */
> > -       flush_cache_all();
> > +//     flush_cache_all();
> 
> I think flush_cache_all is still needed, to flush L1 data, right?
> 

I thought it will be done in generic cpu_suspend.
See: __cpu_suspend_save()

So we still need it?


Regards
Dong Aisheng
> >         cpu_do_idle();
> > 
> >         return 0;
> > @@ -49,7 +49,9 @@ static int imx6sx_enter_wait(struct cpuidle_device *dev,
> >                 cpu_pm_enter();
> >                 cpu_cluster_pm_enter();
> > 
> > +               outer_disable();
> >                 cpu_suspend(0, imx6sx_idle_finish);
> > +               outer_resume();
> 
> Yes, this is in my V2 patch.
> 
> > 
> >                 cpu_cluster_pm_exit();
> >                 cpu_pm_exit();
> > 
> > As this changed the order to L2 restore and cpu resume, so i'm not quite sure if
> > lower power idle still requres L2 restore before CPU resume or something else
> > we're missing.
> 
> In low power idle, L2 may also lose power, so outer_disable/resume is needed, I think.
> 
> Thanks,
> Peng.
> 
> > 
> > Regards
> > Dong Aisheng
> > 
> > > Thanks,
> > > Peng.
> > >
> > > >
> > > > Shawn

Dong Aisheng Dec. 26, 2017, 10:36 a.m. UTC | #3

On Tue, Dec 26, 2017 at 06:20:38PM +0800, Dong Aisheng wrote:
> On Tue, Dec 26, 2017 at 10:11:41AM +0000, Peng Fan wrote:
> > 
> > Hi Aisheng,
> > 
> > > -----Original Message-----
> > > From: Dong Aisheng [mailto:[e-mail]]
> > > Sent: Tuesday, December 26, 2017 6:04 PM
> > > To: Peng Fan <[e-mail]>
> > > Cc: Shawn Guo <[e-mail]>; A.s. Dong <[e-mail]>;
> > > [e-mail]; Russell King <[e-mail]>; Fabio
> > > Estevam <[e-mail]>; Sascha Hauer <[e-mail]>;
> > > [e-mail]; [e-mail]
> > > Subject: Re: [PATCH] arm: imx: suspend/resume: use outer_disable/resume
> > > 
> > > On Tue, Dec 26, 2017 at 09:49:01AM +0000, Peng Fan wrote:
> > > > Hi Shawn
> > > >
> > > > > -----Original Message-----
> > > > > From: Shawn Guo [mailto:[e-mail]]
> > > > > Sent: Tuesday, December 26, 2017 5:13 PM
> > > > > To: Peng Fan <[e-mail]>
> > > > > Cc: A.s. Dong <[e-mail]>; [e-mail];
> > > > > Russell King <[e-mail]>; Fabio Estevam
> > > > > <[e-mail]>; Sascha Hauer <[e-mail]>;
> > > > > [e-mail]; linux-arm- [e-mail]
> > > > > Subject: Re: [PATCH] arm: imx: suspend/resume: use
> > > > > outer_disable/resume
> > > > >
> > > > > On Sun, Dec 10, 2017 at 08:07:18PM +0800, Peng Fan wrote:
> > > > > > Use outer_disable/resume for suspend/resume.
> > > > > > With the two APIs used, code could be simplified and easy to
> > > > > > extend to introduce l2c_write_sec for i.MX platforms when moving
> > > > > > Linux Kernel runs in non-secure world.
> > > > > >
> > > > > > Signed-off-by: Peng Fan <[e-mail]>
> > > > > > Cc: Shawn Guo <[e-mail]>
> > > > > > Cc: Sascha Hauer <[e-mail]>
> > > > > > Cc: Fabio Estevam <[e-mail]>
> > > > > > Cc: Russell King <[e-mail]>
> > > > > > Cc: Dong Aisheng <[e-mail]>
> > > > >
> > > > > Changed 'arm: ' prefix to 'ARM: ', and applied patch.
> > > >
> > > > I just tested it on 6sx-sdb, seems this patch breaks 6sx. Could you
> > > > first drop this patch? I'll send out v2 fix the 6sx issue soon.
> > > >
> > > 
> > > Yes, i tested mx6ul/mx6sl ok but mx6sx sdb failed.
> > 
> > Thanks for the test.
> > 
> > > 
> > > After a few debug, it seems you removed the l2c310_early_resume in
> > > v7_cpu_resume which is shared between lower power idle and suspend.
> > > 
> > > Current only mx6sx/mx6ul supports low power idle in upstream, but mx6ul is
> > > A7 with internal L2, that's probably why only mx6sx showed the issue.
> > > 
> > > I did the following quick try to restore L2 after exit lower power idle, but still
> > > can meet occasional crash during booting.
> > > 
> > > diff --git a/arch/arm/mach-imx/cpuidle-imx6sx.c b/arch/arm/mach-
> > > imx/cpuidle-imx6sx.c
> > > index c5a5c3a..edce5bd 100644
> > > --- a/arch/arm/mach-imx/cpuidle-imx6sx.c
> > > +++ b/arch/arm/mach-imx/cpuidle-imx6sx.c
> > > @@ -26,7 +26,7 @@ static int imx6sx_idle_finish(unsigned long val)
> > >          * to adding conditional code for L2 cache type,
> > >          * just call flush_cache_all() is fine.
> > >          */
> > > -       flush_cache_all();
> > > +//     flush_cache_all();
> > 
> > I think flush_cache_all is still needed, to flush L1 data, right?
> > 
> 
> I thought it will be done in generic cpu_suspend.
> See: __cpu_suspend_save()
> 
> So we still need it?
> 

Shawn,

Do you have comments about it?

Regards
Dong Aisheng

Peng Fan Dec. 27, 2017, 12:54 a.m. UTC | #4

> -----Original Message-----
> From: Dong Aisheng [mailto:[e-mail]]
> Sent: Tuesday, December 26, 2017 6:21 PM
> To: Peng Fan <[e-mail]>
> Cc: Shawn Guo <[e-mail]>; A.s. Dong <[e-mail]>;
> [e-mail]; Russell King <[e-mail]>; Fabio
> Estevam <[e-mail]>; Sascha Hauer <[e-mail]>;
> [e-mail]; [e-mail]
> Subject: Re: [PATCH] arm: imx: suspend/resume: use outer_disable/resume
> 
> On Tue, Dec 26, 2017 at 10:11:41AM +0000, Peng Fan wrote:
> >
> > Hi Aisheng,
> >
> > > -----Original Message-----
> > > From: Dong Aisheng [mailto:[e-mail]]
> > > Sent: Tuesday, December 26, 2017 6:04 PM
> > > To: Peng Fan <[e-mail]>
> > > Cc: Shawn Guo <[e-mail]>; A.s. Dong
> > > <[e-mail]>; [e-mail]; Russell King
> > > <[e-mail]>; Fabio Estevam <[e-mail]>;
> > > Sascha Hauer <[e-mail]>; [e-mail];
> > > [e-mail]
> > > Subject: Re: [PATCH] arm: imx: suspend/resume: use
> > > outer_disable/resume
> > >
> > > On Tue, Dec 26, 2017 at 09:49:01AM +0000, Peng Fan wrote:
> > > > Hi Shawn
> > > >
> > > > > -----Original Message-----
> > > > > From: Shawn Guo [mailto:[e-mail]]
> > > > > Sent: Tuesday, December 26, 2017 5:13 PM
> > > > > To: Peng Fan <[e-mail]>
> > > > > Cc: A.s. Dong <[e-mail]>;
> > > > > [e-mail]; Russell King
> > > > > <[e-mail]>; Fabio Estevam <[e-mail]>;
> > > > > Sascha Hauer <[e-mail]>; [e-mail];
> > > > > linux-arm- [e-mail]
> > > > > Subject: Re: [PATCH] arm: imx: suspend/resume: use
> > > > > outer_disable/resume
> > > > >
> > > > > On Sun, Dec 10, 2017 at 08:07:18PM +0800, Peng Fan wrote:
> > > > > > Use outer_disable/resume for suspend/resume.
> > > > > > With the two APIs used, code could be simplified and easy to
> > > > > > extend to introduce l2c_write_sec for i.MX platforms when
> > > > > > moving Linux Kernel runs in non-secure world.
> > > > > >
> > > > > > Signed-off-by: Peng Fan <[e-mail]>
> > > > > > Cc: Shawn Guo <[e-mail]>
> > > > > > Cc: Sascha Hauer <[e-mail]>
> > > > > > Cc: Fabio Estevam <[e-mail]>
> > > > > > Cc: Russell King <[e-mail]>
> > > > > > Cc: Dong Aisheng <[e-mail]>
> > > > >
> > > > > Changed 'arm: ' prefix to 'ARM: ', and applied patch.
> > > >
> > > > I just tested it on 6sx-sdb, seems this patch breaks 6sx. Could
> > > > you first drop this patch? I'll send out v2 fix the 6sx issue soon.
> > > >
> > >
> > > Yes, i tested mx6ul/mx6sl ok but mx6sx sdb failed.
> >
> > Thanks for the test.
> >
> > >
> > > After a few debug, it seems you removed the l2c310_early_resume in
> > > v7_cpu_resume which is shared between lower power idle and suspend.
> > >
> > > Current only mx6sx/mx6ul supports low power idle in upstream, but
> > > mx6ul is
> > > A7 with internal L2, that's probably why only mx6sx showed the issue.
> > >
> > > I did the following quick try to restore L2 after exit lower power
> > > idle, but still can meet occasional crash during booting.
> > >
> > > diff --git a/arch/arm/mach-imx/cpuidle-imx6sx.c b/arch/arm/mach-
> > > imx/cpuidle-imx6sx.c index c5a5c3a..edce5bd 100644
> > > --- a/arch/arm/mach-imx/cpuidle-imx6sx.c
> > > +++ b/arch/arm/mach-imx/cpuidle-imx6sx.c
> > > @@ -26,7 +26,7 @@ static int imx6sx_idle_finish(unsigned long val)
> > >          * to adding conditional code for L2 cache type,
> > >          * just call flush_cache_all() is fine.
> > >          */
> > > -       flush_cache_all();
> > > +//     flush_cache_all();
> >
> > I think flush_cache_all is still needed, to flush L1 data, right?
> >
> 
> I thought it will be done in generic cpu_suspend.
> See: __cpu_suspend_save()
> 
> So we still need it?

After remove flush_cache_all, seems 6sx-sdb suspend/resume and low power idle both works.
Alought __cpu_suspend_save flush data, my understanding is there is chance to cache data/inst
before "wfi". Then we better keep flush_cache_all here?

Thanks,
Peng.

> 
> 
> Regards
> Dong Aisheng
> > >         cpu_do_idle();
> > >
> > >         return 0;
> > > @@ -49,7 +49,9 @@ static int imx6sx_enter_wait(struct cpuidle_device
> *dev,
> > >                 cpu_pm_enter();
> > >                 cpu_cluster_pm_enter();
> > >
> > > +               outer_disable();
> > >                 cpu_suspend(0, imx6sx_idle_finish);
> > > +               outer_resume();
> >
> > Yes, this is in my V2 patch.
> >
> > >
> > >                 cpu_cluster_pm_exit();
> > >                 cpu_pm_exit();
> > >
> > > As this changed the order to L2 restore and cpu resume, so i'm not
> > > quite sure if lower power idle still requres L2 restore before CPU
> > > resume or something else we're missing.
> >
> > In low power idle, L2 may also lose power, so outer_disable/resume is
> needed, I think.
> >
> > Thanks,
> > Peng.
> >
> > >
> > > Regards
> > > Dong Aisheng
> > >
> > > > Thanks,
> > > > Peng.
> > > >
> > > > >
> > > > > Shawn

Shawn Guo Dec. 27, 2017, 2:07 a.m. UTC | #5

On Tue, Dec 26, 2017 at 06:36:29PM +0800, Dong Aisheng wrote:
> > > > diff --git a/arch/arm/mach-imx/cpuidle-imx6sx.c b/arch/arm/mach-
> > > > imx/cpuidle-imx6sx.c
> > > > index c5a5c3a..edce5bd 100644
> > > > --- a/arch/arm/mach-imx/cpuidle-imx6sx.c
> > > > +++ b/arch/arm/mach-imx/cpuidle-imx6sx.c
> > > > @@ -26,7 +26,7 @@ static int imx6sx_idle_finish(unsigned long val)
> > > >          * to adding conditional code for L2 cache type,
> > > >          * just call flush_cache_all() is fine.
> > > >          */
> > > > -       flush_cache_all();
> > > > +//     flush_cache_all();
> > > 
> > > I think flush_cache_all is still needed, to flush L1 data, right?
> > > 
> > 
> > I thought it will be done in generic cpu_suspend.
> > See: __cpu_suspend_save()
> > 
> > So we still need it?
> > 
> 
> Shawn,
> 
> Do you have comments about it?

It seems that there are comments about this flush_cache_all() call right
above it.  To be safe, I would suggest to keep it as it is.

Shawn

Patch

diff --git a/arch/arm/mach-imx/cpuidle-imx6sx.c b/arch/arm/mach-imx/cpuidle-imx6sx.c
index c5a5c3a..edce5bd 100644
--- a/arch/arm/mach-imx/cpuidle-imx6sx.c
+++ b/arch/arm/mach-imx/cpuidle-imx6sx.c
@@ -26,7 +26,7 @@  static int imx6sx_idle_finish(unsigned long val)
         * to adding conditional code for L2 cache type,
         * just call flush_cache_all() is fine.
         */
-       flush_cache_all();
+//     flush_cache_all();
        cpu_do_idle();
 
        return 0;
@@ -49,7 +49,9 @@  static int imx6sx_enter_wait(struct cpuidle_device *dev,
                cpu_pm_enter();
                cpu_cluster_pm_enter();
 
+               outer_disable();
                cpu_suspend(0, imx6sx_idle_finish);
+               outer_resume();
 
                cpu_cluster_pm_exit();
                cpu_pm_exit();