[v2,1/3] power: reset: Add Renesas reset driver
diff mbox

Message ID 20170216172320.10897-2-chris.brandt@renesas.com
State Under Review
Delegated to: Geert Uytterhoeven
Headers show

Commit Message

Chris Brandt Feb. 16, 2017, 5:23 p.m. UTC
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>
---
v2:
* changed DT bindings from 'wdt-reset' to 'rza-wdt'
* changed hard coded register values to defines
* added msleep to while(1) loop
* 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

Comments

Guenter Roeck Feb. 16, 2017, 6:26 p.m. UTC | #1
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
> 
>
Chris Brandt Feb. 16, 2017, 6:40 p.m. UTC | #2
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
Guenter Roeck Feb. 16, 2017, 10:20 p.m. UTC | #3
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
Chris Brandt Feb. 17, 2017, 2 a.m. UTC | #4
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
Guenter Roeck Feb. 17, 2017, 4:45 a.m. UTC | #5
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
Geert Uytterhoeven Feb. 17, 2017, 8:09 a.m. UTC | #6
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
Guenter Roeck Feb. 17, 2017, 10:06 a.m. UTC | #7
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
>
Chris Brandt Feb. 17, 2017, 1:49 p.m. UTC | #8
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
Chris Brandt Feb. 22, 2017, 1:35 p.m. UTC | #9
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
Guenter Roeck Feb. 22, 2017, 3:28 p.m. UTC | #10
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
Chris Brandt Feb. 22, 2017, 4:04 p.m. UTC | #11
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

Patch
diff mbox

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");