[PATCH] clocksource/drivers/fttmr010: fix set_next_event handler

[PATCH] clocksource/drivers/fttmr010: fix set_next_event handler

[Tao Ren]

Currently, the aspeed MATCH1 register is updated to <current_count -
cycles> in set_next_event handler, with the assumption that COUNT
register value is preserved when the timer is disabled and it continues
decrementing after the timer is enabled. But the assumption is wrong:
RELOAD register is loaded into COUNT register when the aspeed timer is
enabled, which means the next event may be delayed because timer
interrupt won't be generated until <0xFFFFFFFF - current_count +
cycles>.

The problem can be fixed by updating RELOAD register to <cycles>, and
COUNT register will be re-loaded when the timer is enabled and interrupt
is generated when COUNT register overflows.

The test result on Facebook Backpack-CMM BMC hardware (AST2500) shows
the issue is fixed: without the patch, usleep(100) suspends the process
for several milliseconds (and sometimes even over 40 milliseconds);
after applying the fix, usleep(100) takes averagely 240 microseconds to
return under the same workload level.

Signed-off-by: Tao Ren <taoren@fb.com>
---
 drivers/clocksource/timer-fttmr010.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/clocksource/timer-fttmr010.c b/drivers/clocksource/timer-fttmr010.c
index c020038ebfab..cf93f6419b51 100644
--- a/drivers/clocksource/timer-fttmr010.c
+++ b/drivers/clocksource/timer-fttmr010.c
@@ -130,13 +130,17 @@ static int fttmr010_timer_set_next_event(unsigned long cycles,
 	cr &= ~fttmr010->t1_enable_val;
 	writel(cr, fttmr010->base + TIMER_CR);
 
-	/* Setup the match register forward/backward in time */
-	cr = readl(fttmr010->base + TIMER1_COUNT);
-	if (fttmr010->count_down)
-		cr -= cycles;
-	else
-		cr += cycles;
-	writel(cr, fttmr010->base + TIMER1_MATCH1);
+	if (fttmr010->count_down) {
+		/*
+		 * ASPEED Timer Controller will load TIMER1_LOAD register
+		 * into TIMER1_COUNT register when the timer is re-enabled.
+		 */
+		writel(cycles, fttmr010->base + TIMER1_LOAD);
+	} else {
+		/* Setup the match register forward in time */
+		cr = readl(fttmr010->base + TIMER1_COUNT);
+		writel(cr + cycles, fttmr010->base + TIMER1_MATCH1);
+	}
 
 	/* Start */
 	cr = readl(fttmr010->base + TIMER_CR);
-- 
2.17.1

Re: [PATCH] clocksource/drivers/fttmr010: fix set_next_event handler

[Daniel Lezcano]

On 20/09/2018 00:13, Tao Ren wrote:
> Currently, the aspeed MATCH1 register is updated to <current_count -
> cycles> in set_next_event handler, with the assumption that COUNT
> register value is preserved when the timer is disabled and it continues
> decrementing after the timer is enabled. But the assumption is wrong:
> RELOAD register is loaded into COUNT register when the aspeed timer is
> enabled, which means the next event may be delayed because timer
> interrupt won't be generated until <0xFFFFFFFF - current_count +
> cycles>.
> 
> The problem can be fixed by updating RELOAD register to <cycles>, and
> COUNT register will be re-loaded when the timer is enabled and interrupt
> is generated when COUNT register overflows.
> 
> The test result on Facebook Backpack-CMM BMC hardware (AST2500) shows
> the issue is fixed: without the patch, usleep(100) suspends the process
> for several milliseconds (and sometimes even over 40 milliseconds);
> after applying the fix, usleep(100) takes averagely 240 microseconds to
> return under the same workload level.
> 
> Signed-off-by: Tao Ren <taoren@fb.com>

Linus, any comments ?

> ---
>  drivers/clocksource/timer-fttmr010.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/clocksource/timer-fttmr010.c b/drivers/clocksource/timer-fttmr010.c
> index c020038ebfab..cf93f6419b51 100644
> --- a/drivers/clocksource/timer-fttmr010.c
> +++ b/drivers/clocksource/timer-fttmr010.c
> @@ -130,13 +130,17 @@ static int fttmr010_timer_set_next_event(unsigned long cycles,
>  	cr &= ~fttmr010->t1_enable_val;
>  	writel(cr, fttmr010->base + TIMER_CR);
>  
> -	/* Setup the match register forward/backward in time */
> -	cr = readl(fttmr010->base + TIMER1_COUNT);
> -	if (fttmr010->count_down)
> -		cr -= cycles;
> -	else
> -		cr += cycles;
> -	writel(cr, fttmr010->base + TIMER1_MATCH1);
> +	if (fttmr010->count_down) {
> +		/*
> +		 * ASPEED Timer Controller will load TIMER1_LOAD register
> +		 * into TIMER1_COUNT register when the timer is re-enabled.
> +		 */
> +		writel(cycles, fttmr010->base + TIMER1_LOAD);
> +	} else {
> +		/* Setup the match register forward in time */
> +		cr = readl(fttmr010->base + TIMER1_COUNT);
> +		writel(cr + cycles, fttmr010->base + TIMER1_MATCH1);
> +	}
>  
>  	/* Start */
>  	cr = readl(fttmr010->base + TIMER_CR);
> 


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH] clocksource/drivers/fttmr010: fix set_next_event handler

[Linus Walleij]

On Wed, Sep 19, 2018 at 3:13 PM Tao Ren <taoren@fb.com> wrote:

> Currently, the aspeed MATCH1 register is updated to <current_count -
> cycles> in set_next_event handler, with the assumption that COUNT
> register value is preserved when the timer is disabled and it continues
> decrementing after the timer is enabled. But the assumption is wrong:
> RELOAD register is loaded into COUNT register when the aspeed timer is
> enabled, which means the next event may be delayed because timer
> interrupt won't be generated until <0xFFFFFFFF - current_count +
> cycles>.
>
> The problem can be fixed by updating RELOAD register to <cycles>, and
> COUNT register will be re-loaded when the timer is enabled and interrupt
> is generated when COUNT register overflows.
>
> The test result on Facebook Backpack-CMM BMC hardware (AST2500) shows
> the issue is fixed: without the patch, usleep(100) suspends the process
> for several milliseconds (and sometimes even over 40 milliseconds);
> after applying the fix, usleep(100) takes averagely 240 microseconds to
> return under the same workload level.
>
> Signed-off-by: Tao Ren <taoren@fb.com>

Actually this is much more intuitive too, it is the typical way to handle
a down-counting timer. Good catch!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Sorry for any cargo-cult programming on my part :/

Would be nice to get a nod from the AST2400 users that this works
for them too, so included them in the To: field.

I can't test it on up-counting hardware right now but I convinced
myself it is handled just like before so it shouldn't be an issue.

It actually would make kind of sense to restart the up-counting
timer from zero and set match to whatever value is passed in
as well, so I might send a patch for this. It's no regression though
so no hurry with that.

Yours,
Linus Walleij

Re: [PATCH] clocksource/drivers/fttmr010: fix set_next_event handler

[Tao Ren]

On 9/20/18, 8:46 AM, "Linus Walleij" <linus.walleij@linaro.org> wrote:
 
> Actually this is much more intuitive too, it is the typical way to handle
> a down-counting timer. Good catch!
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Thank you Linus for the quick review!
    
> Sorry for any cargo-cult programming on my part :/
> Would be nice to get a nod from the AST2400 users that this works
> for them too, so included them in the To: field.

Make sense. I actually booted up kernel on qemu-palmetto (ast2400) but I'm doubting if test is valid because it depends on how qemu emulates the hardware. It would be great if someone can help to verify the patch on physical ast2400.

> It actually would make kind of sense to restart the up-counting
> timer from zero and set match to whatever value is passed in
> as well, so I might send a patch for this. It's no regression though
> so no hurry with that.

I agree. Besides being more intuitive, timer overflow interrupt (when <cr + cycles> overflows) could also be avoided by resetting the counter.

Thanks,
Tao Ren 

Re: [PATCH] clocksource/drivers/fttmr010: fix set_next_event handler

[Joel Stanley]

On Fri, 21 Sep 2018 at 06:39, Tao Ren <taoren@fb.com> wrote:
>
> On 9/20/18, 8:46 AM, "Linus Walleij" <linus.walleij@linaro.org> wrote:
>
> > Actually this is much more intuitive too, it is the typical way to handle
> > a down-counting timer. Good catch!
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>
> Thank you Linus for the quick review!
>
> > Sorry for any cargo-cult programming on my part :/
> > Would be nice to get a nod from the AST2400 users that this works
> > for them too, so included them in the To: field.
>
> Make sense. I actually booted up kernel on qemu-palmetto (ast2400) but I'm doubting if test is valid because it depends on how qemu emulates the hardware. It would be great if someone can help to verify the patch on physical ast2400.

I gave this a spin on the ast2400. It looked okay, but I was wondering
if you could share you test case so I can run the same test?

Cheers,

Joel

Re: [PATCH] clocksource/drivers/fttmr010: fix set_next_event handler

[Lei YU]

> >
> > Make sense. I actually booted up kernel on qemu-palmetto (ast2400) but I'm doubting if test is valid because it depends on how qemu emulates the hardware. It would be great if someone can help to verify the patch on physical ast2400.
>
> I gave this a spin on the ast2400. It looked okay, but I was wondering
> if you could share you test case so I can run the same test?

Tested on Palmetto (OpenPOWER P8 with AST2400), and it does show the fix is
working on AST2400 as well.
Without the patch, usleep(100) takes tens of milliseconds randomly;
With the patch, usleep(100) takes about 600~700 microseconds stably.

Tested-by: Lei YU <mine260309@gmail.com>

Re: [PATCH] clocksource/drivers/fttmr010: fix set_next_event handler

[Tao Ren]

On 9/20/18, 11:22 PM, "Lei YU" <mine260309@gmail.com> wrote:

> Tested on Palmetto (OpenPOWER P8 with AST2400), and it does show the fix is
> working on AST2400 as well.
> Without the patch, usleep(100) takes tens of milliseconds randomly;
> With the patch, usleep(100) takes about 600~700 microseconds stably. 
>
> Tested-by: Lei YU <mine260309@gmail.com>

Thank you Lei for the testing!


Thanks,
Tao Ren
    

Re: [PATCH] clocksource/drivers/fttmr010: fix set_next_event handler

[Tao Ren]

On 9/20/18, 8:33 PM, "Joel Stanley" <joel@jms.id.au> wrote:

> I gave this a spin on the ast2400. It looked okay, but I was wondering
> if you could share you test case so I can run the same test?

Thank you Joel for the testing.
Sure I can share the test (a small c program which measures delay introduced by nanosleep()), but what would be the preferred way to share it? Like a format-patch? Or maybe I should refine the test and send to openbmc for review?


Thanks,
Tao Ren

Re: [PATCH] clocksource/drivers/fttmr010: fix set_next_event handler

[Joel Stanley]

On Sat, 22 Sep 2018 at 03:41, Tao Ren <taoren@fb.com> wrote:
>
> On 9/20/18, 8:33 PM, "Joel Stanley" <joel@jms.id.au> wrote:
>
> > I gave this a spin on the ast2400. It looked okay, but I was wondering
> > if you could share you test case so I can run the same test?
>
> Thank you Joel for the testing.
> Sure I can share the test (a small c program which measures delay introduced by nanosleep()), but what would be the preferred way to share it? Like a format-patch? Or maybe I should refine the test and send to openbmc for review?

Whatever method is easiest for you. A patch file or a tarball is fine.
I will then add it to the set of tests I run when testing aspeed
kernels.

Cheers,

Joel

Re: [PATCH] clocksource/drivers/fttmr010: fix set_next_event handler

[Tao Ren]

On 9/24/18, 4:20 PM, "Joel Stanley" <joel@jms.id.au> wrote:

> Whatever method is easiest for you. A patch file or a tarball is fine.
> I will then add it to the set of tests I run when testing aspeed
> kernels.

No problem, Joel. I will share the test code with you offline.


Thanks,
Tao Ren

Re: [PATCH] clocksource/drivers/fttmr010: fix set_next_event handler

[Sai Dasari]

On 9/24/18, 4:21 PM, "openbmc on behalf of Joel Stanley" <openbmc-bounces+sdasari=fb.com@lists.ozlabs.org on behalf of joel@jms.id.au> wrote:

    I will then add it to the set of tests I run when testing aspeed
    kernels.
    
Thanks Joel! Curious to know what would be the best place to keep these kind of Kernel test cases for sake of review and/or adding new test cases as needed.     

Re: [PATCH] clocksource/drivers/fttmr010: fix set_next_event handler

[Linus Walleij]

On Tue, Sep 25, 2018 at 7:13 PM Sai Dasari <sdasari@fb.com> wrote:

> On 9/24/18, 4:21 PM, "openbmc on behalf of Joel Stanley" <openbmc-bounces+sdasari=fb.com@lists.ozlabs.org on behalf of joel@jms.id.au> wrote:
>
>     I will then add it to the set of tests I run when testing aspeed
>     kernels.
>
> Thanks Joel! Curious to know what would be the best place to keep these kind of Kernel test cases for sake of review and/or adding new test cases as needed.

tools/time in the kernel tree?

Uh oh there is something familiar there already:
ls tools/time
udelay_test.sh

Yours,
Linus Walleij