| Message ID | 20170209191259.29774-1-chris.brandt@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Simon Horman |
| Headers | show |
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
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
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
> > #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.
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
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
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
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
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
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(+)