From mboxrd@z Thu Jan 1 00:00:00 1970 From: Rajendra Nayak Subject: Re: [PATCH 3/3] ARM: OMAP3: PM: cpuidle: optimize the clkdm idle latency in C1 state Date: Wed, 20 Jun 2012 14:22:10 +0530 Message-ID: <4FE18F3A.8090103@ti.com> References: <1338563468-31403-1-git-send-email-j-pihet@ti.com> <1338563468-31403-4-git-send-email-j-pihet@ti.com> <4FE187A8.8060203@ti.com> <4FE18E02.6020008@ti.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from na3sys009aog133.obsmtp.com ([74.125.149.82]:46969 "EHLO na3sys009aog133.obsmtp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754523Ab2FTIwS (ORCPT ); Wed, 20 Jun 2012 04:52:18 -0400 Received: by obhx4 with SMTP id x4so82496obh.0 for ; Wed, 20 Jun 2012 01:52:16 -0700 (PDT) In-Reply-To: <4FE18E02.6020008@ti.com> Sender: linux-omap-owner@vger.kernel.org List-Id: linux-omap@vger.kernel.org To: Jean Pihet Cc: Kevin Hilman , Grazvydas Ignotas , linux-omap@vger.kernel.org, Jean Pihet Hi Jean, On Wednesday 20 June 2012 02:16 PM, Rajendra Nayak wrote: > On Wednesday 20 June 2012 02:01 PM, Jean Pihet wrote: >> Hi Rajendra, >> >> On Wed, Jun 20, 2012 at 10:19 AM, Rajendra Nayak wrote: >>> Hi Jean, >>> >>> >>> On Friday 01 June 2012 08:41 PM, Jean Pihet wrote: >>>> >>>> For a power domain to idle all the clock domains in it must idle. >>>> This patch implements an optimization of the cpuidle code by >>>> denying and later allowing only the first registered clock domain >>>> of a power domain, and so optimizes the latency of the low power code. >>> >>> >>> How much do we really save doing this? I understand what you are doing >>> by looking at the patch but the changelog seems very confusing. >> The gain is on the registers accesses and the internal PRCM state >> machine. >> If needed the changelog can be updated. > > Can you explain a bit more on which register accesses are you talking > about? and some more on the PRCM state machine. never mind, I looked at the patch again and then the cpuidle code and figured what you are doing. Makes sense to me now :-) regards, Rajendra > >> >>> What the patch does is get rid of a few function indirection >>> and assumes there is only *one* clkdm for mpu and core. Is that right? >> Not exactly. The patch does not assume that there is only one clkdm >> per power domain. It just allows or denies only one clkdm to idle, >> which has the same effect at the power domain level. > > Ok, so lets assume mpu on OMAP3 has 2 clkdms, and you allow only one > of them to idle. Will that have the same effect at the power domain > level? > The first line of the change log says "For a power domain to idle *all* > the clock domains in it must idle" and now you say allowing only *one* > clkdm to idle should have the same effect at the power domain level. > I am confused. > >> >> Thanks for reviewing, >> Jean >> >>> >>> regards, >>> Rajendra >>> >>> >>>> >>>> The functions _cpuidle_allow_idle and _cpuidle_deny_idle are >>>> not used anymore and so are removed. >>>> >>>> Signed-off-by: Jean Pihet >>>> --- >>>> arch/arm/mach-omap2/cpuidle34xx.c | 22 ++++------------------ >>>> 1 files changed, 4 insertions(+), 18 deletions(-) >>>> >>>> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c >>>> b/arch/arm/mach-omap2/cpuidle34xx.c >>>> index 2e2f1c6..e6ae3fe 100644 >>>> --- a/arch/arm/mach-omap2/cpuidle34xx.c >>>> +++ b/arch/arm/mach-omap2/cpuidle34xx.c >>>> @@ -77,20 +77,6 @@ static struct omap3_idle_statedata >>>> omap3_idle_data[] = >>>> { >>>> >>>> static struct powerdomain *mpu_pd, *core_pd, *per_pd, *cam_pd; >>>> >>>> -static int _cpuidle_allow_idle(struct powerdomain *pwrdm, >>>> - struct clockdomain *clkdm) >>>> -{ >>>> - clkdm_allow_idle(clkdm); >>>> - return 0; >>>> -} >>>> - >>>> -static int _cpuidle_deny_idle(struct powerdomain *pwrdm, >>>> - struct clockdomain *clkdm) >>>> -{ >>>> - clkdm_deny_idle(clkdm); >>>> - return 0; >>>> -} >>>> - >>>> static int __omap3_enter_idle(struct cpuidle_device *dev, >>>> struct cpuidle_driver *drv, >>>> int index) >>>> @@ -108,8 +94,8 @@ static int __omap3_enter_idle(struct cpuidle_device >>>> *dev, >>>> >>>> /* Deny idle for C1 */ >>>> if (index == 0) { >>>> - pwrdm_for_each_clkdm(mpu_pd, _cpuidle_deny_idle); >>>> - pwrdm_for_each_clkdm(core_pd, _cpuidle_deny_idle); >>>> + clkdm_deny_idle(mpu_pd->pwrdm_clkdms[0]); >>>> + clkdm_deny_idle(core_pd->pwrdm_clkdms[0]); >>>> } >>>> >>>> /* >>>> @@ -131,8 +117,8 @@ static int __omap3_enter_idle(struct cpuidle_device >>>> *dev, >>>> >>>> /* Re-allow idle for C1 */ >>>> if (index == 0) { >>>> - pwrdm_for_each_clkdm(mpu_pd, _cpuidle_allow_idle); >>>> - pwrdm_for_each_clkdm(core_pd, _cpuidle_allow_idle); >>>> + clkdm_allow_idle(mpu_pd->pwrdm_clkdms[0]); >>>> + clkdm_allow_idle(core_pd->pwrdm_clkdms[0]); >>>> } >>>> >>>> return_sleep_time: >>> >>> >