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"); >> + else >> + sprintf (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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot