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
