Re: [PATCH v7 3/5] clk: Supply the critical clock {init, enable, disable} framework

[Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]

On Fri, 31 Jul 2015, Maxime Ripard wrote:

> On Thu, Jul 30, 2015 at 03:47:20PM -0700, Michael Turquette wrote:
> > > > >    */
> > > > > 
> > > > > Resume:
> > > > >   /* Order is unimportant */
> > > > >   SPI enables Clock 4 (ref == 1)
> > > > >   RAM enables Clock 4 and re-enables .leave_on (ref == 2)
> > > > >   I2C enables Clock 4 (ref == 3)
> > > > 
> > > > Same again. As soon as RAM calls clk_enable_critical the ref count goes
> > > > up. .leave_on does nothing as far as I can tell. The all works because
> > > > of the reference counting, which already exists before this patch
> > > > series.
> > > 
> > > So fundamentally you're right in what you say.  All you really need to
> > > disable a critical clock is write a knowledgeable driver, which is
> > > intentionally unbalanced i.e. just calls clk_disable().  All this
> > 
> > OK, the line above is helpful. What you really want is a formalized
> > hand-off mechanism, whereby the clock is enabled at registration-time
> > and it cannot be turned off until the right driver claims it and decides
> > turning it off is OK (with a priori knowledge that the clock is already
> > enabled).
> 
> There's two things that should be covered, and are related, yet can be
> done in two steps:
> 
>   - Have a way to, no matter what (which configuration we have, if we
>     have multiple users or not that might reparent or disable their
>     clocks, etc.), make sure that a clock will always be running by
>     default. This is done through the call in clk-conf, and we
>     identify such clocks using critical-clocks in the DT.
> 
>   - Now, even though that information is true, some driver who are
>     aware of that fact might want to disable those critical
>     clocks. This is what the clk_disable_critical and
>     clk_enable_critical functions are here for.

+1

> > Note that I don't think this implementation can really work in the near
> > future. Today we do not catch unbalanced calls to clk_enable and
> > clk_disable, but I have a patch that catches this and WARNs loudly in my
> > private tree. More on that in the next stanza.
> > 
> > What I don't understand is if there is ever a case for a clock consumer
> > driver to ever call clk_enable_critical... I do not think there is. What
> > you're trying to protect against is having the clock disabled BEFORE
> > that "knowledgeable driver" has a chance to enable it.

In my mind and in this implementation  clk_disable_critical() will be
used _first_ by the knowledgeable driver, then when the knowledgeable
driver has finished whatever it was doing (shutting down banks of RAM
etc...), it should then call clk_enable_critical() to reset the clock
state back to the way it found it i.e. enabled and marked as critical.

> It's really about what we want the API to look like in the second
> case.
> 
> Do we want such drivers to still call clk_prepare_enable? Some other
> function? Should they assume that the clock has already been enabled,
> or do we want a handover, or a force disable, or whatever.
> 
> I guess we should discuss those questions, before starting to think
> about how to implement it.
> 
> IMHO, I think that the existing way of doing should be used, or at
> least with the same mindset to avoid confusion, errors, and
> misinformed reviews.
> 
> So I'd expect the drivers to do something like:
> 
> probe:
>   clk_get
>   clk_critical_enable

Well becuase the clock has been marked as critical, a reference has
already been taken, so even if there are 0 users the clock now has 2
references attributed to it.

> remove / probe error path:
>   clk_critical_disable
>   clk_put

I think we should assume that the clock is already running in the
knowledgeable driver:

start-up:
  __set_critical_clocks()

probe:
  clk_get()

suspend (or whatever reason the driver wishes to disable the clk):
  clk_disable_critical()

resume (or whatever ...):
  clk_enable_critical()

remove:
  clk_put() /* Or just rely on devm_*() */ 

> and use the clk_critical_enable and clk_critical_disable whenever
> needed, and thing would just work as intended.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html