Hi Wolfgang,
You wrote:
> Dear Bartlomiej Sieka,
>
> In message <[EMAIL PROTECTED]> you wrote:
>> The auto-update feature allows to automatically download software updates
>> from a TFTP server and store them in Flash memory during boot. Updates are
>> contained in a FIT file and protected with SHA-1 checksum.
>>
>> More detailed description can be found in doc/README.au_tftp
>
> This patch then also needs to be adapted when changing to millisecond
> timeout resolution, so please submit a v2 when this is done.
OK, will do. Thanks for your other comments -- please see my replies to
some of them below; comments not replied to are generally OK and will be
addressed in v2 of the patches.
>
>> Signed-off-by: Rafal Czubak <[EMAIL PROTECTED]>
>> Signed-off-by: Bartlomiej Sieka <[EMAIL PROTECTED]>
>> ---
>> README | 12 ++
>> common/Makefile | 1 +
>> common/au_tftp.c | 279
>> +++++++++++++++++++++++++++++++++++++++
>> common/main.c | 7 +
>> doc/README.au_tftp | 89 +++++++++++++
>> doc/uImage.FIT/update3.its | 41 ++++++
>> doc/uImage.FIT/update_uboot.its | 21 +++
>> 7 files changed, 450 insertions(+), 0 deletions(-)
>> create mode 100644 common/au_tftp.c
>> create mode 100644 doc/README.au_tftp
>> create mode 100644 doc/uImage.FIT/update3.its
>> create mode 100644 doc/uImage.FIT/update_uboot.its
>>
>> diff --git a/README b/README
>> index ccd839c..23516eb 100644
>> --- a/README
>> +++ b/README
>> @@ -1737,6 +1737,14 @@ The following options need to be configured:
>> example, some LED's) on your board. At the moment,
>> the following checkpoints are implemented:
>>
>> +- Automatic software updates via TFTP server
>> + CONFIG_AU_TFTP
>> + CONFIG_AU_TFTP_CNT_MAX
>> + CONFIG_AU_TFTP_SEC_MAX
>> +
>> + These options enable and control the auto-update feature;
>> + for a more detailed description refer to doc/README.au_tftp.
>> +
>> Legacy uImage format:
>>
>> Arg Where When
>> @@ -2811,6 +2819,10 @@ Some configuration options can be set using
>> Environment Variables:
>> allowed for use by the bootm command. See also "bootm_low"
>> environment variable.
>>
>> + auto-update - Location of the sofware update file on a TFTP server,
>> used
>
> I think "auto-update" is not a good name (especially since it has a
> different meaning than the similar sounding "autoload"0; also there is
> a typo in "sofware".
>
> But most of all - do we really need a new environment variable? What's
> wrong with our good old "bootfile" ?
The only concern here is the interaction with bootp and dhcp commands --
they will set the "bootfile" env. variable to the file name
got from the server, and the next time around U-Boot will try to use
that file name to get the update. So I'd rather stick with a separate
env. variable for the name of the update file.
>
>> --- a/common/Makefile
>> +++ b/common/Makefile
>> @@ -155,6 +155,7 @@ COBJS-$(CONFIG_LCD) += lcd.o
>> COBJS-$(CONFIG_LYNXKDI) += lynxkdi.o
>> COBJS-$(CONFIG_USB_KEYBOARD) += usb_kbd.o
>> COBJS-$(CONFIG_DDR_SPD) += ddr_spd.o
>> +COBJS-$(CONFIG_AU_TFTP) += au_tftp.o
>
> Maybe we could start making lists sorted again (or at least no add to
> the confusion)? Thanks.
>
>> COBJS := $(sort $(COBJS-y))
>> SRCS := $(AOBJS:.o=.S) $(COBJS:.o=.c)
>> diff --git a/common/au_tftp.c b/common/au_tftp.c
>> new file mode 100644
>> index 0000000..7dfecab
>> --- /dev/null
>> +++ b/common/au_tftp.c
>> @@ -0,0 +1,279 @@
> ...
>> + memset(&saved_netretry, 0, AU_NETRETRY_LEN);
>> + if ((netretry = getenv("netretry")) != NULL) {
>> + if (strlen(netretry) >= AU_NETRETRY_LEN)
>> + printf("netretry value too long, won't be restored\n");
>> + else
>> + strncpy(saved_netretry, netretry, AU_NETRETRY_LEN - 1);
>> + }
>
> Maybe a "char *saved_netretry = strdup(getenv("netretry"));" would have been
> less hassle?
>
>> + /* set timeouts for auto-update */
>> + TftpRRQTimeoutSecs = sec_max;
>> + TftpRRQTimeoutCountMax = cnt_max;
>> +
>> + /* we don't want to retry the connection if errors occur */
>> + setenv("netretry", "no");
>> +
>> + /* download the update file */
>> + load_addr = addr;
>> + copy_filename(BootFile, filename, sizeof(BootFile));
>> + size = NetLoop(TFTP);
>> +
>> + if (size < 0)
>> + rv = 1;
>> + else if (size > 0)
>> + flush_cache(addr, size);
>> +
>> + /* restore changed globals and env variable */
>> + TftpRRQTimeoutSecs = saved_timeout_secs;
>> + TftpRRQTimeoutCountMax = saved_timeout_count;
>> +
>> + if (saved_netretry[0] != '\0')
>> + setenv("netretry", saved_netretry);
>> + else
>> + setenv("netretry", NULL);
>
> See above
>
>> + return rv;
>> +}
>> +
>> +static int au_flash(uint32_t addr_source, uint32_t addr_first, uint32_t
>> size)
>> +{
>> + uint32_t addr_last, bank, sector_end_addr;
>> + flash_info_t *info;
>> + char found;
>> + int i;
>> +
>> + /* compute correct addr_last */
>> + addr_last = addr_first + size - 1;
>> +
>> + if (addr_first >= addr_last) {
>> + printf("Error: end address exceeds addressing space\n");
>> + return 1;
>> + }
>
> This is obviously for NOR flash only.
>
> How could the code be extended to be usable for NAND flash /
> DataFlash / OneNAND / IDE storage devices as well?
Primarily, FIT specification for the update file will have to be
extended. Current approach is able to handle a series of <data, address>
pairs, where the address is enough for U-Boot to tell where the data
should go. If we are to handle other storage types, we need to specify
which storage type the data should go to, and also provide a
type-specific location. This is somewhat akin to the way we access and
boot from these storage types: we use type-specific commands (nand,
nboot, ide, diskboot, etc.).
Also, it would be of course nice to have a framework within U-Boot for
generic data copying between storage types, that would hide all the
type-specific details and provide uniform interface.
>
>> + for (bank = 0; bank < CFG_MAX_FLASH_BANKS && !found; ++bank) {
>> + info = &flash_info[bank];
>> + for (i = 0; i < info->sector_count && !found; ++i) {
>> + /* get the end address of the sector */
>> + if (i == info->sector_count - 1)
>> + sector_end_addr = info->start[0] +
>> + info->size - 1;
>
> Curly braces are needed for multiline statements.
>
>> + else
>> + sector_end_addr = info->start[i+1] - 1;
>
> And then for the following "else" as well.
>
>> + /* enable protection on processed sectors */
>> + if (flash_sect_protect(1, addr_first, addr_last) > 0) {
>> + printf("Error: could not protect flash sectors\n");
>> + return 1;
>> + }
>
> This should only be done if these sectors have been protected before.
>
>> +void au_tftp(void)
>> +{
>
> Is it a good idea to make this a void function? How about error
> handling?
It seems to be a convention for functions conditionally called from
main_loop() to have the error handling withing them, and ignore their
return values in main_loop(). So au_tftp() also handles errors itself,
and doesn't return anything.
>
>> --- a/common/main.c
>> +++ b/common/main.c
>> @@ -56,6 +56,9 @@ extern int do_reset (cmd_tbl_t *cmdtp, int flag, int argc,
>> char *argv[]); /* fo
>>
>> extern int do_bootd (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]);
>>
>> +#if defined(CONFIG_AU_TFTP)
>> +void au_tftp (void);
>> +#endif /* CONFIG_AU_TFTP */
>>
>> #define MAX_DELAY_STOP_STR 32
>>
>> @@ -290,6 +293,10 @@ void main_loop (void)
>> char bcs_set[16];
>> #endif /* CONFIG_BOOTCOUNT_LIMIT */
>>
>> +#if defined(CONFIG_AU_TFTP)
>> + au_tftp ();
>> +#endif /* CONFIG_AU_TFTP */
>> +
>> #if defined(CONFIG_VFD) && defined(VFD_TEST_LOGO)
>> ulong bmp = 0; /* default bitmap */
>> extern int trab_vfd (ulong bitmap);
>
> You definitely don't want to add the function call right in the
> middle of variable declarations, or do you?
The idea is to have au_tftp() called as early as possible, before any
other functions in main_loop().
Here's a larger snippet of the related code:
#if defined(CONFIG_VFD) && defined(VFD_TEST_LOGO)
ulong bmp = 0; /* default bitmap */
extern int trab_vfd (ulong bitmap);
#ifdef CONFIG_MODEM_SUPPORT
if (do_mdm_init)
bmp = 1; /* alternate bitmap */
#endif
trab_vfd (bmp);
#endif /* CONFIG_VFD && VFD_TEST_LOGO */
So if we move the call to au_tftp() someplace below, then when
both CONFIG_VFD and VFD_TEST_LOGO are defined, we'll have a call to
trab_vfd(), which will happen before the software update.
Note that there are several other cases in the main_loop() where
conditionally included code introduces declarations intermixed with
instructions. If this issue is to be cleaned-up, then let's have it done
as a separate patch.
Regards,
Bartlomiej Sieka
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
