diff mbox

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

Message ID 1454136379-7517-1-git-send-email-ezequiel@vanguardiasur.com.ar (mailing list archive)
State New, archived
Headers show

Commit Message

Ezequiel Garcia Jan. 30, 2016, 6:46 a.m. UTC
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(-)

Comments

Joachim Eastwood Jan. 30, 2016, 7:39 p.m. UTC | #1
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
Ezequiel Garcia Jan. 31, 2016, 8:07 p.m. UTC | #2
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 Garcia Feb. 1, 2016, 3:09 p.m. UTC | #3
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.
Joachim Eastwood Feb. 1, 2016, 9:55 p.m. UTC | #4
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
Ezequiel Garcia Feb. 3, 2016, 4:03 p.m. UTC | #5
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)
Joachim Eastwood Feb. 9, 2016, 10:02 a.m. UTC | #6
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
diff mbox

Patch

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