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).
If I remember correctly, the SPL would calculate the size of u-boot proper, and then put the DTB at the end of u-boot proper, so the DTB would fortuitously be put at the _end location.
I haven't yet seen the creation part, but looking into the default board_fdt_blob_setup() the location seems to vary depending on CONFIG_SPL_BUILD. If that's selected (which is the case for those boards), then it depends on yet another SPL config for a separate .bsdd section.
I don't have a board to verify my suspicion but I think reading the DTB without looking into a1 is broken for these boards.
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.
Yes, it seems to me that we could use a config to separate the case between the prior stage and the _end.
Untangling OF_SEPARATE and OF_BOARD is part of a bigger revamp I wanted to do on the handover of a device tree from previous bootloaders, since we do have similar 'problems' in Arm and TF-A. But in principle OF_SEPARATE shouldn't have per board code to overwrite it. OF_BOARD should be used for that. OF_SEPARATE should merely mean "The dtb is concatenated to my U-Boot binary.
Right now RISC-V uses OF_SEPARATE reads the DTB on SPL and then goes back to using the a1 register for U-Boot proper. We could instead read the U-Boot concatenated DTB always in that case. OF_BOARD would then be used in case OpenSBI is compiled with a *different* DTB and you'd want to use that. Any idea if OpenSBI performs fixups before handing over the dtb in a1?
Unfortunately I don't have a board to test apart from QEMU. Let me respin this, with a potential fix I have in mind and we can discuss further.
Just note that, there is a patch on the fly, it modifies the same snippet of code, you might need to update your code based on top of it. https://lists.denx.de/pipermail/u-boot/2021-September/460378.html
I'll reply to that and see if the _end is indeed a problem.
Thanks /Ilias