[PATCH] ARM: shmobile: r7s72100: add restart handler

[PATCH] ARM: shmobile: r7s72100: add restart handler

[Chris Brandt]

Add a simple restart handler which enables the watchdog timer with the
device reset option enabled. This is the only way SW can cause a reset on
this SoC.

If someone has a board that needs more specific operations to be done
first, they can override this function in another file.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
 arch/arm/mach-shmobile/setup-r7s72100.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/arch/arm/mach-shmobile/setup-r7s72100.c b/arch/arm/mach-shmobile/setup-r7s72100.c
index d46639f..cc6237e 100644
--- a/arch/arm/mach-shmobile/setup-r7s72100.c
+++ b/arch/arm/mach-shmobile/setup-r7s72100.c
@@ -15,11 +15,40 @@
  */
 
 #include <linux/kernel.h>
+#include <linux/io.h>
 
 #include <asm/mach/arch.h>
 
 #include "common.h"
 
+/*
+ * This function is declared weak so if you need to do some board specific stuff
+ * before the reset occurs, you can override this function.
+ *
+ * CAUTION: A reboot command doesn't 'sync' before this function
+ * is called. See function reboot() in kernel/reboot.c
+ */
+extern void __attribute__ ((weak)) r7s72100_restart(enum reboot_mode mode,
+						    const char *cmd)
+{
+#define WTCSR 0
+#define WTCNT 2
+#define WRCSR 4
+	void *base = ioremap(0xFCFE0000, 0x10);
+
+	/* 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 */
+
+	/* Wait for WDT overflow */
+	while (1)
+		;
+}
+
 static const char *const r7s72100_boards_compat_dt[] __initconst = {
 	"renesas,r7s72100",
 	NULL,
@@ -29,4 +58,5 @@ DT_MACHINE_START(R7S72100_DT, "Generic R7S72100 (Flattened Device Tree)")
 	.init_early	= shmobile_init_delay,
 	.init_late	= shmobile_init_late,
 	.dt_compat	= r7s72100_boards_compat_dt,
+	.restart	= r7s72100_restart,
 MACHINE_END
-- 
2.10.1

Re: [PATCH] ARM: shmobile: r7s72100: add restart handler

[Geert Uytterhoeven]

Hi Chris,

On Thu, Feb 9, 2017 at 8:12 PM, Chris Brandt <chris.brandt@renesas.com> wrote:
> Add a simple restart handler which enables the watchdog timer with the
> device reset option enabled. This is the only way SW can cause a reset on
> this SoC.
>
> If someone has a board that needs more specific operations to be done
> first, they can override this function in another file.
>
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>

Thanks for your patch!

> ---
>  arch/arm/mach-shmobile/setup-r7s72100.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>
> diff --git a/arch/arm/mach-shmobile/setup-r7s72100.c b/arch/arm/mach-shmobile/setup-r7s72100.c
> index d46639f..cc6237e 100644
> --- a/arch/arm/mach-shmobile/setup-r7s72100.c
> +++ b/arch/arm/mach-shmobile/setup-r7s72100.c
> @@ -15,11 +15,40 @@
>   */
>
>  #include <linux/kernel.h>
> +#include <linux/io.h>
>
>  #include <asm/mach/arch.h>
>
>  #include "common.h"
>
> +/*
> + * This function is declared weak so if you need to do some board specific stuff
> + * before the reset occurs, you can override this function.
> + *
> + * CAUTION: A reboot command doesn't 'sync' before this function
> + * is called. See function reboot() in kernel/reboot.c
> + */
> +extern void __attribute__ ((weak)) r7s72100_restart(enum reboot_mode mode,
> +                                                   const char *cmd)
> +{
> +#define WTCSR 0
> +#define WTCNT 2
> +#define WRCSR 4
> +       void *base = ioremap(0xFCFE0000, 0x10);

This base address should come from a watchdog device node in DT...

> +       /* 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 */
> +
> +       /* Wait for WDT overflow */
> +       while (1)
> +               ;
> +}
> +
>  static const char *const r7s72100_boards_compat_dt[] __initconst = {
>         "renesas,r7s72100",
>         NULL,
> @@ -29,4 +58,5 @@ DT_MACHINE_START(R7S72100_DT, "Generic R7S72100 (Flattened Device Tree)")
>         .init_early     = shmobile_init_delay,
>         .init_late      = shmobile_init_late,
>         .dt_compat      = r7s72100_boards_compat_dt,
> +       .restart        = r7s72100_restart,
>  MACHINE_END

Perhaps unsurprisingly, I'd recommend writing a watchdog driver instead.
drivers/watchdog/renesas_wdt.c (currently supporting R-Car Gen3 only) may
serve as an example.

>From an earlier discussion during development of that driver:

| The RWDT exists on various Renesas SoCs.
| From digging into the datasheets, I had discovered two variants a while go:
|   1. 32-bit registers
|        a. R-Car Gen2: using RST for restarting
|        b. R-Mobile APE6: using SYSC for restarting
|   2. 8-bit registers (SH-Mobile AP4/AG5, R-Mobile A1)
|
| The differences are small: the variant with 8-bit registers has a smaller
| maximum timeout, and no magic value to be stored in the upper bits.
|
| Wolfram discovered the third variant in RZ/A1H, which differs in
| register layout.

IIRC, apart from the different register layout, actual operation on RZ/A1H
is similar to other Renesas SoCs.  Depending on the differences, you may
decide to write a new driver instead, though.

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

RE: [PATCH] ARM: shmobile: r7s72100: add restart handler

[Chris Brandt]

Hi Geert,

On Friday, February 10, 2017, Geert Uytterhoeven wrote:
> >  static const char *const r7s72100_boards_compat_dt[] __initconst = {
> >         "renesas,r7s72100",
> >         NULL,
> > @@ -29,4 +58,5 @@ DT_MACHINE_START(R7S72100_DT, "Generic R7S72100
> (Flattened Device Tree)")
> >         .init_early     = shmobile_init_delay,
> >         .init_late      = shmobile_init_late,
> >         .dt_compat      = r7s72100_boards_compat_dt,
> > +       .restart        = r7s72100_restart,
> >  MACHINE_END
> 
> Perhaps unsurprisingly, I'd recommend writing a watchdog driver instead.
> drivers/watchdog/renesas_wdt.c (currently supporting R-Car Gen3 only) may
> serve as an example.

Why do you guys always make things more difficult for me... ;)


To be clear, you are recommending I make a WDT timer driver (and not
use .restart) and that will reset the system?

Or, make a WDT driver just so I can steal the base address?

Note that the WDT timeout for the RZ/A1 is too short in my opinion, so
it's really only good for resetting the system.


> From an earlier discussion during development of that driver:
> 
> | The RWDT exists on various Renesas SoCs.
> | From digging into the datasheets, I had discovered two variants a while
> go:
> |   1. 32-bit registers
> |        a. R-Car Gen2: using RST for restarting
> |        b. R-Mobile APE6: using SYSC for restarting
> |   2. 8-bit registers (SH-Mobile AP4/AG5, R-Mobile A1)
> |
> | The differences are small: the variant with 8-bit registers has a
> | smaller maximum timeout, and no magic value to be stored in the upper
> bits.
> |
> | Wolfram discovered the third variant in RZ/A1H, which differs in
> | register layout.
> 
> IIRC, apart from the different register layout, actual operation on RZ/A1H
> is similar to other Renesas SoCs.  Depending on the differences, you may
> decide to write a new driver instead, though.

More or less they all do the same thing (all only have 3 registers).
I guess the decision comes down to since the file name is already
"renesas_wdt.c", do we just make the 1 file work for all Renesas SoC
and not add yet another file.
It is only 3 registers we're talking about...it's not like it's going
to turn into another sh_pfc.c file.

Question: R-Car Gen 3 has 3 watchdog timers:
 1. RCLK watchdog timer (RWDT)
 2. Window Watchdog Timer (WWDT)
 3. System Watchdog

#1 and #3 look like they are the same thing (except #3 is enabled on power
on reset). The renesas_wdt.c uses the register names from #1.
Is the idea that you only use #3 to make sure your systems boots and get into
Linux, then from there you use #1 and stop #3 (hence no driver is needed)?

I don’t see the point of having an "overflow interrupt" enabled if the system
is going to reset regardless.


Chris

Re: [PATCH] ARM: shmobile: r7s72100: add restart handler

[Geert Uytterhoeven]

Hi Chris,

On Fri, Feb 10, 2017 at 3:59 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Friday, February 10, 2017, Geert Uytterhoeven wrote:
>> >  static const char *const r7s72100_boards_compat_dt[] __initconst = {
>> >         "renesas,r7s72100",
>> >         NULL,
>> > @@ -29,4 +58,5 @@ DT_MACHINE_START(R7S72100_DT, "Generic R7S72100
>> (Flattened Device Tree)")
>> >         .init_early     = shmobile_init_delay,
>> >         .init_late      = shmobile_init_late,
>> >         .dt_compat      = r7s72100_boards_compat_dt,
>> > +       .restart        = r7s72100_restart,
>> >  MACHINE_END
>>
>> Perhaps unsurprisingly, I'd recommend writing a watchdog driver instead.
>> drivers/watchdog/renesas_wdt.c (currently supporting R-Car Gen3 only) may
>> serve as an example.
>
> Why do you guys always make things more difficult for me... ;)
>
> To be clear, you are recommending I make a WDT timer driver (and not
> use .restart) and that will reset the system?
>
> Or, make a WDT driver just so I can steal the base address?

I meant a WDT timer driver. If the watchdog driver provides a .restart
callback, it will be used for system reset (hmm, renesas_wdt.c no longer has
the .restart callback, as per arm64 "system reset must be implemented using
PSCI"-policy).

> Note that the WDT timeout for the RZ/A1 is too short in my opinion, so
> it's really only good for resetting the system.

Hmm... max. 125 ms is indeed not much.

Alternatively, you can write a restart driver (cfr.
drivers/power/reset/rmobile-reset.c) that binds against a
"renesas,r7s72100-wdt" device node, but doesn't implement watchdog
functionality.
You're gonna need DT bindings anyway.

>> From an earlier discussion during development of that driver:
>>
>> | The RWDT exists on various Renesas SoCs.
>> | From digging into the datasheets, I had discovered two variants a while
>> go:
>> |   1. 32-bit registers
>> |        a. R-Car Gen2: using RST for restarting
>> |        b. R-Mobile APE6: using SYSC for restarting
>> |   2. 8-bit registers (SH-Mobile AP4/AG5, R-Mobile A1)
>> |
>> | The differences are small: the variant with 8-bit registers has a
>> | smaller maximum timeout, and no magic value to be stored in the upper
>> bits.
>> |
>> | Wolfram discovered the third variant in RZ/A1H, which differs in
>> | register layout.
>>
>> IIRC, apart from the different register layout, actual operation on RZ/A1H
>> is similar to other Renesas SoCs.  Depending on the differences, you may
>> decide to write a new driver instead, though.
>
> More or less they all do the same thing (all only have 3 registers).
> I guess the decision comes down to since the file name is already
> "renesas_wdt.c", do we just make the 1 file work for all Renesas SoC
> and not add yet another file.
> It is only 3 registers we're talking about...it's not like it's going
> to turn into another sh_pfc.c file.
>
> Question: R-Car Gen 3 has 3 watchdog timers:
>  1. RCLK watchdog timer (RWDT)
>  2. Window Watchdog Timer (WWDT)

The WWDT does not exist on R-Car H3 and M3-W ;-)

>  3. System Watchdog
>
> #1 and #3 look like they are the same thing (except #3 is enabled on power
> on reset). The renesas_wdt.c uses the register names from #1.
> Is the idea that you only use #3 to make sure your systems boots and get into
> Linux, then from there you use #1 and stop #3 (hence no driver is needed)?

Isn't the SRWDT restricted to secure mode?

> I don’t see the point of having an "overflow interrupt" enabled if the system
> is going to reset regardless.

?

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

Re: [PATCH] ARM: shmobile: r7s72100: add restart handler

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 372 bytes --]


> > #1 and #3 look like they are the same thing (except #3 is enabled on power
> > on reset). The renesas_wdt.c uses the register names from #1.
> > Is the idea that you only use #3 to make sure your systems boots and get into
> > Linux, then from there you use #1 and stop #3 (hence no driver is needed)?
> 
> Isn't the SRWDT restricted to secure mode?

Yes.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH] ARM: shmobile: r7s72100: add restart handler

[Chris Brandt]

Hi Geert,

On Friday, February 10, 2017, Geert Uytterhoeven wrote:
> Alternatively, you can write a restart driver (cfr.
> drivers/power/reset/rmobile-reset.c) that binds against a
> "renesas,r7s72100-wdt" device node, but doesn't implement watchdog
> functionality.
> You're gonna need DT bindings anyway.

I like that idea. That should take me no time at all.
Thank you.

Do you think I can still keep my 'weak function' idea in there??


extern void __attribute__ ((weak)) prepare_for_restart(void)
{
	/* override to do board specific stuff */
}

static int renesas_wdt_reset_handler(struct notifier_block *this,
				 unsigned long mode, void *cmd)
{
	pr_debug("%s %lu\n", __func__, mode);

	prepare_for_restart();

	/* set WDT for reset */
	. . .

	return NOTIFY_DONE;
}



Or...do you think I can just use the rmobile-reset.c driver and
just add WDT to it?

Honestly, the only thing different will be rmobile_reset_handler().
I could make a rmobile_wdt_reset_handler() and I could just pass in
a different notifier_block depending on the DT.

What do you think?

static const struct of_device_id rmobile_reset_of_match[] = {
	{ .compatible = "renesas,sysc-rmobile", },
	{ .compatible = "renesas,wdt-rmobile", },
	{ /* sentinel */ }
};


Chris

Re: [PATCH] ARM: shmobile: r7s72100: add restart handler

[Geert Uytterhoeven]

Hi Chris,

On Fri, Feb 10, 2017 at 8:46 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Friday, February 10, 2017, Geert Uytterhoeven wrote:
>> Alternatively, you can write a restart driver (cfr.
>> drivers/power/reset/rmobile-reset.c) that binds against a
>> "renesas,r7s72100-wdt" device node, but doesn't implement watchdog
>> functionality.
>> You're gonna need DT bindings anyway.
>
> I like that idea. That should take me no time at all.
> Thank you.
>
> Do you think I can still keep my 'weak function' idea in there??
>
>
> extern void __attribute__ ((weak)) prepare_for_restart(void)
> {
>         /* override to do board specific stuff */
> }
>
> static int renesas_wdt_reset_handler(struct notifier_block *this,
>                                  unsigned long mode, void *cmd)
> {
>         pr_debug("%s %lu\n", __func__, mode);
>
>         prepare_for_restart();
>
>         /* set WDT for reset */
>         . . .
>
>         return NOTIFY_DONE;
> }

What do you want to use the board-specific function for?
Have a board-specific reset handler, or do some preparatory cleanup?

In case of the former, please write a separate driver that registers  a
reset handler with a higher priority.
In case of the latter, please use register_reboot_notifier() in the driver
that needs it.

> Or...do you think I can just use the rmobile-reset.c driver and
> just add WDT to it?
>
> Honestly, the only thing different will be rmobile_reset_handler().
> I could make a rmobile_wdt_reset_handler() and I could just pass in
> a different notifier_block depending on the DT.
>
> What do you think?

Given the small amount of code to add to the existing driver, and the sheer
amount of boilerplate for writing a new driver, I'm inclined to say yes.
But in the end, it's up to the subsystem maintainer to decide.

> static const struct of_device_id rmobile_reset_of_match[] = {
>         { .compatible = "renesas,sysc-rmobile", },
>         { .compatible = "renesas,wdt-rmobile", },

renesas,r7s72100-wdt

>         { /* sentinel */ }
> };

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

RE: [PATCH] ARM: shmobile: r7s72100: add restart handler

[Chris Brandt]

Hi Geert,

On Monday, February 13, 2017, Geert Uytterhoeven wrote:
> On Fri, Feb 10, 2017 at 8:46 PM, Chris Brandt <Chris.Brandt@renesas.com>
> wrote:
> > On Friday, February 10, 2017, Geert Uytterhoeven wrote:
> >> Alternatively, you can write a restart driver (cfr.
> >> drivers/power/reset/rmobile-reset.c) that binds against a
> >> "renesas,r7s72100-wdt" device node, but doesn't implement watchdog
> >> functionality.
> >> You're gonna need DT bindings anyway.
> >
> > I like that idea. That should take me no time at all.
> > Thank you.
> >
> > Do you think I can still keep my 'weak function' idea in there??
> >
> >
> > extern void __attribute__ ((weak)) prepare_for_restart(void) {
> >         /* override to do board specific stuff */ }
> >
> > static int renesas_wdt_reset_handler(struct notifier_block *this,
> >                                  unsigned long mode, void *cmd) {
> >         pr_debug("%s %lu\n", __func__, mode);
> >
> >         prepare_for_restart();
> >
> >         /* set WDT for reset */
> >         . . .
> >
> >         return NOTIFY_DONE;
> > }
> 
> What do you want to use the board-specific function for?
> Have a board-specific reset handler, or do some preparatory cleanup?
> 
> In case of the former, please write a separate driver that registers  a
> reset handler with a higher priority.
> In case of the latter, please use register_reboot_notifier() in the driver
> that needs it.

On my board (the RSK), I don't really care. I was thinking more about
other users boards. In other words, what should I tell them what they
should do?
I will suggest your recommendations above (not include the weak modifier).


> > Or...do you think I can just use the rmobile-reset.c driver and just
> > add WDT to it?
> >
> > Honestly, the only thing different will be rmobile_reset_handler().
> > I could make a rmobile_wdt_reset_handler() and I could just pass in a
> > different notifier_block depending on the DT.
> >
> > What do you think?
> 
> Given the small amount of code to add to the existing driver, and the
> sheer amount of boilerplate for writing a new driver, I'm inclined to say
> yes.
> But in the end, it's up to the subsystem maintainer to decide.
> 
> > static const struct of_device_id rmobile_reset_of_match[] = {
> >         { .compatible = "renesas,sysc-rmobile", },
> >         { .compatible = "renesas,wdt-rmobile", },
> 
> renesas,r7s72100-wdt

OK. Thanks!


Chris

RE: [PATCH] ARM: shmobile: r7s72100: add restart handler

[Chris Brandt]

Hi Geert,

On Monday, February 13, 2017, Geert Uytterhoeven wrote: 
> > static const struct of_device_id rmobile_reset_of_match[] = {
> >         { .compatible = "renesas,sysc-rmobile", },
> >         { .compatible = "renesas,wdt-rmobile", },
> 
> renesas,r7s72100-wdt

I remember why I suggested "renesas,wdt-rmobile".

If I thought this WDT was going to be reused again in the RZ/A series,
wouldn't it make sense to just give it a generic name instead off a
specific part number?

And hence "renesas,wdt-rmobile" would be a better name than 
"renesas,r7s72100-wdt", "renesas,rza2-wdt", "renesas,rza3-wdt", etc... ?


Or, just stick with the part number and don't worry about it?


Thanks,
Chris