| Message ID | 20190818180007.2258-1-wsa@the-dreams.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | watchdog: renesas_wdt: support handover from bootloader | expand |
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; >
> 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.
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;