Re: [PATCH] watchdog: renesas_wdt: Add a few cycles delay

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Shimoda-san,

On Mon, Jun 3, 2019 at 12:28 PM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> > From: Geert Uytterhoeven, Sent: Monday, June 3, 2019 6:59 PM
> > On Mon, Jun 3, 2019 at 11:31 AM Yoshihiro Shimoda
> > <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > According to the hardware manual of R-Car Gen2 and Gen3,
> > > software should wait a few RLCK cycles as following:
> > >  - Delay 2 cycles before setting watchdog counter.
> > >  - Delay 3 cycles before disabling module clock.
> > >
> > > So, this patch adds such delays.
> > >
> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> >
> > Thanks for your patch!
>
> Thank you for your review!
>
> > > --- a/drivers/watchdog/renesas_wdt.c
> > > +++ b/drivers/watchdog/renesas_wdt.c
> >
> > > @@ -70,6 +71,16 @@ static int rwdt_init_timeout(struct watchdog_device *wdev)
> > >         return 0;
> > >  }
> > >
> > > +static void rwdt_wait(struct rwdt_priv *priv, unsigned long cycles)
> >
> > "unsigned int" should be sufficiently large.
>
> I got it.
>
> > > +{
> > > +       unsigned long periods, delays;
> > > +
> > > +       periods = DIV_ROUND_UP(priv->clk_rate, cycles);
> >
> > Shouldn't the above be a division with rounding down (i.e. a plain C
> > division), instead of a division with rounding up?
>
> I have no idea which is the correct way (rounding down vs rounding up here).
> At least, I tried to use rounding down before submitting patch and then
> the result seemed the same. So, I submitted this patch with rounding up
> (because the next step also used rounding up...).

If you round up periods, it will decrease the delay, which may become
too small.
If you round up delays, it will increase the delay, which doesn't hurt.

> > > +       delays = DIV_ROUND_UP(1000000UL, periods);
> >
> > Given cycles is always a small number, accuracy can be improved, and one
> > division can be avoided, by calculation this as:
> >
> >     delays = DIV_ROUND_UP(cycles * 1000000 /  priv->clk_rate);
>
> Thank you for your suggest! I think so.
> It should be "s/ \//,/" like below though :)

>         delays = DIV_ROUND_UP(cycles * 1000000, priv->clk_rate);

Oops, indeed. Sorry for that silly mistake.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds