[PATCH 1/2] ARM: i.MX bbu: reimplement IMX_INTERNAL_FLAG_KEEP_DOSPART flag

[PATCH 1/2] ARM: i.MX bbu: reimplement IMX_INTERNAL_FLAG_KEEP_DOSPART flag

[Sascha Hauer]

This patch reimplements the IMX_INTERNAL_FLAG_KEEP_DOSPART flag
and makes it more generic. Until now we only kept a dos partition
table over the update. Beginning with i.MX8 we may also want to
preserve a GPT, so we have to extend the preserved area.

It might also be the case that not (only) a partition table is
stored in the initial area of a device, but also other unrelated
data, so it's better to just keep the initial area that is unused
by the i.MX ROM. It's also good to export the flag to allow boards
to specify the initial area shall be preserved.

When a board wants to set the flag for a mtd like device then it
has to check for suitable erase sizes beforehand. We do not check
this (yet).

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 arch/arm/mach-imx/imx-bbu-internal.c | 70 +++++++++-------------------
 arch/arm/mach-imx/include/mach/bbu.h | 15 ++++++
 2 files changed, 38 insertions(+), 47 deletions(-)

diff --git a/arch/arm/mach-imx/imx-bbu-internal.c b/arch/arm/mach-imx/imx-bbu-internal.c
index 84810f18a9..5422235b1b 100644
--- a/arch/arm/mach-imx/imx-bbu-internal.c
+++ b/arch/arm/mach-imx/imx-bbu-internal.c
@@ -35,7 +35,6 @@
 #define FLASH_HEADER_OFFSET_MMC		0x400
 
 #define IMX_INTERNAL_FLAG_NAND		(1 << 0)
-#define IMX_INTERNAL_FLAG_KEEP_DOSPART	(1 << 1)
 #define IMX_INTERNAL_FLAG_ERASE		(1 << 2)
 
 struct imx_internal_bbu_handler {
@@ -53,26 +52,31 @@ static int imx_bbu_write_device(struct imx_internal_bbu_handler *imx_handler,
 		const char *devicefile, struct bbu_data *data,
 		const void *buf, int image_len)
 {
-	int fd, ret;
-	int written = 0;
+	int fd, ret, offset = 0;
 
 	fd = open(devicefile, O_RDWR | O_CREAT);
 	if (fd < 0)
 		return fd;
 
+	if (imx_handler->handler.flags & IMX_BBU_FLAG_KEEP_HEAD) {
+		image_len -= imx_handler->flash_header_offset;
+		offset += imx_handler->flash_header_offset;
+		buf += imx_handler->flash_header_offset;
+	}
+
 	if (imx_handler->flags & IMX_INTERNAL_FLAG_ERASE) {
-		pr_debug("%s: unprotecting %s from 0 to 0x%08x\n", __func__,
-				devicefile, image_len);
-		ret = protect(fd, image_len, 0, 0);
+		pr_debug("%s: unprotecting %s from 0x%08x to 0x%08x\n", __func__,
+				devicefile, offset, image_len);
+		ret = protect(fd, image_len, offset, 0);
 		if (ret && ret != -ENOSYS) {
 			pr_err("unprotecting %s failed with %s\n", devicefile,
 					strerror(-ret));
 			goto err_close;
 		}
 
-		pr_debug("%s: erasing %s from 0 to 0x%08x\n", __func__,
-				devicefile, image_len);
-		ret = erase(fd, image_len, 0);
+		pr_debug("%s: erasing %s from 0x%08x to 0x%08x\n", __func__,
+				devicefile, offset, image_len);
+		ret = erase(fd, image_len, offset);
 		if (ret) {
 			pr_err("erasing %s failed with %s\n", devicefile,
 					strerror(-ret));
@@ -80,43 +84,14 @@ static int imx_bbu_write_device(struct imx_internal_bbu_handler *imx_handler,
 		}
 	}
 
-	if (imx_handler->flags & IMX_INTERNAL_FLAG_KEEP_DOSPART) {
-		void *mbr = xzalloc(512);
-
-		pr_debug("%s: reading DOS partition table in order to keep it\n", __func__);
-
-		ret = read(fd, mbr, 512);
-		if (ret < 0) {
-			free(mbr);
-			goto err_close;
-		}
-
-		memcpy(mbr, buf, 0x1b8);
-
-		ret = lseek(fd, 0, SEEK_SET);
-		if (ret) {
-			free(mbr);
-			goto err_close;
-		}
-
-		ret = write(fd, mbr, 512);
-
-		free(mbr);
-
-		if (ret < 0)
-			goto err_close;
-
-		written = 512;
-	}
-
-	ret = write(fd, buf + written, image_len - written);
+	ret = pwrite(fd, buf, image_len, offset);
 	if (ret < 0)
 		goto err_close;
 
 	if (imx_handler->flags & IMX_INTERNAL_FLAG_ERASE) {
-		pr_debug("%s: protecting %s from 0 to 0x%08x\n", __func__,
-				devicefile, image_len);
-		ret = protect(fd, image_len, 0, 1);
+		pr_debug("%s: protecting %s from 0x%08x to 0x%08x\n", __func__,
+				devicefile, offset, image_len);
+		ret = protect(fd, image_len, offset, 1);
 		if (ret && ret != -ENOSYS) {
 			pr_err("protecting %s failed with %s\n", devicefile,
 					strerror(-ret));
@@ -454,6 +429,7 @@ static struct imx_internal_bbu_handler *__init_handler(const char *name, char *d
 	struct bbu_handler *handler;
 
 	imx_handler = xzalloc(sizeof(*imx_handler));
+	imx_handler->flags = flags & IMX_BBU_FLAG_MASK;
 	handler = &imx_handler->handler;
 	handler->devicefile = devicefile;
 	handler->name = name;
@@ -482,7 +458,7 @@ imx_bbu_internal_mmc_register_handler(const char *name, char *devicefile,
 	imx_handler = __init_handler(name, devicefile, flags);
 	imx_handler->flash_header_offset = FLASH_HEADER_OFFSET_MMC;
 
-	imx_handler->flags = IMX_INTERNAL_FLAG_KEEP_DOSPART;
+	imx_handler->flags |= IMX_BBU_FLAG_KEEP_HEAD;
 	imx_handler->handler.handler = handler;
 
 	return __register_handler(imx_handler);
@@ -496,7 +472,7 @@ int imx51_bbu_internal_spi_i2c_register_handler(const char *name,
 	imx_handler = __init_handler(name, devicefile, flags);
 	imx_handler->flash_header_offset = FLASH_HEADER_OFFSET_MMC;
 
-	imx_handler->flags = IMX_INTERNAL_FLAG_ERASE;
+	imx_handler->flags |= IMX_INTERNAL_FLAG_ERASE;
 	imx_handler->handler.handler = imx_bbu_internal_v1_update;
 
 	return __register_handler(imx_handler);
@@ -536,7 +512,7 @@ int imx53_bbu_internal_spi_i2c_register_handler(const char *name, char *devicefi
 	imx_handler = __init_handler(name, devicefile, flags);
 	imx_handler->flash_header_offset = FLASH_HEADER_OFFSET_MMC;
 
-	imx_handler->flags = IMX_INTERNAL_FLAG_ERASE;
+	imx_handler->flags |= IMX_INTERNAL_FLAG_ERASE;
 	imx_handler->handler.handler = imx_bbu_internal_v2_update;
 
 	return __register_handler(imx_handler);
@@ -554,7 +530,7 @@ int imx53_bbu_internal_nand_register_handler(const char *name,
 	imx_handler->flash_header_offset = FLASH_HEADER_OFFSET_MMC;
 
 	imx_handler->handler.handler = imx_bbu_internal_v2_update;
-	imx_handler->flags = IMX_INTERNAL_FLAG_NAND;
+	imx_handler->flags |= IMX_INTERNAL_FLAG_NAND;
 	imx_handler->handler.devicefile = "/dev/nand0";
 	imx_handler->device_size = partition_size;
 
@@ -613,7 +589,7 @@ int imx_bbu_external_nor_register_handler(const char *name, char *devicefile,
 	struct imx_internal_bbu_handler *imx_handler;
 
 	imx_handler = __init_handler(name, devicefile, flags);
-	imx_handler->flags = IMX_INTERNAL_FLAG_ERASE;
+	imx_handler->flags |= IMX_INTERNAL_FLAG_ERASE;
 	imx_handler->handler.handler = imx_bbu_external_update;
 
 	return __register_handler(imx_handler);
diff --git a/arch/arm/mach-imx/include/mach/bbu.h b/arch/arm/mach-imx/include/mach/bbu.h
index d623828312..c2e875d342 100644
--- a/arch/arm/mach-imx/include/mach/bbu.h
+++ b/arch/arm/mach-imx/include/mach/bbu.h
@@ -7,6 +7,21 @@
 struct imx_dcd_entry;
 struct imx_dcd_v2_entry;
 
+/*
+ * The ROM code reads images from a certain offset of the boot device
+ * (usually 0x400), whereas the update images start from offset 0x0.
+ * Set this flag to skip the offset on both the update image and the
+ * device so that the initial boot device portion is preserved. This
+ * is useful if a partition table or other data is in this area.
+ */
+#define IMX_BBU_FLAG_KEEP_HEAD	(1 << 16)
+
+/*
+ * The upper 16 bit of the flags passes to the below functions are reserved
+ * for i.MX specific flags
+ */
+#define IMX_BBU_FLAG_MASK	0xffff0000
+
 #ifdef CONFIG_BAREBOX_UPDATE
 
 int imx51_bbu_internal_mmc_register_handler(const char *name, char *devicefile,
-- 
2.17.1


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

[PATCH 2/2] ARM: i.MX bbu: support partitions starting at i.MX header

[Sascha Hauer]

The i.MX header is at an offset in the boot device, usually 0x400 bytes.
This patch adds a flag to support the case that the partition the image
is written to starts at that offset rather than 0x0.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 arch/arm/mach-imx/imx-bbu-internal.c | 7 +++++--
 arch/arm/mach-imx/include/mach/bbu.h | 8 ++++++++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-imx/imx-bbu-internal.c b/arch/arm/mach-imx/imx-bbu-internal.c
index 5422235b1b..23686e9d61 100644
--- a/arch/arm/mach-imx/imx-bbu-internal.c
+++ b/arch/arm/mach-imx/imx-bbu-internal.c
@@ -58,12 +58,15 @@ static int imx_bbu_write_device(struct imx_internal_bbu_handler *imx_handler,
 	if (fd < 0)
 		return fd;
 
-	if (imx_handler->handler.flags & IMX_BBU_FLAG_KEEP_HEAD) {
+	if (imx_handler->handler.flags & (IMX_BBU_FLAG_KEEP_HEAD |
+	    IMX_BBU_FLAG_PARTITION_STARTS_AT_HEADER)) {
 		image_len -= imx_handler->flash_header_offset;
-		offset += imx_handler->flash_header_offset;
 		buf += imx_handler->flash_header_offset;
 	}
 
+	if (imx_handler->handler.flags & IMX_BBU_FLAG_KEEP_HEAD)
+		offset += imx_handler->flash_header_offset;
+
 	if (imx_handler->flags & IMX_INTERNAL_FLAG_ERASE) {
 		pr_debug("%s: unprotecting %s from 0x%08x to 0x%08x\n", __func__,
 				devicefile, offset, image_len);
diff --git a/arch/arm/mach-imx/include/mach/bbu.h b/arch/arm/mach-imx/include/mach/bbu.h
index c2e875d342..19b446ed60 100644
--- a/arch/arm/mach-imx/include/mach/bbu.h
+++ b/arch/arm/mach-imx/include/mach/bbu.h
@@ -16,6 +16,14 @@ struct imx_dcd_v2_entry;
  */
 #define IMX_BBU_FLAG_KEEP_HEAD	(1 << 16)
 
+/*
+ * Set this flag when the partition the update image is written to
+ * actually starts at the offset where the i.MX flash header is expected
+ * (usually 0x400). This means for the update code that it has to skip
+ * the first 0x400 bytes of the image.
+ */
+#define IMX_BBU_FLAG_PARTITION_STARTS_AT_HEADER	(1 << 17)
+
 /*
  * The upper 16 bit of the flags passes to the below functions are reserved
  * for i.MX specific flags
-- 
2.17.1


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

Re: [PATCH 1/2] ARM: i.MX bbu: reimplement IMX_INTERNAL_FLAG_KEEP_DOSPART flag

[Andrey Smirnov]

On Thu, Jun 28, 2018 at 11:49 PM Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> This patch reimplements the IMX_INTERNAL_FLAG_KEEP_DOSPART flag
> and makes it more generic. Until now we only kept a dos partition
> table over the update. Beginning with i.MX8 we may also want to
> preserve a GPT, so we have to extend the preserved area.
>
> It might also be the case that not (only) a partition table is
> stored in the initial area of a device, but also other unrelated
> data, so it's better to just keep the initial area that is unused
> by the i.MX ROM. It's also good to export the flag to allow boards
> to specify the initial area shall be preserved.
>
> When a board wants to set the flag for a mtd like device then it
> has to check for suitable erase sizes beforehand. We do not check
> this (yet).
>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  arch/arm/mach-imx/imx-bbu-internal.c | 70 +++++++++-------------------
>  arch/arm/mach-imx/include/mach/bbu.h | 15 ++++++
>  2 files changed, 38 insertions(+), 47 deletions(-)
>
> diff --git a/arch/arm/mach-imx/imx-bbu-internal.c b/arch/arm/mach-imx/imx-bbu-internal.c
> index 84810f18a9..5422235b1b 100644
> --- a/arch/arm/mach-imx/imx-bbu-internal.c
> +++ b/arch/arm/mach-imx/imx-bbu-internal.c
> @@ -35,7 +35,6 @@
>  #define FLASH_HEADER_OFFSET_MMC                0x400
>
>  #define IMX_INTERNAL_FLAG_NAND         (1 << 0)
> -#define IMX_INTERNAL_FLAG_KEEP_DOSPART (1 << 1)
>  #define IMX_INTERNAL_FLAG_ERASE                (1 << 2)
>
>  struct imx_internal_bbu_handler {
> @@ -53,26 +52,31 @@ static int imx_bbu_write_device(struct imx_internal_bbu_handler *imx_handler,
>                 const char *devicefile, struct bbu_data *data,
>                 const void *buf, int image_len)
>  {
> -       int fd, ret;
> -       int written = 0;
> +       int fd, ret, offset = 0;
>
>         fd = open(devicefile, O_RDWR | O_CREAT);
>         if (fd < 0)
>                 return fd;
>
> +       if (imx_handler->handler.flags & IMX_BBU_FLAG_KEEP_HEAD) {
> +               image_len -= imx_handler->flash_header_offset;
> +               offset += imx_handler->flash_header_offset;
> +               buf += imx_handler->flash_header_offset;
> +       }
> +
>         if (imx_handler->flags & IMX_INTERNAL_FLAG_ERASE) {
> -               pr_debug("%s: unprotecting %s from 0 to 0x%08x\n", __func__,
> -                               devicefile, image_len);
> -               ret = protect(fd, image_len, 0, 0);
> +               pr_debug("%s: unprotecting %s from 0x%08x to 0x%08x\n", __func__,
> +                               devicefile, offset, image_len);
> +               ret = protect(fd, image_len, offset, 0);
>                 if (ret && ret != -ENOSYS) {
>                         pr_err("unprotecting %s failed with %s\n", devicefile,
>                                         strerror(-ret));
>                         goto err_close;
>                 }
>
> -               pr_debug("%s: erasing %s from 0 to 0x%08x\n", __func__,
> -                               devicefile, image_len);
> -               ret = erase(fd, image_len, 0);
> +               pr_debug("%s: erasing %s from 0x%08x to 0x%08x\n", __func__,
> +                               devicefile, offset, image_len);
> +               ret = erase(fd, image_len, offset);
>                 if (ret) {
>                         pr_err("erasing %s failed with %s\n", devicefile,
>                                         strerror(-ret));
> @@ -80,43 +84,14 @@ static int imx_bbu_write_device(struct imx_internal_bbu_handler *imx_handler,
>                 }
>         }
>
> -       if (imx_handler->flags & IMX_INTERNAL_FLAG_KEEP_DOSPART) {
> -               void *mbr = xzalloc(512);
> -
> -               pr_debug("%s: reading DOS partition table in order to keep it\n", __func__);
> -
> -               ret = read(fd, mbr, 512);
> -               if (ret < 0) {
> -                       free(mbr);
> -                       goto err_close;
> -               }
> -
> -               memcpy(mbr, buf, 0x1b8);
> -
> -               ret = lseek(fd, 0, SEEK_SET);
> -               if (ret) {
> -                       free(mbr);
> -                       goto err_close;
> -               }
> -
> -               ret = write(fd, mbr, 512);
> -
> -               free(mbr);
> -
> -               if (ret < 0)
> -                       goto err_close;
> -
> -               written = 512;
> -       }
> -
> -       ret = write(fd, buf + written, image_len - written);
> +       ret = pwrite(fd, buf, image_len, offset);
>         if (ret < 0)
>                 goto err_close;
>
>         if (imx_handler->flags & IMX_INTERNAL_FLAG_ERASE) {
> -               pr_debug("%s: protecting %s from 0 to 0x%08x\n", __func__,
> -                               devicefile, image_len);
> -               ret = protect(fd, image_len, 0, 1);
> +               pr_debug("%s: protecting %s from 0x%08x to 0x%08x\n", __func__,
> +                               devicefile, offset, image_len);
> +               ret = protect(fd, image_len, offset, 1);
>                 if (ret && ret != -ENOSYS) {
>                         pr_err("protecting %s failed with %s\n", devicefile,
>                                         strerror(-ret));
> @@ -454,6 +429,7 @@ static struct imx_internal_bbu_handler *__init_handler(const char *name, char *d
>         struct bbu_handler *handler;
>
>         imx_handler = xzalloc(sizeof(*imx_handler));
> +       imx_handler->flags = flags & IMX_BBU_FLAG_MASK;

I am not sure I understand why this is necessary. You can already
access all of the IMX_BBU flags via imx_handler->handler.flags (which
is exactly what some of your code above does) and this forces all of
those "=" -> "|=" replacements below. Do we really need to copy one
set of flags into another?

Taking a step back, if we are reserving some of the BBU flags for
internal usage, can't we just drop the notion of internal flags
altogether and just use imx_handler->handler.flags for everything
including IMX_INTERNAL_FLAG_ERASE and IMX_INTERNAL_FLAG_NAND?

>         handler = &imx_handler->handler;
>         handler->devicefile = devicefile;
>         handler->name = name;
> @@ -482,7 +458,7 @@ imx_bbu_internal_mmc_register_handler(const char *name, char *devicefile,
>         imx_handler = __init_handler(name, devicefile, flags);
>         imx_handler->flash_header_offset = FLASH_HEADER_OFFSET_MMC;
>
> -       imx_handler->flags = IMX_INTERNAL_FLAG_KEEP_DOSPART;
> +       imx_handler->flags |= IMX_BBU_FLAG_KEEP_HEAD;
>         imx_handler->handler.handler = handler;
>
>         return __register_handler(imx_handler);
> @@ -496,7 +472,7 @@ int imx51_bbu_internal_spi_i2c_register_handler(const char *name,
>         imx_handler = __init_handler(name, devicefile, flags);
>         imx_handler->flash_header_offset = FLASH_HEADER_OFFSET_MMC;
>
> -       imx_handler->flags = IMX_INTERNAL_FLAG_ERASE;
> +       imx_handler->flags |= IMX_INTERNAL_FLAG_ERASE;
>         imx_handler->handler.handler = imx_bbu_internal_v1_update;
>
>         return __register_handler(imx_handler);
> @@ -536,7 +512,7 @@ int imx53_bbu_internal_spi_i2c_register_handler(const char *name, char *devicefi
>         imx_handler = __init_handler(name, devicefile, flags);
>         imx_handler->flash_header_offset = FLASH_HEADER_OFFSET_MMC;
>
> -       imx_handler->flags = IMX_INTERNAL_FLAG_ERASE;
> +       imx_handler->flags |= IMX_INTERNAL_FLAG_ERASE;
>         imx_handler->handler.handler = imx_bbu_internal_v2_update;
>
>         return __register_handler(imx_handler);
> @@ -554,7 +530,7 @@ int imx53_bbu_internal_nand_register_handler(const char *name,
>         imx_handler->flash_header_offset = FLASH_HEADER_OFFSET_MMC;
>
>         imx_handler->handler.handler = imx_bbu_internal_v2_update;
> -       imx_handler->flags = IMX_INTERNAL_FLAG_NAND;
> +       imx_handler->flags |= IMX_INTERNAL_FLAG_NAND;
>         imx_handler->handler.devicefile = "/dev/nand0";
>         imx_handler->device_size = partition_size;
>
> @@ -613,7 +589,7 @@ int imx_bbu_external_nor_register_handler(const char *name, char *devicefile,
>         struct imx_internal_bbu_handler *imx_handler;
>
>         imx_handler = __init_handler(name, devicefile, flags);
> -       imx_handler->flags = IMX_INTERNAL_FLAG_ERASE;
> +       imx_handler->flags |= IMX_INTERNAL_FLAG_ERASE;
>         imx_handler->handler.handler = imx_bbu_external_update;
>
>         return __register_handler(imx_handler);
> diff --git a/arch/arm/mach-imx/include/mach/bbu.h b/arch/arm/mach-imx/include/mach/bbu.h
> index d623828312..c2e875d342 100644
> --- a/arch/arm/mach-imx/include/mach/bbu.h
> +++ b/arch/arm/mach-imx/include/mach/bbu.h
> @@ -7,6 +7,21 @@
>  struct imx_dcd_entry;
>  struct imx_dcd_v2_entry;
>
> +/*
> + * The ROM code reads images from a certain offset of the boot device
> + * (usually 0x400), whereas the update images start from offset 0x0.
> + * Set this flag to skip the offset on both the update image and the
> + * device so that the initial boot device portion is preserved. This
> + * is useful if a partition table or other data is in this area.
> + */
> +#define IMX_BBU_FLAG_KEEP_HEAD (1 << 16)

BIT(16) instead?

Thanks,
Andrey Smirnov

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

Re: [PATCH 2/2] ARM: i.MX bbu: support partitions starting at i.MX header

[Andrey Smirnov]

On Thu, Jun 28, 2018 at 11:50 PM Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> The i.MX header is at an offset in the boot device, usually 0x400 bytes.
> This patch adds a flag to support the case that the partition the image
> is written to starts at that offset rather than 0x0.
>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  arch/arm/mach-imx/imx-bbu-internal.c | 7 +++++--
>  arch/arm/mach-imx/include/mach/bbu.h | 8 ++++++++
>  2 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-imx/imx-bbu-internal.c b/arch/arm/mach-imx/imx-bbu-internal.c
> index 5422235b1b..23686e9d61 100644
> --- a/arch/arm/mach-imx/imx-bbu-internal.c
> +++ b/arch/arm/mach-imx/imx-bbu-internal.c
> @@ -58,12 +58,15 @@ static int imx_bbu_write_device(struct imx_internal_bbu_handler *imx_handler,
>         if (fd < 0)
>                 return fd;
>
> -       if (imx_handler->handler.flags & IMX_BBU_FLAG_KEEP_HEAD) {
> +       if (imx_handler->handler.flags & (IMX_BBU_FLAG_KEEP_HEAD |
> +           IMX_BBU_FLAG_PARTITION_STARTS_AT_HEADER)) {
>                 image_len -= imx_handler->flash_header_offset;
> -               offset += imx_handler->flash_header_offset;
>                 buf += imx_handler->flash_header_offset;
>         }
>
> +       if (imx_handler->handler.flags & IMX_BBU_FLAG_KEEP_HEAD)
> +               offset += imx_handler->flash_header_offset;
> +
>         if (imx_handler->flags & IMX_INTERNAL_FLAG_ERASE) {
>                 pr_debug("%s: unprotecting %s from 0x%08x to 0x%08x\n", __func__,
>                                 devicefile, offset, image_len);
> diff --git a/arch/arm/mach-imx/include/mach/bbu.h b/arch/arm/mach-imx/include/mach/bbu.h
> index c2e875d342..19b446ed60 100644
> --- a/arch/arm/mach-imx/include/mach/bbu.h
> +++ b/arch/arm/mach-imx/include/mach/bbu.h
> @@ -16,6 +16,14 @@ struct imx_dcd_v2_entry;
>   */
>  #define IMX_BBU_FLAG_KEEP_HEAD (1 << 16)
>
> +/*
> + * Set this flag when the partition the update image is written to
> + * actually starts at the offset where the i.MX flash header is expected
> + * (usually 0x400). This means for the update code that it has to skip
> + * the first 0x400 bytes of the image.
> + */
> +#define IMX_BBU_FLAG_PARTITION_STARTS_AT_HEADER        (1 << 17)
> +

BIT(17)?

Thanks,
Andrey Smirnov

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

Re: [PATCH 1/2] ARM: i.MX bbu: reimplement IMX_INTERNAL_FLAG_KEEP_DOSPART flag

[Sascha Hauer]

On Fri, Jun 29, 2018 at 11:28:14AM -0700, Andrey Smirnov wrote:
> On Thu, Jun 28, 2018 at 11:49 PM Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >
> > This patch reimplements the IMX_INTERNAL_FLAG_KEEP_DOSPART flag
> > and makes it more generic. Until now we only kept a dos partition
> > table over the update. Beginning with i.MX8 we may also want to
> > preserve a GPT, so we have to extend the preserved area.
> >
> > It might also be the case that not (only) a partition table is
> > stored in the initial area of a device, but also other unrelated
> > data, so it's better to just keep the initial area that is unused
> > by the i.MX ROM. It's also good to export the flag to allow boards
> > to specify the initial area shall be preserved.
> >
> > When a board wants to set the flag for a mtd like device then it
> > has to check for suitable erase sizes beforehand. We do not check
> > this (yet).
> >
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  arch/arm/mach-imx/imx-bbu-internal.c | 70 +++++++++-------------------
> >  arch/arm/mach-imx/include/mach/bbu.h | 15 ++++++
> >  2 files changed, 38 insertions(+), 47 deletions(-)
> >
> > diff --git a/arch/arm/mach-imx/imx-bbu-internal.c b/arch/arm/mach-imx/imx-bbu-internal.c
> > index 84810f18a9..5422235b1b 100644
> > --- a/arch/arm/mach-imx/imx-bbu-internal.c
> > +++ b/arch/arm/mach-imx/imx-bbu-internal.c
> > @@ -35,7 +35,6 @@
> >  #define FLASH_HEADER_OFFSET_MMC                0x400
> >
> >  #define IMX_INTERNAL_FLAG_NAND         (1 << 0)
> > -#define IMX_INTERNAL_FLAG_KEEP_DOSPART (1 << 1)
> >  #define IMX_INTERNAL_FLAG_ERASE                (1 << 2)
> >
> >  struct imx_internal_bbu_handler {
> > @@ -53,26 +52,31 @@ static int imx_bbu_write_device(struct imx_internal_bbu_handler *imx_handler,
> >                 const char *devicefile, struct bbu_data *data,
> >                 const void *buf, int image_len)
> >  {
> > -       int fd, ret;
> > -       int written = 0;
> > +       int fd, ret, offset = 0;
> >
> >         fd = open(devicefile, O_RDWR | O_CREAT);
> >         if (fd < 0)
> >                 return fd;
> >
> > +       if (imx_handler->handler.flags & IMX_BBU_FLAG_KEEP_HEAD) {
> > +               image_len -= imx_handler->flash_header_offset;
> > +               offset += imx_handler->flash_header_offset;
> > +               buf += imx_handler->flash_header_offset;
> > +       }
> > +
> >         if (imx_handler->flags & IMX_INTERNAL_FLAG_ERASE) {
> > -               pr_debug("%s: unprotecting %s from 0 to 0x%08x\n", __func__,
> > -                               devicefile, image_len);
> > -               ret = protect(fd, image_len, 0, 0);
> > +               pr_debug("%s: unprotecting %s from 0x%08x to 0x%08x\n", __func__,
> > +                               devicefile, offset, image_len);
> > +               ret = protect(fd, image_len, offset, 0);
> >                 if (ret && ret != -ENOSYS) {
> >                         pr_err("unprotecting %s failed with %s\n", devicefile,
> >                                         strerror(-ret));
> >                         goto err_close;
> >                 }
> >
> > -               pr_debug("%s: erasing %s from 0 to 0x%08x\n", __func__,
> > -                               devicefile, image_len);
> > -               ret = erase(fd, image_len, 0);
> > +               pr_debug("%s: erasing %s from 0x%08x to 0x%08x\n", __func__,
> > +                               devicefile, offset, image_len);
> > +               ret = erase(fd, image_len, offset);
> >                 if (ret) {
> >                         pr_err("erasing %s failed with %s\n", devicefile,
> >                                         strerror(-ret));
> > @@ -80,43 +84,14 @@ static int imx_bbu_write_device(struct imx_internal_bbu_handler *imx_handler,
> >                 }
> >         }
> >
> > -       if (imx_handler->flags & IMX_INTERNAL_FLAG_KEEP_DOSPART) {
> > -               void *mbr = xzalloc(512);
> > -
> > -               pr_debug("%s: reading DOS partition table in order to keep it\n", __func__);
> > -
> > -               ret = read(fd, mbr, 512);
> > -               if (ret < 0) {
> > -                       free(mbr);
> > -                       goto err_close;
> > -               }
> > -
> > -               memcpy(mbr, buf, 0x1b8);
> > -
> > -               ret = lseek(fd, 0, SEEK_SET);
> > -               if (ret) {
> > -                       free(mbr);
> > -                       goto err_close;
> > -               }
> > -
> > -               ret = write(fd, mbr, 512);
> > -
> > -               free(mbr);
> > -
> > -               if (ret < 0)
> > -                       goto err_close;
> > -
> > -               written = 512;
> > -       }
> > -
> > -       ret = write(fd, buf + written, image_len - written);
> > +       ret = pwrite(fd, buf, image_len, offset);
> >         if (ret < 0)
> >                 goto err_close;
> >
> >         if (imx_handler->flags & IMX_INTERNAL_FLAG_ERASE) {
> > -               pr_debug("%s: protecting %s from 0 to 0x%08x\n", __func__,
> > -                               devicefile, image_len);
> > -               ret = protect(fd, image_len, 0, 1);
> > +               pr_debug("%s: protecting %s from 0x%08x to 0x%08x\n", __func__,
> > +                               devicefile, offset, image_len);
> > +               ret = protect(fd, image_len, offset, 1);
> >                 if (ret && ret != -ENOSYS) {
> >                         pr_err("protecting %s failed with %s\n", devicefile,
> >                                         strerror(-ret));
> > @@ -454,6 +429,7 @@ static struct imx_internal_bbu_handler *__init_handler(const char *name, char *d
> >         struct bbu_handler *handler;
> >
> >         imx_handler = xzalloc(sizeof(*imx_handler));
> > +       imx_handler->flags = flags & IMX_BBU_FLAG_MASK;
> 
> I am not sure I understand why this is necessary. You can already
> access all of the IMX_BBU flags via imx_handler->handler.flags (which
> is exactly what some of your code above does) and this forces all of
> those "=" -> "|=" replacements below. Do we really need to copy one
> set of flags into another?
> 
> Taking a step back, if we are reserving some of the BBU flags for
> internal usage, can't we just drop the notion of internal flags
> altogether and just use imx_handler->handler.flags for everything
> including IMX_INTERNAL_FLAG_ERASE and IMX_INTERNAL_FLAG_NAND?

You are right. I implemented this, see v2 of this series.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 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