Skip to contentSkip to conversation

Generic firmware loader

PATCH Superseded tien.fong.chee at intel.com 5 participants 19 messages Feb 5, 2018 – Feb 26, 2018

v8 of 10 Superseded Latest is v10 →

lore
Share & tools
lore
#0 lore raw

v8 cover Generic firmware loader

From: Tien Fong Chee <[email protected]>

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

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

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

v7 -> v8 changes:
-----------------
- Return cmd_ubifs_umount() directly.
- Call the UBI functions directly instead of invoking the U-Boot command parser.

Patchset history
----------------
[v1]: https://www.mail-archive.com/u-boot at lists.denx.de/msg271905.html [v2]: https://www.mail-archive.com/u-boot at lists.denx.de/msg271979.html [v3]: https://www.mail-archive.com/u-boot at lists.denx.de/msg272039.html [v4]: https://www.mail-archive.com/u-boot at lists.denx.de/msg272432.html [v5]: https://www.mail-archive.com/u-boot at lists.denx.de/msg272771.html [v6]: https://www.mail-archive.com/u-boot at lists.denx.de/msg273961.html

Tien Fong Chee (4):
spl: Remove static declaration on spl_mmc_find_device function
cmd: ubifs: Move ubifs_initialized checking into cmd_ubifs_umount()
cmd: ubifs: Factor out some checking codes into cmd_ubifs_mount()
common: Generic firmware loader for file system

 cmd/ubifs.c                |  40 +++---
 common/Kconfig             |   9 ++
 common/Makefile            |   1 +
 common/fs_loader.c         | 320 +++++++++++++++++++++++++++++++++++++++++++++
 common/spl/spl_mmc.c       |   2 +-
 doc/README.firmware_loader |  76 +++++++++++
 include/fs_loader.h        |  28 ++++
 include/spl.h              |   2 +
 include/ubi_uboot.h        |   2 +
 9 files changed, 462 insertions(+), 18 deletions(-)
 create mode 100644 common/fs_loader.c
 create mode 100644 doc/README.firmware_loader
 create mode 100644 include/fs_loader.h
Signature · 1 line
2.2.0
#1 lore raw

PATCH v8 1/4 spl: Remove static declaration on spl_mmc_find_device function

From: Tien Fong Chee <[email protected]>

This patch removes the static declation on spl_mmc_find_device_function so this function is accessible by the caller from other file. This patch is required for later patch.

---
 common/spl/spl_mmc.c | 2 +-
 include/spl.h        | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)
common/spl/spl_mmc.c +1 -1
index b57e0b0..183d05a 100644
@@ -114,7 +114,7 @@ static int spl_mmc_get_device_index(u32 boot_device)
 	return -ENODEV;
 }
 
-static int spl_mmc_find_device(struct mmc **mmcp, u32 boot_device)
+int spl_mmc_find_device(struct mmc **mmcp, u32 boot_device)
 {
 #if CONFIG_IS_ENABLED(DM_MMC)
 	struct udevice *dev;
include/spl.h +2 -0
index c14448b..60424d7 100644
@@ -12,6 +12,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
@@ -83,6 +84,7 @@ void preloader_console_init(void);
 u32 spl_boot_device(void);
 u32 spl_boot_mode(const u32 boot_device);
 void spl_set_bd(void);
+int spl_mmc_find_device(struct mmc **mmcp, u32 boot_device);
 
 /**
  * spl_set_header_raw_uboot() - Set up a standard SPL image structure
Signature · 1 line
2.2.0
#2 lore raw

PATCH v8 1/4 spl: Remove static declaration on spl_mmc_find_device function

Quoted 9 lines · tien.fong.chee at intel.com

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.

---

Signature · 2 lines
Best regards,
Marek Vasut
#3 lore raw

PATCH v8 2/4 cmd: ubifs: Move ubifs_initialized checking into cmd_ubifs_umount()

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

From: Tien Fong Chee <[email protected]>

cmd_ubifs_umount() function would be called directly instead of involving whole command machinery in generic firmware loader, so checking on ubifs_initialized status need to be done in cmd_ubifs_umount() without breaking original functionality design.

---
 cmd/ubifs.c         | 18 +++++++++---------
 include/ubi_uboot.h |  1 +
 2 files changed, 10 insertions(+), 9 deletions(-)
cmd/ubifs.c +9 -9
index 5e9d357..0d059ca 100644
@@ -51,11 +51,18 @@ int ubifs_is_mounted(void)
 	return ubifs_mounted;
 }
 
-void cmd_ubifs_umount(void)
+int cmd_ubifs_umount(void)
 {
+	if (ubifs_initialized == 0) {
+		printf("No UBIFS volume mounted!\n");
+		return -1;
+	}
+
 	uboot_ubifs_umount();
 	ubifs_mounted = 0;
 	ubifs_initialized = 0;
+
+	return 0;
 }
 
 static int do_ubifs_umount(cmd_tbl_t *cmdtp, int flag, int argc,
@@ -64,14 +71,7 @@ static int do_ubifs_umount(cmd_tbl_t *cmdtp, int flag, int argc,
 	if (argc != 1)
 		return CMD_RET_USAGE;
 
-	if (ubifs_initialized == 0) {
-		printf("No UBIFS volume mounted!\n");
-		return -1;
-	}
-
-	cmd_ubifs_umount();
-
-	return 0;
+	return cmd_ubifs_umount();
 }
 
 static int do_ubifs_ls(cmd_tbl_t *cmdtp, int flag, int argc,
include/ubi_uboot.h +1 -0
index 80acbcb..827dbfc 100644
@@ -75,5 +75,6 @@ extern int ubi_volume_write(char *volume, void *buf, size_t size);
 extern int ubi_volume_read(char *volume, char *buf, size_t size);
 
 extern struct ubi_device *ubi_devices[];
+int cmd_ubifs_umount(void);
 
 #endif
Signature · 1 line
2.2.0
#4 lore raw

PATCH v8 2/4 cmd: ubifs: Move ubifs_initialized checking into cmd_ubifs_umount()

Quoted 8 lines · tien.fong.chee at intel.com

From: Tien Fong Chee <[email protected]>

cmd_ubifs_umount() function would be called directly instead of involving whole command machinery in generic firmware loader, so checking on ubifs_initialized status need to be done in cmd_ubifs_umount() without breaking original functionality design.

Signature · 2 lines
Best regards,
Marek Vasut
#5 lore raw

PATCH v8 3/4 cmd: ubifs: Factor out some checking codes into cmd_ubifs_mount()

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

From: Tien Fong Chee <[email protected]>

cmd_ubifs_mount() function would be called directly instead of involving whole command machinery for mounting ubifs in generic firmware loader, so some checking codes need to be factored out into cmd_ubifs_mount() without breaking original functionality design.

---
 cmd/ubifs.c         | 22 ++++++++++++++--------
 include/ubi_uboot.h |  1 +
 2 files changed, 15 insertions(+), 8 deletions(-)
cmd/ubifs.c +14 -8
index 0d059ca..8339bed 100644
@@ -20,16 +20,10 @@
 static int ubifs_initialized;
 static int ubifs_mounted;
 
-static int do_ubifs_mount(cmd_tbl_t *cmdtp, int flag, int argc,
-				char * const argv[])
+int cmd_ubifs_mount(char *vol_name)
 {
-	char *vol_name;
 	int ret;
 
-	if (argc != 2)
-		return CMD_RET_USAGE;
-
-	vol_name = argv[1];
 	debug("Using volume %s\n", vol_name);
 
 	if (ubifs_initialized == 0) {
@@ -43,7 +37,19 @@ static int do_ubifs_mount(cmd_tbl_t *cmdtp, int flag, int argc,
 
 	ubifs_mounted = 1;
 
-	return 0;
+	return ret;
+}
+static int do_ubifs_mount(cmd_tbl_t *cmdtp, int flag, int argc,
+				char * const argv[])
+{
+	char *vol_name;
+
+	if (argc != 2)
+		return CMD_RET_USAGE;
+
+	vol_name = argv[1];
+
+	return cmd_ubifs_mount(vol_name);
 }
 
 int ubifs_is_mounted(void)
include/ubi_uboot.h +1 -0
index 827dbfc..fb300e8 100644
@@ -76,5 +76,6 @@ extern int ubi_volume_read(char *volume, char *buf, size_t size);
 
 extern struct ubi_device *ubi_devices[];
 int cmd_ubifs_umount(void);
+int cmd_ubifs_mount(char *vol_name);
 
 #endif
Signature · 1 line
2.2.0
#6 lore raw

PATCH v8 3/4 cmd: ubifs: Factor out some checking codes into cmd_ubifs_mount()

Quoted 8 lines · tien.fong.chee at intel.com

From: Tien Fong Chee <[email protected]>

cmd_ubifs_mount() function would be called directly instead of involving whole command machinery for mounting ubifs in generic firmware loader, so some checking codes need to be factored out into cmd_ubifs_mount() without breaking original functionality design.

[...]

include/ubi_uboot.h extern int ubi_volume_read(char *volume, char *buf, size_t size); · line 79

Quoted 9 lines
include/ubi_uboot.h +1 -0
index 827dbfc..fb300e8 100644
@@ -76,5 +76,6 @@ extern int ubi_volume_read(char *volume, char *buf, size_t size);
 
 extern struct ubi_device *ubi_devices[];
 int cmd_ubifs_umount(void);
+int cmd_ubifs_mount(char *vol_name);

Swap these two -- mount first and umount seconds.

With that

Signature · 2 lines
Best regards,
Marek Vasut
#7 lore raw

PATCH v8 3/4 cmd: ubifs: Factor out some checking codes into cmd_ubifs_mount()

Quoted 15 lines · Marek Vasut
Quoted 11 lines · tien.fong.chee at intel.com

From: Tien Fong Chee <[email protected]>

cmd_ubifs_mount() function would be called directly instead of involving whole command machinery for mounting ubifs in generic firmware loader, so some checking codes need to be factored out
into cmd_ubifs_mount() without breaking original functionality design.

[...]

Quoted 1 line
Quoted 12 lines
Quoted 10 lines
include/ubi_uboot.h +0 -0
index 827dbfc..fb300e8 100644
@@ -76,5 +76,6 @@ extern int ubi_volume_read(char *volume, char
 *buf, size_t size);


 extern struct ubi_device *ubi_devices[];
 int cmd_ubifs_umount(void);
+int cmd_ubifs_mount(char *vol_name);

Swap these two -- mount first and umount seconds.

Okay. noted.

Quoted 3 lines

With that

#8 lore raw

PATCH v8 4/4 common: Generic firmware loader for file system

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

From: Tien Fong Chee <[email protected]>

This is file system generic loader which can be used to load the file image from the storage into target such as memory. The consumer driver would then use this loader to program whatever, ie. the FPGA device.

---
 common/Kconfig             |   9 ++
 common/Makefile            |   1 +
 common/fs_loader.c         | 320 +++++++++++++++++++++++++++++++++++++++++++++
 doc/README.firmware_loader |  76 +++++++++++
 include/fs_loader.h        |  28 ++++
 5 files changed, 434 insertions(+)
 create mode 100644 common/fs_loader.c
 create mode 100644 doc/README.firmware_loader
 create mode 100644 include/fs_loader.h
common/Kconfig +9 -0
index 21e067c..cbb0976 100644
@@ -547,6 +547,15 @@ config DISPLAY_BOARDINFO
 	  when U-Boot starts up. The board function checkboard() is called
 	  to do this.
 
+config GENERIC_FIRMWARE_LOADER
+	bool "Enable generic firmware loader driver for file system"
+	help
+	  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.
+
 menu "Start-up hooks"
 
 config ARCH_EARLY_INIT_R
common/Makefile +1 -0
index c7bde23..92d8fb3 100644
@@ -134,3 +134,4 @@ obj-$(CONFIG_$(SPL_)LOG) += log.o
 obj-$(CONFIG_$(SPL_)LOG_CONSOLE) += log_console.o
 obj-y += s_record.o
 obj-y += xyzModem.o
+obj-$(CONFIG_GENERIC_FIRMWARE_LOADER) += fs_loader.o
common/fs_loader.c new file +320 -0

320 lines not on this page · raw message

doc/README.firmware_loader new file +76 -0

76 lines not on this page · raw message

include/fs_loader.h new file +28 -0
new file mode 100644
index 0000000..60a6ba2
@@ -0,0 +1,28 @@
+/*
+ * Copyright (C) 2018 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
Signature · 1 line
2.2.0
#9 lore raw

PATCH v8 4/4 common: Generic firmware loader for file system

Quoted 9 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.

[...]

common/fs_loader.c line 15

Quoted 9 lines
common/fs_loader.c new file +9 -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>
+#ifdef CONFIG_SPL

Are the ifdefs needed ?

Quoted 17 lines
common/fs_loader.c new file +17 -0
+#include <spl.h>
+#endif
+#include <linux/string.h>
+#ifdef CONFIG_CMD_UBIFS
+#include <ubi_uboot.h>
+#include <ubifs_uboot.h>
+#endif
+#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",

common/fs_loader.c line 42

Quoted 10 lines
common/fs_loader.c new file +10 -0
+		.devpart = "0:1",
+	},
+	{
+		.name = "usb",
+		.devpart = "0:1",
+	},
+	{
+		.name = "sata",
+		.devpart = "0:1",
+	},

How can this load from UBI if it's not listed here ?

Quoted 5 lines
common/fs_loader.c new file +5 -0
+};
+
+/* USB build is not supported yet in SPL */
+#ifndef CONFIG_SPL_BUILD
+#ifdef CONFIG_USB_STORAGE

common/fs_loader.c line 57

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

mode = 0 I think, we don't need this verbose output.

Quoted 9 lines
common/fs_loader.c new file +9 -0
+	if (err)
+		return err;
+#endif
+
+	return err;
+}
+#else
+static int init_usb(void)
+{

common/fs_loader.c line 76

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

Shouldn't the sata device number be pulled from devpart in default_locations table ?

Quoted 37 lines
common/fs_loader.c new file +37 -0
+}
+#else
+static int init_storage_sata(void)
+{
+	debug("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];
+
+	if (ret = ubi_part(location->mtdpart, NULL)) {
+		debug("Cannot find mtd partition %s\n", location->mtdpart);
+		return ret;
+	}
+
+	return cmd_ubifs_mount(location->ubivol);
+}
+
+static int umount_ubifs(void)
+{
+	return cmd_ubifs_umount();
+}
+#else
+static int mount_ubifs(struct device_location *location)
+{
+	debug("Error: Cannot load image: no UBIFS support\n");
+	return -ENOSYS;
+}
+#endif
+
+#if defined(CONFIG_SPL_MMC_SUPPORT) && defined(CONFIG_SPL_BUILD)
+__weak int init_mmc(void)

common/fs_loader.c line 123

Quoted 10 lines
common/fs_loader.c new file +10 -0
+{
+	/* Just for the case MMC is not yet initialized */
+	struct mmc *mmc = NULL;
+	int err;
+
+#ifdef CONFIG_SPL
+	spl_mmc_find_device(&mmc, spl_boot_device());
+#else
+	debug("#define CONFIG_SPL is required or overrriding %s\n",
+	      __func__);

This can be a compile-time error, maybe ?

Quoted 18 lines
common/fs_loader.c new file +18 -0
+	return -ENOENT;
+#endif
+
+	err = mmc_init(mmc);
+	if (err) {
+		debug("spl: mmc init failed with error: %d\n", err);
+		return err;
+	}
+
+	return err;
+}
+#else
+__weak int init_mmc(void)
+{
+	/* Expect somewhere already initialize MMC */
+	return 0;
+}
+#endif

[...]

Signature · 2 lines
Best regards,
Marek Vasut
#10 lore raw

PATCH v8 4/4 common: Generic firmware loader for file system

Quoted 14 lines · Marek Vasut
Quoted 10 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
Quoted 11 lines

common/fs_loader.c line 15

Quoted 9 lines
common/fs_loader.c new file +9 -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>
+#ifdef CONFIG_SPL

Are the ifdefs needed ?

Because spl.h contains some codes have its dependency with SPL. So, Tom adviced to make this part of code depend on CONFIG_SPL. However, only __weak int init_mmc() depend on the codes from spl.h, so user can override their own init_mmc() if SPL is not used.

Quoted 31 lines
Quoted 18 lines
common/fs_loader.c new file +17 -0
+#include <spl.h>
+#endif
+#include <linux/string.h>
+#ifdef CONFIG_CMD_UBIFS
+#include <ubi_uboot.h>
+#include <ubifs_uboot.h>
+#endif
+#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",

common/fs_loader.c line 42

Quoted 10 lines
common/fs_loader.c new file +10 -0
+		.devpart = "0:1",
+	},
+	{
+		.name = "usb",
+		.devpart = "0:1",
+	},
+	{
+		.name = "sata",
+		.devpart = "0:1",
+	},

How can this load from UBI if it's not listed here ?

Quoted 1 line
Quoted 17 lines
Quoted 5 lines
common/fs_loader.c new file +5 -0
+};
+
+/* USB build is not supported yet in SPL */
+#ifndef CONFIG_SPL_BUILD
+#ifdef CONFIG_USB_STORAGE

common/fs_loader.c line 57

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

mode = 0 I think, we don't need this verbose output.

Okay so i will change to usb_stor_scan(0) .

Quoted 14 lines
Quoted 14 lines
common/fs_loader.c new file +13 -0
+	if (err)
+		return err;
+#endif
+
+	return err;
+}
+#else
+static int init_usb(void)
+{
+	debug("Error: Cannot load flash image: no USB support\n");
+	return -ENOSYS;
+}
+#endif
Quoted 9 lines

common/fs_loader.c line 76

Quoted 6 lines
common/fs_loader.c new file +6 -0
+#endif
+
+#ifdef CONFIG_SATA
+static int init_storage_sata(void)
+{
+	return sata_probe(0);

Shouldn't the sata device number be pulled from devpart in default_locations table ?

This may need to add the logic for splitting the device number and partition into integer from the string. Let me think how to do it, may be i can leverage existing code.

Quoted 43 lines
Quoted 43 lines
common/fs_loader.c new file +17 -0
+}
+#else
+static int init_storage_sata(void)
+{
+	debug("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];
+
+	if (ret = ubi_part(location->mtdpart, NULL)) {
+		debug("Cannot find mtd partition %s\n", location-
Quoted 1 line

mtdpart);

common/fs_loader.c new file +24 -0
+		return ret;
+	}
+
+	return cmd_ubifs_mount(location->ubivol);
+}
+
+static int umount_ubifs(void)
+{
+	return cmd_ubifs_umount();
+}
+#else
+static int mount_ubifs(struct device_location *location)
+{
+	debug("Error: Cannot load image: no UBIFS support\n");
+	return -ENOSYS;
+}
+#endif
+
+#if defined(CONFIG_SPL_MMC_SUPPORT) && defined(CONFIG_SPL_BUILD)
+__weak int init_mmc(void)
+{
+	/* Just for the case MMC is not yet initialized */
+	struct mmc *mmc = NULL;
+	int err;
Quoted 9 lines

common/fs_loader.c line 123

Quoted 7 lines
common/fs_loader.c new file +5 -0
+
+#ifdef CONFIG_SPL
+	spl_mmc_find_device(&mmc, spl_boot_device());
+#else
+	debug("#define CONFIG_SPL is required or overrriding

%s\n",

common/fs_loader.c new file +1 -0
+	      __func__);

This can be a compile-time error, maybe ?

No compile error. When you open the file, "%s\n" is actually same line with debug("..... .

Quoted 22 lines
Quoted 20 lines
common/fs_loader.c new file +6 -0
+	return -ENOENT;
+#endif
+
+	err = mmc_init(mmc);
+	if (err) {
+		debug("spl: mmc init failed with error: %d\n",

err);

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

[...]

#11 lore raw

PATCH v8 4/4 common: Generic firmware loader for file system

Quoted 26 lines · Tien Fong
Quoted 14 lines · Marek Vasut
Quoted 10 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
Quoted 11 lines

common/fs_loader.c line 15

Quoted 9 lines
common/fs_loader.c new file +9 -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>
+#ifdef CONFIG_SPL

Are the ifdefs needed ?

Quoted 4 lines

Because spl.h contains some codes have its dependency with SPL. So, Tom adviced to make this part of code depend on CONFIG_SPL. However, only __weak int init_mmc() depend on the codes from spl.h, so user can override their own init_mmc() if SPL is not used.

You probably dont need those ifdefs around headers.

Quoted 20 lines
Quoted 20 lines
Quoted 20 lines
common/fs_loader.c new file +20 -0
+#include <spl.h>
+#endif
+#include <linux/string.h>
+#ifdef CONFIG_CMD_UBIFS
+#include <ubi_uboot.h>
+#include <ubifs_uboot.h>
+#endif
+#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",
+	},
+	{
Quoted 8 lines
Quoted 8 lines

common/fs_loader.c line 42

Quoted 7 lines
common/fs_loader.c new file +7 -0
+		.name = "usb",
+		.devpart = "0:1",
+	},
+	{
+		.name = "sata",
+		.devpart = "0:1",
+	},

How can this load from UBI if it's not listed here ?

Well ?

[...]

Quoted 10 lines
Quoted 7 lines

common/fs_loader.c line 76

Quoted 4 lines
common/fs_loader.c new file +4 -0
+#ifdef CONFIG_SATA
+static int init_storage_sata(void)
+{
+	return sata_probe(0);

Shouldn't the sata device number be pulled from devpart in default_locations table ?

This may need to add the logic for splitting the device number and partition into integer from the string. Let me think how to do it, may be i can leverage existing code.

strtok(), strtol() or something ?

[...]

Quoted 10 lines
Quoted 8 lines

common/fs_loader.c line 123

Quoted 6 lines
common/fs_loader.c new file +4 -0
+#ifdef CONFIG_SPL
+	spl_mmc_find_device(&mmc, spl_boot_device());
+#else
+	debug("#define CONFIG_SPL is required or overrriding

%s\n",

common/fs_loader.c new file +1 -0
+	      __func__);

This can be a compile-time error, maybe ?

No compile error. When you open the file, "%s\n" is actually same line with debug("..... .

So what ? This missing macro can be detected at runtime, heck this code can even depend on CONFIG_SPL in Kconfig.

Quoted 21 lines
Quoted 21 lines
Quoted 20 lines
common/fs_loader.c new file +6 -0
+	return -ENOENT;
+#endif
+
+	err = mmc_init(mmc);
+	if (err) {
+		debug("spl: mmc init failed with error: %d\n",

err);

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

[...]

Signature · 2 lines
Best regards,
Marek Vasut
#12 lore raw

PATCH v8 4/4 common: Generic firmware loader for file system

Quoted 27 lines · Marek Vasut
Quoted 26 lines · Tien Fong
Quoted 25 lines · Marek Vasut
Quoted 10 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.

[...]

common/fs_loader.c line 15

Quoted 10 lines
common/fs_loader.c new file +9 -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>
+#ifdef CONFIG_SPL

Are the ifdefs needed ?

Quoted 6 lines
Quoted 4 lines

Because spl.h contains some codes have its dependency with SPL. So, Tom adviced to make this part of code depend on CONFIG_SPL. However, only __weak int init_mmc() depend on the codes from spl.h, so user can override their own init_mmc() if SPL is not used.

You probably dont need those ifdefs around headers.

In this case, we do. You can only include <spl.h> on architectures which have SPL support.

I wouldn't object to a separate patch series that adds a dummy asm-generic/spl.h and we go that route, if it also cleans up more of the code in general. But I think that's separate from this series. Thanks!

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180222/388023c5/attachment.sig>
#13 lore raw

PATCH v8 4/4 common: Generic firmware loader for file system

Quoted 41 lines · Tom Rini
Quoted 33 lines · Marek Vasut
Quoted 31 lines · Tien Fong
Quoted 29 lines · Marek Vasut
Quoted 12 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.

[...]

common/fs_loader.c line 15

Quoted 11 lines
common/fs_loader.c new file +9 -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>
+#ifdef CONFIG_SPL

Are the ifdefs needed ?

Quoted 7 lines
Quoted 6 lines

Because spl.h contains some codes have its dependency with SPL. So, Tom
adviced to make this part of code depend on CONFIG_SPL. However, only __weak int init_mmc() depend on the codes from spl.h, so
user can override their own init_mmc() if SPL is not used.

You probably dont need those ifdefs around headers.

Quoted 9 lines

In this case, we do.  You can only include <spl.h> on architectures which have SPL support.

I wouldn't object to a separate patch series that adds a dummy asm-generic/spl.h and we go that route, if it also cleans up more of the
code in general.  But I think that's separate from this series.  Thanks!

Planning to add the depend on CONFIG_SPL in Kconfig instead of ifdefs .

#14 lore raw

PATCH v8 4/4 common: Generic firmware loader for file system

In reply to Marek Vasut
Quoted 72 lines · Marek Vasut
Quoted 31 lines · Tien Fong
Quoted 18 lines · Marek Vasut
Quoted 12 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 2 lines
Quoted 11 lines

common/fs_loader.c line 15

Quoted 9 lines
common/fs_loader.c new file +9 -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>
+#ifdef CONFIG_SPL

Are the ifdefs needed ?

Quoted 6 lines

Because spl.h contains some codes have its dependency with SPL. So, Tom
adviced to make this part of code depend on CONFIG_SPL. However, only __weak int init_mmc() depend on the codes from spl.h, so
user can override their own init_mmc() if SPL is not used.

You probably dont need those ifdefs around headers.

Quoted 24 lines
Quoted 24 lines
Quoted 24 lines
common/fs_loader.c new file +12 -0
+#include <spl.h>
+#endif
+#include <linux/string.h>
+#ifdef CONFIG_CMD_UBIFS
+#include <ubi_uboot.h>
+#include <ubifs_uboot.h>
+#endif
+#include <usb.h>
+
+struct firmware_priv {
+	const char *name;	/* Filename */
+	u32 offset;		/* Offset of reading a file

*/

common/fs_loader.c new file +8 -0
+};
+
+static struct device_location default_locations[] = {
+	{
+		.name = "mmc",
+		.devpart = "0:1",
+	},
+	{
Quoted 8 lines
Quoted 8 lines

common/fs_loader.c line 42

Quoted 7 lines
common/fs_loader.c new file +7 -0
+		.name = "usb",
+		.devpart = "0:1",
+	},
+	{
+		.name = "sata",
+		.devpart = "0:1",
+	},

How can this load from UBI if it's not listed here ?

Quoted 2 lines

Well ?

I can add ".name = ubi" and ".devpart = UBI" .

Quoted 12 lines

[...]

Quoted 10 lines
Quoted 10 lines

common/fs_loader.c line 76

Quoted 7 lines
common/fs_loader.c new file +4 -0
+#ifdef CONFIG_SATA
+static int init_storage_sata(void)
+{
+	return sata_probe(0);

Shouldn't the sata device number be pulled from devpart in default_locations table ?

Quoted 6 lines
Quoted 4 lines

This may need to add the logic for splitting the device number and partition into integer from the string. Let me think how to do it, may
be i can leverage existing code.

strtok(), strtol() or something ?

yes, plan to use that.

Quoted 13 lines

[...]

Quoted 11 lines
Quoted 11 lines

common/fs_loader.c line 123

Quoted 9 lines
common/fs_loader.c new file +4 -0
+#ifdef CONFIG_SPL
+	spl_mmc_find_device(&mmc, spl_boot_device());
+#else
+	debug("#define CONFIG_SPL is required or overrriding

%s\n",

common/fs_loader.c new file +1 -0
+	      __func__);

This can be a compile-time error, maybe ?

Quoted 7 lines
Quoted 3 lines

No compile error. When you open the file, "%s\n" is actually same line
with debug("..... .

So what ? This missing macro can be detected at runtime, heck this code
can even depend on CONFIG_SPL in Kconfig.

Oh...I got you, i thought you means syntax error. So, i would remove it and adding the depend on CONFIG_SPL in Kconfig.

Quoted 25 lines
Quoted 24 lines
Quoted 24 lines
Quoted 23 lines
common/fs_loader.c new file +6 -0
+	return -ENOENT;
+#endif
+
+	err = mmc_init(mmc);
+	if (err) {
+		debug("spl: mmc init failed with error: %d\n",

err);

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

[...]

#15 lore raw

PATCH v8 4/4 common: Generic firmware loader for file system

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

Hi,

Quoted 20 lines · On Mon, 5 Feb 2018 15:06:49 +0800 tien.fong.chee at intel.com wrote:

From: Tien Fong Chee <[email protected]>

This is file system generic loader which can be used to load the file image from the storage into target such as memory. The consumer driver would then use this loader to program whatever, ie. the FPGA device.

---
 common/Kconfig             |   9 ++
 common/Makefile            |   1 +
 common/fs_loader.c         | 320 +++++++++++++++++++++++++++++++++++++++++++++
 doc/README.firmware_loader |  76 +++++++++++
 include/fs_loader.h        |  28 ++++
 5 files changed, 434 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 line 122

Quoted 12 lines
common/fs_loader.c new file +11 -0
+#if defined(CONFIG_SPL_MMC_SUPPORT) && defined(CONFIG_SPL_BUILD)
+__weak int init_mmc(void)
+{
+	/* Just for the case MMC is not yet initialized */
+	struct mmc *mmc = NULL;
+	int err;
+
+#ifdef CONFIG_SPL
+	spl_mmc_find_device(&mmc, spl_boot_device());
+#else
+	debug("#define CONFIG_SPL is required or overrriding %s\n",

s/overrr/overr/

Lothar Waßmann

#16 lore raw

PATCH v8 4/4 common: Generic firmware loader for file system

Quoted 27 lines · Lothar Waßmann

Hi,

Quoted 22 lines · On Mon,  5 Feb 2018 15:06:49 +0800 tien.fong.chee at intel.com wrote:

From: Tien Fong Chee <[email protected]>

This is file system generic loader which can be used to load the file image from the storage into target such as memory. The consumer driver would then use this loader to program whatever, ie. the FPGA device.

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

 5 files changed, 434 insertions(+)

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

[...]

Quoted 1 line
Quoted 15 lines
Quoted 11 lines
common/fs_loader.c new file +11 -0
+#if defined(CONFIG_SPL_MMC_SUPPORT) && defined(CONFIG_SPL_BUILD)
+__weak int init_mmc(void)
+{
+	/* Just for the case MMC is not yet initialized */
+	struct mmc *mmc = NULL;
+	int err;
+
+#ifdef CONFIG_SPL
+	spl_mmc_find_device(&mmc, spl_boot_device());
+#else
+	debug("#define CONFIG_SPL is required or overrriding
Quoted 2 lines

%s\n",

s/overrr/overr/

I would remove it and adding the depend on CONFIG_SPL in Kconfig for compile-time error checking.

Quoted 3 lines

Lothar Waßmann

#17 lore raw

v8 cover Generic firmware loader

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

On Mon, 2018-02-05 at 15:06 +0800, tien.fong.chee at intel.com wrote:
Hi,

Quoted 15 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
Marek Vasut in [v7].

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

Quoted 9 lines

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

v7 -> v8 changes:
-----------------
- Return cmd_ubifs_umount() directly.
- Call the UBI functions directly instead of invoking the U-Boot command parser.

Any comment on this patchset?

Quoted 36 lines

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
[v5]: https://www.mail-archive.com/u-boot at lists.denx.de/msg272771.htm l
[v6]: https://www.mail-archive.com/u-boot at lists.denx.de/msg273961.htm l

Tien Fong Chee (4):
  spl: Remove static declaration on spl_mmc_find_device function
  cmd: ubifs: Move ubifs_initialized checking into cmd_ubifs_umount()
  cmd: ubifs: Factor out some checking codes into cmd_ubifs_mount()
  common: Generic firmware loader for file system

 cmd/ubifs.c                |  40 +++---
 common/Kconfig             |   9 ++
 common/Makefile            |   1 +
 common/fs_loader.c         | 320
+++++++++++++++++++++++++++++++++++++++++++++
 common/spl/spl_mmc.c       |   2 +-
 doc/README.firmware_loader |  76 +++++++++++
 include/fs_loader.h        |  28 ++++
 include/spl.h              |   2 +
 include/ubi_uboot.h        |   2 +

 9 files changed, 462 insertions(+), 18 deletions(-)

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

#18 lore raw

v8 cover Generic firmware loader

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

On Mon, 2018-02-05 at 15:06 +0800, tien.fong.chee at intel.com wrote:
Hi Tom,

Quoted 15 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
Marek Vasut in [v7].

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

Quoted 9 lines

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

v7 -> v8 changes:
-----------------
- Return cmd_ubifs_umount() directly.
- Call the UBI functions directly instead of invoking the U-Boot command parser.

Any comment on this patchset?

Quoted 36 lines

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
[v5]: https://www.mail-archive.com/u-boot at lists.denx.de/msg272771.htm l
[v6]: https://www.mail-archive.com/u-boot at lists.denx.de/msg273961.htm l

Tien Fong Chee (4):
  spl: Remove static declaration on spl_mmc_find_device function
  cmd: ubifs: Move ubifs_initialized checking into cmd_ubifs_umount()
  cmd: ubifs: Factor out some checking codes into cmd_ubifs_mount()
  common: Generic firmware loader for file system

 cmd/ubifs.c                |  40 +++---
 common/Kconfig             |   9 ++
 common/Makefile            |   1 +
 common/fs_loader.c         | 320
+++++++++++++++++++++++++++++++++++++++++++++
 common/spl/spl_mmc.c       |   2 +-
 doc/README.firmware_loader |  76 +++++++++++
 include/fs_loader.h        |  28 ++++
 include/spl.h              |   2 +
 include/ubi_uboot.h        |   2 +

 9 files changed, 462 insertions(+), 18 deletions(-)

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