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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
