[PATCH v2] watchdog: da9063: Fix setting/changing timeout

[PATCH v2] watchdog: da9063: Fix setting/changing timeout

[Marco Felsch]

The DA9063 watchdog always resets the system when it changes the timeout
value after the bootloader (e.g. Barebox) has it already set.

To update the timeout value we have to disable the watchdog, clear the
watchdog counter value and write the new timeout value to the watchdog.
Clearing the counter value is a feature to be on the safe side, because the
data sheet doesn't describe the behaviour of the watchdog counter value
after a watchdog disabling-enable-sequence.

The patch is based on Philipp Zabel's <p.zabel@pengutronix.de> previous
patch but doesn't wait 150us because the DA9063 doesn't need this delay.

Fixes: 5e9c16e37608 ("watchdog: Add DA9063 PMIC watchdog driver.")
Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/watchdog/da9063_wdt.c | 33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
index b17ac1bb1f28..bca247d44737 100644
--- a/drivers/watchdog/da9063_wdt.c
+++ b/drivers/watchdog/da9063_wdt.c
@@ -45,8 +45,38 @@ static unsigned int da9063_wdt_timeout_to_sel(unsigned int secs)
 	return DA9063_TWDSCALE_MAX;
 }
 
+/*
+ * Writing a '1' to the self-clearing WATCHDOG bit resets the watchdog counter
+ * value.
+ */
+static int _da9063_wdt_reset_timer(struct da9063 *da9063)
+{
+	return regmap_write(da9063->regmap, DA9063_REG_CONTROL_F,
+			    DA9063_WATCHDOG);
+}
+
 static int _da9063_wdt_set_timeout(struct da9063 *da9063, unsigned int regval)
 {
+	int ret;
+
+	/*
+	 * The watchdog trigger a reboot if a timeout value is already
+	 * programmed. Because the timeout value combines two functions in
+	 * one: indicating the counter limit and starting the watchdog. To be
+	 * able to set the watchdog a second time (first time was done by the
+	 * bootloader) disable the watchdog clear the counter value manually and
+	 * set the new timeout value.
+	 */
+	ret = regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D,
+				 DA9063_TWDSCALE_MASK, DA9063_TWDSCALE_DISABLE);
+	if (ret)
+		dev_warn(da9063->dev,
+			 "Failed to disable watchdog before setting new timeout\n");
+
+	ret = _da9063_wdt_reset_timer(da9063);
+	if (ret)
+		dev_warn(da9063->dev, "Failed to reset watchdog counter\n");
+
 	return regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D,
 				  DA9063_TWDSCALE_MASK, regval);
 }
@@ -85,8 +115,7 @@ static int da9063_wdt_ping(struct watchdog_device *wdd)
 	struct da9063 *da9063 = watchdog_get_drvdata(wdd);
 	int ret;
 
-	ret = regmap_write(da9063->regmap, DA9063_REG_CONTROL_F,
-			   DA9063_WATCHDOG);
+	ret = _da9063_wdt_reset_timer(da9063);
 	if (ret)
 		dev_alert(da9063->dev, "Failed to ping the watchdog (err = %d)\n",
 			  ret);
-- 
2.17.0

RE: [PATCH v2] watchdog: da9063: Fix setting/changing timeout

[Steve Twiss]

Hi Marco,

On 09 May 2018 13:33, Marco Felsch wrote:

> To: wim@linux-watchdog.org; linux@roeck-us.net
> Subject: [PATCH v2] watchdog: da9063: Fix setting/changing timeout
> 
> The DA9063 watchdog always resets the system when it changes the timeout
> value after the bootloader (e.g. Barebox) has it already set.
> 
> To update the timeout value we have to disable the watchdog, clear the
> watchdog counter value and write the new timeout value to the watchdog.
> Clearing the counter value is a feature to be on the safe side, because the
> data sheet doesn't describe the behaviour of the watchdog counter value
> after a watchdog disabling-enable-sequence.
> 
> The patch is based on Philipp Zabel's <p.zabel@pengutronix.de> previous
> patch but doesn't wait 150us because the DA9063 doesn't need this delay.

