Skip to contentSkip to conversation

Generic firmware loader

PATCH Superseded tien.fong.chee at intel.com 3 participants 5 messages Dec 12, 2017

v2 of 10 Superseded Latest is v10 →

lore
Share & tools
lore
#0 lore raw

v2 cover Generic firmware loader

From: Tien Fong Chee <[email protected]>

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

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

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

v1 -> v2 changes:
-----------------
- Fixed the tag.
- Fixed the bugs in generic firmware loader driver.

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   | 299 +++++++++++++++++++++++++++++++++++++++++++++++++++
 common/spl/spl_mmc.c |   2 +-
 include/fs_loader.h  |  30 ++++++
 include/spl.h        |   2 +
 5 files changed, 333 insertions(+), 1 deletion(-)
 create mode 100644 common/fs_loader.c
 create mode 100644 include/fs_loader.h
Signature · 1 line
2.2.0
#1 lore raw

PATCH v2 1/2 spl: Remove static declaration on spl_mmc_find_device function

From: Tien Fong Chee <[email protected]>

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.

---
 common/spl/spl_mmc.c | 2 +-
 include/spl.h        | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)
common/spl/spl_mmc.c +1 -1
index b57e0b0..183d05a 100644
@@ -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;
include/spl.h +2 -0
index 308ce7b..912983a 100644
@@ -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
Signature · 1 line
2.2.0
#2 lore raw

PATCH v2 2/2 common: Generic firmware loader for file system

In reply to tien.fong.chee at intel.com

From: Tien Fong Chee <[email protected]>

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.

---
 common/Makefile     |   1 +
 common/fs_loader.c  | 299 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/fs_loader.h |  30 ++++++
 3 files changed, 330 insertions(+)
 create mode 100644 common/fs_loader.c
 create mode 100644 include/fs_loader.h
common/Makefile +1 -0
index cec506f..2934221 100644
@@ -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
common/fs_loader.c new file +299 -0

299 lines not on this page · raw message

include/fs_loader.h new file +30 -0
new file mode 100644
index 0000000..dacc1f3
@@ -0,0 +1,30 @@
+/*
+ * 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 */
+	const char *name;	/* Filename */
+	u32 offset;		/* Offset of reading a 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
Signature · 1 line
2.2.0
#3 lore raw

PATCH v2 2/2 common: Generic firmware loader for file system

Hi,

Quoted 153 lines · On Tue, 12 Dec 2017 15:27:14 +0800 tien.fong.chee at intel.com wrote:

From: Tien Fong Chee <[email protected]>

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.

---
 common/Makefile     |   1 +
 common/fs_loader.c  | 299 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/fs_loader.h |  30 ++++++
 3 files changed, 330 insertions(+)
 create mode 100644 common/fs_loader.c
 create mode 100644 include/fs_loader.h
common/Makefile +1 -0
index cec506f..2934221 100644
@@ -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
common/fs_loader.c new file +122 -0
new file mode 100644
index 0000000..fdb0fa2
@@ -0,0 +1,299 @@
+/*
+ * 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>
+
+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)

common/fs_loader.c line 132

Quoted 10 lines
common/fs_loader.c new file +10 -0
+{
+	/* Expect somewhere already initialize MMC */
+	return 0;
+}
+#endif
+
+static int select_fs_dev(struct device_location *location)
+{
+	int ret = 0;
+

You should not initialize 'ret' here, to make sure, that the compiler barks at you if 'ret' is not appropriately set in each of the if/else clauses below.

common/fs_loader.c line 136

Quoted 5 lines
common/fs_loader.c new file +4 -0
+	if (!strcmp("mmc", location->name)) {
+		ret = fs_set_blk_dev("mmc", location->devpart, FS_TYPE_ANY);
+	}
+	else if (!strcmp("usb", location->name)) {

'else' should be placed on the same line as the '}'.

Quoted 32 lines
common/fs_loader.c new file +31 -0
+		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 = 0;
+#ifndef CONFIG_SPL_BUILD
+	/* USB build is not supported yet in SPL */
+	if (!strcmp("usb", location->name))
+		ret = init_usb();
+#endif
+

common/fs_loader.c line 177

Quoted 10 lines
common/fs_loader.c new file +10 -0
+	if (!strcmp("mmc", location->name))
+		ret = init_mmc();
+
+	if (!strcmp("sata", location->name))
+		ret = init_storage_sata();
+
+	if (location->ubivol != NULL)
+		ret = mount_ubifs(location);
+
+	return ret;

Is it intended to return 0 (success) here, if none of the if clauses is true (no supported storage device is available)?

common/fs_loader.c line 189

Quoted 13 lines
common/fs_loader.c new file +12 -0
+}
+
+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;
+	}
+
+	return;

useless return statement.

common/fs_loader.c line 202

Quoted 13 lines
common/fs_loader.c new file +13 -0
+}
+
+/*
+ * 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 = NULL;
+	int ret = 0;
+

'ret' is not required any more.

Quoted 13 lines
common/fs_loader.c new file +12 -0
+	*firmware_p = NULL;
+
+	if (!name || name[0] == '\0')
+		return -EINVAL;
+
+	*firmware_p = firmware = calloc(1, sizeof(*firmware));
+
+	if (!firmware) {
+		printf("%s: calloc(struct firmware) failed\n", __func__);
+		return -ENOMEM;
+	}
+

common/fs_loader.c line 224

Quoted 10 lines
common/fs_loader.c new file +10 -0
+	firmware->name = name;
+	firmware->data = dbuf;
+	firmware->size = size;
+	firmware->offset = offset;
+
+	return ret;
+}
+
+/*
+ * fw_get_filesystem_firmware - load firmware into a allocated buffer

<nit>'into an allocated'</nit>

Quoted 13 lines
common/fs_loader.c new file +13 -0
+ * @firmware_p: pointer to firmware image
+ * @location: An array of supported firmware location
+ *
+ * @return: size of total read
+ *	    -ve when error
+ */
+static int fw_get_filesystem_firmware(struct device_location *location,
+				      struct firmware *firmware_p)
+{
+	loff_t actread;
+	char *dev_part;
+	int ret;
+

common/fs_loader.c line 247

Quoted 10 lines
common/fs_loader.c new file +10 -0
+	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;
+
+	select_fs_dev(location);
+

select_fs_dev() may fail. You should check its return value here!

common/fs_loader.c line 256

Quoted 10 lines
common/fs_loader.c new file +9 -0
+	ret = fs_read(firmware_p->name, (ulong)firmware_p->data,
+		      firmware_p->offset, firmware_p->size, &actread);
+
+	if (ret || (actread != firmware_p->size)) {
+		printf("Error: %d Failed to read %s from flash %lld ",
+		      ret,
+		      firmware_p->name,
+		      actread);
+		printf("!= %d.\n", firmware_p->size);

You should not split this message into multiple printf's.

common/fs_loader.c line 257

Quoted 2 lines
common/fs_loader.c new file +1 -0
+		return -EPERM;

That's definitely not the right return code in this situation. If 'ret' is != 0 you should return 'ret', otherwise EIO is more appropriate here.

common/fs_loader.c line 260

Quoted 4 lines
common/fs_loader.c new file +3 -0
+		} else {
+			ret = actread;
+		}

broken indentation!

Quoted 51 lines
common/fs_loader.c new file +39 -0
+
+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
+ * @buf: address of buffer to load firmware into
+ * @size: size of buffer
+ * @offset: offset of a file for start reading
+ *
+ * This 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;
+}
include/fs_loader.h new file +5 -0
new file mode 100644
index 0000000..dacc1f3
@@ -0,0 +1,30 @@
+/*
+ * Copyright (C) 2017 Intel Corporation <www.intel.com>
+ *
+ * SPDX-License-Identifier:    GPL-2.0
+ */

include/fs_loader.h line 15

Quoted 10 lines
include/fs_loader.h new file +10 -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 */
+	const char *name;	/* Filename */
+	u32 offset;		/* Offset of reading a file */

IMO the last two members would belong into a private data structure of the firmware loader pointed to by the priv field below.

include/fs_loader.h line 17

Quoted 2 lines
include/fs_loader.h new file +2 -0
+	void *priv;		/* Firmware loader private fields */
+};

Lothar Waßmann

Signature · 9 lines
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________
#4 lore raw

PATCH v2 2/2 common: Generic firmware loader for file system

Quoted 165 lines · Lothar Waßmann

Hi,

Quoted 162 lines · On Tue, 12 Dec 2017 15:27:14 +0800 tien.fong.chee at intel.com wrote:

From: Tien Fong Chee <[email protected]>

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.

---
 common/Makefile     |   1 +
 common/fs_loader.c  | 299
++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/fs_loader.h |  30 ++++++

 3 files changed, 330 insertions(+)

 create mode 100644 common/fs_loader.c
 create mode 100644 include/fs_loader.h

common/Makefile +0 -0
index cec506f..2934221 100644
@@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o

 obj-y += command.o
 obj-y += s_record.o
 obj-y += xyzModem.o

common/Makefile +1 -0
+obj-y += fs_loader.o
common/fs_loader.c new file +54 -0
new file mode 100644
index 0000000..fdb0fa2
@@ -0,0 +1,299 @@
+/*
+ * 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>
+
+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");

common/fs_loader.c new file +61 -0
+	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);

common/fs_loader.c new file +12 -0
+		return err;
+	}
+
+	return err;
+}
+#else
+static int init_mmc(void)
+{
+	/* Expect somewhere already initialize MMC */
+	return 0;
+}
+#endif
Quoted 9 lines

common/fs_loader.c line 132

Quoted 5 lines
common/fs_loader.c new file +5 -0
+
+static int select_fs_dev(struct device_location *location)
+{
+	int ret = 0;
+

You should not initialize 'ret' here, to make sure, that the compiler barks at you if 'ret' is not appropriately set in each of the if/else clauses below.

Okay.

Quoted 9 lines

common/fs_loader.c

Quoted 7 lines
common/fs_loader.c new file +2 -0
+	if (!strcmp("mmc", location->name)) {
+		ret = fs_set_blk_dev("mmc", location->devpart,

FS_TYPE_ANY);

common/fs_loader.c new file +2 -0
+	}
+	else if (!strcmp("usb", location->name)) {

'else' should be placed on the same line as the '}'.

Okay.

Quoted 41 lines
Quoted 41 lines
common/fs_loader.c new file +1 -0
+		ret = fs_set_blk_dev("usb", location->devpart,

FS_TYPE_ANY);

common/fs_loader.c new file +3 -0
+	}
+	else if (!strcmp("sata", location->name)) {
+		ret = fs_set_blk_dev("sata", location->devpart,

FS_TYPE_ANY);

common/fs_loader.c new file +4 -0
+	}
+	else if (!strcmp("ubi", location->name)) {
+		if (location->ubivol != NULL)
+			ret = fs_set_blk_dev("ubi", NULL,

FS_TYPE_UBIFS);

common/fs_loader.c new file +29 -0
+		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 = 0;
+#ifndef CONFIG_SPL_BUILD
+	/* USB build is not supported yet in SPL */
+	if (!strcmp("usb", location->name))
+		ret = init_usb();
+#endif
+
+	if (!strcmp("mmc", location->name))
+		ret = init_mmc();
+
+	if (!strcmp("sata", location->name))
+		ret = init_storage_sata();
+
Quoted 9 lines

common/fs_loader.c line 177

Quoted 5 lines
common/fs_loader.c new file +4 -0
+	if (location->ubivol != NULL)
+		ret = mount_ubifs(location);
+
+	return ret;

Is it intended to return 0 (success) here, if none of the if clauses is
true (no supported storage device is available)?

Okay, i will add error return if there is no supported storage device is available.

Quoted 16 lines
Quoted 4 lines
common/fs_loader.c new file +2 -0
+}
+

common/fs_loader.c line 189

Quoted 10 lines
common/fs_loader.c new file +10 -0
+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;
+	}
+
+	return;

useless return statement.

Okay.

Quoted 16 lines
Quoted 4 lines
common/fs_loader.c new file +3 -0
+}
+
+/*

common/fs_loader.c line 202

Quoted 10 lines
common/fs_loader.c new file +10 -0
+ * 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 = NULL;
+	int ret = 0;
+

'ret' is not required any more.

okay.

Quoted 19 lines
Quoted 19 lines
common/fs_loader.c new file +9 -0
+	*firmware_p = NULL;
+
+	if (!name || name[0] == '\0')
+		return -EINVAL;
+
+	*firmware_p = firmware = calloc(1, sizeof(*firmware));
+
+	if (!firmware) {
+		printf("%s: calloc(struct firmware) failed\n",

__func__);

common/fs_loader.c new file +8 -0
+		return -ENOMEM;
+	}
+
+	firmware->name = name;
+	firmware->data = dbuf;
+	firmware->size = size;
+	firmware->offset = offset;
+
Quoted 9 lines
Quoted 7 lines
common/fs_loader.c new file +5 -0
+	return ret;
+}
+
+/*
+ * fw_get_filesystem_firmware - load firmware into a allocated

buffer

<nit>'into an allocated'</nit>

okay.

Quoted 18 lines
Quoted 18 lines

+ * @firmware_p: pointer to firmware image
+ * @location: An array of supported firmware location
+ *
+ * @return: size of total read
+ *     -ve when error
+ */
+static int fw_get_filesystem_firmware(struct device_location *location,

common/fs_loader.c new file +9 -0
+				      struct firmware *firmware_p)
+{
+	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);
Quoted 9 lines

common/fs_loader.c line 247

Quoted 7 lines
common/fs_loader.c new file +7 -0
+
+	ret = init_storage_device(location);
+	if (ret)
+		goto out;
+
+	select_fs_dev(location);
+

select_fs_dev() may fail. You should check its return value here!

okay.

Quoted 15 lines

common/fs_loader.c line 256

Quoted 13 lines
common/fs_loader.c new file +2 -0
+	ret = fs_read(firmware_p->name, (ulong)firmware_p->data,
+		      firmware_p->offset, firmware_p->size,

&actread);

common/fs_loader.c new file +3 -0
+
+	if (ret || (actread != firmware_p->size)) {
+		printf("Error: %d Failed to read %s from flash

%lld ",

common/fs_loader.c new file +4 -0
+		      ret,
+		      firmware_p->name,
+		      actread);
+		printf("!= %d.\n", firmware_p->size);

You should not split this message into multiple printf's.

okay.

Quoted 8 lines

common/fs_loader.c line 257

Quoted 3 lines
common/fs_loader.c new file +1 -0
+		return -EPERM;

That's definitely not the right return code in this situation. If 'ret' is != 0 you should return 'ret', otherwise EIO is more appropriate here.

Quoted 1 line
Quoted 6 lines

common/fs_loader.c line 260

Quoted 4 lines
common/fs_loader.c new file +3 -0
+		} else {
+			ret = actread;
+		}

broken indentation!

okay.

Quoted 61 lines
Quoted 61 lines
common/fs_loader.c new file +12 -0
+
+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
+ * @buf: address of buffer to load firmware into
+ * @size: size of buffer
+ * @offset: offset of a file for start reading
+ *
+ * This 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
+ */

common/fs_loader.c new file +8 -0
+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);

common/fs_loader.c new file +7 -0
+	if (ret < 0) /* error */
+		return ret;
+
+	ret = fw_get_filesystem_firmware(location, *firmware_p);
+
+	return ret;
+}
include/fs_loader.h new file +11 -0
new file mode 100644
index 0000000..dacc1f3
@@ -0,0 +1,30 @@
+/*
+ * 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 {
Quoted 8 lines

include/fs_loader.h line 15

Quoted 5 lines
include/fs_loader.h new file +4 -0
+	size_t size;		/* Size of a file */
+	const u8 *data;		/* Buffer for file */
+	const char *name;	/* Filename */
+	u32 offset;		/* Offset of reading a file */

IMO the last two members would belong into a private data structure of
the firmware loader pointed to by the priv field below.

okay.

Quoted 6 lines
Quoted 3 lines
include/fs_loader.h new file +1 -0
+	void *priv;		/* Firmware loader private

fields */
+};

Lothar Waßmann