[PATCH v4 0/4] arm64: dts: rockchip: minimal support for Pre-ICT tester adapter for RK3588 Jaguar + add overlay tests

[PATCH v4 0/4] arm64: dts: rockchip: minimal support for Pre-ICT tester adapter for RK3588 Jaguar + add overlay tests

[Quentin Schulz]

This adds minimal support for the Pre-ICT tester adapter for RK3588
Jaguar.
GPIO3A3, GPIO3A4, GPIO3B2 and GPIO3D2 to GPIO3D5 are all routed to power
rails and can only be used as input and their bias are important to be
able to properly detect soldering issues.

Additionally, this adds build-time overlay application tests for all
Rockchip overlays to try to avoid future regressions.

Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
---
Changes in v4:
- fix typos in WolfVision patch,
- added Rb on WolfVision patch,
- Link to v3: https://lore.kernel.org/r/20250128-pre-ict-jaguar-v3-0-7be2f09d390a@cherry.de

Changes in v3:
- removed Fixes tag and intent to send to stable as this patch almost
  doubles the size of the main DTB. Let's not potentially break users of
  stable releases,
- added Wolfvision PF5 overlay tests, thanks Michael,
- added comment on how to add new overlays (via tests), and the side
  effects,
- grouped the overlay application test target with the definition of its
  dependencies (DTB + DTBO(s)),
- added trailers,
- Link to v2: https://lore.kernel.org/r/20250116-pre-ict-jaguar-v2-0-157d319004fc@cherry.de

Changes in v2:
- add overlay application tests for Edgeble NCM6A WiFi and Rock 5B PCIe
  Endpoint+SNRS
- add overlay application test for RK3588 Jaguar + Pre-ICT tester
  adapter,
- Link to v1: https://lore.kernel.org/r/20241206-pre-ict-jaguar-v1-1-7f660bd4b70c@cherry.de

---
Quentin Schulz (4):
      arm64: dts: rockchip: add overlay test for WolfVision PF5
      arm64: dts: rockchip: add overlay test for Edgeble NCM6A
      arm64: dts: rockchip: add overlay tests for Rock 5B PCIe overlays
      arm64: dts: rockchip: minimal support for Pre-ICT tester adapter for RK3588 Jaguar

 arch/arm64/boot/dts/rockchip/Makefile              |  36 ++++-
 .../dts/rockchip/rk3588-jaguar-pre-ict-tester.dtso | 171 +++++++++++++++++++++
 2 files changed, 202 insertions(+), 5 deletions(-)
---
base-commit: 69e858e0b8b2ea07759e995aa383e8780d9d140c
change-id: 20241206-pre-ict-jaguar-b90fafee8bd8

Best regards,
-- 
Quentin Schulz <quentin.schulz@cherry.de>

[PATCH v4 1/4] arm64: dts: rockchip: add overlay test for WolfVision PF5

[Quentin Schulz]

From: Quentin Schulz <quentin.schulz@cherry.de>

The WolfVision PF5 can have a PF5 Visualizer display and PF5 IO Expander
board connected to it. Therefore, let's generate an overlay test so the
application of the two overlays are validated against the base DTB.

Suggested-by: Michael Riesch <michael.riesch@wolfvision.net>
Reviewed-by: Michael Riesch <michael.riesch@wolfvision.net>
Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
---
 arch/arm64/boot/dts/rockchip/Makefile | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
index def1222c1907eb16b23cff6d540174a4e897abc9..bba9b2f1c761040545bea561878e9b63f8c29488 100644
--- a/arch/arm64/boot/dts/rockchip/Makefile
+++ b/arch/arm64/boot/dts/rockchip/Makefile
@@ -128,8 +128,6 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3568-roc-pc.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3568-rock-3a.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3568-rock-3b.dtb
 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
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3576-armsom-sige5.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3576-evb1-v10.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3582-radxa-e52c.dtb
@@ -170,3 +168,18 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-orangepi-5.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-orangepi-5b.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-rock-5a.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-rock-5c.dtb
+
+# Overlays
+## To build one or more overlays, overlay application tests must be added below.
+##
+## dtb-$(CONFIG_ARCH_ROCKCHIP) += <name of overlay application test>.dtb
+## <name of overlay application test>-dtbs := <base>.dtb <overlay-1>.dtbo [<overlay-2>.dtbo ...]
+##
+## This will generate each individual DTBO listed as a dependency of
+## <name of overlay application test>.dtb **AND** make <base>.dtb keep its
+## symbols (like when DTC_FLAGS has -@ passed).
+
+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

-- 
2.48.1

Re: [PATCH v4 1/4] arm64: dts: rockchip: add overlay test for WolfVision PF5

[Dragan Simic]

On 2025-01-31 11:40, Quentin Schulz wrote:
> From: Quentin Schulz <quentin.schulz@cherry.de>
> 
> The WolfVision PF5 can have a PF5 Visualizer display and PF5 IO 
> Expander
> board connected to it. Therefore, let's generate an overlay test so the
> application of the two overlays are validated against the base DTB.
> 
> Suggested-by: Michael Riesch <michael.riesch@wolfvision.net>
> Reviewed-by: Michael Riesch <michael.riesch@wolfvision.net>
> Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
> ---
>  arch/arm64/boot/dts/rockchip/Makefile | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/Makefile
> b/arch/arm64/boot/dts/rockchip/Makefile
> index
> def1222c1907eb16b23cff6d540174a4e897abc9..bba9b2f1c761040545bea561878e9b63f8c29488
> 100644
> --- a/arch/arm64/boot/dts/rockchip/Makefile
> +++ b/arch/arm64/boot/dts/rockchip/Makefile
> @@ -128,8 +128,6 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3568-roc-pc.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3568-rock-3a.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3568-rock-3b.dtb
>  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
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3576-armsom-sige5.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3576-evb1-v10.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3582-radxa-e52c.dtb
> @@ -170,3 +168,18 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += 
> rk3588s-orangepi-5.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-orangepi-5b.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-rock-5a.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-rock-5c.dtb
> +
> +# Overlays
> +## To build one or more overlays, overlay application tests must be
> added below.
> +##
> +## dtb-$(CONFIG_ARCH_ROCKCHIP) += <name of overlay application 
> test>.dtb
> +## <name of overlay application test>-dtbs := <base>.dtb
> <overlay-1>.dtbo [<overlay-2>.dtbo ...]
> +##
> +## This will generate each individual DTBO listed as a dependency of
> +## <name of overlay application test>.dtb **AND** make <base>.dtb keep 
> its
> +## symbols (like when DTC_FLAGS has -@ passed).
> +
> +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

Obviously, virtually the same comments [*] I made on the patch 3/4
from this series apply here as well.

[*] 
https://lore.kernel.org/linux-rockchip/77b5d0efa6e56bb391575aeeb4d1be80@manjaro.org/

[PATCH v4 2/4] arm64: dts: rockchip: add overlay test for Edgeble NCM6A

[Quentin Schulz]

From: Quentin Schulz <quentin.schulz@cherry.de>

The Edgeble NCM6A can have WiFi modules connected and this is handled
via an overlay (commit 951d6aaa37fe ("arm64: dts: rockchip: Add Edgeble
NCM6A WiFi6 Overlay")).

In order to make sure the overlay is still valid in the future, let's
add a validation test by applying the overlay on top of the main base
at build time.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
---
 arch/arm64/boot/dts/rockchip/Makefile | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
index bba9b2f1c761040545bea561878e9b63f8c29488..267966ea69b194887d59e38a4220239a90a91306 100644
--- a/arch/arm64/boot/dts/rockchip/Makefile
+++ b/arch/arm64/boot/dts/rockchip/Makefile
@@ -136,7 +136,6 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-armsom-w3.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-coolpi-cm5-evb.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-coolpi-cm5-genbook.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-edgeble-neu6a-io.dtb
-dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-edgeble-neu6a-wifi.dtbo
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-edgeble-neu6b-io.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-evb1-v10.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-firefly-itx-3588j.dtb
@@ -183,3 +182,7 @@ 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
+
+dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-edgeble-neu6a-wifi.dtb
+rk3588-edgeble-neu6a-wifi-dtbs := rk3588-edgeble-neu6a-io.dtb \
+	rk3588-edgeble-neu6a-wifi.dtbo

-- 
2.48.1

Re: [PATCH v4 2/4] arm64: dts: rockchip: add overlay test for Edgeble NCM6A

[Dragan Simic]

On 2025-01-31 11:40, Quentin Schulz wrote:
> From: Quentin Schulz <quentin.schulz@cherry.de>
> 
> The Edgeble NCM6A can have WiFi modules connected and this is handled
> via an overlay (commit 951d6aaa37fe ("arm64: dts: rockchip: Add Edgeble
> NCM6A WiFi6 Overlay")).
> 
> In order to make sure the overlay is still valid in the future, let's
> add a validation test by applying the overlay on top of the main base
> at build time.
> 
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
> ---
>  arch/arm64/boot/dts/rockchip/Makefile | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/Makefile
> b/arch/arm64/boot/dts/rockchip/Makefile
> index
> bba9b2f1c761040545bea561878e9b63f8c29488..267966ea69b194887d59e38a4220239a90a91306
> 100644
> --- a/arch/arm64/boot/dts/rockchip/Makefile
> +++ b/arch/arm64/boot/dts/rockchip/Makefile
> @@ -136,7 +136,6 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-armsom-w3.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-coolpi-cm5-evb.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-coolpi-cm5-genbook.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-edgeble-neu6a-io.dtb
> -dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-edgeble-neu6a-wifi.dtbo
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-edgeble-neu6b-io.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-evb1-v10.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-firefly-itx-3588j.dtb
> @@ -183,3 +182,7 @@ 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
> +
> +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-edgeble-neu6a-wifi.dtb
> +rk3588-edgeble-neu6a-wifi-dtbs := rk3588-edgeble-neu6a-io.dtb \
> +	rk3588-edgeble-neu6a-wifi.dtbo

Obviously, virtually the same comments [*] I made on the patch 3/4
from this series apply here as well.

[*] 
https://lore.kernel.org/linux-rockchip/77b5d0efa6e56bb391575aeeb4d1be80@manjaro.org/

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

[Quentin Schulz]

From: Quentin Schulz <quentin.schulz@cherry.de>

According to commit 40658534756f ("arm64: dts: rockchip: Add rock5b
overlays for PCIe endpoint mode"), Rock 5B can operate in PCIe endpoint
mode. For that to work, the rk3588-rock-5b-pcie-ep.dtbo overlay needs to
be applied on Rock 5B base Device Tree. If that Rock 5B is connected to
another Rock 5B, the latter needs to apply the
rk3588-rock-5b-pcie-srns.dtbo overlay.

In order to make sure the overlays are still valid in the future, let's
add a validation test by applying the overlays on top of the main base
at build time.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Niklas Cassel <cassel@kernel.org>
Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
---
 arch/arm64/boot/dts/rockchip/Makefile | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
index 267966ea69b194887d59e38a4220239a90a91306..ebcd16ce976ebe56a98d9685228980cd1f2f180a 100644
--- a/arch/arm64/boot/dts/rockchip/Makefile
+++ b/arch/arm64/boot/dts/rockchip/Makefile
@@ -150,8 +150,6 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-orangepi-5-plus.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-quartzpro64.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5-itx.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b.dtb
-dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b-pcie-ep.dtbo
-dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b-pcie-srns.dtbo
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-tiger-haikou.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-toybrick-x0.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-turing-rk1.dtb
@@ -186,3 +184,11 @@ rk3568-wolfvision-pf5-vz-2-uhd-dtbs := rk3568-wolfvision-pf5.dtb \
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-edgeble-neu6a-wifi.dtb
 rk3588-edgeble-neu6a-wifi-dtbs := rk3588-edgeble-neu6a-io.dtb \
 	rk3588-edgeble-neu6a-wifi.dtbo
+
+dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b-pcie-ep.dtb
+rk3588-rock-5b-pcie-ep-dtbs := rk3588-rock-5b.dtb \
+	rk3588-rock-5b-pcie-ep.dtbo
+
+dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b-pcie-srns.dtb
+rk3588-rock-5b-pcie-srns-dtbs := rk3588-rock-5b.dtb \
+	rk3588-rock-5b-pcie-srns.dtbo

-- 
2.48.1

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

[Dragan Simic]

Hello all,

I'm sorry for being late to the party.  Please, see some important
notes below;  to sum it up, we'll need to rework this a bit.

On 2025-01-31 11:40, Quentin Schulz wrote:
> From: Quentin Schulz <quentin.schulz@cherry.de>
> 
> According to commit 40658534756f ("arm64: dts: rockchip: Add rock5b
> overlays for PCIe endpoint mode"), Rock 5B can operate in PCIe endpoint
> mode. For that to work, the rk3588-rock-5b-pcie-ep.dtbo overlay needs 
> to
> be applied on Rock 5B base Device Tree. If that Rock 5B is connected to
> another Rock 5B, the latter needs to apply the
> rk3588-rock-5b-pcie-srns.dtbo overlay.
> 
> In order to make sure the overlays are still valid in the future, let's
> add a validation test by applying the overlays on top of the main base
> at build time.
> 
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Reviewed-by: Niklas Cassel <cassel@kernel.org>
> Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
> ---
>  arch/arm64/boot/dts/rockchip/Makefile | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/Makefile
> b/arch/arm64/boot/dts/rockchip/Makefile
> index
> 267966ea69b194887d59e38a4220239a90a91306..ebcd16ce976ebe56a98d9685228980cd1f2f180a
> 100644
> --- a/arch/arm64/boot/dts/rockchip/Makefile
> +++ b/arch/arm64/boot/dts/rockchip/Makefile
> @@ -150,8 +150,6 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += 
> rk3588-orangepi-5-plus.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-quartzpro64.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5-itx.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b.dtb
> -dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b-pcie-ep.dtbo
> -dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b-pcie-srns.dtbo
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-tiger-haikou.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-toybrick-x0.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-turing-rk1.dtb
> @@ -186,3 +184,11 @@ rk3568-wolfvision-pf5-vz-2-uhd-dtbs :=
> rk3568-wolfvision-pf5.dtb \
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-edgeble-neu6a-wifi.dtb
>  rk3588-edgeble-neu6a-wifi-dtbs := rk3588-edgeble-neu6a-io.dtb \
>  	rk3588-edgeble-neu6a-wifi.dtbo
> +
> +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b-pcie-ep.dtb
> +rk3588-rock-5b-pcie-ep-dtbs := rk3588-rock-5b.dtb \
> +	rk3588-rock-5b-pcie-ep.dtbo
> +
> +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b-pcie-srns.dtb
> +rk3588-rock-5b-pcie-srns-dtbs := rk3588-rock-5b.dtb \
> +	rk3588-rock-5b-pcie-srns.dtbo

After a quite lengthy excursion into the scripts/Makefile.dtbs and
scripts/Makefile.lib files, I'm afraid that the approach taken in
this patch isn't right.  Please allow me to explain.

Let's have a look at arch/arm64/boot/dts/ti/Makefile first, as an
example that provides already existing DT overlay checks.  Here's
an excerpt from that Makefile, which also provides an important cue
by mentioning CONFIG_OF_ALL_DTBS:

   12 dtb-$(CONFIG_ARCH_K3) += k3-am625-beagleplay.dtb
   13 dtb-$(CONFIG_ARCH_K3) += k3-am625-beagleplay-csi2-ov5640.dtbo
   14 dtb-$(CONFIG_ARCH_K3) += k3-am625-beagleplay-csi2-tevi-ov5640.dtbo

  135 # Build time test only, enabled by CONFIG_OF_ALL_DTBS
  136 k3-am625-beagleplay-csi2-ov5640-dtbs := k3-am625-beagleplay.dtb \
  137         k3-am625-beagleplay-csi2-ov5640.dtbo
  138 k3-am625-beagleplay-csi2-tevi-ov5640-dtbs := 
k3-am625-beagleplay.dtb \
  139         k3-am625-beagleplay-csi2-tevi-ov5640.dtbo

  213 dtb- += k3-am625-beagleplay-csi2-ov5640.dtb \
  214         k3-am625-beagleplay-csi2-tevi-ov5640.dtb \

As visible above, the DT overlays get added to dtb-$(CONFIG_ARCH_K3)
as .dtbo files (not as .dtb files), while the build-time DT overlay
tests specify the dependency chains for the overlays.

Even more importantly, the build-time overlay tests aren't supposed
to be executed until CONFIG_OF_ALL_DTBS is selected, which actually
gets handled at the very start of scripts/Makefile.dtbs:

    3 # If CONFIG_OF_ALL_DTBS is enabled, all DT blobs are built
    4 dtb-$(CONFIG_OF_ALL_DTBS) += $(dtb-)

The way this patch modifies arch/arm64/boot/dts/rockchip/Makefile
actually isn't correct, despite apparently producing the desired
outcome, because the way it adds rk3588-rock-5b-pcie-ep.dtb and
rk3588-rock-5b-pcie-srns.dtb (instead of adding proper .dtbo names)
to dtb-$(CONFIG_ARCH_ROCKCHIP) effectively creates some kind of
"phony targets" that indeed "get the job done", but unfortunately
in a wrong way.  The "phony targets" are handled by the following
"magic" in scripts/Makefile.dtbs:

    6 # Composite DTB (i.e. DTB constructed by overlay)
    7 multi-dtb-y := $(call multi-search, $(dtb-y), .dtb, -dtbs)
    8 # Primitive DTB compiled from *.dts
    9 real-dtb-y := $(call real-search, $(dtb-y), .dtb, -dtbs)
   10 # Base DTB that overlay is applied onto
   11 base-dtb-y := $(filter %.dtb, $(call real-search, $(multi-dtb-y), 
.dtb, -dtbs))

   18 targets         += $(real-dtb-y)

Let's have a look at the relevant part of the output produced when
"make dtbs" is executed with this patch applied:

   DTC     arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-ep.dtbo
   OVL     arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-ep.dtb
   DTC     arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-srns.dtbo
   OVL     arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-srns.dtb

Note the "OVL .*dtb" lines above, in which the ".*dtb" parts are
the above-mentioned "phony targets".  The above-quoted "magic" in
scripts/Makefile.dtbs is what "unfolds" those "phony targets" to
make them apparently produce the desired outcome, by populating
the $(real-dtb-y) variable with "rk3588-rock-5b-pcie-ep.dtbo
rk3588-rock-5b.dtb rk3588-rock-5b-pcie-srns.dtbo", as the proper
.dtbo names that get passed to the dtc utility.

Even more "magic" in scripts/Makefile.dtbs "unfolds" the "phony
targets" to have the build-time DT overlay tests performed in the
apparently proper way, while they actually shouldn't be performed
unless CONFIG_OF_ALL_DTBS is also selected.

With all this is mind, this patch should be reworked to follow
the right approach for defining build-time DT overlay tests, which
is illustrated in arch/arm64/boot/dts/ti/Makefile.  In particular,
we should just add the following DT overlay test definitions to
arch/arm64/boot/dts/rockchip/Makefile:

174 # Build-time tests only, enabled by CONFIG_OF_ALL_DTBS
175 rk3588-rock-5b-pcie-ep-dtbs := rk3588-rock-5b.dtb \
176         rk3588-rock-5b-pcie-ep.dtbo
177 rk3588-rock-5b-pcie-srns-dtbs := rk3588-rock-5b.dtb \
178         rk3588-rock-5b-pcie-srns.dtbo
179 dtb- += rk3588-rock-5b-pcie-ep.dtb \
180         rk3588-rock-5b-pcie-srns.dtb

You'll notice that the $(dtb-) variable pretty much again contains
the same "phony targets", but that's the way they should actually
be used.  I'm not very happy with the way they're specified, but
we should follow the approach from arch/arm64/boot/dts/ti/Makefile
anyway, and possibly improve the whole thing later.

I'd actually prefer to just have these test definitions added to the
end of arch/arm64/boot/dts/ti/Makefile, without moving the .dtbo lines
around.  That would keep the tests separate, which should be more
readable when we get more of them defined.

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

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

[Quentin Schulz]

Hi Dragan,

On 2/4/25 12:22 PM, Dragan Simic wrote:
> Hello all,
> 
> I'm sorry for being late to the party.  Please, see some important

No worries, you don't owe me feedback in a timely manner, or at all :)

> notes below;  to sum it up, we'll need to rework this a bit.
> > On 2025-01-31 11:40, Quentin Schulz wrote:
>> From: Quentin Schulz <quentin.schulz@cherry.de>
>>
>> According to commit 40658534756f ("arm64: dts: rockchip: Add rock5b
>> overlays for PCIe endpoint mode"), Rock 5B can operate in PCIe endpoint
>> mode. For that to work, the rk3588-rock-5b-pcie-ep.dtbo overlay needs to
>> be applied on Rock 5B base Device Tree. If that Rock 5B is connected to
>> another Rock 5B, the latter needs to apply the
>> rk3588-rock-5b-pcie-srns.dtbo overlay.
>>
>> In order to make sure the overlays are still valid in the future, let's
>> add a validation test by applying the overlays on top of the main base
>> at build time.
>>
>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Reviewed-by: Niklas Cassel <cassel@kernel.org>
>> Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
>> ---
>>  arch/arm64/boot/dts/rockchip/Makefile | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/rockchip/Makefile
>> b/arch/arm64/boot/dts/rockchip/Makefile
>> index
>> 267966ea69b194887d59e38a4220239a90a91306..ebcd16ce976ebe56a98d9685228980cd1f2f180a
>> 100644
>> --- a/arch/arm64/boot/dts/rockchip/Makefile
>> +++ b/arch/arm64/boot/dts/rockchip/Makefile
>> @@ -150,8 +150,6 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-orangepi-5- 
>> plus.dtb
>>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-quartzpro64.dtb
>>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5-itx.dtb
>>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b.dtb
>> -dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b-pcie-ep.dtbo
>> -dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b-pcie-srns.dtbo
>>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-tiger-haikou.dtb
>>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-toybrick-x0.dtb
>>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-turing-rk1.dtb
>> @@ -186,3 +184,11 @@ rk3568-wolfvision-pf5-vz-2-uhd-dtbs :=
>> rk3568-wolfvision-pf5.dtb \
>>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-edgeble-neu6a-wifi.dtb
>>  rk3588-edgeble-neu6a-wifi-dtbs := rk3588-edgeble-neu6a-io.dtb \
>>      rk3588-edgeble-neu6a-wifi.dtbo
>> +
>> +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b-pcie-ep.dtb
>> +rk3588-rock-5b-pcie-ep-dtbs := rk3588-rock-5b.dtb \
>> +    rk3588-rock-5b-pcie-ep.dtbo
>> +
>> +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b-pcie-srns.dtb
>> +rk3588-rock-5b-pcie-srns-dtbs := rk3588-rock-5b.dtb \
>> +    rk3588-rock-5b-pcie-srns.dtbo
> 
> After a quite lengthy excursion into the scripts/Makefile.dtbs and
> scripts/Makefile.lib files, I'm afraid that the approach taken in
> this patch isn't right.  Please allow me to explain.
> 
> Let's have a look at arch/arm64/boot/dts/ti/Makefile first, as an
> example that provides already existing DT overlay checks.  Here's
> an excerpt from that Makefile, which also provides an important cue
> by mentioning CONFIG_OF_ALL_DTBS:
> 
>    12 dtb-$(CONFIG_ARCH_K3) += k3-am625-beagleplay.dtb
>    13 dtb-$(CONFIG_ARCH_K3) += k3-am625-beagleplay-csi2-ov5640.dtbo
>    14 dtb-$(CONFIG_ARCH_K3) += k3-am625-beagleplay-csi2-tevi-ov5640.dtbo
> 
>   135 # Build time test only, enabled by CONFIG_OF_ALL_DTBS
>   136 k3-am625-beagleplay-csi2-ov5640-dtbs := k3-am625-beagleplay.dtb \
>   137         k3-am625-beagleplay-csi2-ov5640.dtbo
>   138 k3-am625-beagleplay-csi2-tevi-ov5640-dtbs := k3-am625- 
> beagleplay.dtb \
>   139         k3-am625-beagleplay-csi2-tevi-ov5640.dtbo
> 
>   213 dtb- += k3-am625-beagleplay-csi2-ov5640.dtb \
>   214         k3-am625-beagleplay-csi2-tevi-ov5640.dtb \
> 
> As visible above, the DT overlays get added to dtb-$(CONFIG_ARCH_K3)
> as .dtbo files (not as .dtb files), while the build-time DT overlay
> tests specify the dependency chains for the overlays.
> 
> Even more importantly, the build-time overlay tests aren't supposed
> to be executed until CONFIG_OF_ALL_DTBS is selected, which actually
> gets handled at the very start of scripts/Makefile.dtbs:
> 
>     3 # If CONFIG_OF_ALL_DTBS is enabled, all DT blobs are built
>     4 dtb-$(CONFIG_OF_ALL_DTBS) += $(dtb-)
> 

Do you have something to back your argument that build time tests should 
only be done when CONFIG_OF_ALL_DTBS is enabled? For now this seems like 
it's your interpretation of the use for the symbol. Though I agree that 
if you had any test you absolutely didn't want to run in normal times, 
hiding them behind CONFIG_OF_ALL_DTBS would be the thing to do.

> The way this patch modifies arch/arm64/boot/dts/rockchip/Makefile
> actually isn't correct, despite apparently producing the desired
> outcome, because the way it adds rk3588-rock-5b-pcie-ep.dtb and
> rk3588-rock-5b-pcie-srns.dtb (instead of adding proper .dtbo names)
> to dtb-$(CONFIG_ARCH_ROCKCHIP) effectively creates some kind of
> "phony targets" that indeed "get the job done", but unfortunately
> in a wrong way.  The "phony targets" are handled by the following
> "magic" in scripts/Makefile.dtbs:
> 
>     6 # Composite DTB (i.e. DTB constructed by overlay)
>     7 multi-dtb-y := $(call multi-search, $(dtb-y), .dtb, -dtbs)
>     8 # Primitive DTB compiled from *.dts
>     9 real-dtb-y := $(call real-search, $(dtb-y), .dtb, -dtbs)
>    10 # Base DTB that overlay is applied onto
>    11 base-dtb-y := $(filter %.dtb, $(call real-search, $(multi-dtb- 
> y), .dtb, -dtbs))
> 
>    18 targets         += $(real-dtb-y)
> 
> Let's have a look at the relevant part of the output produced when
> "make dtbs" is executed with this patch applied:
> 
>    DTC     arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-ep.dtbo
>    OVL     arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-ep.dtb
>    DTC     arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-srns.dtbo
>    OVL     arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-srns.dtb
> 
> Note the "OVL .*dtb" lines above, in which the ".*dtb" parts are
> the above-mentioned "phony targets".  The above-quoted "magic" in
> scripts/Makefile.dtbs is what "unfolds" those "phony targets" to 
> make them apparently produce the desired outcome, by populating
> the $(real-dtb-y) variable with "rk3588-rock-5b-pcie-ep.dtbo
> rk3588-rock-5b.dtb rk3588-rock-5b-pcie-srns.dtbo", as the proper
> .dtbo names that get passed to the dtc utility.
> 
> Even more "magic" in scripts/Makefile.dtbs "unfolds" the "phony
> targets" to have the build-time DT overlay tests performed in the
> apparently proper way, while they actually shouldn't be performed
> unless CONFIG_OF_ALL_DTBS is also selected.
> 

I understand the symbol CONFIG_OF_ALL_DTBS as "should build all DTBs for 
all architectures and do tests" and not "tests must only be run when 
CONFIG_OF_ALL_DTBS is selected". I find it so useful to test the 
application of overlays to the base DTB that I don't think it's 
necessarily a good idea to hide those behind CONFIG_OF_ALL_DTBS.

> With all this is mind, this patch should be reworked to follow
> the right approach for defining build-time DT overlay tests, which
> is illustrated in arch/arm64/boot/dts/ti/Makefile.  In particular,
> we should just add the following DT overlay test definitions to
> arch/arm64/boot/dts/rockchip/Makefile:
> 
> 174 # Build-time tests only, enabled by CONFIG_OF_ALL_DTBS
> 175 rk3588-rock-5b-pcie-ep-dtbs := rk3588-rock-5b.dtb \
> 176         rk3588-rock-5b-pcie-ep.dtbo
> 177 rk3588-rock-5b-pcie-srns-dtbs := rk3588-rock-5b.dtb \
> 178         rk3588-rock-5b-pcie-srns.dtbo
> 179 dtb- += rk3588-rock-5b-pcie-ep.dtb \
> 180         rk3588-rock-5b-pcie-srns.dtb
> 
> You'll notice that the $(dtb-) variable pretty much again contains
> the same "phony targets", but that's the way they should actually
> be used.  I'm not very happy with the way they're specified, but
> we should follow the approach from arch/arm64/boot/dts/ti/Makefile
> anyway, and possibly improve the whole thing later.
> 

What I don't like about this is that it allows to build the DTBO without 
providing a build time application test which means maintainer(s) and 
reviewer(s) need to pay even more attention to the patch than simply 
looking at it matching the patterns of how other DTBOs are built. Also, 
you now need to enable CONFIG_OF_ALL_DTBS to run the tests, which isn't 
enabled in the default defconfig for arm64. I would assume we have some 
bots building patches/master with those options enabled but it might be 
a bit too late already.

> I'd actually prefer to just have these test definitions added to the
> end of arch/arm64/boot/dts/ti/Makefile, without moving the .dtbo lines
> around.  That would keep the tests separate, which should be more
> readable when we get more of them defined.
> 

And more likely to forget about adding either the tests or the DTBO 
explicit rule.

> 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 kind of disagree with the feedback here as this only takes ti/Makefile 
as example while **all** other aarch64 Makefile have different logic, 
the one I'm using here for amlogic (meson), NXP (freescale), qcom 
(Qualcomm), Renesas, Xilinx and even Aarch32 NXP, and the one we 
currently use (no build tests) for Mediatek.

If we agree to what you suggest here we cancel the side-effect of having 
the symbols in the base DTB that this patch series introduces. This can 
go both ways, either good (DT symbols in = nothing to do for the user, 
get the base DTB and your DTBO, put in /boot and tada) or bad (DT 
symbols in = big size increase for base DTB). Moreover, enabling 
CONFIG_OF_ALL_DTBS would now generate a different DTB than when it's not 
(keeping the symbols in). If we wanted to keep the symbols in, we would 
need to modify DTC_FLAGS as well.
This could help make the decision(s) as well.

So I would say without much more context on the actual expected use for 
CONFIG_OF_ALL_DTBS that it's up to one's taste, and considering the 
precedent here, likely up to each architecture maintainers' taste.

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.

Additionally, depending on the feedback, I assume we want to migrate all 
current architectures building DTBO to be consistent and use the same 
logic (perhaps not for Mediatek because doing so would keep the DT 
symbols in the DTB, which drastically increases the size of the DT).

Does anyone from DT maintainership have feedback to provide on what is 
expected here generally wrt building and testing DTBOs?

Does Heiko have an opinion on what he would prefer to see happening for 
Rockchip?

Cheers,
Quentin

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

[Dragan Simic]

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:
>>> From: Quentin Schulz <quentin.schulz@cherry.de>
>>> 
>>> According to commit 40658534756f ("arm64: dts: rockchip: Add rock5b
>>> overlays for PCIe endpoint mode"), Rock 5B can operate in PCIe 
>>> endpoint
>>> mode. For that to work, the rk3588-rock-5b-pcie-ep.dtbo overlay needs 
>>> to
>>> be applied on Rock 5B base Device Tree. If that Rock 5B is connected 
>>> to
>>> another Rock 5B, the latter needs to apply the
>>> rk3588-rock-5b-pcie-srns.dtbo overlay.
>>> 
>>> In order to make sure the overlays are still valid in the future, 
>>> let's
>>> add a validation test by applying the overlays on top of the main 
>>> base
>>> at build time.
>>> 
>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>> Reviewed-by: Niklas Cassel <cassel@kernel.org>
>>> Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
>>> ---
>>>  arch/arm64/boot/dts/rockchip/Makefile | 10 ++++++++--
>>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/arch/arm64/boot/dts/rockchip/Makefile
>>> b/arch/arm64/boot/dts/rockchip/Makefile
>>> index
>>> 267966ea69b194887d59e38a4220239a90a91306..ebcd16ce976ebe56a98d9685228980cd1f2f180a
>>> 100644
>>> --- a/arch/arm64/boot/dts/rockchip/Makefile
>>> +++ b/arch/arm64/boot/dts/rockchip/Makefile
>>> @@ -150,8 +150,6 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-orangepi-5- 
>>> plus.dtb
>>>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-quartzpro64.dtb
>>>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5-itx.dtb
>>>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b.dtb
>>> -dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b-pcie-ep.dtbo
>>> -dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b-pcie-srns.dtbo
>>>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-tiger-haikou.dtb
>>>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-toybrick-x0.dtb
>>>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-turing-rk1.dtb
>>> @@ -186,3 +184,11 @@ rk3568-wolfvision-pf5-vz-2-uhd-dtbs :=
>>> rk3568-wolfvision-pf5.dtb \
>>>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-edgeble-neu6a-wifi.dtb
>>>  rk3588-edgeble-neu6a-wifi-dtbs := rk3588-edgeble-neu6a-io.dtb \
>>>      rk3588-edgeble-neu6a-wifi.dtbo
>>> +
>>> +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b-pcie-ep.dtb
>>> +rk3588-rock-5b-pcie-ep-dtbs := rk3588-rock-5b.dtb \
>>> +    rk3588-rock-5b-pcie-ep.dtbo
>>> +
>>> +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b-pcie-srns.dtb
>>> +rk3588-rock-5b-pcie-srns-dtbs := rk3588-rock-5b.dtb \
>>> +    rk3588-rock-5b-pcie-srns.dtbo
>> 
>> After a quite lengthy excursion into the scripts/Makefile.dtbs and
>> scripts/Makefile.lib files, I'm afraid that the approach taken in
>> this patch isn't right.  Please allow me to explain.
>> 
>> Let's have a look at arch/arm64/boot/dts/ti/Makefile first, as an
>> example that provides already existing DT overlay checks.  Here's
>> an excerpt from that Makefile, which also provides an important cue
>> by mentioning CONFIG_OF_ALL_DTBS:
>> 
>>    12 dtb-$(CONFIG_ARCH_K3) += k3-am625-beagleplay.dtb
>>    13 dtb-$(CONFIG_ARCH_K3) += k3-am625-beagleplay-csi2-ov5640.dtbo
>>    14 dtb-$(CONFIG_ARCH_K3) += 
>> k3-am625-beagleplay-csi2-tevi-ov5640.dtbo
>> 
>>   135 # Build time test only, enabled by CONFIG_OF_ALL_DTBS
>>   136 k3-am625-beagleplay-csi2-ov5640-dtbs := k3-am625-beagleplay.dtb 
>> \
>>   137         k3-am625-beagleplay-csi2-ov5640.dtbo
>>   138 k3-am625-beagleplay-csi2-tevi-ov5640-dtbs := k3-am625- 
>> beagleplay.dtb \
>>   139         k3-am625-beagleplay-csi2-tevi-ov5640.dtbo
>> 
>>   213 dtb- += k3-am625-beagleplay-csi2-ov5640.dtb \
>>   214         k3-am625-beagleplay-csi2-tevi-ov5640.dtb \
>> 
>> As visible above, the DT overlays get added to dtb-$(CONFIG_ARCH_K3)
>> as .dtbo files (not as .dtb files), while the build-time DT overlay
>> tests specify the dependency chains for the overlays.
>> 
>> Even more importantly, the build-time overlay tests aren't supposed
>> to be executed until CONFIG_OF_ALL_DTBS is selected, which actually
>> gets handled at the very start of scripts/Makefile.dtbs:
>> 
>>     3 # If CONFIG_OF_ALL_DTBS is enabled, all DT blobs are built
>>     4 dtb-$(CONFIG_OF_ALL_DTBS) += $(dtb-)
> 
> Do you have something to back your argument that build time tests
> should only be done when CONFIG_OF_ALL_DTBS is enabled? For now this
> seems like it's your interpretation of the use for the symbol. Though
> I agree that if you had any test you absolutely didn't want to run in
> normal times, hiding them behind CONFIG_OF_ALL_DTBS would be the thing
> to do.

Well, it isn't my interpretation of CONFIG_OF_ALL_DTBS, it's actually
how arch/arm64/boot/dts/ti/Makefile uses it already for the DT overlay
tests, which I already explained above.

Also, I already wrote that I'm not very happy with the way the config
option CONFIG_OF_ALL_DTBS is tied to the DT overlay tests.  I'll get
back to that below.

>> The way this patch modifies arch/arm64/boot/dts/rockchip/Makefile
>> actually isn't correct, despite apparently producing the desired
>> outcome, because the way it adds rk3588-rock-5b-pcie-ep.dtb and
>> rk3588-rock-5b-pcie-srns.dtb (instead of adding proper .dtbo names)
>> to dtb-$(CONFIG_ARCH_ROCKCHIP) effectively creates some kind of
>> "phony targets" that indeed "get the job done", but unfortunately
>> in a wrong way.  The "phony targets" are handled by the following
>> "magic" in scripts/Makefile.dtbs:
>> 
>>     6 # Composite DTB (i.e. DTB constructed by overlay)
>>     7 multi-dtb-y := $(call multi-search, $(dtb-y), .dtb, -dtbs)
>>     8 # Primitive DTB compiled from *.dts
>>     9 real-dtb-y := $(call real-search, $(dtb-y), .dtb, -dtbs)
>>    10 # Base DTB that overlay is applied onto
>>    11 base-dtb-y := $(filter %.dtb, $(call real-search, $(multi-dtb- 
>> y), .dtb, -dtbs))
>> 
>>    18 targets         += $(real-dtb-y)
>> 
>> Let's have a look at the relevant part of the output produced when
>> "make dtbs" is executed with this patch applied:
>> 
>>    DTC     arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-ep.dtbo
>>    OVL     arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-ep.dtb
>>    DTC     arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-srns.dtbo
>>    OVL     arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-srns.dtb
>> 
>> Note the "OVL .*dtb" lines above, in which the ".*dtb" parts are
>> the above-mentioned "phony targets".  The above-quoted "magic" in
>> scripts/Makefile.dtbs is what "unfolds" those "phony targets" to make 
>> them apparently produce the desired outcome, by populating
>> the $(real-dtb-y) variable with "rk3588-rock-5b-pcie-ep.dtbo
>> rk3588-rock-5b.dtb rk3588-rock-5b-pcie-srns.dtbo", as the proper
>> .dtbo names that get passed to the dtc utility.
>> 
>> Even more "magic" in scripts/Makefile.dtbs "unfolds" the "phony
>> targets" to have the build-time DT overlay tests performed in the
>> apparently proper way, while they actually shouldn't be performed
>> unless CONFIG_OF_ALL_DTBS is also selected.
> 
> I understand the symbol CONFIG_OF_ALL_DTBS as "should build all DTBs
> for all architectures and do tests" and not "tests must only be run
> when CONFIG_OF_ALL_DTBS is selected". I find it so useful to test the
> application of overlays to the base DTB that I don't think it's
> necessarily a good idea to hide those behind CONFIG_OF_ALL_DTBS.

As I wrote above, I'm not very happy with it either.  I'll explain
that further below.

>> With all this is mind, this patch should be reworked to follow
>> the right approach for defining build-time DT overlay tests, which
>> is illustrated in arch/arm64/boot/dts/ti/Makefile.  In particular,
>> we should just add the following DT overlay test definitions to
>> arch/arm64/boot/dts/rockchip/Makefile:
>> 
>> 174 # Build-time tests only, enabled by CONFIG_OF_ALL_DTBS
>> 175 rk3588-rock-5b-pcie-ep-dtbs := rk3588-rock-5b.dtb \
>> 176         rk3588-rock-5b-pcie-ep.dtbo
>> 177 rk3588-rock-5b-pcie-srns-dtbs := rk3588-rock-5b.dtb \
>> 178         rk3588-rock-5b-pcie-srns.dtbo
>> 179 dtb- += rk3588-rock-5b-pcie-ep.dtb \
>> 180         rk3588-rock-5b-pcie-srns.dtb
>> 
>> You'll notice that the $(dtb-) variable pretty much again contains
>> the same "phony targets", but that's the way they should actually
>> be used.  I'm not very happy with the way they're specified, but
>> we should follow the approach from arch/arm64/boot/dts/ti/Makefile
>> anyway, and possibly improve the whole thing later.
> 
> What I don't like about this is that it allows to build the DTBO
> without providing a build time application test which means
> maintainer(s) and reviewer(s) need to pay even more attention to the
> patch than simply looking at it matching the patterns of how other
> DTBOs are built. Also, you now need to enable CONFIG_OF_ALL_DTBS to
> run the tests, which isn't enabled in the default defconfig for arm64.
> I would assume we have some bots building patches/master with those
> options enabled but it might be a bit too late already.

I already agreed above about CONFIG_OF_ALL_DTBS being suboptimal,
but I don't think that the need for making sure the tests are present
would be an issue.  Making sure that the tests are defined should be
simply part of the reviewing, and running those tests is actually
a good way to make revieving a bit easier.  Thus, reviewers should
actually be motivated to make sure the tests are present.

>> I'd actually prefer to just have these test definitions added to the
>> end of arch/arm64/boot/dts/ti/Makefile, without moving the .dtbo lines
>> around.  That would keep the tests separate, which should be more
>> readable when we get more of them defined.
> 
> And more likely to forget about adding either the tests or the DTBO
> explicit rule.

Well, if someone can't remember either of those, then I don't see how
they can remember many other rules required to write DT code. :)

FWIW, both TI and Qualcomm place the test definitions separately from
the additions to dtb-$(CONFIG_ARCH_XYZ).

>> 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 kind of disagree with the feedback here as this only takes
> ti/Makefile as example while **all** other aarch64 Makefile have
> different logic, the one I'm using here for amlogic (meson), NXP
> (freescale), qcom (Qualcomm), Renesas, Xilinx and even Aarch32 NXP,
> and the one we currently use (no build tests) for Mediatek.

Hmm, I see.  To me, the approach taken by TI is more clear, simply
because having .dtb filenames, instead of .dtbo filenames, as part of
dtb-$(CONFIG_ARCH_XYZ) is much less readable.  When you see a .dtbo
there, it's clear that it's an overlay, and as I wrote above, it's
all about the overlays, so they should be both specified in Makefiles
and echoed back by the build system.

> If we agree to what you suggest here we cancel the side-effect of
> having the symbols in the base DTB that this patch series introduces.
> This can go both ways, either good (DT symbols in = nothing to do for
> the user, get the base DTB and your DTBO, put in /boot and tada) or
> bad (DT symbols in = big size increase for base DTB). Moreover,
> enabling CONFIG_OF_ALL_DTBS would now generate a different DTB than
> when it's not (keeping the symbols in). If we wanted to keep the
> symbols in, we would need to modify DTC_FLAGS as well.
> This could help make the decision(s) as well.
> 
> So I would say without much more context on the actual expected use
> for CONFIG_OF_ALL_DTBS that it's up to one's taste, and considering
> the precedent here, likely up to each architecture maintainers' taste.

Good point.  This just shows how suboptimal is tying CONFIG_OF_ALL_DTBS
with the DT overlay tests.  Perhaps it would be best to remove that
association, while keeping the ability to specify .dtbo filenames in
dtb-$(CONFIG_ARCH_XYZ).  Specifying .dtb files there simply looks wrong
to me, as I already explained above.

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

> Additionally, depending on the feedback, I assume we want to migrate
> all current architectures building DTBO to be consistent and use the
> same logic (perhaps not for Mediatek because doing so would keep the
> DT symbols in the DTB, which drastically increases the size of the
> DT).
> 
> Does anyone from DT maintainership have feedback to provide on what is
> expected here generally wrt building and testing DTBOs?
> 
> Does Heiko have an opinion on what he would prefer to see happening
> for Rockchip?

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

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.

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

[Dragan Simic]

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.

[PATCH v4 4/4] arm64: dts: rockchip: minimal support for Pre-ICT tester adapter for RK3588 Jaguar

[Quentin Schulz]

From: Quentin Schulz <quentin.schulz@cherry.de>

The Pre-ICT tester adapter connects to RK3588 Jaguar SBC through its
proprietary Mezzanine connector.

It exposes a PCIe Gen2 1x M.2 connector and two proprietary camera
connectors. Support for the latter will come once the rest of the camera
stack is supported.

Additionally, the adapter loops some GPIOs together as well as route
some GPIOs to power rails.

This adapter is used for manufacturing RK3588 Jaguar to be able to test
the Mezzanine connector is properly soldered.

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
---
 arch/arm64/boot/dts/rockchip/Makefile              |   4 +
 .../dts/rockchip/rk3588-jaguar-pre-ict-tester.dtso | 171 +++++++++++++++++++++
 2 files changed, 175 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
index ebcd16ce976ebe56a98d9685228980cd1f2f180a..a09b9c0060f8ecde582ad39f0f3e27047a31ec9c 100644
--- a/arch/arm64/boot/dts/rockchip/Makefile
+++ b/arch/arm64/boot/dts/rockchip/Makefile
@@ -185,6 +185,10 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-edgeble-neu6a-wifi.dtb
 rk3588-edgeble-neu6a-wifi-dtbs := rk3588-edgeble-neu6a-io.dtb \
 	rk3588-edgeble-neu6a-wifi.dtbo
 
+dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-jaguar-pre-ict-tester.dtb
+rk3588-jaguar-pre-ict-tester-dtbs := rk3588-jaguar.dtb \
+	rk3588-jaguar-pre-ict-tester.dtbo
+
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b-pcie-ep.dtb
 rk3588-rock-5b-pcie-ep-dtbs := rk3588-rock-5b.dtb \
 	rk3588-rock-5b-pcie-ep.dtbo
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-jaguar-pre-ict-tester.dtso b/arch/arm64/boot/dts/rockchip/rk3588-jaguar-pre-ict-tester.dtso
new file mode 100644
index 0000000000000000000000000000000000000000..9d44dfe2f30d793accb994a230c58038f0c3daad
--- /dev/null
+++ b/arch/arm64/boot/dts/rockchip/rk3588-jaguar-pre-ict-tester.dtso
@@ -0,0 +1,171 @@
+// SPDX-License-Identifier: (GPL-2.0-or-later OR MIT)
+/*
+ * Copyright (c) 2024 Cherry Embedded Solutions GmbH
+ *
+ * Device Tree Overlay for the Pre-ICT tester adapter for the Mezzanine
+ * connector on RK3588 Jaguar.
+ *
+ * This adapter has a PCIe Gen2 x1 M.2 M-Key connector and two proprietary
+ * camera connectors (each their own I2C bus, clock, reset and PWM lines as well
+ * as 2-lane CSI).
+ *
+ * This adapter routes some GPIOs to power rails and loops together some other
+ * GPIOs.
+ *
+ * This adapter is used during manufacturing for validating proper soldering of
+ * the mezzanine connector.
+ */
+
+/dts-v1/;
+/plugin/;
+
+#include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/pinctrl/rockchip.h>
+
+&{/} {
+	pre_ict_tester_vcc_1v2: regulator-pre-ict-tester-vcc-1v2 {
+		compatible = "regulator-fixed";
+		regulator-name = "pre_ict_tester_vcc_1v2";
+		regulator-always-on;
+		regulator-boot-on;
+		regulator-min-microvolt = <1200000>;
+		regulator-max-microvolt = <1200000>;
+		vin-supply = <&vcc_3v3_s3>;
+	};
+
+	pre_ict_tester_vcc_2v8: regulator-pre-ict-tester-vcc-2v8 {
+		compatible = "regulator-fixed";
+		regulator-name = "pre_ict_tester_vcc_2v8";
+		regulator-always-on;
+		regulator-boot-on;
+		regulator-min-microvolt = <2800000>;
+		regulator-max-microvolt = <2800000>;
+		vin-supply = <&vcc_3v3_s3>;
+	};
+};
+
+&combphy0_ps {
+	status = "okay";
+};
+
+&gpio3 {
+	pinctrl-0 = <&pre_ict_pwr2gpio>;
+	pinctrl-names = "default";
+};
+
+&pcie2x1l2 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pcie2x1l2_perstn_m0>;
+	reset-gpios = <&gpio3 RK_PD1 GPIO_ACTIVE_HIGH>; /* PCIE20X1_2_PERSTN_M0 */
+	vpcie3v3-supply = <&vcc_3v3_s3>;
+	status = "okay";
+};
+
+&pinctrl {
+	pcie2x1l2 {
+		pcie2x1l2_perstn_m0: pcie2x1l2-perstn-m0 {
+			rockchip,pins = <3 RK_PD1 RK_FUNC_GPIO &pcfg_pull_none>;
+		};
+	};
+
+	pre-ict-tester {
+		pre_ict_pwr2gpio: pre-ict-pwr2gpio-pins {
+			rockchip,pins =
+			/*
+			 * GPIO3_A3 requires two power rails to be properly
+			 * routed to the mezzanine connector to report a proper
+			 * value: VCC_1V8_S0_1 and VCC_IN_2. It may report an
+			 * incorrect value if VCC_1V8_S0_1 isn't properly routed,
+			 * but GPIO3_C6 would catch this HW soldering issue.
+			 * If VCC_IN_2 is properly routed, GPIO3_A3 should be
+			 * LOW. The signal shall not read HIGH in the event
+			 * GPIO3_A3 isn't properly routed due to soldering
+			 * issue. Therefore, let's enforce a pull-up (which is
+			 * the SoC default for this pin).
+			 */
+				<3 RK_PA3 RK_FUNC_GPIO &pcfg_pull_up>,
+			/*
+			 * GPIO3_A4 is directly routed to VCC_1V8_S0_2 power
+			 * rail. It should be HIGH if all is properly soldered.
+			 * To guarantee that, a pull-down is enforced (which is
+			 * the SoC default for this pin) so that LOW is read if
+			 * the loop doesn't exist on HW (soldering issue on
+			 * either signals).
+			 */
+				<3 RK_PA4 RK_FUNC_GPIO &pcfg_pull_down>,
+			/*
+			 * GPIO3_B2 requires two power rails to be properly
+			 * routed to the mezzanine connector to report a proper
+			 * value: VCC_1V8_S0_1 and VCC_IN_1. It may report an
+			 * incorrect value if VCC_1V8_S0_1 isn't properly routed,
+			 * but GPIO3_C6 would catch this HW soldering issue.
+			 * If VCC_IN_1 is properly routed, GPIO3_B2 should be
+			 * LOW. This is an issue if GPIO3_B2 isn't properly
+			 * routed due to soldering issue, because GPIO3_B2
+			 * default bias is pull-down therefore being LOW. So
+			 * the worst case scenario and the pass scenario expect
+			 * the same value. Make GPIO3_B2 a pull-up so that a
+			 * soldering issue on GPIO3_B2 reports HIGH but proper
+			 * soldering reports LOW.
+			 */
+				<3 RK_PB2 RK_FUNC_GPIO &pcfg_pull_up>,
+			/*
+			 * GPIO3_C6 is directly routed to VCC_1V8_S0_1 power
+			 * rail. It should be HIGH if all is properly soldered.
+			 * This is an issue if GPIO3_C6 or VCC_1V8_S0_1 isn't
+			 * properly routed due to soldering issue, because
+			 * GPIO3_C6 default bias is pull-up therefore being HIGH
+			 * in all cases:
+			 *  - GPIO3_C6 is floating (so HIGH) if GPIO3_C6 is not
+			 *    routed properly,
+			 *  - GPIO3_C6 is floating (so HIGH) if VCC_1V8_S0_1 is
+			 *    not routed properly,
+			 *  - GPIO3_C6 is HIGH if everything is proper,
+			 * Make GPIO3_C6 a pull-down so that a soldering issue
+			 * on GPIO3_C6 or VCC_1V8_S0_1 reports LOW but proper
+			 * soldering reports HIGH.
+			 */
+				<3 RK_PC6 RK_FUNC_GPIO &pcfg_pull_down>,
+			/*
+			 * GPIO3_D2 is routed to VCC_5V0_1 power rail through a
+			 * voltage divider on the adapter.
+			 * It should be HIGH if all is properly soldered.
+			 * To guarantee that, a pull-down is enforced (which is
+			 * the SoC default for this pin) so that LOW is read if
+			 * the loop doesn't exist on HW (soldering issue on
+			 * either signals).
+			 */
+				<3 RK_PD2 RK_FUNC_GPIO &pcfg_pull_down>,
+			/*
+			 * GPIO3_D3 is routed to VCC_5V0_2 power rail through a
+			 * voltage divider on the adapter.
+			 * It should be HIGH if all is properly soldered.
+			 * To guarantee that, a pull-down is enforced (which is
+			 * the SoC default for this pin) so that LOW is read if
+			 * the loop doesn't exist on HW (soldering issue on
+			 * either signals).
+			 */
+				<3 RK_PD3 RK_FUNC_GPIO &pcfg_pull_down>,
+			/*
+			 * GPIO3_D4 is routed to VCC_3V3_S3_1 power rail through
+			 * a voltage divider on the adapter.
+			 * It should be HIGH if all is properly soldered.
+			 * To guarantee that, a pull-down is enforced (which is
+			 * the SoC default for this pin) so that LOW is read if
+			 * the loop doesn't exist on HW (soldering issue on
+			 * either signals).
+			 */
+				<3 RK_PD4 RK_FUNC_GPIO &pcfg_pull_down>,
+			/*
+			 * GPIO3_D5 is routed to VCC_3V3_S3_2 power rail through
+			 * a voltage divider on the adapter.
+			 * It should be HIGH if all is properly soldered.
+			 * To guarantee that, a pull-down is enforced (which is
+			 * the SoC default for this pin) so that LOW is read if
+			 * the loop doesn't exist on HW (soldering issue on
+			 * either signals).
+			 */
+				<3 RK_PD5 RK_FUNC_GPIO &pcfg_pull_down>;
+		};
+	};
+};

-- 
2.48.1

Re: [PATCH v4 4/4] arm64: dts: rockchip: minimal support for Pre-ICT tester adapter for RK3588 Jaguar

[Dragan Simic]

On 2025-01-31 11:40, Quentin Schulz wrote:
> From: Quentin Schulz <quentin.schulz@cherry.de>
> 
> The Pre-ICT tester adapter connects to RK3588 Jaguar SBC through its
> proprietary Mezzanine connector.
> 
> It exposes a PCIe Gen2 1x M.2 connector and two proprietary camera
> connectors. Support for the latter will come once the rest of the 
> camera
> stack is supported.
> 
> Additionally, the adapter loops some GPIOs together as well as route
> some GPIOs to power rails.
> 
> This adapter is used for manufacturing RK3588 Jaguar to be able to test
> the Mezzanine connector is properly soldered.
> 
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
> ---
>  arch/arm64/boot/dts/rockchip/Makefile              |   4 +
>  .../dts/rockchip/rk3588-jaguar-pre-ict-tester.dtso | 171 
> +++++++++++++++++++++
>  2 files changed, 175 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/Makefile
> b/arch/arm64/boot/dts/rockchip/Makefile
> index
> ebcd16ce976ebe56a98d9685228980cd1f2f180a..a09b9c0060f8ecde582ad39f0f3e27047a31ec9c
> 100644
> --- a/arch/arm64/boot/dts/rockchip/Makefile
> +++ b/arch/arm64/boot/dts/rockchip/Makefile
> @@ -185,6 +185,10 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) +=
> rk3588-edgeble-neu6a-wifi.dtb
>  rk3588-edgeble-neu6a-wifi-dtbs := rk3588-edgeble-neu6a-io.dtb \
>  	rk3588-edgeble-neu6a-wifi.dtbo
> 
> +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-jaguar-pre-ict-tester.dtb
> +rk3588-jaguar-pre-ict-tester-dtbs := rk3588-jaguar.dtb \
> +	rk3588-jaguar-pre-ict-tester.dtbo
> +
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b-pcie-ep.dtb
>  rk3588-rock-5b-pcie-ep-dtbs := rk3588-rock-5b.dtb \
>  	rk3588-rock-5b-pcie-ep.dtbo

Obviously, virtually the same comments [*] I made on the patch 3/4
from this series apply here as well.

[*] 
https://lore.kernel.org/linux-rockchip/77b5d0efa6e56bb391575aeeb4d1be80@manjaro.org/