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

[Marek Vasut]

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.

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

[...]