| Message ID | 20201031121115.542752-1-robert.marko@sartura.hr (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v4] watchdog: qcom_wdt: set WDOG_HW_RUNNING bit when appropriate | expand |
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; >
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; >>
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
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.
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
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 >
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
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;
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(+)