* [PATCH] clocksource/drivers/fttmr010: fix invalid interrupt register access
@ 2018-10-02 4:14 Tao Ren
2018-10-02 7:35 ` Linus Walleij
0 siblings, 1 reply; 7+ messages in thread
From: Tao Ren @ 2018-10-02 4:14 UTC (permalink / raw)
To: linux-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(-)
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH] clocksource/drivers/fttmr010: fix invalid interrupt register access
2018-10-02 4:14 [PATCH] clocksource/drivers/fttmr010: fix invalid interrupt register access Tao Ren
@ 2018-10-02 7:35 ` Linus Walleij
2018-10-02 22:08 ` Tao Ren
0 siblings, 1 reply; 7+ messages in thread
From: Linus Walleij @ 2018-10-02 7:35 UTC (permalink / raw)
To: linux-aspeed
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
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH] clocksource/drivers/fttmr010: fix invalid interrupt register access
2018-10-02 7:35 ` Linus Walleij
@ 2018-10-02 22:08 ` Tao Ren
2018-10-03 6:56 ` Linus Walleij
0 siblings, 1 reply; 7+ messages in thread
From: Tao Ren @ 2018-10-02 22:08 UTC (permalink / raw)
To: linux-aspeed
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] clocksource/drivers/fttmr010: fix invalid interrupt register access
2018-10-02 22:08 ` Tao Ren
@ 2018-10-03 6:56 ` Linus Walleij
2018-10-03 7:46 ` Tao Ren
0 siblings, 1 reply; 7+ messages in thread
From: Linus Walleij @ 2018-10-03 6:56 UTC (permalink / raw)
To: linux-aspeed
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] clocksource/drivers/fttmr010: fix invalid interrupt register access
2018-10-03 6:56 ` Linus Walleij
@ 2018-10-03 7:46 ` Tao Ren
2018-10-03 12:28 ` Daniel Lezcano
0 siblings, 1 reply; 7+ messages in thread
From: Tao Ren @ 2018-10-03 7:46 UTC (permalink / raw)
To: linux-aspeed
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] clocksource/drivers/fttmr010: fix invalid interrupt register access
2018-10-03 7:46 ` Tao Ren
@ 2018-10-03 12:28 ` Daniel Lezcano
2018-10-03 21:05 ` Tao Ren
0 siblings, 1 reply; 7+ messages in thread
From: Daniel Lezcano @ 2018-10-03 12:28 UTC (permalink / raw)
To: linux-aspeed
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] clocksource/drivers/fttmr010: fix invalid interrupt register access
2018-10-03 12:28 ` Daniel Lezcano
@ 2018-10-03 21:05 ` Tao Ren
0 siblings, 0 replies; 7+ messages in thread
From: Tao Ren @ 2018-10-03 21:05 UTC (permalink / raw)
To: linux-aspeed
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-10-03 21:05 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-10-02 4:14 [PATCH] clocksource/drivers/fttmr010: fix invalid interrupt register access Tao Ren
2018-10-02 7:35 ` Linus Walleij
2018-10-02 22:08 ` Tao Ren
2018-10-03 6:56 ` Linus Walleij
2018-10-03 7:46 ` Tao Ren
2018-10-03 12:28 ` Daniel Lezcano
2018-10-03 21:05 ` Tao Ren
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox