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

[Stefano Babic <sbabic@denx.de>]

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