[U-Boot] [PATCH v7 2/7] ARM: socfpga: Add default FPGA bitstream fitImage for Arria10 SoCDK

[Marek Vasut]

On 2/1/19 5:50 PM, Chee, Tien Fong wrote:
> On Fri, 2019-02-01 at 09:29 +0100, Marek Vasut wrote:
>> On 2/1/19 4:59 AM, Chee, Tien Fong wrote:
>>>
>>> On Thu, 2019-01-31 at 15:54 +0100, Marek Vasut wrote:
>>>>
>>>> On 1/31/19 3:51 PM, tien.fong.chee at intel.com wrote:
>>>>>
>>>>>
>>>>> From: Tien Fong Chee <tien.fong.chee at intel.com>
>>>>>
>>>>> Add default fitImage file bundling FPGA bitstreams for Arria10.
>>>>>
>>>>> Signed-off-by: Tien Fong Chee <tien.fong.chee at intel.com>
>>>>> ---
>>>>>  board/altera/arria10-socdk/fit_spl_fpga.its | 31
>>>>> +++++++++++++++++++++++++++++
>>>>>  1 file changed, 31 insertions(+)
>>>>>  create mode 100644 board/altera/arria10-socdk/fit_spl_fpga.its
>>>>>
>>>>> diff --git a/board/altera/arria10-socdk/fit_spl_fpga.its
>>>>> b/board/altera/arria10-socdk/fit_spl_fpga.its
>>>>> new file mode 100644
>>>>> index 0000000..46b125c
>>>>> --- /dev/null
>>>>> +++ b/board/altera/arria10-socdk/fit_spl_fpga.its
>>>>> @@ -0,0 +1,31 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>> + /*
>>>>> + * Copyright (C) 2019 Intel Corporation <www.intel.com>
>>>>> + *
>>>>> + */
>>>>> +
>>>>> +/dts-v1/;
>>>>> +
>>>>> +/ {
>>>>> +	description = "FIT image with FPGA bistream";
>>>>> +	#address-cells = <1>;
>>>>> +
>>>>> +	images {
>>>>> +		fpga-2 {
>>>> Why is fpga-2 before fpga-1 ?
>>> 1. The main purpose is for solving the performance issue as i
>>> described
>>> in cover letter. We can decide the absolute data position for core
>>> image, and ensure it is in allignment.
>> Where does the alignment problem happen exactly ?
> The allignment problem happen in get_contents function, line 373, at
> fs/fat/fat.c .

But then you're trying to work around a memcpy performance pentalty in
VFAT code by frobbing with file position within a fitImage ? This can
not work, since the file alignment within fitImage is not guaranteed.

> This happens only when reading offset from a file,
> that's why absolute position is very important to set the right offset
> for the core image. The performance penalty can be significantly
> incurred with large size core image.
> 
> 		filesize -= actsize;
> 		actsize -= pos;
> 		memcpy(buffer, tmp_buffer + pos, actsize);
> 		free(tmp_buffer);
> 		*gotsize += actsize;
> 		if (!filesize)
> 			return 0;
> 		buffer += actsize; <= buffer sometimes is altered to  
>                                       unaligned
> 
> This function is basically finding the cluster where the pos resides
> in, adjusting the pos, actsize and file size accordingly when the base
> being changed from beginning of the file to the beginning of the
> cluster where the pos resides in.
> 
> Then copying the actsize size of content from pos to the end of the
> cluster to the buffer above, and updating buffer to the next write. The
> updated buffer can be unaligned especially the pos is not being set
> properly, hence we need the absolute position to fix that.
> 
> When the unaligned buffer is passed as argument to the get_cluster
> function, you would see the print out of "FAT: Misaligned buffer
> address" at line 264 in that function. A very slow disk_read would be
> implemented to transfer the sector by sector content to the unaligned
> buffer.

Can this be fixed then ?

>>
>> Anyway, you cannot rely on this, the alignment within the fitImage
>> may
>> be changed just by using different strings in the ITS file.
> No change for absolute position, it is always same offset based on the
> beginning of a FIT.

Try adding a few properties here and there and/or changing the length of
some of the strings, you'll see the file offset changes.

>>
>>>
>>> 2. Users know where is the data position for core, so easy for them
>>> to
>>> program themself with series commands on U-Boot console.
>> You should use imxtract to pull out the file from fitImage and then
>> program it. imxtract can refer to image name, so there's no need to
>> access raw data within the fitImage by offset.
> Yes, that's one of the most effective way. Another is using fatload
> with offset.

No, it is not, because you do not know the offset. imxtract parses the
fitImage structure and computes the offset for you.

> But the item 1 is the main reason why absolute position is important
> for large core image.
> 
> Eventually positive solution, it is good to improve the performance
> handling for both get_content and get_cluster functions.
>>
>>>
>>>>
>>>>>
>>>>> +			description = "FPGA core bitstream";
>>>>> +			data =
>>>>> /incbin/("../../../ghrd_10as066n2.core.rbf");
>>>>> +			type = "fpga";
>>>>> +			arch = "arm";
>>>>> +			compression = "none";
>>>>> +			load = <0x400>;
>>>>> +		};
>>>>> +
>>>>> +		fpga-1 {
>>>>> +			description = "FPGA peripheral
>>>>> bitstream";
>>>>> +			data =
>>>>> /incbin/("../../../ghrd_10as066n2.periph.rbf");
>>>>> +			type = "fpga";
>>>>> +			arch = "arm";
>>>>> +			compression = "none";
>>>>> +		};
>>>>> +	};
>>>>> +};
>>>>>


-- 
Best regards,
Marek Vasut