[rtc-linux] [PATCH] watchdog: stmp3xxx: Implement GETBOOTSTATUS

[rtc-linux] [PATCH] 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 similiar 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>
---
 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 2939cdc..55ab9ba 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_SET			0x4
@@ -60,6 +61,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,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);
@@ -91,16 +103,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)
@@ -108,6 +126,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;
@@ -304,6 +324,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 a62b1b6..5b3f12a8 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,13 +72,38 @@ 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.bootstatus = pdata->bootstatus;
 
 	ret = watchdog_register_device(&stmp3xxx_wdd);
 	if (ret < 0) {
@@ -83,6 +111,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;
@@ -90,6 +121,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

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[PATCH] 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 similiar 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>
---
 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 2939cdc..55ab9ba 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_SET			0x4
@@ -60,6 +61,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,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);
@@ -91,16 +103,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)
@@ -108,6 +126,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;
@@ -304,6 +324,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 a62b1b6..5b3f12a8 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,13 +72,38 @@ 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.bootstatus = pdata->bootstatus;
 
 	ret = watchdog_register_device(&stmp3xxx_wdd);
 	if (ret < 0) {
@@ -83,6 +111,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;
@@ -90,6 +121,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: [rtc-linux] [PATCH] watchdog: stmp3xxx: Implement GETBOOTSTATUS

[Alexandre Belloni]

Hi,

On 08/04/2015 at 19:45:55 +0000, Harald Geyer wrote :
> 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 similiar to what the legacy driver from freescale
           similar ^

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

The RTC part seems ok to me but the driver itself would benefit from a
few improvements:
 - use STMP_OFFSET_REG_(SET|CLR) instead of defining _SET and _CLR for
   STMP3XXX_RTC_CTRL and STMP3XXX_RTC_PERSISTENT0
 - use syscon instead of passing callbacks to the watchdog driver

It would be greatly appreciated if you could do those changes too.


> ---
>  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 2939cdc..55ab9ba 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_SET			0x4
> @@ -60,6 +61,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,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);
> @@ -91,16 +103,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)
> @@ -108,6 +126,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;
> @@ -304,6 +324,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 a62b1b6..5b3f12a8 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,13 +72,38 @@ 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.bootstatus = pdata->bootstatus;
>  
>  	ret = watchdog_register_device(&stmp3xxx_wdd);
>  	if (ret < 0) {
> @@ -83,6 +111,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;
> @@ -90,6 +121,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
> 
> -- 
> -- 
> You received this message because you are subscribed to "rtc-linux".
> Membership options at http://groups.google.com/group/rtc-linux .
> Please read http://groups.google.com/group/rtc-linux/web/checklist
> before submitting a driver.
> --- 
> You received this message because you are subscribed to the Google Groups "rtc-linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

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

Re: [rtc-linux] [PATCH] watchdog: stmp3xxx: Implement GETBOOTSTATUS

[harald]

Hi!

On Fri, 17 Apr 2015 11:30:23 +0200, Alexandre Belloni
<alexandre.belloni@free-electrons.com> wrote:
> Hi,
> 
> On 08/04/2015 at 19:45:55 +0000, Harald Geyer wrote :
>> 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 similiar to what the legacy driver from freescale
>            similar ^
> 
>> 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>
> 
> The RTC part seems ok to me but the driver itself would benefit from a
> few improvements:
>  - use STMP_OFFSET_REG_(SET|CLR) instead of defining _SET and _CLR for
>    STMP3XXX_RTC_CTRL and STMP3XXX_RTC_PERSISTENT0

This should be easy enough.

>  - use syscon instead of passing callbacks to the watchdog driver

I will put looking into this onto my todo list, but I'm not familiar
with syscon yet and it is not obvious to me how to port the rtc driver
to syscon without breaking devicetree backwards compatibility. So I'd
like to defer this for later unless somebody gives me some guidance.

> It would be greatly appreciated if you could do those changes too.

Is it ok if I send the simple cleanup patch and a rebased version of
my original patch without the syscon conversion? This will be more work
for me testing two patch series, but I think GETBOOTSTATUS is an
important feature that I'd like to see supported soon.

TIA,
Harald
 
>> ---
>>  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 2939cdc..55ab9ba 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_SET			0x4
>> @@ -60,6 +61,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,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);
>> @@ -91,16 +103,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)
>> @@ -108,6 +126,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;
>> @@ -304,6 +324,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 a62b1b6..5b3f12a8 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,13 +72,38 @@ 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.bootstatus = pdata->bootstatus;
>>  
>>  	ret = watchdog_register_device(&stmp3xxx_wdd);
>>  	if (ret < 0) {
>> @@ -83,6 +111,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;
>> @@ -90,6 +121,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
>> 
>> -- 
>> -- 
>> You received this message because you are subscribed to "rtc-linux".
>> Membership options at http://groups.google.com/group/rtc-linux .
>> Please read http://groups.google.com/group/rtc-linux/web/checklist
>> before submitting a driver.
>> --- 
>> You received this message because you are subscribed to the Google
>> Groups "rtc-linux" group.
>> To unsubscribe from this group and stop receiving emails from it, send
>> an email to rtc-linux+unsubscribe@googlegroups.com.
>> For more options, visit https://groups.google.com/d/optout.

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [rtc-linux] [PATCH] watchdog: stmp3xxx: Implement GETBOOTSTATUS

[harald]

Hi!

On Fri, 17 Apr 2015 11:30:23 +0200, Alexandre Belloni
<alexandre.belloni@free-electrons.com> wrote:
> Hi,
> 
> On 08/04/2015 at 19:45:55 +0000, Harald Geyer wrote :
>> 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 similiar to what the legacy driver from freescale
>            similar ^
> 
>> 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>
> 
> The RTC part seems ok to me but the driver itself would benefit from a
> few improvements:
>  - use STMP_OFFSET_REG_(SET|CLR) instead of defining _SET and _CLR for
>    STMP3XXX_RTC_CTRL and STMP3XXX_RTC_PERSISTENT0

This should be easy enough.

>  - use syscon instead of passing callbacks to the watchdog driver

I will put looking into this onto my todo list, but I'm not familiar
with syscon yet and it is not obvious to me how to port the rtc driver
to syscon without breaking devicetree backwards compatibility. So I'd
like to defer this for later unless somebody gives me some guidance.

> It would be greatly appreciated if you could do those changes too.

Is it ok if I send the simple cleanup patch and a rebased version of
my original patch without the syscon conversion? This will be more work
for me testing two patch series, but I think GETBOOTSTATUS is an
important feature that I'd like to see supported soon.

TIA,
Harald
 
>> ---
>>  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 2939cdc..55ab9ba 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_SET			0x4
>> @@ -60,6 +61,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,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);
>> @@ -91,16 +103,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)
>> @@ -108,6 +126,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;
>> @@ -304,6 +324,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 a62b1b6..5b3f12a8 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,13 +72,38 @@ 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.bootstatus = pdata->bootstatus;
>>  
>>  	ret = watchdog_register_device(&stmp3xxx_wdd);
>>  	if (ret < 0) {
>> @@ -83,6 +111,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;
>> @@ -90,6 +121,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
>> 
>> -- 
>> -- 
>> You received this message because you are subscribed to "rtc-linux".
>> Membership options at http://groups.google.com/group/rtc-linux .
>> Please read http://groups.google.com/group/rtc-linux/web/checklist
>> before submitting a driver.
>> --- 
>> You received this message because you are subscribed to the Google
>> Groups "rtc-linux" group.
>> To unsubscribe from this group and stop receiving emails from it, send
>> an email to rtc-linux+unsubscribe@googlegroups.com.
>> For more options, visit https://groups.google.com/d/optout.

Re: [rtc-linux] [PATCH] watchdog: stmp3xxx: Implement GETBOOTSTATUS

[Alexandre Belloni]

Hi,

On 19/04/2015 at 15:41:42 +0200, harald@ccbib.org wrote :
> >  - use syscon instead of passing callbacks to the watchdog driver
> 
> I will put looking into this onto my todo list, but I'm not familiar
> with syscon yet and it is not obvious to me how to port the rtc driver
> to syscon without breaking devicetree backwards compatibility. So I'd
> like to defer this for later unless somebody gives me some guidance.
> 

As the rtc driver is the one instantiating the watchdog driver, there is
no reason why the DT ABI would be broken. You simply have to create
the syscon in the RTC driver and then pass it to the watchdog driver,
the same way it is done with the callbacks.

I can have a look at doing it as I have some i.mx28 platforms.

> > It would be greatly appreciated if you could do those changes too.
> 
> Is it ok if I send the simple cleanup patch and a rebased version of
> my original patch without the syscon conversion? This will be more work
> for me testing two patch series, but I think GETBOOTSTATUS is an
> important feature that I'd like to see supported soon.
> 

I'm fine with your patch, I was simply suggesting cleanups that should
be sent as separate patches. However, I'd like more people to review the
watchdog part.

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

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [rtc-linux] [PATCH] watchdog: stmp3xxx: Implement GETBOOTSTATUS

[Alexandre Belloni]

Hi,

On 19/04/2015 at 15:41:42 +0200, harald@ccbib.org wrote :
> >  - use syscon instead of passing callbacks to the watchdog driver
> 
> I will put looking into this onto my todo list, but I'm not familiar
> with syscon yet and it is not obvious to me how to port the rtc driver
> to syscon without breaking devicetree backwards compatibility. So I'd
> like to defer this for later unless somebody gives me some guidance.
> 

As the rtc driver is the one instantiating the watchdog driver, there is
no reason why the DT ABI would be broken. You simply have to create
the syscon in the RTC driver and then pass it to the watchdog driver,
the same way it is done with the callbacks.

I can have a look at doing it as I have some i.mx28 platforms.

> > It would be greatly appreciated if you could do those changes too.
> 
> Is it ok if I send the simple cleanup patch and a rebased version of
> my original patch without the syscon conversion? This will be more work
> for me testing two patch series, but I think GETBOOTSTATUS is an
> important feature that I'd like to see supported soon.
> 

I'm fine with your patch, I was simply suggesting cleanups that should
be sent as separate patches. However, I'd like more people to review the
watchdog part.

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

Re: [rtc-linux] [PATCH] watchdog: stmp3xxx: Implement GETBOOTSTATUS

[harald]

Hi Alexandre,

On Sun, 19 Apr 2015 17:37:43 +0200, Alexandre Belloni
<alexandre.belloni@free-electrons.com> wrote:
> On 19/04/2015 at 15:41:42 +0200, harald@ccbib.org wrote :
>> >  - use syscon instead of passing callbacks to the watchdog driver
>> 
>> I will put looking into this onto my todo list, but I'm not familiar
>> with syscon yet and it is not obvious to me how to port the rtc driver
>> to syscon without breaking devicetree backwards compatibility. So I'd
>> like to defer this for later unless somebody gives me some guidance.
>> 
> 
> As the rtc driver is the one instantiating the watchdog driver, there is
> no reason why the DT ABI would be broken. You simply have to create
> the syscon in the RTC driver and then pass it to the watchdog driver,
> the same way it is done with the callbacks.

Ok, this is less invasive than what I have been thinking. This makes
sense. 
 
> I can have a look at doing it as I have some i.mx28 platforms.

If you can point me to a driver that is a good reference for your model,
then I can do this myself. Otherwise I'd prefer if you wrote the code.
I can do the testing though, if this helps.

Thanks,
Harald

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [rtc-linux] [PATCH] watchdog: stmp3xxx: Implement GETBOOTSTATUS

[harald]

Hi Alexandre,

On Sun, 19 Apr 2015 17:37:43 +0200, Alexandre Belloni
<alexandre.belloni@free-electrons.com> wrote:
> On 19/04/2015 at 15:41:42 +0200, harald@ccbib.org wrote :
>> >  - use syscon instead of passing callbacks to the watchdog driver
>> 
>> I will put looking into this onto my todo list, but I'm not familiar
>> with syscon yet and it is not obvious to me how to port the rtc driver
>> to syscon without breaking devicetree backwards compatibility. So I'd
>> like to defer this for later unless somebody gives me some guidance.
>> 
> 
> As the rtc driver is the one instantiating the watchdog driver, there is
> no reason why the DT ABI would be broken. You simply have to create
> the syscon in the RTC driver and then pass it to the watchdog driver,
> the same way it is done with the callbacks.

Ok, this is less invasive than what I have been thinking. This makes
sense. 
 
> I can have a look at doing it as I have some i.mx28 platforms.

If you can point me to a driver that is a good reference for your model,
then I can do this myself. Otherwise I'd prefer if you wrote the code.
I can do the testing though, if this helps.

Thanks,
Harald

Re: [rtc-linux] [PATCH] watchdog: stmp3xxx: Implement GETBOOTSTATUS

[Alexandre Belloni]

Hi,

On 20/04/2015 at 09:11:02 +0200, harald@ccbib.org wrote :
> Hi Alexandre,
> 
> On Sun, 19 Apr 2015 17:37:43 +0200, Alexandre Belloni
> <alexandre.belloni@free-electrons.com> wrote:
> > On 19/04/2015 at 15:41:42 +0200, harald@ccbib.org wrote :
> >> >  - use syscon instead of passing callbacks to the watchdog driver
> >> 
> >> I will put looking into this onto my todo list, but I'm not familiar
> >> with syscon yet and it is not obvious to me how to port the rtc driver
> >> to syscon without breaking devicetree backwards compatibility. So I'd
> >> like to defer this for later unless somebody gives me some guidance.
> >> 
> > 
> > As the rtc driver is the one instantiating the watchdog driver, there is
> > no reason why the DT ABI would be broken. You simply have to create
> > the syscon in the RTC driver and then pass it to the watchdog driver,
> > the same way it is done with the callbacks.
> 
> Ok, this is less invasive than what I have been thinking. This makes
> sense. 
>  

I realized I confused syscon with regmap. So indeed, switching to syscon
will require a not nice change to the DT. But what I really meant was
creating a regmap from the rtc driver and pass it to the watchdog
driver.

> > I can have a look at doing it as I have some i.mx28 platforms.
> 
> If you can point me to a driver that is a good reference for your model,
> then I can do this myself. Otherwise I'd prefer if you wrote the code.
> I can do the testing though, if this helps.
> 

You can have a look at the devm_regmap_init_mmio() users.


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

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [rtc-linux] [PATCH] watchdog: stmp3xxx: Implement GETBOOTSTATUS

[Alexandre Belloni]

Hi,

On 20/04/2015 at 09:11:02 +0200, harald@ccbib.org wrote :
> Hi Alexandre,
> 
> On Sun, 19 Apr 2015 17:37:43 +0200, Alexandre Belloni
> <alexandre.belloni@free-electrons.com> wrote:
> > On 19/04/2015 at 15:41:42 +0200, harald@ccbib.org wrote :
> >> >  - use syscon instead of passing callbacks to the watchdog driver
> >> 
> >> I will put looking into this onto my todo list, but I'm not familiar
> >> with syscon yet and it is not obvious to me how to port the rtc driver
> >> to syscon without breaking devicetree backwards compatibility. So I'd
> >> like to defer this for later unless somebody gives me some guidance.
> >> 
> > 
> > As the rtc driver is the one instantiating the watchdog driver, there is
> > no reason why the DT ABI would be broken. You simply have to create
> > the syscon in the RTC driver and then pass it to the watchdog driver,
> > the same way it is done with the callbacks.
> 
> Ok, this is less invasive than what I have been thinking. This makes
> sense. 
>  

I realized I confused syscon with regmap. So indeed, switching to syscon
will require a not nice change to the DT. But what I really meant was
creating a regmap from the rtc driver and pass it to the watchdog
driver.

> > I can have a look at doing it as I have some i.mx28 platforms.
> 
> If you can point me to a driver that is a good reference for your model,
> then I can do this myself. Otherwise I'd prefer if you wrote the code.
> I can do the testing though, if this helps.
> 

You can have a look at the devm_regmap_init_mmio() users.


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

Re: [rtc-linux] [PATCH] watchdog: stmp3xxx: Implement GETBOOTSTATUS

[harald]

Hi Alexandre,

sorry for the very long delay. 

I tried your suggestion to switch the watchdog driver to regmap several
times but couldn't come up with something that wasn't either very ugly
or completely pointless - i.e. no better then passing a pointer to the
register block. See below.

On Tue, 5 May 2015 11:40:31 +0200, Alexandre Belloni
<alexandre.belloni@free-electrons.com> wrote:
> Hi,
> 
> On 20/04/2015 at 09:11:02 +0200, harald@ccbib.org wrote :
>> Hi Alexandre,
>> 
>> On Sun, 19 Apr 2015 17:37:43 +0200, Alexandre Belloni
>> <alexandre.belloni@free-electrons.com> wrote:
>> > On 19/04/2015 at 15:41:42 +0200, harald@ccbib.org wrote :
>> >> >  - use syscon instead of passing callbacks to the watchdog driver
>> >> 
>> >> I will put looking into this onto my todo list, but I'm not familiar
>> >> with syscon yet and it is not obvious to me how to port the rtc
driver
>> >> to syscon without breaking devicetree backwards compatibility. So
I'd
>> >> like to defer this for later unless somebody gives me some guidance.
>> > 
>> > As the rtc driver is the one instantiating the watchdog driver, there
>> > is
>> > no reason why the DT ABI would be broken. You simply have to create
>> > the syscon in the RTC driver and then pass it to the watchdog driver,
>> > the same way it is done with the callbacks.
>> 
>> Ok, this is less invasive than what I have been thinking. This makes
>> sense. 
> 
> I realized I confused syscon with regmap. So indeed, switching to syscon
> will require a not nice change to the DT. But what I really meant was
> creating a regmap from the rtc driver and pass it to the watchdog
> driver.

Since the rtc and watchdog drivers need different bits of the same control
register, we would need to use regmap_fields to pass them around.
Unfortunately this means we can't use the _SET and _CLR registers, which
means we need to use regmap everywhere to get proper locking. A lot of
overhead for no benefit.

After reading lots of messages around the genesis and usage of
regmap_mmio,
it seems to me the intended usecases are to fix some endianness issues
and to have a register cache available during suspend - both of which
don't
apply in this case.

If you want to get rid of the callbacks we should just pass the pointer
to the register block to the child device, I think. Which way is
preferable
is probably only a matter of taste so I won't override the driver authors
decision unless there is some clear statement that this is the preferred
style in the rtc subsystem.

I will send the other patch (unify register access macros) you asked about
shortly.

Harald

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [rtc-linux] [PATCH] watchdog: stmp3xxx: Implement GETBOOTSTATUS

[harald]

Hi Alexandre,

sorry for the very long delay. 

I tried your suggestion to switch the watchdog driver to regmap several
times but couldn't come up with something that wasn't either very ugly
or completely pointless - i.e. no better then passing a pointer to the
register block. See below.

On Tue, 5 May 2015 11:40:31 +0200, Alexandre Belloni
<alexandre.belloni@free-electrons.com> wrote:
> Hi,
> 
> On 20/04/2015 at 09:11:02 +0200, harald@ccbib.org wrote :
>> Hi Alexandre,
>> 
>> On Sun, 19 Apr 2015 17:37:43 +0200, Alexandre Belloni
>> <alexandre.belloni@free-electrons.com> wrote:
>> > On 19/04/2015 at 15:41:42 +0200, harald@ccbib.org wrote :
>> >> >  - use syscon instead of passing callbacks to the watchdog driver
>> >> 
>> >> I will put looking into this onto my todo list, but I'm not familiar
>> >> with syscon yet and it is not obvious to me how to port the rtc
driver
>> >> to syscon without breaking devicetree backwards compatibility. So
I'd
>> >> like to defer this for later unless somebody gives me some guidance.
>> > 
>> > As the rtc driver is the one instantiating the watchdog driver, there
>> > is
>> > no reason why the DT ABI would be broken. You simply have to create
>> > the syscon in the RTC driver and then pass it to the watchdog driver,
>> > the same way it is done with the callbacks.
>> 
>> Ok, this is less invasive than what I have been thinking. This makes
>> sense. 
> 
> I realized I confused syscon with regmap. So indeed, switching to syscon
> will require a not nice change to the DT. But what I really meant was
> creating a regmap from the rtc driver and pass it to the watchdog
> driver.

Since the rtc and watchdog drivers need different bits of the same control
register, we would need to use regmap_fields to pass them around.
Unfortunately this means we can't use the _SET and _CLR registers, which
means we need to use regmap everywhere to get proper locking. A lot of
overhead for no benefit.

After reading lots of messages around the genesis and usage of
regmap_mmio,
it seems to me the intended usecases are to fix some endianness issues
and to have a register cache available during suspend - both of which
don't
apply in this case.

If you want to get rid of the callbacks we should just pass the pointer
to the register block to the child device, I think. Which way is
preferable
is probably only a matter of taste so I won't override the driver authors
decision unless there is some clear statement that this is the preferred
style in the rtc subsystem.

I will send the other patch (unify register access macros) you asked about
shortly.

Harald
--
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: [rtc-linux] [PATCH] watchdog: stmp3xxx: Implement GETBOOTSTATUS

[Alexandre Belloni]

Hi,

On 01/10/2015 at 19:20:32 +0200, harald@ccbib.org wrote :
> Since the rtc and watchdog drivers need different bits of the same control
> register, we would need to use regmap_fields to pass them around.
> Unfortunately this means we can't use the _SET and _CLR registers, which
> means we need to use regmap everywhere to get proper locking. A lot of
> overhead for no benefit.
> 
> After reading lots of messages around the genesis and usage of
> regmap_mmio,
> it seems to me the intended usecases are to fix some endianness issues
> and to have a register cache available during suspend - both of which
> don't
> apply in this case.
> 

I'd say one of the main advantage of regmap is that it does proper
locking of shared registers and I think that is the main reason of using
it when with platform drivers. Obviously, because you have _SET and _CLR
register on that platform you don't really care about locking.

> If you want to get rid of the callbacks we should just pass the pointer
> to the register block to the child device, I think. Which way is
> preferable
> is probably only a matter of taste so I won't override the driver authors
> decision unless there is some clear statement that this is the preferred
> style in the rtc subsystem.
> 

Ok. Like I said, this is not blocking, it was just a cleanup I was
suggesting.
However, this device is clearly an MFD and that callback feels a bit
hackish. Also registering the watchdog would probably better be done
from the mfd subsystem (this is the only RTC driver doing so).
If you have a look at the history, this was a patch from 2011 taken in
2013 so the kernel had plenty of time to evolve in between ;)

