drivers/watchdog ASPEED: Add optional dev tree configs

PATCH Superseded Christopher Bostic 3 participants 9 messages Jun 13, 2017 – Jun 27, 2017

Subsystems (3)

Device tree bindings Watchdog device drivers Arm/Aspeed machine support

lore

Share & tools

lore

Christopher Bostic

author

2017-06-13 20:38 UTC #0 lore raw

cover drivers/watchdog ASPEED: Add optional dev tree configs

Add optional device tree attribute 'external-signal'. Configure watchdog to drive external signal on timeout if this attribute is present.

Add optional device tree attribute 'no-system-reset'. Configure watchdog to not reset system on timeout if this attribute is present. This mode is used when one of the other watchdogs in system is to be responsible for managing system reset.

Christopher Bostic (2):
drivers/watchdog: Document new aspeed optional dev tree properties. drivers/watchdog: ASPEED reference dev tree properties for config

 Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt | 11 +++++++++++
 drivers/watchdog/aspeed_wdt.c                             | 13 +++++++++++--
 2 files changed, 22 insertions(+), 2 deletions(-)

Signature · 1 line

1.8.2.2

Christopher Bostic

author

2017-06-13 20:38 UTC #1 lore raw

PATCH 1/2 drivers/watchdog: Document new aspeed optional dev tree properties.

Describe new optional property 'external-signal'. When present in the system device tree an exernal signal is generated on watchdog timeout.

Describe new optional property 'no-system-reset'. When present in the system device tree no system reset is to occur on watchdog timeout. System reset in this case is managed by one of the other watchogs available.

Signed-off-by Christopher Bostic <[email protected]>

---
 Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt | 11 +++++++++++
 1 file changed, 11 insertions(+)

Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt +11 -0

index c5e74d7..4099ea5 100644
@@ -8,9 +8,20 @@ Required properties:
  - reg: physical base address of the controller and length of memory mapped
    region
 
+Optional properties:
+ - external-signal: If the property is present then an external signal is to
+   be generated on watchdog timeout.  If absent, external signal is not
+   generated.
+
+ - no-system-reset: If the property is present then system will not be reset
+   on watchdog timeout. In this case one of the other watchdogs will handle
+   reset.  If absent then the watchdog resets the system on timeout.
+
 Example:
 
 	wdt1: watchdog@1e785000 {
 		compatible = "aspeed,ast2400-wdt";
 		reg = <0x1e785000 0x1c>;
+		external-signal;
+		no-system-reset;
 	};

Signature · 1 line

1.8.2.2

Guenter Roeck

maintainer

2017-06-24 22:07 UTC #2 lore raw

Re: [1/2] drivers/watchdog: Document new aspeed optional dev tree properties.

Quoted 9 lines · Christopher Bostic

Describe new optional property 'external-signal'. When present in the system device tree an exernal signal is generated on watchdog timeout.
Describe new optional property 'no-system-reset'. When present in the system device tree no system reset is to occur on watchdog timeout. System reset in this case is managed by one of the other watchogs available.
Signed-off-by Christopher Bostic <[email protected]>

This requires DT maintainer approval, but DT maintainers were not copied. Please resend and use scripts/get_maintainer.pl to determine who to send it to.

Guenter

Quoted 29 lines

---
 Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt | 11 +++++++++++
 1 file changed, 11 insertions(+)

Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt +11 -0

index c5e74d7..4099ea5 100644
@@ -8,9 +8,20 @@ Required properties:
  - reg: physical base address of the controller and length of memory mapped
    region
 
+Optional properties:
+ - external-signal: If the property is present then an external signal is to
+   be generated on watchdog timeout.  If absent, external signal is not
+   generated.
+
+ - no-system-reset: If the property is present then system will not be reset
+   on watchdog timeout. In this case one of the other watchdogs will handle
+   reset.  If absent then the watchdog resets the system on timeout.
+
 Example:
 
 	wdt1: watchdog@1e785000 {
 		compatible = "aspeed,ast2400-wdt";
 		reg = <0x1e785000 0x1c>;
+		external-signal;
+		no-system-reset;
 	};

Christopher Bostic

author

2017-06-27 02:16 UTC #3 lore raw

Re: [1/2] drivers/watchdog: Document new aspeed optional dev tree properties.

Quoted 9 lines · Guenter Roeck

Quoted 8 lines · Christopher Bostic
Describe new optional property 'external-signal'. When present in the system device tree an exernal signal is generated on watchdog timeout.
Describe new optional property 'no-system-reset'. When present in the system device tree no system reset is to occur on watchdog timeout. System reset in this case is managed by one of the other watchogs available.

Quoted 6 lines

Quoted 1 line
Signed-off-by Christopher Bostic <[email protected]>
This requires DT maintainer approval, but DT maintainers were not copied. Please resend and use scripts/get_maintainer.pl to determine who to send it to.
Guenter

Hi Guenter,

Will resend with maintainers copied.

Thanks
-Chris

Quoted 29 lines

---
  Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt | 11 +++++++++++
  1 file changed, 11 insertions(+)

Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt +11 -0

index c5e74d7..4099ea5 100644
@@ -8,9 +8,20 @@ Required properties:
   - reg: physical base address of the controller and length of memory mapped
     region
 
+Optional properties:
+ - external-signal: If the property is present then an external signal is to
+   be generated on watchdog timeout.  If absent, external signal is not
+   generated.
+
+ - no-system-reset: If the property is present then system will not be reset
+   on watchdog timeout. In this case one of the other watchdogs will handle
+   reset.  If absent then the watchdog resets the system on timeout.
+
  Example:
 
  	wdt1: watchdog@1e785000 {
  		compatible = "aspeed,ast2400-wdt";
  		reg = <0x1e785000 0x1c>;
+		external-signal;
+		no-system-reset;
  	};

Joel Stanley

maintainer

2017-06-27 03:00 UTC #4 lore raw

Re: [PATCH 1/2] drivers/watchdog: Document new aspeed optional dev tree properties.

In reply to Christopher Bostic

Quoted 19 lines · Christopher Bostic

Describe new optional property 'external-signal'. When present in the system device tree an exernal signal is generated on watchdog timeout.
Describe new optional property 'no-system-reset'. When present in the system device tree no system reset is to occur on watchdog timeout. System reset in this case is managed by one of the other watchogs available.
Signed-off-by Christopher Bostic <[email protected]>
---
 Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt | 11 +++++++++++
 1 file changed, 11 insertions(+)
Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt +0 -0
index c5e74d7..4099ea5 100644
@@ -8,9 +8,20 @@ Required properties:
  - reg: physical base address of the controller and length of memory mapped

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

Quoted 10 lines

Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt +8 -0

    region
 
+Optional properties:
+ - external-signal: If the property is present then an external signal is to
+   be generated on watchdog timeout.  If absent, external signal is not
+   generated.
+
+ - no-system-reset: If the property is present then system will not be reset
+   on watchdog timeout. In this case one of the other watchdogs will handle
+   reset.  If absent then the watchdog resets the system on timeout.

I'm not sure that this describes the hardware. The datasheet says:

Whenever timeout ocurs, WDT can program to generate 6 types of signals:

* ARM reset signal: to reset ARM CPU only
* SOC reset signal: to reset SOC part function
* System reset signal: to reset full chip
* Interrupt signal: to interrupt CPU
* Eternal signal: to external reset counter (only WDT1 and WDT2)
* Alternate boot signal: to boot from alternate block

I think your bindings should describe these modes where possible.

Cheers,

Joel

Quoted 12 lines

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

+
 Example:
 
        wdt1: watchdog@1e785000 {
                compatible = "aspeed,ast2400-wdt";
                reg = <0x1e785000 0x1c>;
+               external-signal;
+               no-system-reset;
        };
--

1.8.2.2

Christopher Bostic

author

2017-06-27 19:44 UTC #5 lore raw

Re: [PATCH 1/2] drivers/watchdog: Document new aspeed optional dev tree properties.

Quoted 35 lines · Joel Stanley

Quoted 19 lines · Christopher Bostic
Describe new optional property 'external-signal'. When present in the system device tree an exernal signal is generated on watchdog timeout.
Describe new optional property 'no-system-reset'. When present in the system device tree no system reset is to occur on watchdog timeout. System reset in this case is managed by one of the other watchogs available.
Signed-off-by Christopher Bostic <[email protected]>
---
  Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt | 11 +++++++++++
  1 file changed, 11 insertions(+)
Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt +0 -0
index c5e74d7..4099ea5 100644
@@ -8,9 +8,20 @@ Required properties:
   - reg: physical base address of the controller and length of memory mapped
Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt Required properties: · line 18
Quoted 10 lines
Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt +8 -0
     region
 
+Optional properties:
+ - external-signal: If the property is present then an external signal is to
+   be generated on watchdog timeout.  If absent, external signal is not
+   generated.
+
+ - no-system-reset: If the property is present then system will not be reset
+   on watchdog timeout. In this case one of the other watchdogs will handle
+   reset.  If absent then the watchdog resets the system on timeout.
I'm not sure that this describes the hardware. The datasheet says:
Whenever timeout ocurs, WDT can program to generate 6 types of signals:

Quoted 8 lines

* ARM reset signal: to reset ARM CPU only
* SOC reset signal: to reset SOC part function
* System reset signal: to reset full chip
* Interrupt signal: to interrupt CPU
* Eternal signal: to external reset counter (only WDT1 and WDT2)
* Alternate boot signal: to boot from alternate block
I think your bindings should describe these modes where possible.

I'll add the modes you describe.

Thanks,
Chris

Quoted 18 lines

Cheers,

Joel

Quoted 12 lines

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

+
  Example:
 
         wdt1: watchdog@1e785000 {
                 compatible = "aspeed,ast2400-wdt";
                 reg = <0x1e785000 0x1c>;
+               external-signal;
+               no-system-reset;
         };
--

1.8.2.2

Christopher Bostic

author

2017-06-13 20:39 UTC #6 lore raw

PATCH 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. Configure for external signal generation if optional attribute 'external-signal' is present in device tree. Configure for reset system after timeout if the optional attribute 'no-system-reset' is not present. Disabling system reset may be required in situations where one of the other watchdogs in the systems is responsible for this.

Signed-off-by Christopher Bostic <[email protected]>

---
 drivers/watchdog/aspeed_wdt.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

drivers/watchdog/aspeed_wdt.c +11 -2

index 1c65258..9deb4be 100644
@@ -140,6 +140,7 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
 {
 	struct aspeed_wdt *wdt;
 	struct resource *res;
+	struct device_node *np;
 	int ret;
 
 	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);

@@ -170,8 +171,16 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
 	 * the SOC and not the full chip
 	 */
 	wdt->ctrl = WDT_CTRL_RESET_MODE_SOC |
-		WDT_CTRL_1MHZ_CLK |
-		WDT_CTRL_RESET_SYSTEM;
+		WDT_CTRL_1MHZ_CLK;
+
+	np = pdev->dev.of_node;
+	if (np && !of_get_property(np, "no-system-reset", NULL))
+		wdt->ctrl |= WDT_CTRL_RESET_SYSTEM;
+
+	if (np && of_get_property(np, "external-signal", NULL))
+		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

Guenter Roeck

maintainer

2017-06-24 22:17 UTC #7 lore raw

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

Quoted 27 lines · Christopher Bostic

Reference the system device tree when configuring the watchdog. Configure for external signal generation if optional attribute 'external-signal' is present in device tree. Configure for reset system after timeout if the optional attribute 'no-system-reset' is not present. Disabling system reset may be required in situations where one of the other watchdogs in the systems is responsible for this.
Signed-off-by Christopher Bostic <[email protected]>
---
 drivers/watchdog/aspeed_wdt.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
drivers/watchdog/aspeed_wdt.c +1 -0
index 1c65258..9deb4be 100644
@@ -140,6 +140,7 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
 {
 	struct aspeed_wdt *wdt;
 	struct resource *res;
+	struct device_node *np;
 	int ret;
 
 	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
@@ -170,8 +171,16 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
 	 * the SOC and not the full chip

drivers/watchdog/aspeed_wdt.c static int aspeed_wdt_probe(struct platform_device *pdev) · line 179

Quoted 10 lines

drivers/watchdog/aspeed_wdt.c +6 -2

 	 */
 	wdt->ctrl = WDT_CTRL_RESET_MODE_SOC |
-		WDT_CTRL_1MHZ_CLK |
-		WDT_CTRL_RESET_SYSTEM;
+		WDT_CTRL_1MHZ_CLK;
+
+	np = pdev->dev.of_node;
+	if (np && !of_get_property(np, "no-system-reset", NULL))
+		wdt->ctrl |= WDT_CTRL_RESET_SYSTEM;
+

Please use of_property_read_bool(). Also, checking np for NULL is unnecessary since the called function does that. Note that the above code as written is wrong - if np is NULL, WDT_CTRL_RESET_SYSTEM will no longer be set.

The new properties should probably start with "aspeed," but I'll leave that up to DT maintainers to decide.

Quoted 7 lines

drivers/watchdog/aspeed_wdt.c +4 -0

+	if (np && of_get_property(np, "external-signal", NULL))
+		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);

Christopher Bostic

author

2017-06-27 02:17 UTC #8 lore raw

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

Quoted 25 lines · Guenter Roeck

Quoted 24 lines · Christopher Bostic
Reference the system device tree when configuring the watchdog. Configure for external signal generation if optional attribute 'external-signal' is present in device tree. Configure for reset system after timeout if the optional attribute 'no-system-reset' is not present. Disabling system reset may be required in situations where one of the other watchdogs in the systems is responsible for this.
Signed-off-by Christopher Bostic <[email protected]>
---
  drivers/watchdog/aspeed_wdt.c | 13 +++++++++++--
  1 file changed, 11 insertions(+), 2 deletions(-)
drivers/watchdog/aspeed_wdt.c +1 -0
index 1c65258..9deb4be 100644
@@ -140,6 +140,7 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
  {
  	struct aspeed_wdt *wdt;
  	struct resource *res;
+	struct device_node *np;
  	int ret;
 

Quoted 16 lines

drivers/watchdog/aspeed_wdt.c static int aspeed_wdt_probe(struct platform_device *pdev) · line 179
Quoted 13 lines
drivers/watchdog/aspeed_wdt.c +6 -2
  	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
@@ -170,8 +171,16 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
  	 * the SOC and not the full chip
  	 */
  	wdt->ctrl = WDT_CTRL_RESET_MODE_SOC |
-		WDT_CTRL_1MHZ_CLK |
-		WDT_CTRL_RESET_SYSTEM;
+		WDT_CTRL_1MHZ_CLK;
+
+	np = pdev->dev.of_node;
+	if (np && !of_get_property(np, "no-system-reset", NULL))
+		wdt->ctrl |= WDT_CTRL_RESET_SYSTEM;
+
Please use of_property_read_bool(). Also, checking np for NULL is unnecessary since the called function does that. Note that the above code as written is wrong - if np is NULL, WDT_CTRL_RESET_SYSTEM will no longer be set.

Will make the suggested changes.

Thanks
-Chris

Quoted 10 lines

The new properties should probably start with "aspeed," but I'll leave that up to DT maintainers to decide.
Quoted 7 lines
drivers/watchdog/aspeed_wdt.c +4 -0
+	if (np && of_get_property(np, "external-signal", NULL))
+		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);