[PATCH v2] watchdog: omap_wdt: Add 'early_disable' module parameter

[PATCH v2] watchdog: omap_wdt: Add 'early_disable' module parameter

[Rostislav Lisovy]

This parameter makes it possible to control if the watchdog
is being disabled during initialization or will stay enabled
in case it was previously initialized in the bootloader.

To maintain the existing behavior, the default value is 'true',
thus the watchdog is disabled during initialization.

This new feature is highly inspired by the w83627hf_wdt.c

Signed-off-by: Rostislav Lisovy <lisovy@merica.cz>
---
Changes since v1:
 * Fix Runtime PM device usage counter leaking (Guenter Roeck)
 * Remove the unnecessary message informing about "stopping the watchdog"
   (Guenter Roeck)
 * Fix proper setting of "omap_wdt_users"

 drivers/watchdog/omap_wdt.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
index 3691b15..abf3b62 100644
--- a/drivers/watchdog/omap_wdt.c
+++ b/drivers/watchdog/omap_wdt.c
@@ -53,6 +53,10 @@ static unsigned timer_margin;
 module_param(timer_margin, uint, 0);
 MODULE_PARM_DESC(timer_margin, "initial watchdog timeout (in seconds)");
 
+static bool early_disable = true;
+module_param(early_disable, bool, 0);
+MODULE_PARM_DESC(early_disable, "Disable watchdog at boot time (default=1)");
+
 struct omap_wdt_dev {
 	void __iomem    *base;          /* physical */
 	struct device   *dev;
@@ -128,9 +132,11 @@ static int omap_wdt_start(struct watchdog_device *wdog)
 
 	mutex_lock(&wdev->lock);
 
-	wdev->omap_wdt_users = true;
-
-	pm_runtime_get_sync(wdev->dev);
+	/* The watchdog was disabled in probe function */
+	if (early_disable) {
+		wdev->omap_wdt_users = true;
+		pm_runtime_get_sync(wdev->dev);
+	}
 
 	/* initialize prescaler */
 	while (readl_relaxed(base + OMAP_WATCHDOG_WPS) & 0x01)
@@ -218,7 +224,7 @@ static int omap_wdt_probe(struct platform_device *pdev)
 	if (!wdev)
 		return -ENOMEM;
 
-	wdev->omap_wdt_users	= false;
+	wdev->omap_wdt_users	= (early_disable) ? false : true;
 	wdev->dev		= &pdev->dev;
 	wdev->wdt_trgr_pattern	= 0x1234;
 	mutex_init(&wdev->lock);
@@ -255,7 +261,14 @@ static int omap_wdt_probe(struct platform_device *pdev)
 	omap_wdt->bootstatus = (rs & (1 << OMAP_MPU_WD_RST_SRC_ID_SHIFT)) ?
 				WDIOF_CARDRESET : 0;
 
-	omap_wdt_disable(wdev);
+	if (early_disable) {
+		omap_wdt_disable(wdev);
+	} else {
+		pr_info("Watchdog already running. Resetting timeout to %d sec\n",
+			omap_wdt->timeout);
+		omap_wdt_set_timeout(omap_wdt, omap_wdt->timeout);
+		omap_wdt_ping(omap_wdt);
+	}
 
 	ret = watchdog_register_device(omap_wdt);
 	if (ret) {
@@ -267,7 +280,8 @@ static int omap_wdt_probe(struct platform_device *pdev)
 		readl_relaxed(wdev->base + OMAP_WATCHDOG_REV) & 0xFF,
 		omap_wdt->timeout);
 
-	pm_runtime_put_sync(wdev->dev);
+	if (early_disable)
+		pm_runtime_put_sync(wdev->dev);
 
 	return 0;
 }
-- 
1.9.1

Re: [PATCH v2] watchdog: omap_wdt: Add 'early_disable' module parameter

[Guenter Roeck]

On Tue, Sep 16, 2014 at 04:20:28PM +0200, Rostislav Lisovy wrote:
> This parameter makes it possible to control if the watchdog
> is being disabled during initialization or will stay enabled
> in case it was previously initialized in the bootloader.
> 
> To maintain the existing behavior, the default value is 'true',
> thus the watchdog is disabled during initialization.
> 
> This new feature is highly inspired by the w83627hf_wdt.c
> 
> Signed-off-by: Rostislav Lisovy <lisovy@merica.cz>
> ---
> Changes since v1:
>  * Fix Runtime PM device usage counter leaking (Guenter Roeck)
>  * Remove the unnecessary message informing about "stopping the watchdog"
>    (Guenter Roeck)
>  * Fix proper setting of "omap_wdt_users"
> 
>  drivers/watchdog/omap_wdt.c | 26 ++++++++++++++++++++------
>  1 file changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
> index 3691b15..abf3b62 100644
> --- a/drivers/watchdog/omap_wdt.c
> +++ b/drivers/watchdog/omap_wdt.c
> @@ -53,6 +53,10 @@ static unsigned timer_margin;
>  module_param(timer_margin, uint, 0);
>  MODULE_PARM_DESC(timer_margin, "initial watchdog timeout (in seconds)");
>  
> +static bool early_disable = true;
> +module_param(early_disable, bool, 0);
> +MODULE_PARM_DESC(early_disable, "Disable watchdog at boot time (default=1)");
> +
>  struct omap_wdt_dev {
>  	void __iomem    *base;          /* physical */
>  	struct device   *dev;
> @@ -128,9 +132,11 @@ static int omap_wdt_start(struct watchdog_device *wdog)
>  
>  	mutex_lock(&wdev->lock);
>  
> -	wdev->omap_wdt_users = true;
> -
> -	pm_runtime_get_sync(wdev->dev);
> +	/* The watchdog was disabled in probe function */
> +	if (early_disable) {

I think this should be
	if (!wdev->omap_wdt_users) {

> +		wdev->omap_wdt_users = true;
> +		pm_runtime_get_sync(wdev->dev);
> +	}
>  
>  	/* initialize prescaler */
>  	while (readl_relaxed(base + OMAP_WATCHDOG_WPS) & 0x01)
> @@ -218,7 +224,7 @@ static int omap_wdt_probe(struct platform_device *pdev)
>  	if (!wdev)
>  		return -ENOMEM;
>  
> -	wdev->omap_wdt_users	= false;
> +	wdev->omap_wdt_users	= (early_disable) ? false : true;

I don't immediately see what early_disable (a boot flag) has to do with 
omap_wdt_users which is supposed to show if the watchdog is running or not.
Also see below.

>  	wdev->dev		= &pdev->dev;
>  	wdev->wdt_trgr_pattern	= 0x1234;
>  	mutex_init(&wdev->lock);
> @@ -255,7 +261,14 @@ static int omap_wdt_probe(struct platform_device *pdev)
>  	omap_wdt->bootstatus = (rs & (1 << OMAP_MPU_WD_RST_SRC_ID_SHIFT)) ?
>  				WDIOF_CARDRESET : 0;
>  
> -	omap_wdt_disable(wdev);
> +	if (early_disable) {
> +		omap_wdt_disable(wdev);
> +	} else {
> +		pr_info("Watchdog already running. Resetting timeout to %d sec\n",
> +			omap_wdt->timeout);

How do you know that the watchdog was already running ?
Seems to me you are just making an assumption that it is running
without really knowing for sure.

> +		omap_wdt_set_timeout(omap_wdt, omap_wdt->timeout);
> +		omap_wdt_ping(omap_wdt);
> +	}
>  
>  	ret = watchdog_register_device(omap_wdt);
>  	if (ret) {
> @@ -267,7 +280,8 @@ static int omap_wdt_probe(struct platform_device *pdev)
>  		readl_relaxed(wdev->base + OMAP_WATCHDOG_REV) & 0xFF,
>  		omap_wdt->timeout);
>  
> -	pm_runtime_put_sync(wdev->dev);
> +	if (early_disable)
> +		pm_runtime_put_sync(wdev->dev);

Again, I think this should be
	if (!wdev->omap_wdt_users)

and you should actually detect if the watchdog is running or not.

>  
>  	return 0;
>  }
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] watchdog: omap_wdt: Add 'early_disable' module parameter

[Rostislav Lisovy]

Hello Guenter;
Thanks for the review.

On Út, 2014-09-16 at 08:49 -0700, Guenter Roeck wrote:
> How do you know that the watchdog was already running ?
> Seems to me you are just making an assumption that it is running
> without really knowing for sure.

You are definitely right, however the OMAP Watchdog seems to be such a
sophisticated piece of hardware that there is no possible way how to
determine if it is running or not (sampling timer counter twice and
comparing both values is they differ is stupid).

Best regards;
Rostislav

Re: [PATCH v2] watchdog: omap_wdt: Add 'early_disable' module parameter

[Guenter Roeck]

On 09/17/2014 08:22 AM, Rostislav Lisovy wrote:
> Hello Guenter;
> Thanks for the review.
>
> On Út, 2014-09-16 at 08:49 -0700, Guenter Roeck wrote:
>> How do you know that the watchdog was already running ?
>> Seems to me you are just making an assumption that it is running
>> without really knowing for sure.
>
> You are definitely right, however the OMAP Watchdog seems to be such a
> sophisticated piece of hardware that there is no possible way how to
> determine if it is running or not (sampling timer counter twice and
> comparing both values is they differ is stupid).
>
How about reading the OMAP_WATCHDOG_SPR register ?

Either case, you can not issue a message about the watchdog being
stopped or started if you don't know if it was actually stopped
or started. Also, if you don't really know if the watchdog was
stopped or started, all you can do is to either stop or start it,
without judgment about its previous condition.

Guenter

Re: [PATCH v2] watchdog: omap_wdt: Add 'early_disable' module parameter

[Rostislav Lisovy]

On Čt, 2014-09-18 at 21:12 -0700, Guenter Roeck wrote:
> How about reading the OMAP_WATCHDOG_SPR register ?

Unfortunately this would be not only extremely fragile (someone writes
correct start sequence followed by some garbage -- WD is running and we
are reading garbage) but according to some experiments seems not to work
at all (I am reading "0" even for a running WD probably as a result to
the pm_runtime_get_sync() call).

One of the TI employees on the TI forum confirmed (I know this is not a
rock-solid information) that it is not possible to safely determine if
the WD is running or not.


> Either case, you can not issue a message about the watchdog being
> stopped or started if you don't know if it was actually stopped
> or started. Also, if you don't really know if the watchdog was
> stopped or started, all you can do is to either stop or start it,
> without judgment about its previous condition.

You are right. I was blinded by our use case where the watchdog is
always started by the bootloader.
I will wait to see if the WATCHDOG_KEEP_ON will be integrated and then I
will add the feature for the omap_wdt.

Best regards;
Rostislav

Re: [PATCH v2] watchdog: omap_wdt: Add 'early_disable' module parameter

[Guenter Roeck]

On Tue, Sep 23, 2014 at 12:44:47PM +0200, Rostislav Lisovy wrote:
> On Čt, 2014-09-18 at 21:12 -0700, Guenter Roeck wrote:
> > How about reading the OMAP_WATCHDOG_SPR register ?
> 
> Unfortunately this would be not only extremely fragile (someone writes
> correct start sequence followed by some garbage -- WD is running and we
> are reading garbage) but according to some experiments seems not to work
> at all (I am reading "0" even for a running WD probably as a result to
> the pm_runtime_get_sync() call).
> 
> One of the TI employees on the TI forum confirmed (I know this is not a
> rock-solid information) that it is not possible to safely determine if
> the WD is running or not.
> 
Annoying, but I guess we'll have to live with that.

> 
> > Either case, you can not issue a message about the watchdog being
> > stopped or started if you don't know if it was actually stopped
> > or started. Also, if you don't really know if the watchdog was
> > stopped or started, all you can do is to either stop or start it,
> > without judgment about its previous condition.
> 
> You are right. I was blinded by our use case where the watchdog is
> always started by the bootloader.
> I will wait to see if the WATCHDOG_KEEP_ON will be integrated and then I
> will add the feature for the omap_wdt.
> 
I would predict that it will be accepted. For my part I am not happy
with the name, though, and I'll have to find the time to thoroughly review
and, if possible, test it.

Guenter

Re: [PATCH v2] watchdog: omap_wdt: Add 'early_disable' module parameter

[Guenter Roeck]

On Tue, Sep 23, 2014 at 12:44:47PM +0200, Rostislav Lisovy wrote:
> On Čt, 2014-09-18 at 21:12 -0700, Guenter Roeck wrote:
> > How about reading the OMAP_WATCHDOG_SPR register ?
> 
> Unfortunately this would be not only extremely fragile (someone writes
> correct start sequence followed by some garbage -- WD is running and we
> are reading garbage) but according to some experiments seems not to work
> at all (I am reading "0" even for a running WD probably as a result to
> the pm_runtime_get_sync() call).
> 
> One of the TI employees on the TI forum confirmed (I know this is not a
> rock-solid information) that it is not possible to safely determine if
> the WD is running or not.
> 
Annoying, but I guess we'll have to live with that.

> 
> > Either case, you can not issue a message about the watchdog being
> > stopped or started if you don't know if it was actually stopped
> > or started. Also, if you don't really know if the watchdog was
> > stopped or started, all you can do is to either stop or start it,
> > without judgment about its previous condition.
> 
> You are right. I was blinded by our use case where the watchdog is
> always started by the bootloader.
> I will wait to see if the WATCHDOG_KEEP_ON will be integrated and then I
> will add the feature for the omap_wdt.
> 
I would predict that it will be accepted. For my part I am not happy
with the name, though, and I'll have to find the time to thoroughly review
and, if possible, test it.

Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html