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

m marx mmarx at silicontkx.com
Sun Oct 4 16:20:15 CEST 2009


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 at denx.de
Sent 10/3/2009 6:43:51 PM
To: Martha M Stan mmarx at silicontkx.com
Cc: u-boot at 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 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(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

-- 
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