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

Simon Glass sjg at chromium.org
Tue Jun 26 03:58:51 UTC 2018


Hi,

On 25 June 2018 at 07:28,  <tien.fong.chee at intel.com> wrote:
> From: Tien Fong Chee <tien.fong.chee at 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 at 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?

>
> 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.

> +       } 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?

> +                                   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/

> +{
> +       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?

> +
> +       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)

> +                    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?

> +#endif
> --
> 2.2.0
>

Regards,
Simon


More information about the U-Boot mailing list