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