Re: [PATCH v3 2/2] renesas: Renesas R-Car Gen4 watchdog driver

[Mattijs Korpershoek <mkorpershoek@kernel.org>]

Hi Shmuel,

On jeu., juin 05, 2025 at 06:33, Shmuel Melamud <smelamud@redhat.com> wrote:

> On Wed, Jun 4, 2025 at 12:02 PM Mattijs Korpershoek
> <mkorpershoek@kernel.org> wrote:
>>
>> Hi Shmuel,
>>
>> Thank you for the patch.
>>
>> On Tue, Jun 03, 2025 at 06:06, Shmuel Leib Melamud via B4 Relay <devnull+smelamud.redhat.com@kernel.org> wrote:
>>
>> > From: Shmuel Leib Melamud <smelamud@redhat.com>

[...]

>> >
>> > +
>> > +static int rwdt_start(struct udevice *dev, u64 timeout, ulong flags)
>> > +{
>> > +     struct rwdt_priv *priv = dev_get_priv(dev);
>> > +     u64 max_timeout;
>> > +     u8 val;
>> > +
>> > +     max_timeout = DIV_BY_CLKS_PER_SEC(priv, 65536);
>>
>> Why 65536. This number is used here ...
>
> 65536 here is the maximal value of the counter plus 1. If we divide it
> by the number of impulses per second, we get the maximal timeout in
> seconds.
>
>> > +     timeout = min(max_timeout, timeout / 1000);
>> > +
>> > +     /* Stop the timer before we modify any register */
>> > +     val = readb_relaxed(&priv->wdt->csra) & ~RWTCSRA_TME;
>> > +     writel_relaxed(val | CSR_MASK, &priv->wdt->csra);
>> > +     /* Delay 2 cycles before setting watchdog counter */
>> > +     rwdt_wait_cycles(priv, 2);
>> > +
>> > +     while (readb_relaxed(&priv->wdt->csra) & RWTCSRA_WRFLG)
>> > +             cpu_relax();
>> > +
>> > +     writel_relaxed((65536 - MUL_BY_CLKS_PER_SEC(priv, timeout)) | CNT_MASK,
>> > +                    &priv->wdt->cnt);
>>
>> ... and here. Can we move it to a define to document its meaning?
>
> 65536 here is for negation. Since the counter is incremented on every
> impulse, the timeout is the number of impulses that are left till
> overflow of the counter.
>
> In both cases it is the maximal value of the counter, so maybe it
> makes sense to add a define for it.

Yes, please move this to a #define. With this constant moved to a
#define, you can add the following to v4 patch:

Reviewed-by: Mattijs Korpershoek <mkorpershoek@kernel.org>

Thanks,
Mattijs

>
>> > +