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

[Marek Vasut]

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 also mentioned the API should copy the linux firmware loader API.

> +{
> +	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
USB init shouldn't be duplicated here IMO.

> +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.

[...]


-- 
Best regards,
Marek Vasut