[U-Boot] [RFC PATCH v1 0/5] Add fastboot UDP support

[Alex Kiernan]

On Tue, Apr 24, 2018 at 6:10 PM, Jocelyn Bohr <bohr at verily.com> wrote:
> Thanks so much for porting this, Alex!
>
> On Tue, Apr 24, 2018 at 4:58 AM Alex Kiernan <alex.kiernan at gmail.com> wrote:
>>
>> On Tue, Apr 24, 2018 at 11:24 AM, Alex Deymo <deymo+ at google.com> wrote:
>> > +Jocelyn.
>> >
>> > Thanks Alex for taking the time to port this!!
>>
>> It turned out to be a great opportunity to play with coccinelle on
>> something trivial, which I'd been meaning to do for ages... the
>> refactor to add response into the fastboot_okay/fail call chain was a
>> breeze with it.
>>
>> > 2018-04-24 11:37 GMT+02:00 Alex Kiernan <alex.kiernan at gmail.com>:
>> >>
>> >>
>> >> This series merges the fastboot UDP support from AOSP into mainline
>> >> U-Boot.
>> >>
>> >> Open questions:
>> >>
>> >> - what's the correct way of attributing the original authors? I've
>> >> added
>> >>   Co-authored-by, is that right? checkpatch doesn't seem to know any
>> >>   of the co- tags
>> >> - currently there's no NAND support and I've no way of testing that, so
>> >>   my inclination is towards leaving it like that until someone with
>> >> that
>> >>   particular itch to scratch can look at it
>> >
>> >
>> > Fastboot uses partition names, like "system" and "boot" which have a
>> > meaning
>> > in the Android partition scheme. For GPT, we just use the partition
>> > names as
>> > the android names; but if you are booting from some other storage like
>> > NAND
>> > where you don't have that then you need some mapping glue ("system" -->
>> > device and partition number). I've seen U-Boot modifications for several
>> > devices where they just stick a table hard-coded in the U-Boot code;
>> > which
>> > works great for that device but it isn't really a generic approach.
>> > Other
>> > than handling the names issue, I don't see any problem with supporting
>> > NAND
>> > here, but a generic way to set global names / alias would be needed for
>> > this.
>> >
>>
>> With the fastboot NAND support in mainline it looks like we end up in
>> mtdparts to deliver that mapping through environment variables. No
>> actual idea what that looks like when you're driving it.
>>
>> >>
>> >> - you can select both USB and UDP fastboot, but the comments in the
>> >>   AOSP code suggest that needs fixing - again, I've no board I can test
>> >>   USB fastboot on
>> >
>> >
>> > I thought we checked in the Kconfig that you couldn't enable both. I
>> > don't
>> > remember the details now but yeah you can't wait for network or USB
>> > traffic
>> > on the current code.
>> >
>>
>> The version I picked from (o-mr1-iot-preview-8) has them as
>> independent symbols, but making them a choice would resolve the issue
>> for now.
>
>
> You can select both network and USB fastboot to be enabled with the Kconfig,
> but at runtime you have to select to wait on either USB or network by
> running
> "fastboot udp" or "fastboot usb <USB_controller>". When the Android
> bootloader
> gets the command to reboot back to fastboot, it will read the "fastbootcmd"
> env
> variable and run that as a command (common/android_bootloader.c:151).
>

Thanks for that (especially the detail on android_bootloader which I'd
not read through). The bit that concerns me is in timed_send_info:

  +/**
  + * Send an INFO packet during long commands based on timer. If
  + * CONFIG_UDP_FUNCTION_FASTBOOT is defined, an INFO packet is sent
  + * if the time is 30 seconds after start. Else, noop.
  + *
  + * TODO: Handle the situation where both UDP and USB fastboot are
  + *       enabled.
  + *
  + * @param start:  Time since last INFO packet was sent.
  + * @param msg:    String describing the reason for waiting
  + */
  +void timed_send_info(ulong *start, const char *msg);

The code in timed_send_info is guarded with an ifdef, rather than
based on the transport that's been selected at runtime. Do we need to
make timed_send_info work based on the runtime state, rather than the
compile time, or can we drop the ifdef guard and remove the TODO? I
guess I'm assuming the former, but with no way to test USB I don't
want head off down the wrong road!

-- 
Alex Kiernan