Hi Günter,
On Fri, Feb 17, 2017 at 5:45 AM, Guenter Roeck <linux@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxx
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
