[U-Boot] [PATCH v6 0/2] Generic firmware loader

[U-Boot] [PATCH v6 0/2] 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
Lothar Waßmann in [v5].

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

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

v5 -> v6 changes:
-----------------
- Return error code when fs_read is fail.
- Return actual read size when fs_read is success.

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

Tien Fong Chee (2):
  spl: Remove static declaration on spl_mmc_find_device function
  common: Generic firmware loader for file system

 common/Makefile            |   1 +
 common/fs_loader.c         | 309 +++++++++++++++++++++++++++++++++++++++++++++
 common/spl/spl_mmc.c       |   2 +-
 doc/README.firmware_loader |  76 +++++++++++
 include/fs_loader.h        |  28 ++++
 include/spl.h              |   2 +
 6 files changed, 417 insertions(+), 1 deletion(-)
 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 v6 1/2] 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>
---
 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 308ce7b..912983a 100644
--- a/include/spl.h
+++ b/include/spl.h
@@ -10,6 +10,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
@@ -72,6 +73,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] [U-Boot, v6, 1/2] spl: Remove static declaration on spl_mmc_find_device function

[Tom Rini]

On Wed, Dec 27, 2017 at 01:04:37PM +0800, 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>

-- 
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/20180115/e763b240/attachment.sig>

[U-Boot] [PATCH v6 2/2] 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>
---
 common/Makefile            |   1 +
 common/fs_loader.c         | 309 +++++++++++++++++++++++++++++++++++++++++++++
 doc/README.firmware_loader |  76 +++++++++++
 include/fs_loader.h        |  28 ++++
 4 files changed, 414 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/Makefile b/common/Makefile
index cec506f..2934221 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o
 obj-y += command.o
 obj-y += s_record.o
 obj-y += xyzModem.o
