Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime & HWMOD support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux