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 7:35 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 07:54 AM, the
> following:
> > Nishant,
> >
> >> -----Original Message-----
> >> From: Nishanth Menon [mailto:nm@ti.com]
> >> Sent: Monday, December 13, 2010 7:13 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 07:35 AM, the
> >> following:
> >>> Nishant,
> >>>
> >>>> -----Original Message-----
> >>>> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> >>>> owner@vger.kernel.org] On Behalf Of Nishanth Menon
> >>>> Sent: Friday, December 03, 2010 10:34 PM
> >>>> To: linux-omap
> >>>> Cc: Eduardo Valentin; Kevin Hilman; Tony Lindgren; Nishanth
> Menon
> >>>> Subject: [PATCH 5/5 v3] OMAP3630: PM: Erratum i583: disable
> >> coreoff if
> >>>> < ES1.2
> >>>>
> >>>> From: Eduardo Valentin <eduardo.valentin@nokia.com>
> >>>>
> >>>> Limitation i583: Self_Refresh Exit issue after OFF mode
> >>>>
> >>>> Issue:
> >>>> When device is waking up from OFF mode, then SDRC state
> machine
> >>>> sends
> >>>> inappropriate sequence violating JEDEC standards.
> >>>>
> >>>> Impact:
> >>>> OMAP3630 < ES1.2 is impacted as follows depending on the
> >> platform:
> >>>> CS0: for 38.4MHz as internal sysclk, DDR content seen to be
> stable,
> >>>> while
> >>>> 	for all other sysclk frequencies, varied levels of instability
> >>>> 	seen based on varied parameters.
> >>>> CS1: impacted
> >>>>
> >>>> This patch takes option #3 as recommended by the Silicon
> erratum:
> >>>> Avoid core power domain transitioning to OFF mode. Power
> >> consumption
> >>>> impact is expected in this case.
> >>>> To do this, we route OFF requests to RET request on the impacted
> >>>> revisions
> >>>> of silicon.
> >>>>
> >>>> Cc: Kevin Hilman <khilman@deeprootsystems.com>
> >>>> Cc: Tony Lindgren <tony@atomide.com>
> >>>>
> >>>> [nm@ti.com: rebased the code to 2.6.37-rc2- short circuit code
> >> changed
> >>>> a bit]
> >>>> Signed-off-by: Nishanth Menon <nm@ti.com>
> >>>> Signed-off-by: Eduardo Valentin <eduardo.valentin@nokia.com>
> >>>> ---
> >>>> v3: no functional change in erratum wa implementation, just
> >> registration
> >>>> of
> >>>>  	erratum is collated to a single cpu detection and version
> check
> >>>> v2: https://patchwork.kernel.org/patch/365262/
> >>>>     rebased to this patch series instead of depending on hs
> changes
> >>>>     fix typo for macro definition
> >>>> v1: http://marc.info/?l=linux-omap&m=129013173425266&w=2
> >>>>  arch/arm/mach-omap2/pm34xx.c |   14 ++++++++++++++
> >>>>  1 files changed, 14 insertions(+), 0 deletions(-)
> >>>>
> >>>> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-
> >>>> omap2/pm34xx.c
> >>>> index b49e02b..ba3c0d6 100644
> >>>> --- a/arch/arm/mach-omap2/pm34xx.c
> >>>> +++ b/arch/arm/mach-omap2/pm34xx.c
> >>>> @@ -56,6 +56,7 @@
> >>>>  #define OMAP343X_CONTROL_REG_VALUE_OFFSET  0xc8
> >>>>
> >>>>  #define RTA_ERRATUM_i608		(1 << 0)
> >>>> +#define SDRC_WAKEUP_ERRATUM_i583	(1 << 1)
> >>>>  static u16 pm34xx_errata;
> >>>>  #define IS_PM34XX_ERRATUM(id)		(pm34xx_errata & (id))
> >>>>
> >>>> @@ -406,6 +407,17 @@ void omap_sram_idle(void)
> >>>>  	}
> >>>>
> >>>>  	/* CORE */
> >>>> +	/*
> >>>> +	 * Erratum i583: implementation for ES rev < Es1.2 on
> 3630
> >>>> +	 * We cannot enable OFF mode in a stable form for
> previous
> >>>> +	 * revisions, transition instead to RET
> >>>> +	 */
> >>>> +	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.

Vishwa

>
> --
> Regards,
> Nishanth Menon