Dear Alan Carvalho de Assis,
In message 37367b3a0909151429h317066ax2efa504d83dbf36a@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@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