Re: [PATCH 4/7] arm64: dts: qcom: sm8450: Add CAMSS device tree node

[Vladimir Zapolskiy <vz@kernel.org>]

Hi Loic.

On 10/27/26 12:31, Loic Poulain wrote:
> Hi Vlad,
> 
> 
> On Tue, Oct 27, 2026 at 10:54 AM Vladimir Zapolskiy <vz@kernel.org> wrote:
>>
>> On 10/27/26 11:07, Loic Poulain wrote:
>>> On Mon, Oct 26, 2026 at 11:39 PM Vladimir Zapolskiy <vz@kernel.org> wrote:
>>>>
>>>> Hi Loic.
>>>>
>>>> On 10/26/26 18:32, Loic Poulain wrote:
>>>>> On Fri, Oct 23, 2026 at 6:31 PM Vladimir Zapolskiy <vz@kernel.org> wrote:
>>>>>>
>>>>>> Add Qualcomm Waipio CAMSS device tree node to the platform dtsi file,
>>>>>> the CAMSS input ports are connected to output ports of CSIPHY devices.
>>>>>>
>>>>>> Signed-off-by: Vladimir Zapolskiy <vz@kernel.org>
>>>>>> ---
>>>>>>     arch/arm64/boot/dts/qcom/sm8450.dtsi | 167 +++++++++++++++++++++++++++
>>>>>>     1 file changed, 167 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
>>>>>> index babf1a4196a1..7f5b0501a23e 100644
>>>>>> --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
>>>>>> +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
>>>>>> @@ -3310,6 +3310,167 @@ cci1_i2c1: i2c-bus@1 {
>>>>>>                            };
>>>>>>                    };
>>>>>>
>>>>>> +               camss: isp@acb6000 {
>>>>>> +                       compatible = "qcom,waipio-camss";
>>>>>> +                       reg = <0 0xacb6000 0 0x1000>,
>>>>>> +                             <0 0xacb7000 0 0x1000>,
>>>>>> +                             <0 0xacb9000 0 0x1000>,
>>>>>> +                             <0 0xacbb000 0 0x1000>,
>>>>>> +                             <0 0xacc6000 0 0x1000>,
>>>>>> +                             <0 0xacca000 0 0x1000>,
>>>>>> +                             <0 0xacf6000 0 0x1000>,
>>>>>> +                             <0 0xacf7000 0 0x1000>,
>>>>>> +                             <0 0xacf8000 0 0x1000>,
>>>>>> +                             <0 0xac62000 0 0x3000>,
>>>>>> +                             <0 0xac71000 0 0x3000>,
>>>>>> +                             <0 0xac80000 0 0x3000>,
>>>>>> +                             <0 0xacc7000 0 0x2000>,
>>>>>> +                             <0 0xaccb000 0 0x2000>;
>>>>>> +                       reg-names = "csid_wrapper",
>>>>>> +                                   "csid0",
>>>>>> +                                   "csid1",
>>>>>> +                                   "csid2",
>>>>>> +                                   "csid_lite0",
>>>>>> +                                   "csid_lite1",
>>>>>> +                                   "tpg0",
>>>>>> +                                   "tpg1",
>>>>>> +                                   "tpg2",
>>>>>> +                                   "vfe0",
>>>>>> +                                   "vfe1",
>>>>>> +                                   "vfe2",
>>>>>> +                                   "vfe_lite0",
>>>>>> +                                   "vfe_lite1";
>>>>>
>>>>> Is there a camss "top" register to describe here? e.g. to retrieve the
>>>>> CAMSS hardware version.
>>>>
>>>> I'm not aware of anything like that. What is "CAMSS hardware version"?
>>>>
>>>> CAMSS embraces various blocks, there are CSID and VFE, full and lite
>>>> IPs, and all these components do export their versions individually.
>>>
>>> I mean, CAMSS usually has a top-level register block (often top or
>>> cpas_top) that contains the Titan/Spectra version information, along
>>> with various global configuration and mux registers.
>>
>> On SM8450 there is "cpas_top" register <0xac13000 0x1000> (not in the
>> series), I believe you reference to it.
>>
>> Is CPAS a natural part of CAMSS? Can it be modelled differently, e.g.
>> as a separate device/driver, like it was done with JPEG or CSIPHY?
>>
>> I see that there are only SM6150 and SM6350 platforms mention this
>> register, to capture and process frames on SM8450 there is no need
>> to program CPAS.
>>
>> Note, that even SM6150 and SM6350 platforms do not use this register,
>> it makes it redundant.
>>
>>> In particular, exposing this block could be useful for reporting the
>>> Spectra version to userspace, for example through libcamera.
>>
>> It could be so, but adding dead code is an anti-pattern in software design.
>>
>>> Moreover, if the goal is to describe the CAMSS hardware exhaustively
>>> in DT, this block should probably be represented as well. Whether it
>>
>> Here I oppose such a goal as impractical, the set goal is to get working
>> camera on the platform with the minimum of irreversible changes. It was
>> proven multiple times in the past, that "an exhaustive description"
>> equals to "the totally broken description", and brings nothing valuable
>> but irrecoverable ethernal problems to overcome.
>>
>>> belongs in this specific binding is open for discussion, but my
>>> inclination is that it does.
>>
>> This register is not needed to get CAMSS up to the functionally
>> working stage, which is represented by the series.
> 
> CAMSS already supports reading the hardware version from such
> top-level register blocks and exposing it through the media framework.
> While this is not strictly required, it will likely become useful to
> libcamera in the future.

Likely so, and I didn't know about the fact that "cpas_top" stores some
hardware version information, which may be utilized by userspace, thank
you for sharing this.

Anyway at this stage, if there is no strong objections, I'd prefer to
follow the conservative model of not adding "cpas_top" to CAMSS.

Like it's been said, it might be preferred to create a new stand-alone
CPAS device abstraction, with no conflict for I/O regions with CAMSS,
especially assuming the latter one just does not need this I/O region
at all to be functional.

Also, I'm a bit concerned when the abstraction inversion is used as
an argument, here the kernel should not depend on libcamera or other
userspace component, but the opposite is to be satisfied, i.e. libcamera
should work properly with 90% of CAMSS variants, which do not export
"cpas_top" info, like this or many other recently added CAMSS variants.

>> I believe someone may create a new fancy CPAS device driver or so, and
>> the new device driver will utilize the "cpas_top" register and other
>> CPAS resources in a better way, the register shall not be nailed to
>> the CAMSS preliminarly.
> 
> Well, yes, we could certainly model it as a syscon node, especially
> now that CAMSS is no longer the monolithic block it used to be.
> 

Sure, that's the view I share to exploit the modularisation at maximum.

As always, thank you very much for review!

-- 
Best wishes,
Vladimir

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy