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

[Vijay Kumar Tumati <vijay.tumati@oss.qualcomm.com>]

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