| Message ID | 20200907053426.1675646-2-takahiro.akashi@linaro.org |
|---|---|
| State | Accepted |
| Commit | 045fd8b13dc7b08a309043c28fc764c8fd2fde14 |
| Headers | show |
| Series | efi_loader: add capsule update support | expand |
On 07.09.20 07:34, AKASHI Takahiro wrote: > This function is essentially independent from tftp, and will also be > utilised in implementing UEFI capsule update in a later commit. > So just give it a more generic name. > In addition, a new configuration option, CONFIG_DFU_ALT, was introduced > so that the file will be compiled with different options, particularly > one added in a later commit. > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > common/update.c | 5 +++-- > drivers/dfu/Kconfig | 5 +++++ > drivers/dfu/Makefile | 2 +- > drivers/dfu/{dfu_tftp.c => dfu_alt.c} | 17 ++++++++++++-- > include/dfu.h | 32 +++++++++++++-------------- > 5 files changed, 40 insertions(+), 21 deletions(-) > rename drivers/dfu/{dfu_tftp.c => dfu_alt.c} (67%) > > diff --git a/common/update.c b/common/update.c > index 36b6b7523d50..39946776d74f 100644 > --- a/common/update.c > +++ b/common/update.c > @@ -324,8 +324,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; > } > diff --git a/drivers/dfu/Kconfig b/drivers/dfu/Kconfig > index 0eec00ba734d..78f901ff348a 100644 > --- a/drivers/dfu/Kconfig > +++ b/drivers/dfu/Kconfig > @@ -14,8 +14,13 @@ config DFU_OVER_TFTP > depends on NET > > if DFU > +config DFU_ALT > + bool You cannot define CONFIG_DFU_ALT as bool. This symbol already exists as string in include/configs/odroid.h, include/configs/s5p_goni.h, et.al. It is used to initialize environment variable dfu_alt_info. Your patch leads to an error on Travis: arm: + odroid +In file included from <command-line>: +include/linux/kconfig.h:21:49: error: pasting "__ARG_PLACEHOLDER_" and ""uImage fat 0 1;"" does not give a valid preprocessing token + 21 | #define _config_enabled(value) __config_enabled(__ARG_PLACEHOLDER_##value) Best regards Heinrich > + default n > + > config DFU_TFTP > bool "DFU via TFTP" > + select DFU_ALT > select DFU_OVER_TFTP > help > This option allows performing update of DFU-managed medium with data > diff --git a/drivers/dfu/Makefile b/drivers/dfu/Makefile > index 0d7925c083ef..cc7de1d3ed9b 100644 > --- a/drivers/dfu/Makefile > +++ b/drivers/dfu/Makefile > @@ -9,5 +9,5 @@ obj-$(CONFIG_$(SPL_)DFU_MTD) += dfu_mtd.o > obj-$(CONFIG_$(SPL_)DFU_NAND) += dfu_nand.o > obj-$(CONFIG_$(SPL_)DFU_RAM) += dfu_ram.o > obj-$(CONFIG_$(SPL_)DFU_SF) += dfu_sf.o > -obj-$(CONFIG_$(SPL_)DFU_TFTP) += dfu_tftp.o > +obj-$(CONFIG_$(SPL_)DFU_ALT) += dfu_alt.o > obj-$(CONFIG_$(SPL_)DFU_VIRT) += dfu_virt.o > diff --git a/drivers/dfu/dfu_tftp.c b/drivers/dfu/dfu_alt.c > similarity index 67% > rename from drivers/dfu/dfu_tftp.c > rename to drivers/dfu/dfu_alt.c > index ffae4bb54f80..5b1b13d7170d 100644 > --- a/drivers/dfu/dfu_tftp.c > +++ b/drivers/dfu/dfu_alt.c > @@ -10,8 +10,21 @@ > #include <errno.h> > #include <dfu.h> > > -int dfu_tftp_write(char *dfu_entity_name, unsigned int addr, unsigned int len, > - char *interface, char *devstring) > +/** > + * dfu_write_by_name() - write data to DFU medium > + * @dfu_entity_name: Name of DFU entity to write > + * @addr: Address of data buffer to write > + * @len: Number of bytes > + * @interface: Destination DFU medium (e.g. "mmc") > + * @devstring: Instance number of destination DFU medium (e.g. "1") > + * > + * This function is storing data received on DFU supported medium which > + * is specified by @dfu_entity_name. > + * > + * Return: 0 - on success, error code - otherwise > + */ > +int dfu_write_by_name(char *dfu_entity_name, unsigned int addr, > + unsigned int len, char *interface, char *devstring) > { > char *s, *sb; > int alt_setting_num, ret; > diff --git a/include/dfu.h b/include/dfu.h > index 84abdc79acd1..cecfbd76597b 100644 > --- a/include/dfu.h > +++ b/include/dfu.h > @@ -494,27 +494,27 @@ static inline int dfu_fill_entity_virt(struct dfu_entity *dfu, char *devstr, > #endif > > /** > - * dfu_tftp_write() - write TFTP data to DFU medium > + * dfu_write_by_name() - write data to DFU medium > + * @dfu_entity_name: Name of DFU entity to write > + * @addr: Address of data buffer to write > + * @len: Number of bytes > + * @interface: Destination DFU medium (e.g. "mmc") > + * @devstring: Instance number of destination DFU medium (e.g. "1") > * > - * This function is storing data received via TFTP on DFU supported medium. > + * This function is storing data received on DFU supported medium which > + * is specified by @dfu_entity_name. > * > - * @dfu_entity_name: name of DFU entity to write > - * @addr: address of data buffer to write > - * @len: number of bytes > - * @interface: destination DFU medium (e.g. "mmc") > - * @devstring: instance number of destination DFU medium (e.g. "1") > - * > - * Return: 0 on success, otherwise error code > + * Return: 0 - on success, error code - otherwise > */ > -#if CONFIG_IS_ENABLED(DFU_TFTP) > -int dfu_tftp_write(char *dfu_entity_name, unsigned int addr, unsigned int len, > - char *interface, char *devstring); > +#if CONFIG_IS_ENABLED(DFU_ALT) > +int dfu_write_by_name(char *dfu_entity_name, unsigned int addr, > + unsigned int len, char *interface, char *devstring); > #else > -static inline int dfu_tftp_write(char *dfu_entity_name, unsigned int addr, > - unsigned int len, char *interface, > - char *devstring) > +static inline int dfu_write_by_name(char *dfu_entity_name, unsigned int addr, > + unsigned int len, char *interface, > + char *devstring) > { > - puts("TFTP write support for DFU not available!\n"); > + puts("write support for DFU not available!\n"); > return -ENOSYS; > } > #endif >
On Wed, Sep 09, 2020 at 02:30:53PM +0200, Heinrich Schuchardt wrote: > On 07.09.20 07:34, AKASHI Takahiro wrote: > > This function is essentially independent from tftp, and will also be > > utilised in implementing UEFI capsule update in a later commit. > > So just give it a more generic name. > > In addition, a new configuration option, CONFIG_DFU_ALT, was introduced > > so that the file will be compiled with different options, particularly > > one added in a later commit. > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > --- > > common/update.c | 5 +++-- > > drivers/dfu/Kconfig | 5 +++++ > > drivers/dfu/Makefile | 2 +- > > drivers/dfu/{dfu_tftp.c => dfu_alt.c} | 17 ++++++++++++-- > > include/dfu.h | 32 +++++++++++++-------------- > > 5 files changed, 40 insertions(+), 21 deletions(-) > > rename drivers/dfu/{dfu_tftp.c => dfu_alt.c} (67%) > > > > diff --git a/common/update.c b/common/update.c > > index 36b6b7523d50..39946776d74f 100644 > > --- a/common/update.c > > +++ b/common/update.c > > @@ -324,8 +324,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; > > } > > diff --git a/drivers/dfu/Kconfig b/drivers/dfu/Kconfig > > index 0eec00ba734d..78f901ff348a 100644 > > --- a/drivers/dfu/Kconfig > > +++ b/drivers/dfu/Kconfig > > @@ -14,8 +14,13 @@ config DFU_OVER_TFTP > > depends on NET > > > > if DFU > > +config DFU_ALT > > + bool > > You cannot define CONFIG_DFU_ALT as bool. This symbol already exists as > string in include/configs/odroid.h, include/configs/s5p_goni.h, et.al. > It is used to initialize environment variable dfu_alt_info. Your patch > leads to an error on Travis: > > arm: + odroid Okay, will fix it. DFU_WRITE_ALT? Or, as Heinrich suggested, move them to dfu.c if Lukasz agrees. # I haven't seen any comments from him yet though. -Takahiro Akashi > +In file included from <command-line>: > > +include/linux/kconfig.h:21:49: error: pasting "__ARG_PLACEHOLDER_" and > ""uImage fat 0 1;"" does not give a valid preprocessing token > > + 21 | #define _config_enabled(value) > __config_enabled(__ARG_PLACEHOLDER_##value) > > Best regards > > Heinrich > > > + default n > > + > > config DFU_TFTP > > bool "DFU via TFTP" > > + select DFU_ALT > > select DFU_OVER_TFTP > > help > > This option allows performing update of DFU-managed medium with data > > diff --git a/drivers/dfu/Makefile b/drivers/dfu/Makefile > > index 0d7925c083ef..cc7de1d3ed9b 100644 > > --- a/drivers/dfu/Makefile > > +++ b/drivers/dfu/Makefile > > @@ -9,5 +9,5 @@ obj-$(CONFIG_$(SPL_)DFU_MTD) += dfu_mtd.o > > obj-$(CONFIG_$(SPL_)DFU_NAND) += dfu_nand.o > > obj-$(CONFIG_$(SPL_)DFU_RAM) += dfu_ram.o > > obj-$(CONFIG_$(SPL_)DFU_SF) += dfu_sf.o > > -obj-$(CONFIG_$(SPL_)DFU_TFTP) += dfu_tftp.o > > +obj-$(CONFIG_$(SPL_)DFU_ALT) += dfu_alt.o > > obj-$(CONFIG_$(SPL_)DFU_VIRT) += dfu_virt.o > > diff --git a/drivers/dfu/dfu_tftp.c b/drivers/dfu/dfu_alt.c > > similarity index 67% > > rename from drivers/dfu/dfu_tftp.c > > rename to drivers/dfu/dfu_alt.c > > index ffae4bb54f80..5b1b13d7170d 100644 > > --- a/drivers/dfu/dfu_tftp.c > > +++ b/drivers/dfu/dfu_alt.c > > @@ -10,8 +10,21 @@ > > #include <errno.h> > > #include <dfu.h> > > > > -int dfu_tftp_write(char *dfu_entity_name, unsigned int addr, unsigned int len, > > - char *interface, char *devstring) > > +/** > > + * dfu_write_by_name() - write data to DFU medium > > + * @dfu_entity_name: Name of DFU entity to write > > + * @addr: Address of data buffer to write > > + * @len: Number of bytes > > + * @interface: Destination DFU medium (e.g. "mmc") > > + * @devstring: Instance number of destination DFU medium (e.g. "1") > > + * > > + * This function is storing data received on DFU supported medium which > > + * is specified by @dfu_entity_name. > > + * > > + * Return: 0 - on success, error code - otherwise > > + */ > > +int dfu_write_by_name(char *dfu_entity_name, unsigned int addr, > > + unsigned int len, char *interface, char *devstring) > > { > > char *s, *sb; > > int alt_setting_num, ret; > > diff --git a/include/dfu.h b/include/dfu.h > > index 84abdc79acd1..cecfbd76597b 100644 > > --- a/include/dfu.h > > +++ b/include/dfu.h > > @@ -494,27 +494,27 @@ static inline int dfu_fill_entity_virt(struct dfu_entity *dfu, char *devstr, > > #endif > > > > /** > > - * dfu_tftp_write() - write TFTP data to DFU medium > > + * dfu_write_by_name() - write data to DFU medium > > + * @dfu_entity_name: Name of DFU entity to write > > + * @addr: Address of data buffer to write > > + * @len: Number of bytes > > + * @interface: Destination DFU medium (e.g. "mmc") > > + * @devstring: Instance number of destination DFU medium (e.g. "1") > > * > > - * This function is storing data received via TFTP on DFU supported medium. > > + * This function is storing data received on DFU supported medium which > > + * is specified by @dfu_entity_name. > > * > > - * @dfu_entity_name: name of DFU entity to write > > - * @addr: address of data buffer to write > > - * @len: number of bytes > > - * @interface: destination DFU medium (e.g. "mmc") > > - * @devstring: instance number of destination DFU medium (e.g. "1") > > - * > > - * Return: 0 on success, otherwise error code > > + * Return: 0 - on success, error code - otherwise > > */ > > -#if CONFIG_IS_ENABLED(DFU_TFTP) > > -int dfu_tftp_write(char *dfu_entity_name, unsigned int addr, unsigned int len, > > - char *interface, char *devstring); > > +#if CONFIG_IS_ENABLED(DFU_ALT) > > +int dfu_write_by_name(char *dfu_entity_name, unsigned int addr, > > + unsigned int len, char *interface, char *devstring); > > #else > > -static inline int dfu_tftp_write(char *dfu_entity_name, unsigned int addr, > > - unsigned int len, char *interface, > > - char *devstring) > > +static inline int dfu_write_by_name(char *dfu_entity_name, unsigned int addr, > > + unsigned int len, char *interface, > > + char *devstring) > > { > > - puts("TFTP write support for DFU not available!\n"); > > + puts("write support for DFU not available!\n"); > > return -ENOSYS; > > } > > #endif > > >
On Thu, Sep 10, 2020 at 10:54:18AM +0900, AKASHI Takahiro wrote: > On Wed, Sep 09, 2020 at 02:30:53PM +0200, Heinrich Schuchardt wrote: > > On 07.09.20 07:34, AKASHI Takahiro wrote: > > > This function is essentially independent from tftp, and will also be > > > utilised in implementing UEFI capsule update in a later commit. > > > So just give it a more generic name. > > > In addition, a new configuration option, CONFIG_DFU_ALT, was introduced > > > so that the file will be compiled with different options, particularly > > > one added in a later commit. > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > --- > > > common/update.c | 5 +++-- > > > drivers/dfu/Kconfig | 5 +++++ > > > drivers/dfu/Makefile | 2 +- > > > drivers/dfu/{dfu_tftp.c => dfu_alt.c} | 17 ++++++++++++-- > > > include/dfu.h | 32 +++++++++++++-------------- > > > 5 files changed, 40 insertions(+), 21 deletions(-) > > > rename drivers/dfu/{dfu_tftp.c => dfu_alt.c} (67%) > > > > > > diff --git a/common/update.c b/common/update.c > > > index 36b6b7523d50..39946776d74f 100644 > > > --- a/common/update.c > > > +++ b/common/update.c > > > @@ -324,8 +324,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; > > > } > > > diff --git a/drivers/dfu/Kconfig b/drivers/dfu/Kconfig > > > index 0eec00ba734d..78f901ff348a 100644 > > > --- a/drivers/dfu/Kconfig > > > +++ b/drivers/dfu/Kconfig > > > @@ -14,8 +14,13 @@ config DFU_OVER_TFTP > > > depends on NET > > > > > > if DFU > > > +config DFU_ALT > > > + bool > > > > You cannot define CONFIG_DFU_ALT as bool. This symbol already exists as > > string in include/configs/odroid.h, include/configs/s5p_goni.h, et.al. > > It is used to initialize environment variable dfu_alt_info. Your patch > > leads to an error on Travis: > > > > arm: + odroid > > Okay, will fix it. > DFU_WRITE_ALT? Or, as Heinrich suggested, move them > to dfu.c if Lukasz agrees. > > # I haven't seen any comments from him yet though. I think what you're introducing as "CONFIG_DFU_ALT" and renaming "drivers/dfu/dfu_tftp.c" to "drivers/dfu/dfu_alt.c" is what's confusing. That I think needs a different name than "alt" to convey that it's a generic "from DRAM to flash/etc" hook. -- Tom
On Wed, Sep 09, 2020 at 10:26:34PM -0400, Tom Rini wrote: > On Thu, Sep 10, 2020 at 10:54:18AM +0900, AKASHI Takahiro wrote: > > On Wed, Sep 09, 2020 at 02:30:53PM +0200, Heinrich Schuchardt wrote: > > > On 07.09.20 07:34, AKASHI Takahiro wrote: > > > > This function is essentially independent from tftp, and will also be > > > > utilised in implementing UEFI capsule update in a later commit. > > > > So just give it a more generic name. > > > > In addition, a new configuration option, CONFIG_DFU_ALT, was introduced > > > > so that the file will be compiled with different options, particularly > > > > one added in a later commit. > > > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > --- > > > > common/update.c | 5 +++-- > > > > drivers/dfu/Kconfig | 5 +++++ > > > > drivers/dfu/Makefile | 2 +- > > > > drivers/dfu/{dfu_tftp.c => dfu_alt.c} | 17 ++++++++++++-- > > > > include/dfu.h | 32 +++++++++++++-------------- > > > > 5 files changed, 40 insertions(+), 21 deletions(-) > > > > rename drivers/dfu/{dfu_tftp.c => dfu_alt.c} (67%) > > > > > > > > diff --git a/common/update.c b/common/update.c > > > > index 36b6b7523d50..39946776d74f 100644 > > > > --- a/common/update.c > > > > +++ b/common/update.c > > > > @@ -324,8 +324,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; > > > > } > > > > diff --git a/drivers/dfu/Kconfig b/drivers/dfu/Kconfig > > > > index 0eec00ba734d..78f901ff348a 100644 > > > > --- a/drivers/dfu/Kconfig > > > > +++ b/drivers/dfu/Kconfig > > > > @@ -14,8 +14,13 @@ config DFU_OVER_TFTP > > > > depends on NET > > > > > > > > if DFU > > > > +config DFU_ALT > > > > + bool > > > > > > You cannot define CONFIG_DFU_ALT as bool. This symbol already exists as > > > string in include/configs/odroid.h, include/configs/s5p_goni.h, et.al. > > > It is used to initialize environment variable dfu_alt_info. Your patch > > > leads to an error on Travis: > > > > > > arm: + odroid > > > > Okay, will fix it. > > DFU_WRITE_ALT? Or, as Heinrich suggested, move them > > to dfu.c if Lukasz agrees. > > > > # I haven't seen any comments from him yet though. > > I think what you're introducing as "CONFIG_DFU_ALT" and renaming > "drivers/dfu/dfu_tftp.c" to "drivers/dfu/dfu_alt.c" is what's confusing. > That I think needs a different name than "alt" to convey that it's a > generic "from DRAM to flash/etc" hook. Whatever the name is, I need reviews on all of my DFU patches from Lukasz in the first place. -Takahiro Akashi > -- > Tom
On Fri, Sep 11, 2020 at 09:46:14AM +0900, AKASHI Takahiro wrote: > On Wed, Sep 09, 2020 at 10:26:34PM -0400, Tom Rini wrote: > > On Thu, Sep 10, 2020 at 10:54:18AM +0900, AKASHI Takahiro wrote: > > > On Wed, Sep 09, 2020 at 02:30:53PM +0200, Heinrich Schuchardt wrote: > > > > On 07.09.20 07:34, AKASHI Takahiro wrote: > > > > > This function is essentially independent from tftp, and will also be > > > > > utilised in implementing UEFI capsule update in a later commit. > > > > > So just give it a more generic name. > > > > > In addition, a new configuration option, CONFIG_DFU_ALT, was introduced > > > > > so that the file will be compiled with different options, particularly > > > > > one added in a later commit. > > > > > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > > --- > > > > > common/update.c | 5 +++-- > > > > > drivers/dfu/Kconfig | 5 +++++ > > > > > drivers/dfu/Makefile | 2 +- > > > > > drivers/dfu/{dfu_tftp.c => dfu_alt.c} | 17 ++++++++++++-- > > > > > include/dfu.h | 32 +++++++++++++-------------- > > > > > 5 files changed, 40 insertions(+), 21 deletions(-) > > > > > rename drivers/dfu/{dfu_tftp.c => dfu_alt.c} (67%) > > > > > > > > > > diff --git a/common/update.c b/common/update.c > > > > > index 36b6b7523d50..39946776d74f 100644 > > > > > --- a/common/update.c > > > > > +++ b/common/update.c > > > > > @@ -324,8 +324,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; > > > > > } > > > > > diff --git a/drivers/dfu/Kconfig b/drivers/dfu/Kconfig > > > > > index 0eec00ba734d..78f901ff348a 100644 > > > > > --- a/drivers/dfu/Kconfig > > > > > +++ b/drivers/dfu/Kconfig > > > > > @@ -14,8 +14,13 @@ config DFU_OVER_TFTP > > > > > depends on NET > > > > > > > > > > if DFU > > > > > +config DFU_ALT > > > > > + bool > > > > > > > > You cannot define CONFIG_DFU_ALT as bool. This symbol already exists as > > > > string in include/configs/odroid.h, include/configs/s5p_goni.h, et.al. > > > > It is used to initialize environment variable dfu_alt_info. Your patch > > > > leads to an error on Travis: > > > > > > > > arm: + odroid > > > > > > Okay, will fix it. > > > DFU_WRITE_ALT? Or, as Heinrich suggested, move them > > > to dfu.c if Lukasz agrees. > > > > > > # I haven't seen any comments from him yet though. > > > > I think what you're introducing as "CONFIG_DFU_ALT" and renaming > > "drivers/dfu/dfu_tftp.c" to "drivers/dfu/dfu_alt.c" is what's confusing. > > That I think needs a different name than "alt" to convey that it's a > > generic "from DRAM to flash/etc" hook. > > Whatever the name is, I need reviews on all of my DFU patches from Lukasz > in the first place. The lower impact on the rest of the DFU subsytem and other automated testing that can be done, the easier it can be for someone else to review the DFU parts if Lukasz isn't able to find time to do it. Thanks. -- Tom
Tom, Lukasz, On Thu, Sep 10, 2020 at 10:41:39PM -0400, Tom Rini wrote: > On Fri, Sep 11, 2020 at 09:46:14AM +0900, AKASHI Takahiro wrote: > > On Wed, Sep 09, 2020 at 10:26:34PM -0400, Tom Rini wrote: > > > On Thu, Sep 10, 2020 at 10:54:18AM +0900, AKASHI Takahiro wrote: > > > > On Wed, Sep 09, 2020 at 02:30:53PM +0200, Heinrich Schuchardt wrote: > > > > > On 07.09.20 07:34, AKASHI Takahiro wrote: > > > > > > This function is essentially independent from tftp, and will also be > > > > > > utilised in implementing UEFI capsule update in a later commit. > > > > > > So just give it a more generic name. > > > > > > In addition, a new configuration option, CONFIG_DFU_ALT, was introduced > > > > > > so that the file will be compiled with different options, particularly > > > > > > one added in a later commit. > > > > > > > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > > > --- > > > > > > common/update.c | 5 +++-- > > > > > > drivers/dfu/Kconfig | 5 +++++ > > > > > > drivers/dfu/Makefile | 2 +- > > > > > > drivers/dfu/{dfu_tftp.c => dfu_alt.c} | 17 ++++++++++++-- > > > > > > include/dfu.h | 32 +++++++++++++-------------- > > > > > > 5 files changed, 40 insertions(+), 21 deletions(-) > > > > > > rename drivers/dfu/{dfu_tftp.c => dfu_alt.c} (67%) > > > > > > > > > > > > diff --git a/common/update.c b/common/update.c > > > > > > index 36b6b7523d50..39946776d74f 100644 > > > > > > --- a/common/update.c > > > > > > +++ b/common/update.c > > > > > > @@ -324,8 +324,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; > > > > > > } > > > > > > diff --git a/drivers/dfu/Kconfig b/drivers/dfu/Kconfig > > > > > > index 0eec00ba734d..78f901ff348a 100644 > > > > > > --- a/drivers/dfu/Kconfig > > > > > > +++ b/drivers/dfu/Kconfig > > > > > > @@ -14,8 +14,13 @@ config DFU_OVER_TFTP > > > > > > depends on NET > > > > > > > > > > > > if DFU > > > > > > +config DFU_ALT > > > > > > + bool > > > > > > > > > > You cannot define CONFIG_DFU_ALT as bool. This symbol already exists as > > > > > string in include/configs/odroid.h, include/configs/s5p_goni.h, et.al. > > > > > It is used to initialize environment variable dfu_alt_info. Your patch > > > > > leads to an error on Travis: > > > > > > > > > > arm: + odroid > > > > > > > > Okay, will fix it. > > > > DFU_WRITE_ALT? Or, as Heinrich suggested, move them > > > > to dfu.c if Lukasz agrees. > > > > > > > > # I haven't seen any comments from him yet though. > > > > > > I think what you're introducing as "CONFIG_DFU_ALT" and renaming > > > "drivers/dfu/dfu_tftp.c" to "drivers/dfu/dfu_alt.c" is what's confusing. > > > That I think needs a different name than "alt" to convey that it's a > > > generic "from DRAM to flash/etc" hook. > > > > Whatever the name is, I need reviews on all of my DFU patches from Lukasz > > in the first place. > > The lower impact on the rest of the DFU subsytem and other automated > testing that can be done, the easier it can be for someone else to > review the DFU parts if Lukasz isn't able to find time to do it. It sounds nice, but who will take that role? Please name that person(s) if you're still busy on other stuffs. -> Lukasz -Takahiro Akashi > Thanks. > > -- > Tom
On Fri, Sep 18, 2020 at 10:28:57AM +0900, AKASHI Takahiro wrote: > Tom, Lukasz, > > On Thu, Sep 10, 2020 at 10:41:39PM -0400, Tom Rini wrote: > > On Fri, Sep 11, 2020 at 09:46:14AM +0900, AKASHI Takahiro wrote: > > > On Wed, Sep 09, 2020 at 10:26:34PM -0400, Tom Rini wrote: > > > > On Thu, Sep 10, 2020 at 10:54:18AM +0900, AKASHI Takahiro wrote: > > > > > On Wed, Sep 09, 2020 at 02:30:53PM +0200, Heinrich Schuchardt wrote: > > > > > > On 07.09.20 07:34, AKASHI Takahiro wrote: > > > > > > > This function is essentially independent from tftp, and will also be > > > > > > > utilised in implementing UEFI capsule update in a later commit. > > > > > > > So just give it a more generic name. > > > > > > > In addition, a new configuration option, CONFIG_DFU_ALT, was introduced > > > > > > > so that the file will be compiled with different options, particularly > > > > > > > one added in a later commit. > > > > > > > > > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > > > > --- > > > > > > > common/update.c | 5 +++-- > > > > > > > drivers/dfu/Kconfig | 5 +++++ > > > > > > > drivers/dfu/Makefile | 2 +- > > > > > > > drivers/dfu/{dfu_tftp.c => dfu_alt.c} | 17 ++++++++++++-- > > > > > > > include/dfu.h | 32 +++++++++++++-------------- > > > > > > > 5 files changed, 40 insertions(+), 21 deletions(-) > > > > > > > rename drivers/dfu/{dfu_tftp.c => dfu_alt.c} (67%) > > > > > > > > > > > > > > diff --git a/common/update.c b/common/update.c > > > > > > > index 36b6b7523d50..39946776d74f 100644 > > > > > > > --- a/common/update.c > > > > > > > +++ b/common/update.c > > > > > > > @@ -324,8 +324,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; > > > > > > > } > > > > > > > diff --git a/drivers/dfu/Kconfig b/drivers/dfu/Kconfig > > > > > > > index 0eec00ba734d..78f901ff348a 100644 > > > > > > > --- a/drivers/dfu/Kconfig > > > > > > > +++ b/drivers/dfu/Kconfig > > > > > > > @@ -14,8 +14,13 @@ config DFU_OVER_TFTP > > > > > > > depends on NET > > > > > > > > > > > > > > if DFU > > > > > > > +config DFU_ALT > > > > > > > + bool > > > > > > > > > > > > You cannot define CONFIG_DFU_ALT as bool. This symbol already exists as > > > > > > string in include/configs/odroid.h, include/configs/s5p_goni.h, et.al. > > > > > > It is used to initialize environment variable dfu_alt_info. Your patch > > > > > > leads to an error on Travis: > > > > > > > > > > > > arm: + odroid > > > > > > > > > > Okay, will fix it. > > > > > DFU_WRITE_ALT? Or, as Heinrich suggested, move them > > > > > to dfu.c if Lukasz agrees. > > > > > > > > > > # I haven't seen any comments from him yet though. > > > > > > > > I think what you're introducing as "CONFIG_DFU_ALT" and renaming > > > > "drivers/dfu/dfu_tftp.c" to "drivers/dfu/dfu_alt.c" is what's confusing. > > > > That I think needs a different name than "alt" to convey that it's a > > > > generic "from DRAM to flash/etc" hook. > > > > > > Whatever the name is, I need reviews on all of my DFU patches from Lukasz > > > in the first place. > > > > The lower impact on the rest of the DFU subsytem and other automated > > testing that can be done, the easier it can be for someone else to > > review the DFU parts if Lukasz isn't able to find time to do it. > > It sounds nice, but who will take that role? Me. -- Tom
On Thu, 17 Sep 2020 21:32:25 -0400 Tom Rini <trini@konsulko.com> wrote: > On Fri, Sep 18, 2020 at 10:28:57AM +0900, AKASHI Takahiro wrote: > > Tom, Lukasz, > > > > On Thu, Sep 10, 2020 at 10:41:39PM -0400, Tom Rini wrote: > > > On Fri, Sep 11, 2020 at 09:46:14AM +0900, AKASHI Takahiro wrote: > > > > On Wed, Sep 09, 2020 at 10:26:34PM -0400, Tom Rini wrote: > > > > > On Thu, Sep 10, 2020 at 10:54:18AM +0900, AKASHI Takahiro > > > > > wrote: > > > > > > On Wed, Sep 09, 2020 at 02:30:53PM +0200, Heinrich > > > > > > Schuchardt wrote: > > > > > > > On 07.09.20 07:34, AKASHI Takahiro wrote: > > > > > > > > This function is essentially independent from tftp, and > > > > > > > > will also be utilised in implementing UEFI capsule > > > > > > > > update in a later commit. So just give it a more > > > > > > > > generic name. In addition, a new configuration option, > > > > > > > > CONFIG_DFU_ALT, was introduced so that the file will be > > > > > > > > compiled with different options, particularly one added > > > > > > > > in a later commit. > > > > > > > > > > > > > > > > Signed-off-by: AKASHI Takahiro > > > > > > > > <takahiro.akashi@linaro.org> --- > > > > > > > > common/update.c | 5 +++-- > > > > > > > > drivers/dfu/Kconfig | 5 +++++ > > > > > > > > drivers/dfu/Makefile | 2 +- > > > > > > > > drivers/dfu/{dfu_tftp.c => dfu_alt.c} | 17 > > > > > > > > ++++++++++++-- include/dfu.h | > > > > > > > > 32 +++++++++++++-------------- 5 files changed, 40 > > > > > > > > insertions(+), 21 deletions(-) rename > > > > > > > > drivers/dfu/{dfu_tftp.c => dfu_alt.c} (67%) > > > > > > > > > > > > > > > > diff --git a/common/update.c b/common/update.c > > > > > > > > index 36b6b7523d50..39946776d74f 100644 > > > > > > > > --- a/common/update.c > > > > > > > > +++ b/common/update.c > > > > > > > > @@ -324,8 +324,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; > > > > > > > > } > > > > > > > > diff --git a/drivers/dfu/Kconfig b/drivers/dfu/Kconfig > > > > > > > > index 0eec00ba734d..78f901ff348a 100644 > > > > > > > > --- a/drivers/dfu/Kconfig > > > > > > > > +++ b/drivers/dfu/Kconfig > > > > > > > > @@ -14,8 +14,13 @@ config DFU_OVER_TFTP > > > > > > > > depends on NET > > > > > > > > > > > > > > > > if DFU > > > > > > > > +config DFU_ALT > > > > > > > > + bool > > > > > > > > > > > > > > You cannot define CONFIG_DFU_ALT as bool. This symbol > > > > > > > already exists as string in include/configs/odroid.h, > > > > > > > include/configs/s5p_goni.h, et.al. It is used to > > > > > > > initialize environment variable dfu_alt_info. Your patch > > > > > > > leads to an error on Travis: > > > > > > > > > > > > > > arm: + odroid > > > > > > > > > > > > Okay, will fix it. > > > > > > DFU_WRITE_ALT? Or, as Heinrich suggested, move them > > > > > > to dfu.c if Lukasz agrees. > > > > > > > > > > > > # I haven't seen any comments from him yet though. > > > > > > > > > > I think what you're introducing as "CONFIG_DFU_ALT" and > > > > > renaming "drivers/dfu/dfu_tftp.c" to "drivers/dfu/dfu_alt.c" > > > > > is what's confusing. That I think needs a different name than > > > > > "alt" to convey that it's a generic "from DRAM to flash/etc" > > > > > hook. > > > > > > > > Whatever the name is, I need reviews on all of my DFU patches > > > > from Lukasz in the first place. > > > > > > The lower impact on the rest of the DFU subsytem and other > > > automated testing that can be done, the easier it can be for > > > someone else to review the DFU parts if Lukasz isn't able to find > > > time to do it. > > > > It sounds nice, but who will take that role? > > Me. > I thought that there is some ongoing discussion between Heinrich and Takahiro regarding those patches ... And yes, I'm busy with other work and my time budget is unfortunately reduced since March 2020. It would be good to have some help to not block ongoing DFU development. 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-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
diff --git a/common/update.c b/common/update.c index 36b6b7523d50..39946776d74f 100644 --- a/common/update.c +++ b/common/update.c @@ -324,8 +324,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; } diff --git a/drivers/dfu/Kconfig b/drivers/dfu/Kconfig index 0eec00ba734d..78f901ff348a 100644 --- a/drivers/dfu/Kconfig +++ b/drivers/dfu/Kconfig @@ -14,8 +14,13 @@ config DFU_OVER_TFTP depends on NET if DFU +config DFU_ALT + bool + default n + config DFU_TFTP bool "DFU via TFTP" + select DFU_ALT select DFU_OVER_TFTP help This option allows performing update of DFU-managed medium with data diff --git a/drivers/dfu/Makefile b/drivers/dfu/Makefile index 0d7925c083ef..cc7de1d3ed9b 100644 --- a/drivers/dfu/Makefile +++ b/drivers/dfu/Makefile @@ -9,5 +9,5 @@ obj-$(CONFIG_$(SPL_)DFU_MTD) += dfu_mtd.o obj-$(CONFIG_$(SPL_)DFU_NAND) += dfu_nand.o obj-$(CONFIG_$(SPL_)DFU_RAM) += dfu_ram.o obj-$(CONFIG_$(SPL_)DFU_SF) += dfu_sf.o -obj-$(CONFIG_$(SPL_)DFU_TFTP) += dfu_tftp.o +obj-$(CONFIG_$(SPL_)DFU_ALT) += dfu_alt.o obj-$(CONFIG_$(SPL_)DFU_VIRT) += dfu_virt.o diff --git a/drivers/dfu/dfu_tftp.c b/drivers/dfu/dfu_alt.c similarity index 67% rename from drivers/dfu/dfu_tftp.c rename to drivers/dfu/dfu_alt.c index ffae4bb54f80..5b1b13d7170d 100644 --- a/drivers/dfu/dfu_tftp.c +++ b/drivers/dfu/dfu_alt.c @@ -10,8 +10,21 @@ #include <errno.h> #include <dfu.h> -int dfu_tftp_write(char *dfu_entity_name, unsigned int addr, unsigned int len, - char *interface, char *devstring) +/** + * dfu_write_by_name() - write data to DFU medium + * @dfu_entity_name: Name of DFU entity to write + * @addr: Address of data buffer to write + * @len: Number of bytes + * @interface: Destination DFU medium (e.g. "mmc") + * @devstring: Instance number of destination DFU medium (e.g. "1") + * + * This function is storing data received on DFU supported medium which + * is specified by @dfu_entity_name. + * + * Return: 0 - on success, error code - otherwise + */ +int dfu_write_by_name(char *dfu_entity_name, unsigned int addr, + unsigned int len, char *interface, char *devstring) { char *s, *sb; int alt_setting_num, ret; diff --git a/include/dfu.h b/include/dfu.h index 84abdc79acd1..cecfbd76597b 100644 --- a/include/dfu.h +++ b/include/dfu.h @@ -494,27 +494,27 @@ static inline int dfu_fill_entity_virt(struct dfu_entity *dfu, char *devstr, #endif /** - * dfu_tftp_write() - write TFTP data to DFU medium + * dfu_write_by_name() - write data to DFU medium + * @dfu_entity_name: Name of DFU entity to write + * @addr: Address of data buffer to write + * @len: Number of bytes + * @interface: Destination DFU medium (e.g. "mmc") + * @devstring: Instance number of destination DFU medium (e.g. "1") * - * This function is storing data received via TFTP on DFU supported medium. + * This function is storing data received on DFU supported medium which + * is specified by @dfu_entity_name. * - * @dfu_entity_name: name of DFU entity to write - * @addr: address of data buffer to write - * @len: number of bytes - * @interface: destination DFU medium (e.g. "mmc") - * @devstring: instance number of destination DFU medium (e.g. "1") - * - * Return: 0 on success, otherwise error code + * Return: 0 - on success, error code - otherwise */ -#if CONFIG_IS_ENABLED(DFU_TFTP) -int dfu_tftp_write(char *dfu_entity_name, unsigned int addr, unsigned int len, - char *interface, char *devstring); +#if CONFIG_IS_ENABLED(DFU_ALT) +int dfu_write_by_name(char *dfu_entity_name, unsigned int addr, + unsigned int len, char *interface, char *devstring); #else -static inline int dfu_tftp_write(char *dfu_entity_name, unsigned int addr, - unsigned int len, char *interface, - char *devstring) +static inline int dfu_write_by_name(char *dfu_entity_name, unsigned int addr, + unsigned int len, char *interface, + char *devstring) { - puts("TFTP write support for DFU not available!\n"); + puts("write support for DFU not available!\n"); return -ENOSYS; } #endif
This function is essentially independent from tftp, and will also be utilised in implementing UEFI capsule update in a later commit. So just give it a more generic name. In addition, a new configuration option, CONFIG_DFU_ALT, was introduced so that the file will be compiled with different options, particularly one added in a later commit. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- common/update.c | 5 +++-- drivers/dfu/Kconfig | 5 +++++ drivers/dfu/Makefile | 2 +- drivers/dfu/{dfu_tftp.c => dfu_alt.c} | 17 ++++++++++++-- include/dfu.h | 32 +++++++++++++-------------- 5 files changed, 40 insertions(+), 21 deletions(-) rename drivers/dfu/{dfu_tftp.c => dfu_alt.c} (67%) -- 2.28.0