From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Update the watchdog minimum timeout value to be derived from
`max_hw_heartbeat_ms` using `DIV_ROUND_UP()` to ensure a valid and
consistent minimum timeout in seconds.

This avoids hardcoding a value of `1` second and allows the driver to
adapt correctly to different hardware configurations that may set
`max_hw_heartbeat_ms` differently (e.g., based on the SoC clock source
and divider).

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v1->v2:
- No changes.
---
 drivers/watchdog/rzv2h_wdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/rzv2h_wdt.c b/drivers/watchdog/rzv2h_wdt.c
index f0e2bf786acc..9c11ce323c16 100644
--- a/drivers/watchdog/rzv2h_wdt.c
+++ b/drivers/watchdog/rzv2h_wdt.c
@@ -263,7 +263,7 @@ static int rzv2h_wdt_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	priv->wdev.min_timeout = 1;
+	priv->wdev.min_timeout = DIV_ROUND_UP(priv->wdev.max_hw_heartbeat_ms, MSEC_PER_SEC);
 	priv->wdev.timeout = WDT_DEFAULT_TIMEOUT;
 	priv->wdev.info = &rzv2h_wdt_ident;
 	priv->wdev.ops = &rzv2h_wdt_ops;
-- 
2.50.1

On Tue, Jul 29, 2025 at 04:59:13PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Update the watchdog minimum timeout value to be derived from
> `max_hw_heartbeat_ms` using `DIV_ROUND_UP()` to ensure a valid and
> consistent minimum timeout in seconds.

I don't understand this change. Why is the _minimum_ timeout based on
the _maximum_ heartbeat?

Hi Wolfram,

Thank you for the review.

On Fri, Aug 1, 2025 at 5:10 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> On Tue, Jul 29, 2025 at 04:59:13PM +0100, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Update the watchdog minimum timeout value to be derived from
> > `max_hw_heartbeat_ms` using `DIV_ROUND_UP()` to ensure a valid and
> > consistent minimum timeout in seconds.
>
> I don't understand this change. Why is the _minimum_ timeout based on
> the _maximum_ heartbeat?
>
The reason for deriving min_timeout from max_hw_heartbeat_ms is to
ensure the minimum watchdog period (in seconds) is compatible with the
underlying hardware.

max_hw_heartbeat_ms is calculated as:
max_hw_heartbeat_ms = (1000 * 16384 * cks_div) / clk_rate;

This value varies by SoC:
 RZ/T2H: cks_div = 8192, clk ≈ 62.5 MHz -> max_hw_heartbeat_ms ~ 2147ms
 RZ/V2H: cks_div = 256, clk ≈ 240 MHz -> max_hw_heartbeat_ms ~ 174ms

Since min_timeout is in seconds, setting it to:
min_timeout = DIV_ROUND_UP(max_hw_heartbeat_ms, 1000);

ensures:
The minimum timeout period is never less than what the hardware can support.
- For T2H, this results in a min_timeout of 3s (2147ms -> 3s).
- For V2H, it’s just 1s (174ms -> 1s).

Cheers,
Prabhakar

On 8/1/25 04:05, Lad, Prabhakar wrote:
> Hi Wolfram,
> 
> Thank you for the review.
> 
> On Fri, Aug 1, 2025 at 5:10 AM Wolfram Sang
> <wsa+renesas@sang-engineering.com> wrote:
>>
>> On Tue, Jul 29, 2025 at 04:59:13PM +0100, Prabhakar wrote:
>>> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>>>
>>> Update the watchdog minimum timeout value to be derived from
>>> `max_hw_heartbeat_ms` using `DIV_ROUND_UP()` to ensure a valid and
>>> consistent minimum timeout in seconds.
>>
>> I don't understand this change. Why is the _minimum_ timeout based on
>> the _maximum_ heartbeat?
>>
> The reason for deriving min_timeout from max_hw_heartbeat_ms is to
> ensure the minimum watchdog period (in seconds) is compatible with the
> underlying hardware.
> 
> max_hw_heartbeat_ms is calculated as:
> max_hw_heartbeat_ms = (1000 * 16384 * cks_div) / clk_rate;
> 
> This value varies by SoC:
>   RZ/T2H: cks_div = 8192, clk ≈ 62.5 MHz -> max_hw_heartbeat_ms ~ 2147ms
>   RZ/V2H: cks_div = 256, clk ≈ 240 MHz -> max_hw_heartbeat_ms ~ 174ms
> 
> Since min_timeout is in seconds, setting it to:
> min_timeout = DIV_ROUND_UP(max_hw_heartbeat_ms, 1000);
> 
> ensures:
> The minimum timeout period is never less than what the hardware can support.
> - For T2H, this results in a min_timeout of 3s (2147ms -> 3s).
> - For V2H, it’s just 1s (174ms -> 1s).
> 

Sorry, I completely fail to understand the logic.

If the maximum timeout is, say, 2 seconds, why would the hardware
not be able to support a timeout of 1 second ?

Guenter

Hi Guenter,

On Fri, Aug 1, 2025 at 2:52 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 8/1/25 04:05, Lad, Prabhakar wrote:
> > Hi Wolfram,
> >
> > Thank you for the review.
> >
> > On Fri, Aug 1, 2025 at 5:10 AM Wolfram Sang
> > <wsa+renesas@sang-engineering.com> wrote:
> >>
> >> On Tue, Jul 29, 2025 at 04:59:13PM +0100, Prabhakar wrote:
> >>> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >>>
> >>> Update the watchdog minimum timeout value to be derived from
> >>> `max_hw_heartbeat_ms` using `DIV_ROUND_UP()` to ensure a valid and
> >>> consistent minimum timeout in seconds.
> >>
> >> I don't understand this change. Why is the _minimum_ timeout based on
> >> the _maximum_ heartbeat?
> >>
> > The reason for deriving min_timeout from max_hw_heartbeat_ms is to
> > ensure the minimum watchdog period (in seconds) is compatible with the
> > underlying hardware.
> >
> > max_hw_heartbeat_ms is calculated as:
> > max_hw_heartbeat_ms = (1000 * 16384 * cks_div) / clk_rate;
> >
> > This value varies by SoC:
> >   RZ/T2H: cks_div = 8192, clk ≈ 62.5 MHz -> max_hw_heartbeat_ms ~ 2147ms
> >   RZ/V2H: cks_div = 256, clk ≈ 240 MHz -> max_hw_heartbeat_ms ~ 174ms
> >
> > Since min_timeout is in seconds, setting it to:
> > min_timeout = DIV_ROUND_UP(max_hw_heartbeat_ms, 1000);
> >
> > ensures:
> > The minimum timeout period is never less than what the hardware can support.
> > - For T2H, this results in a min_timeout of 3s (2147ms -> 3s).
> > - For V2H, it’s just 1s (174ms -> 1s).
> >
>
> Sorry, I completely fail to understand the logic.
>
> If the maximum timeout is, say, 2 seconds, why would the hardware
> not be able to support a timeout of 1 second ?
>
The watchdog timer on RZ/V2H (and RZ/T2H) is a 14 bit down counter. On
initialization the down counters on the SoCs are configured to the max
down counter. On RZ/V2H down counter value 4194304 (which evaluates to
174ms) is and on RZ/T2H is 134217728 (which evaluates to 2147ms). The
board will be reset when we get an underflow error.

So for example on T2H consider this example:
- down counter is 134217728
- min_timeout is set to 1 in the driver
- When set  WDIOC_SETTIMEOUT to 1
In this case the board will be reset after 2147ms, i.e. incorrect
behaviour as we expect the board to be reset after 1 sec. Hence the
min_timeout is set to 3s (2147ms -> 3s).

Please let me know if my understanding of min_timeout is incorrect here.

Cheers,
Prabhakar

