Hi,

(fix your mailer dude)

On Mon, Aug 01, 2011 at 03:26:52PM +0530, Raja, Govindraj wrote:

     > @@ -254,14 +255,14 @@ void omap2_clk_disable(struct clk *clk)
     > � � � � � � � return;
     > � � � }
     >
     > - � � pr_debug("clock: %s: decrementing usecount\n", clk->name);
     > +// � pr_debug("clock: %s: decrementing usecount\n", clk->name);
     >
     > � � � clk->usecount--;
     >
     > � � � if (clk->usecount > 0)
     > � � � � � � � return;
     >
     > - � � pr_debug("clock: %s: disabling in hardware\n", clk->name);
     > +// � pr_debug("clock: %s: disabling in hardware\n", clk->name);
     >
     > � � � if (clk->ops && clk->ops->disable) {
     > � � � � � � � trace_clock_disable(clk->name, 0, smp_processor_id());

     this hunk is unnecessary. Clocks are always on when they are called.

   The problem is:
   [1]:
   runtime_put -> *power.lock* - > rpm->suspend -> above pr_debug ->
   console_write -> get_sync
   -> *power.lock* -> rpm resume
   power.lock contention.

Are you sure ? If the device is still on, won't that get_sync() only
increase the pm counter ? Instead of going through everything ?? Oh
well, this is becoming quite racy :-(

     > @@ -290,14 +291,14 @@ int omap2_clk_enable(struct clk *clk)
     > �{
     > � � � int ret;
     >
     > - � � pr_debug("clock: %s: incrementing usecount\n", clk->name);
     > +// � pr_debug("clock: %s: incrementing usecount\n", clk->name);
     >
     > � � � clk->usecount++;
     >
     > � � � if (clk->usecount > 1)
     > � � � � � � � return 0;
     >
     > - � � pr_debug("clock: %s: enabling in hardware\n", clk->name);
     > +// � pr_debug("clock: %s: enabling in hardware\n", clk->name);

     these two is ok.
     > diff --git a/arch/arm/mach-omap2/omap_hwmod.c
     b/arch/arm/mach-omap2/omap_hwmod.c
     > index 7ed0479..8ca7d40 100644
     > --- a/arch/arm/mach-omap2/omap_hwmod.c
     > +++ b/arch/arm/mach-omap2/omap_hwmod.c
     > @@ -124,7 +124,8 @@
     > � * XXX error return values should be checked to ensure that they are
     > � * appropriate
     > � */
     > -#undef DEBUG
     > +//#undef DEBUG
     > +#define DEBUG

     trailing.
     > @@ -597,7 +598,8 @@ static int _enable_clocks(struct omap_hwmod *oh)
     > �{
     > � � � int i;
     >
     > - � � pr_debug("omap_hwmod: %s: enabling clocks\n", oh->name);
     > + � � if (strcmp(oh->class->name, "uart"))
     > + � � � � � � pr_debug("omap_hwmod: %s: enabling clocks\n", oh->name);

     instead of doing checks, you could move the print to the end of the
     function, when clocks are already enabled. When doind that, of course,
     update the comment to say "%s: clocks enabled\n".

   the problem is the prints causing power.lock contention same as�
   the�scenario�in [1] above.
   �

     > @@ -627,7 +629,8 @@ static int _disable_clocks(struct omap_hwmod *oh)
     > �{
     > � � � int i;
     >
     > - � � pr_debug("omap_hwmod: %s: disabling clocks\n", oh->name);
     > + � � if (strcmp(oh->class->name, "uart"))
     > + � � � � � � pr_debug("omap_hwmod: %s: disabling clocks\n",
     oh->name);

     check not needed, clocks are still on.

   scenario�[1]
   �

     >
     > � � � if (oh->_clk)
     > � � � � � � � clk_disable(oh->_clk);
     > @@ -1232,7 +1235,8 @@ static int _enable(struct omap_hwmod *oh)
     > � � � � � � � return -EINVAL;
     > � � � }
     >
     > - � � pr_debug("omap_hwmod: %s: enabling\n", oh->name);
     > + � � if (strcmp(oh->class->name, "uart"))
     > + � � � � � � pr_debug("omap_hwmod: %s: enabling\n", oh->name);

     move it further down.
     > @@ -1264,8 +1268,9 @@ static int _enable(struct omap_hwmod *oh)
     > � � � � � � � }
     > � � � } else {
     > � � � � � � � _disable_clocks(oh);
     > - � � � � � � pr_debug("omap_hwmod: %s: _wait_target_ready: %d\n",
     > - � � � � � � � � � � �oh->name, r);
     > + � � � � � � if (strcmp(oh->class->name, "uart"))
     > + � � � � � � � � � � pr_debug("omap_hwmod: %s: _wait_target_ready:
     %d\n",
     > + � � � � � � � � � � � � � � �oh->name, r);

     instead of adding check, move the print before _disable_clocks(oh).
     > @@ -1287,7 +1292,8 @@ static int _idle(struct omap_hwmod *oh)
     > � � � � � � � return -EINVAL;
     > � � � }
     >
     > - � � pr_debug("omap_hwmod: %s: idling\n", oh->name);
     > + � � if (strcmp(oh->class->name, "uart"))
     > + � � � � � � pr_debug("omap_hwmod: %s: idling\n", oh->name);

     I believe clocks are still on here too, no checks needed.
     > diff --git a/arch/arm/plat-omap/omap_device.c
     b/arch/arm/plat-omap/omap_device.c
     > index 49fc0df..7b27704 100644
     > --- a/arch/arm/plat-omap/omap_device.c
     > +++ b/arch/arm/plat-omap/omap_device.c
     > @@ -75,7 +75,8 @@
     > � * (device must be reinitialized at this point to use it again)
     > � *
     > � */
     > -#undef DEBUG
     > +//#undef DEBUG
     > +#define DEBUG

     trailing.
     > @@ -114,7 +115,8 @@ static int _omap_device_activate(struct
     > omap_device *od, u8 ignore_lat)
     > �{
     > � � � struct timespec a, b, c;
     >
     > - � � pr_debug("omap_device: %s: activating\n", od->[2]pdev.name);
     > + � � if (strcmp(od->hwmods[0]->class->name, "uart"))
     > + � � � � � � pr_debug("omap_device: %s: activating\n",
     od->[3]pdev.name);

     move it to the end of the function.
     > @@ -138,25 +140,29 @@ static int _omap_device_activate(struct
     > omap_device *od, u8 ignore_lat)
     > � � � � � � � c = timespec_sub(b, a);
     > � � � � � � � act_lat = timespec_to_ns(&c);
     >
     > - � � � � � � pr_debug("omap_device: %s: pm_lat %d: activate: elapsed
     time "
     > - � � � � � � � � � � �"%llu nsec\n", od->[4]pdev.name,
     od->pm_lat_level,
     > - � � � � � � � � � � �act_lat);
     > + � � � � � � if (strcmp(od->hwmods[0]->class->name, "uart"))
     > + � � � � � � � � � � pr_debug("omap_device: %s: pm_lat %d: activate:
     elapsed time "
     > + � � � � � � � � � � � � � � �"%llu nsec\n", od->[5]pdev.name,
     od->pm_lat_level,
     > + � � � � � � � � � � � � � � �act_lat);

     move it further down.
     >
     > � � � � � � � if (act_lat > odpl->activate_lat) {
     > � � � � � � � � � � � odpl->activate_lat_worst = act_lat;
     > � � � � � � � � � � � if (odpl->flags &
     OMAP_DEVICE_LATENCY_AUTO_ADJUST) {
     > � � � � � � � � � � � � � � � odpl->activate_lat = act_lat;
     > - � � � � � � � � � � � � � � pr_warning("omap_device: %s.%d: new
     worst case "
     > - � � � � � � � � � � � � � � � � � � � �"activate latency %d:
     %llu\n",
     > - � � � � � � � � � � � � � � � � � � � �od->[6]pdev.name,
     od->[7]pdev.id,
     > - � � � � � � � � � � � � � � � � � � � �od->pm_lat_level, act_lat);
     > - � � � � � � � � � � } else
     > - � � � � � � � � � � � � � � pr_warning("omap_device: %s.%d: activate
     "
     > - � � � � � � � � � � � � � � � � � � � �"latency %d higher than
     exptected. "
     > - � � � � � � � � � � � � � � � � � � � �"(%llu > %d)\n",
     > - � � � � � � � � � � � � � � � � � � � �od->[8]pdev.name,
     od->[9]pdev.id,
     > - � � � � � � � � � � � � � � � � � � � �od->pm_lat_level, act_lat,
     > - � � � � � � � � � � � � � � � � � � � �odpl->activate_lat);
     > + � � � � � � � � � � � � � � if (strcmp(od->hwmods[0]->class->name,
     "uart"))
     > + � � � � � � � � � � � � � � � � � � pr_warning("omap_device: %s.%d:
     new worst case "
     > + � � � � � � � � � � � � � � � � � � � � � � "activate latency %d:
     %llu\n",
     > + � � � � � � � � � � � � � � � � � � � � � � od->[10]pdev.name,
     od->[11]pdev.id,
     > + � � � � � � � � � � � � � � � � � � � � � � od->pm_lat_level,
     act_lat);
     > + � � � � � � � � � � } else {
     > + � � � � � � � � � � � � � � if (strcmp(od->hwmods[0]->class->name,
     "uart"))
     > + � � � � � � � � � � � � � � � � � � pr_warning("omap_device: %s.%d:
     activate "
     > + � � � � � � � � � � � � � � � � � � � � � � "latency %d higher than
     exptected. "
     > + � � � � � � � � � � � � � � � � � � � � � � "(%llu > %d)\n",
     > + � � � � � � � � � � � � � � � � � � � � � � od->[12]pdev.name,
     od->[13]pdev.id,
     > + � � � � � � � � � � � � � � � � � � � � � � od->pm_lat_level,
     act_lat,
     > + � � � � � � � � � � � � � � � � � � � � � � odpl->activate_lat);

     ->activate_func() has already been called here, clocks are already on.
     > @@ -183,7 +189,8 @@ static int _omap_device_deactivate(struct
     > omap_device *od, u8 ignore_lat)
     > �{
     > � � � struct timespec a, b, c;
     >
     > - � � pr_debug("omap_device: %s: deactivating\n", od->[14]pdev.name);
     > + � � if (strcmp(od->hwmods[0]->class->name, "uart"))
     > + � � � � � � pr_debug("omap_device: %s: deactivating\n",
     od->[15]pdev.name);

     clocks are still on here.
     > @@ -206,25 +213,29 @@ static int _omap_device_deactivate(struct
     > omap_device *od, u8 ignore_lat)
     > � � � � � � � c = timespec_sub(b, a);
     > � � � � � � � deact_lat = timespec_to_ns(&c);
     >
     > - � � � � � � pr_debug("omap_device: %s: pm_lat %d: deactivate:
     elapsed time "
     > - � � � � � � � � � � �"%llu nsec\n", od->[16]pdev.name,
     od->pm_lat_level,
     > - � � � � � � � � � � �deact_lat);
     > + � � � � � � if (strcmp(od->hwmods[0]->class->name, "uart"))
     > + � � � � � � � � � � pr_debug("omap_device: %s: pm_lat %d:
     deactivate: elapsed time "
     > + � � � � � � � � � � � � � � �"%llu nsec\n", od->[17]pdev.name,
     od->pm_lat_level,
     > + � � � � � � � � � � � � � � �deact_lat);

     I'll leave this to Kevin to decide what to do, but clocks are off
     here...

   Yes fine.�
   Since most of these prints will be printed if DEBUG macro
   is defined in respective files and *debug* is used in command line.
   Can't leave uart clocks active always on debug cases.
   [If *debug* �used as command line]
   and gate uart clocks only for non debug cases.
   With this approach�at least�we can have a clean solution
   in uart driver also without adding clock gating from idle path.
   Not sure if this�agreeable.
   As of now gating from idle path seems to be only clean approach.

I see.. that could be one way... let's see how Kevin feels about it
though.

-- 
balbi