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

Thu Nov 16 08:09:13 UTC 2017

On Jum, 2017-11-10 at 11:04 +0100, Marek Vasut wrote:
> 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.ch
> > > > > > > > > > > > > > ee 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/f00
> > > > > > > > > > > > 7cad
> > > > > > > > > > > > 159e99fa
> > > > > > > > > > > > 2acd
> > > > > > > > > > > > 3b2e
> > > > > > > > > > > > 9364
> > > > > > > > > > > > fbb32ad28b971/drivers/base/firmware_class.c#L12
> > > > > > > > > > > > 64.
> > > > > > > > > > > >  
> > > > > > > > > > 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/firm
> > > > > > > > > ware
> > > > > > > > > /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.
> 
I think we can having same function request_firmware_into_buf for chunk
reading, because i can add the offset into firmware structure so the
last write location can be stored into offset. Since we know the size
of the buffer, so we can do the calculation outside of the function and
repetitive calling the function with the return offset until whole file
is read. What do you think?

Thanks.