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

2009-09-21 Thread Fabio Estevam 
Hi Martha,

--- On Mon, 9/21/09, Martha M Stan  wrote:

> From: Martha M Stan 
> Subject: [U-Boot] [PATCH] Add mpc5125ads board and processor to the mpc512x 
> family
> To: u-boot@lists.denx.de
> Cc: "Martha M Stan" 
> Date: Monday, September 21, 2009, 5:27 PM
> Signed-off-by: Martha M Stan 
>

You missed MAKEALL and MAINTAINERS entries.

Regards,

Fabio Estevam


  
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2009-10-03 Thread Wolfgang Denk 
Dear Martha M Stan,

do you plan to send an updated patch for this release?

In addition to Fabio's comments here a few more:

In message <12535648622828-git-send-email-mm...@silicontkx.com> you wrote:
>
...
> diff --git a/board/freescale/mpc5125ads/mpc5125ads.c 
> b/board/freescale/mpc5125ads/mpc5125ads.c
> new file mode 100644
> index 000..445c765
> --- /dev/null
> +++ b/board/freescale/mpc5125ads/mpc5125ads.c
...
> + if (in_8((u8 *)(CONFIG_SYS_CPLD_BASE + 0x08)) & 0x04) {
> + out_8((u8 *)(CONFIG_SYS_CPLD_BASE + 0x08), 0xc1);
> + } else {
> + /* running from Backup flash */
> + out_8((u8 *)(CONFIG_SYS_CPLD_BASE + 0x08), 0x32);
> + }

Please do not use base address plus offset, but define a C struct to
describe the CPLD's register layout.

[That would also be most welcome to fix similar code in
"board/freescale/mpc5121ads/mpc5121ads.c"; sorry - due to lack of
documentation I was not able to clean this up yet.]

> +#endif
> + /* 
> +  * initialize function mux & slew rate for all IO pins
> +  * there are two peripheral options controlled by switch 8 
> +  */
> +if (IS_CFG1_SWITCH) {

Indentation not by TAB.

> +#ifdef CONFIG_MISC_INIT_R
> +int misc_init_r(void)
> +{
> + u8 tmp_val;
> +
> + extern int ads5121_diu_init(void);
> +
> +   immap_t *im = (immap_t *) CONFIG_SYS_IMMR;
> +   u32 bkup_size = flash_info[0].size;
> +   u32 bkup_start = 0x - bkup_size +1;

Indentation not by TAB. Please fix globally.

> + /* Verify if enabled */
> + tmp_val = 0;
> + i2c_read(0x38, 0x08, 1, &tmp_val, sizeof(tmp_val));
...
> + tmp_val = 0;
> + i2c_read(0x38, 0x0A, 1, &tmp_val, sizeof(tmp_val));

Which sense does it make to set tmp_val when you overwrite the value
immediately by running i2c_read()?

And would it be not a good idea to check i2c_read() / i2c_write()
return codes?

> +int checkboard (void)
> +{
> + ushort brd_rev = *(vu_short *) (CONFIG_SYS_CPLD_BASE + 0x00);
> + uchar cpld_rev = *(vu_char *) (CONFIG_SYS_CPLD_BASE + 0x02);
> + uchar cpld_min_rev = *(vu_char *) (CONFIG_SYS_CPLD_BASE + 0x03);

See above - please use a C struct to describe the CPLD layout.

> + if (IS_CFG1_SWITCH) /* CAN1+2, SDHC1, USB1+2, FEC1+2, I2C1+2*/
> + printf("Peripheral Option Set 1\n");
> + else/* CAN1+2, SDHC1, DIU, USB1, FEC1, I2C1+3*/
> + printf("Peripheral Option Set 0\n");

Don't repeat long strings without need. How about:

printf("Peripheral Option Set %d\n", IS_CFG1_SWITCH != 0);

Hm... I wonder what the output format will look like. I guess there
might be vertical alignment issues?

> diff --git a/board/freescale/mpc5125ads/u-boot.lds 
> b/board/freescale/mpc5125ads/u-boot.lds
> new file mode 100644
> index 000..f2f6e14
> --- /dev/null
> +++ b/board/freescale/mpc5125ads/u-boot.lds

Do you really need a private linker script? Why?

> diff --git a/cpu/mpc512x/fixed_sdram.c b/cpu/mpc512x/fixed_sdram.c
> index 63a3035..12ac44d 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
>   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

Isn't there way to do without such #ifdef's in common code?

>   /* Initialize IO Control */
> - out_be32(&im->io_ctrl.io_control_mem, IOCTRL_MUX_DDR);
> +#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

Ditto here.

> diff --git a/cpu/mpc512x/iopin.c b/cpu/mpc512x/iopin.c
> index be20947..ab5dd1c 100644
> --- a/cpu/mpc512x/iopin.c
> +++ b/cpu/mpc512x/iopin.c
> @@ -28,15 +28,27 @@
>  void iopin_initialize(iopin_t *ioregs_init, int len)
>  {
>   short i, j, p;
> - u32 *reg;
>   immap_t *im = (immap_t *)CONFIG_SYS_IMMR;
>  
> - reg = (u32 *)&(im->io_ctrl);
> +#ifdef CONFIG_MPC5125
> + u8 *reg =(u8 *)&(im->io_ctrl);
> +#else
> + u32 *reg =(u32 *)&(im->io_ctrl);
> +#endif

And here again.

This is becoming an #ifdef mess, it seems. Usually this is an
indiaction of a major design issue.

> +#ifdef CONFIG_MPC5125
> + for (p = 0, j = ioregs_init[i].p_offset;
> + p < ioregs_init[i].nr_pins; p++, j++) {
> + if (ioregs_init[i].bit_or)
> + setbits_8(&(reg[j]), ioregs_init[i].val);
> + else
> + out_8(®[j], ioregs_init[i].val);
> + }
> +#else
>   for (p = 0, j = ioregs_init[i].p_offset / sizeof(u_long);
>   p < ioregs_init[i].nr_pins; p

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

2009-10-04 Thread m marx 
Hello Wolfgang,

I am planning to resubmit this patch very soon - by Tuesday perhaps.  
Considering there is some differences in the memory layout esp for the iopin 
struct (going from 32-bit to byte sizes)  I am not sure how I can get around 
the #defs you suggest.  Perhaps an entirely separate .c file for this part ??  

Much of what you pointed to was copied over from the mpc5121ads ... so I didn't 
pay that much attention.  I will fix these too and also fix the 5121 code later.

When do you need it so I can give you enough time to look it over?

Thanks Wolfgang,
Martha


-Original Message-
From: Wolfgang Denk w...@denx.de
Sent 10/3/2009 6:43:51 PM
To: Martha M Stan mm...@silicontkx.com
Cc: u-boot@lists.denx.de
Subject: Re: [U-Boot] [PATCH] Add mpc5125ads board and processor to the mpc512x 
family

Dear Martha M Stan,

do you plan to send an updated patch for this release?

In addition to Fabio's comments here a few more:

In message 12535648622828-git-send-email-mm...@silicontkx.com you wrote:

...
 diff --git a/board/freescale/mpc5125ads/mpc5125ads.c 
b/board/freescale/mpc5125ads/mpc5125ads.c
 new file mode 100644
 index 000..445c765
 --- /dev/null
 +++ b/board/freescale/mpc5125ads/mpc5125ads.c
...
 +if (in_8((u8 *)(CONFIG_SYS_CPLD_BASE + 0x08)) & 0x04) {
 +out_8((u8 *)(CONFIG_SYS_CPLD_BASE + 0x08), 0xc1);
 +} else {
 +/* running from Backup flash */
 +out_8((u8 *)(CONFIG_SYS_CPLD_BASE + 0x08), 0x32);
 +}

Please do not use base address plus offset, but define a C struct to
describe the CPLD's register layout.

[That would also be most welcome to fix similar code in
"board/freescale/mpc5121ads/mpc5121ads.c"; sorry - due to lack of
documentation I was not able to clean this up yet.]

 +#endif
 +/* 
 + * initialize function mux & slew rate for all IO pins
 + * there are two peripheral options controlled by switch 8 
 + */
 + if (IS_CFG1_SWITCH) {

Indentation not by TAB.

 +#ifdef CONFIG_MISC_INIT_R
 +int misc_init_r(void)
 +{
 +u8 tmp_val;
 +
 +extern int ads5121_diu_init(void);
 +
 + immap_t *im = (immap_t *) CONFIG_SYS_IMMR;
 + u32 bkup_size = flash_info[0].size;
 + u32 bkup_start = 0x - bkup_size +1;

Indentation not by TAB. Please fix globally.

 +/* Verify if enabled */
 +tmp_val = 0;
 +i2c_read(0x38, 0x08, 1, &tmp_val, sizeof(tmp_val));
...
 +tmp_val = 0;
 +i2c_read(0x38, 0x0A, 1, &tmp_val, sizeof(tmp_val));

Which sense does it make to set tmp_val when you overwrite the value
immediately by running i2c_read()?

And would it be not a good idea to check i2c_read() / i2c_write()
return codes?

 +int checkboard (void)
 +{
 +ushort brd_rev = *(vu_short *) (CONFIG_SYS_CPLD_BASE + 0x00);
 +uchar cpld_rev = *(vu_char *) (CONFIG_SYS_CPLD_BASE + 0x02);
 +uchar cpld_min_rev = *(vu_char *) (CONFIG_SYS_CPLD_BASE + 0x03);

See above - please use a C struct to describe the CPLD layout.

 +if (IS_CFG1_SWITCH) /* CAN1+2, SDHC1, USB1+2, FEC1+2, I2C1+2*/
 +printf("Peripheral Option Set 1\n");
 +else/* CAN1+2, SDHC1, DIU, USB1, FEC1, I2C1+3*/
 +printf("Peripheral Option Set 0\n");

Don't repeat long strings without need. How about:

printf("Peripheral Option Set %d\n", IS_CFG1_SWITCH != 0);

Hm... I wonder what the output format will look like. I guess there
might be vertical alignment issues?

 diff --git a/board/freescale/mpc5125ads/u-boot.lds 
b/board/freescale/mpc5125ads/u-boot.lds
 new file mode 100644
 index 000..f2f6e14
 --- /dev/null
 +++ b/board/freescale/mpc5125ads/u-boot.lds

Do you really need a private linker script? Why?

 diff --git a/cpu/mpc512x/fixed_sdram.c b/cpu/mpc512x/fixed_sdram.c
 index 63a3035..12ac44d 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
 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

Isn't there way to do without such #ifdef's in common code?

 /* Initialize IO Control */
 -out_be32(io_ctrl.io_control_mem, IOCTRL_MUX_DDR);
 +#ifdef CONFIG_MPC5125
 +out_8(io_ctrl.io_control_mem, IOCTRL_MUX_DDR);
 +#else
 + out_be32(io_ctrl.io_control_mem, IOCTRL_MUX_DDR);
 +#endif

Ditto here.

 diff --git a/cpu/mpc512x/iopin.c b/cpu/mpc512x/iopin.c
 index be20947..ab5dd1c 100644
 --- a/cpu/mpc512x/iopin.c
 +++ b/cpu/mpc512x/iopin.c
 @@ -28,15 +28,27 @@
 void iopin_initialize(iopin_t *ioregs_init, int len)
 {
 short i, j, p;
 -u32 *reg;
 immap_t *im = (immap_t *)CONFIG_SYS_IMMR;

 -reg = (u32 *)&(im-io_ctrl);
 +#ifdef CONFIG_MPC5125
 +u8 *reg =(u8 *)&(im-io_ctrl);
 +#else
 +u32 *reg =(u32 *)&(im-io_ctrl);
 +#endif

And here again.

This is becoming an #ifdef mess, it seems. Usually this is an
indiaction of a major design issue.

 +#ifdef CONFIG_MPC5125
 +for (p = 0,

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

2009-10-04 Thread Wolfgang Denk 
Dear Martha,

umm... your mail header still says "m marx".

In message <20091004102089.sm01...@[206.180.163.89]> you wrote:
> 
> I am planning to resubmit this patch very soon - by Tuesday perhaps.
> Considering there is some differences in the memory layout esp for the
> iopin struct (going from 32-bit to byte sizes)  I am not sure how I can
> get around the #defs you suggest.  Perhaps an entirely separate .c file
> for this part ??  

This depends on the situation. If the struct layout is absolutely
identical, just with different datatypes, we might use a #defined type
in the sruct. If not (which seems to be the case almost everywhere
where I looked), then we have to use at least different structs.

> Much of what you pointed to was copied over from the mpc5121ads ... so I
> didn't pay that much attention.  I will fix these too and also fix the
> 5121 code later.

Not paying much attention is always a bad thing which adds avoidable
efforts.

> When do you need it so I can give you enough time to look it over?

It depends. If you want to see this go in for the current release, we
should have clean and stable code in a week or two. Of head for the
next release, then ther eis no hurry at all.

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: w...@denx.de
Unix: Some say the learning curve is steep,  but  you  only  have  to
climb it once.  - Karl Lehenbauer
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2009-10-13 Thread Wolfgang Denk 
Dear Martha Stan,

In message <1254987657-10103-1-git-send-email-mm...@silicontkx.com> you wrote:
> Signed-off-by: Martha Stan 
> ---
>  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   0x01000

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

2009-10-13 Thread m stan 
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   0x0100  /* CS Master Enable bit */
>>  
>> +/* needed for pin muxing in MPC5125 */
>> +#define IOCTRL_OFFSET0xA000
>> +#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->sic

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

2009-10-18 Thread Wolfgang Denk 
Dear Martha,

In message <200910131345510.sm02...@[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)
> >>  {
> >>

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

2009-10-19 Thread John Rigby 
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: w...@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
>
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2009-10-20 Thread m stan 
> > 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.

How do I override a default array of 30 ulong elements that is declared 
static in common code from my board code ?  I declare my own and use it 
exclusively so the one in common code is a waste of space.  Besides which
it puts constraints that all the elements need a declaration.  Why if I'm not 
using it ?

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

If they had been left as an array with indices as when the 5121 
was first implemented these differences would be trivial ... now 
that each iopin is an elemnet in a struct there are over 50 different 
namesbesides the ordering by the 3rd element is off for the few 
names at the beginning that are the same.
Also the init code still treats it like an array.  I believe you 
NAKed me on doing this a week or two ago.
Perhaps I should take it back to that implementation ???

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

I've read and reread the manual Wolfgang ... I could recite page numbers 
at this point ... I believe you 've glanced at it and have the wrong impression.
> 
> ...
> > >> +#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.

Are you talking about pulling one or the other in at the make ?
SO to share the 8 functions or so like putc and getc I have one file, 
5121 init another and 5125 init a third -- all that to bypass an if statement 
?? 
You're not making any sense to me.
> 
> > > diff --git a/drivers/net/mpc512x_fec.c b/drivers/net/mpc512x_fec.c
> > >> index fb2c19a..9f839a1 100644
> > >> --- a/drivers/n

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

2009-10-20 Thread Wolfgang Denk 
Dear Martha,

can you _please_ fix your mailer and make sure to keep the In-reply-to:
and References: mail headers in place and working, so threading of
messages works?  Thanks.


In message <200910200953803.sm05...@[206.180.163.89]> you wrote:
>
> > 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.
>
> How do I override a default array of 30 ulong elements that is declared 
> static in common code from my board code ?  I declare my own and use it 
> exclusively so the one in common code is a waste of space.  Besides which
> it puts constraints that all the elements need a declaration.  Why if I'm not 
> using it ?

If the assumption that the default code is useful on all boards is
wrong, it has the be revised. We can implement this is a way that it
can be completel enabled/ disabled, but I will not accept any
#ifdef's right in the middle of that array init code.

> > No. You can build either one implementation of the function or the
> > other one, depending of the config settings.
>
> Are you talking about pulling one or the other in at the make ?
> SO to share the 8 functions or so like putc and getc I have one file, 
> 5121 init another and 5125 init a third -- all that to bypass an if statement 
> ?? 

Why compile code for two incompatible architectures when you know at
compile time which one you have? You don't attempt to have one U-Boot
binary image that runs both on a MPC5121 and on a MPC5125 board, or do
you? [I would be interested to see how this could be done in a halfway
clean way.] Whether you split this in several files or not is a
different question.

> You're not making any sense to me.

I'm sorry to hear that.

> > 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.
>
> Can't I initialize both and switch between them ?  I do it now and
> want to keep this functionality.

No, you cannot do that. You must not initialize _any_ network
interface unless U-Boot issues a network related command. And when
switching to another interface, you have to disable the previouly used
one. Same before you boot Linux.

> > 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.
>
> Yes but since you can switch midstream they both need to be initialized 
> and the code needs to determine which one it's using ???

You cannot switch "midstream". You can use a  new  network  interface
with  a new network command - and there it does not make a difference
whether you ran a command on another interface  before  (except  that
you  have  to make sure to disable such other interface first). There
is no and cannot be any switching while  any  command  (say,  a  TFTP
download) is running.

Initialization must not be done before actual use, and then only  for
the network interface that is actually going to be used.

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: w...@denx.de
The human race is faced with a cruel choice: work  or  daytime  tele-
vision.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2009-10-20 Thread Wolfgang Denk 
Dear John,

In message <4b73d43f0910191545x3127cba5w7fdec3f638213...@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

-- 
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: w...@denx.de
"Most people would like to be delivered  from  temptation  but  would
like it to keep in touch." - Robert Orben
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2009-10-21 Thread Kenneth Johansson 
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)



___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot