[RFC] Using a watchdog as system reset

[RFC] Using a watchdog as system reset

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1896 bytes --]

Hello,

I have an i.MX25 system here with an external watchdog (using the
gpio_wdt driver). So the internal watchdog (imx2_wdt) is unused.

The problem with the unused imx2_wdt is that this usually provides the
restart handler and now a reboot ends with

	reboot: Restarting system
	Reboot failed -- System halted

until eventually the watchdog bites and resets the machine.

I imagine that this is a common enough issue to warrant a generic
solution. My suggestion is to formalize and implement something like:

	watchdog {
		compatible = "linux,wdt-gpio";
		...
		provide-system-reset;
	}

with the sematic of: "This is the dedicated mechanism to reset this
machine."

(OK, I could enable the imx2_wdt driver and make sure with some udev
magic that the gpio watchdog is the one being fed by userspace. But
having two watchdogs fills me with some trepidation.)

Having said that, I wonder what the typical restart callback does
different from setting the timeout to a minimal value, start it and then
maybe call delay() to not return until the watchdog triggers. At least
that's what I would do for a watchdog that doesn't provide an explicit
.restart handler but has the provide-system-reset property.

In my eyes this is somewhat of a hardware property, but I can imagine
that others don't agree and argue this shouldn't go into the device
tree. @Rob: What is your position here?

Does this sound sane? Do we also need a property like
"no-provide-system-reset" to better maintain backward compatibility?
(which then would result in not registering the watchdog as reset
trigger even if the driver provides a .restart.)
Does someone know a better name for the property?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC] Using a watchdog as system reset

[Guenter Roeck]

[-- Attachment #1.1: Type: text/plain, Size: 2903 bytes --]

On 10/6/20 3:29 AM, Uwe Kleine-König wrote:
> Hello,
> 
> I have an i.MX25 system here with an external watchdog (using the
> gpio_wdt driver). So the internal watchdog (imx2_wdt) is unused.
> 
> The problem with the unused imx2_wdt is that this usually provides the
> restart handler and now a reboot ends with
> 
> 	reboot: Restarting system
> 	Reboot failed -- System halted
> 
> until eventually the watchdog bites and resets the machine.
> 
> I imagine that this is a common enough issue to warrant a generic
> solution. My suggestion is to formalize and implement something like:
> 
> 	watchdog {
> 		compatible = "linux,wdt-gpio";
> 		...
> 		provide-system-reset;
> 	}
> 
> with the sematic of: "This is the dedicated mechanism to reset this
> machine."
> 

Some systems have more than one means to reset it, which is why
restart handlers have a priority. This in turn suggests that we should
maybe have a means to set that priority dynamically for the imx2_wdt driver
(or for watchdog drivers in general) instead of having it fixed at 128.
That would also solve your problem, assuming there is a different
(currently lower priority) means to reset the hardware in your system.

Alternatively, can't you just blacklist the imx2-wdt driver ?

> (OK, I could enable the imx2_wdt driver and make sure with some udev
> magic that the gpio watchdog is the one being fed by userspace. But
> having two watchdogs fills me with some trepidation.)
> 

Hmm - that suggests that the reset may fail  because the reset code
in imx2_wdt doesn't work, not because it isn't wired.
If so, the driver code handling the reset may be buggy or incomplete.
Have you tried setting (or not setting) the fsl,ext-reset-output
property ?

> Having said that, I wonder what the typical restart callback does
> different from setting the timeout to a minimal value, start it and then
> maybe call delay() to not return until the watchdog triggers. At least
> that's what I would do for a watchdog that doesn't provide an explicit
> .restart handler but has the provide-system-reset property.

The intent of the callback was to handle situations where the watchdog
hardware also generates the system reset. The secondary use was for systems
which don't have a means to reset the system other than what you describe
above.

Guenter

> 
> In my eyes this is somewhat of a hardware property, but I can imagine
> that others don't agree and argue this shouldn't go into the device
> tree. @Rob: What is your position here?
> 
> Does this sound sane? Do we also need a property like
> "no-provide-system-reset" to better maintain backward compatibility?
> (which then would result in not registering the watchdog as reset
> trigger even if the driver provides a .restart.)
> Does someone know a better name for the property?
> 
> Best regards
> Uwe
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC] Using a watchdog as system reset

[Guenter Roeck]

[-- Attachment #1.1: Type: text/plain, Size: 3643 bytes --]

On 10/6/20 4:56 AM, Guenter Roeck wrote:
> On 10/6/20 3:29 AM, Uwe Kleine-König wrote:
>> Hello,
>>
>> I have an i.MX25 system here with an external watchdog (using the
>> gpio_wdt driver). So the internal watchdog (imx2_wdt) is unused.
>>
>> The problem with the unused imx2_wdt is that this usually provides the
>> restart handler and now a reboot ends with
>>
>> 	reboot: Restarting system
>> 	Reboot failed -- System halted
>>
>> until eventually the watchdog bites and resets the machine.
>>
>> I imagine that this is a common enough issue to warrant a generic
>> solution. My suggestion is to formalize and implement something like:
>>
>> 	watchdog {
>> 		compatible = "linux,wdt-gpio";
>> 		...
>> 		provide-system-reset;
>> 	}
>>
>> with the sematic of: "This is the dedicated mechanism to reset this
>> machine."
>>
> 
> Some systems have more than one means to reset it, which is why
> restart handlers have a priority. This in turn suggests that we should
> maybe have a means to set that priority dynamically for the imx2_wdt driver
> (or for watchdog drivers in general) instead of having it fixed at 128.
> That would also solve your problem, assuming there is a different
> (currently lower priority) means to reset the hardware in your system.
> 
> Alternatively, can't you just blacklist the imx2-wdt driver ?
> 

After having another couple hours of sleep and a coffee, I wonder if
this is already done, and the reboot just fails _because_ the imx2_wdt
is _not_ loaded. Is that the case ?

If so, it looks like you want the reset functionality of the imx_wdt driver
but not its watchdog functionality. And the above would be a suggestion
to add a "generic" restart functionality into the watchdog subsystem,
one that uses a watchdog with minimum timeout to reset the system,
even if its driver doesn't explicitly register a restart handler.
Is that what you are trying to suggest ?

Thanks,
Guenter

>> (OK, I could enable the imx2_wdt driver and make sure with some udev
>> magic that the gpio watchdog is the one being fed by userspace. But
>> having two watchdogs fills me with some trepidation.)
>>
> 
> Hmm - that suggests that the reset may fail  because the reset code
> in imx2_wdt doesn't work, not because it isn't wired.
> If so, the driver code handling the reset may be buggy or incomplete.
> Have you tried setting (or not setting) the fsl,ext-reset-output
> property ?
> 
>> Having said that, I wonder what the typical restart callback does
>> different from setting the timeout to a minimal value, start it and then
>> maybe call delay() to not return until the watchdog triggers. At least
>> that's what I would do for a watchdog that doesn't provide an explicit
>> .restart handler but has the provide-system-reset property.
> 
> The intent of the callback was to handle situations where the watchdog
> hardware also generates the system reset. The secondary use was for systems
> which don't have a means to reset the system other than what you describe
> above.
> 
> Guenter
> 
>>
>> In my eyes this is somewhat of a hardware property, but I can imagine
>> that others don't agree and argue this shouldn't go into the device
>> tree. @Rob: What is your position here?
>>
>> Does this sound sane? Do we also need a property like
>> "no-provide-system-reset" to better maintain backward compatibility?
>> (which then would result in not registering the watchdog as reset
>> trigger even if the driver provides a .restart.)
>> Does someone know a better name for the property?
>>
>> Best regards
>> Uwe
>>
> 
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC] Using a watchdog as system reset

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2784 bytes --]

Hello Guenter,

