[U-Boot] [PATCH 2/2] arm: cortexa9: adding support for TI OMAP4430 SDP
V, Aneesh
aneesh at ti.com
Mon Jun 7 16:44:53 CEST 2010
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 at denx.de]
> Sent: Sunday, June 06, 2010 1:25 AM
> To: V, Aneesh
> Cc: u-boot at lists.denx.de; olbpdev at 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 at 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 at 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*//* Using
> fastboot 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 at denx.de
> Real computer scientists don't comment their code. The identifiers
> are so long they can't afford the disk space.
More information about the U-Boot
mailing list