Re: [PATCH v2 1/3] power: reset: Add Renesas reset driver

[Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>]

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-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> 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-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html