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

[Simon Glass]

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.

Regards,
Simon