Hi, On 25 June 2018 at 07:28, <tien.fong.c...@intel.com> wrote: > From: Tien Fong Chee <tien.fong.c...@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.c...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot