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

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

> -----Original Message-----
> From: Nishanth Menon [mailto:nm@ti.com]
> 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@ti.com]
> >> 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