* 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