We are dealing with legacy and I'm fine with that driver staying that
way until it is absolutely necessary to change it.

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

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [rtc-linux] [PATCH] watchdog: stmp3xxx: Implement GETBOOTSTATUS

[Alexandre Belloni]

Hi,

On 01/10/2015 at 19:20:32 +0200, harald@ccbib.org wrote :
> Since the rtc and watchdog drivers need different bits of the same control
> register, we would need to use regmap_fields to pass them around.
> Unfortunately this means we can't use the _SET and _CLR registers, which
> means we need to use regmap everywhere to get proper locking. A lot of
> overhead for no benefit.
> 
> After reading lots of messages around the genesis and usage of
> regmap_mmio,
> it seems to me the intended usecases are to fix some endianness issues
> and to have a register cache available during suspend - both of which
> don't
> apply in this case.
> 

I'd say one of the main advantage of regmap is that it does proper
locking of shared registers and I think that is the main reason of using
it when with platform drivers. Obviously, because you have _SET and _CLR
register on that platform you don't really care about locking.

> If you want to get rid of the callbacks we should just pass the pointer
> to the register block to the child device, I think. Which way is
> preferable
> is probably only a matter of taste so I won't override the driver authors
> decision unless there is some clear statement that this is the preferred
> style in the rtc subsystem.
> 

Ok. Like I said, this is not blocking, it was just a cleanup I was
suggesting.
However, this device is clearly an MFD and that callback feels a bit
hackish. Also registering the watchdog would probably better be done
from the mfd subsystem (this is the only RTC driver doing so).
If you have a look at the history, this was a patch from 2011 taken in
2013 so the kernel had plenty of time to evolve in between ;)

We are dealing with legacy and I'm fine with that driver staying that
way until it is absolutely necessary to change it.

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