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

[Chee, Tien Fong <tien.fong.chee@intel.com>]

On Mon, 2019-02-11 at 12:06 +0100, Marek Vasut wrote:
> On 2/11/19 7:23 AM, Chee, Tien Fong wrote:
> > 
> > On Tue, 2019-02-05 at 09:51 +0100, Marek Vasut wrote:
> > > 
> > > 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@intel.com>
> > > > > > > > 
> > > > > > > > Add default fitImage file bundling FPGA bitstreams for
> > > > > > > > Arria10.
> > > > > > > > 
> > > > > > > > Signed-off-by: Tien Fong Chee <tien.fong.chee@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
> > Yes, setting the absolute data position for the large core rbf file
> > in
> > fitImage.
> > 
> > so, when generating the fitImage through mkimage, you need to set
> > the
> > absolute position as argument to -p. Absolute data position is
> > always
> > fixed offset based on fitImage base.
> This can not work, consider the different filesystems the fitImage
> can
> be stored on. It's not just VFAT with one cluster size, it can be any
> configuration of VFAT U-Boot supports, or any filesystem U-Boot
> supports. And then the performance penalty could be back.
Yes, i agree with you.

This is temporary workaround i can think of until the issue is getting
solved, but i will keep trying.

If there is a solution, i will submit in separate patch set.
> 
> The proper fix is to optimize what VFAT does, if that is a problem.
> Maybe the block cache can help here ?
I tried that before, but it didn't help in this use case. The block
cache only help in condition which repetitive reading at the
same/adjacent locations.(no cache miss)
> 
> > 
> > > 
> > > > 
> > > > 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 ?
> > I have tried few ideas, no one of them work.
> > 
> > Could you help to take a look?
> I am tremendously overloaded, sorry. Try taking a look at the block
> cache , drivers/block/blkcache.c , maybe this can help ?
Replied in above.
> 
> > 
> > > 
> > > 
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > 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.
> > Absolute data position is always fixed offset from base of fitImage
> > regardless how many properties are added or changing. But, there is
> > potential the overlapping would be happended if data position is
> > too
> > close with 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.
> > > No, it is not, because you do not know the offset. imxtract
> > > parses
> > > the
> > > fitImage structure and computes the offset for you.
> > You know the offset, this is absolute offset from the base of
> > fitImage,
> > you set it as argument to -p when running mkimage such as "-p 400"
> > 
> > tools/mkimage -E -p 400 -f board/altera/arria10-
> > socdk/fit_spl_fpga.its
> > fit_spl_fpga.itb
> You're just working around the alignment problem for one specific
> configuration of the VFAT, this does not scale.
Replied in above.
>