[U-Boot] [PATCH v4 6/6] common: Generic loader for file system

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

On Wed, 2018-07-11 at 14:13 -0600, Simon Glass wrote:
> Hi Tien,
> 
> On 6 July 2018 at 02: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 | 295
> > +++++++++++++++++++++++++++++++++++++++++++++++
> >  include/dm/uclass-id.h   |   1 +
> >  include/fs_loader.h      |  79 +++++++++++++
> >  5 files changed, 386 insertions(+)
> >  create mode 100644 drivers/misc/fs_loader.c
> >  create mode 100644 include/fs_loader.h
> > 
> > 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.
> *a* file image?
Okay.
> 
> > 
> > +
> > +         The consumer driver would then use this loader to program
> > whatever,
> > +         ie. the FPGA device.
> e.g. an FPGA device
Okay.
> 
> > 
> > +
> >  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..5fe642b
> > --- /dev/null
> > +++ b/drivers/misc/fs_loader.c
> > @@ -0,0 +1,295 @@
> > +/*
> > + * Copyright (C) 2018 Intel Corporation <www.intel.com>
> > + *
> > + * SPDX-License-Identifier:    GPL-2.0
> > + */
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <errno.h>
> > +#include <blk.h>
> > +#include <fs.h>
> > +#include <fs_loader.h>
> > +#include <linux/string.h>
> > +#include <mapmem.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 */
> I don't understand that comment. Is it the offset within the file to
> read from? Please can you expand it a bit?
Sure. This is offset within the file, can be used in limited memory
buffer, chunk by chunk transfer from device storage to memory.
> 
> > 
> > +};
> > +
> > +#ifdef CONFIG_CMD_UBIFS
> > +static int mount_ubifs(char *mtdpart, char *ubivol)
> > +{
> > +       int ret = ubi_part(mtdpart, NULL);
> > +
> > +       if (ret) {
> > +               debug("Cannot find mtd partition %s\n", mtdpart);
> > +               return ret;
> > +       }
> > +
> > +       return cmd_ubifs_mount(ubivol);
> > +}
> > +
> > +static int umount_ubifs(void)
> > +{
> > +       return cmd_ubifs_umount();
> > +}
> > +#else
> > +static int mount_ubifs(char *mtdpart, char *ubivol)
> > +{
> > +       debug("Error: Cannot load image: no UBIFS support\n");
> blank line here
Okay.
> 
> > 
> > +       return -ENOSYS;
> > +}
> > +#endif
> > +
> > +static int select_fs_dev(struct device_platdata *plat)
> > +{
> > +       int ret;
> > +
> > +       if (plat->phandlepart.phandle) {
> > +               ofnode node;
> > +
> > +               node = ofnode_get_by_phandle(plat-
> > >phandlepart.phandle);
> > +
> > +               int of_offset = ofnode_to_offset(node);
> > +
> > +               struct udevice *dev;
> > +
> > +               ret = device_get_global_by_of_offset(of_offset,
> > &dev);
> Can you please create device_get_global_by_ofnode() and use that
> instead? We should not use offsets in new code.
okay.
> 
> > 
> > +               if (!ret) {
> > +                       struct blk_desc *desc =
> > blk_get_by_device(dev);
> > +                       if (desc) {
> > +                               ret =
> > fs_set_blk_dev_with_part(desc,
> > +                                       plat-
> > >phandlepart.partition);
> > +                       } else {
> > +                               debug("%s: No device found\n",
> > __func__);
> > +                               return -ENODEV;
> > +                       }
> > +               }
> > +       } else if (plat->mtdpart && plat->ubivol) {
> > +               ret = mount_ubifs(plat->mtdpart, plat->ubivol);
> > +               if (ret)
> > +                       return ret;
> > +
> > +               ret = fs_set_blk_dev("ubi", NULL, FS_TYPE_UBIFS);
> > +       } else {
> > +               debug("Error: unsupported storage device.\n");
> > +               return -ENODEV;
> > +       }
> > +
> > +       if (ret)
> > +               debug("Error: could not access storage.\n");
> > +
> > +       return ret;
> > +}
> > +
> > +/**
> > + * _request_firmware_prepare - Prepare firmware struct.
> > + *
> > + * @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.
> > + * @firmwarep: Pointer to pointer to firmware image.
> > + *
> > + * Return: Negative value if fail, 0 for successful.
> > + */
> > +static int _request_firmware_prepare(const char *name, void *dbuf,
> > +                                   size_t size, u32 offset,
> > +                                   struct firmware **firmwarep)
> > +{
> struct firmware *firmware = *firmwarep;
> 
> and use that instead of the clumsy *firmwarep in this function
Okay.
> 
> > 
> > +       if (!name || name[0] == '\0')
> > +               return -EINVAL;
> > +
> > +       /* No memory allocation is required if *firmwarep is
> > allocated */
> > +       if (!(*firmwarep)) {
> Can this not be allocated automatically by the uclass or driver?
Okay, i will try it out.
> 
> > 
> > +               (*firmwarep) = calloc(1, sizeof(struct firmware));
> > +               if (!(*firmwarep))
> > +                       return -ENOMEM;
> > +
> > +               (*firmwarep)->priv = calloc(1, sizeof(struct
> > firmware_priv));
> > +               if (!(*firmwarep)->priv) {
> > +                       free(*firmwarep);
> > +                       return -ENOMEM;
> > +               }
> > +       } else if (!(*firmwarep)->priv) {
> Can you not use the driver's priv_auto_alloc_size to allocate this?
> Or
> the uclass?
Okay, i will try it out.
> 
> > 
> > +               (*firmwarep)->priv = calloc(1, sizeof(struct
> > firmware_priv));
> > +               if (!(*firmwarep)->priv) {
> > +                       free(*firmwarep);
> > +                       return -ENOMEM;
> > +               }
> > +       }
> > +
> > +       ((struct firmware_priv *)((*firmwarep)->priv))->name =
> > name;
> Again this needs a local variable.
Why?
> 
> > 
> > +       ((struct firmware_priv *)((*firmwarep)->priv))->offset =
> > offset;
> > +       (*firmwarep)->data = dbuf;
> > +       (*firmwarep)->size = size;
> > +
> > +       return 0;
> > +}
> > +
> > +/**
> > + * release_firmware - Release the resource associated with a
> > firmware image
> > + * @firmware: Firmware resource to release
> > + */
> > +void release_firmware(struct firmware *firmware)
> > +{
> > +       if (firmware) {
> > +               if (firmware->priv) {
> > +                       free(firmware->priv);
> > +                       firmware->priv = NULL;
> > +               }
> > +               free(firmware);
> > +       }
> > +}
> > +
> > +/**
> > + * fw_get_filesystem_firmware - load firmware into an allocated
> > buffer.
> > + * @plat: Platform data such as storage and partition firmware
> > loading from.
> > + * @firmware: 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)
> > +{
> > +       struct firmware_priv *fw_priv = NULL;
> > +       loff_t actread;
> > +       char *storage_interface, *dev_part, *ubi_mtdpart,
> > *ubi_volume;
> > +       int ret;
> > +
> > +       storage_interface = env_get("storage_interface");
> > +       dev_part = env_get("fw_dev_part");
> > +       ubi_mtdpart = env_get("fw_ubi_mtdpart");
> > +       ubi_volume = env_get("fw_ubi_volume");
> > +
> > +       if (storage_interface && dev_part) {
> > +               ret = fs_set_blk_dev(storage_interface, dev_part,
> > FS_TYPE_ANY);
> > +       } else if (storage_interface && ubi_mtdpart && ubi_volume)
> > {
> > +               ret = mount_ubifs(ubi_mtdpart, ubi_volume);
> > +               if (ret)
> > +                       return ret;
> > +
> > +               if (!strcmp("ubi", storage_interface))
> > +                       ret = fs_set_blk_dev(storage_interface,
> > NULL,
> > +                               FS_TYPE_UBIFS);
> > +               else
> > +                       ret = -ENODEV;
> > +       } else {
> > +               ret = select_fs_dev(plat);
> > +       }
> > +
> > +       if (ret)
> > +               goto out;
> > +
> > +       fw_priv = firmware->priv;
> > +
> > +       ret = fs_read(fw_priv->name, (ulong)map_to_sysmem(firmware-
> > >data),
> > +                       fw_priv->offset, firmware->size, &actread);
> > +       if (ret) {
> > +               debug("Error: %d Failed to read %s from flash %lld
> > != %d.\n",
> > +                     ret, fw_priv->name, actread, firmware->size);
> > +       } else {
> > +               ret = actread;
> > +       }
> > +
> > +out:
> > +#ifdef CONFIG_CMD_UBIFS
> > +       umount_ubifs();
> > +#endif
> debug() here to report failure
Okay.
> 
> > 
> > +       return ret;
> > +}
> > +
> > +/**
> > + * request_firmware_into_buf - Load firmware into a previously
> > allocated buffer.
> > + * @plat: Platform data such as storage and partition firmware
> > loading from.
> > + * @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.
> > + * @firmwarep: Pointer to firmware image.
> > + *
> > + * The firmware is loaded directly into the buffer pointed to by
> > @buf and
> > + * the @firmwarep data member is pointed at @buf.
> > + *
> > + * Return: Size of total read, negative value when error.
> > + */
> > +int request_firmware_into_buf(struct device_platdata *plat,
> > +                             const char *name,
> > +                             void *buf, size_t size, u32 offset,
> > +                             struct firmware **firmwarep)
> > +{
> > +       int ret;
> > +
> > +       if (!plat)
> > +               return -EINVAL;
> > +
> > +       ret = _request_firmware_prepare(name, buf, size, offset,
> > firmwarep);
> > +       if (ret < 0) /* error */
> > +               return ret;
> > +
> > +       ret = fw_get_filesystem_firmware(plat, *firmwarep);
> > +
> > +       return ret;
> > +}
> > +
> > +static int fs_loader_ofdata_to_platdata(struct udevice *dev)
> > +{
> > +       const char *fs_loader_path;
> > +       u32 phandlepart[2];
> > +
> > +       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;
> > +
> > +                       if (!ofnode_read_u32_array(fs_loader_node,
> > +                                                 "phandlepart",
> > +                                                 phandlepart, 2))
> > {
> > +                               plat->phandlepart.phandle =
> > phandlepart[0];
> > +                               plat->phandlepart.partition =
> > phandlepart[1];
> > +                       }
> > +
> > +                       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 = "u-boot,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..0be4f17
> > --- /dev/null
> > +++ b/include/fs_loader.h
> > @@ -0,0 +1,79 @@
> > +/*
> > + * 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 - A place for storing firmware and its
> > attribute data.
> > + *
> > + * This holds information about a firmware and its content.
> > + *
> > + * @size: Size of a file
> > + * @data: Buffer for file
> > + * @priv: Firmware loader private fields
> > + */
> > +struct firmware {
> > +       size_t size;
> > +       const u8 *data;
> > +       void *priv;
> > +};
> Let's try to get rid of priv by using driver model.
Okay.
> 
> > 
> > +
> > +/**
> > + * struct phandle_part - A place for storing phandle of node and
> > its partition
> > + *
> > + * This holds information about a phandle of the block device, and
> > its
> > + * partition where the firmware would be loaded from.
> > + *
> > + * @phandle: Phandle of storage device node
> > + * @partition: Partition of block device
> > + */
> > +struct phandle_part {
> struct device_part ?
> 
> A phandle is a device-tree concept.
Okay.
> 
> > 
> > +       u32 phandle;
> > +       u32 partition;
> > +};
> > +
> > +/**
> > + * struct phandle_part - A place for storing all supported storage
> > devices
> > + *
> > + * This holds information about all supported storage devices for
> > driver use.
> > + *
> > + * @phandlepart: Attribute data for block device.
> nit: please drop your periods at the end of these lines - they are
> not
> sentences.
> 
Okay.
> > 
> > + * @mtdpart: MTD partition for ubi partition.
> > + * @ubivol: UBI volume-name for ubifsmount.
> > + */
> > +struct device_platdata {
> > +       struct phandle_part phandlepart;
> > +       char *mtdpart;
> > +       char *ubivol;
> > +};
> > +
> > +/**
> > + * release_firmware - Release the resource associated with a
> > firmware image
> > + * @firmware: Firmware resource to release
> > + */
> > +void release_firmware(struct firmware *firmware);
> > +
> > +/**
> > + * request_firmware_into_buf - Load firmware into a previously
> > allocated buffer.
> > + * @plat: Platform data such as storage and partition firmware
> > loading from.
> > + * @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.
> > + * @firmwarep: Pointer to firmware image.
> > + *
> > + * The firmware is loaded directly into the buffer pointed to by
> > @buf and
> > + * the @firmwarep data member is pointed at @buf.
> > + *
> > + * Return: Size of total read, negative value when error.
> > + */
> > +int request_firmware_into_buf(struct device_platdata *plat,
> > +                             const char *name,
> > +                             void *buf, size_t size, u32 offset,
> > +                             struct firmware **firmwarep);
> Without a test it's hard to see how this is used, but I'm not keen on
> the struct firmware ** here.
> 
> Can you just use struct firmware * instead?
> 
> Or maybe just return the fields individually since you only have
> start
> address and size, I think.
I can try to remove struct firmware, let driver model handles all
memory allocation, and then struct device_platdata *plat will change
to struct udevice *dev. So, it would become like this
int request_firmware_into_buf(struct udevice *dev,
			      const char *name,
			      void *buf, size_t size, u32 offset);
I will remove release_firmware() after letting driver model to handle
all memory deallocation.
So, the API interface would looks a bit different compare to Linux
firmware API interface. Does this change OK for you?
> 
> > 
> > +#endif
> > --
> > 2.2.0
> > 
> Regards,
> Simon