[U-Boot] [PATCH v2 4/5] arm: socfpga: Add intermediate driver between flash and FPGA manager

[Chee, Tien Fong]

On Sab, 2017-08-12 at 18:49 +0200, Marek Vasut wrote:
> On 08/12/2017 10:03 AM, Chee, Tien Fong wrote:
> [...]
> > 
> > > 
> > > > 
> > > > 1: It having ability to the right memory(OCRAM or SDRAM) to
> > > > achieve
> > > > the
> > > > best FPGA programing performance.
> > > Did you find significant throughput difference ?
> > > 
> > 80% performance improvement with SDRAM.
> This looks more like caches are not enabled ... sort of problem ?
> 
It is because SDRAM size large enough to store whole rbf file and
reduce significantly of the repetitive block transfer. It requires one
time block transfer from SDRAM to FPGA compare to OCRAM which require
more than that.
> > 
> > > 
> > > > 
> > > > 2: It can determine the right size buffer for the fpga rbf
> > > > without
> > > > info
> > > > of buffer size defined by user.
> > > You mean like $filesize variable in the command prompt ?
> > > 
> > Yeah. No filesize is required.
> You can use $filesize instead of reimplementing this functionality
> yourself though ?
> 
I'am sorry to have confused you. What i trying to say is user no longer
require to specify the file size and location when calling fpga loadfs.
So, this would keep the thing simple. The driver in cff.c would handle
all these troublesome things and deciding the best route for good
performance.
> > 
> > > 
> > > > 
> > > > 3: It has ability to know what kind of fpga rbf type, and
> > > > security
> > > > type, such as peripheral, core, combined rbf, encryption and
> > > > unencryption based on any fpga file user pass in .
> > > Is this information used for anything ? I was under the
> > > impression
> > > that
> > > the user just needs to load in the correct RBF file into the
> > > FPGA.
> > > 
> > Yeah, the driver would decode the RBF image to know what type of
> > RBF
> > user loading, and applying correct method(buffer allocation, which
> > memory to use and configuration on FPGA manager) to program FPGA.
> If the code needs to extract some information from the RBF to
> correctly
> configure something in the FPGA manager, then that's where this
> should
> go then.
> 
Those functions for extracting info from RBF and configuring the FPGA
are actually come from fpga driver which i have submited as in previous
patchset. The actualy implementation is in cff.c, as i try to explain
cff.c drivers are mainly to handle all activities of loading rbf from
flashes to program FPGA.
> > 
> > > 
> > > > 
> > > > 4: It supports the checksum.
> > > What checksum ? Can we have a generic hook into the FPGA
> > > framework ?
> > > 
> > This checksum is to ensure integrity of RBF file after loading from
> > flash into SDRAM. This can help to prevent possibility system
> > instability caused by programming corrupt rbf into FPGA. So, this
> > should be implemented in cff.c .
> Or the FPGA manager driver .
> 
This integrity checking is implemented during RBF loading from flash to
SDRAM before calling the functions from fpga driver to program FPGA. So
cff.c which is designed for handling these operation and it should
be appropriate place for checksum.
> > 
> > > 
> > > > 
> > > > 5: support raw flash without fs.
> > > This should go into common code.
> > > 
> > raw flash is part of common codes in cff.c because it is part of
> > mechanism like fs to determine how loading rbf from flash and
> > program
> > into fpga.
> By common code I mean the stuff around FPGA LOADFS , so other FPGAs
> can
> also benefit from that.
> 
But the raw flash to FPGA implementation is designed fully tied to
common codes in cff.c. FPGA loadfs would be acted like a wrapper to it,
so i am not sure it can be shared by other FPGA.
> > 
> > > 
> > > > 
> > > > 6: support the file name defined in DTS and U-boot environment
> > > > variable.
> > > I think you should extend the FPGA LOADFS here instead.
> > > 
> > The peripheral rbf filename and DTS are generated from our bsp
> > tool.
> > But user can run fpga loadfs to reconfigure FPGA in U-boot console
> > after i have supported it.
> And why don't you rather apply some FPGA LOADFS if this property is
> detected in the DT instead of reimplementing it ?
> 
We need to program the FPGA IO ring buffer from very early phase in SPL
before SDRAM can be initialized. So we need the peripheral rbf file
name from DTS, which is defined by user in our BSP tool. I don't think
FPGA loadfs would be available in SPL by that time. So, i think FPGA
loadfs is suitable for reprogramming FPGA in U-boot console, and
filename is passed as parameter to fpga loadfs command.
> > 
> > > 
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > >  Also, the ifdeffery is awful and the explicit
> > > > > depedence on VFAT when loading from FS is real bad.
> > > > > 
> > > > It is because a lot functions is common to sdmmc, nand and qspi
> > > > in
> > > > different fs such as vfat, ubi and raw. It is unavoidable to
> > > > have
> > > > some
> > > > ifdeffery if we want to keep the function common to all flashes
> > > > and
> > > > fs. 
> > > Can the FPGA LOADFS be extended generically ?
> > > 
> > Yeah, FPGA loadfs is considered when design cff.c. But, i plan to
> > to
> > support FPGA loadfs after having complete boot to U-boot console.
> Does that mean the Arria10 is still not even capable of booting into
> U-Boot console ?
> 
> cff.c is unlikely to hit mainline unless there's compelling argument
> that it is needed . Thus far, I see most of the functionality should
> be
> added elsewhere or is already there (fpga loadfs).
> 
Please consider it, as i think that implementation of loading rbf from
any flashes to memory before programming PFGA should not be part of
FPGA driver. FPGA driver should contains only functionality of init
fpga and programming FPGA. cff.c is designed so the fpga loadfs can be 
acted wrapper on it.