| Message ID | 20181002041437.2493192-1-taoren@fb.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | clocksource/drivers/fttmr010: fix invalid interrupt register access | expand |
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
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
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
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
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
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
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,
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(-)