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-mm...@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(®[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: w...@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_
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
