Re: [RFC PATCH v3 4/5] beacon: imx8mn: override env_get_location in imx8mn_beacon.c

[Tommaso Merciai <tomm.merciai@gmail.com>]

On Thu, Jan 27, 2022 at 10:59:51AM -0600, Adam Ford wrote:
> On Wed, Jan 26, 2022 at 2:58 PM Tommaso Merciai <tomm.merciai@gmail.com> wrote:
> >
> > On Wed, Jan 26, 2022 at 12:05:22PM -0600, Adam Ford wrote:
> > > On Sat, Dec 25, 2021 at 2:26 PM Tommaso Merciai <tomm.merciai@gmail.com> wrote:
> > > >
> > > > Override env_get_location function at board level, previously dropped
> > > > down from arch/arm/mach-imx/imx8m/soc.c
> > > >
> > > > References:
> > > >  - commit 37d3e3bb95d7532e2503f115dd6c6762fd3b0262
> > > >
> > > > Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com>
> > >
> >
> > Hi Adam,
> >
> > > I am going to re-do the patch for the beacon board, because I talked
> > > it over with my colleagues, and we'd like to force the environment to
> > > one location or another and not base it on whether or not it was the
> > > boot device.
> > >
> > > I guess my questions is what's the status of the remaining patch
> > > series, and how do I integrate my desired changes into it?  I can send
> > > you what I want for my board if you're going to resubmit the series.
> >
> > It's ok for me. Send me your changes.
> > Thanks.
> >
> > > I wonder if a sub-function could be called where the existing
> > > env_get_location() is renamed to something else, but declared as weak
> > > can called from env_get_location.  Anyone who wants a their own
> > > function can write their own so we don't have two functions with the
> > > same name declared as weak.
> > >
> > > What are your thoughts on that?
> >
> > Right. In this way every boards that use imx8mp/imx8mn can override this
> > function according to their own needs.
> >
> 

Hi Adam,

> After trying some different options, I wonder if we can just delete
> env_get_location() completely from arch/arm/mach-imx/imx8m/soc.c.
> 
> For my board, it does exactly what I want when I delete that function
> since the default from env.c is sufficient.  I think it's probably
> safest to use your original approach to move them to individual board
> files with the idea that some people may not need it at all.  When
> placed into their board files, they can choose what they want for
> themselves.

Ack.

> 
> I would suggest sending a patch without the RFC and place the function
> into the various imx8mn and imx8mp boards, but know that you can skip
> the beacon board.  I would also suggest that you add a note that
> people should test this to see if it's really what they want or if
> they can live without the code and fall back on the default.

Ok, I'll send v4 of the series (without RFC label) in these days.

Thanks,
Tommaso

> 
> adam
> 
> > Tommaso
> >
> > >
> > > adam
> > > > ---
> > > >  board/beacon/imx8mn/imx8mn_beacon.c | 35 ++++++++++++++++++++++++++++-
> > > >  1 file changed, 34 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/board/beacon/imx8mn/imx8mn_beacon.c b/board/beacon/imx8mn/imx8mn_beacon.c
> > > > index 7fe252b262..05ab5613ee 100644
> > > > --- a/board/beacon/imx8mn/imx8mn_beacon.c
> > > > +++ b/board/beacon/imx8mn/imx8mn_beacon.c
> > > > @@ -6,14 +6,47 @@
> > > >  #include <common.h>
> > > >  #include <miiphy.h>
> > > >  #include <netdev.h>
> > > > -
> > > > +#include <env_internal.h>
> > > >  #include <asm/arch/clock.h>
> > > >  #include <asm/arch/sys_proto.h>
> > > > +#include <asm/mach-imx/boot_mode.h>
> > > >  #include <asm/global_data.h>
> > > >  #include <asm/io.h>
> > > >
> > > >  DECLARE_GLOBAL_DATA_PTR;
> > > >
> > > > +enum env_location env_get_location(enum env_operation op, int prio)
> > > > +{
> > > > +       enum boot_device dev = get_boot_device();
> > > > +       enum env_location env_loc = ENVL_UNKNOWN;
> > > > +
> > > > +       if (prio)
> > > > +               return env_loc;
> > > > +
> > > > +       if (IS_ENABLED(CONFIG_ENV_IS_IN_SPI_FLASH) && dev == QSPI_BOOT) {
> > > > +               env_loc = ENVL_SPI_FLASH;
> > > > +       } else if (IS_ENABLED(CONFIG_ENV_IS_IN_NAND) && dev == NAND_BOOT) {
> > > > +               env_loc = ENVL_NAND;
> > > > +       } else if (IS_ENABLED(CONFIG_ENV_IS_IN_MMC)) {
> > > > +               switch (dev) {
> > > > +               case SD1_BOOT:
> > > > +               case SD2_BOOT:
> > > > +               case SD3_BOOT:
> > > > +               case MMC1_BOOT:
> > > > +               case MMC2_BOOT:
> > > > +               case MMC3_BOOT:
> > > > +                       env_loc = ENVL_MMC;
> > > > +                       break;
> > > > +               default:
> > > > +                       break;
> > > > +               }
> > > > +       } else if (IS_ENABLED(CONFIG_ENV_IS_NOWHERE)) {
> > > > +               env_loc = ENVL_MMC;
> > > > +       }
> > > > +
> > > > +       return env_loc;
> > > > +}
> > > > +
> > > >  #if IS_ENABLED(CONFIG_FEC_MXC)
> > > >  static int setup_fec(void)
> > > >  {
> > > > --
> > > > 2.25.1
> > > >