From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kevin Hilman Subject: Re: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2 Date: Wed, 15 Dec 2010 15:47:51 -0800 Message-ID: <87wrnaions.fsf@deeprootsystems.com> References: <1291395818-8639-1-git-send-email-nm@ti.com> <1291395818-8639-6-git-send-email-nm@ti.com> <2cdf7d3d033ee2c88b6f2d4cfa37d9db@mail.gmail.com> <4D0622FE.2070801@ti.com> <4D062806.6090201@ti.com> <4D062F81.407@ti.com> <96505dfaee73d6785e153ccf5c2856d3@mail.gmail.com> <6f507e24d6ae188e265913e85e815f82@mail.gmail.com> <4D063344.2010001@ti.com> <37de3932d97af8e1882a08dd37242a17@mail.gmail.com> <4D063599.9080905@ti.com> <87ei9l10mk.fsf@deeprootsystems.com> <4D0933C6.1060404@ti.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Received: from mail-gy0-f174.google.com ([209.85.160.174]:49200 "EHLO mail-gy0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755781Ab0LOXrz (ORCPT ); Wed, 15 Dec 2010 18:47:55 -0500 Received: by gyb11 with SMTP id 11so1378456gyb.19 for ; Wed, 15 Dec 2010 15:47:55 -0800 (PST) In-Reply-To: <4D0933C6.1060404@ti.com> (Nishanth Menon's message of "Wed, 15 Dec 2010 15:31:50 -0600") Sender: linux-omap-owner@vger.kernel.org List-Id: linux-omap@vger.kernel.org To: Nishanth Menon Cc: Vishwanath Sripathy , linux-omap , Eduardo Valentin , Tony Lindgren Nishanth Menon writes: > Kevin Hilman had written, on 12/13/2010 09:42 PM, the following: >> Nishanth Menon writes: >> >>> Vishwanath Sripathy had written, on 12/13/2010 08:58 AM, the following: >>> [...] >>>>> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach- >>>>> omap2/pm34xx.c >>>>> index ba3c0d6..da12a56 100644 >>>>> --- a/arch/arm/mach-omap2/pm34xx.c >>>>> +++ b/arch/arm/mach-omap2/pm34xx.c >>>>> @@ -932,8 +932,15 @@ void omap3_pm_off_mode_enable(int enable) >>>>> #endif >>>>> >>>>> list_for_each_entry(pwrst, &pwrst_list, node) { >>>>> - pwrst->next_state = state; >>>>> - omap_set_pwrdm_state(pwrst->pwrdm, state); >>>>> + if (IS_PM34XX_ERRATUM(SDRC_WAKEUP_ERRATUM_i583) >>>>> && >>>>> + pwrst->pwrdm == core_pwrdm) { >>>>> + pwrst->next_state = PWRDM_POWER_RET; >>>>> + pr_err("%s: cannot enable Core OFF due to >>>> i583\n", >>>>> + __func__); >>>> You probably need to throw up this warning only if state >>>> == PWRDM_POWER_OFF. Otherwise this code looks fine to me. >>> Thanks for the review. added it. will post a v4 later today if no one >>> cribs with this approach. I will retain the logic in sram_idle as well >>> as a backup measure. >> >> This logic doesn't belong in SRAM idle. To handle the idle case, you >> should also disable the 'valid' bit for any C-state that has CORE off (I >> think there's only one.) > Apologies, but I dont think I get your point. Do you intend to state > that we dynamically add the C7 state in cpuidle34xx.c if this > condition is met? Yes. More precisely, dynamically set the 'valid' bit of C7 based on this condition. > I agree that this additional check in sram_idle should be removed, but > as long as I handle it in omap3_pm_off_mode_enable where the next > states are configured, is'nt that enough or am I missing something? Setting the next states only sets the default states, but CPUidle changes them. Looking closer at omap3_pm_off_mode_enable() though, it already calls into CPUidle and disables the valid bit for any states that have *either* MPU or core off. You'll probably just need to extend this approach to disable only CORE off state(s). Kevin