[U-Boot] [PATCH 7/7] Add support for Freescale's mx35pdk board.

Stefano Babic
2011-01-19T10:01:40Z

On 01/19/2011 09:06 AM, Wolfgang Denk wrote:
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index d7cd09c..3abb4cb 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -554,6 +554,7 @@ Stefano Babic <sbabic@denx.de>
>> ea20 davinci
>> polaris xscale
>> trizepsiv xscale
>> + mx35pdk i.MX35
>> mx51evk i.MX51
>> vision2 i.MX51
>
> Please sort list.

Agree.

>> diff --git a/MAKEALL b/MAKEALL
>> index a732e6a..31dbfe1 100755
>> --- a/MAKEALL
>> +++ b/MAKEALL
>> @@ -409,6 +409,7 @@ LIST_ARM11=" \
>> mx31ads \
>> mx31pdk \
>> mx31pdk_nand \
>> + mx35pdk \
>> qong \
>> smdk6400 \
>> tnetv107x_evm \
>
> NAK. We don't add boards to MAKEALL any more. They get auto-selcted
> from their entry in boards.cfg.

I missed the point, I wil lfix it.

>> -#define X_ARM_MMU_SECTION(abase, vbase, size, cache, buff, access) \
>> - { \
>> - int i; int j = abase; int k = vbase; \
>> - for (i = size; i > 0 ; i--, j++, k++) \
>> - ARM_MMU_SECTION(ttb_base, j, k, cache, buff, access); \
>> - }
>
> Here and everywhere else: Macros with multiple statements should be
> enclosed in a do - while block.

The patch removes for mistake this file, that does not really exist in
u-boot. I have aklready fixed it.

>> +CONFIG_SYS_TEXT_BASE = 0xA0000000
>
> NAK. Please move CONFIG_SYS_TEXT_BASE into board config file and
> ditch config.mk

Thanks, I have understood now how it works.

>
>> +int checkboard(void)
>> +{
>> + u32 system_rev = get_cpu_rev();
>> + u32 board_rev = 0;
>> + struct ccm_regs *ccm =
>> + (struct ccm_regs *)IMX_CCM_BASE;
>> +
>> + puts("Board: MX35 3STACK ");
>
> Is this the correct board name?

I will change it as MX35PDK

>
>> + board_rev = board_detect();
>> +
>> + /* Print board revision */
>> + if (board_rev)
>> + puts("2.0");
>> + else
>> + puts("1.0");
>
> Maybe board_detect() could return the board revision sirectly, so you
> can use a single printf for all this, like:
>
> printf("Board: mx35pdk %d.0", board_detect());
>
> ?
>
>> + /* Print CPU revision */
>> + puts(" i.MX35 ");
>> + if (system_rev & CHIP_REV_2_0)
>> + puts("2.0 [");
>> + else
>> + puts("1.0 [");
>
> Eventually something similar could / should be done here?

Yes, this should be (get_cpu_rev() & CHIP_REV_2_0)

> ...
>> +NAND partitions can be recognized enabling in kernel CONFIG_MTD_REDBOOT_PARTS.
>> +For this board, CONFIG_MTD_REDBOOT_DIRECTORY_BLOCK should be set to 2.
>> +
>> +However, the setup in redboot is not correct and does not use the whole flash.
>> +
>> +Better solution is to use the kernel parameter mtdparts. Here the resulting script to be defined in RedBoot with fconfig:
>
> Lines too long. Please fix globally (at least in text).

Ok

>
> ...
>> +#define CONFIG_EXTRA_ENV_SETTINGS \
> ...
>> + "uboot=u-boot.bin\0" \
>> + "kernel_addr_r=0x80800000\0" \
>> + "kernel=uImage\0" \
>
> Default locations are "<boardname>/u-boot.bin" resp.
> "<boardname>/uImage".
>
>> + "prg_uboot=tftpboot ${loadaddr} ${uboot};" \
>> + "protect off ${uboot_addr} 0xa003ffff;" \
>> + "erase ${uboot_addr} 0xa003ffff;" \
>> + "cp.b ${loadaddr} ${uboot_addr} ${filesize};" \
>> + "setenv filesize;saveenv\0"
>
> We usually split this into "load" and "update" steps, so you don;t
> automatically erase your flash even when the download failed.

I will change it.

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
=====================================================================