* [PATCH v2] clocksource/drivers/fttmr010: fix invalid interrupt register access
@ 2018-10-03 21:53 Tao Ren
2018-10-07 21:03 ` Linus Walleij
0 siblings, 1 reply; 10+ messages in thread
From: Tao Ren @ 2018-10-03 21:53 UTC (permalink / raw)
To: Daniel Lezcano, Thomas Gleixner, Linus Walleij, Joel Stanley,
Andrew Jeffery, Yu Lei, linux-kernel, linux-aspeed, openbmc
Cc: 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.
Besides, "count_down" is renamed to "is_aspeed" in "fttmr010" structure,
and more comments are added so the code is more readble.
Signed-off-by: Tao Ren <taoren@fb.com>
---
Changes in v2:
- "count_down" is renamed to "is_aspeed" in "fttmr010" structure.
- more comments are added to make the code more readable.
Tested the patch on:
- Facebook Backpack CMM AST2500 BMC.
- Facebook Wedge100 AST2400 BMC.
drivers/clocksource/timer-fttmr010.c | 73 ++++++++++++++++------------
1 file changed, 42 insertions(+), 31 deletions(-)
diff --git a/drivers/clocksource/timer-fttmr010.c b/drivers/clocksource/timer-fttmr010.c
index cf93f6419b51..fadff7915dd9 100644
--- a/drivers/clocksource/timer-fttmr010.c
+++ b/drivers/clocksource/timer-fttmr010.c
@@ -21,7 +21,7 @@
#include <linux/delay.h>
/*
- * Register definitions for the timers
+ * Register definitions common for all the timer variants.
*/
#define TIMER1_COUNT (0x00)
#define TIMER1_LOAD (0x04)
@@ -36,9 +36,10 @@
#define TIMER3_MATCH1 (0x28)
#define TIMER3_MATCH2 (0x2c)
#define TIMER_CR (0x30)
-#define TIMER_INTR_STATE (0x34)
-#define TIMER_INTR_MASK (0x38)
+/*
+ * Control register (TMC30) bit fields for fttmr010/gemini/moxart timers.
+ */
#define TIMER_1_CR_ENABLE BIT(0)
#define TIMER_1_CR_CLOCK BIT(1)
#define TIMER_1_CR_INT BIT(2)
@@ -53,8 +54,9 @@
#define TIMER_3_CR_UPDOWN BIT(11)
/*
- * The Aspeed AST2400 moves bits around in the control register
- * and lacks bits for setting the timer to count upwards.
+ * Control register (TMC30) bit fields for aspeed ast2400/ast2500 timers.
+ * The aspeed timers move bits around in the control register and lacks
+ * bits for setting the timer to count upwards.
*/
#define TIMER_1_CR_ASPEED_ENABLE BIT(0)
#define TIMER_1_CR_ASPEED_CLOCK BIT(1)
@@ -66,6 +68,18 @@
#define TIMER_3_CR_ASPEED_CLOCK BIT(9)
#define TIMER_3_CR_ASPEED_INT BIT(10)
+/*
+ * Interrupt status/mask register definitions for fttmr010/gemini/moxart
+ * timers.
+ * The registers don't exist and they are not needed on aspeed timers
+ * because:
+ * - aspeed timer overflow interrupt is controlled by bits in Control
+ * Register (TMC30).
+ * - aspeed timers always generate interrupt when either one of the
+ * Match registers equals to Status register.
+ */
+#define TIMER_INTR_STATE (0x34)
+#define TIMER_INTR_MASK (0x38)
#define TIMER_1_INT_MATCH1 BIT(0)
#define TIMER_1_INT_MATCH2 BIT(1)
#define TIMER_1_INT_OVERFLOW BIT(2)
@@ -80,7 +94,7 @@
struct fttmr010 {
void __iomem *base;
unsigned int tick_rate;
- bool count_down;
+ bool is_aspeed;
u32 t1_enable_val;
struct clock_event_device clkevt;
#ifdef CONFIG_ARM
@@ -130,7 +144,7 @@ static int fttmr010_timer_set_next_event(unsigned long cycles,
cr &= ~fttmr010->t1_enable_val;
writel(cr, fttmr010->base + TIMER_CR);
- if (fttmr010->count_down) {
+ if (fttmr010->is_aspeed) {
/*
* ASPEED Timer Controller will load TIMER1_LOAD register
* into TIMER1_COUNT register when the timer is re-enabled.
@@ -175,16 +189,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->is_aspeed) {
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;
}
@@ -201,9 +216,8 @@ static int fttmr010_timer_set_periodic(struct clock_event_device *evt)
writel(cr, fttmr010->base + TIMER_CR);
/* Setup timer to fire at 1/HZ intervals. */
- if (fttmr010->count_down) {
+ if (fttmr010->is_aspeed) {
writel(period, fttmr010->base + TIMER1_LOAD);
- writel(0, fttmr010->base + TIMER1_MATCH1);
} else {
cr = 0xffffffff - (period - 1);
writel(cr, fttmr010->base + TIMER1_COUNT);
@@ -281,23 +295,21 @@ static int __init fttmr010_common_init(struct device_node *np, bool is_aspeed)
}
/*
- * The Aspeed AST2400 moves bits around in the control register,
- * otherwise it works the same.
+ * The Aspeed timers move bits around in the control register.
*/
if (is_aspeed) {
fttmr010->t1_enable_val = TIMER_1_CR_ASPEED_ENABLE |
TIMER_1_CR_ASPEED_INT;
- /* Downward not available */
- fttmr010->count_down = true;
+ fttmr010->is_aspeed = 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,
@@ -306,9 +318,8 @@ static int __init fttmr010_common_init(struct device_node *np, bool is_aspeed)
if (is_aspeed)
val = TIMER_2_CR_ASPEED_ENABLE;
else {
- val = TIMER_2_CR_ENABLE;
- if (!fttmr010->count_down)
- val |= TIMER_1_CR_UPDOWN | TIMER_2_CR_UPDOWN;
+ val = TIMER_2_CR_ENABLE | TIMER_1_CR_UPDOWN |
+ TIMER_2_CR_UPDOWN;
}
writel(val, fttmr010->base + TIMER_CR);
@@ -321,7 +332,7 @@ static int __init fttmr010_common_init(struct device_node *np, bool is_aspeed)
writel(0, fttmr010->base + TIMER2_MATCH1);
writel(0, fttmr010->base + TIMER2_MATCH2);
- if (fttmr010->count_down) {
+ if (fttmr010->is_aspeed) {
writel(~0, fttmr010->base + TIMER2_LOAD);
clocksource_mmio_init(fttmr010->base + TIMER2_COUNT,
"FTTMR010-TIMER2",
@@ -371,7 +382,7 @@ static int __init fttmr010_common_init(struct device_node *np, bool is_aspeed)
#ifdef CONFIG_ARM
/* Also use this timer for delays */
- if (fttmr010->count_down)
+ if (fttmr010->is_aspeed)
fttmr010->delay_timer.read_current_timer =
fttmr010_read_current_timer_down;
else
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] clocksource/drivers/fttmr010: fix invalid interrupt register access
2018-10-03 21:53 [PATCH v2] clocksource/drivers/fttmr010: fix invalid interrupt register access Tao Ren
@ 2018-10-07 21:03 ` Linus Walleij
0 siblings, 0 replies; 10+ messages in thread
From: Linus Walleij @ 2018-10-07 21:03 UTC (permalink / raw)
To: taoren
Cc: Daniel Lezcano, Thomas Gleixner, Joel Stanley, Andrew Jeffery,
Yu Lei, linux-kernel, linux-aspeed, OpenBMC Maillist
On Wed, Oct 3, 2018 at 11:54 PM Tao Ren <taoren@fb.com> wrote:
> 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.
>
> Besides, "count_down" is renamed to "is_aspeed" in "fttmr010" structure,
> and more comments are added so the code is more readble.
>
> Signed-off-by: Tao Ren <taoren@fb.com>
> ---
> Changes in v2:
> - "count_down" is renamed to "is_aspeed" in "fttmr010" structure.
> - more comments are added to make the code more readable.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] clocksource/drivers/fttmr010: fix invalid interrupt register access
@ 2018-10-07 21:03 ` Linus Walleij
0 siblings, 0 replies; 10+ messages in thread
From: Linus Walleij @ 2018-10-07 21:03 UTC (permalink / raw)
To: taoren
Cc: Daniel Lezcano, Thomas Gleixner, Joel Stanley, Andrew Jeffery,
Yu Lei, linux-kernel, linux-aspeed, OpenBMC Maillist
On Wed, Oct 3, 2018 at 11:54 PM Tao Ren <taoren@fb.com> wrote:
> 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.
>
> Besides, "count_down" is renamed to "is_aspeed" in "fttmr010" structure,
> and more comments are added so the code is more readble.
>
> Signed-off-by: Tao Ren <taoren@fb.com>
> ---
> Changes in v2:
> - "count_down" is renamed to "is_aspeed" in "fttmr010" structure.
> - more comments are added to make the code more readable.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] clocksource/drivers/fttmr010: fix invalid interrupt register access
2018-10-07 21:03 ` Linus Walleij
(?)
@ 2018-10-10 5:50 ` Tao Ren
-1 siblings, 0 replies; 10+ messages in thread
From: Tao Ren @ 2018-10-10 5:50 UTC (permalink / raw)
To: Linus Walleij
Cc: Daniel Lezcano, Thomas Gleixner, Joel Stanley, Andrew Jeffery,
Yu Lei, linux-kernel, linux-aspeed, OpenBMC Maillist
On 10/7/18, 2:03 PM, "Linus Walleij" <linus.walleij@linaro.org> wrote:
>> 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.
>>
>> Besides, "count_down" is renamed to "is_aspeed" in "fttmr010" structure,
>> and more comments are added so the code is more readble.
>>
>> Signed-off-by: Tao Ren <taoren@fb.com>
>> ---
>> Changes in v2:
>> - "count_down" is renamed to "is_aspeed" in "fttmr010" structure.
>> - more comments are added to make the code more readable.
>>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Thanks for the review, Linus.
- Tao Ren
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] clocksource/drivers/fttmr010: fix invalid interrupt register access
2018-10-07 21:03 ` Linus Walleij
(?)
(?)
@ 2018-11-05 18:43 ` Tao Ren
2018-11-05 18:50 ` Daniel Lezcano
-1 siblings, 1 reply; 10+ messages in thread
From: Tao Ren @ 2018-11-05 18:43 UTC (permalink / raw)
To: Daniel Lezcano, Thomas Gleixner
Cc: Joel Stanley, Andrew Jeffery, Yu Lei, linux-kernel, linux-aspeed,
OpenBMC Maillist, Linus Walleij
On 10/7/18, 2:03 PM, "Linus Walleij" <linus.walleij@linaro.org> wrote:
>
> On Wed, Oct 3, 2018 at 11:54 PM Tao Ren <taoren@fb.com> wrote:
>
>> 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.
>>
>> Besides, "count_down" is renamed to "is_aspeed" in "fttmr010" structure,
>> and more comments are added so the code is more readble.
>>
>> Signed-off-by: Tao Ren <taoren@fb.com>
>> ---
>> Changes in v2:
>> - "count_down" is renamed to "is_aspeed" in "fttmr010" structure.
>> - more comments are added to make the code more readable.
>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Hi Daniel / Thomas,
Any further comments on the patch? Or is there anything needed from my side?
Thanks,
Tao Ren
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] clocksource/drivers/fttmr010: fix invalid interrupt register access
2018-11-05 18:43 ` Tao Ren
@ 2018-11-05 18:50 ` Daniel Lezcano
2018-11-05 19:00 ` Tao Ren
0 siblings, 1 reply; 10+ messages in thread
From: Daniel Lezcano @ 2018-11-05 18:50 UTC (permalink / raw)
To: Tao Ren, Thomas Gleixner
Cc: Joel Stanley, Andrew Jeffery, Yu Lei, linux-kernel, linux-aspeed,
OpenBMC Maillist, Linus Walleij
On 05/11/2018 19:43, Tao Ren wrote:
> On 10/7/18, 2:03 PM, "Linus Walleij" <linus.walleij@linaro.org> wrote:
>>
>> On Wed, Oct 3, 2018 at 11:54 PM Tao Ren <taoren@fb.com> wrote:
>>
>>> 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.
>>>
>>> Besides, "count_down" is renamed to "is_aspeed" in "fttmr010" structure,
>>> and more comments are added so the code is more readble.
>>>
>>> Signed-off-by: Tao Ren <taoren@fb.com>
>>> ---
>>> Changes in v2:
>>> - "count_down" is renamed to "is_aspeed" in "fttmr010" structure.
>>> - more comments are added to make the code more readable.
>>
>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>
> Hi Daniel / Thomas,
>
> Any further comments on the patch? Or is there anything needed from my side?
Oh right, sorry. Should it go to stable also ? Is there a Fixes tag it
can apply ?
--
<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] 10+ messages in thread
* Re: [PATCH v2] clocksource/drivers/fttmr010: fix invalid interrupt register access
2018-11-05 18:50 ` Daniel Lezcano
@ 2018-11-05 19:00 ` Tao Ren
2018-12-07 1:13 ` Tao Ren
0 siblings, 1 reply; 10+ messages in thread
From: Tao Ren @ 2018-11-05 19:00 UTC (permalink / raw)
To: Daniel Lezcano, Thomas Gleixner
Cc: Joel Stanley, Andrew Jeffery, Yu Lei, linux-kernel, linux-aspeed,
OpenBMC Maillist, Linus Walleij
On 11/5/18, 10:51 AM, "Daniel Lezcano" <daniel.lezcano@linaro.org> wrote:
> Oh right, sorry. Should it go to stable also ? Is there a Fixes tag it can apply ?
Personally I don't think it needs to go to stable, because I don't see any functional failures caused by this invalid register access.
Thanks,
Tao Ren
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] clocksource/drivers/fttmr010: fix invalid interrupt register access
2018-11-05 19:00 ` Tao Ren
@ 2018-12-07 1:13 ` Tao Ren
2018-12-07 7:04 ` Daniel Lezcano
0 siblings, 1 reply; 10+ messages in thread
From: Tao Ren @ 2018-12-07 1:13 UTC (permalink / raw)
To: Daniel Lezcano, Thomas Gleixner
Cc: Joel Stanley, Andrew Jeffery, Yu Lei, linux-kernel, linux-aspeed,
OpenBMC Maillist, Linus Walleij
On 11/5/18, 11:00 AM, "Tao Ren" <taoren@fb.com> wrote:
> On 11/5/18, 10:51 AM, "Daniel Lezcano" <daniel.lezcano@linaro.org> wrote:
>> Oh right, sorry. Should it go to stable also ? Is there a Fixes tag it can apply ?
>>
> Personally I don't think it needs to go to stable, because I don't see any functional failures caused by this invalid register access.
Hi Daniel,
Not sure if I missed any emails from you, but looks like the patch is not included in your tree? Are we planning to include the patch in 4.21 merge window?
Thanks,
Tao Ren
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] clocksource/drivers/fttmr010: fix invalid interrupt register access
2018-12-07 1:13 ` Tao Ren
@ 2018-12-07 7:04 ` Daniel Lezcano
2018-12-07 21:22 ` Tao Ren
0 siblings, 1 reply; 10+ messages in thread
From: Daniel Lezcano @ 2018-12-07 7:04 UTC (permalink / raw)
To: Tao Ren, Thomas Gleixner
Cc: Joel Stanley, Andrew Jeffery, Yu Lei, linux-kernel, linux-aspeed,
OpenBMC Maillist, Linus Walleij
On 07/12/2018 02:13, Tao Ren wrote:
> On 11/5/18, 11:00 AM, "Tao Ren" <taoren@fb.com> wrote:
>
>> On 11/5/18, 10:51 AM, "Daniel Lezcano" <daniel.lezcano@linaro.org> wrote:
>>> Oh right, sorry. Should it go to stable also ? Is there a Fixes tag it can apply ?
>>>
>> Personally I don't think it needs to go to stable, because I don't see any functional failures caused by this invalid register access.
>
> Hi Daniel,
>
> Not sure if I missed any emails from you, but looks like the patch is not included in your tree? Are we planning to include the patch in 4.21 merge window?
Yes, I have it in the tree. I updated the next branch.
Thanks
-- 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] 10+ messages in thread
* Re: [PATCH v2] clocksource/drivers/fttmr010: fix invalid interrupt register access
2018-12-07 7:04 ` Daniel Lezcano
@ 2018-12-07 21:22 ` Tao Ren
0 siblings, 0 replies; 10+ messages in thread
From: Tao Ren @ 2018-12-07 21:22 UTC (permalink / raw)
To: Daniel Lezcano, Thomas Gleixner
Cc: Joel Stanley, Andrew Jeffery, Yu Lei, linux-kernel, linux-aspeed,
OpenBMC Maillist, Linus Walleij
On 12/6/18, 11:04 PM, "Daniel Lezcano" <daniel.lezcano@linaro.org> wrote:
>
> On 07/12/2018 02:13, Tao Ren wrote:
>> Not sure if I missed any emails from you, but looks like the patch is not included in your tree? Are we planning to include the patch in 4.21 merge window?
>
> Yes, I have it in the tree. I updated the next branch.
Got it. Thank you for the confirmation, Daniel.
Thanks,
Tao Ren
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-12-07 21:23 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-03 21:53 [PATCH v2] clocksource/drivers/fttmr010: fix invalid interrupt register access Tao Ren
2018-10-07 21:03 ` Linus Walleij
2018-10-07 21:03 ` Linus Walleij
2018-10-10 5:50 ` Tao Ren
2018-11-05 18:43 ` Tao Ren
2018-11-05 18:50 ` Daniel Lezcano
2018-11-05 19:00 ` Tao Ren
2018-12-07 1:13 ` Tao Ren
2018-12-07 7:04 ` Daniel Lezcano
2018-12-07 21:22 ` Tao Ren
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.