On 2/1/22 13:26, Adam Ford wrote:
On Tue, Feb 1, 2022 at 5:58 AM Marek Vasut <ma...@denx.de> wrote:
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.merc...@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.merc...@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.
I would think we should ask the users of 8MP and 8MN to test theirs
without this patch to see if it impacts them. The original patch was
done by NXP, so I assume they want it for their EVK's. This code
explicitly makes it impossible to set the environment location to a
device that was not a boot device. From what I can tell, we're
talking about a small number of boards since it's just 8MN and 8MP.
What about a compromise:
Rename env_get_location() in the soc.c to imx_env_get_location().
From the 8MN and 8MP boards (for people who want it or need it), write
a board function called env_get_location() which calls
imx_env_get_location(). This way the code remains, and people can
choose whether they want this behavior, or the behavior or the default
behavior used by 8MM and 8MQ. It's less duplicated code, and it
doesn't break the default behavior for those who want that.
We are still talking about 40 lines of code being copied to 12 different
places, or, 12 boards which need to be fixed up -- instead of -- one
board which needs to be configured correctly to pick env from wherever
it should pick it from instead of the NXP default.
