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

[Steve Twiss <stwiss.opensource@diasemi.com>]

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