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@gmail.com Cc: Lukasz Majewski lukma@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@denx.de