https://www.dialog-semiconductor.com/products/DA9063

Yes, according to the Dialog datasheet DA9063_2v1, there is no 150us minimum
assert time limit. But ... that doesn't seem correct to me, because the DA9062
driver and DA9062 datasheet both show a minimum assertion time, like you said.

https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/tree/drivers/watchdog/da9062_wdt.c?h=v4.17-rc3#n67

So let me check with the hardware engineers.

Regards,
Steve

> 
> Fixes: 5e9c16e37608 ("watchdog: Add DA9063 PMIC watchdog driver.")
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/watchdog/da9063_wdt.c | 33
> +++++++++++++++++++++++++++++++--
>  1 file changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/da9063_wdt.c
> b/drivers/watchdog/da9063_wdt.c
> index b17ac1bb1f28..bca247d44737 100644
> --- a/drivers/watchdog/da9063_wdt.c
> +++ b/drivers/watchdog/da9063_wdt.c
> @@ -45,8 +45,38 @@ static unsigned int
> da9063_wdt_timeout_to_sel(unsigned int secs)
>  	return DA9063_TWDSCALE_MAX;
>  }
> 
> +/*
> + * Writing a '1' to the self-clearing WATCHDOG bit resets the watchdog counter
> + * value.
> + */
> +static int _da9063_wdt_reset_timer(struct da9063 *da9063)
> +{
> +	return regmap_write(da9063->regmap, DA9063_REG_CONTROL_F,
> +			    DA9063_WATCHDOG);
> +}
> +
>  static int _da9063_wdt_set_timeout(struct da9063 *da9063, unsigned int regval)
>  {
> +	int ret;
> +
> +	/*
> +	 * The watchdog trigger a reboot if a timeout value is already
> +	 * programmed. Because the timeout value combines two functions in
> +	 * one: indicating the counter limit and starting the watchdog. To be
> +	 * able to set the watchdog a second time (first time was done by the
> +	 * bootloader) disable the watchdog clear the counter value manually and
> +	 * set the new timeout value.
> +	 */
> +	ret = regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D,
> +				 DA9063_TWDSCALE_MASK, DA9063_TWDSCALE_DISABLE);
> +	if (ret)
> +		dev_warn(da9063->dev,
> +			 "Failed to disable watchdog before setting new timeout\n");
> +
> +	ret = _da9063_wdt_reset_timer(da9063);
> +	if (ret)
> +		dev_warn(da9063->dev, "Failed to reset watchdog counter\n");
> +
>  	return regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D,
>  				  DA9063_TWDSCALE_MASK, regval);
>  }
> @@ -85,8 +115,7 @@ static int da9063_wdt_ping(struct watchdog_device *wdd)
>  	struct da9063 *da9063 = watchdog_get_drvdata(wdd);
>  	int ret;
> 
> -	ret = regmap_write(da9063->regmap, DA9063_REG_CONTROL_F,
> -			   DA9063_WATCHDOG);
> +	ret = _da9063_wdt_reset_timer(da9063);
>  	if (ret)
>  		dev_alert(da9063->dev, "Failed to ping the watchdog (err = %d)\n",
>  			  ret);
> --
> 2.17.0

Re: [PATCH v2] watchdog: da9063: Fix setting/changing timeout

[Marco Felsch]

[-- Attachment #1: Type: text/plain, Size: 2690 bytes --]

Hi Steve,

On 05/09/2018 03:50 PM, Steve Twiss wrote:
> Hi Marco,
>
> On 09 May 2018 13:33, Marco Felsch wrote:
>
>> To: wim@linux-watchdog.org; linux@roeck-us.net
>> Subject: [PATCH v2] watchdog: da9063: Fix setting/changing timeout
>>
>> The DA9063 watchdog always resets the system when it changes the timeout
>> value after the bootloader (e.g. Barebox) has it already set.
>>
>> To update the timeout value we have to disable the watchdog, clear the
>> watchdog counter value and write the new timeout value to the watchdog.
>> Clearing the counter value is a feature to be on the safe side, 
>> because the
>> data sheet doesn't describe the behaviour of the watchdog counter value
>> after a watchdog disabling-enable-sequence.
>>
>> The patch is based on Philipp Zabel's <p.zabel@pengutronix.de>previous
>> patch but doesn't wait 150us because the DA9063 doesn't need this delay.
> https://www.dialog-semiconductor.com/products/DA9063
>
> Yes, according to the Dialog datasheet DA9063_2v1, there is no 150us 
> minimum
> assert time limit. But ... that doesn't seem correct to me, because 
> the DA9062
> driver and DA9062 datasheet both show a minimum assertion time, like 
> you said.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/tree/drivers/watchdog/da9062_wdt.c?h=v4.17-rc3#n67
>
> So let me check with the hardware engineers.

That would be great looking forward to hear from you. I will prepare a 
v3 if it is.

>> +
>>   static int _da9063_wdt_set_timeout(struct da9063 *da9063, unsigned 
>> int regval)
>>   {
>> +    int ret;
>> +
>> +    /*
>> +     * The watchdog trigger a reboot if a timeout value is already
>> +     * programmed. Because the timeout value combines two functions in
>> +     * one: indicating the counter limit and starting the watchdog. 
>> To be
>> +     * able to set the watchdog a second time (first time was done 
>> by the
>> +     * bootloader) disable the watchdog clear the counter value 
>> manually and
>> +     * set the new timeout value.
>> +     */
>> +    ret = regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D,
>> +                 DA9063_TWDSCALE_MASK, DA9063_TWDSCALE_DISABLE);
>> +    if (ret)
>> +        dev_warn(da9063->dev,
>> +             "Failed to disable watchdog before setting new 
>> timeout\n");
>> +
>> +    ret = _da9063_wdt_reset_timer(da9063);
>> +    if (ret)
>> +        dev_warn(da9063->dev, "Failed to reset watchdog counter\n");

BTW. can you ask them if it is necessary to reset the counter value 
register manually or if this is done by disabling the watchdog.

Regards,
Marco

[-- Attachment #2: Type: text/html, Size: 5632 bytes --]

Re: [PATCH v2] watchdog: da9063: Fix setting/changing timeout

[Guenter Roeck]

On Wed, May 09, 2018 at 04:10:15PM +0200, Marco Felsch wrote:
> Hi Steve,
> 
> On 05/09/2018 03:50 PM, Steve Twiss wrote:
> >Hi Marco,
> >
> >On 09 May 2018 13:33, Marco Felsch wrote:
> >
> >>To: wim@linux-watchdog.org; linux@roeck-us.net
> >>Subject: [PATCH v2] watchdog: da9063: Fix setting/changing timeout
> >>
> >>The DA9063 watchdog always resets the system when it changes the timeout
> >>value after the bootloader (e.g. Barebox) has it already set.
> >>
> >>To update the timeout value we have to disable the watchdog, clear the
> >>watchdog counter value and write the new timeout value to the watchdog.
> >>Clearing the counter value is a feature to be on the safe side, because
> >>the
> >>data sheet doesn't describe the behaviour of the watchdog counter value
> >>after a watchdog disabling-enable-sequence.
> >>
> >>The patch is based on Philipp Zabel's <p.zabel@pengutronix.de>previous
> >>patch but doesn't wait 150us because the DA9063 doesn't need this delay.
> >https://www.dialog-semiconductor.com/products/DA9063
> >
> >Yes, according to the Dialog datasheet DA9063_2v1, there is no 150us
> >minimum
> >assert time limit. But ... that doesn't seem correct to me, because the
> >DA9062
> >driver and DA9062 datasheet both show a minimum assertion time, like you
> >said.
> >
> >https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/tree/drivers/watchdog/da9062_wdt.c?h=v4.17-rc3#n67
> >
> >So let me check with the hardware engineers.
> 
> That would be great looking forward to hear from you. I will prepare a v3 if
> it is.
> 
Any updates ?

Guenter

> >>+
> >>  static int _da9063_wdt_set_timeout(struct da9063 *da9063, unsigned int
> >>regval)
> >>  {
> >>+    int ret;
> >>+
> >>+    /*
> >>+     * The watchdog trigger a reboot if a timeout value is already
> >>+     * programmed. Because the timeout value combines two functions in
> >>+     * one: indicating the counter limit and starting the watchdog. To
> >>be
> >>+     * able to set the watchdog a second time (first time was done by
> >>the
> >>+     * bootloader) disable the watchdog clear the counter value
> >>manually and
> >>+     * set the new timeout value.
> >>+     */
> >>+    ret = regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D,
> >>+                 DA9063_TWDSCALE_MASK, DA9063_TWDSCALE_DISABLE);
> >>+    if (ret)
> >>+        dev_warn(da9063->dev,
> >>+             "Failed to disable watchdog before setting new
> >>timeout\n");
> >>+
> >>+    ret = _da9063_wdt_reset_timer(da9063);
> >>+    if (ret)
> >>+        dev_warn(da9063->dev, "Failed to reset watchdog counter\n");
> 
> BTW. can you ask them if it is necessary to reset the counter value register
> manually or if this is done by disabling the watchdog.
> 
> Regards,
> Marco

RE: [PATCH v2] watchdog: da9063: Fix setting/changing timeout

[Steve Twiss]

On 15 May 2018 19:44, Guenter Roeck wrote,

> Subject: Re: [PATCH v2] watchdog: da9063: Fix setting/changing timeout
> 
> On Wed, May 09, 2018 at 04:10:15PM +0200, Marco Felsch wrote:
> > Hi Steve,
> >
> > On 05/09/2018 03:50 PM, Steve Twiss wrote:
> > >On 09 May 2018 13:33, Marco Felsch wrote:
> > >
> > >>
> > >>The DA9063 watchdog always resets the system when it changes the timeout
> > >>value after the bootloader (e.g. Barebox) has it already set.
> > >>
> > >>To update the timeout value we have to disable the watchdog, clear the
> > >>watchdog counter value and write the new timeout value to the watchdog.
> > >>Clearing the counter value is a feature to be on the safe side, because the
> > >>data sheet doesn't describe the behaviour of the watchdog counter value
> > >>after a watchdog disabling-enable-sequence.
> > >>
> > >>The patch is based on Philipp Zabel's <p.zabel@pengutronix.de>previous
> > >>patch but doesn't wait 150us because the DA9063 doesn't need this delay.
> > >>https://www.dialog-semiconductor.com/products/DA9063
> > >
> > >Yes, according to the Dialog datasheet DA9063_2v1, there is no 150us minimum
> > >assert time limit. But ... that doesn't seem correct to me, because the DA9062
> > >driver and DA9062 datasheet both show a minimum assertion time, like you
> > >said.
> > >
> > >https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/tree/drivers/watchdog/da9062_wdt.c?h=v4.17-rc3#n67
> > >
> > >So let me check with the hardware engineers.
> >
> > That would be great looking forward to hear from you. I will prepare a v3 if it is.
> >
> Any updates ?
> 
> Guenter

Hi Guenter and Marco,

Thank you for your patience.
The best advice I can give at the moment is:

Please follow what has been done in the DA9062 and DA9053 device drivers.
https://elixir.bootlin.com/linux/latest/source/drivers/watchdog/da9062_wdt.c#L90
https://elixir.bootlin.com/linux/latest/source/drivers/watchdog/da9052_wdt.c#L76

In these cases:
"setting TWDSCALE to zero for at least 150 us before writing the new value"

There will be a longer answer, but that must wait until the formal datasheets have
been clarified by the hardware engineers.

Regards,
Steve

> 
> > >>+
> > >>  static int _da9063_wdt_set_timeout(struct da9063 *da9063, unsigned int regval)
> > >>  {
> > >>+    int ret;
> > >>+
> > >>+    /*
> > >>+     * The watchdog trigger a reboot if a timeout value is already
> > >>+     * programmed. Because the timeout value combines two functions in
> > >>+     * one: indicating the counter limit and starting the watchdog. To be
> > >>+     * able to set the watchdog a second time (first time was done by the
> > >>+     * bootloader) disable the watchdog clear the counter value manually and
> > >>+     * set the new timeout value.
> > >>+     */
> > >>+    ret = regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D,
> > >>+                 DA9063_TWDSCALE_MASK, DA9063_TWDSCALE_DISABLE);
> > >>+    if (ret)
> > >>+        dev_warn(da9063->dev,
> > >>+             "Failed to disable watchdog before setting new timeout\n");
> > >>+
> > >>+    ret = _da9063_wdt_reset_timer(da9063);
> > >>+    if (ret)
> > >>+        dev_warn(da9063->dev, "Failed to reset watchdog counter\n");
> >
> > BTW. can you ask them if it is necessary to reset the counter value register
> > manually or if this is done by disabling the watchdog.
> >
> > Regards,
> > Marco

Re: [PATCH v2] watchdog: da9063: Fix setting/changing timeout

[Marco Felsch]

Hi Steve and Guenter,

On Wed, May 16, 2018 at 07:35:13AM +0000, Steve Twiss wrote:
> On 15 May 2018 19:44, Guenter Roeck wrote,
> 
> > Subject: Re: [PATCH v2] watchdog: da9063: Fix setting/changing timeout
> > 
> > On Wed, May 09, 2018 at 04:10:15PM +0200, Marco Felsch wrote:
> > > Hi Steve,
> > >
> > >
> > > That would be great looking forward to hear from you. I will prepare a v3 if it is.
> > >
> > Any updates ?
> > 
> > Guenter
> 
> Hi Guenter and Marco,
> 
> Thank you for your patience.
> The best advice I can give at the moment is:
> 
> Please follow what has been done in the DA9062 and DA9053 device drivers.
> https://elixir.bootlin.com/linux/latest/source/drivers/watchdog/da9062_wdt.c#L90
> https://elixir.bootlin.com/linux/latest/source/drivers/watchdog/da9052_wdt.c#L76
> 
> In these cases:
> "setting TWDSCALE to zero for at least 150 us before writing the new value"

Okay. I tested it without a delay and saw no issues but I will add the
delay to be on the safe side.
Is that okay for you Guenter?

> There will be a longer answer, but that must wait until the formal datasheets have
> been clarified by the hardware engineers.
> 
> Regards,
> Steve
> 

Regards,
Marco

Re: [PATCH v2] watchdog: da9063: Fix setting/changing timeout

[Guenter Roeck]

On 05/16/2018 02:01 AM, Marco Felsch wrote:
> Hi Steve and Guenter,
>
> On Wed, May 16, 2018 at 07:35:13AM +0000, Steve Twiss wrote:
>> On 15 May 2018 19:44, Guenter Roeck wrote,
>>
>>> Subject: Re: [PATCH v2] watchdog: da9063: Fix setting/changing timeout
>>>
>>> On Wed, May 09, 2018 at 04:10:15PM +0200, Marco Felsch wrote:
>>>> Hi Steve,
>>>>
>>>>
>>>> That would be great looking forward to hear from you. I will prepare a v3 if it is.
>>>>
>>> Any updates ?
>>>
>>> Guenter
>> Hi Guenter and Marco,
>>
>> Thank you for your patience.
>> The best advice I can give at the moment is:
>>
>> Please follow what has been done in the DA9062 and DA9053 device drivers.
>> https://elixir.bootlin.com/linux/latest/source/drivers/watchdog/da9062_wdt.c#L90
>> https://elixir.bootlin.com/linux/latest/source/drivers/watchdog/da9052_wdt.c#L76
>>
>> In these cases:
>> "setting TWDSCALE to zero for at least 150 us before writing the new value"
> Okay. I tested it without a delay and saw no issues but I will add the
> delay to be on the safe side.
> Is that okay for you Guenter?

Yes. Better safe than sorry.

Thanks,
Guenter

>> There will be a longer answer, but that must wait until the formal datasheets have
>> been clarified by the hardware engineers.
>>
>> Regards,
>> Steve
>>
> Regards,
> Marco
>