[PATCH] clocksource: arm_global_timer: Detect if gt is usable with CPU_FREQ

[PATCH] clocksource: arm_global_timer: Detect if gt is usable with CPU_FREQ

[Ola Jeppsson]

Some Cortex A9 CPU:s (e.g. zynq) have the tick tied to the CPU
frequency. On those CPU:s we cannot use the global-timer as a reliable
clocksource with CPU frequency scaling enabled since this is not
currently taken into account by the driver.

Add a "tied-to-cpu-freq" boolean to the global-timer dt node indicate
this condition.

When the global-timer register function sees this property return
immediately and don't register the clocksource.

Signed-off-by: Ola Jeppsson <ola@adapteva.com>
---
 Documentation/devicetree/bindings/arm/global_timer.txt | 4 ++++
 drivers/clocksource/arm_global_timer.c                 | 7 +++++++
 2 files changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/global_timer.txt b/Documentation/devicetree/bindings/arm/global_timer.txt
index bdae3a818793..465e02c17b5b 100644
--- a/Documentation/devicetree/bindings/arm/global_timer.txt
+++ b/Documentation/devicetree/bindings/arm/global_timer.txt
@@ -17,6 +17,10 @@
 
 - clocks : Should be phandle to a clock.
 
+** Timer node optional properties:
+
+- tied-to-cpu-freq : indicates that the timer scales with the CPU frequency.
+
 Example:
 
 	timer at 2c000600 {
diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c
index e6833771a716..8913ebda3f09 100644
--- a/drivers/clocksource/arm_global_timer.c
+++ b/drivers/clocksource/arm_global_timer.c
@@ -268,6 +268,13 @@ static void __init global_timer_of_register(struct device_node *np)
 		return;
 	}
 
