[U-Boot] [PATCH 5/5] MX5:MX53: add initial support for MX53EVK board

[Stefano Babic]

On 12/16/2010 11:17 AM, Jason Liu wrote:
> Add initial support for MX53EVK board support.
> FEC, SD/MMC, UART, I2C, have been support.
> 
> diff --git a/arch/arm/cpu/armv7/u-boot.lds b/arch/arm/cpu/armv7/u-boot.lds
> index 5725c30..7b6ab66 100644
> --- a/arch/arm/cpu/armv7/u-boot.lds
> +++ b/arch/arm/cpu/armv7/u-boot.lds
> @@ -34,6 +34,7 @@ SECTIONS
>  	. = ALIGN(4);
>  	.text	:
>  	{
> +		*(.ivt)
>  		arch/arm/cpu/armv7/start.o	(.text)
>  		*(.text)
>  	}

We have already discussed this point, see my previous answer here:

	http://marc.info/?l=u-boot&m=127793013695282&w=2

The solution in u-boot is *not* to link statically the IMX header to the
u-boot.bin, but to generate a u-boot.imx image with a configuration
file. This solution is already provided for the i.MX51 processor (same
family), and you should go on this way, modifying tools/imximage.c for
your needs, if required. This solution was previously discussed and
accepted on the ML and it is compatible with other processors from
different manufactures (kirchwood, see also u-boot.kwb).

As already answered by Albert and Wolfgang, the header must not be part
of u-boot.bin.

> +SOBJS	:= ivt.o

This should be removed, and a imximage.cfg file should be written.

> +plugin_start:
> +	/* Save the return address and the function arguments */
> +	push {r0-r3, lr}
> +
> +	ldr r0, =ROM_SI_REV
> +	ldr r1, [r0]
> +
> +	cmp r1, #0x20
> +
> +	/* IOMUX Setup */
> +	ldr r0, =0x53fa8500
> +	moveq r1, #0x00180000
> +	movne r1, #0x00380000
> +	mov r2, #0x00380000
> +	add r2, r2, #0x40
> +	add r3, r1, #0x40
> +	mov r4, #0x00200000
> +
> +	str r1, [r0, #0x54]
> +	str r2, [r0, #0x58]
> +	str r1, [r0, #0x60]
> +	str r3, [r0, #0x64]
> +	str r2, [r0, #0x68]
> +
> +	streq r1, [r0, #0x70]
> +	strne r4, [r0, #0x70]
> +	str r1, [r0, #0x74]
> +	streq r1, [r0, #0x78]
> +	strne r4, [r0, #0x78]
> +	str r2, [r0, #0x7c]
> +	str r3, [r0, #0x80]
> +	str r1, [r0, #0x84]
> +	str r1, [r0, #0x88]
> +	str r2, [r0, #0x90]
> +	str r1, [r0, #0x94]
> +
> +	ldr r0, =0x53fa86f0
> +	str r1, [r0, #0x0]
> +	mov r2, #0x00000200
> +	str r2, [r0, #0x4]
> +	mov r2, #0x00000000
> +	str r2, [r0, #0xc]
> +
> +	ldr r0, =0x53fa8700
> +	str r2, [r0, #0x14]
> +	str r1, [r0, #0x18]
> +	str r1, [r0, #0x1c]
> +	str r1, [r0, #0x20]
> +
> +	moveq r2, #0x02000000
> +	movne r2, #0x06000000
> +
> +	str r2, [r0, #0x24]
> +	str r1, [r0, #0x28]
> +	str r1, [r0, #0x2c]
> +
> +	/* Initialize DDR2 memory - Hynix H5PS2G83AFR */
> +	ldr r0, =ESDCTL_BASE_ADDR
> +
> +	ldreq r1, =0x31333530
> +	ldrne r1, =0x2b2f3031
> +	str r1, [r0, #0x088]
> +
> +	ldreq r1, =0x4a474a44
> +	ldrne r1, =0x40363333
> +	str r1, [r0, #0x090]
> +
> +	/* add 3 logic unit of delay to sdclk  */
> +	ldr r1, =0x00000f00
> +	str r1, [r0, #0x098]
> +
> +	ldr r1, =0x00000800
> +	str r1, [r0, #0x0F8]
> +
> +	ldreq r1, =0x02490241
> +	ldrne r1, =0x01310132
> +	str r1, [r0, #0x07c]
> +
> +	ldreq r1, =0x01710171
> +	ldrne r1, =0x0133014b
> +	str r1, [r0, #0x080]
> +
> +	/* Enable bank interleaving, RALAT = 0x4, DDR2_EN = 1 */
> +	ldr r1, =0x00001710
> +	str r1, [r0, #0x018]
> +
> +	ldr r1, =0xc4110000
> +	str r1, [r0, #0x00]
> +
> +	ldr r1, =0x4d5122d2
> +	str r1, [r0, #0x0C]
> +
> +	ldr r1, =0x92d18a22
> +	str r1, [r0, #0x10]
> +
> +	ldr r1, =0x00c70092
> +	str r1, [r0, #0x14]
> +
> +	ldr r1, =0x000026d2
> +	str r1, [r0, #0x2C]
> +
> +	ldr r1, =0x009f000e
> +	str r1, [r0, #0x30]
> +
> +	ldr r1, =0x12272000
> +	str r1, [r0, #0x08]
> +
> +	ldr r1, =0x00030012
> +	str r1, [r0, #0x04]
> +
> +	ldr r1, =0x04008010
> +	str r1, [r0, #0x1C]
> +
> +	ldr r1, =0x00008032
> +	str r1, [r0, #0x1C]
> +
> +	ldr r1, =0x00008033
> +	str r1, [r0, #0x1C]
> +
> +	ldr r1, =0x00008031
> +	str r1, [r0, #0x1C]
> +
> +	ldr r1, =0x0b5280b0
> +	str r1, [r0, #0x1C]
> +
> +	ldr r1, =0x04008010
> +	str r1, [r0, #0x1C]
> +
> +	ldr r1, =0x00008020
> +	str r1, [r0, #0x1C]
> +
> +	ldr r1, =0x00008020
> +	str r1, [r0, #0x1C]
> +
> +	ldr r1, =0x0a528030
> +	str r1, [r0, #0x1C]
> +
> +	ldr r1, =0x03c68031
> +	str r1, [r0, #0x1C]
> +
> +	ldr r1, =0x00468031
> +	str r1, [r0, #0x1C]
> +
> +	/* Even though Rev B does not have DDR on CSD1, keep these
> +	 * mode register initialization sequences for future uses since
> +	 * it does not hurt to keep them
> +	 */
> +	ldr r1, =0x04008018
> +	str r1, [r0, #0x1C]
> +
> +	ldr r1, =0x0000803a
> +	str r1, [r0, #0x1C]
> +
> +	ldr r1, =0x0000803b
> +	str r1, [r0, #0x1C]
> +
> +	ldr r1, =0x00008039
> +	str r1, [r0, #0x1C]
> +
> +	ldr r1, =0x0b528138
> +	str r1, [r0, #0x1C]
> +
> +	ldr r1, =0x04008018
> +	str r1, [r0, #0x1C]
> +
> +	ldr r1, =0x00008028
> +	str r1, [r0, #0x1C]
> +
> +	ldr r1, =0x00008028
> +	str r1, [r0, #0x1C]
> +
> +	ldr r1, =0x0a528038
> +	str r1, [r0, #0x1C]
> +
> +	ldr r1, =0x03c68039
> +	str r1, [r0, #0x1C]
> +
> +	ldr r1, =0x00468039
> +	str r1, [r0, #0x1C]
> +
> +	ldr r1, =0x00005800
> +	str r1, [r0, #0x20]
> +
> +	ldr r1, =0x00033337
> +	str r1, [r0, #0x58]
> +
> +	ldr r1, =0x00000000
> +	str r1, [r0, #0x1C]
> +

Why do we need to write these registers in assembly ? Why cannot we move
them into board_init or board_early_init_f ? And again, should these
values described better in imximage.cfg ?

> diff --git a/board/freescale/mx53evk/mx53evk.c b/board/freescale/mx53evk/mx53evk.c
> new file mode 100755
> index 0000000..ff6bfb2
> --- /dev/null
> +++ b/board/freescale/mx53evk/mx53evk.c
> @@ -0,0 +1,412 @@
> +/*
> + * Copyright (C) 2007, Guennadi Liakhovetski <lg at denx.de>
> + *
> + * (C) Copyright 2009-2010 Freescale Semiconductor, Inc.
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.

It seems to me that you derived this file from mx51evk.c, but you copied
the header with copyrights from another (MX31, maybe) file.

> +#ifdef CONFIG_I2C_MXC
> +static void setup_i2c(unsigned int module_base)

I think it is better to enumerate the i2c controller as done in manual
as with the physical address. Anyway, I do not see yet any released
manual for this processor on Freescale's site, so I cannot be more precise.

> +void power_init(void)
> +{
> +	unsigned char buf[4] = { 0 };
> +	struct mxc_ccm_reg *mxc_ccm = (struct mxc_ccm_reg *)MXC_CCM_BASE;
> +
> +	i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE);
> +
> +	/* Set core voltage VDDGP to 1.05V for 800MHZ */
> +	buf[0] = 0x45;
> +	buf[1] = 0x4a;
> +	buf[2] = 0x52;

Please remove all fixed constants in the file and replaced them with
named constants, defined in a header file. Check vision2.c as reference.
This board uses the MC13892 PMIC controller, and ./include/mc13892.h
contains all required defines.

> +	if (i2c_write(0x8, 24, 1, buf, 3))
> +		return;

Ditto. The same globally.

> +	if (is_soc_rev(CHIP_REV_2_0) == 0) {

Please add some comments describing why depending on the processor
revision we should to manage the pmic in a different way.

> +int board_mmc_getcd(u8 *cd, struct mmc *mmc)
> +{
> +	struct fsl_esdhc_cfg *cfg = (struct fsl_esdhc_cfg *)mmc->priv;
> +
> +	if (cfg->esdhc_base == MMC_SDHC1_BASE_ADDR)
> +		*cd = readl(GPIO3_BASE_ADDR) & 0x2000;

Please change this. There is mxc_get_gpio() and mxc_set_gpio() accessors.

> +int board_init(void)
> +{
> +	system_rev = get_cpu_rev();

I think we can get rid of system_rev. It is not used in this function
and you can call get_cpu_rev() directly in checkboard() when the value
is really needed.

> +#ifdef BOARD_LATE_INIT
> +int board_late_init(void)
> +{
> +#ifdef CONFIG_I2C_MXC

Is it the board working if the pmic is not configured ? As I remember
from mx51evk, the network did not work if the PMIC via SPI was not
configured. If this is the case even for mx53evk, setting CONFIG_I2C_MXC
is a must, else a lot of thing cannot work. Then I would prefer to
remove these #ifdef and producing an error if it is not set at the
beginning of the file.

> +int checkboard(void)
> +{
> +	puts("Board: MX53EVK [");
> +
> +	switch (__REG(SRC_BASE_ADDR + 0x8)) {

Again, there is a "src" structure for i.MX51. If it is not correct for
i.MX53, you have to adapt it in imx-regs.h, but you cannot access
directly to registers. Please use always the correct structure or define
newer ones if they do not exist.

> +/*
> + * Disabled for now due to build problems under Debian and a significant
> + * increase in the final file size: 144260 vs. 109536 Bytes.
> + */

I see the same comment in mx51evk.h, but does it make sense ?

> +/*
> + * I2C Configs
> + */
> +#define CONFIG_CMD_I2C          1
> +#define CONFIG_HARD_I2C         1
> +#define CONFIG_I2C_MXC          1
> +#define CONFIG_SYS_I2C_PORT             I2C2_BASE_ADDR

As stated before: port means an enumeration value (0,1,..N), and it is
set to a physical address.

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================