| Message ID | 1402566394-23342-4-git-send-email-p.marczak@samsung.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Minkyu Kang |
| Headers | show |
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.
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); # 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. In the other side I can store entities as $dfu_alt_default and change the above code to: # char *alt_default = getenv("dfu_alt_default"); # if (!alt_default) # alt_default = ""; # # int buf_len = strlen(alt_bootloader) + strlen(alt_default) + 4; # char *buf = memalign(1, buf_len); # # sprintf(buf, "%s; %s", alt_bootloader, alt_default); # setenv("dfu_alt_info", buf); But then, there is some mess in the environment because of duplicated default alt_info. Those both, above solutions takes more time than just one simple line: # setenv("dfu_alt_bootlaoder", CONFIG_SOME_ALT_INFO); like in this patch set. 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},..." dfu_alt_info could be set with default variables names in each board config file in the CONFIG_EXTRA_ENV_SETTINGS and then just one proper variable could be set at boot, and others from env - simple and fast. And then in the dfu init code - entities are initialized from env variables - if they exists, like in the loop code from this patch. I need some solution to automatically set proper bootloader entities, since one binary can be stored on SD and eMMC cards. Thank you,
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. > # 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? 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 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.
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,
On 06/18/2014 04:56 AM, Przemyslaw Marczak wrote: > On 06/17/2014 06:36 PM, Stephen Warren wrote: ... >> 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. ... > 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. Which rework are you referring to? I'm not actively working on changing the command-line parameters to the dfu command in any way. I've certainly discussed how I'd prefer the dfu command to work, but I don't have time to actually implement that. So, the existing command-line format is likely to stay as it is for now.
On 06/18/2014 05:46 PM, Stephen Warren wrote: > On 06/18/2014 04:56 AM, Przemyslaw Marczak wrote: >> On 06/17/2014 06:36 PM, Stephen Warren wrote: > ... >>> 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. > ... >> 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. > > Which rework are you referring to? I'm not actively working on changing > the command-line parameters to the dfu command in any way. I've > certainly discussed how I'd prefer the dfu command to work, but I don't > have time to actually implement that. So, the existing command-line > format is likely to stay as it is for now. > Ah ok, my mistake. Anyway I will not touch this code at this time. Regards,
diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c index a938109..8848624 100644 --- a/drivers/dfu/dfu.c +++ b/drivers/dfu/dfu.c @@ -44,24 +44,32 @@ static int dfu_find_alt_num(const char *s) int dfu_init_env_entities(char *interface, int dev) { + const char *alt_info[] = {"dfu_alt_info", "dfu_alt_bootloader"}; const char *str_env; char *env_bkp; - int ret; + int ret, i; + int alt_init_cnt = 0; + + for (i = 0; i < ARRAY_SIZE(alt_info); i++) { + str_env = getenv(alt_info[i]); + if (!str_env) + continue; - str_env = getenv("dfu_alt_info"); - if (!str_env) { - error("\"dfu_alt_info\" env variable not defined!\n"); - return -EINVAL; + env_bkp = strdup(str_env); + ret = dfu_config_entities(env_bkp, interface, dev); + free(env_bkp); + + if (ret) + continue; + + alt_init_cnt++; } - env_bkp = strdup(str_env); - ret = dfu_config_entities(env_bkp, interface, dev); - if (ret) { + if (!alt_init_cnt) { error("DFU entities configuration failed!\n"); - return ret; + return -1; } - free(env_bkp); return 0; }
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. Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com> --- Changes v2: - new commit --- drivers/dfu/dfu.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-)