| Message ID | 20180730105319.79424-2-sgoldschmidt@de.pepperl-fuchs.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [U-Boot,1/2] doc: FIT image: clarify usage of "compression" property | expand |
Dear Simon, In message <20180730105319.79424-2-sgoldschmidt@de.pepperl-fuchs.com> you wrote: > To prepare supporting compression for all image types, change > compression to "none" for ramdisks in all examples. What makes you think this is a correct thing to do? There are different approaches to handle things. For example, traditionally on Power Architecture we would use a raw kernel binray, compress this (for example with gzip) before wrapping it with mkimage into an (uImage or FIT) U-Boot image, and then let U-Boot uncompress the kernel image into rum and start it. On ARM the kernel comes traditionally with it's own wrapper that does unompressions and such. Same for ramdisk handling. On some systems it may make sense to have U-Boot handle the uncompressing, so compression = "gzip"; may be fully intentional. Best regards, Wolfgang Denk
Dear Wolfgang, On 30.07.2018 13:45, Wolfgang Denk wrote: > Dear Simon, > > In message <20180730105319.79424-2-sgoldschmidt@de.pepperl-fuchs.com> you wrote: >> To prepare supporting compression for all image types, change >> compression to "none" for ramdisks in all examples. > > What makes you think this is a correct thing to do? > > There are different approaches to handle things. For example, > traditionally on Power Architecture we would use a raw kernel > binray, compress this (for example with gzip) before wrapping it > with mkimage into an (uImage or FIT) U-Boot image, and then let > U-Boot uncompress the kernel image into rum and start it. On ARM > the kernel comes traditionally with it's own wrapper that does > unompressions and such. > > Same for ramdisk handling. On some systems it may make sense to > have U-Boot handle the uncompressing, so compression = "gzip"; > may be fully intentional. That's the whole point and in the thread I mentioned (https://lists.denx.de/pipermail/u-boot/2018-July/336435.html) it has been discussed that this is the future goal. However, uncompression currently is only implemented for kernels, not for other sub-images. This patch aims at updating the docs and the current .its examples to what they do now. Best regards, Simon
Dear Simon, In message <6bce9049-a812-d8e4-cd2a-4b86dccae67e@de.pepperl-fuchs.com> you wrote: > > However, uncompression currently is only implemented for kernels, not > for other sub-images. This patch aims at updating the docs and the > current .its examples to what they do now. Yes, but the current state should IMO not been taken as reference. I don't have time ATM to achtually check with older versions, but I'm pretty sure that that was handled correctly in the original FIT implementation, and the examples made sense at the time they were being written. I suspect that later changes broke the code, so only the code needs to be fixed. In general for the image handling there should be no distinction at all whether this is a kernel or ramdisk or FPGA or whatever image - compression is a feature independent of the image type, and that's how the code should handle it. Best regards, Wolfgang Denk
On 30.07.2018 14:36, Wolfgang Denk wrote: > Dear Simon, > > In message <6bce9049-a812-d8e4-cd2a-4b86dccae67e@de.pepperl-fuchs.com> you wrote: >> >> However, uncompression currently is only implemented for kernels, not >> for other sub-images. This patch aims at updating the docs and the >> current .its examples to what they do now. > > Yes, but the current state should IMO not been taken as reference. > I don't have time ATM to achtually check with older versions, but > I'm pretty sure that that was handled correctly in the original FIT > implementation, and the examples made sense at the time they were > being written. I suspect that later changes broke the code, so only > the code needs to be fixed. > > In general for the image handling there should be no distinction at > all whether this is a kernel or ramdisk or FPGA or whatever image - > compression is a feature independent of the image type, and that's > how the code should handle it. OK, so at least we all seem to have a common sense of how it should be :-) I'd still argue that the standard example in 'multi.its' should have compression = "none" for the ramdisks. Best regards, Simon
Dear Simon, In message <0787afb7-2da1-4eaa-437e-6f7c7582809c@de.pepperl-fuchs.com> you wrote: > > I'd still argue that the standard example in 'multi.its' should have > compression = "none" for the ramdisks. OK, this is your position then. I can only explain where we are coming from: in the early days of U-Boot (well, PPCBoot by then) resources were much tighter than today - look at the examples in the README: a v2.4.4 Linux kernel image would be around 780 kB uncompressed or 330 kB compressed. At this time it was considered a waste of resources to have the gzip uncompression code in the boot loader and again duplicated in the Linux kernel - so it was omitted there and U-Boot would do all uncompression, also of the ramdisks. I agree this is not standard any more, but it is still a valid use case, I think. I know, I'm an old man ;-) Best regards, Wolfgang Denk
On 30.07.2018 15:25, Wolfgang Denk wrote: > Dear Simon, > > In message <0787afb7-2da1-4eaa-437e-6f7c7582809c@de.pepperl-fuchs.com> you wrote: >> >> I'd still argue that the standard example in 'multi.its' should have >> compression = "none" for the ramdisks. > > OK, this is your position then. I can only explain where we are > coming from: in the early days of U-Boot (well, PPCBoot by then) > resources were much tighter than today - look at the examples in the > README: a v2.4.4 Linux kernel image would be around 780 kB > uncompressed or 330 kB compressed. At this time it was considered a > waste of resources to have the gzip uncompression code in the boot > loader and again duplicated in the Linux kernel - so it was omitted > there and U-Boot would do all uncompression, also of the ramdisks. Well, my point was that I couldn't see U-Boot ever handling this. But this discussion got a bit confused, split into two threads... > I agree this is not standard any more, but it is still a valid use > case, I think. I know, I'm an old man ;-) It *is* a valid use case. But for current kernels (if compression is enabled), it might be faster to let the kernel unzip the content directly to the memory location where it should reside in the end. However, I don't have a strong opinion on these examples. I'll let the maintainers decide :-) Best regards, Simon
Dear Simon, In message <97a1d1e5-4415-4db4-9c77-72023f37bb4f@de.pepperl-fuchs.com> you wrote: > > It *is* a valid use case. But for current kernels (if compression is > enabled), it might be faster to let the kernel unzip the content > directly to the memory location where it should reside in the end. This was true a long, long time ago on ARM (and this is where most of the unclean implmentations originate), when U-Boot did not enable caches. With this fixed, it is almost always wrong, because we now have an unnecessary memory copy: U-Boot loads the compressed kernel image to one location in RAM, which then runs and uncompresses the code to another address. Is it not obvious that it would be more efficient if U-Boot did the uncompressing directly? Best regards, Wolfgang Denk
On Mon, Jul 30, 2018 at 12:53:19PM +0200, Simon Goldschmidt wrote: > To prepare supporting compression for all image types, change > compression to "none" for ramdisks in all examples. > > Signed-off-by: Simon Goldschmidt <sgoldschmidt@de.pepperl-fuchs.com> > --- > > arch/arm/cpu/armv8/fsl-layerscape/doc/README.falcon | 2 +- > doc/README.bcm7xxx | 2 +- > doc/uImage.FIT/multi.its | 4 ++-- > 3 files changed, 4 insertions(+), 4 deletions(-) Sorry for the delay. I think this patch as-is, is wrong. Please split it into a patch per file that CCs the appropriate custodian(s) for them to ack/nak and pick up. It's also appropriate to do a patch now that corrects the behavior of uncompressing ramdisks that can be uncompressed automatically (and no need for a CONFIG option). Thanks!
On Sat, Aug 18, 2018 at 1:14 AM Tom Rini <trini@konsulko.com> wrote: > > On Mon, Jul 30, 2018 at 12:53:19PM +0200, Simon Goldschmidt wrote: > > > To prepare supporting compression for all image types, change > > compression to "none" for ramdisks in all examples. > > > > Signed-off-by: Simon Goldschmidt <sgoldschmidt@de.pepperl-fuchs.com> > > --- > > > > arch/arm/cpu/armv8/fsl-layerscape/doc/README.falcon | 2 +- > > doc/README.bcm7xxx | 2 +- > > doc/uImage.FIT/multi.its | 4 ++-- > > 3 files changed, 4 insertions(+), 4 deletions(-) > > Sorry for the delay. I think this patch as-is, is wrong. Please split > it into a patch per file that CCs the appropriate custodian(s) for them > to ack/nak and pick up. It's also appropriate to do a patch now that > corrects the behavior of uncompressing ramdisks that can be uncompressed > automatically (and no need for a CONFIG option). Thanks! OK, I'll rework it. But I won't have time for that until in two weeks or even later, so I won't be able to provide something for 2018.09. Simon
diff --git a/arch/arm/cpu/armv8/fsl-layerscape/doc/README.falcon b/arch/arm/cpu/armv8/fsl-layerscape/doc/README.falcon index 7dae9f03c3..b3c6693a42 100644 --- a/arch/arm/cpu/armv8/fsl-layerscape/doc/README.falcon +++ b/arch/arm/cpu/armv8/fsl-layerscape/doc/README.falcon @@ -110,7 +110,7 @@ Example: type = "ramdisk"; arch = "arm64"; os = "linux"; - compression = "gzip"; + compression = "none"; load = <0xa0000000>; }; }; diff --git a/doc/README.bcm7xxx b/doc/README.bcm7xxx index 9b5eae4741..1de2f63181 100644 --- a/doc/README.bcm7xxx +++ b/doc/README.bcm7xxx @@ -91,7 +91,7 @@ image.its: type = "ramdisk"; arch = "arm"; os = "linux"; - compression = "gzip"; + compression = "none"; /* * Set the environment variable initrd_high to * 0xffffffff, and set "load" and "entry" here diff --git a/doc/uImage.FIT/multi.its b/doc/uImage.FIT/multi.its index 26c8dad6a2..814d1f73b5 100644 --- a/doc/uImage.FIT/multi.its +++ b/doc/uImage.FIT/multi.its @@ -60,7 +60,7 @@ type = "ramdisk"; arch = "ppc"; os = "linux"; - compression = "gzip"; + compression = "none"; load = <00000000>; entry = <00000000>; hash-1 { @@ -74,7 +74,7 @@ type = "ramdisk"; arch = "ppc"; os = "linux"; - compression = "gzip"; + compression = "none"; load = <00000000>; entry = <00000000>; hash-1 {
To prepare supporting compression for all image types, change compression to "none" for ramdisks in all examples. Signed-off-by: Simon Goldschmidt <sgoldschmidt@de.pepperl-fuchs.com> --- arch/arm/cpu/armv8/fsl-layerscape/doc/README.falcon | 2 +- doc/README.bcm7xxx | 2 +- doc/uImage.FIT/multi.its | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-)