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

[Heiko Stübner]

Am Donnerstag, 6. Februar 2025, 12:07:21 MEZ schrieb 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?

Yes, I do like the variant of _not_ limiting these builds to
CONFIG_OF_ALL_DTBS. From reading up on it, it's supposed to build all
dtbs - even from non-selected architectures?

So on a rockchip-only-build I'd still want to build the dtb+dtbo
combination nevertheless.

zynq, renesas, qcom and more are doing this like Quentin proposed, where
only ti is not.