[U-Boot] [PATCH v2 04/11] drivers:dfu: new feature: separated bootloader alt setting

[Przemyslaw Marczak]

Hello,

On 06/17/2014 06:36 PM, Stephen Warren wrote:
> On 06/17/2014 04:20 AM, Przemyslaw Marczak wrote:
>> Hello Stephen,
>>
>> On 06/16/2014 09:52 PM, Stephen Warren wrote:
>>> On 06/12/2014 03:46 AM, Przemyslaw Marczak wrote:
>>>> This patch introduces new feature: initialization of the dfu
>>>> bootloader entity from a separate environmental variable which
>>>> can be set on a boot time.
>>>>
>>>> By default, DFU always check environmental variable: $dfu_alt_info.
>>>>
>>>> Changes:
>>>> - DFU will also look for environmental variable: $dfu_alt_bootloader
>>>> - if any of dfu_alt_* variable is properly defined, then function
>>>>     dfu_init_env_entities() will return success.
>>>>
>>>> Use case:
>>>> Some devices can boot from various media type (SD, eMMC, NAND, etc.)
>>>> or some board configs are common for more than one board type.
>>>> In a such case, bootloader is probably placed on a different
>>>> devices or even offsets. So such DFU feature is welcome.
>>>
>>> Why should the "dfu" command look at different environment variables?
>>> Whatever code dynamically sets the value of $dfu_alt_bootloader should
>>> simply set $dfu_alt_info instead.
>>
>> Dynamically setting of any entity cold be done at boot time, like this:
>>
>> # int buf_len = strlen(alt_bootloader) + strlen(CONFIG_DFU_ALT) + 4;
>> # char *buf = memalign(1, buf_len);
>> #
>> # sprintf(buf, "%s; %s", alt_bootloader, CONFIG_DFU_ALT);
>
> Note that you can't have a space after the ; due to the use of strsep().
> Switching to strtok() might solve that.
>

Yes - this wasn't tested - just some pseudo code.

>> # setenv("dfu_alt_info", buf);
>>
>> But overwriting the $dfu_alt_info on each boot is not a good idea.
>> If user modify the dfu entities - then it will be overwritten at next boot.
>
> What if the user sees that $dfu_alt_bootloader is set, and modifies that
> without realizing that it's an auto-generated variable? The same thing
> then applies.
>
> Perhaps we need a standard where system-/automatically-set variables are
> named with a auto_ prefix or _auto suffix or something, to make this clear?

Yes, right note.

>
> I'd prefer that the dfu command didn't use any environment variables,
> but rather required the user to always pass the list on the
> command-line. Then, the user could invoke either:
>
> dfu "foo mmc x..." # Manually specified
> dfu $dfu_alt_info # Use 'user-defined' variable
> dfu $dfu_alt_bootloaer # Use 'system-defined' variable

Yes, definitely such feature was missing there.

>
> That way, the code doesn't have to loop over a bunch of variables and
> get more complex. Implicitly depending on environment variables make it
> hard to tell what a sequence of commands will do.
>
> ...
>> Maybe better could be modification of the function
>> dfu_init_env_entities() to support parsing variables
>> in the $dfu_alt_info instead of hard coded env variables
>> names, e.g:
>>
>> dfu_alt_info="${alt_info_boot}, ${alt_info_system},..."
>
> I feel like the shell already has the capability to interpolate variable
> values into strings, so this would be quite easy to do in the shell
> rather than duplicating that code inside the dfu command.
>

Every env macro passed with cmdline will be expanded. And then, we can 
use such style like this:

# setenv alt_kernel "uImage ext4 0 2;zImage ext4 0 2"
# setenv alt_system "boot part 0 2;root part 0 3"
# setenv auto_alt_bootloader "u-boot raw 0x0 0x800"
# setenv alt_info "${alt_kernel};${alt_system};${auto_alt_bootloader}"
(this will expand when passing to "setenv")

or just put this in env default config:

"alt_info=${alt_kernel};${alt_system};${auto_alt_bootloader}\0" ...

So summarizing, I don't want to break your DFU rework, I want just to 
add the Odroid U3 support, so in the next patch set I will use the 
$dfu_alt_info, instead of combining with a short time solution.

And after your work will be done, then I will update Odroid code.

Best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com