[PATCH 00/21] Qualcomm generic board support

[Tom Rini]

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 at 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 at 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 at 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
> > > > >        $ 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20231210/4e2daa97/attachment.sig>