[PATCH 2/2] watchdog: stmp3xxx: Implement GETBOOTSTATUS

[PATCH 2/2] watchdog: stmp3xxx: Implement GETBOOTSTATUS

[Harald Geyer]

When the watchdog is enabled, we set a persitent bit. After booting
we query the bit and see if the system was reset by the watchdog.

This is somewhat similar to what the legacy driver from freescale
does. However we use STMP3XXX_RTC_PERSISTENT2 instead of
STMP3XXX_RTC_PERSISTENT1. I tried that first, but it seems I can't
clear the bit there once it is set. I didn't find any documentation
what this register does - only vague hints that it is meant to
control the boot ROM.

Part of the code from the legacy driver touching this register
is still included. Maybe this is stale, but this patch doesn't
touch any of it because I don't know what it really does or is
meant to do.

Signed-off-by: Harald Geyer <harald@ccbib.org>
---
Changes since initially posting this patch on 08/04/2015:
* fix a spelling error in the commit message
* rebase to a recent version

 drivers/rtc/rtc-stmp3xxx.c          |   23 +++++++++++++++++++++++
 drivers/watchdog/stmp3xxx_rtc_wdt.c |   34 +++++++++++++++++++++++++++++++++-
 include/linux/stmp3xxx_rtc_wdt.h    |    2 ++
 3 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-stmp3xxx.c b/drivers/rtc/rtc-stmp3xxx.c
index ca54d03..5d68090 100644
--- a/drivers/rtc/rtc-stmp3xxx.c
+++ b/drivers/rtc/rtc-stmp3xxx.c
@@ -30,6 +30,7 @@
 #include <linux/of.h>
 #include <linux/stmp_device.h>
 #include <linux/stmp3xxx_rtc_wdt.h>
+#include <linux/watchdog.h>
 
 #define STMP3XXX_RTC_CTRL			0x0
 #define STMP3XXX_RTC_CTRL_ALARM_IRQ_EN		0x00000001
@@ -62,6 +63,9 @@
 /* missing bitmask in headers */
 #define STMP3XXX_RTC_PERSISTENT1_FORCE_UPDATER	0x80000000
 
+#define STMP3XXX_RTC_PERSISTENT2		0x80
+#define STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE	0x00000001
+
 struct stmp3xxx_rtc_data {
 	struct rtc_device *rtc;
 	void __iomem *io;
@@ -83,6 +87,14 @@ struct stmp3xxx_rtc_data {
  * registers is atomic.
  */
 
+static void stmp3xxx_wdt_clear_bootstatus(struct device *dev)
+{
+	struct stmp3xxx_rtc_data *rtc_data = dev_get_drvdata(dev);
+
+	writel(STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE,
+	       STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_CLR + rtc_data->io);
+}
+
 static void stmp3xxx_wdt_set_timeout(struct device *dev, u32 timeout)
 {
 	struct stmp3xxx_rtc_data *rtc_data = dev_get_drvdata(dev);
@@ -93,16 +105,22 @@ static void stmp3xxx_wdt_set_timeout(struct device *dev, u32 timeout)
 		       rtc_data->io + STMP3XXX_RTC_CTRL + STMP_OFFSET_REG_SET);
 		writel(STMP3XXX_RTC_PERSISTENT1_FORCE_UPDATER,
 		       rtc_data->io + STMP3XXX_RTC_PERSISTENT1 + STMP_OFFSET_REG_SET);
+		writel(STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE,
+		       rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_SET);
 	} else {
 		writel(STMP3XXX_RTC_CTRL_WATCHDOGEN,
 		       rtc_data->io + STMP3XXX_RTC_CTRL + STMP_OFFSET_REG_CLR);
 		writel(STMP3XXX_RTC_PERSISTENT1_FORCE_UPDATER,
 		       rtc_data->io + STMP3XXX_RTC_PERSISTENT1 + STMP_OFFSET_REG_CLR);
+		writel(STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE,
+		       rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_CLR);
 	}
 }
 
 static struct stmp3xxx_wdt_pdata wdt_pdata = {
 	.wdt_set_timeout = stmp3xxx_wdt_set_timeout,
+	.wdt_clear_bootstatus = stmp3xxx_wdt_clear_bootstatus,
+	.bootstatus = 0,
 };
 
 static void stmp3xxx_wdt_register(struct platform_device *rtc_pdev)
@@ -110,6 +128,8 @@ static void stmp3xxx_wdt_register(struct platform_device *rtc_pdev)
 	struct platform_device *wdt_pdev =
 		platform_device_alloc("stmp3xxx_rtc_wdt", rtc_pdev->id);
 
+	stmp3xxx_wdt_clear_bootstatus(&rtc_pdev->dev);
+
 	if (wdt_pdev) {
 		wdt_pdev->dev.parent = &rtc_pdev->dev;
 		wdt_pdev->dev.platform_data = &wdt_pdata;
@@ -357,6 +377,9 @@ static int stmp3xxx_rtc_probe(struct platform_device *pdev)
 		return err;
 	}
 
+	if (readl(STMP3XXX_RTC_PERSISTENT2 + rtc_data->io) &
+			 STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE)
+		wdt_pdata.bootstatus |= WDIOF_CARDRESET;
 	stmp3xxx_wdt_register(pdev);
 	return 0;
 }
diff --git a/drivers/watchdog/stmp3xxx_rtc_wdt.c b/drivers/watchdog/stmp3xxx_rtc_wdt.c
index 3ee6128..7609f78 100644
--- a/drivers/watchdog/stmp3xxx_rtc_wdt.c
+++ b/drivers/watchdog/stmp3xxx_rtc_wdt.c
@@ -14,6 +14,8 @@
 #include <linux/watchdog.h>
 #include <linux/platform_device.h>
 #include <linux/stmp3xxx_rtc_wdt.h>
+#include <linux/notifier.h>
+#include <linux/reboot.h>
 
 #define WDOG_TICK_RATE 1000 /* 1 kHz clock */
 #define STMP3XXX_DEFAULT_TIMEOUT 19
@@ -50,7 +52,8 @@ static int wdt_set_timeout(struct watchdog_device *wdd, unsigned new_timeout)
 }
 
 static const struct watchdog_info stmp3xxx_wdt_ident = {
-	.options = WDIOF_MAGICCLOSE | WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
+	.options = WDIOF_MAGICCLOSE | WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
+		WDIOF_CARDRESET,
 	.identity = "STMP3XXX RTC Watchdog",
 };
 
@@ -69,14 +72,39 @@ static struct watchdog_device stmp3xxx_wdd = {
 	.status = WATCHDOG_NOWAYOUT_INIT_STATUS,
 };
 
