Hi Chris,

On Mon, Feb 13, 2017 at 7:25 PM, Chris Brandt <chris.bra...@renesas.com> wrote:
> Some Renesas SoCs do not have a reset register and the only way to do a SW
> controlled reset is to use the watchdog timer. Additionally, since all the
> WDT timeout options are so quick, a system reset is about the only thing
> it's good for.
>
> Signed-off-by: Chris Brandt <chris.bra...@renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+rene...@glider.be>

> --- /dev/null
> +++ b/drivers/power/reset/renesas-reset.c
> @@ -0,0 +1,103 @@

> +/* Watchdog Timer Registers */
> +#define WTCSR 0
> +#define WTCNT 2
> +#define WRCSR 4
> +
> +static void __iomem *base;
> +
> +static int wdt_reset_handler(struct notifier_block *this,
> +                            unsigned long mode, void *cmd)
> +{
> +       pr_debug("%s %lu\n", __func__, mode);
> +
> +       /* Dummy read (must read WRCSR:WOVF at least once before clearing) */
> +       readw(base + WRCSR);
> +
> +       writew(0xA500, base + WRCSR);   /* Clear WOVF */
> +       writew(0x5A5F, base + WRCSR);   /* Reset Enable */
> +       writew(0x5A00, base + WTCNT);   /* Counter to 00 */
> +       writew(0xA578, base + WTCSR);   /* Start timer */

Hardcoded register values? Please use #defines.
Yes, the 0xa5 and 0x5a are magic values, but the other bits aren't.

> +
> +       /* Wait for WDT overflow */
> +       while (1)
> +               ;

This burns CPU, and thus power, albeit for a very short time.
Perhaps add an msleep() to the loop body?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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

Reply via email to