[PATCH v4 0/4] Increase max timeout value of s3c2410 watchdog

[PATCH v4 0/4] Increase max timeout value of s3c2410 watchdog

[Sangwook Shin]

The ExynosAutoV9 and ExynosAutoV920 SoCs have a 32-bit counter register,
but due to code constraints, only 16-bit values could be used.
This series enables these SoCs to use the 32-bit counter.
Additionally, it addresses the issue where the ExynosAutoV9 SoC supports
the DBGACK bit but it was not set.

V3->V4:
  - Merge patches [v3 3/5] and [v3 4/5] into one so that Quirk and its consumer
    are part of the same patch.
  - Link to v3:
    https://lore.kernel.org/linux-watchdog/20250714055440.3138135-1-sw617.shin@samsung.com/
    https://lore.kernel.org/linux-watchdog/20250515075350.3368635-1-sw617.shin@samsung.com/

V2->V3:
  - Correct the incorrect tag information.
  - Link to v2:
    https://lore.kernel.org/linux-watchdog/20250514094220.1561378-1-sw617.shin@samsung.com/

V1->V2:
  - Modify the max_timeout calculation considering overflow
  - Separate tha max_timeout calculation into a separate patch
  - Add max_cnt in struct s3c2410_wdt
  - Set max_cnt once in probe function
  - Add patch that uses S3C2410_WTCON_PRESCALE_MAX instead of hardcoded one
  - Remove unnecessary inner parentheses
  - Link to v1:
    https://lore.kernel.org/linux-watchdog/20250513094711.2691059-1-sw617.shin@samsung.com/

Sangwook Shin (4):
  watchdog: s3c2410_wdt: Replace hardcoded values with macro definitions
  watchdog: s3c2410_wdt: Fix max_timeout being calculated larger
  watchdog: s3c2410_wdt: Increase max timeout value of watchdog
  watchdog: s3c2410_wdt: exynosautov9: Enable supported features

 drivers/watchdog/s3c2410_wdt.c | 37 +++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 12 deletions(-)

-- 
2.25.1

[PATCH v4 1/4] watchdog: s3c2410_wdt: Replace hardcoded values with macro definitions

[Sangwook Shin]

Modify the code to utilize macro-defined values instead of hardcoded
values. The value 0x100 in the s3c2410wdt_set_heartbeat function represents
S3C2410_WTCON_PRESCALE_MAX + 1, but it is hardcoded, making its meaning
difficult to understand and reducing code readability.

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>
Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
Signed-off-by: Sangwook Shin <sw617.shin@samsung.com>
---
 drivers/watchdog/s3c2410_wdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 40901bdac426..95f7207e390a 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -587,7 +587,7 @@ static int s3c2410wdt_set_heartbeat(struct watchdog_device *wdd,
 	if (count >= 0x10000) {
 		divisor = DIV_ROUND_UP(count, 0xffff);
 
-		if (divisor > 0x100) {
+		if (divisor > S3C2410_WTCON_PRESCALE_MAX + 1) {
 			dev_err(wdt->dev, "timeout %d too big\n", timeout);
 			return -EINVAL;
 		}
-- 
2.25.1

[PATCH v4 2/4] watchdog: s3c2410_wdt: Fix max_timeout being calculated larger

[Sangwook Shin]

Fix the issue of max_timeout being calculated larger than actual value.
The calculation result of freq / (S3C2410_WTCON_PRESCALE_MAX + 1) /
S3C2410_WTCON_MAXDIV is smaller than the actual value because the remainder
is discarded during the calculation process. This leads to a larger
calculated value for max_timeout compared to the actual settable value.
A ceiling operation is applied in the calculation process to resolve this.

Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
Signed-off-by: Sangwook Shin <sw617.shin@samsung.com>
---
 drivers/watchdog/s3c2410_wdt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 95f7207e390a..31f7e1ec779e 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -411,8 +411,8 @@ static inline unsigned int s3c2410wdt_max_timeout(struct s3c2410_wdt *wdt)
 {
 	const unsigned long freq = s3c2410wdt_get_freq(wdt);
 
-	return S3C2410_WTCNT_MAXCNT / (freq / (S3C2410_WTCON_PRESCALE_MAX + 1)
-				       / S3C2410_WTCON_MAXDIV);
+	return S3C2410_WTCNT_MAXCNT / DIV_ROUND_UP(freq,
+		(S3C2410_WTCON_PRESCALE_MAX + 1) * S3C2410_WTCON_MAXDIV);
 }
 
 static int s3c2410wdt_disable_wdt_reset(struct s3c2410_wdt *wdt, bool mask)
-- 
2.25.1

[PATCH v4 3/4] watchdog: s3c2410_wdt: Increase max timeout value of watchdog

[Sangwook Shin]

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

[PATCH v4 4/4] watchdog: s3c2410_wdt: exynosautov9: Enable supported features

[Sangwook Shin]

Enable supported features for ExynosAutov9 SoC.
- QUIRK_HAS_DBGACK_BIT
- QUIRK_HAS_32BIT_MAXCNT

Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
Signed-off-by: Sangwook Shin <sw617.shin@samsung.com>
---
 drivers/watchdog/s3c2410_wdt.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 184b1ad46ca6..16a845f41e74 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -305,7 +305,8 @@ static const struct s3c2410_wdt_variant drv_data_exynosautov9_cl0 = {
 	.cnt_en_reg = EXYNOS850_CLUSTER0_NONCPU_OUT,
 	.cnt_en_bit = 7,
 	.quirks = QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_MASK_RESET |
-		  QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_CNT_EN,
+		  QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_CNT_EN |
+		  QUIRK_HAS_DBGACK_BIT | QUIRK_HAS_32BIT_MAXCNT,
 };
 
 static const struct s3c2410_wdt_variant drv_data_exynosautov9_cl1 = {
@@ -317,7 +318,8 @@ static const struct s3c2410_wdt_variant drv_data_exynosautov9_cl1 = {
 	.cnt_en_reg = EXYNOSAUTOV9_CLUSTER1_NONCPU_OUT,
 	.cnt_en_bit = 7,
 	.quirks = QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_MASK_RESET |
-		  QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_CNT_EN,
+		  QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_CNT_EN |
+		  QUIRK_HAS_DBGACK_BIT | QUIRK_HAS_32BIT_MAXCNT,
 };
 
 static const struct s3c2410_wdt_variant drv_data_gs101_cl0 = {
-- 
2.25.1

Re: [PATCH v4 2/4] watchdog: s3c2410_wdt: Fix max_timeout being calculated larger

[Sam Protsenko]

On Thu, Jul 24, 2025 at 3:13 AM Sangwook Shin <sw617.shin@samsung.com> wrote:
>
> Fix the issue of max_timeout being calculated larger than actual value.
> The calculation result of freq / (S3C2410_WTCON_PRESCALE_MAX + 1) /
> S3C2410_WTCON_MAXDIV is smaller than the actual value because the remainder
> is discarded during the calculation process. This leads to a larger
> calculated value for max_timeout compared to the actual settable value.
> A ceiling operation is applied in the calculation process to resolve this.
>
> Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
> Signed-off-by: Sangwook Shin <sw617.shin@samsung.com>
> ---
>  drivers/watchdog/s3c2410_wdt.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> index 95f7207e390a..31f7e1ec779e 100644
> --- a/drivers/watchdog/s3c2410_wdt.c
> +++ b/drivers/watchdog/s3c2410_wdt.c
> @@ -411,8 +411,8 @@ static inline unsigned int s3c2410wdt_max_timeout(struct s3c2410_wdt *wdt)
>  {
>         const unsigned long freq = s3c2410wdt_get_freq(wdt);
>
> -       return S3C2410_WTCNT_MAXCNT / (freq / (S3C2410_WTCON_PRESCALE_MAX + 1)
> -                                      / S3C2410_WTCON_MAXDIV);
> +       return S3C2410_WTCNT_MAXCNT / DIV_ROUND_UP(freq,
> +               (S3C2410_WTCON_PRESCALE_MAX + 1) * S3C2410_WTCON_MAXDIV);
>  }
>

How about something like this instead?

8<--------------------------------------------------------------------->8
static inline unsigned int s3c2410wdt_max_timeout(unsigned long freq)
{
    const u64 div_max = (S3C2410_WTCON_PRESCALE_MAX + 1) *
                S3C2410_WTCON_MAXDIV; /* 32768 */
    const u64 n_max = S3C2410_WTCNT_MAXCNT * div_max;
    u64 t_max = n_max / freq;

    if (t_max > UINT_MAX)
        t_max = UINT_MAX;

    return (unsigned int)t_max;
}
8<--------------------------------------------------------------------->8

This implementation's result:
  - is never greater than real timeout, as it loses the decimal part
after integer division in t_max
  - much closer to the real timeout value, as it benefits from very
big n_max in the numerator (this is the main trick here)
  - prepared for using 32-bit max counter value in your next patch, as
it uses u64 type for calculations

For example, at the clock frequency of 33 kHz:
  - real timeout is: 65074.269 sec
  - old function returns: 65535 sec
  - your function returns: 32767 sec
  - the suggested function returns: 65074 sec

I've prepared the test program you can run on your machine to play
with all implementations: [1].

Thanks!

[1] https://gist.github.com/joe-skb7/c2b2290bb0b0572a4018d81fc4ba1f3e

>  static int s3c2410wdt_disable_wdt_reset(struct s3c2410_wdt *wdt, bool mask)
> --
> 2.25.1
>

Re: [PATCH v4 3/4] watchdog: s3c2410_wdt: Increase max timeout value of watchdog

[Sam Protsenko]

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
>

Re: [PATCH v4 4/4] watchdog: s3c2410_wdt: exynosautov9: Enable supported features

[Sam Protsenko]

On Thu, Jul 24, 2025 at 3:13 AM Sangwook Shin <sw617.shin@samsung.com> wrote:
>
> Enable supported features for ExynosAutov9 SoC.
> - QUIRK_HAS_DBGACK_BIT
> - QUIRK_HAS_32BIT_MAXCNT
>
> Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
> Signed-off-by: Sangwook Shin <sw617.shin@samsung.com>
> ---

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

>  drivers/watchdog/s3c2410_wdt.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> index 184b1ad46ca6..16a845f41e74 100644
> --- a/drivers/watchdog/s3c2410_wdt.c
> +++ b/drivers/watchdog/s3c2410_wdt.c
> @@ -305,7 +305,8 @@ static const struct s3c2410_wdt_variant drv_data_exynosautov9_cl0 = {
>         .cnt_en_reg = EXYNOS850_CLUSTER0_NONCPU_OUT,
>         .cnt_en_bit = 7,
>         .quirks = QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_MASK_RESET |
> -                 QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_CNT_EN,
> +                 QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_CNT_EN |
> +                 QUIRK_HAS_DBGACK_BIT | QUIRK_HAS_32BIT_MAXCNT,
>  };
>
>  static const struct s3c2410_wdt_variant drv_data_exynosautov9_cl1 = {
> @@ -317,7 +318,8 @@ static const struct s3c2410_wdt_variant drv_data_exynosautov9_cl1 = {
>         .cnt_en_reg = EXYNOSAUTOV9_CLUSTER1_NONCPU_OUT,
>         .cnt_en_bit = 7,
>         .quirks = QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_MASK_RESET |
> -                 QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_CNT_EN,
> +                 QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_CNT_EN |
> +                 QUIRK_HAS_DBGACK_BIT | QUIRK_HAS_32BIT_MAXCNT,
>  };
>
>  static const struct s3c2410_wdt_variant drv_data_gs101_cl0 = {
> --
> 2.25.1
>

RE: [PATCH v4 2/4] watchdog: s3c2410_wdt: Fix max_timeout being calculated larger

[sw617.shin]

On Saturday, August 2, 2025 at 1:12 PM Sam Protsenko <semen.protsenko@linaro.org> wrote:

> How about something like this instead?
> 
> 8<--------------------------------------------------------------------->8
> static inline unsigned int s3c2410wdt_max_timeout(unsigned long freq) {
>     const u64 div_max = (S3C2410_WTCON_PRESCALE_MAX + 1) *
>                 S3C2410_WTCON_MAXDIV; /* 32768 */
>     const u64 n_max = S3C2410_WTCNT_MAXCNT * div_max;
>     u64 t_max = n_max / freq;
> 
>     if (t_max > UINT_MAX)
>         t_max = UINT_MAX;
> 
>     return (unsigned int)t_max;
> }
> 8<--------------------------------------------------------------------->8
> 
> This implementation's result:
>   - is never greater than real timeout, as it loses the decimal part after
> integer division in t_max
>   - much closer to the real timeout value, as it benefits from very big
> n_max in the numerator (this is the main trick here)
>   - prepared for using 32-bit max counter value in your next patch, as it
> uses u64 type for calculations
> 
> For example, at the clock frequency of 33 kHz:
>   - real timeout is: 65074.269 sec
>   - old function returns: 65535 sec
>   - your function returns: 32767 sec
>   - the suggested function returns: 65074 sec

Thank you for your feedback.
I'll make the code changes as follows in the next patch set:

static inline unsigned int s3c2410wdt_max_timeout(struct s3c2410_wdt *wdt)
 {
        const unsigned long freq = s3c2410wdt_get_freq(wdt);
+       const u64 div_max = (S3C2410_WTCON_PRESCALE_MAX + 1) *
+                       S3C2410_WTCON_MAXDIV;
+       const u64 n_max = S3C2410_WTCNT_MAXCNT * div_max;
+       u64 t_max = n_max / freq;

-       return S3C2410_WTCNT_MAXCNT / (freq / (S3C2410_WTCON_PRESCALE_MAX + 1)
-                                      / S3C2410_WTCON_MAXDIV);
+       if (t_max > UINT_MAX)
+               t_max = UINT_MAX;
+
+       return (unsigned int)t_max;
 }

RE: [PATCH v4 3/4] watchdog: s3c2410_wdt: Increase max timeout value of watchdog

[sw617.shin]

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.

Re: [PATCH v4 2/4] watchdog: s3c2410_wdt: Fix max_timeout being calculated larger

[Guenter Roeck]

On 8/4/25 21:22, sw617.shin@samsung.com wrote:
> On Saturday, August 2, 2025 at 1:12 PM Sam Protsenko <semen.protsenko@linaro.org> wrote:
> 
>> How about something like this instead?
>>
>> 8<--------------------------------------------------------------------->8
>> static inline unsigned int s3c2410wdt_max_timeout(unsigned long freq) {
>>      const u64 div_max = (S3C2410_WTCON_PRESCALE_MAX + 1) *
>>                  S3C2410_WTCON_MAXDIV; /* 32768 */
>>      const u64 n_max = S3C2410_WTCNT_MAXCNT * div_max;
>>      u64 t_max = n_max / freq;
>>
>>      if (t_max > UINT_MAX)
>>          t_max = UINT_MAX;
>>
>>      return (unsigned int)t_max;
>> }
>> 8<--------------------------------------------------------------------->8
>>
>> This implementation's result:
>>    - is never greater than real timeout, as it loses the decimal part after
>> integer division in t_max
>>    - much closer to the real timeout value, as it benefits from very big
>> n_max in the numerator (this is the main trick here)
>>    - prepared for using 32-bit max counter value in your next patch, as it
>> uses u64 type for calculations
>>
>> For example, at the clock frequency of 33 kHz:
>>    - real timeout is: 65074.269 sec
>>    - old function returns: 65535 sec
>>    - your function returns: 32767 sec
>>    - the suggested function returns: 65074 sec
> 
> Thank you for your feedback.
> I'll make the code changes as follows in the next patch set:
> 
> static inline unsigned int s3c2410wdt_max_timeout(struct s3c2410_wdt *wdt)
>   {
>          const unsigned long freq = s3c2410wdt_get_freq(wdt);
> +       const u64 div_max = (S3C2410_WTCON_PRESCALE_MAX + 1) *
> +                       S3C2410_WTCON_MAXDIV;
> +       const u64 n_max = S3C2410_WTCNT_MAXCNT * div_max;

Not sure if splitting this expression adds any value. Why not just the following ?

const u64 n_max = (u64)(S3C2410_WTCON_PRESCALE_MAX + 1) *
                    S3C2410_WTCON_MAXDIV * S3C2410_WTCNT_MAXCNT;

Or just use a define ?

> +       u64 t_max = n_max / freq;
> 

Make sure this compiles on 32-bit builds.

> -       return S3C2410_WTCNT_MAXCNT / (freq / (S3C2410_WTCON_PRESCALE_MAX + 1)
> -                                      / S3C2410_WTCON_MAXDIV);
> +       if (t_max > UINT_MAX)
> +               t_max = UINT_MAX;
> +
> +       return (unsigned int)t_max;

I am quite sure that this typecast is unnecessary.

Guenter

Re: [PATCH v4 3/4] watchdog: s3c2410_wdt: Increase max timeout value of watchdog

[Sam Protsenko]

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.
>
>
>

Re: [PATCH v4 2/4] watchdog: s3c2410_wdt: Fix max_timeout being calculated larger

[Sam Protsenko]

On Mon, Aug 4, 2025 at 11:47 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 8/4/25 21:22, sw617.shin@samsung.com wrote:
> > On Saturday, August 2, 2025 at 1:12 PM Sam Protsenko <semen.protsenko@linaro.org> wrote:
> >
> >> How about something like this instead?
> >>
> >> 8<--------------------------------------------------------------------->8
> >> static inline unsigned int s3c2410wdt_max_timeout(unsigned long freq) {
> >>      const u64 div_max = (S3C2410_WTCON_PRESCALE_MAX + 1) *
> >>                  S3C2410_WTCON_MAXDIV; /* 32768 */
> >>      const u64 n_max = S3C2410_WTCNT_MAXCNT * div_max;
> >>      u64 t_max = n_max / freq;
> >>
> >>      if (t_max > UINT_MAX)
> >>          t_max = UINT_MAX;
> >>
> >>      return (unsigned int)t_max;
> >> }
> >> 8<--------------------------------------------------------------------->8
> >>
> >> This implementation's result:
> >>    - is never greater than real timeout, as it loses the decimal part after
> >> integer division in t_max
> >>    - much closer to the real timeout value, as it benefits from very big
> >> n_max in the numerator (this is the main trick here)
> >>    - prepared for using 32-bit max counter value in your next patch, as it
> >> uses u64 type for calculations
> >>
> >> For example, at the clock frequency of 33 kHz:
> >>    - real timeout is: 65074.269 sec
> >>    - old function returns: 65535 sec
> >>    - your function returns: 32767 sec
> >>    - the suggested function returns: 65074 sec
> >
> > Thank you for your feedback.
> > I'll make the code changes as follows in the next patch set:
> >
> > static inline unsigned int s3c2410wdt_max_timeout(struct s3c2410_wdt *wdt)
> >   {
> >          const unsigned long freq = s3c2410wdt_get_freq(wdt);
> > +       const u64 div_max = (S3C2410_WTCON_PRESCALE_MAX + 1) *
> > +                       S3C2410_WTCON_MAXDIV;
> > +       const u64 n_max = S3C2410_WTCNT_MAXCNT * div_max;
>
> Not sure if splitting this expression adds any value. Why not just the following ?
>
> const u64 n_max = (u64)(S3C2410_WTCON_PRESCALE_MAX + 1) *
>                     S3C2410_WTCON_MAXDIV * S3C2410_WTCNT_MAXCNT;
>
> Or just use a define ?
>

Oh, that code snippet above is just a suggestion, please treat it as
pseudo-code for this purpose, -- I just wanted to illustrate the idea,
and should've been more clear! Yes, definitely should be revised and
re-tested. And yeah, I agree, one single const or define would be
enough, no need to make it too verbose. Also, the naming may be not
the best, e.g. cnt_max might be better than n_max for example. But
I'll leave it to Sangwook Shin to decide on the implementation, just
wanted to communicate the idea.

> > +       u64 t_max = n_max / freq;
> >
>
> Make sure this compiles on 32-bit builds.
>

Can you please elaborate what might be the possible problem -- just
curious? I admit I never though about 32-bit case when writing that
code, but don't see any immediate issues with that too.

> > -       return S3C2410_WTCNT_MAXCNT / (freq / (S3C2410_WTCON_PRESCALE_MAX + 1)
> > -                                      / S3C2410_WTCON_MAXDIV);
> > +       if (t_max > UINT_MAX)
> > +               t_max = UINT_MAX;
> > +
> > +       return (unsigned int)t_max;
>
> I am quite sure that this typecast is unnecessary.
>
> Guenter
>

RE: [PATCH v4 2/4] watchdog: s3c2410_wdt: Fix max_timeout being calculated larger

[sw617.shin]

On Tuesday, August 5, 2025 at 2:03 PM Sam Protsenko <semen.protsenko@linaro.org> wrote:

> 
> > > +       u64 t_max = n_max / freq;
> > >
> >
> > Make sure this compiles on 32-bit builds.
> >
> 
> Can you please elaborate what might be the possible problem -- just
> curious? I admit I never though about 32-bit case when writing that code,
> but don't see any immediate issues with that too.
> 

In my opinion, it seems that Gunter Reck's explanation is correct.
I've found out that the error of "undefined reference to '__aeabi_uldivmod'" may occur when compiling new code on a 32-bit architecture.
If you don't mind, I would like to proceed with maintaining the previous revision below.
From my perspective, this approach appears to be the most reasonable solution for supporting both 32-bit and 64-bit architectures.

@@ -411,8 +411,8 @@ static inline unsigned int s3c2410wdt_max_timeout(struct s3c2410_wdt *wdt)
 {
 	const unsigned long freq = s3c2410wdt_get_freq(wdt);
 
-	return S3C2410_WTCNT_MAXCNT / (freq / (S3C2410_WTCON_PRESCALE_MAX + 1)
-				       / S3C2410_WTCON_MAXDIV);
+	return S3C2410_WTCNT_MAXCNT / DIV_ROUND_UP(freq,
+		(S3C2410_WTCON_PRESCALE_MAX + 1) * S3C2410_WTCON_MAXDIV);
 }

Re: [PATCH v4 2/4] watchdog: s3c2410_wdt: Fix max_timeout being calculated larger

[Guenter Roeck]

On 8/5/25 00:26, sw617.shin@samsung.com wrote:
> On Tuesday, August 5, 2025 at 2:03 PM Sam Protsenko <semen.protsenko@linaro.org> wrote:
> 
>>
>>>> +       u64 t_max = n_max / freq;
>>>>
>>>
>>> Make sure this compiles on 32-bit builds.
>>>
>>
>> Can you please elaborate what might be the possible problem -- just
>> curious? I admit I never though about 32-bit case when writing that code,
>> but don't see any immediate issues with that too.
>>
> 
> In my opinion, it seems that Gunter Reck's explanation is correct.
> I've found out that the error of "undefined reference to '__aeabi_uldivmod'" may occur when compiling new code on a 32-bit architecture.

Also see include/linux/math64.h. The functions implemented or declared
in that include file do have a reason to exist.

Guenter

Re: [PATCH v4 2/4] watchdog: s3c2410_wdt: Fix max_timeout being calculated larger

[Sam Protsenko]

On Tue, Aug 5, 2025 at 2:26 AM <sw617.shin@samsung.com> wrote:
>
> On Tuesday, August 5, 2025 at 2:03 PM Sam Protsenko <semen.protsenko@linaro.org> wrote:
>
> >
> > > > +       u64 t_max = n_max / freq;
> > > >
> > >
> > > Make sure this compiles on 32-bit builds.
> > >
> >
> > Can you please elaborate what might be the possible problem -- just
> > curious? I admit I never though about 32-bit case when writing that code,
> > but don't see any immediate issues with that too.
> >
>
> In my opinion, it seems that Gunter Reck's explanation is correct.
> I've found out that the error of "undefined reference to '__aeabi_uldivmod'" may occur when compiling new code on a 32-bit architecture.

Indeed. I was able to reproduce that behavior when building for ARM32 too.

> If you don't mind, I would like to proceed with maintaining the previous revision below.
> From my perspective, this approach appears to be the most reasonable solution for supporting both 32-bit and 64-bit architectures.
>
> @@ -411,8 +411,8 @@ static inline unsigned int s3c2410wdt_max_timeout(struct s3c2410_wdt *wdt)
>  {
>         const unsigned long freq = s3c2410wdt_get_freq(wdt);
>
> -       return S3C2410_WTCNT_MAXCNT / (freq / (S3C2410_WTCON_PRESCALE_MAX + 1)
> -                                      / S3C2410_WTCON_MAXDIV);
> +       return S3C2410_WTCNT_MAXCNT / DIV_ROUND_UP(freq,
> +               (S3C2410_WTCON_PRESCALE_MAX + 1) * S3C2410_WTCON_MAXDIV);
>  }
>

I don't mind, although it's quite easy to fix the code I suggested by
replacing this line:

    u64 t_max = n_max / freq;

with this one:

    u64 t_max = div64_ul(n_max, freq);

from <math64.h>, as Guenter suggested. But I'm totally fine with your
implementation as well.

RE: [PATCH v4 2/4] watchdog: s3c2410_wdt: Fix max_timeout being calculated larger

[sw617.shin]

On Wednesday, August 6, 2025 at 7:53 AM Sam Protsenko <semen.protsenko@linaro.org> wrote:

> 
> I don't mind, although it's quite easy to fix the code I suggested by
> replacing this line:
> 
>     u64 t_max = n_max / freq;
> 
> with this one:
> 
>     u64 t_max = div64_ul(n_max, freq);
> 
> from <math64.h>, as Guenter suggested. But I'm totally fine with your
> implementation as well.

Sam Protsenko and Guenter Roeck, thank you for your kind advice.
As you mentioned, I will proceed with the following code for the next patch set.

@@ -27,6 +27,7 @@
 #include <linux/mfd/syscon.h>
 #include <linux/regmap.h>
 #include <linux/delay.h>
+#include <linux/math64.h>

 #define S3C2410_WTCON          0x00
 #define S3C2410_WTDAT          0x04
@@ -410,9 +411,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);
+       //(S3C2410_WTCON_PRESCALE_MAX + 1) * S3C2410_WTCON_MAXDIV = 0x8000
+       u64 t_max = div64_ul((u64)S3C2410_WTCNT_MAXCNT * 0x8000, freq);

-       return S3C2410_WTCNT_MAXCNT / (freq / (S3C2410_WTCON_PRESCALE_MAX + 1)
-                                      / S3C2410_WTCON_MAXDIV);
+       if (t_max > UINT_MAX)
+               t_max = UINT_MAX;
+
+       return t_max;
 }