[PATCH 1/2] clocksource/drivers/lpc32xx: Support periodic mode

This commit adds the support for periodic mode. This is done by not
setting the MR0S (Stop on TnMR0) bit on MCR, thus allowing
interrupts to be periodically generated on MR0 matches.

In order to do this, move the initial configuration that is specific to
the one shot mode to clock_event_device.set_state_oneshot.

Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
 drivers/clocksource/time-lpc32xx.c | 48 ++++++++++++++++++++++++++++++++------
 1 file changed, 41 insertions(+), 7 deletions(-)

diff --git a/drivers/clocksource/time-lpc32xx.c b/drivers/clocksource/time-lpc32xx.c
index 1316876b487a..9b3d4a38c716 100644
--- a/drivers/clocksource/time-lpc32xx.c
+++ b/drivers/clocksource/time-lpc32xx.c

@@ -47,6 +47,8 @@ struct lpc32xx_clock_event_ddata {
 
 /* Needed for the sched clock */
 static void __iomem *clocksource_timer_counter;
+/* Needed for clockevents periodic mode */
+static u32 ticks_per_jiffy;
 
 static u64 notrace lpc32xx_read_sched_clock(void)
 {

@@ -86,11 +88,42 @@ static int lpc32xx_clkevt_shutdown(struct clock_event_device *evtdev)
 
 static int lpc32xx_clkevt_oneshot(struct clock_event_device *evtdev)
 {
+	struct lpc32xx_clock_event_ddata *ddata =
+		container_of(evtdev, struct lpc32xx_clock_event_ddata, evtdev);
+
 	/*
 	 * When using oneshot, we must also disable the timer
 	 * to wait for the first call to set_next_event().
 	 */
-	return lpc32xx_clkevt_shutdown(evtdev);
+	writel_relaxed(0, ddata->base + LPC32XX_TIMER_TCR);
+
+	/* Enable interrupt, reset on match and stop on match (MCR). */
+	writel_relaxed(LPC32XX_TIMER_MCR_MR0I | LPC32XX_TIMER_MCR_MR0R |
+		       LPC32XX_TIMER_MCR_MR0S, ddata->base + LPC32XX_TIMER_MCR);
+	/* Configure a compare match value of 1 on MR0. */
+	writel_relaxed(1, ddata->base + LPC32XX_TIMER_MR0);
+
+	return 0;
+}
+
+static int lpc32xx_clkevt_periodic(struct clock_event_device *evtdev)
+{
+	struct lpc32xx_clock_event_ddata *ddata =
+		container_of(evtdev, struct lpc32xx_clock_event_ddata, evtdev);
+
+	/* Enable interrupt and reset on match. */
+	writel_relaxed(LPC32XX_TIMER_MCR_MR0I | LPC32XX_TIMER_MCR_MR0R,
+		       ddata->base + LPC32XX_TIMER_MCR);
+	/*
+	 * Place timer in reset and set a match value on MR0. An interrupt will
+	 * be generated each time the counter matches MR0. After setup the
+	 * timer is released from reset and enabled.
+	 */
+	writel_relaxed(LPC32XX_TIMER_TCR_CRST, ddata->base + LPC32XX_TIMER_TCR);
+	writel_relaxed(ticks_per_jiffy, ddata->base + LPC32XX_TIMER_MR0);
+	writel_relaxed(LPC32XX_TIMER_TCR_CEN, ddata->base + LPC32XX_TIMER_TCR);
+
+	return 0;
 }
 
 static irqreturn_t lpc32xx_clock_event_handler(int irq, void *dev_id)

@@ -108,11 +141,14 @@ static irqreturn_t lpc32xx_clock_event_handler(int irq, void *dev_id)
 static struct lpc32xx_clock_event_ddata lpc32xx_clk_event_ddata = {
 	.evtdev = {
 		.name			= "lpc3220 clockevent",
-		.features		= CLOCK_EVT_FEAT_ONESHOT,
+		.features		= CLOCK_EVT_FEAT_ONESHOT |
+					  CLOCK_EVT_FEAT_PERIODIC,
 		.rating			= 300,
 		.set_next_event		= lpc32xx_clkevt_next_event,
 		.set_state_shutdown	= lpc32xx_clkevt_shutdown,
 		.set_state_oneshot	= lpc32xx_clkevt_oneshot,
+		.set_state_periodic	= lpc32xx_clkevt_periodic,
+		.tick_resume		= lpc32xx_clkevt_shutdown,
 	},
 };
 

@@ -210,17 +246,15 @@ static int __init lpc32xx_clockevent_init(struct device_node *np)
 
 	/*
 	 * Disable timer and clear any pending interrupt (IR) on match
-	 * channel 0 (MR0). Configure a compare match value of 1 on MR0
-	 * and enable interrupt, reset on match and stop on match (MCR).
+	 * channel 0 (MR0). Enable interrupt, reset on match and stop
+	 * on match (MCR).
 	 */
 	writel_relaxed(0, base + LPC32XX_TIMER_TCR);
 	writel_relaxed(0, base + LPC32XX_TIMER_CTCR);
 	writel_relaxed(LPC32XX_TIMER_IR_MR0INT, base + LPC32XX_TIMER_IR);
-	writel_relaxed(1, base + LPC32XX_TIMER_MR0);
-	writel_relaxed(LPC32XX_TIMER_MCR_MR0I | LPC32XX_TIMER_MCR_MR0R |
-		       LPC32XX_TIMER_MCR_MR0S, base + LPC32XX_TIMER_MCR);
 
 	rate = clk_get_rate(clk);
+	ticks_per_jiffy = (rate + HZ/2) / HZ;
 	lpc32xx_clk_event_ddata.base = base;
 	clockevents_config_and_register(&lpc32xx_clk_event_ddata.evtdev,
 					rate, 1, -1);

-- 
2.7.0

This commit implements the ARM timer-based delay timer for the
LPC32xx, LPC18xx, LPC43xx family of SoCs.

Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
 drivers/clocksource/time-lpc32xx.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/clocksource/time-lpc32xx.c b/drivers/clocksource/time-lpc32xx.c
index 9b3d4a38c716..223bb08720d6 100644
--- a/drivers/clocksource/time-lpc32xx.c
+++ b/drivers/clocksource/time-lpc32xx.c

@@ -18,6 +18,7 @@
 #include <linux/clk.h>
 #include <linux/clockchips.h>
 #include <linux/clocksource.h>
+#include <linux/delay.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/kernel.h>

@@ -55,6 +56,15 @@ static u64 notrace lpc32xx_read_sched_clock(void)
 	return readl(clocksource_timer_counter);
 }
 
+static unsigned long lpc32xx_delay_timer_read(void)
+{
+	return readl(clocksource_timer_counter);
+}
+
+static struct delay_timer lpc32xx_delay_timer = {
+	.read_current_timer = lpc32xx_delay_timer_read,
+};
+
 static int lpc32xx_clkevt_next_event(unsigned long delta,
 				     struct clock_event_device *evtdev)
 {

@@ -198,6 +208,8 @@ static int __init lpc32xx_clocksource_init(struct device_node *np)
 	}
 
 	clocksource_timer_counter = base + LPC32XX_TIMER_TC;
+	lpc32xx_delay_timer.freq = rate;
+	register_current_timer_delay(&lpc32xx_delay_timer);
 	sched_clock_register(lpc32xx_read_sched_clock, 32, rate);
 
 	return 0;

-- 
2.7.0

Hi Ezequiel,

On 30 January 2016 at 07:46, Ezequiel Garcia
[off-list ref] wrote:

Could this use the relaxed version?

As far as I can see 'register_current_timer_delay' is an ARM only function(?).
Since time-lpc32xx.c can be built on other architectures than ARM
using COMPILE_TEST should all this be protected under CONFIG_ARM?


Could you also CC Vladimir Zapolskiy [off-list ref] on these patches
since he is working on the lpc32xx platform now?


regards,
Joachim Eastwood

Hi Ezequiel,

On 30 January 2016 at 07:46, Ezequiel Garcia
[off-list ref] wrote:

Could you avoid this global variable by sticking it to the
lpc32xx_clock_event_ddata struct?

Use a rounding macro?


I will look more closely at the new timer setup later.

btw, didn't I look through a version of this you sent me privately
quite some time ago?
I think I had a comment about using TIMER_PR instead of TIMER_MR0
then. Unsure if that is still valid, though.


regards,
Joachim Eastwood

On 30 January 2016 at 16:39, Joachim  Eastwood [off-list ref] wrote:

Hi Ezequiel,

On 30 January 2016 at 07:46, Ezequiel Garcia
[off-list ref] wrote:

quoted

This commit adds the support for periodic mode. This is done by not
setting the MR0S (Stop on TnMR0) bit on MCR, thus allowing
interrupts to be periodically generated on MR0 matches.

In order to do this, move the initial configuration that is specific to
the one shot mode to clock_event_device.set_state_oneshot.

Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
 drivers/clocksource/time-lpc32xx.c | 48 ++++++++++++++++++++++++++++++++------
 1 file changed, 41 insertions(+), 7 deletions(-)

diff --git a/drivers/clocksource/time-lpc32xx.c b/drivers/clocksource/time-lpc32xx.c
index 1316876b487a..9b3d4a38c716 100644
--- a/drivers/clocksource/time-lpc32xx.c
+++ b/drivers/clocksource/time-lpc32xx.c

@@ -47,6 +47,8 @@ struct lpc32xx_clock_event_ddata {

 /* Needed for the sched clock */
 static void __iomem *clocksource_timer_counter;
+/* Needed for clockevents periodic mode */
+static u32 ticks_per_jiffy;

Could you avoid this global variable by sticking it to the
lpc32xx_clock_event_ddata struct?

Sure.

quoted

 static u64 notrace lpc32xx_read_sched_clock(void)
 {

@@ -86,11 +88,42 @@ static int lpc32xx_clkevt_shutdown(struct clock_event_device *evtdev)

 static int lpc32xx_clkevt_oneshot(struct clock_event_device *evtdev)
 {
+       struct lpc32xx_clock_event_ddata *ddata =
+               container_of(evtdev, struct lpc32xx_clock_event_ddata, evtdev);
+
        /*
         * When using oneshot, we must also disable the timer
         * to wait for the first call to set_next_event().
         */
-       return lpc32xx_clkevt_shutdown(evtdev);
+       writel_relaxed(0, ddata->base + LPC32XX_TIMER_TCR);
+
+       /* Enable interrupt, reset on match and stop on match (MCR). */
+       writel_relaxed(LPC32XX_TIMER_MCR_MR0I | LPC32XX_TIMER_MCR_MR0R |
+                      LPC32XX_TIMER_MCR_MR0S, ddata->base + LPC32XX_TIMER_MCR);
+       /* Configure a compare match value of 1 on MR0. */
+       writel_relaxed(1, ddata->base + LPC32XX_TIMER_MR0);
+
+       return 0;
+}
+
+static int lpc32xx_clkevt_periodic(struct clock_event_device *evtdev)
+{
+       struct lpc32xx_clock_event_ddata *ddata =
+               container_of(evtdev, struct lpc32xx_clock_event_ddata, evtdev);
+
+       /* Enable interrupt and reset on match. */
+       writel_relaxed(LPC32XX_TIMER_MCR_MR0I | LPC32XX_TIMER_MCR_MR0R,
+                      ddata->base + LPC32XX_TIMER_MCR);
+       /*
+        * Place timer in reset and set a match value on MR0. An interrupt will
+        * be generated each time the counter matches MR0. After setup the
+        * timer is released from reset and enabled.
+        */
+       writel_relaxed(LPC32XX_TIMER_TCR_CRST, ddata->base + LPC32XX_TIMER_TCR);
+       writel_relaxed(ticks_per_jiffy, ddata->base + LPC32XX_TIMER_MR0);
+       writel_relaxed(LPC32XX_TIMER_TCR_CEN, ddata->base + LPC32XX_TIMER_TCR);
+
+       return 0;
 }

 static irqreturn_t lpc32xx_clock_event_handler(int irq, void *dev_id)

@@ -108,11 +141,14 @@ static irqreturn_t lpc32xx_clock_event_handler(int irq, void *dev_id)
 static struct lpc32xx_clock_event_ddata lpc32xx_clk_event_ddata = {
        .evtdev = {
                .name                   = "lpc3220 clockevent",
-               .features               = CLOCK_EVT_FEAT_ONESHOT,
+               .features               = CLOCK_EVT_FEAT_ONESHOT |
+                                         CLOCK_EVT_FEAT_PERIODIC,
                .rating                 = 300,
                .set_next_event         = lpc32xx_clkevt_next_event,
                .set_state_shutdown     = lpc32xx_clkevt_shutdown,
                .set_state_oneshot      = lpc32xx_clkevt_oneshot,
+               .set_state_periodic     = lpc32xx_clkevt_periodic,
+               .tick_resume            = lpc32xx_clkevt_shutdown,
        },
 };

@@ -210,17 +246,15 @@ static int __init lpc32xx_clockevent_init(struct device_node *np)

        /*
         * Disable timer and clear any pending interrupt (IR) on match
-        * channel 0 (MR0). Configure a compare match value of 1 on MR0
-        * and enable interrupt, reset on match and stop on match (MCR).
+        * channel 0 (MR0). Enable interrupt, reset on match and stop
+        * on match (MCR).
         */
        writel_relaxed(0, base + LPC32XX_TIMER_TCR);
        writel_relaxed(0, base + LPC32XX_TIMER_CTCR);
        writel_relaxed(LPC32XX_TIMER_IR_MR0INT, base + LPC32XX_TIMER_IR);
-       writel_relaxed(1, base + LPC32XX_TIMER_MR0);
-       writel_relaxed(LPC32XX_TIMER_MCR_MR0I | LPC32XX_TIMER_MCR_MR0R |
-                      LPC32XX_TIMER_MCR_MR0S, base + LPC32XX_TIMER_MCR);

        rate = clk_get_rate(clk);
+       ticks_per_jiffy = (rate + HZ/2) / HZ;

Use a rounding macro?

Done.

I will look more closely at the new timer setup later.

btw, didn't I look through a version of this you sent me privately
quite some time ago?
I think I had a comment about using TIMER_PR instead of TIMER_MR0
then. Unsure if that is still valid, though.

Ah, right. It was ages ago, and forgot your previous review. Sorry about that.
Using TIMER_PR looks less invasive so I'll send a v2 using it instead.
-- 
Ezequiel Garc?a, VanguardiaSur
www.vanguardiasur.com.ar

On 30 January 2016 at 16:27, Joachim  Eastwood [off-list ref] wrote:

Hi Ezequiel,

On 30 January 2016 at 07:46, Ezequiel Garcia
[off-list ref] wrote:

quoted

This commit implements the ARM timer-based delay timer for the
LPC32xx, LPC18xx, LPC43xx family of SoCs.

Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
 drivers/clocksource/time-lpc32xx.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/clocksource/time-lpc32xx.c b/drivers/clocksource/time-lpc32xx.c
index 9b3d4a38c716..223bb08720d6 100644
--- a/drivers/clocksource/time-lpc32xx.c
+++ b/drivers/clocksource/time-lpc32xx.c

@@ -18,6 +18,7 @@
 #include <linux/clk.h>
 #include <linux/clockchips.h>
 #include <linux/clocksource.h>
+#include <linux/delay.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/kernel.h>

@@ -55,6 +56,15 @@ static u64 notrace lpc32xx_read_sched_clock(void)
        return readl(clocksource_timer_counter);
 }

+static unsigned long lpc32xx_delay_timer_read(void)
+{
+       return readl(clocksource_timer_counter);
+}

Could this use the relaxed version?

It seems read_current_timer is used in a busy loop in __timer_delay,
and I'd that's why other drivers use readl() here.

In any case, isn't __iormb a no-op on CPU_V7M ?

quoted

+
+static struct delay_timer lpc32xx_delay_timer = {
+       .read_current_timer = lpc32xx_delay_timer_read,
+};
+
 static int lpc32xx_clkevt_next_event(unsigned long delta,
                                     struct clock_event_device *evtdev)
 {

@@ -198,6 +208,8 @@ static int __init lpc32xx_clocksource_init(struct device_node *np)
        }

        clocksource_timer_counter = base + LPC32XX_TIMER_TC;
+       lpc32xx_delay_timer.freq = rate;
+       register_current_timer_delay(&lpc32xx_delay_timer);

As far as I can see 'register_current_timer_delay' is an ARM only function(?).
Since time-lpc32xx.c can be built on other architectures than ARM
using COMPILE_TEST should all this be protected under CONFIG_ARM?

Yes, you are right.

Could you also CC Vladimir Zapolskiy [off-list ref] on these patches
since he is working on the lpc32xx platform now?

Sure.
-- 
Ezequiel Garc?a, VanguardiaSur
www.vanguardiasur.com.ar

On 31 January 2016 at 21:15, Ezequiel Garcia
[off-list ref] wrote:

On 30 January 2016 at 16:27, Joachim  Eastwood [off-list ref] wrote:

quoted

Hi Ezequiel,

On 30 January 2016 at 07:46, Ezequiel Garcia
[off-list ref] wrote:

quoted

This commit implements the ARM timer-based delay timer for the
LPC32xx, LPC18xx, LPC43xx family of SoCs.

Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
 drivers/clocksource/time-lpc32xx.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/clocksource/time-lpc32xx.c b/drivers/clocksource/time-lpc32xx.c
index 9b3d4a38c716..223bb08720d6 100644
--- a/drivers/clocksource/time-lpc32xx.c
+++ b/drivers/clocksource/time-lpc32xx.c

@@ -18,6 +18,7 @@
 #include <linux/clk.h>
 #include <linux/clockchips.h>
 #include <linux/clocksource.h>
+#include <linux/delay.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/kernel.h>

@@ -55,6 +56,15 @@ static u64 notrace lpc32xx_read_sched_clock(void)
        return readl(clocksource_timer_counter);
 }

+static unsigned long lpc32xx_delay_timer_read(void)
+{
+       return readl(clocksource_timer_counter);
+}

Could this use the relaxed version?

It seems read_current_timer is used in a busy loop in __timer_delay,
and I'd that's why other drivers use readl() here.

As far as I can see only the armada370 and u300 clocksource drivers
use readl() in the read_current_timer callback.

In any case, isn't __iormb a no-op on CPU_V7M ?

For LPC18xx and LPC43xx, which is ARMV7m, I don't think it matters.
But we should also consider LPC32xx, which is ARM9, as it will soon
start to use this driver.

Anyway I am fine with using readl() for now. It can always be changed later.


regards,
Joachim  Eastwood

On 31 January 2016 at 17:07, Ezequiel Garcia
[off-list ref] wrote:

On 30 January 2016 at 16:39, Joachim  Eastwood [off-list ref] wrote:

quoted

Hi Ezequiel,

On 30 January 2016 at 07:46, Ezequiel Garcia
[off-list ref] wrote:

quoted

This commit adds the support for periodic mode. This is done by not
setting the MR0S (Stop on TnMR0) bit on MCR, thus allowing
interrupts to be periodically generated on MR0 matches.

In order to do this, move the initial configuration that is specific to
the one shot mode to clock_event_device.set_state_oneshot.

[...]

quoted

I will look more closely at the new timer setup later.

btw, didn't I look through a version of this you sent me privately
quite some time ago?
I think I had a comment about using TIMER_PR instead of TIMER_MR0
then. Unsure if that is still valid, though.

Ah, right. It was ages ago, and forgot your previous review. Sorry about that.
Using TIMER_PR looks less invasive so I'll send a v2 using it instead.

Actually, using the prescale counter and MR0 = 1 resulted in exactly
half the number of interrupts that I expected (HZ / 2). Using this
patch, with PR=0 and MR0=ticks_per_jiggies, I can see HZ no. of
interrupts per sec.

Why did you choose to use the prescale in your oneshot implementation?
Did you test or measure the timer expire using the PR and MR0 = 1?

To be honest, I'm still trying to make some sense out of my results.
-- 
Ezequiel Garc?a, VanguardiaSur
www.vanguardiasur.com.ar

On 1 February 2016 at 16:09, Ezequiel Garcia
[off-list ref] wrote:

On 31 January 2016 at 17:07, Ezequiel Garcia
[off-list ref] wrote:

quoted

On 30 January 2016 at 16:39, Joachim  Eastwood [off-list ref] wrote:

quoted

Hi Ezequiel,

On 30 January 2016 at 07:46, Ezequiel Garcia
[off-list ref] wrote:

quoted

This commit adds the support for periodic mode. This is done by not
setting the MR0S (Stop on TnMR0) bit on MCR, thus allowing
interrupts to be periodically generated on MR0 matches.

In order to do this, move the initial configuration that is specific to
the one shot mode to clock_event_device.set_state_oneshot.

[...]

quoted

quoted

I will look more closely at the new timer setup later.

btw, didn't I look through a version of this you sent me privately
quite some time ago?
I think I had a comment about using TIMER_PR instead of TIMER_MR0
then. Unsure if that is still valid, though.

Ah, right. It was ages ago, and forgot your previous review. Sorry about that.
Using TIMER_PR looks less invasive so I'll send a v2 using it instead.

Actually, using the prescale counter and MR0 = 1 resulted in exactly
half the number of interrupts that I expected (HZ / 2). Using this
patch, with PR=0 and MR0=ticks_per_jiggies, I can see HZ no. of
interrupts per sec.

Why did you choose to use the prescale in your oneshot implementation?

The clockevent code was copied from arch/arm/mach-lpc32xx/timer.c so
it's really old code.
I think it is a strange way to use the timer, but the logic still
seems good to me.

Did you test or measure the timer expire using the PR and MR0 = 1?

I really can't remember. I do remember going through the timer setup
and it seemed like a valid, albeit slightly strange, way to use the
timer.

To be honest, I'm still trying to make some sense out of my results.

I'll see if can find the time to do some more testing over here as well.


regards,
Joachim Eastwood

Hi Joachim,

(Ccing Vladimir)

On 1 February 2016 at 18:55, Joachim  Eastwood [off-list ref] wrote:

On 1 February 2016 at 16:09, Ezequiel Garcia
[off-list ref] wrote:

quoted

On 31 January 2016 at 17:07, Ezequiel Garcia
[off-list ref] wrote:

quoted

On 30 January 2016 at 16:39, Joachim  Eastwood [off-list ref] wrote:

quoted

Hi Ezequiel,

On 30 January 2016 at 07:46, Ezequiel Garcia
[off-list ref] wrote:

quoted

This commit adds the support for periodic mode. This is done by not
setting the MR0S (Stop on TnMR0) bit on MCR, thus allowing
interrupts to be periodically generated on MR0 matches.

In order to do this, move the initial configuration that is specific to
the one shot mode to clock_event_device.set_state_oneshot.

[...]

quoted

quoted

I will look more closely at the new timer setup later.

btw, didn't I look through a version of this you sent me privately
quite some time ago?
I think I had a comment about using TIMER_PR instead of TIMER_MR0
then. Unsure if that is still valid, though.

Ah, right. It was ages ago, and forgot your previous review. Sorry about that.
Using TIMER_PR looks less invasive so I'll send a v2 using it instead.

Actually, using the prescale counter and MR0 = 1 resulted in exactly
half the number of interrupts that I expected (HZ / 2). Using this
patch, with PR=0 and MR0=ticks_per_jiggies, I can see HZ no. of
interrupts per sec.

Why did you choose to use the prescale in your oneshot implementation?

The clockevent code was copied from arch/arm/mach-lpc32xx/timer.c so
it's really old code.
I think it is a strange way to use the timer, but the logic still
seems good to me.

quoted

Did you test or measure the timer expire using the PR and MR0 = 1?

I really can't remember. I do remember going through the timer setup
and it seemed like a valid, albeit slightly strange, way to use the
timer.

quoted

To be honest, I'm still trying to make some sense out of my results.

I'll see if can find the time to do some more testing over here as well.

I wrote a small test driver to toggle a GPIO on a timer event.
One-shot usage of the timers results in the same measurements
with either implementation.

In other words, when triggering a one-shot timer (MCR_MR0S is set),
I get the same results using the prescale counter (PR=ticks, MR0=1)
or the timer counter (PR=0, MR0=ticks).

Here's the branch in case you want to play with it:
http://git.infradead.org/users/ezequielg/linux/shortlog/refs/heads/misc_timer_test

My logic analyzer is quite crappy, but it was clear that one-shot timers
expired correctly at the configured time, with either implementation.

However, when configuring the timer in periodic mode (MCR_MR0S
is cleared), the result is always ~twice the programmed value
(as if it takes two TC cycles to complete one event).

Hence, it seems the prescale counter is not suitable for periodic mode.

How about we change the current oneshot implementation
to not use the prescale counter at all, and so when the periodic
mode is introduced it'll be consistent with oneshot?

(Tested on LPC4350 Hitex Devkit, no idea how this works on LPC32xx)
-- 
Ezequiel Garc?a, VanguardiaSur
www.vanguardiasur.com.ar

Hi Ezequiel,

On 3 February 2016 at 17:03, Ezequiel Garcia
[off-list ref] wrote:

Hi Joachim,

(Ccing Vladimir)

On 1 February 2016 at 18:55, Joachim  Eastwood [off-list ref] wrote:

quoted

On 1 February 2016 at 16:09, Ezequiel Garcia
[off-list ref] wrote:

quoted

On 31 January 2016 at 17:07, Ezequiel Garcia
[off-list ref] wrote:

quoted

On 30 January 2016 at 16:39, Joachim  Eastwood [off-list ref] wrote:

quoted

Hi Ezequiel,

On 30 January 2016 at 07:46, Ezequiel Garcia
[off-list ref] wrote:

quoted

This commit adds the support for periodic mode. This is done by not
setting the MR0S (Stop on TnMR0) bit on MCR, thus allowing
interrupts to be periodically generated on MR0 matches.

In order to do this, move the initial configuration that is specific to
the one shot mode to clock_event_device.set_state_oneshot.

[...]

quoted

quoted

I will look more closely at the new timer setup later.

btw, didn't I look through a version of this you sent me privately
quite some time ago?
I think I had a comment about using TIMER_PR instead of TIMER_MR0
then. Unsure if that is still valid, though.

Ah, right. It was ages ago, and forgot your previous review. Sorry about that.
Using TIMER_PR looks less invasive so I'll send a v2 using it instead.

Actually, using the prescale counter and MR0 = 1 resulted in exactly
half the number of interrupts that I expected (HZ / 2). Using this
patch, with PR=0 and MR0=ticks_per_jiggies, I can see HZ no. of
interrupts per sec.

Why did you choose to use the prescale in your oneshot implementation?

The clockevent code was copied from arch/arm/mach-lpc32xx/timer.c so
it's really old code.
I think it is a strange way to use the timer, but the logic still
seems good to me.

quoted

Did you test or measure the timer expire using the PR and MR0 = 1?

I really can't remember. I do remember going through the timer setup
and it seemed like a valid, albeit slightly strange, way to use the
timer.

quoted

To be honest, I'm still trying to make some sense out of my results.

I'll see if can find the time to do some more testing over here as well.

Sorry for the late reply.

I wrote a small test driver to toggle a GPIO on a timer event.
One-shot usage of the timers results in the same measurements
with either implementation.

In other words, when triggering a one-shot timer (MCR_MR0S is set),
I get the same results using the prescale counter (PR=ticks, MR0=1)
or the timer counter (PR=0, MR0=ticks).

Here's the branch in case you want to play with it:
http://git.infradead.org/users/ezequielg/linux/shortlog/refs/heads/misc_timer_test

My logic analyzer is quite crappy, but it was clear that one-shot timers
expired correctly at the configured time, with either implementation.

Ok. That is a good thing.

However, when configuring the timer in periodic mode (MCR_MR0S
is cleared), the result is always ~twice the programmed value
(as if it takes two TC cycles to complete one event).

Hence, it seems the prescale counter is not suitable for periodic mode.

I see.

How about we change the current oneshot implementation
to not use the prescale counter at all, and so when the periodic
mode is introduced it'll be consistent with oneshot?

Makes sense to me. I think using the match registers will make the
timer setup easier to follow too.


Hopefully Vladimir can give it a test on LPC32xx also. But afaik the
timer IP should be exactly the same.


regards,
Joachim Eastwood