[U-Boot] [PATCH v8 0/4] Generic firmware loader

[U-Boot] [PATCH v8 0/4] Generic firmware loader

[tien.fong.chee at intel.com]

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

This patchset contains generic firmware loader which is very close to Linux
firmware loader but for U-Boot framework. Generic 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
Marek Vasut in [v7].

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

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

v7 -> v8 changes:
-----------------
- Return cmd_ubifs_umount() directly.
- Call the UBI functions directly instead of invoking the U-Boot command parser.

Patchset history
----------------
[v1]: https://www.mail-archive.com/u-boot at lists.denx.de/msg271905.html
[v2]: https://www.mail-archive.com/u-boot at lists.denx.de/msg271979.html
[v3]: https://www.mail-archive.com/u-boot at lists.denx.de/msg272039.html
[v4]: https://www.mail-archive.com/u-boot at lists.denx.de/msg272432.html
[v5]: https://www.mail-archive.com/u-boot at lists.denx.de/msg272771.html
[v6]: https://www.mail-archive.com/u-boot at lists.denx.de/msg273961.html

Tien Fong Chee (4):
  spl: Remove static declaration on spl_mmc_find_device function
  cmd: ubifs: Move ubifs_initialized checking into cmd_ubifs_umount()
  cmd: ubifs: Factor out some checking codes into cmd_ubifs_mount()
  common: Generic firmware loader for file system

 cmd/ubifs.c                |  40 +++---
 common/Kconfig             |   9 ++
 common/Makefile            |   1 +
 common/fs_loader.c         | 320 +++++++++++++++++++++++++++++++++++++++++++++
 common/spl/spl_mmc.c       |   2 +-
 doc/README.firmware_loader |  76 +++++++++++
 include/fs_loader.h        |  28 ++++
 include/spl.h              |   2 +
 include/ubi_uboot.h        |   2 +
 9 files changed, 462 insertions(+), 18 deletions(-)
 create mode 100644 common/fs_loader.c
 create mode 100644 doc/README.firmware_loader
 create mode 100644 include/fs_loader.h

-- 
2.2.0

[U-Boot] [PATCH v8 1/4] spl: Remove static declaration on spl_mmc_find_device function

[tien.fong.chee at intel.com]

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

This patch removes the static declation on spl_mmc_find_device_function
so this function is accessible by the caller from other file. This patch
is required for later patch.

Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
Reviewed-by: Tom Rini <trini@konsulko.com>
---
 common/spl/spl_mmc.c | 2 +-
 include/spl.h        | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
index b57e0b0..183d05a 100644
--- a/common/spl/spl_mmc.c
+++ b/common/spl/spl_mmc.c
@@ -114,7 +114,7 @@ static int spl_mmc_get_device_index(u32 boot_device)
 	return -ENODEV;
 }
 