+#ifdef CONFIG_CPU_FREQ
+	if (of_property_read_bool(np, "tied-to-cpu-freq")) {
+		pr_warn("global-timer: tied to cpu frequency, not supported with scaling\n");
+		return;
+	}
+#endif
+
 	gt_clk = of_clk_get(np, 0);
 	if (!IS_ERR(gt_clk)) {
 		err = clk_prepare_enable(gt_clk);
-- 
2.3.3

[PATCH] clocksource: arm_global_timer: Detect if gt is usable with CPU_FREQ

[Mark Rutland]

On Mon, Apr 13, 2015 at 06:37:29PM +0100, Ola Jeppsson wrote:
> Some Cortex A9 CPU:s (e.g. zynq) have the tick tied to the CPU
> frequency. On those CPU:s we cannot use the global-timer as a reliable
> clocksource with CPU frequency scaling enabled since this is not
> currently taken into account by the driver.

The global timer is _always_ tied to the CPU frequency -- it's fed by
PERIPHCLK, which must have a period which is a multiple of CLK.

> Add a "tied-to-cpu-freq" boolean to the global-timer dt node indicate
> this condition.

Is this indicating the truism above, or that the kernel is performing
frequency scaling? The latter is an OS property rather than a HW
property, and thus doesn't belong in the DT.

The SMP TWD driver tries to account for frequency scaling. Assuming that
works, the same should be possible here. Otherwise, the global timer is
simply unusable with any frequency scaling.

In either case the property seems pointless; all you care about is
whether the platform actually performs CPU frequency scaling.

Mark.

> 
> When the global-timer register function sees this property return
> immediately and don't register the clocksource.
> 
> Signed-off-by: Ola Jeppsson <ola@adapteva.com>
> ---
>  Documentation/devicetree/bindings/arm/global_timer.txt | 4 ++++
>  drivers/clocksource/arm_global_timer.c                 | 7 +++++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/global_timer.txt b/Documentation/devicetree/bindings/arm/global_timer.txt
> index bdae3a818793..465e02c17b5b 100644
> --- a/Documentation/devicetree/bindings/arm/global_timer.txt
> +++ b/Documentation/devicetree/bindings/arm/global_timer.txt
> @@ -17,6 +17,10 @@
>  
>  - clocks : Should be phandle to a clock.
>  
> +** Timer node optional properties:
> +
> +- tied-to-cpu-freq : indicates that the timer scales with the CPU frequency.
> +
>  Example:
>  
>  	timer at 2c000600 {
> diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c
> index e6833771a716..8913ebda3f09 100644
> --- a/drivers/clocksource/arm_global_timer.c
> +++ b/drivers/clocksource/arm_global_timer.c
> @@ -268,6 +268,13 @@ static void __init global_timer_of_register(struct device_node *np)
>  		return;
>  	}
>  
> +#ifdef CONFIG_CPU_FREQ
> +	if (of_property_read_bool(np, "tied-to-cpu-freq")) {
> +		pr_warn("global-timer: tied to cpu frequency, not supported with scaling\n");
> +		return;
> +	}
> +#endif
> +
>  	gt_clk = of_clk_get(np, 0);
>  	if (!IS_ERR(gt_clk)) {
>  		err = clk_prepare_enable(gt_clk);
> -- 
> 2.3.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH] clocksource: arm_global_timer: Detect if gt is usable with CPU_FREQ

[Srinivas Kandagatla]

+Adding Pete and Maxime

Hi Ola,
Thankyou for sending the patch,

I like the Idea, but I have some specific concerns which would break 
existing SOCs.

On 13/04/15 18:37, Ola Jeppsson wrote:
> Some Cortex A9 CPU:s (e.g. zynq) have the tick tied to the CPU
> frequency. On those CPU:s we cannot use the global-timer as a reliable
> clocksource with CPU frequency scaling enabled since this is not
> currently taken into account by the driver.
>
> Add a "tied-to-cpu-freq" boolean to the global-timer dt node indicate
> this condition.
>
> When the global-timer register function sees this property return
> immediately and don't register the clocksource.
>
> Signed-off-by: Ola Jeppsson <ola@adapteva.com>
> ---
>   Documentation/devicetree/bindings/arm/global_timer.txt | 4 ++++
>   drivers/clocksource/arm_global_timer.c                 | 7 +++++++
>   2 files changed, 11 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/global_timer.txt b/Documentation/devicetree/bindings/arm/global_timer.txt
> index bdae3a818793..465e02c17b5b 100644
> --- a/Documentation/devicetree/bindings/arm/global_timer.txt
> +++ b/Documentation/devicetree/bindings/arm/global_timer.txt
> @@ -17,6 +17,10 @@
>
>   - clocks : Should be phandle to a clock.
>
> +** Timer node optional properties:
> +
> +- tied-to-cpu-freq : indicates that the timer scales with the CPU frequency.
> +
>   Example:
>
>   	timer at 2c000600 {
> diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c
> index e6833771a716..8913ebda3f09 100644
> --- a/drivers/clocksource/arm_global_timer.c
> +++ b/drivers/clocksource/arm_global_timer.c
> @@ -268,6 +268,13 @@ static void __init global_timer_of_register(struct device_node *np)
>   		return;
>   	}
>
> +#ifdef CONFIG_CPU_FREQ
> +	if (of_property_read_bool(np, "tied-to-cpu-freq")) {
> +		pr_warn("global-timer: tied to cpu frequency, not supported with scaling\n");
> +		return;
> +	}
> +#endif
> +

This patch would not let the SOC like STiH415/416 or zynq with 
"tied-to-cpu-freq" property to boot with multi_v7_defconfig. Which is 
not correct thing to do, as STi SOC's do not use cpufreq driver however 
the tick is tied to this clocksource.



--srini

>   	gt_clk = of_clk_get(np, 0);
>   	if (!IS_ERR(gt_clk)) {
>   		err = clk_prepare_enable(gt_clk);
>

[PATCH] clocksource: arm_global_timer: Detect if gt is usable with CPU_FREQ

[Maxime Coquelin]

Hi Srini, Ola,

On 04/14/2015 09:41 AM, Srinivas Kandagatla wrote:
> +Adding Pete and Maxime
>
> Hi Ola,
> Thankyou for sending the patch,
>
> I like the Idea, but I have some specific concerns which would break 
> existing SOCs.

I like the idea too.

>
>
> On 13/04/15 18:37, Ola Jeppsson wrote:
>> Some Cortex A9 CPU:s (e.g. zynq) have the tick tied to the CPU
>> frequency. On those CPU:s we cannot use the global-timer as a reliable
>> clocksource with CPU frequency scaling enabled since this is not
>> currently taken into account by the driver.
>>
>> Add a "tied-to-cpu-freq" boolean to the global-timer dt node indicate
>> this condition.
>>
>> When the global-timer register function sees this property return
>> immediately and don't register the clocksource.
>>
>> Signed-off-by: Ola Jeppsson <ola@adapteva.com>
>> ---
>>   Documentation/devicetree/bindings/arm/global_timer.txt | 4 ++++
>>   drivers/clocksource/arm_global_timer.c                 | 7 +++++++
>>   2 files changed, 11 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/global_timer.txt 
>> b/Documentation/devicetree/bindings/arm/global_timer.txt
>> index bdae3a818793..465e02c17b5b 100644
>> --- a/Documentation/devicetree/bindings/arm/global_timer.txt
>> +++ b/Documentation/devicetree/bindings/arm/global_timer.txt
>> @@ -17,6 +17,10 @@
>>
>>   - clocks : Should be phandle to a clock.
>>
>> +** Timer node optional properties:
>> +
>> +- tied-to-cpu-freq : indicates that the timer scales with the CPU 
>> frequency.
>> +
>>   Example:
>>
>>       timer at 2c000600 {
>> diff --git a/drivers/clocksource/arm_global_timer.c 
>> b/drivers/clocksource/arm_global_timer.c
>> index e6833771a716..8913ebda3f09 100644
>> --- a/drivers/clocksource/arm_global_timer.c
>> +++ b/drivers/clocksource/arm_global_timer.c
>> @@ -268,6 +268,13 @@ static void __init 
>> global_timer_of_register(struct device_node *np)
>>           return;
>>       }
>>
>> +#ifdef CONFIG_CPU_FREQ
>> +    if (of_property_read_bool(np, "tied-to-cpu-freq")) {
>> +        pr_warn("global-timer: tied to cpu frequency, not supported 
>> with scaling\n");
>> +        return;
>> +    }
>> +#endif
>> +
>
> This patch would not let the SOC like STiH415/416 or zynq with 
> "tied-to-cpu-freq" property to boot with multi_v7_defconfig. Which is 
> not correct thing to do, as STi SOC's do not use cpufreq driver 
> however the tick is tied to this clocksource.
Yes, you are right, but I don't see any cleaner way to do this.

On STi, we have another timer we can use as a clocksource when doing CPU 
Freq, the ST LPC timer.
It is not upstreamed yet, but we will try to have it for next release.

I propose we set the "tied-to-cpu-freq" in GT node of STi family as soon 
as we enable the LPC timer one.
Doing that, the STi boot won't break in multi_v7 config.

Kind regards,
Maxime

>
>
>
> --srini
>
>>       gt_clk = of_clk_get(np, 0);
>>       if (!IS_ERR(gt_clk)) {
>>           err = clk_prepare_enable(gt_clk);
>>

[PATCH] clocksource: arm_global_timer: Detect if gt is usable with CPU_FREQ

[Peter Griffin]

Hi Srini, Ola, Maxime,

On Tue, 14 Apr 2015, Maxime Coquelin wrote:
> On 04/14/2015 09:41 AM, Srinivas Kandagatla wrote:
> >+Adding Pete and Maxime

Thanks Srini, as I wouldn't have seen this otherwise.

<snip>
> >>+
> >
> >This patch would not let the SOC like STiH415/416 or zynq with
> >"tied-to-cpu-freq" property to boot with multi_v7_defconfig. Which
> >is not correct thing to do, as STi SOC's do not use cpufreq driver
> >however the tick is tied to this clocksource.
> Yes, you are right, but I don't see any cleaner way to do this.
> 
> On STi, we have another timer we can use as a clocksource when doing
> CPU Freq, the ST LPC timer.
> It is not upstreamed yet, but we will try to have it for next release.

I just sent a series to add the ST LPC timer to LKML, see here 
https://lkml.org/lkml/2015/4/17/181. I've tested this on stih410 and it works ok.

> I propose we set the "tied-to-cpu-freq" in GT node of STi family as
> soon as we enable the LPC timer one.
> Doing that, the STi boot won't break in multi_v7 config.

However this lpc timer IP does not exist on stih415/6 SoCs, and I can't (so far at least)
see any other timer which we could use. So basically we would still have a breakage on
stih415/6 STi platforms with this proposed change.

Upstreaming st_lpc.c timer is still worthwhile though as it paves the way for adding cpufreq
support upstream for stih407 family.

regards,

Peter.

[PATCH] clocksource: arm_global_timer: Detect if gt is usable with CPU_FREQ

[Sören Brinkmann]

On Tue, 2015-04-14 at 08:41AM +0100, Srinivas Kandagatla wrote:
> +Adding Pete and Maxime
> 
> Hi Ola,
> Thankyou for sending the patch,
> 
> I like the Idea, but I have some specific concerns which would break
> existing SOCs.
> 
> On 13/04/15 18:37, Ola Jeppsson wrote:
> >Some Cortex A9 CPU:s (e.g. zynq) have the tick tied to the CPU
> >frequency. On those CPU:s we cannot use the global-timer as a reliable
> >clocksource with CPU frequency scaling enabled since this is not
> >currently taken into account by the driver.
> >
> >Add a "tied-to-cpu-freq" boolean to the global-timer dt node indicate
> >this condition.
> >
> >When the global-timer register function sees this property return
> >immediately and don't register the clocksource.
> >
> >Signed-off-by: Ola Jeppsson <ola@adapteva.com>
> >---
> >  Documentation/devicetree/bindings/arm/global_timer.txt | 4 ++++
> >  drivers/clocksource/arm_global_timer.c                 | 7 +++++++
> >  2 files changed, 11 insertions(+)
> >
> >diff --git a/Documentation/devicetree/bindings/arm/global_timer.txt b/Documentation/devicetree/bindings/arm/global_timer.txt
> >index bdae3a818793..465e02c17b5b 100644
> >--- a/Documentation/devicetree/bindings/arm/global_timer.txt
> >+++ b/Documentation/devicetree/bindings/arm/global_timer.txt
> >@@ -17,6 +17,10 @@
> >
> >  - clocks : Should be phandle to a clock.
> >
> >+** Timer node optional properties:
> >+
> >+- tied-to-cpu-freq : indicates that the timer scales with the CPU frequency.
> >+
> >  Example:
> >
> >  	timer at 2c000600 {
> >diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c
> >index e6833771a716..8913ebda3f09 100644
> >--- a/drivers/clocksource/arm_global_timer.c
> >+++ b/drivers/clocksource/arm_global_timer.c
> >@@ -268,6 +268,13 @@ static void __init global_timer_of_register(struct device_node *np)
> >  		return;
> >  	}
> >
> >+#ifdef CONFIG_CPU_FREQ
> >+	if (of_property_read_bool(np, "tied-to-cpu-freq")) {
> >+		pr_warn("global-timer: tied to cpu frequency, not supported with scaling\n");
> >+		return;
> >+	}
> >+#endif
> >+
> 
> This patch would not let the SOC like STiH415/416 or zynq with
> "tied-to-cpu-freq" property to boot with multi_v7_defconfig. Which is not
> correct thing to do, as STi SOC's do not use cpufreq driver however the tick
> is tied to this clocksource.

For Zynq, it should be OK, since we have the cadence_ttc as alternative
clocksource. Though, I have to admit not having tested this patch.

	S?ren