[PATCH v2 1/3] dt-bindings: media: camss: Add qcom,sm6350-camss

Luca Weiss posted 3 patches 2 months, 3 weeks ago
[PATCH v2 1/3] dt-bindings: media: camss: Add qcom,sm6350-camss
Posted by Luca Weiss 2 months, 3 weeks ago
Add bindings for the Camera Subsystem on the SM6350 SoC.

Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
---
 .../bindings/media/qcom,sm6350-camss.yaml          | 349 +++++++++++++++++++++
 1 file changed, 349 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/qcom,sm6350-camss.yaml b/Documentation/devicetree/bindings/media/qcom,sm6350-camss.yaml
new file mode 100644
index 000000000000..d812b5b50c05
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/qcom,sm6350-camss.yaml
@@ -0,0 +1,349 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/qcom,sm6350-camss.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm SM6350 Camera Subsystem (CAMSS)
+
+maintainers:
+  - Luca Weiss <luca.weiss@fairphone.com>
+
+description:
+  The CAMSS IP is a CSI decoder and ISP present on Qualcomm platforms.
+
+properties:
+  compatible:
+    const: qcom,sm6350-camss
+
+  reg:
+    maxItems: 12
+
+  reg-names:
+    items:
+      - const: csid0
+      - const: csid1
+      - const: csid2
+      - const: csid_lite
+      - const: csiphy0
+      - const: csiphy1
+      - const: csiphy2
+      - const: csiphy3
+      - const: vfe0
+      - const: vfe1
+      - const: vfe2
+      - const: vfe_lite
+
+  clocks:
+    maxItems: 30
+
+  clock-names:
+    items:
+      - const: cam_ahb_clk
+      - const: cam_axi
+      - const: soc_ahb
+      - const: camnoc_axi
+      - const: core_ahb
+      - const: cpas_ahb
+      - const: csiphy0
+      - const: csiphy0_timer
+      - const: csiphy1
+      - const: csiphy1_timer
+      - const: csiphy2
+      - const: csiphy2_timer
+      - const: csiphy3
+      - const: csiphy3_timer
+      - const: slow_ahb_src
+      - const: vfe0_axi
+      - const: vfe0
+      - const: vfe0_cphy_rx
+      - const: vfe0_csid
+      - const: vfe1_axi
+      - const: vfe1
+      - const: vfe1_cphy_rx
+      - const: vfe1_csid
+      - const: vfe2_axi
+      - const: vfe2
+      - const: vfe2_cphy_rx
+      - const: vfe2_csid
+      - const: vfe_lite
+      - const: vfe_lite_cphy_rx
+      - const: vfe_lite_csid
+
+  interrupts:
+    maxItems: 12
+
+  interrupt-names:
+    items:
+      - const: csid0
+      - const: csid1
+      - const: csid2
+      - const: csid_lite
+      - const: csiphy0
+      - const: csiphy1
+      - const: csiphy2
+      - const: csiphy3
+      - const: vfe0
+      - const: vfe1
+      - const: vfe2
+      - const: vfe_lite
+
+  interconnects:
+    maxItems: 4
+
+  interconnect-names:
+    items:
+      - const: ahb
+      - const: hf_mnoc
+      - const: sf_mnoc
+      - const: sf_icp_mnoc
+
+  iommus:
+    maxItems: 4
+
+  power-domains:
+    items:
+      - description: IFE0 GDSC - Image Front End, Global Distributed Switch Controller.
+      - description: IFE1 GDSC - Image Front End, Global Distributed Switch Controller.
+      - description: IFE2 GDSC - Image Front End, Global Distributed Switch Controller.
+      - description: Titan Top GDSC - Titan ISP Block, Global Distributed Switch Controller.
+
+  power-domain-names:
+    items:
+      - const: top
+      - const: ife0
+      - const: ife1
+      - const: ife2
+
+  vdd-csiphy-0p9-supply:
+    description:
+      Phandle to a 0.9V regulator supply to a PHY.
+
+  vdd-csiphy-1p25-supply:
+    description:
+      Phandle to a 1.25V regulator supply to a PHY.
+
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+
+    description:
+      CSI input ports.
+
+    patternProperties:
+      "^port@[0-3]$":
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        unevaluatedProperties: false
+
+        description:
+          Input port for receiving CSI data from a CSIPHY.
+
+        properties:
+          endpoint:
+            $ref: video-interfaces.yaml#
+            unevaluatedProperties: false
+
+            properties:
+              data-lanes:
+                minItems: 1
+                maxItems: 4
+
+              bus-type:
+                enum:
+                  - 1 # MEDIA_BUS_TYPE_CSI2_CPHY
+                  - 4 # MEDIA_BUS_TYPE_CSI2_DPHY
+
+            required:
+              - data-lanes
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - clocks
+  - clock-names
+  - interrupts
+  - interrupt-names
+  - interconnects
+  - interconnect-names
+  - iommus
+  - power-domains
+  - power-domain-names
+  - vdd-csiphy-0p9-supply
+  - vdd-csiphy-1p25-supply
+  - ports
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/qcom,gcc-sm6350.h>
+    #include <dt-bindings/clock/qcom,sm6350-camcc.h>
+    #include <dt-bindings/interconnect/qcom,icc.h>
+    #include <dt-bindings/interconnect/qcom,sm6350.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/media/video-interfaces.h>
+    #include <dt-bindings/power/qcom-rpmpd.h>
+
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        isp@acb3000 {
+            compatible = "qcom,sm6350-camss";
+
+            reg = <0x0 0x0acb3000 0x0 0x1000>,
+                  <0x0 0x0acba000 0x0 0x1000>,
+                  <0x0 0x0acc1000 0x0 0x1000>,
+                  <0x0 0x0acc8000 0x0 0x1000>,
+                  <0x0 0x0ac65000 0x0 0x1000>,
+                  <0x0 0x0ac66000 0x0 0x1000>,
+                  <0x0 0x0ac67000 0x0 0x1000>,
+                  <0x0 0x0ac68000 0x0 0x1000>,
+                  <0x0 0x0acaf000 0x0 0x4000>,
+                  <0x0 0x0acb6000 0x0 0x4000>,
+                  <0x0 0x0acbd000 0x0 0x4000>,
+                  <0x0 0x0acc4000 0x0 0x4000>;
+            reg-names = "csid0",
+                        "csid1",
+                        "csid2",
+                        "csid_lite",
+                        "csiphy0",
+                        "csiphy1",
+                        "csiphy2",
+                        "csiphy3",
+                        "vfe0",
+                        "vfe1",
+                        "vfe2",
+                        "vfe_lite";
+
+            clocks = <&gcc GCC_CAMERA_AHB_CLK>,
+                     <&gcc GCC_CAMERA_AXI_CLK>,
+                     <&camcc CAMCC_SOC_AHB_CLK>,
+                     <&camcc CAMCC_CAMNOC_AXI_CLK>,
+                     <&camcc CAMCC_CORE_AHB_CLK>,
+                     <&camcc CAMCC_CPAS_AHB_CLK>,
+                     <&camcc CAMCC_CSIPHY0_CLK>,
+                     <&camcc CAMCC_CSI0PHYTIMER_CLK>,
+                     <&camcc CAMCC_CSIPHY1_CLK>,
+                     <&camcc CAMCC_CSI1PHYTIMER_CLK>,
+                     <&camcc CAMCC_CSIPHY2_CLK>,
+                     <&camcc CAMCC_CSI2PHYTIMER_CLK>,
+                     <&camcc CAMCC_CSIPHY3_CLK>,
+                     <&camcc CAMCC_CSI3PHYTIMER_CLK>,
+                     <&camcc CAMCC_SLOW_AHB_CLK_SRC>,
+                     <&camcc CAMCC_IFE_0_AXI_CLK>,
+                     <&camcc CAMCC_IFE_0_CLK>,
+                     <&camcc CAMCC_IFE_0_CPHY_RX_CLK>,
+                     <&camcc CAMCC_IFE_0_CSID_CLK>,
+                     <&camcc CAMCC_IFE_1_AXI_CLK>,
+                     <&camcc CAMCC_IFE_1_CLK>,
+                     <&camcc CAMCC_IFE_1_CPHY_RX_CLK>,
+                     <&camcc CAMCC_IFE_1_CSID_CLK>,
+                     <&camcc CAMCC_IFE_2_AXI_CLK>,
+                     <&camcc CAMCC_IFE_2_CLK>,
+                     <&camcc CAMCC_IFE_2_CPHY_RX_CLK>,
+                     <&camcc CAMCC_IFE_2_CSID_CLK>,
+                     <&camcc CAMCC_IFE_LITE_CLK>,
+                     <&camcc CAMCC_IFE_LITE_CPHY_RX_CLK>,
+                     <&camcc CAMCC_IFE_LITE_CSID_CLK>;
+            clock-names = "cam_ahb_clk",
+                          "cam_axi",
+                          "soc_ahb",
+                          "camnoc_axi",
+                          "core_ahb",
+                          "cpas_ahb",
+                          "csiphy0",
+                          "csiphy0_timer",
+                          "csiphy1",
+                          "csiphy1_timer",
+                          "csiphy2",
+                          "csiphy2_timer",
+                          "csiphy3",
+                          "csiphy3_timer",
+                          "slow_ahb_src",
+                          "vfe0_axi",
+                          "vfe0",
+                          "vfe0_cphy_rx",
+                          "vfe0_csid",
+                          "vfe1_axi",
+                          "vfe1",
+                          "vfe1_cphy_rx",
+                          "vfe1_csid",
+                          "vfe2_axi",
+                          "vfe2",
+                          "vfe2_cphy_rx",
+                          "vfe2_csid",
+                          "vfe_lite",
+                          "vfe_lite_cphy_rx",
+                          "vfe_lite_csid";
+
+            interrupts = <GIC_SPI 464 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 466 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 717 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 473 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 477 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 478 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 479 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 461 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 465 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 467 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 718 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 472 IRQ_TYPE_LEVEL_HIGH>;
+            interrupt-names = "csid0",
+                              "csid1",
+                              "csid2",
+                              "csid_lite",
+                              "csiphy0",
+                              "csiphy1",
+                              "csiphy2",
+                              "csiphy3",
+                              "vfe0",
+                              "vfe1",
+                              "vfe2",
+                              "vfe_lite";
+
+            interconnects = <&gem_noc MASTER_AMPSS_M0 QCOM_ICC_TAG_ACTIVE_ONLY
+                             &config_noc SLAVE_CAMERA_CFG QCOM_ICC_TAG_ACTIVE_ONLY>,
+                            <&mmss_noc MASTER_CAMNOC_HF QCOM_ICC_TAG_ALWAYS
+                             &clk_virt SLAVE_EBI_CH0 QCOM_ICC_TAG_ALWAYS>,
+                            <&mmss_noc MASTER_CAMNOC_SF QCOM_ICC_TAG_ALWAYS
+                             &clk_virt SLAVE_EBI_CH0 QCOM_ICC_TAG_ALWAYS>,
+                            <&mmss_noc MASTER_CAMNOC_ICP QCOM_ICC_TAG_ALWAYS
+                             &clk_virt SLAVE_EBI_CH0 QCOM_ICC_TAG_ALWAYS>;
+            interconnect-names = "ahb",
+                                 "hf_mnoc",
+                                 "sf_mnoc",
+                                 "sf_icp_mnoc";
+
+            iommus = <&apps_smmu 0x820 0xc0>,
+                     <&apps_smmu 0x840 0x0>,
+                     <&apps_smmu 0x860 0xc0>,
+                     <&apps_smmu 0x880 0x0>;
+
+            power-domains = <&camcc TITAN_TOP_GDSC>
+                            <&camcc IFE_0_GDSC>,
+                            <&camcc IFE_1_GDSC>,
+                            <&camcc IFE_2_GDSC>;
+            power-domain-names = "top",
+                                 "ife0",
+                                 "ife1",
+                                 "ife2";
+
+            vdd-csiphy-0p9-supply = <&vreg_l18a>;
+            vdd-csiphy-1p25-supply = <&vreg_l22a>;
+
+            ports {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                port@0 {
+                    reg = <0>;
+                    csiphy0_ep: endpoint {
+                        data-lanes = <0 1 2 3>;
+                        bus-type = <MEDIA_BUS_TYPE_CSI2_DPHY>;
+                        remote-endpoint = <&sensor_ep>;
+                    };
+                };
+            };
+        };
+    };

-- 
2.51.2
Re: [PATCH v2 1/3] dt-bindings: media: camss: Add qcom,sm6350-camss
Posted by Rob Herring (Arm) 2 months, 3 weeks ago
On Fri, 14 Nov 2025 12:15:24 +0100, Luca Weiss wrote:
> Add bindings for the Camera Subsystem on the SM6350 SoC.
> 
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> ---
>  .../bindings/media/qcom,sm6350-camss.yaml          | 349 +++++++++++++++++++++
>  1 file changed, 349 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/media/qcom,sm6350-camss.example.dts:169.33-34 syntax error
FATAL ERROR: Unable to parse input tree
make[2]: *** [scripts/Makefile.dtbs:132: Documentation/devicetree/bindings/media/qcom,sm6350-camss.example.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1547: dt_binding_check] Error 2
make: *** [Makefile:248: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20251114-sm6350-camss-v2-1-d1ff67da33b6@fairphone.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
Re: [PATCH v2 1/3] dt-bindings: media: camss: Add qcom,sm6350-camss
Posted by Vladimir Zapolskiy 2 months, 3 weeks ago
Hi Luca.

On 11/14/25 13:15, Luca Weiss wrote:
> Add bindings for the Camera Subsystem on the SM6350 SoC.
> 
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> ---
>   .../bindings/media/qcom,sm6350-camss.yaml          | 349 +++++++++++++++++++++
>   1 file changed, 349 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/qcom,sm6350-camss.yaml b/Documentation/devicetree/bindings/media/qcom,sm6350-camss.yaml
> new file mode 100644
> index 000000000000..d812b5b50c05
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/qcom,sm6350-camss.yaml
> @@ -0,0 +1,349 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/qcom,sm6350-camss.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm SM6350 Camera Subsystem (CAMSS)
> +
> +maintainers:
> +  - Luca Weiss <luca.weiss@fairphone.com>
> +
> +description:
> +  The CAMSS IP is a CSI decoder and ISP present on Qualcomm platforms.
> +
> +properties:
> +  compatible:
> +    const: qcom,sm6350-camss
> +
> +  reg:
> +    maxItems: 12
> +
> +  reg-names:
> +    items:
> +      - const: csid0
> +      - const: csid1
> +      - const: csid2
> +      - const: csid_lite
> +      - const: csiphy0
> +      - const: csiphy1
> +      - const: csiphy2
> +      - const: csiphy3
> +      - const: vfe0
> +      - const: vfe1
> +      - const: vfe2
> +      - const: vfe_lite
> +
> +  clocks:
> +    maxItems: 30
> +
> +  clock-names:
> +    items:
> +      - const: cam_ahb_clk
> +      - const: cam_axi
> +      - const: soc_ahb
> +      - const: camnoc_axi
> +      - const: core_ahb
> +      - const: cpas_ahb
> +      - const: csiphy0
> +      - const: csiphy0_timer
> +      - const: csiphy1
> +      - const: csiphy1_timer
> +      - const: csiphy2
> +      - const: csiphy2_timer
> +      - const: csiphy3
> +      - const: csiphy3_timer
> +      - const: slow_ahb_src
> +      - const: vfe0_axi
> +      - const: vfe0
> +      - const: vfe0_cphy_rx
> +      - const: vfe0_csid
> +      - const: vfe1_axi
> +      - const: vfe1
> +      - const: vfe1_cphy_rx
> +      - const: vfe1_csid
> +      - const: vfe2_axi
> +      - const: vfe2
> +      - const: vfe2_cphy_rx
> +      - const: vfe2_csid
> +      - const: vfe_lite
> +      - const: vfe_lite_cphy_rx
> +      - const: vfe_lite_csid

The sorting order of this list does not follow the sorting order accepted
in the past.

I'm very sorry for the vagueness, but I can not pronounce the accepted
sorting order name, because it triggers people.

> +
> +  interrupts:
> +    maxItems: 12
> +
> +  interrupt-names:
> +    items:
> +      - const: csid0
> +      - const: csid1
> +      - const: csid2
> +      - const: csid_lite
> +      - const: csiphy0
> +      - const: csiphy1
> +      - const: csiphy2
> +      - const: csiphy3
> +      - const: vfe0
> +      - const: vfe1
> +      - const: vfe2
> +      - const: vfe_lite
> +
> +  interconnects:
> +    maxItems: 4
> +
> +  interconnect-names:
> +    items:
> +      - const: ahb
> +      - const: hf_mnoc
> +      - const: sf_mnoc
> +      - const: sf_icp_mnoc

Please remove sf_mnoc and sf_icp_mnoc, they are not needed for enabling
IP to produce raw images, and one day you may use them somewhere else.

> +
> +  iommus:
> +    maxItems: 4
> +
> +  power-domains:
> +    items:
> +      - description: IFE0 GDSC - Image Front End, Global Distributed Switch Controller.
> +      - description: IFE1 GDSC - Image Front End, Global Distributed Switch Controller.
> +      - description: IFE2 GDSC - Image Front End, Global Distributed Switch Controller.
> +      - description: Titan Top GDSC - Titan ISP Block, Global Distributed Switch Controller.
> +
> +  power-domain-names:
> +    items:
> +      - const: top
> +      - const: ife0
> +      - const: ife1
> +      - const: ife2

Note that the list of items and the list of the item descriptions do not
correspond to each other. Titan Top GDSC shall be at the end.

> +
> +  vdd-csiphy-0p9-supply:
> +    description:
> +      Phandle to a 0.9V regulator supply to a PHY.
> +
> +  vdd-csiphy-1p25-supply:
> +    description:
> +      Phandle to a 1.25V regulator supply to a PHY.
> +

Please reference to the schematics or SoC TRM, does SM6350 SoC
have different pads to get supplies to different CSIPHYx IPs?

If so, then please provide hardware properties to get a proper
correspondence between supplies and CSIPHYx, and make all these
properties optional.

> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +
> +    description:
> +      CSI input ports.
> +
> +    patternProperties:
> +      "^port@[0-3]$":
> +        $ref: /schemas/graph.yaml#/$defs/port-base
> +        unevaluatedProperties: false
> +
> +        description:
> +          Input port for receiving CSI data from a CSIPHY.
> +
> +        properties:
> +          endpoint:
> +            $ref: video-interfaces.yaml#
> +            unevaluatedProperties: false
> +
> +            properties:
> +              data-lanes:
> +                minItems: 1
> +                maxItems: 4
> +
> +              bus-type:
> +                enum:
> +                  - 1 # MEDIA_BUS_TYPE_CSI2_CPHY
> +                  - 4 # MEDIA_BUS_TYPE_CSI2_DPHY
> +
> +            required:
> +              - data-lanes
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - clocks
> +  - clock-names
> +  - interrupts
> +  - interrupt-names
> +  - interconnects
> +  - interconnect-names
> +  - iommus
> +  - power-domains
> +  - power-domain-names
> +  - vdd-csiphy-0p9-supply
> +  - vdd-csiphy-1p25-supply

When a change to add CSIPHYx specific supplies is done, please remove
*-supply properties from the list of the requred ones.

> +  - ports
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/qcom,gcc-sm6350.h>
> +    #include <dt-bindings/clock/qcom,sm6350-camcc.h>
> +    #include <dt-bindings/interconnect/qcom,icc.h>
> +    #include <dt-bindings/interconnect/qcom,sm6350.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/media/video-interfaces.h>
> +    #include <dt-bindings/power/qcom-rpmpd.h>
> +
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        isp@acb3000 {
> +            compatible = "qcom,sm6350-camss";
> +
> +            reg = <0x0 0x0acb3000 0x0 0x1000>,
> +                  <0x0 0x0acba000 0x0 0x1000>,
> +                  <0x0 0x0acc1000 0x0 0x1000>,
> +                  <0x0 0x0acc8000 0x0 0x1000>,
> +                  <0x0 0x0ac65000 0x0 0x1000>,
> +                  <0x0 0x0ac66000 0x0 0x1000>,
> +                  <0x0 0x0ac67000 0x0 0x1000>,
> +                  <0x0 0x0ac68000 0x0 0x1000>,
> +                  <0x0 0x0acaf000 0x0 0x4000>,
> +                  <0x0 0x0acb6000 0x0 0x4000>,
> +                  <0x0 0x0acbd000 0x0 0x4000>,
> +                  <0x0 0x0acc4000 0x0 0x4000>;
> +            reg-names = "csid0",
> +                        "csid1",
> +                        "csid2",
> +                        "csid_lite",
> +                        "csiphy0",
> +                        "csiphy1",
> +                        "csiphy2",
> +                        "csiphy3",
> +                        "vfe0",
> +                        "vfe1",
> +                        "vfe2",
> +                        "vfe_lite";
> +
> +            clocks = <&gcc GCC_CAMERA_AHB_CLK>,

I believe this clock is critical, and it is set so in the SM6350 GCC driver,
therefore it should not be added here.

Multiple CAMCC drivers define some of the clocks as "critical" and always
enabled, a misconfiguration in this area may cause the reported warning.

> +                     <&gcc GCC_CAMERA_AXI_CLK>,
> +                     <&camcc CAMCC_SOC_AHB_CLK>,
> +                     <&camcc CAMCC_CAMNOC_AXI_CLK>,
> +                     <&camcc CAMCC_CORE_AHB_CLK>,
> +                     <&camcc CAMCC_CPAS_AHB_CLK>,
> +                     <&camcc CAMCC_CSIPHY0_CLK>,
> +                     <&camcc CAMCC_CSI0PHYTIMER_CLK>,
> +                     <&camcc CAMCC_CSIPHY1_CLK>,
> +                     <&camcc CAMCC_CSI1PHYTIMER_CLK>,
> +                     <&camcc CAMCC_CSIPHY2_CLK>,
> +                     <&camcc CAMCC_CSI2PHYTIMER_CLK>,
> +                     <&camcc CAMCC_CSIPHY3_CLK>,
> +                     <&camcc CAMCC_CSI3PHYTIMER_CLK>,
> +                     <&camcc CAMCC_SLOW_AHB_CLK_SRC>,
> +                     <&camcc CAMCC_IFE_0_AXI_CLK>,
> +                     <&camcc CAMCC_IFE_0_CLK>,
> +                     <&camcc CAMCC_IFE_0_CPHY_RX_CLK>,
> +                     <&camcc CAMCC_IFE_0_CSID_CLK>,
> +                     <&camcc CAMCC_IFE_1_AXI_CLK>,
> +                     <&camcc CAMCC_IFE_1_CLK>,
> +                     <&camcc CAMCC_IFE_1_CPHY_RX_CLK>,
> +                     <&camcc CAMCC_IFE_1_CSID_CLK>,
> +                     <&camcc CAMCC_IFE_2_AXI_CLK>,
> +                     <&camcc CAMCC_IFE_2_CLK>,
> +                     <&camcc CAMCC_IFE_2_CPHY_RX_CLK>,
> +                     <&camcc CAMCC_IFE_2_CSID_CLK>,
> +                     <&camcc CAMCC_IFE_LITE_CLK>,
> +                     <&camcc CAMCC_IFE_LITE_CPHY_RX_CLK>,
> +                     <&camcc CAMCC_IFE_LITE_CSID_CLK>;
> +            clock-names = "cam_ahb_clk",
> +                          "cam_axi",
> +                          "soc_ahb",
> +                          "camnoc_axi",
> +                          "core_ahb",
> +                          "cpas_ahb",
> +                          "csiphy0",
> +                          "csiphy0_timer",
> +                          "csiphy1",
> +                          "csiphy1_timer",
> +                          "csiphy2",
> +                          "csiphy2_timer",
> +                          "csiphy3",
> +                          "csiphy3_timer",
> +                          "slow_ahb_src",
> +                          "vfe0_axi",
> +                          "vfe0",
> +                          "vfe0_cphy_rx",
> +                          "vfe0_csid",
> +                          "vfe1_axi",
> +                          "vfe1",
> +                          "vfe1_cphy_rx",
> +                          "vfe1_csid",
> +                          "vfe2_axi",
> +                          "vfe2",
> +                          "vfe2_cphy_rx",
> +                          "vfe2_csid",
> +                          "vfe_lite",
> +                          "vfe_lite_cphy_rx",
> +                          "vfe_lite_csid";
> +
> +            interrupts = <GIC_SPI 464 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 466 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 717 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 473 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 477 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 478 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 479 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 461 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 465 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 467 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 718 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 472 IRQ_TYPE_LEVEL_HIGH>;

Interrupt types shall be IRQ_TYPE_EDGE_RISING.

> +            interrupt-names = "csid0",
> +                              "csid1",
> +                              "csid2",
> +                              "csid_lite",
> +                              "csiphy0",
> +                              "csiphy1",
> +                              "csiphy2",
> +                              "csiphy3",
> +                              "vfe0",
> +                              "vfe1",
> +                              "vfe2",
> +                              "vfe_lite";
> +
> +            interconnects = <&gem_noc MASTER_AMPSS_M0 QCOM_ICC_TAG_ACTIVE_ONLY
> +                             &config_noc SLAVE_CAMERA_CFG QCOM_ICC_TAG_ACTIVE_ONLY>,
> +                            <&mmss_noc MASTER_CAMNOC_HF QCOM_ICC_TAG_ALWAYS
> +                             &clk_virt SLAVE_EBI_CH0 QCOM_ICC_TAG_ALWAYS>,
> +                            <&mmss_noc MASTER_CAMNOC_SF QCOM_ICC_TAG_ALWAYS
> +                             &clk_virt SLAVE_EBI_CH0 QCOM_ICC_TAG_ALWAYS>,
> +                            <&mmss_noc MASTER_CAMNOC_ICP QCOM_ICC_TAG_ALWAYS
> +                             &clk_virt SLAVE_EBI_CH0 QCOM_ICC_TAG_ALWAYS>;
> +            interconnect-names = "ahb",
> +                                 "hf_mnoc",
> +                                 "sf_mnoc",
> +                                 "sf_icp_mnoc";
> +
> +            iommus = <&apps_smmu 0x820 0xc0>,
> +                     <&apps_smmu 0x840 0x0>,
> +                     <&apps_smmu 0x860 0xc0>,
> +                     <&apps_smmu 0x880 0x0>;
> +
> +            power-domains = <&camcc TITAN_TOP_GDSC>

It should be the last one in the list, if the settled practice is followed.

> +                            <&camcc IFE_0_GDSC>,
> +                            <&camcc IFE_1_GDSC>,
> +                            <&camcc IFE_2_GDSC>;
> +            power-domain-names = "top",
> +                                 "ife0",
> +                                 "ife1",
> +                                 "ife2";
> +
> +            vdd-csiphy-0p9-supply = <&vreg_l18a>;
> +            vdd-csiphy-1p25-supply = <&vreg_l22a>;
> +
> +            ports {
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +
> +                port@0 {
> +                    reg = <0>;
> +                    csiphy0_ep: endpoint {

An empty line before a child node is always needed.

> +                        data-lanes = <0 1 2 3>;
> +                        bus-type = <MEDIA_BUS_TYPE_CSI2_DPHY>;
> +                        remote-endpoint = <&sensor_ep>;
> +                    };
> +                };
> +            };
> +        };
> +    };
> 

-- 
Best wishes,
Vladimir
Re: [PATCH v2 1/3] dt-bindings: media: camss: Add qcom,sm6350-camss
Posted by Luca Weiss 2 months, 3 weeks ago
Hi Vladimir,

On Fri Nov 14, 2025 at 1:40 PM CET, Vladimir Zapolskiy wrote:
> Hi Luca.
>
> On 11/14/25 13:15, Luca Weiss wrote:
>> Add bindings for the Camera Subsystem on the SM6350 SoC.
>> 
>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>> ---
>>   .../bindings/media/qcom,sm6350-camss.yaml          | 349 +++++++++++++++++++++
>>   1 file changed, 349 insertions(+)
>> 
>> diff --git a/Documentation/devicetree/bindings/media/qcom,sm6350-camss.yaml b/Documentation/devicetree/bindings/media/qcom,sm6350-camss.yaml
>> new file mode 100644
>> index 000000000000..d812b5b50c05
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/qcom,sm6350-camss.yaml
>> @@ -0,0 +1,349 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/media/qcom,sm6350-camss.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm SM6350 Camera Subsystem (CAMSS)
>> +
>> +maintainers:
>> +  - Luca Weiss <luca.weiss@fairphone.com>
>> +
>> +description:
>> +  The CAMSS IP is a CSI decoder and ISP present on Qualcomm platforms.
>> +
>> +properties:
>> +  compatible:
>> +    const: qcom,sm6350-camss
>> +
>> +  reg:
>> +    maxItems: 12
>> +
>> +  reg-names:
>> +    items:
>> +      - const: csid0
>> +      - const: csid1
>> +      - const: csid2
>> +      - const: csid_lite
>> +      - const: csiphy0
>> +      - const: csiphy1
>> +      - const: csiphy2
>> +      - const: csiphy3
>> +      - const: vfe0
>> +      - const: vfe1
>> +      - const: vfe2
>> +      - const: vfe_lite
>> +
>> +  clocks:
>> +    maxItems: 30
>> +
>> +  clock-names:
>> +    items:
>> +      - const: cam_ahb_clk
>> +      - const: cam_axi
>> +      - const: soc_ahb
>> +      - const: camnoc_axi
>> +      - const: core_ahb
>> +      - const: cpas_ahb
>> +      - const: csiphy0
>> +      - const: csiphy0_timer
>> +      - const: csiphy1
>> +      - const: csiphy1_timer
>> +      - const: csiphy2
>> +      - const: csiphy2_timer
>> +      - const: csiphy3
>> +      - const: csiphy3_timer
>> +      - const: slow_ahb_src
>> +      - const: vfe0_axi
>> +      - const: vfe0
>> +      - const: vfe0_cphy_rx
>> +      - const: vfe0_csid
>> +      - const: vfe1_axi
>> +      - const: vfe1
>> +      - const: vfe1_cphy_rx
>> +      - const: vfe1_csid
>> +      - const: vfe2_axi
>> +      - const: vfe2
>> +      - const: vfe2_cphy_rx
>> +      - const: vfe2_csid
>> +      - const: vfe_lite
>> +      - const: vfe_lite_cphy_rx
>> +      - const: vfe_lite_csid
>
> The sorting order of this list does not follow the sorting order accepted
> in the past.

What file should I best reference?

>
> I'm very sorry for the vagueness, but I can not pronounce the accepted
> sorting order name, because it triggers people.
>
>> +
>> +  interrupts:
>> +    maxItems: 12
>> +
>> +  interrupt-names:
>> +    items:
>> +      - const: csid0
>> +      - const: csid1
>> +      - const: csid2
>> +      - const: csid_lite
>> +      - const: csiphy0
>> +      - const: csiphy1
>> +      - const: csiphy2
>> +      - const: csiphy3
>> +      - const: vfe0
>> +      - const: vfe1
>> +      - const: vfe2
>> +      - const: vfe_lite
>> +
>> +  interconnects:
>> +    maxItems: 4
>> +
>> +  interconnect-names:
>> +    items:
>> +      - const: ahb
>> +      - const: hf_mnoc
>> +      - const: sf_mnoc
>> +      - const: sf_icp_mnoc
>
> Please remove sf_mnoc and sf_icp_mnoc, they are not needed for enabling
> IP to produce raw images, and one day you may use them somewhere else.

Ack, will give it a try.

>
>> +
>> +  iommus:
>> +    maxItems: 4
>> +
>> +  power-domains:
>> +    items:
>> +      - description: IFE0 GDSC - Image Front End, Global Distributed Switch Controller.
>> +      - description: IFE1 GDSC - Image Front End, Global Distributed Switch Controller.
>> +      - description: IFE2 GDSC - Image Front End, Global Distributed Switch Controller.
>> +      - description: Titan Top GDSC - Titan ISP Block, Global Distributed Switch Controller.
>> +
>> +  power-domain-names:
>> +    items:
>> +      - const: top
>> +      - const: ife0
>> +      - const: ife1
>> +      - const: ife2
>
> Note that the list of items and the list of the item descriptions do not
> correspond to each other. Titan Top GDSC shall be at the end.

In the v1 the comment was that top can now be put on top (because a
limitation in the driver was fixed). But yes, forgot to modify
power-domains description. Will fix.

>
>> +
>> +  vdd-csiphy-0p9-supply:
>> +    description:
>> +      Phandle to a 0.9V regulator supply to a PHY.
>> +
>> +  vdd-csiphy-1p25-supply:
>> +    description:
>> +      Phandle to a 1.25V regulator supply to a PHY.
>> +
>
> Please reference to the schematics or SoC TRM, does SM6350 SoC
> have different pads to get supplies to different CSIPHYx IPs?
>
> If so, then please provide hardware properties to get a proper
> correspondence between supplies and CSIPHYx, and make all these
> properties optional.

I shared the names in replies to v1.

* VDD_CAMSS_PLL_0P9 - Camera SS PLL 0.9 V circuits
    (not referenced in downstream kernel, connected to vreg_s5a in
    schematics, which is MX)
* VDD_A_CSI_x_0P9 - MIPI CSIx 0.9 V circuits
    With pad names VDD_A_CSI_0_0P9 to VDD_A_CSI_3_0P9
* VDD_A_CSI_x_1P25 - MIPI CSIx 1.25 V circuits
    With pad names VDD_A_CSI_0_1P25 to VDD_A_CSI_3_1P25

>
>> +  ports:
>> +    $ref: /schemas/graph.yaml#/properties/ports
>> +
>> +    description:
>> +      CSI input ports.
>> +
>> +    patternProperties:
>> +      "^port@[0-3]$":
>> +        $ref: /schemas/graph.yaml#/$defs/port-base
>> +        unevaluatedProperties: false
>> +
>> +        description:
>> +          Input port for receiving CSI data from a CSIPHY.
>> +
>> +        properties:
>> +          endpoint:
>> +            $ref: video-interfaces.yaml#
>> +            unevaluatedProperties: false
>> +
>> +            properties:
>> +              data-lanes:
>> +                minItems: 1
>> +                maxItems: 4
>> +
>> +              bus-type:
>> +                enum:
>> +                  - 1 # MEDIA_BUS_TYPE_CSI2_CPHY
>> +                  - 4 # MEDIA_BUS_TYPE_CSI2_DPHY
>> +
>> +            required:
>> +              - data-lanes
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - reg-names
>> +  - clocks
>> +  - clock-names
>> +  - interrupts
>> +  - interrupt-names
>> +  - interconnects
>> +  - interconnect-names
>> +  - iommus
>> +  - power-domains
>> +  - power-domain-names
>> +  - vdd-csiphy-0p9-supply
>> +  - vdd-csiphy-1p25-supply
>
> When a change to add CSIPHYx specific supplies is done, please remove
> *-supply properties from the list of the requred ones.

Is this pending some other change that will be posted? Or what do you mean?

>
>> +  - ports
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/qcom,gcc-sm6350.h>
>> +    #include <dt-bindings/clock/qcom,sm6350-camcc.h>
>> +    #include <dt-bindings/interconnect/qcom,icc.h>
>> +    #include <dt-bindings/interconnect/qcom,sm6350.h>
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    #include <dt-bindings/media/video-interfaces.h>
>> +    #include <dt-bindings/power/qcom-rpmpd.h>
>> +
>> +    soc {
>> +        #address-cells = <2>;
>> +        #size-cells = <2>;
>> +
>> +        isp@acb3000 {
>> +            compatible = "qcom,sm6350-camss";
>> +
>> +            reg = <0x0 0x0acb3000 0x0 0x1000>,
>> +                  <0x0 0x0acba000 0x0 0x1000>,
>> +                  <0x0 0x0acc1000 0x0 0x1000>,
>> +                  <0x0 0x0acc8000 0x0 0x1000>,
>> +                  <0x0 0x0ac65000 0x0 0x1000>,
>> +                  <0x0 0x0ac66000 0x0 0x1000>,
>> +                  <0x0 0x0ac67000 0x0 0x1000>,
>> +                  <0x0 0x0ac68000 0x0 0x1000>,
>> +                  <0x0 0x0acaf000 0x0 0x4000>,
>> +                  <0x0 0x0acb6000 0x0 0x4000>,
>> +                  <0x0 0x0acbd000 0x0 0x4000>,
>> +                  <0x0 0x0acc4000 0x0 0x4000>;
>> +            reg-names = "csid0",
>> +                        "csid1",
>> +                        "csid2",
>> +                        "csid_lite",
>> +                        "csiphy0",
>> +                        "csiphy1",
>> +                        "csiphy2",
>> +                        "csiphy3",
>> +                        "vfe0",
>> +                        "vfe1",
>> +                        "vfe2",
>> +                        "vfe_lite";
>> +
>> +            clocks = <&gcc GCC_CAMERA_AHB_CLK>,
>
> I believe this clock is critical, and it is set so in the SM6350 GCC driver,
> therefore it should not be added here.

True, gcc_camera_ahb_clk has CLK_IS_CRITICAL in gcc-sm6350.c

>
> Multiple CAMCC drivers define some of the clocks as "critical" and always
> enabled, a misconfiguration in this area may cause the reported warning.

Will try to remove it then.

>
>> +                     <&gcc GCC_CAMERA_AXI_CLK>,
>> +                     <&camcc CAMCC_SOC_AHB_CLK>,
>> +                     <&camcc CAMCC_CAMNOC_AXI_CLK>,
>> +                     <&camcc CAMCC_CORE_AHB_CLK>,
>> +                     <&camcc CAMCC_CPAS_AHB_CLK>,
>> +                     <&camcc CAMCC_CSIPHY0_CLK>,
>> +                     <&camcc CAMCC_CSI0PHYTIMER_CLK>,
>> +                     <&camcc CAMCC_CSIPHY1_CLK>,
>> +                     <&camcc CAMCC_CSI1PHYTIMER_CLK>,
>> +                     <&camcc CAMCC_CSIPHY2_CLK>,
>> +                     <&camcc CAMCC_CSI2PHYTIMER_CLK>,
>> +                     <&camcc CAMCC_CSIPHY3_CLK>,
>> +                     <&camcc CAMCC_CSI3PHYTIMER_CLK>,
>> +                     <&camcc CAMCC_SLOW_AHB_CLK_SRC>,
>> +                     <&camcc CAMCC_IFE_0_AXI_CLK>,
>> +                     <&camcc CAMCC_IFE_0_CLK>,
>> +                     <&camcc CAMCC_IFE_0_CPHY_RX_CLK>,
>> +                     <&camcc CAMCC_IFE_0_CSID_CLK>,
>> +                     <&camcc CAMCC_IFE_1_AXI_CLK>,
>> +                     <&camcc CAMCC_IFE_1_CLK>,
>> +                     <&camcc CAMCC_IFE_1_CPHY_RX_CLK>,
>> +                     <&camcc CAMCC_IFE_1_CSID_CLK>,
>> +                     <&camcc CAMCC_IFE_2_AXI_CLK>,
>> +                     <&camcc CAMCC_IFE_2_CLK>,
>> +                     <&camcc CAMCC_IFE_2_CPHY_RX_CLK>,
>> +                     <&camcc CAMCC_IFE_2_CSID_CLK>,
>> +                     <&camcc CAMCC_IFE_LITE_CLK>,
>> +                     <&camcc CAMCC_IFE_LITE_CPHY_RX_CLK>,
>> +                     <&camcc CAMCC_IFE_LITE_CSID_CLK>;
>> +            clock-names = "cam_ahb_clk",
>> +                          "cam_axi",
>> +                          "soc_ahb",
>> +                          "camnoc_axi",
>> +                          "core_ahb",
>> +                          "cpas_ahb",
>> +                          "csiphy0",
>> +                          "csiphy0_timer",
>> +                          "csiphy1",
>> +                          "csiphy1_timer",
>> +                          "csiphy2",
>> +                          "csiphy2_timer",
>> +                          "csiphy3",
>> +                          "csiphy3_timer",
>> +                          "slow_ahb_src",
>> +                          "vfe0_axi",
>> +                          "vfe0",
>> +                          "vfe0_cphy_rx",
>> +                          "vfe0_csid",
>> +                          "vfe1_axi",
>> +                          "vfe1",
>> +                          "vfe1_cphy_rx",
>> +                          "vfe1_csid",
>> +                          "vfe2_axi",
>> +                          "vfe2",
>> +                          "vfe2_cphy_rx",
>> +                          "vfe2_csid",
>> +                          "vfe_lite",
>> +                          "vfe_lite_cphy_rx",
>> +                          "vfe_lite_csid";
>> +
>> +            interrupts = <GIC_SPI 464 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 466 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 717 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 473 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 477 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 478 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 479 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 461 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 465 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 467 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 718 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 472 IRQ_TYPE_LEVEL_HIGH>;
>
> Interrupt types shall be IRQ_TYPE_EDGE_RISING.

Ack

>
>> +            interrupt-names = "csid0",
>> +                              "csid1",
>> +                              "csid2",
>> +                              "csid_lite",
>> +                              "csiphy0",
>> +                              "csiphy1",
>> +                              "csiphy2",
>> +                              "csiphy3",
>> +                              "vfe0",
>> +                              "vfe1",
>> +                              "vfe2",
>> +                              "vfe_lite";
>> +
>> +            interconnects = <&gem_noc MASTER_AMPSS_M0 QCOM_ICC_TAG_ACTIVE_ONLY
>> +                             &config_noc SLAVE_CAMERA_CFG QCOM_ICC_TAG_ACTIVE_ONLY>,
>> +                            <&mmss_noc MASTER_CAMNOC_HF QCOM_ICC_TAG_ALWAYS
>> +                             &clk_virt SLAVE_EBI_CH0 QCOM_ICC_TAG_ALWAYS>,
>> +                            <&mmss_noc MASTER_CAMNOC_SF QCOM_ICC_TAG_ALWAYS
>> +                             &clk_virt SLAVE_EBI_CH0 QCOM_ICC_TAG_ALWAYS>,
>> +                            <&mmss_noc MASTER_CAMNOC_ICP QCOM_ICC_TAG_ALWAYS
>> +                             &clk_virt SLAVE_EBI_CH0 QCOM_ICC_TAG_ALWAYS>;
>> +            interconnect-names = "ahb",
>> +                                 "hf_mnoc",
>> +                                 "sf_mnoc",
>> +                                 "sf_icp_mnoc";
>> +
>> +            iommus = <&apps_smmu 0x820 0xc0>,
>> +                     <&apps_smmu 0x840 0x0>,
>> +                     <&apps_smmu 0x860 0xc0>,
>> +                     <&apps_smmu 0x880 0x0>;
>> +
>> +            power-domains = <&camcc TITAN_TOP_GDSC>
>
> It should be the last one in the list, if the settled practice is followed.

See above.

>
>> +                            <&camcc IFE_0_GDSC>,
>> +                            <&camcc IFE_1_GDSC>,
>> +                            <&camcc IFE_2_GDSC>;
>> +            power-domain-names = "top",
>> +                                 "ife0",
>> +                                 "ife1",
>> +                                 "ife2";
>> +
>> +            vdd-csiphy-0p9-supply = <&vreg_l18a>;
>> +            vdd-csiphy-1p25-supply = <&vreg_l22a>;
>> +
>> +            ports {
>> +                #address-cells = <1>;
>> +                #size-cells = <0>;
>> +
>> +                port@0 {
>> +                    reg = <0>;
>> +                    csiphy0_ep: endpoint {
>
> An empty line before a child node is always needed.

Ack

>
>> +                        data-lanes = <0 1 2 3>;
>> +                        bus-type = <MEDIA_BUS_TYPE_CSI2_DPHY>;
>> +                        remote-endpoint = <&sensor_ep>;
>> +                    };
>> +                };
>> +            };
>> +        };
>> +    };
>> 
Re: [PATCH v2 1/3] dt-bindings: media: camss: Add qcom,sm6350-camss
Posted by Bryan O'Donoghue 2 months, 3 weeks ago
On 14/11/2025 13:06, Luca Weiss wrote:
> Hi Vladimir,
> 
> On Fri Nov 14, 2025 at 1:40 PM CET, Vladimir Zapolskiy wrote:
>> Hi Luca.
>>
>> On 11/14/25 13:15, Luca Weiss wrote:
>>> Add bindings for the Camera Subsystem on the SM6350 SoC.
>>>
>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>> ---
>>>    .../bindings/media/qcom,sm6350-camss.yaml          | 349 +++++++++++++++++++++
>>>    1 file changed, 349 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/qcom,sm6350-camss.yaml b/Documentation/devicetree/bindings/media/qcom,sm6350-camss.yaml
>>> new file mode 100644
>>> index 000000000000..d812b5b50c05
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/media/qcom,sm6350-camss.yaml
>>> @@ -0,0 +1,349 @@
>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/media/qcom,sm6350-camss.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Qualcomm SM6350 Camera Subsystem (CAMSS)
>>> +
>>> +maintainers:
>>> +  - Luca Weiss <luca.weiss@fairphone.com>
>>> +
>>> +description:
>>> +  The CAMSS IP is a CSI decoder and ISP present on Qualcomm platforms.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: qcom,sm6350-camss
>>> +
>>> +  reg:
>>> +    maxItems: 12
>>> +
>>> +  reg-names:
>>> +    items:
>>> +      - const: csid0
>>> +      - const: csid1
>>> +      - const: csid2
>>> +      - const: csid_lite
>>> +      - const: csiphy0
>>> +      - const: csiphy1
>>> +      - const: csiphy2
>>> +      - const: csiphy3
>>> +      - const: vfe0
>>> +      - const: vfe1
>>> +      - const: vfe2
>>> +      - const: vfe_lite
>>> +
>>> +  clocks:
>>> +    maxItems: 30
>>> +
>>> +  clock-names:
>>> +    items:
>>> +      - const: cam_ahb_clk
>>> +      - const: cam_axi
>>> +      - const: soc_ahb
>>> +      - const: camnoc_axi
>>> +      - const: core_ahb
>>> +      - const: cpas_ahb
>>> +      - const: csiphy0
>>> +      - const: csiphy0_timer
>>> +      - const: csiphy1
>>> +      - const: csiphy1_timer
>>> +      - const: csiphy2
>>> +      - const: csiphy2_timer
>>> +      - const: csiphy3
>>> +      - const: csiphy3_timer
>>> +      - const: slow_ahb_src
>>> +      - const: vfe0_axi
>>> +      - const: vfe0
>>> +      - const: vfe0_cphy_rx
>>> +      - const: vfe0_csid
>>> +      - const: vfe1_axi
>>> +      - const: vfe1
>>> +      - const: vfe1_cphy_rx
>>> +      - const: vfe1_csid
>>> +      - const: vfe2_axi
>>> +      - const: vfe2
>>> +      - const: vfe2_cphy_rx
>>> +      - const: vfe2_csid
>>> +      - const: vfe_lite
>>> +      - const: vfe_lite_cphy_rx
>>> +      - const: vfe_lite_csid
>>
>> The sorting order of this list does not follow the sorting order accepted
>> in the past.
> 
> What file should I best reference?

Documentation/devicetree/bindings/media/qcom,sdm845-camss.yaml

>>
>> I'm very sorry for the vagueness, but I can not pronounce the accepted
>> sorting order name, because it triggers people.
>>
>>> +
>>> +  interrupts:
>>> +    maxItems: 12
>>> +
>>> +  interrupt-names:
>>> +    items:
>>> +      - const: csid0
>>> +      - const: csid1
>>> +      - const: csid2
>>> +      - const: csid_lite
>>> +      - const: csiphy0
>>> +      - const: csiphy1
>>> +      - const: csiphy2
>>> +      - const: csiphy3
>>> +      - const: vfe0
>>> +      - const: vfe1
>>> +      - const: vfe2
>>> +      - const: vfe_lite
>>> +
>>> +  interconnects:
>>> +    maxItems: 4
>>> +
>>> +  interconnect-names:
>>> +    items:
>>> +      - const: ahb
>>> +      - const: hf_mnoc
>>> +      - const: sf_mnoc
>>> +      - const: sf_icp_mnoc
>>
>> Please remove sf_mnoc and sf_icp_mnoc, they are not needed for enabling
>> IP to produce raw images, and one day you may use them somewhere else.
> 
> Ack, will give it a try.

Disagree with this.

See the Kanaapali patches. I'm asking new submissions to be as complete 
as possible, instead of limiting the hardware description to the RDI.

So listing the ICP noc is the right thing to do.

So please include register banks for

- bps
- cdm
- icp
- ipe
- jpeg
- lrme

>>
>>> +
>>> +  iommus:
>>> +    maxItems: 4
>>> +
>>> +  power-domains:
>>> +    items:
>>> +      - description: IFE0 GDSC - Image Front End, Global Distributed Switch Controller.
>>> +      - description: IFE1 GDSC - Image Front End, Global Distributed Switch Controller.
>>> +      - description: IFE2 GDSC - Image Front End, Global Distributed Switch Controller.
>>> +      - description: Titan Top GDSC - Titan ISP Block, Global Distributed Switch Controller.
>>> +
>>> +  power-domain-names:
>>> +    items:
>>> +      - const: top
>>> +      - const: ife0
>>> +      - const: ife1
>>> +      - const: ife2
>>
>> Note that the list of items and the list of the item descriptions do not
>> correspond to each other. Titan Top GDSC shall be at the end.
> 
> In the v1 the comment was that top can now be put on top (because a
> limitation in the driver was fixed). But yes, forgot to modify
> power-domains description. Will fix.
> 
>>
>>> +
>>> +  vdd-csiphy-0p9-supply:
>>> +    description:
>>> +      Phandle to a 0.9V regulator supply to a PHY.
>>> +
>>> +  vdd-csiphy-1p25-supply:
>>> +    description:
>>> +      Phandle to a 1.25V regulator supply to a PHY.
>>> +
>>
>> Please reference to the schematics or SoC TRM, does SM6350 SoC
>> have different pads to get supplies to different CSIPHYx IPs?
>>
>> If so, then please provide hardware properties to get a proper
>> correspondence between supplies and CSIPHYx, and make all these
>> properties optional.
> 
> I shared the names in replies to v1.
> 
> * VDD_CAMSS_PLL_0P9 - Camera SS PLL 0.9 V circuits
>      (not referenced in downstream kernel, connected to vreg_s5a in
>      schematics, which is MX)
> * VDD_A_CSI_x_0P9 - MIPI CSIx 0.9 V circuits
>      With pad names VDD_A_CSI_0_0P9 to VDD_A_CSI_3_0P9
> * VDD_A_CSI_x_1P25 - MIPI CSIx 1.25 V circuits
>      With pad names VDD_A_CSI_0_1P25 to VDD_A_CSI_3_1P25

I'm fine with your proposed rail names, they appear to correspond to the 
voltage values in the docs.

> 
>>
>>> +  ports:
>>> +    $ref: /schemas/graph.yaml#/properties/ports
>>> +
>>> +    description:
>>> +      CSI input ports.
>>> +
>>> +    patternProperties:
>>> +      "^port@[0-3]$":
>>> +        $ref: /schemas/graph.yaml#/$defs/port-base
>>> +        unevaluatedProperties: false
>>> +
>>> +        description:
>>> +          Input port for receiving CSI data from a CSIPHY.
>>> +
>>> +        properties:
>>> +          endpoint:
>>> +            $ref: video-interfaces.yaml#
>>> +            unevaluatedProperties: false
>>> +
>>> +            properties:
>>> +              data-lanes:
>>> +                minItems: 1
>>> +                maxItems: 4
>>> +
>>> +              bus-type:
>>> +                enum:
>>> +                  - 1 # MEDIA_BUS_TYPE_CSI2_CPHY
>>> +                  - 4 # MEDIA_BUS_TYPE_CSI2_DPHY
>>> +
>>> +            required:
>>> +              - data-lanes
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - reg-names
>>> +  - clocks
>>> +  - clock-names
>>> +  - interrupts
>>> +  - interrupt-names
>>> +  - interconnects
>>> +  - interconnect-names
>>> +  - iommus
>>> +  - power-domains
>>> +  - power-domain-names
>>> +  - vdd-csiphy-0p9-supply
>>> +  - vdd-csiphy-1p25-supply
>>
>> When a change to add CSIPHYx specific supplies is done, please remove
>> *-supply properties from the list of the requred ones.
> 
> Is this pending some other change that will be posted? Or what do you mean?

He means in the current CSIPHY dt its not possible to require these 
properties.

> 
>>
>>> +  - ports
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/clock/qcom,gcc-sm6350.h>
>>> +    #include <dt-bindings/clock/qcom,sm6350-camcc.h>
>>> +    #include <dt-bindings/interconnect/qcom,icc.h>
>>> +    #include <dt-bindings/interconnect/qcom,sm6350.h>
>>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>>> +    #include <dt-bindings/media/video-interfaces.h>
>>> +    #include <dt-bindings/power/qcom-rpmpd.h>
>>> +
>>> +    soc {
>>> +        #address-cells = <2>;
>>> +        #size-cells = <2>;
>>> +
>>> +        isp@acb3000 {
>>> +            compatible = "qcom,sm6350-camss";
>>> +
>>> +            reg = <0x0 0x0acb3000 0x0 0x1000>,
>>> +                  <0x0 0x0acba000 0x0 0x1000>,
>>> +                  <0x0 0x0acc1000 0x0 0x1000>,
>>> +                  <0x0 0x0acc8000 0x0 0x1000>,
>>> +                  <0x0 0x0ac65000 0x0 0x1000>,
>>> +                  <0x0 0x0ac66000 0x0 0x1000>,
>>> +                  <0x0 0x0ac67000 0x0 0x1000>,
>>> +                  <0x0 0x0ac68000 0x0 0x1000>,
>>> +                  <0x0 0x0acaf000 0x0 0x4000>,
>>> +                  <0x0 0x0acb6000 0x0 0x4000>,
>>> +                  <0x0 0x0acbd000 0x0 0x4000>,
>>> +                  <0x0 0x0acc4000 0x0 0x4000>;
>>> +            reg-names = "csid0",
>>> +                        "csid1",
>>> +                        "csid2",
>>> +                        "csid_lite",
>>> +                        "csiphy0",
>>> +                        "csiphy1",
>>> +                        "csiphy2",
>>> +                        "csiphy3",
>>> +                        "vfe0",
>>> +                        "vfe1",
>>> +                        "vfe2",
>>> +                        "vfe_lite";
>>> +
>>> +            clocks = <&gcc GCC_CAMERA_AHB_CLK>,
>>
>> I believe this clock is critical, and it is set so in the SM6350 GCC driver,
>> therefore it should not be added here.
> 
> True, gcc_camera_ahb_clk has CLK_IS_CRITICAL in gcc-sm6350.c

DT describes hardware, not the happenstance of Linux driver setup.

On that basis omitting <&gcc GCC_CAMERA_AHB_CLK> from the clock list is 
not correct.

Because being bornign, can I then reuse this DT in FreeBSD ? No I cannot 
because it won't describe hardware it will desscirbe Linux-DT which 
ain't the same thing.

>>
>> Multiple CAMCC drivers define some of the clocks as "critical" and always
>> enabled, a misconfiguration in this area may cause the reported warning.
> 
> Will try to remove it then.

I really object to that. DT is a hardware description. Listing the 
clocks here does no harm and is factually accurate, which again is the 
point of DT.

> 
>>
>>> +                     <&gcc GCC_CAMERA_AXI_CLK>,
>>> +                     <&camcc CAMCC_SOC_AHB_CLK>,
>>> +                     <&camcc CAMCC_CAMNOC_AXI_CLK>,
>>> +                     <&camcc CAMCC_CORE_AHB_CLK>,
>>> +                     <&camcc CAMCC_CPAS_AHB_CLK>,
>>> +                     <&camcc CAMCC_CSIPHY0_CLK>,
>>> +                     <&camcc CAMCC_CSI0PHYTIMER_CLK>,
>>> +                     <&camcc CAMCC_CSIPHY1_CLK>,
>>> +                     <&camcc CAMCC_CSI1PHYTIMER_CLK>,
>>> +                     <&camcc CAMCC_CSIPHY2_CLK>,
>>> +                     <&camcc CAMCC_CSI2PHYTIMER_CLK>,
>>> +                     <&camcc CAMCC_CSIPHY3_CLK>,
>>> +                     <&camcc CAMCC_CSI3PHYTIMER_CLK>,
>>> +                     <&camcc CAMCC_SLOW_AHB_CLK_SRC>,
>>> +                     <&camcc CAMCC_IFE_0_AXI_CLK>,
>>> +                     <&camcc CAMCC_IFE_0_CLK>,
>>> +                     <&camcc CAMCC_IFE_0_CPHY_RX_CLK>,
>>> +                     <&camcc CAMCC_IFE_0_CSID_CLK>,
>>> +                     <&camcc CAMCC_IFE_1_AXI_CLK>,
>>> +                     <&camcc CAMCC_IFE_1_CLK>,
>>> +                     <&camcc CAMCC_IFE_1_CPHY_RX_CLK>,
>>> +                     <&camcc CAMCC_IFE_1_CSID_CLK>,
>>> +                     <&camcc CAMCC_IFE_2_AXI_CLK>,
>>> +                     <&camcc CAMCC_IFE_2_CLK>,
>>> +                     <&camcc CAMCC_IFE_2_CPHY_RX_CLK>,
>>> +                     <&camcc CAMCC_IFE_2_CSID_CLK>,
>>> +                     <&camcc CAMCC_IFE_LITE_CLK>,
>>> +                     <&camcc CAMCC_IFE_LITE_CPHY_RX_CLK>,
>>> +                     <&camcc CAMCC_IFE_LITE_CSID_CLK>;
>>> +            clock-names = "cam_ahb_clk",
>>> +                          "cam_axi",
>>> +                          "soc_ahb",
>>> +                          "camnoc_axi",
>>> +                          "core_ahb",
>>> +                          "cpas_ahb",
>>> +                          "csiphy0",
>>> +                          "csiphy0_timer",
>>> +                          "csiphy1",
>>> +                          "csiphy1_timer",
>>> +                          "csiphy2",
>>> +                          "csiphy2_timer",
>>> +                          "csiphy3",
>>> +                          "csiphy3_timer",
>>> +                          "slow_ahb_src",
>>> +                          "vfe0_axi",
>>> +                          "vfe0",
>>> +                          "vfe0_cphy_rx",
>>> +                          "vfe0_csid",
>>> +                          "vfe1_axi",
>>> +                          "vfe1",
>>> +                          "vfe1_cphy_rx",
>>> +                          "vfe1_csid",
>>> +                          "vfe2_axi",
>>> +                          "vfe2",
>>> +                          "vfe2_cphy_rx",
>>> +                          "vfe2_csid",
>>> +                          "vfe_lite",
>>> +                          "vfe_lite_cphy_rx",
>>> +                          "vfe_lite_csid";
>>> +
>>> +            interrupts = <GIC_SPI 464 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI 466 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI 717 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI 473 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI 477 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI 478 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI 479 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI 461 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI 465 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI 467 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI 718 IRQ_TYPE_LEVEL_HIGH>,
>>> +                         <GIC_SPI 472 IRQ_TYPE_LEVEL_HIGH>;
>>
>> Interrupt types shall be IRQ_TYPE_EDGE_RISING.
> 
> Ack
> 
>>
>>> +            interrupt-names = "csid0",
>>> +                              "csid1",
>>> +                              "csid2",
>>> +                              "csid_lite",
>>> +                              "csiphy0",
>>> +                              "csiphy1",
>>> +                              "csiphy2",
>>> +                              "csiphy3",
>>> +                              "vfe0",
>>> +                              "vfe1",
>>> +                              "vfe2",
>>> +                              "vfe_lite";
>>> +
>>> +            interconnects = <&gem_noc MASTER_AMPSS_M0 QCOM_ICC_TAG_ACTIVE_ONLY
>>> +                             &config_noc SLAVE_CAMERA_CFG QCOM_ICC_TAG_ACTIVE_ONLY>,
>>> +                            <&mmss_noc MASTER_CAMNOC_HF QCOM_ICC_TAG_ALWAYS
>>> +                             &clk_virt SLAVE_EBI_CH0 QCOM_ICC_TAG_ALWAYS>,
>>> +                            <&mmss_noc MASTER_CAMNOC_SF QCOM_ICC_TAG_ALWAYS
>>> +                             &clk_virt SLAVE_EBI_CH0 QCOM_ICC_TAG_ALWAYS>,
>>> +                            <&mmss_noc MASTER_CAMNOC_ICP QCOM_ICC_TAG_ALWAYS
>>> +                             &clk_virt SLAVE_EBI_CH0 QCOM_ICC_TAG_ALWAYS>;
>>> +            interconnect-names = "ahb",
>>> +                                 "hf_mnoc",
>>> +                                 "sf_mnoc",
>>> +                                 "sf_icp_mnoc";
>>> +
>>> +            iommus = <&apps_smmu 0x820 0xc0>,
>>> +                     <&apps_smmu 0x840 0x0>,
>>> +                     <&apps_smmu 0x860 0xc0>,
>>> +                     <&apps_smmu 0x880 0x0>;
>>> +
>>> +            power-domains = <&camcc TITAN_TOP_GDSC>
>>
>> It should be the last one in the list, if the settled practice is followed.
> 
> See above.
> 
>>
>>> +                            <&camcc IFE_0_GDSC>,
>>> +                            <&camcc IFE_1_GDSC>,
>>> +                            <&camcc IFE_2_GDSC>;
>>> +            power-domain-names = "top",
>>> +                                 "ife0",
>>> +                                 "ife1",
>>> +                                 "ife2";
>>> +
>>> +            vdd-csiphy-0p9-supply = <&vreg_l18a>;
>>> +            vdd-csiphy-1p25-supply = <&vreg_l22a>;
>>> +
>>> +            ports {
>>> +                #address-cells = <1>;
>>> +                #size-cells = <0>;
>>> +
>>> +                port@0 {
>>> +                    reg = <0>;
>>> +                    csiphy0_ep: endpoint {
>>
>> An empty line before a child node is always needed.
> 
> Ack
> 
>>
>>> +                        data-lanes = <0 1 2 3>;
>>> +                        bus-type = <MEDIA_BUS_TYPE_CSI2_DPHY>;
>>> +                        remote-endpoint = <&sensor_ep>;
>>> +                    };
>>> +                };
>>> +            };
>>> +        };
>>> +    };
>>>
>
Re: [PATCH v2 1/3] dt-bindings: media: camss: Add qcom,sm6350-camss
Posted by Vladimir Zapolskiy 2 months, 3 weeks ago
On 11/14/25 18:09, Bryan O'Donoghue wrote:
> On 14/11/2025 13:06, Luca Weiss wrote:
>> Hi Vladimir,
>>
>> On Fri Nov 14, 2025 at 1:40 PM CET, Vladimir Zapolskiy wrote:
>>> Hi Luca.
>>>
>>> On 11/14/25 13:15, Luca Weiss wrote:
>>>> Add bindings for the Camera Subsystem on the SM6350 SoC.
>>>>
>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>>> ---
>>>>     .../bindings/media/qcom,sm6350-camss.yaml          | 349 +++++++++++++++++++++
>>>>     1 file changed, 349 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/media/qcom,sm6350-camss.yaml b/Documentation/devicetree/bindings/media/qcom,sm6350-camss.yaml
>>>> new file mode 100644
>>>> index 000000000000..d812b5b50c05
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/media/qcom,sm6350-camss.yaml
>>>> @@ -0,0 +1,349 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/media/qcom,sm6350-camss.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Qualcomm SM6350 Camera Subsystem (CAMSS)
>>>> +
>>>> +maintainers:
>>>> +  - Luca Weiss <luca.weiss@fairphone.com>
>>>> +
>>>> +description:
>>>> +  The CAMSS IP is a CSI decoder and ISP present on Qualcomm platforms.
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: qcom,sm6350-camss
>>>> +
>>>> +  reg:
>>>> +    maxItems: 12
>>>> +
>>>> +  reg-names:
>>>> +    items:
>>>> +      - const: csid0
>>>> +      - const: csid1
>>>> +      - const: csid2
>>>> +      - const: csid_lite
>>>> +      - const: csiphy0
>>>> +      - const: csiphy1
>>>> +      - const: csiphy2
>>>> +      - const: csiphy3
>>>> +      - const: vfe0
>>>> +      - const: vfe1
>>>> +      - const: vfe2
>>>> +      - const: vfe_lite
>>>> +
>>>> +  clocks:
>>>> +    maxItems: 30
>>>> +
>>>> +  clock-names:
>>>> +    items:
>>>> +      - const: cam_ahb_clk
>>>> +      - const: cam_axi
>>>> +      - const: soc_ahb
>>>> +      - const: camnoc_axi
>>>> +      - const: core_ahb
>>>> +      - const: cpas_ahb
>>>> +      - const: csiphy0
>>>> +      - const: csiphy0_timer
>>>> +      - const: csiphy1
>>>> +      - const: csiphy1_timer
>>>> +      - const: csiphy2
>>>> +      - const: csiphy2_timer
>>>> +      - const: csiphy3
>>>> +      - const: csiphy3_timer
>>>> +      - const: slow_ahb_src
>>>> +      - const: vfe0_axi
>>>> +      - const: vfe0
>>>> +      - const: vfe0_cphy_rx
>>>> +      - const: vfe0_csid
>>>> +      - const: vfe1_axi
>>>> +      - const: vfe1
>>>> +      - const: vfe1_cphy_rx
>>>> +      - const: vfe1_csid
>>>> +      - const: vfe2_axi
>>>> +      - const: vfe2
>>>> +      - const: vfe2_cphy_rx
>>>> +      - const: vfe2_csid
>>>> +      - const: vfe_lite
>>>> +      - const: vfe_lite_cphy_rx
>>>> +      - const: vfe_lite_csid
>>>
>>> The sorting order of this list does not follow the sorting order accepted
>>> in the past.
>>
>> What file should I best reference?
> 
> Documentation/devicetree/bindings/media/qcom,sdm845-camss.yaml
> 
>>>
>>> I'm very sorry for the vagueness, but I can not pronounce the accepted
>>> sorting order name, because it triggers people.
>>>
>>>> +
>>>> +  interrupts:
>>>> +    maxItems: 12
>>>> +
>>>> +  interrupt-names:
>>>> +    items:
>>>> +      - const: csid0
>>>> +      - const: csid1
>>>> +      - const: csid2
>>>> +      - const: csid_lite
>>>> +      - const: csiphy0
>>>> +      - const: csiphy1
>>>> +      - const: csiphy2
>>>> +      - const: csiphy3
>>>> +      - const: vfe0
>>>> +      - const: vfe1
>>>> +      - const: vfe2
>>>> +      - const: vfe_lite
>>>> +
>>>> +  interconnects:
>>>> +    maxItems: 4
>>>> +
>>>> +  interconnect-names:
>>>> +    items:
>>>> +      - const: ahb
>>>> +      - const: hf_mnoc
>>>> +      - const: sf_mnoc
>>>> +      - const: sf_icp_mnoc
>>>
>>> Please remove sf_mnoc and sf_icp_mnoc, they are not needed for enabling
>>> IP to produce raw images, and one day you may use them somewhere else.
>>
>> Ack, will give it a try.
> 
> Disagree with this.
> 
> See the Kanaapali patches. I'm asking new submissions to be as complete
> as possible, instead of limiting the hardware description to the RDI.
> 
> So listing the ICP noc is the right thing to do.
> 
> So please include register banks for
> 
> - bps
> - cdm
> - icp
> - ipe
> - jpeg
> - lrme

