[PATCH 0/8] clocksource: sunxi: Timer fixes and cleanup

[PATCH 0/8] clocksource: sunxi: Timer fixes and cleanup

[Maxime Ripard]

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 from the free running counter found in the
A10/A13 SoCs.

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
 
Maxime Ripard (8):
  clocksource: sun4i: Use the BIT macros where possible
  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 | 107 ++++++++++++++++++++++++++------------
 1 file changed, 75 insertions(+), 32 deletions(-)

-- 
1.8.3.1

[PATCH 0/8] clocksource: sunxi: Timer fixes and cleanup

[Maxime Ripard]

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 from the free running counter found in the
A10/A13 SoCs.

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
 
Maxime Ripard (8):
  clocksource: sun4i: Use the BIT macros where possible
  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 | 107 ++++++++++++++++++++++++++------------
 1 file changed, 75 insertions(+), 32 deletions(-)

-- 
1.8.3.1

[PATCH 1/8] clocksource: sun4i: Use the BIT macros where possible

[Maxime Ripard]

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/clocksource/sun4i_timer.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/clocksource/sun4i_timer.c b/drivers/clocksource/sun4i_timer.c
index d4674e7..bdf34d9 100644
--- a/drivers/clocksource/sun4i_timer.c
+++ b/drivers/clocksource/sun4i_timer.c
@@ -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)
 
-- 
1.8.3.1

[PATCH 1/8] clocksource: sun4i: Use the BIT macros where possible

[Maxime Ripard]

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/clocksource/sun4i_timer.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/clocksource/sun4i_timer.c b/drivers/clocksource/sun4i_timer.c
index d4674e7..bdf34d9 100644
--- a/drivers/clocksource/sun4i_timer.c
+++ b/drivers/clocksource/sun4i_timer.c
@@ -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)
 
-- 
1.8.3.1

[PATCH 2/8] clocksource: sun4i: Add clocksource and sched clock drivers

[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 <maxime.ripard@free-electrons.com>
---
 drivers/clocksource/sun4i_timer.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/clocksource/sun4i_timer.c b/drivers/clocksource/sun4i_timer.c
index bdf34d9..1d2eaa0 100644
--- a/drivers/clocksource/sun4i_timer.c
+++ b/drivers/clocksource/sun4i_timer.c
@@ -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,
 };
 
+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);
+
+	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));
 
-- 
1.8.3.1

[PATCH 2/8] clocksource: sun4i: Add clocksource and sched clock drivers

[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 <maxime.ripard@free-electrons.com>
---
 drivers/clocksource/sun4i_timer.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/clocksource/sun4i_timer.c b/drivers/clocksource/sun4i_timer.c
index bdf34d9..1d2eaa0 100644
--- a/drivers/clocksource/sun4i_timer.c
+++ b/drivers/clocksource/sun4i_timer.c
@@ -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,
 };
 
+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);
+
+	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));
 
-- 
1.8.3.1

[PATCH 3/8] clocksource: sun4i: Don't forget to enable the clock we use

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

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/clocksource/sun4i_timer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clocksource/sun4i_timer.c b/drivers/clocksource/sun4i_timer.c
index 1d2eaa0..8bd66df 100644
--- a/drivers/clocksource/sun4i_timer.c
+++ b/drivers/clocksource/sun4i_timer.c
@@ -135,6 +135,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);
 
-- 
1.8.3.1

[PATCH 3/8] clocksource: sun4i: Don't forget to enable the clock we use

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

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/clocksource/sun4i_timer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clocksource/sun4i_timer.c b/drivers/clocksource/sun4i_timer.c
index 1d2eaa0..8bd66df 100644
--- a/drivers/clocksource/sun4i_timer.c
+++ b/drivers/clocksource/sun4i_timer.c
@@ -135,6 +135,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);
 
-- 
1.8.3.1

[PATCH 4/8] clocksource: sun4i: Fix the next event code

[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, so this code has always been broken.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/clocksource/sun4i_timer.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/clocksource/sun4i_timer.c b/drivers/clocksource/sun4i_timer.c
index 8bd66df..032e504 100644
--- a/drivers/clocksource/sun4i_timer.c
+++ b/drivers/clocksource/sun4i_timer.c
@@ -16,6 +16,7 @@
 
 #include <linux/clk.h>
 #include <linux/clockchips.h>
+#include <linux/delay.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/irqreturn.h>
@@ -69,9 +70,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));
+	udelay(1);
+
+	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;
-- 
1.8.3.1

[PATCH 4/8] clocksource: sun4i: Fix the next event code

[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, so this code has always been broken.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/clocksource/sun4i_timer.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/clocksource/sun4i_timer.c b/drivers/clocksource/sun4i_timer.c
index 8bd66df..032e504 100644
--- a/drivers/clocksource/sun4i_timer.c
+++ b/drivers/clocksource/sun4i_timer.c
@@ -16,6 +16,7 @@
 
 #include <linux/clk.h>
 #include <linux/clockchips.h>
+#include <linux/delay.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/irqreturn.h>
@@ -69,9 +70,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));
+	udelay(1);
+
+	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;
-- 
1.8.3.1

[PATCH 5/8] clocksource: sun4i: Factor out some timer code

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

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/clocksource/sun4i_timer.c | 48 ++++++++++++++++++++++++++-------------
 1 file changed, 32 insertions(+), 16 deletions(-)

diff --git a/drivers/clocksource/sun4i_timer.c b/drivers/clocksource/sun4i_timer.c
index 032e504..7f3b248 100644
--- a/drivers/clocksource/sun4i_timer.c
+++ b/drivers/clocksource/sun4i_timer.c
@@ -45,24 +45,46 @@
 
 static void __iomem *timer_base;
 
+static void sun4i_clkevt_time_stop(void)
+{
+	u32 val = readl(timer_base + TIMER_CTL_REG(0));
+	writel(val & ~TIMER_CTL_ENABLE, timer_base + TIMER_CTL_REG(0));
+	udelay(1);
+}
+
+static void sun4i_clkevt_time_setup(unsigned long delay)
+{
+	writel(delay, timer_base + TIMER_INTVAL_REG(0));
+}
+
+static void sun4i_clkevt_time_start(bool periodic)
+{
+	u32 val = readl(timer_base + TIMER_CTL_REG(0));
+
+	if (periodic)
+		val &= ~TIMER_CTL_ONESHOT;
+	else
+		val |= TIMER_CTL_ONESHOT;
+
+	writel(val | TIMER_CTL_ENABLE, timer_base + TIMER_CTL_REG(0));
+}
+
 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();
+		sun4i_clkevt_time_start(true);
 		break;
-
 	case CLOCK_EVT_MODE_ONESHOT:
-		writel(u | TIMER_CTL_ONESHOT, timer_base + TIMER_CTL_REG(0));
+		sun4i_clkevt_time_stop();
+		sun4i_clkevt_time_start(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();
 		break;
 	}
 }
@@ -70,15 +92,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));
-	udelay(1);
-
-	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();
+	sun4i_clkevt_time_setup(evt);
+	sun4i_clkevt_time_start(false);
 
 	return 0;
 }
-- 
1.8.3.1

[PATCH 5/8] clocksource: sun4i: Factor out some timer code

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

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/clocksource/sun4i_timer.c | 48 ++++++++++++++++++++++++++-------------
 1 file changed, 32 insertions(+), 16 deletions(-)

diff --git a/drivers/clocksource/sun4i_timer.c b/drivers/clocksource/sun4i_timer.c
index 032e504..7f3b248 100644
--- a/drivers/clocksource/sun4i_timer.c
+++ b/drivers/clocksource/sun4i_timer.c
@@ -45,24 +45,46 @@
 
 static void __iomem *timer_base;
 
+static void sun4i_clkevt_time_stop(void)
+{
+	u32 val = readl(timer_base + TIMER_CTL_REG(0));
+	writel(val & ~TIMER_CTL_ENABLE, timer_base + TIMER_CTL_REG(0));
+	udelay(1);
+}
+
+static void sun4i_clkevt_time_setup(unsigned long delay)
+{
+	writel(delay, timer_base + TIMER_INTVAL_REG(0));
+}
+
+static void sun4i_clkevt_time_start(bool periodic)
+{
+	u32 val = readl(timer_base + TIMER_CTL_REG(0));
+
+	if (periodic)
+		val &= ~TIMER_CTL_ONESHOT;
+	else
+		val |= TIMER_CTL_ONESHOT;
+
+	writel(val | TIMER_CTL_ENABLE, timer_base + TIMER_CTL_REG(0));
+}
+
 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();
+		sun4i_clkevt_time_start(true);
 		break;
-
 	case CLOCK_EVT_MODE_ONESHOT:
-		writel(u | TIMER_CTL_ONESHOT, timer_base + TIMER_CTL_REG(0));
+		sun4i_clkevt_time_stop();
+		sun4i_clkevt_time_start(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();
 		break;
 	}
 }
@@ -70,15 +92,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));
-	udelay(1);
-
-	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();
+	sun4i_clkevt_time_setup(evt);
+	sun4i_clkevt_time_start(false);
 
 	return 0;
 }
-- 
1.8.3.1

[PATCH 6/8] clocksource: sun4i: Remove TIMER_SCAL variable

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

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/clocksource/sun4i_timer.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/clocksource/sun4i_timer.c b/drivers/clocksource/sun4i_timer.c
index 7f3b248..912d3e0 100644
--- a/drivers/clocksource/sun4i_timer.c
+++ b/drivers/clocksource/sun4i_timer.c
@@ -36,7 +36,6 @@
 #define TIMER_INTVAL_REG(val)	(0x10 * val + 0x14)
 #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)
@@ -141,7 +140,6 @@ static cycle_t sun4i_timer_clksrc_read(struct clocksource *c)
 
 static void __init sun4i_timer_init(struct device_node *node)
 {
-	unsigned long rate = 0;
 	struct clk *clk;
 	int ret, irq;
 	u32 val;
@@ -159,15 +157,13 @@ static void __init sun4i_timer_init(struct device_node *node)
 		panic("Can't get timer clock");
 	clk_prepare_enable(clk);
 
-	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),
+	writel(clk_get_rate(clk) / HZ,
 	       timer_base + TIMER_INTVAL_REG(0));
 
 	/* set clock source to HOSC, 16 pre-division */
@@ -191,8 +187,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, clk_get_rate(clk),
+					0x1, 0xffffffff);
 }
 CLOCKSOURCE_OF_DECLARE(sun4i, "allwinner,sun4i-timer",
 		       sun4i_timer_init);
-- 
1.8.3.1

[PATCH 6/8] clocksource: sun4i: Remove TIMER_SCAL variable

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

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/clocksource/sun4i_timer.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/clocksource/sun4i_timer.c b/drivers/clocksource/sun4i_timer.c
index 7f3b248..912d3e0 100644
--- a/drivers/clocksource/sun4i_timer.c
+++ b/drivers/clocksource/sun4i_timer.c
@@ -36,7 +36,6 @@
 #define TIMER_INTVAL_REG(val)	(0x10 * val + 0x14)
 #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)
@@ -141,7 +140,6 @@ static cycle_t sun4i_timer_clksrc_read(struct clocksource *c)
 
 static void __init sun4i_timer_init(struct device_node *node)
 {
-	unsigned long rate = 0;
 	struct clk *clk;
 	int ret, irq;
 	u32 val;
@@ -159,15 +157,13 @@ static void __init sun4i_timer_init(struct device_node *node)
 		panic("Can't get timer clock");
 	clk_prepare_enable(clk);
 
-	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),
+	writel(clk_get_rate(clk) / HZ,
 	       timer_base + TIMER_INTVAL_REG(0));
 
 	/* set clock source to HOSC, 16 pre-division */
@@ -191,8 +187,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, clk_get_rate(clk),
+					0x1, 0xffffffff);
 }
 CLOCKSOURCE_OF_DECLARE(sun4i, "allwinner,sun4i-timer",
 		       sun4i_timer_init);
-- 
1.8.3.1

[PATCH 7/8] clocksource: sun4i: Cleanup parent clock setup

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

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/clocksource/sun4i_timer.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/clocksource/sun4i_timer.c b/drivers/clocksource/sun4i_timer.c
index 912d3e0..98e38a6 100644
--- a/drivers/clocksource/sun4i_timer.c
+++ b/drivers/clocksource/sun4i_timer.c
@@ -32,6 +32,9 @@
 #define TIMER_CTL_REG(val)	(0x10 * val + 0x10)
 #define TIMER_CTL_ENABLE		BIT(0)
 #define TIMER_CTL_AUTORELOAD		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)
@@ -166,16 +169,8 @@ static void __init sun4i_timer_init(struct device_node *node)
 	writel(clk_get_rate(clk) / 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_AUTORELOAD, timer_base + TIMER_CTL_REG(0));
+	writel(TIMER_CTL_CLK_SRC(TIMER_CTL_CLK_SRC_OSC24M) | TIMER_CTL_AUTORELOAD,
+	       timer_base + TIMER_CTL_REG(0));
 
 	ret = setup_irq(irq, &sun4i_timer_irq);
 	if (ret)
-- 
1.8.3.1

[PATCH 7/8] clocksource: sun4i: Cleanup parent clock setup

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

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/clocksource/sun4i_timer.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/clocksource/sun4i_timer.c b/drivers/clocksource/sun4i_timer.c
index 912d3e0..98e38a6 100644
--- a/drivers/clocksource/sun4i_timer.c
+++ b/drivers/clocksource/sun4i_timer.c
@@ -32,6 +32,9 @@
 #define TIMER_CTL_REG(val)	(0x10 * val + 0x10)
 #define TIMER_CTL_ENABLE		BIT(0)
 #define TIMER_CTL_AUTORELOAD		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)
@@ -166,16 +169,8 @@ static void __init sun4i_timer_init(struct device_node *node)
 	writel(clk_get_rate(clk) / 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_AUTORELOAD, timer_base + TIMER_CTL_REG(0));
+	writel(TIMER_CTL_CLK_SRC(TIMER_CTL_CLK_SRC_OSC24M) | TIMER_CTL_AUTORELOAD,
+	       timer_base + TIMER_CTL_REG(0));
 
 	ret = setup_irq(irq, &sun4i_timer_irq);
 	if (ret)
-- 
1.8.3.1

[PATCH 8/8] clocksource: sun4i: Fix bug when switching from periodic to oneshot modes

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

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/clocksource/sun4i_timer.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/clocksource/sun4i_timer.c b/drivers/clocksource/sun4i_timer.c
index 98e38a6..b7e1f9e 100644
--- a/drivers/clocksource/sun4i_timer.c
+++ b/drivers/clocksource/sun4i_timer.c
@@ -46,6 +46,7 @@
 #define TIMER_CNT64_HIGH_REG	0xa8
 
 static void __iomem *timer_base;
+static u32 ticks_per_jiffy;
 
 static void sun4i_clkevt_time_stop(void)
 {
@@ -68,7 +69,8 @@ static void sun4i_clkevt_time_start(bool periodic)
 	else
 		val |= TIMER_CTL_ONESHOT;
 
-	writel(val | TIMER_CTL_ENABLE, timer_base + TIMER_CTL_REG(0));
+	writel(val | TIMER_CTL_ENABLE | TIMER_CTL_AUTORELOAD,
+	       timer_base + TIMER_CTL_REG(0));
 }
 
 static void sun4i_clkevt_mode(enum clock_event_mode mode,
@@ -77,6 +79,7 @@ static void sun4i_clkevt_mode(enum clock_event_mode mode,
 	switch (mode) {
 	case CLOCK_EVT_MODE_PERIODIC:
 		sun4i_clkevt_time_stop();
+		sun4i_clkevt_time_setup(ticks_per_jiffy);
 		sun4i_clkevt_time_start(true);
 		break;
 	case CLOCK_EVT_MODE_ONESHOT:
@@ -166,10 +169,9 @@ static void __init sun4i_timer_init(struct device_node *node)
 			      clk_get_rate(clk), 300, 32,
 			      sun4i_timer_clksrc_read);
 
-	writel(clk_get_rate(clk) / HZ,
-	       timer_base + TIMER_INTVAL_REG(0));
+	ticks_per_jiffy = DIV_ROUND_UP(clk_get_rate(clk), HZ);
 
-	writel(TIMER_CTL_CLK_SRC(TIMER_CTL_CLK_SRC_OSC24M) | TIMER_CTL_AUTORELOAD,
+	writel(TIMER_CTL_CLK_SRC(TIMER_CTL_CLK_SRC_OSC24M),
 	       timer_base + TIMER_CTL_REG(0));
 
 	ret = setup_irq(irq, &sun4i_timer_irq);
-- 
1.8.3.1

[PATCH 8/8] clocksource: sun4i: Fix bug when switching from periodic to oneshot modes

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

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/clocksource/sun4i_timer.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/clocksource/sun4i_timer.c b/drivers/clocksource/sun4i_timer.c
index 98e38a6..b7e1f9e 100644
--- a/drivers/clocksource/sun4i_timer.c
+++ b/drivers/clocksource/sun4i_timer.c
@@ -46,6 +46,7 @@
 #define TIMER_CNT64_HIGH_REG	0xa8
 
 static void __iomem *timer_base;
+static u32 ticks_per_jiffy;
 
 static void sun4i_clkevt_time_stop(void)
 {
@@ -68,7 +69,8 @@ static void sun4i_clkevt_time_start(bool periodic)
 	else
 		val |= TIMER_CTL_ONESHOT;
 
-	writel(val | TIMER_CTL_ENABLE, timer_base + TIMER_CTL_REG(0));
+	writel(val | TIMER_CTL_ENABLE | TIMER_CTL_AUTORELOAD,
+	       timer_base + TIMER_CTL_REG(0));
 }
 
 static void sun4i_clkevt_mode(enum clock_event_mode mode,
@@ -77,6 +79,7 @@ static void sun4i_clkevt_mode(enum clock_event_mode mode,
 	switch (mode) {
 	case CLOCK_EVT_MODE_PERIODIC:
 		sun4i_clkevt_time_stop();
+		sun4i_clkevt_time_setup(ticks_per_jiffy);
 		sun4i_clkevt_time_start(true);
 		break;
 	case CLOCK_EVT_MODE_ONESHOT:
@@ -166,10 +169,9 @@ static void __init sun4i_timer_init(struct device_node *node)
 			      clk_get_rate(clk), 300, 32,
 			      sun4i_timer_clksrc_read);
 
-	writel(clk_get_rate(clk) / HZ,
-	       timer_base + TIMER_INTVAL_REG(0));
+	ticks_per_jiffy = DIV_ROUND_UP(clk_get_rate(clk), HZ);
 
-	writel(TIMER_CTL_CLK_SRC(TIMER_CTL_CLK_SRC_OSC24M) | TIMER_CTL_AUTORELOAD,
+	writel(TIMER_CTL_CLK_SRC(TIMER_CTL_CLK_SRC_OSC24M),
 	       timer_base + TIMER_CTL_REG(0));
 
 	ret = setup_irq(irq, &sun4i_timer_irq);
-- 
1.8.3.1

Re: [PATCH 2/8] clocksource: sun4i: Add clocksource and sched clock drivers

[Daniel Lezcano]

On 06/26/2013 11:16 PM, Maxime Ripard wrote:
> 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 <maxime.ripard@free-electrons.com>
> ---
>  drivers/clocksource/sun4i_timer.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/clocksource/sun4i_timer.c b/drivers/clocksource/sun4i_timer.c
> index bdf34d9..1d2eaa0 100644
> --- a/drivers/clocksource/sun4i_timer.c
> +++ b/drivers/clocksource/sun4i_timer.c
> @@ -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,
>  };
>  
> +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 ?

> +
> +	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),

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

>  	       timer_base + TIMER_INTVAL_REG(0));


-- 
 <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 2/8] clocksource: sun4i: Add clocksource and sched clock drivers

[Daniel Lezcano]

On 06/26/2013 11:16 PM, Maxime Ripard wrote:
> 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 <maxime.ripard@free-electrons.com>
> ---
>  drivers/clocksource/sun4i_timer.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/clocksource/sun4i_timer.c b/drivers/clocksource/sun4i_timer.c
> index bdf34d9..1d2eaa0 100644
> --- a/drivers/clocksource/sun4i_timer.c
> +++ b/drivers/clocksource/sun4i_timer.c
> @@ -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,
>  };
>  
> +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 ?

> +
> +	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),

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

>  	       timer_base + TIMER_INTVAL_REG(0));


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

Re: [PATCH 2/8] clocksource: sun4i: Add clocksource and sched clock drivers

[Baruch Siach]

Hi Maxime,

On Wed, Jun 26, 2013 at 11:16:55PM +0200, Maxime Ripard wrote:
> 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 <maxime.ripard@free-electrons.com>
> ---
>  drivers/clocksource/sun4i_timer.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/clocksource/sun4i_timer.c b/drivers/clocksource/sun4i_timer.c
> index bdf34d9..1d2eaa0 100644
> --- a/drivers/clocksource/sun4i_timer.c
> +++ b/drivers/clocksource/sun4i_timer.c
> @@ -23,6 +23,8 @@
>  #include <linux/of_address.h>
>  #include <linux/of_irq.h>
>  
> +#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).

> +
>  #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,
>  };
>  
> +static u32 sun4i_timer_sched_read(void)

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

baruch

> +{
> +	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));
>  

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH 2/8] clocksource: sun4i: Add clocksource and sched clock drivers

[Baruch Siach]

Hi Maxime,

On Wed, Jun 26, 2013 at 11:16:55PM +0200, Maxime Ripard wrote:
> 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 <maxime.ripard@free-electrons.com>
> ---
>  drivers/clocksource/sun4i_timer.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/clocksource/sun4i_timer.c b/drivers/clocksource/sun4i_timer.c
> index bdf34d9..1d2eaa0 100644
> --- a/drivers/clocksource/sun4i_timer.c
> +++ b/drivers/clocksource/sun4i_timer.c
> @@ -23,6 +23,8 @@
>  #include <linux/of_address.h>
>  #include <linux/of_irq.h>
>  
> +#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).

> +
>  #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,
>  };
>  
> +static u32 sun4i_timer_sched_read(void)

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

baruch

> +{
> +	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));
>  

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [linux-sunxi] [PATCH 0/8] clocksource: sunxi: Timer fixes and cleanup

[Hans de Goede]

Hi,

On 06/26/2013 11:16 PM, Maxime Ripard wrote:
> Hi everyone,
>

<snip>

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

Regards,

Hans

[linux-sunxi] [PATCH 0/8] clocksource: sunxi: Timer fixes and cleanup

[Hans de Goede]

Hi,

On 06/26/2013 11:16 PM, Maxime Ripard wrote:
> Hi everyone,
>

<snip>

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

Regards,

Hans

Re: [PATCH 2/8] clocksource: sun4i: Add clocksource and sched clock drivers

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2448 bytes --]

Hi Daniel,

On Wed, Jun 26, 2013 at 11:27:35PM +0200, Daniel Lezcano wrote:
> On 06/26/2013 11:16 PM, Maxime Ripard wrote:
> > 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 <maxime.ripard@free-electrons.com>
> > ---
> >  drivers/clocksource/sun4i_timer.c | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> > 
> > diff --git a/drivers/clocksource/sun4i_timer.c b/drivers/clocksource/sun4i_timer.c
> > index bdf34d9..1d2eaa0 100644
> > --- a/drivers/clocksource/sun4i_timer.c
> > +++ b/drivers/clocksource/sun4i_timer.c
> > @@ -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,
> >  };
> >  
> > +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 ?

Right.

The AND is wrong as well, it should be TIMER_CNT64_CTL_RL, not the reg
offset.

> >  
> > +	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)), ?

It's actually fixed in a later patch, but yes.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 2/8] clocksource: sun4i: Add clocksource and sched clock drivers

[Maxime Ripard]

Hi Daniel,

On Wed, Jun 26, 2013 at 11:27:35PM +0200, Daniel Lezcano wrote:
> On 06/26/2013 11:16 PM, Maxime Ripard wrote:
> > 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 <maxime.ripard@free-electrons.com>
> > ---
> >  drivers/clocksource/sun4i_timer.c | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> > 
> > diff --git a/drivers/clocksource/sun4i_timer.c b/drivers/clocksource/sun4i_timer.c
> > index bdf34d9..1d2eaa0 100644
> > --- a/drivers/clocksource/sun4i_timer.c
> > +++ b/drivers/clocksource/sun4i_timer.c
> > @@ -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,
> >  };
> >  
> > +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 ?

Right.

The AND is wrong as well, it should be TIMER_CNT64_CTL_RL, not the reg
offset.

> >  
> > +	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)), ?

It's actually fixed in a later patch, but yes.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130627/b138f901/attachment.sig>

Re: [PATCH 2/8] clocksource: sun4i: Add clocksource and sched clock drivers

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2234 bytes --]

Hi Baruch,

On Thu, Jun 27, 2013 at 09:02:34AM +0300, Baruch Siach wrote:
> Hi Maxime,
> 
> On Wed, Jun 26, 2013 at 11:16:55PM +0200, Maxime Ripard wrote:
> > 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 <maxime.ripard@free-electrons.com>
> > ---
> >  drivers/clocksource/sun4i_timer.c | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> > 
> > diff --git a/drivers/clocksource/sun4i_timer.c b/drivers/clocksource/sun4i_timer.c
> > index bdf34d9..1d2eaa0 100644
> > --- a/drivers/clocksource/sun4i_timer.c
> > +++ b/drivers/clocksource/sun4i_timer.c
> > @@ -23,6 +23,8 @@
> >  #include <linux/of_address.h>
> >  #include <linux/of_irq.h>
> >  
> > +#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).

Ah, good to know. Thanks!

> > +
> >  #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,
> >  };
> >  
> > +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.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 2/8] clocksource: sun4i: Add clocksource and sched clock drivers

[Maxime Ripard]

Hi Baruch,

On Thu, Jun 27, 2013 at 09:02:34AM +0300, Baruch Siach wrote:
> Hi Maxime,
> 
> On Wed, Jun 26, 2013 at 11:16:55PM +0200, Maxime Ripard wrote:
> > 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 <maxime.ripard@free-electrons.com>
> > ---
> >  drivers/clocksource/sun4i_timer.c | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> > 
> > diff --git a/drivers/clocksource/sun4i_timer.c b/drivers/clocksource/sun4i_timer.c
> > index bdf34d9..1d2eaa0 100644
> > --- a/drivers/clocksource/sun4i_timer.c
> > +++ b/drivers/clocksource/sun4i_timer.c
> > @@ -23,6 +23,8 @@
> >  #include <linux/of_address.h>
> >  #include <linux/of_irq.h>
> >  
> > +#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).

Ah, good to know. Thanks!

> > +
> >  #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,
> >  };
> >  
> > +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.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130627/0a28a285/attachment.sig>

Re: [linux-sunxi] [PATCH 0/8] clocksource: sunxi: Timer fixes and cleanup

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 679 bytes --]

On Thu, Jun 27, 2013 at 11:27:02AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 06/26/2013 11:16 PM, Maxime Ripard wrote:
> >Hi everyone,
> >
> 
> <snip>
> 
> >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.

Do you have any pointers to these discussions?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[linux-sunxi] [PATCH 0/8] clocksource: sunxi: Timer fixes and cleanup

[Maxime Ripard]

On Thu, Jun 27, 2013 at 11:27:02AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 06/26/2013 11:16 PM, Maxime Ripard wrote:
> >Hi everyone,
> >
> 
> <snip>
> 
> >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.

Do you have any pointers to these discussions?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130627/01305c94/attachment.sig>

Re: [PATCH 2/8] clocksource: sun4i: Add clocksource and sched clock drivers

[Baruch Siach]

Hi Maxime,

On Thu, Jun 27, 2013 at 11:35:58AM +0200, Maxime Ripard wrote:
> On Thu, Jun 27, 2013 at 09:02:34AM +0300, Baruch Siach wrote:
> > On Wed, Jun 26, 2013 at 11:16:55PM +0200, Maxime Ripard wrote:
> > > +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.

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.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH 2/8] clocksource: sun4i: Add clocksource and sched clock drivers

[Baruch Siach]

Hi Maxime,

On Thu, Jun 27, 2013 at 11:35:58AM +0200, Maxime Ripard wrote:
> On Thu, Jun 27, 2013 at 09:02:34AM +0300, Baruch Siach wrote:
> > On Wed, Jun 26, 2013 at 11:16:55PM +0200, Maxime Ripard wrote:
> > > +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.

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.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [linux-sunxi] [PATCH 0/8] clocksource: sunxi: Timer fixes and cleanup

[Hans de Goede]

Hi,

On 06/27/2013 11:43 AM, Maxime Ripard wrote:
> On Thu, Jun 27, 2013 at 11:27:02AM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 06/26/2013 11:16 PM, Maxime Ripard wrote:
>>> Hi everyone,
>>>
>>
>> <snip>
>>
>>> 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.
>
> 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) ?

Regards,

Hans

[linux-sunxi] [PATCH 0/8] clocksource: sunxi: Timer fixes and cleanup

[Hans de Goede]

Hi,

On 06/27/2013 11:43 AM, Maxime Ripard wrote:
> On Thu, Jun 27, 2013 at 11:27:02AM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 06/26/2013 11:16 PM, Maxime Ripard wrote:
>>> Hi everyone,
>>>
>>
>> <snip>
>>
>>> 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.
>
> 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) ?

Regards,

Hans

Re: [linux-sunxi] [PATCH 2/8] clocksource: sun4i: Add clocksource and sched clock drivers

[Siarhei Siamashka]

On Wed, 26 Jun 2013 23:16:55 +0200
Maxime Ripard <maxime.ripard@free-electrons.com> wrote:

> 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 <maxime.ripard@free-electrons.com>
> ---
>  drivers/clocksource/sun4i_timer.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/clocksource/sun4i_timer.c b/drivers/clocksource/sun4i_timer.c
> index bdf34d9..1d2eaa0 100644
> --- a/drivers/clocksource/sun4i_timer.c
> +++ b/drivers/clocksource/sun4i_timer.c
> @@ -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,
>  };
>  
> +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).

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

> +
> +	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));
>  



-- 
Best regards,
Siarhei Siamashka

[linux-sunxi] [PATCH 2/8] clocksource: sun4i: Add clocksource and sched clock drivers

[Siarhei Siamashka]

On Wed, 26 Jun 2013 23:16:55 +0200
Maxime Ripard <maxime.ripard@free-electrons.com> wrote:

> 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 <maxime.ripard@free-electrons.com>
> ---
>  drivers/clocksource/sun4i_timer.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/clocksource/sun4i_timer.c b/drivers/clocksource/sun4i_timer.c
> index bdf34d9..1d2eaa0 100644
> --- a/drivers/clocksource/sun4i_timer.c
> +++ b/drivers/clocksource/sun4i_timer.c
> @@ -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,
>  };
>  
> +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).

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

> +
> +	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));
>  



-- 
Best regards,
Siarhei Siamashka

Re: [linux-sunxi] [PATCH 0/8] clocksource: sunxi: Timer fixes and cleanup

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2146 bytes --]

On Thu, Jun 27, 2013 at 11:54:11AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 06/27/2013 11:43 AM, Maxime Ripard wrote:
> >On Thu, Jun 27, 2013 at 11:27:02AM +0200, Hans de Goede wrote:
> >>Hi,
> >>
> >>On 06/26/2013 11:16 PM, Maxime Ripard wrote:
> >>>Hi everyone,
> >>>
> >>
> >><snip>
> >>
> >>>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.
> >
> >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

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

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

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[linux-sunxi] [PATCH 0/8] clocksource: sunxi: Timer fixes and cleanup

[Maxime Ripard]

On Thu, Jun 27, 2013 at 11:54:11AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 06/27/2013 11:43 AM, Maxime Ripard wrote:
> >On Thu, Jun 27, 2013 at 11:27:02AM +0200, Hans de Goede wrote:
> >>Hi,
> >>
> >>On 06/26/2013 11:16 PM, Maxime Ripard wrote:
> >>>Hi everyone,
> >>>
> >>
> >><snip>
> >>
> >>>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.
> >
> >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

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

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

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130627/9055044f/attachment.sig>

Re: [linux-sunxi] [PATCH 2/8] clocksource: sun4i: Add clocksource and sched clock drivers

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2899 bytes --]

Hi Siarhei,

On Thu, Jun 27, 2013 at 01:17:29PM +0300, Siarhei Siamashka wrote:
> On Wed, 26 Jun 2013 23:16:55 +0200
> Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> 
> > 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 <maxime.ripard@free-electrons.com>
> > ---
> >  drivers/clocksource/sun4i_timer.c | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> > 
> > diff --git a/drivers/clocksource/sun4i_timer.c b/drivers/clocksource/sun4i_timer.c
> > index bdf34d9..1d2eaa0 100644
> > --- a/drivers/clocksource/sun4i_timer.c
> > +++ b/drivers/clocksource/sun4i_timer.c
> > @@ -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,
> >  };
> >  
> > +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?

> 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).
> 
> > +	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.

Thanks for your insight on this.

It does make me more eager to merge the simpler approach first, and then
try to take some shortcuts if needed and safe enough.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[linux-sunxi] [PATCH 2/8] clocksource: sun4i: Add clocksource and sched clock drivers

[Maxime Ripard]

Hi Siarhei,

On Thu, Jun 27, 2013 at 01:17:29PM +0300, Siarhei Siamashka wrote:
> On Wed, 26 Jun 2013 23:16:55 +0200
> Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> 
> > 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 <maxime.ripard@free-electrons.com>
> > ---
> >  drivers/clocksource/sun4i_timer.c | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> > 
> > diff --git a/drivers/clocksource/sun4i_timer.c b/drivers/clocksource/sun4i_timer.c
> > index bdf34d9..1d2eaa0 100644
> > --- a/drivers/clocksource/sun4i_timer.c
> > +++ b/drivers/clocksource/sun4i_timer.c
> > @@ -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,
> >  };
> >  
> > +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?

> 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).
> 
> > +	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.

Thanks for your insight on this.

It does make me more eager to merge the simpler approach first, and then
try to take some shortcuts if needed and safe enough.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130627/046606b3/attachment.sig>

Re: [PATCH 2/8] clocksource: sun4i: Add clocksource and sched clock drivers

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1338 bytes --]

Hi Baruch,

On Thu, Jun 27, 2013 at 12:46:49PM +0300, Baruch Siach wrote:
> On Thu, Jun 27, 2013 at 11:35:58AM +0200, Maxime Ripard wrote:
> > On Thu, Jun 27, 2013 at 09:02:34AM +0300, Baruch Siach wrote:
> > > On Wed, Jun 26, 2013 at 11:16:55PM +0200, Maxime Ripard wrote:
> > > > +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.
> 
> 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 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.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 2/8] clocksource: sun4i: Add clocksource and sched clock drivers

[Maxime Ripard]

Hi Baruch,

On Thu, Jun 27, 2013 at 12:46:49PM +0300, Baruch Siach wrote:
> On Thu, Jun 27, 2013 at 11:35:58AM +0200, Maxime Ripard wrote:
> > On Thu, Jun 27, 2013 at 09:02:34AM +0300, Baruch Siach wrote:
> > > On Wed, Jun 26, 2013 at 11:16:55PM +0200, Maxime Ripard wrote:
> > > > +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.
> 
> 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 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.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130627/7719f97c/attachment.sig>

Re: [PATCH 2/8] clocksource: sun4i: Add clocksource and sched clock drivers

[Baruch Siach]

Hi Maxime,

On Thu, Jun 27, 2013 at 07:21:44PM +0200, Maxime Ripard wrote:
> On Thu, Jun 27, 2013 at 12:46:49PM +0300, Baruch Siach wrote:
> > On Thu, Jun 27, 2013 at 11:35:58AM +0200, Maxime Ripard wrote:
> > > On Thu, Jun 27, 2013 at 09:02:34AM +0300, Baruch Siach wrote:
> > > > On Wed, Jun 26, 2013 at 11:16:55PM +0200, Maxime Ripard wrote:
> > > > > +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.
> > 
> > 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.

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

Sound reasonable.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH 2/8] clocksource: sun4i: Add clocksource and sched clock drivers

[Baruch Siach]

Hi Maxime,

On Thu, Jun 27, 2013 at 07:21:44PM +0200, Maxime Ripard wrote:
> On Thu, Jun 27, 2013 at 12:46:49PM +0300, Baruch Siach wrote:
> > On Thu, Jun 27, 2013 at 11:35:58AM +0200, Maxime Ripard wrote:
> > > On Thu, Jun 27, 2013 at 09:02:34AM +0300, Baruch Siach wrote:
> > > > On Wed, Jun 26, 2013 at 11:16:55PM +0200, Maxime Ripard wrote:
> > > > > +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.
> > 
> > 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.

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

Sound reasonable.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [linux-sunxi] [PATCH 0/8] clocksource: sunxi: Timer fixes and cleanup

[Hans de Goede]

Hi,

On 06/27/2013 06:54 PM, Maxime Ripard wrote:
> On Thu, Jun 27, 2013 at 11:54:11AM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 06/27/2013 11:43 AM, Maxime Ripard wrote:
>>> On Thu, Jun 27, 2013 at 11:27:02AM +0200, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 06/26/2013 11:16 PM, Maxime Ripard wrote:
>>>>> Hi everyone,
>>>>>
>>>>
>>>> <snip>
>>>>
>>>>> 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.
>>>
>>> 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
>
>> 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.

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

Regards,

Hans

[linux-sunxi] [PATCH 0/8] clocksource: sunxi: Timer fixes and cleanup

[Hans de Goede]

Hi,

On 06/27/2013 06:54 PM, Maxime Ripard wrote:
> On Thu, Jun 27, 2013 at 11:54:11AM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 06/27/2013 11:43 AM, Maxime Ripard wrote:
>>> On Thu, Jun 27, 2013 at 11:27:02AM +0200, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 06/26/2013 11:16 PM, Maxime Ripard wrote:
>>>>> Hi everyone,
>>>>>
>>>>
>>>> <snip>
>>>>
>>>>> 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.
>>>
>>> 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
>
>> 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.

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

Regards,

Hans

Re: [PATCH 2/8] clocksource: sun4i: Add clocksource and sched clock drivers

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1669 bytes --]

On Thu, Jun 27, 2013 at 08:36:43PM +0300, Baruch Siach wrote:
> Hi Maxime,
> 
> On Thu, Jun 27, 2013 at 07:21:44PM +0200, Maxime Ripard wrote:
> > On Thu, Jun 27, 2013 at 12:46:49PM +0300, Baruch Siach wrote:
> > > On Thu, Jun 27, 2013 at 11:35:58AM +0200, Maxime Ripard wrote:
> > > > On Thu, Jun 27, 2013 at 09:02:34AM +0300, Baruch Siach wrote:
> > > > > On Wed, Jun 26, 2013 at 11:16:55PM +0200, Maxime Ripard wrote:
> > > > > > +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.
> > > 
> > > 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.

Which is a deal-breaker for us.

I'll use the setup_sched_clock_64 introduced by Stephen then :)

Thanks for the time you took to review these patches!

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 2/8] clocksource: sun4i: Add clocksource and sched clock drivers

[Maxime Ripard]

On Thu, Jun 27, 2013 at 08:36:43PM +0300, Baruch Siach wrote:
> Hi Maxime,
> 
> On Thu, Jun 27, 2013 at 07:21:44PM +0200, Maxime Ripard wrote:
> > On Thu, Jun 27, 2013 at 12:46:49PM +0300, Baruch Siach wrote:
> > > On Thu, Jun 27, 2013 at 11:35:58AM +0200, Maxime Ripard wrote:
> > > > On Thu, Jun 27, 2013 at 09:02:34AM +0300, Baruch Siach wrote:
> > > > > On Wed, Jun 26, 2013 at 11:16:55PM +0200, Maxime Ripard wrote:
> > > > > > +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.
> > > 
> > > 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.

Which is a deal-breaker for us.

I'll use the setup_sched_clock_64 introduced by Stephen then :)

Thanks for the time you took to review these patches!

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130627/4b45de4e/attachment.sig>

Re: [linux-sunxi] [PATCH 2/8] clocksource: sun4i: Add clocksource and sched clock drivers

[Siarhei Siamashka]

On Thu, 27 Jun 2013 19:02:28 +0200
Maxime Ripard <maxime.ripard@free-electrons.com> wrote:

> Hi Siarhei,
> 
> On Thu, Jun 27, 2013 at 01:17:29PM +0300, Siarhei Siamashka wrote:
> > On Wed, 26 Jun 2013 23:16:55 +0200
> > Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> > 
> > > 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 <maxime.ripard@free-electrons.com>
> > > ---
> > >  drivers/clocksource/sun4i_timer.c | 27 +++++++++++++++++++++++++++
> > >  1 file changed, 27 insertions(+)
> > > 
> > > diff --git a/drivers/clocksource/sun4i_timer.c b/drivers/clocksource/sun4i_timer.c
> > > index bdf34d9..1d2eaa0 100644
> > > --- a/drivers/clocksource/sun4i_timer.c
> > > +++ b/drivers/clocksource/sun4i_timer.c
> > > @@ -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,
> > >  };
> > >  
> > > +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?

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?

-- 
Best regards,
Siarhei Siamashka

[linux-sunxi] [PATCH 2/8] clocksource: sun4i: Add clocksource and sched clock drivers

[Siarhei Siamashka]

On Thu, 27 Jun 2013 19:02:28 +0200
Maxime Ripard <maxime.ripard@free-electrons.com> wrote:

> Hi Siarhei,
> 
> On Thu, Jun 27, 2013 at 01:17:29PM +0300, Siarhei Siamashka wrote:
> > On Wed, 26 Jun 2013 23:16:55 +0200
> > Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> > 
> > > 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 <maxime.ripard@free-electrons.com>
> > > ---
> > >  drivers/clocksource/sun4i_timer.c | 27 +++++++++++++++++++++++++++
> > >  1 file changed, 27 insertions(+)
> > > 
> > > diff --git a/drivers/clocksource/sun4i_timer.c b/drivers/clocksource/sun4i_timer.c
> > > index bdf34d9..1d2eaa0 100644
> > > --- a/drivers/clocksource/sun4i_timer.c
> > > +++ b/drivers/clocksource/sun4i_timer.c
> > > @@ -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,
> > >  };
> > >  
> > > +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?

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?

-- 
Best regards,
Siarhei Siamashka

Re: [linux-sunxi] [PATCH 0/8] clocksource: sunxi: Timer fixes and cleanup

[Siarhei Siamashka]

On Thu, 27 Jun 2013 18:54:36 +0200
Maxime Ripard <maxime.ripard@free-electrons.com> wrote:

> On Thu, Jun 27, 2013 at 11:54:11AM +0200, Hans de Goede wrote:
> > 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?

-- 
Best regards,
Siarhei Siamashka

[linux-sunxi] [PATCH 0/8] clocksource: sunxi: Timer fixes and cleanup

[Siarhei Siamashka]

On Thu, 27 Jun 2013 18:54:36 +0200
Maxime Ripard <maxime.ripard@free-electrons.com> wrote:

> On Thu, Jun 27, 2013 at 11:54:11AM +0200, Hans de Goede wrote:
> > 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?

-- 
Best regards,
Siarhei Siamashka

Re: [linux-sunxi] [PATCH 0/8] clocksource: sunxi: Timer fixes and cleanup

[Hans de Goede]

Hi,

On 06/27/2013 10:26 PM, Siarhei Siamashka wrote:
> On Thu, 27 Jun 2013 18:54:36 +0200
> Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
>
>> On Thu, Jun 27, 2013 at 11:54:11AM +0200, Hans de Goede wrote:
>>> 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?

No, because if ie CNT64_LO_REG old is 0xffffffff and CNT64_LO_REG new is
say 0x00000001, and we do get the new CNT64_HI_REG things will break.

Regards,

Hans

[linux-sunxi] [PATCH 0/8] clocksource: sunxi: Timer fixes and cleanup

[Hans de Goede]

Hi,

On 06/27/2013 10:26 PM, Siarhei Siamashka wrote:
> On Thu, 27 Jun 2013 18:54:36 +0200
> Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
>
>> On Thu, Jun 27, 2013 at 11:54:11AM +0200, Hans de Goede wrote:
>>> 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?

No, because if ie CNT64_LO_REG old is 0xffffffff and CNT64_LO_REG new is
say 0x00000001, and we do get the new CNT64_HI_REG things will break.

Regards,

Hans

[linux-sunxi] [PATCH 0/8] clocksource: sunxi: Timer fixes and cleanup

[Siarhei Siamashka]

On Fri, 28 Jun 2013 09:43:37 +0800
?? <kevin@allwinnertech.com> wrote:

> > 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?
> 
> 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
the critical code between LDREX and STREX instructions (or whatever
higher level wrappers are available for them). In the case of
contention and reading non-latched garbage value from CNT64_LO_REG
(or from CNT64_LO_REG/CNT64_HI_REG pair if we go 64-bit), this
critical block of code can be just retried again until it eventually
succeeds.

-- 
Best regards,
Siarhei Siamashka

Re: [linux-sunxi] [PATCH 2/8] clocksource: sun4i: Add clocksource and sched clock drivers

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1744 bytes --]

Hi Siarhei,

> > > 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?

Ah, I get what you mean now. We don't even need to bother about
TIMER_CNT64_CTL_CLR now, since it's suppose to be cleared once the
counter is reset.

However, I'd very much prefer to take the safer approach for now, and
try to optimise afterwards.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[linux-sunxi] [PATCH 2/8] clocksource: sun4i: Add clocksource and sched clock drivers

[Maxime Ripard]

Hi Siarhei,

> > > 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?

Ah, I get what you mean now. We don't even need to bother about
TIMER_CNT64_CTL_CLR now, since it's suppose to be cleared once the
counter is reset.

