Skip to contentSkip to conversation

Add ASPEED watchdog device tree properties

PATCH In review Christopher Bostic 4 participants 8 messages Jul 17, 2017 – Aug 15, 2017

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

v5 cover Add ASPEED watchdog device tree properties

Document device tree optional properties for ASPEED watchdog. Reference properties in ASPEED watchdog driver and configure accordingly.

Christopher Bostic (2):
drivers/watchdog: Add optional ASPEED device tree properties drivers/watchdog: ASPEED reference dev tree properties for config

 .../devicetree/bindings/watchdog/aspeed-wdt.txt    | 32 ++++++++++++++++++++++
 drivers/watchdog/aspeed_wdt.c                      | 29 ++++++++++++++++----
 2 files changed, 56 insertions(+), 5 deletions(-)
Signature · 6 lines
1.8.2.2

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#1 lore raw

PATCH v5 1/2 drivers/watchdog: Add optional ASPEED device tree properties

Describe device tree optional properties:

* aspeed,reset-type = "cpu|soc|system|none" One of three different, mutually exclusive, values

"cpu" : ARM CPU reset on signal
"soc" : 'System on chip' reset
"system" : Full system reset

The value can also be set to "none" which indicates that no reset of any kind is to be done via this watchdog. This assumes another watchdog on the chip is to take care of resets.

* aspeed,external-signal - Generate external signal (WDT1 and WDT2 only)
* aspeed,alt-boot - Boot from alternate block on signal

---
v5 - Removed aspeed,interrupt property - no plans at this point to
need this functionality in the driver.
v4 - Add aspeed-reset-type and assign one of four values,
cpu, soc, system, none.
v3 - Invert soc and sys reset to 'no' to preserve backwards
compatibility. SOC and SYS reset will be set by default without any optional parameters set
v2 - Add 'aspeed,' prefix to all optional properties
- Add arm-reset, soc-reset, interrupt, alt-boot properties

---
 .../devicetree/bindings/watchdog/aspeed-wdt.txt    | 32 ++++++++++++++++++++++
 1 file changed, 32 insertions(+)
Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt +32 -0
index c5e74d7..2b34ce9 100644
@@ -8,9 +8,41 @@ Required properties:
  - reg: physical base address of the controller and length of memory mapped
    region
 
+Optional properties:
+
+ - aspeed,reset-type = "cpu|soc|system|none"
+
+   Reset behavior - Whenever a timeout occurs the watchdog can be programmed
+   to generate one of three different, mutually exclusive, types of resets.
+
+   Type "none" can be specified to indicate that no resets are to be done.
+   This is useful in situations where another watchdog engine on chip is
+   to perform the reset.
+
+   If 'aspeed,reset-type=' is not specfied the default is to enable system
+   reset.
+
+   Reset types:
+
+        - cpu: Reset CPU on watchdog timeout
+
+        - soc: Reset 'System on Chip' on watchdog timeout
+
+        - system: Reset system on watchdog timeout
+
+        - none: No reset is performed on timeout. Assumes another watchdog
+                engine is responsible for this.
+
+ - aspeed,external-signal: If property is present then signal is sent to
+			external reset counter (only WDT1 and WDT2). If not
+			specified no external signal is sent.
+ - aspeed,alt-boot:    If property is present then boot from alternate block.
+
 Example:
 
 	wdt1: watchdog@1e785000 {
 		compatible = "aspeed,ast2400-wdt";
 		reg = <0x1e785000 0x1c>;
+		aspeed,reset-type = "system";
+		aspeed,external-signal;
 	};
Signature · 1 line
1.8.2.2
maintainer Acked-by on v5 1/2
#2 lore raw

Re: [PATCH v5 1/2] drivers/watchdog: Add optional ASPEED device tree properties

Quoted 27 lines · Christopher Bostic

Describe device tree optional properties:

* aspeed,reset-type = "cpu|soc|system|none" One of three different, mutually exclusive, values

"cpu" : ARM CPU reset on signal
"soc" : 'System on chip' reset
"system" : Full system reset

The value can also be set to "none" which indicates that no reset of any kind is to be done via this watchdog. This assumes another watchdog on the chip is to take care of resets.

* aspeed,external-signal - Generate external signal (WDT1 and WDT2 only)
* aspeed,alt-boot - Boot from alternate block on signal

---
v5 - Removed aspeed,interrupt property - no plans at this point to
need this functionality in the driver.
v4 - Add aspeed-reset-type and assign one of four values,
cpu, soc, system, none.
v3 - Invert soc and sys reset to 'no' to preserve backwards
compatibility. SOC and SYS reset will be set by default without any optional parameters set
v2 - Add 'aspeed,' prefix to all optional properties
- Add arm-reset, soc-reset, interrupt, alt-boot properties

Quoted 3 lines
---
 .../devicetree/bindings/watchdog/aspeed-wdt.txt    | 32 ++++++++++++++++++++++
 1 file changed, 32 insertions(+)

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#3 lore raw

Re: [PATCH v5 1/2] drivers/watchdog: Add optional ASPEED device tree properties

In reply to Christopher Bostic
Quoted 66 lines · Christopher Bostic

Describe device tree optional properties:

  * aspeed,reset-type = "cpu|soc|system|none" One of three different, mutually exclusive, values

"cpu" : ARM CPU reset on signal
"soc" : 'System on chip' reset
"system" : Full system reset

     The value can also be set to "none" which indicates that no reset of any kind is to be done via this watchdog.  This assumes another watchdog on the chip is to take care of resets.

  * aspeed,external-signal - Generate external signal (WDT1 and WDT2 only)
  * aspeed,alt-boot - Boot from alternate block on signal

Quoted 1 line

---
v5 - Removed aspeed,interrupt property - no plans at this point to need this functionality in the driver.
v4 - Add aspeed-reset-type and assign one of four values,
     cpu, soc, system, none.
v3 - Invert soc and sys reset to 'no' to preserve backwards compatibility.  SOC and SYS reset will be set by default without any optional parameters set
v2 - Add 'aspeed,' prefix to all optional properties
   - Add arm-reset, soc-reset, interrupt, alt-boot properties
---
 .../devicetree/bindings/watchdog/aspeed-wdt.txt    | 32 ++++++++++++++++++++++

 1 file changed, 32 insertions(+)
Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt +0 -0
index c5e74d7..2b34ce9 100644
@@ -8,9 +8,41 @@ Required properties:

  - reg: physical base address of the controller and length of memory mapped region

Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt +26 -0
+Optional properties:
+
+ - aspeed,reset-type = "cpu|soc|system|none"
+
+   Reset behavior - Whenever a timeout occurs the watchdog can be programmed
+   to generate one of three different, mutually exclusive, types of resets.
+
+   Type "none" can be specified to indicate that no resets are to be done.
+   This is useful in situations where another watchdog engine on chip is
+   to perform the reset.
+
+   If 'aspeed,reset-type=' is not specfied the default is to enable system
+   reset.
+
+   Reset types:
+
+        - cpu: Reset CPU on watchdog timeout
+
+        - soc: Reset 'System on Chip' on watchdog timeout
+
+        - system: Reset system on watchdog timeout
+
+        - none: No reset is performed on timeout. Assumes another watchdog
+                engine is responsible for this.
+
+ - aspeed,external-signal: If property is present then signal is sent to
Quoted 1 line
Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt +1 -0
+			external reset counter (only WDT1 and WDT2). If not

Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt Required properties: · line 38

Quoted 1 line
Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt +1 -0
+			specified no external signal is sent.

Sorry I'm a little late on this one, but we should probably also add some words to the effect that if you specify aspeed,external-signal, then you also need to ensure the pinmux is configured for this to work. For example in the devicetree we need to add:

    pinctrl-names = "default";
    pinctrl-0 = <&pinctrl_wdtrst1_default>;

Now the name of the pinctrl node isn't set in stone, so I don't think we should mention it directly. However this is the essence of what we want to describe.

Cheers,

Andrew

Quoted 10 lines

+ - aspeed,alt-boot:    If property is present then boot from alternate block. +
 Example:

Quoted 6 lines
Quoted 1 line

  wdt1: watchdog@1e785000 {

  compatible = "aspeed,ast2400-wdt"; reg = <0x1e785000 0x1c>;

Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt +2 -0
+		aspeed,reset-type = "system";
+		aspeed,external-signal;

  };

maintainer Reviewed-by on v5 1/2
#4 lore raw

Re: [v5, 1/2] drivers/watchdog: Add optional ASPEED device tree properties

In reply to Christopher Bostic
Quoted 13 lines · Christopher Bostic

Describe device tree optional properties:

* aspeed,reset-type = "cpu|soc|system|none" One of three different, mutually exclusive, values

"cpu" : ARM CPU reset on signal
"soc" : 'System on chip' reset
"system" : Full system reset

The value can also be set to "none" which indicates that no reset of any kind is to be done via this watchdog. This assumes another watchdog on the chip is to take care of resets.

Quoted 5 lines

* aspeed,external-signal - Generate external signal (WDT1 and WDT2 only)
* aspeed,alt-boot - Boot from alternate block on signal

Quoted 50 lines

---
v5 - Removed aspeed,interrupt property - no plans at this point to
need this functionality in the driver.
v4 - Add aspeed-reset-type and assign one of four values,
cpu, soc, system, none.
v3 - Invert soc and sys reset to 'no' to preserve backwards
compatibility. SOC and SYS reset will be set by default without any optional parameters set
v2 - Add 'aspeed,' prefix to all optional properties
- Add arm-reset, soc-reset, interrupt, alt-boot properties

---
 .../devicetree/bindings/watchdog/aspeed-wdt.txt    | 32 ++++++++++++++++++++++
 1 file changed, 32 insertions(+)
Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt +28 -0
index c5e74d7..2b34ce9 100644
@@ -8,9 +8,41 @@ Required properties:
  - reg: physical base address of the controller and length of memory mapped
    region
 
+Optional properties:
+
+ - aspeed,reset-type = "cpu|soc|system|none"
+
+   Reset behavior - Whenever a timeout occurs the watchdog can be programmed
+   to generate one of three different, mutually exclusive, types of resets.
+
+   Type "none" can be specified to indicate that no resets are to be done.
+   This is useful in situations where another watchdog engine on chip is
+   to perform the reset.
+
+   If 'aspeed,reset-type=' is not specfied the default is to enable system
+   reset.
+
+   Reset types:
+
+        - cpu: Reset CPU on watchdog timeout
+
+        - soc: Reset 'System on Chip' on watchdog timeout
+
+        - system: Reset system on watchdog timeout
+
+        - none: No reset is performed on timeout. Assumes another watchdog
+                engine is responsible for this.
+
+ - aspeed,external-signal: If property is present then signal is sent to
+			external reset counter (only WDT1 and WDT2). If not
+			specified no external signal is sent.

Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt Required properties: · line 48

Quoted 10 lines
Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt +4 -0
+ - aspeed,alt-boot:    If property is present then boot from alternate block.
+
 Example:
 
 	wdt1: watchdog@1e785000 {
 		compatible = "aspeed,ast2400-wdt";
 		reg = <0x1e785000 0x1c>;
+		aspeed,reset-type = "system";
+		aspeed,external-signal;
 	};

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

maintainer Reviewed-by on v5 1/2
#5 lore raw

Re: [v5, 1/2] drivers/watchdog: Add optional ASPEED device tree properties

In reply to Christopher Bostic
Quoted 13 lines · Christopher Bostic

Describe device tree optional properties:

* aspeed,reset-type = "cpu|soc|system|none" One of three different, mutually exclusive, values

"cpu" : ARM CPU reset on signal
"soc" : 'System on chip' reset
"system" : Full system reset

The value can also be set to "none" which indicates that no reset of any kind is to be done via this watchdog. This assumes another watchdog on the chip is to take care of resets.

Quoted 5 lines

* aspeed,external-signal - Generate external signal (WDT1 and WDT2 only)
* aspeed,alt-boot - Boot from alternate block on signal

Quoted 50 lines

---
v5 - Removed aspeed,interrupt property - no plans at this point to
need this functionality in the driver.
v4 - Add aspeed-reset-type and assign one of four values,
cpu, soc, system, none.
v3 - Invert soc and sys reset to 'no' to preserve backwards
compatibility. SOC and SYS reset will be set by default without any optional parameters set
v2 - Add 'aspeed,' prefix to all optional properties
- Add arm-reset, soc-reset, interrupt, alt-boot properties

---
 .../devicetree/bindings/watchdog/aspeed-wdt.txt    | 32 ++++++++++++++++++++++
 1 file changed, 32 insertions(+)
Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt +28 -0
index c5e74d7..2b34ce9 100644
@@ -8,9 +8,41 @@ Required properties:
  - reg: physical base address of the controller and length of memory mapped
    region
 
+Optional properties:
+
+ - aspeed,reset-type = "cpu|soc|system|none"
+
+   Reset behavior - Whenever a timeout occurs the watchdog can be programmed
+   to generate one of three different, mutually exclusive, types of resets.
+
+   Type "none" can be specified to indicate that no resets are to be done.
+   This is useful in situations where another watchdog engine on chip is
+   to perform the reset.
+
+   If 'aspeed,reset-type=' is not specfied the default is to enable system
+   reset.
+
+   Reset types:
+
+        - cpu: Reset CPU on watchdog timeout
+
+        - soc: Reset 'System on Chip' on watchdog timeout
+
+        - system: Reset system on watchdog timeout
+
+        - none: No reset is performed on timeout. Assumes another watchdog
+                engine is responsible for this.
+
+ - aspeed,external-signal: If property is present then signal is sent to
+			external reset counter (only WDT1 and WDT2). If not
+			specified no external signal is sent.

Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt Required properties: · line 48

