Automatic software updates in U-Boot

There are two aspects of a TFTP transfer involving timeouts:
1. timeout waiting for initial server reply after sending RRQ
2. timeouts while transferring actual data from the server

Since the upcoming auto-update feature attempts a TFTP download during each boot, it is undesirable to have a long delay when the TFTP server is not available. Thus, this commit makes the server timeout (1.) configurable by two global variables:

TftpRRQTimeoutSecs
TftpRRQTimeoutCountMax

TftpRRQTimeoutSecs overrides default timeout when trying to connect to a TFTP server, TftpRRQTimeoutCountMax overrides default number of connection retries. The total delay when trying to download a file from a non-existing TFTP server is TftpRRQTimeoutSecs x TftpRRQTimeoutCountMax seconds.

Quoted 3 lines · Bartlomiej Sieka

There are two aspects of a TFTP transfer involving timeouts:
1. timeout waiting for initial server reply after sending RRQ
2. timeouts while transferring actual data from the server

Are seconds an appropriate scale factor for the timeout? Using tenths (thousandths?) of seconds seems much better for allowing timeout choices. (Thousandths could cause problems with clock tick resolution and is unnecessarily fine grained. Gut feel is tenths of seconds is

Just my two cents worth...

Quoted 9 lines · Jerry Van Baren

Quoted 3 lines · Bartlomiej Sieka

There are two aspects of a TFTP transfer involving timeouts:
1. timeout waiting for initial server reply after sending RRQ
2. timeouts while transferring actual data from the server

Are seconds an appropriate scale factor for the timeout? Using tenths (thousandths?) of seconds seems much better for allowing timeout choices. (Thousandths could cause problems with clock tick resolution and is unnecessarily fine grained. Gut feel is tenths of seconds is

I would have thought that milliseconds would be the most appropriate choice (milliseconds being an SI unit and most timeouts I have seen have been defined in milliseconds) - If it is going to be a problem at the lower level, you can always divide by 10 (or 100) when you actually implement the timeout functionality. I just think at the user level it should be seconds or milliseconds

Regards,
Graeme

Quoted 14 lines · Bartlomiej Sieka

There are two aspects of a TFTP transfer involving timeouts:
1. timeout waiting for initial server reply after sending RRQ
2. timeouts while transferring actual data from the server

Since the upcoming auto-update feature attempts a TFTP download during each
boot, it is undesirable to have a long delay when the TFTP server is not available. Thus, this commit makes the server timeout (1.) configurable by two
global variables:

TftpRRQTimeoutSecs
TftpRRQTimeoutCountMax

Quoted 7 lines

TftpRRQTimeoutSecs overrides default timeout when trying to connect to a TFTP
server, TftpRRQTimeoutCountMax overrides default number of connection retries.
The total delay when trying to download a file from a non-existing TFTP server
is TftpRRQTimeoutSecs x TftpRRQTimeoutCountMax seconds.

Hi Bartlomiej,

Are seconds an appropriate scale factor for the timeout?

Quoted 2 lines

Are seconds an appropriate scale factor for the timeout?

The patch didn't introduce changes in this regard -- TFTP timeouts were defined in seconds originally. The patch makes the timeout behavior configurable, instead of being hardcoded, but the units remain the same:

Dear Bartek,

Quoted 1 line · In message <[email protected]> you wrote:

Quoted 1 line

Are seconds an appropriate scale factor for the timeout?

...

Quoted 3 lines

The patch didn't introduce changes in this regard -- TFTP timeouts were defined in seconds originally. The patch makes the timeout behavior configurable, instead of being hardcoded, but the units remain the same:

But the argument was a perfectly valid one. I am, too, concerned that second resolution is not optimal any more (it was as long as we needed just one timeout long enough, but this situation will be changed by your other patches).

So please extend that patch to change this into milliseconds as discussed before.

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

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

README +8 -0

index ccd839c..23516eb 100644
@@ -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

common/Makefile +0 -0

 COBJS	:= $(sort $(COBJS-y))
 SRCS	:= $(AOBJS:.o=.S) $(COBJS:.o=.c)

common/au_tftp.c new file +0 -0

new file mode 100644
index 0000000..7dfecab
@@ -0,0 +1,279 @@

common/au_tftp.c new file +15 -0

+	/* 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)

common/au_tftp.c new file +7 -0

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

common/main.c +3 -0

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

Dear Bartlomiej Sieka,

Quoted 5 lines · 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.

Quoted 40 lines

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

README +8 -0

index ccd839c..23516eb 100644
@@ -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.
 

README Some configuration options can be set using Environment Variables: · line 2822

Quoted 1 line

README +1 -0

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

common/Makefile COBJS-$(CONFIG_LCD) += lcd.o · line 158

Quoted 8 lines

common/Makefile +1 -0

@@ -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.

Quoted 8 lines

common/Makefile +0 -0

 COBJS	:= $(sort $(COBJS-y))
 SRCS	:= $(AOBJS:.o=.S) $(COBJS:.o=.c)

common/au_tftp.c new file +0 -0

new file mode 100644
index 0000000..7dfecab
@@ -0,0 +1,279 @@

...

common/au_tftp.c line 83

Quoted 7 lines

common/au_tftp.c new file +7 -0

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

Quoted 15 lines

common/au_tftp.c new file +15 -0

+	/* 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)

common/au_tftp.c line 109

Quoted 10 lines

common/au_tftp.c new file +10 -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

Quoted 7 lines

common/au_tftp.c new file +7 -0

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

common/au_tftp.c line 127

Quoted 10 lines

common/au_tftp.c new file +10 -0

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

common/au_tftp.c line 143

Quoted 8 lines

common/au_tftp.c new file +7 -0

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

common/au_tftp.c

Quoted 2 lines

common/au_tftp.c new file +2 -0

+			else
+				sector_end_addr = info->start[i+1] - 1;

And then for the following "else" as well.

common/au_tftp.c line 185

Quoted 5 lines

common/au_tftp.c new file +5 -0

+	/* 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.

common/au_tftp.c line 207

Quoted 2 lines

common/au_tftp.c new file +2 -0

+void au_tftp(void)
+{

Is it a good idea to make this a void function? How about error handling?

Quoted 17 lines

common/main.c +3 -0

@@ -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 */
 

common/main.c void main_loop (void) · line 302

Quoted 7 lines

common/main.c +4 -0

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

Quoted 5 lines

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

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 4 lines

Quoted 4 lines

common/main.c +0 -0

@@ -290,6 +293,10 @@ void main_loop (void)
 	char bcs_set[16];
 #endif /* CONFIG_BOOTCOUNT_LIMIT */
 

Quoted 10 lines

common/main.c void main_loop (void) · line 302

Quoted 7 lines

common/main.c +4 -0

+#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().

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.

Dear Bartek,

Quoted 6 lines · in message <[email protected]> you wrote:

Quoted 6 lines

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

Quoted 5 lines

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.

I see. Maybe we should call the variable "updatefile" or similar, then?

Quoted 13 lines

Quoted 4 lines

Quoted 4 lines

common/main.c +0 -0

@@ -290,6 +293,10 @@ void main_loop (void)
 	char bcs_set[16];
 #endif /* CONFIG_BOOTCOUNT_LIMIT */
 

Quoted 9 lines

common/main.c void main_loop (void) · line 302

Quoted 7 lines

common/main.c +4 -0

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

Quoted 2 lines

The idea is to have au_tftp() called as early as possible, before any other functions in main_loop().

Yes, but this is C, not C++, so declarations go only at the bginning of the function, then follows code (and no more declarations unless you open a new block).

Quoted 3 lines

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.

So you will either have to add some more #ifdef's, or think what could happen if the VFD (Vacuum Fluorescent Display) initialization code runs first - I would not expect any negative impact?

Quoted 3 lines

I see. Maybe we should call the variable "updatefile" or similar, then?

How about "au_file"? "updatefile" suffers from similarity to the commonly used (although not documented) "update" env. variable. But I'm fine either way, just let me know what you prefer.

Quoted 3 lines

So you will either have to add some more #ifdef's, or think what could happen if the VFD (Vacuum Fluorescent Display) initialization code runs first - I would not expect any negative impact?

I don't think there will be any negative impact per se, the update procedure will just start a bit later (the time needed to display the bitmap on the FVD).

Will move au_tftp() call till after the VFD stuff.

Quoted 6 lines

Quoted 2 lines

I see. Maybe we should call the variable "updatefile" or similar, then?

How about "au_file"? "updatefile" suffers from similarity to the commonly used (although not documented) "update" env. variable. But I'm fine either way, just let me know what you prefer.

I would use "updatefile", but if you prefer "au_file" (and nobody else comes up with comments or better suggestions) that's fine with me, too.

Quoted 1 line

Quoted 1 line

More detailed description can be found in doc/README.au_tftp

Quoted 1 line · Bartlomiej Sieka

Quoted 1 line

Quoted 1 line

More detailed description can be found in doc/README.au_tftp

'au' as a prefix seems awfully terse and cryptic to me (not to mention reminding me of Australians and gold), something a bit longer would go a long way to making it clearer what the code does.

Quoted 1 line · On Thu, 25 Sep 2008 10:33:30 -0500 "Andrew Dyer" <[email protected]> wrote:

On Thu, Sep 25, 2008 at 3:16 AM, Bartlomiej Sieka <[email protected]> wrote:

Quoted 4 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

More detailed description can be found in doc/README.au_tftp

'au' as a prefix seems awfully terse and cryptic to me (not to mention reminding me of Australians and gold), something a bit longer would go a long way to making it clearer what the code does.

agreed. autoupdate_tftp?

Kim

Quoted 7 lines · Bartlomiej Sieka

Quoted 1 line

Quoted 1 line

Quoted 1 line

More detailed description can be found in doc/README.au_tftp

'au' as a prefix seems awfully terse and cryptic to me (not to mention reminding me of Australians and gold), something a bit longer would go a long way to making it clearer what the code does.

agreed. autoupdate_tftp?

Dear Kim,

Quoted 8 lines · In message <[email protected]> you wrote:

Quoted 7 lines · Bartlomiej Sieka

Quoted 1 line

Quoted 1 line

Quoted 1 line

More detailed description can be found in doc/README.au_tftp

'au' as a prefix seems awfully terse and cryptic to me (not to mention reminding me of Australians and gold), something a bit longer would go a long way to making it clearer what the code does.

Quoted 1 line

agreed. autoupdate_tftp?

I think we should have a "*file" name so it's clear that this is a file name and not - for example - a logical variable that takes "yes"/"no" value or similar.

Quoted 8 lines · On Thu, 25 Sep 2008 20:17:30 +0200 Wolfgang Denk <[email protected]> wrote:

Dear Kim,

Quoted 5 lines · In message <[email protected]> you wrote:

Quoted 5 lines · Bartlomiej Sieka

Quoted 1 line

Quoted 1 line

Quoted 1 line

More detailed description can be found in doc/README.au_tftp

'au' as a prefix seems awfully terse and cryptic to me (not to mention reminding me of Australians and gold), something a bit longer would go a long way to making it clearer what the code does.

Quoted 4 lines

Quoted 1 line

agreed. autoupdate_tftp?

I think we should have a "*file" name so it's clear that this is a file name and not - for example - a logical variable that takes "yes"/"no" value or similar.

yes, then we'd be consistent with bootfile, fdtfile, etc.

How about 'updatefile' for the env. variable and 'README.update' for the documentation?

Dear Bartlomiej Sieka,

Quoted 2 lines · In message <[email protected]> you wrote:

How about 'updatefile' for the env. variable and 'README.update' for the documentation?

ACK from me (this is doc/README.update, right?)

This is the second version of the patch series adding support for automatic software updates from a TFTP server. V2 adds millisecond granularity of the TFTP timeouts and addresses comments posted to the ML for the first version.

First three patches make some general changes required by the new feature, first one in the Flash subsystem, second and third in the networking code. Then follows a minor clean-up, with the feature proper in fifth patch. Last patch provides minor enhancements to the iminfo output that can become handy when using the new feature.

Please see newly added doc/README.update for more details.

Code has been compile-tested on several ppc, arm and mips targets. The feature itself has been tested on TQM8555 and MPC8555CDS and trab boards.

Dear Bartek,

Quoted 10 lines · in message <[email protected]> you wrote:

This is the second version of the patch series adding support for automatic software updates from a TFTP server. V2 adds millisecond granularity of the TFTP timeouts and addresses comments posted to the ML for the first version.

First three patches make some general changes required by the new feature, first one in the Flash subsystem, second and third in the networking code. Then follows a minor clean-up, with the feature proper in fifth patch. Last patch provides minor enhancements to the iminfo output that can become handy when using the new feature.

Quoted 4 lines

Please see newly added doc/README.update for more details.

Code has been compile-tested on several ppc, arm and mips targets. The feature itself has been tested on TQM8555 and MPC8555CDS and trab boards.

All patches have been checked in into the "next" branch of the U-Boot repository. Can you please check if everything is correct in place and working as expected?

Seems that the bulk of the feature didn't make it into the 'next' branch. Commit 'Automatic software update from TFTP server' on the branch (b830abed) shows only the following files modified:

Seems that the bulk of the feature didn't make it into the 'next' branch. Commit 'Automatic software update from TFTP server' on the branch (b830abed) shows only the following files modified:

common/Makefile

Whereas the patch posted to the list ('[PATCH v2 5/6] Automatic software update from TFTP server') has the following diffstat:

  README                          |   12 ++
  common/Makefile                 |    1 +
  common/main.c                   |    7 +
  common/update.c                 |  315 
+++++++++++++++++++++++++++++++++++++++
  doc/README.update               |   90 +++++++++++
  doc/uImage.FIT/update3.its      |   41 +++++
  doc/uImage.FIT/update_uboot.its |   21 +++
  7 files changed, 487 insertions(+), 0 deletions(-)

create mode 100644 common/update.c
create mode 100644 doc/README.update create mode 100644 doc/uImage.FIT/update3.its create mode 100644 doc/uImage.FIT/update_uboot.its

The above changes and newly added files associated with the feature are not present in the tree.

Other commits needed for the auto-update feature are present on the 'next' branch and look OK.

Dear Bartlomiej,

Quoted 26 lines · in message <[email protected]> you wrote:

Seems that the bulk of the feature didn't make it into the 'next' branch. Commit 'Automatic software update from TFTP server' on the branch (b830abed) shows only the following files modified:

common/Makefile

Whereas the patch posted to the list ('[PATCH v2 5/6] Automatic software update from TFTP server') has the following diffstat:

  README                          |   12 ++
  common/Makefile                 |    1 +
  common/main.c                   |    7 +
  common/update.c                 |  315 
+++++++++++++++++++++++++++++++++++++++
  doc/README.update               |   90 +++++++++++
  doc/uImage.FIT/update3.its      |   41 +++++
  doc/uImage.FIT/update_uboot.its |   21 +++
  7 files changed, 487 insertions(+), 0 deletions(-)

create mode 100644 common/update.c
create mode 100644 doc/README.update create mode 100644 doc/uImage.FIT/update3.its create mode 100644 doc/uImage.FIT/update_uboot.its

The above changes and newly added files associated with the feature are not present in the tree.

Quoted 2 lines

Other commits needed for the auto-update feature are present on the 'next' branch and look OK.

I think I understand what happened, and fixed it.

Please check again.

The upcoming automatic update feature needs the ability to adjust an address within Flash to the end of its respective sector. Factor out this functionality to a new function flash_sect_roundb().

Signed-off-by Rafal Czubak <[email protected]>

Signed-off-by Bartlomiej Sieka <[email protected]>

---
 common/cmd_flash.c |   75 +++++++++++++++++++++++++++++-----------------------
 include/flash.h    |    1 +
 2 files changed, 43 insertions(+), 33 deletions(-)

common/cmd_flash.c +42 -33

index 18d2250..29e5b6d 100644
@@ -105,6 +105,47 @@ abbrev_spec (char *str, flash_info_t ** pinfo, int *psf, int *psl)
 }
 
 /*
+ * Take *addr in Flash and adjust it to fall on the end of its sector
+ */
+int flash_sect_roundb (ulong *addr)
+{
+	flash_info_t *info;
+	ulong bank, sector_end_addr;
+	char found;
+	int i;
+
+	/* find the end addr of the sector where the *addr is */
+	found = 0;
+	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;
+			} else {
+				sector_end_addr = info->start[i+1] - 1;
+			}
+
+			if (*addr <= sector_end_addr &&
+						*addr >= info->start[i]) {
+				found = 1;
+				/* adjust *addr if necessary */
+				if (*addr < sector_end_addr)
+					*addr = sector_end_addr;
+			} /* sector */
+		} /* bank */
+	}
+	if (!found) {
+		/* error, addres not in flash */
+		printf("Error: end address (0x%08lx) not in flash!\n", *addr);
+		return 1;
+	}
+
+	return 0;
+}
+
+/*
  * This function computes the start and end addresses for both
  * erase and protect commands. The range of the addresses on which
  * either of the commands is to operate can be given in two forms:

@@ -126,8 +167,6 @@ addr_spec(char *arg1, char *arg2, ulong *addr_first, ulong *addr_last)
 {
 	char *ep;
 	char len_used; /* indicates if the "start +length" form used */
-	char found;
-	ulong bank;
 
 	*addr_first = simple_strtoul(arg1, &ep, 16);
 	if (ep == arg1 || *ep != '\0')

@@ -157,38 +196,8 @@ addr_spec(char *arg1, char *arg2, ulong *addr_first, ulong *addr_last)
 		 * sector boundary, so that the commands don't fail later on.
 		 */
 
-		/* find the end addr of the sector where the *addr_last is */
-		found = 0;
-		for (bank = 0; bank < CFG_MAX_FLASH_BANKS && !found; ++bank){
-			int i;
-			flash_info_t *info = &flash_info[bank];
-			for (i = 0; i < info->sector_count && !found; ++i){
-				/* get the end address of the sector */
-				ulong sector_end_addr;
-				if (i == info->sector_count - 1){
-					sector_end_addr =
-						info->start[0] + info->size - 1;
-				} else {
-					sector_end_addr =
-						info->start[i+1] - 1;
-				}
-				if (*addr_last <= sector_end_addr &&
-						*addr_last >= info->start[i]){
-					/* sector found */
-					found = 1;
-					/* adjust *addr_last if necessary */
-					if (*addr_last < sector_end_addr){
-						*addr_last = sector_end_addr;
-					}
-				}
-			} /* sector */
-		} /* bank */
-		if (!found){
-			/* error, addres not in flash */
-			printf("Error: end address (0x%08lx) not in flash!\n",
-								*addr_last);
+		if (flash_sect_roundb(addr_last) > 0)
 			return -1;
-		}
 	} /* "start +length" from used */
 
 	return 1;

include/flash.h +1 -0

index af8a7c0..6f5d7d5 100644
@@ -91,6 +91,7 @@ extern void flash_print_info (flash_info_t *);
 extern int flash_erase	(flash_info_t *, int, int);
 extern int flash_sect_erase (ulong addr_first, ulong addr_last);
 extern int flash_sect_protect (int flag, ulong addr_first, ulong addr_last);
+extern int flash_sect_roundb (ulong *addr);
 
 /* common/flash.c */
 extern void flash_protect (int flag, ulong from, ulong to, flash_info_t *info);

Do you have any comments on the below patch?

Hi Bartlomiej,

Quoted 1 line · Bartlomiej Sieka

Do you have any comments on the below patch?

Looks good to me. So:

Acked-by Stefan Roese <[email protected]>

Not sure if I am responsible for picking this up or if this should go in through Wolfgang. It's not really cfi-flash related. But if nobody objects then I'll put it into the next branch of my cfi-flash repository.

Quoted 4 lines

Not sure if I am responsible for picking this up or if this should go in through Wolfgang. It's not really cfi-flash related. But if nobody objects then I'll put it into the next branch of my cfi-flash repository.

It depends on how/when Wolfgang wants to handle the inclusion of the automatic update feature (patches 4-6/6). This feature depends on the flash patch in question (1/6), as well as on two networking changes (patches 2/6 and 3/6), which have been pulled into the 'next' branch of the net repository already.

