From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chee, Tien Fong Date: Mon, 6 Nov 2017 04:15:19 +0000 Subject: [U-Boot] [PATCH 1/2] common: Generic file system firmware loader In-Reply-To: References: <1509527902-5080-1-git-send-email-tien.fong.chee@intel.com> <1509527902-5080-2-git-send-email-tien.fong.chee@intel.com> <05168571-9b72-0ff5-e1c7-07c47b2222a6@denx.de> <1509610841.2368.18.camel@intel.com> Message-ID: <1509941716.2383.1.camel@intel.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de 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 > > > > > > > > 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 > > > > --- > > > >  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. >