[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