[PATCH 1/3] cmd: env: Add 'env import -h' for Marvell hw_info formatted environments

[Tom Rini]

On Fri, Feb 05, 2021 at 11:36:25PM +0100, Luka Kovacic wrote:
> On Fri, Feb 5, 2021 at 11:04 PM Tom Rini <trini at konsulko.com> wrote:
> >
> > On Fri, Feb 05, 2021 at 11:02:23PM +0100, Luka Kovacic wrote:
> > > Hello Tom,
> > >
> > > On Thu, Feb 4, 2021 at 6:06 PM Tom Rini <trini at konsulko.com> wrote:
> > > >
> > > > On Thu, Feb 04, 2021 at 11:46:35AM +0100, Luka Kovacic wrote:
> > > >
> > > > > The '-h' flag is added to the 'env import' command to enable parsing
> > > > > Marvell hw_info formatted environments.
> > > > > This format is often used on Marvell Armada A37XX based devices to store
> > > > > parameters like the board serial number, factory MAC addresses and some
> > > > > other information.
> > > > >
> > > > > Currently this environment format can only be imported, not exported.
> > > > > These parameters are usually written to the flash in the factory.
> > > > >
> > > > > This functionality has been tested on the GST ESPRESSOBin-Ultra board
> > > > > successfully.
> > > > >
> > > > > Usage example:
> > > > >  => sf probe
> > > > >  => sf read ${loadaddr} 0x003E000A 0x1F0 # Read the environment from
> > > > > SPI flash
> > > > >  => env import -h ${loadaddr}
> > > > >
> > > > > Signed-off-by: Luka Kovacic <luka.kovacic at sartura.hr>
> > > > > Cc: Luka Perkov <luka.perkov at sartura.hr>
> > > > > Cc: Robert Marko <robert.marko at sartura.hr>
> > > >
> > > > So, the implementation itself is fine:
> > > >
> > > > Reviewed-by: Tom Rini <trini at konsulko.com>
> > > >
> > > > And I take it as a given that we can't get the boards populated with an
> > > > existing separator, so we need to solve this somehow or another.  I am
> > > > however loathe to increase every platform by a handful of bytes (help
> > > > text, code text) to cover this case, and adding #ifdef around the help
> > > > text in particular would be very ugly. Is there any clean way to write
> > > > this as a board-specific command?  I assume the overall intention is to
> > > > import the factory env for the required information, store it in regular
> > > > U-Boot environment and then never touch the factory area again.  Thanks!
> > >
> > > I have already considered the possibility of implementing this in a
> > > board-specific way, but I tried implementing it here to avoid unnecessary
> > > code duplication.
> > > You are correct, the intention is to use this functionality as a migration
> > > path. It's good to keep the factory area intact, in case it's still needed in
> > > the future.
> > > The Marvell hw_info area is limited to MACs and serial numbers by the
> > > stock utility.
> >
> > OK.  And can we put this in the board-specific path somehow, relatively
> > cleanly?
> 
> I've been thinking about the following options:
>  - Implementing a separate command under cmd/mvebu (hw_info), like done
> in the stock U-Boot.
> I'm not inclined to this one, as I don't see real utility in updating
> this factory
> area. A positive with this one is that it abstracts reading of the SPI flash.
> 
>  - No command, automatic migration, directly from the SPI flash (implemented
> in the board.c file.
> I don't see this as a clean way of doing it, the Armada A37XX board.c is
> already quite full of board-specific init.
> 
>  - Implemented as is, but with #ifdef
> This would break CONFIG_CMD_IMPORTENV into smaller pieces.
> #ifdef wouldn't look too bad around the code, only the inline string changes
> would be ugly.
> 
> What would you recommend?

I hate to say it but I prefer the first, especially if it ends up being
like 50 lines of code or whatever to write the function and all that.
Or the second option, if the first option really ends up being messy
once you write it and the second one less messy.  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210205/dba84c6e/attachment.sig>