From mboxrd@z Thu Jan 1 00:00:00 1970 From: mjmarx Date: Wed, 18 Feb 2009 08:23:22 -0800 (PST) Subject: [U-Boot] [PATCH] ADS5121 Add mem config for current rev4 boards In-Reply-To: <1234908276-26381-1-git-send-email-jrigby@freescale.com> References: <1234908276-26381-1-git-send-email-jrigby@freescale.com> Message-ID: <22082366.post@talk.nabble.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de > -----Original Message----- > From: Wolfgang Denk [mailto:wd at denx.de] > Sent: Wednesday, February 18, 2009 10:00 AM > To: mjmarx > Cc: u-boot at lists.denx.de > Subject: Re: [U-Boot] [PATCH] ADS5121 Add mem config for current rev4 > boards > > 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. > As far as my name change -- I DID NOT DO A WHOLESALE RENAME OF THE MEMORY #defs The init settings that Elpida and Micron share went from _MICRON_ to _DDR_ The setting they don't share stayed as _MICRON_ and now has a counterpart _ELPIDA_ This name change should not be combined with any other name change and belongs in the memory change patch. In fact it will be especially required if I keep one binary. Code section with elipses +#if defined(CONFIG_ADS5121_REV2) || \ + defined(CONFIG_ADS5121_REV3) || \ + defined(CONFIG_ADS5121_REV4M) + /* Micron init sequence */ im->mddrc.ddr_command = CONFIG_SYS_MICRON_INIT_DEV_OP; ... + im->mddrc.ddr_command = CONFIG_SYS_DDR_NOP; + im->mddrc.ddr_command = CONFIG_SYS_DDR_EM2; + im->mddrc.ddr_command = CONFIG_SYS_DDR_NOP; ... +#else + /* Elpida init sequence */ + im->mddrc.ddr_command = CONFIG_SYS_DDR_EM2; ... + im->mddrc.ddr_command = CONFIG_SYS_ELPIDA_INIT_DEV_OP; + udelay(200); +#endif > > 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. I will very much try my darn'dest!!!!! -Martha > > 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 -- View this message in context: http://www.nabble.com/-U-Boot---PATCH--ADS5121-Add-mem-config-for-current-rev4-boards-tp22067475p22082366.html Sent from the Uboot - Users mailing list archive at Nabble.com.