common/image.c: Make boot_get_ramdisk() perform a check for Android images
PATCH 1/1 common/image.c: Make boot_get_ramdisk() perform a check for Android images
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).
--- common/image.c | 9 +++++++++ 1 file changed, 9 insertions(+)
common/image.c +9 -0
@@ -907,6 +907,15 @@ int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images,if (argc >=2 ) select = argv[1 ]; +#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 +/* * Look for a '-' which indicates to ignore the * ramdisk argument
Signature · 1 line
1.7.9.5
PATCH 1/1 common/image.c: Make boot_get_ramdisk() perform a check for Android images
Quoted 20 lines · Tom Rini
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(+)common/image.c +0
-0
common/image.c int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images, · line 908
Quoted 2 lines
common/image.c +0
-0@@ -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.
common/image.c int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images, · line 917
Quoted 9 lines
common/image.c +8
-0+#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.
Rob
PATCH 1/1 common/image.c: Make boot_get_ramdisk() perform a check for Android images
Quoted 16 lines · Rob Herring
Quoted 15 lines · Tom Rini
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(+)
Quoted 11 lines
common/image.c int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images, · line 908
Quoted 7 lines
common/image.c +0
-0@@ -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.
Quoted 19 lines
common/image.c int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images, · line 917
Quoted 8 lines
common/image.c +8
-0+#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;
}
Quoted 2 lines
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.
Signature · 8 lines
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>
PATCH 1/1 common/image.c: Make boot_get_ramdisk() perform a check for Android images
Quoted 29 lines · Tom Rini
Quoted 16 lines · Rob Herring
Quoted 15 lines · Tom Rini
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(+)Quoted 11 lines
common/image.c int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images, · line 908
Quoted 7 lines
common/image.c +0
-0@@ -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.
Quoted 6 lines
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.
Quoted 19 lines
Quoted 19 lines
common/image.c int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images, · line 917
Quoted 8 lines
common/image.c +8
-0+#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;
}
Quoted 5 lines
Quoted 2 lines
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
PATCH 1/1 common/image.c: Make boot_get_ramdisk() perform a check for Android images
Quoted 37 lines · Rob Herring
Quoted 29 lines · Tom Rini
Quoted 16 lines · Rob Herring
Quoted 15 lines · Tom Rini
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(+)Quoted 11 lines
common/image.c int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images, · line 908
Quoted 7 lines
common/image.c +0
-0@@ -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.
Quoted 6 lines
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.
Quoted 2 lines
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.
Quoted 30 lines
Quoted 19 lines
Quoted 19 lines
common/image.c int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images, · line 917
Quoted 8 lines
common/image.c +8
-0+#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;
}Quoted 5 lines
Quoted 2 lines
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:
Quoted 8 lines
common setup/parsing
switch (image type)
- handle each image type
common clean-upMaybe 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.
Signature · 8 lines
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/20150828/161f155d/attachment.sig>
PATCH 1/1 common/image.c: Make boot_get_ramdisk() perform a check for Android images
In reply to Tom RiniLe jeudi 27 ao?t 2015 ? 15:42 -0400, Tom Rini a ?crit :
Quoted 7 lines
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).
That patch does fix my problem (the ramdisk is now correctly passed to the kernel). I suggest that you merge it ASAP.
Thanks!
Quoted 17 lines
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(+)common/image.c +2
-0@@ -907,6 +907,15 @@ int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images,if (argc >=2 ) select = argv[1 ]; +#ifdef CONFIG_ANDROID_BOOT_IMAGE +/*
common/image.c int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images, · line 921
Quoted 10 lines
common/image.c +7
-0+* 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 +/* * Look for a '-' which indicates to ignore the * ramdisk argument
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150901/56473b7c/attachment.sig>
PATCH 1/1 common/image.c: Make boot_get_ramdisk() perform a check for Android images
Quoted 11 lines · Paul Kocialkowski
Le jeudi 27 ao?t 2015 ? 15:42 -0400, Tom Rini a ?crit :
Quoted 7 lines
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).
That patch does fix my problem (the ramdisk is now correctly passed to the kernel). I suggest that you merge it ASAP.
Doesn't look like this was ever merged or respun. I had some comments, but have no issue if they addressed separately as part of some refactoring.
Rob
PATCH 1/1 common/image.c: Make boot_get_ramdisk() perform a check for Android images
Hi,
Le lundi 05 octobre 2015 ? 14:23 -0500, Rob Herring a ?crit :
Quoted 17 lines
Quoted 15 lines · Paul Kocialkowski
Le jeudi 27 ao?t 2015 ? 15:42 -0400, Tom Rini a ?crit :
Quoted 13 lines
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).
Quoted 8 lines
Quoted 3 lines
That patch does fix my problem (the ramdisk is now correctly passed to
the kernel). I suggest that you merge it ASAP.Doesn't look like this was ever merged or respun. I had some comments,
but have no issue if they addressed separately as part of some refactoring.
Well, I would like to see this getting merged soon, because Android booting on sniper (LG Optimus Black) is currently broken.
Tom, would you consider picking it up for the current rc round? I realize I had forgotten about this patch and should have suggested it earlier on in the cycle.
Either way, I should test the current rc on my device and report back if there is anything else going wrong.
Signature · 17 lines
Paul Kocialkowski, Replicant developer Replicant is a fully free Android distribution running on several devices, a free software mobile operating system putting the emphasis on freedom and privacy/security. Website: https://www.replicant.us/ Blog: https://blog.replicant.us/ Wiki/tracker/forums: https://redmine.replicant.us/ -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: This is a digitally signed message part URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151007/d99b0234/attachment.sig>
PATCH 1/1 common/image.c: Make boot_get_ramdisk() perform a check for Android images
Hi,
Le mercredi 07 octobre 2015 ? 20:07 +0200, Paul Kocialkowski a ?crit :
Quoted 30 lines
Le lundi 05 octobre 2015 ? 14:23 -0500, Rob Herring a ?crit :
Quoted 17 lines
Quoted 15 lines · Paul Kocialkowski
Le jeudi 27 ao?t 2015 ? 15:42 -0400, Tom Rini a ?crit :
Quoted 13 lines
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).Quoted 8 lines
Quoted 3 lines
That patch does fix my problem (the ramdisk is now correctly passed to
the kernel). I suggest that you merge it ASAP.Doesn't look like this was ever merged or respun. I had some comments,
but have no issue if they addressed separately as part of some refactoring.Well, I would like to see this getting merged soon, because Android booting on sniper (LG Optimus Black) is currently broken.
Quoted 6 lines
Tom, would you consider picking it up for the current rc round? I realize I had forgotten about this patch and should have suggested it earlier on in the cycle.
Either way, I should test the current rc on my device and report back if there is anything else going wrong.
This is about the only problem I have on the LG Optimus Black P970 (codename sniper) with the current git master, so merging this patch makes the device usable.
Signature · 12 lines
Paul Kocialkowski * Site web : http://www.paulk.fr/ * Blog : http://blog.paulk.fr/ * Dev blog : http://code.paulk.fr/ -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: This is a digitally signed message part URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151010/929ad834/attachment.sig>
PATCH 1/1 common/image.c: Make boot_get_ramdisk() perform a check for Android images
In reply to Rob HerringQuoted 10 lines · Rob Herring
Quoted 9 lines · Paul Kocialkowski
Le jeudi 27 ao?t 2015 ? 15:42 -0400, Tom Rini a ?crit :
Quoted 7 lines
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).
Quoted 6 lines
Quoted 2 lines
That patch does fix my problem (the ramdisk is now correctly passed to the kernel). I suggest that you merge it ASAP.
Doesn't look like this was ever merged or respun. I had some comments, but have no issue if they addressed separately as part of some refactoring.
OK. With respect to the code flow, that's on the "someday, probably" list. I've shuffled things around so that you can still provide a separate ramdisk to override the image as that was the use-case I hadn't considered before.
Signature · 8 lines
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/20151011/45cd43e2/attachment.sig>
Quoted 11 lines · Tom Rini
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]>
Moved around slightly so that we can override the ramdisk still and then:
Applied to u-boot/master, thanks!
Signature · 8 lines
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/20151012/09779d54/attachment.sig>