[U-Boot] [PATCH 3/3] tests: py: dfu: Provide functionality to set test and dummy files alt settings

[Lukasz Majewski]

Hi Stephen,

> On 04/11/2016 02:42 AM, Lukasz Majewski wrote:
> > Hi Stephen,
> >
> >> On 04/08/2016 09:44 AM, Lukasz Majewski wrote:
> >>> After concatenation of "dfu_alt_info" variable from "dfu_alt_boot"
> >>> and "dfu_alt_system" it may happen that test and dummy files alt
> >>> settings are different than default 0 and 1.
> >>>
> >>> This patch provides ability to set different values for them.
> >>> It was the simplest possible solution - akin to the one from
> >>> original bash dfu tests.
> >>
> >>> diff --git a/test/py/tests/test_dfu.py b/test/py/tests/test_dfu.py
> >>
> >>> +        # - after concatenation dfu alt settings for test and
> >>> dummy files are
> >>> +        #   moved from 0 and 1 to other values
> >>
> >> Similar formatting comments to the previous patch. I'd also re-word
> >> this to be much more generic, and simply state the it allows
> >> different alt settings to be used, rather than tieing the
> >> description to one possible reason why you might want to do that.
> >
> > Ok, I will rewrite the description.
> >
> >>
> >>> +        "alt_num_test_file": "5",
> >>> +        "alt_num_dummy_file": "6",
> >>
> >> This feels fragile. What if $dfu_alt_boot changes length? Does it
> >> make more sense to:
> >>
> >> (a) Set alt_info_env_name to dfu_alt_boot instead, so that the
> >> settings specified by the test are always at a known position in
> >> the list, so we can always use alt setting 0 and 1.
> >
> > Unfortunately, dfu_alt_boot (which depends on boot medium) is
> > immutable and set during boot time from CONFIG_DFU_ALT_BOOT_SD and
> > CONFIG_DFU_ALT_BOOT_EMMC
> >
> > Only "dfu_alt_system" can be set by user.
> 
> I don't see anywhere in the code to enforce that. While dfu_alt_boot 
> does appear to be set at boot based on CONFIG_DFU_ALT_BOOT_*, there 
> doesn't appear to be anything that forces the variable to be
> read-only.

I've double checked generation of dfu_alt_boot.

You can change its value at u-boot's prompt. 

ODROID-XU3 # setenv dfu_alt_boot "AABBCCDD"
ODROID-XU3 # printenv dfu_alt_boot
dfu_alt_boot=AABBCCDD

ODROID-XU3 # dfu 0 mmc 0 list
DFU alt info setting: done
DFU alt settings list:
dev: eMMC alt: 0 name: u-boot layout: RAW_ADDR
dev: eMMC alt: 1 name: bl1 layout: RAW_ADDR
dev: eMMC alt: 2 name: bl2 layout: RAW_ADDR
dev: eMMC alt: 3 name: tzsw layout: RAW_ADDR
dev: eMMC alt: 4 name: params.bin layout: RAW_ADDR


Unfortunately it is re-generated from CONFIG_DFU_ALT_BOOT_* each time I
run "dfu 0 mmc 0" command.

ODROID-XU3 printenv dfu_alt_boot
dfu_alt_boot=u-boot raw 0x3f 0x800;bl1 raw 0x1 0x1e;bl2 raw 0x1f
0x1d;tzsw raw 0x83f 0x200;params.bin raw 0x1880 0x20

> 
> >> or:
> >>
> >> (b) Use names rather than numbers for the alt setting?
> >
> > I thought about this option.
> >
> > 1. One possible solution would be to define:
> >
> > "test_file_name": "file.bin"
> > "dummy_file_name": "dummy.bin"
> >
> > at env__dfu_configs.
> >
> > Afterwards, I could automatically set the "alt_info" property int
> > the same map:
> >
> > "alt_info" : "%s ext4 0 2; %s ext4 0 2" % (test_file_name,
> > dummy_file_name)
> 
> I assume you're using the % operator here so that test_file_name can
> be shared with the other config options that define the alt setting 
> names/IDs. That won't work exactly as you've written it, since 
> "test_file_name" isn't a reference to the env__dfu_configs variable,
> and indeed that variable can't be accessed at that location in the
> code. I think you'd need something like:
> 
> test_file_name = "file.bin"
> dummy_file_name = "dummy.bin"
> 
> env__dfu_configs = (
>      {
>          "fixture_id": "emmc",
> ...
>          test_alt_id: test_file_name,
>          dummy_alt_id: dummy_file_name,
>          "alt_info" : "%s ext4 0 2; %s ext4 0 2" % (
>              test_file_name, dummy_file_name)
> 
>      },
> )
> 

Yes, this is the solution I was trying to pursue :-)

> > As a result, I could use -a {test_file_name|dummy_file_name} to
> > access proper alt setting.
> >
> >
> >
> > 2. Another option would be to set the "dfu_alt_system" env variable
> > and then run "dfu-util -l" on host (or 'dfu 0 mmc 0 list') on
> > target and grep output for test_file_name and dummy_file_name and
> > extract alt setting numbers.
> > This would require modification in the framework core code and
> > hence I decided to manually specify the alt settings number (as I
> > did in the bash version of those scripts).
> >
> > However, as I look now for it, I think that option from point 1)
> > seems flexible enough. Stephen what do you think about it?
> 
> Option 1 does seem simplest.

Ok.

> 
> >> Those should
> >> be position-independent. Presumably this would require a slightly
> >> large code change, since we'd need to move from %d to %s
> >> conversions when constructing the dfu command string, but that
> >> should be very easy.
> >>
> >> If you take this approach, I'd suggest making the configuration
> >> file name (alt_num_*_file above) match the Python variable name
> >> (alt_setting_*_file) for consistency.
> >>
> >> (c) Provide a way for the user to turn off the auto-concatenation
> >> feature.
> >
> > This feature is already deployed and I would like to avoid changing
> > it.
> 
> I wasn't suggesting removing it, rather making it read another 
> environment variable that allows disabling the feature at run-time. 
> That'd be something like the following at the start of the function
> that implements it:
> 
> if (getenv("dfu_alt_disable_auto_generation"))
>      return;
> 

Thanks for tip. However, I think that option described by you at point
1 is the way to go (I wouldn't need to modify u-boot and only extend
python code).

> 

Anyway thanks for support.

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group