diff mbox series

[v2,2/3] arm64: dts: rockchip: add overlay tests for Rock 5B PCIe overlays

Message ID 20250116-pre-ict-jaguar-v2-2-157d319004fc@cherry.de (mailing list archive)
State New
Headers show
Series arm64: dts: rockchip: minimal support for Pre-ICT tester adapter for RK3588 Jaguar + add overlay tests | expand

Commit Message

Quentin Schulz Jan. 16, 2025, 2:47 p.m. UTC 
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.

Fixes: 40658534756f ("arm64: dts: rockchip: Add rock5b overlays for PCIe endpoint mode")
Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
---
 arch/arm64/boot/dts/rockchip/Makefile | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Niklas Cassel Jan. 17, 2025, 10:54 a.m. UTC | #1 
On Thu, Jan 16, 2025 at 03:47:11PM +0100, 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.
> 
> Fixes: 40658534756f ("arm64: dts: rockchip: Add rock5b overlays for PCIe endpoint mode")

Not sure if I agree with the Fixes-tag.
I don't think there is anything broken with that commit, but I definitely
think that it is a good idea make sure that they don't break in the future.


> @@ -145,8 +145,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
> @@ -165,5 +163,9 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-rock-5c.dtb
>  
>  # Overlays
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-edgeble-neu6a-wifi.dtb
> +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b-pcie-ep.dtb
> +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b-pcie-srns.dtb

You moved these lines further down, but you changed the suffix from
.dtbo to .dtb, why? It seems a little confusing, is that really needed?


Kind regards,
Niklas

>  
>  rk3588-edgeble-neu6a-wifi-dtbs := rk3588-edgeble-neu6a-io.dtb rk3588-edgeble-neu6a-wifi.dtbo
> +rk3588-rock-5b-pcie-ep-dtbs := rk3588-rock-5b.dtb rk3588-rock-5b-pcie-ep.dtbo
> +rk3588-rock-5b-pcie-srns-dtbs := rk3588-rock-5b.dtb rk3588-rock-5b-pcie-srns.dtbo
> 
> -- 
> 2.48.0
>
Krzysztof Kozlowski Jan. 17, 2025, 11:19 a.m. UTC | #2 
On 16/01/2025 15:47, 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.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof
Quentin Schulz Jan. 17, 2025, 11:50 a.m. UTC | #3 
Hi Niklas,

On 1/17/25 11:54 AM, Niklas Cassel wrote:
> On Thu, Jan 16, 2025 at 03:47:11PM +0100, 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.
>>
>> Fixes: 40658534756f ("arm64: dts: rockchip: Add rock5b overlays for PCIe endpoint mode")
> 
> Not sure if I agree with the Fixes-tag.
> I don't think there is anything broken with that commit, but I definitely
> think that it is a good idea make sure that they don't break in the future.
> 

That's fair. I actually think it'd be a good idea to backport this to 
stable releases. It could be possible for stable right now to somehow 
only backport changes to the base DT without modifying the DTO (or 
vice-versa) and then break the overlay unknowingly.

I added the Fixes to make it a bit simpler to know up to when to 
backport it, though I still haven't decided if I should have added Cc: 
stable there.

So 1) what's your opinion on backporting this to stable
2) if backporting, shouldn't I still remove the Fixes?

> 
>> @@ -145,8 +145,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
>> @@ -165,5 +163,9 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-rock-5c.dtb
>>   
>>   # Overlays
>>   dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-edgeble-neu6a-wifi.dtb
>> +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b-pcie-ep.dtb
>> +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b-pcie-srns.dtb
> 
> You moved these lines further down, but you changed the suffix from
> .dtbo to .dtb, why? It seems a little confusing, is that really needed?
> 

That was also confusing to me, but that's actually how it works.

rk3588-rock-5b-pcie-ep.dtb somehow points at rk3588-rock-5b-pcie-ep-dtbs 
which is the overlay application test of rk3588-rock-5b-pcie-ep.dtbo 
onto rk3588-rock-5b.dtb. For that to work, the .dtbo needs to be 
compiled. The target must be auto-generated somewhere because that still 
works. You can remove all *.dtb* from the tree, apply this patch and 
compile with make dtbs and you'll see that the DTBO is generated and the 
build time test of overlay application done as well (the log line starts 
with OVL).

Not sure exactly how to make this a bit less confusing in the commit 
log, as I myself do not really know why that is necessary or how it is 
working. But "it works" (tm).

This matches what was done for ti/ as well.

Cheers,
Quentin
Niklas Cassel Jan. 17, 2025, 1:04 p.m. UTC | #4 
Hello Quentin,

On Fri, Jan 17, 2025 at 12:50:52PM +0100, Quentin Schulz wrote:
> Hi Niklas,
> 
> On 1/17/25 11:54 AM, Niklas Cassel wrote:
> > On Thu, Jan 16, 2025 at 03:47:11PM +0100, 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.
> > > 
> > > Fixes: 40658534756f ("arm64: dts: rockchip: Add rock5b overlays for PCIe endpoint mode")
> > 
> > Not sure if I agree with the Fixes-tag.
> > I don't think there is anything broken with that commit, but I definitely
> > think that it is a good idea make sure that they don't break in the future.
> > 
> 
> That's fair. I actually think it'd be a good idea to backport this to stable
> releases. It could be possible for stable right now to somehow only backport
> changes to the base DT without modifying the DTO (or vice-versa) and then
> break the overlay unknowingly.
> 
> I added the Fixes to make it a bit simpler to know up to when to backport
> it, though I still haven't decided if I should have added Cc: stable there.

I know what Greg would have said:
https://lore.kernel.org/linux-pci/2025011651-stubborn-certified-b22f@gregkh/


> 
> So 1) what's your opinion on backporting this to stable
> 2) if backporting, shouldn't I still remove the Fixes?

My personal opinion is that a lot of companies are way too greedy, just
taking the work of upstream, but never giving anything back by actually
employing people working on upstream, all while sometimes having hundreds
of employees working on downstream.

I wish I could stop marking patches with CC: stable just to not make their
lives easier. If they want the good stuff, at least they should spend time
upgrading from their 4.14 kernel... Note that I do still (reluctantly) mark
my fixes with CC: stable.


> 
> > 
> > > @@ -145,8 +145,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
> > > @@ -165,5 +163,9 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-rock-5c.dtb
> > >   # Overlays
> > >   dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-edgeble-neu6a-wifi.dtb
> > > +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b-pcie-ep.dtb
> > > +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b-pcie-srns.dtb
> > 
> > You moved these lines further down, but you changed the suffix from
> > .dtbo to .dtb, why? It seems a little confusing, is that really needed?
> > 
> 
> That was also confusing to me, but that's actually how it works.
> 
> rk3588-rock-5b-pcie-ep.dtb somehow points at rk3588-rock-5b-pcie-ep-dtbs
> which is the overlay application test of rk3588-rock-5b-pcie-ep.dtbo onto
> rk3588-rock-5b.dtb. For that to work, the .dtbo needs to be compiled. The
> target must be auto-generated somewhere because that still works. You can
> remove all *.dtb* from the tree, apply this patch and compile with make dtbs
> and you'll see that the DTBO is generated and the build time test of overlay
> application done as well (the log line starts with OVL).
> 
> Not sure exactly how to make this a bit less confusing in the commit log, as
> I myself do not really know why that is necessary or how it is working. But
> "it works" (tm).
> 
> This matches what was done for ti/ as well.

As long as the files still get generated with the .dtbo offset, I'm happy.


Kind regards,
Niklas
Niklas Cassel Jan. 17, 2025, 1:08 p.m. UTC | #5 
On Thu, Jan 16, 2025 at 03:47:11PM +0100, 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.
> 
> Fixes: 40658534756f ("arm64: dts: rockchip: Add rock5b overlays for PCIe endpoint mode")
> Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
> ---

Reviewed-by: Niklas Cassel <cassel@kernel.org>
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
index 3f92d1a9d0b6efeee46ad4aff8c2b8ed3cb83d73..72f8666b42400a2f462421f5db6b3cb0cf369611 100644
--- a/arch/arm64/boot/dts/rockchip/Makefile
+++ b/arch/arm64/boot/dts/rockchip/Makefile
@@ -145,8 +145,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
@@ -165,5 +163,9 @@  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-rock-5c.dtb
 
 # Overlays
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-edgeble-neu6a-wifi.dtb
+dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b-pcie-ep.dtb
+dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b-pcie-srns.dtb
 
 rk3588-edgeble-neu6a-wifi-dtbs := rk3588-edgeble-neu6a-io.dtb rk3588-edgeble-neu6a-wifi.dtbo
+rk3588-rock-5b-pcie-ep-dtbs := rk3588-rock-5b.dtb rk3588-rock-5b-pcie-ep.dtbo
+rk3588-rock-5b-pcie-srns-dtbs := rk3588-rock-5b.dtb rk3588-rock-5b-pcie-srns.dtbo