RE: [PATCH 00/11] OMAP3 CPUidle patches

["Rajendra Nayak" <rnayak@ti.com>]

 

> -----Original Message-----
> From: "Högander" Jouni [mailto:jouni.hogander@nokia.com] 
> Sent: Thursday, July 03, 2008 11:28 AM
> To: ext Rajendra Nayak
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [PATCH 00/11] OMAP3 CPUidle patches
> 
> "ext Rajendra Nayak" <rnayak@ti.com> writes:
> 
> > Hi,
> >
> > The following patches define and enable all of the below 
> mentioned C states for OMAP3. 
> > These are tested with a minimal kernel config on 
> OMAP3430sdp as most drivers today don't have context 
> save/restore in place and
> > some even lack aggresive clock handling. 
> > These apply on top of the workaround patch set submitted by Jouni. 
> > ([PATCH 0/6] 34XX: PM: Workarounds to get omap3 to retention 4th.)
> >
> > The following is neccessay even with a minimal config to 
> achieve OFF.
> >  $ echo 1 > /sys/power/sleep_while_idle
> >  $ echo 1 > /sys/power/clocks_off_while_idle
> >   
> > The following C states are defined
> > C0 - System executing code
> > C1 - MPU WFI + Core active
> > C2 - MPU CSWR + Core active
> > C3 - MPU OFF + Core active
> > C4 - MPU CSWR + Core CSWR
> > C5 - MPU OFF + Core CSWR
> > C6 - MPU OFF + Core OFF
> >  
> > regards,
> > Rajendra
> 
> One more general comment on these patches as I looked at the code
> after applying them.
> 
> Most of the code in omap3_enter_idle in cpuidle34xx.c could be shared
> between suspend/pm_idle/cpuidle. I would do these changes to share
> this code between these three things:
> 
> 1. read mpu_pd pwrst, neon_pd pwrst, core_pd pwrst values in
>    omap_sram_idle in pm34xx.c
> 
> 2. Move all the logic from omap3_enter_idle to omap_sram_idle. Use
>    values read in previous step to decide wether ctx savings/restores
>    are needed, what should be written to neon_pd next_pwrst, wether
>    omap2_gpio_prepare_for/resume_after_retention is needed, wether to
>    disable/enable serial and gpio clocks.
> 
> Basically what is left into omap3_enter_idle is writing next pwrsts
> and cpuidle related stuff, something like this:
> 
> static int omap3_enter_idle(struct cpuidle_device *dev,
> 			struct cpuidle_state *state)
> {
> 	struct omap3_processor_cx *cx = cpuidle_get_statedata(state);
> 	struct timespec ts_preidle, ts_postidle, ts_idle;
> 	struct powerdomain *mpu_pd, *core_pd;
> 
> 	current_cx_state = *cx;
> 
> 	if (cx->type == OMAP3_STATE_C0) {
> 		/* Do nothing for C0, not even a wfi */
> 		return 0;
> 	}
> 
> 	local_irq_disable();
> 	local_fiq_disable();
> 
> 	/* Used to keep track of the total time in idle */
> 	getnstimeofday(&ts_preidle);
> 
> 	if (cx->type > OMAP3_STATE_C1)
> 		sched_clock_idle_sleep_event(); /* about to 
> enter deep idle */
> 
> 	mpu_pd = pwrdm_lookup("mpu_pwrdm");
> 	core_pd = pwrdm_lookup("core_pwrdm");
> 
>         pwrdm_set_next_pwrst(mpu_pd, cx->mpu_state);
> 	pwrdm_set_next_pwrst(core_pd, cx->core_state);
> 
> 	if (omap_irq_pending())
> 		goto return_sleep_time;
> 
> 	/* Execute ARM wfi */
> 	omap_sram_idle();
> 
> return_sleep_time:
> 	getnstimeofday(&ts_postidle);
> 	ts_idle = timespec_sub(ts_postidle, ts_preidle);
> 
> 	if (cx->type > OMAP3_STATE_C1)
> 		sched_clock_idle_wakeup_event(timespec_to_ns(&ts_idle));
> 
> 	local_irq_enable();
> 	local_fiq_disable();
> 
> 	return (u32)timespec_to_ns(&ts_idle)/1000;
> }
> 
> If this is not done, we need to copy paste code from omap3_enter_idle
> into suspend code anyway if we want to use offmode in it. It would be
> more nice to have one funcion with this logic shared between
> suspend/pm_idle/cpuidle rather than two or even three copies of it.

Yes, all this looks good to me.

> 
> -- 
> Jouni Högander
> 
> 

--
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