From: Kevin Hilman <khilman@deeprootsystems.com>
To: Paul Walmsley <paul@pwsan.com>
Cc: linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Tero Kristo <tero.kristo@nokia.com>
Subject: Re: [PATCH 03/10] OMAP3: PM: move device-specific special cases from PM core into CPUidle
Date: Tue, 21 Sep 2010 10:00:02 -0700 [thread overview]
Message-ID: <878w2vkp65.fsf@deeprootsystems.com> (raw)
In-Reply-To: <alpine.DEB.2.00.1009210147180.17727@utopia.booyaka.com> (Paul Walmsley's message of "Tue, 21 Sep 2010 01:52:37 -0600 (MDT)")
Paul Walmsley <paul@pwsan.com> writes:
> Hi Kevin,
>
> 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.
>>
>> This keeps the core, interrupts-disabled idle path streamlined and
>> independent of any device-specific handling, and also allows CPUidle
>> to do the checking only for certain C-states as needed. This patch
>> has the device checks in place for all states with the CHECK_BM flag,
>> namely all states >= C2.
>>
>> This patch was inspired by a similar patch written by Tero Kristo as
>> part of a larger series to add INACTIVE state support.
>
> 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.)
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.
Kevin
>> +
>> +select_state:
>> dev->last_state = new_state;
>> - return omap3_enter_idle(dev, new_state);
>> + ret = omap3_enter_idle(dev, new_state);
>> +
>> + /* Restore original PER state if it was modified */
>> + if (per_next_state != per_saved_state)
>> + pwrdm_set_next_pwrst(per_pd, per_saved_state);
>> +
>> + return ret;
>> }
>>
>> DEFINE_PER_CPU(struct cpuidle_device, omap3_idle_dev);
>> @@ -328,7 +369,8 @@ void omap_init_power_states(void)
>> cpuidle_params_table[OMAP3_STATE_C2].threshold;
>> omap3_power_states[OMAP3_STATE_C2].mpu_state = PWRDM_POWER_ON;
>> omap3_power_states[OMAP3_STATE_C2].core_state = PWRDM_POWER_ON;
>> - omap3_power_states[OMAP3_STATE_C2].flags = CPUIDLE_FLAG_TIME_VALID;
>> + omap3_power_states[OMAP3_STATE_C2].flags = CPUIDLE_FLAG_TIME_VALID |
>> + CPUIDLE_FLAG_CHECK_BM;
>>
>> /* C3 . MPU CSWR + Core inactive */
>> omap3_power_states[OMAP3_STATE_C3].valid =
>> @@ -426,6 +468,8 @@ int __init omap3_idle_init(void)
>>
>> mpu_pd = pwrdm_lookup("mpu_pwrdm");
>> core_pd = pwrdm_lookup("core_pwrdm");
>> + per_pd = pwrdm_lookup("per_pwrdm");
>> + cam_pd = pwrdm_lookup("cam_pwrdm");
>>
>> omap_init_power_states();
>> cpuidle_register_driver(&omap3_idle_driver);
>> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
>> index 429268e..bb2ba1e 100644
>> --- a/arch/arm/mach-omap2/pm34xx.c
>> +++ b/arch/arm/mach-omap2/pm34xx.c
>> @@ -346,7 +346,6 @@ void omap_sram_idle(void)
>> int core_next_state = PWRDM_POWER_ON;
>> int core_prev_state, per_prev_state;
>> u32 sdrc_pwr = 0;
>> - int per_state_modified = 0;
>>
>> if (!_omap_sram_idle)
>> return;
>> @@ -391,19 +390,10 @@ void omap_sram_idle(void)
>> if (per_next_state < PWRDM_POWER_ON) {
>> omap_uart_prepare_idle(2);
>> omap2_gpio_prepare_for_idle(per_next_state);
>> - if (per_next_state == PWRDM_POWER_OFF) {
>> - if (core_next_state == PWRDM_POWER_ON) {
>> - per_next_state = PWRDM_POWER_RET;
>> - pwrdm_set_next_pwrst(per_pwrdm, per_next_state);
>> - per_state_modified = 1;
>> - } else
>> + if (per_next_state == PWRDM_POWER_OFF)
>> omap3_per_save_context();
>> - }
>> }
>>
>> - if (pwrdm_read_pwrst(cam_pwrdm) == PWRDM_POWER_ON)
>> - omap2_clkdm_deny_idle(mpu_pwrdm->pwrdm_clkdms[0]);
>> -
>> /* CORE */
>> if (core_next_state < PWRDM_POWER_ON) {
>> omap_uart_prepare_idle(0);
>> @@ -470,8 +460,6 @@ void omap_sram_idle(void)
>> if (per_prev_state == PWRDM_POWER_OFF)
>> omap3_per_restore_context();
>> omap_uart_resume_idle(2);
>> - if (per_state_modified)
>> - pwrdm_set_next_pwrst(per_pwrdm, PWRDM_POWER_OFF);
>> }
>>
>> /* Disable IO-PAD and IO-CHAIN wakeup */
>> --
>> 1.7.2.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
>
> - Paul
WARNING: multiple messages have this Message-ID (diff)
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: Tue, 21 Sep 2010 10:00:02 -0700 [thread overview]
Message-ID: <878w2vkp65.fsf@deeprootsystems.com> (raw)
In-Reply-To: <alpine.DEB.2.00.1009210147180.17727@utopia.booyaka.com> (Paul Walmsley's message of "Tue, 21 Sep 2010 01:52:37 -0600 (MDT)")
Paul Walmsley <paul@pwsan.com> writes:
> Hi Kevin,
>
> 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.
>>
>> This keeps the core, interrupts-disabled idle path streamlined and
>> independent of any device-specific handling, and also allows CPUidle
>> to do the checking only for certain C-states as needed. This patch
>> has the device checks in place for all states with the CHECK_BM flag,
>> namely all states >= C2.
>>
>> This patch was inspired by a similar patch written by Tero Kristo as
>> part of a larger series to add INACTIVE state support.
>
> 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.)
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.
Kevin
>> +
>> +select_state:
>> dev->last_state = new_state;
>> - return omap3_enter_idle(dev, new_state);
>> + ret = omap3_enter_idle(dev, new_state);
>> +
>> + /* Restore original PER state if it was modified */
>> + if (per_next_state != per_saved_state)
>> + pwrdm_set_next_pwrst(per_pd, per_saved_state);
>> +
>> + return ret;
>> }
>>
>> DEFINE_PER_CPU(struct cpuidle_device, omap3_idle_dev);
>> @@ -328,7 +369,8 @@ void omap_init_power_states(void)
>> cpuidle_params_table[OMAP3_STATE_C2].threshold;
>> omap3_power_states[OMAP3_STATE_C2].mpu_state = PWRDM_POWER_ON;
>> omap3_power_states[OMAP3_STATE_C2].core_state = PWRDM_POWER_ON;
>> - omap3_power_states[OMAP3_STATE_C2].flags = CPUIDLE_FLAG_TIME_VALID;
>> + omap3_power_states[OMAP3_STATE_C2].flags = CPUIDLE_FLAG_TIME_VALID |
>> + CPUIDLE_FLAG_CHECK_BM;
>>
>> /* C3 . MPU CSWR + Core inactive */
>> omap3_power_states[OMAP3_STATE_C3].valid =
>> @@ -426,6 +468,8 @@ int __init omap3_idle_init(void)
>>
>> mpu_pd = pwrdm_lookup("mpu_pwrdm");
>> core_pd = pwrdm_lookup("core_pwrdm");
>> + per_pd = pwrdm_lookup("per_pwrdm");
>> + cam_pd = pwrdm_lookup("cam_pwrdm");
>>
>> omap_init_power_states();
>> cpuidle_register_driver(&omap3_idle_driver);
>> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
>> index 429268e..bb2ba1e 100644
>> --- a/arch/arm/mach-omap2/pm34xx.c
>> +++ b/arch/arm/mach-omap2/pm34xx.c
>> @@ -346,7 +346,6 @@ void omap_sram_idle(void)
>> int core_next_state = PWRDM_POWER_ON;
>> int core_prev_state, per_prev_state;
>> u32 sdrc_pwr = 0;
>> - int per_state_modified = 0;
>>
>> if (!_omap_sram_idle)
>> return;
>> @@ -391,19 +390,10 @@ void omap_sram_idle(void)
>> if (per_next_state < PWRDM_POWER_ON) {
>> omap_uart_prepare_idle(2);
>> omap2_gpio_prepare_for_idle(per_next_state);
>> - if (per_next_state == PWRDM_POWER_OFF) {
>> - if (core_next_state == PWRDM_POWER_ON) {
>> - per_next_state = PWRDM_POWER_RET;
>> - pwrdm_set_next_pwrst(per_pwrdm, per_next_state);
>> - per_state_modified = 1;
>> - } else
>> + if (per_next_state == PWRDM_POWER_OFF)
>> omap3_per_save_context();
>> - }
>> }
>>
>> - if (pwrdm_read_pwrst(cam_pwrdm) == PWRDM_POWER_ON)
>> - omap2_clkdm_deny_idle(mpu_pwrdm->pwrdm_clkdms[0]);
>> -
>> /* CORE */
>> if (core_next_state < PWRDM_POWER_ON) {
>> omap_uart_prepare_idle(0);
>> @@ -470,8 +460,6 @@ void omap_sram_idle(void)
>> if (per_prev_state == PWRDM_POWER_OFF)
>> omap3_per_restore_context();
>> omap_uart_resume_idle(2);
>> - if (per_state_modified)
>> - pwrdm_set_next_pwrst(per_pwrdm, PWRDM_POWER_OFF);
>> }
>>
>> /* Disable IO-PAD and IO-CHAIN wakeup */
>> --
>> 1.7.2.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
>
> - Paul
next prev parent reply other threads:[~2010-09-21 17:00 UTC|newest]
Thread overview: 32+ 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 ` 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 ` Kevin Hilman
2010-09-15 23:55 ` [PATCH 02/10] OMAP: PM debugfs removing OMAP3 hardcodings Kevin Hilman
2010-09-15 23:55 ` 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-15 23:55 ` Kevin Hilman
2010-09-21 7:52 ` Paul Walmsley
2010-09-21 7:52 ` Paul Walmsley
2010-09-21 17:00 ` Kevin Hilman [this message]
2010-09-21 17:00 ` Kevin Hilman
2010-09-23 23:25 ` Paul Walmsley
2010-09-23 23:25 ` Paul Walmsley
2010-09-23 23:47 ` Kevin Hilman
2010-09-23 23:47 ` Kevin Hilman
2010-09-24 0:16 ` Kevin Hilman
2010-09-24 0:16 ` Kevin Hilman
2010-09-15 23:55 ` [PATCH 04/10] OMAP4: pm.c extensions for OMAP4 support Kevin Hilman
2010-09-15 23:55 ` 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 ` 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 ` 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 ` 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 ` 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 ` 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
2010-09-15 23:55 ` 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=878w2vkp65.fsf@deeprootsystems.com \
--to=khilman@deeprootsystems.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-omap@vger.kernel.org \
--cc=paul@pwsan.com \
--cc=tero.kristo@nokia.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.