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

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

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

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

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

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