* [RFC] Using a watchdog as system reset
@ 2020-10-06 10:29 Uwe Kleine-König
2020-10-06 11:56 ` Guenter Roeck
0 siblings, 1 reply; 11+ messages in thread
From: Uwe Kleine-König @ 2020-10-06 10:29 UTC (permalink / raw)
To: Wim Van Sebroeck, Guenter Roeck, Rob Herring, Frank Rowand
Cc: linux-watchdog, devicetree, kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [RFC] Using a watchdog as system reset 2020-10-06 10:29 [RFC] Using a watchdog as system reset Uwe Kleine-König @ 2020-10-06 11:56 ` Guenter Roeck 2020-10-06 14:29 ` Guenter Roeck 0 siblings, 1 reply; 11+ messages in thread From: Guenter Roeck @ 2020-10-06 11:56 UTC (permalink / raw) To: Uwe Kleine-König, Wim Van Sebroeck, Rob Herring, Frank Rowand Cc: linux-watchdog, devicetree, kernel [-- 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 --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] Using a watchdog as system reset 2020-10-06 11:56 ` Guenter Roeck @ 2020-10-06 14:29 ` Guenter Roeck 2020-10-06 18:41 ` Uwe Kleine-König 0 siblings, 1 reply; 11+ messages in thread From: Guenter Roeck @ 2020-10-06 14:29 UTC (permalink / raw) To: Uwe Kleine-König, Wim Van Sebroeck, Rob Herring, Frank Rowand Cc: linux-watchdog, devicetree, kernel [-- 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 --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] Using a watchdog as system reset 2020-10-06 14:29 ` Guenter Roeck @ 2020-10-06 18:41 ` Uwe Kleine-König 2020-10-06 21:04 ` Guenter Roeck 0 siblings, 1 reply; 11+ messages in thread From: Uwe Kleine-König @ 2020-10-06 18:41 UTC (permalink / raw) To: Guenter Roeck Cc: Wim Van Sebroeck, Rob Herring, Frank Rowand, devicetree, linux-watchdog, kernel [-- 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 --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] Using a watchdog as system reset 2020-10-06 18:41 ` Uwe Kleine-König @ 2020-10-06 21:04 ` Guenter Roeck 2020-10-07 7:12 ` Uwe Kleine-König 0 siblings, 1 reply; 11+ messages in thread From: Guenter Roeck @ 2020-10-06 21:04 UTC (permalink / raw) To: Uwe Kleine-König Cc: Wim Van Sebroeck, Rob Herring, Frank Rowand, devicetree, linux-watchdog, kernel [-- 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 --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] Using a watchdog as system reset 2020-10-06 21:04 ` Guenter Roeck @ 2020-10-07 7:12 ` Uwe Kleine-König 2020-10-07 7:25 ` Ahmad Fatoum 0 siblings, 1 reply; 11+ messages in thread From: Uwe Kleine-König @ 2020-10-07 7:12 UTC (permalink / raw) To: Guenter Roeck Cc: devicetree, linux-watchdog, Rob Herring, kernel, Wim Van Sebroeck, Frank Rowand [-- 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 --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] Using a watchdog as system reset 2020-10-07 7:12 ` Uwe Kleine-König @ 2020-10-07 7:25 ` Ahmad Fatoum 2020-10-07 7:32 ` Ahmad Fatoum 2020-10-07 10:18 ` dt-binding to define default watchdog and machine reset (Was: Re: [RFC] Using a watchdog as system reset) Uwe Kleine-König 0 siblings, 2 replies; 11+ messages in thread From: Ahmad Fatoum @ 2020-10-07 7:25 UTC (permalink / raw) To: Uwe Kleine-König, Guenter Roeck Cc: devicetree, linux-watchdog, Rob Herring, kernel, Wim Van Sebroeck, Frank Rowand 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 | ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] Using a watchdog as system reset 2020-10-07 7:25 ` Ahmad Fatoum @ 2020-10-07 7:32 ` Ahmad Fatoum 2020-10-07 10:18 ` dt-binding to define default watchdog and machine reset (Was: Re: [RFC] Using a watchdog as system reset) Uwe Kleine-König 1 sibling, 0 replies; 11+ messages in thread From: Ahmad Fatoum @ 2020-10-07 7:32 UTC (permalink / raw) To: Uwe Kleine-König, Guenter Roeck Cc: devicetree, linux-watchdog, Rob Herring, kernel, Wim Van Sebroeck, Frank Rowand 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 | ^ permalink raw reply [flat|nested] 11+ messages in thread
* dt-binding to define default watchdog and machine reset (Was: Re: [RFC] Using a watchdog as system reset) 2020-10-07 7:25 ` Ahmad Fatoum 2020-10-07 7:32 ` Ahmad Fatoum @ 2020-10-07 10:18 ` Uwe Kleine-König 2020-10-07 11:04 ` Guenter Roeck 1 sibling, 1 reply; 11+ messages in thread From: Uwe Kleine-König @ 2020-10-07 10:18 UTC (permalink / raw) To: Ahmad Fatoum, Rob Herring Cc: Guenter Roeck, devicetree, linux-watchdog, kernel, Wim Van Sebroeck, Frank Rowand [-- 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 --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: dt-binding to define default watchdog and machine reset (Was: Re: [RFC] Using a watchdog as system reset) 2020-10-07 10:18 ` dt-binding to define default watchdog and machine reset (Was: Re: [RFC] Using a watchdog as system reset) Uwe Kleine-König @ 2020-10-07 11:04 ` Guenter Roeck 2020-10-07 11:35 ` Ahmad Fatoum 0 siblings, 1 reply; 11+ messages in thread From: Guenter Roeck @ 2020-10-07 11:04 UTC (permalink / raw) To: Uwe Kleine-König, Ahmad Fatoum, Rob Herring Cc: devicetree, linux-watchdog, kernel, Wim Van Sebroeck, Frank Rowand 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: dt-binding to define default watchdog and machine reset (Was: Re: [RFC] Using a watchdog as system reset) 2020-10-07 11:04 ` Guenter Roeck @ 2020-10-07 11:35 ` Ahmad Fatoum 0 siblings, 0 replies; 11+ messages in thread From: Ahmad Fatoum @ 2020-10-07 11:35 UTC (permalink / raw) To: Guenter Roeck, Uwe Kleine-König, Rob Herring Cc: devicetree, linux-watchdog, kernel, Wim Van Sebroeck, Frank Rowand 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 | ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-10-07 11:35 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-06 10:29 [RFC] Using a watchdog as system reset Uwe Kleine-König 2020-10-06 11:56 ` Guenter Roeck 2020-10-06 14:29 ` Guenter Roeck 2020-10-06 18:41 ` Uwe Kleine-König 2020-10-06 21:04 ` Guenter Roeck 2020-10-07 7:12 ` Uwe Kleine-König 2020-10-07 7:25 ` Ahmad Fatoum 2020-10-07 7:32 ` Ahmad Fatoum 2020-10-07 10:18 ` dt-binding to define default watchdog and machine reset (Was: Re: [RFC] Using a watchdog as system reset) Uwe Kleine-König 2020-10-07 11:04 ` Guenter Roeck 2020-10-07 11:35 ` Ahmad Fatoum
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).