On 8/1/25 08:30, Lad, Prabhakar wrote:
> Hi Guenter,
> 
> On Fri, Aug 1, 2025 at 2:52 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On 8/1/25 04:05, Lad, Prabhakar wrote:
>>> Hi Wolfram,
>>>
>>> Thank you for the review.
>>>
>>> On Fri, Aug 1, 2025 at 5:10 AM Wolfram Sang
>>> <wsa+renesas@sang-engineering.com> wrote:
>>>>
>>>> On Tue, Jul 29, 2025 at 04:59:13PM +0100, Prabhakar wrote:
>>>>> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>>>>>
>>>>> Update the watchdog minimum timeout value to be derived from
>>>>> `max_hw_heartbeat_ms` using `DIV_ROUND_UP()` to ensure a valid and
>>>>> consistent minimum timeout in seconds.
>>>>
>>>> I don't understand this change. Why is the _minimum_ timeout based on
>>>> the _maximum_ heartbeat?
>>>>
>>> The reason for deriving min_timeout from max_hw_heartbeat_ms is to
>>> ensure the minimum watchdog period (in seconds) is compatible with the
>>> underlying hardware.
>>>
>>> max_hw_heartbeat_ms is calculated as:
>>> max_hw_heartbeat_ms = (1000 * 16384 * cks_div) / clk_rate;
>>>
>>> This value varies by SoC:
>>>    RZ/T2H: cks_div = 8192, clk ≈ 62.5 MHz -> max_hw_heartbeat_ms ~ 2147ms
>>>    RZ/V2H: cks_div = 256, clk ≈ 240 MHz -> max_hw_heartbeat_ms ~ 174ms
>>>
>>> Since min_timeout is in seconds, setting it to:
>>> min_timeout = DIV_ROUND_UP(max_hw_heartbeat_ms, 1000);
>>>
>>> ensures:
>>> The minimum timeout period is never less than what the hardware can support.
>>> - For T2H, this results in a min_timeout of 3s (2147ms -> 3s).
>>> - For V2H, it’s just 1s (174ms -> 1s).
>>>
>>
>> Sorry, I completely fail to understand the logic.
>>
>> If the maximum timeout is, say, 2 seconds, why would the hardware
>> not be able to support a timeout of 1 second ?
>>
> The watchdog timer on RZ/V2H (and RZ/T2H) is a 14 bit down counter. On
> initialization the down counters on the SoCs are configured to the max
> down counter. On RZ/V2H down counter value 4194304 (which evaluates to
> 174ms) is and on RZ/T2H is 134217728 (which evaluates to 2147ms). The
> board will be reset when we get an underflow error.
> 
> So for example on T2H consider this example:
> - down counter is 134217728
> - min_timeout is set to 1 in the driver
> - When set  WDIOC_SETTIMEOUT to 1
> In this case the board will be reset after 2147ms, i.e. incorrect
> behaviour as we expect the board to be reset after 1 sec. Hence the
> min_timeout is set to 3s (2147ms -> 3s).
> 
> Please let me know if my understanding of min_timeout is incorrect here.
> 

The driver is missing a set_timeout function. It should set RZ/T2H
to 62514079 if a timeout of 1 second is configured.

Guenter

Hi Guenter,

On Fri, Aug 1, 2025 at 7:04 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 8/1/25 08:30, Lad, Prabhakar wrote:
> > Hi Guenter,
> >
> > On Fri, Aug 1, 2025 at 2:52 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >>
> >> On 8/1/25 04:05, Lad, Prabhakar wrote:
> >>> Hi Wolfram,
> >>>
> >>> Thank you for the review.
> >>>
> >>> On Fri, Aug 1, 2025 at 5:10 AM Wolfram Sang
> >>> <wsa+renesas@sang-engineering.com> wrote:
> >>>>
> >>>> On Tue, Jul 29, 2025 at 04:59:13PM +0100, Prabhakar wrote:
> >>>>> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >>>>>
> >>>>> Update the watchdog minimum timeout value to be derived from
> >>>>> `max_hw_heartbeat_ms` using `DIV_ROUND_UP()` to ensure a valid and
> >>>>> consistent minimum timeout in seconds.
> >>>>
> >>>> I don't understand this change. Why is the _minimum_ timeout based on
> >>>> the _maximum_ heartbeat?
> >>>>
> >>> The reason for deriving min_timeout from max_hw_heartbeat_ms is to
> >>> ensure the minimum watchdog period (in seconds) is compatible with the
> >>> underlying hardware.
> >>>
> >>> max_hw_heartbeat_ms is calculated as:
> >>> max_hw_heartbeat_ms = (1000 * 16384 * cks_div) / clk_rate;
> >>>
> >>> This value varies by SoC:
> >>>    RZ/T2H: cks_div = 8192, clk ≈ 62.5 MHz -> max_hw_heartbeat_ms ~ 2147ms
> >>>    RZ/V2H: cks_div = 256, clk ≈ 240 MHz -> max_hw_heartbeat_ms ~ 174ms
> >>>
> >>> Since min_timeout is in seconds, setting it to:
> >>> min_timeout = DIV_ROUND_UP(max_hw_heartbeat_ms, 1000);
> >>>
> >>> ensures:
> >>> The minimum timeout period is never less than what the hardware can support.
> >>> - For T2H, this results in a min_timeout of 3s (2147ms -> 3s).
> >>> - For V2H, it’s just 1s (174ms -> 1s).
> >>>
> >>
> >> Sorry, I completely fail to understand the logic.
> >>
> >> If the maximum timeout is, say, 2 seconds, why would the hardware
> >> not be able to support a timeout of 1 second ?
> >>
> > The watchdog timer on RZ/V2H (and RZ/T2H) is a 14 bit down counter. On
> > initialization the down counters on the SoCs are configured to the max
> > down counter. On RZ/V2H down counter value 4194304 (which evaluates to
> > 174ms) is and on RZ/T2H is 134217728 (which evaluates to 2147ms). The
> > board will be reset when we get an underflow error.
> >
> > So for example on T2H consider this example:
> > - down counter is 134217728
> > - min_timeout is set to 1 in the driver
> > - When set  WDIOC_SETTIMEOUT to 1
> > In this case the board will be reset after 2147ms, i.e. incorrect
> > behaviour as we expect the board to be reset after 1 sec. Hence the
> > min_timeout is set to 3s (2147ms -> 3s).
> >
> > Please let me know if my understanding of min_timeout is incorrect here.
> >
>
> The driver is missing a set_timeout function. It should set RZ/T2H
> to 62514079 if a timeout of 1 second is configured.
>
Ok, you mean to handle the 1sec case, introduce the set_timeout for RZ/T2H SoC.

Although we cannot achieve the exact 1sec case as we can have only 4
timeout period options (number of cycles):

1] For TIMEOUT_CYCLES = 1024
 - (1000×1024×8192)/62500000 = 134.22 ms
2] For TIMEOUT_CYCLES = 4096
- (1000×4096×8192)/62500000 = 536.87 ms
3] For TIMEOUT_CYCLES = 8192
- (1000×8192×8192)/62500000 = 1,073.74 ms
4] For TIMEOUT_CYCLES = 16384
- (1000×16384×8192)/62500000 = 2,147.48 ms

So to handle the 1sec case I'll set the timeout period to 8192 with
which we get a timeout of 1,073.74 ms.

Cheers,
Prabhakar

On 8/1/25 13:51, Lad, Prabhakar wrote:
> Hi Guenter,
> 
> On Fri, Aug 1, 2025 at 7:04 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On 8/1/25 08:30, Lad, Prabhakar wrote:
>>> Hi Guenter,
>>>
>>> On Fri, Aug 1, 2025 at 2:52 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>>>
>>>> On 8/1/25 04:05, Lad, Prabhakar wrote:
>>>>> Hi Wolfram,
>>>>>
>>>>> Thank you for the review.
>>>>>
>>>>> On Fri, Aug 1, 2025 at 5:10 AM Wolfram Sang
>>>>> <wsa+renesas@sang-engineering.com> wrote:
>>>>>>
>>>>>> On Tue, Jul 29, 2025 at 04:59:13PM +0100, Prabhakar wrote:
>>>>>>> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>>>>>>>
>>>>>>> Update the watchdog minimum timeout value to be derived from
>>>>>>> `max_hw_heartbeat_ms` using `DIV_ROUND_UP()` to ensure a valid and
>>>>>>> consistent minimum timeout in seconds.
>>>>>>
>>>>>> I don't understand this change. Why is the _minimum_ timeout based on
>>>>>> the _maximum_ heartbeat?
>>>>>>
>>>>> The reason for deriving min_timeout from max_hw_heartbeat_ms is to
>>>>> ensure the minimum watchdog period (in seconds) is compatible with the
>>>>> underlying hardware.
>>>>>
>>>>> max_hw_heartbeat_ms is calculated as:
>>>>> max_hw_heartbeat_ms = (1000 * 16384 * cks_div) / clk_rate;
>>>>>
>>>>> This value varies by SoC:
>>>>>     RZ/T2H: cks_div = 8192, clk ≈ 62.5 MHz -> max_hw_heartbeat_ms ~ 2147ms
>>>>>     RZ/V2H: cks_div = 256, clk ≈ 240 MHz -> max_hw_heartbeat_ms ~ 174ms
>>>>>
>>>>> Since min_timeout is in seconds, setting it to:
>>>>> min_timeout = DIV_ROUND_UP(max_hw_heartbeat_ms, 1000);
>>>>>
>>>>> ensures:
>>>>> The minimum timeout period is never less than what the hardware can support.
>>>>> - For T2H, this results in a min_timeout of 3s (2147ms -> 3s).
>>>>> - For V2H, it’s just 1s (174ms -> 1s).
>>>>>
>>>>
>>>> Sorry, I completely fail to understand the logic.
>>>>
>>>> If the maximum timeout is, say, 2 seconds, why would the hardware
>>>> not be able to support a timeout of 1 second ?
>>>>
>>> The watchdog timer on RZ/V2H (and RZ/T2H) is a 14 bit down counter. On
>>> initialization the down counters on the SoCs are configured to the max
>>> down counter. On RZ/V2H down counter value 4194304 (which evaluates to
>>> 174ms) is and on RZ/T2H is 134217728 (which evaluates to 2147ms). The
>>> board will be reset when we get an underflow error.
>>>
>>> So for example on T2H consider this example:
>>> - down counter is 134217728
>>> - min_timeout is set to 1 in the driver
>>> - When set  WDIOC_SETTIMEOUT to 1
>>> In this case the board will be reset after 2147ms, i.e. incorrect
>>> behaviour as we expect the board to be reset after 1 sec. Hence the
>>> min_timeout is set to 3s (2147ms -> 3s).
>>>
>>> Please let me know if my understanding of min_timeout is incorrect here.
>>>
>>
>> The driver is missing a set_timeout function. It should set RZ/T2H
>> to 62514079 if a timeout of 1 second is configured.
>>
> Ok, you mean to handle the 1sec case, introduce the set_timeout for RZ/T2H SoC.
> 
> Although we cannot achieve the exact 1sec case as we can have only 4
> timeout period options (number of cycles):
> 
> 1] For TIMEOUT_CYCLES = 1024
>   - (1000×1024×8192)/62500000 = 134.22 ms
> 2] For TIMEOUT_CYCLES = 4096
> - (1000×4096×8192)/62500000 = 536.87 ms
> 3] For TIMEOUT_CYCLES = 8192
> - (1000×8192×8192)/62500000 = 1,073.74 ms
> 4] For TIMEOUT_CYCLES = 16384
> - (1000×16384×8192)/62500000 = 2,147.48 ms
> 
> So to handle the 1sec case I'll set the timeout period to 8192 with
> which we get a timeout of 1,073.74 ms.
> 

Just four possible values to set the hardware timeout ? That is an odd
hardware. In that case, you could also set the period to 1024 or 4096
and set max_hw_heartbeat_ms accordingly. That would avoid the rounding
error.

Guenter

Hi Guenter,

On Fri, Aug 1, 2025 at 10:04 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 8/1/25 13:51, Lad, Prabhakar wrote:
> > Hi Guenter,
> >
> > On Fri, Aug 1, 2025 at 7:04 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >>
> >> On 8/1/25 08:30, Lad, Prabhakar wrote:
> >>> Hi Guenter,
> >>>
> >>> On Fri, Aug 1, 2025 at 2:52 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >>>>
> >>>> On 8/1/25 04:05, Lad, Prabhakar wrote:
> >>>>> Hi Wolfram,
> >>>>>
> >>>>> Thank you for the review.
> >>>>>
> >>>>> On Fri, Aug 1, 2025 at 5:10 AM Wolfram Sang
> >>>>> <wsa+renesas@sang-engineering.com> wrote:
> >>>>>>
> >>>>>> On Tue, Jul 29, 2025 at 04:59:13PM +0100, Prabhakar wrote:
> >>>>>>> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >>>>>>>
> >>>>>>> Update the watchdog minimum timeout value to be derived from
> >>>>>>> `max_hw_heartbeat_ms` using `DIV_ROUND_UP()` to ensure a valid and
> >>>>>>> consistent minimum timeout in seconds.
> >>>>>>
> >>>>>> I don't understand this change. Why is the _minimum_ timeout based on
> >>>>>> the _maximum_ heartbeat?
> >>>>>>
> >>>>> The reason for deriving min_timeout from max_hw_heartbeat_ms is to
> >>>>> ensure the minimum watchdog period (in seconds) is compatible with the
> >>>>> underlying hardware.
> >>>>>
> >>>>> max_hw_heartbeat_ms is calculated as:
> >>>>> max_hw_heartbeat_ms = (1000 * 16384 * cks_div) / clk_rate;
> >>>>>
> >>>>> This value varies by SoC:
> >>>>>     RZ/T2H: cks_div = 8192, clk ≈ 62.5 MHz -> max_hw_heartbeat_ms ~ 2147ms
> >>>>>     RZ/V2H: cks_div = 256, clk ≈ 240 MHz -> max_hw_heartbeat_ms ~ 174ms
> >>>>>
> >>>>> Since min_timeout is in seconds, setting it to:
> >>>>> min_timeout = DIV_ROUND_UP(max_hw_heartbeat_ms, 1000);
> >>>>>
> >>>>> ensures:
> >>>>> The minimum timeout period is never less than what the hardware can support.
> >>>>> - For T2H, this results in a min_timeout of 3s (2147ms -> 3s).
> >>>>> - For V2H, it’s just 1s (174ms -> 1s).
> >>>>>
> >>>>
> >>>> Sorry, I completely fail to understand the logic.
> >>>>
> >>>> If the maximum timeout is, say, 2 seconds, why would the hardware
> >>>> not be able to support a timeout of 1 second ?
> >>>>
> >>> The watchdog timer on RZ/V2H (and RZ/T2H) is a 14 bit down counter. On
> >>> initialization the down counters on the SoCs are configured to the max
> >>> down counter. On RZ/V2H down counter value 4194304 (which evaluates to
> >>> 174ms) is and on RZ/T2H is 134217728 (which evaluates to 2147ms). The
> >>> board will be reset when we get an underflow error.
> >>>
> >>> So for example on T2H consider this example:
> >>> - down counter is 134217728
> >>> - min_timeout is set to 1 in the driver
> >>> - When set  WDIOC_SETTIMEOUT to 1
> >>> In this case the board will be reset after 2147ms, i.e. incorrect
> >>> behaviour as we expect the board to be reset after 1 sec. Hence the
> >>> min_timeout is set to 3s (2147ms -> 3s).
> >>>
> >>> Please let me know if my understanding of min_timeout is incorrect here.
> >>>
> >>
> >> The driver is missing a set_timeout function. It should set RZ/T2H
> >> to 62514079 if a timeout of 1 second is configured.
> >>
> > Ok, you mean to handle the 1sec case, introduce the set_timeout for RZ/T2H SoC.
> >
> > Although we cannot achieve the exact 1sec case as we can have only 4
> > timeout period options (number of cycles):
> >
> > 1] For TIMEOUT_CYCLES = 1024
> >   - (1000×1024×8192)/62500000 = 134.22 ms
> > 2] For TIMEOUT_CYCLES = 4096
> > - (1000×4096×8192)/62500000 = 536.87 ms
> > 3] For TIMEOUT_CYCLES = 8192
> > - (1000×8192×8192)/62500000 = 1,073.74 ms
> > 4] For TIMEOUT_CYCLES = 16384
> > - (1000×16384×8192)/62500000 = 2,147.48 ms
> >
> > So to handle the 1sec case I'll set the timeout period to 8192 with
> > which we get a timeout of 1,073.74 ms.
> >
>
> Just four possible values to set the hardware timeout ? That is an odd
> hardware. In that case, you could also set the period to 1024 or 4096
> and set max_hw_heartbeat_ms accordingly. That would avoid the rounding
> error.
>
Yes sadly we have four timeout periods only. To clarify, you mean to
set `max_hw_heartbeat_ms` in set_timeout?

Cheers,
Prabhakar

On 8/2/25 12:26, Lad, Prabhakar wrote:
> Hi Guenter,
> 
> On Fri, Aug 1, 2025 at 10:04 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On 8/1/25 13:51, Lad, Prabhakar wrote:
>>> Hi Guenter,
>>>
>>> On Fri, Aug 1, 2025 at 7:04 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>>>
>>>> On 8/1/25 08:30, Lad, Prabhakar wrote:
>>>>> Hi Guenter,
>>>>>
>>>>> On Fri, Aug 1, 2025 at 2:52 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>>>>>
>>>>>> On 8/1/25 04:05, Lad, Prabhakar wrote:
>>>>>>> Hi Wolfram,
>>>>>>>
>>>>>>> Thank you for the review.
>>>>>>>
>>>>>>> On Fri, Aug 1, 2025 at 5:10 AM Wolfram Sang
>>>>>>> <wsa+renesas@sang-engineering.com> wrote:
>>>>>>>>
>>>>>>>> On Tue, Jul 29, 2025 at 04:59:13PM +0100, Prabhakar wrote:
>>>>>>>>> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>>>>>>>>>
>>>>>>>>> Update the watchdog minimum timeout value to be derived from
>>>>>>>>> `max_hw_heartbeat_ms` using `DIV_ROUND_UP()` to ensure a valid and
>>>>>>>>> consistent minimum timeout in seconds.
>>>>>>>>
>>>>>>>> I don't understand this change. Why is the _minimum_ timeout based on
>>>>>>>> the _maximum_ heartbeat?
>>>>>>>>
>>>>>>> The reason for deriving min_timeout from max_hw_heartbeat_ms is to
>>>>>>> ensure the minimum watchdog period (in seconds) is compatible with the
>>>>>>> underlying hardware.
>>>>>>>
>>>>>>> max_hw_heartbeat_ms is calculated as:
>>>>>>> max_hw_heartbeat_ms = (1000 * 16384 * cks_div) / clk_rate;
>>>>>>>
>>>>>>> This value varies by SoC:
>>>>>>>      RZ/T2H: cks_div = 8192, clk ≈ 62.5 MHz -> max_hw_heartbeat_ms ~ 2147ms
>>>>>>>      RZ/V2H: cks_div = 256, clk ≈ 240 MHz -> max_hw_heartbeat_ms ~ 174ms
>>>>>>>
>>>>>>> Since min_timeout is in seconds, setting it to:
>>>>>>> min_timeout = DIV_ROUND_UP(max_hw_heartbeat_ms, 1000);
>>>>>>>
>>>>>>> ensures:
>>>>>>> The minimum timeout period is never less than what the hardware can support.
>>>>>>> - For T2H, this results in a min_timeout of 3s (2147ms -> 3s).
>>>>>>> - For V2H, it’s just 1s (174ms -> 1s).
>>>>>>>
>>>>>>
>>>>>> Sorry, I completely fail to understand the logic.
>>>>>>
>>>>>> If the maximum timeout is, say, 2 seconds, why would the hardware
>>>>>> not be able to support a timeout of 1 second ?
>>>>>>
>>>>> The watchdog timer on RZ/V2H (and RZ/T2H) is a 14 bit down counter. On
>>>>> initialization the down counters on the SoCs are configured to the max
>>>>> down counter. On RZ/V2H down counter value 4194304 (which evaluates to
>>>>> 174ms) is and on RZ/T2H is 134217728 (which evaluates to 2147ms). The
>>>>> board will be reset when we get an underflow error.
>>>>>
>>>>> So for example on T2H consider this example:
>>>>> - down counter is 134217728
>>>>> - min_timeout is set to 1 in the driver
>>>>> - When set  WDIOC_SETTIMEOUT to 1
>>>>> In this case the board will be reset after 2147ms, i.e. incorrect
>>>>> behaviour as we expect the board to be reset after 1 sec. Hence the
>>>>> min_timeout is set to 3s (2147ms -> 3s).
>>>>>
>>>>> Please let me know if my understanding of min_timeout is incorrect here.
>>>>>
>>>>
>>>> The driver is missing a set_timeout function. It should set RZ/T2H
>>>> to 62514079 if a timeout of 1 second is configured.
>>>>
>>> Ok, you mean to handle the 1sec case, introduce the set_timeout for RZ/T2H SoC.
>>>
>>> Although we cannot achieve the exact 1sec case as we can have only 4
>>> timeout period options (number of cycles):
>>>
>>> 1] For TIMEOUT_CYCLES = 1024
>>>    - (1000×1024×8192)/62500000 = 134.22 ms
>>> 2] For TIMEOUT_CYCLES = 4096
>>> - (1000×4096×8192)/62500000 = 536.87 ms
>>> 3] For TIMEOUT_CYCLES = 8192
>>> - (1000×8192×8192)/62500000 = 1,073.74 ms
>>> 4] For TIMEOUT_CYCLES = 16384
>>> - (1000×16384×8192)/62500000 = 2,147.48 ms
>>>
>>> So to handle the 1sec case I'll set the timeout period to 8192 with
>>> which we get a timeout of 1,073.74 ms.
>>>
>>
>> Just four possible values to set the hardware timeout ? That is an odd
>> hardware. In that case, you could also set the period to 1024 or 4096
>> and set max_hw_heartbeat_ms accordingly. That would avoid the rounding
>> error.
>>
> Yes sadly we have four timeout periods only. To clarify, you mean to
> set `max_hw_heartbeat_ms` in set_timeout?
> 

No, during initialization, and have no set_timeout function. max_hw_heartbeat_ms
is not supposed to change during runtime. If you do change it, the results
are undefined.

Guenter

Hi Guenter,

On Sun, Aug 3, 2025 at 1:16 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 8/2/25 12:26, Lad, Prabhakar wrote:
> > Hi Guenter,
> >
> > On Fri, Aug 1, 2025 at 10:04 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >>
> >> On 8/1/25 13:51, Lad, Prabhakar wrote:
> >>> Hi Guenter,
> >>>
> >>> On Fri, Aug 1, 2025 at 7:04 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >>>>
> >>>> On 8/1/25 08:30, Lad, Prabhakar wrote:
> >>>>> Hi Guenter,
> >>>>>
> >>>>> On Fri, Aug 1, 2025 at 2:52 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >>>>>>
> >>>>>> On 8/1/25 04:05, Lad, Prabhakar wrote:
> >>>>>>> Hi Wolfram,
> >>>>>>>
> >>>>>>> Thank you for the review.
> >>>>>>>
> >>>>>>> On Fri, Aug 1, 2025 at 5:10 AM Wolfram Sang
> >>>>>>> <wsa+renesas@sang-engineering.com> wrote:
> >>>>>>>>
> >>>>>>>> On Tue, Jul 29, 2025 at 04:59:13PM +0100, Prabhakar wrote:
> >>>>>>>>> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >>>>>>>>>
> >>>>>>>>> Update the watchdog minimum timeout value to be derived from
> >>>>>>>>> `max_hw_heartbeat_ms` using `DIV_ROUND_UP()` to ensure a valid and
> >>>>>>>>> consistent minimum timeout in seconds.
> >>>>>>>>
> >>>>>>>> I don't understand this change. Why is the _minimum_ timeout based on
> >>>>>>>> the _maximum_ heartbeat?
> >>>>>>>>
> >>>>>>> The reason for deriving min_timeout from max_hw_heartbeat_ms is to
> >>>>>>> ensure the minimum watchdog period (in seconds) is compatible with the
> >>>>>>> underlying hardware.
> >>>>>>>
> >>>>>>> max_hw_heartbeat_ms is calculated as:
> >>>>>>> max_hw_heartbeat_ms = (1000 * 16384 * cks_div) / clk_rate;
> >>>>>>>
> >>>>>>> This value varies by SoC:
> >>>>>>>      RZ/T2H: cks_div = 8192, clk ≈ 62.5 MHz -> max_hw_heartbeat_ms ~ 2147ms
> >>>>>>>      RZ/V2H: cks_div = 256, clk ≈ 240 MHz -> max_hw_heartbeat_ms ~ 174ms
> >>>>>>>
> >>>>>>> Since min_timeout is in seconds, setting it to:
> >>>>>>> min_timeout = DIV_ROUND_UP(max_hw_heartbeat_ms, 1000);
> >>>>>>>
> >>>>>>> ensures:
> >>>>>>> The minimum timeout period is never less than what the hardware can support.
> >>>>>>> - For T2H, this results in a min_timeout of 3s (2147ms -> 3s).
> >>>>>>> - For V2H, it’s just 1s (174ms -> 1s).
> >>>>>>>
> >>>>>>
> >>>>>> Sorry, I completely fail to understand the logic.
> >>>>>>
> >>>>>> If the maximum timeout is, say, 2 seconds, why would the hardware
> >>>>>> not be able to support a timeout of 1 second ?
> >>>>>>
> >>>>> The watchdog timer on RZ/V2H (and RZ/T2H) is a 14 bit down counter. On
> >>>>> initialization the down counters on the SoCs are configured to the max
> >>>>> down counter. On RZ/V2H down counter value 4194304 (which evaluates to
> >>>>> 174ms) is and on RZ/T2H is 134217728 (which evaluates to 2147ms). The
> >>>>> board will be reset when we get an underflow error.
> >>>>>
> >>>>> So for example on T2H consider this example:
> >>>>> - down counter is 134217728
> >>>>> - min_timeout is set to 1 in the driver
> >>>>> - When set  WDIOC_SETTIMEOUT to 1
> >>>>> In this case the board will be reset after 2147ms, i.e. incorrect
> >>>>> behaviour as we expect the board to be reset after 1 sec. Hence the
> >>>>> min_timeout is set to 3s (2147ms -> 3s).
> >>>>>
> >>>>> Please let me know if my understanding of min_timeout is incorrect here.
> >>>>>
> >>>>
> >>>> The driver is missing a set_timeout function. It should set RZ/T2H
> >>>> to 62514079 if a timeout of 1 second is configured.
> >>>>
> >>> Ok, you mean to handle the 1sec case, introduce the set_timeout for RZ/T2H SoC.
> >>>
> >>> Although we cannot achieve the exact 1sec case as we can have only 4
> >>> timeout period options (number of cycles):
> >>>
> >>> 1] For TIMEOUT_CYCLES = 1024
> >>>    - (1000×1024×8192)/62500000 = 134.22 ms
> >>> 2] For TIMEOUT_CYCLES = 4096
> >>> - (1000×4096×8192)/62500000 = 536.87 ms
> >>> 3] For TIMEOUT_CYCLES = 8192
> >>> - (1000×8192×8192)/62500000 = 1,073.74 ms
> >>> 4] For TIMEOUT_CYCLES = 16384
> >>> - (1000×16384×8192)/62500000 = 2,147.48 ms
> >>>
> >>> So to handle the 1sec case I'll set the timeout period to 8192 with
> >>> which we get a timeout of 1,073.74 ms.
> >>>
> >>
> >> Just four possible values to set the hardware timeout ? That is an odd
> >> hardware. In that case, you could also set the period to 1024 or 4096
> >> and set max_hw_heartbeat_ms accordingly. That would avoid the rounding
> >> error.
> >>
> > Yes sadly we have four timeout periods only. To clarify, you mean to
> > set `max_hw_heartbeat_ms` in set_timeout?
> >
>
> No, during initialization, and have no set_timeout function. max_hw_heartbeat_ms
> is not supposed to change during runtime. If you do change it, the results
> are undefined.
>
Thank you for the clarification. Ive done the changes as suggested. I
will send a v3 soon.

Cheers,
Prabhakar