This suggests to get a DT backward compatibility lock forever, which makes it
a very bad advice, which is favourable for just the single interested party,
which offers an alternative to the upstream CAMSS.

Anybody who wants to get support of any CAMSS ISP functionality above RAW
images shall not add anything unrelated and unsupported.

The asked inclusion shall be done later on safely, if ever needed.

>>>
>>>> +
>>>> +  iommus:
>>>> +    maxItems: 4
>>>> +
>>>> +  power-domains:
>>>> +    items:
>>>> +      - description: IFE0 GDSC - Image Front End, Global Distributed Switch Controller.
>>>> +      - description: IFE1 GDSC - Image Front End, Global Distributed Switch Controller.
>>>> +      - description: IFE2 GDSC - Image Front End, Global Distributed Switch Controller.
>>>> +      - description: Titan Top GDSC - Titan ISP Block, Global Distributed Switch Controller.
>>>> +
>>>> +  power-domain-names:
>>>> +    items:
>>>> +      - const: top
>>>> +      - const: ife0
>>>> +      - const: ife1
>>>> +      - const: ife2
>>>
>>> Note that the list of items and the list of the item descriptions do not
>>> correspond to each other. Titan Top GDSC shall be at the end.
>>
>> In the v1 the comment was that top can now be put on top (because a
>> limitation in the driver was fixed). But yes, forgot to modify
>> power-domains description. Will fix.
>>
>>>
>>>> +
>>>> +  vdd-csiphy-0p9-supply:
>>>> +    description:
>>>> +      Phandle to a 0.9V regulator supply to a PHY.
>>>> +
>>>> +  vdd-csiphy-1p25-supply:
>>>> +    description:
>>>> +      Phandle to a 1.25V regulator supply to a PHY.
>>>> +
>>>
>>> Please reference to the schematics or SoC TRM, does SM6350 SoC
>>> have different pads to get supplies to different CSIPHYx IPs?
>>>
>>> If so, then please provide hardware properties to get a proper
>>> correspondence between supplies and CSIPHYx, and make all these
>>> properties optional.
>>
>> I shared the names in replies to v1.
>>
>> * VDD_CAMSS_PLL_0P9 - Camera SS PLL 0.9 V circuits
>>       (not referenced in downstream kernel, connected to vreg_s5a in
>>       schematics, which is MX)
>> * VDD_A_CSI_x_0P9 - MIPI CSIx 0.9 V circuits
>>       With pad names VDD_A_CSI_0_0P9 to VDD_A_CSI_3_0P9
>> * VDD_A_CSI_x_1P25 - MIPI CSIx 1.25 V circuits
>>       With pad names VDD_A_CSI_0_1P25 to VDD_A_CSI_3_1P25
> 

So, if to follow SM8650 CAMSS, the properties shall be:

* vdd-csiphy0-0p9-supply,
* vdd-csiphy0-1p2-supply,
* vdd-csiphy1-0p9-supply,
* vdd-csiphy1-1p2-supply,
* vdd-csiphy2-0p9-supply,
* vdd-csiphy2-1p2-supply,
* vdd-csiphy3-0p9-supply,
* vdd-csiphy3-1p2-supply,

and all of them are optional obviously.

This way you may get a future support of proper CSIPHY separation from
the already overbloated CAMSS device tree node.

> I'm fine with your proposed rail names, they appear to correspond to the
> voltage values in the docs.
> 
>>
>>>
>>>> +  ports:
>>>> +    $ref: /schemas/graph.yaml#/properties/ports
>>>> +
>>>> +    description:
>>>> +      CSI input ports.
>>>> +
>>>> +    patternProperties:
>>>> +      "^port@[0-3]$":
>>>> +        $ref: /schemas/graph.yaml#/$defs/port-base
>>>> +        unevaluatedProperties: false
>>>> +
>>>> +        description:
>>>> +          Input port for receiving CSI data from a CSIPHY.
>>>> +
>>>> +        properties:
>>>> +          endpoint:
>>>> +            $ref: video-interfaces.yaml#
>>>> +            unevaluatedProperties: false
>>>> +
>>>> +            properties:
>>>> +              data-lanes:
>>>> +                minItems: 1
>>>> +                maxItems: 4
>>>> +
>>>> +              bus-type:
>>>> +                enum:
>>>> +                  - 1 # MEDIA_BUS_TYPE_CSI2_CPHY
>>>> +                  - 4 # MEDIA_BUS_TYPE_CSI2_DPHY
>>>> +
>>>> +            required:
>>>> +              - data-lanes
>>>> +
>>>> +required:
>>>> +  - compatible
>>>> +  - reg
>>>> +  - reg-names
>>>> +  - clocks
>>>> +  - clock-names
>>>> +  - interrupts
>>>> +  - interrupt-names
>>>> +  - interconnects
>>>> +  - interconnect-names
>>>> +  - iommus
>>>> +  - power-domains
>>>> +  - power-domain-names
>>>> +  - vdd-csiphy-0p9-supply
>>>> +  - vdd-csiphy-1p25-supply
>>>
>>> When a change to add CSIPHYx specific supplies is done, please remove
>>> *-supply properties from the list of the requred ones.
>>
>> Is this pending some other change that will be posted? Or what do you mean?
> 
> He means in the current CSIPHY dt its not possible to require these
> properties.
> 
>>
>>>
>>>> +  - ports
>>>> +
>>>> +additionalProperties: false
>>>> +
>>>> +examples:
>>>> +  - |
>>>> +    #include <dt-bindings/clock/qcom,gcc-sm6350.h>
>>>> +    #include <dt-bindings/clock/qcom,sm6350-camcc.h>
>>>> +    #include <dt-bindings/interconnect/qcom,icc.h>
>>>> +    #include <dt-bindings/interconnect/qcom,sm6350.h>
>>>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>>>> +    #include <dt-bindings/media/video-interfaces.h>
>>>> +    #include <dt-bindings/power/qcom-rpmpd.h>
>>>> +
>>>> +    soc {
>>>> +        #address-cells = <2>;
>>>> +        #size-cells = <2>;
>>>> +
>>>> +        isp@acb3000 {
>>>> +            compatible = "qcom,sm6350-camss";
>>>> +
>>>> +            reg = <0x0 0x0acb3000 0x0 0x1000>,
>>>> +                  <0x0 0x0acba000 0x0 0x1000>,
>>>> +                  <0x0 0x0acc1000 0x0 0x1000>,
>>>> +                  <0x0 0x0acc8000 0x0 0x1000>,
>>>> +                  <0x0 0x0ac65000 0x0 0x1000>,
>>>> +                  <0x0 0x0ac66000 0x0 0x1000>,
>>>> +                  <0x0 0x0ac67000 0x0 0x1000>,
>>>> +                  <0x0 0x0ac68000 0x0 0x1000>,
>>>> +                  <0x0 0x0acaf000 0x0 0x4000>,
>>>> +                  <0x0 0x0acb6000 0x0 0x4000>,
>>>> +                  <0x0 0x0acbd000 0x0 0x4000>,
>>>> +                  <0x0 0x0acc4000 0x0 0x4000>;
>>>> +            reg-names = "csid0",
>>>> +                        "csid1",
>>>> +                        "csid2",
>>>> +                        "csid_lite",
>>>> +                        "csiphy0",
>>>> +                        "csiphy1",
>>>> +                        "csiphy2",
>>>> +                        "csiphy3",
>>>> +                        "vfe0",
>>>> +                        "vfe1",
>>>> +                        "vfe2",
>>>> +                        "vfe_lite";
>>>> +
>>>> +            clocks = <&gcc GCC_CAMERA_AHB_CLK>,
>>>
>>> I believe this clock is critical, and it is set so in the SM6350 GCC driver,
>>> therefore it should not be added here.
>>
>> True, gcc_camera_ahb_clk has CLK_IS_CRITICAL in gcc-sm6350.c
> 
> DT describes hardware, not the happenstance of Linux driver setup.
> 
> On that basis omitting <&gcc GCC_CAMERA_AHB_CLK> from the clock list is
> not correct.

This has been already discussed, an enumerous amount of Qualcomm/MSM
specific resourced like clocks enabled in ABL, Linux etc. are considered
critical and not described in the dtb.

> 
> Because being bornign, can I then reuse this DT in FreeBSD ? No I cannot
> because it won't describe hardware it will desscirbe Linux-DT which
> ain't the same thing.

Yes, you can reuse it in FreeBSD, because bug-free FreeBSD sets critical
resources as critical and always enabled.

>>>
>>> Multiple CAMCC drivers define some of the clocks as "critical" and always
>>> enabled, a misconfiguration in this area may cause the reported warning.
>>
>> Will try to remove it then.
> 
> I really object to that. DT is a hardware description. Listing the
> clocks here does no harm and is factually accurate, which again is the
> point of DT.

Here you should argue with linux-arm-msm maintainers, it's not something
new.

>>
>>>
>>>> +                     <&gcc GCC_CAMERA_AXI_CLK>,
>>>> +                     <&camcc CAMCC_SOC_AHB_CLK>,
>>>> +                     <&camcc CAMCC_CAMNOC_AXI_CLK>,
>>>> +                     <&camcc CAMCC_CORE_AHB_CLK>,
>>>> +                     <&camcc CAMCC_CPAS_AHB_CLK>,
>>>> +                     <&camcc CAMCC_CSIPHY0_CLK>,
>>>> +                     <&camcc CAMCC_CSI0PHYTIMER_CLK>,
>>>> +                     <&camcc CAMCC_CSIPHY1_CLK>,
>>>> +                     <&camcc CAMCC_CSI1PHYTIMER_CLK>,
>>>> +                     <&camcc CAMCC_CSIPHY2_CLK>,
>>>> +                     <&camcc CAMCC_CSI2PHYTIMER_CLK>,
>>>> +                     <&camcc CAMCC_CSIPHY3_CLK>,
>>>> +                     <&camcc CAMCC_CSI3PHYTIMER_CLK>,
>>>> +                     <&camcc CAMCC_SLOW_AHB_CLK_SRC>,
>>>> +                     <&camcc CAMCC_IFE_0_AXI_CLK>,
>>>> +                     <&camcc CAMCC_IFE_0_CLK>,
>>>> +                     <&camcc CAMCC_IFE_0_CPHY_RX_CLK>,
>>>> +                     <&camcc CAMCC_IFE_0_CSID_CLK>,
>>>> +                     <&camcc CAMCC_IFE_1_AXI_CLK>,
>>>> +                     <&camcc CAMCC_IFE_1_CLK>,
>>>> +                     <&camcc CAMCC_IFE_1_CPHY_RX_CLK>,
>>>> +                     <&camcc CAMCC_IFE_1_CSID_CLK>,
>>>> +                     <&camcc CAMCC_IFE_2_AXI_CLK>,
>>>> +                     <&camcc CAMCC_IFE_2_CLK>,
>>>> +                     <&camcc CAMCC_IFE_2_CPHY_RX_CLK>,
>>>> +                     <&camcc CAMCC_IFE_2_CSID_CLK>,
>>>> +                     <&camcc CAMCC_IFE_LITE_CLK>,
>>>> +                     <&camcc CAMCC_IFE_LITE_CPHY_RX_CLK>,
>>>> +                     <&camcc CAMCC_IFE_LITE_CSID_CLK>;
>>>> +            clock-names = "cam_ahb_clk",
>>>> +                          "cam_axi",
>>>> +                          "soc_ahb",
>>>> +                          "camnoc_axi",
>>>> +                          "core_ahb",
>>>> +                          "cpas_ahb",
>>>> +                          "csiphy0",
>>>> +                          "csiphy0_timer",
>>>> +                          "csiphy1",
>>>> +                          "csiphy1_timer",
>>>> +                          "csiphy2",
>>>> +                          "csiphy2_timer",
>>>> +                          "csiphy3",
>>>> +                          "csiphy3_timer",
>>>> +                          "slow_ahb_src",
>>>> +                          "vfe0_axi",
>>>> +                          "vfe0",
>>>> +                          "vfe0_cphy_rx",
>>>> +                          "vfe0_csid",
>>>> +                          "vfe1_axi",
>>>> +                          "vfe1",
>>>> +                          "vfe1_cphy_rx",
>>>> +                          "vfe1_csid",
>>>> +                          "vfe2_axi",
>>>> +                          "vfe2",
>>>> +                          "vfe2_cphy_rx",
>>>> +                          "vfe2_csid",
>>>> +                          "vfe_lite",
>>>> +                          "vfe_lite_cphy_rx",
>>>> +                          "vfe_lite_csid";
>>>> +
>>>> +            interrupts = <GIC_SPI 464 IRQ_TYPE_LEVEL_HIGH>,
>>>> +                         <GIC_SPI 466 IRQ_TYPE_LEVEL_HIGH>,
>>>> +                         <GIC_SPI 717 IRQ_TYPE_LEVEL_HIGH>,
>>>> +                         <GIC_SPI 473 IRQ_TYPE_LEVEL_HIGH>,
>>>> +                         <GIC_SPI 477 IRQ_TYPE_LEVEL_HIGH>,
>>>> +                         <GIC_SPI 478 IRQ_TYPE_LEVEL_HIGH>,
>>>> +                         <GIC_SPI 479 IRQ_TYPE_LEVEL_HIGH>,
>>>> +                         <GIC_SPI 461 IRQ_TYPE_LEVEL_HIGH>,
>>>> +                         <GIC_SPI 465 IRQ_TYPE_LEVEL_HIGH>,
>>>> +                         <GIC_SPI 467 IRQ_TYPE_LEVEL_HIGH>,
>>>> +                         <GIC_SPI 718 IRQ_TYPE_LEVEL_HIGH>,
>>>> +                         <GIC_SPI 472 IRQ_TYPE_LEVEL_HIGH>;
>>>
>>> Interrupt types shall be IRQ_TYPE_EDGE_RISING.
>>
>> Ack
>>
>>>
>>>> +            interrupt-names = "csid0",
>>>> +                              "csid1",
>>>> +                              "csid2",
>>>> +                              "csid_lite",
>>>> +                              "csiphy0",
>>>> +                              "csiphy1",
>>>> +                              "csiphy2",
>>>> +                              "csiphy3",
>>>> +                              "vfe0",
>>>> +                              "vfe1",
>>>> +                              "vfe2",
>>>> +                              "vfe_lite";
>>>> +
>>>> +            interconnects = <&gem_noc MASTER_AMPSS_M0 QCOM_ICC_TAG_ACTIVE_ONLY
>>>> +                             &config_noc SLAVE_CAMERA_CFG QCOM_ICC_TAG_ACTIVE_ONLY>,
>>>> +                            <&mmss_noc MASTER_CAMNOC_HF QCOM_ICC_TAG_ALWAYS
>>>> +                             &clk_virt SLAVE_EBI_CH0 QCOM_ICC_TAG_ALWAYS>,
>>>> +                            <&mmss_noc MASTER_CAMNOC_SF QCOM_ICC_TAG_ALWAYS
>>>> +                             &clk_virt SLAVE_EBI_CH0 QCOM_ICC_TAG_ALWAYS>,
>>>> +                            <&mmss_noc MASTER_CAMNOC_ICP QCOM_ICC_TAG_ALWAYS
>>>> +                             &clk_virt SLAVE_EBI_CH0 QCOM_ICC_TAG_ALWAYS>;
>>>> +            interconnect-names = "ahb",
>>>> +                                 "hf_mnoc",
>>>> +                                 "sf_mnoc",
>>>> +                                 "sf_icp_mnoc";
>>>> +
>>>> +            iommus = <&apps_smmu 0x820 0xc0>,
>>>> +                     <&apps_smmu 0x840 0x0>,
>>>> +                     <&apps_smmu 0x860 0xc0>,
>>>> +                     <&apps_smmu 0x880 0x0>;
>>>> +
>>>> +            power-domains = <&camcc TITAN_TOP_GDSC>
>>>
>>> It should be the last one in the list, if the settled practice is followed.
>>
>> See above.
>>
>>>
>>>> +                            <&camcc IFE_0_GDSC>,
>>>> +                            <&camcc IFE_1_GDSC>,
>>>> +                            <&camcc IFE_2_GDSC>;
>>>> +            power-domain-names = "top",
>>>> +                                 "ife0",
>>>> +                                 "ife1",
>>>> +                                 "ife2";
>>>> +
>>>> +            vdd-csiphy-0p9-supply = <&vreg_l18a>;
>>>> +            vdd-csiphy-1p25-supply = <&vreg_l22a>;
>>>> +
>>>> +            ports {
>>>> +                #address-cells = <1>;
>>>> +                #size-cells = <0>;
>>>> +
>>>> +                port@0 {
>>>> +                    reg = <0>;
>>>> +                    csiphy0_ep: endpoint {
>>>
>>> An empty line before a child node is always needed.
>>
>> Ack
>>
>>>
>>>> +                        data-lanes = <0 1 2 3>;
>>>> +                        bus-type = <MEDIA_BUS_TYPE_CSI2_DPHY>;
>>>> +                        remote-endpoint = <&sensor_ep>;
>>>> +                    };
>>>> +                };
>>>> +            };
>>>> +        };
>>>> +    };
>>>>
>>
> 

-- 
Best wishes,
Vladimir
Re: [PATCH v2 1/3] dt-bindings: media: camss: Add qcom,sm6350-camss
Posted by Bryan O'Donoghue 2 months, 3 weeks ago
On 14/11/2025 17:06, Vladimir Zapolskiy wrote:
> On 11/14/25 18:09, Bryan O'Donoghue wrote:
>> On 14/11/2025 13:06, Luca Weiss wrote:
>>> Hi Vladimir,
>>>
>>> On Fri Nov 14, 2025 at 1:40 PM CET, Vladimir Zapolskiy wrote:
>>>> Hi Luca.
>>>>
>>>> On 11/14/25 13:15, Luca Weiss wrote:
>>>>> Add bindings for the Camera Subsystem on the SM6350 SoC.
>>>>>
>>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>>>> ---
>>>>>     .../bindings/media/qcom,sm6350-camss.yaml          | 349 ++++++ 
>>>>> +++++++++++++++
>>>>>     1 file changed, 349 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/media/qcom,sm6350- 
>>>>> camss.yaml b/Documentation/devicetree/bindings/media/qcom,sm6350- 
>>>>> camss.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..d812b5b50c05
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/media/qcom,sm6350-camss.yaml
>>>>> @@ -0,0 +1,349 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/media/qcom,sm6350-camss.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: Qualcomm SM6350 Camera Subsystem (CAMSS)
>>>>> +
>>>>> +maintainers:
>>>>> +  - Luca Weiss <luca.weiss@fairphone.com>
>>>>> +
>>>>> +description:
>>>>> +  The CAMSS IP is a CSI decoder and ISP present on Qualcomm 
>>>>> platforms.
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    const: qcom,sm6350-camss
>>>>> +
>>>>> +  reg:
>>>>> +    maxItems: 12
>>>>> +
>>>>> +  reg-names:
>>>>> +    items:
>>>>> +      - const: csid0
>>>>> +      - const: csid1
>>>>> +      - const: csid2
>>>>> +      - const: csid_lite
>>>>> +      - const: csiphy0
>>>>> +      - const: csiphy1
>>>>> +      - const: csiphy2
>>>>> +      - const: csiphy3
>>>>> +      - const: vfe0
>>>>> +      - const: vfe1
>>>>> +      - const: vfe2
>>>>> +      - const: vfe_lite
>>>>> +
>>>>> +  clocks:
>>>>> +    maxItems: 30
>>>>> +
>>>>> +  clock-names:
>>>>> +    items:
>>>>> +      - const: cam_ahb_clk
>>>>> +      - const: cam_axi
>>>>> +      - const: soc_ahb
>>>>> +      - const: camnoc_axi
>>>>> +      - const: core_ahb
>>>>> +      - const: cpas_ahb
>>>>> +      - const: csiphy0
>>>>> +      - const: csiphy0_timer
>>>>> +      - const: csiphy1
>>>>> +      - const: csiphy1_timer
>>>>> +      - const: csiphy2
>>>>> +      - const: csiphy2_timer
>>>>> +      - const: csiphy3
>>>>> +      - const: csiphy3_timer
>>>>> +      - const: slow_ahb_src
>>>>> +      - const: vfe0_axi
>>>>> +      - const: vfe0
>>>>> +      - const: vfe0_cphy_rx
>>>>> +      - const: vfe0_csid
>>>>> +      - const: vfe1_axi
>>>>> +      - const: vfe1
>>>>> +      - const: vfe1_cphy_rx
>>>>> +      - const: vfe1_csid
>>>>> +      - const: vfe2_axi
>>>>> +      - const: vfe2
>>>>> +      - const: vfe2_cphy_rx
>>>>> +      - const: vfe2_csid
>>>>> +      - const: vfe_lite
>>>>> +      - const: vfe_lite_cphy_rx
>>>>> +      - const: vfe_lite_csid
>>>>
>>>> The sorting order of this list does not follow the sorting order 
>>>> accepted
>>>> in the past.
>>>
>>> What file should I best reference?
>>
>> Documentation/devicetree/bindings/media/qcom,sdm845-camss.yaml
>>
>>>>
>>>> I'm very sorry for the vagueness, but I can not pronounce the accepted
>>>> sorting order name, because it triggers people.
>>>>
>>>>> +
>>>>> +  interrupts:
>>>>> +    maxItems: 12
>>>>> +
>>>>> +  interrupt-names:
>>>>> +    items:
>>>>> +      - const: csid0
>>>>> +      - const: csid1
>>>>> +      - const: csid2
>>>>> +      - const: csid_lite
>>>>> +      - const: csiphy0
>>>>> +      - const: csiphy1
>>>>> +      - const: csiphy2
>>>>> +      - const: csiphy3
>>>>> +      - const: vfe0
>>>>> +      - const: vfe1
>>>>> +      - const: vfe2
>>>>> +      - const: vfe_lite
>>>>> +
>>>>> +  interconnects:
>>>>> +    maxItems: 4
>>>>> +
>>>>> +  interconnect-names:
>>>>> +    items:
>>>>> +      - const: ahb
>>>>> +      - const: hf_mnoc
>>>>> +      - const: sf_mnoc
>>>>> +      - const: sf_icp_mnoc
>>>>
>>>> Please remove sf_mnoc and sf_icp_mnoc, they are not needed for enabling
>>>> IP to produce raw images, and one day you may use them somewhere else.
>>>
>>> Ack, will give it a try.
>>
>> Disagree with this.
>>
>> See the Kanaapali patches. I'm asking new submissions to be as complete
>> as possible, instead of limiting the hardware description to the RDI.
>>
>> So listing the ICP noc is the right thing to do.
>>
>> So please include register banks for
>>
>> - bps
>> - cdm
>> - icp
>> - ipe
>> - jpeg
>> - lrme
> 
> This suggests to get a DT backward compatibility lock forever, 

I'm not entirely sure what this comment means.

The objective here is to get a full and complete DT describing camera 
hardware that can be consumed by

- CAMSS RDI
- CAMSS RDI + future goodness
- FreeBSD
- Any other DT consumer of upstream data perhaps even CamX
which
> makes it
> a very bad advice, which is favourable for just the single interested 
> party,
> which offers an alternative to the upstream CAMSS.
> 
> Anybody who wants to get support of any CAMSS ISP functionality above RAW
> images shall not add anything unrelated and unsupported.
> 
> The asked inclusion shall be done later on safely, if ever needed.

As I say the objective is to fully describe the hardware in DT, not to 
describe only the RDI path.

>>>> I believe this clock is critical, and it is set so in the SM6350 GCC 
>>>> driver,
>>>> therefore it should not be added here.
>>>
>>> True, gcc_camera_ahb_clk has CLK_IS_CRITICAL in gcc-sm6350.c
>>
>> DT describes hardware, not the happenstance of Linux driver setup.
>>
>> On that basis omitting <&gcc GCC_CAMERA_AHB_CLK> from the clock list is
>> not correct.
> 
> This has been already discussed, an enumerous amount of Qualcomm/MSM
> specific resourced like clocks enabled in ABL, Linux etc. are considered
> critical and not described in the dtb.

I still think the argument for that is tenuous wrong in fact but, I know 
you are right, this is a lost argument.

@Luca please _do_ include the full array of registers/clocks/nocs as you 
find them and yeah drop the AXI clock from that description because reasons.

https://lore.kernel.org/linux-arm-msm/20251113-add-support-for-camss-on-kaanapali-v6-1-1e6038785a8e@oss.qualcomm.com/

  ---
bod
Re: [PATCH v2 1/3] dt-bindings: media: camss: Add qcom,sm6350-camss
Posted by Vladimir Zapolskiy 1 month, 3 weeks ago
On 11/16/25 16:05, Bryan O'Donoghue wrote:
> On 14/11/2025 17:06, Vladimir Zapolskiy wrote:
>> On 11/14/25 18:09, Bryan O'Donoghue wrote:
>>> On 14/11/2025 13:06, Luca Weiss wrote:
>>>> Hi Vladimir,
>>>>
>>>> On Fri Nov 14, 2025 at 1:40 PM CET, Vladimir Zapolskiy wrote:
>>>>> Hi Luca.
>>>>>
>>>>> On 11/14/25 13:15, Luca Weiss wrote:
>>>>>> Add bindings for the Camera Subsystem on the SM6350 SoC.
>>>>>>
>>>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>>>>> ---
>>>>>>      .../bindings/media/qcom,sm6350-camss.yaml          | 349 ++++++
>>>>>> +++++++++++++++
>>>>>>      1 file changed, 349 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/media/qcom,sm6350-
>>>>>> camss.yaml b/Documentation/devicetree/bindings/media/qcom,sm6350-
>>>>>> camss.yaml
>>>>>> new file mode 100644
>>>>>> index 000000000000..d812b5b50c05
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/media/qcom,sm6350-camss.yaml
>>>>>> @@ -0,0 +1,349 @@
>>>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>>>> +%YAML 1.2
>>>>>> +---
>>>>>> +$id: http://devicetree.org/schemas/media/qcom,sm6350-camss.yaml#
>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>> +
>>>>>> +title: Qualcomm SM6350 Camera Subsystem (CAMSS)
>>>>>> +
>>>>>> +maintainers:
>>>>>> +  - Luca Weiss <luca.weiss@fairphone.com>
>>>>>> +
>>>>>> +description:
>>>>>> +  The CAMSS IP is a CSI decoder and ISP present on Qualcomm
>>>>>> platforms.
>>>>>> +
>>>>>> +properties:
>>>>>> +  compatible:
>>>>>> +    const: qcom,sm6350-camss
>>>>>> +
>>>>>> +  reg:
>>>>>> +    maxItems: 12
>>>>>> +
>>>>>> +  reg-names:
>>>>>> +    items:
>>>>>> +      - const: csid0
>>>>>> +      - const: csid1
>>>>>> +      - const: csid2
>>>>>> +      - const: csid_lite
>>>>>> +      - const: csiphy0
>>>>>> +      - const: csiphy1
>>>>>> +      - const: csiphy2
>>>>>> +      - const: csiphy3
>>>>>> +      - const: vfe0
>>>>>> +      - const: vfe1
>>>>>> +      - const: vfe2
>>>>>> +      - const: vfe_lite
>>>>>> +
>>>>>> +  clocks:
>>>>>> +    maxItems: 30
>>>>>> +
>>>>>> +  clock-names:
>>>>>> +    items:
>>>>>> +      - const: cam_ahb_clk
>>>>>> +      - const: cam_axi
>>>>>> +      - const: soc_ahb
>>>>>> +      - const: camnoc_axi
>>>>>> +      - const: core_ahb
>>>>>> +      - const: cpas_ahb
>>>>>> +      - const: csiphy0
>>>>>> +      - const: csiphy0_timer
>>>>>> +      - const: csiphy1
>>>>>> +      - const: csiphy1_timer
>>>>>> +      - const: csiphy2
>>>>>> +      - const: csiphy2_timer
>>>>>> +      - const: csiphy3
>>>>>> +      - const: csiphy3_timer
>>>>>> +      - const: slow_ahb_src
>>>>>> +      - const: vfe0_axi
>>>>>> +      - const: vfe0
>>>>>> +      - const: vfe0_cphy_rx
>>>>>> +      - const: vfe0_csid
>>>>>> +      - const: vfe1_axi
>>>>>> +      - const: vfe1
>>>>>> +      - const: vfe1_cphy_rx
>>>>>> +      - const: vfe1_csid
>>>>>> +      - const: vfe2_axi
>>>>>> +      - const: vfe2
>>>>>> +      - const: vfe2_cphy_rx
>>>>>> +      - const: vfe2_csid
>>>>>> +      - const: vfe_lite
>>>>>> +      - const: vfe_lite_cphy_rx
>>>>>> +      - const: vfe_lite_csid
>>>>>
>>>>> The sorting order of this list does not follow the sorting order
>>>>> accepted
>>>>> in the past.
>>>>
>>>> What file should I best reference?
>>>
>>> Documentation/devicetree/bindings/media/qcom,sdm845-camss.yaml
>>>
>>>>>
>>>>> I'm very sorry for the vagueness, but I can not pronounce the accepted
>>>>> sorting order name, because it triggers people.
>>>>>
>>>>>> +
>>>>>> +  interrupts:
>>>>>> +    maxItems: 12
>>>>>> +
>>>>>> +  interrupt-names:
>>>>>> +    items:
>>>>>> +      - const: csid0
>>>>>> +      - const: csid1
>>>>>> +      - const: csid2
>>>>>> +      - const: csid_lite
>>>>>> +      - const: csiphy0
>>>>>> +      - const: csiphy1
>>>>>> +      - const: csiphy2
>>>>>> +      - const: csiphy3
>>>>>> +      - const: vfe0
>>>>>> +      - const: vfe1
>>>>>> +      - const: vfe2
>>>>>> +      - const: vfe_lite
>>>>>> +
>>>>>> +  interconnects:
>>>>>> +    maxItems: 4
>>>>>> +
>>>>>> +  interconnect-names:
>>>>>> +    items:
>>>>>> +      - const: ahb
>>>>>> +      - const: hf_mnoc
>>>>>> +      - const: sf_mnoc
>>>>>> +      - const: sf_icp_mnoc
>>>>>
>>>>> Please remove sf_mnoc and sf_icp_mnoc, they are not needed for enabling
>>>>> IP to produce raw images, and one day you may use them somewhere else.
>>>>
>>>> Ack, will give it a try.
>>>
>>> Disagree with this.
>>>
>>> See the Kanaapali patches. I'm asking new submissions to be as complete
>>> as possible, instead of limiting the hardware description to the RDI.
>>>
>>> So listing the ICP noc is the right thing to do.
>>>
>>> So please include register banks for
>>>
>>> - bps
>>> - cdm
>>> - icp
>>> - ipe
>>> - jpeg
>>> - lrme
>>
>> This suggests to get a DT backward compatibility lock forever,
> 
> I'm not entirely sure what this comment means.
> 
> The objective here is to get a full and complete DT describing camera
> hardware that can be consumed by
> 
> - CAMSS RDI
> - CAMSS RDI + future goodness

Here the problem is manifested, there is no even a raw idea of "future
goodness" and how it looks like practically.

Hardware can be represented in DT in many ways, why one arbitrary chosen,
and never tested or even verified for correctness, representation is
selected over any other one?

Let's start from the beginning, why putting everything into one unstructured
pile is better than accurate splitting of various CAMSS IPs into own
device tree nodes? Downstream has different device tree nodes layout,
why is it worse, should it be expected that Qualcomm in downstream moves
to a "better" proposed layout of device tree nodes for CAMSS or only
upstream Linux shall suffer?

Somebody said that there is an idea for "future goodness" to modularize
CAMSS, did it change?

> - FreeBSD
> - Any other DT consumer of upstream data perhaps even CamX

FreeBSD or CamX are software, and they shall be excluded from consideration.

There shall be no guesses about software usage, the only concern is if
the hardware desription in the shape of device tree nodes is proper or not.

Partial verifiable description is a proper description, and adding unknowns
of "future goodness" does not serve any reasonable technical purpose, it
adds an illusion of a proper description only, and it was proven that this
illision shatters even for CAMSS, see CAMSS CSIPHY case, when a wrong DT
choice can not be undone anymore for legacy platforms. It is good to acquire
an ability to learn from the past.

