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

Wolfgang Denk
2011-01-19T08:06:07Z

Dear Stefano Babic,

In message <1295012124-15551-7-git-send-email-sbabic@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@denx.de>
...
> 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.

> 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