However, I'd very much prefer to take the safer approach for now, and
try to optimise afterwards.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130628/844d3b16/attachment.sig>

Re: [linux-sunxi] [PATCH 0/8] clocksource: sunxi: Timer fixes and cleanup

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1984 bytes --]

On Fri, 28 Jun 2013, Siarhei Siamashka wrote:
> On Fri, 28 Jun 2013 09:43:37 +0800
> 张猛 <kevin@allwinnertech.com> wrote:
> 
> > > 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?
> > 
> > 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

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.

Thanks,

	tglx
	

[linux-sunxi] [PATCH 0/8] clocksource: sunxi: Timer fixes and cleanup

[Thomas Gleixner]

On Fri, 28 Jun 2013, Siarhei Siamashka wrote:
> On Fri, 28 Jun 2013 09:43:37 +0800
> ?? <kevin@allwinnertech.com> wrote:
> 
> > > 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?
> > 
> > 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

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.

Thanks,

	tglx
	

[linux-sunxi] [PATCH 0/8] clocksource: sunxi: Timer fixes and cleanup

[Siarhei Siamashka]

On Fri, 28 Jun 2013 09:43:37 +0800
?? <kevin@allwinnertech.com> wrote:

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

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.

-- 
Best regards,
Siarhei Siamashka

Re: [linux-sunxi] [PATCH 0/8] clocksource: sunxi: Timer fixes and cleanup

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 882 bytes --]

Hi Hans,

On Thu, Jun 27, 2013 at 08:13:56PM +0200, Hans de Goede wrote:
> >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.
> 
> 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 ...

And you were right, since I wasn't aware of such discussion.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[linux-sunxi] [PATCH 0/8] clocksource: sunxi: Timer fixes and cleanup

[Maxime Ripard]

Hi Hans,

On Thu, Jun 27, 2013 at 08:13:56PM +0200, Hans de Goede wrote:
> >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.
> 
> 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 ...

And you were right, since I wasn't aware of such discussion.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130628/57aa2e25/attachment.sig>

Re: [linux-sunxi] [PATCH 0/8] clocksource: sunxi: Timer fixes and cleanup

[Siarhei Siamashka]

On Fri, 28 Jun 2013 12:26:10 +0200 (CEST)
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Fri, 28 Jun 2013, Siarhei Siamashka wrote:
> > On Fri, 28 Jun 2013 09:43:37 +0800
> > 张猛 <kevin@allwinnertech.com> wrote:
> > 
> > > > 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?
> > > 
> > > 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
> 
> 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.

Still I would not want some high priority and latency sensitive junk
(such as pulseaudio for example) not to get scheduled at the right
time because of some low priority gettimeofday spammer application.
The time spent latching and reading these counters is non negligible.

Though I agree Maxime that first we need an initial implementation,
which just works safely. And then we can optimize it by testing real
use cases and providing the relevant benchmark numbers. Knowing the
precise details about the hardware helps.

But enough bikeshedding, it's time for me to shut up :)

-- 
Best regards,
Siarhei Siamashka

[linux-sunxi] [PATCH 0/8] clocksource: sunxi: Timer fixes and cleanup

[Siarhei Siamashka]

On Fri, 28 Jun 2013 12:26:10 +0200 (CEST)
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Fri, 28 Jun 2013, Siarhei Siamashka wrote:
> > On Fri, 28 Jun 2013 09:43:37 +0800
> > ?? <kevin@allwinnertech.com> wrote:
> > 
> > > > 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?
> > > 
> > > 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
> 
> 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.

Still I would not want some high priority and latency sensitive junk
(such as pulseaudio for example) not to get scheduled at the right
time because of some low priority gettimeofday spammer application.
The time spent latching and reading these counters is non negligible.

Though I agree Maxime that first we need an initial implementation,
which just works safely. And then we can optimize it by testing real
use cases and providing the relevant benchmark numbers. Knowing the
precise details about the hardware helps.

But enough bikeshedding, it's time for me to shut up :)

-- 
Best regards,
Siarhei Siamashka

Re: Re: [linux-sunxi] [PATCH 0/8] clocksource: sunxi: Timer fixes and cleanup

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1677 bytes --]

On Fri, 28 Jun 2013, 张猛 wrote:
> > 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?
> 
> 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?

Thanks,

	tglx

[linux-sunxi] [PATCH 0/8] clocksource: sunxi: Timer fixes and cleanup

[Thomas Gleixner]

On Fri, 28 Jun 2013, ?? wrote:
> > 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?
> 
> 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?

Thanks,

	tglx

Re: [linux-sunxi] [PATCH 0/8] clocksource: sunxi: Timer fixes and cleanup

[maxime.ripard]

[-- Attachment #1: Type: text/plain, Size: 1706 bytes --]

On Fri, Jun 28, 2013 at 01:29:12PM +0300, Siarhei Siamashka wrote:
> > 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.
> 
> 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.

Come on. This is obviously an automated addition made by the SMTP
server. While it definitely is ironic since it is sent to a public
mailing list, is your comment here really needed?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[linux-sunxi] [PATCH 0/8] clocksource: sunxi: Timer fixes and cleanup

[maxime.ripard]

On Fri, Jun 28, 2013 at 01:29:12PM +0300, Siarhei Siamashka wrote:
> > 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.
> 
> 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.

Come on. This is obviously an automated addition made by the SMTP
server. While it definitely is ironic since it is sent to a public
mailing list, is your comment here really needed?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130628/fc2f7d1e/attachment.sig>

Re: Re: [linux-sunxi] [PATCH 0/8] clocksource: sunxi: Timer fixes and cleanup

[maxime.ripard]

[-- Attachment #1: Type: text/plain, Size: 2222 bytes --]

Hi Thomas,

On Fri, Jun 28, 2013 at 04:02:23PM +0200, Thomas Gleixner wrote:
> On Fri, 28 Jun 2013, 张猛 wrote:
> > > 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?
> > 
> > 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?

Yes, you're right. I actually wanted at first not to use a timer for
this since we had a counter to do just that.

But that's true that actually using a timer would make the code simpler
and presumably faster as well.

I'll change this in the v2.

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[linux-sunxi] [PATCH 0/8] clocksource: sunxi: Timer fixes and cleanup

[maxime.ripard]

Hi Thomas,

On Fri, Jun 28, 2013 at 04:02:23PM +0200, Thomas Gleixner wrote:
> On Fri, 28 Jun 2013, ?? wrote:
> > > 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?
> > 
> > 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?

Yes, you're right. I actually wanted at first not to use a timer for
this since we had a counter to do just that.

But that's true that actually using a timer would make the code simpler
and presumably faster as well.

I'll change this in the v2.

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130628/3705a440/attachment.sig>