| Message ID | 1440704561-17015-1-git-send-email-trini@konsulko.com |
|---|---|
| State | Accepted |
| Delegated to: | Tom Rini |
| Headers | show |
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. > > +#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
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. > > +#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.
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. >> > +#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
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.
Le jeudi 27 août 2015 à 15:42 -0400, Tom Rini a écrit : > 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! > 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]; > > +#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
On Tue, Sep 1, 2015 at 8:50 AM, Paul Kocialkowski <contact@paulk.fr> wrote: > Le jeudi 27 août 2015 à 15:42 -0400, Tom Rini a écrit : >> 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
Hi, Le lundi 05 octobre 2015 à 14:23 -0500, Rob Herring a écrit : > On Tue, Sep 1, 2015 at 8:50 AM, Paul Kocialkowski <contact@paulk.fr> > wrote: > > Le jeudi 27 août 2015 à 15:42 -0400, Tom Rini a écrit : > > > 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. 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.
Hi, Le mercredi 07 octobre 2015 à 20:07 +0200, Paul Kocialkowski a écrit : > Le lundi 05 octobre 2015 à 14:23 -0500, Rob Herring a écrit : > > On Tue, Sep 1, 2015 at 8:50 AM, Paul Kocialkowski <contact@paulk.fr> > > wrote: > > > Le jeudi 27 août 2015 à 15:42 -0400, Tom Rini a écrit : > > > > 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. > > 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. 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.
On Mon, Oct 05, 2015 at 02:23:40PM -0500, Rob Herring wrote: > On Tue, Sep 1, 2015 at 8:50 AM, Paul Kocialkowski <contact@paulk.fr> wrote: > > Le jeudi 27 août 2015 à 15:42 -0400, Tom Rini a écrit : > >> 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. 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.
On Thu, Aug 27, 2015 at 03:42:41PM -0400, Tom Rini 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> Moved around slightly so that we can override the ramdisk still and then: Applied to u-boot/master, thanks!
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]; +#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
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(+)