[v2,0/3] add "delay" clock support to gpio_wdt

Message

Rasmus Villemoes March 4, 2021, 10:12 p.m. UTC

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(+)

Comments

Guenter Roeck March 9, 2021, 5:51 a.m. UTC | #1

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(+)
>