[PATCH v2] watchdog: qcom_wdt: set WDOG_HW_RUNNING bit when appropriate

[PATCH v2] watchdog: qcom_wdt: set WDOG_HW_RUNNING bit when appropriate

[Robert Marko]

If the watchdog hardware is enabled/running during boot, e.g.
due to a boot loader configuring it, we must tell the
watchdog framework about this fact so that it can ping the
watchdog until userspace opens the device and takes over
control.

Do so using the WDOG_HW_RUNNING flag that exists for exactly
that use-case.

Given the watchdog driver core doesn't know what timeout was
originally set by whoever started the watchdog (boot loader),
we make sure to update the timeout in the hardware according
to what the watchdog core thinks it is.

Signed-off-by: Robert Marko <robert.marko@sartura.hr>
Cc: Luka Perkov <luka.perkov@sartura.hr>
---
Changes in v2:
* Correct authorship

 drivers/watchdog/qcom-wdt.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
index ab7465d186fd..28c93a918e38 100644
--- a/drivers/watchdog/qcom-wdt.c
+++ b/drivers/watchdog/qcom-wdt.c
@@ -152,6 +152,13 @@ static int qcom_wdt_restart(struct watchdog_device *wdd, unsigned long action,
 	return 0;
 }
 
+static int qcom_wdt_is_running(struct watchdog_device *wdd)
+{
+	struct qcom_wdt *wdt = to_qcom_wdt(wdd);
+
+	return (readl(wdt_addr(wdt, WDT_EN)) & 1);
+}
+
 static const struct watchdog_ops qcom_wdt_ops = {
 	.start		= qcom_wdt_start,
 	.stop		= qcom_wdt_stop,
@@ -294,6 +301,21 @@ static int qcom_wdt_probe(struct platform_device *pdev)
 	wdt->wdd.timeout = min(wdt->wdd.max_timeout, 30U);
 	watchdog_init_timeout(&wdt->wdd, 0, dev);
 
+	if (qcom_wdt_is_running(&wdt->wdd)) {
+		/*
+		 * Make sure to apply timeout from watchdog core, taking
+		 * the prescaler of this driver here into account (the
+		 * boot loader might be using a different prescaler).
+		 *
+		 * To avoid spurious resets because of different scaling,
+		 * we first disable the watchdog, set the new prescaler
+		 * and timeout, and then re-enable the watchdog.
+		 */
+		qcom_wdt_stop(&wdt->wdd);
+		qcom_wdt_start(&wdt->wdd);
+		set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
+	}
+
 	ret = devm_watchdog_register_device(dev, &wdt->wdd);
 	if (ret)
 		return ret;
-- 
2.28.0

Re: [PATCH v2] watchdog: qcom_wdt: set WDOG_HW_RUNNING bit when appropriate

[kathirav]

On 2020-10-28 17:16, Robert Marko wrote:
> If the watchdog hardware is enabled/running during boot, e.g.
> due to a boot loader configuring it, we must tell the
> watchdog framework about this fact so that it can ping the
> watchdog until userspace opens the device and takes over
> control.
> 
> Do so using the WDOG_HW_RUNNING flag that exists for exactly
> that use-case.
> 
> Given the watchdog driver core doesn't know what timeout was
> originally set by whoever started the watchdog (boot loader),
> we make sure to update the timeout in the hardware according
> to what the watchdog core thinks it is.
> 
> Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> Cc: Luka Perkov <luka.perkov@sartura.hr>
> ---
> Changes in v2:
> * Correct authorship
> 
>  drivers/watchdog/qcom-wdt.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> index ab7465d186fd..28c93a918e38 100644
> --- a/drivers/watchdog/qcom-wdt.c
> +++ b/drivers/watchdog/qcom-wdt.c
> @@ -152,6 +152,13 @@ static int qcom_wdt_restart(struct
> watchdog_device *wdd, unsigned long action,
>  	return 0;
>  }
> 
> +static int qcom_wdt_is_running(struct watchdog_device *wdd)
> +{
> +	struct qcom_wdt *wdt = to_qcom_wdt(wdd);
> +
> +	return (readl(wdt_addr(wdt, WDT_EN)) & 1);

QCOM_WDT_ENABLE macro can be used instead of 1?

> +}
> +
>  static const struct watchdog_ops qcom_wdt_ops = {
>  	.start		= qcom_wdt_start,
>  	.stop		= qcom_wdt_stop,
> @@ -294,6 +301,21 @@ static int qcom_wdt_probe(struct platform_device 
> *pdev)
>  	wdt->wdd.timeout = min(wdt->wdd.max_timeout, 30U);
>  	watchdog_init_timeout(&wdt->wdd, 0, dev);
> 
> +	if (qcom_wdt_is_running(&wdt->wdd)) {
> +		/*
> +		 * Make sure to apply timeout from watchdog core, taking
> +		 * the prescaler of this driver here into account (the
> +		 * boot loader might be using a different prescaler).
> +		 *
> +		 * To avoid spurious resets because of different scaling,
> +		 * we first disable the watchdog, set the new prescaler
> +		 * and timeout, and then re-enable the watchdog.
> +		 */
> +		qcom_wdt_stop(&wdt->wdd);

qcom_wdt_start disables the WDT, configure the timeout values and 
enables it. Do we still need to call qcom_wdt_stop?

> +		qcom_wdt_start(&wdt->wdd);
> +		set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
> +	}
> +
>  	ret = devm_watchdog_register_device(dev, &wdt->wdd);
>  	if (ret)
>  		return ret;

Re: [PATCH v2] watchdog: qcom_wdt: set WDOG_HW_RUNNING bit when appropriate

[Robert Marko]

On Fri, Oct 30, 2020 at 6:21 AM <kathirav@codeaurora.org> wrote:
>
> On 2020-10-28 17:16, Robert Marko wrote:
> > If the watchdog hardware is enabled/running during boot, e.g.
> > due to a boot loader configuring it, we must tell the
> > watchdog framework about this fact so that it can ping the
> > watchdog until userspace opens the device and takes over
> > control.
> >
> > Do so using the WDOG_HW_RUNNING flag that exists for exactly
> > that use-case.
> >
> > Given the watchdog driver core doesn't know what timeout was
> > originally set by whoever started the watchdog (boot loader),
> > we make sure to update the timeout in the hardware according
> > to what the watchdog core thinks it is.
> >
> > Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> > Cc: Luka Perkov <luka.perkov@sartura.hr>
> > ---
> > Changes in v2:
> > * Correct authorship
> >
> >  drivers/watchdog/qcom-wdt.c | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> >
> > diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> > index ab7465d186fd..28c93a918e38 100644
> > --- a/drivers/watchdog/qcom-wdt.c
> > +++ b/drivers/watchdog/qcom-wdt.c
> > @@ -152,6 +152,13 @@ static int qcom_wdt_restart(struct
> > watchdog_device *wdd, unsigned long action,
> >       return 0;
> >  }
> >
> > +static int qcom_wdt_is_running(struct watchdog_device *wdd)
> > +{
> > +     struct qcom_wdt *wdt = to_qcom_wdt(wdd);
> > +
> > +     return (readl(wdt_addr(wdt, WDT_EN)) & 1);
>
> QCOM_WDT_ENABLE macro can be used instead of 1?
Yes, pushed this in v4(Forgot to do it in v3).
Thanks
>
> > +}
> > +
> >  static const struct watchdog_ops qcom_wdt_ops = {
> >       .start          = qcom_wdt_start,
> >       .stop           = qcom_wdt_stop,
> > @@ -294,6 +301,21 @@ static int qcom_wdt_probe(struct platform_device
> > *pdev)
> >       wdt->wdd.timeout = min(wdt->wdd.max_timeout, 30U);
> >       watchdog_init_timeout(&wdt->wdd, 0, dev);
> >
> > +     if (qcom_wdt_is_running(&wdt->wdd)) {
> > +             /*
> > +              * Make sure to apply timeout from watchdog core, taking
> > +              * the prescaler of this driver here into account (the
> > +              * boot loader might be using a different prescaler).
> > +              *
> > +              * To avoid spurious resets because of different scaling,
> > +              * we first disable the watchdog, set the new prescaler
> > +              * and timeout, and then re-enable the watchdog.
> > +              */
> > +             qcom_wdt_stop(&wdt->wdd);
>
> qcom_wdt_start disables the WDT, configure the timeout values and
> enables it. Do we still need to call qcom_wdt_stop?
No, as the start will actually stop the WDT and then set everything up.
Pushed in v3.
>
> > +             qcom_wdt_start(&wdt->wdd);
> > +             set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
> > +     }
> > +
> >       ret = devm_watchdog_register_device(dev, &wdt->wdd);
> >       if (ret)
> >               return ret;