[PATCH v4 0/2] imx6: Implement external watchdog reset

[PATCH v4 0/2] imx6: Implement external watchdog reset

[Akshay Bhat]

[Rebase to next-20151105 and re-sending work done by Tim Harvey]

The IMX6 watchdog supports assertion of a signal (WDOG_B) which
can be pinmux'd to an external pin. This is typically used for boards that
have PMIC's in control of the IMX6 power rails. In fact, failure to use
such an external reset on boards with external PMIC's can result in various
hangs due to the IMX6 not being fully reset [1] as well as the board failing
to reset because its PMIC has not been reset to provide adequate voltate for
the CPU when comming out of reset at 800Mhz when it was at 400Mhz prior to
reset.
 
This adds a new device-tree property 'ext-reset-output' to fsl-imx-wdt in
order to indicate the board has such a reset and to cause the watchdog to be
configured to assert WDOG_B instead of an internal reset both on a
watchdog timeout and in system_restart.
 
The second patch adds the watchdog configuration and pinmux for Gateworks
Ventana boards.
 
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html

Changes:
v3->v4:
- Rebase and test against linux-next tag next-20151105

History:
v1: http://lists.infradead.org/pipermail/linux-arm-kernel/2015-May/347168.html
v2: http://lists.infradead.org/pipermail/linux-arm-kernel/2015-June/348761.html
v3: http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/360188.html

Tim Harvey (2):
  watchdog: imx2_wdt: add external reset support via 'ext-reset-output'
    dt prop
  ARM: dts: ventana: Add ext-reset support

 .../devicetree/bindings/watchdog/fsl-imx-wdt.txt     |  2 ++
 arch/arm/boot/dts/imx6qdl-gw51xx.dtsi                | 12 ++++++++++++
 arch/arm/boot/dts/imx6qdl-gw52xx.dtsi                | 12 ++++++++++++
 arch/arm/boot/dts/imx6qdl-gw53xx.dtsi                | 13 +++++++++++++
 arch/arm/boot/dts/imx6qdl-gw54xx.dtsi                | 17 +++++++++++++++++
 arch/arm/boot/dts/imx6qdl-gw551x.dtsi                | 12 ++++++++++++
 arch/arm/boot/dts/imx6qdl-gw552x.dtsi                | 12 ++++++++++++
 drivers/watchdog/imx2_wdt.c                          | 20 ++++++++++++++++++--
 8 files changed, 98 insertions(+), 2 deletions(-)

-- 
2.6.2

[PATCH v4 0/2] imx6: Implement external watchdog reset

[Akshay Bhat]

[Rebase to next-20151105 and re-sending work done by Tim Harvey]

The IMX6 watchdog supports assertion of a signal (WDOG_B) which
can be pinmux'd to an external pin. This is typically used for boards that
have PMIC's in control of the IMX6 power rails. In fact, failure to use
such an external reset on boards with external PMIC's can result in various
hangs due to the IMX6 not being fully reset [1] as well as the board failing
to reset because its PMIC has not been reset to provide adequate voltate for
the CPU when comming out of reset at 800Mhz when it was at 400Mhz prior to
reset.
 
This adds a new device-tree property 'ext-reset-output' to fsl-imx-wdt in
order to indicate the board has such a reset and to cause the watchdog to be
configured to assert WDOG_B instead of an internal reset both on a
watchdog timeout and in system_restart.
 
The second patch adds the watchdog configuration and pinmux for Gateworks
Ventana boards.
 
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html

Changes:
v3->v4:
- Rebase and test against linux-next tag next-20151105

History:
v1: http://lists.infradead.org/pipermail/linux-arm-kernel/2015-May/347168.html
v2: http://lists.infradead.org/pipermail/linux-arm-kernel/2015-June/348761.html
v3: http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/360188.html

Tim Harvey (2):
  watchdog: imx2_wdt: add external reset support via 'ext-reset-output'
    dt prop
  ARM: dts: ventana: Add ext-reset support

 .../devicetree/bindings/watchdog/fsl-imx-wdt.txt     |  2 ++
 arch/arm/boot/dts/imx6qdl-gw51xx.dtsi                | 12 ++++++++++++
 arch/arm/boot/dts/imx6qdl-gw52xx.dtsi                | 12 ++++++++++++
 arch/arm/boot/dts/imx6qdl-gw53xx.dtsi                | 13 +++++++++++++
 arch/arm/boot/dts/imx6qdl-gw54xx.dtsi                | 17 +++++++++++++++++
 arch/arm/boot/dts/imx6qdl-gw551x.dtsi                | 12 ++++++++++++
 arch/arm/boot/dts/imx6qdl-gw552x.dtsi                | 12 ++++++++++++
 drivers/watchdog/imx2_wdt.c                          | 20 ++++++++++++++++++--
 8 files changed, 98 insertions(+), 2 deletions(-)

-- 
2.6.2

[PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Akshay Bhat]

From: Tim Harvey <tharvey@gateworks.com>

The IMX6 watchdog supports assertion of a signal (WDOG_B) which
can be pinmux'd to an external pin. This is typically used for boards that
have PMIC's in control of the IMX6 power rails. In fact, failure to use
such an external reset on boards with external PMIC's can result in various
hangs due to the IMX6 not being fully reset [1] as well as the board failing
to reset because its PMIC has not been reset to provide adequate voltage for
the CPU when coming out of reset at 800Mhz.

This uses a new device-tree property 'ext-reset-output' to indicate the
board has such a reset and to cause the watchdog to be configured to assert
WDOG_B instead of an internal reset both on a watchdog timeout and in
system_restart.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html

Cc: Lucas Stach <l.stach@pengutronix.de>
Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
 .../devicetree/bindings/watchdog/fsl-imx-wdt.txt     |  2 ++
 drivers/watchdog/imx2_wdt.c                          | 20 ++++++++++++++++++--
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
index 8dab6fd..9b89b3a 100644
--- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
@@ -9,6 +9,8 @@ Optional property:
 - big-endian: If present the watchdog device's registers are implemented
   in big endian mode, otherwise in native mode(same with CPU), for more
   detail please see: Documentation/devicetree/bindings/regmap/regmap.txt.
+- ext-reset-output: If present the watchdog device is configured to assert its
+  external reset (WDOG_B) instead of issuing a software reset.
 
 Examples:
 
diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
index 29ef719..84bfec4 100644
--- a/drivers/watchdog/imx2_wdt.c
+++ b/drivers/watchdog/imx2_wdt.c
@@ -41,6 +41,8 @@
 
 #define IMX2_WDT_WCR		0x00		/* Control Register */
 #define IMX2_WDT_WCR_WT		(0xFF << 8)	/* -> Watchdog Timeout Field */
+#define IMX2_WDT_WCR_WDA	(1 << 5)	/* -> External Reset WDOG_B */
+#define IMX2_WDT_WCR_SRS	(1 << 4)	/* -> Software Reset Signal */
 #define IMX2_WDT_WCR_WRE	(1 << 3)	/* -> WDOG Reset Enable */
 #define IMX2_WDT_WCR_WDE	(1 << 2)	/* -> Watchdog Enable */
 #define IMX2_WDT_WCR_WDZST	(1 << 0)	/* -> Watchdog timer Suspend */
@@ -65,6 +67,7 @@ struct imx2_wdt_device {
 	struct timer_list timer;	/* Pings the watchdog when closed */
 	struct watchdog_device wdog;
 	struct notifier_block restart_handler;
+	bool ext_reset;
 };
 
 static bool nowayout = WATCHDOG_NOWAYOUT;
@@ -90,6 +93,13 @@ static int imx2_restart_handler(struct notifier_block *this, unsigned long mode,
 	struct imx2_wdt_device *wdev = container_of(this,
 						    struct imx2_wdt_device,
 						    restart_handler);
+
+	/* Use internal reset or external - not both */
+	if (wdev->ext_reset)
+		wcr_enable |= IMX2_WDT_WCR_SRS; /* do not assert int reset */
+	else
+		wcr_enable |= IMX2_WDT_WCR_WDA; /* do not assert ext-reset */
+
 	/* Assert SRS signal */
 	regmap_write(wdev->regmap, IMX2_WDT_WCR, wcr_enable);
 	/*
@@ -119,8 +129,12 @@ static inline void imx2_wdt_setup(struct watchdog_device *wdog)
 	val |= IMX2_WDT_WCR_WDZST;
 	/* Strip the old watchdog Time-Out value */
 	val &= ~IMX2_WDT_WCR_WT;
-	/* Generate reset if WDOG times out */
-	val &= ~IMX2_WDT_WCR_WRE;
+	/* Generate internal chip-level reset if WDOG times out */
+	if (!wdev->ext_reset)
+		val &= ~IMX2_WDT_WCR_WRE;
+	/* Or if external-reset assert WDOG_B reset only on time-out */
+	else
+		val |= IMX2_WDT_WCR_WRE;
 	/* Keep Watchdog Disabled */
 	val &= ~IMX2_WDT_WCR_WDE;
 	/* Set the watchdog's Time-Out value */
@@ -267,6 +281,8 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
 	regmap_read(wdev->regmap, IMX2_WDT_WRSR, &val);
 	wdog->bootstatus = val & IMX2_WDT_WRSR_TOUT ? WDIOF_CARDRESET : 0;
 
+	wdev->ext_reset = of_property_read_bool(pdev->dev.of_node,
+						"ext-reset-output");
 	wdog->timeout = clamp_t(unsigned, timeout, 1, IMX2_WDT_MAX_TIME);
 	if (wdog->timeout != timeout)
 		dev_warn(&pdev->dev, "Initial timeout out of range! Clamped from %u to %u\n",
-- 
2.6.2

[PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Akshay Bhat]

From: Tim Harvey <tharvey@gateworks.com>

The IMX6 watchdog supports assertion of a signal (WDOG_B) which
can be pinmux'd to an external pin. This is typically used for boards that
have PMIC's in control of the IMX6 power rails. In fact, failure to use
such an external reset on boards with external PMIC's can result in various
hangs due to the IMX6 not being fully reset [1] as well as the board failing
to reset because its PMIC has not been reset to provide adequate voltage for
the CPU when coming out of reset at 800Mhz.

This uses a new device-tree property 'ext-reset-output' to indicate the
board has such a reset and to cause the watchdog to be configured to assert
WDOG_B instead of an internal reset both on a watchdog timeout and in
system_restart.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html

Cc: Lucas Stach <l.stach@pengutronix.de>
Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
 .../devicetree/bindings/watchdog/fsl-imx-wdt.txt     |  2 ++
 drivers/watchdog/imx2_wdt.c                          | 20 ++++++++++++++++++--
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
index 8dab6fd..9b89b3a 100644
--- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
@@ -9,6 +9,8 @@ Optional property:
 - big-endian: If present the watchdog device's registers are implemented
   in big endian mode, otherwise in native mode(same with CPU), for more
   detail please see: Documentation/devicetree/bindings/regmap/regmap.txt.
+- ext-reset-output: If present the watchdog device is configured to assert its
+  external reset (WDOG_B) instead of issuing a software reset.
 
 Examples:
 
diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
index 29ef719..84bfec4 100644
--- a/drivers/watchdog/imx2_wdt.c
+++ b/drivers/watchdog/imx2_wdt.c
@@ -41,6 +41,8 @@
 
 #define IMX2_WDT_WCR		0x00		/* Control Register */
 #define IMX2_WDT_WCR_WT		(0xFF << 8)	/* -> Watchdog Timeout Field */
+#define IMX2_WDT_WCR_WDA	(1 << 5)	/* -> External Reset WDOG_B */
+#define IMX2_WDT_WCR_SRS	(1 << 4)	/* -> Software Reset Signal */
 #define IMX2_WDT_WCR_WRE	(1 << 3)	/* -> WDOG Reset Enable */
 #define IMX2_WDT_WCR_WDE	(1 << 2)	/* -> Watchdog Enable */
 #define IMX2_WDT_WCR_WDZST	(1 << 0)	/* -> Watchdog timer Suspend */
@@ -65,6 +67,7 @@ struct imx2_wdt_device {
 	struct timer_list timer;	/* Pings the watchdog when closed */
 	struct watchdog_device wdog;
 	struct notifier_block restart_handler;
+	bool ext_reset;
 };
 
 static bool nowayout = WATCHDOG_NOWAYOUT;
@@ -90,6 +93,13 @@ static int imx2_restart_handler(struct notifier_block *this, unsigned long mode,
 	struct imx2_wdt_device *wdev = container_of(this,
 						    struct imx2_wdt_device,
 						    restart_handler);
+
+	/* Use internal reset or external - not both */
+	if (wdev->ext_reset)
+		wcr_enable |= IMX2_WDT_WCR_SRS; /* do not assert int reset */
+	else
+		wcr_enable |= IMX2_WDT_WCR_WDA; /* do not assert ext-reset */
+
 	/* Assert SRS signal */
 	regmap_write(wdev->regmap, IMX2_WDT_WCR, wcr_enable);
 	/*
@@ -119,8 +129,12 @@ static inline void imx2_wdt_setup(struct watchdog_device *wdog)
 	val |= IMX2_WDT_WCR_WDZST;
 	/* Strip the old watchdog Time-Out value */
 	val &= ~IMX2_WDT_WCR_WT;
-	/* Generate reset if WDOG times out */
-	val &= ~IMX2_WDT_WCR_WRE;
+	/* Generate internal chip-level reset if WDOG times out */
+	if (!wdev->ext_reset)
+		val &= ~IMX2_WDT_WCR_WRE;
+	/* Or if external-reset assert WDOG_B reset only on time-out */
+	else
+		val |= IMX2_WDT_WCR_WRE;
 	/* Keep Watchdog Disabled */
 	val &= ~IMX2_WDT_WCR_WDE;
 	/* Set the watchdog's Time-Out value */
@@ -267,6 +281,8 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
 	regmap_read(wdev->regmap, IMX2_WDT_WRSR, &val);
 	wdog->bootstatus = val & IMX2_WDT_WRSR_TOUT ? WDIOF_CARDRESET : 0;
 
+	wdev->ext_reset = of_property_read_bool(pdev->dev.of_node,
+						"ext-reset-output");
 	wdog->timeout = clamp_t(unsigned, timeout, 1, IMX2_WDT_MAX_TIME);
 	if (wdog->timeout != timeout)
 		dev_warn(&pdev->dev, "Initial timeout out of range! Clamped from %u to %u\n",
-- 
2.6.2

[PATCH v4 2/2] ARM: dts: ventana: Add ext-reset support

[Akshay Bhat]

From: Tim Harvey <tharvey@gateworks.com>

The Gateworks Ventana boards have a PMIC that can be used to regulate the
CPU voltage rails for DVFS support. In order to ensure this PMIC is properly
reset the watchdog needs to be configured to assert its external reset
signal.

Additionally the pad used for WDOG_B needs to be configured which we add to
iomux.

Cc: Markus Pargmann <mpa@pengutronix.de>
Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
 arch/arm/boot/dts/imx6qdl-gw51xx.dtsi | 12 ++++++++++++
 arch/arm/boot/dts/imx6qdl-gw52xx.dtsi | 12 ++++++++++++
 arch/arm/boot/dts/imx6qdl-gw53xx.dtsi | 13 +++++++++++++
 arch/arm/boot/dts/imx6qdl-gw54xx.dtsi | 17 +++++++++++++++++
 arch/arm/boot/dts/imx6qdl-gw551x.dtsi | 12 ++++++++++++
 arch/arm/boot/dts/imx6qdl-gw552x.dtsi | 12 ++++++++++++
 6 files changed, 78 insertions(+)

diff --git a/arch/arm/boot/dts/imx6qdl-gw51xx.dtsi b/arch/arm/boot/dts/imx6qdl-gw51xx.dtsi
index 7b31fdb..d81bd72 100644
--- a/arch/arm/boot/dts/imx6qdl-gw51xx.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-gw51xx.dtsi
@@ -210,6 +210,12 @@
 	status = "okay";
 };
 
+&wdog1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_wdog>;
+	ext-reset-output;
+};
+
 &iomuxc {
 	imx6qdl-gw51xx {
 		pinctrl_enet: enetgrp {
@@ -328,5 +334,11 @@
 				MX6QDL_PAD_EIM_D22__GPIO3_IO22		0x1b0b0 /* OTG_PWR_EN */
 			>;
 		};
+
+		pinctrl_wdog: wdoggrp {
+			fsl,pins = <
+				MX6QDL_PAD_DISP0_DAT8__WDOG1_B		0x1b0b0
+			>;
+		};
 	};
 };
diff --git a/arch/arm/boot/dts/imx6qdl-gw52xx.dtsi b/arch/arm/boot/dts/imx6qdl-gw52xx.dtsi
index 1b66328..0d8f201 100644
--- a/arch/arm/boot/dts/imx6qdl-gw52xx.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-gw52xx.dtsi
@@ -326,6 +326,12 @@
 	status = "okay";
 };
 
+&wdog1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_wdog>;
+	ext-reset-output;
+};
+
 &iomuxc {
 	imx6qdl-gw52xx {
 		pinctrl_audmux: audmuxgrp {
@@ -501,5 +507,11 @@
 				MX6QDL_PAD_NANDF_CS1__SD3_VSELECT	0x170f9
 			>;
 		};
+
+		pinctrl_wdog: wdoggrp {
+			fsl,pins = <
+				MX6QDL_PAD_DISP0_DAT8__WDOG1_B		0x1b0b0
+			>;
+		};
 	};
 };
diff --git a/arch/arm/boot/dts/imx6qdl-gw53xx.dtsi b/arch/arm/boot/dts/imx6qdl-gw53xx.dtsi
index 7c51839..36f9ec6 100644
--- a/arch/arm/boot/dts/imx6qdl-gw53xx.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-gw53xx.dtsi
@@ -332,7 +332,14 @@
 	status = "okay";
 };
 
+&wdog1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_wdog>;
+	ext-reset-output;
+};
+
 &iomuxc {
+
 	imx6qdl-gw53xx {
 		pinctrl_audmux: audmuxgrp {
 			fsl,pins = <
@@ -508,5 +515,11 @@
 				MX6QDL_PAD_NANDF_CS1__SD3_VSELECT	0x170f9
 			>;
 		};
+
+		pinctrl_wdog: wdoggrp {
+			fsl,pins = <
+				MX6QDL_PAD_DISP0_DAT8__WDOG1_B		0x1b0b0
+			>;
+		};
 	};
 };
diff --git a/arch/arm/boot/dts/imx6qdl-gw54xx.dtsi b/arch/arm/boot/dts/imx6qdl-gw54xx.dtsi
index 929e0b3..0c1150f 100644
--- a/arch/arm/boot/dts/imx6qdl-gw54xx.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-gw54xx.dtsi
@@ -425,6 +425,17 @@
 	status = "okay";
 };
 
+&wdog1 {
+	status = "disabled";
+};
+
+&wdog2 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_wdog>;
+	ext-reset-output;
+	status = "okay";
+};
+
 &iomuxc {
 	imx6qdl-gw54xx {
 		pinctrl_audmux: audmuxgrp {
@@ -600,5 +611,11 @@
 				MX6QDL_PAD_NANDF_CS1__SD3_VSELECT	0x170f9
 			>;
 		};
+
+		pinctrl_wdog: wdoggrp {
+			fsl,pins = <
+				MX6QDL_PAD_SD1_DAT3__WDOG2_B		0x1b0b0
+			>;
+		};
 	};
 };
diff --git a/arch/arm/boot/dts/imx6qdl-gw551x.dtsi b/arch/arm/boot/dts/imx6qdl-gw551x.dtsi
index 741f3d5..883e577 100644
--- a/arch/arm/boot/dts/imx6qdl-gw551x.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-gw551x.dtsi
@@ -227,6 +227,12 @@
 	status = "okay";
 };
 
+&wdog1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_wdog>;
+	ext-reset-output;
+};
+
 &iomuxc {
 	imx6qdl-gw51xx {
 		pinctrl_flexcan1: flexcan1grp {
@@ -309,5 +315,11 @@
 				MX6QDL_PAD_GPIO_1__USB_OTG_ID		0x17059
 			>;
 		};
+
+		pinctrl_wdog: wdoggrp {
+			fsl,pins = <
+				MX6QDL_PAD_DISP0_DAT8__WDOG1_B		0x1b0b0
+			>;
+		};
 	};
 };
diff --git a/arch/arm/boot/dts/imx6qdl-gw552x.dtsi b/arch/arm/boot/dts/imx6qdl-gw552x.dtsi
index d1e5048..07674c2 100644
--- a/arch/arm/boot/dts/imx6qdl-gw552x.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-gw552x.dtsi
@@ -185,6 +185,12 @@
 	status = "okay";
 };
 
+&wdog1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_wdog>;
+	ext-reset-output;
+};
+
 &iomuxc {
 	imx6qdl-gw552x {
 		pinctrl_gpio_leds: gpioledsgrp {
@@ -262,5 +268,11 @@
 				MX6QDL_PAD_KEY_ROW1__UART5_RX_DATA	0x1b0b1
 			>;
                 };
+
+		pinctrl_wdog: wdoggrp {
+			fsl,pins = <
+				MX6QDL_PAD_DISP0_DAT8__WDOG1_B		0x1b0b0
+			>;
+		};
 	};
 };
-- 
2.6.2

[PATCH v4 2/2] ARM: dts: ventana: Add ext-reset support

[Akshay Bhat]

From: Tim Harvey <tharvey@gateworks.com>

The Gateworks Ventana boards have a PMIC that can be used to regulate the
CPU voltage rails for DVFS support. In order to ensure this PMIC is properly
reset the watchdog needs to be configured to assert its external reset
signal.

Additionally the pad used for WDOG_B needs to be configured which we add to
iomux.

Cc: Markus Pargmann <mpa@pengutronix.de>
Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
 arch/arm/boot/dts/imx6qdl-gw51xx.dtsi | 12 ++++++++++++
 arch/arm/boot/dts/imx6qdl-gw52xx.dtsi | 12 ++++++++++++
 arch/arm/boot/dts/imx6qdl-gw53xx.dtsi | 13 +++++++++++++
 arch/arm/boot/dts/imx6qdl-gw54xx.dtsi | 17 +++++++++++++++++
 arch/arm/boot/dts/imx6qdl-gw551x.dtsi | 12 ++++++++++++
 arch/arm/boot/dts/imx6qdl-gw552x.dtsi | 12 ++++++++++++
 6 files changed, 78 insertions(+)

diff --git a/arch/arm/boot/dts/imx6qdl-gw51xx.dtsi b/arch/arm/boot/dts/imx6qdl-gw51xx.dtsi
index 7b31fdb..d81bd72 100644
--- a/arch/arm/boot/dts/imx6qdl-gw51xx.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-gw51xx.dtsi
@@ -210,6 +210,12 @@
 	status = "okay";
 };
 
+&wdog1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_wdog>;
+	ext-reset-output;
+};
+
 &iomuxc {
 	imx6qdl-gw51xx {
 		pinctrl_enet: enetgrp {
@@ -328,5 +334,11 @@
 				MX6QDL_PAD_EIM_D22__GPIO3_IO22		0x1b0b0 /* OTG_PWR_EN */
 			>;
 		};
+
+		pinctrl_wdog: wdoggrp {
+			fsl,pins = <
+				MX6QDL_PAD_DISP0_DAT8__WDOG1_B		0x1b0b0
+			>;
+		};
 	};
 };
diff --git a/arch/arm/boot/dts/imx6qdl-gw52xx.dtsi b/arch/arm/boot/dts/imx6qdl-gw52xx.dtsi
index 1b66328..0d8f201 100644
--- a/arch/arm/boot/dts/imx6qdl-gw52xx.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-gw52xx.dtsi
@@ -326,6 +326,12 @@
 	status = "okay";
 };
 
+&wdog1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_wdog>;
+	ext-reset-output;
+};
+
 &iomuxc {
 	imx6qdl-gw52xx {
 		pinctrl_audmux: audmuxgrp {
@@ -501,5 +507,11 @@
 				MX6QDL_PAD_NANDF_CS1__SD3_VSELECT	0x170f9
 			>;
 		};
+
+		pinctrl_wdog: wdoggrp {
+			fsl,pins = <
+				MX6QDL_PAD_DISP0_DAT8__WDOG1_B		0x1b0b0
+			>;
+		};
 	};
 };
diff --git a/arch/arm/boot/dts/imx6qdl-gw53xx.dtsi b/arch/arm/boot/dts/imx6qdl-gw53xx.dtsi
index 7c51839..36f9ec6 100644
--- a/arch/arm/boot/dts/imx6qdl-gw53xx.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-gw53xx.dtsi
@@ -332,7 +332,14 @@
 	status = "okay";
 };
 
+&wdog1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_wdog>;
+	ext-reset-output;
+};
+
 &iomuxc {
+
 	imx6qdl-gw53xx {
 		pinctrl_audmux: audmuxgrp {
 			fsl,pins = <
@@ -508,5 +515,11 @@
 				MX6QDL_PAD_NANDF_CS1__SD3_VSELECT	0x170f9
 			>;
 		};
+
+		pinctrl_wdog: wdoggrp {
+			fsl,pins = <
+				MX6QDL_PAD_DISP0_DAT8__WDOG1_B		0x1b0b0
+			>;
+		};
 	};
 };
diff --git a/arch/arm/boot/dts/imx6qdl-gw54xx.dtsi b/arch/arm/boot/dts/imx6qdl-gw54xx.dtsi
index 929e0b3..0c1150f 100644
--- a/arch/arm/boot/dts/imx6qdl-gw54xx.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-gw54xx.dtsi
@@ -425,6 +425,17 @@
 	status = "okay";
 };
 
+&wdog1 {
+	status = "disabled";
+};
+
+&wdog2 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_wdog>;
+	ext-reset-output;
+	status = "okay";
+};
+
 &iomuxc {
 	imx6qdl-gw54xx {
 		pinctrl_audmux: audmuxgrp {
@@ -600,5 +611,11 @@
 				MX6QDL_PAD_NANDF_CS1__SD3_VSELECT	0x170f9
 			>;
 		};
+
+		pinctrl_wdog: wdoggrp {
+			fsl,pins = <
+				MX6QDL_PAD_SD1_DAT3__WDOG2_B		0x1b0b0
+			>;
+		};
 	};
 };
diff --git a/arch/arm/boot/dts/imx6qdl-gw551x.dtsi b/arch/arm/boot/dts/imx6qdl-gw551x.dtsi
index 741f3d5..883e577 100644
--- a/arch/arm/boot/dts/imx6qdl-gw551x.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-gw551x.dtsi
@@ -227,6 +227,12 @@
 	status = "okay";
 };
 
+&wdog1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_wdog>;
+	ext-reset-output;
+};
+
 &iomuxc {
 	imx6qdl-gw51xx {
 		pinctrl_flexcan1: flexcan1grp {
@@ -309,5 +315,11 @@
 				MX6QDL_PAD_GPIO_1__USB_OTG_ID		0x17059
 			>;
 		};
+
+		pinctrl_wdog: wdoggrp {
+			fsl,pins = <
+				MX6QDL_PAD_DISP0_DAT8__WDOG1_B		0x1b0b0
+			>;
+		};
 	};
 };
diff --git a/arch/arm/boot/dts/imx6qdl-gw552x.dtsi b/arch/arm/boot/dts/imx6qdl-gw552x.dtsi
index d1e5048..07674c2 100644
--- a/arch/arm/boot/dts/imx6qdl-gw552x.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-gw552x.dtsi
@@ -185,6 +185,12 @@
 	status = "okay";
 };
 
+&wdog1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_wdog>;
+	ext-reset-output;
+};
+
 &iomuxc {
 	imx6qdl-gw552x {
 		pinctrl_gpio_leds: gpioledsgrp {
@@ -262,5 +268,11 @@
 				MX6QDL_PAD_KEY_ROW1__UART5_RX_DATA	0x1b0b1
 			>;
                 };
+
+		pinctrl_wdog: wdoggrp {
+			fsl,pins = <
+				MX6QDL_PAD_DISP0_DAT8__WDOG1_B		0x1b0b0
+			>;
+		};
 	};
 };
-- 
2.6.2

Re: [PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Guenter Roeck]

On Thu, Nov 05, 2015 at 04:19:21PM -0500, Akshay Bhat wrote:
> From: Tim Harvey <tharvey@gateworks.com>
> 
> The IMX6 watchdog supports assertion of a signal (WDOG_B) which
> can be pinmux'd to an external pin. This is typically used for boards that
> have PMIC's in control of the IMX6 power rails. In fact, failure to use
> such an external reset on boards with external PMIC's can result in various
> hangs due to the IMX6 not being fully reset [1] as well as the board failing
> to reset because its PMIC has not been reset to provide adequate voltage for
> the CPU when coming out of reset at 800Mhz.
> 
> This uses a new device-tree property 'ext-reset-output' to indicate the
> board has such a reset and to cause the watchdog to be configured to assert
> WDOG_B instead of an internal reset both on a watchdog timeout and in
> system_restart.
> 
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html
> 
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> ---
>  .../devicetree/bindings/watchdog/fsl-imx-wdt.txt     |  2 ++
>  drivers/watchdog/imx2_wdt.c                          | 20 ++++++++++++++++++--
>  2 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> index 8dab6fd..9b89b3a 100644
> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> @@ -9,6 +9,8 @@ Optional property:

properties ?

>  - big-endian: If present the watchdog device's registers are implemented
>    in big endian mode, otherwise in native mode(same with CPU), for more
>    detail please see: Documentation/devicetree/bindings/regmap/regmap.txt.
> +- ext-reset-output: If present the watchdog device is configured to assert its

Should that have a vendor prefix ? Also, not sure if "-output"
has any real value in the property name. "fsl,external-reset", maybe ?

> +  external reset (WDOG_B) instead of issuing a software reset.
>  
>  Examples:
>  
> diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
> index 29ef719..84bfec4 100644
> --- a/drivers/watchdog/imx2_wdt.c
> +++ b/drivers/watchdog/imx2_wdt.c
> @@ -41,6 +41,8 @@
>  
>  #define IMX2_WDT_WCR		0x00		/* Control Register */
>  #define IMX2_WDT_WCR_WT		(0xFF << 8)	/* -> Watchdog Timeout Field */
> +#define IMX2_WDT_WCR_WDA	(1 << 5)	/* -> External Reset WDOG_B */
> +#define IMX2_WDT_WCR_SRS	(1 << 4)	/* -> Software Reset Signal */
>  #define IMX2_WDT_WCR_WRE	(1 << 3)	/* -> WDOG Reset Enable */
>  #define IMX2_WDT_WCR_WDE	(1 << 2)	/* -> Watchdog Enable */
>  #define IMX2_WDT_WCR_WDZST	(1 << 0)	/* -> Watchdog timer Suspend */
> @@ -65,6 +67,7 @@ struct imx2_wdt_device {
>  	struct timer_list timer;	/* Pings the watchdog when closed */
>  	struct watchdog_device wdog;
>  	struct notifier_block restart_handler;
> +	bool ext_reset;
>  };
>  
>  static bool nowayout = WATCHDOG_NOWAYOUT;
> @@ -90,6 +93,13 @@ static int imx2_restart_handler(struct notifier_block *this, unsigned long mode,
>  	struct imx2_wdt_device *wdev = container_of(this,
>  						    struct imx2_wdt_device,
>  						    restart_handler);
> +
> +	/* Use internal reset or external - not both */
> +	if (wdev->ext_reset)
> +		wcr_enable |= IMX2_WDT_WCR_SRS; /* do not assert int reset */
> +	else
> +		wcr_enable |= IMX2_WDT_WCR_WDA; /* do not assert ext-reset */
> +
>  	/* Assert SRS signal */
>  	regmap_write(wdev->regmap, IMX2_WDT_WCR, wcr_enable);
>  	/*
> @@ -119,8 +129,12 @@ static inline void imx2_wdt_setup(struct watchdog_device *wdog)
>  	val |= IMX2_WDT_WCR_WDZST;
>  	/* Strip the old watchdog Time-Out value */
>  	val &= ~IMX2_WDT_WCR_WT;
> -	/* Generate reset if WDOG times out */
> -	val &= ~IMX2_WDT_WCR_WRE;
> +	/* Generate internal chip-level reset if WDOG times out */
> +	if (!wdev->ext_reset)
> +		val &= ~IMX2_WDT_WCR_WRE;
> +	/* Or if external-reset assert WDOG_B reset only on time-out */
> +	else
> +		val |= IMX2_WDT_WCR_WRE;

I don't really understand what this means. Normally I would hope that a watchdog
only generates a reset if/when it times out.

>  	/* Keep Watchdog Disabled */
>  	val &= ~IMX2_WDT_WCR_WDE;
>  	/* Set the watchdog's Time-Out value */
> @@ -267,6 +281,8 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
>  	regmap_read(wdev->regmap, IMX2_WDT_WRSR, &val);
>  	wdog->bootstatus = val & IMX2_WDT_WRSR_TOUT ? WDIOF_CARDRESET : 0;
>  
> +	wdev->ext_reset = of_property_read_bool(pdev->dev.of_node,
> +						"ext-reset-output");
>  	wdog->timeout = clamp_t(unsigned, timeout, 1, IMX2_WDT_MAX_TIME);
>  	if (wdog->timeout != timeout)
>  		dev_warn(&pdev->dev, "Initial timeout out of range! Clamped from %u to %u\n",
> -- 
> 2.6.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Guenter Roeck]

On Thu, Nov 05, 2015 at 04:19:21PM -0500, Akshay Bhat wrote:
> From: Tim Harvey <tharvey-UMMOYl/HMS+akBO8gow8eQ@public.gmane.org>
> 
> The IMX6 watchdog supports assertion of a signal (WDOG_B) which
> can be pinmux'd to an external pin. This is typically used for boards that
> have PMIC's in control of the IMX6 power rails. In fact, failure to use
> such an external reset on boards with external PMIC's can result in various
> hangs due to the IMX6 not being fully reset [1] as well as the board failing
> to reset because its PMIC has not been reset to provide adequate voltage for
> the CPU when coming out of reset at 800Mhz.
> 
> This uses a new device-tree property 'ext-reset-output' to indicate the
> board has such a reset and to cause the watchdog to be configured to assert
> WDOG_B instead of an internal reset both on a watchdog timeout and in
> system_restart.
> 
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html
> 
> Cc: Lucas Stach <l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Signed-off-by: Tim Harvey <tharvey-UMMOYl/HMS+akBO8gow8eQ@public.gmane.org>
> ---
>  .../devicetree/bindings/watchdog/fsl-imx-wdt.txt     |  2 ++
>  drivers/watchdog/imx2_wdt.c                          | 20 ++++++++++++++++++--
>  2 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> index 8dab6fd..9b89b3a 100644
> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> @@ -9,6 +9,8 @@ Optional property:

properties ?

>  - big-endian: If present the watchdog device's registers are implemented
>    in big endian mode, otherwise in native mode(same with CPU), for more
>    detail please see: Documentation/devicetree/bindings/regmap/regmap.txt.
> +- ext-reset-output: If present the watchdog device is configured to assert its

Should that have a vendor prefix ? Also, not sure if "-output"
has any real value in the property name. "fsl,external-reset", maybe ?

> +  external reset (WDOG_B) instead of issuing a software reset.
>  
>  Examples:
>  
> diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
> index 29ef719..84bfec4 100644
> --- a/drivers/watchdog/imx2_wdt.c
> +++ b/drivers/watchdog/imx2_wdt.c
> @@ -41,6 +41,8 @@
>  
>  #define IMX2_WDT_WCR		0x00		/* Control Register */
>  #define IMX2_WDT_WCR_WT		(0xFF << 8)	/* -> Watchdog Timeout Field */
> +#define IMX2_WDT_WCR_WDA	(1 << 5)	/* -> External Reset WDOG_B */
> +#define IMX2_WDT_WCR_SRS	(1 << 4)	/* -> Software Reset Signal */
>  #define IMX2_WDT_WCR_WRE	(1 << 3)	/* -> WDOG Reset Enable */
>  #define IMX2_WDT_WCR_WDE	(1 << 2)	/* -> Watchdog Enable */
>  #define IMX2_WDT_WCR_WDZST	(1 << 0)	/* -> Watchdog timer Suspend */
> @@ -65,6 +67,7 @@ struct imx2_wdt_device {
>  	struct timer_list timer;	/* Pings the watchdog when closed */
>  	struct watchdog_device wdog;
>  	struct notifier_block restart_handler;
> +	bool ext_reset;
>  };
>  
>  static bool nowayout = WATCHDOG_NOWAYOUT;
> @@ -90,6 +93,13 @@ static int imx2_restart_handler(struct notifier_block *this, unsigned long mode,
>  	struct imx2_wdt_device *wdev = container_of(this,
>  						    struct imx2_wdt_device,
>  						    restart_handler);
> +
> +	/* Use internal reset or external - not both */
> +	if (wdev->ext_reset)
> +		wcr_enable |= IMX2_WDT_WCR_SRS; /* do not assert int reset */
> +	else
> +		wcr_enable |= IMX2_WDT_WCR_WDA; /* do not assert ext-reset */
> +
>  	/* Assert SRS signal */
>  	regmap_write(wdev->regmap, IMX2_WDT_WCR, wcr_enable);
>  	/*
> @@ -119,8 +129,12 @@ static inline void imx2_wdt_setup(struct watchdog_device *wdog)
>  	val |= IMX2_WDT_WCR_WDZST;
>  	/* Strip the old watchdog Time-Out value */
>  	val &= ~IMX2_WDT_WCR_WT;
> -	/* Generate reset if WDOG times out */
> -	val &= ~IMX2_WDT_WCR_WRE;
> +	/* Generate internal chip-level reset if WDOG times out */
> +	if (!wdev->ext_reset)
> +		val &= ~IMX2_WDT_WCR_WRE;
> +	/* Or if external-reset assert WDOG_B reset only on time-out */
> +	else
> +		val |= IMX2_WDT_WCR_WRE;

I don't really understand what this means. Normally I would hope that a watchdog
only generates a reset if/when it times out.

>  	/* Keep Watchdog Disabled */
>  	val &= ~IMX2_WDT_WCR_WDE;
>  	/* Set the watchdog's Time-Out value */
> @@ -267,6 +281,8 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
>  	regmap_read(wdev->regmap, IMX2_WDT_WRSR, &val);
>  	wdog->bootstatus = val & IMX2_WDT_WRSR_TOUT ? WDIOF_CARDRESET : 0;
>  
> +	wdev->ext_reset = of_property_read_bool(pdev->dev.of_node,
> +						"ext-reset-output");
>  	wdog->timeout = clamp_t(unsigned, timeout, 1, IMX2_WDT_MAX_TIME);
>  	if (wdog->timeout != timeout)
>  		dev_warn(&pdev->dev, "Initial timeout out of range! Clamped from %u to %u\n",
> -- 
> 2.6.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Guenter Roeck]

On Thu, Nov 05, 2015 at 04:19:21PM -0500, Akshay Bhat wrote:
> From: Tim Harvey <tharvey@gateworks.com>
> 
> The IMX6 watchdog supports assertion of a signal (WDOG_B) which
> can be pinmux'd to an external pin. This is typically used for boards that
> have PMIC's in control of the IMX6 power rails. In fact, failure to use
> such an external reset on boards with external PMIC's can result in various
> hangs due to the IMX6 not being fully reset [1] as well as the board failing
> to reset because its PMIC has not been reset to provide adequate voltage for
> the CPU when coming out of reset at 800Mhz.
> 
> This uses a new device-tree property 'ext-reset-output' to indicate the
> board has such a reset and to cause the watchdog to be configured to assert
> WDOG_B instead of an internal reset both on a watchdog timeout and in
> system_restart.
> 
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html
> 
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> ---
>  .../devicetree/bindings/watchdog/fsl-imx-wdt.txt     |  2 ++
>  drivers/watchdog/imx2_wdt.c                          | 20 ++++++++++++++++++--
>  2 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> index 8dab6fd..9b89b3a 100644
> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> @@ -9,6 +9,8 @@ Optional property:

properties ?

>  - big-endian: If present the watchdog device's registers are implemented
>    in big endian mode, otherwise in native mode(same with CPU), for more
>    detail please see: Documentation/devicetree/bindings/regmap/regmap.txt.
> +- ext-reset-output: If present the watchdog device is configured to assert its

Should that have a vendor prefix ? Also, not sure if "-output"
has any real value in the property name. "fsl,external-reset", maybe ?

> +  external reset (WDOG_B) instead of issuing a software reset.
>  
>  Examples:
>  
> diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
> index 29ef719..84bfec4 100644
> --- a/drivers/watchdog/imx2_wdt.c
> +++ b/drivers/watchdog/imx2_wdt.c
> @@ -41,6 +41,8 @@
>  
>  #define IMX2_WDT_WCR		0x00		/* Control Register */
>  #define IMX2_WDT_WCR_WT		(0xFF << 8)	/* -> Watchdog Timeout Field */
> +#define IMX2_WDT_WCR_WDA	(1 << 5)	/* -> External Reset WDOG_B */
> +#define IMX2_WDT_WCR_SRS	(1 << 4)	/* -> Software Reset Signal */
>  #define IMX2_WDT_WCR_WRE	(1 << 3)	/* -> WDOG Reset Enable */
>  #define IMX2_WDT_WCR_WDE	(1 << 2)	/* -> Watchdog Enable */
>  #define IMX2_WDT_WCR_WDZST	(1 << 0)	/* -> Watchdog timer Suspend */
> @@ -65,6 +67,7 @@ struct imx2_wdt_device {
>  	struct timer_list timer;	/* Pings the watchdog when closed */
>  	struct watchdog_device wdog;
>  	struct notifier_block restart_handler;
> +	bool ext_reset;
>  };
>  
>  static bool nowayout = WATCHDOG_NOWAYOUT;
> @@ -90,6 +93,13 @@ static int imx2_restart_handler(struct notifier_block *this, unsigned long mode,
>  	struct imx2_wdt_device *wdev = container_of(this,
>  						    struct imx2_wdt_device,
>  						    restart_handler);
> +
> +	/* Use internal reset or external - not both */
> +	if (wdev->ext_reset)
> +		wcr_enable |= IMX2_WDT_WCR_SRS; /* do not assert int reset */
> +	else
> +		wcr_enable |= IMX2_WDT_WCR_WDA; /* do not assert ext-reset */
> +
>  	/* Assert SRS signal */
>  	regmap_write(wdev->regmap, IMX2_WDT_WCR, wcr_enable);
>  	/*
> @@ -119,8 +129,12 @@ static inline void imx2_wdt_setup(struct watchdog_device *wdog)
>  	val |= IMX2_WDT_WCR_WDZST;
>  	/* Strip the old watchdog Time-Out value */
>  	val &= ~IMX2_WDT_WCR_WT;
> -	/* Generate reset if WDOG times out */
> -	val &= ~IMX2_WDT_WCR_WRE;
> +	/* Generate internal chip-level reset if WDOG times out */
> +	if (!wdev->ext_reset)
> +		val &= ~IMX2_WDT_WCR_WRE;
> +	/* Or if external-reset assert WDOG_B reset only on time-out */
> +	else
> +		val |= IMX2_WDT_WCR_WRE;

I don't really understand what this means. Normally I would hope that a watchdog
only generates a reset if/when it times out.

>  	/* Keep Watchdog Disabled */
>  	val &= ~IMX2_WDT_WCR_WDE;
>  	/* Set the watchdog's Time-Out value */
> @@ -267,6 +281,8 @@ static int __init imx2_wdt_probe(struct platform_device *pdev)
>  	regmap_read(wdev->regmap, IMX2_WDT_WRSR, &val);
>  	wdog->bootstatus = val & IMX2_WDT_WRSR_TOUT ? WDIOF_CARDRESET : 0;
>  
> +	wdev->ext_reset = of_property_read_bool(pdev->dev.of_node,
> +						"ext-reset-output");
>  	wdog->timeout = clamp_t(unsigned, timeout, 1, IMX2_WDT_MAX_TIME);
>  	if (wdog->timeout != timeout)
>  		dev_warn(&pdev->dev, "Initial timeout out of range! Clamped from %u to %u\n",
> -- 
> 2.6.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Tim Harvey]

On Thu, Nov 5, 2015 at 2:23 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Thu, Nov 05, 2015 at 04:19:21PM -0500, Akshay Bhat wrote:
>> From: Tim Harvey <tharvey@gateworks.com>
>>
>> The IMX6 watchdog supports assertion of a signal (WDOG_B) which
>> can be pinmux'd to an external pin. This is typically used for boards that
>> have PMIC's in control of the IMX6 power rails. In fact, failure to use
>> such an external reset on boards with external PMIC's can result in various
>> hangs due to the IMX6 not being fully reset [1] as well as the board failing
>> to reset because its PMIC has not been reset to provide adequate voltage for
>> the CPU when coming out of reset at 800Mhz.
>>
>> This uses a new device-tree property 'ext-reset-output' to indicate the
>> board has such a reset and to cause the watchdog to be configured to assert
>> WDOG_B instead of an internal reset both on a watchdog timeout and in
>> system_restart.
>>
>> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html
>>
>> Cc: Lucas Stach <l.stach@pengutronix.de>
>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>> ---
>>  .../devicetree/bindings/watchdog/fsl-imx-wdt.txt     |  2 ++
>>  drivers/watchdog/imx2_wdt.c                          | 20 ++++++++++++++++++--
>>  2 files changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>> index 8dab6fd..9b89b3a 100644
>> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>> @@ -9,6 +9,8 @@ Optional property:
>
> properties ?
>
>>  - big-endian: If present the watchdog device's registers are implemented
>>    in big endian mode, otherwise in native mode(same with CPU), for more
>>    detail please see: Documentation/devicetree/bindings/regmap/regmap.txt.
>> +- ext-reset-output: If present the watchdog device is configured to assert its
>
> Should that have a vendor prefix ? Also, not sure if "-output"
> has any real value in the property name. "fsl,external-reset", maybe ?

Hi Guenter,

I don't see why a vendor prefix is necessary - its a feature of the
IMX6 watchdog supported by this driver to be able to trigger an
internal chip-level reset and/or an external signal that can be hooked
to additional hardware.

I started with ext-reset, but it was suggested
(http://lists.infradead.org/pipermail/linux-arm-kernel/2015-May/347437.html)
to also make it clear that this is an 'output' and not a reset input.

>
>> +  external reset (WDOG_B) instead of issuing a software reset.
>>
>>  Examples:
>>
>> diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
>> index 29ef719..84bfec4 100644
>> --- a/drivers/watchdog/imx2_wdt.c
>> +++ b/drivers/watchdog/imx2_wdt.c
>> @@ -41,6 +41,8 @@
>>
>>  #define IMX2_WDT_WCR         0x00            /* Control Register */
>>  #define IMX2_WDT_WCR_WT              (0xFF << 8)     /* -> Watchdog Timeout Field */
>> +#define IMX2_WDT_WCR_WDA     (1 << 5)        /* -> External Reset WDOG_B */
>> +#define IMX2_WDT_WCR_SRS     (1 << 4)        /* -> Software Reset Signal */
>>  #define IMX2_WDT_WCR_WRE     (1 << 3)        /* -> WDOG Reset Enable */
>>  #define IMX2_WDT_WCR_WDE     (1 << 2)        /* -> Watchdog Enable */
>>  #define IMX2_WDT_WCR_WDZST   (1 << 0)        /* -> Watchdog timer Suspend */
>> @@ -65,6 +67,7 @@ struct imx2_wdt_device {
>>       struct timer_list timer;        /* Pings the watchdog when closed */
>>       struct watchdog_device wdog;
>>       struct notifier_block restart_handler;
>> +     bool ext_reset;
>>  };
>>
>>  static bool nowayout = WATCHDOG_NOWAYOUT;
>> @@ -90,6 +93,13 @@ static int imx2_restart_handler(struct notifier_block *this, unsigned long mode,
>>       struct imx2_wdt_device *wdev = container_of(this,
>>                                                   struct imx2_wdt_device,
>>                                                   restart_handler);
>> +
>> +     /* Use internal reset or external - not both */
>> +     if (wdev->ext_reset)
>> +             wcr_enable |= IMX2_WDT_WCR_SRS; /* do not assert int reset */
>> +     else
>> +             wcr_enable |= IMX2_WDT_WCR_WDA; /* do not assert ext-reset */
>> +
>>       /* Assert SRS signal */
>>       regmap_write(wdev->regmap, IMX2_WDT_WCR, wcr_enable);
>>       /*
>> @@ -119,8 +129,12 @@ static inline void imx2_wdt_setup(struct watchdog_device *wdog)
>>       val |= IMX2_WDT_WCR_WDZST;
>>       /* Strip the old watchdog Time-Out value */
>>       val &= ~IMX2_WDT_WCR_WT;
>> -     /* Generate reset if WDOG times out */
>> -     val &= ~IMX2_WDT_WCR_WRE;
>> +     /* Generate internal chip-level reset if WDOG times out */
>> +     if (!wdev->ext_reset)
>> +             val &= ~IMX2_WDT_WCR_WRE;
>> +     /* Or if external-reset assert WDOG_B reset only on time-out */
>> +     else
>> +             val |= IMX2_WDT_WCR_WRE;
>
> I don't really understand what this means. Normally I would hope that a watchdog
> only generates a reset if/when it times out.

I think we went a couple of rounds on the info in the comment as well
in the previous patch reversions.

It is a feature of the IMX6 watchdog supported by this driver to be
able to trigger an internal chip-level reset and/or an external signal
that can be hooked to additional hardware.

In the case of using a PMIC that can regulate it's own rails (ie the
Freescale SabreSD reference design) an SoC chip-level reset will not
reset the PMIC and thus if the PMIC's rails have been driven down by
DVFS to a value below the necessary value for the chip to come up you
will hang (which is certainly not what you want to do when a watchdog
timeout occurs). The solution for designs that have a PMIC that is
able to change its voltage levels is to 'not' have the SoC internally
reset and to 'instead' drive an external reset signal that should
reset the PMIC (and possibly other chips if needed). The reason for
not allowing the internal reset is because as soon as the SoC goes
into reset it de-asserts the external reset which makes the time the
output is asserted un-deterministic.

Tim

Re: [PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Tim Harvey]

On Thu, Nov 5, 2015 at 2:23 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Thu, Nov 05, 2015 at 04:19:21PM -0500, Akshay Bhat wrote:
>> From: Tim Harvey <tharvey@gateworks.com>
>>
>> The IMX6 watchdog supports assertion of a signal (WDOG_B) which
>> can be pinmux'd to an external pin. This is typically used for boards that
>> have PMIC's in control of the IMX6 power rails. In fact, failure to use
>> such an external reset on boards with external PMIC's can result in various
>> hangs due to the IMX6 not being fully reset [1] as well as the board failing
>> to reset because its PMIC has not been reset to provide adequate voltage for
>> the CPU when coming out of reset at 800Mhz.
>>
>> This uses a new device-tree property 'ext-reset-output' to indicate the
>> board has such a reset and to cause the watchdog to be configured to assert
>> WDOG_B instead of an internal reset both on a watchdog timeout and in
>> system_restart.
>>
>> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html
>>
>> Cc: Lucas Stach <l.stach@pengutronix.de>
>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>> ---
>>  .../devicetree/bindings/watchdog/fsl-imx-wdt.txt     |  2 ++
>>  drivers/watchdog/imx2_wdt.c                          | 20 ++++++++++++++++++--
>>  2 files changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>> index 8dab6fd..9b89b3a 100644
>> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>> @@ -9,6 +9,8 @@ Optional property:
>
> properties ?
>
>>  - big-endian: If present the watchdog device's registers are implemented
>>    in big endian mode, otherwise in native mode(same with CPU), for more
>>    detail please see: Documentation/devicetree/bindings/regmap/regmap.txt.
>> +- ext-reset-output: If present the watchdog device is configured to assert its
>
> Should that have a vendor prefix ? Also, not sure if "-output"
> has any real value in the property name. "fsl,external-reset", maybe ?

Hi Guenter,

I don't see why a vendor prefix is necessary - its a feature of the
IMX6 watchdog supported by this driver to be able to trigger an
internal chip-level reset and/or an external signal that can be hooked
to additional hardware.

I started with ext-reset, but it was suggested
(http://lists.infradead.org/pipermail/linux-arm-kernel/2015-May/347437.html)
to also make it clear that this is an 'output' and not a reset input.

>
>> +  external reset (WDOG_B) instead of issuing a software reset.
>>
>>  Examples:
>>
>> diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
>> index 29ef719..84bfec4 100644
>> --- a/drivers/watchdog/imx2_wdt.c
>> +++ b/drivers/watchdog/imx2_wdt.c
>> @@ -41,6 +41,8 @@
>>
>>  #define IMX2_WDT_WCR         0x00            /* Control Register */
>>  #define IMX2_WDT_WCR_WT              (0xFF << 8)     /* -> Watchdog Timeout Field */
>> +#define IMX2_WDT_WCR_WDA     (1 << 5)        /* -> External Reset WDOG_B */
>> +#define IMX2_WDT_WCR_SRS     (1 << 4)        /* -> Software Reset Signal */
>>  #define IMX2_WDT_WCR_WRE     (1 << 3)        /* -> WDOG Reset Enable */
>>  #define IMX2_WDT_WCR_WDE     (1 << 2)        /* -> Watchdog Enable */
>>  #define IMX2_WDT_WCR_WDZST   (1 << 0)        /* -> Watchdog timer Suspend */
>> @@ -65,6 +67,7 @@ struct imx2_wdt_device {
>>       struct timer_list timer;        /* Pings the watchdog when closed */
>>       struct watchdog_device wdog;
>>       struct notifier_block restart_handler;
>> +     bool ext_reset;
>>  };
>>
>>  static bool nowayout = WATCHDOG_NOWAYOUT;
>> @@ -90,6 +93,13 @@ static int imx2_restart_handler(struct notifier_block *this, unsigned long mode,
>>       struct imx2_wdt_device *wdev = container_of(this,
>>                                                   struct imx2_wdt_device,
>>                                                   restart_handler);
>> +
>> +     /* Use internal reset or external - not both */
>> +     if (wdev->ext_reset)
>> +             wcr_enable |= IMX2_WDT_WCR_SRS; /* do not assert int reset */
>> +     else
>> +             wcr_enable |= IMX2_WDT_WCR_WDA; /* do not assert ext-reset */
>> +
>>       /* Assert SRS signal */
>>       regmap_write(wdev->regmap, IMX2_WDT_WCR, wcr_enable);
>>       /*
>> @@ -119,8 +129,12 @@ static inline void imx2_wdt_setup(struct watchdog_device *wdog)
>>       val |= IMX2_WDT_WCR_WDZST;
>>       /* Strip the old watchdog Time-Out value */
>>       val &= ~IMX2_WDT_WCR_WT;
>> -     /* Generate reset if WDOG times out */
>> -     val &= ~IMX2_WDT_WCR_WRE;
>> +     /* Generate internal chip-level reset if WDOG times out */
>> +     if (!wdev->ext_reset)
>> +             val &= ~IMX2_WDT_WCR_WRE;
>> +     /* Or if external-reset assert WDOG_B reset only on time-out */
>> +     else
>> +             val |= IMX2_WDT_WCR_WRE;
>
> I don't really understand what this means. Normally I would hope that a watchdog
> only generates a reset if/when it times out.

I think we went a couple of rounds on the info in the comment as well
in the previous patch reversions.

It is a feature of the IMX6 watchdog supported by this driver to be
able to trigger an internal chip-level reset and/or an external signal
that can be hooked to additional hardware.

In the case of using a PMIC that can regulate it's own rails (ie the
Freescale SabreSD reference design) an SoC chip-level reset will not
reset the PMIC and thus if the PMIC's rails have been driven down by
DVFS to a value below the necessary value for the chip to come up you
will hang (which is certainly not what you want to do when a watchdog
timeout occurs). The solution for designs that have a PMIC that is
able to change its voltage levels is to 'not' have the SoC internally
reset and to 'instead' drive an external reset signal that should
reset the PMIC (and possibly other chips if needed). The reason for
not allowing the internal reset is because as soon as the SoC goes
into reset it de-asserts the external reset which makes the time the
output is asserted un-deterministic.

Tim

[PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Tim Harvey]

On Thu, Nov 5, 2015 at 2:23 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Thu, Nov 05, 2015 at 04:19:21PM -0500, Akshay Bhat wrote:
>> From: Tim Harvey <tharvey@gateworks.com>
>>
>> The IMX6 watchdog supports assertion of a signal (WDOG_B) which
>> can be pinmux'd to an external pin. This is typically used for boards that
>> have PMIC's in control of the IMX6 power rails. In fact, failure to use
>> such an external reset on boards with external PMIC's can result in various
>> hangs due to the IMX6 not being fully reset [1] as well as the board failing
>> to reset because its PMIC has not been reset to provide adequate voltage for
>> the CPU when coming out of reset at 800Mhz.
>>
>> This uses a new device-tree property 'ext-reset-output' to indicate the
>> board has such a reset and to cause the watchdog to be configured to assert
>> WDOG_B instead of an internal reset both on a watchdog timeout and in
>> system_restart.
>>
>> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html
>>
>> Cc: Lucas Stach <l.stach@pengutronix.de>
>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>> ---
>>  .../devicetree/bindings/watchdog/fsl-imx-wdt.txt     |  2 ++
>>  drivers/watchdog/imx2_wdt.c                          | 20 ++++++++++++++++++--
>>  2 files changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>> index 8dab6fd..9b89b3a 100644
>> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>> @@ -9,6 +9,8 @@ Optional property:
>
> properties ?
>
>>  - big-endian: If present the watchdog device's registers are implemented
>>    in big endian mode, otherwise in native mode(same with CPU), for more
>>    detail please see: Documentation/devicetree/bindings/regmap/regmap.txt.
>> +- ext-reset-output: If present the watchdog device is configured to assert its
>
> Should that have a vendor prefix ? Also, not sure if "-output"
> has any real value in the property name. "fsl,external-reset", maybe ?

Hi Guenter,

I don't see why a vendor prefix is necessary - its a feature of the
IMX6 watchdog supported by this driver to be able to trigger an
internal chip-level reset and/or an external signal that can be hooked
to additional hardware.

I started with ext-reset, but it was suggested
(http://lists.infradead.org/pipermail/linux-arm-kernel/2015-May/347437.html)
to also make it clear that this is an 'output' and not a reset input.

>
>> +  external reset (WDOG_B) instead of issuing a software reset.
>>
>>  Examples:
>>
>> diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
>> index 29ef719..84bfec4 100644
>> --- a/drivers/watchdog/imx2_wdt.c
>> +++ b/drivers/watchdog/imx2_wdt.c
>> @@ -41,6 +41,8 @@
>>
>>  #define IMX2_WDT_WCR         0x00            /* Control Register */
>>  #define IMX2_WDT_WCR_WT              (0xFF << 8)     /* -> Watchdog Timeout Field */
>> +#define IMX2_WDT_WCR_WDA     (1 << 5)        /* -> External Reset WDOG_B */
>> +#define IMX2_WDT_WCR_SRS     (1 << 4)        /* -> Software Reset Signal */
>>  #define IMX2_WDT_WCR_WRE     (1 << 3)        /* -> WDOG Reset Enable */
>>  #define IMX2_WDT_WCR_WDE     (1 << 2)        /* -> Watchdog Enable */
>>  #define IMX2_WDT_WCR_WDZST   (1 << 0)        /* -> Watchdog timer Suspend */
>> @@ -65,6 +67,7 @@ struct imx2_wdt_device {
>>       struct timer_list timer;        /* Pings the watchdog when closed */
>>       struct watchdog_device wdog;
>>       struct notifier_block restart_handler;
>> +     bool ext_reset;
>>  };
>>
>>  static bool nowayout = WATCHDOG_NOWAYOUT;
>> @@ -90,6 +93,13 @@ static int imx2_restart_handler(struct notifier_block *this, unsigned long mode,
>>       struct imx2_wdt_device *wdev = container_of(this,
>>                                                   struct imx2_wdt_device,
>>                                                   restart_handler);
>> +
>> +     /* Use internal reset or external - not both */
>> +     if (wdev->ext_reset)
>> +             wcr_enable |= IMX2_WDT_WCR_SRS; /* do not assert int reset */
>> +     else
>> +             wcr_enable |= IMX2_WDT_WCR_WDA; /* do not assert ext-reset */
>> +
>>       /* Assert SRS signal */
>>       regmap_write(wdev->regmap, IMX2_WDT_WCR, wcr_enable);
>>       /*
>> @@ -119,8 +129,12 @@ static inline void imx2_wdt_setup(struct watchdog_device *wdog)
>>       val |= IMX2_WDT_WCR_WDZST;
>>       /* Strip the old watchdog Time-Out value */
>>       val &= ~IMX2_WDT_WCR_WT;
>> -     /* Generate reset if WDOG times out */
>> -     val &= ~IMX2_WDT_WCR_WRE;
>> +     /* Generate internal chip-level reset if WDOG times out */
>> +     if (!wdev->ext_reset)
>> +             val &= ~IMX2_WDT_WCR_WRE;
>> +     /* Or if external-reset assert WDOG_B reset only on time-out */
>> +     else
>> +             val |= IMX2_WDT_WCR_WRE;
>
> I don't really understand what this means. Normally I would hope that a watchdog
> only generates a reset if/when it times out.

I think we went a couple of rounds on the info in the comment as well
in the previous patch reversions.

It is a feature of the IMX6 watchdog supported by this driver to be
able to trigger an internal chip-level reset and/or an external signal
that can be hooked to additional hardware.

In the case of using a PMIC that can regulate it's own rails (ie the
Freescale SabreSD reference design) an SoC chip-level reset will not
reset the PMIC and thus if the PMIC's rails have been driven down by
DVFS to a value below the necessary value for the chip to come up you
will hang (which is certainly not what you want to do when a watchdog
timeout occurs). The solution for designs that have a PMIC that is
able to change its voltage levels is to 'not' have the SoC internally
reset and to 'instead' drive an external reset signal that should
reset the PMIC (and possibly other chips if needed). The reason for
not allowing the internal reset is because as soon as the SoC goes
into reset it de-asserts the external reset which makes the time the
output is asserted un-deterministic.

Tim

Re: [PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Guenter Roeck]

On Fri, Nov 06, 2015 at 11:53:42AM -0800, Tim Harvey wrote:
> On Thu, Nov 5, 2015 at 2:23 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> > On Thu, Nov 05, 2015 at 04:19:21PM -0500, Akshay Bhat wrote:
> >> From: Tim Harvey <tharvey@gateworks.com>
> >>
> >> The IMX6 watchdog supports assertion of a signal (WDOG_B) which
> >> can be pinmux'd to an external pin. This is typically used for boards that
> >> have PMIC's in control of the IMX6 power rails. In fact, failure to use
> >> such an external reset on boards with external PMIC's can result in various
> >> hangs due to the IMX6 not being fully reset [1] as well as the board failing
> >> to reset because its PMIC has not been reset to provide adequate voltage for
> >> the CPU when coming out of reset at 800Mhz.
> >>
> >> This uses a new device-tree property 'ext-reset-output' to indicate the
> >> board has such a reset and to cause the watchdog to be configured to assert
> >> WDOG_B instead of an internal reset both on a watchdog timeout and in
> >> system_restart.
> >>
> >> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html
> >>
> >> Cc: Lucas Stach <l.stach@pengutronix.de>
> >> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> >> ---
> >>  .../devicetree/bindings/watchdog/fsl-imx-wdt.txt     |  2 ++
> >>  drivers/watchdog/imx2_wdt.c                          | 20 ++++++++++++++++++--
> >>  2 files changed, 20 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> >> index 8dab6fd..9b89b3a 100644
> >> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> >> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> >> @@ -9,6 +9,8 @@ Optional property:
> >
> > properties ?
> >
> >>  - big-endian: If present the watchdog device's registers are implemented
> >>    in big endian mode, otherwise in native mode(same with CPU), for more
> >>    detail please see: Documentation/devicetree/bindings/regmap/regmap.txt.
> >> +- ext-reset-output: If present the watchdog device is configured to assert its
> >
> > Should that have a vendor prefix ? Also, not sure if "-output"
> > has any real value in the property name. "fsl,external-reset", maybe ?
> 
> Hi Guenter,
> 
> I don't see why a vendor prefix is necessary - its a feature of the
> IMX6 watchdog supported by this driver to be able to trigger an
> internal chip-level reset and/or an external signal that can be hooked
> to additional hardware.
> 
Sounded like vendor specific to me, but then I am not a devicetree maintainer,
so I am not an authority on the subject.

> I started with ext-reset, but it was suggested
> (http://lists.infradead.org/pipermail/linux-arm-kernel/2015-May/347437.html)
> to also make it clear that this is an 'output' and not a reset input.
> 
No problem, as long as you get an Ack from a devicetree maintainer.

Guenter

Re: [PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Guenter Roeck]

On Fri, Nov 06, 2015 at 11:53:42AM -0800, Tim Harvey wrote:
> On Thu, Nov 5, 2015 at 2:23 PM, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
> > On Thu, Nov 05, 2015 at 04:19:21PM -0500, Akshay Bhat wrote:
> >> From: Tim Harvey <tharvey-UMMOYl/HMS+akBO8gow8eQ@public.gmane.org>
> >>
> >> The IMX6 watchdog supports assertion of a signal (WDOG_B) which
> >> can be pinmux'd to an external pin. This is typically used for boards that
> >> have PMIC's in control of the IMX6 power rails. In fact, failure to use
> >> such an external reset on boards with external PMIC's can result in various
> >> hangs due to the IMX6 not being fully reset [1] as well as the board failing
> >> to reset because its PMIC has not been reset to provide adequate voltage for
> >> the CPU when coming out of reset at 800Mhz.
> >>
> >> This uses a new device-tree property 'ext-reset-output' to indicate the
> >> board has such a reset and to cause the watchdog to be configured to assert
> >> WDOG_B instead of an internal reset both on a watchdog timeout and in
> >> system_restart.
> >>
> >> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html
> >>
> >> Cc: Lucas Stach <l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> >> Signed-off-by: Tim Harvey <tharvey-UMMOYl/HMS+akBO8gow8eQ@public.gmane.org>
> >> ---
> >>  .../devicetree/bindings/watchdog/fsl-imx-wdt.txt     |  2 ++
> >>  drivers/watchdog/imx2_wdt.c                          | 20 ++++++++++++++++++--
> >>  2 files changed, 20 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> >> index 8dab6fd..9b89b3a 100644
> >> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> >> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> >> @@ -9,6 +9,8 @@ Optional property:
> >
> > properties ?
> >
> >>  - big-endian: If present the watchdog device's registers are implemented
> >>    in big endian mode, otherwise in native mode(same with CPU), for more
> >>    detail please see: Documentation/devicetree/bindings/regmap/regmap.txt.
> >> +- ext-reset-output: If present the watchdog device is configured to assert its
> >
> > Should that have a vendor prefix ? Also, not sure if "-output"
> > has any real value in the property name. "fsl,external-reset", maybe ?
> 
> Hi Guenter,
> 
> I don't see why a vendor prefix is necessary - its a feature of the
> IMX6 watchdog supported by this driver to be able to trigger an
> internal chip-level reset and/or an external signal that can be hooked
> to additional hardware.
> 
Sounded like vendor specific to me, but then I am not a devicetree maintainer,
so I am not an authority on the subject.

> I started with ext-reset, but it was suggested
> (http://lists.infradead.org/pipermail/linux-arm-kernel/2015-May/347437.html)
> to also make it clear that this is an 'output' and not a reset input.
> 
No problem, as long as you get an Ack from a devicetree maintainer.

Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" 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 v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Guenter Roeck]

On Fri, Nov 06, 2015 at 11:53:42AM -0800, Tim Harvey wrote:
> On Thu, Nov 5, 2015 at 2:23 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> > On Thu, Nov 05, 2015 at 04:19:21PM -0500, Akshay Bhat wrote:
> >> From: Tim Harvey <tharvey@gateworks.com>
> >>
> >> The IMX6 watchdog supports assertion of a signal (WDOG_B) which
> >> can be pinmux'd to an external pin. This is typically used for boards that
> >> have PMIC's in control of the IMX6 power rails. In fact, failure to use
> >> such an external reset on boards with external PMIC's can result in various
> >> hangs due to the IMX6 not being fully reset [1] as well as the board failing
> >> to reset because its PMIC has not been reset to provide adequate voltage for
> >> the CPU when coming out of reset at 800Mhz.
> >>
> >> This uses a new device-tree property 'ext-reset-output' to indicate the
> >> board has such a reset and to cause the watchdog to be configured to assert
> >> WDOG_B instead of an internal reset both on a watchdog timeout and in
> >> system_restart.
> >>
> >> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html
> >>
> >> Cc: Lucas Stach <l.stach@pengutronix.de>
> >> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> >> ---
> >>  .../devicetree/bindings/watchdog/fsl-imx-wdt.txt     |  2 ++
> >>  drivers/watchdog/imx2_wdt.c                          | 20 ++++++++++++++++++--
> >>  2 files changed, 20 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> >> index 8dab6fd..9b89b3a 100644
> >> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> >> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> >> @@ -9,6 +9,8 @@ Optional property:
> >
> > properties ?
> >
> >>  - big-endian: If present the watchdog device's registers are implemented
> >>    in big endian mode, otherwise in native mode(same with CPU), for more
> >>    detail please see: Documentation/devicetree/bindings/regmap/regmap.txt.
> >> +- ext-reset-output: If present the watchdog device is configured to assert its
> >
> > Should that have a vendor prefix ? Also, not sure if "-output"
> > has any real value in the property name. "fsl,external-reset", maybe ?
> 
> Hi Guenter,
> 
> I don't see why a vendor prefix is necessary - its a feature of the
> IMX6 watchdog supported by this driver to be able to trigger an
> internal chip-level reset and/or an external signal that can be hooked
> to additional hardware.
> 
Sounded like vendor specific to me, but then I am not a devicetree maintainer,
so I am not an authority on the subject.

> I started with ext-reset, but it was suggested
> (http://lists.infradead.org/pipermail/linux-arm-kernel/2015-May/347437.html)
> to also make it clear that this is an 'output' and not a reset input.
> 
No problem, as long as you get an Ack from a devicetree maintainer.

Guenter

[PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Guenter Roeck]

On Fri, Nov 06, 2015 at 11:53:42AM -0800, Tim Harvey wrote:
> On Thu, Nov 5, 2015 at 2:23 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> > On Thu, Nov 05, 2015 at 04:19:21PM -0500, Akshay Bhat wrote:
> >> From: Tim Harvey <tharvey@gateworks.com>
> >>
> >> The IMX6 watchdog supports assertion of a signal (WDOG_B) which
> >> can be pinmux'd to an external pin. This is typically used for boards that
> >> have PMIC's in control of the IMX6 power rails. In fact, failure to use
> >> such an external reset on boards with external PMIC's can result in various
> >> hangs due to the IMX6 not being fully reset [1] as well as the board failing
> >> to reset because its PMIC has not been reset to provide adequate voltage for
> >> the CPU when coming out of reset at 800Mhz.
> >>
> >> This uses a new device-tree property 'ext-reset-output' to indicate the
> >> board has such a reset and to cause the watchdog to be configured to assert
> >> WDOG_B instead of an internal reset both on a watchdog timeout and in
> >> system_restart.
> >>
> >> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html
> >>
> >> Cc: Lucas Stach <l.stach@pengutronix.de>
> >> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> >> ---
> >>  .../devicetree/bindings/watchdog/fsl-imx-wdt.txt     |  2 ++
> >>  drivers/watchdog/imx2_wdt.c                          | 20 ++++++++++++++++++--
> >>  2 files changed, 20 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> >> index 8dab6fd..9b89b3a 100644
> >> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> >> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> >> @@ -9,6 +9,8 @@ Optional property:
> >
> > properties ?
> >
> >>  - big-endian: If present the watchdog device's registers are implemented
> >>    in big endian mode, otherwise in native mode(same with CPU), for more
> >>    detail please see: Documentation/devicetree/bindings/regmap/regmap.txt.
> >> +- ext-reset-output: If present the watchdog device is configured to assert its
> >
> > Should that have a vendor prefix ? Also, not sure if "-output"
> > has any real value in the property name. "fsl,external-reset", maybe ?
> 
> Hi Guenter,
> 
> I don't see why a vendor prefix is necessary - its a feature of the
> IMX6 watchdog supported by this driver to be able to trigger an
> internal chip-level reset and/or an external signal that can be hooked
> to additional hardware.
> 
Sounded like vendor specific to me, but then I am not a devicetree maintainer,
so I am not an authority on the subject.

> I started with ext-reset, but it was suggested
> (http://lists.infradead.org/pipermail/linux-arm-kernel/2015-May/347437.html)
> to also make it clear that this is an 'output' and not a reset input.
> 
No problem, as long as you get an Ack from a devicetree maintainer.

Guenter

Re: [PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Akshay Bhat]

On 11/06/2015 05:02 PM, Guenter Roeck wrote:
> On Fri, Nov 06, 2015 at 11:53:42AM -0800, Tim Harvey wrote:
>> On Thu, Nov 5, 2015 at 2:23 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>> On Thu, Nov 05, 2015 at 04:19:21PM -0500, Akshay Bhat wrote:
>>>> From: Tim Harvey <tharvey@gateworks.com>
>>>>
>>>> The IMX6 watchdog supports assertion of a signal (WDOG_B) which
>>>> can be pinmux'd to an external pin. This is typically used for boards that
>>>> have PMIC's in control of the IMX6 power rails. In fact, failure to use
>>>> such an external reset on boards with external PMIC's can result in various
>>>> hangs due to the IMX6 not being fully reset [1] as well as the board failing
>>>> to reset because its PMIC has not been reset to provide adequate voltage for
>>>> the CPU when coming out of reset at 800Mhz.
>>>>
>>>> This uses a new device-tree property 'ext-reset-output' to indicate the
>>>> board has such a reset and to cause the watchdog to be configured to assert
>>>> WDOG_B instead of an internal reset both on a watchdog timeout and in
>>>> system_restart.
>>>>
>>>> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html
>>>>
>>>> Cc: Lucas Stach <l.stach@pengutronix.de>
>>>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>>>> ---
>>>>   .../devicetree/bindings/watchdog/fsl-imx-wdt.txt     |  2 ++
>>>>   drivers/watchdog/imx2_wdt.c                          | 20 ++++++++++++++++++--
>>>>   2 files changed, 20 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>> index 8dab6fd..9b89b3a 100644
>>>> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>> @@ -9,6 +9,8 @@ Optional property:
>>>
>>> properties ?
>>>
>>>>   - big-endian: If present the watchdog device's registers are implemented
>>>>     in big endian mode, otherwise in native mode(same with CPU), for more
>>>>     detail please see: Documentation/devicetree/bindings/regmap/regmap.txt.
>>>> +- ext-reset-output: If present the watchdog device is configured to assert its
>>>
>>> Should that have a vendor prefix ? Also, not sure if "-output"
>>> has any real value in the property name. "fsl,external-reset", maybe ?
>>
>> Hi Guenter,
>>
>> I don't see why a vendor prefix is necessary - its a feature of the
>> IMX6 watchdog supported by this driver to be able to trigger an
>> internal chip-level reset and/or an external signal that can be hooked
>> to additional hardware.
>>
> Sounded like vendor specific to me, but then I am not a devicetree maintainer,
> so I am not an authority on the subject.

Devicetree maintainers,

Any thoughts?

Tim,

After looking at all the other watchdog drivers, it does not appear that 
there is any other processor which uses a similar feature. Since imx is 
the only processor that appears to support this feature, it might make 
sense in making this vendor specific. If in the future it is found more 
processors support a similar functionality, it can be revisited and 
moved out from being vendor specific?

>
>> I started with ext-reset, but it was suggested
>> (http://lists.infradead.org/pipermail/linux-arm-kernel/2015-May/347437.html)
>> to also make it clear that this is an 'output' and not a reset input.
>>
> No problem, as long as you get an Ack from a devicetree maintainer.
>
> Guenter
>

Re: [PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Akshay Bhat]

On 11/06/2015 05:02 PM, Guenter Roeck wrote:
> On Fri, Nov 06, 2015 at 11:53:42AM -0800, Tim Harvey wrote:
>> On Thu, Nov 5, 2015 at 2:23 PM, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
>>> On Thu, Nov 05, 2015 at 04:19:21PM -0500, Akshay Bhat wrote:
>>>> From: Tim Harvey <tharvey-UMMOYl/HMS+akBO8gow8eQ@public.gmane.org>
>>>>
>>>> The IMX6 watchdog supports assertion of a signal (WDOG_B) which
>>>> can be pinmux'd to an external pin. This is typically used for boards that
>>>> have PMIC's in control of the IMX6 power rails. In fact, failure to use
>>>> such an external reset on boards with external PMIC's can result in various
>>>> hangs due to the IMX6 not being fully reset [1] as well as the board failing
>>>> to reset because its PMIC has not been reset to provide adequate voltage for
>>>> the CPU when coming out of reset at 800Mhz.
>>>>
>>>> This uses a new device-tree property 'ext-reset-output' to indicate the
>>>> board has such a reset and to cause the watchdog to be configured to assert
>>>> WDOG_B instead of an internal reset both on a watchdog timeout and in
>>>> system_restart.
>>>>
>>>> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html
>>>>
>>>> Cc: Lucas Stach <l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>>>> Signed-off-by: Tim Harvey <tharvey-UMMOYl/HMS+akBO8gow8eQ@public.gmane.org>
>>>> ---
>>>>   .../devicetree/bindings/watchdog/fsl-imx-wdt.txt     |  2 ++
>>>>   drivers/watchdog/imx2_wdt.c                          | 20 ++++++++++++++++++--
>>>>   2 files changed, 20 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>> index 8dab6fd..9b89b3a 100644
>>>> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>> @@ -9,6 +9,8 @@ Optional property:
>>>
>>> properties ?
>>>
>>>>   - big-endian: If present the watchdog device's registers are implemented
>>>>     in big endian mode, otherwise in native mode(same with CPU), for more
>>>>     detail please see: Documentation/devicetree/bindings/regmap/regmap.txt.
>>>> +- ext-reset-output: If present the watchdog device is configured to assert its
>>>
>>> Should that have a vendor prefix ? Also, not sure if "-output"
>>> has any real value in the property name. "fsl,external-reset", maybe ?
>>
>> Hi Guenter,
>>
>> I don't see why a vendor prefix is necessary - its a feature of the
>> IMX6 watchdog supported by this driver to be able to trigger an
>> internal chip-level reset and/or an external signal that can be hooked
>> to additional hardware.
>>
> Sounded like vendor specific to me, but then I am not a devicetree maintainer,
> so I am not an authority on the subject.

Devicetree maintainers,

Any thoughts?

Tim,

After looking at all the other watchdog drivers, it does not appear that 
there is any other processor which uses a similar feature. Since imx is 
the only processor that appears to support this feature, it might make 
sense in making this vendor specific. If in the future it is found more 
processors support a similar functionality, it can be revisited and 
moved out from being vendor specific?

>
>> I started with ext-reset, but it was suggested
>> (http://lists.infradead.org/pipermail/linux-arm-kernel/2015-May/347437.html)
>> to also make it clear that this is an 'output' and not a reset input.
>>
> No problem, as long as you get an Ack from a devicetree maintainer.
>
> Guenter
>
--
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 v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Akshay Bhat]

On 11/06/2015 05:02 PM, Guenter Roeck wrote:
> On Fri, Nov 06, 2015 at 11:53:42AM -0800, Tim Harvey wrote:
>> On Thu, Nov 5, 2015 at 2:23 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>> On Thu, Nov 05, 2015 at 04:19:21PM -0500, Akshay Bhat wrote:
>>>> From: Tim Harvey <tharvey@gateworks.com>
>>>>
>>>> The IMX6 watchdog supports assertion of a signal (WDOG_B) which
>>>> can be pinmux'd to an external pin. This is typically used for boards that
>>>> have PMIC's in control of the IMX6 power rails. In fact, failure to use
>>>> such an external reset on boards with external PMIC's can result in various
>>>> hangs due to the IMX6 not being fully reset [1] as well as the board failing
>>>> to reset because its PMIC has not been reset to provide adequate voltage for
>>>> the CPU when coming out of reset at 800Mhz.
>>>>
>>>> This uses a new device-tree property 'ext-reset-output' to indicate the
>>>> board has such a reset and to cause the watchdog to be configured to assert
>>>> WDOG_B instead of an internal reset both on a watchdog timeout and in
>>>> system_restart.
>>>>
>>>> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html
>>>>
>>>> Cc: Lucas Stach <l.stach@pengutronix.de>
>>>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>>>> ---
>>>>   .../devicetree/bindings/watchdog/fsl-imx-wdt.txt     |  2 ++
>>>>   drivers/watchdog/imx2_wdt.c                          | 20 ++++++++++++++++++--
>>>>   2 files changed, 20 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>> index 8dab6fd..9b89b3a 100644
>>>> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>> @@ -9,6 +9,8 @@ Optional property:
>>>
>>> properties ?
>>>
>>>>   - big-endian: If present the watchdog device's registers are implemented
>>>>     in big endian mode, otherwise in native mode(same with CPU), for more
>>>>     detail please see: Documentation/devicetree/bindings/regmap/regmap.txt.
>>>> +- ext-reset-output: If present the watchdog device is configured to assert its
>>>
>>> Should that have a vendor prefix ? Also, not sure if "-output"
>>> has any real value in the property name. "fsl,external-reset", maybe ?
>>
>> Hi Guenter,
>>
>> I don't see why a vendor prefix is necessary - its a feature of the
>> IMX6 watchdog supported by this driver to be able to trigger an
>> internal chip-level reset and/or an external signal that can be hooked
>> to additional hardware.
>>
> Sounded like vendor specific to me, but then I am not a devicetree maintainer,
> so I am not an authority on the subject.

Devicetree maintainers,

Any thoughts?

Tim,

After looking at all the other watchdog drivers, it does not appear that 
there is any other processor which uses a similar feature. Since imx is 
the only processor that appears to support this feature, it might make 
sense in making this vendor specific. If in the future it is found more 
processors support a similar functionality, it can be revisited and 
moved out from being vendor specific?

>
>> I started with ext-reset, but it was suggested
>> (http://lists.infradead.org/pipermail/linux-arm-kernel/2015-May/347437.html)
>> to also make it clear that this is an 'output' and not a reset input.
>>
> No problem, as long as you get an Ack from a devicetree maintainer.
>
> Guenter
>

[PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Akshay Bhat]

On 11/06/2015 05:02 PM, Guenter Roeck wrote:
> On Fri, Nov 06, 2015 at 11:53:42AM -0800, Tim Harvey wrote:
>> On Thu, Nov 5, 2015 at 2:23 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>> On Thu, Nov 05, 2015 at 04:19:21PM -0500, Akshay Bhat wrote:
>>>> From: Tim Harvey <tharvey@gateworks.com>
>>>>
>>>> The IMX6 watchdog supports assertion of a signal (WDOG_B) which
>>>> can be pinmux'd to an external pin. This is typically used for boards that
>>>> have PMIC's in control of the IMX6 power rails. In fact, failure to use
>>>> such an external reset on boards with external PMIC's can result in various
>>>> hangs due to the IMX6 not being fully reset [1] as well as the board failing
>>>> to reset because its PMIC has not been reset to provide adequate voltage for
>>>> the CPU when coming out of reset at 800Mhz.
>>>>
>>>> This uses a new device-tree property 'ext-reset-output' to indicate the
>>>> board has such a reset and to cause the watchdog to be configured to assert
>>>> WDOG_B instead of an internal reset both on a watchdog timeout and in
>>>> system_restart.
>>>>
>>>> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html
>>>>
>>>> Cc: Lucas Stach <l.stach@pengutronix.de>
>>>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>>>> ---
>>>>   .../devicetree/bindings/watchdog/fsl-imx-wdt.txt     |  2 ++
>>>>   drivers/watchdog/imx2_wdt.c                          | 20 ++++++++++++++++++--
>>>>   2 files changed, 20 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>> index 8dab6fd..9b89b3a 100644
>>>> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>> @@ -9,6 +9,8 @@ Optional property:
>>>
>>> properties ?
>>>
>>>>   - big-endian: If present the watchdog device's registers are implemented
>>>>     in big endian mode, otherwise in native mode(same with CPU), for more
>>>>     detail please see: Documentation/devicetree/bindings/regmap/regmap.txt.
>>>> +- ext-reset-output: If present the watchdog device is configured to assert its
>>>
>>> Should that have a vendor prefix ? Also, not sure if "-output"
>>> has any real value in the property name. "fsl,external-reset", maybe ?
>>
>> Hi Guenter,
>>
>> I don't see why a vendor prefix is necessary - its a feature of the
>> IMX6 watchdog supported by this driver to be able to trigger an
>> internal chip-level reset and/or an external signal that can be hooked
>> to additional hardware.
>>
> Sounded like vendor specific to me, but then I am not a devicetree maintainer,
> so I am not an authority on the subject.

Devicetree maintainers,

Any thoughts?

Tim,

After looking at all the other watchdog drivers, it does not appear that 
there is any other processor which uses a similar feature. Since imx is 
the only processor that appears to support this feature, it might make 
sense in making this vendor specific. If in the future it is found more 
processors support a similar functionality, it can be revisited and 
moved out from being vendor specific?

>
>> I started with ext-reset, but it was suggested
>> (http://lists.infradead.org/pipermail/linux-arm-kernel/2015-May/347437.html)
>> to also make it clear that this is an 'output' and not a reset input.
>>
> No problem, as long as you get an Ack from a devicetree maintainer.
>
> Guenter
>

Re: [PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Tim Harvey]

On Wed, Dec 2, 2015 at 11:11 AM, Akshay Bhat <akshay.bhat@timesys.com> wrote:
>
>
> On 11/06/2015 05:02 PM, Guenter Roeck wrote:
>>
>> On Fri, Nov 06, 2015 at 11:53:42AM -0800, Tim Harvey wrote:
>>>
>>> On Thu, Nov 5, 2015 at 2:23 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>
>>>> On Thu, Nov 05, 2015 at 04:19:21PM -0500, Akshay Bhat wrote:
>>>>>
>>>>> From: Tim Harvey <tharvey@gateworks.com>
>>>>>
>>>>> The IMX6 watchdog supports assertion of a signal (WDOG_B) which
>>>>> can be pinmux'd to an external pin. This is typically used for boards
>>>>> that
>>>>> have PMIC's in control of the IMX6 power rails. In fact, failure to use
>>>>> such an external reset on boards with external PMIC's can result in
>>>>> various
>>>>> hangs due to the IMX6 not being fully reset [1] as well as the board
>>>>> failing
>>>>> to reset because its PMIC has not been reset to provide adequate
>>>>> voltage for
>>>>> the CPU when coming out of reset at 800Mhz.
>>>>>
>>>>> This uses a new device-tree property 'ext-reset-output' to indicate the
>>>>> board has such a reset and to cause the watchdog to be configured to
>>>>> assert
>>>>> WDOG_B instead of an internal reset both on a watchdog timeout and in
>>>>> system_restart.
>>>>>
>>>>> [1]
>>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html
>>>>>
>>>>> Cc: Lucas Stach <l.stach@pengutronix.de>
>>>>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>>>>> ---
>>>>>   .../devicetree/bindings/watchdog/fsl-imx-wdt.txt     |  2 ++
>>>>>   drivers/watchdog/imx2_wdt.c                          | 20
>>>>> ++++++++++++++++++--
>>>>>   2 files changed, 20 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>> b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>> index 8dab6fd..9b89b3a 100644
>>>>> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>> @@ -9,6 +9,8 @@ Optional property:
>>>>
>>>>
>>>> properties ?
>>>>
>>>>>   - big-endian: If present the watchdog device's registers are
>>>>> implemented
>>>>>     in big endian mode, otherwise in native mode(same with CPU), for
>>>>> more
>>>>>     detail please see:
>>>>> Documentation/devicetree/bindings/regmap/regmap.txt.
>>>>> +- ext-reset-output: If present the watchdog device is configured to
>>>>> assert its
>>>>
>>>>
>>>> Should that have a vendor prefix ? Also, not sure if "-output"
>>>> has any real value in the property name. "fsl,external-reset", maybe ?
>>>
>>>
>>> Hi Guenter,
>>>
>>> I don't see why a vendor prefix is necessary - its a feature of the
>>> IMX6 watchdog supported by this driver to be able to trigger an
>>> internal chip-level reset and/or an external signal that can be hooked
>>> to additional hardware.
>>>
>> Sounded like vendor specific to me, but then I am not a devicetree
>> maintainer,
>> so I am not an authority on the subject.
>
>
> Devicetree maintainers,
>
> Any thoughts?
>
> Tim,
>
> After looking at all the other watchdog drivers, it does not appear that
> there is any other processor which uses a similar feature. Since imx is the
> only processor that appears to support this feature, it might make sense in
> making this vendor specific. If in the future it is found more processors
> support a similar functionality, it can be revisited and moved out from
> being vendor specific?
>

I'm certainly no expert on device-tree policy. I understand your
point, but realize that the driver in question is imx2_wdt.c
(compatible = "fsl,imx21-wdt"). This is an IP block inside the silicon
of only Freescale chips, so its not like a future omap chip would be
using this driver - only fsl devices. So why would it need a 'vendor'
property any more than its other properties?

Regards,

Tim

Re: [PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Tim Harvey]

On Wed, Dec 2, 2015 at 11:11 AM, Akshay Bhat <akshay.bhat-jEh4hwF5bVhBDgjK7y7TUQ@public.gmane.org> wrote:
>
>
> On 11/06/2015 05:02 PM, Guenter Roeck wrote:
>>
>> On Fri, Nov 06, 2015 at 11:53:42AM -0800, Tim Harvey wrote:
>>>
>>> On Thu, Nov 5, 2015 at 2:23 PM, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
>>>>
>>>> On Thu, Nov 05, 2015 at 04:19:21PM -0500, Akshay Bhat wrote:
>>>>>
>>>>> From: Tim Harvey <tharvey-UMMOYl/HMS+akBO8gow8eQ@public.gmane.org>
>>>>>
>>>>> The IMX6 watchdog supports assertion of a signal (WDOG_B) which
>>>>> can be pinmux'd to an external pin. This is typically used for boards
>>>>> that
>>>>> have PMIC's in control of the IMX6 power rails. In fact, failure to use
>>>>> such an external reset on boards with external PMIC's can result in
>>>>> various
>>>>> hangs due to the IMX6 not being fully reset [1] as well as the board
>>>>> failing
>>>>> to reset because its PMIC has not been reset to provide adequate
>>>>> voltage for
>>>>> the CPU when coming out of reset at 800Mhz.
>>>>>
>>>>> This uses a new device-tree property 'ext-reset-output' to indicate the
>>>>> board has such a reset and to cause the watchdog to be configured to
>>>>> assert
>>>>> WDOG_B instead of an internal reset both on a watchdog timeout and in
>>>>> system_restart.
>>>>>
>>>>> [1]
>>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html
>>>>>
>>>>> Cc: Lucas Stach <l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>>>>> Signed-off-by: Tim Harvey <tharvey-UMMOYl/HMS+akBO8gow8eQ@public.gmane.org>
>>>>> ---
>>>>>   .../devicetree/bindings/watchdog/fsl-imx-wdt.txt     |  2 ++
>>>>>   drivers/watchdog/imx2_wdt.c                          | 20
>>>>> ++++++++++++++++++--
>>>>>   2 files changed, 20 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>> b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>> index 8dab6fd..9b89b3a 100644
>>>>> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>> @@ -9,6 +9,8 @@ Optional property:
>>>>
>>>>
>>>> properties ?
>>>>
>>>>>   - big-endian: If present the watchdog device's registers are
>>>>> implemented
>>>>>     in big endian mode, otherwise in native mode(same with CPU), for
>>>>> more
>>>>>     detail please see:
>>>>> Documentation/devicetree/bindings/regmap/regmap.txt.
>>>>> +- ext-reset-output: If present the watchdog device is configured to
>>>>> assert its
>>>>
>>>>
>>>> Should that have a vendor prefix ? Also, not sure if "-output"
>>>> has any real value in the property name. "fsl,external-reset", maybe ?
>>>
>>>
>>> Hi Guenter,
>>>
>>> I don't see why a vendor prefix is necessary - its a feature of the
>>> IMX6 watchdog supported by this driver to be able to trigger an
>>> internal chip-level reset and/or an external signal that can be hooked
>>> to additional hardware.
>>>
>> Sounded like vendor specific to me, but then I am not a devicetree
>> maintainer,
>> so I am not an authority on the subject.
>
>
> Devicetree maintainers,
>
> Any thoughts?
>
> Tim,
>
> After looking at all the other watchdog drivers, it does not appear that
> there is any other processor which uses a similar feature. Since imx is the
> only processor that appears to support this feature, it might make sense in
> making this vendor specific. If in the future it is found more processors
> support a similar functionality, it can be revisited and moved out from
> being vendor specific?
>

I'm certainly no expert on device-tree policy. I understand your
point, but realize that the driver in question is imx2_wdt.c
(compatible = "fsl,imx21-wdt"). This is an IP block inside the silicon
of only Freescale chips, so its not like a future omap chip would be
using this driver - only fsl devices. So why would it need a 'vendor'
property any more than its other properties?

Regards,

Tim
--
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 v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Tim Harvey]

On Wed, Dec 2, 2015 at 11:11 AM, Akshay Bhat <akshay.bhat@timesys.com> wrote:
>
>
> On 11/06/2015 05:02 PM, Guenter Roeck wrote:
>>
>> On Fri, Nov 06, 2015 at 11:53:42AM -0800, Tim Harvey wrote:
>>>
>>> On Thu, Nov 5, 2015 at 2:23 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>
>>>> On Thu, Nov 05, 2015 at 04:19:21PM -0500, Akshay Bhat wrote:
>>>>>
>>>>> From: Tim Harvey <tharvey@gateworks.com>
>>>>>
>>>>> The IMX6 watchdog supports assertion of a signal (WDOG_B) which
>>>>> can be pinmux'd to an external pin. This is typically used for boards
>>>>> that
>>>>> have PMIC's in control of the IMX6 power rails. In fact, failure to use
>>>>> such an external reset on boards with external PMIC's can result in
>>>>> various
>>>>> hangs due to the IMX6 not being fully reset [1] as well as the board
>>>>> failing
>>>>> to reset because its PMIC has not been reset to provide adequate
>>>>> voltage for
>>>>> the CPU when coming out of reset at 800Mhz.
>>>>>
>>>>> This uses a new device-tree property 'ext-reset-output' to indicate the
>>>>> board has such a reset and to cause the watchdog to be configured to
>>>>> assert
>>>>> WDOG_B instead of an internal reset both on a watchdog timeout and in
>>>>> system_restart.
>>>>>
>>>>> [1]
>>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html
>>>>>
>>>>> Cc: Lucas Stach <l.stach@pengutronix.de>
>>>>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>>>>> ---
>>>>>   .../devicetree/bindings/watchdog/fsl-imx-wdt.txt     |  2 ++
>>>>>   drivers/watchdog/imx2_wdt.c                          | 20
>>>>> ++++++++++++++++++--
>>>>>   2 files changed, 20 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>> b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>> index 8dab6fd..9b89b3a 100644
>>>>> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>> @@ -9,6 +9,8 @@ Optional property:
>>>>
>>>>
>>>> properties ?
>>>>
>>>>>   - big-endian: If present the watchdog device's registers are
>>>>> implemented
>>>>>     in big endian mode, otherwise in native mode(same with CPU), for
>>>>> more
>>>>>     detail please see:
>>>>> Documentation/devicetree/bindings/regmap/regmap.txt.
>>>>> +- ext-reset-output: If present the watchdog device is configured to
>>>>> assert its
>>>>
>>>>
>>>> Should that have a vendor prefix ? Also, not sure if "-output"
>>>> has any real value in the property name. "fsl,external-reset", maybe ?
>>>
>>>
>>> Hi Guenter,
>>>
>>> I don't see why a vendor prefix is necessary - its a feature of the
>>> IMX6 watchdog supported by this driver to be able to trigger an
>>> internal chip-level reset and/or an external signal that can be hooked
>>> to additional hardware.
>>>
>> Sounded like vendor specific to me, but then I am not a devicetree
>> maintainer,
>> so I am not an authority on the subject.
>
>
> Devicetree maintainers,
>
> Any thoughts?
>
> Tim,
>
> After looking at all the other watchdog drivers, it does not appear that
> there is any other processor which uses a similar feature. Since imx is the
> only processor that appears to support this feature, it might make sense in
> making this vendor specific. If in the future it is found more processors
> support a similar functionality, it can be revisited and moved out from
> being vendor specific?
>

I'm certainly no expert on device-tree policy. I understand your
point, but realize that the driver in question is imx2_wdt.c
(compatible = "fsl,imx21-wdt"). This is an IP block inside the silicon
of only Freescale chips, so its not like a future omap chip would be
using this driver - only fsl devices. So why would it need a 'vendor'
property any more than its other properties?

Regards,

Tim

[PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Tim Harvey]

On Wed, Dec 2, 2015 at 11:11 AM, Akshay Bhat <akshay.bhat@timesys.com> wrote:
>
>
> On 11/06/2015 05:02 PM, Guenter Roeck wrote:
>>
>> On Fri, Nov 06, 2015 at 11:53:42AM -0800, Tim Harvey wrote:
>>>
>>> On Thu, Nov 5, 2015 at 2:23 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>
>>>> On Thu, Nov 05, 2015 at 04:19:21PM -0500, Akshay Bhat wrote:
>>>>>
>>>>> From: Tim Harvey <tharvey@gateworks.com>
>>>>>
>>>>> The IMX6 watchdog supports assertion of a signal (WDOG_B) which
>>>>> can be pinmux'd to an external pin. This is typically used for boards
>>>>> that
>>>>> have PMIC's in control of the IMX6 power rails. In fact, failure to use
>>>>> such an external reset on boards with external PMIC's can result in
>>>>> various
>>>>> hangs due to the IMX6 not being fully reset [1] as well as the board
>>>>> failing
>>>>> to reset because its PMIC has not been reset to provide adequate
>>>>> voltage for
>>>>> the CPU when coming out of reset at 800Mhz.
>>>>>
>>>>> This uses a new device-tree property 'ext-reset-output' to indicate the
>>>>> board has such a reset and to cause the watchdog to be configured to
>>>>> assert
>>>>> WDOG_B instead of an internal reset both on a watchdog timeout and in
>>>>> system_restart.
>>>>>
>>>>> [1]
>>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html
>>>>>
>>>>> Cc: Lucas Stach <l.stach@pengutronix.de>
>>>>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>>>>> ---
>>>>>   .../devicetree/bindings/watchdog/fsl-imx-wdt.txt     |  2 ++
>>>>>   drivers/watchdog/imx2_wdt.c                          | 20
>>>>> ++++++++++++++++++--
>>>>>   2 files changed, 20 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>> b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>> index 8dab6fd..9b89b3a 100644
>>>>> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>> @@ -9,6 +9,8 @@ Optional property:
>>>>
>>>>
>>>> properties ?
>>>>
>>>>>   - big-endian: If present the watchdog device's registers are
>>>>> implemented
>>>>>     in big endian mode, otherwise in native mode(same with CPU), for
>>>>> more
>>>>>     detail please see:
>>>>> Documentation/devicetree/bindings/regmap/regmap.txt.
>>>>> +- ext-reset-output: If present the watchdog device is configured to
>>>>> assert its
>>>>
>>>>
>>>> Should that have a vendor prefix ? Also, not sure if "-output"
>>>> has any real value in the property name. "fsl,external-reset", maybe ?
>>>
>>>
>>> Hi Guenter,
>>>
>>> I don't see why a vendor prefix is necessary - its a feature of the
>>> IMX6 watchdog supported by this driver to be able to trigger an
>>> internal chip-level reset and/or an external signal that can be hooked
>>> to additional hardware.
>>>
>> Sounded like vendor specific to me, but then I am not a devicetree
>> maintainer,
>> so I am not an authority on the subject.
>
>
> Devicetree maintainers,
>
> Any thoughts?
>
> Tim,
>
> After looking at all the other watchdog drivers, it does not appear that
> there is any other processor which uses a similar feature. Since imx is the
> only processor that appears to support this feature, it might make sense in
> making this vendor specific. If in the future it is found more processors
> support a similar functionality, it can be revisited and moved out from
> being vendor specific?
>

I'm certainly no expert on device-tree policy. I understand your
point, but realize that the driver in question is imx2_wdt.c
(compatible = "fsl,imx21-wdt"). This is an IP block inside the silicon
of only Freescale chips, so its not like a future omap chip would be
using this driver - only fsl devices. So why would it need a 'vendor'
property any more than its other properties?

Regards,

Tim

Re: [PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Tim Harvey]

On Wed, Dec 2, 2015 at 12:54 PM, Tim Harvey <tharvey@gateworks.com> wrote:
>
> On Wed, Dec 2, 2015 at 11:11 AM, Akshay Bhat <akshay.bhat@timesys.com> wrote:
> >
> >
> > On 11/06/2015 05:02 PM, Guenter Roeck wrote:
> >>
> >> On Fri, Nov 06, 2015 at 11:53:42AM -0800, Tim Harvey wrote:
> >>>
> >>> On Thu, Nov 5, 2015 at 2:23 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> >>>>
> >>>> On Thu, Nov 05, 2015 at 04:19:21PM -0500, Akshay Bhat wrote:
> >>>>>
> >>>>> From: Tim Harvey <tharvey@gateworks.com>
> >>>>>
> >>>>> The IMX6 watchdog supports assertion of a signal (WDOG_B) which
> >>>>> can be pinmux'd to an external pin. This is typically used for boards
> >>>>> that
> >>>>> have PMIC's in control of the IMX6 power rails. In fact, failure to use
> >>>>> such an external reset on boards with external PMIC's can result in
> >>>>> various
> >>>>> hangs due to the IMX6 not being fully reset [1] as well as the board
> >>>>> failing
> >>>>> to reset because its PMIC has not been reset to provide adequate
> >>>>> voltage for
> >>>>> the CPU when coming out of reset at 800Mhz.
> >>>>>
> >>>>> This uses a new device-tree property 'ext-reset-output' to indicate the
> >>>>> board has such a reset and to cause the watchdog to be configured to
> >>>>> assert
> >>>>> WDOG_B instead of an internal reset both on a watchdog timeout and in
> >>>>> system_restart.
> >>>>>
> >>>>> [1]
> >>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html
> >>>>>
> >>>>> Cc: Lucas Stach <l.stach@pengutronix.de>
> >>>>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> >>>>> ---
> >>>>>   .../devicetree/bindings/watchdog/fsl-imx-wdt.txt     |  2 ++
> >>>>>   drivers/watchdog/imx2_wdt.c                          | 20
> >>>>> ++++++++++++++++++--
> >>>>>   2 files changed, 20 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> >>>>> b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> >>>>> index 8dab6fd..9b89b3a 100644
> >>>>> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> >>>>> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> >>>>> @@ -9,6 +9,8 @@ Optional property:
> >>>>
> >>>>
> >>>> properties ?
> >>>>
> >>>>>   - big-endian: If present the watchdog device's registers are
> >>>>> implemented
> >>>>>     in big endian mode, otherwise in native mode(same with CPU), for
> >>>>> more
> >>>>>     detail please see:
> >>>>> Documentation/devicetree/bindings/regmap/regmap.txt.
> >>>>> +- ext-reset-output: If present the watchdog device is configured to
> >>>>> assert its
> >>>>
> >>>>
> >>>> Should that have a vendor prefix ? Also, not sure if "-output"
> >>>> has any real value in the property name. "fsl,external-reset", maybe ?
> >>>
> >>>
> >>> Hi Guenter,
> >>>
> >>> I don't see why a vendor prefix is necessary - its a feature of the
> >>> IMX6 watchdog supported by this driver to be able to trigger an
> >>> internal chip-level reset and/or an external signal that can be hooked
> >>> to additional hardware.
> >>>
> >> Sounded like vendor specific to me, but then I am not a devicetree
> >> maintainer,
> >> so I am not an authority on the subject.
> >
> >
> > Devicetree maintainers,
> >
> > Any thoughts?
> >
> > Tim,
> >
> > After looking at all the other watchdog drivers, it does not appear that
> > there is any other processor which uses a similar feature. Since imx is the
> > only processor that appears to support this feature, it might make sense in
> > making this vendor specific. If in the future it is found more processors
> > support a similar functionality, it can be revisited and moved out from
> > being vendor specific?
> >
>
> I'm certainly no expert on device-tree policy. I understand your
> point, but realize that the driver in question is imx2_wdt.c
> (compatible = "fsl,imx21-wdt"). This is an IP block inside the silicon
> of only Freescale chips, so its not like a future omap chip would be
> using this driver - only fsl devices. So why would it need a 'vendor'
> property any more than its other properties?
>
> Regards,
>
> Tim

Wim,

Does the lack of response mean overwhelming approval?

I haven't heard any valid complaints - what does it take to get this approved?

Regards,

Tim

Re: [PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Tim Harvey]

On Wed, Dec 2, 2015 at 12:54 PM, Tim Harvey <tharvey-UMMOYl/HMS+akBO8gow8eQ@public.gmane.org> wrote:
>
> On Wed, Dec 2, 2015 at 11:11 AM, Akshay Bhat <akshay.bhat-jEh4hwF5bVhBDgjK7y7TUQ@public.gmane.org> wrote:
> >
> >
> > On 11/06/2015 05:02 PM, Guenter Roeck wrote:
> >>
> >> On Fri, Nov 06, 2015 at 11:53:42AM -0800, Tim Harvey wrote:
> >>>
> >>> On Thu, Nov 5, 2015 at 2:23 PM, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
> >>>>
> >>>> On Thu, Nov 05, 2015 at 04:19:21PM -0500, Akshay Bhat wrote:
> >>>>>
> >>>>> From: Tim Harvey <tharvey-UMMOYl/HMS+akBO8gow8eQ@public.gmane.org>
> >>>>>
> >>>>> The IMX6 watchdog supports assertion of a signal (WDOG_B) which
> >>>>> can be pinmux'd to an external pin. This is typically used for boards
> >>>>> that
> >>>>> have PMIC's in control of the IMX6 power rails. In fact, failure to use
> >>>>> such an external reset on boards with external PMIC's can result in
> >>>>> various
> >>>>> hangs due to the IMX6 not being fully reset [1] as well as the board
> >>>>> failing
> >>>>> to reset because its PMIC has not been reset to provide adequate
> >>>>> voltage for
> >>>>> the CPU when coming out of reset at 800Mhz.
> >>>>>
> >>>>> This uses a new device-tree property 'ext-reset-output' to indicate the
> >>>>> board has such a reset and to cause the watchdog to be configured to
> >>>>> assert
> >>>>> WDOG_B instead of an internal reset both on a watchdog timeout and in
> >>>>> system_restart.
> >>>>>
> >>>>> [1]
> >>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html
> >>>>>
> >>>>> Cc: Lucas Stach <l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> >>>>> Signed-off-by: Tim Harvey <tharvey-UMMOYl/HMS+akBO8gow8eQ@public.gmane.org>
> >>>>> ---
> >>>>>   .../devicetree/bindings/watchdog/fsl-imx-wdt.txt     |  2 ++
> >>>>>   drivers/watchdog/imx2_wdt.c                          | 20
> >>>>> ++++++++++++++++++--
> >>>>>   2 files changed, 20 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> >>>>> b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> >>>>> index 8dab6fd..9b89b3a 100644
> >>>>> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> >>>>> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> >>>>> @@ -9,6 +9,8 @@ Optional property:
> >>>>
> >>>>
> >>>> properties ?
> >>>>
> >>>>>   - big-endian: If present the watchdog device's registers are
> >>>>> implemented
> >>>>>     in big endian mode, otherwise in native mode(same with CPU), for
> >>>>> more
> >>>>>     detail please see:
> >>>>> Documentation/devicetree/bindings/regmap/regmap.txt.
> >>>>> +- ext-reset-output: If present the watchdog device is configured to
> >>>>> assert its
> >>>>
> >>>>
> >>>> Should that have a vendor prefix ? Also, not sure if "-output"
> >>>> has any real value in the property name. "fsl,external-reset", maybe ?
> >>>
> >>>
> >>> Hi Guenter,
> >>>
> >>> I don't see why a vendor prefix is necessary - its a feature of the
> >>> IMX6 watchdog supported by this driver to be able to trigger an
> >>> internal chip-level reset and/or an external signal that can be hooked
> >>> to additional hardware.
> >>>
> >> Sounded like vendor specific to me, but then I am not a devicetree
> >> maintainer,
> >> so I am not an authority on the subject.
> >
> >
> > Devicetree maintainers,
> >
> > Any thoughts?
> >
> > Tim,
> >
> > After looking at all the other watchdog drivers, it does not appear that
> > there is any other processor which uses a similar feature. Since imx is the
> > only processor that appears to support this feature, it might make sense in
> > making this vendor specific. If in the future it is found more processors
> > support a similar functionality, it can be revisited and moved out from
> > being vendor specific?
> >
>
> I'm certainly no expert on device-tree policy. I understand your
> point, but realize that the driver in question is imx2_wdt.c
> (compatible = "fsl,imx21-wdt"). This is an IP block inside the silicon
> of only Freescale chips, so its not like a future omap chip would be
> using this driver - only fsl devices. So why would it need a 'vendor'
> property any more than its other properties?
>
> Regards,
>
> Tim

Wim,

Does the lack of response mean overwhelming approval?

I haven't heard any valid complaints - what does it take to get this approved?

Regards,

Tim
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" 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 v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Tim Harvey]

On Wed, Dec 2, 2015 at 12:54 PM, Tim Harvey <tharvey@gateworks.com> wrote:
>
> On Wed, Dec 2, 2015 at 11:11 AM, Akshay Bhat <akshay.bhat@timesys.com> wrote:
> >
> >
> > On 11/06/2015 05:02 PM, Guenter Roeck wrote:
> >>
> >> On Fri, Nov 06, 2015 at 11:53:42AM -0800, Tim Harvey wrote:
> >>>
> >>> On Thu, Nov 5, 2015 at 2:23 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> >>>>
> >>>> On Thu, Nov 05, 2015 at 04:19:21PM -0500, Akshay Bhat wrote:
> >>>>>
> >>>>> From: Tim Harvey <tharvey@gateworks.com>
> >>>>>
> >>>>> The IMX6 watchdog supports assertion of a signal (WDOG_B) which
> >>>>> can be pinmux'd to an external pin. This is typically used for boards
> >>>>> that
> >>>>> have PMIC's in control of the IMX6 power rails. In fact, failure to use
> >>>>> such an external reset on boards with external PMIC's can result in
> >>>>> various
> >>>>> hangs due to the IMX6 not being fully reset [1] as well as the board
> >>>>> failing
> >>>>> to reset because its PMIC has not been reset to provide adequate
> >>>>> voltage for
> >>>>> the CPU when coming out of reset at 800Mhz.
> >>>>>
> >>>>> This uses a new device-tree property 'ext-reset-output' to indicate the
> >>>>> board has such a reset and to cause the watchdog to be configured to
> >>>>> assert
> >>>>> WDOG_B instead of an internal reset both on a watchdog timeout and in
> >>>>> system_restart.
> >>>>>
> >>>>> [1]
> >>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html
> >>>>>
> >>>>> Cc: Lucas Stach <l.stach@pengutronix.de>
> >>>>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> >>>>> ---
> >>>>>   .../devicetree/bindings/watchdog/fsl-imx-wdt.txt     |  2 ++
> >>>>>   drivers/watchdog/imx2_wdt.c                          | 20
> >>>>> ++++++++++++++++++--
> >>>>>   2 files changed, 20 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> >>>>> b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> >>>>> index 8dab6fd..9b89b3a 100644
> >>>>> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> >>>>> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> >>>>> @@ -9,6 +9,8 @@ Optional property:
> >>>>
> >>>>
> >>>> properties ?
> >>>>
> >>>>>   - big-endian: If present the watchdog device's registers are
> >>>>> implemented
> >>>>>     in big endian mode, otherwise in native mode(same with CPU), for
> >>>>> more
> >>>>>     detail please see:
> >>>>> Documentation/devicetree/bindings/regmap/regmap.txt.
> >>>>> +- ext-reset-output: If present the watchdog device is configured to
> >>>>> assert its
> >>>>
> >>>>
> >>>> Should that have a vendor prefix ? Also, not sure if "-output"
> >>>> has any real value in the property name. "fsl,external-reset", maybe ?
> >>>
> >>>
> >>> Hi Guenter,
> >>>
> >>> I don't see why a vendor prefix is necessary - its a feature of the
> >>> IMX6 watchdog supported by this driver to be able to trigger an
> >>> internal chip-level reset and/or an external signal that can be hooked
> >>> to additional hardware.
> >>>
> >> Sounded like vendor specific to me, but then I am not a devicetree
> >> maintainer,
> >> so I am not an authority on the subject.
> >
> >
> > Devicetree maintainers,
> >
> > Any thoughts?
> >
> > Tim,
> >
> > After looking at all the other watchdog drivers, it does not appear that
> > there is any other processor which uses a similar feature. Since imx is the
> > only processor that appears to support this feature, it might make sense in
> > making this vendor specific. If in the future it is found more processors
> > support a similar functionality, it can be revisited and moved out from
> > being vendor specific?
> >
>
> I'm certainly no expert on device-tree policy. I understand your
> point, but realize that the driver in question is imx2_wdt.c
> (compatible = "fsl,imx21-wdt"). This is an IP block inside the silicon
> of only Freescale chips, so its not like a future omap chip would be
> using this driver - only fsl devices. So why would it need a 'vendor'
> property any more than its other properties?
>
> Regards,
>
> Tim

Wim,

Does the lack of response mean overwhelming approval?

I haven't heard any valid complaints - what does it take to get this approved?

Regards,

Tim

[PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Tim Harvey]

On Wed, Dec 2, 2015 at 12:54 PM, Tim Harvey <tharvey@gateworks.com> wrote:
>
> On Wed, Dec 2, 2015 at 11:11 AM, Akshay Bhat <akshay.bhat@timesys.com> wrote:
> >
> >
> > On 11/06/2015 05:02 PM, Guenter Roeck wrote:
> >>
> >> On Fri, Nov 06, 2015 at 11:53:42AM -0800, Tim Harvey wrote:
> >>>
> >>> On Thu, Nov 5, 2015 at 2:23 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> >>>>
> >>>> On Thu, Nov 05, 2015 at 04:19:21PM -0500, Akshay Bhat wrote:
> >>>>>
> >>>>> From: Tim Harvey <tharvey@gateworks.com>
> >>>>>
> >>>>> The IMX6 watchdog supports assertion of a signal (WDOG_B) which
> >>>>> can be pinmux'd to an external pin. This is typically used for boards
> >>>>> that
> >>>>> have PMIC's in control of the IMX6 power rails. In fact, failure to use
> >>>>> such an external reset on boards with external PMIC's can result in
> >>>>> various
> >>>>> hangs due to the IMX6 not being fully reset [1] as well as the board
> >>>>> failing
> >>>>> to reset because its PMIC has not been reset to provide adequate
> >>>>> voltage for
> >>>>> the CPU when coming out of reset at 800Mhz.
> >>>>>
> >>>>> This uses a new device-tree property 'ext-reset-output' to indicate the
> >>>>> board has such a reset and to cause the watchdog to be configured to
> >>>>> assert
> >>>>> WDOG_B instead of an internal reset both on a watchdog timeout and in
> >>>>> system_restart.
> >>>>>
> >>>>> [1]
> >>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html
> >>>>>
> >>>>> Cc: Lucas Stach <l.stach@pengutronix.de>
> >>>>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> >>>>> ---
> >>>>>   .../devicetree/bindings/watchdog/fsl-imx-wdt.txt     |  2 ++
> >>>>>   drivers/watchdog/imx2_wdt.c                          | 20
> >>>>> ++++++++++++++++++--
> >>>>>   2 files changed, 20 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> >>>>> b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> >>>>> index 8dab6fd..9b89b3a 100644
> >>>>> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> >>>>> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> >>>>> @@ -9,6 +9,8 @@ Optional property:
> >>>>
> >>>>
> >>>> properties ?
> >>>>
> >>>>>   - big-endian: If present the watchdog device's registers are
> >>>>> implemented
> >>>>>     in big endian mode, otherwise in native mode(same with CPU), for
> >>>>> more
> >>>>>     detail please see:
> >>>>> Documentation/devicetree/bindings/regmap/regmap.txt.
> >>>>> +- ext-reset-output: If present the watchdog device is configured to
> >>>>> assert its
> >>>>
> >>>>
> >>>> Should that have a vendor prefix ? Also, not sure if "-output"
> >>>> has any real value in the property name. "fsl,external-reset", maybe ?
> >>>
> >>>
> >>> Hi Guenter,
> >>>
> >>> I don't see why a vendor prefix is necessary - its a feature of the
> >>> IMX6 watchdog supported by this driver to be able to trigger an
> >>> internal chip-level reset and/or an external signal that can be hooked
> >>> to additional hardware.
> >>>
> >> Sounded like vendor specific to me, but then I am not a devicetree
> >> maintainer,
> >> so I am not an authority on the subject.
> >
> >
> > Devicetree maintainers,
> >
> > Any thoughts?
> >
> > Tim,
> >
> > After looking at all the other watchdog drivers, it does not appear that
> > there is any other processor which uses a similar feature. Since imx is the
> > only processor that appears to support this feature, it might make sense in
> > making this vendor specific. If in the future it is found more processors
> > support a similar functionality, it can be revisited and moved out from
> > being vendor specific?
> >
>
> I'm certainly no expert on device-tree policy. I understand your
> point, but realize that the driver in question is imx2_wdt.c
> (compatible = "fsl,imx21-wdt"). This is an IP block inside the silicon
> of only Freescale chips, so its not like a future omap chip would be
> using this driver - only fsl devices. So why would it need a 'vendor'
> property any more than its other properties?
>
> Regards,
>
> Tim

Wim,

Does the lack of response mean overwhelming approval?

I haven't heard any valid complaints - what does it take to get this approved?

Regards,

Tim

Re: [PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Guenter Roeck]

On 12/17/2015 07:02 AM, Tim Harvey wrote:
> On Wed, Dec 2, 2015 at 12:54 PM, Tim Harvey <tharvey@gateworks.com> wrote:
>>
>> On Wed, Dec 2, 2015 at 11:11 AM, Akshay Bhat <akshay.bhat@timesys.com> wrote:
>>>
>>>
>>> On 11/06/2015 05:02 PM, Guenter Roeck wrote:
>>>>
>>>> On Fri, Nov 06, 2015 at 11:53:42AM -0800, Tim Harvey wrote:
>>>>>
>>>>> On Thu, Nov 5, 2015 at 2:23 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>>>
>>>>>> On Thu, Nov 05, 2015 at 04:19:21PM -0500, Akshay Bhat wrote:
>>>>>>>
>>>>>>> From: Tim Harvey <tharvey@gateworks.com>
>>>>>>>
>>>>>>> The IMX6 watchdog supports assertion of a signal (WDOG_B) which
>>>>>>> can be pinmux'd to an external pin. This is typically used for boards
>>>>>>> that
>>>>>>> have PMIC's in control of the IMX6 power rails. In fact, failure to use
>>>>>>> such an external reset on boards with external PMIC's can result in
>>>>>>> various
>>>>>>> hangs due to the IMX6 not being fully reset [1] as well as the board
>>>>>>> failing
>>>>>>> to reset because its PMIC has not been reset to provide adequate
>>>>>>> voltage for
>>>>>>> the CPU when coming out of reset at 800Mhz.
>>>>>>>
>>>>>>> This uses a new device-tree property 'ext-reset-output' to indicate the
>>>>>>> board has such a reset and to cause the watchdog to be configured to
>>>>>>> assert
>>>>>>> WDOG_B instead of an internal reset both on a watchdog timeout and in
>>>>>>> system_restart.
>>>>>>>
>>>>>>> [1]
>>>>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html
>>>>>>>
>>>>>>> Cc: Lucas Stach <l.stach@pengutronix.de>
>>>>>>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>>>>>>> ---
>>>>>>>    .../devicetree/bindings/watchdog/fsl-imx-wdt.txt     |  2 ++
>>>>>>>    drivers/watchdog/imx2_wdt.c                          | 20
>>>>>>> ++++++++++++++++++--
>>>>>>>    2 files changed, 20 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>>>> b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>>>> index 8dab6fd..9b89b3a 100644
>>>>>>> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>>>> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>>>> @@ -9,6 +9,8 @@ Optional property:
>>>>>>
>>>>>>
>>>>>> properties ?
>>>>>>
>>>>>>>    - big-endian: If present the watchdog device's registers are
>>>>>>> implemented
>>>>>>>      in big endian mode, otherwise in native mode(same with CPU), for
>>>>>>> more
>>>>>>>      detail please see:
>>>>>>> Documentation/devicetree/bindings/regmap/regmap.txt.
>>>>>>> +- ext-reset-output: If present the watchdog device is configured to
>>>>>>> assert its
>>>>>>
>>>>>>
>>>>>> Should that have a vendor prefix ? Also, not sure if "-output"
>>>>>> has any real value in the property name. "fsl,external-reset", maybe ?
>>>>>
>>>>>
>>>>> Hi Guenter,
>>>>>
>>>>> I don't see why a vendor prefix is necessary - its a feature of the
>>>>> IMX6 watchdog supported by this driver to be able to trigger an
>>>>> internal chip-level reset and/or an external signal that can be hooked
>>>>> to additional hardware.
>>>>>
>>>> Sounded like vendor specific to me, but then I am not a devicetree
>>>> maintainer,
>>>> so I am not an authority on the subject.
>>>
>>>
>>> Devicetree maintainers,
>>>
>>> Any thoughts?
>>>
>>> Tim,
>>>
>>> After looking at all the other watchdog drivers, it does not appear that
>>> there is any other processor which uses a similar feature. Since imx is the
>>> only processor that appears to support this feature, it might make sense in
>>> making this vendor specific. If in the future it is found more processors
>>> support a similar functionality, it can be revisited and moved out from
>>> being vendor specific?
>>>
>>
>> I'm certainly no expert on device-tree policy. I understand your
>> point, but realize that the driver in question is imx2_wdt.c
>> (compatible = "fsl,imx21-wdt"). This is an IP block inside the silicon
>> of only Freescale chips, so its not like a future omap chip would be
>> using this driver - only fsl devices. So why would it need a 'vendor'
>> property any more than its other properties?
>>
>> Regards,
>>
>> Tim
>
> Wim,
>
> Does the lack of response mean overwhelming approval?
>
> I haven't heard any valid complaints - what does it take to get this approved?
>

Tim,

Do you have an Ack from a devicetree maintainer ? I don't recall seeing one.

Thanks,
Guenter

Re: [PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Guenter Roeck]

On 12/17/2015 07:02 AM, Tim Harvey wrote:
> On Wed, Dec 2, 2015 at 12:54 PM, Tim Harvey <tharvey-UMMOYl/HMS+akBO8gow8eQ@public.gmane.org> wrote:
>>
>> On Wed, Dec 2, 2015 at 11:11 AM, Akshay Bhat <akshay.bhat-jEh4hwF5bVhBDgjK7y7TUQ@public.gmane.org> wrote:
>>>
>>>
>>> On 11/06/2015 05:02 PM, Guenter Roeck wrote:
>>>>
>>>> On Fri, Nov 06, 2015 at 11:53:42AM -0800, Tim Harvey wrote:
>>>>>
>>>>> On Thu, Nov 5, 2015 at 2:23 PM, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
>>>>>>
>>>>>> On Thu, Nov 05, 2015 at 04:19:21PM -0500, Akshay Bhat wrote:
>>>>>>>
>>>>>>> From: Tim Harvey <tharvey-UMMOYl/HMS+akBO8gow8eQ@public.gmane.org>
>>>>>>>
>>>>>>> The IMX6 watchdog supports assertion of a signal (WDOG_B) which
>>>>>>> can be pinmux'd to an external pin. This is typically used for boards
>>>>>>> that
>>>>>>> have PMIC's in control of the IMX6 power rails. In fact, failure to use
>>>>>>> such an external reset on boards with external PMIC's can result in
>>>>>>> various
>>>>>>> hangs due to the IMX6 not being fully reset [1] as well as the board
>>>>>>> failing
>>>>>>> to reset because its PMIC has not been reset to provide adequate
>>>>>>> voltage for
>>>>>>> the CPU when coming out of reset at 800Mhz.
>>>>>>>
>>>>>>> This uses a new device-tree property 'ext-reset-output' to indicate the
>>>>>>> board has such a reset and to cause the watchdog to be configured to
>>>>>>> assert
>>>>>>> WDOG_B instead of an internal reset both on a watchdog timeout and in
>>>>>>> system_restart.
>>>>>>>
>>>>>>> [1]
>>>>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html
>>>>>>>
>>>>>>> Cc: Lucas Stach <l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>>>>>>> Signed-off-by: Tim Harvey <tharvey-UMMOYl/HMS+akBO8gow8eQ@public.gmane.org>
>>>>>>> ---
>>>>>>>    .../devicetree/bindings/watchdog/fsl-imx-wdt.txt     |  2 ++
>>>>>>>    drivers/watchdog/imx2_wdt.c                          | 20
>>>>>>> ++++++++++++++++++--
>>>>>>>    2 files changed, 20 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>>>> b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>>>> index 8dab6fd..9b89b3a 100644
>>>>>>> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>>>> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>>>> @@ -9,6 +9,8 @@ Optional property:
>>>>>>
>>>>>>
>>>>>> properties ?
>>>>>>
>>>>>>>    - big-endian: If present the watchdog device's registers are
>>>>>>> implemented
>>>>>>>      in big endian mode, otherwise in native mode(same with CPU), for
>>>>>>> more
>>>>>>>      detail please see:
>>>>>>> Documentation/devicetree/bindings/regmap/regmap.txt.
>>>>>>> +- ext-reset-output: If present the watchdog device is configured to
>>>>>>> assert its
>>>>>>
>>>>>>
>>>>>> Should that have a vendor prefix ? Also, not sure if "-output"
>>>>>> has any real value in the property name. "fsl,external-reset", maybe ?
>>>>>
>>>>>
>>>>> Hi Guenter,
>>>>>
>>>>> I don't see why a vendor prefix is necessary - its a feature of the
>>>>> IMX6 watchdog supported by this driver to be able to trigger an
>>>>> internal chip-level reset and/or an external signal that can be hooked
>>>>> to additional hardware.
>>>>>
>>>> Sounded like vendor specific to me, but then I am not a devicetree
>>>> maintainer,
>>>> so I am not an authority on the subject.
>>>
>>>
>>> Devicetree maintainers,
>>>
>>> Any thoughts?
>>>
>>> Tim,
>>>
>>> After looking at all the other watchdog drivers, it does not appear that
>>> there is any other processor which uses a similar feature. Since imx is the
>>> only processor that appears to support this feature, it might make sense in
>>> making this vendor specific. If in the future it is found more processors
>>> support a similar functionality, it can be revisited and moved out from
>>> being vendor specific?
>>>
>>
>> I'm certainly no expert on device-tree policy. I understand your
>> point, but realize that the driver in question is imx2_wdt.c
>> (compatible = "fsl,imx21-wdt"). This is an IP block inside the silicon
>> of only Freescale chips, so its not like a future omap chip would be
>> using this driver - only fsl devices. So why would it need a 'vendor'
>> property any more than its other properties?
>>
>> Regards,
>>
>> Tim
>
> Wim,
>
> Does the lack of response mean overwhelming approval?
>
> I haven't heard any valid complaints - what does it take to get this approved?
>

Tim,

Do you have an Ack from a devicetree maintainer ? I don't recall seeing one.

Thanks,
Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" 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 v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Guenter Roeck]

On 12/17/2015 07:02 AM, Tim Harvey wrote:
> On Wed, Dec 2, 2015 at 12:54 PM, Tim Harvey <tharvey@gateworks.com> wrote:
>>
>> On Wed, Dec 2, 2015 at 11:11 AM, Akshay Bhat <akshay.bhat@timesys.com> wrote:
>>>
>>>
>>> On 11/06/2015 05:02 PM, Guenter Roeck wrote:
>>>>
>>>> On Fri, Nov 06, 2015 at 11:53:42AM -0800, Tim Harvey wrote:
>>>>>
>>>>> On Thu, Nov 5, 2015 at 2:23 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>>>
>>>>>> On Thu, Nov 05, 2015 at 04:19:21PM -0500, Akshay Bhat wrote:
>>>>>>>
>>>>>>> From: Tim Harvey <tharvey@gateworks.com>
>>>>>>>
>>>>>>> The IMX6 watchdog supports assertion of a signal (WDOG_B) which
>>>>>>> can be pinmux'd to an external pin. This is typically used for boards
>>>>>>> that
>>>>>>> have PMIC's in control of the IMX6 power rails. In fact, failure to use
>>>>>>> such an external reset on boards with external PMIC's can result in
>>>>>>> various
>>>>>>> hangs due to the IMX6 not being fully reset [1] as well as the board
>>>>>>> failing
>>>>>>> to reset because its PMIC has not been reset to provide adequate
>>>>>>> voltage for
>>>>>>> the CPU when coming out of reset at 800Mhz.
>>>>>>>
>>>>>>> This uses a new device-tree property 'ext-reset-output' to indicate the
>>>>>>> board has such a reset and to cause the watchdog to be configured to
>>>>>>> assert
>>>>>>> WDOG_B instead of an internal reset both on a watchdog timeout and in
>>>>>>> system_restart.
>>>>>>>
>>>>>>> [1]
>>>>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html
>>>>>>>
>>>>>>> Cc: Lucas Stach <l.stach@pengutronix.de>
>>>>>>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>>>>>>> ---
>>>>>>>    .../devicetree/bindings/watchdog/fsl-imx-wdt.txt     |  2 ++
>>>>>>>    drivers/watchdog/imx2_wdt.c                          | 20
>>>>>>> ++++++++++++++++++--
>>>>>>>    2 files changed, 20 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>>>> b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>>>> index 8dab6fd..9b89b3a 100644
>>>>>>> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>>>> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>>>> @@ -9,6 +9,8 @@ Optional property:
>>>>>>
>>>>>>
>>>>>> properties ?
>>>>>>
>>>>>>>    - big-endian: If present the watchdog device's registers are
>>>>>>> implemented
>>>>>>>      in big endian mode, otherwise in native mode(same with CPU), for
>>>>>>> more
>>>>>>>      detail please see:
>>>>>>> Documentation/devicetree/bindings/regmap/regmap.txt.
>>>>>>> +- ext-reset-output: If present the watchdog device is configured to
>>>>>>> assert its
>>>>>>
>>>>>>
>>>>>> Should that have a vendor prefix ? Also, not sure if "-output"
>>>>>> has any real value in the property name. "fsl,external-reset", maybe ?
>>>>>
>>>>>
>>>>> Hi Guenter,
>>>>>
>>>>> I don't see why a vendor prefix is necessary - its a feature of the
>>>>> IMX6 watchdog supported by this driver to be able to trigger an
>>>>> internal chip-level reset and/or an external signal that can be hooked
>>>>> to additional hardware.
>>>>>
>>>> Sounded like vendor specific to me, but then I am not a devicetree
>>>> maintainer,
>>>> so I am not an authority on the subject.
>>>
>>>
>>> Devicetree maintainers,
>>>
>>> Any thoughts?
>>>
>>> Tim,
>>>
>>> After looking at all the other watchdog drivers, it does not appear that
>>> there is any other processor which uses a similar feature. Since imx is the
>>> only processor that appears to support this feature, it might make sense in
>>> making this vendor specific. If in the future it is found more processors
>>> support a similar functionality, it can be revisited and moved out from
>>> being vendor specific?
>>>
>>
>> I'm certainly no expert on device-tree policy. I understand your
>> point, but realize that the driver in question is imx2_wdt.c
>> (compatible = "fsl,imx21-wdt"). This is an IP block inside the silicon
>> of only Freescale chips, so its not like a future omap chip would be
>> using this driver - only fsl devices. So why would it need a 'vendor'
>> property any more than its other properties?
>>
>> Regards,
>>
>> Tim
>
> Wim,
>
> Does the lack of response mean overwhelming approval?
>
> I haven't heard any valid complaints - what does it take to get this approved?
>

Tim,

Do you have an Ack from a devicetree maintainer ? I don't recall seeing one.

Thanks,
Guenter

[PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Guenter Roeck]

On 12/17/2015 07:02 AM, Tim Harvey wrote:
> On Wed, Dec 2, 2015 at 12:54 PM, Tim Harvey <tharvey@gateworks.com> wrote:
>>
>> On Wed, Dec 2, 2015 at 11:11 AM, Akshay Bhat <akshay.bhat@timesys.com> wrote:
>>>
>>>
>>> On 11/06/2015 05:02 PM, Guenter Roeck wrote:
>>>>
>>>> On Fri, Nov 06, 2015 at 11:53:42AM -0800, Tim Harvey wrote:
>>>>>
>>>>> On Thu, Nov 5, 2015 at 2:23 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>>>
>>>>>> On Thu, Nov 05, 2015 at 04:19:21PM -0500, Akshay Bhat wrote:
>>>>>>>
>>>>>>> From: Tim Harvey <tharvey@gateworks.com>
>>>>>>>
>>>>>>> The IMX6 watchdog supports assertion of a signal (WDOG_B) which
>>>>>>> can be pinmux'd to an external pin. This is typically used for boards
>>>>>>> that
>>>>>>> have PMIC's in control of the IMX6 power rails. In fact, failure to use
>>>>>>> such an external reset on boards with external PMIC's can result in
>>>>>>> various
>>>>>>> hangs due to the IMX6 not being fully reset [1] as well as the board
>>>>>>> failing
>>>>>>> to reset because its PMIC has not been reset to provide adequate
>>>>>>> voltage for
>>>>>>> the CPU when coming out of reset at 800Mhz.
>>>>>>>
>>>>>>> This uses a new device-tree property 'ext-reset-output' to indicate the
>>>>>>> board has such a reset and to cause the watchdog to be configured to
>>>>>>> assert
>>>>>>> WDOG_B instead of an internal reset both on a watchdog timeout and in
>>>>>>> system_restart.
>>>>>>>
>>>>>>> [1]
>>>>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html
>>>>>>>
>>>>>>> Cc: Lucas Stach <l.stach@pengutronix.de>
>>>>>>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>>>>>>> ---
>>>>>>>    .../devicetree/bindings/watchdog/fsl-imx-wdt.txt     |  2 ++
>>>>>>>    drivers/watchdog/imx2_wdt.c                          | 20
>>>>>>> ++++++++++++++++++--
>>>>>>>    2 files changed, 20 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>>>> b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>>>> index 8dab6fd..9b89b3a 100644
>>>>>>> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>>>> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>>>> @@ -9,6 +9,8 @@ Optional property:
>>>>>>
>>>>>>
>>>>>> properties ?
>>>>>>
>>>>>>>    - big-endian: If present the watchdog device's registers are
>>>>>>> implemented
>>>>>>>      in big endian mode, otherwise in native mode(same with CPU), for
>>>>>>> more
>>>>>>>      detail please see:
>>>>>>> Documentation/devicetree/bindings/regmap/regmap.txt.
>>>>>>> +- ext-reset-output: If present the watchdog device is configured to
>>>>>>> assert its
>>>>>>
>>>>>>
>>>>>> Should that have a vendor prefix ? Also, not sure if "-output"
>>>>>> has any real value in the property name. "fsl,external-reset", maybe ?
>>>>>
>>>>>
>>>>> Hi Guenter,
>>>>>
>>>>> I don't see why a vendor prefix is necessary - its a feature of the
>>>>> IMX6 watchdog supported by this driver to be able to trigger an
>>>>> internal chip-level reset and/or an external signal that can be hooked
>>>>> to additional hardware.
>>>>>
>>>> Sounded like vendor specific to me, but then I am not a devicetree
>>>> maintainer,
>>>> so I am not an authority on the subject.
>>>
>>>
>>> Devicetree maintainers,
>>>
>>> Any thoughts?
>>>
>>> Tim,
>>>
>>> After looking at all the other watchdog drivers, it does not appear that
>>> there is any other processor which uses a similar feature. Since imx is the
>>> only processor that appears to support this feature, it might make sense in
>>> making this vendor specific. If in the future it is found more processors
>>> support a similar functionality, it can be revisited and moved out from
>>> being vendor specific?
>>>
>>
>> I'm certainly no expert on device-tree policy. I understand your
>> point, but realize that the driver in question is imx2_wdt.c
>> (compatible = "fsl,imx21-wdt"). This is an IP block inside the silicon
>> of only Freescale chips, so its not like a future omap chip would be
>> using this driver - only fsl devices. So why would it need a 'vendor'
>> property any more than its other properties?
>>
>> Regards,
>>
>> Tim
>
> Wim,
>
> Does the lack of response mean overwhelming approval?
>
> I haven't heard any valid complaints - what does it take to get this approved?
>

Tim,

Do you have an Ack from a devicetree maintainer ? I don't recall seeing one.

Thanks,
Guenter

Re: [PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Wim Van Sebroeck]

Hi Tim,

> On Wed, Dec 2, 2015 at 12:54 PM, Tim Harvey <tharvey@gateworks.com> wrote:
> >
> > On Wed, Dec 2, 2015 at 11:11 AM, Akshay Bhat <akshay.bhat@timesys.com> wrote:
> > >
> > >
> > > On 11/06/2015 05:02 PM, Guenter Roeck wrote:
> > >>
> > >> On Fri, Nov 06, 2015 at 11:53:42AM -0800, Tim Harvey wrote:
> > >>>
> > >>> On Thu, Nov 5, 2015 at 2:23 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> > >>>>
> > >>>> On Thu, Nov 05, 2015 at 04:19:21PM -0500, Akshay Bhat wrote:
> > >>>>>
> > >>>>> From: Tim Harvey <tharvey@gateworks.com>
> > >>>>>
> > >>>>> The IMX6 watchdog supports assertion of a signal (WDOG_B) which
> > >>>>> can be pinmux'd to an external pin. This is typically used for boards
> > >>>>> that
> > >>>>> have PMIC's in control of the IMX6 power rails. In fact, failure to use
> > >>>>> such an external reset on boards with external PMIC's can result in
> > >>>>> various
> > >>>>> hangs due to the IMX6 not being fully reset [1] as well as the board
> > >>>>> failing
> > >>>>> to reset because its PMIC has not been reset to provide adequate
> > >>>>> voltage for
> > >>>>> the CPU when coming out of reset at 800Mhz.
> > >>>>>
> > >>>>> This uses a new device-tree property 'ext-reset-output' to indicate the
> > >>>>> board has such a reset and to cause the watchdog to be configured to
> > >>>>> assert
> > >>>>> WDOG_B instead of an internal reset both on a watchdog timeout and in
> > >>>>> system_restart.
> > >>>>>
> > >>>>> [1]
> > >>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html
> > >>>>>
> > >>>>> Cc: Lucas Stach <l.stach@pengutronix.de>
> > >>>>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> > >>>>> ---
> > >>>>>   .../devicetree/bindings/watchdog/fsl-imx-wdt.txt     |  2 ++
> > >>>>>   drivers/watchdog/imx2_wdt.c                          | 20
> > >>>>> ++++++++++++++++++--
> > >>>>>   2 files changed, 20 insertions(+), 2 deletions(-)
> > >>>>>
> > >>>>> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> > >>>>> b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> > >>>>> index 8dab6fd..9b89b3a 100644
> > >>>>> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> > >>>>> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> > >>>>> @@ -9,6 +9,8 @@ Optional property:
> > >>>>
> > >>>>
> > >>>> properties ?
> > >>>>
> > >>>>>   - big-endian: If present the watchdog device's registers are
> > >>>>> implemented
> > >>>>>     in big endian mode, otherwise in native mode(same with CPU), for
> > >>>>> more
> > >>>>>     detail please see:
> > >>>>> Documentation/devicetree/bindings/regmap/regmap.txt.
> > >>>>> +- ext-reset-output: If present the watchdog device is configured to
> > >>>>> assert its
> > >>>>
> > >>>>
> > >>>> Should that have a vendor prefix ? Also, not sure if "-output"
> > >>>> has any real value in the property name. "fsl,external-reset", maybe ?
> > >>>
> > >>>
> > >>> Hi Guenter,
> > >>>
> > >>> I don't see why a vendor prefix is necessary - its a feature of the
> > >>> IMX6 watchdog supported by this driver to be able to trigger an
> > >>> internal chip-level reset and/or an external signal that can be hooked
> > >>> to additional hardware.
> > >>>
> > >> Sounded like vendor specific to me, but then I am not a devicetree
> > >> maintainer,
> > >> so I am not an authority on the subject.
> > >
> > >
> > > Devicetree maintainers,
> > >
> > > Any thoughts?
> > >
> > > Tim,
> > >
> > > After looking at all the other watchdog drivers, it does not appear that
> > > there is any other processor which uses a similar feature. Since imx is the
> > > only processor that appears to support this feature, it might make sense in
> > > making this vendor specific. If in the future it is found more processors
> > > support a similar functionality, it can be revisited and moved out from
> > > being vendor specific?
> > >
> >
> > I'm certainly no expert on device-tree policy. I understand your
> > point, but realize that the driver in question is imx2_wdt.c
> > (compatible = "fsl,imx21-wdt"). This is an IP block inside the silicon
> > of only Freescale chips, so its not like a future omap chip would be
> > using this driver - only fsl devices. So why would it need a 'vendor'
> > property any more than its other properties?
> >
> > Regards,
> >
> > Tim
> 
> Wim,
> 
> Does the lack of response mean overwhelming approval?
> 
> I haven't heard any valid complaints - what does it take to get this approved?
> 
> Regards,
> 
> Tim

I have no objections against the idea and the code itself.
But as Guenter pointed out: it would be handy to get feedback from the devicetree maintainers on the above discussion.

Kind regards,
Wim.

Re: [PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Wim Van Sebroeck]

Hi Tim,

> On Wed, Dec 2, 2015 at 12:54 PM, Tim Harvey <tharvey-UMMOYl/HMS+akBO8gow8eQ@public.gmane.org> wrote:
> >
> > On Wed, Dec 2, 2015 at 11:11 AM, Akshay Bhat <akshay.bhat-jEh4hwF5bVhBDgjK7y7TUQ@public.gmane.org> wrote:
> > >
> > >
> > > On 11/06/2015 05:02 PM, Guenter Roeck wrote:
> > >>
> > >> On Fri, Nov 06, 2015 at 11:53:42AM -0800, Tim Harvey wrote:
> > >>>
> > >>> On Thu, Nov 5, 2015 at 2:23 PM, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
> > >>>>
> > >>>> On Thu, Nov 05, 2015 at 04:19:21PM -0500, Akshay Bhat wrote:
> > >>>>>
> > >>>>> From: Tim Harvey <tharvey-UMMOYl/HMS+akBO8gow8eQ@public.gmane.org>
> > >>>>>
> > >>>>> The IMX6 watchdog supports assertion of a signal (WDOG_B) which
> > >>>>> can be pinmux'd to an external pin. This is typically used for boards
> > >>>>> that
> > >>>>> have PMIC's in control of the IMX6 power rails. In fact, failure to use
> > >>>>> such an external reset on boards with external PMIC's can result in
> > >>>>> various
> > >>>>> hangs due to the IMX6 not being fully reset [1] as well as the board
> > >>>>> failing
> > >>>>> to reset because its PMIC has not been reset to provide adequate
> > >>>>> voltage for
> > >>>>> the CPU when coming out of reset at 800Mhz.
> > >>>>>
> > >>>>> This uses a new device-tree property 'ext-reset-output' to indicate the
> > >>>>> board has such a reset and to cause the watchdog to be configured to
> > >>>>> assert
> > >>>>> WDOG_B instead of an internal reset both on a watchdog timeout and in
> > >>>>> system_restart.
> > >>>>>
> > >>>>> [1]
> > >>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html
> > >>>>>
> > >>>>> Cc: Lucas Stach <l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > >>>>> Signed-off-by: Tim Harvey <tharvey-UMMOYl/HMS+akBO8gow8eQ@public.gmane.org>
> > >>>>> ---
> > >>>>>   .../devicetree/bindings/watchdog/fsl-imx-wdt.txt     |  2 ++
> > >>>>>   drivers/watchdog/imx2_wdt.c                          | 20
> > >>>>> ++++++++++++++++++--
> > >>>>>   2 files changed, 20 insertions(+), 2 deletions(-)
> > >>>>>
> > >>>>> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> > >>>>> b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> > >>>>> index 8dab6fd..9b89b3a 100644
> > >>>>> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> > >>>>> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> > >>>>> @@ -9,6 +9,8 @@ Optional property:
> > >>>>
> > >>>>
> > >>>> properties ?
> > >>>>
> > >>>>>   - big-endian: If present the watchdog device's registers are
> > >>>>> implemented
> > >>>>>     in big endian mode, otherwise in native mode(same with CPU), for
> > >>>>> more
> > >>>>>     detail please see:
> > >>>>> Documentation/devicetree/bindings/regmap/regmap.txt.
> > >>>>> +- ext-reset-output: If present the watchdog device is configured to
> > >>>>> assert its
> > >>>>
> > >>>>
> > >>>> Should that have a vendor prefix ? Also, not sure if "-output"
> > >>>> has any real value in the property name. "fsl,external-reset", maybe ?
> > >>>
> > >>>
> > >>> Hi Guenter,
> > >>>
> > >>> I don't see why a vendor prefix is necessary - its a feature of the
> > >>> IMX6 watchdog supported by this driver to be able to trigger an
> > >>> internal chip-level reset and/or an external signal that can be hooked
> > >>> to additional hardware.
> > >>>
> > >> Sounded like vendor specific to me, but then I am not a devicetree
> > >> maintainer,
> > >> so I am not an authority on the subject.
> > >
> > >
> > > Devicetree maintainers,
> > >
> > > Any thoughts?
> > >
> > > Tim,
> > >
> > > After looking at all the other watchdog drivers, it does not appear that
> > > there is any other processor which uses a similar feature. Since imx is the
> > > only processor that appears to support this feature, it might make sense in
> > > making this vendor specific. If in the future it is found more processors
> > > support a similar functionality, it can be revisited and moved out from
> > > being vendor specific?
> > >
> >
> > I'm certainly no expert on device-tree policy. I understand your
> > point, but realize that the driver in question is imx2_wdt.c
> > (compatible = "fsl,imx21-wdt"). This is an IP block inside the silicon
> > of only Freescale chips, so its not like a future omap chip would be
> > using this driver - only fsl devices. So why would it need a 'vendor'
> > property any more than its other properties?
> >
> > Regards,
> >
> > Tim
> 
> Wim,
> 
> Does the lack of response mean overwhelming approval?
> 
> I haven't heard any valid complaints - what does it take to get this approved?
> 
> Regards,
> 
> Tim

I have no objections against the idea and the code itself.
But as Guenter pointed out: it would be handy to get feedback from the devicetree maintainers on the above discussion.

Kind regards,
Wim.

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" 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 v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Wim Van Sebroeck]

Hi Tim,

> On Wed, Dec 2, 2015 at 12:54 PM, Tim Harvey <tharvey@gateworks.com> wrote:
> >
> > On Wed, Dec 2, 2015 at 11:11 AM, Akshay Bhat <akshay.bhat@timesys.com> wrote:
> > >
> > >
> > > On 11/06/2015 05:02 PM, Guenter Roeck wrote:
> > >>
> > >> On Fri, Nov 06, 2015 at 11:53:42AM -0800, Tim Harvey wrote:
> > >>>
> > >>> On Thu, Nov 5, 2015 at 2:23 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> > >>>>
> > >>>> On Thu, Nov 05, 2015 at 04:19:21PM -0500, Akshay Bhat wrote:
> > >>>>>
> > >>>>> From: Tim Harvey <tharvey@gateworks.com>
> > >>>>>
> > >>>>> The IMX6 watchdog supports assertion of a signal (WDOG_B) which
> > >>>>> can be pinmux'd to an external pin. This is typically used for boards
> > >>>>> that
> > >>>>> have PMIC's in control of the IMX6 power rails. In fact, failure to use
> > >>>>> such an external reset on boards with external PMIC's can result in
> > >>>>> various
> > >>>>> hangs due to the IMX6 not being fully reset [1] as well as the board
> > >>>>> failing
> > >>>>> to reset because its PMIC has not been reset to provide adequate
> > >>>>> voltage for
> > >>>>> the CPU when coming out of reset at 800Mhz.
> > >>>>>
> > >>>>> This uses a new device-tree property 'ext-reset-output' to indicate the
> > >>>>> board has such a reset and to cause the watchdog to be configured to
> > >>>>> assert
> > >>>>> WDOG_B instead of an internal reset both on a watchdog timeout and in
> > >>>>> system_restart.
> > >>>>>
> > >>>>> [1]
> > >>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html
> > >>>>>
> > >>>>> Cc: Lucas Stach <l.stach@pengutronix.de>
> > >>>>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> > >>>>> ---
> > >>>>>   .../devicetree/bindings/watchdog/fsl-imx-wdt.txt     |  2 ++
> > >>>>>   drivers/watchdog/imx2_wdt.c                          | 20
> > >>>>> ++++++++++++++++++--
> > >>>>>   2 files changed, 20 insertions(+), 2 deletions(-)
> > >>>>>
> > >>>>> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> > >>>>> b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> > >>>>> index 8dab6fd..9b89b3a 100644
> > >>>>> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> > >>>>> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
> > >>>>> @@ -9,6 +9,8 @@ Optional property:
> > >>>>
> > >>>>
> > >>>> properties ?
> > >>>>
> > >>>>>   - big-endian: If present the watchdog device's registers are
> > >>>>> implemented
> > >>>>>     in big endian mode, otherwise in native mode(same with CPU), for
> > >>>>> more
> > >>>>>     detail please see:
> > >>>>> Documentation/devicetree/bindings/regmap/regmap.txt.
> > >>>>> +- ext-reset-output: If present the watchdog device is configured to
> > >>>>> assert its
> > >>>>
> > >>>>
> > >>>> Should that have a vendor prefix ? Also, not sure if "-output"
> > >>>> has any real value in the property name. "fsl,external-reset", maybe ?
> > >>>
> > >>>
> > >>> Hi Guenter,
> > >>>
> > >>> I don't see why a vendor prefix is necessary - its a feature of the
> > >>> IMX6 watchdog supported by this driver to be able to trigger an
> > >>> internal chip-level reset and/or an external signal that can be hooked
> > >>> to additional hardware.
> > >>>
> > >> Sounded like vendor specific to me, but then I am not a devicetree
> > >> maintainer,
> > >> so I am not an authority on the subject.
> > >
> > >
> > > Devicetree maintainers,
> > >
> > > Any thoughts?
> > >
> > > Tim,
> > >
> > > After looking at all the other watchdog drivers, it does not appear that
> > > there is any other processor which uses a similar feature. Since imx is the
> > > only processor that appears to support this feature, it might make sense in
> > > making this vendor specific. If in the future it is found more processors
> > > support a similar functionality, it can be revisited and moved out from
> > > being vendor specific?
> > >
> >
> > I'm certainly no expert on device-tree policy. I understand your
> > point, but realize that the driver in question is imx2_wdt.c
> > (compatible = "fsl,imx21-wdt"). This is an IP block inside the silicon
> > of only Freescale chips, so its not like a future omap chip would be
> > using this driver - only fsl devices. So why would it need a 'vendor'
> > property any more than its other properties?
> >
> > Regards,
> >
> > Tim
> 
> Wim,
> 
> Does the lack of response mean overwhelming approval?
> 
> I haven't heard any valid complaints - what does it take to get this approved?
> 
> Regards,
> 
> Tim

I have no objections against the idea and the code itself.
But as Guenter pointed out: it would be handy to get feedback from the devicetree maintainers on the above discussion.

Kind regards,
Wim.

Re: [PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Akshay Bhat]

Rob,

On 12/28/2015 11:29 AM, Wim Van Sebroeck wrote:
> Hi Tim,
>
>> On Wed, Dec 2, 2015 at 12:54 PM, Tim Harvey <tharvey@gateworks.com> wrote:
>>>
>>> On Wed, Dec 2, 2015 at 11:11 AM, Akshay Bhat <akshay.bhat@timesys.com> wrote:
>>>>
>>>>
>>>> On 11/06/2015 05:02 PM, Guenter Roeck wrote:
>>>>>
>>>>> On Fri, Nov 06, 2015 at 11:53:42AM -0800, Tim Harvey wrote:
>>>>>>
>>>>>> On Thu, Nov 5, 2015 at 2:23 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>>>>
>>>>>>> On Thu, Nov 05, 2015 at 04:19:21PM -0500, Akshay Bhat wrote:
>>>>>>>>
>>>>>>>> From: Tim Harvey <tharvey@gateworks.com>
>>>>>>>>
>>>>>>>> The IMX6 watchdog supports assertion of a signal (WDOG_B) which
>>>>>>>> can be pinmux'd to an external pin. This is typically used for boards
>>>>>>>> that
>>>>>>>> have PMIC's in control of the IMX6 power rails. In fact, failure to use
>>>>>>>> such an external reset on boards with external PMIC's can result in
>>>>>>>> various
>>>>>>>> hangs due to the IMX6 not being fully reset [1] as well as the board
>>>>>>>> failing
>>>>>>>> to reset because its PMIC has not been reset to provide adequate
>>>>>>>> voltage for
>>>>>>>> the CPU when coming out of reset at 800Mhz.
>>>>>>>>
>>>>>>>> This uses a new device-tree property 'ext-reset-output' to indicate the
>>>>>>>> board has such a reset and to cause the watchdog to be configured to
>>>>>>>> assert
>>>>>>>> WDOG_B instead of an internal reset both on a watchdog timeout and in
>>>>>>>> system_restart.
>>>>>>>>
>>>>>>>> [1]
>>>>>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html
>>>>>>>>
>>>>>>>> Cc: Lucas Stach <l.stach@pengutronix.de>
>>>>>>>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>>>>>>>> ---
>>>>>>>>    .../devicetree/bindings/watchdog/fsl-imx-wdt.txt     |  2 ++
>>>>>>>>    drivers/watchdog/imx2_wdt.c                          | 20
>>>>>>>> ++++++++++++++++++--
>>>>>>>>    2 files changed, 20 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>>>>> b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>>>>> index 8dab6fd..9b89b3a 100644
>>>>>>>> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>>>>> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>>>>> @@ -9,6 +9,8 @@ Optional property:
>>>>>>>
>>>>>>>
>>>>>>> properties ?
>>>>>>>
>>>>>>>>    - big-endian: If present the watchdog device's registers are
>>>>>>>> implemented
>>>>>>>>      in big endian mode, otherwise in native mode(same with CPU), for
>>>>>>>> more
>>>>>>>>      detail please see:
>>>>>>>> Documentation/devicetree/bindings/regmap/regmap.txt.
>>>>>>>> +- ext-reset-output: If present the watchdog device is configured to
>>>>>>>> assert its
>>>>>>>
>>>>>>>
>>>>>>> Should that have a vendor prefix ? Also, not sure if "-output"
>>>>>>> has any real value in the property name. "fsl,external-reset", maybe ?
>>>>>>
>>>>>>
>>>>>> Hi Guenter,
>>>>>>
>>>>>> I don't see why a vendor prefix is necessary - its a feature of the
>>>>>> IMX6 watchdog supported by this driver to be able to trigger an
>>>>>> internal chip-level reset and/or an external signal that can be hooked
>>>>>> to additional hardware.
>>>>>>
>>>>> Sounded like vendor specific to me, but then I am not a devicetree
>>>>> maintainer,
>>>>> so I am not an authority on the subject.
>>>>
>>>>
>>>> Devicetree maintainers,
>>>>
>>>> Any thoughts?
>>>>
>>>> Tim,
>>>>
>>>> After looking at all the other watchdog drivers, it does not appear that
>>>> there is any other processor which uses a similar feature. Since imx is the
>>>> only processor that appears to support this feature, it might make sense in
>>>> making this vendor specific. If in the future it is found more processors
>>>> support a similar functionality, it can be revisited and moved out from
>>>> being vendor specific?
>>>>
>>>
>>> I'm certainly no expert on device-tree policy. I understand your
>>> point, but realize that the driver in question is imx2_wdt.c
>>> (compatible = "fsl,imx21-wdt"). This is an IP block inside the silicon
>>> of only Freescale chips, so its not like a future omap chip would be
>>> using this driver - only fsl devices. So why would it need a 'vendor'
>>> property any more than its other properties?
>>>
>>> Regards,
>>>
>>> Tim
>>
>> Wim,
>>
>> Does the lack of response mean overwhelming approval?
>>
>> I haven't heard any valid complaints - what does it take to get this approved?
>>
>> Regards,
>>
>> Tim
>
> I have no objections against the idea and the code itself.
> But as Guenter pointed out: it would be handy to get feedback from the devicetree maintainers on the above discussion.
>
> Kind regards,
> Wim.
>

Any suggestions on whether a vendor specific prefix is necessary?

Thanks,
Akshay

Re: [PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Akshay Bhat]

Rob,

On 12/28/2015 11:29 AM, Wim Van Sebroeck wrote:
> Hi Tim,
>
>> On Wed, Dec 2, 2015 at 12:54 PM, Tim Harvey <tharvey-UMMOYl/HMS+akBO8gow8eQ@public.gmane.org> wrote:
>>>
>>> On Wed, Dec 2, 2015 at 11:11 AM, Akshay Bhat <akshay.bhat-jEh4hwF5bVhBDgjK7y7TUQ@public.gmane.org> wrote:
>>>>
>>>>
>>>> On 11/06/2015 05:02 PM, Guenter Roeck wrote:
>>>>>
>>>>> On Fri, Nov 06, 2015 at 11:53:42AM -0800, Tim Harvey wrote:
>>>>>>
>>>>>> On Thu, Nov 5, 2015 at 2:23 PM, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
>>>>>>>
>>>>>>> On Thu, Nov 05, 2015 at 04:19:21PM -0500, Akshay Bhat wrote:
>>>>>>>>
>>>>>>>> From: Tim Harvey <tharvey-UMMOYl/HMS+akBO8gow8eQ@public.gmane.org>
>>>>>>>>
>>>>>>>> The IMX6 watchdog supports assertion of a signal (WDOG_B) which
>>>>>>>> can be pinmux'd to an external pin. This is typically used for boards
>>>>>>>> that
>>>>>>>> have PMIC's in control of the IMX6 power rails. In fact, failure to use
>>>>>>>> such an external reset on boards with external PMIC's can result in
>>>>>>>> various
>>>>>>>> hangs due to the IMX6 not being fully reset [1] as well as the board
>>>>>>>> failing
>>>>>>>> to reset because its PMIC has not been reset to provide adequate
>>>>>>>> voltage for
>>>>>>>> the CPU when coming out of reset at 800Mhz.
>>>>>>>>
>>>>>>>> This uses a new device-tree property 'ext-reset-output' to indicate the
>>>>>>>> board has such a reset and to cause the watchdog to be configured to
>>>>>>>> assert
>>>>>>>> WDOG_B instead of an internal reset both on a watchdog timeout and in
>>>>>>>> system_restart.
>>>>>>>>
>>>>>>>> [1]
>>>>>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html
>>>>>>>>
>>>>>>>> Cc: Lucas Stach <l.stach-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>>>>>>>> Signed-off-by: Tim Harvey <tharvey-UMMOYl/HMS+akBO8gow8eQ@public.gmane.org>
>>>>>>>> ---
>>>>>>>>    .../devicetree/bindings/watchdog/fsl-imx-wdt.txt     |  2 ++
>>>>>>>>    drivers/watchdog/imx2_wdt.c                          | 20
>>>>>>>> ++++++++++++++++++--
>>>>>>>>    2 files changed, 20 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>>>>> b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>>>>> index 8dab6fd..9b89b3a 100644
>>>>>>>> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>>>>> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>>>>> @@ -9,6 +9,8 @@ Optional property:
>>>>>>>
>>>>>>>
>>>>>>> properties ?
>>>>>>>
>>>>>>>>    - big-endian: If present the watchdog device's registers are
>>>>>>>> implemented
>>>>>>>>      in big endian mode, otherwise in native mode(same with CPU), for
>>>>>>>> more
>>>>>>>>      detail please see:
>>>>>>>> Documentation/devicetree/bindings/regmap/regmap.txt.
>>>>>>>> +- ext-reset-output: If present the watchdog device is configured to
>>>>>>>> assert its
>>>>>>>
>>>>>>>
>>>>>>> Should that have a vendor prefix ? Also, not sure if "-output"
>>>>>>> has any real value in the property name. "fsl,external-reset", maybe ?
>>>>>>
>>>>>>
>>>>>> Hi Guenter,
>>>>>>
>>>>>> I don't see why a vendor prefix is necessary - its a feature of the
>>>>>> IMX6 watchdog supported by this driver to be able to trigger an
>>>>>> internal chip-level reset and/or an external signal that can be hooked
>>>>>> to additional hardware.
>>>>>>
>>>>> Sounded like vendor specific to me, but then I am not a devicetree
>>>>> maintainer,
>>>>> so I am not an authority on the subject.
>>>>
>>>>
>>>> Devicetree maintainers,
>>>>
>>>> Any thoughts?
>>>>
>>>> Tim,
>>>>
>>>> After looking at all the other watchdog drivers, it does not appear that
>>>> there is any other processor which uses a similar feature. Since imx is the
>>>> only processor that appears to support this feature, it might make sense in
>>>> making this vendor specific. If in the future it is found more processors
>>>> support a similar functionality, it can be revisited and moved out from
>>>> being vendor specific?
>>>>
>>>
>>> I'm certainly no expert on device-tree policy. I understand your
>>> point, but realize that the driver in question is imx2_wdt.c
>>> (compatible = "fsl,imx21-wdt"). This is an IP block inside the silicon
>>> of only Freescale chips, so its not like a future omap chip would be
>>> using this driver - only fsl devices. So why would it need a 'vendor'
>>> property any more than its other properties?
>>>
>>> Regards,
>>>
>>> Tim
>>
>> Wim,
>>
>> Does the lack of response mean overwhelming approval?
>>
>> I haven't heard any valid complaints - what does it take to get this approved?
>>
>> Regards,
>>
>> Tim
>
> I have no objections against the idea and the code itself.
> But as Guenter pointed out: it would be handy to get feedback from the devicetree maintainers on the above discussion.
>
> Kind regards,
> Wim.
>

Any suggestions on whether a vendor specific prefix is necessary?

Thanks,
Akshay
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" 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 v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Akshay Bhat]

Rob,

On 12/28/2015 11:29 AM, Wim Van Sebroeck wrote:
> Hi Tim,
>
>> On Wed, Dec 2, 2015 at 12:54 PM, Tim Harvey <tharvey@gateworks.com> wrote:
>>>
>>> On Wed, Dec 2, 2015 at 11:11 AM, Akshay Bhat <akshay.bhat@timesys.com> wrote:
>>>>
>>>>
>>>> On 11/06/2015 05:02 PM, Guenter Roeck wrote:
>>>>>
>>>>> On Fri, Nov 06, 2015 at 11:53:42AM -0800, Tim Harvey wrote:
>>>>>>
>>>>>> On Thu, Nov 5, 2015 at 2:23 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>>>>
>>>>>>> On Thu, Nov 05, 2015 at 04:19:21PM -0500, Akshay Bhat wrote:
>>>>>>>>
>>>>>>>> From: Tim Harvey <tharvey@gateworks.com>
>>>>>>>>
>>>>>>>> The IMX6 watchdog supports assertion of a signal (WDOG_B) which
>>>>>>>> can be pinmux'd to an external pin. This is typically used for boards
>>>>>>>> that
>>>>>>>> have PMIC's in control of the IMX6 power rails. In fact, failure to use
>>>>>>>> such an external reset on boards with external PMIC's can result in
>>>>>>>> various
>>>>>>>> hangs due to the IMX6 not being fully reset [1] as well as the board
>>>>>>>> failing
>>>>>>>> to reset because its PMIC has not been reset to provide adequate
>>>>>>>> voltage for
>>>>>>>> the CPU when coming out of reset at 800Mhz.
>>>>>>>>
>>>>>>>> This uses a new device-tree property 'ext-reset-output' to indicate the
>>>>>>>> board has such a reset and to cause the watchdog to be configured to
>>>>>>>> assert
>>>>>>>> WDOG_B instead of an internal reset both on a watchdog timeout and in
>>>>>>>> system_restart.
>>>>>>>>
>>>>>>>> [1]
>>>>>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html
>>>>>>>>
>>>>>>>> Cc: Lucas Stach <l.stach@pengutronix.de>
>>>>>>>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>>>>>>>> ---
>>>>>>>>    .../devicetree/bindings/watchdog/fsl-imx-wdt.txt     |  2 ++
>>>>>>>>    drivers/watchdog/imx2_wdt.c                          | 20
>>>>>>>> ++++++++++++++++++--
>>>>>>>>    2 files changed, 20 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>>>>> b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>>>>> index 8dab6fd..9b89b3a 100644
>>>>>>>> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>>>>> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>>>>> @@ -9,6 +9,8 @@ Optional property:
>>>>>>>
>>>>>>>
>>>>>>> properties ?
>>>>>>>
>>>>>>>>    - big-endian: If present the watchdog device's registers are
>>>>>>>> implemented
>>>>>>>>      in big endian mode, otherwise in native mode(same with CPU), for
>>>>>>>> more
>>>>>>>>      detail please see:
>>>>>>>> Documentation/devicetree/bindings/regmap/regmap.txt.
>>>>>>>> +- ext-reset-output: If present the watchdog device is configured to
>>>>>>>> assert its
>>>>>>>
>>>>>>>
>>>>>>> Should that have a vendor prefix ? Also, not sure if "-output"
>>>>>>> has any real value in the property name. "fsl,external-reset", maybe ?
>>>>>>
>>>>>>
>>>>>> Hi Guenter,
>>>>>>
>>>>>> I don't see why a vendor prefix is necessary - its a feature of the
>>>>>> IMX6 watchdog supported by this driver to be able to trigger an
>>>>>> internal chip-level reset and/or an external signal that can be hooked
>>>>>> to additional hardware.
>>>>>>
>>>>> Sounded like vendor specific to me, but then I am not a devicetree
>>>>> maintainer,
>>>>> so I am not an authority on the subject.
>>>>
>>>>
>>>> Devicetree maintainers,
>>>>
>>>> Any thoughts?
>>>>
>>>> Tim,
>>>>
>>>> After looking at all the other watchdog drivers, it does not appear that
>>>> there is any other processor which uses a similar feature. Since imx is the
>>>> only processor that appears to support this feature, it might make sense in
>>>> making this vendor specific. If in the future it is found more processors
>>>> support a similar functionality, it can be revisited and moved out from
>>>> being vendor specific?
>>>>
>>>
>>> I'm certainly no expert on device-tree policy. I understand your
>>> point, but realize that the driver in question is imx2_wdt.c
>>> (compatible = "fsl,imx21-wdt"). This is an IP block inside the silicon
>>> of only Freescale chips, so its not like a future omap chip would be
>>> using this driver - only fsl devices. So why would it need a 'vendor'
>>> property any more than its other properties?
>>>
>>> Regards,
>>>
>>> Tim
>>
>> Wim,
>>
>> Does the lack of response mean overwhelming approval?
>>
>> I haven't heard any valid complaints - what does it take to get this approved?
>>
>> Regards,
>>
>> Tim
>
> I have no objections against the idea and the code itself.
> But as Guenter pointed out: it would be handy to get feedback from the devicetree maintainers on the above discussion.
>
> Kind regards,
> Wim.
>

Any suggestions on whether a vendor specific prefix is necessary?

Thanks,
Akshay

[PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Akshay Bhat]

Rob,

On 12/28/2015 11:29 AM, Wim Van Sebroeck wrote:
> Hi Tim,
>
>> On Wed, Dec 2, 2015 at 12:54 PM, Tim Harvey <tharvey@gateworks.com> wrote:
>>>
>>> On Wed, Dec 2, 2015 at 11:11 AM, Akshay Bhat <akshay.bhat@timesys.com> wrote:
>>>>
>>>>
>>>> On 11/06/2015 05:02 PM, Guenter Roeck wrote:
>>>>>
>>>>> On Fri, Nov 06, 2015 at 11:53:42AM -0800, Tim Harvey wrote:
>>>>>>
>>>>>> On Thu, Nov 5, 2015 at 2:23 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>>>>
>>>>>>> On Thu, Nov 05, 2015 at 04:19:21PM -0500, Akshay Bhat wrote:
>>>>>>>>
>>>>>>>> From: Tim Harvey <tharvey@gateworks.com>
>>>>>>>>
>>>>>>>> The IMX6 watchdog supports assertion of a signal (WDOG_B) which
>>>>>>>> can be pinmux'd to an external pin. This is typically used for boards
>>>>>>>> that
>>>>>>>> have PMIC's in control of the IMX6 power rails. In fact, failure to use
>>>>>>>> such an external reset on boards with external PMIC's can result in
>>>>>>>> various
>>>>>>>> hangs due to the IMX6 not being fully reset [1] as well as the board
>>>>>>>> failing
>>>>>>>> to reset because its PMIC has not been reset to provide adequate
>>>>>>>> voltage for
>>>>>>>> the CPU when coming out of reset at 800Mhz.
>>>>>>>>
>>>>>>>> This uses a new device-tree property 'ext-reset-output' to indicate the
>>>>>>>> board has such a reset and to cause the watchdog to be configured to
>>>>>>>> assert
>>>>>>>> WDOG_B instead of an internal reset both on a watchdog timeout and in
>>>>>>>> system_restart.
>>>>>>>>
>>>>>>>> [1]
>>>>>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333689.html
>>>>>>>>
>>>>>>>> Cc: Lucas Stach <l.stach@pengutronix.de>
>>>>>>>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>>>>>>>> ---
>>>>>>>>    .../devicetree/bindings/watchdog/fsl-imx-wdt.txt     |  2 ++
>>>>>>>>    drivers/watchdog/imx2_wdt.c                          | 20
>>>>>>>> ++++++++++++++++++--
>>>>>>>>    2 files changed, 20 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>>>>> b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>>>>> index 8dab6fd..9b89b3a 100644
>>>>>>>> --- a/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>>>>> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx-wdt.txt
>>>>>>>> @@ -9,6 +9,8 @@ Optional property:
>>>>>>>
>>>>>>>
>>>>>>> properties ?
>>>>>>>
>>>>>>>>    - big-endian: If present the watchdog device's registers are
>>>>>>>> implemented
>>>>>>>>      in big endian mode, otherwise in native mode(same with CPU), for
>>>>>>>> more
>>>>>>>>      detail please see:
>>>>>>>> Documentation/devicetree/bindings/regmap/regmap.txt.
>>>>>>>> +- ext-reset-output: If present the watchdog device is configured to
>>>>>>>> assert its
>>>>>>>
>>>>>>>
>>>>>>> Should that have a vendor prefix ? Also, not sure if "-output"
>>>>>>> has any real value in the property name. "fsl,external-reset", maybe ?
>>>>>>
>>>>>>
>>>>>> Hi Guenter,
>>>>>>
>>>>>> I don't see why a vendor prefix is necessary - its a feature of the
>>>>>> IMX6 watchdog supported by this driver to be able to trigger an
>>>>>> internal chip-level reset and/or an external signal that can be hooked
>>>>>> to additional hardware.
>>>>>>
>>>>> Sounded like vendor specific to me, but then I am not a devicetree
>>>>> maintainer,
>>>>> so I am not an authority on the subject.
>>>>
>>>>
>>>> Devicetree maintainers,
>>>>
>>>> Any thoughts?
>>>>
>>>> Tim,
>>>>
>>>> After looking at all the other watchdog drivers, it does not appear that
>>>> there is any other processor which uses a similar feature. Since imx is the
>>>> only processor that appears to support this feature, it might make sense in
>>>> making this vendor specific. If in the future it is found more processors
>>>> support a similar functionality, it can be revisited and moved out from
>>>> being vendor specific?
>>>>
>>>
>>> I'm certainly no expert on device-tree policy. I understand your
>>> point, but realize that the driver in question is imx2_wdt.c
>>> (compatible = "fsl,imx21-wdt"). This is an IP block inside the silicon
>>> of only Freescale chips, so its not like a future omap chip would be
>>> using this driver - only fsl devices. So why would it need a 'vendor'
>>> property any more than its other properties?
>>>
>>> Regards,
>>>
>>> Tim
>>
>> Wim,
>>
>> Does the lack of response mean overwhelming approval?
>>
>> I haven't heard any valid complaints - what does it take to get this approved?
>>
>> Regards,
>>
>> Tim
>
> I have no objections against the idea and the code itself.
> But as Guenter pointed out: it would be handy to get feedback from the devicetree maintainers on the above discussion.
>
> Kind regards,
> Wim.
>

Any suggestions on whether a vendor specific prefix is necessary?

Thanks,
Akshay

Re: [PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Fabio Estevam]

Rob,

On Thu, Jan 28, 2016 at 6:28 PM, Akshay Bhat <akshay.bhat@timesys.com> wrote:

>> I have no objections against the idea and the code itself.
>> But as Guenter pointed out: it would be handy to get feedback from the
>> devicetree maintainers on the above discussion.
>>
>> Kind regards,
>> Wim.
>>
>
> Any suggestions on whether a vendor specific prefix is necessary?

Any comments?

Re: [PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Fabio Estevam]

Rob,

On Thu, Jan 28, 2016 at 6:28 PM, Akshay Bhat <akshay.bhat-jEh4hwF5bVhBDgjK7y7TUQ@public.gmane.org> wrote:

>> I have no objections against the idea and the code itself.
>> But as Guenter pointed out: it would be handy to get feedback from the
>> devicetree maintainers on the above discussion.
>>
>> Kind regards,
>> Wim.
>>
>
> Any suggestions on whether a vendor specific prefix is necessary?

Any comments?
--
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 v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Fabio Estevam]

Rob,

On Thu, Jan 28, 2016 at 6:28 PM, Akshay Bhat <akshay.bhat@timesys.com> wrote:

>> I have no objections against the idea and the code itself.
>> But as Guenter pointed out: it would be handy to get feedback from the
>> devicetree maintainers on the above discussion.
>>
>> Kind regards,
>> Wim.
>>
>
> Any suggestions on whether a vendor specific prefix is necessary?

Any comments?

[PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Fabio Estevam]

Rob,

On Thu, Jan 28, 2016 at 6:28 PM, Akshay Bhat <akshay.bhat@timesys.com> wrote:

>> I have no objections against the idea and the code itself.
>> But as Guenter pointed out: it would be handy to get feedback from the
>> devicetree maintainers on the above discussion.
>>
>> Kind regards,
>> Wim.
>>
>
> Any suggestions on whether a vendor specific prefix is necessary?

Any comments?

Re: [PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Akshay Bhat]

Hi Shawn,

On 02/28/2016 08:56 AM, Fabio Estevam wrote:
> Rob,
>
> On Thu, Jan 28, 2016 at 6:28 PM, Akshay Bhat <akshay.bhat@timesys.com> wrote:
>
>>> I have no objections against the idea and the code itself.
>>> But as Guenter pointed out: it would be handy to get feedback from the
>>> devicetree maintainers on the above discussion.
>>>
>>> Kind regards,
>>> Wim.
>>>
>>
>> Any suggestions on whether a vendor specific prefix is necessary?
>
> Any comments?
>

Is this something you can help with, since you are the iMX 
architecture/devicetree maintainer? Appreciate any feedback.

Thanks,
Akshay

Re: [PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Akshay Bhat]

Hi Shawn,

On 02/28/2016 08:56 AM, Fabio Estevam wrote:
> Rob,
>
> On Thu, Jan 28, 2016 at 6:28 PM, Akshay Bhat <akshay.bhat-jEh4hwF5bVhBDgjK7y7TUQ@public.gmane.org> wrote:
>
>>> I have no objections against the idea and the code itself.
>>> But as Guenter pointed out: it would be handy to get feedback from the
>>> devicetree maintainers on the above discussion.
>>>
>>> Kind regards,
>>> Wim.
>>>
>>
>> Any suggestions on whether a vendor specific prefix is necessary?
>
> Any comments?
>

Is this something you can help with, since you are the iMX 
architecture/devicetree maintainer? Appreciate any feedback.

Thanks,
Akshay
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" 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 v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Akshay Bhat]

Hi Shawn,

On 02/28/2016 08:56 AM, Fabio Estevam wrote:
> Rob,
>
> On Thu, Jan 28, 2016 at 6:28 PM, Akshay Bhat <akshay.bhat@timesys.com> wrote:
>
>>> I have no objections against the idea and the code itself.
>>> But as Guenter pointed out: it would be handy to get feedback from the
>>> devicetree maintainers on the above discussion.
>>>
>>> Kind regards,
>>> Wim.
>>>
>>
>> Any suggestions on whether a vendor specific prefix is necessary?
>
> Any comments?
>

Is this something you can help with, since you are the iMX 
architecture/devicetree maintainer? Appreciate any feedback.

Thanks,
Akshay

[PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Akshay Bhat]

Hi Shawn,

On 02/28/2016 08:56 AM, Fabio Estevam wrote:
> Rob,
>
> On Thu, Jan 28, 2016 at 6:28 PM, Akshay Bhat <akshay.bhat@timesys.com> wrote:
>
>>> I have no objections against the idea and the code itself.
>>> But as Guenter pointed out: it would be handy to get feedback from the
>>> devicetree maintainers on the above discussion.
>>>
>>> Kind regards,
>>> Wim.
>>>
>>
>> Any suggestions on whether a vendor specific prefix is necessary?
>
> Any comments?
>

Is this something you can help with, since you are the iMX 
architecture/devicetree maintainer? Appreciate any feedback.

Thanks,
Akshay

Re: [PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Shawn Guo]

On Mon, Mar 28, 2016 at 04:19:15PM -0400, Akshay Bhat wrote:
> Hi Shawn,
> 
> On 02/28/2016 08:56 AM, Fabio Estevam wrote:
> >Rob,
> >
> >On Thu, Jan 28, 2016 at 6:28 PM, Akshay Bhat <akshay.bhat@timesys.com> wrote:
> >
> >>>I have no objections against the idea and the code itself.
> >>>But as Guenter pointed out: it would be handy to get feedback from the
> >>>devicetree maintainers on the above discussion.
> >>>
> >>>Kind regards,
> >>>Wim.
> >>>
> >>
> >>Any suggestions on whether a vendor specific prefix is necessary?
> >
> >Any comments?
> >
> 
> Is this something you can help with, since you are the iMX
> architecture/devicetree maintainer? Appreciate any feedback.

FWIW,

Acked-by: Shawn Guo <shawnguo@kernel.org>

Guenter,

I think it's reasonable to pick up the patch, if we have it copied to DT
maintainers and on the list for a long period of time, and do not see
any objection from anyone.

Shawn

Re: [PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Shawn Guo]

On Mon, Mar 28, 2016 at 04:19:15PM -0400, Akshay Bhat wrote:
> Hi Shawn,
> 
> On 02/28/2016 08:56 AM, Fabio Estevam wrote:
> >Rob,
> >
> >On Thu, Jan 28, 2016 at 6:28 PM, Akshay Bhat <akshay.bhat@timesys.com> wrote:
> >
> >>>I have no objections against the idea and the code itself.
> >>>But as Guenter pointed out: it would be handy to get feedback from the
> >>>devicetree maintainers on the above discussion.
> >>>
> >>>Kind regards,
> >>>Wim.
> >>>
> >>
> >>Any suggestions on whether a vendor specific prefix is necessary?
> >
> >Any comments?
> >
> 
> Is this something you can help with, since you are the iMX
> architecture/devicetree maintainer? Appreciate any feedback.

FWIW,

Acked-by: Shawn Guo <shawnguo@kernel.org>

Guenter,

I think it's reasonable to pick up the patch, if we have it copied to DT
maintainers and on the list for a long period of time, and do not see
any objection from anyone.

Shawn

[PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Shawn Guo]

On Mon, Mar 28, 2016 at 04:19:15PM -0400, Akshay Bhat wrote:
> Hi Shawn,
> 
> On 02/28/2016 08:56 AM, Fabio Estevam wrote:
> >Rob,
> >
> >On Thu, Jan 28, 2016 at 6:28 PM, Akshay Bhat <akshay.bhat@timesys.com> wrote:
> >
> >>>I have no objections against the idea and the code itself.
> >>>But as Guenter pointed out: it would be handy to get feedback from the
> >>>devicetree maintainers on the above discussion.
> >>>
> >>>Kind regards,
> >>>Wim.
> >>>
> >>
> >>Any suggestions on whether a vendor specific prefix is necessary?
> >
> >Any comments?
> >
> 
> Is this something you can help with, since you are the iMX
> architecture/devicetree maintainer? Appreciate any feedback.

FWIW,

Acked-by: Shawn Guo <shawnguo@kernel.org>

Guenter,

I think it's reasonable to pick up the patch, if we have it copied to DT
maintainers and on the list for a long period of time, and do not see
any objection from anyone.

Shawn

Re: [PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Guenter Roeck]

On Wed, Mar 30, 2016 at 09:22:33AM +0800, Shawn Guo wrote:
> On Mon, Mar 28, 2016 at 04:19:15PM -0400, Akshay Bhat wrote:
> > Hi Shawn,
> > 
> > On 02/28/2016 08:56 AM, Fabio Estevam wrote:
> > >Rob,
> > >
> > >On Thu, Jan 28, 2016 at 6:28 PM, Akshay Bhat <akshay.bhat@timesys.com> wrote:
> > >
> > >>>I have no objections against the idea and the code itself.
> > >>>But as Guenter pointed out: it would be handy to get feedback from the
> > >>>devicetree maintainers on the above discussion.
> > >>>
> > >>>Kind regards,
> > >>>Wim.
> > >>>
> > >>
> > >>Any suggestions on whether a vendor specific prefix is necessary?
> > >
> > >Any comments?
> > >
> > 
> > Is this something you can help with, since you are the iMX
> > architecture/devicetree maintainer? Appreciate any feedback.
> 
> FWIW,
> 
> Acked-by: Shawn Guo <shawnguo@kernel.org>
> 
> Guenter,
> 
> I think it's reasonable to pick up the patch, if we have it copied to DT
> maintainers and on the list for a long period of time, and do not see
> any objection from anyone.
> 
The question was if the property name should be ext-reset-output or
fsl,ext-reset-output. In my opinion, it should be fsl,ext-reset-output
because it is not a generic property. Tim disagrees.

So we have two options: Change the property name to fsl,ext-reset-output,
which I would accept, or wait for a devicetree maintainer to make a decision.

Guenter

Re: [PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Guenter Roeck]

On Wed, Mar 30, 2016 at 09:22:33AM +0800, Shawn Guo wrote:
> On Mon, Mar 28, 2016 at 04:19:15PM -0400, Akshay Bhat wrote:
> > Hi Shawn,
> > 
> > On 02/28/2016 08:56 AM, Fabio Estevam wrote:
> > >Rob,
> > >
> > >On Thu, Jan 28, 2016 at 6:28 PM, Akshay Bhat <akshay.bhat-jEh4hwF5bVhBDgjK7y7TUQ@public.gmane.org> wrote:
> > >
> > >>>I have no objections against the idea and the code itself.
> > >>>But as Guenter pointed out: it would be handy to get feedback from the
> > >>>devicetree maintainers on the above discussion.
> > >>>
> > >>>Kind regards,
> > >>>Wim.
> > >>>
> > >>
> > >>Any suggestions on whether a vendor specific prefix is necessary?
> > >
> > >Any comments?
> > >
> > 
> > Is this something you can help with, since you are the iMX
> > architecture/devicetree maintainer? Appreciate any feedback.
> 
> FWIW,
> 
> Acked-by: Shawn Guo <shawnguo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> 
> Guenter,
> 
> I think it's reasonable to pick up the patch, if we have it copied to DT
> maintainers and on the list for a long period of time, and do not see
> any objection from anyone.
> 
The question was if the property name should be ext-reset-output or
fsl,ext-reset-output. In my opinion, it should be fsl,ext-reset-output
because it is not a generic property. Tim disagrees.

So we have two options: Change the property name to fsl,ext-reset-output,
which I would accept, or wait for a devicetree maintainer to make a decision.

Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" 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 v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Guenter Roeck]

On Wed, Mar 30, 2016 at 09:22:33AM +0800, Shawn Guo wrote:
> On Mon, Mar 28, 2016 at 04:19:15PM -0400, Akshay Bhat wrote:
> > Hi Shawn,
> > 
> > On 02/28/2016 08:56 AM, Fabio Estevam wrote:
> > >Rob,
> > >
> > >On Thu, Jan 28, 2016 at 6:28 PM, Akshay Bhat <akshay.bhat@timesys.com> wrote:
> > >
> > >>>I have no objections against the idea and the code itself.
> > >>>But as Guenter pointed out: it would be handy to get feedback from the
> > >>>devicetree maintainers on the above discussion.
> > >>>
> > >>>Kind regards,
> > >>>Wim.
> > >>>
> > >>
> > >>Any suggestions on whether a vendor specific prefix is necessary?
> > >
> > >Any comments?
> > >
> > 
> > Is this something you can help with, since you are the iMX
> > architecture/devicetree maintainer? Appreciate any feedback.
> 
> FWIW,
> 
> Acked-by: Shawn Guo <shawnguo@kernel.org>
> 
> Guenter,
> 
> I think it's reasonable to pick up the patch, if we have it copied to DT
> maintainers and on the list for a long period of time, and do not see
> any objection from anyone.
> 
The question was if the property name should be ext-reset-output or
fsl,ext-reset-output. In my opinion, it should be fsl,ext-reset-output
because it is not a generic property. Tim disagrees.

So we have two options: Change the property name to fsl,ext-reset-output,
which I would accept, or wait for a devicetree maintainer to make a decision.

Guenter

[PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Guenter Roeck]

On Wed, Mar 30, 2016 at 09:22:33AM +0800, Shawn Guo wrote:
> On Mon, Mar 28, 2016 at 04:19:15PM -0400, Akshay Bhat wrote:
> > Hi Shawn,
> > 
> > On 02/28/2016 08:56 AM, Fabio Estevam wrote:
> > >Rob,
> > >
> > >On Thu, Jan 28, 2016 at 6:28 PM, Akshay Bhat <akshay.bhat@timesys.com> wrote:
> > >
> > >>>I have no objections against the idea and the code itself.
> > >>>But as Guenter pointed out: it would be handy to get feedback from the
> > >>>devicetree maintainers on the above discussion.
> > >>>
> > >>>Kind regards,
> > >>>Wim.
> > >>>
> > >>
> > >>Any suggestions on whether a vendor specific prefix is necessary?
> > >
> > >Any comments?
> > >
> > 
> > Is this something you can help with, since you are the iMX
> > architecture/devicetree maintainer? Appreciate any feedback.
> 
> FWIW,
> 
> Acked-by: Shawn Guo <shawnguo@kernel.org>
> 
> Guenter,
> 
> I think it's reasonable to pick up the patch, if we have it copied to DT
> maintainers and on the list for a long period of time, and do not see
> any objection from anyone.
> 
The question was if the property name should be ext-reset-output or
fsl,ext-reset-output. In my opinion, it should be fsl,ext-reset-output
because it is not a generic property. Tim disagrees.

So we have two options: Change the property name to fsl,ext-reset-output,
which I would accept, or wait for a devicetree maintainer to make a decision.

Guenter

Re: [PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Shawn Guo]

On Wed, Mar 30, 2016 at 02:09:16PM -0700, Guenter Roeck wrote:
> The question was if the property name should be ext-reset-output or
> fsl,ext-reset-output. In my opinion, it should be fsl,ext-reset-output
> because it is not a generic property. Tim disagrees.
> 
> So we have two options: Change the property name to fsl,ext-reset-output,
> which I would accept, or wait for a devicetree maintainer to make a decision.

Guenter,

I agree with you on this point.  Before everyone agrees that this is a
generic binding, we should have vendor prefix for the property.

Tim,

This is a small change which, IMO, shouldn't hold an useful patch from being
merged.  Care to resend with the suggested change?

Shawn

Re: [PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Shawn Guo]

On Wed, Mar 30, 2016 at 02:09:16PM -0700, Guenter Roeck wrote:
> The question was if the property name should be ext-reset-output or
> fsl,ext-reset-output. In my opinion, it should be fsl,ext-reset-output
> because it is not a generic property. Tim disagrees.
> 
> So we have two options: Change the property name to fsl,ext-reset-output,
> which I would accept, or wait for a devicetree maintainer to make a decision.

Guenter,

I agree with you on this point.  Before everyone agrees that this is a
generic binding, we should have vendor prefix for the property.

Tim,

This is a small change which, IMO, shouldn't hold an useful patch from being
merged.  Care to resend with the suggested change?

Shawn

[PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Shawn Guo]

On Wed, Mar 30, 2016 at 02:09:16PM -0700, Guenter Roeck wrote:
> The question was if the property name should be ext-reset-output or
> fsl,ext-reset-output. In my opinion, it should be fsl,ext-reset-output
> because it is not a generic property. Tim disagrees.
> 
> So we have two options: Change the property name to fsl,ext-reset-output,
> which I would accept, or wait for a devicetree maintainer to make a decision.

Guenter,

I agree with you on this point.  Before everyone agrees that this is a
generic binding, we should have vendor prefix for the property.

Tim,

This is a small change which, IMO, shouldn't hold an useful patch from being
merged.  Care to resend with the suggested change?

Shawn

Re: [PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Tim Harvey]

On Wed, Mar 30, 2016 at 6:57 PM, Shawn Guo <shawnguo@kernel.org> wrote:
> On Wed, Mar 30, 2016 at 02:09:16PM -0700, Guenter Roeck wrote:
>> The question was if the property name should be ext-reset-output or
>> fsl,ext-reset-output. In my opinion, it should be fsl,ext-reset-output
>> because it is not a generic property. Tim disagrees.
>>

Guenter,

My issue regarding the vendor prefix was not a hard dissagreement but
was more about me understanding the rational behind using vendor
prefixes. In this case the imx2_wdt driver 'is' a vendor specific
driver as its compatible strings are prefixed with 'fsl,' so isn't
'any' property added to devicetree/bindings/watchdog/fsl-imx-wdt.txt
inherently a vendor specific properly already? I assume that is why
the 'big-endian' property isn't 'fsl,big-endian'.

Maybe it's more about if it 'is' marked as a vendor specific property
its much easier to get approval and accepted by a vendor-specific
maintainer?

>> So we have two options: Change the property name to fsl,ext-reset-output,
>> which I would accept, or wait for a devicetree maintainer to make a decision.
>
> Guenter,
>
> I agree with you on this point.  Before everyone agrees that this is a
> generic binding, we should have vendor prefix for the property.
>
> Tim,
>
> This is a small change which, IMO, shouldn't hold an useful patch from being
> merged.  Care to resend with the suggested change?
>
> Shawn

Shawn,

Sure - I'll rebase and re-submit.

Tim

Re: [PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Tim Harvey]

On Wed, Mar 30, 2016 at 6:57 PM, Shawn Guo <shawnguo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Wed, Mar 30, 2016 at 02:09:16PM -0700, Guenter Roeck wrote:
>> The question was if the property name should be ext-reset-output or
>> fsl,ext-reset-output. In my opinion, it should be fsl,ext-reset-output
>> because it is not a generic property. Tim disagrees.
>>

Guenter,

My issue regarding the vendor prefix was not a hard dissagreement but
was more about me understanding the rational behind using vendor
prefixes. In this case the imx2_wdt driver 'is' a vendor specific
driver as its compatible strings are prefixed with 'fsl,' so isn't
'any' property added to devicetree/bindings/watchdog/fsl-imx-wdt.txt
inherently a vendor specific properly already? I assume that is why
the 'big-endian' property isn't 'fsl,big-endian'.

Maybe it's more about if it 'is' marked as a vendor specific property
its much easier to get approval and accepted by a vendor-specific
maintainer?

>> So we have two options: Change the property name to fsl,ext-reset-output,
>> which I would accept, or wait for a devicetree maintainer to make a decision.
>
> Guenter,
>
> I agree with you on this point.  Before everyone agrees that this is a
> generic binding, we should have vendor prefix for the property.
>
> Tim,
>
> This is a small change which, IMO, shouldn't hold an useful patch from being
> merged.  Care to resend with the suggested change?
>
> Shawn

Shawn,

Sure - I'll rebase and re-submit.

Tim
--
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 v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Tim Harvey]

On Wed, Mar 30, 2016 at 6:57 PM, Shawn Guo <shawnguo@kernel.org> wrote:
> On Wed, Mar 30, 2016 at 02:09:16PM -0700, Guenter Roeck wrote:
>> The question was if the property name should be ext-reset-output or
>> fsl,ext-reset-output. In my opinion, it should be fsl,ext-reset-output
>> because it is not a generic property. Tim disagrees.
>>

Guenter,

My issue regarding the vendor prefix was not a hard dissagreement but
was more about me understanding the rational behind using vendor
prefixes. In this case the imx2_wdt driver 'is' a vendor specific
driver as its compatible strings are prefixed with 'fsl,' so isn't
'any' property added to devicetree/bindings/watchdog/fsl-imx-wdt.txt
inherently a vendor specific properly already? I assume that is why
the 'big-endian' property isn't 'fsl,big-endian'.

Maybe it's more about if it 'is' marked as a vendor specific property
its much easier to get approval and accepted by a vendor-specific
maintainer?

>> So we have two options: Change the property name to fsl,ext-reset-output,
>> which I would accept, or wait for a devicetree maintainer to make a decision.
>
> Guenter,
>
> I agree with you on this point.  Before everyone agrees that this is a
> generic binding, we should have vendor prefix for the property.
>
> Tim,
>
> This is a small change which, IMO, shouldn't hold an useful patch from being
> merged.  Care to resend with the suggested change?
>
> Shawn

Shawn,

Sure - I'll rebase and re-submit.

Tim

[PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Tim Harvey]

On Wed, Mar 30, 2016 at 6:57 PM, Shawn Guo <shawnguo@kernel.org> wrote:
> On Wed, Mar 30, 2016 at 02:09:16PM -0700, Guenter Roeck wrote:
>> The question was if the property name should be ext-reset-output or
>> fsl,ext-reset-output. In my opinion, it should be fsl,ext-reset-output
>> because it is not a generic property. Tim disagrees.
>>

Guenter,

My issue regarding the vendor prefix was not a hard dissagreement but
was more about me understanding the rational behind using vendor
prefixes. In this case the imx2_wdt driver 'is' a vendor specific
driver as its compatible strings are prefixed with 'fsl,' so isn't
'any' property added to devicetree/bindings/watchdog/fsl-imx-wdt.txt
inherently a vendor specific properly already? I assume that is why
the 'big-endian' property isn't 'fsl,big-endian'.

Maybe it's more about if it 'is' marked as a vendor specific property
its much easier to get approval and accepted by a vendor-specific
maintainer?

>> So we have two options: Change the property name to fsl,ext-reset-output,
>> which I would accept, or wait for a devicetree maintainer to make a decision.
>
> Guenter,
>
> I agree with you on this point.  Before everyone agrees that this is a
> generic binding, we should have vendor prefix for the property.
>
> Tim,
>
> This is a small change which, IMO, shouldn't hold an useful patch from being
> merged.  Care to resend with the suggested change?
>
> Shawn

Shawn,

Sure - I'll rebase and re-submit.

Tim

Re: [PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Shawn Guo]

On Thu, Mar 31, 2016 at 11:01:58AM -0700, Tim Harvey wrote:
> On Wed, Mar 30, 2016 at 6:57 PM, Shawn Guo <shawnguo@kernel.org> wrote:
> > On Wed, Mar 30, 2016 at 02:09:16PM -0700, Guenter Roeck wrote:
> >> The question was if the property name should be ext-reset-output or
> >> fsl,ext-reset-output. In my opinion, it should be fsl,ext-reset-output
> >> because it is not a generic property. Tim disagrees.
> >>
> 
> Guenter,
> 
> My issue regarding the vendor prefix was not a hard dissagreement but
> was more about me understanding the rational behind using vendor
> prefixes. In this case the imx2_wdt driver 'is' a vendor specific
> driver as its compatible strings are prefixed with 'fsl,' so isn't
> 'any' property added to devicetree/bindings/watchdog/fsl-imx-wdt.txt
> inherently a vendor specific properly already? I assume that is why
> the 'big-endian' property isn't 'fsl,big-endian'.

We should read it as that 'big-endian' is a generic property defined by
generic bindings - bindings/regmap/regmap.txt, and we just reference the
bindings in fsl-imx-wdt.txt.

Taking mmc bindings as example, bindings/mmc/mmc.txt defines generic
bindings, while bindings/mmc/fsl-imx-esdhc.txt defines i.MX vendor
specific properties, which should ideally have vendor prefix.

Shawn

Re: [PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Shawn Guo]

On Thu, Mar 31, 2016 at 11:01:58AM -0700, Tim Harvey wrote:
> On Wed, Mar 30, 2016 at 6:57 PM, Shawn Guo <shawnguo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > On Wed, Mar 30, 2016 at 02:09:16PM -0700, Guenter Roeck wrote:
> >> The question was if the property name should be ext-reset-output or
> >> fsl,ext-reset-output. In my opinion, it should be fsl,ext-reset-output
> >> because it is not a generic property. Tim disagrees.
> >>
> 
> Guenter,
> 
> My issue regarding the vendor prefix was not a hard dissagreement but
> was more about me understanding the rational behind using vendor
> prefixes. In this case the imx2_wdt driver 'is' a vendor specific
> driver as its compatible strings are prefixed with 'fsl,' so isn't
> 'any' property added to devicetree/bindings/watchdog/fsl-imx-wdt.txt
> inherently a vendor specific properly already? I assume that is why
> the 'big-endian' property isn't 'fsl,big-endian'.

We should read it as that 'big-endian' is a generic property defined by
generic bindings - bindings/regmap/regmap.txt, and we just reference the
bindings in fsl-imx-wdt.txt.

Taking mmc bindings as example, bindings/mmc/mmc.txt defines generic
bindings, while bindings/mmc/fsl-imx-esdhc.txt defines i.MX vendor
specific properties, which should ideally have vendor prefix.

Shawn
--
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 v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Shawn Guo]

On Thu, Mar 31, 2016 at 11:01:58AM -0700, Tim Harvey wrote:
> On Wed, Mar 30, 2016 at 6:57 PM, Shawn Guo <shawnguo@kernel.org> wrote:
> > On Wed, Mar 30, 2016 at 02:09:16PM -0700, Guenter Roeck wrote:
> >> The question was if the property name should be ext-reset-output or
> >> fsl,ext-reset-output. In my opinion, it should be fsl,ext-reset-output
> >> because it is not a generic property. Tim disagrees.
> >>
> 
> Guenter,
> 
> My issue regarding the vendor prefix was not a hard dissagreement but
> was more about me understanding the rational behind using vendor
> prefixes. In this case the imx2_wdt driver 'is' a vendor specific
> driver as its compatible strings are prefixed with 'fsl,' so isn't
> 'any' property added to devicetree/bindings/watchdog/fsl-imx-wdt.txt
> inherently a vendor specific properly already? I assume that is why
> the 'big-endian' property isn't 'fsl,big-endian'.

We should read it as that 'big-endian' is a generic property defined by
generic bindings - bindings/regmap/regmap.txt, and we just reference the
bindings in fsl-imx-wdt.txt.

Taking mmc bindings as example, bindings/mmc/mmc.txt defines generic
bindings, while bindings/mmc/fsl-imx-esdhc.txt defines i.MX vendor
specific properties, which should ideally have vendor prefix.

Shawn

[PATCH v4 1/2] watchdog: imx2_wdt: add external reset support via 'ext-reset-output' dt prop

[Shawn Guo]

On Thu, Mar 31, 2016 at 11:01:58AM -0700, Tim Harvey wrote:
> On Wed, Mar 30, 2016 at 6:57 PM, Shawn Guo <shawnguo@kernel.org> wrote:
> > On Wed, Mar 30, 2016 at 02:09:16PM -0700, Guenter Roeck wrote:
> >> The question was if the property name should be ext-reset-output or
> >> fsl,ext-reset-output. In my opinion, it should be fsl,ext-reset-output
> >> because it is not a generic property. Tim disagrees.
> >>
> 
> Guenter,
> 
> My issue regarding the vendor prefix was not a hard dissagreement but
> was more about me understanding the rational behind using vendor
> prefixes. In this case the imx2_wdt driver 'is' a vendor specific
> driver as its compatible strings are prefixed with 'fsl,' so isn't
> 'any' property added to devicetree/bindings/watchdog/fsl-imx-wdt.txt
> inherently a vendor specific properly already? I assume that is why
> the 'big-endian' property isn't 'fsl,big-endian'.

We should read it as that 'big-endian' is a generic property defined by
generic bindings - bindings/regmap/regmap.txt, and we just reference the
bindings in fsl-imx-wdt.txt.

Taking mmc bindings as example, bindings/mmc/mmc.txt defines generic
bindings, while bindings/mmc/fsl-imx-esdhc.txt defines i.MX vendor
specific properties, which should ideally have vendor prefix.

Shawn