| Message ID | 20180102164223.15230-1-stefan@agner.ch |
|---|---|
| State | New |
| Headers | show |
| Series | [1/7] ARM: imx: add timer stop flag to ARM power off state | expand |
On Tue, Jan 02, 2018 at 05:42:18PM +0100, Stefan Agner wrote: > From: Fugang Duan <fugang.duan@nxp.com> > > Update i.MX 6UltraLite IOMUXC pin defines. That's obvious reading the diff. The commit message should tell me why. They were wrong? > > Signed-off-by: Fugang Duan <fugang.duan@nxp.com> > Signed-off-by: Stefan Agner <stefan@agner.ch>
On Tue, Jan 02, 2018 at 05:42:19PM +0100, Stefan Agner wrote: > From: Bai Ping <ping.bai@nxp.com> > > On i.MX 6ULL, the pin MUX and CTRL register of BOOT_MODEx and TAMPERx > pins are available through IOMUXC_SNVS. Add additional pinfunc defines. > > Signed-off-by: Bai Ping <ping.bai@nxp.com> > Signed-off-by: Stefan Agner <stefan@agner.ch> > --- > arch/arm/boot/dts/imx6ull-pinfunc-snvs.h | 29 +++++++++++++++++++++++++++++ > arch/arm/boot/dts/imx6ull.dtsi | 1 + > 2 files changed, 30 insertions(+) > create mode 100644 arch/arm/boot/dts/imx6ull-pinfunc-snvs.h > > diff --git a/arch/arm/boot/dts/imx6ull-pinfunc-snvs.h b/arch/arm/boot/dts/imx6ull-pinfunc-snvs.h > new file mode 100644 > index 000000000000..da3f412e4269 > --- /dev/null > +++ b/arch/arm/boot/dts/imx6ull-pinfunc-snvs.h > @@ -0,0 +1,29 @@ > +/* > + * Copyright (C) 2016 Freescale Semiconductor, Inc. It's 2018 now. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. Use SPDX. With that, Reviewed-by: Rob Herring <robh@kernel.org>
On 2018-01-05 17:45, Rob Herring wrote: > On Tue, Jan 02, 2018 at 05:42:18PM +0100, Stefan Agner wrote: >> From: Fugang Duan <fugang.duan@nxp.com> >> >> Update i.MX 6UltraLite IOMUXC pin defines. > > That's obvious reading the diff. The commit message should tell me why. > They were wrong? I guess pretty much :-) What I can tell from the change itself it seems that daisy chain configurations were missing. And some additional pinmux options. Will update the commit message accordingly. -- Stefan > >> >> Signed-off-by: Fugang Duan <fugang.duan@nxp.com> >> Signed-off-by: Stefan Agner <stefan@agner.ch>
On 2018-01-05 17:49, Rob Herring wrote: > On Tue, Jan 02, 2018 at 05:42:19PM +0100, Stefan Agner wrote: >> From: Bai Ping <ping.bai@nxp.com> >> >> On i.MX 6ULL, the pin MUX and CTRL register of BOOT_MODEx and TAMPERx >> pins are available through IOMUXC_SNVS. Add additional pinfunc defines. >> >> Signed-off-by: Bai Ping <ping.bai@nxp.com> >> Signed-off-by: Stefan Agner <stefan@agner.ch> >> --- >> arch/arm/boot/dts/imx6ull-pinfunc-snvs.h | 29 +++++++++++++++++++++++++++++ >> arch/arm/boot/dts/imx6ull.dtsi | 1 + >> 2 files changed, 30 insertions(+) >> create mode 100644 arch/arm/boot/dts/imx6ull-pinfunc-snvs.h >> >> diff --git a/arch/arm/boot/dts/imx6ull-pinfunc-snvs.h b/arch/arm/boot/dts/imx6ull-pinfunc-snvs.h >> new file mode 100644 >> index 000000000000..da3f412e4269 >> --- /dev/null >> +++ b/arch/arm/boot/dts/imx6ull-pinfunc-snvs.h >> @@ -0,0 +1,29 @@ >> +/* >> + * Copyright (C) 2016 Freescale Semiconductor, Inc. > > It's 2018 now. > I don't think you are supposed to chance copyright year unless you change it significantly. At least that article suggests so: https://www.copyrightlaws.com/copyright-notice-year/ I took that patch from the downstream NXP kernel, so I guess 2016 was the year of first publication... >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. > > Use SPDX. With that, Agreed. > > Reviewed-by: Rob Herring <robh@kernel.org> -- Stefan
From: Rob Herring <robh@kernel.org> Sent: Saturday, January 06, 2018 12:45 AM >To: Stefan Agner <stefan@agner.ch> >Cc: shawnguo@kernel.org; kernel@pengutronix.de; Fabio Estevam ><fabio.estevam@nxp.com>; mark.rutland@arm.com; linux-arm- >kernel@lists.infradead.org; devicetree@vger.kernel.org; linux- >kernel@vger.kernel.org; Andy Duan <fugang.duan@nxp.com> >Subject: Re: [PATCH 2/7] ARM: dts: imx6ul: update i.MX 6UltraLite iomux >headers > >On Tue, Jan 02, 2018 at 05:42:18PM +0100, Stefan Agner wrote: >> From: Fugang Duan <fugang.duan@nxp.com> >> >> Update i.MX 6UltraLite IOMUXC pin defines. > >That's obvious reading the diff. The commit message should tell me why. >They were wrong? > Yes, the previous iomux header parts of pin setting were wrong, which was generated by IOMUX tool during SOC bringup. The updated header correct the wrong pin setting. >> >> Signed-off-by: Fugang Duan <fugang.duan@nxp.com> >> Signed-off-by: Stefan Agner <stefan@agner.ch>
On Tue, Jan 02, 2018 at 05:42:17PM +0100, Stefan Agner wrote: > When the CPU is in ARM power off state the ARM architected > timers are stopped. The flag is already present in the higher > power WAIT mode. > > This allows to use the ARM generic timer on i.MX 6UL/6ULL SoC. > Without the flag the kernel freezes when the timer enters the > first time ARM power off mode. > > Cc: Anson Huang <anson.huang@nxp.com> > Signed-off-by: Stefan Agner <stefan@agner.ch> It seems ok at my side. Did you meet the real issue? If yes, how to reproduce? Both mx6sx and mx6ul are using GPT which do not need that flag, suppose we should remove it, right? Anson can help confirm it. Regards Dong Aisheng > --- > arch/arm/mach-imx/cpuidle-imx6sx.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/arm/mach-imx/cpuidle-imx6sx.c b/arch/arm/mach-imx/cpuidle-imx6sx.c > index c5a5c3a70ab1..d0f14b761ff7 100644 > --- a/arch/arm/mach-imx/cpuidle-imx6sx.c > +++ b/arch/arm/mach-imx/cpuidle-imx6sx.c > @@ -89,6 +89,7 @@ static struct cpuidle_driver imx6sx_cpuidle_driver = { > */ > .exit_latency = 300, > .target_residency = 500, > + .flags = CPUIDLE_FLAG_TIMER_STOP, > .enter = imx6sx_enter_wait, > .name = "LOW-POWER-IDLE", > .desc = "ARM power off", > -- > 2.15.1 >
On Tue, Jan 02, 2018 at 05:42:18PM +0100, Stefan Agner wrote: > From: Fugang Duan <fugang.duan@nxp.com> > > Update i.MX 6UltraLite IOMUXC pin defines. > > Signed-off-by: Fugang Duan <fugang.duan@nxp.com> > Signed-off-by: Stefan Agner <stefan@agner.ch> This is really hard to review. You probably need at least tell how you generated this file. e.g. updated against nxp release xxx. Regards Dong Aisheng
Best Regards! Anson Huang > -----Original Message----- > From: Dong Aisheng [mailto:dongas86@gmail.com] > Sent: 2018-01-09 5:23 PM > To: Stefan Agner <stefan@agner.ch> > Cc: shawnguo@kernel.org; kernel@pengutronix.de; Fabio Estevam > <fabio.estevam@nxp.com>; robh+dt@kernel.org; mark.rutland@arm.com; > linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; linux- > kernel@vger.kernel.org; Anson Huang <anson.huang@nxp.com>; dl-linux-imx > <linux-imx@nxp.com> > Subject: Re: [PATCH 1/7] ARM: imx: add timer stop flag to ARM power off state > > On Tue, Jan 02, 2018 at 05:42:17PM +0100, Stefan Agner wrote: > > When the CPU is in ARM power off state the ARM architected timers are > > stopped. The flag is already present in the higher power WAIT mode. > > > > This allows to use the ARM generic timer on i.MX 6UL/6ULL SoC. > > Without the flag the kernel freezes when the timer enters the first > > time ARM power off mode. > > > > Cc: Anson Huang <anson.huang@nxp.com> > > Signed-off-by: Stefan Agner <stefan@agner.ch> > > It seems ok at my side. > Did you meet the real issue? If yes, how to reproduce? > > Both mx6sx and mx6ul are using GPT which do not need that flag, suppose we > should remove it, right? > Anson can help confirm it. For UP system like i.MX6SX, we do NOT enable "cortex-a9-twd-timer", so local timer is NOT used, GPT is used instead, GPT's clock is NOT disabled when cpuidle, so I think we should remove all these Timer stop flag for 6SX CPUIDLE. Anson. > > Regards > Dong Aisheng > > > --- > > arch/arm/mach-imx/cpuidle-imx6sx.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/arch/arm/mach-imx/cpuidle-imx6sx.c > > b/arch/arm/mach-imx/cpuidle-imx6sx.c > > index c5a5c3a70ab1..d0f14b761ff7 100644 > > --- a/arch/arm/mach-imx/cpuidle-imx6sx.c > > +++ b/arch/arm/mach-imx/cpuidle-imx6sx.c > > @@ -89,6 +89,7 @@ static struct cpuidle_driver imx6sx_cpuidle_driver = { > > */ > > .exit_latency = 300, > > .target_residency = 500, > > + .flags = CPUIDLE_FLAG_TIMER_STOP, > > .enter = imx6sx_enter_wait, > > .name = "LOW-POWER-IDLE", > > .desc = "ARM power off", > > -- > > 2.15.1 > >
On Sun, Jan 07, 2018 at 10:52:39AM +0100, Stefan Agner wrote: > On 2018-01-05 17:49, Rob Herring wrote: > > On Tue, Jan 02, 2018 at 05:42:19PM +0100, Stefan Agner wrote: > >> From: Bai Ping <ping.bai@nxp.com> > >> > >> On i.MX 6ULL, the pin MUX and CTRL register of BOOT_MODEx and TAMPERx > >> pins are available through IOMUXC_SNVS. Add additional pinfunc defines. > >> > >> Signed-off-by: Bai Ping <ping.bai@nxp.com> > >> Signed-off-by: Stefan Agner <stefan@agner.ch> > >> --- > >> arch/arm/boot/dts/imx6ull-pinfunc-snvs.h | 29 +++++++++++++++++++++++++++++ > >> arch/arm/boot/dts/imx6ull.dtsi | 1 + > >> 2 files changed, 30 insertions(+) > >> create mode 100644 arch/arm/boot/dts/imx6ull-pinfunc-snvs.h > >> > >> diff --git a/arch/arm/boot/dts/imx6ull-pinfunc-snvs.h b/arch/arm/boot/dts/imx6ull-pinfunc-snvs.h > >> new file mode 100644 > >> index 000000000000..da3f412e4269 > >> --- /dev/null > >> +++ b/arch/arm/boot/dts/imx6ull-pinfunc-snvs.h > >> @@ -0,0 +1,29 @@ > >> +/* > >> + * Copyright (C) 2016 Freescale Semiconductor, Inc. > > > > It's 2018 now. > > > > I don't think you are supposed to chance copyright year unless you > change it significantly. > > At least that article suggests so: > https://www.copyrightlaws.com/copyright-notice-year/ > > I took that patch from the downstream NXP kernel, so I guess 2016 was > the year of first publication... > Can you help add below copyright if you keep NXP sign-off as Author? Then you don't need delete the old one. Copyright 2017 NXP. And SPDX mentioned by Rob. Otherwise, Acked-by: Dong Aisheng <aisheng.dong@nxp.com> Regards Dong Aisheng > >> + * > >> + * This program is free software; you can redistribute it and/or modify > >> + * it under the terms of the GNU General Public License version 2 as > >> + * published by the Free Software Foundation. > > > > Use SPDX. With that, > > Agreed. > > > > > Reviewed-by: Rob Herring <robh@kernel.org> > > -- > Stefan
Hi Stefan, On Tue, Jan 02, 2018 at 05:42:21PM +0100, Stefan Agner wrote: > Add per-core ARM architected timer. Unfortunately bootloaders (U-Boot) > currently do not make the necessary initialization. Also specifing the > clock manually using the clock-frequency property seems not to help. > Therefor leave the timer disabled by default for now. > > Signed-off-by: Stefan Agner <stefan@agner.ch> Any special purpose to use arch timer? Regards Dong Aisheng > --- > arch/arm/boot/dts/imx6ul.dtsi | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/arch/arm/boot/dts/imx6ul.dtsi b/arch/arm/boot/dts/imx6ul.dtsi > index 993fbdbdd506..4d76923e8f44 100644 > --- a/arch/arm/boot/dts/imx6ul.dtsi > +++ b/arch/arm/boot/dts/imx6ul.dtsi > @@ -110,6 +110,16 @@ > <0x00a06000 0x2000>; > }; > > + timer { > + compatible = "arm,armv7-timer"; > + interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>, > + <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>, > + <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>, > + <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>; > + interrupt-parent = <&intc>; > + status = "disabled"; > + }; > + > ckil: clock-cli { > compatible = "fixed-clock"; > #clock-cells = <0>; > -- > 2.15.1 >
On Tue, Jan 02, 2018 at 05:42:22PM +0100, Stefan Agner wrote: > The i.MX 6ULL features another IOMUX Controller called IOMUXC > SNVS which allows to control BOOT_MODE and TAMPER pins. Add the > controller to the i.MX 6ULL specific imx6ull.dtsi device tree. > > Signed-off-by: Stefan Agner <stefan@agner.ch> Looks good. Acked-by: Dong Aisheng <aisheng.dong@nxp.com> Regards Dong Aisheng
On Tue, Jan 02, 2018 at 05:42:23PM +0100, Stefan Agner wrote: > In i.MX 6ULL UART8 is part of the AIPS-3 memory map instead of > AIPS-1. Clocks and interrupts remain the same. > > Signed-off-by: Stefan Agner <stefan@agner.ch> Acked-by: Dong Aisheng <aisheng.dong@nxp.com> Regards Dong Aisheng > --- > arch/arm/boot/dts/imx6ull.dtsi | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/arch/arm/boot/dts/imx6ull.dtsi b/arch/arm/boot/dts/imx6ull.dtsi > index bc2cd4fb8b12..571ddd71cdba 100644 > --- a/arch/arm/boot/dts/imx6ull.dtsi > +++ b/arch/arm/boot/dts/imx6ull.dtsi > @@ -43,6 +43,9 @@ > #include "imx6ull-pinfunc.h" > #include "imx6ull-pinfunc-snvs.h" > > +/* Delete UART8 in AIPS-1 (i.MX6UL specific) */ > +/delete-node/ &uart8; > + > / { > soc { > aips3: aips-bus@2200000 { > @@ -56,6 +59,17 @@ > compatible = "fsl,imx6ull-iomuxc-snvs"; > reg = <0x02290000 0x4000>; > }; > + > + uart8: serial@2288000 { > + compatible = "fsl,imx6ul-uart", > + "fsl,imx6q-uart"; > + reg = <0x02288000 0x4000>; > + interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&clks IMX6UL_CLK_UART8_IPG>, > + <&clks IMX6UL_CLK_UART8_SERIAL>; > + clock-names = "ipg", "per"; > + status = "disabled"; > + }; > }; > }; > }; > -- > 2.15.1 >
Am Dienstag, den 09.01.2018, 09:25 +0000 schrieb Anson Huang: > > Best Regards! > Anson Huang > > > > -----Original Message----- > > From: Dong Aisheng [mailto:dongas86@gmail.com] > > Sent: 2018-01-09 5:23 PM > > To: Stefan Agner <stefan@agner.ch> > > Cc: shawnguo@kernel.org; kernel@pengutronix.de; Fabio Estevam > > <fabio.estevam@nxp.com>; robh+dt@kernel.org; mark.rutland@arm.com; > > linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; > > linux- > > kernel@vger.kernel.org; Anson Huang <anson.huang@nxp.com>; dl- > > linux-imx > > <linux-imx@nxp.com> > > Subject: Re: [PATCH 1/7] ARM: imx: add timer stop flag to ARM power > > off state > > > > On Tue, Jan 02, 2018 at 05:42:17PM +0100, Stefan Agner wrote: > > > When the CPU is in ARM power off state the ARM architected timers > > > are > > > stopped. The flag is already present in the higher power WAIT > > > mode. > > > > > > This allows to use the ARM generic timer on i.MX 6UL/6ULL SoC. > > > Without the flag the kernel freezes when the timer enters the > > > first > > > time ARM power off mode. > > > > > > Cc: Anson Huang <anson.huang@nxp.com> > > > Signed-off-by: Stefan Agner <stefan@agner.ch> > > > > It seems ok at my side. > > Did you meet the real issue? If yes, how to reproduce? > > > > Both mx6sx and mx6ul are using GPT which do not need that flag, > > suppose we > > should remove it, right? > > Anson can help confirm it. > > For UP system like i.MX6SX, we do NOT enable "cortex-a9-twd-timer", > so local > timer is NOT used, GPT is used instead, GPT's clock is NOT disabled > when cpuidle, > so I think we should remove all these Timer stop flag for 6SX > CPUIDLE. It's correct to set the flag even on UP systems, as the flag means the CPU _local_ timer is stopped in this sleep mode. Also there are systems out there which are using the TWD on UP, as it operates at a higher frequency leading to better wakeup granularity. Regards, Lucas
On 2018-01-09 10:34, Dong Aisheng wrote: > Hi Stefan, > > On Tue, Jan 02, 2018 at 05:42:21PM +0100, Stefan Agner wrote: >> Add per-core ARM architected timer. Unfortunately bootloaders (U-Boot) >> currently do not make the necessary initialization. Also specifing the >> clock manually using the clock-frequency property seems not to help. >> Therefor leave the timer disabled by default for now. >> >> Signed-off-by: Stefan Agner <stefan@agner.ch> > > Any special purpose to use arch timer? > It is the better option. It supports virtualization and allows direct user space access, e.g. as used in OpenSSL through _armv7_tick. -- Stefan >> --- >> arch/arm/boot/dts/imx6ul.dtsi | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/arch/arm/boot/dts/imx6ul.dtsi b/arch/arm/boot/dts/imx6ul.dtsi >> index 993fbdbdd506..4d76923e8f44 100644 >> --- a/arch/arm/boot/dts/imx6ul.dtsi >> +++ b/arch/arm/boot/dts/imx6ul.dtsi >> @@ -110,6 +110,16 @@ >> <0x00a06000 0x2000>; >> }; >> >> + timer { >> + compatible = "arm,armv7-timer"; >> + interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>, >> + <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>, >> + <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>, >> + <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>; >> + interrupt-parent = <&intc>; >> + status = "disabled"; >> + }; >> + >> ckil: clock-cli { >> compatible = "fixed-clock"; >> #clock-cells = <0>; >> -- >> 2.15.1 >>
On 2018-01-09 10:22, Dong Aisheng wrote: > On Tue, Jan 02, 2018 at 05:42:17PM +0100, Stefan Agner wrote: >> When the CPU is in ARM power off state the ARM architected >> timers are stopped. The flag is already present in the higher >> power WAIT mode. >> >> This allows to use the ARM generic timer on i.MX 6UL/6ULL SoC. >> Without the flag the kernel freezes when the timer enters the >> first time ARM power off mode. >> >> Cc: Anson Huang <anson.huang@nxp.com> >> Signed-off-by: Stefan Agner <stefan@agner.ch> > > It seems ok at my side. > Did you meet the real issue? If yes, how to reproduce? Enable the timer added with Patch 5, use a U-Boot with this patchset applied: https://www.mail-archive.com/u-boot@lists.denx.de/msg273287.html And boot... For me it freezed somewhere early during systemd boot phase, presumably the first time the CPU got into this idle mode. -- Stefan > > Both mx6sx and mx6ul are using GPT which do not need that flag, suppose > we should remove it, right? > Anson can help confirm it. > > Regards > Dong Aisheng > >> --- >> arch/arm/mach-imx/cpuidle-imx6sx.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/arch/arm/mach-imx/cpuidle-imx6sx.c b/arch/arm/mach-imx/cpuidle-imx6sx.c >> index c5a5c3a70ab1..d0f14b761ff7 100644 >> --- a/arch/arm/mach-imx/cpuidle-imx6sx.c >> +++ b/arch/arm/mach-imx/cpuidle-imx6sx.c >> @@ -89,6 +89,7 @@ static struct cpuidle_driver imx6sx_cpuidle_driver = { >> */ >> .exit_latency = 300, >> .target_residency = 500, >> + .flags = CPUIDLE_FLAG_TIMER_STOP, >> .enter = imx6sx_enter_wait, >> .name = "LOW-POWER-IDLE", >> .desc = "ARM power off", >> -- >> 2.15.1 >>
On 2018-01-09 11:13, Lucas Stach wrote: > Am Dienstag, den 09.01.2018, 09:25 +0000 schrieb Anson Huang: >> >> Best Regards! >> Anson Huang >> >> >> > -----Original Message----- >> > From: Dong Aisheng [mailto:dongas86@gmail.com] >> > Sent: 2018-01-09 5:23 PM >> > To: Stefan Agner <stefan@agner.ch> >> > Cc: shawnguo@kernel.org; kernel@pengutronix.de; Fabio Estevam >> > <fabio.estevam@nxp.com>; robh+dt@kernel.org; mark.rutland@arm.com; >> > linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; >> > linux- >> > kernel@vger.kernel.org; Anson Huang <anson.huang@nxp.com>; dl- >> > linux-imx >> > <linux-imx@nxp.com> >> > Subject: Re: [PATCH 1/7] ARM: imx: add timer stop flag to ARM power >> > off state >> > >> > On Tue, Jan 02, 2018 at 05:42:17PM +0100, Stefan Agner wrote: >> > > When the CPU is in ARM power off state the ARM architected timers >> > > are >> > > stopped. The flag is already present in the higher power WAIT >> > > mode. >> > > >> > > This allows to use the ARM generic timer on i.MX 6UL/6ULL SoC. >> > > Without the flag the kernel freezes when the timer enters the >> > > first >> > > time ARM power off mode. >> > > >> > > Cc: Anson Huang <anson.huang@nxp.com> >> > > Signed-off-by: Stefan Agner <stefan@agner.ch> >> > >> > It seems ok at my side. >> > Did you meet the real issue? If yes, how to reproduce? >> > >> > Both mx6sx and mx6ul are using GPT which do not need that flag, >> > suppose we >> > should remove it, right? >> > Anson can help confirm it. >> >> For UP system like i.MX6SX, we do NOT enable "cortex-a9-twd-timer", >> so local >> timer is NOT used, GPT is used instead, GPT's clock is NOT disabled >> when cpuidle, >> so I think we should remove all these Timer stop flag for 6SX >> CPUIDLE. > > It's correct to set the flag even on UP systems, as the flag means the > CPU _local_ timer is stopped in this sleep mode. Also there are systems > out there which are using the TWD on UP, as it operates at a higher > frequency leading to better wakeup granularity. Documentation/devicetree/bindings/arm/twd.txt states that TWD provides "per-cpu local timer". But as far as I can see TWD still uses SPI interrupts, routed through GIC, so is this the differentiation? -- Stefan
Am Dienstag, den 09.01.2018, 14:37 +0100 schrieb Stefan Agner: > On 2018-01-09 11:13, Lucas Stach wrote: > > Am Dienstag, den 09.01.2018, 09:25 +0000 schrieb Anson Huang: > > > > > > Best Regards! > > > Anson Huang > > > > > > > > > > -----Original Message----- > > > > From: Dong Aisheng [mailto:dongas86@gmail.com] > > > > Sent: 2018-01-09 5:23 PM > > > > To: Stefan Agner <stefan@agner.ch> > > > > Cc: shawnguo@kernel.org; kernel@pengutronix.de; Fabio Estevam > > > > <fabio.estevam@nxp.com>; robh+dt@kernel.org; mark.rutland@arm.c > > > > om; > > > > linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.or > > > > g; > > > > linux- > > > > kernel@vger.kernel.org; Anson Huang <anson.huang@nxp.com>; dl- > > > > linux-imx > > > > <linux-imx@nxp.com> > > > > Subject: Re: [PATCH 1/7] ARM: imx: add timer stop flag to ARM > > > > power > > > > off state > > > > > > > > On Tue, Jan 02, 2018 at 05:42:17PM +0100, Stefan Agner wrote: > > > > > When the CPU is in ARM power off state the ARM architected > > > > > timers > > > > > are > > > > > stopped. The flag is already present in the higher power WAIT > > > > > mode. > > > > > > > > > > This allows to use the ARM generic timer on i.MX 6UL/6ULL > > > > > SoC. > > > > > Without the flag the kernel freezes when the timer enters the > > > > > first > > > > > time ARM power off mode. > > > > > > > > > > Cc: Anson Huang <anson.huang@nxp.com> > > > > > Signed-off-by: Stefan Agner <stefan@agner.ch> > > > > > > > > It seems ok at my side. > > > > Did you meet the real issue? If yes, how to reproduce? > > > > > > > > Both mx6sx and mx6ul are using GPT which do not need that flag, > > > > suppose we > > > > should remove it, right? > > > > Anson can help confirm it. > > > > > > For UP system like i.MX6SX, we do NOT enable "cortex-a9-twd- > > > timer", > > > so local > > > timer is NOT used, GPT is used instead, GPT's clock is NOT > > > disabled > > > when cpuidle, > > > so I think we should remove all these Timer stop flag for 6SX > > > CPUIDLE. > > > > It's correct to set the flag even on UP systems, as the flag means > > the > > CPU _local_ timer is stopped in this sleep mode. Also there are > > systems > > out there which are using the TWD on UP, as it operates at a higher > > frequency leading to better wakeup granularity. > > Documentation/devicetree/bindings/arm/twd.txt states that TWD > provides > "per-cpu local timer". But as far as I can see TWD still uses SPI > interrupts, routed through GIC, so is this the differentiation? Maybe what I wrote wasn't entirely clear. I completely agree with this patch. The TWD on Cortex-A9 is a CPU local timer, same as the architected timer in later cores. It doesn't provide all the benefits of the architected timer (the clock frequency varies with CPU core clock and it's not virt capable), but some systems still prefer it over the i.MX GPT, as it provides much better wakeup granularity. So annotating the CPU idle states with the timer stop flag is the right thing to do. This flag has nothing to with the usage of GPT or TWD on a specific system. Regards, Lucas
On 2018-01-09 15:04, Lucas Stach wrote: > Am Dienstag, den 09.01.2018, 14:37 +0100 schrieb Stefan Agner: >> On 2018-01-09 11:13, Lucas Stach wrote: >> > Am Dienstag, den 09.01.2018, 09:25 +0000 schrieb Anson Huang: >> > > >> > > Best Regards! >> > > Anson Huang >> > > >> > > >> > > > -----Original Message----- >> > > > From: Dong Aisheng [mailto:dongas86@gmail.com] >> > > > Sent: 2018-01-09 5:23 PM >> > > > To: Stefan Agner <stefan@agner.ch> >> > > > Cc: shawnguo@kernel.org; kernel@pengutronix.de; Fabio Estevam >> > > > <fabio.estevam@nxp.com>; robh+dt@kernel.org; mark.rutland@arm.c >> > > > om; >> > > > linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.or >> > > > g; >> > > > linux- >> > > > kernel@vger.kernel.org; Anson Huang <anson.huang@nxp.com>; dl- >> > > > linux-imx >> > > > <linux-imx@nxp.com> >> > > > Subject: Re: [PATCH 1/7] ARM: imx: add timer stop flag to ARM >> > > > power >> > > > off state >> > > > >> > > > On Tue, Jan 02, 2018 at 05:42:17PM +0100, Stefan Agner wrote: >> > > > > When the CPU is in ARM power off state the ARM architected >> > > > > timers >> > > > > are >> > > > > stopped. The flag is already present in the higher power WAIT >> > > > > mode. >> > > > > >> > > > > This allows to use the ARM generic timer on i.MX 6UL/6ULL >> > > > > SoC. >> > > > > Without the flag the kernel freezes when the timer enters the >> > > > > first >> > > > > time ARM power off mode. >> > > > > >> > > > > Cc: Anson Huang <anson.huang@nxp.com> >> > > > > Signed-off-by: Stefan Agner <stefan@agner.ch> >> > > > >> > > > It seems ok at my side. >> > > > Did you meet the real issue? If yes, how to reproduce? >> > > > >> > > > Both mx6sx and mx6ul are using GPT which do not need that flag, >> > > > suppose we >> > > > should remove it, right? >> > > > Anson can help confirm it. >> > > >> > > For UP system like i.MX6SX, we do NOT enable "cortex-a9-twd- >> > > timer", >> > > so local >> > > timer is NOT used, GPT is used instead, GPT's clock is NOT >> > > disabled >> > > when cpuidle, >> > > so I think we should remove all these Timer stop flag for 6SX >> > > CPUIDLE. >> > >> > It's correct to set the flag even on UP systems, as the flag means >> > the >> > CPU _local_ timer is stopped in this sleep mode. Also there are >> > systems >> > out there which are using the TWD on UP, as it operates at a higher >> > frequency leading to better wakeup granularity. >> >> Documentation/devicetree/bindings/arm/twd.txt states that TWD >> provides >> "per-cpu local timer". But as far as I can see TWD still uses SPI >> interrupts, routed through GIC, so is this the differentiation? > > Maybe what I wrote wasn't entirely clear. I completely agree with this > patch. > > The TWD on Cortex-A9 is a CPU local timer, same as the architected > timer in later cores. It doesn't provide all the benefits of the > architected timer (the clock frequency varies with CPU core clock and > it's not virt capable), but some systems still prefer it over the i.MX > GPT, as it provides much better wakeup granularity. > > So annotating the CPU idle states with the timer stop flag is the right > thing to do. This flag has nothing to with the usage of GPT or TWD on a > specific system. Can I take that as an Acked-by? -- Stefan
Am Dienstag, den 02.01.2018, 17:42 +0100 schrieb Stefan Agner: > When the CPU is in ARM power off state the ARM architected > timers are stopped. The flag is already present in the higher > power WAIT mode. > > This allows to use the ARM generic timer on i.MX 6UL/6ULL SoC. > Without the flag the kernel freezes when the timer enters the > first time ARM power off mode. > > Cc: Anson Huang <anson.huang@nxp.com> > Signed-off-by: Stefan Agner <stefan@agner.ch> Reviewed-by: Lucas Stach <l.stach@pengutronix.de> > --- > arch/arm/mach-imx/cpuidle-imx6sx.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/arm/mach-imx/cpuidle-imx6sx.c b/arch/arm/mach- > imx/cpuidle-imx6sx.c > index c5a5c3a70ab1..d0f14b761ff7 100644 > --- a/arch/arm/mach-imx/cpuidle-imx6sx.c > +++ b/arch/arm/mach-imx/cpuidle-imx6sx.c > @@ -89,6 +89,7 @@ static struct cpuidle_driver imx6sx_cpuidle_driver > = { > */ > .exit_latency = 300, > .target_residency = 500, > + .flags = CPUIDLE_FLAG_TIMER_STOP, > .enter = imx6sx_enter_wait, > .name = "LOW-POWER-IDLE", > .desc = "ARM power off",
diff --git a/arch/arm/mach-imx/cpuidle-imx6sx.c b/arch/arm/mach-imx/cpuidle-imx6sx.c index c5a5c3a70ab1..d0f14b761ff7 100644 --- a/arch/arm/mach-imx/cpuidle-imx6sx.c +++ b/arch/arm/mach-imx/cpuidle-imx6sx.c @@ -89,6 +89,7 @@ static struct cpuidle_driver imx6sx_cpuidle_driver = { */ .exit_latency = 300, .target_residency = 500, + .flags = CPUIDLE_FLAG_TIMER_STOP, .enter = imx6sx_enter_wait, .name = "LOW-POWER-IDLE", .desc = "ARM power off",
When the CPU is in ARM power off state the ARM architected timers are stopped. The flag is already present in the higher power WAIT mode. This allows to use the ARM generic timer on i.MX 6UL/6ULL SoC. Without the flag the kernel freezes when the timer enters the first time ARM power off mode. Cc: Anson Huang <anson.huang@nxp.com> Signed-off-by: Stefan Agner <stefan@agner.ch> --- arch/arm/mach-imx/cpuidle-imx6sx.c | 1 + 1 file changed, 1 insertion(+)