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

[Kathiravan T]

On 10/31/2020 7:38 PM, Guenter Roeck wrote:
> On 10/31/20 5:11 AM, 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.
> >
> > Signed-off-by: Robert Marko <robert.marko@xxxxxxxxxx>
> Reviewed-by: Guenter Roeck <linux@xxxxxxxxxxxx>

Thanks for addressing the comments and now the patch looks good to me. 
One more suggestion, can we make the initcall level of the driver to 
subsys_initcall_sync so that the driver gets registered immediately 
after the watchdog_core is registered and watchdog_core starts pinging 
the WDT?

> > Cc: Luka Perkov <luka.perkov@xxxxxxxxxx>
> > ---
> > Changes in v4:
> > * Use QCOM_WDT_ENABLE macro
> >
> > Changes in v3:
> > * Drop call to stop as start already does it
> > * Update commit message
> >
> > Changes in v2:
> > * Correct authorship
> >
> >   drivers/watchdog/qcom-wdt.c | 18 ++++++++++++++++++
> >   1 file changed, 18 insertions(+)
> >
> > diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> > index ab7465d186fd..07d399c4edc4 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)) & QCOM_WDT_ENABLE);
> > +}
> > +
> >   static const struct watchdog_ops qcom_wdt_ops = {
> >   	.start		= qcom_wdt_start,
> >   	.stop		= qcom_wdt_stop,
> > @@ -294,6 +301,17 @@ 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 WDT is already running, call WDT start which
> > +	 * will stop the WDT, set timeouts as bootloader
> > +	 * might use different ones and set running bit
> > +	 * to inform the WDT subsystem to ping the WDT
> > +	 */
> > +	if (qcom_wdt_is_running(&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;
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation