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

[Stefan Roese]

On 05.02.21 23:51, Luka Kovacic wrote:
> 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.

Ok. Please rebase on latest mainline and send v2 once it's ready.

Thanks,
Stefan