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

[Andrey Smirnov <andrew.smirnov@gmail.com>]

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