| Message ID | 1320164902-24190-1-git-send-email-swarren@nvidia.com |
|---|---|
| State | Changes Requested |
| Headers | show |
> image_get_ram_disk() and image_get_kernel() perform operations in a > consistent order. Modify image_get_fdt() to do things the same way. > This allows a later change to insert some image header manipulations > into these three functions in a consistent fashion. > > v2: New patch > > Signed-off-by: Stephen Warren <swarren@nvidia.com> Hi Stephen, this patchset is good and all, but can we not also introduce cmd_zload to load zImages? Wolfgang, today's ARM hardware will really benefit from that, uImage holds us back a lot these days. Other option is to extend cmd_bootm() to load zImages. Cheers
Hi, On Sat, Nov 5, 2011 at 11:41 AM, Marek Vasut <marek.vasut@gmail.com> wrote: >> image_get_ram_disk() and image_get_kernel() perform operations in a >> consistent order. Modify image_get_fdt() to do things the same way. >> This allows a later change to insert some image header manipulations >> into these three functions in a consistent fashion. >> >> v2: New patch >> >> Signed-off-by: Stephen Warren <swarren@nvidia.com> > > Hi Stephen, > > this patchset is good and all, but can we not also introduce cmd_zload to load > zImages? Wolfgang, today's ARM hardware will really benefit from that, uImage > holds us back a lot these days. Other option is to extend cmd_bootm() to load > zImages. > > Cheers > Just a quick Q. What is the ultimate intent here? Should we be aiming to have U-Boot copy and decompress the data into RAM ready for Linux? In theory this should be slightly faster since U-Boot already has the data in its cache. I think zImage now supports having an FDT inside but what is the advantage of zImage over a uImage with compressed portions? Regards, Simon
> Hi, > > On Sat, Nov 5, 2011 at 11:41 AM, Marek Vasut <marek.vasut@gmail.com> wrote: > >> image_get_ram_disk() and image_get_kernel() perform operations in a > >> consistent order. Modify image_get_fdt() to do things the same way. > >> This allows a later change to insert some image header manipulations > >> into these three functions in a consistent fashion. > >> > >> v2: New patch > >> > >> Signed-off-by: Stephen Warren <swarren@nvidia.com> > > > > Hi Stephen, > > > > this patchset is good and all, but can we not also introduce cmd_zload to > > load zImages? Wolfgang, today's ARM hardware will really benefit from > > that, uImage holds us back a lot these days. Other option is to extend > > cmd_bootm() to load zImages. > > > > Cheers > > Just a quick Q. What is the ultimate intent here? Should we be aiming > to have U-Boot copy and decompress the data into RAM ready for Linux? Nope, not at all. We have a problem with booting linux images which support multiple different SoCs (because the RAM might be elsewhere for different SoCs). > In theory this should be slightly faster since U-Boot already has the > data in its cache. I think zImage now supports having an FDT inside > but what is the advantage of zImage over a uImage with compressed > portions? That's not the point. We need to load FDT, load zImage, setup the regs and boot it from where we load the zImage. The uImage envelope contains fixed address to where the kernel image is loaded, which interferes with kernel's runtime patching of the kernel base address (zreladdr). Basically kernel can be loaded to address A1 on one SoC, address A2 on different SoC, but in the old times, the kernel had to be linked to a predefined address. That's not true anymore. Now if kernel is loaded to address A1, it adjusts itself and runs from A1, so for A2 etc. uImage blocks this because it forces u-boot to copy zImage to fixed address. > > Regards, > Simon
Hi Marek, On Sat, Nov 5, 2011 at 12:39 PM, Marek Vasut <marek.vasut@gmail.com> wrote: >> Hi, >> >> On Sat, Nov 5, 2011 at 11:41 AM, Marek Vasut <marek.vasut@gmail.com> wrote: >> >> image_get_ram_disk() and image_get_kernel() perform operations in a >> >> consistent order. Modify image_get_fdt() to do things the same way. >> >> This allows a later change to insert some image header manipulations >> >> into these three functions in a consistent fashion. >> >> >> >> v2: New patch >> >> >> >> Signed-off-by: Stephen Warren <swarren@nvidia.com> >> > >> > Hi Stephen, >> > >> > this patchset is good and all, but can we not also introduce cmd_zload to >> > load zImages? Wolfgang, today's ARM hardware will really benefit from >> > that, uImage holds us back a lot these days. Other option is to extend >> > cmd_bootm() to load zImages. >> > >> > Cheers >> >> Just a quick Q. What is the ultimate intent here? Should we be aiming >> to have U-Boot copy and decompress the data into RAM ready for Linux? > > Nope, not at all. We have a problem with booting linux images which support > multiple different SoCs (because the RAM might be elsewhere for different SoCs). > >> In theory this should be slightly faster since U-Boot already has the >> data in its cache. I think zImage now supports having an FDT inside >> but what is the advantage of zImage over a uImage with compressed >> portions? > > That's not the point. We need to load FDT, load zImage, setup the regs and boot > it from where we load the zImage. The uImage envelope contains fixed address to > where the kernel image is loaded, which interferes with kernel's runtime > patching of the kernel base address (zreladdr). > > Basically kernel can be loaded to address A1 on one SoC, address A2 on different > SoC, but in the old times, the kernel had to be linked to a predefined address. > That's not true anymore. Now if kernel is loaded to address A1, it adjusts > itself and runs from A1, so for A2 etc. > > uImage blocks this because it forces u-boot to copy zImage to fixed address. Stephen's patch set should fix that by allowing an unspecified load address . So this means that we are fine if we use a zImage, but what about a uImage? Are we giving up on that altogether? Stephen's original patch on this subject seemed to me to solve the problem with uImage (the one he had all the code size grief with). Can't we commit both of Stephen's patches? It seems to me that they solve different problems. Regards, Simon >> >> Regards, >> Simon >
> Hi Marek, > > On Sat, Nov 5, 2011 at 12:39 PM, Marek Vasut <marek.vasut@gmail.com> wrote: > >> Hi, > >> > >> On Sat, Nov 5, 2011 at 11:41 AM, Marek Vasut <marek.vasut@gmail.com> wrote: > >> >> image_get_ram_disk() and image_get_kernel() perform operations in a > >> >> consistent order. Modify image_get_fdt() to do things the same way. > >> >> This allows a later change to insert some image header manipulations > >> >> into these three functions in a consistent fashion. > >> >> > >> >> v2: New patch > >> >> > >> >> Signed-off-by: Stephen Warren <swarren@nvidia.com> > >> > > >> > Hi Stephen, > >> > > >> > this patchset is good and all, but can we not also introduce cmd_zload > >> > to load zImages? Wolfgang, today's ARM hardware will really benefit > >> > from that, uImage holds us back a lot these days. Other option is to > >> > extend cmd_bootm() to load zImages. > >> > > >> > Cheers > >> > >> Just a quick Q. What is the ultimate intent here? Should we be aiming > >> to have U-Boot copy and decompress the data into RAM ready for Linux? > > > > Nope, not at all. We have a problem with booting linux images which > > support multiple different SoCs (because the RAM might be elsewhere for > > different SoCs). > > > >> In theory this should be slightly faster since U-Boot already has the > >> data in its cache. I think zImage now supports having an FDT inside > >> but what is the advantage of zImage over a uImage with compressed > >> portions? > > > > That's not the point. We need to load FDT, load zImage, setup the regs > > and boot it from where we load the zImage. The uImage envelope contains > > fixed address to where the kernel image is loaded, which interferes with > > kernel's runtime patching of the kernel base address (zreladdr). > > > > Basically kernel can be loaded to address A1 on one SoC, address A2 on > > different SoC, but in the old times, the kernel had to be linked to a > > predefined address. That's not true anymore. Now if kernel is loaded to > > address A1, it adjusts itself and runs from A1, so for A2 etc. > > > > uImage blocks this because it forces u-boot to copy zImage to fixed > > address. > > Stephen's patch set should fix that by allowing an unspecified load address > . > > So this means that we are fine if we use a zImage, but what about a > uImage? Are we giving up on that altogether? Stephen's original patch > on this subject seemed to me to solve the problem with uImage (the one > he had all the code size grief with). I'd be more open to adding some kind of a flag to uImage, rather than using address 0xffffffff. For this is quite fragile and seems a lot like a hack. > > Can't we commit both of Stephen's patches? It seems to me that they > solve different problems. Yes, 1/3 and 2/3 look good. > > Regards, > Simon > > >> Regards, > >> Simon
Hi Marek, On Sat, Nov 5, 2011 at 1:41 PM, Marek Vasut <marek.vasut@gmail.com> wrote: >> Hi Marek, >> >> On Sat, Nov 5, 2011 at 12:39 PM, Marek Vasut <marek.vasut@gmail.com> wrote: >> >> Hi, >> >> >> >> On Sat, Nov 5, 2011 at 11:41 AM, Marek Vasut <marek.vasut@gmail.com> wrote: >> >> >> image_get_ram_disk() and image_get_kernel() perform operations in a >> >> >> consistent order. Modify image_get_fdt() to do things the same way. >> >> >> This allows a later change to insert some image header manipulations >> >> >> into these three functions in a consistent fashion. >> >> >> >> >> >> v2: New patch >> >> >> >> >> >> Signed-off-by: Stephen Warren <swarren@nvidia.com> >> >> > >> >> > Hi Stephen, >> >> > >> >> > this patchset is good and all, but can we not also introduce cmd_zload >> >> > to load zImages? Wolfgang, today's ARM hardware will really benefit >> >> > from that, uImage holds us back a lot these days. Other option is to >> >> > extend cmd_bootm() to load zImages. >> >> > >> >> > Cheers >> >> >> >> Just a quick Q. What is the ultimate intent here? Should we be aiming >> >> to have U-Boot copy and decompress the data into RAM ready for Linux? >> > >> > Nope, not at all. We have a problem with booting linux images which >> > support multiple different SoCs (because the RAM might be elsewhere for >> > different SoCs). >> > >> >> In theory this should be slightly faster since U-Boot already has the >> >> data in its cache. I think zImage now supports having an FDT inside >> >> but what is the advantage of zImage over a uImage with compressed >> >> portions? >> > >> > That's not the point. We need to load FDT, load zImage, setup the regs >> > and boot it from where we load the zImage. The uImage envelope contains >> > fixed address to where the kernel image is loaded, which interferes with >> > kernel's runtime patching of the kernel base address (zreladdr). >> > >> > Basically kernel can be loaded to address A1 on one SoC, address A2 on >> > different SoC, but in the old times, the kernel had to be linked to a >> > predefined address. That's not true anymore. Now if kernel is loaded to >> > address A1, it adjusts itself and runs from A1, so for A2 etc. >> > >> > uImage blocks this because it forces u-boot to copy zImage to fixed >> > address. >> >> Stephen's patch set should fix that by allowing an unspecified load address >> . >> >> So this means that we are fine if we use a zImage, but what about a >> uImage? Are we giving up on that altogether? Stephen's original patch >> on this subject seemed to me to solve the problem with uImage (the one >> he had all the code size grief with). > > I'd be more open to adding some kind of a flag to uImage, rather than using > address 0xffffffff. For this is quite fragile and seems a lot like a hack. Possibly, but at least it makes it very clear that the load address should not be used. >> >> Can't we commit both of Stephen's patches? It seems to me that they >> solve different problems. > > Yes, 1/3 and 2/3 look good. Actually I meant both series, sorry. The previous series enhanced uImage to understand an image that can go anywhere, and I think that is useful also. In both series, a load address of -1 means 'ignore it'. Regards, Simon >> >> Regards, >> Simon >> >> >> Regards, >> >> Simon >
Dear Marek Vasut, In message <201111051941.38920.marek.vasut@gmail.com> you wrote: > > this patchset is good and all, but can we not also introduce cmd_zload to load > zImages? Wolfgang, today's ARM hardware will really benefit from that, uImage > holds us back a lot these days. Other option is to extend cmd_bootm() to load > zImages. uImage holds us back? And hardware will benefit from that? I can't parse that. Could you please elucidate? What exactly _is_ the problem of uImage? Or have you ever considered using a FIT image? Best regards, Wolfgang Denk
Dear Simon Glass, In message <CAPnjgZ0bbhvZ1VPwY=0y9YZJdf+EOBgyKKVPFuAkBqy2z4L3HA@mail.gmail.com> you wrote: > > Just a quick Q. What is the ultimate intent here? Should we be aiming > to have U-Boot copy and decompress the data into RAM ready for Linux? Yes. Rigth from the beginning of PPCBoot / U-Boot we designed it that U-Boot would do all needed steps to verify, load and uncompress an image. It make no sense to attach the uncompression and loading code to each and every image, and to download it and store it again and again and again. This works really well for example on Power, only ARM is one of the examples where the PTB never bothered to acquaint themself with ideas that went beyond the capabilities of Blob or similar boot loaders. > In theory this should be slightly faster since U-Boot already has the > data in its cache. I think zImage now supports having an FDT inside > but what is the advantage of zImage over a uImage with compressed > portions? There is none. Also, there is no advantage in attaching the DT blob to the Linux image/. This is only of use to braindead boot loaders. Best regards, Wolfgang Denk
> Dear Marek Vasut, > > In message <201111051941.38920.marek.vasut@gmail.com> you wrote: > > this patchset is good and all, but can we not also introduce cmd_zload to > > load zImages? Wolfgang, today's ARM hardware will really benefit from > > that, uImage holds us back a lot these days. Other option is to extend > > cmd_bootm() to load zImages. > > uImage holds us back? And hardware will benefit from that? Well you put out quite a good reason in the other mail for uImage. Now the problem basically is the negotiation between linux and uboot as of who has to wrap the image into what envelope. I don't see this happening actually, which sucks. > > I can't parse that. Could you please elucidate? > > What exactly _is_ the problem of uImage? The exact problem here is it loads the kernel to fixed address. > > Or have you ever considered using a FIT image? That looks good, yes. > > Best regards, > > Wolfgang Denk
> Dear Simon Glass, > > In message <CAPnjgZ0bbhvZ1VPwY=0y9YZJdf+EOBgyKKVPFuAkBqy2z4L3HA@mail.gmail.com> you wrote: > > Just a quick Q. What is the ultimate intent here? Should we be aiming > > to have U-Boot copy and decompress the data into RAM ready for Linux? > > Yes. > > Rigth from the beginning of PPCBoot / U-Boot we designed it that > U-Boot would do all needed steps to verify, load and uncompress an > image. It make no sense to attach the uncompression and loading code > to each and every image, and to download it and store it again and > again and again. This works really well for example on Power, only > ARM is one of the examples where the PTB never bothered to acquaint > themself with ideas that went beyond the capabilities of Blob or > similar boot loaders. Right, there's no negotiation between linux and uboot on this topic. I think this is going on for ages now. > > > In theory this should be slightly faster since U-Boot already has the > > data in its cache. I think zImage now supports having an FDT inside > > but what is the advantage of zImage over a uImage with compressed > > portions? > > There is none. Also, there is no advantage in attaching the DT blob > to the Linux image/. This is only of use to braindead boot loaders. > > Best regards, > > Wolfgang Denk
Dear Marek Vasut, In message <201111052039.34646.marek.vasut@gmail.com> you wrote: > > > Just a quick Q. What is the ultimate intent here? Should we be aiming > > to have U-Boot copy and decompress the data into RAM ready for Linux? > > Nope, not at all. We have a problem with booting linux images which support > multiple different SoCs (because the RAM might be elsewhere for different SoCs). You are wrong. The agreement was to allow for addresses (load address, entry point address) that are relative to the start of system RAM. > That's not the point. We need to load FDT, load zImage, setup the regs and boot > it from where we load the zImage. The uImage envelope contains fixed address to > where the kernel image is loaded, which interferes with kernel's runtime > patching of the kernel base address (zreladdr). You ignore the discussions and proposals that were made. > uImage blocks this because it forces u-boot to copy zImage to fixed address. Nonsense. Best regards, Wolfgang Denk
Dear Marek Vasut, In message <201111052141.27086.marek.vasut@gmail.com> you wrote: > > I'd be more open to adding some kind of a flag to uImage, rather than using > address 0xffffffff. For this is quite fragile and seems a lot like a hack. We don't want to have 0xffffffff. The agreement was to support relative imagaes. See 10/18 Stephen Warren [PATCH v2 REPOST 1/3] [COSMETIC] checkpatch whitespace cleanups http://article.gmane.org/gmane.comp.boot-loaders.u-boot/113093 10/18 Stephen Warren [PATCH v2 REPOST 2/3] image: Implement IH_TYPE_KERNEL_REL http://article.gmane.org/gmane.comp.boot-loaders.u-boot/113079 10/18 Stephen Warren [PATCH v2 REPOST 3/3] tegra2: Enable CONFIG_SYS_RELATIVE_IMAGES http://article.gmane.org/gmane.comp.boot-loaders.u-boot/113080 Best regards, Wolfgang Denk
> Dear Marek Vasut, > > In message <201111052141.27086.marek.vasut@gmail.com> you wrote: > > I'd be more open to adding some kind of a flag to uImage, rather than > > using address 0xffffffff. For this is quite fragile and seems a lot like > > a hack. > > We don't want to have 0xffffffff. > > The agreement was to support relative imagaes. > > See > 10/18 Stephen Warren [PATCH v2 REPOST 1/3] [COSMETIC] checkpatch > whitespace cleanups > http://article.gmane.org/gmane.comp.boot-loaders.u-boot/113093 > 10/18 Stephen Warren [PATCH v2 REPOST 2/3] image: Implement > IH_TYPE_KERNEL_REL > http://article.gmane.org/gmane.comp.boot-loaders.u-boot/113079 > 10/18 Stephen Warren [PATCH v2 REPOST 3/3] tegra2: Enable > CONFIG_SYS_RELATIVE_IMAGES > http://article.gmane.org/gmane.comp.boot-loaders.u-boot/113080 > > > Best regards, > > Wolfgang Denk Ok I see, I definitelly missed those. Thanks
Dear Marek Vasut, In message <201111052306.47844.marek.vasut@gmail.com> you wrote: > > Right, there's no negotiation between linux and uboot on this topic. I think > this is going on for ages now. What kind of "negotiation" do you have in mind when patches to add such Linux make targets get routinely rejected? Best regards, Wolfgang Denk
Dear Marek Vasut, In message <201111052306.23919.marek.vasut@gmail.com> you wrote: > > Well you put out quite a good reason in the other mail for uImage. Now the > problem basically is the negotiation between linux and uboot as of who has to > wrap the image into what envelope. I don't see this happening actually, which > sucks. Look at the PPC code. There we have a plain simple uImage make target in Linux, and everything is fine. In ARM, we only have a target which gives us a pre-wrapped image, i. e. which always includes the kernel's wrapper and decompressor. How can we change this? Patches are not accepted. > > What exactly _is_ the problem of uImage? > > The exact problem here is it loads the kernel to fixed address. This has been addressed, and patches are queued. Best regards, Wolfgang Denk
> Dear Marek Vasut, > > In message <201111052306.23919.marek.vasut@gmail.com> you wrote: > > Well you put out quite a good reason in the other mail for uImage. Now > > the problem basically is the negotiation between linux and uboot as of > > who has to wrap the image into what envelope. I don't see this happening > > actually, which sucks. > > Look at the PPC code. There we have a plain simple uImage make target > in Linux, and everything is fine. In ARM, we only have a target which > gives us a pre-wrapped image, i. e. which always includes the kernel's > wrapper and decompressor. How can we change this? Patches are not > accepted. > > > > What exactly _is_ the problem of uImage? > > > > The exact problem here is it loads the kernel to fixed address. > > This has been addressed, and patches are queued. > I went through the previous discussions, sorry for the noise. > > Best regards, > > Wolfgang Denk
Hi Wolfgang, On Sat, Nov 5, 2011 at 2:53 PM, Wolfgang Denk <wd@denx.de> wrote: > Dear Simon Glass, > > In message <CAPnjgZ0bbhvZ1VPwY=0y9YZJdf+EOBgyKKVPFuAkBqy2z4L3HA@mail.gmail.com> you wrote: >> >> Just a quick Q. What is the ultimate intent here? Should we be aiming >> to have U-Boot copy and decompress the data into RAM ready for Linux? > > Yes. > > Rigth from the beginning of PPCBoot / U-Boot we designed it that > U-Boot would do all needed steps to verify, load and uncompress an > image. It make no sense to attach the uncompression and loading code > to each and every image, and to download it and store it again and > again and again. This works really well for example on Power, only > ARM is one of the examples where the PTB never bothered to acquaint > themself with ideas that went beyond the capabilities of Blob or > similar boot loaders. OK, it makes sense to me. If U-Boot is doing some unpacking it should do it all IMO. It is supposed to be the boot loader, not half a boot loader. Perhaps for other boot loaders the situation is different. > >> In theory this should be slightly faster since U-Boot already has the >> data in its cache. I think zImage now supports having an FDT inside >> but what is the advantage of zImage over a uImage with compressed >> portions? > > There is none. Also, there is no advantage in attaching the DT blob > to the Linux image/. This is only of use to braindead boot loaders. If so can you please point me to it? Searching for "DTB append ARM zImage" provides lots of recent activity. The justification for this series seems to be old boot loaders. Is there a LKML thread about why it should/shouldn't be done in U-Boot specifically - other boot loaders may require the kernel to do it, but for U-Boot there would need to be some sort of reason I think. I am currently using a hybrid solution (U-Boot loads the zImage and DTB, then kernel decompresses zImage) and we have discussed changing this. It may need a new build target in the kernel, but not rocket science. Regards, Simon > > Best regards, > > Wolfgang Denk > > -- > DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de > A stone was placed at a ford in a river with the inscription: > "When this stone is covered it is dangerous to ford here." >
Dear Simon Glass, In message <CAPnjgZ2mKwMyYVyGyYwMm8dNMfNPwgbJA+YTH_G1X-uzbkDfpQ@mail.gmail.com> you wrote: > > Searching for "DTB append ARM zImage" provides lots of recent > activity. The justification for this series seems to be old boot > loaders. ... True. In my understanding, this is only relevant to less capable boot loaders (which not always are old - things like UEFI are not much better in this respect). But U-Boot has full DT support, and we are even on the way to configure U-Boot itself through a DT, so we should at least optionally be able to use these features. > ... Is there a LKML thread about why it should/shouldn't be done > in U-Boot specifically - other boot loaders may require the kernel to > do it, but for U-Boot there would need to be some sort of reason I > think. I cannot see a single reason for such format in U-Boot context. We have been using the device tree for years on PPC, and never had any such need. Dt is actually standard technology now. It's only new for ARM, like all the other stuff as FPU support, cache coherency, PCI, SMP, ... you name it. > I am currently using a hybrid solution (U-Boot loads the zImage and > DTB, then kernel decompresses zImage) and we have discussed changing > this. It may need a new build target in the kernel, but not rocket > science. Patches for this have been submitted (and rejected) several times, years ago. Ditto for other useful things (like being able to pass the kernel a ramdisk / initrd address in NOR flash, so we can avoid copying it into RAM first). Search the ML archives if you are interesteed. Best regards, Wolfgang Denk
On Sat, Nov 05, 2011, Marek Vasut wrote: > this patchset is good and all, but can we not also introduce cmd_zload > to load zImages? Wolfgang, today's ARM hardware will really benefit > from that, uImage holds us back a lot these days. Other option is to > extend cmd_bootm() to load zImages. Other architectures like x86 and sh seem to use zboot, perhaps we should stick to zboot for all architectures rather than introducing bootm -z or bootz? AFAIK the file formats are slightly different per architecture, but the U-Boot cmdline usage would be the same.
Dear Stephen Warren, In message <1320164902-24190-1-git-send-email-swarren@nvidia.com> you wrote: > image_get_ram_disk() and image_get_kernel() perform operations in a > consistent order. Modify image_get_fdt() to do things the same way. > This allows a later change to insert some image header manipulations > into these three functions in a consistent fashion. > > v2: New patch > > Signed-off-by: Stephen Warren <swarren@nvidia.com> > --- > common/image.c | 9 +++++++-- > 1 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/common/image.c b/common/image.c > index 555d9d9..b773505 100644 > --- a/common/image.c > +++ b/common/image.c > @@ -1131,14 +1131,19 @@ static const image_header_t *image_get_fdt(ulong fdt_addr) > { > const image_header_t *fdt_hdr = (const image_header_t *)fdt_addr; > > - image_print_contents(fdt_hdr); > + if (!image_check_magic(fdt_hdr)) { > + fdt_error("fdt header bad magic number\n"); > + return NULL; > + } > > - puts(" Verifying Checksum ... "); > if (!image_check_hcrc(fdt_hdr)) { > fdt_error("fdt header checksum invalid"); > return NULL; > } > > + image_print_contents(fdt_hdr); > + > + puts(" Verifying Checksum ... "); > if (!image_check_dcrc(fdt_hdr)) { > fdt_error("fdt checksum invalid"); > return NULL; The rule in U-Boot when generating output is to print a message before you start an action, and then either print an OK or an error message. The reason for this is debug support: if neither an OK nor an error comes you know that the test somehow crashed. Here this principle is violated as image_check_magic() and image_check_hcrc() will run without being announced. Please move the output so we get a message printed before starting to perform the actual tests. Best regards, Wolfgang Denk
(Resending due to MIME encoding last time. Sorry; I really have to stop using Outlook for this list for some reason) On 11/08/2011 09:06 AM, Wolfgang Denk wrote: > In message <1320164902-24190-1-git-send-email-swarren@nvidia.com> you wrote: >> image_get_ram_disk() and image_get_kernel() perform operations in a >> consistent order. Modify image_get_fdt() to do things the same way. >> This allows a later change to insert some image header manipulations >> into these three functions in a consistent fashion. ... >> @@ -1131,14 +1131,19 @@ static const image_header_t *image_get_fdt(ulong fdt_addr) >> { >> const image_header_t *fdt_hdr = (const image_header_t *)fdt_addr; >> >> - image_print_contents(fdt_hdr); >> + if (!image_check_magic(fdt_hdr)) { >> + fdt_error("fdt header bad magic number\n"); >> + return NULL; >> + } >> >> - puts(" Verifying Checksum ... "); >> if (!image_check_hcrc(fdt_hdr)) { >> fdt_error("fdt header checksum invalid"); >> return NULL; >> } >> >> + image_print_contents(fdt_hdr); >> + >> + puts(" Verifying Checksum ... "); >> if (!image_check_dcrc(fdt_hdr)) { >> fdt_error("fdt checksum invalid"); >> return NULL; > > The rule in U-Boot when generating output is to print a message > before you start an action, and then either print an OK or an error > message. The reason for this is debug support: if neither an OK nor > an error comes you know that the test somehow crashed. > > Here this principle is violated as image_check_magic() and > image_check_hcrc() will run without being announced. > > Please move the output so we get a message printed before starting to > perform the actual tests. The new code is exactly the same as the existing image_get_kernel() and image_get_ramdisk(). Are those wrong? I wouldn't want to fix my patch to conform to some supposed standard when the existing code that's been accepted doesn't conform to that standard, or would I be responsible for fixing up that too? But anyway, I imagine there's no point discussing this patch further, because its sole purpose is to support the uImage load_address=-1 patch that follows, and it's pretty clear that won't be accepted, so please consider this patch series withdrawn too.
Dear Stephen Warren, In message <74CDBE0F657A3D45AFBB94109FB122FF173F9A5415@HQMAIL01.nvidia.com> you wrote: > Wolfgang Denk wrote at Tuesday, November 08, 2011 9:07 AM: > > In message <1320164902-24190-1-git-send-email-swarren@nvidia.com> you wrote: > > > image_get_ram_disk() and image_get_kernel() perform operations in a > > > consistent order. Modify image_get_fdt() to do things the same way. > > > This allows a later change to insert some image header manipulations > > > into these three functions in a consistent fashion. > ... > > > @@ -1131,14 +1131,19 @@ static const image_header_t *image_get_fdt(ulong fdt_addr) > > > { > > > const image_header_t *fdt_hdr = (const image_header_t *)fdt_addr; > > > > > > - image_print_contents(fdt_hdr); > > > + if (!image_check_magic(fdt_hdr)) { > > > + fdt_error("fdt header bad magic number\n"); > > > + return NULL; > > > + } > > > > > > - puts(" Verifying Checksum ... "); > > > if (!image_check_hcrc(fdt_hdr)) { > > > fdt_error("fdt header checksum invalid"); > > > return NULL; > > > } > > > > > > + image_print_contents(fdt_hdr); > > > + > > > + puts(" Verifying Checksum ... "); > > > if (!image_check_dcrc(fdt_hdr)) { > > > fdt_error("fdt checksum invalid"); > > > return NULL; ... > > The rule in U-Boot when generating output is to print a message > > before you start an action, and then either print an OK or an error > > message. The reason for this is debug support: if neither an OK nor > > an error comes you know that the test somehow crashed. ... > The new code is exactly the same as the existing image_get_kernel() and > image_get_ramdisk(). Are those wrong? I wouldn't want to fix my patch to > conform to some supposed standard when the existing code that's been > accepted doesn't conform to that standard, or would I be responsible for > fixing up that too? The new code is different from what was there before. Now we have: 1134 image_print_contents(fdt_hdr); 1135 1136 puts(" Verifying Checksum ... "); 1137 if (!image_check_hcrc(fdt_hdr)) { 1138 fdt_error("fdt header checksum invalid"); 1139 return NULL; 1140 } 1141 1142 if (!image_check_dcrc(fdt_hdr)) { 1143 fdt_error("fdt checksum invalid"); 1144 return NULL; i.e. image_print_contents(fdt_hdr); puts(" Verifying Checksum ... "); image_check_hcrc(fdt_hdr); image_check_dcrc(fdt_hdr) After your patch we have: image_check_magic(); image_check_hcrc() image_print_contents((); puts(" Verifying Checksum ... "); image_check_dcrc(); So before, we have the "Verifying Checksum" before running any of the image_check_*() functions, while with your patch we run two of them before that. Best regards, Wolfgang Denk
diff --git a/common/image.c b/common/image.c index 555d9d9..b773505 100644 --- a/common/image.c +++ b/common/image.c @@ -1131,14 +1131,19 @@ static const image_header_t *image_get_fdt(ulong fdt_addr) { const image_header_t *fdt_hdr = (const image_header_t *)fdt_addr; - image_print_contents(fdt_hdr); + if (!image_check_magic(fdt_hdr)) { + fdt_error("fdt header bad magic number\n"); + return NULL; + } - puts(" Verifying Checksum ... "); if (!image_check_hcrc(fdt_hdr)) { fdt_error("fdt header checksum invalid"); return NULL; } + image_print_contents(fdt_hdr); + + puts(" Verifying Checksum ... "); if (!image_check_dcrc(fdt_hdr)) { fdt_error("fdt checksum invalid"); return NULL;
image_get_ram_disk() and image_get_kernel() perform operations in a consistent order. Modify image_get_fdt() to do things the same way. This allows a later change to insert some image header manipulations into these three functions in a consistent fashion. v2: New patch Signed-off-by: Stephen Warren <swarren@nvidia.com> --- common/image.c | 9 +++++++-- 1 files changed, 7 insertions(+), 2 deletions(-)