Add FPGA driver, SDRAM driver, generic firmware loader and booting U-Boot.

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.

Signed-off-by Tien Fong Chee <[email protected]>

---
 common/Makefile     |   1 +
 common/fs_loader.c  | 304 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/fs_loader.h |  30 ++++++
 3 files changed, 335 insertions(+)
 create mode 100644 common/fs_loader.c
 create mode 100644 include/fs_loader.h

common/Makefile +1 -0

index 801ea31..419e915 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 +29 -0

new file mode 100644
index 0000000..04f682b
@@ -0,0 +1,304 @@
+/*
+ * 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",

common/fs_loader.c new file +24 -0

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

common/fs_loader.c new file +6 -0

+}
+
+static int umount_ubifs(void)
+{
+	return run_command("ubifsumount", 0);
+}

common/fs_loader.c new file +13 -0

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

common/fs_loader.c new file +6 -0

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

common/fs_loader.c new file +25 -0

+	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();
+
+	if (location->ubivol != NULL)
+		ret = mount_ubifs(location);
+
+	return ret;

common/fs_loader.c new file +11 -0

+		if (!strcmp(default_locations[i].name, name))
+			default_locations[i].devpart = devpart;
+	}
+
+	return;
+}
+
+/*
+ * Prepare firmware struct;
+ * return -ve if fail.
+ */

common/fs_loader.c new file +20 -0

+	*firmware_p = firmware = calloc(1, sizeof(*firmware));
+
+	if (!firmware) {
+		printf("%s: calloc(struct firmware) failed\n", __func__);
+		return -ENOMEM;
+	}
+
+	firmware->name = name;
+	firmware->data = dbuf;
+	firmware->size = size;
+	firmware->offset = offset;
+
+	return ret;
+}
+
+/*
+ * fw_get_filesystem_firmware - load firmware into a allocated buffer
+ * @firmware_p: pointer to firmware image
+ * @location: An array of supported firmware location
+ *

common/fs_loader.c new file +16 -0

+out:
+	if (location->ubivol != NULL)
+		umount_ubifs();
+
+	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.

common/fs_loader.c new file +6 -0

+		return ret;
+
+	ret = fw_get_filesystem_firmware(location, *firmware_p);
+
+	return ret;
+}

include/fs_loader.h new file +3 -0

new file mode 100644
index 0000000..dacc1f3
@@ -0,0 +1,30 @@
+/*
+ * Copyright (C) 2017 Intel Corporation <www.intel.com>
+ *

include/fs_loader.h new file +7 -0

+	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. */

Hi,

Quoted 66 lines · On Tue,  5 Dec 2017 15:57:57 +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.

Signed-off-by Tien Fong Chee <[email protected]>

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

 3 files changed, 335 insertions(+)

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

common/Makefile +0 -0

index 801ea31..419e915 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 +33 -0

new file mode 100644
index 0000000..04f682b
@@ -0,0 +1,304 @@
+/*
+ * 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",
+	},
+};
+

common/fs_loader.c line 39

Quoted 7 lines

common/fs_loader.c new file +6 -0

+/* USB build is not supported yet in SPL */
+#ifndef CONFIG_SPL_BUILD
+#ifdef CONFIG_USB_STORAGE
+static int init_usb(void)
+{
+	int err = 0;

Useless initialization.

common/fs_loader.c line 43

Quoted 5 lines

common/fs_loader.c new file +3 -0

+	err = usb_init();
+
+	if (err)

Unnecessary blank line.

Quoted 30 lines

common/fs_loader.c new file +12 -0

+		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 +16 -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;
+}

common/fs_loader.c line 77

Quoted 7 lines

common/fs_loader.c new file +6 -0

+#endif
+
+#ifdef CONFIG_CMD_UBIFS
+static int mount_ubifs(struct device_location *location)
+{
+	int ret = 0;

Useless initialization.

common/fs_loader.c line 84

Quoted 8 lines

common/fs_loader.c new file +7 -0

+	char cmd[32];
+
+	sprintf(cmd, "ubi part %s", location->mtdpart);
+
+	ret = run_command(cmd, 0);
+
+	if (ret)

Unnecessary blank line.

Quoted 1 line

common/fs_loader.c line 91

Quoted 8 lines

common/fs_loader.c new file +7 -0

+		return ret;
+
+	sprintf(cmd, "ubifsmount %s", location->ubivol);
+
+	ret = run_command(cmd, 0);
+
+	return res;

s/res/ret/
You obviously never compiled this code with CONFIG_CMD_UBIFS enabled.

Quoted 12 lines

common/fs_loader.c new file +11 -0

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

common/fs_loader.c line 107

Quoted 6 lines

common/fs_loader.c new file +5 -0

+}
+
+static int umount_ubifs(void)
+{
+	printf("Error: Cannot unmount UBIFS: no UBIFS support\n");

This error message is useless, since umount_ubifs() would probably never be called when mount_ubifs() failed.

common/fs_loader.c line 117

Quoted 11 lines

common/fs_loader.c new file +9 -0

+}
+#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 = 0;

Useless initialization.

Quoted 22 lines

common/fs_loader.c new file +7 -0

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

+		return err;
+	}
+
+	return err;
+}
+#else
+static int init_mmc(void)
+{
+	/* Expect somewhere already initialize MMC */
+	return 0;
+}
+#endif
+

common/fs_loader.c line 140

Quoted 4 lines

common/fs_loader.c new file +3 -0

+static int select_fs_dev(struct device_location *location)
+{
+	int ret = 0;

Useless initialization. Omitting the initialization here makes sure, that 'ret' is properly set in each of the below if/else clauses. Otherwise the compiler will complain about an unitialized use of 'ret'.

Quoted 16 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))
+		ret = fs_set_blk_dev("usb", location->devpart,

FS_TYPE_ANY);

common/fs_loader.c new file +2 -0

+	else if (!strcmp("sata", location->name))
+		ret = fs_set_blk_dev("sata", location->devpart,

FS_TYPE_ANY);

common/fs_loader.c new file +3 -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 +2 -0

+		else
+			ret = -ENODEV;

common/fs_loader.c line 157

Quoted 5 lines

common/fs_loader.c new file +5 -0

+	else {
+		printf("Error: unsupported location storage.\n");
+		return -ENODEV;
+	}
+

All if/else clauses should be enclosed in {}.
Refer to Documentation/CodingStyle in the Linux source which also applies to U-Boot.

Quoted 27 lines

common/fs_loader.c new file +25 -0

+	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();
+
+	if (location->ubivol != NULL)
+		ret = mount_ubifs(location);
+
+	return ret;

common/fs_loader.c line 192

Quoted 10 lines

common/fs_loader.c new file +10 -0

+}
+
+static void set_storage_devpart(char *name, char *devpart)
+{
+	int i;
+	u32 size;
+
+	size = ARRAY_SIZE(default_locations);
+
+	for (i = 0; i < size; i++) {

The compiler should complain about a comparison of signed and unsigned
integers.
I would do:

	size_t i;

	for (i = 0; i < ARRAY_SIZE(default_locations); i++) {

[...]

Quoted 16 lines

common/fs_loader.c new file +15 -0

+		if (!strcmp(default_locations[i].name, name))
+			default_locations[i].devpart = devpart;
+	}
+
+	return;
+}
+
+/*
+ * 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)
+{

common/fs_loader.c line 213

Quoted 6 lines

common/fs_loader.c new file +6 -0

+	struct firmware *firmware = NULL;
+	int ret = 0;
+
+	if (!name || name[0] == '\0')
+		ret = -EINVAL;
+

Is it really useful to continue here initializing the 'firmware' struct and returning an error at the end?

Quoted 27 lines

common/fs_loader.c new file +4 -0

+	*firmware_p = firmware = calloc(1, sizeof(*firmware));
+
+	if (!firmware) {
+		printf("%s: calloc(struct firmware) failed\n",

__func__);

common/fs_loader.c new file +13 -0

+		return -ENOMEM;
+	}
+
+	firmware->name = name;
+	firmware->data = dbuf;
+	firmware->size = size;
+	firmware->offset = offset;
+
+	return ret;
+}
+
+/*
+ * fw_get_filesystem_firmware - load firmware into a allocated

buffer
+ * @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

common/fs_loader.c line 243

Quoted 7 lines

*location,

common/fs_loader.c new file +6 -0

+				      struct firmware *firmware_p)
+{
+	loff_t actread;
+	char *dev_part;
+	int ret = 0;
+

Useless initialization.

common/fs_loader.c line 246

Quoted 5 lines

common/fs_loader.c new file +3 -0

+	dev_part = env_get("FW_DEV_PART");
+
+	if (dev_part)

Unnecessary blank line.

common/fs_loader.c line 251

Quoted 6 lines

common/fs_loader.c new file +5 -0

+		set_storage_devpart(location->name, dev_part);
+
+	ret = init_storage_device(location);
+
+	if (ret)

Unnecessary blank line.

Quoted 1 line

common/fs_loader.c line 256

Quoted 6 lines

common/fs_loader.c new file +5 -0

+		goto out;
+
+	select_fs_dev(location);
+
+	ret = fs_read(firmware_p->name, (u32)firmware_p->data,

The second parameter of fs_read is of type 'ulong' NOT 'u32'! This will blow up when compiling for 64bit machines.

Quoted 7 lines

common/fs_loader.c new file +1 -0

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

Quoted 8 lines

common/fs_loader.c new file +8 -0

+		      ret,
+		      firmware_p->name,
+		      actread);
+		printf("!= %d.\n", firmware_p->size);
+		return -EPERM;
+		} else
+			ret = actread;
+

broken indentation.
All clauses of the if/else statement should be enclosed in {}.

Quoted 23 lines

common/fs_loader.c new file +8 -0

+out:
+	if (location->ubivol != NULL)
+		umount_ubifs();
+
+	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 line 294

Quoted 7 lines

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

Useless initialization.

common/fs_loader.c line 298

Quoted 6 lines

common/fs_loader.c new file +1 -0

+	ret = _request_firmware_prepare(firmware_p, name, buf,

size, offset);

common/fs_loader.c new file +2 -0

+
+	if (ret < 0) /* error */

Unnecessary blank line.

Quoted 20 lines

common/fs_loader.c new file +6 -0

+		return ret;
+
+	ret = fw_get_filesystem_firmware(location, *firmware_p);
+
+	return ret;
+}

