On Thu, Jun 28, 2018 at 11:49 PM Sascha Hauer <s.hauer@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
