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

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

[Andrew Jeffery]

Hello,

This series fixes some issues with the Aspeed watchdog, as outlined in the
v1[1] cover letter.

I've added a bit of an explanation as to why we call aspeed_wdt_start() in
probe() if the watchdog is already enabled, but the essence of it is to ensure
the watchdog is configured to the expectations of the kernel driver (rather
than however the bootloader configured it).

Please review!

Andrew

Since v1:

* Rework patch 1/4 to simply remove the writel() call to restore the original
  behaviour.
* Rework patch 4/4 to remove the Kconfig changes to retain the ability to build
  the watchdog as a module, and add the exit handler.

[1] https://lkml.org/lkml/2017/9/18/14

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      |  4 ++--
 drivers/watchdog/aspeed_wdt.c | 21 ++++++++++++++++++---
 2 files changed, 20 insertions(+), 5 deletions(-)

-- 
2.11.0

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

[Andrew Jeffery]

An unintended post-condition of probe() is that the watchdog is
disabled. This behaviour was introduced by an unnecessary write to the
control register to configure the hardware based on the devicetree. The
write is unnecessary because the cached control value that is
manipulated by the code parsing the devicetree is eventually written by
aspeed_wdt_enable(), which is when we care how the control register
should be configured.

Remove the write to restore expected behaviour.

Fixes: b7f0b8ad25f3 ("drivers/watchdog: ASPEED reference dev tree properties for config")
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/watchdog/aspeed_wdt.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
index 79cc766cd30f..6c6dd3f4c48d 100644
--- a/drivers/watchdog/aspeed_wdt.c
+++ b/drivers/watchdog/aspeed_wdt.c
@@ -243,9 +243,13 @@ 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)  {
+		/*
+		 * The watchdog is running, but invoke aspeed_wdt_start() to
+		 * write wdt->ctrl to WDT_CTRL to ensure the watchdog's
+		 * configuration conforms to the driver's expectations.
+		 * Primarily, ensure we're using the 1MHz clock source.
+		 */
 		aspeed_wdt_start(&wdt->wdd);
 		set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
 	}
-- 
2.11.0

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

[Joel Stanley]

On Wed, Sep 20, 2017 at 3:00 PM, Andrew Jeffery <andrew@aj.id.au> wrote:
> An unintended post-condition of probe() is that the watchdog is
> disabled. This behaviour was introduced by an unnecessary write to the
> control register to configure the hardware based on the devicetree. The
> write is unnecessary because the cached control value that is
> manipulated by the code parsing the devicetree is eventually written by
> aspeed_wdt_enable(), which is when we care how the control register
> should be configured.
>
> Remove the write to restore expected behaviour.
>
> Fixes: b7f0b8ad25f3 ("drivers/watchdog: ASPEED reference dev tree properties for config")
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>

Reviewed-by: Joel Stanley <joel@jms.id.au>

Thanks,

Joel

[v2,1/4] watchdog: aspeed: Retain watchdog enabled state

[Guenter Roeck]

On Wed, Sep 20, 2017 at 03:00:17PM +0930, Andrew Jeffery wrote:
> An unintended post-condition of probe() is that the watchdog is
> disabled. This behaviour was introduced by an unnecessary write to the
> control register to configure the hardware based on the devicetree. The
> write is unnecessary because the cached control value that is
> manipulated by the code parsing the devicetree is eventually written by
> aspeed_wdt_enable(), which is when we care how the control register
> should be configured.
> 
> Remove the write to restore expected behaviour.
> 
> Fixes: b7f0b8ad25f3 ("drivers/watchdog: ASPEED reference dev tree properties for config")
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> Reviewed-by: Joel Stanley <joel@jms.id.au>

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

> ---
>  drivers/watchdog/aspeed_wdt.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> index 79cc766cd30f..6c6dd3f4c48d 100644
> --- a/drivers/watchdog/aspeed_wdt.c
> +++ b/drivers/watchdog/aspeed_wdt.c
> @@ -243,9 +243,13 @@ 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)  {
> +		/*
> +		 * The watchdog is running, but invoke aspeed_wdt_start() to
> +		 * write wdt->ctrl to WDT_CTRL to ensure the watchdog's
> +		 * configuration conforms to the driver's expectations.
> +		 * Primarily, ensure we're using the 1MHz clock source.
> +		 */
>  		aspeed_wdt_start(&wdt->wdd);
>  		set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
>  	}