+obj-y += fs_loader.o
diff --git a/common/fs_loader.c b/common/fs_loader.c
new file mode 100644
index 0000000..56d29b6
--- /dev/null
+++ b/common/fs_loader.c
@@ -0,0 +1,309 @@
+/*
+ * Copyright (C) 2017 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>
+#include <spl.h>
+#include <linux/string.h>
+#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) < 0 ? -ENODEV : 0;
+#endif
+
+	return err;
+}
+#else
+static int init_usb(void)
+{
+	printf("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)
+{
+	printf("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];
+
+	sprintf(cmd, "ubi part %s", location->mtdpart);
+
+	ret = run_command(cmd, 0);
+	if (ret)
+		return ret;
+
+	sprintf(cmd, "ubifsmount %s", location->ubivol);
+
+	ret = run_command(cmd, 0);
+
+	return ret;
+}
+
+static int umount_ubifs(void)
+{
+	return run_command("ubifsumount", 0);
+}
+#else
+static int mount_ubifs(struct device_location *location)
+{
+	printf("Error: Cannot load image: no UBIFS support\n");
+	return -ENOSYS;
+}
+#endif
+
+#if defined(CONFIG_SPL_MMC_SUPPORT) && defined(CONFIG_SPL_BUILD)
+static int init_mmc(void)
+{
+	/* Just for the case MMC is not yet initialized */
+	struct mmc *mmc = NULL;
+	int err;
+
+	spl_mmc_find_device(&mmc, spl_boot_device());
+
+	err = mmc_init(mmc);
+	if (err) {
+		printf("spl: mmc init failed with error: %d\n", err);
+		return err;
+	}
+
+	return err;
+}
+#else
+static 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 != NULL)
+			ret = fs_set_blk_dev("ubi", NULL, FS_TYPE_UBIFS);
+		else
+			ret = -ENODEV;
+	} else {
+		printf("Error: unsupported location storage.\n");
+		return -ENODEV;
+	}
+
+	if (ret)
+		printf("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 != NULL) {
+		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 {
+		printf("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;
+	}
+}
+
+/*
+ * Prepare firmware struct;
+ * return -ve if fail.
+ */
+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) {
+		printf("%s: calloc(struct firmware) failed\n", __func__);
+		return -ENOMEM;
+	}
+
+	fw_priv = calloc(1, sizeof(*fw_priv));
+	if (!fw_priv) {
+		printf("%s: calloc(struct fw_priv) failed\n", __func__);
+		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
+ *	    -ve 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) {
+		printf("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 != NULL)
+		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
+ *	    -ve 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..83a8b80
--- /dev/null
+++ b/include/fs_loader.h
@@ -0,0 +1,28 @@
+/*
+ * Copyright (C) 2017 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] [U-Boot, v6, 2/2] common: Generic firmware loader for file system

[Tom Rini]

On Wed, Dec 27, 2017 at 01:04:38PM +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>

Please add Lothar's Reviewed-by for v7.  There's a number of minor
checkpatch.pl issues that checkpatch.pl can in turn fixup itself, please
correct them.

[snip]
> diff --git a/common/Makefile b/common/Makefile
> index cec506f..2934221 100644
> --- a/common/Makefile
> +++ b/common/Makefile
> @@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o
>  obj-y += command.o
>  obj-y += s_record.o
>  obj-y += xyzModem.o
> +obj-y += fs_loader.o

This needs a new Kconfig option and not to be enabled globally, only
when needed.

> diff --git a/common/fs_loader.c b/common/fs_loader.c
> new file mode 100644
> index 0000000..56d29b6
> --- /dev/null
> +++ b/common/fs_loader.c
> @@ -0,0 +1,309 @@
> +/*
> + * Copyright (C) 2017 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>
> +#include <spl.h>

This wants <asm/spl.h> which is not globally available, so you need to
come up with something here.  At least making this Kconfig-enabled will
be a start and perhaps OK for now.

[snip]
> +	if (ret) {
> +		printf("Error: %d Failed to read %s from flash %lld != %d.\n",
> +		      ret, fw_priv->name, actread, firmware_p->size);

The last %d needs to be %zu since it's a size_t, for portability.

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/20180115/306c2eb2/attachment.sig>

[U-Boot] [U-Boot, v6, 2/2] common: Generic firmware loader for file system

[Chee, Tien Fong]

On Mon, 2018-01-15 at 11:36 -0500, Tom Rini wrote:
> On Wed, Dec 27, 2017 at 01:04:38PM +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>
> Please add Lothar's Reviewed-by for v7.  There's a number of minor
> checkpatch.pl issues that checkpatch.pl can in turn fixup itself,
> please
> correct them.
> 
I have ran the checkpatch.pl on this patch, i didn't see any error.

> [snip]
> > 
> > diff --git a/common/Makefile b/common/Makefile
> > index cec506f..2934221 100644
> > --- a/common/Makefile
> > +++ b/common/Makefile
> > @@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o
> >  obj-y += command.o
> >  obj-y += s_record.o
> >  obj-y += xyzModem.o
> > +obj-y += fs_loader.o
> This needs a new Kconfig option and not to be enabled globally, only
> when needed.
> 
Okay.
> > 
> > diff --git a/common/fs_loader.c b/common/fs_loader.c
> > new file mode 100644
> > index 0000000..56d29b6
> > --- /dev/null
> > +++ b/common/fs_loader.c
> > @@ -0,0 +1,309 @@
> > +/*
> > + * Copyright (C) 2017 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>
> > +#include <spl.h>
> This wants <asm/spl.h> which is not globally available, so you need
> to
> come up with something here.  At least making this Kconfig-enabled
> will
> be a start and perhaps OK for now.
> 
I can enable the Kconfig, and put the caution about dependency on
<asm/spl.h> in document.
> [snip]
> > 
> > +	if (ret) {
> > +		printf("Error: %d Failed to read %s from flash
> > %lld != %d.\n",
> > +		      ret, fw_priv->name, actread, firmware_p-
> > >size);
> The last %d needs to be %zu since it's a size_t, for portability.
> 
> Thanks!
> 

[U-Boot] [U-Boot, v6, 2/2] common: Generic firmware loader for file system

[Tom Rini]

On Tue, Jan 16, 2018 at 07:58:00AM +0000, Chee, Tien Fong wrote:
> On Mon, 2018-01-15 at 11:36 -0500, Tom Rini wrote:
> > On Wed, Dec 27, 2017 at 01:04:38PM +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>
> > Please add Lothar's Reviewed-by for v7.  There's a number of minor
> > checkpatch.pl issues that checkpatch.pl can in turn fixup itself,
> > please
> > correct them.
> > 
> I have ran the checkpatch.pl on this patch, i didn't see any error.

When I ran it, it was pointing out cases where you have:
if (foo->bar != NULL)
when you can just use:
if (foo->bar)

[snip]
> > > diff --git a/common/fs_loader.c b/common/fs_loader.c
> > > new file mode 100644
> > > index 0000000..56d29b6
> > > --- /dev/null
> > > +++ b/common/fs_loader.c
> > > @@ -0,0 +1,309 @@
> > > +/*
> > > + * Copyright (C) 2017 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>
> > > +#include <spl.h>
> > This wants <asm/spl.h> which is not globally available, so you need
> > to
> > come up with something here.  At least making this Kconfig-enabled
> > will
> > be a start and perhaps OK for now.
> > 
> I can enable the Kconfig, and put the caution about dependency on
> <asm/spl.h> in document.

Well, you need to make that part of the code depend on CONFIG_SPL as SPL
support requires <asm/spl.h> to exist.   Perhaps part of the code needs
to be refactored to more easily deal with SPL not always being present?

-- 
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/20180116/bde7fd01/attachment.sig>

[U-Boot] [U-Boot, v6, 2/2] common: Generic firmware loader for file system

[Chee, Tien Fong]

On Tue, 2018-01-16 at 09:35 -0500, Tom Rini wrote:
> On Tue, Jan 16, 2018 at 07:58:00AM +0000, Chee, Tien Fong wrote:
> > 
> > On Mon, 2018-01-15 at 11:36 -0500, Tom Rini wrote:
> > > 
> > > On Wed, Dec 27, 2017 at 01:04:38PM +0800, tien.fong.chee at intel.co
> > > m
> > > 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>
> > > Please add Lothar's Reviewed-by for v7.  There's a number of
> > > minor
> > > checkpatch.pl issues that checkpatch.pl can in turn fixup itself,
> > > please
> > > correct them.
> > > 
> > I have ran the checkpatch.pl on this patch, i didn't see any error.
> When I ran it, it was pointing out cases where you have:
> if (foo->bar != NULL)
> when you can just use:
> if (foo->bar)
> 
It's weird for checkpatch.pl not showing the same. I would fix the code
with above example.
> [snip]
> > 
> > > 
> > > > 
> > > > diff --git a/common/fs_loader.c b/common/fs_loader.c
> > > > new file mode 100644
> > > > index 0000000..56d29b6
> > > > --- /dev/null
> > > > +++ b/common/fs_loader.c
> > > > @@ -0,0 +1,309 @@
> > > > +/*
> > > > + * Copyright (C) 2017 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>
> > > > +#include <spl.h>
> > > This wants <asm/spl.h> which is not globally available, so you
> > > need
> > > to
> > > come up with something here.  At least making this Kconfig-
> > > enabled
> > > will
> > > be a start and perhaps OK for now.
> > > 
> > I can enable the Kconfig, and put the caution about dependency on
> > <asm/spl.h> in document.
> Well, you need to make that part of the code depend on CONFIG_SPL as
> SPL
> support requires <asm/spl.h> to exist.   Perhaps part of the code
> needs
> to be refactored to more easily deal with SPL not always being
> present?
> 
How about i just move the whole enum { } from line 17 to line 35 in
asm/spl.h to fs.h, and then enum{} above replaced with #include <fs.h>?

asm/spl.h
----------
#if defined(CONFIG_ARCH_OMAP2PLUS) \
	|| defined(CONFIG_EXYNOS4) || defined(CONFIG_EXYNOS5) \
	|| defined(CONFIG_EXYNOS4210)
/* Platform-specific defines */
#include <asm/arch/spl.h>

#else
#include <fs.h>  <-- replacement
#endif
[...]

[U-Boot] [U-Boot, v6, 2/2] common: Generic firmware loader for file system

[Tom Rini]

On Thu, Jan 18, 2018 at 03:42:14AM +0000, Chee, Tien Fong wrote:
> On Tue, 2018-01-16 at 09:35 -0500, Tom Rini wrote:
> > On Tue, Jan 16, 2018 at 07:58:00AM +0000, Chee, Tien Fong wrote:
> > > 
> > > On Mon, 2018-01-15 at 11:36 -0500, Tom Rini wrote:
> > > > 
> > > > On Wed, Dec 27, 2017 at 01:04:38PM +0800, tien.fong.chee at intel.co
> > > > m
> > > > 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>
> > > > Please add Lothar's Reviewed-by for v7.  There's a number of
> > > > minor
> > > > checkpatch.pl issues that checkpatch.pl can in turn fixup itself,
> > > > please
> > > > correct them.
> > > > 
> > > I have ran the checkpatch.pl on this patch, i didn't see any error.
> > When I ran it, it was pointing out cases where you have:
> > if (foo->bar != NULL)
> > when you can just use:
> > if (foo->bar)
> > 
> It's weird for checkpatch.pl not showing the same. I would fix the code
> with above example.

That is odd.  Are you running it from within the U-Boot tree?

> > [snip]
> > > 
> > > > 
> > > > > 
> > > > > diff --git a/common/fs_loader.c b/common/fs_loader.c
> > > > > new file mode 100644
> > > > > index 0000000..56d29b6
> > > > > --- /dev/null
> > > > > +++ b/common/fs_loader.c
> > > > > @@ -0,0 +1,309 @@
> > > > > +/*
> > > > > + * Copyright (C) 2017 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>
> > > > > +#include <spl.h>
> > > > This wants <asm/spl.h> which is not globally available, so you
> > > > need
> > > > to
> > > > come up with something here.  At least making this Kconfig-
> > > > enabled
> > > > will
> > > > be a start and perhaps OK for now.
> > > > 
> > > I can enable the Kconfig, and put the caution about dependency on
> > > <asm/spl.h> in document.
> > Well, you need to make that part of the code depend on CONFIG_SPL as
> > SPL
> > support requires <asm/spl.h> to exist.   Perhaps part of the code
> > needs
> > to be refactored to more easily deal with SPL not always being
> > present?
> > 
> How about i just move the whole enum { } from line 17 to line 35 in
> asm/spl.h to fs.h, and then enum{} above replaced with #include <fs.h>?
> 
> asm/spl.h
> ----------
> #if defined(CONFIG_ARCH_OMAP2PLUS) \
> 	|| defined(CONFIG_EXYNOS4) || defined(CONFIG_EXYNOS5) \
> 	|| defined(CONFIG_EXYNOS4210)
> /* Platform-specific defines */
> #include <asm/arch/spl.h>
> 
> #else
> #include <fs.h>  <-- replacement
> #endif
> [...]

No, because that's still arch specific code and will blow up some
platforms that do not have asm/spl.h at all and is still non-portable
(look at the various asm/spl.h files that do exist for how
BOOT_DEVICE_xxx is handled in different places).

It's OK to restructure your code to have:
#ifdef CONFIG_SPL
#include <spl.h>
... SPL specific functions
#endif

-- 
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/20180118/0ff5993c/attachment.sig>

[U-Boot] [U-Boot, v6, 2/2] common: Generic firmware loader for file system

[Chee, Tien Fong]

On Thu, 2018-01-18 at 08:18 -0500, Tom Rini wrote:
> On Thu, Jan 18, 2018 at 03:42:14AM +0000, Chee, Tien Fong wrote:
> > 
> > On Tue, 2018-01-16 at 09:35 -0500, Tom Rini wrote:
> > > 
> > > On Tue, Jan 16, 2018 at 07:58:00AM +0000, Chee, Tien Fong wrote:
> > > > 
> > > > 
> > > > On Mon, 2018-01-15 at 11:36 -0500, Tom Rini wrote:
> > > > > 
> > > > > 
> > > > > On Wed, Dec 27, 2017 at 01:04:38PM +0800, tien.fong.chee at inte
> > > > > l.co
> > > > > m
> > > > > 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>
> > > > > Please add Lothar's Reviewed-by for v7.  There's a number of
> > > > > minor
> > > > > checkpatch.pl issues that checkpatch.pl can in turn fixup
> > > > > itself,
> > > > > please
> > > > > correct them.
> > > > > 
> > > > I have ran the checkpatch.pl on this patch, i didn't see any
> > > > error.
> > > When I ran it, it was pointing out cases where you have:
> > > if (foo->bar != NULL)
> > > when you can just use:
> > > if (foo->bar)
> > > 
> > It's weird for checkpatch.pl not showing the same. I would fix the
> > code
> > with above example.
> That is odd.  Are you running it from within the U-Boot tree?
> 
Yes.
> > 
> > > 
> > > [snip]
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > diff --git a/common/fs_loader.c b/common/fs_loader.c
> > > > > > new file mode 100644
> > > > > > index 0000000..56d29b6
> > > > > > --- /dev/null
> > > > > > +++ b/common/fs_loader.c
> > > > > > @@ -0,0 +1,309 @@
> > > > > > +/*
> > > > > > + * Copyright (C) 2017 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>
> > > > > > +#include <spl.h>
> > > > > This wants <asm/spl.h> which is not globally available, so
> > > > > you
> > > > > need
> > > > > to
> > > > > come up with something here.  At least making this Kconfig-
> > > > > enabled
> > > > > will
> > > > > be a start and perhaps OK for now.
> > > > > 
> > > > I can enable the Kconfig, and put the caution about dependency
> > > > on
> > > > <asm/spl.h> in document.
> > > Well, you need to make that part of the code depend on CONFIG_SPL
> > > as
> > > SPL
> > > support requires <asm/spl.h> to exist.   Perhaps part of the code
> > > needs
> > > to be refactored to more easily deal with SPL not always being
> > > present?
> > > 
> > How about i just move the whole enum { } from line 17 to line 35 in
> > asm/spl.h to fs.h, and then enum{} above replaced with #include
> > <fs.h>?
> > 
> > asm/spl.h
> > ----------
> > #if defined(CONFIG_ARCH_OMAP2PLUS) \
> > 	|| defined(CONFIG_EXYNOS4) || defined(CONFIG_EXYNOS5) \
> > 	|| defined(CONFIG_EXYNOS4210)
> > /* Platform-specific defines */
> > #include <asm/arch/spl.h>
> > 
> > #else
> > #include <fs.h>  <-- replacement
> > #endif
> > [...]
> No, because that's still arch specific code and will blow up some
> platforms that do not have asm/spl.h at all and is still non-portable
> (look at the various asm/spl.h files that do exist for how
> BOOT_DEVICE_xxx is handled in different places).
> 
Ok, you are right.
> It's OK to restructure your code to have:
> #ifdef CONFIG_SPL
> #include <spl.h>
> ... SPL specific functions
> #endif
> 
Okay, noted.

[U-Boot] [PATCH v6 2/2] common: Generic firmware loader for file system

[Marek Vasut]

On 12/27/2017 06:04 AM, tien.fong.chee at intel.com wrote:

Whoa, this improved substantially since last time I checked. Minor
nitpicks below.

[...]

> +/* 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) < 0 ? -ENODEV : 0;

if (err)
	return err;
?

> +#endif
> +
> +	return err;
> +}
> +#else
> +static int init_usb(void)
> +{
> +	printf("Error: Cannot load flash image: no USB support\n");

debug() ? Fix globally ...

> +	return -ENOSYS;
> +}
> +#endif
> +#endif
> +
> +#ifdef CONFIG_SATA
> +static int init_storage_sata(void)
> +{
> +	return sata_probe(0);
> +}
> +#else
> +static int init_storage_sata(void)
> +{
> +	printf("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];
> +
> +	sprintf(cmd, "ubi part %s", location->mtdpart);

snprintf() ...

> +	ret = run_command(cmd, 0);
> +	if (ret)
> +		return ret;
> +
> +	sprintf(cmd, "ubifsmount %s", location->ubivol);
> +
> +	ret = run_command(cmd, 0);
> +
> +	return ret;
> +}
> +
> +static int umount_ubifs(void)
> +{
> +	return run_command("ubifsumount", 0);

Just call the function directly ?

> +}
> +#else
> +static int mount_ubifs(struct device_location *location)
> +{
> +	printf("Error: Cannot load image: no UBIFS support\n");
> +	return -ENOSYS;
> +}
> +#endif
> +
> +#if defined(CONFIG_SPL_MMC_SUPPORT) && defined(CONFIG_SPL_BUILD)
> +static int init_mmc(void)
> +{
> +	/* Just for the case MMC is not yet initialized */
> +	struct mmc *mmc = NULL;
> +	int err;
> +
> +	spl_mmc_find_device(&mmc, spl_boot_device());
> +
> +	err = mmc_init(mmc);
> +	if (err) {
> +		printf("spl: mmc init failed with error: %d\n", err);
> +		return err;
> +	}
> +
> +	return err;
> +}
> +#else
> +static 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 != NULL)
> +			ret = fs_set_blk_dev("ubi", NULL, FS_TYPE_UBIFS);
> +		else
> +			ret = -ENODEV;
> +	} else {
> +		printf("Error: unsupported location storage.\n");
> +		return -ENODEV;
> +	}
> +
> +	if (ret)
> +		printf("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 != NULL) {
> +		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 {
> +		printf("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;
> +	}
> +}
> +
> +/*
> + * Prepare firmware struct;
> + * return -ve if fail.

Use kerneldoc and keep it consistent.

> + */
> +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) {
> +		printf("%s: calloc(struct firmware) failed\n", __func__);

If malloc fails, you're screwed anyway and printf will likely fail too,
so drop it.

> +		return -ENOMEM;
> +	}
> +
> +	fw_priv = calloc(1, sizeof(*fw_priv));
> +	if (!fw_priv) {
> +		printf("%s: calloc(struct fw_priv) failed\n", __func__);
> +		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;
> +}
[...]
-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v6 2/2] common: Generic firmware loader for file system

