[U-Boot] [PATCH v4 6/6] common: Generic loader for file system
Chee, Tien Fong
tien.fong.chee at intel.com
Thu Jul 12 07:19:02 UTC 2018
On Wed, 2018-07-11 at 14:13 -0600, Simon Glass wrote:
> Hi Tien,
>
> On 6 July 2018 at 02: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 | 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
More information about the U-Boot
mailing list