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

[U-Boot] [PATCH v4 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, Lukasz Majewski and Prabhakar Kushwaha in [v3].

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

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

v3 -> v4 changes:
-----------------
- Moved the firmware loader private structure declaration to source file.
- Added README.firmware_loader to doc directory.

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

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         | 311 +++++++++++++++++++++++++++++++++++++++++++++
 common/spl/spl_mmc.c       |   2 +-
 doc/README.firmware_loader |  77 +++++++++++
 include/fs_loader.h        |  28 ++++
 include/spl.h              |   2 +
 6 files changed, 420 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 v4 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] [PATCH v4 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         | 311 +++++++++++++++++++++++++++++++++++++++++++++
 doc/README.firmware_loader |  77 +++++++++++
 include/fs_loader.h        |  28 ++++
 4 files changed, 417 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..81cf5d6
--- /dev/null
+++ b/common/fs_loader.c
@@ -0,0 +1,311 @@
+/*
+ * 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 = NULL;
+	struct firmware_priv *fw_priv = NULL;
+
+	*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;
+	}
+
+	fw_priv = calloc(1, sizeof(*fw_priv));
+
+	if (!fw_priv) {
+		printf("%s: calloc(struct fw_priv) failed\n", __func__);
+		return -ENOMEM;
+	}
+
+	fw_priv->name = name;
+	fw_priv->offset = offset;
+	firmware->data = dbuf;
+	firmware->size = size;
+	firmware->priv = fw_priv;
+
+	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;
+
+	select_fs_dev(location);
+	if (ret)
+		goto out;
+
+	fw_priv = (struct firmware_priv *)firmware_p->priv;
+
+	ret = fs_read(fw_priv->name, (ulong)firmware_p->data, fw_priv->offset,
+		     firmware_p->size, &actread);
+
+	if (ret || (actread != firmware_p->size)) {
+		printf("Error: %d Failed to read %s from flash %lld != %d.\n",
+		      ret, fw_priv->name, actread, firmware_p->size);
+		return -EPERM;
+	} 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..d250723
--- /dev/null
+++ b/doc/README.firmware_loader
@@ -0,0 +1,77 @@
+/*
+ * 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] [PATCH v4 2/2] common: Generic firmware loader for file system

[Lothar Waßmann]

Hi,

On Mon, 18 Dec 2017 13:10:56 +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>
> ---
>  common/Makefile            |   1 +
>  common/fs_loader.c         | 311 +++++++++++++++++++++++++++++++++++++++++++++
>  doc/README.firmware_loader |  77 +++++++++++
>  include/fs_loader.h        |  28 ++++
>  4 files changed, 417 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/fs_loader.c b/common/fs_loader.c
> new file mode 100644
> index 0000000..81cf5d6
> --- /dev/null
> +++ b/common/fs_loader.c
[...]
> +/*
> + * 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;
> +	struct firmware_priv *fw_priv = NULL;
> +
> +	*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;
> +	}
> +
> +	fw_priv = calloc(1, sizeof(*fw_priv));
> +
> +	if (!fw_priv) {
> +		printf("%s: calloc(struct fw_priv) failed\n", __func__);
> +		return -ENOMEM;
What about freeing 'firmware' and NULLing *firmware_p here?
Or better, do the assignment of *firmware_p at the end.

> +	}
> +
> +	fw_priv->name = name;
> +	fw_priv->offset = offset;
> +	firmware->data = dbuf;
> +	firmware->size = size;
> +	firmware->priv = fw_priv;
> +
> +	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;
> +
> +	select_fs_dev(location);
> +	if (ret)
> +		goto out;
> +
> +	fw_priv = (struct firmware_priv *)firmware_p->priv;
> +
useless type cast.

> +	ret = fs_read(fw_priv->name, (ulong)firmware_p->data, fw_priv->offset,
> +		     firmware_p->size, &actread);
> +
> +	if (ret || (actread != firmware_p->size)) {
> +		printf("Error: %d Failed to read %s from flash %lld != %d.\n",
> +		      ret, fw_priv->name, actread, firmware_p->size);
> +		return -EPERM;
Quoting myself from an earlier mail
(20171212091442.2f6826fe at karo-electronics.de):
|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.



Lothar Waßmann

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

[Chee, Tien Fong]

On Isn, 2017-12-18 at 08:39 +0100, Lothar Waßmann wrote:
> Hi,
> 
> On Mon, 18 Dec 2017 13:10:56 +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>
> > ---
> >  common/Makefile            |   1 +
> >  common/fs_loader.c         | 311
> > +++++++++++++++++++++++++++++++++++++++++++++
> >  doc/README.firmware_loader |  77 +++++++++++
> >  include/fs_loader.h        |  28 ++++
> >  4 files changed, 417 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/fs_loader.c b/common/fs_loader.c
> > new file mode 100644
> > index 0000000..81cf5d6
> > --- /dev/null
> > +++ b/common/fs_loader.c
> [...]
> > 
> > +/*
> > + * 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;
> > +	struct firmware_priv *fw_priv = NULL;
> > +
> > +	*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;
> > +	}
> > +
> > +	fw_priv = calloc(1, sizeof(*fw_priv));
> > +
> > +	if (!fw_priv) {
> > +		printf("%s: calloc(struct fw_priv) failed\n",
> > __func__);
> > +		return -ENOMEM;
> What about freeing 'firmware' and NULLing *firmware_p here?
There is no "freeing" support in U-Boot. I can assign NULL
to *firmware_p.
> Or better, do the assignment of *firmware_p at the end.
Are you means switch the location between *firmware_p and fw_priv in
calloc?
> 
> > 
> > +	}
> > +
> > +	fw_priv->name = name;
> > +	fw_priv->offset = offset;
> > +	firmware->data = dbuf;
> > +	firmware->size = size;
> > +	firmware->priv = fw_priv;
> > +
> > +	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;
> > +
> > +	select_fs_dev(location);
> > +	if (ret)
> > +		goto out;
> > +
> > +	fw_priv = (struct firmware_priv *)firmware_p->priv;
> > +
> useless type cast.
> 
I assume you are saying autocast, right? Let me check is there any
warning from compiler after removing the cast.
> > 
> > +	ret = fs_read(fw_priv->name, (ulong)firmware_p->data,
> > fw_priv->offset,
> > +		     firmware_p->size, &actread);
> > +
> > +	if (ret || (actread != firmware_p->size)) {
> > +		printf("Error: %d Failed to read %s from flash
> > %lld != %d.\n",
> > +		      ret, fw_priv->name, actread, firmware_p-
> > >size);
> > +		return -EPERM;
> Quoting myself from an earlier mail
> (20171212091442.2f6826fe at karo-electronics.de):
> > 
> > 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.
> 
Sorry for mising out this part. I would change that.
> 
> Lothar Waßmann

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

[Marek Vasut]

On 12/19/2017 11:31 AM, Chee, Tien Fong wrote:
> On Isn, 2017-12-18 at 08:39 +0100, Lothar Waßmann wrote:
>> Hi,
>>
>> On Mon, 18 Dec 2017 13:10:56 +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>
>>> ---
>>>  common/Makefile            |   1 +
>>>  common/fs_loader.c         | 311
>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>  doc/README.firmware_loader |  77 +++++++++++
>>>  include/fs_loader.h        |  28 ++++
>>>  4 files changed, 417 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/fs_loader.c b/common/fs_loader.c
>>> new file mode 100644
>>> index 0000000..81cf5d6
>>> --- /dev/null
>>> +++ b/common/fs_loader.c
>> [...]
>>>
>>> +/*
>>> + * 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;
>>> +	struct firmware_priv *fw_priv = NULL;
>>> +
>>> +	*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;
>>> +	}
>>> +
>>> +	fw_priv = calloc(1, sizeof(*fw_priv));
>>> +
>>> +	if (!fw_priv) {
>>> +		printf("%s: calloc(struct fw_priv) failed\n",
>>> __func__);
>>> +		return -ENOMEM;
>> What about freeing 'firmware' and NULLing *firmware_p here?
> There is no "freeing" support in U-Boot. I can assign NULL
> to *firmware_p.

Try git grep free maybe ?

>> Or better, do the assignment of *firmware_p at the end.
> Are you means switch the location between *firmware_p and fw_priv in
> calloc?

[...

-- 
Best regards,
Marek Vasut

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

[Lothar Waßmann]

Hi,

On Tue, 19 Dec 2017 10:31:13 +0000 Chee, Tien Fong wrote:
> On Isn, 2017-12-18 at 08:39 +0100, Lothar Waßmann wrote:
> > Hi,
> > 
> > On Mon, 18 Dec 2017 13:10:56 +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>
> > > ---
> > >  common/Makefile            |   1 +
> > >  common/fs_loader.c         | 311
> > > +++++++++++++++++++++++++++++++++++++++++++++
> > >  doc/README.firmware_loader |  77 +++++++++++
> > >  include/fs_loader.h        |  28 ++++
> > >  4 files changed, 417 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/fs_loader.c b/common/fs_loader.c
> > > new file mode 100644
> > > index 0000000..81cf5d6
> > > --- /dev/null
> > > +++ b/common/fs_loader.c
> > [...]
> > > 
> > > +/*
> > > + * 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;
> > > +	struct firmware_priv *fw_priv = NULL;
> > > +
> > > +	*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;
> > > +	}
> > > +
> > > +	fw_priv = calloc(1, sizeof(*fw_priv));
> > > +
> > > +	if (!fw_priv) {
> > > +		printf("%s: calloc(struct fw_priv) failed\n",
> > > __func__);
> > > +		return -ENOMEM;
> > What about freeing 'firmware' and NULLing *firmware_p here?
> There is no "freeing" support in U-Boot. I can assign NULL
>
How do you come to that conclusion?

> to *firmware_p.
> > Or better, do the assignment of *firmware_p at the end.
> Are you means switch the location between *firmware_p and fw_priv in
> calloc?
>
No. I would assign *firmware_p only when everything else was OK, so
that there won't be a valid pointer in *firmware_p when the struct
firmware it is pointing to has not been set up completely.

I would do like this:
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;
}
so that *firmware_p is only assigned to when everything was OK.
(Note, that there is no need to initialize firmware and fw_priv to NULL)

> > > + * 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;
> > > +
> > > +	select_fs_dev(location);
> > > +	if (ret)
> > > +		goto out;
> > > +
> > > +	fw_priv = (struct firmware_priv *)firmware_p->priv;
> > > +
> > useless type cast.
> > 
> I assume you are saying autocast, right? Let me check is there any
> warning from compiler after removing the cast.
>
In C void pointers can be assigned to any type pointers without need for
a cast.


Lothar Waßmann

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

[Chee, Tien Fong]

On Sel, 2017-12-19 at 12:21 +0100, Lothar Waßmann wrote:
> Hi,
> 
> On Tue, 19 Dec 2017 10:31:13 +0000 Chee, Tien Fong wrote:
> > 
> > On Isn, 2017-12-18 at 08:39 +0100, Lothar Waßmann wrote:
> > > 
> > > Hi,
> > > 
> > > On Mon, 18 Dec 2017 13:10:56 +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>
> > > > ---
> > > >  common/Makefile            |   1 +
> > > >  common/fs_loader.c         | 311
> > > > +++++++++++++++++++++++++++++++++++++++++++++
> > > >  doc/README.firmware_loader |  77 +++++++++++
> > > >  include/fs_loader.h        |  28 ++++
> > > >  4 files changed, 417 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/fs_loader.c b/common/fs_loader.c
> > > > new file mode 100644
> > > > index 0000000..81cf5d6
> > > > --- /dev/null
> > > > +++ b/common/fs_loader.c
> > > [...]
> > > > 
> > > > 
> > > > +/*
> > > > + * 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;
> > > > +	struct firmware_priv *fw_priv = NULL;
> > > > +
> > > > +	*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;
> > > > +	}
> > > > +
> > > > +	fw_priv = calloc(1, sizeof(*fw_priv));
> > > > +
> > > > +	if (!fw_priv) {
> > > > +		printf("%s: calloc(struct fw_priv) failed\n",
> > > > __func__);
> > > > +		return -ENOMEM;
> > > What about freeing 'firmware' and NULLing *firmware_p here?
> > There is no "freeing" support in U-Boot. I can assign NULL
> > 
> How do you come to that conclusion?
> 
Sorry for misleading because i was tracking the free function until i
saw the function direct return when the bit GD_FLG_FULL_MALLOC_INIT was
found in gd->flags in beginning of the function long time ago. So, i
always assume that memory will always be freed in relocation on most
cases.
I will put the free to the firmware in next release.
> > 
> > to *firmware_p.
> > > 
> > > Or better, do the assignment of *firmware_p at the end.
> > Are you means switch the location between *firmware_p and fw_priv
> > in
> > calloc?
> > 
> No. I would assign *firmware_p only when everything else was OK, so
> that there won't be a valid pointer in *firmware_p when the struct
> firmware it is pointing to has not been set up completely.
> 
> I would do like this:
> 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;
> }
> so that *firmware_p is only assigned to when everything was OK.
> (Note, that there is no need to initialize firmware and fw_priv to
> NULL)
> 
Okay.
> > 
> > > 
> > > > 
> > > > + * 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;
> > > > +
> > > > +	select_fs_dev(location);
> > > > +	if (ret)
> > > > +		goto out;
> > > > +
> > > > +	fw_priv = (struct firmware_priv *)firmware_p->priv;
> > > > +
> > > useless type cast.
> > > 
> > I assume you are saying autocast, right? Let me check is there any
> > warning from compiler after removing the cast.
> > 
> In C void pointers can be assigned to any type pointers without need
> for
> a cast.
> 
Oo....initially i worry the compiler would trigger warnings when the
fw_priv was pass as parameter to fs_read function without casting.
However, i have ran the check, and there is no warning from compiler
without casting. So, i will remove the cast.
> 
> Lothar Waßmann