[U-Boot] [PATCH] Add mpc5125ads board and processor to the mpc512x family
m stan
mmarx at silicontkx.com
Tue Oct 13 19:45:11 CEST 2009
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
More information about the U-Boot
mailing list