From: khilman@deeprootsystems•com (Kevin Hilman)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH 03/10] OMAP3: PM: move device-specific special cases from PM core into CPUidle
Date: Thu, 23 Sep 2010 17:16:14 -0700 [thread overview]
Message-ID: <87vd5wm1wx.fsf@deeprootsystems.com> (raw)
In-Reply-To: <87eickowdo.fsf@deeprootsystems.com> (Kevin Hilman's message of "Thu, 23 Sep 2010 16:47:31 -0700")
Kevin Hilman <khilman@deeprootsystems•com> writes:
> Paul Walmsley <paul@pwsan•com> writes:
>
>> Hi Kevin
>>
>> On Tue, 21 Sep 2010, Kevin Hilman wrote:
>>
>>> Paul Walmsley <paul@pwsan•com> writes:
>>>
>>> > On Wed, 15 Sep 2010, Kevin Hilman wrote:
>>> >
>>> >> In an effort to simplify the core idle path, move any device-specific
>>> >> special case handling from the core PM idle path into the CPUidle
>>> >> pre-idle checking path.
>>> >
>>> > As with the original patch, I don't quite understand the improvement
>>> > here. In particular, this part:
>>> >
>>> >> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c
>>> >> index 3d3d035..0923b82 100644
>>> >> --- a/arch/arm/mach-omap2/cpuidle34xx.c
>>> >> +++ b/arch/arm/mach-omap2/cpuidle34xx.c
>>> >> @@ -233,14 +234,54 @@ static int omap3_enter_idle_bm(struct cpuidle_device *dev,
>>> >> struct cpuidle_state *state)
>>> >> {
>>> >> struct cpuidle_state *new_state = next_valid_state(dev, state);
>>> >> + u32 core_next_state, per_next_state = 0, per_saved_state = 0;
>>> >> + u32 cam_state;
>>> >> + struct omap3_processor_cx *cx;
>>> >> + int ret;
>>> >>
>>> >> if ((state->flags & CPUIDLE_FLAG_CHECK_BM) && omap3_idle_bm_check()) {
>>> >> BUG_ON(!dev->safe_state);
>>> >> new_state = dev->safe_state;
>>> >> + goto select_state;
>>> >> + }
>>> >> +
>>> >> + cx = cpuidle_get_statedata(state);
>>> >> + core_next_state = cx->core_state;
>>> >> +
>>> >> + /*
>>> >> + * Prevent idle completely if CAM is active.
>>> >> + * CAM does not have wakeup capability in OMAP3.
>>> >> + */
>>> >> + cam_state = pwrdm_read_pwrst(cam_pd);
>>> >> + if (cam_state == PWRDM_POWER_ON) {
>>> >> + new_state = dev->safe_state;
>>> >> + goto select_state;
>>> >> }
>>> >>
>>> >> + /*
>>> >> + * Prevent PER off if CORE is not in retention or off as this
>>> >> + * would disable PER wakeups completely.
>>> >> + */
>>> >> + per_next_state = per_saved_state = pwrdm_read_next_pwrst(per_pd);
>>> >> + if ((per_next_state == PWRDM_POWER_OFF) &&
>>> >> + (core_next_state > PWRDM_POWER_RET)) {
>>> >> + per_next_state = PWRDM_POWER_RET;
>>> >> + pwrdm_set_next_pwrst(per_pd, per_next_state);
>>> >> + }
>>> >> +
>>> >> + /* Are we changing PER target state? */
>>> >> + if (per_next_state != per_saved_state)
>>> >> + pwrdm_set_next_pwrst(per_pd, per_next_state);
>>> >
>>> > In this case, the PER / CORE constraints don't have anything to do with
>>> > the MPU or CPUIdle, so they don't seem to belong in the CPUIdle code. The
>>> > extra comments are certainly nice -- they make it more clear as to what is
>>> > going on here -- but maybe those can just be added to pm34xx.c ?
>>>
>>> CPUidle currently manages MPU and CORE powerdomains, so the CORE
>>> constraints seem to make perfect sense here (at least to me.)
>>
>> I do mean CORE also -- basically, anything that is not the CPU. IMHO
>> CPUIdle shouldn't manage CORE directly since it's not part of the CPU.
>> Also since OMAPs have other processors (e.g., DSP, DMA, etc) that can use
>> the CORE independently of the CPU's power state, it doesn't make sense for
>> that code to live inside CPUIdle -- probably it should live in some type
>> of SystemIdle, CORE powerdomain idle or L3 idle. Again IMHO, the C states
>> should only represent the MPU's idle state.
>>
>>> The question is probably more about the PER constraints.
>>>
>>> The basic goal of this is to streamline the core idle (omap_sram_idle())
>>> to be the minimum streamline idle, and to move all the constraint
>>> checking and activity checking to higher layers (like CPUidle.)
>>> Specifically, I'm working towards moving the device-specific idle
>>> constraints out of the core idle path (omap_sram_idle()) and move them
>>> into higher layers where we're checking for activity etc.
>>>
>>> This is just a baby step towards moving the device-idle out of CPUidle
>>> completely to a place where it can be managed by the driver themeselves
>>> using runtime PM or by using constraints instead of these hard-coded
>>> hacks.
>>
>> I agree completely with the goal; it's the implementation that I don't
>> like ;-) But if you agree with the basic idea, that CORE / PER /
>> whatever-idle should ultimately go elsewhere, since I don't have time to
>> come up with a constructive alternative at the moment, would you be
>> willing to just drop a FIXME comment in that code, near the CAM and the
>> PER / CORE stuff, that mentions that that code should ultimately be
>> segmented out into its own idle code?
>
> Absolutely... will do.
>
Here's an updated patch, which will be included in the forthcoming
pm-next pull request. I updated the changelog with a 'NOTE' and also
added a FIXME in the code.
Thanks for the feedback Paul.
Kevin
next prev parent reply other threads:[~2010-09-24 0:16 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-09-15 23:55 [PATCH 00/10] OMAP: misc PM queue for 2.6.37 Kevin Hilman
2010-09-15 23:55 ` [PATCH 01/10] OMAP3: PM: whitespace cleanup around IO wakeup enable Kevin Hilman
2010-09-15 23:55 ` [PATCH 02/10] OMAP: PM debugfs removing OMAP3 hardcodings Kevin Hilman
2010-09-15 23:55 ` [PATCH 03/10] OMAP3: PM: move device-specific special cases from PM core into CPUidle Kevin Hilman
2010-09-21 7:52 ` Paul Walmsley
2010-09-21 17:00 ` Kevin Hilman
2010-09-23 23:25 ` Paul Walmsley
2010-09-23 23:47 ` Kevin Hilman
2010-09-24 0:16 ` Kevin Hilman [this message]
2010-09-15 23:55 ` [PATCH 04/10] OMAP4: pm.c extensions for OMAP4 support Kevin Hilman
2010-09-15 23:55 ` [PATCH 05/10] omap: pm-debug: Move common debug code to pm-debug.c Kevin Hilman
2010-09-15 23:55 ` [PATCH 06/10] omap: pm-debug: Enable wakeup_timer_milliseconds debugfs entry Kevin Hilman
2010-09-15 23:55 ` [PATCH 07/10] omap: pm: Move set_pwrdm_state routine to common pm.c Kevin Hilman
2010-09-15 23:55 ` [PATCH 08/10] OMAP clockdomain: initialize clockdomain registers when the clockdomain layer starts Kevin Hilman
2010-09-15 23:55 ` [PATCH 09/10] Revert "OMAP: omap_device: add omap_device_is_valid()" Kevin Hilman
2010-09-15 23:55 ` [PATCH 10/10] OMAP: omap_device: make all devices a child of a new parent device Kevin Hilman
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=87vd5wm1wx.fsf@deeprootsystems.com \
--to=khilman@deeprootsystems$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox