[U-Boot-Users] Spartan FPGA patch

[Jerry Van Baren <gerald.vanbaren@ge.com>]

Wolfgang Denk wrote:
> Dear Matthias,
> 
> in message <200711071529.49762.matthias.fuchs@esd-electronics.com> you wrote:
>> when copying val from 'data[bytecount++]' it is common practice that val
>> if of the same type as the array elements. So val should be unsigned char.
> 
> Maybe. But what's the difference?
> 
>> I changed the sign check into a 'val & 0x80', which I think is fine an clean.
> 
> If it is indeed intended to be a test for  a  negative  number,  then
> this is neither nice nor clean.
> 
> Best regards,
> 
> Wolfgang Denk


Hi Wolfgang,

You are missing the point of signed vs. unsigned because you got sucked 
into the wrong argument.  The problematic code is:

  512      val = data [bytecount ++];
  513      i = 8;
  514      do {
  515              /* Deassert the clock */
  516              (*fn->clk) (FALSE, TRUE, cookie);
  517              CONFIG_FPGA_DELAY ();
  518              /* Write data */
  519              (*fn->wr) ((val < 0), TRUE, cookie);  <-------- BAD
  520              CONFIG_FPGA_DELAY ();
  521              /* Assert the clock */
  522              (*fn->clk) (TRUE, TRUE, cookie);
  523              CONFIG_FPGA_DELAY ();
  524              val <<= 1;
  525              i --;
  526      } while (i > 0);

As you can see, the code is looking at the MSB of the 8 bit value to see 
if it must program a '1' or a '0' and it is shifting the byte left by 1 
bit each time.

The problem is that it is using a *signed character test* (val < 0) 
where it *should be* using an bit ANDing operation to isolate the MSBit 
of the 8 bit "char" (and I would leave the char as a char, since it is 
immaterial whether it is signed or unsigned *if the correct test were 
used*).  This is what Angelos recommended:
   <http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/33116>

Obviously, I agree with him and recommend that the
   (val < 0)
be changed to
   (val & 0x80)
rather than the original fix (which also works, but is fixing the 
problem in the wrong way IMHO).

Best regards,
gvb