[U-Boot] [PATCH] common/image.c: Make boot_get_ramdisk() perform a check for Android images

[Rob Herring]

On Thu, Aug 27, 2015 at 4:47 PM, Tom Rini <trini at konsulko.com> wrote:
> On Thu, Aug 27, 2015 at 04:04:30PM -0500, Rob Herring wrote:
>> On Thu, Aug 27, 2015 at 2:42 PM, Tom Rini <trini at konsulko.com> wrote:
>> > In 2dd4632 the check for where a ramdisk is found on an Android image
>> > was got moved into the "normal" loop here, causing people to have to
>> > pass the kernel address in the ramdisk address location in order to have
>> > Android boot still.  This changed previous behavior so perform a check
>> > early in the function to see if we have an Android image and if so use
>> > that as where to look for the ramdisk (which is what the rest of the
>> > code here expects).
>> >
>> > Cc: Rob Herring <robh at kernel.org>
>> > Reported-by: Paul Kocialkowski <contact at paulk.fr>
>> > Signed-off-by: Tom Rini <trini at konsulko.com>
>> > ---
>> >  common/image.c |    9 +++++++++
>> >  1 file changed, 9 insertions(+)
>> >
>> > diff --git a/common/image.c b/common/image.c
>> > index ca721c5..e938bea 100644
>> > --- a/common/image.c
>> > +++ b/common/image.c
>> > @@ -907,6 +907,15 @@ int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images,
>> >         if (argc >= 2)
>> >                 select = argv[1];
>>
>> Perhaps this check should come second so you could override the
>> ramdisk with "bootm <bootimg> <ramdisk>". Then again, maybe people
>> should have to pick between a bootimg or separate components.
>
> Yeah, no, I'm not convinced there's a good case for "Android image
> kernel+ramdisk 1" kernel + "Android image+ramdisk 2" ramdisk where you
> wouldn't be cobbling that particular combination together outside of
> U-Boot anyhow.  Is there really?  And we still allow for disabling the
> ramdisk.  Or of course just loading separate components and booting
> Android that way.

I was thinking a separate raw ramdisk. But yes, I agree it is probably
better if we don't support all random combinations.

>> > +#ifdef CONFIG_ANDROID_BOOT_IMAGE
>> > +       /*
>> > +        * Look for an Android boot image.
>> > +        */
>> > +       buf = map_sysmem(images->os.start, 0);
>> > +       if (genimg_get_format(buf) == IMAGE_FORMAT_ANDROID)
>> > +               select = argv[0];
>> > +#endif
>>
>> Tracing code paths in these functions is bad enough. I would do
>> something like this to simplify the code path:
>>
>> buf = map_sysmem(images->os.start, 0);
>> if (genimg_get_format(buf) == IMAGE_FORMAT_ANDROID) {
>>   ret = android_image_get_ramdisk((void *)images->os.start, rd_start, &rd_len);
>>   *rd_end = *rd_start + rd_len;
>>   return ret;
>> }
>>
>> And then remove the case statement. We loose dataflash copy and some
>> debug prints.
>
> But then we're also saying Android images are a special case that needs
> to be treated differently than everyone else here.

The code seems to indicate it is given that most of it doesn't apply...

Having multiple decision points based on the image type makes it hard
to follow the flow. It would be much better if the code structure was:

common setup/parsing
switch (image type)
  - handle each image type
common clean-up

Maybe this function is the wrong level to do this restructuring. The
bootm related code needs some love, but I'm afraid to touch it with
any major change.

Rob