2016-04-26 15:31:22

by Grygorii Strashko

[permalink] [raw]
Subject: [PATCH v1] ARM: clocksource: make ARM_GLOBAL_TIMER selectable

This patch intended to fix following cases:
- SoC-A has ARM GT, defines DT node for ARM GT and selects
ARM_GLOBAL_TIMER statically in Kconfig file. SoC-B has ARM GT and
defines DT node for ARM GT, but do not selects ARM_GLOBAL_TIMER
statically in Kconfig file. In case of multiplatform build ARM GT will
be implicitly enabled for SoC-B.

- There is no way to disable ARM GT without modifying Kconfig file,
once ARM_GLOBAL_TIMER is selected statically in Kconfig file.

Hence, fix above case by defining both HAVE_ARM_GLOBAL_TIMER and
ARM_GLOBAL_TIMER as recommended by 'Adding common features and make
the usage configurable' section in kconfig-language.txt. All places in
ARM folder where ARM_GLOBAL_TIMER was used now replaced on
HAVE_ARM_GLOBAL_TIMER.

Cc: Daniel Lezcano <[email protected]>
Cc: Florian Fainelli <[email protected]>
Cc: Russell King <[email protected]>
Cc: Wei Xu <[email protected]>
Cc: Shawn Guo <[email protected]>
Cc: Sascha Hauer <[email protected]>
Cc: Srinivas Kandagatla <[email protected]>
Cc: Maxime Coquelin <[email protected]>
Cc: Masahiro Yamada <[email protected]>
Cc: Liviu Dudau <[email protected]>
Cc: Sudeep Holla <[email protected]>
Cc: Jun Nie <[email protected]>
Cc: Michal Simek <[email protected]>
Cc: Jesper Nilsson <[email protected]>
Cc: Lars Persson <[email protected]>
Cc: Mike Looijmans <[email protected]>
Acked-by: Sören Brinkmann <[email protected]>
Acked-by: Moritz Fischer <[email protected]>
Signed-off-by: Grygorii Strashko <[email protected]>
---
Changes is v1:
- updated mach-artpec
- rebased on top of tip: timers/core
commit: 86d3473 time: Introduce do_sys_settimeofday64()

arch/arm/mach-artpec/Kconfig | 2 +-
arch/arm/mach-bcm/Kconfig | 4 ++--
arch/arm/mach-hisi/Kconfig | 2 +-
arch/arm/mach-imx/Kconfig | 2 +-
arch/arm/mach-rockchip/Kconfig | 2 +-
arch/arm/mach-sti/Kconfig | 2 +-
arch/arm/mach-uniphier/Kconfig | 2 +-
arch/arm/mach-vexpress/Kconfig | 2 +-
arch/arm/mach-zx/Kconfig | 2 +-
arch/arm/mach-zynq/Kconfig | 2 +-
drivers/clocksource/Kconfig | 7 ++++++-
11 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/arch/arm/mach-artpec/Kconfig b/arch/arm/mach-artpec/Kconfig
index 6cbe5a2..6cbca77 100644
--- a/arch/arm/mach-artpec/Kconfig
+++ b/arch/arm/mach-artpec/Kconfig
@@ -9,7 +9,7 @@ config MACH_ARTPEC6
depends on ARCH_MULTI_V7
select ARM_AMBA
select ARM_GIC
- select ARM_GLOBAL_TIMER
+ select HAVE_ARM_GLOBAL_TIMER
select ARM_PSCI
select HAVE_ARM_ARCH_TIMER
select HAVE_ARM_SCU
diff --git a/arch/arm/mach-bcm/Kconfig b/arch/arm/mach-bcm/Kconfig
index 7ef1214..d4d079a7 100644
--- a/arch/arm/mach-bcm/Kconfig
+++ b/arch/arm/mach-bcm/Kconfig
@@ -14,7 +14,7 @@ config ARCH_BCM_IPROC
select CACHE_L2X0
select HAVE_ARM_SCU if SMP
select HAVE_ARM_TWD if SMP
- select ARM_GLOBAL_TIMER
+ select HAVE_ARM_GLOBAL_TIMER
select COMMON_CLK_IPROC
select CLKSRC_MMIO
select ARCH_REQUIRE_GPIOLIB
@@ -156,7 +156,7 @@ config ARCH_BCM_63XX
select ARM_ERRATA_754322
select ARM_ERRATA_764369 if SMP
select ARM_GIC
- select ARM_GLOBAL_TIMER
+ select HAVE_ARM_GLOBAL_TIMER
select CACHE_L2X0
select HAVE_ARM_ARCH_TIMER
select HAVE_ARM_TWD if SMP
diff --git a/arch/arm/mach-hisi/Kconfig b/arch/arm/mach-hisi/Kconfig
index a3b091a..251bb03 100644
--- a/arch/arm/mach-hisi/Kconfig
+++ b/arch/arm/mach-hisi/Kconfig
@@ -28,7 +28,7 @@ config ARCH_HIP01
depends on ARCH_MULTI_V7
select HAVE_ARM_SCU if SMP
select HAVE_ARM_TWD if SMP
- select ARM_GLOBAL_TIMER
+ select HAVE_ARM_GLOBAL_TIMER
help
Support for Hisilicon HIP01 SoC family

diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
index 8973fae..3fbf38c 100644
--- a/arch/arm/mach-imx/Kconfig
+++ b/arch/arm/mach-imx/Kconfig
@@ -602,7 +602,7 @@ choice
config VF_USE_ARM_GLOBAL_TIMER
bool "Use ARM Global Timer"
depends on ARCH_MULTI_V7
- select ARM_GLOBAL_TIMER
+ select HAVE_ARM_GLOBAL_TIMER
select CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
help
Use the ARM Global Timer as clocksource
diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
index cef42fd..a53b787 100644
--- a/arch/arm/mach-rockchip/Kconfig
+++ b/arch/arm/mach-rockchip/Kconfig
@@ -14,7 +14,7 @@ config ARCH_ROCKCHIP
select DW_APB_TIMER_OF
select REGULATOR if PM
select ROCKCHIP_TIMER
- select ARM_GLOBAL_TIMER
+ select HAVE_ARM_GLOBAL_TIMER
select CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
help
Support for Rockchip's Cortex-A9 Single-to-Quad-Core-SoCs
diff --git a/arch/arm/mach-sti/Kconfig b/arch/arm/mach-sti/Kconfig
index a196d14..c799f9d 100644
--- a/arch/arm/mach-sti/Kconfig
+++ b/arch/arm/mach-sti/Kconfig
@@ -3,7 +3,7 @@ menuconfig ARCH_STI
depends on ARCH_MULTI_V7
select ARM_GIC
select ST_IRQCHIP
- select ARM_GLOBAL_TIMER
+ select HAVE_ARM_GLOBAL_TIMER
select CLKSRC_ST_LPC
select PINCTRL
select PINCTRL_ST
diff --git a/arch/arm/mach-uniphier/Kconfig b/arch/arm/mach-uniphier/Kconfig
index 82dddee..81bdf77 100644
--- a/arch/arm/mach-uniphier/Kconfig
+++ b/arch/arm/mach-uniphier/Kconfig
@@ -2,7 +2,7 @@ config ARCH_UNIPHIER
bool "Socionext UniPhier SoCs"
depends on ARCH_MULTI_V7
select ARM_AMBA
- select ARM_GLOBAL_TIMER
+ select HAVE_ARM_GLOBAL_TIMER
select ARM_GIC
select HAVE_ARM_SCU
select HAVE_ARM_TWD if SMP
diff --git a/arch/arm/mach-vexpress/Kconfig b/arch/arm/mach-vexpress/Kconfig
index 398a297..e89941d 100644
--- a/arch/arm/mach-vexpress/Kconfig
+++ b/arch/arm/mach-vexpress/Kconfig
@@ -5,7 +5,7 @@ menuconfig ARCH_VEXPRESS
select ARCH_SUPPORTS_BIG_ENDIAN
select ARM_AMBA
select ARM_GIC
- select ARM_GLOBAL_TIMER
+ select HAVE_ARM_GLOBAL_TIMER
select ARM_TIMER_SP804
select COMMON_CLK_VERSATILE
select HAVE_ARM_SCU if SMP
diff --git a/arch/arm/mach-zx/Kconfig b/arch/arm/mach-zx/Kconfig
index 209c979..e84b0c2 100644
--- a/arch/arm/mach-zx/Kconfig
+++ b/arch/arm/mach-zx/Kconfig
@@ -11,7 +11,7 @@ if ARCH_ZX
config SOC_ZX296702
def_bool y
select ARM_GIC
- select ARM_GLOBAL_TIMER
+ select HAVE_ARM_GLOBAL_TIMER
select HAVE_ARM_SCU if SMP
select HAVE_ARM_TWD if SMP
select PM_GENERIC_DOMAINS if PM
diff --git a/arch/arm/mach-zynq/Kconfig b/arch/arm/mach-zynq/Kconfig
index fd0aeeb..3165720 100644
--- a/arch/arm/mach-zynq/Kconfig
+++ b/arch/arm/mach-zynq/Kconfig
@@ -5,7 +5,7 @@ config ARCH_ZYNQ
select ARCH_SUPPORTS_BIG_ENDIAN
select ARM_AMBA
select ARM_GIC
- select ARM_GLOBAL_TIMER if !CPU_FREQ
+ select HAVE_ARM_GLOBAL_TIMER if !CPU_FREQ
select CADENCE_TTC_TIMER
select HAVE_ARM_SCU if SMP
select HAVE_ARM_TWD if SMP
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index c346be6..c686811 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -207,9 +207,14 @@ config ARM_ARCH_TIMER_EVTSTREAM
This must be disabled for hardware validation purposes to detect any
hardware anomalies of missing events.

-config ARM_GLOBAL_TIMER
+config HAVE_ARM_GLOBAL_TIMER
bool
+
+config ARM_GLOBAL_TIMER
+ bool "Support for ARM global timer unit"
select CLKSRC_OF if OF
+ default y
+ depends on HAVE_ARM_GLOBAL_TIMER
help
This options enables support for the ARM global timer unit

--
2.8.0


2016-04-26 16:02:50

by Liviu Dudau

[permalink] [raw]
Subject: Re: [PATCH v1] ARM: clocksource: make ARM_GLOBAL_TIMER selectable

On Tue, Apr 26, 2016 at 06:28:52PM +0300, Grygorii Strashko wrote:

Hi Grygorii,

First time I'm seeing this patch, so I have a few questions, mostly
related to the commit message:

> This patch intended to fix following cases:
> - SoC-A has ARM GT, defines DT node for ARM GT and selects
> ARM_GLOBAL_TIMER statically in Kconfig file. SoC-B has ARM GT and
> defines DT node for ARM GT, but do not selects ARM_GLOBAL_TIMER
> statically in Kconfig file. In case of multiplatform build ARM GT will
> be implicitly enabled for SoC-B.

Well, SoC-B has the GT *and* the DT node, so what is the problem with
enabling it for SoC-B? If there are reasons not to use the Global Timer
on SoC-B, surely a better option would be to mark it in DT with status = "disabled";

>
> - There is no way to disable ARM GT without modifying Kconfig file,
> once ARM_GLOBAL_TIMER is selected statically in Kconfig file.

What about disabling the DT node?

Not sure I properly understand the problem you are trying to solve here.

>
> Hence, fix above case by defining both HAVE_ARM_GLOBAL_TIMER and
> ARM_GLOBAL_TIMER as recommended by 'Adding common features and make
> the usage configurable' section in kconfig-language.txt. All places in
> ARM folder where ARM_GLOBAL_TIMER was used now replaced on
> HAVE_ARM_GLOBAL_TIMER.

I'm OK with the way you have changed ARM_GLOBAL_TIMER from a sticky config
option to a selectable one, but I would like more details on the problem
this was causing you.

Best regards,
Liviu

>
> Cc: Daniel Lezcano <[email protected]>
> Cc: Florian Fainelli <[email protected]>
> Cc: Russell King <[email protected]>
> Cc: Wei Xu <[email protected]>
> Cc: Shawn Guo <[email protected]>
> Cc: Sascha Hauer <[email protected]>
> Cc: Srinivas Kandagatla <[email protected]>
> Cc: Maxime Coquelin <[email protected]>
> Cc: Masahiro Yamada <[email protected]>
> Cc: Liviu Dudau <[email protected]>
> Cc: Sudeep Holla <[email protected]>
> Cc: Jun Nie <[email protected]>
> Cc: Michal Simek <[email protected]>
> Cc: Jesper Nilsson <[email protected]>
> Cc: Lars Persson <[email protected]>
> Cc: Mike Looijmans <[email protected]>
> Acked-by: Sören Brinkmann <[email protected]>
> Acked-by: Moritz Fischer <[email protected]>
> Signed-off-by: Grygorii Strashko <[email protected]>
> ---
> Changes is v1:
> - updated mach-artpec
> - rebased on top of tip: timers/core
> commit: 86d3473 time: Introduce do_sys_settimeofday64()
>
> arch/arm/mach-artpec/Kconfig | 2 +-
> arch/arm/mach-bcm/Kconfig | 4 ++--
> arch/arm/mach-hisi/Kconfig | 2 +-
> arch/arm/mach-imx/Kconfig | 2 +-
> arch/arm/mach-rockchip/Kconfig | 2 +-
> arch/arm/mach-sti/Kconfig | 2 +-
> arch/arm/mach-uniphier/Kconfig | 2 +-
> arch/arm/mach-vexpress/Kconfig | 2 +-
> arch/arm/mach-zx/Kconfig | 2 +-
> arch/arm/mach-zynq/Kconfig | 2 +-
> drivers/clocksource/Kconfig | 7 ++++++-
> 11 files changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm/mach-artpec/Kconfig b/arch/arm/mach-artpec/Kconfig
> index 6cbe5a2..6cbca77 100644
> --- a/arch/arm/mach-artpec/Kconfig
> +++ b/arch/arm/mach-artpec/Kconfig
> @@ -9,7 +9,7 @@ config MACH_ARTPEC6
> depends on ARCH_MULTI_V7
> select ARM_AMBA
> select ARM_GIC
> - select ARM_GLOBAL_TIMER
> + select HAVE_ARM_GLOBAL_TIMER
> select ARM_PSCI
> select HAVE_ARM_ARCH_TIMER
> select HAVE_ARM_SCU
> diff --git a/arch/arm/mach-bcm/Kconfig b/arch/arm/mach-bcm/Kconfig
> index 7ef1214..d4d079a7 100644
> --- a/arch/arm/mach-bcm/Kconfig
> +++ b/arch/arm/mach-bcm/Kconfig
> @@ -14,7 +14,7 @@ config ARCH_BCM_IPROC
> select CACHE_L2X0
> select HAVE_ARM_SCU if SMP
> select HAVE_ARM_TWD if SMP
> - select ARM_GLOBAL_TIMER
> + select HAVE_ARM_GLOBAL_TIMER
> select COMMON_CLK_IPROC
> select CLKSRC_MMIO
> select ARCH_REQUIRE_GPIOLIB
> @@ -156,7 +156,7 @@ config ARCH_BCM_63XX
> select ARM_ERRATA_754322
> select ARM_ERRATA_764369 if SMP
> select ARM_GIC
> - select ARM_GLOBAL_TIMER
> + select HAVE_ARM_GLOBAL_TIMER
> select CACHE_L2X0
> select HAVE_ARM_ARCH_TIMER
> select HAVE_ARM_TWD if SMP
> diff --git a/arch/arm/mach-hisi/Kconfig b/arch/arm/mach-hisi/Kconfig
> index a3b091a..251bb03 100644
> --- a/arch/arm/mach-hisi/Kconfig
> +++ b/arch/arm/mach-hisi/Kconfig
> @@ -28,7 +28,7 @@ config ARCH_HIP01
> depends on ARCH_MULTI_V7
> select HAVE_ARM_SCU if SMP
> select HAVE_ARM_TWD if SMP
> - select ARM_GLOBAL_TIMER
> + select HAVE_ARM_GLOBAL_TIMER
> help
> Support for Hisilicon HIP01 SoC family
>
> diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
> index 8973fae..3fbf38c 100644
> --- a/arch/arm/mach-imx/Kconfig
> +++ b/arch/arm/mach-imx/Kconfig
> @@ -602,7 +602,7 @@ choice
> config VF_USE_ARM_GLOBAL_TIMER
> bool "Use ARM Global Timer"
> depends on ARCH_MULTI_V7
> - select ARM_GLOBAL_TIMER
> + select HAVE_ARM_GLOBAL_TIMER
> select CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
> help
> Use the ARM Global Timer as clocksource
> diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
> index cef42fd..a53b787 100644
> --- a/arch/arm/mach-rockchip/Kconfig
> +++ b/arch/arm/mach-rockchip/Kconfig
> @@ -14,7 +14,7 @@ config ARCH_ROCKCHIP
> select DW_APB_TIMER_OF
> select REGULATOR if PM
> select ROCKCHIP_TIMER
> - select ARM_GLOBAL_TIMER
> + select HAVE_ARM_GLOBAL_TIMER
> select CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
> help
> Support for Rockchip's Cortex-A9 Single-to-Quad-Core-SoCs
> diff --git a/arch/arm/mach-sti/Kconfig b/arch/arm/mach-sti/Kconfig
> index a196d14..c799f9d 100644
> --- a/arch/arm/mach-sti/Kconfig
> +++ b/arch/arm/mach-sti/Kconfig
> @@ -3,7 +3,7 @@ menuconfig ARCH_STI
> depends on ARCH_MULTI_V7
> select ARM_GIC
> select ST_IRQCHIP
> - select ARM_GLOBAL_TIMER
> + select HAVE_ARM_GLOBAL_TIMER
> select CLKSRC_ST_LPC
> select PINCTRL
> select PINCTRL_ST
> diff --git a/arch/arm/mach-uniphier/Kconfig b/arch/arm/mach-uniphier/Kconfig
> index 82dddee..81bdf77 100644
> --- a/arch/arm/mach-uniphier/Kconfig
> +++ b/arch/arm/mach-uniphier/Kconfig
> @@ -2,7 +2,7 @@ config ARCH_UNIPHIER
> bool "Socionext UniPhier SoCs"
> depends on ARCH_MULTI_V7
> select ARM_AMBA
> - select ARM_GLOBAL_TIMER
> + select HAVE_ARM_GLOBAL_TIMER
> select ARM_GIC
> select HAVE_ARM_SCU
> select HAVE_ARM_TWD if SMP
> diff --git a/arch/arm/mach-vexpress/Kconfig b/arch/arm/mach-vexpress/Kconfig
> index 398a297..e89941d 100644
> --- a/arch/arm/mach-vexpress/Kconfig
> +++ b/arch/arm/mach-vexpress/Kconfig
> @@ -5,7 +5,7 @@ menuconfig ARCH_VEXPRESS
> select ARCH_SUPPORTS_BIG_ENDIAN
> select ARM_AMBA
> select ARM_GIC
> - select ARM_GLOBAL_TIMER
> + select HAVE_ARM_GLOBAL_TIMER
> select ARM_TIMER_SP804
> select COMMON_CLK_VERSATILE
> select HAVE_ARM_SCU if SMP
> diff --git a/arch/arm/mach-zx/Kconfig b/arch/arm/mach-zx/Kconfig
> index 209c979..e84b0c2 100644
> --- a/arch/arm/mach-zx/Kconfig
> +++ b/arch/arm/mach-zx/Kconfig
> @@ -11,7 +11,7 @@ if ARCH_ZX
> config SOC_ZX296702
> def_bool y
> select ARM_GIC
> - select ARM_GLOBAL_TIMER
> + select HAVE_ARM_GLOBAL_TIMER
> select HAVE_ARM_SCU if SMP
> select HAVE_ARM_TWD if SMP
> select PM_GENERIC_DOMAINS if PM
> diff --git a/arch/arm/mach-zynq/Kconfig b/arch/arm/mach-zynq/Kconfig
> index fd0aeeb..3165720 100644
> --- a/arch/arm/mach-zynq/Kconfig
> +++ b/arch/arm/mach-zynq/Kconfig
> @@ -5,7 +5,7 @@ config ARCH_ZYNQ
> select ARCH_SUPPORTS_BIG_ENDIAN
> select ARM_AMBA
> select ARM_GIC
> - select ARM_GLOBAL_TIMER if !CPU_FREQ
> + select HAVE_ARM_GLOBAL_TIMER if !CPU_FREQ
> select CADENCE_TTC_TIMER
> select HAVE_ARM_SCU if SMP
> select HAVE_ARM_TWD if SMP
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index c346be6..c686811 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -207,9 +207,14 @@ config ARM_ARCH_TIMER_EVTSTREAM
> This must be disabled for hardware validation purposes to detect any
> hardware anomalies of missing events.
>
> -config ARM_GLOBAL_TIMER
> +config HAVE_ARM_GLOBAL_TIMER
> bool
> +
> +config ARM_GLOBAL_TIMER
> + bool "Support for ARM global timer unit"
> select CLKSRC_OF if OF
> + default y
> + depends on HAVE_ARM_GLOBAL_TIMER
> help
> This options enables support for the ARM global timer unit
>
> --
> 2.8.0
>

--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯

2016-04-26 19:37:55

by Grygorii Strashko

[permalink] [raw]
Subject: Re: [PATCH v1] ARM: clocksource: make ARM_GLOBAL_TIMER selectable

On 04/26/2016 07:02 PM, Liviu Dudau wrote:
> On Tue, Apr 26, 2016 at 06:28:52PM +0300, Grygorii Strashko wrote:
>
> Hi Grygorii,
>
> First time I'm seeing this patch, so I have a few questions, mostly
> related to the commit message:

Hm. You are in cc for RFC.
Sry, forgot to add link [1].

>
>> This patch intended to fix following cases:
>> - SoC-A has ARM GT, defines DT node for ARM GT and selects
>> ARM_GLOBAL_TIMER statically in Kconfig file. SoC-B has ARM GT and
>> defines DT node for ARM GT, but do not selects ARM_GLOBAL_TIMER
>> statically in Kconfig file. In case of multiplatform build ARM GT will
>> be implicitly enabled for SoC-B.
>
> Well, SoC-B has the GT *and* the DT node, so what is the problem with
> enabling it for SoC-B? If there are reasons not to use the Global Timer
> on SoC-B, surely a better option would be to mark it in DT with status = "disabled";

This was rejected [2]. DT describes HW and if it is functional the status = "disabled"
is not good choice.
ARM GT can't be used as clocksource/sched_clock/clockevent when CPUFreq or
CPUIdle are enabled :(, and this is Linux specific functionality and
not HW description.

>
>>
>> - There is no way to disable ARM GT without modifying Kconfig file,
>> once ARM_GLOBAL_TIMER is selected statically in Kconfig file.
>
> What about disabling the DT node?
>
> Not sure I properly understand the problem you are trying to solve here.

I'd like to have way to enable/disable ARM GT without modifying Kernel sources
(Kconfig specifically) which is now impossible.

>
>>
>> Hence, fix above case by defining both HAVE_ARM_GLOBAL_TIMER and
>> ARM_GLOBAL_TIMER as recommended by 'Adding common features and make
>> the usage configurable' section in kconfig-language.txt. All places in
>> ARM folder where ARM_GLOBAL_TIMER was used now replaced on
>> HAVE_ARM_GLOBAL_TIMER.
>
> I'm OK with the way you have changed ARM_GLOBAL_TIMER from a sticky config
> option to a selectable one, but I would like more details on the problem
> this was causing you.
>

The same HW (board) could be used with PM features enabled (power saving)
and disabled (-RT). Without this change it will require to have
and maintain two branches, but with it - just separate defconfig.

[1] http://lists.infradead.org/pipermail/linux-rockchip/2016-February/007159.html
[2] http://www.spinics.net/lists/devicetree/msg102918.html

>
>>
>> Cc: Daniel Lezcano <[email protected]>
>> Cc: Florian Fainelli <[email protected]>
>> Cc: Russell King <[email protected]>
>> Cc: Wei Xu <[email protected]>
>> Cc: Shawn Guo <[email protected]>
>> Cc: Sascha Hauer <[email protected]>
>> Cc: Srinivas Kandagatla <[email protected]>
>> Cc: Maxime Coquelin <[email protected]>
>> Cc: Masahiro Yamada <[email protected]>
>> Cc: Liviu Dudau <[email protected]>
>> Cc: Sudeep Holla <[email protected]>
>> Cc: Jun Nie <[email protected]>
>> Cc: Michal Simek <[email protected]>
>> Cc: Jesper Nilsson <[email protected]>
>> Cc: Lars Persson <[email protected]>
>> Cc: Mike Looijmans <[email protected]>
>> Acked-by: Sören Brinkmann <[email protected]>
>> Acked-by: Moritz Fischer <[email protected]>
>> Signed-off-by: Grygorii Strashko <[email protected]>
>> ---
>> Changes is v1:
>> - updated mach-artpec
>> - rebased on top of tip: timers/core
>> commit: 86d3473 time: Introduce do_sys_settimeofday64()
>>
>> arch/arm/mach-artpec/Kconfig | 2 +-
>> arch/arm/mach-bcm/Kconfig | 4 ++--
>> arch/arm/mach-hisi/Kconfig | 2 +-
>> arch/arm/mach-imx/Kconfig | 2 +-
>> arch/arm/mach-rockchip/Kconfig | 2 +-
>> arch/arm/mach-sti/Kconfig | 2 +-
>> arch/arm/mach-uniphier/Kconfig | 2 +-
>> arch/arm/mach-vexpress/Kconfig | 2 +-
>> arch/arm/mach-zx/Kconfig | 2 +-
>> arch/arm/mach-zynq/Kconfig | 2 +-
>> drivers/clocksource/Kconfig | 7 ++++++-
>> 11 files changed, 17 insertions(+), 12 deletions(-)
>>
>> diff --git a/arch/arm/mach-artpec/Kconfig b/arch/arm/mach-artpec/Kconfig
>> index 6cbe5a2..6cbca77 100644
>> --- a/arch/arm/mach-artpec/Kconfig
>> +++ b/arch/arm/mach-artpec/Kconfig
>> @@ -9,7 +9,7 @@ config MACH_ARTPEC6
>> depends on ARCH_MULTI_V7
>> select ARM_AMBA
>> select ARM_GIC
>> - select ARM_GLOBAL_TIMER

[...]

--
regards,
-grygorii

2016-04-27 10:12:22

by Liviu Dudau

[permalink] [raw]
Subject: Re: [PATCH v1] ARM: clocksource: make ARM_GLOBAL_TIMER selectable

On Tue, Apr 26, 2016 at 10:35:08PM +0300, Grygorii Strashko wrote:
> On 04/26/2016 07:02 PM, Liviu Dudau wrote:
> > On Tue, Apr 26, 2016 at 06:28:52PM +0300, Grygorii Strashko wrote:
> >
> > Hi Grygorii,
> >
> > First time I'm seeing this patch, so I have a few questions, mostly
> > related to the commit message:
>
> Hm. You are in cc for RFC.
> Sry, forgot to add link [1].

Hmm, possibly fell through some cracks.

>
> >
> >> This patch intended to fix following cases:
> >> - SoC-A has ARM GT, defines DT node for ARM GT and selects
> >> ARM_GLOBAL_TIMER statically in Kconfig file. SoC-B has ARM GT and
> >> defines DT node for ARM GT, but do not selects ARM_GLOBAL_TIMER
> >> statically in Kconfig file. In case of multiplatform build ARM GT will
> >> be implicitly enabled for SoC-B.
> >
> > Well, SoC-B has the GT *and* the DT node, so what is the problem with
> > enabling it for SoC-B? If there are reasons not to use the Global Timer
> > on SoC-B, surely a better option would be to mark it in DT with status = "disabled";
>
> This was rejected [2]. DT describes HW and if it is functional the status = "disabled"
> is not good choice.

They way I read that thread is that you are trying to disable the Global Timer
for the wrong reasons, not that having status = "disabled" is not a good choice.
DT *does* describe the HW, but there is nothing from stopping someone to say in the DT
that a specific HW is OFF. Removing the node description from DT would be wrong, because
the HW is there, we just want it disabled. Having status = "disabled" is similar to having
a software OFF button.

Now, the only issue one might have is when the HW is enabled at power on and active. In
that case, marking it with status = "disabled" is indeed wrong, because there's going
to be a disconnect between the state of the HW and what the DT says is being active. But
the timers are not really active until setup, so you should be able to play with the
status property.

> ARM GT can't be used as clocksource/sched_clock/clockevent when CPUFreq or
> CPUIdle are enabled :(, and this is Linux specific functionality and
> not HW description.

First of all you probably need to separate the issues you have with TWD versus
the GT timer, they are two different pieces of IP and have individual drivers.

When it comes to CPUidle, the TWD timer does the right thing: if there is no
"always-on" property in the DT then it will set the CLOCK_EVT_FEAT_C3STOP bit in
the features and so the clk framework will know that the clock will be disabled
when the core goes idle and will look for another clock source for wakeups. What
it does seem to be missing is the CLOCK_EVT_FEAT_PERCPU flag though, but that seems
to be a very underused flag in kernel/time/tick-broadcast.c.

As for the GT timer and CPUidle, something similar to the TWD timer needs done (i.e
set C3STOP feature if the clock is not marked as always on).

For the issues that you are seeing with CPUfreq, I'm afraid I need more info on that.

>
> >
> >>
> >> - There is no way to disable ARM GT without modifying Kconfig file,
> >> once ARM_GLOBAL_TIMER is selected statically in Kconfig file.
> >
> > What about disabling the DT node?
> >
> > Not sure I properly understand the problem you are trying to solve here.
>
> I'd like to have way to enable/disable ARM GT without modifying Kernel sources
> (Kconfig specifically) which is now impossible.
>
> >
> >>
> >> Hence, fix above case by defining both HAVE_ARM_GLOBAL_TIMER and
> >> ARM_GLOBAL_TIMER as recommended by 'Adding common features and make
> >> the usage configurable' section in kconfig-language.txt. All places in
> >> ARM folder where ARM_GLOBAL_TIMER was used now replaced on
> >> HAVE_ARM_GLOBAL_TIMER.
> >
> > I'm OK with the way you have changed ARM_GLOBAL_TIMER from a sticky config
> > option to a selectable one, but I would like more details on the problem
> > this was causing you.
> >
>
> The same HW (board) could be used with PM features enabled (power saving)
> and disabled (-RT). Without this change it will require to have
> and maintain two branches, but with it - just separate defconfig.

I'm not NAK-ing the patch for the moment until we understand if there are better
options (and patches) for your problem, but I still fail to see how you are going
to use this patch in the scenario that your commit message describes. If you compile
a kernel to work on both SoC-A and SoC-B you still end up having GT timer enabled for
SoC-B.

Maybe commit message needs changed to say something like "Make ARM_GLOBAL_TIMER a
selectable feature for builds that want to disable the functionality".

Best regards,
Liviu

>
> [1] http://lists.infradead.org/pipermail/linux-rockchip/2016-February/007159.html
> [2] http://www.spinics.net/lists/devicetree/msg102918.html
>
> >
> >>
> >> Cc: Daniel Lezcano <[email protected]>
> >> Cc: Florian Fainelli <[email protected]>
> >> Cc: Russell King <[email protected]>
> >> Cc: Wei Xu <[email protected]>
> >> Cc: Shawn Guo <[email protected]>
> >> Cc: Sascha Hauer <[email protected]>
> >> Cc: Srinivas Kandagatla <[email protected]>
> >> Cc: Maxime Coquelin <[email protected]>
> >> Cc: Masahiro Yamada <[email protected]>
> >> Cc: Liviu Dudau <[email protected]>
> >> Cc: Sudeep Holla <[email protected]>
> >> Cc: Jun Nie <[email protected]>
> >> Cc: Michal Simek <[email protected]>
> >> Cc: Jesper Nilsson <[email protected]>
> >> Cc: Lars Persson <[email protected]>
> >> Cc: Mike Looijmans <[email protected]>
> >> Acked-by: Sören Brinkmann <[email protected]>
> >> Acked-by: Moritz Fischer <[email protected]>
> >> Signed-off-by: Grygorii Strashko <[email protected]>
> >> ---
> >> Changes is v1:
> >> - updated mach-artpec
> >> - rebased on top of tip: timers/core
> >> commit: 86d3473 time: Introduce do_sys_settimeofday64()
> >>
> >> arch/arm/mach-artpec/Kconfig | 2 +-
> >> arch/arm/mach-bcm/Kconfig | 4 ++--
> >> arch/arm/mach-hisi/Kconfig | 2 +-
> >> arch/arm/mach-imx/Kconfig | 2 +-
> >> arch/arm/mach-rockchip/Kconfig | 2 +-
> >> arch/arm/mach-sti/Kconfig | 2 +-
> >> arch/arm/mach-uniphier/Kconfig | 2 +-
> >> arch/arm/mach-vexpress/Kconfig | 2 +-
> >> arch/arm/mach-zx/Kconfig | 2 +-
> >> arch/arm/mach-zynq/Kconfig | 2 +-
> >> drivers/clocksource/Kconfig | 7 ++++++-
> >> 11 files changed, 17 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/arch/arm/mach-artpec/Kconfig b/arch/arm/mach-artpec/Kconfig
> >> index 6cbe5a2..6cbca77 100644
> >> --- a/arch/arm/mach-artpec/Kconfig
> >> +++ b/arch/arm/mach-artpec/Kconfig
> >> @@ -9,7 +9,7 @@ config MACH_ARTPEC6
> >> depends on ARCH_MULTI_V7
> >> select ARM_AMBA
> >> select ARM_GIC
> >> - select ARM_GLOBAL_TIMER
>
> [...]
>
> --
> regards,
> -grygorii
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯

2016-04-27 10:15:57

by Robin Murphy

[permalink] [raw]
Subject: Re: [PATCH v1] ARM: clocksource: make ARM_GLOBAL_TIMER selectable

On 26/04/16 20:35, Grygorii Strashko wrote:
> On 04/26/2016 07:02 PM, Liviu Dudau wrote:
>> On Tue, Apr 26, 2016 at 06:28:52PM +0300, Grygorii Strashko wrote:
>>
>> Hi Grygorii,
>>
>> First time I'm seeing this patch, so I have a few questions, mostly
>> related to the commit message:
>
> Hm. You are in cc for RFC.
> Sry, forgot to add link [1].
>
>>
>>> This patch intended to fix following cases:
>>> - SoC-A has ARM GT, defines DT node for ARM GT and selects
>>> ARM_GLOBAL_TIMER statically in Kconfig file. SoC-B has ARM GT and
>>> defines DT node for ARM GT, but do not selects ARM_GLOBAL_TIMER
>>> statically in Kconfig file. In case of multiplatform build ARM GT will
>>> be implicitly enabled for SoC-B.
>>
>> Well, SoC-B has the GT *and* the DT node, so what is the problem with
>> enabling it for SoC-B? If there are reasons not to use the Global Timer
>> on SoC-B, surely a better option would be to mark it in DT with status = "disabled";
>
> This was rejected [2]. DT describes HW and if it is functional the status = "disabled"
> is not good choice.
> ARM GT can't be used as clocksource/sched_clock/clockevent when CPUFreq or
> CPUIdle are enabled :(, and this is Linux specific functionality and
> not HW description.

It sounds to me like the cleanest option might then be to address it
within the Linux-specific driver itself. How feasible would it be for
the GT driver to detect at runtime whether CPUfreq/idle will be used and
simply refuse to register as a clocksource if so? (I guess there are
probably horrible initialisation order issues, at least...)

Robin.

>>
>>>
>>> - There is no way to disable ARM GT without modifying Kconfig file,
>>> once ARM_GLOBAL_TIMER is selected statically in Kconfig file.
>>
>> What about disabling the DT node?
>>
>> Not sure I properly understand the problem you are trying to solve here.
>
> I'd like to have way to enable/disable ARM GT without modifying Kernel sources
> (Kconfig specifically) which is now impossible.
>
>>
>>>
>>> Hence, fix above case by defining both HAVE_ARM_GLOBAL_TIMER and
>>> ARM_GLOBAL_TIMER as recommended by 'Adding common features and make
>>> the usage configurable' section in kconfig-language.txt. All places in
>>> ARM folder where ARM_GLOBAL_TIMER was used now replaced on
>>> HAVE_ARM_GLOBAL_TIMER.
>>
>> I'm OK with the way you have changed ARM_GLOBAL_TIMER from a sticky config
>> option to a selectable one, but I would like more details on the problem
>> this was causing you.
>>
>
> The same HW (board) could be used with PM features enabled (power saving)
> and disabled (-RT). Without this change it will require to have
> and maintain two branches, but with it - just separate defconfig.
>
> [1] http://lists.infradead.org/pipermail/linux-rockchip/2016-February/007159.html
> [2] http://www.spinics.net/lists/devicetree/msg102918.html
>
>>
>>>
>>> Cc: Daniel Lezcano <[email protected]>
>>> Cc: Florian Fainelli <[email protected]>
>>> Cc: Russell King <[email protected]>
>>> Cc: Wei Xu <[email protected]>
>>> Cc: Shawn Guo <[email protected]>
>>> Cc: Sascha Hauer <[email protected]>
>>> Cc: Srinivas Kandagatla <[email protected]>
>>> Cc: Maxime Coquelin <[email protected]>
>>> Cc: Masahiro Yamada <[email protected]>
>>> Cc: Liviu Dudau <[email protected]>
>>> Cc: Sudeep Holla <[email protected]>
>>> Cc: Jun Nie <[email protected]>
>>> Cc: Michal Simek <[email protected]>
>>> Cc: Jesper Nilsson <[email protected]>
>>> Cc: Lars Persson <[email protected]>
>>> Cc: Mike Looijmans <[email protected]>
>>> Acked-by: Sören Brinkmann <[email protected]>
>>> Acked-by: Moritz Fischer <[email protected]>
>>> Signed-off-by: Grygorii Strashko <[email protected]>
>>> ---
>>> Changes is v1:
>>> - updated mach-artpec
>>> - rebased on top of tip: timers/core
>>> commit: 86d3473 time: Introduce do_sys_settimeofday64()
>>>
>>> arch/arm/mach-artpec/Kconfig | 2 +-
>>> arch/arm/mach-bcm/Kconfig | 4 ++--
>>> arch/arm/mach-hisi/Kconfig | 2 +-
>>> arch/arm/mach-imx/Kconfig | 2 +-
>>> arch/arm/mach-rockchip/Kconfig | 2 +-
>>> arch/arm/mach-sti/Kconfig | 2 +-
>>> arch/arm/mach-uniphier/Kconfig | 2 +-
>>> arch/arm/mach-vexpress/Kconfig | 2 +-
>>> arch/arm/mach-zx/Kconfig | 2 +-
>>> arch/arm/mach-zynq/Kconfig | 2 +-
>>> drivers/clocksource/Kconfig | 7 ++++++-
>>> 11 files changed, 17 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-artpec/Kconfig b/arch/arm/mach-artpec/Kconfig
>>> index 6cbe5a2..6cbca77 100644
>>> --- a/arch/arm/mach-artpec/Kconfig
>>> +++ b/arch/arm/mach-artpec/Kconfig
>>> @@ -9,7 +9,7 @@ config MACH_ARTPEC6
>>> depends on ARCH_MULTI_V7
>>> select ARM_AMBA
>>> select ARM_GIC
>>> - select ARM_GLOBAL_TIMER
>
> [...]
>

2016-04-27 10:41:38

by Russell King - ARM Linux

[permalink] [raw]
Subject: Re: [PATCH v1] ARM: clocksource: make ARM_GLOBAL_TIMER selectable

On Tue, Apr 26, 2016 at 10:35:08PM +0300, Grygorii Strashko wrote:
> On 04/26/2016 07:02 PM, Liviu Dudau wrote:
> > Well, SoC-B has the GT *and* the DT node, so what is the problem with
> > enabling it for SoC-B? If there are reasons not to use the Global Timer
> > on SoC-B, surely a better option would be to mark it in DT with status = "disabled";
>
> This was rejected [2]. DT describes HW and if it is functional the status = "disabled"
> is not good choice.
> ARM GT can't be used as clocksource/sched_clock/clockevent when CPUFreq or
> CPUIdle are enabled :(, and this is Linux specific functionality and
> not HW description.

Sorry, but we don't want to have to disable drivers in the kernel just
because one platform has a problem (consider the single zImage case
where it may be required that the global timer is enabled for some
platform to boot - it becomes mandatory in single zImage at that point.)

Maybe a linux-specific property is needed here - "linux,low-power-unstable"
or something like that?

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

2016-04-27 13:26:42

by Grygorii Strashko

[permalink] [raw]
Subject: Re: [PATCH v1] ARM: clocksource: make ARM_GLOBAL_TIMER selectable

On 04/27/2016 01:15 PM, Robin Murphy wrote:
> On 26/04/16 20:35, Grygorii Strashko wrote:
>> On 04/26/2016 07:02 PM, Liviu Dudau wrote:
>>> On Tue, Apr 26, 2016 at 06:28:52PM +0300, Grygorii Strashko wrote:
>>>
>>> Hi Grygorii,
>>>
>>> First time I'm seeing this patch, so I have a few questions, mostly
>>> related to the commit message:
>>
>> Hm. You are in cc for RFC.
>> Sry, forgot to add link [1].
>>
>>>
>>>> This patch intended to fix following cases:
>>>> - SoC-A has ARM GT, defines DT node for ARM GT and selects
>>>> ARM_GLOBAL_TIMER statically in Kconfig file. SoC-B has ARM GT and
>>>> defines DT node for ARM GT, but do not selects ARM_GLOBAL_TIMER
>>>> statically in Kconfig file. In case of multiplatform build ARM GT will
>>>> be implicitly enabled for SoC-B.
>>>
>>> Well, SoC-B has the GT *and* the DT node, so what is the problem with
>>> enabling it for SoC-B? If there are reasons not to use the Global Timer
>>> on SoC-B, surely a better option would be to mark it in DT with
>>> status = "disabled";
>>
>> This was rejected [2]. DT describes HW and if it is functional the
>> status = "disabled"
>> is not good choice.
>> ARM GT can't be used as clocksource/sched_clock/clockevent when
>> CPUFreq or
>> CPUIdle are enabled :(, and this is Linux specific functionality and
>> not HW description.
>
> It sounds to me like the cleanest option might then be to address it
> within the Linux-specific driver itself. How feasible would it be for
> the GT driver to detect at runtime whether CPUfreq/idle will be used and
> simply refuse to register as a clocksource if so? (I guess there are
> probably horrible initialisation order issues, at least...)
>

I do not thinK this is really possible, taking into account that
clockevent device might need to be registered very early and
PM features can be enabled/disabled dynamically.
For example, PM is enabled by loading module for TI am437x and am335x SoCs,
but ARM GT is initialized much earlier. Before loading PM module,
it is (theoretically) possible to change clocksource and
clockevent (unbind) devices, but there is no way to change sched_clock
(or timer_delay).

--
regards,
-grygorii

2016-04-27 13:32:54

by Grygorii Strashko

[permalink] [raw]
Subject: Re: [PATCH v1] ARM: clocksource: make ARM_GLOBAL_TIMER selectable

Hi Russell,

On 04/27/2016 01:41 PM, Russell King - ARM Linux wrote:
> On Tue, Apr 26, 2016 at 10:35:08PM +0300, Grygorii Strashko wrote:
>> On 04/26/2016 07:02 PM, Liviu Dudau wrote:
>>> Well, SoC-B has the GT *and* the DT node, so what is the problem with
>>> enabling it for SoC-B? If there are reasons not to use the Global Timer
>>> on SoC-B, surely a better option would be to mark it in DT with status = "disabled";
>>
>> This was rejected [2]. DT describes HW and if it is functional the status = "disabled"
>> is not good choice.
>> ARM GT can't be used as clocksource/sched_clock/clockevent when CPUFreq or
>> CPUIdle are enabled :(, and this is Linux specific functionality and
>> not HW description.
>
> Sorry, but we don't want to have to disable drivers in the kernel just
> because one platform has a problem (consider the single zImage case
> where it may be required that the global timer is enabled for some
> platform to boot - it becomes mandatory in single zImage at that point.)

Sorry, but this patch doesn't disable anything. It provides possibility
to do a custom build with disabled ARM GT driver without Kernel code modification -
in my case RT-kernel and non-RT Kernel should run on same HW and
RT kernel should use ARM GT as clocksource/sched_clock, but
non-RT Kernel shouldn't.

I think RT can't be part of single zImage - it make no sense.

>
> Maybe a linux-specific property is needed here - "linux,low-power-unstable"
> or something like that?
>

I've tried smth. like this [1]

And it is not "unstable", it's will just stop in C3 (CPUIdle) and there no possibility
(at least I've not found how to do this) to adjust Freq in case of CPUFreq.
[above is about clocksource/sched_clock]

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-November/386858.html

Oh :(. And it's really hard (I even would say - crazy task) to select
proper clocksource/clockevent/sched_clock and timer_delay (opt, arm)
when SoC provides few choices.

Last discussion:
https://lkml.org/lkml/2016/3/17/38

Prev discussions:
http://www.spinics.net/lists/arm-kernel/msg459803.html
http://www.spinics.net/lists/linux-omap/msg123658.html

--
regards,
-grygorii

2016-04-27 13:48:01

by Grygorii Strashko

[permalink] [raw]
Subject: Re: [PATCH v1] ARM: clocksource: make ARM_GLOBAL_TIMER selectable

On 04/27/2016 01:12 PM, Liviu Dudau wrote:
> On Tue, Apr 26, 2016 at 10:35:08PM +0300, Grygorii Strashko wrote:
>> On 04/26/2016 07:02 PM, Liviu Dudau wrote:
>>> On Tue, Apr 26, 2016 at 06:28:52PM +0300, Grygorii Strashko wrote:
>>>
>>> Hi Grygorii,
>>>
>>> First time I'm seeing this patch, so I have a few questions, mostly
>>> related to the commit message:
>>
>> Hm. You are in cc for RFC.
>> Sry, forgot to add link [1].
>
> Hmm, possibly fell through some cracks.
>
>>
>>>
>>>> This patch intended to fix following cases:
>>>> - SoC-A has ARM GT, defines DT node for ARM GT and selects
>>>> ARM_GLOBAL_TIMER statically in Kconfig file. SoC-B has ARM GT and
>>>> defines DT node for ARM GT, but do not selects ARM_GLOBAL_TIMER
>>>> statically in Kconfig file. In case of multiplatform build ARM GT will
>>>> be implicitly enabled for SoC-B.
>>>
>>> Well, SoC-B has the GT *and* the DT node, so what is the problem with
>>> enabling it for SoC-B? If there are reasons not to use the Global Timer
>>> on SoC-B, surely a better option would be to mark it in DT with status = "disabled";
>>
>> This was rejected [2]. DT describes HW and if it is functional the status = "disabled"
>> is not good choice.
>
> They way I read that thread is that you are trying to disable the Global Timer
> for the wrong reasons, not that having status = "disabled" is not a good choice.
> DT *does* describe the HW, but there is nothing from stopping someone to say in the DT
> that a specific HW is OFF. Removing the node description from DT would be wrong, because
> the HW is there, we just want it disabled. Having status = "disabled" is similar to having
> a software OFF button.
>
> Now, the only issue one might have is when the HW is enabled at power on and active. In
> that case, marking it with status = "disabled" is indeed wrong, because there's going
> to be a disconnect between the state of the HW and what the DT says is being active. But
> the timers are not really active until setup, so you should be able to play with the
> status property.

As I mentioned, I have one HW which can be used in two different configurations
and targeted to perform two different function (RT vs non-RT).
So, is it expected to burn/replace DT in the above case?

Do you replace DT (or u-boot) each time you load new kernel?


>
>> ARM GT can't be used as clocksource/sched_clock/clockevent when CPUFreq or
>> CPUIdle are enabled :(, and this is Linux specific functionality and
>> not HW description.
>
> First of all you probably need to separate the issues you have with TWD versus
> the GT timer, they are two different pieces of IP and have individual drivers.

TWD? I did not mention TWD anywhere here.

>
> When it comes to CPUidle, the TWD timer does the right thing: if there is no
> "always-on" property in the DT then it will set the CLOCK_EVT_FEAT_C3STOP bit in
> the features and so the clk framework will know that the clock will be disabled
> when the core goes idle and will look for another clock source for wakeups. What
> it does seem to be missing is the CLOCK_EVT_FEAT_PERCPU flag though, but that seems
> to be a very underused flag in kernel/time/tick-broadcast.c.
>
> As for the GT timer and CPUidle, something similar to the TWD timer needs done (i.e
> set C3STOP feature if the clock is not marked as always on).

Oh :) There are big difference between clocksource and clockevent devices :(
I assume you are talking about clockevent devices then yes, for these kind
of timers the broadcast timer is used as backup timer in CPUIdle states.
(and TWD is always selected now in favor of ARM GT clockevent device on my HW)

But, I need and use ARM GT as clocksource/sched_clock - for these type of timers
no backup timers can be registered.
Last try to add such functionality was rejected [1]


>
> For the issues that you are seeing with CPUfreq, I'm afraid I need more info on that.
>
>>
>>>
>>>>
>>>> - There is no way to disable ARM GT without modifying Kconfig file,
>>>> once ARM_GLOBAL_TIMER is selected statically in Kconfig file.
>>>
>>> What about disabling the DT node?
>>>
>>> Not sure I properly understand the problem you are trying to solve here.
>>
>> I'd like to have way to enable/disable ARM GT without modifying Kernel sources
>> (Kconfig specifically) which is now impossible.
>>
>>>
>>>>
>>>> Hence, fix above case by defining both HAVE_ARM_GLOBAL_TIMER and
>>>> ARM_GLOBAL_TIMER as recommended by 'Adding common features and make
>>>> the usage configurable' section in kconfig-language.txt. All places in
>>>> ARM folder where ARM_GLOBAL_TIMER was used now replaced on
>>>> HAVE_ARM_GLOBAL_TIMER.
>>>
>>> I'm OK with the way you have changed ARM_GLOBAL_TIMER from a sticky config
>>> option to a selectable one, but I would like more details on the problem
>>> this was causing you.
>>>
>>
>> The same HW (board) could be used with PM features enabled (power saving)
>> and disabled (-RT). Without this change it will require to have
>> and maintain two branches, but with it - just separate defconfig.
>
> I'm not NAK-ing the patch for the moment until we understand if there are better
> options (and patches) for your problem, but I still fail to see how you are going
> to use this patch in the scenario that your commit message describes. If you compile
> a kernel to work on both SoC-A and SoC-B you still end up having GT timer enabled for
> SoC-B.

ok. That fair. I'd need to drop first paragraph. But problem exist anyway

./arch/arm/mach-vexpress/Kconfig: select ARM_GLOBAL_TIMER
vs
./arch/arm/mach-zynq/Kconfig: select ARM_GLOBAL_TIMER if !CPU_FREQ
as example



>
> Maybe commit message needs changed to say something like "Make ARM_GLOBAL_TIMER a
> selectable feature for builds that want to disable the functionality".

I can do that. How about:
---
ARM: clocksource: Make ARM_GLOBAL_TIMER a selectable

There is no way now to disable ARM GT without modifying Kconfig file,
once ARM_GLOBAL_TIMER is selected statically in Kconfig file.

Hence, make ARM_GLOBAL_TIMER a selectable feature for builds that want
to disable that functionality by defining both HAVE_ARM_GLOBAL_TIMER and
ARM_GLOBAL_TIMER as recommended by 'Adding common features and make
the usage configurable' section in kconfig-language.txt. All places in
ARM folder where ARM_GLOBAL_TIMER was used now replaced on
HAVE_ARM_GLOBAL_TIMER.
---




[1] https://lkml.org/lkml/2015/7/6/65
[PATCH] clocksource: Allow toggling between runtime and persistent clocksource for idle


--
regards,
-grygorii

2016-04-27 16:25:01

by Tony Lindgren

[permalink] [raw]
Subject: Re: [PATCH v1] ARM: clocksource: make ARM_GLOBAL_TIMER selectable

* Grygorii Strashko <[email protected]> [160427 06:32]:
> Hi Russell,
>
> On 04/27/2016 01:41 PM, Russell King - ARM Linux wrote:
> > On Tue, Apr 26, 2016 at 10:35:08PM +0300, Grygorii Strashko wrote:
> >> On 04/26/2016 07:02 PM, Liviu Dudau wrote:
> >>> Well, SoC-B has the GT *and* the DT node, so what is the problem with
> >>> enabling it for SoC-B? If there are reasons not to use the Global Timer
> >>> on SoC-B, surely a better option would be to mark it in DT with status = "disabled";
> >>
> >> This was rejected [2]. DT describes HW and if it is functional the status = "disabled"
> >> is not good choice.
> >> ARM GT can't be used as clocksource/sched_clock/clockevent when CPUFreq or
> >> CPUIdle are enabled :(, and this is Linux specific functionality and
> >> not HW description.
> >
> > Sorry, but we don't want to have to disable drivers in the kernel just
> > because one platform has a problem (consider the single zImage case
> > where it may be required that the global timer is enabled for some
> > platform to boot - it becomes mandatory in single zImage at that point.)
>
> Sorry, but this patch doesn't disable anything. It provides possibility
> to do a custom build with disabled ARM GT driver without Kernel code modification -
> in my case RT-kernel and non-RT Kernel should run on same HW and
> RT kernel should use ARM GT as clocksource/sched_clock, but
> non-RT Kernel shouldn't.

How about a kernel cmdline option for both local timers to disable
the selected timer(s)?

Tony

2016-04-27 21:07:10

by Russell King - ARM Linux

[permalink] [raw]
Subject: Re: [PATCH v1] ARM: clocksource: make ARM_GLOBAL_TIMER selectable

On Wed, Apr 27, 2016 at 04:31:05PM +0300, Grygorii Strashko wrote:
> Sorry, but this patch doesn't disable anything. It provides possibility
> to do a custom build with disabled ARM GT driver without Kernel code
> modification - in my case RT-kernel and non-RT Kernel should run on same
> HW and RT kernel should use ARM GT as clocksource/sched_clock, but
> non-RT Kernel shouldn't.
>
> I think RT can't be part of single zImage - it make no sense.
>
> >
> > Maybe a linux-specific property is needed here - "linux,low-power-unstable"
> > or something like that?
> >
>
> I've tried smth. like this [1]
>
> And it is not "unstable", it's will just stop in C3 (CPUIdle) and there no possibility
> (at least I've not found how to do this) to adjust Freq in case of CPUFreq.
> [above is about clocksource/sched_clock]
>
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-November/386858.html

Don't think I'm going to follow URLs - I don't use a desktop mail
client, and as I'm several days behind with email, I don't have time
to go looking through a crappy pipermail web archive at the moment
either.

Anyway, why can't we use the priority system for clocksources? If
ARM GT is problematical, we need a way to register it with a lower
priority than the preferred time keeping source. That's exactly
what the clocksource priority field is for.

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