[U-Boot] [PATCH] Add mpc5125ads board and processor to the mpc512x family
Wolfgang Denk
wd at denx.de
Tue Oct 13 12:02:12 CEST 2009
Dear Martha Stan,
In message <1254987657-10103-1-git-send-email-mmarx at silicontkx.com> you wrote:
> Signed-off-by: Martha Stan <mmarx at silicontkx.com>
> ---
> MAINTAINERS | 5 +
> MAKEALL | 1 +
> Makefile | 3 +
> board/freescale/mpc5125ads/Makefile | 53 ++++
> board/freescale/mpc5125ads/config.mk | 23 ++
> board/freescale/mpc5125ads/cpld.h | 50 ++++
> board/freescale/mpc5125ads/mpc5125ads.c | 415 ++++++++++++++++++++++++++
> cpu/mpc512x/asm-offsets.h | 6 +
> cpu/mpc512x/cpu.c | 3 +
> cpu/mpc512x/fixed_sdram.c | 6 +
> cpu/mpc512x/iopin.c | 22 ++
> cpu/mpc512x/serial.c | 17 +-
> cpu/mpc512x/start.S | 15 +
> drivers/net/mpc512x_fec.c | 51 +++-
> include/asm-ppc/immap_512x.h | 285 +++++++++++++++++-
> include/configs/mpc5125ads.h | 486 +++++++++++++++++++++++++++++++
> 16 files changed, 1418 insertions(+), 23 deletions(-)
> create mode 100644 board/freescale/mpc5125ads/Makefile
> create mode 100644 board/freescale/mpc5125ads/config.mk
> create mode 100644 board/freescale/mpc5125ads/cpld.h
> create mode 100644 board/freescale/mpc5125ads/mpc5125ads.c
> create mode 100644 include/configs/mpc5125ads.h
First, I want to point out that I am well aware that a lot of the
trouble with this patch is caused by factors completely out of your
control (like Freescale coming up with such an ugly incompatibility
in the register interface between chips in the same CPU family), and
that you are not the culprit but the victim actually.
However, I think this current version of the patch is still not ripe
for inclusion into mainline - there are still way too many ugly
#ifdef's everywhere.
We need to find a way to work around these register interface issues
more intelligently.
Besides that, there are a few formal issues with your patch:
- there are whitespace errors:
Applying: Add mpc5125ads board and processor to the mpc512x
family
/home/wd/git/u-boot/work/.git/rebase-apply/patch:815: trailing whitespace.
out_be32(&fec->eth->r_cntrl, (FEC_MAX_FRAME_LEN << 16) | 0x124);
warning: 1 line applied after fixing whitespace errors.
- checkpatch.pl reports a number of issues, especially inconsistent
use of white space
> +++ b/board/freescale/mpc5125ads/cpld.h
> @@ -0,0 +1,50 @@
...
> +/*
> + * MPC5125ADS CPLD Registers
> + */
> +typedef struct mpc5125ads_cpld {
> + u16 board_id;
> + u8 cpld_rev[2];
> + u8 reset_cfg_word[4];
> + u8 nor_flash_ctl;
> + u8 nand_can_gpio_ctl;
> + u8 res1[3];
> + u8 int_mask;
> + u8 int_satus;
> + u8 misc_ctl;
> + u8 video_ctl;
> + u8 user_led;
> + u8 res2;
> + u8 cfg_switch;
> +} mpc5125ads_cpld_t;
This is unrelated to this patch - but could we please have such a
struct for the mpc5121ads CPLD registers, too? Thanks in advance.
> +#if defined(CONFIG_HARD_I2C) || defined(CONFIG_SOFT_I2C)
> + /*
> + * DIU init before the driver in linux takes over
> + * Enable the TFP410 Encoder (I2C address 0x38)
> + */
This seems logically wrong to me. Why does the DIU init code depend
on CONFIG_HARD_I2C or CONFIG_SOFT_I2C? Support for the DIU is an
unrelated feature, which should separately be enabled. And if it's
enabled, you must make sure to enable the prerequisites (like I2C
suport), too.
> + i2c_set_bus_num(1);
> + tmp_val = 0xBF;
> + if (i2c_write(0x38, 0x08, 1, &tmp_val, sizeof(tmp_val)) != 0)
> + goto i2c_err;
> +
> + /* Verify if enabled */
> + if (i2c_read(0x38, 0x08, 1, &tmp_val, sizeof(tmp_val)) != 0)
> + goto i2c_err;
> + debug("DVI Encoder Read: 0x%02lx\n", tmp_val);
> + tmp_val = 0x10;
> + if (i2c_write(0x38, 0x0A, 1, &tmp_val, sizeof(tmp_val)) != 0)
> + goto i2c_err;
> +
> + /* Verify if enabled */
> + if (i2c_read(0x38, 0x0A, 1, &tmp_val, sizeof(tmp_val)) != 0)
> + goto i2c_err;
> + debug("DVI Encoder Read: 0x%02lx\n", tmp_val);
> +#endif
> +#ifdef CONFIG_FSL_DIU_FB
> +#if !(defined(CONFIG_VIDEO) || defined(CONFIG_CFB_CONSOLE))
> + mpc5121_diu_init();
> +#endif
> +#endif
Actually it seems we have copies of identical code in
board/freescale/mpc5121ads/mpc5121ads.c and in
board/freescale/mpc8610hpcd/mpc8610hpcd.c already. So instead of
adding a third copy of that code please factor this out into a
separate, common function that can be called by all such boards (this
needs to be done in a separate patch).
> 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?
> 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.
> /* 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.
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.
> 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.
> 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?
> 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:
/* 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"?
> 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.
> @@ -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?
> +#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.
...
> +#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.
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
"We all agree on the necessity of compromise. We just can't agree on
when it's necessary to compromise."
- Larry Wall in <1991Nov13.194420.28091 at netlabs.com>
More information about the U-Boot
mailing list