Spartan FPGA patch

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

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

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

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

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

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

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

-----------------------------------------------------------------------
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
-----------------------------------------------------------------------

Hello!

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/

Dear Matthias,

Maybe. But what's the difference?

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

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

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

Nobody intended to do that.

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 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.

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

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

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.

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...

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