[PATCH v2 00/15] Armada 370/XP watchdog support

[PATCH v2 00/15] Armada 370/XP watchdog support

[Ezequiel Garcia]

The second patchset to extend Orion watchdog driver adding support for
Armada 370/XP SoC.

On this series I've tried to address all the issues raised when the previous
patchset was submitted. See here [1] for the previous thread.

I suggest that you go through the above thread to get the full picture.
Anyway, long story short, the main two issues are the ones preventing
multiplatform building:

* Watchdog RSTOUT enable.

  While v1 enabled the RSTOUT at each machine initialization, Jason Gunthorpe
  later pointed out [2] that such enabling might lead to a spurious watchdog
  trigger, in the event of the watchdog expired event not being cleared.

  Therefore, the current patchset adds RSTOUT as a second address resource
  (or 'reg' entry in devicetree words) to allow different platforms specify
  the corresponding address of the register. This change allows to build the
  driver on multiplatforms builds as helps remove a mach-specific header.

  The drawback of this is that the DT backwards compatibility gets broken;
  this was timely discussed but no better solution was achieved or proposed.

* BRIDGE CAUSE clear removal

  The watchdog cause clear should be done by the bridge irqchip driver, so
  it's fine to remove it from the watchdog driver and instead request the
  interruption.

  However, there are still a few platforms (orion5x, and legacy
  kirkwood/dove) that doesn't have this bridge irqchip support enabled.
  On these platforms the bridge cause clear is simply *not* done.

  If we are paranoid about this, maybe we can simply add the clear on each
  mach-xxx/irq.c, together with the other irq is initialization.

The rest of the work is mostly the same as v1.

Patches 1-3 introduces the Atomic I/O API needed to access shared registers;
this patch has been properly discussed elsewhere and has been submitted to
the ARM patch tracker for its inclusion in v3.14.

Patches 2-3 uses the atomic I/O API on the clocksource and watchdog drivers.
The orion clocksource patch is submitted here because the register is shared
with the watchdog.

Patch 4 adds the interrupt handling as already explained above.
Patch 5-6 peform the already discussed RSTOUT and BRIDGE CAUSE changes and
allows multiplatform build.

Patches 7-10 extends the orion_wdt driver to support other SoCs.
Patch 11 adds the Armada 370/XP support.

Patches 12-13 updates the devicetree files, patch 14 allows to build
the driver on any Orion platform and finally patch 15 updates the
defconfig.

This series is based on v3.13-rc8 and has been tested on:

 * Kirkwood Openblocks A6
 * Armada 370 Reference Design

[1] http://comments.gmane.org/gmane.linux.ports.arm.kernel/263459
[2] http://permalink.gmane.org/gmane.linux.ports.arm.kernel/263544

Ezequiel Garcia (15):
  ARM: Introduce atomic MMIO modify
  clocksource: orion: Use atomic access for shared registers
  watchdog: orion: Use atomic access for shared registers
  watchdog: orion: Handle IRQ
  watchdog: orion: Make RSTOUT register a separate resource
  watchdog: orion: Remove unneeded BRIDGE_CAUSE clear
  watchdog: orion: Introduce an orion_watchdog device structure
  watchdog: orion: Introduce per-compatible of_device_id data
  watchdog: orion: Add per-compatible clock initialization
  watchdog: orion: Add per-compatible watchdog start implementation
  watchdog: orion: Add support for Armada 370 and Armada XP SoC
  ARM: mvebu: Enable Armada 370/XP watchdog in the devicetree
  ARM: kirkwood: Add RSTOUT 'reg' entry to devicetree
  watchdog: orion: Allow to build on any Orion platform
  ARM: mvebu: Enable watchdog support in defconfig

 .../devicetree/bindings/watchdog/marvel.txt        |   8 +-
 arch/arm/boot/dts/armada-370-xp.dtsi               |   4 +
 arch/arm/boot/dts/armada-370.dtsi                  |   5 +
 arch/arm/boot/dts/armada-xp.dtsi                   |   6 +
 arch/arm/boot/dts/kirkwood.dtsi                    |   2 +-
 arch/arm/configs/mvebu_defconfig                   |   2 +
 arch/arm/include/asm/io.h                          |   6 +
 arch/arm/kernel/io.c                               |  35 +++
 arch/arm/mach-dove/include/mach/bridge-regs.h      |   1 +
 arch/arm/mach-kirkwood/include/mach/bridge-regs.h  |   1 +
 arch/arm/mach-mv78xx0/include/mach/bridge-regs.h   |   1 +
 arch/arm/mach-orion5x/include/mach/bridge-regs.h   |   1 +
 arch/arm/plat-orion/common.c                       |  10 +-
 drivers/clocksource/time-orion.c                   |  28 +-
 drivers/watchdog/Kconfig                           |   2 +-
 drivers/watchdog/orion_wdt.c                       | 316 ++++++++++++++++-----
 16 files changed, 333 insertions(+), 95 deletions(-)

-- 
1.8.1.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 01/15] ARM: Introduce atomic MMIO modify

[Ezequiel Garcia]

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 <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
Submitted to ARM patch tracker:
http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=7930/1

 arch/arm/include/asm/io.h |  6 ++++++
 arch/arm/kernel/io.c      | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+)

diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
index fbeb39c..8aa4cca 100644
--- a/arch/arm/include/asm/io.h
+++ b/arch/arm/include/asm/io.h
@@ -38,6 +38,12 @@
 #define isa_bus_to_virt phys_to_virt
 
 /*
+ * Atomic MMIO-wide IO modify
+ */
+extern void atomic_io_modify(void __iomem *reg, u32 mask, u32 set);
+extern void atomic_io_modify_relaxed(void __iomem *reg, u32 mask, u32 set);
+
+/*
  * Generic IO read/write.  These perform native-endian accesses.  Note
  * that some architectures will want to re-define __raw_{read,write}w.
  */
diff --git a/arch/arm/kernel/io.c b/arch/arm/kernel/io.c
index dcd5b4d..9203cf8 100644
--- a/arch/arm/kernel/io.c
+++ b/arch/arm/kernel/io.c
@@ -1,6 +1,41 @@
 #include <linux/export.h>
 #include <linux/types.h>
 #include <linux/io.h>
+#include <linux/spinlock.h>
+
+static DEFINE_RAW_SPINLOCK(__io_lock);
+
+/*
+ * Generic atomic MMIO modify.
+ *
+ * Allows thread-safe access to registers shared by unrelated subsystems.
+ * The access is protected by a single MMIO-wide lock.
+ */
+void atomic_io_modify_relaxed(void __iomem *reg, u32 mask, u32 set)
+{
+	unsigned long flags;
+	u32 value;
+
+	raw_spin_lock_irqsave(&__io_lock, flags);
+	value = readl_relaxed(reg) & ~mask;
+	value |= (set & mask);
+	writel_relaxed(value, reg);
+	raw_spin_unlock_irqrestore(&__io_lock, flags);
+}
+EXPORT_SYMBOL(atomic_io_modify_relaxed);
+
+void atomic_io_modify(void __iomem *reg, u32 mask, u32 set)
+{
+	unsigned long flags;
+	u32 value;
+
+	raw_spin_lock_irqsave(&__io_lock, flags);
+	value = readl_relaxed(reg) & ~mask;
+	value |= (set & mask);
+	writel(value, reg);
+	raw_spin_unlock_irqrestore(&__io_lock, flags);
+}
+EXPORT_SYMBOL(atomic_io_modify);
 
 /*
  * Copy data from IO memory space to "real" memory space.
-- 
1.8.1.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 01/15] ARM: Introduce atomic MMIO modify

[Arnd Bergmann]

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.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>

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.

	Arnd

Re: [PATCH v2 01/15] ARM: Introduce atomic MMIO modify

[Ezequiel Garcia]

On Tue, Jan 21, 2014 at 10:45:21AM +0100, Arnd Bergmann wrote:
> 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.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> 
> 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.
> 

Yes, true. We've discussed about this and even post an arch-generic API:

http://lwn.net/Articles/564709/

In the end it was decided to keep it ARM-specific for now:

http://www.spinics.net/lists/arm-kernel/msg271773.html

Just for reference, the last posted patchset for the atomic I/O API
is this:

http://www.spinics.net/lists/arm-kernel/msg292775.html

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

Yes, this is true. We tried to use regmap-mmio/syscon but we need this
to initialize the clocksource which is too early for that to work:

https://www.mail-archive.com/linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg484535.html

-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 02/15] clocksource: orion: Use atomic access for shared registers

[Ezequiel Garcia]

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

Signed-off-by: Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/clocksource/time-orion.c | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/drivers/clocksource/time-orion.c b/drivers/clocksource/time-orion.c
index 9c7f018..3f14e56 100644
--- a/drivers/clocksource/time-orion.c
+++ b/drivers/clocksource/time-orion.c
@@ -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.
@@ -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);
 
 	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);
-- 
1.8.1.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 02/15] clocksource: orion: Use atomic access for shared registers

[Arnd Bergmann]

On Tuesday 21 January 2014 06:12:28 Ezequiel Garcia wrote:
> -/*
> - * 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);

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?


	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 02/15] clocksource: orion: Use atomic access for shared registers

[Sebastian Hesselbarth]

On 01/21/14 10:46, Arnd Bergmann wrote:
> On Tuesday 21 January 2014 06:12:28 Ezequiel Garcia wrote:
>> -/*
>> - * 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);
>
> 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.

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 02/15] clocksource: orion: Use atomic access for shared registers

[Arnd Bergmann]

On Tuesday 21 January 2014 10:58:17 Sebastian Hesselbarth wrote:
> On 01/21/14 10:46, Arnd Bergmann wrote:
> > On Tuesday 21 January 2014 06:12:28 Ezequiel Garcia wrote:
> >> -/*
> >> - * 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);
> >
> > 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.

Right, I had missed this part.

> 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.

I think the part where it went wrong is the fact that we have two
device nodes in DT that claim the same register range, which should
not have happened, but seems too hard to fix now without breaking
one part or another.

> 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.

Ok, Ezequiel pointed me to that discussion now, seems ok then.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 02/15] clocksource: orion: Use atomic access for shared registers

[Andrew Lunn]

> > 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.
> 
> > 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.

Hi Arnd.

The plan is to make dove part of MULTI_V7. At that point, you have
both dove and armada-370-xp in the same kernel. Conditional
compilation does not work, the watchdog driver would need to know at
runtime which timer is in use, and call the right timer function.

I'm working on making DT Kirkwood part of MULTI_V5. The changes needed
are mostly trivial. I expect making Dove part of MULTI_V7 is also
trivial, so could happen during the next cycle. So this mix of Dove
and Armada 370 XP is not a theoretical problem, it is something which
needs to be solved ready for merging during the next window.

	Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 02/15] clocksource: orion: Use atomic access for shared registers

[Daniel Lezcano]

On 01/21/2014 10:12 AM, Ezequiel Garcia wrote:
> Replace the driver-specific thread-safe shared register API
> by the recently introduced atomic_io_clear_set().
>
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Hi Ezequiel,

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

Thanks.
   -- Daniel

> ---
>   drivers/clocksource/time-orion.c | 28 ++++++++++------------------
>   1 file changed, 10 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/clocksource/time-orion.c b/drivers/clocksource/time-orion.c
> index 9c7f018..3f14e56 100644
> --- a/drivers/clocksource/time-orion.c
> +++ b/drivers/clocksource/time-orion.c
> @@ -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.
> @@ -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 ?

>
>   	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);
>


-- 
  <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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 02/15] clocksource: orion: Use atomic access for shared registers

[Ezequiel Garcia]

Hello Daniel,

On Tue, Jan 21, 2014 at 10:57:37AM +0100, Daniel Lezcano wrote:
> On 01/21/2014 10:12 AM, Ezequiel Garcia wrote:
> > Replace the driver-specific thread-safe shared register API
> > by the recently introduced atomic_io_clear_set().
> >
> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> 
> in the future, put me in Cc when modifying files in clocksource that 
> will make my life easier to track the changes.
> 

Ouch, sorry about that.

[..]
> >   /*
> >    * Free-running clocksource handling.
> > @@ -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 ?
> 

Sure. Take a look to the patchset that adds the atomic I/O (sorry again for
not Ccing you on that one):

  http://www.spinics.net/lists/arm-kernel/msg292775.html

The TIMER_CTRL register provides clocksource and watchdog control, so we
need a safe way to access the register, since we're sharing it on two
completely unrelated drivers.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 03/15] watchdog: orion: Use atomic access for shared registers

[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 <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/watchdog/orion_wdt.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
index f7722a4..cf64510 100644
--- a/drivers/watchdog/orion_wdt.c
+++ b/drivers/watchdog/orion_wdt.c
@@ -61,8 +61,6 @@ static int orion_wdt_ping(struct watchdog_device *wdt_dev)
 
 static int orion_wdt_start(struct watchdog_device *wdt_dev)
 {
-	u32 reg;
-
 	spin_lock(&wdt_lock);
 
 	/* Set watchdog duration */
