OMAPDSS: add clk_prepare and clk_unprepare

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

Signed-off-by: Rajendra Nayak <redacted>
Cc: Tomi Valkeinen <redacted>
Cc: <redacted>
---
 drivers/video/omap2/dss/dsi.c  |    5 ++++-
 drivers/video/omap2/dss/hdmi.c |    5 ++++-
 drivers/video/omap2/dss/venc.c |    5 ++++-
 3 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index ca8382d..81b474d 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c

@@ -4776,6 +4776,7 @@ static int dsi_get_clocks(struct platform_device *dsidev)
 	}
 
 	dsi->sys_clk = clk;
+	clk_prepare(dsi->sys_clk);
 
 	return 0;
 }

@@ -4786,8 +4787,10 @@ static void dsi_put_clocks(struct platform_device *dsidev)
 
 	if (dsi->dss_clk)
 		clk_put(dsi->dss_clk);
-	if (dsi->sys_clk)
+	if (dsi->sys_clk) {
+		clk_unprepare(dsi->sys_clk);
 		clk_put(dsi->sys_clk);
+	}
 }
 
 static void __init dsi_probe_pdata(struct platform_device *dsidev)

diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index 8195c71..0b7ded3 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c

@@ -544,14 +544,17 @@ static int hdmi_get_clocks(struct platform_device *pdev)
 	}
 
 	hdmi.sys_clk = clk;
+	clk_prepare(hdmi.sys_clk);
 
 	return 0;
 }
 
 static void hdmi_put_clocks(void)
 {
-	if (hdmi.sys_clk)
+	if (hdmi.sys_clk) {
+		clk_unprepare(hdmi.sys_clk);
 		clk_put(hdmi.sys_clk);
+	}
 }
 
 #if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)

diff --git a/drivers/video/omap2/dss/venc.c b/drivers/video/omap2/dss/venc.c
index 2b89739..62f395d 100644
--- a/drivers/video/omap2/dss/venc.c
+++ b/drivers/video/omap2/dss/venc.c

@@ -818,14 +818,17 @@ static int venc_get_clocks(struct platform_device *pdev)
 	}
 
 	venc.tv_dac_clk = clk;
+	clk_prepare(venc.tv_dac_clk);
 
 	return 0;
 }
 
 static void venc_put_clocks(void)
 {
-	if (venc.tv_dac_clk)
+	if (venc.tv_dac_clk) {
+		clk_unprepare(venc.tv_dac_clk);
 		clk_put(venc.tv_dac_clk);
+	}
 }
 
 static void __init venc_probe_pdata(struct platform_device *pdev)

-- 
1.7.1

Hi,

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

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)

 Tomi

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

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.

  Tom

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

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.

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

 Tomi

On Mon, Jun 25, 2012 at 09:07:58AM +0300, Tomi Valkeinen wrote:

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)

I think you're looking at this the wrong way.

Think of clk_prepare() as the _sleepable_ part of clk_enable().  So, think
of clk_prepare() as ideally being placed just before clk_enable() with the
exception that if that's not possible (because you're in a non-atomic
context) it should be placed in a position as close to clk_enable() as
possible which is atomic.

Don't think of it as an extra step to be done after clk_get() but as an
extra step needing to be done before clk_enable().

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

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.

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

  Tomi

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

8 more quoted lines

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

On Monday 25 June 2012 06:44 PM, Tomi Valkeinen wrote:

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?

Well the Common Clk framework is already in mainline, but we still don;t
have CONFIG_COMMON_CLK enabled for our builds yet. So until we do so,
clk_prepare/unprepare will just be stubs which do nothing.
I will repost the patch getting rid of the clk_prepare/unprepare and
adding clk_prepare_enable/disable_unprepare instead.

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 =).

No, its not a stupid question at all. I have been thinking about this
too to figure out whats the best way to handle this. For now, the
patches I posted, do an early clk_prepare (like I did for dss) for all
hwmod clocks as I have no way to know which ones will have their
_enable, _idle etc called in atomic context. Maybe I should see if there
is a way I can do so only for those devices which end up calling a
pm_runtime_irq_safe() and not do it early for all.

On Tue, 2012-06-26 at 10:30 +0530, Rajendra Nayak wrote:

quoted

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?

Well the Common Clk framework is already in mainline, but we still don;t
have CONFIG_COMMON_CLK enabled for our builds yet. So until we do so,
clk_prepare/unprepare will just be stubs which do nothing.

But if I understood correctly, clk_prepare and clk_unprepare are anyway
no-ops with dss clocks, even when CONFIG_COMMON_CLK is enabled?

My point was only to understand if I can safely take the patch into
omapdss tree, instead of it going through l-o, to avoid any possible
conflicts.

 Tomi

On Tuesday 26 June 2012 12:25 PM, Tomi Valkeinen wrote:

On Tue, 2012-06-26 at 10:30 +0530, Rajendra Nayak wrote:

quoted

quoted

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?

Well the Common Clk framework is already in mainline, but we still don;t
have CONFIG_COMMON_CLK enabled for our builds yet. So until we do so,
clk_prepare/unprepare will just be stubs which do nothing.

But if I understood correctly, clk_prepare and clk_unprepare are anyway
no-ops with dss clocks, even when CONFIG_COMMON_CLK is enabled?

With CONFIG_COMMON_CLK enabled, they will do prepare use-counting with
a mutex lock/unlock around it.

My point was only to understand if I can safely take the patch into
omapdss tree, instead of it going through l-o, to avoid any possible
conflicts.

yes, I don't see any issues with it going through dss tree.

  Tomi

On 20120625-16:14, Tomi Valkeinen wrote:

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 =).

I think it's a good question!

If we're going to call clk_prepare_enable from within a runtime pm
callback then I think we'll need to check if _irq_safe() is set and
conditionally call only clk_enable in such a case.

I'm not a runtime pm expert, but if the driver owns the responsibility
of calling pm_runtime_irq_safe then the driver has the proper context
to know that it should call clk_prepare BEFORE calling
pm_runtime_get_sync.

However if some other framework (outside of the driver) calls
pm_runtime_irq_safe then it will be difficult for the driver to know
what to do...

Regards,
Mike

 Tomi

On Tue, 2012-06-26 at 17:47 -0700, Mike Turquette wrote:

On 20120625-16:14, Tomi Valkeinen wrote:

quoted

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 =).

I think it's a good question!

If we're going to call clk_prepare_enable from within a runtime pm
callback then I think we'll need to check if _irq_safe() is set and
conditionally call only clk_enable in such a case.

I'm not a runtime pm expert, but if the driver owns the responsibility
of calling pm_runtime_irq_safe then the driver has the proper context
to know that it should call clk_prepare BEFORE calling
pm_runtime_get_sync.

That's not quite what I meant. If it's the driver that does clk_enable,
be it in runtime PM callback or not, it's driver's responsibility.

But some clocks are not handled by the driver, but the hwmod/omap_device
framework. Mainly I think this is for the functional and interface
clocks. The driver has no visibility to those, they are implicitly
enabled via pm_runtime_get.

 Tomi

On Wednesday 27 June 2012 09:49 AM, Tomi Valkeinen wrote:

On Tue, 2012-06-26 at 17:47 -0700, Mike Turquette wrote:

quoted

On 20120625-16:14, Tomi Valkeinen wrote:

quoted

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 =).

I think it's a good question!

If we're going to call clk_prepare_enable from within a runtime pm
callback then I think we'll need to check if _irq_safe() is set and
conditionally call only clk_enable in such a case.

I'm not a runtime pm expert, but if the driver owns the responsibility
of calling pm_runtime_irq_safe then the driver has the proper context
to know that it should call clk_prepare BEFORE calling
pm_runtime_get_sync.

That's not quite what I meant. If it's the driver that does clk_enable,
be it in runtime PM callback or not, it's driver's responsibility.

But some clocks are not handled by the driver, but the hwmod/omap_device
framework. Mainly I think this is for the functional and interface
clocks. The driver has no visibility to those, they are implicitly
enabled via pm_runtime_get.

yes, thats the tricky part on how would hwmod/omap_device know if the
driver would use runtime pm within atomic context or non-atomic context.
The driver does inform the runtime pm framework about this by calling a
pm_runtime_irq_safe(), which is then used to set the .irq_safe flag 
telling it to leave the interrupts disabled during callbacks.
This information however does not flow down to hwmod/omap_device in any
way. So the way its currently handled is to do an early prepare of all
hwmod controlled clocks.

I have copied Kevin in case he has any better ideas on how this should
be handled.

  Tomi