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

[PATCH v4] 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.

Signed-off-by: Robert Marko <robert.marko@sartura.hr>
Cc: Luka Perkov <luka.perkov@sartura.hr>
---
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;
-- 
2.28.0

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

[Guenter Roeck]

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@sartura.hr>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> Cc: Luka Perkov <luka.perkov@sartura.hr>
> ---
> 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;
> 

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@sartura.hr>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>

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@sartura.hr>
>> ---
>> 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

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

[Guenter Roeck]

On 11/1/20 7:58 PM, Kathiravan T wrote:
> 
> 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@sartura.hr>
>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> 
> 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?
> 

That would mean to replace module_platform_driver(), which would be a whole
different discussion, is not widely needed, and would potentially interfere
with the subsys_initcall_sync() in the watchdog core. This will require
specific evidence that a problem is seen in the field, and that it is truly
needed. Plus, it would have to be a different patch (which you could submit
yourself, with evidence). Let's stick with one logical change per patch,
please.

Guenter

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

[Kathiravan T]

On 11/2/2020 10:33 AM, Guenter Roeck wrote:
> On 11/1/20 7:58 PM, Kathiravan T wrote:
>> 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@sartura.hr>
>>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>> 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?
>>
> That would mean to replace module_platform_driver(), which would be a whole
> different discussion, is not widely needed, and would potentially interfere
> with the subsys_initcall_sync() in the watchdog core. This will require
> specific evidence that a problem is seen in the field, and that it is truly
> needed. Plus, it would have to be a different patch (which you could submit
> yourself, with evidence). Let's stick with one logical change per patch,
> please.
>
> Guenter
Yeah, of course I don't want to squash the initcall level change with 
this one. Just made a suggestion to consider it. Anyway I will try to 
collect some data and post the patch by own on that suggestion. Thanks 
Guenter.

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

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

[Robert Marko]

On Mon, Nov 2, 2020 at 6:19 AM Kathiravan T <kathirav@codeaurora.org> wrote:
>
>
> On 11/2/2020 10:33 AM, Guenter Roeck wrote:
> > On 11/1/20 7:58 PM, Kathiravan T wrote:
> >> 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@sartura.hr>
> >>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> >> 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?
> >>
> > That would mean to replace module_platform_driver(), which would be a whole
> > different discussion, is not widely needed, and would potentially interfere
> > with the subsys_initcall_sync() in the watchdog core. This will require
> > specific evidence that a problem is seen in the field, and that it is truly
> > needed. Plus, it would have to be a different patch (which you could submit
> > yourself, with evidence). Let's stick with one logical change per patch,
> > please.
> >
> > Guenter
> Yeah, of course I don't want to squash the initcall level change with
> this one. Just made a suggestion to consider it. Anyway I will try to
> collect some data and post the patch by own on that suggestion. Thanks
> Guenter.
>
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
>

Any chance this could be picked for 5.11?
I have some boards depending on it for normal boot.

Regards,
Robert

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

[Bjorn Andersson]

On Sat 31 Oct 07:11 CDT 2020, 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@sartura.hr>
> Cc: Luka Perkov <luka.perkov@sartura.hr>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

> ---
> 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);

Although, you don't need the outer () here.

Regards,
Bjorn

> +}
> +
>  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;
> -- 
> 2.28.0
> 

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

[Guenter Roeck]

On Mon, Nov 30, 2020 at 07:27:00PM +0100, Robert Marko wrote:
> On Mon, Nov 2, 2020 at 6:19 AM Kathiravan T <kathirav@codeaurora.org> wrote:
> >
> >
> > On 11/2/2020 10:33 AM, Guenter Roeck wrote:
> > > On 11/1/20 7:58 PM, Kathiravan T wrote:
> > >> 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@sartura.hr>
> > >>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> > >> 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?
> > >>
> > > That would mean to replace module_platform_driver(), which would be a whole
> > > different discussion, is not widely needed, and would potentially interfere
> > > with the subsys_initcall_sync() in the watchdog core. This will require
> > > specific evidence that a problem is seen in the field, and that it is truly
> > > needed. Plus, it would have to be a different patch (which you could submit
> > > yourself, with evidence). Let's stick with one logical change per patch,
> > > please.
> > >
> > > Guenter
> > Yeah, of course I don't want to squash the initcall level change with
> > this one. Just made a suggestion to consider it. Anyway I will try to
> > collect some data and post the patch by own on that suggestion. Thanks
> > Guenter.
> >
> > --
> > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
> >
> 
> Any chance this could be picked for 5.11?
> I have some boards depending on it for normal boot.
> 
It is queued in my watchdog-next branch, so Wim will most likely pick it up.

Guenter