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.

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

> --- 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: [EMAIL PROTECTED]
Hegel was right when he said that we learn from history that man  can
never learn anything from history.              - George Bernard Shaw
_______________________________________________
U-Boot mailing list
[email protected]
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to