[PATCH 1/3 v2] clocksource/drivers/fttmr010: Be stricter on IRQs

[PATCH 1/3 v2] clocksource/drivers/fttmr010: Be stricter on IRQs

[Linus Walleij]

Make sure we check that the right interrupt occurred before
calling the event handler for timer 1. Report spurious IRQs
as IRQ_NONE.

Cc: Cédric Le Goater <clg@kaod.org>
Cc: Joel Stanley <joel@jms.id.au>
Cc: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Do not try to detect spurious IRQs on the Aspeed
  2400 and 2500 that miss the IRQ status register.
---
 drivers/clocksource/timer-fttmr010.c | 42 ++++++++++++++++++++++------
 1 file changed, 34 insertions(+), 8 deletions(-)

diff --git a/drivers/clocksource/timer-fttmr010.c b/drivers/clocksource/timer-fttmr010.c
index 126fb1f259b2..f47099dda96b 100644
--- a/drivers/clocksource/timer-fttmr010.c
+++ b/drivers/clocksource/timer-fttmr010.c
@@ -253,20 +253,46 @@ static int fttmr010_timer_set_periodic(struct clock_event_device *evt)
  */
 static irqreturn_t fttmr010_timer_interrupt(int irq, void *dev_id)
 {
-	struct clock_event_device *evt = dev_id;
+	struct fttmr010 *fttmr010 = dev_id;
+	struct clock_event_device *evt = &fttmr010->clkevt;
+	u32 val;
+
+	if (fttmr010->is_aspeed) {
+		/*
+		 * Aspeed versions do not implement the interrupt
+		 * status register and cannot detect spurious
+		 * interrupts.
+		 */
+		evt->event_handler(evt);
+		return IRQ_HANDLED;
+	}
+
+	val = readl(fttmr010->base + TIMER_INTR_STATE);
+	if (val & (TIMER_1_INT_MATCH1 | TIMER_1_INT_OVERFLOW))
+		evt->event_handler(evt);
+	else
+		/* Spurious IRQ */
+		return IRQ_NONE;
 
-	evt->event_handler(evt);
 	return IRQ_HANDLED;
 }
 
 static irqreturn_t ast2600_timer_interrupt(int irq, void *dev_id)
 {
-	struct clock_event_device *evt = dev_id;
-	struct fttmr010 *fttmr010 = to_fttmr010(evt);
+	struct fttmr010 *fttmr010 = dev_id;
+	struct clock_event_device *evt = &fttmr010->clkevt;
+	u32 val;
 
-	writel(0x1, fttmr010->base + TIMER_INTR_STATE);
+	val = readl(fttmr010->base + TIMER_INTR_STATE);
+	if (val & (TIMER_1_INT_MATCH1 | TIMER_1_INT_OVERFLOW)) {
+		writel(TIMER_1_INT_MATCH1, fttmr010->base + TIMER_INTR_STATE);
+		evt->event_handler(evt);
+	} else {
+		/* Just clear any spurious IRQs from the block */
+		writel(val, fttmr010->base + TIMER_INTR_STATE);
+		return IRQ_NONE;
+	}
 
-	evt->event_handler(evt);
 	return IRQ_HANDLED;
 }
 
@@ -384,12 +410,12 @@ static int __init fttmr010_common_init(struct device_node *np,
 		fttmr010->timer_shutdown = ast2600_timer_shutdown;
 		ret = request_irq(irq, ast2600_timer_interrupt,
 				  IRQF_TIMER, "FTTMR010-TIMER1",
-				  &fttmr010->clkevt);
+				  fttmr010);
 	} else {
 		fttmr010->timer_shutdown = fttmr010_timer_shutdown;
 		ret = request_irq(irq, fttmr010_timer_interrupt,
 				  IRQF_TIMER, "FTTMR010-TIMER1",
-				  &fttmr010->clkevt);
+				  fttmr010);
 	}
 	if (ret) {
 		pr_err("FTTMR010-TIMER1 no IRQ\n");
-- 
2.31.1

[PATCH 2/3 v2] clocksource/drivers/fttmr010: Clear also overflow bit on AST2600

[Linus Walleij]

The code was originally just writing 0x1 into TIMER_INTR_STATE
on the AST2600. But that is just the bit for TIMER_1_INT_MATCH1
so if we're using periodic IRQs we also need to clear
TIMER_1_INT_OVERFLOW.

Cc: Cédric Le Goater <clg@kaod.org>
Cc: Joel Stanley <joel@jms.id.au>
Cc: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Resending with the other patches.
---
 drivers/clocksource/timer-fttmr010.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clocksource/timer-fttmr010.c b/drivers/clocksource/timer-fttmr010.c
index f47099dda96b..5af8ea388cc4 100644
--- a/drivers/clocksource/timer-fttmr010.c
+++ b/drivers/clocksource/timer-fttmr010.c
@@ -285,7 +285,8 @@ static irqreturn_t ast2600_timer_interrupt(int irq, void *dev_id)
 
 	val = readl(fttmr010->base + TIMER_INTR_STATE);
 	if (val & (TIMER_1_INT_MATCH1 | TIMER_1_INT_OVERFLOW)) {
-		writel(TIMER_1_INT_MATCH1, fttmr010->base + TIMER_INTR_STATE);
+		writel(TIMER_1_INT_MATCH1 | TIMER_1_INT_OVERFLOW,
+		       fttmr010->base + TIMER_INTR_STATE);
 		evt->event_handler(evt);
 	} else {
 		/* Just clear any spurious IRQs from the block */
-- 
2.31.1

[PATCH 3/3 v2] clocksource/drivers/fttmr010: Just count down

[Linus Walleij]

All timers can handled just counting down so what about just
doing that instead of special-casing the counting down mode.

This has the upside that overflow cannot occur so we can
remove some handling of that interrupt as well.

Cc: Cédric Le Goater <clg@kaod.org>
Cc: Joel Stanley <joel@jms.id.au>
Cc: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- New patch.
---
 drivers/clocksource/timer-fttmr010.c | 97 +++++-----------------------
 1 file changed, 16 insertions(+), 81 deletions(-)

diff --git a/drivers/clocksource/timer-fttmr010.c b/drivers/clocksource/timer-fttmr010.c
index 5af8ea388cc4..f72ec84884e2 100644
--- a/drivers/clocksource/timer-fttmr010.c
+++ b/drivers/clocksource/timer-fttmr010.c
@@ -119,21 +119,11 @@ static inline struct fttmr010 *to_fttmr010(struct clock_event_device *evt)
 	return container_of(evt, struct fttmr010, clkevt);
 }
 
-static unsigned long fttmr010_read_current_timer_up(void)
-{
-	return readl(local_fttmr->base + TIMER2_COUNT);
-}
-
 static unsigned long fttmr010_read_current_timer_down(void)
 {
 	return ~readl(local_fttmr->base + TIMER2_COUNT);
 }
 
-static u64 notrace fttmr010_read_sched_clock_up(void)
-{
-	return fttmr010_read_current_timer_up();
-}
-
 static u64 notrace fttmr010_read_sched_clock_down(void)
 {
 	return fttmr010_read_current_timer_down();
@@ -148,17 +138,7 @@ static int fttmr010_timer_set_next_event(unsigned long cycles,
 	/* Stop */
 	fttmr010->timer_shutdown(evt);
 
-	if (fttmr010->is_aspeed) {
-		/*
-		 * ASPEED Timer Controller will load TIMER1_LOAD register
-		 * into TIMER1_COUNT register when the timer is re-enabled.
-		 */
-		writel(cycles, fttmr010->base + TIMER1_LOAD);
-	} else {
-		/* Setup the match register forward in time */
-		cr = readl(fttmr010->base + TIMER1_COUNT);
-		writel(cr + cycles, fttmr010->base + TIMER1_MATCH1);
-	}
+	writel(cycles, fttmr010->base + TIMER1_LOAD);
 
 	/* Start */
 	cr = readl(fttmr010->base + TIMER_CR);
@@ -194,24 +174,11 @@ static int fttmr010_timer_shutdown(struct clock_event_device *evt)
 static int fttmr010_timer_set_oneshot(struct clock_event_device *evt)
 {
 	struct fttmr010 *fttmr010 = to_fttmr010(evt);
-	u32 cr;
 
 	/* Stop */
 	fttmr010->timer_shutdown(evt);
-
-	/* Setup counter start from 0 or ~0 */
 	writel(0, fttmr010->base + TIMER1_COUNT);
-	if (fttmr010->is_aspeed) {
-		writel(~0, fttmr010->base + TIMER1_LOAD);
-	} 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);
-	}
+	writel(~0, fttmr010->base + TIMER1_LOAD);
 
 	return 0;
 }
@@ -226,19 +193,7 @@ static int fttmr010_timer_set_periodic(struct clock_event_device *evt)
 	fttmr010->timer_shutdown(evt);
 
 	/* Setup timer to fire at 1/HZ intervals. */
-	if (fttmr010->is_aspeed) {
-		writel(period, fttmr010->base + TIMER1_LOAD);
-	} else {
-		cr = 0xffffffff - (period - 1);
-		writel(cr, fttmr010->base + TIMER1_COUNT);
-		writel(cr, fttmr010->base + TIMER1_LOAD);
-
-		/* Enable interrupt on overflow */
-		cr = readl(fttmr010->base + TIMER_INTR_MASK);
-		cr &= ~(TIMER_1_INT_MATCH1 | TIMER_1_INT_MATCH2);
-		cr |= TIMER_1_INT_OVERFLOW;
-		writel(cr, fttmr010->base + TIMER_INTR_MASK);
-	}
+	writel(period, fttmr010->base + TIMER1_LOAD);
 
 	/* Start the timer */
 	cr = readl(fttmr010->base + TIMER_CR);
@@ -268,7 +223,7 @@ static irqreturn_t fttmr010_timer_interrupt(int irq, void *dev_id)
 	}
 
 	val = readl(fttmr010->base + TIMER_INTR_STATE);
-	if (val & (TIMER_1_INT_MATCH1 | TIMER_1_INT_OVERFLOW))
+	if (val & TIMER_1_INT_MATCH1)
 		evt->event_handler(evt);
 	else
 		/* Spurious IRQ */
@@ -284,9 +239,8 @@ static irqreturn_t ast2600_timer_interrupt(int irq, void *dev_id)
 	u32 val;
 
 	val = readl(fttmr010->base + TIMER_INTR_STATE);
-	if (val & (TIMER_1_INT_MATCH1 | TIMER_1_INT_OVERFLOW)) {
-		writel(TIMER_1_INT_MATCH1 | TIMER_1_INT_OVERFLOW,
-		       fttmr010->base + TIMER_INTR_STATE);
+	if (val & TIMER_1_INT_MATCH1) {
+		writel(TIMER_1_INT_MATCH1, fttmr010->base + TIMER_INTR_STATE);
 		evt->event_handler(evt);
 	} else {
 		/* Just clear any spurious IRQs from the block */
@@ -360,15 +314,10 @@ static int __init fttmr010_common_init(struct device_node *np,
 		writel(0, fttmr010->base + TIMER_INTR_STATE);
 	}
 
-	/*
-	 * Enable timer 1 count up, timer 2 count up, except on Aspeed,
-	 * where everything just counts down.
-	 */
 	if (is_aspeed)
 		val = TIMER_2_CR_ASPEED_ENABLE;
 	else {
-		val = TIMER_2_CR_ENABLE | TIMER_1_CR_UPDOWN |
-			TIMER_2_CR_UPDOWN;
+		val = TIMER_2_CR_ENABLE;
 	}
 	writel(val, fttmr010->base + TIMER_CR);
 
@@ -381,23 +330,13 @@ static int __init fttmr010_common_init(struct device_node *np,
 	writel(0, fttmr010->base + TIMER2_MATCH1);
 	writel(0, fttmr010->base + TIMER2_MATCH2);
 
-	if (fttmr010->is_aspeed) {
-		writel(~0, fttmr010->base + TIMER2_LOAD);
-		clocksource_mmio_init(fttmr010->base + TIMER2_COUNT,
-				      "FTTMR010-TIMER2",
-				      fttmr010->tick_rate,
-				      300, 32, clocksource_mmio_readl_down);
-		sched_clock_register(fttmr010_read_sched_clock_down, 32,
-				     fttmr010->tick_rate);
-	} else {
-		writel(0, fttmr010->base + TIMER2_LOAD);
-		clocksource_mmio_init(fttmr010->base + TIMER2_COUNT,
-				      "FTTMR010-TIMER2",
-				      fttmr010->tick_rate,
-				      300, 32, clocksource_mmio_readl_up);
-		sched_clock_register(fttmr010_read_sched_clock_up, 32,
-				     fttmr010->tick_rate);
-	}
+	writel(~0, fttmr010->base + TIMER2_LOAD);
+	clocksource_mmio_init(fttmr010->base + TIMER2_COUNT,
+			      "FTTMR010-TIMER2",
+			      fttmr010->tick_rate,
+			      300, 32, clocksource_mmio_readl_down);
+	sched_clock_register(fttmr010_read_sched_clock_down, 32,
+			     fttmr010->tick_rate);
 
 	/*
 	 * Setup clockevent timer (interrupt-driven) on timer 1.
@@ -441,12 +380,8 @@ static int __init fttmr010_common_init(struct device_node *np,
 
 #ifdef CONFIG_ARM
 	/* Also use this timer for delays */
-	if (fttmr010->is_aspeed)
-		fttmr010->delay_timer.read_current_timer =
-			fttmr010_read_current_timer_down;
-	else
-		fttmr010->delay_timer.read_current_timer =
-			fttmr010_read_current_timer_up;
+	fttmr010->delay_timer.read_current_timer =
+		fttmr010_read_current_timer_down;
 	fttmr010->delay_timer.freq = fttmr010->tick_rate;
 	register_current_timer_delay(&fttmr010->delay_timer);
 #endif
-- 
2.31.1

Re: [PATCH 1/3 v2] clocksource/drivers/fttmr010: Be stricter on IRQs

[Cédric Le Goater]

Hello Linus,

On 9/22/21 21:56, Linus Walleij wrote:
> Make sure we check that the right interrupt occurred before
> calling the event handler for timer 1. Report spurious IRQs
> as IRQ_NONE.
> 
> Cc: Cédric Le Goater <clg@kaod.org>
> Cc: Joel Stanley <joel@jms.id.au>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

I think we should start by dropping all the AST2600 code which
is unused.

Thanks,

C.

Re: [PATCH 1/3 v2] clocksource/drivers/fttmr010: Be stricter on IRQs

[Linus Walleij]

On Thu, Sep 23, 2021 at 10:11 PM Cédric Le Goater <clg@kaod.org> wrote:

> I think we should start by dropping all the AST2600 code which
> is unused.

I don't see why, the hardware is there is it not?

In my experience it is unwise to try to system manage the kernel,
decide what hardware gets exposed to the frameworks and which
does not.

There have been instances in the past where we have first said we don't
need another timer on the system (so it is "dark silicon") and later brought
it back because it has some upside.

For example for a while the Ux500 was using clksrc-dbx500-prcmu.c
exclusively because it was the only clocksource that would not stop
during sleep, and nomadik-mtu.c was unused. Then we invented a
way to grade the different clocksources and switch between them
before sleep, but tagging one of them with
CLOCK_SOURCE_SUSPEND_NONSTOP and giving them the right
rating, see commit bc0750e464d4.

This was good because nomadi-mtu.c has higher granularity and
higher frequency when the system is awake but clksrc-dbx500-prcmu.c
is always ticking, so each is used for different purposes.

Lesson learned: register all hardware with the timekeeping core and
let the kernel decide what timer to use at what point and for what.

Yours,
Linus Walleij

Re: [PATCH 1/3 v2] clocksource/drivers/fttmr010: Be stricter on IRQs

[Cédric Le Goater]

On 9/23/21 23:05, Linus Walleij wrote:
> On Thu, Sep 23, 2021 at 10:11 PM Cédric Le Goater <clg@kaod.org> wrote:
> 
>> I think we should start by dropping all the AST2600 code which
>> is unused.
> 
> I don't see why, the hardware is there is it not?

The TMC34 register is different on the AST2600.

The only piece of code that makes sense is in ast2600_timer_interrupt() :
     
	writel(0x1, fttmr010->base + TIMER_INTR_STATE);

which clears the status.

If you really insist on keeping the AST2600 support, then I would
rework a bit ast2600_timer_interrupt() : drop TIMER_1_INT_OVERFLOW
and may be use BIT(0) instead of TIMER_1_INT_MATCH1, since the
register layout is different.

There are 8 timers on the AST2600.

Thanks,

C.

> In my experience it is unwise to try to system manage the kernel,
> decide what hardware gets exposed to the frameworks and which
> does not.
> 
> There have been instances in the past where we have first said we don't
> need another timer on the system (so it is "dark silicon") and later brought
> it back because it has some upside.
> 
> For example for a while the Ux500 was using clksrc-dbx500-prcmu.c
> exclusively because it was the only clocksource that would not stop
> during sleep, and nomadik-mtu.c was unused. Then we invented a
> way to grade the different clocksources and switch between them
> before sleep, but tagging one of them with
> CLOCK_SOURCE_SUSPEND_NONSTOP and giving them the right
> rating, see commit bc0750e464d4.
> 
> This was good because nomadi-mtu.c has higher granularity and
> higher frequency when the system is awake but clksrc-dbx500-prcmu.c
> is always ticking, so each is used for different purposes.
> 
> Lesson learned: register all hardware with the timekeeping core and
> let the kernel decide what timer to use at what point and for what.
> 
> Yours,
> Linus Walleij
>