[Chee, Tien Fong]

On Tue, 2018-01-16 at 15:41 +0100, Marek Vasut wrote:
> On 12/27/2017 06:04 AM, tien.fong.chee at intel.com wrote:
> 
> Whoa, this improved substantially since last time I checked. Minor
> nitpicks below.
> 
> [...]
> 
> > 
> > +/* 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) < 0 ? -ENODEV : 0;
> if (err)
> 	return err;
> ?
> 
This is last line code of the function, so it's always return the
result regardless error or not.
> > 
> > +#endif
> > +
> > +	return err;
> > +}
> > +#else
> > +static int init_usb(void)
> > +{
> > +	printf("Error: Cannot load flash image: no USB
> > support\n");
> debug() ? Fix globally ...
> 
okay.
> > 
> > +	return -ENOSYS;
> > +}
> > +#endif
> > +#endif
> > +
> > +#ifdef CONFIG_SATA
> > +static int init_storage_sata(void)
> > +{
> > +	return sata_probe(0);
> > +}
> > +#else
> > +static int init_storage_sata(void)
> > +{
> > +	printf("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];
> > +
> > +	sprintf(cmd, "ubi part %s", location->mtdpart);
> snprintf() ...
> 
okay.
> > 
> > +	ret = run_command(cmd, 0);
> > +	if (ret)
> > +		return ret;
> > +
> > +	sprintf(cmd, "ubifsmount %s", location->ubivol);
> > +
> > +	ret = run_command(cmd, 0);
> > +
> > +	return ret;
> > +}
> > +
> > +static int umount_ubifs(void)
> > +{
> > +	return run_command("ubifsumount", 0);
> Just call the function directly ?
> 
There are some checking like ubifs_initialized in the cmd/ubifs.c.
Direct callng the function would bypass those checking.
> > 
> > +}
> > +#else
> > +static int mount_ubifs(struct device_location *location)
> > +{
> > +	printf("Error: Cannot load image: no UBIFS support\n");
> > +	return -ENOSYS;
> > +}
> > +#endif
> > +
> > +#if defined(CONFIG_SPL_MMC_SUPPORT) && defined(CONFIG_SPL_BUILD)
> > +static int init_mmc(void)
> > +{
> > +	/* Just for the case MMC is not yet initialized */
> > +	struct mmc *mmc = NULL;
> > +	int err;
> > +
> > +	spl_mmc_find_device(&mmc, spl_boot_device());
> > +
> > +	err = mmc_init(mmc);
> > +	if (err) {
> > +		printf("spl: mmc init failed with error: %d\n",
> > err);
> > +		return err;
> > +	}
> > +
> > +	return err;
> > +}
> > +#else
> > +static 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 != NULL)
> > +			ret = fs_set_blk_dev("ubi", NULL,
> > FS_TYPE_UBIFS);
> > +		else
> > +			ret = -ENODEV;
> > +	} else {
> > +		printf("Error: unsupported location storage.\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	if (ret)
> > +		printf("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 != NULL) {
> > +		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 {
> > +		printf("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;
> > +	}
> > +}
> > +
> > +/*
> > + * Prepare firmware struct;
> > + * return -ve if fail.
> Use kerneldoc and keep it consistent.
> 
kerneldoc doesn't has explanation for this function, and this function
is not for user. Or you means i shouldn't put the comment and
description to the function here?
> > 
> > + */
> > +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) {
> > +		printf("%s: calloc(struct firmware) failed\n",
> > __func__);
> If malloc fails, you're screwed anyway and printf will likely fail
> too,
> so drop it.
> 
okay.
> > 
> > +		return -ENOMEM;
> > +	}
> > +
> > +	fw_priv = calloc(1, sizeof(*fw_priv));
> > +	if (!fw_priv) {
> > +		printf("%s: calloc(struct fw_priv) failed\n",
> > __func__);
> > +		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;
> > +}
> [...]

[U-Boot] [PATCH v6 2/2] common: Generic firmware loader for file system

[Marek Vasut]

On 01/18/2018 05:33 AM, Chee, Tien Fong wrote:
> On Tue, 2018-01-16 at 15:41 +0100, Marek Vasut wrote:
>> On 12/27/2017 06:04 AM, tien.fong.chee at intel.com wrote:
>>
>> Whoa, this improved substantially since last time I checked. Minor
>> nitpicks below.
>>
>> [...]
>>
>>>
>>> +/* 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) < 0 ? -ENODEV : 0;
>> if (err)
>> 	return err;
>> ?
>>
> This is last line code of the function, so it's always return the
> result regardless error or not.

You are rewriting the true error code with -ENODEV instead of
propagating it.

>>>
>>> +#endif
>>> +
>>> +	return err;
>>> +}
>>> +#else
>>> +static int init_usb(void)
>>> +{
>>> +	printf("Error: Cannot load flash image: no USB
>>> support\n");
>> debug() ? Fix globally ...
>>
> okay.
>>>
>>> +	return -ENOSYS;
>>> +}
>>> +#endif
>>> +#endif
>>> +
>>> +#ifdef CONFIG_SATA
>>> +static int init_storage_sata(void)
>>> +{
>>> +	return sata_probe(0);
>>> +}
>>> +#else
>>> +static int init_storage_sata(void)
>>> +{
>>> +	printf("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];
>>> +
>>> +	sprintf(cmd, "ubi part %s", location->mtdpart);
>> snprintf() ...
>>
> okay.
>>>
>>> +	ret = run_command(cmd, 0);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	sprintf(cmd, "ubifsmount %s", location->ubivol);
>>> +
>>> +	ret = run_command(cmd, 0);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int umount_ubifs(void)
>>> +{
>>> +	return run_command("ubifsumount", 0);
>> Just call the function directly ?
>>
> There are some checking like ubifs_initialized in the cmd/ubifs.c.
> Direct callng the function would bypass those checking.

Then factor those out into a function you can all and call that function.

>>>
>>> +}
>>> +#else
>>> +static int mount_ubifs(struct device_location *location)
>>> +{
>>> +	printf("Error: Cannot load image: no UBIFS support\n");
>>> +	return -ENOSYS;
>>> +}
>>> +#endif
>>> +
>>> +#if defined(CONFIG_SPL_MMC_SUPPORT) && defined(CONFIG_SPL_BUILD)
>>> +static int init_mmc(void)
>>> +{
>>> +	/* Just for the case MMC is not yet initialized */
>>> +	struct mmc *mmc = NULL;
>>> +	int err;
>>> +
>>> +	spl_mmc_find_device(&mmc, spl_boot_device());
>>> +
>>> +	err = mmc_init(mmc);
>>> +	if (err) {
>>> +		printf("spl: mmc init failed with error: %d\n",
>>> err);
>>> +		return err;
>>> +	}
>>> +
>>> +	return err;
>>> +}
>>> +#else
>>> +static 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 != NULL)
>>> +			ret = fs_set_blk_dev("ubi", NULL,
>>> FS_TYPE_UBIFS);
>>> +		else
>>> +			ret = -ENODEV;
>>> +	} else {
>>> +		printf("Error: unsupported location storage.\n");
>>> +		return -ENODEV;
>>> +	}
>>> +
>>> +	if (ret)
>>> +		printf("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 != NULL) {
>>> +		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 {
>>> +		printf("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;
>>> +	}
>>> +}
>>> +
>>> +/*
>>> + * Prepare firmware struct;
>>> + * return -ve if fail.
>> Use kerneldoc and keep it consistent.
>>
> kerneldoc doesn't has explanation for this function, and this function
> is not for user. Or you means i shouldn't put the comment and
> description to the function here?

Kerneldoc specifies the format of the comment, so that documentation can
be generated from it. If you document functions, use that format.

>>>
>>> + */
>>> +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) {
>>> +		printf("%s: calloc(struct firmware) failed\n",
>>> __func__);
>> If malloc fails, you're screwed anyway and printf will likely fail
>> too,
>> so drop it.
>>
> okay.
>>>
>>> +		return -ENOMEM;
>>> +	}
>>> +
>>> +	fw_priv = calloc(1, sizeof(*fw_priv));
>>> +	if (!fw_priv) {
>>> +		printf("%s: calloc(struct fw_priv) failed\n",
>>> __func__);
>>> +		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;
>>> +}
>> [...]


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v6 2/2] common: Generic firmware loader for file system

[Chee, Tien Fong]

On Thu, 2018-01-18 at 12:12 +0100, Marek Vasut wrote:
> On 01/18/2018 05:33 AM, Chee, Tien Fong wrote:
> > 
> > On Tue, 2018-01-16 at 15:41 +0100, Marek Vasut wrote:
> > > 
> > > On 12/27/2017 06:04 AM, tien.fong.chee at intel.com wrote:
> > > 
> > > Whoa, this improved substantially since last time I checked.
> > > Minor
> > > nitpicks below.
> > > 
> > > [...]
> > > 
> > > > 
> > > > 
> > > > +/* 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) < 0 ? -ENODEV : 0;
> > > if (err)
> > > 	return err;
> > > ?
> > > 
> > This is last line code of the function, so it's always return the
> > result regardless error or not.
> You are rewriting the true error code with -ENODEV instead of
> propagating it.
> 
Ohh....are you saying to change the codes as shown in below:

err = usb_stor_scan(1);
if (err)
return err;


> > > 
> > > > 
> > > > 
> > > > +#endif
> > > > +
> > > > +	return err;
> > > > +}
> > > > +#else
> > > > +static int init_usb(void)
> > > > +{
> > > > +	printf("Error: Cannot load flash image: no USB
> > > > support\n");
> > > debug() ? Fix globally ...
> > > 
> > okay.
> > > 
> > > > 
> > > > 
> > > > +	return -ENOSYS;
> > > > +}
> > > > +#endif
> > > > +#endif
> > > > +
> > > > +#ifdef CONFIG_SATA
> > > > +static int init_storage_sata(void)
> > > > +{
> > > > +	return sata_probe(0);
> > > > +}
> > > > +#else
> > > > +static int init_storage_sata(void)
> > > > +{
> > > > +	printf("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];
> > > > +
> > > > +	sprintf(cmd, "ubi part %s", location->mtdpart);
> > > snprintf() ...
> > > 
> > okay.
> > > 
> > > > 
> > > > 
> > > > +	ret = run_command(cmd, 0);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	sprintf(cmd, "ubifsmount %s", location->ubivol);
> > > > +
> > > > +	ret = run_command(cmd, 0);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static int umount_ubifs(void)
> > > > +{
> > > > +	return run_command("ubifsumount", 0);
> > > Just call the function directly ?
> > > 
> > There are some checking like ubifs_initialized in the cmd/ubifs.c.
> > Direct callng the function would bypass those checking.
> Then factor those out into a function you can all and call that
> function.
> 
Just for curious, is it worth to factor those into a function? Does it
help to boost the performance or for other purpose?
> > 
> > > 
> > > > 
> > > > 
> > > > +}
> > > > +#else
> > > > +static int mount_ubifs(struct device_location *location)
> > > > +{
> > > > +	printf("Error: Cannot load image: no UBIFS
> > > > support\n");
> > > > +	return -ENOSYS;
> > > > +}
> > > > +#endif
> > > > +
> > > > +#if defined(CONFIG_SPL_MMC_SUPPORT) &&
> > > > defined(CONFIG_SPL_BUILD)
> > > > +static int init_mmc(void)
> > > > +{
> > > > +	/* Just for the case MMC is not yet initialized */
> > > > +	struct mmc *mmc = NULL;
> > > > +	int err;
> > > > +
> > > > +	spl_mmc_find_device(&mmc, spl_boot_device());
> > > > +
> > > > +	err = mmc_init(mmc);
> > > > +	if (err) {
> > > > +		printf("spl: mmc init failed with error:
> > > > %d\n",
> > > > err);
> > > > +		return err;
> > > > +	}
> > > > +
> > > > +	return err;
> > > > +}
> > > > +#else
> > > > +static 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 != NULL)
> > > > +			ret = fs_set_blk_dev("ubi", NULL,
> > > > FS_TYPE_UBIFS);
> > > > +		else
> > > > +			ret = -ENODEV;
> > > > +	} else {
> > > > +		printf("Error: unsupported location
> > > > storage.\n");
> > > > +		return -ENODEV;
> > > > +	}
> > > > +
> > > > +	if (ret)
> > > > +		printf("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 != NULL) {
> > > > +		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 {
> > > > +		printf("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;
> > > > +	}
> > > > +}
> > > > +
> > > > +/*
> > > > + * Prepare firmware struct;
> > > > + * return -ve if fail.
> > > Use kerneldoc and keep it consistent.
> > > 
> > kerneldoc doesn't has explanation for this function, and this
> > function
> > is not for user. Or you means i shouldn't put the comment and
> > description to the function here?
> Kerneldoc specifies the format of the comment, so that documentation
> can
> be generated from it. If you document functions, use that format.
> 
Okay.
> > 
> > > 
> > > > 
> > > > 
> > > > + */
> > > > +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) {
> > > > +		printf("%s: calloc(struct firmware) failed\n",
> > > > __func__);
> > > If malloc fails, you're screwed anyway and printf will likely
> > > fail
> > > too,
> > > so drop it.
> > > 
> > okay.
> > > 
> > > > 
> > > > 
> > > > +		return -ENOMEM;
> > > > +	}
> > > > +
> > > > +	fw_priv = calloc(1, sizeof(*fw_priv));
> > > > +	if (!fw_priv) {
> > > > +		printf("%s: calloc(struct fw_priv) failed\n",
> > > > __func__);
> > > > +		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;
> > > > +}
> > > [...]
> 

[U-Boot] [PATCH v6 2/2] common: Generic firmware loader for file system

[Lothar Waßmann]

Hi,

On Mon, 22 Jan 2018 07:11:37 +0000 Chee, Tien Fong wrote:
> On Thu, 2018-01-18 at 12:12 +0100, Marek Vasut wrote:
> > On 01/18/2018 05:33 AM, Chee, Tien Fong wrote:
> > > 
> > > On Tue, 2018-01-16 at 15:41 +0100, Marek Vasut wrote:
> > > > 
> > > > On 12/27/2017 06:04 AM, tien.fong.chee at intel.com wrote:
> > > > 
> > > > Whoa, this improved substantially since last time I checked.
> > > > Minor
> > > > nitpicks below.
> > > > 
> > > > [...]
> > > > 
> > > > > 
> > > > > 
> > > > > +/* 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) < 0 ? -ENODEV : 0;
> > > > if (err)
> > > > 	return err;
> > > > ?
> > > > 
> > > This is last line code of the function, so it's always return the
> > > result regardless error or not.
> > You are rewriting the true error code with -ENODEV instead of
> > propagating it.
> > 
> Ohh....are you saying to change the codes as shown in below:
> 
> err = usb_stor_scan(1);
> if (err)
> return err;
> 
usb_stor_scan() does not return a sensible error code, but '-1' if no
device was found. This should be changed to -ENODEV then!


Lothar Waßmann

[U-Boot] [PATCH v6 2/2] common: Generic firmware loader for file system

[Chee, Tien Fong]

On Mon, 2018-01-22 at 09:44 +0100, Lothar Waßmann wrote:
> Hi,
> 
> On Mon, 22 Jan 2018 07:11:37 +0000 Chee, Tien Fong wrote:
> > 
> > On Thu, 2018-01-18 at 12:12 +0100, Marek Vasut wrote:
> > > 
> > > On 01/18/2018 05:33 AM, Chee, Tien Fong wrote:
> > > > 
> > > > 
> > > > On Tue, 2018-01-16 at 15:41 +0100, Marek Vasut wrote:
> > > > > 
> > > > > 
> > > > > On 12/27/2017 06:04 AM, tien.fong.chee at intel.com wrote:
> > > > > 
> > > > > Whoa, this improved substantially since last time I checked.
> > > > > Minor
> > > > > nitpicks below.
> > > > > 
> > > > > [...]
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > +/* 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) < 0 ? -ENODEV : 0;
> > > > > if (err)
> > > > > 	return err;
> > > > > ?
> > > > > 
> > > > This is last line code of the function, so it's always return
> > > > the
> > > > result regardless error or not.
> > > You are rewriting the true error code with -ENODEV instead of
> > > propagating it.
> > > 
> > Ohh....are you saying to change the codes as shown in below:
> > 
> > err = usb_stor_scan(1);
> > if (err)
> > return err;
> > 
> usb_stor_scan() does not return a sensible error code, but '-1' if no
> device was found. This should be changed to -ENODEV then!
> 
Okay, so this should be fixed in usb_stor_scan() function.
> 
> Lothar Waßmann

[U-Boot] [PATCH v6 2/2] common: Generic firmware loader for file system

[Marek Vasut]

On 01/22/2018 08:11 AM, Chee, Tien Fong wrote:

[...]

>>> This is last line code of the function, so it's always return the
>>> result regardless error or not.
>> You are rewriting the true error code with -ENODEV instead of
>> propagating it.
>>
> Ohh....are you saying to change the codes as shown in below:
> 
> err = usb_stor_scan(1);
> if (err)
> return err;

Right

[...]
>>>>> +static int umount_ubifs(void)
>>>>> +{
>>>>> +	return run_command("ubifsumount", 0);
>>>> Just call the function directly ?
>>>>
>>> There are some checking like ubifs_initialized in the cmd/ubifs.c.
>>> Direct callng the function would bypass those checking.
>> Then factor those out into a function you can all and call that
>> function.
>>
> Just for curious, is it worth to factor those into a function? Does it
> help to boost the performance or for other purpose?

It just makes no sense to involve the whole command machinery if you can
call a function which does exactly the same.

[...]

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v6 2/2] common: Generic firmware loader for file system

[Simon Goldschmidt]

On 27.12.2017 06:04, 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>
> ---
>   common/Makefile            |   1 +
>   common/fs_loader.c         | 309 +++++++++++++++++++++++++++++++++++++++++++++
>   doc/README.firmware_loader |  76 +++++++++++
>   include/fs_loader.h        |  28 ++++
>   4 files changed, 414 insertions(+)
>   create mode 100644 common/fs_loader.c
>   create mode 100644 doc/README.firmware_loader
>   create mode 100644 include/fs_loader.h

<snip>

> +#if defined(CONFIG_SPL_MMC_SUPPORT) && defined(CONFIG_SPL_BUILD)
> +static int init_mmc(void)
> +{
> +	/* Just for the case MMC is not yet initialized */
> +	struct mmc *mmc = NULL;
> +	int err;
> +
> +	spl_mmc_find_device(&mmc, spl_boot_device());
> +
> +	err = mmc_init(mmc);
> +	if (err) {
> +		printf("spl: mmc init failed with error: %d\n", err);
> +		return err;
> +	}
> +
> +	return err;
> +}

I see two problems here: First, you're ignoring the return value of 
spl_mmc_find_device() and initialize 'mmc' to NULL instead. Wouldn't it 
be better to let 'mmc' be uninitialized and return the error code 
returned by spl_mmc_find_device() if there is one?

Second, using spl_boot_device() would prevent making the loader work on 
mach-socfpga when spl is not loaded from mmc, right?

E.g. for the case I'm currently trying to fix (boot from qspi), this 
loader would not work although there's technically no reason since the 
platform only has one mmc. The call to spl_boot_device() could be 
replaced by the exact value here for platforms that only have one mmc. I 
don't know how to fix that, though.

Regards,
Simon

[U-Boot] [PATCH v6 2/2] common: Generic firmware loader for file system

[Chee, Tien Fong]

On Thu, 2018-01-18 at 06:57 +0100, Simon Goldschmidt wrote:
> On 27.12.2017 06:04, 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>
> > ---
> >   common/Makefile            |   1 +
> >   common/fs_loader.c         | 309
> > +++++++++++++++++++++++++++++++++++++++++++++
> >   doc/README.firmware_loader |  76 +++++++++++
> >   include/fs_loader.h        |  28 ++++
> >   4 files changed, 414 insertions(+)
> >   create mode 100644 common/fs_loader.c
> >   create mode 100644 doc/README.firmware_loader
> >   create mode 100644 include/fs_loader.h
> <snip>
> 
> > 
> > +#if defined(CONFIG_SPL_MMC_SUPPORT) && defined(CONFIG_SPL_BUILD)
> > +static int init_mmc(void)
> > +{
> > +	/* Just for the case MMC is not yet initialized */
> > +	struct mmc *mmc = NULL;
> > +	int err;
> > +
> > +	spl_mmc_find_device(&mmc, spl_boot_device());
> > +
> > +	err = mmc_init(mmc);
> > +	if (err) {
> > +		printf("spl: mmc init failed with error: %d\n",
> > err);
> > +		return err;
> > +	}
> > +
> > +	return err;
> > +}
> I see two problems here: First, you're ignoring the return value of 
> spl_mmc_find_device() and initialize 'mmc' to NULL instead. Wouldn't
> it 
> be better to let 'mmc' be uninitialized and return the error code 
> returned by spl_mmc_find_device() if there is one?
> 
Yeah, you are right, i should add the check on the return value. I
think that would better to initialize NULL to mmc, because there is no
checking on the mmc in spl_mmc_find_device(), so that is possible
memory access violation/abort can be happended if unknown value in mmc
pointer.

> Second, using spl_boot_device() would prevent making the loader work
> on 
> mach-socfpga when spl is not loaded from mmc, right?
> 
> E.g. for the case I'm currently trying to fix (boot from qspi), this 
> loader would not work although there's technically no reason since
> the 
> platform only has one mmc. The call to spl_boot_device() could be 
> replaced by the exact value here for platforms that only have one
> mmc. I 
> don't know how to fix that, though.
> 
The main purpose here is to initialize the mmc driver. So which storage
user wants to load the file is totally depend what storage such as mmc
user defines in location->name. Loader would init the storage based on
the storage defined in location->name before accessing it. Since the
loader only support file system at this moment, i would suggest FAT fs 
for mmc and ubi fs for qspi.

> Regards,
> Simon
> 

[U-Boot] [PATCH v6 2/2] common: Generic firmware loader for file system

[Simon Goldschmidt]

On 22.01.2018 09:08, Chee, Tien Fong wrote:
> On Thu, 2018-01-18 at 06:57 +0100, Simon Goldschmidt wrote:
>> On 27.12.2017 06:04, 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>
>>> ---
>>>    common/Makefile            |   1 +
>>>    common/fs_loader.c         | 309
>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>    doc/README.firmware_loader |  76 +++++++++++
>>>    include/fs_loader.h        |  28 ++++
>>>    4 files changed, 414 insertions(+)
>>>    create mode 100644 common/fs_loader.c
>>>    create mode 100644 doc/README.firmware_loader
>>>    create mode 100644 include/fs_loader.h
>> <snip>
>>
>>> +#if defined(CONFIG_SPL_MMC_SUPPORT) && defined(CONFIG_SPL_BUILD)
>>> +static int init_mmc(void)
>>> +{
>>> +	/* Just for the case MMC is not yet initialized */
>>> +	struct mmc *mmc = NULL;
>>> +	int err;
>>> +
>>> +	spl_mmc_find_device(&mmc, spl_boot_device());
>>> +
>>> +	err = mmc_init(mmc);
>>> +	if (err) {
>>> +		printf("spl: mmc init failed with error: %d\n",
>>> err);
>>> +		return err;
>>> +	}
>>> +
>>> +	return err;
>>> +}
>> I see two problems here: First, you're ignoring the return value of
>> spl_mmc_find_device() and initialize 'mmc' to NULL instead. Wouldn't
>> it
>> be better to let 'mmc' be uninitialized and return the error code
>> returned by spl_mmc_find_device() if there is one?
>>
> Yeah, you are right, i should add the check on the return value. I
> think that would better to initialize NULL to mmc, because there is no
> checking on the mmc in spl_mmc_find_device(), so that is possible
> memory access violation/abort can be happended if unknown value in mmc
> pointer.
>
>> Second, using spl_boot_device() would prevent making the loader work
>> on
>> mach-socfpga when spl is not loaded from mmc, right?
>>
>> E.g. for the case I'm currently trying to fix (boot from qspi), this
>> loader would not work although there's technically no reason since
>> the
>> platform only has one mmc. The call to spl_boot_device() could be
>> replaced by the exact value here for platforms that only have one
>> mmc. I
>> don't know how to fix that, though.
>>
> The main purpose here is to initialize the mmc driver. So which storage
> user wants to load the file is totally depend what storage such as mmc
> user defines in location->name. Loader would init the storage based on
> the storage defined in location->name before accessing it. Since the
> loader only support file system at this moment, i would suggest FAT fs
> for mmc and ubi fs for qspi.

What I meant to say is this: at least on mach-socfpga, from reading the 
code, I cannot load a file from mmc when booting from qspi, as 
'spl_boot_device' returns 'BOOT_DEVICE_SPI' in that case, although I 
need to pass 'BOOT_DEVICE_MMC1' to 'spl_mmc_find_device'. Or am I wrong 
here?

Regards,
Simon

[U-Boot] [PATCH v6 2/2] common: Generic firmware loader for file system

[Chee, Tien Fong]

On Mon, 2018-01-22 at 12:41 +0100, Simon Goldschmidt wrote:
> On 22.01.2018 09:08, Chee, Tien Fong wrote:
> > 
> > On Thu, 2018-01-18 at 06:57 +0100, Simon Goldschmidt wrote:
> > > 
> > > On 27.12.2017 06:04, 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>
> > > > ---
> > > >    common/Makefile            |   1 +
> > > >    common/fs_loader.c         | 309
> > > > +++++++++++++++++++++++++++++++++++++++++++++
> > > >    doc/README.firmware_loader |  76 +++++++++++
> > > >    include/fs_loader.h        |  28 ++++
> > > >    4 files changed, 414 insertions(+)
> > > >    create mode 100644 common/fs_loader.c
> > > >    create mode 100644 doc/README.firmware_loader
> > > >    create mode 100644 include/fs_loader.h
> > > <snip>
> > > 
> > > > 
> > > > +#if defined(CONFIG_SPL_MMC_SUPPORT) &&
> > > > defined(CONFIG_SPL_BUILD)
> > > > +static int init_mmc(void)
> > > > +{
> > > > +	/* Just for the case MMC is not yet initialized */
> > > > +	struct mmc *mmc = NULL;
> > > > +	int err;
> > > > +
> > > > +	spl_mmc_find_device(&mmc, spl_boot_device());
> > > > +
> > > > +	err = mmc_init(mmc);
> > > > +	if (err) {
> > > > +		printf("spl: mmc init failed with error:
> > > > %d\n",
> > > > err);
> > > > +		return err;
> > > > +	}
> > > > +
> > > > +	return err;
> > > > +}
> > > I see two problems here: First, you're ignoring the return value
> > > of
> > > spl_mmc_find_device() and initialize 'mmc' to NULL instead.
> > > Wouldn't
> > > it
> > > be better to let 'mmc' be uninitialized and return the error code
> > > returned by spl_mmc_find_device() if there is one?
> > > 
> > Yeah, you are right, i should add the check on the return value. I
> > think that would better to initialize NULL to mmc, because there is
> > no
> > checking on the mmc in spl_mmc_find_device(), so that is possible
> > memory access violation/abort can be happended if unknown value in
> > mmc
> > pointer.
> > 
> > > 
> > > Second, using spl_boot_device() would prevent making the loader
> > > work
> > > on
> > > mach-socfpga when spl is not loaded from mmc, right?
> > > 
> > > E.g. for the case I'm currently trying to fix (boot from qspi),
> > > this
> > > loader would not work although there's technically no reason
> > > since
> > > the
> > > platform only has one mmc. The call to spl_boot_device() could be
> > > replaced by the exact value here for platforms that only have one
> > > mmc. I
> > > don't know how to fix that, though.
> > > 
> > The main purpose here is to initialize the mmc driver. So which
> > storage
> > user wants to load the file is totally depend what storage such as
> > mmc
> > user defines in location->name. Loader would init the storage based
> > on
> > the storage defined in location->name before accessing it. Since
> > the
> > loader only support file system at this moment, i would suggest FAT
> > fs
> > for mmc and ubi fs for qspi.
> What I meant to say is this: at least on mach-socfpga, from reading
> the 
> code, I cannot load a file from mmc when booting from qspi, as 
> 'spl_boot_device' returns 'BOOT_DEVICE_SPI' in that case, although I 
> need to pass 'BOOT_DEVICE_MMC1' to 'spl_mmc_find_device'. Or am I
> wrong 
> here?
> 
Okay, i got you.

Yeah, you are right for use case if you need to load the file from mmc
during SPL boot and SPL is not loaded from mmc.

Since the spl_boot_device is platform dependent, you may try to add
some state machine so that this function can return correct device
type.

Secondly, you can use this function in U-Boot instead of SPL.

Lastly, i can declare __Weak to init_mmc, so that user can define their
own implementation.

> Regards,
> Simon

[U-Boot] [PATCH v6 2/2] common: Generic firmware loader for file system

[Simon Goldschmidt]

