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
