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
The A10 and the A13 has a 64 bits free running counter that we can use
as a clocksource and a sched clock, that were both not used yet on these
platforms.
Signed-off-by: Maxime Ripard <[email protected]>
---
drivers/clocksource/sun4i_timer.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
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
Signed-off-by: Maxime Ripard <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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------------{=
- [email protected] - tel: +972.2.679.5364, http://www.tkos.co.il -
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
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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
> > ---
> > 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
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
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------------{=
- [email protected] - tel: +972.2.679.5364, http://www.tkos.co.il -
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
On Wed, 26 Jun 2013 23:16:55 +0200
Maxime Ripard <[email protected]> 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 <[email protected]>
> ---
> 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
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
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 <[email protected]> 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 <[email protected]>
> > ---
> > 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
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
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------------{=
- [email protected] - tel: +972.2.679.5364, http://www.tkos.co.il -
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
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
On Thu, 27 Jun 2013 19:02:28 +0200
Maxime Ripard <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > ---
> > > 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
On Thu, 27 Jun 2013 18:54:36 +0200
Maxime Ripard <[email protected]> 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
Hi,
On 06/27/2013 10:26 PM, Siarhei Siamashka wrote:
> On Thu, 27 Jun 2013 18:54:36 +0200
> Maxime Ripard <[email protected]> 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
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
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