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
Changes from v1:
- Rebased on top of linux-next to benefit from the move to all
architectures of the sched_clock functions
- Moved the clock source to the second timer instead of the 64 bits
free-running counter like suggested by Thomas.
Maxime Ripard (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 | 97 ++++++++++++++++++++++++++-------------
1 file changed, 64 insertions(+), 33 deletions(-)
--
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 d6621c5..a77fa29 100644
--- a/drivers/clocksource/sun4i_timer.c
+++ b/drivers/clocksource/sun4i_timer.c
@@ -31,6 +31,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)
@@ -144,16 +147,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 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 | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/clocksource/sun4i_timer.c b/drivers/clocksource/sun4i_timer.c
index 6ef10d9..d6621c5 100644
--- a/drivers/clocksource/sun4i_timer.c
+++ b/drivers/clocksource/sun4i_timer.c
@@ -124,7 +124,6 @@ static u32 sun4i_timer_sched_read(void)
static void __init sun4i_timer_init(struct device_node *node)
{
- unsigned long rate = 0;
struct clk *clk;
int ret, irq;
u32 val;
@@ -142,9 +141,7 @@ 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(rate / (TIMER_SCAL * HZ),
+ writel(clk_get_rate(clk) / HZ,
timer_base + TIMER_INTVAL_REG(0));
/* set clock source to HOSC, 16 pre-division */
@@ -168,8 +165,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);
writel(~0, timer_base + TIMER_INTVAL_REG(1));
writel(TIMER_CTL_ENABLE | TIMER_CTL_AUTORELOAD | TIMER_CTL_CLK_SRC(TIMER_CTL_CLK_SRC_OSC24M),
--
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 84ace76..695c8c8 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>
@@ -61,9 +62,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 050e94f..84ace76 100644
--- a/drivers/clocksource/sun4i_timer.c
+++ b/drivers/clocksource/sun4i_timer.c
@@ -118,6 +118,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
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 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 | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/clocksource/sun4i_timer.c b/drivers/clocksource/sun4i_timer.c
index a77fa29..a2a9088 100644
--- a/drivers/clocksource/sun4i_timer.c
+++ b/drivers/clocksource/sun4i_timer.c
@@ -39,6 +39,7 @@
#define TIMER_CNTVAL_REG(val) (0x10 * val + 0x18)
static void __iomem *timer_base;
+static u32 ticks_per_jiffy;
static void sun4i_clkevt_time_stop(u8 timer)
{
@@ -61,7 +62,8 @@ static void sun4i_clkevt_time_start(u8 timer, bool periodic)
else
val |= TIMER_CTL_ONESHOT;
- writel(val | TIMER_CTL_ENABLE, timer_base + TIMER_CTL_REG(timer));
+ writel(val | TIMER_CTL_ENABLE | TIMER_CTL_AUTORELOAD,
+ timer_base + TIMER_CTL_REG(timer));
}
static void sun4i_clkevt_mode(enum clock_event_mode mode,
@@ -70,6 +72,7 @@ static void sun4i_clkevt_mode(enum clock_event_mode mode,
switch (mode) {
case CLOCK_EVT_MODE_PERIODIC:
sun4i_clkevt_time_stop(0);
+ sun4i_clkevt_time_setup(0, ticks_per_jiffy);
sun4i_clkevt_time_start(0, true);
break;
case CLOCK_EVT_MODE_ONESHOT:
@@ -144,10 +147,10 @@ static void __init sun4i_timer_init(struct device_node *node)
panic("Can't get timer clock");
clk_prepare_enable(clk);
- writel(clk_get_rate(clk) / HZ,
- timer_base + TIMER_INTVAL_REG(0));
- writel(TIMER_CTL_CLK_SRC(TIMER_CTL_CLK_SRC_OSC24M) | TIMER_CTL_AUTORELOAD,
+ ticks_per_jiffy = DIV_ROUND_UP(clk_get_rate(clk), HZ);
+
+ 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
Use the second timer found on the Allwinner SoCs as a clock source and
sched clock, that were both not used yet on these platforms.
Signed-off-by: Maxime Ripard <[email protected]>
---
drivers/clocksource/sun4i_timer.c | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/clocksource/sun4i_timer.c b/drivers/clocksource/sun4i_timer.c
index bdf34d9..050e94f 100644
--- a/drivers/clocksource/sun4i_timer.c
+++ b/drivers/clocksource/sun4i_timer.c
@@ -19,6 +19,7 @@
#include <linux/interrupt.h>
#include <linux/irq.h>
#include <linux/irqreturn.h>
+#include <linux/sched_clock.h>
#include <linux/of.h>
#include <linux/of_address.h>
#include <linux/of_irq.h>
@@ -33,8 +34,6 @@
#define TIMER_INTVAL_REG(val) (0x10 * val + 0x14)
#define TIMER_CNTVAL_REG(val) (0x10 * val + 0x18)
-#define TIMER_SCAL 16
-
static void __iomem *timer_base;
static void sun4i_clkevt_mode(enum clock_event_mode mode,
@@ -96,6 +95,11 @@ static struct irqaction sun4i_timer_irq = {
.dev_id = &sun4i_clockevent,
};
+static u32 sun4i_timer_sched_read(void)
+{
+ return ~readl(timer_base + TIMER_CNTVAL_REG(1));
+}
+
static void __init sun4i_timer_init(struct device_node *node)
{
unsigned long rate = 0;
@@ -143,6 +147,15 @@ static void __init sun4i_timer_init(struct device_node *node)
clockevents_config_and_register(&sun4i_clockevent, rate / TIMER_SCAL,
0x1, 0xff);
+
+ writel(~0, timer_base + TIMER_INTVAL_REG(1));
+ writel(TIMER_CTL_ENABLE | TIMER_CTL_AUTORELOAD | TIMER_CTL_CLK_SRC(TIMER_CTL_CLK_SRC_OSC24M),
+ timer_base + TIMER_CTL_REG(1));
+
+ setup_sched_clock(sun4i_timer_sched_read, 32, clk_get_rate(clk));
+ clocksource_mmio_init(timer_base + TIMER_CNTVAL_REG(1), node->name,
+ clk_get_rate(clk), 300, 32,
+ clocksource_mmio_readl_down);
}
CLOCKSOURCE_OF_DECLARE(sun4i, "allwinner,sun4i-timer",
sun4i_timer_init);
--
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 695c8c8..6ef10d9 100644
--- a/drivers/clocksource/sun4i_timer.c
+++ b/drivers/clocksource/sun4i_timer.c
@@ -37,24 +37,46 @@
static void __iomem *timer_base;
+static void sun4i_clkevt_time_stop(u8 timer)
+{
+ u32 val = readl(timer_base + TIMER_CTL_REG(timer));
+ writel(val & ~TIMER_CTL_ENABLE, timer_base + TIMER_CTL_REG(timer));
+ udelay(1);
+}
+
+static void sun4i_clkevt_time_setup(u8 timer, unsigned long delay)
+{
+ writel(delay, timer_base + TIMER_INTVAL_REG(timer));
+}
+
+static void sun4i_clkevt_time_start(u8 timer, bool periodic)
+{
+ u32 val = readl(timer_base + TIMER_CTL_REG(timer));
+
+ if (periodic)
+ val &= ~TIMER_CTL_ONESHOT;
+ else
+ val |= TIMER_CTL_ONESHOT;
+
+ writel(val | TIMER_CTL_ENABLE, timer_base + TIMER_CTL_REG(timer));
+}
+
static void sun4i_clkevt_mode(enum clock_event_mode mode,
struct clock_event_device *clk)
{
- u32 u = readl(timer_base + TIMER_CTL_REG(0));
-
switch (mode) {
case CLOCK_EVT_MODE_PERIODIC:
- u &= ~(TIMER_CTL_ONESHOT);
- writel(u | TIMER_CTL_ENABLE, timer_base + TIMER_CTL_REG(0));
+ sun4i_clkevt_time_stop(0);
+ sun4i_clkevt_time_start(0, true);
break;
-
case CLOCK_EVT_MODE_ONESHOT:
- writel(u | TIMER_CTL_ONESHOT, timer_base + TIMER_CTL_REG(0));
+ sun4i_clkevt_time_stop(0);
+ sun4i_clkevt_time_start(0, false);
break;
case CLOCK_EVT_MODE_UNUSED:
case CLOCK_EVT_MODE_SHUTDOWN:
default:
- writel(u & ~(TIMER_CTL_ENABLE), timer_base + TIMER_CTL_REG(0));
+ sun4i_clkevt_time_stop(0);
break;
}
}
@@ -62,15 +84,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(0);
+ sun4i_clkevt_time_setup(0, evt);
+ sun4i_clkevt_time_start(0, false);
return 0;
}
--
1.8.3.1
On Fri, 28 Jun 2013, Maxime Ripard wrote:
> 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.
Ok.
> Plus the logic to set the actual value was wrong as well, so this
> code has always been broken.
This lacks an explanation why the logic is wrong and what the actual
fix is.
> 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 84ace76..695c8c8 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>
> @@ -61,9 +62,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);
That udelay() is more than suspicious. Is there really no other way to
deal with that hardware?
If no, you really need to put a proper explanation for that into the code.
Thanks,
tglx
On Friday 28 of June 2013 22:13:08 Thomas Gleixner wrote:
> On Fri, 28 Jun 2013, Maxime Ripard wrote:
> > 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.
>
> Ok.
>
> > Plus the logic to set the actual value was wrong as well, so this
> > code has always been broken.
>
> This lacks an explanation why the logic is wrong and what the actual
> fix is.
>
> > 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 84ace76..695c8c8 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>
> >
> > @@ -61,9 +62,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);
>
> That udelay() is more than suspicious.
Not only it is suspicious, but also delays the event by 1 microsecond. Not
much, given usual usage of clock events, but still.
>From what I understand from this code, you keep this timer running and
just stop it to set new event. Can you simply disable autoreload and just
program this timer to start counting from evt down to 0 when it generates
interrupt and just stops itself?
I believe this would simplify the logic a bit, but is it possible with
this hardware?
Best regards,
Tomasz
Hi Thomas,
On Fri, Jun 28, 2013 at 10:13:08PM +0200, Thomas Gleixner wrote:
> On Fri, 28 Jun 2013, Maxime Ripard wrote:
>
> > 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.
>
> Ok.
>
> > Plus the logic to set the actual value was wrong as well, so this
> > code has always been broken.
>
> This lacks an explanation why the logic is wrong and what the actual
> fix is.
Right.
Actually, the interval register can only be modified when the timer is
disabled. So we first need, to disable it, change the interval, and then
enable it back.
> > 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 84ace76..695c8c8 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>
> > @@ -61,9 +62,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);
>
> That udelay() is more than suspicious. Is there really no other way to
> deal with that hardware?
>
> If no, you really need to put a proper explanation for that into the code.
The datasheet states that you have to wait for two ticks of the timer
clock source (in that case, 24MHz, which makes it around 80-85ns) before
you can actually enable it back.
I didn't came up with a better solution.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
On Fri, Jun 28, 2013 at 10:35:29PM +0200, Tomasz Figa wrote:
> On Friday 28 of June 2013 22:13:08 Thomas Gleixner wrote:
> > On Fri, 28 Jun 2013, Maxime Ripard wrote:
> > > 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.
> >
> > Ok.
> >
> > > Plus the logic to set the actual value was wrong as well, so this
> > > code has always been broken.
> >
> > This lacks an explanation why the logic is wrong and what the actual
> > fix is.
> >
> > > 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 84ace76..695c8c8 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>
> > >
> > > @@ -61,9 +62,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);
> >
> > That udelay() is more than suspicious.
>
> Not only it is suspicious, but also delays the event by 1 microsecond. Not
> much, given usual usage of clock events, but still.
>
> From what I understand from this code, you keep this timer running and
> just stop it to set new event. Can you simply disable autoreload and just
> program this timer to start counting from evt down to 0 when it generates
> interrupt and just stops itself?
Something like that, but not completely, because the timer actually
stops.
To reprogram a new interval to a running timer, you have to:
- Disable it
- Program the new interval
- Propagates the new interval and start the timer by setting the bits
ENABLE and (AUTO)RELOAD (AUTORELOAD is probably a bad name here
actually). That is, wether or not it's a oneshot or periodic timer.
Now, between the time you disable the timer and enable it back, you have
to wait at least 2 timer clock source cycles (which is around 85ns).
It's the ONESHOT (BIT(7)) that actually controls wether or not the timer
is periodic.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
On Fri, 28 Jun 2013, Maxime Ripard wrote:
> On Fri, Jun 28, 2013 at 10:13:08PM +0200, Thomas Gleixner wrote:
> > On Fri, 28 Jun 2013, Maxime Ripard wrote:
> > > @@ -61,9 +62,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);
> >
> > That udelay() is more than suspicious. Is there really no other way to
> > deal with that hardware?
> >
> > If no, you really need to put a proper explanation for that into the code.
>
> The datasheet states that you have to wait for two ticks of the timer
> clock source (in that case, 24MHz, which makes it around 80-85ns) before
> you can actually enable it back.
>
> I didn't came up with a better solution.
80-85ns is definitely way less than 1us.
Why not reading the counter register and wait for 2 or 3 cycles to
elapse instead of wasting a full microsecond evertime ?
Thanks,
tglx
On Fri, Jun 28, 2013 at 11:27:25PM +0200, Thomas Gleixner wrote:
> On Fri, 28 Jun 2013, Maxime Ripard wrote:
> > On Fri, Jun 28, 2013 at 10:13:08PM +0200, Thomas Gleixner wrote:
> > > On Fri, 28 Jun 2013, Maxime Ripard wrote:
> > > > @@ -61,9 +62,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);
> > >
> > > That udelay() is more than suspicious. Is there really no other way to
> > > deal with that hardware?
> > >
> > > If no, you really need to put a proper explanation for that into the code.
> >
> > The datasheet states that you have to wait for two ticks of the timer
> > clock source (in that case, 24MHz, which makes it around 80-85ns) before
> > you can actually enable it back.
> >
> > I didn't came up with a better solution.
>
> 80-85ns is definitely way less than 1us.
>
> Why not reading the counter register and wait for 2 or 3 cycles to
> elapse instead of wasting a full microsecond evertime ?
Yes, but then we fall back to the discussion we had in the v1 about the
latch needed to read the counter, which would already take more time
than what we have to wait for.
Maybe we can use the second timer that we use for the clocksource
though: it's always running, already set up, work at the same rate and
we will only read it, so we won't change its monotonic nature.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
On Sat, 29 Jun 2013, Maxime Ripard wrote:
> On Fri, Jun 28, 2013 at 11:27:25PM +0200, Thomas Gleixner wrote:
> > On Fri, 28 Jun 2013, Maxime Ripard wrote:
> > > On Fri, Jun 28, 2013 at 10:13:08PM +0200, Thomas Gleixner wrote:
> > > > On Fri, 28 Jun 2013, Maxime Ripard wrote:
> > > > > @@ -61,9 +62,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);
> > > >
> > > > That udelay() is more than suspicious. Is there really no other way to
> > > > deal with that hardware?
> > > >
> > > > If no, you really need to put a proper explanation for that into the code.
> > >
> > > The datasheet states that you have to wait for two ticks of the timer
> > > clock source (in that case, 24MHz, which makes it around 80-85ns) before
> > > you can actually enable it back.
> > >
> > > I didn't came up with a better solution.
> >
> > 80-85ns is definitely way less than 1us.
> >
> > Why not reading the counter register and wait for 2 or 3 cycles to
> > elapse instead of wasting a full microsecond evertime ?
>
> Yes, but then we fall back to the discussion we had in the v1 about the
> latch needed to read the counter, which would already take more time
> than what we have to wait for.
>
> Maybe we can use the second timer that we use for the clocksource
> though: it's always running, already set up, work at the same rate and
> we will only read it, so we won't change its monotonic nature.
Right. That will give you exact the information you need and make for
the shortest waiting time.
Thanks,
tglx