+static int wdt_notify_sys(struct notifier_block *nb, unsigned long code,
+			  void *unused)
+{
+	struct device *dev = watchdog_get_drvdata(&stmp3xxx_wdd);
+	struct stmp3xxx_wdt_pdata *pdata = dev_get_platdata(dev);
+
+	switch (code) {
+	case SYS_DOWN:	/* keep enabled, system might crash while going down */
+		pdata->wdt_clear_bootstatus(dev->parent);
+		break;
+	case SYS_HALT:  /* allow the system to actually halt */
+	case SYS_POWER_OFF:
+		wdt_stop(&stmp3xxx_wdd);
+		break;
+	}
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block wdt_notifier = {
+	.notifier_call  = wdt_notify_sys,
+};
+
 static int stmp3xxx_wdt_probe(struct platform_device *pdev)
 {
+	struct stmp3xxx_wdt_pdata *pdata = dev_get_platdata(&pdev->dev);
 	int ret;
 
 	watchdog_set_drvdata(&stmp3xxx_wdd, &pdev->dev);
 
 	stmp3xxx_wdd.timeout = clamp_t(unsigned, heartbeat, 1, STMP3XXX_MAX_TIMEOUT);
 	stmp3xxx_wdd.parent = &pdev->dev;
+	stmp3xxx_wdd.bootstatus = pdata->bootstatus;
 
 	ret = watchdog_register_device(&stmp3xxx_wdd);
 	if (ret < 0) {
@@ -84,6 +112,9 @@ static int stmp3xxx_wdt_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	if (register_reboot_notifier(&wdt_notifier))
+		dev_warn(&pdev->dev, "cannot register reboot notifier\n");
+
 	dev_info(&pdev->dev, "initialized watchdog with heartbeat %ds\n",
 			stmp3xxx_wdd.timeout);
 	return 0;
@@ -91,6 +122,7 @@ static int stmp3xxx_wdt_probe(struct platform_device *pdev)
 
 static int stmp3xxx_wdt_remove(struct platform_device *pdev)
 {
+	unregister_reboot_notifier(&wdt_notifier);
 	watchdog_unregister_device(&stmp3xxx_wdd);
 	return 0;
 }
diff --git a/include/linux/stmp3xxx_rtc_wdt.h b/include/linux/stmp3xxx_rtc_wdt.h
index 1dd12c9..62dd9e6 100644
--- a/include/linux/stmp3xxx_rtc_wdt.h
+++ b/include/linux/stmp3xxx_rtc_wdt.h
@@ -10,6 +10,8 @@
 
 struct stmp3xxx_wdt_pdata {
 	void (*wdt_set_timeout)(struct device *dev, u32 timeout);
+	void (*wdt_clear_bootstatus)(struct device *dev);
+	unsigned int bootstatus;
 };
 
 #endif /* __LINUX_STMP3XXX_RTC_WDT_H */
-- 
1.7.10.4

Re: [PATCH 2/2] watchdog: stmp3xxx: Implement GETBOOTSTATUS

[Alexandre Belloni]

Hi,

On 02/10/2015 at 20:18:02 +0000, Harald Geyer wrote :
> +static void stmp3xxx_wdt_clear_bootstatus(struct device *dev)
> +{
> +	struct stmp3xxx_rtc_data *rtc_data = dev_get_drvdata(dev);
> +
> +	writel(STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE,
> +	       STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_CLR + rtc_data->io);

nitpick: I would put rtc_data->io first here for consistency.

Else, this seems fine to me, I can take that patch with the watchdog
maintainers ack.
-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCHv3 1/2] watchdog: stmp3xxx: Stop the watchdog on system halt

[Harald Geyer]

This allows the system to actually halt even if userspace forgot to
disable the watchdog first. Old behaviour was that the watchdog forced
the system to boot again.

Signed-off-by: Harald Geyer <harald@ccbib.org>
---
Changes since v2:
* split this out as a separate patch

 drivers/watchdog/stmp3xxx_rtc_wdt.c |   28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/watchdog/stmp3xxx_rtc_wdt.c b/drivers/watchdog/stmp3xxx_rtc_wdt.c
index 3ee6128..e09a01f 100644
--- a/drivers/watchdog/stmp3xxx_rtc_wdt.c
+++ b/drivers/watchdog/stmp3xxx_rtc_wdt.c
@@ -14,6 +14,8 @@
 #include <linux/watchdog.h>
 #include <linux/platform_device.h>
 #include <linux/stmp3xxx_rtc_wdt.h>
+#include <linux/notifier.h>
+#include <linux/reboot.h>
 
 #define WDOG_TICK_RATE 1000 /* 1 kHz clock */
 #define STMP3XXX_DEFAULT_TIMEOUT 19
@@ -69,6 +71,28 @@ static struct watchdog_device stmp3xxx_wdd = {
 	.status = WATCHDOG_NOWAYOUT_INIT_STATUS,
 };
 
+static int wdt_notify_sys(struct notifier_block *nb, unsigned long code,
+			  void *unused)
+{
+	struct device *dev = watchdog_get_drvdata(&stmp3xxx_wdd);
+	struct stmp3xxx_wdt_pdata *pdata = dev_get_platdata(dev);
+
+	switch (code) {
+	case SYS_DOWN:	/* keep enabled, system might crash while going down */
+		break;
+	case SYS_HALT:  /* allow the system to actually halt */
+	case SYS_POWER_OFF:
+		wdt_stop(&stmp3xxx_wdd);
+		break;
+	}
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block wdt_notifier = {
+	.notifier_call  = wdt_notify_sys,
+};
+
 static int stmp3xxx_wdt_probe(struct platform_device *pdev)
 {
 	int ret;
@@ -84,6 +108,9 @@ static int stmp3xxx_wdt_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	if (register_reboot_notifier(&wdt_notifier))
+		dev_warn(&pdev->dev, "cannot register reboot notifier\n");
+
 	dev_info(&pdev->dev, "initialized watchdog with heartbeat %ds\n",
 			stmp3xxx_wdd.timeout);
 	return 0;
@@ -91,6 +118,7 @@ static int stmp3xxx_wdt_probe(struct platform_device *pdev)
 
 static int stmp3xxx_wdt_remove(struct platform_device *pdev)
 {
+	unregister_reboot_notifier(&wdt_notifier);
 	watchdog_unregister_device(&stmp3xxx_wdd);
 	return 0;
 }
-- 
1.7.10.4

[PATCH 2/2] watchdog: stmp3xxx: Implement GETBOOTSTATUS

[Harald Geyer]

This device doesn't provide any information about boot status.
As workaround we use a persitent bit to track watchdog activity.

Signed-off-by: Harald Geyer <harald@ccbib.org>
---
Changes since v2:
* make code ordering more consistent
* move part of the commit message to a code comment
* rewrite the commit message

Changes since v1:
* make code formatting more consistent with the rest of the driver
* Cc some people who might have better documentation then I do

Changes since initially posting this patch on 08/04/2015:
* fix a spelling error in the commit message
* rebase to a recent version

 drivers/rtc/rtc-stmp3xxx.c          |   37 +++++++++++++++++++++++++++++++++++
 drivers/watchdog/stmp3xxx_rtc_wdt.c |    6 +++++-
 include/linux/stmp3xxx_rtc_wdt.h    |    2 ++
 3 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-stmp3xxx.c b/drivers/rtc/rtc-stmp3xxx.c
index ca54d03..47947ea 100644
--- a/drivers/rtc/rtc-stmp3xxx.c
+++ b/drivers/rtc/rtc-stmp3xxx.c
@@ -30,6 +30,7 @@
 #include <linux/of.h>
 #include <linux/stmp_device.h>
 #include <linux/stmp3xxx_rtc_wdt.h>
+#include <linux/watchdog.h>
 
 #define STMP3XXX_RTC_CTRL			0x0
 #define STMP3XXX_RTC_CTRL_ALARM_IRQ_EN		0x00000001
@@ -62,6 +63,9 @@
 /* missing bitmask in headers */
 #define STMP3XXX_RTC_PERSISTENT1_FORCE_UPDATER	0x80000000
 
+#define STMP3XXX_RTC_PERSISTENT2		0x80
+#define STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE	0x00000001
+
 struct stmp3xxx_rtc_data {
 	struct rtc_device *rtc;
 	void __iomem *io;
@@ -81,6 +85,20 @@ struct stmp3xxx_rtc_data {
  * The watchdog driver is passed the below accessor function via platform_data
  * to configure the watchdog. Locking is not needed because accessing SET/CLR
  * registers is atomic.
+ *
+ * Since this device doesn't report the cause of the last reset, we use
+ * a persistent bit to track watchdog activity. The code from the Freescale
+ * BSP uses the STMP3XXX_RTC_PERSISTENT1 register, which is dedicated to
+ * controlling the boot ROM, for this purpose. However it seems the bit
+ * there can't be cleared once it has been set. So we are using
+ * STMP3XXX_RTC_PERSISTENT2 instead, which is the first register available
+ * for "software use" without restriction.
+ *
+ * I (Harald Geyer <harald@ccbib.org>) don't know if the code touching the
+ * STMP3XXX_RTC_PERSISTENT1 register is doing anything useful. Maybe this
+ * is just a leftover from the BSP code, but then maybe there is something
+ * in the boot ROM or in some bootloader that is using this. Hard to tell
+ * without proper documentation about this register.
  */
 
 static void stmp3xxx_wdt_set_timeout(struct device *dev, u32 timeout)
@@ -93,16 +111,30 @@ static void stmp3xxx_wdt_set_timeout(struct device *dev, u32 timeout)
 		       rtc_data->io + STMP3XXX_RTC_CTRL + STMP_OFFSET_REG_SET);
 		writel(STMP3XXX_RTC_PERSISTENT1_FORCE_UPDATER,
 		       rtc_data->io + STMP3XXX_RTC_PERSISTENT1 + STMP_OFFSET_REG_SET);
+		writel(STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE,
+		       rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_SET);
 	} else {
 		writel(STMP3XXX_RTC_CTRL_WATCHDOGEN,
 		       rtc_data->io + STMP3XXX_RTC_CTRL + STMP_OFFSET_REG_CLR);
 		writel(STMP3XXX_RTC_PERSISTENT1_FORCE_UPDATER,
 		       rtc_data->io + STMP3XXX_RTC_PERSISTENT1 + STMP_OFFSET_REG_CLR);
+		writel(STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE,
+		       rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_CLR);
 	}
 }
 
+static void stmp3xxx_wdt_clear_bootstatus(struct device *dev)
+{
+	struct stmp3xxx_rtc_data *rtc_data = dev_get_drvdata(dev);
+
+	writel(STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE,
+	       rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_CLR);
+}
+
 static struct stmp3xxx_wdt_pdata wdt_pdata = {
 	.wdt_set_timeout = stmp3xxx_wdt_set_timeout,
+	.wdt_clear_bootstatus = stmp3xxx_wdt_clear_bootstatus,
+	.bootstatus = 0,
 };
 
 static void stmp3xxx_wdt_register(struct platform_device *rtc_pdev)
@@ -110,6 +142,8 @@ static void stmp3xxx_wdt_register(struct platform_device *rtc_pdev)
 	struct platform_device *wdt_pdev =
 		platform_device_alloc("stmp3xxx_rtc_wdt", rtc_pdev->id);
 
+	stmp3xxx_wdt_clear_bootstatus(&rtc_pdev->dev);
+
 	if (wdt_pdev) {
 		wdt_pdev->dev.parent = &rtc_pdev->dev;
 		wdt_pdev->dev.platform_data = &wdt_pdata;
@@ -357,6 +391,9 @@ static int stmp3xxx_rtc_probe(struct platform_device *pdev)
 		return err;
 	}
 
+	if (readl(STMP3XXX_RTC_PERSISTENT2 + rtc_data->io) &
+			 STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE)
+		wdt_pdata.bootstatus |= WDIOF_CARDRESET;
 	stmp3xxx_wdt_register(pdev);
 	return 0;
 }
diff --git a/drivers/watchdog/stmp3xxx_rtc_wdt.c b/drivers/watchdog/stmp3xxx_rtc_wdt.c
index e09a01f..7609f78 100644
--- a/drivers/watchdog/stmp3xxx_rtc_wdt.c
+++ b/drivers/watchdog/stmp3xxx_rtc_wdt.c
@@ -52,7 +52,8 @@ static int wdt_set_timeout(struct watchdog_device *wdd, unsigned new_timeout)
 }
 
 static const struct watchdog_info stmp3xxx_wdt_ident = {
-	.options = WDIOF_MAGICCLOSE | WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
+	.options = WDIOF_MAGICCLOSE | WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
+		WDIOF_CARDRESET,
 	.identity = "STMP3XXX RTC Watchdog",
 };
 
@@ -79,6 +80,7 @@ static int wdt_notify_sys(struct notifier_block *nb, unsigned long code,
 
 	switch (code) {
 	case SYS_DOWN:	/* keep enabled, system might crash while going down */
+		pdata->wdt_clear_bootstatus(dev->parent);
 		break;
 	case SYS_HALT:  /* allow the system to actually halt */
 	case SYS_POWER_OFF:
@@ -95,12 +97,14 @@ static struct notifier_block wdt_notifier = {
 
 static int stmp3xxx_wdt_probe(struct platform_device *pdev)
 {
+	struct stmp3xxx_wdt_pdata *pdata = dev_get_platdata(&pdev->dev);
 	int ret;
 
 	watchdog_set_drvdata(&stmp3xxx_wdd, &pdev->dev);
 
 	stmp3xxx_wdd.timeout = clamp_t(unsigned, heartbeat, 1, STMP3XXX_MAX_TIMEOUT);
 	stmp3xxx_wdd.parent = &pdev->dev;
+	stmp3xxx_wdd.bootstatus = pdata->bootstatus;
 
 	ret = watchdog_register_device(&stmp3xxx_wdd);
 	if (ret < 0) {
diff --git a/include/linux/stmp3xxx_rtc_wdt.h b/include/linux/stmp3xxx_rtc_wdt.h
index 1dd12c9..62dd9e6 100644
--- a/include/linux/stmp3xxx_rtc_wdt.h
+++ b/include/linux/stmp3xxx_rtc_wdt.h
@@ -10,6 +10,8 @@
 
 struct stmp3xxx_wdt_pdata {
 	void (*wdt_set_timeout)(struct device *dev, u32 timeout);
+	void (*wdt_clear_bootstatus)(struct device *dev);
+	unsigned int bootstatus;
 };
 
 #endif /* __LINUX_STMP3XXX_RTC_WDT_H */
-- 
1.7.10.4

Re: [PATCHv3,1/2] watchdog: stmp3xxx: Stop the watchdog on system halt

[Guenter Roeck]

On Wed, Oct 07, 2015 at 12:19:11PM +0000, Harald Geyer wrote:
> This allows the system to actually halt even if userspace forgot to
> disable the watchdog first. Old behaviour was that the watchdog forced
> the system to boot again.
> 
> Signed-off-by: Harald Geyer <harald@ccbib.org>

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

> ---
> Changes since v2:
> * split this out as a separate patch
> 
>  drivers/watchdog/stmp3xxx_rtc_wdt.c |   28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/watchdog/stmp3xxx_rtc_wdt.c b/drivers/watchdog/stmp3xxx_rtc_wdt.c
> index 3ee6128..e09a01f 100644
> --- a/drivers/watchdog/stmp3xxx_rtc_wdt.c
> +++ b/drivers/watchdog/stmp3xxx_rtc_wdt.c
> @@ -14,6 +14,8 @@
>  #include <linux/watchdog.h>
>  #include <linux/platform_device.h>
>  #include <linux/stmp3xxx_rtc_wdt.h>
> +#include <linux/notifier.h>
> +#include <linux/reboot.h>
>  
>  #define WDOG_TICK_RATE 1000 /* 1 kHz clock */
>  #define STMP3XXX_DEFAULT_TIMEOUT 19
> @@ -69,6 +71,28 @@ static struct watchdog_device stmp3xxx_wdd = {
>  	.status = WATCHDOG_NOWAYOUT_INIT_STATUS,
>  };
>  
> +static int wdt_notify_sys(struct notifier_block *nb, unsigned long code,
> +			  void *unused)
> +{
> +	struct device *dev = watchdog_get_drvdata(&stmp3xxx_wdd);
> +	struct stmp3xxx_wdt_pdata *pdata = dev_get_platdata(dev);
> +
> +	switch (code) {
> +	case SYS_DOWN:	/* keep enabled, system might crash while going down */
> +		break;
> +	case SYS_HALT:  /* allow the system to actually halt */
> +	case SYS_POWER_OFF:
> +		wdt_stop(&stmp3xxx_wdd);
> +		break;
> +	}
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block wdt_notifier = {
> +	.notifier_call  = wdt_notify_sys,
> +};
> +
>  static int stmp3xxx_wdt_probe(struct platform_device *pdev)
>  {
>  	int ret;
> @@ -84,6 +108,9 @@ static int stmp3xxx_wdt_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	if (register_reboot_notifier(&wdt_notifier))
> +		dev_warn(&pdev->dev, "cannot register reboot notifier\n");
> +
>  	dev_info(&pdev->dev, "initialized watchdog with heartbeat %ds\n",
>  			stmp3xxx_wdd.timeout);
>  	return 0;
> @@ -91,6 +118,7 @@ static int stmp3xxx_wdt_probe(struct platform_device *pdev)
>  
>  static int stmp3xxx_wdt_remove(struct platform_device *pdev)
>  {
> +	unregister_reboot_notifier(&wdt_notifier);
>  	watchdog_unregister_device(&stmp3xxx_wdd);
>  	return 0;
>  }

Re: [PATCHv3 1/2] watchdog: stmp3xxx: Stop the watchdog on system halt

[Uwe Kleine-König]

Hello,

On Wed, Oct 07, 2015 at 12:19:11PM +0000, Harald Geyer wrote:
> This allows the system to actually halt even if userspace forgot to
> disable the watchdog first. Old behaviour was that the watchdog forced
> the system to boot again.
> 
> Signed-off-by: Harald Geyer <harald@ccbib.org>
> ---
> Changes since v2:
> * split this out as a separate patch
> 
>  drivers/watchdog/stmp3xxx_rtc_wdt.c |   28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/watchdog/stmp3xxx_rtc_wdt.c b/drivers/watchdog/stmp3xxx_rtc_wdt.c
> index 3ee6128..e09a01f 100644
> --- a/drivers/watchdog/stmp3xxx_rtc_wdt.c
> +++ b/drivers/watchdog/stmp3xxx_rtc_wdt.c
> @@ -14,6 +14,8 @@
>  #include <linux/watchdog.h>
>  #include <linux/platform_device.h>
>  #include <linux/stmp3xxx_rtc_wdt.h>
> +#include <linux/notifier.h>
> +#include <linux/reboot.h>
>  
>  #define WDOG_TICK_RATE 1000 /* 1 kHz clock */
>  #define STMP3XXX_DEFAULT_TIMEOUT 19
> @@ -69,6 +71,28 @@ static struct watchdog_device stmp3xxx_wdd = {
>  	.status = WATCHDOG_NOWAYOUT_INIT_STATUS,
>  };
>  
> +static int wdt_notify_sys(struct notifier_block *nb, unsigned long code,
> +			  void *unused)
> +{
> +	struct device *dev = watchdog_get_drvdata(&stmp3xxx_wdd);
> +	struct stmp3xxx_wdt_pdata *pdata = dev_get_platdata(dev);
> +
> +	switch (code) {
> +	case SYS_DOWN:	/* keep enabled, system might crash while going down */
> +		break;
> +	case SYS_HALT:  /* allow the system to actually halt */

<nitpick>
You used a tab before the comment "keep enabled ...", but spaces for
"allow the system ...".
</nitpick>

> +	case SYS_POWER_OFF:
> +		wdt_stop(&stmp3xxx_wdd);
> +		break;
> +	}
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block wdt_notifier = {
> +	.notifier_call  = wdt_notify_sys,

s/  / /

> +};
> +
>  static int stmp3xxx_wdt_probe(struct platform_device *pdev)
>  {
>  	int ret;
> @@ -84,6 +108,9 @@ static int stmp3xxx_wdt_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	if (register_reboot_notifier(&wdt_notifier))
> +		dev_warn(&pdev->dev, "cannot register reboot notifier\n");

OK, you don't fail when registering the notifier fails, but ...

>  	dev_info(&pdev->dev, "initialized watchdog with heartbeat %ds\n",
>  			stmp3xxx_wdd.timeout);
>  	return 0;
> @@ -91,6 +118,7 @@ static int stmp3xxx_wdt_probe(struct platform_device *pdev)
>  
>  static int stmp3xxx_wdt_remove(struct platform_device *pdev)
>  {
> +	unregister_reboot_notifier(&wdt_notifier);

... I think it's not ok to unconditionally unregister it then, is it?
(Looking at how unregister_reboot_notifier is implemented it might work
as intended, but if that's only by chance I wouldn't rely on it.)

Best regards
Uwe

>  	watchdog_unregister_device(&stmp3xxx_wdd);
>  	return 0;
>  }
> -- 
> 1.7.10.4
> 
> 
> 

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

Re: [PATCH 2/2] watchdog: stmp3xxx: Implement GETBOOTSTATUS

[Uwe Kleine-König]

On Wed, Oct 07, 2015 at 12:19:12PM +0000, Harald Geyer wrote:
> This device doesn't provide any information about boot status.
> As workaround we use a persitent bit to track watchdog activity.

Hmm, so you set a bit in a persistent register iff the watchdog is
running.  And if that bit is set during probe you assume the watchdog
reset the machine. But this also happens, when the user presses the
reset button which makes the detection unreliable.

> 
> Signed-off-by: Harald Geyer <harald@ccbib.org>
> ---
> Changes since v2:
> * make code ordering more consistent
> * move part of the commit message to a code comment
> * rewrite the commit message
> 
> Changes since v1:
> * make code formatting more consistent with the rest of the driver
> * Cc some people who might have better documentation then I do
> 
> Changes since initially posting this patch on 08/04/2015:
> * fix a spelling error in the commit message
> * rebase to a recent version
> 
>  drivers/rtc/rtc-stmp3xxx.c          |   37 +++++++++++++++++++++++++++++++++++
>  drivers/watchdog/stmp3xxx_rtc_wdt.c |    6 +++++-
>  include/linux/stmp3xxx_rtc_wdt.h    |    2 ++
>  3 files changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/rtc/rtc-stmp3xxx.c b/drivers/rtc/rtc-stmp3xxx.c
> index ca54d03..47947ea 100644
> --- a/drivers/rtc/rtc-stmp3xxx.c
> +++ b/drivers/rtc/rtc-stmp3xxx.c
> @@ -30,6 +30,7 @@
>  #include <linux/of.h>
>  #include <linux/stmp_device.h>
>  #include <linux/stmp3xxx_rtc_wdt.h>
> +#include <linux/watchdog.h>
>  
>  #define STMP3XXX_RTC_CTRL			0x0
>  #define STMP3XXX_RTC_CTRL_ALARM_IRQ_EN		0x00000001
> @@ -62,6 +63,9 @@
>  /* missing bitmask in headers */
>  #define STMP3XXX_RTC_PERSISTENT1_FORCE_UPDATER	0x80000000
>  
> +#define STMP3XXX_RTC_PERSISTENT2		0x80
> +#define STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE	0x00000001
> +
>  struct stmp3xxx_rtc_data {
>  	struct rtc_device *rtc;
>  	void __iomem *io;
> @@ -81,6 +85,20 @@ struct stmp3xxx_rtc_data {
>   * The watchdog driver is passed the below accessor function via platform_data
>   * to configure the watchdog. Locking is not needed because accessing SET/CLR
>   * registers is atomic.
> + *
> + * Since this device doesn't report the cause of the last reset, we use
> + * a persistent bit to track watchdog activity. The code from the Freescale
> + * BSP uses the STMP3XXX_RTC_PERSISTENT1 register, which is dedicated to
> + * controlling the boot ROM, for this purpose. However it seems the bit
> + * there can't be cleared once it has been set. So we are using
> + * STMP3XXX_RTC_PERSISTENT2 instead, which is the first register available
> + * for "software use" without restriction.

Adding a link to the vendor kernel might be interesting here.

> + * I (Harald Geyer <harald@ccbib.org>) don't know if the code touching the
> + * STMP3XXX_RTC_PERSISTENT1 register is doing anything useful. Maybe this
> + * is just a leftover from the BSP code, but then maybe there is something
> + * in the boot ROM or in some bootloader that is using this. Hard to tell
> + * without proper documentation about this register.

Fabio: anything you can do here to help?

>   */
>  
>  static void stmp3xxx_wdt_set_timeout(struct device *dev, u32 timeout)
> @@ -93,16 +111,30 @@ static void stmp3xxx_wdt_set_timeout(struct device *dev, u32 timeout)
>  		       rtc_data->io + STMP3XXX_RTC_CTRL + STMP_OFFSET_REG_SET);
>  		writel(STMP3XXX_RTC_PERSISTENT1_FORCE_UPDATER,
>  		       rtc_data->io + STMP3XXX_RTC_PERSISTENT1 + STMP_OFFSET_REG_SET);
> +		writel(STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE,
> +		       rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_SET);
>  	} else {
>  		writel(STMP3XXX_RTC_CTRL_WATCHDOGEN,
>  		       rtc_data->io + STMP3XXX_RTC_CTRL + STMP_OFFSET_REG_CLR);
>  		writel(STMP3XXX_RTC_PERSISTENT1_FORCE_UPDATER,
>  		       rtc_data->io + STMP3XXX_RTC_PERSISTENT1 + STMP_OFFSET_REG_CLR);
> +		writel(STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE,
> +		       rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_CLR);

checkpatch tells:

WARNING: line over 80 characters
#131: FILE: drivers/rtc/rtc-stmp3xxx.c:115:
+                      rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_SET);

WARNING: line over 80 characters
#138: FILE: drivers/rtc/rtc-stmp3xxx.c:122:
+                      rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_CLR);

>  	}
>  }
>  
> +static void stmp3xxx_wdt_clear_bootstatus(struct device *dev)
> +{
> +	struct stmp3xxx_rtc_data *rtc_data = dev_get_drvdata(dev);
> +
> +	writel(STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE,
> +	       rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_CLR);
> +}
> +
>  static struct stmp3xxx_wdt_pdata wdt_pdata = {
>  	.wdt_set_timeout = stmp3xxx_wdt_set_timeout,
> +	.wdt_clear_bootstatus = stmp3xxx_wdt_clear_bootstatus,
> +	.bootstatus = 0,
>  };

This is out of the scope of your patch, but IMHO wdt_pdata should be
const in case it is used for >1 device.

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: [PATCHv3 1/2] watchdog: stmp3xxx: Stop the watchdog on system halt

[Harald Geyer]

Hello Uwe.

Uwe Kleine-König writes:
> Hello,
> 
> On Wed, Oct 07, 2015 at 12:19:11PM +0000, Harald Geyer wrote:
> > This allows the system to actually halt even if userspace forgot to
> > disable the watchdog first. Old behaviour was that the watchdog forced
> > the system to boot again.
> > 
> > Signed-off-by: Harald Geyer <harald@ccbib.org>
> > ---
> > Changes since v2:
> > * split this out as a separate patch
> > 
> >  drivers/watchdog/stmp3xxx_rtc_wdt.c |   28 ++++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> > 
> > diff --git a/drivers/watchdog/stmp3xxx_rtc_wdt.c b/drivers/watchdog/stmp3xxx_rtc_wdt.c
> > index 3ee6128..e09a01f 100644
> > --- a/drivers/watchdog/stmp3xxx_rtc_wdt.c
> > +++ b/drivers/watchdog/stmp3xxx_rtc_wdt.c
> > @@ -14,6 +14,8 @@
> >  #include <linux/watchdog.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/stmp3xxx_rtc_wdt.h>
> > +#include <linux/notifier.h>
> > +#include <linux/reboot.h>
> >  
> >  #define WDOG_TICK_RATE 1000 /* 1 kHz clock */
> >  #define STMP3XXX_DEFAULT_TIMEOUT 19
> > @@ -69,6 +71,28 @@ static struct watchdog_device stmp3xxx_wdd = {
> >  	.status = WATCHDOG_NOWAYOUT_INIT_STATUS,
> >  };
> >  
> > +static int wdt_notify_sys(struct notifier_block *nb, unsigned long code,
> > +			  void *unused)
> > +{
> > +	struct device *dev = watchdog_get_drvdata(&stmp3xxx_wdd);
> > +	struct stmp3xxx_wdt_pdata *pdata = dev_get_platdata(dev);
> > +
> > +	switch (code) {
> > +	case SYS_DOWN:	/* keep enabled, system might crash while going down */
> > +		break;
> > +	case SYS_HALT:  /* allow the system to actually halt */
> 
> <nitpick>
> You used a tab before the comment "keep enabled ...", but spaces for
> "allow the system ...".
> </nitpick>

Ok
 
> > +	case SYS_POWER_OFF:
> > +		wdt_stop(&stmp3xxx_wdd);
> > +		break;
> > +	}
> > +
> > +	return NOTIFY_DONE;
> > +}
> > +
> > +static struct notifier_block wdt_notifier = {
> > +	.notifier_call  = wdt_notify_sys,
> 
> s/  / /

Ok

> > +};
> > +
> >  static int stmp3xxx_wdt_probe(struct platform_device *pdev)
> >  {
> >  	int ret;
> > @@ -84,6 +108,9 @@ static int stmp3xxx_wdt_probe(struct platform_device *pdev)
> >  		return ret;
> >  	}
> >  
> > +	if (register_reboot_notifier(&wdt_notifier))
> > +		dev_warn(&pdev->dev, "cannot register reboot notifier\n");
> 
> OK, you don't fail when registering the notifier fails, but ...
> 
> >  	dev_info(&pdev->dev, "initialized watchdog with heartbeat %ds\n",
> >  			stmp3xxx_wdd.timeout);
> >  	return 0;
> > @@ -91,6 +118,7 @@ static int stmp3xxx_wdt_probe(struct platform_device *pdev)
> >  
> >  static int stmp3xxx_wdt_remove(struct platform_device *pdev)
> >  {
> > +	unregister_reboot_notifier(&wdt_notifier);
> 
> ... I think it's not ok to unconditionally unregister it then, is it?
> (Looking at how unregister_reboot_notifier is implemented it might work
> as intended, but if that's only by chance I wouldn't rely on it.)

I'm not sure what you refer to by "how unregister_reboot_notifier is
implemented". The documentation for unregister_reboot_notifier says:
/**
  *      unregister_reboot_notifier - Unregister previously registered reboot notifier
  *      @nb: Hook to be unregistered
  *
  *      Unregisters a previously registered reboot
  *      notifier function.
  *
  *      Returns zero on success, or %-ENOENT on failure.
  */

If register_reboot_notifier failed, then unregister_reboot_notifier will
simply return -ENOENT, I don't see a problem here. As this is documented
API I think we can rely on it.

Can you also have a look at the following patch (next in this series,
unfortunately I failed a formatting the subject prefix properly):
http://www.spinics.net/lists/linux-watchdog/msg07483.html

That one is particularly messy, as I don't have all the HW documentation
I wish I had, so I'd appreciate if somebody, who has experience with
the hardware, looked at it.

Thanks,
Harald

> Best regards
> Uwe
> 
> >  	watchdog_unregister_device(&stmp3xxx_wdd);
> >  	return 0;
> >  }
> > -- 
> > 1.7.10.4
> > 
> > 
> > 
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

-- 
If you want to support my work:
see http://friends.ccbib.org/harald/supporting/
or donate via bitcoin to 1FUtd8T9jRN1rFz63vZz7s2fDtB6d6A7aS
--
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 2/2] watchdog: stmp3xxx: Implement GETBOOTSTATUS

[Harald Geyer]

Hi Uwe,

thanks for looking into this.

Uwe Kleine-König writes:
> On Wed, Oct 07, 2015 at 12:19:12PM +0000, Harald Geyer wrote:
> > This device doesn't provide any information about boot status.
> > As workaround we use a persitent bit to track watchdog activity.
> 
> Hmm, so you set a bit in a persistent register iff the watchdog is
> running.  And if that bit is set during probe you assume the watchdog
> reset the machine. But this also happens, when the user presses the
> reset button which makes the detection unreliable.

Yes, kind of. I'm using this driver on an imx233-olinuxino, where the
reset button causes the device to power cycle and thus clears the
persitent bit. So I can't test your case, but surely there are some paths
into reset, that don't clear the bit and cause false positives.

I have considered gathering some additional information from the
power subsystem to refine the result, but decided that it's not
worth the complexity. Since the power system is a different silicon
block, we can't rely on it being available and would need to provide
a fall back implementation anyway.

I believe my patch does all that can be done with only the registers,
that belong to the device, but if you have any idea how to make
it more reliable, then I'd be very interested in that.
 
> > Signed-off-by: Harald Geyer <harald@ccbib.org>
> > ---
> > Changes since v2:
> > * make code ordering more consistent
> > * move part of the commit message to a code comment
> > * rewrite the commit message
> > 
> > Changes since v1:
> > * make code formatting more consistent with the rest of the driver
> > * Cc some people who might have better documentation then I do
> > 
> > Changes since initially posting this patch on 08/04/2015:
> > * fix a spelling error in the commit message
> > * rebase to a recent version
> > 
> >  drivers/rtc/rtc-stmp3xxx.c          |   37 +++++++++++++++++++++++++++++++++++
> >  drivers/watchdog/stmp3xxx_rtc_wdt.c |    6 +++++-
> >  include/linux/stmp3xxx_rtc_wdt.h    |    2 ++
> >  3 files changed, 44 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/rtc/rtc-stmp3xxx.c b/drivers/rtc/rtc-stmp3xxx.c
> > index ca54d03..47947ea 100644
> > --- a/drivers/rtc/rtc-stmp3xxx.c
> > +++ b/drivers/rtc/rtc-stmp3xxx.c
> > @@ -30,6 +30,7 @@
> >  #include <linux/of.h>
> >  #include <linux/stmp_device.h>
> >  #include <linux/stmp3xxx_rtc_wdt.h>
> > +#include <linux/watchdog.h>
> >  
> >  #define STMP3XXX_RTC_CTRL			0x0
> >  #define STMP3XXX_RTC_CTRL_ALARM_IRQ_EN		0x00000001
> > @@ -62,6 +63,9 @@
> >  /* missing bitmask in headers */
> >  #define STMP3XXX_RTC_PERSISTENT1_FORCE_UPDATER	0x80000000
> >  
> > +#define STMP3XXX_RTC_PERSISTENT2		0x80
> > +#define STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE	0x00000001
> > +
> >  struct stmp3xxx_rtc_data {
> >  	struct rtc_device *rtc;
> >  	void __iomem *io;
> > @@ -81,6 +85,20 @@ struct stmp3xxx_rtc_data {
> >   * The watchdog driver is passed the below accessor function via platform_data
> >   * to configure the watchdog. Locking is not needed because accessing SET/CLR
> >   * registers is atomic.
> > + *
> > + * Since this device doesn't report the cause of the last reset, we use
> > + * a persistent bit to track watchdog activity. The code from the Freescale
> > + * BSP uses the STMP3XXX_RTC_PERSISTENT1 register, which is dedicated to
> > + * controlling the boot ROM, for this purpose. However it seems the bit
> > + * there can't be cleared once it has been set. So we are using
> > + * STMP3XXX_RTC_PERSISTENT2 instead, which is the first register available
> > + * for "software use" without restriction.
> 
> Adding a link to the vendor kernel might be interesting here.

Ok, I'll try to dig it up again.
 
> > + * I (Harald Geyer <harald@ccbib.org>) don't know if the code touching the
> > + * STMP3XXX_RTC_PERSISTENT1 register is doing anything useful. Maybe this
> > + * is just a leftover from the BSP code, but then maybe there is something
> > + * in the boot ROM or in some bootloader that is using this. Hard to tell
> > + * without proper documentation about this register.
> 
> Fabio: anything you can do here to help?
> 
> >   */
> >  
> >  static void stmp3xxx_wdt_set_timeout(struct device *dev, u32 timeout)
> > @@ -93,16 +111,30 @@ static void stmp3xxx_wdt_set_timeout(struct device *dev, u32 timeout)
> >  		       rtc_data->io + STMP3XXX_RTC_CTRL + STMP_OFFSET_REG_SET);
> >  		writel(STMP3XXX_RTC_PERSISTENT1_FORCE_UPDATER,
> >  		       rtc_data->io + STMP3XXX_RTC_PERSISTENT1 + STMP_OFFSET_REG_SET);
> > +		writel(STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE,
> > +		       rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_SET);
> >  	} else {
> >  		writel(STMP3XXX_RTC_CTRL_WATCHDOGEN,
> >  		       rtc_data->io + STMP3XXX_RTC_CTRL + STMP_OFFSET_REG_CLR);
> >  		writel(STMP3XXX_RTC_PERSISTENT1_FORCE_UPDATER,
> >  		       rtc_data->io + STMP3XXX_RTC_PERSISTENT1 + STMP_OFFSET_REG_CLR);
> > +		writel(STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE,
> > +		       rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_CLR);
> 
> checkpatch tells:
> 
> WARNING: line over 80 characters
> #131: FILE: drivers/rtc/rtc-stmp3xxx.c:115:
> +                      rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_SET);
> 
> WARNING: line over 80 characters
> #138: FILE: drivers/rtc/rtc-stmp3xxx.c:122:
> +                      rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_CLR);

I tried to follow the style of the surrounding code. If there is a clear
statement about the preferred style for this driver from whoever is
responsible, I will happily change my patch to whatever it is.

> >  	}
> >  }
> >  
> > +static void stmp3xxx_wdt_clear_bootstatus(struct device *dev)
> > +{
> > +	struct stmp3xxx_rtc_data *rtc_data = dev_get_drvdata(dev);
> > +
> > +	writel(STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE,
> > +	       rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_CLR);
> > +}
> > +
> >  static struct stmp3xxx_wdt_pdata wdt_pdata = {
> >  	.wdt_set_timeout = stmp3xxx_wdt_set_timeout,
> > +	.wdt_clear_bootstatus = stmp3xxx_wdt_clear_bootstatus,
> > +	.bootstatus = 0,
> >  };
> 
> This is out of the scope of your patch, but IMHO wdt_pdata should be
> const in case it is used for >1 device.

Hm, if you want to change wdt_pdata to const, then I can't add the
bootstatus field. But I think you can't change this to const anyway,
because the platform_data pointer of struct device is not a const void
pointer. However didn't look into this in detail, so maybe I'm missing
something ...

I guess we could move wdt_pdata into rtc_data, but this would
entangle rtc and watchdog drivers even more, so I'd rather not do that.
What do you think?

Thanks,
Harald

-- 
If you want to support my work:
see http://friends.ccbib.org/harald/supporting/
or donate via bitcoin to 1FUtd8T9jRN1rFz63vZz7s2fDtB6d6A7aS
--
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 2/2] watchdog: stmp3xxx: Implement GETBOOTSTATUS

[Uwe Kleine-König]

Hello Harald,

On Thu, Nov 26, 2015 at 02:39:45PM +0100, Harald Geyer wrote:
> Uwe Kleine-König writes:
> > On Wed, Oct 07, 2015 at 12:19:12PM +0000, Harald Geyer wrote:
> > > This device doesn't provide any information about boot status.
> > > As workaround we use a persitent bit to track watchdog activity.
> > 
> > Hmm, so you set a bit in a persistent register iff the watchdog is
> > running.  And if that bit is set during probe you assume the watchdog
> > reset the machine. But this also happens, when the user presses the
> > reset button which makes the detection unreliable.
> 
> Yes, kind of. I'm using this driver on an imx233-olinuxino, where the
> reset button causes the device to power cycle and thus clears the
> persitent bit. So I can't test your case, but surely there are some paths

So the rtc isn't properly battery backed? AFAIUI the persistent bits
should survive a power cycle?! I think somewhere in our hardware
repository at Pengutronix we have a machine where I can test that, I
will check that.

> into reset, that don't clear the bit and cause false positives.

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 2/2] watchdog: stmp3xxx: Implement GETBOOTSTATUS

[Harald Geyer]

Hi Uwe,

Uwe Kleine-König writes:
> On Thu, Nov 26, 2015 at 02:39:45PM +0100, Harald Geyer wrote:
> > Uwe Kleine-König writes:
> > > Hmm, so you set a bit in a persistent register iff the watchdog is
> > > running.  And if that bit is set during probe you assume the watchdog
> > > reset the machine. But this also happens, when the user presses the
> > > reset button which makes the detection unreliable.
> > 
> > Yes, kind of. I'm using this driver on an imx233-olinuxino, where the
> > reset button causes the device to power cycle and thus clears the
> > persitent bit. So I can't test your case, but surely there are some paths
> 
> So the rtc isn't properly battery backed?

Yes, since the mainline kernel can't do anything useful with the battery
other than reporting it's voltage, nobody uses imx233-olinuxino with
mainline kernel and battery.

> AFAIUI the persistent bits should survive a power cycle?!

I only tested without battery, and in my case they definitely don't.

> I think somewhere in our hardware
> repository at Pengutronix we have a machine where I can test that, I
> will check that.

Thanks!

Best regards,
Harald

> > into reset, that don't clear the bit and cause false positives.
> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

-- 
If you want to support my work:
see http://friends.ccbib.org/harald/supporting/
or donate via bitcoin to 1FUtd8T9jRN1rFz63vZz7s2fDtB6d6A7aS
--
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 2/2] watchdog: stmp3xxx: Implement GETBOOTSTATUS

[Uwe Kleine-König]

On Thu, Nov 26, 2015 at 02:39:45PM +0100, Harald Geyer wrote:
> Hi Uwe,
> 
> thanks for looking into this.
> 
> Uwe Kleine-König writes:
> > On Wed, Oct 07, 2015 at 12:19:12PM +0000, Harald Geyer wrote:
> > > This device doesn't provide any information about boot status.
> > > As workaround we use a persitent bit to track watchdog activity.
> > 
> > Hmm, so you set a bit in a persistent register iff the watchdog is
> > running.  And if that bit is set during probe you assume the watchdog
> > reset the machine. But this also happens, when the user presses the
> > reset button which makes the detection unreliable.
> 
> Yes, kind of. I'm using this driver on an imx233-olinuxino, where the
> reset button causes the device to power cycle and thus clears the
> persitent bit. So I can't test your case, but surely there are some paths
> into reset, that don't clear the bit and cause false positives.

OK, I found a i.MX28 machine in our repository:

	barebox# mw 0x80056080 0xbabecafe
	barebox# boot
	...
	linux# reboot
	barebox# md 0x80056080+4
	80056080: babecafe

After a power cycle the value disappears (I didn't check the details, if
there is a battery it might be empty).

> I have considered gathering some additional information from the
> power subsystem to refine the result, but decided that it's not
> worth the complexity. Since the power system is a different silicon
> block, we can't rely on it being available and would need to provide
> a fall back implementation anyway.

It would be really nice to get some details about boot rom usage of the
persistent flags in use. I'd expect that these allow to determine the
boot source.

> > >   */
> > >  
> > >  static void stmp3xxx_wdt_set_timeout(struct device *dev, u32 timeout)
> > > @@ -93,16 +111,30 @@ static void stmp3xxx_wdt_set_timeout(struct device *dev, u32 timeout)
> > >  		       rtc_data->io + STMP3XXX_RTC_CTRL + STMP_OFFSET_REG_SET);
> > >  		writel(STMP3XXX_RTC_PERSISTENT1_FORCE_UPDATER,
> > >  		       rtc_data->io + STMP3XXX_RTC_PERSISTENT1 + STMP_OFFSET_REG_SET);
> > > +		writel(STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE,
> > > +		       rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_SET);
> > >  	} else {
> > >  		writel(STMP3XXX_RTC_CTRL_WATCHDOGEN,
> > >  		       rtc_data->io + STMP3XXX_RTC_CTRL + STMP_OFFSET_REG_CLR);
> > >  		writel(STMP3XXX_RTC_PERSISTENT1_FORCE_UPDATER,
> > >  		       rtc_data->io + STMP3XXX_RTC_PERSISTENT1 + STMP_OFFSET_REG_CLR);
> > > +		writel(STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE,
> > > +		       rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_CLR);
> > 
> > checkpatch tells:
> > 
> > WARNING: line over 80 characters
> > #131: FILE: drivers/rtc/rtc-stmp3xxx.c:115:
> > +                      rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_SET);
> > 
> > WARNING: line over 80 characters
> > #138: FILE: drivers/rtc/rtc-stmp3xxx.c:122:
> > +                      rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_CLR);
> 
> I tried to follow the style of the surrounding code. If there is a clear
> statement about the preferred style for this driver from whoever is
> responsible, I will happily change my patch to whatever it is.

Yeah, I don't care much. I would have broken the newly introduced lines,
but I'd not go so far to nack your patch when you don't.

> > >  	}
> > >  }
> > >  
> > > +static void stmp3xxx_wdt_clear_bootstatus(struct device *dev)
> > > +{
> > > +	struct stmp3xxx_rtc_data *rtc_data = dev_get_drvdata(dev);
> > > +
> > > +	writel(STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE,
> > > +	       rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_CLR);
> > > +}
> > > +
> > >  static struct stmp3xxx_wdt_pdata wdt_pdata = {
> > >  	.wdt_set_timeout = stmp3xxx_wdt_set_timeout,
> > > +	.wdt_clear_bootstatus = stmp3xxx_wdt_clear_bootstatus,
> > > +	.bootstatus = 0,
> > >  };
> > 
> > This is out of the scope of your patch, but IMHO wdt_pdata should be
> > const in case it is used for >1 device.
> 
> Hm, if you want to change wdt_pdata to const, then I can't add the
> bootstatus field. But I think you can't change this to const anyway,
> because the platform_data pointer of struct device is not a const void
> pointer. However didn't look into this in detail, so maybe I'm missing
> something ...

Conceptually platform data should be const for a driver, not necessarily
for the code that sets it up. So the right thing to do is to add a const
to struct device::platform_data. That is usually not in the way during
creation of the device, but stops drivers to modify it.

Then you can do in stmp3xxx_wdt_register:

	struct ... pdata = {
		.wdt_set_timeout = stmp3xxx_wdt_set_timeout,
		.bootstatus = whatever;
	};
	struct platform_device_info pdevinfo = {
		.parent = &rtc_pdev->dev;
		.data = pdata;
		.size_data = sizeof(pdata);
	}
	platform_device_register_full(&pdevinfo);

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 2/2] watchdog: stmp3xxx: Implement GETBOOTSTATUS

[Uwe Kleine-König]

Hello Harald,

On Fri, Nov 27, 2015 at 11:02:54AM +0100, Uwe Kleine-König wrote:
> On Thu, Nov 26, 2015 at 02:39:45PM +0100, Harald Geyer wrote:
> > Uwe Kleine-König writes:
> > > On Wed, Oct 07, 2015 at 12:19:12PM +0000, Harald Geyer wrote:
> > > > This device doesn't provide any information about boot status.
> > > > As workaround we use a persitent bit to track watchdog activity.
> > > 
> > > Hmm, so you set a bit in a persistent register iff the watchdog is
> > > running.  And if that bit is set during probe you assume the watchdog
> > > reset the machine. But this also happens, when the user presses the
> > > reset button which makes the detection unreliable.
> > 
> > Yes, kind of. I'm using this driver on an imx233-olinuxino, where the
> > reset button causes the device to power cycle and thus clears the
> > persitent bit. So I can't test your case, but surely there are some paths
> > into reset, that don't clear the bit and cause false positives.
> 
> OK, I found a i.MX28 machine in our repository:
> 
> 	barebox# mw 0x80056080 0xbabecafe
> 	barebox# boot
> 	...
> 	linux# reboot
> 	barebox# md 0x80056080+4
> 	80056080: babecafe
> 
> After a power cycle the value disappears (I didn't check the details, if
> there is a battery it might be empty).
> 
> > I have considered gathering some additional information from the
> > power subsystem to refine the result, but decided that it's not
> > worth the complexity. Since the power system is a different silicon
> > block, we can't rely on it being available and would need to provide
> > a fall back implementation anyway.
> 
> It would be really nice to get some details about boot rom usage of the
> persistent flags in use. I'd expect that these allow to determine the
> boot source.

A colleague just hinted me to

	http://git.pengutronix.de/?p=barebox.git;a=blob;f=drivers/watchdog/im28wd.c;h=3510776a3afc8aaf113eee995900b099c12c2be9;hb=HEAD

which contains a few bit definitions for the i.MX28. Maybe they can help
you to understand your machine better?! At a quick glance there is
nothing that helps you to determine the boot reason, but still it might
contain a few interesting things.

Best regards
Uwe

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

Re: [PATCH 2/2] watchdog: stmp3xxx: Implement GETBOOTSTATUS

[Alexandre Belloni]

On 27/11/2015 at 11:02:54 +0100, Uwe Kleine-König wrote :
> OK, I found a i.MX28 machine in our repository:
> 
> 	barebox# mw 0x80056080 0xbabecafe
> 	barebox# boot
> 	...
> 	linux# reboot
> 	barebox# md 0x80056080+4
> 	80056080: babecafe
> 
> After a power cycle the value disappears (I didn't check the details, if
> there is a battery it might be empty).
> 

Yeah, you'd have to ensure that the crystal power domain (VDDXTAL) stays powered.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 2/2] watchdog: stmp3xxx: Implement GETBOOTSTATUS

[Harald Geyer]

Hi Uwe!

On 27.11.2015 11:44, Uwe Kleine-König wrote:
> Hello Harald,
>
> On Fri, Nov 27, 2015 at 11:02:54AM +0100, Uwe Kleine-König wrote:
>> On Thu, Nov 26, 2015 at 02:39:45PM +0100, Harald Geyer wrote:
>> > Uwe Kleine-König writes:
>> > > On Wed, Oct 07, 2015 at 12:19:12PM +0000, Harald Geyer wrote:
>> > > > This device doesn't provide any information about boot status.
>> > > > As workaround we use a persitent bit to track watchdog 
>> activity.
>> > >
>> > > Hmm, so you set a bit in a persistent register iff the watchdog 
>> is
>> > > running.  And if that bit is set during probe you assume the 
>> watchdog
>> > > reset the machine. But this also happens, when the user presses 
>> the
>> > > reset button which makes the detection unreliable.
>> >
>> > Yes, kind of. I'm using this driver on an imx233-olinuxino, where 
>> the
>> > reset button causes the device to power cycle and thus clears the
>> > persitent bit. So I can't test your case, but surely there are 
>> some paths
>> > into reset, that don't clear the bit and cause false positives.
>>
>> OK, I found a i.MX28 machine in our repository:
>>
>> 	barebox# mw 0x80056080 0xbabecafe
>> 	barebox# boot
>> 	...
>> 	linux# reboot
>> 	barebox# md 0x80056080+4
>> 	80056080: babecafe
>>
>> After a power cycle the value disappears (I didn't check the 
>> details, if
>> there is a battery it might be empty).

