SDRC: Remove SDRC_POWER register configuration from SDRC init.

Commit Message

Artem Bityutskiy June 17, 2009, 1:35 p.m. UTC

From: Samu Onkalo <[e-mail]>

Bootloader must configure proper settings for SDRC before starting
kernel from SDRAM. Furthermore, removed lines violated omap3430 and
omap2420 SDRC errata (see errata 1.150)

Signed-off-by: Samu Onkalo <[e-mail]>
---
 arch/arm/mach-omap2/sdrc.c |   10 ++--------
 1 files changed, 2 insertions(+), 8 deletions(-)

Comments

Paul Walmsley June 17, 2009, 4:25 p.m. UTC | #1

Hello Samu, Artem,

On Wed, 17 Jun 2009, Artem Bityutskiy wrote:

> From: Samu Onkalo <[e-mail]>
> 
> Bootloader must configure proper settings for SDRC before starting
> kernel from SDRAM. Furthermore, removed lines violated omap3430 and
> omap2420 SDRC errata (see errata 1.150)

The 2420 and 3430 errata data here seems to be old; neither one contains 
1.150.  While I wait for a new version to arrive, can you provide some 
more context on this errata?  Does it imply any restrictions on 
programming SDRC_POWER from SRAM, e.g., the CORE DVFS code?


- Paul

> 
> Signed-off-by: Samu Onkalo <[e-mail]>
> ---
>  arch/arm/mach-omap2/sdrc.c |   10 ++--------
>  1 files changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/sdrc.c b/arch/arm/mach-omap2/sdrc.c
> index 2045441..0874687 100644
> --- a/arch/arm/mach-omap2/sdrc.c
> +++ b/arch/arm/mach-omap2/sdrc.c
> @@ -86,8 +86,8 @@ void __init omap2_set_globals_sdrc(struct omap_globals *omap2_globals)
>   * @sp: pointer to a null-terminated list of struct omap_sdrc_params
>   *
>   * Turn on smart idle modes for SDRAM scheduler and controller.
> - * Program a known-good configuration for the SDRC to deal with buggy
> - * bootloaders.
> + * Bootloaders should make proper configuration for SDRC since kernel
> + * is running from SDRAM.
>   */
>  void __init omap2_sdrc_init(struct omap_sdrc_params *sp)
>  {
> @@ -104,10 +104,4 @@ void __init omap2_sdrc_init(struct omap_sdrc_params *sp)
>  	sdrc_write_reg(l, SDRC_SYSCONFIG);
>  
>  	sdrc_init_params = sp;
> -
> -	/* XXX Enable SRFRONIDLEREQ here also? */
> -	l = (1 << SDRC_POWER_EXTCLKDIS_SHIFT) |
> -		(1 << SDRC_POWER_PWDENA_SHIFT) |
> -		(1 << SDRC_POWER_PAGEPOLICY_SHIFT);
> -	sdrc_write_reg(l, SDRC_POWER);
>  }
> -- 
> 1.6.0.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


- Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[e-mail] June 18, 2009, 6:03 a.m. UTC | #2

Hi, 

Perhaps someone from TI could comment that. I'm not sure if I can share
errata information for public discussion.

Br,
Samu

>-----Original Message-----
>From: ext Paul Walmsley [mailto:[e-mail]] 
>Sent: 17 June, 2009 19:25
>To: Onkalo Samu.P (Nokia-D/Tampere); Bityutskiy Artem 
>(Nokia-D/Helsinki)
>Cc: [e-mail]; Tony Lindgren
>Subject: Re: [PATCH] SDRC: Remove SDRC_POWER register 
>configuration from SDRC init.
>
>Hello Samu, Artem,
>
>On Wed, 17 Jun 2009, Artem Bityutskiy wrote:
>
>> From: Samu Onkalo <[e-mail]>
>> 
>> Bootloader must configure proper settings for SDRC before starting 
>> kernel from SDRAM. Furthermore, removed lines violated omap3430 and 
>> omap2420 SDRC errata (see errata 1.150)
>
>The 2420 and 3430 errata data here seems to be old; neither 
>one contains 1.150.  While I wait for a new version to arrive, 
>can you provide some more context on this errata?  Does it 
>imply any restrictions on programming SDRC_POWER from SRAM, 
>e.g., the CORE DVFS code?
>
>
>- Paul
>
>> 
>> Signed-off-by: Samu Onkalo <[e-mail]>
>> ---
>>  arch/arm/mach-omap2/sdrc.c |   10 ++--------
>>  1 files changed, 2 insertions(+), 8 deletions(-)
>> 
>> diff --git a/arch/arm/mach-omap2/sdrc.c b/arch/arm/mach-omap2/sdrc.c 
>> index 2045441..0874687 100644
>> --- a/arch/arm/mach-omap2/sdrc.c
>> +++ b/arch/arm/mach-omap2/sdrc.c
>> @@ -86,8 +86,8 @@ void __init omap2_set_globals_sdrc(struct 
>omap_globals *omap2_globals)
>>   * @sp: pointer to a null-terminated list of struct omap_sdrc_params
>>   *
>>   * Turn on smart idle modes for SDRAM scheduler and controller.
>> - * Program a known-good configuration for the SDRC to deal 
>with buggy
>> - * bootloaders.
>> + * Bootloaders should make proper configuration for SDRC 
>since kernel
>> + * is running from SDRAM.
>>   */
>>  void __init omap2_sdrc_init(struct omap_sdrc_params *sp)  { @@ 
>> -104,10 +104,4 @@ void __init omap2_sdrc_init(struct 
>omap_sdrc_params *sp)
>>  	sdrc_write_reg(l, SDRC_SYSCONFIG);
>>  
>>  	sdrc_init_params = sp;
>> -
>> -	/* XXX Enable SRFRONIDLEREQ here also? */
>> -	l = (1 << SDRC_POWER_EXTCLKDIS_SHIFT) |
>> -		(1 << SDRC_POWER_PWDENA_SHIFT) |
>> -		(1 << SDRC_POWER_PAGEPOLICY_SHIFT);
>> -	sdrc_write_reg(l, SDRC_POWER);
>>  }
>> --
>> 1.6.0.6
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe 
>linux-omap" 
>> in the body of a message to [e-mail] More majordomo 
>> info at  http://vger.kernel.org/majordomo-info.html
>> 
>
>
>- Paul
>--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mike Chan June 18, 2009, 7 p.m. UTC | #3

On Wed, Jun 17, 2009 at 11:03 PM, <[e-mail]> wrote:
>
> Hi,
>
> Perhaps someone from TI could comment that. I'm not sure if I can share
> errata information for public discussion.
>
> Br,
> Samu
>
> >-----Original Message-----
> >From: ext Paul Walmsley [mailto:[e-mail]]
> >Sent: 17 June, 2009 19:25
> >To: Onkalo Samu.P (Nokia-D/Tampere); Bityutskiy Artem
> >(Nokia-D/Helsinki)
> >Cc: [e-mail]; Tony Lindgren
> >Subject: Re: [PATCH] SDRC: Remove SDRC_POWER register
> >configuration from SDRC init.
> >
> >Hello Samu, Artem,
> >
> >On Wed, 17 Jun 2009, Artem Bityutskiy wrote:
> >
> >> From: Samu Onkalo <[e-mail]>
> >>
> >> Bootloader must configure proper settings for SDRC before starting
> >> kernel from SDRAM. Furthermore, removed lines violated omap3430 and
> >> omap2420 SDRC errata (see errata 1.150)
> >

This seems like it will make the kernel even more dependent on the
bootloader. As an example, there's code such as the IVA2 reset which
attempts to put the hardware in a good state even if the bootloader
screws up. Unfortunately not everyone has access to their bootloader
code.

-- Mike

