On Wed, 13 Nov 2019 19:43:42 +0200 Andy Shevchenko andriy.shevchenko@linux.intel.com wrote:

In the future we may utilize optional argument in 'dfu' command line. As a preparation for this, refactor do_dfu().

Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com

cmd/dfu.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/cmd/dfu.c b/cmd/dfu.c index 33491d0bc9..14a8ec879e 100644 --- a/cmd/dfu.c +++ b/cmd/dfu.c @@ -30,22 +30,25 @@ static int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) #if defined(CONFIG_DFU_OVER_USB) || defined(CONFIG_DFU_OVER_TFTP) char *interface = NULL; char *devstring = NULL; +#if defined(CONFIG_DFU_OVER_TFTP)

• unsigned long value = 0;

+#endif

if (argc >= 4) { interface = argv[2]; devstring = argv[3]; }

+#if defined(CONFIG_DFU_OVER_TFTP)

• if (argc == 5 || argc == 3)

• value = simple_strtoul(argv[argc - 1], NULL, 0);
  

+#endif #endif

int ret = 0; #ifdef CONFIG_DFU_OVER_TFTP

• unsigned long addr = 0;
• if (!strcmp(argv[1], "tftp")) {

• if (argc == 5 || argc == 3)
  

• 	addr = simple_strtoul(argv[argc - 1], NULL,
  

0); -

• return update_tftp(addr, interface, devstring);
  

• }

• if (!strcmp(argv[1], "tftp"))

• return update_tftp(value, interface, devstring);
  

#endif #ifdef CONFIG_DFU_OVER_USB ret = dfu_init_env_entities(interface, devstring);

Acked-by: Lukasz Majewski lukma@denx.de

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

Hi Andy,

Thank you for your work on enhancing DFU. The patch series is generally Ok.

Please find some minor comments/requests below.

When the `dfu` command is called from the U-Boot environment, it now accepts an optional parameter that specifies a timeout (in seconds). If a DFU connection is not made within that time the `dfu` command exits (as it would if Ctrl+C was pressed). If the timeout is left empty or being zero the `dfu` command behaves as it does now.

This is useful for allowing U-Boot to check to see if anything wants to upload new firmware before continuing to boot.

The patch is based on the commit https://github.com/01org/edison-u-boot/commit/5e966ccc3c65c18c9783741fa04e0c... which has been heavily reworked due to U-Boot changes in the past.

Signed-off-by: Sebastien Colleur sebastienx.colleur@intel.com Signed-off-by: Brad Campbell bradjc5@gmail.com Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com

---

cmd/dfu.c | 15 +++++++++++++-- common/dfu.c | 17 +++++++++++++++++ drivers/dfu/Kconfig | 6 ++++++ drivers/dfu/dfu.c | 15 +++++++++++++++ include/dfu.h | 5 +++++ 5 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/cmd/dfu.c b/cmd/dfu.c index 14a8ec879e..b30f8a5667 100644 --- a/cmd/dfu.c +++ b/cmd/dfu.c @@ -30,7 +30,7 @@ static int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) #if defined(CONFIG_DFU_OVER_USB) || defined(CONFIG_DFU_OVER_TFTP) char *interface = NULL; char *devstring = NULL; -#if defined(CONFIG_DFU_OVER_TFTP) +#if defined(CONFIG_DFU_TIMEOUT) || defined(CONFIG_DFU_OVER_TFTP) unsigned long value = 0; #endif

@@ -39,7 +39,7 @@ static int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) devstring = argv[3]; }

-#if defined(CONFIG_DFU_OVER_TFTP) +#if defined(CONFIG_DFU_TIMEOUT) || defined(CONFIG_DFU_OVER_TFTP) if (argc == 5 || argc == 3) value = simple_strtoul(argv[argc - 1], NULL, 0); #endif @@ -55,6 +55,10 @@ static int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) if (ret) goto done;

+#ifdef CONFIG_DFU_TIMEOUT

• dfu_set_timeout(value * 1000);

+#endif

• ret = CMD_RET_SUCCESS; if (strcmp(argv[argc - 1], "list") == 0) { dfu_show_entities();

@@ -75,10 +79,17 @@ U_BOOT_CMD(dfu, CONFIG_SYS_MAXARGS, 1, do_dfu, "Device Firmware Upgrade", "" #ifdef CONFIG_DFU_OVER_USB +#ifdef CONFIG_DFU_TIMEOUT

• "<USB_controller> [<interface> <dev>] [<timeout>] [list]\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" +#ifdef CONFIG_DFU_TIMEOUT

• " [<timeout>] - specify inactivity timeout in seconds\n"

+#endif " [list] - list available alt settings\n" #endif #ifdef CONFIG_DFU_OVER_TFTP diff --git a/common/dfu.c b/common/dfu.c index 44d1484d3d..da6289b218 100644 --- a/common/dfu.c +++ b/common/dfu.c @@ -35,6 +35,10 @@ int run_usb_dnl_gadget(int usbctrl_index, char *usb_dnl_gadget) return CMD_RET_FAILURE; }

+#ifdef CONFIG_DFU_TIMEOUT

• unsigned long start_time = get_timer(0);

+#endif

• while (1) { if (g_dnl_detach()) { /*

@@ -79,6 +83,19 @@ int run_usb_dnl_gadget(int usbctrl_index, char *usb_dnl_gadget) } }

+#ifdef CONFIG_DFU_TIMEOUT

• unsigned long wait_time = dfu_get_timeout();
  

• if (wait_time) {
  

• 	unsigned long current_time =
  

get_timer(start_time); +

• 	if (current_time > wait_time) {
  

• 		debug("Inactivity timeout, abort
  

DFU\n");

• 		goto exit;
  

• 	}
  

• }
  

+#endif

• WATCHDOG_RESET(); usb_gadget_handle_interrupts(usbctrl_index); }

diff --git a/drivers/dfu/Kconfig b/drivers/dfu/Kconfig index 9fe5bc0f58..e070130b5a 100644 --- a/drivers/dfu/Kconfig +++ b/drivers/dfu/Kconfig @@ -23,6 +23,12 @@ config DFU_TFTP

 Detailed description of this feature can be found at

./doc/README.dfutftp +config DFU_TIMEOUT

• bool "Timeout waiting for DFU"
• help

•  This option adds an optional timeout parameter for DFU
  

which, if set,

•  will cause DFU to only wait for that many seconds before
  

exiting. + config DFU_MMC bool "MMC back end for DFU" help diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c index 38aecd3a05..df50196dfd 100644 --- a/drivers/dfu/dfu.c +++ b/drivers/dfu/dfu.c @@ -21,6 +21,9 @@ static LIST_HEAD(dfu_list); static int dfu_alt_num; static int alt_num_cnt; static struct hash_algo *dfu_hash_algo; +#ifdef CONFIG_DFU_TIMEOUT +static unsigned long dfu_timeout = 0; +#endif

/*

• The purpose of the dfu_flush_callback() function is to

@@ -58,6 +61,18 @@ __weak bool dfu_usb_get_reset(void) #endif }

+#ifdef CONFIG_DFU_TIMEOUT +void dfu_set_timeout(unsigned long timeout) +{

• dfu_timeout = timeout;

+}

I do guess that dfu_set_timeout() is not yet used in this patch series?

+unsigned long dfu_get_timeout(void) +{

• return dfu_timeout;

+} +#endif

static int dfu_find_alt_num(const char *s) { int i = 0; diff --git a/include/dfu.h b/include/dfu.h index 2e3e91c8d2..fb5260d903 100644 --- a/include/dfu.h +++ b/include/dfu.h @@ -178,6 +178,11 @@ unsigned char *dfu_free_buf(void); unsigned long dfu_get_buf_size(void); bool dfu_usb_get_reset(void);

+#ifdef CONFIG_DFU_TIMEOUT +unsigned long dfu_get_timeout(void); +void dfu_set_timeout(unsigned long); +#endif

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 add some description and example of this new option / feature to ./doc/README.dfu file.

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

Hi Andy,

The stock U-Boot on Intel Edison has timeout parameter for DFU command. Enable it here to be compatible with the original U-Boot configuration.

Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com

configs/edison_defconfig | 1 + 1 file changed, 1 insertion(+)

diff --git a/configs/edison_defconfig b/configs/edison_defconfig index 1c74ee9709..227e2f750c 100644 --- a/configs/edison_defconfig +++ b/configs/edison_defconfig @@ -29,6 +29,7 @@ CONFIG_CMD_FS_GENERIC=y CONFIG_DEFAULT_DEVICE_TREE="edison" CONFIG_ENV_IS_IN_MMC=y CONFIG_CPU=y +CONFIG_DFU_TIMEOUT=y CONFIG_DFU_MMC=y CONFIG_DFU_RAM=y CONFIG_SUPPORT_EMMC_BOOT=y

This patch doesn't apply now.

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

On Wed, Nov 27, 2019 at 05:21:32PM +0200, Andy Shevchenko wrote:

On Wed, Nov 27, 2019 at 11:57:53AM +0100, Lukasz Majewski wrote:

...

Hi Andy,

...

The stock U-Boot on Intel Edison has timeout parameter for DFU command. Enable it here to be compatible with the original U-Boot configuration.

Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com

configs/edison_defconfig | 1 + 1 file changed, 1 insertion(+)

diff --git a/configs/edison_defconfig b/configs/edison_defconfig index 1c74ee9709..227e2f750c 100644 --- a/configs/edison_defconfig +++ b/configs/edison_defconfig @@ -29,6 +29,7 @@ CONFIG_CMD_FS_GENERIC=y CONFIG_DEFAULT_DEVICE_TREE="edison" CONFIG_ENV_IS_IN_MMC=y CONFIG_CPU=y +CONFIG_DFU_TIMEOUT=y CONFIG_DFU_MMC=y CONFIG_DFU_RAM=y CONFIG_SUPPORT_EMMC_BOOT=y

This patch doesn't apply now.

I base my patches on official releases / release candidates. It applies very well on top of 2020.01-rc3 as of today. Does DFU has a separate repository / branch to track?

I see few patches in mainstream touching areas nearby. Though, this one can be still applied with a reduced context.