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 <l...@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: off...@denx.de
=====================================================================
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
