clocksource: sunxi: Timer fixes and cleanup

The A10 and the A13 has a 64 bits free running counter that we can use as a clocksource and a sched clock, that were both not used yet on these platforms.

Signed-off-by Maxime Ripard <[email protected]>

---
 drivers/clocksource/sun4i_timer.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

drivers/clocksource/sun4i_timer.c +7 -0

index bdf34d9..1d2eaa0 100644
@@ -23,6 +23,8 @@
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 
+#include <asm/sched_clock.h>
+
 #define TIMER_IRQ_EN_REG	0x00
 #define TIMER_IRQ_EN(val)		BIT(val)
 #define TIMER_IRQ_ST_REG	0x04

@@ -34,6 +36,11 @@
 #define TIMER_CNTVAL_REG(val)	(0x10 * val + 0x18)
 
 #define TIMER_SCAL		16
+#define TIMER_CNT64_CTL_REG	0xa0
+#define TIMER_CNT64_CTL_CLR		BIT(0)
+#define TIMER_CNT64_CTL_RL		BIT(1)
+#define TIMER_CNT64_LOW_REG	0xa4
+#define TIMER_CNT64_HIGH_REG	0xa8
 
 static void __iomem *timer_base;
 

drivers/clocksource/sun4i_timer.c +9 -0

+
+	return readl(timer_base + TIMER_CNT64_LOW_REG);
+}
+
+static cycle_t sun4i_timer_clksrc_read(struct clocksource *c)
+{
+	return sun4i_timer_sched_read();
+}
+
 static void __init sun4i_timer_init(struct device_node *node)
 {
 	unsigned long rate = 0;

drivers/clocksource/sun4i_timer.c +0 -0

 	       timer_base + TIMER_INTVAL_REG(0));

Quoted 38 lines · Maxime Ripard

The A10 and the A13 has a 64 bits free running counter that we can use as a clocksource and a sched clock, that were both not used yet on these platforms.

Signed-off-by Maxime Ripard <[email protected]>

---
 drivers/clocksource/sun4i_timer.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

drivers/clocksource/sun4i_timer.c +7 -0

index bdf34d9..1d2eaa0 100644
@@ -23,6 +23,8 @@
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 
+#include <asm/sched_clock.h>
+
 #define TIMER_IRQ_EN_REG	0x00
 #define TIMER_IRQ_EN(val)		BIT(val)
 #define TIMER_IRQ_ST_REG	0x04

@@ -34,6 +36,11 @@
 #define TIMER_CNTVAL_REG(val)	(0x10 * val + 0x18)
 
 #define TIMER_SCAL		16
+#define TIMER_CNT64_CTL_REG	0xa0
+#define TIMER_CNT64_CTL_CLR		BIT(0)
+#define TIMER_CNT64_CTL_RL		BIT(1)
+#define TIMER_CNT64_LOW_REG	0xa4
+#define TIMER_CNT64_HIGH_REG	0xa8
 
 static void __iomem *timer_base;
 

@@ -96,6 +103,20 @@ static struct irqaction sun4i_timer_irq = {
 	.dev_id = &sun4i_clockevent,
 };
 

drivers/clocksource/sun4i_timer.c static struct irqaction sun4i_timer_irq = { · line 110

Quoted 5 lines

drivers/clocksource/sun4i_timer.c +5 -0

+static u32 sun4i_timer_sched_read(void)
+{
+	u32 reg = readl(timer_base + TIMER_CNT64_CTL_REG);
+	writel(reg | TIMER_CNT64_CTL_RL, timer_base + TIMER_CNT64_CTL_REG);
+	while (readl(timer_base + TIMER_CNT64_CTL_REG) & TIMER_CNT64_CTL_REG);

Isn't a cpu_relax missing here ?

drivers/clocksource/sun4i_timer.c static void __init sun4i_timer_init(struct device_node *node) · line 147

Quoted 8 lines

drivers/clocksource/sun4i_timer.c +6 -0

+	writel(TIMER_CNT64_CTL_CLR, timer_base + TIMER_CNT64_CTL_REG);
+	setup_sched_clock(sun4i_timer_sched_read, 32, clk_get_rate(clk));
+	clocksource_mmio_init(timer_base + TIMER_CNT64_LOW_REG, node->name,
+			      clk_get_rate(clk), 300, 32,
+			      sun4i_timer_clksrc_read);
+
 	writel(rate / (TIMER_SCAL * HZ),

DIV_ROUND_CLOSEST(rate, (TIMER_SCAL * HZ)), ?

The A10 and the A13 has a 64 bits free running counter that we can use as a clocksource and a sched clock, that were both not used yet on these platforms.

Signed-off-by Maxime Ripard <[email protected]>

---
 drivers/clocksource/sun4i_timer.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

drivers/clocksource/sun4i_timer.c +0 -0

index bdf34d9..1d2eaa0 100644

drivers/clocksource/sun4i_timer.c +6 -0

+
 #define TIMER_IRQ_EN_REG	0x00
 #define TIMER_IRQ_EN(val)		BIT(val)
 #define TIMER_IRQ_ST_REG	0x04

@@ -34,6 +36,11 @@
 #define TIMER_CNTVAL_REG(val)	(0x10 * val + 0x18)
 
 #define TIMER_SCAL		16
+#define TIMER_CNT64_CTL_REG	0xa0
+#define TIMER_CNT64_CTL_CLR		BIT(0)
+#define TIMER_CNT64_CTL_RL		BIT(1)
+#define TIMER_CNT64_LOW_REG	0xa4
+#define TIMER_CNT64_HIGH_REG	0xa8
 
 static void __iomem *timer_base;
 

drivers/clocksource/sun4i_timer.c +19 -0

+{
+	u32 reg = readl(timer_base + TIMER_CNT64_CTL_REG);
+	writel(reg | TIMER_CNT64_CTL_RL, timer_base + TIMER_CNT64_CTL_REG);
+	while (readl(timer_base + TIMER_CNT64_CTL_REG) & TIMER_CNT64_CTL_REG);
+
+	return readl(timer_base + TIMER_CNT64_LOW_REG);
+}
+
+static cycle_t sun4i_timer_clksrc_read(struct clocksource *c)
+{
+	return sun4i_timer_sched_read();
+}
+
 static void __init sun4i_timer_init(struct device_node *node)
 {
 	unsigned long rate = 0;

@@ -117,6 +138,12 @@ static void __init sun4i_timer_init(struct device_node *node)
 
 	rate = clk_get_rate(clk);
 
+	writel(TIMER_CNT64_CTL_CLR, timer_base + TIMER_CNT64_CTL_REG);
+	setup_sched_clock(sun4i_timer_sched_read, 32, clk_get_rate(clk));
+	clocksource_mmio_init(timer_base + TIMER_CNT64_LOW_REG, node->name,
+			      clk_get_rate(clk), 300, 32,
+			      sun4i_timer_clksrc_read);
+
 	writel(rate / (TIMER_SCAL * HZ),
 	       timer_base + TIMER_INTVAL_REG(0));
 

Hi Maxime,

Quoted 17 lines · Maxime Ripard

The A10 and the A13 has a 64 bits free running counter that we can use as a clocksource and a sched clock, that were both not used yet on these platforms.

Signed-off-by Maxime Ripard <[email protected]>

---
 drivers/clocksource/sun4i_timer.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

drivers/clocksource/sun4i_timer.c +0 -0

index bdf34d9..1d2eaa0 100644
@@ -23,6 +23,8 @@
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 

drivers/clocksource/sun4i_timer.c line 26

Quoted 1 line

drivers/clocksource/sun4i_timer.c +1 -0

+#include <asm/sched_clock.h>

In the tip.git tree (and -next) this header is moved to <linux/sched_clock.h> in 38ff87f77a (sched_clock: Make ARM's sched_clock generic for all architectures).

Quoted 16 lines

drivers/clocksource/sun4i_timer.c +6 -0

+
 #define TIMER_IRQ_EN_REG	0x00
 #define TIMER_IRQ_EN(val)		BIT(val)
 #define TIMER_IRQ_ST_REG	0x04

@@ -34,6 +36,11 @@
 #define TIMER_CNTVAL_REG(val)	(0x10 * val + 0x18)
 
 #define TIMER_SCAL		16
+#define TIMER_CNT64_CTL_REG	0xa0
+#define TIMER_CNT64_CTL_CLR		BIT(0)
+#define TIMER_CNT64_CTL_RL		BIT(1)
+#define TIMER_CNT64_LOW_REG	0xa4
+#define TIMER_CNT64_HIGH_REG	0xa8
 
 static void __iomem *timer_base;
 

drivers/clocksource/sun4i_timer.c static struct irqaction sun4i_timer_irq = { · line 106

Quoted 5 lines

drivers/clocksource/sun4i_timer.c +1 -0

@@ -96,6 +103,20 @@ static struct irqaction sun4i_timer_irq = {
 	.dev_id = &sun4i_clockevent,
 };
 
+static u32 sun4i_timer_sched_read(void)

You commit message mentions "64 bits free running counter", but this one only returns 32 bit.

Quoted 3 lines · Baruch Siach

Quoted 1 line · Maxime Ripard

+static u32 sun4i_timer_sched_read(void)

Quoted 2 lines

You commit message mentions "64 bits free running counter", but this one only returns 32 bit.

Yeah, the callback setup by setup_sched_clock is supposed to be returning a u32, and clocksource_mmio_init only accepts up to 32 bits as well, so I'm only using the lower 32bits of this 64 bits counter.

I'll amend the commit log to state this.

Quoted 7 lines · Maxime Ripard

Quoted 5 lines · Baruch Siach

Quoted 1 line · Maxime Ripard

+static u32 sun4i_timer_sched_read(void)

You commit message mentions "64 bits free running counter", but this one only returns 32 bit.

Quoted 5 lines

Yeah, the callback setup by setup_sched_clock is supposed to be returning a u32, and clocksource_mmio_init only accepts up to 32 bits as well, so I'm only using the lower 32bits of this 64 bits counter.

I'll amend the commit log to state this.

But using 64 bit counter for sched_clock is much easier that using 32 bit one.

You can either wait for the rest of Stephen's patch set
(http://thread.gmane.org/gmane.linux.ports.arm.msm/4092) before adding 64 bit, or you can just make the trivial change to the now generic sched_clock code.

Quoted 12 lines · Baruch Siach

Quoted 11 lines · Maxime Ripard

Quoted 5 lines · Baruch Siach

Quoted 1 line · Maxime Ripard

+static u32 sun4i_timer_sched_read(void)

You commit message mentions "64 bits free running counter", but this one only returns 32 bit.

Yeah, the callback setup by setup_sched_clock is supposed to be returning a u32, and clocksource_mmio_init only accepts up to 32 bits as well, so I'm only using the lower 32bits of this 64 bits counter.

Quoted 3 lines

Quoted 1 line

I'll amend the commit log to state this.

But using 64 bit counter for sched_clock is much easier that using 32 bit one.

Easier in what aspect? Both API looks similar.

Quoted 4 lines

You can either wait for the rest of Stephen's patch set
(http://thread.gmane.org/gmane.linux.ports.arm.msm/4092) before adding 64 bit, or you can just make the trivial change to the now generic sched_clock code.

I'll wait for the Stephen's patches to be merged then, and add support for 64bits counters to clocksource_mmio_init.

Hi Maxime,

Quoted 15 lines · Maxime Ripard

Quoted 14 lines · Baruch Siach

Quoted 12 lines · Maxime Ripard

Quoted 5 lines · Baruch Siach

Quoted 1 line · Maxime Ripard

+static u32 sun4i_timer_sched_read(void)

You commit message mentions "64 bits free running counter", but this one only returns 32 bit.

Yeah, the callback setup by setup_sched_clock is supposed to be returning a u32, and clocksource_mmio_init only accepts up to 32 bits as well, so I'm only using the lower 32bits of this 64 bits counter.

I'll amend the commit log to state this.

Quoted 3 lines

Quoted 1 line

But using 64 bit counter for sched_clock is much easier that using 32 bit one.

Easier in what aspect? Both API looks similar.

You can just implement your own simple sched_clock() that just returns the current value of this 64 bit counter, and do away with all the tricky code in kernel/time/sched_clock.c (in tip.git) that is needed to make the 32 -> 64 extension safe. This is not compatible with multi-platform kernel, though.

The A10 and the A13 has a 64 bits free running counter that we can use as a clocksource and a sched clock, that were both not used yet on these platforms.

Signed-off-by Maxime Ripard <[email protected]>

---
 drivers/clocksource/sun4i_timer.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

drivers/clocksource/sun4i_timer.c +7 -0

index bdf34d9..1d2eaa0 100644
@@ -23,6 +23,8 @@
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 
+#include <asm/sched_clock.h>
+
 #define TIMER_IRQ_EN_REG	0x00
 #define TIMER_IRQ_EN(val)		BIT(val)
 #define TIMER_IRQ_ST_REG	0x04

@@ -34,6 +36,11 @@
 #define TIMER_CNTVAL_REG(val)	(0x10 * val + 0x18)
 
 #define TIMER_SCAL		16
+#define TIMER_CNT64_CTL_REG	0xa0
+#define TIMER_CNT64_CTL_CLR		BIT(0)
+#define TIMER_CNT64_CTL_RL		BIT(1)
+#define TIMER_CNT64_LOW_REG	0xa4
+#define TIMER_CNT64_HIGH_REG	0xa8
 
 static void __iomem *timer_base;
 

drivers/clocksource/sun4i_timer.c +15 -0

+
+	return readl(timer_base + TIMER_CNT64_LOW_REG);
+}
+
+static cycle_t sun4i_timer_clksrc_read(struct clocksource *c)
+{
+	return sun4i_timer_sched_read();
+}
+
 static void __init sun4i_timer_init(struct device_node *node)
 {
 	unsigned long rate = 0;

@@ -117,6 +138,12 @@ static void __init sun4i_timer_init(struct device_node *node)
 
 	rate = clk_get_rate(clk);
 
+	writel(TIMER_CNT64_CTL_CLR, timer_base + TIMER_CNT64_CTL_REG);
+	setup_sched_clock(sun4i_timer_sched_read, 32, clk_get_rate(clk));
+	clocksource_mmio_init(timer_base + TIMER_CNT64_LOW_REG, node->name,
+			      clk_get_rate(clk), 300, 32,
+			      sun4i_timer_clksrc_read);
+
 	writel(rate / (TIMER_SCAL * HZ),
 	       timer_base + TIMER_INTVAL_REG(0));
 

Quoted 38 lines · On Wed, 26 Jun 2013 23:16:55 +0200 Maxime Ripard…

The A10 and the A13 has a 64 bits free running counter that we can use as a clocksource and a sched clock, that were both not used yet on these platforms.

Signed-off-by Maxime Ripard <[email protected]>

---
 drivers/clocksource/sun4i_timer.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

drivers/clocksource/sun4i_timer.c +7 -0

index bdf34d9..1d2eaa0 100644
@@ -23,6 +23,8 @@
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 
+#include <asm/sched_clock.h>
+
 #define TIMER_IRQ_EN_REG	0x00
 #define TIMER_IRQ_EN(val)		BIT(val)
 #define TIMER_IRQ_ST_REG	0x04

@@ -34,6 +36,11 @@
 #define TIMER_CNTVAL_REG(val)	(0x10 * val + 0x18)
 
 #define TIMER_SCAL		16
+#define TIMER_CNT64_CTL_REG	0xa0
+#define TIMER_CNT64_CTL_CLR		BIT(0)
+#define TIMER_CNT64_CTL_RL		BIT(1)
+#define TIMER_CNT64_LOW_REG	0xa4
+#define TIMER_CNT64_HIGH_REG	0xa8
 
 static void __iomem *timer_base;
 

@@ -96,6 +103,20 @@ static struct irqaction sun4i_timer_irq = {
 	.dev_id = &sun4i_clockevent,
 };
 

drivers/clocksource/sun4i_timer.c static struct irqaction sun4i_timer_irq = { · line 108

Quoted 3 lines

drivers/clocksource/sun4i_timer.c +3 -0

+static u32 sun4i_timer_sched_read(void)
+{
+	u32 reg = readl(timer_base + TIMER_CNT64_CTL_REG);

If we can be absolutely sure that nothing else may ever change the TIMER_CNT64_CTL_REG, then its default value can be probably cached instead of doing expensive read from the hardware register each time?

The gettimeofday() abusers will feel a bit less pain. ARM does not currently enjoy the VDSO optimized gettimeofday, so the software which has been only tested on x86 may get a nasty surprise (an order of magnitude higher gettimeofday overhead).

drivers/clocksource/sun4i_timer.c static struct irqaction sun4i_timer_irq = { · line 110

Quoted 2 lines

drivers/clocksource/sun4i_timer.c +2 -0

+	writel(reg | TIMER_CNT64_CTL_RL, timer_base + TIMER_CNT64_CTL_REG);
+	while (readl(timer_base + TIMER_CNT64_CTL_REG) & TIMER_CNT64_CTL_REG);

Some may think that this particular loop looks like a performance bottleneck, but it is very rarely run for more than one iteration. In fact, most of the time it just happens to be a single HW register read.

Hi Siarhei,

Quoted 41 lines · Siarhei Siamashka

Quoted 38 lines · On Wed, 26 Jun 2013 23:16:55 +0200 Maxime Ripard…

The A10 and the A13 has a 64 bits free running counter that we can use as a clocksource and a sched clock, that were both not used yet on these platforms.

Signed-off-by Maxime Ripard <[email protected]>

---
 drivers/clocksource/sun4i_timer.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

drivers/clocksource/sun4i_timer.c +7 -0

index bdf34d9..1d2eaa0 100644
@@ -23,6 +23,8 @@
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 
+#include <asm/sched_clock.h>
+
 #define TIMER_IRQ_EN_REG	0x00
 #define TIMER_IRQ_EN(val)		BIT(val)
 #define TIMER_IRQ_ST_REG	0x04

@@ -34,6 +36,11 @@
 #define TIMER_CNTVAL_REG(val)	(0x10 * val + 0x18)
 
 #define TIMER_SCAL		16
+#define TIMER_CNT64_CTL_REG	0xa0
+#define TIMER_CNT64_CTL_CLR		BIT(0)
+#define TIMER_CNT64_CTL_RL		BIT(1)
+#define TIMER_CNT64_LOW_REG	0xa4
+#define TIMER_CNT64_HIGH_REG	0xa8
 
 static void __iomem *timer_base;
 

@@ -96,6 +103,20 @@ static struct irqaction sun4i_timer_irq = {
 	.dev_id = &sun4i_clockevent,
 };
 

Quoted 8 lines

drivers/clocksource/sun4i_timer.c static struct irqaction sun4i_timer_irq = { · line 108

Quoted 3 lines

drivers/clocksource/sun4i_timer.c +3 -0

+static u32 sun4i_timer_sched_read(void)
+{
+	u32 reg = readl(timer_base + TIMER_CNT64_CTL_REG);

If we can be absolutely sure that nothing else may ever change the TIMER_CNT64_CTL_REG, then its default value can be probably cached instead of doing expensive read from the hardware register each time?

Since it's a free-running counter, its value will always change, so the caching will bring no additions at all, right?

Quoted 7 lines

Quoted 4 lines

If we can be absolutely sure that nothing else may ever change the TIMER_CNT64_CTL_REG, then its default value can be probably cached instead of doing expensive read from the hardware register each time?

Since it's a free-running counter, its value will always change, so the caching will bring no additions at all, right?

Sorry, 'caching' was not a very good description for something that is already a compile time constant. I mean just replace

    u32 reg = readl(timer_base + TIMER_CNT64_CTL_REG);

with

    u32 reg = TIMER_CNT64_CTL_CLR;

Because we know that the TIMER_CNT64_CTL_REG is already supposed to have the default TIMER_CNT64_CTL_CLR value (initialized in the 'sun4i_timer_init' function) between calls to 'sun4i_timer_sched_read'. Inside of 'sun4i_timer_sched_read' we set an extra TIMER_CNT64_CTL_RL bit in this register, but wait until it clears, effectively reverting TIMER_CNT64_CTL_REG register back to the default TIMER_CNT64_CTL_CLR value.

Removing this extra HW register read can save roughly a hundred of CPU cycles here and provide a ~10% overall improvement for gettimeofday (these estimates are based on the earlier benchmarks done with the Allwinner 3.4 kernel).

Or maybe I'm overlooking something?

Hi everyone,

It also finally adds a clocksource from the free running counter found in the A10/A13 SoCs.

Hi,

Quoted 2 lines · Maxime Ripard

Hi everyone,

<snip>

Quoted 2 lines

It also finally adds a clocksource from the free running counter found in the A10/A13 SoCs.

Hmm, have you benchmarked this? There have been reports from linux-sunxi kernel users (xbmc project) that the waiting for the latch is quite slow. Note we don't have anything better yet in the linux-sunxi kernel.

Quoted 11 lines · Hans de Goede

Hi,

Quoted 2 lines · Maxime Ripard

Hi everyone,

<snip>

Quoted 2 lines

It also finally adds a clocksource from the free running counter found in the A10/A13 SoCs.

Quoted 3 lines

Hmm, have you benchmarked this? There have been reports from linux-sunxi kernel users (xbmc project) that the waiting for the latch is quite slow. Note we don't have anything better yet in the linux-sunxi kernel.

No. I didn't.

Do you have any pointers to these discussions?

Hi,

Quoted 18 lines · Maxime Ripard

Quoted 6 lines · Hans de Goede

Hi,

Quoted 2 lines · Maxime Ripard

Hi everyone,

Quoted 8 lines

<snip>

Quoted 2 lines

It also finally adds a clocksource from the free running counter found in the A10/A13 SoCs.

Hmm, have you benchmarked this? There have been reports from linux-sunxi kernel users (xbmc project) that the waiting for the latch is quite slow. Note we don't have anything better yet in the linux-sunxi kernel.

No. I didn't.

Quoted 2 lines

Do you have any pointers to these discussions?

The original discussion should be somewhere here:
https://groups.google.com/forum/#!forum/linux-sunxi

But I could not find it (it is probably hidden under an unlogical subject).

Looking at my own notes (a small TODO file), I've
written down that the reporter reports:

-current clocksource can cause us to run with interrupts disabled for 17%
of the time, see "perf top" output

This is with a workload which does a lot of gettimeofday calls.

I notice that unlike the sunxi-3.4 code you don't do any locking, so how do you stop 2 clocksource calls from racing (and thus getting a possible wrong value because of things not being properly latched) ?

Quoted 21 lines · Hans de Goede

Hi,

Quoted 18 lines · Maxime Ripard

Quoted 6 lines · Hans de Goede

Hi,

Quoted 2 lines · Maxime Ripard

Hi everyone,

Quoted 8 lines

<snip>

Quoted 2 lines

It also finally adds a clocksource from the free running counter found in the A10/A13 SoCs.

Hmm, have you benchmarked this? There have been reports from linux-sunxi kernel users (xbmc project) that the waiting for the latch is quite slow. Note we don't have anything better yet in the linux-sunxi kernel.

No. I didn't.

Quoted 8 lines

Quoted 2 lines

Do you have any pointers to these discussions?

The original discussion should be somewhere here:
https://groups.google.com/forum/#!forum/linux-sunxi

But I could not find it (it is probably hidden under an unlogical subject).

I searched a bit and it seems to be that discussion:
https://groups.google.com/d/msg/linux-sunxi/gaTDngPT7Is/oeLtWb1N1wIJ

Quoted 8 lines

Looking at my own notes (a small TODO file), I've
written down that the reporter reports:

-current clocksource can cause us to run with interrupts disabled for 17%
of the time, see "perf top" output

This is with a workload which does a lot of gettimeofday calls.

Siarhei however notes that even higher-end SoCs like the exynos5 have similar performances with that regard. So I'm not sure we can do something about it, except what is suggested in the above mail, which looks rather unsafe.

Anyway, like you said, we have no easy other solution, and we lacked such support until now.

So why not merge this code for now, and try to optimise it later if we find it's needed.

Quoted 8 lines

Siarhei however notes that even higher-end SoCs like the exynos5 have similar performances with that regard. So I'm not sure we can do something about it, except what is suggested in the above mail, which looks rather unsafe.

Anyway, like you said, we have no easy other solution, and we lacked such support until now.

Quoted 2 lines

So why not merge this code for now, and try to optimise it later if we find it's needed.

That is fine with me, I just wanted to share that this has shown as a bottleneck in some benchmarks in case anyone has a clever idea to fix it ...

Quoted 4 lines · Hans de Goede

I notice that unlike the sunxi-3.4 code you don't do any locking, so how do you stop 2 clocksource calls from racing (and thus getting a possible wrong value because of things not being properly latched) ?

Hmm, right. I'll add a spinlock.

Quoted 7 lines · On Thu, 27 Jun 2013 18:54:36 +0200 Maxime Ripard…

Quoted 4 lines · Hans de Goede

I notice that unlike the sunxi-3.4 code you don't do any locking, so how do you stop 2 clocksource calls from racing (and thus getting a possible wrong value because of things not being properly latched) ?

Hmm, right. I'll add a spinlock.

I think the best would be to ask the Allwinner people (it's good to have them in CC, right?) whether anything wrong can happen because of "things not being properly latched".

The A10 manual from http://free-electrons.com/~maxime/pub/datasheet/
does not seem to contain any details about what bad things may happen if we try to read CNT64_LO_REG while latching is still in progress and CNT64_RL_EN bit in CNT64_CTRL_REG has not changed to zero yet. I can imagine the following possible scenarios:
1. We read either the old stale CNT64_LO_REG value or the new correct value.
2. We read either the old stale CNT64_LO_REG value or the new correct value, or some random garbage.
3. The processor may deadlock, eat your dog, or do some other nasty thing.

In the case of 1, we probably can get away without using any spinlocks?

Quoted 12 lines

The A10 manual from http://free-electrons.com/~maxime/pub/datasheet/
does not seem to contain any details about what bad things may happen if we try to read CNT64_LO_REG while latching is still in progress and CNT64_RL_EN bit in CNT64_CTRL_REG has not changed to zero yet. I can imagine the following possible scenarios:
1. We read either the old stale CNT64_LO_REG value or the new correct value.
2. We read either the old stale CNT64_LO_REG value or the new correct value, or some random garbage.
3. The processor may deadlock, eat your dog, or do some other nasty thing.

Quoted 1 line

In the case of 1, we probably can get away without using any spinlocks?

About the 64bits counter, the latch bit is needed because of the asynchronous circuit. The internal circuit of 64bits counter is working under 24Mhz clock, and CNT_LO/HI is read with APB clock. So the clock synchronize is needed. The function of the latch is synchronous the 64bits counter from 24Mhz clock domain to APB clock domain. So, if the latch is not completely, value of the CNT_LO/HI maybe a random value, because some bits are latched, but others are not. So, the CNT_LO/HI should be read after latch is completely. The latch just takes 3 cycles of 24Mhz clock, the time is nearly 0.125 micro-second.

Quoted 14 lines · On Fri, 28 Jun 2013 09:43:37 +0800 ?? <[email protected]> wrote:

Quoted 12 lines

The A10 manual from http://free-electrons.com/~maxime/pub/datasheet/
does not seem to contain any details about what bad things may happen if we try to read CNT64_LO_REG while latching is still in progress and CNT64_RL_EN bit in CNT64_CTRL_REG has not changed to zero yet. I can imagine the following possible scenarios:
1. We read either the old stale CNT64_LO_REG value or the new correct value.
2. We read either the old stale CNT64_LO_REG value or the new correct value, or some random garbage.
3. The processor may deadlock, eat your dog, or do some other nasty thing.

Quoted 1 line

In the case of 1, we probably can get away without using any spinlocks?

Quoted 10 lines

About the 64bits counter, the latch bit is needed because of the asynchronous circuit. The internal circuit of 64bits counter is working under 24Mhz clock, and CNT_LO/HI is read with APB clock. So the clock synchronize is needed. The function of the latch is synchronous the 64bits counter from 24Mhz clock domain to APB clock domain. So, if the latch is not completely, value of the CNT_LO/HI maybe a random value, because some bits are latched, but others are not. So, the CNT_LO/HI should be read after latch is completely. The latch just takes 3 cycles of 24Mhz clock, the time is nearly 0.125 micro-second.

Thanks for the clarification! It is very much appreciated.

So basically we get scenario 2, which still allows some optimizations compared to scenario 3. On single-core systems (Allwinner A10), it probably makes sense to avoid spinlocks overhead and just place

Quoted 30 lines · Siarhei Siamashka

Quoted 14 lines · On Fri, 28 Jun 2013 09:43:37 +0800 ?? <[email protected]> wrote:

Quoted 12 lines

The A10 manual from http://free-electrons.com/~maxime/pub/datasheet/
does not seem to contain any details about what bad things may happen if we try to read CNT64_LO_REG while latching is still in progress and CNT64_RL_EN bit in CNT64_CTRL_REG has not changed to zero yet. I can imagine the following possible scenarios:
1. We read either the old stale CNT64_LO_REG value or the new correct value.
2. We read either the old stale CNT64_LO_REG value or the new correct value, or some random garbage.
3. The processor may deadlock, eat your dog, or do some other nasty thing.

Quoted 1 line

In the case of 1, we probably can get away without using any spinlocks?

Quoted 10 lines

About the 64bits counter, the latch bit is needed because of the asynchronous circuit. The internal circuit of 64bits counter is working under 24Mhz clock, and CNT_LO/HI is read with APB clock. So the clock synchronize is needed. The function of the latch is synchronous the 64bits counter from 24Mhz clock domain to APB clock domain. So, if the latch is not completely, value of the CNT_LO/HI maybe a random value, because some bits are latched, but others are not. So, the CNT_LO/HI should be read after latch is completely. The latch just takes 3 cycles of 24Mhz clock, the time is nearly 0.125 micro-second.

Thanks for the clarification! It is very much appreciated.

Quoted 3 lines

So basically we get scenario 2, which still allows some optimizations compared to scenario 3. On single-core systems (Allwinner A10), it probably makes sense to avoid spinlocks overhead and just place

Spinlocks are NOPs on UP and just disable preemption, but they provide you the same ordering guarantees as spinlocks on SMP. So no special case optimization necessary.

Quoted 12 lines

The A10 manual from http://free-electrons.com/~maxime/pub/datasheet/
does not seem to contain any details about what bad things may happen if we try to read CNT64_LO_REG while latching is still in progress and CNT64_RL_EN bit in CNT64_CTRL_REG has not changed to zero yet. I can imagine the following possible scenarios:
1. We read either the old stale CNT64_LO_REG value or the new correct value.
2. We read either the old stale CNT64_LO_REG value or the new correct value, or some random garbage.
3. The processor may deadlock, eat your dog, or do some other nasty thing.

Quoted 1 line

In the case of 1, we probably can get away without using any spinlocks?

About the 64bits counter, the latch bit is needed because of the asynchronous circuit. The internal circuit of 64bits counter is working under 24Mhz clock, and CNT_LO/HI is read with APB clock. So the clock synchronize is needed. The function of the latch is synchronous the 64bits counter from 24Mhz clock domain to APB clock domain. So, if the latch is not completely, value of the CNT_LO/HI maybe a random value, because some bits are latched, but others are not. So, the CNT_LO/HI should be read after latch is completely. The latch just takes 3 cycles of 24Mhz clock, the time is nearly 0.125 micro-second.

NOTICE: This e-mail and any included attachments are intended only for the sole use of named and intended recipient (s) only. If you are the named and intended recipient, please note that the information contained in this email and its embedded files are confidential and privileged. If you are neither the intended nor named recipient, you are hereby notified that any unauthorized review, use, disclosure, dissemination, distribution, or copying of this communication, or any of its contents, is strictly prohibited. Please reply to the sender and destroy the original message and all your records of this message (whether electronic or otherwise). Furthermore, you should not disclose to any other person, use, copy or disseminate the contents of this e-mail and/or the documents accompanying it.

Quoted 5 lines

NOTICE: This e-mail and any included attachments are intended only for the sole use of named and intended recipient (s) only. If you are the named and intended recipient, please note that the information contained in this email and its embedded files are confidential and privileged. If you are neither the intended nor

Quoted 8 lines

named recipient, you are hereby notified that any unauthorized review, use, disclosure, dissemination, distribution, or copying of this communication, or any of its contents, is strictly prohibited. Please reply to the sender and destroy the original message and all your records of this message (whether electronic or otherwise). Furthermore, you should not disclose to any other person, use, copy or disseminate the contents of this e-mail and/or the documents accompanying it.

Ugh, just spotted this at the bottom.

I believe that by sending this information to LKML and the other public mailing lists, the intended recipient is "the whole world", hence most of the stated clauses (for example related to "any other person") can be safely optimized out. Still this does not sound very nice. I hope that the original intention was to authorize sharing and using this information. Please correct me if I'm wrong.

Quoted 12 lines

The A10 manual from http://free-electrons.com/~maxime/pub/datasheet/
does not seem to contain any details about what bad things may happen if we try to read CNT64_LO_REG while latching is still in progress and CNT64_RL_EN bit in CNT64_CTRL_REG has not changed to zero yet. I can imagine the following possible scenarios:
1. We read either the old stale CNT64_LO_REG value or the new correct value.
2. We read either the old stale CNT64_LO_REG value or the new correct value, or some random garbage.
3. The processor may deadlock, eat your dog, or do some other nasty thing.

Quoted 1 line

In the case of 1, we probably can get away without using any spinlocks?

About the 64bits counter, the latch bit is needed because of the asynchronous circuit. The internal circuit of 64bits counter is working under 24Mhz clock, and CNT_LO/HI is read with APB clock. So the clock synchronize is needed. The function of the latch is synchronous the 64bits counter from 24Mhz clock domain to APB clock domain. So, if the latch is not completely, value of the CNT_LO/HI maybe a random value, because some bits are latched, but others are not. So, the CNT_LO/HI should be read after latch is completely.
The latch just takes 3 cycles of 24Mhz clock, the time is nearly 0.125 micro-second.

Quoted 14 lines · ??

Quoted 12 lines

The A10 manual from http://free-electrons.com/~maxime/pub/datasheet/
does not seem to contain any details about what bad things may happen if we try to read CNT64_LO_REG while latching is still in progress and CNT64_RL_EN bit in CNT64_CTRL_REG has not changed to zero yet. I can imagine the following possible scenarios:
1. We read either the old stale CNT64_LO_REG value or the new correct value.
2. We read either the old stale CNT64_LO_REG value or the new correct value, or some random garbage.
3. The processor may deadlock, eat your dog, or do some other nasty thing.

Quoted 1 line

In the case of 1, we probably can get away without using any spinlocks?

Quoted 9 lines

About the 64bits counter, the latch bit is needed because of the asynchronous circuit. The internal circuit of 64bits counter is working under 24Mhz clock, and CNT_LO/HI is read with APB clock. So the clock synchronize is needed. The function of the latch is synchronous the 64bits counter from 24Mhz clock domain to APB clock domain. So, if the latch is not completely, value of the CNT_LO/HI maybe a random value, because some bits are latched, but others are not. So, the CNT_LO/HI should be read after latch is completely.
The latch just takes 3 cycles of 24Mhz clock, the time is nearly 0.125 micro-second.

I really wonder why we're trying to use that timer. AFAICT the A10 has another six 32bit timers which do not have this restriction and the clocksoure/sched_clock implementation works nicely with 32 bits. So what's the point of using that 64 bit counter if it's horrible to access?