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

[Rasmus Villemoes <rasmus.villemoes@prevas.dk>]

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