[U-Boot-Users] Spartan FPGA patch

[U-Boot-Users] Spartan FPGA patch

[Aggelos Manousarides]

The following patch fixes a bug in the slave serial programming mode for 
the xilinx spartan2 FPGA. A "char val" is declared, but it is used as a 
signed char. The check for negative value (<0) is always true on arm, or 
any other platform in which the char is not signed by default. As a 
result the FPGA cannot be programmed.

By the way, I have tested this code and it works fine, apart from this 
error. I am using it in ARM platforms in both slave parallel and slave 
serial modes. I am this because I don't see any other platforms using it.

This is a patch against 1.2.0, but I saw that the same code exists in 
1.3.0-rc3 as well.

--
Angelos Manousarides

-------------- next part --------------
A non-text attachment was scrubbed...
Name: fpga.patch
Type: text/x-patch
Size: 420 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20071030/55725c8e/attachment.bin 

[U-Boot-Users] Spartan FPGA patch

[Wolfgang Denk]

In message <472752F9.9000307@inaccessnetworks.com> you wrote:
>
> The following patch fixes a bug in the slave serial programming mode for 
> the xilinx spartan2 FPGA. A "char val" is declared, but it is used as a 
> signed char. The check for negative value (<0) is always true on arm, or 
> any other platform in which the char is not signed by default. As a 
> result the FPGA cannot be programmed.

I have to admit that I hate to see "signed  char"  in  the  code.  Is
there  any  special  reaso why "val" has to be a "char" type? Why not
making it an "int" ?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
8 Catfish                                               = 1 Octo-puss

[U-Boot-Users] Spartan FPGA patch

[Aggelos Manousarides]

Wolfgang Denk wrote:
> In message <472752F9.9000307@inaccessnetworks.com> you wrote:
>> The following patch fixes a bug in the slave serial programming mode for 
>> the xilinx spartan2 FPGA. A "char val" is declared, but it is used as a 
>> signed char. The check for negative value (<0) is always true on arm, or 
>> any other platform in which the char is not signed by default. As a 
>> result the FPGA cannot be programmed.
> 
> I have to admit that I hate to see "signed  char"  in  the  code.  Is
> there  any  special  reaso why "val" has to be a "char" type? Why not
> making it an "int" ?

Another way to do this safely is to declare it as an "unsigned char" and 
instead of doing "val < 0", do "val & 0x80". I don't like the current 
code either. Shifting a signed char and testing for negativity is 
definitely not the best way to test that the MSB is set.

--
Angelos Manousaridis

[U-Boot-Users] Spartan FPGA patch

[Wolfgang Denk]

In message <472F1393.7040306@inaccessnetworks.com> you wrote:
> Wolfgang Denk wrote:
> > In message <472752F9.9000307@inaccessnetworks.com> you wrote:
> >> The following patch fixes a bug in the slave serial programming mode for 
> >> the xilinx spartan2 FPGA. A "char val" is declared, but it is used as a 
> >> signed char. The check for negative value (<0) is always true on arm, or 
> >> any other platform in which the char is not signed by default. As a 
> >> result the FPGA cannot be programmed.
> > 
> > I have to admit that I hate to see "signed  char"  in  the  code.  Is
> > there  any  special  reaso why "val" has to be a "char" type? Why not
> > making it an "int" ?
> 
> Another way to do this safely is to declare it as an "unsigned char" and 
> instead of doing "val < 0", do "val & 0x80". I don't like the current 
> code either. Shifting a signed char and testing for negativity is 
> definitely not the best way to test that the MSB is set.

So let me repeat my question: is there any special reason  why  "val"
has to be a "char" type? Why not making it an "int" ?


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The idea of male and female are universal constants.
	-- Kirk, "Metamorphosis", stardate 3219.8

[U-Boot-Users] Spartan FPGA patch

[Aggelos Manousarides]

Wolfgang Denk wrote:
> In message <472F1393.7040306@inaccessnetworks.com> you wrote:
>> Wolfgang Denk wrote:
>>> In message <472752F9.9000307@inaccessnetworks.com> you wrote:
>>>> The following patch fixes a bug in the slave serial programming mode for 
>>>> the xilinx spartan2 FPGA. A "char val" is declared, but it is used as a 
>>>> signed char. The check for negative value (<0) is always true on arm, or 
>>>> any other platform in which the char is not signed by default. As a 
>>>> result the FPGA cannot be programmed.
>>> I have to admit that I hate to see "signed  char"  in  the  code.  Is
>>> there  any  special  reaso why "val" has to be a "char" type? Why not
>>> making it an "int" ?
>> Another way to do this safely is to declare it as an "unsigned char" and 
>> instead of doing "val < 0", do "val & 0x80". I don't like the current 
>> code either. Shifting a signed char and testing for negativity is 
>> definitely not the best way to test that the MSB is set.
> 
> So let me repeat my question: is there any special reason  why  "val"
> has to be a "char" type? Why not making it an "int" ?

Because you will run into endianness problems. You want to treat an 
"unsigned char" buffer as a bitstream, reading the MSB every time. How 
are you going to do this portably (in both big and little endian 
architectures) with an "int" type, which is at least 2 bytes wide?

--
Angelos Manousaridis

[U-Boot-Users] Spartan FPGA patch

[Wolfgang Denk]

In message <4731A4E4.20308@inaccessnetworks.com> you wrote:
>
> > So let me repeat my question: is there any special reason  why  "val"
> > has to be a "char" type? Why not making it an "int" ?
> 
> Because you will run into endianness problems. You want to treat an 
> "unsigned char" buffer as a bitstream, reading the MSB every time. How 
> are you going to do this portably (in both big and little endian 
> architectures) with an "int" type, which is at least 2 bytes wide?

Oops? I can't parse that. What's the difference between "signed char"
and "int" except the number of bits?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Wish not to seem, but to be, the best."                  - Aeschylus

[U-Boot-Users] Spartan FPGA patch

[Andreas Schweigstill]

Hello!

Wolfgang Denk schrieb:
> Oops? I can't parse that. What's the difference between "signed char"
> and "int" except the number of bits?

Like mentioned before, accessing data via a (signed|unsigned|.) char *
is independant of the processor endianess. If you want to read a
bitstream which is bytewise formatted you don't want to care about
endianess. Or do you also want some endianess string definitions?

#ifdef __LITTLE_ENDIAN
const char my_text[] = "sihT si xe a \0\0te";
#endif
#ifdef __BIG_ENDIAN
const char my_text[] = "This is a text";
#endif

int * p_text = (int *)my_text;


And it is even worse; on some architectures, like ARM, it is not
allowed to do a 16/32 bit memory access on a non-aligned address,
e.g.:

int val;
int * p_data;

p_data = (int *)0x23400001;
val = *p_data;


Depending of the ARM implementation you either get a data abort or
an "implementation depending" wrong value in val. For write access
it is even worse because this can overwrite some memory.

With best regards
Andreas Schweigstill

-- 
Dipl.-Phys. Andreas Schweigstill
Schweigstill IT | Embedded Systems
Schauenburgerstra?e 116, D-24118 Kiel, Germany
Phone: (+49) 431 5606-435, Fax: (+49) 431 5606-436
Mobile: (+49) 171 6921973, Web: http://www.schweigstill.de/

[U-Boot-Users] Spartan FPGA patch

[Wolfgang Denk]

In message <4731ECA0.1010502@schweigstill.de> you wrote:
> 
> Wolfgang Denk schrieb:
> > Oops? I can't parse that. What's the difference between "signed char"
> > and "int" except the number of bits?
> 
> Like mentioned before, accessing data via a (signed|unsigned|.) char *
> is independant of the processor endianess. If you want to read a

Correct. And storing the result in an "int" type is independent of the
byte order as well,if you do it right.

> bitstream which is bytewise formatted you don't want to care about
> endianess. Or do you also want some endianess string definitions?

What has this to do with what we're discussing?

> And it is even worse; on some architectures, like ARM, it is not
> allowed to do a 16/32 bit memory access on a non-aligned address,
> e.g.:

Nobody intended to do that.

> int val;
> int * p_data;

STOP!  I asked why we cannot change "val" into an "int". I never said
anything about using an "int *" to access to buffer data.

> Depending of the ARM implementation you either get a data abort or
> an "implementation depending" wrong value in val. For write access
> it is even worse because this can overwrite some memory.

This bug is your invention, not mine.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Dear Lord: I just want *one* one-armed manager so  I  never  have  to
hear "On the other hand", again.

[U-Boot-Users] Spartan FPGA patch

[Aggelos Manousarides]

Wolfgang Denk wrote:
> In message <4731ECA0.1010502@schweigstill.de> you wrote:
>> Wolfgang Denk schrieb:
>>> Oops? I can't parse that. What's the difference between "signed char"
>>> and "int" except the number of bits?
>> Like mentioned before, accessing data via a (signed|unsigned|.) char *
>> is independant of the processor endianess. If you want to read a
> 
> Correct. And storing the result in an "int" type is independent of the
> byte order as well,if you do it right.
> 
>> bitstream which is bytewise formatted you don't want to care about
>> endianess. Or do you also want some endianess string definitions?
> 
> What has this to do with what we're discussing?
> 
>> And it is even worse; on some architectures, like ARM, it is not
>> allowed to do a 16/32 bit memory access on a non-aligned address,
>> e.g.:
> 
> Nobody intended to do that.
> 
>> int val;
>> int * p_data;
> 
> STOP!  I asked why we cannot change "val" into an "int". I never said
> anything about using an "int *" to access to buffer data.
> 

This is the main misunderstanding. When you said "int" I though you 
meant dereferencing an "int *", in fact not only me but other people on 
the list as well. So your proposal is to convert the "char val" to an 
"int val". You don't solve the problem I mentioned by doing this.

This is the current code:

*********************
unsigned char *data = ...;
char val;
int i;

...

val = data [bytecount ++];
i = 8;
do {
    ...
    write(..., (val < 0), ...);
    ...
    val <<= 1;
    i--
} while (i > 0);

*********************

Let us not forget that all we want to do here is take the *bits* of the 
buffer one by one, starting from the MSB. Checking for negativity is 
just a hack to acquire the MSB, since signed values are two's complement.

This code works on all platforms in which "char" defaults to "signed 
char", because storing a byte from an "unsigned char *" to a "char" will 
implicitly convert it to a negative value. On ARM, this fails, becuase 
"char" is also "unsigned char", thus "val<0" is always zero.

Your proposed solution also fails. Just converting the "char val" to an 
"int val", will never produce a negative value in any architecture. This 
integer will always be positive.

To avoid a new series of misunderstandings, you said "if you do it 
right" when you talked about the "int". This could be interpreted as 
some kind of cast so that the "int val" is negative on all 
architectures, but I fail to see how this is cleaner than converting the 
val to "unsigned char" like the "data" and doing "val & 0x80".

--
Angelos Manousaridis

[U-Boot-Users] Spartan FPGA patch

[Wolfgang Denk]

In message <473AE49A.40801@inaccessnetworks.com> you wrote:
>
> This is the main misunderstanding. When you said "int" I though you 
> meant dereferencing an "int *", in fact not only me but other people on 
> the list as well. So your proposal is to convert the "char val" to an 
> "int val". You don't solve the problem I mentioned by doing this.

Well, the original error description said the problem was  caused  by
the  fact  that  "char"  might  be treated as "unsigned char" on some
platforms to the test for "< 0" would always fail.

> Let us not forget that all we want to do here is take the *bits* of the 
> buffer one by one, starting from the MSB. Checking for negativity is 
> just a hack to acquire the MSB, since signed values are two's complement.

If this was the intention of the code,  then  the  implementation  of
that  part  is  wrong (as has been pointed out before). Ii is already
wrong to assume that you are on a two complements machine...

> architectures, but I fail to see how this is cleaner than converting the 
> val to "unsigned char" like the "data" and doing "val & 0x80".

It depends on the purpose of the code (which I didn't bother  to  dig
into). If you want to really make a difference between <0 and >=0 you
should use integer types. If you want to test if a certain bit is set
or  not than you should use a logical AND operation. In this case the
bug was in using a '<0', not in the variable type.

I think we can close this now. A patch was submitted which cleans this
up.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Nobody will ever need more than 640k RAM!"       -- Bill Gates, 1981
"Windows 95 needs@least 8 MB RAM."             -- Bill Gates, 1996
"Nobody will ever need Windows 95."             -- logical conclusion

[U-Boot-Users] Spartan FPGA patch

[Matthias Fuchs]

Hi Wolfgang,

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.

I changed the sign check into a 'val & 0x80', which I think is fine an clean.

Matthias

On Monday 05 November 2007 20:21, Wolfgang Denk wrote:
> In message <472F1393.7040306@inaccessnetworks.com> you wrote:
> > Wolfgang Denk wrote:
> > > In message <472752F9.9000307@inaccessnetworks.com> you wrote:
> > >> The following patch fixes a bug in the slave serial programming mode for 
> > >> the xilinx spartan2 FPGA. A "char val" is declared, but it is used as a 
> > >> signed char. The check for negative value (<0) is always true on arm, or 
> > >> any other platform in which the char is not signed by default. As a 
> > >> result the FPGA cannot be programmed.
> > > 
> > > I have to admit that I hate to see "signed  char"  in  the  code.  Is
> > > there  any  special  reaso why "val" has to be a "char" type? Why not
> > > making it an "int" ?
> > 
> > Another way to do this safely is to declare it as an "unsigned char" and 
> > instead of doing "val < 0", do "val & 0x80". I don't like the current 
> > code either. Shifting a signed char and testing for negativity is 
> > definitely not the best way to test that the MSB is set.
> 
> So let me repeat my question: is there any special reason  why  "val"
> has to be a "char" type? Why not making it an "int" ?
> 
> 
> Best regards,
> 
> Wolfgang Denk
> 

-- 
-----------------------------------------------------------------------
Dipl.-Ing. Matthias Fuchs             esd electronic system design gmbh
http://www.esd-electronics.com                    Vahrenwalder Str. 207
phone: +49-511-37298-0, fax: -68                30165 Hannover, Germany
-----------------------------------------------------------------------

[U-Boot-Users] Spartan FPGA patch

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
We have phasers, I vote we blast 'em!
	-- Bailey, "The Corbomite Maneuver", stardate 1514.2

[U-Boot-Users] Spartan FPGA patch

[Jerry Van Baren]

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