[PATCH] arm: imx: suspend/resume: use outer_disable/resume

[PATCH] arm: imx: suspend/resume: use outer_disable/resume

[Peng Fan]

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 <peng.fan@nxp.com>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Dong Aisheng <aisheng.dong@nxp.com>
---
 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

RE: [PATCH] arm: imx: suspend/resume: use outer_disable/resume

[Peng Fan]

Hi All,

Ping...

> -----Original Message-----
> From: Peng Fan
> Sent: Sunday, December 10, 2017 8:07 PM
> To: shawnguo@kernel.org
> Cc: linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> van.freenix@gmail.com; Peng Fan <peng.fan@nxp.com>; Sascha Hauer
> <kernel@pengutronix.de>; Fabio Estevam <fabio.estevam@nxp.com>; Russell
> King <linux@armlinux.org.uk>; A.s. Dong <aisheng.dong@nxp.com>
> 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 <peng.fan@nxp.com>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Dong Aisheng <aisheng.dong@nxp.com>
> ---
>  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.

Re: [PATCH] arm: imx: suspend/resume: use outer_disable/resume

[Shawn Guo]

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 <peng.fan@nxp.com>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Dong Aisheng <aisheng.dong@nxp.com>
> ---
>  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

RE: [PATCH] arm: imx: suspend/resume: use outer_disable/resume

[Peng Fan]

Hi Shawn,

> -----Original Message-----
> From: Shawn Guo [mailto:shawnguo@kernel.org]
> Sent: Tuesday, December 26, 2017 11:31 AM
> To: Peng Fan <peng.fan@nxp.com>
> Cc: A.s. Dong <aisheng.dong@nxp.com>; linux-kernel@vger.kernel.org; Russell
> King <linux@armlinux.org.uk>; Fabio Estevam <fabio.estevam@nxp.com>;
> Sascha Hauer <kernel@pengutronix.de>; van.freenix@gmail.com; linux-arm-
> kernel@lists.infradead.org
> 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 <peng.fan@nxp.com>
> > Cc: Shawn Guo <shawnguo@kernel.org>
> > Cc: Sascha Hauer <kernel@pengutronix.de>
> > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > Cc: Russell King <linux@armlinux.org.uk>
> > Cc: Dong Aisheng <aisheng.dong@nxp.com>
> > ---
> >  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

Re: [PATCH] arm: imx: suspend/resume: use outer_disable/resume

[Shawn Guo]

On Tue, Dec 26, 2017 at 06:52:45AM +0000, Peng Fan wrote:
> Hi Shawn,
> 
> > -----Original Message-----
> > From: Shawn Guo [mailto:shawnguo@kernel.org]
> > Sent: Tuesday, December 26, 2017 11:31 AM
> > To: Peng Fan <peng.fan@nxp.com>
> > Cc: A.s. Dong <aisheng.dong@nxp.com>; linux-kernel@vger.kernel.org; Russell
> > King <linux@armlinux.org.uk>; Fabio Estevam <fabio.estevam@nxp.com>;
> > Sascha Hauer <kernel@pengutronix.de>; van.freenix@gmail.com; linux-arm-
> > kernel@lists.infradead.org
> > 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 <peng.fan@nxp.com>
> > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > Cc: Sascha Hauer <kernel@pengutronix.de>
> > > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > > Cc: Russell King <linux@armlinux.org.uk>
> > > Cc: Dong Aisheng <aisheng.dong@nxp.com>
> > > ---
> > >  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

Re: [PATCH] arm: imx: suspend/resume: use outer_disable/resume

[Shawn Guo]

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 <peng.fan@nxp.com>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Dong Aisheng <aisheng.dong@nxp.com>

Changed 'arm: ' prefix to 'ARM: ', and applied patch.

Shawn

RE: [PATCH] arm: imx: suspend/resume: use outer_disable/resume

[Peng Fan]

Hi Shawn,

