Message-ID: <4044639.44csPzL39Z@diego>
Date: Fri, 07 Feb 2025 14:29:29 +0100
From: Heiko Stübner <heiko@...ech.de>
To: Dragan Simic <dsimic@...jaro.org>,
 Quentin Schulz <quentin.schulz@...rry.de>
Cc: Quentin Schulz <foss+kernel@...il.net>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
 Jagan Teki <jagan@...eble.ai>, Niklas Cassel <cassel@...nel.org>,
 Michael Riesch <michael.riesch@...fvision.net>,
 Jonas Karlman <jonas@...boo.se>, devicetree@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org,
 linux-kernel@...r.kernel.org,
 Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Subject:
 Re: [PATCH v4 3/4] arm64: dts: rockchip: add overlay tests for Rock 5B PCIe
 overlays

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.

Powered by blists - more mailing lists