[U-Boot] [PATCH 1/2] common: Generic file system firmware loader

[Marek Vasut]

On 11/10/2017 10:05 AM, Chee, Tien Fong wrote:
> On Kha, 2017-11-09 at 11:31 +0100, Marek Vasut wrote:
>> On 11/09/2017 11:00 AM, Lukasz Majewski wrote:
>>>
>>> On Thu, 9 Nov 2017 08:05:18 +0100
>>> Marek Vasut <marex at denx.de> wrote:
>>>
>>>>
>>>> On 11/09/2017 07:04 AM, Chee, Tien Fong wrote:
>>>>>
>>>>> On Sel, 2017-11-07 at 10:34 +0100, Marek Vasut wrote:  
>>>>>>
>>>>>> On 11/07/2017 10:03 AM, Chee, Tien Fong wrote:  
>>>>>>>
>>>>>>>
>>>>>>> On Isn, 2017-11-06 at 11:56 +0100, Marek Vasut wrote:  
>>>>>>>>
>>>>>>>>
>>>>>>>> On 11/06/2017 05:15 AM, Chee, Tien Fong wrote:  
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Ahd, 2017-11-05 at 17:43 +0100, Marek Vasut wrote:  
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 11/02/2017 09:20 AM, Chee, Tien Fong wrote:  
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On Rab, 2017-11-01 at 10:26 +0100, Marek Vasut
>>>>>>>>>>> wrote:  
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 11/01/2017 10:18 AM, tien.fong.chee at intel.com
>>>>>>>>>>>> wrote:  
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> From: Tien Fong Chee <tien.fong.chee at intel.com>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Generic firmware loader framework contains some
>>>>>>>>>>>>> common
>>>>>>>>>>>>> functionality
>>>>>>>>>>>>> which is factored out from splash loader. It is
>>>>>>>>>>>>> reusable by
>>>>>>>>>>>>> any
>>>>>>>>>>>>> specific driver file system firmware loader.
>>>>>>>>>>>>> Specific
>>>>>>>>>>>>> driver
>>>>>>>>>>>>> file
>>>>>>>>>>>>> system
>>>>>>>>>>>>> firmware loader handling can be defined with
>>>>>>>>>>>>> both weak
>>>>>>>>>>>>> function
>>>>>>>>>>>>> fsloader_preprocess and fs_loading.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: Tien Fong Chee <tien.fong.chee at i
>>>>>>>>>>>>> ntel.com  
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>  
>>>>>>>>>>>>> ---
>>>>>>>>>>>>>  common/Makefile   |   1 +
>>>>>>>>>>>>>  common/load_fs.c  | 217
>>>>>>>>>>>>> +++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>>>>>> +++++++
>>>>>>>>>>>>>  include/load_fs.h |  38 ++++++++++
>>>>>>>>>>>>>  3 files changed, 256 insertions(+)
>>>>>>>>>>>>>  create mode 100644 common/load_fs.c
>>>>>>>>>>>>>  create mode 100644 include/load_fs.h  
>>>>>>>>>>>> [...]
>>>>>>>>>>>>  
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> +int flash_select_fs_dev(struct flash_location
>>>>>>>>>>>>> *location)  
>>>>>>>>>>>> Why does everything have flash_ prefix ?
>>>>>>>>>>>>  
>>>>>>>>>>> I can remove the flash_ prefix, this generic FS
>>>>>>>>>>> loader
>>>>>>>>>>> should
>>>>>>>>>>> support
>>>>>>>>>>> for all filesystem instead of flash.
>>>>>>>>>>>  
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> I also mentioned the API should copy the linux
>>>>>>>>>>>> firmware
>>>>>>>>>>>> loader
>>>>>>>>>>>> API.
>>>>>>>>>>>>  
>>>>>>>>>>> If i'm not mistaken, you are referring firmware
>>>>>>>>>>> loader API
>>>>>>>>>>> in
>>>>>>>>>>> this
>>>>>>>>>>> link https://github.com/torvalds/linux/blob/f007cad
>>>>>>>>>>> 159e99fa
>>>>>>>>>>> 2acd
>>>>>>>>>>> 3b2e
>>>>>>>>>>> 9364
>>>>>>>>>>> fbb32ad28b971/drivers/base/firmware_class.c#L1264.
>>>>>>>>>>>  
>>>>>>>>> I would like to confirm with you whether we are talking
>>>>>>>>> to the
>>>>>>>>> same
>>>>>>>>> API
>>>>>>>>> above?  
>>>>>>>> https://www.kernel.org/doc/html/v4.13/driver-api/firmware
>>>>>>>> /index.h
>>>>>>>> tml
>>>>>>>>
>>>>>>>> first link on google btw . You might be able to avoid the
>>>>>>>> firmware
>>>>>>>> structure.
>>>>>>>>  
>>>>>>> After assessment, i found that Linux loader is not suitable
>>>>>>> for
>>>>>>> fpga
>>>>>>> loader as fpga loader need
>>>>>>> 1) Able to program FPGA image in SPL chunk bu chunk with
>>>>>>> small
>>>>>>> memory
>>>>>>> allocatted.
>>>>>>> 2) Name of FPGA image defined in DTS, and path of FPGA
>>>>>>> image in
>>>>>>> FAT and
>>>>>>> UBI partition.
>>>>>>>
>>>>>>> Linux loader is strongly designed based on Linux
>>>>>>> environment,
>>>>>>> always
>>>>>>> assume having RFF, env support(which SPL don't have), sysfs
>>>>>>> and
>>>>>>> udev
>>>>>>> feature.  
>>>>>> Sigh, you can just have some additional function call to
>>>>>> fetch
>>>>>> smaller
>>>>>> chunks from a file, I don't think it's that hard of a problem
>>>>>> ...
>>>>>>  
>>>>> We already have that function to support smaller chunks, and it
>>>>> also
>>>>> work for single large image when enough memory is available in
>>>>> ver 1
>>>>> series of fpga loadfs.
>>>>>
>>>>> Since the Linux loader API is totally not suitable for fpga
>>>>> loadfs,
>>>>> and also nothing i can leverage from there for fpga loadfs,
>>>>> could
>>>>> you please consider to accept implementation for this series
>>>>> patches or implementation for fpga loadfs series ver1?  
>>>> You mean going back to completely custom non-generic altera-only
>>>> ad-hoc interface ? I'd like to hear opinion of the others on CC
>>>> ...
>>>>
>>> I must admit that I don't know the exact Altera API for loading
>>> their
>>> bitstream.
>> That's irrelevant for a generic loader. The loader should provide a
>> file
>> or ability to read chunks of file if needed, that's all. The consumer
>> driver would then use that API to program whatever, ie. the FPGA.
>>
>>>
>>> What I would like to have though is a some kind of generic code,
>>> which
>>> would allow me to reuse it on other ARM + DSP SoCs.....
>> ... on other platforms in general.
>>
>>>
>>> If we cannot re-use Linux stuff, then when we add something
>>> different
>>> (more customer/industry aligned), please make it reusable for other
>>> solutions (Xilinx, ADI, etc) - that would require a good
>>> documentation.
>> And what is the problem with the Linux API ? I am not saying to reuse
>> the Linux code, but the API is quite well fleshed out.
>>
> I found there is one function called "request_firmware_into_buf" from
> Linux firmware loader API which can be used for reading a file, or
> ability to read chunks of file.
> 
> So, how about we just go ahead with this function implemented in U-
> Boot?

You can also have request_firmware_chunk() function, since
request_firmware_into_buf() doesn't have offset. That's fine.

-- 
Best regards,
Marek Vasut