On Tue, Oct 06, 2020 at 07:29:11AM -0700, Guenter Roeck wrote:
> On 10/6/20 4:56 AM, Guenter Roeck wrote:
> > On 10/6/20 3:29 AM, Uwe Kleine-König wrote:
> >> Hello,
> >>
> >> I have an i.MX25 system here with an external watchdog (using the
> >> gpio_wdt driver). So the internal watchdog (imx2_wdt) is unused.
> >>
> >> The problem with the unused imx2_wdt is that this usually provides the
> >> restart handler and now a reboot ends with
> >>
> >> 	reboot: Restarting system
> >> 	Reboot failed -- System halted
> >>
> >> until eventually the watchdog bites and resets the machine.
> >>
> >> I imagine that this is a common enough issue to warrant a generic
> >> solution. My suggestion is to formalize and implement something like:
> >>
> >> 	watchdog {
> >> 		compatible = "linux,wdt-gpio";
> >> 		...
> >> 		provide-system-reset;
> >> 	}
> >>
> >> with the sematic of: "This is the dedicated mechanism to reset this
> >> machine."
> >>
> > 
> > Some systems have more than one means to reset it, which is why
> > restart handlers have a priority. This in turn suggests that we should
> > maybe have a means to set that priority dynamically for the imx2_wdt driver
> > (or for watchdog drivers in general) instead of having it fixed at 128.
> > That would also solve your problem, assuming there is a different
> > (currently lower priority) means to reset the hardware in your system.
> > 
> > Alternatively, can't you just blacklist the imx2-wdt driver ?
> 
> After having another couple hours of sleep and a coffee, I wonder if
> this is already done, and the reboot just fails _because_ the imx2_wdt
> is _not_ loaded. Is that the case ?

Right, I disabled the imx2_wdt driver.
 
> If so, it looks like you want the reset functionality of the imx_wdt driver
> but not its watchdog functionality.

My thought was to use the gpio-watchdog as reset source, but using the
imx-watchdog only for reset but not watchdog is an obvious alternative I
didn't think about.

So I either want to make the gpio-watchdog provide a restart handler or
use the imx-watchdog driver to only provide a restart handler (but no
watchdog functionality).

> And the above would be a suggestion to add a "generic" restart
> functionality into the watchdog subsystem, one that uses a watchdog
> with minimum timeout to reset the system, even if its driver doesn't
> explicitly register a restart handler.  Is that what you are trying to
> suggest ?

Yes, every watchdog could provide a restart handler with just using the
watchdog callbacks.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC] Using a watchdog as system reset

[Guenter Roeck]

[-- Attachment #1.1: Type: text/plain, Size: 3593 bytes --]

On 10/6/20 11:41 AM, Uwe Kleine-König wrote:
> Hello Guenter,
> 
> On Tue, Oct 06, 2020 at 07:29:11AM -0700, Guenter Roeck wrote:
>> On 10/6/20 4:56 AM, Guenter Roeck wrote:
>>> On 10/6/20 3:29 AM, Uwe Kleine-König wrote:
>>>> Hello,
>>>>
>>>> I have an i.MX25 system here with an external watchdog (using the
>>>> gpio_wdt driver). So the internal watchdog (imx2_wdt) is unused.
>>>>
>>>> The problem with the unused imx2_wdt is that this usually provides the
>>>> restart handler and now a reboot ends with
>>>>
>>>> 	reboot: Restarting system
>>>> 	Reboot failed -- System halted
>>>>
>>>> until eventually the watchdog bites and resets the machine.
>>>>
>>>> I imagine that this is a common enough issue to warrant a generic
>>>> solution. My suggestion is to formalize and implement something like:
>>>>
>>>> 	watchdog {
>>>> 		compatible = "linux,wdt-gpio";
>>>> 		...
>>>> 		provide-system-reset;
>>>> 	}
>>>>
>>>> with the sematic of: "This is the dedicated mechanism to reset this
>>>> machine."
>>>>
>>>
>>> Some systems have more than one means to reset it, which is why
>>> restart handlers have a priority. This in turn suggests that we should
>>> maybe have a means to set that priority dynamically for the imx2_wdt driver
>>> (or for watchdog drivers in general) instead of having it fixed at 128.
>>> That would also solve your problem, assuming there is a different
>>> (currently lower priority) means to reset the hardware in your system.
>>>
>>> Alternatively, can't you just blacklist the imx2-wdt driver ?
>>
>> After having another couple hours of sleep and a coffee, I wonder if
>> this is already done, and the reboot just fails _because_ the imx2_wdt
>> is _not_ loaded. Is that the case ?
> 
> Right, I disabled the imx2_wdt driver.
>  
>> If so, it looks like you want the reset functionality of the imx_wdt driver
>> but not its watchdog functionality.
> 
> My thought was to use the gpio-watchdog as reset source, but using the
> imx-watchdog only for reset but not watchdog is an obvious alternative I
> didn't think about.
> 
It isn't really something I would have thought to ever be relevant: If
a watchdog can be used to reset the system, and that method to reset
the system is known to work and supposed to be used, why not use it as
system watchdog ? So that use case is quite odd, especially since the
watchdog on that system can apparently be used to trigger an external
pin.

If we assume that there was a reason for not using the SoC watchdog,
we must also assume that using it to reset the system does not really
work (otherwise, what would be the point of having a separate gpio
based watchdog in that system ?).

With that in mind, your other option kind of makes sense. The only
question would be how to express this in devicetree. I am certainly
open to accepting a patch introducing such a property/functionality
into the watchdog core.

Thanks,
Guenter

> So I either want to make the gpio-watchdog provide a restart handler or
> use the imx-watchdog driver to only provide a restart handler (but no
> watchdog functionality).
> 
>> And the above would be a suggestion to add a "generic" restart
>> functionality into the watchdog subsystem, one that uses a watchdog
>> with minimum timeout to reset the system, even if its driver doesn't
>> explicitly register a restart handler.  Is that what you are trying to
>> suggest ?
> 
> Yes, every watchdog could provide a restart handler with just using the
> watchdog callbacks.
> 
> Best regards
> Uwe
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC] Using a watchdog as system reset

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3923 bytes --]

Hello Guenter,

On Tue, Oct 06, 2020 at 02:04:10PM -0700, Guenter Roeck wrote:
> On 10/6/20 11:41 AM, Uwe Kleine-König wrote:
> > Hello Guenter,
> > 
> > On Tue, Oct 06, 2020 at 07:29:11AM -0700, Guenter Roeck wrote:
> >> On 10/6/20 4:56 AM, Guenter Roeck wrote:
> >>> On 10/6/20 3:29 AM, Uwe Kleine-König wrote:
> >>>> Hello,
> >>>>
> >>>> I have an i.MX25 system here with an external watchdog (using the
> >>>> gpio_wdt driver). So the internal watchdog (imx2_wdt) is unused.
> >>>>
> >>>> The problem with the unused imx2_wdt is that this usually provides the
> >>>> restart handler and now a reboot ends with
> >>>>
> >>>> 	reboot: Restarting system
> >>>> 	Reboot failed -- System halted
> >>>>
> >>>> until eventually the watchdog bites and resets the machine.
> >>>>
> >>>> I imagine that this is a common enough issue to warrant a generic
> >>>> solution. My suggestion is to formalize and implement something like:
> >>>>
> >>>> 	watchdog {
> >>>> 		compatible = "linux,wdt-gpio";
> >>>> 		...
> >>>> 		provide-system-reset;
> >>>> 	}
> >>>>
> >>>> with the sematic of: "This is the dedicated mechanism to reset this
> >>>> machine."
> >>>>
> >>>
> >>> Some systems have more than one means to reset it, which is why
> >>> restart handlers have a priority. This in turn suggests that we should
> >>> maybe have a means to set that priority dynamically for the imx2_wdt driver
> >>> (or for watchdog drivers in general) instead of having it fixed at 128.
> >>> That would also solve your problem, assuming there is a different
> >>> (currently lower priority) means to reset the hardware in your system.
> >>>
> >>> Alternatively, can't you just blacklist the imx2-wdt driver ?
> >>
> >> After having another couple hours of sleep and a coffee, I wonder if
> >> this is already done, and the reboot just fails _because_ the imx2_wdt
> >> is _not_ loaded. Is that the case ?
> > 
> > Right, I disabled the imx2_wdt driver.
> >  
> >> If so, it looks like you want the reset functionality of the imx_wdt driver
> >> but not its watchdog functionality.
> > 
> > My thought was to use the gpio-watchdog as reset source, but using the
> > imx-watchdog only for reset but not watchdog is an obvious alternative I
> > didn't think about.
>
> It isn't really something I would have thought to ever be relevant: If
> a watchdog can be used to reset the system, and that method to reset
> the system is known to work and supposed to be used, why not use it as
> system watchdog ? So that use case is quite odd, especially since the
> watchdog on that system can apparently be used to trigger an external
> pin.

The motivation to use the external watchdog is that access to the MRAM
only works if the external watchdog is active. And (if I'm well
informed) this external watchdog was introduced because of some
regulation stuff where it must be guaranteed that there is a watchdog.
With an external watchdog this is much easier to do than with an SoC
internal one.

And then because using two watchdogs is ugly disabling the internal one
was straight forward even though it works just fine (apart from enabling
access to the MRAM).

> If we assume that there was a reason for not using the SoC watchdog,
> we must also assume that using it to reset the system does not really
> work (otherwise, what would be the point of having a separate gpio
> based watchdog in that system ?).
> 
> With that in mind, your other option kind of makes sense. The only
> question would be how to express this in devicetree. I am certainly
> open to accepting a patch introducing such a property/functionality
> into the watchdog core.

OK, will try to come up with a patch.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC] Using a watchdog as system reset

[Ahmad Fatoum]

Hello Uwe,

On 10/7/20 9:12 AM, Uwe Kleine-König wrote:
> Hello Guenter,
> 
> On Tue, Oct 06, 2020 at 02:04:10PM -0700, Guenter Roeck wrote:
>> On 10/6/20 11:41 AM, Uwe Kleine-König wrote:
>>> Hello Guenter,
>>>
>>> On Tue, Oct 06, 2020 at 07:29:11AM -0700, Guenter Roeck wrote:
>>>> On 10/6/20 4:56 AM, Guenter Roeck wrote:
>>>>> On 10/6/20 3:29 AM, Uwe Kleine-König wrote:
>>>>>> Hello,
>>>>>>
>>>>>> I have an i.MX25 system here with an external watchdog (using the
>>>>>> gpio_wdt driver). So the internal watchdog (imx2_wdt) is unused.
>>>>>>
>>>>>> The problem with the unused imx2_wdt is that this usually provides the
>>>>>> restart handler and now a reboot ends with
>>>>>>
>>>>>> 	reboot: Restarting system
>>>>>> 	Reboot failed -- System halted
>>>>>>
>>>>>> until eventually the watchdog bites and resets the machine.
>>>>>>
>>>>>> I imagine that this is a common enough issue to warrant a generic
>>>>>> solution. My suggestion is to formalize and implement something like:
>>>>>>
>>>>>> 	watchdog {
>>>>>> 		compatible = "linux,wdt-gpio";
>>>>>> 		...
>>>>>> 		provide-system-reset;
>>>>>> 	}
>>>>>>
>>>>>> with the sematic of: "This is the dedicated mechanism to reset this
>>>>>> machine."
>>>>>>
>>>>>
>>>>> Some systems have more than one means to reset it, which is why
>>>>> restart handlers have a priority. This in turn suggests that we should
>>>>> maybe have a means to set that priority dynamically for the imx2_wdt driver
>>>>> (or for watchdog drivers in general) instead of having it fixed at 128.
>>>>> That would also solve your problem, assuming there is a different
>>>>> (currently lower priority) means to reset the hardware in your system.
>>>>>
>>>>> Alternatively, can't you just blacklist the imx2-wdt driver ?
>>>>
>>>> After having another couple hours of sleep and a coffee, I wonder if
>>>> this is already done, and the reboot just fails _because_ the imx2_wdt
>>>> is _not_ loaded. Is that the case ?
>>>
>>> Right, I disabled the imx2_wdt driver.
>>>  
>>>> If so, it looks like you want the reset functionality of the imx_wdt driver
>>>> but not its watchdog functionality.
>>>
>>> My thought was to use the gpio-watchdog as reset source, but using the
>>> imx-watchdog only for reset but not watchdog is an obvious alternative I
>>> didn't think about.
>>
>> It isn't really something I would have thought to ever be relevant: If
>> a watchdog can be used to reset the system, and that method to reset
>> the system is known to work and supposed to be used, why not use it as
>> system watchdog ? So that use case is quite odd, especially since the
>> watchdog on that system can apparently be used to trigger an external
>> pin.
> 
> The motivation to use the external watchdog is that access to the MRAM
> only works if the external watchdog is active. And (if I'm well
> informed) this external watchdog was introduced because of some
> regulation stuff where it must be guaranteed that there is a watchdog.
> With an external watchdog this is much easier to do than with an SoC
> internal one.
> 
> And then because using two watchdogs is ugly disabling the internal one
> was straight forward even though it works just fine (apart from enabling
> access to the MRAM).
> 
>> If we assume that there was a reason for not using the SoC watchdog,
>> we must also assume that using it to reset the system does not really
>> work (otherwise, what would be the point of having a separate gpio
>> based watchdog in that system ?).
>>
>> With that in mind, your other option kind of makes sense. The only
>> question would be how to express this in devicetree. I am certainly
>> open to accepting a patch introducing such a property/functionality
>> into the watchdog core.
> 
> OK, will try to come up with a patch.

Instead of having a `provide-system-reset' property, how about providing
it unconditionally, but with a very low priority?

This can be coupled with Guenther's suggestion of having a dynamic
way to set the priority, e.g. a `watchdog-priority' property in the device
tree that's common to all watchdogs? That's the way barebox is handling
multiple watchdogs (default value in driver overridable in DT and at runtime).

What's the DT folks opinion on that?

Cheers,
Ahmad

> 
> Best regards
> Uwe
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [RFC] Using a watchdog as system reset

[Ahmad Fatoum]

Hello,

On 10/7/20 9:25 AM, Ahmad Fatoum wrote:
>>> into the watchdog core.
>>
>> OK, will try to come up with a patch.
> 
> Instead of having a `provide-system-reset' property, how about providing
> it unconditionally, but with a very low priority?
> 
> This can be coupled with Guenther's suggestion of having a dynamic
> way to set the priority, e.g. a `watchdog-priority' property in the device
> tree that's common to all watchdogs? That's the way barebox is handling
> multiple watchdogs (default value in driver overridable in DT and at runtime).
> 
> What's the DT folks opinion on that?

Correction: s/watchdog-priority/reset-priority/

> 
> Cheers,
> Ahmad
> 
>>
>> Best regards
>> Uwe
>>
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

dt-binding to define default watchdog and machine reset (Was: Re: [RFC] Using a watchdog as system reset)

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1904 bytes --]

Hello,

[promoted Rob from Cc: to To: and adapted the subject in the hope to get
some feedback]

On Wed, Oct 07, 2020 at 09:25:30AM +0200, Ahmad Fatoum wrote:
> On 10/7/20 9:12 AM, Uwe Kleine-König wrote:
> > On Tue, Oct 06, 2020 at 02:04:10PM -0700, Guenter Roeck wrote:
> >> With that in mind, your other option kind of makes sense. The only
> >> question would be how to express this in devicetree. I am certainly
> >> open to accepting a patch introducing such a property/functionality
> >> into the watchdog core.
> > 
> > OK, will try to come up with a patch.
> 
> Instead of having a `provide-system-reset' property, how about providing
> it unconditionally, but with a very low priority?
> 
> This can be coupled with Guenther's suggestion of having a dynamic
> way to set the priority, e.g. a `watchdog-priority' property in the device
> tree that's common to all watchdogs? That's the way barebox is handling
> multiple watchdogs (default value in driver overridable in DT and at runtime).

OK, I'll try to put this in more verbose words:

Let's introduce a generic watchdog property `watchdog-priority' that
provides a u32 to order the watchdogs for systems having two or more.
The value 0 means the watchdog is unusable/broken/disabled and the
watchdog with the biggest value is the one supposed to be used by
default.

Analogous a property `watchdog-restart-priority` is used to define if a
watchdog is supposed to be used to restart the machine. Again a value of
0 means "Don't use" and otherwise the highest-value watchdog is used to
reset the machine.

Maybe `restart-priority` is a better name that can also be used by
PMICs?!

> What's the DT folks opinion on that?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: dt-binding to define default watchdog and machine reset (Was: Re: [RFC] Using a watchdog as system reset)

[Guenter Roeck]

On 10/7/20 3:18 AM, Uwe Kleine-König wrote:
> Hello,
> 
> [promoted Rob from Cc: to To: and adapted the subject in the hope to get
> some feedback]
> 
> On Wed, Oct 07, 2020 at 09:25:30AM +0200, Ahmad Fatoum wrote:
>> On 10/7/20 9:12 AM, Uwe Kleine-König wrote:
>>> On Tue, Oct 06, 2020 at 02:04:10PM -0700, Guenter Roeck wrote:
>>>> With that in mind, your other option kind of makes sense. The only
>>>> question would be how to express this in devicetree. I am certainly
>>>> open to accepting a patch introducing such a property/functionality
>>>> into the watchdog core.
>>>
>>> OK, will try to come up with a patch.
>>
>> Instead of having a `provide-system-reset' property, how about providing
>> it unconditionally, but with a very low priority?
>>
Personally I don't think that would be a good idea, first in general but
also because the generic restart handling mechanism is still not universally
used (the last five or so patches needed to make it complete have been
blocked by various people each time they were submitted, so we can be sure
that they will never be accepted).

Also, you would have to have a means to disable it, which would be equivalent
to having a means to enable it, so making it enabled by default seems pointless
and would unnecessarily introduce potential problems.

>> This can be coupled with Guenther's suggestion of having a dynamic
>> way to set the priority, e.g. a `watchdog-priority' property in the device
>> tree that's common to all watchdogs? That's the way barebox is handling
>> multiple watchdogs (default value in driver overridable in DT and at runtime).
> 
> OK, I'll try to put this in more verbose words:
> 
> Let's introduce a generic watchdog property `watchdog-priority' that
> provides a u32 to order the watchdogs for systems having two or more.
> The value 0 means the watchdog is unusable/broken/disabled and the
> watchdog with the biggest value is the one supposed to be used by
> default.
> 

How do you suggest to implement that ? Device naming is determined
by registration order. The watchdog subsystem doesn't decide which of
the watchdogs is being used; userspace does that by opening the
watchdog device. Userspace can already decide which watchdog to use
by checking its sysfs attributes. If we were to create a sysfs attribute
for userspace to read and compare, userspace could as well use the existing
'identity' attribute to make that decision.

> Analogous a property `watchdog-restart-priority` is used to define if a
> watchdog is supposed to be used to restart the machine. Again a value of
> 0 means "Don't use" and otherwise the highest-value watchdog is used to
> reset the machine.
> 

That makes more sense to me.

Guenter

> Maybe `restart-priority` is a better name that can also be used by
> PMICs?!
> 
>> What's the DT folks opinion on that?
> 
> Best regards
> Uwe
> 

Re: dt-binding to define default watchdog and machine reset (Was: Re: [RFC] Using a watchdog as system reset)

[Ahmad Fatoum]

Hello,

On 10/7/20 1:04 PM, Guenter Roeck wrote:
> On 10/7/20 3:18 AM, Uwe Kleine-König wrote:
>> Let's introduce a generic watchdog property `watchdog-priority' that
>> provides a u32 to order the watchdogs for systems having two or more.
>> The value 0 means the watchdog is unusable/broken/disabled and the
>> watchdog with the biggest value is the one supposed to be used by
>> default.
>>
> 
> How do you suggest to implement that ? Device naming is determined
> by registration order. The watchdog subsystem doesn't decide which of
> the watchdogs is being used; userspace does that by opening the
> watchdog device. Userspace can already decide which watchdog to use
> by checking its sysfs attributes. If we were to create a sysfs attribute
> for userspace to read and compare, userspace could as well use the existing
> 'identity' attribute to make that decision.

Might be relevant for users of CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED, so
the kernel only starts that one watchdog? This seems out-of-scope for
Uwe's Problem though.

>> Analogous a property `watchdog-restart-priority` is used to define if a
>> watchdog is supposed to be used to restart the machine. Again a value of
>> 0 means "Don't use" and otherwise the highest-value watchdog is used to
>> reset the machine.
>>
> 
> That makes more sense to me.

Yes, such a `restart-priority' is what I had in mind. Preferably covering
PMICs as well.

Cheers,
Ahmad

> 
> Guenter
> 
>> Maybe `restart-priority` is a better name that can also be used by
>> PMICs?!
>>
>>> What's the DT folks opinion on that?
>>
>> Best regards
>> Uwe
>>
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |