Dear Alan Carvalho de Assis,
In message <37367b3a0909151429h317066ax2efa504d83dbf...@mail.gmail.com> you
wrote:
> This patch adds support to iMX27ADS development board. This board has
> 128MB RAM, 32MB NOR Flash and 128MB NAND Flash. Currently only
> booting from NOR is supported.
>
> Signed-off-by: Alan Carvalho de Assis <acas...@gmail.com>
I will try and mention only the issues not yet covered.
...
> --- a/Makefile
> +++ b/Makefile
> @@ -2965,6 +2965,9 @@ davinci_dm365evm_config : unconfig
> imx27lite_config: unconfig
> @$(MKCONFIG) $(@:_config=) arm arm926ejs imx27lite logicpd mx27
>
> +mx27ads_config : unconfig
> + @$(MKCONFIG) $(@:_config=) arm arm926ejs mx27ads freescale mx27
> +
> lpd7a400_config \
> lpd7a404_config: unconfig
Please keep list sorted.
> diff --git a/board/freescale/mx27ads/Makefile
> b/board/freescale/mx27ads/Makefile
> new file mode 100644
> index 0000000..d142a9e
> --- /dev/null
> +++ b/board/freescale/mx27ads/Makefile
...
> +#########################################################################
> +
Please don;t add trailing empty lines.
> diff --git a/board/freescale/mx27ads/config.mk
> b/board/freescale/mx27ads/config.mk
> new file mode 100644
> index 0000000..a2e7768
> --- /dev/null
> +++ b/board/freescale/mx27ads/config.mk
...
> +/*
> + * For clock initialization, see chapter 3 of the "MCIMX27 Multimedia
> + * Applications Processor Reference Manual, Rev. 0.2".
> + *
> + */
Please don't add random empty comment lines.
...
> + write32 0xA0000F00, 0x00000000
> + write32 0xD8001000, 0xb2100000
> + ldr r0, =0xA0000033
> + mov r1, #0xda
> + strb r1, [r0]
> + ldr r0, =0xA1000000
> + mov r1, #0xff
> + strb r1, [r0]
> + write32 0xD8001000, 0x82226080
...
> + mov r10, lr
...
> + ldr r0, =CSCR
> + ldr r1, [r0]
> + bic r1, r1, #0x03
> + str r1, [r0]
Please use a consistent style for indentation. I suggest you use the
insn name followed by a single TAB character followed by args.
> +++ b/board/freescale/mx27ads/mx27ads.c
> @@ -0,0 +1,93 @@
...
...
> +# define __REG(x) (*((volatile u32 *)(x)))
> +
> +#define IMX_CS4_BASE 0xD4000000
> +#define CS4U __REG(IMX_WEIM_BASE + 0x40) /* Chip Select 4 Upper Register
> */
> +#define CS4L __REG(IMX_WEIM_BASE + 0x44) /* Chip Select 4 Lower Register
> */
> +#define CS4A __REG(IMX_WEIM_BASE + 0x48) /* Chip Select 4 Addition Register
> */
Please use proper I/O accessors. Direct register accesses like here
are forbidden. Also, you probably want to define a C structure
describing the register map.
...
> + /* Configure CPLD on CS4 */
> + CS4U = 0x0000DCF6;
> + CS4L = 0x444A4541;
> + CS4A = 0x44443302;
Please never use suchmagic numbers in the code. Provide some #defines
for these, together with sufficient comments to explain what the
numbers mean.
> + /* Select FEC data through data path */
> + writew(0x0020, IMX_CS4_BASE + 0x10);
> +
> + /* Enable CPLD FEC data path */
> + writew(0x0010, IMX_CS4_BASE + 0x14);
Please use C structs for the register mapping; do not use base address
plus offsets.
> +int dram_init(void)
> +{
> +
No empty line here.
> +#if CONFIG_NR_DRAM_BANKS > 0
> + gd->bd->bi_dram[0].start = PHYS_SDRAM_1;
> + gd->bd->bi_dram[0].size = get_ram_size((volatile void *)PHYS_SDRAM_1,
> + PHYS_SDRAM_1_SIZE);
> +#endif
Um... is CONFIG_NR_DRAM_BANKS <= 0 any configuration that is supposed
to work?
> +#if CONFIG_NR_DRAM_BANKS > 1
> + gd->bd->bi_dram[1].start = PHYS_SDRAM_2;
> + gd->bd->bi_dram[1].size = get_ram_size((volatile void *)PHYS_SDRAM_2,
> + PHYS_SDRAM_2_SIZE);
> +#endif
> +
> + return 0;
You unconditionally return OK< even in case of memory errors? This
seems wrong to me.
> diff --git a/board/freescale/mx27ads/u-boot.lds
> b/board/freescale/mx27ads/u-boot.lds
> new file mode 100644
> index 0000000..f66f20e
> --- /dev/null
> +++ b/board/freescale/mx27ads/u-boot.lds
Does this board need a private linke rscript? Why cannot you use the
common script in cpu/arm926ejs/u-boot.lds ?
> diff --git a/include/configs/mx27ads.h b/include/configs/mx27ads.h
> new file mode 100644
> index 0000000..3360d76
> --- /dev/null
> +++ b/include/configs/mx27ads.h
...
> +/* memtest start address */
> +#define CONFIG_SYS_MEMTEST_START 0xA0000000
> +#define CONFIG_SYS_MEMTEST_END 0xA1000000 /* 16MB RAM
> test */
Did you ever test this? Overwriting low memory where exception vectors
and such is located is probably a bad idea.
...
> +/* Use hardware sector protection */
> +#define CONFIG_SYS_FLASH_PROTECTION 1
> +#define CONFIG_SYS_MAX_FLASH_BANKS 1 /* max number of flash banks */
> +#define CONFIG_SYS_FLASH_SECT_SZ 0x2000 /* 8KB sect size Intel Flash */
> +/* end of flash */
> +#define CONFIG_ENV_OFFSET (PHYS_FLASH_SIZE - 0x20000)
> +/* CS2 Base address */
> +#define PHYS_FLASH_1 0xc0000000
> +/* Flash Base for U-Boot */
> +#define CONFIG_SYS_FLASH_BASE PHYS_FLASH_1
> +/* Flash size 32MB */
> +#define PHYS_FLASH_SIZE 0x2000000
> +#define CONFIG_SYS_MAX_FLASH_SECT (PHYS_FLASH_SIZE / \
> + CONFIG_SYS_FLASH_SECT_SZ)
> +#define CONFIG_SYS_MONITOR_BASE CONFIG_SYS_FLASH_BASE
> +#define CONFIG_SYS_MONITOR_LEN 0x40000 /* Reserve
> 256KiB */
> +#define CONFIG_ENV_SECT_SIZE 0x10000 /* Env sector Size */
This looks strange to me. Above you state "8KB sect size", but here
you set a sector size of 64 kB ?
> +/*#define CONFIG_DRIVER_CS8900 1
> +#define CS8900_BASE 0xD4020300
> +#define CS8900_BUS16 1*/
> +#define CONFIG_ETHADDR 02:80:ad:20:31:e8
Drop that. We don't allow setting a fixed MAC address for all boards.
> +#define MTDPARTS_DEFAULT \
> +
> "mtdparts=physmap-flash.0:256k(U-Boot),3968k(kernel),28416k(user),64k(env1),64k(env2)"
Line too long (please check all files).
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
One essential to success is that you desire be an all-obsessing one,
your thoughts and aims be co-ordinated, and your energy be concentra-
ted and applied without letup.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
