On Fri, Aug 28, 2015 at 10:35:50AM -0500, Rob Herring wrote:
On Thu, Aug 27, 2015 at 4:47 PM, Tom Rini trini@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@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@kernel.org Reported-by: Paul Kocialkowski contact@paulk.fr Signed-off-by: Tom Rini trini@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.
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.