* [PATCH] make new y-modem (PATCH v2) work on big-endian CPU
@ 2012-11-07 8:23 Antony Pavlov
2012-11-07 23:41 ` Robert Jarzmik
0 siblings, 1 reply; 5+ messages in thread
From: Antony Pavlov @ 2012-11-07 8:23 UTC (permalink / raw)
To: barebox
apply this commit after that one:
Author: Robert Jarzmik <robert.jarzmik@free.fr>
Date: Sun Nov 4 18:55:23 2012 +0100
commands: change Y-Modem implementation
---
lib/xymodem.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/lib/xymodem.c b/lib/xymodem.c
index 1469a9a..0e82ce2 100644
--- a/lib/xymodem.c
+++ b/lib/xymodem.c
@@ -257,8 +257,8 @@ static ssize_t xy_read_block(struct xyz_ctxt *proto, struct xy_block *blk,
uint64_t timeout)
{
ssize_t rc, data_len = 0;
- unsigned char hdr, seqs[2], crcs[2];
- int crc = 0, hdr_found = 0;
+ unsigned char hdr, seqs[2];
+ uint16_t crc = 0, hdr_found = 0;
uint64_t start = get_time_ns();
while (!hdr_found) {
@@ -308,12 +308,13 @@ static ssize_t xy_read_block(struct xyz_ctxt *proto, struct xy_block *blk,
switch (proto->crc_mode) {
case CRC_ADD8:
- rc = xy_gets(proto->cdev, proto->fifo, crcs, 1, timeout);
- crc = crcs[0];
+ rc = xy_gets(proto->cdev, proto->fifo,
+ (unsigned char *)&crc, 1, timeout);
break;
case CRC_CRC16:
- rc = xy_gets(proto->cdev, proto->fifo, crcs, 2, timeout);
- crc = be16_to_cpu(*(uint16_t *)crcs);
+ rc = xy_gets(proto->cdev, proto->fifo,
+ (unsigned char *)&crc, 2, timeout);
+ crc = be16_to_cpu(crc);
break;
case CRC_NONE:
rc = 0;
--
1.7.10.4
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] make new y-modem (PATCH v2) work on big-endian CPU
2012-11-07 8:23 [PATCH] make new y-modem (PATCH v2) work on big-endian CPU Antony Pavlov
@ 2012-11-07 23:41 ` Robert Jarzmik
2012-11-08 5:49 ` Antony Pavlov
2012-11-08 19:57 ` Antony Pavlov
0 siblings, 2 replies; 5+ messages in thread
From: Robert Jarzmik @ 2012-11-07 23:41 UTC (permalink / raw)
To: Antony Pavlov; +Cc: barebox
Antony Pavlov <antonynpavlov@gmail.com> writes:
> apply this commit after that one:
>
> Author: Robert Jarzmik <robert.jarzmik@free.fr>
> Date: Sun Nov 4 18:55:23 2012 +0100
>
> commands: change Y-Modem implementation
> ---
> lib/xymodem.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/lib/xymodem.c b/lib/xymodem.c
> index 1469a9a..0e82ce2 100644
> --- a/lib/xymodem.c
> +++ b/lib/xymodem.c
> @@ -257,8 +257,8 @@ static ssize_t xy_read_block(struct xyz_ctxt *proto, struct xy_block *blk,
> uint64_t timeout)
> {
> ssize_t rc, data_len = 0;
> - unsigned char hdr, seqs[2], crcs[2];
> - int crc = 0, hdr_found = 0;
> + unsigned char hdr, seqs[2];
> + uint16_t crc = 0, hdr_found = 0;
> uint64_t start = get_time_ns();
>
> while (!hdr_found) {
> @@ -308,12 +308,13 @@ static ssize_t xy_read_block(struct xyz_ctxt *proto, struct xy_block *blk,
>
> switch (proto->crc_mode) {
> case CRC_ADD8:
> - rc = xy_gets(proto->cdev, proto->fifo, crcs, 1, timeout);
> - crc = crcs[0];
> + rc = xy_gets(proto->cdev, proto->fifo,
> + (unsigned char *)&crc, 1, timeout);
This doesn't look good to me.
In big-endian arch, suppose you read 0xab as the crc.
In that case, won't crc be equal to 0xab00 instead of 0x00ab in this code ? If
that's the case, the previous code was right ...
> break;
> case CRC_CRC16:
> - rc = xy_gets(proto->cdev, proto->fifo, crcs, 2, timeout);
> - crc = be16_to_cpu(*(uint16_t *)crcs);
> + rc = xy_gets(proto->cdev, proto->fifo,
> + (unsigned char *)&crc, 2, timeout);
> + crc = be16_to_cpu(crc);
Does that mean that the code up there doesn't work on big endian ? I'm really
puzzled, as the sender sends in big endian, the xy_gets() receives the bytes
into crcs[2], and the be16_to_cpu() should be a no-op ...
I think that we should do this simpler :
crc = crcs[0] << 8 + crcs[1]
I'll put that into my github branch for testing.
Cheers.
--
Robert
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] make new y-modem (PATCH v2) work on big-endian CPU
2012-11-07 23:41 ` Robert Jarzmik
@ 2012-11-08 5:49 ` Antony Pavlov
2012-11-08 19:59 ` Robert Jarzmik
2012-11-08 19:57 ` Antony Pavlov
1 sibling, 1 reply; 5+ messages in thread
From: Antony Pavlov @ 2012-11-08 5:49 UTC (permalink / raw)
To: barebox
On 8 November 2012 03:41, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
> Antony Pavlov <antonynpavlov@gmail.com> writes:
>
>> apply this commit after that one:
>>
>> Author: Robert Jarzmik <robert.jarzmik@free.fr>
>> Date: Sun Nov 4 18:55:23 2012 +0100
>>
>> commands: change Y-Modem implementation
>> ---
>> lib/xymodem.c | 13 +++++++------
>> 1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/lib/xymodem.c b/lib/xymodem.c
>> index 1469a9a..0e82ce2 100644
>> --- a/lib/xymodem.c
>> +++ b/lib/xymodem.c
>> @@ -257,8 +257,8 @@ static ssize_t xy_read_block(struct xyz_ctxt *proto, struct xy_block *blk,
>> uint64_t timeout)
>> {
>> ssize_t rc, data_len = 0;
>> - unsigned char hdr, seqs[2], crcs[2];
>> - int crc = 0, hdr_found = 0;
>> + unsigned char hdr, seqs[2];
>> + uint16_t crc = 0, hdr_found = 0;
>> uint64_t start = get_time_ns();
>>
>> while (!hdr_found) {
>> @@ -308,12 +308,13 @@ static ssize_t xy_read_block(struct xyz_ctxt *proto, struct xy_block *blk,
>>
>> switch (proto->crc_mode) {
>> case CRC_ADD8:
>> - rc = xy_gets(proto->cdev, proto->fifo, crcs, 1, timeout);
>> - crc = crcs[0];
>> + rc = xy_gets(proto->cdev, proto->fifo,
>> + (unsigned char *)&crc, 1, timeout);
> This doesn't look good to me.
> In big-endian arch, suppose you read 0xab as the crc.
> In that case, won't crc be equal to 0xab00 instead of 0x00ab in this code ? If
> that's the case, the previous code was right ...
To make the patch I got your old working version and your PATCH v2
branches. I got 'git diff'. I found the key difference and tested the
patch. That is all.
May be the 'proto->crc_mode==CRC_ADD8' condition is always false
during my tests?
>> break;
>> case CRC_CRC16:
>> - rc = xy_gets(proto->cdev, proto->fifo, crcs, 2, timeout);
>> - crc = be16_to_cpu(*(uint16_t *)crcs);
>> + rc = xy_gets(proto->cdev, proto->fifo,
>> + (unsigned char *)&crc, 2, timeout);
>> + crc = be16_to_cpu(crc);
>
> Does that mean that the code up there doesn't work on big endian ? I'm really
> puzzled, as the sender sends in big endian, the xy_gets() receives the bytes
> into crcs[2], and the be16_to_cpu() should be a no-op ...
I have no idea just now. I'll try to disassemble generated code.
> I think that we should do this simpler :
> crc = crcs[0] << 8 + crcs[1]
>
> I'll put that into my github branch for testing.
--
Best regards,
Antony Pavlov
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] make new y-modem (PATCH v2) work on big-endian CPU
2012-11-07 23:41 ` Robert Jarzmik
2012-11-08 5:49 ` Antony Pavlov
@ 2012-11-08 19:57 ` Antony Pavlov
1 sibling, 0 replies; 5+ messages in thread
From: Antony Pavlov @ 2012-11-08 19:57 UTC (permalink / raw)
To: Robert Jarzmik; +Cc: barebox
On 8 November 2012 03:41, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
> Antony Pavlov <antonynpavlov@gmail.com> writes:
>
>> apply this commit after that one:
>>
>> Author: Robert Jarzmik <robert.jarzmik@free.fr>
>> Date: Sun Nov 4 18:55:23 2012 +0100
>>
>> commands: change Y-Modem implementation
>> ---
>> lib/xymodem.c | 13 +++++++------
>> 1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/lib/xymodem.c b/lib/xymodem.c
>> index 1469a9a..0e82ce2 100644
>> --- a/lib/xymodem.c
>> +++ b/lib/xymodem.c
>> @@ -257,8 +257,8 @@ static ssize_t xy_read_block(struct xyz_ctxt *proto, struct xy_block *blk,
>> uint64_t timeout)
>> {
>> ssize_t rc, data_len = 0;
>> - unsigned char hdr, seqs[2], crcs[2];
>> - int crc = 0, hdr_found = 0;
>> + unsigned char hdr, seqs[2];
>> + uint16_t crc = 0, hdr_found = 0;
>> uint64_t start = get_time_ns();
>>
>> while (!hdr_found) {
>> @@ -308,12 +308,13 @@ static ssize_t xy_read_block(struct xyz_ctxt *proto, struct xy_block *blk,
>>
>> switch (proto->crc_mode) {
>> case CRC_ADD8:
>> - rc = xy_gets(proto->cdev, proto->fifo, crcs, 1, timeout);
>> - crc = crcs[0];
>> + rc = xy_gets(proto->cdev, proto->fifo,
>> + (unsigned char *)&crc, 1, timeout);
> This doesn't look good to me.
> In big-endian arch, suppose you read 0xab as the crc.
> In that case, won't crc be equal to 0xab00 instead of 0x00ab in this code ? If
> that's the case, the previous code was right ...
>
>> break;
>> case CRC_CRC16:
>> - rc = xy_gets(proto->cdev, proto->fifo, crcs, 2, timeout);
>> - crc = be16_to_cpu(*(uint16_t *)crcs);
>> + rc = xy_gets(proto->cdev, proto->fifo,
>> + (unsigned char *)&crc, 2, timeout);
>> + crc = be16_to_cpu(crc);
>
> Does that mean that the code up there doesn't work on big endian ? I'm really
> puzzled, as the sender sends in big endian, the xy_gets() receives the bytes
> into crcs[2], and the be16_to_cpu() should be a no-op ...
>
> I think that we should do this simpler :
> crc = crcs[0] << 8 + crcs[1]
>
> I'll put that into my github branch for testing.
I have just tested the commits from your github 'xymodem' branch on my
big-endian and little-endian MIPS boards.
It works perfect!
Tested-by: Antony Pavlov <antonynpavlov@gmail.com>
--
Best regards,
Antony Pavlov
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-11-08 19:59 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-11-07 8:23 [PATCH] make new y-modem (PATCH v2) work on big-endian CPU Antony Pavlov
2012-11-07 23:41 ` Robert Jarzmik
2012-11-08 5:49 ` Antony Pavlov
2012-11-08 19:59 ` Robert Jarzmik
2012-11-08 19:57 ` Antony Pavlov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox