[PATCH 0/7] Orion watchdog DT changes to support more SoCs

[PATCH 0/7] Orion watchdog DT changes to support more SoCs

[Ezequiel Garcia]

As part of my work to add watchdog support to Armada 370/XP SoCs,
here's some early patches to get early feedback.

This patchset allows to build the orion_wdt driver in any Orion
platforms.

To achieve this it's necessary to remove the need for a mach-specific,
which in turn is done by splitting the single memory resource into
three memory resources: timer control, watchdog counter and RSTOUT registers;
and handling each of this resources as independent memory resources (as they
really are).

The only drawback with this approach is the breakage of devicetree backwards
compatibility such change produces. This is an important issue, and the main
reason I'm submitting this patchset: Is it possible to introduce such
DT-binding change?

It's worth noting that a similar was proposed in July-15th [1], which received
some acceptance [2].

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/183628.html
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/183857.html

Thanks!

Ezequiel Garcia (7):
  watchdog: orion: Remove unneeded BRIDGE_CAUSE clear
  watchdog: orion: Make counter register a separate resource
  watchdog: orion: Make RSTOUT register a separate resource
  watchdog: orion: Allow to build in any Orion platform
  ARM: kirkwood: Update watchdog 'reg' property
  watchdog: orion: Rename device-tree binding documentation
  watchdog: orion: Update device-tree binding documentation

 .../devicetree/bindings/watchdog/marvel.txt        | 19 ------------
 .../devicetree/bindings/watchdog/orion-wdt.txt     | 24 +++++++++++++++
 arch/arm/boot/dts/kirkwood.dtsi                    |  4 ++-
 arch/arm/mach-kirkwood/include/mach/bridge-regs.h  |  1 +
 arch/arm/plat-orion/common.c                       | 11 ++++---
 drivers/watchdog/Kconfig                           |  2 +-
 drivers/watchdog/orion_wdt.c                       | 35 ++++++++++++++--------
 7 files changed, 59 insertions(+), 37 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/watchdog/marvel.txt
 create mode 100644 Documentation/devicetree/bindings/watchdog/orion-wdt.txt

-- 
1.8.1.5

[PATCH 1/7] watchdog: orion: Remove unneeded BRIDGE_CAUSE clear

[Ezequiel Garcia]

With the introduction of the orion irqchip driver, now the BRIDGE_CAUSE
bit is cleared by it. 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@free-electrons.com>
---
 drivers/watchdog/orion_wdt.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
index 4ea5fcc..d43a3da 100644
--- a/drivers/watchdog/orion_wdt.c
+++ b/drivers/watchdog/orion_wdt.c
@@ -69,9 +69,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 */
 	reg = readl(wdt_reg + TIMER_CTRL);
 	reg |= WDT_EN;
-- 
1.8.1.5

[PATCH 2/7] watchdog: orion: Make counter register a separate resource

[Ezequiel Garcia]

In order to support other SoC, it's required to distinguish
the 'control' timer register, from the 'counter' watchdog register
as different memory resources.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/mach-kirkwood/include/mach/bridge-regs.h |  1 +
 arch/arm/plat-orion/common.c                      | 10 ++++++----
 drivers/watchdog/orion_wdt.c                      | 15 +++++++++++----
 3 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mach-kirkwood/include/mach/bridge-regs.h b/arch/arm/mach-kirkwood/include/mach/bridge-regs.h
index 91242c9..05a1164 100644
--- a/arch/arm/mach-kirkwood/include/mach/bridge-regs.h
+++ b/arch/arm/mach-kirkwood/include/mach/bridge-regs.h
@@ -38,6 +38,7 @@
 
 #define TIMER_VIRT_BASE		(BRIDGE_VIRT_BASE + 0x0300)
 #define TIMER_PHYS_BASE		(BRIDGE_PHYS_BASE + 0x0300)
+#define WDT_COUNTER_OFF		0x24
 
 #define L2_CONFIG_REG		(BRIDGE_VIRT_BASE + 0x0128)
 #define L2_WRITETHROUGH		0x00000010
diff --git a/arch/arm/plat-orion/common.c b/arch/arm/plat-orion/common.c
index c66d163..4e30ed6 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(TIMER_PHYS_BASE + WDT_COUNTER_OFF, 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 d43a3da..739e79c 100644
--- a/drivers/watchdog/orion_wdt.c
+++ b/drivers/watchdog/orion_wdt.c
@@ -32,7 +32,6 @@
  */
 #define TIMER_CTRL		0x0000
 #define WDT_EN			0x0010
-#define WDT_VAL			0x0024
 
 #define WDT_MAX_CYCLE_COUNT	0xffffffff
 #define WDT_IN_USE		0
@@ -47,6 +46,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_val;
 static DEFINE_SPINLOCK(wdt_lock);
 
 static int orion_wdt_ping(struct watchdog_device *wdt_dev)
@@ -54,7 +54,7 @@ static int orion_wdt_ping(struct watchdog_device *wdt_dev)
 	spin_lock(&wdt_lock);
 
 	/* Reload watchdog duration */
-	writel(wdt_tclk * wdt_dev->timeout, wdt_reg + WDT_VAL);
+	writel(wdt_tclk * wdt_dev->timeout, wdt_val);
 
 	spin_unlock(&wdt_lock);
 	return 0;
@@ -67,7 +67,7 @@ static int orion_wdt_start(struct watchdog_device *wdt_dev)
 	spin_lock(&wdt_lock);
 
 	/* Set watchdog duration */
-	writel(wdt_tclk * wdt_dev->timeout, wdt_reg + WDT_VAL);
+	writel(wdt_tclk * wdt_dev->timeout, wdt_val);
 
 	/* Enable watchdog timer */
 	reg = readl(wdt_reg + TIMER_CTRL);
@@ -108,7 +108,7 @@ static unsigned int orion_wdt_get_timeleft(struct watchdog_device *wdt_dev)
 	unsigned int time_left;
 
 	spin_lock(&wdt_lock);
-	time_left = readl(wdt_reg + WDT_VAL) / wdt_tclk;
+	time_left = readl(wdt_val) / wdt_tclk;
 	spin_unlock(&wdt_lock);
 
 	return time_left;
@@ -161,6 +161,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_val = devm_ioremap(&pdev->dev, res->start, resource_size(res));
+	if (!wdt_val)
+		return -ENOMEM;
+
 	wdt_max_duration = WDT_MAX_CYCLE_COUNT / wdt_tclk;
 
 	orion_wdt.timeout = wdt_max_duration;
-- 
1.8.1.5

[PATCH 3/7] 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@free-electrons.com>
---
 arch/arm/plat-orion/common.c |  1 +
 drivers/watchdog/orion_wdt.c | 16 ++++++++++++----
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/arch/arm/plat-orion/common.c b/arch/arm/plat-orion/common.c
index 4e30ed6..860db39 100644
--- a/arch/arm/plat-orion/common.c
+++ b/arch/arm/plat-orion/common.c
@@ -597,6 +597,7 @@ void __init orion_spi_1_init(unsigned long mapbase)
 static struct resource orion_wdt_resource[] = {
 		DEFINE_RES_MEM(TIMER_PHYS_BASE, 0x04),
 		DEFINE_RES_MEM(TIMER_PHYS_BASE + WDT_COUNTER_OFF, 0x04),
+		DEFINE_RES_MEM(RSTOUTn_MASK, 0x04),
 };
 
 static struct platform_device orion_wdt_device = {
diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
index 739e79c..b4fd0a9 100644
--- a/drivers/watchdog/orion_wdt.c
+++ b/drivers/watchdog/orion_wdt.c
@@ -47,6 +47,7 @@ static struct clk *clk;
 static unsigned int wdt_tclk;
 static void __iomem *wdt_reg;
 static void __iomem *wdt_val;
+static void __iomem *wdt_rstout;
 static DEFINE_SPINLOCK(wdt_lock);
 
 static int orion_wdt_ping(struct watchdog_device *wdt_dev)
@@ -75,9 +76,9 @@ static int orion_wdt_start(struct watchdog_device *wdt_dev)
 	writel(reg, wdt_reg + TIMER_CTRL);
 
 	/* Enable reset on watchdog */
-	reg = readl(RSTOUTn_MASK);
+	reg = readl(wdt_rstout);
 	reg |= WDT_RESET_OUT_EN;
-	writel(reg, RSTOUTn_MASK);
+	writel(reg, wdt_rstout);
 
 	spin_unlock(&wdt_lock);
 	return 0;
@@ -90,9 +91,9 @@ static int orion_wdt_stop(struct watchdog_device *wdt_dev)
 	spin_lock(&wdt_lock);
 
 	/* Disable reset on watchdog */
-	reg = readl(RSTOUTn_MASK);
+	reg = readl(wdt_rstout);
 	reg &= ~WDT_RESET_OUT_EN;
-	writel(reg, RSTOUTn_MASK);
+	writel(reg, wdt_rstout);
 
 	/* Disable watchdog timer */
 	reg = readl(wdt_reg + TIMER_CTRL);
@@ -168,6 +169,13 @@ static int orion_wdt_probe(struct platform_device *pdev)
 	if (!wdt_val)
 		return -ENOMEM;
 
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
+	if (!res)
+		return -ENODEV;
+	wdt_rstout = devm_ioremap(&pdev->dev, res->start, resource_size(res));
+	if (!wdt_rstout)
+		return -ENOMEM;
+
 	wdt_max_duration = WDT_MAX_CYCLE_COUNT / wdt_tclk;
 
 	orion_wdt.timeout = wdt_max_duration;
-- 
1.8.1.5

[PATCH 4/7] watchdog: orion: Allow to build in any Orion platform

[Ezequiel Garcia]

This driver has no need for the mach-specific header, so remove it.
Therefore, it's no possible to allow builds in any Orion platform.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/watchdog/Kconfig     | 2 +-
 drivers/watchdog/orion_wdt.c | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 362085d..524f30a 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
diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
index b4fd0a9..04ceb0e 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.
-- 
1.8.1.5

[PATCH 5/7] ARM: kirkwood: Update watchdog 'reg' property

[Ezequiel Garcia]

According to the new DT binding, the 'reg' property requires
three cells, which mean: timer control register, watchdog counter
register, and rstout register, respectively.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/boot/dts/kirkwood.dtsi | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/kirkwood.dtsi b/arch/arm/boot/dts/kirkwood.dtsi
index 9809fc1..5fba37b 100644
--- a/arch/arm/boot/dts/kirkwood.dtsi
+++ b/arch/arm/boot/dts/kirkwood.dtsi
@@ -105,7 +105,9 @@
 
 		wdt at 20300 {
 			compatible = "marvell,orion-wdt";
-			reg = <0x20300 0x28>;
+			reg = <0x20300 0x4>, /* Timer control */
+			      <0x20324 0x4>, /* Watchdog counter */
+			      <0x20108 0x4>; /* RSTOUT */
 			clocks = <&gate_clk 7>;
 			status = "okay";
 		};
-- 
1.8.1.5

[PATCH 6/7] watchdog: orion: Rename device-tree binding documentation

[Ezequiel Garcia]

Name this file to something a bit more judicious.

Cc: devicetree at vger.kernel.org
Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 Documentation/devicetree/bindings/watchdog/{marvel.txt => orion-wdt.txt} | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 rename Documentation/devicetree/bindings/watchdog/{marvel.txt => orion-wdt.txt} (100%)

diff --git a/Documentation/devicetree/bindings/watchdog/marvel.txt b/Documentation/devicetree/bindings/watchdog/orion-wdt.txt
similarity index 100%
rename from Documentation/devicetree/bindings/watchdog/marvel.txt
rename to Documentation/devicetree/bindings/watchdog/orion-wdt.txt
-- 
1.8.1.5

[PATCH 7/7] watchdog: orion: Update device-tree binding documentation

[Ezequiel Garcia]

Change the 'reg' property meaning, by defining three required cells.
It's important to note this commit breaks DT-compatibility for this
device.

Cc: devicetree at vger.kernel.org
Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 Documentation/devicetree/bindings/watchdog/orion-wdt.txt | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/watchdog/orion-wdt.txt b/Documentation/devicetree/bindings/watchdog/orion-wdt.txt
index 5dc8d30..bb7f1a2 100644
--- a/Documentation/devicetree/bindings/watchdog/orion-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/orion-wdt.txt
@@ -3,7 +3,10 @@
 Required Properties:
 
 - Compatibility : "marvell,orion-wdt"
-- reg		: Address of the timer registers
+- reg		: Three cells are required.
+		  First  cell contains the global timer control register.
+		  Second cell contains the watchdog counter register.
+		  Third  cell contains the RSTOUT register.
 
 Optional properties:
 
@@ -13,7 +16,9 @@ Example:
 
 	wdt at 20300 {
 		compatible = "marvell,orion-wdt";
-		reg = <0x20300 0x28>;
+		reg = <0x20300 0x4
+		       0x20324 0x4
+		       0x20108 0x4>;
 		timeout-sec = <10>;
 		status = "okay";
 	};
-- 
1.8.1.5

[PATCH 0/7] Orion watchdog DT changes to support more SoCs

[Jason Cooper]

On Thu, Aug 22, 2013 at 11:41:51AM -0300, Ezequiel Garcia wrote:
> As part of my work to add watchdog support to Armada 370/XP SoCs,
> here's some early patches to get early feedback.
> 
> This patchset allows to build the orion_wdt driver in any Orion
> platforms.
> 
> To achieve this it's necessary to remove the need for a mach-specific,
> which in turn is done by splitting the single memory resource into
> three memory resources: timer control, watchdog counter and RSTOUT registers;
> and handling each of this resources as independent memory resources (as they
> really are).
> 
> The only drawback with this approach is the breakage of devicetree backwards
> compatibility such change produces. This is an important issue, and the main
> reason I'm submitting this patchset: Is it possible to introduce such
> DT-binding change?
> 
> It's worth noting that a similar was proposed in July-15th [1], which received
> some acceptance [2].
> 
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/183628.html
> [2] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/183857.html

Well, my point was that *if* it needed to be broken, now was the best
time to do it.

Please see my other reply for current thoughts.

thx,

Jason.

[PATCH 7/7] watchdog: orion: Update device-tree binding documentation

[Jason Cooper]

On Thu, Aug 22, 2013 at 11:41:58AM -0300, Ezequiel Garcia wrote:
> Change the 'reg' property meaning, by defining three required cells.
> It's important to note this commit breaks DT-compatibility for this
> device.
> 
> Cc: devicetree at vger.kernel.org
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
>  Documentation/devicetree/bindings/watchdog/orion-wdt.txt | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/orion-wdt.txt b/Documentation/devicetree/bindings/watchdog/orion-wdt.txt
> index 5dc8d30..bb7f1a2 100644
> --- a/Documentation/devicetree/bindings/watchdog/orion-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/orion-wdt.txt
> @@ -3,7 +3,10 @@
>  Required Properties:
>  
>  - Compatibility : "marvell,orion-wdt"
> -- reg		: Address of the timer registers
> +- reg		: Three cells are required.
> +		  First  cell contains the global timer control register.
> +		  Second cell contains the watchdog counter register.
> +		  Third  cell contains the RSTOUT register.
>  
>  Optional properties:
>  
> @@ -13,7 +16,9 @@ Example:
>  
>  	wdt at 20300 {
>  		compatible = "marvell,orion-wdt";
> -		reg = <0x20300 0x28>;
> +		reg = <0x20300 0x4
> +		       0x20324 0x4
> +		       0x20108 0x4>;

I don't like this.  It reaches outside of the wdt register.  I think a
more clean way to do this is to do a provider/consumer relationship as
in reset.txt.  eg, here you would retain the original reg binding, and
add a reset phandle.

thx,

Jason.

[PATCH 7/7] watchdog: orion: Update device-tree binding documentation

[Ezequiel Garcia]

On Thu, Aug 22, 2013 at 09:26:21PM -0400, Jason Cooper wrote:
> On Thu, Aug 22, 2013 at 11:41:58AM -0300, Ezequiel Garcia wrote:
> > Change the 'reg' property meaning, by defining three required cells.
> > It's important to note this commit breaks DT-compatibility for this
> > device.
> > 
> > Cc: devicetree at vger.kernel.org
> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > ---
> >  Documentation/devicetree/bindings/watchdog/orion-wdt.txt | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/watchdog/orion-wdt.txt b/Documentation/devicetree/bindings/watchdog/orion-wdt.txt
> > index 5dc8d30..bb7f1a2 100644
> > --- a/Documentation/devicetree/bindings/watchdog/orion-wdt.txt
> > +++ b/Documentation/devicetree/bindings/watchdog/orion-wdt.txt
> > @@ -3,7 +3,10 @@
> >  Required Properties:
> >  
> >  - Compatibility : "marvell,orion-wdt"
> > -- reg		: Address of the timer registers
> > +- reg		: Three cells are required.
> > +		  First  cell contains the global timer control register.
> > +		  Second cell contains the watchdog counter register.
> > +		  Third  cell contains the RSTOUT register.
> >  
> >  Optional properties:
> >  
> > @@ -13,7 +16,9 @@ Example:
> >  
> >  	wdt at 20300 {
> >  		compatible = "marvell,orion-wdt";
> > -		reg = <0x20300 0x28>;
> > +		reg = <0x20300 0x4
> > +		       0x20324 0x4
> > +		       0x20108 0x4>;
> 
> I don't like this.  It reaches outside of the wdt register.  I think a
> more clean way to do this is to do a provider/consumer relationship as
> in reset.txt.  eg, here you would retain the original reg binding, and
> add a reset phandle.
> 

Mmm... I can't see how this fits a reset-controller usage.

The watchdog simply "enables" the RSTOUT bit that allows the whole SoC
to be reset when the watchdog counter expires.

The reset-controller seems to be meant to send reset signals to devices,
which is not this case.

What am I missing?
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH 7/7] watchdog: orion: Update device-tree binding documentation

[Ezequiel Garcia]

On Fri, Aug 23, 2013 at 07:04:51AM -0300, Ezequiel Garcia wrote:
> On Thu, Aug 22, 2013 at 09:26:21PM -0400, Jason Cooper wrote:
> > On Thu, Aug 22, 2013 at 11:41:58AM -0300, Ezequiel Garcia wrote:
> > > Change the 'reg' property meaning, by defining three required cells.
> > > It's important to note this commit breaks DT-compatibility for this
> > > device.
> > > 
> > > Cc: devicetree at vger.kernel.org
> > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > > ---
> > >  Documentation/devicetree/bindings/watchdog/orion-wdt.txt | 9 +++++++--
> > >  1 file changed, 7 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/watchdog/orion-wdt.txt b/Documentation/devicetree/bindings/watchdog/orion-wdt.txt
> > > index 5dc8d30..bb7f1a2 100644
> > > --- a/Documentation/devicetree/bindings/watchdog/orion-wdt.txt
> > > +++ b/Documentation/devicetree/bindings/watchdog/orion-wdt.txt
> > > @@ -3,7 +3,10 @@
> > >  Required Properties:
> > >  
> > >  - Compatibility : "marvell,orion-wdt"
> > > -- reg		: Address of the timer registers
> > > +- reg		: Three cells are required.
> > > +		  First  cell contains the global timer control register.
> > > +		  Second cell contains the watchdog counter register.
> > > +		  Third  cell contains the RSTOUT register.
> > >  
> > >  Optional properties:
> > >  
> > > @@ -13,7 +16,9 @@ Example:
> > >  
> > >  	wdt at 20300 {
> > >  		compatible = "marvell,orion-wdt";
> > > -		reg = <0x20300 0x28>;
> > > +		reg = <0x20300 0x4
> > > +		       0x20324 0x4
> > > +		       0x20108 0x4>;
> > 
> > I don't like this.  It reaches outside of the wdt register.  I think a
> > more clean way to do this is to do a provider/consumer relationship as
> > in reset.txt.  eg, here you would retain the original reg binding, and
> > add a reset phandle.
> > 
> 
> Mmm... I can't see how this fits a reset-controller usage.
> 
> The watchdog simply "enables" the RSTOUT bit that allows the whole SoC
> to be reset when the watchdog counter expires.
> 
> The reset-controller seems to be meant to send reset signals to devices,
> which is not this case.
> 
> What am I missing?

Another possible solution is to simply "enable" the RSTOUT bit for
watchdog somewhere in mach-{kirkwood,mvebu,...} at board boot-up time.

Do you think that would have any drawbacks?
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH 7/7] watchdog: orion: Update device-tree binding documentation

[Sebastian Hesselbarth]

On 08/23/13 14:53, Ezequiel Garcia wrote:
> On Fri, Aug 23, 2013 at 07:04:51AM -0300, Ezequiel Garcia wrote:
>> On Thu, Aug 22, 2013 at 09:26:21PM -0400, Jason Cooper wrote:
>>> On Thu, Aug 22, 2013 at 11:41:58AM -0300, Ezequiel Garcia wrote:
>>>> diff --git a/Documentation/devicetree/bindings/watchdog/orion-wdt.txt b/Documentation/devicetree/bindings/watchdog/orion-wdt.txt
>>>> index 5dc8d30..bb7f1a2 100644
>>>> --- a/Documentation/devicetree/bindings/watchdog/orion-wdt.txt
>>>> +++ b/Documentation/devicetree/bindings/watchdog/orion-wdt.txt
>>>> @@ -13,7 +16,9 @@ Example:
>>>>
>>>>   	wdt at 20300 {
>>>>   		compatible = "marvell,orion-wdt";
>>>> -		reg = <0x20300 0x28>;
>>>> +		reg = <0x20300 0x4
>>>> +		       0x20324 0x4
>>>> +		       0x20108 0x4>;
>>>
>>> I don't like this.  It reaches outside of the wdt register.  I think a
>>> more clean way to do this is to do a provider/consumer relationship as
>>> in reset.txt.  eg, here you would retain the original reg binding, and
>>> add a reset phandle.
>>
>> Mmm... I can't see how this fits a reset-controller usage.
>>
>> The watchdog simply "enables" the RSTOUT bit that allows the whole SoC
>> to be reset when the watchdog counter expires.
>>
>> The reset-controller seems to be meant to send reset signals to devices,
>> which is not this case.
>>
>> What am I missing?
>
> Another possible solution is to simply "enable" the RSTOUT bit for
> watchdog somewhere in mach-{kirkwood,mvebu,...} at board boot-up time.
>
> Do you think that would have any drawbacks?

IMHO, it should be fine to always enable watchdog reset -> rstout_n
assertion. The watchdog driver does it unconditionally anyway.
We can move it to arch specific code now, and reset API handler later.

Sebastian

[PATCH 7/7] watchdog: orion: Update device-tree binding documentation

[Ezequiel Garcia]

On Fri, Aug 23, 2013 at 02:57:05PM +0200, Sebastian Hesselbarth wrote:
> On 08/23/13 14:53, Ezequiel Garcia wrote:
> > On Fri, Aug 23, 2013 at 07:04:51AM -0300, Ezequiel Garcia wrote:
> >> On Thu, Aug 22, 2013 at 09:26:21PM -0400, Jason Cooper wrote:
> >>> On Thu, Aug 22, 2013 at 11:41:58AM -0300, Ezequiel Garcia wrote:
> >>>> diff --git a/Documentation/devicetree/bindings/watchdog/orion-wdt.txt b/Documentation/devicetree/bindings/watchdog/orion-wdt.txt
> >>>> index 5dc8d30..bb7f1a2 100644
> >>>> --- a/Documentation/devicetree/bindings/watchdog/orion-wdt.txt
> >>>> +++ b/Documentation/devicetree/bindings/watchdog/orion-wdt.txt
> >>>> @@ -13,7 +16,9 @@ Example:
> >>>>
> >>>>   	wdt at 20300 {
> >>>>   		compatible = "marvell,orion-wdt";
> >>>> -		reg = <0x20300 0x28>;
> >>>> +		reg = <0x20300 0x4
> >>>> +		       0x20324 0x4
> >>>> +		       0x20108 0x4>;
> >>>
> >>> I don't like this.  It reaches outside of the wdt register.  I think a
> >>> more clean way to do this is to do a provider/consumer relationship as
> >>> in reset.txt.  eg, here you would retain the original reg binding, and
> >>> add a reset phandle.
> >>
> >> Mmm... I can't see how this fits a reset-controller usage.
> >>
> >> The watchdog simply "enables" the RSTOUT bit that allows the whole SoC
> >> to be reset when the watchdog counter expires.
> >>
> >> The reset-controller seems to be meant to send reset signals to devices,
> >> which is not this case.
> >>
> >> What am I missing?
> >
> > Another possible solution is to simply "enable" the RSTOUT bit for
> > watchdog somewhere in mach-{kirkwood,mvebu,...} at board boot-up time.
> >
> > Do you think that would have any drawbacks?
> 
> IMHO, it should be fine to always enable watchdog reset -> rstout_n
> assertion. The watchdog driver does it unconditionally anyway.
> We can move it to arch specific code now, and reset API handler later.
> 

Indeed, that's more or less what I was thinking about :-)
I'll re-send then with these modifications.

Thanks,
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH 7/7] watchdog: orion: Update device-tree binding documentation

[Jason Cooper]

On Fri, Aug 23, 2013 at 07:04:51AM -0300, Ezequiel Garcia wrote:
> On Thu, Aug 22, 2013 at 09:26:21PM -0400, Jason Cooper wrote:
> > On Thu, Aug 22, 2013 at 11:41:58AM -0300, Ezequiel Garcia wrote:
> > > Change the 'reg' property meaning, by defining three required cells.
> > > It's important to note this commit breaks DT-compatibility for this
> > > device.
> > > 
> > > Cc: devicetree@vger.kernel.org
> > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > > ---
> > >  Documentation/devicetree/bindings/watchdog/orion-wdt.txt | 9 +++++++--
> > >  1 file changed, 7 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/watchdog/orion-wdt.txt b/Documentation/devicetree/bindings/watchdog/orion-wdt.txt
> > > index 5dc8d30..bb7f1a2 100644
> > > --- a/Documentation/devicetree/bindings/watchdog/orion-wdt.txt
> > > +++ b/Documentation/devicetree/bindings/watchdog/orion-wdt.txt
> > > @@ -3,7 +3,10 @@
> > >  Required Properties:
> > >  
> > >  - Compatibility : "marvell,orion-wdt"
> > > -- reg		: Address of the timer registers
> > > +- reg		: Three cells are required.
> > > +		  First  cell contains the global timer control register.
> > > +		  Second cell contains the watchdog counter register.
> > > +		  Third  cell contains the RSTOUT register.
> > >  
> > >  Optional properties:
> > >  
> > > @@ -13,7 +16,9 @@ Example:
> > >  
> > >  	wdt@20300 {
> > >  		compatible = "marvell,orion-wdt";
> > > -		reg = <0x20300 0x28>;
> > > +		reg = <0x20300 0x4
> > > +		       0x20324 0x4
> > > +		       0x20108 0x4>;
> > 
> > I don't like this.  It reaches outside of the wdt register.  I think a
> > more clean way to do this is to do a provider/consumer relationship as
> > in reset.txt.  eg, here you would retain the original reg binding, and
> > add a reset phandle.
> > 
> 
> Mmm... I can't see how this fits a reset-controller usage.

Sorry, I wasn't clear.  'as in reset.txt' should have probably been
'similar to reset.txt'.

> The watchdog simply "enables" the RSTOUT bit that allows the whole SoC
> to be reset when the watchdog counter expires.

right.

> The reset-controller seems to be meant to send reset signals to devices,
> which is not this case.

yes, I was thinking (and drinking) of a node for the whole register
(0x20108), with an eventual driver.  Then the phandle in the wdt node.
This hypothetical driver could also incorporate the CPU_SOFT_RESET
(0x2010c) register from orion5x...

> What am I missing?

A suitable application of alcohol and Sun. :)

thx,

Jason.

[PATCH 7/7] watchdog: orion: Update device-tree binding documentation

[Jason Cooper]

On Fri, Aug 23, 2013 at 07:04:51AM -0300, Ezequiel Garcia wrote:
> On Thu, Aug 22, 2013 at 09:26:21PM -0400, Jason Cooper wrote:
> > On Thu, Aug 22, 2013 at 11:41:58AM -0300, Ezequiel Garcia wrote:
> > > Change the 'reg' property meaning, by defining three required cells.
> > > It's important to note this commit breaks DT-compatibility for this
> > > device.
> > > 
> > > Cc: devicetree at vger.kernel.org
> > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > > ---
> > >  Documentation/devicetree/bindings/watchdog/orion-wdt.txt | 9 +++++++--
> > >  1 file changed, 7 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/watchdog/orion-wdt.txt b/Documentation/devicetree/bindings/watchdog/orion-wdt.txt
> > > index 5dc8d30..bb7f1a2 100644
> > > --- a/Documentation/devicetree/bindings/watchdog/orion-wdt.txt
> > > +++ b/Documentation/devicetree/bindings/watchdog/orion-wdt.txt
> > > @@ -3,7 +3,10 @@
> > >  Required Properties:
> > >  
> > >  - Compatibility : "marvell,orion-wdt"
> > > -- reg		: Address of the timer registers
> > > +- reg		: Three cells are required.
> > > +		  First  cell contains the global timer control register.
> > > +		  Second cell contains the watchdog counter register.
> > > +		  Third  cell contains the RSTOUT register.
> > >  
> > >  Optional properties:
> > >  
> > > @@ -13,7 +16,9 @@ Example:
> > >  
> > >  	wdt at 20300 {
> > >  		compatible = "marvell,orion-wdt";
> > > -		reg = <0x20300 0x28>;
> > > +		reg = <0x20300 0x4
> > > +		       0x20324 0x4
> > > +		       0x20108 0x4>;
> > 
> > I don't like this.  It reaches outside of the wdt register.  I think a
> > more clean way to do this is to do a provider/consumer relationship as
> > in reset.txt.  eg, here you would retain the original reg binding, and
> > add a reset phandle.
> > 
> 
> Mmm... I can't see how this fits a reset-controller usage.

Sorry, I wasn't clear.  'as in reset.txt' should have probably been
'similar to reset.txt'.

> The watchdog simply "enables" the RSTOUT bit that allows the whole SoC
> to be reset when the watchdog counter expires.

right.

> The reset-controller seems to be meant to send reset signals to devices,
> which is not this case.

yes, I was thinking (and drinking) of a node for the whole register
(0x20108), with an eventual driver.  Then the phandle in the wdt node.
This hypothetical driver could also incorporate the CPU_SOFT_RESET
(0x2010c) register from orion5x...

> What am I missing?

A suitable application of alcohol and Sun. :)

thx,

Jason.