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
From: Milton Miller <[email protected]>
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 <[email protected]>
Signed-off-by: Eddie James <[email protected]>
---
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
From: Milton Miller <[email protected]>
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 <[email protected]>
Signed-off-by: Eddie James <[email protected]>
---
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
On Fri, Mar 09, 2018 at 03:58:20PM -0600, Eddie James wrote:
> From: Milton Miller <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Eddie James <[email protected]>
This is already documented in the bindings document, so the property
should be ok.
Reviewed-by: Guenter Roeck <[email protected]>
> ---
> 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
>
On Fri, Mar 09, 2018 at 03:58:19PM -0600, Eddie James wrote:
> From: Milton Miller <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Eddie James <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
> ---
> 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
>
On Sat, Mar 10, 2018 at 8:28 AM, Eddie James <[email protected]> wrote:
> From: Milton Miller <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Eddie James <[email protected]>
> ---
> 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
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 <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Eddie James <[email protected]>
> This is already documented in the bindings document, so the property
> should be ok.
>
> Reviewed-by: Guenter Roeck <[email protected]>
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
>>
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 <[email protected]>
> >>
> >>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 <[email protected]>
> >>Signed-off-by: Eddie James <[email protected]>
> >This is already documented in the bindings document, so the property
> >should be ok.
> >
> >Reviewed-by: Guenter Roeck <[email protected]>
>
> 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
> >>
>