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