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

Hi Dragan,

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

Hello Quentin,

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 :)

[...]

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.

[...]

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 can compromise on that :)

@Heiko does this work for you?

Cheers,
Quentin