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

Message ID 20180213214839.9977-1-marek.vasut+renesas@gmail.com
State New
Delegated to: Lukasz Majewski
Headers show
Series
  • [U-Boot,1/2] dfu: Fix up the Kconfig mess
Related show

Commit Message

Marek Vasut Feb. 13, 2018, 9:48 p.m.
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(-)

Comments

Lukasz Majewski Feb. 14, 2018, 7:21 a.m. | #1
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.

Unfortunately, this change causes build breaks:
https://travis-ci.org/lmajewski/u-boot-dfu/jobs/341189959

> 
> 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
> +	imply TFTP_FUNCTION_DFU if NET
>  	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
>  	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"
> 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
>  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
>  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
>  
> -if CMD_DFU
> +if DFU
>  config DFU_TFTP
>  	bool "DFU via TFTP"
> +	depends on TFTP_FUNCTION_DFU
>  	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
>  obj-$(CONFIG_DFU_MMC) += dfu_mmc.o
>  obj-$(CONFIG_DFU_NAND) += dfu_nand.o
>  obj-$(CONFIG_DFU_RAM) += dfu_ram.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
Lukasz Majewski Feb. 15, 2018, 12:24 a.m. | #2
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
Marek Vasut Feb. 15, 2018, 10:21 a.m. | #3
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@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?

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.

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

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

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

>>  	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
>
Lukasz Majewski Feb. 15, 2018, 11:02 a.m. | #4
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@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?  
> 
> 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@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@denx.de

Patch

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
+	imply TFTP_FUNCTION_DFU if NET
 	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
 	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"
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
 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
 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
 
-if CMD_DFU
+if DFU
 config DFU_TFTP
 	bool "DFU via TFTP"
+	depends on TFTP_FUNCTION_DFU
 	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
 obj-$(CONFIG_DFU_MMC) += dfu_mmc.o
 obj-$(CONFIG_DFU_NAND) += dfu_nand.o
 obj-$(CONFIG_DFU_RAM) += dfu_ram.o