[PATCH] clocksource/drivers/fttmr010: fix invalid interrupt register access

[PATCH] clocksource/drivers/fttmr010: fix invalid interrupt register access

[Tao Ren]

TIMER_INTR_MASK register (Base Address of Timer + 0x38) is not designed
for masking interrupts on ast2500 chips, and it's not even listed in
ast2400 datasheet, so it's not safe to access TIMER_INTR_MASK on aspeed
chips.
Similarly, TIMER_INTR_STATE register (Base Address of Timer + 0x34) is
not interrupt status register on ast2400 and ast2500 chips. Although
there is no side effect to reset the register in fttmr010_common_init(),
it's just misleading to do so.

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

diff --git a/drivers/clocksource/timer-fttmr010.c b/drivers/clocksource/timer-fttmr010.c
index c020038ebfab..daf063c9842e 100644
--- a/drivers/clocksource/timer-fttmr010.c
+++ b/drivers/clocksource/timer-fttmr010.c
@@ -171,16 +171,17 @@ static int fttmr010_timer_set_oneshot(struct clock_event_device *evt)
 
 	/* Setup counter start from 0 or ~0 */
 	writel(0, fttmr010->base + TIMER1_COUNT);
-	if (fttmr010->count_down)
+	if (fttmr010->count_down) {
 		writel(~0, fttmr010->base + TIMER1_LOAD);
-	else
+	} else {
 		writel(0, fttmr010->base + TIMER1_LOAD);
 
-	/* Enable interrupt */
-	cr = readl(fttmr010->base + TIMER_INTR_MASK);
-	cr &= ~(TIMER_1_INT_OVERFLOW | TIMER_1_INT_MATCH2);
-	cr |= TIMER_1_INT_MATCH1;
-	writel(cr, fttmr010->base + TIMER_INTR_MASK);
+		/* Enable interrupt */
+		cr = readl(fttmr010->base + TIMER_INTR_MASK);
+		cr &= ~(TIMER_1_INT_OVERFLOW | TIMER_1_INT_MATCH2);
+		cr |= TIMER_1_INT_MATCH1;
+		writel(cr, fttmr010->base + TIMER_INTR_MASK);
+	}
 
 	return 0;
 }
@@ -287,13 +288,13 @@ static int __init fttmr010_common_init(struct device_node *np, bool is_aspeed)
 		fttmr010->count_down = true;
 	} else {
 		fttmr010->t1_enable_val = TIMER_1_CR_ENABLE | TIMER_1_CR_INT;
-	}
 
-	/*
-	 * Reset the interrupt mask and status
-	 */
-	writel(TIMER_INT_ALL_MASK, fttmr010->base + TIMER_INTR_MASK);
-	writel(0, fttmr010->base + TIMER_INTR_STATE);
+		/*
+		 * Reset the interrupt mask and status
+		 */
+		writel(TIMER_INT_ALL_MASK, fttmr010->base + TIMER_INTR_MASK);
+		writel(0, fttmr010->base + TIMER_INTR_STATE);
+	}
 
 	/*
 	 * Enable timer 1 count up, timer 2 count up, except on Aspeed,
-- 
2.17.1

Re: [PATCH] clocksource/drivers/fttmr010: fix invalid interrupt register access

[Linus Walleij]

Hi Tao, thanks for your patch!

On Tue, Oct 2, 2018 at 6:14 AM Tao Ren <taoren@fb.com> wrote:

> -       if (fttmr010->count_down)
> +       if (fttmr010->count_down) {
>                 writel(~0, fttmr010->base + TIMER1_LOAD);

This struct member "count_down" is a bit badly named now don't
you think?

The patch is fine semantically, but please rename this member
"is_aspeed" or something like that and update the code everywhere,
then insert a comments like

/* The Aspeed variant counts downward */
/* The Aspeed variant does not have a match interrupt */

in the code snippets so we see what is going on.

Yours,
Linus Walleij

Re: [PATCH] clocksource/drivers/fttmr010: fix invalid interrupt register access

[Tao Ren]

On 10/02/2018 12:35 AM, Linus Walleij wrote:

> This struct member "count_down" is a bit badly named now don't you think?
> The patch is fine semantically, but please rename this member "is_aspeed" or something like that and update the code everywhere,

Thank you Linus for the quick review.
I was actually planning a cleanup patch which handles is_aspeed/count_down stuff. Basically we have 2 options:
    1) adding "is_aspeed" flag. "count_down" is kept because potentially faraday-fttmr could also be configured as count_down timer (although I don't see any reason to do so).
    2) renaming "count_down" to "is_aspeed". By doing this, we set restriction that faraday-fttmr will always be upward.
What's your thought? Do you want me to include all the changes in this diff?

> then insert a comments like
> /* The Aspeed variant counts downward */
> /* The Aspeed variant does not have a match interrupt */
> in the code snippets so we see what is going on.

Make sense. Let me add more comments.


Thanks,
Tao Ren

Re: [PATCH] clocksource/drivers/fttmr010: fix invalid interrupt register access

[Linus Walleij]

On Wed, Oct 3, 2018 at 12:08 AM Tao Ren <taoren@fb.com> wrote:

>     1) adding "is_aspeed" flag. "count_down" is kept because potentially faraday-fttmr could also be configured as count_down timer (although I don't see any reason to do so).
>     2) renaming "count_down" to "is_aspeed". By doing this, we set restriction that faraday-fttmr will always be upward.
> What's your thought? Do you want me to include all the changes in this diff?

My thought is go for (2) and do all changes in one patch :)

Yours,
Linus Walleij

Re: [PATCH] clocksource/drivers/fttmr010: fix invalid interrupt register access

[Tao Ren]

On 10/2/18, 11:57 PM, "Linus Walleij" <linus.walleij@linaro.org> wrote:

> My thought is go for (2) and do all changes in one patch :)

No problem, Linus.
One more question: looks like my first patch 4451d3f59f2a (fix set_next_event handler) is not merged back to "timers/core". Should I generate this patch on top of the first patch or on top of the current "timers/core"? Which one would be easier for you (or Daniel/Thomas)? Sorry I'm pretty new to the community..

Thanks,
Tao Ren

Re: [PATCH] clocksource/drivers/fttmr010: fix invalid interrupt register access

[Daniel Lezcano]

On 03/10/2018 09:46, Tao Ren wrote:
> On 10/2/18, 11:57 PM, "Linus Walleij" <linus.walleij@linaro.org>
> wrote:
> 
>> My thought is go for (2) and do all changes in one patch :)
> 
> No problem, Linus. One more question: looks like my first patch
> 4451d3f59f2a (fix set_next_event handler) is not merged back to
> "timers/core". Should I generate this patch on top of the first patch
> or on top of the current "timers/core"? Which one would be easier for
> you (or Daniel/Thomas)? Sorry I'm pretty new to the community..


Hi Tao,

the branch tip:timers/urgent will be merged in tip:/timers/core, so the
fixes will be propagated to the master branch.

Base your change in top of tip:timers/urgent, the merge will happen soon
and both fixes will end up in tip:timers/core.

  -- Daniel


-- 
 <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 invalid interrupt register access

[Tao Ren]

On 10/3/18, 5:28 AM, "Daniel Lezcano" <daniel.lezcano@linaro.org> wrote:

> the branch tip:timers/urgent will be merged in tip:/timers/core, so the fixes will be propagated to the master branch.
> Base your change in top of tip:timers/urgent, the merge will happen soon and both fixes will end up in tip:timers/core.

Got it. Thank you Daniel.


- Tao