Increase max_timeout value from 55s to 3664647s (1017h 57min 27s) with
38400000 frequency system if the system has 32-bit WTCNT register.
cat /sys/devices/platform/10060000.watchdog_cl0/watchdog/watchdog0/max_timeout
3664647
[ 0.302473] s3c2410-wdt 10060000.watchdog_cl0: Heartbeat: count=1099394100000, timeout=3664647, freq=300000
[ 0.302479] s3c2410-wdt 10060000.watchdog_cl0: Heartbeat: timeout=3664647, divisor=256, count=1099394100000 (fff8feac)
[ 0.302510] s3c2410-wdt 10060000.watchdog_cl0: starting watchdog timer
[ 0.302722] 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.
Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
Signed-off-by: Sangwook Shin <sw617.shin@samsung.com>
---
drivers/watchdog/s3c2410_wdt.c | 27 +++++++++++++++++++--------
1 file changed, 19 insertions(+), 8 deletions(-)
diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 31f7e1ec779e..184b1ad46ca6 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)
@@ -123,6 +124,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)
@@ -130,6 +135,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 \
@@ -198,6 +204,7 @@ struct s3c2410_wdt {
struct notifier_block freq_transition;
const struct s3c2410_wdt_variant *drv_data;
struct regmap *pmureg;
+ unsigned int max_cnt;
};
static const struct s3c2410_wdt_variant drv_data_s3c2410 = {
@@ -349,7 +356,7 @@ static const struct s3c2410_wdt_variant drv_data_exynosautov920_cl0 = {
.cnt_en_bit = 8,
.quirks = QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_MASK_RESET |
QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_CNT_EN |
- QUIRK_HAS_DBGACK_BIT,
+ QUIRK_HAS_DBGACK_BIT | QUIRK_HAS_32BIT_MAXCNT,
};
static const struct s3c2410_wdt_variant drv_data_exynosautov920_cl1 = {
@@ -362,7 +369,7 @@ static const struct s3c2410_wdt_variant drv_data_exynosautov920_cl1 = {
.cnt_en_bit = 8,
.quirks = QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_MASK_RESET |
QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_CNT_EN |
- QUIRK_HAS_DBGACK_BIT,
+ QUIRK_HAS_DBGACK_BIT | QUIRK_HAS_32BIT_MAXCNT,
};
static const struct of_device_id s3c2410_wdt_match[] = {
@@ -411,7 +418,7 @@ static inline unsigned int s3c2410wdt_max_timeout(struct s3c2410_wdt *wdt)
{
const unsigned long freq = s3c2410wdt_get_freq(wdt);
- return S3C2410_WTCNT_MAXCNT / DIV_ROUND_UP(freq,
+ return wdt->max_cnt / DIV_ROUND_UP(freq,
(S3C2410_WTCON_PRESCALE_MAX + 1) * S3C2410_WTCON_MAXDIV);
}
@@ -566,7 +573,7 @@ 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;
@@ -576,7 +583,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,
@@ -584,8 +591,8 @@ static int s3c2410wdt_set_heartbeat(struct watchdog_device *wdd,
actually make this value
*/
- if (count >= 0x10000) {
- divisor = DIV_ROUND_UP(count, 0xffff);
+ if (count > wdt->max_cnt) {
+ divisor = DIV_ROUND_UP(count, wdt->max_cnt);
if (divisor > S3C2410_WTCON_PRESCALE_MAX + 1) {
dev_err(wdt->dev, "timeout %d too big\n", timeout);
@@ -593,7 +600,7 @@ static int s3c2410wdt_set_heartbeat(struct watchdog_device *wdd,
}
}
- 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);
@@ -801,6 +808,10 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
if (IS_ERR(wdt->src_clk))
return dev_err_probe(dev, PTR_ERR(wdt->src_clk), "failed to get source clock\n");
+ wdt->max_cnt = S3C2410_WTCNT_MAXCNT;
+ if ((wdt->drv_data->quirks & QUIRK_HAS_32BIT_MAXCNT))
+ wdt->max_cnt = S3C2410_WTCNT_MAXCNT_32;
+
wdt->wdt_device.min_timeout = 1;
wdt->wdt_device.max_timeout = s3c2410wdt_max_timeout(wdt);
--
2.25.1
On Thu, Jul 24, 2025 at 3:13 AM Sangwook Shin <sw617.shin@samsung.com> wrote: > > Increase max_timeout value from 55s to 3664647s (1017h 57min 27s) with > 38400000 frequency system if the system has 32-bit WTCNT register. > > cat /sys/devices/platform/10060000.watchdog_cl0/watchdog/watchdog0/max_timeout > 3664647 > > [ 0.302473] s3c2410-wdt 10060000.watchdog_cl0: Heartbeat: count=1099394100000, timeout=3664647, freq=300000 > [ 0.302479] s3c2410-wdt 10060000.watchdog_cl0: Heartbeat: timeout=3664647, divisor=256, count=1099394100000 (fff8feac) > [ 0.302510] s3c2410-wdt 10060000.watchdog_cl0: starting watchdog timer > [ 0.302722] 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. > > Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com> > Signed-off-by: Sangwook Shin <sw617.shin@samsung.com> > --- Not a strong point, but I'd break this patch into two: 1. Add 32-bit counter feature (without enabling it in exynosautov920 implementation) 2. Enable 32-bit counter feature in exynosautov920 > drivers/watchdog/s3c2410_wdt.c | 27 +++++++++++++++++++-------- > 1 file changed, 19 insertions(+), 8 deletions(-) > > diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c > index 31f7e1ec779e..184b1ad46ca6 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 Suggest renaming this to S3C2410_WTCNT_MAXCNT_16, to emphasize the fact this value is for 16-bit counters. And for consistency with the below one. > +#define S3C2410_WTCNT_MAXCNT_32 0xffffffff > > #define S3C2410_WTCON_RSTEN BIT(0) > #define S3C2410_WTCON_INTEN BIT(2) > @@ -123,6 +124,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 Why not name it like QUIRK_HAS_32BIT_CNT or QUIRK_HAS_32BIT_COUNTER? As I understand, the quirk means that the chip has 32-bit counter, so MAX bit is not really needed? > + * 32-bit registers, larger values to be set, which means that larger timeouts Spelling: "to be set" -> "will be set" (or "have to be set"). > + * value can be set. > */ > #define QUIRK_HAS_WTCLRINT_REG BIT(0) > #define QUIRK_HAS_PMU_MASK_RESET BIT(1) > @@ -130,6 +135,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 \ > @@ -198,6 +204,7 @@ struct s3c2410_wdt { > struct notifier_block freq_transition; > const struct s3c2410_wdt_variant *drv_data; > struct regmap *pmureg; > + unsigned int max_cnt; Maybe make it u32? It definitely refers to a 32-bit register value, so will be more explicit that way. Not a strong opinion though. > }; > > static const struct s3c2410_wdt_variant drv_data_s3c2410 = { > @@ -349,7 +356,7 @@ static const struct s3c2410_wdt_variant drv_data_exynosautov920_cl0 = { > .cnt_en_bit = 8, > .quirks = QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_MASK_RESET | > QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_CNT_EN | > - QUIRK_HAS_DBGACK_BIT, > + QUIRK_HAS_DBGACK_BIT | QUIRK_HAS_32BIT_MAXCNT, > }; > > static const struct s3c2410_wdt_variant drv_data_exynosautov920_cl1 = { > @@ -362,7 +369,7 @@ static const struct s3c2410_wdt_variant drv_data_exynosautov920_cl1 = { > .cnt_en_bit = 8, > .quirks = QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_MASK_RESET | > QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_CNT_EN | > - QUIRK_HAS_DBGACK_BIT, > + QUIRK_HAS_DBGACK_BIT | QUIRK_HAS_32BIT_MAXCNT, Yeah, I think it would be easier to review and handle further if this exynosautov920 enablement is extracted into a separate patch. > }; > > static const struct of_device_id s3c2410_wdt_match[] = { > @@ -411,7 +418,7 @@ static inline unsigned int s3c2410wdt_max_timeout(struct s3c2410_wdt *wdt) > { > const unsigned long freq = s3c2410wdt_get_freq(wdt); > > - return S3C2410_WTCNT_MAXCNT / DIV_ROUND_UP(freq, > + return wdt->max_cnt / DIV_ROUND_UP(freq, > (S3C2410_WTCON_PRESCALE_MAX + 1) * S3C2410_WTCON_MAXDIV); > } > > @@ -566,7 +573,7 @@ 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; > > @@ -576,7 +583,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, > @@ -584,8 +591,8 @@ static int s3c2410wdt_set_heartbeat(struct watchdog_device *wdd, > actually make this value > */ > > - if (count >= 0x10000) { > - divisor = DIV_ROUND_UP(count, 0xffff); > + if (count > wdt->max_cnt) { wdt->max_cnt + 1? > + divisor = DIV_ROUND_UP(count, wdt->max_cnt); > > if (divisor > S3C2410_WTCON_PRESCALE_MAX + 1) { > dev_err(wdt->dev, "timeout %d too big\n", timeout); > @@ -593,7 +600,7 @@ static int s3c2410wdt_set_heartbeat(struct watchdog_device *wdd, > } > } > > - 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); > @@ -801,6 +808,10 @@ static int s3c2410wdt_probe(struct platform_device *pdev) > if (IS_ERR(wdt->src_clk)) > return dev_err_probe(dev, PTR_ERR(wdt->src_clk), "failed to get source clock\n"); > > + wdt->max_cnt = S3C2410_WTCNT_MAXCNT; > + if ((wdt->drv_data->quirks & QUIRK_HAS_32BIT_MAXCNT)) Double braces don't seem to be needed. > + wdt->max_cnt = S3C2410_WTCNT_MAXCNT_32; > + Style (minor nitpick): this block can be more explicit, i.e.: if ((wdt->drv_data->quirks & QUIRK_HAS_32BIT_MAXCNT)) wdt->max_cnt = S3C2410_WTCNT_MAXCNT_32; else wdt->max_cnt = S3C2410_WTCNT_MAXCNT; > wdt->wdt_device.min_timeout = 1; > wdt->wdt_device.max_timeout = s3c2410wdt_max_timeout(wdt); > > -- > 2.25.1 >
On Saturday, August 2, 2025 at 1:37 PM Sam Protsenko <semen.protsenko@linaro.org> wrote: > Not a strong point, but I'd break this patch into two: > 1. Add 32-bit counter feature (without enabling it in exynosautov920 > implementation) > 2. Enable 32-bit counter feature in exynosautov920 > I'll break this patch into two in the next patch set. > > #define S3C2410_WTCNT_MAXCNT 0xffff > > Suggest renaming this to S3C2410_WTCNT_MAXCNT_16, to emphasize the fact > this value is for 16-bit counters. And for consistency with the below one. > > > +#define S3C2410_WTCNT_MAXCNT_32 0xffffffff > > I'll rename this to S3C2410_WTCNT_MAXCNT_16 in the next patch set. > > + * %QUIRK_HAS_32BIT_MAXCNT: WTDAT and WTCNT are 32-bit registers. > > + With these > > Why not name it like QUIRK_HAS_32BIT_CNT or QUIRK_HAS_32BIT_COUNTER? > As I understand, the quirk means that the chip has 32-bit counter, so MAX > bit is not really needed? > > > + * 32-bit registers, larger values to be set, which means that larger > > + timeouts > > Spelling: "to be set" -> "will be set" (or "have to be set"). I'll modify this in the next patch set. > > + unsigned int max_cnt; > > Maybe make it u32? It definitely refers to a 32-bit register value, so > will be more explicit that way. Not a strong opinion though. > I'll change this to u32 in the next patch set. > > }; > > > > static const struct s3c2410_wdt_variant drv_data_s3c2410 = { > > @@ -349,7 +356,7 @@ static const struct s3c2410_wdt_variant > drv_data_exynosautov920_cl0 = { > > .cnt_en_bit = 8, > > .quirks = QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_MASK_RESET | > > QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_CNT_EN | > > - QUIRK_HAS_DBGACK_BIT, > > + QUIRK_HAS_DBGACK_BIT | QUIRK_HAS_32BIT_MAXCNT, > > }; > > > > static const struct s3c2410_wdt_variant drv_data_exynosautov920_cl1 = { > > @@ -362,7 +369,7 @@ static const struct s3c2410_wdt_variant > drv_data_exynosautov920_cl1 = { > > .cnt_en_bit = 8, > > .quirks = QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_MASK_RESET | > > QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_CNT_EN | > > - QUIRK_HAS_DBGACK_BIT, > > + QUIRK_HAS_DBGACK_BIT | QUIRK_HAS_32BIT_MAXCNT, > > Yeah, I think it would be easier to review and handle further if this > exynosautov920 enablement is extracted into a separate patch. > I'll break this patch into two in the next patch set. > > > > - if (count >= 0x10000) { > > - divisor = DIV_ROUND_UP(count, 0xffff); > > + if (count > wdt->max_cnt) { > > wdt->max_cnt + 1? > Yes, 0x10000 represented 'wdt->max_cnt + 1.' Would you like to suggest any revisions? > > + wdt->max_cnt = S3C2410_WTCNT_MAXCNT; > > + if ((wdt->drv_data->quirks & QUIRK_HAS_32BIT_MAXCNT)) > > Double braces don't seem to be needed. > > > + wdt->max_cnt = S3C2410_WTCNT_MAXCNT_32; > > + > > Style (minor nitpick): this block can be more explicit, i.e.: > > if ((wdt->drv_data->quirks & QUIRK_HAS_32BIT_MAXCNT)) > wdt->max_cnt = S3C2410_WTCNT_MAXCNT_32; > else > wdt->max_cnt = S3C2410_WTCNT_MAXCNT; > I'll fix these in the next patch set.
On Mon, Aug 4, 2025 at 11:23 PM <sw617.shin@samsung.com> wrote: > > On Saturday, August 2, 2025 at 1:37 PM Sam Protsenko <semen.protsenko@linaro.org> wrote: > > Not a strong point, but I'd break this patch into two: > > 1. Add 32-bit counter feature (without enabling it in exynosautov920 > > implementation) > > 2. Enable 32-bit counter feature in exynosautov920 > > > > I'll break this patch into two in the next patch set. > > > > #define S3C2410_WTCNT_MAXCNT 0xffff > > > > Suggest renaming this to S3C2410_WTCNT_MAXCNT_16, to emphasize the fact > > this value is for 16-bit counters. And for consistency with the below one. > > > > > +#define S3C2410_WTCNT_MAXCNT_32 0xffffffff > > > > > I'll rename this to S3C2410_WTCNT_MAXCNT_16 in the next patch set. > > > > + * %QUIRK_HAS_32BIT_MAXCNT: WTDAT and WTCNT are 32-bit registers. > > > + With these > > > > Why not name it like QUIRK_HAS_32BIT_CNT or QUIRK_HAS_32BIT_COUNTER? > > As I understand, the quirk means that the chip has 32-bit counter, so MAX > > bit is not really needed? > > > > > + * 32-bit registers, larger values to be set, which means that larger > > > + timeouts > > > > Spelling: "to be set" -> "will be set" (or "have to be set"). > > I'll modify this in the next patch set. > > > > + unsigned int max_cnt; > > > > Maybe make it u32? It definitely refers to a 32-bit register value, so > > will be more explicit that way. Not a strong opinion though. > > > > I'll change this to u32 in the next patch set. > > > > }; > > > > > > static const struct s3c2410_wdt_variant drv_data_s3c2410 = { > > > @@ -349,7 +356,7 @@ static const struct s3c2410_wdt_variant > > drv_data_exynosautov920_cl0 = { > > > .cnt_en_bit = 8, > > > .quirks = QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_MASK_RESET | > > > QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_CNT_EN | > > > - QUIRK_HAS_DBGACK_BIT, > > > + QUIRK_HAS_DBGACK_BIT | QUIRK_HAS_32BIT_MAXCNT, > > > }; > > > > > > static const struct s3c2410_wdt_variant drv_data_exynosautov920_cl1 = { > > > @@ -362,7 +369,7 @@ static const struct s3c2410_wdt_variant > > drv_data_exynosautov920_cl1 = { > > > .cnt_en_bit = 8, > > > .quirks = QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_MASK_RESET | > > > QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_CNT_EN | > > > - QUIRK_HAS_DBGACK_BIT, > > > + QUIRK_HAS_DBGACK_BIT | QUIRK_HAS_32BIT_MAXCNT, > > > > Yeah, I think it would be easier to review and handle further if this > > exynosautov920 enablement is extracted into a separate patch. > > > > I'll break this patch into two in the next patch set. > > > > > > > - if (count >= 0x10000) { > > > - divisor = DIV_ROUND_UP(count, 0xffff); > > > + if (count > wdt->max_cnt) { > > > > wdt->max_cnt + 1? > > > > Yes, 0x10000 represented 'wdt->max_cnt + 1.' > Would you like to suggest any revisions? > Ah, sorry, I didn't notice you also changed >= to just >. All well and good, no change is needed here! > > > + wdt->max_cnt = S3C2410_WTCNT_MAXCNT; > > > + if ((wdt->drv_data->quirks & QUIRK_HAS_32BIT_MAXCNT)) > > > > Double braces don't seem to be needed. > > > > > + wdt->max_cnt = S3C2410_WTCNT_MAXCNT_32; > > > + > > > > Style (minor nitpick): this block can be more explicit, i.e.: > > > > if ((wdt->drv_data->quirks & QUIRK_HAS_32BIT_MAXCNT)) > > wdt->max_cnt = S3C2410_WTCNT_MAXCNT_32; > > else > > wdt->max_cnt = S3C2410_WTCNT_MAXCNT; > > > > I'll fix these in the next patch set. > > >
© 2016 - 2025 Red Hat, Inc.