| Message ID | 20190306043042.8926-1-okuno.kohji@jp.panasonic.com |
|---|---|
| State | New |
| Headers | show |
| Series | ARM: imx6: cpuidle: omit the unnecessary unmask of GINT | expand |
On Wed, Mar 06, 2019 at 01:30:42PM +0900, Kohji Okuno wrote: > In imx6_set_lpm, we only need to unmask GINT when not WAIT_CLOCKED, > so add a check condition. Can you elaborate the problem we have without this code change? Shawn > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > Signed-off-by: Kohji Okuno <okuno.kohji@jp.panasonic.com> > --- > arch/arm/mach-imx/pm-imx6.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/mach-imx/pm-imx6.c b/arch/arm/mach-imx/pm-imx6.c > index 87f45b926c78..54add0178b96 100644 > --- a/arch/arm/mach-imx/pm-imx6.c > +++ b/arch/arm/mach-imx/pm-imx6.c > @@ -354,9 +354,11 @@ int imx6_set_lpm(enum mxc_cpu_pwr_mode mode) > * > * Note that IRQ #32 is GIC SPI #0. > */ > - imx_gpc_hwirq_unmask(0); > + if (mode != WAIT_CLOCKED) > + imx_gpc_hwirq_unmask(0); > writel_relaxed(val, ccm_base + CLPCR); > - imx_gpc_hwirq_mask(0); > + if (mode != WAIT_CLOCKED) > + imx_gpc_hwirq_mask(0); > > return 0; > } > -- > 2.17.1 >
Hi Peng and Shawn, This patch was made by Peng. Peng, could you explain about this? My guess is that the patch is to eliminate the unnecessary processing. Best regards, Kohji Okuno Shawn Guo <shawnguo@kernel.org> wrote: > On Wed, Mar 06, 2019 at 01:30:42PM +0900, Kohji Okuno wrote: >> In imx6_set_lpm, we only need to unmask GINT when not WAIT_CLOCKED, >> so add a check condition. > > Can you elaborate the problem we have without this code change? > > Shawn > >> >> Signed-off-by: Peng Fan <peng.fan@nxp.com> >> Signed-off-by: Kohji Okuno <okuno.kohji@jp.panasonic.com> >> --- >> arch/arm/mach-imx/pm-imx6.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm/mach-imx/pm-imx6.c b/arch/arm/mach-imx/pm-imx6.c >> index 87f45b926c78..54add0178b96 100644 >> --- a/arch/arm/mach-imx/pm-imx6.c >> +++ b/arch/arm/mach-imx/pm-imx6.c >> @@ -354,9 +354,11 @@ int imx6_set_lpm(enum mxc_cpu_pwr_mode mode) >> * >> * Note that IRQ #32 is GIC SPI #0. >> */ >> - imx_gpc_hwirq_unmask(0); >> + if (mode != WAIT_CLOCKED) >> + imx_gpc_hwirq_unmask(0); >> writel_relaxed(val, ccm_base + CLPCR); >> - imx_gpc_hwirq_mask(0); >> + if (mode != WAIT_CLOCKED) >> + imx_gpc_hwirq_mask(0); >> >> return 0; >> } >> -- >> 2.17.1 >>
> -----Original Message----- > From: Kohji Okuno [mailto:okuno.kohji@jp.panasonic.com] > Sent: 2019年3月20日 8:07 > To: shawnguo@kernel.org; Peng Fan <peng.fan@nxp.com> > Cc: linux-arm-kernel@lists.infradead.org; s.hauer@pengutronix.de; > kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx > <linux-imx@nxp.com>; Peng Fan <peng.fan@nxp.com>; > okuno.kohji@jp.panasonic.com > Subject: Re: [PATCH] ARM: imx6: cpuidle: omit the unnecessary unmask of > GINT > > Hi Peng and Shawn, > > This patch was made by Peng. Peng, could you explain about this? > My guess is that the patch is to eliminate the unnecessary processing. Yes. This is optimization, there is no need to unmask/mask when WAIT_CLOCKED. See the errata description. /* * ERR007265: CCM: When improper low-power sequence is used, * the SoC enters low power mode before the ARM core executes WFI. * * Software workaround: * 1) Software should trigger IRQ #32 (IOMUX) to be always pending * by setting IOMUX_GPR1_GINT. * 2) Software should then unmask IRQ #32 in GPC before setting CCM * Low-Power mode. * 3) Software should mask IRQ #32 right after CCM Low-Power mode * is set (set bits 0-1 of CCM_CLPCR). * * Note that IRQ #32 is GIC SPI #0. */ Regards, Peng. > > Best regards, > Kohji Okuno > > Shawn Guo <shawnguo@kernel.org> wrote: > > On Wed, Mar 06, 2019 at 01:30:42PM +0900, Kohji Okuno wrote: > >> In imx6_set_lpm, we only need to unmask GINT when not > WAIT_CLOCKED, > >> so add a check condition. > > > > Can you elaborate the problem we have without this code change? > > > > Shawn > > > >> > >> Signed-off-by: Peng Fan <peng.fan@nxp.com> > >> Signed-off-by: Kohji Okuno <okuno.kohji@jp.panasonic.com> > >> --- > >> arch/arm/mach-imx/pm-imx6.c | 6 ++++-- > >> 1 file changed, 4 insertions(+), 2 deletions(-) > >> > >> diff --git a/arch/arm/mach-imx/pm-imx6.c > >> b/arch/arm/mach-imx/pm-imx6.c index 87f45b926c78..54add0178b96 > 100644 > >> --- a/arch/arm/mach-imx/pm-imx6.c > >> +++ b/arch/arm/mach-imx/pm-imx6.c > >> @@ -354,9 +354,11 @@ int imx6_set_lpm(enum mxc_cpu_pwr_mode > mode) > >> * > >> * Note that IRQ #32 is GIC SPI #0. > >> */ > >> - imx_gpc_hwirq_unmask(0); > >> + if (mode != WAIT_CLOCKED) > >> + imx_gpc_hwirq_unmask(0); > >> writel_relaxed(val, ccm_base + CLPCR); > >> - imx_gpc_hwirq_mask(0); > >> + if (mode != WAIT_CLOCKED) > >> + imx_gpc_hwirq_mask(0); > >> > >> return 0; > >> } > >> -- > >> 2.17.1 > >>
> From: Peng Fan > > -----Original Message----- > > From: Kohji Okuno [mailto:okuno.kohji@jp.panasonic.com] > > Subject: Re: [PATCH] ARM: imx6: cpuidle: omit the unnecessary unmask > > of GINT > > > > Hi Peng and Shawn, > > > > This patch was made by Peng. Peng, could you explain about this? > > My guess is that the patch is to eliminate the unnecessary processing. > > Yes. This is optimization, there is no need to unmask/mask when > WAIT_CLOCKED. See the errata description. > /* > * ERR007265: CCM: When improper low-power sequence is used, > * the SoC enters low power mode before the ARM core executes WFI. > * > * Software workaround: > * 1) Software should trigger IRQ #32 (IOMUX) to be always pending > * by setting IOMUX_GPR1_GINT. > * 2) Software should then unmask IRQ #32 in GPC before setting CCM > * Low-Power mode. > * 3) Software should mask IRQ #32 right after CCM Low-Power mode > * is set (set bits 0-1 of CCM_CLPCR). > * > * Note that IRQ #32 is GIC SPI #0. > */ Your code comments did not mention it's only for WAIT_CLOCKED. For errata, I saw it seems also affect STOP mode. Can you explain a bit more on why it's safe for all other modes except WAIT_CLOCKED? Regards Dong Aisheng > Regards, > Peng. > > > > > Best regards, > > Kohji Okuno > > > > Shawn Guo <shawnguo@kernel.org> wrote: > > > On Wed, Mar 06, 2019 at 01:30:42PM +0900, Kohji Okuno wrote: > > >> In imx6_set_lpm, we only need to unmask GINT when not > > WAIT_CLOCKED, > > >> so add a check condition. > > > > > > Can you elaborate the problem we have without this code change? > > > > > > Shawn > > > > > >> > > >> Signed-off-by: Peng Fan <peng.fan@nxp.com> > > >> Signed-off-by: Kohji Okuno <okuno.kohji@jp.panasonic.com> > > >> --- > > >> arch/arm/mach-imx/pm-imx6.c | 6 ++++-- > > >> 1 file changed, 4 insertions(+), 2 deletions(-) > > >> > > >> diff --git a/arch/arm/mach-imx/pm-imx6.c > > >> b/arch/arm/mach-imx/pm-imx6.c index 87f45b926c78..54add0178b96 > > 100644 > > >> --- a/arch/arm/mach-imx/pm-imx6.c > > >> +++ b/arch/arm/mach-imx/pm-imx6.c > > >> @@ -354,9 +354,11 @@ int imx6_set_lpm(enum mxc_cpu_pwr_mode > > mode) > > >> * > > >> * Note that IRQ #32 is GIC SPI #0. > > >> */ > > >> - imx_gpc_hwirq_unmask(0); > > >> + if (mode != WAIT_CLOCKED) > > >> + imx_gpc_hwirq_unmask(0); > > >> writel_relaxed(val, ccm_base + CLPCR); > > >> - imx_gpc_hwirq_mask(0); > > >> + if (mode != WAIT_CLOCKED) > > >> + imx_gpc_hwirq_mask(0); > > >> > > >> return 0; > > >> } > > >> -- > > >> 2.17.1 > > >>
On Wed, Mar 20, 2019 at 01:12:01AM +0000, Peng Fan wrote: > > > > -----Original Message----- > > From: Kohji Okuno [mailto:okuno.kohji@jp.panasonic.com] > > Sent: 2019年3月20日 8:07 > > To: shawnguo@kernel.org; Peng Fan <peng.fan@nxp.com> > > Cc: linux-arm-kernel@lists.infradead.org; s.hauer@pengutronix.de; > > kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx > > <linux-imx@nxp.com>; Peng Fan <peng.fan@nxp.com>; > > okuno.kohji@jp.panasonic.com > > Subject: Re: [PATCH] ARM: imx6: cpuidle: omit the unnecessary unmask of > > GINT > > > > Hi Peng and Shawn, > > > > This patch was made by Peng. Peng, could you explain about this? > > My guess is that the patch is to eliminate the unnecessary processing. > > Yes. This is optimization, there is no need to unmask/mask when > WAIT_CLOCKED. See the errata description. > /* > * ERR007265: CCM: When improper low-power sequence is used, > * the SoC enters low power mode before the ARM core executes WFI. > * > * Software workaround: > * 1) Software should trigger IRQ #32 (IOMUX) to be always pending > * by setting IOMUX_GPR1_GINT. > * 2) Software should then unmask IRQ #32 in GPC before setting CCM > * Low-Power mode. > * 3) Software should mask IRQ #32 right after CCM Low-Power mode > * is set (set bits 0-1 of CCM_CLPCR). > * > * Note that IRQ #32 is GIC SPI #0. > */ So are you saying the workaround of ERR007265 does not need to apply on WAIT_CLOCKED mode? But WAIT_CLOCKED is one of low-power modes, isn't it? Shawn
Hi Shawn, > -----Original Message----- > From: Shawn Guo [mailto:shawnguo@kernel.org] > Sent: 2019年3月20日 15:45 > To: Peng Fan <peng.fan@nxp.com> > Cc: Kohji Okuno <okuno.kohji@jp.panasonic.com>; > linux-arm-kernel@lists.infradead.org; s.hauer@pengutronix.de; > kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx > <linux-imx@nxp.com> > Subject: Re: [PATCH] ARM: imx6: cpuidle: omit the unnecessary unmask of > GINT > > On Wed, Mar 20, 2019 at 01:12:01AM +0000, Peng Fan wrote: > > > > > > > -----Original Message----- > > > From: Kohji Okuno [mailto:okuno.kohji@jp.panasonic.com] > > > Sent: 2019年3月20日 8:07 > > > To: shawnguo@kernel.org; Peng Fan <peng.fan@nxp.com> > > > Cc: linux-arm-kernel@lists.infradead.org; s.hauer@pengutronix.de; > > > kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx > > > <linux-imx@nxp.com>; Peng Fan <peng.fan@nxp.com>; > > > okuno.kohji@jp.panasonic.com > > > Subject: Re: [PATCH] ARM: imx6: cpuidle: omit the unnecessary unmask > > > of GINT > > > > > > Hi Peng and Shawn, > > > > > > This patch was made by Peng. Peng, could you explain about this? > > > My guess is that the patch is to eliminate the unnecessary processing. > > > > Yes. This is optimization, there is no need to unmask/mask when > > WAIT_CLOCKED. See the errata description. > > /* > > * ERR007265: CCM: When improper low-power sequence is used, > > * the SoC enters low power mode before the ARM core executes WFI. > > * > > * Software workaround: > > * 1) Software should trigger IRQ #32 (IOMUX) to be always pending > > * by setting IOMUX_GPR1_GINT. > > * 2) Software should then unmask IRQ #32 in GPC before setting CCM > > * Low-Power mode. > > * 3) Software should mask IRQ #32 right after CCM Low-Power mode > > * is set (set bits 0-1 of CCM_CLPCR). > > * > > * Note that IRQ #32 is GIC SPI #0. > > */ > > So are you saying the workaround of ERR007265 does not need to apply on > WAIT_CLOCKED mode? But WAIT_CLOCKED is one of low-power modes, > isn't it? If understand correct, I think WAIT_CLOCKED is for run mode, not wait/stop mode. According to i.MX6Q RM, "18.6.18 CCM Low Power Control Register (CCM_CLPCR)" Setting the low power mode that system will enter on next assertion of dsm_request signal. NOTE: Set CCM_CGPR[INT_MEM_CLK_LPM] and CCM_CGPR[1] bits to 1 when setting CCM_CLPCR[LPM] bits to 01 (WAIT Mode) or 10 (STOP mode) without power gating. CCM_CGPR[INT_MEM_CLK_LPM] and CCM_CGPR[1] bits do not have to be set for STOP mode entry. 00 Remain in run mode 01 Transfer to wait mode 10 Transfer to stop mode 11 Reserved And according to the function: int imx6q_set_lpm(enum mxc_cpu_pwr_mode mode) { u32 val = readl_relaxed(ccm_base + CLPCR); val &= ~BM_CLPCR_LPM; switch (mode) { case WAIT_CLOCKED: break; WAIT_CLOCKED will clear the LPM bits. According to https://www.nxp.com/docs/en/errata/IMX6DQCE.pdf " ERR007265 CCM: When improper low-power sequence is used, the SoC enters low power mode before the ARM core executes WFI When software tries to enter Low-Power mode with the following sequence, the SoC enters Low-Power mode before the ARM core executes the WFI instruction: 1. Set CCM_CLPCR[1:0] to 2’b00 2. ARM core enters WFI 3. ARM core wakeup from an interrupt event, which is masked by GPC or not visible to GPC, such as an interrupt from a local timer 4. Set CCM_CLPCR[1:0] to 2’b01 or 2’b10 5. ARM core executes WFI Before the last step, the SoC enters WAIT mode if CCM_CLPCR[1:0] is set to 2’b01, or STOP mode if CCM_CLPCR[1:0] is set to 2’b10. " It mentions that soc will enter wait/stop mode early before wfi if set CCM_CLPCR to 1/2. But WAIT_CLOCKED is not wait mode or stop mode, it will set LPM to run mode. Regards, Peng. > > Shawn
Hi Peng, On Wed, Mar 20, 2019 at 07:59:19AM +0000, Peng Fan wrote: > > So are you saying the workaround of ERR007265 does not need to apply on > > WAIT_CLOCKED mode? But WAIT_CLOCKED is one of low-power modes, > > isn't it? > > If understand correct, I think WAIT_CLOCKED is for run mode, not wait/stop mode. > > According to i.MX6Q RM, "18.6.18 CCM Low Power Control Register (CCM_CLPCR)" > > Setting the low power mode that system will enter on next assertion of dsm_request signal. > NOTE: Set CCM_CGPR[INT_MEM_CLK_LPM] and CCM_CGPR[1] bits to 1 when setting > CCM_CLPCR[LPM] bits to 01 (WAIT Mode) or 10 (STOP mode) without power gating. > CCM_CGPR[INT_MEM_CLK_LPM] and CCM_CGPR[1] bits do not have to be set for STOP > mode entry. > 00 Remain in run mode > 01 Transfer to wait mode > 10 Transfer to stop mode > 11 Reserved > > And according to the function: > int imx6q_set_lpm(enum mxc_cpu_pwr_mode mode) > { > u32 val = readl_relaxed(ccm_base + CLPCR); > > val &= ~BM_CLPCR_LPM; > switch (mode) { > case WAIT_CLOCKED: > break; > > WAIT_CLOCKED will clear the LPM bits. > > > According to https://www.nxp.com/docs/en/errata/IMX6DQCE.pdf > " > ERR007265 CCM: When improper low-power sequence is used, the SoC enters > low power mode before the ARM core executes WFI > > When software tries to enter Low-Power mode with the following sequence, the SoC enters > Low-Power mode before the ARM core executes the WFI instruction: > 1. Set CCM_CLPCR[1:0] to 2’b00 > 2. ARM core enters WFI > 3. ARM core wakeup from an interrupt event, which is masked by GPC or not visible to GPC, > such as an interrupt from a local timer > 4. Set CCM_CLPCR[1:0] to 2’b01 or 2’b10 > 5. ARM core executes WFI > Before the last step, the SoC enters WAIT mode if CCM_CLPCR[1:0] is set to 2’b01, or STOP > mode if CCM_CLPCR[1:0] is set to 2’b10. > " > > It mentions that soc will enter wait/stop mode early before wfi if set CCM_CLPCR to 1/2. > But WAIT_CLOCKED is not wait mode or stop mode, it will set LPM to run mode. Thanks much for the detailed explanation. I messed WAIT_CLOCKED with WAIT_UNCLOCKED. Yes, I agree it's an optimization that we skip applying the workaround to WAIT_CLOCKED. Shawn
On Wed, Mar 06, 2019 at 01:30:42PM +0900, Kohji Okuno wrote: > In imx6_set_lpm, we only need to unmask GINT when not WAIT_CLOCKED, > so add a check condition. > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > Signed-off-by: Kohji Okuno <okuno.kohji@jp.panasonic.com> Applied, thanks.
diff --git a/arch/arm/mach-imx/pm-imx6.c b/arch/arm/mach-imx/pm-imx6.c index 87f45b926c78..54add0178b96 100644 --- a/arch/arm/mach-imx/pm-imx6.c +++ b/arch/arm/mach-imx/pm-imx6.c @@ -354,9 +354,11 @@ int imx6_set_lpm(enum mxc_cpu_pwr_mode mode) * * Note that IRQ #32 is GIC SPI #0. */ - imx_gpc_hwirq_unmask(0); + if (mode != WAIT_CLOCKED) + imx_gpc_hwirq_unmask(0); writel_relaxed(val, ccm_base + CLPCR); - imx_gpc_hwirq_mask(0); + if (mode != WAIT_CLOCKED) + imx_gpc_hwirq_mask(0); return 0; }