Dear Stefano Babic,

In message <1295012124-15551-7-git-send-email-sba...@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 <sba...@denx.de>
...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d7cd09c..3abb4cb 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -554,6 +554,7 @@ Stefano Babic <sba...@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: w...@denx.de
Time is fluid ... like a river with currents, eddies, backwash.
        -- Spock, "The City on the Edge of Forever", stardate 3134.0
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to