[U-Boot] [PATCH 1/2] include: android_bl_msg.h: Initial import

[Sam Protsenko]

Hi Eugeniu,

I created GitHub repo with all related patches applied on top of most
recent mainline U-Boot master: [1]. It contains next patches (on
"misc-reboot-reason" branch):

1. Series from Ruslan Trofymenko: [PATCH v3 0/7] android: implement
A/B boot process:
  * cmd: part: Add 'number' sub-command
  * disk: part: Extend API to get partition info
  * common: Implement A/B metadata
  * cmd: Add 'ab_select' command
  * test/py: Add base test case for A/B updates
  * doc: android: Add simple guide for A/B updates
  * env: am57xx: Implement A/B boot process
2. Series from Ruslan Trofymenko: [U-Boot][PATCH 0/4] Implement Reboot
reason support:
  * common: Implement Reboot reason flow
  * cmd: Add 'rb_reason' command
  * env: am57xx: Add Reboot reason support
  * configs: am57xx_evm: Enable Reboot reason support
  * Rename android_bl_msg.h -> android_bl_msg2.h (my patch to make
this series apply along with next patches from Eugeniu)
3. Series from Eugeniu Rosca: [PATCH 0/2] Add 'bcb' command to
read/modify/write Android BCB:
  * include: android_bl_msg.h: Initial import
  * cmd: Add 'bcb' command to read/modify/write BCB fields
4. My local patches to enable it all on X15 board, make it work
correctly, and use original android_bl_msg.h (as close as possible to
AOSP file):
  * configs: am57xx: Enable BCB command
  * environment: ti: Fix USB controller number
  * android: reboot reason: Use original android_bl_msg.h
  * android: ab: Use original android_bl_msg.h
  * android: Remove android_bl_msg2.h
  * android: bcb: Use original android_bl_msg.h API

With all those patches applied, I'm able to do next (after "adb reboot
bootloader" command):
1. Use "bcb" command by Eugeniu:
    => bcb load 1 4
    => bcb dump command
2. Use "rb_reason" command by Ruslan:
    => rb_reason mmc 1:4
    => rb_reason mmc 1:misc
3. U-Boot automatically gets into fastboot mode when "adb reboot
bootloader" command was issued

Now we need to figure out how to do next (w.r.t. repo [1]):
  1. How to merge your "bcb" command and Ruslan's "rb_reason" command;
I can see they both are working with "misc" BCB. So maybe it's good
idea to merge them into one command.
  2. How to handle android_bl_msg.h: it's a dependency for all 3 patch
series (A/B, "reboot reason" command, BCB command). Maybe we should
rework it and send as a separate patch, mentioning why it's needed,
and after it's applied, we can re-send our patch series without that
file included.

Please let me know what do you think.

Thanks!

[1] https://github.com/joe-skb7/u-boot-misc/commits/misc-reboot-reason

On Wed, May 8, 2019 at 5:46 PM Eugeniu Rosca <erosca at de.adit-jv.com> wrote:
>
> Hi Sam,
>
> On Wed, May 08, 2019 at 05:26:04PM +0300, Sam Protsenko wrote:
> > Hi Eugeniu,
> >
> > Please see my comments below. Overall, we will need to think it over
> > and merge 3 patch series somehow (this one, the one for A/B support
> > from Ruslan, and one internal patch series Ruslan sent for reboot
> > reason support). I will provide more details a bit later, working on
> > this right now.
>
> Thanks for letting me know. I am willing to cooperate, if needed.
> To my understanding, this patch is the only overlapping area, so I
> think the idea is to agree on its contents/structure. Then, you could
> include it in your series in the agreed shape. Once it is merged, I
> will rebase my work on top of u-boot/master.
>
> > On Mon, Apr 8, 2019 at 1:02 AM Eugeniu Rosca <roscaeugeniu at gmail.com> wrote:
> [..]
> > > +
> > > +#ifdef __UBOOT__
> > > +/* U-Boot-specific types for improved syntax/readability */
> > > +typedef struct bootloader_message      andr_bl_msg;
> > > +typedef struct bootloader_message_ab   andr_bl_msg_ab;
> > > +typedef struct bootloader_control      andr_bl_control;
> >
> > I would argue against creating user types for structures. It breaks
> > next rule from Linux kernel coding style (which we use in U-Boot):
> >
> >     "It’s a mistake to use typedef for structures and pointers."
> >
> > Which more important, it breaks that rule outside of this file. Also,
> > as you mentioned before, it's probably wise to keep this file as close
> > as possible to original, as we will need to update it at some point.
>
> I will wait for comments from Tom and will stick to whichever policy is
> established when importing out-of-tree headers.
>
> >
> > > +#endif
> > > +
> > > +// Spaces used by misc partition are as below:
> > > +// 0   - 2K     For bootloader_message
> > > +// 2K  - 16K    Used by Vendor's bootloader (the 2K - 4K range may be optionally used
> > > +//              as bootloader_message_ab struct)
> > > +// 16K - 64K    Used by uncrypt and recovery to store wipe_package for A/B devices
> > > +// Note that these offsets are admitted by bootloader,recovery and uncrypt, so they
> > > +// are not configurable without changing all of them.
> > > +
> > > +#ifndef __UBOOT__
> >
> > This stuff is very nice. I guess we can upstream it further to AOSP,
> > then this file will be almost the same in AOSP and in U-Boot.
>
> Sounds good to me.
>
> --
> Best Regards,
> Eugeniu.