RE: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable coreoff if < ES1.2

[Vishwanath Sripathy <vishwanath.bs@ti.com>]

Nishant,

> -----Original Message-----
> From: Nishanth Menon [mailto:nm@ti.com]
> Sent: Monday, December 13, 2010 8:07 PM
> To: Vishwanath Sripathy
> Cc: linux-omap; Eduardo Valentin; Kevin Hilman; Tony Lindgren
> Subject: Re: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable
> coreoff if < ES1.2
>
> Vishwanath Sripathy had written, on 12/13/2010 08:25 AM, the
> following:
> [...]
> >>>>>> +	if (IS_PM34XX_ERRATUM(SDRC_WAKEUP_ERRATUM_i583)
> >> &&
> >>>>>> +			(core_next_state == PWRDM_POWER_OFF))
> >> {
> >>>>>> +		pwrdm_set_next_pwrst(core_pwrdm,
> >>>>>> PWRDM_POWER_RET);
> >>>>>> +		core_next_state = PWRDM_POWER_RET;
> >>>>>> +	}
> >>>>> Since next_state in pwrst_list (for core) is not updated, this is
> >>> throwing
> >>>>> up an error "Powerdomain (core_pwrdm) didn't enter target
> state
> >> 0"
> >>>> when
> >>>>> you off mode is enabled for ES1.1 or lesser (in suspend path).
> It's
> >>> not
> >>>>> really true that Core has not entered target state. It has entered
> >>>>> Retention state which is the actual target state set in
> >>> omap_sram_idle.
> >>>>> However it did not enter the state that was passed by
> >>>> omap3_pm_suspend. Is
> >>>>> this expected behavior?
> >>>> we could go both ways on this - this patch will(as you noticed)
> >> indicate
> >>>> that the transition failed on <ES1.2, or we could make it entirely
> >>>> transparent(by modifying the the pwrst_list - claim we achieved
> off,
> >>>> while not really hitting off - I personally dont think that is
> > correct.
> >>> The point I am making is that you cannot distinguish between
> genuine
> >> off
> >>> /retention failure since this message is thrown for both pass and
> > fail.
> >>> May be an additional trace message indicating that system entered
> >>> retention instead of off (for ES<1.2) will be useful.
> >> hmm... good point there.
> >> two issues here:
> >> a) omap3_pm_suspend should probably state which state was
> achieved
> >> as
> >> well in the error message (trivial fix).
> >> b) how do we notify users that it was due to
> >> SDRC_WAKEUP_ERRATUM_i583
> >> that core-off was denied. -> do this in omap3_pm_suspend(when
> user
> >> attempts actual OFF) OR omap3_pm_off_mode_enable(when user
> >> attempts to
> >> enable OFF mode)?
> >>
> >> Any suggestions to allow the same uImage boot on all silicon + allow
> >> cpu_idle + suspend paths not to spew pr_info messages(aka cant
> add
> >> prints in sram_idle)?
> > I vote for denying off mode for Core (for ES<1.2) in
> > omap3_pm_off_mode_enable and throw up a message saying that
> Core off is
> > not enabled. Then we will not get this failure message in suspend path
> > since pwrst_list will have the right state.
> Keep in mind - if we disable it in omap3_pm_off_mode_enable - we will
> deny OFF wholesale if I understand the logic right- not just core-off -
> I kind of think that is extreme.
No, I am not saying that deny idle for all power domains. Deny it only for
Core domain, something like this.
void omap3_pm_off_mode_enable(int enable)
{
	struct power_state *pwrst;
	u32 state;

	if (enable)
		state = PWRDM_POWER_OFF;
	else
		state = PWRDM_POWER_RET;

#ifdef CONFIG_CPU_IDLE
	omap3_cpuidle_update_states();
#endif

	list_for_each_entry(pwrst, &pwrst_list, node) {
		pwrst->next_state = state;
		if (strcmp("core_pwrdm", pwrst->pwrdm->name)==0) {
		if (IS_PM34XX_ERRATUM(SDRC_WAKEUP_ERRATUM_i583)
&& state ==PWRDM_POWER_OFF)
		pwrst->next_state = PWRDM_POWER_RET;
		}
		omap_set_pwrdm_state(pwrst->pwrdm, pwrst->next_state);
	}
}

Vishwa
>
> --
> Regards,
> Nishanth Menon