* [PATCH 0/2] add ripple counter dt binding and driver @ 2021-02-26 14:14 Rasmus Villemoes 2021-02-26 14:14 ` [PATCH 1/2] dt-bindings: misc: add binding for generic ripple counter Rasmus Villemoes ` (3 more replies) 0 siblings, 4 replies; 25+ messages in thread From: Rasmus Villemoes @ 2021-02-26 14:14 UTC (permalink / raw) To: Greg Kroah-Hartman, Rob Herring Cc: devicetree, linux-kernel, Arnd Bergmann, linux-clk, Rasmus Villemoes 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 1/2] dt-bindings: misc: add binding for generic ripple counter 2021-02-26 14:14 [PATCH 0/2] add ripple counter dt binding and driver Rasmus Villemoes @ 2021-02-26 14:14 ` Rasmus Villemoes 2021-03-08 17:21 ` Rob Herring 2021-02-26 14:14 ` [PATCH 2/2] drivers: misc: add ripple counter driver Rasmus Villemoes ` (2 subsequent siblings) 3 siblings, 1 reply; 25+ messages in thread From: Rasmus Villemoes @ 2021-02-26 14:14 UTC (permalink / raw) To: Greg Kroah-Hartman, Rob Herring Cc: devicetree, linux-kernel, Arnd Bergmann, linux-clk, Rasmus Villemoes 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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] dt-bindings: misc: add binding for generic ripple counter 2021-02-26 14:14 ` [PATCH 1/2] dt-bindings: misc: add binding for generic ripple counter Rasmus Villemoes @ 2021-03-08 17:21 ` Rob Herring 2021-03-08 20:02 ` Rasmus Villemoes 0 siblings, 1 reply; 25+ messages in thread From: Rob Herring @ 2021-03-08 17:21 UTC (permalink / raw) To: Rasmus Villemoes Cc: Greg Kroah-Hartman, devicetree, linux-kernel, Arnd Bergmann, linux-clk 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. > > 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". Nothing linux specific about this. > +- clocks: Input clock specifier. Refer to common clock bindings. > -- > 2.29.2 > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] dt-bindings: misc: add binding for generic ripple counter 2021-03-08 17:21 ` Rob Herring @ 2021-03-08 20:02 ` Rasmus Villemoes 2021-03-08 21:38 ` Rob Herring 0 siblings, 1 reply; 25+ messages in thread From: Rasmus Villemoes @ 2021-03-08 20:02 UTC (permalink / raw) To: Rob Herring Cc: Greg Kroah-Hartman, devicetree, linux-kernel, Arnd Bergmann, linux-clk, Guenter Roeck On 08/03/2021 18.21, Rob Herring wrote: > 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. 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. >> + >> +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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] dt-bindings: misc: add binding for generic ripple counter 2021-03-08 20:02 ` Rasmus Villemoes @ 2021-03-08 21:38 ` Rob Herring 2021-03-09 7:39 ` Rasmus Villemoes 0 siblings, 1 reply; 25+ messages in thread From: Rob Herring @ 2021-03-08 21:38 UTC (permalink / raw) To: Rasmus Villemoes Cc: Greg Kroah-Hartman, devicetree, linux-kernel, Arnd Bergmann, linux-clk, Guenter Roeck On Mon, Mar 08, 2021 at 09:02:29PM +0100, Rasmus Villemoes wrote: > On 08/03/2021 18.21, Rob Herring wrote: > > 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. > > 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. > >> + > >> +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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] dt-bindings: misc: add binding for generic ripple counter 2021-03-08 21:38 ` Rob Herring @ 2021-03-09 7:39 ` Rasmus Villemoes 2021-03-09 15:44 ` Rob Herring 0 siblings, 1 reply; 25+ messages in thread From: Rasmus Villemoes @ 2021-03-09 7:39 UTC (permalink / raw) To: Rob Herring, Rasmus Villemoes Cc: Greg Kroah-Hartman, devicetree, linux-kernel, Arnd Bergmann, linux-clk, Guenter Roeck On 08/03/2021 22.38, Rob Herring wrote: > On Mon, Mar 08, 2021 at 09:02:29PM +0100, Rasmus Villemoes wrote: >> On 08/03/2021 18.21, Rob Herring wrote: >>> 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. >> >> 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. 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. >>>> +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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] dt-bindings: misc: add binding for generic ripple counter 2021-03-09 7:39 ` Rasmus Villemoes @ 2021-03-09 15:44 ` Rob Herring 0 siblings, 0 replies; 25+ messages in thread From: Rob Herring @ 2021-03-09 15:44 UTC (permalink / raw) To: Rasmus Villemoes Cc: Rasmus Villemoes, Greg Kroah-Hartman, devicetree, linux-kernel, Arnd Bergmann, linux-clk, Guenter Roeck On Tue, Mar 9, 2021 at 12:39 AM Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote: > > On 08/03/2021 22.38, Rob Herring wrote: > > On Mon, Mar 08, 2021 at 09:02:29PM +0100, Rasmus Villemoes wrote: > >> On 08/03/2021 18.21, Rob Herring wrote: > >>> 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. > >> > >> 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. > > 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). 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. > > >>>> +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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 2/2] drivers: misc: add ripple counter driver 2021-02-26 14:14 [PATCH 0/2] add ripple counter dt binding and driver Rasmus Villemoes 2021-02-26 14:14 ` [PATCH 1/2] dt-bindings: misc: add binding for generic ripple counter Rasmus Villemoes @ 2021-02-26 14:14 ` Rasmus Villemoes 2021-02-28 5:47 ` Chen, Mike Ximing [not found] ` <CAHp75Vc8S2E0vWFcqK-jO9Nhd-Us_7t-aWNj-7k+fWDcqR1XkQ@mail.gmail.com> 2021-02-26 14:35 ` [PATCH 0/2] add ripple counter dt binding and driver Arnd Bergmann 2021-03-04 22:12 ` [PATCH v2 0/3] add "delay" clock support to gpio_wdt Rasmus Villemoes 3 siblings, 2 replies; 25+ messages in thread From: Rasmus Villemoes @ 2021-02-26 14:14 UTC (permalink / raw) To: Greg Kroah-Hartman, Rob Herring Cc: devicetree, linux-kernel, Arnd Bergmann, linux-clk, Rasmus Villemoes 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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* RE: [PATCH 2/2] drivers: misc: add ripple counter driver 2021-02-26 14:14 ` [PATCH 2/2] drivers: misc: add ripple counter driver Rasmus Villemoes @ 2021-02-28 5:47 ` Chen, Mike Ximing [not found] ` <CAHp75Vc8S2E0vWFcqK-jO9Nhd-Us_7t-aWNj-7k+fWDcqR1XkQ@mail.gmail.com> 1 sibling, 0 replies; 25+ messages in thread From: Chen, Mike Ximing @ 2021-02-28 5:47 UTC (permalink / raw) To: Rasmus Villemoes, Greg Kroah-Hartman, Rob Herring Cc: devicetree, linux-kernel, Arnd Bergmann, linux-clk > -----Original Message----- > From: Rasmus Villemoes <linux@rasmusvillemoes.dk> > Sent: Friday, February 26, 2021 9:14 AM > To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob Herring > <robh+dt@kernel.org> > Cc: devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Arnd Bergmann > <arnd@arndb.de>; linux-clk@vger.kernel.org; Rasmus Villemoes > <linux@rasmusvillemoes.dk> > Subject: [PATCH 2/2] drivers: misc: add ripple counter driver > > 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); Missing MODULE_LICENSE() tag? See https://www.kernel.org/doc/html/latest/process/license-rules.html#license-identifiers > -- > 2.29.2 ^ permalink raw reply [flat|nested] 25+ messages in thread
[parent not found: <CAHp75Vc8S2E0vWFcqK-jO9Nhd-Us_7t-aWNj-7k+fWDcqR1XkQ@mail.gmail.com>]
* Re: [PATCH 2/2] drivers: misc: add ripple counter driver [not found] ` <CAHp75Vc8S2E0vWFcqK-jO9Nhd-Us_7t-aWNj-7k+fWDcqR1XkQ@mail.gmail.com> @ 2021-02-28 9:29 ` Andy Shevchenko 2021-02-28 9:33 ` Andy Shevchenko 0 siblings, 1 reply; 25+ messages in thread From: Andy Shevchenko @ 2021-02-28 9:29 UTC (permalink / raw) To: Rasmus Villemoes Cc: Greg Kroah-Hartman, Rob Herring, devicetree, linux-kernel, Arnd Bergmann, linux-clk On Sun, Feb 28, 2021 at 11:07 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Friday, February 26, 2021, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: >> >> 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(). > > We have a clock API to do the same (something like marking it used or so) why do you need a driver? Example: https://elixir.bootlin.com/linux/latest/source/drivers/platform/x86/pmc_atom.c#L365 If it's a DT based platform I think you can make it somehow work thru DT. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] drivers: misc: add ripple counter driver 2021-02-28 9:29 ` Andy Shevchenko @ 2021-02-28 9:33 ` Andy Shevchenko 2021-03-01 8:29 ` Rasmus Villemoes 0 siblings, 1 reply; 25+ messages in thread From: Andy Shevchenko @ 2021-02-28 9:33 UTC (permalink / raw) To: Rasmus Villemoes Cc: Greg Kroah-Hartman, Rob Herring, devicetree, linux-kernel, Arnd Bergmann, linux-clk On Sun, Feb 28, 2021 at 11:29 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Sun, Feb 28, 2021 at 11:07 AM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Friday, February 26, 2021, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > >> > >> 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(). > > > > We have a clock API to do the same (something like marking it used or so) why do you need a driver? > > Example: > https://elixir.bootlin.com/linux/latest/source/drivers/platform/x86/pmc_atom.c#L365 > > If it's a DT based platform I think you can make it somehow work thru DT. Okay, briefly looking at the state of affairs [1] seems like you need to hack it into clock provider. [1]: https://elixir.bootlin.com/linux/latest/C/ident/CLK_IS_CRITICAL -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] drivers: misc: add ripple counter driver 2021-02-28 9:33 ` Andy Shevchenko @ 2021-03-01 8:29 ` Rasmus Villemoes 0 siblings, 0 replies; 25+ messages in thread From: Rasmus Villemoes @ 2021-03-01 8:29 UTC (permalink / raw) To: Andy Shevchenko Cc: Greg Kroah-Hartman, Rob Herring, devicetree, linux-kernel, Arnd Bergmann, linux-clk On 28/02/2021 10.33, Andy Shevchenko wrote: > On Sun, Feb 28, 2021 at 11:29 AM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: >> >> On Sun, Feb 28, 2021 at 11:07 AM Andy Shevchenko >> <andy.shevchenko@gmail.com> wrote: >>> On Friday, February 26, 2021, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: >>>> >>>> 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(). >>> >>> We have a clock API to do the same (something like marking it used or so) why do you need a driver? >> >> Example: >> https://elixir.bootlin.com/linux/latest/source/drivers/platform/x86/pmc_atom.c#L365 >> >> If it's a DT based platform I think you can make it somehow work thru DT. > > Okay, briefly looking at the state of affairs [1] seems like you need > to hack it into clock provider. > > [1]: https://elixir.bootlin.com/linux/latest/C/ident/CLK_IS_CRITICAL > I did find CLK_IS_CRITICAL and CLK_IGNORE_UNUSED while trying to figure out how to handle this. However, while CLK_IS_CRITICAL is in principle settable via DT, the comment above of_clk_detect_critical() seems to make it clear that adding a call of that from the RTC driver is a total no-no. CLK_IGNORE_UNUSED can't be set at all from DT, and wouldn't solve the problem fully - while we can and do make sure the bootloader sets the appropriate bit in the RTCs registers, it's more robust if we also ensure the kernel explicitly enables the clock. But if there is some way to do this within the clk framework/existing bindings, I'm all ears - that's the reason I cc'ed the clk list. Rasmus ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/2] add ripple counter dt binding and driver 2021-02-26 14:14 [PATCH 0/2] add ripple counter dt binding and driver Rasmus Villemoes 2021-02-26 14:14 ` [PATCH 1/2] dt-bindings: misc: add binding for generic ripple counter Rasmus Villemoes 2021-02-26 14:14 ` [PATCH 2/2] drivers: misc: add ripple counter driver Rasmus Villemoes @ 2021-02-26 14:35 ` Arnd Bergmann 2021-02-26 16:35 ` Rasmus Villemoes 2021-03-04 22:12 ` [PATCH v2 0/3] add "delay" clock support to gpio_wdt Rasmus Villemoes 3 siblings, 1 reply; 25+ messages in thread From: Arnd Bergmann @ 2021-02-26 14:35 UTC (permalink / raw) To: Rasmus Villemoes Cc: Greg Kroah-Hartman, Rob Herring, DTML, linux-kernel, Arnd Bergmann, linux-clk 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? Arnd ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/2] add ripple counter dt binding and driver 2021-02-26 14:35 ` [PATCH 0/2] add ripple counter dt binding and driver Arnd Bergmann @ 2021-02-26 16:35 ` Rasmus Villemoes 2021-02-26 19:53 ` Guenter Roeck 0 siblings, 1 reply; 25+ messages in thread From: Rasmus Villemoes @ 2021-02-26 16:35 UTC (permalink / raw) To: Arnd Bergmann Cc: Greg Kroah-Hartman, Rob Herring, DTML, linux-kernel, Arnd Bergmann, linux-clk, Guenter Roeck, Wim Van Sebroeck, linux-watchdog 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/2] add ripple counter dt binding and driver 2021-02-26 16:35 ` Rasmus Villemoes @ 2021-02-26 19:53 ` Guenter Roeck 2021-03-01 8:34 ` Rasmus Villemoes 0 siblings, 1 reply; 25+ messages in thread From: Guenter Roeck @ 2021-02-26 19:53 UTC (permalink / raw) To: Rasmus Villemoes, Arnd Bergmann Cc: Greg Kroah-Hartman, Rob Herring, DTML, linux-kernel, Arnd Bergmann, linux-clk, Wim Van Sebroeck, linux-watchdog 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/2] add ripple counter dt binding and driver 2021-02-26 19:53 ` Guenter Roeck @ 2021-03-01 8:34 ` Rasmus Villemoes 2021-03-01 9:44 ` Arnd Bergmann 0 siblings, 1 reply; 25+ messages in thread From: Rasmus Villemoes @ 2021-03-01 8:34 UTC (permalink / raw) To: Guenter Roeck, Arnd Bergmann Cc: Greg Kroah-Hartman, Rob Herring, DTML, linux-kernel, Arnd Bergmann, linux-clk, Wim Van Sebroeck, linux-watchdog 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/2] add ripple counter dt binding and driver 2021-03-01 8:34 ` Rasmus Villemoes @ 2021-03-01 9:44 ` Arnd Bergmann 2021-03-01 14:21 ` Guenter Roeck 0 siblings, 1 reply; 25+ messages in thread From: Arnd Bergmann @ 2021-03-01 9:44 UTC (permalink / raw) To: Rasmus Villemoes Cc: Guenter Roeck, Greg Kroah-Hartman, Rob Herring, DTML, linux-kernel, Arnd Bergmann, linux-clk, Wim Van Sebroeck, linux-watchdog 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/2] add ripple counter dt binding and driver 2021-03-01 9:44 ` Arnd Bergmann @ 2021-03-01 14:21 ` Guenter Roeck 0 siblings, 0 replies; 25+ messages in thread From: Guenter Roeck @ 2021-03-01 14:21 UTC (permalink / raw) To: Arnd Bergmann, Rasmus Villemoes Cc: Greg Kroah-Hartman, Rob Herring, DTML, linux-kernel, Arnd Bergmann, linux-clk, Wim Van Sebroeck, linux-watchdog 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v2 0/3] add "delay" clock support to gpio_wdt 2021-02-26 14:14 [PATCH 0/2] add ripple counter dt binding and driver Rasmus Villemoes ` (2 preceding siblings ...) 2021-02-26 14:35 ` [PATCH 0/2] add ripple counter dt binding and driver Arnd Bergmann @ 2021-03-04 22:12 ` Rasmus Villemoes 2021-03-04 22:12 ` [PATCH v2 1/3] clk: add devm_clk_prepare_enable() helper Rasmus Villemoes ` (3 more replies) 3 siblings, 4 replies; 25+ messages in thread From: Rasmus Villemoes @ 2021-03-04 22:12 UTC (permalink / raw) To: Arnd Bergmann, Guenter Roeck, Stephen Boyd Cc: Rob Herring, devicetree, linux-kernel, linux-clk, linux-watchdog, 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v2 1/3] clk: add devm_clk_prepare_enable() helper 2021-03-04 22:12 ` [PATCH v2 0/3] add "delay" clock support to gpio_wdt Rasmus Villemoes @ 2021-03-04 22:12 ` Rasmus Villemoes 2021-03-09 5:28 ` Guenter Roeck 2021-03-04 22:12 ` [PATCH v2 2/3] dt-bindings: watchdog: add optional "delay" clock to gpio-wdt binding Rasmus Villemoes ` (2 subsequent siblings) 3 siblings, 1 reply; 25+ messages in thread From: Rasmus Villemoes @ 2021-03-04 22:12 UTC (permalink / raw) To: Arnd Bergmann, Guenter Roeck, Stephen Boyd Cc: Rob Herring, devicetree, linux-kernel, linux-clk, linux-watchdog, 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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/3] clk: add devm_clk_prepare_enable() helper 2021-03-04 22:12 ` [PATCH v2 1/3] clk: add devm_clk_prepare_enable() helper Rasmus Villemoes @ 2021-03-09 5:28 ` Guenter Roeck 2022-03-05 2:41 ` Dmitry Torokhov 0 siblings, 1 reply; 25+ messages in thread From: Guenter Roeck @ 2021-03-09 5:28 UTC (permalink / raw) To: Rasmus Villemoes, Arnd Bergmann, Stephen Boyd Cc: Rob Herring, devicetree, linux-kernel, linux-clk, linux-watchdog 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/3] clk: add devm_clk_prepare_enable() helper 2021-03-09 5:28 ` Guenter Roeck @ 2022-03-05 2:41 ` Dmitry Torokhov 0 siblings, 0 replies; 25+ messages in thread From: Dmitry Torokhov @ 2022-03-05 2:41 UTC (permalink / raw) To: Guenter Roeck, Russell King Cc: Rasmus Villemoes, Arnd Bergmann, Stephen Boyd, Rob Herring, DTML, lkml, linux-clk, LINUXWATCHDOG 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v2 2/3] dt-bindings: watchdog: add optional "delay" clock to gpio-wdt binding 2021-03-04 22:12 ` [PATCH v2 0/3] add "delay" clock support to gpio_wdt Rasmus Villemoes 2021-03-04 22:12 ` [PATCH v2 1/3] clk: add devm_clk_prepare_enable() helper Rasmus Villemoes @ 2021-03-04 22:12 ` Rasmus Villemoes 2021-03-04 22:12 ` [PATCH v2 3/3] watchdog: gpio_wdt: implement support for optional "delay" clock Rasmus Villemoes 2021-03-09 5:51 ` [PATCH v2 0/3] add "delay" clock support to gpio_wdt Guenter Roeck 3 siblings, 0 replies; 25+ messages in thread From: Rasmus Villemoes @ 2021-03-04 22:12 UTC (permalink / raw) To: Arnd Bergmann, Guenter Roeck, Stephen Boyd Cc: Rob Herring, devicetree, linux-kernel, linux-clk, linux-watchdog, Rasmus Villemoes, Arnd Bergmann [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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v2 3/3] watchdog: gpio_wdt: implement support for optional "delay" clock 2021-03-04 22:12 ` [PATCH v2 0/3] add "delay" clock support to gpio_wdt Rasmus Villemoes 2021-03-04 22:12 ` [PATCH v2 1/3] clk: add devm_clk_prepare_enable() helper Rasmus Villemoes 2021-03-04 22:12 ` [PATCH v2 2/3] dt-bindings: watchdog: add optional "delay" clock to gpio-wdt binding Rasmus Villemoes @ 2021-03-04 22:12 ` Rasmus Villemoes 2021-03-09 5:51 ` [PATCH v2 0/3] add "delay" clock support to gpio_wdt Guenter Roeck 3 siblings, 0 replies; 25+ messages in thread From: Rasmus Villemoes @ 2021-03-04 22:12 UTC (permalink / raw) To: Arnd Bergmann, Guenter Roeck, Stephen Boyd Cc: Rob Herring, devicetree, linux-kernel, linux-clk, linux-watchdog, 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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v2 0/3] add "delay" clock support to gpio_wdt 2021-03-04 22:12 ` [PATCH v2 0/3] add "delay" clock support to gpio_wdt Rasmus Villemoes ` (2 preceding siblings ...) 2021-03-04 22:12 ` [PATCH v2 3/3] watchdog: gpio_wdt: implement support for optional "delay" clock Rasmus Villemoes @ 2021-03-09 5:51 ` Guenter Roeck 3 siblings, 0 replies; 25+ messages in thread From: Guenter Roeck @ 2021-03-09 5:51 UTC (permalink / raw) To: Rasmus Villemoes, Arnd Bergmann, Stephen Boyd Cc: Rob Herring, devicetree, linux-kernel, linux-clk, linux-watchdog 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(+) > ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2022-03-05 2:41 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-26 14:14 [PATCH 0/2] add ripple counter dt binding and driver Rasmus Villemoes 2021-02-26 14:14 ` [PATCH 1/2] dt-bindings: misc: add binding for generic ripple counter Rasmus Villemoes 2021-03-08 17:21 ` Rob Herring 2021-03-08 20:02 ` Rasmus Villemoes 2021-03-08 21:38 ` Rob Herring 2021-03-09 7:39 ` Rasmus Villemoes 2021-03-09 15:44 ` Rob Herring 2021-02-26 14:14 ` [PATCH 2/2] drivers: misc: add ripple counter driver Rasmus Villemoes 2021-02-28 5:47 ` Chen, Mike Ximing [not found] ` <CAHp75Vc8S2E0vWFcqK-jO9Nhd-Us_7t-aWNj-7k+fWDcqR1XkQ@mail.gmail.com> 2021-02-28 9:29 ` Andy Shevchenko 2021-02-28 9:33 ` Andy Shevchenko 2021-03-01 8:29 ` Rasmus Villemoes 2021-02-26 14:35 ` [PATCH 0/2] add ripple counter dt binding and driver Arnd Bergmann 2021-02-26 16:35 ` Rasmus Villemoes 2021-02-26 19:53 ` Guenter Roeck 2021-03-01 8:34 ` Rasmus Villemoes 2021-03-01 9:44 ` Arnd Bergmann 2021-03-01 14:21 ` Guenter Roeck 2021-03-04 22:12 ` [PATCH v2 0/3] add "delay" clock support to gpio_wdt Rasmus Villemoes 2021-03-04 22:12 ` [PATCH v2 1/3] clk: add devm_clk_prepare_enable() helper Rasmus Villemoes 2021-03-09 5:28 ` Guenter Roeck 2022-03-05 2:41 ` Dmitry Torokhov 2021-03-04 22:12 ` [PATCH v2 2/3] dt-bindings: watchdog: add optional "delay" clock to gpio-wdt binding Rasmus Villemoes 2021-03-04 22:12 ` [PATCH v2 3/3] watchdog: gpio_wdt: implement support for optional "delay" clock Rasmus Villemoes 2021-03-09 5:51 ` [PATCH v2 0/3] add "delay" clock support to gpio_wdt Guenter Roeck
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).