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

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

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.
> > 
> > +       } 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.
> > 
> > +
> > +       ubi_volume = env_get("fw_ubi_volume");
> > +       if (ubi_volume)
> > +               set_storage_ubivol(plat, ubi_volume);
> > +
> > +       ret = select_fs_dev(plat);
> > +       if (ret)
> > +               goto out;
> > +
> > +       fw_priv = firmware_p->priv;
> > +
> > +       ret = fs_read(fw_priv->name, (ulong)firmware_p->data,
> > fw_priv->offset,
> map_to_sysmem(firmware_p->data)
> 
> (so that it works on sandbox)
> 
Okay.
> > 
> > +                    firmware_p->size, &actread);
> > +       if (ret) {
> > +               debug("Error: %d Failed to read %s from flash %lld
> > != %d.\n",
> > +                     ret, fw_priv->name, actread, firmware_p-
> > >size);
> > +       } else {
> > +               ret = actread;
> > +       }
> > +
> > +out:
> > +       return ret;
> > +}
> > +
> > +/**
> > + * request_firmware_into_buf - Load firmware into a previously
> > allocated buffer.
> > + * @plat: Platform data such as storage and partition firmware
> > loading from.
> > + * @firmware_p: Pointer to firmware image.
> > + * @name: Name of firmware file.
> > + * @buf: Address of buffer to load firmware into.
> > + * @size: Size of buffer.
> > + * @offset: Offset of a file for start reading into buffer.
> > + *
> > + * The firmware is loaded directly into the buffer pointed to by
> > @buf and
> > + * the @firmware_p data member is pointed at @buf.
> > + *
> > + * Return: Size of total read, negative value when error.
> > + */
> > +int request_firmware_into_buf(struct device_platdata *plat,
> > +                             struct firmware **firmware_p,
> > +                             const char *name,
> > +                             void *buf, size_t size, u32 offset)
> > +{
> > +       int ret;
> > +
> > +       if (!plat)
> > +               return -EINVAL;
> > +
> > +       ret = _request_firmware_prepare(firmware_p, name, buf,
> > size, offset);
> > +       if (ret < 0) /* error */
> > +               return ret;
> > +
> > +       ret = fw_get_filesystem_firmware(plat, *firmware_p);
> > +
> > +       return ret;
> > +}
> > +
> > +static int fs_loader_ofdata_to_platdata(struct udevice *dev)
> > +{
> > +       const char *fs_loader_path;
> > +
> > +       fs_loader_path = ofnode_get_chosen_prop("firmware-loader");
> > +
> > +       if (fs_loader_path) {
> > +               ofnode fs_loader_node;
> > +
> > +               fs_loader_node = ofnode_path(fs_loader_path);
> > +               if (ofnode_valid(fs_loader_node)) {
> > +                       struct device_platdata *plat;
> > +                       plat = dev->platdata;
> > +
> > +                       plat->name = (char
> > *)ofnode_read_string(fs_loader_node,
> > +                                        "storage_device");
> > +
> > +                       plat->devpart = (char *)ofnode_read_string(
> > +                                        fs_loader_node,
> > "devpart");
> > +
> > +                       plat->mtdpart = (char *)ofnode_read_string(
> > +                                        fs_loader_node,
> > "mtdpart");
> > +
> > +                       plat->ubivol = (char *)ofnode_read_string(
> > +                                        fs_loader_node, "ubivol");
> > +               }
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int fs_loader_probe(struct udevice *dev)
> > +{
> > +       return 0;
> > +};
> > +
> > +static const struct udevice_id fs_loader_ids[] = {
> > +       { .compatible = "fs_loader"},
> > +       { }
> > +};
> > +
> > +U_BOOT_DRIVER(fs_loader) = {
> > +       .name                   = "fs_loader",
> > +       .id                     = UCLASS_FS_FIRMWARE_LOADER,
> > +       .of_match               = fs_loader_ids,
> > +       .probe                  = fs_loader_probe,
> > +       .ofdata_to_platdata     = fs_loader_ofdata_to_platdata,
> > +       .platdata_auto_alloc_size       = sizeof(struct
> > device_platdata),
> > +};
> > +
> > +UCLASS_DRIVER(fs_loader) = {
> > +       .id             = UCLASS_FS_FIRMWARE_LOADER,
> > +       .name           = "fs_loader",
> > +};
> > diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> > index d7f9df3..39e88ac 100644
> > --- a/include/dm/uclass-id.h
> > +++ b/include/dm/uclass-id.h
> > @@ -36,6 +36,7 @@ enum uclass_id {
> >         UCLASS_DMA,             /* Direct Memory Access */
> >         UCLASS_EFI,             /* EFI managed devices */
> >         UCLASS_ETH,             /* Ethernet device */
> > +       UCLASS_FS_FIRMWARE_LOADER,              /* Generic loader
> > */
> >         UCLASS_GPIO,            /* Bank of general-purpose I/O pins
> > */
> >         UCLASS_FIRMWARE,        /* Firmware */
> >         UCLASS_I2C,             /* I2C bus */
> > diff --git a/include/fs_loader.h b/include/fs_loader.h
> > new file mode 100644
> > index 0000000..e3e5eeb
> > --- /dev/null
> > +++ b/include/fs_loader.h
> > @@ -0,0 +1,28 @@
> > +/*
> > + * Copyright (C) 2018 Intel Corporation <www.intel.com>
> > + *
> > + * SPDX-License-Identifier:    GPL-2.0
> > + */
> > +#ifndef _FS_LOADER_H_
> > +#define _FS_LOADER_H_
> > +
> > +#include <dm.h>
> > +
> > +struct firmware {
> > +       size_t size;            /* Size of a file */
> > +       const u8 *data;         /* Buffer for file */
> > +       void *priv;             /* Firmware loader private fields
> > */
> > +};
> > +
> > +struct device_platdata {
> > +       char *name;     /* Such as mmc, usb,and sata. */
> > +       char *devpart;  /* Use the load command dev:part
> > conventions */
> > +       char *mtdpart;  /* MTD partition for ubi part */
> > +       char *ubivol;   /* UBI volume-name for ubifsmount */
> > +};
> > +
> > +int request_firmware_into_buf(struct device_platdata *plat,
> > +                             struct firmware **firmware_p,
> > +                             const char *name,
> > +                             void *buf, size_t size, u32 offset);
> Please can you add a full function comment here?
> 
Okay.
> > 
> > +#endif
> > --
> > 2.2.0
> > 
> Regards,
> Simon