Re: [U-Boot] [PATCH] Orion5x: bugfix: window size (mis)calculation
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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] Orion5x: bugfix: window size (mis)calculation
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 Aribaudalbert.arib...@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 0x1; i++) {
+ for (i = 0; val= 0x1; 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 = 0x1; i++) {
j |= (1 i);
val = val 1;
}
return 0x j;
}
Notes:
a) Masking with 0x 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 0x 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 0x001f 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 0x01ff 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 0x1fff 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 0x the original orion5x_winctrl_calcsize
returns 0x7fff
For range 0x to 0x0001 Albert Aribaud's
orion5x_winctrl_calcsize returns 0x0
For range 0x0002 to 0x0003 Albert Aribaud's
orion5x_winctrl_calcsize returns 0x1
For range 0x0004 to 0x0007 Albert Aribaud's
orion5x_winctrl_calcsize returns 0x3
For range 0x0008 to 0x000f Albert Aribaud's
orion5x_winctrl_calcsize returns 0x7
For range 0x0010 to 0x001f Albert Aribaud's
orion5x_winctrl_calcsize returns 0xf
For range 0x0020 to 0x003f Albert Aribaud's
orion5x_winctrl_calcsize returns 0x1f
Re: [U-Boot] [PATCH] Orion5x: bugfix: window size (mis)calculation
Le 27/08/2010 07:00, Chris Moore a écrit : Hi list, Hi Chris! [...] For range 0x to 0x Chris Moore's fast orion5x_winctrl_calcsize returns 0x [...] For range 0x8001 to 0x Chris Moore's fast orion5x_winctrl_calcsize returns 0x 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 mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] Orion5x: bugfix: window size (mis)calculation
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 mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] Orion5x: bugfix: window size (mis)calculation
-Original Message-
From: u-boot-boun...@lists.denx.de
[mailto:u-boot-boun...@lists.denx.de] On Behalf Of Albert Aribaud
Sent: Tuesday, August 24, 2010 6:58 PM
To: u-boot@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.arib...@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 0x1; i++) {
+ for (i = 0; val = 0x1; i++) {
j |= (1 i);
val = val 1;
}
--
Applied to u-boot-marvell.git master branch
Regards..
Prafulla . .
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
