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

[Luka Kovacic]

On Fri, Feb 5, 2021 at 11:40 PM Tom Rini <trini at konsulko.com> wrote:
>
> 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!

Okay, I agree.

I think the first option should end up cleaner than both other options.
I'll do it this way.

>
> --
> Tom
Kind regards,
Luka