[PATCH 0/2] drivers/watchdog ASPEED: Add optional dev tree configs

[PATCH 0/2] drivers/watchdog ASPEED: Add optional dev tree configs

[Christopher Bostic]

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

-- 
1.8.2.2

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

[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 <cbostic@linux.vnet.ibm.com>
---
 Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
index c5e74d7..4099ea5 100644
--- a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
@@ -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;
 	};
-- 
1.8.2.2

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

[Guenter Roeck]

On Tue, Jun 13, 2017 at 03:38:17PM -0500, Christopher Bostic wrote:
> 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 <cbostic@linux.vnet.ibm.com>

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

> ---
>  Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
> index c5e74d7..4099ea5 100644
> --- a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
> @@ -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;
>  	};

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

[Christopher Bostic]

On 6/24/17 5:07 PM, Guenter Roeck wrote:
> On Tue, Jun 13, 2017 at 03:38:17PM -0500, Christopher Bostic wrote:
>> 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 <cbostic@linux.vnet.ibm.com>
> 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

>> ---
>>   Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
>> index c5e74d7..4099ea5 100644
>> --- a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
>> +++ b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
>> @@ -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;
>>   	};

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

[Joel Stanley]

On Wed, Jun 14, 2017 at 6:08 AM, Christopher Bostic
<cbostic@linux.vnet.ibm.com> wrote:
> 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 <cbostic@linux.vnet.ibm.com>
> ---
>  Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
> index c5e74d7..4099ea5 100644
> --- a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
> @@ -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.

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


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

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

[Christopher Bostic]

On 6/26/17 9:59 PM, Joel Stanley wrote:
> On Wed, Jun 14, 2017 at 6:08 AM, Christopher Bostic
> <cbostic@linux.vnet.ibm.com> wrote:
>> 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 <cbostic@linux.vnet.ibm.com>
>> ---
>>   Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
>> index c5e74d7..4099ea5 100644
>> --- a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
>> +++ b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
>> @@ -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.
> 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.

I'll add the modes you describe.

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

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

[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 <cbostic@linux.vnet.ibm.com>
---
 drivers/watchdog/aspeed_wdt.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
index 1c65258..9deb4be 100644
--- a/drivers/watchdog/aspeed_wdt.c
+++ b/drivers/watchdog/aspeed_wdt.c
@@ -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);
-- 
1.8.2.2

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

[Guenter Roeck]

On Tue, Jun 13, 2017 at 03:38:18PM -0500, Christopher Bostic wrote:
> 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 <cbostic@linux.vnet.ibm.com>
> ---
>  drivers/watchdog/aspeed_wdt.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> index 1c65258..9deb4be 100644
> --- a/drivers/watchdog/aspeed_wdt.c
> +++ b/drivers/watchdog/aspeed_wdt.c
> @@ -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;
> +
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.

> +	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);

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

[Christopher Bostic]

On 6/24/17 5:17 PM, Guenter Roeck wrote:
> On Tue, Jun 13, 2017 at 03:38:18PM -0500, Christopher Bostic wrote:
>> 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 <cbostic@linux.vnet.ibm.com>
>> ---
>>   drivers/watchdog/aspeed_wdt.c | 13 +++++++++++--
>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
>> index 1c65258..9deb4be 100644
>> --- a/drivers/watchdog/aspeed_wdt.c
>> +++ b/drivers/watchdog/aspeed_wdt.c
>> @@ -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;
>> +
> 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

> The new properties should probably start with "aspeed," but I'll leave that
> up to DT maintainers to decide.
>
>> +	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);