[PATCH v4 4/4] phytec: phycore_imx8mp: override env_get_location

[Adam Ford]

On Tue, Feb 1, 2022 at 5:46 AM Marek Vasut <marex at denx.de> wrote:
>
> On 2/1/22 12:22, Adam Ford wrote:
> > On Tue, Feb 1, 2022 at 3:09 AM Tommaso Merciai
> > <tommaso.merciai at amarulasolutions.com> wrote:
> >>
> >> On Tue, Feb 01, 2022 at 04:16:52AM +0100, Marek Vasut wrote:
> >>> On 1/31/22 23:15, Tommaso Merciai wrote:
> >>>> On Mon, Jan 31, 2022 at 06:03:58PM +0100, Marek Vasut wrote:
> >>>>> On 1/31/22 17:58, Tommaso Merciai wrote:
> >>>>>> Override env_get_location function at board level, previously dropped
> >>>>>> down from arch/arm/mach-imx/imx8m/soc.c
> >>>>>>
> >>>>>> References:
> >>>>>>     - commit 98af80d3c969e69a1b8ce98bb20e5ad844022da2
> >>>>>>
> >>>>>> Signed-off-by: Tommaso Merciai <tommaso.merciai at amarulasolutions.com>
> >>>>>> ---
> >>>>>>     board/phytec/phycore_imx8mp/phycore-imx8mp.c | 33 ++++++++++++++++++++
> >>>>>>     1 file changed, 33 insertions(+)
> >>>>>>
> >>>>>> diff --git a/board/phytec/phycore_imx8mp/phycore-imx8mp.c b/board/phytec/phycore_imx8mp/phycore-imx8mp.c
> >>>>>> index a8f0821437..05926eefa3 100644
> >>>>>> --- a/board/phytec/phycore_imx8mp/phycore-imx8mp.c
> >>>>>> +++ b/board/phytec/phycore_imx8mp/phycore-imx8mp.c
> >>>>>> @@ -11,9 +11,42 @@
> >>>>>>     #include <asm/mach-imx/boot_mode.h>
> >>>>>>     #include <env.h>
> >>>>>>     #include <miiphy.h>
> >>>>>> +#include <env_internal.h>
> >>>>>>     DECLARE_GLOBAL_DATA_PTR;
> >>>>>> +enum env_location env_get_location(enum env_operation op, int prio)
> >>>>>> +{
> >>>>>
> >>>>> Why don't you just turn this into default __weak function and override it on
> >>>>> board level when it is really needed to be overridden ?
> >>>>
> >>>> Hi Marek,
> >>>> env_get_location is already declared as __weak, check env/env.c. We
> >>>> can't override it 2 times.
> >>>
> >>> Oh, it is this problem with missing ability to define multiple levels of
> >>> symbol strength.
> >>>
> >>> __weak enum env_location arch_env_get_location(enum env_operation op, int
> >>> prio)
> >>> {
> >>>          if (prio >= ARRAY_SIZE(env_locations))
> >>>                  return ENVL_UNKNOWN;
> >>>
> >>>          return env_locations[prio];
> >>> }
> >>>
> >>> __weak enum env_location board_env_get_location(enum env_operation op, int
> >>> prio)
> >>> {
> >>>        return arch_env_get_location(op, prio);
> >>> }
> >>>
> >>> __weak enum env_location env_get_location(enum env_operation op, int prio)
> >>> {
> >>>        return board_env_get_location(op, prio);
> >>> }
> >>>
> >>> By default, the compiler will optimize it all out. If you have arch-specific
> >>> default (like imx does), implement arch_env_get_location(), if you have even
> >>> board-specific default (like your board likely does), implement
> >>> board_env_get_location(), if you need to override both, then override
> >>> env_get_location() (unlikely).
> >>>
> >>> This is also inline with all the other arch_*() and board_*() functions we
> >>> have, and you won't have much duplication either.
> >>
> >> Hi Marek,
> >> Thanks for the tips, then if I understand correctly, your idea is: use:
> >>
> >> arch_env_get_location in (soc.c)
> >>
> >> In this way imx8m users can override this function at board level using:
> >>
> >> board_env_get_location
> >>
> >> right?
> >
> > What about those of us who want to use the default option found in
> > env.c?  It seems like we're creating more abstraction to address the
> > abstraction we don't all want.
>
> On MX8M platforms which have their own arch-specific default, you
> override the env settings on your board level with the same stuff that
> is in env/env.c already. Most of the MX8M users will likely stick to the
> arch-specific MX8M default as they do so far.

This function is encapsulated in an ifdef which is only true for 8MN
and 8MP.  It's not defined for 8MM or 8MQ, so those boards already
default back to the __weak function in env.c

adam

>
> > From my interpretation, the whole
> > point of creating the default in env.c was to let people define the
> > location of their environment, and this function in soc.c undid that.
> > If people want it for their boards, put this function in their boards,
> > otherwise, just use the default, or write your own.
>
> [...]