Re: [PATCH 05/11] OMAPDSS: add clk_prepare and clk_unprepare

On Mon, 2012-06-25 at 17:18 +0530, Rajendra Nayak wrote:

On Monday 25 June 2012 01:28 PM, Tomi Valkeinen wrote:

quoted

On Mon, 2012-06-25 at 12:29 +0530, Rajendra Nayak wrote:

quoted

On Monday 25 June 2012 11:37 AM, Tomi Valkeinen wrote:

quoted

Hi,

On Fri, 2012-06-22 at 19:18 +0530, Rajendra Nayak wrote:

quoted

In preparation of OMAP moving to Common Clk Framework(CCF) add clk_prepare()
and clk_unprepare() for the omapdss clocks.

You used clk_prepare and clk_unprepare instead of clk_prepare_enable and
clk_disable_unprepare. I didn't check the dss driver yet, but my hunch
is that the clocks are normally not enabled/disabled from atomic
context.

What does the prepare/unprepare actually do? Is there any benefit in
delaying preparing, i.e. is there a difference between prepare right
after clk_get, or prepare right before clk_enable? (And similarly for
unprepare)

clk_prepare/unprepare are useful for clocks which need the 'enable'
logic to be implemented as a slow part (which can sleep) and a fast part
(which does not sleep). For all the dss clocks in question we don't need
a slow part and hence they do not have a .clk_prepare/unprepare
platform hook.

The framework however still does prepare usecounting (it has a prepare
count and an enable count, and prepare count is expected to be non-zero
while the clock is being enabled) and uses a mutex around to guard it.
So while the dss driver would do multiple clk_enable/disable while its
active, it seems fair to just prepare/unprepare the clocks once just
after clk_get() and before clk_put() in this particular case.

But the driver should not presume anything special about the clocks. In
this case the dss driver would presume that the clocks it uses do not
have prepare and unprepare hooks.

If the generally proper way to use prepare/unprepare is in combination
of enable/disable, then I think we should try to do that.

makes sense. Lets see if any of the clk_enable/disable happen in  atomic
context, if not it would be just a matter of replacing all with a
clk_prepare_enable/disable_unprepare. Else we might have to find a safe
place sometime before clk_enable to prepare the clk and after
clk_disable to unprepare it.

quoted

I'll check if any of the dss clocks are enabled or disabled in atomic
context.

venc and hdmi use clk_enable/disable in runtime PM callbacks (suspend &
resume). If I understand correctly, the callbacks are not called in
atomic context if pm_runtime_irq_safe() has not been used. And it is not
used in omapdss.

dsi uses clk_enable/disable in a different manner, but not in atomic
context.

So as far as I see, clocks are never handled in atomic context. Is
everything related to the base clk stuff already in mainline? Can I take
the clk_prepare/unprepare patch into my omapdss tree?


A question about clk_prepare/unprepare, not directly related: let's say
I have a driver for some HW block. The driver doesn't use clk functions,
but uses runtime PM. The driver also sets pm_runtime_irq_safe().

Now, the driver can call pm_runtime_get_sync() in an atomic context, and
this would lead to the underlying framework (hwmod, omap_device, I don't
know who =) enabling the func clock for that HW. But this would happen
in atomic context, so the underlying framework can't use clk_prepare.

How does the underlying framework handle that case? (sorry if that's a
stupid question =).

 Tomi