> -----Original Message-----
> From: Nishanth Menon [mailto:[email protected]]
> Sent: Monday, December 13, 2010 8:23 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:48 AM, the
> following:
> >> -----Original Message-----
> >> From: Nishanth Menon [mailto:[email protected]]
> >> Sent: Monday, December 13, 2010 8:14 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
> >>
> >>> -----Original Message-----
> >>> 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.
> >> I take that back, we could do something like the following instead:
> >> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-
> >> omap2/pm34xx.c
> >> index ba3c0d6..74842f1 100644
> >> --- a/arch/arm/mach-omap2/pm34xx.c
> >> +++ b/arch/arm/mach-omap2/pm34xx.c
> >> @@ -933,7 +933,14 @@ void omap3_pm_off_mode_enable(int
> enable)
> >>
> >> 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) {
> >> + omap_set_pwrdm_state(pwrst->pwrdm,
> >> PWRDM_POWER_RET);
> > You need to update pwrst->next_state as well.
> > Otherwise suspend will still throw the same error. I just sent the
code
> in
> > other email.
> yep. except, I think we dont need to do string compare. the following
> looks any better?
>
> 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.
Vishwa
> + } else {
> + pwrst->next_state = state;
> + }
> + omap_set_pwrdm_state(pwrst->pwrdm, pwrst-
> >next_state);
> }
> }
>
> --
> Regards,
> Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
