[U-Boot] [PATCH 1/2] dfu: Fix up the Kconfig mess

Lukasz Majewski lukma at denx.de
Thu Feb 15 00:24:41 UTC 2018


Hi Marek,

> Clean up the screaming mess of configuration options that DFU is.
> It was impossible to configure DFU such that TFTP is enabled and
> USB is not, this patch fixes that and assures that DFU TFTP and
> DFU USB can be enabled separatelly and that the correct pieces
> of code are compiled in.
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas at gmail.com>
> Cc: Lukasz Majewski <lukma at denx.de>
> ---
>  cmd/Kconfig          |  3 ++-
>  cmd/dfu.c            | 18 +++++++++++++-----
>  common/Makefile      |  6 ++++--
>  drivers/dfu/Kconfig  | 13 ++++++++++++-
>  drivers/dfu/Makefile |  2 +-
>  5 files changed, 32 insertions(+), 10 deletions(-)
> 
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 7368b6df52..1ef4c31202 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -582,7 +582,8 @@ config CMD_DEMO
>  
>  config CMD_DFU
>  	bool "dfu"
> -	select USB_FUNCTION_DFU
> +	imply USB_FUNCTION_DFU if USB_GADGET
	      ^^^^^^^^^^^- This name is OK. It is the same as other
	      "gadget" names - like USB_FUNCTION_SDP,
	      USB_FUNCTION_THOR, etc.

> +	imply TFTP_FUNCTION_DFU if NET
	      ^^^^^^^^^^^^^^ - this name is a bit misleading
	Why we cannot select CONFIG_DFU_TFTP here directly and drop
	this config?
 
>  	help
>  	  Enables the command "dfu" which is used to have U-Boot
> create a DFU class device via USB. This command requires that the
> "dfu_alt_info" diff --git a/cmd/dfu.c b/cmd/dfu.c
> index 04291f6c08..76b89ca5ed 100644
> --- a/cmd/dfu.c
> +++ b/cmd/dfu.c
> @@ -25,12 +25,14 @@ static int do_dfu(cmd_tbl_t *cmdtp, int flag, int
> argc, char * const argv[]) if (argc < 4)
>  		return CMD_RET_USAGE;
>  
> +#ifdef CONFIG_USB_FUNCTION_DFU
>  	char *usb_controller = argv[1];
> +#endif
>  	char *interface = argv[2];
>  	char *devstring = argv[3];
>  
> -	int ret;
> -#ifdef CONFIG_DFU_TFTP
> +	int ret = 0;
> +#ifdef CONFIG_TFTP_FUNCTION_DFU
>  	unsigned long addr = 0;
>  	if (!strcmp(argv[1], "tftp")) {
>  		if (argc == 5)
> @@ -39,7 +41,7 @@ static int do_dfu(cmd_tbl_t *cmdtp, int flag, int
> argc, char * const argv[]) return update_tftp(addr, interface,
> devstring); }
>  #endif
> -
> +#ifdef CONFIG_USB_FUNCTION_DFU
>  	ret = dfu_init_env_entities(interface, devstring);
>  	if (ret)
>  		goto done;
> @@ -56,18 +58,24 @@ static int do_dfu(cmd_tbl_t *cmdtp, int flag, int
> argc, char * const argv[]) 
>  done:
>  	dfu_free_entities();
> +#endif

Please exclude relevant pieces of code for TFTP and for USB in a
separate static functions:

#ifdef CONFIG_USB_FUNCTION_DFU
	dfu_usb() {

	}
#else
	dfu_usb() {return 0};
#fi

#ifdef CONFIG_DFU_TFTP
	dfu_tftp() {

	}
#else
	dfu_tftp() {return 0};
#fi


do_dfu () {
	...
	ret = dfu_usb();
	ret = dfu_tftp();
	....
}

>  	return ret;
>  }
>  
>  U_BOOT_CMD(dfu, CONFIG_SYS_MAXARGS, 1, do_dfu,
>  	"Device Firmware Upgrade",
> +#ifdef CONFIG_USB_FUNCTION_DFU
>  	"<USB_controller> <interface> <dev> [list]\n"
>  	"  - 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
> -	"dfu tftp <interface> <dev> [<addr>]\n"
> +#endif
> +#ifdef CONFIG_TFTP_FUNCTION_DFU
> +#ifdef CONFIG_USB_FUNCTION_DFU
> +	"dfu "
> +#endif
> +	"tftp <interface> <dev> [<addr>]\n"
>  	"  - device firmware upgrade via TFTP\n"
>  	"    on device <dev>, attached to interface\n"
>  	"    <interface>\n"

Please simply use (in a way similar to ./cmd/mmc.c) :

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


> diff --git a/common/Makefile b/common/Makefile
> index c7bde239c1..cdd0ab19d8 100644
> --- a/common/Makefile
> +++ b/common/Makefile
> @@ -66,7 +66,9 @@ endif # !CONFIG_SPL_BUILD
>  obj-$(CONFIG_$(SPL_TPL_)BOOTSTAGE) += bootstage.o
>  
>  ifdef CONFIG_SPL_BUILD
> -obj-$(CONFIG_SPL_DFU_SUPPORT) += dfu.o
> +ifdef CONFIG_SPL_DFU_SUPPORT
> +obj-$(CONFIG_USB_FUNCTION_DFU) += dfu.o
> +endif

This shall be left as it is - Faiz is going to clean up this.
http://patchwork.ozlabs.org/patch/873372/

And side question - the CONFIG_SPL_DFU_SUPPORT shall be orthogonal
to other DFU settings (as it is for SPL).

Does your board require DFU support in SPL ?

>  obj-$(CONFIG_SPL_DFU_SUPPORT) += cli_hush.o
>  obj-$(CONFIG_SPL_HASH_SUPPORT) += hash.o
>  obj-$(CONFIG_SPL_YMODEM_SUPPORT) += xyzModem.o
> @@ -128,7 +130,7 @@ endif
>  
>  obj-y += cli.o
>  obj-$(CONFIG_FSL_DDR_INTERACTIVE) += cli_simple.o cli_readline.o
> -obj-$(CONFIG_CMD_DFU) += dfu.o
> +obj-$(CONFIG_USB_FUNCTION_DFU) += dfu.o

Yes, this is correct - only needed for USB DFU.

>  obj-y += command.o
>  obj-$(CONFIG_$(SPL_)LOG) += log.o
>  obj-$(CONFIG_$(SPL_)LOG_CONSOLE) += log_console.o
> diff --git a/drivers/dfu/Kconfig b/drivers/dfu/Kconfig
> index fa27efbb40..1a578546c7 100644
> --- a/drivers/dfu/Kconfig
> +++ b/drivers/dfu/Kconfig
> @@ -1,12 +1,23 @@
>  menu "DFU support"
>  
> +config DFU
> +	bool
> +
>  config USB_FUNCTION_DFU
>  	bool
>  	select HASH
> +	select DFU
> +	depends on USB_GADGET
> +
> +config TFTP_FUNCTION_DFU
> +	bool
> +	select DFU
> +	depends on NET

This shall be dropped - we shall use CONFIG_DFU_TFTP directly

>  
> -if CMD_DFU
> +if DFU
>  config DFU_TFTP
>  	bool "DFU via TFTP"
> +	depends on TFTP_FUNCTION_DFU
		  
This is not needed.

>  	help
>  	  This option allows performing update of DFU-managed medium
> with data sent via TFTP boot.
> diff --git a/drivers/dfu/Makefile b/drivers/dfu/Makefile
> index 61f2b71f91..7f35871ddc 100644
> --- a/drivers/dfu/Makefile
> +++ b/drivers/dfu/Makefile
> @@ -5,7 +5,7 @@
>  # SPDX-License-Identifier:	GPL-2.0+
>  #
>  
> -obj-$(CONFIG_USB_FUNCTION_DFU) += dfu.o
> +obj-$(CONFIG_DFU) += dfu.o

+1

>  obj-$(CONFIG_DFU_MMC) += dfu_mmc.o
>  obj-$(CONFIG_DFU_NAND) += dfu_nand.o
>  obj-$(CONFIG_DFU_RAM) += dfu_ram.o

I do like the idea of first enabling CONFIG_CMD_DFU, which then if
applicable enables USB_FUNCTION_DFU or DFU_TFTP.

Also CONFIG_DFU shall add generic drivers/dfu/dfu.o.


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-10 Fax: (+49)-8142-66989-80 Email: wd 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/20180215/16ed74f2/attachment.sig>


More information about the U-Boot mailing list