[U-Boot] [PATCH] spl: fpga: Implement fpga bistream loading with fpga_load

[Michal Simek]

On 19.7.2018 10:35, Marek Vasut wrote:
> On 07/19/2018 08:44 AM, Michal Simek wrote:
>> On 19.7.2018 08:36, Luis Araneda wrote:
>>> Hi,
>>>
>>> On Thu, Jul 19, 2018 at 1:58 AM Michal Simek <michal.simek at xilinx.com> wrote:
>>>> On 18.7.2018 22:11, Marek Vasut wrote:
>>>>> On 07/18/2018 04:57 PM, Michal Simek wrote:
>>>>>> On 18.7.2018 16:24, Marek Vasut wrote:
>>>>>>> On 07/18/2018 04:18 PM, Michal Simek wrote:
>>>>>>>> On 18.7.2018 16:15, Marek Vasut wrote:
>>>>>>>>> On 07/18/2018 04:00 PM, Michal Simek wrote:
>>>>>>>>>> On 18.7.2018 14:54, Marek Vasut wrote:
>>>>>>>>>>> This breaks Arria10, sorry. The private loading function is needed on
>>>>>>>>>>> Arria10 as the whole bitstream is not available in RAM and needs to be
>>>>>>>>>>> loaded piece by piece, see [1]
>>>>>>>>>>>
>>>>>>>>>>> [1]
>>>>>>>>>>> http://git.denx.de/?p=u-boot/u-boot-socfpga.git;a=blobdiff;f=arch/arm/mach-socfpga/spl.c;h=82adb5dfb8de62e3d928f6f4405705f3f32a780c;hp=7ee988a2d59831ec6bff927b2a5fdad7f57da055;hb=21f835ebf2b40fc8a3e8b818c5c5ba2555dd7c65;hpb=bd198801cb95b5a8460c95a762cc4a9a44ca85ef
>>>>>>>
>>>>>>> [...]
>>>>>>>>>> The second solution is to check if load address is not 0 and call
>>>>>>>>>> fpga_load only for that. In this case there is a need to check size for SPL.
>>>>>>>>>
>>>>>>>>> 0 is a valid load address, so no.
>>>>>>>>
>>>>>>>> Then new Kconfig option is another way to go now.
>>>>>>>
>>>>>>> No, the firmware loader is a way to go. Sadly, it's still work in progress.
>>>>>>
>>>>>> I have looked at that series and it will take some time to get it done
>>>>>> but even that series has no user and also only support filesystems.
>>>>>> Which is fine but not enough and support for RAW mode is necessary too.
>>>>>
>>>>> It already took like a year and half I think ... well, better invest
>>>>> your resources in perfecting it for your usecase, for that's the way to go.
>>>>
>>>> I don't think so because for SPL boot we need support for a RAW mode.
>>>>
>>>>>
>>>>>> Anyway Luis sent series where this SPL fpga supported is requested to
>>>>>> add and I had this functionality in more raw state in Xilinx tree for
>>>>>> quite a long time and it is time to support it because on Zybo (because
>>>>>> of i2c eeprom) and also cc108 (because of uart routing via PL) fpga load
>>>>>> needs to be done in SPL and we need that support.
>>>>>
>>>>> Ha
>>>>>
>>>>>> I have not a problem to keep your code in SPL but I need a way to enable
>>>>>> fpga load directly with all that features like hashes which are already
>>>>>> available. GZIP can be added pretty easily too.
>>>>>> That's why please suggest a way what you are comfortable with not to
>>>>>> block functionality on these devices.
>>>>>
>>>>> Look at the A10 nand branch, it uses full fit with all the bells and
>>>>> whistles. Maybe that's the way to go if you have DRAM available.
>>>>
>>>> Enabling full fit should be possible but there is really no need to
>>>> enable more and more features for load something to fpga. SPL still
>>>> needs to fit to small space.
>>>
>>> I tested Michal's patch by replacing the fourth patch of my series [1]
>>> with his patch, and it worked fine on a Zybo Z7-20. I think that for
>>> devices with DRAM available is the way to go, but as Marek said, it
>>> will brake Arria10.
>>
>> It doesn't break anything in mainline because there is no support for
>> Arria10.
> 
> You might want to git grep a bit ?

I meant arria10 spl fpga support not soc itself.

> 
>> His patch for this interface shouldn't reach mainline like it
>> is normal style in Linux. No user, no interface. Also if there is
>> interface already but no user then interfaces are removed like happened
>> with SG_DMA.
> 
> Uhh, that's some strong wording right there. We have many other __weak
> functions in U-Boot which are probably never implemented either. Why
> don't you clean those up too instead of focusing on one which enables
> competing platform to work and is protected by Kconfig option, so it
> doesn't change anything for the other platforms ?

I think all of them should be removed.


>>> I'd like to see the functionality merged, but waiting to firmware
>>> loader might delay it. So I propose the following:
>>> - Keep the spl_load_fpga_image() function for devices
>>>   like the Arria10.
>>> - Apply the first three patches of my series [1],
>>>   as they fix FPGA (Zynq) support on SPL.
>>
>> They are fine please send me as regular patches and I will take them.
>>
>>> - Apply a modified version of Michal's patch, adding
>>>   a temporary Kconfig option, like he suggested,
>>>   to choose between the two implementations:
>>>   fpga_load() for devices with DRAM available, and
>>>   spl_load_fpga_image() for devices like the Arria10.
>>> - Once firmware loader is ready, convert the code to
>>>   use it and unify the functions if possible.
>>
>> Let's continue to talk with Marek on this. If there he is not open to
>> better solution we need to create new Kconfig property to cover it.
> 
> Like CONFIG_SPL_FPGA_SUPPORT ?

One approach is to copy the whole image to DDR and use it.
The second copy it by chunks.

The first is what we need for Xilinx. The second what you need for
Altera because you don't have DDR up and running.
That's why there needs to be likely to Kconfig entries.

Thanks,
Michal