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

[U-Boot] [PATCH v3 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 Simon and Marek in [v2].
Patch set for sandbox will be sent out separately.

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

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

v2 -> v3 changes
----------------
- Node for this driver go in /chosen
- Update firmware doc and device tree binding doc.

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/chosen.txt         |  22 +++
 doc/device-tree-bindings/misc/fs_loader.txt |  52 ++++++
 doc/driver-model/fs_firmware_loader.txt     | 129 +++++++++++++++
 drivers/misc/Kconfig                        |  10 ++
 drivers/misc/Makefile                       |   1 +
 drivers/misc/fs_loader.c                    | 238 ++++++++++++++++++++++++++++
 include/dm/uclass-id.h                      |   1 +
 include/fs_loader.h                         |  28 ++++
 8 files changed, 481 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 v3 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 storage device and partition 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 | 129 ++++++++++++++++++++++++++++++++
 1 file changed, 129 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..102e14b
--- /dev/null
+++ b/doc/driver-model/fs_firmware_loader.txt
@@ -0,0 +1,129 @@
+# 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, 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
+-------------------------------------------------------
+	For passing data like storage device and partition where the firmware
+	loading from to the firmware loader driver, those data could be
+	defined in fs_loader node as shown in below:
+
+	fs_loader0: fs_loader at 0 {
+		u-boot,dm-pre-reloc;
+		compatible = "fs_loader";
+		storage_device = "mmc";
+		devpart = "0:1";
+	};
+
+	Then, firmware_loader property would be set with the path of fs_loader
+	node under /chosen node such as:
+	/{
+		chosen {
+			firmware_loader = &fs_loader0;
+		};
+	};
+
+	Example of storage type and device partition search set for mmc, usb,
+	sata and ubi as shown in below:
+	Example for mmc:
+	fs_loader0: fs_loader at 0 {
+		u-boot,dm-pre-reloc;
+		compatible = "fs_loader";
+		storage_device = "mmc";
+		devpart = "0:1";
+	};
+
+	Example for usb:
+	fs_loader1: fs_loader at 1 {
+		u-boot,dm-pre-reloc;
+		compatible = "fs_loader";
+		storage_device = "usb";
+		devpart = "0:1";
+	};
+
+	Example for sata:
+	fs_loader2: fs_loader at 2 {
+		u-boot,dm-pre-reloc;
+		compatible = "fs_loader";
+		storage_device = "sata";
+		devpart = "0:1";
+	};
+
+	Example for ubi:
+	fs_loader3: fs_loader at 3 {
+		u-boot,dm-pre-reloc;
+		compatible = "fs_loader";
+		storage_device = "ubi";
+		mtdpart = "UBI",
+		ubivol = "ubi0";
+	};
+
+
+	However, the property of devpart, mtdpart, and ubivol values from
+	fs_loader node 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 device_platdata *plat,
+				 struct firmware **firmware_p,
+				 const char *name,
+				 void *buf, size_t size, u32 offset)
+--------------------------------------------------------------------
+Load firmware into a previously allocated buffer
+
+Parameters:
+
+1. struct device_platdata *plat
+	Platform data such as storage and partition firmware loading from
+
+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
+
+Example of creating firmware loader instance and calling
+request_firmware_into_buf API:
+	if (uclass_get_device(UCLASS_FS_FIRMWARE_LOADER, 0, &dev)) {
+		request_firmware_into_buf(dev->plat, &fw, filename,
+			 buffer_location, buffer_size, offset_ofreading);
+	}
-- 
2.2.0

[U-Boot] [PATCH v3 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/chosen.txt         | 22 ++++++++++++
 doc/device-tree-bindings/misc/fs_loader.txt | 52 +++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+)
 create mode 100644 doc/device-tree-bindings/misc/fs_loader.txt

diff --git a/doc/device-tree-bindings/chosen.txt b/doc/device-tree-bindings/chosen.txt
index c96b8f7..738673c 100644
--- a/doc/device-tree-bindings/chosen.txt
+++ b/doc/device-tree-bindings/chosen.txt
@@ -73,3 +73,25 @@ Example
 		u-boot,spl-boot-order = "same-as-spl", &sdmmc, "/sdhci at fe330000";
 	};
 };
+
+firmware-loader property
+------------------------
+Multiple file system firmware loader nodes could be defined in device trees for
+multiple storage type and their default partition, then a property
+"firmware-loader" can be used to pass default firmware loader
+node(default storage type) to the firmware loader driver.
+
+Example
+-------
+/ {
+	chosen {
+		firmware-loader = &fs_loader0;
+	};
+
+	fs_loader0: fs_loader at 0 {
+		u-boot,dm-pre-reloc;
+		compatible = "fs_loader";
+		storage_device = "mmc";
+		devpart = "0:1";
+	};
+};
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..78bea66
--- /dev/null
+++ b/doc/device-tree-bindings/misc/fs_loader.txt
@@ -0,0 +1,52 @@
+* 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 storage device and partition search set for mmc, usb, sata and
+ubi in device tree source as shown in below:
+
+	Example of storage type and device partition search set for mmc, usb,
+	sata and ubi as shown in below:
+	Example for mmc:
+	fs_loader0: fs_loader at 0 {
+		u-boot,dm-pre-reloc;
+		compatible = "fs_loader";
+		storage_device = "mmc";
+		devpart = "0:1";
+	};
+
+	Example for usb:
+	fs_loader1: fs_loader at 1 {
+		u-boot,dm-pre-reloc;
+		compatible = "fs_loader";
+		storage_device = "usb";
+		devpart = "0:1";
+	};
+
+	Example for sata:
+	fs_loader2: fs_loader at 2 {
+		u-boot,dm-pre-reloc;
+		compatible = "fs_loader";
+		storage_device = "sata";
+		devpart = "0:1";
+	};
+
+	Example for ubi:
+	fs_loader3: fs_loader at 3 {
+		u-boot,dm-pre-reloc;
+		compatible = "fs_loader";
+		storage_device = "ubi";
+		mtdpart = "UBI",
+		ubivol = "ubi0";
+	};
-- 
2.2.0

[U-Boot] [PATCH v3 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     |  10 ++
 drivers/misc/Makefile    |   1 +
 drivers/misc/fs_loader.c | 238 +++++++++++++++++++++++++++++++++++++++++++++++
 include/dm/uclass-id.h   |   1 +
 include/fs_loader.h      |  28 ++++++
 5 files changed, 278 insertions(+)
 create mode 100644 drivers/misc/fs_loader.c
 create mode 100644 include/fs_loader.h

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 17b3a80..4163b4f 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -277,4 +277,14 @@ config GDSYS_RXAUI_CTRL
 	depends on MISC
 	help
 	  Support gdsys FPGA's RXAUI control.
+
+config FS_LOADER
+	bool "Enable loader driver for file system"
+	help
+	  This is file system generic loader which can be used to load
+	  the file image from the storage into target such as memory.
+
+	  The consumer driver would then use this loader to program whatever,
+	  ie. the FPGA device.
+
 endmenu
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 4ce9d21..67a36f8 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -54,3 +54,4 @@ obj-$(CONFIG_STM32_RCC) += stm32_rcc.o
 obj-$(CONFIG_STM32MP_FUSE) += stm32mp_fuse.o
 obj-$(CONFIG_SYS_DPAA_QBMAN) += fsl_portals.o
 obj-$(CONFIG_GDSYS_RXAUI_CTRL) += gdsys_rxaui_ctrl.o
+obj-$(CONFIG_FS_LOADER) += fs_loader.o
diff --git a/drivers/misc/fs_loader.c b/drivers/misc/fs_loader.c
new file mode 100644
index 0000000..0dc385f
--- /dev/null
+++ b/drivers/misc/fs_loader.c
@@ -0,0 +1,238 @@
+/*
+ * Copyright (C) 2018 Intel Corporation <www.intel.com>
+ *
+ * SPDX-License-Identifier:    GPL-2.0
+ */
+#include <common.h>
+#include <dm.h>
+#include <errno.h>
+#include <fs.h>
+#include <fs_loader.h>
+#include <linux/string.h>
+#include <malloc.h>
+#include <spl.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+struct firmware_priv {
+	const char *name;	/* Filename */
+	u32 offset;		/* Offset of reading a file */
+};
+
+static int select_fs_dev(struct device_platdata *plat)
+{
+	int ret;
+
+	if (!strcmp("mmc", plat->name)) {
+		ret = fs_set_blk_dev("mmc", plat->devpart, FS_TYPE_ANY);
+	} else if (!strcmp("usb", plat->name)) {
+		ret = fs_set_blk_dev("usb", plat->devpart, FS_TYPE_ANY);
+	} else if (!strcmp("sata", plat->name)) {
+		ret = fs_set_blk_dev("sata", plat->devpart, FS_TYPE_ANY);
+	} else if (!strcmp("ubi", plat->name)) {
+		if (plat->ubivol)
+			ret = fs_set_blk_dev("ubi", NULL, FS_TYPE_UBIFS);
+		else
+			ret = -ENODEV;
+	} else {
+		debug("Error: unsupported storage device.\n");
+		return -ENODEV;
+	}
+
+	if (ret)
+		debug("Error: could not access storage.\n");
+
+	return ret;
+}
+
+static void set_storage_devpart(struct device_platdata *plat, char *devpart)
+{
+	plat->devpart = devpart;
+}
+
+static void set_storage_mtdpart(struct device_platdata *plat, char *mtdpart)
+{
+	plat->mtdpart = mtdpart;
+}
+
+static void set_storage_ubivol(struct device_platdata *plat, char *ubivol)
+{
+	plat->ubivol = ubivol;
+}
+
+/**
+ * _request_firmware_prepare - Prepare firmware struct.
+ *
+ * @firmware_p: Pointer to pointer to firmware image.
+ * @name: Name of firmware file.
+ * @dbuf: Address of buffer to load firmware into.
+ * @size: Size of buffer.
+ * @offset: Offset of a file for start reading into buffer.
+ *
+ * Return: Negative value if fail, 0 for successful.
+ */
+static int _request_firmware_prepare(struct firmware **firmware_p,
+				    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.
+ * @plat: Platform data such as storage and partition firmware loading from.
+ * @firmware_p: pointer to firmware image.
+ *
+ * Return: Size of total read, negative value when error.
+ */
+static int fw_get_filesystem_firmware(struct device_platdata *plat,
+				     struct firmware *firmware_p)
+{
+	struct firmware_priv *fw_priv = NULL;
+	loff_t actread;
+	char *dev_part, *ubi_mtdpart, *ubi_volume;
+	int ret;
+
+	dev_part = env_get("fw_dev_part");
+	if (dev_part)
+		set_storage_devpart(plat, dev_part);
+
+	ubi_mtdpart = env_get("fw_ubi_mtdpart");
+	if (ubi_mtdpart)
+		set_storage_mtdpart(plat, ubi_mtdpart);
+
+	ubi_volume = env_get("fw_ubi_volume");
+	if (ubi_volume)
+		set_storage_ubivol(plat, ubi_volume);
+
+	ret = select_fs_dev(plat);
+	if (ret)
+		goto out;
+
+	fw_priv = firmware_p->priv;
+
+	ret = fs_read(fw_priv->name, (ulong)firmware_p->data, fw_priv->offset,
+		     firmware_p->size, &actread);
+	if (ret) {
+		debug("Error: %d Failed to read %s from flash %lld != %d.\n",
+		      ret, fw_priv->name, actread, firmware_p->size);
+	} else {
+		ret = actread;
+	}
+
+out:
+	return ret;
+}
+
+/**
+ * request_firmware_into_buf - Load firmware into a previously allocated buffer.
+ * @plat: Platform data such as storage and partition firmware loading from.
+ * @firmware_p: Pointer to firmware image.
+ * @name: Name of firmware file.
+ * @buf: Address of buffer to load firmware into.
+ * @size: Size of buffer.
+ * @offset: Offset of a file for start reading into buffer.
+ *
+ * The firmware is loaded directly into the buffer pointed to by @buf and
+ * the @firmware_p data member is pointed at @buf.
+ *
+ * Return: Size of total read, negative value when error.
+ */
+int request_firmware_into_buf(struct device_platdata *plat,
+			      struct firmware **firmware_p,
+			      const char *name,
+			      void *buf, size_t size, u32 offset)
+{
+	int ret;
+
+	if (!plat)
+		return -EINVAL;
+
+	ret = _request_firmware_prepare(firmware_p, name, buf, size, offset);
+	if (ret < 0) /* error */
+		return ret;
+
+	ret = fw_get_filesystem_firmware(plat, *firmware_p);
+
+	return ret;
+}
+
+static int fs_loader_ofdata_to_platdata(struct udevice *dev)
+{
+	const char *fs_loader_path;
+
+	fs_loader_path = ofnode_get_chosen_prop("firmware-loader");
+
+	if (fs_loader_path) {
+		ofnode fs_loader_node;
+
+		fs_loader_node = ofnode_path(fs_loader_path);
+		if (ofnode_valid(fs_loader_node)) {
+			struct device_platdata *plat;
+			plat = dev->platdata;
+
+			plat->name = (char *)ofnode_read_string(fs_loader_node,
+					 "storage_device");
+
+			plat->devpart = (char *)ofnode_read_string(
+					 fs_loader_node, "devpart");
+
+			plat->mtdpart = (char *)ofnode_read_string(
+					 fs_loader_node, "mtdpart");
+
+			plat->ubivol = (char *)ofnode_read_string(
+					 fs_loader_node, "ubivol");
+		}
+	}
+
+	return 0;
+}
+
+static int fs_loader_probe(struct udevice *dev)
+{
+	return 0;
+};
+
+static const struct udevice_id fs_loader_ids[] = {
+	{ .compatible = "fs_loader"},
+	{ }
+};
+
+U_BOOT_DRIVER(fs_loader) = {
+	.name			= "fs_loader",
+	.id			= UCLASS_FS_FIRMWARE_LOADER,
+	.of_match		= fs_loader_ids,
+	.probe			= fs_loader_probe,
+	.ofdata_to_platdata	= fs_loader_ofdata_to_platdata,
+	.platdata_auto_alloc_size	= sizeof(struct device_platdata),
+};
+
+UCLASS_DRIVER(fs_loader) = {
+	.id		= UCLASS_FS_FIRMWARE_LOADER,
+	.name		= "fs_loader",
+};
diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
index d7f9df3..39e88ac 100644
--- a/include/dm/uclass-id.h
+++ b/include/dm/uclass-id.h
@@ -36,6 +36,7 @@ enum uclass_id {
 	UCLASS_DMA,		/* Direct Memory Access */
 	UCLASS_EFI,		/* EFI managed devices */
 	UCLASS_ETH,		/* Ethernet device */
+	UCLASS_FS_FIRMWARE_LOADER,		/* Generic loader */
 	UCLASS_GPIO,		/* Bank of general-purpose I/O pins */
 	UCLASS_FIRMWARE,	/* Firmware */
 	UCLASS_I2C,		/* I2C bus */
diff --git a/include/fs_loader.h b/include/fs_loader.h
new file mode 100644
index 0000000..e3e5eeb
--- /dev/null
+++ b/include/fs_loader.h
@@ -0,0 +1,28 @@
+/*
+ * Copyright (C) 2018 Intel Corporation <www.intel.com>
+ *
+ * SPDX-License-Identifier:    GPL-2.0
+ */
+#ifndef _FS_LOADER_H_
+#define _FS_LOADER_H_
+
+#include <dm.h>
+
+struct firmware {
+	size_t size;		/* Size of a file */
+	const u8 *data;		/* Buffer for file */
+	void *priv;		/* Firmware loader private fields */
+};
+
+struct device_platdata {
+	char *name;	/* Such as mmc, usb,and sata. */
+	char *devpart;  /* Use the load command dev:part conventions */
+	char *mtdpart;	/* MTD partition for ubi part */
+	char *ubivol;	/* UBI volume-name for ubifsmount */
+};
+
+int request_firmware_into_buf(struct device_platdata *plat,
+			      struct firmware **firmware_p,
+			      const char *name,
+			      void *buf, size_t size, u32 offset);
+#endif
-- 
2.2.0

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

[Simon Glass]

Hi,

On 25 June 2018 at 07:28,  <tien.fong.chee@intel.com> wrote:
> 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/chosen.txt         | 22 ++++++++++++
>  doc/device-tree-bindings/misc/fs_loader.txt | 52 +++++++++++++++++++++++++++++
>  2 files changed, 74 insertions(+)
>  create mode 100644 doc/device-tree-bindings/misc/fs_loader.txt
>
> diff --git a/doc/device-tree-bindings/chosen.txt b/doc/device-tree-bindings/chosen.txt
> index c96b8f7..738673c 100644
> --- a/doc/device-tree-bindings/chosen.txt
> +++ b/doc/device-tree-bindings/chosen.txt
> @@ -73,3 +73,25 @@ Example
>                 u-boot,spl-boot-order = "same-as-spl", &sdmmc, "/sdhci at fe330000";
>         };
>  };
> +
> +firmware-loader property
> +------------------------
> +Multiple file system firmware loader nodes could be defined in device trees for
> +multiple storage type and their default partition, then a property
> +"firmware-loader" can be used to pass default firmware loader
> +node(default storage type) to the firmware loader driver.
> +
> +Example
> +-------
> +/ {
> +       chosen {
> +               firmware-loader = &fs_loader0;
> +       };
> +
> +       fs_loader0: fs_loader at 0 {

This should be :

> +       fs_loader0: fs-loader at 0 {

since hyphen is used for node and property names. Underscore is used
for phandles (so you have that correct).

> +               u-boot,dm-pre-reloc;
> +               compatible = "fs_loader";

How about u-boot,fs-loader since this is U-Boot specific I think.


> +               storage_device = "mmc";

storage-device

> +               devpart = "0:1";

I think you should use a phandle to the node containing the mmc device to use.

   storage-device = <&mmc_3 1>;

for example (meaning use that MMC, partition 1.

> +       };
> +};
> 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..78bea66
> --- /dev/null
> +++ b/doc/device-tree-bindings/misc/fs_loader.txt
> @@ -0,0 +1,52 @@
> +* 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 storage device and partition search set for mmc, usb, sata and
> +ubi in device tree source as shown in below:
> +
> +       Example of storage type and device partition search set for mmc, usb,
> +       sata and ubi as shown in below:
> +       Example for mmc:
> +       fs_loader0: fs_loader at 0 {
> +               u-boot,dm-pre-reloc;
> +               compatible = "fs_loader";
> +               storage_device = "mmc";
> +               devpart = "0:1";
> +       };
> +
> +       Example for usb:
> +       fs_loader1: fs_loader at 1 {
> +               u-boot,dm-pre-reloc;
> +               compatible = "fs_loader";
> +               storage_device = "usb";
> +               devpart = "0:1";
> +       };
> +
> +       Example for sata:
> +       fs_loader2: fs_loader at 2 {
> +               u-boot,dm-pre-reloc;
> +               compatible = "fs_loader";
> +               storage_device = "sata";
> +               devpart = "0:1";
> +       };
> +
> +       Example for ubi:
> +       fs_loader3: fs_loader at 3 {
> +               u-boot,dm-pre-reloc;
> +               compatible = "fs_loader";
> +               storage_device = "ubi";
> +               mtdpart = "UBI",
> +               ubivol = "ubi0";

I'm not sure about ubi. It needs to refer to a particular device I
suppose. How do we know the mapping from ubi to device?

> +       };
> --
> 2.2.0
>

Regards,
Simon

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

[Simon Glass]

Hi,

On 25 June 2018 at 07: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 | 238 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/dm/uclass-id.h   |   1 +
>  include/fs_loader.h      |  28 ++++++
>  5 files changed, 278 insertions(+)
>  create mode 100644 drivers/misc/fs_loader.c
>  create mode 100644 include/fs_loader.h

This is definitely looking good. I think we need to figure out the
binding to devices, to use phandles instead of strings. Also we need a
sandbox driver and test.

I'm a little worried that you are blazing a trail here, but it is a
valuable trail :-)

Tom, do you have any ideas / thoughts on the design?

>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 17b3a80..4163b4f 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -277,4 +277,14 @@ config GDSYS_RXAUI_CTRL
>         depends on MISC
>         help
>           Support gdsys FPGA's RXAUI control.
> +
> +config FS_LOADER
> +       bool "Enable loader driver for file system"
> +       help
> +         This is file system generic loader which can be used to load
> +         the file image from the storage into target such as memory.
> +
> +         The consumer driver would then use this loader to program whatever,
> +         ie. the FPGA device.
> +
>  endmenu
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 4ce9d21..67a36f8 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -54,3 +54,4 @@ obj-$(CONFIG_STM32_RCC) += stm32_rcc.o
>  obj-$(CONFIG_STM32MP_FUSE) += stm32mp_fuse.o
>  obj-$(CONFIG_SYS_DPAA_QBMAN) += fsl_portals.o
>  obj-$(CONFIG_GDSYS_RXAUI_CTRL) += gdsys_rxaui_ctrl.o
> +obj-$(CONFIG_FS_LOADER) += fs_loader.o
> diff --git a/drivers/misc/fs_loader.c b/drivers/misc/fs_loader.c
> new file mode 100644
> index 0000000..0dc385f
> --- /dev/null
> +++ b/drivers/misc/fs_loader.c
> @@ -0,0 +1,238 @@
> +/*
> + * Copyright (C) 2018 Intel Corporation <www.intel.com>
> + *
> + * SPDX-License-Identifier:    GPL-2.0
> + */
> +#include <common.h>
> +#include <dm.h>
> +#include <errno.h>
> +#include <fs.h>
> +#include <fs_loader.h>
> +#include <linux/string.h>
> +#include <malloc.h>
> +#include <spl.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +struct firmware_priv {
> +       const char *name;       /* Filename */
> +       u32 offset;             /* Offset of reading a file */
> +};
> +
> +static int select_fs_dev(struct device_platdata *plat)
> +{
> +       int ret;
> +
> +       if (!strcmp("mmc", plat->name)) {
> +               ret = fs_set_blk_dev("mmc", plat->devpart, FS_TYPE_ANY);
> +       } else if (!strcmp("usb", plat->name)) {
> +               ret = fs_set_blk_dev("usb", plat->devpart, FS_TYPE_ANY);
> +       } else if (!strcmp("sata", plat->name)) {
> +               ret = fs_set_blk_dev("sata", plat->devpart, FS_TYPE_ANY);

For these I think you can look up the phandle to obtain the device,
then check its uclass to find out its type.

> +       } else if (!strcmp("ubi", plat->name)) {
> +               if (plat->ubivol)
> +                       ret = fs_set_blk_dev("ubi", NULL, FS_TYPE_UBIFS);
> +               else
> +                       ret = -ENODEV;
> +       } else {
> +               debug("Error: unsupported storage device.\n");
> +               return -ENODEV;
> +       }
> +
> +       if (ret)
> +               debug("Error: could not access storage.\n");
> +
> +       return ret;
> +}
> +
> +static void set_storage_devpart(struct device_platdata *plat, char *devpart)
> +{
> +       plat->devpart = devpart;
> +}
> +
> +static void set_storage_mtdpart(struct device_platdata *plat, char *mtdpart)
> +{
> +       plat->mtdpart = mtdpart;
> +}
> +
> +static void set_storage_ubivol(struct device_platdata *plat, char *ubivol)
> +{
> +       plat->ubivol = ubivol;
> +}
> +
> +/**
> + * _request_firmware_prepare - Prepare firmware struct.
> + *
> + * @firmware_p: Pointer to pointer to firmware image.
> + * @name: Name of firmware file.
> + * @dbuf: Address of buffer to load firmware into.
> + * @size: Size of buffer.
> + * @offset: Offset of a file for start reading into buffer.
> + *
> + * Return: Negative value if fail, 0 for successful.
> + */
> +static int _request_firmware_prepare(struct firmware **firmware_p,

Can you please move this to be the last arg and rename to firmwarep?

> +                                   const char *name, void *dbuf,device to ubi
> +                                   size_t size, u32 offset)
> +{
> +       struct firmware *firmware;
> +       struct firmware_priv *fw_priv;
> +
> +       *firmware_p = NULL;
> +
> +       if (!name || name[0] == '\0')
> +               return -EINVAL;
> +
> +       firmware = calloc(1, sizeof(*firmware));
> +       if (!firmware)
> +               return -ENOMEM;
> +
> +       fw_priv = calloc(1, sizeof(*fw_priv));
> +       if (!fw_priv) {
> +               free(firmware);
> +               return -ENOMEM;
> +       }
> +
> +       fw_priv->name = name;
> +       fw_priv->offset = offset;
> +       firmware->data = dbuf;
> +       firmware->size = size;
> +       firmware->priv = fw_priv;
> +       *firmware_p = firmware;
> +
> +       return 0;
> +}
> +
> +/**
> + * fw_get_filesystem_firmware - load firmware into an allocated buffer.
> + * @plat: Platform data such as storage and partition firmware loading from.
> + * @firmware_p: pointer to firmware image.
> + *
> + * Return: Size of total read, negative value when error.
> + */
> +static int fw_get_filesystem_firmware(struct device_platdata *plat,
> +                                    struct firmware *firmware_p)

s/firmware_p/firmware/

> +{
> +       struct firmware_priv *fw_priv = NULL;
> +       loff_t actread;
> +       char *dev_part, *ubi_mtdpart, *ubi_volume;
> +       int ret;
> +
> +       dev_part = env_get("fw_dev_part");
> +       if (dev_part)
> +               set_storage_devpart(plat, dev_part);
> +
> +       ubi_mtdpart = env_get("fw_ubi_mtdpart");
> +       if (ubi_mtdpart)
> +               set_storage_mtdpart(plat, ubi_mtdpart);

Do you mean that the storage partition comes from the environment? I
thought it came from the FDT?

> +
> +       ubi_volume = env_get("fw_ubi_volume");
> +       if (ubi_volume)
> +               set_storage_ubivol(plat, ubi_volume);
> +
> +       ret = select_fs_dev(plat);
> +       if (ret)
> +               goto out;
> +
> +       fw_priv = firmware_p->priv;
> +
> +       ret = fs_read(fw_priv->name, (ulong)firmware_p->data, fw_priv->offset,

map_to_sysmem(firmware_p->data)

(so that it works on sandbox)

> +                    firmware_p->size, &actread);
> +       if (ret) {
> +               debug("Error: %d Failed to read %s from flash %lld != %d.\n",
> +                     ret, fw_priv->name, actread, firmware_p->size);
> +       } else {
> +               ret = actread;
> +       }
> +
> +out:
> +       return ret;
> +}
> +
> +/**
> + * request_firmware_into_buf - Load firmware into a previously allocated buffer.
> + * @plat: Platform data such as storage and partition firmware loading from.
> + * @firmware_p: Pointer to firmware image.
> + * @name: Name of firmware file.
> + * @buf: Address of buffer to load firmware into.
> + * @size: Size of buffer.
> + * @offset: Offset of a file for start reading into buffer.
> + *
> + * The firmware is loaded directly into the buffer pointed to by @buf and
> + * the @firmware_p data member is pointed at @buf.
> + *
> + * Return: Size of total read, negative value when error.
> + */
> +int request_firmware_into_buf(struct device_platdata *plat,
> +                             struct firmware **firmware_p,
> +                             const char *name,
> +                             void *buf, size_t size, u32 offset)
> +{
> +       int ret;
> +
> +       if (!plat)
> +               return -EINVAL;
> +
> +       ret = _request_firmware_prepare(firmware_p, name, buf, size, offset);
> +       if (ret < 0) /* error */
> +               return ret;
> +
> +       ret = fw_get_filesystem_firmware(plat, *firmware_p);
> +
> +       return ret;
> +}
> +
> +static int fs_loader_ofdata_to_platdata(struct udevice *dev)
> +{
> +       const char *fs_loader_path;
> +
> +       fs_loader_path = ofnode_get_chosen_prop("firmware-loader");
> +
> +       if (fs_loader_path) {
> +               ofnode fs_loader_node;
> +
> +               fs_loader_node = ofnode_path(fs_loader_path);
> +               if (ofnode_valid(fs_loader_node)) {
> +                       struct device_platdata *plat;
> +                       plat = dev->platdata;
> +
> +                       plat->name = (char *)ofnode_read_string(fs_loader_node,
> +                                        "storage_device");
> +
> +                       plat->devpart = (char *)ofnode_read_string(
> +                                        fs_loader_node, "devpart");
> +
> +                       plat->mtdpart = (char *)ofnode_read_string(
> +                                        fs_loader_node, "mtdpart");
> +
> +                       plat->ubivol = (char *)ofnode_read_string(
> +                                        fs_loader_node, "ubivol");
> +               }
> +       }
> +
> +       return 0;
> +}
> +
> +static int fs_loader_probe(struct udevice *dev)
> +{
> +       return 0;
> +};
> +
> +static const struct udevice_id fs_loader_ids[] = {
> +       { .compatible = "fs_loader"},
> +       { }
> +};
> +
> +U_BOOT_DRIVER(fs_loader) = {
> +       .name                   = "fs_loader",
> +       .id                     = UCLASS_FS_FIRMWARE_LOADER,
> +       .of_match               = fs_loader_ids,
> +       .probe                  = fs_loader_probe,
> +       .ofdata_to_platdata     = fs_loader_ofdata_to_platdata,
> +       .platdata_auto_alloc_size       = sizeof(struct device_platdata),
> +};
> +
> +UCLASS_DRIVER(fs_loader) = {
> +       .id             = UCLASS_FS_FIRMWARE_LOADER,
> +       .name           = "fs_loader",
> +};
> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> index d7f9df3..39e88ac 100644
> --- a/include/dm/uclass-id.h
> +++ b/include/dm/uclass-id.h
> @@ -36,6 +36,7 @@ enum uclass_id {
>         UCLASS_DMA,             /* Direct Memory Access */
>         UCLASS_EFI,             /* EFI managed devices */
>         UCLASS_ETH,             /* Ethernet device */
> +       UCLASS_FS_FIRMWARE_LOADER,              /* Generic loader */
>         UCLASS_GPIO,            /* Bank of general-purpose I/O pins */
>         UCLASS_FIRMWARE,        /* Firmware */
>         UCLASS_I2C,             /* I2C bus */
> diff --git a/include/fs_loader.h b/include/fs_loader.h
> new file mode 100644
> index 0000000..e3e5eeb
> --- /dev/null
> +++ b/include/fs_loader.h
> @@ -0,0 +1,28 @@
> +/*
> + * Copyright (C) 2018 Intel Corporation <www.intel.com>
> + *
> + * SPDX-License-Identifier:    GPL-2.0
> + */
> +#ifndef _FS_LOADER_H_
> +#define _FS_LOADER_H_
> +
> +#include <dm.h>
> +
> +struct firmware {
> +       size_t size;            /* Size of a file */
> +       const u8 *data;         /* Buffer for file */
> +       void *priv;             /* Firmware loader private fields */
> +};
> +
> +struct device_platdata {
> +       char *name;     /* Such as mmc, usb,and sata. */
> +       char *devpart;  /* Use the load command dev:part conventions */
> +       char *mtdpart;  /* MTD partition for ubi part */
> +       char *ubivol;   /* UBI volume-name for ubifsmount */
> +};
> +
> +int request_firmware_into_buf(struct device_platdata *plat,
> +                             struct firmware **firmware_p,
> +                             const char *name,
> +                             void *buf, size_t size, u32 offset);

Please can you add a full function comment here?

> +#endif
> --
> 2.2.0
>

Regards,
Simon

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

[Chee, Tien Fong]

On Mon, 2018-06-25 at 21:58 -0600, Simon Glass wrote:
> Hi,
> 
> On 25 June 2018 at 07:28,  <tien.fong.chee@intel.com> wrote:
> > 
> > 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/chosen.txt         | 22 ++++++++++++
> >  doc/device-tree-bindings/misc/fs_loader.txt | 52
> > +++++++++++++++++++++++++++++
> >  2 files changed, 74 insertions(+)
> >  create mode 100644 doc/device-tree-bindings/misc/fs_loader.txt
> > 
> > diff --git a/doc/device-tree-bindings/chosen.txt b/doc/device-tree-
> > bindings/chosen.txt
> > index c96b8f7..738673c 100644
> > --- a/doc/device-tree-bindings/chosen.txt
> > +++ b/doc/device-tree-bindings/chosen.txt
> > @@ -73,3 +73,25 @@ Example
> >                 u-boot,spl-boot-order = "same-as-spl", &sdmmc, "/sd
> > hci at fe330000";
> >         };
> >  };
> > +
> > +firmware-loader property
> > +------------------------
> > +Multiple file system firmware loader nodes could be defined in
> > device trees for
> > +multiple storage type and their default partition, then a property
> > +"firmware-loader" can be used to pass default firmware loader
> > +node(default storage type) to the firmware loader driver.
> > +
> > +Example
> > +-------
> > +/ {
> > +       chosen {
> > +               firmware-loader = &fs_loader0;
> > +       };
> > +
> > +       fs_loader0: fs_loader at 0 {
> This should be :
> 
> > 
> > +       fs_loader0: fs-loader at 0 {
> since hyphen is used for node and property names. Underscore is used
> for phandles (so you have that correct).
> 
Okay.
> > 
> > +               u-boot,dm-pre-reloc;
> > +               compatible = "fs_loader";
> How about u-boot,fs-loader since this is U-Boot specific I think.
> 
> 
> > 
> > +               storage_device = "mmc";
> storage-device
> 
Okay
> > 
> > +               devpart = "0:1";
> I think you should use a phandle to the node containing the mmc
> device to use.
> 
>    storage-device = <&mmc_3 1>;
> 
> for example (meaning use that MMC, partition 1.
> 
How to get device number based on node of a storage?
This could make design more complicated because this driver is designed
to support both fdt and without fdt(U-boot env and hard coding in
driver).
> > 
> > +       };
> > +};
> > 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..78bea66
> > --- /dev/null
> > +++ b/doc/device-tree-bindings/misc/fs_loader.txt
> > @@ -0,0 +1,52 @@
> > +* 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 storage device and partition search set for mmc, usb,
> > sata and
> > +ubi in device tree source as shown in below:
> > +
> > +       Example of storage type and device partition search set for
> > mmc, usb,
> > +       sata and ubi as shown in below:
> > +       Example for mmc:
> > +       fs_loader0: fs_loader at 0 {
> > +               u-boot,dm-pre-reloc;
> > +               compatible = "fs_loader";
> > +               storage_device = "mmc";
> > +               devpart = "0:1";
> > +       };
> > +
> > +       Example for usb:
> > +       fs_loader1: fs_loader at 1 {
> > +               u-boot,dm-pre-reloc;
> > +               compatible = "fs_loader";
> > +               storage_device = "usb";
> > +               devpart = "0:1";
> > +       };
> > +
> > +       Example for sata:
> > +       fs_loader2: fs_loader at 2 {
> > +               u-boot,dm-pre-reloc;
> > +               compatible = "fs_loader";
> > +               storage_device = "sata";
> > +               devpart = "0:1";
> > +       };
> > +
> > +       Example for ubi:
> > +       fs_loader3: fs_loader at 3 {
> > +               u-boot,dm-pre-reloc;
> > +               compatible = "fs_loader";
> > +               storage_device = "ubi";
> > +               mtdpart = "UBI",
> > +               ubivol = "ubi0";
> I'm not sure about ubi. It needs to refer to a particular device I
> suppose. How do we know the mapping from ubi to device?
> 
Actually this design is based on file system implementation which is
abstract from physical device, storage_device is used as interface of
file system, so nand and qspi may having the same ubi interface.

May be i can change the storage_device into storage_interface to avoid
confusing. 
> > 
> > +       };
> > --
> > 2.2.0
> > 
> Regards,
> Simon

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

[Chee, Tien Fong]

On Mon, 2018-06-25 at 21:58 -0600, Simon Glass wrote:
> Hi,
> 
> On 25 June 2018 at 07: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 | 238
> > +++++++++++++++++++++++++++++++++++++++++++++++
> >  include/dm/uclass-id.h   |   1 +
> >  include/fs_loader.h      |  28 ++++++
> >  5 files changed, 278 insertions(+)
> >  create mode 100644 drivers/misc/fs_loader.c
> >  create mode 100644 include/fs_loader.h
> This is definitely looking good. I think we need to figure out the
> binding to devices, to use phandles instead of strings. Also we need
> a
> sandbox driver and test.
> 
> I'm a little worried that you are blazing a trail here, but it is a
> valuable trail :-)
> 
> Tom, do you have any ideas / thoughts on the design?
> 
yeah, this part is most challenging, because this driver is designed
based on file system implementation, which is abstract from physical
device. So, it requires data like interface type, device number and
partition to work. Wonder how we can get those data if based on
physical storage node.
> > 
> > 
> > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> > index 17b3a80..4163b4f 100644
> > --- a/drivers/misc/Kconfig
> > +++ b/drivers/misc/Kconfig
> > @@ -277,4 +277,14 @@ config GDSYS_RXAUI_CTRL
> >         depends on MISC
> >         help
> >           Support gdsys FPGA's RXAUI control.
> > +
> > +config FS_LOADER
> > +       bool "Enable loader driver for file system"
> > +       help
> > +         This is file system generic loader which can be used to
> > load
> > +         the file image from the storage into target such as
> > memory.
> > +
> > +         The consumer driver would then use this loader to program
> > whatever,
> > +         ie. the FPGA device.
> > +
> >  endmenu
> > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> > index 4ce9d21..67a36f8 100644
> > --- a/drivers/misc/Makefile
> > +++ b/drivers/misc/Makefile
> > @@ -54,3 +54,4 @@ obj-$(CONFIG_STM32_RCC) += stm32_rcc.o
> >  obj-$(CONFIG_STM32MP_FUSE) += stm32mp_fuse.o
> >  obj-$(CONFIG_SYS_DPAA_QBMAN) += fsl_portals.o
> >  obj-$(CONFIG_GDSYS_RXAUI_CTRL) += gdsys_rxaui_ctrl.o
> > +obj-$(CONFIG_FS_LOADER) += fs_loader.o
> > diff --git a/drivers/misc/fs_loader.c b/drivers/misc/fs_loader.c
> > new file mode 100644
> > index 0000000..0dc385f
> > --- /dev/null
> > +++ b/drivers/misc/fs_loader.c
> > @@ -0,0 +1,238 @@
> > +/*
> > + * Copyright (C) 2018 Intel Corporation <www.intel.com>
> > + *
> > + * SPDX-License-Identifier:    GPL-2.0
> > + */
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <errno.h>
> > +#include <fs.h>
> > +#include <fs_loader.h>
> > +#include <linux/string.h>
> > +#include <malloc.h>
> > +#include <spl.h>
> > +
> > +DECLARE_GLOBAL_DATA_PTR;
> > +
> > +struct firmware_priv {
> > +       const char *name;       /* Filename */
> > +       u32 offset;             /* Offset of reading a file */
> > +};
> > +
> > +static int select_fs_dev(struct device_platdata *plat)
> > +{
> > +       int ret;
> > +
> > +       if (!strcmp("mmc", plat->name)) {
> > +               ret = fs_set_blk_dev("mmc", plat->devpart,
> > FS_TYPE_ANY);
> > +       } else if (!strcmp("usb", plat->name)) {
> > +               ret = fs_set_blk_dev("usb", plat->devpart,
> > FS_TYPE_ANY);
> > +       } else if (!strcmp("sata", plat->name)) {
> > +               ret = fs_set_blk_dev("sata", plat->devpart,
> > FS_TYPE_ANY);
> For these I think you can look up the phandle to obtain the device,
> then check its uclass to find out its type.
> 
How to find the interface type of the file system based on the physical
device node? Some devices like NAND and QSPI could share the similar
interface type like UBI.
> > 
> > +       } else if (!strcmp("ubi", plat->name)) {
> > +               if (plat->ubivol)
> > +                       ret = fs_set_blk_dev("ubi", NULL,
> > FS_TYPE_UBIFS);
> > +               else
> > +                       ret = -ENODEV;
> > +       } else {
> > +               debug("Error: unsupported storage device.\n");
> > +               return -ENODEV;
> > +       }
> > +
> > +       if (ret)
> > +               debug("Error: could not access storage.\n");
> > +
> > +       return ret;
> > +}
> > +
> > +static void set_storage_devpart(struct device_platdata *plat, char
> > *devpart)
> > +{
> > +       plat->devpart = devpart;
> > +}
> > +
> > +static void set_storage_mtdpart(struct device_platdata *plat, char
> > *mtdpart)
> > +{
> > +       plat->mtdpart = mtdpart;
> > +}
> > +
> > +static void set_storage_ubivol(struct device_platdata *plat, char
> > *ubivol)
> > +{
> > +       plat->ubivol = ubivol;
> > +}
> > +
> > +/**
> > + * _request_firmware_prepare - Prepare firmware struct.
> > + *
> > + * @firmware_p: Pointer to pointer to firmware image.
> > + * @name: Name of firmware file.
> > + * @dbuf: Address of buffer to load firmware into.
> > + * @size: Size of buffer.
> > + * @offset: Offset of a file for start reading into buffer.
> > + *
> > + * Return: Negative value if fail, 0 for successful.
> > + */
> > +static int _request_firmware_prepare(struct firmware **firmware_p,
> Can you please move this to be the last arg and rename to firmwarep?
> 
Okay.
> > 
> > +                                   const char *name, void
> > *dbuf,device to ubi
> > +                                   size_t size, u32 offset)
> > +{
> > +       struct firmware *firmware;
> > +       struct firmware_priv *fw_priv;
> > +
> > +       *firmware_p = NULL;
> > +
> > +       if (!name || name[0] == '\0')
> > +               return -EINVAL;
> > +
> > +       firmware = calloc(1, sizeof(*firmware));
> > +       if (!firmware)
> > +               return -ENOMEM;
> > +
> > +       fw_priv = calloc(1, sizeof(*fw_priv));
> > +       if (!fw_priv) {
> > +               free(firmware);
> > +               return -ENOMEM;
> > +       }
> > +
> > +       fw_priv->name = name;
> > +       fw_priv->offset = offset;
> > +       firmware->data = dbuf;
> > +       firmware->size = size;
> > +       firmware->priv = fw_priv;
> > +       *firmware_p = firmware;
> > +
> > +       return 0;
> > +}
> > +
> > +/**
> > + * fw_get_filesystem_firmware - load firmware into an allocated
> > buffer.
> > + * @plat: Platform data such as storage and partition firmware
> > loading from.
> > + * @firmware_p: pointer to firmware image.
> > + *
> > + * Return: Size of total read, negative value when error.
> > + */
> > +static int fw_get_filesystem_firmware(struct device_platdata
> > *plat,
> > +                                    struct firmware *firmware_p)
> s/firmware_p/firmware/
> 
Okay.
> > 
> > +{
> > +       struct firmware_priv *fw_priv = NULL;
> > +       loff_t actread;
> > +       char *dev_part, *ubi_mtdpart, *ubi_volume;
> > +       int ret;
> > +
> > +       dev_part = env_get("fw_dev_part");
> > +       if (dev_part)
> > +               set_storage_devpart(plat, dev_part);
> > +
> > +       ubi_mtdpart = env_get("fw_ubi_mtdpart");
> > +       if (ubi_mtdpart)
> > +               set_storage_mtdpart(plat, ubi_mtdpart);
> Do you mean that the storage partition comes from the environment? I
> thought it came from the FDT?
> 
This driver is designed not only supporting the FDT, it also work with
U-boot env, U-boot script and without FDT.

For example, fpga loadfs commands from U-boot console can call this
driver to load bitstream file from the storage.
> > 
> > +
> > +       ubi_volume = env_get("fw_ubi_volume");
> > +       if (ubi_volume)
> > +               set_storage_ubivol(plat, ubi_volume);
> > +
> > +       ret = select_fs_dev(plat);
> > +       if (ret)
> > +               goto out;
> > +
> > +       fw_priv = firmware_p->priv;
> > +
> > +       ret = fs_read(fw_priv->name, (ulong)firmware_p->data,
> > fw_priv->offset,
> map_to_sysmem(firmware_p->data)
> 
> (so that it works on sandbox)
> 
Okay.
> > 
> > +                    firmware_p->size, &actread);
> > +       if (ret) {
> > +               debug("Error: %d Failed to read %s from flash %lld
> > != %d.\n",
> > +                     ret, fw_priv->name, actread, firmware_p-
> > >size);
> > +       } else {
> > +               ret = actread;
> > +       }
> > +
> > +out:
> > +       return ret;
> > +}
> > +
> > +/**
> > + * request_firmware_into_buf - Load firmware into a previously
> > allocated buffer.
> > + * @plat: Platform data such as storage and partition firmware
> > loading from.
> > + * @firmware_p: Pointer to firmware image.
> > + * @name: Name of firmware file.
> > + * @buf: Address of buffer to load firmware into.
> > + * @size: Size of buffer.
> > + * @offset: Offset of a file for start reading into buffer.
> > + *
> > + * The firmware is loaded directly into the buffer pointed to by
> > @buf and
> > + * the @firmware_p data member is pointed at @buf.
> > + *
> > + * Return: Size of total read, negative value when error.
> > + */
> > +int request_firmware_into_buf(struct device_platdata *plat,
> > +                             struct firmware **firmware_p,
> > +                             const char *name,
> > +                             void *buf, size_t size, u32 offset)
> > +{
> > +       int ret;
> > +
> > +       if (!plat)
> > +               return -EINVAL;
> > +
> > +       ret = _request_firmware_prepare(firmware_p, name, buf,
> > size, offset);
> > +       if (ret < 0) /* error */
> > +               return ret;
> > +
> > +       ret = fw_get_filesystem_firmware(plat, *firmware_p);
> > +
> > +       return ret;
> > +}
> > +
> > +static int fs_loader_ofdata_to_platdata(struct udevice *dev)
> > +{
> > +       const char *fs_loader_path;
> > +
> > +       fs_loader_path = ofnode_get_chosen_prop("firmware-loader");
> > +
> > +       if (fs_loader_path) {
> > +               ofnode fs_loader_node;
> > +
> > +               fs_loader_node = ofnode_path(fs_loader_path);
> > +               if (ofnode_valid(fs_loader_node)) {
> > +                       struct device_platdata *plat;
> > +                       plat = dev->platdata;
> > +
> > +                       plat->name = (char
> > *)ofnode_read_string(fs_loader_node,
> > +                                        "storage_device");
> > +
> > +                       plat->devpart = (char *)ofnode_read_string(
> > +                                        fs_loader_node,
> > "devpart");
> > +
> > +                       plat->mtdpart = (char *)ofnode_read_string(
> > +                                        fs_loader_node,
> > "mtdpart");
> > +
> > +                       plat->ubivol = (char *)ofnode_read_string(
> > +                                        fs_loader_node, "ubivol");
> > +               }
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int fs_loader_probe(struct udevice *dev)
> > +{
> > +       return 0;
> > +};
> > +
> > +static const struct udevice_id fs_loader_ids[] = {
> > +       { .compatible = "fs_loader"},
> > +       { }
> > +};
> > +
> > +U_BOOT_DRIVER(fs_loader) = {
> > +       .name                   = "fs_loader",
> > +       .id                     = UCLASS_FS_FIRMWARE_LOADER,
> > +       .of_match               = fs_loader_ids,
> > +       .probe                  = fs_loader_probe,
> > +       .ofdata_to_platdata     = fs_loader_ofdata_to_platdata,
> > +       .platdata_auto_alloc_size       = sizeof(struct
> > device_platdata),
> > +};
> > +
> > +UCLASS_DRIVER(fs_loader) = {
> > +       .id             = UCLASS_FS_FIRMWARE_LOADER,
> > +       .name           = "fs_loader",
> > +};
> > diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> > index d7f9df3..39e88ac 100644
> > --- a/include/dm/uclass-id.h
> > +++ b/include/dm/uclass-id.h
> > @@ -36,6 +36,7 @@ enum uclass_id {
> >         UCLASS_DMA,             /* Direct Memory Access */
> >         UCLASS_EFI,             /* EFI managed devices */
> >         UCLASS_ETH,             /* Ethernet device */
> > +       UCLASS_FS_FIRMWARE_LOADER,              /* Generic loader
> > */
> >         UCLASS_GPIO,            /* Bank of general-purpose I/O pins
> > */
> >         UCLASS_FIRMWARE,        /* Firmware */
> >         UCLASS_I2C,             /* I2C bus */
> > diff --git a/include/fs_loader.h b/include/fs_loader.h
> > new file mode 100644
> > index 0000000..e3e5eeb
> > --- /dev/null
> > +++ b/include/fs_loader.h
> > @@ -0,0 +1,28 @@
> > +/*
> > + * Copyright (C) 2018 Intel Corporation <www.intel.com>
> > + *
> > + * SPDX-License-Identifier:    GPL-2.0
> > + */
> > +#ifndef _FS_LOADER_H_
> > +#define _FS_LOADER_H_
> > +
> > +#include <dm.h>
> > +
> > +struct firmware {
> > +       size_t size;            /* Size of a file */
> > +       const u8 *data;         /* Buffer for file */
> > +       void *priv;             /* Firmware loader private fields
> > */
> > +};
> > +
> > +struct device_platdata {
> > +       char *name;     /* Such as mmc, usb,and sata. */
> > +       char *devpart;  /* Use the load command dev:part
> > conventions */
> > +       char *mtdpart;  /* MTD partition for ubi part */
> > +       char *ubivol;   /* UBI volume-name for ubifsmount */
> > +};
> > +
> > +int request_firmware_into_buf(struct device_platdata *plat,
> > +                             struct firmware **firmware_p,
> > +                             const char *name,
> > +                             void *buf, size_t size, u32 offset);
> Please can you add a full function comment here?
> 
Okay.
> > 
> > +#endif
> > --
> > 2.2.0
> > 
> Regards,
> Simon

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

[Tom Rini]

On Mon, Jun 25, 2018 at 09:58:51PM -0600, Simon Glass wrote:
> Hi,
> 
> On 25 June 2018 at 07: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 | 238 +++++++++++++++++++++++++++++++++++++++++++++++
> >  include/dm/uclass-id.h   |   1 +
> >  include/fs_loader.h      |  28 ++++++
> >  5 files changed, 278 insertions(+)
> >  create mode 100644 drivers/misc/fs_loader.c
> >  create mode 100644 include/fs_loader.h
> 
> This is definitely looking good. I think we need to figure out the
> binding to devices, to use phandles instead of strings. Also we need a
> sandbox driver and test.
> 
> I'm a little worried that you are blazing a trail here, but it is a
> valuable trail :-)
> 
> Tom, do you have any ideas / thoughts on the design?

If we're still able to support the various use cases, and everyone is
happier than before, that's good enough for me for now.  We can always
evolve things as time goes on.  Once there's tests of course :)

-- 
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/20180627/ba1fddcc/attachment.sig>

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

[Simon Glass]

Hi Tien Fong,

On 27 June 2018 at 01:41, Chee, Tien Fong <tien.fong.chee@intel.com> wrote:
> On Mon, 2018-06-25 at 21:58 -0600, Simon Glass wrote:
>> Hi,
>>
>> On 25 June 2018 at 07:28,  <tien.fong.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 | 238
>> > +++++++++++++++++++++++++++++++++++++++++++++++
>> >  include/dm/uclass-id.h   |   1 +
>> >  include/fs_loader.h      |  28 ++++++
>> >  5 files changed, 278 insertions(+)
>> >  create mode 100644 drivers/misc/fs_loader.c
>> >  create mode 100644 include/fs_loader.h
>> This is definitely looking good. I think we need to figure out the
>> binding to devices, to use phandles instead of strings. Also we need
>> a
>> sandbox driver and test.
>>
>> I'm a little worried that you are blazing a trail here, but it is a
>> valuable trail :-)
>>
>> Tom, do you have any ideas / thoughts on the design?
>>
> yeah, this part is most challenging, because this driver is designed
> based on file system implementation, which is abstract from physical
> device. So, it requires data like interface type, device number and
> partition to work. Wonder how we can get those data if based on
> physical storage node.
>> >
>> >
>> > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>> > index 17b3a80..4163b4f 100644
>> > --- a/drivers/misc/Kconfig
>> > +++ b/drivers/misc/Kconfig
>> > @@ -277,4 +277,14 @@ config GDSYS_RXAUI_CTRL
>> >         depends on MISC
>> >         help
>> >           Support gdsys FPGA's RXAUI control.
>> > +
>> > +config FS_LOADER
>> > +       bool "Enable loader driver for file system"
>> > +       help
>> > +         This is file system generic loader which can be used to
>> > load
>> > +         the file image from the storage into target such as
>> > memory.
>> > +
>> > +         The consumer driver would then use this loader to program
>> > whatever,
>> > +         ie. the FPGA device.
>> > +
>> >  endmenu
>> > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
>> > index 4ce9d21..67a36f8 100644
>> > --- a/drivers/misc/Makefile
>> > +++ b/drivers/misc/Makefile
>> > @@ -54,3 +54,4 @@ obj-$(CONFIG_STM32_RCC) += stm32_rcc.o
>> >  obj-$(CONFIG_STM32MP_FUSE) += stm32mp_fuse.o
>> >  obj-$(CONFIG_SYS_DPAA_QBMAN) += fsl_portals.o
>> >  obj-$(CONFIG_GDSYS_RXAUI_CTRL) += gdsys_rxaui_ctrl.o
>> > +obj-$(CONFIG_FS_LOADER) += fs_loader.o
>> > diff --git a/drivers/misc/fs_loader.c b/drivers/misc/fs_loader.c
>> > new file mode 100644
>> > index 0000000..0dc385f
>> > --- /dev/null
>> > +++ b/drivers/misc/fs_loader.c
>> > @@ -0,0 +1,238 @@
>> > +/*
>> > + * Copyright (C) 2018 Intel Corporation <www.intel.com>
>> > + *
>> > + * SPDX-License-Identifier:    GPL-2.0
>> > + */
>> > +#include <common.h>
>> > +#include <dm.h>
>> > +#include <errno.h>
>> > +#include <fs.h>
>> > +#include <fs_loader.h>
>> > +#include <linux/string.h>
>> > +#include <malloc.h>
>> > +#include <spl.h>
>> > +
>> > +DECLARE_GLOBAL_DATA_PTR;
>> > +
>> > +struct firmware_priv {
>> > +       const char *name;       /* Filename */
>> > +       u32 offset;             /* Offset of reading a file */
>> > +};
>> > +
>> > +static int select_fs_dev(struct device_platdata *plat)
>> > +{
>> > +       int ret;
>> > +
>> > +       if (!strcmp("mmc", plat->name)) {
>> > +               ret = fs_set_blk_dev("mmc", plat->devpart,
>> > FS_TYPE_ANY);
>> > +       } else if (!strcmp("usb", plat->name)) {
>> > +               ret = fs_set_blk_dev("usb", plat->devpart,
>> > FS_TYPE_ANY);
>> > +       } else if (!strcmp("sata", plat->name)) {
>> > +               ret = fs_set_blk_dev("sata", plat->devpart,
>> > FS_TYPE_ANY);
>> For these I think you can look up the phandle to obtain the device,
>> then check its uclass to find out its type.
>>
> How to find the interface type of the file system based on the physical
> device node? Some devices like NAND and QSPI could share the similar
> interface type like UBI.

See my other email on this.

>> >
>> > +       } else if (!strcmp("ubi", plat->name)) {
>> > +               if (plat->ubivol)
>> > +                       ret = fs_set_blk_dev("ubi", NULL,
>> > FS_TYPE_UBIFS);
>> > +               else
>> > +                       ret = -ENODEV;
>> > +       } else {
>> > +               debug("Error: unsupported storage device.\n");
>> > +               return -ENODEV;
>> > +       }
>> > +
>> > +       if (ret)
>> > +               debug("Error: could not access storage.\n");
>> > +
>> > +       return ret;
>> > +}
>> > +
>> > +static void set_storage_devpart(struct device_platdata *plat, char
>> > *devpart)
>> > +{
>> > +       plat->devpart = devpart;
>> > +}
>> > +
>> > +static void set_storage_mtdpart(struct device_platdata *plat, char
>> > *mtdpart)
>> > +{
>> > +       plat->mtdpart = mtdpart;
>> > +}
>> > +
>> > +static void set_storage_ubivol(struct device_platdata *plat, char
>> > *ubivol)
>> > +{
>> > +       plat->ubivol = ubivol;
>> > +}
>> > +
>> > +/**
>> > + * _request_firmware_prepare - Prepare firmware struct.
>> > + *
>> > + * @firmware_p: Pointer to pointer to firmware image.
>> > + * @name: Name of firmware file.
>> > + * @dbuf: Address of buffer to load firmware into.
>> > + * @size: Size of buffer.
>> > + * @offset: Offset of a file for start reading into buffer.
>> > + *
>> > + * Return: Negative value if fail, 0 for successful.
>> > + */
>> > +static int _request_firmware_prepare(struct firmware **firmware_p,
>> Can you please move this to be the last arg and rename to firmwarep?
>>
> Okay.
>> >
>> > +                                   const char *name, void
>> > *dbuf,device to ubi
>> > +                                   size_t size, u32 offset)
>> > +{
>> > +       struct firmware *firmware;
>> > +       struct firmware_priv *fw_priv;
>> > +
>> > +       *firmware_p = NULL;
>> > +
>> > +       if (!name || name[0] == '\0')
>> > +               return -EINVAL;
>> > +
>> > +       firmware = calloc(1, sizeof(*firmware));
>> > +       if (!firmware)
>> > +               return -ENOMEM;
>> > +
>> > +       fw_priv = calloc(1, sizeof(*fw_priv));
>> > +       if (!fw_priv) {
>> > +               free(firmware);
>> > +               return -ENOMEM;
>> > +       }
>> > +
>> > +       fw_priv->name = name;
>> > +       fw_priv->offset = offset;
>> > +       firmware->data = dbuf;
>> > +       firmware->size = size;
>> > +       firmware->priv = fw_priv;
>> > +       *firmware_p = firmware;
>> > +
>> > +       return 0;
>> > +}
>> > +
>> > +/**
>> > + * fw_get_filesystem_firmware - load firmware into an allocated
>> > buffer.
>> > + * @plat: Platform data such as storage and partition firmware
>> > loading from.
>> > + * @firmware_p: pointer to firmware image.
>> > + *
>> > + * Return: Size of total read, negative value when error.
>> > + */
>> > +static int fw_get_filesystem_firmware(struct device_platdata
>> > *plat,
>> > +                                    struct firmware *firmware_p)
>> s/firmware_p/firmware/
>>
> Okay.
>> >
>> > +{
>> > +       struct firmware_priv *fw_priv = NULL;
>> > +       loff_t actread;
>> > +       char *dev_part, *ubi_mtdpart, *ubi_volume;
>> > +       int ret;
>> > +
>> > +       dev_part = env_get("fw_dev_part");
>> > +       if (dev_part)
>> > +               set_storage_devpart(plat, dev_part);
>> > +
>> > +       ubi_mtdpart = env_get("fw_ubi_mtdpart");
>> > +       if (ubi_mtdpart)
>> > +               set_storage_mtdpart(plat, ubi_mtdpart);
>> Do you mean that the storage partition comes from the environment? I
>> thought it came from the FDT?
>>
> This driver is designed not only supporting the FDT, it also work with
> U-boot env, U-boot script and without FDT.
>
> For example, fpga loadfs commands from U-boot console can call this
> driver to load bitstream file from the storage.

OK, but in that case why use environment? Can't you just put the
parameters in the command?

[..]

Regards,
Simon

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

[Simon Glass]

Hi Tien Fong,

On 27 June 2018 at 01:32, Chee, Tien Fong <tien.fong.chee@intel.com> wrote:
> On Mon, 2018-06-25 at 21:58 -0600, Simon Glass wrote:
>> Hi,
>>
>> On 25 June 2018 at 07:28,  <tien.fong.chee@intel.com> wrote:
>> >
>> > 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/chosen.txt         | 22 ++++++++++++
>> >  doc/device-tree-bindings/misc/fs_loader.txt | 52
>> > +++++++++++++++++++++++++++++
>> >  2 files changed, 74 insertions(+)
>> >  create mode 100644 doc/device-tree-bindings/misc/fs_loader.txt
>> >
>> > diff --git a/doc/device-tree-bindings/chosen.txt b/doc/device-tree-
>> > bindings/chosen.txt
>> > index c96b8f7..738673c 100644
>> > --- a/doc/device-tree-bindings/chosen.txt
>> > +++ b/doc/device-tree-bindings/chosen.txt
>> > @@ -73,3 +73,25 @@ Example
>> >                 u-boot,spl-boot-order = "same-as-spl", &sdmmc, "/sd
>> > hci at fe330000";
>> >         };
>> >  };
>> > +
>> > +firmware-loader property
>> > +------------------------
>> > +Multiple file system firmware loader nodes could be defined in
>> > device trees for
>> > +multiple storage type and their default partition, then a property
>> > +"firmware-loader" can be used to pass default firmware loader
>> > +node(default storage type) to the firmware loader driver.
>> > +
>> > +Example
>> > +-------
>> > +/ {
>> > +       chosen {
>> > +               firmware-loader = &fs_loader0;
>> > +       };
>> > +
>> > +       fs_loader0: fs_loader at 0 {
>> This should be :
>>
>> >
>> > +       fs_loader0: fs-loader at 0 {
>> since hyphen is used for node and property names. Underscore is used
>> for phandles (so you have that correct).
>>
> Okay.
>> >
>> > +               u-boot,dm-pre-reloc;
>> > +               compatible = "fs_loader";
>> How about u-boot,fs-loader since this is U-Boot specific I think.
>>
>>
>> >
>> > +               storage_device = "mmc";
>> storage-device
>>
> Okay
>> >
>> > +               devpart = "0:1";
>> I think you should use a phandle to the node containing the mmc
>> device to use.
>>
>>    storage-device = <&mmc_3 1>;
>>
>> for example (meaning use that MMC, partition 1.
>>
> How to get device number based on node of a storage?
> This could make design more complicated because this driver is designed
> to support both fdt and without fdt(U-boot env and hard coding in
> driver).

I think it is fine to support the environment as a way of providing
this info, but there is no need to support non-DM. Everything should
be converting to DM now.

We have:

uclass_get_device_by_phandle_id()
device_get_global_by_of_offset()

I think we need to create a function called

device_get_global_by_phandle_id()

which can be used to obtain the device based on a phandle.

>> >
>> > +       };
>> > +};
>> > 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..78bea66
>> > --- /dev/null
>> > +++ b/doc/device-tree-bindings/misc/fs_loader.txt
>> > @@ -0,0 +1,52 @@
>> > +* 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 storage device and partition search set for mmc, usb,
>> > sata and
>> > +ubi in device tree source as shown in below:
>> > +
>> > +       Example of storage type and device partition search set for
>> > mmc, usb,
>> > +       sata and ubi as shown in below:
>> > +       Example for mmc:
>> > +       fs_loader0: fs_loader at 0 {
>> > +               u-boot,dm-pre-reloc;
>> > +               compatible = "fs_loader";
>> > +               storage_device = "mmc";
>> > +               devpart = "0:1";
>> > +       };
>> > +
>> > +       Example for usb:
>> > +       fs_loader1: fs_loader at 1 {
>> > +               u-boot,dm-pre-reloc;
>> > +               compatible = "fs_loader";
>> > +               storage_device = "usb";
>> > +               devpart = "0:1";
>> > +       };
>> > +
>> > +       Example for sata:
>> > +       fs_loader2: fs_loader at 2 {
>> > +               u-boot,dm-pre-reloc;
>> > +               compatible = "fs_loader";
>> > +               storage_device = "sata";
>> > +               devpart = "0:1";
>> > +       };
>> > +
>> > +       Example for ubi:
>> > +       fs_loader3: fs_loader at 3 {
>> > +               u-boot,dm-pre-reloc;
>> > +               compatible = "fs_loader";
>> > +               storage_device = "ubi";
>> > +               mtdpart = "UBI",
>> > +               ubivol = "ubi0";
>> I'm not sure about ubi. It needs to refer to a particular device I
>> suppose. How do we know the mapping from ubi to device?
>>
> Actually this design is based on file system implementation which is
> abstract from physical device, storage_device is used as interface of
> file system, so nand and qspi may having the same ubi interface.
>
> May be i can change the storage_device into storage_interface to avoid
> confusing.

OK I see. Well I suppose this is OK then, at least for now. I'm not
have any great ideas on how to specify which ubi storage device to
use. Let's assume we only have one for now.

Regards,
Simon

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

[Chee, Tien Fong]

On Wed, 2018-06-27 at 14:03 -0700, Simon Glass wrote:
> Hi Tien Fong,
> 
> On 27 June 2018 at 01:41, Chee, Tien Fong <tien.fong.chee@intel.com>
> wrote:
> > 
> > On Mon, 2018-06-25 at 21:58 -0600, Simon Glass wrote:
> > > 
> > > Hi,
> > > 
> > > On 25 June 2018 at 07:28,  <tien.fong.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 | 238
> > > > +++++++++++++++++++++++++++++++++++++++++++++++
> > > >  include/dm/uclass-id.h   |   1 +
> > > >  include/fs_loader.h      |  28 ++++++
> > > >  5 files changed, 278 insertions(+)
> > > >  create mode 100644 drivers/misc/fs_loader.c
> > > >  create mode 100644 include/fs_loader.h
> > > This is definitely looking good. I think we need to figure out
> > > the
> > > binding to devices, to use phandles instead of strings. Also we
> > > need
> > > a
> > > sandbox driver and test.
> > > 
> > > I'm a little worried that you are blazing a trail here, but it is
> > > a
> > > valuable trail :-)
> > > 
> > > Tom, do you have any ideas / thoughts on the design?
> > > 
> > yeah, this part is most challenging, because this driver is
> > designed
> > based on file system implementation, which is abstract from
> > physical
> > device. So, it requires data like interface type, device number and
> > partition to work. Wonder how we can get those data if based on
> > physical storage node.
> > > 
> > > > 
> > > > 
> > > > 
> > > > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> > > > index 17b3a80..4163b4f 100644
> > > > --- a/drivers/misc/Kconfig
> > > > +++ b/drivers/misc/Kconfig
> > > > @@ -277,4 +277,14 @@ config GDSYS_RXAUI_CTRL
> > > >         depends on MISC
> > > >         help
> > > >           Support gdsys FPGA's RXAUI control.
> > > > +
> > > > +config FS_LOADER
> > > > +       bool "Enable loader driver for file system"
> > > > +       help
> > > > +         This is file system generic loader which can be used
> > > > to
> > > > load
> > > > +         the file image from the storage into target such as
> > > > memory.
> > > > +
> > > > +         The consumer driver would then use this loader to
> > > > program
> > > > whatever,
> > > > +         ie. the FPGA device.
> > > > +
> > > >  endmenu
> > > > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> > > > index 4ce9d21..67a36f8 100644
> > > > --- a/drivers/misc/Makefile
> > > > +++ b/drivers/misc/Makefile
> > > > @@ -54,3 +54,4 @@ obj-$(CONFIG_STM32_RCC) += stm32_rcc.o
> > > >  obj-$(CONFIG_STM32MP_FUSE) += stm32mp_fuse.o
> > > >  obj-$(CONFIG_SYS_DPAA_QBMAN) += fsl_portals.o
> > > >  obj-$(CONFIG_GDSYS_RXAUI_CTRL) += gdsys_rxaui_ctrl.o
> > > > +obj-$(CONFIG_FS_LOADER) += fs_loader.o
> > > > diff --git a/drivers/misc/fs_loader.c
> > > > b/drivers/misc/fs_loader.c
> > > > new file mode 100644
> > > > index 0000000..0dc385f
> > > > --- /dev/null
> > > > +++ b/drivers/misc/fs_loader.c
> > > > @@ -0,0 +1,238 @@
> > > > +/*
> > > > + * Copyright (C) 2018 Intel Corporation <www.intel.com>
> > > > + *
> > > > + * SPDX-License-Identifier:    GPL-2.0
> > > > + */
> > > > +#include <common.h>
> > > > +#include <dm.h>
> > > > +#include <errno.h>
> > > > +#include <fs.h>
> > > > +#include <fs_loader.h>
> > > > +#include <linux/string.h>
> > > > +#include <malloc.h>
> > > > +#include <spl.h>
> > > > +
> > > > +DECLARE_GLOBAL_DATA_PTR;
> > > > +
> > > > +struct firmware_priv {
> > > > +       const char *name;       /* Filename */
> > > > +       u32 offset;             /* Offset of reading a file */
> > > > +};
> > > > +
> > > > +static int select_fs_dev(struct device_platdata *plat)
> > > > +{
> > > > +       int ret;
> > > > +
> > > > +       if (!strcmp("mmc", plat->name)) {
> > > > +               ret = fs_set_blk_dev("mmc", plat->devpart,
> > > > FS_TYPE_ANY);
> > > > +       } else if (!strcmp("usb", plat->name)) {
> > > > +               ret = fs_set_blk_dev("usb", plat->devpart,
> > > > FS_TYPE_ANY);
> > > > +       } else if (!strcmp("sata", plat->name)) {
> > > > +               ret = fs_set_blk_dev("sata", plat->devpart,
> > > > FS_TYPE_ANY);
> > > For these I think you can look up the phandle to obtain the
> > > device,
> > > then check its uclass to find out its type.
> > > 
> > How to find the interface type of the file system based on the
> > physical
> > device node? Some devices like NAND and QSPI could share the
> > similar
> > interface type like UBI.
> See my other email on this.
> 
Okay.
> > 
> > > 
> > > > 
> > > > 
> > > > +       } else if (!strcmp("ubi", plat->name)) {
> > > > +               if (plat->ubivol)
> > > > +                       ret = fs_set_blk_dev("ubi", NULL,
> > > > FS_TYPE_UBIFS);
> > > > +               else
> > > > +                       ret = -ENODEV;
> > > > +       } else {
> > > > +               debug("Error: unsupported storage device.\n");
> > > > +               return -ENODEV;
> > > > +       }
> > > > +
> > > > +       if (ret)
> > > > +               debug("Error: could not access storage.\n");
> > > > +
> > > > +       return ret;
> > > > +}
> > > > +
> > > > +static void set_storage_devpart(struct device_platdata *plat,
> > > > char
> > > > *devpart)
> > > > +{
> > > > +       plat->devpart = devpart;
> > > > +}
> > > > +
> > > > +static void set_storage_mtdpart(struct device_platdata *plat,
> > > > char
> > > > *mtdpart)
> > > > +{
> > > > +       plat->mtdpart = mtdpart;
> > > > +}
> > > > +
> > > > +static void set_storage_ubivol(struct device_platdata *plat,
> > > > char
> > > > *ubivol)
> > > > +{
> > > > +       plat->ubivol = ubivol;
> > > > +}
> > > > +
> > > > +/**
> > > > + * _request_firmware_prepare - Prepare firmware struct.
> > > > + *
> > > > + * @firmware_p: Pointer to pointer to firmware image.
> > > > + * @name: Name of firmware file.
> > > > + * @dbuf: Address of buffer to load firmware into.
> > > > + * @size: Size of buffer.
> > > > + * @offset: Offset of a file for start reading into buffer.
> > > > + *
> > > > + * Return: Negative value if fail, 0 for successful.
> > > > + */
> > > > +static int _request_firmware_prepare(struct firmware
> > > > **firmware_p,
> > > Can you please move this to be the last arg and rename to
> > > firmwarep?
> > > 
> > Okay.
> > > 
> > > > 
> > > > 
> > > > +                                   const char *name, void
> > > > *dbuf,device to ubi
> > > > +                                   size_t size, u32 offset)
> > > > +{
> > > > +       struct firmware *firmware;
> > > > +       struct firmware_priv *fw_priv;
> > > > +
> > > > +       *firmware_p = NULL;
> > > > +
> > > > +       if (!name || name[0] == '\0')
> > > > +               return -EINVAL;
> > > > +
> > > > +       firmware = calloc(1, sizeof(*firmware));
> > > > +       if (!firmware)
> > > > +               return -ENOMEM;
> > > > +
> > > > +       fw_priv = calloc(1, sizeof(*fw_priv));
> > > > +       if (!fw_priv) {
> > > > +               free(firmware);
> > > > +               return -ENOMEM;
> > > > +       }
> > > > +
> > > > +       fw_priv->name = name;
> > > > +       fw_priv->offset = offset;
> > > > +       firmware->data = dbuf;
> > > > +       firmware->size = size;
> > > > +       firmware->priv = fw_priv;
> > > > +       *firmware_p = firmware;
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +/**
> > > > + * fw_get_filesystem_firmware - load firmware into an
> > > > allocated
> > > > buffer.
> > > > + * @plat: Platform data such as storage and partition firmware
> > > > loading from.
> > > > + * @firmware_p: pointer to firmware image.
> > > > + *
> > > > + * Return: Size of total read, negative value when error.
> > > > + */
> > > > +static int fw_get_filesystem_firmware(struct device_platdata
> > > > *plat,
> > > > +                                    struct firmware
> > > > *firmware_p)
> > > s/firmware_p/firmware/
> > > 
> > Okay.
> > > 
> > > > 
> > > > 
> > > > +{
> > > > +       struct firmware_priv *fw_priv = NULL;
> > > > +       loff_t actread;
> > > > +       char *dev_part, *ubi_mtdpart, *ubi_volume;
> > > > +       int ret;
> > > > +
> > > > +       dev_part = env_get("fw_dev_part");
> > > > +       if (dev_part)
> > > > +               set_storage_devpart(plat, dev_part);
> > > > +
> > > > +       ubi_mtdpart = env_get("fw_ubi_mtdpart");
> > > > +       if (ubi_mtdpart)
> > > > +               set_storage_mtdpart(plat, ubi_mtdpart);
> > > Do you mean that the storage partition comes from the
> > > environment? I
> > > thought it came from the FDT?
> > > 
> > This driver is designed not only supporting the FDT, it also work
> > with
> > U-boot env, U-boot script and without FDT.
> > 
> > For example, fpga loadfs commands from U-boot console can call this
> > driver to load bitstream file from the storage.
> OK, but in that case why use environment? Can't you just put the
> parameters in the command?
> 
User can save the value like partition into environment and saving in
the storage as new default env value. So, this env value would be used
for every power on.
> [..]
> 
> Regards,
> Simon

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

[Chee, Tien Fong]

On Wed, 2018-06-27 at 14:03 -0700, Simon Glass wrote:
> Hi Tien Fong,
> 
> On 27 June 2018 at 01:32, Chee, Tien Fong <tien.fong.chee@intel.com>
> wrote:
> > 
> > On Mon, 2018-06-25 at 21:58 -0600, Simon Glass wrote:
> > > 
> > > Hi,
> > > 
> > > On 25 June 2018 at 07:28,  <tien.fong.chee@intel.com> wrote:
> > > > 
> > > > 
> > > > 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/chosen.txt         | 22 ++++++++++++
> > > >  doc/device-tree-bindings/misc/fs_loader.txt | 52
> > > > +++++++++++++++++++++++++++++
> > > >  2 files changed, 74 insertions(+)
> > > >  create mode 100644 doc/device-tree-bindings/misc/fs_loader.txt
> > > > 
> > > > diff --git a/doc/device-tree-bindings/chosen.txt b/doc/device-
> > > > tree-
> > > > bindings/chosen.txt
> > > > index c96b8f7..738673c 100644
> > > > --- a/doc/device-tree-bindings/chosen.txt
> > > > +++ b/doc/device-tree-bindings/chosen.txt
> > > > @@ -73,3 +73,25 @@ Example
> > > >                 u-boot,spl-boot-order = "same-as-spl", &sdmmc,
> > > > "/sd
> > > > hci at fe330000";
> > > >         };
> > > >  };
> > > > +
> > > > +firmware-loader property
> > > > +------------------------
> > > > +Multiple file system firmware loader nodes could be defined in
> > > > device trees for
> > > > +multiple storage type and their default partition, then a
> > > > property
> > > > +"firmware-loader" can be used to pass default firmware loader
> > > > +node(default storage type) to the firmware loader driver.
> > > > +
> > > > +Example
> > > > +-------
> > > > +/ {
> > > > +       chosen {
> > > > +               firmware-loader = &fs_loader0;
> > > > +       };
> > > > +
> > > > +       fs_loader0: fs_loader at 0 {
> > > This should be :
> > > 
> > > > 
> > > > 
> > > > +       fs_loader0: fs-loader at 0 {
> > > since hyphen is used for node and property names. Underscore is
> > > used
> > > for phandles (so you have that correct).
> > > 
> > Okay.
> > > 
> > > > 
> > > > 
> > > > +               u-boot,dm-pre-reloc;
> > > > +               compatible = "fs_loader";
> > > How about u-boot,fs-loader since this is U-Boot specific I think.
> > > 
> > > 
> > > > 
> > > > 
> > > > +               storage_device = "mmc";
> > > storage-device
> > > 
> > Okay
> > > 
> > > > 
> > > > 
> > > > +               devpart = "0:1";
> > > I think you should use a phandle to the node containing the mmc
> > > device to use.
> > > 
> > >    storage-device = <&mmc_3 1>;
> > > 
> > > for example (meaning use that MMC, partition 1.
> > > 
> > How to get device number based on node of a storage?
> > This could make design more complicated because this driver is
> > designed
> > to support both fdt and without fdt(U-boot env and hard coding in
> > driver).
> I think it is fine to support the environment as a way of providing
> this info, but there is no need to support non-DM. Everything should
> be converting to DM now.
> 
Ok,i would keep the DM and environment support.

> We have:
> 
> uclass_get_device_by_phandle_id()
> device_get_global_by_of_offset()
> 
> I think we need to create a function called
> 
> device_get_global_by_phandle_id()
> 
> which can be used to obtain the device based on a phandle.
> 
I means the device number in the struct blk_desc, block device. I don't
 know how the device number is built up during driver model init. Is
there a function to retrive the device number?
> > 
> > > 
> > > > 
> > > > 
> > > > +       };
> > > > +};
> > > > 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..78bea66
> > > > --- /dev/null
> > > > +++ b/doc/device-tree-bindings/misc/fs_loader.txt
> > > > @@ -0,0 +1,52 @@
> > > > +* 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 storage device and partition search set for mmc,
> > > > usb,
> > > > sata and
> > > > +ubi in device tree source as shown in below:
> > > > +
> > > > +       Example of storage type and device partition search set
> > > > for
> > > > mmc, usb,
> > > > +       sata and ubi as shown in below:
> > > > +       Example for mmc:
> > > > +       fs_loader0: fs_loader at 0 {
> > > > +               u-boot,dm-pre-reloc;
> > > > +               compatible = "fs_loader";
> > > > +               storage_device = "mmc";
> > > > +               devpart = "0:1";
> > > > +       };
> > > > +
> > > > +       Example for usb:
> > > > +       fs_loader1: fs_loader at 1 {
> > > > +               u-boot,dm-pre-reloc;
> > > > +               compatible = "fs_loader";
> > > > +               storage_device = "usb";
> > > > +               devpart = "0:1";
> > > > +       };
> > > > +
> > > > +       Example for sata:
> > > > +       fs_loader2: fs_loader at 2 {
> > > > +               u-boot,dm-pre-reloc;
> > > > +               compatible = "fs_loader";
> > > > +               storage_device = "sata";
> > > > +               devpart = "0:1";
> > > > +       };
> > > > +
> > > > +       Example for ubi:
> > > > +       fs_loader3: fs_loader at 3 {
> > > > +               u-boot,dm-pre-reloc;
> > > > +               compatible = "fs_loader";
> > > > +               storage_device = "ubi";
> > > > +               mtdpart = "UBI",
> > > > +               ubivol = "ubi0";
> > > I'm not sure about ubi. It needs to refer to a particular device
> > > I
> > > suppose. How do we know the mapping from ubi to device?
> > > 
> > Actually this design is based on file system implementation which
> > is
> > abstract from physical device, storage_device is used as interface
> > of
> > file system, so nand and qspi may having the same ubi interface.
> > 
> > May be i can change the storage_device into storage_interface to
> > avoid
> > confusing.
> OK I see. Well I suppose this is OK then, at least for now. I'm not
> have any great ideas on how to specify which ubi storage device to
> use. Let's assume we only have one for now.
> 
> Regards,
> Simon

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

[Chee, Tien Fong]

On Thu, 2018-06-28 at 01:04 -0700, Chee, Tien Fong wrote:
> On Wed, 2018-06-27 at 14:03 -0700, Simon Glass wrote:
> > 
> > Hi Tien Fong,
> > 
> > On 27 June 2018 at 01:32, Chee, Tien Fong <tien.fong.chee@intel.com
> > >
> > wrote:
> > > 
> > > 
> > > On Mon, 2018-06-25 at 21:58 -0600, Simon Glass wrote:
> > > > 
> > > > 
> > > > Hi,
> > > > 
> > > > On 25 June 2018 at 07:28,  <tien.fong.chee@intel.com> wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > 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/chosen.txt         | 22
> > > > > ++++++++++++
> > > > >  doc/device-tree-bindings/misc/fs_loader.txt | 52
> > > > > +++++++++++++++++++++++++++++
> > > > >  2 files changed, 74 insertions(+)
> > > > >  create mode 100644 doc/device-tree-
> > > > > bindings/misc/fs_loader.txt
> > > > > 
> > > > > diff --git a/doc/device-tree-bindings/chosen.txt
> > > > > b/doc/device-
> > > > > tree-
> > > > > bindings/chosen.txt
> > > > > index c96b8f7..738673c 100644
> > > > > --- a/doc/device-tree-bindings/chosen.txt
> > > > > +++ b/doc/device-tree-bindings/chosen.txt
> > > > > @@ -73,3 +73,25 @@ Example
> > > > >                 u-boot,spl-boot-order = "same-as-spl",
> > > > > &sdmmc,
> > > > > "/sd
> > > > > hci at fe330000";
> > > > >         };
> > > > >  };
> > > > > +
> > > > > +firmware-loader property
> > > > > +------------------------
> > > > > +Multiple file system firmware loader nodes could be defined
> > > > > in
> > > > > device trees for
> > > > > +multiple storage type and their default partition, then a
> > > > > property
> > > > > +"firmware-loader" can be used to pass default firmware
> > > > > loader
> > > > > +node(default storage type) to the firmware loader driver.
> > > > > +
> > > > > +Example
> > > > > +-------
> > > > > +/ {
> > > > > +       chosen {
> > > > > +               firmware-loader = &fs_loader0;
> > > > > +       };
> > > > > +
> > > > > +       fs_loader0: fs_loader at 0 {
> > > > This should be :
> > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > +       fs_loader0: fs-loader at 0 {
> > > > since hyphen is used for node and property names. Underscore is
> > > > used
> > > > for phandles (so you have that correct).
> > > > 
> > > Okay.
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > +               u-boot,dm-pre-reloc;
> > > > > +               compatible = "fs_loader";
> > > > How about u-boot,fs-loader since this is U-Boot specific I
> > > > think.
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > +               storage_device = "mmc";
> > > > storage-device
> > > > 
> > > Okay
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > +               devpart = "0:1";
> > > > I think you should use a phandle to the node containing the mmc
> > > > device to use.
> > > > 
> > > >    storage-device = <&mmc_3 1>;
> > > > 
> > > > for example (meaning use that MMC, partition 1.
> > > > 
> > > How to get device number based on node of a storage?
> > > This could make design more complicated because this driver is
> > > designed
> > > to support both fdt and without fdt(U-boot env and hard coding in
> > > driver).
> > I think it is fine to support the environment as a way of providing
> > this info, but there is no need to support non-DM. Everything
> > should
> > be converting to DM now.
> > 
> Ok,i would keep the DM and environment support.
> 
> > 
> > We have:
> > 
> > uclass_get_device_by_phandle_id()
> > device_get_global_by_of_offset()
> > 
> > I think we need to create a function called
> > 
> > device_get_global_by_phandle_id()
> > 
> > which can be used to obtain the device based on a phandle.
> > 
> I means the device number in the struct blk_desc, block device. I
> don't
>  know how the device number is built up during driver model init. Is
> there a function to retrive the device number?
After roughly checking the blk-uclass.c, i think we can get the device
number for blok device and also mainstain the consistent interface with
UBI(non block device), we need two parameters in FDT,
1) storage-interface:
block device - 
	"ide"
	"scsi"
	"atapi"
	"usb"
	"doc"
	"mmc"
	"sd"
	"sata"
	"host"
	"nvme"
	"efi"

non block device
	"ubi"

2) phandle for physical storage node(parent node) which contain the
uclass support as listed below for block devices:
	UCLASS_IDE
	UCLASS_SCSI
	UCLASS_INVALID
	UCLASS_MASS_STORAGE
	UCLASS_INVALID
	UCLASS_MMC
	UCLASS_INVALID
	UCLASS_AHCI
	UCLASS_ROOT
	UCLASS_NVME
	UCLASS_EFI
Above nodes must contain the child node UCLASS_BLK(this node contains
device number)

No node required for UBI interface.

I can create a new function, which is specific for getting the device
number from the parent node(block device) which contains the child node
UCLASS_BLK. May be that function can be put into fs.c.

I'm not sure would this work as expected, any ideas/thoughts on my
proposal?
> > 
> > > 
> > > 
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > +       };
> > > > > +};
> > > > > 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..78bea66
> > > > > --- /dev/null
> > > > > +++ b/doc/device-tree-bindings/misc/fs_loader.txt
> > > > > @@ -0,0 +1,52 @@
> > > > > +* 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 storage device and partition search set for mmc,
> > > > > usb,
> > > > > sata and
> > > > > +ubi in device tree source as shown in below:
> > > > > +
> > > > > +       Example of storage type and device partition search
> > > > > set
> > > > > for
> > > > > mmc, usb,
> > > > > +       sata and ubi as shown in below:
> > > > > +       Example for mmc:
> > > > > +       fs_loader0: fs_loader at 0 {
> > > > > +               u-boot,dm-pre-reloc;
> > > > > +               compatible = "fs_loader";
> > > > > +               storage_device = "mmc";
> > > > > +               devpart = "0:1";
> > > > > +       };
> > > > > +
> > > > > +       Example for usb:
> > > > > +       fs_loader1: fs_loader at 1 {
> > > > > +               u-boot,dm-pre-reloc;
> > > > > +               compatible = "fs_loader";
> > > > > +               storage_device = "usb";
> > > > > +               devpart = "0:1";
> > > > > +       };
> > > > > +
> > > > > +       Example for sata:
> > > > > +       fs_loader2: fs_loader at 2 {
> > > > > +               u-boot,dm-pre-reloc;
> > > > > +               compatible = "fs_loader";
> > > > > +               storage_device = "sata";
> > > > > +               devpart = "0:1";
> > > > > +       };
> > > > > +
> > > > > +       Example for ubi:
> > > > > +       fs_loader3: fs_loader at 3 {
> > > > > +               u-boot,dm-pre-reloc;
> > > > > +               compatible = "fs_loader";
> > > > > +               storage_device = "ubi";
> > > > > +               mtdpart = "UBI",
> > > > > +               ubivol = "ubi0";
> > > > I'm not sure about ubi. It needs to refer to a particular
> > > > device
> > > > I
> > > > suppose. How do we know the mapping from ubi to device?
> > > > 
> > > Actually this design is based on file system implementation which
> > > is
> > > abstract from physical device, storage_device is used as
> > > interface
> > > of
> > > file system, so nand and qspi may having the same ubi interface.
> > > 
> > > May be i can change the storage_device into storage_interface to
> > > avoid
> > > confusing.
> > OK I see. Well I suppose this is OK then, at least for now. I'm not
> > have any great ideas on how to specify which ubi storage device to
> > use. Let's assume we only have one for now.
> > 
> > Regards,
> > Simon

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

[Anatolij Gustschin]

Hi,

please see some comments below.

On Mon, 25 Jun 2018 21:28:58 +0800
tien.fong.chee at intel.com tien.fong.chee at intel.com wrote:

> +/**
> + * _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;
> +	}

please add a note to API description that the API user should
free() *firmware_p and *firmware_p->priv structs after usage of
request_firmware_into_buf(), otherwise it will always leak memory
while subsequent calls of request_firmware_into_buf() with the
same *firmware_p argument.

Or probably we should better allow request_firmware_into_buf() to
be called multiple times with prepared *firmware_p stuct for
reloading the same firmware image when needed.
Then request_firmware_into_buf() should check if the given
*firmware_p stuct is already initialized and must not call
_request_firmware_prepare() in this case.

> +
> +	fw_priv->name = name;
> +	fw_priv->offset = offset;
> +	firmware->data = dbuf;
> +	firmware->size = size;
> +	firmware->priv = fw_priv;
> +	*firmware_p = firmware;
> +
> +	return 0;
> +}

--
Anatolij

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

[Simon Glass]

Hi Tien Fong,

On 28 June 2018 at 01:58, Chee, Tien Fong <tien.fong.chee@intel.com> wrote:
> On Thu, 2018-06-28 at 01:04 -0700, Chee, Tien Fong wrote:
>> On Wed, 2018-06-27 at 14:03 -0700, Simon Glass wrote:
>> >
>> > Hi Tien Fong,
>> >
>> > On 27 June 2018 at 01:32, Chee, Tien Fong <tien.fong.chee@intel.com
>> > >
>> > wrote:
>> > >
>> > >
>> > > On Mon, 2018-06-25 at 21:58 -0600, Simon Glass wrote:
>> > > >
>> > > >
>> > > > Hi,
>> > > >
>> > > > On 25 June 2018 at 07:28,  <tien.fong.chee@intel.com> wrote:
>> > > > >
>> > > > >
>> > > > >
>> > > > > 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/chosen.txt         | 22
>> > > > > ++++++++++++
>> > > > >  doc/device-tree-bindings/misc/fs_loader.txt | 52
>> > > > > +++++++++++++++++++++++++++++
>> > > > >  2 files changed, 74 insertions(+)
>> > > > >  create mode 100644 doc/device-tree-
>> > > > > bindings/misc/fs_loader.txt
>> > > > >
>> > > > > diff --git a/doc/device-tree-bindings/chosen.txt
>> > > > > b/doc/device-
>> > > > > tree-
>> > > > > bindings/chosen.txt
>> > > > > index c96b8f7..738673c 100644
>> > > > > --- a/doc/device-tree-bindings/chosen.txt
>> > > > > +++ b/doc/device-tree-bindings/chosen.txt
>> > > > > @@ -73,3 +73,25 @@ Example
>> > > > >                 u-boot,spl-boot-order = "same-as-spl",
>> > > > > &sdmmc,
>> > > > > "/sd
>> > > > > hci at fe330000";
>> > > > >         };
>> > > > >  };
>> > > > > +
>> > > > > +firmware-loader property
>> > > > > +------------------------
>> > > > > +Multiple file system firmware loader nodes could be defined
>> > > > > in
>> > > > > device trees for
>> > > > > +multiple storage type and their default partition, then a
>> > > > > property
>> > > > > +"firmware-loader" can be used to pass default firmware
>> > > > > loader
>> > > > > +node(default storage type) to the firmware loader driver.
>> > > > > +
>> > > > > +Example
>> > > > > +-------
>> > > > > +/ {
>> > > > > +       chosen {
>> > > > > +               firmware-loader = &fs_loader0;
>> > > > > +       };
>> > > > > +
>> > > > > +       fs_loader0: fs_loader at 0 {
>> > > > This should be :
>> > > >
>> > > > >
>> > > > >
>> > > > >
>> > > > > +       fs_loader0: fs-loader at 0 {
>> > > > since hyphen is used for node and property names. Underscore is
>> > > > used
>> > > > for phandles (so you have that correct).
>> > > >
>> > > Okay.
>> > > >
>> > > >
>> > > > >
>> > > > >
>> > > > >
>> > > > > +               u-boot,dm-pre-reloc;
>> > > > > +               compatible = "fs_loader";
>> > > > How about u-boot,fs-loader since this is U-Boot specific I
>> > > > think.
>> > > >
>> > > >
>> > > > >
>> > > > >
>> > > > >
>> > > > > +               storage_device = "mmc";
>> > > > storage-device
>> > > >
>> > > Okay
>> > > >
>> > > >
>> > > > >
>> > > > >
>> > > > >
>> > > > > +               devpart = "0:1";
>> > > > I think you should use a phandle to the node containing the mmc
>> > > > device to use.
>> > > >
>> > > >    storage-device = <&mmc_3 1>;
>> > > >
>> > > > for example (meaning use that MMC, partition 1.
>> > > >
>> > > How to get device number based on node of a storage?
>> > > This could make design more complicated because this driver is
>> > > designed
>> > > to support both fdt and without fdt(U-boot env and hard coding in
>> > > driver).
>> > I think it is fine to support the environment as a way of providing
>> > this info, but there is no need to support non-DM. Everything
>> > should
>> > be converting to DM now.
>> >
>> Ok,i would keep the DM and environment support.
>>
>> >
>> > We have:
>> >
>> > uclass_get_device_by_phandle_id()
>> > device_get_global_by_of_offset()
>> >
>> > I think we need to create a function called
>> >
>> > device_get_global_by_phandle_id()
>> >
>> > which can be used to obtain the device based on a phandle.
>> >
>> I means the device number in the struct blk_desc, block device. I
>> don't
>>  know how the device number is built up during driver model init. Is
>> there a function to retrive the device number?
> After roughly checking the blk-uclass.c, i think we can get the device
> number for blok device and also mainstain the consistent interface with
> UBI(non block device), we need two parameters in FDT,
> 1) storage-interface:
> block device -
>         "ide"
>         "scsi"
>         "atapi"
>         "usb"
>         "doc"
>         "mmc"
>         "sd"
>         "sata"
>         "host"
>         "nvme"
>         "efi"

Do you need this? Can it not be obtained from uclass_get_name() using
the phandle (below)?

>
> non block device
>         "ubi"
>
> 2) phandle for physical storage node(parent node) which contain the
> uclass support as listed below for block devices:
>         UCLASS_IDE
>         UCLASS_SCSI
>         UCLASS_INVALID
>         UCLASS_MASS_STORAGE
>         UCLASS_INVALID
>         UCLASS_MMC
>         UCLASS_INVALID
>         UCLASS_AHCI
>         UCLASS_ROOT
>         UCLASS_NVME
>         UCLASS_EFI
> Above nodes must contain the child node UCLASS_BLK(this node contains
> device number)
>
> No node required for UBI interface.
>
> I can create a new function, which is specific for getting the device
> number from the parent node(block device) which contains the child node
> UCLASS_BLK. May be that function can be put into fs.c.
>
> I'm not sure would this work as expected, any ideas/thoughts on my
> proposal?

Sounds right to me.

[...]

Regards,
Simon

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

[Chee, Tien Fong]

On Fri, 2018-06-29 at 14:13 +0200, Anatolij Gustschin wrote:
> Hi,
> 
> please see some comments below.
> 
> On Mon, 25 Jun 2018 21:28:58 +0800
> tien.fong.chee at intel.com tien.fong.chee at intel.com wrote:
> 
> > 
> > +/**
> > + * _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;
> > +	}
> please add a note to API description that the API user should
> free() *firmware_p and *firmware_p->priv structs after usage of
> request_firmware_into_buf(), otherwise it will always leak memory
> while subsequent calls of request_firmware_into_buf() with the
> same *firmware_p argument.
> 
Okay, i will add the description into doc. I can create a function to
release memory allocation and setting both *firmware_p and fw_priv to
null for user.
> Or probably we should better allow request_firmware_into_buf() to
> be called multiple times with prepared *firmware_p stuct for
> reloading the same firmware image when needed.
> Then request_firmware_into_buf() should check if the given
> *firmware_p stuct is already initialized and must not call
> _request_firmware_prepare() in this case.
> 
Okay, this should work in this way by checking both *firmware_p and
fw_priv, if they are not null, memory allocation would be skipped for
better performance. However, *firmware_p always need to get updated
with function arguments, because it could be chunk by chunk
transferring if image is larger than available buffer.
> > 
> > +
> > +	fw_priv->name = name;
> > +	fw_priv->offset = offset;
> > +	firmware->data = dbuf;
> > +	firmware->size = size;
> > +	firmware->priv = fw_priv;
> > +	*firmware_p = firmware;
> > +
> > +	return 0;
> > +}
> --
> Anatolij
> 

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

[Chee, Tien Fong]

On Fri, 2018-06-29 at 21:19 -0700, Simon Glass wrote:
> Hi Tien Fong,
> 
> On 28 June 2018 at 01:58, Chee, Tien Fong <tien.fong.chee@intel.com>
> wrote:
> > 
> > On Thu, 2018-06-28 at 01:04 -0700, Chee, Tien Fong wrote:
> > > 
> > > On Wed, 2018-06-27 at 14:03 -0700, Simon Glass wrote:
> > > > 
> > > > 
> > > > Hi Tien Fong,
> > > > 
> > > > On 27 June 2018 at 01:32, Chee, Tien Fong <tien.fong.chee@intel
> > > > .com
> > > > > 
> > > > > 
> > > > wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > On Mon, 2018-06-25 at 21:58 -0600, Simon Glass wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > Hi,
> > > > > > 
> > > > > > On 25 June 2018 at 07:28,  <tien.fong.chee@intel.com>
> > > > > > wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 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/chosen.txt         | 22
> > > > > > > ++++++++++++
> > > > > > >  doc/device-tree-bindings/misc/fs_loader.txt | 52
> > > > > > > +++++++++++++++++++++++++++++
> > > > > > >  2 files changed, 74 insertions(+)
> > > > > > >  create mode 100644 doc/device-tree-
> > > > > > > bindings/misc/fs_loader.txt
> > > > > > > 
> > > > > > > diff --git a/doc/device-tree-bindings/chosen.txt
> > > > > > > b/doc/device-
> > > > > > > tree-
> > > > > > > bindings/chosen.txt
> > > > > > > index c96b8f7..738673c 100644
> > > > > > > --- a/doc/device-tree-bindings/chosen.txt
> > > > > > > +++ b/doc/device-tree-bindings/chosen.txt
> > > > > > > @@ -73,3 +73,25 @@ Example
> > > > > > >                 u-boot,spl-boot-order = "same-as-spl",
> > > > > > > &sdmmc,
> > > > > > > "/sd
> > > > > > > hci at fe330000";
> > > > > > >         };
> > > > > > >  };
> > > > > > > +
> > > > > > > +firmware-loader property
> > > > > > > +------------------------
> > > > > > > +Multiple file system firmware loader nodes could be
> > > > > > > defined
> > > > > > > in
> > > > > > > device trees for
> > > > > > > +multiple storage type and their default partition, then
> > > > > > > a
> > > > > > > property
> > > > > > > +"firmware-loader" can be used to pass default firmware
> > > > > > > loader
> > > > > > > +node(default storage type) to the firmware loader
> > > > > > > driver.
> > > > > > > +
> > > > > > > +Example
> > > > > > > +-------
> > > > > > > +/ {
> > > > > > > +       chosen {
> > > > > > > +               firmware-loader = &fs_loader0;
> > > > > > > +       };
> > > > > > > +
> > > > > > > +       fs_loader0: fs_loader at 0 {
> > > > > > This should be :
> > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > +       fs_loader0: fs-loader at 0 {
> > > > > > since hyphen is used for node and property names.
> > > > > > Underscore is
> > > > > > used
> > > > > > for phandles (so you have that correct).
> > > > > > 
> > > > > Okay.
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > +               u-boot,dm-pre-reloc;
> > > > > > > +               compatible = "fs_loader";
> > > > > > How about u-boot,fs-loader since this is U-Boot specific I
> > > > > > think.
> > > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > +               storage_device = "mmc";
> > > > > > storage-device
> > > > > > 
> > > > > Okay
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > +               devpart = "0:1";
> > > > > > I think you should use a phandle to the node containing the
> > > > > > mmc
> > > > > > device to use.
> > > > > > 
> > > > > >    storage-device = <&mmc_3 1>;
> > > > > > 
> > > > > > for example (meaning use that MMC, partition 1.
> > > > > > 
> > > > > How to get device number based on node of a storage?
> > > > > This could make design more complicated because this driver
> > > > > is
> > > > > designed
> > > > > to support both fdt and without fdt(U-boot env and hard
> > > > > coding in
> > > > > driver).
> > > > I think it is fine to support the environment as a way of
> > > > providing
> > > > this info, but there is no need to support non-DM. Everything
> > > > should
> > > > be converting to DM now.
> > > > 
> > > Ok,i would keep the DM and environment support.
> > > 
> > > > 
> > > > 
> > > > We have:
> > > > 
> > > > uclass_get_device_by_phandle_id()
> > > > device_get_global_by_of_offset()
> > > > 
> > > > I think we need to create a function called
> > > > 
> > > > device_get_global_by_phandle_id()
> > > > 
> > > > which can be used to obtain the device based on a phandle.
> > > > 
> > > I means the device number in the struct blk_desc, block device. I
> > > don't
> > >  know how the device number is built up during driver model init.
> > > Is
> > > there a function to retrive the device number?
> > After roughly checking the blk-uclass.c, i think we can get the
> > device
> > number for blok device and also mainstain the consistent interface
> > with
> > UBI(non block device), we need two parameters in FDT,
> > 1) storage-interface:
> > block device -
> >         "ide"
> >         "scsi"
> >         "atapi"
> >         "usb"
> >         "doc"
> >         "mmc"
> >         "sd"
> >         "sata"
> >         "host"
> >         "nvme"
> >         "efi"
> Do you need this? Can it not be obtained from uclass_get_name() using
> the phandle (below)?
> 
The purpose of above parameter is used to differentiate the block
devices and non block devices(like UBI). This function would be called
if it is block device.

Is the name is standard defined as above block devices string?
> > 
> > 
> > non block device
> >         "ubi"
> > 
> > 2) phandle for physical storage node(parent node) which contain the
> > uclass support as listed below for block devices:
> >         UCLASS_IDE
> >         UCLASS_SCSI
> >         UCLASS_INVALID
> >         UCLASS_MASS_STORAGE
> >         UCLASS_INVALID
> >         UCLASS_MMC
> >         UCLASS_INVALID
> >         UCLASS_AHCI
> >         UCLASS_ROOT
> >         UCLASS_NVME
> >         UCLASS_EFI
> > Above nodes must contain the child node UCLASS_BLK(this node
> > contains
> > device number)
> > 
> > No node required for UBI interface.
> > 
> > I can create a new function, which is specific for getting the
> > device
> > number from the parent node(block device) which contains the child
> > node
> > UCLASS_BLK. May be that function can be put into fs.c.
> > 
> > I'm not sure would this work as expected, any ideas/thoughts on my
> > proposal?
> Sounds right to me.
> 
Is that possible one parent device(like MMC) contains multiple child
devices in UCLASS_BLK?
> [...]
> 
> Regards,
> Simon

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

[Simon Glass]

Hi Tien Fong,

On 2 July 2018 at 01:26, Chee, Tien Fong <tien.fong.chee@intel.com> wrote:
> On Fri, 2018-06-29 at 21:19 -0700, Simon Glass wrote:
>> Hi Tien Fong,
>>
>> On 28 June 2018 at 01:58, Chee, Tien Fong <tien.fong.chee@intel.com>
>> wrote:
>> >
>> > On Thu, 2018-06-28 at 01:04 -0700, Chee, Tien Fong wrote:
>> > >
>> > > On Wed, 2018-06-27 at 14:03 -0700, Simon Glass wrote:
>> > > >
>> > > >
>> > > > Hi Tien Fong,
>> > > >
>> > > > On 27 June 2018 at 01:32, Chee, Tien Fong <tien.fong.chee@intel
>> > > > .com
>> > > > >
>> > > > >
>> > > > wrote:
>> > > > >
>> > > > >
>> > > > >
>> > > > > On Mon, 2018-06-25 at 21:58 -0600, Simon Glass wrote:
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > > Hi,
>> > > > > >
>> > > > > > On 25 June 2018 at 07:28,  <tien.fong.chee@intel.com>
>> > > > > > wrote:
>> > > > > > >
>> > > > > > >
>> > > > > > >
>> > > > > > >
>> > > > > > > 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/chosen.txt         | 22
>> > > > > > > ++++++++++++
>> > > > > > >  doc/device-tree-bindings/misc/fs_loader.txt | 52
>> > > > > > > +++++++++++++++++++++++++++++
>> > > > > > >  2 files changed, 74 insertions(+)
>> > > > > > >  create mode 100644 doc/device-tree-
>> > > > > > > bindings/misc/fs_loader.txt
>> > > > > > >
>> > > > > > > diff --git a/doc/device-tree-bindings/chosen.txt
>> > > > > > > b/doc/device-
>> > > > > > > tree-
>> > > > > > > bindings/chosen.txt
>> > > > > > > index c96b8f7..738673c 100644
>> > > > > > > --- a/doc/device-tree-bindings/chosen.txt
>> > > > > > > +++ b/doc/device-tree-bindings/chosen.txt
>> > > > > > > @@ -73,3 +73,25 @@ Example
>> > > > > > >                 u-boot,spl-boot-order = "same-as-spl",
>> > > > > > > &sdmmc,
>> > > > > > > "/sd
>> > > > > > > hci at fe330000";
>> > > > > > >         };
>> > > > > > >  };
>> > > > > > > +
>> > > > > > > +firmware-loader property
>> > > > > > > +------------------------
>> > > > > > > +Multiple file system firmware loader nodes could be
>> > > > > > > defined
>> > > > > > > in
>> > > > > > > device trees for
>> > > > > > > +multiple storage type and their default partition, then
>> > > > > > > a
>> > > > > > > property
>> > > > > > > +"firmware-loader" can be used to pass default firmware
>> > > > > > > loader
>> > > > > > > +node(default storage type) to the firmware loader
>> > > > > > > driver.
>> > > > > > > +
>> > > > > > > +Example
>> > > > > > > +-------
>> > > > > > > +/ {
>> > > > > > > +       chosen {
>> > > > > > > +               firmware-loader = &fs_loader0;
>> > > > > > > +       };
>> > > > > > > +
>> > > > > > > +       fs_loader0: fs_loader at 0 {
>> > > > > > This should be :
>> > > > > >
>> > > > > > >
>> > > > > > >
>> > > > > > >
>> > > > > > >
>> > > > > > > +       fs_loader0: fs-loader at 0 {
>> > > > > > since hyphen is used for node and property names.
>> > > > > > Underscore is
>> > > > > > used
>> > > > > > for phandles (so you have that correct).
>> > > > > >
>> > > > > Okay.
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > > >
>> > > > > > >
>> > > > > > >
>> > > > > > >
>> > > > > > > +               u-boot,dm-pre-reloc;
>> > > > > > > +               compatible = "fs_loader";
>> > > > > > How about u-boot,fs-loader since this is U-Boot specific I
>> > > > > > think.
>> > > > > >
>> > > > > >
>> > > > > > >
>> > > > > > >
>> > > > > > >
>> > > > > > >
>> > > > > > > +               storage_device = "mmc";
>> > > > > > storage-device
>> > > > > >
>> > > > > Okay
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > > >
>> > > > > > >
>> > > > > > >
>> > > > > > >
>> > > > > > > +               devpart = "0:1";
>> > > > > > I think you should use a phandle to the node containing the
>> > > > > > mmc
>> > > > > > device to use.
>> > > > > >
>> > > > > >    storage-device = <&mmc_3 1>;
>> > > > > >
>> > > > > > for example (meaning use that MMC, partition 1.
>> > > > > >
>> > > > > How to get device number based on node of a storage?
>> > > > > This could make design more complicated because this driver
>> > > > > is
>> > > > > designed
>> > > > > to support both fdt and without fdt(U-boot env and hard
>> > > > > coding in
>> > > > > driver).
>> > > > I think it is fine to support the environment as a way of
>> > > > providing
>> > > > this info, but there is no need to support non-DM. Everything
>> > > > should
>> > > > be converting to DM now.
>> > > >
>> > > Ok,i would keep the DM and environment support.
>> > >
>> > > >
>> > > >
>> > > > We have:
>> > > >
>> > > > uclass_get_device_by_phandle_id()
>> > > > device_get_global_by_of_offset()
>> > > >
>> > > > I think we need to create a function called
>> > > >
>> > > > device_get_global_by_phandle_id()
>> > > >
>> > > > which can be used to obtain the device based on a phandle.
>> > > >
>> > > I means the device number in the struct blk_desc, block device. I
>> > > don't
>> > >  know how the device number is built up during driver model init.
>> > > Is
>> > > there a function to retrive the device number?
>> > After roughly checking the blk-uclass.c, i think we can get the
>> > device
>> > number for blok device and also mainstain the consistent interface
>> > with
>> > UBI(non block device), we need two parameters in FDT,
>> > 1) storage-interface:
>> > block device -
>> >         "ide"
>> >         "scsi"
>> >         "atapi"
>> >         "usb"
>> >         "doc"
>> >         "mmc"
>> >         "sd"
>> >         "sata"
>> >         "host"
>> >         "nvme"
>> >         "efi"
>> Do you need this? Can it not be obtained from uclass_get_name() using
>> the phandle (below)?
>>
> The purpose of above parameter is used to differentiate the block
> devices and non block devices(like UBI). This function would be called
> if it is block device.

Did you see the strings in the uclass drivers? E.g.:

UCLASS_DRIVER(mmc) = {
        .id             = UCLASS_MMC,
        .name           = "mmc",
        .flags          = DM_UC_FLAG_SEQ_ALIAS,
        .per_device_auto_alloc_size = sizeof(struct mmc_uclass_priv),
};

>
> Is the name is standard defined as above block devices string?

I don't understand this. The same is defined for all uclasses that can
have a block device as a child.

>> >
>> >
>> > non block device
>> >         "ubi"
>> >
>> > 2) phandle for physical storage node(parent node) which contain the
>> > uclass support as listed below for block devices:
>> >         UCLASS_IDE
>> >         UCLASS_SCSI
>> >         UCLASS_INVALID
>> >         UCLASS_MASS_STORAGE
>> >         UCLASS_INVALID
>> >         UCLASS_MMC
>> >         UCLASS_INVALID
>> >         UCLASS_AHCI
>> >         UCLASS_ROOT
>> >         UCLASS_NVME
>> >         UCLASS_EFI
>> > Above nodes must contain the child node UCLASS_BLK(this node
>> > contains
>> > device number)
>> >
>> > No node required for UBI interface.
>> >
>> > I can create a new function, which is specific for getting the
>> > device
>> > number from the parent node(block device) which contains the child
>> > node
>> > UCLASS_BLK. May be that function can be put into fs.c.
>> >
>> > I'm not sure would this work as expected, any ideas/thoughts on my
>> > proposal?
>> Sounds right to me.
>>
> Is that possible one parent device(like MMC) contains multiple child
> devices in UCLASS_BLK?

No, only one is supported.

Regards,
Simon

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

[Chee, Tien Fong]

On Sun, 2018-07-08 at 20:39 -0600, Simon Glass wrote:
> Hi Tien Fong,
> 
> On 2 July 2018 at 01:26, Chee, Tien Fong <tien.fong.chee@intel.com>
> wrote:
> > 
> > On Fri, 2018-06-29 at 21:19 -0700, Simon Glass wrote:
> > > 
> > > Hi Tien Fong,
> > > 
> > > On 28 June 2018 at 01:58, Chee, Tien Fong <tien.fong.chee@intel.c
> > > om>
> > > wrote:
> > > > 
> > > > 
> > > > On Thu, 2018-06-28 at 01:04 -0700, Chee, Tien Fong wrote:
> > > > > 
> > > > > 
> > > > > On Wed, 2018-06-27 at 14:03 -0700, Simon Glass wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > Hi Tien Fong,
> > > > > > 
> > > > > > On 27 June 2018 at 01:32, Chee, Tien Fong <tien.fong.chee@i
> > > > > > ntel
> > > > > > .com
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > On Mon, 2018-06-25 at 21:58 -0600, Simon Glass wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > Hi,
> > > > > > > > 
> > > > > > > > On 25 June 2018 at 07:28,  <tien.fong.chee@intel.com>
> > > > > > > > wrote:
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 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.c
> > > > > > > > > om>
> > > > > > > > > ---
> > > > > > > > >  doc/device-tree-bindings/chosen.txt         | 22
> > > > > > > > > ++++++++++++
> > > > > > > > >  doc/device-tree-bindings/misc/fs_loader.txt | 52
> > > > > > > > > +++++++++++++++++++++++++++++
> > > > > > > > >  2 files changed, 74 insertions(+)
> > > > > > > > >  create mode 100644 doc/device-tree-
> > > > > > > > > bindings/misc/fs_loader.txt
> > > > > > > > > 
> > > > > > > > > diff --git a/doc/device-tree-bindings/chosen.txt
> > > > > > > > > b/doc/device-
> > > > > > > > > tree-
> > > > > > > > > bindings/chosen.txt
> > > > > > > > > index c96b8f7..738673c 100644
> > > > > > > > > --- a/doc/device-tree-bindings/chosen.txt
> > > > > > > > > +++ b/doc/device-tree-bindings/chosen.txt
> > > > > > > > > @@ -73,3 +73,25 @@ Example
> > > > > > > > >                 u-boot,spl-boot-order = "same-as-
> > > > > > > > > spl",
> > > > > > > > > &sdmmc,
> > > > > > > > > "/sd
> > > > > > > > > hci at fe330000";
> > > > > > > > >         };
> > > > > > > > >  };
> > > > > > > > > +
> > > > > > > > > +firmware-loader property
> > > > > > > > > +------------------------
> > > > > > > > > +Multiple file system firmware loader nodes could be
> > > > > > > > > defined
> > > > > > > > > in
> > > > > > > > > device trees for
> > > > > > > > > +multiple storage type and their default partition,
> > > > > > > > > then
> > > > > > > > > a
> > > > > > > > > property
> > > > > > > > > +"firmware-loader" can be used to pass default
> > > > > > > > > firmware
> > > > > > > > > loader
> > > > > > > > > +node(default storage type) to the firmware loader
> > > > > > > > > driver.
> > > > > > > > > +
> > > > > > > > > +Example
> > > > > > > > > +-------
> > > > > > > > > +/ {
> > > > > > > > > +       chosen {
> > > > > > > > > +               firmware-loader = &fs_loader0;
> > > > > > > > > +       };
> > > > > > > > > +
> > > > > > > > > +       fs_loader0: fs_loader at 0 {
> > > > > > > > This should be :
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > +       fs_loader0: fs-loader at 0 {
> > > > > > > > since hyphen is used for node and property names.
> > > > > > > > Underscore is
> > > > > > > > used
> > > > > > > > for phandles (so you have that correct).
> > > > > > > > 
> > > > > > > Okay.
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > +               u-boot,dm-pre-reloc;
> > > > > > > > > +               compatible = "fs_loader";
> > > > > > > > How about u-boot,fs-loader since this is U-Boot
> > > > > > > > specific I
> > > > > > > > think.
> > > > > > > > 
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > +               storage_device = "mmc";
> > > > > > > > storage-device
> > > > > > > > 
> > > > > > > Okay
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > +               devpart = "0:1";
> > > > > > > > I think you should use a phandle to the node containing
> > > > > > > > the
> > > > > > > > mmc
> > > > > > > > device to use.
> > > > > > > > 
> > > > > > > >    storage-device = <&mmc_3 1>;
> > > > > > > > 
> > > > > > > > for example (meaning use that MMC, partition 1.
> > > > > > > > 
> > > > > > > How to get device number based on node of a storage?
> > > > > > > This could make design more complicated because this
> > > > > > > driver
> > > > > > > is
> > > > > > > designed
> > > > > > > to support both fdt and without fdt(U-boot env and hard
> > > > > > > coding in
> > > > > > > driver).
> > > > > > I think it is fine to support the environment as a way of
> > > > > > providing
> > > > > > this info, but there is no need to support non-DM.
> > > > > > Everything
> > > > > > should
> > > > > > be converting to DM now.
> > > > > > 
> > > > > Ok,i would keep the DM and environment support.
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > We have:
> > > > > > 
> > > > > > uclass_get_device_by_phandle_id()
> > > > > > device_get_global_by_of_offset()
> > > > > > 
> > > > > > I think we need to create a function called
> > > > > > 
> > > > > > device_get_global_by_phandle_id()
> > > > > > 
> > > > > > which can be used to obtain the device based on a phandle.
> > > > > > 
> > > > > I means the device number in the struct blk_desc, block
> > > > > device. I
> > > > > don't
> > > > >  know how the device number is built up during driver model
> > > > > init.
> > > > > Is
> > > > > there a function to retrive the device number?
> > > > After roughly checking the blk-uclass.c, i think we can get the
> > > > device
> > > > number for blok device and also mainstain the consistent
> > > > interface
> > > > with
> > > > UBI(non block device), we need two parameters in FDT,
> > > > 1) storage-interface:
> > > > block device -
> > > >         "ide"
> > > >         "scsi"
> > > >         "atapi"
> > > >         "usb"
> > > >         "doc"
> > > >         "mmc"
> > > >         "sd"
> > > >         "sata"
> > > >         "host"
> > > >         "nvme"
> > > >         "efi"
> > > Do you need this? Can it not be obtained from uclass_get_name()
> > > using
> > > the phandle (below)?
> > > 
> > The purpose of above parameter is used to differentiate the block
> > devices and non block devices(like UBI). This function would be
> > called
> > if it is block device.
> Did you see the strings in the uclass drivers? E.g.:
> 
> UCLASS_DRIVER(mmc) = {
>         .id             = UCLASS_MMC,
>         .name           = "mmc",
>         .flags          = DM_UC_FLAG_SEQ_ALIAS,
>         .per_device_auto_alloc_size = sizeof(struct mmc_uclass_priv),
> };
> 
Yeah, but i'm not sure all uclass drivers having same string as above
interface string.

Anyway, i found alternate way using blk structure instead of the
interface string. You can check the v[4] patchset i have submitted from
last week.
> > 
> > 
> > Is the name is standard defined as above block devices string?
> I don't understand this. The same is defined for all uclasses that
> can
> have a block device as a child.
> 
> > 
> > > 
> > > > 
> > > > 
> > > > 
> > > > non block device
> > > >         "ubi"
> > > > 
> > > > 2) phandle for physical storage node(parent node) which contain
> > > > the
> > > > uclass support as listed below for block devices:
> > > >         UCLASS_IDE
> > > >         UCLASS_SCSI
> > > >         UCLASS_INVALID
> > > >         UCLASS_MASS_STORAGE
> > > >         UCLASS_INVALID
> > > >         UCLASS_MMC
> > > >         UCLASS_INVALID
> > > >         UCLASS_AHCI
> > > >         UCLASS_ROOT
> > > >         UCLASS_NVME
> > > >         UCLASS_EFI
> > > > Above nodes must contain the child node UCLASS_BLK(this node
> > > > contains
> > > > device number)
> > > > 
> > > > No node required for UBI interface.
> > > > 
> > > > I can create a new function, which is specific for getting the
> > > > device
> > > > number from the parent node(block device) which contains the
> > > > child
> > > > node
> > > > UCLASS_BLK. May be that function can be put into fs.c.
> > > > 
> > > > I'm not sure would this work as expected, any ideas/thoughts on
> > > > my
> > > > proposal?
> > > Sounds right to me.
> > > 
> > Is that possible one parent device(like MMC) contains multiple
> > child
> > devices in UCLASS_BLK?
> No, only one is supported.
> 
Noted.
> Regards,
> Simon