Re: [RFC v2]: Issues implementing clock handling mechanism within UART driver

Felipe Balbi
2011-08-01T09:03:30Z

[-- Attachment #1: Type: text/plain, Size: 8335 bytes --]

Hi,

On Fri, Jul 29, 2011 at 08:43:49PM +0530, Govindraj wrote:

[giant snip]

> 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

trailing... but you know that :-p

> @@ -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.

> @@ -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".

> @@ -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.

>
> 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->pdev.name);
> + if (strcmp(od->hwmods[0]->class->name, "uart"))
> + pr_debug("omap_device: %s: activating\n", od->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->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);

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->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);

->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->pdev.name);
> + if (strcmp(od->hwmods[0]->class->name, "uart"))
> + pr_debug("omap_device: %s: deactivating\n", od->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->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);

I'll leave this to Kevin to decide what to do, but clocks are off
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);
> + }

and here...

--
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]