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@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?
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@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
