[PATCH 0/2] aspeed: watchdog: fix system reset and read alt-boot option

[PATCH 0/2] aspeed: watchdog: fix system reset and read alt-boot option

[Eddie James]

This series provides a fix to correctly set the reset mode of the control
register. Previously, configuring anything other than a full chip reset would
not reset anything when the watchdog timer expires. The series also provides a
patch to read in the existing aspeed,alt-boot boolean option from the
device-tree and set the appropriate bit in the control register.

Milton Miller (2):
  watchdog: aspeed: Fix translation of reset mode to ctrl register
  watchdog: aspeed: Allow configuring for alternate boot

 drivers/watchdog/aspeed_wdt.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

-- 
1.8.3.1

[PATCH 1/2] watchdog: aspeed: Fix translation of reset mode to ctrl register

[Eddie James]

From: Milton Miller <miltonm@us.ibm.com>

Assert RESET_SYSTEM bit for any reset and set MODE field from reset
type.

The watchdog control register has a RESET_SYSTEM bit that is really
closer to activate a reset, and RESET_SYSTEM_MODE field that chooses
how much to reset.

Before this patch, a node without these optional property would do a
SOC reset, but a node with properties requesting a cpu or SOC reset
would do nothing and a node requesting a system reset would do a
SOC reset.

Fixes: b7f0b8ad25f3 ("drivers/watchdog: ASPEED reference dev tree properties for config")
Signed-off-by: Milton Miller <miltonm@us.ibm.com>
Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
---
 drivers/watchdog/aspeed_wdt.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
index ca5b91e..d1987d6 100644
--- a/drivers/watchdog/aspeed_wdt.c
+++ b/drivers/watchdog/aspeed_wdt.c
@@ -232,11 +232,14 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
 		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;
+			wdt->ctrl |= WDT_CTRL_RESET_MODE_ARM_CPU |
+				     WDT_CTRL_RESET_SYSTEM;
 		else if (!strcmp(reset_type, "soc"))
-			wdt->ctrl |= WDT_CTRL_RESET_MODE_SOC;
+			wdt->ctrl |= WDT_CTRL_RESET_MODE_SOC |
+				     WDT_CTRL_RESET_SYSTEM;
 		else if (!strcmp(reset_type, "system"))
-			wdt->ctrl |= WDT_CTRL_RESET_SYSTEM;
+			wdt->ctrl |= WDT_CTRL_RESET_MODE_FULL_CHIP |
+				     WDT_CTRL_RESET_SYSTEM;
 		else if (strcmp(reset_type, "none"))
 			return -EINVAL;
 	}
-- 
1.8.3.1

[PATCH 2/2] watchdog: aspeed: Allow configuring for alternate boot

[Eddie James]

From: Milton Miller <miltonm@us.ibm.com>

Allow the device tree to specify a watchdog to fallover to
the alternate boot source.

The aspeeed watchdog can set a latch directing flash chip select 0 to
chip select 1, allowing boot from an alternate media if the watchdog
is not reset in time.  On the ast2400 bank 1 also goes to flash bank 1,
while on the ast2500 the chip selects are swapped.

Signed-off-by: Milton Miller <miltonm@us.ibm.com>
Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
---
 drivers/watchdog/aspeed_wdt.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
index d1987d6..f41d246 100644
--- a/drivers/watchdog/aspeed_wdt.c
+++ b/drivers/watchdog/aspeed_wdt.c
@@ -46,6 +46,7 @@ struct aspeed_wdt_config {
 #define WDT_RELOAD_VALUE	0x04
 #define WDT_RESTART		0x08
 #define WDT_CTRL		0x0C
+#define   WDT_CTRL_BOOT_SECONDARY	BIT(7)
 #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)
@@ -245,6 +246,8 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
 	}
 	if (of_property_read_bool(np, "aspeed,external-signal"))
 		wdt->ctrl |= WDT_CTRL_WDT_EXT;
