Re: [PATCH v5 2/6] media: dt-bindings: Add CAMSS device for Kaanapali

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

On 31/10/2025 17:39, Vijay Kumar Tumati wrote:
> 
> On 10/31/2025 6:50 AM, Bryan O'Donoghue wrote:
>> On 31/10/2025 02:59, Hangxiang Ma wrote:
>>> Add the compatible string "qcom,kaanapali-camss" to support the Camera
>>> Subsystem (CAMSS) on the Qualcomm Kaanapali platform.
>>>
>>> The Kaanapali platform provides:
>>> - 3 x VFE, 5 RDI per VFE
>>> - 2 x VFE Lite, 4 RDI per VFE Lite
>>> - 3 x CSID
>>> - 2 x CSID Lite
>>> - 6 x CSIPHY
>>>
>>> Signed-off-by: Hangxiang Ma <hangxiang.ma@oss.qualcomm.com>
>>> ---
>>>   .../bindings/media/qcom,kaanapali-camss.yaml       | 406 ++++++++++ 
>>> +++++++++++
>>>   1 file changed, 406 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/qcom,kaanapali- 
>>> camss.yaml b/Documentation/devicetree/bindings/media/qcom,kaanapali- 
>>> camss.yaml
>>> new file mode 100644
>>> index 000000000000..c34867022fd1
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/media/qcom,kaanapali-camss.yaml
>>> @@ -0,0 +1,406 @@
>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/media/qcom,kaanapali-camss.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Qualcomm Kaanapali Camera Subsystem (CAMSS)
>>> +
>>> +maintainers:
>>> +  - Hangxiang Ma <hangxiang.ma@oss.qualcomm.com>
>>> +
>>> +description:
>>> +  The CAMSS IP is a CSI decoder and ISP present on Qualcomm platforms.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: qcom,kaanapali-camss
>>> +
>>> +  reg:
>>> +    maxItems: 16
>>> +
>>> +  reg-names:
>>> +    items:
>>> +      - const: csid0
>>> +      - const: csid1
>>> +      - const: csid2
>>> +      - const: csid_lite0
>>> +      - const: csid_lite1
>>> +      - const: csiphy0
>>> +      - const: csiphy1
>>> +      - const: csiphy2
>>> +      - const: csiphy3
>>> +      - const: csiphy4
>>> +      - const: csiphy5
>>> +      - const: vfe0
>>> +      - const: vfe1
>>> +      - const: vfe2
>>> +      - const: vfe_lite0
>>> +      - const: vfe_lite1
>>
>> No test pattern generator on this part ?
>>
>> We have patches in-flight to add TPG so it makes no sense to omit 
>> these registers from current or new submissions.
>>
>> https://lore.kernel.org/linux-media/20251017-camss_tpg-v5-1- 
>> cafe3ad42163@oss.qualcomm.com/
>>
>> While we're at it we should consider adding in the other key 
>> functional blocks.
>>
>> OFE, IPE etc, there's no harm in including the registers even if the 
>> intention and outcome is never switching that functionality on.
>>
> Hi Bryan, we have quite a few register spaces on Kaanapali or any other 
> target that are not required for the RDI only CAMSS driver, including 
> ICP, JPEG, OFE, IPE, CDMs and some custom modules like CRE along with 
> the TPG. So do I understand your suggestion correctly that you advise 
> all of those are enlisted in the DTSI and the bindings although the 
> driver doesn't make use of or map them?

TPG is in process of being upstreamed by qcom.

I think the list of registers above should be included in the dts 
because the DTS is a description of hardware, not a description of 
camss/rdi.

The point of DTS is to do that, describe hardware and to be consumable 
outside of the upstream linux kernel.

u-boot, BSD, potentially even a downstream Linux kernel or driver.

>>> +
>>> +  clocks:
>>> +    maxItems: 34
>>> +
>>> +  clock-names:
>>> +    items:
>>> +      - const: camnoc_nrt_axi
>>> +      - const: camnoc_rt_axi
>>> +      - const: camnoc_rt_vfe0
>>> +      - const: camnoc_rt_vfe1
>>> +      - const: camnoc_rt_vfe2
>>> +      - const: camnoc_rt_vfe_lite
>>> +      - const: cam_top_ahb
>>> +      - const: cam_top_fast_ahb
>>> +      - const: csid
>>> +      - const: csid_csiphy_rx
>>> +      - const: csiphy0
>>> +      - const: csiphy0_timer
>>> +      - const: csiphy1
>>> +      - const: csiphy1_timer
>>> +      - const: csiphy2
>>> +      - const: csiphy2_timer
>>> +      - const: csiphy3
>>> +      - const: csiphy3_timer
>>> +      - const: csiphy4
>>> +      - const: csiphy4_timer
>>> +      - const: csiphy5
>>> +      - const: csiphy5_timer
>>> +      - const: gcc_hf_axi
>>> +      - const: vfe0
>>> +      - const: vfe0_fast_ahb
>>> +      - const: vfe1
>>> +      - const: vfe1_fast_ahb
>>> +      - const: vfe2
>>> +      - const: vfe2_fast_ahb
>>> +      - const: vfe_lite
>>> +      - const: vfe_lite_ahb
>>> +      - const: vfe_lite_cphy_rx
>>> +      - const: vfe_lite_csid
>>> +      - const: qdss_debug_xo
>>> +
>>> +  interrupts:
>>> +    maxItems: 16
>>> +
>>> +  interrupt-names:
>>> +    items:
>>> +      - const: csid0
>>> +      - const: csid1
>>> +      - const: csid2
>>> +      - const: csid_lite0
>>> +      - const: csid_lite1
>>> +      - const: csiphy0
>>> +      - const: csiphy1
>>> +      - const: csiphy2
>>> +      - const: csiphy3
>>> +      - const: csiphy4
>>> +      - const: csiphy5
>>> +      - const: vfe0
>>> +      - const: vfe1
>>> +      - const: vfe2
>>> +      - const: vfe_lite0
>>> +      - const: vfe_lite1
>>> +
>>> +  interconnects:
>>> +    maxItems: 2
>>> +
>>> +  interconnect-names:
>>> +    items:
>>> +      - const: ahb
>>> +      - const: hf_mnoc
>>> +
>>> +  iommus:
>>> +    maxItems: 1
>>
>>
>> This can't be right.
>>
>> The experience we are having with Iris for example shows that 
>> restricting the iommus is wrong.
>>
>> For this and future bindings I'm expecting to see the full list of 
>> AC_VM_HLOS S2 VMID targets.
>>
>> The second we try to switch on say something like the JPEG encoder 
>> this list and its upstream binding becomes a problem.
>>
>> - S1_IFE_HLOS        @ 0x1c00
>> - S1_CDM_BPS_IPS_HLOS    @ 0x1820
>> - S1_CDM_BPS_IPS_HLOS    @ 0x18c0
>> - S1_CDM_BPS_IPS_HLOS    @ 0x1980
>> - S1_CDM_BPS_IPS_HLOS    @ 0x1800
>> - S1_JPEG_HLOS        @ 0x18a0
>> - S1_RT_CDM_HLOS    @ 0x1860
>> - S1_CDM_BPS_IPE_HLOS    @ 0x1840
>> - S1_CDM_BPS_IPE_HLOS    @ 0x1880
>> - S1_CRE_HLOS        @ 0x18e0
>>
>> The ICP mappings can come later if ever via iommu-maps..
>>
>> ---
>> bod
>>
> Similar to the above, You are advising to declare all the S2 HLOS mapped 
> streams in the bindings and the DTSI? If we do that in the DTSI, I 
> wonder how we can specifically map the RDI output buffers to the IFE 
> context bank only, for instance, going by the current CAMSS driver 
> implementation. Perhaps, IFE should be the first one in the list for now 
> and the driver will be extended later when we support more devices? I 
> will explore on that. Good to understand these details and practices. 
> Thank you.
> 

We've run into trouble with that in Iris.

https://lore.kernel.org/linux-arm-msm/c9d8f76a-513f-4a09-bba4-cb8f0df1d2fe@kernel.org/

The right thing to do is to describe everything that targets the HLOS - 
main CPU.

For non CPU targets - like say setting up the SMMU for the ICP - we 
could add those mappings in with iommu-map later.

The CPU side SID map should be complete. It doesn't divulge any 
propitiatory information or secret sauce, it just makes our lives easier 
in the end.

---
bod