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

[Tom Rini]

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.

> > +#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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150827/958bf440/attachment.sig>