[PATCH v9 0/3] watchdog: da9063: Fix timeout handling

[PATCH v9 0/3] watchdog: da9063: Fix timeout handling

[Marco Felsch]

The current set_timeout() handling can't change the timeout value if the
watchdog is already running because the watchdog must be
disabled before a new timeout value can be set. This sounds a bit
strange but it is the only way to change the timeout value.

The watchdog also has a second quirk. The timeout value combines the
timeout and the watchdog enable signal. So a value not equal zero
indicates that the watchdog is running. This leads into problems if the
watchdog is disabled and a new timeout value should be set because
during setting the timeout value the watchdog gets enabled. The driver
must check if the watchdog is disabled. In that case the timeout value
must be stored to be available for the later wdt_start() call. If the
watchdog is already running the driver must write the new timeout value
immediately to the watchdog.

There was also a issue during the driver probe sequence. The driver must
set a new timeout reference mark if the watchdog was enabled in a earlier
stage e.g. the bootloader.

Changelog:

v9:
 - Set the default driver timeout during probe() if the watchdog is
   already running.
 - drop renaming _da9063_wdt_set_timeout [v6]

v8:
 - make use of watchdog_active() helper instead of watchdog_hw_running()
 - fix misspellings
 - drop set HW_RUNNING flag during wdt_start() introduced with v6
 - call _da9063_wdt_set_timeout() during probe() directly if watchdog is
   running.
 - da9063_wdt_is_running() returns now the raw timeout regval instead of
   the maped timeout in seconds.
 - fix da9063_wdt_is_running() return value, use unsigned int instead of
   int.

v7:
 - reorder patch stack "Fixes:" patches first.

v6:
 - fix timeout value buffering (don't buffer it twice, use HW_RUNNING
   flag)
 - set HW_RUNNING flag during wdt_start()
 - add watchdog enable check during probe()
 - rename _da9063_wdt_set_timeout -> da9063_wdt_update_timeout

v5:
 - add buffering the timeout if the watchdog is disabled
 - add more comments

v4:
 - Fix patch description

v3:
 - add delay between disable watchdog and write new timeout value during
   timeout update
 - fix comment to be more common

v2:
 - style changes

v1:
 - initial commit

Marco Felsch (3):
  watchdog: da9063: Fix setting/changing timeout
  watchdog: da9063: Fix updating timeout value
  watchdog: da9063: Fix timeout handling during probe

 drivers/watchdog/da9063_wdt.c | 64 ++++++++++++++++++++++++++++++++---
 1 file changed, 60 insertions(+), 4 deletions(-)

-- 
2.17.0

[PATCH v9 1/3] watchdog: da9063: Fix setting/changing timeout

[Marco Felsch]

If the timeout value is set more than once the DA9063 watchdog triggers
a reset signal which reset the system.

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 previous patch.

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

diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
index b17ac1bb1f28..c1216e61e64e 100644
--- a/drivers/watchdog/da9063_wdt.c
+++ b/drivers/watchdog/da9063_wdt.c
@@ -45,8 +45,31 @@ static unsigned int da9063_wdt_timeout_to_sel(unsigned int secs)
 	return DA9063_TWDSCALE_MAX;
 }
 
+static int da9063_wdt_disable_timer(struct da9063 *da9063)
+{
+	return regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D,
+				  DA9063_TWDSCALE_MASK,
+				  DA9063_TWDSCALE_DISABLE);
+}
+
 static int _da9063_wdt_set_timeout(struct da9063 *da9063, unsigned int regval)
 {
+	int ret;
+
+	/*
+	 * The watchdog triggers 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.
+	 * The watchdog must be disabled to be able to change the timeout
+	 * value if the watchdog is already running. Then we can set the
+	 * new timeout value which enables the watchdog again.
+	 */
+	ret = da9063_wdt_disable_timer(da9063);
+	if (ret)
+		return ret;
+
+	usleep_range(150, 300);
+
 	return regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D,
 				  DA9063_TWDSCALE_MASK, regval);
 }
@@ -71,8 +94,7 @@ static int da9063_wdt_stop(struct watchdog_device *wdd)
 	struct da9063 *da9063 = watchdog_get_drvdata(wdd);
 	int ret;
 
-	ret = regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D,
-				 DA9063_TWDSCALE_MASK, DA9063_TWDSCALE_DISABLE);
+	ret = da9063_wdt_disable_timer(da9063);
 	if (ret)
 		dev_alert(da9063->dev, "Watchdog failed to stop (err = %d)\n",
 			  ret);
-- 
2.17.0

Re: [PATCH v9 1/3] watchdog: da9063: Fix setting/changing timeout

[Guenter Roeck]

On 05/27/2018 11:45 PM, Marco Felsch wrote:
> If the timeout value is set more than once the DA9063 watchdog triggers
> a reset signal which reset the system.
> 
> 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 previous patch.
> 
> Fixes: 5e9c16e37608 ("watchdog: Add DA9063 PMIC watchdog driver.")
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/watchdog/da9063_wdt.c | 26 ++++++++++++++++++++++++--
>   1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
> index b17ac1bb1f28..c1216e61e64e 100644
> --- a/drivers/watchdog/da9063_wdt.c
> +++ b/drivers/watchdog/da9063_wdt.c
> @@ -45,8 +45,31 @@ static unsigned int da9063_wdt_timeout_to_sel(unsigned int secs)
>   	return DA9063_TWDSCALE_MAX;
>   }
>   
> +static int da9063_wdt_disable_timer(struct da9063 *da9063)
> +{
> +	return regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D,
> +				  DA9063_TWDSCALE_MASK,
> +				  DA9063_TWDSCALE_DISABLE);
> +}
> +
>   static int _da9063_wdt_set_timeout(struct da9063 *da9063, unsigned int regval)
>   {
> +	int ret;
> +
> +	/*
> +	 * The watchdog triggers 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.
> +	 * The watchdog must be disabled to be able to change the timeout
> +	 * value if the watchdog is already running. Then we can set the
> +	 * new timeout value which enables the watchdog again.
> +	 */
> +	ret = da9063_wdt_disable_timer(da9063);
> +	if (ret)
> +		return ret;
> +
> +	usleep_range(150, 300);
> +
>   	return regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D,
>   				  DA9063_TWDSCALE_MASK, regval);
>   }
> @@ -71,8 +94,7 @@ static int da9063_wdt_stop(struct watchdog_device *wdd)
>   	struct da9063 *da9063 = watchdog_get_drvdata(wdd);
>   	int ret;
>   
> -	ret = regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D,
> -				 DA9063_TWDSCALE_MASK, DA9063_TWDSCALE_DISABLE);
> +	ret = da9063_wdt_disable_timer(da9063);
>   	if (ret)
>   		dev_alert(da9063->dev, "Watchdog failed to stop (err = %d)\n",
>   			  ret);
> 

[PATCH v9 2/3] watchdog: da9063: Fix updating timeout value

[Marco Felsch]

The DA9063 watchdog has only one register field to store the timeout value
and to enable the watchdog. The watchdog gets enabled if the value is
not zero. There is no issue if the watchdog is already running but it
leads into problems if the watchdog is disabled.

If the watchdog is disabled and only the timeout value should be prepared
the watchdog gets enabled too. Add a check to get the current watchdog
state and update the watchdog timeout value on hw-side only if the
watchdog is already active.

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

diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
index c1216e61e64e..6b0092b7d5a6 100644
--- a/drivers/watchdog/da9063_wdt.c
+++ b/drivers/watchdog/da9063_wdt.c
@@ -121,10 +121,23 @@ static int da9063_wdt_set_timeout(struct watchdog_device *wdd,
 {
 	struct da9063 *da9063 = watchdog_get_drvdata(wdd);
 	unsigned int selector;
-	int ret;
+	int ret = 0;
 
 	selector = da9063_wdt_timeout_to_sel(timeout);
-	ret = _da9063_wdt_set_timeout(da9063, selector);
+
+	/*
+	 * There are two cases when a set_timeout() will be called:
+	 * 1. The watchdog is off and someone wants to set the timeout for the
+	 *    further use.
+	 * 2. The watchdog is already running and a new timeout value should be
+	 *    set.
+	 *
+	 * The watchdog can't store a timeout value not equal zero without
+	 * enabling the watchdog, so the timeout must be buffered by the driver.
+	 */
+	if (watchdog_active(wdd))
+		ret = _da9063_wdt_set_timeout(da9063, selector);
+
 	if (ret)
 		dev_err(da9063->dev, "Failed to set watchdog timeout (err = %d)\n",
 			ret);
-- 
2.17.0

Re: [PATCH v9 2/3] watchdog: da9063: Fix updating timeout value

[Guenter Roeck]

On 05/27/2018 11:45 PM, Marco Felsch wrote:
> The DA9063 watchdog has only one register field to store the timeout value
> and to enable the watchdog. The watchdog gets enabled if the value is
> not zero. There is no issue if the watchdog is already running but it
> leads into problems if the watchdog is disabled.
> 
> If the watchdog is disabled and only the timeout value should be prepared
> the watchdog gets enabled too. Add a check to get the current watchdog
> state and update the watchdog timeout value on hw-side only if the
> watchdog is already active.
> 
> Fixes: 5e9c16e37608 ("watchdog: Add DA9063 PMIC watchdog driver.")
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/watchdog/da9063_wdt.c | 17 +++++++++++++++--
>   1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
> index c1216e61e64e..6b0092b7d5a6 100644
> --- a/drivers/watchdog/da9063_wdt.c
> +++ b/drivers/watchdog/da9063_wdt.c
> @@ -121,10 +121,23 @@ static int da9063_wdt_set_timeout(struct watchdog_device *wdd,
>   {
>   	struct da9063 *da9063 = watchdog_get_drvdata(wdd);
>   	unsigned int selector;
> -	int ret;
> +	int ret = 0;
>   
>   	selector = da9063_wdt_timeout_to_sel(timeout);
> -	ret = _da9063_wdt_set_timeout(da9063, selector);
> +
> +	/*
> +	 * There are two cases when a set_timeout() will be called:
> +	 * 1. The watchdog is off and someone wants to set the timeout for the
> +	 *    further use.
> +	 * 2. The watchdog is already running and a new timeout value should be
> +	 *    set.
> +	 *
> +	 * The watchdog can't store a timeout value not equal zero without
> +	 * enabling the watchdog, so the timeout must be buffered by the driver.
> +	 */
> +	if (watchdog_active(wdd))
> +		ret = _da9063_wdt_set_timeout(da9063, selector);
> +
>   	if (ret)
>   		dev_err(da9063->dev, "Failed to set watchdog timeout (err = %d)\n",
>   			ret);
> 

[PATCH v9 3/3] watchdog: da9063: Fix timeout handling during probe

[Marco Felsch]

The watchdog can be enabled in previous steps (e.g. the bootloader). Set
the driver default timeout value (8s) if the watchdog is already running
and the HW_RUNNING flag. So the watchdog core framework will ping the
watchdog till the user space activates the watchdog explicit with the
desired timeout value.

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

diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
index 6b0092b7d5a6..a4380a887e85 100644
--- a/drivers/watchdog/da9063_wdt.c
+++ b/drivers/watchdog/da9063_wdt.c
@@ -45,6 +45,18 @@ static unsigned int da9063_wdt_timeout_to_sel(unsigned int secs)
 	return DA9063_TWDSCALE_MAX;
 }
 
+/*
+ * Return 0 if watchdog is disabled, else non zero.
+ */
+static unsigned int da9063_wdt_is_running(struct da9063 *da9063)
+{
+	unsigned int val;
+
+	regmap_read(da9063->regmap, DA9063_REG_CONTROL_D, &val);
+
+	return val & DA9063_TWDSCALE_MASK;
+}
+
 static int da9063_wdt_disable_timer(struct da9063 *da9063)
 {
 	return regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D,
@@ -206,6 +218,15 @@ static int da9063_wdt_probe(struct platform_device *pdev)
 
 	watchdog_set_drvdata(wdd, da9063);
 
+	/* Change the timeout to the default value if the watchdog is running */
+	if (da9063_wdt_is_running(da9063)) {
+		unsigned int timeout;
+
+		timeout = da9063_wdt_timeout_to_sel(DA9063_WDG_TIMEOUT);
+		_da9063_wdt_set_timeout(da9063, timeout);
+		set_bit(WDOG_HW_RUNNING, &wdd->status);
+	}
+
 	return devm_watchdog_register_device(&pdev->dev, wdd);
 }
 
-- 
2.17.0

Re: [PATCH v9 3/3] watchdog: da9063: Fix timeout handling during probe

[Guenter Roeck]

On 05/27/2018 11:45 PM, Marco Felsch wrote:
> The watchdog can be enabled in previous steps (e.g. the bootloader). Set
> the driver default timeout value (8s) if the watchdog is already running
> and the HW_RUNNING flag. So the watchdog core framework will ping the
> watchdog till the user space activates the watchdog explicit with the
> desired timeout value.
> 
> Fixes: 5e9c16e37608 ("watchdog: Add DA9063 PMIC watchdog driver.")
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/watchdog/da9063_wdt.c | 21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
> index 6b0092b7d5a6..a4380a887e85 100644
> --- a/drivers/watchdog/da9063_wdt.c
> +++ b/drivers/watchdog/da9063_wdt.c
> @@ -45,6 +45,18 @@ static unsigned int da9063_wdt_timeout_to_sel(unsigned int secs)
>   	return DA9063_TWDSCALE_MAX;
>   }
>   
> +/*
> + * Return 0 if watchdog is disabled, else non zero.
> + */
> +static unsigned int da9063_wdt_is_running(struct da9063 *da9063)
> +{
> +	unsigned int val;
> +
> +	regmap_read(da9063->regmap, DA9063_REG_CONTROL_D, &val);
> +
> +	return val & DA9063_TWDSCALE_MASK;
> +}
> +
>   static int da9063_wdt_disable_timer(struct da9063 *da9063)
>   {
>   	return regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D,
> @@ -206,6 +218,15 @@ static int da9063_wdt_probe(struct platform_device *pdev)
>   
>   	watchdog_set_drvdata(wdd, da9063);
>   
> +	/* Change the timeout to the default value if the watchdog is running */
> +	if (da9063_wdt_is_running(da9063)) {
> +		unsigned int timeout;
> +
> +		timeout = da9063_wdt_timeout_to_sel(DA9063_WDG_TIMEOUT);
> +		_da9063_wdt_set_timeout(da9063, timeout);
> +		set_bit(WDOG_HW_RUNNING, &wdd->status);
> +	}
> +
>   	return devm_watchdog_register_device(&pdev->dev, wdd);
>   }
>   
>