Re: [U-Boot] [PATCH] mpc512x: Add esd gmbh mecp5123 board support

Dear Stefan Roese,

In message 1244447665-14264-1-git-send-email-sr@denx.de you wrote:

MECP5123 is a MPC5121E based module by esd gmbh.

Signed-off-by: Stefan Roese sr@denx.de Cc: Reinhard Arlt reinhard.arlt@esd-electronics.com

...

+int eeprom_write_enable(unsigned dev_addr, int state) +{

• volatile immap_t *im = (immap_t *)CONFIG_SYS_IMMR;

•    if (dev_addr != CONFIG_SYS_I2C_EEPROM_ADDR)
  

•        return -1;
  

•    if (state == 0)
  

• setbits_be32(&im->gpio.gpdat, 0x00100000);
  

• else

• clrbits_be32(&im->gpio.gpdat, 0x00100000);
  

•    return 0;
  

+}

Please fix indentation - indentation by TAB only, please.

...

+int board_early_init_f(void) +{

...

• /* Initialize DDR Priority Manager */
• out_be32(&im->mddrc.prioman_config1, CONFIG_SYS_MDDRCGRP_PM_CFG1);
• out_be32(&im->mddrc.prioman_config2, CONFIG_SYS_MDDRCGRP_PM_CFG2);
• out_be32(&im->mddrc.hiprio_config, CONFIG_SYS_MDDRCGRP_HIPRIO_CFG);
• out_be32(&im->mddrc.lut_table0_main_upper, CONFIG_SYS_MDDRCGRP_LUT0_MU);
• out_be32(&im->mddrc.lut_table0_main_lower, CONFIG_SYS_MDDRCGRP_LUT0_ML);
• out_be32(&im->mddrc.lut_table1_main_upper, CONFIG_SYS_MDDRCGRP_LUT1_MU);
• out_be32(&im->mddrc.lut_table1_main_lower, CONFIG_SYS_MDDRCGRP_LUT1_ML);
• out_be32(&im->mddrc.lut_table2_main_upper, CONFIG_SYS_MDDRCGRP_LUT2_MU);
• out_be32(&im->mddrc.lut_table2_main_lower, CONFIG_SYS_MDDRCGRP_LUT2_ML);
• out_be32(&im->mddrc.lut_table3_main_upper, CONFIG_SYS_MDDRCGRP_LUT3_MU);
• out_be32(&im->mddrc.lut_table3_main_lower, CONFIG_SYS_MDDRCGRP_LUT3_ML);
• out_be32(&im->mddrc.lut_table4_main_upper, CONFIG_SYS_MDDRCGRP_LUT4_MU);
• out_be32(&im->mddrc.lut_table4_main_lower, CONFIG_SYS_MDDRCGRP_LUT4_ML);
• out_be32(&im->mddrc.lut_table0_alternate_upper, CONFIG_SYS_MDDRCGRP_LUT0_AU);
• out_be32(&im->mddrc.lut_table0_alternate_lower, CONFIG_SYS_MDDRCGRP_LUT0_AL);
• out_be32(&im->mddrc.lut_table1_alternate_upper, CONFIG_SYS_MDDRCGRP_LUT1_AU);
• out_be32(&im->mddrc.lut_table1_alternate_lower, CONFIG_SYS_MDDRCGRP_LUT1_AL);
• out_be32(&im->mddrc.lut_table2_alternate_upper, CONFIG_SYS_MDDRCGRP_LUT2_AU);
• out_be32(&im->mddrc.lut_table2_alternate_lower, CONFIG_SYS_MDDRCGRP_LUT2_AL);
• out_be32(&im->mddrc.lut_table3_alternate_upper, CONFIG_SYS_MDDRCGRP_LUT3_AU);
• out_be32(&im->mddrc.lut_table3_alternate_lower, CONFIG_SYS_MDDRCGRP_LUT3_AL);
• out_be32(&im->mddrc.lut_table4_alternate_upper, CONFIG_SYS_MDDRCGRP_LUT4_AU);
• out_be32(&im->mddrc.lut_table4_alternate_lower, CONFIG_SYS_MDDRCGRP_LUT4_AL);

Lines too long.

+phys_size_t initdram(int board_type) +{

• return fixed_sdram();

+}

Use auto-sizing?

+int misc_init_r(void) +{

• volatile immap_t *im = (immap_t *) CONFIG_SYS_IMMR;
• clrbits_be32(&im->clk.scfr[0], 0x00003800);
• setbits_be32(&im->clk.scfr[0], 0x01 << 11);
• clrbits_be32(&im->lpc.cs_dccr, 0x000000ff);
• setbits_be32(&im->lpc.cs_dccr, (0x00 << 4) | (0x01 << 0));
• clrbits_be32(&im->lpc.cs_hccr, 0x000000ff);
• setbits_be32(&im->lpc.cs_hccr, (0x00 << 4) | (0x01 << 0));

Please add some comments what you are doing here, and eventually #define the magic constants used.

...

--- /dev/null +++ b/include/configs/mecp5123.h @@ -0,0 +1,455 @@ +/*

• • (C) Copyright 2007, 2008 DENX Software Engineering

2009?

...

+/*

• • NOR FLASH on the Local Bus
• */

+#define CONFIG_SYS_FLASH_CFI /* use the Common Flash Interface */ +#define CONFIG_FLASH_CFI_DRIVER /* use the CFI driver */

+#define CONFIG_SYS_FLASH_BASE 0xFFC00000 /* start of FLASH */ +#define CONFIG_SYS_FLASH_SIZE 0x00400000 /* max flash size in bytes */

Lines too long (check everywhere!)

...

+#define CONFIG_EXTRA_ENV_SETTINGS \

• "u-boot_addr_r=200000\0" \
• "kernel_addr_r=600000\0" \
• "fdt_addr_r=880000\0" \
• "ramdisk_addr_r=900000\0" \
• "u-boot_addr=FFF00000\0" \
• "kernel_addr=FFC40000\0" \
• "fdt_addr=FFEC0000\0" \
• "ramdisk_addr=FC040000\0" \
• "ramdiskfile=/tftpboot/mecp512x/uRamdisk\0" \
• "u-boot=/tftpboot/mecp512x/u-boot.bin\0" \
• "bootfile=/tftpboot/mecp512x/uImage\0" \
• "fdtfile=/tftpboot/mecp512x/mecp512x.dtb\0" \
• "rootpath=/tftpboot/mecp512x/target_root\n" \
• "netdev=eth0\0" \
• "consdev=ttyPSC0\0" \
• "nfsargs=setenv bootargs root=/dev/nfs rw " \

• "nfsroot=${serverip}:${rootpath}\0"			\
  

• "ramargs=setenv bootargs root=/dev/ram rw\0" \
• "addip=setenv bootargs ${bootargs} " \

• "ip=${ipaddr}:${serverip}:${gatewayip}:${netmask}"	\
  

• ":${hostname}:${netdev}:off panic=1\0"			\
  

• "addtty=setenv bootargs ${bootargs} " \

• "console=${consdev},${baudrate}\0"			\
  

• "flash_nfs=run nfsargs addip addtty;" \

• "bootm ${kernel_addr} - ${fdt_addr}\0"			\
  

• "flash_self=run ramargs addip addtty;" \

• "bootm ${kernel_addr} ${ramdisk_addr} ${fdt_addr}\0"	\
  

• "net_nfs=tftp ${kernel_addr_r} ${bootfile};" \

• "tftp ${fdt_addr_r} ${fdtfile};"			\
  

• "run nfsargs addip addtty;"				\
  

• "bootm ${kernel_addr_r} - ${fdt_addr_r}\0"		\
  

• "net_self=tftp ${kernel_addr_r} ${bootfile};" \

• "tftp ${ramdisk_addr_r} ${ramdiskfile};"		\
  

• "tftp ${fdt_addr_r} ${fdtfile};"			\
  

• "run ramargs addip addtty;"				\
  

• "bootm ${kernel_addr_r} ${ramdisk_addr_r} ${fdt_addr_r}\0"\
  

• "load=tftp ${u-boot_addr_r} ${u-boot}\0" \
• "update=protect off ${u-boot_addr} +${filesize};" \

• "era ${u-boot_addr} +${filesize};"			\
  

• "cp.b ${u-boot_addr_r} ${u-boot_addr} ${filesize}\0"	\
  

• "upd=run load update\0" \
• ""

How long does it to boot with such an environment in EEPROM?

+#define CONFIG_OF_LIBFDT 1 +#define CONFIG_OF_BOARD_SETUP 1 +#define CONFIG_OF_SUPPORT_OLD_DEVICE_TREES 1

Do you really need this on a new board?

Best regards,

Wolfgang Denk