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

[Chee, Tien Fong]

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 intel.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/f007cad159e99fa2acd3b2e
> > 9364
> > fbb32ad28b971/drivers/base/firmware_class.c#L1264.
> > 
I would like to confirm with you whether we are talking to the same API
above?

> > Actually we have almost same framework in filesystem loader
> > portion,
> > just different implementation, and Linux firmware loader is more
> > specific to Linux environment such as hard code path searching in
> > RFS.
> > The generic FS loader in this patch is much more flexible, let user
> > to
> > define their own prefer implementation.
> >  Linux FS firmware loader  <--->   U-Boot FS firmware loader
> > --------------------------       ---------------------------
> > 1) request_firmware			flash_load_fs
> > 2) _request_firmware_prepare          weak fsloader_preprocess
> > 3) fw_get_filesystem_firmware          weak fs_loading            
> >    
> The API should be the same or very similar to make porting of drivers
> from Linux easy and allow people to know only one API, not two.
> 
> > 
> > > 
> > > > 
> > > > +	int res;
> > > > +
> > > > +	switch (location->storage) {
> > > > +	case FLASH_STORAGE_MMC:
> > > > +		res = fs_set_blk_dev("mmc", location->devpart,
> > > > FS_TYPE_ANY);
> > > > +		break;
> > > > +	case FLASH_STORAGE_USB:
> > > > +		res = fs_set_blk_dev("usb", location->devpart,
> > > > FS_TYPE_ANY);
> > > > +		break;
> > > > +	case FLASH_STORAGE_SATA:
> > > > +		res = fs_set_blk_dev("sata", location-
> > > > >devpart,
> > > > FS_TYPE_ANY);
> > > > +		break;
> > > > +	case FLASH_STORAGE_NAND:
> > > > +		if (location->ubivol != NULL)
> > > > +			res = fs_set_blk_dev("ubi", NULL,
> > > > FS_TYPE_UBIFS);
> > > > +		else
> > > > +			res = -ENODEV;
> > > > +		break;
> > > > +	default:
> > > > +		error("Error: unsupported location
> > > > storage.\n");
> > > > +		return -ENODEV;
> > > > +	}
> > > > +
> > > > +	if (res)
> > > > +		error("Error: could not access storage.\n");
> > > > +
> > > > +	return res;
> > > > +}
> > > > +
> > > > +#ifndef CONFIG_SPL_BUILD
> > > > +#ifdef CONFIG_USB_STORAGE
> > > This looks wrong, the USB can be supported in SPL no problem. And
> > > this
> > Technically, USB can be supported in SPL, but the build for USB in
> > SPL
> > is not supported yet.
> > > 
> > > USB init shouldn't be duplicated here IMO.
> > > 
> > This is just for the case USB init is not yet started, but loader
> > is
> > called 1st.
> I am not asking WHY this is needed. I suspect we have this code
> somewhere already, so it's a duplicate here.
>