[U-Boot-Users] [PATCH] mpc85xx: fix upmconfig

[U-Boot-Users] [PATCH] mpc85xx: fix upmconfig

[Sebastian Siewior]

This actually shouldn't work. Imagina 0xf0000000 base address that
gets translated into 0x1e000 and causes my box to hang. Writing
to 0xf0000000 seems the better way.
Also don't compare against the UPM mask but agaist the MSEL mask.

Cc: Sergei Poselenov <sposelenov@emcraft.com>
Cc: Andy Fleming <afleming@freescale.com>
Signed-off-by: Sebastian Siewior <bigeasy@linutronix.de>
---
 cpu/mpc85xx/cpu.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/cpu/mpc85xx/cpu.c b/cpu/mpc85xx/cpu.c
index 0497422..2373b4a 100644
--- a/cpu/mpc85xx/cpu.c
+++ b/cpu/mpc85xx/cpu.c
@@ -71,8 +71,7 @@ struct cpu_type *identify_cpu(u32 ver)
 
 static void set_lcb_clock(uint clkdiv)
 {
-	volatile immap_t *immap = (immap_t *)CFG_IMMR;
-	volatile ccsr_lbc_t *lbc= &immap->im_lbc;
+	volatile ccsr_lbc_t *lbc = (void *)(CFG_MPC85xx_LBC_ADDR);
 	uint lcrr;
 
 	lcrr = lbc->lcrr;
@@ -352,8 +351,8 @@ void upmconfig (uint upm, uint * table, uint size)
 		 i++, brp += 2, orp += 2) {
 
 		/* Look for a valid BR with selected UPM */
-		if ((in_be32(brp) & (BR_V | upmmask)) == (BR_V | upmmask)) {
-			dummy = (volatile u8*)(in_be32(brp) >> BR_BA_SHIFT);
+		if ((in_be32(brp) & (BR_V | BR_MSEL)) == (BR_V | upmmask)) {
+			dummy = (volatile u8*)(in_be32(brp) & BR_BA);
 			break;
 		}
 	}
-- 
1.5.5.2

[U-Boot-Users] [PATCH] mpc85xx: fix upmconfig

[Andy Fleming]

On Thu, Jul 10, 2008 at 7:36 AM, Sebastian Siewior
<bigeasy@linutronix.de> wrote:
> This actually shouldn't work. Imagina 0xf0000000 base address that
> gets translated into 0x1e000 and causes my box to hang. Writing
> to 0xf0000000 seems the better way.
> Also don't compare against the UPM mask but agaist the MSEL mask.
>
> Cc: Sergei Poselenov <sposelenov@emcraft.com>
> Cc: Andy Fleming <afleming@freescale.com>
> Signed-off-by: Sebastian Siewior <bigeasy@linutronix.de>
> ---
>  cpu/mpc85xx/cpu.c |    7 +++----
>  1 files changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/cpu/mpc85xx/cpu.c b/cpu/mpc85xx/cpu.c
> index 0497422..2373b4a 100644
> --- a/cpu/mpc85xx/cpu.c
> +++ b/cpu/mpc85xx/cpu.c
> @@ -71,8 +71,7 @@ struct cpu_type *identify_cpu(u32 ver)
>
>  static void set_lcb_clock(uint clkdiv)
>  {
> -       volatile immap_t *immap = (immap_t *)CFG_IMMR;
> -       volatile ccsr_lbc_t *lbc= &immap->im_lbc;
> +       volatile ccsr_lbc_t *lbc = (void *)(CFG_MPC85xx_LBC_ADDR);
>        uint lcrr;
>
>        lcrr = lbc->lcrr;
> @@ -352,8 +351,8 @@ void upmconfig (uint upm, uint * table, uint size)
>                 i++, brp += 2, orp += 2) {
>
>                /* Look for a valid BR with selected UPM */
> -               if ((in_be32(brp) & (BR_V | upmmask)) == (BR_V | upmmask)) {
> -                       dummy = (volatile u8*)(in_be32(brp) >> BR_BA_SHIFT);
> +               if ((in_be32(brp) & (BR_V | BR_MSEL)) == (BR_V | upmmask)) {
> +                       dummy = (volatile u8*)(in_be32(brp) & BR_BA);

This looks pretty good to me, but I'm not very familiar with this
code.  Could you explain a little more deeply what is wrong with the
old way, and why the new way is better?  I think I understand the
problem with the BA_SHIFT (actually, it alarms me, a bit), but I'm not
so sure about using MSEL on one side of the ==, and not the other.

Andy

[U-Boot-Users] [PATCH] mpc85xx: fix upmconfig

[Sebastian Siewior]

* Andy Fleming | 2008-07-14 19:32:56 [-0500]:

>> --- a/cpu/mpc85xx/cpu.c
>> +++ b/cpu/mpc85xx/cpu.c
>> @@ -71,8 +71,7 @@ struct cpu_type *identify_cpu(u32 ver)
>>
>>  static void set_lcb_clock(uint clkdiv)
>>  {
>> -       volatile immap_t *immap = (immap_t *)CFG_IMMR;
>> -       volatile ccsr_lbc_t *lbc= &immap->im_lbc;
>> +       volatile ccsr_lbc_t *lbc = (void *)(CFG_MPC85xx_LBC_ADDR);
>>        uint lcrr;
>>
>>        lcrr = lbc->lcrr;
>> @@ -352,8 +351,8 @@ void upmconfig (uint upm, uint * table, uint size)
>>                 i++, brp += 2, orp += 2) {
>>
>>                /* Look for a valid BR with selected UPM */
>> -               if ((in_be32(brp) & (BR_V | upmmask)) == (BR_V | upmmask)) {
>> -                       dummy = (volatile u8*)(in_be32(brp) >> BR_BA_SHIFT);
>> +               if ((in_be32(brp) & (BR_V | BR_MSEL)) == (BR_V | upmmask)) {
>> +                       dummy = (volatile u8*)(in_be32(brp) & BR_BA);
>
>This looks pretty good to me, but I'm not very familiar with this
>code.  Could you explain a little more deeply what is wrong with the
>old way, and why the new way is better?  I think I understand the
>problem with the BA_SHIFT (actually, it alarms me, a bit), but I'm not
the first part, changes the computation of the dummy address. The spec
[1] chap 14.4.4.2 referes to the dummy address as "...by a (dummy) write
transaction to the relevant UPM assigned bank." The address is assigned
in the relevant BRx[BA] register (bits 0-16 if not used extened
addresses) and therefore shift is wrong, logical and is correct.

>so sure about using MSEL on one side of the ==, and not the other.
BRx[MSEL] specifies the machines used (table 14-4, pdf page 631 in my
document). MSEL is the mask while upmmask (UPMC, UPMB, or UPMB) is the
content of the register. For instance, if your BR[MSEL] register would
have all bits set (what is reserved / not legal) than the old compare
method would match UPMA, UPMB & UPMC where is the new method would skip
it.

>Andy
Sebastian
[1] MPC8544ERM.pdf Rev1

[U-Boot-Users] [PATCH] mpc85xx: fix upmconfig

[Detlev Zundel]

Hi,

> This actually shouldn't work. Imagina 0xf0000000 base address that
> gets translated into 0x1e000 and causes my box to hang. Writing
> to 0xf0000000 seems the better way.
> Also don't compare against the UPM mask but agaist the MSEL mask.
>
> Cc: Sergei Poselenov <sposelenov@emcraft.com>
> Cc: Andy Fleming <afleming@freescale.com>
> Signed-off-by: Sebastian Siewior <bigeasy@linutronix.de>
> ---
>  cpu/mpc85xx/cpu.c |    7 +++----
>  1 files changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/cpu/mpc85xx/cpu.c b/cpu/mpc85xx/cpu.c
> index 0497422..2373b4a 100644
> --- a/cpu/mpc85xx/cpu.c
> +++ b/cpu/mpc85xx/cpu.c
> @@ -71,8 +71,7 @@ struct cpu_type *identify_cpu(u32 ver)
>  
>  static void set_lcb_clock(uint clkdiv)
>  {
> -	volatile immap_t *immap = (immap_t *)CFG_IMMR;
> -	volatile ccsr_lbc_t *lbc= &immap->im_lbc;
> +	volatile ccsr_lbc_t *lbc = (void *)(CFG_MPC85xx_LBC_ADDR);
>  	uint lcrr;
>  
>  	lcrr = lbc->lcrr;

This hunk does not apply for me, but the next one unbreaks the socrates
board, so

> @@ -352,8 +351,8 @@ void upmconfig (uint upm, uint * table, uint size)
>  		 i++, brp += 2, orp += 2) {
>  
>  		/* Look for a valid BR with selected UPM */
> -		if ((in_be32(brp) & (BR_V | upmmask)) == (BR_V | upmmask)) {
> -			dummy = (volatile u8*)(in_be32(brp) >> BR_BA_SHIFT);
> +		if ((in_be32(brp) & (BR_V | BR_MSEL)) == (BR_V | upmmask)) {
> +			dummy = (volatile u8*)(in_be32(brp) & BR_BA);
>  			break;
>  		}
>  	}

Acked-by: Detlev Zundel <dzu@denx.de>

Cheers
  Detlev

-- 
error compiling committee.c: too many arguments to function
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot-Users] [PATCH] mpc85xx: fix upmconfig

[Wolfgang Denk]

Hi Andy,

in message <20080715084956.GA30428@www.tglx.de> Sebastian Siewior wrote:
> * Andy Fleming | 2008-07-14 19:32:56 [-0500]:
...
> >This looks pretty good to me, but I'm not very familiar with this
> >code.  Could you explain a little more deeply what is wrong with the
> >old way, and why the new way is better?  I think I understand the
> >problem with the BA_SHIFT (actually, it alarms me, a bit), but I'm not
> the first part, changes the computation of the dummy address. The spec
> [1] chap 14.4.4.2 referes to the dummy address as "...by a (dummy) write
> transaction to the relevant UPM assigned bank." The address is assigned
> in the relevant BRx[BA] register (bits 0-16 if not used extened
> addresses) and therefore shift is wrong, logical and is correct.
> 
> >so sure about using MSEL on one side of the ==, and not the other.
> BRx[MSEL] specifies the machines used (table 14-4, pdf page 631 in my
> document). MSEL is the mask while upmmask (UPMC, UPMB, or UPMB) is the
> content of the register. For instance, if your BR[MSEL] register would
> have all bits set (what is reserved / not legal) than the old compare
> method would match UPMA, UPMB & UPMC where is the new method would skip
> it.

Any comment about the state of this patch?  Thanks in advance.

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
Many aligators will be slain, but the swamp will remain.