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

What I'm trying to do:

We have a board with an external watchdog circuit (the kind that is
petted by flipping a gpio). The reset output of that is split in two:
One gives an immediate interrupt, so software knows that a hard reset
is imminent. The other half removes the "reset input" from a ripple
counter, causing that to start counting at 32kHz, and after 64ms, the
SOC's reset pin then gets pulled.

Unfortunately, the board designer overlooked that the 32kHz output
from the RTC can be disabled, which (since no other component uses
it), is just what happens when clk_disable_unused() gets called. When
the watchdog fires, we do get the interrupt, but the board does not
get reset as it should, since the counter doesn't count.

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.

The "clk_ignore_unused" kernel parameter is fine for debugging, but
too big a hammer (since it applies to all "unused" clocks).


Rasmus Villemoes (2):
  dt-bindings: misc: add binding for generic ripple counter
  drivers: misc: add ripple counter driver

 .../devicetree/bindings/misc/ripple-ctr.txt   |  8 +++++
 drivers/misc/Kconfig                          |  7 +++++
 drivers/misc/Makefile                         |  1 +
 drivers/misc/ripple-ctr.c                     | 31 +++++++++++++++++++
 4 files changed, 47 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/ripple-ctr.txt
 create mode 100644 drivers/misc/ripple-ctr.c

-- 
2.29.2

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.

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".
+- clocks: Input clock specifier. Refer to common clock bindings.

-- 
2.29.2

The only purpose of this driver is to serve as a consumer of the input
clock, to prevent it from being disabled by clk_disable_unused().

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/misc/Kconfig      |  7 +++++++
 drivers/misc/Makefile     |  1 +
 drivers/misc/ripple-ctr.c | 31 +++++++++++++++++++++++++++++++
 3 files changed, 39 insertions(+)
 create mode 100644 drivers/misc/ripple-ctr.c

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index f532c59bb59b..44b0b6ce42df 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig

@@ -445,6 +445,13 @@ config HISI_HIKEY_USB
 	  switching between the dual-role USB-C port and the USB-A host ports
 	  using only one USB controller.
 
+config RIPPLE_CTR
+	tristate "Trivial ripple counter driver"
+	help
+	  This provides a stub driver for a ripple counter, whose
+	  only purpose is to request and enable the clock source
+	  driving the counter.
+
 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"

diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 99b6f15a3c70..d560163068a9 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile

@@ -56,3 +56,4 @@ obj-$(CONFIG_HABANA_AI)		+= habanalabs/
 obj-$(CONFIG_UACCE)		+= uacce/
 obj-$(CONFIG_XILINX_SDFEC)	+= xilinx_sdfec.o
 obj-$(CONFIG_HISI_HIKEY_USB)	+= hisi_hikey_usb.o
+obj-$(CONFIG_RIPPLE_CTR)	+= ripple-ctr.o

diff --git a/drivers/misc/ripple-ctr.c b/drivers/misc/ripple-ctr.c
new file mode 100644
index 000000000000..f086eaf335df
--- /dev/null
+++ b/drivers/misc/ripple-ctr.c

@@ -0,0 +1,31 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+static int ripple_ctr_probe(struct platform_device *pdev)
+{
+	struct clk *clk;
+
+	clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+	return clk_prepare_enable(clk);
+}
+
+static const struct of_device_id ripple_ctr_ids[] = {
+	{ .compatible = "linux,ripple-counter", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ripple_ctr_ids);
+
+static struct platform_driver ripple_ctr_driver = {
+	.driver	= {
+		.name		= "ripple-counter",
+		.of_match_table	= ripple_ctr_ids,
+	},
+	.probe	= ripple_ctr_probe,
+};
+module_platform_driver(ripple_ctr_driver);

-- 
2.29.2

On Fri, Feb 26, 2021 at 3:14 PM Rasmus Villemoes
[off-list ref] 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
[off-list ref] wrote:

quoted

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 2/26/21 8:35 AM, Rasmus Villemoes wrote:

On 26/02/2021 15.35, Arnd Bergmann wrote:

quoted

On Fri, Feb 26, 2021 at 3:14 PM Rasmus Villemoes
[off-list ref] wrote:

quoted

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

Missing MODULE_LICENSE() tag?
See https://www.kernel.org/doc/html/latest/process/license-rules.html#license-identifiers

--
2.29.2

On 26/02/2021 20.53, Guenter Roeck wrote:

On 2/26/21 8:35 AM, Rasmus Villemoes wrote:

quoted

On 26/02/2021 15.35, Arnd Bergmann wrote:

quoted

On Fri, Feb 26, 2021 at 3:14 PM Rasmus Villemoes
[off-list ref] wrote:

quoted

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 Mon, Mar 1, 2021 at 9:34 AM Rasmus Villemoes
[off-list ref] wrote:

On 26/02/2021 20.53, Guenter Roeck wrote:

quoted

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

quoted

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

On 3/1/21 1:44 AM, Arnd Bergmann wrote:

On Mon, Mar 1, 2021 at 9:34 AM Rasmus Villemoes
[off-list ref] wrote:

quoted

On 26/02/2021 20.53, Guenter Roeck wrote:

quoted

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

quoted

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

-- 
2.29.2

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

[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

[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

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.

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:

quoted

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.

quoted

+
+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:

quoted

On Fri, Feb 26, 2021 at 03:14:10PM +0100, Rasmus Villemoes wrote:

quoted

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.

quoted

quoted

+
+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:

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 08/03/2021 22.38, Rob Herring wrote:

On Mon, Mar 08, 2021 at 09:02:29PM +0100, Rasmus Villemoes wrote:

quoted

On 08/03/2021 18.21, Rob Herring wrote:

quoted

On Fri, Feb 26, 2021 at 03:14:10PM +0100, Rasmus Villemoes wrote:

quoted

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.

quoted

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.

quoted

quoted

quoted

+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
[off-list ref] wrote:

On 08/03/2021 22.38, Rob Herring wrote:

quoted

On Mon, Mar 08, 2021 at 09:02:29PM +0100, Rasmus Villemoes wrote:

quoted

On 08/03/2021 18.21, Rob Herring wrote:

quoted

On Fri, Feb 26, 2021 at 03:14:10PM +0100, Rasmus Villemoes wrote:

quoted

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.

quoted

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

quoted

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

quoted

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.

quoted

quoted

quoted

quoted

+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

On Mon, Mar 8, 2021 at 9:32 PM Guenter Roeck [off-list ref] wrote:

On 3/4/21 2:12 PM, Rasmus Villemoes wrote:

quoted

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