Heinrich,
On Wed, Jul 15, 2020 at 09:07:26AM +0200, Heinrich Schuchardt wrote:
> On 15.07.20 06:28, AKASHI Takahiro wrote:
> > Heinrich,
> >
> > On Fri, Jul 10, 2020 at 06:24:45PM +0200, Heinrich Schuchardt wrote:
> >> On 10.07.20 03:25, AKASHI Takahiro wrote:
> >>> The main purpose of this patch is to separate a generic interface for
> >>> updating firmware using DFU drivers from "auto-update" via tftp.
> >>>
> >>> This function will also be used in implementing UEFI capsule update
> >>> in a later commit.
> >>>
> >>> Signed-off-by: AKASHI Takahiro <takahiro.aka...@linaro.org>
> >>> ---
> >>> common/Kconfig | 16 +++++++++++
> >>> common/Makefile | 2 +-
> >>> common/update.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++--
> >>> include/image.h | 12 ++++++++
> >>> 4 files changed, 102 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/common/Kconfig b/common/Kconfig
> >>> index 2d86dd7e63c6..a51d2a7b9d2a 100644
> >>> --- a/common/Kconfig
> >>> +++ b/common/Kconfig
> >>> @@ -985,9 +985,16 @@ endmenu
> >>>
> >>> menu "Update support"
> >>>
> >>> +config UPDATE_COMMON
> >>> + bool
> >>> + default n
> >>> + select DFU_ALT
> >>> +
> >>> config UPDATE_TFTP
> >>> bool "Auto-update using fitImage via TFTP"
> >>> depends on FIT
> >>> + depends on DFU
> >>> + select UPDATE_COMMON
> >>> help
> >>> This option allows performing update of NOR with data in fitImage
> >>> sent via TFTP boot.
> >>> @@ -1002,6 +1009,15 @@ config UPDATE_TFTP_MSEC_MAX
> >>> default 100
> >>> depends on UPDATE_TFTP
> >>>
> >>> +config UPDATE_FIT
> >>> + bool "Firmware update using fitImage"
> >>> + depends on FIT
> >>> + depends on DFU
> >>> + select UPDATE_COMMON
> >>> + help
> >>> + This option allows performing update of DFU-capable storage with
> >>> + data in fitImage.
> >>> +
> >>> config ANDROID_AB
> >>> bool "Android A/B updates"
> >>> default n
> >>> diff --git a/common/Makefile b/common/Makefile
> >>> index 2e7a090588d9..c238db8108e7 100644
> >>> --- a/common/Makefile
> >>> +++ b/common/Makefile
> >>> @@ -53,7 +53,7 @@ obj-$(CONFIG_LCD_ROTATION) += lcd_console_rotation.o
> >>> obj-$(CONFIG_LCD_DT_SIMPLEFB) += lcd_simplefb.o
> >>> obj-$(CONFIG_LYNXKDI) += lynxkdi.o
> >>> obj-$(CONFIG_MENU) += menu.o
> >>> -obj-$(CONFIG_UPDATE_TFTP) += update.o
> >>> +obj-$(CONFIG_UPDATE_COMMON) += update.o
> >>> obj-$(CONFIG_DFU_TFTP) += update.o
> >>> obj-$(CONFIG_USB_KEYBOARD) += usb_kbd.o
> >>> obj-$(CONFIG_CMDLINE) += cli_readline.o cli_simple.o
> >>> diff --git a/common/update.c b/common/update.c
> >>> index 3547dc68bf7c..a8151383fae7 100644
> >>> --- a/common/update.c
> >>> +++ b/common/update.c
> >>> @@ -24,6 +24,7 @@
> >>> #include <errno.h>
> >>> #include <mtd/cfi_flash.h>
> >>>
> >>> +#ifdef CONFIG_UPDATE_TFTP
> >>> /* env variable holding the location of the update file */
> >>> #define UPDATE_FILE_ENV "updatefile"
> >>>
> >>> @@ -211,6 +212,7 @@ static int update_flash(ulong addr_source, ulong
> >>> addr_first, ulong size)
> >>> return 1;
> >>> #endif
> >>> }
> >>> +#endif /* CONFIG_UPDATE_TFTP */
> >>>
> >>> static int update_fit_getparams(const void *fit, int noffset, ulong
> >>> *addr,
> >>> ulong *fladdr, ulong *size)
> >>> @@ -228,6 +230,7 @@ static int update_fit_getparams(const void *fit, int
> >>> noffset, ulong *addr,
> >>> return 0;
> >>> }
> >>>
> >>> +#ifdef CONFIG_UPDATE_TFTP
> >>> int update_tftp(ulong addr, char *interface, char *devstring)
> >>> {
> >>> char *filename, *env_addr, *fit_image_name;
> >>> @@ -322,8 +325,9 @@ got_update_file:
> >>> }
> >>> } else if (fit_image_check_type(fit, noffset,
> >>> IH_TYPE_FIRMWARE)) {
> >>> - ret = dfu_tftp_write(fit_image_name, update_addr,
> >>> - update_size, interface, devstring);
> >>> + ret = dfu_write_by_name(fit_image_name, update_addr,
> >>> + update_size, interface,
> >>> + devstring);
> >>> if (ret)
> >>> return ret;
> >>> }
> >>> @@ -333,3 +337,70 @@ next_node:
> >>>
> >>> return ret;
> >>> }
> >>> +#endif
> >>> +
> >>> +#ifdef CONFIG_UPDATE_FIT
> >>> +/**
> >>> + * fit_update - update storage with FIT image
> >>> + * @fit: Pointer to FIT image
> >>> + *
> >>> + * Update firmware on storage using FIT image as input.
> >>> + * The storage area to be update will be identified by the name
> >>> + * in FIT and matching it to "dfu_alt_info" variable.
> >>> + *
> >>> + * Return: 0 - on success, non-zero - otherwise
> >>> + */
> >>> +int fit_update(const void *fit)
> >>
> >> With patch
> >>
> >> cmd: drop fitupd command
> >> https://patchwork.ozlabs.org/project/uboot/patch/20200617090903.9268-1-xypron.g...@gmx.de/
> >>
> >> the fitupd command is removed.
> >>
> >> Lukasz has to put the patch into a pull request, yet
> >>
> >> Once that patch has been inserted only the automatic fit updates will
> >> use code in common/update.c. This code is not called by the 'dfu tftp'
> >> command.
> >
> > ?
> > In cmd/dfu.c,
> > ===8<===
> > #ifdef CONFIG_DFU_OVER_TFTP
> > if (!strcmp(argv[1], "tftp"))
> > return update_tftp(value, interface, devstring);
> > #endif
> > ===>8===
> >
> >> The function fit_update() that you create should be considered a part of
> >> the dfu framework and should be placed in drivers/dfu/dfu.c. Please,
> >> give it a name starting with dfu_.
> >
> > Please read my code carefully.
> > There are some helper functions common to update_tftp() and
> > fit_update(). There is a good reason why I put this code in this file.
>
> common/update.c should be completely dropped from U-Boot because it is
> not used by any board and the dfu command can be used instead.
As I said in my previous email, update_tftp() is used in cmd/dfu.c.
> The DFU API is described in include/dfu.h. This is sufficient to
> implement an update. I agree that it may make sense to provide functions
> at a higher abstraction level. These functions should be in drivers/dfu/.
Which part of my code do you think is 'functions at a higher abstraction
level'?
-Takahiro Akashi
> Best regards
>
> Heinrich
>
> >
> > -Takahiro Akashi
> >
> >> This way there is no need to change the Kconfig for common/update.c as
> >> update.c remains dedicated to automatic updates via tFTP.
> >>
> >> Best regards
> >>
> >> Heinrich
