Hi Tien Fong,
On 27 June 2018 at 01:41, Chee, Tien Fong <tien.fong.c...@intel.com> wrote:
> On Mon, 2018-06-25 at 21:58 -0600, Simon Glass wrote:
>> 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?
>>
> 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.
See my other email on this.
>> >
>> > + } 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.
OK, but in that case why use environment? Can't you just put the
parameters in the command?
[..]
Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