@@ -72,14 +70,10 @@ static int orion_wdt_start(struct watchdog_device *wdt_dev)
 	writel(~WDT_INT_REQ, BRIDGE_CAUSE);
 
 	/* Enable watchdog timer */
-	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);
 
 	/* Enable reset on watchdog */
-	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);
 
 	spin_unlock(&wdt_lock);
 	return 0;
@@ -87,19 +81,13 @@ static int orion_wdt_start(struct watchdog_device *wdt_dev)
 
 static int orion_wdt_stop(struct watchdog_device *wdt_dev)
 {
-	u32 reg;
-
 	spin_lock(&wdt_lock);
 
 	/* Disable reset on watchdog */
-	reg = readl(RSTOUTn_MASK);
-	reg &= ~WDT_RESET_OUT_EN;
-	writel(reg, RSTOUTn_MASK);
+	atomic_io_modify(RSTOUTn_MASK, WDT_RESET_OUT_EN, 0);
 
 	/* Disable watchdog timer */
-	reg = readl(wdt_reg + TIMER_CTRL);
-	reg &= ~WDT_EN;
-	writel(reg, wdt_reg + TIMER_CTRL);
+	atomic_io_modify(wdt_reg + TIMER_CTRL, WDT_EN, 0);
 
 	spin_unlock(&wdt_lock);
 	return 0;
-- 
1.8.1.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 03/15] watchdog: orion: Use atomic access for shared registers

[Fabio Estevam]

On Tue, Jan 21, 2014 at 7:12 AM, Ezequiel Garcia
<ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> 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 <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  drivers/watchdog/orion_wdt.c | 20 ++++----------------
>  1 file changed, 4 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
> index f7722a4..cf64510 100644
> --- a/drivers/watchdog/orion_wdt.c
> +++ b/drivers/watchdog/orion_wdt.c
> @@ -61,8 +61,6 @@ static int orion_wdt_ping(struct watchdog_device *wdt_dev)
>
>  static int orion_wdt_start(struct watchdog_device *wdt_dev)
>  {
> -       u32 reg;
> -
>         spin_lock(&wdt_lock);

Shouldn't this spin_lock be dropped now?

Regards,

Fabio Estevam
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 03/15] watchdog: orion: Use atomic access for shared registers

[Ezequiel Garcia]

On Tue, Jan 21, 2014 at 07:19:52AM -0200, Fabio Estevam wrote:
> On Tue, Jan 21, 2014 at 7:12 AM, Ezequiel Garcia
> <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> > 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 <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > ---
> >  drivers/watchdog/orion_wdt.c | 20 ++++----------------
> >  1 file changed, 4 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
> > index f7722a4..cf64510 100644
> > --- a/drivers/watchdog/orion_wdt.c
> > +++ b/drivers/watchdog/orion_wdt.c
> > @@ -61,8 +61,6 @@ static int orion_wdt_ping(struct watchdog_device *wdt_dev)
> >
> >  static int orion_wdt_start(struct watchdog_device *wdt_dev)
> >  {
> > -       u32 reg;
> > -
> >         spin_lock(&wdt_lock);
> 
> Shouldn't this spin_lock be dropped now?
> 

Hm... yes. The watchdog core uses a mutex to serialize all the watchdog hooks:
start, stop, ping, set_timeout, etc. So it seems you're right, the spinlock
should be dropped entirely as I see no need for it.

Thanks for the feedback,
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 03/15] watchdog: orion: Use atomic access for shared registers

[Arnd Bergmann]

On Tuesday 21 January 2014 06:12:29 Ezequiel Garcia wrote:
>         writel(~WDT_INT_REQ, BRIDGE_CAUSE);
>  
>         /* Enable watchdog timer */
> -       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.

>         /* Enable reset on watchdog */
> -       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:

* Turn the "marvell,armada-370-xp-system-controller" device into a "syscon"
  device that can be used through regmap from other parts of the kernel.

* Move arch/arm/mach-mvebu/system-controller.c into drivers/soc (we don't
  have this directory yet, but have discussed it in the past) to export
  soc-specific functions from there.

* Move all of the system-controller implementation into the wdt driver
  and require that this driver be enabled in order to do regular system
  reset. The system-controller can register itself as a reboot hook,
  so you can remove the ".restart = mvebu_restart" part from mach-mvebu.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 03/15] watchdog: orion: Use atomic access for shared registers

[Andrew Lunn]

On Tue, Jan 21, 2014 at 10:54:37AM +0100, Arnd Bergmann wrote:
> On Tuesday 21 January 2014 06:12:29 Ezequiel Garcia wrote:
> >         writel(~WDT_INT_REQ, BRIDGE_CAUSE);
> >  
> >         /* Enable watchdog timer */
> > -       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.

Hi Arnd

As Sebastian points out, Armada 370 XP does not have this function. 

> 
> >         /* Enable reset on watchdog */
> > -       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:

Please be aware that arch/arm/mach-mvebu/system-controller.c is
currently DT only. We need a solution which works for both with DT and
without DT, since orion5x and mv78xx0 none DT boards are not going
away soon.

     Andrew
 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 04/15] watchdog: orion: Handle IRQ

[Ezequiel Garcia]

DT-enabled where an irqchip driver for the brigde interrupt controller is
available can handle the watchdog IRQ properly. Therefore, we request
the interruption and add a dummy handler that merely calls panic().

This is done in order to have an initial 'ack' of the interruption,
which clears the watchdog state.

Furthermore, since some platforms don't have such IRQ, this commit
makes the interruption specification optional.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 .../devicetree/bindings/watchdog/marvel.txt        |  2 ++
 drivers/watchdog/orion_wdt.c                       | 22 +++++++++++++++++++++-
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/watchdog/marvel.txt b/Documentation/devicetree/bindings/watchdog/marvel.txt
index 5dc8d30..0731fbd 100644
--- a/Documentation/devicetree/bindings/watchdog/marvel.txt
+++ b/Documentation/devicetree/bindings/watchdog/marvel.txt
@@ -7,6 +7,7 @@ Required Properties:
 
 Optional properties:
 
+- interrupts	: Contains the IRQ for watchdog expiration
 - timeout-sec	: Contains the watchdog timeout in seconds
 
 Example:
@@ -14,6 +15,7 @@ Example:
 	wdt@20300 {
 		compatible = "marvell,orion-wdt";
 		reg = <0x20300 0x28>;
+		interrupts = <3>;
 		timeout-sec = <10>;
 		status = "okay";
 	};
diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
index cf64510..06e766f 100644
--- a/drivers/watchdog/orion_wdt.c
+++ b/drivers/watchdog/orion_wdt.c
@@ -19,6 +19,7 @@
 #include <linux/platform_device.h>
 #include <linux/watchdog.h>
 #include <linux/init.h>
+#include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/spinlock.h>
 #include <linux/clk.h>
@@ -131,10 +132,16 @@ static struct watchdog_device orion_wdt = {
 	.min_timeout = 1,
 };
 
+static irqreturn_t orion_wdt_irq(int irq, void *devid)
+{
+	panic("Watchdog Timeout");
+	return IRQ_HANDLED;
+}
+
 static int orion_wdt_probe(struct platform_device *pdev)
 {
 	struct resource *res;
-	int ret;
+	int ret, irq;
 
 	clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(clk)) {
@@ -151,6 +158,19 @@ static int orion_wdt_probe(struct platform_device *pdev)
 	if (!wdt_reg)
 		return -ENOMEM;
 
+	irq = platform_get_irq(pdev, 0);
+	if (irq > 0) {
+		/*
+		 * Not all supported platforms specify an interruption for the
+		 * watchdog, so let's make it optional.
+		 */
+		ret = request_irq(irq, orion_wdt_irq, 0, pdev->name, NULL);
+		if (ret < 0) {
+			dev_err(&pdev->dev, "failed to request IRQ\n");
+			return ret;
+		}
+	}
+
 	wdt_max_duration = WDT_MAX_CYCLE_COUNT / wdt_tclk;
 
 	orion_wdt.timeout = wdt_max_duration;
-- 
1.8.1.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 05/15] watchdog: orion: Make RSTOUT register a separate resource

[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.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 Documentation/devicetree/bindings/watchdog/marvel.txt |  6 ++++--
 arch/arm/mach-dove/include/mach/bridge-regs.h         |  1 +
 arch/arm/mach-kirkwood/include/mach/bridge-regs.h     |  1 +
 arch/arm/mach-mv78xx0/include/mach/bridge-regs.h      |  1 +
 arch/arm/mach-orion5x/include/mach/bridge-regs.h      |  1 +
 arch/arm/plat-orion/common.c                          | 10 ++++++----
 drivers/watchdog/orion_wdt.c                          | 12 ++++++++++--
 7 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/watchdog/marvel.txt b/Documentation/devicetree/bindings/watchdog/marvel.txt
index 0731fbd..1544fe9 100644
--- a/Documentation/devicetree/bindings/watchdog/marvel.txt
+++ b/Documentation/devicetree/bindings/watchdog/marvel.txt
@@ -3,7 +3,9 @@
 Required Properties:
 
 - Compatibility : "marvell,orion-wdt"
-- reg		: Address of the timer registers
+- reg		: Should contain two entries: first one with the
+		  timer control address, second one with the
+		  rstout enable address.
 
 Optional properties:
 
@@ -14,7 +16,7 @@ Example:
 
 	wdt@20300 {
 		compatible = "marvell,orion-wdt";
-		reg = <0x20300 0x28>;
+		reg = <0x20300 0x28>, <0x20108 0x4>;
 		interrupts = <3>;
 		timeout-sec = <10>;
 		status = "okay";
diff --git a/arch/arm/mach-dove/include/mach/bridge-regs.h b/arch/arm/mach-dove/include/mach/bridge-regs.h
index 5362df3..f4a5b34 100644
--- a/arch/arm/mach-dove/include/mach/bridge-regs.h
+++ b/arch/arm/mach-dove/include/mach/bridge-regs.h
@@ -21,6 +21,7 @@
 #define  CPU_CTRL_PCIE1_LINK	0x00000008
 
 #define RSTOUTn_MASK		(BRIDGE_VIRT_BASE + 0x0108)
+#define RSTOUTn_MASK_PHYS	(BRIDGE_PHYS_BASE + 0x0108)
 #define  SOFT_RESET_OUT_EN	0x00000004
 
 #define SYSTEM_SOFT_RESET	(BRIDGE_VIRT_BASE + 0x010c)
diff --git a/arch/arm/mach-kirkwood/include/mach/bridge-regs.h b/arch/arm/mach-kirkwood/include/mach/bridge-regs.h
index 8b9d1c9..60f6421 100644
--- a/arch/arm/mach-kirkwood/include/mach/bridge-regs.h
+++ b/arch/arm/mach-kirkwood/include/mach/bridge-regs.h
@@ -21,6 +21,7 @@
 #define CPU_RESET		0x00000002
 
 #define RSTOUTn_MASK		(BRIDGE_VIRT_BASE + 0x0108)
+#define RSTOUTn_MASK_PHYS	(BRIDGE_PHYS_BASE + 0x0108)
 #define SOFT_RESET_OUT_EN	0x00000004
 
 #define SYSTEM_SOFT_RESET	(BRIDGE_VIRT_BASE + 0x010c)
diff --git a/arch/arm/mach-mv78xx0/include/mach/bridge-regs.h b/arch/arm/mach-mv78xx0/include/mach/bridge-regs.h
index 5f03484..e20d6da 100644
--- a/arch/arm/mach-mv78xx0/include/mach/bridge-regs.h
+++ b/arch/arm/mach-mv78xx0/include/mach/bridge-regs.h
@@ -15,6 +15,7 @@
 #define L2_WRITETHROUGH		0x00020000
 
 #define RSTOUTn_MASK		(BRIDGE_VIRT_BASE + 0x0108)
+#define RSTOUTn_MASK_PHYS	(BRIDGE_PHYS_BASE + 0x0108)
 #define SOFT_RESET_OUT_EN	0x00000004
 
 #define SYSTEM_SOFT_RESET	(BRIDGE_VIRT_BASE + 0x010c)
diff --git a/arch/arm/mach-orion5x/include/mach/bridge-regs.h b/arch/arm/mach-orion5x/include/mach/bridge-regs.h
index f727d03..5766e3f 100644
--- a/arch/arm/mach-orion5x/include/mach/bridge-regs.h
+++ b/arch/arm/mach-orion5x/include/mach/bridge-regs.h
@@ -18,6 +18,7 @@
 #define CPU_CTRL		(ORION5X_BRIDGE_VIRT_BASE + 0x104)
 
 #define RSTOUTn_MASK		(ORION5X_BRIDGE_VIRT_BASE + 0x108)
+#define RSTOUTn_MASK_PHYS	(ORION5X_BRIDGE_PHYS_BASE + 0x108)
 
 #define CPU_SOFT_RESET		(ORION5X_BRIDGE_VIRT_BASE + 0x10c)
 
diff --git a/arch/arm/plat-orion/common.c b/arch/arm/plat-orion/common.c
index c66d163..3375037 100644
--- a/arch/arm/plat-orion/common.c
+++ b/arch/arm/plat-orion/common.c
@@ -594,14 +594,16 @@ void __init orion_spi_1_init(unsigned long mapbase)
 /*****************************************************************************
  * Watchdog
  ****************************************************************************/
-static struct resource orion_wdt_resource =
-		DEFINE_RES_MEM(TIMER_PHYS_BASE, 0x28);
+static struct resource orion_wdt_resource[] = {
+		DEFINE_RES_MEM(TIMER_PHYS_BASE, 0x04),
+		DEFINE_RES_MEM(RSTOUTn_MASK_PHYS, 0x04),
+};
 
 static struct platform_device orion_wdt_device = {
 	.name		= "orion_wdt",
 	.id		= -1,
-	.num_resources	= 1,
-	.resource	= &orion_wdt_resource,
+	.num_resources	= ARRAY_SIZE(orion_wdt_resource),
+	.resource	= orion_wdt_resource,
 };
 
 void __init orion_wdt_init(void)
diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
index 06e766f..9c7d695 100644
--- a/drivers/watchdog/orion_wdt.c
+++ b/drivers/watchdog/orion_wdt.c
@@ -47,6 +47,7 @@ static unsigned int wdt_max_duration;	/* (seconds) */
 static struct clk *clk;
 static unsigned int wdt_tclk;
 static void __iomem *wdt_reg;
+static void __iomem *wdt_rstout;
 static DEFINE_SPINLOCK(wdt_lock);
 
 static int orion_wdt_ping(struct watchdog_device *wdt_dev)
@@ -74,7 +75,7 @@ static int orion_wdt_start(struct watchdog_device *wdt_dev)
 	atomic_io_modify(wdt_reg + TIMER_CTRL, WDT_EN, WDT_EN);
 
 	/* Enable reset on watchdog */
-	atomic_io_modify(RSTOUTn_MASK, WDT_RESET_OUT_EN, WDT_RESET_OUT_EN);
+	atomic_io_modify(wdt_rstout, WDT_RESET_OUT_EN, WDT_RESET_OUT_EN);
 
 	spin_unlock(&wdt_lock);
 	return 0;
@@ -85,7 +86,7 @@ static int orion_wdt_stop(struct watchdog_device *wdt_dev)
 	spin_lock(&wdt_lock);
 
 	/* Disable reset on watchdog */
-	atomic_io_modify(RSTOUTn_MASK, WDT_RESET_OUT_EN, 0);
+	atomic_io_modify(wdt_rstout, WDT_RESET_OUT_EN, 0);
 
 	/* Disable watchdog timer */
 	atomic_io_modify(wdt_reg + TIMER_CTRL, WDT_EN, 0);
@@ -158,6 +159,13 @@ static int orion_wdt_probe(struct platform_device *pdev)
 	if (!wdt_reg)
 		return -ENOMEM;
 
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (!res)
+		return -ENODEV;
+	wdt_rstout = devm_ioremap(&pdev->dev, res->start, resource_size(res));
+	if (!wdt_rstout)
+		return -ENOMEM;
+
 	irq = platform_get_irq(pdev, 0);
 	if (irq > 0) {
 		/*
-- 
1.8.1.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 05/15] watchdog: orion: Make RSTOUT register a separate resource

[Jason Gunthorpe]

On Tue, Jan 21, 2014 at 06:12:31AM -0300, Ezequiel Garcia wrote:
> 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.

> +	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?

Regards,
Jason
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 05/15] watchdog: orion: Make RSTOUT register a separate resource

[Arnd Bergmann]

On Tuesday 21 January 2014 16:33:21 Jason Gunthorpe wrote:
> On Tue, Jan 21, 2014 at 06:12:31AM -0300, Ezequiel Garcia wrote:
> > 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.
> 
> > +     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?

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.

	Arnd

Re: [PATCH v2 05/15] watchdog: orion: Make RSTOUT register a separate resource

[Ezequiel Garcia]

Jason, Arnd:

On Wed, Jan 22, 2014 at 10:48:37AM +0100, Arnd Bergmann wrote:
> On Tuesday 21 January 2014 16:33:21 Jason Gunthorpe wrote:
> > On Tue, Jan 21, 2014 at 06:12:31AM -0300, Ezequiel Garcia wrote:
> > > 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.
> > 
> > > +     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?
> 
> 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.
> 

Exactly. Just for the record, I warned about this a while back:

http://www.spinics.net/lists/arm-kernel/msg270163.html

That said, it truly sucks to break compatibility so finding a way to
keep backwards compatibility would be certainly desirable.

> 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:

If the second resource is missing *and* the compatible-string is
"orion-watchdog" (or it's not DT-registered), then use a hardcoded
RSTOUTn_MASK address. And pr_warn() something.

Thanks for the feedback,
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 05/15] watchdog: orion: Make RSTOUT register a separate resource

[Ezequiel Garcia]

On Wed, Jan 22, 2014 at 01:21:36PM -0300, Ezequiel Garcia wrote:
> > 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:
> 
[..]

How about something like this?

diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
index a861b88..0014d23 100644
--- a/drivers/watchdog/orion_wdt.c
+++ b/drivers/watchdog/orion_wdt.c
@@ -26,6 +26,9 @@
 #include <linux/of.h>
 #include <mach/bridge-regs.h>
 
+/* RSTOUT mask register physical address for Orion5x, Kirkwood and Dove */
+#define ORION_RSTOUT_MASK	0xf1020108
+
 /*
  * Watchdog timer block registers.
  */
@@ -119,6 +122,30 @@ static irqreturn_t orion_wdt_irq(int irq, void *devid)
 	return IRQ_HANDLED;
 }
 
+/*
+ * The original devicetree binding for this driver specified only
+ * one memory resource, so in order to keep DT backwards compatibility
+ * we try to fallback to a hardcoded register address, if the resource
+ * is missing from the devicetree.
+ */
+static void __iomem * try_compat_rstout_ioremap(struct platform_device *pdev)
+{
+	struct resource *res;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (res)
+		return devm_ioremap(&pdev->dev, res->start,
+				    resource_size(res));
+
+	/* This workaround works only for "orion-wdt", DT-enabled */
+	if (!of_device_is_compatible(pdev->dev.of_node, "marvell,orion-wdt"))
+		return NULL;
+
+	dev_warn(&pdev->dev, "falling back to harcoded RSTOUT reg 0x%x\n",
+		 ORION_RSTOUT_MASK);
+	return devm_ioremap(&pdev->dev, ORION_RSTOUT_MASK, 0x4);
+}
+
 static int orion_wdt_probe(struct platform_device *pdev)
 {
 	struct resource *res;
@@ -139,10 +166,7 @@ static int orion_wdt_probe(struct platform_device *pdev)
 	if (!wdt_reg)
 		return -ENOMEM;
 
-	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;
 
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 05/15] watchdog: orion: Make RSTOUT register a separate resource

[Jason Gunthorpe]

On Wed, Jan 22, 2014 at 03:01:01PM -0300, Ezequiel Garcia wrote:
> On Wed, Jan 22, 2014 at 01:21:36PM -0300, Ezequiel Garcia wrote:
> > > 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:
> > 
> [..]
> 
> 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.

> +	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",

?

> @@ -139,10 +166,7 @@ static int orion_wdt_probe(struct platform_device *pdev)
>  	if (!wdt_reg)
>  		return -ENOMEM;
>  
> -	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?

Jason
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 05/15] watchdog: orion: Make RSTOUT register a separate resource

[Arnd Bergmann]

On Wednesday 22 January 2014 11:14:09 Jason Gunthorpe wrote:
> On Wed, Jan 22, 2014 at 03:01:01PM -0300, Ezequiel Garcia wrote:
> > On Wed, Jan 22, 2014 at 01:21:36PM -0300, Ezequiel Garcia wrote:
> > > > 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:
> > > 
> > [..]
> > 
> > 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.

That wasn't actually my idea, but it does sound reasonable.

> > +     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",

I was thinking of WARN_ON(), i.e. something that users will actually notice ;-)

> > @@ -139,10 +166,7 @@ static int orion_wdt_probe(struct platform_device *pdev)
> >       if (!wdt_reg)
> >               return -ENOMEM;
> >  
> > -     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?

I don't think so. I'd probably use ENODEV, but it's not ideal.

	Arnd

Re: [PATCH v2 05/15] watchdog: orion: Make RSTOUT register a separate resource

[Ezequiel Garcia]

On Wed, Jan 22, 2014 at 11:14:09AM -0700, Jason Gunthorpe wrote:
> On Wed, Jan 22, 2014 at 03:01:01PM -0300, Ezequiel Garcia wrote:
> > On Wed, Jan 22, 2014 at 01:21:36PM -0300, Ezequiel Garcia wrote:
> > > > 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:
> > > 
> > [..]
> > 
> > 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.
> 

Good idea. Maybe extracting the base address from the timer control
register: wdt_reg & 0xff000000 ?
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 06/15] watchdog: orion: Remove unneeded BRIDGE_CAUSE clear

[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.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/watchdog/orion_wdt.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
index 9c7d695..33e323d 100644
--- a/drivers/watchdog/orion_wdt.c
+++ b/drivers/watchdog/orion_wdt.c
@@ -25,7 +25,6 @@
 #include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/of.h>
-#include <mach/bridge-regs.h>
 
 /*
  * Watchdog timer block registers.
@@ -39,7 +38,6 @@
 #define WDT_OK_TO_CLOSE		1
 
 #define WDT_RESET_OUT_EN	BIT(1)
-#define WDT_INT_REQ		BIT(3)
 
 static bool nowayout = WATCHDOG_NOWAYOUT;
 static int heartbeat = -1;		/* module parameter (seconds) */
@@ -68,9 +66,6 @@ static int orion_wdt_start(struct watchdog_device *wdt_dev)
 	/* Set watchdog duration */
 	writel(wdt_tclk * wdt_dev->timeout, wdt_reg + WDT_VAL);
 
-	/* Clear watchdog timer interrupt */
-	writel(~WDT_INT_REQ, BRIDGE_CAUSE);
-
 	/* Enable watchdog timer */
 	atomic_io_modify(wdt_reg + TIMER_CTRL, WDT_EN, WDT_EN);
 
-- 
1.8.1.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 06/15] watchdog: orion: Remove unneeded BRIDGE_CAUSE clear

[Jason Gunthorpe]

On Tue, Jan 21, 2014 at 06:12:32AM -0300, Ezequiel Garcia wrote:
> 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?

> @@ -68,9 +66,6 @@ static int orion_wdt_start(struct watchdog_device *wdt_dev)
>  	/* Set watchdog duration */
>  	writel(wdt_tclk * wdt_dev->timeout, wdt_reg + WDT_VAL);
>  
> -	/* Clear watchdog timer interrupt */
> -	writel(~WDT_INT_REQ, BRIDGE_CAUSE);
> -
>  	/* Enable watchdog timer */
>  	atomic_io_modify(wdt_reg + TIMER_CTRL, WDT_EN, WDT_EN);
>  

Regards,
Jason
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 06/15] watchdog: orion: Remove unneeded BRIDGE_CAUSE clear

[Ezequiel Garcia]

On Tue, Jan 21, 2014 at 04:35:37PM -0700, Jason Gunthorpe wrote:
> On Tue, Jan 21, 2014 at 06:12:32AM -0300, Ezequiel Garcia wrote:
> > 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
> 

Sure, I wrote this series with that in mind and made some tests to
ensure that no spurious trigger was possible.

> 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?

As an example, s3c2410wdt calls stop() from start(). Maybe we should
do something like it?

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...

I'll trace the code to confirm this.

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:

diff --git a/drivers/irqchip/irq-orion.c b/drivers/irqchip/irq-orion.c
index e51d400..fef9171 100644
--- a/drivers/irqchip/irq-orion.c
+++ b/drivers/irqchip/irq-orion.c
@@ -180,6 +180,9 @@ static int __init orion_bridge_irq_init(struct device_node *np,
 	gc->chip_types[0].chip.irq_mask = irq_gc_mask_clr_bit;
 	gc->chip_types[0].chip.irq_unmask = irq_gc_mask_set_bit;
 
+	/* clear pending interrupts */
+	writel(0, gc->reg_base + ORION_BRIDGE_IRQ_CAUSE);
+
 	/* mask all interrupts */
 	writel(0, gc->reg_base + ORION_BRIDGE_IRQ_MASK);
 
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 06/15] watchdog: orion: Remove unneeded BRIDGE_CAUSE clear

[Jason Gunthorpe]

On Wed, Jan 22, 2014 at 01:49:05PM -0300, Ezequiel Garcia wrote:
> > 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.

> 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..

> 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.

> 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..

> 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:

Hrm, irq_startup looks like the right hook to put something like this
in.

Jason
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 06/15] watchdog: orion: Remove unneeded BRIDGE_CAUSE clear

[Ezequiel Garcia]

On Wed, Jan 22, 2014 at 10:34:17AM -0700, Jason Gunthorpe wrote:
> On Wed, Jan 22, 2014 at 01:49:05PM -0300, Ezequiel Garcia wrote:
> > > 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.
> 
> > 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..
> 
> > 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?

I don't have a strong opinion. It's not like the watchdog is going to be
frequently started/stopped (right?) so we can easily do the
request_irq() in the start() without worrying.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 06/15] watchdog: orion: Remove unneeded BRIDGE_CAUSE clear

[Jason Gunthorpe]

On Wed, Jan 22, 2014 at 02:45:40PM -0300, Ezequiel Garcia wrote:
 
> 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?

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

Jason
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 06/15] watchdog: orion: Remove unneeded BRIDGE_CAUSE clear

[Ezequiel Garcia]

On Wed, Jan 22, 2014 at 10:50:30AM -0700, Jason Gunthorpe wrote:
> On Wed, Jan 22, 2014 at 02:45:40PM -0300, Ezequiel Garcia wrote:
>  
> > 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?
> 
> Right, I would keep the request_irq in probe, just because that feels
> more idiomatic..
> 

Done. Baking v4...
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 06/15] watchdog: orion: Remove unneeded BRIDGE_CAUSE clear

[Sebastian Hesselbarth]

On 01/22/2014 06:34 PM, Jason Gunthorpe wrote:
> On Wed, Jan 22, 2014 at 01:49:05PM -0300, Ezequiel Garcia wrote:
>>> 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.
>
>> 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..
>
>> 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:

Ezequiel,

irqchip/irq-orion.c does mask all interrupts but you are right, it 
should also clear pending interrupts right after that.

So
	/* mask all interrupts */
	writel(0, gc->reg_base + ORION_BRIDGE_IRQ_MASK);

should become

	/* mask and clear all interrupts */
	writel(0, gc->reg_base + ORION_BRIDGE_IRQ_MASK);
	writel(~0, gc->reg_base + ORION_BRIDGE_IRQ_CAUSE);

Could also be clear on write 0, I'll check that. I already had some
beer, so I'll postpone any patches till tomorrow.

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.

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 06/15] watchdog: orion: Remove unneeded BRIDGE_CAUSE clear

[Jason Gunthorpe]

> 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.

> 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.

Jason
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 06/15] watchdog: orion: Remove unneeded BRIDGE_CAUSE clear

[Ezequiel Garcia]

On Wed, Jan 22, 2014 at 01:52:13PM -0700, Jason Gunthorpe wrote:
> > 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?

diff --git a/drivers/irqchip/irq-orion.c b/drivers/irqchip/irq-orion.c
index e51d400..91a3955 100644
--- a/drivers/irqchip/irq-orion.c
+++ b/drivers/irqchip/irq-orion.c
@@ -108,6 +108,16 @@ IRQCHIP_DECLARE(orion_intc, "marvell,orion-intc", orion_irq_init);
 #define ORION_BRIDGE_IRQ_CAUSE	0x00
 #define ORION_BRIDGE_IRQ_MASK	0x04
 
+static unsigned int orion_bridge_irq_startup(struct irq_data *data)
+{
+	/* Ack pending interrupts */
+	data->chip->irq_ack(data);
+
+	/* Unmask the interrupt */
+	data->chip->irq_unmask(data);
+	return 0;
+}
+
 static void orion_bridge_irq_handler(unsigned int irq, struct irq_desc *desc)
 {
 	struct irq_domain *d = irq_get_handler_data(irq);
@@ -176,6 +186,7 @@ static int __init orion_bridge_irq_init(struct device_node *np,
 
 	gc->chip_types[0].regs.ack = ORION_BRIDGE_IRQ_CAUSE;
 	gc->chip_types[0].regs.mask = ORION_BRIDGE_IRQ_MASK;
+	gc->chip_types[0].chip.irq_startup = orion_bridge_irq_startup;
 	gc->chip_types[0].chip.irq_ack = irq_gc_ack_clr_bit;
 	gc->chip_types[0].chip.irq_mask = irq_gc_mask_clr_bit;
 	gc->chip_types[0].chip.irq_unmask = irq_gc_mask_set_bit;


-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 06/15] watchdog: orion: Remove unneeded BRIDGE_CAUSE clear

[Jason Gunthorpe]

On Wed, Jan 22, 2014 at 07:12:38PM -0300, Ezequiel Garcia wrote:
> On Wed, Jan 22, 2014 at 01:52:13PM -0700, Jason Gunthorpe wrote:
> > > 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?

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? :)

Jason
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 06/15] watchdog: orion: Remove unneeded BRIDGE_CAUSE clear

[Ezequiel Garcia]

On Wed, Jan 22, 2014 at 03:31:17PM -0700, Jason Gunthorpe wrote:
> On Wed, Jan 22, 2014 at 07:12:38PM -0300, Ezequiel Garcia wrote:
> > On Wed, Jan 22, 2014 at 01:52:13PM -0700, Jason Gunthorpe wrote:
> > > > 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?
> 
> Looks sane to me.
> 
> I looked some more and there are other drivers (eg irq-metag-ext) that
> take this same approach.
> 

Yup, I took that one as a starting point.

[..]
> 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'm not familiar with the differences between handle_level_irq and
handle_edge_irq, but -AFAICS- both seem to ack the IRQ.

In fact handle_level_irq(), masks and acks the IRQ as the first thing.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 06/15] watchdog: orion: Remove unneeded BRIDGE_CAUSE clear

[Sebastian Hesselbarth]

On 01/22/2014 11:31 PM, Jason Gunthorpe wrote:
> On Wed, Jan 22, 2014 at 07:12:38PM -0300, Ezequiel Garcia wrote:
>> On Wed, Jan 22, 2014 at 01:52:13PM -0700, Jason Gunthorpe wrote:
>>>> 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?
>
> 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? :)

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.

 From a HW point-of-view, I'd say the difference is that an
edge-triggered irq samples level transitions from e.g. low to high.
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.

A level-triggered irq will be asserted as long as the cause of the irq
is asserted. If you clear the cause, you'll also clear that bit in the
upstream controller.

*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.

Sebastian


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 06/15] watchdog: orion: Remove unneeded BRIDGE_CAUSE clear

[Jason Gunthorpe]

On Thu, Jan 23, 2014 at 01:03:26AM +0100, Sebastian Hesselbarth wrote:
> >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.

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)

> 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.

> *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.

Jason
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 06/15] watchdog: orion: Remove unneeded BRIDGE_CAUSE clear

[Sebastian Hesselbarth]

On 01/23/2014 01:19 AM, Jason Gunthorpe wrote:
> On Thu, Jan 23, 2014 at 01:03:26AM +0100, Sebastian Hesselbarth wrote:
>>> 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.
>
> 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)
>
>> 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.

Yeah, I know. But you don't get new ones if mask them. At least for
edge triggered irqs, you can also clear them without clearing the
cause of the interrupt. Nevertheless, I think we agree here.

>> *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.

Just checked that for Dove, it is the same there. Main IRQ_CAUSE is
RO, BRIDGE_CAUSE is RW0C, and PMU_CAUSE is RW *sigh*.

I need to remember that when Dove moves over to mach-mvebu, as we need
a different chained irq handler for PMU that deals with that broken RW
register.

Sebastian

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 06/15] watchdog: orion: Remove unneeded BRIDGE_CAUSE clear

[Ezequiel Garcia]

On Wed, Jan 22, 2014 at 05:19:34PM -0700, Jason Gunthorpe wrote:
[..]
> 
> 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.
> 

Which means we should change the bridge handler to edge?

Is there any _noticeable_ side-effect in doing the handling via level?

-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 06/15] watchdog: orion: Remove unneeded BRIDGE_CAUSE clear

[Sebastian Hesselbarth]

On 01/22/2014 09:52 PM, Jason Gunthorpe wrote:
>> 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.

Actually, I missed that we mask all BRIDGE irqs in
orion_bridge_irq_init. If we now also clear already pending irqs, that
will not raise any old interrupts as long as watchdog clears all
reasons for upstream irqs before requesting a BRIDGE irq.

> 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.
 >
>> 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.

I should have been more precise here: I meant watchdog driver should
clear all sources of possible upstream interrupts in its _own_
registers.

> 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?

I'll stich a patch for that hopefully tomorrow.

Sebastian

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 06/15] watchdog: orion: Remove unneeded BRIDGE_CAUSE clear

[Ezequiel Garcia]

On Thu, Jan 23, 2014 at 12:49:50AM +0100, Sebastian Hesselbarth wrote:
[..]
> > 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.

Although I'm no sure I'm making sense here or just noise...
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 06/15] watchdog: orion: Remove unneeded BRIDGE_CAUSE clear

[Ezequiel Garcia]

Sebastian,

On Thu, Jan 23, 2014 at 08:10:49AM -0300, Ezequiel Garcia wrote:
> On Thu, Jan 23, 2014 at 12:49:50AM +0100, Sebastian Hesselbarth wrote:
> [..]
> > > 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.
> 

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.)
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 06/15] watchdog: orion: Remove unneeded BRIDGE_CAUSE clear

[Sebastian Hesselbarth]

Jason, Ezequiel,

On 01/23/2014 12:54 PM, Ezequiel Garcia wrote:
> Sebastian,
>
> On Thu, Jan 23, 2014 at 08:10:49AM -0300, Ezequiel Garcia wrote:
>> On Thu, Jan 23, 2014 at 12:49:50AM +0100, Sebastian Hesselbarth wrote:
>> [..]
>>>> 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.
>>
>
> 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.)

You guys were so right ;)

I just tested your v4 of the watchdog patches and forced a stale
watchdog irq. It will cause a watchdog reset as you predicted.

I have some fixes for irq-orion.c's bridge irq that take care of:
- handle_edge_irq
- mask _and_ clear irqs on init
- use irq_enable to ensure stale irqs are acked before unmask

With the last one, the stale watchdog irq will not raise watchdog's
irq handler anymore.

BTW, during my test, it looks like RSTOUT wasn't set, so I'll add
my Tested-by on v4 when I have investigated that.

Sebastian

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 07/15] watchdog: orion: Introduce an orion_watchdog device structure

[Ezequiel Garcia]

In order to prepare to support multiple compatible-strings, this
commit adds a device structure to hold the driver's state.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/watchdog/orion_wdt.c | 114 ++++++++++++++++++++++++++-----------------
 1 file changed, 68 insertions(+), 46 deletions(-)

diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
index 33e323d..a95968c 100644
--- a/drivers/watchdog/orion_wdt.c
+++ b/drivers/watchdog/orion_wdt.c
@@ -41,62 +41,71 @@
 
 static bool nowayout = WATCHDOG_NOWAYOUT;
 static int heartbeat = -1;		/* module parameter (seconds) */
-static unsigned int wdt_max_duration;	/* (seconds) */
-static struct clk *clk;
-static unsigned int wdt_tclk;
-static void __iomem *wdt_reg;
-static void __iomem *wdt_rstout;
-static DEFINE_SPINLOCK(wdt_lock);
+
+struct orion_watchdog {
+	struct watchdog_device wdt;
+	void __iomem *reg;
+	void __iomem *rstout;
+	spinlock_t lock;
+	unsigned long clk_rate;
+	struct clk *clk;
+};
 
 static int orion_wdt_ping(struct watchdog_device *wdt_dev)
 {
-	spin_lock(&wdt_lock);
+	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
+	spin_lock(&dev->lock);
 
 	/* Reload watchdog duration */
-	writel(wdt_tclk * wdt_dev->timeout, wdt_reg + WDT_VAL);
+	writel(dev->clk_rate * wdt_dev->timeout, dev->reg + WDT_VAL);
 
-	spin_unlock(&wdt_lock);
+	spin_unlock(&dev->lock);
 	return 0;
 }
 
 static int orion_wdt_start(struct watchdog_device *wdt_dev)
 {
-	spin_lock(&wdt_lock);
+	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
+
+	spin_lock(&dev->lock);
 
 	/* Set watchdog duration */
-	writel(wdt_tclk * wdt_dev->timeout, wdt_reg + WDT_VAL);
+	writel(dev->clk_rate * wdt_dev->timeout, dev->reg + WDT_VAL);
 
 	/* Enable watchdog timer */
-	atomic_io_modify(wdt_reg + TIMER_CTRL, WDT_EN, WDT_EN);
+	atomic_io_modify(dev->reg + TIMER_CTRL, WDT_EN, WDT_EN);
 
 	/* Enable reset on watchdog */
-	atomic_io_modify(wdt_rstout, WDT_RESET_OUT_EN, WDT_RESET_OUT_EN);
+	atomic_io_modify(dev->rstout, WDT_RESET_OUT_EN, WDT_RESET_OUT_EN);
 
-	spin_unlock(&wdt_lock);
+	spin_unlock(&dev->lock);
 	return 0;
 }
 
 static int orion_wdt_stop(struct watchdog_device *wdt_dev)
 {
-	spin_lock(&wdt_lock);
+	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
+
+	spin_lock(&dev->lock);
 
 	/* Disable reset on watchdog */
-	atomic_io_modify(wdt_rstout, WDT_RESET_OUT_EN, 0);
+	atomic_io_modify(dev->rstout, WDT_RESET_OUT_EN, 0);
 
 	/* Disable watchdog timer */
-	atomic_io_modify(wdt_reg + TIMER_CTRL, WDT_EN, 0);
+	atomic_io_modify(dev->reg + TIMER_CTRL, WDT_EN, 0);
 
-	spin_unlock(&wdt_lock);
+	spin_unlock(&dev->lock);
 	return 0;
 }
 
 static unsigned int orion_wdt_get_timeleft(struct watchdog_device *wdt_dev)
 {
+	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
 	unsigned int time_left;
 
-	spin_lock(&wdt_lock);
-	time_left = readl(wdt_reg + WDT_VAL) / wdt_tclk;
-	spin_unlock(&wdt_lock);
+	spin_lock(&dev->lock);
+	time_left = readl(dev->reg + WDT_VAL) / dev->clk_rate;
+	spin_unlock(&dev->lock);
 
 	return time_left;
 }
@@ -122,12 +131,6 @@ static const struct watchdog_ops orion_wdt_ops = {
 	.get_timeleft = orion_wdt_get_timeleft,
 };
 
-static struct watchdog_device orion_wdt = {
-	.info = &orion_wdt_info,
-	.ops = &orion_wdt_ops,
-	.min_timeout = 1,
-};
-
 static irqreturn_t orion_wdt_irq(int irq, void *devid)
 {
 	panic("Watchdog Timeout");
@@ -136,29 +139,42 @@ static irqreturn_t orion_wdt_irq(int irq, void *devid)
 
 static int orion_wdt_probe(struct platform_device *pdev)
 {
+	struct orion_watchdog *dev;
+	unsigned int wdt_max_duration;	/* (seconds) */
 	struct resource *res;
 	int ret, irq;
 
-	clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(clk)) {
+	dev = devm_kzalloc(&pdev->dev, sizeof(struct orion_watchdog),
+			   GFP_KERNEL);
+	if (!dev)
+		return -ENOMEM;
+
+	dev->wdt.info = &orion_wdt_info;
+	dev->wdt.ops = &orion_wdt_ops;
+	dev->wdt.min_timeout = 1;
+
+	dev->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(dev->clk)) {
 		dev_err(&pdev->dev, "Orion Watchdog missing clock\n");
 		return -ENODEV;
 	}
-	clk_prepare_enable(clk);
-	wdt_tclk = clk_get_rate(clk);
+	clk_prepare_enable(dev->clk);
+	dev->clk_rate = clk_get_rate(dev->clk);
+	spin_lock_init(&dev->lock);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res)
 		return -ENODEV;
-	wdt_reg = devm_ioremap(&pdev->dev, res->start, resource_size(res));
-	if (!wdt_reg)
+	dev->reg = devm_ioremap(&pdev->dev, res->start,
+			       resource_size(res));
+	if (!dev->reg)
 		return -ENOMEM;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
 	if (!res)
 		return -ENODEV;
-	wdt_rstout = devm_ioremap(&pdev->dev, res->start, resource_size(res));
-	if (!wdt_rstout)
+	dev->rstout = devm_ioremap(&pdev->dev, res->start, resource_size(res));
+	if (!dev->rstout)
 		return -ENOMEM;
 
 	irq = platform_get_irq(pdev, 0);
@@ -174,34 +190,40 @@ static int orion_wdt_probe(struct platform_device *pdev)
 		}
 	}
 
-	wdt_max_duration = WDT_MAX_CYCLE_COUNT / wdt_tclk;
+	wdt_max_duration = WDT_MAX_CYCLE_COUNT / dev->clk_rate;
 
-	orion_wdt.timeout = wdt_max_duration;
-	orion_wdt.max_timeout = wdt_max_duration;
-	watchdog_init_timeout(&orion_wdt, heartbeat, &pdev->dev);
+	dev->wdt.timeout = wdt_max_duration;
+	dev->wdt.max_timeout = wdt_max_duration;
+	watchdog_init_timeout(&dev->wdt, heartbeat, &pdev->dev);
 
-	watchdog_set_nowayout(&orion_wdt, nowayout);
-	ret = watchdog_register_device(&orion_wdt);
+	platform_set_drvdata(pdev, &dev->wdt);
+	watchdog_set_drvdata(&dev->wdt, dev);
+	watchdog_set_nowayout(&dev->wdt, nowayout);
+	ret = watchdog_register_device(&dev->wdt);
 	if (ret) {
-		clk_disable_unprepare(clk);
+		clk_disable_unprepare(dev->clk);
 		return ret;
 	}
 
 	pr_info("Initial timeout %d sec%s\n",
-		orion_wdt.timeout, nowayout ? ", nowayout" : "");
+		dev->wdt.timeout, nowayout ? ", nowayout" : "");
 	return 0;
 }
 
 static int orion_wdt_remove(struct platform_device *pdev)
 {
-	watchdog_unregister_device(&orion_wdt);
-	clk_disable_unprepare(clk);
+	struct watchdog_device *wdt_dev = platform_get_drvdata(pdev);
+	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
+
+	watchdog_unregister_device(wdt_dev);
+	clk_disable_unprepare(dev->clk);
 	return 0;
 }
 
 static void orion_wdt_shutdown(struct platform_device *pdev)
 {
-	orion_wdt_stop(&orion_wdt);
+	struct watchdog_device *wdt_dev = platform_get_drvdata(pdev);
+	orion_wdt_stop(wdt_dev);
 }
 
 static const struct of_device_id orion_wdt_of_match_table[] = {
-- 
1.8.1.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 08/15] watchdog: orion: Introduce per-compatible of_device_id data

[Ezequiel Garcia]

This commit adds an orion_watchdog_data structure to holda compatible-data
information. This allows to remove the driver-wide definition and to
future add support for multiple compatible-strings.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/watchdog/orion_wdt.c | 63 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 47 insertions(+), 16 deletions(-)

diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
index a95968c..a9632ba 100644
--- a/drivers/watchdog/orion_wdt.c
+++ b/drivers/watchdog/orion_wdt.c
@@ -25,23 +25,32 @@
 #include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 
 /*
  * Watchdog timer block registers.
  */
 #define TIMER_CTRL		0x0000
-#define WDT_EN			0x0010
-#define WDT_VAL			0x0024
 
 #define WDT_MAX_CYCLE_COUNT	0xffffffff
 #define WDT_IN_USE		0
 #define WDT_OK_TO_CLOSE		1
 
-#define WDT_RESET_OUT_EN	BIT(1)
+#define WDT_A370_RATIO_MASK(v)	((v) << 16)
+#define WDT_A370_RATIO_SHIFT	5
+#define WDT_A370_RATIO		(1 << WDT_A370_RATIO_SHIFT)
+
+#define WDT_AXP_FIXED_ENABLE_BIT BIT(10)
 
 static bool nowayout = WATCHDOG_NOWAYOUT;
 static int heartbeat = -1;		/* module parameter (seconds) */
 
+struct orion_watchdog_data {
+	int wdt_counter_offset;
+	int wdt_enable_bit;
+	int rstout_enable_bit;
+};
+
 struct orion_watchdog {
 	struct watchdog_device wdt;
 	void __iomem *reg;
@@ -49,6 +58,7 @@ struct orion_watchdog {
 	spinlock_t lock;
 	unsigned long clk_rate;
 	struct clk *clk;
+	struct orion_watchdog_data *data;
 };
 
 static int orion_wdt_ping(struct watchdog_device *wdt_dev)
@@ -57,7 +67,8 @@ static int orion_wdt_ping(struct watchdog_device *wdt_dev)
 	spin_lock(&dev->lock);
 
 	/* Reload watchdog duration */
-	writel(dev->clk_rate * wdt_dev->timeout, dev->reg + WDT_VAL);
+	writel(dev->clk_rate * wdt_dev->timeout,
+	       dev->reg + dev->data->wdt_counter_offset);
 
 	spin_unlock(&dev->lock);
 	return 0;
@@ -70,13 +81,16 @@ static int orion_wdt_start(struct watchdog_device *wdt_dev)
 	spin_lock(&dev->lock);
 
 	/* Set watchdog duration */
-	writel(dev->clk_rate * wdt_dev->timeout, dev->reg + WDT_VAL);
+	writel(dev->clk_rate * wdt_dev->timeout,
+	       dev->reg + dev->data->wdt_counter_offset);
 
 	/* Enable watchdog timer */
-	atomic_io_modify(dev->reg + TIMER_CTRL, WDT_EN, WDT_EN);
+	atomic_io_modify(dev->reg + TIMER_CTRL, dev->data->wdt_enable_bit,
+						dev->data->wdt_enable_bit);
 
 	/* Enable reset on watchdog */
-	atomic_io_modify(dev->rstout, WDT_RESET_OUT_EN, WDT_RESET_OUT_EN);
+	atomic_io_modify(dev->rstout, dev->data->rstout_enable_bit,
+				      dev->data->rstout_enable_bit);
 
 	spin_unlock(&dev->lock);
 	return 0;
@@ -89,10 +103,10 @@ static int orion_wdt_stop(struct watchdog_device *wdt_dev)
 	spin_lock(&dev->lock);
 
 	/* Disable reset on watchdog */
-	atomic_io_modify(dev->rstout, WDT_RESET_OUT_EN, 0);
+	atomic_io_modify(dev->rstout, dev->data->rstout_enable_bit, 0);
 
 	/* Disable watchdog timer */
-	atomic_io_modify(dev->reg + TIMER_CTRL, WDT_EN, 0);
+	atomic_io_modify(dev->reg + TIMER_CTRL, dev->data->wdt_enable_bit, 0);
 
 	spin_unlock(&dev->lock);
 	return 0;
@@ -104,7 +118,8 @@ static unsigned int orion_wdt_get_timeleft(struct watchdog_device *wdt_dev)
 	unsigned int time_left;
 
 	spin_lock(&dev->lock);
-	time_left = readl(dev->reg + WDT_VAL) / dev->clk_rate;
+	time_left = readl(dev->reg + dev->data->wdt_counter_offset);
+	time_left /= dev->clk_rate;
 	spin_unlock(&dev->lock);
 
 	return time_left;
@@ -137,9 +152,25 @@ static irqreturn_t orion_wdt_irq(int irq, void *devid)
 	return IRQ_HANDLED;
 }
 
+static const struct orion_watchdog_data orion_data = {
+	.rstout_enable_bit = BIT(1),
+	.wdt_enable_bit = BIT(4),
+	.wdt_counter_offset = 0x24,
+};
+
+static const struct of_device_id orion_wdt_of_match_table[] = {
+	{
+		.compatible = "marvell,orion-wdt",
+		.data = &orion_data,
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, orion_wdt_of_match_table);
+
 static int orion_wdt_probe(struct platform_device *pdev)
 {
 	struct orion_watchdog *dev;
+	const struct of_device_id *match;
 	unsigned int wdt_max_duration;	/* (seconds) */
 	struct resource *res;
 	int ret, irq;
@@ -149,9 +180,15 @@ static int orion_wdt_probe(struct platform_device *pdev)
 	if (!dev)
 		return -ENOMEM;
 
+	match = of_match_device(orion_wdt_of_match_table, &pdev->dev);
+	if (!match)
+		/* Default legacy match */
+		match = &orion_wdt_of_match_table[0];
+
 	dev->wdt.info = &orion_wdt_info;
 	dev->wdt.ops = &orion_wdt_ops;
 	dev->wdt.min_timeout = 1;
+	dev->data = (struct orion_watchdog_data *)match->data;
 
 	dev->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(dev->clk)) {
@@ -226,12 +263,6 @@ static void orion_wdt_shutdown(struct platform_device *pdev)
 	orion_wdt_stop(wdt_dev);
 }
 
-static const struct of_device_id orion_wdt_of_match_table[] = {
-	{ .compatible = "marvell,orion-wdt", },
-	{},
-};
-MODULE_DEVICE_TABLE(of, orion_wdt_of_match_table);
-
 static struct platform_driver orion_wdt_driver = {
 	.probe		= orion_wdt_probe,
 	.remove		= orion_wdt_remove,
-- 
1.8.1.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 09/15] watchdog: orion: Add per-compatible clock initialization

[Ezequiel Garcia]

Following the introduction of the compatible-data field,
it's now possible to further abstract the clock initialization.
This will allow to support SoC with a different clock setup.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/watchdog/orion_wdt.c | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
index a9632ba..e94bb97 100644
--- a/drivers/watchdog/orion_wdt.c
+++ b/drivers/watchdog/orion_wdt.c
@@ -45,10 +45,14 @@
 static bool nowayout = WATCHDOG_NOWAYOUT;
 static int heartbeat = -1;		/* module parameter (seconds) */
 
+struct orion_watchdog;
+
 struct orion_watchdog_data {
 	int wdt_counter_offset;
 	int wdt_enable_bit;
 	int rstout_enable_bit;
+	int (*clock_init) (struct platform_device *,
+			   struct orion_watchdog *);
 };
 
 struct orion_watchdog {
@@ -61,6 +65,17 @@ struct orion_watchdog {
 	struct orion_watchdog_data *data;
 };
 
+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;
+	clk_prepare_enable(dev->clk);
+	dev->clk_rate = clk_get_rate(dev->clk);
+	return 0;
+}
+
 static int orion_wdt_ping(struct watchdog_device *wdt_dev)
 {
 	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
@@ -156,6 +171,7 @@ static const struct orion_watchdog_data orion_data = {
 	.rstout_enable_bit = BIT(1),
 	.wdt_enable_bit = BIT(4),
 	.wdt_counter_offset = 0x24,
+	.clock_init = orion_wdt_clock_init,
 };
 
 static const struct of_device_id orion_wdt_of_match_table[] = {
@@ -190,13 +206,6 @@ static int orion_wdt_probe(struct platform_device *pdev)
 	dev->wdt.min_timeout = 1;
 	dev->data = (struct orion_watchdog_data *)match->data;
 
-	dev->clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(dev->clk)) {
-		dev_err(&pdev->dev, "Orion Watchdog missing clock\n");
-		return -ENODEV;
-	}
-	clk_prepare_enable(dev->clk);
-	dev->clk_rate = clk_get_rate(dev->clk);
 	spin_lock_init(&dev->lock);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -214,6 +223,12 @@ static int orion_wdt_probe(struct platform_device *pdev)
 	if (!dev->rstout)
 		return -ENOMEM;
 
+	ret = dev->data->clock_init(pdev, dev);
+	if (ret) {
+		dev_err(&pdev->dev, "cannot initialize clock\n");
+		return ret;
+	}
+
 	irq = platform_get_irq(pdev, 0);
 	if (irq > 0) {
 		/*
-- 
1.8.1.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 09/15] watchdog: orion: Add per-compatible clock initialization

[Fabio Estevam]

On Tue, Jan 21, 2014 at 7:12 AM, Ezequiel Garcia
<ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:

> +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?


> +       clk_prepare_enable(dev->clk);

clk_prepare_enable may fail, so better check its return value.

Regards,

Fabio Estevam
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 09/15] watchdog: orion: Add per-compatible clock initialization

[Ezequiel Garcia]

On Tue, Jan 21, 2014 at 07:18:44AM -0200, Fabio Estevam wrote:
> On Tue, Jan 21, 2014 at 7:12 AM, Ezequiel Garcia
> <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> 
> > +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?
> 
> 
[..]
> > +       clk_prepare_enable(dev->clk);
> 
> clk_prepare_enable may fail, so better check its return value.
> 

Indeed.

Thanks!
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 10/15] watchdog: orion: Add per-compatible watchdog start implementation

[Ezequiel Garcia]

To handle differences between SoCs this commit adds per-compatible
string start() function for the watchdog kick-off. This is preparation
work and makes no functionality changes to the current driver.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/watchdog/orion_wdt.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
index e94bb97..d6009fe 100644
--- a/drivers/watchdog/orion_wdt.c
+++ b/drivers/watchdog/orion_wdt.c
@@ -53,6 +53,7 @@ struct orion_watchdog_data {
 	int rstout_enable_bit;
 	int (*clock_init) (struct platform_device *,
 			   struct orion_watchdog *);
+	int (*start) (struct watchdog_device *);
 };
 
 struct orion_watchdog {
@@ -89,7 +90,7 @@ static int orion_wdt_ping(struct watchdog_device *wdt_dev)
 	return 0;
 }
 
-static int orion_wdt_start(struct watchdog_device *wdt_dev)
+static int orion_start(struct watchdog_device *wdt_dev)
 {
 	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
 
@@ -111,6 +112,14 @@ static int orion_wdt_start(struct watchdog_device *wdt_dev)
 	return 0;
 }
 
+static int orion_wdt_start(struct watchdog_device *wdt_dev)
+{
+	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
+
+	/* There are some per-SoC quirks to handle */
+	return dev->data->start(wdt_dev);
+}
+
 static int orion_wdt_stop(struct watchdog_device *wdt_dev)
 {
 	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
@@ -172,6 +181,7 @@ static const struct orion_watchdog_data orion_data = {
 	.wdt_enable_bit = BIT(4),
 	.wdt_counter_offset = 0x24,
 	.clock_init = orion_wdt_clock_init,
+	.start = orion_start,
 };
 
 static const struct of_device_id orion_wdt_of_match_table[] = {
-- 
1.8.1.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 11/15] watchdog: orion: Add support for Armada 370 and Armada XP SoC

[Ezequiel Garcia]

Using the added infrastructure for handling SoC differences,
this commit adds support for the watchdog controller available
in Armada 370 and Armada XP SoCs.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/watchdog/orion_wdt.c | 89 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 89 insertions(+)

diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
index d6009fe..8631756 100644
--- a/drivers/watchdog/orion_wdt.c
+++ b/drivers/watchdog/orion_wdt.c
@@ -31,6 +31,7 @@
  * Watchdog timer block registers.
  */
 #define TIMER_CTRL		0x0000
+#define TIMER_A370_STATUS	0x04
 
 #define WDT_MAX_CYCLE_COUNT	0xffffffff
 #define WDT_IN_USE		0
@@ -42,6 +43,12 @@
 
 #define WDT_AXP_FIXED_ENABLE_BIT BIT(10)
 
+#define WDT_A370_RATIO_MASK(v)	((v) << 16)
+#define WDT_A370_RATIO_SHIFT	5
+#define WDT_A370_RATIO		(1 << WDT_A370_RATIO_SHIFT)
+
+#define WDT_A370_EXPIRED	BIT(31)
+
 static bool nowayout = WATCHDOG_NOWAYOUT;
 static int heartbeat = -1;		/* module parameter (seconds) */
 
@@ -77,6 +84,40 @@ static int orion_wdt_clock_init(struct platform_device *pdev,
 	return 0;
 }
 
+static int armada370_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;
+	clk_prepare_enable(dev->clk);
+
+	/* Setup watchdog input clock */
+	atomic_io_modify(dev->reg + TIMER_CTRL,
+			WDT_A370_RATIO_MASK(WDT_A370_RATIO_SHIFT),
+			WDT_A370_RATIO_MASK(WDT_A370_RATIO_SHIFT));
+
+	dev->clk_rate = clk_get_rate(dev->clk) / WDT_A370_RATIO;
+	return 0;
+}
+
+static int armadaxp_wdt_clock_init(struct platform_device *pdev,
+				   struct orion_watchdog *dev)
+{
+	dev->clk = of_clk_get_by_name(pdev->dev.of_node, "fixed");
+	if (IS_ERR(dev->clk))
+		return -ENODEV;
+	clk_prepare_enable(dev->clk);
+
+	/* Enable the fixed watchdog clock input */
+	atomic_io_modify(dev->reg + TIMER_CTRL,
+		WDT_AXP_FIXED_ENABLE_BIT,
+		WDT_AXP_FIXED_ENABLE_BIT);
+
+	dev->clk_rate = clk_get_rate(dev->clk);
+	return 0;
+}
+
 static int orion_wdt_ping(struct watchdog_device *wdt_dev)
 {
 	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
@@ -90,6 +131,30 @@ static int orion_wdt_ping(struct watchdog_device *wdt_dev)
 	return 0;
 }
 
+static int armada370_start(struct watchdog_device *wdt_dev)
+{
+	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
+
+	spin_lock(&dev->lock);
+
+	/* Set watchdog duration */
+	writel(dev->clk_rate * wdt_dev->timeout,
+	       dev->reg + dev->data->wdt_counter_offset);
+
+	/* Clear the watchdog expiration bit */
+	atomic_io_modify(dev->reg + TIMER_A370_STATUS, WDT_A370_EXPIRED, 0);
+
+	/* Enable watchdog timer */
+	atomic_io_modify(dev->reg + TIMER_CTRL, dev->data->wdt_enable_bit,
+						dev->data->wdt_enable_bit);
+
+	atomic_io_modify(dev->rstout, dev->data->rstout_enable_bit,
+				      dev->data->rstout_enable_bit);
+
+	spin_unlock(&dev->lock);
+	return 0;
+}
+
 static int orion_start(struct watchdog_device *wdt_dev)
 {
 	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
@@ -184,11 +249,35 @@ static const struct orion_watchdog_data orion_data = {
 	.start = orion_start,
 };
 
+static const struct orion_watchdog_data armada370_data = {
+	.rstout_enable_bit = BIT(8),
+	.wdt_enable_bit = BIT(8),
+	.wdt_counter_offset = 0x34,
+	.clock_init = armada370_wdt_clock_init,
+	.start = armada370_start,
+};
+
+static const struct orion_watchdog_data armadaxp_data = {
+	.rstout_enable_bit = BIT(8),
+	.wdt_enable_bit = BIT(8),
+	.wdt_counter_offset = 0x34,
+	.clock_init = armadaxp_wdt_clock_init,
+	.start = armada370_start,
+};
+
 static const struct of_device_id orion_wdt_of_match_table[] = {
 	{
 		.compatible = "marvell,orion-wdt",
 		.data = &orion_data,
 	},
+	{
+		.compatible = "marvell,armada-370-wdt",
+		.data = &armada370_data,
+	},
+	{
+		.compatible = "marvell,armada-xp-wdt",
+		.data = &armadaxp_data,
+	},
 	{},
 };
 MODULE_DEVICE_TABLE(of, orion_wdt_of_match_table);
-- 
1.8.1.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 12/15] ARM: mvebu: Enable Armada 370/XP watchdog in the devicetree

[Ezequiel Garcia]

Add the DT nodes to enable watchdog support available in Armada 370
and Armada XP SoCs.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 arch/arm/boot/dts/armada-370-xp.dtsi | 4 ++++
 arch/arm/boot/dts/armada-370.dtsi    | 5 +++++
 arch/arm/boot/dts/armada-xp.dtsi     | 6 ++++++
 3 files changed, 15 insertions(+)

diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
index 7f10f62..96e0389 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -151,6 +151,10 @@
 				interrupts = <37>, <38>, <39>, <40>, <5>, <6>;
 			};
 
+			watchdog@20300 {
+				reg = <0x20300 0x34>, <0x20704 0x4>;
+			};
+
 			sata@a0000 {
 				compatible = "marvell,orion-sata";
 				reg = <0xa0000 0x5000>;
diff --git a/arch/arm/boot/dts/armada-370.dtsi b/arch/arm/boot/dts/armada-370.dtsi
index 7a4b82e..aebed9e 100644
--- a/arch/arm/boot/dts/armada-370.dtsi
+++ b/arch/arm/boot/dts/armada-370.dtsi
@@ -168,6 +168,11 @@
 				clocks = <&coreclk 2>;
 			};
 
+			watchdog@20300 {
+				compatible = "marvell,armada-370-wdt";
+				clocks = <&coreclk 2>;
+			};
+
 			coreclk: mvebu-sar@18230 {
 				compatible = "marvell,armada-370-core-clock";
 				reg = <0x18230 0x08>;
diff --git a/arch/arm/boot/dts/armada-xp.dtsi b/arch/arm/boot/dts/armada-xp.dtsi
index 281c644..8c6c06c 100644
--- a/arch/arm/boot/dts/armada-xp.dtsi
+++ b/arch/arm/boot/dts/armada-xp.dtsi
@@ -74,6 +74,12 @@
 				clock-names = "nbclk", "fixed";
 			};
 
+			watchdog@20300 {
+				compatible = "marvell,armada-xp-wdt";
+				clocks = <&coreclk 2>, <&refclk>;
+				clock-names = "nbclk", "fixed";
+			};
+
 			coreclk: mvebu-sar@18230 {
 				compatible = "marvell,armada-xp-core-clock";
 				reg = <0x18230 0x08>;
-- 
1.8.1.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 13/15] ARM: kirkwood: Add RSTOUT 'reg' entry to devicetree

[Ezequiel Garcia]

In order to support multiplatform builds the watchdog devicetree binding
was modified and now the 'reg' property is specified to need two
entries. This commit adds the second entry as-per the new specification.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 arch/arm/boot/dts/kirkwood.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/kirkwood.dtsi b/arch/arm/boot/dts/kirkwood.dtsi
index 8b73c80..80a56b0 100644
--- a/arch/arm/boot/dts/kirkwood.dtsi
+++ b/arch/arm/boot/dts/kirkwood.dtsi
@@ -165,7 +165,7 @@
 
 		wdt: watchdog-timer@20300 {
 			compatible = "marvell,orion-wdt";
-			reg = <0x20300 0x28>;
+			reg = <0x20300 0x28>, <0x20108 0x4>;
 			interrupt-parent = <&bridge_intc>;
 			interrupts = <3>;
 			clocks = <&gate_clk 7>;
-- 
1.8.1.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 14/15] watchdog: orion: Allow to build on any Orion platform

[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 <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/watchdog/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 5be6e91..1689f72 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -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
 	select WATCHDOG_CORE
 	help
 	  Say Y here if to include support for the watchdog timer
-- 
1.8.1.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 14/15] watchdog: orion: Allow to build on any Orion platform

[Sebastian Hesselbarth]

On 01/21/14 10:12, Ezequiel Garcia wrote:
> 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 <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>   drivers/watchdog/Kconfig | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 5be6e91..1689f72 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -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.

Sebastian

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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 14/15] watchdog: orion: Allow to build on any Orion platform

[Andrew Lunn]

On Tue, Jan 21, 2014 at 10:41:17AM +0100, Sebastian Hesselbarth wrote:
> On 01/21/14 10:12, Ezequiel Garcia wrote:
> >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 <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> >---
> >  drivers/watchdog/Kconfig | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> >index 5be6e91..1689f72 100644
> >--- a/drivers/watchdog/Kconfig
> >+++ b/drivers/watchdog/Kconfig
> >@@ -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.

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.

	   Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 14/15] watchdog: orion: Allow to build on any Orion platform

[Ezequiel Garcia]

Hi Andrew, Sebastian:

On Tue, Jan 21, 2014 at 10:45:01AM +0100, Andrew Lunn wrote:
> On Tue, Jan 21, 2014 at 10:41:17AM +0100, Sebastian Hesselbarth wrote:
> > On 01/21/14 10:12, Ezequiel Garcia wrote:
> > >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 <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > >---
> > >  drivers/watchdog/Kconfig | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > >diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > >index 5be6e91..1689f72 100644
> > >--- a/drivers/watchdog/Kconfig
> > >+++ b/drivers/watchdog/Kconfig
> > >@@ -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
[..]
> > 
> > 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.
> 

Hm... not sure why we depend on PLAT_ORION. I'll take a look.

> 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.
> 

OK, will do.

Thanks!
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Removing PLAT_ORION dependency from ARCH_MVEBU (Was Re: [PATCH v2 14/15] watchdog: orion: Allow to build on any Orion platform)

[Ezequiel Garcia]

Sebastian,

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

On Tue, Jan 21, 2014 at 10:41:17AM +0100, Sebastian Hesselbarth wrote:
> On 01/21/14 10:12, Ezequiel Garcia wrote:
> > 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 <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > ---
> >   drivers/watchdog/Kconfig | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index 5be6e91..1689f72 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -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.
> 

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?
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Removing PLAT_ORION dependency from ARCH_MVEBU (Was Re: [PATCH v2 14/15] watchdog: orion: Allow to build on any Orion platform)

[Sebastian Hesselbarth]

On 01/21/14 12:06, Ezequiel Garcia wrote:
> Sebastian,
>
> I'm picking on this discussion, with a new Subject.
>
> On Tue, Jan 21, 2014 at 10:41:17AM +0100, Sebastian Hesselbarth wrote:
>> On 01/21/14 10:12, Ezequiel Garcia wrote:
>>> 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 <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>>> ---
>>>    drivers/watchdog/Kconfig | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>> index 5be6e91..1689f72 100644
>>> --- a/drivers/watchdog/Kconfig
>>> +++ b/drivers/watchdog/Kconfig
>>> @@ -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.
>>
>
> 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?

Actually, you are right, PLAT_ORION is dependency for a bunch of
drivers. But as PLAT_ORION - code-wise - almost only contains non-DT
stuff, I'd prefer to use ARCH_MVEBU for DT enabled Marvell SoCs where
possible.

So for the above, just add ARCH_MVEBU and remove ARCH_KIRKWOOD,
ARCH_DOVE later when we kill them, instead of hiding all behind
PLAT_ORION. Also remove the dependency of ARCH_MVEBU to PLAT_ORION
where possible or replace it with ARCH_MVEBU directly.

This way we would have a clean ARCH_MVEBU config for DT-enabled
SoCs and PLAT_ORION for non-DT ones. BTW, there is also
PLAT_ORION_LEGACY which is just selecting PLAT_ORION. IIRC, I added
that to distinguish non-DT and DT earlier, but ARCH_MVEBU selecting
PLAT_ORION made that redundant.

Sebastian

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 15/15] ARM: mvebu: Enable watchdog support in defconfig

[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 <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 arch/arm/configs/mvebu_defconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/configs/mvebu_defconfig b/arch/arm/configs/mvebu_defconfig
index 594d706..84ec924 100644
--- a/arch/arm/configs/mvebu_defconfig
+++ b/arch/arm/configs/mvebu_defconfig
@@ -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
-- 
1.8.1.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 15/15] ARM: mvebu: Enable watchdog support in defconfig

[Jason Cooper]

On Tue, Jan 21, 2014 at 06:12:41AM -0300, Ezequiel Garcia wrote:
> Now that we have proper support for Armada 370/XP watchdog
> let's enable it in the defconfig.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  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.

Hopefully I can get some defconfig changes in for -rc1? :)

thx,

Jason.

> diff --git a/arch/arm/configs/mvebu_defconfig b/arch/arm/configs/mvebu_defconfig
> index 594d706..84ec924 100644
> --- a/arch/arm/configs/mvebu_defconfig
> +++ b/arch/arm/configs/mvebu_defconfig
> @@ -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
> -- 
> 1.8.1.5
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 15/15] ARM: mvebu: Enable watchdog support in defconfig

[Andrew Lunn]

On Tue, Jan 21, 2014 at 07:47:46AM -0500, Jason Cooper wrote:
> On Tue, Jan 21, 2014 at 06:12:41AM -0300, Ezequiel Garcia wrote:
> > Now that we have proper support for Armada 370/XP watchdog
> > let's enable it in the defconfig.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > ---
> >  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.

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.

    Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 15/15] ARM: mvebu: Enable watchdog support in defconfig

[Jason Cooper]

On Tue, Jan 21, 2014 at 01:53:39PM +0100, Andrew Lunn wrote:
> On Tue, Jan 21, 2014 at 07:47:46AM -0500, Jason Cooper wrote:
> > On Tue, Jan 21, 2014 at 06:12:41AM -0300, Ezequiel Garcia wrote:
> > > Now that we have proper support for Armada 370/XP watchdog
> > > let's enable it in the defconfig.
> > > 
> > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > > ---
> > >  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.
> 
> Hi Jason
> 
> Hopefully you can also tackle multi_v5 as well, once DT kirkwood moves
> into mach-mvebu.

Yes.

> 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.

Yes, I was more specifically looking to leverage arm-buildbot...

thx,

Jason.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html