Hello Wolfgang,

I am planning to resubmit this patch very soon - by Tuesday perhaps. Considering there is some differences in the memory layout esp for the iopin struct (going from 32-bit to byte sizes) I am not sure how I can get around the #defs you suggest. Perhaps an entirely separate .c file for this part ??

Much of what you pointed to was copied over from the mpc5121ads ... so I didn't pay that much attention. I will fix these too and also fix the 5121 code later.

When do you need it so I can give you enough time to look it over?

Thanks Wolfgang, Martha

-----Original Message----- From: Wolfgang Denk wd@denx.de Sent 10/3/2009 6:43:51 PM To: Martha M Stan mmarx@silicontkx.com Cc: u-boot@lists.denx.de Subject: Re: [U-Boot] [PATCH] Add mpc5125ads board and processor to the mpc512x family

Dear Martha M Stan,

do you plan to send an updated patch for this release?

In addition to Fabio's comments here a few more:

In message 12535648622828-git-send-email-mmarx@silicontkx.com you wrote:

... diff --git a/board/freescale/mpc5125ads/mpc5125ads.c b/board/freescale/mpc5125ads/mpc5125ads.c new file mode 100644 index 0000000..445c765 --- /dev/null +++ b/board/freescale/mpc5125ads/mpc5125ads.c ... +if (in_8((u8 *)(CONFIG_SYS_CPLD_BASE + 0x08)) & 0x04) { +out_8((u8 *)(CONFIG_SYS_CPLD_BASE + 0x08), 0xc1); +} else { +/* running from Backup flash */ +out_8((u8 *)(CONFIG_SYS_CPLD_BASE + 0x08), 0x32); +}

Please do not use base address plus offset, but define a C struct to describe the CPLD's register layout.

[That would also be most welcome to fix similar code in "board/freescale/mpc5121ads/mpc5121ads.c"; sorry - due to lack of documentation I was not able to clean this up yet.]

+#endif +/* + * initialize function mux & slew rate for all IO pins + * there are two peripheral options controlled by switch 8 + */ + if (IS_CFG1_SWITCH) {

Indentation not by TAB.

+#ifdef CONFIG_MISC_INIT_R +int misc_init_r(void) +{ +u8 tmp_val; + +extern int ads5121_diu_init(void); + + immap_t *im = (immap_t *) CONFIG_SYS_IMMR; + u32 bkup_size = flash_info[0].size; + u32 bkup_start = 0xffffffff - bkup_size +1;

Indentation not by TAB. Please fix globally.

+/* Verify if enabled */ +tmp_val = 0; +i2c_read(0x38, 0x08, 1, &tmp_val, sizeof(tmp_val)); ... +tmp_val = 0; +i2c_read(0x38, 0x0A, 1, &tmp_val, sizeof(tmp_val));

Which sense does it make to set tmp_val when you overwrite the value immediately by running i2c_read()?

And would it be not a good idea to check i2c_read() / i2c_write() return codes?

+int checkboard (void) +{ +ushort brd_rev = *(vu_short *) (CONFIG_SYS_CPLD_BASE + 0x00); +uchar cpld_rev = *(vu_char *) (CONFIG_SYS_CPLD_BASE + 0x02); +uchar cpld_min_rev = *(vu_char *) (CONFIG_SYS_CPLD_BASE + 0x03);

See above - please use a C struct to describe the CPLD layout.

+if (IS_CFG1_SWITCH) /* CAN1+2, SDHC1, USB1+2, FEC1+2, I2C1+2*/ +printf("Peripheral Option Set 1\n"); +else/* CAN1+2, SDHC1, DIU, USB1, FEC1, I2C1+3*/ +printf("Peripheral Option Set 0\n");

Don't repeat long strings without need. How about:

printf("Peripheral Option Set %d\n", IS_CFG1_SWITCH != 0);

Hm... I wonder what the output format will look like. I guess there might be vertical alignment issues?

diff --git a/board/freescale/mpc5125ads/u-boot.lds b/board/freescale/mpc5125ads/u-boot.lds new file mode 100644 index 0000000..f2f6e14 --- /dev/null +++ b/board/freescale/mpc5125ads/u-boot.lds

Do you really need a private linker script? Why?

diff --git a/cpu/mpc512x/fixed_sdram.c b/cpu/mpc512x/fixed_sdram.c index 63a3035..12ac44d 100644 --- a/cpu/mpc512x/fixed_sdram.c +++ b/cpu/mpc512x/fixed_sdram.c @@ -36,6 +36,7 @@ u32 default_mddrc_config[4] = {

u32 default_init_seq[] = { +#ifndef CONFIG_SYS_DDR_OVRIDE_DEF CONFIG_SYS_DDRCMD_NOP, CONFIG_SYS_DDRCMD_NOP, CONFIG_SYS_DDRCMD_NOP, @@ -67,6 +68,7 @@ u32 default_init_seq[] = { CONFIG_SYS_DDRCMD_OCD_DEFAULT, CONFIG_SYS_DDRCMD_PCHG_ALL, CONFIG_SYS_DDRCMD_NOP +#endif

Isn't there way to do without such #ifdef's in common code?

/* Initialize IO Control */ -out_be32(io_ctrl.io_control_mem, IOCTRL_MUX_DDR); +#ifdef CONFIG_MPC5125 +out_8(io_ctrl.io_control_mem, IOCTRL_MUX_DDR); +#else + out_be32(io_ctrl.io_control_mem, IOCTRL_MUX_DDR); +#endif

Ditto here.

diff --git a/cpu/mpc512x/iopin.c b/cpu/mpc512x/iopin.c index be20947..ab5dd1c 100644 --- a/cpu/mpc512x/iopin.c +++ b/cpu/mpc512x/iopin.c @@ -28,15 +28,27 @@ void iopin_initialize(iopin_t *ioregs_init, int len) { short i, j, p; -u32 *reg; immap_t *im = (immap_t *)CONFIG_SYS_IMMR;

-reg = (u32 *)&(im-io_ctrl); +#ifdef CONFIG_MPC5125 +u8 *reg =(u8 *)&(im-io_ctrl); +#else +u32 *reg =(u32 *)&(im-io_ctrl); +#endif

And here again.

This is becoming an #ifdef mess, it seems. Usually this is an indiaction of a major design issue.

+#ifdef CONFIG_MPC5125 +for (p = 0, j = ioregs_init[i].p_offset; +p ioregs_init[i].nr_pins; p++, j++) { +if (ioregs_init[i].bit_or) +setbits_8(&(reg[j]), ioregs_init[i].val); +else +out_8(&reg[j], ioregs_init[i].val); +} +#else for (p = 0, j = ioregs_init[i].p_offset / sizeof(u_long); p ioregs_init[i].nr_pins; p++, j++) { if (ioregs_init[i].bit_or) @@ -44,6 +56,7 @@ void iopin_initialize(iopin_t *ioregs_init, int len) else out_be32 (reg + j, ioregs_init[i].val); } +#endif

And again. That's enough: NAK!

diff --git a/drivers/net/mpc512x_fec.c b/drivers/net/mpc512x_fec.c index fb2c19a..3ff1c31 100644 --- a/drivers/net/mpc512x_fec.c +++ b/drivers/net/mpc512x_fec.c @@ -41,7 +41,11 @@ static int rx_buff_idx = 0; static void mpc512x_fec_phydump (char *devname) { u16 phyStatus, i; -u8 phyAddr = CONFIG_PHY_ADDR; +#ifdef CONFIG_MPC5125 +uint8 phyAddr = ((devname[3]=='2') ? CONFIG_PHY2_ADDR : CONFIG_PHY_ADDR); +#else +uint8 phyAddr = CONFIG_PHY_ADDR; +#endif

Umm.. does this work with CONFIG_NET_MULTI ?

diff --git a/include/asm-ppc/immap_512x.h b/include/asm-ppc/immap_512x.h index 79cdd80..50ef20c 100644 --- a/include/asm-ppc/immap_512x.h +++ b/include/asm-ppc/immap_512x.h @@ -36,6 +36,8 @@ #define_START_OFFSETEXC_OFF_SYS_RESET

#define SPR_5121E0x80180000 +#define SPR_51250x80190000 +

Please don't add random white space.

/* * IMMRBAR - Internal Memory Register Base Address @@ -210,21 +212,25 @@ typedef struct clk512x { #define CLOCK_SCCR1_TPR_EN0x00001000 #define CLOCK_SCCR1_PCI_EN0x00000800 #define CLOCK_SCCR1_DDR_EN0x00000400 +#define CLOCK_SCCR1_FEC2_EN0x00000200

/* System Clock Control Register 2 commands */ #define CLOCK_SCCR2_DIU_EN0x80000000 #define CLOCK_SCCR2_AXE_EN0x40000000 #define CLOCK_SCCR2_MEM_EN0x20000000 -#define CLOCK_SCCR2_USB2_EN0x10000000 -#define CLOCK_SCCR2_USB1_EN0x08000000 +#define CLOCK_SCCR2_USB1_EN0x10000000 +#define CLOCK_SCCR2_USB2_EN0x08000000

Is this a bug fix to existing code? This should be a separate patch, then.

#define CLOCK_SCCR2_BDLC_EN0x02000000 +#define CLOCK_SCCR2_AUTO_EN0x02000000 #define CLOCK_SCCR2_SDHC_EN0x01000000 +#define CLOCK_SCCR2_AUTO_EN0x02000000

Why are you adding CLOCK_SCCR2_AUTO_EN twice?

typedef struct ioctrl512x { -u32io_control_mem;/* MEM pad ctrl reg */ -u32io_control_gp;/* GP pad ctrl reg */ -u32io_control_lpc_clk;/* LPC_CLK pad ctrl reg */ -u32io_control_lpc_oe;/* LPC_OE pad ctrl reg */ -u32io_control_lpc_rw;/* LPC_R/W pad ctrl reg */ -u32io_control_lpc_ack;/* LPC_ACK pad ctrl reg */ -u32io_control_lpc_cs0;/* LPC_CS0 pad ctrl reg */ -u32io_control_nfc_ce0;/* NFC_CE0 pad ctrl reg */ -u32io_control_lpc_cs1;/* LPC_CS1 pad ctrl reg */ -u32io_control_lpc_cs2;/* LPC_CS2 pad ctrl reg */ -u32io_control_lpc_ax03;/* LPC_AX03 pad ctrl reg */ -u32io_control_emb_ax02;/* EMB_AX02 pad ctrl reg */ -u32io_control_emb_ax01;/* EMB_AX01 pad ctrl reg */ -u32io_control_emb_ax00;/* EMB_AX00 pad ctrl reg */ -u32io_control_emb_ad31;/* EMB_AD31 pad ctrl reg */ ... -u32io_control_usb_phy_drvvbus;/* USB2_DRVVBUS pad ctrl reg */ -u8reserved[0x0cfc];/* fill to 4096 bytes size */ +#ifdef CONFIG_MPC5125 +u8 io_control_mem;/* offset 0x00 mem pad ctrl reg */ +u8 io_control_gbobe;/* offset 0x01 gbobe pad ctrl reg */ +u8res1[2]; +u8 io_control_lpc_clk;/* offset 0x04 lpc_clk pad ctrl reg */ +u8 io_control_lpc_oe_b;/* offset 0x05 lpc_oe_b pad ctrl reg */ +u8 io_control_lpc_rwb;/* offset 0x06 lpc_rwb pad ctrl reg */ +u8 io_control_lpc_cs0_b;/* offset 0x07 lpc_cs0_b*/ +u8 io_control_lpc_ack_b;/* offset 0x08 lpc_ack_b pad ctrl reg */ +u8 io_control_lpc_ax03;/* offset 0x09 lpc_ax03 pad ctrl reg */ +u8 io_control_emb_ax02;/* offset 0x0a emb_ax02 pad ctrl reg */ +u8 io_control_emb_ax01;/* offset 0x0b emb_ax01 pad ctrl reg */ +u8 io_control_emb_ax00;/* offset 0x0c emb_ax00 pad ctrl reg */ +u8 io_control_emb_ad31;/* offset 0x0d emb_ad31 pad ctrl reg */

NAK!!! If structures are so fundamentally different, it makes zero sense trying to make them look the same. Instead, make clear that these are separate, incompatible things. Declare a new struct for the 5125.

typedef struct psc512x { +#ifdef CONFIG_MPC5125 +volatile u8mr1;/* PSC + 0x00 */ +volatile u8res0[3]; +volatile u8mr2;/* PSC + 0x04 */ +volatile u8res0a[3]; +volatile u16psc_status;/* PSC + 0x08 */ +volatile u16res1; +volatile u16psc_clock_select;/* PSC + 0x0C mpc5125 manual has this as u8 */ + /* it has u8 res after it and for compatibility */ + /* will keep u16 so high bits are set as before */ +volatile u16res1a; +volatile u8command;/* PSC + 0x10 */ +volatile u8res2[3]; +union {/* PSC + 0x14 */ +volatile u8buffer_8; +volatile u16buffer_16; +volatile u32buffer_32; +} buffer;

Same here. Such huge monster-#ifdef's make zero sense. If structs are different, they _are_ different and code shoud reflect that.

diff --git a/include/configs/mpc5121ads.h b/include/configs/mpc5121ads.h index ebc518c..35e69c4 100644 --- a/include/configs/mpc5121ads.h +++ b/include/configs/mpc5121ads.h @@ -28,6 +28,7 @@ #define __CONFIG_H

#define CONFIG_MPC5121ADS 1 + /*

What exactly is the intention behind such a random whitespace change? Please do not mess aroiund with unrelated files.

diff --git a/include/configs/mpc5125ads.h b/include/configs/mpc5125ads.h new file mode 100644 index 0000000..2ba7ba3 --- /dev/null +++ b/include/configs/mpc5125ads.h ... +/* video */ +#undef CONFIG_VIDEO

Please do not add dead code - do not undef undefined stuff.

+ * Enable Fast boot + */ +#define CONFIG_FASTBOOT

And do not add non-existing #defines either.

... +#define CONFIG_SYS_CS_ALETIMING0x00000005/* Use alternative CS timing for CS0 and CS2 */

Lines too long. Please fix globally.

+/* + * IIM - IC Identification Module + */ +#undef CONFIG_IIM

See above. Don;t add dead code.

+/* I2C */ +#define CONFIG_HARD_I2C/* defd in ads5121 I2C with hardware support */ +#undef CONFIG_SOFT_I2C/* so disable bit-banged I2C */

Ditto.

+#if defined(CONFIG_HARD_I2C) || defined(CONFIG_SOFT_I2C) +#define CONFIG_I2C_MULTI_BUS +#define CONFIG_I2C_CMD_TREE +#define CONFIG_SYS_I2C_SPEED100000/* I2C speed and slave address */ +#define CONFIG_SYS_I2C_SLAVE0x7F +#if 0 +#define CONFIG_SYS_I2C_NOPROBES{{0,0x69}}/* Don't probe these addrs */ +#endif

And again.

+/* #define CONFIG_WATCHDOG *//* enable watchdog */

And again - please fix globally.

+#define CONFIG_SYS_LONGHELP/* undef to save memory */ +#define CONFIG_SYS_LOAD_ADDR0x2000000/* default load address */

Probably too low for most recent kernel versions... And inconsistent with later settings...

Seems this needs a major overhaul.

Best regards,

Wolfgang Denk