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 david-b@pacbell.net
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 g.liakhovetski@gmx.de Nishanth Menon nm@ti.com
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 <config_cmd_default.h>
+#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