Re: [PATCH 0/2] add ripple counter dt binding and driver

Re: [PATCH 0/2] add ripple counter dt binding and driver

[Rasmus Villemoes]

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

Re: [PATCH 0/2] add ripple counter dt binding and driver

[Guenter Roeck]

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. What else does "consumer" mean ? And why
not just add optional clock support to the gpio_wdt driver ?

Guenter

Re: [PATCH 0/2] add ripple counter dt binding and driver

[Rasmus Villemoes]

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

Re: [PATCH 0/2] add ripple counter dt binding and driver

[Arnd Bergmann]

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.

        Arnd

Re: [PATCH 0/2] add ripple counter dt binding and driver

[Guenter Roeck]

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

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

[Rasmus Villemoes]

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

-- 
2.29.2

[PATCH v2 1/3] clk: add devm_clk_prepare_enable() helper

[Rasmus Villemoes]

Add a managed wrapper for clk_prepare_enable().

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 .../driver-api/driver-model/devres.rst        |  1 +
 drivers/clk/clk-devres.c                      | 29 +++++++++++++++++++
 include/linux/clk.h                           | 13 +++++++++
 3 files changed, 43 insertions(+)

diff --git a/Documentation/driver-api/driver-model/devres.rst b/Documentation/driver-api/driver-model/devres.rst
index cd8b6e657b94..8ee2557f9ad7 100644
--- a/Documentation/driver-api/driver-model/devres.rst
+++ b/Documentation/driver-api/driver-model/devres.rst
@@ -253,6 +253,7 @@ CLOCK
   devm_clk_hw_register()
   devm_of_clk_add_hw_provider()
   devm_clk_hw_register_clkdev()
+  devm_clk_prepare_enable()
 
 DMA
   dmaenginem_async_device_register()
diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index be160764911b..d5bfa8cd7347 100644
--- a/drivers/clk/clk-devres.c
+++ b/drivers/clk/clk-devres.c
@@ -156,3 +156,32 @@ struct clk *devm_get_clk_from_child(struct device *dev,
 	return clk;
 }
 EXPORT_SYMBOL(devm_get_clk_from_child);
+
+static void devm_clk_disable_unprepare(struct device *dev, void *res)
+{
+	clk_disable_unprepare(*(struct clk **)res);
+}
+
+int devm_clk_prepare_enable(struct device *dev, struct clk *clk)
+{
+	struct clk **ptr;
+	int ret;
+
+	if (!clk)
+		return 0;
+
+	ptr = devres_alloc(devm_clk_disable_unprepare, sizeof(*ptr), GFP_KERNEL);
+	if (!ptr)
+		return -ENOMEM;
+
+	ret = clk_prepare_enable(clk);
+	if (!ret) {
+		*ptr = clk;
+		devres_add(dev, ptr);
+	} else {
+		devres_free(ptr);
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL(devm_clk_prepare_enable);
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 266e8de3cb51..04d135520480 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -485,6 +485,19 @@ struct clk *devm_clk_get_optional(struct device *dev, const char *id);
  */
 struct clk *devm_get_clk_from_child(struct device *dev,
 				    struct device_node *np, const char *con_id);
+/**
+ * devm_clk_prepare_enable - prepare and enable a clock source
+ * @dev: device for clock "consumer"
+ * @clk: clock source
+ *
+ * This function calls clk_prepare_enable() on @clk, and ensures the
+ * clock will automatically be disabled and unprepared when the device
+ * is unbound from the bus.
+ *
+ * Must not be called from within atomic context.
+ */
+int devm_clk_prepare_enable(struct device *dev, struct clk *clk);
+
 /**
  * clk_rate_exclusive_get - get exclusivity over the rate control of a
  *                          producer
-- 
2.29.2

[PATCH v2 2/3] dt-bindings: watchdog: add optional "delay" clock to gpio-wdt binding

[Rasmus Villemoes]

[DO NOT MERGE - see cover letter]

We have a board where the reset output from the ADM706S is split in
two: directly routed to an interrupt, and also to start a ripple
counter, which 64 ms later than pulls the SOC's reset pin. That ripple
counter only works if the RTC's 32kHz output is enabled, and since
linux by default disables unused clocks, that effectively renders the
watchdog useless. So add an optional "delay" clock binding.

Suggested-by: Arnd Bergmann <arnd@kernel.org>
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 Documentation/devicetree/bindings/watchdog/gpio-wdt.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt b/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
index 198794963786..527be6b30451 100644
--- a/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
@@ -17,6 +17,10 @@ Optional Properties:
 - always-running: If the watchdog timer cannot be disabled, add this flag to
   have the driver keep toggling the signal without a client. It will only cease
   to toggle the signal when the device is open and the timeout elapsed.
+- clock-names: May contain the entry "delay" if the board has logic
+  that delays the reset signal from the watchdog and which requires an
+  external signal to function.
+- clocks: Phandles corresponding to the clock-names.
 
 Example:
 	watchdog: watchdog {
@@ -25,4 +29,6 @@ Example:
 		gpios = <&gpio3 9 GPIO_ACTIVE_LOW>;
 		hw_algo = "toggle";
 		hw_margin_ms = <1600>;
+		clock-names = "delay";
+		clocks = <&rtc 1>;
 	};
-- 
2.29.2

[PATCH v2 3/3] watchdog: gpio_wdt: implement support for optional "delay" clock

[Rasmus Villemoes]

[DO NOT MERGE - see cover letter]

We have a board where the reset output from the ADM706S is split in
two: directly routed to an interrupt, and also to start a ripple
counter, which 64 ms later than pulls the SOC's reset pin. That ripple
counter only works if the RTC's 32kHz output is enabled, and since
linux by default disables unused clocks, that effectively renders the
watchdog useless.

Add driver support for an optional "delay" clock, as documented in the
preceding patch.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/watchdog/gpio_wdt.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c
index 0923201ce874..f812c39bc1e8 100644
--- a/drivers/watchdog/gpio_wdt.c
+++ b/drivers/watchdog/gpio_wdt.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/err.h>
+#include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/module.h>
 #include <linux/gpio/consumer.h>
@@ -111,6 +112,7 @@ static int gpio_wdt_probe(struct platform_device *pdev)
 	enum gpiod_flags gflags;
 	unsigned int hw_margin;
 	const char *algo;
+	struct clk *clk;
 	int ret;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
@@ -164,6 +166,13 @@ static int gpio_wdt_probe(struct platform_device *pdev)
 	if (priv->always_running)
 		gpio_wdt_start(&priv->wdd);
 
+	clk = devm_clk_get_optional(dev, "delay");
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+	ret = devm_clk_prepare_enable(dev, clk);
+	if (ret)
+		return ret;
+
 	return devm_watchdog_register_device(dev, &priv->wdd);
 }
 
-- 
2.29.2

Re: [PATCH v2 1/3] clk: add devm_clk_prepare_enable() helper

[Guenter Roeck]

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

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

[Guenter Roeck]

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

Re: [PATCH v2 1/3] clk: add devm_clk_prepare_enable() helper

[Dmitry Torokhov]

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.

-- 
Dmitry