[U-Boot] Subject: [PATCH v3] mx27ads: add support for iMX27ADS board from Freescale

Tue Sep 22 20:26:30 CEST 2009

Dear Alan Carvalho de Assis,

In message <37367b3a0909151429h317066ax2efa504d83dbf36a at 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 <acassis at 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: wd at 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.