[PATCH v2 0/5] watchdog: pnx4008: add restart handler

[PATCH v2 0/5] watchdog: pnx4008: add restart handler

[slemieux.tyco]

From: Sylvain Lemieux <slemieux@tycoint.com>

The LPC32xx provide the capability to perform a soft or hard reset;
a soft reset will no reset the external peripheral
(i.e. RESOUT_N output not asserted).

This changeset migrate the restart code from the "mach-lpc32xx" to
the "pnx-4008" watchdog driver and add the soft reset capability.

The default behavior (i.e. hard reset) remain unchanged.

The core restart handler support added by this change:
2165bf524da5f5e496d - "watchdog: core: add restart handler support"
is not use in this changeset; it does not support the reboot mode.

---
Changes from v1 to v2:
* Rename patch title;
  was "arm: lpc32xx: restart: add support for soft reset"
* Update the patch as per the feedback received from:
  Joachim: http://permalink.gmane.org/gmane.linux.ports.arm.kernel/479791
  - migrate restart code from "mach-lpc32xx" to "pnx-4008" driver.
* Add soft reset support to "pnx-4008" driver instead of "mach-lpc32xx".
* Use define available in "pnx-4008" when writting to watchdog register.
* Remove restart handler from "mach-lpc32xx".
* Remove restart hook from "phy3250.c".
* Update driver logging during power-on. 

Sylvain Lemieux (5):
  watchdog: pnx4008: update logging during power-on
  watchdog: pnx4008: add restart handler
  watchdog: pnx4008: add support for soft reset
  watchdog: pnx4008: restart: support "cmd" from userspace
  arm: lpc32xx: remove restart handler

 arch/arm/mach-lpc32xx/common.c  | 15 ------------
 arch/arm/mach-lpc32xx/common.h  |  1 -
 arch/arm/mach-lpc32xx/phy3250.c |  1 -
 drivers/watchdog/pnx4008_wdt.c  | 53 +++++++++++++++++++++++++++++++++++++++--
 4 files changed, 51 insertions(+), 19 deletions(-)

-- 
1.8.3.1

[PATCH v2 1/5] watchdog: pnx4008: update logging during power-on

[slemieux.tyco]

From: Sylvain Lemieux <slemieux@tycoint.com>

There is no need add the driver name in the text to display
on the console during the power-on:
pnx4008-watchdog 4003c000.watchdog: PNX4008 Watchdog Timer: heartbeat 19 sec

Signed-off-by: Sylvain Lemieux <slemieux@tycoint.com>
---
Changes from v1 to v2:
- new patch in v2.

 drivers/watchdog/pnx4008_wdt.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/watchdog/pnx4008_wdt.c b/drivers/watchdog/pnx4008_wdt.c
index 313cd1c..88264a2 100644
--- a/drivers/watchdog/pnx4008_wdt.c
+++ b/drivers/watchdog/pnx4008_wdt.c
@@ -178,8 +178,7 @@ static int pnx4008_wdt_probe(struct platform_device *pdev)
 		goto disable_clk;
 	}
 
-	dev_info(&pdev->dev, "PNX4008 Watchdog Timer: heartbeat %d sec\n",
-		 pnx4008_wdd.timeout);
+	dev_info(&pdev->dev, "heartbeat %d sec\n", pnx4008_wdd.timeout);
 
 	return 0;
 
-- 
1.8.3.1

[PATCH v2 2/5] watchdog: pnx4008: add restart handler

[slemieux.tyco]

From: Sylvain Lemieux <slemieux@tycoint.com>

Add restart handler capability to the driver;
the restart handler implementation was taken from
"mach-lpc32xx" ("lpc23xx_restart" function).

Signed-off-by: Sylvain Lemieux <slemieux@tycoint.com>
---
Changes from v1 to v2:
- new patch in v2.

 drivers/watchdog/pnx4008_wdt.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/watchdog/pnx4008_wdt.c b/drivers/watchdog/pnx4008_wdt.c
index 88264a2..4b01b2b 100644
--- a/drivers/watchdog/pnx4008_wdt.c
+++ b/drivers/watchdog/pnx4008_wdt.c
@@ -31,7 +31,10 @@
 #include <linux/slab.h>
 #include <linux/err.h>
 #include <linux/of.h>
+#include <linux/delay.h>
+#include <linux/reboot.h>
 #include <mach/hardware.h>
+#include <mach/platform.h>
 
 /* WatchDog Timer - Chapter 23 Page 207 */
 
@@ -77,6 +80,7 @@
 
 static bool nowayout = WATCHDOG_NOWAYOUT;
 static unsigned int heartbeat = DEFAULT_HEARTBEAT;
+static struct notifier_block restart_handler;
 
 static DEFINE_SPINLOCK(io_lock);
 static void __iomem	*wdt_base;
@@ -124,6 +128,27 @@ static int pnx4008_wdt_set_timeout(struct watchdog_device *wdd,
 	return 0;
 }
 
+static void pnx4008_wdt_sys_reset(enum reboot_mode mode, const char *cmd)
+{
+	/* Make sure WDT clocks are enabled */
+	writel(LPC32XX_CLKPWR_PWMCLK_WDOG_EN, LPC32XX_CLKPWR_TIMER_CLK_CTRL);
+
+	/* Instant assert of RESETOUT_N with pulse length 1mS */
+	writel(13000, WDTIM_PULSE(wdt_base));
+	writel(M_RES2 | RESFRC1 | RESFRC2, WDTIM_MCTRL(wdt_base));
+
+	/* Wait for watchdog to reset system */
+	mdelay(1000);
+}
+
+static int pnx4008_restart_handler(struct notifier_block *this,
+				   unsigned long mode, void *cmd)
+{
+	pnx4008_wdt_sys_reset((enum reboot_mode)mode, cmd);
+
+	return NOTIFY_DONE;
+}
+
 static const struct watchdog_info pnx4008_wdt_ident = {
 	.options = WDIOF_CARDRESET | WDIOF_MAGICCLOSE |
 	    WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
@@ -157,6 +182,11 @@ static int pnx4008_wdt_probe(struct platform_device *pdev)
 	if (IS_ERR(wdt_base))
 		return PTR_ERR(wdt_base);
 
+	/* Register restart handler. */
+	restart_handler.notifier_call = pnx4008_restart_handler;
+	restart_handler.priority = 128;
+	ret = register_restart_handler(&restart_handler);
+
 	wdt_clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(wdt_clk))
 		return PTR_ERR(wdt_clk);
-- 
1.8.3.1

Re: [PATCH v2 2/5] watchdog: pnx4008: add restart handler

[Joachim Eastwood]

Hi Sylvain,

On 22 February 2016 at 17:55,  <slemieux.tyco@gmail.com> wrote:
> From: Sylvain Lemieux <slemieux@tycoint.com>
>
> Add restart handler capability to the driver;
> the restart handler implementation was taken from
> "mach-lpc32xx" ("lpc23xx_restart" function).

I think it would be better if you were to expand on "watchdog: core:
add restart handler support" to give you the parameters you need to
make the restart hook work for you. But that is up to watchdog
maintainers to decide.

> Signed-off-by: Sylvain Lemieux <slemieux@tycoint.com>
> ---
> Changes from v1 to v2:
> - new patch in v2.
>
>  drivers/watchdog/pnx4008_wdt.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>
> diff --git a/drivers/watchdog/pnx4008_wdt.c b/drivers/watchdog/pnx4008_wdt.c
> index 88264a2..4b01b2b 100644
> --- a/drivers/watchdog/pnx4008_wdt.c
> +++ b/drivers/watchdog/pnx4008_wdt.c
> @@ -31,7 +31,10 @@
>  #include <linux/slab.h>
>  #include <linux/err.h>
>  #include <linux/of.h>
> +#include <linux/delay.h>
> +#include <linux/reboot.h>
>  #include <mach/hardware.h>
> +#include <mach/platform.h>

What are you using from mach/platform.h?

>
>  /* WatchDog Timer - Chapter 23 Page 207 */
>
> @@ -77,6 +80,7 @@
>
>  static bool nowayout = WATCHDOG_NOWAYOUT;
>  static unsigned int heartbeat = DEFAULT_HEARTBEAT;
> +static struct notifier_block restart_handler;
>
>  static DEFINE_SPINLOCK(io_lock);
>  static void __iomem    *wdt_base;
> @@ -124,6 +128,27 @@ static int pnx4008_wdt_set_timeout(struct watchdog_device *wdd,
>         return 0;
>  }
>
> +static void pnx4008_wdt_sys_reset(enum reboot_mode mode, const char *cmd)
> +{
> +       /* Make sure WDT clocks are enabled */
> +       writel(LPC32XX_CLKPWR_PWMCLK_WDOG_EN, LPC32XX_CLKPWR_TIMER_CLK_CTRL);

Doesn't the watchdog driver already enable this clock in probe?
Either way doing a direct write to LPC32XX_CLKPWR_TIMER_CLK_CTRL from
here isn't very nice.


> +       /* Instant assert of RESETOUT_N with pulse length 1mS */
> +       writel(13000, WDTIM_PULSE(wdt_base));
> +       writel(M_RES2 | RESFRC1 | RESFRC2, WDTIM_MCTRL(wdt_base));
> +
> +       /* Wait for watchdog to reset system */
> +       mdelay(1000);
> +}
> +
> +static int pnx4008_restart_handler(struct notifier_block *this,
> +                                  unsigned long mode, void *cmd)
> +{
> +       pnx4008_wdt_sys_reset((enum reboot_mode)mode, cmd);

Why can you just the stuff from pnx4008_wdt_sys_reset() in this function?


> +
> +       return NOTIFY_DONE;
> +}
> +
>  static const struct watchdog_info pnx4008_wdt_ident = {
>         .options = WDIOF_CARDRESET | WDIOF_MAGICCLOSE |
>             WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
> @@ -157,6 +182,11 @@ static int pnx4008_wdt_probe(struct platform_device *pdev)
>         if (IS_ERR(wdt_base))
>                 return PTR_ERR(wdt_base);
>
> +       /* Register restart handler. */
> +       restart_handler.notifier_call = pnx4008_restart_handler;
> +       restart_handler.priority = 128;
> +       ret = register_restart_handler(&restart_handler);

Checking return value would be a nice thing to do.

You should also do a unregister_restart_handler() in driver remove hook.


regards,
Joachim Eastwood

Re: [PATCH v2 2/5] watchdog: pnx4008: add restart handler

[Sylvain Lemieux]

On Mon, 2016-02-22 at 19:03 +0100, Joachim Eastwood wrote:
> Hi Sylvain,
> 
> On 22 February 2016 at 17:55,  <slemieux.tyco@gmail.com> wrote:
> > From: Sylvain Lemieux <slemieux@tycoint.com>
> >
> > Add restart handler capability to the driver;
> > the restart handler implementation was taken from
> > "mach-lpc32xx" ("lpc23xx_restart" function).
> 
> I think it would be better if you were to expand on "watchdog: core:
> add restart handler support" to give you the parameters you need to
> make the restart hook work for you. But that is up to watchdog
> maintainers to decide.

I prefer to do this as a separate patchset;
I will wait for the watchdog maintainer feedback on this.

> 
> > Signed-off-by: Sylvain Lemieux <slemieux@tycoint.com>
> > ---
> > Changes from v1 to v2:
> > - new patch in v2.
> >
> >  drivers/watchdog/pnx4008_wdt.c | 30 ++++++++++++++++++++++++++++++
> >  1 file changed, 30 insertions(+)
> >
> > diff --git a/drivers/watchdog/pnx4008_wdt.c b/drivers/watchdog/pnx4008_wdt.c
> > index 88264a2..4b01b2b 100644
> > --- a/drivers/watchdog/pnx4008_wdt.c
> > +++ b/drivers/watchdog/pnx4008_wdt.c
> > @@ -31,7 +31,10 @@
> >  #include <linux/slab.h>
> >  #include <linux/err.h>
> >  #include <linux/of.h>
> > +#include <linux/delay.h>
> > +#include <linux/reboot.h>
> >  #include <mach/hardware.h>
> > +#include <mach/platform.h>
> 
> What are you using from mach/platform.h?
This file is require for the define use to directly
access the clock register (see note below).
> 
> >
> >  /* WatchDog Timer - Chapter 23 Page 207 */
> >
> > @@ -77,6 +80,7 @@
> >
> >  static bool nowayout = WATCHDOG_NOWAYOUT;
> >  static unsigned int heartbeat = DEFAULT_HEARTBEAT;
> > +static struct notifier_block restart_handler;
> >
> >  static DEFINE_SPINLOCK(io_lock);
> >  static void __iomem    *wdt_base;
> > @@ -124,6 +128,27 @@ static int pnx4008_wdt_set_timeout(struct watchdog_device *wdd,
> >         return 0;
> >  }
> >
> > +static void pnx4008_wdt_sys_reset(enum reboot_mode mode, const char *cmd)
> > +{
> > +       /* Make sure WDT clocks are enabled */
> > +       writel(LPC32XX_CLKPWR_PWMCLK_WDOG_EN, LPC32XX_CLKPWR_TIMER_CLK_CTRL);
> 
> Doesn't the watchdog driver already enable this clock in probe?
> Either way doing a direct write to LPC32XX_CLKPWR_TIMER_CLK_CTRL from
> here isn't very nice.
> 
I keep this from the "mach-lpc32xx" implementation, in case the call to
the "devm_clk_get" fail in the probe; the restart will still work.

I can remove it, and the platform.h include.
> 
> > +       /* Instant assert of RESETOUT_N with pulse length 1mS */
> > +       writel(13000, WDTIM_PULSE(wdt_base));
> > +       writel(M_RES2 | RESFRC1 | RESFRC2, WDTIM_MCTRL(wdt_base));
> > +
> > +       /* Wait for watchdog to reset system */
> > +       mdelay(1000);
> > +}
> > +
> > +static int pnx4008_restart_handler(struct notifier_block *this,
> > +                                  unsigned long mode, void *cmd)
> > +{
> > +       pnx4008_wdt_sys_reset((enum reboot_mode)mode, cmd);
> 
> Why can you just the stuff from pnx4008_wdt_sys_reset() in this function?
> 
OK
> 
> > +
> > +       return NOTIFY_DONE;
> > +}
> > +
> >  static const struct watchdog_info pnx4008_wdt_ident = {
> >         .options = WDIOF_CARDRESET | WDIOF_MAGICCLOSE |
> >             WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
> > @@ -157,6 +182,11 @@ static int pnx4008_wdt_probe(struct platform_device *pdev)
> >         if (IS_ERR(wdt_base))
> >                 return PTR_ERR(wdt_base);
> >
> > +       /* Register restart handler. */
> > +       restart_handler.notifier_call = pnx4008_restart_handler;
> > +       restart_handler.priority = 128;
> > +       ret = register_restart_handler(&restart_handler);
> 
> Checking return value would be a nice thing to do.
OK
> 
> You should also do a unregister_restart_handler() in driver remove hook.
Thanks for catching it.

I will wait, in case there is other feedback, and submit a revision 3
of the patchset.
> 
> 
> regards,
> Joachim Eastwood

[PATCH v2 3/5] watchdog: pnx4008: add support for soft reset

[slemieux.tyco]

From: Sylvain Lemieux <slemieux@tycoint.com>

Add support for explicit soft reset using the reboot mode.

The default reboot mode behavior is unchanged;
you can overwrite the default reboot type in the board specific file
"DT_MACHINE_START" definition using the "reboot_mode" parameter.

Signed-off-by: Sylvain Lemieux <slemieux@tycoint.com>
---
Changes from v1 to v2:
- Rename patch title;
  was "arm: lpc32xx: restart: add support for soft reset"
- Add change to "pnx-4008" driver instead of "mach-lpc32xx".
- Use define available in "pnx-4008" when writting to watchdog register.

 drivers/watchdog/pnx4008_wdt.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/watchdog/pnx4008_wdt.c b/drivers/watchdog/pnx4008_wdt.c
index 4b01b2b..2275fd4 100644
--- a/drivers/watchdog/pnx4008_wdt.c
+++ b/drivers/watchdog/pnx4008_wdt.c
@@ -133,9 +133,16 @@ static void pnx4008_wdt_sys_reset(enum reboot_mode mode, const char *cmd)
 	/* Make sure WDT clocks are enabled */
 	writel(LPC32XX_CLKPWR_PWMCLK_WDOG_EN, LPC32XX_CLKPWR_TIMER_CLK_CTRL);
 
-	/* Instant assert of RESETOUT_N with pulse length 1mS */
-	writel(13000, WDTIM_PULSE(wdt_base));
-	writel(M_RES2 | RESFRC1 | RESFRC2, WDTIM_MCTRL(wdt_base));
+	if (mode == REBOOT_SOFT) {
+		/* Force match output active */
+		writel(EXT_MATCH0, WDTIM_EMR(wdt_base));
+		/* Internal reset on match output (RESOUT_N not asserted) */
+		writel(M_RES1, WDTIM_MCTRL(wdt_base));
+	} else {
+		/* Instant assert of RESETOUT_N with pulse length 1mS */
+		writel(13000, WDTIM_PULSE(wdt_base));
+		writel(M_RES2 | RESFRC1 | RESFRC2, WDTIM_MCTRL(wdt_base));
+	}
 
 	/* Wait for watchdog to reset system */
 	mdelay(1000);
-- 
1.8.3.1

[PATCH v2 4/5] watchdog: pnx4008: restart: support "cmd" from userspace

[slemieux.tyco]

From: Sylvain Lemieux <slemieux@tycoint.com>

Added support to verify if a "cmd" is passed from the userspace program rebooting the system;
- if a valid "cmd" is available, handle it;
- If the received "cmd" is not supported, use the default reboot mode.

Signed-off-by: Sylvain Lemieux <slemieux@tycoint.com>
---
Changes from v1 to v2:
- Rename patch title;
  was "arm: lpc32xx: restart: support "cmd" from userspace"
- Add change to "pnx-4008" driver instead of "mach-lpc32xx".

 drivers/watchdog/pnx4008_wdt.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/watchdog/pnx4008_wdt.c b/drivers/watchdog/pnx4008_wdt.c
index 2275fd4..5f34299 100644
--- a/drivers/watchdog/pnx4008_wdt.c
+++ b/drivers/watchdog/pnx4008_wdt.c
@@ -133,6 +133,19 @@ static void pnx4008_wdt_sys_reset(enum reboot_mode mode, const char *cmd)
 	/* Make sure WDT clocks are enabled */
 	writel(LPC32XX_CLKPWR_PWMCLK_WDOG_EN, LPC32XX_CLKPWR_TIMER_CLK_CTRL);
 
+	/*
+	 * Verify if a "cmd" passed from the userspace program rebooting
+	 * the system; if available, and handle it.
+	 * - For details, see the 'reboot' syscall in kernel/reboot.c
+	 * - If the received "cmd" is not supported, use the default mode.
+	 */
+	if (cmd) {
+		if (cmd[0] == 'h')
+			mode = REBOOT_HARD;
+		else if (cmd[0] == 's')
+			mode = REBOOT_SOFT;
+	}
+
 	if (mode == REBOOT_SOFT) {
 		/* Force match output active */
 		writel(EXT_MATCH0, WDTIM_EMR(wdt_base));
-- 
1.8.3.1

[PATCH v2 5/5] arm: lpc32xx: remove restart handler

[slemieux.tyco]

From: Sylvain Lemieux <slemieux@tycoint.com>

Remove the restart handler from "mach-lpc32xx";
this functionality is now available in the
pnx4008 watchdog driver.

Signed-off-by: Sylvain Lemieux <slemieux@tycoint.com>
---
Changes from v1 to v2:
- new patch in v2.

 arch/arm/mach-lpc32xx/common.c  | 15 ---------------
 arch/arm/mach-lpc32xx/common.h  |  1 -
 arch/arm/mach-lpc32xx/phy3250.c |  1 -
 3 files changed, 17 deletions(-)

diff --git a/arch/arm/mach-lpc32xx/common.c b/arch/arm/mach-lpc32xx/common.c
index 716e83e..5b7a1e7 100644
--- a/arch/arm/mach-lpc32xx/common.c
+++ b/arch/arm/mach-lpc32xx/common.c
@@ -194,21 +194,6 @@ void __init lpc32xx_map_io(void)
 	iotable_init(lpc32xx_io_desc, ARRAY_SIZE(lpc32xx_io_desc));
 }
 
-void lpc23xx_restart(enum reboot_mode mode, const char *cmd)
-{
-	/* Make sure WDT clocks are enabled */
-	__raw_writel(LPC32XX_CLKPWR_PWMCLK_WDOG_EN,
-		LPC32XX_CLKPWR_TIMER_CLK_CTRL);
-
-	/* Instant assert of RESETOUT_N with pulse length 1mS */
-	__raw_writel(13000, io_p2v(LPC32XX_WDTIM_BASE + 0x18));
-	__raw_writel(0x70, io_p2v(LPC32XX_WDTIM_BASE + 0xC));
-
-	/* Wait for watchdog to reset system */
-	while (1)
-		;
-}
-
 static int __init lpc32xx_check_uid(void)
 {
 	u32 uid[4];
diff --git a/arch/arm/mach-lpc32xx/common.h b/arch/arm/mach-lpc32xx/common.h
index 1cd8853..2d90801 100644
--- a/arch/arm/mach-lpc32xx/common.h
+++ b/arch/arm/mach-lpc32xx/common.h
@@ -30,7 +30,6 @@ extern void lpc32xx_timer_init(void);
 extern void __init lpc32xx_init_irq(void);
 extern void __init lpc32xx_map_io(void);
 extern void __init lpc32xx_serial_init(void);
-extern void lpc23xx_restart(enum reboot_mode, const char *);
 
 
 /*
diff --git a/arch/arm/mach-lpc32xx/phy3250.c b/arch/arm/mach-lpc32xx/phy3250.c
index 77d6b1b..942bc3f 100644
--- a/arch/arm/mach-lpc32xx/phy3250.c
+++ b/arch/arm/mach-lpc32xx/phy3250.c
@@ -263,5 +263,4 @@ DT_MACHINE_START(LPC32XX_DT, "LPC32XX SoC (Flattened Device Tree)")
 	.init_time	= lpc32xx_timer_init,
 	.init_machine	= lpc3250_machine_init,
 	.dt_compat	= lpc32xx_dt_compat,
-	.restart	= lpc23xx_restart,
 MACHINE_END
-- 
1.8.3.1