[U-Boot] [PATCH 6/6] ARM:OMAP3:SDP3430: initial support

[Peter Tyser]

Hi Nishanth,

On Fri, 2009-09-18 at 21:21 -0500, Nishanth Menon wrote:
> From: David Brownell <david-b at pacbell.net>
> 
> Start of SDP3430 support in "mainline"
> u-boot mainline code
> 
> Original Patch written by David Brownell

I don't think the above comments are necessary.  David will be credited
with authorship already, and the subject line and text below make it
clear what this patch does.
 
> Support default jumpering and:
>  - UART1/ttyS0 console(legacy sdp3430 u-boot)
>  - UART3/ttyS2 console (matching other boards,
> 		 and SDP HW docs)
>  - Ethernet
>  - mmc0
>  - NOR boot
> 
> TODO:
>  - mmc1
>  - NAND (boot or 128M storage)
>  - OneNAND (boot or 256M storage)
>  - Fix NOR env variable load
>  - Review SDRC timing configuration/DPLL
> 	configuration
>  - Dynamically read FPGA dip switch settings and
>  	map NOR/NAND/ONENAND devices to right
> 	chipselects
> 
> Currently the UART1 is enabled by default.  for
> compatibility with other OMAP3 u-boot platforms,
> enable the #define of CONSOLE_J9.
> 
> Ref: SDP3430:
> http://focus.ti.com/general/docs/wtbu/wtbugencontent.tsp?templateId=6123&navigationId=12013&contentId=28741
> 
> Signed-off-by: David Brownell <david-b at pacbell.net>
> Signed-off-by: Nishanth Menon <nm at ti.com>
> ---
>  MAINTAINERS                 |    1 +
>  MAKEALL                     |    1 +
>  Makefile                    |    3 +
>  board/ti/sdp3430/Makefile   |   49 ++++++
>  board/ti/sdp3430/config.mk  |   33 ++++
>  board/ti/sdp3430/sdp.c      |  194 ++++++++++++++++++++++
>  board/ti/sdp3430/sdp.h      |  376 +++++++++++++++++++++++++++++++++++++++++++
>  board/ti/sdp3430/u-boot.lds |   63 +++++++
>  include/configs/omap3_sdp.h |  367 +++++++++++++++++++++++++++++++++++++++++
>  9 files changed, 1087 insertions(+), 0 deletions(-)
>  create mode 100644 board/ti/sdp3430/Makefile
>  create mode 100644 board/ti/sdp3430/config.mk
>  create mode 100644 board/ti/sdp3430/sdp.c
>  create mode 100644 board/ti/sdp3430/sdp.h
>  create mode 100644 board/ti/sdp3430/u-boot.lds
>  create mode 100644 include/configs/omap3_sdp.h

The board config header file should be renamed to sdp.h from
omap3_sdp.h.  There was a recent thread discussing this convention "ARM
Pull Request" around Sept 6.

> diff --git a/MAINTAINERS b/MAINTAINERS
> index e9db278..adc8a63 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -619,6 +619,7 @@ Guennadi Liakhovetski <g.liakhovetski at gmx.de>
>  Nishanth Menon <nm at ti.com>
>  
>  	omap3_zoom1	ARM CORTEX-A8 (OMAP3xx SoC)
> +	omap3_sdp	ARM CORTEX-A8 (OMAP3xx SoC)

You may as well keep the boards ordered alphabetically (and remove the
omap_ prefix from sdp).

>  David Mller <d.mueller at elsoft.ch>
>  
> 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
> +

Ditto.

<snip>

> +/******************************************************************************
> + * Routine: board_init
> + * Description: Early hardware init.
> + *****************************************************************************/
> +int board_init(void)
> +{
> +	DECLARE_GLOBAL_DATA_PTR;
> +


I'd use the preferred multi-line comment style:
/*
 *
 */

There are lots of other non-preferred multi-line comments throughout the
patch as well.

I personally don't think its necessary to put "Routine: <name>" stuff
for each function either.  It doesn't add any benefit, adds cruft to
grep output, and might get out of sync with the real function name at
some point.  If it were me, I would get rid of "Description: " text too.
Its pretty obvious that the text "Early hardware init" is a description
of the function.

Best,
Peter