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

[Michal Simek]

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?

Thanks,
Michal