Re: [U-Boot] [PATCH] Add mpc5125ads board and processor to the mpc512x family
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
Dear Martha,
In message <200910131345510.SM02960@[206.180.163.89]> you wrote:
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
I was afraid you'd say that...
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 ?
Well, the ideas was that this was the default setting that fits most boards, and if it doesn't fit it will be overwritten by another array. Adding #ifdef's here is a strict No-No.
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 ???
Feel free to implement something that needs less memory, but do not add #ifdef's here.
+#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.
I think it's not the only place, and it's just a symptom of the problem. I think we should try to avoid duplicating structs that are more or less the same, except for the data type.
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.
It's primarily and issue of being able to read and maintain the code. Duplicating the structs makes no sense if they are essentially the same except for the u8 versus u32 difference.
You claim the elements are all different? I didn't get this impression from reading either your code or the RefMan.
...
+#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 ???
No. You can build either one implementation of the function or the other one, depending of the config settings.
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
I'm not sure what you mean. In U-Boot, there cannot be 'two active FECs'; at any point of time, no more than one network interface will be enabled - if any at all.
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 ?
No, but I don't want so many #ifdef's in common code.
+#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.
I'm not talking about any printed messages here. I smell a basic misunderstaning in your comments - there cannot be more than one network interface active and in use in U-Boot.
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. ...
Umm... Why should we care about this? See bullet # 2 at http://www.denx.de/wiki/view/U-Boot/DesignPrinciples#2_Keep_it_Fast
U-Boot should not care about these things, unless you run a network command on the second Ethernet interface.
... The board code .. in my board's case .. eitherhas the DIU or a 2nd FEC and a 2nd USB .. I assume other boards may or may
In which way should this be a concern to U-Boot, unless it attempts to use any of these?
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 ?
Well, maybe we can wait until the MPC5125 sees an official announcement.
- 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
You definition of readable does not match mine. I nak this code.
Best regards,
Wolfgang Denk
Regarding 512x psc register maps:
The register map for 5125 does not just change the size of the registers. Some registers change locations. The issue is that the hardware guys decided to "fix" the old broken register access. The 5200, 5121, 5123 had some registers that were:
Function A when read but function B when written.
For this case the old register is replaced by two read/write registers. So the index of all subsequent registers is incremented.
Function A on first access after writing to another register and Function B on subsequent accesses.
This was also fixed by replacing the statefull register by two registers.
So the problem is painful but I believe doable. The problem I never resolved was dealing with this mess in linux where the same binary has to work with both platforms. I decided that the register accesses needed to be done via an offsets array that was populated at run time but I never got around to implementing that.
John
- 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
You definition of readable does not match mine. I nak this code.
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@denx.de There is a time in the tides of men, Which, taken at its flood, leads on to success. On the other hand, don't count on it. - T. K. Lawson _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Dear John,
In message 4b73d43f0910191545x3127cba5w7fdec3f6382138e3@mail.gmail.com you wrote:
The register map for 5125 does not just change the size of the registers. Some registers change locations. The issue is that the hardware guys decided to "fix" the old broken register access. The 5200, 5121, 5123 had some registers that were:
I always stand fascinated about the inventiveness of these guys; even when just releasing a new chip from one family where one would expect basicly upward-compatibility they find ways not to simplify the design but to make it more complex and wonderful. Nobody else does so much to save our jobs.
So the problem is painful but I believe doable. The problem I never resolved was dealing with this mess in linux where the same binary has to work with both platforms. I decided that the register accesses needed to be done via an offsets array that was populated at run time but I never got around to implementing that.
Heh. I don't envy the guy who has to do this.
Best regards,
Wolfgang Denk
On Mon, 2009-10-19 at 16:45 -0600, John Rigby wrote:
Regarding 512x psc register maps:
The register map for 5125 does not just change the size of the registers. Some registers change locations. The issue is that the hardware guys decided to "fix" the old broken register access. The 5200, 5121, 5123 had some registers that were:
Function A when read but function B when written.
I would like to hear what the designer had for argument to justify that solution. Another favorite is the write only register.
GPIO without set/reset registers that force a read/modify/write cycle.
Then we have things that are more bugs than design
The "god" register that is not affected by any reset turning a reboot/reset into a game of chance.
The occasional write register. do { *reg =xx }while (*reg != xx)
participants (4)
-
John Rigby -
Kenneth Johansson -
m stan -
Wolfgang Denk