[PATCH] ARM: imx6ull: Make suspend/resume work like on 6ul

[PATCH] ARM: imx6ull: Make suspend/resume work like on 6ul

[Leonard Crestez]

Suspend and resume on imx6ull is currenty not working because of some
missed checks where behavior should match imx6ul.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 arch/arm/mach-imx/mxc.h     | 6 ++++++
 arch/arm/mach-imx/pm-imx6.c | 6 ++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-imx/mxc.h b/arch/arm/mach-imx/mxc.h
index 34f2ff6..e00d626 100644
--- a/arch/arm/mach-imx/mxc.h
+++ b/arch/arm/mach-imx/mxc.h
@@ -39,6 +39,7 @@
 #define MXC_CPU_IMX6SX		0x62
 #define MXC_CPU_IMX6Q		0x63
 #define MXC_CPU_IMX6UL		0x64
+#define MXC_CPU_IMX6ULL		0x65
 #define MXC_CPU_IMX7D		0x72
 
 #define IMX_DDR_TYPE_LPDDR2		1
@@ -73,6 +74,11 @@ static inline bool cpu_is_imx6ul(void)
 	return __mxc_cpu_type == MXC_CPU_IMX6UL;
 }
 
+static inline bool cpu_is_imx6ull(void)
+{
+	return __mxc_cpu_type == MXC_CPU_IMX6ULL;
+}
+
 static inline bool cpu_is_imx6q(void)
 {
 	return __mxc_cpu_type == MXC_CPU_IMX6Q;
diff --git a/arch/arm/mach-imx/pm-imx6.c b/arch/arm/mach-imx/pm-imx6.c
index e61b1d1..ecdf071 100644
--- a/arch/arm/mach-imx/pm-imx6.c
+++ b/arch/arm/mach-imx/pm-imx6.c
@@ -295,7 +295,8 @@ int imx6_set_lpm(enum mxc_cpu_pwr_mode mode)
 		val &= ~BM_CLPCR_SBYOS;
 		if (cpu_is_imx6sl())
 			val |= BM_CLPCR_BYPASS_PMIC_READY;
-		if (cpu_is_imx6sl() || cpu_is_imx6sx() || cpu_is_imx6ul())
+		if (cpu_is_imx6sl() || cpu_is_imx6sx() || cpu_is_imx6ul() ||
+		    cpu_is_imx6ull())
 			val |= BM_CLPCR_BYP_MMDC_CH0_LPM_HS;
 		else
 			val |= BM_CLPCR_BYP_MMDC_CH1_LPM_HS;
@@ -312,7 +313,8 @@ int imx6_set_lpm(enum mxc_cpu_pwr_mode mode)
 		val |= BM_CLPCR_SBYOS;
 		if (cpu_is_imx6sl() || cpu_is_imx6sx())
 			val |= BM_CLPCR_BYPASS_PMIC_READY;
-		if (cpu_is_imx6sl() || cpu_is_imx6sx() || cpu_is_imx6ul())
+		if (cpu_is_imx6sl() || cpu_is_imx6sx() || cpu_is_imx6ul() ||
+		    cpu_is_imx6ull())
 			val |= BM_CLPCR_BYP_MMDC_CH0_LPM_HS;
 		else
 			val |= BM_CLPCR_BYP_MMDC_CH1_LPM_HS;
-- 
2.7.4

Re: [PATCH] ARM: imx6ull: Make suspend/resume work like on 6ul

[Shawn Guo]

On Tue, May 30, 2017 at 07:11:19PM +0300, Leonard Crestez wrote:
> Suspend and resume on imx6ull is currenty not working because of some
> missed checks where behavior should match imx6ul.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  arch/arm/mach-imx/mxc.h     | 6 ++++++
>  arch/arm/mach-imx/pm-imx6.c | 6 ++++--
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/mxc.h b/arch/arm/mach-imx/mxc.h
> index 34f2ff6..e00d626 100644
> --- a/arch/arm/mach-imx/mxc.h
> +++ b/arch/arm/mach-imx/mxc.h
> @@ -39,6 +39,7 @@
>  #define MXC_CPU_IMX6SX		0x62
>  #define MXC_CPU_IMX6Q		0x63
>  #define MXC_CPU_IMX6UL		0x64
> +#define MXC_CPU_IMX6ULL		0x65

Since you are adding a new CPU type, you should probably patch
imx_soc_device_init() for it as well.

Shawn

>  #define MXC_CPU_IMX7D		0x72
>  
>  #define IMX_DDR_TYPE_LPDDR2		1
> @@ -73,6 +74,11 @@ static inline bool cpu_is_imx6ul(void)
>  	return __mxc_cpu_type == MXC_CPU_IMX6UL;
>  }
>  
> +static inline bool cpu_is_imx6ull(void)
> +{
> +	return __mxc_cpu_type == MXC_CPU_IMX6ULL;
> +}
> +
>  static inline bool cpu_is_imx6q(void)
>  {
>  	return __mxc_cpu_type == MXC_CPU_IMX6Q;
> diff --git a/arch/arm/mach-imx/pm-imx6.c b/arch/arm/mach-imx/pm-imx6.c
> index e61b1d1..ecdf071 100644
> --- a/arch/arm/mach-imx/pm-imx6.c
> +++ b/arch/arm/mach-imx/pm-imx6.c
> @@ -295,7 +295,8 @@ int imx6_set_lpm(enum mxc_cpu_pwr_mode mode)
>  		val &= ~BM_CLPCR_SBYOS;
>  		if (cpu_is_imx6sl())
>  			val |= BM_CLPCR_BYPASS_PMIC_READY;
> -		if (cpu_is_imx6sl() || cpu_is_imx6sx() || cpu_is_imx6ul())
> +		if (cpu_is_imx6sl() || cpu_is_imx6sx() || cpu_is_imx6ul() ||
> +		    cpu_is_imx6ull())
>  			val |= BM_CLPCR_BYP_MMDC_CH0_LPM_HS;
>  		else
>  			val |= BM_CLPCR_BYP_MMDC_CH1_LPM_HS;
> @@ -312,7 +313,8 @@ int imx6_set_lpm(enum mxc_cpu_pwr_mode mode)
>  		val |= BM_CLPCR_SBYOS;
>  		if (cpu_is_imx6sl() || cpu_is_imx6sx())
>  			val |= BM_CLPCR_BYPASS_PMIC_READY;
> -		if (cpu_is_imx6sl() || cpu_is_imx6sx() || cpu_is_imx6ul())
> +		if (cpu_is_imx6sl() || cpu_is_imx6sx() || cpu_is_imx6ul() ||
> +		    cpu_is_imx6ull())
>  			val |= BM_CLPCR_BYP_MMDC_CH0_LPM_HS;
>  		else
>  			val |= BM_CLPCR_BYP_MMDC_CH1_LPM_HS;
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] ARM: imx6ull: Make suspend/resume work like on 6ul

[Leonard Crestez]

On Mon, 2017-06-05 at 13:37 +0800, Shawn Guo wrote:
> On Tue, May 30, 2017 at 07:11:19PM +0300, Leonard Crestez wrote:
> > 
> > Suspend and resume on imx6ull is currenty not working because of some
> > missed checks where behavior should match imx6ul.
> > 
> > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> > ---
> >  arch/arm/mach-imx/mxc.h     | 6 ++++++
> >  arch/arm/mach-imx/pm-imx6.c | 6 ++++--
> >  2 files changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm/mach-imx/mxc.h b/arch/arm/mach-imx/mxc.h
> > index 34f2ff6..e00d626 100644
> > --- a/arch/arm/mach-imx/mxc.h
> > +++ b/arch/arm/mach-imx/mxc.h
> > @@ -39,6 +39,7 @@
> >  #define MXC_CPU_IMX6SX		0x62
> >  #define MXC_CPU_IMX6Q		0x63
> >  #define MXC_CPU_IMX6UL		0x64
> > +#define MXC_CPU_IMX6ULL		0x65
> Since you are adding a new CPU type, you should probably patch
> imx_soc_device_init() for it as well.

Ok, I will resend as a 2-patch series.

BTW, it actually seems to me that setting BM_CLPCR_BYP_MMDC_CH0_LPM_HS
on imx6sl/sx/ul/ull is not actually needed. That bit (19) is documented
as "reserved" in the Reference Manual and likely ignored by hardware.

As far as I understand the MMDC on imx6qdl has two channels and unless
2-channel mode is enabled (not currently supported) the handshake with
CH1 needs to be disabled. Other reduced chips only have one MMDC
channel and that is CH1 (CH0 was removed) and nothing needs to be done
from them. The only important thing is to avoid setting
BM_CLPCR_BYP_MMDC_CH1_LPM_HS.

However perhaps what I am saying is wrong for some early chip versions?
Because this behavior of setting BM_CLPCR_BYP_MMDC_CH0_LPM_HS has been
in the kernel for a long time.

--
Regards,
Leonard

Re: [PATCH] ARM: imx6ull: Make suspend/resume work like on 6ul

[Shawn Guo]

On Tue, Jun 06, 2017 at 01:51:53PM +0300, Leonard Crestez wrote:
> On Mon, 2017-06-05 at 13:37 +0800, Shawn Guo wrote:
> > On Tue, May 30, 2017 at 07:11:19PM +0300, Leonard Crestez wrote:
> > > 
> > > Suspend and resume on imx6ull is currenty not working because of some
> > > missed checks where behavior should match imx6ul.
> > > 
> > > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> > > ---
> > >  arch/arm/mach-imx/mxc.h     | 6 ++++++
> > >  arch/arm/mach-imx/pm-imx6.c | 6 ++++--
> > >  2 files changed, 10 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/arm/mach-imx/mxc.h b/arch/arm/mach-imx/mxc.h
> > > index 34f2ff6..e00d626 100644
> > > --- a/arch/arm/mach-imx/mxc.h
> > > +++ b/arch/arm/mach-imx/mxc.h
> > > @@ -39,6 +39,7 @@
> > >  #define MXC_CPU_IMX6SX		0x62
> > >  #define MXC_CPU_IMX6Q		0x63
> > >  #define MXC_CPU_IMX6UL		0x64
> > > +#define MXC_CPU_IMX6ULL		0x65
> > Since you are adding a new CPU type, you should probably patch
> > imx_soc_device_init() for it as well.
> 
> Ok, I will resend as a 2-patch series.
> 
> BTW, it actually seems to me that setting BM_CLPCR_BYP_MMDC_CH0_LPM_HS
> on imx6sl/sx/ul/ull is not actually needed. That bit (19) is documented
> as "reserved" in the Reference Manual and likely ignored by hardware.
> 
> As far as I understand the MMDC on imx6qdl has two channels and unless
> 2-channel mode is enabled (not currently supported) the handshake with
> CH1 needs to be disabled. Other reduced chips only have one MMDC
> channel and that is CH1 (CH0 was removed) and nothing needs to be done
> from them. The only important thing is to avoid setting
> BM_CLPCR_BYP_MMDC_CH1_LPM_HS.
> 
> However perhaps what I am saying is wrong for some early chip versions?
> Because this behavior of setting BM_CLPCR_BYP_MMDC_CH0_LPM_HS has been
> in the kernel for a long time.

@Anson, you might be the right person to comment here?

Shawn

RE: [PATCH] ARM: imx6ull: Make suspend/resume work like on 6ul

[Anson Huang]

Best Regards!
Anson Huang



> -----Original Message-----
> From: Shawn Guo [mailto:shawnguo@kernel.org]
> Sent: 2017-06-07 11:21 AM
> To: Anson Huang <anson.huang@nxp.com>
> Cc: Leonard Crestez <leonard.crestez@nxp.com>; Peter Chen
> <peter.chen@nxp.com>; linux-kernel@vger.kernel.org; Fabio Estevam
> <fabio.estevam@nxp.com>; linux-arm-kernel@lists.infradead.org; Lucas Stach
> <l.stach@pengutronix.de>
> Subject: Re: [PATCH] ARM: imx6ull: Make suspend/resume work like on 6ul
> 
> On Tue, Jun 06, 2017 at 01:51:53PM +0300, Leonard Crestez wrote:
> > On Mon, 2017-06-05 at 13:37 +0800, Shawn Guo wrote:
> > > On Tue, May 30, 2017 at 07:11:19PM +0300, Leonard Crestez wrote:
> > > >
> > > > Suspend and resume on imx6ull is currenty not working because of
> > > > some missed checks where behavior should match imx6ul.
> > > >
> > > > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> > > > ---
> > > >  arch/arm/mach-imx/mxc.h     | 6 ++++++
> > > >  arch/arm/mach-imx/pm-imx6.c | 6 ++++--
> > > >  2 files changed, 10 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/arch/arm/mach-imx/mxc.h b/arch/arm/mach-imx/mxc.h
> > > > index 34f2ff6..e00d626 100644
> > > > --- a/arch/arm/mach-imx/mxc.h
> > > > +++ b/arch/arm/mach-imx/mxc.h
> > > > @@ -39,6 +39,7 @@
> > > >  #define MXC_CPU_IMX6SX		0x62
> > > >  #define MXC_CPU_IMX6Q		0x63
> > > >  #define MXC_CPU_IMX6UL		0x64
> > > > +#define MXC_CPU_IMX6ULL		0x65
> > > Since you are adding a new CPU type, you should probably patch
> > > imx_soc_device_init() for it as well.
> >
> > Ok, I will resend as a 2-patch series.
> >
> > BTW, it actually seems to me that setting
> BM_CLPCR_BYP_MMDC_CH0_LPM_HS
> > on imx6sl/sx/ul/ull is not actually needed. That bit (19) is
> > documented as "reserved" in the Reference Manual and likely ignored by
> hardware.
> >
> > As far as I understand the MMDC on imx6qdl has two channels and unless
> > 2-channel mode is enabled (not currently supported) the handshake with
> > CH1 needs to be disabled. Other reduced chips only have one MMDC
> > channel and that is CH1 (CH0 was removed) and nothing needs to be done
> > from them. The only important thing is to avoid setting
> > BM_CLPCR_BYP_MMDC_CH1_LPM_HS.
> >
> > However perhaps what I am saying is wrong for some early chip versions?
> > Because this behavior of setting BM_CLPCR_BYP_MMDC_CH0_LPM_HS has
> been
> > in the kernel for a long time.

As far as I know, since i.MX6SX, we only use MMDC_CH1, but MMDC_CH0 is still
there and we need to bypass its handshake when entering low power mode.

The bit 19 in DOC is incorrect, I remembered I ever tried it and discuss with design
time, they request DOC team to update DOC,
but I think doc team forgot to do it. You can try removing this bit 19 setting
and see if standby/mem suspend can still work? And try to modify this bit 19 value
to see if it can be modified.

Anson.


> 
> @Anson, you might be the right person to comment here?
> 
> Shawn

Re: [PATCH] ARM: imx6ull: Make suspend/resume work like on 6ul

[Leonard Crestez]

On Wed, 2017-06-07 at 03:38 +0000, Anson Huang wrote:
> > -----Original Message-----
> > From: Shawn Guo [mailto:shawnguo@kernel.org]
> > Sent: 2017-06-07 11:21 AM
> > To: Anson Huang <anson.huang@nxp.com>
> > Cc: Leonard Crestez <leonard.crestez@nxp.com>; Peter Chen
> > <peter.chen@nxp.com>; linux-kernel@vger.kernel.org; Fabio Estevam
> > <fabio.estevam@nxp.com>; linux-arm-kernel@lists.infradead.org; Lucas Stach
> > <l.stach@pengutronix.de>
> > Subject: Re: [PATCH] ARM: imx6ull: Make suspend/resume work like on 6ul
> > 
> > On Tue, Jun 06, 2017 at 01:51:53PM +0300, Leonard Crestez wrote:
> > > 
> > > On Mon, 2017-06-05 at 13:37 +0800, Shawn Guo wrote:
> > > > 
> > > > On Tue, May 30, 2017 at 07:11:19PM +0300, Leonard Crestez wrote:
> > > > > 
> > > > > 
> > > > > Suspend and resume on imx6ull is currenty not working because of
> > > > > some missed checks where behavior should match imx6ul.
> > > > > 
> > > > > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> > > > > ---
> > > > >  arch/arm/mach-imx/mxc.h     | 6 ++++++
> > > > >  arch/arm/mach-imx/pm-imx6.c | 6 ++++--
> > > > >  2 files changed, 10 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/arch/arm/mach-imx/mxc.h b/arch/arm/mach-imx/mxc.h
> > > > > index 34f2ff6..e00d626 100644
> > > > > --- a/arch/arm/mach-imx/mxc.h
> > > > > +++ b/arch/arm/mach-imx/mxc.h
> > > > > @@ -39,6 +39,7 @@
> > > > >  #define MXC_CPU_IMX6SX		0x62
> > > > >  #define MXC_CPU_IMX6Q		0x63
> > > > >  #define MXC_CPU_IMX6UL		0x64
> > > > > +#define MXC_CPU_IMX6ULL		0x65
> > > > Since you are adding a new CPU type, you should probably patch
> > > > imx_soc_device_init() for it as well.
> > > Ok, I will resend as a 2-patch series.
> > > 
> > > BTW, it actually seems to me that setting
> > BM_CLPCR_BYP_MMDC_CH0_LPM_HS
> > > 
> > > on imx6sl/sx/ul/ull is not actually needed. That bit (19) is
> > > documented as "reserved" in the Reference Manual and likely ignored by
> > hardware.
> > > 
> > > 
> > > As far as I understand the MMDC on imx6qdl has two channels and unless
> > > 2-channel mode is enabled (not currently supported) the handshake with
> > > CH1 needs to be disabled. Other reduced chips only have one MMDC
> > > channel and that is CH1 (CH0 was removed) and nothing needs to be done
> > > from them. The only important thing is to avoid setting
> > > BM_CLPCR_BYP_MMDC_CH1_LPM_HS.
> > > 
> > > However perhaps what I am saying is wrong for some early chip versions?
> > > Because this behavior of setting BM_CLPCR_BYP_MMDC_CH0_LPM_HS has
> > been
> > > 
> > > in the kernel for a long time.
> As far as I know, since i.MX6SX, we only use MMDC_CH1, but MMDC_CH0 is still
> there and we need to bypass its handshake when entering low power mode.
> 
> The bit 19 in DOC is incorrect, I remembered I ever tried it and discuss with design
> time, they request DOC team to update DOC,
> but I think doc team forgot to do it. You can try removing this bit 19 setting
> and see if standby/mem suspend can still work? And try to modify this bit 19 value
> to see if it can be modified.

After some testing it seems that setting MMDC_CH0 is indeed required.
So nevermind

I remember testing this earlier and getting the opposite result but I
probably confused the STOP_POWER_ON and STOP_POWER_OFF paths.

--
Regards,
Leonard