[U-Boot] [PATCH v2 0/3] Generic file system firmware loader DM

[U-Boot] [PATCH v2 0/3] Generic file system firmware loader DM

[tien.fong.chee at intel.com]

From: Tien Fong Chee <tien.fong.chee@intel.com>

This patchset contains generic file system loader DM which is very close to
Linux firmware loader but for U-Boot framework. Generic file system firmware
loader can be used load whatever into target location, and then consumer driver
would use it to program whatever, ie. the FPGA. This version mainly resolved
comments from Tom Rini in [v1].

This series is working on top of u-boot-socfpga.git -
 http://git.denx.de/u-boot.git .

[v1]: https://www.mail-archive.com/u-boot at lists.denx.de/msg286294.html

v1 -> v2 changes
----------------
- Changed the documents to rST format.

Tien Fong Chee (3):
  doc: Add new doc for file system firmware loader driver model
  doc: dtbinding: Add file system firmware loader binding document
  common: Generic loader for file system

 doc/device-tree-bindings/misc/fs_loader.txt |  50 ++++++
 doc/driver-model/fs_firmware_loader.txt     | 103 ++++++++++++
 drivers/misc/Kconfig                        |  11 ++
 drivers/misc/Makefile                       |   1 +
 drivers/misc/fs_loader.c                    | 240 ++++++++++++++++++++++++++++
 include/dm/uclass-id.h                      |   1 +
 include/fs_loader.h                         |  28 ++++
 7 files changed, 434 insertions(+)
 create mode 100644 doc/device-tree-bindings/misc/fs_loader.txt
 create mode 100644 doc/driver-model/fs_firmware_loader.txt
 create mode 100644 drivers/misc/fs_loader.c
 create mode 100644 include/fs_loader.h

-- 
2.2.0

[U-Boot] [PATCH v2 1/3] doc: Add new doc for file system firmware loader driver model

[tien.fong.chee at intel.com]

From: Tien Fong Chee <tien.fong.chee@intel.com>

Provide information about

- overview of file system firmware loader driver model
- describe default storage device in device tree source
- describe fie system firmware loader API

Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
---
 doc/driver-model/fs_firmware_loader.txt | 103 ++++++++++++++++++++++++++++++++
 1 file changed, 103 insertions(+)
 create mode 100644 doc/driver-model/fs_firmware_loader.txt

diff --git a/doc/driver-model/fs_firmware_loader.txt b/doc/driver-model/fs_firmware_loader.txt
new file mode 100644
index 0000000..f05db8d
--- /dev/null
+++ b/doc/driver-model/fs_firmware_loader.txt
@@ -0,0 +1,103 @@
+# Copyright (C) 2018 Intel Corporation <www.intel.com>
+#
+# SPDX-License-Identifier:    GPL-2.0
+
+Introduction
+============
+
+This is file system firmware loader for U-Boot framework, which has very close
+to some Linux Firmware API. For the details of Linux Firmware API, you can refer
+to https://01.org/linuxgraphics/gfx-docs/drm/driver-api/firmware/index.html.
+
+File system firmware loader can be used to load whatever(firmware, image,
+and binary) from the storage device in file system format into target location
+such as memory, then consumer driver such as FPGA driver can program FPGA image
+from the target location into FPGA.
+
+To enable firmware loader driver model, CONFIG_FS_LOADER need to be set at
+<board_name>_defconfig such as "CONFIG_FS_LOADER=y".
+
+Firmware Loader API core features
+---------------------------------
+
+Firmware storage device described in device tree source
+-------------------------------------------------------
+	Example of default storage device partition search set for mmc, usb,
+	sata and ubi as shown in below:
+	Example for mmc:
+	fs_loader {
+		u-boot,dm-pre-reloc;
+		compatible = "fs_loader";
+		storage_device = "mmc";
+		devpart = "0:1";
+	};
+
+	Example for usb:
+	fs_loader {
+		u-boot,dm-pre-reloc;
+		compatible = "fs_loader";
+		storage_device = "usb";
+		devpart = "0:1";
+	};
+
+	Example for sata:
+	fs_loader {
+		u-boot,dm-pre-reloc;
+		compatible = "fs_loader";
+		storage_device = "sata";
+		devpart = "0:1";
+	};
+
+	Example for ubi:
+	fs_loader {
+		u-boot,dm-pre-reloc;
+		compatible = "fs_loader";
+		storage_device = "ubi";
+		mtdpart = "UBI",
+		ubivol = "ubi0";
+	};
+
+
+	However, the default fs_loader with property devpart, mtdpart, and
+	ubivol values can be overwritten with value which is defined in the
+	environment variable "fw_dev_part", "fw_ubi_mtdpart", and
+	"fw_ubi_volume" respectively.
+	For example: env_set("fw_dev_part", "0:2");
+
+File system firmware Loader API
+-------------------------------
+
+int request_firmware_into_buf(struct udevice *dev,
+				 struct firmware **firmware_p,
+				 const char *name,
+				 void *buf, size_t size, u32 offset)
+--------------------------------------------------------------------
+Load firmware into a previously allocated buffer
+
+Parameters:
+
+1. struct udevice *dev
+	An instance of a driver
+
+2. struct firmware **firmware_p
+	pointer to firmware image
+
+3. const char *name
+	name of firmware file
+
+4. void *buf
+	address of buffer to load firmware into
+
+5. size_t size
+	size of buffer
+
+6. u32 offset
+	offset of a file for start reading into buffer
+
+return:
+	size of total read
+	-ve when error
+
+Description:
+	The firmware is loaded directly into the buffer pointed to by buf and
+	the @firmware_p data member is pointed at buf.
-- 
2.2.0

[U-Boot] [PATCH v2 2/3] doc: dtbinding: Add file system firmware loader binding document

[tien.fong.chee at intel.com]

From: Tien Fong Chee <tien.fong.chee@intel.com>

Add a document to describe file system firmware loader binding
information.

Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
---
 doc/device-tree-bindings/misc/fs_loader.txt | 50 +++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)
 create mode 100644 doc/device-tree-bindings/misc/fs_loader.txt

diff --git a/doc/device-tree-bindings/misc/fs_loader.txt b/doc/device-tree-bindings/misc/fs_loader.txt
new file mode 100644
index 0000000..066d690
--- /dev/null
+++ b/doc/device-tree-bindings/misc/fs_loader.txt
@@ -0,0 +1,50 @@
+* File system firmware loader
+
+Required properties:
+--------------------
+
+- compatible: should contain "fs_loader"
+- storage_device: which storage device loading from, could be:
+		  - mmc, usb, sata, and ubi.
+- devpart: which storage device and partition the image loading from,
+	   this property is required for mmc, usb and sata.
+- mdtpart: which partition of ubi the image loading from, this property is
+	   required for ubi.
+- ubivol: which volume of ubi the image loading from, this proprety is required
+	  for ubi.
+
+Example of default storage device partition search set for mmc, usb, sata and
+ubi in device tree source as shown in below:
+
+Example for mmc:
+	fs_loader {
+		u-boot,dm-pre-reloc;
+		compatible = "fs_loader";
+		storage_device = "mmc";
+		devpart = "0:1";
+	};
+
+Example for usb:
+	fs_loader {
+		u-boot,dm-pre-reloc;
+		compatible = "fs_loader";
+		storage_device = "usb";
+		devpart = "0:1";
+	};
+
+Example for sata:
+	fs_loader {
+		u-boot,dm-pre-reloc;
+		compatible = "fs_loader";
+		storage_device = "sata";
+		devpart = "0:1";
+	};
+
+Example for ubi:
+	fs_loader {
+		u-boot,dm-pre-reloc;
+		compatible = "fs_loader";
+		storage_device = "ubi";
+		mtdpart = "UBI",
+		ubivol = "ubi0";
+	};
-- 
2.2.0

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

