Skip to contentSkip to conversation

clockevents for 4.10

GIT PULL In review Daniel Lezcano 4 participants 9 messages Nov 22, 2016 – Nov 23, 2016

Subsystems (6)
lore
Share & tools
lore
#0 lore raw

Hi Thomas, Ingo,

This pull request contains a few changes.

- Unmap the region obtained by of_iomap in case of error in the init path for the bcm2835 timer (Arvind Yadav).

- Add a boolean property in the DT for the arch_arm_timer in order to tell the driver the hardware implementation has an error because it does not belong to an always-on power domain (Brian Norris).

- Replace of_iomap() by of_io_request_and_map() for the arch_arm_timer (Stephen Boyd).

Thanks !

-- Daniel

The following changes since commit baa73d9e478ff32d62f3f9422822b59dd9a95a21:

posix-timers: Make them configurable (2016-11-16 09:26:35 +0100)

are available in the git repository at:

https://git.linaro.org/people/daniel.lezcano/linux.git

for you to fetch changes up to 84c39b8b7d46883f7a7514c7d55909831aa846fd:

clocksource/drivers/bcm2835_timer: Unmap region obtained by of_iomap (2016-11-22 09:22:59 +0100)

----------------------------------------------------------------
Arvind Yadav (1):
clocksource/drivers/bcm2835_timer: Unmap region obtained by of_iomap

Brian Norris (2):
clocksource/drivers/arm_arch_timer: Don't assume clock runs in suspend
arm64: dts: rockchip: Arch counter doesn't tick in system suspend

Stephen Boyd (1):
clocksource/drivers/arm_arch_timer: Map frame with of_io_request_and_map()

 Documentation/devicetree/bindings/arm/arch_timer.txt |  5 +++++
 arch/arm64/boot/dts/rockchip/rk3399.dtsi             |  1 +
 drivers/clocksource/arm_arch_timer.c                 | 14 +++++++++++---
 drivers/clocksource/bcm2835_timer.c                  | 14 ++++++++++----
 4 files changed, 27 insertions(+), 7 deletions(-)
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
#1 lore raw

PATCH 1/4 clocksource/drivers/arm_arch_timer: Don't assume clock runs in suspend

From: Brian Norris <[email protected]>

The ARM specifies that the system counter "must be implemented in an always-on power domain," and so we try to use the counter as a source of timekeeping across suspend/resume. Unfortunately, some SoCs (e.g., Rockchip's RK3399) do not keep the counter ticking properly when switched from their high-power clock to the lower-power clock used in system suspend. Support this quirk by adding a new device tree property.

---
 Documentation/devicetree/bindings/arm/arch_timer.txt | 5 +++++
 drivers/clocksource/arm_arch_timer.c                 | 9 ++++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/arm/arch_timer.txt +5 -0
index ef5fbe9..ad440a2 100644
@@ -38,6 +38,11 @@ to deliver its interrupts via SPIs.
   architecturally-defined reset values. Only supported for 32-bit
   systems which follow the ARMv7 architected reset values.
 
+- arm,no-tick-in-suspend : The main counter does not tick when the system is in
+  low-power system suspend on some SoCs. This behavior does not match the
+  Architecture Reference Manual's specification that the system counter "must
+  be implemented in an always-on power domain."
+
 
 Example:
 
drivers/clocksource/arm_arch_timer.c +8 -1
index 73c487d..a2503db 100644
@@ -81,6 +81,7 @@ static struct clock_event_device __percpu *arch_timer_evt;
 static enum ppi_nr arch_timer_uses_ppi = VIRT_PPI;
 static bool arch_timer_c3stop;
 static bool arch_timer_mem_use_virtual;
+static bool arch_counter_suspend_stop;
 
 static bool evtstrm_enable = IS_ENABLED(CONFIG_ARM_ARCH_TIMER_EVTSTREAM);
 
@@ -576,7 +577,7 @@ static struct clocksource clocksource_counter = {
 	.rating	= 400,
 	.read	= arch_counter_read,
 	.mask	= CLOCKSOURCE_MASK(56),
-	.flags	= CLOCK_SOURCE_IS_CONTINUOUS | CLOCK_SOURCE_SUSPEND_NONSTOP,
+	.flags	= CLOCK_SOURCE_IS_CONTINUOUS,
 };
 
 static struct cyclecounter cyclecounter = {
@@ -616,6 +617,8 @@ static void __init arch_counter_register(unsigned type)
 		arch_timer_read_counter = arch_counter_get_cntvct_mem;
 	}
 
+	if (!arch_counter_suspend_stop)
+		clocksource_counter.flags |= CLOCK_SOURCE_SUSPEND_NONSTOP;
 	start_count = arch_timer_read_counter();
 	clocksource_register_hz(&clocksource_counter, arch_timer_rate);
 	cyclecounter.mult = clocksource_counter.mult;
@@ -907,6 +910,10 @@ static int __init arch_timer_of_init(struct device_node *np)
 	    of_property_read_bool(np, "arm,cpu-registers-not-fw-configured"))
 		arch_timer_uses_ppi = PHYS_SECURE_PPI;
 
