[U-Boot] [PATCH] spl: fit: Enable GZIP compression also for no kernel partitions

[Michal Simek]

On 30.7.2018 13:28, Simon Goldschmidt wrote:
> 
> 
> On 30.07.2018 10:54, Michal Simek wrote:
>> On 26.7.2018 19:16, Tom Rini wrote:
>>> On Thu, Jul 26, 2018 at 04:23:14PM +0000, York Sun wrote:
>>>> On 07/25/2018 11:52 PM, Michal Simek wrote:
>>>>> On 25.7.2018 23:18, York Sun wrote:
>>>>>> On 07/24/2018 10:58 PM, Michal Simek wrote:
>>>>>>> On 24.7.2018 18:26, York Sun wrote:
>>>>>>>> On 07/24/2018 06:07 AM, Michal Simek wrote:
>>>>>>>>> There is no reason to limit gzip usage only for OS_BOOT and
>>>>>>>>> kernel image
>>>>>>>>> type.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Michal Simek <michal.simek at xilinx.com>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>>   common/spl/spl_fit.c | 5 +----
>>>>>>>>>   1 file changed, 1 insertion(+), 4 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
>>>>>>>>> index 9eabb1c1058b..dbf5ac33a845 100644
>>>>>>>>> --- a/common/spl/spl_fit.c
>>>>>>>>> +++ b/common/spl/spl_fit.c
>>>>>>>>> @@ -257,10 +257,7 @@ static int spl_load_fit_image(struct
>>>>>>>>> spl_load_info *info, ulong sector,
>>>>>>>>>       board_fit_image_post_process(&src, &length);
>>>>>>>>>   #endif
>>>>>>>>>   -    if (IS_ENABLED(CONFIG_SPL_OS_BOOT)    &&
>>>>>>>>> -        IS_ENABLED(CONFIG_SPL_GZIP)        &&
>>>>>>>>> -        image_comp == IH_COMP_GZIP        &&
>>>>>>>>> -        type == IH_TYPE_KERNEL) {
>>>>>>>>> +    if (IS_ENABLED(CONFIG_SPL_GZIP) && image_comp ==
>>>>>>>>> IH_COMP_GZIP) {
>>>>>>>>>           size = length;
>>>>>>>>>           if (gunzip((void *)load_addr, CONFIG_SYS_BOOTM_LEN,
>>>>>>>>>                  src, &size)) {
>>>>>>>>>
>>>>>>>>
>>>>>>>> This will uncompress ramdisk unnecessarily.
>>>>>>>
>>>>>>> Can you please share your its fragment? Also is there any other
>>>>>>> image
>>>>>>> which should be exclude?
>>>>>>
>>>>>> I used it for falcon boot. I guess the executable image should
>>>>>> have "entry". In
>>>>>> my setup, only kernel image has "entry". Here is my its file.
>>>>>>
>>>>>> /dts-v1/;
>>>>>>
>>>>>> / {
>>>>>>     description = "Image file for the LS1046A Linux Kernel";
>>>>>>     #address-cells = <1>;
>>>>>>
>>>>>>     images {
>>>>>>         kernel at 1 {
>>>>>>             description = "ARM64 Linux kernel";
>>>>>>             data = /incbin/("./arch/arm64/boot/Image.gz");
>>>>>>             type = "kernel";
>>>>>>             arch = "arm64";
>>>>>>             os = "linux";
>>>>>>             compression = "gzip";
>>>>>>             load = <0x80080000>;
>>>>>>             entry = <0x80080000>;
>>>>>>         };
>>>>>>         fdt at 1 {
>>>>>>             description = "Flattened Device Tree blob";
>>>>>>             data = /incbin/("./fsl-ls1046ardb.dtb");
>>>>>>             type = "flat_dt";
>>>>>>             arch = "arm64";
>>>>>>             compression = "none";
>>>>>>             load = <0x90000000>;
>>>>>>         };
>>>>>>         ramdisk at 1 {
>>>>>>             description = "Buildroot initramfs";
>>>>>>                          data = /incbin/("./rootfs.cpio.gz");
>>>>>>             type = "ramdisk";
>>>>>>             arch = "arm64";
>>>>>>             os = "linux";
>>>>>>             compression = "gzip";
>>>>>
>>>>> I have tested full u-boot and there is also no uncompression for
>>>>> ramdisk
>>>>> when you put gzip compress there.
>>>>> I have even tried gzip compression for fdt with expectation that
>>>>> u-boot
>>>>> will uncompress it.
>>>>>
>>>>> Based on doc/uImage.FIT/source_file_format.txt:
>>>>> 165  - compression : Compression used by included data. Supported
>>>>> compressions
>>>>> 166     are "gzip" and "bzip2". If no compression is used compression
>>>>> property
>>>>> 167     should be set to "none".
>>>>>
>>>>>
>>>>> Based on me this means that data inside fit are compressed and you are
>>>>> asking u-boot in boot to uncompress it. If you are not asking for that
>>>>> you should put none there.
>>>>> But it doesn't look like this is supported at all for fdt/ramdisk
>>>>> and it
>>>>> is only handled for OS.
>>>>> I see here two cases.
>>>>> 1. I want u-boot to uncompress my data in fit image (whatever it is)
>>>>> before passing control to OS that's why I putting there compression
>>>>> method.
>>>>> 2. I want OS to uncompress data but I want pass this data unchanged
>>>>> from
>>>>> u-boot to OS that's why I am putting compression method at "none"
>>>>>
>>>>> I am expecting when you put "none" there than you will boot in falcon
>>>>> mode without any issue.
>>>>
>>>> That will work. I can put "none" for the images I don't want U-Boot to
>>>> uncompress.
>>>
>>> Please also update the document to be clear that "none" for "don't touch
>>> my compressed data!" is expected.  The existing language isn't clear but
>>> I agree it makes sense (and follows the long standing practice of "-C
>>> none" on compressed ramdisks for legacy style images).
>>>
>>> And per Simon Goldschmidt's suggestion, after we've made the docs
>>> clearer, making it so that U-Boot does decompress things would be good,
>>> but we may also need to make that behavior configurable as I can see
>>> people having put the correct compression in and not wanting it to be
>>> uncompressed as we have examples of both none and gzip for ramdisks
>>> today.  It's also possible (and if someone wants to dig back /
>>> experiment and confirm) that long ago it did auto-decompress the data
>>> and now doesn't, it would be a bugfix and no we don't need to make it
>>> configurable.
>>
>> Simon Goldschmidt: Can you please update that doc?
> 
> Done here:
> https://lists.denx.de/pipermail/u-boot/2018-July/336438.html

Thanks.

> 
> Are you planning to implement fpga uncompression in U-Boot proper? I
> don't know when I would find the time to do so...

I would like to resolve spl fpga fit image loading first and fpga
mkimage is next one to check. Also we have fpga image loading as the
part of bootm command. All of these should be checked.

Thanks,
Michal