Re: [PATCH 1/2] dt-bindings: misc: add binding for generic ripple counter

[Rob Herring]

On Tue, Mar 9, 2021 at 12:39 AM Rasmus Villemoes
<rasmus.villemoes@xxxxxxxxx> 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@xxxxxxxxxxxx/
> >> 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@xxxxxxxxxxxxx/
> >> 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