[PATCH 0/4] watchdog: aspeed: Retain enabled state and move to

[PATCH 0/4] watchdog: aspeed: Retain enabled state and move to

[Andrew Jeffery]

Hello,

We had reports of Aspeed BMC systems entering a reboot loop, each time
attempting and failing to probe some PMBus devices. For whatever reason the
PMBus devices weren't appearing on the I2C bus, and several factors came into
play:

1. i2c-aspeed's transfer timeout is set to 5 seconds
2. The kernel's pmbus core now tests for the presence of the status
   word, then the status byte. Not all devices support the status word,
   therefore on error we fall back to probing the status byte. This leads to
   back-to-back uninterruptible transfers, totalling 10 seconds of delay if the
   device is not present before propagating a probe error back up the call
   chain
3. The BMC watchdogs are enabled by u-boot to catch a kernel hang
4. The hardware's default watchdog counter value equates to a 22 second period
5. The watchdog driver is probed after the I2C subsystem iterates all the
   described devices.

Thus as it stands nearly 50% of the watchdog period can be spent dealing with
one missing PMBus device. Arguably the I2C timeout value is too large, but as
the watchdog driver is not probed until after the I2C busses are iterated, the
work to ping the watchdog cannot even be scheduled to take place between
transfers.

Patch 4 shifts aspeed_wdt to arch_initcall so the watchdog can be pinged as
needed. Patch 1 fixes an oversight that lead to the watchdogs being disabled
until userspace opened the chardev. The remaining two patches are minor fixes
to the Kconfig.

Please review!

Cheers,

Andrew

Andrew Jeffery (4):
  watchdog: aspeed: Retain watchdog enabled state
  watchdog: aspeed: Fix 'Apseed' typo in Kconfig
  watchdog: aspeed: Remove specific reference to AST2400 in Kconfig
  watchdog: aspeed: Move init to arch_initcall

 drivers/watchdog/Kconfig      |  8 +++-----
 drivers/watchdog/aspeed_wdt.c | 16 +++++++++++-----
 2 files changed, 14 insertions(+), 10 deletions(-)

-- 
2.11.0

[PATCH 1/4] watchdog: aspeed: Retain watchdog enabled state

[Andrew Jeffery]

An unintended post-condition of probe() is that the watchdog is
disabled. Rework probe() such that we retain the value of the "enabled"
bit from the control register, and take the appropriate actions with
respect to the watchdog core if so. Otherwise, just configure the
watchdog as directed.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/watchdog/aspeed_wdt.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
index 79cc766cd30f..99bc6fbd8852 100644
--- a/drivers/watchdog/aspeed_wdt.c
+++ b/drivers/watchdog/aspeed_wdt.c
@@ -221,7 +221,8 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
 		return -EINVAL;
 	config = ofdid->data;
 
-	wdt->ctrl = WDT_CTRL_1MHZ_CLK;
+	wdt->ctrl |= readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE;
+	wdt->ctrl |= WDT_CTRL_1MHZ_CLK;
 
 	/*
 	 * Control reset on a per-device basis to ensure the
@@ -243,11 +244,11 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
 	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)  {
+	if (wdt->ctrl & WDT_CTRL_ENABLE) {
 		aspeed_wdt_start(&wdt->wdd);
 		set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
+	} else {
+		writel(wdt->ctrl, wdt->base + WDT_CTRL);
 	}
 
 	if (of_device_is_compatible(np, "aspeed,ast2500-wdt")) {
-- 
2.11.0

[PATCH 2/4] watchdog: aspeed: Fix 'Apseed' typo in Kconfig

[Andrew Jeffery]

Apseed sounds like a good name for a web/mobile start-up incubator, but
isn't a reflection of Aspeed themselves.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/watchdog/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index c722cbfdc7e6..b562d2e03eb9 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -746,7 +746,7 @@ config ASPEED_WATCHDOG
 	select WATCHDOG_CORE
 	help
 	  Say Y here to include support for the watchdog timer
-	  in Apseed BMC SoCs.
+	  in Aspeed BMC SoCs.
 
 	  This driver is required to reboot the SoC.
 
-- 
2.11.0

[PATCH 3/4] watchdog: aspeed: Remove specific reference to AST2400 in Kconfig

[Andrew Jeffery]

The driver also supports the watchdog in the AST25xx series, and
may work on earlier SoCs as well.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/watchdog/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index b562d2e03eb9..a1b92ebe74b6 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -741,7 +741,7 @@ config RENESAS_RZAWDT
 	  Renesas RZ/A SoCs. These watchdogs can be used to reset a system.
 
 config ASPEED_WATCHDOG
-	tristate "Aspeed 2400 watchdog support"
+	tristate "Aspeed BMC watchdog support"
 	depends on ARCH_ASPEED || COMPILE_TEST
 	select WATCHDOG_CORE
 	help
-- 
2.11.0

[PATCH 4/4] watchdog: aspeed: Move init to arch_initcall

[Andrew Jeffery]

Probing at device_initcall time lead to perverse cases where the
watchdog was probed after, say, I2C, which then leaves a potentially
running watchdog at the mercy of I2C device behaviour and bus
conditions.

Load the watchdog driver early to ensure that the kernel is patting it
well before initialising peripherals.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/watchdog/Kconfig      | 6 ++----
 drivers/watchdog/aspeed_wdt.c | 7 ++++++-
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index a1b92ebe74b6..6103185983ed 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -741,8 +741,9 @@ config RENESAS_RZAWDT
 	  Renesas RZ/A SoCs. These watchdogs can be used to reset a system.
 
 config ASPEED_WATCHDOG
-	tristate "Aspeed BMC watchdog support"
+	bool "Aspeed BMC watchdog support"
 	depends on ARCH_ASPEED || COMPILE_TEST
+	default y if ARCH_ASPEED
 	select WATCHDOG_CORE
 	help
 	  Say Y here to include support for the watchdog timer
@@ -750,9 +751,6 @@ config ASPEED_WATCHDOG
 
 	  This driver is required to reboot the SoC.
 
-	  To compile this driver as a module, choose M here: the
-	  module will be called aspeed_wdt.
-
 config ZX2967_WATCHDOG
 	tristate "ZTE zx2967 SoCs watchdog support"
 	depends on ARCH_ZX
diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
index 99bc6fbd8852..679c35abadc4 100644
--- a/drivers/watchdog/aspeed_wdt.c
+++ b/drivers/watchdog/aspeed_wdt.c
@@ -313,7 +313,12 @@ static struct platform_driver aspeed_watchdog_driver = {
 		.of_match_table = of_match_ptr(aspeed_wdt_of_table),
 	},
 };
-module_platform_driver(aspeed_watchdog_driver);
+
+static int __init aspeed_wdt_init(void)
+{
+	return platform_driver_register(&aspeed_watchdog_driver);
+}
+arch_initcall(aspeed_wdt_init);
 
 MODULE_DESCRIPTION("Aspeed Watchdog Driver");
 MODULE_LICENSE("GPL");
-- 
2.11.0

Re: [PATCH 0/4] watchdog: aspeed: Retain enabled state and move to

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 201 bytes --]

On Mon, 2017-09-18 at 15:19 +0930, Andrew Jeffery wrote:
> Hello,
> 

*snip*

Ah, my subject got truncated. It should read:

    watchdog: aspeed: Retain enabled state and move to arch_initcall

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 0/4] watchdog: aspeed: Retain enabled state and move to

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 201 bytes --]

On Mon, 2017-09-18 at 15:19 +0930, Andrew Jeffery wrote:
> Hello,
> 

*snip*

Ah, my subject got truncated. It should read:

    watchdog: aspeed: Retain enabled state and move to arch_initcall

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 1/4] watchdog: aspeed: Retain watchdog enabled state

[Guenter Roeck]

On 09/17/2017 10:49 PM, Andrew Jeffery wrote:
> An unintended post-condition of probe() is that the watchdog is
> disabled. Rework probe() such that we retain the value of the "enabled"
> bit from the control register, and take the appropriate actions with
> respect to the watchdog core if so. Otherwise, just configure the
> watchdog as directed.
> 
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>   drivers/watchdog/aspeed_wdt.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> index 79cc766cd30f..99bc6fbd8852 100644
> --- a/drivers/watchdog/aspeed_wdt.c
> +++ b/drivers/watchdog/aspeed_wdt.c
> @@ -221,7 +221,8 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
>   		return -EINVAL;
>   	config = ofdid->data;
>   
> -	wdt->ctrl = WDT_CTRL_1MHZ_CLK;
> +	wdt->ctrl |= readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE;
> +	wdt->ctrl |= WDT_CTRL_1MHZ_CLK;
>   
>   	/*
>   	 * Control reset on a per-device basis to ensure the
> @@ -243,11 +244,11 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
>   	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)  {
> +	if (wdt->ctrl & WDT_CTRL_ENABLE) {
>   		aspeed_wdt_start(&wdt->wdd);

Why call the start function in this case ?

>   		set_bit(WDOG_HW_RUNNING, &wdt->wdd.status); > +	} else {
> +		writel(wdt->ctrl, wdt->base + WDT_CTRL);
>   	}
>   
>   	if (of_device_is_compatible(np, "aspeed,ast2500-wdt")) {
> 

Re: [PATCH 4/4] watchdog: aspeed: Move init to arch_initcall

[Guenter Roeck]

On 09/17/2017 10:49 PM, Andrew Jeffery wrote:
> Probing at device_initcall time lead to perverse cases where the
> watchdog was probed after, say, I2C, which then leaves a potentially
> running watchdog at the mercy of I2C device behaviour and bus
> conditions.
> 
> Load the watchdog driver early to ensure that the kernel is patting it
> well before initialising peripherals.
> 
But you are doing a bit more. You are making it bool, and you are enabling it
by default. Both isn't needed for the intended goal (arch_initcall is converted
to module_initcall if a driver is built as module). Your change focuses on
and optimizes the case where the watchdog is already running. That may not
always be the case, and there may be systems where the driver is not loaded
on purpose.

Guenter

> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>   drivers/watchdog/Kconfig      | 6 ++----
>   drivers/watchdog/aspeed_wdt.c | 7 ++++++-
>   2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index a1b92ebe74b6..6103185983ed 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -741,8 +741,9 @@ config RENESAS_RZAWDT
>   	  Renesas RZ/A SoCs. These watchdogs can be used to reset a system.
>   
>   config ASPEED_WATCHDOG
> -	tristate "Aspeed BMC watchdog support"
> +	bool "Aspeed BMC watchdog support"
>   	depends on ARCH_ASPEED || COMPILE_TEST
> +	default y if ARCH_ASPEED
>   	select WATCHDOG_CORE
>   	help
>   	  Say Y here to include support for the watchdog timer
> @@ -750,9 +751,6 @@ config ASPEED_WATCHDOG
>   
>   	  This driver is required to reboot the SoC.
>   
> -	  To compile this driver as a module, choose M here: the
> -	  module will be called aspeed_wdt.
> -
>   config ZX2967_WATCHDOG
>   	tristate "ZTE zx2967 SoCs watchdog support"
>   	depends on ARCH_ZX
> diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> index 99bc6fbd8852..679c35abadc4 100644
> --- a/drivers/watchdog/aspeed_wdt.c
> +++ b/drivers/watchdog/aspeed_wdt.c
> @@ -313,7 +313,12 @@ static struct platform_driver aspeed_watchdog_driver = {
>   		.of_match_table = of_match_ptr(aspeed_wdt_of_table),
>   	},
>   };
> -module_platform_driver(aspeed_watchdog_driver);
> +
> +static int __init aspeed_wdt_init(void)
> +{
> +	return platform_driver_register(&aspeed_watchdog_driver);
> +}
> +arch_initcall(aspeed_wdt_init);
>   
>   MODULE_DESCRIPTION("Aspeed Watchdog Driver");
>   MODULE_LICENSE("GPL");
> 

Re: [PATCH 2/4] watchdog: aspeed: Fix 'Apseed' typo in Kconfig

[Guenter Roeck]

On Mon, Sep 18, 2017 at 03:19:03PM +0930, Andrew Jeffery wrote:
> Apseed sounds like a good name for a web/mobile start-up incubator, but
> isn't a reflection of Aspeed themselves.
> 
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>

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

> ---
>  drivers/watchdog/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index c722cbfdc7e6..b562d2e03eb9 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -746,7 +746,7 @@ config ASPEED_WATCHDOG
>  	select WATCHDOG_CORE
>  	help
>  	  Say Y here to include support for the watchdog timer
> -	  in Apseed BMC SoCs.
> +	  in Aspeed BMC SoCs.
>  
>  	  This driver is required to reboot the SoC.
>  
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] watchdog: aspeed: Remove specific reference to AST2400 in Kconfig

[Guenter Roeck]

On Mon, Sep 18, 2017 at 03:19:04PM +0930, Andrew Jeffery wrote:
> The driver also supports the watchdog in the AST25xx series, and
> may work on earlier SoCs as well.
> 
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>

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

> ---
>  drivers/watchdog/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index b562d2e03eb9..a1b92ebe74b6 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -741,7 +741,7 @@ config RENESAS_RZAWDT
>  	  Renesas RZ/A SoCs. These watchdogs can be used to reset a system.
>  
>  config ASPEED_WATCHDOG
> -	tristate "Aspeed 2400 watchdog support"
> +	tristate "Aspeed BMC watchdog support"
>  	depends on ARCH_ASPEED || COMPILE_TEST
>  	select WATCHDOG_CORE
>  	help
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] watchdog: aspeed: Move init to arch_initcall

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 3954 bytes --]

On Mon, 2017-09-18 at 06:32 -0700, Guenter Roeck wrote:
> On 09/17/2017 10:49 PM, Andrew Jeffery wrote:
> > Probing at device_initcall time lead to perverse cases where the
> > watchdog was probed after, say, I2C, which then leaves a potentially
> > running watchdog at the mercy of I2C device behaviour and bus
> > conditions.
> > 
> > Load the watchdog driver early to ensure that the kernel is patting it
> > well before initialising peripherals.
> > 
> 
> But you are doing a bit more. 

True.

> You are making it bool, and you are enabling it
> by default. Both isn't needed for the intended goal (arch_initcall is converted
> to module_initcall if a driver is built as module).

Okay, my understanding on this small point was fuzzy and was something 
I should have chased up. Thanks for the feedback.

>  Your change focuses on
> and optimizes the case where the watchdog is already running. That may not
> always be the case, and there may be systems where the driver is not loaded
> on purpose.

Is this a general comment, or regarding a specific Aspeed BMC-based
system? Currently selecting ARCH_ASPEED will force select WATCHDOG and
ASPEED_WATCHDOG. Thus "default y if ARCH_ASPEED" doesn't actually
change the resulting configuration, but removes the need for the
explicit select in arch/arm/mach-aspeed/Kconfig. I don't know what's
better, but I felt that if we moved from tristate to boolean then
changing the default in the watchdog Kconfig was the right way to go.

The way to reboot the Aspeed SoC is to trip the watchdog. Perhaps
wanting to build without the driver isn't unreasonable, but it would
produce a system that you couldn't reboot. However, that's independent
of whether or not to build as a module so it can remain a tristate. I
think I'll just drop the Kconfig changes and add the exit handler. Sing
out if I should do anything different.

Cheers,

Andrew

> 
> Guenter
> 
> > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> >   drivers/watchdog/Kconfig      | 6 ++----
> >   drivers/watchdog/aspeed_wdt.c | 7 ++++++-
> >   2 files changed, 8 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index a1b92ebe74b6..6103185983ed 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -741,8 +741,9 @@ config RENESAS_RZAWDT
> > > >   	  Renesas RZ/A SoCs. These watchdogs can be used to reset a system.
> >   
> >   config ASPEED_WATCHDOG
> > > > -	tristate "Aspeed BMC watchdog support"
> > > > +	bool "Aspeed BMC watchdog support"
> > > >   	depends on ARCH_ASPEED || COMPILE_TEST
> > > > +	default y if ARCH_ASPEED
> > > >   	select WATCHDOG_CORE
> > > >   	help
> > > >   	  Say Y here to include support for the watchdog timer
> > @@ -750,9 +751,6 @@ config ASPEED_WATCHDOG
> >   
> > > >   	  This driver is required to reboot the SoC.
> >   
> > > > -	  To compile this driver as a module, choose M here: the
> > > > -	  module will be called aspeed_wdt.
> > -
> >   config ZX2967_WATCHDOG
> > > >   	tristate "ZTE zx2967 SoCs watchdog support"
> > > >   	depends on ARCH_ZX
> > diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> > index 99bc6fbd8852..679c35abadc4 100644
> > --- a/drivers/watchdog/aspeed_wdt.c
> > +++ b/drivers/watchdog/aspeed_wdt.c
> > @@ -313,7 +313,12 @@ static struct platform_driver aspeed_watchdog_driver = {
> > > >   		.of_match_table = of_match_ptr(aspeed_wdt_of_table),
> > > >   	},
> >   };
> > -module_platform_driver(aspeed_watchdog_driver);
> > +
> > +static int __init aspeed_wdt_init(void)
> > +{
> > > > +	return platform_driver_register(&aspeed_watchdog_driver);
> > +}
> > +arch_initcall(aspeed_wdt_init);
> >   
> >   MODULE_DESCRIPTION("Aspeed Watchdog Driver");
> >   MODULE_LICENSE("GPL");
> > 
> 
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 4/4] watchdog: aspeed: Move init to arch_initcall

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 3954 bytes --]

On Mon, 2017-09-18 at 06:32 -0700, Guenter Roeck wrote:
> On 09/17/2017 10:49 PM, Andrew Jeffery wrote:
> > Probing at device_initcall time lead to perverse cases where the
> > watchdog was probed after, say, I2C, which then leaves a potentially
> > running watchdog at the mercy of I2C device behaviour and bus
> > conditions.
> > 
> > Load the watchdog driver early to ensure that the kernel is patting it
> > well before initialising peripherals.
> > 
> 
> But you are doing a bit more. 

True.

> You are making it bool, and you are enabling it
> by default. Both isn't needed for the intended goal (arch_initcall is converted
> to module_initcall if a driver is built as module).

Okay, my understanding on this small point was fuzzy and was something 
I should have chased up. Thanks for the feedback.

>  Your change focuses on
> and optimizes the case where the watchdog is already running. That may not
> always be the case, and there may be systems where the driver is not loaded
> on purpose.

Is this a general comment, or regarding a specific Aspeed BMC-based
system? Currently selecting ARCH_ASPEED will force select WATCHDOG and
ASPEED_WATCHDOG. Thus "default y if ARCH_ASPEED" doesn't actually
change the resulting configuration, but removes the need for the
explicit select in arch/arm/mach-aspeed/Kconfig. I don't know what's
better, but I felt that if we moved from tristate to boolean then
changing the default in the watchdog Kconfig was the right way to go.

The way to reboot the Aspeed SoC is to trip the watchdog. Perhaps
wanting to build without the driver isn't unreasonable, but it would
produce a system that you couldn't reboot. However, that's independent
of whether or not to build as a module so it can remain a tristate. I
think I'll just drop the Kconfig changes and add the exit handler. Sing
out if I should do anything different.

Cheers,

Andrew

> 
> Guenter
> 
> > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> >   drivers/watchdog/Kconfig      | 6 ++----
> >   drivers/watchdog/aspeed_wdt.c | 7 ++++++-
> >   2 files changed, 8 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index a1b92ebe74b6..6103185983ed 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -741,8 +741,9 @@ config RENESAS_RZAWDT
> > > >   	  Renesas RZ/A SoCs. These watchdogs can be used to reset a system.
> >   
> >   config ASPEED_WATCHDOG
> > > > -	tristate "Aspeed BMC watchdog support"
> > > > +	bool "Aspeed BMC watchdog support"
> > > >   	depends on ARCH_ASPEED || COMPILE_TEST
> > > > +	default y if ARCH_ASPEED
> > > >   	select WATCHDOG_CORE
> > > >   	help
> > > >   	  Say Y here to include support for the watchdog timer
> > @@ -750,9 +751,6 @@ config ASPEED_WATCHDOG
> >   
> > > >   	  This driver is required to reboot the SoC.
> >   
> > > > -	  To compile this driver as a module, choose M here: the
> > > > -	  module will be called aspeed_wdt.
> > -
> >   config ZX2967_WATCHDOG
> > > >   	tristate "ZTE zx2967 SoCs watchdog support"
> > > >   	depends on ARCH_ZX
> > diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> > index 99bc6fbd8852..679c35abadc4 100644
> > --- a/drivers/watchdog/aspeed_wdt.c
> > +++ b/drivers/watchdog/aspeed_wdt.c
> > @@ -313,7 +313,12 @@ static struct platform_driver aspeed_watchdog_driver = {
> > > >   		.of_match_table = of_match_ptr(aspeed_wdt_of_table),
> > > >   	},
> >   };
> > -module_platform_driver(aspeed_watchdog_driver);
> > +
> > +static int __init aspeed_wdt_init(void)
> > +{
> > > > +	return platform_driver_register(&aspeed_watchdog_driver);
> > +}
> > +arch_initcall(aspeed_wdt_init);
> >   
> >   MODULE_DESCRIPTION("Aspeed Watchdog Driver");
> >   MODULE_LICENSE("GPL");
> > 
> 
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 1/4] watchdog: aspeed: Retain watchdog enabled state

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 2060 bytes --]

On Mon, 2017-09-18 at 06:25 -0700, Guenter Roeck wrote:
> On 09/17/2017 10:49 PM, Andrew Jeffery wrote:
> > An unintended post-condition of probe() is that the watchdog is
> > disabled. Rework probe() such that we retain the value of the "enabled"
> > bit from the control register, and take the appropriate actions with
> > respect to the watchdog core if so. Otherwise, just configure the
> > watchdog as directed.
> > 
> > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> >   drivers/watchdog/aspeed_wdt.c | 9 +++++----
> >   1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> > index 79cc766cd30f..99bc6fbd8852 100644
> > --- a/drivers/watchdog/aspeed_wdt.c
> > +++ b/drivers/watchdog/aspeed_wdt.c
> > @@ -221,7 +221,8 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
> > > >   		return -EINVAL;
> > > >   	config = ofdid->data;
> >   
> > > > -	wdt->ctrl = WDT_CTRL_1MHZ_CLK;
> > > > +	wdt->ctrl |= readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE;
> > > > +	wdt->ctrl |= WDT_CTRL_1MHZ_CLK;
> >   
> > > >   	/*
> > > >   	 * Control reset on a per-device basis to ensure the
> > @@ -243,11 +244,11 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
> > > >   	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)  {
> > > > +	if (wdt->ctrl & WDT_CTRL_ENABLE) {
> >   		aspeed_wdt_start(&wdt->wdd);
> 
> Why call the start function in this case ?

... Good question. It was already there so I didn't touch it. I expect
it can be dropped - I'll look into it.

> 
> > > > > >   		set_bit(WDOG_HW_RUNNING, &wdt->wdd.status); > +	} else {
> > > > +		writel(wdt->ctrl, wdt->base + WDT_CTRL);
> > > >   	}
> >   
> > > >   	if (of_device_is_compatible(np, "aspeed,ast2500-wdt")) {
> > 
> 
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 1/4] watchdog: aspeed: Retain watchdog enabled state

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 2060 bytes --]

On Mon, 2017-09-18 at 06:25 -0700, Guenter Roeck wrote:
> On 09/17/2017 10:49 PM, Andrew Jeffery wrote:
> > An unintended post-condition of probe() is that the watchdog is
> > disabled. Rework probe() such that we retain the value of the "enabled"
> > bit from the control register, and take the appropriate actions with
> > respect to the watchdog core if so. Otherwise, just configure the
> > watchdog as directed.
> > 
> > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> >   drivers/watchdog/aspeed_wdt.c | 9 +++++----
> >   1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> > index 79cc766cd30f..99bc6fbd8852 100644
> > --- a/drivers/watchdog/aspeed_wdt.c
> > +++ b/drivers/watchdog/aspeed_wdt.c
> > @@ -221,7 +221,8 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
> > > >   		return -EINVAL;
> > > >   	config = ofdid->data;
> >   
> > > > -	wdt->ctrl = WDT_CTRL_1MHZ_CLK;
> > > > +	wdt->ctrl |= readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE;
> > > > +	wdt->ctrl |= WDT_CTRL_1MHZ_CLK;
> >   
> > > >   	/*
> > > >   	 * Control reset on a per-device basis to ensure the
> > @@ -243,11 +244,11 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
> > > >   	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)  {
> > > > +	if (wdt->ctrl & WDT_CTRL_ENABLE) {
> >   		aspeed_wdt_start(&wdt->wdd);
> 
> Why call the start function in this case ?

... Good question. It was already there so I didn't touch it. I expect
it can be dropped - I'll look into it.

> 
> > > > > >   		set_bit(WDOG_HW_RUNNING, &wdt->wdd.status); > +	} else {
> > > > +		writel(wdt->ctrl, wdt->base + WDT_CTRL);
> > > >   	}
> >   
> > > >   	if (of_device_is_compatible(np, "aspeed,ast2500-wdt")) {
> > 
> 
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 1/4] watchdog: aspeed: Retain watchdog enabled state

[Joel Stanley]

On Mon, Sep 18, 2017 at 3:19 PM, Andrew Jeffery <andrew@aj.id.au> wrote:
> An unintended post-condition of probe() is that the watchdog is
> disabled. Rework probe() such that we retain the value of the "enabled"
> bit from the control register, and take the appropriate actions with
> respect to the watchdog core if so. Otherwise, just configure the
> watchdog as directed.
>

This should have a fixes line. The code as it stands in 4.14-rc1
unconditionally disables the watchdog at boot :(

> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  drivers/watchdog/aspeed_wdt.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> index 79cc766cd30f..99bc6fbd8852 100644
> --- a/drivers/watchdog/aspeed_wdt.c
> +++ b/drivers/watchdog/aspeed_wdt.c
> @@ -221,7 +221,8 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
>                 return -EINVAL;
>         config = ofdid->data;
>
> -       wdt->ctrl = WDT_CTRL_1MHZ_CLK;
> +       wdt->ctrl |= readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE;

If we go back to before b7f0b8ad25f3 ("drivers/watchdog: ASPEED
reference dev tree properties for config"), the driver set up the
cached ctrl value and then tested the hardware state to decide if we
should have the watchdog enabled.

Looking at the driver now there's little reason to keep the cached
ctrl value. I'd suggest reworking the driver to not have it so we can
avoid bugs like the ones that b7f0b8ad25f3 introduced.

Cheers,

Joel

> +       wdt->ctrl |= WDT_CTRL_1MHZ_CLK;
>
>         /*
>          * Control reset on a per-device basis to ensure the
> @@ -243,11 +244,11 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
>         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)  {
> +       if (wdt->ctrl & WDT_CTRL_ENABLE) {
>                 aspeed_wdt_start(&wdt->wdd);
>                 set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
> +       } else {
> +               writel(wdt->ctrl, wdt->base + WDT_CTRL);
>         }
>
>         if (of_device_is_compatible(np, "aspeed,ast2500-wdt")) {
> --
> 2.11.0
>

Re: [PATCH 1/4] watchdog: aspeed: Retain watchdog enabled state

[Joel Stanley]

On Mon, Sep 18, 2017 at 3:19 PM, Andrew Jeffery <andrew@aj.id.au> wrote:
> An unintended post-condition of probe() is that the watchdog is
> disabled. Rework probe() such that we retain the value of the "enabled"
> bit from the control register, and take the appropriate actions with
> respect to the watchdog core if so. Otherwise, just configure the
> watchdog as directed.
>

This should have a fixes line. The code as it stands in 4.14-rc1
unconditionally disables the watchdog at boot :(

> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  drivers/watchdog/aspeed_wdt.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> index 79cc766cd30f..99bc6fbd8852 100644
> --- a/drivers/watchdog/aspeed_wdt.c
> +++ b/drivers/watchdog/aspeed_wdt.c
> @@ -221,7 +221,8 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
>                 return -EINVAL;
>         config = ofdid->data;
>
> -       wdt->ctrl = WDT_CTRL_1MHZ_CLK;
> +       wdt->ctrl |= readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE;

If we go back to before b7f0b8ad25f3 ("drivers/watchdog: ASPEED
reference dev tree properties for config"), the driver set up the
cached ctrl value and then tested the hardware state to decide if we
should have the watchdog enabled.

Looking at the driver now there's little reason to keep the cached
ctrl value. I'd suggest reworking the driver to not have it so we can
avoid bugs like the ones that b7f0b8ad25f3 introduced.

Cheers,

Joel

> +       wdt->ctrl |= WDT_CTRL_1MHZ_CLK;
>
>         /*
>          * Control reset on a per-device basis to ensure the
> @@ -243,11 +244,11 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
>         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)  {
> +       if (wdt->ctrl & WDT_CTRL_ENABLE) {
>                 aspeed_wdt_start(&wdt->wdd);
>                 set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
> +       } else {
> +               writel(wdt->ctrl, wdt->base + WDT_CTRL);
>         }
>
>         if (of_device_is_compatible(np, "aspeed,ast2500-wdt")) {
> --
> 2.11.0
>

Re: [PATCH 1/4] watchdog: aspeed: Retain watchdog enabled state

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 3451 bytes --]

On Wed, 2017-09-20 at 11:17 +0930, Joel Stanley wrote:
> > On Mon, Sep 18, 2017 at 3:19 PM, Andrew Jeffery <andrew@aj.id.au> wrote:
> > An unintended post-condition of probe() is that the watchdog is
> > disabled. Rework probe() such that we retain the value of the "enabled"
> > bit from the control register, and take the appropriate actions with
> > respect to the watchdog core if so. Otherwise, just configure the
> > watchdog as directed.
> > 
> 
> This should have a fixes line. The code as it stands in 4.14-rc1
> unconditionally disables the watchdog at boot :(

I'll add a fixes line.

> 
> > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> >  drivers/watchdog/aspeed_wdt.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> > index 79cc766cd30f..99bc6fbd8852 100644
> > --- a/drivers/watchdog/aspeed_wdt.c
> > +++ b/drivers/watchdog/aspeed_wdt.c
> > @@ -221,7 +221,8 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
> >                 return -EINVAL;
> >         config = ofdid->data;
> > 
> > -       wdt->ctrl = WDT_CTRL_1MHZ_CLK;
> > +       wdt->ctrl |= readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE;
> 
> If we go back to before b7f0b8ad25f3 ("drivers/watchdog: ASPEED
> reference dev tree properties for config"), the driver set up the
> cached ctrl value and then tested the hardware state to decide if we
> should have the watchdog enabled.
> 
> Looking at the driver now there's little reason to keep the cached
> ctrl value. I'd suggest reworking the driver to not have it so we can
> avoid bugs like the ones that b7f0b8ad25f3 introduced.

Alternatively, we can just drop the write I moved to the else branch
and rely on aspeed_wdt_start() to configure the control register for
us.

Guenter: This means we retain the call you questioned. It's name might
be slightly misleading in terms of the effect it gives us (configuring
the watchdog to match the driver's assumptions), but doing it that way
effectively turns my original patch into a one-liner to delete the
writel(). If the watchdog is disabled at the point of kernel
initialisation then the configuration doesn't matter until userspace
opens the chardev, at which point we'll write the configuration via
aspeed_wdt_start() anyway.

Andrew

> 
> Cheers,
> 
> Joel
> 
> > +       wdt->ctrl |= WDT_CTRL_1MHZ_CLK;
> > 
> >         /*
> >          * Control reset on a per-device basis to ensure the
> > @@ -243,11 +244,11 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
> >         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)  {
> > +       if (wdt->ctrl & WDT_CTRL_ENABLE) {
> >                 aspeed_wdt_start(&wdt->wdd);
> >                 set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
> > +       } else {
> > +               writel(wdt->ctrl, wdt->base + WDT_CTRL);
> >         }
> > 
> >         if (of_device_is_compatible(np, "aspeed,ast2500-wdt")) {
> > --
> > 2.11.0
> > 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 1/4] watchdog: aspeed: Retain watchdog enabled state

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 3451 bytes --]

On Wed, 2017-09-20 at 11:17 +0930, Joel Stanley wrote:
> > On Mon, Sep 18, 2017 at 3:19 PM, Andrew Jeffery <andrew@aj.id.au> wrote:
> > An unintended post-condition of probe() is that the watchdog is
> > disabled. Rework probe() such that we retain the value of the "enabled"
> > bit from the control register, and take the appropriate actions with
> > respect to the watchdog core if so. Otherwise, just configure the
> > watchdog as directed.
> > 
> 
> This should have a fixes line. The code as it stands in 4.14-rc1
> unconditionally disables the watchdog at boot :(

I'll add a fixes line.

> 
> > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> >  drivers/watchdog/aspeed_wdt.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> > index 79cc766cd30f..99bc6fbd8852 100644
> > --- a/drivers/watchdog/aspeed_wdt.c
> > +++ b/drivers/watchdog/aspeed_wdt.c
> > @@ -221,7 +221,8 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
> >                 return -EINVAL;
> >         config = ofdid->data;
> > 
> > -       wdt->ctrl = WDT_CTRL_1MHZ_CLK;
> > +       wdt->ctrl |= readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE;
> 
> If we go back to before b7f0b8ad25f3 ("drivers/watchdog: ASPEED
> reference dev tree properties for config"), the driver set up the
> cached ctrl value and then tested the hardware state to decide if we
> should have the watchdog enabled.
> 
> Looking at the driver now there's little reason to keep the cached
> ctrl value. I'd suggest reworking the driver to not have it so we can
> avoid bugs like the ones that b7f0b8ad25f3 introduced.

Alternatively, we can just drop the write I moved to the else branch
and rely on aspeed_wdt_start() to configure the control register for
us.

Guenter: This means we retain the call you questioned. It's name might
be slightly misleading in terms of the effect it gives us (configuring
the watchdog to match the driver's assumptions), but doing it that way
effectively turns my original patch into a one-liner to delete the
writel(). If the watchdog is disabled at the point of kernel
initialisation then the configuration doesn't matter until userspace
opens the chardev, at which point we'll write the configuration via
aspeed_wdt_start() anyway.

Andrew

> 
> Cheers,
> 
> Joel
> 
> > +       wdt->ctrl |= WDT_CTRL_1MHZ_CLK;
> > 
> >         /*
> >          * Control reset on a per-device basis to ensure the
> > @@ -243,11 +244,11 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
> >         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)  {
> > +       if (wdt->ctrl & WDT_CTRL_ENABLE) {
> >                 aspeed_wdt_start(&wdt->wdd);
> >                 set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
> > +       } else {
> > +               writel(wdt->ctrl, wdt->base + WDT_CTRL);
> >         }
> > 
> >         if (of_device_is_compatible(np, "aspeed,ast2500-wdt")) {
> > --
> > 2.11.0
> > 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]