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

Wolfgang Denk wd at denx.de
Sun Oct 4 00:43:51 CEST 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 at 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 at 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_


More information about the U-Boot mailing list