From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mail-io0-x243.google.com ([2607:f8b0:4001:c06::243]) by bombadil.infradead.org with esmtps (Exim 4.90_1 #2 (Red Hat Linux)) id 1fYy8L-0003Yz-My for barebox@lists.infradead.org; Fri, 29 Jun 2018 18:28:39 +0000 Received: by mail-io0-x243.google.com with SMTP id e13-v6so9270892iof.6 for ; Fri, 29 Jun 2018 11:28:26 -0700 (PDT) MIME-Version: 1.0 References: <20180629064914.518-1-s.hauer@pengutronix.de> In-Reply-To: <20180629064914.518-1-s.hauer@pengutronix.de> From: Andrey Smirnov Date: Fri, 29 Jun 2018 11:28:14 -0700 Message-ID: List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "barebox" Errors-To: barebox-bounces+u.kleine-koenig=pengutronix.de@lists.infradead.org Subject: Re: [PATCH 1/2] ARM: i.MX bbu: reimplement IMX_INTERNAL_FLAG_KEEP_DOSPART flag To: Sascha Hauer Cc: Barebox List On Thu, Jun 28, 2018 at 11:49 PM Sascha Hauer 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 > --- > 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