[tien.fong.chee at intel.com]

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     |  11 +++
 drivers/misc/Makefile    |   1 +
 drivers/misc/fs_loader.c | 240 +++++++++++++++++++++++++++++++++++++++++++++++
 include/dm/uclass-id.h   |   1 +
 include/fs_loader.h      |  28 ++++++
 5 files changed, 281 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 be900cf..59f716b 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -268,4 +268,15 @@ config GDSYS_RXAUI_CTRL
 	depends on MISC
 	help
 	  Support gdsys FPGA's RXAUI control.
+
+config FS_LOADER
+	bool "Enable loader driver for file system"
+	depends on DM
+	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 e362609..74364a0 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -53,3 +53,4 @@ obj-$(CONFIG_ROCKCHIP_EFUSE) += rockchip-efuse.o
 obj-$(CONFIG_STM32_RCC) += stm32_rcc.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..127f7f1
--- /dev/null
+++ b/drivers/misc/fs_loader.c
@@ -0,0 +1,240 @@
+/*
+ * Copyright (C) 2018 Intel Corporation <www.intel.com>
+ *
+ * SPDX-License-Identifier:    GPL-2.0
+ */
+#include <common.h>
+#include <dm.h>
+#include <errno.h>
+#include <fdtdec.h>
+#include <fs.h>
+#include <fs_loader.h>
+#include <linux/string.h>
+#include <malloc.h>
+#include <spl.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+struct firmware_priv {
+	const char *name;	/* Filename */
+	u32 offset;		/* Offset of reading a file */
+};
+
+static int select_fs_dev(struct udevice *dev)
+{
+	int ret;
+	struct device_platdata *plat;
+
+	plat = dev->platdata;
+	if (!strcmp("mmc", plat->name)) {
+		ret = fs_set_blk_dev("mmc", plat->devpart, FS_TYPE_ANY);
+	} else if (!strcmp("usb", plat->name)) {
+		ret = fs_set_blk_dev("usb", plat->devpart, FS_TYPE_ANY);
+	} else if (!strcmp("sata", plat->name)) {
+		ret = fs_set_blk_dev("sata", plat->devpart, FS_TYPE_ANY);
+	} else if (!strcmp("ubi", plat->name)) {
+		if (plat->ubivol)
+			ret = fs_set_blk_dev("ubi", NULL, FS_TYPE_UBIFS);
+		else
+			ret = -ENODEV;
+	} else {
+		debug("Error: unsupported storage device.\n");
+		return -ENODEV;
+	}
+
+	if (ret)
+		debug("Error: could not access storage.\n");
+
+	return ret;
+}
+
+static void set_storage_devpart(struct udevice *dev, char *devpart)
+{
+	struct device_platdata *plat;
+
+	plat = dev->platdata;
+	plat->devpart = devpart;
+}
+
+static void set_storage_mtdpart(struct udevice *dev, char *mtdpart)
+{
+	struct device_platdata *plat;
+
+	plat = dev->platdata;
+	plat->mtdpart = mtdpart;
+}
+
+static void set_storage_ubivol(struct udevice *dev, char *ubivol)
+{
+	struct device_platdata *plat;
+
+	plat = dev->platdata;
+	plat->ubivol = ubivol;
+}
+
+/**
+ * _request_firmware_prepare - Prepare firmware struct.
+ *
+ * @firmware_p: Pointer to pointer to firmware image.
+ * @name: Name of firmware file.
+ * @dbuf: Address of buffer to load firmware into.
+ * @size: Size of buffer.
+ * @offset: Offset of a file for start reading into buffer.
+ *
+ * Return: Negative value if fail, 0 for successful.
+ */
+static int _request_firmware_prepare(struct firmware **firmware_p,
+				    const char *name, void *dbuf,
+				    size_t size, u32 offset)
+{
+	struct firmware *firmware;
+	struct firmware_priv *fw_priv;
+
+	*firmware_p = NULL;
+
+	if (!name || name[0] == '\0')
+		return -EINVAL;
+
+	firmware = calloc(1, sizeof(*firmware));
+	if (!firmware)
+		return -ENOMEM;
+
+	fw_priv = calloc(1, sizeof(*fw_priv));
+	if (!fw_priv) {
+		free(firmware);
+		return -ENOMEM;
+	}
+
+	fw_priv->name = name;
+	fw_priv->offset = offset;
+	firmware->data = dbuf;
+	firmware->size = size;
+	firmware->priv = fw_priv;
+	*firmware_p = firmware;
+
+	return 0;
+}
+
+/**
+ * fw_get_filesystem_firmware - load firmware into an allocated buffer.
+ * @dev: An instance of a driver.
+ * @firmware_p: pointer to firmware image.
+ *
+ * Return: Size of total read, negative value when error.
+ */
+static int fw_get_filesystem_firmware(struct udevice *dev,
+				     struct firmware *firmware_p)
+{
+	struct firmware_priv *fw_priv = NULL;
+	loff_t actread;
+	char *dev_part, *ubi_mtdpart, *ubi_volume;
+	int ret;
+
+	dev_part = env_get("fw_dev_part");
+	if (dev_part)
+		set_storage_devpart(dev, dev_part);
+
+	ubi_mtdpart = env_get("fw_ubi_mtdpart");
+	if (ubi_mtdpart)
+		set_storage_mtdpart(dev, ubi_mtdpart);
+
+	ubi_volume = env_get("fw_ubi_volume");
+	if (ubi_volume)
+		set_storage_ubivol(dev, ubi_volume);
+
+	ret = select_fs_dev(dev);
+	if (ret)
+		goto out;
+
+	fw_priv = firmware_p->priv;
+
+	ret = fs_read(fw_priv->name, (ulong)firmware_p->data, fw_priv->offset,
+		     firmware_p->size, &actread);
+	if (ret) {
+		debug("Error: %d Failed to read %s from flash %lld != %d.\n",
+		      ret, fw_priv->name, actread, firmware_p->size);
+	} else {
+		ret = actread;
+	}
+
+out:
+	return ret;
+}
+
+/**
+ * request_firmware_into_buf - Load firmware into a previously allocated buffer.
+ * @dev: An instance of a driver.
+ * @firmware_p: Pointer to firmware image.
+ * @name: Name of firmware file.
+ * @buf: Address of buffer to load firmware into.
+ * @size: Size of buffer.
+ * @offset: Offset of a file for start reading into buffer.
+ *
+ * The firmware is loaded directly into the buffer pointed to by @buf and
+ * the @firmware_p data member is pointed at @buf.
+ *
+ * Return: Size of total read, negative value when error.
+ */
+int request_firmware_into_buf(struct udevice *dev,
+			      struct firmware **firmware_p,
+			      const char *name,
+			      void *buf, size_t size, u32 offset)
+{
+	int ret;
+
+	ret = _request_firmware_prepare(firmware_p, name, buf, size, offset);
+	if (ret < 0) /* error */
+		return ret;
+
+	ret = fw_get_filesystem_firmware(dev, *firmware_p);
+
+	return ret;
+}
+
+static int fs_loader_ofdata_to_platdata(struct udevice *dev)
+{
+	struct device_platdata *plat;
+	const void *blob;
+	int node;
+
+	plat = dev->platdata;
+	blob = gd->fdt_blob;
+	node = dev_of_offset(dev);
+	plat->name = (char *)fdt_getprop(blob, node,
+				"storage_device", NULL);
+
+	plat->devpart = (char *)fdt_getprop(blob, node,
+				"devpart", NULL);
+
+	plat->mtdpart = (char *)fdt_getprop(blob, node,
+				"mtdpart", NULL);
+
+	plat->ubivol = (char *)fdt_getprop(blob, node,
+				"ubivol", NULL);
+
+	return 0;
+}
+
+static int fs_loader_probe(struct udevice *dev)
+{
+	return 0;
+};
+
+static const struct udevice_id fs_loader_ids[] = {
+	{ .compatible = "fs_loader"},
+	{ }
+};
+
+U_BOOT_DRIVER(fs_loader) = {
+	.name			= "fs_loader",
+	.id			= UCLASS_FS_FIRMWARE_LOADER,
+	.of_match		= fs_loader_ids,
+	.probe			= fs_loader_probe,
+	.ofdata_to_platdata	= fs_loader_ofdata_to_platdata,
+	.platdata_auto_alloc_size	= sizeof(struct device_platdata),
+};
+
+UCLASS_DRIVER(fs_loader) = {
+	.id		= UCLASS_FS_FIRMWARE_LOADER,
+	.name		= "fs_loader",
+};
diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
index d7f9df3..39e88ac 100644
--- a/include/dm/uclass-id.h
+++ b/include/dm/uclass-id.h
@@ -36,6 +36,7 @@ enum uclass_id {
 	UCLASS_DMA,		/* Direct Memory Access */
 	UCLASS_EFI,		/* EFI managed devices */
 	UCLASS_ETH,		/* Ethernet device */
+	UCLASS_FS_FIRMWARE_LOADER,		/* Generic loader */
 	UCLASS_GPIO,		/* Bank of general-purpose I/O pins */
 	UCLASS_FIRMWARE,	/* Firmware */
 	UCLASS_I2C,		/* I2C bus */
diff --git a/include/fs_loader.h b/include/fs_loader.h
new file mode 100644
index 0000000..0f0ea00
--- /dev/null
+++ b/include/fs_loader.h
@@ -0,0 +1,28 @@
+/*
+ * Copyright (C) 2018 Intel Corporation <www.intel.com>
+ *
+ * SPDX-License-Identifier:    GPL-2.0
+ */
+#ifndef _FS_LOADER_H_
+#define _FS_LOADER_H_
+
+#include <dm.h>
+
+struct firmware {
+	size_t size;		/* Size of a file */
+	const u8 *data;		/* Buffer for file */
+	void *priv;		/* Firmware loader private fields */
+};
+
+struct device_platdata {
+	char *name;	/* Such as mmc, usb,and sata. */
+	char *devpart;  /* Use the load command dev:part conventions */
+	char *mtdpart;	/* MTD partition for ubi part */
+	char *ubivol;	/* UBI volume-name for ubifsmount */
+};
+
+int request_firmware_into_buf(struct udevice *dev,
+			      struct firmware **firmware_p,
+			      const char *name,
+			      void *buf, size_t size, u32 offset);
+#endif
-- 
2.2.0

[U-Boot] [PATCH v2 0/3] Generic file system firmware loader DM

[Chee, Tien Fong]

On Thu, 2018-05-24 at 13:04 +0800, tien.fong.chee at intel.com wrote:
Hi All,
> From: Tien Fong Chee <tien.fong.chee@intel.com>
> 
> This patchset contains generic file system loader DM which is very
> close to
> Linux firmware loader but for U-Boot framework. Generic file system
> firmware
> loader can be used load whatever into target location, and then
> consumer driver
> would use it to program whatever, ie. the FPGA. This version mainly
> resolved
> comments from Tom Rini in [v1].
> 
> This series is working on top of u-boot-socfpga.git -
>  http://git.denx.de/u-boot.git .
> 
> [v1]: https://www.mail-archive.com/u-boot at lists.denx.de/msg286294.htm
> l
> 
> v1 -> v2 changes
> ----------------
> - Changed the documents to rST format.
> 
Any comment on this patchset? Good to go :)?
> Tien Fong Chee (3):
>   doc: Add new doc for file system firmware loader driver model
>   doc: dtbinding: Add file system firmware loader binding document
>   common: Generic loader for file system
> 
>  doc/device-tree-bindings/misc/fs_loader.txt |  50 ++++++
>  doc/driver-model/fs_firmware_loader.txt     | 103 ++++++++++++
>  drivers/misc/Kconfig                        |  11 ++
>  drivers/misc/Makefile                       |   1 +
>  drivers/misc/fs_loader.c                    | 240
> ++++++++++++++++++++++++++++
>  include/dm/uclass-id.h                      |   1 +
>  include/fs_loader.h                         |  28 ++++
>  7 files changed, 434 insertions(+)
>  create mode 100644 doc/device-tree-bindings/misc/fs_loader.txt
>  create mode 100644 doc/driver-model/fs_firmware_loader.txt
>  create mode 100644 drivers/misc/fs_loader.c
>  create mode 100644 include/fs_loader.h
> 

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

[Marek Vasut]

On 05/24/2018 07:04 AM, 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>
> ---
[...]
> +static int fs_loader_probe(struct udevice *dev)
> +{
> +	return 0;
> +};
> +
> +static const struct udevice_id fs_loader_ids[] = {
> +	{ .compatible = "fs_loader"},

Why exactly is there a DT compatible for a firmware 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",
> +};

[...]
-- 
Best regards,
Marek Vasut

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

[Chee, Tien Fong]

On Wed, 2018-06-06 at 10:39 +0200, Marek Vasut wrote:
> On 05/24/2018 07:04 AM, 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>
> > ---
> [...]
> > 
> > +static int fs_loader_probe(struct udevice *dev)
> > +{
> > +	return 0;
> > +};
> > +
> > +static const struct udevice_id fs_loader_ids[] = {
> > +	{ .compatible = "fs_loader"},
> Why exactly is there a DT compatible for a firmware loader?
> 
Correct me if i'm wrong, this is required to look the platform data
from DTS, right? Details of DTS in patch 2.
> > 
> > +	{ }
> > +};
> > +
> > +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",
> > +};
> [...]

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

[Marek Vasut]

On 06/07/2018 06:04 AM, Chee, Tien Fong wrote:
> On Wed, 2018-06-06 at 10:39 +0200, Marek Vasut wrote:
>> On 05/24/2018 07:04 AM, 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>
>>> ---
>> [...]
>>>
>>> +static int fs_loader_probe(struct udevice *dev)
>>> +{
>>> +	return 0;
>>> +};
>>> +
>>> +static const struct udevice_id fs_loader_ids[] = {
>>> +	{ .compatible = "fs_loader"},
>> Why exactly is there a DT compatible for a firmware loader?
>>
> Correct me if i'm wrong, this is required to look the platform data
> from DTS, right? Details of DTS in patch 2.

How so ? The FW loader should behave as a library for other drivers to
use, not like a driver.

>>>
>>> +	{ }
>>> +};
>>> +
>>> +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",
>>> +};
>> [...]


-- 
Best regards,
Marek Vasut

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

[Chee, Tien Fong]

On Thu, 2018-06-07 at 08:51 +0200, Marek Vasut wrote:
> On 06/07/2018 06:04 AM, Chee, Tien Fong wrote:
> > 
> > On Wed, 2018-06-06 at 10:39 +0200, Marek Vasut wrote:
> > > 
> > > On 05/24/2018 07:04 AM, 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>
> > > > ---
> > > [...]
> > > > 
> > > > 
> > > > +static int fs_loader_probe(struct udevice *dev)
> > > > +{
> > > > +	return 0;
> > > > +};
> > > > +
> > > > +static const struct udevice_id fs_loader_ids[] = {
> > > > +	{ .compatible = "fs_loader"},
> > > Why exactly is there a DT compatible for a firmware loader?
> > > 
> > Correct me if i'm wrong, this is required to look the platform data
> > from DTS, right? Details of DTS in patch 2.
> How so ? The FW loader should behave as a library for other drivers
> to
> use, not like a driver.
> 
The fs_loader node in DTS just provide a way for user to tell the
firmware loader what storage device and default partition to load data
from. Default partition can be overriden through the variable
environment.

Caller/other drivers can create different firmware loader instance
based on different fs_loader node(different storage device) for their
loading purpose.

Why this cannot be used by other driver?
> > 
> > > 
> > > > 
> > > > 
> > > > +	{ }
> > > > +};
> > > > +
> > > > +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",
> > > > +};
> > > [...]
> 

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

[Marek Vasut]

On 06/07/2018 10:36 AM, Chee, Tien Fong wrote:
> On Thu, 2018-06-07 at 08:51 +0200, Marek Vasut wrote:
>> On 06/07/2018 06:04 AM, Chee, Tien Fong wrote:
>>>
>>> On Wed, 2018-06-06 at 10:39 +0200, Marek Vasut wrote:
>>>>
>>>> On 05/24/2018 07:04 AM, 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>
>>>>> ---
>>>> [...]
>>>>>
>>>>>
>>>>> +static int fs_loader_probe(struct udevice *dev)
>>>>> +{
>>>>> +	return 0;
>>>>> +};
>>>>> +
>>>>> +static const struct udevice_id fs_loader_ids[] = {
>>>>> +	{ .compatible = "fs_loader"},
>>>> Why exactly is there a DT compatible for a firmware loader?
>>>>
>>> Correct me if i'm wrong, this is required to look the platform data
>>> from DTS, right? Details of DTS in patch 2.
>> How so ? The FW loader should behave as a library for other drivers
>> to
>> use, not like a driver.
>>
> The fs_loader node in DTS just provide a way for user to tell the
> firmware loader what storage device and default partition to load data
> from. Default partition can be overriden through the variable
> environment.

So that's sitting in the chosen node ? But why do you need to match on it ?

> Caller/other drivers can create different firmware loader instance
> based on different fs_loader node(different storage device) for their
> loading purpose.

They can, but that could be done even without the DT compatible.

> Why this cannot be used by other driver?
I don't understand the question.

-- 
Best regards,
Marek Vasut

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

[Chee, Tien Fong]

On Thu, 2018-06-07 at 10:41 +0200, Marek Vasut wrote:
> On 06/07/2018 10:36 AM, Chee, Tien Fong wrote:
> > 
> > On Thu, 2018-06-07 at 08:51 +0200, Marek Vasut wrote:
> > > 
> > > On 06/07/2018 06:04 AM, Chee, Tien Fong wrote:
> > > > 
> > > > 
> > > > On Wed, 2018-06-06 at 10:39 +0200, Marek Vasut wrote:
> > > > > 
> > > > > 
> > > > > On 05/24/2018 07:04 AM, 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>
> > > > > > ---
> > > > > [...]
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > +static int fs_loader_probe(struct udevice *dev)
> > > > > > +{
> > > > > > +	return 0;
> > > > > > +};
> > > > > > +
> > > > > > +static const struct udevice_id fs_loader_ids[] = {
> > > > > > +	{ .compatible = "fs_loader"},
> > > > > Why exactly is there a DT compatible for a firmware loader?
> > > > > 
> > > > Correct me if i'm wrong, this is required to look the platform
> > > > data
> > > > from DTS, right? Details of DTS in patch 2.
> > > How so ? The FW loader should behave as a library for other
> > > drivers
> > > to
> > > use, not like a driver.
> > > 
> > The fs_loader node in DTS just provide a way for user to tell the
> > firmware loader what storage device and default partition to load
> > data
> > from. Default partition can be overriden through the variable
> > environment.
> So that's sitting in the chosen node ? But why do you need to match
> on it ?
> 
This is new device tree binding for firmware loader called fs_loader
node. firmware loader need to know where is the storage device it need
to load from. Those storage device would be defined in fs_loader node.
> > 
> > Caller/other drivers can create different firmware loader instance
> > based on different fs_loader node(different storage device) for
> > their
> > loading purpose.
> They can, but that could be done even without the DT compatible.
> 
Yes, once you get dev, you can set the storage type and partiiton
attributes through accessing dev->platdata.
> > 
> > Why this cannot be used by other driver?
> I don't understand the question.
> 
I means why u say this firmware loader cannot be used by other drivers.
I have tested with FPGA Manager driver(as caller).

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

[Marek Vasut]

On 06/07/2018 11:45 AM, Chee, Tien Fong wrote:
> On Thu, 2018-06-07 at 10:41 +0200, Marek Vasut wrote:
>> On 06/07/2018 10:36 AM, Chee, Tien Fong wrote:
>>>
>>> On Thu, 2018-06-07 at 08:51 +0200, Marek Vasut wrote:
>>>>
>>>> On 06/07/2018 06:04 AM, Chee, Tien Fong wrote:
>>>>>
>>>>>
>>>>> On Wed, 2018-06-06 at 10:39 +0200, Marek Vasut wrote:
>>>>>>
>>>>>>
>>>>>> On 05/24/2018 07:04 AM, 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>
>>>>>>> ---
>>>>>> [...]
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> +static int fs_loader_probe(struct udevice *dev)
>>>>>>> +{
>>>>>>> +	return 0;
>>>>>>> +};
>>>>>>> +
>>>>>>> +static const struct udevice_id fs_loader_ids[] = {
>>>>>>> +	{ .compatible = "fs_loader"},
>>>>>> Why exactly is there a DT compatible for a firmware loader?
>>>>>>
>>>>> Correct me if i'm wrong, this is required to look the platform
>>>>> data
>>>>> from DTS, right? Details of DTS in patch 2.
>>>> How so ? The FW loader should behave as a library for other
>>>> drivers
>>>> to
>>>> use, not like a driver.
>>>>
>>> The fs_loader node in DTS just provide a way for user to tell the
>>> firmware loader what storage device and default partition to load
>>> data
>>> from. Default partition can be overriden through the variable
>>> environment.
>> So that's sitting in the chosen node ? But why do you need to match
>> on it ?
>>
> This is new device tree binding for firmware loader called fs_loader
> node. firmware loader need to know where is the storage device it need
> to load from. Those storage device would be defined in fs_loader node.

This is a configuration. You do not need (new) a DT compatible for that.
So why is the DT compatible even needed in the FW loader at all ?

>>> Caller/other drivers can create different firmware loader instance
>>> based on different fs_loader node(different storage device) for
>>> their
>>> loading purpose.
>> They can, but that could be done even without the DT compatible.
>>
> Yes, once you get dev, you can set the storage type and partiiton
> attributes through accessing dev->platdata.

Can you show me the DT patch needed for this to work ?

>>> Why this cannot be used by other driver?
>> I don't understand the question.
>>
> I means why u say this firmware loader cannot be used by other drivers.
> I have tested with FPGA Manager driver(as caller).

It must be used by other drivers, that is the point.

-- 
Best regards,
Marek Vasut

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

[Chee, Tien Fong]

On Thu, 2018-06-07 at 11:50 +0200, Marek Vasut wrote:
> On 06/07/2018 11:45 AM, Chee, Tien Fong wrote:
> > 
> > On Thu, 2018-06-07 at 10:41 +0200, Marek Vasut wrote:
> > > 
> > > On 06/07/2018 10:36 AM, Chee, Tien Fong wrote:
> > > > 
> > > > 
> > > > On Thu, 2018-06-07 at 08:51 +0200, Marek Vasut wrote:
> > > > > 
> > > > > 
> > > > > On 06/07/2018 06:04 AM, Chee, Tien Fong wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > On Wed, 2018-06-06 at 10:39 +0200, Marek Vasut wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > On 05/24/2018 07:04 AM, 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
> > > > > > > > >
> > > > > > > > ---
> > > > > > > [...]
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > +static int fs_loader_probe(struct udevice *dev)
> > > > > > > > +{
> > > > > > > > +	return 0;
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +static const struct udevice_id fs_loader_ids[] = {
> > > > > > > > +	{ .compatible = "fs_loader"},
> > > > > > > Why exactly is there a DT compatible for a firmware
> > > > > > > loader?
> > > > > > > 
> > > > > > Correct me if i'm wrong, this is required to look the
> > > > > > platform
> > > > > > data
> > > > > > from DTS, right? Details of DTS in patch 2.
> > > > > How so ? The FW loader should behave as a library for other
> > > > > drivers
> > > > > to
> > > > > use, not like a driver.
> > > > > 
> > > > The fs_loader node in DTS just provide a way for user to tell
> > > > the
> > > > firmware loader what storage device and default partition to
> > > > load
> > > > data
> > > > from. Default partition can be overriden through the variable
> > > > environment.
> > > So that's sitting in the chosen node ? But why do you need to
> > > match
> > > on it ?
> > > 
> > This is new device tree binding for firmware loader called
> > fs_loader
> > node. firmware loader need to know where is the storage device it
> > need
> > to load from. Those storage device would be defined in fs_loader
> > node.
> This is a configuration. You do not need (new) a DT compatible for
> that.
> So why is the DT compatible even needed in the FW loader at all ?
> 
I thought DT compatible is used by driver to find the fs_loader node in
DTS. May be i am wrong.
> > 
> > > 
> > > > 
> > > > Caller/other drivers can create different firmware loader
> > > > instance
> > > > based on different fs_loader node(different storage device) for
> > > > their
> > > > loading purpose.
> > > They can, but that could be done even without the DT compatible.
> > > 
> > Yes, once you get dev, you can set the storage type and partiiton
> > attributes through accessing dev->platdata.
> Can you show me the DT patch needed for this to work ?
> 
I can send you the patches separately, but this driver is designed to
work with/without DT.
> > 
> > > 
> > > > 
> > > > Why this cannot be used by other driver?
> > > I don't understand the question.
> > > 
> > I means why u say this firmware loader cannot be used by other
> > drivers.
> > I have tested with FPGA Manager driver(as caller).
> It must be used by other drivers, that is the point.
> 
yes, it can be used by FPGA manager driver.

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

[Marek Vasut]

On 06/07/2018 12:11 PM, Chee, Tien Fong wrote:
> On Thu, 2018-06-07 at 11:50 +0200, Marek Vasut wrote:
>> On 06/07/2018 11:45 AM, Chee, Tien Fong wrote:
>>>
>>> On Thu, 2018-06-07 at 10:41 +0200, Marek Vasut wrote:
>>>>
>>>> On 06/07/2018 10:36 AM, Chee, Tien Fong wrote:
>>>>>
>>>>>
>>>>> On Thu, 2018-06-07 at 08:51 +0200, Marek Vasut wrote:
>>>>>>
>>>>>>
>>>>>> On 06/07/2018 06:04 AM, Chee, Tien Fong wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Wed, 2018-06-06 at 10:39 +0200, Marek Vasut wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 05/24/2018 07:04 AM, 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
>>>>>>>>>>
>>>>>>>>> ---
>>>>>>>> [...]
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> +static int fs_loader_probe(struct udevice *dev)
>>>>>>>>> +{
>>>>>>>>> +	return 0;
>>>>>>>>> +};
>>>>>>>>> +
>>>>>>>>> +static const struct udevice_id fs_loader_ids[] = {
>>>>>>>>> +	{ .compatible = "fs_loader"},
>>>>>>>> Why exactly is there a DT compatible for a firmware
>>>>>>>> loader?
>>>>>>>>
>>>>>>> Correct me if i'm wrong, this is required to look the
>>>>>>> platform
>>>>>>> data
>>>>>>> from DTS, right? Details of DTS in patch 2.
>>>>>> How so ? The FW loader should behave as a library for other
>>>>>> drivers
>>>>>> to
>>>>>> use, not like a driver.
>>>>>>
>>>>> The fs_loader node in DTS just provide a way for user to tell
>>>>> the
>>>>> firmware loader what storage device and default partition to
>>>>> load
>>>>> data
>>>>> from. Default partition can be overriden through the variable
>>>>> environment.
>>>> So that's sitting in the chosen node ? But why do you need to
>>>> match
>>>> on it ?
>>>>
>>> This is new device tree binding for firmware loader called
>>> fs_loader
>>> node. firmware loader need to know where is the storage device it
>>> need
>>> to load from. Those storage device would be defined in fs_loader
>>> node.
>> This is a configuration. You do not need (new) a DT compatible for
>> that.
>> So why is the DT compatible even needed in the FW loader at all ?
>>
> I thought DT compatible is used by driver to find the fs_loader node in
> DTS. May be i am wrong.

There should be no FW loader in the DTS. Why would there be one ?

>>>
>>>>
>>>>>
>>>>> Caller/other drivers can create different firmware loader
>>>>> instance
>>>>> based on different fs_loader node(different storage device) for
>>>>> their
>>>>> loading purpose.
>>>> They can, but that could be done even without the DT compatible.
>>>>
>>> Yes, once you get dev, you can set the storage type and partiiton
>>> attributes through accessing dev->platdata.
>> Can you show me the DT patch needed for this to work ?
>>
> I can send you the patches separately, but this driver is designed to
> work with/without DT.

This is not a driver though, it should be a library (or so) for drivers
to use.

>>>
>>>>
>>>>>
>>>>> Why this cannot be used by other driver?
>>>> I don't understand the question.
>>>>
>>> I means why u say this firmware loader cannot be used by other
>>> drivers.
>>> I have tested with FPGA Manager driver(as caller).
>> It must be used by other drivers, that is the point.
>>
> yes, it can be used by FPGA manager driver.
> 


-- 
Best regards,
Marek Vasut

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

[Chee, Tien Fong]

On Thu, 2018-06-07 at 12:29 +0200, Marek Vasut wrote:
> On 06/07/2018 12:11 PM, Chee, Tien Fong wrote:
> > 
> > On Thu, 2018-06-07 at 11:50 +0200, Marek Vasut wrote:
> > > 
> > > On 06/07/2018 11:45 AM, Chee, Tien Fong wrote:
> > > > 
> > > > 
> > > > On Thu, 2018-06-07 at 10:41 +0200, Marek Vasut wrote:
> > > > > 
> > > > > 
> > > > > On 06/07/2018 10:36 AM, Chee, Tien Fong wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > On Thu, 2018-06-07 at 08:51 +0200, Marek Vasut wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > On 06/07/2018 06:04 AM, Chee, Tien Fong wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > On Wed, 2018-06-06 at 10:39 +0200, Marek Vasut wrote:
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > On 05/24/2018 07:04 AM, 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
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > ---
> > > > > > > > > [...]
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > +static int fs_loader_probe(struct udevice *dev)
> > > > > > > > > > +{
> > > > > > > > > > +	return 0;
> > > > > > > > > > +};
> > > > > > > > > > +
> > > > > > > > > > +static const struct udevice_id fs_loader_ids[] = {
> > > > > > > > > > +	{ .compatible = "fs_loader"},
> > > > > > > > > Why exactly is there a DT compatible for a firmware
> > > > > > > > > loader?
> > > > > > > > > 
> > > > > > > > Correct me if i'm wrong, this is required to look the
> > > > > > > > platform
> > > > > > > > data
> > > > > > > > from DTS, right? Details of DTS in patch 2.
> > > > > > > How so ? The FW loader should behave as a library for
> > > > > > > other
> > > > > > > drivers
> > > > > > > to
> > > > > > > use, not like a driver.
> > > > > > > 
> > > > > > The fs_loader node in DTS just provide a way for user to
> > > > > > tell
> > > > > > the
> > > > > > firmware loader what storage device and default partition
> > > > > > to
> > > > > > load
> > > > > > data
> > > > > > from. Default partition can be overriden through the
> > > > > > variable
> > > > > > environment.
> > > > > So that's sitting in the chosen node ? But why do you need to
> > > > > match
> > > > > on it ?
> > > > > 
> > > > This is new device tree binding for firmware loader called
> > > > fs_loader
> > > > node. firmware loader need to know where is the storage device
> > > > it
> > > > need
> > > > to load from. Those storage device would be defined in
> > > > fs_loader
> > > > node.
> > > This is a configuration. You do not need (new) a DT compatible
> > > for
> > > that.
> > > So why is the DT compatible even needed in the FW loader at all ?
> > > 
> > I thought DT compatible is used by driver to find the fs_loader
> > node in
> > DTS. May be i am wrong.
> There should be no FW loader in the DTS. Why would there be one ?
> 
 I added DTS support for user to define storage type and default
partition. So you want me to remove the DTS?
Removing the DTS, then user can only set storage type and partition
through dev instance. So, this design OK for you?
> > 
> > > 
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > Caller/other drivers can create different firmware loader
> > > > > > instance
> > > > > > based on different fs_loader node(different storage device)
> > > > > > for
> > > > > > their
> > > > > > loading purpose.
> > > > > They can, but that could be done even without the DT
> > > > > compatible.
> > > > > 
> > > > Yes, once you get dev, you can set the storage type and
> > > > partiiton
> > > > attributes through accessing dev->platdata.
> > > Can you show me the DT patch needed for this to work ?
> > > 
> > I can send you the patches separately, but this driver is designed
> > to
> > work with/without DT.
> This is not a driver though, it should be a library (or so) for
> drivers
> to use.
> 
Yes, this is designed for other drivers to use.
> > 
> > > 
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > Why this cannot be used by other driver?
> > > > > I don't understand the question.
> > > > > 
> > > > I means why u say this firmware loader cannot be used by other
> > > > drivers.
> > > > I have tested with FPGA Manager driver(as caller).
> > > It must be used by other drivers, that is the point.
> > > 
> > yes, it can be used by FPGA manager driver.
> > 
> 

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

[Marek Vasut]

On 06/11/2018 07:01 AM, Chee, Tien Fong wrote:
[...]
>>>> This is a configuration. You do not need (new) a DT compatible
>>>> for
>>>> that.
>>>> So why is the DT compatible even needed in the FW loader at all ?
>>>>
>>> I thought DT compatible is used by driver to find the fs_loader
>>> node in
>>> DTS. May be i am wrong.
>> There should be no FW loader in the DTS. Why would there be one ?
>>
>  I added DTS support for user to define storage type and default
> partition. So you want me to remove the DTS?
> Removing the DTS, then user can only set storage type and partition
> through dev instance. So, this design OK for you?

How does Linux do it ?

[...]
-- 
Best regards,
Marek Vasut

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

[Chee, Tien Fong]

On Mon, 2018-06-11 at 11:39 +0200, Marek Vasut wrote:
> On 06/11/2018 07:01 AM, Chee, Tien Fong wrote:
> [...]
> > 
> > > 
> > > > 
> > > > > 
> > > > > This is a configuration. You do not need (new) a DT
> > > > > compatible
> > > > > for
> > > > > that.
> > > > > So why is the DT compatible even needed in the FW loader at
> > > > > all ?
> > > > > 
> > > > I thought DT compatible is used by driver to find the fs_loader
> > > > node in
> > > > DTS. May be i am wrong.
> > > There should be no FW loader in the DTS. Why would there be one ?
> > > 
> >  I added DTS support for user to define storage type and default
> > partition. So you want me to remove the DTS?
> > Removing the DTS, then user can only set storage type and partition
> > through dev instance. So, this design OK for you?
> How does Linux do it ?
> 
Linux firmware loading method is different with this
Linux loading firmware with
1. Firmware search paths - which is hardcoded in root file system
2. Built-in firmware - part of kernel binaries

This patch loading firmware with
1. Storage type and partition specified in DTS, but storage type can
be overridden dev instance and partition through U-boot variable
environment.

Or:

2. Storage type can be set through dev instance, and partition through
U-boot variable environment. No DTS is required.

> [...]

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

[Marek Vasut]

On 06/11/2018 01:53 PM, Chee, Tien Fong wrote:
> On Mon, 2018-06-11 at 11:39 +0200, Marek Vasut wrote:
>> On 06/11/2018 07:01 AM, Chee, Tien Fong wrote:
>> [...]
>>>
>>>>
>>>>>
>>>>>>
>>>>>> This is a configuration. You do not need (new) a DT
>>>>>> compatible
>>>>>> for
>>>>>> that.
>>>>>> So why is the DT compatible even needed in the FW loader at
>>>>>> all ?
>>>>>>
>>>>> I thought DT compatible is used by driver to find the fs_loader
>>>>> node in
>>>>> DTS. May be i am wrong.
>>>> There should be no FW loader in the DTS. Why would there be one ?
>>>>
>>>  I added DTS support for user to define storage type and default
>>> partition. So you want me to remove the DTS?
>>> Removing the DTS, then user can only set storage type and partition
>>> through dev instance. So, this design OK for you?
>> How does Linux do it ?
>>
> Linux firmware loading method is different with this
> Linux loading firmware with
> 1. Firmware search paths - which is hardcoded in root file system
> 2. Built-in firmware - part of kernel binaries
> 
> This patch loading firmware with
> 1. Storage type and partition specified in DTS, but storage type can
> be overridden dev instance and partition through U-boot variable
> environment.
> 
> Or:
> 
> 2. Storage type can be set through dev instance, and partition through
> U-boot variable environment. No DTS is required.

And why can we not do as Linux does ?

-- 
Best regards,
Marek Vasut

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

[Chee, Tien Fong]

On Mon, 2018-06-11 at 13:55 +0200, Marek Vasut wrote:
> On 06/11/2018 01:53 PM, Chee, Tien Fong wrote:
> > 
> > On Mon, 2018-06-11 at 11:39 +0200, Marek Vasut wrote:
> > > 
> > > On 06/11/2018 07:01 AM, Chee, Tien Fong wrote:
> > > [...]
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > This is a configuration. You do not need (new) a DT
> > > > > > > compatible
> > > > > > > for
> > > > > > > that.
> > > > > > > So why is the DT compatible even needed in the FW loader
> > > > > > > at
> > > > > > > all ?
> > > > > > > 
> > > > > > I thought DT compatible is used by driver to find the
> > > > > > fs_loader
> > > > > > node in
> > > > > > DTS. May be i am wrong.
> > > > > There should be no FW loader in the DTS. Why would there be
> > > > > one ?
> > > > > 
> > > >  I added DTS support for user to define storage type and
> > > > default
> > > > partition. So you want me to remove the DTS?
> > > > Removing the DTS, then user can only set storage type and
> > > > partition
> > > > through dev instance. So, this design OK for you?
> > > How does Linux do it ?
> > > 
> > Linux firmware loading method is different with this
> > Linux loading firmware with
> > 1. Firmware search paths - which is hardcoded in root file system
> > 2. Built-in firmware - part of kernel binaries
> > 
> > This patch loading firmware with
> > 1. Storage type and partition specified in DTS, but storage type
> > can
> > be overridden dev instance and partition through U-boot variable
> > environment.
> > 
> > Or:
> > 
> > 2. Storage type can be set through dev instance, and partition
> > through
> > U-boot variable environment. No DTS is required.
> And why can we not do as Linux does ?
> 
Because we don't have root file system, but i have mimicked the search
path in our variable environment, but with both storage type and
partition.

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

[Marek Vasut]

On 06/11/2018 02:42 PM, Chee, Tien Fong wrote:
> On Mon, 2018-06-11 at 13:55 +0200, Marek Vasut wrote:
>> On 06/11/2018 01:53 PM, Chee, Tien Fong wrote:
>>>
>>> On Mon, 2018-06-11 at 11:39 +0200, Marek Vasut wrote:
>>>>
>>>> On 06/11/2018 07:01 AM, Chee, Tien Fong wrote:
>>>> [...]
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> This is a configuration. You do not need (new) a DT
>>>>>>>> compatible
>>>>>>>> for
>>>>>>>> that.
>>>>>>>> So why is the DT compatible even needed in the FW loader
>>>>>>>> at
>>>>>>>> all ?
>>>>>>>>
>>>>>>> I thought DT compatible is used by driver to find the
>>>>>>> fs_loader
>>>>>>> node in
>>>>>>> DTS. May be i am wrong.
>>>>>> There should be no FW loader in the DTS. Why would there be
>>>>>> one ?
>>>>>>
>>>>>  I added DTS support for user to define storage type and
>>>>> default
>>>>> partition. So you want me to remove the DTS?
>>>>> Removing the DTS, then user can only set storage type and
>>>>> partition
>>>>> through dev instance. So, this design OK for you?
>>>> How does Linux do it ?
>>>>
>>> Linux firmware loading method is different with this
>>> Linux loading firmware with
>>> 1. Firmware search paths - which is hardcoded in root file system
>>> 2. Built-in firmware - part of kernel binaries
>>>
>>> This patch loading firmware with
>>> 1. Storage type and partition specified in DTS, but storage type
>>> can
>>> be overridden dev instance and partition through U-boot variable
>>> environment.
>>>
>>> Or:
>>>
>>> 2. Storage type can be set through dev instance, and partition
>>> through
>>> U-boot variable environment. No DTS is required.
>> And why can we not do as Linux does ?
>>
> Because we don't have root file system, but i have mimicked the search
> path in our variable environment, but with both storage type and
> partition.

OK, so user can configure environment variable to inform U-Boot where to
look for firmware, good (although, this probably fails in early env,
dunno of that's a problem).

But why do we need the DT part of things ? I don't think we do need it
at all.

-- 
Best regards,
Marek Vasut

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

[Chee, Tien Fong]

On Mon, 2018-06-11 at 15:38 +0200, Marek Vasut wrote:
> On 06/11/2018 02:42 PM, Chee, Tien Fong wrote:
> > 
> > On Mon, 2018-06-11 at 13:55 +0200, Marek Vasut wrote:
> > > 
> > > On 06/11/2018 01:53 PM, Chee, Tien Fong wrote:
> > > > 
> > > > 
> > > > On Mon, 2018-06-11 at 11:39 +0200, Marek Vasut wrote:
> > > > > 
> > > > > 
> > > > > On 06/11/2018 07:01 AM, Chee, Tien Fong wrote:
> > > > > [...]
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > This is a configuration. You do not need (new) a DT
> > > > > > > > > compatible
> > > > > > > > > for
> > > > > > > > > that.
> > > > > > > > > So why is the DT compatible even needed in the FW
> > > > > > > > > loader
> > > > > > > > > at
> > > > > > > > > all ?
> > > > > > > > > 
> > > > > > > > I thought DT compatible is used by driver to find the
> > > > > > > > fs_loader
> > > > > > > > node in
> > > > > > > > DTS. May be i am wrong.
> > > > > > > There should be no FW loader in the DTS. Why would there
> > > > > > > be
> > > > > > > one ?
> > > > > > > 
> > > > > >  I added DTS support for user to define storage type and
> > > > > > default
> > > > > > partition. So you want me to remove the DTS?
> > > > > > Removing the DTS, then user can only set storage type and
> > > > > > partition
> > > > > > through dev instance. So, this design OK for you?
> > > > > How does Linux do it ?
> > > > > 
> > > > Linux firmware loading method is different with this
> > > > Linux loading firmware with
> > > > 1. Firmware search paths - which is hardcoded in root file
> > > > system
> > > > 2. Built-in firmware - part of kernel binaries
> > > > 
> > > > This patch loading firmware with
> > > > 1. Storage type and partition specified in DTS, but storage
> > > > type
> > > > can
> > > > be overridden dev instance and partition through U-boot
> > > > variable
> > > > environment.
> > > > 
> > > > Or:
> > > > 
> > > > 2. Storage type can be set through dev instance, and partition
> > > > through
> > > > U-boot variable environment. No DTS is required.
> > > And why can we not do as Linux does ?
> > > 
> > Because we don't have root file system, but i have mimicked the
> > search
> > path in our variable environment, but with both storage type and
> > partition.
> OK, so user can configure environment variable to inform U-Boot where
> to
> look for firmware, good (although, this probably fails in early env,
> dunno of that's a problem).
> 
Without DTS, then you need to configure env variable, so that SPL and
U-boot only know what storage type and partiton to look for firmware.

> But why do we need the DT part of things ? I don't think we do need
> it
> at all.
> 
Leverage the flexibility and benefits of the DT such as changing both
storage type and partitions without changing the codes. 

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

[Marek Vasut]

On 06/11/2018 03:55 PM, Chee, Tien Fong wrote:
[...]
>>>>> Linux firmware loading method is different with this
>>>>> Linux loading firmware with
>>>>> 1. Firmware search paths - which is hardcoded in root file
>>>>> system
>>>>> 2. Built-in firmware - part of kernel binaries
>>>>>
>>>>> This patch loading firmware with
>>>>> 1. Storage type and partition specified in DTS, but storage
>>>>> type
>>>>> can
>>>>> be overridden dev instance and partition through U-boot
>>>>> variable
>>>>> environment.
>>>>>
>>>>> Or:
>>>>>
>>>>> 2. Storage type can be set through dev instance, and partition
>>>>> through
>>>>> U-boot variable environment. No DTS is required.
>>>> And why can we not do as Linux does ?
>>>>
>>> Because we don't have root file system, but i have mimicked the
>>> search
>>> path in our variable environment, but with both storage type and
>>> partition.
>> OK, so user can configure environment variable to inform U-Boot where
>> to
>> look for firmware, good (although, this probably fails in early env,
>> dunno of that's a problem).
>>
> Without DTS, then you need to configure env variable, so that SPL and
> U-boot only know what storage type and partiton to look for firmware.

I guess that's fine ?

>> But why do we need the DT part of things ? I don't think we do need
>> it
>> at all.
>>
> Leverage the flexibility and benefits of the DT such as changing both
> storage type and partitions without changing the codes. 

DT is a hardware description. Does this describe hardware ? No

-- 
Best regards,
Marek Vasut

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

[Chee, Tien Fong]

On Mon, 2018-06-11 at 16:03 +0200, Marek Vasut wrote:
> On 06/11/2018 03:55 PM, Chee, Tien Fong wrote:
> [...]
> > 
> > > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > Linux firmware loading method is different with this
> > > > > > Linux loading firmware with
> > > > > > 1. Firmware search paths - which is hardcoded in root file
> > > > > > system
> > > > > > 2. Built-in firmware - part of kernel binaries
> > > > > > 
> > > > > > This patch loading firmware with
> > > > > > 1. Storage type and partition specified in DTS, but storage
> > > > > > type
> > > > > > can
> > > > > > be overridden dev instance and partition through U-boot
> > > > > > variable
> > > > > > environment.
> > > > > > 
> > > > > > Or:
> > > > > > 
> > > > > > 2. Storage type can be set through dev instance, and
> > > > > > partition
> > > > > > through
> > > > > > U-boot variable environment. No DTS is required.
> > > > > And why can we not do as Linux does ?
> > > > > 
> > > > Because we don't have root file system, but i have mimicked the
> > > > search
> > > > path in our variable environment, but with both storage type
> > > > and
> > > > partition.
> > > OK, so user can configure environment variable to inform U-Boot
> > > where
> > > to
> > > look for firmware, good (although, this probably fails in early
> > > env,
> > > dunno of that's a problem).
> > > 
> > Without DTS, then you need to configure env variable, so that SPL
> > and
> > U-boot only know what storage type and partiton to look for
> > firmware.
> I guess that's fine ?
> 
Yes, it is already supported in this patches.
> > 
> > > 
> > > But why do we need the DT part of things ? I don't think we do
> > > need
> > > it
> > > at all.
> > > 
> > Leverage the flexibility and benefits of the DT such as changing
> > both
> > storage type and partitions without changing the codes. 
> DT is a hardware description. Does this describe hardware ? No
> 
Okay, then i will remove DT part.

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

[Marek Vasut]

On 06/11/2018 04:13 PM, Chee, Tien Fong wrote:
> On Mon, 2018-06-11 at 16:03 +0200, Marek Vasut wrote:
>> On 06/11/2018 03:55 PM, Chee, Tien Fong wrote:
>> [...]
>>>
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>> Linux firmware loading method is different with this
>>>>>>> Linux loading firmware with
>>>>>>> 1. Firmware search paths - which is hardcoded in root file
>>>>>>> system
>>>>>>> 2. Built-in firmware - part of kernel binaries
>>>>>>>
>>>>>>> This patch loading firmware with
>>>>>>> 1. Storage type and partition specified in DTS, but storage
>>>>>>> type
>>>>>>> can
>>>>>>> be overridden dev instance and partition through U-boot
>>>>>>> variable
>>>>>>> environment.
>>>>>>>
>>>>>>> Or:
>>>>>>>
>>>>>>> 2. Storage type can be set through dev instance, and
>>>>>>> partition
>>>>>>> through
>>>>>>> U-boot variable environment. No DTS is required.
>>>>>> And why can we not do as Linux does ?
>>>>>>
>>>>> Because we don't have root file system, but i have mimicked the
>>>>> search
>>>>> path in our variable environment, but with both storage type
>>>>> and
>>>>> partition.
>>>> OK, so user can configure environment variable to inform U-Boot
>>>> where
>>>> to
>>>> look for firmware, good (although, this probably fails in early
>>>> env,
>>>> dunno of that's a problem).
>>>>
>>> Without DTS, then you need to configure env variable, so that SPL
>>> and
>>> U-boot only know what storage type and partiton to look for
>>> firmware.
>> I guess that's fine ?
>>
> Yes, it is already supported in this patches.
>>>
>>>>
>>>> But why do we need the DT part of things ? I don't think we do
>>>> need
>>>> it
>>>> at all.
>>>>
>>> Leverage the flexibility and benefits of the DT such as changing
>>> both
>>> storage type and partitions without changing the codes. 
>> DT is a hardware description. Does this describe hardware ? No
>>
> Okay, then i will remove DT part.

Wait for feedback from sjg at least.

-- 
Best regards,
Marek Vasut

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

[Simon Glass]

Hi,

On 11 June 2018 at 08:15, Marek Vasut <marex@denx.de> wrote:
> On 06/11/2018 04:13 PM, Chee, Tien Fong wrote:
>> On Mon, 2018-06-11 at 16:03 +0200, Marek Vasut wrote:
>>> On 06/11/2018 03:55 PM, Chee, Tien Fong wrote:
>>> [...]
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> Linux firmware loading method is different with this
>>>>>>>> Linux loading firmware with
>>>>>>>> 1. Firmware search paths - which is hardcoded in root file
>>>>>>>> system
>>>>>>>> 2. Built-in firmware - part of kernel binaries
>>>>>>>>
>>>>>>>> This patch loading firmware with
>>>>>>>> 1. Storage type and partition specified in DTS, but storage
>>>>>>>> type
>>>>>>>> can
>>>>>>>> be overridden dev instance and partition through U-boot
>>>>>>>> variable
>>>>>>>> environment.
>>>>>>>>
>>>>>>>> Or:
>>>>>>>>
>>>>>>>> 2. Storage type can be set through dev instance, and
>>>>>>>> partition
>>>>>>>> through
>>>>>>>> U-boot variable environment. No DTS is required.
>>>>>>> And why can we not do as Linux does ?
>>>>>>>
>>>>>> Because we don't have root file system, but i have mimicked the
>>>>>> search
>>>>>> path in our variable environment, but with both storage type
>>>>>> and
>>>>>> partition.
>>>>> OK, so user can configure environment variable to inform U-Boot
>>>>> where
>>>>> to
>>>>> look for firmware, good (although, this probably fails in early
>>>>> env,
>>>>> dunno of that's a problem).
>>>>>
>>>> Without DTS, then you need to configure env variable, so that SPL
>>>> and
>>>> U-boot only know what storage type and partiton to look for
>>>> firmware.
>>> I guess that's fine ?
>>>
>> Yes, it is already supported in this patches.
>>>>
>>>>>
>>>>> But why do we need the DT part of things ? I don't think we do
>>>>> need
>>>>> it
>>>>> at all.
>>>>>
>>>> Leverage the flexibility and benefits of the DT such as changing
>>>> both
>>>> storage type and partitions without changing the codes.
>>> DT is a hardware description. Does this describe hardware ? No
>>>
>> Okay, then i will remove DT part.
>
> Wait for feedback from sjg at least.

I think using device tree is fine for this, but I suggest that the
binding file (when you add it) should mention that the node for this
driver should go in /chosen. That's what we use for settings, etc.

Things to do:
- Add a sandbox driver for this uclass, and a test in test/dm/...
- Add some documentation about it somehwere
- Add a device-tree binding file - see doc/device-tree-bindings

Regards,
Simon

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

[Chee, Tien Fong]

On Mon, 2018-06-11 at 13:38 -0600, Simon Glass wrote:
> Hi,
> 
> On 11 June 2018 at 08:15, Marek Vasut <marex@denx.de> wrote:
> > 
> > On 06/11/2018 04:13 PM, Chee, Tien Fong wrote:
> > > 
> > > On Mon, 2018-06-11 at 16:03 +0200, Marek Vasut wrote:
> > > > 
> > > > On 06/11/2018 03:55 PM, Chee, Tien Fong wrote:
> > > > [...]
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Linux firmware loading method is different with this
> > > > > > > > > Linux loading firmware with
> > > > > > > > > 1. Firmware search paths - which is hardcoded in root
> > > > > > > > > file
> > > > > > > > > system
> > > > > > > > > 2. Built-in firmware - part of kernel binaries
> > > > > > > > > 
> > > > > > > > > This patch loading firmware with
> > > > > > > > > 1. Storage type and partition specified in DTS, but
> > > > > > > > > storage
> > > > > > > > > type
> > > > > > > > > can
> > > > > > > > > be overridden dev instance and partition through U-
> > > > > > > > > boot
> > > > > > > > > variable
> > > > > > > > > environment.
> > > > > > > > > 
> > > > > > > > > Or:
> > > > > > > > > 
> > > > > > > > > 2. Storage type can be set through dev instance, and
> > > > > > > > > partition
> > > > > > > > > through
> > > > > > > > > U-boot variable environment. No DTS is required.
> > > > > > > > And why can we not do as Linux does ?
> > > > > > > > 
> > > > > > > Because we don't have root file system, but i have
> > > > > > > mimicked the
> > > > > > > search
> > > > > > > path in our variable environment, but with both storage
> > > > > > > type
> > > > > > > and
> > > > > > > partition.
> > > > > > OK, so user can configure environment variable to inform U-
> > > > > > Boot
> > > > > > where
> > > > > > to
> > > > > > look for firmware, good (although, this probably fails in
> > > > > > early
> > > > > > env,
> > > > > > dunno of that's a problem).
> > > > > > 
> > > > > Without DTS, then you need to configure env variable, so that
> > > > > SPL
> > > > > and
> > > > > U-boot only know what storage type and partiton to look for
> > > > > firmware.
> > > > I guess that's fine ?
> > > > 
> > > Yes, it is already supported in this patches.
> > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > But why do we need the DT part of things ? I don't think we
> > > > > > do
> > > > > > need
> > > > > > it
> > > > > > at all.
> > > > > > 
> > > > > Leverage the flexibility and benefits of the DT such as
> > > > > changing
> > > > > both
> > > > > storage type and partitions without changing the codes.
> > > > DT is a hardware description. Does this describe hardware ? No
> > > > 
> > > Okay, then i will remove DT part.
> > Wait for feedback from sjg at least.
> I think using device tree is fine for this, but I suggest that the
> binding file (when you add it) should mention that the node for this
> driver should go in /chosen. That's what we use for settings, etc.
> 
Okay, sure. So, we agree supporting device tree is good to go.

> Things to do:
> - Add a sandbox driver for this uclass, and a test in test/dm/...
> - Add some documentation about it somehwere
> - Add a device-tree binding file - see doc/device-tree-bindings
> 
Okay, let me try.

> Regards,
> Simon