Hi Shmuel,
On jeu., juin 05, 2025 at 06:33, Shmuel Melamud <smela...@redhat.com> wrote:
> On Wed, Jun 4, 2025 at 12:02 PM Mattijs Korpershoek
> <mkorpersh...@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....@kernel.org> wrote:
>>
>> > From: Shmuel Leib Melamud <smela...@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 <mkorpersh...@kernel.org>
Thanks,
Mattijs
>
>> > +
