[U-Boot] [PATCH] ADS5121 Add mem config for current rev4 boards

[Wolfgang Denk <wd@denx.de>]

Dear Martha,

in message <22077983.post@talk.nabble.com> you wrote:
> 
> > > -	im->mddrc.ddr_command = CONFIG_SYS_MICRON_PCHG_ALL;
> > > -	im->mddrc.ddr_command = CONFIG_SYS_MICRON_NOP;
> > > -	im->mddrc.ddr_command = CONFIG_SYS_MICRON_RFSH;
> > > -	im->mddrc.ddr_command = CONFIG_SYS_MICRON_NOP;
> > > -	im->mddrc.ddr_command = CONFIG_SYS_MICRON_RFSH;
> > > -	im->mddrc.ddr_command = CONFIG_SYS_MICRON_NOP;
> > > +		im->mddrc.ddr_command = CONFIG_SYS_DDR_NOP;
> > > +
> > > +	im->mddrc.ddr_command = CONFIG_SYS_DDR_PCHG_ALL;
> > > +	im->mddrc.ddr_command = CONFIG_SYS_DDR_NOP;
> > > +	im->mddrc.ddr_command = CONFIG_SYS_DDR_RFSH;
> > > +	im->mddrc.ddr_command = CONFIG_SYS_DDR_NOP;
> > > +	im->mddrc.ddr_command = CONFIG_SYS_DDR_RFSH;
> > > +	im->mddrc.ddr_command = CONFIG_SYS_DDR_NOP;
> > 
> > While performing such a global rename, please let's get rid of the
> > CONFIG_SYS_ names. These are NOT options that can be changed by the
> > user in the board config file, so they should not look like such.
> 
> CONFIG_SYS_ was in there already for all the memory #defs
> (as of the global rename in October)

Yes, you are right. But as I mentioned - while performing such a
global rename, we should use the opportunity and do it right this
time.

> IF you look -- the only rename I did was to change _MICRON_ to _DDR_ 
> and it is very appropriate for it to be in this patch.

But if we change the names anyway, we could correct the other mistake
as well, right?

> > > +#elif defined(CONFIG_ADS5121_REV3) || \
> > > +      defined(CONFIG_ADS5121_REV4M)
> > >  #define CONFIG_SYS_MDDRC_SYS_CFG	 0xFA804A00
> > >  #define CONFIG_SYS_MDDRC_SYS_CFG_RUN	 0xEA804A00
> > >  #define CONFIG_SYS_MDDRC_TIME_CFG1	 0x68EC1168
> > >  #define CONFIG_SYS_MDDRC_TIME_CFG2	 0x34310864
> > > +#else
> > > +#define CONFIG_SYS_MDDRC_SYS_CFG	 0xFA802b40
> > > +#define CONFIG_SYS_MDDRC_SYS_CFG_RUN	 0xEA802b40
> > > +#define CONFIG_SYS_MDDRC_TIME_CFG1	 0x690e1189
> > > +#define CONFIG_SYS_MDDRC_TIME_CFG2	 0x35410864
> > >  #endif
> > 
> > If I understand correctly, these are the only  real  changes  to  the
> > memory  init sequence  right? This is virtually invisible in the huge
> > number of variable renames. You should split this into  two  separate
> > patches,  one  doing  the  renames  only, and one adding the real new
> > configuration.
>
> See comment above.

In any case: please split this into two patches: one doing the rename
only, and another one adding the new functionality.

> The memory change was made due to a supply problem .. all new
> boards will have the new memory but unfortunately there is no
> new rev (or CPLD reg setting) for these boards  (it is not an 
> unwillingness on my part)

I understand this. On the other hand, there might  be  a  new  supply
problem in N days/weeks/months from now, and we definitely don't want
to  have  an  ever  growing number of incompatible configurations and
binary images if it can be avoided (and I'm sure it can be avoided).

> I will attempt to create one binary using Micron settings for rev 4.0
> and below, Elpida settings for rev 4.1 and beyond and for the small
> number of 4.1 Micron boards out there I will attempt to have them run
> with Elpida settings (the memory will run slightly more slowly)  I would
> like to keep an #ifdef in there to force Micron settings, however.

Let's try and find a solution that works with only one binary image,
without additional target names and #ifdef's. 

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Engineering without management is art."               - Jeff Johnson