Quoted 10 lines
Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt +4 -0
+ - aspeed,alt-boot:    If property is present then boot from alternate block.
+
 Example:
 
 	wdt1: watchdog@1e785000 {
 		compatible = "aspeed,ast2400-wdt";
 		reg = <0x1e785000 0x1c>;
+		aspeed,reset-type = "system";
+		aspeed,external-signal;
 	};

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#6 lore raw

PATCH v5 2/2 drivers/watchdog: ASPEED reference dev tree properties for config

In reply to Christopher Bostic

Reference the system device tree when configuring the watchdog engines. If property 'aspeed,reset_type' is present then set reset behavior based on the specified value. This can be one of three different mutually exclusive values
* cpu - Reset CPU only on watchdog timeout
* soc - Reset System on Chip
* system - Full system reset

No reset can also be specified by indicating:
* none - No reset, assumes another watchdog is responsible for this.

Add optional property 'aspeed,external-signal'. If present then configure to generate external signal on watchdog timeout.

---
v5 - Add explicit check for property type "none". Return error
if property type is not a known value.
- Default reset type when no property present changed to match original code: SOC + SYSTEM reset
v4 - Change the three reset type parameters to a new property
'aspeed,reset_type' and check assignment for one of four different values, cpu, soc, system, none
v3 - Invert the logic for system reset dev tree property to
preserve backwards compatibility. If not specified the default is to configure for system reset
- Add check for 'aspeed,no-soc-reset' property and only if not present is SOC reset to be configured. This preserves backwards compatibility.
v2 - Change of_get_property() to of_property_read_bool()
- Remove redundant check for NULL struct device_node pointer
- Optional property names now start with prefix 'aspeed,'

---
 drivers/watchdog/aspeed_wdt.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)
