[PATCH v3 0/3] net: fm: Add support for loading firmware from filesystem
This adds support for loading Fman firmware from a filesystem using the firmware loader subsystem. It was originally part of [1], but has been split off because it is conceptually separate.
[1] https://lore.kernel.org/u-boot/20220324182306.2037094-1-sean.anderson@seco.c...
Changes in v3: - Rebased onto u-boot/next
Changes in v2: - Split series into two
Sean Anderson (3): misc: fs_loader: Add function to get the chosen loader net: fm: Add firmware name parameter net: fm: Support loading firmware from a filesystem
arch/arm/mach-k3/common.c | 2 +- arch/arm/mach-omap2/boot-common.c | 2 +- drivers/fpga/socfpga_arria10.c | 24 ++----------------- drivers/misc/fs_loader.c | 27 +++++++++++++++++++++ drivers/net/fm/fm.c | 40 +++++++++++++++++++++++++++---- drivers/net/fm/fm.h | 2 +- drivers/qe/Kconfig | 4 ++++ include/fs_loader.h | 12 ++++++++++ 8 files changed, 84 insertions(+), 29 deletions(-)
The fs_loader device is used to pull in settings via the chosen node. However, there was no library function for this, so arria10 was doing it explicitly. This function subsumes that, and uses ofnode_get_chosen_node instead of navigating the device tree directly. Because fs_loader pulls its config from the environment by default, it's fine to create a device with nothing backing it at all. Doing this allows enabling CONFIG_FS_LOADER without needing to modify the device tree.
Signed-off-by: Sean Anderson sean.anderson@seco.com Reviewed-by: Simon Glass sjg@chromium.org Reviewed-by: Ramon Fried rfried.dev@gmail.com ---
(no changes since v1)
arch/arm/mach-k3/common.c | 2 +- arch/arm/mach-omap2/boot-common.c | 2 +- drivers/fpga/socfpga_arria10.c | 24 ++---------------------- drivers/misc/fs_loader.c | 27 +++++++++++++++++++++++++++ include/fs_loader.h | 12 ++++++++++++ 5 files changed, 43 insertions(+), 24 deletions(-)
diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c index d5e1f8e2e7..a2adb791f6 100644 --- a/arch/arm/mach-k3/common.c +++ b/arch/arm/mach-k3/common.c @@ -181,7 +181,7 @@ int load_firmware(char *name_fw, char *name_loadaddr, u32 *loadaddr) if (!*loadaddr) return 0;
- if (!uclass_get_device(UCLASS_FS_FIRMWARE_LOADER, 0, &fsdev)) { + if (!get_fs_loader(&fsdev)) { size = request_firmware_into_buf(fsdev, name, (void *)*loadaddr, 0, 0); } diff --git a/arch/arm/mach-omap2/boot-common.c b/arch/arm/mach-omap2/boot-common.c index d104f23b3e..9a342a1bf9 100644 --- a/arch/arm/mach-omap2/boot-common.c +++ b/arch/arm/mach-omap2/boot-common.c @@ -214,7 +214,7 @@ int load_firmware(char *name_fw, u32 *loadaddr) if (!*loadaddr) return 0;
- if (!uclass_get_device(UCLASS_FS_FIRMWARE_LOADER, 0, &fsdev)) { + if (!get_fs_loader(&fsdev)) { size = request_firmware_into_buf(fsdev, name_fw, (void *)*loadaddr, 0, 0); } diff --git a/drivers/fpga/socfpga_arria10.c b/drivers/fpga/socfpga_arria10.c index 3b785e67d0..b69107aa33 100644 --- a/drivers/fpga/socfpga_arria10.c +++ b/drivers/fpga/socfpga_arria10.c @@ -787,32 +787,12 @@ int socfpga_loadfs(fpga_fs_info *fpga_fsinfo, const void *buf, size_t bsize, u32 phandle;
node = get_fpga_mgr_ofnode(ofnode_null()); - - if (ofnode_valid(node)) { - phandle_p = ofnode_get_property(node, "firmware-loader", &size); - if (!phandle_p) { - node = ofnode_path("/chosen"); - if (!ofnode_valid(node)) { - debug("FPGA: /chosen node was not found.\n"); - return -ENOENT; - } - - phandle_p = ofnode_get_property(node, "firmware-loader", - &size); - if (!phandle_p) { - debug("FPGA: firmware-loader property was not"); - debug(" found.\n"); - return -ENOENT; - } - } - } else { + if (!ofnode_valid(node)) { debug("FPGA: FPGA manager node was not found.\n"); return -ENOENT; }
- phandle = fdt32_to_cpu(*phandle_p); - ret = uclass_get_device_by_phandle_id(UCLASS_FS_FIRMWARE_LOADER, - phandle, &dev); + ret = get_fs_loader(&dev); if (ret) return ret;
diff --git a/drivers/misc/fs_loader.c b/drivers/misc/fs_loader.c index 5b4d03639c..ccf5c7a803 100644 --- a/drivers/misc/fs_loader.c +++ b/drivers/misc/fs_loader.c @@ -15,6 +15,8 @@ #include <fs_loader.h> #include <log.h> #include <asm/global_data.h> +#include <dm/device-internal.h> +#include <dm/root.h> #include <linux/string.h> #include <mapmem.h> #include <malloc.h> @@ -297,6 +299,31 @@ U_BOOT_DRIVER(fs_loader) = { .priv_auto = sizeof(struct firmware), };
+static struct device_plat default_plat = { 0 }; + +int get_fs_loader(struct udevice **dev) +{ + int ret; + ofnode node = ofnode_get_chosen_node("firmware-loader"); + + if (ofnode_valid(node)) + return uclass_get_device_by_ofnode(UCLASS_FS_FIRMWARE_LOADER, + node, dev); + + /* Try the first device if none was chosen */ + ret = uclass_first_device_err(UCLASS_FS_FIRMWARE_LOADER, dev); + if (ret != -ENODEV) + return ret; + + /* Just create a new device */ + ret = device_bind(dm_root(), DM_DRIVER_GET(fs_loader), "default-loader", + &default_plat, ofnode_null(), dev); + if (ret) + return ret; + + return device_probe(*dev); +} + UCLASS_DRIVER(fs_loader) = { .id = UCLASS_FS_FIRMWARE_LOADER, .name = "fs-loader", diff --git a/include/fs_loader.h b/include/fs_loader.h index 8de7cb18dc..5eb5b7ab4a 100644 --- a/include/fs_loader.h +++ b/include/fs_loader.h @@ -52,4 +52,16 @@ struct device_plat { int request_firmware_into_buf(struct udevice *dev, const char *name, void *buf, size_t size, u32 offset); + +/** + * get_fs_loader() - Get the chosen filesystem loader + * @dev: Where to store the device + * + * This gets a filesystem loader device based on the value of + * /chosen/firmware-loader. If no such property exists, it returns a + * firmware loader which is configured by environmental variables. + * + * Return: 0 on success, negative value on error + */ +int get_fs_loader(struct udevice **dev); #endif
On Thu, Dec 29, 2022 at 11:52:59AM -0500, Sean Anderson wrote:
The fs_loader device is used to pull in settings via the chosen node. However, there was no library function for this, so arria10 was doing it explicitly. This function subsumes that, and uses ofnode_get_chosen_node instead of navigating the device tree directly. Because fs_loader pulls its config from the environment by default, it's fine to create a device with nothing backing it at all. Doing this allows enabling CONFIG_FS_LOADER without needing to modify the device tree.
Signed-off-by: Sean Anderson sean.anderson@seco.com Reviewed-by: Simon Glass sjg@chromium.org Reviewed-by: Ramon Fried rfried.dev@gmail.com
Applied to u-boot/master, thanks!
In order to read the firmware from the filesystem, we need a file name. Read the firmware name from the device tree, using the firmware-name property. This property is commonly used in Linux to determine the correct name to use (and can be seen in several device trees in U-Boot).
Signed-off-by: Sean Anderson sean.anderson@seco.com Reviewed-by: Ramon Fried rfried.dev@gmail.com ---
(no changes since v1)
drivers/net/fm/fm.c | 15 ++++++++++++--- drivers/net/fm/fm.h | 2 +- 2 files changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/net/fm/fm.c b/drivers/net/fm/fm.c index 055dd61fbe..457200e766 100644 --- a/drivers/net/fm/fm.c +++ b/drivers/net/fm/fm.c @@ -8,6 +8,7 @@ #include <image.h> #include <malloc.h> #include <asm/io.h> +#include <dm/device_compat.h> #include <linux/errno.h> #include <u-boot/crc.h> #include <dm.h> @@ -353,7 +354,7 @@ static void fm_init_qmi(struct fm_qmi_common *qmi)
/* Init common part of FM, index is fm num# like fm as above */ #ifdef CONFIG_TFABOOT -int fm_init_common(int index, struct ccsr_fman *reg) +int fm_init_common(int index, struct ccsr_fman *reg, const char *firmware_name) { int rc; void *addr = NULL; @@ -448,7 +449,7 @@ int fm_init_common(int index, struct ccsr_fman *reg) return fm_init_bmi(index, ®->fm_bmi_common); } #else -int fm_init_common(int index, struct ccsr_fman *reg) +int fm_init_common(int index, struct ccsr_fman *reg, const char *firmware_name) { int rc; #if defined(CONFIG_SYS_QE_FMAN_FW_IN_NOR) @@ -561,6 +562,8 @@ static const struct udevice_id fman_ids[] = {
static int fman_probe(struct udevice *dev) { + const char *firmware_name = NULL; + int ret; struct fman_priv *priv = dev_get_priv(dev);
priv->reg = (struct ccsr_fman *)(uintptr_t)dev_read_addr(dev); @@ -570,7 +573,13 @@ static int fman_probe(struct udevice *dev) return -EINVAL; }
- return fm_init_common(priv->fman_id, priv->reg); + ret = dev_read_string_index(dev, "firmware-name", 0, &firmware_name); + if (ret && ret != -EINVAL) { + dev_dbg(dev, "Could not read firmware-name\n"); + return ret; + } + + return fm_init_common(priv->fman_id, priv->reg, firmware_name); }
static int fman_remove(struct udevice *dev) diff --git a/drivers/net/fm/fm.h b/drivers/net/fm/fm.h index ba858cc24b..a2d5b03429 100644 --- a/drivers/net/fm/fm.h +++ b/drivers/net/fm/fm.h @@ -106,7 +106,7 @@ struct fm_port_global_pram {
void *fm_muram_alloc(int fm_idx, size_t size, ulong align); void *fm_muram_base(int fm_idx); -int fm_init_common(int index, struct ccsr_fman *reg); +int fm_init_common(int index, struct ccsr_fman *reg, const char *firmware_name); int fm_eth_initialize(struct ccsr_fman *reg, struct fm_eth_info *info); phy_interface_t fman_port_enet_if(enum fm_port port); void fman_disable_port(enum fm_port port);
Hi Sean,
On Thu, 29 Dec 2022 at 10:53, Sean Anderson sean.anderson@seco.com wrote:
In order to read the firmware from the filesystem, we need a file name. Read the firmware name from the device tree, using the firmware-name property. This property is commonly used in Linux to determine the correct name to use (and can be seen in several device trees in U-Boot).
Signed-off-by: Sean Anderson sean.anderson@seco.com Reviewed-by: Ramon Fried rfried.dev@gmail.com
(no changes since v1)
drivers/net/fm/fm.c | 15 ++++++++++++--- drivers/net/fm/fm.h | 2 +- 2 files changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/net/fm/fm.c b/drivers/net/fm/fm.c index 055dd61fbe..457200e766 100644 --- a/drivers/net/fm/fm.c +++ b/drivers/net/fm/fm.c @@ -8,6 +8,7 @@ #include <image.h> #include <malloc.h> #include <asm/io.h> +#include <dm/device_compat.h> #include <linux/errno.h> #include <u-boot/crc.h> #include <dm.h> @@ -353,7 +354,7 @@ static void fm_init_qmi(struct fm_qmi_common *qmi)
/* Init common part of FM, index is fm num# like fm as above */ #ifdef CONFIG_TFABOOT -int fm_init_common(int index, struct ccsr_fman *reg) +int fm_init_common(int index, struct ccsr_fman *reg, const char *firmware_name) { int rc; void *addr = NULL; @@ -448,7 +449,7 @@ int fm_init_common(int index, struct ccsr_fman *reg) return fm_init_bmi(index, ®->fm_bmi_common); } #else -int fm_init_common(int index, struct ccsr_fman *reg) +int fm_init_common(int index, struct ccsr_fman *reg, const char *firmware_name) { int rc; #if defined(CONFIG_SYS_QE_FMAN_FW_IN_NOR) @@ -561,6 +562,8 @@ static const struct udevice_id fman_ids[] = {
static int fman_probe(struct udevice *dev) {
const char *firmware_name = NULL;int ret; struct fman_priv *priv = dev_get_priv(dev); priv->reg = (struct ccsr_fman *)(uintptr_t)dev_read_addr(dev);@@ -570,7 +573,13 @@ static int fman_probe(struct udevice *dev) return -EINVAL; }
return fm_init_common(priv->fman_id, priv->reg);
ret = dev_read_string_index(dev, "firmware-name", 0, &firmware_name);if (ret && ret != -EINVAL) {dev_dbg(dev, "Could not read firmware-name\n");return ret;}return fm_init_common(priv->fman_id, priv->reg, firmware_name);}
static int fman_remove(struct udevice *dev) diff --git a/drivers/net/fm/fm.h b/drivers/net/fm/fm.h index ba858cc24b..a2d5b03429 100644 --- a/drivers/net/fm/fm.h +++ b/drivers/net/fm/fm.h @@ -106,7 +106,7 @@ struct fm_port_global_pram {
void *fm_muram_alloc(int fm_idx, size_t size, ulong align); void *fm_muram_base(int fm_idx); -int fm_init_common(int index, struct ccsr_fman *reg); +int fm_init_common(int index, struct ccsr_fman *reg, const char *firmware_name);
Please add a full function comment.
int fm_eth_initialize(struct ccsr_fman *reg, struct fm_eth_info *info); phy_interface_t fman_port_enet_if(enum fm_port port); void fman_disable_port(enum fm_port port); -- 2.35.1.1320.gc452695387.dirty
Regards, Simon
On 12/29/22 17:38, Simon Glass wrote:
Hi Sean,
On Thu, 29 Dec 2022 at 10:53, Sean Anderson sean.anderson@seco.com wrote:
In order to read the firmware from the filesystem, we need a file name. Read the firmware name from the device tree, using the firmware-name property. This property is commonly used in Linux to determine the correct name to use (and can be seen in several device trees in U-Boot).
Signed-off-by: Sean Anderson sean.anderson@seco.com Reviewed-by: Ramon Fried rfried.dev@gmail.com
(no changes since v1)
drivers/net/fm/fm.c | 15 ++++++++++++--- drivers/net/fm/fm.h | 2 +- 2 files changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/net/fm/fm.c b/drivers/net/fm/fm.c index 055dd61fbe..457200e766 100644 --- a/drivers/net/fm/fm.c +++ b/drivers/net/fm/fm.c @@ -8,6 +8,7 @@ #include <image.h> #include <malloc.h> #include <asm/io.h> +#include <dm/device_compat.h> #include <linux/errno.h> #include <u-boot/crc.h> #include <dm.h> @@ -353,7 +354,7 @@ static void fm_init_qmi(struct fm_qmi_common *qmi)
/* Init common part of FM, index is fm num# like fm as above */ #ifdef CONFIG_TFABOOT -int fm_init_common(int index, struct ccsr_fman *reg) +int fm_init_common(int index, struct ccsr_fman *reg, const char *firmware_name) { int rc; void *addr = NULL; @@ -448,7 +449,7 @@ int fm_init_common(int index, struct ccsr_fman *reg) return fm_init_bmi(index, ®->fm_bmi_common); } #else -int fm_init_common(int index, struct ccsr_fman *reg) +int fm_init_common(int index, struct ccsr_fman *reg, const char *firmware_name) { int rc; #if defined(CONFIG_SYS_QE_FMAN_FW_IN_NOR) @@ -561,6 +562,8 @@ static const struct udevice_id fman_ids[] = {
static int fman_probe(struct udevice *dev) {
const char *firmware_name = NULL;int ret; struct fman_priv *priv = dev_get_priv(dev); priv->reg = (struct ccsr_fman *)(uintptr_t)dev_read_addr(dev);@@ -570,7 +573,13 @@ static int fman_probe(struct udevice *dev) return -EINVAL; }
return fm_init_common(priv->fman_id, priv->reg);
ret = dev_read_string_index(dev, "firmware-name", 0, &firmware_name);if (ret && ret != -EINVAL) {dev_dbg(dev, "Could not read firmware-name\n");return ret;}return fm_init_common(priv->fman_id, priv->reg, firmware_name);}
static int fman_remove(struct udevice *dev) diff --git a/drivers/net/fm/fm.h b/drivers/net/fm/fm.h index ba858cc24b..a2d5b03429 100644 --- a/drivers/net/fm/fm.h +++ b/drivers/net/fm/fm.h @@ -106,7 +106,7 @@ struct fm_port_global_pram {
void *fm_muram_alloc(int fm_idx, size_t size, ulong align); void *fm_muram_base(int fm_idx); -int fm_init_common(int index, struct ccsr_fman *reg); +int fm_init_common(int index, struct ccsr_fman *reg, const char *firmware_name);
Please add a full function comment.
I don't think that's really necessary. This is called from one place, and serves mostly to organize the code (now that non-DM net is gone).
--Sean
Hi Sean,
On Fri, 30 Dec 2022 at 09:32, Sean Anderson sean.anderson@seco.com wrote:
On 12/29/22 17:38, Simon Glass wrote:
Hi Sean,
On Thu, 29 Dec 2022 at 10:53, Sean Anderson sean.anderson@seco.com wrote:
In order to read the firmware from the filesystem, we need a file name. Read the firmware name from the device tree, using the firmware-name property. This property is commonly used in Linux to determine the correct name to use (and can be seen in several device trees in U-Boot).
Signed-off-by: Sean Anderson sean.anderson@seco.com Reviewed-by: Ramon Fried rfried.dev@gmail.com
(no changes since v1)
drivers/net/fm/fm.c | 15 ++++++++++++--- drivers/net/fm/fm.h | 2 +- 2 files changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/net/fm/fm.c b/drivers/net/fm/fm.c index 055dd61fbe..457200e766 100644 --- a/drivers/net/fm/fm.c +++ b/drivers/net/fm/fm.c @@ -8,6 +8,7 @@ #include <image.h> #include <malloc.h> #include <asm/io.h> +#include <dm/device_compat.h> #include <linux/errno.h> #include <u-boot/crc.h> #include <dm.h> @@ -353,7 +354,7 @@ static void fm_init_qmi(struct fm_qmi_common *qmi)
/* Init common part of FM, index is fm num# like fm as above */ #ifdef CONFIG_TFABOOT -int fm_init_common(int index, struct ccsr_fman *reg) +int fm_init_common(int index, struct ccsr_fman *reg, const char *firmware_name) { int rc; void *addr = NULL; @@ -448,7 +449,7 @@ int fm_init_common(int index, struct ccsr_fman *reg) return fm_init_bmi(index, ®->fm_bmi_common); } #else -int fm_init_common(int index, struct ccsr_fman *reg) +int fm_init_common(int index, struct ccsr_fman *reg, const char *firmware_name) { int rc; #if defined(CONFIG_SYS_QE_FMAN_FW_IN_NOR) @@ -561,6 +562,8 @@ static const struct udevice_id fman_ids[] = {
static int fman_probe(struct udevice *dev) {
const char *firmware_name = NULL;int ret; struct fman_priv *priv = dev_get_priv(dev); priv->reg = (struct ccsr_fman *)(uintptr_t)dev_read_addr(dev);@@ -570,7 +573,13 @@ static int fman_probe(struct udevice *dev) return -EINVAL; }
return fm_init_common(priv->fman_id, priv->reg);
ret = dev_read_string_index(dev, "firmware-name", 0, &firmware_name);if (ret && ret != -EINVAL) {dev_dbg(dev, "Could not read firmware-name\n");return ret;}return fm_init_common(priv->fman_id, priv->reg, firmware_name);}
static int fman_remove(struct udevice *dev) diff --git a/drivers/net/fm/fm.h b/drivers/net/fm/fm.h index ba858cc24b..a2d5b03429 100644 --- a/drivers/net/fm/fm.h +++ b/drivers/net/fm/fm.h @@ -106,7 +106,7 @@ struct fm_port_global_pram {
void *fm_muram_alloc(int fm_idx, size_t size, ulong align); void *fm_muram_base(int fm_idx); -int fm_init_common(int index, struct ccsr_fman *reg); +int fm_init_common(int index, struct ccsr_fman *reg, const char *firmware_name);
Please add a full function comment.
I don't think that's really necessary. This is called from one place, and serves mostly to organize the code (now that non-DM net is gone).
As a matter of good practice, all exported functions should be documented in their header files. May as well start now, can perhaps include the header docs in an rST file.
Regards, Simon
On 12/30/22 12:51, Simon Glass wrote:
Hi Sean,
On Fri, 30 Dec 2022 at 09:32, Sean Anderson sean.anderson@seco.com wrote:
On 12/29/22 17:38, Simon Glass wrote:
Hi Sean,
On Thu, 29 Dec 2022 at 10:53, Sean Anderson sean.anderson@seco.com wrote:
In order to read the firmware from the filesystem, we need a file name. Read the firmware name from the device tree, using the firmware-name property. This property is commonly used in Linux to determine the correct name to use (and can be seen in several device trees in U-Boot).
Signed-off-by: Sean Anderson sean.anderson@seco.com Reviewed-by: Ramon Fried rfried.dev@gmail.com
(no changes since v1)
drivers/net/fm/fm.c | 15 ++++++++++++--- drivers/net/fm/fm.h | 2 +- 2 files changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/net/fm/fm.c b/drivers/net/fm/fm.c index 055dd61fbe..457200e766 100644 --- a/drivers/net/fm/fm.c +++ b/drivers/net/fm/fm.c @@ -8,6 +8,7 @@ #include <image.h> #include <malloc.h> #include <asm/io.h> +#include <dm/device_compat.h> #include <linux/errno.h> #include <u-boot/crc.h> #include <dm.h> @@ -353,7 +354,7 @@ static void fm_init_qmi(struct fm_qmi_common *qmi)
/* Init common part of FM, index is fm num# like fm as above */ #ifdef CONFIG_TFABOOT -int fm_init_common(int index, struct ccsr_fman *reg) +int fm_init_common(int index, struct ccsr_fman *reg, const char *firmware_name) { int rc; void *addr = NULL; @@ -448,7 +449,7 @@ int fm_init_common(int index, struct ccsr_fman *reg) return fm_init_bmi(index, ®->fm_bmi_common); } #else -int fm_init_common(int index, struct ccsr_fman *reg) +int fm_init_common(int index, struct ccsr_fman *reg, const char *firmware_name) { int rc; #if defined(CONFIG_SYS_QE_FMAN_FW_IN_NOR) @@ -561,6 +562,8 @@ static const struct udevice_id fman_ids[] = {
static int fman_probe(struct udevice *dev) {
const char *firmware_name = NULL;int ret; struct fman_priv *priv = dev_get_priv(dev); priv->reg = (struct ccsr_fman *)(uintptr_t)dev_read_addr(dev);@@ -570,7 +573,13 @@ static int fman_probe(struct udevice *dev) return -EINVAL; }
return fm_init_common(priv->fman_id, priv->reg);
ret = dev_read_string_index(dev, "firmware-name", 0, &firmware_name);if (ret && ret != -EINVAL) {dev_dbg(dev, "Could not read firmware-name\n");return ret;}return fm_init_common(priv->fman_id, priv->reg, firmware_name);}
static int fman_remove(struct udevice *dev) diff --git a/drivers/net/fm/fm.h b/drivers/net/fm/fm.h index ba858cc24b..a2d5b03429 100644 --- a/drivers/net/fm/fm.h +++ b/drivers/net/fm/fm.h @@ -106,7 +106,7 @@ struct fm_port_global_pram {
void *fm_muram_alloc(int fm_idx, size_t size, ulong align); void *fm_muram_base(int fm_idx); -int fm_init_common(int index, struct ccsr_fman *reg); +int fm_init_common(int index, struct ccsr_fman *reg, const char *firmware_name);
Please add a full function comment.
I don't think that's really necessary. This is called from one place, and serves mostly to organize the code (now that non-DM net is gone).
As a matter of good practice, all exported functions should be documented in their header files. May as well start now, can perhaps include the header docs in an rST file.
Well, it's not actually used in any other file any more, so I propose just marking it as static and removing it from the header.
--Sean
Hi Sean,
On Fri, 30 Dec 2022 at 11:53, Sean Anderson sean.anderson@seco.com wrote:
On 12/30/22 12:51, Simon Glass wrote:
Hi Sean,
On Fri, 30 Dec 2022 at 09:32, Sean Anderson sean.anderson@seco.com wrote:
On 12/29/22 17:38, Simon Glass wrote:
Hi Sean,
On Thu, 29 Dec 2022 at 10:53, Sean Anderson sean.anderson@seco.com wrote:
In order to read the firmware from the filesystem, we need a file name. Read the firmware name from the device tree, using the firmware-name property. This property is commonly used in Linux to determine the correct name to use (and can be seen in several device trees in U-Boot).
Signed-off-by: Sean Anderson sean.anderson@seco.com Reviewed-by: Ramon Fried rfried.dev@gmail.com
(no changes since v1)
drivers/net/fm/fm.c | 15 ++++++++++++--- drivers/net/fm/fm.h | 2 +- 2 files changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/net/fm/fm.c b/drivers/net/fm/fm.c index 055dd61fbe..457200e766 100644 --- a/drivers/net/fm/fm.c +++ b/drivers/net/fm/fm.c @@ -8,6 +8,7 @@ #include <image.h> #include <malloc.h> #include <asm/io.h> +#include <dm/device_compat.h> #include <linux/errno.h> #include <u-boot/crc.h> #include <dm.h> @@ -353,7 +354,7 @@ static void fm_init_qmi(struct fm_qmi_common *qmi)
/* Init common part of FM, index is fm num# like fm as above */ #ifdef CONFIG_TFABOOT -int fm_init_common(int index, struct ccsr_fman *reg) +int fm_init_common(int index, struct ccsr_fman *reg, const char *firmware_name) { int rc; void *addr = NULL; @@ -448,7 +449,7 @@ int fm_init_common(int index, struct ccsr_fman *reg) return fm_init_bmi(index, ®->fm_bmi_common); } #else -int fm_init_common(int index, struct ccsr_fman *reg) +int fm_init_common(int index, struct ccsr_fman *reg, const char *firmware_name) { int rc; #if defined(CONFIG_SYS_QE_FMAN_FW_IN_NOR) @@ -561,6 +562,8 @@ static const struct udevice_id fman_ids[] = {
static int fman_probe(struct udevice *dev) {
const char *firmware_name = NULL;int ret; struct fman_priv *priv = dev_get_priv(dev); priv->reg = (struct ccsr_fman *)(uintptr_t)dev_read_addr(dev);@@ -570,7 +573,13 @@ static int fman_probe(struct udevice *dev) return -EINVAL; }
return fm_init_common(priv->fman_id, priv->reg);
ret = dev_read_string_index(dev, "firmware-name", 0, &firmware_name);if (ret && ret != -EINVAL) {dev_dbg(dev, "Could not read firmware-name\n");return ret;}return fm_init_common(priv->fman_id, priv->reg, firmware_name);}
static int fman_remove(struct udevice *dev) diff --git a/drivers/net/fm/fm.h b/drivers/net/fm/fm.h index ba858cc24b..a2d5b03429 100644 --- a/drivers/net/fm/fm.h +++ b/drivers/net/fm/fm.h @@ -106,7 +106,7 @@ struct fm_port_global_pram {
void *fm_muram_alloc(int fm_idx, size_t size, ulong align); void *fm_muram_base(int fm_idx); -int fm_init_common(int index, struct ccsr_fman *reg); +int fm_init_common(int index, struct ccsr_fman *reg, const char *firmware_name);
Please add a full function comment.
I don't think that's really necessary. This is called from one place, and serves mostly to organize the code (now that non-DM net is gone).
As a matter of good practice, all exported functions should be documented in their header files. May as well start now, can perhaps include the header docs in an rST file.
Well, it's not actually used in any other file any more, so I propose just marking it as static and removing it from the header.
OK
Regards, Simon
On Thu, Dec 29, 2022 at 11:53:00AM -0500, Sean Anderson wrote:
In order to read the firmware from the filesystem, we need a file name. Read the firmware name from the device tree, using the firmware-name property. This property is commonly used in Linux to determine the correct name to use (and can be seen in several device trees in U-Boot).
Signed-off-by: Sean Anderson sean.anderson@seco.com Reviewed-by: Ramon Fried rfried.dev@gmail.com
Applied to u-boot/master, thanks!
This adds a new method to load Fman firmware from a filesystem. This allows users to use regular files instead of hard-coded offsets for the firmware.
Signed-off-by: Sean Anderson sean.anderson@seco.com Reviewed-by: Ramon Fried rfried.dev@gmail.com ---
(no changes since v1)
drivers/net/fm/fm.c | 25 ++++++++++++++++++++++++- drivers/qe/Kconfig | 4 ++++ 2 files changed, 28 insertions(+), 1 deletion(-)
diff --git a/drivers/net/fm/fm.c b/drivers/net/fm/fm.c index 457200e766..e1fba24471 100644 --- a/drivers/net/fm/fm.c +++ b/drivers/net/fm/fm.c @@ -5,6 +5,7 @@ */ #include <common.h> #include <env.h> +#include <fs_loader.h> #include <image.h> #include <malloc.h> #include <asm/io.h> @@ -452,7 +453,29 @@ int fm_init_common(int index, struct ccsr_fman *reg, const char *firmware_name) int fm_init_common(int index, struct ccsr_fman *reg, const char *firmware_name) { int rc; -#if defined(CONFIG_SYS_QE_FMAN_FW_IN_NOR) +#if defined(CONFIG_SYS_QE_FMAN_FW_IN_FS) + struct udevice *fs_loader; + void *addr = malloc(CONFIG_SYS_QE_FMAN_FW_LENGTH); + + if (!addr) + return -ENOMEM; + + rc = get_fs_loader(&fs_loader); + if (rc) { + debug("could not get fs loader: %d\n", rc); + return rc; + } + + if (!firmware_name) + firmware_name = "fman.itb"; + + rc = request_firmware_into_buf(fs_loader, firmware_name, addr, + CONFIG_SYS_QE_FMAN_FW_LENGTH, 0); + if (rc < 0) { + debug("could not request %s: %d\n", firmware_name, rc); + return rc; + } +#elif defined(CONFIG_SYS_QE_FMAN_FW_IN_NOR) void *addr = (void *)CONFIG_SYS_FMAN_FW_ADDR; #elif defined(CONFIG_SYS_QE_FMAN_FW_IN_NAND) size_t fw_length = CONFIG_SYS_QE_FMAN_FW_LENGTH; diff --git a/drivers/qe/Kconfig b/drivers/qe/Kconfig index c44a81f69a..89a75c175b 100644 --- a/drivers/qe/Kconfig +++ b/drivers/qe/Kconfig @@ -27,6 +27,10 @@ choice depends on FMAN_ENET || QE default SYS_QE_FMAN_FW_IN_ROM
+config SYS_QE_FMAN_FW_IN_FS + depends on FS_LOADER && FMAN_ENET + bool "Filesystem" + config SYS_QE_FMAN_FW_IN_NOR bool "NOR flash"
Hi Sean,
On Thu, 29 Dec 2022 at 10:54, Sean Anderson sean.anderson@seco.com wrote:
This adds a new method to load Fman firmware from a filesystem. This allows users to use regular files instead of hard-coded offsets for the firmware.
Signed-off-by: Sean Anderson sean.anderson@seco.com Reviewed-by: Ramon Fried rfried.dev@gmail.com
(no changes since v1)
drivers/net/fm/fm.c | 25 ++++++++++++++++++++++++- drivers/qe/Kconfig | 4 ++++ 2 files changed, 28 insertions(+), 1 deletion(-)
diff --git a/drivers/net/fm/fm.c b/drivers/net/fm/fm.c index 457200e766..e1fba24471 100644 --- a/drivers/net/fm/fm.c +++ b/drivers/net/fm/fm.c @@ -5,6 +5,7 @@ */ #include <common.h> #include <env.h> +#include <fs_loader.h> #include <image.h> #include <malloc.h> #include <asm/io.h> @@ -452,7 +453,29 @@ int fm_init_common(int index, struct ccsr_fman *reg, const char *firmware_name) int fm_init_common(int index, struct ccsr_fman *reg, const char *firmware_name) { int rc; -#if defined(CONFIG_SYS_QE_FMAN_FW_IN_NOR) +#if defined(CONFIG_SYS_QE_FMAN_FW_IN_FS)
Cam this use C code?
struct udevice *fs_loader;void *addr = malloc(CONFIG_SYS_QE_FMAN_FW_LENGTH);
For this you can use something like:
IF_ENABLED_INT(CONFIG_SYS_QE_FMAN_FW_IN_FS, CONFIG_SYS_QE_FMAN_FW_LENGTH)
so that C works
if (!addr)return -ENOMEM;rc = get_fs_loader(&fs_loader);if (rc) {debug("could not get fs loader: %d\n", rc);return rc;}if (!firmware_name)firmware_name = "fman.itb";rc = request_firmware_into_buf(fs_loader, firmware_name, addr,CONFIG_SYS_QE_FMAN_FW_LENGTH, 0);if (rc < 0) {debug("could not request %s: %d\n", firmware_name, rc);return rc;}+#elif defined(CONFIG_SYS_QE_FMAN_FW_IN_NOR) void *addr = (void *)CONFIG_SYS_FMAN_FW_ADDR; #elif defined(CONFIG_SYS_QE_FMAN_FW_IN_NAND) size_t fw_length = CONFIG_SYS_QE_FW_LENGTH; diff --git a/drivers/qe/Kconfig b/drivers/qe/Kconfig index c44a81f69a..89a75c175b 100644 --- a/drivers/qe/Kconfig +++ b/drivers/qe/Kconfig @@ -27,6 +27,10 @@ choice depends on FMAN_ENET || QE default SYS_QE_FMAN_FW_IN_ROM
+config SYS_QE_FMAN_FW_IN_FS
depends on FS_LOADER && FMAN_ENETbool "Filesystem"
Should this be a choice? In any case it needs some decent help! e
config SYS_QE_FMAN_FW_IN_NOR bool "NOR flash"
-- 2.35.1.1320.gc452695387.dirty
Regards, Simon
On 12/29/22 17:38, Simon Glass wrote:
Hi Sean,
On Thu, 29 Dec 2022 at 10:54, Sean Anderson sean.anderson@seco.com wrote:
This adds a new method to load Fman firmware from a filesystem. This allows users to use regular files instead of hard-coded offsets for the firmware.
Signed-off-by: Sean Anderson sean.anderson@seco.com Reviewed-by: Ramon Fried rfried.dev@gmail.com
(no changes since v1)
drivers/net/fm/fm.c | 25 ++++++++++++++++++++++++- drivers/qe/Kconfig | 4 ++++ 2 files changed, 28 insertions(+), 1 deletion(-)
diff --git a/drivers/net/fm/fm.c b/drivers/net/fm/fm.c index 457200e766..e1fba24471 100644 --- a/drivers/net/fm/fm.c +++ b/drivers/net/fm/fm.c @@ -5,6 +5,7 @@ */ #include <common.h> #include <env.h> +#include <fs_loader.h> #include <image.h> #include <malloc.h> #include <asm/io.h> @@ -452,7 +453,29 @@ int fm_init_common(int index, struct ccsr_fman *reg, const char *firmware_name) int fm_init_common(int index, struct ccsr_fman *reg, const char *firmware_name) { int rc; -#if defined(CONFIG_SYS_QE_FMAN_FW_IN_NOR) +#if defined(CONFIG_SYS_QE_FMAN_FW_IN_FS)
Cam this use C code?
I'll look into it...
struct udevice *fs_loader;void *addr = malloc(CONFIG_SYS_QE_FMAN_FW_LENGTH);For this you can use something like:
IF_ENABLED_INT(CONFIG_SYS_QE_FMAN_FW_IN_FS, CONFIG_SYS_QE_FMAN_FW_LENGTH)
so that C works
if (!addr)return -ENOMEM;rc = get_fs_loader(&fs_loader);if (rc) {debug("could not get fs loader: %d\n", rc);return rc;}if (!firmware_name)firmware_name = "fman.itb";rc = request_firmware_into_buf(fs_loader, firmware_name, addr,CONFIG_SYS_QE_FMAN_FW_LENGTH, 0);if (rc < 0) {debug("could not request %s: %d\n", firmware_name, rc);return rc;}+#elif defined(CONFIG_SYS_QE_FMAN_FW_IN_NOR) void *addr = (void *)CONFIG_SYS_FMAN_FW_ADDR; #elif defined(CONFIG_SYS_QE_FMAN_FW_IN_NAND) size_t fw_length = CONFIG_SYS_QE_FW_LENGTH; diff --git a/drivers/qe/Kconfig b/drivers/qe/Kconfig index c44a81f69a..89a75c175b 100644 --- a/drivers/qe/Kconfig +++ b/drivers/qe/Kconfig @@ -27,6 +27,10 @@ choice depends on FMAN_ENET || QE default SYS_QE_FMAN_FW_IN_ROM
+config SYS_QE_FMAN_FW_IN_FS
depends on FS_LOADER && FMAN_ENETbool "Filesystem"Should this be a choice?
It is.
In any case it needs some decent help!
I think it's reasonable in context (the choice is "QUICC Engine FMan ethernet firmware location"). It's in the same (terse) style as the rest of the file.
--Sean
config SYS_QE_FMAN_FW_IN_NOR bool "NOR flash"
-- 2.35.1.1320.gc452695387.dirty
Regards, Simon
Hi Sean,
On Fri, 30 Dec 2022 at 09:36, Sean Anderson sean.anderson@seco.com wrote:
On 12/29/22 17:38, Simon Glass wrote:
Hi Sean,
On Thu, 29 Dec 2022 at 10:54, Sean Anderson sean.anderson@seco.com wrote:
This adds a new method to load Fman firmware from a filesystem. This allows users to use regular files instead of hard-coded offsets for the firmware.
Signed-off-by: Sean Anderson sean.anderson@seco.com Reviewed-by: Ramon Fried rfried.dev@gmail.com
(no changes since v1)
drivers/net/fm/fm.c | 25 ++++++++++++++++++++++++- drivers/qe/Kconfig | 4 ++++ 2 files changed, 28 insertions(+), 1 deletion(-)
diff --git a/drivers/net/fm/fm.c b/drivers/net/fm/fm.c index 457200e766..e1fba24471 100644 --- a/drivers/net/fm/fm.c +++ b/drivers/net/fm/fm.c @@ -5,6 +5,7 @@ */ #include <common.h> #include <env.h> +#include <fs_loader.h> #include <image.h> #include <malloc.h> #include <asm/io.h> @@ -452,7 +453,29 @@ int fm_init_common(int index, struct ccsr_fman *reg, const char *firmware_name) int fm_init_common(int index, struct ccsr_fman *reg, const char *firmware_name) { int rc; -#if defined(CONFIG_SYS_QE_FMAN_FW_IN_NOR) +#if defined(CONFIG_SYS_QE_FMAN_FW_IN_FS)
Cam this use C code?
I'll look into it...
struct udevice *fs_loader;void *addr = malloc(CONFIG_SYS_QE_FMAN_FW_LENGTH);For this you can use something like:
IF_ENABLED_INT(CONFIG_SYS_QE_FMAN_FW_IN_FS, CONFIG_SYS_QE_FMAN_FW_LENGTH)
so that C works
if (!addr)return -ENOMEM;rc = get_fs_loader(&fs_loader);if (rc) {debug("could not get fs loader: %d\n", rc);return rc;}if (!firmware_name)firmware_name = "fman.itb";rc = request_firmware_into_buf(fs_loader, firmware_name, addr,CONFIG_SYS_QE_FMAN_FW_LENGTH, 0);if (rc < 0) {debug("could not request %s: %d\n", firmware_name, rc);return rc;}+#elif defined(CONFIG_SYS_QE_FMAN_FW_IN_NOR) void *addr = (void *)CONFIG_SYS_FMAN_FW_ADDR; #elif defined(CONFIG_SYS_QE_FMAN_FW_IN_NAND) size_t fw_length = CONFIG_SYS_QE_FW_LENGTH; diff --git a/drivers/qe/Kconfig b/drivers/qe/Kconfig index c44a81f69a..89a75c175b 100644 --- a/drivers/qe/Kconfig +++ b/drivers/qe/Kconfig @@ -27,6 +27,10 @@ choice depends on FMAN_ENET || QE default SYS_QE_FMAN_FW_IN_ROM
+config SYS_QE_FMAN_FW_IN_FS
depends on FS_LOADER && FMAN_ENETbool "Filesystem"Should this be a choice?
It is.
OK I see. But which filesystem, which filename, ...?
In any case it needs some decent help!
I think it's reasonable in context (the choice is "QUICC Engine FMan ethernet firmware location"). It's in the same (terse) style as the rest of the file.
Terse is one word for it. Is there actually any documentation? I see some stuff in README which is the wrong place.
Regards, Simon
--Sean
config SYS_QE_FMAN_FW_IN_NOR bool "NOR flash"
-- 2.35.1.1320.gc452695387.dirty
Regards, Simon
On Thu, Dec 29, 2022 at 11:53:01AM -0500, Sean Anderson wrote:
This adds a new method to load Fman firmware from a filesystem. This allows users to use regular files instead of hard-coded offsets for the firmware.
Signed-off-by: Sean Anderson sean.anderson@seco.com Reviewed-by: Ramon Fried rfried.dev@gmail.com
Applied to u-boot/master, thanks!
participants (3)
-
Sean Anderson -
Simon Glass -
Tom Rini