linux-watchdog.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/5] watchdog: imx7ulp: Fix reboot hang
@ 2019-10-29 17:40 Fabio Estevam
  2019-10-29 17:40 ` [PATCH 2/5] watchdog: imx7ulp: Pass the wdog instance inimx7ulp_wdt_enable() Fabio Estevam
                   ` (4 more replies)
  0 siblings, 5 replies; 14+ messages in thread
From: Fabio Estevam @ 2019-10-29 17:40 UTC (permalink / raw)
  To: linux
  Cc: wim, linux-watchdog, Anson.Huang, shawnguo, linux-imx, kernel,
	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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/5] watchdog: imx7ulp: Pass the wdog instance inimx7ulp_wdt_enable()
  2019-10-29 17:40 [PATCH 1/5] watchdog: imx7ulp: Fix reboot hang Fabio Estevam
@ 2019-10-29 17:40 ` Fabio Estevam
  2019-11-02 15:37   ` Guenter Roeck
  2019-10-29 17:40 ` [PATCH 3/5] watchdog: imx7ulp: Remove unused structure member Fabio Estevam
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 14+ messages in thread
From: Fabio Estevam @ 2019-10-29 17:40 UTC (permalink / raw)
  To: linux
  Cc: wim, linux-watchdog, Anson.Huang, shawnguo, linux-imx, kernel,
	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



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 3/5] watchdog: imx7ulp: Remove unused structure member
  2019-10-29 17:40 [PATCH 1/5] watchdog: imx7ulp: Fix reboot hang Fabio Estevam
  2019-10-29 17:40 ` [PATCH 2/5] watchdog: imx7ulp: Pass the wdog instance inimx7ulp_wdt_enable() Fabio Estevam
@ 2019-10-29 17:40 ` Fabio Estevam
  2019-11-02 15:39   ` Guenter Roeck
  2019-10-29 17:40 ` [PATCH 4/5] watchdog: imx7ulp: Remove inline annotations Fabio Estevam
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 14+ messages in thread
From: Fabio Estevam @ 2019-10-29 17:40 UTC (permalink / raw)
  To: linux
  Cc: wim, linux-watchdog, Anson.Huang, shawnguo, linux-imx, kernel,
	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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 4/5] watchdog: imx7ulp: Remove inline annotations
  2019-10-29 17:40 [PATCH 1/5] watchdog: imx7ulp: Fix reboot hang Fabio Estevam
  2019-10-29 17:40 ` [PATCH 2/5] watchdog: imx7ulp: Pass the wdog instance inimx7ulp_wdt_enable() Fabio Estevam
  2019-10-29 17:40 ` [PATCH 3/5] watchdog: imx7ulp: Remove unused structure member Fabio Estevam
@ 2019-10-29 17:40 ` Fabio Estevam
  2019-11-02 15:39   ` Guenter Roeck
  2019-10-29 17:40 ` [PATCH 5/5] watchdog: imx7ulp: Use definitions instead of magic values Fabio Estevam
  2019-11-02 15:36 ` [PATCH 1/5] watchdog: imx7ulp: Fix reboot hang Guenter Roeck
  4 siblings, 1 reply; 14+ messages in thread
From: Fabio Estevam @ 2019-10-29 17:40 UTC (permalink / raw)
  To: linux
  Cc: wim, linux-watchdog, Anson.Huang, shawnguo, linux-imx, kernel,
	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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 5/5] watchdog: imx7ulp: Use definitions instead of magic values
  2019-10-29 17:40 [PATCH 1/5] watchdog: imx7ulp: Fix reboot hang Fabio Estevam
                   ` (2 preceding siblings ...)
  2019-10-29 17:40 ` [PATCH 4/5] watchdog: imx7ulp: Remove inline annotations Fabio Estevam
@ 2019-10-29 17:40 ` Fabio Estevam
  2019-11-02 15:41   ` Guenter Roeck
  2019-11-02 15:36 ` [PATCH 1/5] watchdog: imx7ulp: Fix reboot hang Guenter Roeck
  4 siblings, 1 reply; 14+ messages in thread
From: Fabio Estevam @ 2019-10-29 17:40 UTC (permalink / raw)
  To: linux
  Cc: wim, linux-watchdog, Anson.Huang, shawnguo, linux-imx, kernel,
	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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/5] watchdog: imx7ulp: Fix reboot hang
  2019-10-29 17:40 [PATCH 1/5] watchdog: imx7ulp: Fix reboot hang Fabio Estevam
                   ` (3 preceding siblings ...)
  2019-10-29 17:40 ` [PATCH 5/5] watchdog: imx7ulp: Use definitions instead of magic values Fabio Estevam
@ 2019-11-02 15:36 ` Guenter Roeck
  2019-11-04 13:45   ` Fabio Estevam
  4 siblings, 1 reply; 14+ messages in thread
From: Guenter Roeck @ 2019-11-02 15:36 UTC (permalink / raw)
  To: Fabio Estevam
  Cc: wim, linux-watchdog, Anson.Huang, shawnguo, linux-imx, kernel

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/5] watchdog: imx7ulp: Pass the wdog instance inimx7ulp_wdt_enable()
  2019-10-29 17:40 ` [PATCH 2/5] watchdog: imx7ulp: Pass the wdog instance inimx7ulp_wdt_enable() Fabio Estevam
@ 2019-11-02 15:37   ` Guenter Roeck
  0 siblings, 0 replies; 14+ messages in thread
From: Guenter Roeck @ 2019-11-02 15:37 UTC (permalink / raw)
  To: Fabio Estevam
  Cc: wim, linux-watchdog, Anson.Huang, shawnguo, linux-imx, kernel

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)

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/5] watchdog: imx7ulp: Remove unused structure member
  2019-10-29 17:40 ` [PATCH 3/5] watchdog: imx7ulp: Remove unused structure member Fabio Estevam
@ 2019-11-02 15:39   ` Guenter Roeck
  0 siblings, 0 replies; 14+ messages in thread
From: Guenter Roeck @ 2019-11-02 15:39 UTC (permalink / raw)
  To: Fabio Estevam
  Cc: wim, linux-watchdog, Anson.Huang, shawnguo, linux-imx, kernel

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;

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 4/5] watchdog: imx7ulp: Remove inline annotations
  2019-10-29 17:40 ` [PATCH 4/5] watchdog: imx7ulp: Remove inline annotations Fabio Estevam
@ 2019-11-02 15:39   ` Guenter Roeck
  0 siblings, 0 replies; 14+ messages in thread
From: Guenter Roeck @ 2019-11-02 15:39 UTC (permalink / raw)
  To: Fabio Estevam
  Cc: wim, linux-watchdog, Anson.Huang, shawnguo, linux-imx, kernel

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 5/5] watchdog: imx7ulp: Use definitions instead of magic values
  2019-10-29 17:40 ` [PATCH 5/5] watchdog: imx7ulp: Use definitions instead of magic values Fabio Estevam
@ 2019-11-02 15:41   ` Guenter Roeck
  0 siblings, 0 replies; 14+ messages in thread
From: Guenter Roeck @ 2019-11-02 15:41 UTC (permalink / raw)
  To: Fabio Estevam
  Cc: wim, linux-watchdog, Anson.Huang, shawnguo, linux-imx, kernel

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);
>  }
>  

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/5] watchdog: imx7ulp: Fix reboot hang
  2019-11-02 15:36 ` [PATCH 1/5] watchdog: imx7ulp: Fix reboot hang Guenter Roeck
@ 2019-11-04 13:45   ` Fabio Estevam
  2019-11-04 14:06     ` Guenter Roeck
  0 siblings, 1 reply; 14+ messages in thread
From: Fabio Estevam @ 2019-11-04 13:45 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: wim, linux-watchdog, Yongcai Huang, Shawn Guo, NXP Linux Team,
	Sascha Hauer

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/5] watchdog: imx7ulp: Fix reboot hang
  2019-11-04 13:45   ` Fabio Estevam
@ 2019-11-04 14:06     ` Guenter Roeck
  2019-11-07 17:46       ` Fabio Estevam
  0 siblings, 1 reply; 14+ messages in thread
From: Guenter Roeck @ 2019-11-04 14:06 UTC (permalink / raw)
  To: Fabio Estevam
  Cc: wim, linux-watchdog, Yongcai Huang, Shawn Guo, NXP Linux Team,
	Sascha Hauer

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/5] watchdog: imx7ulp: Fix reboot hang
  2019-11-04 14:06     ` Guenter Roeck
@ 2019-11-07 17:46       ` Fabio Estevam
  2019-11-08 14:39         ` Guenter Roeck
  0 siblings, 1 reply; 14+ messages in thread
From: Fabio Estevam @ 2019-11-07 17:46 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: wim, linux-watchdog, Yongcai Huang, Shawn Guo, NXP Linux Team,
	Sascha Hauer

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/5] watchdog: imx7ulp: Fix reboot hang
  2019-11-07 17:46       ` Fabio Estevam
@ 2019-11-08 14:39         ` Guenter Roeck
  0 siblings, 0 replies; 14+ messages in thread
From: Guenter Roeck @ 2019-11-08 14:39 UTC (permalink / raw)
  To: Fabio Estevam
  Cc: wim, linux-watchdog, Yongcai Huang, Shawn Guo, NXP Linux Team,
	Sascha Hauer

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2019-11-08 14:39 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-29 17:40 [PATCH 1/5] watchdog: imx7ulp: Fix reboot hang Fabio Estevam
2019-10-29 17:40 ` [PATCH 2/5] watchdog: imx7ulp: Pass the wdog instance inimx7ulp_wdt_enable() Fabio Estevam
2019-11-02 15:37   ` Guenter Roeck
2019-10-29 17:40 ` [PATCH 3/5] watchdog: imx7ulp: Remove unused structure member Fabio Estevam
2019-11-02 15:39   ` Guenter Roeck
2019-10-29 17:40 ` [PATCH 4/5] watchdog: imx7ulp: Remove inline annotations Fabio Estevam
2019-11-02 15:39   ` Guenter Roeck
2019-10-29 17:40 ` [PATCH 5/5] watchdog: imx7ulp: Use definitions instead of magic values Fabio Estevam
2019-11-02 15:41   ` Guenter Roeck
2019-11-02 15:36 ` [PATCH 1/5] watchdog: imx7ulp: Fix reboot hang Guenter Roeck
2019-11-04 13:45   ` Fabio Estevam
2019-11-04 14:06     ` Guenter Roeck
2019-11-07 17:46       ` Fabio Estevam
2019-11-08 14:39         ` Guenter Roeck

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).