LKML Archive on lore.kernel.org
 help / Atom feed
* [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; 9+ 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	[flat|nested] 9+ 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
  2018-10-10  5:50   ` Tao Ren
  2018-11-05 18:43   ` Tao Ren
  0 siblings, 2 replies; 9+ 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] 9+ 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
  2018-11-05 18:43   ` Tao Ren
  1 sibling, 0 replies; 9+ 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] 9+ 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
@ 2018-11-05 18:43   ` Tao Ren
  2018-11-05 18:50     ` Daniel Lezcano
  1 sibling, 1 reply; 9+ 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] 9+ 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; 9+ 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] 9+ 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; 9+ 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] 9+ 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; 9+ 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] 9+ 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; 9+ 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] 9+ 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; 9+ 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] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ 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-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

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox