From mboxrd@z Thu Jan  1 00:00:00 1970
From: Chris Moore <moore@free.fr>
Date: Sat, 28 Aug 2010 07:32:18 +0200
Subject: [U-Boot] [PATCH] Orion5x: bugfix: window size (mis)calculation
In-Reply-To: <4C774F01.3020703@free.fr>
References: <1282656460-10999-1-git-send-email-albert.aribaud@free.fr>
	<4C774652.6090200@free.fr> <4C774F01.3020703@free.fr>
Message-ID: <4C789F62.3050306@free.fr>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

  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