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 <[email protected]>
Cc: Luka Perkov <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
> Cc: Luka Perkov <[email protected]>
> ---
> 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;
>
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 <[email protected]>
> Reviewed-by: Guenter Roeck <[email protected]>
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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
>> Reviewed-by: Guenter Roeck <[email protected]>
>
> 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
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 <[email protected]>
>>> Reviewed-by: Guenter Roeck <[email protected]>
>> 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
On Mon, Nov 2, 2020 at 6:19 AM Kathiravan T <[email protected]> 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 <[email protected]>
> >>> Reviewed-by: Guenter Roeck <[email protected]>
> >> 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
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 <[email protected]>
> Cc: Luka Perkov <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
> ---
> 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
>
On Mon, Nov 30, 2020 at 07:27:00PM +0100, Robert Marko wrote:
> On Mon, Nov 2, 2020 at 6:19 AM Kathiravan T <[email protected]> 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 <[email protected]>
> > >>> Reviewed-by: Guenter Roeck <[email protected]>
> > >> 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