On 23.01.2018 07:28, Chee, Tien Fong wrote:
> On Mon, 2018-01-22 at 12:41 +0100, Simon Goldschmidt wrote:
>> On 22.01.2018 09:08, Chee, Tien Fong wrote:
>>> On Thu, 2018-01-18 at 06:57 +0100, Simon Goldschmidt wrote:
>>>> On 27.12.2017 06:04, 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>
>>>>> ---
>>>>>     common/Makefile            |   1 +
>>>>>     common/fs_loader.c         | 309
>>>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>>>     doc/README.firmware_loader |  76 +++++++++++
>>>>>     include/fs_loader.h        |  28 ++++
>>>>>     4 files changed, 414 insertions(+)
>>>>>     create mode 100644 common/fs_loader.c
>>>>>     create mode 100644 doc/README.firmware_loader
>>>>>     create mode 100644 include/fs_loader.h
>>>> <snip>
>>>>
>>>>> +#if defined(CONFIG_SPL_MMC_SUPPORT) &&
>>>>> defined(CONFIG_SPL_BUILD)
>>>>> +static int init_mmc(void)
>>>>> +{
>>>>> +	/* Just for the case MMC is not yet initialized */
>>>>> +	struct mmc *mmc = NULL;
>>>>> +	int err;
>>>>> +
>>>>> +	spl_mmc_find_device(&mmc, spl_boot_device());
>>>>> +
>>>>> +	err = mmc_init(mmc);
>>>>> +	if (err) {
>>>>> +		printf("spl: mmc init failed with error:
>>>>> %d\n",
>>>>> err);
>>>>> +		return err;
>>>>> +	}
>>>>> +
>>>>> +	return err;
>>>>> +}
>>>> I see two problems here: First, you're ignoring the return value
>>>> of
>>>> spl_mmc_find_device() and initialize 'mmc' to NULL instead.
>>>> Wouldn't
>>>> it
>>>> be better to let 'mmc' be uninitialized and return the error code
>>>> returned by spl_mmc_find_device() if there is one?
>>>>
>>> Yeah, you are right, i should add the check on the return value. I
>>> think that would better to initialize NULL to mmc, because there is
>>> no
>>> checking on the mmc in spl_mmc_find_device(), so that is possible
>>> memory access violation/abort can be happended if unknown value in
>>> mmc
>>> pointer.
>>>
>>>> Second, using spl_boot_device() would prevent making the loader
>>>> work
>>>> on
>>>> mach-socfpga when spl is not loaded from mmc, right?
>>>>
>>>> E.g. for the case I'm currently trying to fix (boot from qspi),
>>>> this
>>>> loader would not work although there's technically no reason
>>>> since
>>>> the
>>>> platform only has one mmc. The call to spl_boot_device() could be
>>>> replaced by the exact value here for platforms that only have one
>>>> mmc. I
>>>> don't know how to fix that, though.
>>>>
>>> The main purpose here is to initialize the mmc driver. So which
>>> storage
>>> user wants to load the file is totally depend what storage such as
>>> mmc
>>> user defines in location->name. Loader would init the storage based
>>> on
>>> the storage defined in location->name before accessing it. Since
>>> the
>>> loader only support file system at this moment, i would suggest FAT
>>> fs
>>> for mmc and ubi fs for qspi.
>> What I meant to say is this: at least on mach-socfpga, from reading
>> the
>> code, I cannot load a file from mmc when booting from qspi, as
>> 'spl_boot_device' returns 'BOOT_DEVICE_SPI' in that case, although I
>> need to pass 'BOOT_DEVICE_MMC1' to 'spl_mmc_find_device'. Or am I
>> wrong
>> here?
>>
> Okay, i got you.
>
> Yeah, you are right for use case if you need to load the file from mmc
> during SPL boot and SPL is not loaded from mmc.
>
> Since the spl_boot_device is platform dependent, you may try to add
> some state machine so that this function can return correct device
> type.
>
> Secondly, you can use this function in U-Boot instead of SPL.

You're right on that. Thinking about it, I would probably use it from 
U-Boot, not from SPL...

> Lastly, i can declare __Weak to init_mmc, so that user can define their
> own implementation.

At least somehow allowing to override which device is passed to 
'spl_mmc_find_device' might be good, instead of hardcoding it to 
'spl_boot_device'...

Regards,
Simon

[U-Boot] [PATCH v6 2/2] common: Generic firmware loader for file system

[Simon Goldschmidt]

On 27.12.2017 06:04, 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>
> ---
>   common/Makefile            |   1 +
>   common/fs_loader.c         | 309 +++++++++++++++++++++++++++++++++++++++++++++
>   doc/README.firmware_loader |  76 +++++++++++
>   include/fs_loader.h        |  28 ++++
>   4 files changed, 414 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/Makefile b/common/Makefile
> index cec506f..2934221 100644
> --- a/common/Makefile
> +++ b/common/Makefile
> @@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o

<snip>

> diff --git a/include/fs_loader.h b/include/fs_loader.h
> new file mode 100644
> index 0000000..83a8b80
> --- /dev/null
> +++ b/include/fs_loader.h
> @@ -0,0 +1,28 @@
> +/*
> + * Copyright (C) 2017 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. */

Would it make sense to use 'enum if_type' from blk.h here instead of a 
"magic" name? Or are the constants passed here "well-known" enough to 
hide them?

Regards,
Simon

> +	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

[U-Boot] [PATCH v6 2/2] common: Generic firmware loader for file system

[Chee, Tien Fong]

On Tue, 2018-01-23 at 08:58 +0100, Simon Goldschmidt wrote:
> On 27.12.2017 06:04, 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>
> > ---
> >   common/Makefile            |   1 +
> >   common/fs_loader.c         | 309
> > +++++++++++++++++++++++++++++++++++++++++++++
> >   doc/README.firmware_loader |  76 +++++++++++
> >   include/fs_loader.h        |  28 ++++
> >   4 files changed, 414 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/Makefile b/common/Makefile
> > index cec506f..2934221 100644
> > --- a/common/Makefile
> > +++ b/common/Makefile
> > @@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o
> <snip>
> 
> > 
> > diff --git a/include/fs_loader.h b/include/fs_loader.h
> > new file mode 100644
> > index 0000000..83a8b80
> > --- /dev/null
> > +++ b/include/fs_loader.h
> > @@ -0,0 +1,28 @@
> > +/*
> > + * Copyright (C) 2017 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. */
> Would it make sense to use 'enum if_type' from blk.h here instead of
> a 
> "magic" name? Or are the constants passed here "well-known" enough
> to 
> hide them?
> 
This structure is declared such way so that it can be compatible with
common/splash.c. It also much more easy to port fs_loader into
common/splash_source.c in later.

> Regards,
> Simon
> 
> > 
> > +	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

[U-Boot] [PATCH v6 2/2] common: Generic firmware loader for file system

[Simon Goldschmidt]

On 23.01.2018 09:31, Chee, Tien Fong wrote:
> On Tue, 2018-01-23 at 08:58 +0100, Simon Goldschmidt wrote:
>> On 27.12.2017 06:04, 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>
>>> ---
>>>    common/Makefile            |   1 +
>>>    common/fs_loader.c         | 309
>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>    doc/README.firmware_loader |  76 +++++++++++
>>>    include/fs_loader.h        |  28 ++++
>>>    4 files changed, 414 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/Makefile b/common/Makefile
>>> index cec506f..2934221 100644
>>> --- a/common/Makefile
>>> +++ b/common/Makefile
>>> @@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o
>> <snip>
>>
>>> diff --git a/include/fs_loader.h b/include/fs_loader.h
>>> new file mode 100644
>>> index 0000000..83a8b80
>>> --- /dev/null
>>> +++ b/include/fs_loader.h
>>> @@ -0,0 +1,28 @@
>>> +/*
>>> + * Copyright (C) 2017 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. */
>> Would it make sense to use 'enum if_type' from blk.h here instead of
>> a
>> "magic" name? Or are the constants passed here "well-known" enough
>> to
>> hide them?
>>
> This structure is declared such way so that it can be compatible with
> common/splash.c. It also much more easy to port fs_loader into
> common/splash_source.c in later.

OK, but reading splash_source.c, it seems to me that 'enum 
splash_storage' is used to detect the device to load from, not 'char *name'.

However, since these constant strings already seem to be "common 
knowledge" for callers of fs.h functions, it might be OK to use them in 
the firmware loader, too. I just wanted to point this out while this is 
in the reviewing process.

>> Regards,
>> Simon
>>
>>> +	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

[U-Boot] [PATCH v6 2/2] common: Generic firmware loader for file system

[Chee, Tien Fong]

On Tue, 2018-01-23 at 10:13 +0100, Simon Goldschmidt wrote:
> On 23.01.2018 09:31, Chee, Tien Fong wrote:
> > 
> > On Tue, 2018-01-23 at 08:58 +0100, Simon Goldschmidt wrote:
> > > 
> > > On 27.12.2017 06:04, 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>
> > > > ---
> > > >    common/Makefile            |   1 +
> > > >    common/fs_loader.c         | 309
> > > > +++++++++++++++++++++++++++++++++++++++++++++
> > > >    doc/README.firmware_loader |  76 +++++++++++
> > > >    include/fs_loader.h        |  28 ++++
> > > >    4 files changed, 414 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/Makefile b/common/Makefile
> > > > index cec506f..2934221 100644
> > > > --- a/common/Makefile
> > > > +++ b/common/Makefile
> > > > @@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o
> > > <snip>
> > > 
> > > > 
> > > > diff --git a/include/fs_loader.h b/include/fs_loader.h
> > > > new file mode 100644
> > > > index 0000000..83a8b80
> > > > --- /dev/null
> > > > +++ b/include/fs_loader.h
> > > > @@ -0,0 +1,28 @@
> > > > +/*
> > > > + * Copyright (C) 2017 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. */
> > > Would it make sense to use 'enum if_type' from blk.h here instead
> > > of
> > > a
> > > "magic" name? Or are the constants passed here "well-known"
> > > enough
> > > to
> > > hide them?
> > > 
> > This structure is declared such way so that it can be compatible
> > with
> > common/splash.c. It also much more easy to port fs_loader into
> > common/splash_source.c in later.
> OK, but reading splash_source.c, it seems to me that 'enum 
> splash_storage' is used to detect the device to load from, not 'char
> *name'.
> 
> However, since these constant strings already seem to be "common 
> knowledge" for callers of fs.h functions, it might be OK to use them
> in 
> the firmware loader, too. I just wanted to point this out while this
> is 
> in the reviewing process.
> 
Actually this is common interface name used in any fs related. You can
refer more details regarding @ifname in include/fs.h and include part.h
.

