* [PATCH] watchdog: renesas_wdt: support handover from bootloader
@ 2019-08-18 18:00 Wolfram Sang
2019-08-18 19:00 ` Guenter Roeck
0 siblings, 1 reply; 3+ messages in thread
From: Wolfram Sang @ 2019-08-18 18:00 UTC (permalink / raw)
To: linux-watchdog
Cc: linux-renesas-soc, Geert Uytterhoeven, Guenter Roeck, Wolfram Sang
From: Wolfram Sang <wsa+renesas@sang-engineering.com>
Support an already running watchdog by checking its enable bit and set
up the status accordingly before registering the device. Introduce a new
flag to remember all this to keep RPM calls balanced.
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
Changes since RFC:
* Geert ensured that the module clock for the RWDT will stay active
during the boot process because clock will only be stopped at the end
of init if there is no refcnt for this clk.
* So, we make sure to have a refcnt when FW enabled the wdog. Once the
first call to open comes, we "transfer" the refcnt to that call.
(Is that the correct behaviour? I think it is a tad better than to
place the balancing RPM call in remove, but I am open here)
* Tested with "open_timeout" kernel parameter. System can now reboot
if userspace hasn't taken over the watchdog with <n> seconds.
drivers/watchdog/renesas_wdt.c | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
index 00662a8e039c..11cef69f329b 100644
--- a/drivers/watchdog/renesas_wdt.c
+++ b/drivers/watchdog/renesas_wdt.c
@@ -50,6 +50,7 @@ struct rwdt_priv {
struct watchdog_device wdev;
unsigned long clk_rate;
u8 cks;
+ bool started_by_fw;
};
static void rwdt_write(struct rwdt_priv *priv, u32 val, unsigned int reg)
@@ -85,7 +86,11 @@ static int rwdt_start(struct watchdog_device *wdev)
struct rwdt_priv *priv = watchdog_get_drvdata(wdev);
u8 val;
- pm_runtime_get_sync(wdev->parent);
+ if (priv->started_by_fw)
+ /* we already called this function and RPM is active */
+ priv->started_by_fw = false;
+ else
+ pm_runtime_get_sync(wdev->parent);
/* Stop the timer before we modify any register */
val = readb_relaxed(priv->base + RWTCSRA) & ~RWTCSRA_TME;
@@ -194,6 +199,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(dev))
return -ENODEV;
@@ -213,8 +219,8 @@ static int rwdt_probe(struct platform_device *pdev)
pm_runtime_enable(dev);
pm_runtime_get_sync(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(dev);
if (!priv->clk_rate) {
@@ -252,6 +258,14 @@ 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, dev);
+ /* Check if FW enabled the watchdog */
+ if (csra & RWTCSRA_TME) {
+ /* Ensure properly initialized dividers */
+ rwdt_start(&priv->wdev);
+ set_bit(WDOG_HW_RUNNING, &priv->wdev.status);
+ priv->started_by_fw = true;
+ }
+
ret = watchdog_register_device(&priv->wdev);
if (ret < 0)
goto out_pm_disable;
--
2.20.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] watchdog: renesas_wdt: support handover from bootloader
2019-08-18 18:00 [PATCH] watchdog: renesas_wdt: support handover from bootloader Wolfram Sang
@ 2019-08-18 19:00 ` Guenter Roeck
2019-08-19 16:06 ` Wolfram Sang
0 siblings, 1 reply; 3+ messages in thread
From: Guenter Roeck @ 2019-08-18 19:00 UTC (permalink / raw)
To: Wolfram Sang, linux-watchdog
Cc: linux-renesas-soc, Geert Uytterhoeven, Wolfram Sang
On 8/18/19 11:00 AM, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
>
> Support an already running watchdog by checking its enable bit and set
> up the status accordingly before registering the device. Introduce a new
> flag to remember all this to keep RPM calls balanced.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>
> Changes since RFC:
>
> * Geert ensured that the module clock for the RWDT will stay active
> during the boot process because clock will only be stopped at the end
> of init if there is no refcnt for this clk.
>
> * So, we make sure to have a refcnt when FW enabled the wdog. Once the
> first call to open comes, we "transfer" the refcnt to that call.
> (Is that the correct behaviour? I think it is a tad better than to
> place the balancing RPM call in remove, but I am open here)
>
> * Tested with "open_timeout" kernel parameter. System can now reboot
> if userspace hasn't taken over the watchdog with <n> seconds.
>
>
> drivers/watchdog/renesas_wdt.c | 20 +++++++++++++++++---
> 1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
> index 00662a8e039c..11cef69f329b 100644
> --- a/drivers/watchdog/renesas_wdt.c
> +++ b/drivers/watchdog/renesas_wdt.c
> @@ -50,6 +50,7 @@ struct rwdt_priv {
> struct watchdog_device wdev;
> unsigned long clk_rate;
> u8 cks;
> + bool started_by_fw;
> };
>
> static void rwdt_write(struct rwdt_priv *priv, u32 val, unsigned int reg)
> @@ -85,7 +86,11 @@ static int rwdt_start(struct watchdog_device *wdev)
> struct rwdt_priv *priv = watchdog_get_drvdata(wdev);
> u8 val;
>
> - pm_runtime_get_sync(wdev->parent);
> + if (priv->started_by_fw)
> + /* we already called this function and RPM is active */
> + priv->started_by_fw = false;
If the HW watchdog is running, the start function should not be called
on open. It would be called after the watchdog was stopped and is then
started again. With that, opening the watchdog the first time would not
call this function, and started_by_fw would remain true. Closing it would
then stopp the watchdog and call pm_runtime_put(). The next open() would
hit the above case, and not call pm_runtime_get_sync(). pm would then be
out of sync.
What am I missing ?
Guenter
> + else
> + pm_runtime_get_sync(wdev->parent);
>
> /* Stop the timer before we modify any register */
> val = readb_relaxed(priv->base + RWTCSRA) & ~RWTCSRA_TME;
> @@ -194,6 +199,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(dev))
> return -ENODEV;
> @@ -213,8 +219,8 @@ static int rwdt_probe(struct platform_device *pdev)
> pm_runtime_enable(dev);
> pm_runtime_get_sync(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(dev);
>
> if (!priv->clk_rate) {
> @@ -252,6 +258,14 @@ 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, dev);
>
> + /* Check if FW enabled the watchdog */
> + if (csra & RWTCSRA_TME) {
> + /* Ensure properly initialized dividers */
> + rwdt_start(&priv->wdev);
> + set_bit(WDOG_HW_RUNNING, &priv->wdev.status);
> + priv->started_by_fw = true;
> + }
> +
> ret = watchdog_register_device(&priv->wdev);
> if (ret < 0)
> goto out_pm_disable;
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] watchdog: renesas_wdt: support handover from bootloader
2019-08-18 19:00 ` Guenter Roeck
@ 2019-08-19 16:06 ` Wolfram Sang
0 siblings, 0 replies; 3+ messages in thread
From: Wolfram Sang @ 2019-08-19 16:06 UTC (permalink / raw)
To: Guenter Roeck
Cc: linux-watchdog, linux-renesas-soc, Geert Uytterhoeven, Wolfram Sang
[-- Attachment #1: Type: text/plain, Size: 999 bytes --]
> If the HW watchdog is running, the start function should not be called
> on open. It would be called after the watchdog was stopped and is then
> started again. With that, opening the watchdog the first time would not
> call this function, and started_by_fw would remain true. Closing it would
> then stopp the watchdog and call pm_runtime_put(). The next open() would
> hit the above case, and not call pm_runtime_get_sync(). pm would then be
> out of sync.
>
> What am I missing ?
Obviously, I was missing something. I didn't see that the watchdog core
calls ->ping instead of ->start when WDOG_HW_RUNNING is set. And once
I knew about it, I even found it in the documentation:
+ Note: when you register the watchdog timer device with this bit set,
+ then opening /dev/watchdog will skip the start operation but send a keepalive
+ request instead.
This makes all my code to keep the refcnt balanced very moot.
Sorry for the noise. I will fix up things and retest.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-08-19 16:06 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-18 18:00 [PATCH] watchdog: renesas_wdt: support handover from bootloader Wolfram Sang
2019-08-18 19:00 ` Guenter Roeck
2019-08-19 16:06 ` Wolfram Sang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).