[PATCH linux dev-4.18] clocksource/drivers/fttmr010: Fix set_next_event handler

[PATCH linux dev-4.18] 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>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Tested-by: Lei YU <mine260309@gmail.com>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 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 linux dev-4.18] clocksource/drivers/fttmr010: Fix set_next_event handler

[Tao Ren]

On 10/14/18, 8:47 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>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Tested-by: Lei YU <mine260309@gmail.com>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
 
Hi Joel / Andrew,

The patch is included in v4.19-rc6 (commit 4451d3f59f2a), so you could either apply the patch to dev-4.18 now, or it can also be included when kernel is upgraded to 4.19.


Thanks,

Tao Ren

Re: [PATCH linux dev-4.18] clocksource/drivers/fttmr010: Fix set_next_event handler

[Joel Stanley]

On Mon, 15 Oct 2018 at 14:27, Tao Ren <taoren@fb.com> wrote:
>
> On 10/14/18, 8:47 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>
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > Tested-by: Lei YU <mine260309@gmail.com>
> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>
> Hi Joel / Andrew,
>
> The patch is included in v4.19-rc6 (commit 4451d3f59f2a), so you could either apply the patch to dev-4.18 now, or it can also be included when kernel is upgraded to 4.19.

This patch was included in stable, so when openbmc moved to 4.18.16 we
got it for "free".

Thanks for submitting the fix upstream. This saves a everyone from
duplicated effort.

Cheers,

Joel