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

[Marek Vasut]

On 2/1/22 12:56, Tommaso Merciai wrote:
> On Tue, Feb 01, 2022 at 05:22:06AM -0600, 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?
> 
> Hi Adam,
> You are right. Mmm in this way you have to duplicate the code of env.c
> into your board.c. This doesn't look very functional.
> I think remove it from soc.c is the right way.

How do you propose to fix all the boards which depend on the current 
arch-specific behavior, duplicate that code in multiple copies into 
board code ? That will be a lot of duplication too.