Armada 370/XP watchdog support

Some SoC have MMIO regions that are shared across orthogonal subsystems. This commit implements a possible solution for the thread-safe access of such regions through a spinlock-protected API.

Concurrent access is protected with a single spinlock for the entire MMIO address space. While this protects shared-registers, it also serializes access to unrelated/unshared registers.

We add relaxed and non-relaxed variants, by using writel_relaxed and writel, respectively. The rationale for this is that some users may not require register write completion but only thread-safe access to a register.

Signed-off-by Ezequiel Garcia <[email protected]>

Quoted 12 lines · On Tuesday 21 January 2014 06:12:27 Ezequiel Garcia wrote:

Some SoC have MMIO regions that are shared across orthogonal subsystems. This commit implements a possible solution for the thread-safe access of such regions through a spinlock-protected API.

Concurrent access is protected with a single spinlock for the entire MMIO address space. While this protects shared-registers, it also serializes access to unrelated/unshared registers.

We add relaxed and non-relaxed variants, by using writel_relaxed and writel, respectively. The rationale for this is that some users may not require register write completion but only thread-safe access to a register.

Quoted 1 line

Signed-off-by Ezequiel Garcia <[email protected]>

You add the new atomic mmio interfaces in an ARM global header file, but at the same time make them ARM-only. I'm not opposed to having interfaces like that, but I'm not convinced they are actually needed for this case and if we go there, it needs to be done more carefully and should be available for all architectures so that portable drivers can use them.

It also seems to duplicate functionality that is already present in regmap-mmio.

Quoted 9 lines · On Tuesday 21 January 2014 06:12:28 Ezequiel Garcia wrote:

drivers/clocksource/time-orion.c +0 -9

-/*
- * Thread-safe access to TIMER_CTRL register
- * (shared with watchdog timer)
- */
-void orion_timer_ctrl_clrset(u32 clr, u32 set)
-{
-       spin_lock(&timer_ctrl_lock);
-       writel((readl(timer_base + TIMER_CTRL) & ~clr) | set,
-               timer_base + TIMER_CTRL);

drivers/clocksource/time-orion.c line 51

Quoted 3 lines

drivers/clocksource/time-orion.c +0 -3

-       spin_unlock(&timer_ctrl_lock);
-}
-EXPORT_SYMBOL(orion_timer_ctrl_clrset);

I don't understand what's wrong with this function, it seems like a cleaner approach than touching the register directly from two different drivers. Is this something that would only work on orion but not on armadaxp?

Quoted 10 lines · Arnd Bergmann

Quoted 9 lines · On Tuesday 21 January 2014 06:12:28 Ezequiel Garcia wrote:

drivers/clocksource/time-orion.c +0 -9

-/*
- * Thread-safe access to TIMER_CTRL register
- * (shared with watchdog timer)
- */
-void orion_timer_ctrl_clrset(u32 clr, u32 set)
-{
-       spin_lock(&timer_ctrl_lock);
-       writel((readl(timer_base + TIMER_CTRL) & ~clr) | set,
-               timer_base + TIMER_CTRL);

Quoted 8 lines

drivers/clocksource/time-orion.c line 51

Quoted 3 lines

drivers/clocksource/time-orion.c +0 -3

-       spin_unlock(&timer_ctrl_lock);
-}
-EXPORT_SYMBOL(orion_timer_ctrl_clrset);

I don't understand what's wrong with this function, it seems like a cleaner approach than touching the register directly from two different drivers. Is this something that would only work on orion but not on armadaxp?

The real problem with this is that it resides in orion-time.c which is fine for Orion SoCs. Armada 370/XP use a different timer and therefore the _common_ watchdog driver cannot call this function.

Moreover, Dove (out of Orion) and Armada 370/XP will happily live in one V7 kernel with both time-orion and time-armada-370-xp compiled in.

The idea of the atomic readl/writel was to have something lightweight and _early_ to allow such locking even for timers.

IIRC, there was some kind of consensus that it is okay to have atomic readl/writel here.

Quoted 3 lines

The real problem with this is that it resides in orion-time.c which is fine for Orion SoCs. Armada 370/XP use a different timer and therefore the _common_ watchdog driver cannot call this function.

Right, I had missed this part.

Quoted 2 lines

Moreover, Dove (out of Orion) and Armada 370/XP will happily live in one V7 kernel with both time-orion and time-armada-370-xp compiled in.

It still wouldn't be too hard to do: both clock drivers could export functions with different identifiers, and the wdt driver can use conditional compilation (#ifdef or if(IS_ENABLED())) to only call those functions when the respective clocksource drivers are enabled.

Replace the driver-specific thread-safe shared register API by the recently introduced atomic_io_clear_set().

Signed-off-by Ezequiel Garcia <[email protected]>

---
  drivers/clocksource/time-orion.c | 28 ++++++++++------------------
  1 file changed, 10 insertions(+), 18 deletions(-)

drivers/clocksource/time-orion.c +0 -14

index 9c7f018..3f14e56 100644
@@ -35,20 +35,6 @@
  #define ORION_ONESHOT_MAX	0xfffffffe
 
  static void __iomem *timer_base;
-static DEFINE_SPINLOCK(timer_ctrl_lock);
-
-/*
- * Thread-safe access to TIMER_CTRL register
- * (shared with watchdog timer)
- */
-void orion_timer_ctrl_clrset(u32 clr, u32 set)
-{
-	spin_lock(&timer_ctrl_lock);
-	writel((readl(timer_base + TIMER_CTRL) & ~clr) | set,
-		timer_base + TIMER_CTRL);
-	spin_unlock(&timer_ctrl_lock);
-}
-EXPORT_SYMBOL(orion_timer_ctrl_clrset);
  /*
   * Free-running clocksource handling.

drivers/clocksource/time-orion.c +8 -3

  	return 0;
  }

@@ -80,10 +67,13 @@ static void orion_clkevt_mode(enum clock_event_mode mode,
  		/* setup and enable periodic timer at 1/HZ intervals */
  		writel(ticks_per_jiffy - 1, timer_base + TIMER1_RELOAD);
  		writel(ticks_per_jiffy - 1, timer_base + TIMER1_VAL);
-		orion_timer_ctrl_clrset(0, TIMER1_RELOAD_EN | TIMER1_EN);
+		atomic_io_modify(timer_base + TIMER_CTRL,
+			TIMER1_RELOAD_EN | TIMER1_EN,
+			TIMER1_RELOAD_EN | TIMER1_EN);
  	} else {
  		/* disable timer */
-		orion_timer_ctrl_clrset(TIMER1_RELOAD_EN | TIMER1_EN, 0);
+		atomic_io_modify(timer_base + TIMER_CTRL,
+			TIMER1_RELOAD_EN | TIMER1_EN, 0);
  	}
  }
 

@@ -131,7 +121,9 @@ static void __init orion_timer_init(struct device_node *np)
  	/* setup timer0 as free-running clocksource */
  	writel(~0, timer_base + TIMER0_VAL);
  	writel(~0, timer_base + TIMER0_RELOAD);
-	orion_timer_ctrl_clrset(0, TIMER0_RELOAD_EN | TIMER0_EN);
+	atomic_io_modify(timer_base + TIMER_CTRL,
+		TIMER0_RELOAD_EN | TIMER0_EN,
+		TIMER0_RELOAD_EN | TIMER0_EN);
  	clocksource_mmio_init(timer_base + TIMER0_VAL, "orion_clocksource",
  			      clk_get_rate(clk), 300, 32,
  			      clocksource_mmio_readl_down);

Quoted 4 lines · Ezequiel Garcia

Replace the driver-specific thread-safe shared register API by the recently introduced atomic_io_clear_set().

Signed-off-by Ezequiel Garcia <[email protected]>

in the future, put me in Cc when modifying files in clocksource that will make my life easier to track the changes.

drivers/clocksource/time-orion.c static int orion_clkevt_next_event(unsigned long delta, · line 58

Quoted 9 lines

/*
* Free-running clocksource handling.

drivers/clocksource/time-orion.c +2 -1

@@ -68,7 +54,8 @@ static int orion_clkevt_next_event(unsigned long delta,
  {
  	/* setup and enable one-shot timer */
  	writel(delta, timer_base + TIMER1_VAL);
-	orion_timer_ctrl_clrset(TIMER1_RELOAD_EN, TIMER1_EN);
+	atomic_io_modify(timer_base + TIMER_CTRL,
+		TIMER1_RELOAD_EN | TIMER1_EN, TIMER1_EN);

I am not convinced this change is worth.

Could you elaborate the race the spinlock should prevent ?

Since the timer control register is shared with the clocksource driver, use the recently introduced atomic_io_clear_set() to access such register.

Signed-off-by Ezequiel Garcia <[email protected]>

---
 drivers/watchdog/orion_wdt.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

drivers/watchdog/orion_wdt.c +0 -0

index f7722a4..cf64510 100644

Quoted 14 lines · Ezequiel Garcia

Since the timer control register is shared with the clocksource driver, use the recently introduced atomic_io_clear_set() to access such register.

Signed-off-by Ezequiel Garcia <[email protected]>

---
 drivers/watchdog/orion_wdt.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

drivers/watchdog/orion_wdt.c +0 -0

index f7722a4..cf64510 100644
@@ -61,8 +61,6 @@ static int orion_wdt_ping(struct watchdog_device *wdt_dev)
 

drivers/watchdog/orion_wdt.c static int orion_wdt_ping(struct watchdog_device *wdt_dev) · line 64

Quoted 5 lines

drivers/watchdog/orion_wdt.c +0 -2

 static int orion_wdt_start(struct watchdog_device *wdt_dev)
 {
-       u32 reg;
-
        spin_lock(&wdt_lock);

Shouldn't this spin_lock be dropped now?

drivers/watchdog/orion_wdt.c static int orion_wdt_start(struct watchdog_device *wdt_dev) · line 73

Quoted 7 lines · On Tuesday 21 January 2014 06:12:29 Ezequiel Garcia wrote:

        writel(~WDT_INT_REQ, BRIDGE_CAUSE);
 
        /* Enable watchdog timer */

drivers/watchdog/orion_wdt.c +1 -3

-       reg = readl(wdt_reg + TIMER_CTRL);
-       reg |= WDT_EN;
-       writel(reg, wdt_reg + TIMER_CTRL);
+       atomic_io_modify(wdt_reg + TIMER_CTRL, WDT_EN, WDT_EN);

As mentioned in my comment for patch 2, it seems that the exported orion_timer_ctrl_clrset() function would be more appropriate for this.

drivers/watchdog/orion_wdt.c static int orion_wdt_start(struct watchdog_device *wdt_dev) · line 76

Quoted 6 lines

        /* Enable reset on watchdog */

drivers/watchdog/orion_wdt.c +1 -3

-       reg = readl(RSTOUTn_MASK);
-       reg |= WDT_RESET_OUT_EN;
-       writel(reg, RSTOUTn_MASK);
+       atomic_io_modify(RSTOUTn_MASK, WDT_RESET_OUT_EN, WDT_RESET_OUT_EN);

And this register already has an abstraction in arch/arm/mach-mvebu/system-controller.c. I would prefer to find a way to generalize that. I can see multiple ways to do that:

drivers/watchdog/orion_wdt.c static int orion_wdt_probe(struct platform_device *pdev) · line 164

Quoted 7 lines · Ezequiel Garcia

In order to support other SoC, it's required to distinguish the 'control' timer register, from the 'rstout' register that enables system reset on watchdog expiration.

drivers/watchdog/orion_wdt.c +3 -0

+     res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+     if (!res)
+             return -ENODEV;

           ^^^^^^^^^^^^^^^^^^^^^^

This change seems to break compatibility with existing DT files that have only a single entry in reg?

Can the value be defaulted some how if missing?

Quoted 10 lines · On Tuesday 21 January 2014 16:33:21 Jason Gunthorpe wrote:

drivers/watchdog/orion_wdt.c static int orion_wdt_probe(struct platform_device *pdev) · line 164

Quoted 7 lines · Ezequiel Garcia

In order to support other SoC, it's required to distinguish the 'control' timer register, from the 'rstout' register that enables system reset on watchdog expiration.

drivers/watchdog/orion_wdt.c +3 -0

+     res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+     if (!res)
+             return -ENODEV;

           ^^^^^^^^^^^^^^^^^^^^^^

Quoted 4 lines

This change seems to break compatibility with existing DT files that have only a single entry in reg?

Can the value be defaulted some how if missing?

I think this is a direct consequence of the attempt to remove the header file dependency, since the RSTOUTn_MASK macro is defined in mach/bridge-regs.h.

I don't see a good way out that would preserve backwards compatibility, other than hardcoding the physical address in the driver, which seems just as bad as breaking compatibility. That said, it is always the same constant (0xf1000000 + 0x20000 + 0x0108) on Dove, Kirkwood and Orion5x (not on mv78xx0, but that doesn't use the wdt), so hardcoding a fallback would technically work, but we should print a fat warning at boot time if we actually fall back to that.

Quoted 8 lines

I don't see a good way out that would preserve backwards compatibility, other than hardcoding the physical address in the driver, which seems just as bad as breaking compatibility. That said, it is always the same constant (0xf1000000 + 0x20000 + 0x0108) on Dove, Kirkwood and Orion5x (not on mv78xx0, but that doesn't use the wdt), so hardcoding a fallback would technically work, but we should print a fat warning at boot time if we actually fall back to that.

Yes, I was thinking just about this. Namely:

Quoted 9 lines · Ezequiel Garcia

Quoted 8 lines

I don't see a good way out that would preserve backwards compatibility, other than hardcoding the physical address in the driver, which seems just as bad as breaking compatibility. That said, it is always the same constant (0xf1000000 + 0x20000 + 0x0108) on Dove, Kirkwood and Orion5x (not on mv78xx0, but that doesn't use the wdt), so hardcoding a fallback would technically work, but we should print a fat warning at boot time if we actually fall back to that.

Quoted 2 lines

Yes, I was thinking just about this. Namely:

[..]

How about something like this?

Quoted 10 lines · Ezequiel Garcia

Quoted 9 lines · Ezequiel Garcia

Quoted 8 lines

I don't see a good way out that would preserve backwards compatibility, other than hardcoding the physical address in the driver, which seems just as bad as breaking compatibility. That said, it is always the same constant (0xf1000000 + 0x20000 + 0x0108) on Dove, Kirkwood and Orion5x (not on mv78xx0, but that doesn't use the wdt), so hardcoding a fallback would technically work, but we should print a fat warning at boot time if we actually fall back to that.

Quoted 5 lines

Quoted 2 lines

Yes, I was thinking just about this. Namely:

[..]

How about something like this?

I liked Arnd's idea to use an offset from the first register. With the mbus driver we can now actually change the 0xF1.. prefix via the DT.

drivers/watchdog/orion_wdt.c static irqreturn_t orion_wdt_irq(int irq, void *devid) · line 145

Quoted 2 lines

drivers/watchdog/orion_wdt.c +2 -0

+     dev_warn(&pdev->dev, "falling back to harcoded RSTOUT reg 0x%x\n",
+              ORION_RSTOUT_MASK);

Maybe:
dev_warn(&pdev->dev, FW_BUG "falling back to harcoded RSTOUT reg 0x%x\n",

Quoted 4 lines

drivers/watchdog/orion_wdt.c +0 -0

@@ -139,10 +166,7 @@ static int orion_wdt_probe(struct platform_device *pdev)
      if (!wdt_reg)
              return -ENOMEM;
 

drivers/watchdog/orion_wdt.c static int orion_wdt_probe(struct platform_device *pdev) · line 171

Quoted 7 lines

drivers/watchdog/orion_wdt.c +1 -4

-     res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
-     if (!res)
-             return -ENODEV;
-     wdt_rstout = devm_ioremap(&pdev->dev, res->start, resource_size(res));
+     wdt_rstout = try_compat_rstout_ioremap(pdev);
      if (!wdt_rstout)
              return -ENOMEM;

ENOMEM is probably not the right errno? Is there a standard errno for malformed DT?

Quoted 10 lines · Ezequiel Garcia

Quoted 9 lines · Ezequiel Garcia

Quoted 8 lines

I don't see a good way out that would preserve backwards compatibility, other than hardcoding the physical address in the driver, which seems just as bad as breaking compatibility. That said, it is always the same constant (0xf1000000 + 0x20000 + 0x0108) on Dove, Kirkwood and Orion5x (not on mv78xx0, but that doesn't use the wdt), so hardcoding a fallback would technically work, but we should print a fat warning at boot time if we actually fall back to that.

Quoted 5 lines

Quoted 2 lines

Yes, I was thinking just about this. Namely:

[..]

How about something like this?

I liked Arnd's idea to use an offset from the first register. With the mbus driver we can now actually change the 0xF1.. prefix via the DT.

After adding the IRQ request, the BRIDGE_CAUSE bit should be cleared by the bridge interrupt controller. There's no longer a need to do it in the watchdog driver, so we can simply remove it.

Quoted 3 lines · Ezequiel Garcia

After adding the IRQ request, the BRIDGE_CAUSE bit should be cleared by the bridge interrupt controller. There's no longer a need to do it in the watchdog driver, so we can simply remove it.

When we talked about this before I pointed out that sequence here is important:

- Disable WDT
- Clear bridge
- Enable WDT

Looking at this patch in isolation it looks to me like the clear bridge lines should be replaced with a request_irq (as that does the clear) - is the request_irq in the wrong spot?

Quoted 3 lines

Looking at this patch in isolation it looks to me like the clear bridge lines should be replaced with a request_irq (as that does the clear) - is the request_irq in the wrong spot?

First of all, it seems to me that the first item "Disable WDT" is not currently true on this driver. orion_wdt_start() seem to reset the counter, but doesn't clear the WDT_EN bit. Do you think we should enforce a "true" disable?

Regarding the sequence. Let me see if I got this problem right. The concern here is about the bootloader leaving the registers in a weird-state, right?

In that case, I thought that requesting the IRQ at probe time was enough to ensure the BRIDGE_CAUSE would be cleared by the time the watchdog is started. However, after reading through the irqchip code again, I'm no longer sure this is the case.

It looks like the BRIDGE_CAUSE register is cleared when the interruption is acked (which happens in the handler if I understood the code right). So requesting the IRQ is useless...

Sebastian: If the above is correct, do you think we can add a cause clear to the orion irqchip? (supposing it's harmful) Something like this:

Quoted 8 lines · Ezequiel Garcia

Quoted 3 lines

Looking at this patch in isolation it looks to me like the clear bridge lines should be replaced with a request_irq (as that does the clear) - is the request_irq in the wrong spot?

First of all, it seems to me that the first item "Disable WDT" is not currently true on this driver. orion_wdt_start() seem to reset the counter, but doesn't clear the WDT_EN bit. Do you think we should enforce a "true" disable?

I think so.

Quoted 3 lines

Regarding the sequence. Let me see if I got this problem right. The concern here is about the bootloader leaving the registers in a weird-state, right?

It isn't just bootloaders to worry about, but also things like kexec..

Quoted 4 lines

In that case, I thought that requesting the IRQ at probe time was enough to ensure the BRIDGE_CAUSE would be cleared by the time the watchdog is started. However, after reading through the irqchip code again, I'm no longer sure this is the case.

The watchdog should ideally be fully stopped before request_irq so there is no possible race.

Agreed. So, let's assume we can guarantee that request_irq() does the job of clearing the cause register (clearing pending irqs).

So, your suggestion is to put request_irq() in the watchdog start()?

Otherwise, we can ensure a watchdog full stop at probe(), before requesting the IRQ. Both solutions should work, uh?

Quoted 3 lines · Ezequiel Garcia

Agreed. So, let's assume we can guarantee that request_irq() does the job of clearing the cause register (clearing pending irqs).

Quoted 4 lines

So, your suggestion is to put request_irq() in the watchdog start()?

Otherwise, we can ensure a watchdog full stop at probe(), before requesting the IRQ. Both solutions should work, uh?

Right, I would keep the request_irq in probe, just because that feels more idiomatic..

Quoted 8 lines · Ezequiel Garcia

Quoted 3 lines

Looking at this patch in isolation it looks to me like the clear bridge lines should be replaced with a request_irq (as that does the clear) - is the request_irq in the wrong spot?

In that case, I thought that requesting the IRQ at probe time was enough to ensure the BRIDGE_CAUSE would be cleared by the time the watchdog is started. However, after reading through the irqchip code again, I'm no longer sure this is the case.

The watchdog should ideally be fully stopped before request_irq so there is no possible race.

Quoted 3 lines

It looks like the BRIDGE_CAUSE register is cleared when the interruption is acked (which happens in the handler if I understood the code right). So requesting the IRQ is useless...

IMHO, the IRQ stuff should clear out pending edge triggered interrupts at request_irq time. It makes no sense to take an interrupt for a stale edge event.

I had always assumed the core code did this via irq_gc_ack_clr_bit - but I don't see an obvious path..

Quoted 2 lines

Sebastian: If the above is correct, do you think we can add a cause clear to the orion irqchip? (supposing it's harmful) Something like this:

Clearing BRIDGE_CAUSE will only clear all currently pending upstream IRQs, of course. If WDT IRQ will be re-raised right after that in BRIDGE_CAUSE depends on the actual HW implementation, i.e. we do no clear the causing IRQ itself but just what it raised in BRIDGE_CAUSE.

So, you should also clear WDT's irq in the driver yourself to clear a possible pending upstream BRIDGE_CAUSE.

Quoted 4 lines

Clearing BRIDGE_CAUSE will only clear all currently pending upstream IRQs, of course. If WDT IRQ will be re-raised right after that in BRIDGE_CAUSE depends on the actual HW implementation, i.e. we do no clear the causing IRQ itself but just what it raised in BRIDGE_CAUSE.

Which is why it makes no sense to clear it one time at kernel start.

Quoted 7 lines · Jason Gunthorpe

Quoted 4 lines

Clearing BRIDGE_CAUSE will only clear all currently pending upstream IRQs, of course. If WDT IRQ will be re-raised right after that in BRIDGE_CAUSE depends on the actual HW implementation, i.e. we do no clear the causing IRQ itself but just what it raised in BRIDGE_CAUSE.

Which is why it makes no sense to clear it one time at kernel start.

So, it seems we need to handle irq_startup(), as you suggested. I've just tested the attached patch, and it's working fine: the driver's probe() fully stops the watchdog, and then request_irq() acks and pending interrupts, through the added irq_startup().

How does it look?

Quoted 14 lines · Ezequiel Garcia

Quoted 7 lines · Jason Gunthorpe

Quoted 4 lines

Clearing BRIDGE_CAUSE will only clear all currently pending upstream IRQs, of course. If WDT IRQ will be re-raised right after that in BRIDGE_CAUSE depends on the actual HW implementation, i.e. we do no clear the causing IRQ itself but just what it raised in BRIDGE_CAUSE.

Which is why it makes no sense to clear it one time at kernel start.

So, it seems we need to handle irq_startup(), as you suggested. I've just tested the attached patch, and it's working fine: the driver's probe() fully stops the watchdog, and then request_irq() acks and pending interrupts, through the added irq_startup().

Quoted 1 line

How does it look?

Looks sane to me.

I looked some more and there are other drivers (eg irq-metag-ext) that take this same approach.

I looked at the irq-orion driver a bit more and noticed this:

        ret = irq_alloc_domain_generic_chips(domain, nrirqs, 1, np->name,
                             handle_level_irq, clr, 0, IRQ_GC_INIT_MASK_CACHE);
                           ^^^^^^^^^^^^^^^^^^^^^

Shouldn't it be handle_edge_irq? Otherwise who is calling irq_ack? How does this work at all? :)

Quoted 9 lines · Ezequiel Garcia

Quoted 7 lines · Jason Gunthorpe

Quoted 4 lines

Clearing BRIDGE_CAUSE will only clear all currently pending upstream IRQs, of course. If WDT IRQ will be re-raised right after that in BRIDGE_CAUSE depends on the actual HW implementation, i.e. we do no clear the causing IRQ itself but just what it raised in BRIDGE_CAUSE.

Which is why it makes no sense to clear it one time at kernel start.

Quoted 6 lines

So, it seems we need to handle irq_startup(), as you suggested. I've just tested the attached patch, and it's working fine: the driver's probe() fully stops the watchdog, and then request_irq() acks and pending interrupts, through the added irq_startup().

How does it look?

Looks sane to me.

I looked some more and there are other drivers (eg irq-metag-ext) that take this same approach.

Sebastian:
I looked at the irq-orion driver a bit more and noticed this:

         ret = irq_alloc_domain_generic_chips(domain, nrirqs, 1, np->name,
                              handle_level_irq, clr, 0, IRQ_GC_INIT_MASK_CACHE);
                            ^^^^^^^^^^^^^^^^^^^^^

Shouldn't it be handle_edge_irq? Otherwise who is calling irq_ack? How does this work at all? :)

Quoted 8 lines

Sebastian:
I looked at the irq-orion driver a bit more and noticed this:

        ret = irq_alloc_domain_generic_chips(domain, nrirqs, 1, np->name,
                             handle_level_irq, clr, 0, IRQ_GC_INIT_MASK_CACHE);
                           ^^^^^^^^^^^^^^^^^^^^^

Shouldn't it be handle_edge_irq? Otherwise who is calling irq_ack? How does this work at all? :)

I can tell you that it comes from arch/arm/plat-orion/irq.c and I blindly copied it. I never really checked the differences in handling level/edge irqs. Besides, if it wasn't working, we wouldn't get far in booting the kernel without timer irqs.

It also remains asserted if you clear the actual cause of the interrupt and is only asserted again on the next low-to-high transition.

*BUT*, I will double-check how Linux deals with level/edge irqs and if Orion SoCs have edge or level triggered cause registers. That should reveal, if it is more sane to use handle_edge_irq here and possibly in the main interrupt controller, too.

Quoted 9 lines · Sebastian Hesselbarth

Quoted 8 lines

Sebastian:
I looked at the irq-orion driver a bit more and noticed this:

         ret = irq_alloc_domain_generic_chips(domain, nrirqs, 1, np->name,
                              handle_level_irq, clr, 0, IRQ_GC_INIT_MASK_CACHE);
                            ^^^^^^^^^^^^^^^^^^^^^

Shouldn't it be handle_edge_irq? Otherwise who is calling irq_ack? How does this work at all? :)

Quoted 4 lines

I can tell you that it comes from arch/arm/plat-orion/irq.c and I blindly copied it. I never really checked the differences in handling level/edge irqs. Besides, if it wasn't working, we wouldn't get far in booting the kernel without timer irqs.

Ezequiel found the ack call I missed, so it makes sense it works.

I think the difference in routines only starts to matter when you can get another incoming edge IRQ while already handling one (due to SMP? threaded interrupts? RealTime? not sure)

Quoted 2 lines

It also remains asserted if you clear the actual cause of the interrupt and is only asserted again on the next low-to-high transition.

Which is why Ezequiel's patch is the right approach: we need to clear the interrupt latched in the cause register after the watchdog driver disable but before enabling/unmasking the interrupt.

Remember, the BRIDGE_MASK register has no effect on the BRIDGE_CAUSE, it only effects which bits propogate to the main cause register.

Quoted 4 lines

*BUT*, I will double-check how Linux deals with level/edge irqs and if Orion SoCs have edge or level triggered cause registers. That should reveal, if it is more sane to use handle_edge_irq here and possibly in the main interrupt controller, too.

There is a mixture.

Any cause bit documented to be clearable is edge triggered, all others are level.

On Kirkwood this means all of the main interrupt controller bits are level and all the bridge bits are edge. Which means edge is definitely correct for the bridge handler, and level correct for the main handler.

On Kirkwood this means all of the main interrupt controller bits are level and all the bridge bits are edge. Which means edge is definitely correct for the bridge handler, and level correct for the main handler.

Quoted 4 lines

Clearing BRIDGE_CAUSE will only clear all currently pending upstream IRQs, of course. If WDT IRQ will be re-raised right after that in BRIDGE_CAUSE depends on the actual HW implementation, i.e. we do no clear the causing IRQ itself but just what it raised in BRIDGE_CAUSE.

Which is why it makes no sense to clear it one time at kernel start.

Either you only get new edge triggered interrupts after request_irq (sane behavior) or you might sometimes get an old pending edge triggered interrupt after request_irq (crazy behavior).

Clearing BRIDGE_CAUSE at kernel start only shortens the racy window it doesn't eliminate it.

In the more familiar level triggered world the driver would go to the device and ensure it wasn't asserting an IRQ level and then do the request_irq. This guarentees it won't get an interrupt callback.

In a edge triggered world the driver should go to the device an ensure that it won't create a new IRQ, then do request_irq - confident that there will NEVER be a call to the IRQ handler, under any circumstances.

So I think edge triggered interrupts need to ack any possible old edge trigger in the cause register before the first unmask - eg in the setup callback.

Quoted 2 lines

So, you should also clear WDT's irq in the driver yourself to clear a possible pending upstream BRIDGE_CAUSE.

Which isn't possible - the BRIDGE_CAUSE is owned by the irq driver and it must be cleared there, and it must only be cleared after the wdt has been stopped so it doesn't set it again.

Notice that Ezequiel has added an IRQ handler that just calls panic, so a spurious interrupt call is VERY VERY BAD.

Quoted 2 lines

Notice that Ezequiel has added an IRQ handler that just calls panic, so a spurious interrupt call is VERY VERY BAD.

And I understand that he now clears watchdog's register before requesting an irq. All that is missing is bridge_irq driver clearing CAUSE register after masking all irqs, right?

On Thu, Jan 23, 2014 at 12:49:50AM +0100, Sebastian Hesselbarth wrote:
[..]

Quoted 7 lines

Quoted 2 lines

Notice that Ezequiel has added an IRQ handler that just calls panic, so a spurious interrupt call is VERY VERY BAD.

And I understand that he now clears watchdog's register before requesting an irq. All that is missing is bridge_irq driver clearing CAUSE register after masking all irqs, right?

Are you sure clearing the CAUSE register after masking the IRQs will be enough?

AFAICS, until now nobody unmasks the watchdog IRQ (at least the orion_wdt driver didn't request the interruption) but *still* the CAUSE register is set upon watchdog expiration. So I would guessed a masked interrupt still raises a bit in the CAUSE register.

Sebastian,

Quoted 10 lines · Ezequiel Garcia

On Thu, Jan 23, 2014 at 12:49:50AM +0100, Sebastian Hesselbarth wrote:
[..]

Quoted 7 lines

Quoted 2 lines

Notice that Ezequiel has added an IRQ handler that just calls panic, so a spurious interrupt call is VERY VERY BAD.

And I understand that he now clears watchdog's register before requesting an irq. All that is missing is bridge_irq driver clearing CAUSE register after masking all irqs, right?

Quoted 7 lines

Are you sure clearing the CAUSE register after masking the IRQs will be enough?

AFAICS, until now nobody unmasks the watchdog IRQ (at least the orion_wdt driver didn't request the interruption) but *still* the CAUSE register is set upon watchdog expiration. So I would guessed a masked interrupt still raises a bit in the CAUSE register.

Let me add some real information instead of my speculations. Taken from the Kirkwood specification:

Table 136: Mbus-L to Mbus Bridge Interrupt Mask Register
Offset: 0x00020114
Field: Mask
Type/InitVal: RW 0x0
Description: There is a mask bit per each cause bit. Mask only affects the
assertion of interrupt pins. It does not affect the setting of bits in the Cause register.

So I guess this is why Jason has been insisting with the introduction of the irq_startup.

(Just for reference, the little patch I attached yesterday proved to work here.)

+ clk_prepare_enable(dev->clk);

drivers/watchdog/orion_wdt.c struct orion_watchdog { · line 73

Quoted 6 lines · Ezequiel Garcia

drivers/watchdog/orion_wdt.c +6 -0

+static int orion_wdt_clock_init(struct platform_device *pdev,
+                               struct orion_watchdog *dev)
+{
+       dev->clk = devm_clk_get(&pdev->dev, NULL);
+       if (IS_ERR(dev->clk))
+               return -ENODEV;

Shouldn't it be 'return PTR_ERR(dev->clk)' instead?

Quoted 1 line

+ clk_prepare_enable(dev->clk);

clk_prepare_enable may fail, so better check its return value.

After getting rid of all the mach-specific code, it's now possible to allow builds in any Orion platform.

Signed-off-by Ezequiel Garcia <[email protected]>

---
  drivers/watchdog/Kconfig | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

drivers/watchdog/Kconfig +0 -0

index 5be6e91..1689f72 100644

drivers/watchdog/Kconfig +0 -0

  	select WATCHDOG_CORE
  	help
  	  Say Y here if to include support for the watchdog timer

Quoted 13 lines · Ezequiel Garcia

After getting rid of all the mach-specific code, it's now possible to allow builds in any Orion platform.

Signed-off-by Ezequiel Garcia <[email protected]>

---
 drivers/watchdog/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/watchdog/Kconfig +0 -0

index 5be6e91..1689f72 100644

drivers/watchdog/Kconfig config DAVINCI_WATCHDOG · line 285

Quoted 5 lines

drivers/watchdog/Kconfig +1 -1

@@ -282,7 +282,7 @@ config DAVINCI_WATCHDOG
 
 config ORION_WATCHDOG
 	tristate "Orion watchdog"
-	depends on ARCH_ORION5X || ARCH_KIRKWOOD || ARCH_DOVE
+	depends on PLAT_ORION

Ezequiel,

while above is true now, I tend to rather have ARCH_MVEBU added here. We really want to get rid of both !ARCH_MVEBU and PLAT_ORION and this unnecessarily will make it more complicated.

I haven't checked why ARCH_MVEBU at all added PLAT_ORION as dependency, but IIRC it was just because of a missing mbus driver? If it is just this, we should also remove PLAT_ORION from ARCH_MVEBU to have a clean cut between new common arch and existing sub-archs.

Quoted 19 lines · Sebastian Hesselbarth

Quoted 18 lines · Ezequiel Garcia

After getting rid of all the mach-specific code, it's now possible to allow builds in any Orion platform.

Signed-off-by Ezequiel Garcia <[email protected]>

---
 drivers/watchdog/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/watchdog/Kconfig +1 -1

index 5be6e91..1689f72 100644
@@ -282,7 +282,7 @@ config DAVINCI_WATCHDOG
 
 config ORION_WATCHDOG
 	tristate "Orion watchdog"
-	depends on ARCH_ORION5X || ARCH_KIRKWOOD || ARCH_DOVE
+	depends on PLAT_ORION

Quoted 9 lines

while above is true now, I tend to rather have ARCH_MVEBU added here. We really want to get rid of both !ARCH_MVEBU and PLAT_ORION and this unnecessarily will make it more complicated.

I haven't checked why ARCH_MVEBU at all added PLAT_ORION as dependency, but IIRC it was just because of a missing mbus driver? If it is just this, we should also remove PLAT_ORION from ARCH_MVEBU to have a clean cut between new common arch and existing sub-archs.

I took a look at this when moving DT kirkwood into mach-mvebu. There is nothing in plat-orion which ARCH_MVEBU needs. So i agree with Sebastian, it should be PLAT_ORION || ARCH_MVEBU.

Quoted 18 lines · Ezequiel Garcia

After getting rid of all the mach-specific code, it's now possible to allow builds in any Orion platform.

Signed-off-by Ezequiel Garcia <[email protected]>

---
  drivers/watchdog/Kconfig | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

drivers/watchdog/Kconfig +1 -1

index 5be6e91..1689f72 100644
@@ -282,7 +282,7 @@ config DAVINCI_WATCHDOG
 
  config ORION_WATCHDOG
  	tristate "Orion watchdog"
-	depends on ARCH_ORION5X || ARCH_KIRKWOOD || ARCH_DOVE
+	depends on PLAT_ORION

I haven't checked why ARCH_MVEBU at all added PLAT_ORION as dependency, but IIRC it was just because of a missing mbus driver? If it is just this, we should also remove PLAT_ORION from ARCH_MVEBU to have a clean cut between new common arch and existing sub-archs.

Sebastian,

I'm picking on this discussion, with a new Subject.

Quoted 19 lines · Sebastian Hesselbarth

Quoted 18 lines · Ezequiel Garcia

After getting rid of all the mach-specific code, it's now possible to allow builds in any Orion platform.

Signed-off-by Ezequiel Garcia <[email protected]>

---
   drivers/watchdog/Kconfig | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)

drivers/watchdog/Kconfig +1 -1

index 5be6e91..1689f72 100644
@@ -282,7 +282,7 @@ config DAVINCI_WATCHDOG
 
   config ORION_WATCHDOG
   	tristate "Orion watchdog"
-	depends on ARCH_ORION5X || ARCH_KIRKWOOD || ARCH_DOVE
+	depends on PLAT_ORION

[..]

Quoted 6 lines

I haven't checked why ARCH_MVEBU at all added PLAT_ORION as dependency, but IIRC it was just because of a missing mbus driver? If it is just this, we should also remove PLAT_ORION from ARCH_MVEBU to have a clean cut between new common arch and existing sub-archs.

Well, I'm not sure if I'm following you. Many drivers currently depend on PLAT_ORION, so if we remove PLAT_ORION from ARCH_MVEBU we would have to replace: s/PLAT_ORION/ARCH_{MVEBU, KIRKWOOD, DOVE, ...}.

Is this the suggested roadmap or I've completely misunderstood it?

Now that we have proper support for Armada 370/XP watchdog let's enable it in the defconfig.

Signed-off-by Ezequiel Garcia <[email protected]>

---
 arch/arm/configs/mvebu_defconfig | 2 ++
 1 file changed, 2 insertions(+)

arch/arm/configs/mvebu_defconfig +2 -0

index 594d706..84ec924 100644
@@ -60,6 +60,8 @@ CONFIG_GPIOLIB=y
 CONFIG_GPIO_SYSFS=y
 CONFIG_THERMAL=y
 CONFIG_ARMADA_THERMAL=y
+CONFIG_WATCHDOG=y
+CONFIG_ORION_WATCHDOG=y
 CONFIG_USB_SUPPORT=y
 CONFIG_USB=y
 CONFIG_USB_EHCI_HCD=y

Signature · 1 line

1.8.1.5

Quoted 3 lines · Ezequiel Garcia

Now that we have proper support for Armada 370/XP watchdog let's enable it in the defconfig.

Quoted 4 lines

Signed-off-by Ezequiel Garcia <[email protected]>

---
 arch/arm/configs/mvebu_defconfig | 2 ++
 1 file changed, 2 insertions(+)

Just a note to myself more than anything: I need to square up multi_v7 with mvebu. As we make these changes next cycle, we need to be able to easily test that we aren't breaking multiplat build or boot.

Quoted 9 lines · Jason Cooper

Quoted 7 lines · Ezequiel Garcia

Now that we have proper support for Armada 370/XP watchdog let's enable it in the defconfig.

Signed-off-by Ezequiel Garcia <[email protected]>

---
 arch/arm/configs/mvebu_defconfig | 2 ++
 1 file changed, 2 insertions(+)

Quoted 3 lines

Just a note to myself more than anything: I need to square up multi_v7 with mvebu. As we make these changes next cycle, we need to be able to easily test that we aren't breaking multiplat build or boot.

Hi Jason

Hopefully you can also tackle multi_v5 as well, once DT kirkwood moves into mach-mvebu.

To check for breaking boot, you need some other hardware platform. Nomadik was breaking kirkwood multi_v5, and you would not see the problem on Nomadik.