[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(®[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