> -----Original Message----- > From: Jean Pihet [mailto:jean.pihet@xxxxxxxxxxxxxx] > Sent: Monday, February 21, 2011 3:49 PM > To: Santosh Shilimkar > Cc: linux-omap@xxxxxxxxxxxxxxx; khilman@xxxxxx; linux-arm- > kernel@xxxxxxxxxxxxxxxxxxx; Rajendra Nayak > Subject: Re: [PATCH 14/17] omap4: cpuidle: Add MPUSS RET OFF states > > Hi Santosh, > [...] > > > + * cpuidle CORE retention support. > > + * Currently only MPUSS latency numbers are added based on > > + * measurements done internally. The numbers for MPUSS are > > + * not board dependent and hence set directly here instead of > > + * passing it from board files. > > + */ > > static struct cpuidle_params cpuidle_params_table[] = { > > - /* C1 */ > > - {1, 2, 2, 5}, > > + /* C1 - CPU0 WFI + CPU1 ON/OFF + MPU ON + CORE ON */ > > + {1, 2, 2, 5}, > > + /* C2 - CPU0 ON + CPU1 OFF + MPU ON + CORE ON */ > > + {1, 140, 160, 300}, > > + /* C3 - CPU0 OFF + CPU1 OFF + MPU CSWR + CORE ON */ > > + {1, 200, 300, 700}, > > + /* C4 - CPU0 OFF + CPU1 OFF + MPU OFF + CORE ON */ > > + {1, 1400, 600, 5000}, > > }; > > > > +DEFINE_PER_CPU(struct cpuidle_device, omap4_idle_dev); > > + > > +static int omap4_idle_bm_check(void) > > +{ > > + /* FIXME: Populate this with CORE retention support */ > > + return 0; > > +} > > + > > /** > > * omap4_enter_idle - Programs OMAP4 to enter the specified state > > * @dev: cpuidle device > > @@ -57,7 +92,9 @@ static struct cpuidle_params > cpuidle_params_table[] = { > > static int omap4_enter_idle(struct cpuidle_device *dev, > > struct cpuidle_state *state) > > { > > + struct omap4_processor_cx *cx = > cpuidle_get_statedata(state); > > struct timespec ts_preidle, ts_postidle, ts_idle; > > + u32 cpu1_state; > > > > /* Used to keep track of the total time in idle */ > > getnstimeofday(&ts_preidle); > > @@ -65,28 +102,74 @@ static int omap4_enter_idle(struct > cpuidle_device *dev, > > local_irq_disable(); > > local_fiq_disable(); > > > > - cpu_do_idle(); > > I think the piece of code below is rather difficult to read and > understand. Based on the patch description and the comment here > below > I do not see the relation with the code. > There is relation. The comments are as per the conditions. And The hardware constraint is back-ground behind the conditions. > " On OMAP4 because of hardware constraints, no low power states are > targeted when both CPUs are online and in SMP mode. The low power > states are attempted only when secondary CPU gets offline to OFF > through hotplug infrastructure. " > The test below does not seem to match this comment. > > > + /* > > + * Special hardware/software considerations: > > + * 1. Do only WFI for secondary CPU(non-boot - CPU1). > > + * Secondary cores are taken down only via hotplug > path. > The comment looks contradictory. Which one is taken OFF using this > code, which one from hotplug? > Does this correspond to the condition '(dev->cpu)' in the test > below? Yes. > > > + * 2. Do only a WFI as long as in SMP mode. > Does this correspond to the condition '(num_online_cpus() > 1)' in > the > test below? If so it this one triggering the low power mode for > cpu0? yes > > > + * 3. Continue to do only WFI till CPU1 hits OFF state. > > + * This is necessary to honour hardware recommondation > > + * of triggeing all the possible low power modes once > CPU1 is > > + * out of coherency and in OFF mode. > Does this correspond to the condition '(cpu1_state != > PWRDM_POWER_OFF)' in the test below? > Yes > > + * Update dev->last_state so that governor stats reflects > right > > + * data. > > + */ > > + cpu1_state = pwrdm_read_pwrst(cpu1_pd); > > + if ((dev->cpu) || (num_online_cpus() > 1) || > > + (cpu1_state != PWRDM_POWER_OFF)) { > Are '||' correct here? Yes. > Sorry if the code behaves correctly, the remarks are about > readability especially in the comments. > You got all three correct. Instead of having three If's doing same thing I have merged them and added comments above. And you got all of them correctly. Regards, Santosh -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
