| Message ID | 1460130291-24223-2-git-send-email-l.majewski@samsung.com |
|---|---|
| State | Superseded |
| Delegated to: | Łukasz Majewski |
| Headers | show |
On 04/08/2016 09:44 AM, Lukasz Majewski wrote: > By default (on almost all systems) the dfu env variable, which defines > available alt settings, is named as "dfu_alt_info". > > However on some platforms (i.e. Odroid XU3), the 'dfu_alt_info' is concatenated > from other variables - namely 'dfu_alt_boot' and 'dfu_alt_system' at run time > (when one types 'dfu 0 mmc 0' for first time). > > 'dfu_alt_boot' describes alt settings which depend on boot medium - for example > boot loader's LBA sectors which are different on eMMC and SD card because of e.g. > MBR/GPT. > > 'dfu_alt_system' describes board agnostic alt settings - like rootfs, kernel. > On such system we can only append/modify this env variable. > > Because of the above, we must have way to modify other than "dfu_ale_info" > variable to perform tests. Uggh. That's a pretty nasty user-experience since it prevents the user from controlling what happens. U-Boot should never over-ride what the user requests for alt_info, but rather simply provide help to the user allowing them to make use of the system-supplied information if they want. I.e. dfu_alt_boot should be set at boot time based on code. The user should then be free to define dfu_alt_info as they see fit; either hard-coded, or including the value of $dfu_alt_boot /if/ they want. Anyway, I suppose since this feature exists, we have no choice but to support it. > diff --git a/test/py/tests/test_dfu.py b/test/py/tests/test_dfu.py > index df3b561..1ed6020 100644 > --- a/test/py/tests/test_dfu.py > +++ b/test/py/tests/test_dfu.py > @@ -44,6 +44,11 @@ env__dfu_configs = ( > # configurations, but don't want to test every single transfer size > # on each, to avoid bloating the overall time taken by testing. > "test_sizes": (63, 64, 65), > + # Optional values. > + # Those values are necessary on boards like Odroid XU3, where > + # - dfu alt info env variable is concatenated from boot medium dependent > + # (dfu_alt_boot) and user defined (dfu_alt_system) variables > + "alt_info_env_name": "dfu_alt_system", I think the "- " and leading space should be dropped; this isn't a list of items. I'd suggest re-phrasing this as: # The name of the environment variable that the the dfu command reads # alt info from. If unspecified, this defaults to dfu_alt_info, which is # valid for most systems. Some systems use a different variable name. # One example is the Odroid XU3, which automatically generates # $dfu_alt_info, each time the dfu command is run, by concatenating # $dfu_alt_boot and $dfu_alt_system. > }, > ) > > @@ -124,7 +129,12 @@ def test_dfu(u_boot_console, env__usb_dev_port, env__dfu_config): > u_boot_console.log.action( > 'Starting long-running U-Boot dfu shell command') > > - cmd = 'setenv dfu_alt_info "%s"' % env__dfu_config['alt_info'] > + dfu_alt_info_env = "dfu_alt_info" > + if "alt_info_env_name" in env__dfu_config: > + dfu_alt_info_env = env__dfu_config['alt_info_env_name'] That can be just: dfu_alt_info_env = env__dfu_config.get('alt_info_env_name', \ 'dfu_alt_info') > + > + cmd = 'setenv "%s" "%s"' % (dfu_alt_info_env, > + env__dfu_config['alt_info']) > u_boot_console.run_command(cmd) > > cmd = 'dfu 0 ' + env__dfu_config['cmd_params']
On 04/08/2016 09:44 AM, Lukasz Majewski wrote: > By default (on almost all systems) the dfu env variable, which defines > available alt settings, is named as "dfu_alt_info". > > However on some platforms (i.e. Odroid XU3), the 'dfu_alt_info' is concatenated > from other variables - namely 'dfu_alt_boot' and 'dfu_alt_system' at run time > (when one types 'dfu 0 mmc 0' for first time). > > 'dfu_alt_boot' describes alt settings which depend on boot medium - for example > boot loader's LBA sectors which are different on eMMC and SD card because of e.g. > MBR/GPT. > > 'dfu_alt_system' describes board agnostic alt settings - like rootfs, kernel. > On such system we can only append/modify this env variable. > > Because of the above, we must have way to modify other than "dfu_ale_info" > variable to perform tests. > diff --git a/test/py/tests/test_dfu.py b/test/py/tests/test_dfu.py > + # Optional values. > + # Those values are necessary on boards like Odroid XU3, where > + # - dfu alt info env variable is concatenated from boot medium dependent > + # (dfu_alt_boot) and user defined (dfu_alt_system) variables > + "alt_info_env_name": "dfu_alt_system", The previous value is also optional. I'd suggest simply prefixing each optional value with the same text as already used: # This value is optional.
On Fri, 8 Apr 2016 10:28:06 -0600 Stephen Warren <swarren@wwwdotorg.org> wrote: > On 04/08/2016 09:44 AM, Lukasz Majewski wrote: > > By default (on almost all systems) the dfu env variable, which > > defines available alt settings, is named as "dfu_alt_info". > > > > However on some platforms (i.e. Odroid XU3), the 'dfu_alt_info' is > > concatenated from other variables - namely 'dfu_alt_boot' and > > 'dfu_alt_system' at run time (when one types 'dfu 0 mmc 0' for > > first time). > > > > 'dfu_alt_boot' describes alt settings which depend on boot medium - > > for example boot loader's LBA sectors which are different on eMMC > > and SD card because of e.g. MBR/GPT. > > > > 'dfu_alt_system' describes board agnostic alt settings - like > > rootfs, kernel. On such system we can only append/modify this env > > variable. > > > > Because of the above, we must have way to modify other than > > "dfu_ale_info" variable to perform tests. > > Uggh. That's a pretty nasty user-experience since it prevents the > user from controlling what happens. U-Boot should never over-ride > what the user requests for alt_info, but rather simply provide help > to the user allowing them to make use of the system-supplied > information if they want. I.e. dfu_alt_boot should be set at boot > time based on code. The user should then be free to define > dfu_alt_info as they see fit; either hard-coded, or including the > value of $dfu_alt_boot /if/ they want. > > Anyway, I suppose since this feature exists, we have no choice but to > support it. This indeed could be done a bit differently. > > > diff --git a/test/py/tests/test_dfu.py b/test/py/tests/test_dfu.py > > index df3b561..1ed6020 100644 > > --- a/test/py/tests/test_dfu.py > > +++ b/test/py/tests/test_dfu.py > > @@ -44,6 +44,11 @@ env__dfu_configs = ( > > # configurations, but don't want to test every single > > transfer size # on each, to avoid bloating the overall time taken > > by testing. "test_sizes": (63, 64, 65), > > + # Optional values. > > + # Those values are necessary on boards like Odroid XU3, > > where > > + # - dfu alt info env variable is concatenated from boot > > medium dependent > > + # (dfu_alt_boot) and user defined (dfu_alt_system) > > variables > > + "alt_info_env_name": "dfu_alt_system", > > I think the "- " and leading space should be dropped; this isn't a > list of items. I'd suggest re-phrasing this as: > > # The name of the environment variable that the the dfu command reads > # alt info from. If unspecified, this defaults to dfu_alt_info, which > is # valid for most systems. Some systems use a different variable > name. # One example is the Odroid XU3, which automatically generates > # $dfu_alt_info, each time the dfu command is run, by concatenating > # $dfu_alt_boot and $dfu_alt_system. +1 > > > }, > > ) > > > > @@ -124,7 +129,12 @@ def test_dfu(u_boot_console, > > env__usb_dev_port, env__dfu_config): u_boot_console.log.action( > > 'Starting long-running U-Boot dfu shell command') > > > > - cmd = 'setenv dfu_alt_info "%s"' % > > env__dfu_config['alt_info'] > > + dfu_alt_info_env = "dfu_alt_info" > > + if "alt_info_env_name" in env__dfu_config: > > + dfu_alt_info_env = > > env__dfu_config['alt_info_env_name'] > > That can be just: > dfu_alt_info_env = env__dfu_config.get('alt_info_env_name', \ > 'dfu_alt_info') This is the idiom which I was looking for :-). Thanks for hint. I will rewrite it in such way. > > > + > > + cmd = 'setenv "%s" "%s"' % (dfu_alt_info_env, > > + env__dfu_config['alt_info']) > > u_boot_console.run_command(cmd) > > > > cmd = 'dfu 0 ' + env__dfu_config['cmd_params'] > >
On Fri, 8 Apr 2016 10:29:31 -0600 Stephen Warren <swarren@wwwdotorg.org> wrote: > On 04/08/2016 09:44 AM, Lukasz Majewski wrote: > > By default (on almost all systems) the dfu env variable, which > > defines available alt settings, is named as "dfu_alt_info". > > > > However on some platforms (i.e. Odroid XU3), the 'dfu_alt_info' is > > concatenated from other variables - namely 'dfu_alt_boot' and > > 'dfu_alt_system' at run time (when one types 'dfu 0 mmc 0' for > > first time). > > > > 'dfu_alt_boot' describes alt settings which depend on boot medium - > > for example boot loader's LBA sectors which are different on eMMC > > and SD card because of e.g. MBR/GPT. > > > > 'dfu_alt_system' describes board agnostic alt settings - like > > rootfs, kernel. On such system we can only append/modify this env > > variable. > > > > Because of the above, we must have way to modify other than > > "dfu_ale_info" variable to perform tests. > > > diff --git a/test/py/tests/test_dfu.py b/test/py/tests/test_dfu.py > > > + # Optional values. > > + # Those values are necessary on boards like Odroid XU3, > > where > > + # - dfu alt info env variable is concatenated from boot > > medium dependent > > + # (dfu_alt_boot) and user defined (dfu_alt_system) > > variables > > + "alt_info_env_name": "dfu_alt_system", > > The previous value is also optional. I'd suggest simply prefixing > each optional value with the same text as already used: > > # This value is optional. > Ok, no problem. Best regards, Łukasz Majewski
diff --git a/test/py/tests/test_dfu.py b/test/py/tests/test_dfu.py index df3b561..1ed6020 100644 --- a/test/py/tests/test_dfu.py +++ b/test/py/tests/test_dfu.py @@ -44,6 +44,11 @@ env__dfu_configs = ( # configurations, but don't want to test every single transfer size # on each, to avoid bloating the overall time taken by testing. "test_sizes": (63, 64, 65), + # Optional values. + # Those values are necessary on boards like Odroid XU3, where + # - dfu alt info env variable is concatenated from boot medium dependent + # (dfu_alt_boot) and user defined (dfu_alt_system) variables + "alt_info_env_name": "dfu_alt_system", }, ) @@ -124,7 +129,12 @@ def test_dfu(u_boot_console, env__usb_dev_port, env__dfu_config): u_boot_console.log.action( 'Starting long-running U-Boot dfu shell command') - cmd = 'setenv dfu_alt_info "%s"' % env__dfu_config['alt_info'] + dfu_alt_info_env = "dfu_alt_info" + if "alt_info_env_name" in env__dfu_config: + dfu_alt_info_env = env__dfu_config['alt_info_env_name'] + + cmd = 'setenv "%s" "%s"' % (dfu_alt_info_env, + env__dfu_config['alt_info']) u_boot_console.run_command(cmd) cmd = 'dfu 0 ' + env__dfu_config['cmd_params']
By default (on almost all systems) the dfu env variable, which defines available alt settings, is named as "dfu_alt_info". However on some platforms (i.e. Odroid XU3), the 'dfu_alt_info' is concatenated from other variables - namely 'dfu_alt_boot' and 'dfu_alt_system' at run time (when one types 'dfu 0 mmc 0' for first time). 'dfu_alt_boot' describes alt settings which depend on boot medium - for example boot loader's LBA sectors which are different on eMMC and SD card because of e.g. MBR/GPT. 'dfu_alt_system' describes board agnostic alt settings - like rootfs, kernel. On such system we can only append/modify this env variable. Because of the above, we must have way to modify other than "dfu_ale_info" variable to perform tests. Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> --- test/py/tests/test_dfu.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)