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.c...@intel.com wrote: >>> >>> From: Tien Fong Chee <tien.fong.c...@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.c...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot