Hi Wolfgang,
I had to chuckle when I read your opening paragraph – let me say that I have never thought of myself as a victim – even at the hands of these FSL silicon guys!!! But I did get a pretty raw deal – yes.
Now I am going to respond a little before I go and resubmit.
Very Best Regards, Martha
diff --git a/cpu/mpc512x/asm-offsets.h b/cpu/mpc512x/asm-offsets.h index 4b14778..b79c656 100644 --- a/cpu/mpc512x/asm-offsets.h +++ b/cpu/mpc512x/asm-offsets.h @@ -11,5 +11,11 @@ #define CS_CTRL 0x00020 #define CS_CTRL_ME 0x01000000 /* CS Master Enable bit */
+/* needed for pin muxing in MPC5125 */ +#define IOCTRL_OFFSET 0xA000 +#define IOCTRL_LPC_AX03 0x09 +#define IOCTRL_I2C1_SCL 0x4f +#define IOCTRL_I2C1_SDA 0x50
We should avoid adding stuff to asm-offsets.h; instead, we should finally auto-generate this file from the respective C structs as it's done in Linux. Do you dare to tackle this?
I can't do this any time soon Wolfgang … I work part-time now (post wedding issues to take care of).
diff --git a/cpu/mpc512x/fixed_sdram.c b/cpu/mpc512x/fixed_sdram.c
index 673d61e..09985e7 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 /* makes it unnecessary to declare these */ 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 };
NAK. Please don't add #ifdef's here. This is the default init sequence, and if it does not fit your purposes, then please use a private one.
Yes but the default has constants like CONFIG_SYS_MICRON_INIT_DEV_OP … must I then declare this if I am using CONFIG_SYS_ELPIDA_INIT_DEV_OP ? The default constants are a large mem array that just plain doesn't need to be there if you must override it anyway. I don't understand the impetus to save on printf strings, for example, and not wanting to save here ???
/* Initialize IO Control */+#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
This is something which happens a lot in the remaining code - so often that it is plain unacceptable. As mentioned above, I know that you are just a victim here, but we need a less ugly implementation.
Actually – since I redid the entire iopin_initialize function to a separate one for the mpc5125 this is the only place where an ugly #ifdef'ed iopin init occurs now.
If I understand correctly, all MPC512x actually use 8 bit only here, even though the register declarations on MPC5121/5123 are 32 bit wide. Eventually we should do what Grant Likely already did in the Linux kernel and change all thjis code to use 8 bit accesses only, which means to add the needed padding to the respective data stricts
- similar to what was done to make the 16550 serial driver really
generic. However, this is a pretty invasive operation, that will have to wait for the next release in any case. And actually I would like to delay this until at least an official Reference Manual for the MPC5125 has been publicly released by Freescale.
As I said – since I redid the iopin_initialize (there are now 2 different functions) I don't think this is necessary … it's not just a size difference … there is also a bit configuration difference. I redid the #define for this too. Also .. the elements within the struct are all different.
diff --git a/cpu/mpc512x/serial.c b/cpu/mpc512x/serial.c
index 4fc4693..89fa139 100644 --- a/cpu/mpc512x/serial.c +++ b/cpu/mpc512x/serial.c @@ -80,7 +80,7 @@ int serial_init(void) volatile psc512x_t *psc = (psc512x_t *) &im->psc[CONFIG_PSC_CONSOLE];
fifo_init (psc);
+#ifndef CONFIG_MPC5125 /* set MR register to point to MR1 */ out_8(&psc->command, PSC_SEL_MODE_REG_1);
@@ -93,12 +93,25 @@ int serial_init(void) /* switch to UART mode */ out_be32(&psc->sicr, 0);
/* mode register points to mr1 */ /* configure parity, bit length and so on in mode register 1*/
/* mode register points to mr1 */ out_8(&psc->mode, PSC_MODE_8_BITS | PSC_MODE_PARNONE); /* now, mode register points to mr2 */ out_8(&psc->mode, PSC_MODE_1_STOPBIT);+#else
/* disable Tx/Rx */out_8(&psc->command, PSC_TX_DISABLE | PSC_RX_DISABLE);/* choose the prescaler the Tx/Rx clock generation */out_8(&psc->psc_clock_select, 0xdd);/* switch to UART mode */out_be32(&psc->sicr, 0);/* configure parity, bit length and so on in mode registers */out_8(&psc->mr1, PSC_MODE_8_BITS | PSC_MODE_PARNONE);out_8(&psc->mr2, PSC_MODE_1_STOPBIT);+#endif /* set baudrate */ serial_setbrg();
I think we should move the differing code into separate functions.
Fine – but I'll have to check the processor type to see which one to call at some point if I take out the ifdefs ???
diff --git a/drivers/net/mpc512x_fec.c b/drivers/net/mpc512x_fec.c
index fb2c19a..9f839a1 100644 --- a/drivers/net/mpc512x_fec.c +++ b/drivers/net/mpc512x_fec.c @@ -41,7 +41,12 @@ 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
Can we not do without this #ifdef here?
I'm open to suggestions … there are possibly two active FECS and this seemed to be the best solution … The MPC5125 code will work for the MPC5121e so I suppose I could take out the ifdefs in lieu of doing this check each time. Are you against this extra check for non-5125 boards ?
u8 reg_mask[] = {/* regs to print: 0...8, 21,27,31 */ 1, 1, 1, 1, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0,@@ -242,9 +247,17 @@ static int mpc512x_fec_init (struct eth_device *dev, bd_t * bis) /* Set Opcode/Pause Duration Register */ out_be32(&fec->eth->op_pause, 0x00010020);
+#ifdef CONFIG_MPC5125
/* RMII Mode */if (dev->name[3] == '2')out_be32(&fec->eth->r_cntrl, (FEC_MAX_FRAME_LEN << 16) | 0x124);else/* Frame length=1522; MII mode */out_be32(&fec->eth->r_cntrl, (FEC_MAX_FRAME_LEN << 16) | 0x24);+#else /* Frame length=1522; MII mode */ out_be32(&fec->eth->r_cntrl, (FEC_MAX_FRAME_LEN << 16) | 0x24);
Ditto. There is a lot of code duplication here. How about something similar to this:
OK – will do
/* Frame length=1522; MII mode */
val = (FEC_MAX_FRAME_LEN << 16) | 0x24;
if (dev->name[3] == '2')
val |= 0x100; /* RMII Mode */
out_be32(&fec->eth->r_cntrl, val);
etc. More simplifications like this should be applied elsewhere. Please rework globally.
sprintf (dev->name, "FEC ETHERNET");
if (cur_fec == &(im->fec))sprintf (dev->name, "FEC ETHERNET");elsesprintf (dev->name, "FEC2 ETHERNET");Maybe sprintf (dev->name, "FEC%s ETHERNET", (cur_fec == &(im->fec)) ? "" : "2"); or similar?
+#ifdef CONFIG_MPC5125
/* 2nd fec may not be in use */if (cur_fec == &(im->fec) &&(in_be32(&im->clk.sccr[0]) & CLOCK_SCCR1_FEC2_EN)) {cur_fec = &(im->fec2);goto fec_init_start;}+#endif
What do you mean by "2nd fec may not be in use"?
I will consolidate my printf strings .. yes. As far as the Clock on the 2nd FEC – I check it to see if it's ON – Only the 1st FEC is necessary in u-boot while the 2nd is optional. The board code – in my board's case – either has the DIU or a 2nd FEC and a 2nd USB .. I assume other boards may or may not use the 2nd fec. I am planning a README for the board to explain these things … Speaking of README … do you think one should be done for the CPU family now that this processor has changed things a little ?
diff --git a/include/asm-ppc/immap_512x.h b/include/asm-ppc/immap_512x.h
index 79cdd80..1c1d235 100644 --- a/include/asm-ppc/immap_512x.h +++ b/include/asm-ppc/immap_512x.h
...
+#ifndef CONFIG_MPC5125 /*
- PSC
- MPC5121 PSC
- note: MPC5121e register overloading is handled by unions with #defines to
- reference the reg seemlessly these #defines must not exist for MPC5125 code
- since it does not have this overloading. Since the register naming is the
- same as the MPC5125 Reference Manual and this naming is exactly the reg names
- used in the init code (which is nearly identical) it causes compile errors to
- leave in and must be #ifdef'ed out. It also helps to share code to have the
*/
- same structure for both MPC5121 and MPC5125
I disagree. To me the code becomes pretty much unreadable. I think we need to find a better implementation for this.
Look Wolfgang … It's very readable – with a 5121 type processor it's one struct and with a 5125 it's the other definition … I took out all the register by register #ifdefs and now there is only one... and it's very appropriate that it's like this considering the silicon differences. All the regs are the same names but in different places – the beauty of a struct hides this nicely. The same code works for both because all the Freescale guys did was to give the registers more room – Everything is functionally the same.
@@ -1200,23 +1445,39 @@ typedef struct immap {
fec512x_t fec; /* Fast Ethernet Controller */ ulpi512x_t ulpi; /* USB ULPI */ u8 res4[0xa00];+#ifdef CONFIG_MPC5125
ulpi512x_t ulpi2; /* USB ULPI */u8 res5[0x200];fec512x_t fec2; /* 2nd Fast Eth Controller */gpt512x_t gpt2; /* 2nd General Purpose Timer */sdhc512x_t sdhc2; /* 2nd SDHC */u8 res6[0x3e00];ddr512x_t mddrc; /* DDR Memory Controller */ioctrl5125_t io_ctrl; /* IO Control */+#else utmi512x_t utmi; /* USB UTMI */ u8 res5[0x1000]; pcidma512x_t pci_dma; /* PCI DMA */ pciconf512x_t pci_conf; /* PCI Configuration */ u8 res6[0x80]; ios512x_t ios; /* PCI Sequencer */
pcictrl512x_t pci_ctrl; /* PCI Controller Control and Status */
pcictrl512x_t pci_ctrl; /* PCI Controller Control */ u8 res7[0xa00]; ddr512x_t mddrc; /* Multi-port DDR Memory Controller */ ioctrl512x_t io_ctrl; /* IO Control */+#endif
Is there any reason for having identical entries (mddrc, io_ctrl) in both branches? Whyno factor these out, too?
They differ from the utmi/ulpi thru ioctrl .. I put the ioctrl in a different struct like you asked ,i.e. ioctrl5125_t
+#ifdef CONFIG_MPC5125
psc512x_t psc[10]; /* PSCs */u8 res10[0x500];+#else psc512x_t psc[12]; /* PSCs */ u8 res10[0x300]; +#endif
This should be handled by #defining constants and without an #ifdef here.
I disagree … I think I'd rather see these most basic Memory map differences … it's almost as good as reading the manual sometimes … And I think it would just obfuscate things esp when there are 2 constants -- the PSC array size and the reserve array size -- also -- none of the other reserve arrays in the memory map have constants. ...
+#include <config_cmd_default.h>
+#define CONFIG_CMD_ASKENV +#define CONFIG_CMD_DHCP +#define CONFIG_CMD_MII +#define CONFIG_CMD_NFS +#define CONFIG_CMD_PING +#define CONFIG_CMD_REGINFO +#define CONFIG_CMD_FUSE
+#if defined(CONFIG_HARD_I2C) || defined(CONFIG_SOFT_I2C) +#define CONFIG_CMD_EEPROM +#define CONFIG_CMD_DATE +#define CONFIG_CMD_I2C +#endif
You may want to keep such lists sorted.
Will do