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

[Bryan O'Donoghue]

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@xxxxxxxxxxxxxxxx>
> ---
>   .../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@xxxxxxxxxxxxxxxx>
> +
> +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@xxxxxxxxxxxxxxxx/

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.

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