Everything can be added ad-hoc while keeping the hardware description in
a proper and correct state.

-- 
Best wishes,
Vladimir
Re: [PATCH v2 1/3] dt-bindings: media: camss: Add qcom,sm6350-camss
Posted by Bryan O'Donoghue 1 month, 3 weeks ago
On 19/12/2025 16:18, Vladimir Zapolskiy wrote:
> FreeBSD or CamX are software, and they shall be excluded from consideration.
> 
> There shall be no guesses about software usage, the only concern is if
> the hardware desription in the shape of device tree nodes is proper or not.
> 
> Partial verifiable description is a proper description, and adding unknowns
> of "future goodness" does not serve any reasonable technical purpose, it
> adds an illusion of a proper description only, and it was proven that this
> illision shatters even for CAMSS, see CAMSS CSIPHY case, when a wrong DT
> choice can not be undone anymore for legacy platforms. It is good to acquire
> an ability to learn from the past.
> 
> Everything can be added ad-hoc while keeping the hardware description in
> a proper and correct state.

Are all of the VFE Lite's verified ?

Even some of them ? I think not.

We are seeing in multiple places how failure to properly articulate SIDs 
results in mess.

So full description of the hardware > partial.

BTW your statement about not being able to change DT bindings is wrong. 
You just need a good argument and to make that case successfully upstream.

---
bod
Re: [PATCH v2 1/3] dt-bindings: media: camss: Add qcom,sm6350-camss
Posted by Luca Weiss 1 month, 3 weeks ago
Hi Bryan,

On Sun Nov 16, 2025 at 3:05 PM CET, Bryan O'Donoghue wrote:
> On 14/11/2025 17:06, Vladimir Zapolskiy wrote:
>> On 11/14/25 18:09, Bryan O'Donoghue wrote:
>>> On 14/11/2025 13:06, Luca Weiss wrote:
>>>> Hi Vladimir,
>>>>
>>>> On Fri Nov 14, 2025 at 1:40 PM CET, Vladimir Zapolskiy wrote:
>>>>> Hi Luca.
>>>>>
>>>>> On 11/14/25 13:15, Luca Weiss wrote:
>>>>>> Add bindings for the Camera Subsystem on the SM6350 SoC.
>>>>>>
>>>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>>>>> ---
>>>>>>     .../bindings/media/qcom,sm6350-camss.yaml          | 349 ++++++ 
>>>>>> +++++++++++++++
>>>>>>     1 file changed, 349 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/media/qcom,sm6350- 
>>>>>> camss.yaml b/Documentation/devicetree/bindings/media/qcom,sm6350- 
>>>>>> camss.yaml
>>>>>> new file mode 100644
>>>>>> index 000000000000..d812b5b50c05
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/media/qcom,sm6350-camss.yaml
>>>>>> @@ -0,0 +1,349 @@
>>>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>>>> +%YAML 1.2
>>>>>> +---
>>>>>> +$id: http://devicetree.org/schemas/media/qcom,sm6350-camss.yaml#
>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>> +
>>>>>> +title: Qualcomm SM6350 Camera Subsystem (CAMSS)
>>>>>> +
>>>>>> +maintainers:
>>>>>> +  - Luca Weiss <luca.weiss@fairphone.com>
>>>>>> +
>>>>>> +description:
>>>>>> +  The CAMSS IP is a CSI decoder and ISP present on Qualcomm 
>>>>>> platforms.
>>>>>> +
>>>>>> +properties:
>>>>>> +  compatible:
>>>>>> +    const: qcom,sm6350-camss
>>>>>> +
>>>>>> +  reg:
>>>>>> +    maxItems: 12
>>>>>> +
>>>>>> +  reg-names:
>>>>>> +    items:
>>>>>> +      - const: csid0
>>>>>> +      - const: csid1
>>>>>> +      - const: csid2
>>>>>> +      - const: csid_lite
>>>>>> +      - const: csiphy0
>>>>>> +      - const: csiphy1
>>>>>> +      - const: csiphy2
>>>>>> +      - const: csiphy3
>>>>>> +      - const: vfe0
>>>>>> +      - const: vfe1
>>>>>> +      - const: vfe2
>>>>>> +      - const: vfe_lite
>>>>>> +
>>>>>> +  clocks:
>>>>>> +    maxItems: 30
>>>>>> +
>>>>>> +  clock-names:
>>>>>> +    items:
>>>>>> +      - const: cam_ahb_clk
>>>>>> +      - const: cam_axi
>>>>>> +      - const: soc_ahb
>>>>>> +      - const: camnoc_axi
>>>>>> +      - const: core_ahb
>>>>>> +      - const: cpas_ahb
>>>>>> +      - const: csiphy0
>>>>>> +      - const: csiphy0_timer
>>>>>> +      - const: csiphy1
>>>>>> +      - const: csiphy1_timer
>>>>>> +      - const: csiphy2
>>>>>> +      - const: csiphy2_timer
>>>>>> +      - const: csiphy3
>>>>>> +      - const: csiphy3_timer
>>>>>> +      - const: slow_ahb_src
>>>>>> +      - const: vfe0_axi
>>>>>> +      - const: vfe0
>>>>>> +      - const: vfe0_cphy_rx
>>>>>> +      - const: vfe0_csid
>>>>>> +      - const: vfe1_axi
>>>>>> +      - const: vfe1
>>>>>> +      - const: vfe1_cphy_rx
>>>>>> +      - const: vfe1_csid
>>>>>> +      - const: vfe2_axi
>>>>>> +      - const: vfe2
>>>>>> +      - const: vfe2_cphy_rx
>>>>>> +      - const: vfe2_csid
>>>>>> +      - const: vfe_lite
>>>>>> +      - const: vfe_lite_cphy_rx
>>>>>> +      - const: vfe_lite_csid
>>>>>
>>>>> The sorting order of this list does not follow the sorting order 
>>>>> accepted
>>>>> in the past.
>>>>
>>>> What file should I best reference?
>>>
>>> Documentation/devicetree/bindings/media/qcom,sdm845-camss.yaml
>>>
>>>>>
>>>>> I'm very sorry for the vagueness, but I can not pronounce the accepted
>>>>> sorting order name, because it triggers people.
>>>>>
>>>>>> +
>>>>>> +  interrupts:
>>>>>> +    maxItems: 12
>>>>>> +
>>>>>> +  interrupt-names:
>>>>>> +    items:
>>>>>> +      - const: csid0
>>>>>> +      - const: csid1
>>>>>> +      - const: csid2
>>>>>> +      - const: csid_lite
>>>>>> +      - const: csiphy0
>>>>>> +      - const: csiphy1
>>>>>> +      - const: csiphy2
>>>>>> +      - const: csiphy3
>>>>>> +      - const: vfe0
>>>>>> +      - const: vfe1
>>>>>> +      - const: vfe2
>>>>>> +      - const: vfe_lite
>>>>>> +
>>>>>> +  interconnects:
>>>>>> +    maxItems: 4
>>>>>> +
>>>>>> +  interconnect-names:
>>>>>> +    items:
>>>>>> +      - const: ahb
>>>>>> +      - const: hf_mnoc
>>>>>> +      - const: sf_mnoc
>>>>>> +      - const: sf_icp_mnoc
>>>>>
>>>>> Please remove sf_mnoc and sf_icp_mnoc, they are not needed for enabling
>>>>> IP to produce raw images, and one day you may use them somewhere else.
>>>>
>>>> Ack, will give it a try.
>>>
>>> Disagree with this.
>>>
>>> See the Kanaapali patches. I'm asking new submissions to be as complete
>>> as possible, instead of limiting the hardware description to the RDI.
>>>
>>> So listing the ICP noc is the right thing to do.
>>>
>>> So please include register banks for
>>>
>>> - bps
>>> - cdm
>>> - icp
>>> - ipe
>>> - jpeg
>>> - lrme
>> 
>> This suggests to get a DT backward compatibility lock forever, 
>
> I'm not entirely sure what this comment means.
>
> The objective here is to get a full and complete DT describing camera 
> hardware that can be consumed by
>
> - CAMSS RDI
> - CAMSS RDI + future goodness
> - FreeBSD
> - Any other DT consumer of upstream data perhaps even CamX
> which
>> makes it
>> a very bad advice, which is favourable for just the single interested 
>> party,
>> which offers an alternative to the upstream CAMSS.
>> 
>> Anybody who wants to get support of any CAMSS ISP functionality above RAW
>> images shall not add anything unrelated and unsupported.
>> 
>> The asked inclusion shall be done later on safely, if ever needed.
>
> As I say the objective is to fully describe the hardware in DT, not to 
> describe only the RDI path.

So far I've gotten this diff on top of v2, not quite sure how to
continue further...

Please advice how to turn these resources into a usable dt-binding.

I'm also a bit worried that it'll be subtly wrong due to all the
additions being impossible to test for me.

Regards
Luca


diff --git a/Documentation/devicetree/bindings/media/qcom,sm6350-camss.yaml b/Documentation/devicetree/bindings/media/qcom,sm6350-camss.yaml
index d812b5b50c05..340bf03ffe91 100644
--- a/Documentation/devicetree/bindings/media/qcom,sm6350-camss.yaml
+++ b/Documentation/devicetree/bindings/media/qcom,sm6350-camss.yaml
@@ -203,6 +202,23 @@ examples:
                   <0x0 0x0acb6000 0x0 0x4000>,
                   <0x0 0x0acbd000 0x0 0x4000>,
                   <0x0 0x0acc4000 0x0 0x4000>;
+
+                  // ICP ?? == qcom,cam-a5 ?
+               // <0x0 0x0ac00000 0x0 0x6000>, // ? a5_qgic
+               // <0x0 0x0ac10000 0x0 0x8000>, // ? a5_sierra
+               // <0x0 0x0ac18000 0x0 0x3000>; // ? a5_csr
+
+               // <0x0 0x0ac87000 0x0 0xa000> // "ipe" IPE (Image Processing Engine)
+               // <0x0 0x0ac6f000 0x0 0x8000> // "bps"? BPS
+               // <0x0 0x0ac6b000 0x0 0xa00> // "lrme"? LRME
+
+               // <0x0 0x0ac40000 0x0 0x1000>, // ? CAM CPAS: cam_cpas_top
+               // <0x0 0x0ac42000 0x0 0x4600>, // ? CAM CPAS: cam_camnoc
+               // <0x0 0x01fc0000 0x0 0x40000>; // ? CAM CPAS: core_top_csr_tcsr
+
+               // <0x0 0x0ac52000 0x0 0x4000> // "jpeg_dma" JPEG DMA & Downscaler
+               // <0x0 0x0ac4e000 0x0 0x4000> // "jpeg_enc" JPEG Encoder
+               // <0x0 0x0ac48000 0x0 0x1000> // ? CPAS CDM
             reg-names = "csid0",
                         "csid1",
                         "csid2",
@@ -216,8 +232,12 @@ examples:
                         "vfe2",
                         "vfe_lite";
 
-            clocks = <&gcc GCC_CAMERA_AHB_CLK>,
-                     <&gcc GCC_CAMERA_AXI_CLK>,
+            // CAMCC_FAST_AHB_CLK_SRC + CAMCC_ICP_CLK for qcom,cam-a5
+            // CAMCC_IPE_0_AHB_CLK + CAMCC_IPE_0_AREG_CLK + CAMCC_IPE_0_AXI_CLK + CAMCC_IPE_0_CLK for qcom,cam-ipe
+            // CAMCC_BPS_AHB_CLK + CAMCC_BPS_AREG_CLK + CAMCC_BPS_AXI_CLK + CAMCC_BPS_CLK for qcom,cam-bps
+            // CAMCC_JPEG_CLK for qcom,cam_jpeg_enc / qcom,cam_jpeg_dma
+            // CAMCC_LRME_CLK for qcom,lrme
+            clocks = <&gcc GCC_CAMERA_AXI_CLK>,
                      <&camcc CAMCC_SOC_AHB_CLK>,
                      <&camcc CAMCC_CAMNOC_AXI_CLK>,
                      <&camcc CAMCC_CORE_AHB_CLK>,
@@ -246,8 +266,7 @@ examples:
                      <&camcc CAMCC_IFE_LITE_CLK>,
                      <&camcc CAMCC_IFE_LITE_CPHY_RX_CLK>,
                      <&camcc CAMCC_IFE_LITE_CSID_CLK>;
-            clock-names = "cam_ahb_clk",
-                          "cam_axi",
+            clock-names = "cam_axi",
                           "soc_ahb",
                           "camnoc_axi",
                           "core_ahb",
@@ -277,18 +296,24 @@ examples:
                           "vfe_lite_cphy_rx",
                           "vfe_lite_csid";
 
-            interrupts = <GIC_SPI 464 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 466 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 717 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 473 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 477 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 478 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 479 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 461 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 465 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 467 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 718 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 472 IRQ_TYPE_LEVEL_HIGH>;
+            // 469 for cpas-cdm
+            // 463 for qcom,cam-a5
+            // 474 for qcom,cam_jpeg_enc
+            // 475 for qcom,cam_jpeg_dma
+            // 476 for qcom,lrme
+            // 459 for qcom,cam-cpas
+            interrupts = <GIC_SPI 464 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 466 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 717 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 473 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 477 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 478 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 479 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 461 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 465 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 467 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 718 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 472 IRQ_TYPE_EDGE_RISING>;
             interrupt-names = "csid0",
                               "csid1",
                               "csid2",
@@ -302,6 +327,9 @@ examples:
                               "vfe2",
                               "vfe_lite";
 
+            // MASTER_CAMNOC_HF0_UNCOMP -> SLAVE_CAMNOC_UNCOMP ?
+            // MASTER_CAMNOC_SF_UNCOMP -> SLAVE_CAMNOC_UNCOMP ?
+            // MASTER_CAMNOC_ICP_UNCOMP -> SLAVE_CAMNOC_UNCOMP ?
             interconnects = <&gem_noc MASTER_AMPSS_M0 QCOM_ICC_TAG_ACTIVE_ONLY
                              &config_noc SLAVE_CAMERA_CFG QCOM_ICC_TAG_ACTIVE_ONLY>,
                             <&mmss_noc MASTER_CAMNOC_HF QCOM_ICC_TAG_ALWAYS
@@ -315,12 +343,18 @@ examples:
                                  "sf_mnoc",
                                  "sf_icp_mnoc";
 
+            // <&apps_smmu 0xd40 0x20> & <&apps_smmu 0xd60 0x20> for lrme
+            // <&apps_smmu 0xd00 0x20> & <&apps_smmu 0xd20 0x20> for jpeg
+            // <&apps_smmu 0xca2 0x0> & <&apps_smmu 0xc40 0x20> & <&apps_smmu 0xc60 0x20> & <&apps_smmu 0xcc0 0x20> & <&apps_smmu 0xce0 0x20> for icp
+            // <&apps_smmu 0xc80 0x0> for cpas-cdm0
             iommus = <&apps_smmu 0x820 0xc0>,
                      <&apps_smmu 0x840 0x0>,
                      <&apps_smmu 0x860 0xc0>,
                      <&apps_smmu 0x880 0x0>;
 
-            power-domains = <&camcc TITAN_TOP_GDSC>
+            // ipe_0_gdsc for qcom,cam-ipe
+            // bps_gdsc for qcom,cam-bps
+            power-domains = <&camcc TITAN_TOP_GDSC>,
                             <&camcc IFE_0_GDSC>,
                             <&camcc IFE_1_GDSC>,
                             <&camcc IFE_2_GDSC>;
@@ -329,8 +363,14 @@ examples:
                                  "ife1",
                                  "ife2";
 
