* [PATCH 2/3] watchdog: da9063: Use pre configured timeout when watchdog is running
2020-03-26 15:02 [PATCH 1/3] watchdog: da9062: Use pre-configured timeout until userspace takes over Stefan Riedmueller
@ 2020-03-26 15:02 ` Stefan Riedmueller
2020-03-30 16:38 ` Adam Thomson
2020-03-26 15:02 ` [PATCH 3/3] watchdog: da9062: No need to ping manually before setting timeout Stefan Riedmueller
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Stefan Riedmueller @ 2020-03-26 15:02 UTC (permalink / raw)
To: Wim Van Sebroeck, Guenter Roeck, Support Opensource
Cc: linux-watchdog, linux-kernel
If the watchdog is already running during probe use its pre-configured
timeout instead of a default timeout to make sure the watchdog is pinged
in time until userspace takes over.
Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
---
drivers/watchdog/da9063_wdt.c | 29 ++++++++++++++++++-----------
1 file changed, 18 insertions(+), 11 deletions(-)
diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
index 3d65e92a4e3f..34d0c4f03814 100644
--- a/drivers/watchdog/da9063_wdt.c
+++ b/drivers/watchdog/da9063_wdt.c
@@ -46,15 +46,16 @@ static unsigned int da9063_wdt_timeout_to_sel(unsigned int secs)
}
/*
- * Return 0 if watchdog is disabled, else non zero.
+ * Read the currently active timeout.
+ * Zero means the watchdog is disabled.
*/
-static unsigned int da9063_wdt_is_running(struct da9063 *da9063)
+static unsigned int da9063_wdt_read_timeout(struct da9063 *da9063)
{
unsigned int val;
regmap_read(da9063->regmap, DA9063_REG_CONTROL_D, &val);
- return val & DA9063_TWDSCALE_MASK;
+ return wdt_timeout[val & DA9063_TWDSCALE_MASK];
}
static int da9063_wdt_disable_timer(struct da9063 *da9063)
@@ -191,6 +192,7 @@ static int da9063_wdt_probe(struct platform_device *pdev)
struct device *dev = &pdev->dev;
struct da9063 *da9063;
struct watchdog_device *wdd;
+ int timeout;
if (!dev->parent)
return -EINVAL;
@@ -214,15 +216,20 @@ static int da9063_wdt_probe(struct platform_device *pdev)
watchdog_set_restart_priority(wdd, 128);
watchdog_set_drvdata(wdd, da9063);
- /* Set default timeout, maybe override it with DT value, scale it */
- wdd->timeout = DA9063_WDG_TIMEOUT;
- watchdog_init_timeout(wdd, 0, dev);
- da9063_wdt_set_timeout(wdd, wdd->timeout);
-
- /* Change the timeout to the default value if the watchdog is running */
- if (da9063_wdt_is_running(da9063)) {
- da9063_wdt_update_timeout(da9063, wdd->timeout);
+ /*
+ * Use pre-configured timeout if watchdog is already running.
+ * Otherwise set default timeout, maybe override it with DT value,
+ * scale it
+ */
+ timeout = da9063_wdt_read_timeout(da9063);
+ if (timeout) {
+ wdd->timeout = timeout;
set_bit(WDOG_HW_RUNNING, &wdd->status);
+ dev_info(da9063->dev, "watchdog is running (%u s)", timeout);
+ } else {
+ wdd->timeout = DA9063_WDG_TIMEOUT;
+ watchdog_init_timeout(wdd, 0, dev);
+ da9063_wdt_set_timeout(wdd, wdd->timeout);
}
return devm_watchdog_register_device(dev, wdd);
--
2.23.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* RE: [PATCH 2/3] watchdog: da9063: Use pre configured timeout when watchdog is running
2020-03-26 15:02 ` [PATCH 2/3] watchdog: da9063: Use pre configured timeout when watchdog is running Stefan Riedmueller
@ 2020-03-30 16:38 ` Adam Thomson
2020-03-31 16:08 ` Guenter Roeck
0 siblings, 1 reply; 10+ messages in thread
From: Adam Thomson @ 2020-03-30 16:38 UTC (permalink / raw)
To: Stefan Riedmueller, Wim Van Sebroeck, Guenter Roeck, Support Opensource
Cc: linux-watchdog, linux-kernel
On 26 March 2020 15:02, Stefan Riedmueller wrote:
> If the watchdog is already running during probe use its pre-configured
> timeout instead of a default timeout to make sure the watchdog is pinged
> in time until userspace takes over.
At least for this driver I don't think there's an issue here with regards to
not pinging in time. Calling 'da9063_wdt_update_timeout()', as it currently
does in the probe() when the watchdog is already active, actually disables the
watchdog before then setting a new timeout value, so by that method we're
avoiding a timeout and starting a new timer period.
To my mind the timeout value should come from DT if possible, which I would
assume for the most part would match whatever is defined in the bootloader as
well, unless I'm mistaken. If that's not available though then I would maybe
agree on falling back to a value that was already programmed in the bootloader
rather than the driver default which should be the last resort.
>
> Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
> ---
> drivers/watchdog/da9063_wdt.c | 29 ++++++++++++++++++-----------
> 1 file changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
> index 3d65e92a4e3f..34d0c4f03814 100644
> --- a/drivers/watchdog/da9063_wdt.c
> +++ b/drivers/watchdog/da9063_wdt.c
> @@ -46,15 +46,16 @@ static unsigned int da9063_wdt_timeout_to_sel(unsigned
> int secs)
> }
>
> /*
> - * Return 0 if watchdog is disabled, else non zero.
> + * Read the currently active timeout.
> + * Zero means the watchdog is disabled.
> */
> -static unsigned int da9063_wdt_is_running(struct da9063 *da9063)
> +static unsigned int da9063_wdt_read_timeout(struct da9063 *da9063)
> {
> unsigned int val;
>
> regmap_read(da9063->regmap, DA9063_REG_CONTROL_D, &val);
>
> - return val & DA9063_TWDSCALE_MASK;
> + return wdt_timeout[val & DA9063_TWDSCALE_MASK];
> }
>
> static int da9063_wdt_disable_timer(struct da9063 *da9063)
> @@ -191,6 +192,7 @@ static int da9063_wdt_probe(struct platform_device
> *pdev)
> struct device *dev = &pdev->dev;
> struct da9063 *da9063;
> struct watchdog_device *wdd;
> + int timeout;
>
> if (!dev->parent)
> return -EINVAL;
> @@ -214,15 +216,20 @@ static int da9063_wdt_probe(struct platform_device
> *pdev)
> watchdog_set_restart_priority(wdd, 128);
> watchdog_set_drvdata(wdd, da9063);
>
> - /* Set default timeout, maybe override it with DT value, scale it */
> - wdd->timeout = DA9063_WDG_TIMEOUT;
> - watchdog_init_timeout(wdd, 0, dev);
> - da9063_wdt_set_timeout(wdd, wdd->timeout);
> -
> - /* Change the timeout to the default value if the watchdog is running */
> - if (da9063_wdt_is_running(da9063)) {
> - da9063_wdt_update_timeout(da9063, wdd->timeout);
> + /*
> + * Use pre-configured timeout if watchdog is already running.
> + * Otherwise set default timeout, maybe override it with DT value,
> + * scale it
> + */
> + timeout = da9063_wdt_read_timeout(da9063);
> + if (timeout) {
> + wdd->timeout = timeout;
> set_bit(WDOG_HW_RUNNING, &wdd->status);
> + dev_info(da9063->dev, "watchdog is running (%u s)", timeout);
> + } else {
> + wdd->timeout = DA9063_WDG_TIMEOUT;
> + watchdog_init_timeout(wdd, 0, dev);
> + da9063_wdt_set_timeout(wdd, wdd->timeout);
> }
>
> return devm_watchdog_register_device(dev, wdd);
> --
> 2.23.0
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 2/3] watchdog: da9063: Use pre configured timeout when watchdog is running
2020-03-30 16:38 ` Adam Thomson
@ 2020-03-31 16:08 ` Guenter Roeck
2020-04-01 8:19 ` Stefan Riedmüller
0 siblings, 1 reply; 10+ messages in thread
From: Guenter Roeck @ 2020-03-31 16:08 UTC (permalink / raw)
To: Adam Thomson, Stefan Riedmueller, Wim Van Sebroeck, Support Opensource
Cc: linux-watchdog, linux-kernel
On 3/30/20 9:38 AM, Adam Thomson wrote:
> On 26 March 2020 15:02, Stefan Riedmueller wrote:
>
>> If the watchdog is already running during probe use its pre-configured
>> timeout instead of a default timeout to make sure the watchdog is pinged
>> in time until userspace takes over.
>
> At least for this driver I don't think there's an issue here with regards to
> not pinging in time. Calling 'da9063_wdt_update_timeout()', as it currently
> does in the probe() when the watchdog is already active, actually disables the
> watchdog before then setting a new timeout value, so by that method we're
> avoiding a timeout and starting a new timer period.
>
> To my mind the timeout value should come from DT if possible, which I would
> assume for the most part would match whatever is defined in the bootloader as
> well, unless I'm mistaken. If that's not available though then I would maybe
> agree on falling back to a value that was already programmed in the bootloader
> rather than the driver default which should be the last resort.
>
Agreed.
Guenter
>>
>> Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
>> ---
>> drivers/watchdog/da9063_wdt.c | 29 ++++++++++++++++++-----------
>> 1 file changed, 18 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
>> index 3d65e92a4e3f..34d0c4f03814 100644
>> --- a/drivers/watchdog/da9063_wdt.c
>> +++ b/drivers/watchdog/da9063_wdt.c
>> @@ -46,15 +46,16 @@ static unsigned int da9063_wdt_timeout_to_sel(unsigned
>> int secs)
>> }
>>
>> /*
>> - * Return 0 if watchdog is disabled, else non zero.
>> + * Read the currently active timeout.
>> + * Zero means the watchdog is disabled.
>> */
>> -static unsigned int da9063_wdt_is_running(struct da9063 *da9063)
>> +static unsigned int da9063_wdt_read_timeout(struct da9063 *da9063)
>> {
>> unsigned int val;
>>
>> regmap_read(da9063->regmap, DA9063_REG_CONTROL_D, &val);
>>
>> - return val & DA9063_TWDSCALE_MASK;
>> + return wdt_timeout[val & DA9063_TWDSCALE_MASK];
>> }
>>
>> static int da9063_wdt_disable_timer(struct da9063 *da9063)
>> @@ -191,6 +192,7 @@ static int da9063_wdt_probe(struct platform_device
>> *pdev)
>> struct device *dev = &pdev->dev;
>> struct da9063 *da9063;
>> struct watchdog_device *wdd;
>> + int timeout;
>>
>> if (!dev->parent)
>> return -EINVAL;
>> @@ -214,15 +216,20 @@ static int da9063_wdt_probe(struct platform_device
>> *pdev)
>> watchdog_set_restart_priority(wdd, 128);
>> watchdog_set_drvdata(wdd, da9063);
>>
>> - /* Set default timeout, maybe override it with DT value, scale it */
>> - wdd->timeout = DA9063_WDG_TIMEOUT;
>> - watchdog_init_timeout(wdd, 0, dev);
>> - da9063_wdt_set_timeout(wdd, wdd->timeout);
>> -
>> - /* Change the timeout to the default value if the watchdog is running */
>> - if (da9063_wdt_is_running(da9063)) {
>> - da9063_wdt_update_timeout(da9063, wdd->timeout);
>> + /*
>> + * Use pre-configured timeout if watchdog is already running.
>> + * Otherwise set default timeout, maybe override it with DT value,
>> + * scale it
>> + */
>> + timeout = da9063_wdt_read_timeout(da9063);
>> + if (timeout) {
>> + wdd->timeout = timeout;
>> set_bit(WDOG_HW_RUNNING, &wdd->status);
>> + dev_info(da9063->dev, "watchdog is running (%u s)", timeout);
>> + } else {
>> + wdd->timeout = DA9063_WDG_TIMEOUT;
>> + watchdog_init_timeout(wdd, 0, dev);
>> + da9063_wdt_set_timeout(wdd, wdd->timeout);
>> }
>>
>> return devm_watchdog_register_device(dev, wdd);
>> --
>> 2.23.0
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 2/3] watchdog: da9063: Use pre configured timeout when watchdog is running
2020-03-31 16:08 ` Guenter Roeck
@ 2020-04-01 8:19 ` Stefan Riedmüller
0 siblings, 0 replies; 10+ messages in thread
From: Stefan Riedmüller @ 2020-04-01 8:19 UTC (permalink / raw)
To: Guenter Roeck, Adam Thomson
Cc: Wim Van Sebroeck, Support Opensource, linux-watchdog,
linux-kernel, Stefan Riedmueller
Hi Guenter, Adam,
Guenter Roeck <linux@roeck-us.net> wrote on Tue, 31. Mar 20 09:08:
> On 3/30/20 9:38 AM, Adam Thomson wrote:
> > On 26 March 2020 15:02, Stefan Riedmueller wrote:
> >
> >> If the watchdog is already running during probe use its pre-configured
> >> timeout instead of a default timeout to make sure the watchdog is pinged
> >> in time until userspace takes over.
> >
> > At least for this driver I don't think there's an issue here with regards to
> > not pinging in time. Calling 'da9063_wdt_update_timeout()', as it currently
> > does in the probe() when the watchdog is already active, actually disables the
> > watchdog before then setting a new timeout value, so by that method we're
> > avoiding a timeout and starting a new timer period.
> >
> > To my mind the timeout value should come from DT if possible, which I would
> > assume for the most part would match whatever is defined in the bootloader as
> > well, unless I'm mistaken. If that's not available though then I would maybe
> > agree on falling back to a value that was already programmed in the bootloader
> > rather than the driver default which should be the last resort.
> >
> Agreed.
Thanks for both your feedback. I'll drop the pre-configured timeout part and
stick with the default timeout and do the same procedure (init_timeout +
update_timeout) for the da9062.
Thanks
Stefan
>
> Guenter
>
> >>
> >> Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
> >> ---
> >> drivers/watchdog/da9063_wdt.c | 29 ++++++++++++++++++-----------
> >> 1 file changed, 18 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
> >> index 3d65e92a4e3f..34d0c4f03814 100644
> >> --- a/drivers/watchdog/da9063_wdt.c
> >> +++ b/drivers/watchdog/da9063_wdt.c
> >> @@ -46,15 +46,16 @@ static unsigned int da9063_wdt_timeout_to_sel(unsigned
> >> int secs)
> >> }
> >>
> >> /*
> >> - * Return 0 if watchdog is disabled, else non zero.
> >> + * Read the currently active timeout.
> >> + * Zero means the watchdog is disabled.
> >> */
> >> -static unsigned int da9063_wdt_is_running(struct da9063 *da9063)
> >> +static unsigned int da9063_wdt_read_timeout(struct da9063 *da9063)
> >> {
> >> unsigned int val;
> >>
> >> regmap_read(da9063->regmap, DA9063_REG_CONTROL_D, &val);
> >>
> >> - return val & DA9063_TWDSCALE_MASK;
> >> + return wdt_timeout[val & DA9063_TWDSCALE_MASK];
> >> }
> >>
> >> static int da9063_wdt_disable_timer(struct da9063 *da9063)
> >> @@ -191,6 +192,7 @@ static int da9063_wdt_probe(struct platform_device
> >> *pdev)
> >> struct device *dev = &pdev->dev;
> >> struct da9063 *da9063;
> >> struct watchdog_device *wdd;
> >> + int timeout;
> >>
> >> if (!dev->parent)
> >> return -EINVAL;
> >> @@ -214,15 +216,20 @@ static int da9063_wdt_probe(struct platform_device
> >> *pdev)
> >> watchdog_set_restart_priority(wdd, 128);
> >> watchdog_set_drvdata(wdd, da9063);
> >>
> >> - /* Set default timeout, maybe override it with DT value, scale it */
> >> - wdd->timeout = DA9063_WDG_TIMEOUT;
> >> - watchdog_init_timeout(wdd, 0, dev);
> >> - da9063_wdt_set_timeout(wdd, wdd->timeout);
> >> -
> >> - /* Change the timeout to the default value if the watchdog is running */
> >> - if (da9063_wdt_is_running(da9063)) {
> >> - da9063_wdt_update_timeout(da9063, wdd->timeout);
> >> + /*
> >> + * Use pre-configured timeout if watchdog is already running.
> >> + * Otherwise set default timeout, maybe override it with DT value,
> >> + * scale it
> >> + */
> >> + timeout = da9063_wdt_read_timeout(da9063);
> >> + if (timeout) {
> >> + wdd->timeout = timeout;
> >> set_bit(WDOG_HW_RUNNING, &wdd->status);
> >> + dev_info(da9063->dev, "watchdog is running (%u s)", timeout);
> >> + } else {
> >> + wdd->timeout = DA9063_WDG_TIMEOUT;
> >> + watchdog_init_timeout(wdd, 0, dev);
> >> + da9063_wdt_set_timeout(wdd, wdd->timeout);
> >> }
> >>
> >> return devm_watchdog_register_device(dev, wdd);
> >> --
> >> 2.23.0
> >
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/3] watchdog: da9062: No need to ping manually before setting timeout
2020-03-26 15:02 [PATCH 1/3] watchdog: da9062: Use pre-configured timeout until userspace takes over Stefan Riedmueller
2020-03-26 15:02 ` [PATCH 2/3] watchdog: da9063: Use pre configured timeout when watchdog is running Stefan Riedmueller
@ 2020-03-26 15:02 ` Stefan Riedmueller
2020-03-31 16:11 ` Guenter Roeck
2020-04-01 10:20 ` Adam Thomson
2020-03-31 16:00 ` [PATCH 1/3] watchdog: da9062: Use pre-configured timeout until userspace takes over Guenter Roeck
2020-03-31 16:04 ` Guenter Roeck
3 siblings, 2 replies; 10+ messages in thread
From: Stefan Riedmueller @ 2020-03-26 15:02 UTC (permalink / raw)
To: Wim Van Sebroeck, Guenter Roeck, Support Opensource
Cc: linux-watchdog, linux-kernel
There is actually no need to ping the watchdog before disabling it
during timeout change. Disabling the watchdog already takes care of
resetting the counter.
This fixes an issue during boot when the userspace watchdog handler takes
over and the watchdog is already running. Opening the watchdog in this case
leads to the first ping and directly after that without the required
heartbeat delay a second ping issued by the set_timeout call. Due to the
missing delay this resulted in a reset.
Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
---
drivers/watchdog/da9062_wdt.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/drivers/watchdog/da9062_wdt.c b/drivers/watchdog/da9062_wdt.c
index 6d81b1276b87..c5bd075c8b14 100644
--- a/drivers/watchdog/da9062_wdt.c
+++ b/drivers/watchdog/da9062_wdt.c
@@ -66,11 +66,6 @@ static int da9062_wdt_update_timeout_register(struct da9062_watchdog *wdt,
unsigned int regval)
{
struct da9062 *chip = wdt->hw;
- int ret;
-
- ret = da9062_reset_watchdog_timer(wdt);
- if (ret)
- return ret;
regmap_update_bits(chip->regmap,
DA9062AA_CONTROL_D,
--
2.23.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 3/3] watchdog: da9062: No need to ping manually before setting timeout
2020-03-26 15:02 ` [PATCH 3/3] watchdog: da9062: No need to ping manually before setting timeout Stefan Riedmueller
@ 2020-03-31 16:11 ` Guenter Roeck
2020-04-01 10:20 ` Adam Thomson
1 sibling, 0 replies; 10+ messages in thread
From: Guenter Roeck @ 2020-03-31 16:11 UTC (permalink / raw)
To: Stefan Riedmueller, Wim Van Sebroeck, Support Opensource
Cc: linux-watchdog, linux-kernel
On 3/26/20 8:02 AM, Stefan Riedmueller wrote:
> There is actually no need to ping the watchdog before disabling it
> during timeout change. Disabling the watchdog already takes care of
> resetting the counter.
>
> This fixes an issue during boot when the userspace watchdog handler takes
> over and the watchdog is already running. Opening the watchdog in this case
> leads to the first ping and directly after that without the required
> heartbeat delay a second ping issued by the set_timeout call. Due to the
> missing delay this resulted in a reset.
>
> Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
Makes sense to me.
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Guenter
> ---
> drivers/watchdog/da9062_wdt.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/watchdog/da9062_wdt.c b/drivers/watchdog/da9062_wdt.c
> index 6d81b1276b87..c5bd075c8b14 100644
> --- a/drivers/watchdog/da9062_wdt.c
> +++ b/drivers/watchdog/da9062_wdt.c
> @@ -66,11 +66,6 @@ static int da9062_wdt_update_timeout_register(struct da9062_watchdog *wdt,
> unsigned int regval)
> {
> struct da9062 *chip = wdt->hw;
> - int ret;
> -
> - ret = da9062_reset_watchdog_timer(wdt);
> - if (ret)
> - return ret;
>
> regmap_update_bits(chip->regmap,
> DA9062AA_CONTROL_D,
>
^ permalink raw reply [flat|nested] 10+ messages in thread* RE: [PATCH 3/3] watchdog: da9062: No need to ping manually before setting timeout
2020-03-26 15:02 ` [PATCH 3/3] watchdog: da9062: No need to ping manually before setting timeout Stefan Riedmueller
2020-03-31 16:11 ` Guenter Roeck
@ 2020-04-01 10:20 ` Adam Thomson
1 sibling, 0 replies; 10+ messages in thread
From: Adam Thomson @ 2020-04-01 10:20 UTC (permalink / raw)
To: Stefan Riedmueller, Wim Van Sebroeck, Guenter Roeck, Support Opensource
Cc: linux-watchdog, linux-kernel
On 26 March 2020 15:02, Stefan Riedmueller wrote:
> There is actually no need to ping the watchdog before disabling it
> during timeout change. Disabling the watchdog already takes care of
> resetting the counter.
>
> This fixes an issue during boot when the userspace watchdog handler takes
> over and the watchdog is already running. Opening the watchdog in this case
> leads to the first ping and directly after that without the required
> heartbeat delay a second ping issued by the set_timeout call. Due to the
> missing delay this resulted in a reset.
>
> Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
Thanks for the update:
Reviewed-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
> ---
> drivers/watchdog/da9062_wdt.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/watchdog/da9062_wdt.c b/drivers/watchdog/da9062_wdt.c
> index 6d81b1276b87..c5bd075c8b14 100644
> --- a/drivers/watchdog/da9062_wdt.c
> +++ b/drivers/watchdog/da9062_wdt.c
> @@ -66,11 +66,6 @@ static int da9062_wdt_update_timeout_register(struct
> da9062_watchdog *wdt,
> unsigned int regval)
> {
> struct da9062 *chip = wdt->hw;
> - int ret;
> -
> - ret = da9062_reset_watchdog_timer(wdt);
> - if (ret)
> - return ret;
>
> regmap_update_bits(chip->regmap,
> DA9062AA_CONTROL_D,
> --
> 2.23.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] watchdog: da9062: Use pre-configured timeout until userspace takes over
2020-03-26 15:02 [PATCH 1/3] watchdog: da9062: Use pre-configured timeout until userspace takes over Stefan Riedmueller
2020-03-26 15:02 ` [PATCH 2/3] watchdog: da9063: Use pre configured timeout when watchdog is running Stefan Riedmueller
2020-03-26 15:02 ` [PATCH 3/3] watchdog: da9062: No need to ping manually before setting timeout Stefan Riedmueller
@ 2020-03-31 16:00 ` Guenter Roeck
2020-03-31 16:04 ` Guenter Roeck
3 siblings, 0 replies; 10+ messages in thread
From: Guenter Roeck @ 2020-03-31 16:00 UTC (permalink / raw)
To: Stefan Riedmueller
Cc: Wim Van Sebroeck, Support Opensource, linux-watchdog, linux-kernel
On Thu, Mar 26, 2020 at 04:02:01PM +0100, Stefan Riedmueller wrote:
> If the watchdog is already running during probe read back its
> pre-configured timeout (set e.g. by the bootloader) and use it to ping
> the watchdog until userspace takes over. Otherwise the default timeout
> set before might not result in a fast enough ping.
>
> Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
> ---
> drivers/watchdog/da9062_wdt.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/watchdog/da9062_wdt.c b/drivers/watchdog/da9062_wdt.c
> index 0ad15d55071c..6d81b1276b87 100644
> --- a/drivers/watchdog/da9062_wdt.c
> +++ b/drivers/watchdog/da9062_wdt.c
> @@ -35,6 +35,14 @@ struct da9062_watchdog {
> bool use_sw_pm;
> };
>
> +static unsigned int da9062_wdt_read_timeout(struct da9062_watchdog *wdt)
> +{
> + int val;
> +
> + regmap_read(wdt->hw->regmap, DA9062AA_CONTROL_D, &val);
> + return wdt_timeout[val & DA9062AA_TWDSCALE_MASK];
> +}
> +
> static unsigned int da9062_wdt_timeout_to_sel(unsigned int secs)
> {
> unsigned int i;
> @@ -184,6 +192,7 @@ static int da9062_wdt_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> int ret;
> + int timeout;
> struct da9062 *chip;
> struct da9062_watchdog *wdt;
>
> @@ -213,6 +222,13 @@ static int da9062_wdt_probe(struct platform_device *pdev)
> watchdog_set_drvdata(&wdt->wdtdev, wdt);
> dev_set_drvdata(dev, &wdt->wdtdev);
>
> + timeout = da9062_wdt_read_timeout(wdt);
> + if (timeout > 0) {
> + wdt->wdtdev.timeout = timeout;
> + set_bit(WDOG_HW_RUNNING, &wdt->wdtdev.status);
> + dev_info(wdt->hw->dev, "watchdog is running (%u s)", timeout);
The user won't know what "(%u s)" means.e that "%u" reflects the timeout.
Also, the newline at the end is missing.
Personally, I think the message is just noise and should be dropped entirely.
Guenter
> + }
> +
> ret = devm_watchdog_register_device(dev, &wdt->wdtdev);
> if (ret < 0)
> return ret;
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/3] watchdog: da9062: Use pre-configured timeout until userspace takes over
2020-03-26 15:02 [PATCH 1/3] watchdog: da9062: Use pre-configured timeout until userspace takes over Stefan Riedmueller
` (2 preceding siblings ...)
2020-03-31 16:00 ` [PATCH 1/3] watchdog: da9062: Use pre-configured timeout until userspace takes over Guenter Roeck
@ 2020-03-31 16:04 ` Guenter Roeck
3 siblings, 0 replies; 10+ messages in thread
From: Guenter Roeck @ 2020-03-31 16:04 UTC (permalink / raw)
To: Stefan Riedmueller
Cc: Wim Van Sebroeck, Support Opensource, linux-watchdog, linux-kernel
On Thu, Mar 26, 2020 at 04:02:01PM +0100, Stefan Riedmueller wrote:
> If the watchdog is already running during probe read back its
> pre-configured timeout (set e.g. by the bootloader) and use it to ping
> the watchdog until userspace takes over. Otherwise the default timeout
> set before might not result in a fast enough ping.
>
> Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
> ---
> drivers/watchdog/da9062_wdt.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/watchdog/da9062_wdt.c b/drivers/watchdog/da9062_wdt.c
> index 0ad15d55071c..6d81b1276b87 100644
> --- a/drivers/watchdog/da9062_wdt.c
> +++ b/drivers/watchdog/da9062_wdt.c
> @@ -35,6 +35,14 @@ struct da9062_watchdog {
> bool use_sw_pm;
> };
>
> +static unsigned int da9062_wdt_read_timeout(struct da9062_watchdog *wdt)
> +{
> + int val;
> +
> + regmap_read(wdt->hw->regmap, DA9062AA_CONTROL_D, &val);
> + return wdt_timeout[val & DA9062AA_TWDSCALE_MASK];
> +}
> +
> static unsigned int da9062_wdt_timeout_to_sel(unsigned int secs)
> {
> unsigned int i;
> @@ -184,6 +192,7 @@ static int da9062_wdt_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> int ret;
> + int timeout;
> struct da9062 *chip;
> struct da9062_watchdog *wdt;
>
> @@ -213,6 +222,13 @@ static int da9062_wdt_probe(struct platform_device *pdev)
> watchdog_set_drvdata(&wdt->wdtdev, wdt);
> dev_set_drvdata(dev, &wdt->wdtdev);
>
> + timeout = da9062_wdt_read_timeout(wdt);
> + if (timeout > 0) {
> + wdt->wdtdev.timeout = timeout;
> + set_bit(WDOG_HW_RUNNING, &wdt->wdtdev.status);
> + dev_info(wdt->hw->dev, "watchdog is running (%u s)", timeout);
> + }
Oh, and I agree with the comment made for the da9063 driver: This driver
should really implement watchdog_init_timeout() and set the default timeout
accordingly if specified in dt (or, in other words, follow the da9063
implementation).
Guenter
> +
> ret = devm_watchdog_register_device(dev, &wdt->wdtdev);
> if (ret < 0)
> return ret;
^ permalink raw reply [flat|nested] 10+ messages in thread