[U-Boot] [PATCH 2/3] Automatic software update from TFTP server

Wolfgang Denk
2008-09-22T21:03:08Z

Dear Bartlomiej Sieka,

In message <12217502101047-git-send-email-tur@semihalf.com> 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.

> Signed-off-by: Rafal Czubak <rcz@semihalf.com>
> Signed-off-by: Bartlomiej Sieka <tur@semihalf.com>
> ---
> 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" ?

> --- 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?

> + 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?

> --- 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?

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Hegel was right when he said that we learn from history that man can
never learn anything from history. - George Bernard Shaw