| Message ID | 20240513101108.5237-4-shentey@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Trivial ARM changes | expand |
On 13/5/24 12:11, Bernhard Beschow wrote: > The same statement is executed unconditionally right before the if statement. > > Cc: Guenter Roeck <linux@roeck-us.net> > Signed-off-by: Bernhard Beschow <shentey@gmail.com> > > --- > > The duplicate line may indicate a bug. I'm not familiar with the code, so this > patch may go into the wrong direction. Please check! The bug might be in imx2_wdt_write() where the WRSR register is overwritten, shouldn't we OR the SFTW bit, keeping other (such TOUT) set? if (!(value & IMX2_WDT_WCR_SRS)) { s->wrsr = IMX2_WDT_WRSR_SFTW; } > --- > hw/watchdog/wdt_imx2.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/hw/watchdog/wdt_imx2.c b/hw/watchdog/wdt_imx2.c > index 6452fc4721..f9a7ea287f 100644 > --- a/hw/watchdog/wdt_imx2.c > +++ b/hw/watchdog/wdt_imx2.c > @@ -39,7 +39,6 @@ static void imx2_wdt_expired(void *opaque) > > /* Perform watchdog action if watchdog is enabled */ > if (s->wcr & IMX2_WDT_WCR_WDE) { > - s->wrsr = IMX2_WDT_WRSR_TOUT; > watchdog_perform_action(); > } > }
On 5/13/24 03:11, Bernhard Beschow wrote: > The same statement is executed unconditionally right before the if statement. > > Cc: Guenter Roeck <linux@roeck-us.net> > Signed-off-by: Bernhard Beschow <shentey@gmail.com> > > --- > > The duplicate line may indicate a bug. I'm not familiar with the code, so this > patch may go into the wrong direction. Please check! Should be ok. Technically the function should not be called to start with if the watchdog isn't running. If it is, it might be useful to trace the content of wcr and try to determine why the timer isn't stopped if / when the watchdog is disabled. Reviewed-by: Guenter Roeck <linux@roeck-us.net> Thanks, Guenter > --- > hw/watchdog/wdt_imx2.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/hw/watchdog/wdt_imx2.c b/hw/watchdog/wdt_imx2.c > index 6452fc4721..f9a7ea287f 100644 > --- a/hw/watchdog/wdt_imx2.c > +++ b/hw/watchdog/wdt_imx2.c > @@ -39,7 +39,6 @@ static void imx2_wdt_expired(void *opaque) > > /* Perform watchdog action if watchdog is enabled */ > if (s->wcr & IMX2_WDT_WCR_WDE) { > - s->wrsr = IMX2_WDT_WRSR_TOUT; > watchdog_perform_action(); > } > }
diff --git a/hw/watchdog/wdt_imx2.c b/hw/watchdog/wdt_imx2.c index 6452fc4721..f9a7ea287f 100644 --- a/hw/watchdog/wdt_imx2.c +++ b/hw/watchdog/wdt_imx2.c @@ -39,7 +39,6 @@ static void imx2_wdt_expired(void *opaque) /* Perform watchdog action if watchdog is enabled */ if (s->wcr & IMX2_WDT_WCR_WDE) { - s->wrsr = IMX2_WDT_WRSR_TOUT; watchdog_perform_action(); } }
The same statement is executed unconditionally right before the if statement. Cc: Guenter Roeck <linux@roeck-us.net> Signed-off-by: Bernhard Beschow <shentey@gmail.com> --- The duplicate line may indicate a bug. I'm not familiar with the code, so this patch may go into the wrong direction. Please check! --- hw/watchdog/wdt_imx2.c | 1 - 1 file changed, 1 deletion(-)