[PATCH v3 04/17] common: update: add a generic interface for FIT image

[Heinrich Schuchardt]

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.akashi at 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.glpk@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.

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

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