> -----Original Message-----
> From: Shawn Guo [mailto:shawnguo@kernel.org]
> Sent: Tuesday, December 26, 2017 5:09 PM
> To: Peng Fan <peng.fan@nxp.com>
> Cc: A.s. Dong <aisheng.dong@nxp.com>; linux-kernel@vger.kernel.org; Russell
> King <linux@armlinux.org.uk>; van.freenix@gmail.com; Sascha Hauer
> <kernel@pengutronix.de>; Fabio Estevam <fabio.estevam@nxp.com>; linux-
> arm-kernel@lists.infradead.org
> 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:shawnguo@kernel.org]
> > > Sent: Tuesday, December 26, 2017 11:31 AM
> > > To: Peng Fan <peng.fan@nxp.com>
> > > Cc: A.s. Dong <aisheng.dong@nxp.com>; linux-kernel@vger.kernel.org;
> > > Russell King <linux@armlinux.org.uk>; Fabio Estevam
> > > <fabio.estevam@nxp.com>; Sascha Hauer <kernel@pengutronix.de>;
> > > van.freenix@gmail.com; linux-arm- kernel@lists.infradead.org
> > > 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 <peng.fan@nxp.com>
> > > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > > Cc: Sascha Hauer <kernel@pengutronix.de>
> > > > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > > > Cc: Russell King <linux@armlinux.org.uk>
> > > > Cc: Dong Aisheng <aisheng.dong@nxp.com>
> > > > ---
> > > >  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

RE: [PATCH] arm: imx: suspend/resume: use outer_disable/resume

[Peng Fan]

Hi Shawn

> -----Original Message-----
> From: Shawn Guo [mailto:shawnguo@kernel.org]
> Sent: Tuesday, December 26, 2017 5:13 PM
> To: Peng Fan <peng.fan@nxp.com>
> Cc: A.s. Dong <aisheng.dong@nxp.com>; linux-kernel@vger.kernel.org; Russell
> King <linux@armlinux.org.uk>; Fabio Estevam <fabio.estevam@nxp.com>;
> Sascha Hauer <kernel@pengutronix.de>; van.freenix@gmail.com; linux-arm-
> kernel@lists.infradead.org
> 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 <peng.fan@nxp.com>
> > Cc: Shawn Guo <shawnguo@kernel.org>
> > Cc: Sascha Hauer <kernel@pengutronix.de>
> > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > Cc: Russell King <linux@armlinux.org.uk>
> > Cc: Dong Aisheng <aisheng.dong@nxp.com>
> 
> 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

Re: [PATCH] arm: imx: suspend/resume: use outer_disable/resume

[Dong Aisheng]

On Tue, Dec 26, 2017 at 09:49:01AM +0000, Peng Fan wrote:
> Hi Shawn
> 
> > -----Original Message-----
> > From: Shawn Guo [mailto:shawnguo@kernel.org]
> > Sent: Tuesday, December 26, 2017 5:13 PM
> > To: Peng Fan <peng.fan@nxp.com>
> > Cc: A.s. Dong <aisheng.dong@nxp.com>; linux-kernel@vger.kernel.org; Russell
> > King <linux@armlinux.org.uk>; Fabio Estevam <fabio.estevam@nxp.com>;
> > Sascha Hauer <kernel@pengutronix.de>; van.freenix@gmail.com; linux-arm-
> > kernel@lists.infradead.org
> > 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 <peng.fan@nxp.com>
> > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > Cc: Sascha Hauer <kernel@pengutronix.de>
> > > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > > Cc: Russell King <linux@armlinux.org.uk>
> > > Cc: Dong Aisheng <aisheng.dong@nxp.com>
> > 
> > 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

RE: [PATCH] arm: imx: suspend/resume: use outer_disable/resume

[Peng Fan]

Hi Aisheng,

> -----Original Message-----
> From: Dong Aisheng [mailto:dongas86@gmail.com]
> Sent: Tuesday, December 26, 2017 6:04 PM
> To: Peng Fan <peng.fan@nxp.com>
> Cc: Shawn Guo <shawnguo@kernel.org>; A.s. Dong <aisheng.dong@nxp.com>;
> linux-kernel@vger.kernel.org; Russell King <linux@armlinux.org.uk>; Fabio
> Estevam <fabio.estevam@nxp.com>; Sascha Hauer <kernel@pengutronix.de>;
> van.freenix@gmail.com; linux-arm-kernel@lists.infradead.org
> 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:shawnguo@kernel.org]
> > > Sent: Tuesday, December 26, 2017 5:13 PM
> > > To: Peng Fan <peng.fan@nxp.com>
> > > Cc: A.s. Dong <aisheng.dong@nxp.com>; linux-kernel@vger.kernel.org;
> > > Russell King <linux@armlinux.org.uk>; Fabio Estevam
> > > <fabio.estevam@nxp.com>; Sascha Hauer <kernel@pengutronix.de>;
> > > van.freenix@gmail.com; linux-arm- kernel@lists.infradead.org
> > > 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 <peng.fan@nxp.com>
> > > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > > Cc: Sascha Hauer <kernel@pengutronix.de>
> > > > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > > > Cc: Russell King <linux@armlinux.org.uk>
> > > > Cc: Dong Aisheng <aisheng.dong@nxp.com>
> > >
> > > 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

Re: [PATCH] arm: imx: suspend/resume: use outer_disable/resume

[Dong Aisheng]

On Tue, Dec 26, 2017 at 10:11:41AM +0000, Peng Fan wrote:
> 
> Hi Aisheng,
> 
> > -----Original Message-----
> > From: Dong Aisheng [mailto:dongas86@gmail.com]
> > Sent: Tuesday, December 26, 2017 6:04 PM
> > To: Peng Fan <peng.fan@nxp.com>
> > Cc: Shawn Guo <shawnguo@kernel.org>; A.s. Dong <aisheng.dong@nxp.com>;
> > linux-kernel@vger.kernel.org; Russell King <linux@armlinux.org.uk>; Fabio
> > Estevam <fabio.estevam@nxp.com>; Sascha Hauer <kernel@pengutronix.de>;
> > van.freenix@gmail.com; linux-arm-kernel@lists.infradead.org
> > 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:shawnguo@kernel.org]
> > > > Sent: Tuesday, December 26, 2017 5:13 PM
> > > > To: Peng Fan <peng.fan@nxp.com>
> > > > Cc: A.s. Dong <aisheng.dong@nxp.com>; linux-kernel@vger.kernel.org;
> > > > Russell King <linux@armlinux.org.uk>; Fabio Estevam
> > > > <fabio.estevam@nxp.com>; Sascha Hauer <kernel@pengutronix.de>;
> > > > van.freenix@gmail.com; linux-arm- kernel@lists.infradead.org
> > > > 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 <peng.fan@nxp.com>
> > > > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > > > Cc: Sascha Hauer <kernel@pengutronix.de>
> > > > > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > > > > Cc: Russell King <linux@armlinux.org.uk>
> > > > > Cc: Dong Aisheng <aisheng.dong@nxp.com>
> > > >
> > > > 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

Re: [PATCH] arm: imx: suspend/resume: use outer_disable/resume

[Dong Aisheng]

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:dongas86@gmail.com]
> > > Sent: Tuesday, December 26, 2017 6:04 PM
> > > To: Peng Fan <peng.fan@nxp.com>
> > > Cc: Shawn Guo <shawnguo@kernel.org>; A.s. Dong <aisheng.dong@nxp.com>;
> > > linux-kernel@vger.kernel.org; Russell King <linux@armlinux.org.uk>; Fabio
> > > Estevam <fabio.estevam@nxp.com>; Sascha Hauer <kernel@pengutronix.de>;
> > > van.freenix@gmail.com; linux-arm-kernel@lists.infradead.org
> > > 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:shawnguo@kernel.org]
> > > > > Sent: Tuesday, December 26, 2017 5:13 PM
> > > > > To: Peng Fan <peng.fan@nxp.com>
> > > > > Cc: A.s. Dong <aisheng.dong@nxp.com>; linux-kernel@vger.kernel.org;
> > > > > Russell King <linux@armlinux.org.uk>; Fabio Estevam
> > > > > <fabio.estevam@nxp.com>; Sascha Hauer <kernel@pengutronix.de>;
> > > > > van.freenix@gmail.com; linux-arm- kernel@lists.infradead.org
> > > > > 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 <peng.fan@nxp.com>
> > > > > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > > > > Cc: Sascha Hauer <kernel@pengutronix.de>
> > > > > > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > > > > > Cc: Russell King <linux@armlinux.org.uk>
> > > > > > Cc: Dong Aisheng <aisheng.dong@nxp.com>
> > > > >
> > > > > 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

