On Mon, Aug 1, 2011 at 2:33 PM, Felipe Balbi <balbi@xxxxxx> wrote:
Hi,
On Fri, Jul 29, 2011 at 08:43:49PM +0530, Govindraj wrote:
[giant snip]
trailing... but you know that :-p
> Actually there is much more than this:
>
> <<SNIP>>
>
> diff --git a/arch/arm/mach-omap2/clock.c b/arch/arm/mach-omap2/clock.c
> index 180299e..221ffb9 100644
> --- a/arch/arm/mach-omap2/clock.c
> +++ b/arch/arm/mach-omap2/clock.c
> @@ -12,7 +12,8 @@
> * it under the terms of the GNU General Public License version 2 as
> * published by the Free Software Foundation.
> */
> -#undef DEBUG
> +//#undef DEBUG
> +#define DEBUG
yes true.
this hunk is unnecessary. Clocks are always on when they are called.
> @@ -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());
The problem is:
[1]:
runtime_put -> *power.lock* - > rpm->suspend -> above pr_debug -> console_write -> get_sync
-> *power.lock* -> rpm resume
power.lock contention.
these two is ok.
> @@ -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);
trailing.
> 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
instead of doing checks, you could move the print to the end of the
> @@ -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);
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.
check not needed, clocks are still on.
> @@ -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);
scenario [1]
move it further down.
>
> 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);
instead of adding check, move the print before _disable_clocks(oh).
> @@ -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);
I believe clocks are still on here too, no checks needed.
> @@ -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);
trailing.
> 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
move it to the end of the function.
> @@ -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->pdev.name);
> + if (strcmp(od->hwmods[0]->class->name, "uart"))
> + pr_debug("omap_device: %s: activating\n", od->pdev.name);
move it further down.
> @@ -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->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->pdev.name, od->pm_lat_level,
> + act_lat);
->activate_func() has already been called here, clocks are already on.
>
> 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->pdev.name, od->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->pdev.name, od->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->pdev.name, od->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->pdev.name, od->pdev.id,
> + od->pm_lat_level, act_lat,
> + odpl->activate_lat);
clocks are still on here.
> @@ -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->pdev.name);
> + if (strcmp(od->hwmods[0]->class->name, "uart"))
> + pr_debug("omap_device: %s: deactivating\n", od->pdev.name);
I'll leave this to Kevin to decide what to do, but clocks are off
> @@ -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->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->pdev.name, od->pm_lat_level,
> + deact_lat);
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.
--
Thanks,
Govindraj.R
and here...
> if (deact_lat > odpl->deactivate_lat) {
> odpl->deactivate_lat_worst = deact_lat;
> if (odpl->flags & OMAP_DEVICE_LATENCY_AUTO_ADJUST) {
> odpl->deactivate_lat = deact_lat;
> - pr_warning("omap_device: %s.%d: new worst case "
> - "deactivate latency %d: %llu\n",
> - od->pdev.name, od->pdev.id,
> - od->pm_lat_level, deact_lat);
> - } else
> - pr_warning("omap_device: %s.%d: deactivate "
> + if (strcmp(od->hwmods[0]->class->name, "uart"))
> + pr_warning("omap_device: %s.%d: new worst case "
> + "deactivate latency %d: %llu\n",
> + od->pdev.name, od->pdev.id,
> + od->pm_lat_level, deact_lat);
> + } else {
> + if (strcmp(od->hwmods[0]->class->name, "uart"))
> + pr_warning("omap_device: %s.%d: deactivate "
> "latency %d higher than exptected. "
> "(%llu > %d)\n",
> od->pdev.name, od->pdev.id,
> od->pm_lat_level, deact_lat,
> odpl->deactivate_lat);
> + }
--
balbi
_______________________________________________ linux-pm mailing list linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/linux-pm
