| Message ID | 20210304221247.488173-1-linux@rasmusvillemoes.dk |
|---|---|
| Headers | show |
| Series | add "delay" clock support to gpio_wdt | expand |
On 3/4/21 2:12 PM, Rasmus Villemoes wrote: > Add a managed wrapper for clk_prepare_enable(). > > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> That has been tried several times, including by yours truly, and has always been rejected. Just use devm_add_action_or_reset() like many other watchdog drivers. Guenter
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 Mon, Mar 8, 2021 at 9:32 PM Guenter Roeck <linux@roeck-us.net> wrote: > > On 3/4/21 2:12 PM, Rasmus Villemoes wrote: > > Add a managed wrapper for clk_prepare_enable(). > > > > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> > > That has been tried several times, including by yours truly, > and has always been rejected. > > Just use devm_add_action_or_reset() like many other watchdog > drivers. Can we apply the devm version for crying out loud? I do not see what benefit there is to force everyone open-code it with devm_add_action_or_reset(). By simply blocking it we are not making the kernel better and it's been stalled for a very long time. Thanks.