On Fri, Aug 28, 2015 at 10:35:50AM -0500, Rob Herring wrote: > On Thu, Aug 27, 2015 at 4:47 PM, Tom Rini <[email protected]> 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 <[email protected]> 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 <[email protected]> > >> > Reported-by: Paul Kocialkowski <[email protected]> > >> > Signed-off-by: Tom Rini <[email protected]> > >> > --- > >> > 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.
OK, separate raw ramdisk is a use case I can get behind, so check
android image @ argv[0], then if argv[1] exists poke at that.
> >> > +#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.
Yeah, Simon gave things a re-org a while back and it took forever to
shake out some of the corner cases. There's room for futher
improvement still.
--
Tom
signature.asc
Description: Digital signature
_______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
