[U-Boot] [PATCH v2 6/9] update: tftp: dfu: Extend update_tftp() function to support DFU

[Simon Glass]

Hi Lukasz,

On 10 August 2015 at 11:01, Lukasz Majewski <l.majewski at majess.pl> wrote:
> 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.

Sounds good, thanks.

[snip]

Regards,
Simon