Orion watchdog DT changes to support more SoCs

PATCH Superseded Ezequiel Garcia 3 participants 15 messages Aug 22, 2013 – Aug 26, 2013

Subsystems (5)

Device tree bindings Arm (32-bit) Arm sub-architectures Watchdog device drivers +1 more subsystems

lore

Share & tools

lore

Ezequiel Garcia

author

2013-08-22 14:41 UTC #0 lore raw

cover Orion watchdog DT changes to support more SoCs

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

Signature · 1 line

1.8.1.5

Ezequiel Garcia

author

2013-08-22 14:41 UTC #1 lore raw

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

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 <[email protected]>

---
 drivers/watchdog/orion_wdt.c | 3 ---
 1 file changed, 3 deletions(-)

drivers/watchdog/orion_wdt.c +0 -3

index 4ea5fcc..d43a3da 100644
@@ -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;

Signature · 1 line

1.8.1.5

Ezequiel Garcia

author

2013-08-22 14:41 UTC #2 lore raw

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

In reply to 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 <[email protected]>

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

arch/arm/mach-kirkwood/include/mach/bridge-regs.h +1 -0

index 91242c9..05a1164 100644
@@ -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

arch/arm/plat-orion/common.c +6 -4

index c66d163..4e30ed6 100644
@@ -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)

drivers/watchdog/orion_wdt.c +11 -4

index d43a3da..739e79c 100644
@@ -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;

Signature · 1 line

1.8.1.5

Ezequiel Garcia

author

2013-08-22 14:41 UTC #3 lore raw

PATCH 3/7 watchdog: orion: Make RSTOUT register a separate resource

In reply to 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 <[email protected]>

---
 arch/arm/plat-orion/common.c |  1 +
 drivers/watchdog/orion_wdt.c | 16 ++++++++++++----
 2 files changed, 13 insertions(+), 4 deletions(-)

arch/arm/plat-orion/common.c +1 -0

index 4e30ed6..860db39 100644
@@ -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 = {

drivers/watchdog/orion_wdt.c +12 -4

index 739e79c..b4fd0a9 100644
@@ -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;

Signature · 1 line

1.8.1.5

Ezequiel Garcia

author

2013-08-22 14:41 UTC #4 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/watchdog/Kconfig     | 2 +-
 drivers/watchdog/orion_wdt.c | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

drivers/watchdog/Kconfig +1 -1

index 362085d..524f30a 100644
@@ -282,7 +282,7 @@ config DAVINCI_WATCHDOG
 
 config ORION_WATCHDOG
 	tristate "Orion watchdog"
-	depends on ARCH_ORION5X || ARCH_KIRKWOOD || ARCH_DOVE
+	depends on PLAT_ORION
 	select WATCHDOG_CORE
 	help
 	  Say Y here if to include support for the watchdog timer

drivers/watchdog/orion_wdt.c +0 -1

index b4fd0a9..04ceb0e 100644
@@ -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.

Signature · 1 line

1.8.1.5

Ezequiel Garcia

author

2013-08-22 14:41 UTC #5 lore raw

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

In reply to 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 <[email protected]>

---
 arch/arm/boot/dts/kirkwood.dtsi | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

arch/arm/boot/dts/kirkwood.dtsi +3 -1

index 9809fc1..5fba37b 100644
@@ -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";
 		};

Signature · 1 line

1.8.1.5

Ezequiel Garcia

author

2013-08-22 14:41 UTC #6 lore raw

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

In reply to Ezequiel Garcia

Name this file to something a bit more judicious.

Cc devicetree at vger.kernel.org

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

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

Documentation/devicetree/bindings/watchdog/orion-wdt.txt renamed from Documentation/devicetree/bindings/watchdog/marvel.txt +0 -0

similarity index 100%
rename from Documentation/devicetree/bindings/watchdog/marvel.txt
rename to Documentation/devicetree/bindings/watchdog/orion-wdt.txt

Signature · 1 line

1.8.1.5

Ezequiel Garcia

author

2013-08-22 14:41 UTC #7 lore raw

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

In reply to 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 <[email protected]>

---
 Documentation/devicetree/bindings/watchdog/orion-wdt.txt | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Documentation/devicetree/bindings/watchdog/orion-wdt.txt +7 -2

index 5dc8d30..bb7f1a2 100644
@@ -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";
 	};

Signature · 1 line

1.8.1.5

Jason Cooper

2013-08-23 01:26 UTC #8 lore raw

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

Quoted 27 lines · 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 <[email protected]>
---
 Documentation/devicetree/bindings/watchdog/orion-wdt.txt | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)
Documentation/devicetree/bindings/watchdog/orion-wdt.txt +4 -1
index 5dc8d30..bb7f1a2 100644
@@ -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:
 

Documentation/devicetree/bindings/watchdog/orion-wdt.txt Example: · line 21

Quoted 7 lines

Documentation/devicetree/bindings/watchdog/orion-wdt.txt +3 -1

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

Ezequiel Garcia

author

2013-08-23 10:04 UTC #9 lore raw

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

Quoted 29 lines · Jason Cooper

Quoted 28 lines · 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 <[email protected]>
---
 Documentation/devicetree/bindings/watchdog/orion-wdt.txt | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)
Documentation/devicetree/bindings/watchdog/orion-wdt.txt +4 -1
index 5dc8d30..bb7f1a2 100644
@@ -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:
 

Quoted 12 lines

Documentation/devicetree/bindings/watchdog/orion-wdt.txt Example: · line 21
Quoted 6 lines
Documentation/devicetree/bindings/watchdog/orion-wdt.txt +3 -1
 	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?

Signature · 3 lines

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

Ezequiel Garcia

author

2013-08-23 12:53 UTC #10 lore raw

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

Quoted 45 lines · Ezequiel Garcia

Quoted 29 lines · Jason Cooper
Quoted 28 lines · 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 <[email protected]>
---
 Documentation/devicetree/bindings/watchdog/orion-wdt.txt | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)
Documentation/devicetree/bindings/watchdog/orion-wdt.txt +4 -1
index 5dc8d30..bb7f1a2 100644
@@ -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:
 
Quoted 12 lines
Documentation/devicetree/bindings/watchdog/orion-wdt.txt Example: · line 21
Quoted 6 lines
Documentation/devicetree/bindings/watchdog/orion-wdt.txt +3 -1
 	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.

Quoted 7 lines

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?

Signature · 3 lines

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

Sebastian Hesselbarth

maintainer

2013-08-23 12:57 UTC #11 lore raw

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

Quoted 29 lines · Ezequiel Garcia

Quoted 28 lines · Ezequiel Garcia
Quoted 7 lines · Jason Cooper
Quoted 6 lines · Ezequiel Garcia
Documentation/devicetree/bindings/watchdog/orion-wdt.txt +0 -0
index 5dc8d30..bb7f1a2 100644
@@ -13,7 +16,9 @@ Example:
 
Quoted 11 lines
Documentation/devicetree/bindings/watchdog/orion-wdt.txt Example: · line 21
Quoted 6 lines
Documentation/devicetree/bindings/watchdog/orion-wdt.txt +3 -1
  	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.

Quoted 6 lines

Quoted 1 line
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

Ezequiel Garcia

author

2013-08-23 13:07 UTC #12 lore raw

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

Quoted 32 lines · Sebastian Hesselbarth

Quoted 31 lines · Ezequiel Garcia
Quoted 29 lines · Ezequiel Garcia
Quoted 7 lines · Jason Cooper
Quoted 6 lines · Ezequiel Garcia
Documentation/devicetree/bindings/watchdog/orion-wdt.txt +0 -0
index 5dc8d30..bb7f1a2 100644
@@ -13,7 +16,9 @@ Example:
 
Quoted 11 lines
Documentation/devicetree/bindings/watchdog/orion-wdt.txt Example: · line 21
Quoted 6 lines
Documentation/devicetree/bindings/watchdog/orion-wdt.txt +3 -1
  	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?

Quoted 9 lines

Quoted 4 lines
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,

Signature · 3 lines

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

Jason Cooper

2013-08-26 14:00 UTC #13 lore raw

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

In reply to Ezequiel Garcia

Quoted 43 lines · Ezequiel Garcia

Quoted 29 lines · Jason Cooper
Quoted 28 lines · 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 <[email protected]>
---
 Documentation/devicetree/bindings/watchdog/orion-wdt.txt | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)
Documentation/devicetree/bindings/watchdog/orion-wdt.txt +4 -1
index 5dc8d30..bb7f1a2 100644
@@ -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:
 
Quoted 12 lines
Documentation/devicetree/bindings/watchdog/orion-wdt.txt Example: · line 21
Quoted 6 lines
Documentation/devicetree/bindings/watchdog/orion-wdt.txt +3 -1
 	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.

Quoted 1 line

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

Quoted 2 lines

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

right.

Quoted 2 lines

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

Quoted 1 line

What am I missing?

A suitable application of alcohol and Sun. :)

thx,

Jason.

Jason Cooper

2013-08-23 01:07 UTC #14 lore raw

cover Orion watchdog DT changes to support more SoCs

In reply to Ezequiel Garcia

Quoted 17 lines · 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?

Quoted 5 lines

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.