[PATCH v1 1/6] watchdog: rename dev to hwdev

[PATCH v1 1/6] watchdog: rename dev to hwdev

[Oleksij Rempel]

From: Sascha Hauer <s.hauer@pengutronix.de>

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/watchdog/ar9344_wdt.c  | 2 +-
 drivers/watchdog/davinci_wdt.c | 2 +-
 drivers/watchdog/dw_wdt.c      | 4 ++--
 drivers/watchdog/im28wd.c      | 2 +-
 drivers/watchdog/imxwd.c       | 2 +-
 drivers/watchdog/orion_wdt.c   | 2 +-
 drivers/watchdog/wd_core.c     | 4 ++--
 include/watchdog.h             | 2 +-
 8 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/watchdog/ar9344_wdt.c b/drivers/watchdog/ar9344_wdt.c
index d570cb40f..c1dfc994a 100644
--- a/drivers/watchdog/ar9344_wdt.c
+++ b/drivers/watchdog/ar9344_wdt.c
@@ -95,7 +95,7 @@ static int ar9344_wdt_probe(struct device_d *dev)
 
 	priv->base = IOMEM(iores->start);
 	priv->wd.set_timeout = ar9344_watchdog_set_timeout;
-	priv->wd.dev = dev;
+	priv->wd.hwdev = dev;
 	priv->dev = dev;
 
 	dev->priv = priv;
diff --git a/drivers/watchdog/davinci_wdt.c b/drivers/watchdog/davinci_wdt.c
index 03dc83408..2ac5f8b38 100644
--- a/drivers/watchdog/davinci_wdt.c
+++ b/drivers/watchdog/davinci_wdt.c
@@ -149,7 +149,7 @@ static int davinci_wdt_probe(struct device_d *dev)
 	clk_enable(davinci_wdt->clk);
 
 	davinci_wdt->wd.set_timeout = davinci_wdt_set_timeout;
-	davinci_wdt->wd.dev = dev;
+	davinci_wdt->wd.hwdev = dev;
 
 	ret = watchdog_register(&davinci_wdt->wd);
 	if (ret < 0)
diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
index 8fd8c81e6..85e810f24 100644
--- a/drivers/watchdog/dw_wdt.c
+++ b/drivers/watchdog/dw_wdt.c
@@ -73,7 +73,7 @@ static int dw_wdt_stop(struct watchdog *wdd)
 	struct dw_wdt *dw_wdt = to_dw_wdt(wdd);
 
 	if (IS_ERR(dw_wdt->rst)) {
-		dev_warn(dw_wdt->wdd.dev, "No reset line. Will not stop.\n");
+		dev_warn(dw_wdt->wdd.hwdev, "No reset line. Will not stop.\n");
 		return PTR_ERR(dw_wdt->rst);
 	}
 
@@ -157,7 +157,7 @@ static int dw_wdt_drv_probe(struct device_d *dev)
 
 	wdd = &dw_wdt->wdd;
 	wdd->name = "dw_wdt";
-	wdd->dev = dev;
+	wdd->hwdev = dev;
 	wdd->set_timeout = dw_wdt_set_timeout;
 
 	ret = watchdog_register(wdd);
diff --git a/drivers/watchdog/im28wd.c b/drivers/watchdog/im28wd.c
index 1956fdb73..43edea8d0 100644
--- a/drivers/watchdog/im28wd.c
+++ b/drivers/watchdog/im28wd.c
@@ -199,7 +199,7 @@ static int imx28_wd_probe(struct device_d *dev)
 		return PTR_ERR(iores);
 	priv->regs = IOMEM(iores->start);
 	priv->wd.set_timeout = imx28_watchdog_set_timeout;
