[U-Boot] [PATCHv2 4/4] armv7: Add support for ST-Ericsson U8500 href platform

[Wolfgang Denk]

Dear John Rigby,

In message <1301761196-26072-5-git-send-email-john.rigby at linaro.org> you wrote:
> Based on ST-Ericsson private git repo.
> Plus changes to use arm_pl180_mmci driver.
> 
> This board support requires the vexpress mmc driver patch set from
> Matt Waddel.

All these comments are not really useful as commit message.  YOu might
add these as comments.

For the commit message, I'd rather like to see a short description of
the board, and which board features are supported by this port.

...
> +extern int prcmu_i2c_read(u8 reg, u16 slave);
> +extern int prcmu_i2c_write(u8 reg, u16 slave, u8 reg_data);

Please move prototype declarations to approproate header file.

> +int board_mmc_init(bd_t *bd)
> +{
> +	if (u8500_mmci_board_init())
> +		return -ENODEV;
> +
> +	arm_pl180_mmci_init();

Error.  You ignore the return code from arm_pl180_mmci_init() here.
Please fix.

...
> +	/* check ARM clock source */
> +	reg = readl(PRCM_ARM_CHGCLKREQ_REG);
> +	printf("A9 running on ");
> +	if (reg & 1)
> +		printf("external clock");
> +	else
> +		printf("ARM PLL");
> +	printf("\n");

Something like

	printf("A9 running on %s\n",
		(reg & 1) ? "external clock" : "ARM PLL";

would probably result in smaller (and faster) code.

The same applies to other places, too.

...
> +#define CONFIG_SYS_MEMTEST_START	0x00000000
> +#define CONFIG_SYS_MEMTEST_END	0x1FFFFFFF

Has this actually been tested?


> +#define CONFIG_BOARD_EARLY_INIT_F	1
> +#define BOARD_LATE_INIT		1
...
> +#define CONFIG_MMC		1
> +#define CONFIG_GENERIC_MMC	1
> +#define CONFIG_DOS_PARTITION	1

Please omit the '1' in all defines that select features only.


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
"More software projects have gone awry for lack of calendar time than
for all other causes combined."
                         - Fred Brooks, Jr., _The Mythical Man Month_