Hi Tomi,
On 6/4/2011 10:01 AM, Valkeinen, Tomi wrote:
On Fri, 2011-06-03 at 15:53 -0700, Kevin Hilman wrote:
Tomi Valkeinen<tomi.valkeinen@xxxxxx> writes:
Hi Kevin,
On Fri, 2011-06-03 at 09:45 -0700, Kevin Hilman wrote:
Tomi Valkeinen<tomi.valkeinen@xxxxxx> writes:
Use PM runtime and HWMOD support to handle enabling and disabling of DSS
modules.
Each DSS module will have get and put functions which can be used to
enable and disable that module. The functions use pm_runtime and hwmod
opt-clocks to enable the hardware.
Signed-off-by: Tomi Valkeinen<tomi.valkeinen@xxxxxx>
[...]
+int dispc_runtime_get(void)
+{
+ int r;
+
+ mutex_lock(&dispc.runtime_lock);
It's not clear to me what the lock is trying to protect. I guess it's
the counter? I don't think it should be needed...
Yes, the counter. I don't think
if (dispc.runtime_count++ == 0)
is thread safe.
OK, if it's just the counter, you can drop the mutex and use an atomic
variable and use atomic_inc(), atomic_dec() etc. Then it will be clear
from reading what exactly is protected.
Hmm, sorry, my mistake. It's actually for the whole function: we can't
do "put" before the whole "get" has finished. Otherwise we could end up,
for example, disabling a clock before enabling it.
+ if (dispc.runtime_count++ == 0) {
You shouldn't need your own use-counting here. The runtime PM core is
already doing usage counting.
Instead, you need to use the ->runtime_suspend() and ->runtime_resume()
callbacks (in dev_pm_ops). These callbacks are called by the runtime PM
core when the usage count goes to/from zero.
Yes, I wish I could do that =).
I tried to explain this in the 00-patch, I guess I should've explained
it in this patch also. Perhaps also in a comment.
Oops, my fault. I didn't read the whole 00 patch. I'm pretty ignorant
about DSS, so I was focused in on the runtime PM implementation only.
Sorry about that.
From the introduction:
---
Due to DSS' peculiar clock setup the code is not as elegant as I'd like. The
problem is that on OMAP4 we have to enable an optional clock before calling
pm_runtime_get(), and similarly we need to keep the optional clock enabled
until after pm_runtime_put() has been called.
Just to clarify, what exactly does the opt clock have to be enabled for?
I'm not sure if this is a valid definition, but in my mind the opt clock
has two uses: 1) a functional clock, to make the HW tick and registers
accessible, and 2) act as a source clock for the outgoing pixel clock.
That terminology in the PRCM just means that an opt clock will not be
handled automatically by the PRCM and will require SW control.
This is not the case for mandatory clock. Upon module enable the PRCM
will ensure that all mandatory clocks (functional and interface) are
enabled automagically. If the clock is marked as optional it means that
the SW will have to enable it explicitly before enabling the module.
The modulemode was not there previously on OMAP2 & 3, but it is more or
less equivalent to icken=1 + fcken=1.
This idea was to hide the explicit clock management especially for the
iclk that were already supposed to always be in autoidle.
Since the current hwmod + clock fmwks are still based on the previous
clock centric approach we used to have on OMAP2 & 3, we cannot match
properly the modulemode to any clock and thus cannot handle properly the
DSS fclk as the main clock instead of the optional clock.
A temporary option will be to consider the modulemode as the interface
clock and thus remove it from the main_clk and replace it by the real
DSS fclk.
It should work be will unfortunately not be compliant with PRCM
recommendation to enable the modulemode once every clocks are enabled.
The long term solution is to update the hwmod fmwk to handle the
modulemode directly and not through the clock fmwk. It will allow the
main_clk to be connnected to the dss_fclk.
You will not have that nasty opt_clock issue anymore.
Regards,
Benoit
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
