| Message ID | 1411108199-1280-2-git-send-email-jszhang@marvell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Sep 19, 2014 at 02:29:58PM +0800, Jisheng Zhang wrote: > The TOP_INIT may be zero, so the timeout period may be very short after What is TOP_INIT ? > initialization is done, thus the system may be reset soon after enabling. > We fix this problem by restarting the counter immediately after enabling > WDT. > > Signed-off-by: Jisheng Zhang <jszhang@marvell.com> > --- > drivers/watchdog/dw_wdt.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c > index 9f21029..ad0619d 100644 > --- a/drivers/watchdog/dw_wdt.c > +++ b/drivers/watchdog/dw_wdt.c > @@ -146,6 +146,7 @@ static int dw_wdt_open(struct inode *inode, struct file *filp) > dw_wdt_set_top(DW_WDT_MAX_TOP); This sets the timeout to the maximum, so I guess there must be some other event/register (TOP_INIT ?) which can be zero. > writel(WDOG_CONTROL_REG_WDT_EN_MASK, > dw_wdt.regs + WDOG_CONTROL_REG_OFFSET); What guarantees that the reset didn't already happen by the time the keepalive call is executed ? Does this change fix the problem, or does it just make it more unlikely to be seen ? Thanks, Guenter > + dw_wdt_keepalive(); > } > > dw_wdt_set_next_heartbeat(); > -- > 2.1.0 >
On 09/18/2014 11:29 PM, Jisheng Zhang wrote: > The TOP_INIT may be zero, so the timeout period may be very short after > initialization is done, thus the system may be reset soon after enabling. > We fix this problem by restarting the counter immediately after enabling > WDT. > > Signed-off-by: Jisheng Zhang <jszhang@marvell.com> > --- > drivers/watchdog/dw_wdt.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c > index 9f21029..ad0619d 100644 > --- a/drivers/watchdog/dw_wdt.c > +++ b/drivers/watchdog/dw_wdt.c > @@ -146,6 +146,7 @@ static int dw_wdt_open(struct inode *inode, struct file *filp) > dw_wdt_set_top(DW_WDT_MAX_TOP); > writel(WDOG_CONTROL_REG_WDT_EN_MASK, > dw_wdt.regs + WDOG_CONTROL_REG_OFFSET); > + dw_wdt_keepalive(); > } > > dw_wdt_set_next_heartbeat(); > After getting access to the datasheet, I concluded that this fix is wrong or at least more risky than necessary. The datasheet states that top_init, ie bit 4-7 of the wdt_torr register, needs to be initialized with the desired timeout period prior to enabling the watchdog. dw_wdt_set_top() sets it to 0 instead, ie to the lowest possible timeout period. Guenter
Dear Guenter, On Sat, 20 Sep 2014 06:51:56 -0700 Guenter Roeck <linux@roeck-us.net> wrote: > On 09/18/2014 11:29 PM, Jisheng Zhang wrote: > > The TOP_INIT may be zero, so the timeout period may be very short after > > initialization is done, thus the system may be reset soon after enabling. > > We fix this problem by restarting the counter immediately after enabling > > WDT. > > > > Signed-off-by: Jisheng Zhang <jszhang@marvell.com> > > --- > > drivers/watchdog/dw_wdt.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c > > index 9f21029..ad0619d 100644 > > --- a/drivers/watchdog/dw_wdt.c > > +++ b/drivers/watchdog/dw_wdt.c > > @@ -146,6 +146,7 @@ static int dw_wdt_open(struct inode *inode, struct > > file *filp) dw_wdt_set_top(DW_WDT_MAX_TOP); > > writel(WDOG_CONTROL_REG_WDT_EN_MASK, > > dw_wdt.regs + WDOG_CONTROL_REG_OFFSET); > > + dw_wdt_keepalive(); > > } > > > > dw_wdt_set_next_heartbeat(); > > > After getting access to the datasheet, I concluded that this fix is wrong > or at least more risky than necessary. The datasheet states that top_init, > ie bit 4-7 of the wdt_torr register, needs to be initialized with the > desired timeout period prior to enabling the watchdog. dw_wdt_set_top() > sets it to 0 instead, ie to the lowest possible timeout period. > In patch V2, I initialize the TOP_INIT when setting TOP in function dw_wdt_set_top(), this also can fix the "reboot soon" problem. Could you please have a review again? Thanks, Jisheng
diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c index 9f21029..ad0619d 100644 --- a/drivers/watchdog/dw_wdt.c +++ b/drivers/watchdog/dw_wdt.c @@ -146,6 +146,7 @@ static int dw_wdt_open(struct inode *inode, struct file *filp) dw_wdt_set_top(DW_WDT_MAX_TOP); writel(WDOG_CONTROL_REG_WDT_EN_MASK, dw_wdt.regs + WDOG_CONTROL_REG_OFFSET); + dw_wdt_keepalive(); } dw_wdt_set_next_heartbeat();
The TOP_INIT may be zero, so the timeout period may be very short after initialization is done, thus the system may be reset soon after enabling. We fix this problem by restarting the counter immediately after enabling WDT. Signed-off-by: Jisheng Zhang <jszhang@marvell.com> --- drivers/watchdog/dw_wdt.c | 1 + 1 file changed, 1 insertion(+)