[PATCH v2 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>
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 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 v2 2/4] watchdog: aspeed: Fix 'Apseed' typo in Kconfig

[Joel Stanley]

On Wed, Sep 20, 2017 at 3:00 PM, Andrew Jeffery <andrew@aj.id.au> 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 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 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.

lol.

Reviewed-by: Joel Stanley <joel@jms.id.au>

[PATCH v2 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>
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

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

[Joel Stanley]

On Wed, Sep 20, 2017 at 3:00 PM, Andrew Jeffery <andrew@aj.id.au> 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>

Reviewed-by: Joel Stanley <joel@jms.id.au>

[PATCH v2 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 devices, 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/aspeed_wdt.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
index 6c6dd3f4c48d..ca5b91e2eb92 100644
--- a/drivers/watchdog/aspeed_wdt.c
+++ b/drivers/watchdog/aspeed_wdt.c
@@ -316,7 +316,18 @@ 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);
+
+static void __exit aspeed_wdt_exit(void)
+{
+	platform_driver_unregister(&aspeed_watchdog_driver);
+}
+module_exit(aspeed_wdt_exit);
 
 MODULE_DESCRIPTION("Aspeed Watchdog Driver");
 MODULE_LICENSE("GPL");
-- 
2.11.0

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

[Joel Stanley]

On Wed, Sep 20, 2017 at 3:00 PM, Andrew Jeffery <andrew@aj.id.au> wrote:
> Probing at device_initcall time lead to perverse cases where the
> watchdog was probed after, say, I2C devices, 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>

I agree that we need to make sure the watchdog driver is loaded
earlier. I think this is the correct method, but I'll defer to Guenter
on this one.

Cheers,

Joel

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

[Andrew Jeffery]

On Wed, 2017-09-20 at 15:43 +0930, Joel Stanley wrote:
> > On Wed, Sep 20, 2017 at 3:00 PM, Andrew Jeffery <andrew@aj.id.au> wrote:
> > Probing at device_initcall time lead to perverse cases where the
> > watchdog was probed after, say, I2C devices, 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>
> 
> I agree that we need to make sure the watchdog driver is loaded
> earlier. I think this is the correct method, but I'll defer to Guenter
> on this one.

Just following up on Joel's comments: Is there anything else I need to
address?

Cheers,

Andrew
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/linux-aspeed/attachments/20171017/ba995a0c/attachment.sig>

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

[Guenter Roeck]

On 10/17/2017 04:35 AM, Andrew Jeffery wrote:
> On Wed, 2017-09-20 at 15:43 +0930, Joel Stanley wrote:
>>> On Wed, Sep 20, 2017 at 3:00 PM, Andrew Jeffery <andrew@aj.id.au> wrote:
>>> Probing at device_initcall time lead to perverse cases where the
>>> watchdog was probed after, say, I2C devices, 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>
>>
>> I agree that we need to make sure the watchdog driver is loaded
>> earlier. I think this is the correct method, but I'll defer to Guenter
>> on this one.
> 
> Just following up on Joel's comments: Is there anything else I need to
> address?
> 

No. Sorry, I have been terribly busy. Digging through patch backlog today.

Guenter

[v2,4/4] watchdog: aspeed: Move init to arch_initcall

[Guenter Roeck]

On Wed, Sep 20, 2017 at 03:00:20PM +0930, Andrew Jeffery wrote:
> Probing at device_initcall time lead to perverse cases where the
> watchdog was probed after, say, I2C devices, 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>

At some point we'll need to come up with a generic means to handle those
situations. Until then, I don't have a better idea.

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

> ---
>  drivers/watchdog/aspeed_wdt.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> index 6c6dd3f4c48d..ca5b91e2eb92 100644
> --- a/drivers/watchdog/aspeed_wdt.c
> +++ b/drivers/watchdog/aspeed_wdt.c
> @@ -316,7 +316,18 @@ 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);
> +
> +static void __exit aspeed_wdt_exit(void)
> +{
> +	platform_driver_unregister(&aspeed_watchdog_driver);
> +}
> +module_exit(aspeed_wdt_exit);
>  
>  MODULE_DESCRIPTION("Aspeed Watchdog Driver");
>  MODULE_LICENSE("GPL");