| Message ID | 20210226141411.2517368-1-linux@rasmusvillemoes.dk |
|---|---|
| Headers | show |
| Series | add ripple counter dt binding and driver | expand |
On Fri, Feb 26, 2021 at 3:14 PM Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > > So I'm thinking that the proper way to handle this is to be able to > represent that ripple counter as a clock consumer in DT and have a > driver do the clk_prepare_enable(), even if that driver doesn't and > can't do anything else. But I'm certainly open to other suggestions. How about adding support for the optional clock to the gpio_wdt driver, would that work? Arnd
On 26/02/2021 15.35, Arnd Bergmann wrote: > On Fri, Feb 26, 2021 at 3:14 PM Rasmus Villemoes > <linux@rasmusvillemoes.dk> wrote: > >> >> So I'm thinking that the proper way to handle this is to be able to >> represent that ripple counter as a clock consumer in DT and have a >> driver do the clk_prepare_enable(), even if that driver doesn't and >> can't do anything else. But I'm certainly open to other suggestions. > > How about adding support for the optional clock to the gpio_wdt driver, > would that work? I think it would _work_ (all I need is some piece of code doing the clock_prepare_enable(), and until now we've just stashed that in some otherwise unrelated out-of-tree driver, but we're trying to get rid of that one), but the watchdog chip isn't really the consumer of the clock signal, so in-so-far as DT is supposed to describe the hardware, I don't think it's appropriate. OTOH, one could argue that the watchdog chip and the ripple counter together constitute the watchdog circuit. Cc += watchdog maintainers. Context: I have a gpio-wdt which can unfortunately effectively be disabled by disabling a clock output, and that happens automatically unless the clock has a consumer in DT. But the actual consumer is not the gpio-wdt. Please see https://lore.kernel.org/lkml/20210226141411.2517368-1-linux@rasmusvillemoes.dk/ for the original thread. Rasmus
On 26/02/2021 20.53, Guenter Roeck wrote: > On 2/26/21 8:35 AM, Rasmus Villemoes wrote: >> On 26/02/2021 15.35, Arnd Bergmann wrote: >>> On Fri, Feb 26, 2021 at 3:14 PM Rasmus Villemoes >>> <linux@rasmusvillemoes.dk> wrote: >>> >>>> >>>> So I'm thinking that the proper way to handle this is to be able to >>>> represent that ripple counter as a clock consumer in DT and have a >>>> driver do the clk_prepare_enable(), even if that driver doesn't and >>>> can't do anything else. But I'm certainly open to other suggestions. >>> >>> How about adding support for the optional clock to the gpio_wdt driver, >>> would that work? >> >> I think it would _work_ (all I need is some piece of code doing the >> clock_prepare_enable(), and until now we've just stashed that in some >> otherwise unrelated out-of-tree driver, but we're trying to get rid of >> that one), but the watchdog chip isn't really the consumer of the clock >> signal, so in-so-far as DT is supposed to describe the hardware, I don't >> think it's appropriate. >> >> OTOH, one could argue that the watchdog chip and the ripple counter >> together constitute the watchdog circuit. >> >> Cc += watchdog maintainers. Context: I have a gpio-wdt which can >> unfortunately effectively be disabled by disabling a clock output, and >> that happens automatically unless the clock has a consumer in DT. But >> the actual consumer is not the gpio-wdt. >> Please see >> https://lore.kernel.org/lkml/20210226141411.2517368-1-linux@rasmusvillemoes.dk/ >> for the original thread. >> > > Sorry, I am missing something. If the watchdog is controlled by the clock, > it is a consumer of that clock. But that's just it, the watchdog chip is _not_ a consumer of the clock - I don't think I've ever seen a gpio_wdt that is not internally clocked, but even if they exist, that's not the case for this board. What else does "consumer" mean ? And why > not just add optional clock support to the gpio_wdt driver ? Because, the consumer is a piece of electronics sitting _between_ the watchdog chip's reset output and the SOCs reset pin, namely the ripple counter that implements a 64 ms delay from the watchdog fires till the actual reset. (The watchdog's reset is also routed directly to an interrupt; so software gets a 64 ms warning that a hard reset is imminent). Rasmus
On 3/1/21 1:44 AM, Arnd Bergmann wrote: > On Mon, Mar 1, 2021 at 9:34 AM Rasmus Villemoes > <linux@rasmusvillemoes.dk> wrote: >> On 26/02/2021 20.53, Guenter Roeck wrote: >>> >>> Sorry, I am missing something. If the watchdog is controlled by the clock, >>> it is a consumer of that clock. >> >> But that's just it, the watchdog chip is _not_ a consumer of the clock - >> I don't think I've ever seen a gpio_wdt that is not internally clocked, >> but even if they exist, that's not the case for this board. >> >> What else does "consumer" mean ? And why >>> not just add optional clock support to the gpio_wdt driver ? >> >> Because, the consumer is a piece of electronics sitting _between_ the >> watchdog chip's reset output and the SOCs reset pin, namely the ripple >> counter that implements a 64 ms delay from the watchdog fires till the >> actual reset. (The watchdog's reset is also routed directly to an >> interrupt; so software gets a 64 ms warning that a hard reset is imminent). > > I think it's a question of how you look at what the gpio_wdt device is. > While physical gpio chip is not a consumer of the clock, I agree with > Guenter that the conceptual device is: The functionality of the watchdog > in this case is provided by the combination of the external chip with the > ripple counter. I think it is therefore appropriate to have the gpio_wdt > and the driver refer to the clock as part of the watchdog. > I agree. All electronics needed for the watchdog to operate is part of the watchdog, and for me that includes the circuitry that connects it to the reset pin. The clock is needed for proper watchdog operation, so I would consider the watchdog to be a consumer. Guenter
As Arnd and Guenther suggested, this adds support to the gpio_wdt driver for being a consumer of the clock driving the ripple counter. However, I don't think it should be merged as-is, see below. The first patch makes sense on its own, quick grepping suggests plenty of places that could benefit from this, and I thought it would be odd to have to re-introduce a .remove callback in the gpio_wdt driver. Unfortunately, this turns out to be a bit of an "operation succeeded, patient (almost) died": We use CONFIG_GPIO_WATCHDOG_ARCH_INITCALL because the watchdog has a rather short timeout (1.0-2.25s, 1.6s typical according to data sheet). At first, I put the new code right after the devm_gpiod_get(), but the problem is that this early, we get -EPROBE_DEFER since the clock provider (the RTC which sits off i2c) isn't probed yet. But then the board would reset because it takes way too long for the rest of the machine to initialize. [The bootloader makes sure to turn on the RTC's clock output so the watchdog is actually functional, the task here is to figure out the proper way to prevent clk_disable_unused() from disabling it.] Moving the logic to after the first "is it always-running and if so give it an initial ping" made the board survive, but unfortunately the second, and succesful, probe happens a little more than a second later, which happens to work on this particular board, but is obviously not suitable for production given that it's already above what the spec says, and other random changes in the future might make the gap even wider. So I don't know. The hardware is obviously misdesigned, and I don't know how far the mainline kernel should stretch to support this; OTOH the kernel does contain lots of workarounds for quirks and hardware bugs. Rasmus Villemoes (3): clk: add devm_clk_prepare_enable() helper dt-bindings: watchdog: add optional "delay" clock to gpio-wdt binding watchdog: gpio_wdt: implement support for optional "delay" clock .../devicetree/bindings/watchdog/gpio-wdt.txt | 6 ++++ .../driver-api/driver-model/devres.rst | 1 + drivers/clk/clk-devres.c | 29 +++++++++++++++++++ drivers/watchdog/gpio_wdt.c | 9 ++++++ include/linux/clk.h | 13 +++++++++ 5 files changed, 58 insertions(+)
On Fri, Feb 26, 2021 at 03:14:10PM +0100, Rasmus Villemoes wrote: > While a ripple counter can not usually be interfaced with (directly) > from software, it may still be a crucial component in a board > layout. To prevent its input clock from being disabled by the clock > core because it apparently has no consumer, one needs to be able to > represent that consumer in DT. I'm okay with this as it is describing h/w, but we already 'protected-clocks' property which should work. > > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> > --- > Documentation/devicetree/bindings/misc/ripple-ctr.txt | 8 ++++++++ > 1 file changed, 8 insertions(+) > create mode 100644 Documentation/devicetree/bindings/misc/ripple-ctr.txt > > diff --git a/Documentation/devicetree/bindings/misc/ripple-ctr.txt b/Documentation/devicetree/bindings/misc/ripple-ctr.txt > new file mode 100644 > index 000000000000..1497d3a237a7 > --- /dev/null > +++ b/Documentation/devicetree/bindings/misc/ripple-ctr.txt > @@ -0,0 +1,8 @@ > +Generic ripple counter > + > +A ripple counter is a simple component that can for example be used to > +delay propagation of a signal. > + > +Required properties: > +- compatible: Must be "linux,ripple-ctr". Nothing linux specific about this. > +- clocks: Input clock specifier. Refer to common clock bindings. > -- > 2.29.2 >
On 08/03/2021 18.21, Rob Herring wrote: > On Fri, Feb 26, 2021 at 03:14:10PM +0100, Rasmus Villemoes wrote: >> While a ripple counter can not usually be interfaced with (directly) >> from software, it may still be a crucial component in a board >> layout. To prevent its input clock from being disabled by the clock >> core because it apparently has no consumer, one needs to be able to >> represent that consumer in DT. > > I'm okay with this as it is describing h/w, but we already > 'protected-clocks' property which should work. Hm. Unless https://lore.kernel.org/lkml/20200903040015.5627-2-samuel@sholland.org/ gets merged, I don't see how this would work out-of-the-box. Note that I sent a completely different v2, which made the gpio-wdt the clock consumer based on feedback from Guenter and Arnd, but that v2 isn't suitable for our case because it post-poned handling of the watchdog till after i2c is ready, which is too late. Somewhat similar to https://lore.kernel.org/lkml/20210222171247.97609-2-sebastian.reichel@collabora.com/ it seems. >> + >> +Required properties: >> +- compatible: Must be "linux,ripple-ctr". > > Nothing linux specific about this. True, but I was following the lead of the existing gpio-wdt binding. Is there some other "vendor" name one can and should use for completely generic and simple components like these? "generic"? Rasmus
On Mon, Mar 08, 2021 at 09:02:29PM +0100, Rasmus Villemoes wrote: > On 08/03/2021 18.21, Rob Herring wrote: > > On Fri, Feb 26, 2021 at 03:14:10PM +0100, Rasmus Villemoes wrote: > >> While a ripple counter can not usually be interfaced with (directly) > >> from software, it may still be a crucial component in a board > >> layout. To prevent its input clock from being disabled by the clock > >> core because it apparently has no consumer, one needs to be able to > >> represent that consumer in DT. > > > > I'm okay with this as it is describing h/w, but we already > > 'protected-clocks' property which should work. > > Hm. Unless > https://lore.kernel.org/lkml/20200903040015.5627-2-samuel@sholland.org/ > gets merged, I don't see how this would work out-of-the-box. Hum, no really clear what the hold up is there given it seems it was asked for. Letting it sit for 5 months is certainly not the way to get it merged. Anyways, that's the kernel's problem, not mine as far as DT bindings are concerned. > > Note that I sent a completely different v2, which made the gpio-wdt the > clock consumer based on feedback from Guenter and Arnd, but that v2 > isn't suitable for our case because it post-poned handling of the > watchdog till after i2c is ready, which is too late. Somewhat similar to > https://lore.kernel.org/lkml/20210222171247.97609-2-sebastian.reichel@collabora.com/ > it seems. Now at that one in my queue... I think 'protected-clocks' is the best way to avoid any driver probe ordering issues. It's the only thing that really captures don't turn off this clock. The ripple counter binding doesn't really capture that or what it is related to. Also, the ripple-counter driver could be a module and you'd still have the same issue as v2. > >> + > >> +Required properties: > >> +- compatible: Must be "linux,ripple-ctr". > > > > Nothing linux specific about this. > > True, but I was following the lead of the existing gpio-wdt binding. Is > there some other "vendor" name one can and should use for completely > generic and simple components like these? "generic"? Most 'generic' and GPIO based interfaces have no vendor prefix. Rob
On 3/4/21 2:12 PM, Rasmus Villemoes wrote: > As Arnd and Guenther suggested, this adds support to the gpio_wdt > driver for being a consumer of the clock driving the ripple > counter. However, I don't think it should be merged as-is, see below. > > The first patch makes sense on its own, quick grepping suggests plenty > of places that could benefit from this, and I thought it would be odd > to have to re-introduce a .remove callback in the gpio_wdt driver. > This has zero chance to be accepted. As suggested in the patch, just use devm_add_action(), like many other watchdog drivers. > Unfortunately, this turns out to be a bit of an "operation succeeded, > patient (almost) died": We use CONFIG_GPIO_WATCHDOG_ARCH_INITCALL > because the watchdog has a rather short timeout (1.0-2.25s, 1.6s > typical according to data sheet). At first, I put the new code right > after the devm_gpiod_get(), but the problem is that this early, we get > -EPROBE_DEFER since the clock provider (the RTC which sits off i2c) > isn't probed yet. But then the board would reset because it takes way > too long for the rest of the machine to initialize. [The bootloader > makes sure to turn on the RTC's clock output so the watchdog is > actually functional, the task here is to figure out the proper way to > prevent clk_disable_unused() from disabling it.] > Is there a property indicating always-on for clocks, similar to regulator-always-on ? The idea seems to exist, but it looks like it is always explict (ie mentioned somewhere in the code that a clock is always on, or "safe"). It would help if the clock in question can be marked as always-on without explicit consumer. Thanks, Guenter > Moving the logic to after the first "is it always-running and if so > give it an initial ping" made the board survive, but unfortunately the > second, and succesful, probe happens a little more than a second > later, which happens to work on this particular board, but is > obviously not suitable for production given that it's already above > what the spec says, and other random changes in the future might make > the gap even wider. > > So I don't know. The hardware is obviously misdesigned, and I don't > know how far the mainline kernel should stretch to support this; OTOH > the kernel does contain lots of workarounds for quirks and hardware > bugs. > > > > > Rasmus Villemoes (3): > clk: add devm_clk_prepare_enable() helper > dt-bindings: watchdog: add optional "delay" clock to gpio-wdt binding > watchdog: gpio_wdt: implement support for optional "delay" clock > > .../devicetree/bindings/watchdog/gpio-wdt.txt | 6 ++++ > .../driver-api/driver-model/devres.rst | 1 + > drivers/clk/clk-devres.c | 29 +++++++++++++++++++ > drivers/watchdog/gpio_wdt.c | 9 ++++++ > include/linux/clk.h | 13 +++++++++ > 5 files changed, 58 insertions(+) >
On 08/03/2021 22.38, Rob Herring wrote: > On Mon, Mar 08, 2021 at 09:02:29PM +0100, Rasmus Villemoes wrote: >> On 08/03/2021 18.21, Rob Herring wrote: >>> On Fri, Feb 26, 2021 at 03:14:10PM +0100, Rasmus Villemoes wrote: >>>> While a ripple counter can not usually be interfaced with (directly) >>>> from software, it may still be a crucial component in a board >>>> layout. To prevent its input clock from being disabled by the clock >>>> core because it apparently has no consumer, one needs to be able to >>>> represent that consumer in DT. >>> >>> I'm okay with this as it is describing h/w, but we already >>> 'protected-clocks' property which should work. >> >> Hm. Unless >> https://lore.kernel.org/lkml/20200903040015.5627-2-samuel@sholland.org/ >> gets merged, I don't see how this would work out-of-the-box. > > Hum, no really clear what the hold up is there given it seems it was > asked for. Letting it sit for 5 months is certainly not the way > to get it merged. Anyways, that's the kernel's problem, not mine as far > as DT bindings are concerned. > >> >> Note that I sent a completely different v2, which made the gpio-wdt the >> clock consumer based on feedback from Guenter and Arnd, but that v2 >> isn't suitable for our case because it post-poned handling of the >> watchdog till after i2c is ready, which is too late. Somewhat similar to >> https://lore.kernel.org/lkml/20210222171247.97609-2-sebastian.reichel@collabora.com/ >> it seems. > > Now at that one in my queue... I think 'protected-clocks' is the best > way to avoid any driver probe ordering issues. It's the only thing that > really captures don't turn off this clock. Agreed, and I did start by looking for a generic way to mark the clock as either "hands off, kernel" (relying on the bootloader to enable it), or better "make sure it's enabled". The closest I found was of_clk_detect_critical(), but the comment above that one says not to use it, so adding a call to some random RTC driver to support the clock-critical property just for my use case didn't seem like the right way to go. I didn't know about protected-clocks until you mentioned it, and it does seem to be the right way to handle these situations (which are apparently more common than I thought). The ripple counter binding > doesn't really capture that or what it is related to. Agreed, it was a "hail mary" and why I explained what I was really trying to achieve in the cover letter. Also, the > ripple-counter driver could be a module and you'd still have the same > issue as v2. Well, not quite. First of all, for a board like this, one always uses a tailor-made .config, where one would never set that to be a module (and even more obviously one wouldn't make the gpio-wdt driver a module). Second, it wouldn't be the same issue as v2. Rather, if the clock only gets enabled later when the ripple counter module would get loaded, there would be a period of time where the watchdog was rendered useless - the problem with v2 was that the watchdog wouldn't be petted in time, so the board would be reset before it booted completely. >>>> +Required properties: >>>> +- compatible: Must be "linux,ripple-ctr". >>> >>> Nothing linux specific about this. >> >> True, but I was following the lead of the existing gpio-wdt binding. Is >> there some other "vendor" name one can and should use for completely >> generic and simple components like these? "generic"? > > Most 'generic' and GPIO based interfaces have no vendor prefix. Ah, I see. Can we add just plain "wdt-gpio" to the gpio-wdt binding, and deprecate the "linux,wdt-gpio"? It's a little awkward to handle a "linux,wdt-gpio" compatible in a U-Boot driver. Rasmus
On Tue, Mar 9, 2021 at 12:39 AM Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote: > > On 08/03/2021 22.38, Rob Herring wrote: > > On Mon, Mar 08, 2021 at 09:02:29PM +0100, Rasmus Villemoes wrote: > >> On 08/03/2021 18.21, Rob Herring wrote: > >>> On Fri, Feb 26, 2021 at 03:14:10PM +0100, Rasmus Villemoes wrote: > >>>> While a ripple counter can not usually be interfaced with (directly) > >>>> from software, it may still be a crucial component in a board > >>>> layout. To prevent its input clock from being disabled by the clock > >>>> core because it apparently has no consumer, one needs to be able to > >>>> represent that consumer in DT. > >>> > >>> I'm okay with this as it is describing h/w, but we already > >>> 'protected-clocks' property which should work. > >> > >> Hm. Unless > >> https://lore.kernel.org/lkml/20200903040015.5627-2-samuel@sholland.org/ > >> gets merged, I don't see how this would work out-of-the-box. > > > > Hum, no really clear what the hold up is there given it seems it was > > asked for. Letting it sit for 5 months is certainly not the way > > to get it merged. Anyways, that's the kernel's problem, not mine as far > > as DT bindings are concerned. > > > >> > >> Note that I sent a completely different v2, which made the gpio-wdt the > >> clock consumer based on feedback from Guenter and Arnd, but that v2 > >> isn't suitable for our case because it post-poned handling of the > >> watchdog till after i2c is ready, which is too late. Somewhat similar to > >> https://lore.kernel.org/lkml/20210222171247.97609-2-sebastian.reichel@collabora.com/ > >> it seems. > > > > Now at that one in my queue... I think 'protected-clocks' is the best > > way to avoid any driver probe ordering issues. It's the only thing that > > really captures don't turn off this clock. > > Agreed, and I did start by looking for a generic way to mark the clock > as either "hands off, kernel" (relying on the bootloader to enable it), > or better "make sure it's enabled". The closest I found was > of_clk_detect_critical(), but the comment above that one says not to use > it, so adding a call to some random RTC driver to support the > clock-critical property just for my use case didn't seem like the right > way to go. > > I didn't know about protected-clocks until you mentioned it, and it does > seem to be the right way to handle these situations (which are > apparently more common than I thought). > > The ripple counter binding > > doesn't really capture that or what it is related to. > > Agreed, it was a "hail mary" and why I explained what I was really > trying to achieve in the cover letter. > > Also, the > > ripple-counter driver could be a module and you'd still have the same > > issue as v2. > > Well, not quite. First of all, for a board like this, one always uses a > tailor-made .config, where one would never set that to be a module (and > even more obviously one wouldn't make the gpio-wdt driver a module). Yes, I'd expect so in this case, but in general we really should try to avoid things dependent on being built-in (and ordering of initcalls). The whole notion of disabling resources in late_initcall is also kind of broken IMO and doesn't account for modules. > Second, it wouldn't be the same issue as v2. Rather, if the clock only > gets enabled later when the ripple counter module would get loaded, > there would be a period of time where the watchdog was rendered useless > - the problem with v2 was that the watchdog wouldn't be petted in time, > so the board would be reset before it booted completely. > > >>>> +Required properties: > >>>> +- compatible: Must be "linux,ripple-ctr". > >>> > >>> Nothing linux specific about this. > >> > >> True, but I was following the lead of the existing gpio-wdt binding. Is > >> there some other "vendor" name one can and should use for completely > >> generic and simple components like these? "generic"? > > > > Most 'generic' and GPIO based interfaces have no vendor prefix. > > Ah, I see. Can we add just plain "wdt-gpio" to the gpio-wdt binding, and > deprecate the "linux,wdt-gpio"? It's a little awkward to handle a > "linux,wdt-gpio" compatible in a U-Boot driver. No, just leave it. We have a few of these, but let's just not add new ones. In the end, it's just a string identifier. Rob