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

[Marek Vasut]

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/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                

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.

-- 
Best regards,
Marek Vasut