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

Lee Jones
2015-07-31T08:32:16Z

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