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

Thu Feb 15 11:02:03 UTC 2018

Hi Marek,

> On 02/15/2018 01:24 AM, Lukasz Majewski wrote:
> > Hi Marek,  
> 
> Hi,
> 
> >> 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?  
> 
> No, it just follows suit with the already messed up naming. But I
> have a subsequent patch doing
> 
>     dfu: Rename _FUNCTION_DFU to DFU_OVER_
> 
>     Do the following to make the symbol names less confusing.
> 
>     sed -i "s/\([TU][^_]\+\)_FUNCTION_DFU/DFU_OVER_\1/g" \
>             `git grep _FUNCTION_DFU | cut -d ":" -f 1 | sort -u`
> 
> This should make the purpose clear.

Yes. Good idea.

> 
> >>  	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();
> > 	....
> > }  
> 
> This is kinda hard to split, but let's see.

Ok.

> 
> >>  	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) :  
> 
> This does not work, you see, if you use U_BOOT_CMD it prefixes only
> the first entry with the command name. Not the subsequent entries.
> That's why this extra ifdef is needed.
> 
> The code below, triggered by help dfu, would print "dfu dfu tftp" if
> CONFIG_USB_FUNCTION_DFU was disabled.

Ach... the mmc has set of "common" commands, so it looks nice when
extended in that way.

> 
> > #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 ?  
> 
> No, but I also don't want to break other boards.
> 
> >>  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  
> 
> This is needed to not break boards which select CONFIG_DFU_TFTP
> 
> >> -if CMD_DFU
> >> +if DFU
> >>  config DFU_TFTP
> >>  	bool "DFU via TFTP"
> >> +	depends on TFTP_FUNCTION_DFU  
> > 		  
> > This is not needed.  
> 
> So is this. It's another crutch to prevent this mess from completely
> unraveling.

Ok.

> 
> >>  	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 
> 
> 

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/efc9f1a5/attachment.sig>