So, why this interface name must be in characters string?
It's actually serving in 2 main purposes, and 1 minor purpose:
1. console environment, most commands using those interface name as
their arguments in characters string format such as FPGA loadfs
command.

2. These command interface name is one of the characters string
argument of fs_set_blk_dev too.

3. As identity of default location and its dev_part configuration.

> > 
> > > 
> > > Regards,
> > > Simon
> > > 
> > > > 
> > > > +	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

[U-Boot] [PATCH v6 2/2] common: Generic firmware loader for file system

[Simon Goldschmidt]

On 24.01.2018 06:13, Chee, Tien Fong wrote:
> On Tue, 2018-01-23 at 10:13 +0100, Simon Goldschmidt wrote:
>> On 23.01.2018 09:31, Chee, Tien Fong wrote:
>>> On Tue, 2018-01-23 at 08:58 +0100, Simon Goldschmidt wrote:
>>>> On 27.12.2017 06:04, 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>
>>>>> ---
>>>>>     common/Makefile            |   1 +
>>>>>     common/fs_loader.c         | 309
>>>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>>>     doc/README.firmware_loader |  76 +++++++++++
>>>>>     include/fs_loader.h        |  28 ++++
>>>>>     4 files changed, 414 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/Makefile b/common/Makefile
>>>>> index cec506f..2934221 100644
>>>>> --- a/common/Makefile
>>>>> +++ b/common/Makefile
>>>>> @@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o
>>>> <snip>
>>>>
>>>>> diff --git a/include/fs_loader.h b/include/fs_loader.h
>>>>> new file mode 100644
>>>>> index 0000000..83a8b80
>>>>> --- /dev/null
>>>>> +++ b/include/fs_loader.h
>>>>> @@ -0,0 +1,28 @@
>>>>> +/*
>>>>> + * Copyright (C) 2017 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. */
>>>> Would it make sense to use 'enum if_type' from blk.h here instead
>>>> of
>>>> a
>>>> "magic" name? Or are the constants passed here "well-known"
>>>> enough
>>>> to
>>>> hide them?
>>>>
>>> This structure is declared such way so that it can be compatible
>>> with
>>> common/splash.c. It also much more easy to port fs_loader into
>>> common/splash_source.c in later.
>> OK, but reading splash_source.c, it seems to me that 'enum
>> splash_storage' is used to detect the device to load from, not 'char
>> *name'.
>>
>> However, since these constant strings already seem to be "common
>> knowledge" for callers of fs.h functions, it might be OK to use them
>> in
>> the firmware loader, too. I just wanted to point this out while this
>> is
>> in the reviewing process.
>>
> Actually this is common interface name used in any fs related. You can
> refer more details regarding @ifname in include/fs.h and include part.h
> .
>
> So, why this interface name must be in characters string?
> It's actually serving in 2 main purposes, and 1 minor purpose:
> 1. console environment, most commands using those interface name as
> their arguments in characters string format such as FPGA loadfs
> command.
>
> 2. These command interface name is one of the characters string
> argument of fs_set_blk_dev too.
>
> 3. As identity of default location and its dev_part configuration.

Right. I see that there does not seem to be a way to provide the 
interface type to fs.h without using a string. In most cases, this 
string is provided by the environment or via console, so I see where 
that comes from. It's probably not up to this patch to change it.

>>>> Regards,
>>>> Simon
>>>>
>>>>> +	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

[U-Boot] [PATCH v6 0/2] Generic firmware loader

[Chee, Tien Fong]

On Rab, 2017-12-27 at 13:04 +0800, tien.fong.chee at intel.com wrote:
Hi Lothar Waßmann,
> 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
> Lothar Waßmann in [v5].
> 
Are you OK with the patches?

> This series is working on top of u-boot.git -
>  http://git.denx.de/u-boot.git .
> 
> [v5]: https://www.mail-archive.com/u-boot at lists.denx.de/msg272771.htm
> l
> 
> v5 -> v6 changes:
> -----------------
> - Return error code when fs_read is fail.
> - Return actual read size when fs_read is success.
> 
> 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
> 
> Tien Fong Chee (2):
>   spl: Remove static declaration on spl_mmc_find_device function
>   common: Generic firmware loader for file system
> 
>  common/Makefile            |   1 +
>  common/fs_loader.c         | 309
> +++++++++++++++++++++++++++++++++++++++++++++
>  common/spl/spl_mmc.c       |   2 +-
>  doc/README.firmware_loader |  76 +++++++++++
>  include/fs_loader.h        |  28 ++++
>  include/spl.h              |   2 +
>  6 files changed, 417 insertions(+), 1 deletion(-)
>  create mode 100644 common/fs_loader.c
>  create mode 100644 doc/README.firmware_loader
>  create mode 100644 include/fs_loader.h
> 

[U-Boot] [PATCH v6 0/2] Generic firmware loader

[Lothar Waßmann]

Hi,

On Wed, 3 Jan 2018 08:46:09 +0000 Chee, Tien Fong wrote:
> On Rab, 2017-12-27 at 13:04 +0800, tien.fong.chee at intel.com wrote:
> Hi Lothar Waßmann,
> > 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
> > Lothar Waßmann in [v5].
> > 
> Are you OK with the patches?
> 
Sorry for the delay. I have been on vacation until now.
The patch series Looks good to me now.

Reviewed-By: Lothar Waßmann <LW@KARO-electronics.de>


Lothar Waßmann

[U-Boot] [PATCH v6 0/2] Generic firmware loader

[Chee, Tien Fong]

On Isn, 2018-01-08 at 09:07 +0100, Lothar Waßmann wrote:
> Hi,
> 
> On Wed, 3 Jan 2018 08:46:09 +0000 Chee, Tien Fong wrote:
> > 
> > On Rab, 2017-12-27 at 13:04 +0800, tien.fong.chee at intel.com wrote:
> > Hi Lothar Waßmann,
> > > 
> > > 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
> > > Lothar Waßmann in [v5].
> > > 
> > Are you OK with the patches?
> > 
> Sorry for the delay. I have been on vacation until now.
> The patch series Looks good to me now.
> 
> Reviewed-By: Lothar Waßmann <LW@KARO-electronics.de>
> 
Great, thanks for helping to review the codes.
> 
> Lothar Waßmann

[U-Boot] [PATCH v6 0/2] Generic firmware loader

[Chee, Tien Fong]

On Mon, 2018-01-08 at 09:07 +0100, Lothar Waßmann wrote:
Hi Tom Rini,
> Hi,
> 
> On Wed, 3 Jan 2018 08:46:09 +0000 Chee, Tien Fong wrote:
> > 
> > On Rab, 2017-12-27 at 13:04 +0800, tien.fong.chee at intel.com wrote:
> > Hi Lothar Waßmann,
> > > 
> > > 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
> > > Lothar Waßmann in [v5].
> > > 
> > Are you OK with the patches?
Inlcude Tom Rini for reviewing this patch.
> > 
> Sorry for the delay. I have been on vacation until now.
> The patch series Looks good to me now.
> 
> Reviewed-By: Lothar Waßmann <LW@KARO-electronics.de>
> 
> 
> Lothar Waßmann

[U-Boot] [PATCH v6 0/2] Generic firmware loader

[Chee, Tien Fong]

On Wed, 2017-12-27 at 13:04 +0800, tien.fong.chee at intel.com wrote:
Hi Tom Rini,
> 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
> Lothar Waßmann in [v5].
> 
> This series is working on top of u-boot.git -
>  http://git.denx.de/u-boot.git .
> 
Do you ok with this patchset?

Best regards,
TF
> [v5]: https://www.mail-archive.com/u-
> boot at lists.denx.de/msg272771.html
> 
> v5 -> v6 changes:
> -----------------
> - Return error code when fs_read is fail.
> - Return actual read size when fs_read is success.
> 
> 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
> 
> Tien Fong Chee (2):
>   spl: Remove static declaration on spl_mmc_find_device function
>   common: Generic firmware loader for file system
> 
>  common/Makefile            |   1 +
>  common/fs_loader.c         | 309
> +++++++++++++++++++++++++++++++++++++++++++++
>  common/spl/spl_mmc.c       |   2 +-
>  doc/README.firmware_loader |  76 +++++++++++
>  include/fs_loader.h        |  28 ++++
>  include/spl.h              |   2 +
>  6 files changed, 417 insertions(+), 1 deletion(-)
>  create mode 100644 common/fs_loader.c
>  create mode 100644 doc/README.firmware_loader
>  create mode 100644 include/fs_loader.h
>