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

[Dragan Simic <dsimic@manjaro.org>]

Hello Quentin,

On 2025-02-06 12:07, Quentin Schulz wrote:
> On 2/4/25 2:35 PM, Dragan Simic wrote:
>> 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 :)
> 
> [...]

Oh, indeed.  I'll get back to it below.

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

On second thought, I'd agree that having the additional "extended"
.dtb files, i.e. the versions with the DT overlays already applied,
could be quite useful.  It's just that having them built and installed
as well possibly makes everything a bit more convoluted, maybe even
a bit confusing, but that's pretty much inevitable.  However, the
benefits should outweigh those slight downsides.

Regarding the naming, I don't think that prepending a self-descriptive
prefix would actually work as expected, because of the way "magic"
in scripts/Makefile.dtbs works.  Actually, I just tested that, and
it didn't seem to work as expected.

> [...]
> 
>>> 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 fully agree with getting away from the tests depending on the
CONFIG_OF_ALL_DTBS configuration option, which I wasn't happy with
from the very beginning of this discussion.  It just felt and still
feels wrong, especially because CONFIG_OF_ALL_DTBS depends on other
configuration option(s) that pretty much don't go together with a
non-development kernel build.

The above-provided example is perfectly fine with me, and it follows
the way "magic" in scripts/Makefile.dtbs works.  I just tested it,
to make sure it works as expected, which it does.  As a note, I like
that the already present .dtbo lines remain unmoved, because that
keeps the DT overlay tests separate from the "meat" of the Makefile,
which should make it more readable and less error-prone.

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip