[U-Boot] [PATCH v1 3/4] dfu: Add optional timeout parameter

[Lukasz Majewski <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/5e966ccc3c65c18c9783741fa04e0c45e021780c
> 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 at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20191127/d508438b/attachment.sig>