[PATCH] OMAP2: gptimer min_delta_ns should be rounded up

[PATCH] OMAP2: gptimer min_delta_ns should be rounded up

[Aaro Koskinen]

clockevent_delta2ns() used to initialize min_delta_ns rounds the value
down, so the result can be too small. When clockevents_program_event()
is using too small value it will try to program the timer with zero
ticks stalling the timer queue.

Signed-off-by: Aaro Koskinen <Aaro.Koskinen@nokia.com>
---
 arch/arm/mach-omap2/timer-gp.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/timer-gp.c b/arch/arm/mach-omap2/timer-gp.c
index 787cfef..375c18e 100644
--- a/arch/arm/mach-omap2/timer-gp.c
+++ b/arch/arm/mach-omap2/timer-gp.c
@@ -121,7 +121,7 @@ static void __init omap2_gp_clockevent_init(void)
 	clockevent_gpt.max_delta_ns =
 		clockevent_delta2ns(0xffffffff, &clockevent_gpt);
 	clockevent_gpt.min_delta_ns =
-		clockevent_delta2ns(1, &clockevent_gpt);
+		clockevent_delta2ns(1, &clockevent_gpt) + 1;
 
 	clockevent_gpt.cpumask = cpumask_of_cpu(0);
 	clockevents_register_device(&clockevent_gpt);
-- 
1.5.4.3

RE: [PATCH] OMAP2: gptimer min_delta_ns should be rounded up

[Woodruff, Richard]

Hi,

> clockevent_delta2ns() used to initialize min_delta_ns rounds the value
> down, so the result can be too small. When clockevents_program_event()
> is using too small value it will try to program the timer with zero
> ticks stalling the timer queue.

> +             clockevent_delta2ns(1, &clockevent_gpt) + 1;

That value is still not correct.  The base perhaps should be '3'.  The additional +1 is needed if you use a base value of 1 which can cause the error your patch addresses.

        clockevent_delta2ns(3, &clockevent_gpt);

min_delta_ns stops the frame work from trying to program an expiry time which may not be achievable due to timer costs.

A write to the timer when it has a 32KHz F-Clock can take ~3 32Khz clock cycles to complete.  Hence the cost of '3' not '1'.

If your lucky the write will be posted and not stall the CPU.  But just because the IP block has captured the change doesn't mean it will take effect immediately.  The write will still have to resynchronization internally with f-clk.

Regards,
Richard W.

Re: [PATCH] OMAP2: gptimer min_delta_ns should be rounded up

[Aaro Koskinen]

Hello,

Woodruff, Richard wrote:
>> +             clockevent_delta2ns(1, &clockevent_gpt) + 1;
> 
> That value is still not correct.

[...]

>         clockevent_delta2ns(3, &clockevent_gpt);
> 
> min_delta_ns stops the frame work from trying to program an expiry time which may not be achievable due to timer costs.
> 
> A write to the timer when it has a 32KHz F-Clock can take ~3 32Khz clock cycles to complete.  Hence the cost of '3' not '1'.

I'm not sure if I see any problem. If the timer programming cost is 
bigger than min_delta_ns, the framework should adapt to it.

I guess you would want to make min_delta_ns bigger to avoid excessive 
scheduling/wakeups, but I think there is no any single "correct" value 
for every system/board.

A.

RE: [PATCH] OMAP2: gptimer min_delta_ns should be rounded up

[Woodruff, Richard]

Hi,

> >         clockevent_delta2ns(3, &clockevent_gpt);
> >
> > min_delta_ns stops the frame work from trying to program an expiry time
> which may not be achievable due to timer costs.
> >
> > A write to the timer when it has a 32KHz F-Clock can take ~3 32Khz clock
> cycles to complete.  Hence the cost of '3' not '1'.
>
> I'm not sure if I see any problem. If the timer programming cost is
> bigger than min_delta_ns, the framework should adapt to it.

By setting this value to the cost of the timer operation you allow the frame work to adapt.

If 5 users call into this api and try and set wake ups at an rate the hardware can't support, you end up wasting time writing wake ups which can't be met.

Worse to stall the processor.  If you look at the ETM trace output of programming the timer you will see the whole call path to writing to the timer takes some few uS.  By you can be stuck at the writing of the timer registers up to 90uS (if f-clk is set to 32khz).

By allowing frequent accesses you:
        - don't meet the deadline anyway
                - program a wake for 20uS it won't signal till 90uS anyway.
                - you can at least query and round to get what the hardware supports if the interface is set properly.
        - you stall the processor and waste cycles.

> I guess you would want to make min_delta_ns bigger to avoid excessive
> scheduling/wakeups, but I think there is no any single "correct" value
> for every system/board.

The correct value for the chip timer IP is 3 if the functional clock is at 32MHz.  If you use a faster time base like 12Mhz use 1.   Next, If you want to try and factor in the software cost you can try.  This software cost (what DPLL rate am I at, how much code is executed on path, ...) will be very board and port specific.

Regards,
Richard W.

Re: [PATCH] OMAP2: gptimer min_delta_ns should be rounded up

[Aaro Koskinen]

Hello,

Woodruff, Richard wrote:
>>>         clockevent_delta2ns(3, &clockevent_gpt);
>>>
>>> min_delta_ns stops the frame work from trying to program an expiry time
>> which may not be achievable due to timer costs.
>>> A write to the timer when it has a 32KHz F-Clock can take ~3 32Khz clock
>> cycles to complete.  Hence the cost of '3' not '1'.
>>
>> I'm not sure if I see any problem. If the timer programming cost is
>> bigger than min_delta_ns, the framework should adapt to it.
> 
> By setting this value to the cost of the timer operation you allow the frame work to adapt.
> 
> If 5 users call into this api and try and set wake ups at an rate the hardware can't support,  you end up wasting time writing wake ups which can't be met.

Yes, I actually made a test case with hrtimers too see the difference 
(though I think I'm also seeing hrtimer/oneshot code still doing some 
unnecessary programming...)

Thanks for you patience, I will try to send a new patch.

A.

RE: [PATCH] OMAP2: gptimer min_delta_ns should be rounded up

[Woodruff, Richard]

Hi,

> > If 5 users call into this api and try and set wake ups at an rate the
> hardware can't support,  you end up wasting time writing wake ups which can't
> be met.
>
> Yes, I actually made a test case with hrtimers too see the difference
> (though I think I'm also seeing hrtimer/oneshot code still doing some
> unnecessary programming...)

Cool. A quantitative test.

Actually, the functional spec for timer says max re-sync time is:
        3 OCP clock + 2.5 TIMER clock
        3(1/83MHz) + 76uS = ~76uS

When I've measured the stall portion I usually don't see more then ~65uS stall.  There is still a bit of time once the block has taken in the command before its issued.

* There is posting at the timer-ip-block which means the first write doesn't stall the MPU but it will take time to see the external effect (ie your interrupt/signal while internal resync happens).  The 2nd write (to same register) starts to be really costly as the MPU stalls till the 1st one completes.  This write is caused by the frame work trying to reprogram for timer events it can't meet. So if you don't set the delta_ns correctly it allows programming at a rate which the hardware can't support and you end up stalling mpu and missing deadline which wasn't realistic to start off with.

** Your original patch WAS good.  It fixed a real bug.  My comment was mainly that the patch could go one further, fix the bug, and setup the time interface so it didn't suck mpu time and break user expectations about timer delivery.

When the code is set to MHz time base a (1,)+1 is the right value.  When it uses 32KHz the right value is (2.5,).  I don't think interface can take 2.5 so I rounded to 3.  An optimization would be to use 2.5's effective value.

Thanks for taking the time to understand.

Regards,
Richard W.

RE: [PATCH] OMAP2: gptimer min_delta_ns should be rounded up

[Paul Walmsley]

Hi,

a few quick comments, possibly nit-picking...

On Wed, 31 Dec 2008, Woodruff, Richard wrote:

> Actually, the functional spec for timer says max re-sync time is:
>         3 OCP clock + 2.5 TIMER clock
>         3(1/83MHz) + 76uS = ~76uS

The OCP clock rate is dependent on the timer's interface clock 
clockdomain.  On OMAP3, GPTIMER1 uses sys_clk as the OCP interface clock, 
so the interface portion of this delay could be as slow as 3 periods/12MHz 
periods = 250 ns.

For GPTIMERs in CORE_L4 or PER_L4, the choice of CORE OPP will also change 
the OCP delay portion.  The L4 bus clock rate could be as low as 20.75MHz 
in 34xx VDD2 OPP1; resulting in a 145 ns delay for that portion.

These factors are noise with the 32KiHz timer source, but we are overusing 
it.  In the case of Beagle, the HF clk source can't be disabled, so we 
should be able to use sys_ck.  Same for other boards when AUTOEXTCLKMODE 
is disabled.  In those situations we should probably take the OCP-related 
delay into account.


- Paul

RE: [PATCH] OMAP2: gptimer min_delta_ns should be rounded up

[Woodruff, Richard]

Hi,

> a few quick comments, possibly nit-picking...
>
> On Wed, 31 Dec 2008, Woodruff, Richard wrote:
>
> > Actually, the functional spec for timer says max re-sync time is:
> >         3 OCP clock + 2.5 TIMER clock
> >         3(1/83MHz) + 76uS = ~76uS
>
> The OCP clock rate is dependent on the timer's interface clock
> clockdomain.  On OMAP3, GPTIMER1 uses sys_clk as the OCP interface clock,
> so the interface portion of this delay could be as slow as 3 periods/12MHz
> periods = 250 ns.
>
> For GPTIMERs in CORE_L4 or PER_L4, the choice of CORE OPP will also change
> the OCP delay portion.  The L4 bus clock rate could be as low as 20.75MHz
> in 34xx VDD2 OPP1; resulting in a 145 ns delay for that portion.
>
> These factors are noise with the 32KiHz timer source, but we are overusing
> it.  In the case of Beagle, the HF clk source can't be disabled, so we
> should be able to use sys_ck.  Same for other boards when AUTOEXTCLKMODE
> is disabled.  In those situations we should probably take the OCP-related
> delay into account.

Yes, you are correct on L4 clock. I wrote a bit quickly.  It is noise compared to the 32K.

Your idea about AUOEXTCLKMODE might work. I'd be worried about some hidden CM (from prcm) dependency which might stop RET/OFF.  Several other clock sources which route through CM can keep it up and CORE from going down.  Probably worth a quick test.

Regards,
Richard W.