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

[Chee, Tien Fong]

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 . 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.
> 
> 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.
> 
> > 
> > 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.
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";
> > > > +		};
> > > > +	};
> > > > +};
> > > > 
>