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

[Quentin Schulz <quentin.schulz@cherry.de>]

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