[PATCH u-boot-marvell 00/13] Yet another kwboot improvements

[PATCH u-boot-marvell 00/13] Yet another kwboot improvements

[Marek Behún]

From: Marek Behún <marek.behun@nic.cz>

Hello Stefan,

these are another improvements for kwboot, please apply only after series
  arm: mvebu: nandpagesize support for kwbimage v1

The main improvement is in patch 5, which changes where we inject the code
for changing baudrate back to 115200 Baud after fast upload. Instead of
injecting it before the main data image, we now inject it after.

This is because there are some kwb images that upload at address 0, and
injecting the code before that doesn't work, since there is no RAM mapped
at 0xfffff000.

Marek & Pali

Pali Rohár (13):
  tools: kwboot: Initialize rfds to zero
  tools: kwboot: Fix initialization of tty device
  tools: kwboot: Reserve enough space for patching kwbimage in memory
  tools: kwboot: Validate 4-byte image data checksum
  tools: kwboot: Inject baudrate change back code after data part
  tools: kwboot: Recalculate 4-byte data checksum after injecting
    baudrate code
  tools: kwboot: Correctly set configuration of UART for BootROM
    messages
  tools: kwboot: Show verbose message when waiting for baudrate change
    magic
  tools: kwboot: Simplify code for aligning image header
  tools: kwboot: Do not modify kwbimage header before increasing its
    size
  tools: kwboot: Calculate real used space in kwbimage header when
    calling kwboot_img_grow_hdr()
  tools: kwboot: Change retry loop from decreasing to increasing
  tools: kwboot: Resend first 3 xmodem retry packets immediately

 tools/kwboot.c | 178 +++++++++++++++++++++++++++++++++----------------
 1 file changed, 120 insertions(+), 58 deletions(-)

-- 
2.32.0

[PATCH u-boot-marvell 01/13] tools: kwboot: Initialize rfds to zero

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

Explicitly zero out the rfds fd_set with FD_ZERO() before using it.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-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 7e1be29623..695d433b96 100644
--- a/tools/kwboot.c
+++ b/tools/kwboot.c
@@ -1151,6 +1151,7 @@ kwboot_terminal(int tty)
 		fd_set rfds;
 		int nfds = 0;
 
+		FD_ZERO(&rfds);
 		FD_SET(tty, &rfds);
 		nfds = nfds < tty ? tty : nfds;
 
-- 
2.32.0

[PATCH u-boot-marvell 02/13] tools: kwboot: Fix initialization of tty device

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

Explicitly disable 2 stop bits by clearing CSTOPB flag, disable modem
control flow by clearing CRTSCTS flag and do not send hangup after closing
device by clearing HUPCL flag.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-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 695d433b96..c55b41025b 100644
--- a/tools/kwboot.c
+++ b/tools/kwboot.c
@@ -657,6 +657,7 @@ kwboot_open_tty(const char *path, int baudrate)
 
 	cfmakeraw(&tio);
 	tio.c_cflag |= CREAD | CLOCAL;
+	tio.c_cflag &= ~(CSTOPB | HUPCL | CRTSCTS);
 	tio.c_cc[VMIN] = 1;
 	tio.c_cc[VTIME] = 0;
 
-- 
2.32.0

[PATCH u-boot-marvell 03/13] tools: kwboot: Reserve enough space for patching kwbimage in memory

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

SPI image header and data parts do not have to be aligned to 128 byte
xmodem block size. So reserve additional memory for aligning header part
and additional memory for aligning data part.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 tools/kwboot.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/kwboot.c b/tools/kwboot.c
index c55b41025b..4e29317f10 100644
--- a/tools/kwboot.c
+++ b/tools/kwboot.c
@@ -1672,8 +1672,10 @@ main(int argc, char **argv)
 	else
 		/* ensure we have enough space for baudrate change code */
 		after_img_rsv += KWBOOT_BAUDRATE_BIN_HEADER_SZ +
+				 KWBOOT_XM_BLKSZ +
 				 sizeof(kwboot_pre_baud_code) +
-				 sizeof(kwboot_baud_code);
+				 sizeof(kwboot_baud_code) +
+				 KWBOOT_XM_BLKSZ;
 
 	if (imgpath) {
 		img = kwboot_read_image(imgpath, &size, after_img_rsv);
-- 
2.32.0

[PATCH u-boot-marvell 04/13] tools: kwboot: Validate 4-byte image data checksum

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

Data part of the image contains 4-byte checksum. Validate it when
processing the image.

Signed-off-by: Pali Rohár <pali@kernel.org>
[ refactored ]
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 tools/kwboot.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/tools/kwboot.c b/tools/kwboot.c
index 4e29317f10..bc44301535 100644
--- a/tools/kwboot.c
+++ b/tools/kwboot.c
@@ -1251,6 +1251,37 @@ kwboot_hdr_csum8(const void *hdr)
 	return csum;
 }
 
+static uint32_t *
+kwboot_img_csum32_ptr(void *img)
+{
+	struct main_hdr_v1 *hdr = img;
+	uint32_t datasz;
+
+	datasz = le32_to_cpu(hdr->blocksize) - sizeof(uint32_t);
+
+	return img + le32_to_cpu(hdr->srcaddr) + datasz;
+}
+
+static uint32_t
+kwboot_img_csum32(const void *img)
+{
+	const struct main_hdr_v1 *hdr = img;
+	uint32_t datasz, csum = 0;
+	const uint32_t *data;
+
+	datasz = le32_to_cpu(hdr->blocksize) - sizeof(csum);
+	if (datasz % sizeof(uint32_t))
+		return 0;
+
+	data = img + le32_to_cpu(hdr->srcaddr);
+	while (datasz > 0) {
+		csum += le32_to_cpu(*data++);
+		datasz -= 4;
+	}
+
+	return cpu_to_le32(csum);
+}
+
 static int
 kwboot_img_is_secure(void *img)
 {
@@ -1462,6 +1493,9 @@ kwboot_img_patch(void *img, size_t *size, int baudrate)
 	    *size < le32_to_cpu(hdr->srcaddr) + le32_to_cpu(hdr->blocksize))
 		goto err;
 
+	if (kwboot_img_csum32(img) != *kwboot_img_csum32_ptr(img))
+		goto err;
+
 	is_secure = kwboot_img_is_secure(img);
 
 	if (hdr->blockid != IBR_HDR_UART_ID) {
-- 
2.32.0

[PATCH u-boot-marvell 05/13] tools: kwboot: Inject baudrate change back code after data part

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

Some vendor U-Boot kwbimage binaries (e.g. those for A375) have load
address set to zero. Therefore it is not possible to inject code which
changes baudrate back to 115200 Bd before the data part.

So instead inject it after the data part and change kwbimage execution
address to that offset. Also store original execution address into
baudrate change code, so after it changes baudrate back to 115200 Bd, it
can jump to orignal address.

Signed-off-by: Pali Rohár <pali@kernel.org>
[ refactored ]
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 tools/kwboot.c | 72 ++++++++++++++++++++++----------------------------
 1 file changed, 31 insertions(+), 41 deletions(-)

diff --git a/tools/kwboot.c b/tools/kwboot.c
index bc44301535..bf26a667b7 100644
--- a/tools/kwboot.c
+++ b/tools/kwboot.c
@@ -1295,34 +1295,22 @@ kwboot_img_is_secure(void *img)
 }
 
 static void *
-kwboot_img_grow_data_left(void *img, size_t *size, size_t grow)
+kwboot_img_grow_data_right(void *img, size_t *size, size_t grow)
 {
-	uint32_t hdrsz, datasz, srcaddr;
 	struct main_hdr_v1 *hdr = img;
-	uint8_t *data;
-
-	srcaddr = le32_to_cpu(hdr->srcaddr);
-
-	hdrsz = kwbheader_size(hdr);
-	data = (uint8_t *)img + srcaddr;
-	datasz = *size - srcaddr;
-
-	/* only move data if there is not enough space */
-	if (hdrsz + grow > srcaddr) {
-		size_t need = hdrsz + grow - srcaddr;
-
-		/* move data by enough bytes */
-		memmove(data + need, data, datasz);
-		*size += need;
-		srcaddr += need;
-	}
+	void *result;
 
-	srcaddr -= grow;
-	hdr->srcaddr = cpu_to_le32(srcaddr);
-	hdr->destaddr = cpu_to_le32(le32_to_cpu(hdr->destaddr) - grow);
+	/*
+	 * 32-bit checksum comes after end of image code, so we will be putting
+	 * new code there. So we get this pointer and then increase data size
+	 * (since increasing data size changes kwboot_img_csum32_ptr() return
+	 *  value).
+	 */
+	result = kwboot_img_csum32_ptr(img);
 	hdr->blocksize = cpu_to_le32(le32_to_cpu(hdr->blocksize) + grow);
+	*size += grow;
 
-	return (uint8_t *)img + srcaddr;
+	return result;
 }
 
 static void
@@ -1400,14 +1388,20 @@ kwboot_add_bin_ohdr_v1(void *img, size_t *size, uint32_t binsz)
 }
 
 static void
-_copy_baudrate_change_code(struct main_hdr_v1 *hdr, void *dst, int pre,
-			   int old_baud, int new_baud)
+_inject_baudrate_change_code(void *img, size_t *size, int pre,
+			     int old_baud, int new_baud)
 {
-	size_t codesz = sizeof(kwboot_baud_code);
-	uint8_t *code = dst;
+	uint32_t codesz = sizeof(kwboot_baud_code);
+	struct main_hdr_v1 *hdr = img;
+	uint8_t *code;
 
 	if (pre) {
-		size_t presz = sizeof(kwboot_pre_baud_code);
+		uint32_t presz = sizeof(kwboot_pre_baud_code);
+		uint32_t orig_datasz;
+
+		orig_datasz = le32_to_cpu(hdr->blocksize) - sizeof(uint32_t);
+
+		code = kwboot_img_grow_data_right(img, size, presz + codesz);
 
 		/*
 		 * We need to prepend code that loads lr register with original
@@ -1421,9 +1415,12 @@ _copy_baudrate_change_code(struct main_hdr_v1 *hdr, void *dst, int pre,
 		memcpy(code, kwboot_pre_baud_code, presz);
 		*(uint32_t *)code = hdr->execaddr;
 
-		hdr->execaddr = cpu_to_le32(le32_to_cpu(hdr->destaddr) + 4);
+		hdr->execaddr = cpu_to_le32(le32_to_cpu(hdr->destaddr) +
+					    orig_datasz + 4);
 
 		code += presz;
+	} else {
+		code = kwboot_add_bin_ohdr_v1(img, size, codesz);
 	}
 
 	memcpy(code, kwboot_baud_code, codesz - 8);
@@ -1516,9 +1513,6 @@ kwboot_img_patch(void *img, size_t *size, int baudrate)
 	}
 
 	if (baudrate) {
-		uint32_t codesz = sizeof(kwboot_baud_code);
-		void *code;
-
 		if (image_ver == 0) {
 			fprintf(stderr,
 				"Cannot inject code for changing baudrate into v0 image header\n");
@@ -1539,20 +1533,16 @@ kwboot_img_patch(void *img, size_t *size, int baudrate)
 		 */
 		kwboot_printv("Injecting binary header code for changing baudrate to %d Bd\n",
 			      baudrate);
-
-		code = kwboot_add_bin_ohdr_v1(img, size, codesz);
-		_copy_baudrate_change_code(hdr, code, 0, 115200, baudrate);
+		_inject_baudrate_change_code(img, size, 0, 115200, baudrate);
 
 		/*
 		 * Now inject code that changes the baudrate back to 115200 Bd.
-		 * This code is prepended to the data part of the image, so it
-		 * is executed before U-Boot proper.
+		 * This code is appended after the data part of the image, and
+		 * execaddr is changed so that it is executed before U-Boot
+		 * proper.
 		 */
 		kwboot_printv("Injecting code for changing baudrate back\n");
-
-		codesz += sizeof(kwboot_pre_baud_code);
-		code = kwboot_img_grow_data_left(img, size, codesz);
-		_copy_baudrate_change_code(hdr, code, 1, baudrate, 115200);
+		_inject_baudrate_change_code(img, size, 1, baudrate, 115200);
 
 		/* recompute header size */
 		hdrsz = kwbheader_size(hdr);
-- 
2.32.0

[PATCH u-boot-marvell 06/13] tools: kwboot: Recalculate 4-byte data checksum after injecting baudrate code

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

If data part of image is modified, update 4-byte data checksum.

It looks like A385 BootROM does not verify this checksum for image
loaded via UART, but we do not know if other BootROMs are also ignoring
it. It is always better to provide correct checksum.

Signed-off-by: Pali Rohár <pali@kernel.org>
[ refactored ]
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 tools/kwboot.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/kwboot.c b/tools/kwboot.c
index bf26a667b7..1131c2eb1c 100644
--- a/tools/kwboot.c
+++ b/tools/kwboot.c
@@ -1544,6 +1544,9 @@ kwboot_img_patch(void *img, size_t *size, int baudrate)
 		kwboot_printv("Injecting code for changing baudrate back\n");
 		_inject_baudrate_change_code(img, size, 1, baudrate, 115200);
 
+		/* Update the 32-bit data checksum */
+		*kwboot_img_csum32_ptr(img) = kwboot_img_csum32(img);
+
 		/* recompute header size */
 		hdrsz = kwbheader_size(hdr);
 	}
-- 
2.32.0

[PATCH u-boot-marvell 07/13] tools: kwboot: Correctly set configuration of UART for BootROM messages

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

For kwbimage v1, tell BootROM to send BootROM messages to UART port number
0 (used also for UART booting) with default baudrate (which should be
115200) and do not touch UART MPP configuration.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 tools/kwboot.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/tools/kwboot.c b/tools/kwboot.c
index 1131c2eb1c..6228838228 100644
--- a/tools/kwboot.c
+++ b/tools/kwboot.c
@@ -1507,6 +1507,17 @@ kwboot_img_patch(void *img, size_t *size, int baudrate)
 	}
 
 	if (!is_secure) {
+		if (image_ver == 1) {
+			/*
+			 * Tell BootROM to send BootROM messages to UART port
+			 * number 0 (used also for UART booting) with default
+			 * baudrate (which should be 115200) and do not touch
+			 * UART MPP configuration.
+			 */
+			hdr->options &= ~0x1F;
+			hdr->options |= MAIN_HDR_V1_OPT_BAUD_DEFAULT;
+			hdr->options |= 0 << 3;
+		}
 		if (image_ver == 0)
 			((struct main_hdr_v0 *)img)->nandeccmode = IBR_HDR_ECC_DISABLED;
 		hdr->nandpagesize = 0;
-- 
2.32.0

[PATCH u-boot-marvell 08/13] tools: kwboot: Show verbose message when waiting for baudrate change magic

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

It is hard to debug why kwboot is failing when the last message is
'Finishing transfer' and no additional output. So show verbose message when
kwboot finished transfer and is waiting for baudrate change magic sequence.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-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 6228838228..7fd28aa754 100644
--- a/tools/kwboot.c
+++ b/tools/kwboot.c
@@ -1065,7 +1065,7 @@ kwboot_xmodem(int tty, const void *_img, size_t size, int baudrate)
 	if (baudrate) {
 		char buf[sizeof(kwb_baud_magic)];
 
-		/* Wait 1s for baudrate change magic */
+		kwboot_printv("Waiting 1s for baudrate change magic\n");
 		rc = kwboot_tty_recv(tty, buf, sizeof(buf), 1000);
 		if (rc)
 			return rc;
-- 
2.32.0

[PATCH u-boot-marvell 09/13] tools: kwboot: Simplify code for aligning image header

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

Expression (hdrsz % KWBOOT_XM_BLKSZ) is non-zero therefore expression
(KWBOOT_XM_BLKSZ - hdrsz % KWBOOT_XM_BLKSZ) is always less than value
KWBOOT_XM_BLKSZ. So there is no need to add another modulo. Also rename
variable `offset` to `grow` which better describes what is stored in
this variable.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 tools/kwboot.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/tools/kwboot.c b/tools/kwboot.c
index 7fd28aa754..adec4ec97d 100644
--- a/tools/kwboot.c
+++ b/tools/kwboot.c
@@ -1563,8 +1563,7 @@ kwboot_img_patch(void *img, size_t *size, int baudrate)
 	}
 
 	if (hdrsz % KWBOOT_XM_BLKSZ) {
-		size_t offset = (KWBOOT_XM_BLKSZ - hdrsz % KWBOOT_XM_BLKSZ) %
-				KWBOOT_XM_BLKSZ;
+		size_t grow = KWBOOT_XM_BLKSZ - hdrsz % KWBOOT_XM_BLKSZ;
 
 		if (is_secure) {
 			fprintf(stderr, "Cannot align image with secure header\n");
@@ -1572,7 +1571,7 @@ kwboot_img_patch(void *img, size_t *size, int baudrate)
 		}
 
 		kwboot_printv("Aligning image header to Xmodem block size\n");
-		kwboot_img_grow_hdr(img, size, offset);
+		kwboot_img_grow_hdr(img, size, grow);
 	}
 
 	hdr->checksum = kwboot_hdr_csum8(hdr) - csum;
-- 
2.32.0

[PATCH u-boot-marvell 10/13] tools: kwboot: Do not modify kwbimage header before increasing its size

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

This ensures that kwboot_img_grow_hdr() function still sees valid kwbimage
header.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 tools/kwboot.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/kwboot.c b/tools/kwboot.c
index adec4ec97d..bb7555369c 100644
--- a/tools/kwboot.c
+++ b/tools/kwboot.c
@@ -1352,17 +1352,18 @@ kwboot_add_bin_ohdr_v1(void *img, size_t *size, uint32_t binsz)
 	uint32_t num_args;
 	uint32_t offset;
 	uint32_t ohdrsz;
+	uint8_t *prev_ext;
 
 	if (hdr->ext & 0x1) {
 		for_each_opt_hdr_v1 (ohdr, img)
 			if (opt_hdr_v1_next(ohdr) == NULL)
 				break;
 
-		*opt_hdr_v1_ext(ohdr) |= 1;
-		ohdr = opt_hdr_v1_next(ohdr);
+		prev_ext = opt_hdr_v1_ext(ohdr);
+		ohdr = _opt_hdr_v1_next(ohdr);
 	} else {
-		hdr->ext |= 1;
 		ohdr = (void *)(hdr + 1);
+		prev_ext = &hdr->ext;
 	}
 
 	/*
@@ -1377,6 +1378,8 @@ kwboot_add_bin_ohdr_v1(void *img, size_t *size, uint32_t binsz)
 	ohdrsz = sizeof(*ohdr) + 4 + 4 * num_args + binsz + 4;
 	kwboot_img_grow_hdr(hdr, size, ohdrsz);
 
+	*prev_ext |= 1;
+
 	ohdr->headertype = OPT_HDR_V1_BINARY_TYPE;
 	ohdr->headersz_msb = ohdrsz >> 16;
 	ohdr->headersz_lsb = cpu_to_le16(ohdrsz & 0xffff);
-- 
2.32.0

[PATCH u-boot-marvell 11/13] tools: kwboot: Calculate real used space in kwbimage header when calling kwboot_img_grow_hdr()

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

Size of the header stored in kwbimage may be larger than real used size in
the kwbimage header. If there is unused space in kwbimage header then use
it for growing it. So update code to calculate used space of kwbimage
header.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 tools/kwboot.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/tools/kwboot.c b/tools/kwboot.c
index bb7555369c..5d7cb7a774 100644
--- a/tools/kwboot.c
+++ b/tools/kwboot.c
@@ -1318,11 +1318,20 @@ kwboot_img_grow_hdr(void *img, size_t *size, size_t grow)
 {
 	uint32_t hdrsz, datasz, srcaddr;
 	struct main_hdr_v1 *hdr = img;
+	struct opt_hdr_v1 *ohdr;
 	uint8_t *data;
 
 	srcaddr = le32_to_cpu(hdr->srcaddr);
 
-	hdrsz = kwbheader_size(img);
+	/* calculate real used space in kwbimage header */
+	if (kwbimage_version(img) == 0) {
+		hdrsz = kwbheader_size(img);
+	} else {
+		hdrsz = sizeof(*hdr);
+		for_each_opt_hdr_v1 (ohdr, hdr)
+			hdrsz += opt_hdr_v1_size(ohdr);
+	}
+
 	data = (uint8_t *)img + srcaddr;
 	datasz = *size - srcaddr;
 
@@ -1339,8 +1348,10 @@ kwboot_img_grow_hdr(void *img, size_t *size, size_t grow)
 
 	if (kwbimage_version(img) == 1) {
 		hdrsz += grow;
-		hdr->headersz_msb = hdrsz >> 16;
-		hdr->headersz_lsb = cpu_to_le16(hdrsz & 0xffff);
+		if (hdrsz > kwbheader_size(img)) {
+			hdr->headersz_msb = hdrsz >> 16;
+			hdr->headersz_lsb = cpu_to_le16(hdrsz & 0xffff);
+		}
 	}
 }
 
-- 
2.32.0

[PATCH u-boot-marvell 12/13] tools: kwboot: Change retry loop from decreasing to increasing

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

This patch does not change behavior of the code, just allows to implement
new changes more easily.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 tools/kwboot.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/kwboot.c b/tools/kwboot.c
index 5d7cb7a774..16c5a84825 100644
--- a/tools/kwboot.c
+++ b/tools/kwboot.c
@@ -925,7 +925,7 @@ kwboot_xm_sendblock(int fd, struct kwboot_block *block, int allow_non_xm,
 
 	*done_print = 0;
 
-	retries = 16;
+	retries = 0;
 	do {
 		rc = kwboot_tty_send(fd, block, sizeof(*block));
 		if (rc)
@@ -944,7 +944,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-- > 0);
+	} while (c == NAK && retries++ < 16);
 
 	if (non_xm_print)
 		kwboot_printv("\n");
@@ -973,7 +973,7 @@ kwboot_xm_finish(int fd)
 
 	kwboot_printv("Finishing transfer\n");
 
-	retries = 16;
+	retries = 0;
 	do {
 		rc = kwboot_tty_send_char(fd, EOT);
 		if (rc)
@@ -982,7 +982,7 @@ kwboot_xm_finish(int fd)
 		rc = kwboot_xm_recv_reply(fd, &c, 0, NULL, 0, NULL);
 		if (rc)
 			return rc;
-	} while (c == NAK && retries-- > 0);
+	} while (c == NAK && retries++ < 16);
 
 	return _xm_reply_to_error(c);
 }
-- 
2.32.0

[PATCH u-boot-marvell 13/13] tools: kwboot: Resend first 3 xmodem retry packets immediately

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

Currently when kwboot receive some garbage reply which does not understand,
it waits 1s before it tries to resend packet again.

The most common error on UART is that receiver sees some bit flipped which
results in invalid reply.

This behavior slows down xmodem transfer over UART as basically on every
error kwboot is waiting one second.

To fix this, try to resend xmodem packet for first 3 attempts immediately
without any delay. If broken reply is received also after the 3 attempts,
continue retrying with 1s delay like it was before.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 tools/kwboot.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/tools/kwboot.c b/tools/kwboot.c
index 16c5a84825..bb7cae9f05 100644
--- a/tools/kwboot.c
+++ b/tools/kwboot.c
@@ -851,7 +851,8 @@ kwboot_baud_magic_handle(int fd, char c, int baudrate)
 }
 
 static int
-kwboot_xm_recv_reply(int fd, char *c, int allow_non_xm, int *non_xm_print,
+kwboot_xm_recv_reply(int fd, char *c, int nak_on_non_xm,
+		     int allow_non_xm, int *non_xm_print,
 		     int baudrate, int *baud_changed)
 {
 	int timeout = allow_non_xm ? KWBOOT_HDR_RSP_TIMEO : blk_rsp_timeo;
@@ -904,6 +905,10 @@ kwboot_xm_recv_reply(int fd, char *c, int allow_non_xm, int *non_xm_print,
 				*non_xm_print = 1;
 			}
 		} else {
+			if (nak_on_non_xm) {
+				*c = NAK;
+				break;
+			}
 			timeout = recv_until - _now();
 			if (timeout < 0) {
 				errno = ETIMEDOUT;
@@ -937,7 +942,8 @@ kwboot_xm_sendblock(int fd, struct kwboot_block *block, int allow_non_xm,
 			*done_print = 1;
 		}
 
-		rc = kwboot_xm_recv_reply(fd, &c, allow_non_xm, &non_xm_print,
+		rc = kwboot_xm_recv_reply(fd, &c, retries < 3,
+					  allow_non_xm, &non_xm_print,
 					  baudrate, &baud_changed);
 		if (rc)
 			goto can;
@@ -979,7 +985,8 @@ kwboot_xm_finish(int fd)
 		if (rc)
 			return rc;
 
-		rc = kwboot_xm_recv_reply(fd, &c, 0, NULL, 0, NULL);
+		rc = kwboot_xm_recv_reply(fd, &c, retries < 3,
+					  0, NULL, 0, NULL);
 		if (rc)
 			return rc;
 	} while (c == NAK && retries++ < 16);
-- 
2.32.0

Re: [PATCH u-boot-marvell 00/13] Yet another kwboot improvements

[Stefan Roese]

Hi Marek,

On 25.10.21 15:12, Marek Behún wrote:
> From: Marek Behún <marek.behun@nic.cz>
> 
> Hello Stefan,
> 
> these are another improvements for kwboot, please apply only after series
>    arm: mvebu: nandpagesize support for kwbimage v1

I'm checking right now and have applied the 3 NAND patches on current
master. But this patchset fails at this one:

tools: kwboot: Do not modify kwbimage header before increasing its size

[stefan@ryzen u-boot-marvell (kwboot-test1)]$ git am -3 ~/tmp/kwboot2/*
Applying: tools: kwboot: Initialize rfds to zero
Applying: tools: kwboot: Fix initialization of tty device
Applying: tools: kwboot: Reserve enough space for patching kwbimage in 
memory
Applying: tools: kwboot: Validate 4-byte image data checksum
Applying: tools: kwboot: Inject baudrate change back code after data part
Applying: tools: kwboot: Recalculate 4-byte data checksum after 
injecting baudrate code
Applying: tools: kwboot: Correctly set configuration of UART for BootROM 
messages
Applying: tools: kwboot: Show verbose message when waiting for baudrate 
change magic
Applying: tools: kwboot: Simplify code for aligning image header
Applying: tools: kwboot: Do not modify kwbimage header before increasing 
its size
error: sha1 information is lacking or useless (tools/kwboot.c).
error: could not build fake ancestor
Patch failed at 0010 tools: kwboot: Do not modify kwbimage header before 
increasing its size
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Any idea what's missing here?

Thanks,
Stefan

> The main improvement is in patch 5, which changes where we inject the code
> for changing baudrate back to 115200 Baud after fast upload. Instead of
> injecting it before the main data image, we now inject it after.
> 
> This is because there are some kwb images that upload at address 0, and
> injecting the code before that doesn't work, since there is no RAM mapped
> at 0xfffff000.
> 
> Marek & Pali
> 
> Pali Rohár (13):
>    tools: kwboot: Initialize rfds to zero
>    tools: kwboot: Fix initialization of tty device
>    tools: kwboot: Reserve enough space for patching kwbimage in memory
>    tools: kwboot: Validate 4-byte image data checksum
>    tools: kwboot: Inject baudrate change back code after data part
>    tools: kwboot: Recalculate 4-byte data checksum after injecting
>      baudrate code
>    tools: kwboot: Correctly set configuration of UART for BootROM
>      messages
>    tools: kwboot: Show verbose message when waiting for baudrate change
>      magic
>    tools: kwboot: Simplify code for aligning image header
>    tools: kwboot: Do not modify kwbimage header before increasing its
>      size
>    tools: kwboot: Calculate real used space in kwbimage header when
>      calling kwboot_img_grow_hdr()
>    tools: kwboot: Change retry loop from decreasing to increasing
>    tools: kwboot: Resend first 3 xmodem retry packets immediately
> 
>   tools/kwboot.c | 178 +++++++++++++++++++++++++++++++++----------------
>   1 file changed, 120 insertions(+), 58 deletions(-)
> 


Viele Grüße,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 00/13] Yet another kwboot improvements

[Pali Rohár]

On Monday 25 October 2021 16:39:44 Stefan Roese wrote:
> Hi Marek,
> 
> On 25.10.21 15:12, Marek Behún wrote:
> > From: Marek Behún <marek.behun@nic.cz>
> > 
> > Hello Stefan,
> > 
> > these are another improvements for kwboot, please apply only after series
> >    arm: mvebu: nandpagesize support for kwbimage v1
> 
> I'm checking right now and have applied the 3 NAND patches on current
> master. But this patchset fails at this one:
> 
> tools: kwboot: Do not modify kwbimage header before increasing its size
> 
> [stefan@ryzen u-boot-marvell (kwboot-test1)]$ git am -3 ~/tmp/kwboot2/*
> Applying: tools: kwboot: Initialize rfds to zero
> Applying: tools: kwboot: Fix initialization of tty device
> Applying: tools: kwboot: Reserve enough space for patching kwbimage in
> memory
> Applying: tools: kwboot: Validate 4-byte image data checksum
> Applying: tools: kwboot: Inject baudrate change back code after data part
> Applying: tools: kwboot: Recalculate 4-byte data checksum after injecting
> baudrate code
> Applying: tools: kwboot: Correctly set configuration of UART for BootROM
> messages
> Applying: tools: kwboot: Show verbose message when waiting for baudrate
> change magic
> Applying: tools: kwboot: Simplify code for aligning image header
> Applying: tools: kwboot: Do not modify kwbimage header before increasing its
> size
> error: sha1 information is lacking or useless (tools/kwboot.c).
> error: could not build fake ancestor
> Patch failed at 0010 tools: kwboot: Do not modify kwbimage header before
> increasing its size
> hint: Use 'git am --show-current-patch=diff' to see the failed patch
> When you have resolved this problem, run "git am --continue".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".
> 
> Any idea what's missing here?

Hello! I'm using also this patch:
https://patchwork.ozlabs.org/project/uboot/patch/20211021144609.9319-2-pali@kernel.org/

> Thanks,
> Stefan
> 
> > The main improvement is in patch 5, which changes where we inject the code
> > for changing baudrate back to 115200 Baud after fast upload. Instead of
> > injecting it before the main data image, we now inject it after.
> > 
> > This is because there are some kwb images that upload at address 0, and
> > injecting the code before that doesn't work, since there is no RAM mapped
> > at 0xfffff000.
> > 
> > Marek & Pali
> > 
> > Pali Rohár (13):
> >    tools: kwboot: Initialize rfds to zero
> >    tools: kwboot: Fix initialization of tty device
> >    tools: kwboot: Reserve enough space for patching kwbimage in memory
> >    tools: kwboot: Validate 4-byte image data checksum
> >    tools: kwboot: Inject baudrate change back code after data part
> >    tools: kwboot: Recalculate 4-byte data checksum after injecting
> >      baudrate code
> >    tools: kwboot: Correctly set configuration of UART for BootROM
> >      messages
> >    tools: kwboot: Show verbose message when waiting for baudrate change
> >      magic
> >    tools: kwboot: Simplify code for aligning image header
> >    tools: kwboot: Do not modify kwbimage header before increasing its
> >      size
> >    tools: kwboot: Calculate real used space in kwbimage header when
> >      calling kwboot_img_grow_hdr()
> >    tools: kwboot: Change retry loop from decreasing to increasing
> >    tools: kwboot: Resend first 3 xmodem retry packets immediately
> > 
> >   tools/kwboot.c | 178 +++++++++++++++++++++++++++++++++----------------
> >   1 file changed, 120 insertions(+), 58 deletions(-)
> > 
> 
> 
> Viele Grüße,
> Stefan
> 
> -- 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 00/13] Yet another kwboot improvements

[Stefan Roese]

Hi Pali,

On 25.10.21 16:42, Pali Rohár wrote:
> On Monday 25 October 2021 16:39:44 Stefan Roese wrote:
>> Hi Marek,
>>
>> On 25.10.21 15:12, Marek Behún wrote:
>>> From: Marek Behún <marek.behun@nic.cz>
>>>
>>> Hello Stefan,
>>>
>>> these are another improvements for kwboot, please apply only after series
>>>     arm: mvebu: nandpagesize support for kwbimage v1
>>
>> I'm checking right now and have applied the 3 NAND patches on current
>> master. But this patchset fails at this one:
>>
>> tools: kwboot: Do not modify kwbimage header before increasing its size
>>
>> [stefan@ryzen u-boot-marvell (kwboot-test1)]$ git am -3 ~/tmp/kwboot2/*
>> Applying: tools: kwboot: Initialize rfds to zero
>> Applying: tools: kwboot: Fix initialization of tty device
>> Applying: tools: kwboot: Reserve enough space for patching kwbimage in
>> memory
>> Applying: tools: kwboot: Validate 4-byte image data checksum
>> Applying: tools: kwboot: Inject baudrate change back code after data part
>> Applying: tools: kwboot: Recalculate 4-byte data checksum after injecting
>> baudrate code
>> Applying: tools: kwboot: Correctly set configuration of UART for BootROM
>> messages
>> Applying: tools: kwboot: Show verbose message when waiting for baudrate
>> change magic
>> Applying: tools: kwboot: Simplify code for aligning image header
>> Applying: tools: kwboot: Do not modify kwbimage header before increasing its
>> size
>> error: sha1 information is lacking or useless (tools/kwboot.c).
>> error: could not build fake ancestor
>> Patch failed at 0010 tools: kwboot: Do not modify kwbimage header before
>> increasing its size
>> hint: Use 'git am --show-current-patch=diff' to see the failed patch
>> When you have resolved this problem, run "git am --continue".
>> If you prefer to skip this patch, run "git am --skip" instead.
>> To restore the original branch and stop patching, run "git am --abort".
>>
>> Any idea what's missing here?
> 
> Hello! I'm using also this patch:
> https://patchwork.ozlabs.org/project/uboot/patch/20211021144609.9319-2-pali@kernel.org/

Ah, yes. That does the trick. Now all patches apply clean. Thanks.

Testing with all these patches on my AXP target does show, it still
does not work with baudrate > 115k:

[stefan@ryzen u-boot-marvell (kwboot-test1)]$ ./tools/kwboot -B 230400 
-b u-boot-spl.kwb -t 
/dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
Patching image boot signature to UART
Injecting binary header code for changing baudrate to 230400 Bd
Injecting code for changing baudrate back
Sending boot message. Please reboot the target...|
Waiting 2s and flushing tty
Sending boot image header (90112 bytes)...
   0 % 
[......................................................................]
  10 % 
[......................................................................]
  20 % 
[......................................................................]
  29 % 
[......................................................................]
  39 % 
[......................................................................]
  49 % 
[......................................................................]
  59 % 
[......................................................................]
  69 % 
[......................................................................]
  79 % 
[......................................................................]
  89 % 
[......................................................................]
  99 % [.... 
       ]
Done

U-Boot SPL 2021.10-00908-gc129aa2f173a (Oct 25 2021 - 17:10:55 +0200)
High speed PHY - Version: 2.1.5 (COM-PHY-V20)
High speed PHY - Ended Successfully
DDR3 Training Sequence - Ver 5.7.4
DDR3 Training Sequence - Ended Successfully
Trying to boot from BOOTROM
Returning to BootROM (return address 0xffff0aa0)...

Changing baudrate to 230400 Bd
Baudrate was not changed


xmodem: Protocol error
[stefan@ryzen u-boot-marvell (kwboot-test1)]$


Not changing the baudrate still works. Any idea what I should test? Or
do you have further changes in the queue that I should wait upon?

Thanks,
Stefan

Re: [PATCH u-boot-marvell 01/13] tools: kwboot: Initialize rfds to zero

[Stefan Roese]

On 25.10.21 15:12, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> Explicitly zero out the rfds fd_set with FD_ZERO() before using it.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-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 7e1be29623..695d433b96 100644
> --- a/tools/kwboot.c
> +++ b/tools/kwboot.c
> @@ -1151,6 +1151,7 @@ kwboot_terminal(int tty)
>   		fd_set rfds;
>   		int nfds = 0;
>   
> +		FD_ZERO(&rfds);
>   		FD_SET(tty, &rfds);
>   		nfds = nfds < tty ? tty : nfds;
>   
> 


Viele Grüße,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 02/13] tools: kwboot: Fix initialization of tty device

[Stefan Roese]

On 25.10.21 15:12, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> Explicitly disable 2 stop bits by clearing CSTOPB flag, disable modem
> control flow by clearing CRTSCTS flag and do not send hangup after closing
> device by clearing HUPCL flag.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-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 695d433b96..c55b41025b 100644
> --- a/tools/kwboot.c
> +++ b/tools/kwboot.c
> @@ -657,6 +657,7 @@ kwboot_open_tty(const char *path, int baudrate)
>   
>   	cfmakeraw(&tio);
>   	tio.c_cflag |= CREAD | CLOCAL;
> +	tio.c_cflag &= ~(CSTOPB | HUPCL | CRTSCTS);
>   	tio.c_cc[VMIN] = 1;
>   	tio.c_cc[VTIME] = 0;
>   
> 


Viele Grüße,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 03/13] tools: kwboot: Reserve enough space for patching kwbimage in memory

[Stefan Roese]

On 25.10.21 15:12, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> SPI image header and data parts do not have to be aligned to 128 byte
> xmodem block size. So reserve additional memory for aligning header part
> and additional memory for aligning data part.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <marek.behun@nic.cz>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   tools/kwboot.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/kwboot.c b/tools/kwboot.c
> index c55b41025b..4e29317f10 100644
> --- a/tools/kwboot.c
> +++ b/tools/kwboot.c
> @@ -1672,8 +1672,10 @@ main(int argc, char **argv)
>   	else
>   		/* ensure we have enough space for baudrate change code */
>   		after_img_rsv += KWBOOT_BAUDRATE_BIN_HEADER_SZ +
> +				 KWBOOT_XM_BLKSZ +
>   				 sizeof(kwboot_pre_baud_code) +
> -				 sizeof(kwboot_baud_code);
> +				 sizeof(kwboot_baud_code) +
> +				 KWBOOT_XM_BLKSZ;
>   
>   	if (imgpath) {
>   		img = kwboot_read_image(imgpath, &size, after_img_rsv);
> 


Viele Grüße,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 04/13] tools: kwboot: Validate 4-byte image data checksum

[Stefan Roese]

On 25.10.21 15:12, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> Data part of the image contains 4-byte checksum. Validate it when
> processing the image.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> [ refactored ]
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   tools/kwboot.c | 34 ++++++++++++++++++++++++++++++++++
>   1 file changed, 34 insertions(+)
> 
> diff --git a/tools/kwboot.c b/tools/kwboot.c
> index 4e29317f10..bc44301535 100644
> --- a/tools/kwboot.c
> +++ b/tools/kwboot.c
> @@ -1251,6 +1251,37 @@ kwboot_hdr_csum8(const void *hdr)
>   	return csum;
>   }
>   
> +static uint32_t *
> +kwboot_img_csum32_ptr(void *img)
> +{
> +	struct main_hdr_v1 *hdr = img;
> +	uint32_t datasz;
> +
> +	datasz = le32_to_cpu(hdr->blocksize) - sizeof(uint32_t);
> +
> +	return img + le32_to_cpu(hdr->srcaddr) + datasz;
> +}
> +
> +static uint32_t
> +kwboot_img_csum32(const void *img)
> +{
> +	const struct main_hdr_v1 *hdr = img;
> +	uint32_t datasz, csum = 0;
> +	const uint32_t *data;
> +
> +	datasz = le32_to_cpu(hdr->blocksize) - sizeof(csum);
> +	if (datasz % sizeof(uint32_t))
> +		return 0;
> +
> +	data = img + le32_to_cpu(hdr->srcaddr);
> +	while (datasz > 0) {
> +		csum += le32_to_cpu(*data++);
> +		datasz -= 4;
> +	}
> +
> +	return cpu_to_le32(csum);
> +}
> +
>   static int
>   kwboot_img_is_secure(void *img)
>   {
> @@ -1462,6 +1493,9 @@ kwboot_img_patch(void *img, size_t *size, int baudrate)
>   	    *size < le32_to_cpu(hdr->srcaddr) + le32_to_cpu(hdr->blocksize))
>   		goto err;
>   
> +	if (kwboot_img_csum32(img) != *kwboot_img_csum32_ptr(img))
> +		goto err;
> +
>   	is_secure = kwboot_img_is_secure(img);
>   
>   	if (hdr->blockid != IBR_HDR_UART_ID) {
> 


Viele Grüße,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 05/13] tools: kwboot: Inject baudrate change back code after data part

[Stefan Roese]

On 25.10.21 15:12, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> Some vendor U-Boot kwbimage binaries (e.g. those for A375) have load
> address set to zero. Therefore it is not possible to inject code which
> changes baudrate back to 115200 Bd before the data part.
> 
> So instead inject it after the data part and change kwbimage execution
> address to that offset. Also store original execution address into
> baudrate change code, so after it changes baudrate back to 115200 Bd, it
> can jump to orignal address.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> [ refactored ]
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   tools/kwboot.c | 72 ++++++++++++++++++++++----------------------------
>   1 file changed, 31 insertions(+), 41 deletions(-)
> 
> diff --git a/tools/kwboot.c b/tools/kwboot.c
> index bc44301535..bf26a667b7 100644
> --- a/tools/kwboot.c
> +++ b/tools/kwboot.c
> @@ -1295,34 +1295,22 @@ kwboot_img_is_secure(void *img)
>   }
>   
>   static void *
> -kwboot_img_grow_data_left(void *img, size_t *size, size_t grow)
> +kwboot_img_grow_data_right(void *img, size_t *size, size_t grow)
>   {
> -	uint32_t hdrsz, datasz, srcaddr;
>   	struct main_hdr_v1 *hdr = img;
> -	uint8_t *data;
> -
> -	srcaddr = le32_to_cpu(hdr->srcaddr);
> -
> -	hdrsz = kwbheader_size(hdr);
> -	data = (uint8_t *)img + srcaddr;
> -	datasz = *size - srcaddr;
> -
> -	/* only move data if there is not enough space */
> -	if (hdrsz + grow > srcaddr) {
> -		size_t need = hdrsz + grow - srcaddr;
> -
> -		/* move data by enough bytes */
> -		memmove(data + need, data, datasz);
> -		*size += need;
> -		srcaddr += need;
> -	}
> +	void *result;
>   
> -	srcaddr -= grow;
> -	hdr->srcaddr = cpu_to_le32(srcaddr);
> -	hdr->destaddr = cpu_to_le32(le32_to_cpu(hdr->destaddr) - grow);
> +	/*
> +	 * 32-bit checksum comes after end of image code, so we will be putting
> +	 * new code there. So we get this pointer and then increase data size
> +	 * (since increasing data size changes kwboot_img_csum32_ptr() return
> +	 *  value).
> +	 */
> +	result = kwboot_img_csum32_ptr(img);
>   	hdr->blocksize = cpu_to_le32(le32_to_cpu(hdr->blocksize) + grow);
> +	*size += grow;
>   
> -	return (uint8_t *)img + srcaddr;
> +	return result;
>   }
>   
>   static void
> @@ -1400,14 +1388,20 @@ kwboot_add_bin_ohdr_v1(void *img, size_t *size, uint32_t binsz)
>   }
>   
>   static void
> -_copy_baudrate_change_code(struct main_hdr_v1 *hdr, void *dst, int pre,
> -			   int old_baud, int new_baud)
> +_inject_baudrate_change_code(void *img, size_t *size, int pre,
> +			     int old_baud, int new_baud)
>   {
> -	size_t codesz = sizeof(kwboot_baud_code);
> -	uint8_t *code = dst;
> +	uint32_t codesz = sizeof(kwboot_baud_code);
> +	struct main_hdr_v1 *hdr = img;
> +	uint8_t *code;
>   
>   	if (pre) {
> -		size_t presz = sizeof(kwboot_pre_baud_code);
> +		uint32_t presz = sizeof(kwboot_pre_baud_code);
> +		uint32_t orig_datasz;
> +
> +		orig_datasz = le32_to_cpu(hdr->blocksize) - sizeof(uint32_t);
> +
> +		code = kwboot_img_grow_data_right(img, size, presz + codesz);
>   
>   		/*
>   		 * We need to prepend code that loads lr register with original
> @@ -1421,9 +1415,12 @@ _copy_baudrate_change_code(struct main_hdr_v1 *hdr, void *dst, int pre,
>   		memcpy(code, kwboot_pre_baud_code, presz);
>   		*(uint32_t *)code = hdr->execaddr;
>   
> -		hdr->execaddr = cpu_to_le32(le32_to_cpu(hdr->destaddr) + 4);
> +		hdr->execaddr = cpu_to_le32(le32_to_cpu(hdr->destaddr) +
> +					    orig_datasz + 4);
>   
>   		code += presz;
> +	} else {
> +		code = kwboot_add_bin_ohdr_v1(img, size, codesz);
>   	}
>   
>   	memcpy(code, kwboot_baud_code, codesz - 8);
> @@ -1516,9 +1513,6 @@ kwboot_img_patch(void *img, size_t *size, int baudrate)
>   	}
>   
>   	if (baudrate) {
> -		uint32_t codesz = sizeof(kwboot_baud_code);
> -		void *code;
> -
>   		if (image_ver == 0) {
>   			fprintf(stderr,
>   				"Cannot inject code for changing baudrate into v0 image header\n");
> @@ -1539,20 +1533,16 @@ kwboot_img_patch(void *img, size_t *size, int baudrate)
>   		 */
>   		kwboot_printv("Injecting binary header code for changing baudrate to %d Bd\n",
>   			      baudrate);
> -
> -		code = kwboot_add_bin_ohdr_v1(img, size, codesz);
> -		_copy_baudrate_change_code(hdr, code, 0, 115200, baudrate);
> +		_inject_baudrate_change_code(img, size, 0, 115200, baudrate);
>   
>   		/*
>   		 * Now inject code that changes the baudrate back to 115200 Bd.
> -		 * This code is prepended to the data part of the image, so it
> -		 * is executed before U-Boot proper.
> +		 * This code is appended after the data part of the image, and
> +		 * execaddr is changed so that it is executed before U-Boot
> +		 * proper.
>   		 */
>   		kwboot_printv("Injecting code for changing baudrate back\n");
> -
> -		codesz += sizeof(kwboot_pre_baud_code);
> -		code = kwboot_img_grow_data_left(img, size, codesz);
> -		_copy_baudrate_change_code(hdr, code, 1, baudrate, 115200);
> +		_inject_baudrate_change_code(img, size, 1, baudrate, 115200);
>   
>   		/* recompute header size */
>   		hdrsz = kwbheader_size(hdr);
> 


Viele Grüße,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 06/13] tools: kwboot: Recalculate 4-byte data checksum after injecting baudrate code

[Stefan Roese]

On 25.10.21 15:12, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> If data part of image is modified, update 4-byte data checksum.
> 
> It looks like A385 BootROM does not verify this checksum for image
> loaded via UART, but we do not know if other BootROMs are also ignoring
> it. It is always better to provide correct checksum.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> [ refactored ]
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   tools/kwboot.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/tools/kwboot.c b/tools/kwboot.c
> index bf26a667b7..1131c2eb1c 100644
> --- a/tools/kwboot.c
> +++ b/tools/kwboot.c
> @@ -1544,6 +1544,9 @@ kwboot_img_patch(void *img, size_t *size, int baudrate)
>   		kwboot_printv("Injecting code for changing baudrate back\n");
>   		_inject_baudrate_change_code(img, size, 1, baudrate, 115200);
>   
> +		/* Update the 32-bit data checksum */
> +		*kwboot_img_csum32_ptr(img) = kwboot_img_csum32(img);
> +
>   		/* recompute header size */
>   		hdrsz = kwbheader_size(hdr);
>   	}
> 


Viele Grüße,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 07/13] tools: kwboot: Correctly set configuration of UART for BootROM messages

[Stefan Roese]

On 25.10.21 15:12, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> For kwbimage v1, tell BootROM to send BootROM messages to UART port number
> 0 (used also for UART booting) with default baudrate (which should be
> 115200) and do not touch UART MPP configuration.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <marek.behun@nic.cz>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   tools/kwboot.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/tools/kwboot.c b/tools/kwboot.c
> index 1131c2eb1c..6228838228 100644
> --- a/tools/kwboot.c
> +++ b/tools/kwboot.c
> @@ -1507,6 +1507,17 @@ kwboot_img_patch(void *img, size_t *size, int baudrate)
>   	}
>   
>   	if (!is_secure) {
> +		if (image_ver == 1) {
> +			/*
> +			 * Tell BootROM to send BootROM messages to UART port
> +			 * number 0 (used also for UART booting) with default
> +			 * baudrate (which should be 115200) and do not touch
> +			 * UART MPP configuration.
> +			 */
> +			hdr->options &= ~0x1F;
> +			hdr->options |= MAIN_HDR_V1_OPT_BAUD_DEFAULT;
> +			hdr->options |= 0 << 3;
> +		}
>   		if (image_ver == 0)
>   			((struct main_hdr_v0 *)img)->nandeccmode = IBR_HDR_ECC_DISABLED;
>   		hdr->nandpagesize = 0;
> 


Viele Grüße,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 08/13] tools: kwboot: Show verbose message when waiting for baudrate change magic

[Stefan Roese]

On 25.10.21 15:12, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> It is hard to debug why kwboot is failing when the last message is
> 'Finishing transfer' and no additional output. So show verbose message when
> kwboot finished transfer and is waiting for baudrate change magic sequence.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-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 6228838228..7fd28aa754 100644
> --- a/tools/kwboot.c
> +++ b/tools/kwboot.c
> @@ -1065,7 +1065,7 @@ kwboot_xmodem(int tty, const void *_img, size_t size, int baudrate)
>   	if (baudrate) {
>   		char buf[sizeof(kwb_baud_magic)];
>   
> -		/* Wait 1s for baudrate change magic */
> +		kwboot_printv("Waiting 1s for baudrate change magic\n");
>   		rc = kwboot_tty_recv(tty, buf, sizeof(buf), 1000);
>   		if (rc)
>   			return rc;
> 


Viele Grüße,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 09/13] tools: kwboot: Simplify code for aligning image header

[Stefan Roese]

On 25.10.21 15:13, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> Expression (hdrsz % KWBOOT_XM_BLKSZ) is non-zero therefore expression
> (KWBOOT_XM_BLKSZ - hdrsz % KWBOOT_XM_BLKSZ) is always less than value
> KWBOOT_XM_BLKSZ. So there is no need to add another modulo. Also rename
> variable `offset` to `grow` which better describes what is stored in
> this variable.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <marek.behun@nic.cz>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   tools/kwboot.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/kwboot.c b/tools/kwboot.c
> index 7fd28aa754..adec4ec97d 100644
> --- a/tools/kwboot.c
> +++ b/tools/kwboot.c
> @@ -1563,8 +1563,7 @@ kwboot_img_patch(void *img, size_t *size, int baudrate)
>   	}
>   
>   	if (hdrsz % KWBOOT_XM_BLKSZ) {
> -		size_t offset = (KWBOOT_XM_BLKSZ - hdrsz % KWBOOT_XM_BLKSZ) %
> -				KWBOOT_XM_BLKSZ;
> +		size_t grow = KWBOOT_XM_BLKSZ - hdrsz % KWBOOT_XM_BLKSZ;
>   
>   		if (is_secure) {
>   			fprintf(stderr, "Cannot align image with secure header\n");
> @@ -1572,7 +1571,7 @@ kwboot_img_patch(void *img, size_t *size, int baudrate)
>   		}
>   
>   		kwboot_printv("Aligning image header to Xmodem block size\n");
> -		kwboot_img_grow_hdr(img, size, offset);
> +		kwboot_img_grow_hdr(img, size, grow);
>   	}
>   
>   	hdr->checksum = kwboot_hdr_csum8(hdr) - csum;
> 


Viele Grüße,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 10/13] tools: kwboot: Do not modify kwbimage header before increasing its size

[Stefan Roese]

On 25.10.21 15:13, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> This ensures that kwboot_img_grow_hdr() function still sees valid kwbimage
> header.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <marek.behun@nic.cz>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   tools/kwboot.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/kwboot.c b/tools/kwboot.c
> index adec4ec97d..bb7555369c 100644
> --- a/tools/kwboot.c
> +++ b/tools/kwboot.c
> @@ -1352,17 +1352,18 @@ kwboot_add_bin_ohdr_v1(void *img, size_t *size, uint32_t binsz)
>   	uint32_t num_args;
>   	uint32_t offset;
>   	uint32_t ohdrsz;
> +	uint8_t *prev_ext;
>   
>   	if (hdr->ext & 0x1) {
>   		for_each_opt_hdr_v1 (ohdr, img)
>   			if (opt_hdr_v1_next(ohdr) == NULL)
>   				break;
>   
> -		*opt_hdr_v1_ext(ohdr) |= 1;
> -		ohdr = opt_hdr_v1_next(ohdr);
> +		prev_ext = opt_hdr_v1_ext(ohdr);
> +		ohdr = _opt_hdr_v1_next(ohdr);
>   	} else {
> -		hdr->ext |= 1;
>   		ohdr = (void *)(hdr + 1);
> +		prev_ext = &hdr->ext;
>   	}
>   
>   	/*
> @@ -1377,6 +1378,8 @@ kwboot_add_bin_ohdr_v1(void *img, size_t *size, uint32_t binsz)
>   	ohdrsz = sizeof(*ohdr) + 4 + 4 * num_args + binsz + 4;
>   	kwboot_img_grow_hdr(hdr, size, ohdrsz);
>   
> +	*prev_ext |= 1;
> +
>   	ohdr->headertype = OPT_HDR_V1_BINARY_TYPE;
>   	ohdr->headersz_msb = ohdrsz >> 16;
>   	ohdr->headersz_lsb = cpu_to_le16(ohdrsz & 0xffff);
> 


Viele Grüße,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 11/13] tools: kwboot: Calculate real used space in kwbimage header when calling kwboot_img_grow_hdr()

[Stefan Roese]

On 25.10.21 15:13, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> Size of the header stored in kwbimage may be larger than real used size in
> the kwbimage header. If there is unused space in kwbimage header then use
> it for growing it. So update code to calculate used space of kwbimage
> header.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <marek.behun@nic.cz>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   tools/kwboot.c | 17 ++++++++++++++---
>   1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/kwboot.c b/tools/kwboot.c
> index bb7555369c..5d7cb7a774 100644
> --- a/tools/kwboot.c
> +++ b/tools/kwboot.c
> @@ -1318,11 +1318,20 @@ kwboot_img_grow_hdr(void *img, size_t *size, size_t grow)
>   {
>   	uint32_t hdrsz, datasz, srcaddr;
>   	struct main_hdr_v1 *hdr = img;
> +	struct opt_hdr_v1 *ohdr;
>   	uint8_t *data;
>   
>   	srcaddr = le32_to_cpu(hdr->srcaddr);
>   
> -	hdrsz = kwbheader_size(img);
> +	/* calculate real used space in kwbimage header */
> +	if (kwbimage_version(img) == 0) {
> +		hdrsz = kwbheader_size(img);
> +	} else {
> +		hdrsz = sizeof(*hdr);
> +		for_each_opt_hdr_v1 (ohdr, hdr)
> +			hdrsz += opt_hdr_v1_size(ohdr);
> +	}
> +
>   	data = (uint8_t *)img + srcaddr;
>   	datasz = *size - srcaddr;
>   
> @@ -1339,8 +1348,10 @@ kwboot_img_grow_hdr(void *img, size_t *size, size_t grow)
>   
>   	if (kwbimage_version(img) == 1) {
>   		hdrsz += grow;
> -		hdr->headersz_msb = hdrsz >> 16;
> -		hdr->headersz_lsb = cpu_to_le16(hdrsz & 0xffff);
> +		if (hdrsz > kwbheader_size(img)) {
> +			hdr->headersz_msb = hdrsz >> 16;
> +			hdr->headersz_lsb = cpu_to_le16(hdrsz & 0xffff);
> +		}
>   	}
>   }
>   
> 


Viele Grüße,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 12/13] tools: kwboot: Change retry loop from decreasing to increasing

[Stefan Roese]

On 25.10.21 15:13, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> This patch does not change behavior of the code, just allows to implement
> new changes more easily.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <marek.behun@nic.cz>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   tools/kwboot.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/kwboot.c b/tools/kwboot.c
> index 5d7cb7a774..16c5a84825 100644
> --- a/tools/kwboot.c
> +++ b/tools/kwboot.c
> @@ -925,7 +925,7 @@ kwboot_xm_sendblock(int fd, struct kwboot_block *block, int allow_non_xm,
>   
>   	*done_print = 0;
>   
> -	retries = 16;
> +	retries = 0;
>   	do {
>   		rc = kwboot_tty_send(fd, block, sizeof(*block));
>   		if (rc)
> @@ -944,7 +944,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-- > 0);
> +	} while (c == NAK && retries++ < 16);
>   
>   	if (non_xm_print)
>   		kwboot_printv("\n");
> @@ -973,7 +973,7 @@ kwboot_xm_finish(int fd)
>   
>   	kwboot_printv("Finishing transfer\n");
>   
> -	retries = 16;
> +	retries = 0;
>   	do {
>   		rc = kwboot_tty_send_char(fd, EOT);
>   		if (rc)
> @@ -982,7 +982,7 @@ kwboot_xm_finish(int fd)
>   		rc = kwboot_xm_recv_reply(fd, &c, 0, NULL, 0, NULL);
>   		if (rc)
>   			return rc;
> -	} while (c == NAK && retries-- > 0);
> +	} while (c == NAK && retries++ < 16);
>   
>   	return _xm_reply_to_error(c);
>   }
> 


