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

Wolfgang Denk
2010-01-17T13:05:23Z

Dear Stefano Babic,

In message <1263212760-27272-10-git-send-email-sbabic@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