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 <[email protected]>
---
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
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 <[email protected]>
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
On Wed, Sep 19, 2018 at 3:13 PM Tao Ren <[email protected]> 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 <[email protected]>
Actually this is much more intuitive too, it is the typical way to handle
a down-counting timer. Good catch!
Reviewed-by: Linus Walleij <[email protected]>
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
On 9/20/18, 8:46 AM, "Linus Walleij" <[email protected]> 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 <[email protected]>
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
On Fri, 21 Sep 2018 at 06:39, Tao Ren <[email protected]> wrote:
>
> On 9/20/18, 8:46 AM, "Linus Walleij" <[email protected]> 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 <[email protected]>
>
> 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
> >
> > 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 <[email protected]>
On 9/20/18, 11:22 PM, "Lei YU" <[email protected]> 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 <[email protected]>
Thank you Lei for the testing!
Thanks,
Tao Ren
On 9/20/18, 8:33 PM, "Joel Stanley" <[email protected]> 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
On Sat, 22 Sep 2018 at 03:41, Tao Ren <[email protected]> wrote:
>
> On 9/20/18, 8:33 PM, "Joel Stanley" <[email protected]> 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
On 9/24/18, 4:20 PM, "Joel Stanley" <[email protected]> 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
On 9/24/18, 4:21 PM, "openbmc on behalf of Joel Stanley" <[email protected] on behalf of [email protected]> 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.
On Tue, Sep 25, 2018 at 7:13 PM Sai Dasari <[email protected]> wrote:
> On 9/24/18, 4:21 PM, "openbmc on behalf of Joel Stanley" <[email protected] on behalf of [email protected]> 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