-static int spl_mmc_find_device(struct mmc **mmcp, u32 boot_device)
+int spl_mmc_find_device(struct mmc **mmcp, u32 boot_device)
 {
 #if CONFIG_IS_ENABLED(DM_MMC)
 	struct udevice *dev;
diff --git a/include/spl.h b/include/spl.h
index c14448b..60424d7 100644
--- a/include/spl.h
+++ b/include/spl.h
@@ -12,6 +12,7 @@
 /* Platform-specific defines */
 #include <linux/compiler.h>
 #include <asm/spl.h>
+#include <mmc.h>
 
 /* Value in r0 indicates we booted from U-Boot */
 #define UBOOT_NOT_LOADED_FROM_SPL	0x13578642
@@ -83,6 +84,7 @@ void preloader_console_init(void);
 u32 spl_boot_device(void);
 u32 spl_boot_mode(const u32 boot_device);
 void spl_set_bd(void);
+int spl_mmc_find_device(struct mmc **mmcp, u32 boot_device);
 
 /**
  * spl_set_header_raw_uboot() - Set up a standard SPL image structure
-- 
2.2.0

[U-Boot] [PATCH v8 2/4] cmd: ubifs: Move ubifs_initialized checking into cmd_ubifs_umount()

[tien.fong.chee at intel.com]

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

cmd_ubifs_umount() function would be called directly instead of involving
whole command machinery in generic firmware loader, so checking on
ubifs_initialized status need to be done in cmd_ubifs_umount() without
breaking original functionality design.

Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>64db5518baa2ea1a8a0e81cc485d760b850c7052
---
 cmd/ubifs.c         | 18 +++++++++---------
 include/ubi_uboot.h |  1 +
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/cmd/ubifs.c b/cmd/ubifs.c
index 5e9d357..0d059ca 100644
--- a/cmd/ubifs.c
+++ b/cmd/ubifs.c
@@ -51,11 +51,18 @@ int ubifs_is_mounted(void)
 	return ubifs_mounted;
 }
 
-void cmd_ubifs_umount(void)
+int cmd_ubifs_umount(void)
 {
+	if (ubifs_initialized == 0) {
+		printf("No UBIFS volume mounted!\n");
+		return -1;
+	}
+
 	uboot_ubifs_umount();
 	ubifs_mounted = 0;
 	ubifs_initialized = 0;
+
+	return 0;
 }
 
 static int do_ubifs_umount(cmd_tbl_t *cmdtp, int flag, int argc,
@@ -64,14 +71,7 @@ static int do_ubifs_umount(cmd_tbl_t *cmdtp, int flag, int argc,
 	if (argc != 1)
 		return CMD_RET_USAGE;
 
-	if (ubifs_initialized == 0) {
-		printf("No UBIFS volume mounted!\n");
-		return -1;
-	}
-
-	cmd_ubifs_umount();
-
-	return 0;
+	return cmd_ubifs_umount();
 }
 
 static int do_ubifs_ls(cmd_tbl_t *cmdtp, int flag, int argc,
diff --git a/include/ubi_uboot.h b/include/ubi_uboot.h
index 80acbcb..827dbfc 100644
--- a/include/ubi_uboot.h
+++ b/include/ubi_uboot.h
@@ -75,5 +75,6 @@ extern int ubi_volume_write(char *volume, void *buf, size_t size);
 extern int ubi_volume_read(char *volume, char *buf, size_t size);
 
 extern struct ubi_device *ubi_devices[];
+int cmd_ubifs_umount(void);
 
 #endif
-- 
2.2.0

[U-Boot] [PATCH v8 3/4] cmd: ubifs: Factor out some checking codes into cmd_ubifs_mount()

[tien.fong.chee at intel.com]

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

cmd_ubifs_mount() function would be called directly instead of
involving whole command machinery for mounting ubifs in
generic firmware loader, so some checking codes need to be factored out
into cmd_ubifs_mount() without breaking original functionality design.

Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
---
 cmd/ubifs.c         | 22 ++++++++++++++--------
 include/ubi_uboot.h |  1 +
 2 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/cmd/ubifs.c b/cmd/ubifs.c
index 0d059ca..8339bed 100644
--- a/cmd/ubifs.c
+++ b/cmd/ubifs.c
@@ -20,16 +20,10 @@
 static int ubifs_initialized;
 static int ubifs_mounted;
 
-static int do_ubifs_mount(cmd_tbl_t *cmdtp, int flag, int argc,
-				char * const argv[])
+int cmd_ubifs_mount(char *vol_name)
 {
-	char *vol_name;
 	int ret;
 
-	if (argc != 2)
-		return CMD_RET_USAGE;
-
-	vol_name = argv[1];
 	debug("Using volume %s\n", vol_name);
 
 	if (ubifs_initialized == 0) {
@@ -43,7 +37,19 @@ static int do_ubifs_mount(cmd_tbl_t *cmdtp, int flag, int argc,
 
 	ubifs_mounted = 1;
 
-	return 0;
+	return ret;
+}
+static int do_ubifs_mount(cmd_tbl_t *cmdtp, int flag, int argc,
+				char * const argv[])
+{
+	char *vol_name;
+
+	if (argc != 2)
+		return CMD_RET_USAGE;
+
+	vol_name = argv[1];
+
+	return cmd_ubifs_mount(vol_name);
 }
 
 int ubifs_is_mounted(void)
diff --git a/include/ubi_uboot.h b/include/ubi_uboot.h
index 827dbfc..fb300e8 100644
--- a/include/ubi_uboot.h
+++ b/include/ubi_uboot.h
@@ -76,5 +76,6 @@ extern int ubi_volume_read(char *volume, char *buf, size_t size);
 
 extern struct ubi_device *ubi_devices[];
 int cmd_ubifs_umount(void);
+int cmd_ubifs_mount(char *vol_name);
 
 #endif
-- 
2.2.0

[U-Boot] [PATCH v8 4/4] common: Generic firmware 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>
Reviewed-by: Lothar Waßmann <LW@KARO-electronics.de>
---
 common/Kconfig             |   9 ++
 common/Makefile            |   1 +
 common/fs_loader.c         | 320 +++++++++++++++++++++++++++++++++++++++++++++
 doc/README.firmware_loader |  76 +++++++++++
 include/fs_loader.h        |  28 ++++
 5 files changed, 434 insertions(+)
 create mode 100644 common/fs_loader.c
 create mode 100644 doc/README.firmware_loader
 create mode 100644 include/fs_loader.h

diff --git a/common/Kconfig b/common/Kconfig
index 21e067c..cbb0976 100644
--- a/common/Kconfig
+++ b/common/Kconfig
@@ -547,6 +547,15 @@ config DISPLAY_BOARDINFO
 	  when U-Boot starts up. The board function checkboard() is called
 	  to do this.
 
+config GENERIC_FIRMWARE_LOADER
+	bool "Enable generic firmware 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.
+
 menu "Start-up hooks"
 
 config ARCH_EARLY_INIT_R
diff --git a/common/Makefile b/common/Makefile
index c7bde23..92d8fb3 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -134,3 +134,4 @@ obj-$(CONFIG_$(SPL_)LOG) += log.o
 obj-$(CONFIG_$(SPL_)LOG_CONSOLE) += log_console.o
 obj-y += s_record.o
 obj-y += xyzModem.o
+obj-$(CONFIG_GENERIC_FIRMWARE_LOADER) += fs_loader.o
diff --git a/common/fs_loader.c b/common/fs_loader.c
new file mode 100644
index 0000000..c30393e
--- /dev/null
+++ b/common/fs_loader.c
@@ -0,0 +1,320 @@
+/*
+ * Copyright (C) 2018 Intel Corporation <www.intel.com>
+ *
+ * SPDX-License-Identifier:    GPL-2.0
+ */
+
+#include <common.h>
+#include <errno.h>
+#include <fs.h>
+#include <fs_loader.h>
+#include <nand.h>
+#include <sata.h>
+#include <spi.h>
+#include <spi_flash.h>
+#ifdef CONFIG_SPL
+#include <spl.h>
+#endif
+#include <linux/string.h>
+#ifdef CONFIG_CMD_UBIFS
+#include <ubi_uboot.h>
+#include <ubifs_uboot.h>
+#endif
+#include <usb.h>
+
+struct firmware_priv {
+	const char *name;	/* Filename */
+	u32 offset;		/* Offset of reading a file */
+};
+
+static struct device_location default_locations[] = {
+	{
+		.name = "mmc",
+		.devpart = "0:1",
+	},
+	{
+		.name = "usb",
+		.devpart = "0:1",
+	},
+	{
+		.name = "sata",
+		.devpart = "0:1",
+	},
+};
+
+/* USB build is not supported yet in SPL */
+#ifndef CONFIG_SPL_BUILD
+#ifdef CONFIG_USB_STORAGE
+static int init_usb(void)
+{
+	int err;
+
+	err = usb_init();
+	if (err)
+		return err;
+
+#ifndef CONFIG_DM_USB
+	err = usb_stor_scan(1);
+	if (err)
+		return err;
+#endif
+
+	return err;
+}
+#else
+static int init_usb(void)
+{
+	debug("Error: Cannot load flash image: no USB support\n");
+	return -ENOSYS;
+}
+#endif
+#endif
+
+#ifdef CONFIG_SATA
+static int init_storage_sata(void)
+{
+	return sata_probe(0);
+}
+#else
+static int init_storage_sata(void)
+{
+	debug("Error: Cannot load image: no SATA support\n");
+	return -ENOSYS;
+}
+#endif
+
+#ifdef CONFIG_CMD_UBIFS
+static int mount_ubifs(struct device_location *location)
+{
+	int ret;
+	char cmd[32];
+
+	if (ret = ubi_part(location->mtdpart, NULL)) {
+		debug("Cannot find mtd partition %s\n", location->mtdpart);
+		return ret;
+	}
+
+	return cmd_ubifs_mount(location->ubivol);
+}
+
+static int umount_ubifs(void)
+{
+	return cmd_ubifs_umount();
+}
+#else
+static int mount_ubifs(struct device_location *location)
+{
+	debug("Error: Cannot load image: no UBIFS support\n");
+	return -ENOSYS;
+}
+#endif
+
+#if defined(CONFIG_SPL_MMC_SUPPORT) && defined(CONFIG_SPL_BUILD)
+__weak int init_mmc(void)
+{
+	/* Just for the case MMC is not yet initialized */
+	struct mmc *mmc = NULL;
+	int err;
+
+#ifdef CONFIG_SPL
+	spl_mmc_find_device(&mmc, spl_boot_device());
+#else
+	debug("#define CONFIG_SPL is required or overrriding %s\n",
+	      __func__);
+	return -ENOENT;
+#endif
+
+	err = mmc_init(mmc);
+	if (err) {
+		debug("spl: mmc init failed with error: %d\n", err);
+		return err;
+	}
+
+	return err;
+}
+#else
+__weak int init_mmc(void)
+{
+	/* Expect somewhere already initialize MMC */
+	return 0;
+}
+#endif
+
+static int select_fs_dev(struct device_location *location)
+{
+	int ret;
+
+	if (!strcmp("mmc", location->name)) {
+		ret = fs_set_blk_dev("mmc", location->devpart, FS_TYPE_ANY);
+	} else if (!strcmp("usb", location->name)) {
+		ret = fs_set_blk_dev("usb", location->devpart, FS_TYPE_ANY);
+	} else if (!strcmp("sata", location->name)) {
+		ret = fs_set_blk_dev("sata", location->devpart, FS_TYPE_ANY);
+	} else if (!strcmp("ubi", location->name)) {
+		if (location->ubivol)
+			ret = fs_set_blk_dev("ubi", NULL, FS_TYPE_UBIFS);
+		else
+			ret = -ENODEV;
+	} else {
+		debug("Error: unsupported location storage.\n");
+		return -ENODEV;
+	}
+
+	if (ret)
+		debug("Error: could not access storage.\n");
+
+	return ret;
+}
+
+static int init_storage_device(struct device_location *location)
+{
+	int ret;
+
+	if (!strcmp("mmc", location->name)) {
+		ret = init_mmc();
+	} else if (!strcmp("sata", location->name)) {
+		ret = init_storage_sata();
+	} else if (location->ubivol) {
+		ret = mount_ubifs(location);
+#ifndef CONFIG_SPL_BUILD
+	/* USB build is not supported yet in SPL */
+	} else if (!strcmp("usb", location->name)) {
+		ret = init_usb();
+#endif
+	} else {
+		debug("Error: no supported storage device is available.\n");
+		ret = -ENODEV;
+	}
+
+	return ret;
+}
+
+static void set_storage_devpart(char *name, char *devpart)
+{
+	size_t i;
+
+	for (i = 0; i < ARRAY_SIZE(default_locations); i++) {
+		if (!strcmp(default_locations[i].name, name))
+			default_locations[i].devpart = devpart;
+	}
+}
+
+/**
+ * _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.
+ * @location: An array of supported firmware location.
+ * @firmware_p: pointer to firmware image.
+ *
+ * Return: Size of total read, negative value when error.
+ */
+static int fw_get_filesystem_firmware(struct device_location *location,
+				      struct firmware *firmware_p)
+{
+	struct firmware_priv *fw_priv = NULL;
+	loff_t actread;
+	char *dev_part;
+	int ret;
+
+	dev_part = env_get("fw_dev_part");
+	if (dev_part)
+		set_storage_devpart(location->name, dev_part);
+
+	ret = init_storage_device(location);
+	if (ret)
+		goto out;
+
+	ret = select_fs_dev(location);
+	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:
+#ifdef CONFIG_CMD_UBIFS
+	if (location->ubivol)
+		umount_ubifs();
+#endif
+
+	return ret;
+}
+
+/**
+ * request_firmware_into_buf - Load firmware into a previously allocated buffer.
+ * @firmware_p: Pointer to firmware image.
+ * @name: Name of firmware file.
+ * @location: An array of supported firmware location.
+ * @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 firmware **firmware_p,
+			      const char *name,
+			      struct device_location *location,
+			      void *buf, size_t size, u32 offset)
+{
+	int ret;
+
+	ret = _request_firmware_prepare(firmware_p, name, buf, size, offset);
+	if (ret < 0) /* error */
+		return ret;
+
+	ret = fw_get_filesystem_firmware(location, *firmware_p);
+
+	return ret;
+}
diff --git a/doc/README.firmware_loader b/doc/README.firmware_loader
new file mode 100644
index 0000000..f6e4c6b
--- /dev/null
+++ b/doc/README.firmware_loader
@@ -0,0 +1,76 @@
+/*
+ * Copyright (C) 2017 Intel Corporation <www.intel.com>
+ *
+ * SPDX-License-Identifier:    GPL-2.0
+ */
+
+Introduction
+------------
+This is 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.
+
+Firmware loader can be used to load whatever(firmware, image, and binary) from
+the flash 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.
+
+Firmware Loader API core features
+---------------------------------
+=> Firmware storage device partition search
+   ----------------------------------------
+   =>	Default storage device partition search set for mmc, usb and sata
+	as shown in below:
+	static struct device_location default_locations[] = {
+		{
+			.name = "mmc",
+			.devpart = "0:1",
+		},
+		{
+			.name = "usb",
+			.devpart = "0:1",
+		},
+		{
+			.name = "sata",
+			.devpart = "0:1",
+		},
+	};
+
+	However, the default storage device .devpart value can be overwritten
+	with value which is defined in the environment variable "FW_DEV_PART".
+	For example: env_set("fw_dev_part", "0:2");
+
+Firmware Loader API
+-------------------
+=> int request_firmware_into_buf(struct firmware **firmware_p,
+			      const char *name,
+			      struct device_location *location,
+			      void *buf, size_t size, u32 offset)
+   --------------------------------------------------------------
+   =>	Load firmware into a previously allocated buffer
+
+	Parameters:
+	struct firmware **firmware_p
+		pointer to firmware image
+
+	const char *name
+		name of firmware file
+
+	struct device_location *location
+		an array of supported firmware location
+
+	void *buf
+		address of buffer to load firmware into
+
+	size_t size
+		size of buffer
+
+	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.
diff --git a/include/fs_loader.h b/include/fs_loader.h
new file mode 100644
index 0000000..60a6ba2
--- /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 <linux/types.h>
+
+struct firmware {
+	size_t size;		/* Size of a file */
+	const u8 *data;		/* Buffer for file */
+	void *priv;		/* Firmware loader private fields */
+};
+
+struct device_location {
+	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 firmware **firmware_p,
+			      const char *name,
+			      struct device_location *location,
+			      void *buf, size_t size, u32 offset);
+#endif
-- 
2.2.0

[U-Boot] [PATCH v8 0/4] Generic firmware loader

[Chee, Tien Fong]

On Mon, 2018-02-05 at 15:06 +0800, tien.fong.chee at intel.com wrote:
Hi,
> From: Tien Fong Chee <tien.fong.chee@intel.com>
> 
> This patchset contains generic firmware loader which is very close to
> Linux
> firmware loader but for U-Boot framework. Generic 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
> Marek Vasut in [v7].
> 
> This series is working on top of u-boot.git -
>  http://git.denx.de/u-boot.git .
> 
> [v7]: https://www.mail-archive.com/u-boot at lists.denx.de/msg276775.htm
> l
> 
> v7 -> v8 changes:
> -----------------
> - Return cmd_ubifs_umount() directly.
> - Call the UBI functions directly instead of invoking the U-Boot
> command parser.
> 
Any comment on this patchset?

> Patchset history
> ----------------
> [v1]: https://www.mail-archive.com/u-boot at lists.denx.de/msg271905.htm
> l
> [v2]: https://www.mail-archive.com/u-boot at lists.denx.de/msg271979.htm
> l
> [v3]: https://www.mail-archive.com/u-boot at lists.denx.de/msg272039.htm
> l
> [v4]: https://www.mail-archive.com/u-boot at lists.denx.de/msg272432.htm
> l
> [v5]: https://www.mail-archive.com/u-boot at lists.denx.de/msg272771.htm
> l
> [v6]: https://www.mail-archive.com/u-boot at lists.denx.de/msg273961.htm
> l
> 
> Tien Fong Chee (4):
>   spl: Remove static declaration on spl_mmc_find_device function
>   cmd: ubifs: Move ubifs_initialized checking into cmd_ubifs_umount()
>   cmd: ubifs: Factor out some checking codes into cmd_ubifs_mount()
>   common: Generic firmware loader for file system
> 
>  cmd/ubifs.c                |  40 +++---
>  common/Kconfig             |   9 ++
>  common/Makefile            |   1 +
>  common/fs_loader.c         | 320
> +++++++++++++++++++++++++++++++++++++++++++++
>  common/spl/spl_mmc.c       |   2 +-
>  doc/README.firmware_loader |  76 +++++++++++
>  include/fs_loader.h        |  28 ++++
>  include/spl.h              |   2 +
>  include/ubi_uboot.h        |   2 +
>  9 files changed, 462 insertions(+), 18 deletions(-)
>  create mode 100644 common/fs_loader.c
>  create mode 100644 doc/README.firmware_loader
>  create mode 100644 include/fs_loader.h
> 

[U-Boot] [PATCH v8 0/4] Generic firmware loader

[Chee, Tien Fong]

On Mon, 2018-02-05 at 15:06 +0800, tien.fong.chee at intel.com wrote:
Hi Tom,
> From: Tien Fong Chee <tien.fong.chee@intel.com>
> 
> This patchset contains generic firmware loader which is very close to
> Linux
> firmware loader but for U-Boot framework. Generic 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
> Marek Vasut in [v7].
> 
> This series is working on top of u-boot.git -
>  http://git.denx.de/u-boot.git .
> 
> [v7]: https://www.mail-archive.com/u-boot at lists.denx.de/msg276775.htm
> l
> 
> v7 -> v8 changes:
> -----------------
> - Return cmd_ubifs_umount() directly.
> - Call the UBI functions directly instead of invoking the U-Boot
> command parser.
> 
Any comment on this patchset?

> Patchset history
> ----------------
> [v1]: https://www.mail-archive.com/u-boot at lists.denx.de/msg271905.htm
> l
> [v2]: https://www.mail-archive.com/u-boot at lists.denx.de/msg271979.htm
> l
> [v3]: https://www.mail-archive.com/u-boot at lists.denx.de/msg272039.htm
> l
> [v4]: https://www.mail-archive.com/u-boot at lists.denx.de/msg272432.htm
> l
> [v5]: https://www.mail-archive.com/u-boot at lists.denx.de/msg272771.htm
> l
> [v6]: https://www.mail-archive.com/u-boot at lists.denx.de/msg273961.htm
> l
> 
> Tien Fong Chee (4):
>   spl: Remove static declaration on spl_mmc_find_device function
>   cmd: ubifs: Move ubifs_initialized checking into cmd_ubifs_umount()
>   cmd: ubifs: Factor out some checking codes into cmd_ubifs_mount()
>   common: Generic firmware loader for file system
> 
>  cmd/ubifs.c                |  40 +++---
>  common/Kconfig             |   9 ++
>  common/Makefile            |   1 +
>  common/fs_loader.c         | 320
> +++++++++++++++++++++++++++++++++++++++++++++
>  common/spl/spl_mmc.c       |   2 +-
>  doc/README.firmware_loader |  76 +++++++++++
>  include/fs_loader.h        |  28 ++++
>  include/spl.h              |   2 +
>  include/ubi_uboot.h        |   2 +
>  9 files changed, 462 insertions(+), 18 deletions(-)
>  create mode 100644 common/fs_loader.c
>  create mode 100644 doc/README.firmware_loader
>  create mode 100644 include/fs_loader.h
> 

[U-Boot] [PATCH v8 4/4] common: Generic firmware loader for file system

[Marek Vasut]

On 02/05/2018 08:06 AM, tien.fong.chee at intel.com wrote:
> From: Tien Fong Chee <tien.fong.chee@intel.com>
> 
> This is file system generic loader which can be used to load
> the file image from the storage into target such as memory.
> The consumer driver would then use this loader to program whatever,
> ie. the FPGA device.
> 
> Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
> Reviewed-by: Lothar Waßmann <LW@KARO-electronics.de>
[...]

> +#include <common.h>
> +#include <errno.h>
> +#include <fs.h>
> +#include <fs_loader.h>
> +#include <nand.h>
> +#include <sata.h>
> +#include <spi.h>
> +#include <spi_flash.h>
> +#ifdef CONFIG_SPL

Are the ifdefs needed ?

