On Wed, Dec 06, 2023 at 12:44:47PM +0200, Ilias Apalodimas wrote:
Hi Tom,
On Wed, 22 Nov 2023 at 16:28, Tom Rini trini@konsulko.com wrote:
On Wed, Nov 22, 2023 at 07:44:09PM +0530, Sumit Garg wrote:
On Wed, 22 Nov 2023 at 19:31, Tom Rini trini@konsulko.com wrote:
On Wed, Nov 22, 2023 at 11:51:29AM +0530, Sumit Garg wrote:
Hi Caleb,
On Tue, 21 Nov 2023 at 22:39, Caleb Connolly caleb.connolly@linaro.org wrote:
[snip]
== DT loading ==
Previously, boards used the FDT blob embedded into U-Boot (via OF_SEPARATE). However, most Qualcomm boards run U-Boot as a secondary bootloader, so we can instead rely on the first-stage bootloader to populate some useful FDT properties for us (notably the /memory node and KASLR seed) and fetch the DTB that it provides. Combined with the memory map changes above, this let's us entirely avoid configuring the memory map explicitly.
Since with this change, we don't need to embed FDT blob in the u-boot binary, so I was thinking if we really need to import DTs from Linux for different platforms and then play a catchup game?
IMO, the build command would look like following if we import pre-built FDT blob from Linux:
Build u-boot::
$ export CROSS_COMPILE=<aarch64 toolchain prefix> $ make qcom_defconfig $ makegzip u-boot::
gzip u-boot-nodtb.binAppend dtb to gzipped u-boot::
cat u-boot-nodtb.bin.gz<linux-tree>/arch/arm64/boot/dts/qcom/your-board.dtb > u-boot-nodtb.bin.gz-dtb
This would avoid the maintenance burden to keep DT in sync with that of Linux. And since DT bindings in Linux are backwards compatible, we can say u-boot should work with DTB picked up from any Linux kernel stable release.
I guess one question I have is, are we being passed the device tree (since we're acting like the Linux Kernel)
Yeah that is the case here, see patch #1 in this series regarding how FDT address is being retrieved from previous stage bootloader (ABL on sdm845 and qcs404 SoCs).
That's what I thought.
or knowing that we have the dtb attached to the end of us and making use of the old kernel appended dtb option? We're fine in for example the rpi_arm64 case of just being given a device tree from the previous stage and not needing one in-tree.
That's good to know and we can replicate that for Qcom platforms which are chainloaded and don't need an embedded DT.
So yes, moving these towards the direction of rpi_arm64 and specifically using imply OF_HAS_PRIOR_STAGE or select OF_HAS_PRIOR_STAGE (force that the dtb must be provided to us) sounds like the right direction to take these platforms.
IMHO that option isn't that useful. Grepping for OF_HAS_PRIOR_STAGE shows that we use it to print where the DT came from unless I am missing something...
So first, all of "imply OF_HAS_PRIOR_STAGE" is wrong and should be "select OF_HAS_PRIOR_STAGE". And then yes, it's OF_HAS_PRIOR_STAGE + OF_BOARD (which is default y if OF_HAS_PRIOR_STAGE). That is the set of symbols for lib/fdtdec.c::fdtdec_setup() to know to call board_fdt_blob_setup() and it's up to the board to know where to find the device tree we were given at run time. Generally the case here is that we're being a fake Linux Kernel and our dtb is in memory address $X and that in turn is set in the appropriate register.
On top of that having implies in the Kconfig to control those prints makes little sense to me and it's a half-baked solution anyway, because may boards don't fill this properly. This thing was cleaned up in 2e8d2f88439d, 2ea63271e522f, and d6f8ab30a2af46 but then got reintroduced in 275b4832f6bf91 without anyone acking or reviewing. I am fine with Caleb suggests here, but I think we can do way better.
I think the tags just got missed in 275b4832f6bf91 because that's close enough to what I wanted at the time.
Instead of having that imply we can get rid of it and only have OF_BOARD(and perhaps rename it). So a general cleanup I have in mind is
I _think_ the problem is that we wanted to be able to allow developers to still be able to override the device tree. So we imply (should be select) OF_HAS_PRIOR_STAGE so that OF_BOARD=y unless told otherwise.
CONFIG_OF_SEPARATE -> rename it to CONFIG_OF_APPEND because it is actually appended at the end of the binary.
I'm indifferent on this part. Reading lib/fdtdec.c::fdt_find_separate() I can see why it's "separate" and not "append", but one could reword things to read "append" and not "separate".
CONFIG_OF_EMBED -> Leave it as is, it's there for debugging mostly.
Agreed.
CONFIG_OF_BOARD perhaps rename it to something more useful, but the semantics are 'The DT comes from an *external* source. A bloblist, some EEPROM location, CPU registers etc' CONFIG_OF_HAS_PRIOR_STAGE -> Get rid of it again. gd already has gd->fdt_src, just add a FDTSRC_UNKNOWN as default and delegate the responsibility of filling this properly at fdtdec_setup(). The boards that use OF_BOARD can fill this in properly, instead of relying on Kconfig imply options and we can even be more explicit on where the DT came from.
I don't know. All of the things you mention for what CONFIG_OF_BOARD comes down to "the board defines where it comes from", and so OF_BOARD is what we came up with. And maybe that whole "imply" rather than "select" thing is so that a developer can more clearly change to OF_SEPARATE instead.