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 <[email protected]> wrote: > > > > On Wed, Nov 22, 2023 at 07:44:09PM +0530, Sumit Garg wrote: > > > On Wed, 22 Nov 2023 at 19:31, Tom Rini <[email protected]> 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 > > > > > <[email protected]> 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 > > > > > $ make > > > > > > > > > > - gzip u-boot:: > > > > > > > > > > gzip u-boot-nodtb.bin > > > > > > > > > > - Append 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. -- Tom
signature.asc
Description: PGP signature
