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

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

[Ezequiel Garcia]

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

[PATCH 2/2] clocksource/drivers/lpc32xx: Support timer-based ARM delay

[Ezequiel Garcia]

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

[PATCH 2/2] clocksource/drivers/lpc32xx: Support timer-based ARM delay

[Joachim Eastwood]

Hi Ezequiel,

On 30 January 2016 at 07:46, Ezequiel Garcia
<ezequiel@vanguardiasur.com.ar> wrote:
> 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?

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


Could you also CC Vladimir Zapolskiy <vz@mleia.com> on these patches
since he is working on the lpc32xx platform now?


regards,
Joachim Eastwood

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

[Joachim Eastwood]

Hi Ezequiel,

On 30 January 2016 at 07:46, Ezequiel Garcia
<ezequiel@vanguardiasur.com.ar> wrote:
> 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?


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


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

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

[Ezequiel Garcia]

On 30 January 2016 at 16:39, Joachim  Eastwood <manabian@gmail.com> wrote:
> Hi Ezequiel,
>
> On 30 January 2016 at 07:46, Ezequiel Garcia
> <ezequiel@vanguardiasur.com.ar> wrote:
>> 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.

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

[PATCH 2/2] clocksource/drivers/lpc32xx: Support timer-based ARM delay

[Ezequiel Garcia]

On 30 January 2016 at 16:27, Joachim  Eastwood <manabian@gmail.com> wrote:
> Hi Ezequiel,
>
> On 30 January 2016 at 07:46, Ezequiel Garcia
> <ezequiel@vanguardiasur.com.ar> wrote:
>> 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 ?

>> +
>> +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 <vz@mleia.com> on these patches
> since he is working on the lpc32xx platform now?
>

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

[PATCH 2/2] clocksource/drivers/lpc32xx: Support timer-based ARM delay

[Joachim Eastwood]

On 31 January 2016 at 21:15, Ezequiel Garcia
<ezequiel@vanguardiasur.com.ar> wrote:
> On 30 January 2016 at 16:27, Joachim  Eastwood <manabian@gmail.com> wrote:
>> Hi Ezequiel,
>>
>> On 30 January 2016 at 07:46, Ezequiel Garcia
>> <ezequiel@vanguardiasur.com.ar> wrote:
>>> 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

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

[Ezequiel Garcia]

On 31 January 2016 at 17:07, Ezequiel Garcia
<ezequiel@vanguardiasur.com.ar> wrote:
> On 30 January 2016 at 16:39, Joachim  Eastwood <manabian@gmail.com> wrote:
>> Hi Ezequiel,
>>
>> On 30 January 2016 at 07:46, Ezequiel Garcia
>> <ezequiel@vanguardiasur.com.ar> wrote:
>>> 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.
>>>
[...]
>
>>
>> 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

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

[Joachim Eastwood]

On 1 February 2016 at 16:09, Ezequiel Garcia
<ezequiel@vanguardiasur.com.ar> wrote:
> On 31 January 2016 at 17:07, Ezequiel Garcia
> <ezequiel@vanguardiasur.com.ar> wrote:
>> On 30 January 2016 at 16:39, Joachim  Eastwood <manabian@gmail.com> wrote:
>>> Hi Ezequiel,
>>>
>>> On 30 January 2016 at 07:46, Ezequiel Garcia
>>> <ezequiel@vanguardiasur.com.ar> wrote:
>>>> 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.
>>>>
> [...]
>>
>>>
>>> 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

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

[Ezequiel Garcia]

Hi Joachim,

(Ccing Vladimir)

On 1 February 2016 at 18:55, Joachim  Eastwood <manabian@gmail.com> wrote:
> On 1 February 2016 at 16:09, Ezequiel Garcia
> <ezequiel@vanguardiasur.com.ar> wrote:
>> On 31 January 2016 at 17:07, Ezequiel Garcia
>> <ezequiel@vanguardiasur.com.ar> wrote:
>>> On 30 January 2016 at 16:39, Joachim  Eastwood <manabian@gmail.com> wrote:
>>>> Hi Ezequiel,
>>>>
>>>> On 30 January 2016 at 07:46, Ezequiel Garcia
>>>> <ezequiel@vanguardiasur.com.ar> wrote:
>>>>> 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.
>>>>>
>> [...]
>>>
>>>>
>>>> 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.
>

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

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

[Joachim Eastwood]

Hi Ezequiel,

On 3 February 2016 at 17:03, Ezequiel Garcia
<ezequiel@vanguardiasur.com.ar> wrote:
> Hi Joachim,
>
> (Ccing Vladimir)
>
> On 1 February 2016 at 18:55, Joachim  Eastwood <manabian@gmail.com> wrote:
>> On 1 February 2016 at 16:09, Ezequiel Garcia
>> <ezequiel@vanguardiasur.com.ar> wrote:
>>> On 31 January 2016 at 17:07, Ezequiel Garcia
>>> <ezequiel@vanguardiasur.com.ar> wrote:
>>>> On 30 January 2016 at 16:39, Joachim  Eastwood <manabian@gmail.com> wrote:
>>>>> Hi Ezequiel,
>>>>>
>>>>> On 30 January 2016 at 07:46, Ezequiel Garcia
>>>>> <ezequiel@vanguardiasur.com.ar> wrote:
>>>>>> 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.
>>>>>>
>>> [...]
>>>>
>>>>>
>>>>> 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.
>>

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