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
Attachment:
signature.asc
Description: Digital signature
_______________________________________________ linux-pm mailing list linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/linux-pm