-	priv->wd.dev = dev;
+	priv->wd.hwdev = dev;
 
 	if (!(readl(priv->regs + MXS_RTC_STAT) & MXS_RTC_STAT_WD_PRESENT)) {
 		rc = -ENODEV;
diff --git a/drivers/watchdog/imxwd.c b/drivers/watchdog/imxwd.c
index bd5e518bc..6b33834fe 100644
--- a/drivers/watchdog/imxwd.c
+++ b/drivers/watchdog/imxwd.c
@@ -218,7 +218,7 @@ static int imx_wd_probe(struct device_d *dev)
 	priv->base = IOMEM(iores->start);
 	priv->ops = ops;
 	priv->wd.set_timeout = imx_watchdog_set_timeout;
-	priv->wd.dev = dev;
+	priv->wd.hwdev = dev;
 	priv->dev = dev;
 
 	priv->ext_reset = of_property_read_bool(dev->device_node,
diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
index 2802033f7..4fe1ac1fd 100644
--- a/drivers/watchdog/orion_wdt.c
+++ b/drivers/watchdog/orion_wdt.c
@@ -88,7 +88,7 @@ static int orion_wdt_probe(struct device_d *dev)
 
 	ddata->wd.set_timeout = armada_xp_set_timeout;
 	ddata->wd.name = "orion_wdt";
-	ddata->wd.dev = dev;
+	ddata->wd.hwdev = dev;
 
 	res_timer = dev_request_mem_resource(dev, 0);
 	if (IS_ERR(res_timer)) {
diff --git a/drivers/watchdog/wd_core.c b/drivers/watchdog/wd_core.c
index 3a3f51964..56436e6f8 100644
--- a/drivers/watchdog/wd_core.c
+++ b/drivers/watchdog/wd_core.c
@@ -23,8 +23,8 @@ static LIST_HEAD(watchdog_list);
 
 static const char *watchdog_name(struct watchdog *wd)
 {
-	if (wd->dev)
-		return dev_name(wd->dev);
+	if (wd->hwdev)
+		return dev_name(wd->hwdev);
 	if (wd->name)
 		return wd->name;
 
diff --git a/include/watchdog.h b/include/watchdog.h
index 3e8a487a4..848981cc0 100644
--- a/include/watchdog.h
+++ b/include/watchdog.h
@@ -16,7 +16,7 @@
 struct watchdog {
 	int (*set_timeout)(struct watchdog *, unsigned);
 	const char *name;
-	struct device_d *dev;
+	struct device_d *hwdev;
 	unsigned int priority;
 	struct list_head list;
 };
-- 
2.16.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v1 2/6] watchdog: move max timeout test in to wd_core

[Oleksij Rempel]

From: Oleksij Rempel <linux@rempel-privat.de>

this also will be needed for watchdog poller as well

Signed-off-by: Oleksij Rempel <linux@rempel-privat.de>
---
 drivers/watchdog/ar9344_wdt.c | 15 ++++-----------
 drivers/watchdog/im28wd.c     |  4 +---
 drivers/watchdog/imxwd.c      | 10 ++++------
 drivers/watchdog/orion_wdt.c  |  4 +---
 drivers/watchdog/wd_core.c    |  6 ++++++
 include/watchdog.h            |  1 +
 6 files changed, 17 insertions(+), 23 deletions(-)

diff --git a/drivers/watchdog/ar9344_wdt.c b/drivers/watchdog/ar9344_wdt.c
index c1dfc994a..461528863 100644
--- a/drivers/watchdog/ar9344_wdt.c
+++ b/drivers/watchdog/ar9344_wdt.c
@@ -41,20 +41,11 @@ struct ar9344_wd {
 static int ar9344_watchdog_set_timeout(struct watchdog *wd, unsigned timeout)
 {
 	struct ar9344_wd *priv = to_ar9344_wd(wd);
-	u32 val, ctrl, rate, max_timout;
-
-	rate = clk_get_rate(priv->clk);
-	max_timout = U32_MAX / rate;
-
-	if (timeout > max_timout) {
-		dev_err(priv->dev, "timeout value out of range: %d > %d\n",
-			timeout, max_timout);
-		return -EINVAL;
-	}
+	u32 val, ctrl;
 
 	if (timeout) {
 		ctrl = AR9344_WD_CTRL_ACTION_FCR;
-		val = timeout * rate;
+		val = timeout * clk_get_rate(priv->clk);
 	} else {
 		ctrl = AR9344_WD_CTRL_ACTION_NONE;
 		val = U32_MAX;
@@ -111,6 +102,8 @@ static int ar9344_wdt_probe(struct device_d *dev)
 
 	clk_enable(priv->clk);
 
+	priv->wd.timeout_max = U32_MAX / clk_get_rate(priv->clk);
+
 	ret = watchdog_register(&priv->wd);
 	if (ret)
 		goto on_error;
diff --git a/drivers/watchdog/im28wd.c b/drivers/watchdog/im28wd.c
index 43edea8d0..2b233ede2 100644
--- a/drivers/watchdog/im28wd.c
+++ b/drivers/watchdog/im28wd.c
@@ -133,9 +133,6 @@ static int imx28_watchdog_set_timeout(struct watchdog *wd, unsigned timeout)
 	struct imx28_wd *pwd = (struct imx28_wd *)to_imx28_wd(wd);
 	void __iomem *base;
 
-	if (timeout > (ULONG_MAX / WDOG_TICK_RATE))
-		return -EINVAL;
-
 	if (timeout) {
 		writel(timeout * WDOG_TICK_RATE, pwd->regs + MXS_RTC_WATCHDOG);
 		base = pwd->regs + MXS_RTC_SET_ADDR;
@@ -199,6 +196,7 @@ static int imx28_wd_probe(struct device_d *dev)
 		return PTR_ERR(iores);
 	priv->regs = IOMEM(iores->start);
 	priv->wd.set_timeout = imx28_watchdog_set_timeout;
+	priv->wd.timeout_max = ULONG_MAX / WDOG_TICK_RATE;
 	priv->wd.hwdev = dev;
 
 	if (!(readl(priv->regs + MXS_RTC_STAT) & MXS_RTC_STAT_WD_PRESENT)) {
diff --git a/drivers/watchdog/imxwd.c b/drivers/watchdog/imxwd.c
index 6b33834fe..a66fae400 100644
--- a/drivers/watchdog/imxwd.c
+++ b/drivers/watchdog/imxwd.c
@@ -28,6 +28,7 @@ struct imx_wd_ops {
 	int (*set_timeout)(struct imx_wd *, unsigned);
 	void (*soc_reset)(struct imx_wd *);
 	int (*init)(struct imx_wd *);
+	unsigned int timeout_max;
 };
 
 struct imx_wd {
@@ -71,9 +72,6 @@ static int imx1_watchdog_set_timeout(struct imx_wd *priv, unsigned timeout)
 
 	dev_dbg(priv->dev, "%s: %d\n", __func__, timeout);
 
-	if (timeout > 64)
-		return -EINVAL;
-
 	if (!timeout) {
 		writew(IMX1_WDOG_WCR_WHALT, priv->base + IMX1_WDOG_WCR);
 		return 0;
@@ -102,9 +100,6 @@ static int imx21_watchdog_set_timeout(struct imx_wd *priv, unsigned timeout)
 
 	dev_dbg(priv->dev, "%s: %d\n", __func__, timeout);
 
-	if (timeout > 128)
-		return -EINVAL;
-
 	if (timeout == 0) /* bit 2 (WDE) cannot be set to 0 again */
 		return -ENOSYS;
 
@@ -218,6 +213,7 @@ static int imx_wd_probe(struct device_d *dev)
 	priv->base = IOMEM(iores->start);
 	priv->ops = ops;
 	priv->wd.set_timeout = imx_watchdog_set_timeout;
+	priv->wd.timeout_max = priv->ops->timeout_max;
 	priv->wd.hwdev = dev;
 	priv->dev = dev;
 
@@ -259,11 +255,13 @@ static const struct imx_wd_ops imx21_wd_ops = {
 	.set_timeout = imx21_watchdog_set_timeout,
 	.soc_reset = imx21_soc_reset,
 	.init = imx21_wd_init,
+	.timeout_max = 128,
 };
 
 static const struct imx_wd_ops imx1_wd_ops = {
 	.set_timeout = imx1_watchdog_set_timeout,
 	.soc_reset = imx1_soc_reset,
+	.timeout_max = 64,
 };
 
 static __maybe_unused struct of_device_id imx_wdt_dt_ids[] = {
diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
index 4fe1ac1fd..dd1fa3a04 100644
--- a/drivers/watchdog/orion_wdt.c
+++ b/drivers/watchdog/orion_wdt.c
@@ -49,9 +49,6 @@ static int armada_xp_set_timeout(struct watchdog *wd, unsigned timeout)
 		container_of(wd, struct orion_wdt_ddata, wd);
 	u32 ctrl;
 
-	if (0xffffffff / CLKRATE < timeout)
-		return -EINVAL;
-
 	ctrl = readl(ddata->timer_base + TIMER_CTRL);
 
 	if (timeout == 0) {
@@ -89,6 +86,7 @@ static int orion_wdt_probe(struct device_d *dev)
 	ddata->wd.set_timeout = armada_xp_set_timeout;
 	ddata->wd.name = "orion_wdt";
 	ddata->wd.hwdev = dev;
+	ddata->wd.timeout_max = U32_MAX / CLKRATE;
 
 	res_timer = dev_request_mem_resource(dev, 0);
 	if (IS_ERR(res_timer)) {
diff --git a/drivers/watchdog/wd_core.c b/drivers/watchdog/wd_core.c
index 56436e6f8..1d0f0de7d 100644
--- a/drivers/watchdog/wd_core.c
+++ b/drivers/watchdog/wd_core.c
@@ -36,6 +36,9 @@ int watchdog_register(struct watchdog *wd)
 	if (!wd->priority)
 		wd->priority = WATCHDOG_DEFAULT_PRIORITY;
 
+	dev_add_param_uint32_ro(&wd->dev, "timeout_max",
+			&wd->timeout_max, "%u");
+
 	list_add_tail(&wd->list, &watchdog_list);
 
 	pr_debug("registering watchdog %s with priority %d\n", watchdog_name(wd),
@@ -81,6 +84,9 @@ int watchdog_set_timeout(unsigned timeout)
 	if (!wd)
 		return -ENODEV;
 
+	if (timeout > wd->timeout_max)
+		return -EINVAL;
+
 	pr_debug("setting timeout on %s to %ds\n", watchdog_name(wd), timeout);
 
 	return wd->set_timeout(wd, timeout);
diff --git a/include/watchdog.h b/include/watchdog.h
index 848981cc0..1e06fbc3f 100644
--- a/include/watchdog.h
+++ b/include/watchdog.h
@@ -18,6 +18,7 @@ struct watchdog {
 	const char *name;
 	struct device_d *hwdev;
 	unsigned int priority;
+	unsigned int timeout_max;
 	struct list_head list;
 };
 
-- 
2.16.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v1 3/6] watchdog: register watchdog virtual device with short name wdog

[Oleksij Rempel]

the watchdog hwdev is usually named with devicetree schema
which is not practical for CLI.

On device registration "wdog" will be extended with some index number
extracted from devicetree (if awailable) or automatically assigned
first available number. End result will be "wdog0" .. etc.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/watchdog/wd_core.c | 6 ++++++
 include/watchdog.h         | 1 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/watchdog/wd_core.c b/drivers/watchdog/wd_core.c
index 1d0f0de7d..97e2ab568 100644
--- a/drivers/watchdog/wd_core.c
+++ b/drivers/watchdog/wd_core.c
@@ -33,6 +33,11 @@ static const char *watchdog_name(struct watchdog *wd)
 
 int watchdog_register(struct watchdog *wd)
 {
+
+	strcpy(wd->dev.name, "wdog");
+	wd->dev.parent = wd->hwdev;
+	register_device(&wd->dev);
+
 	if (!wd->priority)
 		wd->priority = WATCHDOG_DEFAULT_PRIORITY;
 
@@ -50,6 +55,7 @@ EXPORT_SYMBOL(watchdog_register);
 
 int watchdog_deregister(struct watchdog *wd)
 {
+	unregister_device(&wd->dev);
 	list_del(&wd->list);
 
 	return 0;
diff --git a/include/watchdog.h b/include/watchdog.h
index 1e06fbc3f..a2459d255 100644
--- a/include/watchdog.h
+++ b/include/watchdog.h
@@ -17,6 +17,7 @@ struct watchdog {
 	int (*set_timeout)(struct watchdog *, unsigned);
 	const char *name;
 	struct device_d *hwdev;
+	struct device_d dev;
 	unsigned int priority;
 	unsigned int timeout_max;
 	struct list_head list;
-- 
2.16.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v1 4/6] watchdog: set some reasonable timeout_max value if no other is available

[Oleksij Rempel]

Some drivers do not provide timeout_max value.
Using some value is probbaly better then setting timeout_max to 0.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/watchdog/wd_core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/watchdog/wd_core.c b/drivers/watchdog/wd_core.c
index 97e2ab568..66dbd790d 100644
--- a/drivers/watchdog/wd_core.c
+++ b/drivers/watchdog/wd_core.c
@@ -41,6 +41,10 @@ int watchdog_register(struct watchdog *wd)
 	if (!wd->priority)
 		wd->priority = WATCHDOG_DEFAULT_PRIORITY;
 
+	/* set some default sane value */
+	if (!wd->timeout_max)
+		wd->timeout_max = 60 * 4;
+
 	dev_add_param_uint32_ro(&wd->dev, "timeout_max",
 			&wd->timeout_max, "%u");
 
-- 
2.16.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v1 5/6] watchdog: provide timeout_cur value

[Oleksij Rempel]

timeout_cur will be used for watchdog poller.
Provided values should be good enough for most users
and still can be changed for separate projects and
fit to needed requirements.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/watchdog/wd_core.c | 15 +++++++++++++++
 include/watchdog.h         |  1 +
 2 files changed, 16 insertions(+)

diff --git a/drivers/watchdog/wd_core.c b/drivers/watchdog/wd_core.c
index 66dbd790d..3162dd59c 100644
--- a/drivers/watchdog/wd_core.c
+++ b/drivers/watchdog/wd_core.c
@@ -31,6 +31,16 @@ static const char *watchdog_name(struct watchdog *wd)
 	return "unknown";
 }
 
+static int watchdog_set_cur(struct param_d *param, void *priv)
+{
+	struct watchdog *wd = priv;
+
+	if (wd->timeout_cur > wd->timeout_max)
+		return -EINVAL;
+
+	return 0;
+}
+
 int watchdog_register(struct watchdog *wd)
 {
 
@@ -45,8 +55,13 @@ int watchdog_register(struct watchdog *wd)
 	if (!wd->timeout_max)
 		wd->timeout_max = 60 * 4;
 
+	if (!wd->timeout_cur || wd->timeout_cur > wd->timeout_max)
+		wd->timeout_cur = wd->timeout_max;
+
 	dev_add_param_uint32_ro(&wd->dev, "timeout_max",
 			&wd->timeout_max, "%u");
+	dev_add_param_uint32(&wd->dev, "timeout_cur", watchdog_set_cur, NULL,
+			&wd->timeout_cur, "%u", wd);
 
 	list_add_tail(&wd->list, &watchdog_list);
 
diff --git a/include/watchdog.h b/include/watchdog.h
index a2459d255..2f1874c19 100644
--- a/include/watchdog.h
+++ b/include/watchdog.h
@@ -20,6 +20,7 @@ struct watchdog {
 	struct device_d dev;
 	unsigned int priority;
 	unsigned int timeout_max;
+	unsigned int timeout_cur;
 	struct list_head list;
 };
 
-- 
2.16.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v1 6/6] watchdog: add watchdog poller

[Oleksij Rempel]

In some cases it is practical to supervise as match as possible of
barebox execution with watchdog (or multiple watchdogs). This
patch provide async poller for watchdog core framework which can
be enabled by user and store this configuration to nv.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/watchdog/wd_core.c | 54 ++++++++++++++++++++++++++++++++++++++++------
 include/watchdog.h         |  4 ++++
 2 files changed, 52 insertions(+), 6 deletions(-)

diff --git a/drivers/watchdog/wd_core.c b/drivers/watchdog/wd_core.c
index 3162dd59c..6f1cadaf8 100644
--- a/drivers/watchdog/wd_core.c
+++ b/drivers/watchdog/wd_core.c
@@ -31,6 +31,16 @@ static const char *watchdog_name(struct watchdog *wd)
 	return "unknown";
 }
 
+static int _watchdog_set_timeout(struct watchdog *wd, unsigned timeout)
+{
+	if (timeout > wd->timeout_max)
+		return -EINVAL;
+
+	pr_debug("setting timeout on %s to %ds\n", watchdog_name(wd), timeout);
+
+	return wd->set_timeout(wd, timeout);
+}
+
 static int watchdog_set_cur(struct param_d *param, void *priv)
 {
 	struct watchdog *wd = priv;
@@ -41,6 +51,37 @@ static int watchdog_set_cur(struct param_d *param, void *priv)
 	return 0;
 }
 
+static void watchdog_poller_cb(void *priv);
+
+static void watchdog_poller_start(struct watchdog *wd)
+{
+	_watchdog_set_timeout(wd, wd->timeout_cur);
+	poller_call_async(&wd->poller, 500 * MSECOND,
+			watchdog_poller_cb, wd);
+
+}
+
+static void watchdog_poller_cb(void *priv)
+{
+	struct watchdog *wd = priv;
+
+	if (wd->poller_enable)
+		watchdog_poller_start(wd);
+}
+
+static int watchdog_set_poller(struct param_d *param, void *priv)
+{
+	struct watchdog *wd = priv;
+
+
+	if (wd->poller_enable)
+		watchdog_poller_start(wd);
+	else
+		poller_async_cancel(&wd->poller);
+
+	return 0;
+}
+
 int watchdog_register(struct watchdog *wd)
 {
 
@@ -63,6 +104,10 @@ int watchdog_register(struct watchdog *wd)
 	dev_add_param_uint32(&wd->dev, "timeout_cur", watchdog_set_cur, NULL,
 			&wd->timeout_cur, "%u", wd);
 
+	poller_async_register(&wd->poller);
+	dev_add_param_bool(&wd->dev, "poller_enable", watchdog_set_poller, NULL,
+			&wd->poller_enable, wd);
+
 	list_add_tail(&wd->list, &watchdog_list);
 
 	pr_debug("registering watchdog %s with priority %d\n", watchdog_name(wd),
@@ -74,6 +119,8 @@ EXPORT_SYMBOL(watchdog_register);
 
 int watchdog_deregister(struct watchdog *wd)
 {
+	poller_async_cancel(&wd->poller);
+	poller_async_unregister(&wd->poller);
 	unregister_device(&wd->dev);
 	list_del(&wd->list);
 
@@ -109,12 +156,7 @@ int watchdog_set_timeout(unsigned timeout)
 	if (!wd)
 		return -ENODEV;
 
-	if (timeout > wd->timeout_max)
-		return -EINVAL;
-
-	pr_debug("setting timeout on %s to %ds\n", watchdog_name(wd), timeout);
-
-	return wd->set_timeout(wd, timeout);
+	return _watchdog_set_timeout(wd, timeout);
 }
 EXPORT_SYMBOL(watchdog_set_timeout);
 
diff --git a/include/watchdog.h b/include/watchdog.h
index 2f1874c19..0db4263a3 100644
--- a/include/watchdog.h
+++ b/include/watchdog.h
@@ -13,6 +13,8 @@
 #ifndef INCLUDE_WATCHDOG_H
 # define INCLUDE_WATCHDOG_H
 
+#include <poller.h>
+
 struct watchdog {
 	int (*set_timeout)(struct watchdog *, unsigned);
 	const char *name;
@@ -21,6 +23,8 @@ struct watchdog {
 	unsigned int priority;
 	unsigned int timeout_max;
 	unsigned int timeout_cur;
+	unsigned int poller_enable;
+	struct poller_async poller;
 	struct list_head list;
 };
 
-- 
2.16.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v1 6/6] watchdog: add watchdog poller

[Jan Lübbe]

Hi Oleksij,

On Thu, 2018-03-08 at 12:05 +0100, Oleksij Rempel wrote:
> In some cases it is practical to supervise as match as possible of
s/match/much/
> barebox execution with watchdog (or multiple watchdogs). This
 ^ the                  ^ a
> patch provide async poller for watchdog core framework which can
              ^-s an
> be enabled by user and store this configuration to nv.
               ^the           ^-s

Also, does this patch make the poller support required for using
watchdogs? It unconditionally calls the poller_* functions.

Also, it should be documented explicitly, that this will cause barebox
to keep triggering the watchdog, even when it drops to the shell after
a boot error. This makes it unsuitable for unattended use.

I'd prefer to make this optional at compile time.

> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/watchdog/wd_core.c | 54
> ++++++++++++++++++++++++++++++++++++++++------
>  include/watchdog.h         |  4 ++++
>  2 files changed, 52 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/watchdog/wd_core.c b/drivers/watchdog/wd_core.c
> index 3162dd59c..6f1cadaf8 100644
> --- a/drivers/watchdog/wd_core.c
> +++ b/drivers/watchdog/wd_core.c
> @@ -31,6 +31,16 @@ static const char *watchdog_name(struct watchdog
> *wd)
>  	return "unknown";
>  }
>  
> +static int _watchdog_set_timeout(struct watchdog *wd, unsigned
> timeout)
> +{
> +	if (timeout > wd->timeout_max)
> +		return -EINVAL;
> +
> +	pr_debug("setting timeout on %s to %ds\n",
> watchdog_name(wd), timeout);
> +
> +	return wd->set_timeout(wd, timeout);
> +}
> +
>  static int watchdog_set_cur(struct param_d *param, void *priv)
>  {
>  	struct watchdog *wd = priv;
> @@ -41,6 +51,37 @@ static int watchdog_set_cur(struct param_d *param,
> void *priv)
>  	return 0;
>  }
>  
> +static void watchdog_poller_cb(void *priv);
> +
> +static void watchdog_poller_start(struct watchdog *wd)
> +{
> +	_watchdog_set_timeout(wd, wd->timeout_cur);
> +	poller_call_async(&wd->poller, 500 * MSECOND,
> +			watchdog_poller_cb, wd);
> +
> +}
> +
> +static void watchdog_poller_cb(void *priv)
> +{
> +	struct watchdog *wd = priv;
> +
> +	if (wd->poller_enable)
> +		watchdog_poller_start(wd);
> +}
> +
> +static int watchdog_set_poller(struct param_d *param, void *priv)
> +{
> +	struct watchdog *wd = priv;
> +
> +
> +	if (wd->poller_enable)
> +		watchdog_poller_start(wd);
> +	else
> +		poller_async_cancel(&wd->poller);
> +
> +	return 0;
> +}
> +
>  int watchdog_register(struct watchdog *wd)
>  {
>  
> @@ -63,6 +104,10 @@ int watchdog_register(struct watchdog *wd)
>  	dev_add_param_uint32(&wd->dev, "timeout_cur",
> watchdog_set_cur, NULL,
>  			&wd->timeout_cur, "%u", wd);
>  
> +	poller_async_register(&wd->poller);
> +	dev_add_param_bool(&wd->dev, "poller_enable",
> watchdog_set_poller, NULL,
> +			&wd->poller_enable, wd);
Isn't the usage of the poller an implementation detail? Perhaps calls
this something like "auto_trigger"?

> +
>  	list_add_tail(&wd->list, &watchdog_list);
>  
>  	pr_debug("registering watchdog %s with priority %d\n",
> watchdog_name(wd),
> @@ -74,6 +119,8 @@ EXPORT_SYMBOL(watchdog_register);
>  
>  int watchdog_deregister(struct watchdog *wd)
>  {
> +	poller_async_cancel(&wd->poller);
> +	poller_async_unregister(&wd->poller);
>  	unregister_device(&wd->dev);
>  	list_del(&wd->list);
>  
> @@ -109,12 +156,7 @@ int watchdog_set_timeout(unsigned timeout)
>  	if (!wd)
>  		return -ENODEV;
>  
> -	if (timeout > wd->timeout_max)
> -		return -EINVAL;
> -
> -	pr_debug("setting timeout on %s to %ds\n",
> watchdog_name(wd), timeout);
> -
> -	return wd->set_timeout(wd, timeout);
> +	return _watchdog_set_timeout(wd, timeout);
>  }
>  EXPORT_SYMBOL(watchdog_set_timeout);
>  
> diff --git a/include/watchdog.h b/include/watchdog.h
> index 2f1874c19..0db4263a3 100644
> --- a/include/watchdog.h
> +++ b/include/watchdog.h
> @@ -13,6 +13,8 @@
>  #ifndef INCLUDE_WATCHDOG_H
>  # define INCLUDE_WATCHDOG_H
>  
> +#include <poller.h>
> +
>  struct watchdog {
>  	int (*set_timeout)(struct watchdog *, unsigned);
>  	const char *name;
> @@ -21,6 +23,8 @@ struct watchdog {
>  	unsigned int priority;
>  	unsigned int timeout_max;
>  	unsigned int timeout_cur;
> +	unsigned int poller_enable;
> +	struct poller_async poller;
>  	struct list_head list;
>  };
>  
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v1 6/6] watchdog: add watchdog poller

[Oleksij Rempel]

[-- Attachment #1.1.1: Type: text/plain, Size: 5819 bytes --]

Hi,

thank you for review.

Am 08.03.2018 um 14:49 schrieb Jan Lübbe:
> Hi Oleksij,
> 
> On Thu, 2018-03-08 at 12:05 +0100, Oleksij Rempel wrote:
>> In some cases it is practical to supervise as match as possible of
> s/match/much/
>> barebox execution with watchdog (or multiple watchdogs). This
>  ^ the                  ^ a
>> patch provide async poller for watchdog core framework which can
>               ^-s an
>> be enabled by user and store this configuration to nv.
>                ^the           ^-s
> 
> Also, does this patch make the poller support required for using
> watchdogs? It unconditionally calls the poller_* functions.
> 
> Also, it should be documented explicitly, that this will cause barebox
> to keep triggering the watchdog, even when it drops to the shell after
> a boot error. This makes it unsuitable for unattended use.

I would prefer to use controlled reboot over uncontrolled watchdog reset.
For example it would be better to have boot and fail strategy. In case
of network boot, it would be better to retry download in some time and
not cause watchdog reset. If retry count exceeded then some thing should
be done. It can be power off, reboot, fall back to CLI.

The reason for controlled reboot is the fact that the reset impact (or
Reset Sensitivity) is different for every product and source of reset.

This example is take from MiniRISC EZ4021-FC documentation:
				Soft				TAP Ctrl
Module			Reset	Reset	PrRst	ERst	TRST	Reset
CPU			yes	yes	yes	no	no	no
CP0			yes	yes	yes	no	no	no
ICCi			yes	yes	yes	no	no	no
DCC			yes	yes	yes	no	no	no
BIU			yes	yes	yes	no	no	no
MMU			yes	no	no	no	no	no
MDU			yes	yes	yes	no	no	no
EJTAG iface:
- DMA/CPU Acc		yes	yes	yes	yes	yes	yes
  logic	
- Protocol engine	yes	no	no	yes	yes	yes
- Breakpoint		yes	no	no	yes	no	no
- PC trace yes no no yes no no


Most Atheros/QCA WiSoCs will not reset complete SoC even with watchdog
triggered reset.


> I'd prefer to make this optional at compile time.
> 
>>
>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>> ---
>>  drivers/watchdog/wd_core.c | 54
>> ++++++++++++++++++++++++++++++++++++++++------
>>  include/watchdog.h         |  4 ++++
>>  2 files changed, 52 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/watchdog/wd_core.c b/drivers/watchdog/wd_core.c
>> index 3162dd59c..6f1cadaf8 100644
>> --- a/drivers/watchdog/wd_core.c
>> +++ b/drivers/watchdog/wd_core.c
>> @@ -31,6 +31,16 @@ static const char *watchdog_name(struct watchdog
>> *wd)
>>  	return "unknown";
>>  }
>>  
>> +static int _watchdog_set_timeout(struct watchdog *wd, unsigned
>> timeout)
>> +{
>> +	if (timeout > wd->timeout_max)
>> +		return -EINVAL;
>> +
>> +	pr_debug("setting timeout on %s to %ds\n",
>> watchdog_name(wd), timeout);
>> +
>> +	return wd->set_timeout(wd, timeout);
>> +}
>> +
>>  static int watchdog_set_cur(struct param_d *param, void *priv)
>>  {
>>  	struct watchdog *wd = priv;
>> @@ -41,6 +51,37 @@ static int watchdog_set_cur(struct param_d *param,
>> void *priv)
>>  	return 0;
>>  }
>>  
>> +static void watchdog_poller_cb(void *priv);
>> +
>> +static void watchdog_poller_start(struct watchdog *wd)
>> +{
>> +	_watchdog_set_timeout(wd, wd->timeout_cur);
>> +	poller_call_async(&wd->poller, 500 * MSECOND,
>> +			watchdog_poller_cb, wd);
>> +
>> +}
>> +
>> +static void watchdog_poller_cb(void *priv)
>> +{
>> +	struct watchdog *wd = priv;
>> +
>> +	if (wd->poller_enable)
>> +		watchdog_poller_start(wd);
>> +}
>> +
>> +static int watchdog_set_poller(struct param_d *param, void *priv)
>> +{
>> +	struct watchdog *wd = priv;
>> +
>> +
>> +	if (wd->poller_enable)
>> +		watchdog_poller_start(wd);
>> +	else
>> +		poller_async_cancel(&wd->poller);
>> +
>> +	return 0;
>> +}
>> +
>>  int watchdog_register(struct watchdog *wd)
>>  {
>>  
>> @@ -63,6 +104,10 @@ int watchdog_register(struct watchdog *wd)
>>  	dev_add_param_uint32(&wd->dev, "timeout_cur",
>> watchdog_set_cur, NULL,
>>  			&wd->timeout_cur, "%u", wd);
>>  
>> +	poller_async_register(&wd->poller);
>> +	dev_add_param_bool(&wd->dev, "poller_enable",
>> watchdog_set_poller, NULL,
>> +			&wd->poller_enable, wd);
> Isn't the usage of the poller an implementation detail? Perhaps calls
> this something like "auto_trigger"?
> 
>> +
>>  	list_add_tail(&wd->list, &watchdog_list);
>>  
>>  	pr_debug("registering watchdog %s with priority %d\n",
>> watchdog_name(wd),
>> @@ -74,6 +119,8 @@ EXPORT_SYMBOL(watchdog_register);
>>  
>>  int watchdog_deregister(struct watchdog *wd)
>>  {
>> +	poller_async_cancel(&wd->poller);
>> +	poller_async_unregister(&wd->poller);
>>  	unregister_device(&wd->dev);
>>  	list_del(&wd->list);
>>  
>> @@ -109,12 +156,7 @@ int watchdog_set_timeout(unsigned timeout)
>>  	if (!wd)
>>  		return -ENODEV;
>>  
>> -	if (timeout > wd->timeout_max)
>> -		return -EINVAL;
>> -
>> -	pr_debug("setting timeout on %s to %ds\n",
>> watchdog_name(wd), timeout);
>> -
>> -	return wd->set_timeout(wd, timeout);
>> +	return _watchdog_set_timeout(wd, timeout);
>>  }
>>  EXPORT_SYMBOL(watchdog_set_timeout);
>>  
>> diff --git a/include/watchdog.h b/include/watchdog.h
>> index 2f1874c19..0db4263a3 100644
>> --- a/include/watchdog.h
>> +++ b/include/watchdog.h
>> @@ -13,6 +13,8 @@
>>  #ifndef INCLUDE_WATCHDOG_H
>>  # define INCLUDE_WATCHDOG_H
>>  
>> +#include <poller.h>
>> +
>>  struct watchdog {
>>  	int (*set_timeout)(struct watchdog *, unsigned);
>>  	const char *name;
>> @@ -21,6 +23,8 @@ struct watchdog {
>>  	unsigned int priority;
>>  	unsigned int timeout_max;
>>  	unsigned int timeout_cur;
>> +	unsigned int poller_enable;
>> +	struct poller_async poller;
>>  	struct list_head list;
>>  };
>>  



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[-- Attachment #2: Type: text/plain, Size: 149 bytes --]

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v1 6/6] watchdog: add watchdog poller

[Jan Lübbe]

Hi Oleksij,

On Thu, 2018-03-08 at 15:16 +0100, Oleksij Rempel wrote:
> > Also, it should be documented explicitly, that this will cause barebox
> > to keep triggering the watchdog, even when it drops to the shell after
> > a boot error. This makes it unsuitable for unattended use.
> 
> I would prefer to use controlled reboot over uncontrolled watchdog reset.
> For example it would be better to have boot and fail strategy. In case
> of network boot, it would be better to retry download in some time and
> not cause watchdog reset. If retry count exceeded then some thing should
> be done. It can be power off, reboot, fall back to CLI.

In my experience, the watchdog is used as a last resort to handle any
*unanticipated* problems. So, by definition, there isn't any code to
handle these problems. The way to do this is that the watchdog is only
triggered when the boot process has made actual progress towards a
running system. For example:
- once barebox probes the watchdog driver
- from the shell init scripts
- after loading the kernel, just before jumping to the kernel

This way, there is no possible way which could cause barebox to just
wait on the prompt: an idle or hung system will always be restarted via
the watchdog.

> The reason for controlled reboot is the fact that the reset impact (or
> Reset Sensitivity) is different for every product and source of reset.
> 
> This example is take from MiniRISC EZ4021-FC documentation:
> 				Soft				TAP Ctrl
> Module		Reset	Reset	PrRst	ERst	TRST	Reset
> CPU			yes	yes	yes	no	no	no
> CP0			yes	yes	yes	no	no	no
> ICCi			yes	yes	yes	no	no	no
> DCC			yes	yes	yes	no	no	no
> BIU			yes	yes	yes	no	no	no
> MMU			yes	no	no	no	no	no
> MDU			yes	yes	yes	no	no	no
> EJTAG iface:
> - DMA/CPU Acc		yes	yes	yes	yes	yes	yes
>   logic	
> - Protocol engine	yes	no	no	yes	yes	yes
> - Breakpoint		yes	no	no	yes	no	no
> - PC trace yes no no yes no no

It is not clear to me from this table which reset is triggered by the
hardware watchdog. I would expect that it is the first column, which
resets everything.

> Most Atheros/QCA WiSoCs will not reset complete SoC even with watchdog
> triggered reset.

If you can't be sure that the watchdog resets enough to recover from
any transient problem, you cannot rely on it at all (and should
possibly use an external watchdog).

Regards,
Jan
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v1 3/6] watchdog: register watchdog virtual device with short name wdog

[Sascha Hauer]

Hi Oleksij,

On Thu, Mar 08, 2018 at 12:05:12PM +0100, Oleksij Rempel wrote:
> the watchdog hwdev is usually named with devicetree schema
> which is not practical for CLI.
> 
> On device registration "wdog" will be extended with some index number
> extracted from devicetree (if awailable) or automatically assigned
> first available number. End result will be "wdog0" .. etc.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/watchdog/wd_core.c | 6 ++++++
>  include/watchdog.h         | 1 +
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/watchdog/wd_core.c b/drivers/watchdog/wd_core.c
> index 1d0f0de7d..97e2ab568 100644
> --- a/drivers/watchdog/wd_core.c
> +++ b/drivers/watchdog/wd_core.c
> @@ -33,6 +33,11 @@ static const char *watchdog_name(struct watchdog *wd)
>  
>  int watchdog_register(struct watchdog *wd)
>  {
> +

Please drop that blank line.

> +	strcpy(wd->dev.name, "wdog");

I would prefer a of_alias_get() as first try and "wdog" only as
fallback.

For the hardcoded string wd->dev.id needs to be initialized with
DEVICE_ID_DYNAMIC, otherwise only a single watchdog can be successfully
registered.

> +	wd->dev.parent = wd->hwdev;
> +	register_device(&wd->dev);

Check return value.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v1 4/6] watchdog: set some reasonable timeout_max value if no other is available

[Sascha Hauer]

On Thu, Mar 08, 2018 at 12:05:13PM +0100, Oleksij Rempel wrote:
> Some drivers do not provide timeout_max value.
> Using some value is probbaly better then setting timeout_max to 0.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/watchdog/wd_core.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/watchdog/wd_core.c b/drivers/watchdog/wd_core.c
> index 97e2ab568..66dbd790d 100644
> --- a/drivers/watchdog/wd_core.c
> +++ b/drivers/watchdog/wd_core.c
> @@ -41,6 +41,10 @@ int watchdog_register(struct watchdog *wd)
>  	if (!wd->priority)
>  		wd->priority = WATCHDOG_DEFAULT_PRIORITY;
>  
> +	/* set some default sane value */
> +	if (!wd->timeout_max)
> +		wd->timeout_max = 60 * 4;

4 minutes? Why so low? I would expect something like a few hours.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox