On Fri, 31 Jul 2015, Maxime Ripard wrote:

On Thu, Jul 30, 2015 at 03:47:20PM -0700, Michael Turquette wrote:

quoted

quoted

quoted

quoted

   */

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

quoted

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