Linux-Watchdog Archive on lore.kernel.org
 help / Atom feed
* [RFC PATCH] watchdog: renesas_wdt: support handover from bootloader
@ 2019-04-15 10:52 Wolfram Sang
  2019-04-17 18:05 ` Guenter Roeck
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Wolfram Sang @ 2019-04-15 10:52 UTC (permalink / raw)
  To: linux-watchdog; +Cc: linux-renesas-soc, Geert Uytterhoeven, Wolfram Sang

Support an already running watchdog by checking its enable bit and set
up the status accordingly before registering the device.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

This patch was tested using a Renesas Salvator XS board (R-Car M3N). It works.
However, there is a small window where the watchdog clock is disabled, namely
after the MSSR clock driver initializes it until RuntimePM of the watchdog
driver takes over. If the system hangs in this window, bad luck. So, I'd think
it makes sense to have this clock either always-on or to keep the state which
came from the firmware. Geert, what do you think?

 drivers/watchdog/renesas_wdt.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
index 565dbc1ec638..37d757288b22 100644
--- a/drivers/watchdog/renesas_wdt.c
+++ b/drivers/watchdog/renesas_wdt.c
@@ -179,6 +179,7 @@ static int rwdt_probe(struct platform_device *pdev)
 	struct clk *clk;
 	unsigned long clks_per_sec;
 	int ret, i;
+	u8 csra;
 
 	if (rwdt_blacklisted(&pdev->dev))
 		return -ENODEV;
@@ -198,8 +199,8 @@ static int rwdt_probe(struct platform_device *pdev)
 	pm_runtime_enable(&pdev->dev);
 	pm_runtime_get_sync(&pdev->dev);
 	priv->clk_rate = clk_get_rate(clk);
-	priv->wdev.bootstatus = (readb_relaxed(priv->base + RWTCSRA) &
-				RWTCSRA_WOVF) ? WDIOF_CARDRESET : 0;
+	csra = readb_relaxed(priv->base + RWTCSRA);
+	priv->wdev.bootstatus = csra & RWTCSRA_WOVF ? WDIOF_CARDRESET : 0;
 	pm_runtime_put(&pdev->dev);
 
 	if (!priv->clk_rate) {
@@ -237,6 +238,16 @@ static int rwdt_probe(struct platform_device *pdev)
 	/* This overrides the default timeout only if DT configuration was found */
 	watchdog_init_timeout(&priv->wdev, 0, &pdev->dev);
 
+	if (csra & RWTCSRA_TME) {
+		/* Ensure properly initialized dividers */
+		rwdt_start(&priv->wdev);
+		set_bit(WDOG_HW_RUNNING, &priv->wdev.status);
+		//FIXME: We are missing pm_runtime_put in some code paths to
+		// to balance PM calls. We first need to decide if we maybe
+		// should have the RWDT clock always-on or if using RPM for
+		// clock management is OK.
+	}
+
 	ret = watchdog_register_device(&priv->wdev);
 	if (ret < 0)
 		goto out_pm_disable;
-- 
2.11.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] watchdog: renesas_wdt: support handover from bootloader
  2019-04-15 10:52 [RFC PATCH] watchdog: renesas_wdt: support handover from bootloader Wolfram Sang
@ 2019-04-17 18:05 ` Guenter Roeck
  2019-04-17 19:01   ` Wolfram Sang
  2019-05-09  7:38 ` Geert Uytterhoeven
  2019-05-24 13:52 ` Wolfram Sang
  2 siblings, 1 reply; 7+ messages in thread
From: Guenter Roeck @ 2019-04-17 18:05 UTC (permalink / raw)
  To: Wolfram Sang; +Cc: linux-watchdog, linux-renesas-soc, Geert Uytterhoeven

On Mon, Apr 15, 2019 at 12:52:01PM +0200, Wolfram Sang wrote:
> Support an already running watchdog by checking its enable bit and set
> up the status accordingly before registering the device.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> This patch was tested using a Renesas Salvator XS board (R-Car M3N). It works.
> However, there is a small window where the watchdog clock is disabled, namely
> after the MSSR clock driver initializes it until RuntimePM of the watchdog
> driver takes over. If the system hangs in this window, bad luck. So, I'd think
> it makes sense to have this clock either always-on or to keep the state which
> came from the firmware. Geert, what do you think?
> 
>  drivers/watchdog/renesas_wdt.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
> index 565dbc1ec638..37d757288b22 100644
> --- a/drivers/watchdog/renesas_wdt.c
> +++ b/drivers/watchdog/renesas_wdt.c
> @@ -179,6 +179,7 @@ static int rwdt_probe(struct platform_device *pdev)
>  	struct clk *clk;
>  	unsigned long clks_per_sec;
>  	int ret, i;
> +	u8 csra;
>  
>  	if (rwdt_blacklisted(&pdev->dev))
>  		return -ENODEV;
> @@ -198,8 +199,8 @@ static int rwdt_probe(struct platform_device *pdev)
>  	pm_runtime_enable(&pdev->dev);
>  	pm_runtime_get_sync(&pdev->dev);
>  	priv->clk_rate = clk_get_rate(clk);
> -	priv->wdev.bootstatus = (readb_relaxed(priv->base + RWTCSRA) &
> -				RWTCSRA_WOVF) ? WDIOF_CARDRESET : 0;
> +	csra = readb_relaxed(priv->base + RWTCSRA);
> +	priv->wdev.bootstatus = csra & RWTCSRA_WOVF ? WDIOF_CARDRESET : 0;
>  	pm_runtime_put(&pdev->dev);
>  
>  	if (!priv->clk_rate) {
> @@ -237,6 +238,16 @@ static int rwdt_probe(struct platform_device *pdev)
>  	/* This overrides the default timeout only if DT configuration was found */
>  	watchdog_init_timeout(&priv->wdev, 0, &pdev->dev);
>  
> +	if (csra & RWTCSRA_TME) {
> +		/* Ensure properly initialized dividers */
> +		rwdt_start(&priv->wdev);
> +		set_bit(WDOG_HW_RUNNING, &priv->wdev.status);
> +		//FIXME: We are missing pm_runtime_put in some code paths to
> +		// to balance PM calls. We first need to decide if we maybe
> +		// should have the RWDT clock always-on or if using RPM for
> +		// clock management is OK.

Maybe I am missing something, but ..

Is handover even possible if the clock is controlled by clock management ?
Seems to me the clock would then be turned off through pm, which effectively
turns off the watchdog. So it will be off between clock/pm initialization
and the above code, meaning wdt handover from the boot loader is for all
practical purposes useless if the kernel gets stuck in between.

Thanks,
Guenter

> +	}
> +
>  	ret = watchdog_register_device(&priv->wdev);
>  	if (ret < 0)
>  		goto out_pm_disable;
> -- 
> 2.11.0
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] watchdog: renesas_wdt: support handover from bootloader
  2019-04-17 18:05 ` Guenter Roeck
@ 2019-04-17 19:01   ` Wolfram Sang
  0 siblings, 0 replies; 7+ messages in thread
From: Wolfram Sang @ 2019-04-17 19:01 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Wolfram Sang, linux-watchdog, linux-renesas-soc, Geert Uytterhoeven

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

Hi Guenter,

> > driver takes over. If the system hangs in this window, bad luck. So, I'd think
> > it makes sense to have this clock either always-on or to keep the state which
> > came from the firmware.

I wrote this paragraph...

> Is handover even possible if the clock is controlled by clock management ?
> Seems to me the clock would then be turned off through pm, which effectively
> turns off the watchdog. So it will be off between clock/pm initialization
> and the above code, meaning wdt handover from the boot loader is for all
> practical purposes useless if the kernel gets stuck in between.

... because I fully agree with you :)


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] watchdog: renesas_wdt: support handover from bootloader
  2019-04-15 10:52 [RFC PATCH] watchdog: renesas_wdt: support handover from bootloader Wolfram Sang
  2019-04-17 18:05 ` Guenter Roeck
@ 2019-05-09  7:38 ` Geert Uytterhoeven
  2019-05-24 13:52 ` Wolfram Sang
  2 siblings, 0 replies; 7+ messages in thread
From: Geert Uytterhoeven @ 2019-05-09  7:38 UTC (permalink / raw)
  To: Wolfram Sang; +Cc: Linux Watchdog Mailing List, Linux-Renesas

Hi Wolfram,

On Mon, Apr 15, 2019 at 12:52 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> Support an already running watchdog by checking its enable bit and set
> up the status accordingly before registering the device.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>
> This patch was tested using a Renesas Salvator XS board (R-Car M3N). It works.
> However, there is a small window where the watchdog clock is disabled, namely
> after the MSSR clock driver initializes it until RuntimePM of the watchdog
> driver takes over. If the system hangs in this window, bad luck. So, I'd think
> it makes sense to have this clock either always-on or to keep the state which
> came from the firmware. Geert, what do you think?

The MSSR clock driver does not disable the clock. The clock's core
clk_disable_unused() does, which is a late initcall.
So if the handover code calls rwdt_start() before that (i.e. no deferred
probing happens), the clock would never be disabled.

Note that pm_runtime_put() in rwdt_probe() queues a power down request,
but as it is not the _sync variant, it is delayed by some time, so
probably it would never happen if rwdt_start() is called by the handover
code in probe.

Now, if we would mark the clock always-on (CLK_IS_CRITICAL),
we can never disable it, even if the wdt is not used or the driver is
not compiled-in.

I don't think there's a way to mark a clock as "keep the state which
came from the firmware", CLK_IS_CRITICAL enables the clock in
__clk_core_init().

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] watchdog: renesas_wdt: support handover from bootloader
  2019-04-15 10:52 [RFC PATCH] watchdog: renesas_wdt: support handover from bootloader Wolfram Sang
  2019-04-17 18:05 ` Guenter Roeck
  2019-05-09  7:38 ` Geert Uytterhoeven
@ 2019-05-24 13:52 ` Wolfram Sang
  2019-06-07 20:41   ` Guenter Roeck
  2 siblings, 1 reply; 7+ messages in thread
From: Wolfram Sang @ 2019-05-24 13:52 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: linux-watchdog, linux-renesas-soc, Geert Uytterhoeven, Guenter Roeck

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

On Mon, Apr 15, 2019 at 12:52:01PM +0200, Wolfram Sang wrote:
> Support an already running watchdog by checking its enable bit and set
> up the status accordingly before registering the device.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

After second thought, I am getting confused a little. If the WDT is
already running then

a) before this patch: after successful probe, RPM will disable the
clock until userspace opens the watchdog device

b) after this patch: during probe, our default timeout will be
programmed and because of WDOG_HW_RUNNING, the core will generate pings
until userspace opens the watchdog device.

So, b) will protect from a crashing kernel (no pings anymore) but not
from something like missing rootfs, or?

The usecase I had in mind ("give the kernel <x> seconds to boot into
working userspace") seems to be achieved by loading the WDT driver as a
module then, I guess?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] watchdog: renesas_wdt: support handover from bootloader
  2019-05-24 13:52 ` Wolfram Sang
@ 2019-06-07 20:41   ` Guenter Roeck
  2019-06-07 20:55     ` Wolfram Sang
  0 siblings, 1 reply; 7+ messages in thread
From: Guenter Roeck @ 2019-06-07 20:41 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Wolfram Sang, linux-watchdog, linux-renesas-soc, Geert Uytterhoeven

On Fri, May 24, 2019 at 03:52:37PM +0200, Wolfram Sang wrote:
> On Mon, Apr 15, 2019 at 12:52:01PM +0200, Wolfram Sang wrote:
> > Support an already running watchdog by checking its enable bit and set
> > up the status accordingly before registering the device.
> > 
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> After second thought, I am getting confused a little. If the WDT is
> already running then
> 
> a) before this patch: after successful probe, RPM will disable the
> clock until userspace opens the watchdog device
> 
> b) after this patch: during probe, our default timeout will be
> programmed and because of WDOG_HW_RUNNING, the core will generate pings
> until userspace opens the watchdog device.
> 
> So, b) will protect from a crashing kernel (no pings anymore) but not
> from something like missing rootfs, or?
> 
> The usecase I had in mind ("give the kernel <x> seconds to boot into
> working userspace") seems to be achieved by loading the WDT driver as a
> module then, I guess?
> 

Would
https://lore.kernel.org/linux-watchdog/20190605140628.618-1-rasmus.villemoes@prevas.dk/

solve your use case ?

Guenter

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] watchdog: renesas_wdt: support handover from bootloader
  2019-06-07 20:41   ` Guenter Roeck
@ 2019-06-07 20:55     ` Wolfram Sang
  0 siblings, 0 replies; 7+ messages in thread
From: Wolfram Sang @ 2019-06-07 20:55 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Wolfram Sang, linux-watchdog, linux-renesas-soc, Geert Uytterhoeven

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


> > The usecase I had in mind ("give the kernel <x> seconds to boot into
> > working userspace") seems to be achieved by loading the WDT driver as a
> > module then, I guess?
> > 
> 
> Would
> https://lore.kernel.org/linux-watchdog/20190605140628.618-1-rasmus.villemoes@prevas.dk/
> 
> solve your use case ?

Yes, it would. Thanks for the pointer! I missed the
"handle_boot_enabled" parameter, too, for some reason. I think this
could also be enough for some scenarios.

As a result, it seems it makes sense to respin my patch, and test it
with "handle_boot_enabled" and the patch series you were pointing out.

I'll try to get this done within the next two weeks.

Thanks again!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-15 10:52 [RFC PATCH] watchdog: renesas_wdt: support handover from bootloader Wolfram Sang
2019-04-17 18:05 ` Guenter Roeck
2019-04-17 19:01   ` Wolfram Sang
2019-05-09  7:38 ` Geert Uytterhoeven
2019-05-24 13:52 ` Wolfram Sang
2019-06-07 20:41   ` Guenter Roeck
2019-06-07 20:55     ` Wolfram Sang

Linux-Watchdog Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-watchdog/0 linux-watchdog/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-watchdog linux-watchdog/ https://lore.kernel.org/linux-watchdog \
		linux-watchdog@vger.kernel.org linux-watchdog@archiver.kernel.org
	public-inbox-index linux-watchdog


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-watchdog


AGPL code for this site: git clone https://public-inbox.org/ public-inbox