Hi Geert,
Thanks for the feedback.
> Subject: Re: [PATCH v3 2/2] watchdog: Add Watchdog Timer driver for RZ/G2L
>
> Hi Biju,
>
> On Tue, Nov 30, 2021 at 10:59 AM Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> wrote:
> > > Subject: Re: [PATCH v3 2/2] watchdog: Add Watchdog Timer driver for
> > > RZ/G2L On 11/22/21 3:35 AM, Biju Das wrote:
> > > > Add Watchdog Timer driver for RZ/G2L SoC.
> > > >
> > > > WDT IP block supports normal watchdog timer function and reset
> > > > request function due to CPU parity error.
> > > >
> > > > This driver currently supports normal watchdog timer function and
> > > > later will add support for reset request function due to CPU
> > > > parity error.
> > > >
> > > > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > > > Reviewed-by: Guenter Roeck <linux@xxxxxxxxxxxx>
>
> > > > --- /dev/null
> > > > +++ b/drivers/watchdog/rzg2l_wdt.c
> > > > @@ -0,0 +1,255 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Renesas RZ/G2L WDT Watchdog Driver
> > > > + *
> > > > + * Copyright (C) 2021 Renesas Electronics Corporation */
> > > > +#include <linux/bitops.h> #include <linux/clk.h> #include
> > > > +<linux/delay.h> #include <linux/io.h> #include <linux/kernel.h>
> > > > +#include <linux/module.h> #include <linux/of.h> #include
> > > > +<linux/platform_device.h> #include <linux/pm_runtime.h> #include
> > > > +<linux/reset.h> #include <linux/watchdog.h>
> > > > +
> > > > +#define WDTCNT 0x00
> > > > +#define WDTSET 0x04
> > > > +#define WDTTIM 0x08
> > > > +#define WDTINT 0x0C
> > > > +#define WDTCNT_WDTEN BIT(0)
> > > > +#define WDTINT_INTDISP BIT(0)
> > > > +
> > > > +#define WDT_DEFAULT_TIMEOUT 60U
> > > > +
> > > > +/* Setting period time register only 12 bit set in WDTSET[31:20] */
> > > > +#define WDTSET_COUNTER_MASK (0xFFF00000)
> > > > +#define WDTSET_COUNTER_VAL(f) ((f) << 20)
> > > > +
> > > > +#define F2CYCLE_NSEC(f) (1000000000 / (f))
> > > > +#define WDT_CYCLE_MSEC(f, wdttime) ((1024 * 1024 *
> > > > +(((u64)wdttime)
> > > + 1)) / \
> > > > + ((f) / 1000000))
> > >
> > > This macro generates a 64 bit divide operation - as noticed by 0-day
> > > - and will have to be rewritten.
> > >
> >
> > OK, will rewrite like below to make the ARCH=m68k compiler happy
> >
> > -#define F2CYCLE_NSEC(f) (1000000000 / (f))
> > -#define WDT_CYCLE_MSEC(f, wdttime) ((1024 * 1024 * (((u64)wdttime)
> + 1)) / \
> > - ((f) / 1000000))
> > +#define F2CYCLE_NSEC(f) (1000000000 / (f))
> > +#define WDT_CYCLE_MSEC_CONST (1024 * 1024 * (u64)1000000)
>
> 1000000ULL (casts are evil ;-)
OK will get rid of this macro as you suggested below.
>
> >
> > +static u32 rzg2l_wdt_get_cycle_msec(u32 cycle, u32 wdttime)
>
> cycle == priv->osc_clk_rate, i.e. unsigned long?
OK will make unsigned long.
>
> > +{
> > + /* timer_cycle = clk_cycle *1024 *1024 * (WDTTIME + 1) */
>
> Comment not needed if you get rid of WDT_CYCLE_MSEC_CONST?
Agreed.
>
> > + u64 timer_cycle_ms = WDT_CYCLE_MSEC_CONST * (wdttime + 1);
> > + do_div(timer_cycle_ms, cycle);
>
> div64_ul()?
OK, will do since it is 64 bit by 64 bit division.
Regards,
Biju
>
> > +
> > + return timer_cycle_ms;
> > +}
>
> 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
