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

[Wolfgang Denk]

Dear Bartlomiej Sieka,

In message <12217502101047-git-send-email-tur at 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 at semihalf.com>
> Signed-off-by: Bartlomiej Sieka <tur at 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