Hello Wolfgang,
Thanks for your comments. We will fix the issues you have pointed out.
+#endif /* CONFIG_DISPLAY_BOARDINFO */
...
+#ifdef CONFIG_DISPLAY_CPUINFO
These #defines have never been documented. It seems they are being copied around a lot, but I'm not sure if anybody ever does NOT set these.
I think we should at least document these - or rather drop them everywhere. What do you think?
I'm ok with dropping it everywhere.
+{
- printf ("CPU : OMAP4430 ES1.0 \n");
puts()?
And: no spaces after function names please (fix globally!).
But how about this rule mentioned in your wiki:
"All contributions to U-Boot should conform to the Linux kernel coding style; see the file "Documentation/CodingStyle" and the script "scripts/Lindent" in your Linux kernel source directory. In sources originating from U-Boot a style corresponding to "Lindent -pcs" (adding spaces before parameters to function calls) is actually used."
http://www.denx.de/wiki/U-Boot/CodingStyle
In fact, I didn't add spaces initially. They got added when I ran "Lindent -pcs". Is this rule not applicable anymore? Or am I missing something?
Best regards, Aneesh
-----Original Message----- From: Wolfgang Denk [mailto:wd@denx.de] Sent: Sunday, June 06, 2010 1:25 AM To: V, Aneesh Cc: u-boot@lists.denx.de; olbpdev@list.ti.com - OMAP Linux Baseport Development Team (May contain non-TIers) Subject: Re: [U-Boot] [PATCH 2/2] arm: cortexa9: adding support for TI OMAP4430 SDP
Dear Aneesh V,
In message 1274769577-29021-3-git-send-email-aneesh@ti.com you wrote:
Adding support for OMAP4430 SDP board based on the TI OMAP4430 SOC.
TI OMAP4430 is an SOC based on ARM Cortex-A9 cpu. OMAP4430 SDP is a reference board based on OMAP4430.
This patch adds minimum support for booting the board. OMAP4430 SDP does not support XIP. U-boot is loaded to SDRAM with the
help of
another small bootloader running from SRAM. U-boot currently relies on
this
bootloader for clock, mux, and SDRAM initialization.
This will change when OMAP4430 Configuration Header(CH) is attached to u-boot. CH is a feature by which ROM code can be made to intialize
the
clocks and SDRAM and to copy u-boot directly into SDRAM from a non-XIP
device.
More support such as MMC, ethernet etc will be added in subsequent
patches.
Signed-off-by: Aneesh V aneesh@ti.com
In addition to John's comments:
Makefile | 8 +- arch/arm/cpu/armv7/omap4/Makefile | 50 +++++ arch/arm/cpu/armv7/omap4/cache.S | 128 ++++++++++++ arch/arm/cpu/armv7/omap4/lowlevel_init.S | 49 +++++ arch/arm/cpu/armv7/omap4/omap4.c | 97 +++++++++ arch/arm/cpu/armv7/omap4/reset.S | 36 ++++ arch/arm/cpu/armv7/omap4/sys_info.c | 58 ++++++ arch/arm/cpu/armv7/omap4/timer.c | 140 +++++++++++++ arch/arm/include/asm/arch-omap4/cpu.h | 89 +++++++++ arch/arm/include/asm/arch-omap4/omap4.h | 142 ++++++++++++++ arch/arm/include/asm/arch-omap4/sys_proto.h | 36 ++++ board/ti/sdp4430/Makefile | 49 +++++ board/ti/sdp4430/config.mk | 32 +++ board/ti/sdp4430/sdp.c | 56 ++++++ include/configs/omap4_sdp4430.h | 280
+++++++++++++++++++++++++++
15 files changed, 1249 insertions(+), 1 deletions(-) create mode 100644 arch/arm/cpu/armv7/omap4/Makefile create mode 100644 arch/arm/cpu/armv7/omap4/cache.S create mode 100644 arch/arm/cpu/armv7/omap4/lowlevel_init.S create mode 100644 arch/arm/cpu/armv7/omap4/omap4.c create mode 100644 arch/arm/cpu/armv7/omap4/reset.S create mode 100644 arch/arm/cpu/armv7/omap4/sys_info.c create mode 100644 arch/arm/cpu/armv7/omap4/timer.c create mode 100644 arch/arm/include/asm/arch-omap4/cpu.h create mode 100644 arch/arm/include/asm/arch-omap4/omap4.h create mode 100644 arch/arm/include/asm/arch-omap4/sys_proto.h create mode 100644 board/ti/sdp4430/Makefile create mode 100644 board/ti/sdp4430/config.mk create mode 100644 board/ti/sdp4430/sdp.c create mode 100644 include/configs/omap4_sdp4430.h
Entries to MAKEALL and MAINTAINERS files missing.
+int checkboard (void) +{
- printf ("%s\n", sysinfo.board_string);
- return 0;
+}
Consider using puts() when you don't need any output formatting.
+#endif /* CONFIG_DISPLAY_BOARDINFO */
...
+#ifdef CONFIG_DISPLAY_CPUINFO
These #defines have never been documented. It seems they are being copied around a lot, but I'm not sure if anybody ever does NOT set these.
I think we should at least document these - or rather drop them everywhere. What do you think?
+{
- printf ("CPU : OMAP4430 ES1.0 \n");
puts()?
And: no spaces after function names please (fix globally!).
...
+/*
- High Level Configuration Options
- */
+#define CONFIG_ARMCORTEXA9 1 /* This is an ARM V7 CPU core */ +#define CONFIG_OMAP 1 /* in a TI OMAP core */ +#define CONFIG_OMAP44XX 1 /* which is a 44XX */ +#define CONFIG_OMAP4430 1 /* which is in a 4430 */ +#define CONFIG_4430SDP 1 /* working with SDP */
/*#define CONFIG_FASTBOOT 1*//* Usingfastboot interface */
Line too long - please check (and fix) globally.
And please don't add dead code like this.
+#if 0 +/* Enabled commands */ +#define CONFIG_CMD_DHCP /* DHCP Support */ +#define CONFIG_CMD_EXT2 /* EXT2 Support */ +#define CONFIG_CMD_FAT /* FAT support */ +#define CONFIG_CMD_I2C /* I2C serial bus support */ +#define CONFIG_CMD_MMC /* MMC support */ +#endif
... or like this.
...
+/* SDRAM Test range - start at 16 meg boundary -ends at 1Meg -
- a basic sanity check ONLY
- IF you would like to increase coverage, increase the end address
- or run the test with custom options
- */
Incorrect multiline comment style - in addition, the comment ("start at 16 meg boundary -ends at 1Meg") makes no sense to me.
+#define CONFIG_SYS_MEMTEST_START 0x80000000 +#define CONFIG_SYS_MEMTEST_END (CONFIG_SYS_MEMTEST_START + (1
<< 20))
Why would you want to test only such a small fration of your system RAM?
+/*
- Although we don't have
- CFI FLASH driver setup
- NOR boot support - single 1 Gbit PF48F6000M0 Strataflash
- */
Can't parse this!
+#if 0
No dead code, please.
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@denx.de Real computer scientists don't comment their code. The identifiers are so long they can't afford the disk space.