+	/* On some systems, the counter stops ticking when in suspend. */
+	arch_counter_suspend_stop = of_property_read_bool(np,
+							 "arm,no-tick-in-suspend");
+
 	return arch_timer_init();
 }
 CLOCKSOURCE_OF_DECLARE(armv7_arch_timer, "arm,armv7-timer", arch_timer_of_init);
Signature · 1 line
2.7.4
#2 lore raw

PATCH 2/4 arm64: dts: rockchip: Arch counter doesn't tick in system suspend

From: Brian Norris <[email protected]>

The "arm,no-tick-in-suspend" property was introduced to note implementations where the system counter does not quite follow the ARM specification that it "must be implemented in an always-on power domain".

Particularly, RK3399's counter stops ticking when we switch from the 24MHz clock to the 32KHz clock in low-power suspend, so let's mark it as such.

---
 arch/arm64/boot/dts/rockchip/rk3399.dtsi | 1 +
 1 file changed, 1 insertion(+)
arch/arm64/boot/dts/rockchip/rk3399.dtsi +1 -0
index b65c193..d85b651 100644
@@ -174,6 +174,7 @@
 			     <GIC_PPI 14 IRQ_TYPE_LEVEL_LOW 0>,
 			     <GIC_PPI 11 IRQ_TYPE_LEVEL_LOW 0>,
 			     <GIC_PPI 10 IRQ_TYPE_LEVEL_LOW 0>;
+		arm,no-tick-in-suspend;
 	};
 
 	xin24m: xin24m {
Signature · 1 line
2.7.4
maintainer Acked-by on v1 2/4
#3 lore raw

Re: [PATCH 2/4] arm64: dts: rockchip: Arch counter doesn't tick in system suspend

Am Dienstag, 22. November 2016, 10:44:22 schrieb Daniel Lezcano:

Quoted 7 lines

From: Brian Norris <[email protected]>

The "arm,no-tick-in-suspend" property was introduced to note implementations where the system counter does not quite follow the ARM specification that it "must be implemented in an always-on power domain".

Quoted 7 lines

Particularly, RK3399's counter stops ticking when we switch from the 24MHz clock to the 32KHz clock in low-power suspend, so let's mark it as such.

not sure if needed, but anyway

Quoted 16 lines
---
 arch/arm64/boot/dts/rockchip/rk3399.dtsi | 1 +
 1 file changed, 1 insertion(+)
(quoted diff) +0 -0

b/arch/arm64/boot/dts/rockchip/rk3399.dtsi index b65c193..d85b651 100644

arch/arm64/boot/dts/rockchip/rk3399.dtsi +1 -0
@@ -174,6 +174,7 @@
 			     <GIC_PPI 14 IRQ_TYPE_LEVEL_LOW 0>,
 			     <GIC_PPI 11 IRQ_TYPE_LEVEL_LOW 0>,
 			     <GIC_PPI 10 IRQ_TYPE_LEVEL_LOW 0>;
+		arm,no-tick-in-suspend;
 	};
 
 	xin24m: xin24m {
#4 lore raw

PATCH 3/4 clocksource/drivers/arm_arch_timer: Map frame with of_io_request_and_map()

In reply to Daniel Lezcano

From: Stephen Boyd <[email protected]>

Let's use the of_io_request_and_map() API so that the frame region is protected and shows up in /proc/iomem.

---
 drivers/clocksource/arm_arch_timer.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/clocksource/arm_arch_timer.c +3 -2
index a2503db..02fef68 100644
@@ -971,8 +971,9 @@ static int __init arch_timer_mem_init(struct device_node *np)
 	}
 
 	ret= -ENXIO;
-	base = arch_counter_base = of_iomap(best_frame, 0);
-	if (!base) {
+	base = arch_counter_base = of_io_request_and_map(best_frame, 0,
+							 "arch_mem_timer");
+	if (IS_ERR(base)) {
 		pr_err("arch_timer: Can't map frame's registers\n");
 		goto out;
 	}
Signature · 1 line
2.7.4
#5 lore raw

PATCH 4/4 clocksource/drivers/bcm2835_timer: Unmap region obtained by of_iomap

In reply to Daniel Lezcano

From: Arvind Yadav <[email protected]>

Free memory mapping, if bcm2835_timer_init is not successful.

---
 drivers/clocksource/bcm2835_timer.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)
drivers/clocksource/bcm2835_timer.c +10 -4
index e71acf2..f2f29d2 100644
@@ -96,7 +96,7 @@ static int __init bcm2835_timer_init(struct device_node *node)
 	ret = of_property_read_u32(node, "clock-frequency", &freq);
 	if (ret) {
 		pr_err("Can't read clock-frequency");
-		return ret;
+		goto err_iounmap;
 	}
 
 	system_clock = base + REG_COUNTER_LO;
@@ -108,13 +108,15 @@ static int __init bcm2835_timer_init(struct device_node *node)
 	irq = irq_of_parse_and_map(node, DEFAULT_TIMER);
 	if (irq <= 0) {
 		pr_err("Can't parse IRQ");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto err_iounmap;
 	}
 
 	timer = kzalloc(sizeof(*timer), GFP_KERNEL);
 	if (!timer) {
 		pr_err("Can't allocate timer struct\n");
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto err_iounmap;
 	}
 
 	timer->control = base + REG_CONTROL;
@@ -133,7 +135,7 @@ static int __init bcm2835_timer_init(struct device_node *node)
 	ret = setup_irq(irq, &timer->act);
 	if (ret) {
 		pr_err("Can't set up timer IRQ\n");
-		return ret;
+		goto err_iounmap;
 	}
 
 	clockevents_config_and_register(&timer->evt, freq, 0xf, 0xffffffff);
@@ -141,6 +143,10 @@ static int __init bcm2835_timer_init(struct device_node *node)
 	pr_info("bcm2835: system timer (irq = %d)\n", irq);
 
 	return 0;
+
+err_iounmap:
+	iounmap(base);
+	return ret;
 }
 CLOCKSOURCE_OF_DECLARE(bcm2835, "brcm,bcm2835-system-timer",
 			bcm2835_timer_init);
Signature · 1 line
2.7.4
maintainer Acked-by on v1 1/4
#6 lore raw

Re: [PATCH 1/4] clocksource/drivers/arm_arch_timer: Don't assume clock runs in suspend

In reply to Daniel Lezcano
Quoted 9 lines · Daniel Lezcano

From: Brian Norris <[email protected]>

The ARM specifies that the system counter "must be implemented in an always-on power domain," and so we try to use the counter as a source of timekeeping across suspend/resume. Unfortunately, some SoCs (e.g., Rockchip's RK3399) do not keep the counter ticking properly when switched from their high-power clock to the lower-power clock used in system suspend. Support this quirk by adding a new device tree property.

Quoted 6 lines
---
 Documentation/devicetree/bindings/arm/arch_timer.txt | 5 +++++
Quoted 2 lines
 drivers/clocksource/arm_arch_timer.c                 | 9 ++++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)
#7 lore raw
In reply to Daniel Lezcano
Quoted 19 lines · Daniel Lezcano

Hi Thomas, Ingo,

This pull request contains a few changes.

- Unmap the region obtained by of_iomap in case of error in the init path for the bcm2835 timer (Arvind Yadav).

- Add a boolean property in the DT for the arch_arm_timer in order to tell the driver the hardware implementation has an error because it does not belong to an always-on power domain (Brian Norris).

- Replace of_iomap() by of_io_request_and_map() for the arch_arm_timer (Stephen Boyd).

Thanks !

-- Daniel

Quoted 7 lines

The following changes since commit baa73d9e478ff32d62f3f9422822b59dd9a95a21:

posix-timers: Make them configurable (2016-11-16 09:26:35 +0100)

are available in the git repository at:

https://git.linaro.org/people/daniel.lezcano/linux.git

In branch clockevents/4.10 I presume?

Thanks,

Ingo

#8 lore raw
Quoted 24 lines · Ingo Molnar
Quoted 21 lines · Daniel Lezcano

Hi Thomas, Ingo,

This pull request contains a few changes.

- Unmap the region obtained by of_iomap in case of error in the init path for the bcm2835 timer (Arvind Yadav).

- Add a boolean property in the DT for the arch_arm_timer in order to tell the driver the hardware implementation has an error because it does not belong to an always-on power domain (Brian Norris).

- Replace of_iomap() by of_io_request_and_map() for the arch_arm_timer (Stephen Boyd).

Thanks !

-- Daniel

The following changes since commit baa73d9e478ff32d62f3f9422822b59dd9a95a21:

Quoted 7 lines
Quoted 5 lines

posix-timers: Make them configurable (2016-11-16 09:26:35 +0100)

are available in the git repository at:

https://git.linaro.org/people/daniel.lezcano/linux.git

In branch clockevents/4.10 I presume?

Ah, yes.

I'm surprised the branch did not show up with the request-pull script :/

-- Daniel

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