| Message ID | 1341417241-11563-1-git-send-email-zumeng.chen@windriver.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
To: Shubhrajyoti On 2012?07?04? 23:54, Zumeng Chen wrote: > 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<zumeng.chen@windriver.com> > --- > 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(-) > > diff --git a/arch/arm/mach-omap2/prcm.c b/arch/arm/mach-omap2/prcm.c > index 480f40a..43f3feb 100644 > --- a/arch/arm/mach-omap2/prcm.c > +++ b/arch/arm/mach-omap2/prcm.c > @@ -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; > > diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c > index 8285d65..ea57078 100644 > --- a/drivers/watchdog/omap_wdt.c > +++ b/drivers/watchdog/omap_wdt.c > @@ -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); > diff --git a/drivers/watchdog/omap_wdt.h b/drivers/watchdog/omap_wdt.h > index 09b774c..d8d5daa 100644 > --- a/drivers/watchdog/omap_wdt.h > +++ b/drivers/watchdog/omap_wdt.h > @@ -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. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Wednesday 04 July 2012 09:27 PM, zumeng.chen wrote: > To: Shubhrajyoti > On 2012?07?04? 23:54, Zumeng Chen wrote: >> Does the following fix make sense? yes , thanks for the patch. IIRC Rajendra had a similar one. Some comments below. >> >> 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<zumeng.chen@windriver.com> >> --- >> 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(-) >> >> diff --git a/arch/arm/mach-omap2/prcm.c b/arch/arm/mach-omap2/prcm.c >> index 480f40a..43f3feb 100644 >> --- a/arch/arm/mach-omap2/prcm.c >> +++ b/arch/arm/mach-omap2/prcm.c >> @@ -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; >> >> diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c >> index 8285d65..ea57078 100644 >> --- a/drivers/watchdog/omap_wdt.c >> +++ b/drivers/watchdog/omap_wdt.c >> @@ -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. >> return put_user(0, (int __user *)arg); >> case WDIOC_KEEPALIVE: >> pm_runtime_get_sync(wdev->dev); >> diff --git a/drivers/watchdog/omap_wdt.h b/drivers/watchdog/omap_wdt.h >> index 09b774c..d8d5daa 100644 >> --- a/drivers/watchdog/omap_wdt.h >> +++ b/drivers/watchdog/omap_wdt.h >> @@ -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. > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
? 2012?07?05? 13:39, Shubhrajyoti ??: > Hello, > On Wednesday 04 July 2012 09:27 PM, zumeng.chen wrote: >> To: Shubhrajyoti >> On 2012?07?04? 23:54, Zumeng Chen wrote: >>> Does the following fix make sense? > yes , thanks for the patch. > IIRC Rajendra had a similar one. > Some comments below. >>> 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<zumeng.chen@windriver.com> >>> --- >>> 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(-) >>> >>> diff --git a/arch/arm/mach-omap2/prcm.c b/arch/arm/mach-omap2/prcm.c >>> index 480f40a..43f3feb 100644 >>> --- a/arch/arm/mach-omap2/prcm.c >>> +++ b/arch/arm/mach-omap2/prcm.c >>> @@ -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; >>> >>> diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c >>> index 8285d65..ea57078 100644 >>> --- a/drivers/watchdog/omap_wdt.c >>> +++ b/drivers/watchdog/omap_wdt.c >>> @@ -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. > > (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 > >>> return put_user(0, (int __user *)arg); >>> case WDIOC_KEEPALIVE: >>> pm_runtime_get_sync(wdev->dev); >>> diff --git a/drivers/watchdog/omap_wdt.h b/drivers/watchdog/omap_wdt.h >>> index 09b774c..d8d5daa 100644 >>> --- a/drivers/watchdog/omap_wdt.h >>> +++ b/drivers/watchdog/omap_wdt.h >>> @@ -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. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Wed, Jul 04, 2012 at 21:24:01, Zumeng Chen wrote: > 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 <zumeng.chen@windriver.com> > --- > 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(-) > > diff --git a/arch/arm/mach-omap2/prcm.c b/arch/arm/mach-omap2/prcm.c > index 480f40a..43f3feb 100644 > --- a/arch/arm/mach-omap2/prcm.c > +++ b/arch/arm/mach-omap2/prcm.c > @@ -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? > > diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c > index 8285d65..ea57078 100644 > --- a/drivers/watchdog/omap_wdt.c > +++ b/drivers/watchdog/omap_wdt.c > @@ -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. > case WDIOC_KEEPALIVE: > pm_runtime_get_sync(wdev->dev); > diff --git a/drivers/watchdog/omap_wdt.h b/drivers/watchdog/omap_wdt.h > index 09b774c..d8d5daa 100644 > --- a/drivers/watchdog/omap_wdt.h > +++ b/drivers/watchdog/omap_wdt.h > @@ -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. > -- > 1.7.5.4 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2012?07?05? 21:05, Bedia, Vaibhav wrote: > Hello, > > On Wed, Jul 04, 2012 at 21:24:01, Zumeng Chen wrote: >> 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<zumeng.chen@windriver.com> >> --- >> 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(-) >> >> diff --git a/arch/arm/mach-omap2/prcm.c b/arch/arm/mach-omap2/prcm.c >> index 480f40a..43f3feb 100644 >> --- a/arch/arm/mach-omap2/prcm.c >> +++ b/arch/arm/mach-omap2/prcm.c >> @@ -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? 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. >> >> diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c >> index 8285d65..ea57078 100644 >> --- a/drivers/watchdog/omap_wdt.c >> +++ b/drivers/watchdog/omap_wdt.c >> @@ -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 > 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. > >> case WDIOC_KEEPALIVE: >> pm_runtime_get_sync(wdev->dev); >> diff --git a/drivers/watchdog/omap_wdt.h b/drivers/watchdog/omap_wdt.h >> index 09b774c..d8d5daa 100644 >> --- a/drivers/watchdog/omap_wdt.h >> +++ b/drivers/watchdog/omap_wdt.h >> @@ -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. >> -- >> 1.7.5.4 >> >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >> -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/mach-omap2/prcm.c b/arch/arm/mach-omap2/prcm.c index 480f40a..43f3feb 100644 --- a/arch/arm/mach-omap2/prcm.c +++ b/arch/arm/mach-omap2/prcm.c @@ -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; diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c index 8285d65..ea57078 100644 --- a/drivers/watchdog/omap_wdt.c +++ b/drivers/watchdog/omap_wdt.c @@ -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); diff --git a/drivers/watchdog/omap_wdt.h b/drivers/watchdog/omap_wdt.h index 09b774c..d8d5daa 100644 --- a/drivers/watchdog/omap_wdt.h +++ b/drivers/watchdog/omap_wdt.h @@ -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.
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 <zumeng.chen@windriver.com> --- 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(-)