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

[Tommaso Merciai]

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?

Thanks,
Tommaso

-- 
Tommaso Merciai
Embedded Linux Developer
tommaso.merciai at amarulasolutions.com
__________________________________

Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
info at amarulasolutions.com
www.amarulasolutions.com