diff mbox series

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

Message ID 20200710012537.6264-5-takahiro.akashi@linaro.org
State Superseded, archived
Delegated to: Heinrich Schuchardt
Headers show
Series efi_loader: add capsule update support | expand

Commit Message

AKASHI Takahiro July 10, 2020, 1:25 a.m. UTC
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@linaro.org>
---
 common/Kconfig  | 16 +++++++++++
 common/Makefile |  2 +-
 common/update.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++--
 include/image.h | 12 ++++++++
 4 files changed, 102 insertions(+), 3 deletions(-)

Comments

Heinrich Schuchardt July 10, 2020, 4:24 p.m. UTC | #1
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@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.

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

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

> +{
> +	char *fit_image_name;
> +	ulong update_addr, update_fladdr, update_size;
> +	int images_noffset, ndepth, noffset;
> +	int ret = 0;
> +
> +	if (!fit)
> +		return -EINVAL;
> +
> +	if (!fit_check_format((void *)fit)) {
> +		printf("Bad FIT format of the update file, aborting auto-update\n");
> +		return -EINVAL;
> +	}
> +
> +	/* process updates */
> +	images_noffset = fdt_path_offset(fit, FIT_IMAGES_PATH);
> +
> +	ndepth = 0;
> +	noffset = fdt_next_node(fit, images_noffset, &ndepth);
> +	while (noffset >= 0 && ndepth > 0) {
> +		if (ndepth != 1)
> +			goto next_node;
> +
> +		fit_image_name = (char *)fit_get_name(fit, noffset, NULL);
> +		printf("Processing update '%s' :", fit_image_name);
> +
> +		if (!fit_image_verify(fit, noffset)) {
> +			printf("Error: invalid update hash, aborting\n");
> +			ret = 1;
> +			goto next_node;
> +		}
> +
> +		printf("\n");
> +		if (update_fit_getparams(fit, noffset, &update_addr,
> +					 &update_fladdr, &update_size)) {
> +			printf("Error: can't get update parameters, aborting\n");
> +			ret = 1;
> +			goto next_node;
> +		}
> +
> +		if (fit_image_check_type(fit, noffset, IH_TYPE_FIRMWARE)) {
> +			ret = dfu_write_by_name(fit_image_name, update_addr,
> +						update_size, NULL, NULL);
> +			if (ret)
> +				return ret;
> +		}
> +next_node:
> +		noffset = fdt_next_node(fit, noffset, &ndepth);
> +	}
> +
> +	return ret;
> +}
> +#endif
> diff --git a/include/image.h b/include/image.h
> index ad81dad44429..1f2c53339d51 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -1573,4 +1573,16 @@ struct fit_loadable_tbl {
>  		.handler = _handler, \
>  	}
>
> +/**
> + * 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 indentified 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);
> +
>  #endif	/* __IMAGE_H__ */
>
AKASHI Takahiro July 15, 2020, 4:28 a.m. UTC | #2
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@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.

-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
> 
> > +{
> > +	char *fit_image_name;
> > +	ulong update_addr, update_fladdr, update_size;
> > +	int images_noffset, ndepth, noffset;
> > +	int ret = 0;
> > +
> > +	if (!fit)
> > +		return -EINVAL;
> > +
> > +	if (!fit_check_format((void *)fit)) {
> > +		printf("Bad FIT format of the update file, aborting auto-update\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* process updates */
> > +	images_noffset = fdt_path_offset(fit, FIT_IMAGES_PATH);
> > +
> > +	ndepth = 0;
> > +	noffset = fdt_next_node(fit, images_noffset, &ndepth);
> > +	while (noffset >= 0 && ndepth > 0) {
> > +		if (ndepth != 1)
> > +			goto next_node;
> > +
> > +		fit_image_name = (char *)fit_get_name(fit, noffset, NULL);
> > +		printf("Processing update '%s' :", fit_image_name);
> > +
> > +		if (!fit_image_verify(fit, noffset)) {
> > +			printf("Error: invalid update hash, aborting\n");
> > +			ret = 1;
> > +			goto next_node;
> > +		}
> > +
> > +		printf("\n");
> > +		if (update_fit_getparams(fit, noffset, &update_addr,
> > +					 &update_fladdr, &update_size)) {
> > +			printf("Error: can't get update parameters, aborting\n");
> > +			ret = 1;
> > +			goto next_node;
> > +		}
> > +
> > +		if (fit_image_check_type(fit, noffset, IH_TYPE_FIRMWARE)) {
> > +			ret = dfu_write_by_name(fit_image_name, update_addr,
> > +						update_size, NULL, NULL);
> > +			if (ret)
> > +				return ret;
> > +		}
> > +next_node:
> > +		noffset = fdt_next_node(fit, noffset, &ndepth);
> > +	}
> > +
> > +	return ret;
> > +}
> > +#endif
> > diff --git a/include/image.h b/include/image.h
> > index ad81dad44429..1f2c53339d51 100644
> > --- a/include/image.h
> > +++ b/include/image.h
> > @@ -1573,4 +1573,16 @@ struct fit_loadable_tbl {
> >  		.handler = _handler, \
> >  	}
> >
> > +/**
> > + * 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 indentified 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);
> > +
> >  #endif	/* __IMAGE_H__ */
> >
>
Heinrich Schuchardt July 15, 2020, 7:07 a.m. UTC | #3
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@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
AKASHI Takahiro July 16, 2020, 12:43 a.m. UTC | #4
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.akashi@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.

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
diff mbox series

Patch

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)
+{
+	char *fit_image_name;
+	ulong update_addr, update_fladdr, update_size;
+	int images_noffset, ndepth, noffset;
+	int ret = 0;
+
+	if (!fit)
+		return -EINVAL;
+
+	if (!fit_check_format((void *)fit)) {
+		printf("Bad FIT format of the update file, aborting auto-update\n");
+		return -EINVAL;
+	}
+
+	/* process updates */
+	images_noffset = fdt_path_offset(fit, FIT_IMAGES_PATH);
+
+	ndepth = 0;
+	noffset = fdt_next_node(fit, images_noffset, &ndepth);
+	while (noffset >= 0 && ndepth > 0) {
+		if (ndepth != 1)
+			goto next_node;
+
+		fit_image_name = (char *)fit_get_name(fit, noffset, NULL);
+		printf("Processing update '%s' :", fit_image_name);
+
+		if (!fit_image_verify(fit, noffset)) {
+			printf("Error: invalid update hash, aborting\n");
+			ret = 1;
+			goto next_node;
+		}
+
+		printf("\n");
+		if (update_fit_getparams(fit, noffset, &update_addr,
+					 &update_fladdr, &update_size)) {
+			printf("Error: can't get update parameters, aborting\n");
+			ret = 1;
+			goto next_node;
+		}
+
+		if (fit_image_check_type(fit, noffset, IH_TYPE_FIRMWARE)) {
+			ret = dfu_write_by_name(fit_image_name, update_addr,
+						update_size, NULL, NULL);
+			if (ret)
+				return ret;
+		}
+next_node:
+		noffset = fdt_next_node(fit, noffset, &ndepth);
+	}
+
+	return ret;
+}
+#endif
diff --git a/include/image.h b/include/image.h
index ad81dad44429..1f2c53339d51 100644
--- a/include/image.h
+++ b/include/image.h
@@ -1573,4 +1573,16 @@  struct fit_loadable_tbl {
 		.handler = _handler, \
 	}
 
+/**
+ * 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 indentified 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);
+
 #endif	/* __IMAGE_H__ */