From: "Raja, Govindraj" <govindraj.raja@ti.com>
To: balbi@ti.com
Cc: Partha Basak <p-basak2@ti.com>, Tero Kristo <t-kristo@ti.com>,
linux-serial@vger.kernel.org, Govindraj <govindraj.ti@gmail.com>,
linux-pm@lists.linux-foundation.org, linux-omap@vger.kernel.org
Subject: Re: [RFC v2]: Issues implementing clock handling mechanism within UART driver
Date: Mon, 1 Aug 2011 15:26:52 +0530 [thread overview]
Message-ID: <CAMrsUdL_9mzKsjrKf=spihxcRTQc6Tsr+MAP9GFAz_7sE27B6Q@mail.gmail.com> (raw)
In-Reply-To: <20110801090328.GG31013@legolas.emea.dhcp.ti.com>
[-- Attachment #1.1: Type: text/plain, Size: 11681 bytes --]
On Mon, Aug 1, 2011 at 2:33 PM, Felipe Balbi <balbi@ti.com> wrote:
> 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
>
yes true.
> > @@ -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.
>
> > @@ -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->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...
>
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
> > 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 #1.2: Type: text/html, Size: 16868 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
next prev parent reply other threads:[~2011-08-01 9:56 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-07-28 9:29 [RFC v2]: Issues implementing clock handling mechanism within UART driver Govindraj.R
2011-07-29 9:55 ` Felipe Balbi
2011-07-29 9:55 ` Felipe Balbi
2011-07-29 11:24 ` Govindraj
2011-07-29 11:24 ` Govindraj
2011-07-29 11:37 ` Felipe Balbi
2011-07-29 11:59 ` Govindraj
2011-07-29 11:59 ` Govindraj
2011-07-29 12:19 ` Felipe Balbi
2011-07-29 12:19 ` Felipe Balbi
2011-07-29 12:58 ` Govindraj
2011-07-29 12:58 ` Govindraj
2011-07-29 14:02 ` Felipe Balbi
2011-07-29 15:13 ` Govindraj
2011-08-01 9:03 ` Felipe Balbi
2011-08-01 9:03 ` Felipe Balbi
2011-08-01 9:56 ` Raja, Govindraj [this message]
2011-08-01 10:02 ` Felipe Balbi
2011-08-01 12:46 ` Govindraj
2011-08-01 12:46 ` Govindraj
2011-08-01 10:02 ` Felipe Balbi
2011-08-01 10:00 ` Govindraj
2011-08-01 10:00 ` Govindraj
2011-07-29 15:13 ` Govindraj
2011-07-29 14:02 ` Felipe Balbi
2011-07-29 11:37 ` Felipe Balbi
2011-07-28 9:29 Govindraj.R
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAMrsUdL_9mzKsjrKf=spihxcRTQc6Tsr+MAP9GFAz_7sE27B6Q@mail.gmail.com' \
--to=govindraj.raja@ti.com \
--cc=balbi@ti.com \
--cc=govindraj.ti@gmail.com \
--cc=linux-omap@vger.kernel.org \
--cc=linux-pm@lists.linux-foundation.org \
--cc=linux-serial@vger.kernel.org \
--cc=p-basak2@ti.com \
--cc=t-kristo@ti.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.