[U-Boot] [PATCH v2 0/9] dfu: tftp: update: Support for DFU upgrades via ETH (TFTP)

[U-Boot] [PATCH v2 0/9] dfu: tftp: update: Support for DFU upgrades via ETH (TFTP)

[Lukasz Majewski]

This commit series enables DFU subsystem to use ETH and TFTP protocol as 
a medium for downloading data, which should bring substantial speedup
for writing large files (like rootfs).

Please read provided ./doc/README.dfutftp documentation entry for more
information.

Those patches should be applied on the u-boot-dfu tree:
SHA1: 09e46b733a9b02d10d3d29001c316dc937fe6b44

Lukasz Majewski (9):
  doc: dfu: tftp: README entry for TFTP extension of DFU
  net: tftp: Move tftp.h file from ./net to ./include/net
  tftp: update: Allow some parts of the code to be reused when
    CONFIG_SYS_NO_FLASH is set
  dfu: tftp: update: Provide tftp support for the DFU subsystem
  dfu: tftp: update: Add dfu_write_from_mem_addr() function
  update: tftp: dfu: Extend update_tftp() function to support DFU
  dfu: command: Extend "dfu" command to handle receiving data via TFTP
  config: bbb: Configs necessary for running update via TFTP on Beagle
    Bone Black
  dfu:tests: Modify dfu_gadget_test.sh to accept USB device major:minor
    number

 common/Makefile              |   1 +
 common/cmd_dfu.c             |  25 ++++++++++
 common/cmd_fitupd.c          |   2 +-
 common/main.c                |   2 +-
 common/update.c              |  51 +++++++++++++------
 doc/README.dfutftp           | 113 +++++++++++++++++++++++++++++++++++++++++++
 drivers/dfu/Makefile         |   1 +
 drivers/dfu/dfu.c            |  48 ++++++++++++++++++
 drivers/dfu/dfu_tftp.c       |  65 +++++++++++++++++++++++++
 include/configs/am335x_evm.h |   8 +++
 include/dfu.h                |  14 ++++++
 include/net.h                |   2 +-
 include/net/tftp.h           |  30 ++++++++++++
 net/bootp.c                  |   2 +-
 net/net.c                    |   2 +-
 net/rarp.c                   |   2 +-
 net/tftp.c                   |   2 +-
 net/tftp.h                   |  30 ------------
 test/dfu/README              |   9 +++-
 test/dfu/dfu_gadget_test.sh  |  18 ++++---
 20 files changed, 366 insertions(+), 61 deletions(-)
 create mode 100644 doc/README.dfutftp
 create mode 100644 drivers/dfu/dfu_tftp.c
 create mode 100644 include/net/tftp.h
 delete mode 100644 net/tftp.h

-- 
2.1.4

[U-Boot] [PATCH v2 1/9] doc: dfu: tftp: README entry for TFTP extension of DFU

[Lukasz Majewski]

Documentation file for DFU extension. With this functionality it is now
possible to transfer FIT images with firmware updates via TFTP and use
DFU backend for storing them.

Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
---
Changes for v2:
- Remove section regarding update_tftp() specific environment variables
---
 doc/README.dfutftp | 113 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 113 insertions(+)
 create mode 100644 doc/README.dfutftp

diff --git a/doc/README.dfutftp b/doc/README.dfutftp
new file mode 100644
index 0000000..3b00386
--- /dev/null
+++ b/doc/README.dfutftp
@@ -0,0 +1,113 @@
+Device Firmware Upgrade (DFU) - extension to use TFTP
+=====================================================
+
+Why?
+----
+
+* Update TFTP (CONFIG_UPDATE_TFTP) only supports writing
+code to NAND memory via TFTP.
+* DFU supports writing data to the variety of mediums (NAND,
+eMMC, SD, partitions, RAM, etc) via USB.
+
+Combination of both solves their shortcomings!
+
+
+Overview
+--------
+
+This document briefly describes how to use DFU for
+upgrading firmware (e.g. kernel, u-boot, rootfs, etc.)
+via TFTP protocol.
+
+By using Ethernet (TFTP protocol to be precise) it was
+possible to overcome the major problem of USB based DFU -
+the relatively low transfer speed for large files.
+This was caused by DFU standard, which imposed utilization
+of only EP0 for transfer. By using Ethernet we can circumvent
+this shortcoming.
+
+Beagle Bone Black (BBB) powered by TI's am335x CPU has been used
+as a demo board.
+
+To utilize this feature, one needs to first enable support
+for USB based DFU (CONFIG_DFU_*) and DFU TFTP update
+(CONFIG_DFU_TFTP) described in ./doc/README.update.
+
+The "dfu" command has been extended to support transfer via TFTP - one
+needs to type for example "dfu tftp 0 mmc 0"
+
+This code works without "fitupd" command enabled.
+
+As of this writing (SHA1:241746e618fa725491e9ff689710c5f73ffd9139) the
+update.c code is not enabled (CONFIG_UPDATE_TFTP) by any board in the
+contemporary u-boot tree.
+
+
+Environment variables
+---------------------
+
+The "dfu tftp" command can be used in the "preboot" environment variable
+(when it is enabled by defining CONFIG_PREBOOT).
+This is the preferable way of using this command in the early boot stage
+as the opposition to legacy update_tftp() function invoking.
+
+
+Beagle Bone Black (BBB) setup
+-----------------------------
+
+1. Setup tftp env variables:
+   *  select desired eth device - 'ethact' variable ["ethact=cpsw"]
+      (use "bdinfo" to check current setting)
+   *  setup "serverip" and "ipaddr" variables
+   *  set "loadaddr" as a fixed buffer where incoming data is placed
+      ["loadaddr=0x81000000"]
+
+#########
+# BONUS #
+#########
+It is possible to use USB interface to emulate ETH connection by setting
+"ethact=usb_ether". In this way one can have very fast DFU transfer via USB.
+
+For 33MiB test image the transfer rate was 1MiB/s
+
+2. Setup update_tftp variables:
+   *  set "updatefile" - the file name to be downloaded via TFTP (stored on
+      the HOST at e.g. /srv/tftp)
+
+3. If required, to update firmware on boot, put the "dfu tftp 0 mmc 0" in the
+    "preboot" env variable. Otherwise use this command from u-boot prompt.
+
+4. Inspect "dfu" specific variables:
+   * "dfu_alt_info" - information about available DFU entities
+   * "dfu_bufsiz"   - variable to set buffer size [in bytes] - when it is not
+		      possible to set large enough default buffer (8 MiB @ BBB)
+
+
+
+FIT image format for download
+-----------------------------
+
+To create FIT image for download one should follow the update tftp README file
+(./doc/README.update) with one notable difference:
+
+The original snippet of ./doc/uImage.FIT/update_uboot.its
+
+	images {
+		update at 1 {
+			description = "U-Boot binary";
+
+should look like
+
+	images {
+		u-boot.bin at 1 {
+			description = "U-Boot binary";
+
+where "u-boot.bin" is the DFU entity name to be stored.
+
+
+
+To do
+-----
+
+* Extend dfu-util command to support TFTP based transfers
+* Upload support (via TFTP)
-- 
2.1.4

[U-Boot] [PATCH v2 2/9] net: tftp: Move tftp.h file from ./net to ./include/net

[Lukasz Majewski]

This change gives the ability to reuse the <tftp.h> header file by other
subsystems (like e.g. dfu).

Without this change compilation error emerges for the legacy update.c file.

Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>

---
Changes for v2:
- Move tftp.h from ./include to ./include/net/ directory
---
 include/net/tftp.h | 30 ++++++++++++++++++++++++++++++
 net/bootp.c        |  2 +-
 net/net.c          |  2 +-
 net/rarp.c         |  2 +-
 net/tftp.c         |  2 +-
 net/tftp.h         | 30 ------------------------------
 6 files changed, 34 insertions(+), 34 deletions(-)
 create mode 100644 include/net/tftp.h
 delete mode 100644 net/tftp.h

diff --git a/include/net/tftp.h b/include/net/tftp.h
new file mode 100644
index 0000000..c411c9b
--- /dev/null
+++ b/include/net/tftp.h
@@ -0,0 +1,30 @@
+/*
+ *	LiMon - BOOTP/TFTP.
+ *
+ *	Copyright 1994, 1995, 2000 Neil Russell.
+ *	Copyright 2011 Comelit Group SpA
+ *	               Luca Ceresoli <luca.ceresoli@comelit.it>
+ *	(See License)
+ */
+
+#ifndef __TFTP_H__
+#define __TFTP_H__
+
+/**********************************************************************/
+/*
+ *	Global functions and variables.
+ */
+
+/* tftp.c */
+void tftp_start(enum proto_t protocol);	/* Begin TFTP get/put */
+
+#ifdef CONFIG_CMD_TFTPSRV
+void tftp_start_server(void);	/* Wait for incoming TFTP put */
+#endif
+
+extern ulong tftp_timeout_ms;
+extern int tftp_timeout_count_max;
+
+/**********************************************************************/
+
+#endif /* __TFTP_H__ */
diff --git a/net/bootp.c b/net/bootp.c
index 43466af..b2f8ad4 100644
--- a/net/bootp.c
+++ b/net/bootp.c
@@ -11,8 +11,8 @@
 #include <common.h>
 #include <command.h>
 #include <net.h>
+#include <net/tftp.h>
 #include "bootp.h"
-#include "tftp.h"
 #include "nfs.h"
 #ifdef CONFIG_STATUS_LED
 #include <status_led.h>
diff --git a/net/net.c b/net/net.c
index 67e0ad2..61e010f 100644
--- a/net/net.c
+++ b/net/net.c
@@ -86,6 +86,7 @@
 #include <environment.h>
 #include <errno.h>
 #include <net.h>
+#include <net/tftp.h>
 #if defined(CONFIG_STATUS_LED)
 #include <miiphy.h>
 #include <status_led.h>
@@ -105,7 +106,6 @@
 #if defined(CONFIG_CMD_SNTP)
 #include "sntp.h"
 #endif
-#include "tftp.h"
 
 DECLARE_GLOBAL_DATA_PTR;
 
diff --git a/net/rarp.c b/net/rarp.c
index 4ce2f37..1fa11b6 100644
--- a/net/rarp.c
+++ b/net/rarp.c
@@ -8,10 +8,10 @@
 #include <common.h>
 #include <command.h>
 #include <net.h>
+#include <net/tftp.h>
 #include "nfs.h"
 #include "bootp.h"
 #include "rarp.h"
-#include "tftp.h"
 
 #define TIMEOUT 5000UL /* Milliseconds before trying BOOTP again */
 #ifndef	CONFIG_NET_RETRY_COUNT
diff --git a/net/tftp.c b/net/tftp.c
index 3e99e73..48ccceb 100644
--- a/net/tftp.c
+++ b/net/tftp.c
@@ -10,7 +10,7 @@
 #include <command.h>
 #include <mapmem.h>
 #include <net.h>
-#include "tftp.h"
+#include <net/tftp.h>
 #include "bootp.h"
 #ifdef CONFIG_SYS_DIRECT_FLASH_TFTP
 #include <flash.h>
diff --git a/net/tftp.h b/net/tftp.h
deleted file mode 100644
index c411c9b..0000000
--- a/net/tftp.h
+++ /dev/null
@@ -1,30 +0,0 @@
-/*
- *	LiMon - BOOTP/TFTP.
- *
- *	Copyright 1994, 1995, 2000 Neil Russell.
- *	Copyright 2011 Comelit Group SpA
- *	               Luca Ceresoli <luca.ceresoli@comelit.it>
- *	(See License)
- */
-
-#ifndef __TFTP_H__
-#define __TFTP_H__
-
-/**********************************************************************/
-/*
- *	Global functions and variables.
- */
-
-/* tftp.c */
-void tftp_start(enum proto_t protocol);	/* Begin TFTP get/put */
-
-#ifdef CONFIG_CMD_TFTPSRV
-void tftp_start_server(void);	/* Wait for incoming TFTP put */
-#endif
-
-extern ulong tftp_timeout_ms;
-extern int tftp_timeout_count_max;
-
-/**********************************************************************/
-
-#endif /* __TFTP_H__ */
-- 
2.1.4

[U-Boot] [PATCH v2 3/9] tftp: update: Allow some parts of the code to be reused when CONFIG_SYS_NO_FLASH is set

[Lukasz Majewski]

Up till now it was impossible to use code from update.c when system
was not equipped with raw FLASH memory.
Such behavior prevented DFU from reusing this code.

Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
Acked-by: Joe Hershberger <joe.hershberger@ni.com>

---
Changes for v2:
- None
---
 common/update.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/common/update.c b/common/update.c
index 1c6aa18..542915c 100644
--- a/common/update.c
+++ b/common/update.c
@@ -13,10 +13,6 @@
 #error "CONFIG_FIT and CONFIG_OF_LIBFDT are required for auto-update feature"
 #endif
 
-#if defined(CONFIG_SYS_NO_FLASH)
-#error "CONFIG_SYS_NO_FLASH defined, but FLASH is required for auto-update feature"
-#endif
-
 #include <command.h>
 #include <flash.h>
 #include <net.h>
@@ -41,11 +37,11 @@
 
 extern ulong tftp_timeout_ms;
 extern int tftp_timeout_count_max;
-extern flash_info_t flash_info[];
 extern ulong load_addr;
-
+#ifndef CONFIG_SYS_NO_FLASH
+extern flash_info_t flash_info[];
 static uchar *saved_prot_info;
-
+#endif
 static int update_load(char *filename, ulong msec_max, int cnt_max, ulong addr)
 {
 	int size, rv;
@@ -94,6 +90,7 @@ static int update_load(char *filename, ulong msec_max, int cnt_max, ulong addr)
 	return rv;
 }
 
+#ifndef CONFIG_SYS_NO_FLASH
 static int update_flash_protect(int prot, ulong addr_first, ulong addr_last)
 {
 	uchar *sp_info_ptr;
@@ -165,9 +162,11 @@ static int update_flash_protect(int prot, ulong addr_first, ulong addr_last)
 
 	return 0;
 }
+#endif
 
 static int update_flash(ulong addr_source, ulong addr_first, ulong size)
 {
+#ifndef CONFIG_SYS_NO_FLASH
 	ulong addr_last = addr_first + size - 1;
 
 	/* round last address to the sector boundary */
@@ -203,7 +202,7 @@ static int update_flash(ulong addr_source, ulong addr_first, ulong size)
 		printf("Error: could not protect flash sectors\n");
 		return 1;
 	}
-
+#endif
 	return 0;
 }
 
-- 
2.1.4

[U-Boot] [PATCH v2 4/9] dfu: tftp: update: Provide tftp support for the DFU subsystem

[Lukasz Majewski]

This commit adds initial support for using tftp for downloading and
upgrading firmware on the device.

Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
---
Changes for v2:
- Return -ENOSYS instead of plain -1
- Remove interface and devstring env variables reading in the dfu_tftp_write()
- Those parameters are now passed to dfu_tftp_write() function as its arguments
---
 drivers/dfu/Makefile   |  1 +
 drivers/dfu/dfu_tftp.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/dfu.h          | 13 ++++++++++
 3 files changed, 79 insertions(+)
 create mode 100644 drivers/dfu/dfu_tftp.c

diff --git a/drivers/dfu/Makefile b/drivers/dfu/Makefile
index cebea30..61f2b71 100644
--- a/drivers/dfu/Makefile
+++ b/drivers/dfu/Makefile
@@ -10,3 +10,4 @@ obj-$(CONFIG_DFU_MMC) += dfu_mmc.o
 obj-$(CONFIG_DFU_NAND) += dfu_nand.o
 obj-$(CONFIG_DFU_RAM) += dfu_ram.o
 obj-$(CONFIG_DFU_SF) += dfu_sf.o
+obj-$(CONFIG_DFU_TFTP) += dfu_tftp.o
diff --git a/drivers/dfu/dfu_tftp.c b/drivers/dfu/dfu_tftp.c
new file mode 100644
index 0000000..cd71708
--- /dev/null
+++ b/drivers/dfu/dfu_tftp.c
@@ -0,0 +1,65 @@
+/*
+ * (C) Copyright 2015
+ * Lukasz Majewski <l.majewski@majess.pl>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <common.h>
+#include <malloc.h>
+#include <errno.h>
+#include <dfu.h>
+
+int dfu_tftp_write(char *dfu_entity_name, unsigned int addr, unsigned int len,
+		   char *interface, char *devstring)
+{
+	char *s, *sb;
+	int alt_setting_num, ret;
+	struct dfu_entity *dfu;
+
+	debug("%s: name: %s addr: 0x%x len: %d device: %s:%s\n", __func__,
+	      dfu_entity_name, addr, len, interface, devstring);
+
+	ret = dfu_init_env_entities(interface, devstring);
+	if (ret)
+		goto done;
+
+	/*
+	 * We need to copy name pointed by *dfu_entity_name since this text
+	 * is the integral part of the FDT image.
+	 * Any implicit modification (i.e. done by strsep()) will corrupt
+	 * the FDT image and prevent other images to be stored.
+	 */
+	s = strdup(dfu_entity_name);
+	sb = s;
+	if (!s) {
+		ret = -ENOMEM;
+		goto done;
+	}
+
+	strsep(&s, "@");
+	debug("%s: image name: %s strlen: %d\n", __func__, sb, strlen(sb));
+
+	alt_setting_num = dfu_get_alt(sb);
+	free(sb);
+	if (alt_setting_num < 0) {
+		error("Alt setting [%d] to write not found!",
+		      alt_setting_num);
+		ret = -ENODEV;
+		goto done;
+	}
+
+	dfu = dfu_get_entity(alt_setting_num);
+	if (!dfu) {
+		error("DFU entity for alt: %d not found!", alt_setting_num);
+		ret = -ENODEV;
+		goto done;
+	}
+
+	ret = dfu_write_from_mem_addr(dfu, (void *)addr, len);
+
+done:
+	dfu_free_entities();
+
+	return ret;
+}
diff --git a/include/dfu.h b/include/dfu.h
index 7d31abd..7f78cc2 100644
--- a/include/dfu.h
+++ b/include/dfu.h
@@ -207,5 +207,18 @@ static inline int dfu_fill_entity_sf(struct dfu_entity *dfu, char *devstr,
 }
 #endif
 
+#ifdef CONFIG_DFU_TFTP
+int dfu_tftp_write(char *dfu_entity_name, unsigned int addr, unsigned int len,
+		   char *interface, char *devstring);
+#else
+static inline int dfu_tftp_write(char *dfu_entity_name, unsigned int addr,
+				 unsigned int len, char *interface,
+				 char *devstring)
+{
+	puts("TFTP write support for DFU not available!\n");
+	return -ENOSYS;
+}
+#endif
+
 int dfu_add(struct usb_configuration *c);
 #endif /* __DFU_ENTITY_H_ */
-- 
2.1.4

[U-Boot] [PATCH v2 5/9] dfu: tftp: update: Add dfu_write_from_mem_addr() function

[Lukasz Majewski]

This function allows writing via DFU data stored from fixed buffer address
(like e.g. loadaddr env variable).

Such predefined buffers are used in the update_tftp() code. In fact this
function is a wrapper on the dfu_write() and dfu_flush().

Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
---
Changes for v2:
- Use min() macro instead of comparison
- Change definition of left variable to be unsigned long - this allowed
  safe usage of min() macro
---
 drivers/dfu/dfu.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/dfu.h     |  1 +
 2 files changed, 49 insertions(+)

diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
index 2267dbf..c83ee41 100644
--- a/drivers/dfu/dfu.c
+++ b/drivers/dfu/dfu.c
@@ -568,3 +568,51 @@ int dfu_get_alt(char *name)
 
 	return -ENODEV;
 }
+
+/**
+ * dfu_write_from_mem_addr - this function adds support for writing data
+ *                           starting from fixed memory address (like $loadaddr)
+ *                           to dfu managed medium (e.g. NAND, MMC)
+ *
+ * @param dfu - dfu entity to which we want to store data
+ * @param buf - fixed memory addres from where data starts
+ * @param size - number of bytes to write
+ *
+ * @return - 0 on success, other value on failure
+ */
+int dfu_write_from_mem_addr(struct dfu_entity *dfu, void *buf, int size)
+{
+	unsigned long dfu_buf_size, write, left = size;
+	int i, ret = 0;
+	void *dp = buf;
+
+	/*
+	 * Here we must call dfu_get_buf(dfu) first to be sure that dfu_buf_size
+	 * has been properly initialized - e.g. if "dfu_bufsiz" has been taken
+	 * into account.
+	 */
+	dfu_get_buf(dfu);
+	dfu_buf_size = dfu_get_buf_size();
+	debug("%s: dfu buf size: %lu\n", __func__, dfu_buf_size);
+
+	for (i = 0; left > 0; i++) {
+		write = min(dfu_buf_size, left);
+
+		debug("%s: dp: 0x%p left: %lu write: %lu\n", __func__,
+		      dp, left, write);
+		ret = dfu_write(dfu, dp, write, i);
+		if (ret) {
+			error("DFU write failed\n");
+			return ret;
+		}
+
+		dp += write;
+		left -= write;
+	}
+
+	ret = dfu_flush(dfu, NULL, 0, i);
+	if (ret)
+		error("DFU flush failed!");
+
+	return ret;
+}
diff --git a/include/dfu.h b/include/dfu.h
index 7f78cc2..e624c41 100644
--- a/include/dfu.h
+++ b/include/dfu.h
@@ -162,6 +162,7 @@ bool dfu_usb_get_reset(void);
 int dfu_read(struct dfu_entity *de, void *buf, int size, int blk_seq_num);
 int dfu_write(struct dfu_entity *de, void *buf, int size, int blk_seq_num);
 int dfu_flush(struct dfu_entity *de, void *buf, int size, int blk_seq_num);
+int dfu_write_from_mem_addr(struct dfu_entity *dfu, void *buf, int size);
 /* Device specific */
 #ifdef CONFIG_DFU_MMC
 extern int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr, char *s);
-- 
2.1.4

[U-Boot] [PATCH v2 6/9] update: tftp: dfu: Extend update_tftp() function to support DFU

[Lukasz Majewski]

This code allows using DFU defined mediums for storing data received via
TFTP protocol.

It reuses and preserves functionality of legacy code at common/update.c.

The update_tftp() function now accepts parameters - namely medium device
name and its number (e.g. mmc 1).

Without this information passed old behavior is preserved.

Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
---
Changes for v2:
- Remove env variables from update_tftp() function
- Add parameters to update_tftp() function - without them old behavior is
  preserved
- Stop compilation when legacy flags (CONFIG_UPDATE_TFTP and CONFIG_SYS_NO_FLASH)
  are wrongly defined
- In the u-boot code legacy calls to update_tftp(0UL) have been changed to
  update_tftp(0UL, NULL, NULL)
---
 common/Makefile     |  1 +
 common/cmd_fitupd.c |  2 +-
 common/main.c       |  2 +-
 common/update.c     | 40 ++++++++++++++++++++++++++++++----------
 include/net.h       |  2 +-
 5 files changed, 34 insertions(+), 13 deletions(-)

diff --git a/common/Makefile b/common/Makefile
index d6c1d48..76626f1 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -208,6 +208,7 @@ obj-$(CONFIG_LYNXKDI) += lynxkdi.o
 obj-$(CONFIG_MENU) += menu.o
 obj-$(CONFIG_MODEM_SUPPORT) += modem.o
 obj-$(CONFIG_UPDATE_TFTP) += update.o
+obj-$(CONFIG_DFU_TFTP) += update.o
 obj-$(CONFIG_USB_KEYBOARD) += usb_kbd.o
 obj-$(CONFIG_CMD_DFU) += cmd_dfu.o
 obj-$(CONFIG_CMD_GPT) += cmd_gpt.o
diff --git a/common/cmd_fitupd.c b/common/cmd_fitupd.c
index b045974..78b8747 100644
--- a/common/cmd_fitupd.c
+++ b/common/cmd_fitupd.c
@@ -23,7 +23,7 @@ static int do_fitupd(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	if (argc == 2)
 		addr = simple_strtoul(argv[1], NULL, 16);
 
-	return update_tftp(addr);
+	return update_tftp(addr, NULL, NULL);
 }
 
 U_BOOT_CMD(fitupd, 2, 0, do_fitupd,
diff --git a/common/main.c b/common/main.c
index 2979fbe..ead0cd1 100644
--- a/common/main.c
+++ b/common/main.c
@@ -75,7 +75,7 @@ void main_loop(void)
 	run_preboot_environment_command();
 
 #if defined(CONFIG_UPDATE_TFTP)
-	update_tftp(0UL);
+	update_tftp(0UL, NULL, NULL);
 #endif /* CONFIG_UPDATE_TFTP */
 
 	s = bootdelay_process();
diff --git a/common/update.c b/common/update.c
index 542915c..1d082b0 100644
--- a/common/update.c
+++ b/common/update.c
@@ -13,11 +13,16 @@
 #error "CONFIG_FIT and CONFIG_OF_LIBFDT are required for auto-update feature"
 #endif
 
+#if defined(CONFIG_UPDATE_TFTP) && defined(CONFIG_SYS_NO_FLASH)
+#error "CONFIG_UPDATE_TFTP and CONFIG_SYS_NO_FLASH needed for legacy behaviour"
+#endif
+
 #include <command.h>
 #include <flash.h>
 #include <net.h>
 #include <net/tftp.h>
 #include <malloc.h>
+#include <dfu.h>
 
 /* env variable holding the location of the update file */
 #define UPDATE_FILE_ENV		"updatefile"
@@ -222,13 +227,17 @@ static int update_fit_getparams(const void *fit, int noffset, ulong *addr,
 	return 0;
 }
 
-int update_tftp(ulong addr)
+int update_tftp(ulong addr, char *interface, char *devstring)
 {
-	char *filename, *env_addr;
-	int images_noffset, ndepth, noffset;
+	char *filename, *env_addr, *fit_image_name;
 	ulong update_addr, update_fladdr, update_size;
-	void *fit;
+	int images_noffset, ndepth, noffset;
+	bool update_tftp_dfu = false;
 	int ret = 0;
+	void *fit;
+
+	if (interface && devstring)
+		update_tftp_dfu = true;
 
 	/* use already present image */
 	if (addr)
@@ -277,8 +286,8 @@ got_update_file:
 		if (ndepth != 1)
 			goto next_node;
 
-		printf("Processing update '%s' :",
-			fit_get_name(fit, noffset, NULL));
+		fit_image_name = (char *)fit_get_name(fit, noffset, NULL);
+		printf("Processing update '%s' :", fit_image_name);
 
 		if (!fit_image_verify(fit, noffset)) {
 			printf("Error: invalid update hash, aborting\n");
@@ -294,10 +303,21 @@ got_update_file:
 			ret = 1;
 			goto next_node;
 		}
-		if (update_flash(update_addr, update_fladdr, update_size)) {
-			printf("Error: can't flash update, aborting\n");
-			ret = 1;
-			goto next_node;
+
+		if (!update_tftp_dfu) {
+			if (update_flash(update_addr, update_fladdr,
+					 update_size)) {
+				printf("Error: can't flash update, aborting\n");
+				ret = 1;
+				goto next_node;
+			}
+		} else if (fit_image_check_type(fit, noffset,
+						IH_TYPE_FIRMWARE)) {
+			if (dfu_tftp_write(fit_image_name, update_addr,
+					   update_size, interface, devstring)) {
+				ret = 1;
+				goto next_node;
+			}
 		}
 next_node:
 		noffset = fdt_next_node(fit, noffset, &ndepth);
diff --git a/include/net.h b/include/net.h
index d17173d..9af3190 100644
--- a/include/net.h
+++ b/include/net.h
@@ -804,7 +804,7 @@ void copy_filename(char *dst, const char *src, int size);
 unsigned int random_port(void);
 
 /* Update U-Boot over TFTP */
-int update_tftp(ulong addr);
+int update_tftp(ulong addr, char *interface, char *devstring);
 
 /**********************************************************************/
 
-- 
2.1.4

[U-Boot] [PATCH v2 7/9] dfu: command: Extend "dfu" command to handle receiving data via TFTP

[Lukasz Majewski]

The "dfu" command has been extended to support transfers via TFTP protocol.

Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
---
Changes for v2:
- Remove "dfutftp" command
- Modify "dfu" command to support optional [tftp] parameter
- Only one flag (CONFIG_DFU_TFTP) needs to be enabled
---
 common/cmd_dfu.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c
index 857148f..aea466b 100644
--- a/common/cmd_dfu.c
+++ b/common/cmd_dfu.c
@@ -1,6 +1,9 @@
 /*
  * cmd_dfu.c -- dfu command
  *
+ * Copyright (C) 2015
+ * Lukasz Majewski <l.majewski@majess.pl>
+ *
  * Copyright (C) 2012 Samsung Electronics
  * authors: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
  *	    Lukasz Majewski <l.majewski@samsung.com>
@@ -13,6 +16,9 @@
 #include <dfu.h>
 #include <g_dnl.h>
 #include <usb.h>
+#ifdef CONFIG_DFU_TFTP
+#include <net.h>
+#endif
 
 static int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
@@ -26,7 +32,18 @@ static int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	char *devstring = argv[3];
 
 	int ret, i = 0;
+#ifdef CONFIG_DFU_TFTP
+	unsigned long addr = 0;
+	if (!strcmp(usb_controller, "tftp")) {
+		usb_controller = argv[2];
+		interface = argv[3];
+		devstring = argv[4];
+		if (argc == 6)
+			addr = simple_strtoul(argv[5], NULL, 0);
 
+		return update_tftp(addr, interface, devstring);
+	}
+#endif
 	ret = dfu_init_env_entities(interface, devstring);
 	if (ret)
 		goto done;
@@ -84,9 +101,17 @@ done:
 
 U_BOOT_CMD(dfu, CONFIG_SYS_MAXARGS, 1, do_dfu,
 	"Device Firmware Upgrade",
+#ifdef CONFIG_DFU_TFTP
+	"[tftp] <USB_controller> <interface> <dev> [list] [addr]\n"
+#else
 	"<USB_controller> <interface> <dev> [list]\n"
+#endif
 	"  - device firmware upgrade via <USB_controller>\n"
 	"    on device <dev>, attached to interface\n"
 	"    <interface>\n"
 	"    [list] - list available alt settings\n"
+#ifdef CONFIG_DFU_TFTP
+	"    [tftp] - use TFTP protocol to download data\n"
+	"    [addr] - address where FIT image has been stored\n"
+#endif
 );
-- 
2.1.4

[U-Boot] [PATCH v2 8/9] config: bbb: Configs necessary for running update via TFTP on Beagle Bone Black

[Lukasz Majewski]

Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
---
Changes for v2:
- Do not enable CONFIG_UPDATE_TFTP since CONFIG_DFU_TFTP enables the common code
- Do not enable CONFIG_CMD_DFUTFTP since dfutftp commands has been removed
---
 include/configs/am335x_evm.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/configs/am335x_evm.h b/include/configs/am335x_evm.h
index 633391b..be000fa 100644
--- a/include/configs/am335x_evm.h
+++ b/include/configs/am335x_evm.h
@@ -46,6 +46,14 @@
 #define CONFIG_CMD_GPT
 #define CONFIG_EFI_PARTITION
 
+/* Commands to enable DFU TFTP support */
+#define CONFIG_DFU_TFTP
+#define CONFIG_OF_LIBFDT
+
+/* Enable SHA1 support */
+#define CONFIG_SHA1
+#define CONFIG_CMD_SHA1SUM
+
 #ifdef CONFIG_NAND
 #define NANDARGS \
 	"mtdids=" MTDIDS_DEFAULT "\0" \
-- 
2.1.4

[U-Boot] [PATCH v2 9/9] dfu:tests: Modify dfu_gadget_test.sh to accept USB device major:minor number

[Lukasz Majewski]

In the dfu-util it is possible to set major:minor number by unsing -d flag
(-d 0451:d022).
Such option is very handy when many DFU devices are connected to a single
host PC. This commit allows testing when above situation emerges.

Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
---
Changes for v2:
- New patch
---
 test/dfu/README             |  9 ++++++++-
 test/dfu/dfu_gadget_test.sh | 18 +++++++++++-------
 2 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/test/dfu/README b/test/dfu/README
index 5176aba..8925a91 100644
--- a/test/dfu/README
+++ b/test/dfu/README
@@ -26,12 +26,19 @@ Example usage:
    setenv dfu_alt_info dfu_test.bin fat 0 6\;dfudummy.bin fat 0 6
    dfu 0 mmc 0
 2. On the host:
-   test/dfu/dfu_gadget_test.sh X Y  [test file name]
+   test/dfu/dfu_gadget_test.sh X Y [test file name] [usb device major:minor]
    e.g. test/dfu/dfu_gadget_test.sh 0 1
    or
    e.g. test/dfu/dfu_gadget_test.sh 0 1 ./dat_960.img
+   or
+   e.g. test/dfu/dfu_gadget_test.sh 0 1 0451:d022
+   or
+   e.g. test/dfu/dfu_gadget_test.sh 0 1 ./dat_960.img 0451:d022
 
 ... where X and Y are dfu_test.bin's and dfudummy.bin's alt setting numbers.
 They can be obtained from dfu-util -l or $dfu_alt_info.
 It is also possible to pass optional [test file name] to force the script to
 test one particular file.
+If many DFU devices are connected, it is necessary to specify USB device major
+and minor numbers (0451:d022). One can get them by running "lsusb" command on
+a host PC.
diff --git a/test/dfu/dfu_gadget_test.sh b/test/dfu/dfu_gadget_test.sh
index 2f5b7db..9c79422 100755
--- a/test/dfu/dfu_gadget_test.sh
+++ b/test/dfu/dfu_gadget_test.sh
@@ -45,18 +45,18 @@ dfu_test_file () {
     printf "$COLOUR_GREEN ========================================================================================= $COLOUR_DEFAULT\n"
     printf "File:$COLOUR_GREEN %s $COLOUR_DEFAULT\n" $1
 
-    dfu-util -D $1 -a $TARGET_ALT_SETTING >> $LOG_FILE 2>&1 || die $?
+    dfu-util $USB_DEV -D $1 -a $TARGET_ALT_SETTING >> $LOG_FILE 2>&1 || die $?
 
     echo -n "TX: "
     calculate_md5sum $1
 
     MD5_TX=$MD5SUM
 
-    dfu-util -D ${DIR}/dfudummy.bin -a $TARGET_ALT_SETTING_B >> $LOG_FILE 2>&1 || die $?
+    dfu-util $USB_DEV -D ${DIR}/dfudummy.bin -a $TARGET_ALT_SETTING_B >> $LOG_FILE 2>&1 || die $?
 
     N_FILE=$DIR$RCV_DIR${1:2}"_rcv"
 
-    dfu-util -U $N_FILE -a $TARGET_ALT_SETTING >> $LOG_FILE 2>&1 || die $?
+    dfu-util $USB_DEV -U $N_FILE -a $TARGET_ALT_SETTING >> $LOG_FILE 2>&1 || die $?
 
     echo -n "RX: "
     calculate_md5sum $N_FILE
@@ -89,13 +89,17 @@ fi
 TARGET_ALT_SETTING=$1
 TARGET_ALT_SETTING_B=$2
 
-if [ -n "$3" ]
+file=$3
+[[ $3 == *':'* ]] && USB_DEV="-d $3" && file=""
+[ $# -eq 4 ] && USB_DEV="-d $4"
+
+if [ -n "$file" ]
 then
-	dfu_test_file $3
+	dfu_test_file $file
 else
-	for file in $DIR*.$SUFFIX
+	for f in $DIR*.$SUFFIX
 	do
-	    dfu_test_file $file
+	    dfu_test_file $f
 	done
 fi
 
-- 
2.1.4

[U-Boot] [PATCH v2 2/9] net: tftp: Move tftp.h file from ./net to ./include/net

[Wolfgang Denk]

Dear Lukasz Majewski,

In message <1437811877-13764-3-git-send-email-l.majewski@majess.pl> you wrote:
> This change gives the ability to reuse the <tftp.h> header file by other
> subsystems (like e.g. dfu).
> 
> Without this change compilation error emerges for the legacy update.c file.
> 
> Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> 
> ---
> Changes for v2:
> - Move tftp.h from ./include to ./include/net/ directory
> ---
>  include/net/tftp.h | 30 ++++++++++++++++++++++++++++++
>  net/bootp.c        |  2 +-
>  net/net.c          |  2 +-
>  net/rarp.c         |  2 +-
>  net/tftp.c         |  2 +-
>  net/tftp.h         | 30 ------------------------------
>  6 files changed, 34 insertions(+), 34 deletions(-)
>  create mode 100644 include/net/tftp.h
>  delete mode 100644 net/tftp.h

NAK.  Please resubmit and use "-M -C" when generating the patches, so
the renames will be properly detected and not result in a file
deletion and the addition of an (apparently new) file.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
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
Unsichtbar macht sich die Dummheit, indem sie immer  gr??ere  Ausma?e
annimmt.                             -- Bertold Brecht: Der Tui-Roman

[U-Boot] [PATCH v2 2/9] net: tftp: Move tftp.h file from ./net to ./include/net

[Lukasz Majewski]

On Sat, 25 Jul 2015 14:24:43 +0200
Wolfgang Denk <wd@denx.de> wrote:

> Dear Lukasz Majewski,
> 
> In message <1437811877-13764-3-git-send-email-l.majewski@majess.pl>
> you wrote:
> > This change gives the ability to reuse the <tftp.h> header file by
> > other subsystems (like e.g. dfu).
> > 
> > Without this change compilation error emerges for the legacy
> > update.c file.
> > 
> > Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> > 
> > ---
> > Changes for v2:
> > - Move tftp.h from ./include to ./include/net/ directory
> > ---
> >  include/net/tftp.h | 30 ++++++++++++++++++++++++++++++
> >  net/bootp.c        |  2 +-
> >  net/net.c          |  2 +-
> >  net/rarp.c         |  2 +-
> >  net/tftp.c         |  2 +-
> >  net/tftp.h         | 30 ------------------------------
> >  6 files changed, 34 insertions(+), 34 deletions(-)
> >  create mode 100644 include/net/tftp.h
> >  delete mode 100644 net/tftp.h
> 
> NAK.  Please resubmit and use "-M -C" when generating the patches, so
> the renames will be properly detected and not result in a file
> deletion and the addition of an (apparently new) file.

Ok, Thanks for pointing this out.

> 
> Best regards,
> 
> Wolfgang Denk
> 

Best regards,
Lukasz Majewski
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150725/7b9ccc18/attachment.sig>

[U-Boot] [PATCH v2 2/9] net: tftp: Move tftp.h file from ./net to ./include/net

[Simon Glass]

[prune cc]

Hi Lukasz,

On 25 July 2015 at 09:02, Lukasz Majewski <l.majewski@majess.pl> wrote:
> On Sat, 25 Jul 2015 14:24:43 +0200
> Wolfgang Denk <wd@denx.de> wrote:
>
>> Dear Lukasz Majewski,
>>
>> In message <1437811877-13764-3-git-send-email-l.majewski@majess.pl>
>> you wrote:
>> > This change gives the ability to reuse the <tftp.h> header file by
>> > other subsystems (like e.g. dfu).
>> >
>> > Without this change compilation error emerges for the legacy
>> > update.c file.
>> >
>> > Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
>> >
>> > ---
>> > Changes for v2:
>> > - Move tftp.h from ./include to ./include/net/ directory
>> > ---
>> >  include/net/tftp.h | 30 ++++++++++++++++++++++++++++++
>> >  net/bootp.c        |  2 +-
>> >  net/net.c          |  2 +-
>> >  net/rarp.c         |  2 +-
>> >  net/tftp.c         |  2 +-
>> >  net/tftp.h         | 30 ------------------------------
>> >  6 files changed, 34 insertions(+), 34 deletions(-)
>> >  create mode 100644 include/net/tftp.h
>> >  delete mode 100644 net/tftp.h
>>
>> NAK.  Please resubmit and use "-M -C" when generating the patches, so
>> the renames will be properly detected and not result in a file
>> deletion and the addition of an (apparently new) file.
>
> Ok, Thanks for pointing this out.

Or use patman, which does this for you :-)

Regards,
Simon

[U-Boot] [PATCH v2 1/9] doc: dfu: tftp: README entry for TFTP extension of DFU

[Simon Glass]

Hi Lukasz,

On 25 July 2015 at 02:11, Lukasz Majewski <l.majewski@majess.pl> wrote:
> Documentation file for DFU extension. With this functionality it is now
> possible to transfer FIT images with firmware updates via TFTP and use
> DFU backend for storing them.
>
> Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> ---
> Changes for v2:
> - Remove section regarding update_tftp() specific environment variables
> ---
>  doc/README.dfutftp | 113 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 113 insertions(+)
>  create mode 100644 doc/README.dfutftp

Reviewed-by: Simon Glass <sjg@chromium.org>

Can you add a link to your new doc in README.update?

Regards,
Simon

[U-Boot] [PATCH v2 3/9] tftp: update: Allow some parts of the code to be reused when CONFIG_SYS_NO_FLASH is set

[Simon Glass]

On 25 July 2015 at 02:11, Lukasz Majewski <l.majewski@majess.pl> wrote:
> Up till now it was impossible to use code from update.c when system
> was not equipped with raw FLASH memory.
> Such behavior prevented DFU from reusing this code.
>
> Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> Acked-by: Joe Hershberger <joe.hershberger@ni.com>
>
> ---
> Changes for v2:
> - None
> ---
>  common/update.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH v2 4/9] dfu: tftp: update: Provide tftp support for the DFU subsystem

[Simon Glass]

Hi Lukasz,

On 25 July 2015 at 02:11, Lukasz Majewski <l.majewski@majess.pl> wrote:
> This commit adds initial support for using tftp for downloading and
> upgrading firmware on the device.
>
> Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> ---
> Changes for v2:
> - Return -ENOSYS instead of plain -1
> - Remove interface and devstring env variables reading in the dfu_tftp_write()
> - Those parameters are now passed to dfu_tftp_write() function as its arguments
> ---
>  drivers/dfu/Makefile   |  1 +
>  drivers/dfu/dfu_tftp.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/dfu.h          | 13 ++++++++++
>  3 files changed, 79 insertions(+)
>  create mode 100644 drivers/dfu/dfu_tftp.c

Is there a Kconfig option needed here?

Regards,
Simon

[U-Boot] [PATCH v2 6/9] update: tftp: dfu: Extend update_tftp() function to support DFU

[Simon Glass]

Hi Lukasz,

On 25 July 2015 at 02:11, Lukasz Majewski <l.majewski@majess.pl> wrote:
> This code allows using DFU defined mediums for storing data received via
> TFTP protocol.
>
> It reuses and preserves functionality of legacy code at common/update.c.
>
> The update_tftp() function now accepts parameters - namely medium device
> name and its number (e.g. mmc 1).
>
> Without this information passed old behavior is preserved.
>
> Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> ---
> Changes for v2:
> - Remove env variables from update_tftp() function
> - Add parameters to update_tftp() function - without them old behavior is
>   preserved
> - Stop compilation when legacy flags (CONFIG_UPDATE_TFTP and CONFIG_SYS_NO_FLASH)
>   are wrongly defined
> - In the u-boot code legacy calls to update_tftp(0UL) have been changed to
>   update_tftp(0UL, NULL, NULL)
> ---
>  common/Makefile     |  1 +
>  common/cmd_fitupd.c |  2 +-
>  common/main.c       |  2 +-
>  common/update.c     | 40 ++++++++++++++++++++++++++++++----------
>  include/net.h       |  2 +-
>  5 files changed, 34 insertions(+), 13 deletions(-)
>
> diff --git a/common/Makefile b/common/Makefile
> index d6c1d48..76626f1 100644
> --- a/common/Makefile
> +++ b/common/Makefile
> @@ -208,6 +208,7 @@ obj-$(CONFIG_LYNXKDI) += lynxkdi.o
>  obj-$(CONFIG_MENU) += menu.o
>  obj-$(CONFIG_MODEM_SUPPORT) += modem.o
>  obj-$(CONFIG_UPDATE_TFTP) += update.o
> +obj-$(CONFIG_DFU_TFTP) += update.o
>  obj-$(CONFIG_USB_KEYBOARD) += usb_kbd.o
>  obj-$(CONFIG_CMD_DFU) += cmd_dfu.o
>  obj-$(CONFIG_CMD_GPT) += cmd_gpt.o
> diff --git a/common/cmd_fitupd.c b/common/cmd_fitupd.c
> index b045974..78b8747 100644
> --- a/common/cmd_fitupd.c
> +++ b/common/cmd_fitupd.c
> @@ -23,7 +23,7 @@ static int do_fitupd(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>         if (argc == 2)
>                 addr = simple_strtoul(argv[1], NULL, 16);
>
> -       return update_tftp(addr);
> +       return update_tftp(addr, NULL, NULL);
>  }
>
>  U_BOOT_CMD(fitupd, 2, 0, do_fitupd,
> diff --git a/common/main.c b/common/main.c
> index 2979fbe..ead0cd1 100644
> --- a/common/main.c
> +++ b/common/main.c
> @@ -75,7 +75,7 @@ void main_loop(void)
>         run_preboot_environment_command();
>
>  #if defined(CONFIG_UPDATE_TFTP)
> -       update_tftp(0UL);
> +       update_tftp(0UL, NULL, NULL);
>  #endif /* CONFIG_UPDATE_TFTP */
>
>         s = bootdelay_process();
> diff --git a/common/update.c b/common/update.c
> index 542915c..1d082b0 100644
> --- a/common/update.c
> +++ b/common/update.c
> @@ -13,11 +13,16 @@
>  #error "CONFIG_FIT and CONFIG_OF_LIBFDT are required for auto-update feature"
>  #endif
>
> +#if defined(CONFIG_UPDATE_TFTP) && defined(CONFIG_SYS_NO_FLASH)
> +#error "CONFIG_UPDATE_TFTP and CONFIG_SYS_NO_FLASH needed for legacy behaviour"
> +#endif
> +
>  #include <command.h>
>  #include <flash.h>
>  #include <net.h>
>  #include <net/tftp.h>
>  #include <malloc.h>
> +#include <dfu.h>
>
>  /* env variable holding the location of the update file */
>  #define UPDATE_FILE_ENV                "updatefile"
> @@ -222,13 +227,17 @@ static int update_fit_getparams(const void *fit, int noffset, ulong *addr,
>         return 0;
>  }
>
> -int update_tftp(ulong addr)
> +int update_tftp(ulong addr, char *interface, char *devstring)

Should these be const char *?

>  {
> -       char *filename, *env_addr;
> -       int images_noffset, ndepth, noffset;
> +       char *filename, *env_addr, *fit_image_name;

And these?

>         ulong update_addr, update_fladdr, update_size;
> -       void *fit;
> +       int images_noffset, ndepth, noffset;
> +       bool update_tftp_dfu = false;
>         int ret = 0;
> +       void *fit;
> +
> +       if (interface && devstring)
> +               update_tftp_dfu = true;
>
>         /* use already present image */
>         if (addr)
> @@ -277,8 +286,8 @@ got_update_file:
>                 if (ndepth != 1)
>                         goto next_node;
>
> -               printf("Processing update '%s' :",
> -                       fit_get_name(fit, noffset, NULL));
> +               fit_image_name = (char *)fit_get_name(fit, noffset, NULL);

Can you drop that cast?

> +               printf("Processing update '%s' :", fit_image_name);
>
>                 if (!fit_image_verify(fit, noffset)) {
>                         printf("Error: invalid update hash, aborting\n");
> @@ -294,10 +303,21 @@ got_update_file:
>                         ret = 1;
>                         goto next_node;
>                 }
> -               if (update_flash(update_addr, update_fladdr, update_size)) {
> -                       printf("Error: can't flash update, aborting\n");
> -                       ret = 1;
> -                       goto next_node;
> +
> +               if (!update_tftp_dfu) {
> +                       if (update_flash(update_addr, update_fladdr,
> +                                        update_size)) {
> +                               printf("Error: can't flash update, aborting\n");
> +                               ret = 1;
> +                               goto next_node;
> +                       }
> +               } else if (fit_image_check_type(fit, noffset,
> +                                               IH_TYPE_FIRMWARE)) {
> +                       if (dfu_tftp_write(fit_image_name, update_addr,
> +                                          update_size, interface, devstring)) {
> +                               ret = 1;
> +                               goto next_node;
> +                       }
>                 }
>  next_node:
>                 noffset = fdt_next_node(fit, noffset, &ndepth);
> diff --git a/include/net.h b/include/net.h
> index d17173d..9af3190 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -804,7 +804,7 @@ void copy_filename(char *dst, const char *src, int size);
>  unsigned int random_port(void);
>
>  /* Update U-Boot over TFTP */
> -int update_tftp(ulong addr);
> +int update_tftp(ulong addr, char *interface, char *devstring);

Function comment - what are the parameters?

>
>  /**********************************************************************/
>
> --
> 2.1.4
>

Regards,
Simon

[U-Boot] [PATCH v2 5/9] dfu: tftp: update: Add dfu_write_from_mem_addr() function

[Simon Glass]

Hi Lukasz,

On 25 July 2015 at 02:11, Lukasz Majewski <l.majewski@majess.pl> wrote:
> This function allows writing via DFU data stored from fixed buffer address
> (like e.g. loadaddr env variable).
>
> Such predefined buffers are used in the update_tftp() code. In fact this
> function is a wrapper on the dfu_write() and dfu_flush().
>
> Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> ---
> Changes for v2:
> - Use min() macro instead of comparison
> - Change definition of left variable to be unsigned long - this allowed
>   safe usage of min() macro
> ---
>  drivers/dfu/dfu.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/dfu.h     |  1 +
>  2 files changed, 49 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

But see nits below.

>
> diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
> index 2267dbf..c83ee41 100644
> --- a/drivers/dfu/dfu.c
> +++ b/drivers/dfu/dfu.c
> @@ -568,3 +568,51 @@ int dfu_get_alt(char *name)
>
>         return -ENODEV;
>  }
> +
> +/**
> + * dfu_write_from_mem_addr - this function adds support for writing data
> + *                           starting from fixed memory address (like $loadaddr)
> + *                           to dfu managed medium (e.g. NAND, MMC)

I think it's better to start with a direct one-line description, like:

+ * dfu_write_from_mem_addr - wite data to dfu-managed medium
+ *
+ * More detail goes here ...

> + *
> + * @param dfu - dfu entity to which we want to store data
> + * @param buf - fixed memory addres from where data starts
> + * @param size - number of bytes to write
> + *
> + * @return - 0 on success, other value on failure
> + */
> +int dfu_write_from_mem_addr(struct dfu_entity *dfu, void *buf, int size)

const void *buf? My understanding is that this buffer is not changed.

> +{
> +       unsigned long dfu_buf_size, write, left = size;
> +       int i, ret = 0;
> +       void *dp = buf;
> +
> +       /*
> +        * Here we must call dfu_get_buf(dfu) first to be sure that dfu_buf_size
> +        * has been properly initialized - e.g. if "dfu_bufsiz" has been taken
> +        * into account.
> +        */
> +       dfu_get_buf(dfu);
> +       dfu_buf_size = dfu_get_buf_size();
> +       debug("%s: dfu buf size: %lu\n", __func__, dfu_buf_size);
> +
> +       for (i = 0; left > 0; i++) {
> +               write = min(dfu_buf_size, left);
> +
> +               debug("%s: dp: 0x%p left: %lu write: %lu\n", __func__,
> +                     dp, left, write);
> +               ret = dfu_write(dfu, dp, write, i);
> +               if (ret) {
> +                       error("DFU write failed\n");
> +                       return ret;
> +               }
> +
> +               dp += write;
> +               left -= write;
> +       }
> +
> +       ret = dfu_flush(dfu, NULL, 0, i);
> +       if (ret)
> +               error("DFU flush failed!");
> +
> +       return ret;
> +}
> diff --git a/include/dfu.h b/include/dfu.h
> index 7f78cc2..e624c41 100644
> --- a/include/dfu.h
> +++ b/include/dfu.h
> @@ -162,6 +162,7 @@ bool dfu_usb_get_reset(void);
>  int dfu_read(struct dfu_entity *de, void *buf, int size, int blk_seq_num);
>  int dfu_write(struct dfu_entity *de, void *buf, int size, int blk_seq_num);
>  int dfu_flush(struct dfu_entity *de, void *buf, int size, int blk_seq_num);

Please can you put your function comment here in the header?

> +int dfu_write_from_mem_addr(struct dfu_entity *dfu, void *buf, int size);
>  /* Device specific */
>  #ifdef CONFIG_DFU_MMC
>  extern int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr, char *s);
> --
> 2.1.4
>

Regards,
Simon

[U-Boot] [PATCH v2 7/9] dfu: command: Extend "dfu" command to handle receiving data via TFTP

[Simon Glass]

Hi Lukasz,

On 25 July 2015 at 02:11, Lukasz Majewski <l.majewski@majess.pl> wrote:
> The "dfu" command has been extended to support transfers via TFTP protocol.
>
> Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> ---
> Changes for v2:
> - Remove "dfutftp" command
> - Modify "dfu" command to support optional [tftp] parameter
> - Only one flag (CONFIG_DFU_TFTP) needs to be enabled
> ---
>  common/cmd_dfu.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

See below.

>
> diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c
> index 857148f..aea466b 100644
> --- a/common/cmd_dfu.c
> +++ b/common/cmd_dfu.c
> @@ -1,6 +1,9 @@
>  /*
>   * cmd_dfu.c -- dfu command
>   *
> + * Copyright (C) 2015
> + * Lukasz Majewski <l.majewski@majess.pl>
> + *
>   * Copyright (C) 2012 Samsung Electronics
>   * authors: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
>   *         Lukasz Majewski <l.majewski@samsung.com>
> @@ -13,6 +16,9 @@
>  #include <dfu.h>
>  #include <g_dnl.h>
>  #include <usb.h>
> +#ifdef CONFIG_DFU_TFTP
> +#include <net.h>
> +#endif
>
>  static int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  {
> @@ -26,7 +32,18 @@ static int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>         char *devstring = argv[3];
>
>         int ret, i = 0;
> +#ifdef CONFIG_DFU_TFTP
> +       unsigned long addr = 0;
> +       if (!strcmp(usb_controller, "tftp")) {
> +               usb_controller = argv[2];
> +               interface = argv[3];
> +               devstring = argv[4];
> +               if (argc == 6)
> +                       addr = simple_strtoul(argv[5], NULL, 0);
>
> +               return update_tftp(addr, interface, devstring);
> +       }
> +#endif
>         ret = dfu_init_env_entities(interface, devstring);
>         if (ret)
>                 goto done;
> @@ -84,9 +101,17 @@ done:
>
>  U_BOOT_CMD(dfu, CONFIG_SYS_MAXARGS, 1, do_dfu,
>         "Device Firmware Upgrade",
> +#ifdef CONFIG_DFU_TFTP
> +       "[tftp] <USB_controller> <interface> <dev> [list] [addr]\n"

It's a bit confusing since tftp looks like a parameter but I think you
mean we should type it. Maybe it would be better to have:

+       "[tftp] <USB_controller> <interface> <dev> [<list>] [<addr>]\n"


> +#else
>         "<USB_controller> <interface> <dev> [list]\n"
> +#endif
>         "  - device firmware upgrade via <USB_controller>\n"
>         "    on device <dev>, attached to interface\n"
>         "    <interface>\n"
>         "    [list] - list available alt settings\n"
> +#ifdef CONFIG_DFU_TFTP
> +       "    [tftp] - use TFTP protocol to download data\n"
> +       "    [addr] - address where FIT image has been stored\n"
> +#endif
>  );
> --
> 2.1.4
>

Regards,
Simon

[U-Boot] [PATCH v2 8/9] config: bbb: Configs necessary for running update via TFTP on Beagle Bone Black

[Simon Glass]

HI Lukasz,

On 25 July 2015 at 02:11, Lukasz Majewski <l.majewski@majess.pl> wrote:

Commit message? E.g. 'Enable DFU via update on Beaglebone Black'

> Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> ---
> Changes for v2:
> - Do not enable CONFIG_UPDATE_TFTP since CONFIG_DFU_TFTP enables the common code
> - Do not enable CONFIG_CMD_DFUTFTP since dfutftp commands has been removed
> ---
>  include/configs/am335x_evm.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/include/configs/am335x_evm.h b/include/configs/am335x_evm.h
> index 633391b..be000fa 100644
> --- a/include/configs/am335x_evm.h
> +++ b/include/configs/am335x_evm.h
> @@ -46,6 +46,14 @@
>  #define CONFIG_CMD_GPT
>  #define CONFIG_EFI_PARTITION
>
> +/* Commands to enable DFU TFTP support */
> +#define CONFIG_DFU_TFTP

Can this go in Kconfig and then the board's defconfig?

> +#define CONFIG_OF_LIBFDT
> +
> +/* Enable SHA1 support */
> +#define CONFIG_SHA1
> +#define CONFIG_CMD_SHA1SUM
> +
>  #ifdef CONFIG_NAND
>  #define NANDARGS \
>         "mtdids=" MTDIDS_DEFAULT "\0" \
> --
> 2.1.4
>
Regards,

Simon

[U-Boot] [PATCH v2 9/9] dfu:tests: Modify dfu_gadget_test.sh to accept USB device major:minor number

[Simon Glass]

Hi Lukasz,

On 25 July 2015 at 02:11, Lukasz Majewski <l.majewski@majess.pl> wrote:
> In the dfu-util it is possible to set major:minor number by unsing -d flag

using

> (-d 0451:d022).
> Such option is very handy when many DFU devices are connected to a single
> host PC. This commit allows testing when above situation emerges.

Does this test cover the new tftp feature?

>
> Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> ---
> Changes for v2:
> - New patch

Reviewed-by: Simon Glass <sjg@chromium.org>

> ---
>  test/dfu/README             |  9 ++++++++-
>  test/dfu/dfu_gadget_test.sh | 18 +++++++++++-------
>  2 files changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/test/dfu/README b/test/dfu/README
> index 5176aba..8925a91 100644
> --- a/test/dfu/README
> +++ b/test/dfu/README
> @@ -26,12 +26,19 @@ Example usage:
>     setenv dfu_alt_info dfu_test.bin fat 0 6\;dfudummy.bin fat 0 6
>     dfu 0 mmc 0
>  2. On the host:
> -   test/dfu/dfu_gadget_test.sh X Y  [test file name]
> +   test/dfu/dfu_gadget_test.sh X Y [test file name] [usb device major:minor]
>     e.g. test/dfu/dfu_gadget_test.sh 0 1
>     or
>     e.g. test/dfu/dfu_gadget_test.sh 0 1 ./dat_960.img
> +   or
> +   e.g. test/dfu/dfu_gadget_test.sh 0 1 0451:d022
> +   or
> +   e.g. test/dfu/dfu_gadget_test.sh 0 1 ./dat_960.img 0451:d022
>
>  ... where X and Y are dfu_test.bin's and dfudummy.bin's alt setting numbers.
>  They can be obtained from dfu-util -l or $dfu_alt_info.
>  It is also possible to pass optional [test file name] to force the script to
>  test one particular file.
> +If many DFU devices are connected, it is necessary to specify USB device major
> +and minor numbers (0451:d022). One can get them by running "lsusb" command on
> +a host PC.
> diff --git a/test/dfu/dfu_gadget_test.sh b/test/dfu/dfu_gadget_test.sh
> index 2f5b7db..9c79422 100755
> --- a/test/dfu/dfu_gadget_test.sh
> +++ b/test/dfu/dfu_gadget_test.sh
> @@ -45,18 +45,18 @@ dfu_test_file () {
>      printf "$COLOUR_GREEN ========================================================================================= $COLOUR_DEFAULT\n"
>      printf "File:$COLOUR_GREEN %s $COLOUR_DEFAULT\n" $1
>
> -    dfu-util -D $1 -a $TARGET_ALT_SETTING >> $LOG_FILE 2>&1 || die $?
> +    dfu-util $USB_DEV -D $1 -a $TARGET_ALT_SETTING >> $LOG_FILE 2>&1 || die $?
>
>      echo -n "TX: "
>      calculate_md5sum $1
>
>      MD5_TX=$MD5SUM
>
> -    dfu-util -D ${DIR}/dfudummy.bin -a $TARGET_ALT_SETTING_B >> $LOG_FILE 2>&1 || die $?
> +    dfu-util $USB_DEV -D ${DIR}/dfudummy.bin -a $TARGET_ALT_SETTING_B >> $LOG_FILE 2>&1 || die $?
>
>      N_FILE=$DIR$RCV_DIR${1:2}"_rcv"
>
> -    dfu-util -U $N_FILE -a $TARGET_ALT_SETTING >> $LOG_FILE 2>&1 || die $?
> +    dfu-util $USB_DEV -U $N_FILE -a $TARGET_ALT_SETTING >> $LOG_FILE 2>&1 || die $?
>
>      echo -n "RX: "
>      calculate_md5sum $N_FILE
> @@ -89,13 +89,17 @@ fi
>  TARGET_ALT_SETTING=$1
>  TARGET_ALT_SETTING_B=$2
>
> -if [ -n "$3" ]
> +file=$3
> +[[ $3 == *':'* ]] && USB_DEV="-d $3" && file=""
> +[ $# -eq 4 ] && USB_DEV="-d $4"
> +
> +if [ -n "$file" ]
>  then
> -       dfu_test_file $3
> +       dfu_test_file $file
>  else
> -       for file in $DIR*.$SUFFIX
> +       for f in $DIR*.$SUFFIX
>         do
> -           dfu_test_file $file
> +           dfu_test_file $f
>         done
>  fi
>
> --
> 2.1.4
>

Regards,
Simon

[U-Boot] [PATCH v2 1/9] doc: dfu: tftp: README entry for TFTP extension of DFU

[Lukasz Majewski]

Hi Simon,

> Hi Lukasz,
> 
> On 25 July 2015 at 02:11, Lukasz Majewski <l.majewski@majess.pl>
> wrote:
> > Documentation file for DFU extension. With this functionality it is
> > now possible to transfer FIT images with firmware updates via TFTP
> > and use DFU backend for storing them.
> >
> > Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> > ---
> > Changes for v2:
> > - Remove section regarding update_tftp() specific environment
> > variables ---
> >  doc/README.dfutftp | 113
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file
> > changed, 113 insertions(+) create mode 100644 doc/README.dfutftp
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> Can you add a link to your new doc in README.update?

Simon, thanks for your review. I will correct my patches and send
v3.

> 
> Regards,
> Simon

Best regards,
Lukasz
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150803/5d7b6325/attachment.sig>

[U-Boot] [PATCH v2 4/9] dfu: tftp: update: Provide tftp support for the DFU subsystem

[Joe Hershberger]

Hi Lukasz,

On Sat, Jul 25, 2015 at 3:11 AM, Lukasz Majewski <l.majewski@majess.pl> wrote:
> This commit adds initial support for using tftp for downloading and
> upgrading firmware on the device.
>
> Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> ---
> Changes for v2:
> - Return -ENOSYS instead of plain -1
> - Remove interface and devstring env variables reading in the dfu_tftp_write()
> - Those parameters are now passed to dfu_tftp_write() function as its arguments
> ---

Acked-by: Joe Hershberger <joe.hershberger@ni.com>

[U-Boot] [PATCH v2 5/9] dfu: tftp: update: Add dfu_write_from_mem_addr() function

[Joe Hershberger]

Hi Lukasz,

On Sat, Jul 25, 2015 at 3:11 AM, Lukasz Majewski <l.majewski@majess.pl> wrote:
> This function allows writing via DFU data stored from fixed buffer address
> (like e.g. loadaddr env variable).
>
> Such predefined buffers are used in the update_tftp() code. In fact this
> function is a wrapper on the dfu_write() and dfu_flush().
>
> Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> ---
> Changes for v2:
> - Use min() macro instead of comparison
> - Change definition of left variable to be unsigned long - this allowed
>   safe usage of min() macro
> ---

Address Simon's nits and then,
Acked-by: Joe Hershberger <joe.hershberger@ni.com>

[U-Boot] [PATCH v2 6/9] update: tftp: dfu: Extend update_tftp() function to support DFU

[Joe Hershberger]

Hi Lukasz,

On Sat, Jul 25, 2015 at 3:11 AM, Lukasz Majewski <l.majewski@majess.pl> wrote:
> This code allows using DFU defined mediums for storing data received via
> TFTP protocol.
>
> It reuses and preserves functionality of legacy code at common/update.c.
>
> The update_tftp() function now accepts parameters - namely medium device
> name and its number (e.g. mmc 1).
>
> Without this information passed old behavior is preserved.
>
> Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> ---
> Changes for v2:
> - Remove env variables from update_tftp() function
> - Add parameters to update_tftp() function - without them old behavior is
>   preserved
> - Stop compilation when legacy flags (CONFIG_UPDATE_TFTP and CONFIG_SYS_NO_FLASH)
>   are wrongly defined
> - In the u-boot code legacy calls to update_tftp(0UL) have been changed to
>   update_tftp(0UL, NULL, NULL)
> ---
>  common/Makefile     |  1 +
>  common/cmd_fitupd.c |  2 +-
>  common/main.c       |  2 +-
>  common/update.c     | 40 ++++++++++++++++++++++++++++++----------
>  include/net.h       |  2 +-
>  5 files changed, 34 insertions(+), 13 deletions(-)

Address Simon's nits and then,
Acked-by: Joe Hershberger <joe.hershberger@ni.com>

> diff --git a/common/Makefile b/common/Makefile
> index d6c1d48..76626f1 100644
> --- a/common/Makefile
> +++ b/common/Makefile
> @@ -208,6 +208,7 @@ obj-$(CONFIG_LYNXKDI) += lynxkdi.o
>  obj-$(CONFIG_MENU) += menu.o
>  obj-$(CONFIG_MODEM_SUPPORT) += modem.o
>  obj-$(CONFIG_UPDATE_TFTP) += update.o
> +obj-$(CONFIG_DFU_TFTP) += update.o

Nice. That's much cleaner.

>  obj-$(CONFIG_USB_KEYBOARD) += usb_kbd.o
>  obj-$(CONFIG_CMD_DFU) += cmd_dfu.o
>  obj-$(CONFIG_CMD_GPT) += cmd_gpt.o

[U-Boot] [PATCH v2 7/9] dfu: command: Extend "dfu" command to handle receiving data via TFTP

[Joe Hershberger]

Hi Lukasz,

On Sat, Jul 25, 2015 at 3:11 AM, Lukasz Majewski <l.majewski@majess.pl> wrote:
> The "dfu" command has been extended to support transfers via TFTP protocol.
>
> Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> ---
> Changes for v2:
> - Remove "dfutftp" command
> - Modify "dfu" command to support optional [tftp] parameter
> - Only one flag (CONFIG_DFU_TFTP) needs to be enabled
> ---
>  common/cmd_dfu.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>
> diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c
> index 857148f..aea466b 100644
> --- a/common/cmd_dfu.c
> +++ b/common/cmd_dfu.c
> @@ -1,6 +1,9 @@
>  /*
>   * cmd_dfu.c -- dfu command
>   *
> + * Copyright (C) 2015
> + * Lukasz Majewski <l.majewski@majess.pl>
> + *
>   * Copyright (C) 2012 Samsung Electronics
>   * authors: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
>   *         Lukasz Majewski <l.majewski@samsung.com>
> @@ -13,6 +16,9 @@
>  #include <dfu.h>
>  #include <g_dnl.h>
>  #include <usb.h>
> +#ifdef CONFIG_DFU_TFTP
> +#include <net.h>
> +#endif

Generally you shouldn't need to guard an include. Just include net.h
all the time.

>  static int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  {
> @@ -26,7 +32,18 @@ static int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>         char *devstring = argv[3];
>
>         int ret, i = 0;
> +#ifdef CONFIG_DFU_TFTP
> +       unsigned long addr = 0;

Maybe you should reinitialize devstring to NULL here?

> +       if (!strcmp(usb_controller, "tftp")) {

This would look a lot clearer if you used argv[1] instead of
usb_controller. You are not using it as the usb_controller parameter,
it just happens to be the second word.

> +               usb_controller = argv[2];

You shouldn't be keeping the usb_controller parameter around. It makes
no sense for the tftp case. Why not just drop it?

> +               interface = argv[3];
> +               devstring = argv[4];

Is it safe to read argv[4] on your optional parameter without checking
for argc >= 5?

> +               if (argc == 6)
> +                       addr = simple_strtoul(argv[5], NULL, 0);
>
> +               return update_tftp(addr, interface, devstring);
> +       }
> +#endif
>         ret = dfu_init_env_entities(interface, devstring);
>         if (ret)
>                 goto done;
> @@ -84,9 +101,17 @@ done:
>
>  U_BOOT_CMD(dfu, CONFIG_SYS_MAXARGS, 1, do_dfu,
>         "Device Firmware Upgrade",
> +#ifdef CONFIG_DFU_TFTP
> +       "[tftp] <USB_controller> <interface> <dev> [list] [addr]\n"

Also drop <USB_controller> from the help here.

> +#else
>         "<USB_controller> <interface> <dev> [list]\n"
> +#endif
>         "  - device firmware upgrade via <USB_controller>\n"
>         "    on device <dev>, attached to interface\n"
>         "    <interface>\n"
>         "    [list] - list available alt settings\n"
> +#ifdef CONFIG_DFU_TFTP
> +       "    [tftp] - use TFTP protocol to download data\n"
> +       "    [addr] - address where FIT image has been stored\n"

Probably not helpful to include the '[' and ']' here. They are
supposed to indicate optional parameters. We already know they are
optional from above.  Good to fix up the '[list]' as well.

> +#endif
>  );
> --
> 2.1.4
>

[U-Boot] [PATCH v2 8/9] config: bbb: Configs necessary for running update via TFTP on Beagle Bone Black

[Joe Hershberger]

Hi Lukasz,

On Sat, Jul 25, 2015 at 3:11 AM, Lukasz Majewski <l.majewski@majess.pl> wrote:
> Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> ---
> Changes for v2:
> - Do not enable CONFIG_UPDATE_TFTP since CONFIG_DFU_TFTP enables the common code
> - Do not enable CONFIG_CMD_DFUTFTP since dfutftp commands has been removed
> ---

I agree with Simon on his 2 points. Please address those.

[U-Boot] [PATCH v2 9/9] dfu:tests: Modify dfu_gadget_test.sh to accept USB device major:minor number

[Joe Hershberger]

Hi Lukasz,

On Sat, Jul 25, 2015 at 3:11 AM, Lukasz Majewski <l.majewski@majess.pl> wrote:
> In the dfu-util it is possible to set major:minor number by unsing -d flag
> (-d 0451:d022).
> Such option is very handy when many DFU devices are connected to a single
> host PC. This commit allows testing when above situation emerges.
>
> Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> ---
> Changes for v2:
> - New patch
> ---
>  test/dfu/README             |  9 ++++++++-
>  test/dfu/dfu_gadget_test.sh | 18 +++++++++++-------
>  2 files changed, 19 insertions(+), 8 deletions(-)

This seems unrelated to the series (aside from generally also being
DFU).  Probably just send it separately. What about a test for TFTP?

Thanks,
-Joe

[U-Boot] [PATCH v2 4/9] dfu: tftp: update: Provide tftp support for the DFU subsystem

[Lukasz Majewski]

Hi Simon,

> Hi Lukasz,
> 
> On 25 July 2015 at 02:11, Lukasz Majewski <l.majewski@majess.pl>
> wrote:
> > This commit adds initial support for using tftp for downloading and
> > upgrading firmware on the device.
> >
> > Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> > ---
> > Changes for v2:
> > - Return -ENOSYS instead of plain -1
> > - Remove interface and devstring env variables reading in the
> > dfu_tftp_write()
> > - Those parameters are now passed to dfu_tftp_write() function as
> > its arguments ---
> >  drivers/dfu/Makefile   |  1 +
> >  drivers/dfu/dfu_tftp.c | 65
> > ++++++++++++++++++++++++++++++++++++++++++++++++++
> > include/dfu.h          | 13 ++++++++++ 3 files changed, 79
> > insertions(+) create mode 100644 drivers/dfu/dfu_tftp.c
> 
> Is there a Kconfig option needed here?

I will add proper Kconfig option for this code in the following patch:

[PATCH v2 8/9] config: bbb: Configs necessary for running update via
TFTP on Beagle Bone Black

IMHO this patch is a better place to do that.

> 
> Regards,
> Simon

Best regards,
Lukasz Majewski
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150809/c8179b75/attachment.sig>

[U-Boot] [PATCH v2 5/9] dfu: tftp: update: Add dfu_write_from_mem_addr() function

[Lukasz Majewski]

Hi Simon,

> Hi Lukasz,
> 
> On 25 July 2015 at 02:11, Lukasz Majewski <l.majewski@majess.pl>
> wrote:
> > This function allows writing via DFU data stored from fixed buffer
> > address (like e.g. loadaddr env variable).
> >
> > Such predefined buffers are used in the update_tftp() code. In fact
> > this function is a wrapper on the dfu_write() and dfu_flush().
> >
> > Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> > ---
> > Changes for v2:
> > - Use min() macro instead of comparison
> > - Change definition of left variable to be unsigned long - this
> > allowed safe usage of min() macro
> > ---
> >  drivers/dfu/dfu.c | 48
> > ++++++++++++++++++++++++++++++++++++++++++++++++ include/dfu.h
> > |  1 + 2 files changed, 49 insertions(+)
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> But see nits below.
> 
> >
> > diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
> > index 2267dbf..c83ee41 100644
> > --- a/drivers/dfu/dfu.c
> > +++ b/drivers/dfu/dfu.c
> > @@ -568,3 +568,51 @@ int dfu_get_alt(char *name)
> >
> >         return -ENODEV;
> >  }
> > +
> > +/**
> > + * dfu_write_from_mem_addr - this function adds support for
> > writing data
> > + *                           starting from fixed memory address
> > (like $loadaddr)
> > + *                           to dfu managed medium (e.g. NAND, MMC)
> 
> I think it's better to start with a direct one-line description, like:
> 
> + * dfu_write_from_mem_addr - wite data to dfu-managed medium
> + *
> + * More detail goes here ...

Ok

> 
> > + *
> > + * @param dfu - dfu entity to which we want to store data
> > + * @param buf - fixed memory addres from where data starts
> > + * @param size - number of bytes to write
> > + *
> > + * @return - 0 on success, other value on failure
> > + */
> > +int dfu_write_from_mem_addr(struct dfu_entity *dfu, void *buf, int
> > size)
> 
> const void *buf? My understanding is that this buffer is not changed.
In principle I agree with you.

For practical reasons, I would opt for leaving the buf definition as
void *, since other functions (like globally visible dfu_write) uses
this buffer as void *.

Adding const there would require removing this qualifier in the line
[1] since then dp pointer is passed to dfu_write(), which requires void
* pointer.
> 
> > +{
> > +       unsigned long dfu_buf_size, write, left = size;
> > +       int i, ret = 0;
> > +       void *dp = buf;
	    ^^^^^^^^^^^^^^^^ [1]

> > +
> > +       /*
> > +        * Here we must call dfu_get_buf(dfu) first to be sure that
> > dfu_buf_size
> > +        * has been properly initialized - e.g. if "dfu_bufsiz" has
> > been taken
> > +        * into account.
> > +        */
> > +       dfu_get_buf(dfu);
> > +       dfu_buf_size = dfu_get_buf_size();
> > +       debug("%s: dfu buf size: %lu\n", __func__, dfu_buf_size);
> > +
> > +       for (i = 0; left > 0; i++) {
> > +               write = min(dfu_buf_size, left);
> > +
> > +               debug("%s: dp: 0x%p left: %lu write: %lu\n",
> > __func__,
> > +                     dp, left, write);
> > +               ret = dfu_write(dfu, dp, write, i);
> > +               if (ret) {
> > +                       error("DFU write failed\n");
> > +                       return ret;
> > +               }
> > +
> > +               dp += write;
> > +               left -= write;
> > +       }
> > +
> > +       ret = dfu_flush(dfu, NULL, 0, i);
> > +       if (ret)
> > +               error("DFU flush failed!");
> > +
> > +       return ret;
> > +}
> > diff --git a/include/dfu.h b/include/dfu.h
> > index 7f78cc2..e624c41 100644
> > --- a/include/dfu.h
> > +++ b/include/dfu.h
> > @@ -162,6 +162,7 @@ bool dfu_usb_get_reset(void);
> >  int dfu_read(struct dfu_entity *de, void *buf, int size, int
> > blk_seq_num); int dfu_write(struct dfu_entity *de, void *buf, int
> > size, int blk_seq_num); int dfu_flush(struct dfu_entity *de, void
> > *buf, int size, int blk_seq_num);
> 
> Please can you put your function comment here in the header?

ok

> 
> > +int dfu_write_from_mem_addr(struct dfu_entity *dfu, void *buf, int
> > size); /* Device specific */
> >  #ifdef CONFIG_DFU_MMC
> >  extern int dfu_fill_entity_mmc(struct dfu_entity *dfu, char
> > *devstr, char *s); --
> > 2.1.4
> >
> 
> Regards,
> Simon

Best regards,

Lukasz Majewski
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150809/a28d0aa0/attachment.sig>

[U-Boot] [PATCH v2 6/9] update: tftp: dfu: Extend update_tftp() function to support DFU

[Lukasz Majewski]

Hi Simon,

> Hi Lukasz,
> 
> On 25 July 2015 at 02:11, Lukasz Majewski <l.majewski@majess.pl>
> wrote:
> > This code allows using DFU defined mediums for storing data
> > received via TFTP protocol.
> >
> > It reuses and preserves functionality of legacy code at
> > common/update.c.
> >
> > The update_tftp() function now accepts parameters - namely medium
> > device name and its number (e.g. mmc 1).
> >
> > Without this information passed old behavior is preserved.
> >
> > Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
> > ---
> > Changes for v2:
> > - Remove env variables from update_tftp() function
> > - Add parameters to update_tftp() function - without them old
> > behavior is preserved
> > - Stop compilation when legacy flags (CONFIG_UPDATE_TFTP and
> > CONFIG_SYS_NO_FLASH) are wrongly defined
> > - In the u-boot code legacy calls to update_tftp(0UL) have been
> > changed to update_tftp(0UL, NULL, NULL)
> > ---
> >  common/Makefile     |  1 +
> >  common/cmd_fitupd.c |  2 +-
> >  common/main.c       |  2 +-
> >  common/update.c     | 40 ++++++++++++++++++++++++++++++----------
> >  include/net.h       |  2 +-
> >  5 files changed, 34 insertions(+), 13 deletions(-)
> >
> > diff --git a/common/Makefile b/common/Makefile
> > index d6c1d48..76626f1 100644
> > --- a/common/Makefile
> > +++ b/common/Makefile
> > @@ -208,6 +208,7 @@ obj-$(CONFIG_LYNXKDI) += lynxkdi.o
> >  obj-$(CONFIG_MENU) += menu.o
> >  obj-$(CONFIG_MODEM_SUPPORT) += modem.o
> >  obj-$(CONFIG_UPDATE_TFTP) += update.o
> > +obj-$(CONFIG_DFU_TFTP) += update.o
> >  obj-$(CONFIG_USB_KEYBOARD) += usb_kbd.o
> >  obj-$(CONFIG_CMD_DFU) += cmd_dfu.o
> >  obj-$(CONFIG_CMD_GPT) += cmd_gpt.o
> > diff --git a/common/cmd_fitupd.c b/common/cmd_fitupd.c
> > index b045974..78b8747 100644
> > --- a/common/cmd_fitupd.c
> > +++ b/common/cmd_fitupd.c
> > @@ -23,7 +23,7 @@ static int do_fitupd(cmd_tbl_t *cmdtp, int flag,
> > int argc, char * const argv[]) if (argc == 2)
> >                 addr = simple_strtoul(argv[1], NULL, 16);
> >
> > -       return update_tftp(addr);
> > +       return update_tftp(addr, NULL, NULL);
> >  }
> >
> >  U_BOOT_CMD(fitupd, 2, 0, do_fitupd,
> > diff --git a/common/main.c b/common/main.c
> > index 2979fbe..ead0cd1 100644
> > --- a/common/main.c
> > +++ b/common/main.c
> > @@ -75,7 +75,7 @@ void main_loop(void)
> >         run_preboot_environment_command();
> >
> >  #if defined(CONFIG_UPDATE_TFTP)
> > -       update_tftp(0UL);
> > +       update_tftp(0UL, NULL, NULL);
> >  #endif /* CONFIG_UPDATE_TFTP */
> >
> >         s = bootdelay_process();
> > diff --git a/common/update.c b/common/update.c
> > index 542915c..1d082b0 100644
> > --- a/common/update.c
> > +++ b/common/update.c
> > @@ -13,11 +13,16 @@
> >  #error "CONFIG_FIT and CONFIG_OF_LIBFDT are required for
> > auto-update feature" #endif
> >
> > +#if defined(CONFIG_UPDATE_TFTP) && defined(CONFIG_SYS_NO_FLASH)
> > +#error "CONFIG_UPDATE_TFTP and CONFIG_SYS_NO_FLASH needed for
> > legacy behaviour" +#endif
> > +
> >  #include <command.h>
> >  #include <flash.h>
> >  #include <net.h>
> >  #include <net/tftp.h>
> >  #include <malloc.h>
> > +#include <dfu.h>
> >
> >  /* env variable holding the location of the update file */
> >  #define UPDATE_FILE_ENV                "updatefile"
> > @@ -222,13 +227,17 @@ static int update_fit_getparams(const void
> > *fit, int noffset, ulong *addr, return 0;
> >  }
> >
> > -int update_tftp(ulong addr)
> > +int update_tftp(ulong addr, char *interface, char *devstring)
> 
> Should these be const char *?
> 
> >  {
> > -       char *filename, *env_addr;
> > -       int images_noffset, ndepth, noffset;
> > +       char *filename, *env_addr, *fit_image_name;
> 
> And these?

I'm quite puzzled here. In other places DFU code is operating on the
char * strings. Even in the dfu.h file all other functions use char *.

To do it right I would need to change char * to const char * in many
places at the DFU subsystem. Hence I think that such major change
deserves a separate patch series - not related to this one.

For the reasons presented above, I would opt for leaving the code as it
is and afterwards change char * to const char * globally for DFU
subsystem.

> 
> >         ulong update_addr, update_fladdr, update_size;
> > -       void *fit;
> > +       int images_noffset, ndepth, noffset;
> > +       bool update_tftp_dfu = false;
> >         int ret = 0;
> > +       void *fit;
> > +
> > +       if (interface && devstring)
> > +               update_tftp_dfu = true;
> >
> >         /* use already present image */
> >         if (addr)
> > @@ -277,8 +286,8 @@ got_update_file:
> >                 if (ndepth != 1)
> >                         goto next_node;
> >
> > -               printf("Processing update '%s' :",
> > -                       fit_get_name(fit, noffset, NULL));
> > +               fit_image_name = (char *)fit_get_name(fit, noffset,
> > NULL);
> 
> Can you drop that cast?
> 
> > +               printf("Processing update '%s' :", fit_image_name);
> >
> >                 if (!fit_image_verify(fit, noffset)) {
> >                         printf("Error: invalid update hash,
> > aborting\n"); @@ -294,10 +303,21 @@ got_update_file:
> >                         ret = 1;
> >                         goto next_node;
> >                 }
> > -               if (update_flash(update_addr, update_fladdr,
> > update_size)) {
> > -                       printf("Error: can't flash update,
> > aborting\n");
> > -                       ret = 1;
> > -                       goto next_node;
> > +
> > +               if (!update_tftp_dfu) {
> > +                       if (update_flash(update_addr, update_fladdr,
> > +                                        update_size)) {
> > +                               printf("Error: can't flash update,
> > aborting\n");
> > +                               ret = 1;
> > +                               goto next_node;
> > +                       }
> > +               } else if (fit_image_check_type(fit, noffset,
> > +                                               IH_TYPE_FIRMWARE)) {
> > +                       if (dfu_tftp_write(fit_image_name,
> > update_addr,
> > +                                          update_size, interface,
> > devstring)) {
> > +                               ret = 1;
> > +                               goto next_node;
> > +                       }
> >                 }
> >  next_node:
> >                 noffset = fdt_next_node(fit, noffset, &ndepth);
> > diff --git a/include/net.h b/include/net.h
> > index d17173d..9af3190 100644
> > --- a/include/net.h
> > +++ b/include/net.h
> > @@ -804,7 +804,7 @@ void copy_filename(char *dst, const char *src,
> > int size); unsigned int random_port(void);
> >
> >  /* Update U-Boot over TFTP */
> > -int update_tftp(ulong addr);
> > +int update_tftp(ulong addr, char *interface, char *devstring);
> 
> Function comment - what are the parameters?

No problem, I will fix it.

> 
> >
> >  /**********************************************************************/
> >
> > --
> > 2.1.4
> >
> 
> Regards,
> Simon

Best regards,

Lukasz Majewski
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150810/17037d20/attachment.sig>

[U-Boot] [PATCH v2 6/9] update: tftp: dfu: Extend update_tftp() function to support DFU

[Simon Glass]

Hi Lukasz,

On 10 August 2015 at 11:01, Lukasz Majewski <l.majewski@majess.pl> wrote:
> Hi Simon,
>
>> Hi Lukasz,
>>
>> On 25 July 2015 at 02:11, Lukasz Majewski <l.majewski@majess.pl>
>> wrote:
>> > This code allows using DFU defined mediums for storing data
>> > received via TFTP protocol.
>> >
>> > It reuses and preserves functionality of legacy code at
>> > common/update.c.
>> >
>> > The update_tftp() function now accepts parameters - namely medium
>> > device name and its number (e.g. mmc 1).
>> >
>> > Without this information passed old behavior is preserved.
>> >
>> > Signed-off-by: Lukasz Majewski <l.majewski@majess.pl>
>> > ---
>> > Changes for v2:
>> > - Remove env variables from update_tftp() function
>> > - Add parameters to update_tftp() function - without them old
>> > behavior is preserved
>> > - Stop compilation when legacy flags (CONFIG_UPDATE_TFTP and
>> > CONFIG_SYS_NO_FLASH) are wrongly defined
>> > - In the u-boot code legacy calls to update_tftp(0UL) have been
>> > changed to update_tftp(0UL, NULL, NULL)
>> > ---
>> >  common/Makefile     |  1 +
>> >  common/cmd_fitupd.c |  2 +-
>> >  common/main.c       |  2 +-
>> >  common/update.c     | 40 ++++++++++++++++++++++++++++++----------
>> >  include/net.h       |  2 +-
>> >  5 files changed, 34 insertions(+), 13 deletions(-)
>> >
>> > diff --git a/common/Makefile b/common/Makefile
>> > index d6c1d48..76626f1 100644
>> > --- a/common/Makefile
>> > +++ b/common/Makefile
>> > @@ -208,6 +208,7 @@ obj-$(CONFIG_LYNXKDI) += lynxkdi.o
>> >  obj-$(CONFIG_MENU) += menu.o
>> >  obj-$(CONFIG_MODEM_SUPPORT) += modem.o
>> >  obj-$(CONFIG_UPDATE_TFTP) += update.o
>> > +obj-$(CONFIG_DFU_TFTP) += update.o
>> >  obj-$(CONFIG_USB_KEYBOARD) += usb_kbd.o
>> >  obj-$(CONFIG_CMD_DFU) += cmd_dfu.o
>> >  obj-$(CONFIG_CMD_GPT) += cmd_gpt.o
>> > diff --git a/common/cmd_fitupd.c b/common/cmd_fitupd.c
>> > index b045974..78b8747 100644
>> > --- a/common/cmd_fitupd.c
>> > +++ b/common/cmd_fitupd.c
>> > @@ -23,7 +23,7 @@ static int do_fitupd(cmd_tbl_t *cmdtp, int flag,
>> > int argc, char * const argv[]) if (argc == 2)
>> >                 addr = simple_strtoul(argv[1], NULL, 16);
>> >
>> > -       return update_tftp(addr);
>> > +       return update_tftp(addr, NULL, NULL);
>> >  }
>> >
>> >  U_BOOT_CMD(fitupd, 2, 0, do_fitupd,
>> > diff --git a/common/main.c b/common/main.c
>> > index 2979fbe..ead0cd1 100644
>> > --- a/common/main.c
>> > +++ b/common/main.c
>> > @@ -75,7 +75,7 @@ void main_loop(void)
>> >         run_preboot_environment_command();
>> >
>> >  #if defined(CONFIG_UPDATE_TFTP)
>> > -       update_tftp(0UL);
>> > +       update_tftp(0UL, NULL, NULL);
>> >  #endif /* CONFIG_UPDATE_TFTP */
>> >
>> >         s = bootdelay_process();
>> > diff --git a/common/update.c b/common/update.c
>> > index 542915c..1d082b0 100644
>> > --- a/common/update.c
>> > +++ b/common/update.c
>> > @@ -13,11 +13,16 @@
>> >  #error "CONFIG_FIT and CONFIG_OF_LIBFDT are required for
>> > auto-update feature" #endif
>> >
>> > +#if defined(CONFIG_UPDATE_TFTP) && defined(CONFIG_SYS_NO_FLASH)
>> > +#error "CONFIG_UPDATE_TFTP and CONFIG_SYS_NO_FLASH needed for
>> > legacy behaviour" +#endif
>> > +
>> >  #include <command.h>
>> >  #include <flash.h>
>> >  #include <net.h>
>> >  #include <net/tftp.h>
>> >  #include <malloc.h>
>> > +#include <dfu.h>
>> >
>> >  /* env variable holding the location of the update file */
>> >  #define UPDATE_FILE_ENV                "updatefile"
>> > @@ -222,13 +227,17 @@ static int update_fit_getparams(const void
>> > *fit, int noffset, ulong *addr, return 0;
>> >  }
>> >
>> > -int update_tftp(ulong addr)
>> > +int update_tftp(ulong addr, char *interface, char *devstring)
>>
>> Should these be const char *?
>>
>> >  {
>> > -       char *filename, *env_addr;
>> > -       int images_noffset, ndepth, noffset;
>> > +       char *filename, *env_addr, *fit_image_name;
>>
>> And these?
>
> I'm quite puzzled here. In other places DFU code is operating on the
> char * strings. Even in the dfu.h file all other functions use char *.
>
> To do it right I would need to change char * to const char * in many
> places at the DFU subsystem. Hence I think that such major change
> deserves a separate patch series - not related to this one.
>
> For the reasons presented above, I would opt for leaving the code as it
> is and afterwards change char * to const char * globally for DFU
> subsystem.

Sounds good, thanks.

[snip]

Regards,
Simon