Viele Grüße,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 13/13] tools: kwboot: Resend first 3 xmodem retry packets immediately

[Stefan Roese]

On 25.10.21 15:13, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> Currently when kwboot receive some garbage reply which does not understand,
> it waits 1s before it tries to resend packet again.
> 
> The most common error on UART is that receiver sees some bit flipped which
> results in invalid reply.
> 
> This behavior slows down xmodem transfer over UART as basically on every
> error kwboot is waiting one second.
> 
> To fix this, try to resend xmodem packet for first 3 attempts immediately
> without any delay. If broken reply is received also after the 3 attempts,
> continue retrying with 1s delay like it was before.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <marek.behun@nic.cz>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   tools/kwboot.c | 13 ++++++++++---
>   1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/kwboot.c b/tools/kwboot.c
> index 16c5a84825..bb7cae9f05 100644
> --- a/tools/kwboot.c
> +++ b/tools/kwboot.c
> @@ -851,7 +851,8 @@ kwboot_baud_magic_handle(int fd, char c, int baudrate)
>   }
>   
>   static int
> -kwboot_xm_recv_reply(int fd, char *c, int allow_non_xm, int *non_xm_print,
> +kwboot_xm_recv_reply(int fd, char *c, int nak_on_non_xm,
> +		     int allow_non_xm, int *non_xm_print,
>   		     int baudrate, int *baud_changed)
>   {
>   	int timeout = allow_non_xm ? KWBOOT_HDR_RSP_TIMEO : blk_rsp_timeo;
> @@ -904,6 +905,10 @@ kwboot_xm_recv_reply(int fd, char *c, int allow_non_xm, int *non_xm_print,
>   				*non_xm_print = 1;
>   			}
>   		} else {
> +			if (nak_on_non_xm) {
> +				*c = NAK;
> +				break;
> +			}
>   			timeout = recv_until - _now();
>   			if (timeout < 0) {
>   				errno = ETIMEDOUT;
> @@ -937,7 +942,8 @@ kwboot_xm_sendblock(int fd, struct kwboot_block *block, int allow_non_xm,
>   			*done_print = 1;
>   		}
>   
> -		rc = kwboot_xm_recv_reply(fd, &c, allow_non_xm, &non_xm_print,
> +		rc = kwboot_xm_recv_reply(fd, &c, retries < 3,
> +					  allow_non_xm, &non_xm_print,
>   					  baudrate, &baud_changed);
>   		if (rc)
>   			goto can;
> @@ -979,7 +985,8 @@ kwboot_xm_finish(int fd)
>   		if (rc)
>   			return rc;
>   
> -		rc = kwboot_xm_recv_reply(fd, &c, 0, NULL, 0, NULL);
> +		rc = kwboot_xm_recv_reply(fd, &c, retries < 3,
> +					  0, NULL, 0, NULL);
>   		if (rc)
>   			return rc;
>   	} while (c == NAK && retries++ < 16);
> 


Viele Grüße,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 00/13] Yet another kwboot improvements

[Pali Rohár]

On Monday 25 October 2021 17:15:14 Stefan Roese wrote:
> Hi Pali,
> 
> On 25.10.21 16:42, Pali Rohár wrote:
> > On Monday 25 October 2021 16:39:44 Stefan Roese wrote:
> > > Hi Marek,
> > > 
> > > On 25.10.21 15:12, Marek Behún wrote:
> > > > From: Marek Behún <marek.behun@nic.cz>
> > > > 
> > > > Hello Stefan,
> > > > 
> > > > these are another improvements for kwboot, please apply only after series
> > > >     arm: mvebu: nandpagesize support for kwbimage v1
> > > 
> > > I'm checking right now and have applied the 3 NAND patches on current
> > > master. But this patchset fails at this one:
> > > 
> > > tools: kwboot: Do not modify kwbimage header before increasing its size
> > > 
> > > [stefan@ryzen u-boot-marvell (kwboot-test1)]$ git am -3 ~/tmp/kwboot2/*
> > > Applying: tools: kwboot: Initialize rfds to zero
> > > Applying: tools: kwboot: Fix initialization of tty device
> > > Applying: tools: kwboot: Reserve enough space for patching kwbimage in
> > > memory
> > > Applying: tools: kwboot: Validate 4-byte image data checksum
> > > Applying: tools: kwboot: Inject baudrate change back code after data part
> > > Applying: tools: kwboot: Recalculate 4-byte data checksum after injecting
> > > baudrate code
> > > Applying: tools: kwboot: Correctly set configuration of UART for BootROM
> > > messages
> > > Applying: tools: kwboot: Show verbose message when waiting for baudrate
> > > change magic
> > > Applying: tools: kwboot: Simplify code for aligning image header
> > > Applying: tools: kwboot: Do not modify kwbimage header before increasing its
> > > size
> > > error: sha1 information is lacking or useless (tools/kwboot.c).
> > > error: could not build fake ancestor
> > > Patch failed at 0010 tools: kwboot: Do not modify kwbimage header before
> > > increasing its size
> > > hint: Use 'git am --show-current-patch=diff' to see the failed patch
> > > When you have resolved this problem, run "git am --continue".
> > > If you prefer to skip this patch, run "git am --skip" instead.
> > > To restore the original branch and stop patching, run "git am --abort".
> > > 
> > > Any idea what's missing here?
> > 
> > Hello! I'm using also this patch:
> > https://patchwork.ozlabs.org/project/uboot/patch/20211021144609.9319-2-pali@kernel.org/
> 
> Ah, yes. That does the trick. Now all patches apply clean. Thanks.
> 
> Testing with all these patches on my AXP target does show, it still
> does not work with baudrate > 115k:
> 
> [stefan@ryzen u-boot-marvell (kwboot-test1)]$ ./tools/kwboot -B 230400 -b
> u-boot-spl.kwb -t
> /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
> Patching image boot signature to UART
> Injecting binary header code for changing baudrate to 230400 Bd
> Injecting code for changing baudrate back
> Sending boot message. Please reboot the target...|
> Waiting 2s and flushing tty
> Sending boot image header (90112 bytes)...
>   0 %
> [......................................................................]
>  10 %
> [......................................................................]
>  20 %
> [......................................................................]
>  29 %
> [......................................................................]
>  39 %
> [......................................................................]
>  49 %
> [......................................................................]
>  59 %
> [......................................................................]
>  69 %
> [......................................................................]
>  79 %
> [......................................................................]
>  89 %
> [......................................................................]
>  99 % [....       ]
> Done
> 
> U-Boot SPL 2021.10-00908-gc129aa2f173a (Oct 25 2021 - 17:10:55 +0200)
> High speed PHY - Version: 2.1.5 (COM-PHY-V20)
> High speed PHY - Ended Successfully
> DDR3 Training Sequence - Ver 5.7.4
> DDR3 Training Sequence - Ended Successfully
> Trying to boot from BOOTROM
> Returning to BootROM (return address 0xffff0aa0)...
> 
> Changing baudrate to 230400 Bd
> Baudrate was not changed
> 
> 
> xmodem: Protocol error
> [stefan@ryzen u-boot-marvell (kwboot-test1)]$
> 
> 
> Not changing the baudrate still works. Any idea what I should test? Or
> do you have further changes in the queue that I should wait upon?

Could you try to boot with baudrate 115201? This value would trigger to
use baudrate change code and because it is too close to standard 115200
there would be no real change.

And what is the output of following command immediately after kwboot?

stty -F /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0

Also, could try to catch whole strace log and send it to me?

Re: [PATCH u-boot-marvell 00/13] Yet another kwboot improvements

[Stefan Roese]

On 26.10.21 10:33, Pali Rohár wrote:
> On Monday 25 October 2021 17:15:14 Stefan Roese wrote:
>> Hi Pali,
>>
>> On 25.10.21 16:42, Pali Rohár wrote:
>>> On Monday 25 October 2021 16:39:44 Stefan Roese wrote:
>>>> Hi Marek,
>>>>
>>>> On 25.10.21 15:12, Marek Behún wrote:
>>>>> From: Marek Behún <marek.behun@nic.cz>
>>>>>
>>>>> Hello Stefan,
>>>>>
>>>>> these are another improvements for kwboot, please apply only after series
>>>>>      arm: mvebu: nandpagesize support for kwbimage v1
>>>>
>>>> I'm checking right now and have applied the 3 NAND patches on current
>>>> master. But this patchset fails at this one:
>>>>
>>>> tools: kwboot: Do not modify kwbimage header before increasing its size
>>>>
>>>> [stefan@ryzen u-boot-marvell (kwboot-test1)]$ git am -3 ~/tmp/kwboot2/*
>>>> Applying: tools: kwboot: Initialize rfds to zero
>>>> Applying: tools: kwboot: Fix initialization of tty device
>>>> Applying: tools: kwboot: Reserve enough space for patching kwbimage in
>>>> memory
>>>> Applying: tools: kwboot: Validate 4-byte image data checksum
>>>> Applying: tools: kwboot: Inject baudrate change back code after data part
>>>> Applying: tools: kwboot: Recalculate 4-byte data checksum after injecting
>>>> baudrate code
>>>> Applying: tools: kwboot: Correctly set configuration of UART for BootROM
>>>> messages
>>>> Applying: tools: kwboot: Show verbose message when waiting for baudrate
>>>> change magic
>>>> Applying: tools: kwboot: Simplify code for aligning image header
>>>> Applying: tools: kwboot: Do not modify kwbimage header before increasing its
>>>> size
>>>> error: sha1 information is lacking or useless (tools/kwboot.c).
>>>> error: could not build fake ancestor
>>>> Patch failed at 0010 tools: kwboot: Do not modify kwbimage header before
>>>> increasing its size
>>>> hint: Use 'git am --show-current-patch=diff' to see the failed patch
>>>> When you have resolved this problem, run "git am --continue".
>>>> If you prefer to skip this patch, run "git am --skip" instead.
>>>> To restore the original branch and stop patching, run "git am --abort".
>>>>
>>>> Any idea what's missing here?
>>>
>>> Hello! I'm using also this patch:
>>> https://patchwork.ozlabs.org/project/uboot/patch/20211021144609.9319-2-pali@kernel.org/
>>
>> Ah, yes. That does the trick. Now all patches apply clean. Thanks.
>>
>> Testing with all these patches on my AXP target does show, it still
>> does not work with baudrate > 115k:
>>
>> [stefan@ryzen u-boot-marvell (kwboot-test1)]$ ./tools/kwboot -B 230400 -b
>> u-boot-spl.kwb -t
>> /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
>> Patching image boot signature to UART
>> Injecting binary header code for changing baudrate to 230400 Bd
>> Injecting code for changing baudrate back
>> Sending boot message. Please reboot the target...|
>> Waiting 2s and flushing tty
>> Sending boot image header (90112 bytes)...
>>    0 %
>> [......................................................................]
>>   10 %
>> [......................................................................]
>>   20 %
>> [......................................................................]
>>   29 %
>> [......................................................................]
>>   39 %
>> [......................................................................]
>>   49 %
>> [......................................................................]
>>   59 %
>> [......................................................................]
>>   69 %
>> [......................................................................]
>>   79 %
>> [......................................................................]
>>   89 %
>> [......................................................................]
>>   99 % [....       ]
>> Done
>>
>> U-Boot SPL 2021.10-00908-gc129aa2f173a (Oct 25 2021 - 17:10:55 +0200)
>> High speed PHY - Version: 2.1.5 (COM-PHY-V20)
>> High speed PHY - Ended Successfully
>> DDR3 Training Sequence - Ver 5.7.4
>> DDR3 Training Sequence - Ended Successfully
>> Trying to boot from BOOTROM
>> Returning to BootROM (return address 0xffff0aa0)...
>>
>> Changing baudrate to 230400 Bd
>> Baudrate was not changed
>>
>>
>> xmodem: Protocol error
>> [stefan@ryzen u-boot-marvell (kwboot-test1)]$
>>
>>
>> Not changing the baudrate still works. Any idea what I should test? Or
>> do you have further changes in the queue that I should wait upon?
> 
> Could you try to boot with baudrate 115201? This value would trigger to
> use baudrate change code and because it is too close to standard 115200
> there would be no real change.

Ok. This seems to work at least partly (SPL):

[stefan@ryzen u-boot-marvell (kwboot-test1)]$ ./tools/kwboot -B 115201 
-b u-boot-spl.kwb -t 
/dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
Patching image boot signature to UART
Injecting binary header code for changing baudrate to 115201 Bd
Injecting code for changing baudrate back
Sending boot message. Please reboot the target...|
Waiting 2s and flushing tty
Sending boot image header (90112 bytes)...
   0 % 
[......................................................................]
  10 % 
[......................................................................]
  20 % 
[......................................................................]
  29 % 
[......................................................................]
  39 % 
[......................................................................]
  49 % 
[......................................................................]
  59 % 
[......................................................................]
  69 % 
[......................................................................]
  79 % 
[......................................................................]
  89 % 
[......................................................................]
  99 % [.... 
       ]
Done

U-Boot SPL 2021.10-00908-gc129aa2f173a (Oct 26 2021 - 10:39:55 +0200)
High speed PHY - Version: 2.1.5 (COM-PHY-V20)
High speed PHY - Ended Successfully
DDR3 Training Sequence - Ver 5.7.4
DDR3 Training Sequence - Ended Successfully
Trying to boot from BOOTROM
Returning to BootROM (return address 0xffff0aa0)...

Changing baudrate to 115201 Bd

Sending boot image data (549892 bytes)...
   0 % 
[......................................................................]
   1 % 
[......................................................................]
...
  97 % 
[......................................................................]
  99 % [........................... 
       ]
Done
Finishing transfer
Waiting 1s for baudrate change magic

Changing baudrate back to 115200 Bd

[Type Ctrl-\ + c to quit]

BootROM 1.20
Booting from SPI flash


So the CPU has run through a reset here.

> And what is the output of following command immediately after kwboot?
> 
> stty -F /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0

Changing baudrate to 230400 Bd
Baudrate was not changed


xmodem: Protocol error
[stefan@ryzen u-boot-marvell (kwboot-test1)]$ stty -F 
/dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
speed 230400 baud; line = 0;
intr = <undef>; quit = <undef>; erase = <undef>; kill = <undef>; eof = 
<undef>; start = <undef>; stop = <undef>; susp = <undef>; rprnt = <undef>;
werase = <undef>; lnext = <undef>; discard = <undef>; min = 1; time = 0;
-brkint -icrnl -imaxbel
-opost -onlcr
-isig -icanon -iexten -echo -echoe -echok -echoctl -echoke

> Also, could try to catch whole strace log and send it to me?

I'll try to do this later today.

Thanks,
Stefan

Re: [PATCH u-boot-marvell 00/13] Yet another kwboot improvements

[Pali Rohár]

On Tuesday 26 October 2021 10:45:09 Stefan Roese wrote:
> On 26.10.21 10:33, Pali Rohár wrote:
> > On Monday 25 October 2021 17:15:14 Stefan Roese wrote:
> > > Hi Pali,
> > > 
> > > On 25.10.21 16:42, Pali Rohár wrote:
> > > > On Monday 25 October 2021 16:39:44 Stefan Roese wrote:
> > > > > Hi Marek,
> > > > > 
> > > > > On 25.10.21 15:12, Marek Behún wrote:
> > > > > > From: Marek Behún <marek.behun@nic.cz>
> > > > > > 
> > > > > > Hello Stefan,
> > > > > > 
> > > > > > these are another improvements for kwboot, please apply only after series
> > > > > >      arm: mvebu: nandpagesize support for kwbimage v1
> > > > > 
> > > > > I'm checking right now and have applied the 3 NAND patches on current
> > > > > master. But this patchset fails at this one:
> > > > > 
> > > > > tools: kwboot: Do not modify kwbimage header before increasing its size
> > > > > 
> > > > > [stefan@ryzen u-boot-marvell (kwboot-test1)]$ git am -3 ~/tmp/kwboot2/*
> > > > > Applying: tools: kwboot: Initialize rfds to zero
> > > > > Applying: tools: kwboot: Fix initialization of tty device
> > > > > Applying: tools: kwboot: Reserve enough space for patching kwbimage in
> > > > > memory
> > > > > Applying: tools: kwboot: Validate 4-byte image data checksum
> > > > > Applying: tools: kwboot: Inject baudrate change back code after data part
> > > > > Applying: tools: kwboot: Recalculate 4-byte data checksum after injecting
> > > > > baudrate code
> > > > > Applying: tools: kwboot: Correctly set configuration of UART for BootROM
> > > > > messages
> > > > > Applying: tools: kwboot: Show verbose message when waiting for baudrate
> > > > > change magic
> > > > > Applying: tools: kwboot: Simplify code for aligning image header
> > > > > Applying: tools: kwboot: Do not modify kwbimage header before increasing its
> > > > > size
> > > > > error: sha1 information is lacking or useless (tools/kwboot.c).
> > > > > error: could not build fake ancestor
> > > > > Patch failed at 0010 tools: kwboot: Do not modify kwbimage header before
> > > > > increasing its size
> > > > > hint: Use 'git am --show-current-patch=diff' to see the failed patch
> > > > > When you have resolved this problem, run "git am --continue".
> > > > > If you prefer to skip this patch, run "git am --skip" instead.
> > > > > To restore the original branch and stop patching, run "git am --abort".
> > > > > 
> > > > > Any idea what's missing here?
> > > > 
> > > > Hello! I'm using also this patch:
> > > > https://patchwork.ozlabs.org/project/uboot/patch/20211021144609.9319-2-pali@kernel.org/
> > > 
> > > Ah, yes. That does the trick. Now all patches apply clean. Thanks.
> > > 
> > > Testing with all these patches on my AXP target does show, it still
> > > does not work with baudrate > 115k:
> > > 
> > > [stefan@ryzen u-boot-marvell (kwboot-test1)]$ ./tools/kwboot -B 230400 -b
> > > u-boot-spl.kwb -t
> > > /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
> > > Patching image boot signature to UART
> > > Injecting binary header code for changing baudrate to 230400 Bd
> > > Injecting code for changing baudrate back
> > > Sending boot message. Please reboot the target...|
> > > Waiting 2s and flushing tty
> > > Sending boot image header (90112 bytes)...
> > >    0 %
> > > [......................................................................]
> > >   10 %
> > > [......................................................................]
> > >   20 %
> > > [......................................................................]
> > >   29 %
> > > [......................................................................]
> > >   39 %
> > > [......................................................................]
> > >   49 %
> > > [......................................................................]
> > >   59 %
> > > [......................................................................]
> > >   69 %
> > > [......................................................................]
> > >   79 %
> > > [......................................................................]
> > >   89 %
> > > [......................................................................]
> > >   99 % [....       ]
> > > Done
> > > 
> > > U-Boot SPL 2021.10-00908-gc129aa2f173a (Oct 25 2021 - 17:10:55 +0200)
> > > High speed PHY - Version: 2.1.5 (COM-PHY-V20)
> > > High speed PHY - Ended Successfully
> > > DDR3 Training Sequence - Ver 5.7.4
> > > DDR3 Training Sequence - Ended Successfully
> > > Trying to boot from BOOTROM
> > > Returning to BootROM (return address 0xffff0aa0)...
> > > 
> > > Changing baudrate to 230400 Bd
> > > Baudrate was not changed
> > > 
> > > 
> > > xmodem: Protocol error
> > > [stefan@ryzen u-boot-marvell (kwboot-test1)]$
> > > 
> > > 
> > > Not changing the baudrate still works. Any idea what I should test? Or
> > > do you have further changes in the queue that I should wait upon?
> > 
> > Could you try to boot with baudrate 115201? This value would trigger to
> > use baudrate change code and because it is too close to standard 115200
> > there would be no real change.

Now I found logical error in kwboot code which handles retransmission of
the last header packet. State of "baudrate change" is cleared on every
retransmission. Please apply following diff, so state variables are
initialized only once.

diff --git a/tools/kwboot.c b/tools/kwboot.c
index d38ee0065177..835ccc8c113a 100644
--- a/tools/kwboot.c
+++ b/tools/kwboot.c
@@ -859,11 +859,6 @@ kwboot_xm_recv_reply(int fd, char *c, int nak_on_non_xm,
 	uint64_t recv_until = _now() + timeout;
 	int rc;
 
-	if (non_xm_print)
-		*non_xm_print = 0;
-	if (baud_changed)
-		*baud_changed = 0;
-
 	while (1) {
 		rc = kwboot_tty_recv(fd, c, 1, timeout);
 		if (rc) {
@@ -929,6 +924,8 @@ kwboot_xm_sendblock(int fd, struct kwboot_block *block, int allow_non_xm,
 	char c;
 
 	*done_print = 0;
+	non_xm_print = 0;
+	baud_changed = 0;
 
 	retries = 0;
 	do {

> Ok. This seems to work at least partly (SPL):
> 
> [stefan@ryzen u-boot-marvell (kwboot-test1)]$ ./tools/kwboot -B 115201 -b
> u-boot-spl.kwb -t
> /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
> Patching image boot signature to UART
> Injecting binary header code for changing baudrate to 115201 Bd
> Injecting code for changing baudrate back
> Sending boot message. Please reboot the target...|
> Waiting 2s and flushing tty
> Sending boot image header (90112 bytes)...
>   0 %
> [......................................................................]
>  10 %
> [......................................................................]
>  20 %
> [......................................................................]
>  29 %
> [......................................................................]
>  39 %
> [......................................................................]
>  49 %
> [......................................................................]
>  59 %
> [......................................................................]
>  69 %
> [......................................................................]
>  79 %
> [......................................................................]
>  89 %
> [......................................................................]
>  99 % [....       ]
> Done
> 
> U-Boot SPL 2021.10-00908-gc129aa2f173a (Oct 26 2021 - 10:39:55 +0200)
> High speed PHY - Version: 2.1.5 (COM-PHY-V20)
> High speed PHY - Ended Successfully
> DDR3 Training Sequence - Ver 5.7.4
> DDR3 Training Sequence - Ended Successfully
> Trying to boot from BOOTROM
> Returning to BootROM (return address 0xffff0aa0)...
> 
> Changing baudrate to 115201 Bd
> 
> Sending boot image data (549892 bytes)...
>   0 %
> [......................................................................]
>   1 %
> [......................................................................]
> ...
>  97 %
> [......................................................................]
>  99 % [...........................       ]
> Done
> Finishing transfer
> Waiting 1s for baudrate change magic
> 
> Changing baudrate back to 115200 Bd
> 
> [Type Ctrl-\ + c to quit]
> 
> BootROM 1.20
> Booting from SPI flash
> 
> 
> So the CPU has run through a reset here.

What kind of CPU core has your AXP board? Maybe there is some arm
instruction in kwboot_baud_code[] array which is not supported by that
core (and so reset occurs)?

And could you send me commands which you are using for compiling U-Boot
for that your AXP board? I would like to examine resulted binary.

> > And what is the output of following command immediately after kwboot?
> > 
> > stty -F /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
> 
> Changing baudrate to 230400 Bd
> Baudrate was not changed
> 
> 
> xmodem: Protocol error
> [stefan@ryzen u-boot-marvell (kwboot-test1)]$ stty -F
> /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
> speed 230400 baud; line = 0;

... but baudrate _was_ changed.

So it looks like a bug in kwboot code. Try to apply above diff, it is
really possible that you hit that logical error. (Interesting that
neither Marek nor me saw it)

> intr = <undef>; quit = <undef>; erase = <undef>; kill = <undef>; eof =
> <undef>; start = <undef>; stop = <undef>; susp = <undef>; rprnt = <undef>;
> werase = <undef>; lnext = <undef>; discard = <undef>; min = 1; time = 0;
> -brkint -icrnl -imaxbel
> -opost -onlcr
> -isig -icanon -iexten -echo -echoe -echok -echoctl -echoke
> 
> > Also, could try to catch whole strace log and send it to me?
> 
> I'll try to do this later today.

'strace kwboot ... 1>log 2>&1' should do it...

> Thanks,
> Stefan

Re: [PATCH u-boot-marvell 00/13] Yet another kwboot improvements

[Stefan Roese]

On 26.10.21 11:06, Pali Rohár wrote:

<snip>

> Now I found logical error in kwboot code which handles retransmission of
> the last header packet. State of "baudrate change" is cleared on every
> retransmission. Please apply following diff, so state variables are
> initialized only once.
> 
> diff --git a/tools/kwboot.c b/tools/kwboot.c
> index d38ee0065177..835ccc8c113a 100644
> --- a/tools/kwboot.c
> +++ b/tools/kwboot.c
> @@ -859,11 +859,6 @@ kwboot_xm_recv_reply(int fd, char *c, int nak_on_non_xm,
>   	uint64_t recv_until = _now() + timeout;
>   	int rc;
>   
> -	if (non_xm_print)
> -		*non_xm_print = 0;
> -	if (baud_changed)
> -		*baud_changed = 0;
> -
>   	while (1) {
>   		rc = kwboot_tty_recv(fd, c, 1, timeout);
>   		if (rc) {
> @@ -929,6 +924,8 @@ kwboot_xm_sendblock(int fd, struct kwboot_block *block, int allow_non_xm,
>   	char c;
>   
>   	*done_print = 0;
> +	non_xm_print = 0;
> +	baud_changed = 0;
>   
>   	retries = 0;
>   	do {

This definitely helps (a bit). Now I get this:

[stefan@ryzen u-boot-marvell (kwboot-test1)]$ ./tools/kwboot -B 230400 
-b u-boot-spl.kwb -t 
/dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
Patching image boot signature to UART
Injecting binary header code for changing baudrate to 230400 Bd
Injecting code for changing baudrate back
Sending boot message. Please reboot the target...|
Waiting 2s and flushing tty
Sending boot image header (90112 bytes)...
   0 % 
[......................................................................]
  10 % 
[......................................................................]
  20 % 
[......................................................................]
  29 % 
[......................................................................]
  39 % 
[......................................................................]
  49 % 
[......................................................................]
  59 % 
[......................................................................]
  69 % 
[......................................................................]
  79 % 
[......................................................................]
  89 % 
[......................................................................]
  99 % [.... 
       ]
Done

U-Boot SPL 2021.10-00908-gc129aa2f173a-dirty (Oct 26 2021 - 12:48:11 +0200)
High speed PHY - Version: 2.1.5 (COM-PHY-V20)
High speed PHY - Ended Successfully
DDR3 Training Sequence - Ver 5.7.4
DDR3 Training Sequence - Ended Successfully
Trying to boot from BOOTROM
Returning to BootROM (return address 0xffff0aa0)...

Changing baudrate to 230400 Bd

Sending boot image data (549892 bytes)...
   0 % 
[......................................................................]

...

  99 % [........................... 
       ]
Done
Finishing transfer
Waiting 1s for baudrate change magic
xmodem: Connection timed out


BTW: The baudrate change does not seem to work or have any effect on
the image download speed. Comparing 230400 and 921600 baud speeds, I can
spot no real time difference here:

230400 baud:
[stefan@ryzen u-boot-marvell (kwboot-test1)]$ time ./tools/kwboot -B 
230400 -b u-boot-spl.kwb -t 
/dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
Patching image boot signature to UART
Injecting binary header code for changing baudrate to 230400 Bd

...

  99 % [........................... 
       ]
Done
Finishing transfer
Waiting 1s for baudrate change magic
xmodem: Connection timed out
[2]+  Done                    emacs tools/kwboot.c

real    1m27,279s
user    0m2,313s
sys     0m0,337s

921600 baud:
[stefan@ryzen u-boot-marvell (kwboot-test1)]$ time ./tools/kwboot -B 
921600 -b u-boot-spl.kwb -t 
/dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
Patching image boot signature to UART
Injecting binary header code for changing baudrate to 921600 Bd

...

  99 % [........................... 
       ]
Done
Finishing transfer
Waiting 1s for baudrate change magic
xmodem: Connection timed out

real    1m27,839s
user    0m0,224s
sys     0m0,194s

Any idea why this is the case?

>> Ok. This seems to work at least partly (SPL):
>>
>> [stefan@ryzen u-boot-marvell (kwboot-test1)]$ ./tools/kwboot -B 115201 -b
>> u-boot-spl.kwb -t
>> /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
>> Patching image boot signature to UART
>> Injecting binary header code for changing baudrate to 115201 Bd
>> Injecting code for changing baudrate back
>> Sending boot message. Please reboot the target...|
>> Waiting 2s and flushing tty
>> Sending boot image header (90112 bytes)...
>>    0 %
>> [......................................................................]
>>   10 %
>> [......................................................................]
>>   20 %
>> [......................................................................]
>>   29 %
>> [......................................................................]
>>   39 %
>> [......................................................................]
>>   49 %
>> [......................................................................]
>>   59 %
>> [......................................................................]
>>   69 %
>> [......................................................................]
>>   79 %
>> [......................................................................]
>>   89 %
>> [......................................................................]
>>   99 % [....       ]
>> Done
>>
>> U-Boot SPL 2021.10-00908-gc129aa2f173a (Oct 26 2021 - 10:39:55 +0200)
>> High speed PHY - Version: 2.1.5 (COM-PHY-V20)
>> High speed PHY - Ended Successfully
>> DDR3 Training Sequence - Ver 5.7.4
>> DDR3 Training Sequence - Ended Successfully
>> Trying to boot from BOOTROM
>> Returning to BootROM (return address 0xffff0aa0)...
>>
>> Changing baudrate to 115201 Bd
>>
>> Sending boot image data (549892 bytes)...
>>    0 %
>> [......................................................................]
>>    1 %
>> [......................................................................]
>> ...
>>   97 %
>> [......................................................................]
>>   99 % [...........................       ]
>> Done
>> Finishing transfer
>> Waiting 1s for baudrate change magic
>>
>> Changing baudrate back to 115200 Bd
>>
>> [Type Ctrl-\ + c to quit]
>>
>> BootROM 1.20
>> Booting from SPI flash
>>
>>
>> So the CPU has run through a reset here.
> 
> What kind of CPU core has your AXP board? Maybe there is some arm
> instruction in kwboot_baud_code[] array which is not supported by that
> core (and so reset occurs)?

AXP stands for Armada XP, which is ARM v7 AFAIK.

> And could you send me commands which you are using for compiling U-Boot
> for that your AXP board? I would like to examine resulted binary.

I'm currently using a kernel.org toolchain:

[stefan@ryzen u-boot-marvell (kwboot-test1)]$ set | grep CROSS
CROSS_COMPILE=/opt/kernel.org/gcc-11.1.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-
[stefan@ryzen u-boot-marvell (kwboot-test1)]$ 
/opt/kernel.org/gcc-11.1.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-gcc 
--version
arm-linux-gnueabi-gcc (GCC) 11.1.0
Copyright (C) 2021 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Here my commands to generate the U-Boot binary:

make mrproper
make theadorable_debug_defconfig
make -s -j20

>>> And what is the output of following command immediately after kwboot?
>>>
>>> stty -F /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
>>
>> Changing baudrate to 230400 Bd
>> Baudrate was not changed
>>
>>
>> xmodem: Protocol error
>> [stefan@ryzen u-boot-marvell (kwboot-test1)]$ stty -F
>> /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
>> speed 230400 baud; line = 0;
> 
> ... but baudrate _was_ changed.
> 
> So it looks like a bug in kwboot code. Try to apply above diff, it is
> really possible that you hit that logical error. (Interesting that
> neither Marek nor me saw it)
> 
>> intr = <undef>; quit = <undef>; erase = <undef>; kill = <undef>; eof =
>> <undef>; start = <undef>; stop = <undef>; susp = <undef>; rprnt = <undef>;
>> werase = <undef>; lnext = <undef>; discard = <undef>; min = 1; time = 0;
>> -brkint -icrnl -imaxbel
>> -opost -onlcr
>> -isig -icanon -iexten -echo -echoe -echok -echoctl -echoke
>>
>>> Also, could try to catch whole strace log and send it to me?
>>
>> I'll try to do this later today.
> 
> 'strace kwboot ... 1>log 2>&1' should do it...

Okay, will follow in the next privatze mail.

Thanks,
Stefan

Re: [PATCH u-boot-marvell 00/13] Yet another kwboot improvements

[Pali Rohár]

On Tuesday 26 October 2021 13:09:42 Stefan Roese wrote:
> On 26.10.21 11:06, Pali Rohár wrote:
> 
> <snip>
> 
> > Now I found logical error in kwboot code which handles retransmission of
> > the last header packet. State of "baudrate change" is cleared on every
> > retransmission. Please apply following diff, so state variables are
> > initialized only once.
> > 
> > diff --git a/tools/kwboot.c b/tools/kwboot.c
> > index d38ee0065177..835ccc8c113a 100644
> > --- a/tools/kwboot.c
> > +++ b/tools/kwboot.c
> > @@ -859,11 +859,6 @@ kwboot_xm_recv_reply(int fd, char *c, int nak_on_non_xm,
> >   	uint64_t recv_until = _now() + timeout;
> >   	int rc;
> > -	if (non_xm_print)
> > -		*non_xm_print = 0;
> > -	if (baud_changed)
> > -		*baud_changed = 0;
> > -
> >   	while (1) {
> >   		rc = kwboot_tty_recv(fd, c, 1, timeout);
> >   		if (rc) {
> > @@ -929,6 +924,8 @@ kwboot_xm_sendblock(int fd, struct kwboot_block *block, int allow_non_xm,
> >   	char c;
> >   	*done_print = 0;
> > +	non_xm_print = 0;
> > +	baud_changed = 0;
> >   	retries = 0;
> >   	do {
> 
> This definitely helps (a bit). Now I get this:

Perfect! So this fixes issue with retransmission of the last header
packet.

But there is still issue with starting main U-Boot binary.

> [stefan@ryzen u-boot-marvell (kwboot-test1)]$ ./tools/kwboot -B 230400 -b
> u-boot-spl.kwb -t
> /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
> Patching image boot signature to UART
> Injecting binary header code for changing baudrate to 230400 Bd
> Injecting code for changing baudrate back
> Sending boot message. Please reboot the target...|
> Waiting 2s and flushing tty
> Sending boot image header (90112 bytes)...
>   0 %
> [......................................................................]
>  10 %
> [......................................................................]
>  20 %
> [......................................................................]
>  29 %
> [......................................................................]
>  39 %
> [......................................................................]
>  49 %
> [......................................................................]
>  59 %
> [......................................................................]
>  69 %
> [......................................................................]
>  79 %
> [......................................................................]
>  89 %
> [......................................................................]
>  99 % [....       ]
> Done
> 
> U-Boot SPL 2021.10-00908-gc129aa2f173a-dirty (Oct 26 2021 - 12:48:11 +0200)
> High speed PHY - Version: 2.1.5 (COM-PHY-V20)
> High speed PHY - Ended Successfully
> DDR3 Training Sequence - Ver 5.7.4
> DDR3 Training Sequence - Ended Successfully
> Trying to boot from BOOTROM
> Returning to BootROM (return address 0xffff0aa0)...
> 
> Changing baudrate to 230400 Bd
> 
> Sending boot image data (549892 bytes)...
>   0 %
> [......................................................................]
> 
> ...
> 
>  99 % [...........................       ]
> Done
> Finishing transfer
> Waiting 1s for baudrate change magic
> xmodem: Connection timed out
> 
> 
> BTW: The baudrate change does not seem to work or have any effect on
> the image download speed. Comparing 230400 and 921600 baud speeds, I can
> spot no real time difference here:
> 
> 230400 baud:
> [stefan@ryzen u-boot-marvell (kwboot-test1)]$ time ./tools/kwboot -B 230400
> -b u-boot-spl.kwb -t
> /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
> Patching image boot signature to UART
> Injecting binary header code for changing baudrate to 230400 Bd
> 
> ...
> 
>  99 % [...........................       ]
> Done
> Finishing transfer
> Waiting 1s for baudrate change magic
> xmodem: Connection timed out
> [2]+  Done                    emacs tools/kwboot.c
> 
> real    1m27,279s
> user    0m2,313s
> sys     0m0,337s
> 
> 921600 baud:
> [stefan@ryzen u-boot-marvell (kwboot-test1)]$ time ./tools/kwboot -B 921600
> -b u-boot-spl.kwb -t
> /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
> Patching image boot signature to UART
> Injecting binary header code for changing baudrate to 921600 Bd
> 
> ...
> 
>  99 % [...........................       ]
> Done
> Finishing transfer
> Waiting 1s for baudrate change magic
> xmodem: Connection timed out
> 
> real    1m27,839s
> user    0m0,224s
> sys     0m0,194s
> 
> Any idea why this is the case?

UARTs at both sides must be configured to 921600 speed, otherwise they
would not be able to communicate.

What is happening here? I do not know. But as Marek told me that
observed same issue and replacing USB-UART cable by another decreased
transfer time. So I think that issue is somewhere in USB-UART
transmitter. My guess is that USB-UART transmitter send at 921600 speed:
start bit, 8 bits of data, stop bit and then loooong idle pause (logical
one). After that sends another 10 bits.

Maybe kernel is not filling next byte into USB-UART hw queue? Or USB-UART
cannot send more bytes faster and inserts those long idle pauses?

Could you try following diff which disables calling tcdrain() for xmodem
packets, to remove any waits between individual bytes?

diff --git a/tools/kwboot.c b/tools/kwboot.c
index 835ccc8c113a..5f4ff673972e 100644
--- a/tools/kwboot.c
+++ b/tools/kwboot.c
@@ -404,7 +404,7 @@ out:
 }
 
 static int
-kwboot_tty_send(int fd, const void *buf, size_t len)
+kwboot_tty_send(int fd, const void *buf, size_t len, int nodrain)
 {
 	if (!buf)
 		return 0;
@@ -412,13 +412,16 @@ kwboot_tty_send(int fd, const void *buf, size_t len)
 	if (kwboot_write(fd, buf, len) < 0)
 		return -1;
 
+	if (nodrain)
+		return 0;
+
 	return tcdrain(fd);
 }
 
 static int
 kwboot_tty_send_char(int fd, unsigned char c)
 {
-	return kwboot_tty_send(fd, &c, 1);
+	return kwboot_tty_send(fd, &c, 1, 0);
 }
 
 static speed_t
@@ -705,7 +708,7 @@ kwboot_bootmsg(int tty, void *msg)
 			break;
 
 		for (count = 0; count < 128; count++) {
-			rc = kwboot_tty_send(tty, msg, 8);
+			rc = kwboot_tty_send(tty, msg, 8, 0);
 			if (rc) {
 				usleep(msg_req_delay * 1000);
 				continue;
@@ -737,7 +740,7 @@ kwboot_debugmsg(int tty, void *msg)
 		if (rc)
 			break;
 
-		rc = kwboot_tty_send(tty, msg, 8);
+		rc = kwboot_tty_send(tty, msg, 8, 0);
 		if (rc) {
 			usleep(msg_req_delay * 1000);
 			continue;
@@ -929,7 +932,7 @@ kwboot_xm_sendblock(int fd, struct kwboot_block *block, int allow_non_xm,
 
 	retries = 0;
 	do {
-		rc = kwboot_tty_send(fd, block, sizeof(*block));
+		rc = kwboot_tty_send(fd, block, sizeof(*block), 1);
 		if (rc)
 			return rc;
 

If it is kernel who does not fill next bytes into hw UART queue as it
waits for tcdrain then above patch should fix it.

Or if you have other USB-UART cables, try another. I have good
experience with pl2303-based.

> > > Ok. This seems to work at least partly (SPL):
> > > 
> > > [stefan@ryzen u-boot-marvell (kwboot-test1)]$ ./tools/kwboot -B 115201 -b
> > > u-boot-spl.kwb -t
> > > /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
> > > Patching image boot signature to UART
> > > Injecting binary header code for changing baudrate to 115201 Bd
> > > Injecting code for changing baudrate back
> > > Sending boot message. Please reboot the target...|
> > > Waiting 2s and flushing tty
> > > Sending boot image header (90112 bytes)...
> > >    0 %
> > > [......................................................................]
> > >   10 %
> > > [......................................................................]
> > >   20 %
> > > [......................................................................]
> > >   29 %
> > > [......................................................................]
> > >   39 %
> > > [......................................................................]
> > >   49 %
> > > [......................................................................]
> > >   59 %
> > > [......................................................................]
> > >   69 %
> > > [......................................................................]
> > >   79 %
> > > [......................................................................]
> > >   89 %
> > > [......................................................................]
> > >   99 % [....       ]
> > > Done
> > > 
> > > U-Boot SPL 2021.10-00908-gc129aa2f173a (Oct 26 2021 - 10:39:55 +0200)
> > > High speed PHY - Version: 2.1.5 (COM-PHY-V20)
> > > High speed PHY - Ended Successfully
> > > DDR3 Training Sequence - Ver 5.7.4
> > > DDR3 Training Sequence - Ended Successfully
> > > Trying to boot from BOOTROM
> > > Returning to BootROM (return address 0xffff0aa0)...
> > > 
> > > Changing baudrate to 115201 Bd
> > > 
> > > Sending boot image data (549892 bytes)...
> > >    0 %
> > > [......................................................................]
> > >    1 %
> > > [......................................................................]
> > > ...
> > >   97 %
> > > [......................................................................]
> > >   99 % [...........................       ]
> > > Done
> > > Finishing transfer
> > > Waiting 1s for baudrate change magic
> > > 
> > > Changing baudrate back to 115200 Bd
> > > 
> > > [Type Ctrl-\ + c to quit]
> > > 
> > > BootROM 1.20
> > > Booting from SPI flash
> > > 
> > > 
> > > So the CPU has run through a reset here.
> > 
> > What kind of CPU core has your AXP board? Maybe there is some arm
> > instruction in kwboot_baud_code[] array which is not supported by that
> > core (and so reset occurs)?
> 
> AXP stands for Armada XP, which is ARM v7 AFAIK.

I remember that older Marvell Armada SoCs used custom designed ARM
cores, so there could be some differences what is supported by licensed
ARM core and custom ARM core.

Anyway, as changing baudrate after execution of SPL is working fine, it
should mean that there is no undefined/unsupported instruction.

My another guess there could be a problem is usage of stack. Maybe it is
possible that register with stack pointer is not initialized after the
full transfer when going to execute main image. Same arm baudrate change
code is used after the SPL and before main U-Boot, and the first
instruction is "push". Maybe modifying the arm code to not use stack
when switching the baudrate back to 115200 could also help. I will look
at it.

> > And could you send me commands which you are using for compiling U-Boot
> > for that your AXP board? I would like to examine resulted binary.
> 
> I'm currently using a kernel.org toolchain:
> 
> [stefan@ryzen u-boot-marvell (kwboot-test1)]$ set | grep CROSS
> CROSS_COMPILE=/opt/kernel.org/gcc-11.1.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-
> [stefan@ryzen u-boot-marvell (kwboot-test1)]$
> /opt/kernel.org/gcc-11.1.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-gcc
> --version
> arm-linux-gnueabi-gcc (GCC) 11.1.0
> Copyright (C) 2021 Free Software Foundation, Inc.
> This is free software; see the source for copying conditions.  There is NO
> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> 
> Here my commands to generate the U-Boot binary:
> 
> make mrproper
> make theadorable_debug_defconfig
> make -s -j20

Ok.

> > > > And what is the output of following command immediately after kwboot?
> > > > 
> > > > stty -F /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
> > > 
> > > Changing baudrate to 230400 Bd
> > > Baudrate was not changed
> > > 
> > > 
> > > xmodem: Protocol error
> > > [stefan@ryzen u-boot-marvell (kwboot-test1)]$ stty -F
> > > /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
> > > speed 230400 baud; line = 0;
> > 
> > ... but baudrate _was_ changed.
> > 
> > So it looks like a bug in kwboot code. Try to apply above diff, it is
> > really possible that you hit that logical error. (Interesting that
> > neither Marek nor me saw it)
> > 
> > > intr = <undef>; quit = <undef>; erase = <undef>; kill = <undef>; eof =
> > > <undef>; start = <undef>; stop = <undef>; susp = <undef>; rprnt = <undef>;
> > > werase = <undef>; lnext = <undef>; discard = <undef>; min = 1; time = 0;
> > > -brkint -icrnl -imaxbel
> > > -opost -onlcr
> > > -isig -icanon -iexten -echo -echoe -echok -echoctl -echoke
> > > 
> > > > Also, could try to catch whole strace log and send it to me?
> > > 
> > > I'll try to do this later today.
> > 
> > 'strace kwboot ... 1>log 2>&1' should do it...
> 
> Okay, will follow in the next privatze mail.

Received. I have looked at it. And there are two interesting parts:

1) Both parts of transfer (header at default speed 115200 and main at
high speed 921600) were transferred without any error, except for the
error indication of the last header packet. Error indication was sent
after the execution of header itself (SPL and baudrate change), it means
that there was no error and bootrom did not sent any error indication.
kwboot did retransmission of this one packet and it succeeded (thanks to
that fixup which I have sent).

So this error must have been just fluctuation on UART and probably
caused by changing UART speed. I guess that your USB-UART chip needs
more time to stabilize new speed and so USB-UART receiver lost/damaged
reply byte sent by bootrom.

Maybe you could increase delay in arm baudrate change code, so CPU waits
longer time before it returns to bootrom?

Replacing code in kwboot_baud_code[] array:

				/*  ; Sleep 1ms ~~ 600000 cycles at 1200 MHz  */
				/*  ; r1 = 600000                             */
	0x9f, 0x1d, 0xa0, 0xe3, /* mov   r1, #0x27c0                          */
	0x09, 0x10, 0x40, 0xe3, /* movt  r1, #0x0009                          */

by:

				/*  ; Sleep 10ms ~~ 6M cycles at 1200 MHz     */
				/*  ; r1 = 6000000                            */
	0x80, 0x1d, 0x08, 0xe3, /* mov   r1, #0x8d80                          */
	0x5b, 0x10, 0x40, 0xe3, /* movt  r1, #0x005b                          */

should do it.

2) Transfer was successful (EOT was ACKed) but there is absolutely no
output on UART after the transfer. Which should indicate that bootrom
should have started execution of the main image, but something crashed
and board reset... Maybe it is really due to usage of stack...

> Thanks,
> Stefan

Re: [PATCH u-boot-marvell 00/13] Yet another kwboot improvements

[Marek Behún]

On Tue, 26 Oct 2021 14:40:48 +0200
Pali Rohár <pali@kernel.org> wrote:

> What is happening here? I do not know. But as Marek told me that
> observed same issue and replacing USB-UART cable by another decreased
> transfer time. So I think that issue is somewhere in USB-UART
> transmitter. My guess is that USB-UART transmitter send at 921600 speed:
> start bit, 8 bits of data, stop bit and then loooong idle pause (logical
> one). After that sends another 10 bits.

Yes, my FT232RL USB-UART converters do not send at higher speeds,
although they report baudrate change.

I do not really know what is going on. Maybe the ftdi-sio driver needs
to be looked at. (It definitely needs to be looked at, because of other
things. Last I looked into it the baudrate change code seemed okay.)

Using pl2303 converter works like a charm.

Note that I have another FT232RL converter for MOX, which works at
1.8V, and on MOX higher speeds work correctly.

Maybe some of the FT232RL chips are those fake ones?

Stefan, could you check in a different way whether your converter can
transfer at higher speed? For example with another device, or in U-Boot
via kermit / ymodem, or whatnot.

Marek

Re: [PATCH u-boot-marvell 00/13] Yet another kwboot improvements

[Stefan Roese]

On 26.10.21 15:06, Marek Behún wrote:
> On Tue, 26 Oct 2021 14:40:48 +0200
> Pali Rohár <pali@kernel.org> wrote:
> 
>> What is happening here? I do not know. But as Marek told me that
>> observed same issue and replacing USB-UART cable by another decreased
>> transfer time. So I think that issue is somewhere in USB-UART
>> transmitter. My guess is that USB-UART transmitter send at 921600 speed:
>> start bit, 8 bits of data, stop bit and then loooong idle pause (logical
>> one). After that sends another 10 bits.
> 
> Yes, my FT232RL USB-UART converters do not send at higher speeds,
> although they report baudrate change.
> 
> I do not really know what is going on. Maybe the ftdi-sio driver needs
> to be looked at. (It definitely needs to be looked at, because of other
> things. Last I looked into it the baudrate change code seemed okay.)
> 
> Using pl2303 converter works like a charm.
> 
> Note that I have another FT232RL converter for MOX, which works at
> 1.8V, and on MOX higher speeds work correctly.
> 
> Maybe some of the FT232RL chips are those fake ones?

Maybe. Unfortunately, on this Armada XP board this FTDI chip is soldered
on the target, so I can't easily switch adapters.

> Stefan, could you check in a different way whether your converter can
> transfer at higher speed? For example with another device, or in U-Boot
> via kermit / ymodem, or whatnot.

Connecting with 230400 baud and booting into U-Boot etc shows this:

$ cu -s 230400 -l 
/dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
Connected.
�������������▒�����`�▒3������▒�▒�▒�▒30�▒0����`f憞~�▒���������f�`

So it seems, that baudrate change seems to work fine with the chip.

Thanks,
Stefan

Re: [PATCH u-boot-marvell 00/13] Yet another kwboot improvements

[Stefan Roese]

On 26.10.21 14:40, Pali Rohár wrote:
> On Tuesday 26 October 2021 13:09:42 Stefan Roese wrote:
>> On 26.10.21 11:06, Pali Rohár wrote:
>>
>> <snip>
>>
>>> Now I found logical error in kwboot code which handles retransmission of
>>> the last header packet. State of "baudrate change" is cleared on every
>>> retransmission. Please apply following diff, so state variables are
>>> initialized only once.
>>>
>>> diff --git a/tools/kwboot.c b/tools/kwboot.c
>>> index d38ee0065177..835ccc8c113a 100644
>>> --- a/tools/kwboot.c
>>> +++ b/tools/kwboot.c
>>> @@ -859,11 +859,6 @@ kwboot_xm_recv_reply(int fd, char *c, int nak_on_non_xm,
>>>    	uint64_t recv_until = _now() + timeout;
>>>    	int rc;
>>> -	if (non_xm_print)
>>> -		*non_xm_print = 0;
>>> -	if (baud_changed)
>>> -		*baud_changed = 0;
>>> -
>>>    	while (1) {
>>>    		rc = kwboot_tty_recv(fd, c, 1, timeout);
>>>    		if (rc) {
>>> @@ -929,6 +924,8 @@ kwboot_xm_sendblock(int fd, struct kwboot_block *block, int allow_non_xm,
>>>    	char c;
>>>    	*done_print = 0;
>>> +	non_xm_print = 0;
>>> +	baud_changed = 0;
>>>    	retries = 0;
>>>    	do {
>>
>> This definitely helps (a bit). Now I get this:
> 
> Perfect! So this fixes issue with retransmission of the last header
> packet.
> 
> But there is still issue with starting main U-Boot binary.
> 
>> [stefan@ryzen u-boot-marvell (kwboot-test1)]$ ./tools/kwboot -B 230400 -b
>> u-boot-spl.kwb -t
>> /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
>> Patching image boot signature to UART
>> Injecting binary header code for changing baudrate to 230400 Bd
>> Injecting code for changing baudrate back
>> Sending boot message. Please reboot the target...|
>> Waiting 2s and flushing tty
>> Sending boot image header (90112 bytes)...
>>    0 %
>> [......................................................................]
>>   10 %
>> [......................................................................]
>>   20 %
>> [......................................................................]
>>   29 %
>> [......................................................................]
>>   39 %
>> [......................................................................]
>>   49 %
>> [......................................................................]
>>   59 %
>> [......................................................................]
>>   69 %
>> [......................................................................]
>>   79 %
>> [......................................................................]
>>   89 %
>> [......................................................................]
>>   99 % [....       ]
>> Done
>>
>> U-Boot SPL 2021.10-00908-gc129aa2f173a-dirty (Oct 26 2021 - 12:48:11 +0200)
>> High speed PHY - Version: 2.1.5 (COM-PHY-V20)
>> High speed PHY - Ended Successfully
>> DDR3 Training Sequence - Ver 5.7.4
>> DDR3 Training Sequence - Ended Successfully
>> Trying to boot from BOOTROM
>> Returning to BootROM (return address 0xffff0aa0)...
>>
>> Changing baudrate to 230400 Bd
>>
>> Sending boot image data (549892 bytes)...
>>    0 %
>> [......................................................................]
>>
>> ...
>>
>>   99 % [...........................       ]
>> Done
>> Finishing transfer
>> Waiting 1s for baudrate change magic
>> xmodem: Connection timed out
>>
>>
>> BTW: The baudrate change does not seem to work or have any effect on
>> the image download speed. Comparing 230400 and 921600 baud speeds, I can
>> spot no real time difference here:
>>
>> 230400 baud:
>> [stefan@ryzen u-boot-marvell (kwboot-test1)]$ time ./tools/kwboot -B 230400
>> -b u-boot-spl.kwb -t
>> /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
>> Patching image boot signature to UART
>> Injecting binary header code for changing baudrate to 230400 Bd
>>
>> ...
>>
>>   99 % [...........................       ]
>> Done
>> Finishing transfer
>> Waiting 1s for baudrate change magic
>> xmodem: Connection timed out
>> [2]+  Done                    emacs tools/kwboot.c
>>
>> real    1m27,279s
>> user    0m2,313s
>> sys     0m0,337s
>>
>> 921600 baud:
>> [stefan@ryzen u-boot-marvell (kwboot-test1)]$ time ./tools/kwboot -B 921600
>> -b u-boot-spl.kwb -t
>> /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
>> Patching image boot signature to UART
>> Injecting binary header code for changing baudrate to 921600 Bd
>>
>> ...
>>
>>   99 % [...........................       ]
>> Done
>> Finishing transfer
>> Waiting 1s for baudrate change magic
>> xmodem: Connection timed out
>>
>> real    1m27,839s
>> user    0m0,224s
>> sys     0m0,194s
>>
>> Any idea why this is the case?
> 
> UARTs at both sides must be configured to 921600 speed, otherwise they
> would not be able to communicate.

I'm also surpised here.

> What is happening here? I do not know. But as Marek told me that
> observed same issue and replacing USB-UART cable by another decreased
> transfer time. So I think that issue is somewhere in USB-UART
> transmitter. My guess is that USB-UART transmitter send at 921600 speed:
> start bit, 8 bits of data, stop bit and then loooong idle pause (logical
> one). After that sends another 10 bits.
> 
> Maybe kernel is not filling next byte into USB-UART hw queue? Or USB-UART
> cannot send more bytes faster and inserts those long idle pauses?
> 
> Could you try following diff which disables calling tcdrain() for xmodem
> packets, to remove any waits between individual bytes?
> 
> diff --git a/tools/kwboot.c b/tools/kwboot.c
> index 835ccc8c113a..5f4ff673972e 100644
> --- a/tools/kwboot.c
> +++ b/tools/kwboot.c
> @@ -404,7 +404,7 @@ out:
>   }
>   
>   static int
> -kwboot_tty_send(int fd, const void *buf, size_t len)
> +kwboot_tty_send(int fd, const void *buf, size_t len, int nodrain)
>   {
>   	if (!buf)
>   		return 0;
> @@ -412,13 +412,16 @@ kwboot_tty_send(int fd, const void *buf, size_t len)
>   	if (kwboot_write(fd, buf, len) < 0)
>   		return -1;
>   
> +	if (nodrain)
> +		return 0;
> +
>   	return tcdrain(fd);
>   }
>   
>   static int
>   kwboot_tty_send_char(int fd, unsigned char c)
>   {
> -	return kwboot_tty_send(fd, &c, 1);
> +	return kwboot_tty_send(fd, &c, 1, 0);
>   }
>   
>   static speed_t
> @@ -705,7 +708,7 @@ kwboot_bootmsg(int tty, void *msg)
>   			break;
>   
>   		for (count = 0; count < 128; count++) {
> -			rc = kwboot_tty_send(tty, msg, 8);
> +			rc = kwboot_tty_send(tty, msg, 8, 0);
>   			if (rc) {
>   				usleep(msg_req_delay * 1000);
>   				continue;
> @@ -737,7 +740,7 @@ kwboot_debugmsg(int tty, void *msg)
>   		if (rc)
>   			break;
>   
> -		rc = kwboot_tty_send(tty, msg, 8);
> +		rc = kwboot_tty_send(tty, msg, 8, 0);
>   		if (rc) {
>   			usleep(msg_req_delay * 1000);
>   			continue;
> @@ -929,7 +932,7 @@ kwboot_xm_sendblock(int fd, struct kwboot_block *block, int allow_non_xm,
>   
>   	retries = 0;
>   	do {
> -		rc = kwboot_tty_send(fd, block, sizeof(*block));
> +		rc = kwboot_tty_send(fd, block, sizeof(*block), 1);
>   		if (rc)
>   			return rc;
>   
> 
> If it is kernel who does not fill next bytes into hw UART queue as it
> waits for tcdrain then above patch should fix it.

Still no ciguar. Here the end of the log with "-B 921600":

  97 % 
[......................................................................]
  99 % [........................... 
       ]
Done
Finishing transfer
Waiting 1s for baudrate change magic
xmodem: Connection timed out


> Or if you have other USB-UART cables, try another. I have good
> experience with pl2303-based.

I have tons of USB-UART adapters / cables. But as mentioned in the
other mail, I can't connect them, as the FTDI chip is soldered on the
target.

Still it should work at least with 115201 baud, even if baudrate
changing does not work. Right? So we have at least one other problem
here AFAICT.

>>>> Ok. This seems to work at least partly (SPL):
>>>>
>>>> [stefan@ryzen u-boot-marvell (kwboot-test1)]$ ./tools/kwboot -B 115201 -b
>>>> u-boot-spl.kwb -t
>>>> /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
>>>> Patching image boot signature to UART
>>>> Injecting binary header code for changing baudrate to 115201 Bd
>>>> Injecting code for changing baudrate back
>>>> Sending boot message. Please reboot the target...|
>>>> Waiting 2s and flushing tty
>>>> Sending boot image header (90112 bytes)...
>>>>     0 %
>>>> [......................................................................]
>>>>    10 %
>>>> [......................................................................]
>>>>    20 %
>>>> [......................................................................]
>>>>    29 %
>>>> [......................................................................]
>>>>    39 %
>>>> [......................................................................]
>>>>    49 %
>>>> [......................................................................]
>>>>    59 %
>>>> [......................................................................]
>>>>    69 %
>>>> [......................................................................]
>>>>    79 %
>>>> [......................................................................]
>>>>    89 %
>>>> [......................................................................]
>>>>    99 % [....       ]
>>>> Done
>>>>
>>>> U-Boot SPL 2021.10-00908-gc129aa2f173a (Oct 26 2021 - 10:39:55 +0200)
>>>> High speed PHY - Version: 2.1.5 (COM-PHY-V20)
>>>> High speed PHY - Ended Successfully
>>>> DDR3 Training Sequence - Ver 5.7.4
>>>> DDR3 Training Sequence - Ended Successfully
>>>> Trying to boot from BOOTROM
>>>> Returning to BootROM (return address 0xffff0aa0)...
>>>>
>>>> Changing baudrate to 115201 Bd
>>>>
>>>> Sending boot image data (549892 bytes)...
>>>>     0 %
>>>> [......................................................................]
>>>>     1 %
>>>> [......................................................................]
>>>> ...
>>>>    97 %
>>>> [......................................................................]
>>>>    99 % [...........................       ]
>>>> Done
>>>> Finishing transfer
>>>> Waiting 1s for baudrate change magic
>>>>
>>>> Changing baudrate back to 115200 Bd
>>>>
>>>> [Type Ctrl-\ + c to quit]
>>>>
>>>> BootROM 1.20
>>>> Booting from SPI flash
>>>>
>>>>
>>>> So the CPU has run through a reset here.
>>>
>>> What kind of CPU core has your AXP board? Maybe there is some arm
>>> instruction in kwboot_baud_code[] array which is not supported by that
>>> core (and so reset occurs)?
>>
>> AXP stands for Armada XP, which is ARM v7 AFAIK.
> 
> I remember that older Marvell Armada SoCs used custom designed ARM
> cores, so there could be some differences what is supported by licensed
> ARM core and custom ARM core.
> 
> Anyway, as changing baudrate after execution of SPL is working fine, it
> should mean that there is no undefined/unsupported instruction.
> 
> My another guess there could be a problem is usage of stack. Maybe it is
> possible that register with stack pointer is not initialized after the
> full transfer when going to execute main image. Same arm baudrate change
> code is used after the SPL and before main U-Boot, and the first
> instruction is "push". Maybe modifying the arm code to not use stack
> when switching the baudrate back to 115200 could also help. I will look
> at it.

Thanks.

>>> And could you send me commands which you are using for compiling U-Boot
>>> for that your AXP board? I would like to examine resulted binary.
>>
>> I'm currently using a kernel.org toolchain:
>>
>> [stefan@ryzen u-boot-marvell (kwboot-test1)]$ set | grep CROSS
>> CROSS_COMPILE=/opt/kernel.org/gcc-11.1.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-
>> [stefan@ryzen u-boot-marvell (kwboot-test1)]$
>> /opt/kernel.org/gcc-11.1.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-gcc
>> --version
>> arm-linux-gnueabi-gcc (GCC) 11.1.0
>> Copyright (C) 2021 Free Software Foundation, Inc.
>> This is free software; see the source for copying conditions.  There is NO
>> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
>>
>> Here my commands to generate the U-Boot binary:
>>
>> make mrproper
>> make theadorable_debug_defconfig
>> make -s -j20
> 
> Ok.
> 
>>>>> And what is the output of following command immediately after kwboot?
>>>>>
>>>>> stty -F /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
>>>>
>>>> Changing baudrate to 230400 Bd
>>>> Baudrate was not changed
>>>>
>>>>
>>>> xmodem: Protocol error
>>>> [stefan@ryzen u-boot-marvell (kwboot-test1)]$ stty -F
>>>> /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
>>>> speed 230400 baud; line = 0;
>>>
>>> ... but baudrate _was_ changed.
>>>
>>> So it looks like a bug in kwboot code. Try to apply above diff, it is
>>> really possible that you hit that logical error. (Interesting that
>>> neither Marek nor me saw it)
>>>
>>>> intr = <undef>; quit = <undef>; erase = <undef>; kill = <undef>; eof =
>>>> <undef>; start = <undef>; stop = <undef>; susp = <undef>; rprnt = <undef>;
>>>> werase = <undef>; lnext = <undef>; discard = <undef>; min = 1; time = 0;
>>>> -brkint -icrnl -imaxbel
>>>> -opost -onlcr
>>>> -isig -icanon -iexten -echo -echoe -echok -echoctl -echoke
>>>>
>>>>> Also, could try to catch whole strace log and send it to me?
>>>>
>>>> I'll try to do this later today.
>>>
>>> 'strace kwboot ... 1>log 2>&1' should do it...
>>
>> Okay, will follow in the next privatze mail.
> 
> Received. I have looked at it. And there are two interesting parts:
> 
> 1) Both parts of transfer (header at default speed 115200 and main at
> high speed 921600) were transferred without any error, except for the
> error indication of the last header packet. Error indication was sent
> after the execution of header itself (SPL and baudrate change), it means
> that there was no error and bootrom did not sent any error indication.
> kwboot did retransmission of this one packet and it succeeded (thanks to
> that fixup which I have sent).
> 
> So this error must have been just fluctuation on UART and probably
> caused by changing UART speed. I guess that your USB-UART chip needs
> more time to stabilize new speed and so USB-UART receiver lost/damaged
> reply byte sent by bootrom.
> 
> Maybe you could increase delay in arm baudrate change code, so CPU waits
> longer time before it returns to bootrom?
> 
> Replacing code in kwboot_baud_code[] array:
> 
> 				/*  ; Sleep 1ms ~~ 600000 cycles at 1200 MHz  */
> 				/*  ; r1 = 600000                             */
> 	0x9f, 0x1d, 0xa0, 0xe3, /* mov   r1, #0x27c0                          */
> 	0x09, 0x10, 0x40, 0xe3, /* movt  r1, #0x0009                          */
> 
> by:
> 
> 				/*  ; Sleep 10ms ~~ 6M cycles at 1200 MHz     */
> 				/*  ; r1 = 6000000                            */
> 	0x80, 0x1d, 0x08, 0xe3, /* mov   r1, #0x8d80                          */
> 	0x5b, 0x10, 0x40, 0xe3, /* movt  r1, #0x005b                          */
> 
> should do it.

Still no change. Also not in the download speed AFAICT.

...
  97 % 
[......................................................................]
  99 % [........................... 
       ]
Done
Finishing transfer
Waiting 1s for baudrate change magic

Changing baudrate back to 115200 Bd

[Type Ctrl-\ + c to quit]

BootROM 1.20

BTW: There is a external watchdog on the target, which might be the root
cause for the reset I'm seeing here. It kicks in at around 2-3 minutes
after power-up AFAIR.

> 2) Transfer was successful (EOT was ACKed) but there is absolutely no
> output on UART after the transfer. Which should indicate that bootrom
> should have started execution of the main image, but something crashed
> and board reset... Maybe it is really due to usage of stack...

Yep.

Thanks,
Stefan

Re: [PATCH u-boot-marvell 00/13] Yet another kwboot improvements

[Pali Rohár]

On Tuesday 26 October 2021 16:21:02 Stefan Roese wrote:
> On 26.10.21 14:40, Pali Rohár wrote:
> > On Tuesday 26 October 2021 13:09:42 Stefan Roese wrote:
> > > On 26.10.21 11:06, Pali Rohár wrote:
> > > 
> > > <snip>
> > > 
> > > > Now I found logical error in kwboot code which handles retransmission of
> > > > the last header packet. State of "baudrate change" is cleared on every
> > > > retransmission. Please apply following diff, so state variables are
> > > > initialized only once.
> > > > 
> > > > diff --git a/tools/kwboot.c b/tools/kwboot.c
> > > > index d38ee0065177..835ccc8c113a 100644
> > > > --- a/tools/kwboot.c
> > > > +++ b/tools/kwboot.c
> > > > @@ -859,11 +859,6 @@ kwboot_xm_recv_reply(int fd, char *c, int nak_on_non_xm,
> > > >    	uint64_t recv_until = _now() + timeout;
> > > >    	int rc;
> > > > -	if (non_xm_print)
> > > > -		*non_xm_print = 0;
> > > > -	if (baud_changed)
> > > > -		*baud_changed = 0;
> > > > -
> > > >    	while (1) {
> > > >    		rc = kwboot_tty_recv(fd, c, 1, timeout);
> > > >    		if (rc) {
> > > > @@ -929,6 +924,8 @@ kwboot_xm_sendblock(int fd, struct kwboot_block *block, int allow_non_xm,
> > > >    	char c;
> > > >    	*done_print = 0;
> > > > +	non_xm_print = 0;
> > > > +	baud_changed = 0;
> > > >    	retries = 0;
> > > >    	do {
> > > 
> > > This definitely helps (a bit). Now I get this:
> > 
> > Perfect! So this fixes issue with retransmission of the last header
> > packet.
> > 
> > But there is still issue with starting main U-Boot binary.
> > 
> > > [stefan@ryzen u-boot-marvell (kwboot-test1)]$ ./tools/kwboot -B 230400 -b
> > > u-boot-spl.kwb -t
> > > /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
> > > Patching image boot signature to UART
> > > Injecting binary header code for changing baudrate to 230400 Bd
> > > Injecting code for changing baudrate back
> > > Sending boot message. Please reboot the target...|
> > > Waiting 2s and flushing tty
> > > Sending boot image header (90112 bytes)...
> > >    0 %
> > > [......................................................................]
> > >   10 %
> > > [......................................................................]
> > >   20 %
> > > [......................................................................]
> > >   29 %
> > > [......................................................................]
> > >   39 %
> > > [......................................................................]
> > >   49 %
> > > [......................................................................]
> > >   59 %
> > > [......................................................................]
> > >   69 %
> > > [......................................................................]
> > >   79 %
> > > [......................................................................]
> > >   89 %
> > > [......................................................................]
> > >   99 % [....       ]
> > > Done
> > > 
> > > U-Boot SPL 2021.10-00908-gc129aa2f173a-dirty (Oct 26 2021 - 12:48:11 +0200)
> > > High speed PHY - Version: 2.1.5 (COM-PHY-V20)
> > > High speed PHY - Ended Successfully
> > > DDR3 Training Sequence - Ver 5.7.4
> > > DDR3 Training Sequence - Ended Successfully
> > > Trying to boot from BOOTROM
> > > Returning to BootROM (return address 0xffff0aa0)...
> > > 
> > > Changing baudrate to 230400 Bd
> > > 
> > > Sending boot image data (549892 bytes)...
> > >    0 %
> > > [......................................................................]
> > > 
> > > ...
> > > 
> > >   99 % [...........................       ]
> > > Done
> > > Finishing transfer
> > > Waiting 1s for baudrate change magic
> > > xmodem: Connection timed out
> > > 
> > > 
> > > BTW: The baudrate change does not seem to work or have any effect on
> > > the image download speed. Comparing 230400 and 921600 baud speeds, I can
> > > spot no real time difference here:
> > > 
> > > 230400 baud:
> > > [stefan@ryzen u-boot-marvell (kwboot-test1)]$ time ./tools/kwboot -B 230400
> > > -b u-boot-spl.kwb -t
> > > /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
> > > Patching image boot signature to UART
> > > Injecting binary header code for changing baudrate to 230400 Bd
> > > 
> > > ...
> > > 
> > >   99 % [...........................       ]
> > > Done
> > > Finishing transfer
> > > Waiting 1s for baudrate change magic
> > > xmodem: Connection timed out
> > > [2]+  Done                    emacs tools/kwboot.c
> > > 
> > > real    1m27,279s
> > > user    0m2,313s
> > > sys     0m0,337s
> > > 
> > > 921600 baud:
> > > [stefan@ryzen u-boot-marvell (kwboot-test1)]$ time ./tools/kwboot -B 921600
> > > -b u-boot-spl.kwb -t
> > > /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
> > > Patching image boot signature to UART
> > > Injecting binary header code for changing baudrate to 921600 Bd
> > > 
> > > ...
> > > 
> > >   99 % [...........................       ]
> > > Done
> > > Finishing transfer
> > > Waiting 1s for baudrate change magic
> > > xmodem: Connection timed out
> > > 
> > > real    1m27,839s
> > > user    0m0,224s
> > > sys     0m0,194s
> > > 
> > > Any idea why this is the case?
> > 
> > UARTs at both sides must be configured to 921600 speed, otherwise they
> > would not be able to communicate.
> 
> I'm also surpised here.
> 
> > What is happening here? I do not know. But as Marek told me that
> > observed same issue and replacing USB-UART cable by another decreased
> > transfer time. So I think that issue is somewhere in USB-UART
> > transmitter. My guess is that USB-UART transmitter send at 921600 speed:
> > start bit, 8 bits of data, stop bit and then loooong idle pause (logical
> > one). After that sends another 10 bits.
> > 
> > Maybe kernel is not filling next byte into USB-UART hw queue? Or USB-UART
> > cannot send more bytes faster and inserts those long idle pauses?
> > 
> > Could you try following diff which disables calling tcdrain() for xmodem
> > packets, to remove any waits between individual bytes?
> > 
> > diff --git a/tools/kwboot.c b/tools/kwboot.c
> > index 835ccc8c113a..5f4ff673972e 100644
> > --- a/tools/kwboot.c
> > +++ b/tools/kwboot.c
> > @@ -404,7 +404,7 @@ out:
> >   }
> >   static int
> > -kwboot_tty_send(int fd, const void *buf, size_t len)
> > +kwboot_tty_send(int fd, const void *buf, size_t len, int nodrain)
> >   {
> >   	if (!buf)
> >   		return 0;
> > @@ -412,13 +412,16 @@ kwboot_tty_send(int fd, const void *buf, size_t len)
> >   	if (kwboot_write(fd, buf, len) < 0)
> >   		return -1;
> > +	if (nodrain)
> > +		return 0;
> > +
> >   	return tcdrain(fd);
> >   }
> >   static int
> >   kwboot_tty_send_char(int fd, unsigned char c)
> >   {
> > -	return kwboot_tty_send(fd, &c, 1);
> > +	return kwboot_tty_send(fd, &c, 1, 0);
> >   }
> >   static speed_t
> > @@ -705,7 +708,7 @@ kwboot_bootmsg(int tty, void *msg)
> >   			break;
> >   		for (count = 0; count < 128; count++) {
> > -			rc = kwboot_tty_send(tty, msg, 8);
> > +			rc = kwboot_tty_send(tty, msg, 8, 0);
> >   			if (rc) {
> >   				usleep(msg_req_delay * 1000);
> >   				continue;
> > @@ -737,7 +740,7 @@ kwboot_debugmsg(int tty, void *msg)
> >   		if (rc)
> >   			break;
> > -		rc = kwboot_tty_send(tty, msg, 8);
> > +		rc = kwboot_tty_send(tty, msg, 8, 0);
> >   		if (rc) {
> >   			usleep(msg_req_delay * 1000);
> >   			continue;
> > @@ -929,7 +932,7 @@ kwboot_xm_sendblock(int fd, struct kwboot_block *block, int allow_non_xm,
> >   	retries = 0;
> >   	do {
> > -		rc = kwboot_tty_send(fd, block, sizeof(*block));
> > +		rc = kwboot_tty_send(fd, block, sizeof(*block), 1);
> >   		if (rc)
> >   			return rc;
> > 
> > If it is kernel who does not fill next bytes into hw UART queue as it
> > waits for tcdrain then above patch should fix it.
> 
> Still no ciguar. Here the end of the log with "-B 921600":

So no change in total transfer time with above patch?

>  97 %
> [......................................................................]
>  99 % [...........................       ]
> Done
> Finishing transfer
> Waiting 1s for baudrate change magic
> xmodem: Connection timed out
> 
> 
> > Or if you have other USB-UART cables, try another. I have good
> > experience with pl2303-based.
> 
> I have tons of USB-UART adapters / cables. But as mentioned in the
> other mail, I can't connect them, as the FTDI chip is soldered on the
> target.

Understood.

> Still it should work at least with 115201 baud, even if baudrate
> changing does not work. Right? So we have at least one other problem
> here AFAICT.

Baudrate change must work. Using 115201 baud could just show output from
board/bootrom if CPU prematurely resets (which resets baudrate to
default 115200).

> > > > > Ok. This seems to work at least partly (SPL):
> > > > > 
> > > > > [stefan@ryzen u-boot-marvell (kwboot-test1)]$ ./tools/kwboot -B 115201 -b
> > > > > u-boot-spl.kwb -t
> > > > > /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
> > > > > Patching image boot signature to UART
> > > > > Injecting binary header code for changing baudrate to 115201 Bd
> > > > > Injecting code for changing baudrate back
> > > > > Sending boot message. Please reboot the target...|
> > > > > Waiting 2s and flushing tty
> > > > > Sending boot image header (90112 bytes)...
> > > > >     0 %
> > > > > [......................................................................]
> > > > >    10 %
> > > > > [......................................................................]
> > > > >    20 %
> > > > > [......................................................................]
> > > > >    29 %
> > > > > [......................................................................]
> > > > >    39 %
> > > > > [......................................................................]
> > > > >    49 %
> > > > > [......................................................................]
> > > > >    59 %
> > > > > [......................................................................]
> > > > >    69 %
> > > > > [......................................................................]
> > > > >    79 %
> > > > > [......................................................................]
> > > > >    89 %
> > > > > [......................................................................]
> > > > >    99 % [....       ]
> > > > > Done
> > > > > 
> > > > > U-Boot SPL 2021.10-00908-gc129aa2f173a (Oct 26 2021 - 10:39:55 +0200)
> > > > > High speed PHY - Version: 2.1.5 (COM-PHY-V20)
> > > > > High speed PHY - Ended Successfully
> > > > > DDR3 Training Sequence - Ver 5.7.4
> > > > > DDR3 Training Sequence - Ended Successfully
> > > > > Trying to boot from BOOTROM
> > > > > Returning to BootROM (return address 0xffff0aa0)...
> > > > > 
> > > > > Changing baudrate to 115201 Bd
> > > > > 
> > > > > Sending boot image data (549892 bytes)...
> > > > >     0 %
> > > > > [......................................................................]
> > > > >     1 %
> > > > > [......................................................................]
> > > > > ...
> > > > >    97 %
> > > > > [......................................................................]
> > > > >    99 % [...........................       ]
> > > > > Done
> > > > > Finishing transfer
> > > > > Waiting 1s for baudrate change magic
> > > > > 
> > > > > Changing baudrate back to 115200 Bd
> > > > > 
> > > > > [Type Ctrl-\ + c to quit]
> > > > > 
> > > > > BootROM 1.20
> > > > > Booting from SPI flash
> > > > > 
> > > > > 
> > > > > So the CPU has run through a reset here.
> > > > 
> > > > What kind of CPU core has your AXP board? Maybe there is some arm
> > > > instruction in kwboot_baud_code[] array which is not supported by that
> > > > core (and so reset occurs)?
> > > 
> > > AXP stands for Armada XP, which is ARM v7 AFAIK.
> > 
> > I remember that older Marvell Armada SoCs used custom designed ARM
> > cores, so there could be some differences what is supported by licensed
> > ARM core and custom ARM core.
> > 
> > Anyway, as changing baudrate after execution of SPL is working fine, it
> > should mean that there is no undefined/unsupported instruction.
> > 
> > My another guess there could be a problem is usage of stack. Maybe it is
> > possible that register with stack pointer is not initialized after the
> > full transfer when going to execute main image. Same arm baudrate change
> > code is used after the SPL and before main U-Boot, and the first
> > instruction is "push". Maybe modifying the arm code to not use stack
> > when switching the baudrate back to 115200 could also help. I will look
> > at it.
> 
> Thanks.
> 
> > > > And could you send me commands which you are using for compiling U-Boot
> > > > for that your AXP board? I would like to examine resulted binary.
> > > 
> > > I'm currently using a kernel.org toolchain:
> > > 
> > > [stefan@ryzen u-boot-marvell (kwboot-test1)]$ set | grep CROSS
> > > CROSS_COMPILE=/opt/kernel.org/gcc-11.1.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-
> > > [stefan@ryzen u-boot-marvell (kwboot-test1)]$
> > > /opt/kernel.org/gcc-11.1.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-gcc
> > > --version
> > > arm-linux-gnueabi-gcc (GCC) 11.1.0
> > > Copyright (C) 2021 Free Software Foundation, Inc.
> > > This is free software; see the source for copying conditions.  There is NO
> > > warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> > > 
> > > Here my commands to generate the U-Boot binary:
> > > 
> > > make mrproper
> > > make theadorable_debug_defconfig
> > > make -s -j20
> > 
> > Ok.
> > 
> > > > > > And what is the output of following command immediately after kwboot?
> > > > > > 
> > > > > > stty -F /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
> > > > > 
> > > > > Changing baudrate to 230400 Bd
> > > > > Baudrate was not changed
> > > > > 
> > > > > 
> > > > > xmodem: Protocol error
> > > > > [stefan@ryzen u-boot-marvell (kwboot-test1)]$ stty -F
> > > > > /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
> > > > > speed 230400 baud; line = 0;
> > > > 
> > > > ... but baudrate _was_ changed.
> > > > 
> > > > So it looks like a bug in kwboot code. Try to apply above diff, it is
> > > > really possible that you hit that logical error. (Interesting that
> > > > neither Marek nor me saw it)
> > > > 
> > > > > intr = <undef>; quit = <undef>; erase = <undef>; kill = <undef>; eof =
> > > > > <undef>; start = <undef>; stop = <undef>; susp = <undef>; rprnt = <undef>;
> > > > > werase = <undef>; lnext = <undef>; discard = <undef>; min = 1; time = 0;
> > > > > -brkint -icrnl -imaxbel
> > > > > -opost -onlcr
> > > > > -isig -icanon -iexten -echo -echoe -echok -echoctl -echoke
> > > > > 
> > > > > > Also, could try to catch whole strace log and send it to me?
> > > > > 
> > > > > I'll try to do this later today.
> > > > 
> > > > 'strace kwboot ... 1>log 2>&1' should do it...
> > > 
> > > Okay, will follow in the next privatze mail.
> > 
> > Received. I have looked at it. And there are two interesting parts:
> > 
> > 1) Both parts of transfer (header at default speed 115200 and main at
> > high speed 921600) were transferred without any error, except for the
> > error indication of the last header packet. Error indication was sent
> > after the execution of header itself (SPL and baudrate change), it means
> > that there was no error and bootrom did not sent any error indication.
> > kwboot did retransmission of this one packet and it succeeded (thanks to
> > that fixup which I have sent).
> > 
> > So this error must have been just fluctuation on UART and probably
> > caused by changing UART speed. I guess that your USB-UART chip needs
> > more time to stabilize new speed and so USB-UART receiver lost/damaged
> > reply byte sent by bootrom.
> > 
> > Maybe you could increase delay in arm baudrate change code, so CPU waits
> > longer time before it returns to bootrom?
> > 
> > Replacing code in kwboot_baud_code[] array:
> > 
> > 				/*  ; Sleep 1ms ~~ 600000 cycles at 1200 MHz  */
> > 				/*  ; r1 = 600000                             */
> > 	0x9f, 0x1d, 0xa0, 0xe3, /* mov   r1, #0x27c0                          */
> > 	0x09, 0x10, 0x40, 0xe3, /* movt  r1, #0x0009                          */
> > 
> > by:
> > 
> > 				/*  ; Sleep 10ms ~~ 6M cycles at 1200 MHz     */
> > 				/*  ; r1 = 6000000                            */
> > 	0x80, 0x1d, 0x08, 0xe3, /* mov   r1, #0x8d80                          */
> > 	0x5b, 0x10, 0x40, 0xe3, /* movt  r1, #0x005b                          */
> > 
> > should do it.
> 
> Still no change. Also not in the download speed AFAICT.
> 
> ...
>  97 %
> [......................................................................]
>  99 % [...........................       ]
> Done
> Finishing transfer
> Waiting 1s for baudrate change magic
> 
> Changing baudrate back to 115200 Bd
> 
> [Type Ctrl-\ + c to quit]
> 
> BootROM 1.20
> 
> BTW: There is a external watchdog on the target, which might be the root
> cause for the reset I'm seeing here. It kicks in at around 2-3 minutes
> after power-up AFAIR.

But there is no more "xmodem: Connection timed out" error.

When that "BootROM 1.20" message was printed? Immediately after the
"Changing baudrate back to 115200 Bd"? Or it was e.g. minute later? I
would like to know if board reset relates to watchdog or if it relates
to baudrate change code.

If there are timeout errors related to "Waiting 1s for baudrate change
magic" then increasing 1s (1000 ms value) to e.g. 10s could help.

> > 2) Transfer was successful (EOT was ACKed) but there is absolutely no
> > output on UART after the transfer. Which should indicate that bootrom
> > should have started execution of the main image, but something crashed
> > and board reset... Maybe it is really due to usage of stack...
> 
> Yep.
> 
> Thanks,
> Stefan

Re: [PATCH u-boot-marvell 00/13] Yet another kwboot improvements

[Stefan Roese]

On 26.10.21 16:48, Pali Rohár wrote:
> On Tuesday 26 October 2021 16:21:02 Stefan Roese wrote:
>> On 26.10.21 14:40, Pali Rohár wrote:
>>> On Tuesday 26 October 2021 13:09:42 Stefan Roese wrote:
>>>> On 26.10.21 11:06, Pali Rohár wrote:
>>>>
>>>> <snip>
>>>>
>>>>> Now I found logical error in kwboot code which handles retransmission of
>>>>> the last header packet. State of "baudrate change" is cleared on every
>>>>> retransmission. Please apply following diff, so state variables are
>>>>> initialized only once.
>>>>>
>>>>> diff --git a/tools/kwboot.c b/tools/kwboot.c
>>>>> index d38ee0065177..835ccc8c113a 100644
>>>>> --- a/tools/kwboot.c
>>>>> +++ b/tools/kwboot.c
>>>>> @@ -859,11 +859,6 @@ kwboot_xm_recv_reply(int fd, char *c, int nak_on_non_xm,
>>>>>     	uint64_t recv_until = _now() + timeout;
>>>>>     	int rc;
>>>>> -	if (non_xm_print)
>>>>> -		*non_xm_print = 0;
>>>>> -	if (baud_changed)
>>>>> -		*baud_changed = 0;
>>>>> -
>>>>>     	while (1) {
>>>>>     		rc = kwboot_tty_recv(fd, c, 1, timeout);
>>>>>     		if (rc) {
>>>>> @@ -929,6 +924,8 @@ kwboot_xm_sendblock(int fd, struct kwboot_block *block, int allow_non_xm,
>>>>>     	char c;
>>>>>     	*done_print = 0;
>>>>> +	non_xm_print = 0;
>>>>> +	baud_changed = 0;
>>>>>     	retries = 0;
>>>>>     	do {
>>>>
>>>> This definitely helps (a bit). Now I get this:
>>>
>>> Perfect! So this fixes issue with retransmission of the last header
>>> packet.
>>>
>>> But there is still issue with starting main U-Boot binary.
>>>
>>>> [stefan@ryzen u-boot-marvell (kwboot-test1)]$ ./tools/kwboot -B 230400 -b
>>>> u-boot-spl.kwb -t
>>>> /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
>>>> Patching image boot signature to UART
>>>> Injecting binary header code for changing baudrate to 230400 Bd
>>>> Injecting code for changing baudrate back
>>>> Sending boot message. Please reboot the target...|
>>>> Waiting 2s and flushing tty
>>>> Sending boot image header (90112 bytes)...
>>>>     0 %
>>>> [......................................................................]
>>>>    10 %
>>>> [......................................................................]
>>>>    20 %
>>>> [......................................................................]
>>>>    29 %
>>>> [......................................................................]
>>>>    39 %
>>>> [......................................................................]
>>>>    49 %
>>>> [......................................................................]
>>>>    59 %
>>>> [......................................................................]
>>>>    69 %
>>>> [......................................................................]
>>>>    79 %
>>>> [......................................................................]
>>>>    89 %
>>>> [......................................................................]
>>>>    99 % [....       ]
>>>> Done
>>>>
>>>> U-Boot SPL 2021.10-00908-gc129aa2f173a-dirty (Oct 26 2021 - 12:48:11 +0200)
>>>> High speed PHY - Version: 2.1.5 (COM-PHY-V20)
>>>> High speed PHY - Ended Successfully
>>>> DDR3 Training Sequence - Ver 5.7.4
>>>> DDR3 Training Sequence - Ended Successfully
>>>> Trying to boot from BOOTROM
>>>> Returning to BootROM (return address 0xffff0aa0)...
>>>>
>>>> Changing baudrate to 230400 Bd
>>>>
>>>> Sending boot image data (549892 bytes)...
>>>>     0 %
>>>> [......................................................................]
>>>>
>>>> ...
>>>>
>>>>    99 % [...........................       ]
>>>> Done
>>>> Finishing transfer
>>>> Waiting 1s for baudrate change magic
>>>> xmodem: Connection timed out
>>>>
>>>>
>>>> BTW: The baudrate change does not seem to work or have any effect on
>>>> the image download speed. Comparing 230400 and 921600 baud speeds, I can
>>>> spot no real time difference here:
>>>>
>>>> 230400 baud:
>>>> [stefan@ryzen u-boot-marvell (kwboot-test1)]$ time ./tools/kwboot -B 230400
>>>> -b u-boot-spl.kwb -t
>>>> /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
>>>> Patching image boot signature to UART
>>>> Injecting binary header code for changing baudrate to 230400 Bd
>>>>
>>>> ...
>>>>
>>>>    99 % [...........................       ]
>>>> Done
>>>> Finishing transfer
>>>> Waiting 1s for baudrate change magic
>>>> xmodem: Connection timed out
>>>> [2]+  Done                    emacs tools/kwboot.c
>>>>
>>>> real    1m27,279s
>>>> user    0m2,313s
>>>> sys     0m0,337s
>>>>
>>>> 921600 baud:
>>>> [stefan@ryzen u-boot-marvell (kwboot-test1)]$ time ./tools/kwboot -B 921600
>>>> -b u-boot-spl.kwb -t
>>>> /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
>>>> Patching image boot signature to UART
>>>> Injecting binary header code for changing baudrate to 921600 Bd
>>>>
>>>> ...
>>>>
>>>>    99 % [...........................       ]
>>>> Done
>>>> Finishing transfer
>>>> Waiting 1s for baudrate change magic
>>>> xmodem: Connection timed out
>>>>
>>>> real    1m27,839s
>>>> user    0m0,224s
>>>> sys     0m0,194s
>>>>
>>>> Any idea why this is the case?
>>>
>>> UARTs at both sides must be configured to 921600 speed, otherwise they
>>> would not be able to communicate.
>>
>> I'm also surpised here.
>>
>>> What is happening here? I do not know. But as Marek told me that
>>> observed same issue and replacing USB-UART cable by another decreased
>>> transfer time. So I think that issue is somewhere in USB-UART
>>> transmitter. My guess is that USB-UART transmitter send at 921600 speed:
>>> start bit, 8 bits of data, stop bit and then loooong idle pause (logical
>>> one). After that sends another 10 bits.
>>>
>>> Maybe kernel is not filling next byte into USB-UART hw queue? Or USB-UART
>>> cannot send more bytes faster and inserts those long idle pauses?
>>>
>>> Could you try following diff which disables calling tcdrain() for xmodem
>>> packets, to remove any waits between individual bytes?
>>>
>>> diff --git a/tools/kwboot.c b/tools/kwboot.c
>>> index 835ccc8c113a..5f4ff673972e 100644
>>> --- a/tools/kwboot.c
>>> +++ b/tools/kwboot.c
>>> @@ -404,7 +404,7 @@ out:
>>>    }
>>>    static int
>>> -kwboot_tty_send(int fd, const void *buf, size_t len)
>>> +kwboot_tty_send(int fd, const void *buf, size_t len, int nodrain)
>>>    {
>>>    	if (!buf)
>>>    		return 0;
>>> @@ -412,13 +412,16 @@ kwboot_tty_send(int fd, const void *buf, size_t len)
>>>    	if (kwboot_write(fd, buf, len) < 0)
>>>    		return -1;
>>> +	if (nodrain)
>>> +		return 0;
>>> +
>>>    	return tcdrain(fd);
>>>    }
>>>    static int
>>>    kwboot_tty_send_char(int fd, unsigned char c)
>>>    {
>>> -	return kwboot_tty_send(fd, &c, 1);
>>> +	return kwboot_tty_send(fd, &c, 1, 0);
>>>    }
>>>    static speed_t
>>> @@ -705,7 +708,7 @@ kwboot_bootmsg(int tty, void *msg)
>>>    			break;
>>>    		for (count = 0; count < 128; count++) {
>>> -			rc = kwboot_tty_send(tty, msg, 8);
>>> +			rc = kwboot_tty_send(tty, msg, 8, 0);
>>>    			if (rc) {
>>>    				usleep(msg_req_delay * 1000);
>>>    				continue;
>>> @@ -737,7 +740,7 @@ kwboot_debugmsg(int tty, void *msg)
>>>    		if (rc)
>>>    			break;
>>> -		rc = kwboot_tty_send(tty, msg, 8);
>>> +		rc = kwboot_tty_send(tty, msg, 8, 0);
>>>    		if (rc) {
>>>    			usleep(msg_req_delay * 1000);
>>>    			continue;
>>> @@ -929,7 +932,7 @@ kwboot_xm_sendblock(int fd, struct kwboot_block *block, int allow_non_xm,
>>>    	retries = 0;
>>>    	do {
>>> -		rc = kwboot_tty_send(fd, block, sizeof(*block));
>>> +		rc = kwboot_tty_send(fd, block, sizeof(*block), 1);
>>>    		if (rc)
>>>    			return rc;
>>>
>>> If it is kernel who does not fill next bytes into hw UART queue as it
>>> waits for tcdrain then above patch should fix it.
>>
>> Still no ciguar. Here the end of the log with "-B 921600":
> 
> So no change in total transfer time with above patch?

No. AFAICT, it's always the same time. Like ~1.5 minutes for the overall
U-Boot image.

>>   97 %
>> [......................................................................]
>>   99 % [...........................       ]
>> Done
>> Finishing transfer
>> Waiting 1s for baudrate change magic
>> xmodem: Connection timed out
>>
>>
>>> Or if you have other USB-UART cables, try another. I have good
>>> experience with pl2303-based.
>>
>> I have tons of USB-UART adapters / cables. But as mentioned in the
>> other mail, I can't connect them, as the FTDI chip is soldered on the
>> target.
> 
> Understood.
> 
>> Still it should work at least with 115201 baud, even if baudrate
>> changing does not work. Right? So we have at least one other problem
>> here AFAICT.
> 
> Baudrate change must work. Using 115201 baud could just show output from
> board/bootrom if CPU prematurely resets (which resets baudrate to
> default 115200).
> 
>>>>>> Ok. This seems to work at least partly (SPL):
>>>>>>
>>>>>> [stefan@ryzen u-boot-marvell (kwboot-test1)]$ ./tools/kwboot -B 115201 -b
>>>>>> u-boot-spl.kwb -t
>>>>>> /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
>>>>>> Patching image boot signature to UART
>>>>>> Injecting binary header code for changing baudrate to 115201 Bd
>>>>>> Injecting code for changing baudrate back
>>>>>> Sending boot message. Please reboot the target...|
>>>>>> Waiting 2s and flushing tty
>>>>>> Sending boot image header (90112 bytes)...
>>>>>>      0 %
>>>>>> [......................................................................]
>>>>>>     10 %
>>>>>> [......................................................................]
>>>>>>     20 %
>>>>>> [......................................................................]
>>>>>>     29 %
>>>>>> [......................................................................]
>>>>>>     39 %
>>>>>> [......................................................................]
>>>>>>     49 %
>>>>>> [......................................................................]
>>>>>>     59 %
>>>>>> [......................................................................]
>>>>>>     69 %
>>>>>> [......................................................................]
>>>>>>     79 %
>>>>>> [......................................................................]
>>>>>>     89 %
>>>>>> [......................................................................]
>>>>>>     99 % [....       ]
>>>>>> Done
>>>>>>
>>>>>> U-Boot SPL 2021.10-00908-gc129aa2f173a (Oct 26 2021 - 10:39:55 +0200)
>>>>>> High speed PHY - Version: 2.1.5 (COM-PHY-V20)
>>>>>> High speed PHY - Ended Successfully
>>>>>> DDR3 Training Sequence - Ver 5.7.4
>>>>>> DDR3 Training Sequence - Ended Successfully
>>>>>> Trying to boot from BOOTROM
>>>>>> Returning to BootROM (return address 0xffff0aa0)...
>>>>>>
>>>>>> Changing baudrate to 115201 Bd
>>>>>>
>>>>>> Sending boot image data (549892 bytes)...
>>>>>>      0 %
>>>>>> [......................................................................]
>>>>>>      1 %
>>>>>> [......................................................................]
>>>>>> ...
>>>>>>     97 %
>>>>>> [......................................................................]
>>>>>>     99 % [...........................       ]
>>>>>> Done
>>>>>> Finishing transfer
>>>>>> Waiting 1s for baudrate change magic
>>>>>>
>>>>>> Changing baudrate back to 115200 Bd
>>>>>>
>>>>>> [Type Ctrl-\ + c to quit]
>>>>>>
>>>>>> BootROM 1.20
>>>>>> Booting from SPI flash
>>>>>>
>>>>>>
>>>>>> So the CPU has run through a reset here.
>>>>>
>>>>> What kind of CPU core has your AXP board? Maybe there is some arm
>>>>> instruction in kwboot_baud_code[] array which is not supported by that
>>>>> core (and so reset occurs)?
>>>>
>>>> AXP stands for Armada XP, which is ARM v7 AFAIK.
>>>
>>> I remember that older Marvell Armada SoCs used custom designed ARM
>>> cores, so there could be some differences what is supported by licensed
>>> ARM core and custom ARM core.
>>>
>>> Anyway, as changing baudrate after execution of SPL is working fine, it
>>> should mean that there is no undefined/unsupported instruction.
>>>
>>> My another guess there could be a problem is usage of stack. Maybe it is
>>> possible that register with stack pointer is not initialized after the
>>> full transfer when going to execute main image. Same arm baudrate change
>>> code is used after the SPL and before main U-Boot, and the first
>>> instruction is "push". Maybe modifying the arm code to not use stack
>>> when switching the baudrate back to 115200 could also help. I will look
>>> at it.
>>
>> Thanks.
>>
>>>>> And could you send me commands which you are using for compiling U-Boot
>>>>> for that your AXP board? I would like to examine resulted binary.
>>>>
>>>> I'm currently using a kernel.org toolchain:
>>>>
>>>> [stefan@ryzen u-boot-marvell (kwboot-test1)]$ set | grep CROSS
>>>> CROSS_COMPILE=/opt/kernel.org/gcc-11.1.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-
>>>> [stefan@ryzen u-boot-marvell (kwboot-test1)]$
>>>> /opt/kernel.org/gcc-11.1.0-nolibc/arm-linux-gnueabi/bin/arm-linux-gnueabi-gcc
>>>> --version
>>>> arm-linux-gnueabi-gcc (GCC) 11.1.0
>>>> Copyright (C) 2021 Free Software Foundation, Inc.
>>>> This is free software; see the source for copying conditions.  There is NO
>>>> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
>>>>
>>>> Here my commands to generate the U-Boot binary:
>>>>
>>>> make mrproper
>>>> make theadorable_debug_defconfig
>>>> make -s -j20
>>>
>>> Ok.
>>>
>>>>>>> And what is the output of following command immediately after kwboot?
>>>>>>>
>>>>>>> stty -F /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
>>>>>>
>>>>>> Changing baudrate to 230400 Bd
>>>>>> Baudrate was not changed
>>>>>>
>>>>>>
>>>>>> xmodem: Protocol error
>>>>>> [stefan@ryzen u-boot-marvell (kwboot-test1)]$ stty -F
>>>>>> /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
>>>>>> speed 230400 baud; line = 0;
>>>>>
>>>>> ... but baudrate _was_ changed.
>>>>>
>>>>> So it looks like a bug in kwboot code. Try to apply above diff, it is
>>>>> really possible that you hit that logical error. (Interesting that
>>>>> neither Marek nor me saw it)
>>>>>
>>>>>> intr = <undef>; quit = <undef>; erase = <undef>; kill = <undef>; eof =
>>>>>> <undef>; start = <undef>; stop = <undef>; susp = <undef>; rprnt = <undef>;
>>>>>> werase = <undef>; lnext = <undef>; discard = <undef>; min = 1; time = 0;
>>>>>> -brkint -icrnl -imaxbel
>>>>>> -opost -onlcr
>>>>>> -isig -icanon -iexten -echo -echoe -echok -echoctl -echoke
>>>>>>
>>>>>>> Also, could try to catch whole strace log and send it to me?
>>>>>>
>>>>>> I'll try to do this later today.
>>>>>
>>>>> 'strace kwboot ... 1>log 2>&1' should do it...
>>>>
>>>> Okay, will follow in the next privatze mail.
>>>
>>> Received. I have looked at it. And there are two interesting parts:
>>>
>>> 1) Both parts of transfer (header at default speed 115200 and main at
>>> high speed 921600) were transferred without any error, except for the
>>> error indication of the last header packet. Error indication was sent
>>> after the execution of header itself (SPL and baudrate change), it means
>>> that there was no error and bootrom did not sent any error indication.
>>> kwboot did retransmission of this one packet and it succeeded (thanks to
>>> that fixup which I have sent).
>>>
>>> So this error must have been just fluctuation on UART and probably
>>> caused by changing UART speed. I guess that your USB-UART chip needs
>>> more time to stabilize new speed and so USB-UART receiver lost/damaged
>>> reply byte sent by bootrom.
>>>
>>> Maybe you could increase delay in arm baudrate change code, so CPU waits
>>> longer time before it returns to bootrom?
>>>
>>> Replacing code in kwboot_baud_code[] array:
>>>
>>> 				/*  ; Sleep 1ms ~~ 600000 cycles at 1200 MHz  */
>>> 				/*  ; r1 = 600000                             */
>>> 	0x9f, 0x1d, 0xa0, 0xe3, /* mov   r1, #0x27c0                          */
>>> 	0x09, 0x10, 0x40, 0xe3, /* movt  r1, #0x0009                          */
>>>
>>> by:
>>>
>>> 				/*  ; Sleep 10ms ~~ 6M cycles at 1200 MHz     */
>>> 				/*  ; r1 = 6000000                            */
>>> 	0x80, 0x1d, 0x08, 0xe3, /* mov   r1, #0x8d80                          */
>>> 	0x5b, 0x10, 0x40, 0xe3, /* movt  r1, #0x005b                          */
>>>
>>> should do it.
>>
>> Still no change. Also not in the download speed AFAICT.
>>
>> ...
>>   97 %
>> [......................................................................]
>>   99 % [...........................       ]
>> Done
>> Finishing transfer
>> Waiting 1s for baudrate change magic
>>
>> Changing baudrate back to 115200 Bd
>>
>> [Type Ctrl-\ + c to quit]
>>
>> BootROM 1.20
>>
>> BTW: There is a external watchdog on the target, which might be the root
>> cause for the reset I'm seeing here. It kicks in at around 2-3 minutes
>> after power-up AFAIR.
> 
> But there is no more "xmodem: Connection timed out" error.
> 
> When that "BootROM 1.20" message was printed? Immediately after the
> "Changing baudrate back to 115200 Bd"? Or it was e.g. minute later? I
> would like to know if board reset relates to watchdog or if it relates
> to baudrate change code.

I've disabled the watchdog now. So it will not interfere with these
tests and confuse us any more.

I've not seen the reset any more now. So it's most likely only related
to the watchdog. Here the latest log with all your suggested changes:

[stefan@ryzen u-boot-marvell (kwboot-test1)]$ ./tools/kwboot -B 230400 
-b u-boot-spl.kwb -t 
/dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
Patching image boot signature to UART
Injecting binary header code for changing baudrate to 230400 Bd
Injecting code for changing baudrate back
Sending boot message. Please reboot the target...\
Waiting 2s and flushing tty
Sending boot image header (90112 bytes)...
   0 % 
[......................................................................]
  10 % 
[......................................................................]
  20 % 
[......................................................................]
  29 % 
[......................................................................]
  39 % 
[......................................................................]
  49 % 
[......................................................................]
  59 % 
[......................................................................]
  69 % 
[......................................................................]
  79 % 
[......................................................................]
  89 % 
[......................................................................]
  99 % [.... 
       ]
Done

U-Boot SPL 2021.10-00908-gc129aa2f173a-dirty (Oct 26 2021 - 17:02:40 +0200)
High speed PHY - Version: 2.1.5 (COM-PHY-V20)
High speed PHY - Ended Successfully
DDR3 Training Sequence - Ver 5.7.4
DDR3 Training Sequence - Ended Successfully
Trying to boot from BOOTROM
Returning to BootROM (return address 0xffff0aa0)...

Changing baudrate to 230400 Bd

Sending boot image data (548868 bytes)...
   0 % 
[......................................................................]
   1 % 
[......................................................................]
   3 % 
[......................................................................]
   4 % 
[......................................................................]
   6 % 
[......................................................................]
   8 % 
[......................................................................]
   9 % 
[......................................................................]
  11 % 
[......................................................................]
  13 % 
[......................................................................]
  14 % 
[......................................................................]
  16 % 
[......................................................................]
  17 % 
[......................................................................]
  19 % 
[......................................................................]
  21 % 
[......................................................................]
  22 % 
[......................................................................]
  24 % 
[......................................................................]
  26 % 
[......................................................................]
  27 % 
[......................................................................]
  29 % 
[......................................................................]
  31 % 
[......................................................................]
  32 % 
[......................................................................]
  34 % 
[......................................................................]
  35 % 
[......................................................................]
  37 % 
[......................................................................]
  39 % 
[......................................................................]
  40 % 
[......................................................................]
  42 % 
[......................................................................]
  44 % 
[......................................................................]
  45 % 
[......................................................................]
  47 % 
[......................................................................]
  48 % 
[......................................................................]
  50 % 
[......................................................................]
  52 % 
[......................................................................]
  53 % 
[......................................................................]
  55 % 
[......................................................................]
  57 % 
[......................................................................]
  58 % 
[......................................................................]
  60 % 
[......................................................................]
  62 % 
[......................................................................]
  63 % 
[......................................................................]
  65 % 
[......................................................................]
  66 % 
[......................................................................]
  68 % 
[......................................................................]
  70 % 
[......................................................................]
  71 % 
[......................................................................]
  73 % 
[......................................................................]
  75 % 
[......................................................................]
  76 % 
[......................................................................]
  78 % 
[......................................................................]
  80 % 
[......................................................................]
  81 % 
[......................................................................]
  83 % 
[......................................................................]
  84 % 
[......................................................................]
  86 % 
[......................................................................]
  88 % 
[......................................................................]
  89 % 
[......................................................................]
  91 % 
[......................................................................]
  93 % 
[......................................................................]
  94 % 
[......................................................................]
  96 % 
[......................................................................]
  97 % 
[......................................................................]
  99 % [................... 
       ]
Done
Finishing transfer
Waiting 1s for baudrate change magic

Changing baudrate back to 115200 Bd

[Type Ctrl-\ + c to quit]


Here is now just hangs forever. No output from main U-Boot proper at
all. This happed every time, when the baudrate is changed, meaning a
non 115200 -B is passed to kwboot. I checked with 115201 and here no
U-Boot proper output is printed as well.

> If there are timeout errors related to "Waiting 1s for baudrate change
> magic" then increasing 1s (1000 ms value) to e.g. 10s could help.

I changed the timeout to 10s without any change. It did not help.

Thanks,
Stefan

Re: [PATCH u-boot-marvell 00/13] Yet another kwboot improvements

[Marek Behún]

On Tue, 26 Oct 2021 17:13:05 +0200
Stefan Roese <sr@denx.de> wrote:

> Here is now just hangs forever. No output from main U-Boot proper at
> all. This happed every time, when the baudrate is changed, meaning a
> non 115200 -B is passed to kwboot. I checked with 115201 and here no
> U-Boot proper output is printed as well.
> 
> > If there are timeout errors related to "Waiting 1s for baudrate change
> > magic" then increasing 1s (1000 ms value) to e.g. 10s could help.  
> 
> I changed the timeout to 10s without any change. It did not help.

At what address is the image being sent to? Please send dumpimage
output for the kwb image.

Is it possible to connect JTAG to the board?

Marek

Re: [PATCH u-boot-marvell 00/13] Yet another kwboot improvements

[Stefan Roese]

On 26.10.21 17:20, Marek Behún wrote:
> On Tue, 26 Oct 2021 17:13:05 +0200
> Stefan Roese <sr@denx.de> wrote:
> 
>> Here is now just hangs forever. No output from main U-Boot proper at
>> all. This happed every time, when the baudrate is changed, meaning a
>> non 115200 -B is passed to kwboot. I checked with 115201 and here no
>> U-Boot proper output is printed as well.
>>
>>> If there are timeout errors related to "Waiting 1s for baudrate change
>>> magic" then increasing 1s (1000 ms value) to e.g. 10s could help.
>>
>> I changed the timeout to 10s without any change. It did not help.
> 
> At what address is the image being sent to? Please send dumpimage
> output for the kwb image.

[stefan@ryzen u-boot-marvell (kwboot-test1)]$ ./tools/dumpimage -l 
u-boot-spl.kwb
Image Type:   MVEBU Boot from spi Image
Image version:1
BIN Hdr Size: 89196 Bytes = 87.11 KiB = 0.09 MiB
Data Size:    548604 Bytes = 535.75 KiB = 0.52 MiB
Load Address: 00800000
Entry Point:  00800000

> Is it possible to connect JTAG to the board?

It is. But this would be a bit difficult for me, to dig all these old
tools out and find the correct configuration of the BDI.

Thanks,
Stefan

Re: [PATCH u-boot-marvell 00/13] Yet another kwboot improvements

[Marek Behún]

On Tue, 26 Oct 2021 17:25:10 +0200
Stefan Roese <sr@denx.de> wrote:

> On 26.10.21 17:20, Marek Behún wrote:
> > On Tue, 26 Oct 2021 17:13:05 +0200
> > Stefan Roese <sr@denx.de> wrote:
> >   
> >> Here is now just hangs forever. No output from main U-Boot proper at
> >> all. This happed every time, when the baudrate is changed, meaning a
> >> non 115200 -B is passed to kwboot. I checked with 115201 and here no
> >> U-Boot proper output is printed as well.
> >>  
> >>> If there are timeout errors related to "Waiting 1s for baudrate change
> >>> magic" then increasing 1s (1000 ms value) to e.g. 10s could help.  
> >>
> >> I changed the timeout to 10s without any change. It did not help.  
> > 
> > At what address is the image being sent to? Please send dumpimage
> > output for the kwb image.  
> 
> [stefan@ryzen u-boot-marvell (kwboot-test1)]$ ./tools/dumpimage -l 
> u-boot-spl.kwb
> Image Type:   MVEBU Boot from spi Image
> Image version:1
> BIN Hdr Size: 89196 Bytes = 87.11 KiB = 0.09 MiB
> Data Size:    548604 Bytes = 535.75 KiB = 0.52 MiB
> Load Address: 00800000
> Entry Point:  00800000

I think the baudrate changing code is not the problem, because it
executes correctly in the binhdr when increasing the baudrate.
Otherwise
  Changing baudrate to 230400 Bd
wouldn't be printed.

Stefan, did it previously work on this board? I mean at the time the
patches were first merged into u-boot-marvell Did the problem start
occuring only now, when the code is merged in u-boot master?

> > Is it possible to connect JTAG to the board?  
> 
> It is. But this would be a bit difficult for me, to dig all these old
> tools out and find the correct configuration of the BDI.

Yes, lets keep that as last resort.

Marek

Re: [PATCH u-boot-marvell 00/13] Yet another kwboot improvements

[Stefan Roese]

On 26.10.21 17:34, Marek Behún wrote:
> On Tue, 26 Oct 2021 17:25:10 +0200
> Stefan Roese <sr@denx.de> wrote:
> 
>> On 26.10.21 17:20, Marek Behún wrote:
>>> On Tue, 26 Oct 2021 17:13:05 +0200
>>> Stefan Roese <sr@denx.de> wrote:
>>>    
>>>> Here is now just hangs forever. No output from main U-Boot proper at
>>>> all. This happed every time, when the baudrate is changed, meaning a
>>>> non 115200 -B is passed to kwboot. I checked with 115201 and here no
>>>> U-Boot proper output is printed as well.
>>>>   
>>>>> If there are timeout errors related to "Waiting 1s for baudrate change
>>>>> magic" then increasing 1s (1000 ms value) to e.g. 10s could help.
>>>>
>>>> I changed the timeout to 10s without any change. It did not help.
>>>
>>> At what address is the image being sent to? Please send dumpimage
>>> output for the kwb image.
>>
>> [stefan@ryzen u-boot-marvell (kwboot-test1)]$ ./tools/dumpimage -l
>> u-boot-spl.kwb
>> Image Type:   MVEBU Boot from spi Image
>> Image version:1
>> BIN Hdr Size: 89196 Bytes = 87.11 KiB = 0.09 MiB
>> Data Size:    548604 Bytes = 535.75 KiB = 0.52 MiB
>> Load Address: 00800000
>> Entry Point:  00800000
> 
> I think the baudrate changing code is not the problem, because it
> executes correctly in the binhdr when increasing the baudrate.
> Otherwise
>    Changing baudrate to 230400 Bd
> wouldn't be printed.

Okay. Still it's unclear, why I don't see any speedup in the download
time here.

> Stefan, did it previously work on this board?

No...

> I mean at the time the
> patches were first merged into u-boot-marvell Did the problem start
> occuring only now, when the code is merged in u-boot master?

... Meaning, it still works when the baudrate is not changed at all.
I never tried with a baudrate != 115200 until your work in this area
here.

>>> Is it possible to connect JTAG to the board?
>>
>> It is. But this would be a bit difficult for me, to dig all these old
>> tools out and find the correct configuration of the BDI.
> 
> Yes, lets keep that as last resort.

Agreed.

Thanks,
Stefan

Re: [PATCH u-boot-marvell 00/13] Yet another kwboot improvements

[Pali Rohár]

On Tuesday 26 October 2021 16:21:02 Stefan Roese wrote:
> On 26.10.21 14:40, Pali Rohár wrote:
> > My another guess there could be a problem is usage of stack. Maybe it is
> > possible that register with stack pointer is not initialized after the
> > full transfer when going to execute main image. Same arm baudrate change
> > code is used after the SPL and before main U-Boot, and the first
> > instruction is "push". Maybe modifying the arm code to not use stack
> > when switching the baudrate back to 115200 could also help. I will look
> > at it.
> 
> Thanks.

Here is dirty hack patch which completely disable calling code for
changing baudrate back to 115200 on ARM side:

diff --git a/tools/kwboot.c b/tools/kwboot.c
index 5f4ff673972e..00d58a239c71 100644
--- a/tools/kwboot.c
+++ b/tools/kwboot.c
@@ -1070,17 +1070,17 @@ kwboot_xmodem(int tty, const void *_img, size_t size, int baudrate)
 		return rc;
 
 	if (baudrate) {
-		char buf[sizeof(kwb_baud_magic)];
-
-		kwboot_printv("Waiting 1s for baudrate change magic\n");
-		rc = kwboot_tty_recv(tty, buf, sizeof(buf), 1000);
-		if (rc)
-			return rc;
-
-		if (memcmp(buf, kwb_baud_magic, sizeof(buf))) {
-			errno = EPROTO;
-			return -1;
-		}
+//		char buf[sizeof(kwb_baud_magic)];
+//
+//		kwboot_printv("Waiting 1s for baudrate change magic\n");
+//		rc = kwboot_tty_recv(tty, buf, sizeof(buf), 1000);
+//		if (rc)
+//			return rc;
+//
+//		if (memcmp(buf, kwb_baud_magic, sizeof(buf))) {
+//			errno = EPROTO;
+//			return -1;
+//		}
 
 		kwboot_printv("\nChanging baudrate back to 115200 Bd\n\n");
 		rc = kwboot_tty_change_baudrate(tty, 115200);
@@ -1551,8 +1551,8 @@ kwboot_img_patch(void *img, size_t *size, int baudrate)
 		 * This code is appended after the data part of the image and
 		 * execaddr is changed to execute this code before U-Boot proper.
 		 */
-		kwboot_printv("Injecting code for changing baudrate back\n");
-		_copy_baudrate_change_code(hdr, size, 1, baudrate, 115200);
+//		kwboot_printv("Injecting code for changing baudrate back\n");
+//		_copy_baudrate_change_code(hdr, size, 1, baudrate, 115200);
 
 		/* recompute header size */
 		hdrsz = kwbheader_size(hdr);

As main U-Boot binary on ARM resets UART, it means that baudrate is
properly set to 115200. Probably beginning of the U-Boot output could be
lost but at least console should start.

Could you try this patch if it starts working now?

Re: [PATCH u-boot-marvell 00/13] Yet another kwboot improvements

[Stefan Roese]

On 26.10.21 20:48, Pali Rohár wrote:
> On Tuesday 26 October 2021 16:21:02 Stefan Roese wrote:
>> On 26.10.21 14:40, Pali Rohár wrote:
>>> My another guess there could be a problem is usage of stack. Maybe it is
>>> possible that register with stack pointer is not initialized after the
>>> full transfer when going to execute main image. Same arm baudrate change
>>> code is used after the SPL and before main U-Boot, and the first
>>> instruction is "push". Maybe modifying the arm code to not use stack
>>> when switching the baudrate back to 115200 could also help. I will look
>>> at it.
>>
>> Thanks.
> 
> Here is dirty hack patch which completely disable calling code for
> changing baudrate back to 115200 on ARM side:
> 
> diff --git a/tools/kwboot.c b/tools/kwboot.c
> index 5f4ff673972e..00d58a239c71 100644
> --- a/tools/kwboot.c
> +++ b/tools/kwboot.c
> @@ -1070,17 +1070,17 @@ kwboot_xmodem(int tty, const void *_img, size_t size, int baudrate)
>   		return rc;
>   
>   	if (baudrate) {
> -		char buf[sizeof(kwb_baud_magic)];
> -
> -		kwboot_printv("Waiting 1s for baudrate change magic\n");
> -		rc = kwboot_tty_recv(tty, buf, sizeof(buf), 1000);
> -		if (rc)
> -			return rc;
> -
> -		if (memcmp(buf, kwb_baud_magic, sizeof(buf))) {
> -			errno = EPROTO;
> -			return -1;
> -		}
> +//		char buf[sizeof(kwb_baud_magic)];
> +//
> +//		kwboot_printv("Waiting 1s for baudrate change magic\n");
> +//		rc = kwboot_tty_recv(tty, buf, sizeof(buf), 1000);
> +//		if (rc)
> +//			return rc;
> +//
> +//		if (memcmp(buf, kwb_baud_magic, sizeof(buf))) {
> +//			errno = EPROTO;
> +//			return -1;
> +//		}
>   
>   		kwboot_printv("\nChanging baudrate back to 115200 Bd\n\n");
>   		rc = kwboot_tty_change_baudrate(tty, 115200);
> @@ -1551,8 +1551,8 @@ kwboot_img_patch(void *img, size_t *size, int baudrate)
>   		 * This code is appended after the data part of the image and
>   		 * execaddr is changed to execute this code before U-Boot proper.
>   		 */
> -		kwboot_printv("Injecting code for changing baudrate back\n");
> -		_copy_baudrate_change_code(hdr, size, 1, baudrate, 115200);
> +//		kwboot_printv("Injecting code for changing baudrate back\n");
> +//		_copy_baudrate_change_code(hdr, size, 1, baudrate, 115200);

I do have this here in my version as well:

		/* Update the 32-bit data checksum */
		*kwboot_img_csum32_ptr(img) = kwboot_img_csum32(img);

		/* recompute header size */
		hdrsz = kwbheader_size(hdr);

So I'm using the newer version, just to be sure.

>   
>   		/* recompute header size */
>   		hdrsz = kwbheader_size(hdr);
> 
> As main U-Boot binary on ARM resets UART, it means that baudrate is
> properly set to 115200. Probably beginning of the U-Boot output could be
> lost but at least console should start.
> 
> Could you try this patch if it starts working now?

Okay, applied this patch and and booting with different baudrates works
on this board again (tested with 230400):

  96 % 
[......................................................................]
  98 % 
[......................................................................]
  99 % [................ 
       ]
Done
Finishing transfer

Changing baudrate back to 115200 Bd

[Type Ctrl-\ + c to quit]


U-Boot 2021.10-00908-gc129aa2f173a-dirty (Oct 27 2021 - 07:05:39 +0200)

SoC:   MV78260-B0 at 1333 MHz
I2C:   ready
DRAM:  2 GiB (667 MHz, 64-bit, ECC not enabled)
Loading Environment from SPIFlash... SF: Detected m25p128 with page size 
256 Bytes, erase size 256 KiB, total 16 MiB
OK
Model: Marvell Armada XP theadorable
...


Thanks,
Stefan

Re: [PATCH u-boot-marvell 00/13] Yet another kwboot improvements

[Pali Rohár]

On Wednesday 27 October 2021 07:09:42 Stefan Roese wrote:
> On 26.10.21 20:48, Pali Rohár wrote:
> > On Tuesday 26 October 2021 16:21:02 Stefan Roese wrote:
> > > On 26.10.21 14:40, Pali Rohár wrote:
> > > > My another guess there could be a problem is usage of stack. Maybe it is
> > > > possible that register with stack pointer is not initialized after the
> > > > full transfer when going to execute main image. Same arm baudrate change
> > > > code is used after the SPL and before main U-Boot, and the first
> > > > instruction is "push". Maybe modifying the arm code to not use stack
> > > > when switching the baudrate back to 115200 could also help. I will look
> > > > at it.
> > > 
> > > Thanks.
> > 
> > Here is dirty hack patch which completely disable calling code for
> > changing baudrate back to 115200 on ARM side:
> > 
> > diff --git a/tools/kwboot.c b/tools/kwboot.c
> > index 5f4ff673972e..00d58a239c71 100644
> > --- a/tools/kwboot.c
> > +++ b/tools/kwboot.c
> > @@ -1070,17 +1070,17 @@ kwboot_xmodem(int tty, const void *_img, size_t size, int baudrate)
> >   		return rc;
> >   	if (baudrate) {
> > -		char buf[sizeof(kwb_baud_magic)];
> > -
> > -		kwboot_printv("Waiting 1s for baudrate change magic\n");
> > -		rc = kwboot_tty_recv(tty, buf, sizeof(buf), 1000);
> > -		if (rc)
> > -			return rc;
> > -
> > -		if (memcmp(buf, kwb_baud_magic, sizeof(buf))) {
> > -			errno = EPROTO;
> > -			return -1;
> > -		}
> > +//		char buf[sizeof(kwb_baud_magic)];
> > +//
> > +//		kwboot_printv("Waiting 1s for baudrate change magic\n");
> > +//		rc = kwboot_tty_recv(tty, buf, sizeof(buf), 1000);
> > +//		if (rc)
> > +//			return rc;
> > +//
> > +//		if (memcmp(buf, kwb_baud_magic, sizeof(buf))) {
> > +//			errno = EPROTO;
> > +//			return -1;
> > +//		}
> >   		kwboot_printv("\nChanging baudrate back to 115200 Bd\n\n");
> >   		rc = kwboot_tty_change_baudrate(tty, 115200);
> > @@ -1551,8 +1551,8 @@ kwboot_img_patch(void *img, size_t *size, int baudrate)
> >   		 * This code is appended after the data part of the image and
> >   		 * execaddr is changed to execute this code before U-Boot proper.
> >   		 */
> > -		kwboot_printv("Injecting code for changing baudrate back\n");
> > -		_copy_baudrate_change_code(hdr, size, 1, baudrate, 115200);
> > +//		kwboot_printv("Injecting code for changing baudrate back\n");
> > +//		_copy_baudrate_change_code(hdr, size, 1, baudrate, 115200);
> 
> I do have this here in my version as well:
> 
> 		/* Update the 32-bit data checksum */
> 		*kwboot_img_csum32_ptr(img) = kwboot_img_csum32(img);
> 
> 		/* recompute header size */
> 		hdrsz = kwbheader_size(hdr);
> 
> So I'm using the newer version, just to be sure.

Ok, I probably generated diff against older version, but you have
figured out how to apply it.

> >   		/* recompute header size */
> >   		hdrsz = kwbheader_size(hdr);
> > 
> > As main U-Boot binary on ARM resets UART, it means that baudrate is
> > properly set to 115200. Probably beginning of the U-Boot output could be
> > lost but at least console should start.
> > 
> > Could you try this patch if it starts working now?
> 
> Okay, applied this patch and and booting with different baudrates works
> on this board again (tested with 230400):
> 
>  96 %
> [......................................................................]
>  98 %
> [......................................................................]
>  99 % [................       ]
> Done
> Finishing transfer
> 
> Changing baudrate back to 115200 Bd
> 
> [Type Ctrl-\ + c to quit]
> 
> 
> U-Boot 2021.10-00908-gc129aa2f173a-dirty (Oct 27 2021 - 07:05:39 +0200)
> 
> SoC:   MV78260-B0 at 1333 MHz
> I2C:   ready
> DRAM:  2 GiB (667 MHz, 64-bit, ECC not enabled)
> Loading Environment from SPIFlash... SF: Detected m25p128 with page size 256
> Bytes, erase size 256 KiB, total 16 MiB
> OK
> Model: Marvell Armada XP theadorable
> ...
> 
> 
> Thanks,
> Stefan

Perfect! So it really looks like that issue is in the code which resets
baudrate back to the value 115200.

I have there another diff which removes usage of the stack in code which
resets baudrate back to default value:

diff --git a/tools/kwboot.c b/tools/kwboot.c
index b56c9a0c8104..8f0e50501398 100644
--- a/tools/kwboot.c
+++ b/tools/kwboot.c
@@ -1444,6 +1444,11 @@ _inject_baudrate_change_code(void *img, size_t *size, int pre,
 	memcpy(code, kwboot_baud_code, codesz - 8);
 	*(uint32_t *)(code + codesz - 8) = cpu_to_le32(old_baud);
 	*(uint32_t *)(code + codesz - 4) = cpu_to_le32(new_baud);
+
+	if (!pre) {
+		*(uint32_t *)code = cpu_to_le32(0xe1a00000); /* arm nop */
+		*(uint32_t *)(code + codesz - 4*7) = cpu_to_le32(0xe12fff1e); /* bx lr */
+	}
 }
 
 static int

Could you try to apply this change instead of my previous one?

Re: [PATCH u-boot-marvell 00/13] Yet another kwboot improvements

[Pali Rohár]

On Wednesday 27 October 2021 15:52:47 Pali Rohár wrote:
> On Wednesday 27 October 2021 07:09:42 Stefan Roese wrote:
> > On 26.10.21 20:48, Pali Rohár wrote:
> > > On Tuesday 26 October 2021 16:21:02 Stefan Roese wrote:
> > > > On 26.10.21 14:40, Pali Rohár wrote:
> > > > > My another guess there could be a problem is usage of stack. Maybe it is
> > > > > possible that register with stack pointer is not initialized after the
> > > > > full transfer when going to execute main image. Same arm baudrate change
> > > > > code is used after the SPL and before main U-Boot, and the first
> > > > > instruction is "push". Maybe modifying the arm code to not use stack
> > > > > when switching the baudrate back to 115200 could also help. I will look
> > > > > at it.
> > > > 
> > > > Thanks.
> > > 
> > > Here is dirty hack patch which completely disable calling code for
> > > changing baudrate back to 115200 on ARM side:
> > > 
> > > diff --git a/tools/kwboot.c b/tools/kwboot.c
> > > index 5f4ff673972e..00d58a239c71 100644
> > > --- a/tools/kwboot.c
> > > +++ b/tools/kwboot.c
> > > @@ -1070,17 +1070,17 @@ kwboot_xmodem(int tty, const void *_img, size_t size, int baudrate)
> > >   		return rc;
> > >   	if (baudrate) {
> > > -		char buf[sizeof(kwb_baud_magic)];
> > > -
> > > -		kwboot_printv("Waiting 1s for baudrate change magic\n");
> > > -		rc = kwboot_tty_recv(tty, buf, sizeof(buf), 1000);
> > > -		if (rc)
> > > -			return rc;
> > > -
> > > -		if (memcmp(buf, kwb_baud_magic, sizeof(buf))) {
> > > -			errno = EPROTO;
> > > -			return -1;
> > > -		}
> > > +//		char buf[sizeof(kwb_baud_magic)];
> > > +//
> > > +//		kwboot_printv("Waiting 1s for baudrate change magic\n");
> > > +//		rc = kwboot_tty_recv(tty, buf, sizeof(buf), 1000);
> > > +//		if (rc)
> > > +//			return rc;
> > > +//
> > > +//		if (memcmp(buf, kwb_baud_magic, sizeof(buf))) {
> > > +//			errno = EPROTO;
> > > +//			return -1;
> > > +//		}
> > >   		kwboot_printv("\nChanging baudrate back to 115200 Bd\n\n");
> > >   		rc = kwboot_tty_change_baudrate(tty, 115200);
> > > @@ -1551,8 +1551,8 @@ kwboot_img_patch(void *img, size_t *size, int baudrate)
> > >   		 * This code is appended after the data part of the image and
> > >   		 * execaddr is changed to execute this code before U-Boot proper.
> > >   		 */
> > > -		kwboot_printv("Injecting code for changing baudrate back\n");
> > > -		_copy_baudrate_change_code(hdr, size, 1, baudrate, 115200);
> > > +//		kwboot_printv("Injecting code for changing baudrate back\n");
> > > +//		_copy_baudrate_change_code(hdr, size, 1, baudrate, 115200);
> > 
> > I do have this here in my version as well:
> > 
> > 		/* Update the 32-bit data checksum */
> > 		*kwboot_img_csum32_ptr(img) = kwboot_img_csum32(img);
> > 
> > 		/* recompute header size */
> > 		hdrsz = kwbheader_size(hdr);
> > 
> > So I'm using the newer version, just to be sure.
> 
> Ok, I probably generated diff against older version, but you have
> figured out how to apply it.
> 
> > >   		/* recompute header size */
> > >   		hdrsz = kwbheader_size(hdr);
> > > 
> > > As main U-Boot binary on ARM resets UART, it means that baudrate is
> > > properly set to 115200. Probably beginning of the U-Boot output could be
> > > lost but at least console should start.
> > > 
> > > Could you try this patch if it starts working now?
> > 
> > Okay, applied this patch and and booting with different baudrates works
> > on this board again (tested with 230400):
> > 
> >  96 %
> > [......................................................................]
> >  98 %
> > [......................................................................]
> >  99 % [................       ]
> > Done
> > Finishing transfer
> > 
> > Changing baudrate back to 115200 Bd
> > 
> > [Type Ctrl-\ + c to quit]
> > 
> > 
> > U-Boot 2021.10-00908-gc129aa2f173a-dirty (Oct 27 2021 - 07:05:39 +0200)
> > 
> > SoC:   MV78260-B0 at 1333 MHz
> > I2C:   ready
> > DRAM:  2 GiB (667 MHz, 64-bit, ECC not enabled)
> > Loading Environment from SPIFlash... SF: Detected m25p128 with page size 256
> > Bytes, erase size 256 KiB, total 16 MiB
> > OK
> > Model: Marvell Armada XP theadorable
> > ...
> > 
> > 
> > Thanks,
> > Stefan
> 
> Perfect! So it really looks like that issue is in the code which resets
> baudrate back to the value 115200.
> 
> I have there another diff which removes usage of the stack in code which
> resets baudrate back to default value:
> 
> diff --git a/tools/kwboot.c b/tools/kwboot.c
> index b56c9a0c8104..8f0e50501398 100644
> --- a/tools/kwboot.c
> +++ b/tools/kwboot.c
> @@ -1444,6 +1444,11 @@ _inject_baudrate_change_code(void *img, size_t *size, int pre,
>  	memcpy(code, kwboot_baud_code, codesz - 8);
>  	*(uint32_t *)(code + codesz - 8) = cpu_to_le32(old_baud);
>  	*(uint32_t *)(code + codesz - 4) = cpu_to_le32(new_baud);
> +
> +	if (!pre) {

Ou, there is a mistake, it should be "if (pre) {"

> +		*(uint32_t *)code = cpu_to_le32(0xe1a00000); /* arm nop */
> +		*(uint32_t *)(code + codesz - 4*7) = cpu_to_le32(0xe12fff1e); /* bx lr */
> +	}
>  }
>  
>  static int
> 
> Could you try to apply this change instead of my previous one?

Re: [PATCH u-boot-marvell 00/13] Yet another kwboot improvements

[Marek Behún]

On Wed, 27 Oct 2021 16:10:21 +0200
Pali Rohár <pali@kernel.org> wrote:

> On Wednesday 27 October 2021 15:52:47 Pali Rohár wrote:
> > On Wednesday 27 October 2021 07:09:42 Stefan Roese wrote:  
> > > On 26.10.21 20:48, Pali Rohár wrote:  
> > > > On Tuesday 26 October 2021 16:21:02 Stefan Roese wrote:  
> > > > > On 26.10.21 14:40, Pali Rohár wrote:  
> > > > > > My another guess there could be a problem is usage of stack. Maybe it is
> > > > > > possible that register with stack pointer is not initialized after the
> > > > > > full transfer when going to execute main image. Same arm baudrate change
> > > > > > code is used after the SPL and before main U-Boot, and the first
> > > > > > instruction is "push". Maybe modifying the arm code to not use stack
> > > > > > when switching the baudrate back to 115200 could also help. I will look
> > > > > > at it.  
> > > > > 
> > > > > Thanks.  
> > > > 
> > > > Here is dirty hack patch which completely disable calling code for
> > > > changing baudrate back to 115200 on ARM side:
> > > > 
> > > > diff --git a/tools/kwboot.c b/tools/kwboot.c
> > > > index 5f4ff673972e..00d58a239c71 100644
> > > > --- a/tools/kwboot.c
> > > > +++ b/tools/kwboot.c
> > > > @@ -1070,17 +1070,17 @@ kwboot_xmodem(int tty, const void *_img, size_t size, int baudrate)
> > > >   		return rc;
> > > >   	if (baudrate) {
> > > > -		char buf[sizeof(kwb_baud_magic)];
> > > > -
> > > > -		kwboot_printv("Waiting 1s for baudrate change magic\n");
> > > > -		rc = kwboot_tty_recv(tty, buf, sizeof(buf), 1000);
> > > > -		if (rc)
> > > > -			return rc;
> > > > -
> > > > -		if (memcmp(buf, kwb_baud_magic, sizeof(buf))) {
> > > > -			errno = EPROTO;
> > > > -			return -1;
> > > > -		}
> > > > +//		char buf[sizeof(kwb_baud_magic)];
> > > > +//
> > > > +//		kwboot_printv("Waiting 1s for baudrate change magic\n");
> > > > +//		rc = kwboot_tty_recv(tty, buf, sizeof(buf), 1000);
> > > > +//		if (rc)
> > > > +//			return rc;
> > > > +//
> > > > +//		if (memcmp(buf, kwb_baud_magic, sizeof(buf))) {
> > > > +//			errno = EPROTO;
> > > > +//			return -1;
> > > > +//		}
> > > >   		kwboot_printv("\nChanging baudrate back to 115200 Bd\n\n");
> > > >   		rc = kwboot_tty_change_baudrate(tty, 115200);
> > > > @@ -1551,8 +1551,8 @@ kwboot_img_patch(void *img, size_t *size, int baudrate)
> > > >   		 * This code is appended after the data part of the image and
> > > >   		 * execaddr is changed to execute this code before U-Boot proper.
> > > >   		 */
> > > > -		kwboot_printv("Injecting code for changing baudrate back\n");
> > > > -		_copy_baudrate_change_code(hdr, size, 1, baudrate, 115200);
> > > > +//		kwboot_printv("Injecting code for changing baudrate back\n");
> > > > +//		_copy_baudrate_change_code(hdr, size, 1, baudrate, 115200);  
> > > 
> > > I do have this here in my version as well:
> > > 
> > > 		/* Update the 32-bit data checksum */
> > > 		*kwboot_img_csum32_ptr(img) = kwboot_img_csum32(img);
> > > 
> > > 		/* recompute header size */
> > > 		hdrsz = kwbheader_size(hdr);
> > > 
> > > So I'm using the newer version, just to be sure.  
> > 
> > Ok, I probably generated diff against older version, but you have
> > figured out how to apply it.
> >   
> > > >   		/* recompute header size */
> > > >   		hdrsz = kwbheader_size(hdr);
> > > > 
> > > > As main U-Boot binary on ARM resets UART, it means that baudrate is
> > > > properly set to 115200. Probably beginning of the U-Boot output could be
> > > > lost but at least console should start.
> > > > 
> > > > Could you try this patch if it starts working now?  
> > > 
> > > Okay, applied this patch and and booting with different baudrates works
> > > on this board again (tested with 230400):
> > > 
> > >  96 %
> > > [......................................................................]
> > >  98 %
> > > [......................................................................]
> > >  99 % [................       ]
> > > Done
> > > Finishing transfer
> > > 
> > > Changing baudrate back to 115200 Bd
> > > 
> > > [Type Ctrl-\ + c to quit]
> > > 
> > > 
> > > U-Boot 2021.10-00908-gc129aa2f173a-dirty (Oct 27 2021 - 07:05:39 +0200)
> > > 
> > > SoC:   MV78260-B0 at 1333 MHz
> > > I2C:   ready
> > > DRAM:  2 GiB (667 MHz, 64-bit, ECC not enabled)
> > > Loading Environment from SPIFlash... SF: Detected m25p128 with page size 256
> > > Bytes, erase size 256 KiB, total 16 MiB
> > > OK
> > > Model: Marvell Armada XP theadorable
> > > ...
> > > 
> > > 
> > > Thanks,
> > > Stefan  
> > 
> > Perfect! So it really looks like that issue is in the code which resets
> > baudrate back to the value 115200.
> > 
> > I have there another diff which removes usage of the stack in code which
> > resets baudrate back to default value:
> > 
> > diff --git a/tools/kwboot.c b/tools/kwboot.c
> > index b56c9a0c8104..8f0e50501398 100644
> > --- a/tools/kwboot.c
> > +++ b/tools/kwboot.c
> > @@ -1444,6 +1444,11 @@ _inject_baudrate_change_code(void *img, size_t *size, int pre,
> >  	memcpy(code, kwboot_baud_code, codesz - 8);
> >  	*(uint32_t *)(code + codesz - 8) = cpu_to_le32(old_baud);
> >  	*(uint32_t *)(code + codesz - 4) = cpu_to_le32(new_baud);
> > +
> > +	if (!pre) {  
> 
> Ou, there is a mistake, it should be "if (pre) {"
> 
> > +		*(uint32_t *)code = cpu_to_le32(0xe1a00000); /* arm nop */
> > +		*(uint32_t *)(code + codesz - 4*7) = cpu_to_le32(0xe12fff1e); /* bx lr */
> > +	}

This fixes it for me as well, for that one Omnia which didn't work
which I was debugging a few days ago.

I guess this can't be done in the binhdr? So we need 2 different
versions? I don't quite like this ad-hoc change, but I also don't
like two copies with just a small change between them...

Marek

Re: [PATCH u-boot-marvell 00/13] Yet another kwboot improvements

[Pali Rohár]

On Wednesday 27 October 2021 17:08:35 Marek Behún wrote:
> On Wed, 27 Oct 2021 16:10:21 +0200
> Pali Rohár <pali@kernel.org> wrote:
> 
> > On Wednesday 27 October 2021 15:52:47 Pali Rohár wrote:
> > > On Wednesday 27 October 2021 07:09:42 Stefan Roese wrote:  
> > > > On 26.10.21 20:48, Pali Rohár wrote:  
> > > > > On Tuesday 26 October 2021 16:21:02 Stefan Roese wrote:  
> > > > > > On 26.10.21 14:40, Pali Rohár wrote:  
> > > > > > > My another guess there could be a problem is usage of stack. Maybe it is
> > > > > > > possible that register with stack pointer is not initialized after the
> > > > > > > full transfer when going to execute main image. Same arm baudrate change
> > > > > > > code is used after the SPL and before main U-Boot, and the first
> > > > > > > instruction is "push". Maybe modifying the arm code to not use stack
> > > > > > > when switching the baudrate back to 115200 could also help. I will look
> > > > > > > at it.  
> > > > > > 
> > > > > > Thanks.  
> > > > > 
> > > > > Here is dirty hack patch which completely disable calling code for
> > > > > changing baudrate back to 115200 on ARM side:
> > > > > 
> > > > > diff --git a/tools/kwboot.c b/tools/kwboot.c
> > > > > index 5f4ff673972e..00d58a239c71 100644
> > > > > --- a/tools/kwboot.c
> > > > > +++ b/tools/kwboot.c
> > > > > @@ -1070,17 +1070,17 @@ kwboot_xmodem(int tty, const void *_img, size_t size, int baudrate)
> > > > >   		return rc;
> > > > >   	if (baudrate) {
> > > > > -		char buf[sizeof(kwb_baud_magic)];
> > > > > -
> > > > > -		kwboot_printv("Waiting 1s for baudrate change magic\n");
> > > > > -		rc = kwboot_tty_recv(tty, buf, sizeof(buf), 1000);
> > > > > -		if (rc)
> > > > > -			return rc;
> > > > > -
> > > > > -		if (memcmp(buf, kwb_baud_magic, sizeof(buf))) {
> > > > > -			errno = EPROTO;
> > > > > -			return -1;
> > > > > -		}
> > > > > +//		char buf[sizeof(kwb_baud_magic)];
> > > > > +//
> > > > > +//		kwboot_printv("Waiting 1s for baudrate change magic\n");
> > > > > +//		rc = kwboot_tty_recv(tty, buf, sizeof(buf), 1000);
> > > > > +//		if (rc)
> > > > > +//			return rc;
> > > > > +//
> > > > > +//		if (memcmp(buf, kwb_baud_magic, sizeof(buf))) {
> > > > > +//			errno = EPROTO;
> > > > > +//			return -1;
> > > > > +//		}
> > > > >   		kwboot_printv("\nChanging baudrate back to 115200 Bd\n\n");
> > > > >   		rc = kwboot_tty_change_baudrate(tty, 115200);
> > > > > @@ -1551,8 +1551,8 @@ kwboot_img_patch(void *img, size_t *size, int baudrate)
> > > > >   		 * This code is appended after the data part of the image and
> > > > >   		 * execaddr is changed to execute this code before U-Boot proper.
> > > > >   		 */
> > > > > -		kwboot_printv("Injecting code for changing baudrate back\n");
> > > > > -		_copy_baudrate_change_code(hdr, size, 1, baudrate, 115200);
> > > > > +//		kwboot_printv("Injecting code for changing baudrate back\n");
> > > > > +//		_copy_baudrate_change_code(hdr, size, 1, baudrate, 115200);  
> > > > 
> > > > I do have this here in my version as well:
> > > > 
> > > > 		/* Update the 32-bit data checksum */
> > > > 		*kwboot_img_csum32_ptr(img) = kwboot_img_csum32(img);
> > > > 
> > > > 		/* recompute header size */
> > > > 		hdrsz = kwbheader_size(hdr);
> > > > 
> > > > So I'm using the newer version, just to be sure.  
> > > 
> > > Ok, I probably generated diff against older version, but you have
> > > figured out how to apply it.
> > >   
> > > > >   		/* recompute header size */
> > > > >   		hdrsz = kwbheader_size(hdr);
> > > > > 
> > > > > As main U-Boot binary on ARM resets UART, it means that baudrate is
> > > > > properly set to 115200. Probably beginning of the U-Boot output could be
> > > > > lost but at least console should start.
> > > > > 
> > > > > Could you try this patch if it starts working now?  
> > > > 
> > > > Okay, applied this patch and and booting with different baudrates works
> > > > on this board again (tested with 230400):
> > > > 
> > > >  96 %
> > > > [......................................................................]
> > > >  98 %
> > > > [......................................................................]
> > > >  99 % [................       ]
> > > > Done
> > > > Finishing transfer
> > > > 
> > > > Changing baudrate back to 115200 Bd
> > > > 
> > > > [Type Ctrl-\ + c to quit]
> > > > 
> > > > 
> > > > U-Boot 2021.10-00908-gc129aa2f173a-dirty (Oct 27 2021 - 07:05:39 +0200)
> > > > 
> > > > SoC:   MV78260-B0 at 1333 MHz
> > > > I2C:   ready
> > > > DRAM:  2 GiB (667 MHz, 64-bit, ECC not enabled)
> > > > Loading Environment from SPIFlash... SF: Detected m25p128 with page size 256
> > > > Bytes, erase size 256 KiB, total 16 MiB
> > > > OK
> > > > Model: Marvell Armada XP theadorable
> > > > ...
> > > > 
> > > > 
> > > > Thanks,
> > > > Stefan  
> > > 
> > > Perfect! So it really looks like that issue is in the code which resets
> > > baudrate back to the value 115200.
> > > 
> > > I have there another diff which removes usage of the stack in code which
> > > resets baudrate back to default value:
> > > 
> > > diff --git a/tools/kwboot.c b/tools/kwboot.c
> > > index b56c9a0c8104..8f0e50501398 100644
> > > --- a/tools/kwboot.c
> > > +++ b/tools/kwboot.c
> > > @@ -1444,6 +1444,11 @@ _inject_baudrate_change_code(void *img, size_t *size, int pre,
> > >  	memcpy(code, kwboot_baud_code, codesz - 8);
> > >  	*(uint32_t *)(code + codesz - 8) = cpu_to_le32(old_baud);
> > >  	*(uint32_t *)(code + codesz - 4) = cpu_to_le32(new_baud);
> > > +
> > > +	if (!pre) {  
> > 
> > Ou, there is a mistake, it should be "if (pre) {"
> > 
> > > +		*(uint32_t *)code = cpu_to_le32(0xe1a00000); /* arm nop */
> > > +		*(uint32_t *)(code + codesz - 4*7) = cpu_to_le32(0xe12fff1e); /* bx lr */
> > > +	}
> 
> This fixes it for me as well, for that one Omnia which didn't work
> which I was debugging a few days ago.
> 
> I guess this can't be done in the binhdr? So we need 2 different
> versions? I don't quite like this ad-hoc change, but I also don't
> like two copies with just a small change between them...
> 
> Marek

I will prepare cleanup / proper patch with one (configurable) version.
Just I need to know if this change fixes issue also for AXP.

Re: [PATCH u-boot-marvell 00/13] Yet another kwboot improvements

[Stefan Roese]

On 27.10.21 16:10, Pali Rohár wrote:

<snip>

>> Perfect! So it really looks like that issue is in the code which resets
>> baudrate back to the value 115200.
>>
>> I have there another diff which removes usage of the stack in code which
>> resets baudrate back to default value:
>>
>> diff --git a/tools/kwboot.c b/tools/kwboot.c
>> index b56c9a0c8104..8f0e50501398 100644
>> --- a/tools/kwboot.c
>> +++ b/tools/kwboot.c
>> @@ -1444,6 +1444,11 @@ _inject_baudrate_change_code(void *img, size_t *size, int pre,
>>   	memcpy(code, kwboot_baud_code, codesz - 8);
>>   	*(uint32_t *)(code + codesz - 8) = cpu_to_le32(old_baud);
>>   	*(uint32_t *)(code + codesz - 4) = cpu_to_le32(new_baud);
>> +
>> +	if (!pre) {
> 
> Ou, there is a mistake, it should be "if (pre) {"
> 
>> +		*(uint32_t *)code = cpu_to_le32(0xe1a00000); /* arm nop */
>> +		*(uint32_t *)(code + codesz - 4*7) = cpu_to_le32(0xe12fff1e); /* bx lr */
>> +	}
>>   }
>>   
>>   static int
>>
>> Could you try to apply this change instead of my previous one?

Tested with this corrected patch:

  97 % 
[......................................................................]
  99 % [................... 
       ]
Done
Finishing transfer
Waiting 2s for baudrate change magic

Changing baudrate back to 115200 Bd

[Type Ctrl-\ + c to quit]


U-Boot 2021.10-00916-gc6142e537e88-dirty (Oct 27 2021 - 17:23:24 +0200)

SoC:   MV78260-B0 at 1333 MHz
I2C:   ready
DRAM:  2 GiB (667 MHz, 64-bit, ECC not enabled)
Loading Environment from SPIFlash... SF: Detected m25p128 with page size 
256 Bytes, erase size 256 KiB, total 16 MiB
OK
Model: Marvell Armada XP theadorable
...

Perfect. This works on Armada XP. :)

Still I see no speed change. But this is a different story...

Thanks,
Stefan

Re: [PATCH u-boot-marvell 00/13] Yet another kwboot improvements

[Pali Rohár]

On Wednesday 27 October 2021 17:27:41 Stefan Roese wrote:
> On 27.10.21 16:10, Pali Rohár wrote:
> 
> <snip>
> 
> > > Perfect! So it really looks like that issue is in the code which resets
> > > baudrate back to the value 115200.
> > > 
> > > I have there another diff which removes usage of the stack in code which
> > > resets baudrate back to default value:
> > > 
> > > diff --git a/tools/kwboot.c b/tools/kwboot.c
> > > index b56c9a0c8104..8f0e50501398 100644
> > > --- a/tools/kwboot.c
> > > +++ b/tools/kwboot.c
> > > @@ -1444,6 +1444,11 @@ _inject_baudrate_change_code(void *img, size_t *size, int pre,
> > >   	memcpy(code, kwboot_baud_code, codesz - 8);
> > >   	*(uint32_t *)(code + codesz - 8) = cpu_to_le32(old_baud);
> > >   	*(uint32_t *)(code + codesz - 4) = cpu_to_le32(new_baud);
> > > +
> > > +	if (!pre) {
> > 
> > Ou, there is a mistake, it should be "if (pre) {"
> > 
> > > +		*(uint32_t *)code = cpu_to_le32(0xe1a00000); /* arm nop */
> > > +		*(uint32_t *)(code + codesz - 4*7) = cpu_to_le32(0xe12fff1e); /* bx lr */
> > > +	}
> > >   }
> > >   static int
> > > 
> > > Could you try to apply this change instead of my previous one?
> 
> Tested with this corrected patch:
> 
>  97 %
> [......................................................................]
>  99 % [...................       ]
> Done
> Finishing transfer
> Waiting 2s for baudrate change magic
> 
> Changing baudrate back to 115200 Bd
> 
> [Type Ctrl-\ + c to quit]
> 
> 
> U-Boot 2021.10-00916-gc6142e537e88-dirty (Oct 27 2021 - 17:23:24 +0200)
> 
> SoC:   MV78260-B0 at 1333 MHz
> I2C:   ready
> DRAM:  2 GiB (667 MHz, 64-bit, ECC not enabled)
> Loading Environment from SPIFlash... SF: Detected m25p128 with page size 256
> Bytes, erase size 256 KiB, total 16 MiB
> OK
> Model: Marvell Armada XP theadorable
> ...
> 
> Perfect. This works on Armada XP. :)

Nice! Therefore I will prepare proper patch for removing stack usage.

> Still I see no speed change. But this is a different story...
> 
> Thanks,
> Stefan

Re: [PATCH u-boot-marvell 00/13] Yet another kwboot improvements

[Pali Rohár]

On Wednesday 27 October 2021 17:27:41 Stefan Roese wrote:
> Still I see no speed change. But this is a different story...

Could you try to upload some file in u-boot via xmodem at different
speeds? E.g. via loadx command:

loadx <addr> 921600

and

loadx <addr> 115200

And compare if xmodem transfer is in U-Boot faster or not. Ideally also
try via kermit protocol at different speeds as kernel supports
streaming.

This could verify if issue is in kwboot or in USB-UART/kernel.

Re: [PATCH u-boot-marvell 00/13] Yet another kwboot improvements

[Stefan Roese]

On 27.10.21 23:03, Pali Rohár wrote:
> On Wednesday 27 October 2021 17:27:41 Stefan Roese wrote:
>> Still I see no speed change. But this is a different story...
> 
> Could you try to upload some file in u-boot via xmodem at different
> speeds? E.g. via loadx command:
> 
> loadx <addr> 921600
> 
> and
> 
> loadx <addr> 115200

loadx & loady are producing nearly the same numbers on my Armada XP
target for download (slow of cause). This is really strange.

I also tested with "normal" console actions (logging) with higher
baudrates. Here these numbers:

time md 2000000 10000:

115200 baud:
time: 1 minutes, 32.694 seconds

921600 baud:
time: 11.587 seconds

92.7s vs 11.6s -> factor ~8. Which is exactly the baudrate difference.
So at least here I see a speed difference, which means that the USB
UART converter must be working in general. I really have no idea right
now to explain, why this does not any speed differences while
downloading.

> And compare if xmodem transfer is in U-Boot faster or not. Ideally also
> try via kermit protocol at different speeds as kernel supports
> streaming.

I'm currently struggling to get gkermit working on my Ubuntu machine -
so no numbers here yet, sorry.

> This could verify if issue is in kwboot or in USB-UART/kernel.

Makes sense.

Thanks,
Stefan

Re: [PATCH u-boot-marvell 00/13] Yet another kwboot improvements

[Pali Rohár]

On Thursday 28 October 2021 08:16:24 Stefan Roese wrote:
> On 27.10.21 23:03, Pali Rohár wrote:
> > On Wednesday 27 October 2021 17:27:41 Stefan Roese wrote:
> > > Still I see no speed change. But this is a different story...
> > 
> > Could you try to upload some file in u-boot via xmodem at different
> > speeds? E.g. via loadx command:
> > 
> > loadx <addr> 921600
> > 
> > and
> > 
> > loadx <addr> 115200
> 
> loadx & loady are producing nearly the same numbers on my Armada XP
> target for download (slow of cause). This is really strange.

Ok. So it means that the issue should not be in kwboot, if also U-Boot
xmodem implementation is affected.

> I also tested with "normal" console actions (logging) with higher
> baudrates. Here these numbers:
> 
> time md 2000000 10000:
> 
> 115200 baud:
> time: 1 minutes, 32.694 seconds
> 
> 921600 baud:
> time: 11.587 seconds
> 
> 92.7s vs 11.6s -> factor ~8. Which is exactly the baudrate difference.
> So at least here I see a speed difference, which means that the USB
> UART converter must be working in general. I really have no idea right
> now to explain, why this does not any speed differences while
> downloading.

So this means that USB-UART converter must work also on higher speeds.

My idea is that there is some significant overhead on USB-UART when
accessing either RX or TX queue. xmodem protocol is: send 132 bytes,
wait for 1 byte reply, send another 132 bytes, wait again for 1 byte
reply, ... So if USB-UART has internal overhead which cause that every
byte is postponed by some delay then it would mean that xmodem is most
time just waiting, meaning overhead is applied for every one 132-byte
long packet. Test with md cause continuous stream of data, therefore
such overhead is applied only at beginning of the md transfer.

Similar issues with can observed also on high speed networks were is
very big latency when using protocols where each small packet has to be
acknowledged. TCP is using windowing to prevent such kind of issues.

And kermit protocol too via its streaming mode. So kermit transfer could
prove if this is really that issue.

Anyway, I have looked at your strace output again and I saw that there
is a long gap after tcdrain() and end of the read(). So if kernel
implements tcdrain() for your USB-UART hw it means that it take too much
time to receive reply byte from USB-UART hw to the kernel. So it could
be another indication of that overhead.

> > And compare if xmodem transfer is in U-Boot faster or not. Ideally also
> > try via kermit protocol at different speeds as kernel supports
> > streaming.
> 
> I'm currently struggling to get gkermit working on my Ubuntu machine -
> so no numbers here yet, sorry.

In U-Boot console call:

  loadb _addr_

And then quit terminal emulator used for U-Boot /dev/ttyUSBX device.
Then run gkermit with following arguments:

  gkermit -i -X -s _file_to_send_ < /dev/ttyUSBX > /dev/ttyUSBX

(redirecting both stdin and stdout is required)

After successful transfer gkermit exit. BEWARE that gkermit does not
print any progress information and neither any error. So do not kill
gkermit via CTRL+C prematurely. You can use "time gkermit ..." for
measuring total transfer time.

If you are going to use different speed than 115200 in u-boot run:

  loadb _addr_ _speed_

And then on host start gkermit with more commands around:

  stty -F /dev/ttyUSBX _speed_
  printf '\015' > /dev/ttyUSBX
  gkermit -i -X -s _file_to_send < /dev/ttyUSBX > /dev/ttyUSBX
  sleep 0.5
  stty -F /dev/ttyUSBX 115200
  printf '\033' > /dev/ttyUSBX

Due to absence of progress bar and error information I'm using ckermit
application for kermit file transfers. But for testing purposes is
gkermit application here also fine.

> > This could verify if issue is in kwboot or in USB-UART/kernel.
> 
> Makes sense.
> 
> Thanks,
> Stefan

Re: [PATCH u-boot-marvell 00/13] Yet another kwboot improvements

[Stefan Roese]

On 28.10.21 13:04, Pali Rohár wrote:
> On Thursday 28 October 2021 08:16:24 Stefan Roese wrote:
>> On 27.10.21 23:03, Pali Rohár wrote:
>>> On Wednesday 27 October 2021 17:27:41 Stefan Roese wrote:
>>>> Still I see no speed change. But this is a different story...
>>>
>>> Could you try to upload some file in u-boot via xmodem at different
>>> speeds? E.g. via loadx command:
>>>
>>> loadx <addr> 921600
>>>
>>> and
>>>
>>> loadx <addr> 115200
>>
>> loadx & loady are producing nearly the same numbers on my Armada XP
>> target for download (slow of cause). This is really strange.
> 
> Ok. So it means that the issue should not be in kwboot, if also U-Boot
> xmodem implementation is affected.

Agreed.

>> I also tested with "normal" console actions (logging) with higher
>> baudrates. Here these numbers:
>>
>> time md 2000000 10000:
>>
>> 115200 baud:
>> time: 1 minutes, 32.694 seconds
>>
>> 921600 baud:
>> time: 11.587 seconds
>>
>> 92.7s vs 11.6s -> factor ~8. Which is exactly the baudrate difference.
>> So at least here I see a speed difference, which means that the USB
>> UART converter must be working in general. I really have no idea right
>> now to explain, why this does not any speed differences while
>> downloading.
> 
> So this means that USB-UART converter must work also on higher speeds.
> 
> My idea is that there is some significant overhead on USB-UART when
> accessing either RX or TX queue. xmodem protocol is: send 132 bytes,
> wait for 1 byte reply, send another 132 bytes, wait again for 1 byte
> reply, ... So if USB-UART has internal overhead which cause that every
> byte is postponed by some delay then it would mean that xmodem is most
> time just waiting, meaning overhead is applied for every one 132-byte
> long packet. Test with md cause continuous stream of data, therefore
> such overhead is applied only at beginning of the md transfer.
> 
> Similar issues with can observed also on high speed networks were is
> very big latency when using protocols where each small packet has to be
> acknowledged. TCP is using windowing to prevent such kind of issues.
> 
> And kermit protocol too via its streaming mode. So kermit transfer could
> prove if this is really that issue.
> 
> Anyway, I have looked at your strace output again and I saw that there
> is a long gap after tcdrain() and end of the read(). So if kernel
> implements tcdrain() for your USB-UART hw it means that it take too much
> time to receive reply byte from USB-UART hw to the kernel. So it could
> be another indication of that overhead.

Thanks for the input here.

>>> And compare if xmodem transfer is in U-Boot faster or not. Ideally also
>>> try via kermit protocol at different speeds as kernel supports
>>> streaming.
>>
>> I'm currently struggling to get gkermit working on my Ubuntu machine -
>> so no numbers here yet, sorry.
> 
> In U-Boot console call:
> 
>    loadb _addr_
> 
> And then quit terminal emulator used for U-Boot /dev/ttyUSBX device.
> Then run gkermit with following arguments:
> 
>    gkermit -i -X -s _file_to_send_ < /dev/ttyUSBX > /dev/ttyUSBX
> 
> (redirecting both stdin and stdout is required)
> 
> After successful transfer gkermit exit. BEWARE that gkermit does not
> print any progress information and neither any error. So do not kill
> gkermit via CTRL+C prematurely. You can use "time gkermit ..." for
> measuring total transfer time.
> 
> If you are going to use different speed than 115200 in u-boot run:
> 
>    loadb _addr_ _speed_
> 
> And then on host start gkermit with more commands around:
> 
>    stty -F /dev/ttyUSBX _speed_
>    printf '\015' > /dev/ttyUSBX
>    gkermit -i -X -s _file_to_send < /dev/ttyUSBX > /dev/ttyUSBX
>    sleep 0.5
>    stty -F /dev/ttyUSBX 115200
>    printf '\033' > /dev/ttyUSBX
> 
> Due to absence of progress bar and error information I'm using ckermit
> application for kermit file transfers. But for testing purposes is
> gkermit application here also fine.

Ah, perfect. Here my number now using these steps and gkermit, with
a file size of 549373 bytes:

115200 baud: ~72 sec
921600 baud: ~11 sec

But:
I see this on my PC on the first use of a baudrate change:
$ stty -F /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0 
921600
stty: /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0: 
unable to perform all requested operations

The 2nd call succeeds. Not sure what this means. Perhaps this has
something to do with the speed problem, I'm seeing here on my AXP
target with the FTDI USB-UART controller.

Thanks,
Stefan

Re: [PATCH u-boot-marvell 00/13] Yet another kwboot improvements

[Pali Rohár]

On Thursday 28 October 2021 16:20:11 Stefan Roese wrote:
> On 28.10.21 13:04, Pali Rohár wrote:
> > On Thursday 28 October 2021 08:16:24 Stefan Roese wrote:
> > > On 27.10.21 23:03, Pali Rohár wrote:
> > > > On Wednesday 27 October 2021 17:27:41 Stefan Roese wrote:
> > > > > Still I see no speed change. But this is a different story...
> > > > 
> > > > Could you try to upload some file in u-boot via xmodem at different
> > > > speeds? E.g. via loadx command:
> > > > 
> > > > loadx <addr> 921600
> > > > 
> > > > and
> > > > 
> > > > loadx <addr> 115200
> > > 
> > > loadx & loady are producing nearly the same numbers on my Armada XP
> > > target for download (slow of cause). This is really strange.
> > 
> > Ok. So it means that the issue should not be in kwboot, if also U-Boot
> > xmodem implementation is affected.
> 
> Agreed.
> 
> > > I also tested with "normal" console actions (logging) with higher
> > > baudrates. Here these numbers:
> > > 
> > > time md 2000000 10000:
> > > 
> > > 115200 baud:
> > > time: 1 minutes, 32.694 seconds
> > > 
> > > 921600 baud:
> > > time: 11.587 seconds
> > > 
> > > 92.7s vs 11.6s -> factor ~8. Which is exactly the baudrate difference.
> > > So at least here I see a speed difference, which means that the USB
> > > UART converter must be working in general. I really have no idea right
> > > now to explain, why this does not any speed differences while
> > > downloading.
> > 
> > So this means that USB-UART converter must work also on higher speeds.
> > 
> > My idea is that there is some significant overhead on USB-UART when
> > accessing either RX or TX queue. xmodem protocol is: send 132 bytes,
> > wait for 1 byte reply, send another 132 bytes, wait again for 1 byte
> > reply, ... So if USB-UART has internal overhead which cause that every
> > byte is postponed by some delay then it would mean that xmodem is most
> > time just waiting, meaning overhead is applied for every one 132-byte
> > long packet. Test with md cause continuous stream of data, therefore
> > such overhead is applied only at beginning of the md transfer.
> > 
> > Similar issues with can observed also on high speed networks were is
> > very big latency when using protocols where each small packet has to be
> > acknowledged. TCP is using windowing to prevent such kind of issues.
> > 
> > And kermit protocol too via its streaming mode. So kermit transfer could
> > prove if this is really that issue.
> > 
> > Anyway, I have looked at your strace output again and I saw that there
> > is a long gap after tcdrain() and end of the read(). So if kernel
> > implements tcdrain() for your USB-UART hw it means that it take too much
> > time to receive reply byte from USB-UART hw to the kernel. So it could
> > be another indication of that overhead.
> 
> Thanks for the input here.
> 
> > > > And compare if xmodem transfer is in U-Boot faster or not. Ideally also
> > > > try via kermit protocol at different speeds as kernel supports
> > > > streaming.
> > > 
> > > I'm currently struggling to get gkermit working on my Ubuntu machine -
> > > so no numbers here yet, sorry.
> > 
> > In U-Boot console call:
> > 
> >    loadb _addr_
> > 
> > And then quit terminal emulator used for U-Boot /dev/ttyUSBX device.
> > Then run gkermit with following arguments:
> > 
> >    gkermit -i -X -s _file_to_send_ < /dev/ttyUSBX > /dev/ttyUSBX
> > 
> > (redirecting both stdin and stdout is required)
> > 
> > After successful transfer gkermit exit. BEWARE that gkermit does not
> > print any progress information and neither any error. So do not kill
> > gkermit via CTRL+C prematurely. You can use "time gkermit ..." for
> > measuring total transfer time.
> > 
> > If you are going to use different speed than 115200 in u-boot run:
> > 
> >    loadb _addr_ _speed_
> > 
> > And then on host start gkermit with more commands around:
> > 
> >    stty -F /dev/ttyUSBX _speed_
> >    printf '\015' > /dev/ttyUSBX
> >    gkermit -i -X -s _file_to_send < /dev/ttyUSBX > /dev/ttyUSBX
> >    sleep 0.5
> >    stty -F /dev/ttyUSBX 115200
> >    printf '\033' > /dev/ttyUSBX
> > 
> > Due to absence of progress bar and error information I'm using ckermit
> > application for kermit file transfers. But for testing purposes is
> > gkermit application here also fine.
> 
> Ah, perfect. Here my number now using these steps and gkermit, with
> a file size of 549373 bytes:
> 
> 115200 baud: ~72 sec
> 921600 baud: ~11 sec

So I think it is clear now. Either USB-UART HW or kernel driver for it
has some big overhead for any protocol which needs acknowledgement of
received data. So xmodem protocol is not suitable for it.

It would require to debug kernel driver and ideally monitor traffic on
oscilloscope to see where exactly is the issue.

And because bootrom supports only xmodem protocol we cannot do more in
kwboot.

For other cases it is a good idea to use kermit protocol instead of
xmodem.

> But:
> I see this on my PC on the first use of a baudrate change:
> $ stty -F /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
> 921600
> stty: /dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0: unable
> to perform all requested operations
> 
> The 2nd call succeeds. Not sure what this means. Perhaps this has
> something to do with the speed problem, I'm seeing here on my AXP
> target with the FTDI USB-UART controller.

Maybe this could indicate bugs in kernel driver.

> Thanks,
> Stefan

Re: [PATCH u-boot-marvell 00/13] Yet another kwboot improvements

[Stefan Roese]

On 28.10.21 19:00, Pali Rohár wrote:
> On Thursday 28 October 2021 16:20:11 Stefan Roese wrote:
>> On 28.10.21 13:04, Pali Rohár wrote:
>>> On Thursday 28 October 2021 08:16:24 Stefan Roese wrote:
>>>> On 27.10.21 23:03, Pali Rohár wrote:
>>>>> On Wednesday 27 October 2021 17:27:41 Stefan Roese wrote:
>>>>>> Still I see no speed change. But this is a different story...
>>>>>
>>>>> Could you try to upload some file in u-boot via xmodem at different
>>>>> speeds? E.g. via loadx command:
>>>>>
>>>>> loadx <addr> 921600
>>>>>
>>>>> and
>>>>>
>>>>> loadx <addr> 115200
>>>>
>>>> loadx & loady are producing nearly the same numbers on my Armada XP
>>>> target for download (slow of cause). This is really strange.
>>>
>>> Ok. So it means that the issue should not be in kwboot, if also U-Boot
>>> xmodem implementation is affected.
>>
>> Agreed.
>>
>>>> I also tested with "normal" console actions (logging) with higher
>>>> baudrates. Here these numbers:
>>>>
>>>> time md 2000000 10000:
>>>>
>>>> 115200 baud:
>>>> time: 1 minutes, 32.694 seconds
>>>>
>>>> 921600 baud:
>>>> time: 11.587 seconds
>>>>
>>>> 92.7s vs 11.6s -> factor ~8. Which is exactly the baudrate difference.
>>>> So at least here I see a speed difference, which means that the USB
>>>> UART converter must be working in general. I really have no idea right
>>>> now to explain, why this does not any speed differences while
>>>> downloading.
>>>
>>> So this means that USB-UART converter must work also on higher speeds.
>>>
>>> My idea is that there is some significant overhead on USB-UART when
>>> accessing either RX or TX queue. xmodem protocol is: send 132 bytes,
>>> wait for 1 byte reply, send another 132 bytes, wait again for 1 byte
>>> reply, ... So if USB-UART has internal overhead which cause that every
>>> byte is postponed by some delay then it would mean that xmodem is most
>>> time just waiting, meaning overhead is applied for every one 132-byte
>>> long packet. Test with md cause continuous stream of data, therefore
>>> such overhead is applied only at beginning of the md transfer.
>>>
>>> Similar issues with can observed also on high speed networks were is
>>> very big latency when using protocols where each small packet has to be
>>> acknowledged. TCP is using windowing to prevent such kind of issues.
>>>
>>> And kermit protocol too via its streaming mode. So kermit transfer could
>>> prove if this is really that issue.
>>>
>>> Anyway, I have looked at your strace output again and I saw that there
>>> is a long gap after tcdrain() and end of the read(). So if kernel
>>> implements tcdrain() for your USB-UART hw it means that it take too much
>>> time to receive reply byte from USB-UART hw to the kernel. So it could
>>> be another indication of that overhead.
>>
>> Thanks for the input here.
>>
>>>>> And compare if xmodem transfer is in U-Boot faster or not. Ideally also
>>>>> try via kermit protocol at different speeds as kernel supports
>>>>> streaming.
>>>>
>>>> I'm currently struggling to get gkermit working on my Ubuntu machine -
>>>> so no numbers here yet, sorry.
>>>
>>> In U-Boot console call:
>>>
>>>     loadb _addr_
>>>
>>> And then quit terminal emulator used for U-Boot /dev/ttyUSBX device.
>>> Then run gkermit with following arguments:
>>>
>>>     gkermit -i -X -s _file_to_send_ < /dev/ttyUSBX > /dev/ttyUSBX
>>>
>>> (redirecting both stdin and stdout is required)
>>>
>>> After successful transfer gkermit exit. BEWARE that gkermit does not
>>> print any progress information and neither any error. So do not kill
>>> gkermit via CTRL+C prematurely. You can use "time gkermit ..." for
>>> measuring total transfer time.
>>>
>>> If you are going to use different speed than 115200 in u-boot run:
>>>
>>>     loadb _addr_ _speed_
>>>
>>> And then on host start gkermit with more commands around:
>>>
>>>     stty -F /dev/ttyUSBX _speed_
>>>     printf '\015' > /dev/ttyUSBX
>>>     gkermit -i -X -s _file_to_send < /dev/ttyUSBX > /dev/ttyUSBX
>>>     sleep 0.5
>>>     stty -F /dev/ttyUSBX 115200
>>>     printf '\033' > /dev/ttyUSBX
>>>
>>> Due to absence of progress bar and error information I'm using ckermit
>>> application for kermit file transfers. But for testing purposes is
>>> gkermit application here also fine.
>>
>> Ah, perfect. Here my number now using these steps and gkermit, with
>> a file size of 549373 bytes:
>>
>> 115200 baud: ~72 sec
>> 921600 baud: ~11 sec
> 
> So I think it is clear now. Either USB-UART HW or kernel driver for it
> has some big overhead for any protocol which needs acknowledgement of
> received data. So xmodem protocol is not suitable for it.
> 
> It would require to debug kernel driver and ideally monitor traffic on
> oscilloscope to see where exactly is the issue.
> 
> And because bootrom supports only xmodem protocol we cannot do more in
> kwboot.

Agreed. Let's close this issue here. Important is, that kwboot still
works on all supported SoC's - even if the speedup improvements may
not be available for all boards.

Thanks for all your work here,
Stefan

Re: [PATCH u-boot-marvell 00/13] Yet another kwboot improvements

[Stefan Roese]

On 25.10.21 15:12, Marek Behún wrote:
> From: Marek Behún <marek.behun@nic.cz>
> 
> Hello Stefan,
> 
> these are another improvements for kwboot, please apply only after series
>    arm: mvebu: nandpagesize support for kwbimage v1
> 
> The main improvement is in patch 5, which changes where we inject the code
> for changing baudrate back to 115200 Baud after fast upload. Instead of
> injecting it before the main data image, we now inject it after.
> 
> This is because there are some kwb images that upload at address 0, and
> injecting the code before that doesn't work, since there is no RAM mapped
> at 0xfffff000.
> 
> Marek & Pali
> 
> Pali Rohár (13):
>    tools: kwboot: Initialize rfds to zero
>    tools: kwboot: Fix initialization of tty device
>    tools: kwboot: Reserve enough space for patching kwbimage in memory
>    tools: kwboot: Validate 4-byte image data checksum
>    tools: kwboot: Inject baudrate change back code after data part
>    tools: kwboot: Recalculate 4-byte data checksum after injecting
>      baudrate code
>    tools: kwboot: Correctly set configuration of UART for BootROM
>      messages
>    tools: kwboot: Show verbose message when waiting for baudrate change
>      magic
>    tools: kwboot: Simplify code for aligning image header
>    tools: kwboot: Do not modify kwbimage header before increasing its
>      size
>    tools: kwboot: Calculate real used space in kwbimage header when
>      calling kwboot_img_grow_hdr()
>    tools: kwboot: Change retry loop from decreasing to increasing
>    tools: kwboot: Resend first 3 xmodem retry packets immediately
> 
>   tools/kwboot.c | 178 +++++++++++++++++++++++++++++++++----------------
>   1 file changed, 120 insertions(+), 58 deletions(-)
> 

Applied to u-boot-marvell/master

Thanks,
Stefan