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 <[email protected]>
Cc: Shawn Guo <[email protected]>
Cc: Sascha Hauer <[email protected]>
Cc: Fabio Estevam <[email protected]>
Cc: Russell King <[email protected]>
Cc: Dong Aisheng <[email protected]>
---
arch/arm/mach-imx/pm-imx6.c | 2 ++
arch/arm/mach-imx/suspend-imx6.S | 24 ------------------------
2 files changed, 2 insertions(+), 24 deletions(-)
diff --git a/arch/arm/mach-imx/pm-imx6.c b/arch/arm/mach-imx/pm-imx6.c
index ecdf071653d4..153a0afc7645 100644
--- a/arch/arm/mach-imx/pm-imx6.c
+++ b/arch/arm/mach-imx/pm-imx6.c
@@ -392,8 +392,10 @@ static int imx6q_pm_enter(suspend_state_t state)
imx6_enable_rbc(true);
imx_gpc_pre_suspend(true);
imx_anatop_pre_suspend();
+ outer_disable();
/* Zzz ... */
cpu_suspend(0, imx6q_suspend_finish);
+ outer_resume();
if (cpu_is_imx6q() || cpu_is_imx6dl())
imx_smp_prepare();
imx_anatop_post_resume();
diff --git a/arch/arm/mach-imx/suspend-imx6.S b/arch/arm/mach-imx/suspend-imx6.S
index 76ee2ceec8d5..324f6b165e82 100644
--- a/arch/arm/mach-imx/suspend-imx6.S
+++ b/arch/arm/mach-imx/suspend-imx6.S
@@ -74,24 +74,6 @@
.align 3
- .macro sync_l2_cache
-
- /* sync L2 cache to drain L2's buffers to DRAM. */
-#ifdef CONFIG_CACHE_L2X0
- ldr r11, [r0, #PM_INFO_MX6Q_L2_V_OFFSET]
- teq r11, #0
- beq 6f
- mov r6, #0x0
- str r6, [r11, #L2X0_CACHE_SYNC]
-1:
- ldr r6, [r11, #L2X0_CACHE_SYNC]
- ands r6, r6, #0x1
- bne 1b
-6:
-#endif
-
- .endm
-
.macro resume_mmdc
/* restore MMDC IO */
@@ -185,9 +167,6 @@ ENTRY(imx6_suspend)
str r9, [r11, #MX6Q_SRC_GPR1]
str r1, [r11, #MX6Q_SRC_GPR2]
- /* need to sync L2 cache before DSM. */
- sync_l2_cache
-
ldr r11, [r0, #PM_INFO_MX6Q_MMDC_V_OFFSET]
/*
* put DDR explicitly into self-refresh and
@@ -342,8 +321,5 @@ ENDPROC(imx6_suspend)
ENTRY(v7_cpu_resume)
bl v7_invalidate_l1
-#ifdef CONFIG_CACHE_L2X0
- bl l2c310_early_resume
-#endif
b cpu_resume
ENDPROC(v7_cpu_resume)
--
2.14.1
Hi All,
Ping...
> -----Original Message-----
> From: Peng Fan
> Sent: Sunday, December 10, 2017 8:07 PM
> To: [email protected]
> Cc: [email protected]; [email protected];
> [email protected]; Peng Fan <[email protected]>; Sascha Hauer
> <[email protected]>; Fabio Estevam <[email protected]>; Russell
> King <[email protected]>; A.s. Dong <[email protected]>
> Subject: [PATCH] arm: imx: suspend/resume: use outer_disable/resume
>
> 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 <[email protected]>
> Cc: Shawn Guo <[email protected]>
> Cc: Sascha Hauer <[email protected]>
> Cc: Fabio Estevam <[email protected]>
> Cc: Russell King <[email protected]>
> Cc: Dong Aisheng <[email protected]>
> ---
> arch/arm/mach-imx/pm-imx6.c | 2 ++
> arch/arm/mach-imx/suspend-imx6.S | 24 ------------------------
> 2 files changed, 2 insertions(+), 24 deletions(-)
>
> diff --git a/arch/arm/mach-imx/pm-imx6.c b/arch/arm/mach-imx/pm-imx6.c
> index ecdf071653d4..153a0afc7645 100644
> --- a/arch/arm/mach-imx/pm-imx6.c
> +++ b/arch/arm/mach-imx/pm-imx6.c
> @@ -392,8 +392,10 @@ static int imx6q_pm_enter(suspend_state_t state)
> imx6_enable_rbc(true);
> imx_gpc_pre_suspend(true);
> imx_anatop_pre_suspend();
> + outer_disable();
> /* Zzz ... */
> cpu_suspend(0, imx6q_suspend_finish);
> + outer_resume();
> if (cpu_is_imx6q() || cpu_is_imx6dl())
> imx_smp_prepare();
> imx_anatop_post_resume();
> diff --git a/arch/arm/mach-imx/suspend-imx6.S b/arch/arm/mach-
> imx/suspend-imx6.S
> index 76ee2ceec8d5..324f6b165e82 100644
> --- a/arch/arm/mach-imx/suspend-imx6.S
> +++ b/arch/arm/mach-imx/suspend-imx6.S
> @@ -74,24 +74,6 @@
>
> .align 3
>
> - .macro sync_l2_cache
> -
> - /* sync L2 cache to drain L2's buffers to DRAM. */
> -#ifdef CONFIG_CACHE_L2X0
> - ldr r11, [r0, #PM_INFO_MX6Q_L2_V_OFFSET]
> - teq r11, #0
> - beq 6f
> - mov r6, #0x0
> - str r6, [r11, #L2X0_CACHE_SYNC]
> -1:
> - ldr r6, [r11, #L2X0_CACHE_SYNC]
> - ands r6, r6, #0x1
> - bne 1b
> -6:
> -#endif
> -
> - .endm
> -
> .macro resume_mmdc
>
> /* restore MMDC IO */
> @@ -185,9 +167,6 @@ ENTRY(imx6_suspend)
> str r9, [r11, #MX6Q_SRC_GPR1]
> str r1, [r11, #MX6Q_SRC_GPR2]
>
> - /* need to sync L2 cache before DSM. */
> - sync_l2_cache
> -
> ldr r11, [r0, #PM_INFO_MX6Q_MMDC_V_OFFSET]
> /*
> * put DDR explicitly into self-refresh and @@ -342,8 +321,5 @@
> ENDPROC(imx6_suspend)
>
> ENTRY(v7_cpu_resume)
> bl v7_invalidate_l1
> -#ifdef CONFIG_CACHE_L2X0
> - bl l2c310_early_resume
> -#endif
> b cpu_resume
> ENDPROC(v7_cpu_resume)
> --
> 2.14.1
Thanks,
Peng.
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 <[email protected]>
> Cc: Shawn Guo <[email protected]>
> Cc: Sascha Hauer <[email protected]>
> Cc: Fabio Estevam <[email protected]>
> Cc: Russell King <[email protected]>
> Cc: Dong Aisheng <[email protected]>
> ---
> arch/arm/mach-imx/pm-imx6.c | 2 ++
> arch/arm/mach-imx/suspend-imx6.S | 24 ------------------------
I'm fine with the patch in general. But this piece of code is running
on a few i.MX6 platforms, and I'm wondering on which SoCs you have
verified the change work fine.
Shawn
Hi Shawn,
> -----Original Message-----
> From: Shawn Guo [mailto:[email protected]]
> Sent: Tuesday, December 26, 2017 11:31 AM
> To: Peng Fan <[email protected]>
> Cc: A.s. Dong <[email protected]>; [email protected]; Russell
> King <[email protected]>; Fabio Estevam <[email protected]>;
> Sascha Hauer <[email protected]>; [email protected]; linux-arm-
> [email protected]
> 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 <[email protected]>
> > Cc: Shawn Guo <[email protected]>
> > Cc: Sascha Hauer <[email protected]>
> > Cc: Fabio Estevam <[email protected]>
> > Cc: Russell King <[email protected]>
> > Cc: Dong Aisheng <[email protected]>
> > ---
> > arch/arm/mach-imx/pm-imx6.c | 2 ++
> > arch/arm/mach-imx/suspend-imx6.S | 24 ------------------------
>
> I'm fine with the patch in general. But this piece of code is running on a few
> i.MX6 platforms, and I'm wondering on which SoCs you have verified the
> change work fine.
I tested it on 6Q-SDB board. Is it ok?
Thanks,
Peng.
>
> Shawn
On Tue, Dec 26, 2017 at 06:52:45AM +0000, Peng Fan wrote:
> Hi Shawn,
>
> > -----Original Message-----
> > From: Shawn Guo [mailto:[email protected]]
> > Sent: Tuesday, December 26, 2017 11:31 AM
> > To: Peng Fan <[email protected]>
> > Cc: A.s. Dong <[email protected]>; [email protected]; Russell
> > King <[email protected]>; Fabio Estevam <[email protected]>;
> > Sascha Hauer <[email protected]>; [email protected]; linux-arm-
> > [email protected]
> > 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 <[email protected]>
> > > Cc: Shawn Guo <[email protected]>
> > > Cc: Sascha Hauer <[email protected]>
> > > Cc: Fabio Estevam <[email protected]>
> > > Cc: Russell King <[email protected]>
> > > Cc: Dong Aisheng <[email protected]>
> > > ---
> > > arch/arm/mach-imx/pm-imx6.c | 2 ++
> > > arch/arm/mach-imx/suspend-imx6.S | 24 ------------------------
> >
> > I'm fine with the patch in general. But this piece of code is running on a few
> > i.MX6 platforms, and I'm wondering on which SoCs you have verified the
> > change work fine.
>
> I tested it on 6Q-SDB board. Is it ok?
Okay, I will get it into linux-next for a bit wider testing.
Shawn
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 <[email protected]>
> Cc: Shawn Guo <[email protected]>
> Cc: Sascha Hauer <[email protected]>
> Cc: Fabio Estevam <[email protected]>
> Cc: Russell King <[email protected]>
> Cc: Dong Aisheng <[email protected]>
Changed 'arm: ' prefix to 'ARM: ', and applied patch.
Shawn
Hi Shawn,
> -----Original Message-----
> From: Shawn Guo [mailto:[email protected]]
> Sent: Tuesday, December 26, 2017 5:09 PM
> To: Peng Fan <[email protected]>
> Cc: A.s. Dong <[email protected]>; [email protected]; Russell
> King <[email protected]>; [email protected]; Sascha Hauer
> <[email protected]>; Fabio Estevam <[email protected]>; linux-
> [email protected]
> Subject: Re: [PATCH] arm: imx: suspend/resume: use outer_disable/resume
>
> On Tue, Dec 26, 2017 at 06:52:45AM +0000, Peng Fan wrote:
> > Hi Shawn,
> >
> > > -----Original Message-----
> > > From: Shawn Guo [mailto:[email protected]]
> > > Sent: Tuesday, December 26, 2017 11:31 AM
> > > To: Peng Fan <[email protected]>
> > > Cc: A.s. Dong <[email protected]>; [email protected];
> > > Russell King <[email protected]>; Fabio Estevam
> > > <[email protected]>; Sascha Hauer <[email protected]>;
> > > [email protected]; linux-arm- [email protected]
> > > 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 <[email protected]>
> > > > Cc: Shawn Guo <[email protected]>
> > > > Cc: Sascha Hauer <[email protected]>
> > > > Cc: Fabio Estevam <[email protected]>
> > > > Cc: Russell King <[email protected]>
> > > > Cc: Dong Aisheng <[email protected]>
> > > > ---
> > > > arch/arm/mach-imx/pm-imx6.c | 2 ++
> > > > arch/arm/mach-imx/suspend-imx6.S | 24 ------------------------
> > >
> > > I'm fine with the patch in general. But this piece of code is
> > > running on a few
> > > i.MX6 platforms, and I'm wondering on which SoCs you have verified
> > > the change work fine.
> >
> > I tested it on 6Q-SDB board. Is it ok?
>
> Okay, I will get it into linux-next for a bit wider testing.
Thanks. I missed to note that, I applied this patch in my testing
https://www.spinics.net/lists/arm-kernel/msg623610.html
to avoid suspicious RCU usage.
Thanks,
Peng.
>
> Shawn
Hi Shawn
> -----Original Message-----
> From: Shawn Guo [mailto:[email protected]]
> Sent: Tuesday, December 26, 2017 5:13 PM
> To: Peng Fan <[email protected]>
> Cc: A.s. Dong <[email protected]>; [email protected]; Russell
> King <[email protected]>; Fabio Estevam <[email protected]>;
> Sascha Hauer <[email protected]>; [email protected]; linux-arm-
> [email protected]
> 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 <[email protected]>
> > Cc: Shawn Guo <[email protected]>
> > Cc: Sascha Hauer <[email protected]>
> > Cc: Fabio Estevam <[email protected]>
> > Cc: Russell King <[email protected]>
> > Cc: Dong Aisheng <[email protected]>
>
> 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.
Thanks,
Peng.
>
> Shawn
On Tue, Dec 26, 2017 at 09:49:01AM +0000, Peng Fan wrote:
> Hi Shawn
>
> > -----Original Message-----
> > From: Shawn Guo [mailto:[email protected]]
> > Sent: Tuesday, December 26, 2017 5:13 PM
> > To: Peng Fan <[email protected]>
> > Cc: A.s. Dong <[email protected]>; [email protected]; Russell
> > King <[email protected]>; Fabio Estevam <[email protected]>;
> > Sascha Hauer <[email protected]>; [email protected]; linux-arm-
> > [email protected]
> > 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 <[email protected]>
> > > Cc: Shawn Guo <[email protected]>
> > > Cc: Sascha Hauer <[email protected]>
> > > Cc: Fabio Estevam <[email protected]>
> > > Cc: Russell King <[email protected]>
> > > Cc: Dong Aisheng <[email protected]>
> >
> > 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.
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();
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
Hi Aisheng,
> -----Original Message-----
> From: Dong Aisheng [mailto:[email protected]]
> Sent: Tuesday, December 26, 2017 6:04 PM
> To: Peng Fan <[email protected]>
> Cc: Shawn Guo <[email protected]>; A.s. Dong <[email protected]>;
> [email protected]; Russell King <[email protected]>; Fabio
> Estevam <[email protected]>; Sascha Hauer <[email protected]>;
> [email protected]; [email protected]
> 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:[email protected]]
> > > Sent: Tuesday, December 26, 2017 5:13 PM
> > > To: Peng Fan <[email protected]>
> > > Cc: A.s. Dong <[email protected]>; [email protected];
> > > Russell King <[email protected]>; Fabio Estevam
> > > <[email protected]>; Sascha Hauer <[email protected]>;
> > > [email protected]; linux-arm- [email protected]
> > > 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 <[email protected]>
> > > > Cc: Shawn Guo <[email protected]>
> > > > Cc: Sascha Hauer <[email protected]>
> > > > Cc: Fabio Estevam <[email protected]>
> > > > Cc: Russell King <[email protected]>
> > > > Cc: Dong Aisheng <[email protected]>
> > >
> > > 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
On Tue, Dec 26, 2017 at 10:11:41AM +0000, Peng Fan wrote:
>
> Hi Aisheng,
>
> > -----Original Message-----
> > From: Dong Aisheng [mailto:[email protected]]
> > Sent: Tuesday, December 26, 2017 6:04 PM
> > To: Peng Fan <[email protected]>
> > Cc: Shawn Guo <[email protected]>; A.s. Dong <[email protected]>;
> > [email protected]; Russell King <[email protected]>; Fabio
> > Estevam <[email protected]>; Sascha Hauer <[email protected]>;
> > [email protected]; [email protected]
> > 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:[email protected]]
> > > > Sent: Tuesday, December 26, 2017 5:13 PM
> > > > To: Peng Fan <[email protected]>
> > > > Cc: A.s. Dong <[email protected]>; [email protected];
> > > > Russell King <[email protected]>; Fabio Estevam
> > > > <[email protected]>; Sascha Hauer <[email protected]>;
> > > > [email protected]; linux-arm- [email protected]
> > > > 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 <[email protected]>
> > > > > Cc: Shawn Guo <[email protected]>
> > > > > Cc: Sascha Hauer <[email protected]>
> > > > > Cc: Fabio Estevam <[email protected]>
> > > > > Cc: Russell King <[email protected]>
> > > > > Cc: Dong Aisheng <[email protected]>
> > > >
> > > > 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
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:[email protected]]
> > > Sent: Tuesday, December 26, 2017 6:04 PM
> > > To: Peng Fan <[email protected]>
> > > Cc: Shawn Guo <[email protected]>; A.s. Dong <[email protected]>;
> > > [email protected]; Russell King <[email protected]>; Fabio
> > > Estevam <[email protected]>; Sascha Hauer <[email protected]>;
> > > [email protected]; [email protected]
> > > 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:[email protected]]
> > > > > Sent: Tuesday, December 26, 2017 5:13 PM
> > > > > To: Peng Fan <[email protected]>
> > > > > Cc: A.s. Dong <[email protected]>; [email protected];
> > > > > Russell King <[email protected]>; Fabio Estevam
> > > > > <[email protected]>; Sascha Hauer <[email protected]>;
> > > > > [email protected]; linux-arm- [email protected]
> > > > > 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 <[email protected]>
> > > > > > Cc: Shawn Guo <[email protected]>
> > > > > > Cc: Sascha Hauer <[email protected]>
> > > > > > Cc: Fabio Estevam <[email protected]>
> > > > > > Cc: Russell King <[email protected]>
> > > > > > Cc: Dong Aisheng <[email protected]>
> > > > >
> > > > > 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
> -----Original Message-----
> From: Dong Aisheng [mailto:[email protected]]
> Sent: Tuesday, December 26, 2017 6:21 PM
> To: Peng Fan <[email protected]>
> Cc: Shawn Guo <[email protected]>; A.s. Dong <[email protected]>;
> [email protected]; Russell King <[email protected]>; Fabio
> Estevam <[email protected]>; Sascha Hauer <[email protected]>;
> [email protected]; [email protected]
> 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:[email protected]]
> > > Sent: Tuesday, December 26, 2017 6:04 PM
> > > To: Peng Fan <[email protected]>
> > > Cc: Shawn Guo <[email protected]>; A.s. Dong
> > > <[email protected]>; [email protected]; Russell King
> > > <[email protected]>; Fabio Estevam <[email protected]>;
> > > Sascha Hauer <[email protected]>; [email protected];
> > > [email protected]
> > > 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:[email protected]]
> > > > > Sent: Tuesday, December 26, 2017 5:13 PM
> > > > > To: Peng Fan <[email protected]>
> > > > > Cc: A.s. Dong <[email protected]>;
> > > > > [email protected]; Russell King
> > > > > <[email protected]>; Fabio Estevam <[email protected]>;
> > > > > Sascha Hauer <[email protected]>; [email protected];
> > > > > linux-arm- [email protected]
> > > > > 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 <[email protected]>
> > > > > > Cc: Shawn Guo <[email protected]>
> > > > > > Cc: Sascha Hauer <[email protected]>
> > > > > > Cc: Fabio Estevam <[email protected]>
> > > > > > Cc: Russell King <[email protected]>
> > > > > > Cc: Dong Aisheng <[email protected]>
> > > > >
> > > > > 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
On Tue, Dec 26, 2017 at 09:49:01AM +0000, Peng Fan wrote:
> Hi Shawn
>
> > -----Original Message-----
> > From: Shawn Guo [mailto:[email protected]]
> > Sent: Tuesday, December 26, 2017 5:13 PM
> > To: Peng Fan <[email protected]>
> > Cc: A.s. Dong <[email protected]>; [email protected]; Russell
> > King <[email protected]>; Fabio Estevam <[email protected]>;
> > Sascha Hauer <[email protected]>; [email protected]; linux-arm-
> > [email protected]
> > 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 <[email protected]>
> > > Cc: Shawn Guo <[email protected]>
> > > Cc: Sascha Hauer <[email protected]>
> > > Cc: Fabio Estevam <[email protected]>
> > > Cc: Russell King <[email protected]>
> > > Cc: Dong Aisheng <[email protected]>
> >
> > 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.
Patch dropped.
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
Hi Shawn,
> >
> > 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 <[email protected]>
> > > Cc: Shawn Guo <[email protected]>
> > > Cc: Sascha Hauer <[email protected]>
> > > Cc: Fabio Estevam <[email protected]>
> > > Cc: Russell King <[email protected]>
> > > Cc: Dong Aisheng <[email protected]>
> >
> > 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.
I just send out V2 patch to fix the 6sx issue. Sorry for the inconvenience about V1
that break 6sx. Please review.
Thanks,
Peng.
.
>
> Thanks,
> Peng.
>
> >
> > Shawn