From: Peter Tyser <ptyser@xes-inc.com>
Date: Thu, 20 Aug 2009 12:02:16 -0500
Subject: [U-Boot] [PATCH v3] Adding support for DevKit8000
In-Reply-To: <1250786842-9134-1-git-send-email-frederik@kriewitz.eu>
References: <1250786842-9134-1-git-send-email-frederik@kriewitz.eu>
Message-ID: <1250787736.2789.23.camel@localhost.localdomain>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi Frederik,
I had some minor aesthetic nitpicks.  I'd change the title to "Add
support for the DevKit8000 board".

<snip>

> diff --git a/MAINTAINERS b/MAINTAINERS
> index 620604c..03b2d10 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -706,6 +706,10 @@ Alex Z
>  	lart		SA1100
>  	dnp1110		SA1110
>  
> +Frederik Kriewitz <frederik@kriewitz.eu>
> +
> +	devkit8000	ARM CORTEX-A8 (OMAP3530 SoC)
> +

You should maintain the alphabetical order of MAINTAINERS when adding
yourself.

>  -------------------------------------------------------------------------
>  
>  Unknown / orphaned boards:
> diff --git a/MAKEALL b/MAKEALL
> index edebaea..34235b7 100755
> --- a/MAKEALL
> +++ b/MAKEALL
> @@ -581,6 +581,7 @@ LIST_ARM_CORTEX_A8="		\
>  	omap3_pandora		\
>  	omap3_zoom1		\
>  	omap3_zoom2		\
> +	devkit8000	\
>  "

You should maintain the alphabetical order of LIST_ARM_CORTEX_A8.

<snip>

> +/*-----------------------------------------------------------------------
> + * Stack sizes
> + *
> + * The stack sizes are set up in start.S using the settings below
> + */

Other's might disagree, but I think the "----" in the comments above are
not necessary/non-standard.  I'd personally use:

/*
 * Stack sizes
 *
 * The stack sizes are set up in start.S using the settings below
 */

Or just:
/* The stack sizes are set up in start.S using the settings below */

> +#define CONFIG_STACKSIZE	SZ_128K	/* regular stack */
> +#ifdef CONFIG_USE_IRQ
> +#define CONFIG_STACKSIZE_IRQ	SZ_4K	/* IRQ stack */
> +#define CONFIG_STACKSIZE_FIQ	SZ_4K	/* FIQ stack */
> +#endif
> +
> +/*-----------------------------------------------------------------------
> + * Physical Memory Map
> + */
> +#define CONFIG_NR_DRAM_BANKS	2	/* CS1 may or may not be populated */
> +#define PHYS_SDRAM_1		OMAP34XX_SDRC_CS0
> +#define PHYS_SDRAM_1_SIZE	SZ_128M	/* at least 128 meg */
> +#define PHYS_SDRAM_2		OMAP34XX_SDRC_CS1
> +
> +/* SDRAM Bank Allocation method */
> +#define SDRC_R_B_C		1
> +
> +/*-----------------------------------------------------------------------
> + * FLASH and environment organization
> + */
> +
> +/* **** PISMO SUPPORT *** */

You should use a standard comment style for "PISMO SUPPORT", eg less *'s
and standard capitalization.

> +
> +/* Configure the PISMO */

Maybe get rid of the above comment too - its pretty clear that you're
configuring the PISMO based on the "PISMO SUPPORT" comment above and the
define name.

> +#define PISMO1_NAND_SIZE		GPMC_SIZE_128M

Best,
Peter