| Message ID | 20170216172320.10897-2-chris.brandt@renesas.com (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
On Thu, Feb 16, 2017 at 12:23:18PM -0500, Chris Brandt 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.brandt@renesas.com> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Wondering - why not use the watchdog driver and the infrastructure provided by the watchdog core (ie the restart callback) instead ? > --- > v2: > * changed DT bindings from 'wdt-reset' to 'rza-wdt' > * changed hard coded register values to defines > * added msleep to while(1) loop Sure you can sleep here ? Guenter > * removed unnecessary #include files > * added Reviewed-by: Geert Uytterhoeven > --- > drivers/power/reset/Kconfig | 9 +++ > drivers/power/reset/Makefile | 1 + > drivers/power/reset/renesas-reset.c | 112 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 122 insertions(+) > create mode 100644 drivers/power/reset/renesas-reset.c > > diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig > index b8caccc..e3100c9 100644 > --- a/drivers/power/reset/Kconfig > +++ b/drivers/power/reset/Kconfig > @@ -130,6 +130,15 @@ config POWER_RESET_QNAP > > Say Y if you have a QNAP NAS. > > +config POWER_RESET_RENESAS > + tristate "Renesas WDT reset driver" > + depends on ARCH_RENESAS || COMPILE_TEST > + depends on HAS_IOMEM > + help > + Reboot support for Renesas SoCs with WDT reset. > + 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. > + > config POWER_RESET_RESTART > bool "Restart power-off driver" > help > diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile > index 11dae3b..a78a56c 100644 > --- a/drivers/power/reset/Makefile > +++ b/drivers/power/reset/Makefile > @@ -13,6 +13,7 @@ obj-$(CONFIG_POWER_RESET_MSM) += msm-poweroff.o > obj-$(CONFIG_POWER_RESET_PIIX4_POWEROFF) += piix4-poweroff.o > obj-$(CONFIG_POWER_RESET_LTC2952) += ltc2952-poweroff.o > obj-$(CONFIG_POWER_RESET_QNAP) += qnap-poweroff.o > +obj-$(CONFIG_POWER_RESET_RENESAS) += renesas-reset.o > obj-$(CONFIG_POWER_RESET_RESTART) += restart-poweroff.o > obj-$(CONFIG_POWER_RESET_ST) += st-poweroff.o > obj-$(CONFIG_POWER_RESET_VERSATILE) += arm-versatile-reboot.o > diff --git a/drivers/power/reset/renesas-reset.c b/drivers/power/reset/renesas-reset.c > new file mode 100644 > index 0000000..812cee0 > --- /dev/null > +++ b/drivers/power/reset/renesas-reset.c > @@ -0,0 +1,112 @@ > +/* > + * Renesas WDT Reset Driver > + * > + * Copyright (C) 2017 Renesas Electronics America, Inc. > + * Copyright (C) 2017 Chris Brandt > + * > + * This file is subject to the terms and conditions of the GNU General Public > + * License. See the file "COPYING" in the main directory of this archive > + * for more details. > + * > + * based on rmobile-reset.c > + * > + */ > + > +#include <linux/platform_device.h> > +#include <linux/module.h> > +#include <linux/of_address.h> > +#include <linux/reboot.h> > +#include <linux/delay.h> > + > +/* Watchdog Timer Registers */ > +#define WTCSR 0 > +#define WTCSR_MAGIC 0xA500 > +#define WTSCR_WT (1<<6) > +#define WTSCR_TME (1<<5) > + > +#define WTCNT 2 > +#define WTCNT_MAGIC 0x5A00 > + > +#define WRCSR 4 > +#define WRCSR_MAGIC 0x5A00 > +#define WRCSR_RSTE (1<<6) > +#define WRCSR_CLEAR_WOVF 0xA500 /* special value */ > + > +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) */ > + readb(base + WRCSR); > + > + writew(WRCSR_CLEAR_WOVF, base + WRCSR); /* Clear WOVF */ > + writew(WRCSR_MAGIC | WRCSR_RSTE, base + WRCSR); /* Reset Enable */ > + writew(WTCNT_MAGIC, base + WTCNT); /* Counter to 00 */ > + > + /* Start timer */ > + writew(WTCSR_MAGIC | WTSCR_WT | WTSCR_TME, base + WTCSR); > + > + /* Wait for WDT overflow (reset) */ > + while (1) > + msleep(1); > + > + return NOTIFY_DONE; > +} > + > +static struct notifier_block wdt_reset_nb = { > + .notifier_call = wdt_reset_handler, > + .priority = 192, > +}; > + > +static int wdt_reset_probe(struct platform_device *pdev) > +{ > + int error; > + > + base = of_iomap(pdev->dev.of_node, 0); > + if (!base) > + return -ENODEV; > + > + error = register_restart_handler(&wdt_reset_nb); > + if (error) { > + dev_err(&pdev->dev, > + "cannot register restart handler (err=%d)\n", error); > + goto fail_unmap; > + } > + > + return 0; > + > +fail_unmap: > + iounmap(base); > + return error; > +} > + > +static int wdt_reset_remove(struct platform_device *pdev) > +{ > + unregister_restart_handler(&wdt_reset_nb); > + iounmap(base); > + return 0; > +} > + > +static const struct of_device_id wdt_reset_of_match[] = { > + { .compatible = "renesas,rza-wdt", }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, wdt_reset_of_match); > + > +static struct platform_driver wdt_reset_driver = { > + .probe = wdt_reset_probe, > + .remove = wdt_reset_remove, > + .driver = { > + .name = "wdt_reset", > + .of_match_table = wdt_reset_of_match, > + }, > +}; > + > +module_platform_driver(wdt_reset_driver); > + > +MODULE_DESCRIPTION("Renesas WDT Reset Driver"); > +MODULE_AUTHOR("Chris Brandt <chris.brandt@renesas.com>"); > +MODULE_LICENSE("GPL v2"); > -- > 2.10.1 > >
On Thursday, February 16, 2017, Guenter Roeck wrote: > On Thu, Feb 16, 2017 at 12:23:18PM -0500, Chris Brandt 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.brandt@renesas.com> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Wondering - why not use the watchdog driver and the infrastructure > provided by the watchdog core (ie the restart callback) instead ? That was Geert's first suggestion, but then he said: On Friday, February 10, 2017, Geert Uytterhoeven wrote: > 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). > > v2: > > * changed DT bindings from 'wdt-reset' to 'rza-wdt' > > * changed hard coded register values to defines > > * added msleep to while(1) loop > > Sure you can sleep here ? As per Geert's review: On Wednesday, February 15, 2017, Geert Uytterhoeven wrote: > > + > > + /* 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? Note that you only have 7.7us before the restart occurs, so probably not much sleeping will be going on. Chris
On Thu, Feb 16, 2017 at 06:40:05PM +0000, Chris Brandt wrote: > On Thursday, February 16, 2017, Guenter Roeck wrote: > > On Thu, Feb 16, 2017 at 12:23:18PM -0500, Chris Brandt 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.brandt@renesas.com> Reviewed-by: Geert > > > Uytterhoeven <geert+renesas@glider.be> > > > > Wondering - why not use the watchdog driver and the infrastructure provided > > by the watchdog core (ie the restart callback) instead ? > > That was Geert's first suggestion, but then he said: > > On Friday, February 10, 2017, Geert Uytterhoeven wrote: > > 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). > Hmm, ok. Guess I don't have to understand that you can not use the watchdog driver because of the above, but implementing exactly the same functionality in a separate driver is ok. [ I am sure I am missing something here, so just ignore what I am saying ] > > > > > v2: * changed DT bindings from 'wdt-reset' to 'rza-wdt' * changed hard > > > coded register values to defines * added msleep to while(1) loop > > > > Sure you can sleep here ? > > As per Geert's review: > > On Wednesday, February 15, 2017, Geert Uytterhoeven wrote: > > > + + /* 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? > > Note that you only have 7.7us before the restart occurs, so probably not much > sleeping will be going on. > Let me rephrase my question. Is it guaranteed that you _can_ sleep in this function, or in other words that it is guaranteed that interrupts are enabled ? Thanks, Guenter
On Thursday, February 16, 2017, Guenter Roeck wrote: > Hmm, ok. Guess I don't have to understand that you can not use the > watchdog driver because of the above, but implementing exactly the same > functionality in a separate driver is ok. > > [ I am sure I am missing something here, so just ignore what I am saying ] Honestly, I don't have a handle on all the latest 'policies and procedures' for all the various subsystems. All I know is that I want to figure out how to execute my 5 lines of code to reset the system when the user types "reboot" on the command line. If this WDT had a timeout longer than 125ms, I would make a real watchdog driver out of it. But at the moment, it just serves as the only way to reset the chip. Historically, this was the only way to reset a SH4 device...and we just lived with that fact. When Renesas moved from SH4 to ARM, a lot of the design teams just kept the same philosophy (and copied the HW blocks they knew already worked) > > > > v2: * changed DT bindings from 'wdt-reset' to 'rza-wdt' * changed > > > > hard coded register values to defines * added msleep to while(1) > > > > loop > > > > > > Sure you can sleep here ? > > > > As per Geert's review: > > > > On Wednesday, February 15, 2017, Geert Uytterhoeven wrote: > > > > + + /* 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? > > > > Note that you only have 7.7us before the restart occurs, so probably > > not much sleeping will be going on. > > > Let me rephrase my question. Is it guaranteed that you _can_ sleep in this > function, or in other words that it is guaranteed that interrupts are > enabled ? Hmm, I'm not sure if will actually 'sleep' or not. Regardless, interrupts or not, in 7.7us, the internal reset circuit is going to get triggered and the whole system is going to reboot not matter what is going on. I know Geert's suggestion was in reference to saving power...but in reality it's probably negligible when we are talking about 7.7us. The reboot is going to automatically shut off all the peripherals clocks as well. Chris
On 02/16/2017 06:00 PM, Chris Brandt wrote: > On Thursday, February 16, 2017, Guenter Roeck wrote: >> Hmm, ok. Guess I don't have to understand that you can not use the >> watchdog driver because of the above, but implementing exactly the same >> functionality in a separate driver is ok. >> >> [ I am sure I am missing something here, so just ignore what I am saying ] > > Honestly, I don't have a handle on all the latest 'policies and procedures' > for all the various subsystems. All I know is that I want to figure out how > to execute my 5 lines of code to reset the system when the user types "reboot" > on the command line. > > If this WDT had a timeout longer than 125ms, I would make a real watchdog driver > out of it. But at the moment, it just serves as the only way to reset the chip. > Historically, this was the only way to reset a SH4 device...and we just lived > with that fact. When Renesas moved from SH4 to ARM, a lot of the design teams > just kept the same philosophy (and copied the HW blocks they knew already worked) > FWIW, the watchdog subsystem should support that easily, even with 125 ms hardware timeout. We added that capability for that very purpose. That would only fail if the system is stuck with interrupts disabled for more than 125 ms, which seems unlikely. I think the gpio watchdog on some systems has a similar low hardware timeout. > >>>>> v2: * changed DT bindings from 'wdt-reset' to 'rza-wdt' * changed >>>>> hard coded register values to defines * added msleep to while(1) >>>>> loop >>>> >>>> Sure you can sleep here ? >>> >>> As per Geert's review: >>> >>> On Wednesday, February 15, 2017, Geert Uytterhoeven wrote: >>>>> + + /* 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? >>> >>> Note that you only have 7.7us before the restart occurs, so probably >>> not much sleeping will be going on. >>> >> Let me rephrase my question. Is it guaranteed that you _can_ sleep in this >> function, or in other words that it is guaranteed that interrupts are >> enabled ? > > Hmm, I'm not sure if will actually 'sleep' or not. Regardless, interrupts or not, > in 7.7us, the internal reset circuit is going to get triggered and the whole system > is going to reboot not matter what is going on. > That isn't the point, really. You might get a WARN blob if msleep() is called with interrupts disabled, but of course you won't really see that because it can not be displayed in 7.7 us. > I know Geert's suggestion was in reference to saving power...but in reality it's > probably negligible when we are talking about 7.7us. The reboot is going to > automatically shut off all the peripherals clocks as well. > Maybe udelay(10); would have been more acceptable (and appropriate). Guenter
Hi Günter, On Fri, Feb 17, 2017 at 5:45 AM, Guenter Roeck <linux@roeck-us.net> wrote: > On 02/16/2017 06:00 PM, Chris Brandt wrote: >> On Thursday, February 16, 2017, Guenter Roeck wrote: >> If this WDT had a timeout longer than 125ms, I would make a real watchdog >> driver >> out of it. But at the moment, it just serves as the only way to reset the >> chip. >> Historically, this was the only way to reset a SH4 device...and we just >> lived >> with that fact. When Renesas moved from SH4 to ARM, a lot of the design >> teams >> just kept the same philosophy (and copied the HW blocks they knew already >> worked) > > FWIW, the watchdog subsystem should support that easily, even with 125 ms > hardware > timeout. We added that capability for that very purpose. That would only > fail if > the system is stuck with interrupts disabled for more than 125 ms, which > seems > unlikely. I think the gpio watchdog on some systems has a similar low > hardware > timeout. I also thought 125ms was a bit short, but I'm happy to be proven wrong! Let's make a real watchdog driver instead ;-) >>>>>> v2: * changed DT bindings from 'wdt-reset' to 'rza-wdt' * changed >>>>>> hard coded register values to defines * added msleep to while(1) >>>>>> loop >>>>> >>>>> Sure you can sleep here ? >>>> >>>> As per Geert's review: >>>> >>>> On Wednesday, February 15, 2017, Geert Uytterhoeven wrote: >>>>>> >>>>>> + + /* 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? >>>> >>>> Note that you only have 7.7us before the restart occurs, so probably >>>> not much sleeping will be going on. >>>> >>> Let me rephrase my question. Is it guaranteed that you _can_ sleep in >>> this >>> function, or in other words that it is guaranteed that interrupts are >>> enabled ? >> >> Hmm, I'm not sure if will actually 'sleep' or not. Regardless, interrupts >> or not, >> in 7.7us, the internal reset circuit is going to get triggered and the >> whole system >> is going to reboot not matter what is going on. >> > > That isn't the point, really. You might get a WARN blob if msleep() is > called > with interrupts disabled, but of course you won't really see that because it > can > not be displayed in 7.7 us. If it's not 100% guaranteed that we cannot sleep, we should not use msleep(), and stick to busy waiting. On ARM, we could also use wfi(). >> I know Geert's suggestion was in reference to saving power...but in >> reality it's >> probably negligible when we are talking about 7.7us. The reboot is going >> to >> automatically shut off all the peripherals clocks as well. > > Maybe udelay(10); would have been more acceptable (and appropriate). Inside the while (1) loop? That's the same as a plain "while (1) ;" ;-) Or just udelay(10) and return, with the latter never happening if everything goes well? Then the next restart handler will be tried, if available. 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
On 02/17/2017 12:09 AM, Geert Uytterhoeven wrote: > Hi Günter, > > On Fri, Feb 17, 2017 at 5:45 AM, Guenter Roeck <linux@roeck-us.net> wrote: >> On 02/16/2017 06:00 PM, Chris Brandt wrote: >>> On Thursday, February 16, 2017, Guenter Roeck wrote: >>> If this WDT had a timeout longer than 125ms, I would make a real watchdog >>> driver >>> out of it. But at the moment, it just serves as the only way to reset the >>> chip. >>> Historically, this was the only way to reset a SH4 device...and we just >>> lived >>> with that fact. When Renesas moved from SH4 to ARM, a lot of the design >>> teams >>> just kept the same philosophy (and copied the HW blocks they knew already >>> worked) >> >> FWIW, the watchdog subsystem should support that easily, even with 125 ms >> hardware >> timeout. We added that capability for that very purpose. That would only >> fail if >> the system is stuck with interrupts disabled for more than 125 ms, which >> seems >> unlikely. I think the gpio watchdog on some systems has a similar low >> hardware >> timeout. > > I also thought 125ms was a bit short, but I'm happy to be proven wrong! > Let's make a real watchdog driver instead ;-) > >>>>>>> v2: * changed DT bindings from 'wdt-reset' to 'rza-wdt' * changed >>>>>>> hard coded register values to defines * added msleep to while(1) >>>>>>> loop >>>>>> >>>>>> Sure you can sleep here ? >>>>> >>>>> As per Geert's review: >>>>> >>>>> On Wednesday, February 15, 2017, Geert Uytterhoeven wrote: >>>>>>> >>>>>>> + + /* 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? >>>>> >>>>> Note that you only have 7.7us before the restart occurs, so probably >>>>> not much sleeping will be going on. >>>>> >>>> Let me rephrase my question. Is it guaranteed that you _can_ sleep in >>>> this >>>> function, or in other words that it is guaranteed that interrupts are >>>> enabled ? >>> >>> Hmm, I'm not sure if will actually 'sleep' or not. Regardless, interrupts >>> or not, >>> in 7.7us, the internal reset circuit is going to get triggered and the >>> whole system >>> is going to reboot not matter what is going on. >>> >> >> That isn't the point, really. You might get a WARN blob if msleep() is >> called >> with interrupts disabled, but of course you won't really see that because it >> can >> not be displayed in 7.7 us. > > If it's not 100% guaranteed that we cannot sleep, we should not use msleep(), > and stick to busy waiting. > On ARM, we could also use wfi(). > >>> I know Geert's suggestion was in reference to saving power...but in >>> reality it's >>> probably negligible when we are talking about 7.7us. The reboot is going >>> to >>> automatically shut off all the peripherals clocks as well. >> >> Maybe udelay(10); would have been more acceptable (and appropriate). > > Inside the while (1) loop? That's the same as a plain "while (1) ;" ;-) > Or just udelay(10) and return, with the latter never happening if everything > goes well? Then the next restart handler will be tried, if available. > That is what I meant. Or use udelay(20) to be on the safe side. Guenter > 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 > -- > To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
Hi Günter and Geert, Catching up on the subject (because I was sleeping here in the US...) On Friday, February 17, 2017, Geert Uytterhoeven wrote: > > FWIW, the watchdog subsystem should support that easily, even with 125 > > ms hardware timeout. We added that capability for that very purpose. > > That would only fail if the system is stuck with interrupts disabled > > for more than 125 ms, which seems unlikely. I think the gpio watchdog > > on some systems has a similar low hardware timeout. > > I also thought 125ms was a bit short, but I'm happy to be proven wrong! > Let's make a real watchdog driver instead ;-) OK, I'll see about switching over to a real watchdog driver. As long as I can get my reset handler, I'll be happy. Besides, if I get the 8-bit counter register changed to 16-bit (a useful WDT), then I already have a WDT made. The good thing is that the DT binding will stay exactly the same, I just need a new driver. FWIW, if I was making a real product I would always have a WDT running (but with a 1-3 second timeout just to make sure I was completely dead). # side note, I tell people to set up a DMA with a timer trigger and automatically feed the WDT that way, then they can set the DMA count to whatever failsafe timeout they want. Not sure if I could submit that kind of hackery in an upstream driver. > >>> I know Geert's suggestion was in reference to saving power...but in > >>> reality it's probably negligible when we are talking about 7.7us. > >>> The reboot is going to automatically shut off all the peripherals > >>> clocks as well. > >> > >> Maybe udelay(10); would have been more acceptable (and appropriate). > > > > Inside the while (1) loop? That's the same as a plain "while (1) ;" > > ;-) Or just udelay(10) and return, with the latter never happening if > > everything goes well? Then the next restart handler will be tried, if > available. > > > > That is what I meant. Or use udelay(20) to be on the safe side. OK. Sounds like we agree on: /* Start timer */ writew(WTCSR_MAGIC | WTSCR_WT | WTSCR_TME, base + WTCSR); /* Wait for WDT overflow (reset) */ Udelay(20); return NOTIFY_DONE; } FYI, I'll be at ELC next week, so I might not get to this until after then. Thanks for the discussion. Cheers Chris
Hello Geert and Guenter, On Thursday, February 16, 2017, Guenter Roeck wrote: > FWIW, the watchdog subsystem should support that easily, even with 125 ms > hardware timeout. We added that capability for that very purpose. That > would only fail if the system is stuck with interrupts disabled for more > than 125 ms, which seems unlikely. I think the gpio watchdog on some > systems has a similar low hardware timeout. While I'm going to try that for the RZ/A1, I have a question for you guys (or anyone else that has an opinion about end applications) If I were going to make a request to the chip designers to make the timeout longer for the next RZ/A chip, what would be a good max timeout for common Linux applications? Looking through the drivers in the watchdog directly, I see default timeouts of 20, 30, 60, and 120 seconds. Thank you, Chris
On Wed, Feb 22, 2017 at 01:35:12PM +0000, Chris Brandt wrote: > Hello Geert and Guenter, > > On Thursday, February 16, 2017, Guenter Roeck wrote: > > FWIW, the watchdog subsystem should support that easily, even with 125 ms > > hardware timeout. We added that capability for that very purpose. That > > would only fail if the system is stuck with interrupts disabled for more > > than 125 ms, which seems unlikely. I think the gpio watchdog on some > > systems has a similar low hardware timeout. > > > While I'm going to try that for the RZ/A1, I have a question for you guys > (or anyone else that has an opinion about end applications) > > > If I were going to make a request to the chip designers to make the timeout longer > for the next RZ/A chip, what would be a good max timeout for common Linux applications? > > Looking through the drivers in the watchdog directly, I see default timeouts of 20, > 30, 60, and 120 seconds. > 30 and 60 are pretty common for default timeouts, though I personally think they are a bit long. If you want direct HW support, a maximum of 120 seconds should be sufficient though not really necessary anymore since the core supports virtual timeouts. A maximum of at least 30 seconds would be needed if the watchdog is supposed to run at boot time (ie if it is enabled by ROMMON/BIOS and kept running by the kernel). Hope this helps, Guenter
Hello Guenter, On Wednesday, February 22, 2017, Guenter Roeck wrote: > > Looking through the drivers in the watchdog directly, I see default > > timeouts of 20, 30, 60, and 120 seconds. > > > 30 and 60 are pretty common for default timeouts, though I personally > think they are a bit long. If you want direct HW support, a maximum of 120 > seconds should be sufficient though not really necessary anymore since the > core supports virtual timeouts. A maximum of at least 30 seconds would be > needed if the watchdog is supposed to run at boot time (ie if it is > enabled by ROMMON/BIOS and kept running by the kernel). Thank you! Good point about the boot time. Chris
diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig index b8caccc..e3100c9 100644 --- a/drivers/power/reset/Kconfig +++ b/drivers/power/reset/Kconfig @@ -130,6 +130,15 @@ config POWER_RESET_QNAP Say Y if you have a QNAP NAS. +config POWER_RESET_RENESAS + tristate "Renesas WDT reset driver" + depends on ARCH_RENESAS || COMPILE_TEST + depends on HAS_IOMEM + help + Reboot support for Renesas SoCs with WDT reset. + 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. + config POWER_RESET_RESTART bool "Restart power-off driver" help diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile index 11dae3b..a78a56c 100644 --- a/drivers/power/reset/Makefile +++ b/drivers/power/reset/Makefile @@ -13,6 +13,7 @@ obj-$(CONFIG_POWER_RESET_MSM) += msm-poweroff.o obj-$(CONFIG_POWER_RESET_PIIX4_POWEROFF) += piix4-poweroff.o obj-$(CONFIG_POWER_RESET_LTC2952) += ltc2952-poweroff.o obj-$(CONFIG_POWER_RESET_QNAP) += qnap-poweroff.o +obj-$(CONFIG_POWER_RESET_RENESAS) += renesas-reset.o obj-$(CONFIG_POWER_RESET_RESTART) += restart-poweroff.o obj-$(CONFIG_POWER_RESET_ST) += st-poweroff.o obj-$(CONFIG_POWER_RESET_VERSATILE) += arm-versatile-reboot.o diff --git a/drivers/power/reset/renesas-reset.c b/drivers/power/reset/renesas-reset.c new file mode 100644 index 0000000..812cee0 --- /dev/null +++ b/drivers/power/reset/renesas-reset.c @@ -0,0 +1,112 @@ +/* + * Renesas WDT Reset Driver + * + * Copyright (C) 2017 Renesas Electronics America, Inc. + * Copyright (C) 2017 Chris Brandt + * + * This file is subject to the terms and conditions of the GNU General Public + * License. See the file "COPYING" in the main directory of this archive + * for more details. + * + * based on rmobile-reset.c + * + */ + +#include <linux/platform_device.h> +#include <linux/module.h> +#include <linux/of_address.h> +#include <linux/reboot.h> +#include <linux/delay.h> + +/* Watchdog Timer Registers */ +#define WTCSR 0 +#define WTCSR_MAGIC 0xA500 +#define WTSCR_WT (1<<6) +#define WTSCR_TME (1<<5) + +#define WTCNT 2 +#define WTCNT_MAGIC 0x5A00 + +#define WRCSR 4 +#define WRCSR_MAGIC 0x5A00 +#define WRCSR_RSTE (1<<6) +#define WRCSR_CLEAR_WOVF 0xA500 /* special value */ + +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) */ + readb(base + WRCSR); + + writew(WRCSR_CLEAR_WOVF, base + WRCSR); /* Clear WOVF */ + writew(WRCSR_MAGIC | WRCSR_RSTE, base + WRCSR); /* Reset Enable */ + writew(WTCNT_MAGIC, base + WTCNT); /* Counter to 00 */ + + /* Start timer */ + writew(WTCSR_MAGIC | WTSCR_WT | WTSCR_TME, base + WTCSR); + + /* Wait for WDT overflow (reset) */ + while (1) + msleep(1); + + return NOTIFY_DONE; +} + +static struct notifier_block wdt_reset_nb = { + .notifier_call = wdt_reset_handler, + .priority = 192, +}; + +static int wdt_reset_probe(struct platform_device *pdev) +{ + int error; + + base = of_iomap(pdev->dev.of_node, 0); + if (!base) + return -ENODEV; + + error = register_restart_handler(&wdt_reset_nb); + if (error) { + dev_err(&pdev->dev, + "cannot register restart handler (err=%d)\n", error); + goto fail_unmap; + } + + return 0; + +fail_unmap: + iounmap(base); + return error; +} + +static int wdt_reset_remove(struct platform_device *pdev) +{ + unregister_restart_handler(&wdt_reset_nb); + iounmap(base); + return 0; +} + +static const struct of_device_id wdt_reset_of_match[] = { + { .compatible = "renesas,rza-wdt", }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, wdt_reset_of_match); + +static struct platform_driver wdt_reset_driver = { + .probe = wdt_reset_probe, + .remove = wdt_reset_remove, + .driver = { + .name = "wdt_reset", + .of_match_table = wdt_reset_of_match, + }, +}; + +module_platform_driver(wdt_reset_driver); + +MODULE_DESCRIPTION("Renesas WDT Reset Driver"); +MODULE_AUTHOR("Chris Brandt <chris.brandt@renesas.com>"); +MODULE_LICENSE("GPL v2");