| Message ID | 20250513094711.2691059-2-sw617.shin@samsung.com |
|---|---|
| State | New |
| Headers | show |
| Series | Increase max timeout value of s3c2410 watchdog | expand |
On 5/13/25 02:47, Sangwook Shin wrote: > Increase max_timeout value from 55s to 3665038s (1018h 3min 58s) with > 38400000 frequency system if the system has 32-bit WTCNT register. > > cat /sys/devices/platform/10060000.watchdog_cl0/watchdog/watchdog0/max_timeout > 3665038 > > [ 0.208763][ T1] s3c2410-wdt 10060000.watchdog_cl0: Heartbeat: count=1099511400000, timeout=3665038, freq=300000 > [ 0.208767][ T1] s3c2410-wdt 10060000.watchdog_cl0: Heartbeat: timeout=3665038, divisor=256, count=1099511400000 (fffffc87) > [ 0.208799][ T1] s3c2410-wdt 10060000.watchdog_cl0: starting watchdog timer > [ 0.208969][ T1] s3c2410-wdt 10060000.watchdog_cl0: watchdog active, reset enabled, irq disabled > > If system has 32-bit WTCNT, add QUIRK_HAS_32BIT_MAXCNT to its quirk flags, then > it will operation with 32-bit counter. If not, with 16-bit counter like previous. > > Signed-off-by: Sangwook Shin <sw617.shin@samsung.com> > Signed-off-by: Kyunghwan Seo <khwan.seo@samsung.com> > --- > drivers/watchdog/s3c2410_wdt.c | 30 ++++++++++++++++++++++-------- > 1 file changed, 22 insertions(+), 8 deletions(-) > > diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c > index bdd81d8074b2..a13768a11f20 100644 > --- a/drivers/watchdog/s3c2410_wdt.c > +++ b/drivers/watchdog/s3c2410_wdt.c > @@ -34,6 +34,7 @@ > #define S3C2410_WTCLRINT 0x0c > > #define S3C2410_WTCNT_MAXCNT 0xffff > +#define S3C2410_WTCNT_MAXCNT_32 0xffffffff > > #define S3C2410_WTCON_RSTEN BIT(0) > #define S3C2410_WTCON_INTEN BIT(2) > @@ -119,6 +120,10 @@ > * %QUIRK_HAS_DBGACK_BIT: WTCON register has DBGACK_MASK bit. Setting the > * DBGACK_MASK bit disables the watchdog outputs when the SoC is in debug mode. > * Debug mode is determined by the DBGACK CPU signal. > + * > + * %QUIRK_HAS_32BIT_MAXCNT: WTDAT and WTCNT are 32-bit registers. With these > + * 32-bit registers, larger values to be set, which means that larger timeouts > + * value can be set. > */ > #define QUIRK_HAS_WTCLRINT_REG BIT(0) > #define QUIRK_HAS_PMU_MASK_RESET BIT(1) > @@ -126,6 +131,7 @@ > #define QUIRK_HAS_PMU_AUTO_DISABLE BIT(3) > #define QUIRK_HAS_PMU_CNT_EN BIT(4) > #define QUIRK_HAS_DBGACK_BIT BIT(5) > +#define QUIRK_HAS_32BIT_MAXCNT BIT(6) > > /* These quirks require that we have a PMU register map */ > #define QUIRKS_HAVE_PMUREG \ > @@ -378,9 +384,13 @@ static inline unsigned long s3c2410wdt_get_freq(struct s3c2410_wdt *wdt) > static inline unsigned int s3c2410wdt_max_timeout(struct s3c2410_wdt *wdt) > { > const unsigned long freq = s3c2410wdt_get_freq(wdt); > + unsigned long max_cnt = S3C2410_WTCNT_MAXCNT; > + > + if ((wdt->drv_data->quirks & QUIRK_HAS_32BIT_MAXCNT)) > + max_cnt = S3C2410_WTCNT_MAXCNT_32; > > - return S3C2410_WTCNT_MAXCNT / (freq / (S3C2410_WTCON_PRESCALE_MAX + 1) > - / S3C2410_WTCON_MAXDIV); > + return max_cnt * (S3C2410_WTCON_PRESCALE_MAX + 1) > + * S3C2410_WTCON_MAXDIV / freq; This will overflow if the build is a 32-bit build with sizeof(long) == 4. The previous calculation did not have that problem. If there was a secondary reason to change the calculation, please submit a second patch with that change and explanation on its own. Thanks, Guenter > } > > static int s3c2410wdt_disable_wdt_reset(struct s3c2410_wdt *wdt, bool mask) > @@ -534,9 +544,10 @@ static int s3c2410wdt_set_heartbeat(struct watchdog_device *wdd, > { > struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd); > unsigned long freq = s3c2410wdt_get_freq(wdt); > - unsigned int count; > + unsigned long count; > unsigned int divisor = 1; > unsigned long wtcon; > + unsigned int max_cnt = S3C2410_WTCNT_MAXCNT; > > if (timeout < 1) > return -EINVAL; > @@ -544,7 +555,7 @@ static int s3c2410wdt_set_heartbeat(struct watchdog_device *wdd, > freq = DIV_ROUND_UP(freq, 128); > count = timeout * freq; > > - dev_dbg(wdt->dev, "Heartbeat: count=%d, timeout=%d, freq=%lu\n", > + dev_dbg(wdt->dev, "Heartbeat: count=%lu, timeout=%d, freq=%lu\n", > count, timeout, freq); > > /* if the count is bigger than the watchdog register, > @@ -552,16 +563,19 @@ static int s3c2410wdt_set_heartbeat(struct watchdog_device *wdd, > actually make this value > */ > > - if (count >= 0x10000) { > - divisor = DIV_ROUND_UP(count, 0xffff); > + if ((wdt->drv_data->quirks & QUIRK_HAS_32BIT_MAXCNT)) > + max_cnt = S3C2410_WTCNT_MAXCNT_32; > + I think it would make sense to store max_cnt in struct s3c2410_wdt instead of recalculating it repeatedly during runtime. > + if (count > max_cnt) { > + divisor = DIV_ROUND_UP(count, max_cnt); > > - if (divisor > 0x100) { > + if (divisor > (S3C2410_WTCON_PRESCALE_MAX + 1)) { That is another change that seems unrelated to this patch. Note that inner the ( ) s unnecessary. > dev_err(wdt->dev, "timeout %d too big\n", timeout); > return -EINVAL; > } > } > > - dev_dbg(wdt->dev, "Heartbeat: timeout=%d, divisor=%d, count=%d (%08x)\n", > + dev_dbg(wdt->dev, "Heartbeat: timeout=%d, divisor=%d, count=%lu (%08lx)\n", > timeout, divisor, count, DIV_ROUND_UP(count, divisor)); > > count = DIV_ROUND_UP(count, divisor);
diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c index bdd81d8074b2..a13768a11f20 100644 --- a/drivers/watchdog/s3c2410_wdt.c +++ b/drivers/watchdog/s3c2410_wdt.c @@ -34,6 +34,7 @@ #define S3C2410_WTCLRINT 0x0c #define S3C2410_WTCNT_MAXCNT 0xffff +#define S3C2410_WTCNT_MAXCNT_32 0xffffffff #define S3C2410_WTCON_RSTEN BIT(0) #define S3C2410_WTCON_INTEN BIT(2) @@ -119,6 +120,10 @@ * %QUIRK_HAS_DBGACK_BIT: WTCON register has DBGACK_MASK bit. Setting the * DBGACK_MASK bit disables the watchdog outputs when the SoC is in debug mode. * Debug mode is determined by the DBGACK CPU signal. + * + * %QUIRK_HAS_32BIT_MAXCNT: WTDAT and WTCNT are 32-bit registers. With these + * 32-bit registers, larger values to be set, which means that larger timeouts + * value can be set. */ #define QUIRK_HAS_WTCLRINT_REG BIT(0) #define QUIRK_HAS_PMU_MASK_RESET BIT(1) @@ -126,6 +131,7 @@ #define QUIRK_HAS_PMU_AUTO_DISABLE BIT(3) #define QUIRK_HAS_PMU_CNT_EN BIT(4) #define QUIRK_HAS_DBGACK_BIT BIT(5) +#define QUIRK_HAS_32BIT_MAXCNT BIT(6) /* These quirks require that we have a PMU register map */ #define QUIRKS_HAVE_PMUREG \ @@ -378,9 +384,13 @@ static inline unsigned long s3c2410wdt_get_freq(struct s3c2410_wdt *wdt) static inline unsigned int s3c2410wdt_max_timeout(struct s3c2410_wdt *wdt) { const unsigned long freq = s3c2410wdt_get_freq(wdt); + unsigned long max_cnt = S3C2410_WTCNT_MAXCNT; + + if ((wdt->drv_data->quirks & QUIRK_HAS_32BIT_MAXCNT)) + max_cnt = S3C2410_WTCNT_MAXCNT_32; - return S3C2410_WTCNT_MAXCNT / (freq / (S3C2410_WTCON_PRESCALE_MAX + 1) - / S3C2410_WTCON_MAXDIV); + return max_cnt * (S3C2410_WTCON_PRESCALE_MAX + 1) + * S3C2410_WTCON_MAXDIV / freq; } static int s3c2410wdt_disable_wdt_reset(struct s3c2410_wdt *wdt, bool mask) @@ -534,9 +544,10 @@ static int s3c2410wdt_set_heartbeat(struct watchdog_device *wdd, { struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd); unsigned long freq = s3c2410wdt_get_freq(wdt); - unsigned int count; + unsigned long count; unsigned int divisor = 1; unsigned long wtcon; + unsigned int max_cnt = S3C2410_WTCNT_MAXCNT; if (timeout < 1) return -EINVAL; @@ -544,7 +555,7 @@ static int s3c2410wdt_set_heartbeat(struct watchdog_device *wdd, freq = DIV_ROUND_UP(freq, 128); count = timeout * freq; - dev_dbg(wdt->dev, "Heartbeat: count=%d, timeout=%d, freq=%lu\n", + dev_dbg(wdt->dev, "Heartbeat: count=%lu, timeout=%d, freq=%lu\n", count, timeout, freq); /* if the count is bigger than the watchdog register, @@ -552,16 +563,19 @@ static int s3c2410wdt_set_heartbeat(struct watchdog_device *wdd, actually make this value */ - if (count >= 0x10000) { - divisor = DIV_ROUND_UP(count, 0xffff); + if ((wdt->drv_data->quirks & QUIRK_HAS_32BIT_MAXCNT)) + max_cnt = S3C2410_WTCNT_MAXCNT_32; + + if (count > max_cnt) { + divisor = DIV_ROUND_UP(count, max_cnt); - if (divisor > 0x100) { + if (divisor > (S3C2410_WTCON_PRESCALE_MAX + 1)) { dev_err(wdt->dev, "timeout %d too big\n", timeout); return -EINVAL; } } - dev_dbg(wdt->dev, "Heartbeat: timeout=%d, divisor=%d, count=%d (%08x)\n", + dev_dbg(wdt->dev, "Heartbeat: timeout=%d, divisor=%d, count=%lu (%08lx)\n", timeout, divisor, count, DIV_ROUND_UP(count, divisor)); count = DIV_ROUND_UP(count, divisor);