linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] ARM: Drop fixed 200 Hz timer requirement from Samsung platforms
@ 2016-11-18  7:16 Krzysztof Kozlowski
  2016-11-18  7:57 ` Kukjin Kim
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Krzysztof Kozlowski @ 2016-11-18  7:16 UTC (permalink / raw)
  To: Russell King, Kukjin Kim, Krzysztof Kozlowski,
	Javier Martinez Canillas, linux-arm-kernel, linux-samsung-soc,
	linux-kernel
  Cc: Ben Dooks, Lee Jones, Arnd Bergmann, Marek Szyprowski,
	Bartlomiej Zolnierkiewicz, Sylwester Nawrocki, Tomasz Figa

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(-)

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] ARM: Drop fixed 200 Hz timer requirement from Samsung platforms
  2016-11-18  7:16 [PATCH v2] ARM: Drop fixed 200 Hz timer requirement from Samsung platforms Krzysztof Kozlowski
@ 2016-11-18  7:57 ` Kukjin Kim
  2016-11-18  8:46 ` Arnd Bergmann
  2016-11-18 13:17 ` Lee Jones
  2 siblings, 0 replies; 10+ messages in thread
From: Kukjin Kim @ 2016-11-18  7:57 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Russell King, Kukjin Kim, Javier Martinez Canillas,
	linux-arm-kernel, linux-samsung-soc, linux-kernel, Ben Dooks,
	Lee Jones, Arnd Bergmann, Marek Szyprowski,
	Bartlomiej Zolnierkiewicz, Sylwester Nawrocki, Tomasz Figa

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
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] ARM: Drop fixed 200 Hz timer requirement from Samsung platforms
  2016-11-18  7:16 [PATCH v2] ARM: Drop fixed 200 Hz timer requirement from Samsung platforms Krzysztof Kozlowski
  2016-11-18  7:57 ` Kukjin Kim
@ 2016-11-18  8:46 ` Arnd Bergmann
  2016-11-18 10:43   ` Sylwester Nawrocki
  2016-11-21  6:01   ` Tomasz Figa
  2016-11-18 13:17 ` Lee Jones
  2 siblings, 2 replies; 10+ messages in thread
From: Arnd Bergmann @ 2016-11-18  8:46 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Krzysztof Kozlowski, Russell King, Kukjin Kim,
	Javier Martinez Canillas, linux-samsung-soc, linux-kernel,
	Bartlomiej Zolnierkiewicz, Tomasz Figa, Ben Dooks,
	Sylwester Nawrocki, Lee Jones, Marek Szyprowski

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] ARM: Drop fixed 200 Hz timer requirement from Samsung platforms
  2016-11-18  8:46 ` Arnd Bergmann
@ 2016-11-18 10:43   ` Sylwester Nawrocki
  2016-11-18 11:10     ` Krzysztof Kozlowski
  2016-11-21  6:01   ` Tomasz Figa
  1 sibling, 1 reply; 10+ messages in thread
From: Sylwester Nawrocki @ 2016-11-18 10:43 UTC (permalink / raw)
  To: Arnd Bergmann, linux-arm-kernel, Krzysztof Kozlowski
  Cc: Russell King, Kukjin Kim, Javier Martinez Canillas,
	linux-samsung-soc, linux-kernel, Bartlomiej Zolnierkiewicz,
	Tomasz Figa, Ben Dooks, Lee Jones, Marek Szyprowski

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] ARM: Drop fixed 200 Hz timer requirement from Samsung platforms
  2016-11-18 10:43   ` Sylwester Nawrocki
@ 2016-11-18 11:10     ` Krzysztof Kozlowski
  0 siblings, 0 replies; 10+ messages in thread
From: Krzysztof Kozlowski @ 2016-11-18 11:10 UTC (permalink / raw)
  To: Sylwester Nawrocki
  Cc: Arnd Bergmann, linux-arm-kernel, Krzysztof Kozlowski,
	Russell King, Kukjin Kim, Javier Martinez Canillas,
	linux-samsung-soc, linux-kernel, Bartlomiej Zolnierkiewicz,
	Tomasz Figa, Ben Dooks, Lee Jones, Marek Szyprowski

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] ARM: Drop fixed 200 Hz timer requirement from Samsung platforms
  2016-11-18  7:16 [PATCH v2] ARM: Drop fixed 200 Hz timer requirement from Samsung platforms Krzysztof Kozlowski
  2016-11-18  7:57 ` Kukjin Kim
  2016-11-18  8:46 ` Arnd Bergmann
@ 2016-11-18 13:17 ` Lee Jones
  2 siblings, 0 replies; 10+ messages in thread
From: Lee Jones @ 2016-11-18 13:17 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Russell King, Kukjin Kim, Javier Martinez Canillas,
	linux-arm-kernel, linux-samsung-soc, linux-kernel, Ben Dooks,
	Arnd Bergmann, Marek Szyprowski, Bartlomiej Zolnierkiewicz,
	Sylwester Nawrocki, Tomasz Figa

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
>  

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] ARM: Drop fixed 200 Hz timer requirement from Samsung platforms
  2016-11-18  8:46 ` Arnd Bergmann
  2016-11-18 10:43   ` Sylwester Nawrocki
@ 2016-11-21  6:01   ` Tomasz Figa
  2016-11-21  8:59     ` Ben Dooks
  2016-11-21  9:24     ` Russell King - ARM Linux
  1 sibling, 2 replies; 10+ messages in thread
From: Tomasz Figa @ 2016-11-21  6:01 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: linux-arm-kernel, Krzysztof Kozlowski, Russell King, Kukjin Kim,
	Javier Martinez Canillas, linux-samsung-soc, linux-kernel,
	Bartlomiej Zolnierkiewicz, Ben Dooks, Sylwester Nawrocki,
	Lee Jones, Marek Szyprowski

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] ARM: Drop fixed 200 Hz timer requirement from Samsung platforms
  2016-11-21  6:01   ` Tomasz Figa
@ 2016-11-21  8:59     ` Ben Dooks
  2016-11-21  9:07       ` Russell King - ARM Linux
  2016-11-21  9:24     ` Russell King - ARM Linux
  1 sibling, 1 reply; 10+ messages in thread
From: Ben Dooks @ 2016-11-21  8:59 UTC (permalink / raw)
  To: Tomasz Figa, Arnd Bergmann
  Cc: linux-arm-kernel, Krzysztof Kozlowski, Russell King, Kukjin Kim,
	Javier Martinez Canillas, linux-samsung-soc, linux-kernel,
	Bartlomiej Zolnierkiewicz, Sylwester Nawrocki, Lee Jones,
	Marek Szyprowski

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
>


-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] ARM: Drop fixed 200 Hz timer requirement from Samsung platforms
  2016-11-21  8:59     ` Ben Dooks
@ 2016-11-21  9:07       ` Russell King - ARM Linux
  0 siblings, 0 replies; 10+ messages in thread
From: Russell King - ARM Linux @ 2016-11-21  9:07 UTC (permalink / raw)
  To: Ben Dooks
  Cc: Tomasz Figa, Arnd Bergmann, linux-samsung-soc,
	Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski, linux-kernel,
	Javier Martinez Canillas, Kukjin Kim, Sylwester Nawrocki,
	Lee Jones, linux-arm-kernel, Marek Szyprowski

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.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] ARM: Drop fixed 200 Hz timer requirement from Samsung platforms
  2016-11-21  6:01   ` Tomasz Figa
  2016-11-21  8:59     ` Ben Dooks
@ 2016-11-21  9:24     ` Russell King - ARM Linux
  1 sibling, 0 replies; 10+ messages in thread
From: Russell King - ARM Linux @ 2016-11-21  9:24 UTC (permalink / raw)
  To: Tomasz Figa
  Cc: Arnd Bergmann, linux-arm-kernel, Krzysztof Kozlowski, Kukjin Kim,
	Javier Martinez Canillas, linux-samsung-soc, linux-kernel,
	Bartlomiej Zolnierkiewicz, Ben Dooks, Sylwester Nawrocki,
	Lee Jones, Marek Szyprowski

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.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2016-11-21  9:25 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-18  7:16 [PATCH v2] ARM: Drop fixed 200 Hz timer requirement from Samsung platforms Krzysztof Kozlowski
2016-11-18  7:57 ` Kukjin Kim
2016-11-18  8:46 ` Arnd Bergmann
2016-11-18 10:43   ` Sylwester Nawrocki
2016-11-18 11:10     ` Krzysztof Kozlowski
2016-11-21  6:01   ` Tomasz Figa
2016-11-21  8:59     ` Ben Dooks
2016-11-21  9:07       ` Russell King - ARM Linux
2016-11-21  9:24     ` Russell King - ARM Linux
2016-11-18 13:17 ` Lee Jones

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).