[U-Boot] [PATCH v7 1/7] ARM: socfpga: Description on FPGA bitstream type and file name for Arria 10

Marek Vasut
2019-02-11T11:01:46Z

On 2/11/19 6:36 AM, Chee, Tien Fong wrote:
> On Tue, 2019-02-05 at 09:46 +0100, Marek Vasut wrote:
>> On 2/1/19 5:02 PM, Chee, Tien Fong wrote:
>>>
>>> On Fri, 2019-02-01 at 09:25 +0100, Marek Vasut wrote:
>>>>
>>>> On 2/1/19 4:48 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>
>>>>>>>
>>>>>>> This patch adds description on properties about file name
>>>>>>> used
>>>>>>> for
>>>>>>> both
>>>>>>> peripheral bitstream and core bitstream.
>>>>>>>
>>>>>>> Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
>>>>>>>
>>>>>>> ---
>>>>>>>
>>>>>>> changes for v7
>>>>>>> - Provided example of setting FPGA FIT image for both early
>>>>>>> IO
>>>>>>> release
>>>>>>>   and full release FPGA configuration.
>>>>>>> ---
>>>>>>>  .../fpga/altera-socfpga-a10-fpga-mgr.txt           | 34
>>>>>>> +++++++++++++++++++++-
>>>>>>>  1 file changed, 33 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/doc/device-tree-bindings/fpga/altera-socfpga-
>>>>>>> a10-
>>>>>>> fpga-
>>>>>>> mgr.txt b/doc/device-tree-bindings/fpga/altera-socfpga-a10-
>>>>>>> fpga-
>>>>>>> mgr.txt
>>>>>>> index 2fd8e7a..5f81a32 100644
>>>>>>> --- a/doc/device-tree-bindings/fpga/altera-socfpga-a10-
>>>>>>> fpga-
>>>>>>> mgr.txt
>>>>>>> +++ b/doc/device-tree-bindings/fpga/altera-socfpga-a10-
>>>>>>> fpga-
>>>>>>> mgr.txt
>>>>>>> @@ -7,8 +7,39 @@ Required properties:
>>>>>>>                 - The second index is for writing FPGA
>>>>>>> configuration data.
>>>>>>>  - resets     : Phandle and reset specifier for the
>>>>>>> device's
>>>>>>> reset.
>>>>>>>  - clocks     : Clocks used by the device.
>>>>>>> +- altr,bitstream : File name for FPGA peripheral bitstream
>>>>>>> which
>>>>>>> is used
>>>>>>> +    to initialize FPGA IOs, PLL, IO48 and
>>>>>>> DDR.
>>>>>>> This
>>>>>>> bitstream is
>>>>>>> +    required to get DDR up running.
>>>>>>> +    or
>>>>>>> +    File name for full bitstream, consist
>>>>>>> of
>>>>>>> peripheral bitstream
>>>>>>> +    and core bitstream.
>>>>>>> +- altr,bitstream-core(optional) : File name for core
>>>>>>> bitstream
>>>>>>> which contains
>>>>>> Is the name of the property 'altr,bitstream-core(optional)' ?
>>>>>> I
>>>>>> think
>>>>>> the "optional" part should be in the description.
>>>>> Yes, you are right.
>>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> +   FPGA design which is
>>>>>>> used to
>>>>>>> program FPGA CRAM
>>>>>>> +   and ERAM.
>>>>>>>
>>>>>>> -Example:
>>>>>>> +Example: Bundles both peripheral bitstream and core
>>>>>>> bitstream
>>>>>>> into
>>>>>>> FIT image
>>>>>>> +  called fit_spl_fpga.itb. This FIT image can be
>>>>>>> created
>>>>>>> through running
>>>>>>> +  this command: tools/mkimage
>>>>>>> +        -E -p 400
>>>>>>> +        -f board/altera/arria10-
>>>>>>> socdk/fit_spl_fpga.its
>>>>>>> +        fit_spl_fpga.itb
>>>>>>> +
>>>>>>> +  For details of describing structure and contents
>>>>>>> of
>>>>>>> the
>>>>>>> FIT image,
>>>>>>> +  please refer board/altera/arria10-
>>>>>>> socdk/fit_spl_fpga.its
>>>>>>> +
>>>>>>> +- Examples for booting with early IO release, and enter
>>>>>>> early
>>>>>>> user
>>>>>>> mode:
>>>>>>> +
>>>>>>> + fpga_mgr: fpga-mgr at ffd03000 {
>>>>>>> + compatible = "altr,socfpga-a10-fpga-mgr";
>>>>>>> + reg = <0xffd03000 0x100
>>>>>>> +        0xffcfe400 0x20>;
>>>>>>> + clocks = <&l4_mp_clk>;
>>>>>>> + resets = <&rst FPGAMGR_RESET>;
>>>>>>> + altr,bitstream = "fit_spl_fpga.itb";
>>>>>>> + altr,bitstream-core = "fit_spl_fpga.itb";
>>>>>> It's the same file, why does it use two properties ?
>>>>> 1. Allows user to run optional for program core. When "" is set
>>>>> to
>>>>> altr,bitstream-core, then SPL would skip programming FPGA with
>>>>> core, so
>>>>> user can program it later on U-Boot or Linux.
>>>> You can just pass in a fitImage with only the periph image in it
>>>> in
>>>> such
>>>> a case.
>>> What if user want to program the core on U-Boot? User need to
>>> create
>>> two FIT images, one FIT with periph image, and another FIT with
>>> core
>>> image only.
>>>
>>> Current implementation supports one FIT image for above
>>> configuration.
>> What if user want to program the core on U-Boot in this
>> implementation?
>> It is not possible either, is it ?
> There are few ways user can do:
> 1. Running commands such as imxtract/fatload with socfpga load
> 2. Script
> 3. Env

And how is that different from using a single fitImage with
configuration node describing the parts of the bitstream that should be
loaded ?

>>>>> 2. Allows core in different FIT file.
>>>> Is this really useful ?
>>> Yes, for the use case which support different core image for
>>> different
>>> revision of board but using same periph image.
>> Seems you an Dalon are discussing this and it's not a supported flow
>> ?
> Dalon has some concerns.
>
> But, we can keep the more flexibility for users by adding support of
> configuration in fit image, then removing the altr,bitstream-core
> properties in FPGA manager node. So, users are freely to use this
> implementation in their own use cases.
>
> So, user need to add the default configuration for FPGA.
> For property "fpga" = 1st string is for periph.rbf or full rbf.
>     = 2nd string is for core.rbf. This is optional, no
> string is found, the core configuration would be skipped.
>                     =  3rd string onwards would be ignored.
>
> configurations {
> default = "config-1";
> config-1 {
> description = "Boot with FPGA early IO release config";
> fpga = "fpga-1", "fpga-2";   <= sequence for bitstream
> configuration
> };
> };

I want to be able to do something like
configuration {
...
config-board1 {
...
fpga = "fpga-core1";
};
config-board2 {
...
fpga = "fpga-core2", "fpga-periph2";
};
};

Does this make sense ?

Dalon, is this mix-and-match approach to bitstreams something we want to
support at all ? It seems a bit dangerous to me.

> Anyway, please bear in mind that multi fpga nodes in one fpga fit image
> is not good for performance. I have tried few ideas to fix the
> performance penalty caused by buffer alignment at reading cluster, but
> it didn't work.
>
> What do you think?

I think if there is a performance penalty when loading file from VFAT,
it is VFAT code or SD/MMC driver that needs fixing. We definitely can
not hack it up by realigning fitImage to magically work around that.

>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>> And where is this
>>>>>> file loaded from ?
>>>>> You need to set the default source in DTS, for example
>>>>> "firmware-
>>>>> loader
>>>>> = &fs_loader0", that's for power boot up purpose. After that,
>>>>> generic
>>>>> firmware loader would go to the dsignated storage as described
>>>>> below to
>>>>> find the FPGA FIT image according description from above.
>>>>>
>>>>> fs_loader0: fs-loader at 0 {
>>>>> u-boot,dm-pre-reloc;
>>>>> compatible = "u-boot,fs-loader";
>>>>> phandlepart = <&mmc 1>;
>>>>> };
>>>> How does the driver bound to fpga-mgr know which firmware loader
>>>> instance to use ? There's no phandle.
>>> Current firmware loader supports only one instance, that is default
>>> instance described in chosen node. It is good enough to solve our
>>> problem where to define a default storage for FPGA images and how
>>> to
>>> tell the SPL to load it from the default storage when the board is
>>> powered up. I don't see there is a need to support more than one
>>> instance for fpga-mgr during SPL runtime, at least for now. User
>>> can
>>> program the FPGA with core image from any storage with series of
>>> commands such as fatload and socfpga load on U-Boot console.
>> Since you already have the label for the fs-loader _and_ you have the
>> address for it (fs-loader at 0), you should supply the phandle. And
>> eventually, someone will try to use multiple loaders, so we should do
>> this correctly from the beginning -- by supplying the phandle to the
>> loader node.
> Okay, i will improve firmware loader in separate patch set.
> Basically, the ideas are for
> 1. For majority HWs use default storage, defined in console node.
> (Already supported)

I'm not sure I understand this point, default storage in console node ?

> 2. For minority HWs use, other than default storage, users should add
> this property "firmware-loader = <phandle>" to their respective HW
> nodes. This would enable overwritten of default storage, phandle from
> console node. (planning to enhance)
> 3. Enable sequence number(DM_FLAG_PRE_RELOC) support through hard
> coding. (planning to enhance)

Why do we want to hard-code anything ?

> So, the interface is still same for this patch set, using item 1,
> default storage.
>
> What do you think?
>>
>>>
>>> It is good to improve the firmware loader to support
>>> the DM_FLAG_PRE_RELOC, which allow user to choose different
>>> firmware
>>> loader node through setting the right sequence number when creating
>>> the
>>> firmare loader instance in the parent driver such as fpga mgr, but
>>> i
>>> don't see there is urgency need to be done now.
>>>>
>>>>
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> + };
>>>>>>> +
>>>>>>> +- Examples for booting with full release, enter user mode
>>>>>>> with
>>>>>>> full bitstream:
>>>>>>>
>>>>>>>   fpga_mgr: fpga-mgr at ffd03000 {
>>>>>>>   compatible = "altr,socfpga-a10-fpga-mgr";
>>>>>>> @@ -16,4 +47,5 @@ Example:
>>>>>>>          0xffcfe400 0x20>;
>>>>>>>   clocks = <&l4_mp_clk>;
>>>>>>>   resets = <&rst FPGAMGR_RESET>;
>>>>>>> + altr,bitstream = "fit_spl_fpga.itb";
>>>>>>>   };
>>>>>>>

--
Best regards,
Marek Vasut