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

[Adam Ford]

On Wed, Jan 26, 2022 at 2:58 PM Tommaso Merciai <tomm.merciai at 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 at 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 at 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.
>

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.

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.

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