| Message ID | 1479453418-25314-1-git-send-email-krzk@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2016. 11. 18. 16:16 Krzysztof Kozlowski <krzk@kernel.org> wrote: > All Samsung platforms, including the Exynos, are selecting HZ_FIXED with > 200 Hz. Unfortunately in case of multiplatform image this affects also > other platforms when Exynos is enabled. > > This looks like an very old legacy code, dating back to initial > upstreaming of S3C24xx. Probably it was required for s3c24xx timer > driver, which was removed in commit ad38bdd15d5b ("ARM: SAMSUNG: Remove > unused plat-samsung/time.c"). > > Since then, this fixed 200 Hz spread everywhere, including out-of-tree > Samsung kernels (SoC vendor's and Tizen's). I believe this choice > was rather an effect of coincidence instead of conscious choice. > > Exynos uses its own MCT or arch timer and can work with all HZ values. > Older platforms use newer Samsung PWM timer driver which should handle > down to 100 Hz. > > Few perf mem and sched tests on Odroid XU3 board (Exynos5422, 4x Cortex > A7, 4x Cortex A15) show no regressions when switching from 200 Hz to > other values. > > Reported-by: Lee Jones <lee.jones@linaro.org> > [Dropping 200_HZ from S3C/S5P suggested by Arnd] > Reported-by: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> > Cc: Kukjin Kim <kgene@kernel.org> Acked-by: Kukjin Kim <kgene@kernel.org> > Tested-by: Javier Martinez Canillas <javier@osg.samsung.com> > > --- > > Tested on Exynos5422 and Exynos5800 (by Javier). It would be > appreciated if anyone could test it on S3C24xx or S5PV210. > > Changes since v1: > 1. Add Javier's tested-by. > 2. Drop HZ_FIXED also from ARCH_S5PV210 and ARCH_S3C24XX after Arnd > suggestions and analysis. > --- > arch/arm/Kconfig | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > index b5d529fdffab..ced2e08a9d08 100644 > --- a/arch/arm/Kconfig > +++ b/arch/arm/Kconfig > @@ -1496,8 +1496,7 @@ source kernel/Kconfig.preempt > > config HZ_FIXED > int > - default 200 if ARCH_EBSA110 || ARCH_S3C24XX || \ > - ARCH_S5PV210 || ARCH_EXYNOS4 > + default 200 if ARCH_EBSA110 > default 128 if SOC_AT91RM9200 > default 0 > > -- > 2.7.4 >
On Friday, November 18, 2016 9:16:58 AM CET Krzysztof Kozlowski wrote: > All Samsung platforms, including the Exynos, are selecting HZ_FIXED with > 200 Hz. Unfortunately in case of multiplatform image this affects also > other platforms when Exynos is enabled. > > This looks like an very old legacy code, dating back to initial > upstreaming of S3C24xx. Probably it was required for s3c24xx timer > driver, which was removed in commit ad38bdd15d5b ("ARM: SAMSUNG: Remove > unused plat-samsung/time.c"). > > Since then, this fixed 200 Hz spread everywhere, including out-of-tree > Samsung kernels (SoC vendor's and Tizen's). I believe this choice > was rather an effect of coincidence instead of conscious choice. > > Exynos uses its own MCT or arch timer and can work with all HZ values. > Older platforms use newer Samsung PWM timer driver which should handle > down to 100 Hz. > > Few perf mem and sched tests on Odroid XU3 board (Exynos5422, 4x Cortex > A7, 4x Cortex A15) show no regressions when switching from 200 Hz to > other values. > > Reported-by: Lee Jones <lee.jones@linaro.org> > [Dropping 200_HZ from S3C/S5P suggested by Arnd] > Reported-by: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> > Cc: Kukjin Kim <kgene@kernel.org> > Tested-by: Javier Martinez Canillas <javier@osg.samsung.com> > Acked-by: Arnd Bergmann <arnd@arndb.de> Maybe add a paragraph about the specific problem: "On s3c24xx, the PWM counter is only 16 bit wide, and with the typical 12MHz input clock that overflows every 5.5ms. This works with HZ=200 or higher but not with HZ=100 which needs a 10ms interval between ticks. On Later chips (S3C64xx, S5P and EXYNOS), the counter is 32 bits and does not have this problem. The new samsung_pwm_timer driver solves the problem by scaling the input clock by a factor of 50 on s3c24xx, which makes it less accurate but allows HZ=100 as well as CONFIG_NO_HZ with fewer wakeups". Arnd
On 11/18/2016 09:46 AM, Arnd Bergmann wrote: > On Friday, November 18, 2016 9:16:58 AM CET Krzysztof Kozlowski wrote: >> > All Samsung platforms, including the Exynos, are selecting HZ_FIXED with >> > 200 Hz. Unfortunately in case of multiplatform image this affects also >> > other platforms when Exynos is enabled. >> > >> > This looks like an very old legacy code, dating back to initial >> > upstreaming of S3C24xx. Probably it was required for s3c24xx timer >> > driver, which was removed in commit ad38bdd15d5b ("ARM: SAMSUNG: Remove >> > unused plat-samsung/time.c"). >> > >> > Since then, this fixed 200 Hz spread everywhere, including out-of-tree >> > Samsung kernels (SoC vendor's and Tizen's). I believe this choice >> > was rather an effect of coincidence instead of conscious choice. >> > >> > Exynos uses its own MCT or arch timer and can work with all HZ values. >> > Older platforms use newer Samsung PWM timer driver which should handle >> > down to 100 Hz. >> > >> > Few perf mem and sched tests on Odroid XU3 board (Exynos5422, 4x Cortex >> > A7, 4x Cortex A15) show no regressions when switching from 200 Hz to >> > other values. >> > >> > Reported-by: Lee Jones <lee.jones@linaro.org> >> > [Dropping 200_HZ from S3C/S5P suggested by Arnd] >> > Reported-by: Arnd Bergmann <arnd@arndb.de> >> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> >> > Cc: Kukjin Kim <kgene@kernel.org> >> > Tested-by: Javier Martinez Canillas <javier@osg.samsung.com> >> > > Acked-by: Arnd Bergmann <arnd@arndb.de> > > Maybe add a paragraph about the specific problem: > > "On s3c24xx, the PWM counter is only 16 bit wide, and with the > typical 12MHz input clock that overflows every 5.5ms. This works > with HZ=200 or higher but not with HZ=100 which needs a 10ms > interval between ticks. On Later chips (S3C64xx, S5P and EXYNOS), > the counter is 32 bits and does not have this problem. > The new samsung_pwm_timer driver solves the problem by scaling > the input clock by a factor of 50 on s3c24xx, which makes it > less accurate but allows HZ=100 as well as CONFIG_NO_HZ with > fewer wakeups". I've tested on S3C2440 SoC based board and I didn't notice any issues with HZ=100. Clock frequencies look a bit different because AFAIU MPLL clock is mostly used as a root clock. The 12 MHz oscillator clock is used a root clock for the MPLL. refclk: 12000 kHz mpll: 405000 kHz upll: 48000 kHz fclk: 405000 kHz hclk: 101250 kHz pclk: 50625 kHz So frequency of the timer block's source clock (PCLK) is 50.625 MHz. This is further divided by 50 in the prescaler as you pointed out. So the 16-bit is clocked with 1012500 Hz clock. I added some printks to verify this. Here is boot log for HZ=200: http://pastebin.com/JuWZdYwh and HZ=100 http://pastebin.com/HnDnBfhc samsung_clocksource_init:351 pclk: 50625000, timer clock_rate: 1012500 sched_clock: 16 bits at 1012kHz, resolution 987ns, wraps every 32362962ns I just don't understand why the log says timer overflow is every 32.362 ms and not twice this value (65536 * 1/1012500). -- Thanks, Sylwester
On Fri, Nov 18, 2016 at 11:43:14AM +0100, Sylwester Nawrocki wrote: > On 11/18/2016 09:46 AM, Arnd Bergmann wrote: > > On Friday, November 18, 2016 9:16:58 AM CET Krzysztof Kozlowski wrote: > >> > All Samsung platforms, including the Exynos, are selecting HZ_FIXED with > >> > 200 Hz. Unfortunately in case of multiplatform image this affects also > >> > other platforms when Exynos is enabled. > >> > > >> > This looks like an very old legacy code, dating back to initial > >> > upstreaming of S3C24xx. Probably it was required for s3c24xx timer > >> > driver, which was removed in commit ad38bdd15d5b ("ARM: SAMSUNG: Remove > >> > unused plat-samsung/time.c"). > >> > > >> > Since then, this fixed 200 Hz spread everywhere, including out-of-tree > >> > Samsung kernels (SoC vendor's and Tizen's). I believe this choice > >> > was rather an effect of coincidence instead of conscious choice. > >> > > >> > Exynos uses its own MCT or arch timer and can work with all HZ values. > >> > Older platforms use newer Samsung PWM timer driver which should handle > >> > down to 100 Hz. > >> > > >> > Few perf mem and sched tests on Odroid XU3 board (Exynos5422, 4x Cortex > >> > A7, 4x Cortex A15) show no regressions when switching from 200 Hz to > >> > other values. > >> > > >> > Reported-by: Lee Jones <lee.jones@linaro.org> > >> > [Dropping 200_HZ from S3C/S5P suggested by Arnd] > >> > Reported-by: Arnd Bergmann <arnd@arndb.de> > >> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> > >> > Cc: Kukjin Kim <kgene@kernel.org> > >> > Tested-by: Javier Martinez Canillas <javier@osg.samsung.com> > >> > > > Acked-by: Arnd Bergmann <arnd@arndb.de> > > > > Maybe add a paragraph about the specific problem: > > > > "On s3c24xx, the PWM counter is only 16 bit wide, and with the > > typical 12MHz input clock that overflows every 5.5ms. This works > > with HZ=200 or higher but not with HZ=100 which needs a 10ms > > interval between ticks. On Later chips (S3C64xx, S5P and EXYNOS), > > the counter is 32 bits and does not have this problem. > > The new samsung_pwm_timer driver solves the problem by scaling > > the input clock by a factor of 50 on s3c24xx, which makes it > > less accurate but allows HZ=100 as well as CONFIG_NO_HZ with > > fewer wakeups". > > I've tested on S3C2440 SoC based board and I didn't notice any > issues with HZ=100. > > Clock frequencies look a bit different because AFAIU MPLL > clock is mostly used as a root clock. The 12 MHz oscillator clock > is used a root clock for the MPLL. > > refclk: 12000 kHz > mpll: 405000 kHz > upll: 48000 kHz > fclk: 405000 kHz > hclk: 101250 kHz > pclk: 50625 kHz > > So frequency of the timer block's source clock (PCLK) is 50.625 MHz. > This is further divided by 50 in the prescaler as you pointed out. > > So the 16-bit is clocked with 1012500 Hz clock. I added some printks > to verify this. > > Here is boot log for HZ=200: http://pastebin.com/JuWZdYwh > and HZ=100 http://pastebin.com/HnDnBfhc > > samsung_clocksource_init:351 pclk: 50625000, timer clock_rate: 1012500 > sched_clock: 16 bits at 1012kHz, resolution 987ns, wraps every 32362962ns Thanks for tests! I really appreciate it. > I just don't understand why the log says timer overflow is every 32.362 ms > and not twice this value (65536 * 1/1012500). The answer could be at kernel/time/clocksource.c: * NOTE: This function includes a safety margin of 50%, in other words, we * return half the number of nanoseconds the hardware counter can technically * cover. Best regards, Krzysztof
On Fri, 18 Nov 2016, Krzysztof Kozlowski wrote: > All Samsung platforms, including the Exynos, are selecting HZ_FIXED with > 200 Hz. Unfortunately in case of multiplatform image this affects also > other platforms when Exynos is enabled. > > This looks like an very old legacy code, dating back to initial > upstreaming of S3C24xx. Probably it was required for s3c24xx timer > driver, which was removed in commit ad38bdd15d5b ("ARM: SAMSUNG: Remove > unused plat-samsung/time.c"). > > Since then, this fixed 200 Hz spread everywhere, including out-of-tree > Samsung kernels (SoC vendor's and Tizen's). I believe this choice > was rather an effect of coincidence instead of conscious choice. > > Exynos uses its own MCT or arch timer and can work with all HZ values. > Older platforms use newer Samsung PWM timer driver which should handle > down to 100 Hz. > > Few perf mem and sched tests on Odroid XU3 board (Exynos5422, 4x Cortex > A7, 4x Cortex A15) show no regressions when switching from 200 Hz to > other values. > > Reported-by: Lee Jones <lee.jones@linaro.org> Acked-by: Lee Jones <lee.jones@linaro.org> > [Dropping 200_HZ from S3C/S5P suggested by Arnd] > Reported-by: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> > Cc: Kukjin Kim <kgene@kernel.org> > Tested-by: Javier Martinez Canillas <javier@osg.samsung.com> > > --- > > Tested on Exynos5422 and Exynos5800 (by Javier). It would be > appreciated if anyone could test it on S3C24xx or S5PV210. > > Changes since v1: > 1. Add Javier's tested-by. > 2. Drop HZ_FIXED also from ARCH_S5PV210 and ARCH_S3C24XX after Arnd > suggestions and analysis. > --- > arch/arm/Kconfig | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > index b5d529fdffab..ced2e08a9d08 100644 > --- a/arch/arm/Kconfig > +++ b/arch/arm/Kconfig > @@ -1496,8 +1496,7 @@ source kernel/Kconfig.preempt > > config HZ_FIXED > int > - default 200 if ARCH_EBSA110 || ARCH_S3C24XX || \ > - ARCH_S5PV210 || ARCH_EXYNOS4 > + default 200 if ARCH_EBSA110 > default 128 if SOC_AT91RM9200 > default 0 >
2016-11-18 17:46 GMT+09:00 Arnd Bergmann <arnd@arndb.de>: > Maybe add a paragraph about the specific problem: > > "On s3c24xx, the PWM counter is only 16 bit wide, and with the > typical 12MHz input clock that overflows every 5.5ms. This works > with HZ=200 or higher but not with HZ=100 which needs a 10ms > interval between ticks. On Later chips (S3C64xx, S5P and EXYNOS), > the counter is 32 bits and does not have this problem. > The new samsung_pwm_timer driver solves the problem by scaling > the input clock by a factor of 50 on s3c24xx, which makes it > less accurate but allows HZ=100 as well as CONFIG_NO_HZ with > fewer wakeups". One thing to correct here is that the typical clock is PCLK, which is derived from one of the PLLs and AFAIR is between 33-66 MHz on s3c24xx. Technically you can drive the PWM block from an external clock (12 MHz for some board-file based boards), but for simplicity this functionality was omitted in the new PWM timer driver used for DT boards (which worked fine with the PWM driven by PCLK). Also I'm wondering if the divisor we use right now for 16-bit timers isn't too small, since it gives us a really short wraparound time, which means getting more timer interrupts for longer intervals, kind of defeating the benefit of tickless mode. However, AFAICT it doesn't affect the HZ problem. Best regards. Tomasz
On 21/11/16 06:01, Tomasz Figa wrote: > 2016-11-18 17:46 GMT+09:00 Arnd Bergmann <arnd@arndb.de>: >> Maybe add a paragraph about the specific problem: >> >> "On s3c24xx, the PWM counter is only 16 bit wide, and with the >> typical 12MHz input clock that overflows every 5.5ms. This works >> with HZ=200 or higher but not with HZ=100 which needs a 10ms >> interval between ticks. On Later chips (S3C64xx, S5P and EXYNOS), >> the counter is 32 bits and does not have this problem. >> The new samsung_pwm_timer driver solves the problem by scaling >> the input clock by a factor of 50 on s3c24xx, which makes it >> less accurate but allows HZ=100 as well as CONFIG_NO_HZ with >> fewer wakeups". > > One thing to correct here is that the typical clock is PCLK, which is > derived from one of the PLLs and AFAIR is between 33-66 MHz on > s3c24xx. Technically you can drive the PWM block from an external > clock (12 MHz for some board-file based boards), but for simplicity > this functionality was omitted in the new PWM timer driver used for DT > boards (which worked fine with the PWM driven by PCLK). Given it was a clock mux option, that would not have been difficult to acheive. However these platforms are now so old people don't care, I think all my pre-armv7 stuff is now in a box. The use of the 12MHz input was to give something to run PWM timers from that wasn't interrupted by cpu frequency scaling as PCLK generally is half HCLK which is divided down from the core CPU clock. (Later devices had multiple PLL sources so you didn't have to have the CPU fed from the same clock as the peripherals) > Also I'm wondering if the divisor we use right now for 16-bit timers > isn't too small, since it gives us a really short wraparound time, > which means getting more timer interrupts for longer intervals, kind > of defeating the benefit of tickless mode. However, AFAICT it doesn't > affect the HZ problem. The original implementation was to go for the best accuracy from the timer at the expense of 200 irqs per second instead of the usual 100. > Best regards. > Tomasz >
On Mon, Nov 21, 2016 at 08:59:06AM +0000, Ben Dooks wrote: > On 21/11/16 06:01, Tomasz Figa wrote: > >2016-11-18 17:46 GMT+09:00 Arnd Bergmann <arnd@arndb.de>: > >>Maybe add a paragraph about the specific problem: > >> > >>"On s3c24xx, the PWM counter is only 16 bit wide, and with the > >>typical 12MHz input clock that overflows every 5.5ms. This works > >>with HZ=200 or higher but not with HZ=100 which needs a 10ms > >>interval between ticks. On Later chips (S3C64xx, S5P and EXYNOS), > >>the counter is 32 bits and does not have this problem. > >>The new samsung_pwm_timer driver solves the problem by scaling > >>the input clock by a factor of 50 on s3c24xx, which makes it > >>less accurate but allows HZ=100 as well as CONFIG_NO_HZ with > >>fewer wakeups". > > > >One thing to correct here is that the typical clock is PCLK, which is > >derived from one of the PLLs and AFAIR is between 33-66 MHz on > >s3c24xx. Technically you can drive the PWM block from an external > >clock (12 MHz for some board-file based boards), but for simplicity > >this functionality was omitted in the new PWM timer driver used for DT > >boards (which worked fine with the PWM driven by PCLK). > > Given it was a clock mux option, that would not have been difficult to > acheive. However these platforms are now so old people don't care, I > think all my pre-armv7 stuff is now in a box. However, there are still s3c2410 machines running out there... so we should do our best not to break them. > The use of the 12MHz input was to give something to run PWM timers from > that wasn't interrupted by cpu frequency scaling as PCLK generally is > half HCLK which is divided down from the core CPU clock. ... > The original implementation was to go for the best accuracy from the > timer at the expense of 200 irqs per second instead of the usual 100. This sounds like a good enough reason not to switch away from using 200Hz and the 12MHz input.
On Mon, Nov 21, 2016 at 03:01:57PM +0900, Tomasz Figa wrote: > One thing to correct here is that the typical clock is PCLK, which is > derived from one of the PLLs and AFAIR is between 33-66 MHz on > s3c24xx. Technically you can drive the PWM block from an external > clock (12 MHz for some board-file based boards), but for simplicity > this functionality was omitted in the new PWM timer driver used for DT > boards (which worked fine with the PWM driven by PCLK). So that's a knowingly-made regression for s3c24xx then. Pretty disgusting developer behavior to do that, IMHO.
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index b5d529fdffab..ced2e08a9d08 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -1496,8 +1496,7 @@ source kernel/Kconfig.preempt config HZ_FIXED int - default 200 if ARCH_EBSA110 || ARCH_S3C24XX || \ - ARCH_S5PV210 || ARCH_EXYNOS4 + default 200 if ARCH_EBSA110 default 128 if SOC_AT91RM9200 default 0
All Samsung platforms, including the Exynos, are selecting HZ_FIXED with 200 Hz. Unfortunately in case of multiplatform image this affects also other platforms when Exynos is enabled. This looks like an very old legacy code, dating back to initial upstreaming of S3C24xx. Probably it was required for s3c24xx timer driver, which was removed in commit ad38bdd15d5b ("ARM: SAMSUNG: Remove unused plat-samsung/time.c"). Since then, this fixed 200 Hz spread everywhere, including out-of-tree Samsung kernels (SoC vendor's and Tizen's). I believe this choice was rather an effect of coincidence instead of conscious choice. Exynos uses its own MCT or arch timer and can work with all HZ values. Older platforms use newer Samsung PWM timer driver which should handle down to 100 Hz. Few perf mem and sched tests on Odroid XU3 board (Exynos5422, 4x Cortex A7, 4x Cortex A15) show no regressions when switching from 200 Hz to other values. Reported-by: Lee Jones <lee.jones@linaro.org> [Dropping 200_HZ from S3C/S5P suggested by Arnd] Reported-by: Arnd Bergmann <arnd@arndb.de> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> Cc: Kukjin Kim <kgene@kernel.org> Tested-by: Javier Martinez Canillas <javier@osg.samsung.com> --- Tested on Exynos5422 and Exynos5800 (by Javier). It would be appreciated if anyone could test it on S3C24xx or S5PV210. Changes since v1: 1. Add Javier's tested-by. 2. Drop HZ_FIXED also from ARCH_S5PV210 and ARCH_S3C24XX after Arnd suggestions and analysis. --- arch/arm/Kconfig | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)