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

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

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@1 { + description = "U-Boot binary"; + +should look like + + images { + u-boot.bin@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)

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

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

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

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

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

[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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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.

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

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

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
participants (4)
-
Joe Hershberger
-
Lukasz Majewski
-
Simon Glass
-
Wolfgang Denk