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

[Wolfgang Denk]

Dear Stefano Babic,

In message <1295012124-15551-7-git-send-email-sbabic at denx.de> you wrote:
> The patch adds suupport for the Freescale's mx35pdk board
> (known as well as mx35_3stack).
> 
> The board boots from the NOR flash. Following devices
> are supported:
>  - two ethernet devices (FEC and SMC911x on debug board)
>  - I2C
>  - PMIC (MC13892) via I2C interface
>  - UART
>  - NOR flash (64MB)
>  - NAND flash (2GB)
>  - basic access to mc9sdz60 registers via I2C interface
> 
> Signed-off-by: Stefano Babic <sbabic at denx.de>
...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d7cd09c..3abb4cb 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -554,6 +554,7 @@ Stefano Babic <sbabic at denx.de>
>  	ea20		davinci
>  	polaris		xscale
>  	trizepsiv	xscale
> +	mx35pdk		i.MX35
>  	mx51evk		i.MX51
>   	vision2		i.MX51

Please sort list.

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

> -#define ARM_MMU_FIRST_LEVEL_DESCRIPTOR_ADDRESS(ttb_base, table_index) \
> -	(unsigned long *)((unsigned long)(ttb_base) + ((table_index) << 2))
> -
> -#define ARM_FIRST_LEVEL_PAGE_TABLE_SIZE 0x4000
> -
> -#define ARM_MMU_SECTION(ttb_base, actual_base, virtual_base,		\
> -			cacheable, bufferable, perm)			\
> -	{								\
> -	register union ARM_MMU_FIRST_LEVEL_DESCRIPTOR desc;		\
> -	desc.word = 0;							\
> -	desc.section.id = ARM_MMU_FIRST_LEVEL_SECTION_ID;		\
> -	desc.section.domain = 0;					\
> -	desc.section.c = (cacheable);					\
> -	desc.section.b = (bufferable);					\
> -	desc.section.ap = (perm);					\
> -	desc.section.base_address = (actual_base);			\
> -	*ARM_MMU_FIRST_LEVEL_DESCRIPTOR_ADDRESS(ttb_base, (virtual_base)) \
> -				= desc.word;				\
> -	}
> -
> -#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.

> diff --git a/board/freescale/mx35pdk/config.mk b/board/freescale/mx35pdk/config.mk
> new file mode 100644
> index 0000000..3db1c85
> --- /dev/null
> +++ b/board/freescale/mx35pdk/config.mk
...
> +CONFIG_SYS_TEXT_BASE = 0xA0000000

NAK.  Please move CONFIG_SYS_TEXT_BASE into board config file and
ditch config.mk

> +/* To support 133MHz DDR */
> +.macro  init_drive_strength
> +/*
> +	mov r0, #0x2
> +	ldr r1, =IOMUXC_BASE_ADDR
> +	add r1, r1, #0x368
> +        add r2, r1, #0x4C8 - 0x368
> +1:      str r0, [r1], #4
> +	cmp r1, r2
> +        ble 1b
> +*/
> +.endm /* init_drive_strength */

Please remove dead code - please fix globally.

Please use TAB for indentation - please fix globally.

> +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?

> +	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?


> --- /dev/null
> +++ b/doc/README.mx35pdk
...
...
> +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).

> --- /dev/null
> +++ b/include/configs/mx35pdk.h
...
...
> +#define CONFIG_CMDLINE_TAG		1	/* enable passing of ATAGs */
> +#define CONFIG_REVISION_TAG		1
> +#define CONFIG_SETUP_MEMORY_TAGS	1
> +#define CONFIG_INITRD_TAG		1

Please omit all such '1'.

...
> +#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.


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
Time is fluid ... like a river with currents, eddies, backwash.
	-- Spock, "The City on the Edge of Forever", stardate 3134.0