| Message ID | c63fd9031493cefed47d59c9918c0ecb76ffaf85.1496160320.git.leonard.crestez@nxp.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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;
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(-)