From mboxrd@z Thu Jan 1 00:00:00 1970 From: Przemyslaw Marczak Date: Wed, 18 Jun 2014 12:56:56 +0200 Subject: [U-Boot] [PATCH v2 04/11] drivers:dfu: new feature: separated bootloader alt setting In-Reply-To: <53A06E70.50103@wwwdotorg.org> References: <1402399510-8965-1-git-send-email-p.marczak@samsung.com> <1402566394-23342-1-git-send-email-p.marczak@samsung.com> <1402566394-23342-4-git-send-email-p.marczak@samsung.com> <539F4B18.2040406@wwwdotorg.org> <53A01654.1050407@samsung.com> <53A06E70.50103@wwwdotorg.org> Message-ID: <53A17078.1050200@samsung.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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