| Message ID | 20221229165301.2621563-3-sean.anderson@seco.com |
|---|---|
| State | Accepted |
| Commit | e4f0cc5ddf6ad24e06fcd6a25bbaaf3d55fa0899 |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | net: fm: Add support for loading firmware from filesystem | expand |
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!
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);