include/fs_loader.h new file +7 -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/fs_loader.h line 13

Quoted 7 lines

include/fs_loader.h new file +6 -0

+
+#include <linux/types.h>
+
+struct firmware {
+	size_t size;		/* Size of a file */
+	const u8 *data;		/* Buffer for file */

const void *data?

Quoted 11 lines

include/fs_loader.h new file +3 -0

+	const char *name;	/* Filename */
+	u32 offset;		/* Offset of reading a file */
+	void *priv;		/* Firmware loader private

fields */

include/fs_loader.h new file +5 -0

+};
+
+struct device_location {
+	char *name;	/* Such as mmc, usb,and sata. */
+	char *devpart;  /* Use the load command dev:part

conventions */

include/fs_loader.h line 30

Quoted 9 lines

include/fs_loader.h new file +9 -0

+	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

Lothar Waßmann

Quoted 69 lines · Lothar Waßmann

Hi,

Quoted 66 lines · On Tue,  5 Dec 2017 15:57:57 +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.

Signed-off-by Tien Fong Chee <[email protected]>

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

 3 files changed, 335 insertions(+)

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

common/Makefile +0 -0

index 801ea31..419e915 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 +33 -0

new file mode 100644
index 0000000..04f682b
@@ -0,0 +1,304 @@
+/*
+ * 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",
+	},
+};
+

Quoted 9 lines

common/fs_loader.c line 39

Quoted 7 lines

common/fs_loader.c new file +6 -0

+/* USB build is not supported yet in SPL */
+#ifndef CONFIG_SPL_BUILD
+#ifdef CONFIG_USB_STORAGE
+static int init_usb(void)
+{
+	int err = 0;

Useless initialization.

noted.

Quoted 1 line

Quoted 1 line

Quoted 6 lines

common/fs_loader.c line 43

Quoted 4 lines

common/fs_loader.c new file +3 -0

+	err = usb_init();
+
+	if (err)

Unnecessary blank line.

Sorry, i'm not catching you because there is no blank line between "if" and "return"

Quoted 16 lines

Quoted 16 lines

common/fs_loader.c new file +15 -0

+		if (!strcmp(default_locations[i].name, name))
+			default_locations[i].devpart = devpart;
+	}
+
+	return;
+}
+
+/*
+ * 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)
+{

Quoted 9 lines

common/fs_loader.c line 213

Quoted 6 lines

common/fs_loader.c new file +6 -0

+	struct firmware *firmware = NULL;
+	int ret = 0;
+
+	if (!name || name[0] == '\0')
+		ret = -EINVAL;
+

Is it really useful to continue here initializing the 'firmware' struct and returning an error at the end?

I try to keep it very close to Linux firmware loader. When more API ported in from Linux in future, this can be helper function. Anyway, i have no strong opinion, i can move to caller if you guys think that is better.

Hi,

Quoted 94 lines · Tien Fong

Quoted 73 lines · Lothar Waßmann

Hi,

Quoted 68 lines · On Tue,  5 Dec 2017 15:57:57 +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.

Signed-off-by Tien Fong Chee <[email protected]>

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

 3 files changed, 335 insertions(+)

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

common/Makefile +0 -0

index 801ea31..419e915 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 +33 -0

new file mode 100644
index 0000000..04f682b
@@ -0,0 +1,304 @@
+/*
+ * 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",
+	},
+};
+

Quoted 9 lines

common/fs_loader.c line 39

Quoted 7 lines

common/fs_loader.c new file +6 -0

+/* USB build is not supported yet in SPL */
+#ifndef CONFIG_SPL_BUILD
+#ifdef CONFIG_USB_STORAGE
+static int init_usb(void)
+{
+	int err = 0;

Useless initialization.

noted.

Quoted 9 lines

common/fs_loader.c line 43

Quoted 7 lines

common/fs_loader.c new file +3 -0

+	err = usb_init();
+
+	if (err)

Unnecessary blank line.

Quoted 4 lines

Sorry, i'm not catching you because there is no blank line between "if"
and "return"

I meant the blank line between 'err = ...' and 'if (err)'

Quoted 31 lines

Quoted 22 lines

Quoted 22 lines

common/fs_loader.c new file +2 -0

+		if (!strcmp(default_locations[i].name, name))
+			default_locations[i].devpart =

devpart;

common/fs_loader.c new file +10 -0

+	}
+
+	return;
+}
+
+/*
+ * Prepare firmware struct;
+ * return -ve if fail.
+ */
+static int _request_firmware_prepare(struct firmware

**firmware_p,

common/fs_loader.c new file +1 -0

+				     const char *name, void

*dbuf,

common/fs_loader.c new file +2 -0

+				     size_t size, u32 offset)
+{

Quoted 9 lines

common/fs_loader.c line 213

Quoted 6 lines

common/fs_loader.c new file +6 -0

+	struct firmware *firmware = NULL;
+	int ret = 0;
+
+	if (!name || name[0] == '\0')
+		ret = -EINVAL;
+

Is it really useful to continue here initializing the 'firmware' struct and returning an error at the end?

Quoted 6 lines

I try to keep it very close to Linux firmware loader. When more API ported in from Linux in future, this can be helper function. Anyway, i
have no strong opinion, i can move to caller if you guys think that is
better.

The Linux firmware loader has this:

Quoted 1 line

Quoted 4 lines

	if (!name || name[0] == '\0') {
		ret = -EINVAL;
		goto out;
	}

Note the 'goto out' which is missing in your code. If following the Linux code closely, you would have to set *firmware_p
to NULL and exit in this case.

Lothar Waßmann

Quoted 97 lines · Lothar Waßmann

Hi,

Quoted 94 lines · Tien Fong

Quoted 73 lines · Lothar Waßmann

Hi,

Quoted 68 lines · On Tue,  5 Dec 2017 15:57:57 +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.

Signed-off-by Tien Fong Chee <[email protected]>

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

 3 files changed, 335 insertions(+)

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

common/Makefile +0 -0

index 801ea31..419e915 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 +33 -0

new file mode 100644
index 0000000..04f682b
@@ -0,0 +1,304 @@
+/*
+ * 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",
+	},
+};
+

Quoted 9 lines

common/fs_loader.c line 39

Quoted 7 lines

common/fs_loader.c new file +6 -0

+/* USB build is not supported yet in SPL */
+#ifndef CONFIG_SPL_BUILD
+#ifdef CONFIG_USB_STORAGE
+static int init_usb(void)
+{
+	int err = 0;

Useless initialization.

noted.

Quoted 9 lines

common/fs_loader.c line 43

Quoted 7 lines

common/fs_loader.c new file +3 -0

+	err = usb_init();
+
+	if (err)

Unnecessary blank line.

Quoted 6 lines

Quoted 4 lines

Sorry, i'm not catching you because there is no blank line between "if"
and "return"

I meant the blank line between 'err = ...' and 'if (err)'

Okay, i can change that.

Quoted 39 lines

Quoted 31 lines

Quoted 22 lines

Quoted 22 lines

common/fs_loader.c new file +2 -0

+		if (!strcmp(default_locations[i].name, name))
+			default_locations[i].devpart =

devpart;

common/fs_loader.c new file +10 -0

+	}
+
+	return;
+}
+
+/*
+ * Prepare firmware struct;
+ * return -ve if fail.
+ */
+static int _request_firmware_prepare(struct firmware

**firmware_p,

common/fs_loader.c new file +1 -0

+				     const char *name, void

*dbuf,

common/fs_loader.c new file +2 -0

+				     size_t size, u32 offset)
+{

Quoted 9 lines

common/fs_loader.c line 213

Quoted 6 lines

common/fs_loader.c new file +6 -0

+	struct firmware *firmware = NULL;
+	int ret = 0;
+
+	if (!name || name[0] == '\0')
+		ret = -EINVAL;
+

Is it really useful to continue here initializing the 'firmware' struct and returning an error at the end?

Quoted 6 lines

I try to keep it very close to Linux firmware loader. When more API ported in from Linux in future, this can be helper function. Anyway, i
have no strong opinion, i can move to caller if you guys think that is
better.

The Linux firmware loader has this:

Quoted 1 line

Quoted 9 lines

Quoted 4 lines

	if (!name || name[0] == '\0') {
		ret = -EINVAL;
		goto out;
	}

Note the 'goto out' which is missing in your code. If following the Linux code closely, you would have to set *firmware_p
to NULL and exit in this case.

I can set the *firmware_p to NUll in failing case. But, i checked the static int _request_firmware_prepare in Linux, there is no goto out error handling method in the function. Fyi, there is no allocated memory release in U-Boot.
https://elixir.free-electrons.com/linux/v4.13.15/source/drivers/base/fi
rmware_class.c#L1191

Quoted 1 line

Hi,

Quoted 173 lines · Tien Fong

Quoted 104 lines · Lothar Waßmann

Hi,

Quoted 100 lines · Tien Fong

Quoted 76 lines · Lothar Waßmann

Hi,

Quoted 70 lines · On Tue,  5 Dec 2017 15:57:57 +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.

Signed-off-by Tien Fong Chee <[email protected]>

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

 3 files changed, 335 insertions(+)

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

common/Makefile +0 -0

index 801ea31..419e915 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 +33 -0

new file mode 100644
index 0000000..04f682b
@@ -0,0 +1,304 @@
+/*
+ * 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",
+	},
+};
+

Quoted 9 lines

common/fs_loader.c line 39

Quoted 7 lines

common/fs_loader.c new file +6 -0

+/* USB build is not supported yet in SPL */
+#ifndef CONFIG_SPL_BUILD
+#ifdef CONFIG_USB_STORAGE
+static int init_usb(void)
+{
+	int err = 0;

Useless initialization.

noted.

Quoted 11 lines

common/fs_loader.c line 43

Quoted 9 lines

common/fs_loader.c new file +3 -0

+	err = usb_init();
+
+	if (err)

Unnecessary blank line.

Quoted 7 lines

Quoted 5 lines

Sorry, i'm not catching you because there is no blank line between
"if"
and "return"

I meant the blank line between 'err = ...' and 'if (err)'

Okay, i can change that.

Quoted 50 lines

Quoted 39 lines

Quoted 30 lines

Quoted 30 lines

common/fs_loader.c new file +1 -0

+		if (!strcmp(default_locations[i].name,

name))

common/fs_loader.c new file +1 -0

+			default_locations[i].devpart =

devpart;

common/fs_loader.c new file +10 -0

+	}
+
+	return;
+}
+
+/*
+ * Prepare firmware struct;
+ * return -ve if fail.
+ */
+static int _request_firmware_prepare(struct firmware

**firmware_p,

common/fs_loader.c new file +1 -0

+				     const char *name,

void
*dbuf,

common/fs_loader.c new file +1 -0

+				     size_t size, u32

offset)

common/fs_loader.c new file +2 -0

+{
+	struct firmware *firmware = NULL;

Quoted 9 lines

common/fs_loader.c line 213

Quoted 5 lines

common/fs_loader.c new file +5 -0

+	int ret = 0;
+
+	if (!name || name[0] == '\0')
+		ret = -EINVAL;
+

Is it really useful to continue here initializing the 'firmware'
struct and returning an error at the end?

Quoted 8 lines

I try to keep it very close to Linux firmware loader. When more API
ported in from Linux in future, this can be helper function. Anyway, i
have no strong opinion, i can move to caller if you guys think that
is
better.

The Linux firmware loader has this:

Quoted 2 lines

Quoted 9 lines

Quoted 4 lines

	if (!name || name[0] == '\0') {
		ret = -EINVAL;
		goto out;
	}

Note the 'goto out' which is missing in your code. If following the Linux code closely, you would have to set *firmware_p
to NULL and exit in this case.

Quoted 10 lines

I can set the *firmware_p to NUll in failing case. But, i checked the
static int _request_firmware_prepare in Linux, there is no goto out error handling method in the function. Fyi, there is no allocated memory release in U-Boot.
https://elixir.free-electrons.com/linux/v4.13.15/source/drivers/bas
e/fi
rmware_class.c#L1191

Quoted 2 lines

I was referring to _request_firmware() which calls request_firmware_prepare() and does the checking of 'name' as your code does.

Lothar Waßmann

Quoted 186 lines · Lothar Waßmann

Hi,

Quoted 173 lines · Tien Fong

Quoted 104 lines · Lothar Waßmann

Hi,

Quoted 100 lines · Tien Fong

Quoted 76 lines · Lothar Waßmann

Hi,

Quoted 70 lines · On Tue,  5 Dec 2017 15:57:57 +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.

Signed-off-by Tien Fong Chee <[email protected]>

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

 3 files changed, 335 insertions(+)

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

common/Makefile +0 -0

index 801ea31..419e915 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 +33 -0

new file mode 100644
index 0000000..04f682b
@@ -0,0 +1,304 @@
+/*
+ * 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",
+	},
+};
+

Quoted 9 lines

common/fs_loader.c line 39

Quoted 7 lines

common/fs_loader.c new file +6 -0

+/* USB build is not supported yet in SPL */
+#ifndef CONFIG_SPL_BUILD
+#ifdef CONFIG_USB_STORAGE
+static int init_usb(void)
+{
+	int err = 0;

Useless initialization.

noted.

Quoted 11 lines

common/fs_loader.c line 42

Quoted 9 lines

common/fs_loader.c new file +3 -0

+	err = usb_init();
+
+	if (err)

Unnecessary blank line.

Quoted 7 lines

Quoted 5 lines

Sorry, i'm not catching you because there is no blank line between
"if"
and "return"

I meant the blank line between 'err = ...' and 'if (err)'

Okay, i can change that.

Quoted 50 lines

Quoted 39 lines

Quoted 30 lines

Quoted 30 lines

common/fs_loader.c new file +1 -0

+		if (!strcmp(default_locations[i].name,

name))

common/fs_loader.c new file +1 -0

+			default_locations[i].devpart =

devpart;

common/fs_loader.c new file +10 -0

+	}
+
+	return;
+}
+
+/*
+ * Prepare firmware struct;
+ * return -ve if fail.
+ */
+static int _request_firmware_prepare(struct firmware

**firmware_p,

common/fs_loader.c new file +1 -0

+				     const char *name,

void
*dbuf,

common/fs_loader.c new file +1 -0

+				     size_t size, u32

offset)

common/fs_loader.c new file +2 -0

+{
+	struct firmware *firmware = NULL;

Quoted 9 lines

common/fs_loader.c

Quoted 5 lines

common/fs_loader.c new file +5 -0

+	int ret = 0;
+
+	if (!name || name[0] == '\0')
+		ret = -EINVAL;
+

Is it really useful to continue here initializing the 'firmware'
struct and returning an error at the end?

Quoted 8 lines

I try to keep it very close to Linux firmware loader. When more API
ported in from Linux in future, this can be helper function. Anyway, i
have no strong opinion, i can move to caller if you guys think that
is
better.

The Linux firmware loader has this:

Quoted 2 lines

Quoted 9 lines

Quoted 4 lines

	if (!name || name[0] == '\0') {
		ret = -EINVAL;
		goto out;
	}

Note the 'goto out' which is missing in your code. If following the Linux code closely, you would have to set *firmware_p
to NULL and exit in this case.

Quoted 10 lines

I can set the *firmware_p to NUll in failing case. But, i checked the
static int _request_firmware_prepare in Linux, there is no goto out error handling method in the function. Fyi, there is no allocated memory release in U-Boot.
https://elixir.free-electrons.com/linux/v4.13.15/source/drivers/bas
e/fi
rmware_class.c#L1191

Quoted 2 lines

Quoted 4 lines

I was referring to _request_firmware() which calls request_firmware_prepare() and does the checking of 'name' as your code does.

Ohh.....i skipped _request_firmware() because there is no error handling required in U-Boot such as memory release. For the checking at both firmware_p and name, i would rather to merge them into request_firware_prepare() because they are related to each other instead of creating one function just for checking purpose. What do you think?

Hi,

Quoted 219 lines · Tien Fong

Quoted 212 lines · Lothar Waßmann

Hi,

Quoted 193 lines · Tien Fong

Quoted 115 lines · Lothar Waßmann

Hi,

Quoted 110 lines · Tien Fong

Quoted 83 lines · Lothar Waßmann

Hi,

Quoted 75 lines · On Tue,  5 Dec 2017 15:57:57 +0800 tien.fong.chee at intel.c om 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.

Signed-off-by Tien Fong Chee <[email protected]

Quoted 1 line

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

 3 files changed, 335 insertions(+)

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

common/Makefile +0 -0

index 801ea31..419e915 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 +2 -0

new file mode 100644
index 0000000..04f682b
@@ -0,0 +1,304 @@
+/*
+ * Copyright (C) 2017 Intel Corporation <www.intel.com

Quoted 1 line

+ *
+ * SPDX-License-Identifier:    GPL-2.0
+ */

common/fs_loader.c new file +28 -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",
+	},
+};
+

