Re: [PATCH v4 3/4] arm64: dts: rockchip: add overlay tests for Rock 5B PCIe overlays

Hello Quentin,

On 2025-02-06 12:07, Quentin Schulz wrote:

On 2/4/25 2:35 PM, Dragan Simic wrote:

quoted

On 2025-02-04 13:20, Quentin Schulz wrote:

quoted

On 2/4/25 12:22 PM, Dragan Simic wrote:

quoted

quoted

On 2025-01-31 11:40, Quentin Schulz wrote:

Not discussing CONFIG_OF_ALL_DTBS relevancy wrt hiding overlay tests
behind, unrelated to this series I believe :)

[...]

Oh, indeed.  I'll get back to it below.

quoted

quoted

quoted

With the above-proposed changes in place, and with 
CONFIG_OF_ALL_DTBS
selected, the relevant part of the "make dtbs" output looks like 
this:

   DTC     arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dtb
   DTC     arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-ep.dtbo
   DTC     
arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-srns.dtbo
   OVL     arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-ep.dtb
   OVL     arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-srns.dtb

No more "phony targets" in the produced output. :)

Funnily enough, I would prefer to see OVL for overlays rather than
DTC, but I guess it's just one more occurrence of developers
disagreeing on how to name things :)

I actually agree with that, just like I prefer to see .dtbo files
as additions to dtb-$(CONFIG_ARCH_XYZ).  It's all about the overlays,
so they should be both specified and echoed back.

Moreover, we currently also have additional .dtb files with applied
overlays left after the build and installed afterwards, which doesn't
make much sense to me.  To me, those additional .dtb files should be
deleted as build artefacts and not installed.

I **think** it could be useful for systems without overlay support.
Then you have a dtb which is the result of an overlay applied on top
of the base dtb and you can replace your previous dtb with that one,
and voilà.

What I don't like is that it's difficult to differentiate them from
the "normal" base DTB or even from the DTBO (simple base DTB + overlay
test is usually named after the overlay, and in the case of the Rock
5B test: rk3588-rock-5b-pcie-srns.dtbo and
arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-srns.dtb), easy to
pick the wrong one. Though that is on **me** as I could pick another
name for the overlay test and e.g. prepend "test-ovl_" to the filename
for example.

On second thought, I'd agree that having the additional "extended"
.dtb files, i.e. the versions with the DT overlays already applied,
could be quite useful.  It's just that having them built and installed
as well possibly makes everything a bit more convoluted, maybe even
a bit confusing, but that's pretty much inevitable.  However, the
benefits should outweigh those slight downsides.

Regarding the naming, I don't think that prepending a self-descriptive
prefix would actually work as expected, because of the way "magic"
in scripts/Makefile.dtbs works.  Actually, I just tested that, and
it didn't seem to work as expected.

[...]

quoted

quoted

I won't be too difficult to convince here, just want some "authority"
or a piece of history about CONFIG_OF_ALL_DTBS that would go your
direction, before doing the change. I believe automated build tests
without needing to enable a symbol, and that taking DTB and DTBO from
the build output and apply DTBO on top of DTB works without having to
go through some length to get the symbols, are good reasons to keep 
it
the way it is in this patch series.

I'd like the most to perform the above-proposed "divorcing" of the DT
overlay tests from CONFIG_OF_ALL_DTBS, so we don't have to enable any
additional options to have the overlay tests run automatically, but
to keep .dtbo filenames in dtb-$(CONFIG_ARCH_XYZ).  I think that would
bring the best of both worlds, so to speak.

So, just to recap:

dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3568-wolfvision-pf5.dtb
dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3568-wolfvision-pf5-display-vz.dtbo
dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3568-wolfvision-pf5-io-expander.dtbo

stays and I add:

dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3568-wolfvision-pf5-vz-2-uhd.dtb
rk3568-wolfvision-pf5-vz-2-uhd-dtbs := rk3568-wolfvision-pf5.dtb \
	rk3568-wolfvision-pf5-display-vz.dtbo \
	rk3568-wolfvision-pf5-io-expander.dtbo

at the bottom of the Makefile. I specifically do NOT want to make this
depend on CONFIG_OF_ALL_DTBS (by using dtb- like in ti/), so that the
base DTB will always have the symbols in, regardless of
CONFIG_OF_ALL_DTBS.

I think the redundancy is unnecessary but I guess it's worth getting
away from implicit rules.

I fully agree with getting away from the tests depending on the
CONFIG_OF_ALL_DTBS configuration option, which I wasn't happy with
from the very beginning of this discussion.  It just felt and still
feels wrong, especially because CONFIG_OF_ALL_DTBS depends on other
configuration option(s) that pretty much don't go together with a
non-development kernel build.

The above-provided example is perfectly fine with me, and it follows
the way "magic" in scripts/Makefile.dtbs works.  I just tested it,
to make sure it works as expected, which it does.  As a note, I like
that the already present .dtbo lines remain unmoved, because that
keeps the DT overlay tests separate from the "meat" of the Makefile,
which should make it more readable and less error-prone.