RE: [PATCH] arm: imx: suspend/resume: use outer_disable/resume

[Peng Fan]

> -----Original Message-----
> From: Dong Aisheng [mailto:dongas86@gmail.com]
> Sent: Tuesday, December 26, 2017 6:21 PM
> To: Peng Fan <peng.fan@nxp.com>
> Cc: Shawn Guo <shawnguo@kernel.org>; A.s. Dong <aisheng.dong@nxp.com>;
> linux-kernel@vger.kernel.org; Russell King <linux@armlinux.org.uk>; Fabio
> Estevam <fabio.estevam@nxp.com>; Sascha Hauer <kernel@pengutronix.de>;
> van.freenix@gmail.com; linux-arm-kernel@lists.infradead.org
> 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:dongas86@gmail.com]
> > > Sent: Tuesday, December 26, 2017 6:04 PM
> > > To: Peng Fan <peng.fan@nxp.com>
> > > Cc: Shawn Guo <shawnguo@kernel.org>; A.s. Dong
> > > <aisheng.dong@nxp.com>; linux-kernel@vger.kernel.org; Russell King
> > > <linux@armlinux.org.uk>; Fabio Estevam <fabio.estevam@nxp.com>;
> > > Sascha Hauer <kernel@pengutronix.de>; van.freenix@gmail.com;
> > > linux-arm-kernel@lists.infradead.org
> > > 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:shawnguo@kernel.org]
> > > > > Sent: Tuesday, December 26, 2017 5:13 PM
> > > > > To: Peng Fan <peng.fan@nxp.com>
> > > > > Cc: A.s. Dong <aisheng.dong@nxp.com>;
> > > > > linux-kernel@vger.kernel.org; Russell King
> > > > > <linux@armlinux.org.uk>; Fabio Estevam <fabio.estevam@nxp.com>;
> > > > > Sascha Hauer <kernel@pengutronix.de>; van.freenix@gmail.com;
> > > > > linux-arm- kernel@lists.infradead.org
> > > > > 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 <peng.fan@nxp.com>
> > > > > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > > > > Cc: Sascha Hauer <kernel@pengutronix.de>
> > > > > > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > > > > > Cc: Russell King <linux@armlinux.org.uk>
> > > > > > Cc: Dong Aisheng <aisheng.dong@nxp.com>
> > > > >
> > > > > 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

Re: [PATCH] arm: imx: suspend/resume: use outer_disable/resume

[Shawn Guo]

On Tue, Dec 26, 2017 at 09:49:01AM +0000, Peng Fan wrote:
> Hi Shawn
> 
> > -----Original Message-----
> > From: Shawn Guo [mailto:shawnguo@kernel.org]
> > Sent: Tuesday, December 26, 2017 5:13 PM
> > To: Peng Fan <peng.fan@nxp.com>
> > Cc: A.s. Dong <aisheng.dong@nxp.com>; linux-kernel@vger.kernel.org; Russell
> > King <linux@armlinux.org.uk>; Fabio Estevam <fabio.estevam@nxp.com>;
> > Sascha Hauer <kernel@pengutronix.de>; van.freenix@gmail.com; linux-arm-
> > kernel@lists.infradead.org
> > 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 <peng.fan@nxp.com>
> > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > Cc: Sascha Hauer <kernel@pengutronix.de>
> > > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > > Cc: Russell King <linux@armlinux.org.uk>
> > > Cc: Dong Aisheng <aisheng.dong@nxp.com>
> > 
> > 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.

Re: [PATCH] arm: imx: suspend/resume: use outer_disable/resume

[Shawn Guo]

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

RE: [PATCH] arm: imx: suspend/resume: use outer_disable/resume

[Peng Fan]

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 <peng.fan@nxp.com>
> > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > Cc: Sascha Hauer <kernel@pengutronix.de>
> > > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > > Cc: Russell King <linux@armlinux.org.uk>
> > > Cc: Dong Aisheng <aisheng.dong@nxp.com>
> >
> > 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