[U-Boot] [PATCH v6 0/2] Generic firmware loader
From: Tien Fong Chee tien.fong.chee@intel.com
This patchset contains generic firmware loader which is very close to Linux firmware loader but for U-Boot framework. Generic firmware loader can be used load whatever into target location, and then consumer driver would use it to program whatever, ie. the FPGA. This version mainly resolved comments from Lothar Waßmann 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@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@lists.denx.de/msg271905.html [v2]: https://www.mail-archive.com/u-boot@lists.denx.de/msg271979.html [v3]: https://www.mail-archive.com/u-boot@lists.denx.de/msg272039.html [v4]: https://www.mail-archive.com/u-boot@lists.denx.de/msg272432.html
Tien Fong Chee (2): spl: Remove static declaration on spl_mmc_find_device function common: Generic firmware loader for file system
common/Makefile | 1 + common/fs_loader.c | 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
From: Tien Fong Chee tien.fong.chee@intel.com
This patch removes the static declation on spl_mmc_find_device_function so this function is accessible by the caller from other file. This patch is required for later patch.
Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com --- common/spl/spl_mmc.c | 2 +- include/spl.h | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c index b57e0b0..183d05a 100644 --- a/common/spl/spl_mmc.c +++ b/common/spl/spl_mmc.c @@ -114,7 +114,7 @@ static int spl_mmc_get_device_index(u32 boot_device) return -ENODEV; }
-static int spl_mmc_find_device(struct mmc **mmcp, u32 boot_device) +int spl_mmc_find_device(struct mmc **mmcp, u32 boot_device) { #if CONFIG_IS_ENABLED(DM_MMC) struct udevice *dev; diff --git a/include/spl.h b/include/spl.h index 308ce7b..912983a 100644 --- a/include/spl.h +++ b/include/spl.h @@ -10,6 +10,7 @@ /* Platform-specific defines */ #include <linux/compiler.h> #include <asm/spl.h> +#include <mmc.h>
/* Value in r0 indicates we booted from U-Boot */ #define UBOOT_NOT_LOADED_FROM_SPL 0x13578642 @@ -72,6 +73,7 @@ void preloader_console_init(void); u32 spl_boot_device(void); u32 spl_boot_mode(const u32 boot_device); void spl_set_bd(void); +int spl_mmc_find_device(struct mmc **mmcp, u32 boot_device);
/** * spl_set_header_raw_uboot() - Set up a standard SPL image structure
On Wed, Dec 27, 2017 at 01:04:37PM +0800, tien.fong.chee@intel.com wrote:
From: Tien Fong Chee tien.fong.chee@intel.com
This patch removes the static declation on spl_mmc_find_device_function so this function is accessible by the caller from other file. This patch is required for later patch.
Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com
Reviewed-by: Tom Rini trini@konsulko.com
From: Tien Fong Chee tien.fong.chee@intel.com
This is file system generic loader which can be used to load the file image from the storage into target such as memory. The consumer driver would then use this loader to program whatever, ie. the FPGA device.
Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com --- common/Makefile | 1 + common/fs_loader.c | 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
diff --git a/common/Makefile b/common/Makefile index cec506f..2934221 100644 --- a/common/Makefile +++ b/common/Makefile @@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o obj-y += command.o obj-y += s_record.o obj-y += xyzModem.o +obj-y += fs_loader.o diff --git a/common/fs_loader.c b/common/fs_loader.c new file mode 100644 index 0000000..56d29b6 --- /dev/null +++ b/common/fs_loader.c @@ -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> +#include <spi.h> +#include <spi_flash.h> +#include <spl.h> +#include <linux/string.h> +#include <usb.h> + +struct firmware_priv { + const char *name; /* Filename */ + u32 offset; /* Offset of reading a file */ +}; + +static struct device_location default_locations[] = { + { + .name = "mmc", + .devpart = "0:1", + }, + { + .name = "usb", + .devpart = "0:1", + }, + { + .name = "sata", + .devpart = "0:1", + }, +}; + +/* USB build is not supported yet in SPL */ +#ifndef CONFIG_SPL_BUILD +#ifdef CONFIG_USB_STORAGE +static int init_usb(void) +{ + int err; + + err = usb_init(); + if (err) + return err; + +#ifndef CONFIG_DM_USB + err = usb_stor_scan(1) < 0 ? -ENODEV : 0; +#endif + + return err; +} +#else +static int init_usb(void) +{ + printf("Error: Cannot load flash image: no USB support\n"); + return -ENOSYS; +} +#endif +#endif + +#ifdef CONFIG_SATA +static int init_storage_sata(void) +{ + return sata_probe(0); +} +#else +static int init_storage_sata(void) +{ + printf("Error: Cannot load image: no SATA support\n"); + return -ENOSYS; +} +#endif + +#ifdef CONFIG_CMD_UBIFS +static int mount_ubifs(struct device_location *location) +{ + int ret; + char cmd[32]; + + sprintf(cmd, "ubi part %s", location->mtdpart); + + ret = run_command(cmd, 0); + if (ret) + return ret; + + sprintf(cmd, "ubifsmount %s", location->ubivol); + + ret = run_command(cmd, 0); + + return ret; +} + +static int umount_ubifs(void) +{ + return run_command("ubifsumount", 0); +} +#else +static int mount_ubifs(struct device_location *location) +{ + printf("Error: Cannot load image: no UBIFS support\n"); + return -ENOSYS; +} +#endif + +#if defined(CONFIG_SPL_MMC_SUPPORT) && defined(CONFIG_SPL_BUILD) +static int init_mmc(void) +{ + /* Just for the case MMC is not yet initialized */ + struct mmc *mmc = NULL; + int err; + + spl_mmc_find_device(&mmc, spl_boot_device()); + + err = mmc_init(mmc); + if (err) { + printf("spl: mmc init failed with error: %d\n", err); + return err; + } + + return err; +} +#else +static int init_mmc(void) +{ + /* Expect somewhere already initialize MMC */ + return 0; +} +#endif + +static int select_fs_dev(struct device_location *location) +{ + int ret; + + if (!strcmp("mmc", location->name)) { + ret = fs_set_blk_dev("mmc", location->devpart, FS_TYPE_ANY); + } else if (!strcmp("usb", location->name)) { + ret = fs_set_blk_dev("usb", location->devpart, FS_TYPE_ANY); + } else if (!strcmp("sata", location->name)) { + ret = fs_set_blk_dev("sata", location->devpart, FS_TYPE_ANY); + } else if (!strcmp("ubi", location->name)) { + if (location->ubivol != NULL) + ret = fs_set_blk_dev("ubi", NULL, FS_TYPE_UBIFS); + else + ret = -ENODEV; + } else { + printf("Error: unsupported location storage.\n"); + return -ENODEV; + } + + if (ret) + printf("Error: could not access storage.\n"); + + return ret; +} + +static int init_storage_device(struct device_location *location) +{ + int ret; + + if (!strcmp("mmc", location->name)) { + ret = init_mmc(); + } else if (!strcmp("sata", location->name)) { + ret = init_storage_sata(); + } else if (location->ubivol != NULL) { + ret = mount_ubifs(location); +#ifndef CONFIG_SPL_BUILD + /* USB build is not supported yet in SPL */ + } else if (!strcmp("usb", location->name)) { + ret = init_usb(); +#endif + } else { + printf("Error: no supported storage device is available.\n"); + ret = -ENODEV; + } + + return ret; +} + +static void set_storage_devpart(char *name, char *devpart) +{ + size_t i; + + for (i = 0; i < ARRAY_SIZE(default_locations); i++) { + if (!strcmp(default_locations[i].name, name)) + default_locations[i].devpart = devpart; + } +} + +/* + * Prepare firmware struct; + * return -ve if fail. + */ +static int _request_firmware_prepare(struct firmware **firmware_p, + const char *name, void *dbuf, + size_t size, u32 offset) +{ + struct firmware *firmware; + struct firmware_priv *fw_priv; + + *firmware_p = NULL; + + if (!name || name[0] == '\0') + return -EINVAL; + + firmware = calloc(1, sizeof(*firmware)); + if (!firmware) { + printf("%s: calloc(struct firmware) failed\n", __func__); + return -ENOMEM; + } + + fw_priv = calloc(1, sizeof(*fw_priv)); + if (!fw_priv) { + printf("%s: calloc(struct fw_priv) failed\n", __func__); + free(firmware); + return -ENOMEM; + } + + fw_priv->name = name; + fw_priv->offset = offset; + firmware->data = dbuf; + firmware->size = size; + firmware->priv = fw_priv; + *firmware_p = firmware; + + return 0; +} + +/* + * fw_get_filesystem_firmware - load firmware into an allocated buffer + * @location: An array of supported firmware location + * @firmware_p: pointer to firmware image + * + * @return: size of total read + * -ve when error + */ +static int fw_get_filesystem_firmware(struct device_location *location, + struct firmware *firmware_p) +{ + struct firmware_priv *fw_priv = NULL; + loff_t actread; + char *dev_part; + int ret; + + dev_part = env_get("fw_dev_part"); + if (dev_part) + set_storage_devpart(location->name, dev_part); + + ret = init_storage_device(location); + if (ret) + goto out; + + ret = select_fs_dev(location); + if (ret) + goto out; + + fw_priv = firmware_p->priv; + + ret = fs_read(fw_priv->name, (ulong)firmware_p->data, fw_priv->offset, + firmware_p->size, &actread); + if (ret) { + printf("Error: %d Failed to read %s from flash %lld != %d.\n", + ret, fw_priv->name, actread, firmware_p->size); + } else { + ret = actread; + } + +out: +#ifdef CONFIG_CMD_UBIFS + if (location->ubivol != NULL) + umount_ubifs(); +#endif + + return ret; +} + +/* + * request_firmware_into_buf - load firmware into a previously allocated buffer + * @firmware_p: pointer to firmware image + * @name: name of firmware file + * @location: an array of supported firmware location + * @buf: address of buffer to load firmware into + * @size: size of buffer + * @offset: offset of a file for start reading into buffer + * + * The firmware is loaded directly into the buffer pointed to by @buf and + * the @firmware_p data member is pointed at @buf. + * + * @return: size of total read + * -ve when error + */ +int request_firmware_into_buf(struct firmware **firmware_p, + const char *name, + struct device_location *location, + void *buf, size_t size, u32 offset) +{ + int ret; + + ret = _request_firmware_prepare(firmware_p, name, buf, size, offset); + if (ret < 0) /* error */ + return ret; + + ret = fw_get_filesystem_firmware(location, *firmware_p); + + return ret; +} diff --git a/doc/README.firmware_loader b/doc/README.firmware_loader new file mode 100644 index 0000000..f6e4c6b --- /dev/null +++ b/doc/README.firmware_loader @@ -0,0 +1,76 @@ +/* + * Copyright (C) 2017 Intel Corporation <www.intel.com> + * + * SPDX-License-Identifier: GPL-2.0 + */ + +Introduction +------------ +This is firmware loader for U-Boot framework, which has very close to some Linux +Firmware API. For the details of Linux Firmware API, you can refer to +https://01.org/linuxgraphics/gfx-docs/drm/driver-api/firmware/index.html. + +Firmware loader can be used to load whatever(firmware, image, and binary) from +the flash in file system format into target location such as memory, then +consumer driver such as FPGA driver can program FPGA image from the target +location into FPGA. + +Firmware Loader API core features +--------------------------------- +=> Firmware storage device partition search + ---------------------------------------- + => Default storage device partition search set for mmc, usb and sata + as shown in below: + static struct device_location default_locations[] = { + { + .name = "mmc", + .devpart = "0:1", + }, + { + .name = "usb", + .devpart = "0:1", + }, + { + .name = "sata", + .devpart = "0:1", + }, + }; + + However, the default storage device .devpart value can be overwritten + with value which is defined in the environment variable "FW_DEV_PART". + For example: env_set("fw_dev_part", "0:2"); + +Firmware Loader API +------------------- +=> int request_firmware_into_buf(struct firmware **firmware_p, + const char *name, + struct device_location *location, + void *buf, size_t size, u32 offset) + -------------------------------------------------------------- + => Load firmware into a previously allocated buffer + + Parameters: + struct firmware **firmware_p + pointer to firmware image + + const char *name + name of firmware file + + struct device_location *location + an array of supported firmware location + + void *buf + address of buffer to load firmware into + + size_t size + size of buffer + + u32 offset + offset of a file for start reading into buffer + + return: size of total read + -ve when error + + Description: + The firmware is loaded directly into the buffer pointed to by buf and + the @firmware_p data member is pointed at buf. diff --git a/include/fs_loader.h b/include/fs_loader.h new file mode 100644 index 0000000..83a8b80 --- /dev/null +++ b/include/fs_loader.h @@ -0,0 +1,28 @@ +/* + * Copyright (C) 2017 Intel Corporation <www.intel.com> + * + * SPDX-License-Identifier: GPL-2.0 + */ +#ifndef _FS_LOADER_H_ +#define _FS_LOADER_H_ + +#include <linux/types.h> + +struct firmware { + size_t size; /* Size of a file */ + const u8 *data; /* Buffer for file */ + void *priv; /* Firmware loader private fields */ +}; + +struct device_location { + char *name; /* Such as mmc, usb,and sata. */ + char *devpart; /* Use the load command dev:part conventions */ + char *mtdpart; /* MTD partition for ubi part */ + char *ubivol; /* UBI volume-name for ubifsmount */ +}; + +int request_firmware_into_buf(struct firmware **firmware_p, + const char *name, + struct device_location *location, + void *buf, size_t size, u32 offset); +#endif
On Wed, Dec 27, 2017 at 01:04:38PM +0800, tien.fong.chee@intel.com wrote:
From: Tien Fong Chee tien.fong.chee@intel.com
This is file system generic loader which can be used to load the file image from the storage into target such as memory. The consumer driver would then use this loader to program whatever, ie. the FPGA device.
Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com
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]
diff --git a/common/Makefile b/common/Makefile index cec506f..2934221 100644 --- a/common/Makefile +++ b/common/Makefile @@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o obj-y += command.o obj-y += s_record.o obj-y += xyzModem.o +obj-y += fs_loader.o
This needs a new Kconfig option and not to be enabled globally, only when needed.
diff --git a/common/fs_loader.c b/common/fs_loader.c new file mode 100644 index 0000000..56d29b6 --- /dev/null +++ b/common/fs_loader.c @@ -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> +#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]
- if (ret) {
printf("Error: %d Failed to read %s from flash %lld != %d.\n",ret, fw_priv->name, actread, firmware_p->size);
The last %d needs to be %zu since it's a size_t, for portability.
Thanks!
On Mon, 2018-01-15 at 11:36 -0500, Tom Rini wrote:
On Wed, Dec 27, 2017 at 01:04:38PM +0800, tien.fong.chee@intel.com wrote:
From: Tien Fong Chee tien.fong.chee@intel.com
This is file system generic loader which can be used to load the file image from the storage into target such as memory. The consumer driver would then use this loader to program whatever, ie. the FPGA device.
Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com
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.
[snip]
diff --git a/common/Makefile b/common/Makefile index cec506f..2934221 100644 --- a/common/Makefile +++ b/common/Makefile @@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o obj-y += command.o obj-y += s_record.o obj-y += xyzModem.o +obj-y += fs_loader.o
This needs a new Kconfig option and not to be enabled globally, only when needed.
Okay.
diff --git a/common/fs_loader.c b/common/fs_loader.c new file mode 100644 index 0000000..56d29b6 --- /dev/null +++ b/common/fs_loader.c @@ -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> +#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.
[snip]
- if (ret) {
printf("Error: %d Failed to read %s from flash%lld != %d.\n",
ret, fw_priv->name, actread, firmware_p-size);
The last %d needs to be %zu since it's a size_t, for portability.
Thanks!
On Tue, Jan 16, 2018 at 07:58:00AM +0000, Chee, Tien Fong wrote:
On Mon, 2018-01-15 at 11:36 -0500, Tom Rini wrote:
On Wed, Dec 27, 2017 at 01:04:38PM +0800, tien.fong.chee@intel.com wrote:
From: Tien Fong Chee tien.fong.chee@intel.com
This is file system generic loader which can be used to load the file image from the storage into target such as memory. The consumer driver would then use this loader to program whatever, ie. the FPGA device.
Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com
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.
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]
diff --git a/common/fs_loader.c b/common/fs_loader.c new file mode 100644 index 0000000..56d29b6 --- /dev/null +++ b/common/fs_loader.c @@ -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> +#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.
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?
On Tue, 2018-01-16 at 09:35 -0500, Tom Rini wrote:
On Tue, Jan 16, 2018 at 07:58:00AM +0000, Chee, Tien Fong wrote:
On Mon, 2018-01-15 at 11:36 -0500, Tom Rini wrote:
On Wed, Dec 27, 2017 at 01:04:38PM +0800, tien.fong.chee@intel.co m wrote:
From: Tien Fong Chee tien.fong.chee@intel.com
This is file system generic loader which can be used to load the file image from the storage into target such as memory. The consumer driver would then use this loader to program whatever, ie. the FPGA device.
Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com
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.
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.
[snip]
diff --git a/common/fs_loader.c b/common/fs_loader.c new file mode 100644 index 0000000..56d29b6 --- /dev/null +++ b/common/fs_loader.c @@ -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> +#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.
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 [...]
On Thu, Jan 18, 2018 at 03:42:14AM +0000, Chee, Tien Fong wrote:
On Tue, 2018-01-16 at 09:35 -0500, Tom Rini wrote:
On Tue, Jan 16, 2018 at 07:58:00AM +0000, Chee, Tien Fong wrote:
On Mon, 2018-01-15 at 11:36 -0500, Tom Rini wrote:
On Wed, Dec 27, 2017 at 01:04:38PM +0800, tien.fong.chee@intel.co m wrote:
From: Tien Fong Chee tien.fong.chee@intel.com
This is file system generic loader which can be used to load the file image from the storage into target such as memory. The consumer driver would then use this loader to program whatever, ie. the FPGA device.
Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com
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.
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.
That is odd. Are you running it from within the U-Boot tree?
[snip]
diff --git a/common/fs_loader.c b/common/fs_loader.c new file mode 100644 index 0000000..56d29b6 --- /dev/null +++ b/common/fs_loader.c @@ -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> +#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.
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 [...]
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
On Thu, 2018-01-18 at 08:18 -0500, Tom Rini wrote:
On Thu, Jan 18, 2018 at 03:42:14AM +0000, Chee, Tien Fong wrote:
On Tue, 2018-01-16 at 09:35 -0500, Tom Rini wrote:
On Tue, Jan 16, 2018 at 07:58:00AM +0000, Chee, Tien Fong wrote:
On Mon, 2018-01-15 at 11:36 -0500, Tom Rini wrote:
On Wed, Dec 27, 2017 at 01:04:38PM +0800, tien.fong.chee@inte l.co m wrote:
From: Tien Fong Chee tien.fong.chee@intel.com
This is file system generic loader which can be used to load the file image from the storage into target such as memory. The consumer driver would then use this loader to program whatever, ie. the FPGA device.
Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com
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.
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.
That is odd. Are you running it from within the U-Boot tree?
Yes.
[snip]
diff --git a/common/fs_loader.c b/common/fs_loader.c new file mode 100644 index 0000000..56d29b6 --- /dev/null +++ b/common/fs_loader.c @@ -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> +#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.
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 [...]
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).
Ok, you are right.
It's OK to restructure your code to have: #ifdef CONFIG_SPL #include <spl.h> ... SPL specific functions #endif
Okay, noted.
On 12/27/2017 06:04 AM, tien.fong.chee@intel.com wrote:
Whoa, this improved substantially since last time I checked. Minor nitpicks below.
[...]
+/* USB build is not supported yet in SPL */ +#ifndef CONFIG_SPL_BUILD +#ifdef CONFIG_USB_STORAGE +static int init_usb(void) +{
- int err;
- err = usb_init();
- if (err)
return err;+#ifndef CONFIG_DM_USB
- err = usb_stor_scan(1) < 0 ? -ENODEV : 0;
if (err) return err; ?
+#endif
- return err;
+} +#else +static int init_usb(void) +{
- printf("Error: Cannot load flash image: no USB support\n");
debug() ? Fix globally ...
- return -ENOSYS;
+} +#endif +#endif
+#ifdef CONFIG_SATA +static int init_storage_sata(void) +{
- return sata_probe(0);
+} +#else +static int init_storage_sata(void) +{
- printf("Error: Cannot load image: no SATA support\n");
- return -ENOSYS;
+} +#endif
+#ifdef CONFIG_CMD_UBIFS +static int mount_ubifs(struct device_location *location) +{
- int ret;
- char cmd[32];
- sprintf(cmd, "ubi part %s", location->mtdpart);
snprintf() ...
- ret = run_command(cmd, 0);
- if (ret)
return ret;- sprintf(cmd, "ubifsmount %s", location->ubivol);
- ret = run_command(cmd, 0);
- return ret;
+}
+static int umount_ubifs(void) +{
- return run_command("ubifsumount", 0);
Just call the function directly ?
+} +#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);elseret = -ENODEV;- } else {
printf("Error: unsupported location storage.\n");return -ENODEV;- }
- if (ret)
printf("Error: could not access storage.\n");- return ret;
+}
+static int init_storage_device(struct device_location *location) +{
- int ret;
- if (!strcmp("mmc", location->name)) {
ret = init_mmc();- } else if (!strcmp("sata", location->name)) {
ret = init_storage_sata();- } else if (location->ubivol != NULL) {
ret = mount_ubifs(location);+#ifndef CONFIG_SPL_BUILD
- /* USB build is not supported yet in SPL */
- } else if (!strcmp("usb", location->name)) {
ret = init_usb();+#endif
- } else {
printf("Error: no supported storage device is available.\n");ret = -ENODEV;- }
- return ret;
+}
+static void set_storage_devpart(char *name, char *devpart) +{
- size_t i;
- for (i = 0; i < ARRAY_SIZE(default_locations); i++) {
if (!strcmp(default_locations[i].name, name))default_locations[i].devpart = devpart;- }
+}
+/*
- Prepare firmware struct;
- return -ve if fail.
Use kerneldoc and keep it consistent.
- */
+static int _request_firmware_prepare(struct firmware **firmware_p,
const char *name, void *dbuf,size_t size, u32 offset)+{
- struct firmware *firmware;
- struct firmware_priv *fw_priv;
- *firmware_p = NULL;
- if (!name || name[0] == '\0')
return -EINVAL;- firmware = calloc(1, sizeof(*firmware));
- if (!firmware) {
printf("%s: calloc(struct firmware) failed\n", __func__);
If malloc fails, you're screwed anyway and printf will likely fail too, so drop it.
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 Tue, 2018-01-16 at 15:41 +0100, Marek Vasut wrote:
On 12/27/2017 06:04 AM, tien.fong.chee@intel.com wrote:
Whoa, this improved substantially since last time I checked. Minor nitpicks below.
[...]
+/* USB build is not supported yet in SPL */ +#ifndef CONFIG_SPL_BUILD +#ifdef CONFIG_USB_STORAGE +static int init_usb(void) +{
- int err;
- err = usb_init();
- if (err)
return err;+#ifndef CONFIG_DM_USB
- err = usb_stor_scan(1) < 0 ? -ENODEV : 0;
if (err) return err; ?
This is last line code of the function, so it's always return the result regardless error or not.
+#endif
- return err;
+} +#else +static int init_usb(void) +{
- printf("Error: Cannot load flash image: no USB
support\n");
debug() ? Fix globally ...
okay.
- return -ENOSYS;
+} +#endif +#endif
+#ifdef CONFIG_SATA +static int init_storage_sata(void) +{
- return sata_probe(0);
+} +#else +static int init_storage_sata(void) +{
- printf("Error: Cannot load image: no SATA support\n");
- return -ENOSYS;
+} +#endif
+#ifdef CONFIG_CMD_UBIFS +static int mount_ubifs(struct device_location *location) +{
- int ret;
- char cmd[32];
- sprintf(cmd, "ubi part %s", location->mtdpart);
snprintf() ...
okay.
- ret = run_command(cmd, 0);
- if (ret)
return ret;- sprintf(cmd, "ubifsmount %s", location->ubivol);
- ret = run_command(cmd, 0);
- return ret;
+}
+static int umount_ubifs(void) +{
- return run_command("ubifsumount", 0);
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.
+} +#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);
elseret = -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 isavailable.\n");
ret = -ENODEV;- }
- return ret;
+}
+static void set_storage_devpart(char *name, char *devpart) +{
- size_t i;
- for (i = 0; i < ARRAY_SIZE(default_locations); i++) {
if (!strcmp(default_locations[i].name, name))default_locations[i].devpart = devpart;- }
+}
+/*
- Prepare firmware struct;
- return -ve if fail.
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?
- */
+static int _request_firmware_prepare(struct firmware **firmware_p,
const char *name, void *dbuf,size_t size, u32 offset)+{
- struct firmware *firmware;
- struct firmware_priv *fw_priv;
- *firmware_p = NULL;
- if (!name || name[0] == '\0')
return -EINVAL;- firmware = calloc(1, sizeof(*firmware));
- if (!firmware) {
printf("%s: calloc(struct firmware) failed\n",__func__);
If malloc fails, you're screwed anyway and printf will likely fail too, so drop it.
okay.
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 01/18/2018 05:33 AM, Chee, Tien Fong wrote:
On Tue, 2018-01-16 at 15:41 +0100, Marek Vasut wrote:
On 12/27/2017 06:04 AM, tien.fong.chee@intel.com wrote:
Whoa, this improved substantially since last time I checked. Minor nitpicks below.
[...]
+/* USB build is not supported yet in SPL */ +#ifndef CONFIG_SPL_BUILD +#ifdef CONFIG_USB_STORAGE +static int init_usb(void) +{
- int err;
- err = usb_init();
- if (err)
return err;+#ifndef CONFIG_DM_USB
- err = usb_stor_scan(1) < 0 ? -ENODEV : 0;
if (err) return err; ?
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.
+#endif
- return err;
+} +#else +static int init_usb(void) +{
- printf("Error: Cannot load flash image: no USB
support\n");
debug() ? Fix globally ...
okay.
- return -ENOSYS;
+} +#endif +#endif
+#ifdef CONFIG_SATA +static int init_storage_sata(void) +{
- return sata_probe(0);
+} +#else +static int init_storage_sata(void) +{
- printf("Error: Cannot load image: no SATA support\n");
- return -ENOSYS;
+} +#endif
+#ifdef CONFIG_CMD_UBIFS +static int mount_ubifs(struct device_location *location) +{
- int ret;
- char cmd[32];
- sprintf(cmd, "ubi part %s", location->mtdpart);
snprintf() ...
okay.
- ret = run_command(cmd, 0);
- if (ret)
return ret;- sprintf(cmd, "ubifsmount %s", location->ubivol);
- ret = run_command(cmd, 0);
- return ret;
+}
+static int umount_ubifs(void) +{
- return run_command("ubifsumount", 0);
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.
Then factor those out into a function you can all and call that function.
+} +#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);
elseret = -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 isavailable.\n");
ret = -ENODEV;- }
- return ret;
+}
+static void set_storage_devpart(char *name, char *devpart) +{
- size_t i;
- for (i = 0; i < ARRAY_SIZE(default_locations); i++) {
if (!strcmp(default_locations[i].name, name))default_locations[i].devpart = devpart;- }
+}
+/*
- Prepare firmware struct;
- return -ve if fail.
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?
Kerneldoc specifies the format of the comment, so that documentation can be generated from it. If you document functions, use that format.
- */
+static int _request_firmware_prepare(struct firmware **firmware_p,
const char *name, void *dbuf,size_t size, u32 offset)+{
- struct firmware *firmware;
- struct firmware_priv *fw_priv;
- *firmware_p = NULL;
- if (!name || name[0] == '\0')
return -EINVAL;- firmware = calloc(1, sizeof(*firmware));
- if (!firmware) {
printf("%s: calloc(struct firmware) failed\n",__func__);
If malloc fails, you're screwed anyway and printf will likely fail too, so drop it.
okay.
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 Thu, 2018-01-18 at 12:12 +0100, Marek Vasut wrote:
On 01/18/2018 05:33 AM, Chee, Tien Fong wrote:
On Tue, 2018-01-16 at 15:41 +0100, Marek Vasut wrote:
On 12/27/2017 06:04 AM, tien.fong.chee@intel.com wrote:
Whoa, this improved substantially since last time I checked. Minor nitpicks below.
[...]
+/* USB build is not supported yet in SPL */ +#ifndef CONFIG_SPL_BUILD +#ifdef CONFIG_USB_STORAGE +static int init_usb(void) +{
- int err;
- err = usb_init();
- if (err)
return err;+#ifndef CONFIG_DM_USB
- err = usb_stor_scan(1) < 0 ? -ENODEV : 0;
if (err) return err; ?
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;
+#endif
- return err;
+} +#else +static int init_usb(void) +{
- printf("Error: Cannot load flash image: no USB
support\n");
debug() ? Fix globally ...
okay.
- return -ENOSYS;
+} +#endif +#endif
+#ifdef CONFIG_SATA +static int init_storage_sata(void) +{
- return sata_probe(0);
+} +#else +static int init_storage_sata(void) +{
- printf("Error: Cannot load image: no SATA support\n");
- return -ENOSYS;
+} +#endif
+#ifdef CONFIG_CMD_UBIFS +static int mount_ubifs(struct device_location *location) +{
- int ret;
- char cmd[32];
- sprintf(cmd, "ubi part %s", location->mtdpart);
snprintf() ...
okay.
- ret = run_command(cmd, 0);
- if (ret)
return ret;- sprintf(cmd, "ubifsmount %s", location->ubivol);
- ret = run_command(cmd, 0);
- return ret;
+}
+static int umount_ubifs(void) +{
- return run_command("ubifsumount", 0);
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.
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?
+} +#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);
elseret = -ENODEV;- } else {
printf("Error: unsupported locationstorage.\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 isavailable.\n");
ret = -ENODEV;- }
- return ret;
+}
+static void set_storage_devpart(char *name, char *devpart) +{
- size_t i;
- for (i = 0; i < ARRAY_SIZE(default_locations); i++) {
if (!strcmp(default_locations[i].name, name))default_locations[i].devpart =devpart;
- }
+}
+/*
- Prepare firmware struct;
- return -ve if fail.
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?
Kerneldoc specifies the format of the comment, so that documentation can be generated from it. If you document functions, use that format.
Okay.
- */
+static int _request_firmware_prepare(struct firmware **firmware_p,
const char *name, void*dbuf,
size_t size, u32 offset)+{
- struct firmware *firmware;
- struct firmware_priv *fw_priv;
- *firmware_p = NULL;
- if (!name || name[0] == '\0')
return -EINVAL;- firmware = calloc(1, sizeof(*firmware));
- if (!firmware) {
printf("%s: calloc(struct firmware) failed\n",__func__);
If malloc fails, you're screwed anyway and printf will likely fail too, so drop it.
okay.
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;
+}
[...]
Hi,
On Mon, 22 Jan 2018 07:11:37 +0000 Chee, Tien Fong wrote:
On Thu, 2018-01-18 at 12:12 +0100, Marek Vasut wrote:
On 01/18/2018 05:33 AM, Chee, Tien Fong wrote:
On Tue, 2018-01-16 at 15:41 +0100, Marek Vasut wrote:
On 12/27/2017 06:04 AM, tien.fong.chee@intel.com wrote:
Whoa, this improved substantially since last time I checked. Minor nitpicks below.
[...]
+/* USB build is not supported yet in SPL */ +#ifndef CONFIG_SPL_BUILD +#ifdef CONFIG_USB_STORAGE +static int init_usb(void) +{
- int err;
- err = usb_init();
- if (err)
return err;+#ifndef CONFIG_DM_USB
- err = usb_stor_scan(1) < 0 ? -ENODEV : 0;
if (err) return err; ?
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;
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
On Mon, 2018-01-22 at 09:44 +0100, Lothar Waßmann wrote:
Hi,
On Mon, 22 Jan 2018 07:11:37 +0000 Chee, Tien Fong wrote:
On Thu, 2018-01-18 at 12:12 +0100, Marek Vasut wrote:
On 01/18/2018 05:33 AM, Chee, Tien Fong wrote:
On Tue, 2018-01-16 at 15:41 +0100, Marek Vasut wrote:
On 12/27/2017 06:04 AM, tien.fong.chee@intel.com wrote:
Whoa, this improved substantially since last time I checked. Minor nitpicks below.
[...]
+/* USB build is not supported yet in SPL */ +#ifndef CONFIG_SPL_BUILD +#ifdef CONFIG_USB_STORAGE +static int init_usb(void) +{
- int err;
- err = usb_init();
- if (err)
return err;+#ifndef CONFIG_DM_USB
- err = usb_stor_scan(1) < 0 ? -ENODEV : 0;
if (err) return err; ?
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;
usb_stor_scan() does not return a sensible error code, but '-1' if no device was found. This should be changed to -ENODEV then!
Okay, so this should be fixed in usb_stor_scan() function.
Lothar Waßmann
On 01/22/2018 08:11 AM, Chee, Tien Fong wrote:
[...]
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;
Right
[...]
+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.
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?
It just makes no sense to involve the whole command machinery if you can call a function which does exactly the same.
[...]
On 27.12.2017 06:04, tien.fong.chee@intel.com wrote:
From: Tien Fong Chee tien.fong.chee@intel.com
This is file system generic loader which can be used to load the file image from the storage into target such as memory. The consumer driver would then use this loader to program whatever, ie. the FPGA device.
Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com
common/Makefile | 1 + common/fs_loader.c | 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>
+#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;
+}
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
On Thu, 2018-01-18 at 06:57 +0100, Simon Goldschmidt wrote:
On 27.12.2017 06:04, tien.fong.chee@intel.com wrote:
From: Tien Fong Chee tien.fong.chee@intel.com
This is file system generic loader which can be used to load the file image from the storage into target such as memory. The consumer driver would then use this loader to program whatever, ie. the FPGA device.
Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com
common/Makefile | 1 + common/fs_loader.c | 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>
+#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;
+}
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.
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.
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.
Regards, Simon
On 22.01.2018 09:08, Chee, Tien Fong wrote:
On Thu, 2018-01-18 at 06:57 +0100, Simon Goldschmidt wrote:
On 27.12.2017 06:04, tien.fong.chee@intel.com wrote:
From: Tien Fong Chee tien.fong.chee@intel.com
This is file system generic loader which can be used to load the file image from the storage into target such as memory. The consumer driver would then use this loader to program whatever, ie. the FPGA device.
Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com
common/Makefile | 1 + common/fs_loader.c | 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>
+#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;
+}
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.
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.
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
On Mon, 2018-01-22 at 12:41 +0100, Simon Goldschmidt wrote:
On 22.01.2018 09:08, Chee, Tien Fong wrote:
On Thu, 2018-01-18 at 06:57 +0100, Simon Goldschmidt wrote:
On 27.12.2017 06:04, tien.fong.chee@intel.com wrote:
From: Tien Fong Chee tien.fong.chee@intel.com
This is file system generic loader which can be used to load the file image from the storage into target such as memory. The consumer driver would then use this loader to program whatever, ie. the FPGA device.
Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com
common/Makefile | 1 + common/fs_loader.c | 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>
+#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;
+}
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.
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.
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?
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.
Regards, Simon
On 23.01.2018 07:28, Chee, Tien Fong wrote:
On Mon, 2018-01-22 at 12:41 +0100, Simon Goldschmidt wrote:
On 22.01.2018 09:08, Chee, Tien Fong wrote:
On Thu, 2018-01-18 at 06:57 +0100, Simon Goldschmidt wrote:
On 27.12.2017 06:04, tien.fong.chee@intel.com wrote:
From: Tien Fong Chee tien.fong.chee@intel.com
This is file system generic loader which can be used to load the file image from the storage into target such as memory. The consumer driver would then use this loader to program whatever, ie. the FPGA device.
Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com
common/Makefile | 1 + common/fs_loader.c | 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>
+#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;
+}
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.
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.
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?
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.
You're right on that. Thinking about it, I would probably use it from U-Boot, not from SPL...
Lastly, i can declare __Weak to init_mmc, so that user can define their own implementation.
At least somehow allowing to override which device is passed to 'spl_mmc_find_device' might be good, instead of hardcoding it to 'spl_boot_device'...
Regards, Simon
On 27.12.2017 06:04, tien.fong.chee@intel.com wrote:
From: Tien Fong Chee tien.fong.chee@intel.com
This is file system generic loader which can be used to load the file image from the storage into target such as memory. The consumer driver would then use this loader to program whatever, ie. the FPGA device.
Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com
common/Makefile | 1 + common/fs_loader.c | 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
diff --git a/common/Makefile b/common/Makefile index cec506f..2934221 100644 --- a/common/Makefile +++ b/common/Makefile @@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o
<snip>
diff --git a/include/fs_loader.h b/include/fs_loader.h new file mode 100644 index 0000000..83a8b80 --- /dev/null +++ b/include/fs_loader.h @@ -0,0 +1,28 @@ +/*
- Copyright (C) 2017 Intel Corporation <www.intel.com>
- SPDX-License-Identifier: GPL-2.0
- */
+#ifndef _FS_LOADER_H_ +#define _FS_LOADER_H_
+#include <linux/types.h>
+struct firmware {
- size_t size; /* Size of a file */
- const u8 *data; /* Buffer for file */
- void *priv; /* Firmware loader private fields */
+};
+struct device_location {
- char *name; /* Such as mmc, usb,and sata. */
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
- 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
On Tue, 2018-01-23 at 08:58 +0100, Simon Goldschmidt wrote:
On 27.12.2017 06:04, tien.fong.chee@intel.com wrote:
From: Tien Fong Chee tien.fong.chee@intel.com
This is file system generic loader which can be used to load the file image from the storage into target such as memory. The consumer driver would then use this loader to program whatever, ie. the FPGA device.
Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com
common/Makefile | 1 + common/fs_loader.c | 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
diff --git a/common/Makefile b/common/Makefile index cec506f..2934221 100644 --- a/common/Makefile +++ b/common/Makefile @@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o
<snip>
diff --git a/include/fs_loader.h b/include/fs_loader.h new file mode 100644 index 0000000..83a8b80 --- /dev/null +++ b/include/fs_loader.h @@ -0,0 +1,28 @@ +/*
- Copyright (C) 2017 Intel Corporation <www.intel.com>
- SPDX-License-Identifier: GPL-2.0
- */
+#ifndef _FS_LOADER_H_ +#define _FS_LOADER_H_
+#include <linux/types.h>
+struct firmware {
- size_t size; /* Size of a file */
- const u8 *data; /* Buffer for file */
- void *priv; /* Firmware loader private
fields */ +};
+struct device_location {
- char *name; /* Such as mmc, usb,and sata. */
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.
Regards, Simon
- 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
On 23.01.2018 09:31, Chee, Tien Fong wrote:
On Tue, 2018-01-23 at 08:58 +0100, Simon Goldschmidt wrote:
On 27.12.2017 06:04, tien.fong.chee@intel.com wrote:
From: Tien Fong Chee tien.fong.chee@intel.com
This is file system generic loader which can be used to load the file image from the storage into target such as memory. The consumer driver would then use this loader to program whatever, ie. the FPGA device.
Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com
common/Makefile | 1 + common/fs_loader.c | 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
diff --git a/common/Makefile b/common/Makefile index cec506f..2934221 100644 --- a/common/Makefile +++ b/common/Makefile @@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o
<snip>
diff --git a/include/fs_loader.h b/include/fs_loader.h new file mode 100644 index 0000000..83a8b80 --- /dev/null +++ b/include/fs_loader.h @@ -0,0 +1,28 @@ +/*
- Copyright (C) 2017 Intel Corporation <www.intel.com>
- SPDX-License-Identifier: GPL-2.0
- */
+#ifndef _FS_LOADER_H_ +#define _FS_LOADER_H_
+#include <linux/types.h>
+struct firmware {
- size_t size; /* Size of a file */
- const u8 *data; /* Buffer for file */
- void *priv; /* Firmware loader private
fields */ +};
+struct device_location {
- char *name; /* Such as mmc, usb,and sata. */
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.
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.
Regards, Simon
- 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
On Tue, 2018-01-23 at 10:13 +0100, Simon Goldschmidt wrote:
On 23.01.2018 09:31, Chee, Tien Fong wrote:
On Tue, 2018-01-23 at 08:58 +0100, Simon Goldschmidt wrote:
On 27.12.2017 06:04, tien.fong.chee@intel.com wrote:
From: Tien Fong Chee tien.fong.chee@intel.com
This is file system generic loader which can be used to load the file image from the storage into target such as memory. The consumer driver would then use this loader to program whatever, ie. the FPGA device.
Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com
common/Makefile | 1 + common/fs_loader.c | 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
diff --git a/common/Makefile b/common/Makefile index cec506f..2934221 100644 --- a/common/Makefile +++ b/common/Makefile @@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o
<snip>
diff --git a/include/fs_loader.h b/include/fs_loader.h new file mode 100644 index 0000000..83a8b80 --- /dev/null +++ b/include/fs_loader.h @@ -0,0 +1,28 @@ +/*
- Copyright (C) 2017 Intel Corporation <www.intel.com>
- SPDX-License-Identifier: GPL-2.0
- */
+#ifndef _FS_LOADER_H_ +#define _FS_LOADER_H_
+#include <linux/types.h>
+struct firmware {
- size_t size; /* Size of a file */
- const u8 *data; /* Buffer for file */
- void *priv; /* Firmware loader private
fields */ +};
+struct device_location {
- char *name; /* Such as mmc, usb,and sata. */
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.
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.
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.
Regards, Simon
- 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, u32offset); +#endif
On 24.01.2018 06:13, Chee, Tien Fong wrote:
On Tue, 2018-01-23 at 10:13 +0100, Simon Goldschmidt wrote:
On 23.01.2018 09:31, Chee, Tien Fong wrote:
On Tue, 2018-01-23 at 08:58 +0100, Simon Goldschmidt wrote:
On 27.12.2017 06:04, tien.fong.chee@intel.com wrote:
From: Tien Fong Chee tien.fong.chee@intel.com
This is file system generic loader which can be used to load the file image from the storage into target such as memory. The consumer driver would then use this loader to program whatever, ie. the FPGA device.
Signed-off-by: Tien Fong Chee tien.fong.chee@intel.com
common/Makefile | 1 + common/fs_loader.c | 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
diff --git a/common/Makefile b/common/Makefile index cec506f..2934221 100644 --- a/common/Makefile +++ b/common/Makefile @@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o
<snip>
diff --git a/include/fs_loader.h b/include/fs_loader.h new file mode 100644 index 0000000..83a8b80 --- /dev/null +++ b/include/fs_loader.h @@ -0,0 +1,28 @@ +/*
- Copyright (C) 2017 Intel Corporation <www.intel.com>
- SPDX-License-Identifier: GPL-2.0
- */
+#ifndef _FS_LOADER_H_ +#define _FS_LOADER_H_
+#include <linux/types.h>
+struct firmware {
- size_t size; /* Size of a file */
- const u8 *data; /* Buffer for file */
- void *priv; /* Firmware loader private
fields */ +};
+struct device_location {
- char *name; /* Such as mmc, usb,and sata. */
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.
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.
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:
- console environment, most commands using those interface name as
their arguments in characters string format such as FPGA loadfs command.
- These command interface name is one of the characters string
argument of fs_set_blk_dev too.
- As identity of default location and its dev_part configuration.
Right. I see that there does not seem to be a way to provide the interface type to fs.h without using a string. In most cases, this string is provided by the environment or via console, so I see where that comes from. It's probably not up to this patch to change it.
Regards, Simon
- 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, u32offset); +#endif
On Rab, 2017-12-27 at 13:04 +0800, tien.fong.chee@intel.com wrote: Hi Lothar Waßmann,
From: Tien Fong Chee tien.fong.chee@intel.com
This patchset contains generic firmware loader which is very close to Linux firmware loader but for U-Boot framework. Generic firmware loader can be used load whatever into target location, and then consumer driver would use it to program whatever, ie. the FPGA. This version mainly resolved comments from Lothar Waßmann in [v5].
Are you OK with the patches?
This series is working on top of u-boot.git - http://git.denx.de/u-boot.git .
l
v5 -> v6 changes:
- Return error code when fs_read is fail.
- Return actual read size when fs_read is success.
Patchset history
l [v2]: https://www.mail-archive.com/u-boot@lists.denx.de/msg271979.htm l [v3]: https://www.mail-archive.com/u-boot@lists.denx.de/msg272039.htm l [v4]: https://www.mail-archive.com/u-boot@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
Hi,
On Wed, 3 Jan 2018 08:46:09 +0000 Chee, Tien Fong wrote:
On Rab, 2017-12-27 at 13:04 +0800, tien.fong.chee@intel.com wrote: Hi Lothar Waßmann,
From: Tien Fong Chee tien.fong.chee@intel.com
This patchset contains generic firmware loader which is very close to Linux firmware loader but for U-Boot framework. Generic firmware loader can be used load whatever into target location, and then consumer driver would use it to program whatever, ie. the FPGA. This version mainly resolved comments from Lothar Waßmann in [v5].
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 LW@KARO-electronics.de
Lothar Waßmann
On Isn, 2018-01-08 at 09:07 +0100, Lothar Waßmann wrote:
Hi,
On Wed, 3 Jan 2018 08:46:09 +0000 Chee, Tien Fong wrote:
On Rab, 2017-12-27 at 13:04 +0800, tien.fong.chee@intel.com wrote: Hi Lothar Waßmann,
From: Tien Fong Chee tien.fong.chee@intel.com
This patchset contains generic firmware loader which is very close to Linux firmware loader but for U-Boot framework. Generic firmware loader can be used load whatever into target location, and then consumer driver would use it to program whatever, ie. the FPGA. This version mainly resolved comments from Lothar Waßmann in [v5].
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 LW@KARO-electronics.de
Great, thanks for helping to review the codes.
Lothar Waßmann
On Mon, 2018-01-08 at 09:07 +0100, Lothar Waßmann wrote: Hi Tom Rini,
Hi,
On Wed, 3 Jan 2018 08:46:09 +0000 Chee, Tien Fong wrote:
On Rab, 2017-12-27 at 13:04 +0800, tien.fong.chee@intel.com wrote: Hi Lothar Waßmann,
From: Tien Fong Chee tien.fong.chee@intel.com
This patchset contains generic firmware loader which is very close to Linux firmware loader but for U-Boot framework. Generic firmware loader can be used load whatever into target location, and then consumer driver would use it to program whatever, ie. the FPGA. This version mainly resolved comments from Lothar Waßmann in [v5].
Are you OK with the patches?
Inlcude Tom Rini for reviewing this patch.
Sorry for the delay. I have been on vacation until now. The patch series Looks good to me now.
Reviewed-By: Lothar Waßmann LW@KARO-electronics.de
Lothar Waßmann
On Wed, 2017-12-27 at 13:04 +0800, tien.fong.chee@intel.com wrote: Hi Tom Rini,
From: Tien Fong Chee tien.fong.chee@intel.com
This patchset contains generic firmware loader which is very close to Linux firmware loader but for U-Boot framework. Generic firmware loader can be used load whatever into target location, and then consumer driver would use it to program whatever, ie. the FPGA. This version mainly resolved comments from Lothar Waßmann in [v5].
This series is working on top of u-boot.git - http://git.denx.de/u-boot.git .
Do you ok with this patchset?
Best regards, TF
boot@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
l [v2]: https://www.mail-archive.com/u-boot@lists.denx.de/msg271979.htm l [v3]: https://www.mail-archive.com/u-boot@lists.denx.de/msg272039.htm l [v4]: https://www.mail-archive.com/u-boot@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
participants (6)
-
Chee, Tien Fong -
Lothar Waßmann -
Marek Vasut -
Simon Goldschmidt -
tien.fong.chee＠intel.com
-
Tom Rini