Skip to contentSkip to conversation

Watchdog: OMAP3: fix wrong boot status from wdt reboot

PATCH New Zumeng Chen 4 participants 6 messages Jul 4, 2012 – Jul 5, 2012

Subsystems (4)
Arm (32-bit) Arm sub-architectures OMAP2+ support Watchdog device drivers
lore
Share & tools
lore

Zumeng Chen

author
#0 lore raw

PATCH 1/1 Watchdog: OMAP3: fix wrong boot status from wdt reboot

Does the following fix make sense?

WDIOC_GETBOOTSTATUS always return 0 even if the machine comes from omap-wdt reboot. Because WKUP_MOD is not right for OMAP3, so give the right addr 0xA00 of PRM_RSTST for get_reset_sources, which inputs the signal from omap-wdt reboot, and return 1 when coming from omap-wdt reboot for WDIOC_GETBOOTSTATUS.

Signed-off-by Zumeng Chen <[email protected]>
---
 arch/arm/mach-omap2/prcm.c  |    4 +++-
 drivers/watchdog/omap_wdt.c |    3 +++
 drivers/watchdog/omap_wdt.h |    3 +++
 3 files changed, 9 insertions(+), 1 deletions(-)
arch/arm/mach-omap2/prcm.c +3 -1
index 480f40a..43f3feb 100644
@@ -49,8 +49,10 @@ void __iomem *prcm_mpu_base;
 u32 omap_prcm_get_reset_sources(void)
 {
 	/* XXX This presumably needs modification for 34XX */
-	if (cpu_is_omap24xx() || cpu_is_omap34xx())
+	if (cpu_is_omap24xx())
 		return omap2_prm_read_mod_reg(WKUP_MOD, OMAP2_RM_RSTST) & 0x7f;
+	if (cpu_is_omap34xx())
+		return omap2_prm_read_mod_reg(0xA00, OMAP2_RM_RSTST) & 0x7f;
 	if (cpu_is_omap44xx())
 		return omap2_prm_read_mod_reg(WKUP_MOD, OMAP4_RM_RSTST) & 0x7f;
drivers/watchdog/omap_wdt.c +3 -0
index 8285d65..ea57078 100644
@@ -234,6 +234,9 @@ static long omap_wdt_ioctl(struct file *file, unsigned int cmd,
 		if (cpu_is_omap24xx())
 			return put_user(omap_prcm_get_reset_sources(),
 					(int __user *)arg);
+		if (cpu_is_omap34xx())
+			return put_user(omap_prcm_get_reset_sources() & 0x10 >>
+				OMAP3_PRM_RSTST_BIT, (int __user *)arg);
 		return put_user(0, (int __user *)arg);
 	case WDIOC_KEEPALIVE:
 		pm_runtime_get_sync(wdev->dev);
drivers/watchdog/omap_wdt.h +3 -0
index 09b774c..d8d5daa 100644
@@ -40,6 +40,9 @@
 #define OMAP_WATCHDOG_WPS		(0x34)
 #define OMAP_WATCHDOG_SPR		(0x48)
 
+/* PRM_RSTST MPU_WD_RST bit */
+#define OMAP3_PRM_RSTST_BIT		4
+
 /* Using the prescaler, the OMAP watchdog could go for many
  * months before firing.  These limits work without scaling,
  * with the 60 second default assumed by most tools and docs.
Signature · 1 line
1.7.5.4
#1 lore raw

PATCH 1/1 Watchdog: OMAP3: fix wrong boot status from wdt reboot

To: Shubhrajyoti

Quoted 60 lines · Zumeng Chen

Does the following fix make sense?

WDIOC_GETBOOTSTATUS always return 0 even if the machine comes from omap-wdt reboot. Because WKUP_MOD is not right for OMAP3, so give the right addr 0xA00 of PRM_RSTST for get_reset_sources, which inputs the signal from omap-wdt reboot, and return 1 when coming from omap-wdt reboot for WDIOC_GETBOOTSTATUS.

Signed-off-by Zumeng Chen<[email protected]>
---
  arch/arm/mach-omap2/prcm.c  |    4 +++-
  drivers/watchdog/omap_wdt.c |    3 +++
  drivers/watchdog/omap_wdt.h |    3 +++
  3 files changed, 9 insertions(+), 1 deletions(-)
arch/arm/mach-omap2/prcm.c +3 -1
index 480f40a..43f3feb 100644
@@ -49,8 +49,10 @@ void __iomem *prcm_mpu_base;
  u32 omap_prcm_get_reset_sources(void)
  {
  	/* XXX This presumably needs modification for 34XX */
-	if (cpu_is_omap24xx() || cpu_is_omap34xx())
+	if (cpu_is_omap24xx())
  		return omap2_prm_read_mod_reg(WKUP_MOD, OMAP2_RM_RSTST)&  0x7f;
+	if (cpu_is_omap34xx())
+		return omap2_prm_read_mod_reg(0xA00, OMAP2_RM_RSTST)&  0x7f;
  	if (cpu_is_omap44xx())
  		return omap2_prm_read_mod_reg(WKUP_MOD, OMAP4_RM_RSTST)&  0x7f;
drivers/watchdog/omap_wdt.c +3 -0
index 8285d65..ea57078 100644
@@ -234,6 +234,9 @@ static long omap_wdt_ioctl(struct file *file, unsigned int cmd,
  		if (cpu_is_omap24xx())
  			return put_user(omap_prcm_get_reset_sources(),
  					(int __user *)arg);
+		if (cpu_is_omap34xx())
+			return put_user(omap_prcm_get_reset_sources()&  0x10>>
+				OMAP3_PRM_RSTST_BIT, (int __user *)arg);
  		return put_user(0, (int __user *)arg);
  	case WDIOC_KEEPALIVE:
  		pm_runtime_get_sync(wdev->dev);
drivers/watchdog/omap_wdt.h +3 -0
index 09b774c..d8d5daa 100644
@@ -40,6 +40,9 @@
  #define OMAP_WATCHDOG_WPS		(0x34)
  #define OMAP_WATCHDOG_SPR		(0x48)
 
+/* PRM_RSTST MPU_WD_RST bit */
+#define OMAP3_PRM_RSTST_BIT		4
+
  /* Using the prescaler, the OMAP watchdog could go for many
   * months before firing.  These limits work without scaling,
   * with the 60 second default assumed by most tools and docs.
#2 lore raw

PATCH 1/1 Watchdog: OMAP3: fix wrong boot status from wdt reboot

Hello,

Quoted 3 lines · zumeng.chen

To: Shubhrajyoti

Quoted 1 line · Zumeng Chen

Does the following fix make sense?

yes , thanks for the patch.
IIRC Rajendra had a similar one.
Some comments below.

Quoted 33 lines
Quoted 33 lines

WDIOC_GETBOOTSTATUS always return 0 even if the machine comes from omap-wdt reboot. Because WKUP_MOD is not right for OMAP3,so give the right addr 0xA00 of PRM_RSTST for get_reset_sources, which inputs the signal from omap-wdt reboot, and return 1 when coming from omap-wdt reboot for WDIOC_GETBOOTSTATUS.

Signed-off-by Zumeng Chen<[email protected]>
---
  arch/arm/mach-omap2/prcm.c  |    4 +++-
  drivers/watchdog/omap_wdt.c |    3 +++
  drivers/watchdog/omap_wdt.h |    3 +++
  3 files changed, 9 insertions(+), 1 deletions(-)
arch/arm/mach-omap2/prcm.c +3 -1
index 480f40a..43f3feb 100644
@@ -49,8 +49,10 @@ void __iomem *prcm_mpu_base;
  u32 omap_prcm_get_reset_sources(void)
  {
      /* XXX This presumably needs modification for 34XX */
-    if (cpu_is_omap24xx() || cpu_is_omap34xx())
+    if (cpu_is_omap24xx())
          return omap2_prm_read_mod_reg(WKUP_MOD, OMAP2_RM_RSTST)& 
 0x7f;
+    if (cpu_is_omap34xx())
+        return omap2_prm_read_mod_reg(0xA00, OMAP2_RM_RSTST)&  0x7f;
      if (cpu_is_omap44xx())
          return omap2_prm_read_mod_reg(WKUP_MOD, OMAP4_RM_RSTST)&

0x7f;

Quoted 12 lines

drivers/watchdog/omap_wdt.c static long omap_wdt_ioctl(struct file *file, · line 240

Quoted 12 lines
drivers/watchdog/omap_wdt.c +3 -0
index 8285d65..ea57078 100644
@@ -234,6 +234,9 @@ static long omap_wdt_ioctl(struct file *file,
 unsigned int cmd,
          if (cpu_is_omap24xx())
              return put_user(omap_prcm_get_reset_sources(),
                      (int __user *)arg);
+        if (cpu_is_omap34xx())
+            return put_user(omap_prcm_get_reset_sources()&  0x10>>
+                OMAP3_PRM_RSTST_BIT, (int __user *)arg);

Actually instead of returning yes/no.
The correct expectation is to return WDIOF_* flags as defined in include/linux/watchdog.h.

(BTW I agree that was something even current code is not following).Since you are at it may be that?s something you can consider.

Quoted 18 lines
Quoted 17 lines
drivers/watchdog/omap_wdt.c +0 -0
          return put_user(0, (int __user *)arg);
      case WDIOC_KEEPALIVE:

          pm_runtime_get_sync(wdev->dev);
drivers/watchdog/omap_wdt.h +3 -0
index 09b774c..d8d5daa 100644
@@ -40,6 +40,9 @@
  #define OMAP_WATCHDOG_WPS        (0x34)
  #define OMAP_WATCHDOG_SPR        (0x48)
 
+/* PRM_RSTST MPU_WD_RST bit */
+#define OMAP3_PRM_RSTST_BIT        4
+
  /* Using the prescaler, the OMAP watchdog could go for many
   * months before firing.  These limits work without scaling,
   * with the 60 second default assumed by most tools and docs.
author
#3 lore raw

PATCH 1/1 Watchdog: OMAP3: fix wrong boot status from wdt reboot

? 2012?07?05? 13:39, Shubhrajyoti ??:

Quoted 40 lines

Hello,

Quoted 3 lines · zumeng.chen

To: Shubhrajyoti

Quoted 1 line · Zumeng Chen

Does the following fix make sense?

yes , thanks for the patch.
IIRC Rajendra had a similar one.
Some comments below.

Quoted 32 lines
Quoted 32 lines

WDIOC_GETBOOTSTATUS always return 0 even if the machine comes from omap-wdt reboot. Because WKUP_MOD is not right for OMAP3,so give the right addr 0xA00 of PRM_RSTST for get_reset_sources, which inputs the signal from omap-wdt reboot, and return 1 when coming from omap-wdt reboot for WDIOC_GETBOOTSTATUS.

Signed-off-by Zumeng Chen<[email protected]>
---
   arch/arm/mach-omap2/prcm.c  |    4 +++-
   drivers/watchdog/omap_wdt.c |    3 +++
   drivers/watchdog/omap_wdt.h |    3 +++
   3 files changed, 9 insertions(+), 1 deletions(-)
arch/arm/mach-omap2/prcm.c +3 -1
index 480f40a..43f3feb 100644
@@ -49,8 +49,10 @@ void __iomem *prcm_mpu_base;
   u32 omap_prcm_get_reset_sources(void)
   {
       /* XXX This presumably needs modification for 34XX */
-    if (cpu_is_omap24xx() || cpu_is_omap34xx())
+    if (cpu_is_omap24xx())
           return omap2_prm_read_mod_reg(WKUP_MOD, OMAP2_RM_RSTST)&
 0x7f;
+    if (cpu_is_omap34xx())
+        return omap2_prm_read_mod_reg(0xA00, OMAP2_RM_RSTST)&   0x7f;
       if (cpu_is_omap44xx())
           return omap2_prm_read_mod_reg(WKUP_MOD, OMAP4_RM_RSTST)&

0x7f;

Quoted 15 lines
Quoted 4 lines
Quoted 4 lines
drivers/watchdog/omap_wdt.c +0 -0
index 8285d65..ea57078 100644
Quoted 8 lines

drivers/watchdog/omap_wdt.c static long omap_wdt_ioctl(struct file *file, · line 240

Quoted 8 lines
drivers/watchdog/omap_wdt.c +3 -0
@@ -234,6 +234,9 @@ static long omap_wdt_ioctl(struct file *file,
 unsigned int cmd,
           if (cpu_is_omap24xx())
               return put_user(omap_prcm_get_reset_sources(),
                       (int __user *)arg);
+        if (cpu_is_omap34xx())
+            return put_user(omap_prcm_get_reset_sources()&   0x10>>
+                OMAP3_PRM_RSTST_BIT, (int __user *)arg);

Actually instead of returning yes/no.
The correct expectation is to return WDIOF_* flags as defined in include/linux/watchdog.h.

Yes, return WDIOF_CARDRESET is more general, thanks.

Quoted 4 lines

(BTW I agree that was something even current code is not following).Since you are at it may be that?s something you can consider.

Yes, I just find them, I'll try it to get it more following, like get_status.

Regards,
Zumeng

Quoted 18 lines
Quoted 18 lines
Quoted 18 lines
drivers/watchdog/omap_wdt.c +0 -0
           return put_user(0, (int __user *)arg);
       case WDIOC_KEEPALIVE:

           pm_runtime_get_sync(wdev->dev);
drivers/watchdog/omap_wdt.h +3 -0
index 09b774c..d8d5daa 100644
@@ -40,6 +40,9 @@
   #define OMAP_WATCHDOG_WPS        (0x34)
   #define OMAP_WATCHDOG_SPR        (0x48)
 
+/* PRM_RSTST MPU_WD_RST bit */
+#define OMAP3_PRM_RSTST_BIT        4
+
   /* Using the prescaler, the OMAP watchdog could go for many
    * months before firing.  These limits work without scaling,
    * with the 60 second default assumed by most tools and docs.
#4 lore raw

PATCH 1/1 Watchdog: OMAP3: fix wrong boot status from wdt reboot

In reply to Zumeng Chen

Hello,

Quoted 21 lines · Zumeng Chen

Does the following fix make sense?

WDIOC_GETBOOTSTATUS always return 0 even if the machine comes from omap-wdt reboot. Because WKUP_MOD is not right for OMAP3, so give the right addr 0xA00 of PRM_RSTST for get_reset_sources, which inputs the signal from omap-wdt reboot, and return 1 when coming from omap-wdt reboot for WDIOC_GETBOOTSTATUS.

Signed-off-by Zumeng Chen <[email protected]>
---
 arch/arm/mach-omap2/prcm.c  |    4 +++-
 drivers/watchdog/omap_wdt.c |    3 +++
 drivers/watchdog/omap_wdt.h |    3 +++
 3 files changed, 9 insertions(+), 1 deletions(-)
arch/arm/mach-omap2/prcm.c +0 -0
index 480f40a..43f3feb 100644

arch/arm/mach-omap2/prcm.c void __iomem *prcm_mpu_base; · line 57

Quoted 10 lines
arch/arm/mach-omap2/prcm.c +3 -1
@@ -49,8 +49,10 @@ void __iomem *prcm_mpu_base;
 u32 omap_prcm_get_reset_sources(void)
 {
 	/* XXX This presumably needs modification for 34XX */
-	if (cpu_is_omap24xx() || cpu_is_omap34xx())
+	if (cpu_is_omap24xx())
 		return omap2_prm_read_mod_reg(WKUP_MOD, OMAP2_RM_RSTST) & 0x7f;
+	if (cpu_is_omap34xx())
+		return omap2_prm_read_mod_reg(0xA00, OMAP2_RM_RSTST) & 0x7f;
 	if (cpu_is_omap44xx())
 		return omap2_prm_read_mod_reg(WKUP_MOD, OMAP4_RM_RSTST) & 0x7f;

Instead of adding more cpu_is_* checks maybe you could switch to a function pointers based approach here?

drivers/watchdog/omap_wdt.c static long omap_wdt_ioctl(struct file *file, unsigned int cmd, · line 240

Quoted 13 lines
drivers/watchdog/omap_wdt.c +3 -0
index 8285d65..ea57078 100644
@@ -234,6 +234,9 @@ static long omap_wdt_ioctl(struct file *file, unsigned int cmd,
 		if (cpu_is_omap24xx())
 			return put_user(omap_prcm_get_reset_sources(),
 					(int __user *)arg);
+		if (cpu_is_omap34xx())
+			return put_user(omap_prcm_get_reset_sources() & 0x10 >>
+				OMAP3_PRM_RSTST_BIT, (int __user *)arg);
 		return put_user(0, (int __user *)arg);

Usage of PRCM bit masks in the driver looks wrong. Why not introduce an API like omap_prcm_check_reset_reason() which returns true or false based on the reset reason being checked?

In case of WDT, the driver can then pass the right flag to userspace.

Regards,
Vaibhav B.

Quoted 25 lines
drivers/watchdog/omap_wdt.c +0 -0
 	case WDIOC_KEEPALIVE:
 		pm_runtime_get_sync(wdev->dev);
drivers/watchdog/omap_wdt.h +3 -0
index 09b774c..d8d5daa 100644
@@ -40,6 +40,9 @@
 #define OMAP_WATCHDOG_WPS		(0x34)
 #define OMAP_WATCHDOG_SPR		(0x48)
 
+/* PRM_RSTST MPU_WD_RST bit */
+#define OMAP3_PRM_RSTST_BIT		4
+
 /* Using the prescaler, the OMAP watchdog could go for many
  * months before firing.  These limits work without scaling,
  * with the 60 second default assumed by most tools and docs.
Signature · 7 lines
1.7.5.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
#5 lore raw

PATCH 1/1 Watchdog: OMAP3: fix wrong boot status from wdt reboot

Quoted 28 lines · Vaibhav

Hello,

Quoted 25 lines · Zumeng Chen

Does the following fix make sense?

WDIOC_GETBOOTSTATUS always return 0 even if the machine comes from omap-wdt reboot. Because WKUP_MOD is not right for OMAP3, so give the right addr 0xA00 of PRM_RSTST for get_reset_sources, which inputs the signal from omap-wdt reboot, and return 1 when coming from omap-wdt reboot for WDIOC_GETBOOTSTATUS.

Signed-off-by Zumeng Chen<[email protected]>
---
  arch/arm/mach-omap2/prcm.c  |    4 +++-
  drivers/watchdog/omap_wdt.c |    3 +++
  drivers/watchdog/omap_wdt.h |    3 +++
  3 files changed, 9 insertions(+), 1 deletions(-)
arch/arm/mach-omap2/prcm.c +0 -1
index 480f40a..43f3feb 100644
@@ -49,8 +49,10 @@ void __iomem *prcm_mpu_base;
  u32 omap_prcm_get_reset_sources(void)
  {
  	/* XXX This presumably needs modification for 34XX */
-	if (cpu_is_omap24xx() || cpu_is_omap34xx())
Quoted 8 lines

arch/arm/mach-omap2/prcm.c void __iomem *prcm_mpu_base;

Quoted 6 lines
arch/arm/mach-omap2/prcm.c +3 -0
+	if (cpu_is_omap24xx())
  		return omap2_prm_read_mod_reg(WKUP_MOD, OMAP2_RM_RSTST)&  0x7f;
+	if (cpu_is_omap34xx())
+		return omap2_prm_read_mod_reg(0xA00, OMAP2_RM_RSTST)&  0x7f;
  	if (cpu_is_omap44xx())
  		return omap2_prm_read_mod_reg(WKUP_MOD, OMAP4_RM_RSTST)&  0x7f;

Instead of adding more cpu_is_* checks maybe you could switch to a function pointers based approach here?

I don't see any more checks VS before like ( cpu_is_omap24xx() || cpu_is_omap34xx())

Actually what we want is just to read a register with different offset responding to the different SOC.

Quoted 14 lines

drivers/watchdog/omap_wdt.c static long omap_wdt_ioctl(struct file *file, unsigned int cmd, · line 240

Quoted 13 lines
drivers/watchdog/omap_wdt.c +3 -0
index 8285d65..ea57078 100644
@@ -234,6 +234,9 @@ static long omap_wdt_ioctl(struct file *file, unsigned int cmd,
  		if (cpu_is_omap24xx())
  			return put_user(omap_prcm_get_reset_sources(),
  					(int __user *)arg);
+		if (cpu_is_omap34xx())
+			return put_user(omap_prcm_get_reset_sources()&  0x10>>
+				OMAP3_PRM_RSTST_BIT, (int __user *)arg);
  		return put_user(0, (int __user *)arg);

Usage of PRCM bit masks in the driver looks wrong. Why not

Maybe the not proper definition causes "looks wrong". It should be MPU_WD_RST_BIT, so you see, it is about watchdog bit.

Anyway, I'll try to send V2 patch with Hubhrajyoti's and your comments

Regards,
Zumeng

Quoted 34 lines

introduce an API like omap_prcm_check_reset_reason() which returns true or false based on the reset reason being checked?

In case of WDT, the driver can then pass the right flag to userspace.

Regards,
Vaibhav B.

Quoted 25 lines
drivers/watchdog/omap_wdt.c +0 -0
  	case WDIOC_KEEPALIVE:
  		pm_runtime_get_sync(wdev->dev);
drivers/watchdog/omap_wdt.h +3 -0
index 09b774c..d8d5daa 100644
@@ -40,6 +40,9 @@
  #define OMAP_WATCHDOG_WPS		(0x34)
  #define OMAP_WATCHDOG_SPR		(0x48)
 
+/* PRM_RSTST MPU_WD_RST bit */
+#define OMAP3_PRM_RSTST_BIT		4
+
  /* Using the prescaler, the OMAP watchdog could go for many
   * months before firing.  These limits work without scaling,
   * with the 60 second default assumed by most tools and docs.
Signature · 7 lines
1.7.5.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel