[U-Boot] [PATCH] Add mpc5125ads board and processor to the mpc512x family

[Wolfgang Denk]

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 .. either
> has 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

-- 
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
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