Re: [PATCH v2 4/4] ARM: i.MX: Add support for ZII RDU1 board

From: Lucas Stach <l.stach@pengutronix.de>
To: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: nikita.yoush@cogentembedded.com, barebox@lists.infradead.org,
	cphealy@gmail.com
Subject: Re: [PATCH v2 4/4] ARM: i.MX: Add support for ZII RDU1 board
Date: Mon, 25 Jun 2018 18:30:07 +0200	[thread overview]
Message-ID: <1529944207.9910.29.camel@pengutronix.de> (raw)
In-Reply-To: <CAHQ1cqEMipPPqTvVFPO4YYCB7ojNmh8Jq6Lcem8e7UMrKU83mQ@mail.gmail.com>

Am Montag, den 25.06.2018, 09:11 -0700 schrieb Andrey Smirnov:
[...]
> > > +
> > > +/**
> > > + * zii_rdu1_bbu_spi_update - RDU1 specific BBU handler
> > > + *
> > > > + * @handler:       BBU handler pointer passed down by BBU framework
> > > > + * @data:  BBU data pointer passed down by BBU framework
> > > 
> > > + *
> > > + * RDU1 design chose to use first page of the onboard dataflash to
> > > + * store vendor board-specific paramters, which is problematic because
> > > + * imx_bbu_internal_v1_update() will not spare that region when
> > > + * peforming the update.
> > > + *
> > > + * This functions works as a thin wrapper around
> > > + * imx_bbu_internal_v1_update() and saves the data before and restores
> > > + * it after, keeping board specific data intact.
> > > + */
> > 
> > In general I like the idea of replacing the update handler in the
> > board, as it shows clearly that this is some board specific quirk.
> > 
> > I don't fully agree on the implementation of the handler though.
> > Overwriting and restoring the config area still has the risk of
> > destroying this data, leaving the unit in a bricked state.
> > 
> 
> The only info contained in config area is unit's MAC address and and
> LCD panel type. Loosing this information, while inconvenient, is not
> going to brick the unit. Additionally:
> 
> -  LCD type information could and is tentatively planed to be derived
> from P/N information available via RAVE SP EEPROM, so that leaves us
> with just MAC address.
> 
> - Config header in SPI NOR is also duplicated in SPI EEPROM connected
> to i.MX51, so it can be resorted from there if need be.
> 
> - Removing this risk will do nothing to the risk of truly bricking the
> unit via failed bootloader upgrade which is several orders of
> magnitude more likely due to size difference of bootloader image vs
> config area.

Yea, I thought there was some more valuable thing stored in this area.

> > As the config area is page aligned, there is no need to touch it at all
> > (I think). All you need to do is to point the BBU target at the barebox
> > partition on the dataflash and truncate the barebox image in the custom
> > handler by changing data->image and data->len in the custom RDU1
> > handler.
> > 
> 
> I'll give this approach a try since it sounds like it's going to allow
> me to drop a bit of extra code, but if it doesn't I think the original
> code is good enough and we should let the perfect be the enemy of the
> good.

Agreed. I won't object to the current approach if the simpler option
turns out to not work for some reason.

Regards,
Lucas

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