[U-Boot] [PATCH] Orion5x: bugfix: window size (mis)calculation

[U-Boot] [PATCH] Orion5x: bugfix: window size (mis)calculation

[Albert Aribaud]

Fix orion5x_winctrl_calcsize() off-by-1 bug which caused mapping
windows to be cut by half. This afected all windows including NOR
flash (causing half the flash to be unaccessible) but DRAM was and
still is fine as its size is determined otherwise.

Signed-off-by: Albert Aribaud <albert.aribaud@free.fr>
---
 arch/arm/cpu/arm926ejs/orion5x/cpu.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/cpu/arm926ejs/orion5x/cpu.c b/arch/arm/cpu/arm926ejs/orion5x/cpu.c
index 3740e33..260f88b 100644
--- a/arch/arm/cpu/arm926ejs/orion5x/cpu.c
+++ b/arch/arm/cpu/arm926ejs/orion5x/cpu.c
@@ -61,7 +61,7 @@ unsigned int orion5x_winctrl_calcsize(unsigned int sizeval)
 	unsigned int j = 0;
 	u32 val = sizeval >> 1;
 
-	for (i = 0; val > 0x10000; i++) {
+	for (i = 0; val >= 0x10000; i++) {
 		j |= (1 << i);
 		val = val >> 1;
 	}
-- 
1.7.1

[U-Boot] [PATCH] Orion5x: bugfix: window size (mis)calculation

[Prafulla Wadaskar]

 

> -----Original Message-----
> From: u-boot-bounces at lists.denx.de 
> [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Albert Aribaud
> Sent: Tuesday, August 24, 2010 6:58 PM
> To: u-boot at lists.denx.de
> Subject: [U-Boot] [PATCH] Orion5x: bugfix: window size 
> (mis)calculation
> 
> Fix orion5x_winctrl_calcsize() off-by-1 bug which caused mapping
> windows to be cut by half. This afected all windows including NOR
> flash (causing half the flash to be unaccessible) but DRAM was and
> still is fine as its size is determined otherwise.
> 
> Signed-off-by: Albert Aribaud <albert.aribaud@free.fr>
> ---
>  arch/arm/cpu/arm926ejs/orion5x/cpu.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/cpu/arm926ejs/orion5x/cpu.c 
> b/arch/arm/cpu/arm926ejs/orion5x/cpu.c
> index 3740e33..260f88b 100644
> --- a/arch/arm/cpu/arm926ejs/orion5x/cpu.c
> +++ b/arch/arm/cpu/arm926ejs/orion5x/cpu.c
> @@ -61,7 +61,7 @@ unsigned int 
> orion5x_winctrl_calcsize(unsigned int sizeval)
>  	unsigned int j = 0;
>  	u32 val = sizeval >> 1;
>  
> -	for (i = 0; val > 0x10000; i++) {
> +	for (i = 0; val >= 0x10000; i++) {
>  		j |= (1 << i);
>  		val = val >> 1;
>  	}
> -- 

Applied to u-boot-marvell.git master branch

Regards..
Prafulla . .

[U-Boot] [PATCH] Orion5x: bugfix: window size (mis)calculation

[Chris Moore]

  Hi list,

Le 24/08/2010 15:27, Albert Aribaud a ?crit :
> Fix orion5x_winctrl_calcsize() off-by-1 bug which caused mapping
> windows to be cut by half. This afected all windows including NOR
> flash (causing half the flash to be unaccessible) but DRAM was and
> still is fine as its size is determined otherwise.
>
> Signed-off-by: Albert Aribaud<albert.aribaud@free.fr>
> ---
>   arch/arm/cpu/arm926ejs/orion5x/cpu.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/cpu/arm926ejs/orion5x/cpu.c b/arch/arm/cpu/arm926ejs/orion5x/cpu.c
> index 3740e33..260f88b 100644
> --- a/arch/arm/cpu/arm926ejs/orion5x/cpu.c
> +++ b/arch/arm/cpu/arm926ejs/orion5x/cpu.c
> @@ -61,7 +61,7 @@ unsigned int orion5x_winctrl_calcsize(unsigned int sizeval)
>   	unsigned int j = 0;
>   	u32 val = sizeval>>  1;
>
> -	for (i = 0; val>  0x10000; i++) {
> +	for (i = 0; val>= 0x10000; i++) {
>   		j |= (1<<  i);
>   		val = val>>  1;
>   	}

Sorry for this late reply.
Both versions looked wrong to me but I couldn't test them at the time.

Albert's version gives the correct result for exact powers of 2.
This is admittedly the usual case.
However AFAICS it gives a wrong result for *all* other values above 64 KiB.

The following version gives the results I would expect:

unsigned int orion5x_winctrl_calcsize(unsigned int sizeval)
{
        int i;
        unsigned int j = 0;
        u32 val = sizeval - 1;

        for (i = 0; val >= 0x10000; i++) {
                j |= (1 << i);
                val = val >> 1;
        }
        return 0x0000ffff & j;
}

Notes:
a) Masking with 0x0000ffff is not necessary.
I kept it to keep the code as close as possible to the original.
b) A size of 0 on entry is stupid and must be considered as a special case.
The most useful is probably to treat 0 as 4 GiB.
This occurs naturally and needs no special handling.
Otherwise we could treat it as one 64 KiB page.
Otherwise we could BUG_ON.

However I have completely rewritten the function using a more efficient 
algorithm.
It gives identical results to the previous one above.
For large window sizes this version is much faster.
It avoids the loop which repeatedly divides the size by 2.

u32 orion5x_winctrl_calcsize(u32 x)
{
   /*
    * Step 1: Get the most significant one in the right bit position.
    *
    * Calculate the number of 64 KiB blocks needed minus one (rounding up).
    * It is equivalent to x = (u32) ceil((double) x / 65536.0) - 1
    */
   x = (x - 1) >> 16;

   /*
    * Step 2: Copy the most significant one (MSO) into all bits to its 
right.
    *
    * At this point the right shift above ensures that the 16 MSB of x 
are zero.
    * Propagate the MSO to all bits to its right (up to 15 bits).
    */
   x |= x >> 1;  /* Set the bit to the right of the MSO */
   x |= x >> 2;  /* Set the next 2 bits to the right of the MSO */
   x |= x >> 4;  /* Set the next 4 bits to the right of the MSO */
   x |= x >> 8;  /* Set the next 8 bits to the right of the MS0 */

   return x;
}

Here are my test results for all four versions:

For range 0x00000000 to 0x00020001 the original orion5x_winctrl_calcsize 
returns 0x0
For range 0x00020002 to 0x00040003 the original orion5x_winctrl_calcsize 
returns 0x1
For range 0x00040004 to 0x00080007 the original orion5x_winctrl_calcsize 
returns 0x3
For range 0x00080008 to 0x0010000f the original orion5x_winctrl_calcsize 
returns 0x7
For range 0x00100010 to 0x0020001f the original orion5x_winctrl_calcsize 
returns 0xf
For range 0x00200020 to 0x0040003f the original orion5x_winctrl_calcsize 
returns 0x1f
For range 0x00400040 to 0x0080007f the original orion5x_winctrl_calcsize 
returns 0x3f
For range 0x00800080 to 0x010000ff the original orion5x_winctrl_calcsize 
returns 0x7f
For range 0x01000100 to 0x020001ff the original orion5x_winctrl_calcsize 
returns 0xff
For range 0x02000200 to 0x040003ff the original orion5x_winctrl_calcsize 
returns 0x1ff
For range 0x04000400 to 0x080007ff the original orion5x_winctrl_calcsize 
returns 0x3ff
For range 0x08000800 to 0x10000fff the original orion5x_winctrl_calcsize 
returns 0x7ff
For range 0x10001000 to 0x20001fff the original orion5x_winctrl_calcsize 
returns 0xfff
For range 0x20002000 to 0x40003fff the original orion5x_winctrl_calcsize 
returns 0x1fff
For range 0x40004000 to 0x80007fff the original orion5x_winctrl_calcsize 
returns 0x3fff
For range 0x80008000 to 0xffffffff the original orion5x_winctrl_calcsize 
returns 0x7fff

For range 0x00000000 to 0x0001ffff Albert Aribaud's 
orion5x_winctrl_calcsize returns 0x0
For range 0x00020000 to 0x0003ffff Albert Aribaud's 
orion5x_winctrl_calcsize returns 0x1
For range 0x00040000 to 0x0007ffff Albert Aribaud's 
orion5x_winctrl_calcsize returns 0x3
For range 0x00080000 to 0x000fffff Albert Aribaud's 
orion5x_winctrl_calcsize returns 0x7
For range 0x00100000 to 0x001fffff Albert Aribaud's 
orion5x_winctrl_calcsize returns 0xf
For range 0x00200000 to 0x003fffff Albert Aribaud's 
orion5x_winctrl_calcsize returns 0x1f
For range 0x00400000 to 0x007fffff Albert Aribaud's 
orion5x_winctrl_calcsize returns 0x3f
For range 0x00800000 to 0x00ffffff Albert Aribaud's 
orion5x_winctrl_calcsize returns 0x7f
For range 0x01000000 to 0x01ffffff Albert Aribaud's 
orion5x_winctrl_calcsize returns 0xff
For range 0x02000000 to 0x03ffffff Albert Aribaud's 
orion5x_winctrl_calcsize returns 0x1ff
For range 0x04000000 to 0x07ffffff Albert Aribaud's 
orion5x_winctrl_calcsize returns 0x3ff
For range 0x08000000 to 0x0fffffff Albert Aribaud's 
orion5x_winctrl_calcsize returns 0x7ff
For range 0x10000000 to 0x1fffffff Albert Aribaud's 
orion5x_winctrl_calcsize returns 0xfff
For range 0x20000000 to 0x3fffffff Albert Aribaud's 
orion5x_winctrl_calcsize returns 0x1fff
For range 0x40000000 to 0x7fffffff Albert Aribaud's 
orion5x_winctrl_calcsize returns 0x3fff
For range 0x80000000 to 0xffffffff Albert Aribaud's 
orion5x_winctrl_calcsize returns 0x7fff

For range 0x00000000 to 0x00000000 Chris Moore's 
orion5x_winctrl_calcsize returns 0xffff
For range 0x00000001 to 0x00010000 Chris Moore's 
orion5x_winctrl_calcsize returns 0x0
For range 0x00010001 to 0x00020000 Chris Moore's 
orion5x_winctrl_calcsize returns 0x1
For range 0x00020001 to 0x00040000 Chris Moore's 
orion5x_winctrl_calcsize returns 0x3
For range 0x00040001 to 0x00080000 Chris Moore's 
orion5x_winctrl_calcsize returns 0x7
For range 0x00080001 to 0x00100000 Chris Moore's 
orion5x_winctrl_calcsize returns 0xf
For range 0x00100001 to 0x00200000 Chris Moore's 
orion5x_winctrl_calcsize returns 0x1f
For range 0x00200001 to 0x00400000 Chris Moore's 
orion5x_winctrl_calcsize returns 0x3f
For range 0x00400001 to 0x00800000 Chris Moore's 
orion5x_winctrl_calcsize returns 0x7f
For range 0x00800001 to 0x01000000 Chris Moore's 
orion5x_winctrl_calcsize returns 0xff
For range 0x01000001 to 0x02000000 Chris Moore's 
orion5x_winctrl_calcsize returns 0x1ff
For range 0x02000001 to 0x04000000 Chris Moore's 
orion5x_winctrl_calcsize returns 0x3ff
For range 0x04000001 to 0x08000000 Chris Moore's 
orion5x_winctrl_calcsize returns 0x7ff
For range 0x08000001 to 0x10000000 Chris Moore's 
orion5x_winctrl_calcsize returns 0xfff
For range 0x10000001 to 0x20000000 Chris Moore's 
orion5x_winctrl_calcsize returns 0x1fff
For range 0x20000001 to 0x40000000 Chris Moore's 
orion5x_winctrl_calcsize returns 0x3fff
For range 0x40000001 to 0x80000000 Chris Moore's 
orion5x_winctrl_calcsize returns 0x7fff
For range 0x80000001 to 0xffffffff Chris Moore's 
orion5x_winctrl_calcsize returns 0xffff

For range 0x00000000 to 0x00000000 Chris Moore's fast 
orion5x_winctrl_calcsize returns 0xffff
For range 0x00000001 to 0x00010000 Chris Moore's fast 
orion5x_winctrl_calcsize returns 0x0
For range 0x00010001 to 0x00020000 Chris Moore's fast 
orion5x_winctrl_calcsize returns 0x1
For range 0x00020001 to 0x00040000 Chris Moore's fast 
orion5x_winctrl_calcsize returns 0x3
For range 0x00040001 to 0x00080000 Chris Moore's fast 
orion5x_winctrl_calcsize returns 0x7
For range 0x00080001 to 0x00100000 Chris Moore's fast 
orion5x_winctrl_calcsize returns 0xf
For range 0x00100001 to 0x00200000 Chris Moore's fast 
orion5x_winctrl_calcsize returns 0x1f
For range 0x00200001 to 0x00400000 Chris Moore's fast 
orion5x_winctrl_calcsize returns 0x3f
For range 0x00400001 to 0x00800000 Chris Moore's fast 
orion5x_winctrl_calcsize returns 0x7f
For range 0x00800001 to 0x01000000 Chris Moore's fast 
orion5x_winctrl_calcsize returns 0xff
For range 0x01000001 to 0x02000000 Chris Moore's fast 
orion5x_winctrl_calcsize returns 0x1ff
For range 0x02000001 to 0x04000000 Chris Moore's fast 
orion5x_winctrl_calcsize returns 0x3ff
For range 0x04000001 to 0x08000000 Chris Moore's fast 
orion5x_winctrl_calcsize returns 0x7ff
For range 0x08000001 to 0x10000000 Chris Moore's fast 
orion5x_winctrl_calcsize returns 0xfff
For range 0x10000001 to 0x20000000 Chris Moore's fast 
orion5x_winctrl_calcsize returns 0x1fff
For range 0x20000001 to 0x40000000 Chris Moore's fast 
orion5x_winctrl_calcsize returns 0x3fff
For range 0x40000001 to 0x80000000 Chris Moore's fast 
orion5x_winctrl_calcsize returns 0x7fff
For range 0x80000001 to 0xffffffff Chris Moore's fast 
orion5x_winctrl_calcsize returns 0xffff

AIUI (apart from the question of how best to handle a size of 0) this is 
the required result.
If I have misunderstood, please tell me and I'll rewrite.

HTH.

Cheers,
Chris

[U-Boot] [PATCH] Orion5x: bugfix: window size (mis)calculation

[Albert ARIBAUD]

Le 27/08/2010 07:00, Chris Moore a ?crit :

> Hi list,

Hi Chris!

> [...]
> For range 0x00000000 to 0x00000000 Chris Moore's fast
> orion5x_winctrl_calcsize returns 0xffff
> [...]
> For range 0x80000001 to 0xffffffff Chris Moore's fast
> orion5x_winctrl_calcsize returns 0xffff
>
> AIUI (apart from the question of how best to handle a size of 0) this is
> the required result.

I think your proposal to handle size 0 as meaning '4 MB' is fine, since 
there is no way to express 4MB and a zero size is meaningless as such.

> If I have misunderstood, please tell me and I'll rewrite.

That's fine. Do you want me to resubmit a V2 patch with your change, or 
will you subit it yourself?

> HTH.

It does, as does every fix that brings orion/kirkwood U-boot nearer to 
industry strength. :)

> Cheers,
> Chris

Amicalement,
-- 
Albert.

[U-Boot] [PATCH] Orion5x: bugfix: window size (mis)calculation

[Albert ARIBAUD]

Le 27/08/2010 07:37, Albert ARIBAUD a ?crit :

> I think your proposal to handle size 0 as meaning '4 MB'

That's 4 *TB*, of course. :)

Amicalement,
-- 
Albert.

[U-Boot] [PATCH] Orion5x: bugfix: window size (mis)calculation

[Chris Moore]

  Hello Albert,

Le 27/08/2010 07:37, Albert ARIBAUD a ?crit :
> Le 27/08/2010 07:00, Chris Moore a ?crit :
>
>
> I think your proposal to handle size 0 as meaning '4 MB' is fine, 
> since there is no way to express 4MB and a zero size is meaningless as 
> such.
>

s/MB/GiB/
I agree that it is the most useful.
It also gives the shortest code as it needs no special handling ;-)

>> If I have misunderstood, please tell me and I'll rewrite.
>
> That's fine. Do you want me to resubmit a V2 patch with your change, 
> or will you subit it yourself?
>

I'd prefer you to submit it if you don't mind as I don't have a U-Boot 
git tree ATM.

Please consider the following version where I have tried to improve the 
comments.
The only other changes are:
a) to keep the return value an unsigned int as in the original.
b) to copy the argument to a local variable.
This enables me to have an argument with a descriptive name and a 
variable with a short name to stay within 78 columns.

/*
  * The spammers are right: size *is* important ;-)
  * This version will not work if sizeval is more than 32 bits.
  * I have therefore made it a u32 to underline this.
  * The return value does not need to be a u32 so I left it as an 
unsigned int.
  */
unsigned int orion5x_winctrl_calcsize_CM_fast(u32 sizeval)
{
   u32 x = sizeval;

   /*
    * Requesting a window with a size of 0 bytes makes no sense.
    * A sizeval of 0 therefore needs special handling such as:
    * a) treat 0 as 4 GiB.
    * b) treat 0 as an error.
    * c) treat 0 as requiring one 64 KiB block.
    *
    * The most useful is probably to treat 0 as 4 GiB as we do here.
    * This occurs naturally and needs no special handling.
    */

   /*
    * Step 1: Get the most significant one (MSO) in the correct bit 
position.
    *
    * Calculate the number of 64 KiB blocks needed minus one (rounding up).
    * For x > 0 this is equivalent to x = (u32) ceil((double) x / 
65536.0) - 1
    */
   x = (x - 1) >> 16;

   /*
    * Step 2: Force all bits to the right of the MSO to one.
    *
    * The right shift above ensures that the 16 MSB of x are zero.
    * So, for a u32, there are never more than 15 bits to the right of 
the MSO.
    * We 'or' the MSO into them which forces them to one.
    */
   x |= x >> 1;  /* Force the first bit to the right of the MSO to one */
   x |= x >> 2;  /* Force the next 2 bits to the right of the MSO to one */
   x |= x >> 4;  /* Force the next 4 bits to the right of the MSO to one */
   x |= x >> 8;  /* Force the next 8 bits to the right of the MS0 to one */

   return x;
}

Without comments I think the code would be difficult to understand.
However it may now be overcommented.
Please feel free to modify the comments.
If necessary you can add my SOB.

Cheers,
Chris