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

[Michael Riesch]

Hi Quentin,

On 1/20/25 10:20, Quentin Schulz wrote:
> Hi Michael,
> 
> On 1/20/25 10:06 AM, Michael Riesch wrote:
>> Hi Quentin,
>>
>> On 1/16/25 15:47, Quentin Schulz wrote:
>>> 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 (some)
>>> Rockchip overlays to try to avoid future regressions.
>>>
>>> Notably, the Device Trees from Wolfvision PF5 aren't migrated (but
>>> should) as I do not own the device and couldn't figure out from the
>>> introducing commit logs what the possible valid combinations are.
>>> +Cc Michael Riesch for awareness
>>
>> Thanks for the heads up!
>>
>> Just to make sure I understood correctly: By migrated you mean that the
>> overlay entries are moved to a separate section in the Makefile and
>> there are explicit combinations of base DTS and overlays for
>> compile-time testing purposes? If so, I don't consider the PF5 migration
>> as *that* urgent. I don't think you can break anything on our side. Or
>> am I missing something?
>>
> 
> I think it makes sense to backport the patches in this series (except
> the one adding support for the Pre-ICT tester adapter on RK3588 Jaguar).
> Because we cannot backport the patch for Pre-ICT tester adapter (it's an
> addition, not a bug fix), any patch that is applied after that one will
> result in a git conflict when backporting to stable releases.
> 
> I believe it makes sense to backport build time overlay application tests.
> 
> The git conflict will likely be no big deal but if we can avoid it,
> better avoid it :)

OK.

> 
>> Maybe you can move the lines
>>
>>    dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3568-wolfvision-pf5-display-vz.dtbo
>>    dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3568-wolfvision-pf5-io-expander.dtbo
>>
>> to the overlay section as well? This should not cause any functional
>> changes.
>>
> 
> The overlay section would only support the syntax which allows build
> time testing. I would like to avoid confusion around what to do when
> adding a new overlay. Specifically note the missing o in the extension
> with the build time tests.
> 
>>> I'm wondering if we shouldn't backport patches 1 and 2 to stable? In
>>> which case, it would make sense to try to have the Wolfvision PF5
>>> overlay tests merged before the addition of the Pre-ICT tester adapter
>>> support for RK3588 Jaguar as that one won't be backported to stable and
>>> backporting the Wolfvision overlay test would incur an unnecessary
>>> (though not difficult) git conflict to resolve.
>>>
>>> I also do not know what kind of tests we should have when overlay
>>> combinations are possible (let's say there are A, B and C overlays that
>>> can all be applied, should we have base + A, base + B, base + C,
>>> base + A + B, base + A + C, base + B + C and base + A + B + C tests?
>>> maybe even base + B + A, base + C + B, base A + C + B, base + B + A + C,
>>> base + B + C + A, base + C + B + A and base + C + A + B tests?).
>>
>> I have never been good at combinatorics, but I feel this has the
>> potential to explode :-) My two cents: The overlays *should* be
>> orthogonal to each other, i.e., no dependencies between them in the
>> sense that overlay A creates a node that is used by overlay B and that
>> sort of thing. Then
> 
> I disagree. I can already tell you the following usecase:
> 
> our Pre-ICT tester adapter for RK3588 Jaguar has two proprietary camera
> connectors. We already have two camera modules working with it, so the
> following would make sense:
> 
> pre-ict-tester.dtbo
> pre-ict-tester-con1-camX.dtbo
> pre-ict-tester-con1-camY.dtbo
> pre-ict-tester-con2-camX.dtbo
> pre-ict-tester-con2-camY.dtbo
> 
> You would then apply pre-ict-tester.dtbo followed by one or two cam
> dtbos. The pre-ict-tester can be used without the camera modules (c.f.
> this patch :) ).

Most probably there is no general answer. In the end the use cases
decide what tests do make sense.

> 
>>   - Permutation can be ignored. (base + A + C = base + C + A)
> 
> I think that's fair. It would anyway be an issue with dtso which are
> using /delete-node/ or /delete-property/.
> 
>>   - Composition (base + A + B + C) may be ignored in favor of individual
>>     tests.
> 
> Not sure this is ideal either.
> 
> Our RK3588 Jaguar main PCBA also has two proprietary camera connectors.
> It would make sense to test that applying a dtso for main PCBA is not
> messing with applying a dtso for the camera module on the adapter.
> 
> This is a bit theoretical at the moment though since there's no camera
> stack available for RK3588.
> 
>>   - Individual tests may be ignored in favor of (a) composition(s) that
>>     cover(s) all individual tests.
>>
>> But of course this is likely to vary from case to case. In our case, in
>> the composition
>>
>>    rk3568-wolfvision-pf5-vz-2-uhd := rk3568-wolfvision-pf5.dtb \
>>      rk3568-wolfvision-pf5-io-expander.dtbo \
>>      rk3568-wolfvision-pf5-display-vz.dtbo
>>
>> would do the trick.
>>
> 
> Thanks, will send a patch for that in v3.

That'd be great, thanks!

Regards, Michael

> 
> Cheers,
> Quentin