Patchwork [U-Boot,v2,1/3] image: Make image_get_fdt work like image_get_{ram_disk, kernel}

login
register
mail settings
Submitter Stephen Warren
Date Nov. 1, 2011, 4:28 p.m.
Message ID <1320164902-24190-1-git-send-email-swarren@nvidia.com>
Download mbox | patch
Permalink /patch/123094/
State Changes Requested
Headers show

Comments

Stephen Warren - Nov. 1, 2011, 4:28 p.m.
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(-)
Marek Vasut - Nov. 5, 2011, 6:41 p.m.
> 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
Simon Glass - Nov. 5, 2011, 7:21 p.m.
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
Marek Vasut - Nov. 5, 2011, 7:39 p.m.
> 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
Simon Glass - Nov. 5, 2011, 8:38 p.m.
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
>
Marek Vasut - Nov. 5, 2011, 8:41 p.m.
> 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
Simon Glass - Nov. 5, 2011, 8:49 p.m.
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
>
Wolfgang Denk - Nov. 5, 2011, 9:40 p.m.
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
Wolfgang Denk - Nov. 5, 2011, 9:53 p.m.
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
Marek Vasut - Nov. 5, 2011, 10:06 p.m.
> 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
Marek Vasut - Nov. 5, 2011, 10:06 p.m.
> 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
Wolfgang Denk - Nov. 5, 2011, 10:06 p.m.
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
Wolfgang Denk - Nov. 5, 2011, 10:15 p.m.
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
Marek Vasut - Nov. 5, 2011, 10:18 p.m.
> 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
Wolfgang Denk - Nov. 5, 2011, 10:22 p.m.
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
Wolfgang Denk - Nov. 5, 2011, 10:24 p.m.
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
Marek Vasut - Nov. 5, 2011, 10:38 p.m.
> 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
Simon Glass - Nov. 6, 2011, 4:52 a.m.
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."
>
Wolfgang Denk - Nov. 6, 2011, 8:57 a.m.
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
Loïc Minier - Nov. 8, 2011, 2:24 p.m.
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.
Wolfgang Denk - Nov. 8, 2011, 4:06 p.m.
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
Stephen Warren - Nov. 8, 2011, 6:15 p.m.
(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.
Wolfgang Denk - Nov. 8, 2011, 7:33 p.m.
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

Patch

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;