[PATCH v2 0/3] Support usb booting on i.MX8MP

[PATCH v2 0/3] Support usb booting on i.MX8MP

[Uwe Kleine-König]

Hello,

compared to (implicit) v1, the following changed here:

 - be a bit more conservative in "imx-usb-loader: Add support for
   i.MX8MP" regarding last_transfer; feedback by Sascha Hauer

 - Add support in PBL to be actually booted using imx-usb-loader (only
   tested the vendor U-Boot for v1), also skip the barebox header (i.e.
   the first (usually) 0x8000 bytes) before the IVT header on uploadeing

There are two things that resulted in discussions between Lucas, Ahmad
and me during development of this series:

 - On i.MX8MM and i.MX8MQ after the PBL is uploaded it's followed up by
   the complete image. So the PBL is uploaded twice. The motivation for
   this behaviour was that when booting from MMC loading from offset 0
   is easier because depending on the mode the hardware is in the offset
   has to be specified in different ways. So to make the two modes more
   similar the PBL is loaded twice for both modes.

   Here however after the PBL only the piggy data is sent because it
   feels wrong for me to spend some extra effort in imx-usb-loader to do
   a kludge for USB just because there are some difficulties with MMC.

   Arguments for this choice (apart from that already being completed
   and tested :-) are:

   - No duplication of data that is overwritten anyhow
   - Better interoperability with U-Boot/mfg-tools
     While this is usually not our focus I consider it quite annoying
     that there are at least three implementations of imx-usb-loader and
     depending on which machine and bootloader you use you have to pick
     the right implementation. With the longterm goal to have
     imx-usb-loader and barebox available in Debian some
     interoperability would be nice.

   I quickly tried using the boot rom load image support when booting
   from SD, but it didn't work out of the box and I didn't debug that.
   However I imagine that in this mode it would also be more natural to
   not expect the PBL twice.

   Switching to the duplicated operation mode would make imx-usb-loader
   a bit more complicated (because in the MXS code path the duplication
   isn't available yet), in return we'd save an offset calculation in
   the PBL.

   I should be possible to look at the first post-PBL data chunk coming
   in via USB and judge which mode of operation is used and behave
   accordingly. If this is considered a good idea I can create a patch
   for that.

 - When booting via USB the boot source is identified as "serial". I
   would expect that the semantic of that is rs232 and not USB. The
   source of this confusion is probably that the Reference Manual calls
   the USB  boot mode "USB Serial Download boot". I stuck to
   BOOTSOURCE_SERIAL here for consistency. If it's not only me who would
   consider using BOOTSOURCE_USB instead a fix this should be changed
   consistently for all i.MX platforms.
   Ahmad pointed out that this might break barebox shell scripts.

Uwe Kleine-König (3):
  imx8mp-evk: Add support for booting via USB
  imx-usb-loader: Drop nearly unused struct usb_id
  imx-usb-loader: Add support for i.MX8MP

 arch/arm/boards/nxp-imx8mp-evk/lowlevel.c |  27 +++++
 arch/arm/mach-imx/boot.c                  |   4 +-
 include/asm-generic/sections.h            |   1 +
 scripts/imx/imx-usb-loader.c              | 131 ++++++++++++----------
 4 files changed, 103 insertions(+), 60 deletions(-)


base-commit: 72424fd057d135ec0e41139fe4cb5740471d33a5
-- 
2.30.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 1/3] imx8mp-evk: Add support for booting via USB

[Uwe Kleine-König]

---
 arch/arm/boards/nxp-imx8mp-evk/lowlevel.c | 27 +++++++++++++++++++++++
 arch/arm/mach-imx/boot.c                  |  4 +++-
 include/asm-generic/sections.h            |  1 +
 3 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boards/nxp-imx8mp-evk/lowlevel.c b/arch/arm/boards/nxp-imx8mp-evk/lowlevel.c
index 3298ded5866d..1fb7899198d6 100644
--- a/arch/arm/boards/nxp-imx8mp-evk/lowlevel.c
+++ b/arch/arm/boards/nxp-imx8mp-evk/lowlevel.c
@@ -99,6 +99,30 @@ static int power_init_board(void)
 	return 0;
 }
 
+/* read piggydata via a bootrom callback and place it behind our copy in SDRAM */
+static int imx8m_bootrom_load_image(void)
+{
+	int (*download_image)(u8 *dest, u32 offset, u32 size, u32 xor) = *(void **)0x988;
+	size_t count = __piggydata_end - __piggydata_start;
+	char *p = (char *)MX8M_ATF_BL33_BASE_ADDR + (__piggydata_start - __image_start);
+
+	while (count) {
+		size_t chunksize = min(count, (size_t)1024);
+		int ret;
+
+		ret = download_image(p, 0, chunksize, (uintptr_t)p ^ chunksize);
+		if (ret != 0xf0) {
+			pr_err("Failed to load piggy data (ret = %x)\n", ret);
+			return -EIO;
+		}
+
+		p += chunksize;
+		count -= chunksize;
+	}
+
+	return 0;
+}
+
 extern struct dram_timing_info imx8mp_evk_dram_timing;
 
 static void start_atf(void)
@@ -125,6 +149,9 @@ static void start_atf(void)
 	case BOOTSOURCE_MMC:
 		imx8mp_esdhc_load_image(instance, false);
 		break;
+	case BOOTSOURCE_SERIAL:
+		imx8m_bootrom_load_image();
+		break;
 	default:
 		printf("Unhandled bootsource BOOTSOURCE_%d\n", src);
 		hang();
diff --git a/arch/arm/mach-imx/boot.c b/arch/arm/mach-imx/boot.c
index 2b66bbf71eb1..7c1d49291045 100644
--- a/arch/arm/mach-imx/boot.c
+++ b/arch/arm/mach-imx/boot.c
@@ -495,10 +495,12 @@ static void __imx7_get_boot_source(enum bootsource *src, int *instance,
 	case 5:
 		*src = BOOTSOURCE_NOR;
 		break;
-	case 15:
+	case 14: /* observed on i.MX8MP for USB "serial" booting */
+	case 15: /* observed on i.MX8MM for USB "serial" booting */
 		*src = BOOTSOURCE_SERIAL;
 		break;
 	default:
+		*src = BOOTSOURCE_UNKNOWN;
 		break;
 	}
 }
diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
index 870bff21f668..597c4951ea5e 100644
--- a/include/asm-generic/sections.h
+++ b/include/asm-generic/sections.h
@@ -9,6 +9,7 @@ extern char _end[];
 extern char __image_start[];
 extern char __image_end[];
 extern char __piggydata_start[];
+extern char __piggydata_end[];
 extern void *_barebox_image_size;
 extern void *_barebox_bare_init_size;
 extern void *_barebox_pbl_size;
-- 
2.30.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 2/3] imx-usb-loader: Drop nearly unused struct usb_id

[Uwe Kleine-König]

Only one of the two members of struct usb_id is actually used. So
replace struct usb_id by a struct mach_id.

Signed-off-by: Uwe Kleine-Koenig <u.kleine-koenig@pengutronix.de>
---
 scripts/imx/imx-usb-loader.c | 51 ++++++++++++------------------------
 1 file changed, 17 insertions(+), 34 deletions(-)

diff --git a/scripts/imx/imx-usb-loader.c b/scripts/imx/imx-usb-loader.c
index cff77f27f264..3e96c86f2f29 100644
--- a/scripts/imx/imx-usb-loader.c
+++ b/scripts/imx/imx-usb-loader.c
@@ -52,7 +52,7 @@
 
 int verbose;
 static struct libusb_device_handle *usb_dev_handle;
-static struct usb_id *usb_id;
+static const struct mach_id *mach_id;
 
 struct mach_id {
 	struct mach_id * next;
@@ -79,11 +79,6 @@ struct usb_work {
 	unsigned char plug;
 };
 
-struct usb_id {
-	const struct mach_id *mach_id;
-	struct usb_work *work;
-};
-
 static const struct mach_id imx_ids[] = {
 	{
 		.vid = 0x066f,
@@ -507,8 +502,8 @@ static int read_file(const char *name, unsigned char **buffer, unsigned *size)
 static int transfer(int report, unsigned char *p, unsigned cnt, int *last_trans)
 {
 	int err;
-	if (cnt > usb_id->mach_id->max_transfer)
-		cnt = usb_id->mach_id->max_transfer;
+	if (cnt > mach_id->max_transfer)
+		cnt = mach_id->max_transfer;
 
 	if (verbose > 4) {
 		printf("report=%i\n", report);
@@ -516,7 +511,7 @@ static int transfer(int report, unsigned char *p, unsigned cnt, int *last_trans)
 			dump_bytes(p, cnt, 0);
 	}
 
-	if (usb_id->mach_id->mode == MODE_BULK) {
+	if (mach_id->mode == MODE_BULK) {
 		*last_trans = 0;
 		err = libusb_bulk_transfer(usb_dev_handle,
 					   (report < 3) ? 1 : 2 + EP_IN, p, cnt, last_trans, 1000);
@@ -602,7 +597,7 @@ static int do_status(void)
 			break;
 	}
 
-	if (usb_id->mach_id->mode == MODE_HID) {
+	if (mach_id->mode == MODE_HID) {
 		err = transfer(4, tmp, sizeof(tmp), &last_trans);
 		if (err)
 			printf("4 in err=%i, last_trans=%i  %02x %02x %02x %02x\n",
@@ -810,7 +805,7 @@ static int load_file(void *buf, unsigned len, unsigned dladdr,
 
 	retry = 0;
 
-	if (usb_id->mach_id->mode == MODE_BULK) {
+	if (mach_id->mode == MODE_BULK) {
 		err = transfer(3, tmp, sizeof(tmp), &last_trans);
 		if (err)
 			printf("in err=%i, last_trans=%i  %02x %02x %02x %02x\n",
@@ -821,7 +816,7 @@ static int load_file(void *buf, unsigned len, unsigned dladdr,
 	cnt = len;
 
 	while (1) {
-		int now = get_min(cnt, usb_id->mach_id->max_transfer);
+		int now = get_min(cnt, mach_id->max_transfer);
 
 		if (!now)
 			break;
@@ -839,7 +834,7 @@ static int load_file(void *buf, unsigned len, unsigned dladdr,
 	if (mode_barebox)
 		return transfer_size;
 
-	if (usb_id->mach_id->mode == MODE_HID) {
+	if (mach_id->mode == MODE_HID) {
 		err = transfer(3, tmp, sizeof(tmp), &last_trans);
 		if (err)
 			printf("3 in err=%i, last_trans=%i  %02x %02x %02x %02x\n",
@@ -1233,7 +1228,7 @@ static int is_header(const unsigned char *p)
 	const struct imx_flash_header_v2 *hdr =
 		(const struct imx_flash_header_v2 *)p;
 
-	switch (usb_id->mach_id->header_type) {
+	switch (mach_id->header_type) {
 	case HDR_MX51:
 		if (ohdr->app_code_barker == 0xb1)
 			return 1;
@@ -1253,7 +1248,7 @@ static int perform_dcd(unsigned char *p, const unsigned char *file_start,
 	struct imx_flash_header_v2 *hdr = (struct imx_flash_header_v2 *)p;
 	int ret = 0;
 
-	switch (usb_id->mach_id->header_type) {
+	switch (mach_id->header_type) {
 	case HDR_MX51:
 		ret = write_dcd_table_old(ohdr, file_start, cnt);
 		ohdr->dcd_block_len = 0;
@@ -1274,7 +1269,7 @@ static int get_dl_start(const unsigned char *p, const unsigned char *file_start,
 		unsigned *header_addr)
 {
 	const unsigned char *file_end = file_start + cnt;
-	switch (usb_id->mach_id->header_type) {
+	switch (mach_id->header_type) {
 	case HDR_MX51:
 	{
 		struct imx_flash_header *ohdr = (struct imx_flash_header *)p;
@@ -1315,7 +1310,7 @@ static int get_payload_start(const unsigned char *p, uint32_t *ofs)
 {
 	struct imx_flash_header_v2 *hdr = (struct imx_flash_header_v2 *)p;
 
-	switch (usb_id->mach_id->header_type) {
+	switch (mach_id->header_type) {
 	case HDR_MX51:
 		return -EINVAL;
 
@@ -1430,7 +1425,7 @@ static int do_irom_download(struct usb_work *curr, int verify)
 
 		memcpy(verify_buffer, image, 64);
 
-		if ((type == FT_APP) && (usb_id->mach_id->mode != MODE_HID)) {
+		if ((type == FT_APP) && (mach_id->mode != MODE_HID)) {
 			type = FT_LOAD_ONLY;
 			verify = 2;
 		}
@@ -1469,7 +1464,7 @@ static int do_irom_download(struct usb_work *curr, int verify)
 		}
 	}
 
-	if (usb_id->mach_id->mode == MODE_HID && type == FT_APP) {
+	if (mach_id->mode == MODE_HID && type == FT_APP) {
 		printf("jumping to 0x%08x\n", header_addr);
 
 		ret = sdp_jump_address(header_addr);
@@ -1532,7 +1527,7 @@ static int mxs_load_file(libusb_device_handle *dev, uint8_t *data, int size)
 	cnt = size;
 
 	while (1) {
-		int now = get_min(cnt, usb_id->mach_id->max_transfer);
+		int now = get_min(cnt, mach_id->max_transfer);
 
 		if (!now)
 			break;
@@ -1591,7 +1586,6 @@ static void usage(const char *prgname)
 
 int main(int argc, char *argv[])
 {
-	const struct mach_id *mach;
 	libusb_device **devs;
 	libusb_device *dev;
 	int r;
@@ -1663,7 +1657,7 @@ int main(int argc, char *argv[])
 		goto out;
 	}
 
-	dev = find_imx_dev(devs, &mach, devpath, devtype);
+	dev = find_imx_dev(devs, &mach_id, devpath, devtype);
 	if (!dev) {
 		fprintf(stderr, "no supported device found\n");
 		goto out;
@@ -1682,15 +1676,7 @@ int main(int argc, char *argv[])
 		goto out;
 	}
 
-	usb_id = malloc(sizeof(*usb_id));
-	if (!usb_id) {
-		perror("malloc");
-		exit(1);
-	}
-
-	usb_id->mach_id = mach;
-
-	if (mach->dev_type == DEV_MXS) {
+	if (mach_id->dev_type == DEV_MXS) {
 		ret = mxs_work(&w);
 		goto out;
 	}
@@ -1715,9 +1701,6 @@ int main(int argc, char *argv[])
 
 	ret = 0;
 out:
-	if (usb_id)
-		free(usb_id);
-
 	if (usb_dev_handle)
 		libusb_close(usb_dev_handle);
 
-- 
2.30.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 3/3] imx-usb-loader: Add support for i.MX8MP

[Uwe Kleine-König]

The i.MX8MP uses a protocol similar to the MXS. The relevant differences
are:

 - Maximal transfer size is 1020
 - HID reports must be sent to EP1 instead of using a control transfer
 - The FW_DOWNLOAD command must not be send.
 - The image to upload must start with the IVT header (usually at offset
   0x8000), so the barebox header isn't transferred.

Signed-off-by: Uwe Kleine-Koenig <u.kleine-koenig@pengutronix.de>
---
 scripts/imx/imx-usb-loader.c | 80 +++++++++++++++++++++++++-----------
 1 file changed, 55 insertions(+), 25 deletions(-)

diff --git a/scripts/imx/imx-usb-loader.c b/scripts/imx/imx-usb-loader.c
index 3e96c86f2f29..cf87e0f91d65 100644
--- a/scripts/imx/imx-usb-loader.c
+++ b/scripts/imx/imx-usb-loader.c
@@ -71,6 +71,7 @@ struct mach_id {
 #define DEV_IMX		0
 #define DEV_MXS		1
 	unsigned char dev_type;
+	unsigned char hid_endpoint;
 };
 
 struct usb_work {
@@ -177,6 +178,14 @@ static const struct mach_id imx_ids[] = {
 		.header_type = HDR_MX53,
 		.mode = MODE_HID,
 		.max_transfer = 1024,
+	}, {
+		.vid = 0x1fc9,
+		.pid = 0x0146,
+		.name = "i.MX8MP",
+		.header_type = HDR_MX53,
+		.max_transfer = 1020,
+		.dev_type = DEV_MXS,
+		.hid_endpoint = 1,
 	}, {
 		.vid = 0x1fc9,
 		.pid = 0x012b,
@@ -522,15 +531,22 @@ static int transfer(int report, unsigned char *p, unsigned cnt, int *last_trans)
 
 		if (report < 3) {
 			memcpy(&tmp[1], p, cnt);
-			err = libusb_control_transfer(usb_dev_handle,
-					CTRL_OUT,
-					HID_SET_REPORT,
-					(HID_REPORT_TYPE_OUTPUT << 8) | report,
-					0,
-					tmp, cnt + 1, 1000);
-			*last_trans = (err > 0) ? err - 1 : 0;
-			if (err > 0)
-				err = 0;
+			if (mach_id->hid_endpoint) {
+				int trans;
+				err = libusb_interrupt_transfer(usb_dev_handle,
+						mach_id->hid_endpoint, tmp, cnt + 1, &trans, 1000);
+				*last_trans = trans - 1;
+			} else {
+				err = libusb_control_transfer(usb_dev_handle,
+						CTRL_OUT,
+						HID_SET_REPORT,
+						(HID_REPORT_TYPE_OUTPUT << 8) | report,
+						0,
+						tmp, cnt + 1, 1000);
+				*last_trans = (err > 0) ? err - 1 : 0;
+				if (err > 0)
+					err = 0;
+			}
 		} else {
 			*last_trans = 0;
 			memset(&tmp[1], 0, cnt);
@@ -1500,32 +1516,46 @@ static int write_mem(const struct config_data *data, uint32_t addr,
 }
 
 /* MXS section */
-static int mxs_load_file(libusb_device_handle *dev, uint8_t *data, int size)
+static int mxs_load_file(struct usb_work *curr, libusb_device_handle *dev, uint8_t *data, int size)
 {
 	static struct mxs_command dl_command;
 	int last_trans, err;
 	void *p;
 	int cnt;
 
-	dl_command.sign = htonl(0x424c5443); /* Signature: BLTC */
-	dl_command.tag = htonl(0x1);
-	dl_command.size = htonl(size);
-	dl_command.flags = 0;
-	dl_command.rsvd[0] = 0;
-	dl_command.rsvd[1] = 0;
-	dl_command.cmd = MXS_CMD_FW_DOWNLOAD;
-	dl_command.dw_size = htonl(size);
-
-	err = transfer(1, (unsigned char *) &dl_command, 20, &last_trans);
-	if (err) {
-		printf("transfer error at init step: err=%i, last_trans=%i\n",
-		       err, last_trans);
-		return err;
+	if (!mach_id->hid_endpoint) {
+		dl_command.sign = htonl(0x424c5443); /* Signature: BLTC */
+		dl_command.tag = htonl(0x1);
+		dl_command.size = htonl(size);
+		dl_command.flags = 0;
+		dl_command.rsvd[0] = 0;
+		dl_command.rsvd[1] = 0;
+		dl_command.cmd = MXS_CMD_FW_DOWNLOAD;
+		dl_command.dw_size = htonl(size);
+
+		err = transfer(1, (unsigned char *) &dl_command, 20, &last_trans);
+		if (err) {
+			printf("transfer error at init step: err=%i, last_trans=%i\n",
+					err, last_trans);
+			return err;
+		}
 	}
 
 	p = data;
 	cnt = size;
 
+	if (mach_id->header_type != HDR_NONE) {
+		unsigned int dummy;
+		err = process_header(curr, p, cnt, &dummy, &dummy, &dummy);
+		if (err < 0) {
+			printf("Failed to find IVT header\n");
+			return err;
+		}
+
+		p += err;
+		cnt -= err;
+	}
+
 	while (1) {
 		int now = get_min(cnt, mach_id->max_transfer);
 
@@ -1555,7 +1585,7 @@ static int mxs_work(struct usb_work *curr)
 	if (ret < 0)
 		return ret;
 
-	return mxs_load_file(usb_dev_handle, buf, fsize);
+	return mxs_load_file(curr, usb_dev_handle, buf, fsize);
 }
 /* end of mxs section */
 
-- 
2.30.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 1/3] imx8mp-evk: Add support for booting via USB

[Ahmad Fatoum]

On 13.08.21 17:22, Uwe Kleine-König wrote:

S-o-b missing.

> ---
>  arch/arm/boards/nxp-imx8mp-evk/lowlevel.c | 27 +++++++++++++++++++++++
>  arch/arm/mach-imx/boot.c                  |  4 +++-
>  include/asm-generic/sections.h            |  1 +
>  3 files changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boards/nxp-imx8mp-evk/lowlevel.c b/arch/arm/boards/nxp-imx8mp-evk/lowlevel.c
> index 3298ded5866d..1fb7899198d6 100644
> --- a/arch/arm/boards/nxp-imx8mp-evk/lowlevel.c
> +++ b/arch/arm/boards/nxp-imx8mp-evk/lowlevel.c
> @@ -99,6 +99,30 @@ static int power_init_board(void)
>  	return 0;
>  }
>  
> +/* read piggydata via a bootrom callback and place it behind our copy in SDRAM */
> +static int imx8m_bootrom_load_image(void)
> +{
> +	int (*download_image)(u8 *dest, u32 offset, u32 size, u32 xor) = *(void **)0x988;

Would be nice to have this in a header, e.g.

extern struct imx8mp_bootrom_ops {
	/* ... */
	int (*download_image)(u8 *dest, u32 offset, u32 size, u32 xor);
} *imx8mp_bootrom_ops = (void *)0x988;

> +	size_t count = __piggydata_end - __piggydata_start;
> +	char *p = (char *)MX8M_ATF_BL33_BASE_ADDR + (__piggydata_start - __image_start);
> +
> +	while (count) {
> +		size_t chunksize = min(count, (size_t)1024);
> +		int ret;
> +
> +		ret = download_image(p, 0, chunksize, (uintptr_t)p ^ chunksize);
> +		if (ret != 0xf0) {
> +			pr_err("Failed to load piggy data (ret = %x)\n", ret);
> +			return -EIO;
> +		}
> +
> +		p += chunksize;
> +		count -= chunksize;
> +	}
> +
> +	return 0;
> +}
> +
>  extern struct dram_timing_info imx8mp_evk_dram_timing;
>  
>  static void start_atf(void)
> @@ -125,6 +149,9 @@ static void start_atf(void)
>  	case BOOTSOURCE_MMC:
>  		imx8mp_esdhc_load_image(instance, false);
>  		break;
> +	case BOOTSOURCE_SERIAL:
> +		imx8m_bootrom_load_image();
> +		break;
>  	default:
>  		printf("Unhandled bootsource BOOTSOURCE_%d\n", src);
>  		hang();
> diff --git a/arch/arm/mach-imx/boot.c b/arch/arm/mach-imx/boot.c
> index 2b66bbf71eb1..7c1d49291045 100644
> --- a/arch/arm/mach-imx/boot.c
> +++ b/arch/arm/mach-imx/boot.c
> @@ -495,10 +495,12 @@ static void __imx7_get_boot_source(enum bootsource *src, int *instance,
>  	case 5:
>  		*src = BOOTSOURCE_NOR;
>  		break;
> -	case 15:
> +	case 14: /* observed on i.MX8MP for USB "serial" booting */
> +	case 15: /* observed on i.MX8MM for USB "serial" booting */
>  		*src = BOOTSOURCE_SERIAL;
>  		break;
>  	default:
> +		*src = BOOTSOURCE_UNKNOWN;
>  		break;
>  	}
>  }
> diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
> index 870bff21f668..597c4951ea5e 100644
> --- a/include/asm-generic/sections.h
> +++ b/include/asm-generic/sections.h
> @@ -9,6 +9,7 @@ extern char _end[];
>  extern char __image_start[];
>  extern char __image_end[];
>  extern char __piggydata_start[];
> +extern char __piggydata_end[];

Other code normally uses __image_end, but it seems __piggydata_end should be
equal to it. I'd prefer __image_end, because conceptually, you want to copy
off the rest of the image (which happens to be just the piggy data)

@Rouven, The sha sum is at the end of the image, right?
Would this be included this way?

>  extern void *_barebox_image_size;
>  extern void *_barebox_bare_init_size;
>  extern void *_barebox_pbl_size;
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 1/3] imx8mp-evk: Add support for booting via USB

[Lucas Stach]

Am Freitag, dem 13.08.2021 um 21:26 +0200 schrieb Ahmad Fatoum:
> On 13.08.21 17:22, Uwe Kleine-König wrote:
> 
> S-o-b missing.
> 
> > ---
> >  arch/arm/boards/nxp-imx8mp-evk/lowlevel.c | 27 +++++++++++++++++++++++
> >  arch/arm/mach-imx/boot.c                  |  4 +++-
> >  include/asm-generic/sections.h            |  1 +
> >  3 files changed, 31 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm/boards/nxp-imx8mp-evk/lowlevel.c b/arch/arm/boards/nxp-imx8mp-evk/lowlevel.c
> > index 3298ded5866d..1fb7899198d6 100644
> > --- a/arch/arm/boards/nxp-imx8mp-evk/lowlevel.c
> > +++ b/arch/arm/boards/nxp-imx8mp-evk/lowlevel.c
> > @@ -99,6 +99,30 @@ static int power_init_board(void)
> >  	return 0;
> >  }
> >  
> > +/* read piggydata via a bootrom callback and place it behind our copy in SDRAM */
> > +static int imx8m_bootrom_load_image(void)
> > +{
> > +	int (*download_image)(u8 *dest, u32 offset, u32 size, u32 xor) = *(void **)0x988;
> 
> Would be nice to have this in a header, e.g.
> 
> extern struct imx8mp_bootrom_ops {
> 	/* ... */
> 	int (*download_image)(u8 *dest, u32 offset, u32 size, u32 xor);
> } *imx8mp_bootrom_ops = (void *)0x988;
> 
> > +	size_t count = __piggydata_end - __piggydata_start;
> > +	char *p = (char *)MX8M_ATF_BL33_BASE_ADDR + (__piggydata_start - __image_start);
> > +
> > +	while (count) {
> > +		size_t chunksize = min(count, (size_t)1024);
> > +		int ret;
> > +
> > +		ret = download_image(p, 0, chunksize, (uintptr_t)p ^ chunksize);
> > +		if (ret != 0xf0) {
> > +			pr_err("Failed to load piggy data (ret = %x)\n", ret);
> > +			return -EIO;
> > +		}
> > +
> > +		p += chunksize;
> > +		count -= chunksize;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
This shouldn't be in board code, but in some location where it is
reusable by other i.MX8MP boards.

> >  extern struct dram_timing_info imx8mp_evk_dram_timing;
> >  
> >  static void start_atf(void)
> > @@ -125,6 +149,9 @@ static void start_atf(void)
> >  	case BOOTSOURCE_MMC:
> >  		imx8mp_esdhc_load_image(instance, false);
> >  		break;
> > +	case BOOTSOURCE_SERIAL:
> > +		imx8m_bootrom_load_image();
> > +		break;
> >  	default:
> >  		printf("Unhandled bootsource BOOTSOURCE_%d\n", src);
> >  		hang();
> > diff --git a/arch/arm/mach-imx/boot.c b/arch/arm/mach-imx/boot.c
> > index 2b66bbf71eb1..7c1d49291045 100644
> > --- a/arch/arm/mach-imx/boot.c
> > +++ b/arch/arm/mach-imx/boot.c
> > @@ -495,10 +495,12 @@ static void __imx7_get_boot_source(enum bootsource *src, int *instance,
> >  	case 5:
> >  		*src = BOOTSOURCE_NOR;
> >  		break;
> > -	case 15:
> > +	case 14: /* observed on i.MX8MP for USB "serial" booting */
> > +	case 15: /* observed on i.MX8MM for USB "serial" booting */
> >  		*src = BOOTSOURCE_SERIAL;
> >  		break;
> >  	default:
> > +		*src = BOOTSOURCE_UNKNOWN;
> >  		break;
> >  	}
> >  }
> > diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
> > index 870bff21f668..597c4951ea5e 100644
> > --- a/include/asm-generic/sections.h
> > +++ b/include/asm-generic/sections.h
> > @@ -9,6 +9,7 @@ extern char _end[];
> >  extern char __image_start[];
> >  extern char __image_end[];
> >  extern char __piggydata_start[];
> > +extern char __piggydata_end[];
> 
> Other code normally uses __image_end, but it seems __piggydata_end should be
> equal to it. I'd prefer __image_end, because conceptually, you want to copy
> off the rest of the image (which happens to be just the piggy data)
> 
> @Rouven, The sha sum is at the end of the image, right?
> Would this be included this way?

The checksum is built into the PBL, as it is used to authenticate the
piggydata, as the BootROM will only authenticate the initial loaded
part of the image, i.e. the PBL.

Regards,
Lucas


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 0/3] Support usb booting on i.MX8MP

[Lucas Stach]

Hi Uwe,

Am Freitag, dem 13.08.2021 um 17:22 +0200 schrieb Uwe Kleine-König:
> Hello,
> 
> compared to (implicit) v1, the following changed here:
> 
>  - be a bit more conservative in "imx-usb-loader: Add support for
>    i.MX8MP" regarding last_transfer; feedback by Sascha Hauer
> 
>  - Add support in PBL to be actually booted using imx-usb-loader (only
>    tested the vendor U-Boot for v1), also skip the barebox header (i.e.
>    the first (usually) 0x8000 bytes) before the IVT header on uploadeing
> 
> There are two things that resulted in discussions between Lucas, Ahmad
> and me during development of this series:
> 
>  - On i.MX8MM and i.MX8MQ after the PBL is uploaded it's followed up by
>    the complete image. So the PBL is uploaded twice. The motivation for
>    this behaviour was that when booting from MMC loading from offset 0
>    is easier because depending on the mode the hardware is in the offset
>    has to be specified in different ways. So to make the two modes more
>    similar the PBL is loaded twice for both modes.
> 
>    Here however after the PBL only the piggy data is sent because it
>    feels wrong for me to spend some extra effort in imx-usb-loader to do
>    a kludge for USB just because there are some difficulties with MMC.
> 
>    Arguments for this choice (apart from that already being completed
>    and tested :-) are:
> 
>    - No duplication of data that is overwritten anyhow
>    - Better interoperability with U-Boot/mfg-tools
>      While this is usually not our focus I consider it quite annoying
>      that there are at least three implementations of imx-usb-loader and
>      depending on which machine and bootloader you use you have to pick
>      the right implementation. With the longterm goal to have
>      imx-usb-loader and barebox available in Debian some
>      interoperability would be nice.
> 
>    I quickly tried using the boot rom load image support when booting
>    from SD, but it didn't work out of the box and I didn't debug that.
>    However I imagine that in this mode it would also be more natural to
>    not expect the PBL twice.
> 
>    Switching to the duplicated operation mode would make imx-usb-loader
>    a bit more complicated (because in the MXS code path the duplication
>    isn't available yet), in return we'd save an offset calculation in
>    the PBL.

I can follow those arguments, but for the sake of the sanity of
everyone involved I would vote for changing the i.MX8MM USB loading to
work in the same way, maybe even using the ROM API if it works there
too. i.MX8MM isn't too widely used yet, so I guess we can still change
it without inflicting too much incompatibility pain.

The i.MX8M* Barebox lowlevel flow is already quite delicate, so I would
rather avoid having to deal with subtle differences between the various
family members.

> 
>    I should be possible to look at the first post-PBL data chunk coming
>    in via USB and judge which mode of operation is used and behave
>    accordingly. If this is considered a good idea I can create a patch
>    for that.
> 
>  - When booting via USB the boot source is identified as "serial". I
>    would expect that the semantic of that is rs232 and not USB. The
>    source of this confusion is probably that the Reference Manual calls
>    the USB  boot mode "USB Serial Download boot". I stuck to
>    BOOTSOURCE_SERIAL here for consistency. If it's not only me who would
>    consider using BOOTSOURCE_USB instead a fix this should be changed
>    consistently for all i.MX platforms.
>    Ahmad pointed out that this might break barebox shell scripts.

This however sounds like breaking backwards compat for no real gain. As
even the reference manual calls this boot mode "serial" I always found
this quite clear. My vote is on keeping things as they are.

Regards,
Lucas

> 
> Uwe Kleine-König (3):
>   imx8mp-evk: Add support for booting via USB
>   imx-usb-loader: Drop nearly unused struct usb_id
>   imx-usb-loader: Add support for i.MX8MP
> 
>  arch/arm/boards/nxp-imx8mp-evk/lowlevel.c |  27 +++++
>  arch/arm/mach-imx/boot.c                  |   4 +-
>  include/asm-generic/sections.h            |   1 +
>  scripts/imx/imx-usb-loader.c              | 131 ++++++++++++----------
>  4 files changed, 103 insertions(+), 60 deletions(-)
> 
> 
> base-commit: 72424fd057d135ec0e41139fe4cb5740471d33a5



_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 1/3] imx8mp-evk: Add support for booting via USB

[Ahmad Fatoum]

On 13.08.21 22:20, Lucas Stach wrote:
> Am Freitag, dem 13.08.2021 um 21:26 +0200 schrieb Ahmad Fatoum:
>> On 13.08.21 17:22, Uwe Kleine-König wrote:
>>
>> S-o-b missing.
>>
>>> ---
>>>  arch/arm/boards/nxp-imx8mp-evk/lowlevel.c | 27 +++++++++++++++++++++++
>>>  arch/arm/mach-imx/boot.c                  |  4 +++-
>>>  include/asm-generic/sections.h            |  1 +
>>>  3 files changed, 31 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/boards/nxp-imx8mp-evk/lowlevel.c b/arch/arm/boards/nxp-imx8mp-evk/lowlevel.c
>>> index 3298ded5866d..1fb7899198d6 100644
>>> --- a/arch/arm/boards/nxp-imx8mp-evk/lowlevel.c
>>> +++ b/arch/arm/boards/nxp-imx8mp-evk/lowlevel.c
>>> @@ -99,6 +99,30 @@ static int power_init_board(void)
>>>  	return 0;
>>>  }
>>>  
>>> +/* read piggydata via a bootrom callback and place it behind our copy in SDRAM */
>>> +static int imx8m_bootrom_load_image(void)
>>> +{
>>> +	int (*download_image)(u8 *dest, u32 offset, u32 size, u32 xor) = *(void **)0x988;
>>
>> Would be nice to have this in a header, e.g.
>>
>> extern struct imx8mp_bootrom_ops {
>> 	/* ... */
>> 	int (*download_image)(u8 *dest, u32 offset, u32 size, u32 xor);
>> } *imx8mp_bootrom_ops = (void *)0x988;
>>
>>> +	size_t count = __piggydata_end - __piggydata_start;
>>> +	char *p = (char *)MX8M_ATF_BL33_BASE_ADDR + (__piggydata_start - __image_start);
>>> +
>>> +	while (count) {
>>> +		size_t chunksize = min(count, (size_t)1024);
>>> +		int ret;
>>> +
>>> +		ret = download_image(p, 0, chunksize, (uintptr_t)p ^ chunksize);
>>> +		if (ret != 0xf0) {
>>> +			pr_err("Failed to load piggy data (ret = %x)\n", ret);
>>> +			return -EIO;
>>> +		}
>>> +
>>> +		p += chunksize;
>>> +		count -= chunksize;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
> This shouldn't be in board code, but in some location where it is
> reusable by other i.MX8MP boards.
> 
>>>  extern struct dram_timing_info imx8mp_evk_dram_timing;
>>>  
>>>  static void start_atf(void)
>>> @@ -125,6 +149,9 @@ static void start_atf(void)
>>>  	case BOOTSOURCE_MMC:
>>>  		imx8mp_esdhc_load_image(instance, false);
>>>  		break;
>>> +	case BOOTSOURCE_SERIAL:
>>> +		imx8m_bootrom_load_image();
>>> +		break;
>>>  	default:
>>>  		printf("Unhandled bootsource BOOTSOURCE_%d\n", src);
>>>  		hang();
>>> diff --git a/arch/arm/mach-imx/boot.c b/arch/arm/mach-imx/boot.c
>>> index 2b66bbf71eb1..7c1d49291045 100644
>>> --- a/arch/arm/mach-imx/boot.c
>>> +++ b/arch/arm/mach-imx/boot.c
>>> @@ -495,10 +495,12 @@ static void __imx7_get_boot_source(enum bootsource *src, int *instance,
>>>  	case 5:
>>>  		*src = BOOTSOURCE_NOR;
>>>  		break;
>>> -	case 15:
>>> +	case 14: /* observed on i.MX8MP for USB "serial" booting */
>>> +	case 15: /* observed on i.MX8MM for USB "serial" booting */
>>>  		*src = BOOTSOURCE_SERIAL;
>>>  		break;
>>>  	default:
>>> +		*src = BOOTSOURCE_UNKNOWN;
>>>  		break;
>>>  	}
>>>  }
>>> diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
>>> index 870bff21f668..597c4951ea5e 100644
>>> --- a/include/asm-generic/sections.h
>>> +++ b/include/asm-generic/sections.h
>>> @@ -9,6 +9,7 @@ extern char _end[];
>>>  extern char __image_start[];
>>>  extern char __image_end[];
>>>  extern char __piggydata_start[];
>>> +extern char __piggydata_end[];
>>
>> Other code normally uses __image_end, but it seems __piggydata_end should be
>> equal to it. I'd prefer __image_end, because conceptually, you want to copy
>> off the rest of the image (which happens to be just the piggy data)
>>
>> @Rouven, The sha sum is at the end of the image, right?
>> Would this be included this way?
> 
> The checksum is built into the PBL, as it is used to authenticate the
> piggydata, as the BootROM will only authenticate the initial loaded
> part of the image, i.e. the PBL.

Of course. Excuse the brain fart.

> 
> Regards,
> Lucas
> 
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 0/3] Support usb booting on i.MX8MP

[Ahmad Fatoum]

On 13.08.21 22:32, Lucas Stach wrote:
> Hi Uwe,
> 
> Am Freitag, dem 13.08.2021 um 17:22 +0200 schrieb Uwe Kleine-König:
>> Hello,
>>
>> compared to (implicit) v1, the following changed here:
>>
>>  - be a bit more conservative in "imx-usb-loader: Add support for
>>    i.MX8MP" regarding last_transfer; feedback by Sascha Hauer
>>
>>  - Add support in PBL to be actually booted using imx-usb-loader (only
>>    tested the vendor U-Boot for v1), also skip the barebox header (i.e.
>>    the first (usually) 0x8000 bytes) before the IVT header on uploadeing
>>
>> There are two things that resulted in discussions between Lucas, Ahmad
>> and me during development of this series:
>>
>>  - On i.MX8MM and i.MX8MQ after the PBL is uploaded it's followed up by
>>    the complete image. So the PBL is uploaded twice. The motivation for
>>    this behaviour was that when booting from MMC loading from offset 0
>>    is easier because depending on the mode the hardware is in the offset
>>    has to be specified in different ways. So to make the two modes more
>>    similar the PBL is loaded twice for both modes.
>>
>>    Here however after the PBL only the piggy data is sent because it
>>    feels wrong for me to spend some extra effort in imx-usb-loader to do
>>    a kludge for USB just because there are some difficulties with MMC.
>>
>>    Arguments for this choice (apart from that already being completed
>>    and tested :-) are:
>>
>>    - No duplication of data that is overwritten anyhow
>>    - Better interoperability with U-Boot/mfg-tools
>>      While this is usually not our focus I consider it quite annoying
>>      that there are at least three implementations of imx-usb-loader and
>>      depending on which machine and bootloader you use you have to pick
>>      the right implementation. With the longterm goal to have
>>      imx-usb-loader and barebox available in Debian some
>>      interoperability would be nice.
>>
>>    I quickly tried using the boot rom load image support when booting
>>    from SD, but it didn't work out of the box and I didn't debug that.
>>    However I imagine that in this mode it would also be more natural to
>>    not expect the PBL twice.
>>
>>    Switching to the duplicated operation mode would make imx-usb-loader
>>    a bit more complicated (because in the MXS code path the duplication
>>    isn't available yet), in return we'd save an offset calculation in
>>    the PBL.
> 
> I can follow those arguments, but for the sake of the sanity of
> everyone involved I would vote for changing the i.MX8MM USB loading to
> work in the same way, maybe even using the ROM API if it works there
> too. i.MX8MM isn't too widely used yet, so I guess we can still change
> it without inflicting too much incompatibility pain.

8MM doesn't have ROM API. 8MN does.

> The i.MX8M* Barebox lowlevel flow is already quite delicate, so I would
> rather avoid having to deal with subtle differences between the various
> family members.

On 8MM, the PBL loaded in the second upload is overwritten with the running
PBL (from the first upload), so practically, the only change seems to be
whether we overwrite a PBL transferred for nought or not?

>>    I should be possible to look at the first post-PBL data chunk coming
>>    in via USB and judge which mode of operation is used and behave
>>    accordingly. If this is considered a good idea I can create a patch
>>    for that.

Sounds like a good compromise, but I don't yet understand Lucas'
point. PBL is entered twice with Uwe's changes as well.

>>  - When booting via USB the boot source is identified as "serial". I
>>    would expect that the semantic of that is rs232 and not USB. The
>>    source of this confusion is probably that the Reference Manual calls
>>    the USB  boot mode "USB Serial Download boot". I stuck to
>>    BOOTSOURCE_SERIAL here for consistency. If it's not only me who would
>>    consider using BOOTSOURCE_USB instead a fix this should be changed
>>    consistently for all i.MX platforms.
>>    Ahmad pointed out that this might break barebox shell scripts.
> 
> This however sounds like breaking backwards compat for no real gain. As
> even the reference manual calls this boot mode "serial" I always found
> this quite clear. My vote is on keeping things as they are.

Agreed.

Cheers,
Ahmad

> 
> Regards,
> Lucas
> 
>>
>> Uwe Kleine-König (3):
>>   imx8mp-evk: Add support for booting via USB
>>   imx-usb-loader: Drop nearly unused struct usb_id
>>   imx-usb-loader: Add support for i.MX8MP
>>
>>  arch/arm/boards/nxp-imx8mp-evk/lowlevel.c |  27 +++++
>>  arch/arm/mach-imx/boot.c                  |   4 +-
>>  include/asm-generic/sections.h            |   1 +
>>  scripts/imx/imx-usb-loader.c              | 131 ++++++++++++----------
>>  4 files changed, 103 insertions(+), 60 deletions(-)
>>
>>
>> base-commit: 72424fd057d135ec0e41139fe4cb5740471d33a5
> 
> 
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 1/3] imx8mp-evk: Add support for booting via USB

[Ahmad Fatoum]

On 13.08.21 17:22, Uwe Kleine-König wrote:
> ---

I copied these changes to the nxp-imx8mn-evk, which has the same
ROM API and I can bootstrap with uuu, therefore:

Tested-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

>  arch/arm/boards/nxp-imx8mp-evk/lowlevel.c | 27 +++++++++++++++++++++++
>  arch/arm/mach-imx/boot.c                  |  4 +++-
>  include/asm-generic/sections.h            |  1 +
>  3 files changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boards/nxp-imx8mp-evk/lowlevel.c b/arch/arm/boards/nxp-imx8mp-evk/lowlevel.c
> index 3298ded5866d..1fb7899198d6 100644
> --- a/arch/arm/boards/nxp-imx8mp-evk/lowlevel.c
> +++ b/arch/arm/boards/nxp-imx8mp-evk/lowlevel.c
> @@ -99,6 +99,30 @@ static int power_init_board(void)
>  	return 0;
>  }
>  
> +/* read piggydata via a bootrom callback and place it behind our copy in SDRAM */
> +static int imx8m_bootrom_load_image(void)
> +{
> +	int (*download_image)(u8 *dest, u32 offset, u32 size, u32 xor) = *(void **)0x988;
> +	size_t count = __piggydata_end - __piggydata_start;
> +	char *p = (char *)MX8M_ATF_BL33_BASE_ADDR + (__piggydata_start - __image_start);
> +
> +	while (count) {
> +		size_t chunksize = min(count, (size_t)1024);
> +		int ret;
> +
> +		ret = download_image(p, 0, chunksize, (uintptr_t)p ^ chunksize);
> +		if (ret != 0xf0) {
> +			pr_err("Failed to load piggy data (ret = %x)\n", ret);
> +			return -EIO;
> +		}
> +
> +		p += chunksize;
> +		count -= chunksize;
> +	}
> +
> +	return 0;
> +}
> +
>  extern struct dram_timing_info imx8mp_evk_dram_timing;
>  
>  static void start_atf(void)
> @@ -125,6 +149,9 @@ static void start_atf(void)
>  	case BOOTSOURCE_MMC:
>  		imx8mp_esdhc_load_image(instance, false);
>  		break;
> +	case BOOTSOURCE_SERIAL:
> +		imx8m_bootrom_load_image();
> +		break;
>  	default:
>  		printf("Unhandled bootsource BOOTSOURCE_%d\n", src);
>  		hang();
> diff --git a/arch/arm/mach-imx/boot.c b/arch/arm/mach-imx/boot.c
> index 2b66bbf71eb1..7c1d49291045 100644
> --- a/arch/arm/mach-imx/boot.c
> +++ b/arch/arm/mach-imx/boot.c
> @@ -495,10 +495,12 @@ static void __imx7_get_boot_source(enum bootsource *src, int *instance,
>  	case 5:
>  		*src = BOOTSOURCE_NOR;
>  		break;
> -	case 15:
> +	case 14: /* observed on i.MX8MP for USB "serial" booting */
> +	case 15: /* observed on i.MX8MM for USB "serial" booting */
>  		*src = BOOTSOURCE_SERIAL;
>  		break;
>  	default:
> +		*src = BOOTSOURCE_UNKNOWN;
>  		break;
>  	}
>  }
> diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
> index 870bff21f668..597c4951ea5e 100644
> --- a/include/asm-generic/sections.h
> +++ b/include/asm-generic/sections.h
> @@ -9,6 +9,7 @@ extern char _end[];
>  extern char __image_start[];
>  extern char __image_end[];
>  extern char __piggydata_start[];
> +extern char __piggydata_end[];
>  extern void *_barebox_image_size;
>  extern void *_barebox_bare_init_size;
>  extern void *_barebox_pbl_size;
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 2/3] imx-usb-loader: Drop nearly unused struct usb_id

[Ahmad Fatoum]

On 13.08.21 17:22, Uwe Kleine-König wrote:
> Only one of the two members of struct usb_id is actually used. So
> replace struct usb_id by a struct mach_id.
> 
> Signed-off-by: Uwe Kleine-Koenig <u.kleine-koenig@pengutronix.de>

Tested-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

> ---
>  scripts/imx/imx-usb-loader.c | 51 ++++++++++++------------------------
>  1 file changed, 17 insertions(+), 34 deletions(-)
> 
> diff --git a/scripts/imx/imx-usb-loader.c b/scripts/imx/imx-usb-loader.c
> index cff77f27f264..3e96c86f2f29 100644
> --- a/scripts/imx/imx-usb-loader.c
> +++ b/scripts/imx/imx-usb-loader.c
> @@ -52,7 +52,7 @@
>  
>  int verbose;
>  static struct libusb_device_handle *usb_dev_handle;
> -static struct usb_id *usb_id;
> +static const struct mach_id *mach_id;
>  
>  struct mach_id {
>  	struct mach_id * next;
> @@ -79,11 +79,6 @@ struct usb_work {
>  	unsigned char plug;
>  };
>  
> -struct usb_id {
> -	const struct mach_id *mach_id;
> -	struct usb_work *work;
> -};
> -
>  static const struct mach_id imx_ids[] = {
>  	{
>  		.vid = 0x066f,
> @@ -507,8 +502,8 @@ static int read_file(const char *name, unsigned char **buffer, unsigned *size)
>  static int transfer(int report, unsigned char *p, unsigned cnt, int *last_trans)
>  {
>  	int err;
> -	if (cnt > usb_id->mach_id->max_transfer)
> -		cnt = usb_id->mach_id->max_transfer;
> +	if (cnt > mach_id->max_transfer)
> +		cnt = mach_id->max_transfer;
>  
>  	if (verbose > 4) {
>  		printf("report=%i\n", report);
> @@ -516,7 +511,7 @@ static int transfer(int report, unsigned char *p, unsigned cnt, int *last_trans)
>  			dump_bytes(p, cnt, 0);
>  	}
>  
> -	if (usb_id->mach_id->mode == MODE_BULK) {
> +	if (mach_id->mode == MODE_BULK) {
>  		*last_trans = 0;
>  		err = libusb_bulk_transfer(usb_dev_handle,
>  					   (report < 3) ? 1 : 2 + EP_IN, p, cnt, last_trans, 1000);
> @@ -602,7 +597,7 @@ static int do_status(void)
>  			break;
>  	}
>  
> -	if (usb_id->mach_id->mode == MODE_HID) {
> +	if (mach_id->mode == MODE_HID) {
>  		err = transfer(4, tmp, sizeof(tmp), &last_trans);
>  		if (err)
>  			printf("4 in err=%i, last_trans=%i  %02x %02x %02x %02x\n",
> @@ -810,7 +805,7 @@ static int load_file(void *buf, unsigned len, unsigned dladdr,
>  
>  	retry = 0;
>  
> -	if (usb_id->mach_id->mode == MODE_BULK) {
> +	if (mach_id->mode == MODE_BULK) {
>  		err = transfer(3, tmp, sizeof(tmp), &last_trans);
>  		if (err)
>  			printf("in err=%i, last_trans=%i  %02x %02x %02x %02x\n",
> @@ -821,7 +816,7 @@ static int load_file(void *buf, unsigned len, unsigned dladdr,
>  	cnt = len;
>  
>  	while (1) {
> -		int now = get_min(cnt, usb_id->mach_id->max_transfer);
> +		int now = get_min(cnt, mach_id->max_transfer);
>  
>  		if (!now)
>  			break;
> @@ -839,7 +834,7 @@ static int load_file(void *buf, unsigned len, unsigned dladdr,
>  	if (mode_barebox)
>  		return transfer_size;
>  
> -	if (usb_id->mach_id->mode == MODE_HID) {
> +	if (mach_id->mode == MODE_HID) {
>  		err = transfer(3, tmp, sizeof(tmp), &last_trans);
>  		if (err)
>  			printf("3 in err=%i, last_trans=%i  %02x %02x %02x %02x\n",
> @@ -1233,7 +1228,7 @@ static int is_header(const unsigned char *p)
>  	const struct imx_flash_header_v2 *hdr =
>  		(const struct imx_flash_header_v2 *)p;
>  
> -	switch (usb_id->mach_id->header_type) {
> +	switch (mach_id->header_type) {
>  	case HDR_MX51:
>  		if (ohdr->app_code_barker == 0xb1)
>  			return 1;
> @@ -1253,7 +1248,7 @@ static int perform_dcd(unsigned char *p, const unsigned char *file_start,
>  	struct imx_flash_header_v2 *hdr = (struct imx_flash_header_v2 *)p;
>  	int ret = 0;
>  
> -	switch (usb_id->mach_id->header_type) {
> +	switch (mach_id->header_type) {
>  	case HDR_MX51:
>  		ret = write_dcd_table_old(ohdr, file_start, cnt);
>  		ohdr->dcd_block_len = 0;
> @@ -1274,7 +1269,7 @@ static int get_dl_start(const unsigned char *p, const unsigned char *file_start,
>  		unsigned *header_addr)
>  {
>  	const unsigned char *file_end = file_start + cnt;
> -	switch (usb_id->mach_id->header_type) {
> +	switch (mach_id->header_type) {
>  	case HDR_MX51:
>  	{
>  		struct imx_flash_header *ohdr = (struct imx_flash_header *)p;
> @@ -1315,7 +1310,7 @@ static int get_payload_start(const unsigned char *p, uint32_t *ofs)
>  {
>  	struct imx_flash_header_v2 *hdr = (struct imx_flash_header_v2 *)p;
>  
> -	switch (usb_id->mach_id->header_type) {
> +	switch (mach_id->header_type) {
>  	case HDR_MX51:
>  		return -EINVAL;
>  
> @@ -1430,7 +1425,7 @@ static int do_irom_download(struct usb_work *curr, int verify)
>  
>  		memcpy(verify_buffer, image, 64);
>  
> -		if ((type == FT_APP) && (usb_id->mach_id->mode != MODE_HID)) {
> +		if ((type == FT_APP) && (mach_id->mode != MODE_HID)) {
>  			type = FT_LOAD_ONLY;
>  			verify = 2;
>  		}
> @@ -1469,7 +1464,7 @@ static int do_irom_download(struct usb_work *curr, int verify)
>  		}
>  	}
>  
> -	if (usb_id->mach_id->mode == MODE_HID && type == FT_APP) {
> +	if (mach_id->mode == MODE_HID && type == FT_APP) {
>  		printf("jumping to 0x%08x\n", header_addr);
>  
>  		ret = sdp_jump_address(header_addr);
> @@ -1532,7 +1527,7 @@ static int mxs_load_file(libusb_device_handle *dev, uint8_t *data, int size)
>  	cnt = size;
>  
>  	while (1) {
> -		int now = get_min(cnt, usb_id->mach_id->max_transfer);
> +		int now = get_min(cnt, mach_id->max_transfer);
>  
>  		if (!now)
>  			break;
> @@ -1591,7 +1586,6 @@ static void usage(const char *prgname)
>  
>  int main(int argc, char *argv[])
>  {
> -	const struct mach_id *mach;
>  	libusb_device **devs;
>  	libusb_device *dev;
>  	int r;
> @@ -1663,7 +1657,7 @@ int main(int argc, char *argv[])
>  		goto out;
>  	}
>  
> -	dev = find_imx_dev(devs, &mach, devpath, devtype);
> +	dev = find_imx_dev(devs, &mach_id, devpath, devtype);
>  	if (!dev) {
>  		fprintf(stderr, "no supported device found\n");
>  		goto out;
> @@ -1682,15 +1676,7 @@ int main(int argc, char *argv[])
>  		goto out;
>  	}
>  
> -	usb_id = malloc(sizeof(*usb_id));
> -	if (!usb_id) {
> -		perror("malloc");
> -		exit(1);
> -	}
> -
> -	usb_id->mach_id = mach;
> -
> -	if (mach->dev_type == DEV_MXS) {
> +	if (mach_id->dev_type == DEV_MXS) {
>  		ret = mxs_work(&w);
>  		goto out;
>  	}
> @@ -1715,9 +1701,6 @@ int main(int argc, char *argv[])
>  
>  	ret = 0;
>  out:
> -	if (usb_id)
> -		free(usb_id);
> -
>  	if (usb_dev_handle)
>  		libusb_close(usb_dev_handle);
>  
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 3/3] imx-usb-loader: Add support for i.MX8MP

[Ahmad Fatoum]

On 13.08.21 17:22, Uwe Kleine-König wrote:
> The i.MX8MP uses a protocol similar to the MXS. The relevant differences
> are:
> 
>  - Maximal transfer size is 1020
>  - HID reports must be sent to EP1 instead of using a control transfer
>  - The FW_DOWNLOAD command must not be send.
>  - The image to upload must start with the IVT header (usually at offset
>    0x8000), so the barebox header isn't transferred.
> 
> Signed-off-by: Uwe Kleine-Koenig <u.kleine-koenig@pengutronix.de>

With correct VID/PID added, this works on the i.MX8MN as well:

Tested-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

> ---
>  scripts/imx/imx-usb-loader.c | 80 +++++++++++++++++++++++++-----------
>  1 file changed, 55 insertions(+), 25 deletions(-)
> 
> diff --git a/scripts/imx/imx-usb-loader.c b/scripts/imx/imx-usb-loader.c
> index 3e96c86f2f29..cf87e0f91d65 100644
> --- a/scripts/imx/imx-usb-loader.c
> +++ b/scripts/imx/imx-usb-loader.c
> @@ -71,6 +71,7 @@ struct mach_id {
>  #define DEV_IMX		0
>  #define DEV_MXS		1
>  	unsigned char dev_type;
> +	unsigned char hid_endpoint;
>  };
>  
>  struct usb_work {
> @@ -177,6 +178,14 @@ static const struct mach_id imx_ids[] = {
>  		.header_type = HDR_MX53,
>  		.mode = MODE_HID,
>  		.max_transfer = 1024,
> +	}, {
> +		.vid = 0x1fc9,
> +		.pid = 0x0146,
> +		.name = "i.MX8MP",
> +		.header_type = HDR_MX53,
> +		.max_transfer = 1020,
> +		.dev_type = DEV_MXS,
> +		.hid_endpoint = 1,
>  	}, {
>  		.vid = 0x1fc9,
>  		.pid = 0x012b,
> @@ -522,15 +531,22 @@ static int transfer(int report, unsigned char *p, unsigned cnt, int *last_trans)
>  
>  		if (report < 3) {
>  			memcpy(&tmp[1], p, cnt);
> -			err = libusb_control_transfer(usb_dev_handle,
> -					CTRL_OUT,
> -					HID_SET_REPORT,
> -					(HID_REPORT_TYPE_OUTPUT << 8) | report,
> -					0,
> -					tmp, cnt + 1, 1000);
> -			*last_trans = (err > 0) ? err - 1 : 0;
> -			if (err > 0)
> -				err = 0;
> +			if (mach_id->hid_endpoint) {
> +				int trans;
> +				err = libusb_interrupt_transfer(usb_dev_handle,
> +						mach_id->hid_endpoint, tmp, cnt + 1, &trans, 1000);
> +				*last_trans = trans - 1;
> +			} else {
> +				err = libusb_control_transfer(usb_dev_handle,
> +						CTRL_OUT,
> +						HID_SET_REPORT,
> +						(HID_REPORT_TYPE_OUTPUT << 8) | report,
> +						0,
> +						tmp, cnt + 1, 1000);
> +				*last_trans = (err > 0) ? err - 1 : 0;
> +				if (err > 0)
> +					err = 0;
> +			}
>  		} else {
>  			*last_trans = 0;
>  			memset(&tmp[1], 0, cnt);
> @@ -1500,32 +1516,46 @@ static int write_mem(const struct config_data *data, uint32_t addr,
>  }
>  
>  /* MXS section */
> -static int mxs_load_file(libusb_device_handle *dev, uint8_t *data, int size)
> +static int mxs_load_file(struct usb_work *curr, libusb_device_handle *dev, uint8_t *data, int size)
>  {
>  	static struct mxs_command dl_command;
>  	int last_trans, err;
>  	void *p;
>  	int cnt;
>  
> -	dl_command.sign = htonl(0x424c5443); /* Signature: BLTC */
> -	dl_command.tag = htonl(0x1);
> -	dl_command.size = htonl(size);
> -	dl_command.flags = 0;
> -	dl_command.rsvd[0] = 0;
> -	dl_command.rsvd[1] = 0;
> -	dl_command.cmd = MXS_CMD_FW_DOWNLOAD;
> -	dl_command.dw_size = htonl(size);
> -
> -	err = transfer(1, (unsigned char *) &dl_command, 20, &last_trans);
> -	if (err) {
> -		printf("transfer error at init step: err=%i, last_trans=%i\n",
> -		       err, last_trans);
> -		return err;
> +	if (!mach_id->hid_endpoint) {
> +		dl_command.sign = htonl(0x424c5443); /* Signature: BLTC */
> +		dl_command.tag = htonl(0x1);
> +		dl_command.size = htonl(size);
> +		dl_command.flags = 0;
> +		dl_command.rsvd[0] = 0;
> +		dl_command.rsvd[1] = 0;
> +		dl_command.cmd = MXS_CMD_FW_DOWNLOAD;
> +		dl_command.dw_size = htonl(size);
> +
> +		err = transfer(1, (unsigned char *) &dl_command, 20, &last_trans);
> +		if (err) {
> +			printf("transfer error at init step: err=%i, last_trans=%i\n",
> +					err, last_trans);
> +			return err;
> +		}
>  	}
>  
>  	p = data;
>  	cnt = size;
>  
> +	if (mach_id->header_type != HDR_NONE) {
> +		unsigned int dummy;
> +		err = process_header(curr, p, cnt, &dummy, &dummy, &dummy);
> +		if (err < 0) {
> +			printf("Failed to find IVT header\n");
> +			return err;
> +		}
> +
> +		p += err;
> +		cnt -= err;
> +	}
> +
>  	while (1) {
>  		int now = get_min(cnt, mach_id->max_transfer);
>  
> @@ -1555,7 +1585,7 @@ static int mxs_work(struct usb_work *curr)
>  	if (ret < 0)
>  		return ret;
>  
> -	return mxs_load_file(usb_dev_handle, buf, fsize);
> +	return mxs_load_file(curr, usb_dev_handle, buf, fsize);
>  }
>  /* end of mxs section */
>  
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox