Re: [U-Boot] [PATCH] Add mpc5125ads board and processor to the mpc512x family

4 Oct 2009

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(&im->io_ctrl.io_control_mem, IOCTRL_MUX_DDR);

+#ifdef CONFIG_MPC5125

out_8(&im->io_ctrl.io_control_mem, IOCTRL_MUX_DDR);

+#else

out_be32(&im->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_OFFSET		EXC_OFF_SYS_RESET
#define SPR_5121E		0x80180000
+#define SPR_5125		0x80190000



Please don't add random white space.
...
/*

IMMRBAR - Internal Memory Register Base Address

@@ -210,21 +212,25 @@ typedef struct clk512x {
 #define CLOCK_SCCR1_TPR_EN		0x00001000
 #define CLOCK_SCCR1_PCI_EN		0x00000800
 #define CLOCK_SCCR1_DDR_EN		0x00000400
+#define CLOCK_SCCR1_FEC2_EN		0x00000200
/* System Clock Control Register 2 commands */
 #define CLOCK_SCCR2_DIU_EN		0x80000000
 #define CLOCK_SCCR2_AXE_EN		0x40000000
 #define CLOCK_SCCR2_MEM_EN		0x20000000
-#define CLOCK_SCCR2_USB2_EN		0x10000000
-#define CLOCK_SCCR2_USB1_EN		0x08000000
+#define CLOCK_SCCR2_USB1_EN		0x10000000
+#define CLOCK_SCCR2_USB2_EN		0x08000000
Is this a bug fix to existing code? This should be a separate patch,
then.
...
#define CLOCK_SCCR2_BDLC_EN		0x02000000
+#define CLOCK_SCCR2_AUTO_EN		0x02000000
 #define CLOCK_SCCR2_SDHC_EN		0x01000000
+#define CLOCK_SCCR2_AUTO_EN		0x02000000
Why are you adding CLOCK_SCCR2_AUTO_EN twice?
...
typedef struct ioctrl512x {

u32	io_control_mem;			/* MEM pad ctrl reg */
u32	io_control_gp;			/* GP pad ctrl reg */
u32	io_control_lpc_clk;		/* LPC_CLK pad ctrl reg */
u32	io_control_lpc_oe;		/* LPC_OE pad ctrl reg */
u32	io_control_lpc_rw;		/* LPC_R/W pad ctrl reg */
u32	io_control_lpc_ack;		/* LPC_ACK pad ctrl reg */
u32	io_control_lpc_cs0;		/* LPC_CS0 pad ctrl reg */
u32	io_control_nfc_ce0;		/* NFC_CE0 pad ctrl reg */
u32	io_control_lpc_cs1;		/* LPC_CS1 pad ctrl reg */
u32	io_control_lpc_cs2;		/* LPC_CS2 pad ctrl reg */
u32	io_control_lpc_ax03;		/* LPC_AX03 pad ctrl reg */
u32	io_control_emb_ax02;		/* EMB_AX02 pad ctrl reg */
u32	io_control_emb_ax01;		/* EMB_AX01 pad ctrl reg */
u32	io_control_emb_ax00;		/* EMB_AX00 pad ctrl reg */
u32	io_control_emb_ad31;		/* EMB_AD31 pad ctrl reg */

...
...

u32	io_control_usb_phy_drvvbus;	/* USB2_DRVVBUS pad ctrl reg */
u8	reserved[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 	*/
u8	res1[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 u8	mr1;		/* PSC + 0x00 */
volatile u8	res0[3];
volatile u8	mr2;		/* PSC + 0x04 */
volatile u8	res0a[3];
volatile u16	psc_status;	/* PSC + 0x08 */
volatile u16	res1;
volatile u16	psc_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 u16	res1a;
volatile u8	command;	/* PSC + 0x10 */
volatile u8	res2[3];
union {				/* PSC + 0x14 */
volatile u8	buffer_8;


volatile u16	buffer_16;


volatile u32	buffer_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_ALETIMING		0x00000005	/* 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_SPEED		100000	/* I2C speed and slave address */
+#define CONFIG_SYS_I2C_SLAVE		0x7F
+#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_ADDR	0x2000000	/* 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
-- 
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@denx.de
If programming was easy, they wouldn't need something as  complicated
as a human being to do it, now would they?
                       - L. Wall & R. L. Schwartz, _Programming Perl_

    