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

[Chee, Tien Fong]

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/f007cad159e99fa2acd3b2e9364
fbb32ad28b971/drivers/base/firmware_class.c#L1264.

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                
         
> > +	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.
> > 
> > +static int flash_init_usb(void)
> > +{
> > +	int err;
> > +
> > +	err = usb_init();
> > +	if (err)
> > +		return err;
> > +
> > +#ifndef CONFIG_DM_USB
> > +	err = usb_stor_scan(1) < 0 ? -ENODEV : 0;
> > +#endif
> > +
> > +	return err;
> > +}
> > +#else
> > +static inline int flash_init_usb(void)
> Don't use inline , the compiler can decide better.
> 
Okay.
> [...]
> 
>