[PATCH v2 1/3] watchdog: da9062: Initialize timeout during probe

[PATCH v2 1/3] watchdog: da9062: Initialize timeout during probe

[Stefan Riedmueller]

During probe try to set the timeout from device tree and fall back to
either the pre-configured timeout set by e.g. the bootloader in case the
watchdog is already running or the default value.

If the watchdog is already running make sure to update the timeout and
tell the framework about the running state to make sure the watchdog is
handled correctly until user space takes over. Updating the timeout also
removes the need for an additional manual ping so we can remove that as
well.

Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
---
Changes in v2:
 - Reworked the patch to use the pre-configured timeout instead of the default
   value as a fallback in case no DT value is present.
 - To achieve the previous point watchdog_init_timeout was added to get the
   DT value if present.
 - Added a timeout update if the watchdog is running to set the desired
   timeout and in this instance removed the manual ping at the end.
 - Removed info message.
---
 drivers/watchdog/da9062_wdt.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/watchdog/da9062_wdt.c b/drivers/watchdog/da9062_wdt.c
index 0ad15d55071c..10b37dd65bed 100644
--- a/drivers/watchdog/da9062_wdt.c
+++ b/drivers/watchdog/da9062_wdt.c
@@ -35,6 +35,15 @@ struct da9062_watchdog {
 	bool use_sw_pm;
 };
 
+static unsigned int da9062_wdt_read_timeout(struct da9062_watchdog *wdt)
+{
+	unsigned 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;
@@ -183,7 +192,7 @@ MODULE_DEVICE_TABLE(of, da9062_compatible_id_table);
 static int da9062_wdt_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	int ret;
+	unsigned int timeout;
 	struct da9062 *chip;
 	struct da9062_watchdog *wdt;
 
@@ -213,11 +222,19 @@ static int da9062_wdt_probe(struct platform_device *pdev)
 	watchdog_set_drvdata(&wdt->wdtdev, wdt);
 	dev_set_drvdata(dev, &wdt->wdtdev);
 
-	ret = devm_watchdog_register_device(dev, &wdt->wdtdev);
-	if (ret < 0)
-		return ret;
+	timeout = da9062_wdt_read_timeout(wdt);
+	if (timeout)
+		wdt->wdtdev.timeout = timeout;
+
+	/* Set timeout from DT value if available */
+	watchdog_init_timeout(&wdt->wdtdev, 0, dev);
+
+	if (timeout) {
+		da9062_wdt_set_timeout(&wdt->wdtdev, wdt->wdtdev.timeout);
+		set_bit(WDOG_HW_RUNNING, &wdt->wdtdev.status);
+	}
 
-	return da9062_wdt_ping(&wdt->wdtdev);
+	return devm_watchdog_register_device(dev, &wdt->wdtdev);
 }
 
 static int __maybe_unused da9062_wdt_suspend(struct device *dev)
-- 
2.23.0

[PATCH v2 2/3] watchdog: da9063: Make use of pre-configured timeout during probe

[Stefan Riedmueller]

The watchdog might already be running during boot with a timeout set by
e.g. the bootloader. Make use of this pre-configured timeout instead of
falling back to the default timeout if no device tree value is given.

Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
---
Changes in v2:
 - Reworked patch to use the pre-configured timeout onyl as a fallback
   instead of the default value.
 - Removed info message.
---
 drivers/watchdog/da9063_wdt.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
index 3d65e92a4e3f..423584252606 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;
+	unsigned int timeout;
 
 	if (!dev->parent)
 		return -EINVAL;
@@ -214,13 +216,19 @@ 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;
+
+	/* Use pre-configured timeout if watchdog is already running. */
+	timeout = da9063_wdt_read_timeout(da9063);
+	if (timeout)
+		wdd->timeout = timeout;
+
+	/* Set timeout, maybe override it with DT value, scale it */
 	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)) {
+	/* Update timeout if the watchdog is already running. */
+	if (timeout) {
 		da9063_wdt_update_timeout(da9063, wdd->timeout);
 		set_bit(WDOG_HW_RUNNING, &wdd->status);
 	}
-- 
2.23.0

[PATCH v2 3/3] watchdog: da9062: No need to ping manually before setting timeout

[Stefan Riedmueller]

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>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
---
Changes in v2:
 - Added Reviewed-by tags.
---
 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 10b37dd65bed..706fb09c2f24 100644
--- a/drivers/watchdog/da9062_wdt.c
+++ b/drivers/watchdog/da9062_wdt.c
@@ -67,11 +67,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

RE: [PATCH v2 1/3] watchdog: da9062: Initialize timeout during probe

[Adam Thomson]

On 03 April 2020 14:07, Stefan Riedmueller wrote:

> During probe try to set the timeout from device tree and fall back to
> either the pre-configured timeout set by e.g. the bootloader in case the
> watchdog is already running or the default value.
> 
> If the watchdog is already running make sure to update the timeout and
> tell the framework about the running state to make sure the watchdog is
> handled correctly until user space takes over. Updating the timeout also
> removes the need for an additional manual ping so we can remove that as
> well.
> 
> Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>

Reviewed-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>

> ---
> Changes in v2:
>  - Reworked the patch to use the pre-configured timeout instead of the default
>    value as a fallback in case no DT value is present.
>  - To achieve the previous point watchdog_init_timeout was added to get the
>    DT value if present.
>  - Added a timeout update if the watchdog is running to set the desired
>    timeout and in this instance removed the manual ping at the end.
>  - Removed info message.
> ---
>  drivers/watchdog/da9062_wdt.c | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/watchdog/da9062_wdt.c b/drivers/watchdog/da9062_wdt.c
> index 0ad15d55071c..10b37dd65bed 100644
> --- a/drivers/watchdog/da9062_wdt.c
> +++ b/drivers/watchdog/da9062_wdt.c
> @@ -35,6 +35,15 @@ struct da9062_watchdog {
>  	bool use_sw_pm;
>  };
> 
> +static unsigned int da9062_wdt_read_timeout(struct da9062_watchdog *wdt)
> +{
> +	unsigned 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;
> @@ -183,7 +192,7 @@ MODULE_DEVICE_TABLE(of,
> da9062_compatible_id_table);
>  static int da9062_wdt_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> -	int ret;
> +	unsigned int timeout;
>  	struct da9062 *chip;
>  	struct da9062_watchdog *wdt;
> 
> @@ -213,11 +222,19 @@ static int da9062_wdt_probe(struct platform_device
> *pdev)
>  	watchdog_set_drvdata(&wdt->wdtdev, wdt);
>  	dev_set_drvdata(dev, &wdt->wdtdev);
> 
> -	ret = devm_watchdog_register_device(dev, &wdt->wdtdev);
> -	if (ret < 0)
> -		return ret;
> +	timeout = da9062_wdt_read_timeout(wdt);
> +	if (timeout)
> +		wdt->wdtdev.timeout = timeout;
> +
> +	/* Set timeout from DT value if available */
> +	watchdog_init_timeout(&wdt->wdtdev, 0, dev);
> +
> +	if (timeout) {
> +		da9062_wdt_set_timeout(&wdt->wdtdev, wdt-
> >wdtdev.timeout);
> +		set_bit(WDOG_HW_RUNNING, &wdt->wdtdev.status);
> +	}
> 
> -	return da9062_wdt_ping(&wdt->wdtdev);
> +	return devm_watchdog_register_device(dev, &wdt->wdtdev);
>  }
> 
>  static int __maybe_unused da9062_wdt_suspend(struct device *dev)
> --
> 2.23.0

RE: [PATCH v2 2/3] watchdog: da9063: Make use of pre-configured timeout during probe

[Adam Thomson]

On 03 April 2020 14:07, Stefan Riedmueller wrote:

> The watchdog might already be running during boot with a timeout set by
> e.g. the bootloader. Make use of this pre-configured timeout instead of
> falling back to the default timeout if no device tree value is given.
> 
> Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>

Reviewed-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>

> ---
> Changes in v2:
>  - Reworked patch to use the pre-configured timeout onyl as a fallback
>    instead of the default value.
>  - Removed info message.
> ---
>  drivers/watchdog/da9063_wdt.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
> index 3d65e92a4e3f..423584252606 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;
> +	unsigned int timeout;
> 
>  	if (!dev->parent)
>  		return -EINVAL;
> @@ -214,13 +216,19 @@ 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;
> +
> +	/* Use pre-configured timeout if watchdog is already running. */
> +	timeout = da9063_wdt_read_timeout(da9063);
> +	if (timeout)
> +		wdd->timeout = timeout;
> +
> +	/* Set timeout, maybe override it with DT value, scale it */
>  	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)) {
> +	/* Update timeout if the watchdog is already running. */
> +	if (timeout) {
>  		da9063_wdt_update_timeout(da9063, wdd->timeout);
>  		set_bit(WDOG_HW_RUNNING, &wdd->status);
>  	}
> --
> 2.23.0

Re: [PATCH v2 1/3] watchdog: da9062: Initialize timeout during probe

[Guenter Roeck]

On Fri, Apr 03, 2020 at 03:07:26PM +0200, Stefan Riedmueller wrote:
> During probe try to set the timeout from device tree and fall back to
> either the pre-configured timeout set by e.g. the bootloader in case the
> watchdog is already running or the default value.
> 
> If the watchdog is already running make sure to update the timeout and
> tell the framework about the running state to make sure the watchdog is
> handled correctly until user space takes over. Updating the timeout also
> removes the need for an additional manual ping so we can remove that as
> well.
> 
> Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
> Reviewed-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>

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

> ---
> Changes in v2:
>  - Reworked the patch to use the pre-configured timeout instead of the default
>    value as a fallback in case no DT value is present.
>  - To achieve the previous point watchdog_init_timeout was added to get the
>    DT value if present.
>  - Added a timeout update if the watchdog is running to set the desired
>    timeout and in this instance removed the manual ping at the end.
>  - Removed info message.
> ---
>  drivers/watchdog/da9062_wdt.c | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/watchdog/da9062_wdt.c b/drivers/watchdog/da9062_wdt.c
> index 0ad15d55071c..10b37dd65bed 100644
> --- a/drivers/watchdog/da9062_wdt.c
> +++ b/drivers/watchdog/da9062_wdt.c
> @@ -35,6 +35,15 @@ struct da9062_watchdog {
>  	bool use_sw_pm;
>  };
>  
> +static unsigned int da9062_wdt_read_timeout(struct da9062_watchdog *wdt)
> +{
> +	unsigned 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;
> @@ -183,7 +192,7 @@ MODULE_DEVICE_TABLE(of, da9062_compatible_id_table);
>  static int da9062_wdt_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> -	int ret;
> +	unsigned int timeout;
>  	struct da9062 *chip;
>  	struct da9062_watchdog *wdt;
>  
> @@ -213,11 +222,19 @@ static int da9062_wdt_probe(struct platform_device *pdev)
>  	watchdog_set_drvdata(&wdt->wdtdev, wdt);
>  	dev_set_drvdata(dev, &wdt->wdtdev);
>  
> -	ret = devm_watchdog_register_device(dev, &wdt->wdtdev);
> -	if (ret < 0)
> -		return ret;
> +	timeout = da9062_wdt_read_timeout(wdt);
> +	if (timeout)
> +		wdt->wdtdev.timeout = timeout;
> +
> +	/* Set timeout from DT value if available */
> +	watchdog_init_timeout(&wdt->wdtdev, 0, dev);
> +
> +	if (timeout) {
> +		da9062_wdt_set_timeout(&wdt->wdtdev, wdt->wdtdev.timeout);
> +		set_bit(WDOG_HW_RUNNING, &wdt->wdtdev.status);
> +	}
>  
> -	return da9062_wdt_ping(&wdt->wdtdev);
> +	return devm_watchdog_register_device(dev, &wdt->wdtdev);
>  }
>  
>  static int __maybe_unused da9062_wdt_suspend(struct device *dev)

Re: [PATCH v2 2/3] watchdog: da9063: Make use of pre-configured timeout during probe

[Guenter Roeck]

On Fri, Apr 03, 2020 at 03:07:27PM +0200, Stefan Riedmueller wrote:
> The watchdog might already be running during boot with a timeout set by
> e.g. the bootloader. Make use of this pre-configured timeout instead of
> falling back to the default timeout if no device tree value is given.
> 
> Signed-off-by: Stefan Riedmueller <s.riedmueller@phytec.de>
> Reviewed-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>

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

> ---
> Changes in v2:
>  - Reworked patch to use the pre-configured timeout onyl as a fallback
>    instead of the default value.
>  - Removed info message.
> ---
>  drivers/watchdog/da9063_wdt.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
> index 3d65e92a4e3f..423584252606 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;
> +	unsigned int timeout;
>  
>  	if (!dev->parent)
>  		return -EINVAL;
> @@ -214,13 +216,19 @@ 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;
> +
> +	/* Use pre-configured timeout if watchdog is already running. */
> +	timeout = da9063_wdt_read_timeout(da9063);
> +	if (timeout)
> +		wdd->timeout = timeout;
> +
> +	/* Set timeout, maybe override it with DT value, scale it */
>  	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)) {
> +	/* Update timeout if the watchdog is already running. */
> +	if (timeout) {
>  		da9063_wdt_update_timeout(da9063, wdd->timeout);
>  		set_bit(WDOG_HW_RUNNING, &wdd->status);
>  	}