2017-07-07 00:49:17

by Christopher Bostic

[permalink] [raw]
Subject: [PATCH v4 0/2] 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 | 35 ++++++++++++++++++++++
drivers/watchdog/aspeed_wdt.c | 27 +++++++++++++----
2 files changed, 57 insertions(+), 5 deletions(-)

--
1.8.2.2


2017-07-07 00:49:25

by Christopher Bostic

[permalink] [raw]
Subject: [PATCH v4 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,interrupt - Interrupt CPU on signal
* aspeed,external-signal - Generate external signal (WDT1 and WDT2 only)
* aspeed,alt-boot - Boot from alternate block on signal

Signed-off-by: Christopher Bostic <[email protected]>
---
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 | 35 ++++++++++++++++++++++
1 file changed, 35 insertions(+)

diff --git a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
index c5e74d7..f526b00 100644
--- a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
@@ -8,9 +8,44 @@ 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,interrupt: If property is present then interrupt CPU.
+ If not specified then don't interrupt CPU.
+
+ - 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;
};
--
1.8.2.2

2017-07-07 00:49:30

by Christopher Bostic

[permalink] [raw]
Subject: [PATCH v4 2/2] drivers/watchdog: ASPEED reference dev tree properties for config

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.

Signed-off-by: Christopher Bostic <[email protected]>
---
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 | 27 ++++++++++++++++++++++-----
1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
index 1c65258..3ca79565 100644
--- a/drivers/watchdog/aspeed_wdt.c
+++ b/drivers/watchdog/aspeed_wdt.c
@@ -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,28 @@ 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_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;
+ }
+ 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);
--
1.8.2.2

2017-07-08 14:55:36

by Guenter Roeck

[permalink] [raw]
Subject: Re: [v4, 2/2] drivers/watchdog: ASPEED reference dev tree properties for config

On Thu, Jul 06, 2017 at 07:49:00PM -0500, Christopher Bostic wrote:
> 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.
>
> Signed-off-by: Christopher Bostic <[email protected]>
> ---
> 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 | 27 ++++++++++++++++++++++-----
> 1 file changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> index 1c65258..3ca79565 100644
> --- a/drivers/watchdog/aspeed_wdt.c
> +++ b/drivers/watchdog/aspeed_wdt.c
> @@ -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,28 @@ 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_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;

This silently accepts 'aspeed,reset-type="junk"' as "none".
I think it would be better to explicitly check for "none".

Also, if I remember the datasheet correctly, bit 1 (WDT_CTRL_RESET_SYSTEM)
enables the reset in general, and bit 5/6 specify the reset type.
With that in mind, I don't think the above will work as expected.
I think it would have to be

WDT_CTRL_RESET_MODE_FULL_CHIP | WDT_CTRL_RESET_SYSTEM
WDT_CTRL_RESET_MODE_SOC | WDT_CTRL_RESET_SYSTEM
WDT_CTRL_RESET_MODE_ARM_CPU | WDT_CTRL_RESET_SYSTEM

To match the original code, the default should be

WDT_CTRL_RESET_MODE_SOC | WDT_CTRL_RESET_SYSTEM

> + }
> + if (of_property_read_bool(np, "aspeed,external-signal"))
> + wdt->ctrl |= WDT_CTRL_WDT_EXT;
> +
No plan to support the other new configuration flags ?

> + writel(wdt->ctrl, wdt->base + WDT_CTRL);
>
> if (readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE) {
> aspeed_wdt_start(&wdt->wdd);

2017-07-08 14:59:13

by Guenter Roeck

[permalink] [raw]
Subject: Re: [v4, 1/2] drivers/watchdog: Add optional ASPEED device tree properties

On Thu, Jul 06, 2017 at 07:48:59PM -0500, Christopher Bostic wrote:
> 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,interrupt - Interrupt CPU on signal

After thinking about that, I wonder if this is necessary. It could be
implied by providing an interrupt to the driver. The driver could then
set the interrupt configuration bit automatically.

[ And the bit by itself doesn't really make sense if the driver doesn't
also register an interrupt handler ]

> * aspeed,external-signal - Generate external signal (WDT1 and WDT2 only)
> * aspeed,alt-boot - Boot from alternate block on signal
>
> Signed-off-by: Christopher Bostic <[email protected]>
> ---
> 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 | 35 ++++++++++++++++++++++
> 1 file changed, 35 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
> index c5e74d7..f526b00 100644
> --- a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
> @@ -8,9 +8,44 @@ 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,interrupt: If property is present then interrupt CPU.
> + If not specified then don't interrupt CPU.
> +
> + - 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;
> };

2017-07-10 15:01:47

by Christopher Bostic

[permalink] [raw]
Subject: Re: [v4, 2/2] drivers/watchdog: ASPEED reference dev tree properties for config



On 7/8/17 9:55 AM, Guenter Roeck wrote:
> On Thu, Jul 06, 2017 at 07:49:00PM -0500, Christopher Bostic wrote:
>> 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.
>>
>> Signed-off-by: Christopher Bostic <[email protected]>
>> ---
>> 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 | 27 ++++++++++++++++++++++-----
>> 1 file changed, 22 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
>> index 1c65258..3ca79565 100644
>> --- a/drivers/watchdog/aspeed_wdt.c
>> +++ b/drivers/watchdog/aspeed_wdt.c
>> @@ -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,28 @@ 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_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;
> This silently accepts 'aspeed,reset-type="junk"' as "none".
> I think it would be better to explicitly check for "none".

Will add that check.
> Also, if I remember the datasheet correctly, bit 1 (WDT_CTRL_RESET_SYSTEM)
> enables the reset in general, and bit 5/6 specify the reset type.
> With that in mind, I don't think the above will work as expected.
> I think it would have to be
>
> WDT_CTRL_RESET_MODE_FULL_CHIP | WDT_CTRL_RESET_SYSTEM
> WDT_CTRL_RESET_MODE_SOC | WDT_CTRL_RESET_SYSTEM
> WDT_CTRL_RESET_MODE_ARM_CPU | WDT_CTRL_RESET_SYSTEM
>
> To match the original code, the default should be
>
> WDT_CTRL_RESET_MODE_SOC | WDT_CTRL_RESET_SYSTEM

Yes, true. Will make the change in line above to match original code.

>> + }
>> + if (of_property_read_bool(np, "aspeed,external-signal"))
>> + wdt->ctrl |= WDT_CTRL_WDT_EXT;
>> +
> No plan to support the other new configuration flags ?

No plan to support the other config flags at this time.

Thanks,
Chris
>
>> + writel(wdt->ctrl, wdt->base + WDT_CTRL);
>>
>> if (readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE) {
>> aspeed_wdt_start(&wdt->wdd);

2017-07-10 15:08:04

by Christopher Bostic

[permalink] [raw]
Subject: Re: [v4, 1/2] drivers/watchdog: Add optional ASPEED device tree properties



On 7/8/17 9:59 AM, Guenter Roeck wrote:
> On Thu, Jul 06, 2017 at 07:48:59PM -0500, Christopher Bostic wrote:
>> 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,interrupt - Interrupt CPU on signal
> After thinking about that, I wonder if this is necessary. It could be
> implied by providing an interrupt to the driver. The driver could then
> set the interrupt configuration bit automatically.
>
> [ And the bit by itself doesn't really make sense if the driver doesn't
> also register an interrupt handler ]

The 'aspeed,interrupt' property was added for the sake of documenting
all potential hardware configurations. There is no plan as of now to
enable this so I'm inclined to just remove this part from the
documentation. When and if this function is ever used the optional
property can be documented at that time.

Thanks,
Chris

>> * aspeed,external-signal - Generate external signal (WDT1 and WDT2 only)
>> * aspeed,alt-boot - Boot from alternate block on signal
>>
>> Signed-off-by: Christopher Bostic <[email protected]>
>> ---
>> 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 | 35 ++++++++++++++++++++++
>> 1 file changed, 35 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
>> index c5e74d7..f526b00 100644
>> --- a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
>> +++ b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
>> @@ -8,9 +8,44 @@ 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,interrupt: If property is present then interrupt CPU.
>> + If not specified then don't interrupt CPU.
>> +
>> + - 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;
>> };

2017-07-10 18:13:31

by Guenter Roeck

[permalink] [raw]
Subject: Re: [v4, 1/2] drivers/watchdog: Add optional ASPEED device tree properties

On Mon, Jul 10, 2017 at 10:07:40AM -0500, Christopher Bostic wrote:
>
>
> On 7/8/17 9:59 AM, Guenter Roeck wrote:
> >On Thu, Jul 06, 2017 at 07:48:59PM -0500, Christopher Bostic wrote:
> >>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,interrupt - Interrupt CPU on signal
> >After thinking about that, I wonder if this is necessary. It could be
> >implied by providing an interrupt to the driver. The driver could then
> >set the interrupt configuration bit automatically.
> >
> >[ And the bit by itself doesn't really make sense if the driver doesn't
> > also register an interrupt handler ]
>
> The 'aspeed,interrupt' property was added for the sake of documenting all
> potential hardware configurations. There is no plan as of now to enable
> this so I'm inclined to just remove this part from the documentation. When
> and if this function is ever used the optional property can be documented at
> that time.
>
Ok.

Thanks,
Guenter

> Thanks,
> Chris
>
> >> * aspeed,external-signal - Generate external signal (WDT1 and WDT2 only)
> >> * aspeed,alt-boot - Boot from alternate block on signal
> >>
> >>Signed-off-by: Christopher Bostic <[email protected]>
> >>---
> >>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 | 35 ++++++++++++++++++++++
> >> 1 file changed, 35 insertions(+)
> >>
> >>diff --git a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
> >>index c5e74d7..f526b00 100644
> >>--- a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
> >>+++ b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
> >>@@ -8,9 +8,44 @@ 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,interrupt: If property is present then interrupt CPU.
> >>+ If not specified then don't interrupt CPU.
> >>+
> >>+ - 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;
> >> };
>