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

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

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

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

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

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

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

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

[Maxime Ripard]

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

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 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

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

[Maxime Ripard]

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

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 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

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

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

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

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

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

[Thomas Gleixner]

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

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

[Thomas Gleixner]

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

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

[Tomasz Figa]

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

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

[Tomasz Figa]

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

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

[Maxime Ripard]

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

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

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

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

[Maxime Ripard]

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 <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 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
-------------- 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/697eeeb2/attachment-0001.sig>

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

[Maxime Ripard]

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

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

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

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

[Maxime Ripard]

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 <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 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
-------------- 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/0f311dd3/attachment.sig>

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

[Thomas Gleixner]

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

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

[Thomas Gleixner]

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

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

[Maxime Ripard]

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

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

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

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

[Maxime Ripard]

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
-------------- 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/20130629/a74496cb/attachment.sig>

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

[Thomas Gleixner]

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

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

[Thomas Gleixner]

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

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

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

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

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

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

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

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

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