[U-Boot] [PATCH v3 13/13] net: fastboot: Merge AOSP UDP fastboot

[Alex Kiernan]

On Tue, May 15, 2018 at 5:05 PM Simon Glass <sjg at chromium.org> wrote:

> Hi Alex,

> On 15 May 2018 at 07:11, Alex Kiernan <alex.kiernan at gmail.com> wrote:
> > On Mon, May 14, 2018 at 8:53 PM Simon Glass <sjg at chromium.org> wrote:
> >
> >> Hi Alex,
> >
> >> On 14 May 2018 at 03:09, Alex Kiernan <alex.kiernan at gmail.com> wrote:
> >> > Merge UDP fastboot support from AOSP:
> >> >
> >> >
> >
https://android.googlesource.com/platform/external/u-boot/+/android-o-mr1-iot-preview-8
> >> >
> >> > Signed-off-by: Alex Kiernan <alex.kiernan at gmail.com>
> >> > Signed-off-by: Alex Deymo <deymo at google.com>
> >> > Signed-off-by: Jocelyn Bohr <bohr at google.com>
> >> > ---
> >> >
> >> > Changes in v3:
> >> > - use FASTBOOT as our guard in Kconfig not a list of USB || UDP
> >> > - correct mis-translation from AOSP introduced when cleaning up for
> >> >   checkpatch - we should write when buffer is not NULL, rather than
> >> >   erasing, and erase when buffer is NULL
> >> > - use CMD_RET_USAGE from do_fastboot
> >> > - remove do_fastboot_udp from cmd/net.c and rewrite using net_loop()
> >> > - rename timed_send_info to fastboot_send_info, rename
> > fastboot_send_info to
> >> >   fastboot_udp_send_info
> >> > - replace FASTBOOT_HEADER_SIZE with sizeof(struct fastboot_header)
> >> > - move start time into timed_send_info() rather than passing it in
> >> > - make calls to fastboot_udp_send_info a runtime dependency, not a
> > compile
> >> >   time one
> >> > - set ${filesize} to size of downloaded image
> >> > - add progress meter from USB path during download
> >> > - add support for 'oem format' command from the USB path
> >> > - rename 'fastbootcmd' to 'fastboot_bootcmd' to make clear that this
is
> > the
> >> >   fastboot boot command
> >> > - make getvar implementation table driven
> >> > - add fastboot_buf_addr, fastboot_buf_size to override buffer address
> > and
> >> >   size
> >> > - return correct filesystem type in getvar partition-type on MMC
> >> > - process "fastboot." prefixed env variables in getvar first so you
> >> >   can override the normal values (this also lets you set a fs type
for
> >> >   NAND devices)
> >> > - squash subsequent patches which change this code into this one:
> >> >   - If the fastboot flash/erase commands are disabled, remove that
> > support
> >> >     so we still build correctly.
> >> >   - Add NAND support to fastboot UDP flash/erase commands
> >> >   - If we don't have a partition name passed, report it as not found.
> >> >   - Change the behaviour of the fastboot net code such that
> >> >     "reboot-bootloader" is no longer written to
> > CONFIG_FASTBOOT_BUF_ADDR for
> >> >     use as a marker on reboot (the AOSP code in
> > common/android-bootloader.c
> >> >     uses this marker - this code could be reinstated there if that
gets
> >> >     merged).
> >> >   - Merge USB and UDP boot code. The USB implementation stays the
same,
> > but
> >> >     UDP no longer passes an fdt. We introduce a new environment
variable
> >> >     'fastboot_bootcmd' which if set overrides the hardcoded boot
> > command,
> >> >     setting this then allows the UDP implementation to remain the
same.
> > If
> >> >     after running 'fastboot_bootcmd' the board has not booted,
control
> > is
> >> >     returned to U-Boot and the fastboot process ends.
> >> >   - Separate the fastboot protocol handling from the fastboot UDP
code
> > in
> >> >     preparation for reusing it in the USB code.
> >> >
> >> > Changes in v2:
> >> > - ensure fastboot syntax is backward compatible - 'fastboot 0' means
> >> >   'fastboot usb 0'
> >> >
> >> >  cmd/fastboot.c                |  91 ++++++++++++-
> >> >  drivers/fastboot/Kconfig      |  15 ++
> >> >  drivers/fastboot/Makefile     |   2 +
> >> >  drivers/fastboot/fb_command.c | 258
+++++++++++++++++++++++++++++++++++
> >> >  drivers/fastboot/fb_common.c  |  48 +++++++
> >> >  drivers/fastboot/fb_getvar.c  | 222 ++++++++++++++++++++++++++++++
> >> >  drivers/fastboot/fb_mmc.c     |  67 ++++++++-
> >> >  drivers/fastboot/fb_nand.c    |  12 +-
> >> >  include/fastboot.h            |  47 +++++++
> >> >  include/fb_mmc.h              |   8 +-
> >> >  include/fb_nand.h             |  10 +-
> >> >  include/net.h                 |   2 +-
> >> >  include/net/fastboot.h        |  21 +++
> >> >  net/Makefile                  |   1 +
> >> >  net/fastboot.c                | 310
> > ++++++++++++++++++++++++++++++++++++++++++
> >> >  net/net.c                     |   7 +
> >> >  16 files changed, 1105 insertions(+), 16 deletions(-)
> >> >  create mode 100644 drivers/fastboot/fb_command.c
> >> >  create mode 100644 drivers/fastboot/fb_getvar.c
> >> >  create mode 100644 include/net/fastboot.h
> >> >  create mode 100644 net/fastboot.c
> >
> >> Reviewed-by: Simon Glass <sjg at chromium.org>
> >
> >> Please can you add function comments to the rest of the fastboot.h
> >> stuff, see if you can remove the global variables in that file, and
> >> also add function comments to non-trivial static functions?
> >
> >
> > Will do... I don't think I can easily make the remaining globals go
away,
> > but I can certainly put them behind accessor functions.

> If you can't make them go away, or put them in a struct, then I don't
> think you should bother with accessor function, which just mess up the
> code IMO.

So it was a worthwhile exercise to go through it again... I got two more of
the globals into statics (and a function to return the amount of data we
still expect to transfer), which left the buffer address, buffer size, and
the progress callback to keep the protocol alive, all of which are only
used inside drivers/fastboot itself, which I'll move to a
fastboot-internal.h


--
Alex Kiernan