Dear Stefano Babic,
In message 1263212760-27272-10-git-send-email-sbabic@denx.de you wrote:
The patch adds initial support for the Freescale mx51evk board. Network (FEC) and SD controller (fsl_esdhc) are supported.
--- a/Makefile +++ b/Makefile @@ -324,6 +324,10 @@ $(obj)u-boot.img: $(obj)u-boot.bin sed -e 's/"[ ]*$$/ for $(BOARD) board"/') \ -d $< $@
+$(obj)u-boot.imx: $(obj)u-boot.bin
$(obj)tools/mkimage -n $(IMX_CONFIG) -T imximage \-e $(TEXT_BASE) -d $< $@
This actually belongs into the patch that adds the imx image format support.
+int dram_init(void) +{
- gd->bd->bi_dram[0].start = PHYS_SDRAM_1;
- gd->bd->bi_dram[0].size = get_ram_size((long *)PHYS_SDRAM_1, PHYS_SDRAM_1_SIZE);
Line too long. Please check globally.
- return 0;
+}
+static void setup_uart(void) +{
- unsigned int pad = PAD_CTL_HYS_ENABLE | PAD_CTL_PKE_ENABLE |
PAD_CTL_PUE_PULL | PAD_CTL_DRV_HIGH;
Indentation by TAB, please. Please check globally.
+static void setup_expio(void) +{
- u32 reg;
- /* CS5 setup */
- mxc_request_iomux(MX51_PIN_EIM_CS5, IOMUX_CONFIG_ALT0);
- writel(0x00410089, WEIM_BASE_ADDR + 0x78 + CSGCR1);
- writel(0x00000002, WEIM_BASE_ADDR + 0x78 + CSGCR2);
- /* RWSC=50, RADVA=2, RADVN=6, OEA=0, OEN=0, RCSA=0, RCSN=0 */
- writel(0x32260000, WEIM_BASE_ADDR + 0x78 + CSRCR1);
- /* APR = 0 */
- writel(0x00000000, WEIM_BASE_ADDR + 0x78 + CSRCR2);
What is this magic offset 0x78 here? Please get rid of using base address + offset notation, use C struncts instead (see previous review comments about this). Please fix globally.
- /* Reset the XUART and Ethernet controllers */
- reg = readw(&(mx51_io_board->sw_reset));
- reg |= 0x9;
- writew(reg, &(mx51_io_board->sw_reset));
- reg &= ~0x9;
- writew(reg, &(mx51_io_board->sw_reset));
+}
+static void setup_fec(void) +{
FEC should only be set up (and eventually only be reset, too), if there is any network support at all on this board.
...
+int board_mmc_init(bd_t *bis) +{
- u32 interface_esdhc = 0;
- s32 status = 0;
- u32 esdhc_base_pointer;
- for (interface_esdhc = 0; interface_esdhc < CONFIG_SYS_FSL_ESDHC_NUM; interface_esdhc++) {
switch (interface_esdhc) {case 0:
Incorrect indent. And line too loing above, of course.
...
break;case 1:
...
break;}
Incorrect indent. Default case?
diff --git a/board/freescale/mx51evk/mx51evk.h b/board/freescale/mx51evk/mx51evk.h new file mode 100644 index 0000000..fec886d --- /dev/null +++ b/board/freescale/mx51evk/mx51evk.h @@ -0,0 +1,49 @@ +/*
- Copyright 2009 Freescale Semiconductor, Inc. All Rights Reserved.
- */
Please get rid off the "All Rights Reserved."
+/*
- The code contained herein is licensed under the GNU General Public
- License. You may obtain a copy of the GNU General Public License
- Version 2 or later at the following locations:
As mentioned before, this clause is not acceptable.
diff --git a/include/configs/mx51evk.h b/include/configs/mx51evk.h new file mode 100644 index 0000000..c8b2970 --- /dev/null +++ b/include/configs/mx51evk.h
...
- /* High Level Configuration Options */
+#define CONFIG_SYS_APCS_GNU
What's this? It seems to be not used anywhere, nor documented?
+#define CONFIG_L2_OFF
Is this a "High Level Configuration Option"? Move down!
+#define CONFIG_SYS_MEMTEST_START CSD0_BASE_ADDR +#define CONFIG_SYS_MEMTEST_END 0x10000
+#define CONFIG_SYS_LOAD_ADDR CONFIG_LOADADDR
Here and everywhere else: please use TABs for vertical alignment.
+#define CONFIG_ENV_SECT_SIZE (128 * 1024) +#define CONFIG_ENV_SIZE CONFIG_ENV_SECT_SIZE +#define CONFIG_ENV_IS_NOWHERE
Seems strange to me to define an environment sector size and an environment size and then to say there is no environment at all?
Best regards,
Wolfgang Denk