>
> >The 2420 and 3430 errata data here seems to be old; neither
> >one contains 1.150.  While I wait for a new version to arrive,
> >can you provide some more context on this errata?  Does it
> >imply any restrictions on programming SDRC_POWER from SRAM,
> >e.g., the CORE DVFS code?
> >
> >
> >- Paul
> >
> >>
> >> Signed-off-by: Samu Onkalo <[e-mail]>
> >> ---
> >>  arch/arm/mach-omap2/sdrc.c |   10 ++--------
> >>  1 files changed, 2 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/arch/arm/mach-omap2/sdrc.c b/arch/arm/mach-omap2/sdrc.c
> >> index 2045441..0874687 100644
> >> --- a/arch/arm/mach-omap2/sdrc.c
> >> +++ b/arch/arm/mach-omap2/sdrc.c
> >> @@ -86,8 +86,8 @@ void __init omap2_set_globals_sdrc(struct
> >omap_globals *omap2_globals)
> >>   * @sp: pointer to a null-terminated list of struct omap_sdrc_params
> >>   *
> >>   * Turn on smart idle modes for SDRAM scheduler and controller.
> >> - * Program a known-good configuration for the SDRC to deal
> >with buggy
> >> - * bootloaders.
> >> + * Bootloaders should make proper configuration for SDRC
> >since kernel
> >> + * is running from SDRAM.
> >>   */
> >>  void __init omap2_sdrc_init(struct omap_sdrc_params *sp)  { @@
> >> -104,10 +104,4 @@ void __init omap2_sdrc_init(struct
> >omap_sdrc_params *sp)
> >>      sdrc_write_reg(l, SDRC_SYSCONFIG);
> >>
> >>      sdrc_init_params = sp;
> >> -
> >> -    /* XXX Enable SRFRONIDLEREQ here also? */
> >> -    l = (1 << SDRC_POWER_EXTCLKDIS_SHIFT) |
> >> -            (1 << SDRC_POWER_PWDENA_SHIFT) |
> >> -            (1 << SDRC_POWER_PAGEPOLICY_SHIFT);
> >> -    sdrc_write_reg(l, SDRC_POWER);
> >>  }
> >> --
> >> 1.6.0.6
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe
> >linux-omap"
> >> in the body of a message to [e-mail] More majordomo
> >> info at  http://vger.kernel.org/majordomo-info.html
> >>
> >
> >
> >- Paul
> >--
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Woodruff, Richard June 18, 2009, 7:07 p.m. UTC | #4

> From: Mike Chan [mailto:[e-mail]]
> Sent: Thursday, June 18, 2009 2:01 PM

> > >> Bootloader must configure proper settings for SDRC before starting
> > >> kernel from SDRAM. Furthermore, removed lines violated omap3430 and
> > >> omap2420 SDRC errata (see errata 1.150)
> > >
>
> This seems like it will make the kernel even more dependent on the
> bootloader. As an example, there's code such as the IVA2 reset which
> attempts to put the hardware in a good state even if the bootloader
> screws up. Unfortunately not everyone has access to their bootloader
> code.

Yes, I agree with Mike here.  Not every one can touch the bootloader.  And many operations the boot loader doesn't have context to know what the best setting for an application will be.

The boot loader needs to setup a valid DDR config for sure.  However, its unlikely it will setup for 'optimal' power management.  Further, when there are run time bugs fixes really only the kernel can do this.

You will need to turn on and off autocount before OFF mode on ES3.1.  Not doing so can result in trouble as the mask ROM itself before restoring context to kernel makes a mistake.  There are some SDRC_POWER settings you have to have at run time.

The init code shouldn't through out ACTIMINGS for DDR unless there is some policy in place, but it has less authority around power mode settings.

Regards,
Richard W.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Paul Walmsley June 18, 2009, 9:29 p.m. UTC | #5

Hi, 

On Wed, 17 Jun 2009, Paul Walmsley wrote:

> On Wed, 17 Jun 2009, Artem Bityutskiy wrote:
> 
> > From: Samu Onkalo <[e-mail]>
> > 
> > Bootloader must configure proper settings for SDRC before starting
> > kernel from SDRAM. Furthermore, removed lines violated omap3430 and
> > omap2420 SDRC errata (see errata 1.150)
> 
> The 2420 and 3430 errata data here seems to be old; neither one contains 
> 1.150.  While I wait for a new version to arrive, can you provide some 
> more context on this errata?  Does it imply any restrictions on 
> programming SDRC_POWER from SRAM, e.g., the CORE DVFS code?

Okay, so erratum 1.150 basically is that when SDRC_POWER.PWDENA is set, 
the SDRC can power down the SDRAM before writes complete, and so PWDENA 
should never be used.  That's easy enough to deal with by removing the 
PWDENA bit set from the SDRC_POWER register write.  So I'll pass on this 
patch.

The patch description does raise another issue, which is that SDRC_POWER 
should probably be written from SRAM, per TRM section 11.2.4.4.9.2.  We 
could do this as part of omap3_sram_configure_core_dpll, which is is 
called at boot for boards that define their SDRAM memory parameters.  
Another option would be to add another SRAM function to do this.  I'm 
leaning towards the former.  Any other opinions on this?


- Paul

> 
> 
> - Paul
> 
> > 
> > Signed-off-by: Samu Onkalo <[e-mail]>
> > ---
> >  arch/arm/mach-omap2/sdrc.c |   10 ++--------
> >  1 files changed, 2 insertions(+), 8 deletions(-)
> > 
> > diff --git a/arch/arm/mach-omap2/sdrc.c b/arch/arm/mach-omap2/sdrc.c
> > index 2045441..0874687 100644
> > --- a/arch/arm/mach-omap2/sdrc.c
> > +++ b/arch/arm/mach-omap2/sdrc.c
> > @@ -86,8 +86,8 @@ void __init omap2_set_globals_sdrc(struct omap_globals *omap2_globals)
> >   * @sp: pointer to a null-terminated list of struct omap_sdrc_params
> >   *
> >   * Turn on smart idle modes for SDRAM scheduler and controller.
> > - * Program a known-good configuration for the SDRC to deal with buggy
> > - * bootloaders.
> > + * Bootloaders should make proper configuration for SDRC since kernel
> > + * is running from SDRAM.
> >   */
> >  void __init omap2_sdrc_init(struct omap_sdrc_params *sp)
> >  {
> > @@ -104,10 +104,4 @@ void __init omap2_sdrc_init(struct omap_sdrc_params *sp)
> >  	sdrc_write_reg(l, SDRC_SYSCONFIG);
> >  
> >  	sdrc_init_params = sp;
> > -
> > -	/* XXX Enable SRFRONIDLEREQ here also? */
> > -	l = (1 << SDRC_POWER_EXTCLKDIS_SHIFT) |
> > -		(1 << SDRC_POWER_PWDENA_SHIFT) |
> > -		(1 << SDRC_POWER_PAGEPOLICY_SHIFT);
> > -	sdrc_write_reg(l, SDRC_POWER);
> >  }
> > -- 
> > 1.6.0.6
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> > the body of a message to [e-mail]
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 
> 
> - Paul
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


- Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/arm/mach-omap2/sdrc.c b/arch/arm/mach-omap2/sdrc.c
index 2045441..0874687 100644
--- a/arch/arm/mach-omap2/sdrc.c
+++ b/arch/arm/mach-omap2/sdrc.c
@@ -86,8 +86,8 @@  void __init omap2_set_globals_sdrc(struct omap_globals *omap2_globals)
  * @sp: pointer to a null-terminated list of struct omap_sdrc_params
  *
  * Turn on smart idle modes for SDRAM scheduler and controller.
- * Program a known-good configuration for the SDRC to deal with buggy
- * bootloaders.
+ * Bootloaders should make proper configuration for SDRC since kernel
+ * is running from SDRAM.
  */
 void __init omap2_sdrc_init(struct omap_sdrc_params *sp)
 {
@@ -104,10 +104,4 @@  void __init omap2_sdrc_init(struct omap_sdrc_params *sp)
 	sdrc_write_reg(l, SDRC_SYSCONFIG);
 
 	sdrc_init_params = sp;
-
-	/* XXX Enable SRFRONIDLEREQ here also? */
-	l = (1 << SDRC_POWER_EXTCLKDIS_SHIFT) |
-		(1 << SDRC_POWER_PWDENA_SHIFT) |
-		(1 << SDRC_POWER_PAGEPOLICY_SHIFT);
-	sdrc_write_reg(l, SDRC_POWER);
 }