Re: [PATCH v2 0/3] add "delay" clock support to gpio_wdt

[Guenter Roeck <linux@roeck-us.net>]

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