> +#include <spl.h>
> +#endif
> +#include <linux/string.h>
> +#ifdef CONFIG_CMD_UBIFS
> +#include <ubi_uboot.h>
> +#include <ubifs_uboot.h>
> +#endif
> +#include <usb.h>
> +
> +struct firmware_priv {
> +	const char *name;	/* Filename */
> +	u32 offset;		/* Offset of reading a file */
> +};
> +
> +static struct device_location default_locations[] = {
> +	{
> +		.name = "mmc",
> +		.devpart = "0:1",
> +	},
> +	{
> +		.name = "usb",
> +		.devpart = "0:1",
> +	},
> +	{
> +		.name = "sata",
> +		.devpart = "0:1",
> +	},

How can this load from UBI if it's not listed here ?

> +};
> +
> +/* USB build is not supported yet in SPL */
> +#ifndef CONFIG_SPL_BUILD
> +#ifdef CONFIG_USB_STORAGE
> +static int init_usb(void)
> +{
> +	int err;
> +
> +	err = usb_init();
> +	if (err)
> +		return err;
> +
> +#ifndef CONFIG_DM_USB
> +	err = usb_stor_scan(1);

mode = 0 I think, we don't need this verbose output.

> +	if (err)
> +		return err;
> +#endif
> +
> +	return err;
> +}
> +#else
> +static int init_usb(void)
> +{
> +	debug("Error: Cannot load flash image: no USB support\n");
> +	return -ENOSYS;
> +}
> +#endif
> +#endif
> +
> +#ifdef CONFIG_SATA
> +static int init_storage_sata(void)
> +{
> +	return sata_probe(0);

Shouldn't the sata device number be pulled from devpart in
default_locations table ?

> +}
> +#else
> +static int init_storage_sata(void)
> +{
> +	debug("Error: Cannot load image: no SATA support\n");
> +	return -ENOSYS;
> +}
> +#endif
> +
> +#ifdef CONFIG_CMD_UBIFS
> +static int mount_ubifs(struct device_location *location)
> +{
> +	int ret;
> +	char cmd[32];
> +
> +	if (ret = ubi_part(location->mtdpart, NULL)) {
> +		debug("Cannot find mtd partition %s\n", location->mtdpart);
> +		return ret;
> +	}
> +
> +	return cmd_ubifs_mount(location->ubivol);
> +}
> +
> +static int umount_ubifs(void)
> +{
> +	return cmd_ubifs_umount();
> +}
> +#else
> +static int mount_ubifs(struct device_location *location)
> +{
> +	debug("Error: Cannot load image: no UBIFS support\n");
> +	return -ENOSYS;
> +}
> +#endif
> +
> +#if defined(CONFIG_SPL_MMC_SUPPORT) && defined(CONFIG_SPL_BUILD)
> +__weak int init_mmc(void)
> +{
> +	/* Just for the case MMC is not yet initialized */
> +	struct mmc *mmc = NULL;
> +	int err;
> +
> +#ifdef CONFIG_SPL
> +	spl_mmc_find_device(&mmc, spl_boot_device());
> +#else
> +	debug("#define CONFIG_SPL is required or overrriding %s\n",
> +	      __func__);

This can be a compile-time error, maybe ?

> +	return -ENOENT;
> +#endif
> +
> +	err = mmc_init(mmc);
> +	if (err) {
> +		debug("spl: mmc init failed with error: %d\n", err);
> +		return err;
> +	}
> +
> +	return err;
> +}
> +#else
> +__weak int init_mmc(void)
> +{
> +	/* Expect somewhere already initialize MMC */
> +	return 0;
> +}
> +#endif

[...]

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v8 1/4] spl: Remove static declaration on spl_mmc_find_device function

[Marek Vasut]

On 02/05/2018 08:06 AM, tien.fong.chee at intel.com wrote:
> From: Tien Fong Chee <tien.fong.chee@intel.com>
> 
> This patch removes the static declation on spl_mmc_find_device_function
> so this function is accessible by the caller from other file. This patch
> is required for later patch.
> 
> Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
> Reviewed-by: Tom Rini <trini@konsulko.com>
> ---

Reviewed-by: Marek Vasut <marex@denx.de>

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v8 2/4] cmd: ubifs: Move ubifs_initialized checking into cmd_ubifs_umount()

[Marek Vasut]

On 02/05/2018 08:06 AM, tien.fong.chee at intel.com wrote:
> From: Tien Fong Chee <tien.fong.chee@intel.com>
> 
> cmd_ubifs_umount() function would be called directly instead of involving
> whole command machinery in generic firmware loader, so checking on
> ubifs_initialized status need to be done in cmd_ubifs_umount() without
> breaking original functionality design.
> 
> Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>64db5518baa2ea1a8a0e81cc485d760b850c7052

Reviewed-by: Marek Vasut <marex@denx.de>

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v8 3/4] cmd: ubifs: Factor out some checking codes into cmd_ubifs_mount()

[Marek Vasut]

On 02/05/2018 08:06 AM, tien.fong.chee at intel.com wrote:
> From: Tien Fong Chee <tien.fong.chee@intel.com>
> 
> cmd_ubifs_mount() function would be called directly instead of
> involving whole command machinery for mounting ubifs in
> generic firmware loader, so some checking codes need to be factored out
> into cmd_ubifs_mount() without breaking original functionality design.
> 
> Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>

[...]

> diff --git a/include/ubi_uboot.h b/include/ubi_uboot.h
> index 827dbfc..fb300e8 100644
> --- a/include/ubi_uboot.h
> +++ b/include/ubi_uboot.h
> @@ -76,5 +76,6 @@ extern int ubi_volume_read(char *volume, char *buf, size_t size);
>  
>  extern struct ubi_device *ubi_devices[];
>  int cmd_ubifs_umount(void);
> +int cmd_ubifs_mount(char *vol_name);

Swap these two -- mount first and umount seconds.

With that
Reviewed-by: Marek Vasut <marex@denx.de>

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v8 3/4] cmd: ubifs: Factor out some checking codes into cmd_ubifs_mount()

[Chee, Tien Fong]

On Thu, 2018-02-15 at 15:59 +0100, Marek Vasut wrote:
> On 02/05/2018 08:06 AM, tien.fong.chee at intel.com wrote:
> > 
> > From: Tien Fong Chee <tien.fong.chee@intel.com>
> > 
> > cmd_ubifs_mount() function would be called directly instead of
> > involving whole command machinery for mounting ubifs in
> > generic firmware loader, so some checking codes need to be factored
> > out
> > into cmd_ubifs_mount() without breaking original functionality
> > design.
> > 
> > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
> [...]
> 
> > 
> > diff --git a/include/ubi_uboot.h b/include/ubi_uboot.h
> > index 827dbfc..fb300e8 100644
> > --- a/include/ubi_uboot.h
> > +++ b/include/ubi_uboot.h
> > @@ -76,5 +76,6 @@ extern int ubi_volume_read(char *volume, char
> > *buf, size_t size);
> >  
> >  extern struct ubi_device *ubi_devices[];
> >  int cmd_ubifs_umount(void);
> > +int cmd_ubifs_mount(char *vol_name);
> Swap these two -- mount first and umount seconds.
> 
Okay. noted.

> With that
> Reviewed-by: Marek Vasut <marex@denx.de>
> 

[U-Boot] [PATCH v8 4/4] common: Generic firmware loader for file system

[Chee, Tien Fong]

On Thu, 2018-02-15 at 15:58 +0100, Marek Vasut wrote:
> On 02/05/2018 08:06 AM, tien.fong.chee at intel.com wrote:
> > 
> > From: Tien Fong Chee <tien.fong.chee@intel.com>
> > 
> > This is file system generic loader which can be used to load
> > the file image from the storage into target such as memory.
> > The consumer driver would then use this loader to program whatever,
> > ie. the FPGA device.
> > 
> > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
> > Reviewed-by: Lothar Waßmann <LW@KARO-electronics.de>
> [...]
> 
> > 
> > +#include <common.h>
> > +#include <errno.h>
> > +#include <fs.h>
> > +#include <fs_loader.h>
> > +#include <nand.h>
> > +#include <sata.h>
> > +#include <spi.h>
> > +#include <spi_flash.h>
> > +#ifdef CONFIG_SPL
> Are the ifdefs needed ?
> 
Because spl.h contains some codes have its dependency with SPL. So, Tom
adviced to make this part of code depend on CONFIG_SPL.
However, only __weak int init_mmc() depend on the codes from spl.h, so
user can override their own init_mmc() if SPL is not used.

> > 
> > +#include <spl.h>
> > +#endif
> > +#include <linux/string.h>
> > +#ifdef CONFIG_CMD_UBIFS
> > +#include <ubi_uboot.h>
> > +#include <ubifs_uboot.h>
> > +#endif
> > +#include <usb.h>
> > +
> > +struct firmware_priv {
> > +	const char *name;	/* Filename */
> > +	u32 offset;		/* Offset of reading a file */
> > +};
> > +
> > +static struct device_location default_locations[] = {
> > +	{
> > +		.name = "mmc",
> > +		.devpart = "0:1",
> > +	},
> > +	{
> > +		.name = "usb",
> > +		.devpart = "0:1",
> > +	},
> > +	{
> > +		.name = "sata",
> > +		.devpart = "0:1",
> > +	},
> How can this load from UBI if it's not listed here ?
> 
> > 
> > +};
> > +
> > +/* USB build is not supported yet in SPL */
> > +#ifndef CONFIG_SPL_BUILD
> > +#ifdef CONFIG_USB_STORAGE
> > +static int init_usb(void)
> > +{
> > +	int err;
> > +
> > +	err = usb_init();
> > +	if (err)
> > +		return err;
> > +
> > +#ifndef CONFIG_DM_USB
> > +	err = usb_stor_scan(1);
> mode = 0 I think, we don't need this verbose output.
> 
Okay so i will change to usb_stor_scan(0) .
> > 
> > +	if (err)
> > +		return err;
> > +#endif
> > +
> > +	return err;
> > +}
> > +#else
> > +static int init_usb(void)
> > +{
> > +	debug("Error: Cannot load flash image: no USB support\n");
> > +	return -ENOSYS;
> > +}
> > +#endif
> > +#endif
> > +
> > +#ifdef CONFIG_SATA
> > +static int init_storage_sata(void)
> > +{
> > +	return sata_probe(0);
> Shouldn't the sata device number be pulled from devpart in
> default_locations table ?
> 
This may need to add the logic for splitting the device number and
partition into integer from the string. Let me think how to do it, may
be i can leverage existing code.
> > 
> > +}
> > +#else
> > +static int init_storage_sata(void)
> > +{
> > +	debug("Error: Cannot load image: no SATA support\n");
> > +	return -ENOSYS;
> > +}
> > +#endif
> > +
> > +#ifdef CONFIG_CMD_UBIFS
> > +static int mount_ubifs(struct device_location *location)
> > +{
> > +	int ret;
> > +	char cmd[32];
> > +
> > +	if (ret = ubi_part(location->mtdpart, NULL)) {
> > +		debug("Cannot find mtd partition %s\n", location-
> > >mtdpart);
> > +		return ret;
> > +	}
> > +
> > +	return cmd_ubifs_mount(location->ubivol);
> > +}
> > +
> > +static int umount_ubifs(void)
> > +{
> > +	return cmd_ubifs_umount();
> > +}
> > +#else
> > +static int mount_ubifs(struct device_location *location)
> > +{
> > +	debug("Error: Cannot load image: no UBIFS support\n");
> > +	return -ENOSYS;
> > +}
> > +#endif
> > +
> > +#if defined(CONFIG_SPL_MMC_SUPPORT) && defined(CONFIG_SPL_BUILD)
> > +__weak int init_mmc(void)
> > +{
> > +	/* Just for the case MMC is not yet initialized */
> > +	struct mmc *mmc = NULL;
> > +	int err;
> > +
> > +#ifdef CONFIG_SPL
> > +	spl_mmc_find_device(&mmc, spl_boot_device());
> > +#else
> > +	debug("#define CONFIG_SPL is required or overrriding
> > %s\n",
> > +	      __func__);
> This can be a compile-time error, maybe ?
> 
No compile error. When you open the file, "%s\n" is actually same line
with debug("..... .

> > 
> > +	return -ENOENT;
> > +#endif
> > +
> > +	err = mmc_init(mmc);
> > +	if (err) {
> > +		debug("spl: mmc init failed with error: %d\n",
> > err);
> > +		return err;
> > +	}
> > +
> > +	return err;
> > +}
> > +#else
> > +__weak int init_mmc(void)
> > +{
> > +	/* Expect somewhere already initialize MMC */
> > +	return 0;
> > +}
> > +#endif
> [...]
> 

[U-Boot] [PATCH v8 4/4] common: Generic firmware loader for file system

[Lothar Waßmann]

Hi,

On Mon,  5 Feb 2018 15:06:49 +0800 tien.fong.chee at intel.com wrote:
> From: Tien Fong Chee <tien.fong.chee@intel.com>
> 
> This is file system generic loader which can be used to load
> the file image from the storage into target such as memory.
> The consumer driver would then use this loader to program whatever,
> ie. the FPGA device.
> 
> Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
> Reviewed-by: Lothar Waßmann <LW@KARO-electronics.de>
> ---
>  common/Kconfig             |   9 ++
>  common/Makefile            |   1 +
>  common/fs_loader.c         | 320 +++++++++++++++++++++++++++++++++++++++++++++
>  doc/README.firmware_loader |  76 +++++++++++
>  include/fs_loader.h        |  28 ++++
>  5 files changed, 434 insertions(+)
>  create mode 100644 common/fs_loader.c
>  create mode 100644 doc/README.firmware_loader
>  create mode 100644 include/fs_loader.h
> 
[...]
> +#if defined(CONFIG_SPL_MMC_SUPPORT) && defined(CONFIG_SPL_BUILD)
> +__weak int init_mmc(void)
> +{
> +	/* Just for the case MMC is not yet initialized */
> +	struct mmc *mmc = NULL;
> +	int err;
> +
> +#ifdef CONFIG_SPL
> +	spl_mmc_find_device(&mmc, spl_boot_device());
> +#else
> +	debug("#define CONFIG_SPL is required or overrriding %s\n",
>
s/overrr/overr/



Lothar Waßmann

[U-Boot] [PATCH v8 4/4] common: Generic firmware loader for file system

[Marek Vasut]

On 02/22/2018 09:18 AM, Chee, Tien Fong wrote:
> On Thu, 2018-02-15 at 15:58 +0100, Marek Vasut wrote:
>> On 02/05/2018 08:06 AM, tien.fong.chee at intel.com wrote:
>>>
>>> From: Tien Fong Chee <tien.fong.chee@intel.com>
>>>
>>> This is file system generic loader which can be used to load
>>> the file image from the storage into target such as memory.
>>> The consumer driver would then use this loader to program whatever,
>>> ie. the FPGA device.
>>>
>>> Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
>>> Reviewed-by: Lothar Waßmann <LW@KARO-electronics.de>
>> [...]
>>
>>>
>>> +#include <common.h>
>>> +#include <errno.h>
>>> +#include <fs.h>
>>> +#include <fs_loader.h>
>>> +#include <nand.h>
>>> +#include <sata.h>
>>> +#include <spi.h>
>>> +#include <spi_flash.h>
>>> +#ifdef CONFIG_SPL
>> Are the ifdefs needed ?
>>
> Because spl.h contains some codes have its dependency with SPL. So, Tom
> adviced to make this part of code depend on CONFIG_SPL.
> However, only __weak int init_mmc() depend on the codes from spl.h, so
> user can override their own init_mmc() if SPL is not used.

You probably dont need those ifdefs around headers.

>>> +#include <spl.h>
>>> +#endif
>>> +#include <linux/string.h>
>>> +#ifdef CONFIG_CMD_UBIFS
>>> +#include <ubi_uboot.h>
>>> +#include <ubifs_uboot.h>
>>> +#endif
>>> +#include <usb.h>
>>> +
>>> +struct firmware_priv {
>>> +	const char *name;	/* Filename */
>>> +	u32 offset;		/* Offset of reading a file */
>>> +};
>>> +
>>> +static struct device_location default_locations[] = {
>>> +	{
>>> +		.name = "mmc",
>>> +		.devpart = "0:1",
>>> +	},
>>> +	{
>>> +		.name = "usb",
>>> +		.devpart = "0:1",
>>> +	},
>>> +	{
>>> +		.name = "sata",
>>> +		.devpart = "0:1",
>>> +	},
>> How can this load from UBI if it's not listed here ?

Well ?

[...]

>>> +#ifdef CONFIG_SATA
>>> +static int init_storage_sata(void)
>>> +{
>>> +	return sata_probe(0);
>> Shouldn't the sata device number be pulled from devpart in
>> default_locations table ?
>>
> This may need to add the logic for splitting the device number and
> partition into integer from the string. Let me think how to do it, may
> be i can leverage existing code.

strtok(), strtol() or something ?

[...]

>>> +#ifdef CONFIG_SPL
>>> +	spl_mmc_find_device(&mmc, spl_boot_device());
>>> +#else
>>> +	debug("#define CONFIG_SPL is required or overrriding
>>> %s\n",
>>> +	      __func__);
>> This can be a compile-time error, maybe ?
>>
> No compile error. When you open the file, "%s\n" is actually same line
> with debug("..... .

So what ? This missing macro can be detected at runtime, heck this code
can even depend on CONFIG_SPL in Kconfig.

>>>
>>> +	return -ENOENT;
>>> +#endif
>>> +
>>> +	err = mmc_init(mmc);
>>> +	if (err) {
>>> +		debug("spl: mmc init failed with error: %d\n",
>>> err);
>>> +		return err;
>>> +	}
>>> +
>>> +	return err;
>>> +}
>>> +#else
>>> +__weak int init_mmc(void)
>>> +{
>>> +	/* Expect somewhere already initialize MMC */
>>> +	return 0;
>>> +}
>>> +#endif
>> [...]


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v8 4/4] common: Generic firmware loader for file system

[Tom Rini]

On Thu, Feb 22, 2018 at 03:28:12PM +0100, Marek Vasut wrote:
> On 02/22/2018 09:18 AM, Chee, Tien Fong wrote:
> > On Thu, 2018-02-15 at 15:58 +0100, Marek Vasut wrote:
> >> On 02/05/2018 08:06 AM, tien.fong.chee at intel.com wrote:
> >>>
> >>> From: Tien Fong Chee <tien.fong.chee@intel.com>
> >>>
> >>> This is file system generic loader which can be used to load
> >>> the file image from the storage into target such as memory.
> >>> The consumer driver would then use this loader to program whatever,
> >>> ie. the FPGA device.
> >>>
> >>> Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
> >>> Reviewed-by: Lothar Waßmann <LW@KARO-electronics.de>
> >> [...]
> >>
> >>>
> >>> +#include <common.h>
> >>> +#include <errno.h>
> >>> +#include <fs.h>
> >>> +#include <fs_loader.h>
> >>> +#include <nand.h>
> >>> +#include <sata.h>
> >>> +#include <spi.h>
> >>> +#include <spi_flash.h>
> >>> +#ifdef CONFIG_SPL
> >> Are the ifdefs needed ?
> >>
> > Because spl.h contains some codes have its dependency with SPL. So, Tom
> > adviced to make this part of code depend on CONFIG_SPL.
> > However, only __weak int init_mmc() depend on the codes from spl.h, so
> > user can override their own init_mmc() if SPL is not used.
> 
> You probably dont need those ifdefs around headers.

In this case, we do.  You can only include <spl.h> on architectures
which have SPL support.

I wouldn't object to a separate patch series that adds a dummy
asm-generic/spl.h and we go that route, if it also cleans up more of the
code in general.  But I think that's separate from this series.  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180222/388023c5/attachment.sig>

[U-Boot] [PATCH v8 4/4] common: Generic firmware loader for file system

[Chee, Tien Fong]

On Thu, 2018-02-22 at 15:28 +0100, Marek Vasut wrote:
> On 02/22/2018 09:18 AM, Chee, Tien Fong wrote:
> > 
> > On Thu, 2018-02-15 at 15:58 +0100, Marek Vasut wrote:
> > > 
> > > On 02/05/2018 08:06 AM, tien.fong.chee at intel.com wrote:
> > > > 
> > > > 
> > > > From: Tien Fong Chee <tien.fong.chee@intel.com>
> > > > 
> > > > This is file system generic loader which can be used to load
> > > > the file image from the storage into target such as memory.
> > > > The consumer driver would then use this loader to program
> > > > whatever,
> > > > ie. the FPGA device.
> > > > 
> > > > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
> > > > Reviewed-by: Lothar Waßmann <LW@KARO-electronics.de>
> > > [...]
> > > 
> > > > 
> > > > 
> > > > +#include <common.h>
> > > > +#include <errno.h>
> > > > +#include <fs.h>
> > > > +#include <fs_loader.h>
> > > > +#include <nand.h>
> > > > +#include <sata.h>
> > > > +#include <spi.h>
> > > > +#include <spi_flash.h>
> > > > +#ifdef CONFIG_SPL
> > > Are the ifdefs needed ?
> > > 
> > Because spl.h contains some codes have its dependency with SPL. So,
> > Tom
> > adviced to make this part of code depend on CONFIG_SPL.
> > However, only __weak int init_mmc() depend on the codes from spl.h,
> > so
> > user can override their own init_mmc() if SPL is not used.
> You probably dont need those ifdefs around headers.
> 
> > 
> > > 
> > > > 
> > > > +#include <spl.h>
> > > > +#endif
> > > > +#include <linux/string.h>
> > > > +#ifdef CONFIG_CMD_UBIFS
> > > > +#include <ubi_uboot.h>
> > > > +#include <ubifs_uboot.h>
> > > > +#endif
> > > > +#include <usb.h>
> > > > +
> > > > +struct firmware_priv {
> > > > +	const char *name;	/* Filename */
> > > > +	u32 offset;		/* Offset of reading a file
> > > > */
> > > > +};
> > > > +
> > > > +static struct device_location default_locations[] = {
> > > > +	{
> > > > +		.name = "mmc",
> > > > +		.devpart = "0:1",
> > > > +	},
> > > > +	{
> > > > +		.name = "usb",
> > > > +		.devpart = "0:1",
> > > > +	},
> > > > +	{
> > > > +		.name = "sata",
> > > > +		.devpart = "0:1",
> > > > +	},
> > > How can this load from UBI if it's not listed here ?
> Well ?
> 
I can add ".name = ubi" and ".devpart = UBI" .
> [...]
> 
> > 
> > > 
> > > > 
> > > > +#ifdef CONFIG_SATA
> > > > +static int init_storage_sata(void)
> > > > +{
> > > > +	return sata_probe(0);
> > > Shouldn't the sata device number be pulled from devpart in
> > > default_locations table ?
> > > 
> > This may need to add the logic for splitting the device number and
> > partition into integer from the string. Let me think how to do it,
> > may
> > be i can leverage existing code.
> strtok(), strtol() or something ?
> 
yes, plan to use that.
> [...]
> 
> > 
> > > 
> > > > 
> > > > +#ifdef CONFIG_SPL
> > > > +	spl_mmc_find_device(&mmc, spl_boot_device());
> > > > +#else
> > > > +	debug("#define CONFIG_SPL is required or overrriding
> > > > %s\n",
> > > > +	      __func__);
> > > This can be a compile-time error, maybe ?
> > > 
> > No compile error. When you open the file, "%s\n" is actually same
> > line
> > with debug("..... .
> So what ? This missing macro can be detected at runtime, heck this
> code
> can even depend on CONFIG_SPL in Kconfig.
> 
Oh...I got you, i thought you means syntax error. So, i would remove it
and adding the depend on CONFIG_SPL in Kconfig.
> > 
> > > 
> > > > 
> > > > 
> > > > +	return -ENOENT;
> > > > +#endif
> > > > +
> > > > +	err = mmc_init(mmc);
> > > > +	if (err) {
> > > > +		debug("spl: mmc init failed with error: %d\n",
> > > > err);
> > > > +		return err;
> > > > +	}
> > > > +
> > > > +	return err;
> > > > +}
> > > > +#else
> > > > +__weak int init_mmc(void)
> > > > +{
> > > > +	/* Expect somewhere already initialize MMC */
> > > > +	return 0;
> > > > +}
> > > > +#endif
> > > [...]
> 

[U-Boot] [PATCH v8 4/4] common: Generic firmware loader for file system

[Chee, Tien Fong]

On Thu, 2018-02-22 at 10:50 -0500, Tom Rini wrote:
> On Thu, Feb 22, 2018 at 03:28:12PM +0100, Marek Vasut wrote:
> > 
> > On 02/22/2018 09:18 AM, Chee, Tien Fong wrote:
> > > 
> > > On Thu, 2018-02-15 at 15:58 +0100, Marek Vasut wrote:
> > > > 
> > > > On 02/05/2018 08:06 AM, tien.fong.chee at intel.com wrote:
> > > > > 
> > > > > 
> > > > > From: Tien Fong Chee <tien.fong.chee@intel.com>
> > > > > 
> > > > > This is file system generic loader which can be used to load
> > > > > the file image from the storage into target such as memory.
> > > > > The consumer driver would then use this loader to program
> > > > > whatever,
> > > > > ie. the FPGA device.
> > > > > 
> > > > > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
> > > > > Reviewed-by: Lothar Waßmann <LW@KARO-electronics.de>
> > > > [...]
> > > > 
> > > > > 
> > > > > 
> > > > > +#include <common.h>
> > > > > +#include <errno.h>
> > > > > +#include <fs.h>
> > > > > +#include <fs_loader.h>
> > > > > +#include <nand.h>
> > > > > +#include <sata.h>
> > > > > +#include <spi.h>
> > > > > +#include <spi_flash.h>
> > > > > +#ifdef CONFIG_SPL
> > > > Are the ifdefs needed ?
> > > > 
> > > Because spl.h contains some codes have its dependency with SPL.
> > > So, Tom
> > > adviced to make this part of code depend on CONFIG_SPL.
> > > However, only __weak int init_mmc() depend on the codes from
> > > spl.h, so
> > > user can override their own init_mmc() if SPL is not used.
> > You probably dont need those ifdefs around headers.
> In this case, we do.  You can only include <spl.h> on architectures
> which have SPL support.
> 
> I wouldn't object to a separate patch series that adds a dummy
> asm-generic/spl.h and we go that route, if it also cleans up more of
> the
> code in general.  But I think that's separate from this
> series.  Thanks!
> 
Planning to add the depend on CONFIG_SPL in Kconfig instead of ifdefs .

[U-Boot] [PATCH v8 4/4] common: Generic firmware loader for file system

[Chee, Tien Fong]

On Thu, 2018-02-22 at 10:02 +0100, Lothar Waßmann wrote:
> Hi,
> 
> On Mon,  5 Feb 2018 15:06:49 +0800 tien.fong.chee at intel.com wrote:
> > 
> > From: Tien Fong Chee <tien.fong.chee@intel.com>
> > 
> > This is file system generic loader which can be used to load
> > the file image from the storage into target such as memory.
> > The consumer driver would then use this loader to program whatever,
> > ie. the FPGA device.
> > 
> > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
> > Reviewed-by: Lothar Waßmann <LW@KARO-electronics.de>
> > ---
> >  common/Kconfig             |   9 ++
> >  common/Makefile            |   1 +
> >  common/fs_loader.c         | 320
> > +++++++++++++++++++++++++++++++++++++++++++++
> >  doc/README.firmware_loader |  76 +++++++++++
> >  include/fs_loader.h        |  28 ++++
> >  5 files changed, 434 insertions(+)
> >  create mode 100644 common/fs_loader.c
> >  create mode 100644 doc/README.firmware_loader
> >  create mode 100644 include/fs_loader.h
> > 
> [...]
> > 
> > +#if defined(CONFIG_SPL_MMC_SUPPORT) && defined(CONFIG_SPL_BUILD)
> > +__weak int init_mmc(void)
> > +{
> > +	/* Just for the case MMC is not yet initialized */
> > +	struct mmc *mmc = NULL;
> > +	int err;
> > +
> > +#ifdef CONFIG_SPL
> > +	spl_mmc_find_device(&mmc, spl_boot_device());
> > +#else
> > +	debug("#define CONFIG_SPL is required or overrriding
> > %s\n",
> > 
> s/overrr/overr/
> 
I would remove it and adding the depend on CONFIG_SPL in Kconfig for
compile-time error checking.
> 
> 
> Lothar Waßmann