drivers/watchdog/aspeed_wdt.c +24 -5
index 1c65258..c707ab6 100644
@@ -36,6 +36,7 @@ struct aspeed_wdt {
 #define WDT_CTRL		0x0C
 #define   WDT_CTRL_RESET_MODE_SOC	(0x00 << 5)
 #define   WDT_CTRL_RESET_MODE_FULL_CHIP	(0x01 << 5)
+#define   WDT_CTRL_RESET_MODE_ARM_CPU	(0x10 << 5)
 #define   WDT_CTRL_1MHZ_CLK		BIT(4)
 #define   WDT_CTRL_WDT_EXT		BIT(3)
 #define   WDT_CTRL_WDT_INTR		BIT(2)
@@ -140,6 +141,8 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
 {
 	struct aspeed_wdt *wdt;
 	struct resource *res;
+	struct device_node *np;
+	const char *reset_type;
 	int ret;
 
 	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
@@ -164,14 +167,30 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
 	wdt->wdd.timeout = WDT_DEFAULT_TIMEOUT;
 	watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev);
 
+	wdt->ctrl = WDT_CTRL_1MHZ_CLK;
+
 	/*
 	 * Control reset on a per-device basis to ensure the
-	 * host is not affected by a BMC reboot, so only reset
-	 * the SOC and not the full chip
+	 * host is not affected by a BMC reboot
 	 */
-	wdt->ctrl = WDT_CTRL_RESET_MODE_SOC |
-		WDT_CTRL_1MHZ_CLK |
-		WDT_CTRL_RESET_SYSTEM;
+	np = pdev->dev.of_node;
+	ret = of_property_read_string(np, "aspeed,reset-type", &reset_type);
+	if (ret) {
+		wdt->ctrl |= WDT_CTRL_RESET_MODE_SOC | WDT_CTRL_RESET_SYSTEM;
+	} else {
+		if (!strcmp(reset_type, "cpu"))
+			wdt->ctrl |= WDT_CTRL_RESET_MODE_ARM_CPU;
+		else if (!strcmp(reset_type, "soc"))
+			wdt->ctrl |= WDT_CTRL_RESET_MODE_SOC;
+		else if (!strcmp(reset_type, "system"))
+			wdt->ctrl |= WDT_CTRL_RESET_SYSTEM;
+		else if (strcmp(reset_type, "none"))
+			return -EINVAL;
+	}
+	if (of_property_read_bool(np, "aspeed,external-signal"))
+		wdt->ctrl |= WDT_CTRL_WDT_EXT;
+
+	writel(wdt->ctrl, wdt->base + WDT_CTRL);
 
 	if (readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE)  {
 		aspeed_wdt_start(&wdt->wdd);
Signature · 1 line
1.8.2.2
maintainer Reviewed-by on v5 2/2
#7 lore raw

Re: [v5, 2/2] drivers/watchdog: ASPEED reference dev tree properties for config

Quoted 8 lines · Christopher Bostic

Reference the system device tree when configuring the watchdog engines. If property 'aspeed,reset_type' is present then set reset behavior based on the specified value. This can be one of three different mutually exclusive values
* cpu - Reset CPU only on watchdog timeout
* soc - Reset System on Chip
* system - Full system reset

Quoted 8 lines

No reset can also be specified by indicating:
* none - No reset, assumes another watchdog is responsible for this.

Add optional property 'aspeed,external-signal'. If present then configure to generate external signal on watchdog timeout.

Quoted 78 lines

---
v5 - Add explicit check for property type "none". Return error
if property type is not a known value.
- Default reset type when no property present changed to match original code: SOC + SYSTEM reset
v4 - Change the three reset type parameters to a new property
'aspeed,reset_type' and check assignment for one of four different values, cpu, soc, system, none
v3 - Invert the logic for system reset dev tree property to
preserve backwards compatibility. If not specified the default is to configure for system reset
- Add check for 'aspeed,no-soc-reset' property and only if not present is SOC reset to be configured. This preserves backwards compatibility.
v2 - Change of_get_property() to of_property_read_bool()
- Remove redundant check for NULL struct device_node pointer
- Optional property names now start with prefix 'aspeed,'

---
 drivers/watchdog/aspeed_wdt.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)
drivers/watchdog/aspeed_wdt.c +24 -5
index 1c65258..c707ab6 100644
@@ -36,6 +36,7 @@ struct aspeed_wdt {
 #define WDT_CTRL		0x0C
 #define   WDT_CTRL_RESET_MODE_SOC	(0x00 << 5)
 #define   WDT_CTRL_RESET_MODE_FULL_CHIP	(0x01 << 5)
+#define   WDT_CTRL_RESET_MODE_ARM_CPU	(0x10 << 5)
 #define   WDT_CTRL_1MHZ_CLK		BIT(4)
 #define   WDT_CTRL_WDT_EXT		BIT(3)
 #define   WDT_CTRL_WDT_INTR		BIT(2)
@@ -140,6 +141,8 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
 {
 	struct aspeed_wdt *wdt;
 	struct resource *res;
+	struct device_node *np;
+	const char *reset_type;
 	int ret;
 
 	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
@@ -164,14 +167,30 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
 	wdt->wdd.timeout = WDT_DEFAULT_TIMEOUT;
 	watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev);
 
+	wdt->ctrl = WDT_CTRL_1MHZ_CLK;
+
 	/*
 	 * Control reset on a per-device basis to ensure the
-	 * host is not affected by a BMC reboot, so only reset
-	 * the SOC and not the full chip
+	 * host is not affected by a BMC reboot
 	 */
-	wdt->ctrl = WDT_CTRL_RESET_MODE_SOC |
-		WDT_CTRL_1MHZ_CLK |
-		WDT_CTRL_RESET_SYSTEM;
+	np = pdev->dev.of_node;
+	ret = of_property_read_string(np, "aspeed,reset-type", &reset_type);
+	if (ret) {
+		wdt->ctrl |= WDT_CTRL_RESET_MODE_SOC | WDT_CTRL_RESET_SYSTEM;
+	} else {
+		if (!strcmp(reset_type, "cpu"))
+			wdt->ctrl |= WDT_CTRL_RESET_MODE_ARM_CPU;
+		else if (!strcmp(reset_type, "soc"))
+			wdt->ctrl |= WDT_CTRL_RESET_MODE_SOC;
+		else if (!strcmp(reset_type, "system"))
+			wdt->ctrl |= WDT_CTRL_RESET_SYSTEM;
+		else if (strcmp(reset_type, "none"))
+			return -EINVAL;
+	}
+	if (of_property_read_bool(np, "aspeed,external-signal"))
+		wdt->ctrl |= WDT_CTRL_WDT_EXT;
+
+	writel(wdt->ctrl, wdt->base + WDT_CTRL);
 
 	if (readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE)  {
 		aspeed_wdt_start(&wdt->wdd);