clocksource: arm_global_timer: Allow DT to specify already reset timer counter

PATCH New Jassi Brar 5 participants 12 messages Nov 25, 2015 – Dec 17, 2015

Subsystems (3)

Device tree bindings Clocksource, clockevent drivers Arm/STI architecture

lore

Share & tools

lore

Jassi Brar

author

2015-11-25 05:24 UTC #0 lore raw

PATCH 1/1 clocksource: arm_global_timer: Allow DT to specify already reset timer counter

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 <[email protected]>

---
 Documentation/devicetree/bindings/arm/global_timer.txt |  6 ++++++
 drivers/clocksource/arm_global_timer.c                 | 12 +++++++++---
 2 files changed, 15 insertions(+), 3 deletions(-)

Documentation/devicetree/bindings/arm/global_timer.txt +6 -0

index bdae3a8..bb897a9 100644
@@ -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 {

drivers/clocksource/arm_global_timer.c +9 -3

index a2cb6fa..952bab6 100644
@@ -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));

Signature · 1 line

1.9.1

Daniel Lezcano

2015-12-15 18:02 UTC #1 lore raw

PATCH 1/1 clocksource: arm_global_timer: Allow DT to specify already reset timer counter

Quoted 8 lines · 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 <[email protected]>

Hmm, I doubt that is a hardware description.

Rob ?

One comment below assuming this is acceptable.

Quoted 27 lines

---
  Documentation/devicetree/bindings/arm/global_timer.txt |  6 ++++++
  drivers/clocksource/arm_global_timer.c                 | 12 +++++++++---
  2 files changed, 15 insertions(+), 3 deletions(-)

Documentation/devicetree/bindings/arm/global_timer.txt +6 -0

index bdae3a8..bb897a9 100644
@@ -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 {

drivers/clocksource/arm_global_timer.c +0 -0

index a2cb6fa..952bab6 100644

drivers/clocksource/arm_global_timer.c static void __iomem *gt_base; · line 54

Quoted 4 lines

drivers/clocksource/arm_global_timer.c +1 -0

@@ -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.

Quoted 28 lines

drivers/clocksource/arm_global_timer.c +8 -3

  /*
   * 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));

Signature · 5 lines

  <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

Jassi Brar

2015-12-16 04:11 UTC #2 lore raw

PATCH 1/1 clocksource: arm_global_timer: Allow DT to specify already reset timer counter

Quoted 9 lines · Daniel Lezcano

Quoted 8 lines · 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.

Quoted 5 lines

Quoted 1 line
Signed-off-by Jassi Brar <[email protected]>
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.

Quoted 6 lines

Quoted 6 lines
drivers/clocksource/arm_global_timer.c +0 -0
index a2cb6fa..952bab6 100644
@@ -51,6 +51,7 @@ static void __iomem *gt_base;

Quoted 9 lines

drivers/clocksource/arm_global_timer.c static void __iomem *gt_base; · line 54
Quoted 4 lines
drivers/clocksource/arm_global_timer.c +1 -0
  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.

Daniel Lezcano

2015-12-16 06:59 UTC #3 lore raw

PATCH 1/1 clocksource: arm_global_timer: Allow DT to specify already reset timer counter

Quoted 14 lines · Jassi Brar

Quoted 12 lines · Daniel Lezcano
Quoted 9 lines · 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 <[email protected]>

Quoted 7 lines

Quoted 2 lines
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 ?

Signature · 5 lines

  <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

Jassi Brar

author

2015-12-17 09:37 UTC #4 lore raw

PATCH 1/1 clocksource: arm_global_timer: Allow DT to specify already reset timer counter

Quoted 21 lines · Daniel Lezcano

Quoted 20 lines · Jassi Brar
Quoted 17 lines · Daniel Lezcano
Quoted 10 lines · 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 <[email protected]>
Hmm, I doubt that is a hardware description.

Quoted 9 lines

Quoted 5 lines
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.

Marc Zyngier

2015-12-17 10:08 UTC #5 lore raw

PATCH 1/1 clocksource: arm_global_timer: Allow DT to specify already reset timer counter

Quoted 25 lines · Jassi Brar

Quoted 24 lines · Daniel Lezcano
Quoted 23 lines · Jassi Brar
Quoted 11 lines · Daniel Lezcano
Quoted 9 lines · 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.
Quoted 6 lines
Quoted 1 line
Signed-off-by Jassi Brar <[email protected]>
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.

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
'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.

Signature · 1 line

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

Jassi Brar

author

2015-12-17 10:29 UTC #6 lore raw

PATCH 1/1 clocksource: arm_global_timer: Allow DT to specify already reset timer counter

On 17 December 2015 at 15:38, Marc Zyngier <[email protected]> wrote:
...

Quoted 4 lines

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.

Daniel Lezcano

2015-12-17 10:16 UTC #7 lore raw

PATCH 1/1 clocksource: arm_global_timer: Allow DT to specify already reset timer counter

In reply to Jassi Brar

Quoted 25 lines · Jassi Brar

Quoted 24 lines · Daniel Lezcano
Quoted 23 lines · Jassi Brar
Quoted 11 lines · Daniel Lezcano
Quoted 9 lines · 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.
Quoted 6 lines
Quoted 1 line
Signed-off-by Jassi Brar <[email protected]>
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.

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
'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>

Signature · 5 lines

  <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

Jassi Brar

author

2015-12-17 10:24 UTC #8 lore raw

PATCH 1/1 clocksource: arm_global_timer: Allow DT to specify already reset timer counter

Quoted 4 lines · Daniel Lezcano

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.

Mark Rutland

2015-12-17 11:02 UTC #9 lore raw

PATCH 1/1 clocksource: arm_global_timer: Allow DT to specify already reset timer counter

In reply to Daniel Lezcano

Quoted 32 lines · Daniel Lezcano

Quoted 31 lines · Jassi Brar
Quoted 30 lines · Daniel Lezcano
Quoted 18 lines · Jassi Brar
Quoted 15 lines · Daniel Lezcano
Quoted 10 lines · 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 <[email protected]>
Quoted 7 lines
Quoted 2 lines
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 ?

Quoted 8 lines

Quoted 2 lines
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.

Quoted 11 lines

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 ?

Quoted 7 lines

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.

Mark Rutland

2015-12-15 18:14 UTC #10 lore raw

PATCH 1/1 clocksource: arm_global_timer: Allow DT to specify already reset timer counter

In reply to Jassi Brar

Quoted 6 lines · 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.

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?

Quoted 11 lines

Signed-off-by Jassi Brar <[email protected]>
---
 Documentation/devicetree/bindings/arm/global_timer.txt |  6 ++++++
 drivers/clocksource/arm_global_timer.c                 | 12 +++++++++---
 2 files changed, 15 insertions(+), 3 deletions(-)
Documentation/devicetree/bindings/arm/global_timer.txt +0 -0
index bdae3a8..bb897a9 100644

Documentation/devicetree/bindings/arm/global_timer.txt line 24

Quoted 8 lines

Documentation/devicetree/bindings/arm/global_timer.txt +5 -0

@@ -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.

Jassi Brar

2015-12-16 04:17 UTC #11 lore raw

PATCH 1/1 clocksource: arm_global_timer: Allow DT to specify already reset timer counter

Quoted 10 lines · Mark Rutland

Quoted 6 lines · 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.
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.

Quoted 3 lines

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.

Quoted 10 lines

Documentation/devicetree/bindings/arm/global_timer.txt line 24
Quoted 5 lines
Documentation/devicetree/bindings/arm/global_timer.txt +5 -0
+
+** 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.

Quoted 3 lines

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.