[PATCH v2 0/4] watchdog: omap: several cleanups

[PATCH v2 0/4] watchdog: omap: several cleanups

[Uwe Kleine-König]

Hello,

this is v2 of the series I sent on Friday. The changes to the patches
are documented in the respective mails. Thanks to Felipe Balbi and
Guenter Roeck for the feedback. I added Reviewed-by tags for Guenter who
didn't even saw these patches up to now (but who gave a carte blanche).
I assume that's ok and as intended, Guenter?

Best regards
Uwe

Uwe Kleine-König (4):
  watchdog: omap: clearify device tree documentation
  watchdog: omap: use watchdog_init_timeout instead of open coding it
  watchdog: omap: put struct watchdog_device into driver data
  watchdog: omap: simplify assignment of bootstatus

 .../devicetree/bindings/watchdog/omap-wdt.txt      |  9 +--
 drivers/watchdog/omap_wdt.c                        | 66 +++++++++-------------
 2 files changed, 32 insertions(+), 43 deletions(-)

-- 
2.1.4

[PATCH v2 1/4] watchdog: omap: clearify device tree documentation

[Uwe Kleine-König]

ti,hwmods doesn't belong into the compatible section but is a property
on it's own. Also reformat the section of required properties to match the
usual style of dt binding documents.

Reviewed-by: Felipe Balbi <balbi@ti.com>
Acked-by: Felipe Balbi <balbi@ti.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Notes:
    new in v2

 Documentation/devicetree/bindings/watchdog/omap-wdt.txt | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/watchdog/omap-wdt.txt b/Documentation/devicetree/bindings/watchdog/omap-wdt.txt
index c227970671ea..597e19d18dca 100644
--- a/Documentation/devicetree/bindings/watchdog/omap-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/omap-wdt.txt
@@ -1,10 +1,8 @@
 TI Watchdog Timer (WDT) Controller for OMAP
 
 Required properties:
-compatible:
-- "ti,omap3-wdt" for OMAP3
-- "ti,omap4-wdt" for OMAP4
-- ti,hwmods: Name of the hwmod associated to the WDT
+- compatible : "ti,omap3-wdt" for OMAP3 or "ti,omap4-wdt" for OMAP4
+- ti,hwmods : Name of the hwmod associated to the WDT
 
 Examples:
 
-- 
2.1.4

[PATCH v2 2/4] watchdog: omap: use watchdog_init_timeout instead of open coding it

[Uwe Kleine-König]

Instead of (partly) open coding watchdog_init_timeout to determine the
inital timeout use the core function that exists for exactly this
purpose.

As a side effect the "timeout-sec" device-tree property is recognized now
(though currently unused in the omap device trees).

Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Notes:
    Changes since (implicit) v1, sent with
    Message-Id: 1429868913-24049-2-git-send-email-u.kleine-koenig@pengutronix.de:
    
     - reword commit log
     - add Guenter's Reviewed-by tag

 Documentation/devicetree/bindings/watchdog/omap-wdt.txt | 3 +++
 drivers/watchdog/omap_wdt.c                             | 5 +----
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/watchdog/omap-wdt.txt b/Documentation/devicetree/bindings/watchdog/omap-wdt.txt
index 597e19d18dca..1fa20e453a2d 100644
--- a/Documentation/devicetree/bindings/watchdog/omap-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/omap-wdt.txt
@@ -4,6 +4,9 @@ Required properties:
 - compatible : "ti,omap3-wdt" for OMAP3 or "ti,omap4-wdt" for OMAP4
 - ti,hwmods : Name of the hwmod associated to the WDT
 
+Optional properties:
+- timeout-sec : default watchdog timeout in seconds
+
 Examples:
 
 wdt2: wdt@4a314000 {
diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
index 39a6cfcba016..0eb9ca04e672 100644
--- a/drivers/watchdog/omap_wdt.c
+++ b/drivers/watchdog/omap_wdt.c
@@ -234,10 +234,7 @@ static int omap_wdt_probe(struct platform_device *pdev)
 	omap_wdt->min_timeout = TIMER_MARGIN_MIN;
 	omap_wdt->max_timeout = TIMER_MARGIN_MAX;
 
-	if (timer_margin >= TIMER_MARGIN_MIN &&
-	    timer_margin <= TIMER_MARGIN_MAX)
-		omap_wdt->timeout = timer_margin;
-	else
+	if (watchdog_init_timeout(omap_wdt, timer_margin, &pdev->dev) < 0)
 		omap_wdt->timeout = TIMER_MARGIN_DEFAULT;
 
 	watchdog_set_drvdata(omap_wdt, wdev);
-- 
2.1.4

[PATCH v2 3/4] watchdog: omap: put struct watchdog_device into driver data

[Uwe Kleine-König]

This way only a single allocation is needed (per device). Also this
simplifies the data structure used by the driver because there is no
need anymore to link from one struct to the other (by means of
watchdog_{set,get}_drvdata).

Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Notes:
    Changes since (implicit) v1, sent with
    Message-Id: 1429868913-24049-3-git-send-email-u.kleine-koenig@pengutronix.de:
    
     - reword commit log
     - add Guenter's Reviewed-by tag

 drivers/watchdog/omap_wdt.c | 55 ++++++++++++++++++++-------------------------
 1 file changed, 24 insertions(+), 31 deletions(-)

diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
index 0eb9ca04e672..479e7c8e44f5 100644
--- a/drivers/watchdog/omap_wdt.c
+++ b/drivers/watchdog/omap_wdt.c
@@ -53,7 +53,10 @@ static unsigned timer_margin;
 module_param(timer_margin, uint, 0);
 MODULE_PARM_DESC(timer_margin, "initial watchdog timeout (in seconds)");
 
+#define to_omap_wdt_dev(_wdog)	container_of(_wdog, struct omap_wdt_dev, wdog)
+
 struct omap_wdt_dev {
+	struct watchdog_device wdog;
 	void __iomem    *base;          /* physical */
 	struct device   *dev;
 	bool		omap_wdt_users;
@@ -123,7 +126,7 @@ static void omap_wdt_set_timer(struct omap_wdt_dev *wdev,
 
 static int omap_wdt_start(struct watchdog_device *wdog)
 {
-	struct omap_wdt_dev *wdev = watchdog_get_drvdata(wdog);
+	struct omap_wdt_dev *wdev = to_omap_wdt_dev(wdog);
 	void __iomem *base = wdev->base;
 
 	mutex_lock(&wdev->lock);
@@ -151,7 +154,7 @@ static int omap_wdt_start(struct watchdog_device *wdog)
 
 static int omap_wdt_stop(struct watchdog_device *wdog)
 {
-	struct omap_wdt_dev *wdev = watchdog_get_drvdata(wdog);
+	struct omap_wdt_dev *wdev = to_omap_wdt_dev(wdog);
 
 	mutex_lock(&wdev->lock);
 	omap_wdt_disable(wdev);
@@ -163,7 +166,7 @@ static int omap_wdt_stop(struct watchdog_device *wdog)
 
 static int omap_wdt_ping(struct watchdog_device *wdog)
 {
-	struct omap_wdt_dev *wdev = watchdog_get_drvdata(wdog);
+	struct omap_wdt_dev *wdev = to_omap_wdt_dev(wdog);
 
 	mutex_lock(&wdev->lock);
 	omap_wdt_reload(wdev);
@@ -175,7 +178,7 @@ static int omap_wdt_ping(struct watchdog_device *wdog)
 static int omap_wdt_set_timeout(struct watchdog_device *wdog,
 				unsigned int timeout)
 {
-	struct omap_wdt_dev *wdev = watchdog_get_drvdata(wdog);
+	struct omap_wdt_dev *wdev = to_omap_wdt_dev(wdog);
 
 	mutex_lock(&wdev->lock);
 	omap_wdt_disable(wdev);
@@ -204,16 +207,11 @@ static const struct watchdog_ops omap_wdt_ops = {
 static int omap_wdt_probe(struct platform_device *pdev)
 {
 	struct omap_wd_timer_platform_data *pdata = dev_get_platdata(&pdev->dev);
-	struct watchdog_device *omap_wdt;
 	struct resource *res;
 	struct omap_wdt_dev *wdev;
 	u32 rs;
 	int ret;
 
-	omap_wdt = devm_kzalloc(&pdev->dev, sizeof(*omap_wdt), GFP_KERNEL);
-	if (!omap_wdt)
-		return -ENOMEM;
-
 	wdev = devm_kzalloc(&pdev->dev, sizeof(*wdev), GFP_KERNEL);
 	if (!wdev)
 		return -ENOMEM;
@@ -229,18 +227,17 @@ static int omap_wdt_probe(struct platform_device *pdev)
 	if (IS_ERR(wdev->base))
 		return PTR_ERR(wdev->base);
 
-	omap_wdt->info	      = &omap_wdt_info;
-	omap_wdt->ops	      = &omap_wdt_ops;
-	omap_wdt->min_timeout = TIMER_MARGIN_MIN;
-	omap_wdt->max_timeout = TIMER_MARGIN_MAX;
+	wdev->wdog.info = &omap_wdt_info;
+	wdev->wdog.ops = &omap_wdt_ops;
+	wdev->wdog.min_timeout = TIMER_MARGIN_MIN;
+	wdev->wdog.max_timeout = TIMER_MARGIN_MAX;
 
-	if (watchdog_init_timeout(omap_wdt, timer_margin, &pdev->dev) < 0)
-		omap_wdt->timeout = TIMER_MARGIN_DEFAULT;
+	if (watchdog_init_timeout(&wdev->wdog, timer_margin, &pdev->dev) < 0)
+		wdev->wdog.timeout = TIMER_MARGIN_DEFAULT;
 
-	watchdog_set_drvdata(omap_wdt, wdev);
-	watchdog_set_nowayout(omap_wdt, nowayout);
+	watchdog_set_nowayout(&wdev->wdog, nowayout);
 
-	platform_set_drvdata(pdev, omap_wdt);
+	platform_set_drvdata(pdev, wdev);
 
 	pm_runtime_enable(wdev->dev);
 	pm_runtime_get_sync(wdev->dev);
@@ -249,12 +246,12 @@ static int omap_wdt_probe(struct platform_device *pdev)
 		rs = pdata->read_reset_sources();
 	else
 		rs = 0;
-	omap_wdt->bootstatus = (rs & (1 << OMAP_MPU_WD_RST_SRC_ID_SHIFT)) ?
-				WDIOF_CARDRESET : 0;
+	wdev->wdog.bootstatus = (rs & (1 << OMAP_MPU_WD_RST_SRC_ID_SHIFT)) ?
+		WDIOF_CARDRESET : 0;
 
 	omap_wdt_disable(wdev);
 
-	ret = watchdog_register_device(omap_wdt);
+	ret = watchdog_register_device(&wdev->wdog);
 	if (ret) {
 		pm_runtime_disable(wdev->dev);
 		return ret;
@@ -262,7 +259,7 @@ static int omap_wdt_probe(struct platform_device *pdev)
 
 	pr_info("OMAP Watchdog Timer Rev 0x%02x: initial timeout %d sec\n",
 		readl_relaxed(wdev->base + OMAP_WATCHDOG_REV) & 0xFF,
-		omap_wdt->timeout);
+		wdev->wdog.timeout);
 
 	pm_runtime_put_sync(wdev->dev);
 
@@ -271,8 +268,7 @@ static int omap_wdt_probe(struct platform_device *pdev)
 
 static void omap_wdt_shutdown(struct platform_device *pdev)
 {
-	struct watchdog_device *wdog = platform_get_drvdata(pdev);
-	struct omap_wdt_dev *wdev = watchdog_get_drvdata(wdog);
+	struct omap_wdt_dev *wdev = platform_get_drvdata(pdev);
 
 	mutex_lock(&wdev->lock);
 	if (wdev->omap_wdt_users) {
@@ -284,11 +280,10 @@ static void omap_wdt_shutdown(struct platform_device *pdev)
 
 static int omap_wdt_remove(struct platform_device *pdev)
 {
-	struct watchdog_device *wdog = platform_get_drvdata(pdev);
-	struct omap_wdt_dev *wdev = watchdog_get_drvdata(wdog);
+	struct omap_wdt_dev *wdev = platform_get_drvdata(pdev);
 
 	pm_runtime_disable(wdev->dev);
-	watchdog_unregister_device(wdog);
+	watchdog_unregister_device(&wdev->wdog);
 
 	return 0;
 }
@@ -303,8 +298,7 @@ static int omap_wdt_remove(struct platform_device *pdev)
 
 static int omap_wdt_suspend(struct platform_device *pdev, pm_message_t state)
 {
-	struct watchdog_device *wdog = platform_get_drvdata(pdev);
-	struct omap_wdt_dev *wdev = watchdog_get_drvdata(wdog);
+	struct omap_wdt_dev *wdev = platform_get_drvdata(pdev);
 
 	mutex_lock(&wdev->lock);
 	if (wdev->omap_wdt_users) {
@@ -318,8 +312,7 @@ static int omap_wdt_suspend(struct platform_device *pdev, pm_message_t state)
 
 static int omap_wdt_resume(struct platform_device *pdev)
 {
-	struct watchdog_device *wdog = platform_get_drvdata(pdev);
-	struct omap_wdt_dev *wdev = watchdog_get_drvdata(wdog);
+	struct omap_wdt_dev *wdev = platform_get_drvdata(pdev);
 
 	mutex_lock(&wdev->lock);
 	if (wdev->omap_wdt_users) {
-- 
2.1.4

[PATCH v2 4/4] watchdog: omap: simplify assignment of bootstatus

[Uwe Kleine-König]

Instead of using an over-long expression involving the ?: operator use
an if and instead of an else branch rely on the fact that the data
structure was allocated using devm_kzalloc. This also allows to put the
used helper variable into a more local scope.

There is no functional change.

Reviewed-by: Felipe Balbi <balbi@ti.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Notes:
    Changes since (implicit) v1, sent with
    Message-Id: 1429868913-24049-4-git-send-email-u.kleine-koenig@pengutronix.de:
    
     - add Felipe's and Guenter's Reviewed-by tag
     - fix typo in commit log

 drivers/watchdog/omap_wdt.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
index 479e7c8e44f5..0421c06a6cf0 100644
--- a/drivers/watchdog/omap_wdt.c
+++ b/drivers/watchdog/omap_wdt.c
@@ -209,7 +209,6 @@ static int omap_wdt_probe(struct platform_device *pdev)
 	struct omap_wd_timer_platform_data *pdata = dev_get_platdata(&pdev->dev);
 	struct resource *res;
 	struct omap_wdt_dev *wdev;
-	u32 rs;
 	int ret;
 
 	wdev = devm_kzalloc(&pdev->dev, sizeof(*wdev), GFP_KERNEL);
@@ -242,12 +241,11 @@ static int omap_wdt_probe(struct platform_device *pdev)
 	pm_runtime_enable(wdev->dev);
 	pm_runtime_get_sync(wdev->dev);
 
-	if (pdata && pdata->read_reset_sources)
-		rs = pdata->read_reset_sources();
-	else
-		rs = 0;
-	wdev->wdog.bootstatus = (rs & (1 << OMAP_MPU_WD_RST_SRC_ID_SHIFT)) ?
-		WDIOF_CARDRESET : 0;
+	if (pdata && pdata->read_reset_sources) {
+		u32 rs = pdata->read_reset_sources();
+		if (rs & (1 << OMAP_MPU_WD_RST_SRC_ID_SHIFT))
+			wdev->wdog.bootstatus = WDIOF_CARDRESET;
+	}
 
 	omap_wdt_disable(wdev);
 
-- 
2.1.4

[PATCH v2 5/4] watchdog: omap: assert the counter being stopped before reprogramming

[Uwe Kleine-König]

The omap watchdog has the annoying behaviour that writes to most
registers don't have any effect when the watchdog is already running.
Quoting the AM335x reference manual:

	To modify the timer counter value (the WDT_WCRR register),
	prescaler ratio (the WDT_WCLR[4:2] PTV bit field), delay
	configuration value (the WDT_WDLY[31:0] DLY_VALUE bit field), or
	the load value (the WDT_WLDR[31:0] TIMER_LOAD bit field), the
	watchdog timer must be disabled by using the start/stop sequence
	(the WDT_WSPR register).

Currently the timer is stopped in the .probe callback but still there
are possibilities that yield to a situation where omap_wdt_start is
entered with the timer running (e.g. when /dev/watchdog is closed
without stopping and then reopened). In such a case programming the
timeout silently fails!

To circumvent this stop the timer before reprogramming.

Assuming one of the first things the watchdog user does is setting the
timeout explicitly nothing too bad should happen because this explicit
setting works fine.

Fixes: 7768a13c252a ("[PATCH] OMAP: Add Watchdog driver support")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/watchdog/omap_wdt.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
index 0421c06a6cf0..d7619dd7c1ca 100644
--- a/drivers/watchdog/omap_wdt.c
+++ b/drivers/watchdog/omap_wdt.c
@@ -135,6 +135,13 @@ static int omap_wdt_start(struct watchdog_device *wdog)
 
 	pm_runtime_get_sync(wdev->dev);
 
+	/*
+	 * Make sure the watchdog is disabled. This is unfortunately required
+	 * because writing to various registers with the watchdog running has no
+	 * effect.
+	 */
+	omap_wdt_disable(wdev);
+
 	/* initialize prescaler */
 	while (readl_relaxed(base + OMAP_WATCHDOG_WPS) & 0x01)
 		cpu_relax();
-- 
2.1.4

[PATCH v2 6/4] watchdog: omap: allow to keep timer running at probe time

[Uwe Kleine-König]

From: unknown author <unknown.author@example.com>

On some machines it might be desirable to keep the watchdog that is
started in the boot ROM running until userspace starts stroking it to be
able to handle boot failures.

To not introduce regressions for users not caring about the watchdog
keep the previous behaviour of disabling the watchdog in the probe
function unless a module parameter stop_at_probe is set to false.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/watchdog/omap_wdt.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
index d7619dd7c1ca..5525afadf0ec 100644
--- a/drivers/watchdog/omap_wdt.c
+++ b/drivers/watchdog/omap_wdt.c
@@ -53,6 +53,11 @@ static unsigned timer_margin;
 module_param(timer_margin, uint, 0);
 MODULE_PARM_DESC(timer_margin, "initial watchdog timeout (in seconds)");
 
+static bool stop_at_probe = true;
+module_param(stop_at_probe, bool, 0);
+MODULE_PARM_DESC(stop_at_probe,
+		 "Watchdog is stopped at probe time (default=true)");
+
 #define to_omap_wdt_dev(_wdog)	container_of(_wdog, struct omap_wdt_dev, wdog)
 
 struct omap_wdt_dev {
@@ -254,7 +259,8 @@ static int omap_wdt_probe(struct platform_device *pdev)
 			wdev->wdog.bootstatus = WDIOF_CARDRESET;
 	}
 
-	omap_wdt_disable(wdev);
+	if (stop_at_probe)
+		omap_wdt_disable(wdev);
 
 	ret = watchdog_register_device(&wdev->wdog);
 	if (ret) {
@@ -262,9 +268,9 @@ static int omap_wdt_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	pr_info("OMAP Watchdog Timer Rev 0x%02x: initial timeout %d sec\n",
+	pr_info("OMAP Watchdog Timer Rev 0x%02x: initial timeout %d sec, %sstopped\n",
 		readl_relaxed(wdev->base + OMAP_WATCHDOG_REV) & 0xFF,
-		wdev->wdog.timeout);
+		wdev->wdog.timeout, stop_at_probe ? "" : "not ");
 
 	pm_runtime_put_sync(wdev->dev);
 
-- 
2.1.4

Re: [PATCH v2 6/4] watchdog: omap: allow to keep timer running at probe time

[Uwe Kleine-König]

On Wed, Apr 29, 2015 at 08:38:47PM +0200, Uwe Kleine-König wrote:
> From: unknown author <unknown.author@example.com>
this is nonsense, the patch is mine. Sorry for the noise.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
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 6/4] watchdog: omap: allow to keep timer running at probe time

[Guenter Roeck]

On 04/29/2015 11:38 AM, Uwe Kleine-König wrote:
> From: unknown author <unknown.author@example.com>
>
> On some machines it might be desirable to keep the watchdog that is
> started in the boot ROM running until userspace starts stroking it to be
> able to handle boot failures.
>
> To not introduce regressions for users not caring about the watchdog
> keep the previous behaviour of disabling the watchdog in the probe
> function unless a module parameter stop_at_probe is set to false.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Uwe,

would this address the problem you are tryng to solve ?

http://www.spinics.net/lists/arm-kernel/msg413658.html

Guenter

> ---
>   drivers/watchdog/omap_wdt.c | 12 +++++++++---
>   1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
> index d7619dd7c1ca..5525afadf0ec 100644
> --- a/drivers/watchdog/omap_wdt.c
> +++ b/drivers/watchdog/omap_wdt.c
> @@ -53,6 +53,11 @@ static unsigned timer_margin;
>   module_param(timer_margin, uint, 0);
>   MODULE_PARM_DESC(timer_margin, "initial watchdog timeout (in seconds)");
>
> +static bool stop_at_probe = true;
> +module_param(stop_at_probe, bool, 0);
> +MODULE_PARM_DESC(stop_at_probe,
> +		 "Watchdog is stopped at probe time (default=true)");
> +
>   #define to_omap_wdt_dev(_wdog)	container_of(_wdog, struct omap_wdt_dev, wdog)
>
>   struct omap_wdt_dev {
> @@ -254,7 +259,8 @@ static int omap_wdt_probe(struct platform_device *pdev)
>   			wdev->wdog.bootstatus = WDIOF_CARDRESET;
>   	}
>
> -	omap_wdt_disable(wdev);
> +	if (stop_at_probe)
> +		omap_wdt_disable(wdev);
>
>   	ret = watchdog_register_device(&wdev->wdog);
>   	if (ret) {
> @@ -262,9 +268,9 @@ static int omap_wdt_probe(struct platform_device *pdev)
>   		return ret;
>   	}
>
> -	pr_info("OMAP Watchdog Timer Rev 0x%02x: initial timeout %d sec\n",
> +	pr_info("OMAP Watchdog Timer Rev 0x%02x: initial timeout %d sec, %sstopped\n",
>   		readl_relaxed(wdev->base + OMAP_WATCHDOG_REV) & 0xFF,
> -		wdev->wdog.timeout);
> +		wdev->wdog.timeout, stop_at_probe ? "" : "not ");
>
>   	pm_runtime_put_sync(wdev->dev);
>
>

Re: [PATCH v2 5/4] watchdog: omap: assert the counter being stopped before reprogramming

[Guenter Roeck]

On 04/29/2015 11:38 AM, Uwe Kleine-König wrote:
> The omap watchdog has the annoying behaviour that writes to most
> registers don't have any effect when the watchdog is already running.
> Quoting the AM335x reference manual:
>
> 	To modify the timer counter value (the WDT_WCRR register),
> 	prescaler ratio (the WDT_WCLR[4:2] PTV bit field), delay
> 	configuration value (the WDT_WDLY[31:0] DLY_VALUE bit field), or
> 	the load value (the WDT_WLDR[31:0] TIMER_LOAD bit field), the
> 	watchdog timer must be disabled by using the start/stop sequence
> 	(the WDT_WSPR register).
>
> Currently the timer is stopped in the .probe callback but still there
> are possibilities that yield to a situation where omap_wdt_start is
> entered with the timer running (e.g. when /dev/watchdog is closed
> without stopping and then reopened). In such a case programming the
> timeout silently fails!
>
> To circumvent this stop the timer before reprogramming.
>
> Assuming one of the first things the watchdog user does is setting the
> timeout explicitly nothing too bad should happen because this explicit
> setting works fine.
>
> Fixes: 7768a13c252a ("[PATCH] OMAP: Add Watchdog driver support")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Annoying, but it looks like it is necessary.

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

> ---
>   drivers/watchdog/omap_wdt.c | 7 +++++++
>   1 file changed, 7 insertions(+)
>
> diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
> index 0421c06a6cf0..d7619dd7c1ca 100644
> --- a/drivers/watchdog/omap_wdt.c
> +++ b/drivers/watchdog/omap_wdt.c
> @@ -135,6 +135,13 @@ static int omap_wdt_start(struct watchdog_device *wdog)
>
>   	pm_runtime_get_sync(wdev->dev);
>
> +	/*
> +	 * Make sure the watchdog is disabled. This is unfortunately required
> +	 * because writing to various registers with the watchdog running has no
> +	 * effect.
> +	 */
> +	omap_wdt_disable(wdev);
> +
>   	/* initialize prescaler */
>   	while (readl_relaxed(base + OMAP_WATCHDOG_WPS) & 0x01)
>   		cpu_relax();
>

Re: [PATCH v2 6/4] watchdog: omap: allow to keep timer running at probe time

[Uwe Kleine-König]

Hello Guenter,

On Thu, Apr 30, 2015 at 09:52:19PM -0700, Guenter Roeck wrote:
> On 04/29/2015 11:38 AM, Uwe Kleine-König wrote:
> >From: unknown author <unknown.author@example.com>
> >
> >On some machines it might be desirable to keep the watchdog that is
> >started in the boot ROM running until userspace starts stroking it to be
> >able to handle boot failures.
> >
> >To not introduce regressions for users not caring about the watchdog
> >keep the previous behaviour of disabling the watchdog in the probe
> >function unless a module parameter stop_at_probe is set to false.
> >
> >Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> Uwe,
> 
> would this address the problem you are tryng to solve ?
> 
> http://www.spinics.net/lists/arm-kernel/msg413658.html
yeah, it would. It just doesn't solve my problem now as the series isn't
ready for merging yet :-) I will keep my patch locally for the project
I'm working on and already provided feedback to this thread.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v2 0/4] watchdog: omap: several cleanups

[Uwe Kleine-König]

Hello,

On Mon, Apr 27, 2015 at 11:22:57AM +0200, Uwe Kleine-König wrote:
> this is v2 of the series I sent on Friday. The changes to the patches
> are documented in the respective mails. Thanks to Felipe Balbi and
> Guenter Roeck for the feedback. I added Reviewed-by tags for Guenter who
> didn't even saw these patches up to now (but who gave a carte blanche).
> I assume that's ok and as intended, Guenter?
Patches 1 to 5 got positive feedback, Wim, do you intend to take them
for the next merge window?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v2 0/4] watchdog: omap: several cleanups

[Uwe Kleine-König]

On Fri, May 08, 2015 at 09:35:02AM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Mon, Apr 27, 2015 at 11:22:57AM +0200, Uwe Kleine-König wrote:
> > this is v2 of the series I sent on Friday. The changes to the patches
> > are documented in the respective mails. Thanks to Felipe Balbi and
> > Guenter Roeck for the feedback. I added Reviewed-by tags for Guenter who
> > didn't even saw these patches up to now (but who gave a carte blanche).
> > I assume that's ok and as intended, Guenter?
> Patches 1 to 5 got positive feedback, Wim, do you intend to take them
> for the next merge window?
gentle ping!

Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v2 0/4] watchdog: omap: several cleanups

[Guenter Roeck]

On Fri, May 22, 2015 at 07:59:23PM +0200, Uwe Kleine-König wrote:
> On Fri, May 08, 2015 at 09:35:02AM +0200, Uwe Kleine-König wrote:
> > Hello,
> > 
> > On Mon, Apr 27, 2015 at 11:22:57AM +0200, Uwe Kleine-König wrote:
> > > this is v2 of the series I sent on Friday. The changes to the patches
> > > are documented in the respective mails. Thanks to Felipe Balbi and
> > > Guenter Roeck for the feedback. I added Reviewed-by tags for Guenter who
> > > didn't even saw these patches up to now (but who gave a carte blanche).
> > > I assume that's ok and as intended, Guenter?
> > Patches 1 to 5 got positive feedback, Wim, do you intend to take them
> > for the next merge window?
> gentle ping!
> 
Hi Uwe,

The patches have been in my watchdog-next branch for a while.
I sent a pull request to Wim a minute ago, to help him decide.

Thanks,
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

Re: [PATCH v2 0/4] watchdog: omap: several cleanups

[Uwe Kleine-König]

Hello,

On Fri, May 22, 2015 at 12:18:10PM -0700, Guenter Roeck wrote:
> On Fri, May 22, 2015 at 07:59:23PM +0200, Uwe Kleine-König wrote:
> > On Fri, May 08, 2015 at 09:35:02AM +0200, Uwe Kleine-König wrote:
> > > Hello,
> > > 
> > > On Mon, Apr 27, 2015 at 11:22:57AM +0200, Uwe Kleine-König wrote:
> > > > this is v2 of the series I sent on Friday. The changes to the patches
> > > > are documented in the respective mails. Thanks to Felipe Balbi and
> > > > Guenter Roeck for the feedback. I added Reviewed-by tags for Guenter who
> > > > didn't even saw these patches up to now (but who gave a carte blanche).
> > > > I assume that's ok and as intended, Guenter?
> > > Patches 1 to 5 got positive feedback, Wim, do you intend to take them
> > > for the next merge window?
> > gentle ping!
> > 
> The patches have been in my watchdog-next branch for a while.
> I sent a pull request to Wim a minute ago, to help him decide.

I didn't hear anything back since this pull request and in the meantime
other patches entered, with b2102eb36e7909c779e46f66595fda75aa219f4c
being conceptual similar to my patch 6. Also I think adding
omap_wdt_start directly after pm_runtime_put_sync is suboptimal?!

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v2 0/4] watchdog: omap: several cleanups

[Guenter Roeck]

Hi Uwe,

On 07/31/2015 02:33 AM, Uwe Kleine-König wrote:
> Hello,
>
> On Fri, May 22, 2015 at 12:18:10PM -0700, Guenter Roeck wrote:
>> On Fri, May 22, 2015 at 07:59:23PM +0200, Uwe Kleine-König wrote:
>>> On Fri, May 08, 2015 at 09:35:02AM +0200, Uwe Kleine-König wrote:
>>>> Hello,
>>>>
>>>> On Mon, Apr 27, 2015 at 11:22:57AM +0200, Uwe Kleine-König wrote:
>>>>> this is v2 of the series I sent on Friday. The changes to the patches
>>>>> are documented in the respective mails. Thanks to Felipe Balbi and
>>>>> Guenter Roeck for the feedback. I added Reviewed-by tags for Guenter who
>>>>> didn't even saw these patches up to now (but who gave a carte blanche).
>>>>> I assume that's ok and as intended, Guenter?
>>>> Patches 1 to 5 got positive feedback, Wim, do you intend to take them
>>>> for the next merge window?
>>> gentle ping!
>>>
>> The patches have been in my watchdog-next branch for a while.
>> I sent a pull request to Wim a minute ago, to help him decide.
>
> I didn't hear anything back since this pull request and in the meantime
> other patches entered, with b2102eb36e7909c779e46f66595fda75aa219f4c
> being conceptual similar to my patch 6. Also I think adding
> omap_wdt_start directly after pm_runtime_put_sync is suboptimal?!
>

I see five of your patches upstream. The only one missing is patch #6,
which should be addressed (at least for the most part) with the patch
referenced above. Is there anything else missing ?

Not sure I can follow your comment regarding omap_wdt_start() and pm_runtime_put_sync().
Do you think the watchdog should be enabled earlier ? If so, feel free to submit
a patch. You'd have to be careful with pm handling, though, since omap_wdt_start()
calls pm_runtime_get_sync().

Thanks,
Guenter

Re: [PATCH v2 0/4] watchdog: omap: several cleanups

[Uwe Kleine-König]

Hello Guenter,

On Fri, Jul 31, 2015 at 03:04:28AM -0700, Guenter Roeck wrote:
> On 07/31/2015 02:33 AM, Uwe Kleine-König wrote:
> >Hello,
> >
> >On Fri, May 22, 2015 at 12:18:10PM -0700, Guenter Roeck wrote:
> >>On Fri, May 22, 2015 at 07:59:23PM +0200, Uwe Kleine-König wrote:
> >>>On Fri, May 08, 2015 at 09:35:02AM +0200, Uwe Kleine-König wrote:
> >>>>Hello,
> >>>>
> >>>>On Mon, Apr 27, 2015 at 11:22:57AM +0200, Uwe Kleine-König wrote:
> >>>>>this is v2 of the series I sent on Friday. The changes to the patches
> >>>>>are documented in the respective mails. Thanks to Felipe Balbi and
> >>>>>Guenter Roeck for the feedback. I added Reviewed-by tags for Guenter who
> >>>>>didn't even saw these patches up to now (but who gave a carte blanche).
> >>>>>I assume that's ok and as intended, Guenter?
> >>>>Patches 1 to 5 got positive feedback, Wim, do you intend to take them
> >>>>for the next merge window?
> >>>gentle ping!
> >>>
> >>The patches have been in my watchdog-next branch for a while.
> >>I sent a pull request to Wim a minute ago, to help him decide.
> >
> >I didn't hear anything back since this pull request and in the meantime
> >other patches entered, with b2102eb36e7909c779e46f66595fda75aa219f4c
> >being conceptual similar to my patch 6. Also I think adding
> >omap_wdt_start directly after pm_runtime_put_sync is suboptimal?!
> >
> 
> I see five of your patches upstream. The only one missing is patch #6,
> which should be addressed (at least for the most part) with the patch
> referenced above. Is there anything else missing ?
You're right. I cannot reconstruct which command convinced me before
that the whole series is missing. I guess PEBKAC. Thanks.

> Not sure I can follow your comment regarding omap_wdt_start() and pm_runtime_put_sync().
> Do you think the watchdog should be enabled earlier ? If so, feel free to submit
> a patch. You'd have to be careful with pm handling, though, since omap_wdt_start()
> calls pm_runtime_get_sync().
I admit I'm not fluent with that runtime pm stuff. But it looks wrong to
me to have:

	omap_wdt_disable(wdev);

	[...]

	pm_runtime_put_sync(wdev->dev);

	if (early_enable)
		omap_wdt_start(&wdev->wdog);

And AFAIU pm_runtime_get and .._put are like references, so moving the
omap_wdt_start shouldn't hurt?! The only (positive!) effect is that the
call to pm_runtime_idle isn't done just to be reversed by
pm_runtime_resume as triggered by pm_runtime_get_sync.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v2 0/4] watchdog: omap: several cleanups

[Guenter Roeck]

Hi Uwe,

On 07/31/2015 07:52 AM, Uwe Kleine-König wrote:
> Hello Guenter,
>
> On Fri, Jul 31, 2015 at 03:04:28AM -0700, Guenter Roeck wrote:
>> On 07/31/2015 02:33 AM, Uwe Kleine-König wrote:
>>> Hello,
>>>
>>> On Fri, May 22, 2015 at 12:18:10PM -0700, Guenter Roeck wrote:
>>>> On Fri, May 22, 2015 at 07:59:23PM +0200, Uwe Kleine-König wrote:
>>>>> On Fri, May 08, 2015 at 09:35:02AM +0200, Uwe Kleine-König wrote:
>>>>>> Hello,
>>>>>>
>>>>>> On Mon, Apr 27, 2015 at 11:22:57AM +0200, Uwe Kleine-König wrote:
>>>>>>> this is v2 of the series I sent on Friday. The changes to the patches
>>>>>>> are documented in the respective mails. Thanks to Felipe Balbi and
>>>>>>> Guenter Roeck for the feedback. I added Reviewed-by tags for Guenter who
>>>>>>> didn't even saw these patches up to now (but who gave a carte blanche).
>>>>>>> I assume that's ok and as intended, Guenter?
>>>>>> Patches 1 to 5 got positive feedback, Wim, do you intend to take them
>>>>>> for the next merge window?
>>>>> gentle ping!
>>>>>
>>>> The patches have been in my watchdog-next branch for a while.
>>>> I sent a pull request to Wim a minute ago, to help him decide.
>>>
>>> I didn't hear anything back since this pull request and in the meantime
>>> other patches entered, with b2102eb36e7909c779e46f66595fda75aa219f4c
>>> being conceptual similar to my patch 6. Also I think adding
>>> omap_wdt_start directly after pm_runtime_put_sync is suboptimal?!
>>>
>>
>> I see five of your patches upstream. The only one missing is patch #6,
>> which should be addressed (at least for the most part) with the patch
>> referenced above. Is there anything else missing ?
> You're right. I cannot reconstruct which command convinced me before
> that the whole series is missing. I guess PEBKAC. Thanks.
>
>> Not sure I can follow your comment regarding omap_wdt_start() and pm_runtime_put_sync().
>> Do you think the watchdog should be enabled earlier ? If so, feel free to submit
>> a patch. You'd have to be careful with pm handling, though, since omap_wdt_start()
>> calls pm_runtime_get_sync().
> I admit I'm not fluent with that runtime pm stuff. But it looks wrong to
> me to have:
>
> 	omap_wdt_disable(wdev);
>
> 	[...]
>
> 	pm_runtime_put_sync(wdev->dev);
>
> 	if (early_enable)
> 		omap_wdt_start(&wdev->wdog);
>
> And AFAIU pm_runtime_get and .._put are like references, so moving the
> omap_wdt_start shouldn't hurt?! The only (positive!) effect is that the
> call to pm_runtime_idle isn't done just to be reversed by
> pm_runtime_resume as triggered by pm_runtime_get_sync.
>

I would agree, but I must admit that I have no idea how the runtime code
works. What happens if pm_runtime_get_sync() is called twice ?
It seems to be doing much more than just incrementing a reference count.
If you think it is ok, feel free to submit a patch to change the call order.

There is a few other things I don't understand about the driver, such as
why it needs a separate lock "to avoid races with PM". It would be useful
to understand why this is needed and what exactly the race condition is,
because it might apply to other drivers and ask for an infrastructure
solution.

Anyway, help me remember - isn't this the chip where it is impossible
to find out if it is running or not ? If so, maybe it would make sense
to simply always enable it in probe and let the infrastructure handle
keepalives while the device is closed (I am working on a simplified version
of that infrastructure code).

Thanks,
Guenter