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

[Quentin Schulz]

Hi Dragan,

On 2/4/25 2:35 PM, Dragan Simic wrote:
> Hello Quentin,
>
> On 2025-02-04 13:20, Quentin Schulz wrote:
> > On 2/4/25 12:22 PM, Dragan Simic wrote:
> > > > 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 :)

[...]

> > > 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.

[...]

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