[U-Boot] [PATCH v2 6/9] update: tftp: dfu: Extend update_tftp() function to support DFU
Lukasz Majewski
l.majewski at majess.pl
Mon Aug 10 19:01:45 CEST 2015
Hi Simon,
> Hi Lukasz,
>
> On 25 July 2015 at 02:11, Lukasz Majewski <l.majewski at majess.pl>
> wrote:
> > This code allows using DFU defined mediums for storing data
> > received via TFTP protocol.
> >
> > It reuses and preserves functionality of legacy code at
> > common/update.c.
> >
> > The update_tftp() function now accepts parameters - namely medium
> > device name and its number (e.g. mmc 1).
> >
> > Without this information passed old behavior is preserved.
> >
> > Signed-off-by: Lukasz Majewski <l.majewski at majess.pl>
> > ---
> > Changes for v2:
> > - Remove env variables from update_tftp() function
> > - Add parameters to update_tftp() function - without them old
> > behavior is preserved
> > - Stop compilation when legacy flags (CONFIG_UPDATE_TFTP and
> > CONFIG_SYS_NO_FLASH) are wrongly defined
> > - In the u-boot code legacy calls to update_tftp(0UL) have been
> > changed to update_tftp(0UL, NULL, NULL)
> > ---
> > common/Makefile | 1 +
> > common/cmd_fitupd.c | 2 +-
> > common/main.c | 2 +-
> > common/update.c | 40 ++++++++++++++++++++++++++++++----------
> > include/net.h | 2 +-
> > 5 files changed, 34 insertions(+), 13 deletions(-)
> >
> > diff --git a/common/Makefile b/common/Makefile
> > index d6c1d48..76626f1 100644
> > --- a/common/Makefile
> > +++ b/common/Makefile
> > @@ -208,6 +208,7 @@ obj-$(CONFIG_LYNXKDI) += lynxkdi.o
> > obj-$(CONFIG_MENU) += menu.o
> > obj-$(CONFIG_MODEM_SUPPORT) += modem.o
> > obj-$(CONFIG_UPDATE_TFTP) += update.o
> > +obj-$(CONFIG_DFU_TFTP) += update.o
> > obj-$(CONFIG_USB_KEYBOARD) += usb_kbd.o
> > obj-$(CONFIG_CMD_DFU) += cmd_dfu.o
> > obj-$(CONFIG_CMD_GPT) += cmd_gpt.o
> > diff --git a/common/cmd_fitupd.c b/common/cmd_fitupd.c
> > index b045974..78b8747 100644
> > --- a/common/cmd_fitupd.c
> > +++ b/common/cmd_fitupd.c
> > @@ -23,7 +23,7 @@ static int do_fitupd(cmd_tbl_t *cmdtp, int flag,
> > int argc, char * const argv[]) if (argc == 2)
> > addr = simple_strtoul(argv[1], NULL, 16);
> >
> > - return update_tftp(addr);
> > + return update_tftp(addr, NULL, NULL);
> > }
> >
> > U_BOOT_CMD(fitupd, 2, 0, do_fitupd,
> > diff --git a/common/main.c b/common/main.c
> > index 2979fbe..ead0cd1 100644
> > --- a/common/main.c
> > +++ b/common/main.c
> > @@ -75,7 +75,7 @@ void main_loop(void)
> > run_preboot_environment_command();
> >
> > #if defined(CONFIG_UPDATE_TFTP)
> > - update_tftp(0UL);
> > + update_tftp(0UL, NULL, NULL);
> > #endif /* CONFIG_UPDATE_TFTP */
> >
> > s = bootdelay_process();
> > diff --git a/common/update.c b/common/update.c
> > index 542915c..1d082b0 100644
> > --- a/common/update.c
> > +++ b/common/update.c
> > @@ -13,11 +13,16 @@
> > #error "CONFIG_FIT and CONFIG_OF_LIBFDT are required for
> > auto-update feature" #endif
> >
> > +#if defined(CONFIG_UPDATE_TFTP) && defined(CONFIG_SYS_NO_FLASH)
> > +#error "CONFIG_UPDATE_TFTP and CONFIG_SYS_NO_FLASH needed for
> > legacy behaviour" +#endif
> > +
> > #include <command.h>
> > #include <flash.h>
> > #include <net.h>
> > #include <net/tftp.h>
> > #include <malloc.h>
> > +#include <dfu.h>
> >
> > /* env variable holding the location of the update file */
> > #define UPDATE_FILE_ENV "updatefile"
> > @@ -222,13 +227,17 @@ static int update_fit_getparams(const void
> > *fit, int noffset, ulong *addr, return 0;
> > }
> >
> > -int update_tftp(ulong addr)
> > +int update_tftp(ulong addr, char *interface, char *devstring)
>
> Should these be const char *?
>
> > {
> > - char *filename, *env_addr;
> > - int images_noffset, ndepth, noffset;
> > + char *filename, *env_addr, *fit_image_name;
>
> And these?
I'm quite puzzled here. In other places DFU code is operating on the
char * strings. Even in the dfu.h file all other functions use char *.
To do it right I would need to change char * to const char * in many
places at the DFU subsystem. Hence I think that such major change
deserves a separate patch series - not related to this one.
For the reasons presented above, I would opt for leaving the code as it
is and afterwards change char * to const char * globally for DFU
subsystem.
>
> > ulong update_addr, update_fladdr, update_size;
> > - void *fit;
> > + int images_noffset, ndepth, noffset;
> > + bool update_tftp_dfu = false;
> > int ret = 0;
> > + void *fit;
> > +
> > + if (interface && devstring)
> > + update_tftp_dfu = true;
> >
> > /* use already present image */
> > if (addr)
> > @@ -277,8 +286,8 @@ got_update_file:
> > if (ndepth != 1)
> > goto next_node;
> >
> > - printf("Processing update '%s' :",
> > - fit_get_name(fit, noffset, NULL));
> > + fit_image_name = (char *)fit_get_name(fit, noffset,
> > NULL);
>
> Can you drop that cast?
>
> > + printf("Processing update '%s' :", fit_image_name);
> >
> > if (!fit_image_verify(fit, noffset)) {
> > printf("Error: invalid update hash,
> > aborting\n"); @@ -294,10 +303,21 @@ got_update_file:
> > ret = 1;
> > goto next_node;
> > }
> > - if (update_flash(update_addr, update_fladdr,
> > update_size)) {
> > - printf("Error: can't flash update,
> > aborting\n");
> > - ret = 1;
> > - goto next_node;
> > +
> > + if (!update_tftp_dfu) {
> > + if (update_flash(update_addr, update_fladdr,
> > + update_size)) {
> > + printf("Error: can't flash update,
> > aborting\n");
> > + ret = 1;
> > + goto next_node;
> > + }
> > + } else if (fit_image_check_type(fit, noffset,
> > + IH_TYPE_FIRMWARE)) {
> > + if (dfu_tftp_write(fit_image_name,
> > update_addr,
> > + update_size, interface,
> > devstring)) {
> > + ret = 1;
> > + goto next_node;
> > + }
> > }
> > next_node:
> > noffset = fdt_next_node(fit, noffset, &ndepth);
> > diff --git a/include/net.h b/include/net.h
> > index d17173d..9af3190 100644
> > --- a/include/net.h
> > +++ b/include/net.h
> > @@ -804,7 +804,7 @@ void copy_filename(char *dst, const char *src,
> > int size); unsigned int random_port(void);
> >
> > /* Update U-Boot over TFTP */
> > -int update_tftp(ulong addr);
> > +int update_tftp(ulong addr, char *interface, char *devstring);
>
> Function comment - what are the parameters?
No problem, I will fix it.
>
> >
> > /**********************************************************************/
> >
> > --
> > 2.1.4
> >
>
> Regards,
> Simon
Best regards,
Lukasz Majewski
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150810/17037d20/attachment.sig>
More information about the U-Boot
mailing list