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

[Marek Vasut]

On 08/14/2017 05:58 AM, Chee, Tien Fong wrote:
> 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.

OK, do that as a subsequent optimization.

>>>
>>>>
>>>>>
>>>>> 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.

OK, separate patch

>>>
>>>>
>>>>>
>>>>> 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.

Such functionality should be generic and split between fpga loadfs and
the driver.

>>>
>>>>
>>>>>
>>>>> 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.

Add a checksum hook into the FPGA framework ?

>>>
>>>>
>>>>>
>>>>> 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.

I do not want any ad-hoc wrapper, implement it properly be extending
fpga loadfs or the fpga framework.

>>>
>>>>
>>>>>
>>>>> 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.

Then just call the required bits from fpga loadfs during SPL . Extract
them into some fpga-loadfs-common and make both fpga loadfs and your SPL
code call those functions.

>>>
>>>>
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>  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.

That's correct.

> 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.

And that is the part I have real problem with. Extend the common code
and then use it to load whatever stuff into the FPGA instead of making
common code a wrapper around ad-hoc stuff.

-- 
Best regards,
Marek Vasut