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+rene...@gmail.com> > >> Cc: Lukasz Majewski <lu...@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:
> > w...@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: w...@denx.de
pgphyTs9U3dc4.pgp
Description: OpenPGP digital signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
