[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 at 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 at 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 at seco.com>
> >> Reviewed-by: Ramon Fried <rfried.dev at 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