[PATCH] clocksource: arm_global_timer: Allow DT to specify already reset timer counter

[PATCH] clocksource: arm_global_timer: Allow DT to specify already reset timer counter

[Jassi Brar]

The GT counter is common to every core in a cluster. There is a usecase
when Linux is spawned by a 'master' firmware/OS running on some core of
the same cluster and the GT is used by the both.
 Linux, upon boot, resetting the GT counter is obviously fatal to the
other OS. So provide a way for DT to tell Linux if it's running in that
'slave' mode and must not reset the counter.

Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
 Documentation/devicetree/bindings/arm/global_timer.txt |  6 ++++++
 drivers/clocksource/arm_global_timer.c                 | 12 +++++++++---
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/global_timer.txt b/Documentation/devicetree/bindings/arm/global_timer.txt
index bdae3a8..bb897a9 100644
--- a/Documentation/devicetree/bindings/arm/global_timer.txt
+++ b/Documentation/devicetree/bindings/arm/global_timer.txt
@@ -17,6 +17,12 @@
 
 - clocks : Should be phandle to a clock.
 
+
+** Optional properties:
+
+- arm,gt_no_reset : Firmware/bootloader already initialized the
+	global timer-counter and expects it to be not reset again.
+
 Example:
 
 	timer at 2c000600 {
diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c
index a2cb6fa..952bab6 100644
--- a/drivers/clocksource/arm_global_timer.c
+++ b/drivers/clocksource/arm_global_timer.c
@@ -51,6 +51,7 @@ static void __iomem *gt_base;
 static unsigned long gt_clk_rate;
 static int gt_ppi;
 static struct clock_event_device __percpu *gt_evt;
+static bool gt_reset_counter;
 
 /*
  * To get the value from the Global Timer Counter register proceed as follows:
@@ -212,9 +213,11 @@ static u64 notrace gt_sched_clock_read(void)
 
 static void __init gt_clocksource_init(void)
 {
-	writel(0, gt_base + GT_CONTROL);
-	writel(0, gt_base + GT_COUNTER0);
-	writel(0, gt_base + GT_COUNTER1);
+	if (gt_reset_counter) {
+		writel(0, gt_base + GT_CONTROL);
+		writel(0, gt_base + GT_COUNTER0);
+		writel(0, gt_base + GT_COUNTER1);
+	}
 	/* enables timer on all the cores */
 	writel(GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL);
 
@@ -303,6 +306,9 @@ static void __init global_timer_of_register(struct device_node *np)
 		goto out_irq;
 	}
 
+	/* See if we are told we can't reset the global timer counter */
+	gt_reset_counter = !of_property_read_bool(np, "arm,gt_no_reset");
+
 	/* Immediately configure the timer on the boot CPU */
 	gt_clocksource_init();
 	gt_clockevents_init(this_cpu_ptr(gt_evt));
-- 
1.9.1

[PATCH] clocksource: arm_global_timer: Allow DT to specify already reset timer counter

[Daniel Lezcano]

On 11/25/2015 06:24 AM, Jassi Brar wrote:
> The GT counter is common to every core in a cluster. There is a usecase
> when Linux is spawned by a 'master' firmware/OS running on some core of
> the same cluster and the GT is used by the both.
>   Linux, upon boot, resetting the GT counter is obviously fatal to the
> other OS. So provide a way for DT to tell Linux if it's running in that
> 'slave' mode and must not reset the counter.
>
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>

Hmm, I doubt that is a hardware description.

Rob ?

One comment below assuming this is acceptable.

> ---
>   Documentation/devicetree/bindings/arm/global_timer.txt |  6 ++++++
>   drivers/clocksource/arm_global_timer.c                 | 12 +++++++++---
>   2 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/arm/global_timer.txt b/Documentation/devicetree/bindings/arm/global_timer.txt
> index bdae3a8..bb897a9 100644
> --- a/Documentation/devicetree/bindings/arm/global_timer.txt
> +++ b/Documentation/devicetree/bindings/arm/global_timer.txt
> @@ -17,6 +17,12 @@
>
>   - clocks : Should be phandle to a clock.
>
> +
> +** Optional properties:
> +
> +- arm,gt_no_reset : Firmware/bootloader already initialized the
> +	global timer-counter and expects it to be not reset again.
> +
>   Example:
>
>   	timer at 2c000600 {
> diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c
> index a2cb6fa..952bab6 100644
> --- a/drivers/clocksource/arm_global_timer.c
> +++ b/drivers/clocksource/arm_global_timer.c
> @@ -51,6 +51,7 @@ static void __iomem *gt_base;
>   static unsigned long gt_clk_rate;
>   static int gt_ppi;
>   static struct clock_event_device __percpu *gt_evt;
> +static bool gt_reset_counter;

Defining a global static boolean just for a check in the init function 
is ... pointless. Adding a parameter to clocksource_init would make more 
sense.

>   /*
>    * To get the value from the Global Timer Counter register proceed as follows:
> @@ -212,9 +213,11 @@ static u64 notrace gt_sched_clock_read(void)
>
>   static void __init gt_clocksource_init(void)
>   {
> -	writel(0, gt_base + GT_CONTROL);
> -	writel(0, gt_base + GT_COUNTER0);
> -	writel(0, gt_base + GT_COUNTER1);
> +	if (gt_reset_counter) {
> +		writel(0, gt_base + GT_CONTROL);
> +		writel(0, gt_base + GT_COUNTER0);
> +		writel(0, gt_base + GT_COUNTER1);
> +	}
>   	/* enables timer on all the cores */
>   	writel(GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL);
>
> @@ -303,6 +306,9 @@ static void __init global_timer_of_register(struct device_node *np)
>   		goto out_irq;
>   	}
>
> +	/* See if we are told we can't reset the global timer counter */
> +	gt_reset_counter = !of_property_read_bool(np, "arm,gt_no_reset");
> +
>   	/* Immediately configure the timer on the boot CPU */
>   	gt_clocksource_init();
>   	gt_clockevents_init(this_cpu_ptr(gt_evt));
>


-- 
  <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH] clocksource: arm_global_timer: Allow DT to specify already reset timer counter

[Jassi Brar]

On Tue, Dec 15, 2015 at 11:32 PM, Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
> On 11/25/2015 06:24 AM, Jassi Brar wrote:
>>
>> The GT counter is common to every core in a cluster. There is a usecase
>> when Linux is spawned by a 'master' firmware/OS running on some core of
>> the same cluster and the GT is used by the both.
>>   Linux, upon boot, resetting the GT counter is obviously fatal to the
>> other OS. So provide a way for DT to tell Linux if it's running in that
>> 'slave' mode and must not reset the counter.
>>
>> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
>
>
> Hmm, I doubt that is a hardware description.
>
Its a regular 2core-1cluster platform. The master RTOS on cpu0
maintains the lifecycle of Linux on cpu1.

'No_Counter_Reset' is not a h/w thing, I agree, but I don't know how
to better convey that platform specific constraint.

>> diff --git a/drivers/clocksource/arm_global_timer.c
>> b/drivers/clocksource/arm_global_timer.c
>> index a2cb6fa..952bab6 100644
>> --- a/drivers/clocksource/arm_global_timer.c
>> +++ b/drivers/clocksource/arm_global_timer.c
>> @@ -51,6 +51,7 @@ static void __iomem *gt_base;
>>   static unsigned long gt_clk_rate;
>>   static int gt_ppi;
>>   static struct clock_event_device __percpu *gt_evt;
>> +static bool gt_reset_counter;
>
>
> Defining a global static boolean just for a check in the init function is
> ... pointless. Adding a parameter to clocksource_init would make more sense.
>
Yes, indeed.

Thanks.

[PATCH] clocksource: arm_global_timer: Allow DT to specify already reset timer counter

[Daniel Lezcano]

On 12/16/2015 05:11 AM, Jassi Brar wrote:
> On Tue, Dec 15, 2015 at 11:32 PM, Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>> On 11/25/2015 06:24 AM, Jassi Brar wrote:
>>>
>>> The GT counter is common to every core in a cluster. There is a usecase
>>> when Linux is spawned by a 'master' firmware/OS running on some core of
>>> the same cluster and the GT is used by the both.
>>>    Linux, upon boot, resetting the GT counter is obviously fatal to the
>>> other OS. So provide a way for DT to tell Linux if it's running in that
>>> 'slave' mode and must not reset the counter.
>>>
>>> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
>>
>>
>> Hmm, I doubt that is a hardware description.
>>
> Its a regular 2core-1cluster platform. The master RTOS on cpu0
> maintains the lifecycle of Linux on cpu1.
>
> 'No_Counter_Reset' is not a h/w thing, I agree, but I don't know how
> to better convey that platform specific constraint.

May be with a kernel parameter ?

-- 
  <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH] clocksource: arm_global_timer: Allow DT to specify already reset timer counter

[Jassi Brar]

On 16 December 2015 at 12:29, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> On 12/16/2015 05:11 AM, Jassi Brar wrote:
>>
>> On Tue, Dec 15, 2015 at 11:32 PM, Daniel Lezcano
>> <daniel.lezcano@linaro.org> wrote:
>>>
>>> On 11/25/2015 06:24 AM, Jassi Brar wrote:
>>>>
>>>>
>>>> The GT counter is common to every core in a cluster. There is a usecase
>>>> when Linux is spawned by a 'master' firmware/OS running on some core of
>>>> the same cluster and the GT is used by the both.
>>>>    Linux, upon boot, resetting the GT counter is obviously fatal to the
>>>> other OS. So provide a way for DT to tell Linux if it's running in that
>>>> 'slave' mode and must not reset the counter.
>>>>
>>>> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
>>>
>>>
>>>
>>> Hmm, I doubt that is a hardware description.
>>>
>> Its a regular 2core-1cluster platform. The master RTOS on cpu0
>> maintains the lifecycle of Linux on cpu1.
>>
>> 'No_Counter_Reset' is not a h/w thing, I agree, but I don't know how
>> to better convey that platform specific constraint.
>
>
> May be with a kernel parameter ?
>
The fact, that arch-timer driver does it via DT (see commit
65b5732d241b8), changes anything? :)

Thanks.

[PATCH] clocksource: arm_global_timer: Allow DT to specify already reset timer counter

[Marc Zyngier]

On 17/12/15 09:37, Jassi Brar wrote:
> On 16 December 2015 at 12:29, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>> On 12/16/2015 05:11 AM, Jassi Brar wrote:
>>>
>>> On Tue, Dec 15, 2015 at 11:32 PM, Daniel Lezcano
>>> <daniel.lezcano@linaro.org> wrote:
>>>>
>>>> On 11/25/2015 06:24 AM, Jassi Brar wrote:
>>>>>
>>>>>
>>>>> The GT counter is common to every core in a cluster. There is a usecase
>>>>> when Linux is spawned by a 'master' firmware/OS running on some core of
>>>>> the same cluster and the GT is used by the both.
>>>>>    Linux, upon boot, resetting the GT counter is obviously fatal to the
>>>>> other OS. So provide a way for DT to tell Linux if it's running in that
>>>>> 'slave' mode and must not reset the counter.
>>>>>
>>>>> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
>>>>
>>>>
>>>>
>>>> Hmm, I doubt that is a hardware description.
>>>>
>>> Its a regular 2core-1cluster platform. The master RTOS on cpu0
>>> maintains the lifecycle of Linux on cpu1.
>>>
>>> 'No_Counter_Reset' is not a h/w thing, I agree, but I don't know how
>>> to better convey that platform specific constraint.
>>
>>
>> May be with a kernel parameter ?
>>
> The fact, that arch-timer driver does it via DT (see commit
> 65b5732d241b8), changes anything? :)

This is a horrible, platform specific hack. I wish we could have pushed
back on this by forcing people to fix their firmware. Why can't you
simply get the firmware to *remove* the node from your device tree
altogether?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH] clocksource: arm_global_timer: Allow DT to specify already reset timer counter

[Jassi Brar]

On 17 December 2015 at 15:38, Marc Zyngier <marc.zyngier@arm.com> wrote:
...
> Why can't you
> simply get the firmware to *remove* the node from your device tree
> altogether?
>
... but Linux (on cpu1) and the other OS (on cpu0) both _need_ to use the GT.

Thanks.

[PATCH] clocksource: arm_global_timer: Allow DT to specify already reset timer counter

[Daniel Lezcano]

On 12/17/2015 10:37 AM, Jassi Brar wrote:
> On 16 December 2015 at 12:29, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>> On 12/16/2015 05:11 AM, Jassi Brar wrote:
>>>
>>> On Tue, Dec 15, 2015 at 11:32 PM, Daniel Lezcano
>>> <daniel.lezcano@linaro.org> wrote:
>>>>
>>>> On 11/25/2015 06:24 AM, Jassi Brar wrote:
>>>>>
>>>>>
>>>>> The GT counter is common to every core in a cluster. There is a usecase
>>>>> when Linux is spawned by a 'master' firmware/OS running on some core of
>>>>> the same cluster and the GT is used by the both.
>>>>>     Linux, upon boot, resetting the GT counter is obviously fatal to the
>>>>> other OS. So provide a way for DT to tell Linux if it's running in that
>>>>> 'slave' mode and must not reset the counter.
>>>>>
>>>>> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
>>>>
>>>>
>>>>
>>>> Hmm, I doubt that is a hardware description.
>>>>
>>> Its a regular 2core-1cluster platform. The master RTOS on cpu0
>>> maintains the lifecycle of Linux on cpu1.
>>>
>>> 'No_Counter_Reset' is not a h/w thing, I agree, but I don't know how
>>> to better convey that platform specific constraint.
>>
>>
>> May be with a kernel parameter ?
>>
> The fact, that arch-timer driver does it via DT (see commit
> 65b5732d241b8), changes anything? :)

It is debatable (cc'ed Rob).

I acked it because the patch was on the edge between soft/hw in the 
description. But now it sounds like that created a precedence and we 
consider these DT options exceptions as acceptable.

I don't know if there is a better way than a kernel parameter but IMO it 
makes sense to pass this kind of options via the kernel command line as 
we can extend it easily for more options in the future.

Furthermore, I suspect this feature may be needed for other arch/plat. 
So having a generic option could be useful for future changes.

Thomas, do you have an opinion ?

Can we consider adding a set of kernel parameters instead of adding DT 
exceptions time to time ?

eg.

clocksource.reset = <0/1>
clocksource.virtual = <0/1>



-- 
  <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH] clocksource: arm_global_timer: Allow DT to specify already reset timer counter

[Jassi Brar]

On 17 December 2015 at 15:46, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:

> I don't know if there is a better way than a kernel parameter but IMO it
> makes sense to pass this kind of options via the kernel command line as we
> can extend it easily for more options in the future.
>
I agree, for such 'soft' properties, cmdline is a better option.

Thanks.

[PATCH] clocksource: arm_global_timer: Allow DT to specify already reset timer counter

[Mark Rutland]

On Thu, Dec 17, 2015 at 11:16:39AM +0100, Daniel Lezcano wrote:
> On 12/17/2015 10:37 AM, Jassi Brar wrote:
> >On 16 December 2015 at 12:29, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> >>On 12/16/2015 05:11 AM, Jassi Brar wrote:
> >>>
> >>>On Tue, Dec 15, 2015 at 11:32 PM, Daniel Lezcano
> >>><daniel.lezcano@linaro.org> wrote:
> >>>>
> >>>>On 11/25/2015 06:24 AM, Jassi Brar wrote:
> >>>>>
> >>>>>
> >>>>>The GT counter is common to every core in a cluster. There is a usecase
> >>>>>when Linux is spawned by a 'master' firmware/OS running on some core of
> >>>>>the same cluster and the GT is used by the both.
> >>>>>    Linux, upon boot, resetting the GT counter is obviously fatal to the
> >>>>>other OS. So provide a way for DT to tell Linux if it's running in that
> >>>>>'slave' mode and must not reset the counter.
> >>>>>
> >>>>>Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> >>>>
> >>>>
> >>>>
> >>>>Hmm, I doubt that is a hardware description.
> >>>>
> >>>Its a regular 2core-1cluster platform. The master RTOS on cpu0
> >>>maintains the lifecycle of Linux on cpu1.
> >>>
> >>>'No_Counter_Reset' is not a h/w thing, I agree, but I don't know how
> >>>to better convey that platform specific constraint.
> >>
> >>
> >>May be with a kernel parameter ?
> >>
> >The fact, that arch-timer driver does it via DT (see commit
> >65b5732d241b8), changes anything? :)
> 
> It is debatable (cc'ed Rob).
> 
> I acked it because the patch was on the edge between soft/hw in the
> description. But now it sounds like that created a precedence and we
> consider these DT options exceptions as acceptable.

While not strictly a HW property, it is a property of the system,
outside of the control of Linux. It's fine to place such things in the
DT when required.

I don't think it makes sense to have to use kernel commadn line
parameters for this.

> I don't know if there is a better way than a kernel parameter but
> IMO it makes sense to pass this kind of options via the kernel
> command line as we can extend it easily for more options in the
> future.
> 
> Furthermore, I suspect this feature may be needed for other
> arch/plat. So having a generic option could be useful for future
> changes.
> 
> Thomas, do you have an opinion ?
> 
> Can we consider adding a set of kernel parameters instead of adding
> DT exceptions time to time ?
> 
> eg.
> 
> clocksource.reset = <0/1>
> clocksource.virtual = <0/1>

I don't think you can have generic optins for this. What exactly you
need will depend on the specific clocksource and set of other agents
using it. Generic options won't adequately capture those details.

Thanks,
Mark.

[PATCH] clocksource: arm_global_timer: Allow DT to specify already reset timer counter

[Mark Rutland]

On Wed, Nov 25, 2015 at 10:54:04AM +0530, Jassi Brar wrote:
> The GT counter is common to every core in a cluster. There is a usecase
> when Linux is spawned by a 'master' firmware/OS running on some core of
> the same cluster and the GT is used by the both.
>  Linux, upon boot, resetting the GT counter is obviously fatal to the
> other OS. So provide a way for DT to tell Linux if it's running in that
> 'slave' mode and must not reset the counter.

Is the other OS only using the counter?

Or are there other porions of the global timer that it assumes its in
sole control of?

> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> ---
>  Documentation/devicetree/bindings/arm/global_timer.txt |  6 ++++++
>  drivers/clocksource/arm_global_timer.c                 | 12 +++++++++---
>  2 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/global_timer.txt b/Documentation/devicetree/bindings/arm/global_timer.txt
> index bdae3a8..bb897a9 100644
> --- a/Documentation/devicetree/bindings/arm/global_timer.txt
> +++ b/Documentation/devicetree/bindings/arm/global_timer.txt
> @@ -17,6 +17,12 @@
>  
>  - clocks : Should be phandle to a clock.
>  
> +
> +** Optional properties:
> +
> +- arm,gt_no_reset : Firmware/bootloader already initialized the
> +	global timer-counter and expects it to be not reset again.

s/_/-/ in property names.

No need for the "gt" prefix, this is on tthe glboal timer node.

It "no-counter-reset" seems like a better description, though that
really depends on how the other OS is using this.

Mark.

[PATCH] clocksource: arm_global_timer: Allow DT to specify already reset timer counter

[Jassi Brar]

On Tue, Dec 15, 2015 at 11:44 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Nov 25, 2015 at 10:54:04AM +0530, Jassi Brar wrote:
>> The GT counter is common to every core in a cluster. There is a usecase
>> when Linux is spawned by a 'master' firmware/OS running on some core of
>> the same cluster and the GT is used by the both.
>>  Linux, upon boot, resetting the GT counter is obviously fatal to the
>> other OS. So provide a way for DT to tell Linux if it's running in that
>> 'slave' mode and must not reset the counter.
>
> Is the other OS only using the counter?
>
I understand only the Global Counter and the (unused) PreScalar are
common to cores in a cluster.

> Or are there other porions of the global timer that it assumes its in
> sole control of?
>
More important than 'control' is the fact that the other OS boots
first. So it can not survive a reset to the global counter.

>> +
>> +** Optional properties:
>> +
>> +- arm,gt_no_reset : Firmware/bootloader already initialized the
>> +     global timer-counter and expects it to be not reset again.
>
> s/_/-/ in property names.
>
> No need for the "gt" prefix, this is on tthe glboal timer node.
>
OK

> It "no-counter-reset" seems like a better description, though that
> really depends on how the other OS is using this.
>
OK will change it to "no-counter-reset".

Thanks.