+	if (of_property_read_bool(np, "aspeed,alt-boot"))
+		wdt->ctrl |= WDT_CTRL_BOOT_SECONDARY;
 
 	if (readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE)  {
 		/*
-- 
1.8.3.1

Re: [PATCH 1/2] watchdog: aspeed: Fix translation of reset mode to ctrl register

[Guenter Roeck]

On Fri, Mar 09, 2018 at 03:58:19PM -0600, Eddie James wrote:
> From: Milton Miller <miltonm@us.ibm.com>
> 
> Assert RESET_SYSTEM bit for any reset and set MODE field from reset
> type.
> 
> The watchdog control register has a RESET_SYSTEM bit that is really
> closer to activate a reset, and RESET_SYSTEM_MODE field that chooses
> how much to reset.
> 
> Before this patch, a node without these optional property would do a
> SOC reset, but a node with properties requesting a cpu or SOC reset
> would do nothing and a node requesting a system reset would do a
> SOC reset.
> 
> Fixes: b7f0b8ad25f3 ("drivers/watchdog: ASPEED reference dev tree properties for config")
> Signed-off-by: Milton Miller <miltonm@us.ibm.com>
> Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  drivers/watchdog/aspeed_wdt.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> index ca5b91e..d1987d6 100644
> --- a/drivers/watchdog/aspeed_wdt.c
> +++ b/drivers/watchdog/aspeed_wdt.c
> @@ -232,11 +232,14 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
>  		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;
> +			wdt->ctrl |= WDT_CTRL_RESET_MODE_ARM_CPU |
> +				     WDT_CTRL_RESET_SYSTEM;
>  		else if (!strcmp(reset_type, "soc"))
> -			wdt->ctrl |= WDT_CTRL_RESET_MODE_SOC;
> +			wdt->ctrl |= WDT_CTRL_RESET_MODE_SOC |
> +				     WDT_CTRL_RESET_SYSTEM;
>  		else if (!strcmp(reset_type, "system"))
> -			wdt->ctrl |= WDT_CTRL_RESET_SYSTEM;
> +			wdt->ctrl |= WDT_CTRL_RESET_MODE_FULL_CHIP |
> +				     WDT_CTRL_RESET_SYSTEM;
>  		else if (strcmp(reset_type, "none"))
>  			return -EINVAL;
>  	}
> -- 
> 1.8.3.1
> 

Re: [PATCH 2/2] watchdog: aspeed: Allow configuring for alternate boot

[Guenter Roeck]

On Fri, Mar 09, 2018 at 03:58:20PM -0600, Eddie James wrote:
> From: Milton Miller <miltonm@us.ibm.com>
> 
> Allow the device tree to specify a watchdog to fallover to
> the alternate boot source.
> 
> The aspeeed watchdog can set a latch directing flash chip select 0 to
> chip select 1, allowing boot from an alternate media if the watchdog
> is not reset in time.  On the ast2400 bank 1 also goes to flash bank 1,
> while on the ast2500 the chip selects are swapped.
> 
> Signed-off-by: Milton Miller <miltonm@us.ibm.com>
> Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>

This is already documented in the bindings document, so the property
should be ok.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  drivers/watchdog/aspeed_wdt.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> index d1987d6..f41d246 100644
> --- a/drivers/watchdog/aspeed_wdt.c
> +++ b/drivers/watchdog/aspeed_wdt.c
> @@ -46,6 +46,7 @@ struct aspeed_wdt_config {
>  #define WDT_RELOAD_VALUE	0x04
>  #define WDT_RESTART		0x08
>  #define WDT_CTRL		0x0C
> +#define   WDT_CTRL_BOOT_SECONDARY	BIT(7)
>  #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)
> @@ -245,6 +246,8 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
>  	}
>  	if (of_property_read_bool(np, "aspeed,external-signal"))
>  		wdt->ctrl |= WDT_CTRL_WDT_EXT;
> +	if (of_property_read_bool(np, "aspeed,alt-boot"))
> +		wdt->ctrl |= WDT_CTRL_BOOT_SECONDARY;
>  
>  	if (readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE)  {
>  		/*
> -- 
> 1.8.3.1
> 

Re: [PATCH 2/2] watchdog: aspeed: Allow configuring for alternate boot

[Joel Stanley]

On Sat, Mar 10, 2018 at 8:28 AM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
> From: Milton Miller <miltonm@us.ibm.com>
>
> Allow the device tree to specify a watchdog to fallover to
> the alternate boot source.
>
> The aspeeed watchdog can set a latch directing flash chip select 0 to
> chip select 1, allowing boot from an alternate media if the watchdog
> is not reset in time.  On the ast2400 bank 1 also goes to flash bank 1,
> while on the ast2500 the chip selects are swapped.
>
> Signed-off-by: Milton Miller <miltonm@us.ibm.com>
> Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
> ---
>  drivers/watchdog/aspeed_wdt.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> index d1987d6..f41d246 100644
> --- a/drivers/watchdog/aspeed_wdt.c
> +++ b/drivers/watchdog/aspeed_wdt.c
> @@ -46,6 +46,7 @@ struct aspeed_wdt_config {
>  #define WDT_RELOAD_VALUE       0x04
>  #define WDT_RESTART            0x08
>  #define WDT_CTRL               0x0C
> +#define   WDT_CTRL_BOOT_SECONDARY      BIT(7)
>  #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)
> @@ -245,6 +246,8 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
>         }
>         if (of_property_read_bool(np, "aspeed,external-signal"))
>                 wdt->ctrl |= WDT_CTRL_WDT_EXT;
> +       if (of_property_read_bool(np, "aspeed,alt-boot"))
> +               wdt->ctrl |= WDT_CTRL_BOOT_SECONDARY;

If a user sets this property on the only watchdog in the system, then
they will trigger this behaviour when doing a normal 'reboot'. That
would not be desirable.

We could mitigate this by:

- not registering the watchdog as a reboot device if this property is enabled

- clearing the WDT_CTRL_BOOT_SECONDARY bit in the aspeed_wdt_restart path

The second option is neater as it allows the watchdog to behave
normally when only one is enabled. The first would be confusing if a
system didn't have more than one watchdog enabled.

Cheers,

Joel

Re: [PATCH 2/2] watchdog: aspeed: Allow configuring for alternate boot

[Eddie James]

On 03/09/2018 04:06 PM, Guenter Roeck wrote:
> On Fri, Mar 09, 2018 at 03:58:20PM -0600, Eddie James wrote:
>> From: Milton Miller <miltonm@us.ibm.com>
>>
>> Allow the device tree to specify a watchdog to fallover to
>> the alternate boot source.
>>
>> The aspeeed watchdog can set a latch directing flash chip select 0 to
>> chip select 1, allowing boot from an alternate media if the watchdog
>> is not reset in time.  On the ast2400 bank 1 also goes to flash bank 1,
>> while on the ast2500 the chip selects are swapped.
>>
>> Signed-off-by: Milton Miller <miltonm@us.ibm.com>
>> Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
> This is already documented in the bindings document, so the property
> should be ok.
>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Thanks. On a related note, our system has need of a way to determine if 
it's currently booted from the alternate chip or not. It seems the only 
way to tell is to look at the ast2400/2500 watchdog timeout status 
register. We can add some debugfs to the aspeed driver I guess, but I 
wonder if this is a common enough situation that the watchdog core could 
use some debugfs or sysfs for providing that info?

Thanks,
Eddie

>
>> ---
>>   drivers/watchdog/aspeed_wdt.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
>> index d1987d6..f41d246 100644
>> --- a/drivers/watchdog/aspeed_wdt.c
>> +++ b/drivers/watchdog/aspeed_wdt.c
>> @@ -46,6 +46,7 @@ struct aspeed_wdt_config {
>>   #define WDT_RELOAD_VALUE	0x04
>>   #define WDT_RESTART		0x08
>>   #define WDT_CTRL		0x0C
>> +#define   WDT_CTRL_BOOT_SECONDARY	BIT(7)
>>   #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)
>> @@ -245,6 +246,8 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
>>   	}
>>   	if (of_property_read_bool(np, "aspeed,external-signal"))
>>   		wdt->ctrl |= WDT_CTRL_WDT_EXT;
>> +	if (of_property_read_bool(np, "aspeed,alt-boot"))
>> +		wdt->ctrl |= WDT_CTRL_BOOT_SECONDARY;
>>   
>>   	if (readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE)  {
>>   		/*
>> -- 
>> 1.8.3.1
>>

Re: [PATCH 2/2] watchdog: aspeed: Allow configuring for alternate boot

[Guenter Roeck]

On Wed, Mar 14, 2018 at 02:53:13PM -0500, Eddie James wrote:
> 
> 
> On 03/09/2018 04:06 PM, Guenter Roeck wrote:
> >On Fri, Mar 09, 2018 at 03:58:20PM -0600, Eddie James wrote:
> >>From: Milton Miller <miltonm@us.ibm.com>
> >>
> >>Allow the device tree to specify a watchdog to fallover to
> >>the alternate boot source.
> >>
> >>The aspeeed watchdog can set a latch directing flash chip select 0 to
> >>chip select 1, allowing boot from an alternate media if the watchdog
> >>is not reset in time.  On the ast2400 bank 1 also goes to flash bank 1,
> >>while on the ast2500 the chip selects are swapped.
> >>
> >>Signed-off-by: Milton Miller <miltonm@us.ibm.com>
> >>Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
> >This is already documented in the bindings document, so the property
> >should be ok.
> >
> >Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> 
> Thanks. On a related note, our system has need of a way to determine if it's
> currently booted from the alternate chip or not. It seems the only way to
> tell is to look at the ast2400/2500 watchdog timeout status register. We can
> add some debugfs to the aspeed driver I guess, but I wonder if this is a
> common enough situation that the watchdog core could use some debugfs or
> sysfs for providing that info?
> 
If you have the means to determine that the watchdog timed out in the previous
boot, can you just set the WDIOF_CARDRESET status bit ?

Guenter

> Thanks,
> Eddie
> 
> >
> >>---
> >>  drivers/watchdog/aspeed_wdt.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >>diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> >>index d1987d6..f41d246 100644
> >>--- a/drivers/watchdog/aspeed_wdt.c
> >>+++ b/drivers/watchdog/aspeed_wdt.c
> >>@@ -46,6 +46,7 @@ struct aspeed_wdt_config {
> >>  #define WDT_RELOAD_VALUE	0x04
> >>  #define WDT_RESTART		0x08
> >>  #define WDT_CTRL		0x0C
> >>+#define   WDT_CTRL_BOOT_SECONDARY	BIT(7)
> >>  #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)
> >>@@ -245,6 +246,8 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
> >>  	}
> >>  	if (of_property_read_bool(np, "aspeed,external-signal"))
> >>  		wdt->ctrl |= WDT_CTRL_WDT_EXT;
> >>+	if (of_property_read_bool(np, "aspeed,alt-boot"))
> >>+		wdt->ctrl |= WDT_CTRL_BOOT_SECONDARY;
> >>  	if (readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE)  {
> >>  		/*
> >>-- 
> >>1.8.3.1
> >>
>