[U-Boot] [PATCH 9/9] Add initial support for Freescale mx51evk board

[Wolfgang Denk]

Dear Stefano Babic,

In message <1263212760-27272-10-git-send-email-sbabic at denx.de> you wrote:
> The patch adds initial support for the Freescale mx51evk board.
> Network (FEC) and SD controller (fsl_esdhc) are supported.

> --- a/Makefile
> +++ b/Makefile
> @@ -324,6 +324,10 @@ $(obj)u-boot.img:	$(obj)u-boot.bin
>  			sed -e 's/"[	 ]*$$/ for $(BOARD) board"/') \
>  		-d $< $@
>  
> +$(obj)u-boot.imx:       $(obj)u-boot.bin
> +		$(obj)tools/mkimage -n $(IMX_CONFIG) -T imximage \
> +		-e $(TEXT_BASE) -d $< $@

This actually belongs into the patch that adds the imx image format
support.


> +int dram_init(void)
> +{
> +	gd->bd->bi_dram[0].start = PHYS_SDRAM_1;
> +	gd->bd->bi_dram[0].size = get_ram_size((long *)PHYS_SDRAM_1, PHYS_SDRAM_1_SIZE);

Line too long. Please check globally.

> +	return 0;
> +}
> +
> +static void setup_uart(void)
> +{
> +	unsigned int pad = PAD_CTL_HYS_ENABLE | PAD_CTL_PKE_ENABLE |
> +			 PAD_CTL_PUE_PULL | PAD_CTL_DRV_HIGH;

Indentation by TAB, please. Please check globally.

> +static void setup_expio(void)
> +{
> +	u32 reg;
> +
> +	/* CS5 setup */
> +	mxc_request_iomux(MX51_PIN_EIM_CS5, IOMUX_CONFIG_ALT0);
> +	writel(0x00410089, WEIM_BASE_ADDR + 0x78 + CSGCR1);
> +	writel(0x00000002, WEIM_BASE_ADDR + 0x78 + CSGCR2);
> +	/* RWSC=50, RADVA=2, RADVN=6, OEA=0, OEN=0, RCSA=0, RCSN=0 */
> +	writel(0x32260000, WEIM_BASE_ADDR + 0x78 + CSRCR1);
> +	/* APR = 0 */
> +	writel(0x00000000, WEIM_BASE_ADDR + 0x78 + CSRCR2);


What is this magic offset 0x78 here?  Please get rid of using base
address + offset notation, use C struncts instead (see previous review
comments about this). Please fix globally.

> +	/* Reset the XUART and Ethernet controllers */
> +	reg = readw(&(mx51_io_board->sw_reset));
> +	reg |= 0x9;
> +	writew(reg, &(mx51_io_board->sw_reset));
> +	reg &= ~0x9;
> +	writew(reg, &(mx51_io_board->sw_reset));
> +}
> +
> +static void setup_fec(void)
> +{

FEC should only be set up (and eventually only be reset, too), if
there is any network support at all on this board.

...
> +int board_mmc_init(bd_t *bis)
> +{
> +	u32 interface_esdhc = 0;
> +	s32 status = 0;
> +	u32 esdhc_base_pointer;
> +
> +	for (interface_esdhc = 0; interface_esdhc < CONFIG_SYS_FSL_ESDHC_NUM; interface_esdhc++) {
> +		switch (interface_esdhc) {
> +			case 0:

Incorrect indent. And line too loing above, of course.

...
> +				break;
> +			case 1:
...
> +				break;
> +			}

Incorrect indent. Default case?

> diff --git a/board/freescale/mx51evk/mx51evk.h b/board/freescale/mx51evk/mx51evk.h
> new file mode 100644
> index 0000000..fec886d
> --- /dev/null
> +++ b/board/freescale/mx51evk/mx51evk.h
> @@ -0,0 +1,49 @@
> +/*
> + * Copyright 2009 Freescale Semiconductor, Inc. All Rights Reserved.
> + */

Please get rid off the "All Rights Reserved."

> +/*
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:

As mentioned before, this clause is not acceptable.

> diff --git a/include/configs/mx51evk.h b/include/configs/mx51evk.h
> new file mode 100644
> index 0000000..c8b2970
> --- /dev/null
> +++ b/include/configs/mx51evk.h
...
> + /* High Level Configuration Options */
> +#define CONFIG_SYS_APCS_GNU

What's this? It seems to be not used anywhere, nor documented?

> +#define CONFIG_L2_OFF

Is this a "High Level Configuration Option"? Move down!

> +#define CONFIG_SYS_MEMTEST_START       CSD0_BASE_ADDR
> +#define CONFIG_SYS_MEMTEST_END         0x10000
> +
> +#define CONFIG_SYS_LOAD_ADDR		CONFIG_LOADADDR

Here and everywhere else: please use TABs for vertical alignment.

> +#define CONFIG_ENV_SECT_SIZE    (128 * 1024)
> +#define CONFIG_ENV_SIZE         CONFIG_ENV_SECT_SIZE
> +#define CONFIG_ENV_IS_NOWHERE

Seems strange to me to define an environment sector size and an
environment size and then to say there is no environment at all?


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
Nothing ever becomes real until it is experienced.       - John Keats