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.
On the other hand, I'm missing explanations what SDP3430 might be?
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.
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.
diff --git a/MAKEALL b/MAKEALL index f0ed8ea..53620eb 100755 --- a/MAKEALL +++ b/MAKEALL @@ -588,6 +588,7 @@ LIST_ARM_CORTEX_A8=" \ omap3_pandora \ omap3_zoom1 \ omap3_zoom2 \
- omap3_sdp \
"
Ditto.
######################################################################### diff --git a/Makefile b/Makefile index 5a4a109..2626147 100644 --- a/Makefile +++ b/Makefile @@ -3172,6 +3172,9 @@ omap3_zoom1_config : unconfig omap3_zoom2_config : unconfig @$(MKCONFIG) $(@:_config=) arm arm_cortexa8 zoom2 logicpd omap3
+omap3_sdp_config : unconfig
- @$(MKCONFIG) $(@:_config=) arm arm_cortexa8 sdp3430 ti omap3
Here again. Please check all other similar places.
...
+static const u32 gpmc_sdp_nor[] = {
- 0x00001200, /*CONF1 */
- 0x001F1F00, /*CONF2 */
- 0x00080802, /*CONF3 */
- 0x1C091C09, /*CONF4 */
- 0x01131F1F, /*CONF5 */
- 0x1F0F03C2, /*CONF6 */
- /*CONF7- computed as params */
+}; +static const u32 gpmc_sdp_debug[] = {
- 0x00611200, /*CONF1 FPGA needs WAIT1 line-active Low to function now.. */
- 0x001F1F01, /*CONF2 */
- 0x00080803, /*CONF3 */
- 0x1D091D09, /*CONF4 */
- 0x041D1F1F, /*CONF5 */
- 0x1D0904C4, /*CONF6 */
- /*CONF7- computed as params */
+}; +static const u32 gpmc_sdp_onenand[] = {
- 0x00001200, /*CONF1 */
- 0x000F0F01, /*CONF2 */
- 0x00030301, /*CONF3 */
- 0x0F040F04, /*CONF4 */
- 0x010F1010, /*CONF5 */
- 0x1F060000, /*CONF6 */
- /*CONF7- computed as params */
+};
+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.
+/******************************************************************************
- Routine: board_init
- Description: Early hardware init.
- *****************************************************************************/
Incorrect multiline comment style. Please fix globally.
...
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.
What exacty is the purpose of the comment? It does not carry any information. Seems just a waste of line length to me?
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?
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.
...
+/*
- 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.
+#if 0 +#define CONSOLE_J9 /* else J8/UART1 (innermost) */ +#endif
Please delete - don't add dead code.
+/* 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.
+/* OMITTED: single 2 Gbit KFM2G16 OneNAND flash */
Only a single blank line in places like thsi, please.
+/* 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.
...
+#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?
+#undef CONFIG_SYS_CLKS_IN_HZ /* everything, incl board info, in Hz */
There is no need to undefine non-existent variables.
+#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...]
Best regards,
Wolfgang Denk