On Wed, Sep 29, 2021 at 12:02:16PM +0300, Ilias Apalodimas wrote:
Hi Zong,
[...]
diff --git a/board/sifive/unleashed/unleashed.c b/board/sifive/unleashed/unleashed.c index 8cd514df3005..7e89c3f740a7 100644 --- a/board/sifive/unleashed/unleashed.c +++ b/board/sifive/unleashed/unleashed.c @@ -116,12 +116,10 @@ int misc_init_r(void)
void *board_fdt_blob_setup(void) {
if (IS_ENABLED(CONFIG_OF_SEPARATE)) {if (gd->arch.firmware_fdt_addr)return (ulong *)gd->arch.firmware_fdt_addr;elsereturn (ulong *)&_end;}
if (gd->arch.firmware_fdt_addr)return (void *)gd->arch.firmware_fdt_addr;elsereturn (void *)&_end;}
I was wondering if we need to check CONFIG_OF_BOARD here? I'm not sure whether we should distinguish the value of a1 register which is meaningless. It means that if we don't expect the device tree to be passed by prior stage, then the a1 register might be a trash value at the beginning, so it would still return the arch.firmware_fdt_addr here, rather than _end.
I thought about it as well. Those boards were configured up to now with 'CONFIG_OF_SEPARATE'. Which means we are looking at an existing issue? IOW the device tree was passed as part of U-Boot, which would mean a1 would have had thrash as well. Maybe a1 always has a valid DT on those boards so we never noticed?
And do you think that we should enable the CONFIG_OF_BOARD for unmatched and unleashed? Because it seems to me that we actually pass the device tree by prior stage (i.e. OpenSBI).
Yes in that case what you request makes sense for unmatched/unleashed. Return gd->arch.firmware_fdt_addr in OF_BOARD is selected otherwise return _end (instead of the current check). If that sounds good to you I'll send a v2
Looking a bit more at it... Apparently those boards boot from SPL. So it's SPL->OpenSBI->U-Boot. By having the config as OF_SEPARATE the *U-Boot* DTB is used. SPL passes it to OpenSBI and OpenSBI passes it on a1 to U-Boot proper. That's why the register reading works for that config.
In that case the pre-existing code is 'wrong' as well, since the DTB is not at _end, but the bogus path is never taken... (check the __weak board_fdt_blob_setup for details).
So I think I'll send a v2, keeping the config as-is and fixing the return address of the DTB in case OF_BOARD is ever selected.
Cheers /Ilias
[...]
Regards /Ilias