Are there any concerns left on your side? Ie do you want me to do
any additional tests before sending v4 of this patch with the
whitespace changes and added URL to the freescale kernel?

>> > I have considered gathering some additional information from the
>> > power subsystem to refine the result, but decided that it's not
>> > worth the complexity. Since the power system is a different 
>> silicon
>> > block, we can't rely on it being available and would need to 
>> provide
>> > a fall back implementation anyway.
>>
>> It would be really nice to get some details about boot rom usage of 
>> the
>> persistent flags in use. I'd expect that these allow to determine 
>> the
>> boot source.
>
> A colleague just hinted me to
>
> 
> 	http://git.pengutronix.de/?p=barebox.git;a=blob;f=drivers/watchdog/im28wd.c;h=3510776a3afc8aaf113eee995900b099c12c2be9;hb=HEAD
>
> which contains a few bit definitions for the i.MX28. Maybe they can 
> help
> you to understand your machine better?! At a quick glance there is
> nothing that helps you to determine the boot reason, but still it 
> might
> contain a few interesting things.

Thanks. Unfortunately about the bit that we set in mainline kernel for
unknown reasons, it also only says:
/* dubious meaning from inside the SoC's firmware ROM */
# define MXS_RTC_PERSISTENT1_FORCE_UPDATER (1 << 31)

best regards,
Harald

Re: [PATCH 2/2] watchdog: stmp3xxx: Implement GETBOOTSTATUS

[Uwe Kleine-König]

Hello Harald,

On Sun, Nov 29, 2015 at 11:41:22AM +0100, Harald Geyer wrote:
> On 27.11.2015 11:44, Uwe Kleine-König wrote:
> >Hello Harald,
> >
> >On Fri, Nov 27, 2015 at 11:02:54AM +0100, Uwe Kleine-König wrote:
> >>On Thu, Nov 26, 2015 at 02:39:45PM +0100, Harald Geyer wrote:
> >>> Uwe Kleine-König writes:
> >>> > On Wed, Oct 07, 2015 at 12:19:12PM +0000, Harald Geyer wrote:
> >>> > > This device doesn't provide any information about boot status.
> >>> > > As workaround we use a persitent bit to track watchdog activity.
> >>> >
> >>> > Hmm, so you set a bit in a persistent register iff the watchdog is
> >>> > running.  And if that bit is set during probe you assume the
> >>watchdog
> >>> > reset the machine. But this also happens, when the user presses the
> >>> > reset button which makes the detection unreliable.
> >>>
> >>> Yes, kind of. I'm using this driver on an imx233-olinuxino, where the
> >>> reset button causes the device to power cycle and thus clears the
> >>> persitent bit. So I can't test your case, but surely there are some
> >>paths
> >>> into reset, that don't clear the bit and cause false positives.
> >>
> >>OK, I found a i.MX28 machine in our repository:
> >>
> >>	barebox# mw 0x80056080 0xbabecafe
> >>	barebox# boot
> >>	...
> >>	linux# reboot
> >>	barebox# md 0x80056080+4
> >>	80056080: babecafe
> >>
> >>After a power cycle the value disappears (I didn't check the details, if
> >>there is a battery it might be empty).
> 
> Are there any concerns left on your side? Ie do you want me to do
> any additional tests before sending v4 of this patch with the
> whitespace changes and added URL to the freescale kernel?

I just notice that I failed to do the proper test. I.e. I reset the
machine using "reboot" instead of a reset button. But I'd expect that
doing the latter also keeps the value of the persistant register, which
would make your patch wrong here. I will check that tomorrow when I can
make someone do this test for me. (I'm 600 km from the machine above
away.)
 
Best regards
Uwe

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