[U-Boot] [PATCH 2/2] arm: cortexa9: adding support for TI OMAP4430 SDP

[Wolfgang Denk]

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.