[PATCH v3 0/3] net: fm: Add support for loading firmware from filesystem

[PATCH v3 0/3] net: fm: Add support for loading firmware from filesystem

[Sean Anderson]

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.com/

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

-- 
2.35.1.1320.gc452695387.dirty

[PATCH v3 1/3] misc: fs_loader: Add function to get the chosen loader

[Sean Anderson]

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
-- 
2.35.1.1320.gc452695387.dirty

[PATCH v3 2/3] net: fm: Add firmware name parameter

[Sean Anderson]

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, &reg->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);
-- 
2.35.1.1320.gc452695387.dirty

[PATCH v3 3/3] net: fm: Support loading firmware from a filesystem

[Sean Anderson]

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"
 
-- 
2.35.1.1320.gc452695387.dirty

Re: [PATCH v3 3/3] net: fm: Support loading firmware from a filesystem

[Simon Glass]

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_ENET
> +       bool "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

Re: [PATCH v3 2/3] net: fm: Add firmware name parameter

[Simon Glass]

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, &reg->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

Re: [PATCH v3 2/3] net: fm: Add firmware name parameter

[Sean Anderson]

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, &reg->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

Re: [PATCH v3 3/3] net: fm: Support loading firmware from a filesystem

[Sean Anderson]

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_ENET
>> +       bool "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

Re: [PATCH v3 3/3] net: fm: Support loading firmware from a filesystem

[Simon Glass]

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_ENET
> >> +       bool "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

Re: [PATCH v3 2/3] net: fm: Add firmware name parameter

[Simon Glass]

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, &reg->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

Re: [PATCH v3 2/3] net: fm: Add firmware name parameter

[Sean Anderson]

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, &reg->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

Re: [PATCH v3 2/3] net: fm: Add firmware name parameter

[Simon Glass]

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, &reg->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

Re: [PATCH v3 1/3] misc: fs_loader: Add function to get the chosen loader

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 775 bytes --]

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!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v3 2/3] net: fm: Add firmware name parameter

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 520 bytes --]

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!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v3 3/3] net: fm: Support loading firmware from a filesystem

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 387 bytes --]

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!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]