[U-Boot] [PATCH v3 3/3] common: Generic loader for file system

[Chee, Tien Fong <tien.fong.chee@intel.com>]

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 <tien.fong.chee@intel.com>
> wrote:
> > 
> > On Mon, 2018-06-25 at 21:58 -0600, Simon Glass wrote:
> > > 
> > > Hi,
> > > 
> > > On 25 June 2018 at 07:28,  <tien.fong.chee@intel.com> wrote:
> > > > 
> > > > 
> > > > From: Tien Fong Chee <tien.fong.chee@intel.com>
> > > > 
> > > > 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 <tien.fong.chee@intel.com>
> > > > ---
> > > >  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 <www.intel.com>
> > > > + *
> > > > + * SPDX-License-Identifier:    GPL-2.0
> > > > + */
> > > > +#include <common.h>
> > > > +#include <dm.h>
> > > > +#include <errno.h>
> > > > +#include <fs.h>
> > > > +#include <fs_loader.h>
> > > > +#include <linux/string.h>
> > > > +#include <malloc.h>
> > > > +#include <spl.h>
> > > > +
> > > > +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