From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marek.vasut@gmail.com>
Date: Wed, 19 Jan 2011 15:36:27 +0100
Subject: [U-Boot] [PATCH 3/3 v3] iMX5: EfikaMX: Preliminary board support
In-Reply-To: <20110119084941.CF9F52FC@gemini.denx.de>
References: <1295379040-11563-1-git-send-email-marek.vasut@gmail.com>
	<1295379040-11563-3-git-send-email-marek.vasut@gmail.com>
	<20110119084941.CF9F52FC@gemini.denx.de>
Message-ID: <201101191536.27666.marek.vasut@gmail.com>
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

On Wednesday 19 January 2011 09:49:41 Wolfgang Denk wrote:
> Dear Marek Vasut,
> 
> In message <1295379040-11563-3-git-send-email-marek.vasut@gmail.com> you 
wrote:
> > Supported:
> > MMC
> > IDE
> > PMIC
> > SPI flash
> > LEDs
> 
> ...
> 
> > +/***********************************************************************
> > ******* + * Compile-time error checking
> > +
> > ************************************************************************
> > ******/
> 
> Incorrect multiline comment style.
> 
> I really do not understand why you submit this again, after we
> discussed this in lenght both by e-main and on IRC.
> 
> Please fix it globally, the code will not go in as is.

Mistake, sorry ... I have it fixed in my tree, just checked.
> 
> > +/***********************************************************************
> > ******* + * Board identification
> > +
> > ************************************************************************
> > ******/ +static u32 board_rev;
> 
> Be careful!!  This way board_rev will be located in bss, and BSS data
> MUST NOT be accessed before relocation, but it appears you to this.
> 
> This must be fixed.

Dang, good catch, thanks.
> 
> > +int board_init(void)
> > +{
> > +	gd->bd->bi_arch_number = MACH_TYPE_MX51_LANGE51;
> > +	gd->bd->bi_boot_params = PHYS_SDRAM_1 + 0x100;
> 
> Is this the correct MACH_TYPE for the EfikaMX board?

Apparently it is, they renamed the thing. Lange is probably a codename.
> 
> > +/*
> > + * Bootloader Components Configuration
> > + */
> > +#define CONFIG_CMD_SPI
> > +#define CONFIG_CMD_SF
> > +#define CONFIG_CMD_MMC
> > +#define	CONFIG_CMD_FAT
> > +#define	CONFIG_CMD_IDE
> > +#undef	CONFIG_CMD_IMLS
> 
> PLease be consistent - either always use space after #define
> (recommended), or always use TAB, but do not mix both styles.
> 
> > +/*
> > + * ATAG setup
> > + */
> > +#define CONFIG_CMDLINE_TAG		1	/* enable passing of ATAGs */
> > +#define CONFIG_REVISION_TAG		1
> > +#define CONFIG_SETUP_MEMORY_TAGS	1
> > +#define CONFIG_INITRD_TAG		1
> 
> Please omit all these '1'.  Please fix globally...
> 
> > +#define CONFIG_HARD_SPI			1
> > +#define CONFIG_MXC_SPI			1
> 
> ... i.e. here and in similar places, too.

Fixed in v4, thanks for the review
> 
> 
> Best regards,
> 
> Wolfgang Denk