Generic firmware loader

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.

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

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

new file mode 100644
index 0000000..56d29b6
@@ -0,0 +1,309 @@
+/*
+ * Copyright (C) 2017 Intel Corporation <www.intel.com>
+ *
+ * SPDX-License-Identifier:    GPL-2.0
+ */

Quoted 8 lines · 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.

Quoted 1 line

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

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

[snip]

Quoted 10 lines

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
+obj-y += fs_loader.o

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

Quoted 19 lines

common/fs_loader.c new file +12 -0

new file mode 100644
index 0000000..56d29b6
@@ -0,0 +1,309 @@
+/*
+ * Copyright (C) 2017 Intel Corporation <www.intel.com>
+ *
+ * SPDX-License-Identifier:    GPL-2.0
+ */
+
+#include <common.h>
+#include <errno.h>
+#include <fs.h>
+#include <fs_loader.h>
+#include <nand.h>
+#include <sata.h>

common/fs_loader.c line 15

Quoted 3 lines

common/fs_loader.c new file +3 -0

+#include <spi.h>
+#include <spi_flash.h>
+#include <spl.h>

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

[snip]

Quoted 6 lines

common/fs_loader.c new file +2 -0

+	if (ret) {
+		printf("Error: %d Failed to read %s from flash

%lld != %d.\n",

common/fs_loader.c new file +1 -0

+		      ret, fw_priv->name, actread, firmware_p-

Quoted 1 line

size);

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

Thanks!

Quoted 11 lines · Tom Rini

Quoted 8 lines · 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.

Quoted 6 lines

Quoted 1 line

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

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

I have ran the checkpatch.pl on this patch, i didn't see any error.

Quoted 18 lines

Quoted 18 lines

common/fs_loader.c new file +12 -0

new file mode 100644
index 0000000..56d29b6
@@ -0,0 +1,309 @@
+/*
+ * Copyright (C) 2017 Intel Corporation <www.intel.com>
+ *
+ * SPDX-License-Identifier:    GPL-2.0
+ */
+
+#include <common.h>
+#include <errno.h>
+#include <fs.h>
+#include <fs_loader.h>
+#include <nand.h>
+#include <sata.h>

Quoted 9 lines

common/fs_loader.c line 15

Quoted 3 lines

common/fs_loader.c new file +3 -0

+#include <spi.h>
+#include <spi_flash.h>
+#include <spl.h>

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

I can enable the Kconfig, and put the caution about dependency on <asm/spl.h> in document.

Quoted 24 lines · Tien Fong

Quoted 22 lines · Tom Rini

Quoted 11 lines · tien.fong.chee at intel.co m

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

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

Quoted 1 line

I have ran the checkpatch.pl on this patch, i didn't see any error.

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

[snip]

Quoted 32 lines

Quoted 32 lines

Quoted 14 lines

common/fs_loader.c new file +5 -0

new file mode 100644
index 0000000..56d29b6
@@ -0,0 +1,309 @@
+/*
+ * Copyright (C) 2017 Intel Corporation <www.intel.com>
+ *
+ * SPDX-License-Identifier:    GPL-2.0
+ */

common/fs_loader.c line 15

Quoted 10 lines

common/fs_loader.c new file +10 -0

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

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

Quoted 2 lines

I can enable the Kconfig, and put the caution about dependency on <asm/spl.h> in document.

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

Quoted 25 lines · Tom Rini

Quoted 24 lines · Tien Fong

Quoted 22 lines · Tom Rini

Quoted 11 lines · tien.fong.chee at intel.co m

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

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

Quoted 6 lines

Quoted 1 line

I have ran the checkpatch.pl on this patch, i didn't see any error.

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

It's weird for checkpatch.pl not showing the same. I would fix the code with above example.

Quoted 33 lines

[snip]

Quoted 32 lines

Quoted 32 lines

Quoted 14 lines

common/fs_loader.c new file +5 -0

new file mode 100644
index 0000000..56d29b6
@@ -0,0 +1,309 @@
+/*
+ * Copyright (C) 2017 Intel Corporation <www.intel.com>
+ *
+ * SPDX-License-Identifier:    GPL-2.0
+ */

common/fs_loader.c line 15

Quoted 10 lines

common/fs_loader.c new file +10 -0

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

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

Quoted 9 lines

Quoted 2 lines

I can enable the Kconfig, and put the caution about dependency on <asm/spl.h> in document.

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

How about i just move the whole enum { } from line 17 to line 35 in asm/spl.h to fs.h, and then enum{} above replaced with #include <fs.h>?

asm/spl.h
----------

#if defined(CONFIG_ARCH_OMAP2PLUS) \
	|| defined(CONFIG_EXYNOS4) || defined(CONFIG_EXYNOS5) \
	|| defined(CONFIG_EXYNOS4210)

/* Platform-specific defines */
#include <asm/arch/spl.h>

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

Quoted 41 lines · Tien Fong

Quoted 39 lines · Tom Rini

Quoted 30 lines · Tien Fong

Quoted 19 lines · Tom Rini

On Wed, Dec 27, 2017 at 01:04:38PM +0800, tien.fong.chee at inte l.co
m

Quoted 12 lines · 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.

Quoted 8 lines

Quoted 1 line

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

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

Quoted 2 lines

I have ran the checkpatch.pl on this patch, i didn't see any error.

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

Quoted 3 lines

It's weird for checkpatch.pl not showing the same. I would fix the code
with above example.

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

Quoted 62 lines

Quoted 42 lines

[snip]

Quoted 36 lines

Quoted 27 lines

Quoted 17 lines

common/fs_loader.c new file +5 -0

new file mode 100644
index 0000000..56d29b6
@@ -0,0 +1,309 @@
+/*
+ * Copyright (C) 2017 Intel Corporation <www.intel.com>
+ *
+ * SPDX-License-Identifier:    GPL-2.0
+ */

common/fs_loader.c line 15

Quoted 10 lines

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

Quoted 9 lines

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

Quoted 3 lines

I can enable the Kconfig, and put the caution about dependency on
<asm/spl.h> in document.

Quoted 8 lines

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

How about i just move the whole enum { } from line 17 to line 35 in asm/spl.h to fs.h, and then enum{} above replaced with #include <fs.h>?

asm/spl.h
----------

#if defined(CONFIG_ARCH_OMAP2PLUS) \
	|| defined(CONFIG_EXYNOS4) || defined(CONFIG_EXYNOS5) \
	|| defined(CONFIG_EXYNOS4210)

/* Platform-specific defines */
#include <asm/arch/spl.h>

Quoted 4 lines

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

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

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

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

+	ret = run_command(cmd, 0);
+	if (ret)
+		return ret;
+

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

+ */

common/fs_loader.c new file +5 -0

+static int _request_firmware_prepare(struct firmware **firmware_p,
+				     const char *name, void *dbuf,
+				     size_t size, u32 offset)
+{
+	struct firmware *firmware;

common/fs_loader.c new file +19 -0

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

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

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

[...]

Quoted 4 lines

common/fs_loader.c new file +3 -0

+/* USB build is not supported yet in SPL */
+#ifndef CONFIG_SPL_BUILD
+#ifdef CONFIG_USB_STORAGE

common/fs_loader.c line 51

Quoted 10 lines

common/fs_loader.c new file +10 -0

+static int init_usb(void)
+{
+	int err;
+
+	err = usb_init();
+	if (err)
+		return err;
+
+#ifndef CONFIG_DM_USB
+	err = usb_stor_scan(1) < 0 ? -ENODEV : 0;

if (err)

	return err;

?

Quoted 10 lines

common/fs_loader.c new file +8 -0

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

support\n");

debug() ? Fix globally ...

Quoted 19 lines

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

common/fs_loader.c line 84

Quoted 7 lines

common/fs_loader.c new file +7 -0

+#ifdef CONFIG_CMD_UBIFS
+static int mount_ubifs(struct device_location *location)
+{
+	int ret;
+	char cmd[32];
+
+	sprintf(cmd, "ubi part %s", location->mtdpart);

snprintf() ...

Quoted 5 lines

common/fs_loader.c new file +4 -0

+	ret = run_command(cmd, 0);
+	if (ret)
+		return ret;
+

common/fs_loader.c line 99

Quoted 10 lines

common/fs_loader.c new file +10 -0

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

Just call the function directly ?

Quoted 96 lines

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

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

(quoted diff) +2 -0

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

FS_TYPE_ANY);

(quoted diff) +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 +30 -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;
+
+	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");

common/fs_loader.c new file +12 -0

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

common/fs_loader.c line 195

Quoted 7 lines

common/fs_loader.c new file +7 -0

+			default_locations[i].devpart = devpart;
+	}
+}
+
+/*
+ * Prepare firmware struct;
+ * return -ve if fail.

Use kerneldoc and keep it consistent.

Quoted 13 lines

+ */

common/fs_loader.c new file +11 -0

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

Quoted 5 lines

common/fs_loader.c new file +4 -0

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

__func__);

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

Quoted 21 lines

common/fs_loader.c new file +6 -0

+		return -ENOMEM;
+	}
+
+	fw_priv = calloc(1, sizeof(*fw_priv));
+	if (!fw_priv) {
+		printf("%s: calloc(struct fw_priv) failed\n",

__func__);

common/fs_loader.c new file +13 -0

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

[...]

Quoted 22 lines · Marek Vasut

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

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

[...]

Quoted 4 lines

common/fs_loader.c new file +3 -0

+/* USB build is not supported yet in SPL */
+#ifndef CONFIG_SPL_BUILD
+#ifdef CONFIG_USB_STORAGE

common/fs_loader.c line 51

Quoted 10 lines

common/fs_loader.c new file +10 -0

+static int init_usb(void)
+{
+	int err;
+
+	err = usb_init();
+	if (err)
+		return err;
+
+#ifndef CONFIG_DM_USB
+	err = usb_stor_scan(1) < 0 ? -ENODEV : 0;

if (err)

Quoted 3 lines

	return err;

?

This is last line code of the function, so it's always return the result regardless error or not.

Quoted 12 lines

Quoted 10 lines

common/fs_loader.c new file +8 -0

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

support\n");

debug() ? Fix globally ...

okay.

Quoted 19 lines

Quoted 19 lines

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

Quoted 9 lines

common/fs_loader.c line 84

Quoted 7 lines

common/fs_loader.c new file +7 -0

+#ifdef CONFIG_CMD_UBIFS
+static int mount_ubifs(struct device_location *location)
+{
+	int ret;
+	char cmd[32];
+
+	sprintf(cmd, "ubi part %s", location->mtdpart);

snprintf() ...

okay.

Quoted 17 lines

Quoted 5 lines

common/fs_loader.c new file +4 -0

+	ret = run_command(cmd, 0);
+	if (ret)
+		return ret;
+

common/fs_loader.c line 99

Quoted 10 lines

common/fs_loader.c new file +10 -0

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

Just call the function directly ?

There are some checking like ubifs_initialized in the cmd/ubifs.c. Direct callng the function would bypass those checking.

Quoted 96 lines

Quoted 96 lines

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

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

(quoted diff) +2 -0

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

FS_TYPE_ANY);

(quoted diff) +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 +30 -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;
+
+	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");

common/fs_loader.c new file +12 -0

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

Quoted 9 lines

common/fs_loader.c line 195

Quoted 7 lines

common/fs_loader.c new file +7 -0

+			default_locations[i].devpart = devpart;
+	}
+}
+
+/*
+ * Prepare firmware struct;
+ * return -ve if fail.

Use kerneldoc and keep it consistent.

kerneldoc doesn't has explanation for this function, and this function is not for user. Or you means i shouldn't put the comment and description to the function here?

Quoted 13 lines

Quoted 13 lines

+ */

common/fs_loader.c new file +11 -0

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

Quoted 9 lines

Quoted 5 lines

common/fs_loader.c new file +4 -0

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

__func__);

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

okay.

Quoted 22 lines

Quoted 21 lines

common/fs_loader.c new file +6 -0

+		return -ENOMEM;
+	}
+
+	fw_priv = calloc(1, sizeof(*fw_priv));
+	if (!fw_priv) {
+		printf("%s: calloc(struct fw_priv) failed\n",

__func__);

common/fs_loader.c new file +13 -0

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

[...]

Quoted 30 lines · Tien Fong

Quoted 28 lines · Marek Vasut

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

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

[...]

Quoted 5 lines

common/fs_loader.c new file +3 -0

+/* USB build is not supported yet in SPL */
+#ifndef CONFIG_SPL_BUILD
+#ifdef CONFIG_USB_STORAGE

common/fs_loader.c line 51

Quoted 10 lines

common/fs_loader.c new file +10 -0

+static int init_usb(void)
+{
+	int err;
+
+	err = usb_init();
+	if (err)
+		return err;
+
+#ifndef CONFIG_DM_USB
+	err = usb_stor_scan(1) < 0 ? -ENODEV : 0;

if (err)

	return err;

?

Quoted 2 lines

This is last line code of the function, so it's always return the result regardless error or not.

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

Quoted 65 lines

Quoted 14 lines

Quoted 11 lines

common/fs_loader.c new file +8 -0

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

Quoted 1 line

support\n");

debug() ? Fix globally ...

okay.

Quoted 21 lines

Quoted 21 lines

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

Quoted 9 lines

common/fs_loader.c line 84

Quoted 7 lines

common/fs_loader.c new file +7 -0

+#ifdef CONFIG_CMD_UBIFS
+static int mount_ubifs(struct device_location *location)
+{
+	int ret;
+	char cmd[32];
+
+	sprintf(cmd, "ubi part %s", location->mtdpart);

snprintf() ...

okay.

Quoted 19 lines

Quoted 7 lines

common/fs_loader.c new file +4 -0

+	ret = run_command(cmd, 0);
+	if (ret)
+		return ret;
+

common/fs_loader.c line 99

Quoted 10 lines

common/fs_loader.c new file +10 -0

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

Just call the function directly ?

Quoted 2 lines

There are some checking like ubifs_initialized in the cmd/ubifs.c. Direct callng the function would bypass those checking.

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

Quoted 115 lines

Quoted 115 lines

Quoted 107 lines

common/fs_loader.c new file +5 -0

+}
+#else
+static int mount_ubifs(struct device_location *location)
+{
+	printf("Error: Cannot load image: no UBIFS

support\n");

common/fs_loader.c new file +5 -0

+	return -ENOSYS;
+}
+#endif
+
+#if defined(CONFIG_SPL_MMC_SUPPORT) &&

defined(CONFIG_SPL_BUILD)

common/fs_loader.c new file +11 -0

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

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

(quoted diff) +2 -0

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

FS_TYPE_ANY);

(quoted diff) +2 -0

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

Quoted 1 line

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

+		else
+			ret = -ENODEV;
+	} else {
+		printf("Error: unsupported location

storage.\n");

common/fs_loader.c new file +10 -0

+		return -ENODEV;
+	}
+
+	if (ret)
+		printf("Error: could not access storage.\n");
+
+	return ret;
+}
+
+static int init_storage_device(struct device_location

*location)

common/fs_loader.c new file +16 -0

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

common/fs_loader.c new file +13 -0

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

common/fs_loader.c line 195

Quoted 6 lines

common/fs_loader.c new file +6 -0

+	}
+}
+
+/*
+ * Prepare firmware struct;
+ * return -ve if fail.

Use kerneldoc and keep it consistent.

Quoted 4 lines

kerneldoc doesn't has explanation for this function, and this function
is not for user. Or you means i shouldn't put the comment and description to the function here?

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

Quoted 53 lines

Quoted 19 lines

Quoted 19 lines

+ */
+static int _request_firmware_prepare(struct firmware **firmware_p,

(quoted diff) +1 -0

+				     const char *name, void

*dbuf,

common/fs_loader.c new file +10 -0

+				     size_t size, u32 offset)
+{
+	struct firmware *firmware;
+	struct firmware_priv *fw_priv;
+
+	*firmware_p = NULL;
+
+	if (!name || name[0] == '\0')
+		return -EINVAL;
+

Quoted 9 lines

Quoted 4 lines

common/fs_loader.c new file +3 -0

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

__func__);

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

okay.

Quoted 24 lines

Quoted 23 lines

common/fs_loader.c new file +6 -0

+		return -ENOMEM;
+	}
+
+	fw_priv = calloc(1, sizeof(*fw_priv));
+	if (!fw_priv) {
+		printf("%s: calloc(struct fw_priv) failed\n",

__func__);

common/fs_loader.c new file +13 -0

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

[...]

Quoted 33 lines · Marek Vasut

Quoted 32 lines · Tien Fong

Quoted 25 lines · Marek Vasut

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

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

[...]

Quoted 5 lines

common/fs_loader.c new file +3 -0

+/* USB build is not supported yet in SPL */
+#ifndef CONFIG_SPL_BUILD
+#ifdef CONFIG_USB_STORAGE

common/fs_loader.c line 51

Quoted 10 lines

common/fs_loader.c new file +10 -0

+static int init_usb(void)
+{
+	int err;
+
+	err = usb_init();
+	if (err)
+		return err;
+
+#ifndef CONFIG_DM_USB
+	err = usb_stor_scan(1) < 0 ? -ENODEV : 0;

if (err)

Quoted 3 lines

	return err;

?

This is last line code of the function, so it's always return the result regardless error or not.

Quoted 3 lines

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

Ohh....are you saying to change the codes as shown in below:

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

Hi,

Quoted 43 lines · Tien Fong

Quoted 41 lines · Marek Vasut

Quoted 33 lines · Tien Fong

Quoted 27 lines · Marek Vasut

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

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

[...]

Quoted 6 lines

common/fs_loader.c new file +3 -0

+/* USB build is not supported yet in SPL */
+#ifndef CONFIG_SPL_BUILD
+#ifdef CONFIG_USB_STORAGE

common/fs_loader.c line 51

Quoted 10 lines

common/fs_loader.c new file +10 -0

+static int init_usb(void)
+{
+	int err;
+
+	err = usb_init();
+	if (err)
+		return err;
+
+#ifndef CONFIG_DM_USB
+	err = usb_stor_scan(1) < 0 ? -ENODEV : 0;

if (err)

Quoted 3 lines

	return err;

?

Quoted 3 lines

This is last line code of the function, so it's always return the
result regardless error or not.

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

Quoted 6 lines

Ohh....are you saying to change the codes as shown in below:

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

usb_stor_scan() does not return a sensible error code, but '-1' if no device was found. This should be changed to -ENODEV then!

Lothar Waßmann

Quoted 5 lines

Quoted 2 lines

This is last line code of the function, so it's always return the result regardless error or not.

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

Ohh....are you saying to change the codes as shown in below:

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

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

common/fs_loader.c line 99

Quoted 3 lines

common/fs_loader.c new file +3 -0

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

Just call the function directly ?

Quoted 5 lines

Quoted 2 lines

There are some checking like ubifs_initialized in the cmd/ubifs.c. Direct callng the function would bypass those checking.

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

Just for curious, is it worth to factor those into a function? Does it help to boost the performance or for other purpose?

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         | 309 +++++++++++++++++++++++++++++++++++++++++++++
  doc/README.firmware_loader |  76 +++++++++++
  include/fs_loader.h        |  28 ++++
  4 files changed, 414 insertions(+)

create mode 100644 common/fs_loader.c
create mode 100644 doc/README.firmware_loader
create mode 100644 include/fs_loader.h

common/fs_loader.c new file +7 -0

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

Quoted 19 lines · 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.

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

---
  common/Makefile            |   1 +
  common/fs_loader.c         | 309
+++++++++++++++++++++++++++++++++++++++++++++
  doc/README.firmware_loader |  76 +++++++++++
  include/fs_loader.h        |  28 ++++

  4 files changed, 414 insertions(+)

  create mode 100644 common/fs_loader.c
  create mode 100644 doc/README.firmware_loader
  create mode 100644 include/fs_loader.h

<snip>

Quoted 16 lines

common/fs_loader.c new file +12 -0

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

(quoted diff) +2 -0

+		return err;
+	}

common/fs_loader.c line 55

Quoted 3 lines

common/fs_loader.c new file +3 -0

+
+	return err;
+}

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

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

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

Regards,
Simon

Quoted 36 lines · Simon Goldschmidt

Quoted 18 lines · 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.

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

---
   common/Makefile            |   1 +
   common/fs_loader.c         | 309
+++++++++++++++++++++++++++++++++++++++++++++
   doc/README.firmware_loader |  76 +++++++++++
   include/fs_loader.h        |  28 ++++
   4 files changed, 414 insertions(+)

  create mode 100644 common/fs_loader.c
  create mode 100644 doc/README.firmware_loader
  create mode 100644 include/fs_loader.h

<snip>

Quoted 15 lines

common/fs_loader.c new file +12 -0

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

(quoted diff) +2 -0

+		return err;
+	}

Quoted 9 lines

common/fs_loader.c line 55

Quoted 3 lines

common/fs_loader.c new file +3 -0

+
+	return err;
+}

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

Yeah, you are right, i should add the check on the return value. I think that would better to initialize NULL to mmc, because there is no checking on the mmc in spl_mmc_find_device(), so that is possible memory access violation/abort can be happended if unknown value in mmc pointer.

Quoted 4 lines

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

Quoted 8 lines

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

The main purpose here is to initialize the mmc driver. So which storage user wants to load the file is totally depend what storage such as mmc user defines in location->name. Loader would init the storage based on the storage defined in location->name before accessing it. Since the loader only support file system at this moment, i would suggest FAT fs for mmc and ubi fs for qspi.

Quoted 79 lines · Tien Fong

Quoted 44 lines · Simon Goldschmidt

Quoted 20 lines · 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.

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

---
   common/Makefile            |   1 +
   common/fs_loader.c         | 309
+++++++++++++++++++++++++++++++++++++++++++++
   doc/README.firmware_loader |  76 +++++++++++
   include/fs_loader.h        |  28 ++++

   4 files changed, 414 insertions(+)

   create mode 100644 common/fs_loader.c
   create mode 100644 doc/README.firmware_loader
   create mode 100644 include/fs_loader.h

<snip>

Quoted 20 lines

+#if defined(CONFIG_SPL_MMC_SUPPORT) &&
defined(CONFIG_SPL_BUILD)

common/fs_loader.c new file +11 -0

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

+		return err;
+	}
+
+	return err;

Quoted 9 lines

Quoted 1 line

+}

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

Yeah, you are right, i should add the check on the return value. I think that would better to initialize NULL to mmc, because there is no
checking on the mmc in spl_mmc_find_device(), so that is possible memory access violation/abort can be happended if unknown value in mmc
pointer.

Quoted 6 lines

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

Quoted 10 lines

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

Quoted 11 lines

The main purpose here is to initialize the mmc driver. So which storage
user wants to load the file is totally depend what storage such as mmc
user defines in location->name. Loader would init the storage based on
the storage defined in location->name before accessing it. Since the
loader only support file system at this moment, i would suggest FAT fs
for mmc and ubi fs for qspi.

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

Regards,
Simon

Quoted 86 lines · Simon Goldschmidt

Quoted 74 lines · Tien Fong

Quoted 41 lines · Simon Goldschmidt

Quoted 19 lines · 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.

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

---
    common/Makefile            |   1 +
    common/fs_loader.c         | 309
+++++++++++++++++++++++++++++++++++++++++++++
    doc/README.firmware_loader |  76 +++++++++++
    include/fs_loader.h        |  28 ++++
    4 files changed, 414 insertions(+)

   create mode 100644 common/fs_loader.c
   create mode 100644 doc/README.firmware_loader
   create mode 100644 include/fs_loader.h

<snip>

Quoted 19 lines

+#if defined(CONFIG_SPL_MMC_SUPPORT) &&
defined(CONFIG_SPL_BUILD)

common/fs_loader.c new file +11 -0

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

+		return err;
+	}
+
+	return err;

Quoted 9 lines

Quoted 1 line

+}

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

Yeah, you are right, i should add the check on the return value. I think that would better to initialize NULL to mmc, because there is no
checking on the mmc in spl_mmc_find_device(), so that is possible memory access violation/abort can be happended if unknown value in mmc
pointer.

Quoted 5 lines

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

Quoted 10 lines

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

Quoted 11 lines

The main purpose here is to initialize the mmc driver. So which storage
user wants to load the file is totally depend what storage such as mmc
user defines in location->name. Loader would init the storage based on
the storage defined in location->name before accessing it. Since the
loader only support file system at this moment, i would suggest FAT fs
for mmc and ubi fs for qspi.

Quoted 8 lines

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

Okay, i got you.

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

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

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

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

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         | 309 +++++++++++++++++++++++++++++++++++++++++++++
  doc/README.firmware_loader |  76 +++++++++++
  include/fs_loader.h        |  28 ++++
  4 files changed, 414 insertions(+)

create mode 100644 common/fs_loader.c
create mode 100644 doc/README.firmware_loader
create mode 100644 include/fs_loader.h

common/Makefile +0 -0

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

include/fs_loader.h new file +8 -0

new file mode 100644
index 0000000..83a8b80
@@ -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/fs_loader.h new file +10 -0

+	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

Quoted 25 lines · 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.

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

---
  common/Makefile            |   1 +
  common/fs_loader.c         | 309
+++++++++++++++++++++++++++++++++++++++++++++
  doc/README.firmware_loader |  76 +++++++++++
  include/fs_loader.h        |  28 ++++

  4 files changed, 414 insertions(+)

  create mode 100644 common/fs_loader.c
  create mode 100644 doc/README.firmware_loader
  create mode 100644 include/fs_loader.h

common/Makefile +0 -0

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

<snip>

Quoted 23 lines

include/fs_loader.h new file +14 -0

new file mode 100644
index 0000000..83a8b80
@@ -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 */
+};

include/fs_loader.h line 18

Quoted 3 lines

include/fs_loader.h new file +3 -0

+
+struct device_location {
+	char *name;	/* Such as mmc, usb,and sata. */

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

Regards,
Simon

Quoted 12 lines

include/fs_loader.h new file +1 -0

+	char *devpart;  /* Use the load command dev:part

conventions */

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

Quoted 49 lines · Simon Goldschmidt

Quoted 24 lines · 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.

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

---
   common/Makefile            |   1 +
   common/fs_loader.c         | 309
+++++++++++++++++++++++++++++++++++++++++++++
   doc/README.firmware_loader |  76 +++++++++++
   include/fs_loader.h        |  28 ++++
   4 files changed, 414 insertions(+)

  create mode 100644 common/fs_loader.c
  create mode 100644 doc/README.firmware_loader
  create mode 100644 include/fs_loader.h

common/Makefile +0 -0

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

<snip>

Quoted 22 lines

include/fs_loader.h new file +14 -0

new file mode 100644
index 0000000..83a8b80
@@ -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 */
+};

Quoted 9 lines

include/fs_loader.h line 18

Quoted 3 lines

include/fs_loader.h new file +3 -0

+
+struct device_location {
+	char *name;	/* Such as mmc, usb,and sata. */

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

This structure is declared such way so that it can be compatible with common/splash.c. It also much more easy to port fs_loader into common/splash_source.c in later.

Quoted 14 lines

Regards,
Simon

Quoted 11 lines

include/fs_loader.h new file +1 -0

+	char *devpart;  /* Use the load command dev:part

conventions */

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

Quoted 66 lines · Tien Fong

Quoted 55 lines · Simon Goldschmidt

Quoted 26 lines · 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.

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

---
   common/Makefile            |   1 +
   common/fs_loader.c         | 309
+++++++++++++++++++++++++++++++++++++++++++++
   doc/README.firmware_loader |  76 +++++++++++
   include/fs_loader.h        |  28 ++++

   4 files changed, 414 insertions(+)

   create mode 100644 common/fs_loader.c
   create mode 100644 doc/README.firmware_loader create mode 100644 include/fs_loader.h

common/Makefile +0 -0

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

<snip>

Quoted 25 lines

include/fs_loader.h new file +14 -0

new file mode 100644
index 0000000..83a8b80
@@ -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 */

include/fs_loader.h new file +3 -0

+};
+
+struct device_location {

Quoted 9 lines

include/fs_loader.h line 18

Quoted 1 line

include/fs_loader.h new file +1 -0

+	char *name;	/* Such as mmc, usb,and sata. */

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

Quoted 4 lines

This structure is declared such way so that it can be compatible with
common/splash.c. It also much more easy to port fs_loader into common/splash_source.c in later.

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

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

Quoted 20 lines

Quoted 20 lines

Regards,
Simon

Quoted 15 lines

include/fs_loader.h new file +1 -0

+	char *devpart;  /* Use the load command dev:part

conventions */

include/fs_loader.h new file +2 -0

+	char *mtdpart;	/* MTD partition for ubi part */
+	char *ubivol;	/* UBI volume-name for ubifsmount

*/

include/fs_loader.h new file +5 -0

+};
+
+int request_firmware_into_buf(struct firmware **firmware_p,
+			      const char *name,
+			      struct device_location

*location,

include/fs_loader.h new file +1 -0

+			      void *buf, size_t size, u32

offset);
+#endif

Quoted 71 lines · Simon Goldschmidt

Quoted 62 lines · Tien Fong

Quoted 52 lines · Simon Goldschmidt

Quoted 25 lines · 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.

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

---
    common/Makefile            |   1 +
    common/fs_loader.c         | 309
+++++++++++++++++++++++++++++++++++++++++++++
    doc/README.firmware_loader |  76 +++++++++++
    include/fs_loader.h        |  28 ++++
    4 files changed, 414 insertions(+)

   create mode 100644 common/fs_loader.c
   create mode 100644 doc/README.firmware_loader create mode 100644 include/fs_loader.h

common/Makefile +0 -0

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

<snip>

Quoted 24 lines

include/fs_loader.h new file +14 -0

new file mode 100644
index 0000000..83a8b80
@@ -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 */

include/fs_loader.h new file +3 -0

+};
+
+struct device_location {

Quoted 9 lines

include/fs_loader.h line 18

Quoted 1 line

include/fs_loader.h new file +1 -0

+	char *name;	/* Such as mmc, usb,and sata. */

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

Quoted 4 lines

This structure is declared such way so that it can be compatible with
common/splash.c. It also much more easy to port fs_loader into common/splash_source.c in later.

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

Quoted 7 lines

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

Actually this is common interface name used in any fs related. You can refer more details regarding @ifname in include/fs.h and include part.h .

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

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

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

Quoted 17 lines

Quoted 17 lines

Quoted 17 lines

Regards,
Simon

Quoted 14 lines

include/fs_loader.h new file +1 -0

+	char *devpart;  /* Use the load command dev:part

conventions */

include/fs_loader.h new file +2 -0

+	char *mtdpart;	/* MTD partition for ubi part */
+	char *ubivol;	/* UBI volume-name for ubifsmount

*/

include/fs_loader.h new file +5 -0

+};
+
+int request_firmware_into_buf(struct firmware **firmware_p,
+			      const char *name,
+			      struct device_location

*location,

include/fs_loader.h new file +1 -0

+			      void *buf, size_t size, u32

offset);
+#endif

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
Lothar Waßmann in [v5].

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

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

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

Patchset history
----------------
[v1]: https://www.mail-archive.com/u-boot at lists.denx.de/msg271905.htm l
[v2]: https://www.mail-archive.com/u-boot at lists.denx.de/msg271979.htm l
[v3]: https://www.mail-archive.com/u-boot at lists.denx.de/msg272039.htm l
[v4]: https://www.mail-archive.com/u-boot at lists.denx.de/msg272432.htm l

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

 common/Makefile            |   1 +
 common/fs_loader.c         | 309
+++++++++++++++++++++++++++++++++++++++++++++
 common/spl/spl_mmc.c       |   2 +-
 doc/README.firmware_loader |  76 +++++++++++
 include/fs_loader.h        |  28 ++++
 include/spl.h              |   2 +

 6 files changed, 417 insertions(+), 1 deletion(-)

 create mode 100644 common/fs_loader.c
 create mode 100644 doc/README.firmware_loader
 create mode 100644 include/fs_loader.h

On Rab, 2017-12-27 at 13:04 +0800, tien.fong.chee at intel.com wrote:
Hi Lothar Waßmann,

Quoted 12 lines

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
Lothar Waßmann in [v5].

Are you OK with the patches?

Hi,

Quoted 20 lines · Tien Fong

On Rab, 2017-12-27 at 13:04 +0800, tien.fong.chee at intel.com wrote:
Hi Lothar Waßmann,

Quoted 17 lines

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
Lothar Waßmann in [v5].

Quoted 2 lines

Are you OK with the patches?

Sorry for the delay. I have been on vacation until now. The patch series Looks good to me now.

Reviewed-by Lothar Waßmann <[email protected]>

Lothar Waßmann

Hi,

Quoted 21 lines · Tien Fong

On Rab, 2017-12-27 at 13:04 +0800, tien.fong.chee at intel.com wrote:
Hi Lothar Waßmann,

Quoted 17 lines

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
Lothar Waßmann in [v5].

Are you OK with the patches?

Quoted 1 line

Sorry for the delay. I have been on vacation until now. The patch series Looks good to me now.

Reviewed-by Lothar Waßmann <[email protected]>

Lothar Waßmann

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
Lothar Waßmann in [v5].

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

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

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

Patchset history
----------------
[v1]: https://www.mail-archive.com/u-boot at lists.denx.de/msg271905.htm l
[v2]: https://www.mail-archive.com/u-boot at lists.denx.de/msg271979.htm l
[v3]: https://www.mail-archive.com/u-boot at lists.denx.de/msg272039.htm l
[v4]: https://www.mail-archive.com/u-boot at lists.denx.de/msg272432.htm l

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

 common/Makefile            |   1 +
 common/fs_loader.c         | 309
+++++++++++++++++++++++++++++++++++++++++++++
 common/spl/spl_mmc.c       |   2 +-
 doc/README.firmware_loader |  76 +++++++++++
 include/fs_loader.h        |  28 ++++
 include/spl.h              |   2 +

 6 files changed, 417 insertions(+), 1 deletion(-)

 create mode 100644 common/fs_loader.c
 create mode 100644 doc/README.firmware_loader
 create mode 100644 include/fs_loader.h