* [U-Boot] [PATCH v4 6/6] common: Generic loader for file system
@ 2018-07-06 8:28 tien.fong.chee at intel.com
2018-07-11 14:02 ` Simon Glass
` (3 more replies)
0 siblings, 4 replies; 29+ messages in thread
From: tien.fong.chee at intel.com @ 2018-07-06 8:28 UTC (permalink / raw)
To: u-boot
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.
+
+ 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..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 */
+};
+
+#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");
+ 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);
+ 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)
+{
+ if (!name || name[0] == '\0')
+ return -EINVAL;
+
+ /* No memory allocation is required if *firmwarep is allocated */
+ if (!(*firmwarep)) {
+ (*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) {
+ (*firmwarep)->priv = calloc(1, sizeof(struct firmware_priv));
+ if (!(*firmwarep)->priv) {
+ free(*firmwarep);
+ return -ENOMEM;
+ }
+ }
+
+ ((struct firmware_priv *)((*firmwarep)->priv))->name = name;
+ ((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
+ 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;
+};
+
+/**
+ * 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 {
+ 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.
+ * @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);
+#endif
--
2.2.0
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [U-Boot] [PATCH v4 6/6] common: Generic loader for file system
2018-07-06 8:28 [U-Boot] [PATCH v4 6/6] common: Generic loader for file system tien.fong.chee at intel.com
@ 2018-07-11 14:02 ` Simon Glass
2018-07-11 14:20 ` Chee, Tien Fong
2018-07-11 20:13 ` Simon Glass
` (2 subsequent siblings)
3 siblings, 1 reply; 29+ messages in thread
From: Simon Glass @ 2018-07-11 14:02 UTC (permalink / raw)
To: u-boot
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
I don't see a sandbox test for this. We need to add a test for every
new uclass. Please let me know if you want some pointers to ideas.
[..]
Regards,
Simon
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [PATCH v4 6/6] common: Generic loader for file system
2018-07-11 14:02 ` Simon Glass
@ 2018-07-11 14:20 ` Chee, Tien Fong
0 siblings, 0 replies; 29+ messages in thread
From: Chee, Tien Fong @ 2018-07-11 14:20 UTC (permalink / raw)
To: u-boot
On Wed, 2018-07-11 at 08:02 -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
> I don't see a sandbox test for this. We need to add a test for every
> new uclass. Please let me know if you want some pointers to ideas.
>
Yeah, i will add the sandbox for this once we finalize the design.
Sandbox is new to me, i would appreciate it very much if you can share
some ideas.
> [..]
>
> Regards,
> Simon
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [PATCH v4 6/6] common: Generic loader for file system
2018-07-06 8:28 [U-Boot] [PATCH v4 6/6] common: Generic loader for file system tien.fong.chee at intel.com
2018-07-11 14:02 ` Simon Glass
@ 2018-07-11 20:13 ` Simon Glass
2018-07-12 7:19 ` Chee, Tien Fong
2018-07-18 14:48 ` Michal Simek
2018-09-29 15:43 ` [U-Boot] [U-Boot, v4, " Tom Rini
3 siblings, 1 reply; 29+ messages in thread
From: Simon Glass @ 2018-07-11 20:13 UTC (permalink / raw)
To: u-boot
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?
> +
> + The consumer driver would then use this loader to program whatever,
> + ie. the FPGA device.
e.g. an 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..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?
> +};
> +
> +#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
> + 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.
> + 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
> + 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?
> + (*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?
> + (*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.
> + ((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
> + 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.
> +
> +/**
> + * 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.
> + 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.
> + * @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.
> +#endif
> --
> 2.2.0
>
Regards,
Simon
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [PATCH v4 6/6] common: Generic loader for file system
2018-07-11 20:13 ` Simon Glass
@ 2018-07-12 7:19 ` Chee, Tien Fong
2018-07-15 21:21 ` Simon Glass
0 siblings, 1 reply; 29+ messages in thread
From: Chee, Tien Fong @ 2018-07-12 7:19 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [PATCH v4 6/6] common: Generic loader for file system
2018-07-12 7:19 ` Chee, Tien Fong
@ 2018-07-15 21:21 ` Simon Glass
2018-07-17 4:46 ` Chee, Tien Fong
0 siblings, 1 reply; 29+ messages in thread
From: Simon Glass @ 2018-07-15 21:21 UTC (permalink / raw)
To: u-boot
Hi Tien Fong,
On 12 July 2018 at 01:19, Chee, Tien Fong <tien.fong.chee@intel.com> wrote:
> 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
>> >
[..]
>>
>> >
>> > + (*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?
Because these casts are really ugly and repetitive, particularly when
used for assignments :-)
[..]
>> > + * 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?
I believe you would need:
> int request_firmware_into_buf(struct udevice *dev,
> const char *name,
> void **bufp, size_t *sizep, u32 offset);
since you need to return the buffer and size?
What exactly is 'dev'? Is it the device in the FS_LOADER uclass?
Regards,
Simon
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [PATCH v4 6/6] common: Generic loader for file system
2018-07-15 21:21 ` Simon Glass
@ 2018-07-17 4:46 ` Chee, Tien Fong
0 siblings, 0 replies; 29+ messages in thread
From: Chee, Tien Fong @ 2018-07-17 4:46 UTC (permalink / raw)
To: u-boot
On Sun, 2018-07-15 at 15:21 -0600, Simon Glass wrote:
> Hi Tien Fong,
>
> On 12 July 2018 at 01:19, Chee, Tien Fong <tien.fong.chee@intel.com>
> wrote:
> >
> > 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
> > > >
> [..]
>
> >
> > >
> > >
> > > >
> > > >
> > > > + (*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?
> Because these casts are really ugly and repetitive, particularly when
> used for assignments :-)
Okay.
>
> [..]
>
> >
> > >
> > > >
> > > > + * 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?
> I believe you would need:
>
> >
> > int request_firmware_into_buf(struct udevice *dev,
> > const char *name,
> > void **bufp, size_t *sizep, u32
> > offset);
> since you need to return the buffer and size?
Why need to return the buffer and size? There is no modification
required on noth buffer and size arguments by
request_firmware_into_buf().
Both buffer and size are allocated and defined by user, then
request_firmware_into_buf() would load the file into buffer based on
the size exactly defined by user.
>
> What exactly is 'dev'? Is it the device in the FS_LOADER uclass?
Yes, contains FDT HW data.
>
> Regards,
> Simon
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [PATCH v4 6/6] common: Generic loader for file system
2018-07-06 8:28 [U-Boot] [PATCH v4 6/6] common: Generic loader for file system tien.fong.chee at intel.com
2018-07-11 14:02 ` Simon Glass
2018-07-11 20:13 ` Simon Glass
@ 2018-07-18 14:48 ` Michal Simek
2018-07-25 6:31 ` Chee, Tien Fong
2018-09-29 15:43 ` [U-Boot] [U-Boot, v4, " Tom Rini
3 siblings, 1 reply; 29+ messages in thread
From: Michal Simek @ 2018-07-18 14:48 UTC (permalink / raw)
To: u-boot
On 6.7.2018 10:28, tien.fong.chee at 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.
> +
> + 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..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 */
> +};
> +
> +#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");
> + 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);
> + 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);
I am curious why it is in generic FS loader any code which target any
filesystem. It should be filesystem independent.
Also that DT binding is quite weird and I don't think you will get ACK
for this from device tree community at all. I think that calling via
platdata and avoid DT nodes would be better way to go.
Also I can't see any usage of firmware_loader from chosen which you have
in 4/6.
Also based on discussion with Marek I am not quite sure if this can be
used for his SPL FPGA A10 bitstream loading from FS by chuck from FIT
image as he mentioned.
And last but not least I am missing user of this loader. I think it will
be the best to also send a user of this to see how exactly this will be
called and used.
Thanks,
Michal
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [PATCH v4 6/6] common: Generic loader for file system
2018-07-18 14:48 ` Michal Simek
@ 2018-07-25 6:31 ` Chee, Tien Fong
2018-07-25 9:48 ` Michal Simek
0 siblings, 1 reply; 29+ messages in thread
From: Chee, Tien Fong @ 2018-07-25 6:31 UTC (permalink / raw)
To: u-boot
On Wed, 2018-07-18 at 16:48 +0200, Michal Simek wrote:
> On 6.7.2018 10:28, tien.fong.chee at 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.
> > +
> > + 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..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 */
> > +};
> > +
> > +#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");
> > + 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);
> > + 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);
> I am curious why it is in generic FS loader any code which target any
> filesystem. It should be filesystem independent.
Because it supports our use case, and our preference using file system
instead of RAW. As I agree with Tom, it can be evolved to support RAW
in future.
>
> Also that DT binding is quite weird and I don't think you will get
> ACK
> for this from device tree community at all. I think that calling via
> platdata and avoid DT nodes would be better way to go.
Why do you think DT binding is weird? The DT is designed based on Simon
proposal, and i believe following the rules in DTS spec.
There are some DT benefits with current design, i think someone may be
maintainer need to made the final decision on the design.
>
> Also I can't see any usage of firmware_loader from chosen which you
> have
> in 4/6.
https://patchwork.ozlabs.org/patch/940319/ This contains some
explanation about chosen, default storage defined by user in DTS.
fs_loader_ofdata_to platdata() is used to parse the configuration in
chosen.
>
> Also based on discussion with Marek I am not quite sure if this can
> be
> used for his SPL FPGA A10 bitstream loading from FS by chuck from FIT
> image as he mentioned.
This design is currently not support RAW, but i think it can be
enhanced in future. I'm still prefer to put the FPGA design in
filesystem instead of RAW, thinking about the filesystem benefits.
>
> And last but not least I am missing user of this loader. I think it
> will
> be the best to also send a user of this to see how exactly this will
> be
> called and used.
https://patchwork.ozlabs.org/patch/940319/ , did you read the doc?
I have setting up a test env for this design which is using FPGA
manager to call this loader for chunk by chunk transfering the FPGA
design from storage into FPGA manager. It would be a bit messy and
complicated to understand from the codes but it would be helpful to
understand the use of this loader. Do you want it?
>
> Thanks,
> Michal
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [PATCH v4 6/6] common: Generic loader for file system
2018-07-25 6:31 ` Chee, Tien Fong
@ 2018-07-25 9:48 ` Michal Simek
2018-07-25 15:47 ` Simon Glass
2018-07-26 9:23 ` Chee, Tien Fong
0 siblings, 2 replies; 29+ messages in thread
From: Michal Simek @ 2018-07-25 9:48 UTC (permalink / raw)
To: u-boot
On 25.7.2018 08:31, Chee, Tien Fong wrote:
> On Wed, 2018-07-18 at 16:48 +0200, Michal Simek wrote:
>> On 6.7.2018 10:28, tien.fong.chee at 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.
>>> +
>>> + 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..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 */
>>> +};
>>> +
>>> +#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");
>>> + 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);
>>> + 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);
>> I am curious why it is in generic FS loader any code which target any
>> filesystem. It should be filesystem independent.
> Because it supports our use case, and our preference using file system
> instead of RAW. As I agree with Tom, it can be evolved to support RAW
> in future.
It is not a problem that you have decided to support filesystems at
first place. I don't understand why you have UBIFS specific code here.
I would expect that this will done as CMD_FS_GENERIC option which is
based on code also able to work with UBIFS. It means this code will call
generic function and based on FS type proper functions will be chosen.
I didn't work with UBIFS but it is supported in fs.c that's why I would
expect that this shouldn't be a problem to get it work.
>>
>> Also that DT binding is quite weird and I don't think you will get
>> ACK
>> for this from device tree community at all. I think that calling via
>> platdata and avoid DT nodes would be better way to go.
> Why do you think DT binding is weird? The DT is designed based on Simon
> proposal, and i believe following the rules in DTS spec.
> There are some DT benefits with current design, i think someone may be
> maintainer need to made the final decision on the design.
It is software configuration in file which should mostly describe
hardware and state for hardware configuration.
Your fs_loader node is purely describe sw configuration which shouldn't
be here.
You have there run time configuration via variables. I think using only
this way is enough. Default variables will match what you would want to
add to DT.
>>
>> Also I can't see any usage of firmware_loader from chosen which you
>> have
>> in 4/6.
> https://patchwork.ozlabs.org/patch/940319/ This contains some
> explanation about chosen, default storage defined by user in DTS.
> fs_loader_ofdata_to platdata() is used to parse the configuration in
> chosen.
>>
>> Also based on discussion with Marek I am not quite sure if this can
>> be
>> used for his SPL FPGA A10 bitstream loading from FS by chuck from FIT
>> image as he mentioned.
> This design is currently not support RAW, but i think it can be
> enhanced in future. I'm still prefer to put the FPGA design in
> filesystem instead of RAW, thinking about the filesystem benefits.
I have no problem that you support filesystem first.
>>
>> And last but not least I am missing user of this loader. I think it
>> will
>> be the best to also send a user of this to see how exactly this will
>> be
>> called and used.
> https://patchwork.ozlabs.org/patch/940319/ , did you read the doc?
> I have setting up a test env for this design which is using FPGA
> manager to call this loader for chunk by chunk transfering the FPGA
> design from storage into FPGA manager. It would be a bit messy and
> complicated to understand from the codes but it would be helpful to
> understand the use of this loader. Do you want it?
Yes please send this out to see how exactly you want to use this. We
have done this in zynq_loadfs by using generic fs_read functions.
Feel free to also check this series which is cleaning up fpga command.
https://lists.denx.de/pipermail/u-boot/2018-July/335193.html
Thanks,
Michal
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [PATCH v4 6/6] common: Generic loader for file system
2018-07-25 9:48 ` Michal Simek
@ 2018-07-25 15:47 ` Simon Glass
2018-07-25 16:03 ` Tom Rini
2018-07-26 9:23 ` Chee, Tien Fong
1 sibling, 1 reply; 29+ messages in thread
From: Simon Glass @ 2018-07-25 15:47 UTC (permalink / raw)
To: u-boot
Hi,
On 25 July 2018 at 03:48, Michal Simek <michal.simek@xilinx.com> wrote:
>
> On 25.7.2018 08:31, Chee, Tien Fong wrote:
> > On Wed, 2018-07-18 at 16:48 +0200, Michal Simek wrote:
> >> On 6.7.2018 10:28, tien.fong.chee at intel.com wrote:
> >>>
> >>> From: Tien Fong Chee <tien.fong.chee@intel.com>
> >>>
[...]
> >>
> >> Also that DT binding is quite weird and I don't think you will get
> >> ACK
> >> for this from device tree community at all. I think that calling via
> >> platdata and avoid DT nodes would be better way to go.
> > Why do you think DT binding is weird? The DT is designed based on Simon
> > proposal, and i believe following the rules in DTS spec.
> > There are some DT benefits with current design, i think someone may be
> > maintainer need to made the final decision on the design.
>
> It is software configuration in file which should mostly describe
> hardware and state for hardware configuration.
>
> Your fs_loader node is purely describe sw configuration which shouldn't
> be here.
> You have there run time configuration via variables. I think using only
> this way is enough. Default variables will match what you would want to
> add to DT.
I think DT makes sense in the U-Boot context.
We don't have a user space to handle policy decisions, and the
'chosen' node is a good place to configure these common features.
While you can argue that the partition or filesystem where an image
comes from is a software config, it is something that has to be
configured. It has impact on hardware too, since the FPGA has to get
its firmware from somewhere. We use the chosen node to specify the
UART to use, and this is no different. Again, we don't have user-space
config files in U-Boot.
This argument comes up from time to time and I'd really like to put it
to bed for U-Boot. I understand that Linux has its own approach and
rules, but in some cases they serve U-Boot poorly.
Regards,
Simon
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [PATCH v4 6/6] common: Generic loader for file system
2018-07-25 15:47 ` Simon Glass
@ 2018-07-25 16:03 ` Tom Rini
2018-07-26 9:03 ` Michal Simek
0 siblings, 1 reply; 29+ messages in thread
From: Tom Rini @ 2018-07-25 16:03 UTC (permalink / raw)
To: u-boot
On Wed, Jul 25, 2018 at 09:47:17AM -0600, Simon Glass wrote:
> Hi,
>
> On 25 July 2018 at 03:48, Michal Simek <michal.simek@xilinx.com> wrote:
> >
> > On 25.7.2018 08:31, Chee, Tien Fong wrote:
> > > On Wed, 2018-07-18 at 16:48 +0200, Michal Simek wrote:
> > >> On 6.7.2018 10:28, tien.fong.chee at intel.com wrote:
> > >>>
> > >>> From: Tien Fong Chee <tien.fong.chee@intel.com>
> > >>>
> [...]
>
> > >>
> > >> Also that DT binding is quite weird and I don't think you will get
> > >> ACK
> > >> for this from device tree community at all. I think that calling via
> > >> platdata and avoid DT nodes would be better way to go.
> > > Why do you think DT binding is weird? The DT is designed based on Simon
> > > proposal, and i believe following the rules in DTS spec.
> > > There are some DT benefits with current design, i think someone may be
> > > maintainer need to made the final decision on the design.
> >
> > It is software configuration in file which should mostly describe
> > hardware and state for hardware configuration.
> >
> > Your fs_loader node is purely describe sw configuration which shouldn't
> > be here.
> > You have there run time configuration via variables. I think using only
> > this way is enough. Default variables will match what you would want to
> > add to DT.
>
> I think DT makes sense in the U-Boot context.
>
> We don't have a user space to handle policy decisions, and the
> 'chosen' node is a good place to configure these common features.
>
> While you can argue that the partition or filesystem where an image
> comes from is a software config, it is something that has to be
> configured. It has impact on hardware too, since the FPGA has to get
> its firmware from somewhere. We use the chosen node to specify the
> UART to use, and this is no different. Again, we don't have user-space
> config files in U-Boot.
>
> This argument comes up from time to time and I'd really like to put it
> to bed for U-Boot. I understand that Linux has its own approach and
> rules, but in some cases they serve U-Boot poorly.
I want to second this as well. So long as we're using our prefix and
we've thought through and discussed what we're trying to do here, it's
OK to do things that might not be accepted for Linux.
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180725/64974b81/attachment.sig>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [PATCH v4 6/6] common: Generic loader for file system
2018-07-25 16:03 ` Tom Rini
@ 2018-07-26 9:03 ` Michal Simek
2018-07-27 8:40 ` Chee, Tien Fong
0 siblings, 1 reply; 29+ messages in thread
From: Michal Simek @ 2018-07-26 9:03 UTC (permalink / raw)
To: u-boot
On 25.7.2018 18:03, Tom Rini wrote:
> On Wed, Jul 25, 2018 at 09:47:17AM -0600, Simon Glass wrote:
>> Hi,
>>
>> On 25 July 2018 at 03:48, Michal Simek <michal.simek@xilinx.com> wrote:
>>>
>>> On 25.7.2018 08:31, Chee, Tien Fong wrote:
>>>> On Wed, 2018-07-18 at 16:48 +0200, Michal Simek wrote:
>>>>> On 6.7.2018 10:28, tien.fong.chee at intel.com wrote:
>>>>>>
>>>>>> From: Tien Fong Chee <tien.fong.chee@intel.com>
>>>>>>
>> [...]
>>
>>>>>
>>>>> Also that DT binding is quite weird and I don't think you will get
>>>>> ACK
>>>>> for this from device tree community at all. I think that calling via
>>>>> platdata and avoid DT nodes would be better way to go.
>>>> Why do you think DT binding is weird? The DT is designed based on Simon
>>>> proposal, and i believe following the rules in DTS spec.
>>>> There are some DT benefits with current design, i think someone may be
>>>> maintainer need to made the final decision on the design.
>>>
>>> It is software configuration in file which should mostly describe
>>> hardware and state for hardware configuration.
>>>
>>> Your fs_loader node is purely describe sw configuration which shouldn't
>>> be here.
>>> You have there run time configuration via variables. I think using only
>>> this way is enough. Default variables will match what you would want to
>>> add to DT.
>>
>> I think DT makes sense in the U-Boot context.
>>
>> We don't have a user space to handle policy decisions, and the
>> 'chosen' node is a good place to configure these common features.
>>
>> While you can argue that the partition or filesystem where an image
>> comes from is a software config, it is something that has to be
>> configured. It has impact on hardware too, since the FPGA has to get
>> its firmware from somewhere. We use the chosen node to specify the
>> UART to use, and this is no different. Again, we don't have user-space
>> config files in U-Boot.
>>
>> This argument comes up from time to time and I'd really like to put it
>> to bed for U-Boot. I understand that Linux has its own approach and
>> rules, but in some cases they serve U-Boot poorly.
>
> I want to second this as well. So long as we're using our prefix and
> we've thought through and discussed what we're trying to do here, it's
> OK to do things that might not be accepted for Linux.
>
I have not a problem with using chosen node with u-boot prefix
properties and my colleague hopefully with finish work about moving
u-boot,dm-pre-reloc; to chosen node where it should be (because current
solution has also problem with ordering).
In this loader case doc is saying that you can rewrite it with variables
on the prompt (or via script).
For cases that you want to autodetect platform and pass/load correct dtb
which setup u-boot this can be problematic and using DT is could be
considered as easier for use.
In this case this is what was proposed:
+ fs_loader0: fs-loader at 0 {
+ u-boot,dm-pre-reloc;
+ compatible = "u-boot,fs-loader";
+ phandlepart = <&mmc 1>;
+ };
+ fs_loader1: fs-loader at 1 {
+ u-boot,dm-pre-reloc;
+ compatible = "u-boot,fs-loader";
+ mtdpart = "UBI",
+ ubivol = "ubi0";
+ };
u-boot,dm-pre-reloc; requires DM_FLAG_PRE_RELOC which is not setup for
this driver - it means this should be here.
compatible = "u-boot,fs-loader"; - bind and probe are empty that's why
this is only used for filling platdata but driver has no user that's why
this is unused till someone calls that functions.
phandlepart/mtdpart/ubivol is just for setup.
For the first case you can just use in chosen node:
u-boot,fs-loader = <&mmc 1>;
And for UBIfs. I have never played with that but I expect it shouldn't
be big problem to describe it differently too (something like)
u-boot,fs-loader = <0 ubi0>;
Then this driver/interface can stay in DT where it should stay. The only
thing is how this should be initialized because there is no compatible
string. But you can do that via platdata for platforms which want to use
this.
Thanks,
Michal
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [PATCH v4 6/6] common: Generic loader for file system
2018-07-25 9:48 ` Michal Simek
2018-07-25 15:47 ` Simon Glass
@ 2018-07-26 9:23 ` Chee, Tien Fong
2018-07-26 10:29 ` Michal Simek
1 sibling, 1 reply; 29+ messages in thread
From: Chee, Tien Fong @ 2018-07-26 9:23 UTC (permalink / raw)
To: u-boot
On Wed, 2018-07-25 at 11:48 +0200, Michal Simek wrote:
> On 25.7.2018 08:31, Chee, Tien Fong wrote:
> >
> > On Wed, 2018-07-18 at 16:48 +0200, Michal Simek wrote:
> > >
> > > On 6.7.2018 10:28, tien.fong.chee at 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.
> > > > +
> > > > + 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..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
> > > > */
> > > > +};
> > > > +
> > > > +#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");
> > > > + 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);
> > > > + 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);
> > > I am curious why it is in generic FS loader any code which target
> > > any
> > > filesystem. It should be filesystem independent.
> > Because it supports our use case, and our preference using file
> > system
> > instead of RAW. As I agree with Tom, it can be evolved to support
> > RAW
> > in future.
> It is not a problem that you have decided to support filesystems at
> first place. I don't understand why you have UBIFS specific code
> here.
>
> I would expect that this will done as CMD_FS_GENERIC option which is
> based on code also able to work with UBIFS. It means this code will
> call
> generic function and based on FS type proper functions will be
> chosen.
>
> I didn't work with UBIFS but it is supported in fs.c that's why I
> would
> expect that this shouldn't be a problem to get it work
If fs_set_blk_dev is called without mounting the ubi first, error would
be returned from blk_get_device_part_str().
Regards,
TF
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [PATCH v4 6/6] common: Generic loader for file system
2018-07-26 9:23 ` Chee, Tien Fong
@ 2018-07-26 10:29 ` Michal Simek
2018-07-27 8:18 ` Chee, Tien Fong
0 siblings, 1 reply; 29+ messages in thread
From: Michal Simek @ 2018-07-26 10:29 UTC (permalink / raw)
To: u-boot
On 26.7.2018 11:23, Chee, Tien Fong wrote:
> On Wed, 2018-07-25 at 11:48 +0200, Michal Simek wrote:
>> On 25.7.2018 08:31, Chee, Tien Fong wrote:
>>>
>>> On Wed, 2018-07-18 at 16:48 +0200, Michal Simek wrote:
>>>>
>>>> On 6.7.2018 10:28, tien.fong.chee at 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.
>>>>> +
>>>>> + 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..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
>>>>> */
>>>>> +};
>>>>> +
>>>>> +#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");
>>>>> + 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);
>>>>> + 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);
>>>> I am curious why it is in generic FS loader any code which target
>>>> any
>>>> filesystem. It should be filesystem independent.
>>> Because it supports our use case, and our preference using file
>>> system
>>> instead of RAW. As I agree with Tom, it can be evolved to support
>>> RAW
>>> in future.
>> It is not a problem that you have decided to support filesystems at
>> first place. I don't understand why you have UBIFS specific code
>> here.
>>
>> I would expect that this will done as CMD_FS_GENERIC option which is
>> based on code also able to work with UBIFS. It means this code will
>> call
>> generic function and based on FS type proper functions will be
>> chosen.
>>
>> I didn't work with UBIFS but it is supported in fs.c that's why I
>> would
>> expect that this shouldn't be a problem to get it work
> If fs_set_blk_dev is called without mounting the ubi first, error would
> be returned from blk_get_device_part_str().
I am not working with ubifs but is this proper error. Or is this just
bug in the code that this error is shown?
Thanks,
Michal
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [PATCH v4 6/6] common: Generic loader for file system
2018-07-26 10:29 ` Michal Simek
@ 2018-07-27 8:18 ` Chee, Tien Fong
0 siblings, 0 replies; 29+ messages in thread
From: Chee, Tien Fong @ 2018-07-27 8:18 UTC (permalink / raw)
To: u-boot
On Thu, 2018-07-26 at 12:29 +0200, Michal Simek wrote:
> On 26.7.2018 11:23, Chee, Tien Fong wrote:
> >
> > On Wed, 2018-07-25 at 11:48 +0200, Michal Simek wrote:
> > >
> > > On 25.7.2018 08:31, Chee, Tien Fong wrote:
> > > >
> > > >
> > > > On Wed, 2018-07-18 at 16:48 +0200, Michal Simek wrote:
> > > > >
> > > > >
> > > > > On 6.7.2018 10:28, tien.fong.chee at 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.
> > > > > > +
> > > > > > + 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..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
> > > > > > */
> > > > > > +};
> > > > > > +
> > > > > > +#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");
> > > > > > + 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);
> > > > > > + 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);
> > > > > I am curious why it is in generic FS loader any code which
> > > > > target
> > > > > any
> > > > > filesystem. It should be filesystem independent.
> > > > Because it supports our use case, and our preference using file
> > > > system
> > > > instead of RAW. As I agree with Tom, it can be evolved to
> > > > support
> > > > RAW
> > > > in future.
> > > It is not a problem that you have decided to support filesystems
> > > at
> > > first place. I don't understand why you have UBIFS specific code
> > > here.
> > >
> > > I would expect that this will done as CMD_FS_GENERIC option which
> > > is
> > > based on code also able to work with UBIFS. It means this code
> > > will
> > > call
> > > generic function and based on FS type proper functions will be
> > > chosen.
> > >
> > > I didn't work with UBIFS but it is supported in fs.c that's why I
> > > would
> > > expect that this shouldn't be a problem to get it work
> > If fs_set_blk_dev is called without mounting the ubi first, error
> > would
> > be returned from blk_get_device_part_str().
> I am not working with ubifs but is this proper error. Or is this just
> bug in the code that this error is shown?
This is proper error, mounting is required before running
fs_set_blk_dev(). Hence, both arguments mtd partition and ubi volume
are required for mounting.
>
> Thanks,
> Michal
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [PATCH v4 6/6] common: Generic loader for file system
2018-07-26 9:03 ` Michal Simek
@ 2018-07-27 8:40 ` Chee, Tien Fong
2018-07-30 7:07 ` Michal Simek
2018-07-30 13:26 ` Simon Glass
0 siblings, 2 replies; 29+ messages in thread
From: Chee, Tien Fong @ 2018-07-27 8:40 UTC (permalink / raw)
To: u-boot
On Thu, 2018-07-26 at 11:03 +0200, Michal Simek wrote:
> On 25.7.2018 18:03, Tom Rini wrote:
> >
> > On Wed, Jul 25, 2018 at 09:47:17AM -0600, Simon Glass wrote:
> > >
> > > Hi,
> > >
> > > On 25 July 2018 at 03:48, Michal Simek <michal.simek@xilinx.com>
> > > wrote:
> > > >
> > > >
> > > > On 25.7.2018 08:31, Chee, Tien Fong wrote:
> > > > >
> > > > > On Wed, 2018-07-18 at 16:48 +0200, Michal Simek wrote:
> > > > > >
> > > > > > On 6.7.2018 10:28, tien.fong.chee at intel.com wrote:
> > > > > > >
> > > > > > >
> > > > > > > From: Tien Fong Chee <tien.fong.chee@intel.com>
> > > > > > >
> > > [...]
> > >
> > > >
> > > > >
> > > > > >
> > > > > >
> > > > > > Also that DT binding is quite weird and I don't think you
> > > > > > will get
> > > > > > ACK
> > > > > > for this from device tree community at all. I think that
> > > > > > calling via
> > > > > > platdata and avoid DT nodes would be better way to go.
> > > > > Why do you think DT binding is weird? The DT is designed
> > > > > based on Simon
> > > > > proposal, and i believe following the rules in DTS spec.
> > > > > There are some DT benefits with current design, i think
> > > > > someone may be
> > > > > maintainer need to made the final decision on the design.
> > > > It is software configuration in file which should mostly
> > > > describe
> > > > hardware and state for hardware configuration.
> > > >
> > > > Your fs_loader node is purely describe sw configuration which
> > > > shouldn't
> > > > be here.
> > > > You have there run time configuration via variables. I think
> > > > using only
> > > > this way is enough. Default variables will match what you would
> > > > want to
> > > > add to DT.
> > > I think DT makes sense in the U-Boot context.
> > >
> > > We don't have a user space to handle policy decisions, and the
> > > 'chosen' node is a good place to configure these common features.
> > >
> > > While you can argue that the partition or filesystem where an
> > > image
> > > comes from is a software config, it is something that has to be
> > > configured. It has impact on hardware too, since the FPGA has to
> > > get
> > > its firmware from somewhere. We use the chosen node to specify
> > > the
> > > UART to use, and this is no different. Again, we don't have user-
> > > space
> > > config files in U-Boot.
> > >
> > > This argument comes up from time to time and I'd really like to
> > > put it
> > > to bed for U-Boot. I understand that Linux has its own approach
> > > and
> > > rules, but in some cases they serve U-Boot poorly.
> > I want to second this as well. So long as we're using our prefix
> > and
> > we've thought through and discussed what we're trying to do here,
> > it's
> > OK to do things that might not be accepted for Linux.
> >
> I have not a problem with using chosen node with u-boot prefix
> properties and my colleague hopefully with finish work about moving
> u-boot,dm-pre-reloc; to chosen node where it should be (because
> current
> solution has also problem with ordering).
>
> In this loader case doc is saying that you can rewrite it with
> variables
> on the prompt (or via script).
> For cases that you want to autodetect platform and pass/load correct
> dtb
> which setup u-boot this can be problematic and using DT is could be
> considered as easier for use.
>
> In this case this is what was proposed:
>
> + fs_loader0: fs-loader at 0 {
> + u-boot,dm-pre-reloc;
> + compatible = "u-boot,fs-loader";
> + phandlepart = <&mmc 1>;
> + };
>
> + fs_loader1: fs-loader at 1 {
> + u-boot,dm-pre-reloc;
> + compatible = "u-boot,fs-loader";
> + mtdpart = "UBI",
> + ubivol = "ubi0";
> + };
>
> u-boot,dm-pre-reloc; requires DM_FLAG_PRE_RELOC which is not setup
> for
> this driver - it means this should be here.
You are right, i missed this one. The intention of design enables user
to call any loader with default storage through the sequence number if
fs loader is not defined in chosen. For example, there is a case where
system loading the file from SDMMC, NAND and QSPI.
>
> compatible = "u-boot,fs-loader"; - bind and probe are empty that's
> why
> this is only used for filling platdata but driver has no user that's
> why
> this is unused till someone calls that functions.
>
> phandlepart/mtdpart/ubivol is just for setup.
There are some benefits with driver model:
1. Saving space, calling when need.
2. Handle memory allocation and deallocation automatically.
>
> For the first case you can just use in chosen node:
> u-boot,fs-loader = <&mmc 1>;
>
> And for UBIfs. I have never played with that but I expect it
> shouldn't
> be big problem to describe it differently too (something like)
> u-boot,fs-loader = <0 ubi0>;
Need consider description for UBIFS, using fs-loader seems not working
for UBIFS, since more arguments such as mtdpartition and mtd volume
need passing into driver. In order to avoid messing, fs_loader can act
the pointer to the chosen.
Anyway, i have no strong opinion with driver designed via platdata or
driver model if we can resolve the problem for UBIFS and maintainers
agree with it.
>
> Then this driver/interface can stay in DT where it should stay. The
> only
> thing is how this should be initialized because there is no
> compatible
> string. But you can do that via platdata for platforms which want to
> use
> this.
>
> Thanks,
> Michal
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [PATCH v4 6/6] common: Generic loader for file system
2018-07-27 8:40 ` Chee, Tien Fong
@ 2018-07-30 7:07 ` Michal Simek
2018-07-30 13:26 ` Simon Glass
1 sibling, 0 replies; 29+ messages in thread
From: Michal Simek @ 2018-07-30 7:07 UTC (permalink / raw)
To: u-boot
On 27.7.2018 10:40, Chee, Tien Fong wrote:
> On Thu, 2018-07-26 at 11:03 +0200, Michal Simek wrote:
>> On 25.7.2018 18:03, Tom Rini wrote:
>>>
>>> On Wed, Jul 25, 2018 at 09:47:17AM -0600, Simon Glass wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 25 July 2018 at 03:48, Michal Simek <michal.simek@xilinx.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> On 25.7.2018 08:31, Chee, Tien Fong wrote:
>>>>>>
>>>>>> On Wed, 2018-07-18 at 16:48 +0200, Michal Simek wrote:
>>>>>>>
>>>>>>> On 6.7.2018 10:28, tien.fong.chee at intel.com wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> From: Tien Fong Chee <tien.fong.chee@intel.com>
>>>>>>>>
>>>> [...]
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Also that DT binding is quite weird and I don't think you
>>>>>>> will get
>>>>>>> ACK
>>>>>>> for this from device tree community at all. I think that
>>>>>>> calling via
>>>>>>> platdata and avoid DT nodes would be better way to go.
>>>>>> Why do you think DT binding is weird? The DT is designed
>>>>>> based on Simon
>>>>>> proposal, and i believe following the rules in DTS spec.
>>>>>> There are some DT benefits with current design, i think
>>>>>> someone may be
>>>>>> maintainer need to made the final decision on the design.
>>>>> It is software configuration in file which should mostly
>>>>> describe
>>>>> hardware and state for hardware configuration.
>>>>>
>>>>> Your fs_loader node is purely describe sw configuration which
>>>>> shouldn't
>>>>> be here.
>>>>> You have there run time configuration via variables. I think
>>>>> using only
>>>>> this way is enough. Default variables will match what you would
>>>>> want to
>>>>> add to DT.
>>>> I think DT makes sense in the U-Boot context.
>>>>
>>>> We don't have a user space to handle policy decisions, and the
>>>> 'chosen' node is a good place to configure these common features.
>>>>
>>>> While you can argue that the partition or filesystem where an
>>>> image
>>>> comes from is a software config, it is something that has to be
>>>> configured. It has impact on hardware too, since the FPGA has to
>>>> get
>>>> its firmware from somewhere. We use the chosen node to specify
>>>> the
>>>> UART to use, and this is no different. Again, we don't have user-
>>>> space
>>>> config files in U-Boot.
>>>>
>>>> This argument comes up from time to time and I'd really like to
>>>> put it
>>>> to bed for U-Boot. I understand that Linux has its own approach
>>>> and
>>>> rules, but in some cases they serve U-Boot poorly.
>>> I want to second this as well. So long as we're using our prefix
>>> and
>>> we've thought through and discussed what we're trying to do here,
>>> it's
>>> OK to do things that might not be accepted for Linux.
>>>
>> I have not a problem with using chosen node with u-boot prefix
>> properties and my colleague hopefully with finish work about moving
>> u-boot,dm-pre-reloc; to chosen node where it should be (because
>> current
>> solution has also problem with ordering).
>>
>> In this loader case doc is saying that you can rewrite it with
>> variables
>> on the prompt (or via script).
>> For cases that you want to autodetect platform and pass/load correct
>> dtb
>> which setup u-boot this can be problematic and using DT is could be
>> considered as easier for use.
>>
>> In this case this is what was proposed:
>>
>> + fs_loader0: fs-loader at 0 {
>> + u-boot,dm-pre-reloc;
>> + compatible = "u-boot,fs-loader";
>> + phandlepart = <&mmc 1>;
>> + };
>>
>> + fs_loader1: fs-loader at 1 {
>> + u-boot,dm-pre-reloc;
>> + compatible = "u-boot,fs-loader";
>> + mtdpart = "UBI",
>> + ubivol = "ubi0";
>> + };
>>
>> u-boot,dm-pre-reloc; requires DM_FLAG_PRE_RELOC which is not setup
>> for
>> this driver - it means this should be here.
> You are right, i missed this one. The intention of design enables user
> to call any loader with default storage through the sequence number if
> fs loader is not defined in chosen. For example, there is a case where
> system loading the file from SDMMC, NAND and QSPI.
>>
>> compatible = "u-boot,fs-loader"; - bind and probe are empty that's
>> why
>> this is only used for filling platdata but driver has no user that's
>> why
>> this is unused till someone calls that functions.
>>
>> phandlepart/mtdpart/ubivol is just for setup.
> There are some benefits with driver model:
> 1. Saving space, calling when need.
> 2. Handle memory allocation and deallocation automatically.
>>
>> For the first case you can just use in chosen node:
>> u-boot,fs-loader = <&mmc 1>;
>>
>> And for UBIfs. I have never played with that but I expect it
>> shouldn't
>> be big problem to describe it differently too (something like)
>> u-boot,fs-loader = <0 ubi0>;
> Need consider description for UBIFS, using fs-loader seems not working
> for UBIFS, since more arguments such as mtdpartition and mtd volume
> need passing into driver. In order to avoid messing, fs_loader can act
> the pointer to the chosen.
>
> Anyway, i have no strong opinion with driver designed via platdata or
> driver model if we can resolve the problem for UBIFS and maintainers
> agree with it.
The driver is still using DM. It is only about way how driver is binded.
Take a look for example on board/hisilicon/hikey/hikey.c
Thanks,
Michal
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [PATCH v4 6/6] common: Generic loader for file system
2018-07-27 8:40 ` Chee, Tien Fong
2018-07-30 7:07 ` Michal Simek
@ 2018-07-30 13:26 ` Simon Glass
2018-07-30 13:30 ` Michal Simek
1 sibling, 1 reply; 29+ messages in thread
From: Simon Glass @ 2018-07-30 13:26 UTC (permalink / raw)
To: u-boot
Hi,
On 27 July 2018 at 02:40, Chee, Tien Fong <tien.fong.chee@intel.com> wrote:
>
> On Thu, 2018-07-26 at 11:03 +0200, Michal Simek wrote:
> > On 25.7.2018 18:03, Tom Rini wrote:
> > >
> > > On Wed, Jul 25, 2018 at 09:47:17AM -0600, Simon Glass wrote:
> > > >
> > > > Hi,
> > > >
> > > > On 25 July 2018 at 03:48, Michal Simek <michal.simek@xilinx.com>
> > > > wrote:
> > > > >
> > > > >
> > > > > On 25.7.2018 08:31, Chee, Tien Fong wrote:
> > > > > >
> > > > > > On Wed, 2018-07-18 at 16:48 +0200, Michal Simek wrote:
> > > > > > >
> > > > > > > On 6.7.2018 10:28, tien.fong.chee at intel.com wrote:
> > > > > > > >
> > > > > > > >
> > > > > > > > From: Tien Fong Chee <tien.fong.chee@intel.com>
> > > > > > > >
> > > > [...]
> > > >
> > > > >
> > > > > >
> > > > > > >
> > > > > > >
> > > > > > > Also that DT binding is quite weird and I don't think you
> > > > > > > will get
> > > > > > > ACK
> > > > > > > for this from device tree community at all. I think that
> > > > > > > calling via
> > > > > > > platdata and avoid DT nodes would be better way to go.
> > > > > > Why do you think DT binding is weird? The DT is designed
> > > > > > based on Simon
> > > > > > proposal, and i believe following the rules in DTS spec.
> > > > > > There are some DT benefits with current design, i think
> > > > > > someone may be
> > > > > > maintainer need to made the final decision on the design.
> > > > > It is software configuration in file which should mostly
> > > > > describe
> > > > > hardware and state for hardware configuration.
> > > > >
> > > > > Your fs_loader node is purely describe sw configuration which
> > > > > shouldn't
> > > > > be here.
> > > > > You have there run time configuration via variables. I think
> > > > > using only
> > > > > this way is enough. Default variables will match what you would
> > > > > want to
> > > > > add to DT.
> > > > I think DT makes sense in the U-Boot context.
> > > >
> > > > We don't have a user space to handle policy decisions, and the
> > > > 'chosen' node is a good place to configure these common features.
> > > >
> > > > While you can argue that the partition or filesystem where an
> > > > image
> > > > comes from is a software config, it is something that has to be
> > > > configured. It has impact on hardware too, since the FPGA has to
> > > > get
> > > > its firmware from somewhere. We use the chosen node to specify
> > > > the
> > > > UART to use, and this is no different. Again, we don't have user-
> > > > space
> > > > config files in U-Boot.
> > > >
> > > > This argument comes up from time to time and I'd really like to
> > > > put it
> > > > to bed for U-Boot. I understand that Linux has its own approach
> > > > and
> > > > rules, but in some cases they serve U-Boot poorly.
> > > I want to second this as well. So long as we're using our prefix
> > > and
> > > we've thought through and discussed what we're trying to do here,
> > > it's
> > > OK to do things that might not be accepted for Linux.
> > >
> > I have not a problem with using chosen node with u-boot prefix
> > properties and my colleague hopefully with finish work about moving
> > u-boot,dm-pre-reloc; to chosen node where it should be (because
> > current
> > solution has also problem with ordering).
> >
> > In this loader case doc is saying that you can rewrite it with
> > variables
> > on the prompt (or via script).
> > For cases that you want to autodetect platform and pass/load correct
> > dtb
> > which setup u-boot this can be problematic and using DT is could be
> > considered as easier for use.
> >
> > In this case this is what was proposed:
> >
> > + fs_loader0: fs-loader at 0 {
> > + u-boot,dm-pre-reloc;
> > + compatible = "u-boot,fs-loader";
> > + phandlepart = <&mmc 1>;
> > + };
> >
> > + fs_loader1: fs-loader at 1 {
> > + u-boot,dm-pre-reloc;
> > + compatible = "u-boot,fs-loader";
> > + mtdpart = "UBI",
> > + ubivol = "ubi0";
> > + };
> >
> > u-boot,dm-pre-reloc; requires DM_FLAG_PRE_RELOC which is not setup
> > for
> > this driver - it means this should be here.
> You are right, i missed this one. The intention of design enables user
> to call any loader with default storage through the sequence number if
> fs loader is not defined in chosen. For example, there is a case where
> system loading the file from SDMMC, NAND and QSPI.
> >
> > compatible = "u-boot,fs-loader"; - bind and probe are empty that's
> > why
> > this is only used for filling platdata but driver has no user that's
> > why
> > this is unused till someone calls that functions.
> >
> > phandlepart/mtdpart/ubivol is just for setup.
> There are some benefits with driver model:
> 1. Saving space, calling when need.
> 2. Handle memory allocation and deallocation automatically.
> >
> > For the first case you can just use in chosen node:
> > u-boot,fs-loader = <&mmc 1>;
> >
> > And for UBIfs. I have never played with that but I expect it
> > shouldn't
> > be big problem to describe it differently too (something like)
> > u-boot,fs-loader = <0 ubi0>;
> Need consider description for UBIFS, using fs-loader seems not working
> for UBIFS, since more arguments such as mtdpartition and mtd volume
> need passing into driver. In order to avoid messing, fs_loader can act
> the pointer to the chosen.
>
> Anyway, i have no strong opinion with driver designed via platdata or
> driver model if we can resolve the problem for UBIFS and maintainers
> agree with it.
> >
> > Then this driver/interface can stay in DT where it should stay. The
> > only
> > thing is how this should be initialized because there is no
> > compatible
> > string. But you can do that via platdata for platforms which want to
> > use
> > this.
We should add a compatible string then :-)
Regards,
Simon
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [PATCH v4 6/6] common: Generic loader for file system
2018-07-30 13:26 ` Simon Glass
@ 2018-07-30 13:30 ` Michal Simek
2018-07-30 16:05 ` Simon Glass
0 siblings, 1 reply; 29+ messages in thread
From: Michal Simek @ 2018-07-30 13:30 UTC (permalink / raw)
To: u-boot
On 30.7.2018 15:26, Simon Glass wrote:
> Hi,
>
> On 27 July 2018 at 02:40, Chee, Tien Fong <tien.fong.chee@intel.com> wrote:
>>
>> On Thu, 2018-07-26 at 11:03 +0200, Michal Simek wrote:
>>> On 25.7.2018 18:03, Tom Rini wrote:
>>>>
>>>> On Wed, Jul 25, 2018 at 09:47:17AM -0600, Simon Glass wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> On 25 July 2018 at 03:48, Michal Simek <michal.simek@xilinx.com>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> On 25.7.2018 08:31, Chee, Tien Fong wrote:
>>>>>>>
>>>>>>> On Wed, 2018-07-18 at 16:48 +0200, Michal Simek wrote:
>>>>>>>>
>>>>>>>> On 6.7.2018 10:28, tien.fong.chee at intel.com wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> From: Tien Fong Chee <tien.fong.chee@intel.com>
>>>>>>>>>
>>>>> [...]
>>>>>
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Also that DT binding is quite weird and I don't think you
>>>>>>>> will get
>>>>>>>> ACK
>>>>>>>> for this from device tree community at all. I think that
>>>>>>>> calling via
>>>>>>>> platdata and avoid DT nodes would be better way to go.
>>>>>>> Why do you think DT binding is weird? The DT is designed
>>>>>>> based on Simon
>>>>>>> proposal, and i believe following the rules in DTS spec.
>>>>>>> There are some DT benefits with current design, i think
>>>>>>> someone may be
>>>>>>> maintainer need to made the final decision on the design.
>>>>>> It is software configuration in file which should mostly
>>>>>> describe
>>>>>> hardware and state for hardware configuration.
>>>>>>
>>>>>> Your fs_loader node is purely describe sw configuration which
>>>>>> shouldn't
>>>>>> be here.
>>>>>> You have there run time configuration via variables. I think
>>>>>> using only
>>>>>> this way is enough. Default variables will match what you would
>>>>>> want to
>>>>>> add to DT.
>>>>> I think DT makes sense in the U-Boot context.
>>>>>
>>>>> We don't have a user space to handle policy decisions, and the
>>>>> 'chosen' node is a good place to configure these common features.
>>>>>
>>>>> While you can argue that the partition or filesystem where an
>>>>> image
>>>>> comes from is a software config, it is something that has to be
>>>>> configured. It has impact on hardware too, since the FPGA has to
>>>>> get
>>>>> its firmware from somewhere. We use the chosen node to specify
>>>>> the
>>>>> UART to use, and this is no different. Again, we don't have user-
>>>>> space
>>>>> config files in U-Boot.
>>>>>
>>>>> This argument comes up from time to time and I'd really like to
>>>>> put it
>>>>> to bed for U-Boot. I understand that Linux has its own approach
>>>>> and
>>>>> rules, but in some cases they serve U-Boot poorly.
>>>> I want to second this as well. So long as we're using our prefix
>>>> and
>>>> we've thought through and discussed what we're trying to do here,
>>>> it's
>>>> OK to do things that might not be accepted for Linux.
>>>>
>>> I have not a problem with using chosen node with u-boot prefix
>>> properties and my colleague hopefully with finish work about moving
>>> u-boot,dm-pre-reloc; to chosen node where it should be (because
>>> current
>>> solution has also problem with ordering).
>>>
>>> In this loader case doc is saying that you can rewrite it with
>>> variables
>>> on the prompt (or via script).
>>> For cases that you want to autodetect platform and pass/load correct
>>> dtb
>>> which setup u-boot this can be problematic and using DT is could be
>>> considered as easier for use.
>>>
>>> In this case this is what was proposed:
>>>
>>> + fs_loader0: fs-loader at 0 {
>>> + u-boot,dm-pre-reloc;
>>> + compatible = "u-boot,fs-loader";
>>> + phandlepart = <&mmc 1>;
>>> + };
>>>
>>> + fs_loader1: fs-loader at 1 {
>>> + u-boot,dm-pre-reloc;
>>> + compatible = "u-boot,fs-loader";
>>> + mtdpart = "UBI",
>>> + ubivol = "ubi0";
>>> + };
>>>
>>> u-boot,dm-pre-reloc; requires DM_FLAG_PRE_RELOC which is not setup
>>> for
>>> this driver - it means this should be here.
>> You are right, i missed this one. The intention of design enables user
>> to call any loader with default storage through the sequence number if
>> fs loader is not defined in chosen. For example, there is a case where
>> system loading the file from SDMMC, NAND and QSPI.
>>>
>>> compatible = "u-boot,fs-loader"; - bind and probe are empty that's
>>> why
>>> this is only used for filling platdata but driver has no user that's
>>> why
>>> this is unused till someone calls that functions.
>>>
>>> phandlepart/mtdpart/ubivol is just for setup.
>> There are some benefits with driver model:
>> 1. Saving space, calling when need.
>> 2. Handle memory allocation and deallocation automatically.
>>>
>>> For the first case you can just use in chosen node:
>>> u-boot,fs-loader = <&mmc 1>;
>>>
>>> And for UBIfs. I have never played with that but I expect it
>>> shouldn't
>>> be big problem to describe it differently too (something like)
>>> u-boot,fs-loader = <0 ubi0>;
>> Need consider description for UBIFS, using fs-loader seems not working
>> for UBIFS, since more arguments such as mtdpartition and mtd volume
>> need passing into driver. In order to avoid messing, fs_loader can act
>> the pointer to the chosen.
>>
>> Anyway, i have no strong opinion with driver designed via platdata or
>> driver model if we can resolve the problem for UBIFS and maintainers
>> agree with it.
>>>
>>> Then this driver/interface can stay in DT where it should stay. The
>>> only
>>> thing is how this should be initialized because there is no
>>> compatible
>>> string. But you can do that via platdata for platforms which want to
>>> use
>>> this.
>
> We should add a compatible string then :-)
Isn't driver name used in case of platdata initialization?
M
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [PATCH v4 6/6] common: Generic loader for file system
2018-07-30 13:30 ` Michal Simek
@ 2018-07-30 16:05 ` Simon Glass
2018-07-31 5:12 ` Chee, Tien Fong
2018-07-31 6:22 ` Michal Simek
0 siblings, 2 replies; 29+ messages in thread
From: Simon Glass @ 2018-07-30 16:05 UTC (permalink / raw)
To: u-boot
Hi Michal,
On 30 July 2018 at 07:30, Michal Simek <michal.simek@xilinx.com> wrote:
> On 30.7.2018 15:26, Simon Glass wrote:
>> Hi,
>>
>> On 27 July 2018 at 02:40, Chee, Tien Fong <tien.fong.chee@intel.com> wrote:
>>>
>>> On Thu, 2018-07-26 at 11:03 +0200, Michal Simek wrote:
>>>> On 25.7.2018 18:03, Tom Rini wrote:
>>>>>
>>>>> On Wed, Jul 25, 2018 at 09:47:17AM -0600, Simon Glass wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On 25 July 2018 at 03:48, Michal Simek <michal.simek@xilinx.com>
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 25.7.2018 08:31, Chee, Tien Fong wrote:
>>>>>>>>
>>>>>>>> On Wed, 2018-07-18 at 16:48 +0200, Michal Simek wrote:
>>>>>>>>>
>>>>>>>>> On 6.7.2018 10:28, tien.fong.chee at intel.com wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> From: Tien Fong Chee <tien.fong.chee@intel.com>
>>>>>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Also that DT binding is quite weird and I don't think you
>>>>>>>>> will get
>>>>>>>>> ACK
>>>>>>>>> for this from device tree community at all. I think that
>>>>>>>>> calling via
>>>>>>>>> platdata and avoid DT nodes would be better way to go.
>>>>>>>> Why do you think DT binding is weird? The DT is designed
>>>>>>>> based on Simon
>>>>>>>> proposal, and i believe following the rules in DTS spec.
>>>>>>>> There are some DT benefits with current design, i think
>>>>>>>> someone may be
>>>>>>>> maintainer need to made the final decision on the design.
>>>>>>> It is software configuration in file which should mostly
>>>>>>> describe
>>>>>>> hardware and state for hardware configuration.
>>>>>>>
>>>>>>> Your fs_loader node is purely describe sw configuration which
>>>>>>> shouldn't
>>>>>>> be here.
>>>>>>> You have there run time configuration via variables. I think
>>>>>>> using only
>>>>>>> this way is enough. Default variables will match what you would
>>>>>>> want to
>>>>>>> add to DT.
>>>>>> I think DT makes sense in the U-Boot context.
>>>>>>
>>>>>> We don't have a user space to handle policy decisions, and the
>>>>>> 'chosen' node is a good place to configure these common features.
>>>>>>
>>>>>> While you can argue that the partition or filesystem where an
>>>>>> image
>>>>>> comes from is a software config, it is something that has to be
>>>>>> configured. It has impact on hardware too, since the FPGA has to
>>>>>> get
>>>>>> its firmware from somewhere. We use the chosen node to specify
>>>>>> the
>>>>>> UART to use, and this is no different. Again, we don't have user-
>>>>>> space
>>>>>> config files in U-Boot.
>>>>>>
>>>>>> This argument comes up from time to time and I'd really like to
>>>>>> put it
>>>>>> to bed for U-Boot. I understand that Linux has its own approach
>>>>>> and
>>>>>> rules, but in some cases they serve U-Boot poorly.
>>>>> I want to second this as well. So long as we're using our prefix
>>>>> and
>>>>> we've thought through and discussed what we're trying to do here,
>>>>> it's
>>>>> OK to do things that might not be accepted for Linux.
>>>>>
>>>> I have not a problem with using chosen node with u-boot prefix
>>>> properties and my colleague hopefully with finish work about moving
>>>> u-boot,dm-pre-reloc; to chosen node where it should be (because
>>>> current
>>>> solution has also problem with ordering).
>>>>
>>>> In this loader case doc is saying that you can rewrite it with
>>>> variables
>>>> on the prompt (or via script).
>>>> For cases that you want to autodetect platform and pass/load correct
>>>> dtb
>>>> which setup u-boot this can be problematic and using DT is could be
>>>> considered as easier for use.
>>>>
>>>> In this case this is what was proposed:
>>>>
>>>> + fs_loader0: fs-loader at 0 {
>>>> + u-boot,dm-pre-reloc;
>>>> + compatible = "u-boot,fs-loader";
>>>> + phandlepart = <&mmc 1>;
>>>> + };
>>>>
>>>> + fs_loader1: fs-loader at 1 {
>>>> + u-boot,dm-pre-reloc;
>>>> + compatible = "u-boot,fs-loader";
>>>> + mtdpart = "UBI",
>>>> + ubivol = "ubi0";
>>>> + };
>>>>
>>>> u-boot,dm-pre-reloc; requires DM_FLAG_PRE_RELOC which is not setup
>>>> for
>>>> this driver - it means this should be here.
>>> You are right, i missed this one. The intention of design enables user
>>> to call any loader with default storage through the sequence number if
>>> fs loader is not defined in chosen. For example, there is a case where
>>> system loading the file from SDMMC, NAND and QSPI.
>>>>
>>>> compatible = "u-boot,fs-loader"; - bind and probe are empty that's
>>>> why
>>>> this is only used for filling platdata but driver has no user that's
>>>> why
>>>> this is unused till someone calls that functions.
>>>>
>>>> phandlepart/mtdpart/ubivol is just for setup.
>>> There are some benefits with driver model:
>>> 1. Saving space, calling when need.
>>> 2. Handle memory allocation and deallocation automatically.
>>>>
>>>> For the first case you can just use in chosen node:
>>>> u-boot,fs-loader = <&mmc 1>;
>>>>
>>>> And for UBIfs. I have never played with that but I expect it
>>>> shouldn't
>>>> be big problem to describe it differently too (something like)
>>>> u-boot,fs-loader = <0 ubi0>;
>>> Need consider description for UBIFS, using fs-loader seems not working
>>> for UBIFS, since more arguments such as mtdpartition and mtd volume
>>> need passing into driver. In order to avoid messing, fs_loader can act
>>> the pointer to the chosen.
>>>
>>> Anyway, i have no strong opinion with driver designed via platdata or
>>> driver model if we can resolve the problem for UBIFS and maintainers
>>> agree with it.
>>>>
>>>> Then this driver/interface can stay in DT where it should stay. The
>>>> only
>>>> thing is how this should be initialized because there is no
>>>> compatible
>>>> string. But you can do that via platdata for platforms which want to
>>>> use
>>>> this.
>>
>> We should add a compatible string then :-)
>
> Isn't driver name used in case of platdata initialization?
If the node is in /chosen and has a compatible string, it will be
bound automatically. Manually binding a device is really just a
fallback for particular situations (e.g. buses like PCI where we often
rely on probing to find out what is on the bus).
Regards,
Simon
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [PATCH v4 6/6] common: Generic loader for file system
2018-07-30 16:05 ` Simon Glass
@ 2018-07-31 5:12 ` Chee, Tien Fong
2018-07-31 6:22 ` Michal Simek
1 sibling, 0 replies; 29+ messages in thread
From: Chee, Tien Fong @ 2018-07-31 5:12 UTC (permalink / raw)
To: u-boot
On Mon, 2018-07-30 at 10:05 -0600, Simon Glass wrote:
> Hi Michal,
>
> On 30 July 2018 at 07:30, Michal Simek <michal.simek@xilinx.com>
> wrote:
> >
> > On 30.7.2018 15:26, Simon Glass wrote:
> > >
> > > Hi,
> > >
> > > On 27 July 2018 at 02:40, Chee, Tien Fong <tien.fong.chee@intel.c
> > > om> wrote:
> > > >
> > > >
> > > > On Thu, 2018-07-26 at 11:03 +0200, Michal Simek wrote:
> > > > >
> > > > > On 25.7.2018 18:03, Tom Rini wrote:
> > > > > >
> > > > > >
> > > > > > On Wed, Jul 25, 2018 at 09:47:17AM -0600, Simon Glass
> > > > > > wrote:
> > > > > > >
> > > > > > >
> > > > > > > Hi,
> > > > > > >
> > > > > > > On 25 July 2018 at 03:48, Michal Simek <michal.simek@xili
> > > > > > > nx.com>
> > > > > > > wrote:
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > On 25.7.2018 08:31, Chee, Tien Fong wrote:
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > On Wed, 2018-07-18 at 16:48 +0200, Michal Simek
> > > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > On 6.7.2018 10:28, tien.fong.chee at intel.com wrote:
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > From: Tien Fong Chee <tien.fong.chee@intel.com>
> > > > > > > > > > >
> > > > > > > [...]
> > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Also that DT binding is quite weird and I don't
> > > > > > > > > > think you
> > > > > > > > > > will get
> > > > > > > > > > ACK
> > > > > > > > > > for this from device tree community at all. I think
> > > > > > > > > > that
> > > > > > > > > > calling via
> > > > > > > > > > platdata and avoid DT nodes would be better way to
> > > > > > > > > > go.
> > > > > > > > > Why do you think DT binding is weird? The DT is
> > > > > > > > > designed
> > > > > > > > > based on Simon
> > > > > > > > > proposal, and i believe following the rules in DTS
> > > > > > > > > spec.
> > > > > > > > > There are some DT benefits with current design, i
> > > > > > > > > think
> > > > > > > > > someone may be
> > > > > > > > > maintainer need to made the final decision on the
> > > > > > > > > design.
> > > > > > > > It is software configuration in file which should
> > > > > > > > mostly
> > > > > > > > describe
> > > > > > > > hardware and state for hardware configuration.
> > > > > > > >
> > > > > > > > Your fs_loader node is purely describe sw configuration
> > > > > > > > which
> > > > > > > > shouldn't
> > > > > > > > be here.
> > > > > > > > You have there run time configuration via variables. I
> > > > > > > > think
> > > > > > > > using only
> > > > > > > > this way is enough. Default variables will match what
> > > > > > > > you would
> > > > > > > > want to
> > > > > > > > add to DT.
> > > > > > > I think DT makes sense in the U-Boot context.
> > > > > > >
> > > > > > > We don't have a user space to handle policy decisions,
> > > > > > > and the
> > > > > > > 'chosen' node is a good place to configure these common
> > > > > > > features.
> > > > > > >
> > > > > > > While you can argue that the partition or filesystem
> > > > > > > where an
> > > > > > > image
> > > > > > > comes from is a software config, it is something that has
> > > > > > > to be
> > > > > > > configured. It has impact on hardware too, since the FPGA
> > > > > > > has to
> > > > > > > get
> > > > > > > its firmware from somewhere. We use the chosen node to
> > > > > > > specify
> > > > > > > the
> > > > > > > UART to use, and this is no different. Again, we don't
> > > > > > > have user-
> > > > > > > space
> > > > > > > config files in U-Boot.
> > > > > > >
> > > > > > > This argument comes up from time to time and I'd really
> > > > > > > like to
> > > > > > > put it
> > > > > > > to bed for U-Boot. I understand that Linux has its own
> > > > > > > approach
> > > > > > > and
> > > > > > > rules, but in some cases they serve U-Boot poorly.
> > > > > > I want to second this as well. So long as we're using our
> > > > > > prefix
> > > > > > and
> > > > > > we've thought through and discussed what we're trying to do
> > > > > > here,
> > > > > > it's
> > > > > > OK to do things that might not be accepted for Linux.
> > > > > >
> > > > > I have not a problem with using chosen node with u-boot
> > > > > prefix
> > > > > properties and my colleague hopefully with finish work about
> > > > > moving
> > > > > u-boot,dm-pre-reloc; to chosen node where it should be
> > > > > (because
> > > > > current
> > > > > solution has also problem with ordering).
> > > > >
> > > > > In this loader case doc is saying that you can rewrite it
> > > > > with
> > > > > variables
> > > > > on the prompt (or via script).
> > > > > For cases that you want to autodetect platform and pass/load
> > > > > correct
> > > > > dtb
> > > > > which setup u-boot this can be problematic and using DT is
> > > > > could be
> > > > > considered as easier for use.
> > > > >
> > > > > In this case this is what was proposed:
> > > > >
> > > > > + fs_loader0: fs-loader at 0 {
> > > > > + u-boot,dm-pre-reloc;
> > > > > + compatible = "u-boot,fs-loader";
> > > > > + phandlepart = <&mmc 1>;
> > > > > + };
> > > > >
> > > > > + fs_loader1: fs-loader at 1 {
> > > > > + u-boot,dm-pre-reloc;
> > > > > + compatible = "u-boot,fs-loader";
> > > > > + mtdpart = "UBI",
> > > > > + ubivol = "ubi0";
> > > > > + };
> > > > >
> > > > > u-boot,dm-pre-reloc; requires DM_FLAG_PRE_RELOC which is not
> > > > > setup
> > > > > for
> > > > > this driver - it means this should be here.
> > > > You are right, i missed this one. The intention of design
> > > > enables user
> > > > to call any loader with default storage through the sequence
> > > > number if
> > > > fs loader is not defined in chosen. For example, there is a
> > > > case where
> > > > system loading the file from SDMMC, NAND and QSPI.
> > > > >
> > > > >
> > > > > compatible = "u-boot,fs-loader"; - bind and probe are empty
> > > > > that's
> > > > > why
> > > > > this is only used for filling platdata but driver has no user
> > > > > that's
> > > > > why
> > > > > this is unused till someone calls that functions.
> > > > >
> > > > > phandlepart/mtdpart/ubivol is just for setup.
> > > > There are some benefits with driver model:
> > > > 1. Saving space, calling when need.
> > > > 2. Handle memory allocation and deallocation automatically.
> > > > >
> > > > >
> > > > > For the first case you can just use in chosen node:
> > > > > u-boot,fs-loader = <&mmc 1>;
> > > > >
> > > > > And for UBIfs. I have never played with that but I expect it
> > > > > shouldn't
> > > > > be big problem to describe it differently too (something
> > > > > like)
> > > > > u-boot,fs-loader = <0 ubi0>;
> > > > Need consider description for UBIFS, using fs-loader seems not
> > > > working
> > > > for UBIFS, since more arguments such as mtdpartition and mtd
> > > > volume
> > > > need passing into driver. In order to avoid messing, fs_loader
> > > > can act
> > > > the pointer to the chosen.
> > > >
> > > > Anyway, i have no strong opinion with driver designed via
> > > > platdata or
> > > > driver model if we can resolve the problem for UBIFS and
> > > > maintainers
> > > > agree with it.
> > > > >
> > > > >
> > > > > Then this driver/interface can stay in DT where it should
> > > > > stay. The
> > > > > only
> > > > > thing is how this should be initialized because there is no
> > > > > compatible
> > > > > string. But you can do that via platdata for platforms which
> > > > > want to
> > > > > use
> > > > > this.
> > > We should add a compatible string then :-)
> > Isn't driver name used in case of platdata initialization?
> If the node is in /chosen and has a compatible string, it will be
> bound automatically. Manually binding a device is really just a
> fallback for particular situations (e.g. buses like PCI where we
> often
> rely on probing to find out what is on the bus).
So, is this still the same with current implementation?
/ {
chosen {
firmware-loader = &fs_loader0;
};
fs_loader0: fs-loader at 0 {
u-boot,dm-pre-reloc;
compatible = "u-boot,fs-loader";
source-partition = <&mmc 1>;
};
};
>
> Regards,
> Simon
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [PATCH v4 6/6] common: Generic loader for file system
2018-07-30 16:05 ` Simon Glass
2018-07-31 5:12 ` Chee, Tien Fong
@ 2018-07-31 6:22 ` Michal Simek
2018-09-21 4:42 ` Chee, Tien Fong
1 sibling, 1 reply; 29+ messages in thread
From: Michal Simek @ 2018-07-31 6:22 UTC (permalink / raw)
To: u-boot
On 30.7.2018 18:05, Simon Glass wrote:
> Hi Michal,
>
> On 30 July 2018 at 07:30, Michal Simek <michal.simek@xilinx.com> wrote:
>> On 30.7.2018 15:26, Simon Glass wrote:
>>> Hi,
>>>
>>> On 27 July 2018 at 02:40, Chee, Tien Fong <tien.fong.chee@intel.com> wrote:
>>>>
>>>> On Thu, 2018-07-26 at 11:03 +0200, Michal Simek wrote:
>>>>> On 25.7.2018 18:03, Tom Rini wrote:
>>>>>>
>>>>>> On Wed, Jul 25, 2018 at 09:47:17AM -0600, Simon Glass wrote:
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 25 July 2018 at 03:48, Michal Simek <michal.simek@xilinx.com>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 25.7.2018 08:31, Chee, Tien Fong wrote:
>>>>>>>>>
>>>>>>>>> On Wed, 2018-07-18 at 16:48 +0200, Michal Simek wrote:
>>>>>>>>>>
>>>>>>>>>> On 6.7.2018 10:28, tien.fong.chee at intel.com wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> From: Tien Fong Chee <tien.fong.chee@intel.com>
>>>>>>>>>>>
>>>>>>> [...]
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Also that DT binding is quite weird and I don't think you
>>>>>>>>>> will get
>>>>>>>>>> ACK
>>>>>>>>>> for this from device tree community at all. I think that
>>>>>>>>>> calling via
>>>>>>>>>> platdata and avoid DT nodes would be better way to go.
>>>>>>>>> Why do you think DT binding is weird? The DT is designed
>>>>>>>>> based on Simon
>>>>>>>>> proposal, and i believe following the rules in DTS spec.
>>>>>>>>> There are some DT benefits with current design, i think
>>>>>>>>> someone may be
>>>>>>>>> maintainer need to made the final decision on the design.
>>>>>>>> It is software configuration in file which should mostly
>>>>>>>> describe
>>>>>>>> hardware and state for hardware configuration.
>>>>>>>>
>>>>>>>> Your fs_loader node is purely describe sw configuration which
>>>>>>>> shouldn't
>>>>>>>> be here.
>>>>>>>> You have there run time configuration via variables. I think
>>>>>>>> using only
>>>>>>>> this way is enough. Default variables will match what you would
>>>>>>>> want to
>>>>>>>> add to DT.
>>>>>>> I think DT makes sense in the U-Boot context.
>>>>>>>
>>>>>>> We don't have a user space to handle policy decisions, and the
>>>>>>> 'chosen' node is a good place to configure these common features.
>>>>>>>
>>>>>>> While you can argue that the partition or filesystem where an
>>>>>>> image
>>>>>>> comes from is a software config, it is something that has to be
>>>>>>> configured. It has impact on hardware too, since the FPGA has to
>>>>>>> get
>>>>>>> its firmware from somewhere. We use the chosen node to specify
>>>>>>> the
>>>>>>> UART to use, and this is no different. Again, we don't have user-
>>>>>>> space
>>>>>>> config files in U-Boot.
>>>>>>>
>>>>>>> This argument comes up from time to time and I'd really like to
>>>>>>> put it
>>>>>>> to bed for U-Boot. I understand that Linux has its own approach
>>>>>>> and
>>>>>>> rules, but in some cases they serve U-Boot poorly.
>>>>>> I want to second this as well. So long as we're using our prefix
>>>>>> and
>>>>>> we've thought through and discussed what we're trying to do here,
>>>>>> it's
>>>>>> OK to do things that might not be accepted for Linux.
>>>>>>
>>>>> I have not a problem with using chosen node with u-boot prefix
>>>>> properties and my colleague hopefully with finish work about moving
>>>>> u-boot,dm-pre-reloc; to chosen node where it should be (because
>>>>> current
>>>>> solution has also problem with ordering).
>>>>>
>>>>> In this loader case doc is saying that you can rewrite it with
>>>>> variables
>>>>> on the prompt (or via script).
>>>>> For cases that you want to autodetect platform and pass/load correct
>>>>> dtb
>>>>> which setup u-boot this can be problematic and using DT is could be
>>>>> considered as easier for use.
>>>>>
>>>>> In this case this is what was proposed:
>>>>>
>>>>> + fs_loader0: fs-loader at 0 {
>>>>> + u-boot,dm-pre-reloc;
>>>>> + compatible = "u-boot,fs-loader";
>>>>> + phandlepart = <&mmc 1>;
>>>>> + };
>>>>>
>>>>> + fs_loader1: fs-loader at 1 {
>>>>> + u-boot,dm-pre-reloc;
>>>>> + compatible = "u-boot,fs-loader";
>>>>> + mtdpart = "UBI",
>>>>> + ubivol = "ubi0";
>>>>> + };
>>>>>
>>>>> u-boot,dm-pre-reloc; requires DM_FLAG_PRE_RELOC which is not setup
>>>>> for
>>>>> this driver - it means this should be here.
>>>> You are right, i missed this one. The intention of design enables user
>>>> to call any loader with default storage through the sequence number if
>>>> fs loader is not defined in chosen. For example, there is a case where
>>>> system loading the file from SDMMC, NAND and QSPI.
>>>>>
>>>>> compatible = "u-boot,fs-loader"; - bind and probe are empty that's
>>>>> why
>>>>> this is only used for filling platdata but driver has no user that's
>>>>> why
>>>>> this is unused till someone calls that functions.
>>>>>
>>>>> phandlepart/mtdpart/ubivol is just for setup.
>>>> There are some benefits with driver model:
>>>> 1. Saving space, calling when need.
>>>> 2. Handle memory allocation and deallocation automatically.
>>>>>
>>>>> For the first case you can just use in chosen node:
>>>>> u-boot,fs-loader = <&mmc 1>;
>>>>>
>>>>> And for UBIfs. I have never played with that but I expect it
>>>>> shouldn't
>>>>> be big problem to describe it differently too (something like)
>>>>> u-boot,fs-loader = <0 ubi0>;
>>>> Need consider description for UBIFS, using fs-loader seems not working
>>>> for UBIFS, since more arguments such as mtdpartition and mtd volume
>>>> need passing into driver. In order to avoid messing, fs_loader can act
>>>> the pointer to the chosen.
>>>>
>>>> Anyway, i have no strong opinion with driver designed via platdata or
>>>> driver model if we can resolve the problem for UBIFS and maintainers
>>>> agree with it.
>>>>>
>>>>> Then this driver/interface can stay in DT where it should stay. The
>>>>> only
>>>>> thing is how this should be initialized because there is no
>>>>> compatible
>>>>> string. But you can do that via platdata for platforms which want to
>>>>> use
>>>>> this.
>>>
>>> We should add a compatible string then :-)
>>
>> Isn't driver name used in case of platdata initialization?
>
> If the node is in /chosen and has a compatible string, it will be
> bound automatically. Manually binding a device is really just a
> fallback for particular situations (e.g. buses like PCI where we often
> rely on probing to find out what is on the bus).
up2you guys. I just have different opinion how this should be done.
Thanks,
Michal
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [PATCH v4 6/6] common: Generic loader for file system
2018-07-31 6:22 ` Michal Simek
@ 2018-09-21 4:42 ` Chee, Tien Fong
2018-09-25 7:02 ` Chee, Tien Fong
0 siblings, 1 reply; 29+ messages in thread
From: Chee, Tien Fong @ 2018-09-21 4:42 UTC (permalink / raw)
To: u-boot
On Tue, 2018-07-31 at 08:22 +0200, Michal Simek wrote:
> On 30.7.2018 18:05, Simon Glass wrote:
> >
> > Hi Michal,
> >
> > On 30 July 2018 at 07:30, Michal Simek <michal.simek@xilinx.com>
> > wrote:
> > >
> > > On 30.7.2018 15:26, Simon Glass wrote:
> > > >
> > > > Hi,
> > > >
> > > > On 27 July 2018 at 02:40, Chee, Tien Fong <tien.fong.chee@intel
> > > > .com> wrote:
> > > > >
> > > > >
> > > > > On Thu, 2018-07-26 at 11:03 +0200, Michal Simek wrote:
> > > > > >
> > > > > > On 25.7.2018 18:03, Tom Rini wrote:
> > > > > > >
> > > > > > >
> > > > > > > On Wed, Jul 25, 2018 at 09:47:17AM -0600, Simon Glass
> > > > > > > wrote:
> > > > > > > >
> > > > > > > >
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > On 25 July 2018 at 03:48, Michal Simek <michal.simek@xi
> > > > > > > > linx.com>
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > On 25.7.2018 08:31, Chee, Tien Fong wrote:
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > On Wed, 2018-07-18 at 16:48 +0200, Michal Simek
> > > > > > > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > On 6.7.2018 10:28, tien.fong.chee at intel.com
> > > > > > > > > > > wrote:
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > From: Tien Fong Chee <tien.fong.chee@intel.com>
> > > > > > > > > > > >
> > > > > > > > [...]
> > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Also that DT binding is quite weird and I don't
> > > > > > > > > > > think you
> > > > > > > > > > > will get
> > > > > > > > > > > ACK
> > > > > > > > > > > for this from device tree community at all. I
> > > > > > > > > > > think that
> > > > > > > > > > > calling via
> > > > > > > > > > > platdata and avoid DT nodes would be better way
> > > > > > > > > > > to go.
> > > > > > > > > > Why do you think DT binding is weird? The DT is
> > > > > > > > > > designed
> > > > > > > > > > based on Simon
> > > > > > > > > > proposal, and i believe following the rules in DTS
> > > > > > > > > > spec.
> > > > > > > > > > There are some DT benefits with current design, i
> > > > > > > > > > think
> > > > > > > > > > someone may be
> > > > > > > > > > maintainer need to made the final decision on the
> > > > > > > > > > design.
> > > > > > > > > It is software configuration in file which should
> > > > > > > > > mostly
> > > > > > > > > describe
> > > > > > > > > hardware and state for hardware configuration.
> > > > > > > > >
> > > > > > > > > Your fs_loader node is purely describe sw
> > > > > > > > > configuration which
> > > > > > > > > shouldn't
> > > > > > > > > be here.
> > > > > > > > > You have there run time configuration via variables.
> > > > > > > > > I think
> > > > > > > > > using only
> > > > > > > > > this way is enough. Default variables will match what
> > > > > > > > > you would
> > > > > > > > > want to
> > > > > > > > > add to DT.
> > > > > > > > I think DT makes sense in the U-Boot context.
> > > > > > > >
> > > > > > > > We don't have a user space to handle policy decisions,
> > > > > > > > and the
> > > > > > > > 'chosen' node is a good place to configure these common
> > > > > > > > features.
> > > > > > > >
> > > > > > > > While you can argue that the partition or filesystem
> > > > > > > > where an
> > > > > > > > image
> > > > > > > > comes from is a software config, it is something that
> > > > > > > > has to be
> > > > > > > > configured. It has impact on hardware too, since the
> > > > > > > > FPGA has to
> > > > > > > > get
> > > > > > > > its firmware from somewhere. We use the chosen node to
> > > > > > > > specify
> > > > > > > > the
> > > > > > > > UART to use, and this is no different. Again, we don't
> > > > > > > > have user-
> > > > > > > > space
> > > > > > > > config files in U-Boot.
> > > > > > > >
> > > > > > > > This argument comes up from time to time and I'd really
> > > > > > > > like to
> > > > > > > > put it
> > > > > > > > to bed for U-Boot. I understand that Linux has its own
> > > > > > > > approach
> > > > > > > > and
> > > > > > > > rules, but in some cases they serve U-Boot poorly.
> > > > > > > I want to second this as well. So long as we're using
> > > > > > > our prefix
> > > > > > > and
> > > > > > > we've thought through and discussed what we're trying to
> > > > > > > do here,
> > > > > > > it's
> > > > > > > OK to do things that might not be accepted for Linux.
> > > > > > >
> > > > > > I have not a problem with using chosen node with u-boot
> > > > > > prefix
> > > > > > properties and my colleague hopefully with finish work
> > > > > > about moving
> > > > > > u-boot,dm-pre-reloc; to chosen node where it should be
> > > > > > (because
> > > > > > current
> > > > > > solution has also problem with ordering).
> > > > > >
> > > > > > In this loader case doc is saying that you can rewrite it
> > > > > > with
> > > > > > variables
> > > > > > on the prompt (or via script).
> > > > > > For cases that you want to autodetect platform and
> > > > > > pass/load correct
> > > > > > dtb
> > > > > > which setup u-boot this can be problematic and using DT is
> > > > > > could be
> > > > > > considered as easier for use.
> > > > > >
> > > > > > In this case this is what was proposed:
> > > > > >
> > > > > > + fs_loader0: fs-loader at 0 {
> > > > > > + u-boot,dm-pre-reloc;
> > > > > > + compatible = "u-boot,fs-loader";
> > > > > > + phandlepart = <&mmc 1>;
> > > > > > + };
> > > > > >
> > > > > > + fs_loader1: fs-loader at 1 {
> > > > > > + u-boot,dm-pre-reloc;
> > > > > > + compatible = "u-boot,fs-loader";
> > > > > > + mtdpart = "UBI",
> > > > > > + ubivol = "ubi0";
> > > > > > + };
> > > > > >
> > > > > > u-boot,dm-pre-reloc; requires DM_FLAG_PRE_RELOC which is
> > > > > > not setup
> > > > > > for
> > > > > > this driver - it means this should be here.
> > > > > You are right, i missed this one. The intention of design
> > > > > enables user
> > > > > to call any loader with default storage through the sequence
> > > > > number if
> > > > > fs loader is not defined in chosen. For example, there is a
> > > > > case where
> > > > > system loading the file from SDMMC, NAND and QSPI.
> > > > > >
> > > > > >
> > > > > > compatible = "u-boot,fs-loader"; - bind and probe are empty
> > > > > > that's
> > > > > > why
> > > > > > this is only used for filling platdata but driver has no
> > > > > > user that's
> > > > > > why
> > > > > > this is unused till someone calls that functions.
> > > > > >
> > > > > > phandlepart/mtdpart/ubivol is just for setup.
> > > > > There are some benefits with driver model:
> > > > > 1. Saving space, calling when need.
> > > > > 2. Handle memory allocation and deallocation automatically.
> > > > > >
> > > > > >
> > > > > > For the first case you can just use in chosen node:
> > > > > > u-boot,fs-loader = <&mmc 1>;
> > > > > >
> > > > > > And for UBIfs. I have never played with that but I expect
> > > > > > it
> > > > > > shouldn't
> > > > > > be big problem to describe it differently too (something
> > > > > > like)
> > > > > > u-boot,fs-loader = <0 ubi0>;
> > > > > Need consider description for UBIFS, using fs-loader seems
> > > > > not working
> > > > > for UBIFS, since more arguments such as mtdpartition and mtd
> > > > > volume
> > > > > need passing into driver. In order to avoid messing,
> > > > > fs_loader can act
> > > > > the pointer to the chosen.
> > > > >
> > > > > Anyway, i have no strong opinion with driver designed via
> > > > > platdata or
> > > > > driver model if we can resolve the problem for UBIFS and
> > > > > maintainers
> > > > > agree with it.
> > > > > >
> > > > > >
> > > > > > Then this driver/interface can stay in DT where it should
> > > > > > stay. The
> > > > > > only
> > > > > > thing is how this should be initialized because there is no
> > > > > > compatible
> > > > > > string. But you can do that via platdata for platforms
> > > > > > which want to
> > > > > > use
> > > > > > this.
> > > > We should add a compatible string then :-)
> > > Isn't driver name used in case of platdata initialization?
> > If the node is in /chosen and has a compatible string, it will be
> > bound automatically. Manually binding a device is really just a
> > fallback for particular situations (e.g. buses like PCI where we
> > often
> > rely on probing to find out what is on the bus).
> up2you guys. I just have different opinion how this should be done.
>
> Thanks,
> Michal
Sorry for late reply, because rushing the project.
So, are you guys OK with current implementation because we have no
solution for the UBIFS setting with other solution than using DT?
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [PATCH v4 6/6] common: Generic loader for file system
2018-09-21 4:42 ` Chee, Tien Fong
@ 2018-09-25 7:02 ` Chee, Tien Fong
2018-09-25 19:37 ` Tom Rini
2018-09-25 19:39 ` Simon Glass
0 siblings, 2 replies; 29+ messages in thread
From: Chee, Tien Fong @ 2018-09-25 7:02 UTC (permalink / raw)
To: u-boot
On Thu, 2018-09-20 at 21:42 -0700, Chee, Tien Fong wrote:
> >
If everybody agree with current framework, then the next version i will
include the fixes:
1. Adding DM_FLAG_PRE_RELOC, this would allow runtime to choose the
right fs_loader if the chosen node u-boot, fs-loader is not defined.
2. Let driver model handles all memory allocation
3. Using local variable instead of messy casts.
Thanks.
> On Tue, 2018-07-31 at 08:22 +0200, Michal Simek wrote:
> >
> > On 30.7.2018 18:05, Simon Glass wrote:
> > >
> > >
> > > Hi Michal,
> > >
> > > On 30 July 2018 at 07:30, Michal Simek <michal.simek@xilinx.com>
> > > wrote:
> > > >
> > > >
> > > > On 30.7.2018 15:26, Simon Glass wrote:
> > > > >
> > > > >
> > > > > Hi,
> > > > >
> > > > > On 27 July 2018 at 02:40, Chee, Tien Fong <tien.fong.chee@int
> > > > > el
> > > > > .com> wrote:
> > > > > >
> > > > > >
> > > > > >
> > > > > > On Thu, 2018-07-26 at 11:03 +0200, Michal Simek wrote:
> > > > > > >
> > > > > > >
> > > > > > > On 25.7.2018 18:03, Tom Rini wrote:
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > On Wed, Jul 25, 2018 at 09:47:17AM -0600, Simon Glass
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > Hi,
> > > > > > > > >
> > > > > > > > > On 25 July 2018 at 03:48, Michal Simek <michal.simek@
> > > > > > > > > xi
> > > > > > > > > linx.com>
> > > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > On 25.7.2018 08:31, Chee, Tien Fong wrote:
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > On Wed, 2018-07-18 at 16:48 +0200, Michal Simek
> > > > > > > > > > > wrote:
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > On 6.7.2018 10:28, tien.fong.chee at intel.com
> > > > > > > > > > > > wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > From: Tien Fong Chee <tien.fong.chee@intel.co
> > > > > > > > > > > > > m>
> > > > > > > > > > > > >
> > > > > > > > > [...]
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > Also that DT binding is quite weird and I don't
> > > > > > > > > > > > think you
> > > > > > > > > > > > will get
> > > > > > > > > > > > ACK
> > > > > > > > > > > > for this from device tree community at all. I
> > > > > > > > > > > > think that
> > > > > > > > > > > > calling via
> > > > > > > > > > > > platdata and avoid DT nodes would be better way
> > > > > > > > > > > > to go.
> > > > > > > > > > > Why do you think DT binding is weird? The DT is
> > > > > > > > > > > designed
> > > > > > > > > > > based on Simon
> > > > > > > > > > > proposal, and i believe following the rules in
> > > > > > > > > > > DTS
> > > > > > > > > > > spec.
> > > > > > > > > > > There are some DT benefits with current design, i
> > > > > > > > > > > think
> > > > > > > > > > > someone may be
> > > > > > > > > > > maintainer need to made the final decision on the
> > > > > > > > > > > design.
> > > > > > > > > > It is software configuration in file which should
> > > > > > > > > > mostly
> > > > > > > > > > describe
> > > > > > > > > > hardware and state for hardware configuration.
> > > > > > > > > >
> > > > > > > > > > Your fs_loader node is purely describe sw
> > > > > > > > > > configuration which
> > > > > > > > > > shouldn't
> > > > > > > > > > be here.
> > > > > > > > > > You have there run time configuration via
> > > > > > > > > > variables.
> > > > > > > > > > I think
> > > > > > > > > > using only
> > > > > > > > > > this way is enough. Default variables will match
> > > > > > > > > > what
> > > > > > > > > > you would
> > > > > > > > > > want to
> > > > > > > > > > add to DT.
> > > > > > > > > I think DT makes sense in the U-Boot context.
> > > > > > > > >
> > > > > > > > > We don't have a user space to handle policy
> > > > > > > > > decisions,
> > > > > > > > > and the
> > > > > > > > > 'chosen' node is a good place to configure these
> > > > > > > > > common
> > > > > > > > > features.
> > > > > > > > >
> > > > > > > > > While you can argue that the partition or filesystem
> > > > > > > > > where an
> > > > > > > > > image
> > > > > > > > > comes from is a software config, it is something that
> > > > > > > > > has to be
> > > > > > > > > configured. It has impact on hardware too, since the
> > > > > > > > > FPGA has to
> > > > > > > > > get
> > > > > > > > > its firmware from somewhere. We use the chosen node
> > > > > > > > > to
> > > > > > > > > specify
> > > > > > > > > the
> > > > > > > > > UART to use, and this is no different. Again, we
> > > > > > > > > don't
> > > > > > > > > have user-
> > > > > > > > > space
> > > > > > > > > config files in U-Boot.
> > > > > > > > >
> > > > > > > > > This argument comes up from time to time and I'd
> > > > > > > > > really
> > > > > > > > > like to
> > > > > > > > > put it
> > > > > > > > > to bed for U-Boot. I understand that Linux has its
> > > > > > > > > own
> > > > > > > > > approach
> > > > > > > > > and
> > > > > > > > > rules, but in some cases they serve U-Boot poorly.
> > > > > > > > I want to second this as well. So long as we're using
> > > > > > > > our prefix
> > > > > > > > and
> > > > > > > > we've thought through and discussed what we're trying
> > > > > > > > to
> > > > > > > > do here,
> > > > > > > > it's
> > > > > > > > OK to do things that might not be accepted for Linux.
> > > > > > > >
> > > > > > > I have not a problem with using chosen node with u-boot
> > > > > > > prefix
> > > > > > > properties and my colleague hopefully with finish work
> > > > > > > about moving
> > > > > > > u-boot,dm-pre-reloc; to chosen node where it should be
> > > > > > > (because
> > > > > > > current
> > > > > > > solution has also problem with ordering).
> > > > > > >
> > > > > > > In this loader case doc is saying that you can rewrite it
> > > > > > > with
> > > > > > > variables
> > > > > > > on the prompt (or via script).
> > > > > > > For cases that you want to autodetect platform and
> > > > > > > pass/load correct
> > > > > > > dtb
> > > > > > > which setup u-boot this can be problematic and using DT
> > > > > > > is
> > > > > > > could be
> > > > > > > considered as easier for use.
> > > > > > >
> > > > > > > In this case this is what was proposed:
> > > > > > >
> > > > > > > + fs_loader0: fs-loader at 0 {
> > > > > > > + u-boot,dm-pre-reloc;
> > > > > > > + compatible = "u-boot,fs-loader";
> > > > > > > + phandlepart = <&mmc 1>;
> > > > > > > + };
> > > > > > >
> > > > > > > + fs_loader1: fs-loader at 1 {
> > > > > > > + u-boot,dm-pre-reloc;
> > > > > > > + compatible = "u-boot,fs-loader";
> > > > > > > + mtdpart = "UBI",
> > > > > > > + ubivol = "ubi0";
> > > > > > > + };
> > > > > > >
> > > > > > > u-boot,dm-pre-reloc; requires DM_FLAG_PRE_RELOC which is
> > > > > > > not setup
> > > > > > > for
> > > > > > > this driver - it means this should be here.
> > > > > > You are right, i missed this one. The intention of design
> > > > > > enables user
> > > > > > to call any loader with default storage through the
> > > > > > sequence
> > > > > > number if
> > > > > > fs loader is not defined in chosen. For example, there is a
> > > > > > case where
> > > > > > system loading the file from SDMMC, NAND and QSPI.
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > compatible = "u-boot,fs-loader"; - bind and probe are
> > > > > > > empty
> > > > > > > that's
> > > > > > > why
> > > > > > > this is only used for filling platdata but driver has no
> > > > > > > user that's
> > > > > > > why
> > > > > > > this is unused till someone calls that functions.
> > > > > > >
> > > > > > > phandlepart/mtdpart/ubivol is just for setup.
> > > > > > There are some benefits with driver model:
> > > > > > 1. Saving space, calling when need.
> > > > > > 2. Handle memory allocation and deallocation automatically.
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > For the first case you can just use in chosen node:
> > > > > > > u-boot,fs-loader = <&mmc 1>;
> > > > > > >
> > > > > > > And for UBIfs. I have never played with that but I expect
> > > > > > > it
> > > > > > > shouldn't
> > > > > > > be big problem to describe it differently too (something
> > > > > > > like)
> > > > > > > u-boot,fs-loader = <0 ubi0>;
> > > > > > Need consider description for UBIFS, using fs-loader seems
> > > > > > not working
> > > > > > for UBIFS, since more arguments such as mtdpartition and
> > > > > > mtd
> > > > > > volume
> > > > > > need passing into driver. In order to avoid messing,
> > > > > > fs_loader can act
> > > > > > the pointer to the chosen.
> > > > > >
> > > > > > Anyway, i have no strong opinion with driver designed via
> > > > > > platdata or
> > > > > > driver model if we can resolve the problem for UBIFS and
> > > > > > maintainers
> > > > > > agree with it.
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > Then this driver/interface can stay in DT where it should
> > > > > > > stay. The
> > > > > > > only
> > > > > > > thing is how this should be initialized because there is
> > > > > > > no
> > > > > > > compatible
> > > > > > > string. But you can do that via platdata for platforms
> > > > > > > which want to
> > > > > > > use
> > > > > > > this.
> > > > > We should add a compatible string then :-)
> > > > Isn't driver name used in case of platdata initialization?
> > > If the node is in /chosen and has a compatible string, it will be
> > > bound automatically. Manually binding a device is really just a
> > > fallback for particular situations (e.g. buses like PCI where we
> > > often
> > > rely on probing to find out what is on the bus).
> > up2you guys. I just have different opinion how this should be done.
> >
> > Thanks,
> > Michal
> Sorry for late reply, because rushing the project.
> So, are you guys OK with current implementation because we have no
> solution for the UBIFS setting with other solution than using DT?
> >
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [PATCH v4 6/6] common: Generic loader for file system
2018-09-25 7:02 ` Chee, Tien Fong
@ 2018-09-25 19:37 ` Tom Rini
2018-09-27 8:08 ` Chee, Tien Fong
2018-09-25 19:39 ` Simon Glass
1 sibling, 1 reply; 29+ messages in thread
From: Tom Rini @ 2018-09-25 19:37 UTC (permalink / raw)
To: u-boot
On Tue, Sep 25, 2018 at 07:02:42AM +0000, Chee, Tien Fong wrote:
> On Thu, 2018-09-20 at 21:42 -0700, Chee, Tien Fong wrote:
> > >
> If everybody agree with current framework, then the next version i will
> include the fixes:
> 1. Adding DM_FLAG_PRE_RELOC, this would allow runtime to choose the
> right fs_loader if the chosen node u-boot, fs-loader is not defined.
>
> 2. Let driver model handles all memory allocation
>
> 3. Using local variable instead of messy casts.
A new version of this series, or a follow up series? Thanks again!
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180925/9f94fa50/attachment.sig>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [PATCH v4 6/6] common: Generic loader for file system
2018-09-25 7:02 ` Chee, Tien Fong
2018-09-25 19:37 ` Tom Rini
@ 2018-09-25 19:39 ` Simon Glass
1 sibling, 0 replies; 29+ messages in thread
From: Simon Glass @ 2018-09-25 19:39 UTC (permalink / raw)
To: u-boot
Hi,
On 25 September 2018 at 01:02, Chee, Tien Fong <tien.fong.chee@intel.com> wrote:
> On Thu, 2018-09-20 at 21:42 -0700, Chee, Tien Fong wrote:
>> >
> If everybody agree with current framework, then the next version i will
> include the fixes:
> 1. Adding DM_FLAG_PRE_RELOC, this would allow runtime to choose the
> right fs_loader if the chosen node u-boot, fs-loader is not defined.
>
> 2. Let driver model handles all memory allocation
>
> 3. Using local variable instead of messy casts.
Sounds good.
Can you please give an example of this? If you just mean within a
function, that's fine. We try to avoid static variables with driver
model.
Regards,
Simon
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [PATCH v4 6/6] common: Generic loader for file system
2018-09-25 19:37 ` Tom Rini
@ 2018-09-27 8:08 ` Chee, Tien Fong
0 siblings, 0 replies; 29+ messages in thread
From: Chee, Tien Fong @ 2018-09-27 8:08 UTC (permalink / raw)
To: u-boot
On Tue, 2018-09-25 at 15:37 -0400, Tom Rini wrote:
> On Tue, Sep 25, 2018 at 07:02:42AM +0000, Chee, Tien Fong wrote:
>
> >
> > On Thu, 2018-09-20 at 21:42 -0700, Chee, Tien Fong wrote:
> > >
> > > >
> > > >
> > If everybody agree with current framework, then the next version i
> > will
> > include the fixes:
> > 1. Adding DM_FLAG_PRE_RELOC, this would allow runtime to choose the
> > right fs_loader if the chosen node u-boot, fs-loader is not
> > defined.
> >
> > 2. Let driver model handles all memory allocation
> >
> > 3. Using local variable instead of messy casts.
> A new version of this series, or a follow up series? Thanks again!
>
This changes will be in follow up series to address feedback raised by
Simon and michal on v4.
^ permalink raw reply [flat|nested] 29+ messages in thread
* [U-Boot] [U-Boot, v4, 6/6] common: Generic loader for file system
2018-07-06 8:28 [U-Boot] [PATCH v4 6/6] common: Generic loader for file system tien.fong.chee at intel.com
` (2 preceding siblings ...)
2018-07-18 14:48 ` Michal Simek
@ 2018-09-29 15:43 ` Tom Rini
3 siblings, 0 replies; 29+ messages in thread
From: Tom Rini @ 2018-09-29 15:43 UTC (permalink / raw)
To: u-boot
On Fri, Jul 06, 2018 at 04:28:03PM +0800, tien.fong.chee at 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>
Applied to u-boot/master, thanks!
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180929/f5c7af28/attachment.sig>
^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2018-09-29 15:43 UTC | newest]
Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-06 8:28 [U-Boot] [PATCH v4 6/6] common: Generic loader for file system tien.fong.chee at intel.com
2018-07-11 14:02 ` Simon Glass
2018-07-11 14:20 ` Chee, Tien Fong
2018-07-11 20:13 ` Simon Glass
2018-07-12 7:19 ` Chee, Tien Fong
2018-07-15 21:21 ` Simon Glass
2018-07-17 4:46 ` Chee, Tien Fong
2018-07-18 14:48 ` Michal Simek
2018-07-25 6:31 ` Chee, Tien Fong
2018-07-25 9:48 ` Michal Simek
2018-07-25 15:47 ` Simon Glass
2018-07-25 16:03 ` Tom Rini
2018-07-26 9:03 ` Michal Simek
2018-07-27 8:40 ` Chee, Tien Fong
2018-07-30 7:07 ` Michal Simek
2018-07-30 13:26 ` Simon Glass
2018-07-30 13:30 ` Michal Simek
2018-07-30 16:05 ` Simon Glass
2018-07-31 5:12 ` Chee, Tien Fong
2018-07-31 6:22 ` Michal Simek
2018-09-21 4:42 ` Chee, Tien Fong
2018-09-25 7:02 ` Chee, Tien Fong
2018-09-25 19:37 ` Tom Rini
2018-09-27 8:08 ` Chee, Tien Fong
2018-09-25 19:39 ` Simon Glass
2018-07-26 9:23 ` Chee, Tien Fong
2018-07-26 10:29 ` Michal Simek
2018-07-27 8:18 ` Chee, Tien Fong
2018-09-29 15:43 ` [U-Boot] [U-Boot, v4, " Tom Rini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.