Quoted 10 lines

Quoted 4 lines

Not sure if I am responsible for picking this up or if this should go in through Wolfgang. It's not really cfi-flash related. But if nobody objects then I'll put it into the next branch of my cfi-flash repository.

It depends on how/when Wolfgang wants to handle the inclusion of the automatic update feature (patches 4-6/6). This feature depends on the flash patch in question (1/6), as well as on two networking changes (patches 2/6 and 3/6), which have been pulled into the 'next' branch of the net repository already.

If eveybody co-operates (Stefan for the flash part, Ben for the network part), I would like to pull this into my "next" branch.

Stefan, Ben - please let me know when your next branches are ready for pulling, so I can coordinate this.

OK, I added this patch [1/6] to the u-boot-cfi-flash next branch. Here the pull request for the *next* branch:

The following changes since commit 8fd4166c467a46773f80208bda1ec3b4757747bc:
Stefan Roese (1):
ppc4xx: Canyonlands: Remove unnecessary FDT warning upon DTB fixup

are available in the git repository at:

git://www.denx.de/git/u-boot-cfi-flash.git next

Bartlomiej Sieka (1):
flash: factor out adjusting of Flash address to the end of sector

 common/cmd_flash.c |   75 +++++++++++++++++++++++++++++-----------------------
 include/flash.h    |    1 +
 2 files changed, 43 insertions(+), 33 deletions(-)

Dear Bartek,

Quoted 10 lines · in message <[email protected]> you wrote:

Quoted 4 lines

Not sure if I am responsible for picking this up or if this should go in through Wolfgang. It's not really cfi-flash related. But if nobody objects then I'll put it into the next branch of my cfi-flash repository.

It depends on how/when Wolfgang wants to handle the inclusion of the automatic update feature (patches 4-6/6). This feature depends on the flash patch in question (1/6), as well as on two networking changes (patches 2/6 and 3/6), which have been pulled into the 'next' branch of the net repository already.

If eveybody co-operates (Stefan for the flash part, Ben for the network part), I would like to pull this into my "next" branch.

Stefan, Ben - please let me know when your next branches are ready for pulling, so I can coordinate this.

Thanks.

Best regards,

Wolfgang Denk

You can pull net/next now if you'd like. I'm not able to generate a pull request from here, so if you need that you'll have to wait maybe 10 or 11 hours.

Enforce millisecond semantics of the first argument to NetSetTimeout() -- the change is transparent for well-behaving boards (CFG_HZ == 1000 and get_timer() countiing in milliseconds).

Rationale for this patch is to enable millisecond granularity for network-related timeouts, which is needed for the upcoming automatic software update feature.

Summary of changes:
- do not scale the first argument to NetSetTimeout() by CFG_HZ
- change timeout values used in the networking code to milliseconds

Signed-off-by Rafal Czubak <[email protected]>

Signed-off-by Bartlomiej Sieka <[email protected]>

---

There are two aspects of a TFTP transfer involving timeouts:
1. timeout waiting for initial server reply after sending RRQ
2. timeouts while transferring actual data from the server

Since the upcoming auto-update feature attempts a TFTP download during each boot, it is undesirable to have a long delay when the TFTP server is not available. Thus, this commit makes the server timeout (1.) configurable by two global variables:

TftpRRQTimeoutMSecs
TftpRRQTimeoutCountMax

TftpRRQTimeoutMSecs overrides default timeout when trying to connect to a TFTP server, TftpRRQTimeoutCountMax overrides default number of connection retries. The total delay when trying to download a file from a non-existing TFTP server is TftpRRQTimeoutMSecs x TftpRRQTimeoutCountMax milliseconds.

Timeouts during file transfers (2.) are unaffected.

Signed-off-by Rafal Czubak <[email protected]>

Signed-off-by Bartlomiej Sieka <[email protected]>

---

common/Makefile

Quoted 7 lines · Bartlomiej Sieka

Signed-off-by Bartlomiej Sieka <[email protected]>

---
 common/Makefile |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

common/Makefile +0 -0

index 8bddf8e..bb6655d 100644

Please base your patch on u-boot-arm/next

and I've plane to split it in a subsection to not have it in the middle of the command

[dropped sr at denx.de and biggerbadderben at gmail.com from CC]

Hi Jean-Christophe,

Quoted 9 lines · Jean-Christophe PLAGNIOL-VILLARD

common/Makefile

Quoted 7 lines · Bartlomiej Sieka

Signed-off-by Bartlomiej Sieka <[email protected]>

---
 common/Makefile |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

common/Makefile +0 -0

index 8bddf8e..bb6655d 100644

Please base your patch on u-boot-arm/next

Sorry, I'm not sure here -- the patch makes a simple change to common/Makefile, so why would re-basing to arm/next be necessary?

Quoted 3 lines

and I've plane to split it in a subsection to not have it in the middle of the command

Is this comment related to the patch? Could you please elaborate?

Quoted 14 lines · Bartlomiej Sieka

[dropped sr at denx.de and biggerbadderben at gmail.com from CC]

Hi Jean-Christophe,

Quoted 9 lines · Jean-Christophe PLAGNIOL-VILLARD

common/Makefile

Quoted 7 lines · Bartlomiej Sieka

Signed-off-by Bartlomiej Sieka <[email protected]>

---
 common/Makefile |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

common/Makefile +0 -0

index 8bddf8e..bb6655d 100644

Please base your patch on u-boot-arm/next

Quoted 6 lines

Sorry, I'm not sure here -- the patch makes a simple change to common/Makefile, so why would re-basing to arm/next be necessary?

Quoted 2 lines

and I've plane to split it in a subsection to not have it in the middle of the command

Is this comment related to the patch? Could you please elaborate?

I've split the Makefile in 4 subgroup

# core
# core command
# environment
# command

It will be good to regroup non command file to

# others
or something like this

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.update.

Signed-off-by Rafal Czubak <[email protected]>

Signed-off-by Bartlomiej Sieka <[email protected]>

---
 README                          |   12 ++
 common/Makefile                 |    1 +
 common/main.c                   |    7 +
 common/update.c                 |  315 +++++++++++++++++++++++++++++++++++++++
 doc/README.update               |   90 +++++++++++
 doc/uImage.FIT/update3.its      |   41 +++++
 doc/uImage.FIT/update_uboot.its |   21 +++
 7 files changed, 487 insertions(+), 0 deletions(-)
 create mode 100644 common/update.c
 create mode 100644 doc/README.update
 create mode 100644 doc/uImage.FIT/update3.its
 create mode 100644 doc/uImage.FIT/update_uboot.its