> -----Original Message----- > From: Nishanth Menon [mailto:nm@xxxxxx] > 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:nm@xxxxxx] > >> 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
