From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nishanth Menon Date: Thu, 24 Sep 2009 03:47:39 +0300 Subject: [U-Boot] [PATCH 6/6] ARM:OMAP3:SDP3430: initial support In-Reply-To: <20090923195131.A8617832E864@gemini.denx.de> References: <1253326918-1670-1-git-send-email-nm@ti.com> <1253326918-1670-2-git-send-email-nm@ti.com> <1253326918-1670-3-git-send-email-nm@ti.com> <1253326918-1670-4-git-send-email-nm@ti.com> <1253326918-1670-5-git-send-email-nm@ti.com> <1253326918-1670-6-git-send-email-nm@ti.com> <1253326918-1670-7-git-send-email-nm@ti.com> <20090923195131.A8617832E864@gemini.denx.de> Message-ID: <4ABAC1AB.1020505@gmail.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Wolfgang Denk said the following on 09/23/2009 10:51 PM: > Dear Nishanth Menon, > > In message <1253326918-1670-7-git-send-email-nm@ti.com> you wrote: > >> --===============1247028818== >> >> From: David Brownell >> >> Start of SDP3430 support in "mainline" >> u-boot mainline code >> >> Original Patch written by David Brownell >> > > Um... this seems redundant information to me (the "From:" line and > the Signed-off-by: line already say that David Brownell is the > author. > Acked as previously discussed. > On the other hand, I'm missing explanations what SDP3430 might be? > hmm.. my bad. will fix. I should really be adding a few more lines to README.omap3. I will do that along with this change. > > >> diff --git a/MAINTAINERS b/MAINTAINERS >> index e9db278..adc8a63 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -619,6 +619,7 @@ Guennadi Liakhovetski >> Nishanth Menon >> >> omap3_zoom1 ARM CORTEX-A8 (OMAP3xx SoC) >> + omap3_sdp ARM CORTEX-A8 (OMAP3xx SoC) >> > > Please keep lists sorted. > Ack > General remark: > > The board name is "SDP3430", right? The board directory name is > board/ti/sdp3430/, which is ok. But then the configuration name should > be "sdp3430", too. > Thanks. >> + >> +static const u32 gpmc_sdp_nand[] = { >> + 0x00000800, /*CONF1 */ >> + 0x00141400, /*CONF2 */ >> + 0x00141400, /*CONF3 */ >> + 0x0F010F01, /*CONF4 */ >> + 0x010C1414, /*CONF5 */ >> + 0x1F040A80, /*CONF6 */ >> + /*CONF7- computed as params */ >> +}; >> > > Please comment what all these magic numbers mean. > > these are configuration register values for GPMC (General Purpose Memory Controller) I should be indeed adding proper comments here. will do. >> +/****************************************************************************** >> + * Routine: board_init >> + * Description: Early hardware init. >> + *****************************************************************************/ >> > > Incorrect multiline comment style. Please fix globally. > Ack > ... > >> diff --git a/board/ti/sdp3430/sdp.h b/board/ti/sdp3430/sdp.h >> new file mode 100644 >> index 0000000..5ad2920 >> --- /dev/null >> +++ b/board/ti/sdp3430/sdp.h >> > ... > >> +#define MUX_SDP3430()\ >> + /*SDRC*/\ >> + MUX_VAL(CP(SDRC_D0), (IEN | PTD | DIS | M0)) /*SDRC_D0*/\ >> + MUX_VAL(CP(SDRC_D1), (IEN | PTD | DIS | M0)) /*SDRC_D1*/\ >> > ... > > Incorrect indentation. > (having spend almost an hr cleaning that piece of mux header up) groan.. any recommendations? is it because of the " "? > What exacty is the purpose of the comment? It does not carry any > information. Seems just a waste of line length to me? > you mean /* SDRC_D0 */? hmmm.. might actually make sense if I am not using default mux mode 0 to note why I am using a new value. > >> diff --git a/board/ti/sdp3430/u-boot.lds b/board/ti/sdp3430/u-boot.lds >> new file mode 100644 >> index 0000000..4ecc6dd >> --- /dev/null >> +++ b/board/ti/sdp3430/u-boot.lds >> > > Is it really necessary that this board uses a custom linke rscript? > Cannot we use a generic one for several boards? > Ack. > >> diff --git a/include/configs/omap3_sdp.h b/include/configs/omap3_sdp.h >> new file mode 100644 >> index 0000000..176617a >> --- /dev/null >> +++ b/include/configs/omap3_sdp.h >> > > This should be include/configs/sdp3430.h, accorsing to the board name. > Ack. I guess this is a hangover from the days where we wanted all omap3 boards to look similar. > ... > >> +/* >> + * Size of malloc() pool >> + */ >> +#define CONFIG_ENV_SIZE SZ_256K /* Total Size Environment */ >> > > Please do not use any of these SZ_ defines; they will be removed soon. > Ack.. > >> +#if 0 >> +#define CONSOLE_J9 /* else J8/UART1 (innermost) */ >> +#endif >> > > Please delete - don't add dead code. > CONSOLE_J9 is really an option, but I get your point here I should be using #undef even if i wanted to allow folks to tweak around.. #if 0s are *evil* > >> +/* timeout values are in ticks */ >> +#define CONFIG_SYS_FLASH_ERASE_TOUT (100 * CONFIG_SYS_HZ) >> +#define CONFIG_SYS_FLASH_WRITE_TOUT (100 * CONFIG_SYS_HZ) >> > > Strictly speaking the comment is wrong. The timeouts are in > milliseconds. > gotcha. Ack. > >> +/* OMITTED: single 2 Gbit KFM2G16 OneNAND flash */ >> + >> + >> > > Only a single blank line in places like thsi, please. > ok ok.. though I think you are nit picking ;).. > >> +/* commands to include */ >> +#include >> + >> +#define CONFIG_CMD_NET >> +#define CONFIG_CMD_DHCP /* DHCP Support */ >> +#define CONFIG_CMD_I2C /* I2C serial bus support */ >> +#define CONFIG_CMD_JFFS2 /* JFFS2 Support */ >> +#define CONFIG_CMD_MMC /* MMC support */ >> +#define CONFIG_CMD_FAT /* FAT support */ >> +#define CONFIG_CMD_EXT2 /* EXT2 Support */ >> > > We consider it good style to keep such lists sorted. > I suppose Alphabetical sort? > ... > >> +#define CONFIG_SYS_MEMTEST_START (OMAP34XX_SDRC_CS0) /* memtest */ >> + /* works on */ >> +#define CONFIG_SYS_MEMTEST_END (OMAP34XX_SDRC_CS0 + \ >> + 0x01F00000) /* 31MB */ >> > > Has this been tested? Can you really overwrite low memory? No > exception vectors needed there? > yep it does boot :D.. exception vectors are stored in SRAM(which is a different address range).. but you have a point here -> will my sdram test actually overwrite my u-boot itself - heh heh, wont be much use then .. will check and fix. thanks > >> +#undef CONFIG_SYS_CLKS_IN_HZ /* everything, incl board info, in Hz */ >> > > There is no need to undefine non-existent variables. > yep, will check > >> +#define CONFIG_SYS_LOAD_ADDR (OMAP34XX_SDRC_CS0) /* default */ >> + /* load address */ >> > > Does this really work? Is low memory unused on this CPU? [Dorry for > asking stupid questions, just want to be sure...] > > yes, it does -> see previous comment. Regards, Nishanth Menon