[PATCH u-boot-marvell 00/14] Another set of kwboot improvements

From: Marek Behún marek.behun@nic.cz
Hello Stefan,
here comes another set of kwboot improvements from Pali, reviewed and signed off by myself.
Marek
Pali Rohár (14): tools: kwboot: Increase blk_rsp_timeo to 2s tools: kwboot: Wait blk_rsp_timeo when flushing tools: kwboot: Improve retrying logic for incomplete xmodem packets tools: kwboot: Remove code for handling CAN byte tools: kwboot: Do not change received character in kwboot_xm_recv_reply() tools: kwboot: Fix handling of repeated xmodem packets tools: kwboot: Show 'E' in progress output when error occurs tools: kwboot: Allow to use option -b without image path tools: kwboot: Force BootROM to flush input queue after boot pattern tools: kwboot: Remove 2s delay before sending first xmodem packet tools: kwboot: Handle EINTR in kwboot_write() tools: kwboot: Handle EINTR in kwboot_tty_recv() tools: kwboot: Fix usage of -D without -t tools: kwboot: Set debug flag to 1
tools/kwboot.c | 154 ++++++++++++++++++++++++++++++++++++------------- 1 file changed, 115 insertions(+), 39 deletions(-)

From: Pali Rohár pali@kernel.org
Fix xmodem retry mechanism if some bytes from xmodem packet were lost and BootROM is still waiting for completing previous xmodem packet.
It is required to wait at least 1.312s on A385, otherwise BootROM does not accept next xmodem packet if previous one was not completely transferred.
2s should be enough timeout cause that BootROM will drop incomplete xmodem packet and expects new packet.
Signed-off-by: Pali Rohár pali@kernel.org Signed-off-by: Marek Behún marek.behun@nic.cz --- tools/kwboot.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/kwboot.c b/tools/kwboot.c index c3d8ab6544..82cfd9a827 100644 --- a/tools/kwboot.c +++ b/tools/kwboot.c @@ -75,7 +75,7 @@ struct kwboot_block { uint8_t csum; } __packed;
-#define KWBOOT_BLK_RSP_TIMEO 1000 /* ms */ +#define KWBOOT_BLK_RSP_TIMEO 2000 /* ms */ #define KWBOOT_HDR_RSP_TIMEO 10000 /* ms */
/* ARM code to change baudrate */

On 1/25/22 18:13, Marek Behún wrote:
From: Pali Rohár pali@kernel.org
Fix xmodem retry mechanism if some bytes from xmodem packet were lost and BootROM is still waiting for completing previous xmodem packet.
It is required to wait at least 1.312s on A385, otherwise BootROM does not accept next xmodem packet if previous one was not completely transferred.
2s should be enough timeout cause that BootROM will drop incomplete xmodem packet and expects new packet.
Signed-off-by: Pali Rohár pali@kernel.org Signed-off-by: Marek Behún marek.behun@nic.cz
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
tools/kwboot.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/kwboot.c b/tools/kwboot.c index c3d8ab6544..82cfd9a827 100644 --- a/tools/kwboot.c +++ b/tools/kwboot.c @@ -75,7 +75,7 @@ struct kwboot_block { uint8_t csum; } __packed;
-#define KWBOOT_BLK_RSP_TIMEO 1000 /* ms */ +#define KWBOOT_BLK_RSP_TIMEO 2000 /* ms */ #define KWBOOT_HDR_RSP_TIMEO 10000 /* ms */
/* ARM code to change baudrate */
Viele Grüße, Stefan Roese

From: Pali Rohár pali@kernel.org
Use the blk_rsp_timeo variable when sleeping before flushing tty.
Signed-off-by: Pali Rohár pali@kernel.org Signed-off-by: Marek Behún marek.behun@nic.cz --- tools/kwboot.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/kwboot.c b/tools/kwboot.c index 82cfd9a827..1477c0f078 100644 --- a/tools/kwboot.c +++ b/tools/kwboot.c @@ -1081,8 +1081,8 @@ kwboot_xmodem(int tty, const void *_img, size_t size, int baudrate) */ hdrsz += (KWBOOT_XM_BLKSZ - hdrsz % KWBOOT_XM_BLKSZ) % KWBOOT_XM_BLKSZ;
- kwboot_printv("Waiting 2s and flushing tty\n"); - sleep(2); /* flush isn't effective without it */ + kwboot_printv("Waiting %d ms and flushing tty\n", blk_rsp_timeo); + usleep(blk_rsp_timeo * 1000); tcflush(tty, TCIOFLUSH);
pnum = 1;

On 1/25/22 18:13, Marek Behún wrote:
From: Pali Rohár pali@kernel.org
Use the blk_rsp_timeo variable when sleeping before flushing tty.
Signed-off-by: Pali Rohár pali@kernel.org Signed-off-by: Marek Behún marek.behun@nic.cz
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
tools/kwboot.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/kwboot.c b/tools/kwboot.c index 82cfd9a827..1477c0f078 100644 --- a/tools/kwboot.c +++ b/tools/kwboot.c @@ -1081,8 +1081,8 @@ kwboot_xmodem(int tty, const void *_img, size_t size, int baudrate) */ hdrsz += (KWBOOT_XM_BLKSZ - hdrsz % KWBOOT_XM_BLKSZ) % KWBOOT_XM_BLKSZ;
- kwboot_printv("Waiting 2s and flushing tty\n");
- sleep(2); /* flush isn't effective without it */
kwboot_printv("Waiting %d ms and flushing tty\n", blk_rsp_timeo);
usleep(blk_rsp_timeo * 1000); tcflush(tty, TCIOFLUSH);
pnum = 1;
Viele Grüße, Stefan Roese

From: Pali Rohár pali@kernel.org
Sometimes if the first byte of xmodem packet (SOH) is incorrectly transmitted, BootROM sends NAK for every non-SOH received byte, which makes BootROM and the host kwboot tool out of sync. BootROM automatically re-synchronizes after 2s pause by dropping its input queue. So when attempting retransmit for 9th time or later, ignore NAK reply from BootROM and either wait for valid ACK or let kwboot timeout, which implies re-synchronization.
This fixes retransmission of xmodem packets and allows kwboot to work also without "Waiting ... and flushing tty" code which is at the beginning of kwboot xmodem transfer.
Signed-off-by: Pali Rohár pali@kernel.org Signed-off-by: Marek Behún marek.behun@nic.cz --- tools/kwboot.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/tools/kwboot.c b/tools/kwboot.c index 1477c0f078..be9a751406 100644 --- a/tools/kwboot.c +++ b/tools/kwboot.c @@ -880,6 +880,7 @@ kwboot_baud_magic_handle(int fd, char c, int baudrate)
static int kwboot_xm_recv_reply(int fd, char *c, int nak_on_non_xm, + int ignore_nak_reply, int allow_non_xm, int *non_xm_print, int baudrate, int *baud_changed) { @@ -899,8 +900,14 @@ kwboot_xm_recv_reply(int fd, char *c, int nak_on_non_xm, }
/* If received xmodem reply, end. */ - if (_is_xm_reply(*c)) + if (_is_xm_reply(*c)) { + if (*c == NAK && ignore_nak_reply) { + timeout = recv_until - _now(); + if (timeout >= 0) + continue; + } break; + }
/* * If receiving/printing non-xmodem text output is allowed and @@ -968,6 +975,7 @@ kwboot_xm_sendblock(int fd, struct kwboot_block *block, int allow_non_xm, }
rc = kwboot_xm_recv_reply(fd, &c, retries < 3, + retries > 8, allow_non_xm, &non_xm_print, baudrate, &baud_changed); if (rc) @@ -1011,6 +1019,7 @@ kwboot_xm_finish(int fd) return rc;
rc = kwboot_xm_recv_reply(fd, &c, retries < 3, + retries > 8, 0, NULL, 0, NULL); if (rc) return rc;

On 1/25/22 18:13, Marek Behún wrote:
From: Pali Rohár pali@kernel.org
Sometimes if the first byte of xmodem packet (SOH) is incorrectly transmitted, BootROM sends NAK for every non-SOH received byte, which makes BootROM and the host kwboot tool out of sync. BootROM automatically re-synchronizes after 2s pause by dropping its input queue. So when attempting retransmit for 9th time or later, ignore NAK reply from BootROM and either wait for valid ACK or let kwboot timeout, which implies re-synchronization.
This fixes retransmission of xmodem packets and allows kwboot to work also without "Waiting ... and flushing tty" code which is at the beginning of kwboot xmodem transfer.
Signed-off-by: Pali Rohár pali@kernel.org Signed-off-by: Marek Behún marek.behun@nic.cz
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
tools/kwboot.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/tools/kwboot.c b/tools/kwboot.c index 1477c0f078..be9a751406 100644 --- a/tools/kwboot.c +++ b/tools/kwboot.c @@ -880,6 +880,7 @@ kwboot_baud_magic_handle(int fd, char c, int baudrate)
static int kwboot_xm_recv_reply(int fd, char *c, int nak_on_non_xm,
{int ignore_nak_reply, int allow_non_xm, int *non_xm_print, int baudrate, int *baud_changed)
@@ -899,8 +900,14 @@ kwboot_xm_recv_reply(int fd, char *c, int nak_on_non_xm, }
/* If received xmodem reply, end. */
if (_is_xm_reply(*c))
if (_is_xm_reply(*c)) {
if (*c == NAK && ignore_nak_reply) {
timeout = recv_until - _now();
if (timeout >= 0)
continue;
} break;
}
/*
- If receiving/printing non-xmodem text output is allowed and
@@ -968,6 +975,7 @@ kwboot_xm_sendblock(int fd, struct kwboot_block *block, int allow_non_xm, }
rc = kwboot_xm_recv_reply(fd, &c, retries < 3,
if (rc)retries > 8, allow_non_xm, &non_xm_print, baudrate, &baud_changed);
@@ -1011,6 +1019,7 @@ kwboot_xm_finish(int fd) return rc;
rc = kwboot_xm_recv_reply(fd, &c, retries < 3,
if (rc) return rc;retries > 8, 0, NULL, 0, NULL);
Viele Grüße, Stefan Roese

From: Pali Rohár pali@kernel.org
It is unknown why handling of CAN byte was added into kwboot tool as Marvell BootROM does not support CAN byte. It never sends CAN byte to host and if host sends CAN byte BootROM handles it as an unknown byte.
Remove code for handling and sending CAN bytes from the kwboot tool.
Signed-off-by: Pali Rohár pali@kernel.org Signed-off-by: Marek Behún marek.behun@nic.cz --- tools/kwboot.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/tools/kwboot.c b/tools/kwboot.c index be9a751406..0b97990d09 100644 --- a/tools/kwboot.c +++ b/tools/kwboot.c @@ -63,7 +63,6 @@ static unsigned char kwboot_msg_debug[] = { #define EOT 4 /* sender end of block transfer */ #define ACK 6 /* target block ack */ #define NAK 21 /* target block negative ack */ -#define CAN 24 /* target/sender transfer cancellation */
#define KWBOOT_XM_BLKSZ 128 /* xmodem block size */
@@ -826,7 +825,7 @@ _now(void) static int _is_xm_reply(char c) { - return c == ACK || c == NAK || c == CAN; + return c == ACK || c == NAK; }
static int @@ -841,9 +840,6 @@ _xm_reply_to_error(int c) case NAK: errno = EBADMSG; break; - case CAN: - errno = ECANCELED; - break; default: errno = EPROTO; break; @@ -966,7 +962,7 @@ kwboot_xm_sendblock(int fd, struct kwboot_block *block, int allow_non_xm, do { rc = kwboot_tty_send(fd, block, sizeof(*block), 1); if (rc) - return rc; + goto err;
if (allow_non_xm && !*done_print) { kwboot_progress(100, '.'); @@ -979,7 +975,7 @@ kwboot_xm_sendblock(int fd, struct kwboot_block *block, int allow_non_xm, allow_non_xm, &non_xm_print, baudrate, &baud_changed); if (rc) - goto can; + goto err;
if (!allow_non_xm && c != ACK) kwboot_progress(-1, '+'); @@ -990,15 +986,13 @@ kwboot_xm_sendblock(int fd, struct kwboot_block *block, int allow_non_xm,
if (allow_non_xm && baudrate && !baud_changed) { fprintf(stderr, "Baudrate was not changed\n"); - rc = -1; errno = EPROTO; - goto can; + return -1; }
return _xm_reply_to_error(c); -can: +err: err = errno; - kwboot_tty_send_char(fd, CAN); kwboot_printv("\n"); errno = err; return rc;

On 1/25/22 18:13, Marek Behún wrote:
From: Pali Rohár pali@kernel.org
It is unknown why handling of CAN byte was added into kwboot tool as Marvell BootROM does not support CAN byte. It never sends CAN byte to host and if host sends CAN byte BootROM handles it as an unknown byte.
Remove code for handling and sending CAN bytes from the kwboot tool.
Signed-off-by: Pali Rohár pali@kernel.org Signed-off-by: Marek Behún marek.behun@nic.cz
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
tools/kwboot.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/tools/kwboot.c b/tools/kwboot.c index be9a751406..0b97990d09 100644 --- a/tools/kwboot.c +++ b/tools/kwboot.c @@ -63,7 +63,6 @@ static unsigned char kwboot_msg_debug[] = { #define EOT 4 /* sender end of block transfer */ #define ACK 6 /* target block ack */ #define NAK 21 /* target block negative ack */ -#define CAN 24 /* target/sender transfer cancellation */
#define KWBOOT_XM_BLKSZ 128 /* xmodem block size */
@@ -826,7 +825,7 @@ _now(void) static int _is_xm_reply(char c) {
- return c == ACK || c == NAK || c == CAN;
return c == ACK || c == NAK; }
static int
@@ -841,9 +840,6 @@ _xm_reply_to_error(int c) case NAK: errno = EBADMSG; break;
- case CAN:
errno = ECANCELED;
default: errno = EPROTO; break;break;
@@ -966,7 +962,7 @@ kwboot_xm_sendblock(int fd, struct kwboot_block *block, int allow_non_xm, do { rc = kwboot_tty_send(fd, block, sizeof(*block), 1); if (rc)
return rc;
goto err;
if (allow_non_xm && !*done_print) { kwboot_progress(100, '.');
@@ -979,7 +975,7 @@ kwboot_xm_sendblock(int fd, struct kwboot_block *block, int allow_non_xm, allow_non_xm, &non_xm_print, baudrate, &baud_changed); if (rc)
goto can;
goto err;
if (!allow_non_xm && c != ACK) kwboot_progress(-1, '+');
@@ -990,15 +986,13 @@ kwboot_xm_sendblock(int fd, struct kwboot_block *block, int allow_non_xm,
if (allow_non_xm && baudrate && !baud_changed) { fprintf(stderr, "Baudrate was not changed\n");
errno = EPROTO;rc = -1;
goto can;
return -1;
}
return _xm_reply_to_error(c);
-can: +err: err = errno;
- kwboot_tty_send_char(fd, CAN); kwboot_printv("\n"); errno = err; return rc;
Viele Grüße, Stefan Roese

From: Pali Rohár pali@kernel.org
Marvell BootROM expects retransmission of previous xmodem packet only in the case when it sends NAK response to the host.
Do not change non-xmodem response (possibly UART transfer error) to NAK in kwboot_xm_recv_reply() function. Allow caller to receive original response from device.
Change argument 'nak_on_non_xm' to 'stop_on_non_xm'. Instead of changing non-xmodem character to NAK, stop processing on invalid character and return it.
Signed-off-by: Pali Rohár pali@kernel.org Signed-off-by: Marek Behún marek.behun@nic.cz --- tools/kwboot.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/tools/kwboot.c b/tools/kwboot.c index 0b97990d09..a619a6c3c1 100644 --- a/tools/kwboot.c +++ b/tools/kwboot.c @@ -875,7 +875,7 @@ kwboot_baud_magic_handle(int fd, char c, int baudrate) }
static int -kwboot_xm_recv_reply(int fd, char *c, int nak_on_non_xm, +kwboot_xm_recv_reply(int fd, char *c, int stop_on_non_xm, int ignore_nak_reply, int allow_non_xm, int *non_xm_print, int baudrate, int *baud_changed) @@ -931,10 +931,8 @@ kwboot_xm_recv_reply(int fd, char *c, int nak_on_non_xm, *non_xm_print = 1; } } else { - if (nak_on_non_xm) { - *c = NAK; + if (stop_on_non_xm) break; - } timeout = recv_until - _now(); if (timeout < 0) { errno = ETIMEDOUT;

On 1/25/22 18:13, Marek Behún wrote:
From: Pali Rohár pali@kernel.org
Marvell BootROM expects retransmission of previous xmodem packet only in the case when it sends NAK response to the host.
Do not change non-xmodem response (possibly UART transfer error) to NAK in kwboot_xm_recv_reply() function. Allow caller to receive original response from device.
Change argument 'nak_on_non_xm' to 'stop_on_non_xm'. Instead of changing non-xmodem character to NAK, stop processing on invalid character and return it.
Signed-off-by: Pali Rohár pali@kernel.org Signed-off-by: Marek Behún marek.behun@nic.cz
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
tools/kwboot.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/tools/kwboot.c b/tools/kwboot.c index 0b97990d09..a619a6c3c1 100644 --- a/tools/kwboot.c +++ b/tools/kwboot.c @@ -875,7 +875,7 @@ kwboot_baud_magic_handle(int fd, char c, int baudrate) }
static int -kwboot_xm_recv_reply(int fd, char *c, int nak_on_non_xm, +kwboot_xm_recv_reply(int fd, char *c, int stop_on_non_xm, int ignore_nak_reply, int allow_non_xm, int *non_xm_print, int baudrate, int *baud_changed) @@ -931,10 +931,8 @@ kwboot_xm_recv_reply(int fd, char *c, int nak_on_non_xm, *non_xm_print = 1; } } else {
if (nak_on_non_xm) {
*c = NAK;
if (stop_on_non_xm) break;
} timeout = recv_until - _now(); if (timeout < 0) { errno = ETIMEDOUT;
Viele Grüße, Stefan Roese

From: Pali Rohár pali@kernel.org
Unfortunately during some stages of xmodem transfer, A385 BootROM is not able to handle repeated xmodem packets. So if an error occurs during that stage, stop the transfer and return failure.
Signed-off-by: Pali Rohár pali@kernel.org Signed-off-by: Marek Behún marek.behun@nic.cz --- tools/kwboot.c | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-)
diff --git a/tools/kwboot.c b/tools/kwboot.c index a619a6c3c1..dfac8aed7b 100644 --- a/tools/kwboot.c +++ b/tools/kwboot.c @@ -946,7 +946,7 @@ kwboot_xm_recv_reply(int fd, char *c, int stop_on_non_xm,
static int kwboot_xm_sendblock(int fd, struct kwboot_block *block, int allow_non_xm, - int *done_print, int baudrate) + int *done_print, int baudrate, int allow_retries) { int non_xm_print, baud_changed; int rc, err, retries; @@ -977,7 +977,7 @@ kwboot_xm_sendblock(int fd, struct kwboot_block *block, int allow_non_xm,
if (!allow_non_xm && c != ACK) kwboot_progress(-1, '+'); - } while (c == NAK && retries++ < 16); + } while (c == NAK && allow_retries && retries++ < 16);
if (non_xm_print) kwboot_printv("\n"); @@ -1044,8 +1044,30 @@ kwboot_xmodem_one(int tty, int *pnum, int header, const uint8_t *data,
last_block = (left <= blksz);
+ /* + * Handling of repeated xmodem packets is completely broken in + * Armada 385 BootROM - it completely ignores xmodem packet + * numbers, they are only used for checksum verification. + * BootROM can handle a retry of the xmodem packet only during + * the transmission of kwbimage header and only if BootROM + * itself sent NAK response to previous attempt (it does it on + * checksum failure). During the transmission of kwbimage data + * part, BootROM always expects next xmodem packet, even if it + * sent NAK to previous attempt - there is absolutely no way to + * repair incorrectly transmitted xmodem packet during kwbimage + * data part upload. Also, if kwboot receives non-ACK/NAK + * response (meaning that original BootROM response was damaged + * on UART) there is no way to detect if BootROM accepted xmodem + * packet or not and no way to check if kwboot could repeat the + * packet or not. + * + * Stop transfer and return failure if kwboot receives unknown + * reply if non-xmodem reply is not allowed (for all xmodem + * packets except the last header packet) or when non-ACK reply + * is received during data part transfer. + */ rc = kwboot_xm_sendblock(tty, &block, header && last_block, - &done_print, baudrate); + &done_print, baudrate, header); if (rc) goto out;

On 1/25/22 18:13, Marek Behún wrote:
From: Pali Rohár pali@kernel.org
Unfortunately during some stages of xmodem transfer, A385 BootROM is not able to handle repeated xmodem packets. So if an error occurs during that stage, stop the transfer and return failure.
Signed-off-by: Pali Rohár pali@kernel.org Signed-off-by: Marek Behún marek.behun@nic.cz
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
tools/kwboot.c | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-)
diff --git a/tools/kwboot.c b/tools/kwboot.c index a619a6c3c1..dfac8aed7b 100644 --- a/tools/kwboot.c +++ b/tools/kwboot.c @@ -946,7 +946,7 @@ kwboot_xm_recv_reply(int fd, char *c, int stop_on_non_xm,
static int kwboot_xm_sendblock(int fd, struct kwboot_block *block, int allow_non_xm,
int *done_print, int baudrate)
{ int non_xm_print, baud_changed; int rc, err, retries;int *done_print, int baudrate, int allow_retries)
@@ -977,7 +977,7 @@ kwboot_xm_sendblock(int fd, struct kwboot_block *block, int allow_non_xm,
if (!allow_non_xm && c != ACK) kwboot_progress(-1, '+');
- } while (c == NAK && retries++ < 16);
} while (c == NAK && allow_retries && retries++ < 16);
if (non_xm_print) kwboot_printv("\n");
@@ -1044,8 +1044,30 @@ kwboot_xmodem_one(int tty, int *pnum, int header, const uint8_t *data,
last_block = (left <= blksz);
/*
* Handling of repeated xmodem packets is completely broken in
* Armada 385 BootROM - it completely ignores xmodem packet
* numbers, they are only used for checksum verification.
* BootROM can handle a retry of the xmodem packet only during
* the transmission of kwbimage header and only if BootROM
* itself sent NAK response to previous attempt (it does it on
* checksum failure). During the transmission of kwbimage data
* part, BootROM always expects next xmodem packet, even if it
* sent NAK to previous attempt - there is absolutely no way to
* repair incorrectly transmitted xmodem packet during kwbimage
* data part upload. Also, if kwboot receives non-ACK/NAK
* response (meaning that original BootROM response was damaged
* on UART) there is no way to detect if BootROM accepted xmodem
* packet or not and no way to check if kwboot could repeat the
* packet or not.
*
* Stop transfer and return failure if kwboot receives unknown
* reply if non-xmodem reply is not allowed (for all xmodem
* packets except the last header packet) or when non-ACK reply
* is received during data part transfer.
rc = kwboot_xm_sendblock(tty, &block, header && last_block,*/
&done_print, baudrate);
if (rc) goto out;&done_print, baudrate, header);
Viele Grüße, Stefan Roese

From: Pali Rohár pali@kernel.org
When kwboot is unable to resend current xmodem packet, show an 'E' in the progress output instead of a '+'. This allows to distinguish between the state when kwboot is retrying sending the packet and when retry is not possible.
Signed-off-by: Pali Rohár pali@kernel.org Signed-off-by: Marek Behún marek.behun@nic.cz --- tools/kwboot.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/tools/kwboot.c b/tools/kwboot.c index dfac8aed7b..1dcec1969a 100644 --- a/tools/kwboot.c +++ b/tools/kwboot.c @@ -975,8 +975,12 @@ kwboot_xm_sendblock(int fd, struct kwboot_block *block, int allow_non_xm, if (rc) goto err;
- if (!allow_non_xm && c != ACK) - kwboot_progress(-1, '+'); + if (!allow_non_xm && c != ACK) { + if (c == NAK && allow_retries && retries + 1 < 16) + kwboot_progress(-1, '+'); + else + kwboot_progress(-1, 'E'); + } } while (c == NAK && allow_retries && retries++ < 16);
if (non_xm_print)

On 1/25/22 18:13, Marek Behún wrote:
From: Pali Rohár pali@kernel.org
When kwboot is unable to resend current xmodem packet, show an 'E' in the progress output instead of a '+'. This allows to distinguish between the state when kwboot is retrying sending the packet and when retry is not possible.
Signed-off-by: Pali Rohár pali@kernel.org Signed-off-by: Marek Behún marek.behun@nic.cz
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
tools/kwboot.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/tools/kwboot.c b/tools/kwboot.c index dfac8aed7b..1dcec1969a 100644 --- a/tools/kwboot.c +++ b/tools/kwboot.c @@ -975,8 +975,12 @@ kwboot_xm_sendblock(int fd, struct kwboot_block *block, int allow_non_xm, if (rc) goto err;
if (!allow_non_xm && c != ACK)
kwboot_progress(-1, '+');
if (!allow_non_xm && c != ACK) {
if (c == NAK && allow_retries && retries + 1 < 16)
kwboot_progress(-1, '+');
else
kwboot_progress(-1, 'E');
}
} while (c == NAK && allow_retries && retries++ < 16);
if (non_xm_print)
Viele Grüße, Stefan Roese

From: Pali Rohár pali@kernel.org
Allow option -b without image path parameter, to send boot pattern and wait for response but not send any image. This allows to use kwboot just for processing boot pattern and user can use any other xmodem tool for transferring the image itself (e.g. sx). Useful for debugging purposes.
Signed-off-by: Pali Rohár pali@kernel.org Signed-off-by: Marek Behún marek.behun@nic.cz --- tools/kwboot.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/tools/kwboot.c b/tools/kwboot.c index 1dcec1969a..c413a8bf51 100644 --- a/tools/kwboot.c +++ b/tools/kwboot.c @@ -1699,6 +1699,8 @@ main(int argc, char **argv) size_t size; size_t after_img_rsv; int baudrate; + int prev_optind; + int c;
rv = 1; tty = -1; @@ -1716,22 +1718,32 @@ main(int argc, char **argv) kwboot_verbose = isatty(STDOUT_FILENO);
do { - int c = getopt(argc, argv, "hb:ptaB:dD:q:s:o:"); + prev_optind = optind; + c = getopt(argc, argv, "hbptaB:dD:q:s:o:"); if (c < 0) break;
switch (c) { case 'b': + if (imgpath || bootmsg || debugmsg) + goto usage; bootmsg = kwboot_msg_boot; - imgpath = optarg; + if (prev_optind == optind) + goto usage; + if (argv[optind] && argv[optind][0] != '-') + imgpath = argv[optind++]; break;
case 'D': + if (imgpath || bootmsg || debugmsg) + goto usage; bootmsg = NULL; imgpath = optarg; break;
case 'd': + if (imgpath || bootmsg || debugmsg) + goto usage; debugmsg = kwboot_msg_debug; break;
@@ -1774,11 +1786,11 @@ main(int argc, char **argv) if (!bootmsg && !term && !debugmsg) goto usage;
- if (argc - optind < 1) - goto usage; - ttypath = argv[optind++];
+ if (optind != argc) + goto usage; + tty = kwboot_open_tty(ttypath, imgpath ? 115200 : baudrate); if (tty < 0) { perror(ttypath);

On 1/25/22 18:13, Marek Behún wrote:
From: Pali Rohár pali@kernel.org
Allow option -b without image path parameter, to send boot pattern and wait for response but not send any image. This allows to use kwboot just for processing boot pattern and user can use any other xmodem tool for transferring the image itself (e.g. sx). Useful for debugging purposes.
Signed-off-by: Pali Rohár pali@kernel.org Signed-off-by: Marek Behún marek.behun@nic.cz
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
tools/kwboot.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/tools/kwboot.c b/tools/kwboot.c index 1dcec1969a..c413a8bf51 100644 --- a/tools/kwboot.c +++ b/tools/kwboot.c @@ -1699,6 +1699,8 @@ main(int argc, char **argv) size_t size; size_t after_img_rsv; int baudrate;
int prev_optind;
int c;
rv = 1; tty = -1;
@@ -1716,22 +1718,32 @@ main(int argc, char **argv) kwboot_verbose = isatty(STDOUT_FILENO);
do {
int c = getopt(argc, argv, "hb:ptaB:dD:q:s:o:");
prev_optind = optind;
c = getopt(argc, argv, "hbptaB:dD:q:s:o:");
if (c < 0) break;
switch (c) { case 'b':
if (imgpath || bootmsg || debugmsg)
goto usage; bootmsg = kwboot_msg_boot;
imgpath = optarg;
if (prev_optind == optind)
goto usage;
if (argv[optind] && argv[optind][0] != '-')
imgpath = argv[optind++]; break;
case 'D':
if (imgpath || bootmsg || debugmsg)
goto usage; bootmsg = NULL; imgpath = optarg; break;
case 'd':
if (imgpath || bootmsg || debugmsg)
goto usage; debugmsg = kwboot_msg_debug; break;
@@ -1774,11 +1786,11 @@ main(int argc, char **argv) if (!bootmsg && !term && !debugmsg) goto usage;
- if (argc - optind < 1)
goto usage;
- ttypath = argv[optind++];
- if (optind != argc)
goto usage;
- tty = kwboot_open_tty(ttypath, imgpath ? 115200 : baudrate); if (tty < 0) { perror(ttypath);
Viele Grüße, Stefan Roese

From: Pali Rohár pali@kernel.org
Force the BootROM to flush its input queue after sending boot pattern.
This ensures that after function kwboot_bootmsg() finishes, BootROM is able to start receiving xmodem packets without any specific delay or setup.
Signed-off-by: Pali Rohár pali@kernel.org Signed-off-by: Marek Behún marek.behun@nic.cz --- tools/kwboot.c | 36 +++++++++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-)
diff --git a/tools/kwboot.c b/tools/kwboot.c index c413a8bf51..824ae005b2 100644 --- a/tools/kwboot.c +++ b/tools/kwboot.c @@ -717,6 +717,7 @@ out: static int kwboot_bootmsg(int tty, void *msg) { + struct kwboot_block block; int rc; char c; int count; @@ -747,7 +748,40 @@ kwboot_bootmsg(int tty, void *msg)
kwboot_printv("\n");
- return rc; + if (rc) + return rc; + + /* + * At this stage we have sent more boot message patterns and BootROM + * (at least on Armada XP and 385) started interpreting sent bytes as + * part of xmodem packets. If BootROM is expecting SOH byte as start of + * a xmodem packet and it receives byte 0xff, then it throws it away and + * sends a NAK reply to host. If BootROM does not receive any byte for + * 2s when expecting some continuation of the xmodem packet, it throws + * away the partially received xmodem data and sends NAK reply to host. + * + * Therefore for starting xmodem transfer we have two options: Either + * wait 2s or send 132 0xff bytes (which is the size of xmodem packet) + * to ensure that BootROM throws away any partially received data. + */ + + /* flush output queue with remaining boot message patterns */ + tcflush(tty, TCOFLUSH); + + /* send one xmodem packet with 0xff bytes to force BootROM to re-sync */ + memset(&block, 0xff, sizeof(block)); + kwboot_tty_send(tty, &block, sizeof(block), 0); + + /* + * Sending 132 bytes via 115200B/8-N-1 takes 11.45 ms, reading 132 bytes + * takes 11.45 ms, so waiting for 30 ms should be enough. + */ + usleep(30 * 1000); + + /* flush remaining NAK replies from input queue */ + tcflush(tty, TCIFLUSH); + + return 0; }
static int

On 1/25/22 18:13, Marek Behún wrote:
From: Pali Rohár pali@kernel.org
Force the BootROM to flush its input queue after sending boot pattern.
This ensures that after function kwboot_bootmsg() finishes, BootROM is able to start receiving xmodem packets without any specific delay or setup.
Signed-off-by: Pali Rohár pali@kernel.org Signed-off-by: Marek Behún marek.behun@nic.cz
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
tools/kwboot.c | 36 +++++++++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-)
diff --git a/tools/kwboot.c b/tools/kwboot.c index c413a8bf51..824ae005b2 100644 --- a/tools/kwboot.c +++ b/tools/kwboot.c @@ -717,6 +717,7 @@ out: static int kwboot_bootmsg(int tty, void *msg) {
- struct kwboot_block block; int rc; char c; int count;
@@ -747,7 +748,40 @@ kwboot_bootmsg(int tty, void *msg)
kwboot_printv("\n");
- return rc;
if (rc)
return rc;
/*
* At this stage we have sent more boot message patterns and BootROM
* (at least on Armada XP and 385) started interpreting sent bytes as
* part of xmodem packets. If BootROM is expecting SOH byte as start of
* a xmodem packet and it receives byte 0xff, then it throws it away and
* sends a NAK reply to host. If BootROM does not receive any byte for
* 2s when expecting some continuation of the xmodem packet, it throws
* away the partially received xmodem data and sends NAK reply to host.
*
* Therefore for starting xmodem transfer we have two options: Either
* wait 2s or send 132 0xff bytes (which is the size of xmodem packet)
* to ensure that BootROM throws away any partially received data.
*/
/* flush output queue with remaining boot message patterns */
tcflush(tty, TCOFLUSH);
/* send one xmodem packet with 0xff bytes to force BootROM to re-sync */
memset(&block, 0xff, sizeof(block));
kwboot_tty_send(tty, &block, sizeof(block), 0);
/*
* Sending 132 bytes via 115200B/8-N-1 takes 11.45 ms, reading 132 bytes
* takes 11.45 ms, so waiting for 30 ms should be enough.
*/
usleep(30 * 1000);
/* flush remaining NAK replies from input queue */
tcflush(tty, TCIFLUSH);
return 0; }
static int
Viele Grüße, Stefan Roese

From: Pali Rohár pali@kernel.org
This delay is not needed anymore since kwboot already handles retrying logic for incomplete xmodem packets and also forces BootROM to flush its input queue. Removing it decreases total transfer time.
Signed-off-by: Pali Rohár pali@kernel.org Signed-off-by: Marek Behún marek.behun@nic.cz --- tools/kwboot.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/tools/kwboot.c b/tools/kwboot.c index 824ae005b2..de433c1b04 100644 --- a/tools/kwboot.c +++ b/tools/kwboot.c @@ -1142,10 +1142,6 @@ kwboot_xmodem(int tty, const void *_img, size_t size, int baudrate) */ hdrsz += (KWBOOT_XM_BLKSZ - hdrsz % KWBOOT_XM_BLKSZ) % KWBOOT_XM_BLKSZ;
- kwboot_printv("Waiting %d ms and flushing tty\n", blk_rsp_timeo); - usleep(blk_rsp_timeo * 1000); - tcflush(tty, TCIOFLUSH); - pnum = 1;
rc = kwboot_xmodem_one(tty, &pnum, 1, img, hdrsz, baudrate);

On 1/25/22 18:13, Marek Behún wrote:
From: Pali Rohár pali@kernel.org
This delay is not needed anymore since kwboot already handles retrying logic for incomplete xmodem packets and also forces BootROM to flush its input queue. Removing it decreases total transfer time.
Signed-off-by: Pali Rohár pali@kernel.org Signed-off-by: Marek Behún marek.behun@nic.cz
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
tools/kwboot.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/tools/kwboot.c b/tools/kwboot.c index 824ae005b2..de433c1b04 100644 --- a/tools/kwboot.c +++ b/tools/kwboot.c @@ -1142,10 +1142,6 @@ kwboot_xmodem(int tty, const void *_img, size_t size, int baudrate) */ hdrsz += (KWBOOT_XM_BLKSZ - hdrsz % KWBOOT_XM_BLKSZ) % KWBOOT_XM_BLKSZ;
kwboot_printv("Waiting %d ms and flushing tty\n", blk_rsp_timeo);
usleep(blk_rsp_timeo * 1000);
tcflush(tty, TCIOFLUSH);
pnum = 1;
rc = kwboot_xmodem_one(tty, &pnum, 1, img, hdrsz, baudrate);
Viele Grüße, Stefan Roese

From: Pali Rohár pali@kernel.org
The write() syscall may be interrupted. Handle EINTR and retry it.
Signed-off-by: Pali Rohár pali@kernel.org Signed-off-by: Marek Behún marek.behun@nic.cz --- tools/kwboot.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/tools/kwboot.c b/tools/kwboot.c index de433c1b04..8b748f0fdd 100644 --- a/tools/kwboot.c +++ b/tools/kwboot.c @@ -292,13 +292,15 @@ static int blk_rsp_timeo = KWBOOT_BLK_RSP_TIMEO; static ssize_t kwboot_write(int fd, const char *buf, size_t len) { - size_t tot = 0; + ssize_t tot = 0;
while (tot < len) { ssize_t wr = write(fd, buf + tot, len - tot);
- if (wr < 0) - return -1; + if (wr < 0 && errno == EINTR) + continue; + else if (wr < 0) + return wr;
tot += wr; }

On 1/25/22 18:13, Marek Behún wrote:
From: Pali Rohár pali@kernel.org
The write() syscall may be interrupted. Handle EINTR and retry it.
Signed-off-by: Pali Rohár pali@kernel.org Signed-off-by: Marek Behún marek.behun@nic.cz
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
tools/kwboot.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/tools/kwboot.c b/tools/kwboot.c index de433c1b04..8b748f0fdd 100644 --- a/tools/kwboot.c +++ b/tools/kwboot.c @@ -292,13 +292,15 @@ static int blk_rsp_timeo = KWBOOT_BLK_RSP_TIMEO; static ssize_t kwboot_write(int fd, const char *buf, size_t len) {
- size_t tot = 0;
ssize_t tot = 0;
while (tot < len) { ssize_t wr = write(fd, buf + tot, len - tot);
if (wr < 0)
return -1;
if (wr < 0 && errno == EINTR)
continue;
else if (wr < 0)
return wr;
tot += wr; }
Viele Grüße, Stefan Roese

From: Pali Rohár pali@kernel.org
The select() and read() syscalls may be interrupted. Handle EINTR and retry them.
Signed-off-by: Pali Rohár pali@kernel.org Signed-off-by: Marek Behún marek.behun@nic.cz --- tools/kwboot.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/tools/kwboot.c b/tools/kwboot.c index 8b748f0fdd..fca1c73c55 100644 --- a/tools/kwboot.c +++ b/tools/kwboot.c @@ -409,15 +409,19 @@ kwboot_tty_recv(int fd, void *buf, size_t len, int timeo)
do { nfds = select(fd + 1, &rfds, NULL, NULL, &tv); - if (nfds < 0) + if (nfds < 0 && errno == EINTR) + continue; + else if (nfds < 0) goto out; - if (!nfds) { + else if (!nfds) { errno = ETIMEDOUT; goto out; }
n = read(fd, buf, len); - if (n <= 0) + if (n < 0 && errno == EINTR) + continue; + else if (n <= 0) goto out;
buf = (char *)buf + n;

On 1/25/22 18:13, Marek Behún wrote:
From: Pali Rohár pali@kernel.org
The select() and read() syscalls may be interrupted. Handle EINTR and retry them.
Signed-off-by: Pali Rohár pali@kernel.org Signed-off-by: Marek Behún marek.behun@nic.cz
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
tools/kwboot.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/tools/kwboot.c b/tools/kwboot.c index 8b748f0fdd..fca1c73c55 100644 --- a/tools/kwboot.c +++ b/tools/kwboot.c @@ -409,15 +409,19 @@ kwboot_tty_recv(int fd, void *buf, size_t len, int timeo)
do { nfds = select(fd + 1, &rfds, NULL, NULL, &tv);
if (nfds < 0)
if (nfds < 0 && errno == EINTR)
continue;
else if (nfds < 0) goto out;
if (!nfds) {
else if (!nfds) { errno = ETIMEDOUT; goto out;
}
n = read(fd, buf, len);
if (n <= 0)
if (n < 0 && errno == EINTR)
continue;
else if (n <= 0) goto out;
buf = (char *)buf + n;
Viele Grüße, Stefan Roese

From: Pali Rohár pali@kernel.org
When -D is specified then both bootmsg and debugmsg are not set, but imgpath is set. Fix this check for valid and required parameters.
Signed-off-by: Pali Rohár pali@kernel.org Signed-off-by: Marek Behún marek.behun@nic.cz --- tools/kwboot.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/kwboot.c b/tools/kwboot.c index fca1c73c55..859559fb72 100644 --- a/tools/kwboot.c +++ b/tools/kwboot.c @@ -1819,7 +1819,7 @@ main(int argc, char **argv) } } while (1);
- if (!bootmsg && !term && !debugmsg) + if (!bootmsg && !term && !debugmsg && !imgpath) goto usage;
ttypath = argv[optind++];

On 1/25/22 18:13, Marek Behún wrote:
From: Pali Rohár pali@kernel.org
When -D is specified then both bootmsg and debugmsg are not set, but imgpath is set. Fix this check for valid and required parameters.
Signed-off-by: Pali Rohár pali@kernel.org Signed-off-by: Marek Behún marek.behun@nic.cz
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
tools/kwboot.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/kwboot.c b/tools/kwboot.c index fca1c73c55..859559fb72 100644 --- a/tools/kwboot.c +++ b/tools/kwboot.c @@ -1819,7 +1819,7 @@ main(int argc, char **argv) } } while (1);
- if (!bootmsg && !term && !debugmsg)
if (!bootmsg && !term && !debugmsg && !imgpath) goto usage;
ttypath = argv[optind++];
Viele Grüße, Stefan Roese

From: Pali Rohár pali@kernel.org
This should enable BootROM output on UART.
(At least on A385 BootROM this is broken, BootROM ignores this debug flag and does not enable its output on UART if some valid image is available in SPI-NOR.)
Signed-off-by: Pali Rohár pali@kernel.org Signed-off-by: Marek Behún marek.behun@nic.cz --- tools/kwboot.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/tools/kwboot.c b/tools/kwboot.c index 859559fb72..2684f0e75a 100644 --- a/tools/kwboot.c +++ b/tools/kwboot.c @@ -1631,6 +1631,7 @@ kwboot_img_patch(void *img, size_t *size, int baudrate) * baudrate (which should be 115200) and do not touch * UART MPP configuration. */ + hdr->flags |= 0x1; hdr->options &= ~0x1F; hdr->options |= MAIN_HDR_V1_OPT_BAUD_DEFAULT; hdr->options |= 0 << 3;

On 1/25/22 18:13, Marek Behún wrote:
From: Pali Rohár pali@kernel.org
This should enable BootROM output on UART.
(At least on A385 BootROM this is broken, BootROM ignores this debug flag and does not enable its output on UART if some valid image is available in SPI-NOR.)
Signed-off-by: Pali Rohár pali@kernel.org Signed-off-by: Marek Behún marek.behun@nic.cz
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
tools/kwboot.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/tools/kwboot.c b/tools/kwboot.c index 859559fb72..2684f0e75a 100644 --- a/tools/kwboot.c +++ b/tools/kwboot.c @@ -1631,6 +1631,7 @@ kwboot_img_patch(void *img, size_t *size, int baudrate) * baudrate (which should be 115200) and do not touch * UART MPP configuration. */
hdr->flags |= 0x1; hdr->options &= ~0x1F; hdr->options |= MAIN_HDR_V1_OPT_BAUD_DEFAULT; hdr->options |= 0 << 3;
Viele Grüße, Stefan Roese

On 1/25/22 18:12, Marek Behún wrote:
From: Marek Behún marek.behun@nic.cz
Hello Stefan,
here comes another set of kwboot improvements from Pali, reviewed and signed off by myself.
Marek
Pali Rohár (14): tools: kwboot: Increase blk_rsp_timeo to 2s tools: kwboot: Wait blk_rsp_timeo when flushing tools: kwboot: Improve retrying logic for incomplete xmodem packets tools: kwboot: Remove code for handling CAN byte tools: kwboot: Do not change received character in kwboot_xm_recv_reply() tools: kwboot: Fix handling of repeated xmodem packets tools: kwboot: Show 'E' in progress output when error occurs tools: kwboot: Allow to use option -b without image path tools: kwboot: Force BootROM to flush input queue after boot pattern tools: kwboot: Remove 2s delay before sending first xmodem packet tools: kwboot: Handle EINTR in kwboot_write() tools: kwboot: Handle EINTR in kwboot_tty_recv() tools: kwboot: Fix usage of -D without -t tools: kwboot: Set debug flag to 1
tools/kwboot.c | 154 ++++++++++++++++++++++++++++++++++++------------- 1 file changed, 115 insertions(+), 39 deletions(-)
Applied to u-boot-marvell/master
Thanks, Stefan
participants (2)
-
Marek Behún
-
Stefan Roese