[PATCH 1/5] watchdog: imx7ulp: Fix reboot hang

[PATCH 1/5] watchdog: imx7ulp: Fix reboot hang

[Fabio Estevam]

The following hang is observed when a 'reboot' command is issued:

# reboot
# Stopping network: OK
Stopping klogd: OK
Stopping syslogd: OK
umount: devtmpfs busy - remounted read-only
[    8.612079] EXT4-fs (mmcblk0p2): re-mounted. Opts: (null)
The system is going down NOW!
Sent SIGTERM to all processes
Sent SIGKILL to all processes
Requesting system reboot
[   10.694753] reboot: Restarting system
[   11.699008] Reboot failed -- System halted

Fix this problem by adding a .restart ops member.

Fixes: 41b630f41bf7 ("watchdog: Add i.MX7ULP watchdog support")
Signed-off-by: Fabio Estevam <festevam@gmail.com>
---
 drivers/watchdog/imx7ulp_wdt.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c
index 5ce51026989a..ba5d535a6db2 100644
--- a/drivers/watchdog/imx7ulp_wdt.c
+++ b/drivers/watchdog/imx7ulp_wdt.c
@@ -106,12 +106,28 @@ static int imx7ulp_wdt_set_timeout(struct watchdog_device *wdog,
 	return 0;
 }
 
+static int imx7ulp_wdt_restart(struct watchdog_device *wdog,
+			       unsigned long action, void *data)
+{
+	struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
+
+	imx7ulp_wdt_enable(wdt->base, true);
+	imx7ulp_wdt_set_timeout(&wdt->wdd, 1);
+
+	/* wait for wdog to fire */
+	while (true)
+		;
+
+	return NOTIFY_DONE;
+}
+
 static const struct watchdog_ops imx7ulp_wdt_ops = {
 	.owner = THIS_MODULE,
 	.start = imx7ulp_wdt_start,
 	.stop  = imx7ulp_wdt_stop,
 	.ping  = imx7ulp_wdt_ping,
 	.set_timeout = imx7ulp_wdt_set_timeout,
+	.restart = imx7ulp_wdt_restart,
 };
 
 static const struct watchdog_info imx7ulp_wdt_info = {
-- 
2.17.1

[PATCH 2/5] watchdog: imx7ulp: Pass the wdog instance inimx7ulp_wdt_enable()

[Fabio Estevam]

It is more natural to pass the watchdog instance inside
imx7ulp_wdt_enable() instead of the base address.

This also has the benefit to reduce the code a bit.

Signed-off-by: Fabio Estevam <festevam@gmail.com>
---
 drivers/watchdog/imx7ulp_wdt.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c
index ba5d535a6db2..eea415609d44 100644
--- a/drivers/watchdog/imx7ulp_wdt.c
+++ b/drivers/watchdog/imx7ulp_wdt.c
@@ -47,15 +47,17 @@ struct imx7ulp_wdt_device {
 	struct clk *clk;
 };
 
-static inline void imx7ulp_wdt_enable(void __iomem *base, bool enable)
+static inline void imx7ulp_wdt_enable(struct watchdog_device *wdog, bool enable)
 {
-	u32 val = readl(base + WDOG_CS);
+	struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
+
+	u32 val = readl(wdt->base + WDOG_CS);
 
-	writel(UNLOCK, base + WDOG_CNT);
+	writel(UNLOCK, wdt->base + WDOG_CNT);
 	if (enable)
-		writel(val | WDOG_CS_EN, base + WDOG_CS);
+		writel(val | WDOG_CS_EN, wdt->base + WDOG_CS);
 	else
-		writel(val & ~WDOG_CS_EN, base + WDOG_CS);
+		writel(val & ~WDOG_CS_EN, wdt->base + WDOG_CS);
 }
 
 static inline bool imx7ulp_wdt_is_enabled(void __iomem *base)
@@ -76,18 +78,15 @@ static int imx7ulp_wdt_ping(struct watchdog_device *wdog)
 
 static int imx7ulp_wdt_start(struct watchdog_device *wdog)
 {
-	struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
 
-	imx7ulp_wdt_enable(wdt->base, true);
+	imx7ulp_wdt_enable(wdog, true);
 
 	return 0;
 }
 
 static int imx7ulp_wdt_stop(struct watchdog_device *wdog)
 {
-	struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
-
-	imx7ulp_wdt_enable(wdt->base, false);
+	imx7ulp_wdt_enable(wdog, false);
 
 	return 0;
 }
@@ -109,10 +108,8 @@ static int imx7ulp_wdt_set_timeout(struct watchdog_device *wdog,
 static int imx7ulp_wdt_restart(struct watchdog_device *wdog,
 			       unsigned long action, void *data)
 {
-	struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
-
-	imx7ulp_wdt_enable(wdt->base, true);
-	imx7ulp_wdt_set_timeout(&wdt->wdd, 1);
+	imx7ulp_wdt_enable(wdog, true);
+	imx7ulp_wdt_set_timeout(wdog, 1);
 
 	/* wait for wdog to fire */
 	while (true)
-- 
2.17.1

Re: [PATCH 2/5] watchdog: imx7ulp: Pass the wdog instance inimx7ulp_wdt_enable()

[Guenter Roeck]

On Tue, Oct 29, 2019 at 02:40:34PM -0300, Fabio Estevam wrote:
> It is more natural to pass the watchdog instance inside
> imx7ulp_wdt_enable() instead of the base address.
> 
> This also has the benefit to reduce the code a bit.
> 
> Signed-off-by: Fabio Estevam <festevam@gmail.com>

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

> ---
>  drivers/watchdog/imx7ulp_wdt.c | 25 +++++++++++--------------
>  1 file changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c
> index ba5d535a6db2..eea415609d44 100644
> --- a/drivers/watchdog/imx7ulp_wdt.c
> +++ b/drivers/watchdog/imx7ulp_wdt.c
> @@ -47,15 +47,17 @@ struct imx7ulp_wdt_device {
>  	struct clk *clk;
>  };
>  
> -static inline void imx7ulp_wdt_enable(void __iomem *base, bool enable)
> +static inline void imx7ulp_wdt_enable(struct watchdog_device *wdog, bool enable)
>  {
> -	u32 val = readl(base + WDOG_CS);
> +	struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
> +
> +	u32 val = readl(wdt->base + WDOG_CS);
>  
> -	writel(UNLOCK, base + WDOG_CNT);
> +	writel(UNLOCK, wdt->base + WDOG_CNT);
>  	if (enable)
> -		writel(val | WDOG_CS_EN, base + WDOG_CS);
> +		writel(val | WDOG_CS_EN, wdt->base + WDOG_CS);
>  	else
> -		writel(val & ~WDOG_CS_EN, base + WDOG_CS);
> +		writel(val & ~WDOG_CS_EN, wdt->base + WDOG_CS);
>  }
>  
>  static inline bool imx7ulp_wdt_is_enabled(void __iomem *base)
> @@ -76,18 +78,15 @@ static int imx7ulp_wdt_ping(struct watchdog_device *wdog)
>  
>  static int imx7ulp_wdt_start(struct watchdog_device *wdog)
>  {
> -	struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
>  
> -	imx7ulp_wdt_enable(wdt->base, true);
> +	imx7ulp_wdt_enable(wdog, true);
>  
>  	return 0;
>  }
>  
>  static int imx7ulp_wdt_stop(struct watchdog_device *wdog)
>  {
> -	struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
> -
> -	imx7ulp_wdt_enable(wdt->base, false);
> +	imx7ulp_wdt_enable(wdog, false);
>  
>  	return 0;
>  }
> @@ -109,10 +108,8 @@ static int imx7ulp_wdt_set_timeout(struct watchdog_device *wdog,
>  static int imx7ulp_wdt_restart(struct watchdog_device *wdog,
>  			       unsigned long action, void *data)
>  {
> -	struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
> -
> -	imx7ulp_wdt_enable(wdt->base, true);
> -	imx7ulp_wdt_set_timeout(&wdt->wdd, 1);
> +	imx7ulp_wdt_enable(wdog, true);
> +	imx7ulp_wdt_set_timeout(wdog, 1);
>  
>  	/* wait for wdog to fire */
>  	while (true)

[PATCH 3/5] watchdog: imx7ulp: Remove unused structure member

[Fabio Estevam]

The 'notifier_block' structure member is unused, so just remove it.

Signed-off-by: Fabio Estevam <festevam@gmail.com>
---
 drivers/watchdog/imx7ulp_wdt.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c
index 670102924bc1..ed4f7d439f2e 100644
--- a/drivers/watchdog/imx7ulp_wdt.c
+++ b/drivers/watchdog/imx7ulp_wdt.c
@@ -41,7 +41,6 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
 		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
 
 struct imx7ulp_wdt_device {
-	struct notifier_block restart_handler;
 	struct watchdog_device wdd;
 	void __iomem *base;
 	struct clk *clk;
-- 
2.17.1

Re: [PATCH 3/5] watchdog: imx7ulp: Remove unused structure member

[Guenter Roeck]

On Tue, Oct 29, 2019 at 02:40:35PM -0300, Fabio Estevam wrote:
> The 'notifier_block' structure member is unused, so just remove it.
> 
> Signed-off-by: Fabio Estevam <festevam@gmail.com>

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

> ---
>  drivers/watchdog/imx7ulp_wdt.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c
> index 670102924bc1..ed4f7d439f2e 100644
> --- a/drivers/watchdog/imx7ulp_wdt.c
> +++ b/drivers/watchdog/imx7ulp_wdt.c
> @@ -41,7 +41,6 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
>  		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>  
>  struct imx7ulp_wdt_device {
> -	struct notifier_block restart_handler;
>  	struct watchdog_device wdd;
>  	void __iomem *base;
>  	struct clk *clk;

[PATCH 4/5] watchdog: imx7ulp: Remove inline annotations

[Fabio Estevam]

Compiler is smart enough and knows when to inline, so there
is no need to mark some of these functions as 'inline'.

Remove such annotations.

Signed-off-by: Fabio Estevam <festevam@gmail.com>
---
 drivers/watchdog/imx7ulp_wdt.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c
index ed4f7d439f2e..bcef3b6a9782 100644
--- a/drivers/watchdog/imx7ulp_wdt.c
+++ b/drivers/watchdog/imx7ulp_wdt.c
@@ -46,7 +46,7 @@ struct imx7ulp_wdt_device {
 	struct clk *clk;
 };
 
-static inline void imx7ulp_wdt_enable(struct watchdog_device *wdog, bool enable)
+static void imx7ulp_wdt_enable(struct watchdog_device *wdog, bool enable)
 {
 	struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
 
@@ -59,7 +59,7 @@ static inline void imx7ulp_wdt_enable(struct watchdog_device *wdog, bool enable)
 		writel(val & ~WDOG_CS_EN, wdt->base + WDOG_CS);
 }
 
-static inline bool imx7ulp_wdt_is_enabled(void __iomem *base)
+static bool imx7ulp_wdt_is_enabled(void __iomem *base)
 {
 	u32 val = readl(base + WDOG_CS);
 
@@ -132,7 +132,7 @@ static const struct watchdog_info imx7ulp_wdt_info = {
 		    WDIOF_MAGICCLOSE,
 };
 
-static inline void imx7ulp_wdt_init(void __iomem *base, unsigned int timeout)
+static void imx7ulp_wdt_init(void __iomem *base, unsigned int timeout)
 {
 	u32 val;
 
-- 
2.17.1

Re: [PATCH 4/5] watchdog: imx7ulp: Remove inline annotations

[Guenter Roeck]

On Tue, Oct 29, 2019 at 02:40:36PM -0300, Fabio Estevam wrote:
> Compiler is smart enough and knows when to inline, so there
> is no need to mark some of these functions as 'inline'.
> 
> Remove such annotations.
> 
> Signed-off-by: Fabio Estevam <festevam@gmail.com>

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

> ---
>  drivers/watchdog/imx7ulp_wdt.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c
> index ed4f7d439f2e..bcef3b6a9782 100644
> --- a/drivers/watchdog/imx7ulp_wdt.c
> +++ b/drivers/watchdog/imx7ulp_wdt.c
> @@ -46,7 +46,7 @@ struct imx7ulp_wdt_device {
>  	struct clk *clk;
>  };
>  
> -static inline void imx7ulp_wdt_enable(struct watchdog_device *wdog, bool enable)
> +static void imx7ulp_wdt_enable(struct watchdog_device *wdog, bool enable)
>  {
>  	struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
>  
> @@ -59,7 +59,7 @@ static inline void imx7ulp_wdt_enable(struct watchdog_device *wdog, bool enable)
>  		writel(val & ~WDOG_CS_EN, wdt->base + WDOG_CS);
>  }
>  
> -static inline bool imx7ulp_wdt_is_enabled(void __iomem *base)
> +static bool imx7ulp_wdt_is_enabled(void __iomem *base)
>  {
>  	u32 val = readl(base + WDOG_CS);
>  
> @@ -132,7 +132,7 @@ static const struct watchdog_info imx7ulp_wdt_info = {
>  		    WDIOF_MAGICCLOSE,
>  };
>  
> -static inline void imx7ulp_wdt_init(void __iomem *base, unsigned int timeout)
> +static void imx7ulp_wdt_init(void __iomem *base, unsigned int timeout)
>  {
>  	u32 val;
>  

[PATCH 5/5] watchdog: imx7ulp: Use definitions instead of magic values

[Fabio Estevam]

Use definitions instead of magic values in order to improve readability.

Since the CLK field of the WDOG CS register is composed of two bits
to select the watchdog clock source, use a shift representation
instead of BIT().

Signed-off-by: Fabio Estevam <festevam@gmail.com>
---
 drivers/watchdog/imx7ulp_wdt.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c
index bcef3b6a9782..e3c1382dac52 100644
--- a/drivers/watchdog/imx7ulp_wdt.c
+++ b/drivers/watchdog/imx7ulp_wdt.c
@@ -17,6 +17,9 @@
 #define WDOG_CS_CMD32EN		BIT(13)
 #define WDOG_CS_ULK		BIT(11)
 #define WDOG_CS_RCS		BIT(10)
+#define LPO_CLK			0x1
+#define LPO_CLK_SHIFT		8
+#define WDOG_CS_CLK		(LPO_CLK << LPO_CLK_SHIFT)
 #define WDOG_CS_EN		BIT(7)
 #define WDOG_CS_UPDATE		BIT(5)
 
@@ -143,7 +146,7 @@ static void imx7ulp_wdt_init(void __iomem *base, unsigned int timeout)
 	/* set an initial timeout value in TOVAL */
 	writel(timeout, base + WDOG_TOVAL);
 	/* enable 32bit command sequence and reconfigure */
-	val = BIT(13) | BIT(8) | BIT(5);
+	val = WDOG_CS_CMD32EN | WDOG_CS_CLK | WDOG_CS_UPDATE;
 	writel(val, base + WDOG_CS);
 }
 
-- 
2.17.1

Re: [PATCH 5/5] watchdog: imx7ulp: Use definitions instead of magic values

[Guenter Roeck]

On Tue, Oct 29, 2019 at 02:40:37PM -0300, Fabio Estevam wrote:
> Use definitions instead of magic values in order to improve readability.
> 
> Since the CLK field of the WDOG CS register is composed of two bits
> to select the watchdog clock source, use a shift representation
> instead of BIT().
> 
> Signed-off-by: Fabio Estevam <festevam@gmail.com>

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

> ---
>  drivers/watchdog/imx7ulp_wdt.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c
> index bcef3b6a9782..e3c1382dac52 100644
> --- a/drivers/watchdog/imx7ulp_wdt.c
> +++ b/drivers/watchdog/imx7ulp_wdt.c
> @@ -17,6 +17,9 @@
>  #define WDOG_CS_CMD32EN		BIT(13)
>  #define WDOG_CS_ULK		BIT(11)
>  #define WDOG_CS_RCS		BIT(10)
> +#define LPO_CLK			0x1
> +#define LPO_CLK_SHIFT		8
> +#define WDOG_CS_CLK		(LPO_CLK << LPO_CLK_SHIFT)
>  #define WDOG_CS_EN		BIT(7)
>  #define WDOG_CS_UPDATE		BIT(5)
>  
> @@ -143,7 +146,7 @@ static void imx7ulp_wdt_init(void __iomem *base, unsigned int timeout)
>  	/* set an initial timeout value in TOVAL */
>  	writel(timeout, base + WDOG_TOVAL);
>  	/* enable 32bit command sequence and reconfigure */
> -	val = BIT(13) | BIT(8) | BIT(5);
> +	val = WDOG_CS_CMD32EN | WDOG_CS_CLK | WDOG_CS_UPDATE;
>  	writel(val, base + WDOG_CS);
>  }
>  

Re: [PATCH 1/5] watchdog: imx7ulp: Fix reboot hang

[Guenter Roeck]

On Tue, Oct 29, 2019 at 02:40:33PM -0300, Fabio Estevam wrote:
> The following hang is observed when a 'reboot' command is issued:
> 
> # reboot
> # Stopping network: OK
> Stopping klogd: OK
> Stopping syslogd: OK
> umount: devtmpfs busy - remounted read-only
> [    8.612079] EXT4-fs (mmcblk0p2): re-mounted. Opts: (null)
> The system is going down NOW!
> Sent SIGTERM to all processes
> Sent SIGKILL to all processes
> Requesting system reboot
> [   10.694753] reboot: Restarting system
> [   11.699008] Reboot failed -- System halted
> 
> Fix this problem by adding a .restart ops member.
> 
> Fixes: 41b630f41bf7 ("watchdog: Add i.MX7ULP watchdog support")
> Signed-off-by: Fabio Estevam <festevam@gmail.com>

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

However, just to be sure: This registers the watchdog based restart handler
as restart handler of last resort. I assume this on purpose, I just want
to make sure it is intentional since it is not explicitly mentioned in
the commit message.

Thanks,
Guenter

> ---
>  drivers/watchdog/imx7ulp_wdt.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c
> index 5ce51026989a..ba5d535a6db2 100644
> --- a/drivers/watchdog/imx7ulp_wdt.c
> +++ b/drivers/watchdog/imx7ulp_wdt.c
> @@ -106,12 +106,28 @@ static int imx7ulp_wdt_set_timeout(struct watchdog_device *wdog,
>  	return 0;
>  }
>  
> +static int imx7ulp_wdt_restart(struct watchdog_device *wdog,
> +			       unsigned long action, void *data)
> +{
> +	struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
> +
> +	imx7ulp_wdt_enable(wdt->base, true);
> +	imx7ulp_wdt_set_timeout(&wdt->wdd, 1);
> +
> +	/* wait for wdog to fire */
> +	while (true)
> +		;
> +
> +	return NOTIFY_DONE;
> +}
> +
>  static const struct watchdog_ops imx7ulp_wdt_ops = {
>  	.owner = THIS_MODULE,
>  	.start = imx7ulp_wdt_start,
>  	.stop  = imx7ulp_wdt_stop,
>  	.ping  = imx7ulp_wdt_ping,
>  	.set_timeout = imx7ulp_wdt_set_timeout,
> +	.restart = imx7ulp_wdt_restart,
>  };
>  
>  static const struct watchdog_info imx7ulp_wdt_info = {

Re: [PATCH 1/5] watchdog: imx7ulp: Fix reboot hang

[Fabio Estevam]

Hi Guenter,

On Sat, Nov 2, 2019 at 12:36 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Tue, Oct 29, 2019 at 02:40:33PM -0300, Fabio Estevam wrote:
> > The following hang is observed when a 'reboot' command is issued:
> >
> > # reboot
> > # Stopping network: OK
> > Stopping klogd: OK
> > Stopping syslogd: OK
> > umount: devtmpfs busy - remounted read-only
> > [    8.612079] EXT4-fs (mmcblk0p2): re-mounted. Opts: (null)
> > The system is going down NOW!
> > Sent SIGTERM to all processes
> > Sent SIGKILL to all processes
> > Requesting system reboot
> > [   10.694753] reboot: Restarting system
> > [   11.699008] Reboot failed -- System halted
> >
> > Fix this problem by adding a .restart ops member.
> >
> > Fixes: 41b630f41bf7 ("watchdog: Add i.MX7ULP watchdog support")
> > Signed-off-by: Fabio Estevam <festevam@gmail.com>
>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>
> However, just to be sure: This registers the watchdog based restart handler
> as restart handler of last resort. I assume this on purpose, I just want
> to make sure it is intentional since it is not explicitly mentioned in
> the commit message.

To be honest, I thought that registering the restart handler was mandatory.

By the way, I have just noticed that even though this patch fixes the
reboot on a imx7ulp evk board, it does not work on a imx7ulp com
board.

I will debug this, so please discard this patch for now. The other
ones of this series should be fine to apply.

Thanks

Re: [PATCH 1/5] watchdog: imx7ulp: Fix reboot hang

[Guenter Roeck]

On 11/4/19 5:45 AM, Fabio Estevam wrote:
> Hi Guenter,
> 
> On Sat, Nov 2, 2019 at 12:36 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On Tue, Oct 29, 2019 at 02:40:33PM -0300, Fabio Estevam wrote:
>>> The following hang is observed when a 'reboot' command is issued:
>>>
>>> # reboot
>>> # Stopping network: OK
>>> Stopping klogd: OK
>>> Stopping syslogd: OK
>>> umount: devtmpfs busy - remounted read-only
>>> [    8.612079] EXT4-fs (mmcblk0p2): re-mounted. Opts: (null)
>>> The system is going down NOW!
>>> Sent SIGTERM to all processes
>>> Sent SIGKILL to all processes
>>> Requesting system reboot
>>> [   10.694753] reboot: Restarting system
>>> [   11.699008] Reboot failed -- System halted
>>>
>>> Fix this problem by adding a .restart ops member.
>>>
>>> Fixes: 41b630f41bf7 ("watchdog: Add i.MX7ULP watchdog support")
>>> Signed-off-by: Fabio Estevam <festevam@gmail.com>
>>
>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>>
>> However, just to be sure: This registers the watchdog based restart handler
>> as restart handler of last resort. I assume this on purpose, I just want
>> to make sure it is intentional since it is not explicitly mentioned in
>> the commit message.
> 
> To be honest, I thought that registering the restart handler was mandatory.
> 
Maybe I should have said "there is no call to watchdog_set_restart_priority()".

> By the way, I have just noticed that even though this patch fixes the
> reboot on a imx7ulp evk board, it does not work on a imx7ulp com
> board.
> 
> I will debug this, so please discard this patch for now. The other
> ones of this series should be fine to apply.
> 

Thanks for the update.

Guenter

Re: [PATCH 1/5] watchdog: imx7ulp: Fix reboot hang

[Fabio Estevam]

Hi Guenter,

On Mon, Nov 4, 2019 at 11:06 AM Guenter Roeck <linux@roeck-us.net> wrote:

> Maybe I should have said "there is no call to watchdog_set_restart_priority()".

Would you like me to respin this series with a call to
watchdog_set_restart_priority()?

> > By the way, I have just noticed that even though this patch fixes the
> > reboot on a imx7ulp evk board, it does not work on a imx7ulp com
> > board.
> >
> > I will debug this, so please discard this patch for now. The other
> > ones of this series should be fine to apply.
> >
>
> Thanks for the update.

I have just retested this on a imx7ulp-com and imx7ulp-evk boards and
the "reboot" command is working fine on the two boards.

Looks like the issue I had earlier was related to dtb.

Thanks

Re: [PATCH 1/5] watchdog: imx7ulp: Fix reboot hang

[Guenter Roeck]

On 11/7/19 9:46 AM, Fabio Estevam wrote:
> Hi Guenter,
> 
> On Mon, Nov 4, 2019 at 11:06 AM Guenter Roeck <linux@roeck-us.net> wrote:
> 
>> Maybe I should have said "there is no call to watchdog_set_restart_priority()".
> 
> Would you like me to respin this series with a call to
> watchdog_set_restart_priority()?
> 

Your call, really. As written, the restart handler serves as restart handler
of last resort. If that is the intention we are fine.

>>> By the way, I have just noticed that even though this patch fixes the
>>> reboot on a imx7ulp evk board, it does not work on a imx7ulp com
>>> board.
>>>
>>> I will debug this, so please discard this patch for now. The other
>>> ones of this series should be fine to apply.
>>>
>>
>> Thanks for the update.
> 
> I have just retested this on a imx7ulp-com and imx7ulp-evk boards and
> the "reboot" command is working fine on the two boards.
> 
> Looks like the issue I had earlier was related to dtb.
> 

Thanks for the update!

Guenter