From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chee, Tien Fong Date: Thu, 28 Jun 2018 04:45:17 +0000 Subject: [U-Boot] [PATCH v3 3/3] common: Generic loader for file system In-Reply-To: References: <1529933338-11010-1-git-send-email-tien.fong.chee@intel.com> <1529933338-11010-4-git-send-email-tien.fong.chee@intel.com> <1530088890.9999.19.camel@intel.com> Message-ID: <1530161117.10114.3.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 Wed, 2018-06-27 at 14:03 -0700, Simon Glass wrote: > Hi Tien Fong, > > On 27 June 2018 at 01:41, Chee, Tien Fong > wrote: > > > > On Mon, 2018-06-25 at 21:58 -0600, Simon Glass wrote: > > > > > > Hi, > > > > > > On 25 June 2018 at 07:28,   wrote: > > > > > > > > > > > > From: Tien Fong Chee > > > > > > > > This is file system generic loader which can be used to load > > > > the file image from the storage into target such as memory. > > > > The consumer driver would then use this loader to program > > > > whatever, > > > > ie. the FPGA device. > > > > > > > > Signed-off-by: Tien Fong Chee > > > > --- > > > >  drivers/misc/Kconfig     |  10 ++ > > > >  drivers/misc/Makefile    |   1 + > > > >  drivers/misc/fs_loader.c | 238 > > > > +++++++++++++++++++++++++++++++++++++++++++++++ > > > >  include/dm/uclass-id.h   |   1 + > > > >  include/fs_loader.h      |  28 ++++++ > > > >  5 files changed, 278 insertions(+) > > > >  create mode 100644 drivers/misc/fs_loader.c > > > >  create mode 100644 include/fs_loader.h > > > This is definitely looking good. I think we need to figure out > > > the > > > binding to devices, to use phandles instead of strings. Also we > > > need > > > a > > > sandbox driver and test. > > > > > > I'm a little worried that you are blazing a trail here, but it is > > > a > > > valuable trail :-) > > > > > > Tom, do you have any ideas / thoughts on the design? > > > > > yeah, this part is most challenging, because this driver is > > designed > > based on file system implementation, which is abstract from > > physical > > device. So, it requires data like interface type, device number and > > partition to work. Wonder how we can get those data if based on > > physical storage node. > > > > > > > > > > > > > > > > > > > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig > > > > index 17b3a80..4163b4f 100644 > > > > --- a/drivers/misc/Kconfig > > > > +++ b/drivers/misc/Kconfig > > > > @@ -277,4 +277,14 @@ config GDSYS_RXAUI_CTRL > > > >         depends on MISC > > > >         help > > > >           Support gdsys FPGA's RXAUI control. > > > > + > > > > +config FS_LOADER > > > > +       bool "Enable loader driver for file system" > > > > +       help > > > > +         This is file system generic loader which can be used > > > > to > > > > load > > > > +         the file image from the storage into target such as > > > > memory. > > > > + > > > > +         The consumer driver would then use this loader to > > > > program > > > > whatever, > > > > +         ie. the FPGA device. > > > > + > > > >  endmenu > > > > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile > > > > index 4ce9d21..67a36f8 100644 > > > > --- a/drivers/misc/Makefile > > > > +++ b/drivers/misc/Makefile > > > > @@ -54,3 +54,4 @@ obj-$(CONFIG_STM32_RCC) += stm32_rcc.o > > > >  obj-$(CONFIG_STM32MP_FUSE) += stm32mp_fuse.o > > > >  obj-$(CONFIG_SYS_DPAA_QBMAN) += fsl_portals.o > > > >  obj-$(CONFIG_GDSYS_RXAUI_CTRL) += gdsys_rxaui_ctrl.o > > > > +obj-$(CONFIG_FS_LOADER) += fs_loader.o > > > > diff --git a/drivers/misc/fs_loader.c > > > > b/drivers/misc/fs_loader.c > > > > new file mode 100644 > > > > index 0000000..0dc385f > > > > --- /dev/null > > > > +++ b/drivers/misc/fs_loader.c > > > > @@ -0,0 +1,238 @@ > > > > +/* > > > > + * Copyright (C) 2018 Intel Corporation > > > > + * > > > > + * SPDX-License-Identifier:    GPL-2.0 > > > > + */ > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > + > > > > +DECLARE_GLOBAL_DATA_PTR; > > > > + > > > > +struct firmware_priv { > > > > +       const char *name;       /* Filename */ > > > > +       u32 offset;             /* Offset of reading a file */ > > > > +}; > > > > + > > > > +static int select_fs_dev(struct device_platdata *plat) > > > > +{ > > > > +       int ret; > > > > + > > > > +       if (!strcmp("mmc", plat->name)) { > > > > +               ret = fs_set_blk_dev("mmc", plat->devpart, > > > > FS_TYPE_ANY); > > > > +       } else if (!strcmp("usb", plat->name)) { > > > > +               ret = fs_set_blk_dev("usb", plat->devpart, > > > > FS_TYPE_ANY); > > > > +       } else if (!strcmp("sata", plat->name)) { > > > > +               ret = fs_set_blk_dev("sata", plat->devpart, > > > > FS_TYPE_ANY); > > > For these I think you can look up the phandle to obtain the > > > device, > > > then check its uclass to find out its type. > > > > > How to find the interface type of the file system based on the > > physical > > device node? Some devices like NAND and QSPI could share the > > similar > > interface type like UBI. > See my other email on this. > Okay. > > > > > > > > > > > > > > > > > +       } else if (!strcmp("ubi", plat->name)) { > > > > +               if (plat->ubivol) > > > > +                       ret = fs_set_blk_dev("ubi", NULL, > > > > FS_TYPE_UBIFS); > > > > +               else > > > > +                       ret = -ENODEV; > > > > +       } else { > > > > +               debug("Error: unsupported storage device.\n"); > > > > +               return -ENODEV; > > > > +       } > > > > + > > > > +       if (ret) > > > > +               debug("Error: could not access storage.\n"); > > > > + > > > > +       return ret; > > > > +} > > > > + > > > > +static void set_storage_devpart(struct device_platdata *plat, > > > > char > > > > *devpart) > > > > +{ > > > > +       plat->devpart = devpart; > > > > +} > > > > + > > > > +static void set_storage_mtdpart(struct device_platdata *plat, > > > > char > > > > *mtdpart) > > > > +{ > > > > +       plat->mtdpart = mtdpart; > > > > +} > > > > + > > > > +static void set_storage_ubivol(struct device_platdata *plat, > > > > char > > > > *ubivol) > > > > +{ > > > > +       plat->ubivol = ubivol; > > > > +} > > > > + > > > > +/** > > > > + * _request_firmware_prepare - Prepare firmware struct. > > > > + * > > > > + * @firmware_p: Pointer to pointer to firmware image. > > > > + * @name: Name of firmware file. > > > > + * @dbuf: Address of buffer to load firmware into. > > > > + * @size: Size of buffer. > > > > + * @offset: Offset of a file for start reading into buffer. > > > > + * > > > > + * Return: Negative value if fail, 0 for successful. > > > > + */ > > > > +static int _request_firmware_prepare(struct firmware > > > > **firmware_p, > > > Can you please move this to be the last arg and rename to > > > firmwarep? > > > > > Okay. > > > > > > > > > > > > > > > +                                   const char *name, void > > > > *dbuf,device to ubi > > > > +                                   size_t size, u32 offset) > > > > +{ > > > > +       struct firmware *firmware; > > > > +       struct firmware_priv *fw_priv; > > > > + > > > > +       *firmware_p = NULL; > > > > + > > > > +       if (!name || name[0] == '\0') > > > > +               return -EINVAL; > > > > + > > > > +       firmware = calloc(1, sizeof(*firmware)); > > > > +       if (!firmware) > > > > +               return -ENOMEM; > > > > + > > > > +       fw_priv = calloc(1, sizeof(*fw_priv)); > > > > +       if (!fw_priv) { > > > > +               free(firmware); > > > > +               return -ENOMEM; > > > > +       } > > > > + > > > > +       fw_priv->name = name; > > > > +       fw_priv->offset = offset; > > > > +       firmware->data = dbuf; > > > > +       firmware->size = size; > > > > +       firmware->priv = fw_priv; > > > > +       *firmware_p = firmware; > > > > + > > > > +       return 0; > > > > +} > > > > + > > > > +/** > > > > + * fw_get_filesystem_firmware - load firmware into an > > > > allocated > > > > buffer. > > > > + * @plat: Platform data such as storage and partition firmware > > > > loading from. > > > > + * @firmware_p: pointer to firmware image. > > > > + * > > > > + * Return: Size of total read, negative value when error. > > > > + */ > > > > +static int fw_get_filesystem_firmware(struct device_platdata > > > > *plat, > > > > +                                    struct firmware > > > > *firmware_p) > > > s/firmware_p/firmware/ > > > > > Okay. > > > > > > > > > > > > > > > +{ > > > > +       struct firmware_priv *fw_priv = NULL; > > > > +       loff_t actread; > > > > +       char *dev_part, *ubi_mtdpart, *ubi_volume; > > > > +       int ret; > > > > + > > > > +       dev_part = env_get("fw_dev_part"); > > > > +       if (dev_part) > > > > +               set_storage_devpart(plat, dev_part); > > > > + > > > > +       ubi_mtdpart = env_get("fw_ubi_mtdpart"); > > > > +       if (ubi_mtdpart) > > > > +               set_storage_mtdpart(plat, ubi_mtdpart); > > > Do you mean that the storage partition comes from the > > > environment? I > > > thought it came from the FDT? > > > > > This driver is designed not only supporting the FDT, it also work > > with > > U-boot env, U-boot script and without FDT. > > > > For example, fpga loadfs commands from U-boot console can call this > > driver to load bitstream file from the storage. > OK, but in that case why use environment? Can't you just put the > parameters in the command? > User can save the value like partition into environment and saving in the storage as new default env value. So, this env value would be used for every power on. > [..] > > Regards, > Simon