Quoted 9 lines

common/fs_loader.c

Quoted 7 lines

common/fs_loader.c new file +6 -0

+/* USB build is not supported yet in SPL */
+#ifndef CONFIG_SPL_BUILD
+#ifdef CONFIG_USB_STORAGE
+static int init_usb(void)
+{
+	int err = 0;

Useless initialization.

noted.

Quoted 13 lines

common/fs_loader.c

Quoted 11 lines

common/fs_loader.c new file +3 -0

+	err = usb_init();
+
+	if (err)

Unnecessary blank line.

Quoted 7 lines

Quoted 5 lines

Sorry, i'm not catching you because there is no blank line between
"if"
and "return"

I meant the blank line between 'err = ...' and 'if (err)'

Okay, i can change that.

Quoted 58 lines

Quoted 43 lines

Quoted 34 lines

Quoted 34 lines

common/fs_loader.c new file +1 -0

+		if (!strcmp(default_locations[i].name,

name))

common/fs_loader.c new file +1 -0

+			default_locations[i].devpart =

devpart;

common/fs_loader.c new file +10 -0

+	}
+
+	return;
+}
+
+/*
+ * Prepare firmware struct;
+ * return -ve if fail.
+ */
+static int _request_firmware_prepare(struct firmware

**firmware_p,

common/fs_loader.c new file +1 -0

+				     const char *name,

void
*dbuf,

common/fs_loader.c new file +1 -0

+				     size_t size, u32

offset)

common/fs_loader.c new file +2 -0

+{
+	struct firmware *firmware = NULL;

Quoted 9 lines

common/fs_loader.c

Quoted 5 lines

common/fs_loader.c new file +5 -0

+	int ret = 0;
+
+	if (!name || name[0] == '\0')
+		ret = -EINVAL;
+

Is it really useful to continue here initializing the 'firmware'
struct and returning an error at the end?

Quoted 11 lines

I try to keep it very close to Linux firmware loader. When more
API
ported in from Linux in future, this can be helper function.
Anyway, i
have no strong opinion, i can move to caller if you guys think
that
is
better.

The Linux firmware loader has this:

Quoted 3 lines

Quoted 9 lines

Quoted 4 lines

	if (!name || name[0] == '\0') {
		ret = -EINVAL;
		goto out;
	}

Note the 'goto out' which is missing in your code. If following the Linux code closely, you would have to set *firmware_p
to NULL and exit in this case.

Quoted 15 lines

I can set the *firmware_p to NUll in failing case. But, i checked
the
static int _request_firmware_prepare in Linux, there is no goto out
error handling method in the function. Fyi, there is no allocated
memory release in U-Boot.
https://elixir.free-electrons.com/linux/v4.13.15/source/drivers
/bas
e/fi
rmware_class.c#L1191

Quoted 3 lines

Quoted 5 lines

I was referring to _request_firmware() which calls request_firmware_prepare() and does the checking of 'name' as your
code does.

Quoted 9 lines

Ohh.....i skipped _request_firmware() because there is no error handling required in U-Boot such as memory release. For the checking at
both firmware_p and name, i would rather to merge them into request_firware_prepare() because they are related to each other instead of creating one function just for checking purpose. What do you
think?

I don't mind whether you combine those functions or not as long as you
reproduce the same functionality which your patch currently does not do.

Lothar Waßmann

Quoted 231 lines · Lothar Waßmann

Hi,

Quoted 228 lines · Tien Fong

Quoted 212 lines · Lothar Waßmann

Hi,

Quoted 193 lines · Tien Fong

Quoted 115 lines · Lothar Waßmann

Hi,

Quoted 110 lines · Tien Fong

Quoted 83 lines · Lothar Waßmann

Hi,

Quoted 75 lines · On Tue,  5 Dec 2017 15:57:57 +0800 tien.fong.chee at intel.c om 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.

Signed-off-by Tien Fong Chee <[email protected]

Quoted 1 line

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

 3 files changed, 335 insertions(+)

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

common/Makefile +0 -0

index 801ea31..419e915 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 +2 -0

new file mode 100644
index 0000000..04f682b
@@ -0,0 +1,304 @@
+/*
+ * Copyright (C) 2017 Intel Corporation <www.intel.com

Quoted 1 line

+ *
+ * SPDX-License-Identifier:    GPL-2.0
+ */

common/fs_loader.c new file +28 -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",
+	},
+};
+

Quoted 9 lines

common/fs_loader.c

Quoted 7 lines

common/fs_loader.c new file +6 -0

+/* USB build is not supported yet in SPL */
+#ifndef CONFIG_SPL_BUILD
+#ifdef CONFIG_USB_STORAGE
+static int init_usb(void)
+{
+	int err = 0;

Useless initialization.

noted.

Quoted 13 lines

common/fs_loader.c

Quoted 11 lines

common/fs_loader.c new file +3 -0

+	err = usb_init();
+
+	if (err)

Unnecessary blank line.

Quoted 7 lines

Quoted 5 lines

Sorry, i'm not catching you because there is no blank line between
"if"
and "return"

I meant the blank line between 'err = ...' and 'if (err)'

Okay, i can change that.

Quoted 58 lines

Quoted 43 lines

Quoted 34 lines

Quoted 34 lines

common/fs_loader.c new file +1 -0

+		if (!strcmp(default_locations[i].name,

name))

common/fs_loader.c new file +1 -0

+			default_locations[i].devpart =

devpart;

common/fs_loader.c new file +10 -0

+	}
+
+	return;
+}
+
+/*
+ * Prepare firmware struct;
+ * return -ve if fail.
+ */
+static int _request_firmware_prepare(struct firmware

**firmware_p,

common/fs_loader.c new file +1 -0

+				     const char *name,

void
*dbuf,

common/fs_loader.c new file +1 -0

+				     size_t size, u32

offset)

common/fs_loader.c new file +2 -0

+{
+	struct firmware *firmware = NULL;

Quoted 9 lines

common/fs_loader.c

Quoted 5 lines

common/fs_loader.c new file +5 -0

+	int ret = 0;
+
+	if (!name || name[0] == '\0')
+		ret = -EINVAL;
+

Is it really useful to continue here initializing the 'firmware'
struct and returning an error at the end?

Quoted 11 lines

I try to keep it very close to Linux firmware loader. When more
API
ported in from Linux in future, this can be helper function.
Anyway, i
have no strong opinion, i can move to caller if you guys think
that
is
better.

The Linux firmware loader has this:

Quoted 3 lines

Quoted 9 lines

Quoted 4 lines

	if (!name || name[0] == '\0') {
		ret = -EINVAL;
		goto out;
	}

Note the 'goto out' which is missing in your code. If following the Linux code closely, you would have to set *firmware_p
to NULL and exit in this case.

Quoted 15 lines

I can set the *firmware_p to NUll in failing case. But, i checked
the
static int _request_firmware_prepare in Linux, there is no goto out
error handling method in the function. Fyi, there is no allocated
memory release in U-Boot.
https://elixir.free-electrons.com/linux/v4.13.15/source/drivers
/bas
e/fi
rmware_class.c#L1191

Quoted 3 lines

Quoted 5 lines

I was referring to _request_firmware() which calls request_firmware_prepare() and does the checking of 'name' as your
code does.

Ohh.....i skipped _request_firmware() because there is no error handling required in U-Boot such as memory release. For the checking at
both firmware_p and name, i would rather to merge them into request_firware_prepare() because they are related to each other instead of creating one function just for checking purpose. What do you
think?

Quoted 5 lines

I don't mind whether you combine those functions or not as long as you
reproduce the same functionality which your patch currently does not do.

Okay. SO i will add the checking on firmware_p whether is NULL, and assign NULL to *firmware_P if calloc is failed. There is no goout since there is no error handling required.

Quoted 1 line