-            vdd-csiphy-0p9-supply = <&vreg_l18a>;
-            vdd-csiphy-1p25-supply = <&vreg_l22a>;
+            vdd-csiphy0-0p9-supply = <&vreg_l18a>;
+            vdd-csiphy0-1p25-supply = <&vreg_l22a>;
+            vdd-csiphy1-0p9-supply = <&vreg_l18a>;
+            vdd-csiphy1-1p25-supply = <&vreg_l22a>;
+            vdd-csiphy2-0p9-supply = <&vreg_l18a>;
+            vdd-csiphy2-1p25-supply = <&vreg_l22a>;
+            vdd-csiphy3-0p9-supply = <&vreg_l18a>;
+            vdd-csiphy3-1p25-supply = <&vreg_l22a>;
 
             ports {
                 #address-cells = <1>;

>
>>>>> I believe this clock is critical, and it is set so in the SM6350 GCC 
>>>>> driver,
>>>>> therefore it should not be added here.
>>>>
>>>> True, gcc_camera_ahb_clk has CLK_IS_CRITICAL in gcc-sm6350.c
>>>
>>> DT describes hardware, not the happenstance of Linux driver setup.
>>>
>>> On that basis omitting <&gcc GCC_CAMERA_AHB_CLK> from the clock list is
>>> not correct.
>> 
>> This has been already discussed, an enumerous amount of Qualcomm/MSM
>> specific resourced like clocks enabled in ABL, Linux etc. are considered
>> critical and not described in the dtb.
>
> I still think the argument for that is tenuous wrong in fact but, I know 
> you are right, this is a lost argument.
>
> @Luca please _do_ include the full array of registers/clocks/nocs as you 
> find them and yeah drop the AXI clock from that description because reasons.
>
> https://lore.kernel.org/linux-arm-msm/20251113-add-support-for-camss-on-kaanapali-v6-1-1e6038785a8e@oss.qualcomm.com/
>
>   ---
> bod
Re: [PATCH v2 1/3] dt-bindings: media: camss: Add qcom,sm6350-camss
Posted by Bryan O'Donoghue 1 month, 3 weeks ago
On 19/12/2025 14:06, Luca Weiss wrote:
> Hi Bryan,
> 
> On Sun Nov 16, 2025 at 3:05 PM CET, Bryan O'Donoghue wrote:
>> On 14/11/2025 17:06, Vladimir Zapolskiy wrote:
>>> On 11/14/25 18:09, Bryan O'Donoghue wrote:
>>>> On 14/11/2025 13:06, Luca Weiss wrote:
>>>>> Hi Vladimir,
>>>>>
>>>>> On Fri Nov 14, 2025 at 1:40 PM CET, Vladimir Zapolskiy wrote:
>>>>>> Hi Luca.
>>>>>>
>>>>>> On 11/14/25 13:15, Luca Weiss wrote:
>>>>>>> Add bindings for the Camera Subsystem on the SM6350 SoC.
>>>>>>>
>>>>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>>>>>> ---
>>>>>>>      .../bindings/media/qcom,sm6350-camss.yaml          | 349 ++++++
>>>>>>> +++++++++++++++
>>>>>>>      1 file changed, 349 insertions(+)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/media/qcom,sm6350-
>>>>>>> camss.yaml b/Documentation/devicetree/bindings/media/qcom,sm6350-
>>>>>>> camss.yaml
>>>>>>> new file mode 100644
>>>>>>> index 000000000000..d812b5b50c05
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/media/qcom,sm6350-camss.yaml
>>>>>>> @@ -0,0 +1,349 @@
>>>>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>>>>> +%YAML 1.2
>>>>>>> +---
>>>>>>> +$id: http://devicetree.org/schemas/media/qcom,sm6350-camss.yaml#
>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>>> +
>>>>>>> +title: Qualcomm SM6350 Camera Subsystem (CAMSS)
>>>>>>> +
>>>>>>> +maintainers:
>>>>>>> +  - Luca Weiss <luca.weiss@fairphone.com>
>>>>>>> +
>>>>>>> +description:
>>>>>>> +  The CAMSS IP is a CSI decoder and ISP present on Qualcomm
>>>>>>> platforms.
>>>>>>> +
>>>>>>> +properties:
>>>>>>> +  compatible:
>>>>>>> +    const: qcom,sm6350-camss
>>>>>>> +
>>>>>>> +  reg:
>>>>>>> +    maxItems: 12
>>>>>>> +
>>>>>>> +  reg-names:
>>>>>>> +    items:
>>>>>>> +      - const: csid0
>>>>>>> +      - const: csid1
>>>>>>> +      - const: csid2
>>>>>>> +      - const: csid_lite
>>>>>>> +      - const: csiphy0
>>>>>>> +      - const: csiphy1
>>>>>>> +      - const: csiphy2
>>>>>>> +      - const: csiphy3
>>>>>>> +      - const: vfe0
>>>>>>> +      - const: vfe1
>>>>>>> +      - const: vfe2
>>>>>>> +      - const: vfe_lite
>>>>>>> +
>>>>>>> +  clocks:
>>>>>>> +    maxItems: 30
>>>>>>> +
>>>>>>> +  clock-names:
>>>>>>> +    items:
>>>>>>> +      - const: cam_ahb_clk
>>>>>>> +      - const: cam_axi
>>>>>>> +      - const: soc_ahb
>>>>>>> +      - const: camnoc_axi
>>>>>>> +      - const: core_ahb
>>>>>>> +      - const: cpas_ahb
>>>>>>> +      - const: csiphy0
>>>>>>> +      - const: csiphy0_timer
>>>>>>> +      - const: csiphy1
>>>>>>> +      - const: csiphy1_timer
>>>>>>> +      - const: csiphy2
>>>>>>> +      - const: csiphy2_timer
>>>>>>> +      - const: csiphy3
>>>>>>> +      - const: csiphy3_timer
>>>>>>> +      - const: slow_ahb_src
>>>>>>> +      - const: vfe0_axi
>>>>>>> +      - const: vfe0
>>>>>>> +      - const: vfe0_cphy_rx
>>>>>>> +      - const: vfe0_csid
>>>>>>> +      - const: vfe1_axi
>>>>>>> +      - const: vfe1
>>>>>>> +      - const: vfe1_cphy_rx
>>>>>>> +      - const: vfe1_csid
>>>>>>> +      - const: vfe2_axi
>>>>>>> +      - const: vfe2
>>>>>>> +      - const: vfe2_cphy_rx
>>>>>>> +      - const: vfe2_csid
>>>>>>> +      - const: vfe_lite
>>>>>>> +      - const: vfe_lite_cphy_rx
>>>>>>> +      - const: vfe_lite_csid
>>>>>>
>>>>>> The sorting order of this list does not follow the sorting order
>>>>>> accepted
>>>>>> in the past.
>>>>>
>>>>> What file should I best reference?
>>>>
>>>> Documentation/devicetree/bindings/media/qcom,sdm845-camss.yaml
>>>>
>>>>>>
>>>>>> I'm very sorry for the vagueness, but I can not pronounce the accepted
>>>>>> sorting order name, because it triggers people.
>>>>>>
>>>>>>> +
>>>>>>> +  interrupts:
>>>>>>> +    maxItems: 12
>>>>>>> +
>>>>>>> +  interrupt-names:
>>>>>>> +    items:
>>>>>>> +      - const: csid0
>>>>>>> +      - const: csid1
>>>>>>> +      - const: csid2
>>>>>>> +      - const: csid_lite
>>>>>>> +      - const: csiphy0
>>>>>>> +      - const: csiphy1
>>>>>>> +      - const: csiphy2
>>>>>>> +      - const: csiphy3
>>>>>>> +      - const: vfe0
>>>>>>> +      - const: vfe1
>>>>>>> +      - const: vfe2
>>>>>>> +      - const: vfe_lite
>>>>>>> +
>>>>>>> +  interconnects:
>>>>>>> +    maxItems: 4
>>>>>>> +
>>>>>>> +  interconnect-names:
>>>>>>> +    items:
>>>>>>> +      - const: ahb
>>>>>>> +      - const: hf_mnoc
>>>>>>> +      - const: sf_mnoc
>>>>>>> +      - const: sf_icp_mnoc
>>>>>>
>>>>>> Please remove sf_mnoc and sf_icp_mnoc, they are not needed for enabling
>>>>>> IP to produce raw images, and one day you may use them somewhere else.
>>>>>
>>>>> Ack, will give it a try.
>>>>
>>>> Disagree with this.
>>>>
>>>> See the Kanaapali patches. I'm asking new submissions to be as complete
>>>> as possible, instead of limiting the hardware description to the RDI.
>>>>
>>>> So listing the ICP noc is the right thing to do.
>>>>
>>>> So please include register banks for
>>>>
>>>> - bps
>>>> - cdm
>>>> - icp
>>>> - ipe
>>>> - jpeg
>>>> - lrme
>>>
>>> This suggests to get a DT backward compatibility lock forever,
>>
>> I'm not entirely sure what this comment means.
>>
>> The objective here is to get a full and complete DT describing camera
>> hardware that can be consumed by
>>
>> - CAMSS RDI
>> - CAMSS RDI + future goodness
>> - FreeBSD
>> - Any other DT consumer of upstream data perhaps even CamX
>> which
>>> makes it
>>> a very bad advice, which is favourable for just the single interested
>>> party,
>>> which offers an alternative to the upstream CAMSS.
>>>
>>> Anybody who wants to get support of any CAMSS ISP functionality above RAW
>>> images shall not add anything unrelated and unsupported.
>>>
>>> The asked inclusion shall be done later on safely, if ever needed.
>>
>> As I say the objective is to fully describe the hardware in DT, not to
>> describe only the RDI path.
> 
> So far I've gotten this diff on top of v2, not quite sure how to
> continue further...
> 
> Please advice how to turn these resources into a usable dt-binding.
> 
> I'm also a bit worried that it'll be subtly wrong due to all the
> additions being impossible to test for me.
> 
> Regards
> Luca
> 
> 
> diff --git a/Documentation/devicetree/bindings/media/qcom,sm6350-camss.yaml b/Documentation/devicetree/bindings/media/qcom,sm6350-camss.yaml
> index d812b5b50c05..340bf03ffe91 100644
> --- a/Documentation/devicetree/bindings/media/qcom,sm6350-camss.yaml
> +++ b/Documentation/devicetree/bindings/media/qcom,sm6350-camss.yaml
> @@ -203,6 +202,23 @@ examples:
>                     <0x0 0x0acb6000 0x0 0x4000>,
>                     <0x0 0x0acbd000 0x0 0x4000>,
>                     <0x0 0x0acc4000 0x0 0x4000>;
> +
> +                  // ICP ?? == qcom,cam-a5 ?
> +               // <0x0 0x0ac00000 0x0 0x6000>, // ? a5_qgic
> +               // <0x0 0x0ac10000 0x0 0x8000>, // ? a5_sierra
> +               // <0x0 0x0ac18000 0x0 0x3000>; // ? a5_csr
> +
> +               // <0x0 0x0ac87000 0x0 0xa000> // "ipe" IPE (Image Processing Engine)
> +               // <0x0 0x0ac6f000 0x0 0x8000> // "bps"? BPS
> +               // <0x0 0x0ac6b000 0x0 0xa00> // "lrme"? LRME
> +
> +               // <0x0 0x0ac40000 0x0 0x1000>, // ? CAM CPAS: cam_cpas_top
> +               // <0x0 0x0ac42000 0x0 0x4600>, // ? CAM CPAS: cam_camnoc
> +               // <0x0 0x01fc0000 0x0 0x40000>; // ? CAM CPAS: core_top_csr_tcsr
> +
> +               // <0x0 0x0ac52000 0x0 0x4000> // "jpeg_dma" JPEG DMA & Downscaler
> +               // <0x0 0x0ac4e000 0x0 0x4000> // "jpeg_enc" JPEG Encoder
> +               // <0x0 0x0ac48000 0x0 0x1000> // ? CPAS CDM

Looks correct.

>               reg-names = "csid0",
>                           "csid1",
>                           "csid2",
> @@ -216,8 +232,12 @@ examples:
>                           "vfe2",
>                           "vfe_lite";
>   
> -            clocks = <&gcc GCC_CAMERA_AHB_CLK>,
> -                     <&gcc GCC_CAMERA_AXI_CLK>,
> +            // CAMCC_FAST_AHB_CLK_SRC + CAMCC_ICP_CLK for qcom,cam-a5
> +            // CAMCC_IPE_0_AHB_CLK + CAMCC_IPE_0_AREG_CLK + CAMCC_IPE_0_AXI_CLK + CAMCC_IPE_0_CLK for qcom,cam-ipe
> +            // CAMCC_BPS_AHB_CLK + CAMCC_BPS_AREG_CLK + CAMCC_BPS_AXI_CLK + CAMCC_BPS_CLK for qcom,cam-bps
> +            // CAMCC_JPEG_CLK for qcom,cam_jpeg_enc / qcom,cam_jpeg_dma
> +            // CAMCC_LRME_CLK for qcom,lrme
> +            clocks = <&gcc GCC_CAMERA_AXI_CLK>,
>                        <&camcc CAMCC_SOC_AHB_CLK>,
>                        <&camcc CAMCC_CAMNOC_AXI_CLK>,
>                        <&camcc CAMCC_CORE_AHB_CLK>,
> @@ -246,8 +266,7 @@ examples:
>                        <&camcc CAMCC_IFE_LITE_CLK>,
>                        <&camcc CAMCC_IFE_LITE_CPHY_RX_CLK>,
>                        <&camcc CAMCC_IFE_LITE_CSID_CLK>;
> -            clock-names = "cam_ahb_clk",
> -                          "cam_axi",
> +            clock-names = "cam_axi",
>                             "soc_ahb",
>                             "camnoc_axi",
>                             "core_ahb",
> @@ -277,18 +296,24 @@ examples:
>                             "vfe_lite_cphy_rx",
>                             "vfe_lite_csid";
>   
> -            interrupts = <GIC_SPI 464 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 466 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 717 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 473 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 477 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 478 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 479 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 461 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 465 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 467 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 718 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 472 IRQ_TYPE_LEVEL_HIGH>;
> +            // 469 for cpas-cdm
> +            // 463 for qcom,cam-a5
> +            // 474 for qcom,cam_jpeg_enc
> +            // 475 for qcom,cam_jpeg_dma
> +            // 476 for qcom,lrme
> +            // 459 for qcom,cam-cpas
> +            interrupts = <GIC_SPI 464 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 466 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 717 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 473 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 477 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 478 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 479 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 461 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 465 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 467 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 718 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 472 IRQ_TYPE_EDGE_RISING>;
>               interrupt-names = "csid0",
>                                 "csid1",
>                                 "csid2",
> @@ -302,6 +327,9 @@ examples:
>                                 "vfe2",
>                                 "vfe_lite";
>   
> +            // MASTER_CAMNOC_HF0_UNCOMP -> SLAVE_CAMNOC_UNCOMP ?
> +            // MASTER_CAMNOC_SF_UNCOMP -> SLAVE_CAMNOC_UNCOMP ?
> +            // MASTER_CAMNOC_ICP_UNCOMP -> SLAVE_CAMNOC_UNCOMP ?
>               interconnects = <&gem_noc MASTER_AMPSS_M0 QCOM_ICC_TAG_ACTIVE_ONLY
>                                &config_noc SLAVE_CAMERA_CFG QCOM_ICC_TAG_ACTIVE_ONLY>,
>                               <&mmss_noc MASTER_CAMNOC_HF QCOM_ICC_TAG_ALWAYS
> @@ -315,12 +343,18 @@ examples:
>                                    "sf_mnoc",
>                                    "sf_icp_mnoc";
>   
> +            // <&apps_smmu 0xd40 0x20> & <&apps_smmu 0xd60 0x20> for lrme
> +            // <&apps_smmu 0xd00 0x20> & <&apps_smmu 0xd20 0x20> for jpeg
> +            // <&apps_smmu 0xca2 0x0> & <&apps_smmu 0xc40 0x20> & <&apps_smmu 0xc60 0x20> & <&apps_smmu 0xcc0 0x20> & <&apps_smmu 0xce0 0x20> for icp
> +            // <&apps_smmu 0xc80 0x0> for cpas-cdm0
>               iommus = <&apps_smmu 0x820 0xc0>,
>                        <&apps_smmu 0x840 0x0>,
>                        <&apps_smmu 0x860 0xc0>,
>                        <&apps_smmu 0x880 0x0>;
>   
> -            power-domains = <&camcc TITAN_TOP_GDSC>
> +            // ipe_0_gdsc for qcom,cam-ipe
> +            // bps_gdsc for qcom,cam-bps
> +            power-domains = <&camcc TITAN_TOP_GDSC>,
>                               <&camcc IFE_0_GDSC>,
>                               <&camcc IFE_1_GDSC>,
>                               <&camcc IFE_2_GDSC>;
> @@ -329,8 +363,14 @@ examples:
>                                    "ife1",
>                                    "ife2";
>   
> -            vdd-csiphy-0p9-supply = <&vreg_l18a>;
> -            vdd-csiphy-1p25-supply = <&vreg_l22a>;
> +            vdd-csiphy0-0p9-supply = <&vreg_l18a>;
> +            vdd-csiphy0-1p25-supply = <&vreg_l22a>;
> +            vdd-csiphy1-0p9-supply = <&vreg_l18a>;
> +            vdd-csiphy1-1p25-supply = <&vreg_l22a>;
> +            vdd-csiphy2-0p9-supply = <&vreg_l18a>;
> +            vdd-csiphy2-1p25-supply = <&vreg_l22a>;
> +            vdd-csiphy3-0p9-supply = <&vreg_l18a>;
> +            vdd-csiphy3-1p25-supply = <&vreg_l22a>;
>   
>               ports {
>                   #address-cells = <1>;
> 
These look right to me, consistent with what I see downstream.

---
bod