Skip to contentSkip to conversation

clocksource: sunxi: Timer fixes and cleanup

PATCH Applied Maxime Ripard 2 participants 12 messages Jul 11, 2013 – Jul 16, 2013

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

v5 cover clocksource: sunxi: Timer fixes and cleanup

Hi everyone,

The first timer code we merged when adding support for the A13 some time back was mostly a clean up from the source drop we had, without any documentation. This happened to work, but the code merged in turned out to be far from perfect, and had several flaws.

This patchset hopefully fixes these flaws, and cleanup most of the driver as well, to end up in an almost complete rewrite of it (even though it's not that long).

It also finally adds a clocksource driver using the second timer as our monotonic clock source.

These flaws have all been spotted when trying to add the A31 support, work that is still ongoing, but will hopefully benefit from this patchset as well.

Thanks,
Maxime

Changes from v4:
- Removed the last clk_get_rate() user
- Got a few beers

Changes from v3:
- Reintroduce the rate variable to cache the parent clock rate
- Remove the interval programming at probe time that was reintroduced in the v3 due to a poor rebase.

Changes from v2:
- Use the clocksource timer to get the amount of time we have to wait for when disabling and enabling back a timer
- Added patch to add parenthesis around the macros arguments
- Renamed the AUTORELOAD register define to the more meaningful RELOAD name

Changes from v1:
- Rebased on top of linux-next to benefit from the move to all architectures of the sched_clock functions
- Moved the clock source to the second timer instead of the 64 bits free-running counter like suggested by Thomas.

Maxime Ripard (10):
clocksource: sun4i: Use the BIT macros where possible
clocksource: sun4i: Wrap macros arguments in parenthesis
clocksource: sun4i: rename AUTORELOAD define to RELOAD
clocksource: sun4i: Add clocksource and sched clock drivers
clocksource: sun4i: Don't forget to enable the clock we use
clocksource: sun4i: Fix the next event code
clocksource: sun4i: Factor out some timer code
clocksource: sun4i: Remove TIMER_SCAL variable
clocksource: sun4i: Cleanup parent clock setup
clocksource: sun4i: Fix bug when switching from periodic to oneshot
modes

 drivers/clocksource/sun4i_timer.c | 110 +++++++++++++++++++++++++++-----------
 1 file changed, 78 insertions(+), 32 deletions(-)
Signature · 1 line
1.8.3.2
#1 lore raw

PATCH v5 1/10 clocksource: sun4i: Use the BIT macros where possible

---
 drivers/clocksource/sun4i_timer.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/clocksource/sun4i_timer.c +4 -4
index d4674e7..bdf34d9 100644
@@ -24,12 +24,12 @@
 #include <linux/of_irq.h>
 
 #define TIMER_IRQ_EN_REG	0x00
-#define TIMER_IRQ_EN(val)		(1 << val)
+#define TIMER_IRQ_EN(val)		BIT(val)
 #define TIMER_IRQ_ST_REG	0x04
 #define TIMER_CTL_REG(val)	(0x10 * val + 0x10)
-#define TIMER_CTL_ENABLE		(1 << 0)
-#define TIMER_CTL_AUTORELOAD		(1 << 1)
-#define TIMER_CTL_ONESHOT		(1 << 7)
+#define TIMER_CTL_ENABLE		BIT(0)
+#define TIMER_CTL_AUTORELOAD		BIT(1)
+#define TIMER_CTL_ONESHOT		BIT(7)
 #define TIMER_INTVAL_REG(val)	(0x10 * val + 0x14)
 #define TIMER_CNTVAL_REG(val)	(0x10 * val + 0x18)
 
Signature · 1 line
1.8.3.2
#2 lore raw

PATCH v5 2/10 clocksource: sun4i: Wrap macros arguments in parenthesis

In reply to Maxime Ripard

The macros were not using parenthesis to escape the arguments passed to them. It is pretty unsafe, so add those parenthesis.

---
 drivers/clocksource/sun4i_timer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/clocksource/sun4i_timer.c +2 -2
index bdf34d9..34ab658 100644
@@ -30,8 +30,8 @@
 #define TIMER_CTL_ENABLE		BIT(0)
 #define TIMER_CTL_AUTORELOAD		BIT(1)
 #define TIMER_CTL_ONESHOT		BIT(7)
-#define TIMER_INTVAL_REG(val)	(0x10 * val + 0x14)
-#define TIMER_CNTVAL_REG(val)	(0x10 * val + 0x18)
+#define TIMER_INTVAL_REG(val)	(0x10 * (val) + 0x14)
+#define TIMER_CNTVAL_REG(val)	(0x10 * (val) + 0x18)
 
 #define TIMER_SCAL		16
 
Signature · 1 line
1.8.3.2
#3 lore raw

PATCH v5 3/10 clocksource: sun4i: rename AUTORELOAD define to RELOAD

In reply to Maxime Ripard

The name AUTORELOAD was actually pretty bad since it doesn't make the register reload the previous interval when it expires, but setting this value pushes the new programmed interval to the internal timer counter. Rename it to RELOAD instead.

---
 drivers/clocksource/sun4i_timer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/clocksource/sun4i_timer.c +2 -2
index 34ab658..f5e227b 100644
@@ -28,7 +28,7 @@
 #define TIMER_IRQ_ST_REG	0x04
 #define TIMER_CTL_REG(val)	(0x10 * val + 0x10)
 #define TIMER_CTL_ENABLE		BIT(0)
-#define TIMER_CTL_AUTORELOAD		BIT(1)
+#define TIMER_CTL_RELOAD		BIT(1)
 #define TIMER_CTL_ONESHOT		BIT(7)
 #define TIMER_INTVAL_REG(val)	(0x10 * (val) + 0x14)
 #define TIMER_CNTVAL_REG(val)	(0x10 * (val) + 0x18)
@@ -129,7 +129,7 @@ static void __init sun4i_timer_init(struct device_node *node)
 
 	/* set mode to auto reload */
 	val = readl(timer_base + TIMER_CTL_REG(0));
-	writel(val | TIMER_CTL_AUTORELOAD, timer_base + TIMER_CTL_REG(0));
+	writel(val | TIMER_CTL_RELOAD, timer_base + TIMER_CTL_REG(0));
 
 	ret = setup_irq(irq, &sun4i_timer_irq);
 	if (ret)
Signature · 1 line
1.8.3.2
#4 lore raw

PATCH v5 4/10 clocksource: sun4i: Add clocksource and sched clock drivers

In reply to Maxime Ripard

Use the second timer found on the Allwinner SoCs as a clock source and sched clock, that were both not used yet on these platforms.

---
 drivers/clocksource/sun4i_timer.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
drivers/clocksource/sun4i_timer.c +15 -0
index f5e227b..b581c93 100644
@@ -19,6 +19,7 @@
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/irqreturn.h>
+#include <linux/sched_clock.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
@@ -96,6 +97,11 @@ static struct irqaction sun4i_timer_irq = {
 	.dev_id = &sun4i_clockevent,
 };
 
+static u32 sun4i_timer_sched_read(void)
+{
+	return ~readl(timer_base + TIMER_CNTVAL_REG(1));
+}
+
 static void __init sun4i_timer_init(struct device_node *node)
 {
 	unsigned long rate = 0;
@@ -117,6 +123,15 @@ static void __init sun4i_timer_init(struct device_node *node)
 
 	rate = clk_get_rate(clk);
 
+	writel(~0, timer_base + TIMER_INTVAL_REG(1));
+	writel(TIMER_CTL_ENABLE | TIMER_CTL_RELOAD |
+	       TIMER_CTL_CLK_SRC(TIMER_CTL_CLK_SRC_OSC24M),
+	       timer_base + TIMER_CTL_REG(1));
+
+	setup_sched_clock(sun4i_timer_sched_read, 32, rate);
+	clocksource_mmio_init(timer_base + TIMER_CNTVAL_REG(1), node->name,
+			      rate, 300, 32, clocksource_mmio_readl_down);
+
 	writel(rate / (TIMER_SCAL * HZ),
 	       timer_base + TIMER_INTVAL_REG(0));
 
Signature · 1 line
1.8.3.2
#5 lore raw

PATCH v5 5/10 clocksource: sun4i: Don't forget to enable the clock we use

In reply to Maxime Ripard

Even if in our case, this clock was non-gatable, used as a parent clock for several IPs, it still is a good idea to enable it.

---
 drivers/clocksource/sun4i_timer.c | 1 +
 1 file changed, 1 insertion(+)
drivers/clocksource/sun4i_timer.c +1 -0
index b581c93..8e9c651 100644
@@ -120,6 +120,7 @@ static void __init sun4i_timer_init(struct device_node *node)
 	clk = of_clk_get(node, 0);
 	if (IS_ERR(clk))
 		panic("Can't get timer clock");
+	clk_prepare_enable(clk);
 
 	rate = clk_get_rate(clk);
 
Signature · 1 line
1.8.3.2
#6 lore raw

PATCH v5 6/10 clocksource: sun4i: Fix the next event code

In reply to Maxime Ripard

The next_event logic was setting the next interval to fire in the current timer value instead of the interval value register, which is obviously wrong.

Plus, the logic to set the actual value was wrong as well: the interval register can only be modified when the timer is disabled, and then enable it back, otherwise, it'll have no effect. Fix this logic as well since that code couldn't possibly work.

---
 drivers/clocksource/sun4i_timer.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)
drivers/clocksource/sun4i_timer.c +22 -3
index 8e9c651..7123f65 100644
@@ -38,6 +38,20 @@
 
 static void __iomem *timer_base;
 
+/*
+ * When we disable a timer, we need to wait at least for 2 cycles of
+ * the timer source clock. We will use for that the clocksource timer
+ * that is already setup and runs at the same frequency than the other
+ * timers, and we never will be disabled.
+ */
+static void sun4i_clkevt_sync(void)
+{
+	u32 old = readl(timer_base + TIMER_CNTVAL_REG(1));
+
+	while ((old - readl(timer_base + TIMER_CNTVAL_REG(1))) < 3)
+		cpu_relax();
+}
+
 static void sun4i_clkevt_mode(enum clock_event_mode mode,
 			      struct clock_event_device *clk)
 {
@@ -63,9 +77,14 @@ static void sun4i_clkevt_mode(enum clock_event_mode mode,
 static int sun4i_clkevt_next_event(unsigned long evt,
 				   struct clock_event_device *unused)
 {
-	u32 u = readl(timer_base + TIMER_CTL_REG(0));
-	writel(evt, timer_base + TIMER_CNTVAL_REG(0));
-	writel(u | TIMER_CTL_ENABLE | TIMER_CTL_AUTORELOAD,
+	u32 val = readl(timer_base + TIMER_CTL_REG(0));
+	writel(val & ~TIMER_CTL_ENABLE, timer_base + TIMER_CTL_REG(0));
+	sun4i_clkevt_sync();
+
+	writel(evt, timer_base + TIMER_INTVAL_REG(0));
+
+	val = readl(timer_base + TIMER_CTL_REG(0));
+	writel(val | TIMER_CTL_ENABLE | TIMER_CTL_AUTORELOAD,
 	       timer_base + TIMER_CTL_REG(0));
 
 	return 0;
Signature · 1 line
1.8.3.2
#7 lore raw

PATCH v5 7/10 clocksource: sun4i: Factor out some timer code

In reply to Maxime Ripard

The set_next_event and set_mode callbacks share a lot of common code we can easily factor to avoid duplication and mistakes.

---
 drivers/clocksource/sun4i_timer.c | 48 ++++++++++++++++++++++++++-------------
 1 file changed, 32 insertions(+), 16 deletions(-)
drivers/clocksource/sun4i_timer.c +32 -16
index 7123f65..dd78b63 100644
@@ -52,24 +52,46 @@ static void sun4i_clkevt_sync(void)
 		cpu_relax();
 }
 
+static void sun4i_clkevt_time_stop(u8 timer)
+{
+	u32 val = readl(timer_base + TIMER_CTL_REG(timer));
+	writel(val & ~TIMER_CTL_ENABLE, timer_base + TIMER_CTL_REG(timer));
+	sun4i_clkevt_sync();
+}
+
+static void sun4i_clkevt_time_setup(u8 timer, unsigned long delay)
+{
+	writel(delay, timer_base + TIMER_INTVAL_REG(timer));
+}
+
+static void sun4i_clkevt_time_start(u8 timer, bool periodic)
+{
+	u32 val = readl(timer_base + TIMER_CTL_REG(timer));
+
+	if (periodic)
+		val &= ~TIMER_CTL_ONESHOT;
+	else
+		val |= TIMER_CTL_ONESHOT;
+
+	writel(val | TIMER_CTL_ENABLE, timer_base + TIMER_CTL_REG(timer));
+}
+
 static void sun4i_clkevt_mode(enum clock_event_mode mode,
 			      struct clock_event_device *clk)
 {
-	u32 u = readl(timer_base + TIMER_CTL_REG(0));
-
 	switch (mode) {
 	case CLOCK_EVT_MODE_PERIODIC:
-		u &= ~(TIMER_CTL_ONESHOT);
-		writel(u | TIMER_CTL_ENABLE, timer_base + TIMER_CTL_REG(0));
+		sun4i_clkevt_time_stop(0);
+		sun4i_clkevt_time_start(0, true);
 		break;
-
 	case CLOCK_EVT_MODE_ONESHOT:
-		writel(u | TIMER_CTL_ONESHOT, timer_base + TIMER_CTL_REG(0));
+		sun4i_clkevt_time_stop(0);
+		sun4i_clkevt_time_start(0, false);
 		break;
 	case CLOCK_EVT_MODE_UNUSED:
 	case CLOCK_EVT_MODE_SHUTDOWN:
 	default:
-		writel(u & ~(TIMER_CTL_ENABLE), timer_base + TIMER_CTL_REG(0));
+		sun4i_clkevt_time_stop(0);
 		break;
 	}
 }
@@ -77,15 +99,9 @@ static void sun4i_clkevt_mode(enum clock_event_mode mode,
 static int sun4i_clkevt_next_event(unsigned long evt,
 				   struct clock_event_device *unused)
 {
-	u32 val = readl(timer_base + TIMER_CTL_REG(0));
-	writel(val & ~TIMER_CTL_ENABLE, timer_base + TIMER_CTL_REG(0));
-	sun4i_clkevt_sync();
-
-	writel(evt, timer_base + TIMER_INTVAL_REG(0));
-
-	val = readl(timer_base + TIMER_CTL_REG(0));
-	writel(val | TIMER_CTL_ENABLE | TIMER_CTL_AUTORELOAD,
-	       timer_base + TIMER_CTL_REG(0));
+	sun4i_clkevt_time_stop(0);
+	sun4i_clkevt_time_setup(0, evt);
+	sun4i_clkevt_time_start(0, false);
 
 	return 0;
 }
Signature · 1 line
1.8.3.2
#8 lore raw

PATCH v5 8/10 clocksource: sun4i: Remove TIMER_SCAL variable

In reply to Maxime Ripard

The prescaler is only used when using the internal low frequency oscillator (at 32kHz). Since we're using the higher frequency oscillator at 24MHz, we can just remove it.

---
 drivers/clocksource/sun4i_timer.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)
drivers/clocksource/sun4i_timer.c +3 -6
index dd78b63..3217adc 100644
@@ -34,8 +34,6 @@
 #define TIMER_INTVAL_REG(val)	(0x10 * (val) + 0x14)
 #define TIMER_CNTVAL_REG(val)	(0x10 * (val) + 0x18)
 
-#define TIMER_SCAL		16
-
 static void __iomem *timer_base;
 
 /*
@@ -168,8 +166,7 @@ static void __init sun4i_timer_init(struct device_node *node)
 	clocksource_mmio_init(timer_base + TIMER_CNTVAL_REG(1), node->name,
 			      rate, 300, 32, clocksource_mmio_readl_down);
 
-	writel(rate / (TIMER_SCAL * HZ),
-	       timer_base + TIMER_INTVAL_REG(0));
+	writel(rate / HZ, timer_base + TIMER_INTVAL_REG(0));
 
 	/* set clock source to HOSC, 16 pre-division */
 	val = readl(timer_base + TIMER_CTL_REG(0));
@@ -192,8 +189,8 @@ static void __init sun4i_timer_init(struct device_node *node)
 
 	sun4i_clockevent.cpumask = cpumask_of(0);
 
-	clockevents_config_and_register(&sun4i_clockevent, rate / TIMER_SCAL,
-					0x1, 0xff);
+	clockevents_config_and_register(&sun4i_clockevent, rate, 0x1,
+					0xffffffff);
 }
 CLOCKSOURCE_OF_DECLARE(sun4i, "allwinner,sun4i-timer",
 		       sun4i_timer_init);
Signature · 1 line
1.8.3.2
#9 lore raw

PATCH v5 9/10 clocksource: sun4i: Cleanup parent clock setup

In reply to Maxime Ripard

The current bring-up code for the timer was overly complicated. The only thing we need is actually which clock we want to use as source and that's pretty much all. Let's keep it that way.

---
 drivers/clocksource/sun4i_timer.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)
drivers/clocksource/sun4i_timer.c +5 -10
index 3217adc..2fadb3b 100644
@@ -30,6 +30,9 @@
 #define TIMER_CTL_REG(val)	(0x10 * val + 0x10)
 #define TIMER_CTL_ENABLE		BIT(0)
 #define TIMER_CTL_RELOAD		BIT(1)
+#define TIMER_CTL_CLK_SRC(val)		(((val) & 0x3) << 2)
+#define TIMER_CTL_CLK_SRC_OSC24M		(1)
+#define TIMER_CTL_CLK_PRES(val)		(((val) & 0x7) << 4)
 #define TIMER_CTL_ONESHOT		BIT(7)
 #define TIMER_INTVAL_REG(val)	(0x10 * (val) + 0x14)
 #define TIMER_CNTVAL_REG(val)	(0x10 * (val) + 0x18)
@@ -168,16 +171,8 @@ static void __init sun4i_timer_init(struct device_node *node)
 
 	writel(rate / HZ, timer_base + TIMER_INTVAL_REG(0));
 
-	/* set clock source to HOSC, 16 pre-division */
-	val = readl(timer_base + TIMER_CTL_REG(0));
-	val &= ~(0x07 << 4);
-	val &= ~(0x03 << 2);
-	val |= (4 << 4) | (1 << 2);
-	writel(val, timer_base + TIMER_CTL_REG(0));
-
-	/* set mode to auto reload */
-	val = readl(timer_base + TIMER_CTL_REG(0));
-	writel(val | TIMER_CTL_RELOAD, timer_base + TIMER_CTL_REG(0));
+	writel(TIMER_CTL_CLK_SRC(TIMER_CTL_CLK_SRC_OSC24M) | TIMER_CTL_RELOAD,
+	       timer_base + TIMER_CTL_REG(0));
 
 	ret = setup_irq(irq, &sun4i_timer_irq);
 	if (ret)
Signature · 1 line
1.8.3.2
#10 lore raw

PATCH v5 10/10 clocksource: sun4i: Fix bug when switching from periodic to oneshot modes

In reply to Maxime Ripard

The interval was firing at was set up at probe time, and only changed in the set_next_event, and never changed back, which is not really what is expected.

When enabling the periodic mode, now set an interval to tick every jiffy.

---
 drivers/clocksource/sun4i_timer.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/clocksource/sun4i_timer.c +6 -3
index 2fadb3b..8ead025 100644
@@ -38,6 +38,7 @@
 #define TIMER_CNTVAL_REG(val)	(0x10 * (val) + 0x18)
 
 static void __iomem *timer_base;
+static u32 ticks_per_jiffy;
 
 /*
  * When we disable a timer, we need to wait at least for 2 cycles of
@@ -74,7 +75,8 @@ static void sun4i_clkevt_time_start(u8 timer, bool periodic)
 	else
 		val |= TIMER_CTL_ONESHOT;
 
-	writel(val | TIMER_CTL_ENABLE, timer_base + TIMER_CTL_REG(timer));
+	writel(val | TIMER_CTL_ENABLE | TIMER_CTL_RELOAD,
+	       timer_base + TIMER_CTL_REG(timer));
 }
 
 static void sun4i_clkevt_mode(enum clock_event_mode mode,
@@ -83,6 +85,7 @@ static void sun4i_clkevt_mode(enum clock_event_mode mode,
 	switch (mode) {
 	case CLOCK_EVT_MODE_PERIODIC:
 		sun4i_clkevt_time_stop(0);
+		sun4i_clkevt_time_setup(0, ticks_per_jiffy);
 		sun4i_clkevt_time_start(0, true);
 		break;
 	case CLOCK_EVT_MODE_ONESHOT:
@@ -169,9 +172,9 @@ static void __init sun4i_timer_init(struct device_node *node)
 	clocksource_mmio_init(timer_base + TIMER_CNTVAL_REG(1), node->name,
 			      rate, 300, 32, clocksource_mmio_readl_down);
 
-	writel(rate / HZ, timer_base + TIMER_INTVAL_REG(0));
+	ticks_per_jiffy = DIV_ROUND_UP(rate, HZ);
 
-	writel(TIMER_CTL_CLK_SRC(TIMER_CTL_CLK_SRC_OSC24M) | TIMER_CTL_RELOAD,
+	writel(TIMER_CTL_CLK_SRC(TIMER_CTL_CLK_SRC_OSC24M),
 	       timer_base + TIMER_CTL_REG(0));
 
 	ret = setup_irq(irq, &sun4i_timer_irq);
Signature · 1 line
1.8.3.2
#11 lore raw
In reply to Maxime Ripard

On 07/11/2013 06:31 PM, Maxime Ripard wrote:

Applied to my tree for 3.12.

Thanks
-- Daniel

Quoted 59 lines

Hi everyone,

The first timer code we merged when adding support for the A13 some time back was mostly a clean up from the source drop we had, without any documentation. This happened to work, but the code merged in turned out to be far from perfect, and had several flaws.

This patchset hopefully fixes these flaws, and cleanup most of the driver as well, to end up in an almost complete rewrite of it (even though it's not that long).

It also finally adds a clocksource driver using the second timer as our monotonic clock source.

These flaws have all been spotted when trying to add the A31 support, work that is still ongoing, but will hopefully benefit from this patchset as well.

Thanks,
Maxime

Changes from v4:
- Removed the last clk_get_rate() user
- Got a few beers

Changes from v3:
- Reintroduce the rate variable to cache the parent clock rate
- Remove the interval programming at probe time that was reintroduced in the v3 due to a poor rebase.

Changes from v2:
- Use the clocksource timer to get the amount of time we have to wait for when disabling and enabling back a timer
- Added patch to add parenthesis around the macros arguments
- Renamed the AUTORELOAD register define to the more meaningful RELOAD name

Changes from v1:
- Rebased on top of linux-next to benefit from the move to all architectures of the sched_clock functions
- Moved the clock source to the second timer instead of the 64 bits free-running counter like suggested by Thomas.

Maxime Ripard (10):
clocksource: sun4i: Use the BIT macros where possible
clocksource: sun4i: Wrap macros arguments in parenthesis
clocksource: sun4i: rename AUTORELOAD define to RELOAD
clocksource: sun4i: Add clocksource and sched clock drivers
clocksource: sun4i: Don't forget to enable the clock we use
clocksource: sun4i: Fix the next event code
clocksource: sun4i: Factor out some timer code
clocksource: sun4i: Remove TIMER_SCAL variable
clocksource: sun4i: Cleanup parent clock setup
clocksource: sun4i: Fix bug when switching from periodic to oneshot
modes

 drivers/clocksource/sun4i_timer.c | 110 +++++++++++++++++++++++++++-----------
 1 file changed, 78 insertions(+), 32 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