[PATCH 0/6] media: qcom: camss: Add sc7280 support

[PATCH 0/6] media: qcom: camss: Add sc7280 support

[Vikram Sharma]

SC7280 is a Qualcomm SoC. This series adds support to
bring up the CSIPHY, CSID, VFE/RDI interfaces in SC7280.

SC7280 provides

- 3 x VFE, 3 RDI per VFE
- 2 x VFE Lite, 4 RDI per VFE
- 3 x CSID
- 2 x CSID Lite
- 5 x CSI PHY

This series is rebased based on：
https://lore.kernel.org/linux-arm-msm/20240522154659.510-1-quic_grosikop@quicinc.com/

The changes are verified on SC7280 qcs6490-rb3gen2 board, the base dts for qcs6490-rb3gen2
is:
https://lore.kernel.org/all/20231103184655.23555-1-quic_kbajaj@quicinc.com/

Suresh Vankadara (2):
media: qcom: camss: support for camss driver on sc7280
arm64: dts: qcom: sc7280: Add support for camss

Trishansh Bhardwaj (2):
media: qcom: camss: support for camss driver on sc7280
arm64: dts: qcom: sc7280: Add support for camss

Vikram Sharma (1):
media: dt-bindings: media: camss: Add qcom,sc7280-camss binding

Hariram Purshotam (3):
i2c: Enable IMX577 camera sensor for qcm6490
arm64: dts: qcom: qcs6490-rb3gen2: Enable IMX577 camera sensor
arm64: dts: qcom: sc7280: Add IMX577 camera sensor

Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com>
---
Suresh Vankadara (1):
      media: qcom: camss: support for camss driver for sc7280

Vikram Sharma (5):
      media: dt-bindings: media: camss: Add qcom,sc7280-camss binding
      arm64: dts: qcom: sc7280: Add support for camss
      arm64: dts: qcom: sc7280: Add IMX577 camera sensor
      arm64: dts: qcom: qcs6490-rb3gen2: Enable IMX577 camera sensor
      i2c: Enable IMX577 camera sensor for qcm6490

 .../bindings/media/qcom,sc7280-camss.yaml          | 477 +++++++++++++++++++++
 arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts       |  67 +++
 arch/arm64/boot/dts/qcom/sc7280.dtsi               | 215 ++++++++++
 drivers/i2c/busses/i2c-qcom-cci.c                  |   1 +
 drivers/media/platform/qcom/camss/camss-csid.c     |  16 +-
 .../platform/qcom/camss/camss-csiphy-3ph-1-0.c     |   2 +
 drivers/media/platform/qcom/camss/camss-vfe.c      |   2 +
 drivers/media/platform/qcom/camss/camss.c          | 340 +++++++++++++++
 drivers/media/platform/qcom/camss/camss.h          |   2 +
 9 files changed, 1119 insertions(+), 3 deletions(-)
---
base-commit: 18eeb2d92baca167809cd5d48eb60e0a5c036d51
change-id: 20240627-camss_first_post_linux_next-f4163c90093c

Best regards,
-- 
Vikram Sharma <quic_vikramsa@quicinc.com>

[PATCH 1/6] media: dt-bindings: media: camss: Add qcom,sc7280-camss binding

[Vikram Sharma]

Add bindings for qcom,sc7280-camss in order to support the camera
subsystem for sc7280.

Signed-off-by: Suresh Vankadara <quic_svankada@quicinc.com>
Signed-off-by: Trishansh Bhardwaj <quic_tbhardwa@quicinc.com>
Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com>
---
 .../bindings/media/qcom,sc7280-camss.yaml          | 477 +++++++++++++++++++++
 1 file changed, 477 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/qcom,sc7280-camss.yaml b/Documentation/devicetree/bindings/media/qcom,sc7280-camss.yaml
new file mode 100644
index 000000000000..588c6fb50e2f
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/qcom,sc7280-camss.yaml
@@ -0,0 +1,477 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+
+---
+$id: http://devicetree.org/schemas/media/qcom,sc7280-camss.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Technologies, Inc. SC7280 CAMSS ISP
+
+maintainers:
+  - Azam Sadiq Pasha Kapatrala Syed <akapatra@quicinc.com>
+  - Hariram Purushothaman <hariramp@quicinc.com>
+
+description: |
+  The CAMSS IP is a CSI decoder and ISP present on
+  Qualcomm Technologies, Inc. platforms.
+
+properties:
+  compatible:
+    const: qcom,sc7280-camss
+
+  clocks:
+    minItems: 41
+    maxItems: 41
+
+  clock-names:
+    items:
+
+      - const: cam_hf_axi
+      - const: slow_ahb_src
+      - const: cpas_ahb
+      - const: camnoc_axi_src
+      - const: camnoc_axi
+      - const: csiphy0
+      - const: csiphy0_timer
+      - const: csiphy0_timer_src
+      - const: csiphy1
+      - const: csiphy1_timer
+      - const: csiphy1_timer_src
+      - const: csiphy2
+      - const: csiphy2_timer
+      - const: csiphy2_timer_src
+      - const: csiphy3
+      - const: csiphy3_timer
+      - const: csiphy3_timer_src
+      - const: csiphy4
+      - const: csiphy4_timer
+      - const: csiphy4_timer_src
+      - const: vfe0_csid
+      - const: vfe0_cphy_rx
+      - const: vfe0
+      - const: vfe0_axi
+      - const: csiphy_rx_src
+      - const: vfe1_csid
+      - const: vfe1_cphy_rx
+      - const: vfe1
+      - const: vfe1_axi
+      - const: vfe2_csid
+      - const: vfe2_cphy_rx
+      - const: vfe2
+      - const: vfe2_axi
+      - const: vfe0_lite_csid
+      - const: vfe0_lite_cphy_rx
+      - const: vfe0_lite
+      - const: vfe1_lite_csid
+      - const: vfe1_lite_cphy_rx
+      - const: vfe1_lite
+      - const: vfe_lite0
+      - const: vfe_lite1
+
+  interrupts:
+    minItems: 15
+    maxItems: 15
+
+  interrupt-names:
+    items:
+      - const: csiphy0
+      - const: csiphy1
+      - const: csiphy2
+      - const: csiphy3
+      - const: csiphy4
+      - const: csid0
+      - const: csid1
+      - const: csid2
+      - const: csid_lite0
+      - const: csid_lite1
+      - const: vfe0
+      - const: vfe1
+      - const: vfe2
+      - const: vfe_lite0
+      - const: vfe_lite1
+
+  iommus:
+    maxItems: 1
+
+  interconnects:
+    minItems: 2
+    maxItems: 2
+
+  interconnect-names:
+    items:
+      - const: cam_ahb
+      - const: cam_hf_0
+
+  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 GDSC - Titan ISP Block, Global Distributed Switch Controller.
+
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+
+    description:
+      CSI input ports.
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        unevaluatedProperties: false
+        description:
+          Input port for receiving CSI data.
+
+        properties:
+          endpoint:
+            $ref: video-interfaces.yaml#
+            unevaluatedProperties: false
+
+            properties:
+              clock-lanes:
+                maxItems: 1
+
+              data-lanes:
+                minItems: 1
+                maxItems: 4
+
+            required:
+              - clock-lanes
+              - data-lanes
+
+      port@1:
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        unevaluatedProperties: false
+        description:
+          Input port for receiving CSI data.
+
+        properties:
+          endpoint:
+            $ref: video-interfaces.yaml#
+            unevaluatedProperties: false
+
+            properties:
+              clock-lanes:
+                maxItems: 1
+
+              data-lanes:
+                minItems: 1
+                maxItems: 4
+
+            required:
+              - clock-lanes
+              - data-lanes
+
+      port@2:
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        unevaluatedProperties: false
+        description:
+          Input port for receiving CSI data.
+
+        properties:
+          endpoint:
+            $ref: video-interfaces.yaml#
+            unevaluatedProperties: false
+
+            properties:
+              clock-lanes:
+                maxItems: 1
+
+              data-lanes:
+                minItems: 1
+                maxItems: 4
+
+            required:
+              - clock-lanes
+              - data-lanes
+
+      port@3:
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        unevaluatedProperties: false
+        description:
+          Input port for receiving CSI data.
+
+        properties:
+          endpoint:
+            $ref: video-interfaces.yaml#
+            unevaluatedProperties: false
+
+            properties:
+              clock-lanes:
+                maxItems: 1
+
+              data-lanes:
+                minItems: 1
+                maxItems: 4
+
+            required:
+              - clock-lanes
+              - data-lanes
+
+      port@4:
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        unevaluatedProperties: false
+        description:
+          Input port for receiving CSI data.
+
+        properties:
+          endpoint:
+            $ref: video-interfaces.yaml#
+            unevaluatedProperties: false
+
+            properties:
+              clock-lanes:
+                maxItems: 1
+
+              data-lanes:
+                minItems: 1
+                maxItems: 4
+
+            required:
+              - clock-lanes
+              - data-lanes
+
+      port@5:
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        unevaluatedProperties: false
+        description:
+          Input port for receiving CSI data.
+
+        properties:
+          endpoint:
+            $ref: video-interfaces.yaml#
+            unevaluatedProperties: false
+
+            properties:
+              clock-lanes:
+                maxItems: 1
+
+              data-lanes:
+                minItems: 1
+                maxItems: 4
+
+            required:
+              - clock-lanes
+              - data-lanes
+
+  reg:
+    minItems: 10
+    maxItems: 10
+
+  reg-names:
+    items:
+      - const: vfe0
+      - const: vfe1
+      - const: vfe2
+      - const: vfe_lite0
+      - const: vfe_lite1
+      - const: csiphy0
+      - const: csiphy1
+      - const: csiphy2
+      - const: csiphy3
+      - const: csiphy4
+
+required:
+  - clock-names
+  - clocks
+  - compatible
+  - interconnects
+  - interconnect-names
+  - interrupts
+  - interrupt-names
+  - iommus
+  - power-domains
+  - reg
+  - reg-names
+
+additionalProperties: false
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/clock/qcom,camcc-sc7280.h>
+    #include <dt-bindings/interconnect/qcom,sc7280.h>
+    #include <dt-bindings/clock/qcom,gcc-sc7280.h>
+    #include <dt-bindings/power/qcom-rpmpd.h>
+
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        camss: camss@acaf000 {
+            compatible = "qcom,sc7280-camss";
+            reg = <0x0 0x0acaf000 0x0 0x5200>,
+                <0x0 0x0acb6000 0x0 0x5200>,
+                <0x0 0x0acbd000 0x0 0x5200>,
+                <0x0 0x0acc4000 0x0 0x5000>,
+                <0x0 0x0accb000 0x0 0x5000>,
+                <0x0 0x0ace0000 0x0 0x2000>,
+                <0x0 0x0ace2000 0x0 0x2000>,
+                <0x0 0x0ace4000 0x0 0x2000>,
+                <0x0 0x0ace6000 0x0 0x2000>,
+                <0x0 0x0ace8000 0x0 0x2000>;
+
+            reg-names = "vfe0",
+                "vfe1",
+                "vfe2",
+                "vfe_lite0",
+                "vfe_lite1",
+                "csiphy0",
+                "csiphy1",
+                "csiphy2",
+                "csiphy3",
+                "csiphy4";
+
+            interrupts = <GIC_SPI 477 IRQ_TYPE_EDGE_RISING>,
+                <GIC_SPI 478 IRQ_TYPE_EDGE_RISING>,
+                <GIC_SPI 479 IRQ_TYPE_EDGE_RISING>,
+                <GIC_SPI 448 IRQ_TYPE_EDGE_RISING>,
+                <GIC_SPI 122 IRQ_TYPE_EDGE_RISING>,
+                <GIC_SPI 464 IRQ_TYPE_EDGE_RISING>,
+                <GIC_SPI 466 IRQ_TYPE_EDGE_RISING>,
+                <GIC_SPI 640 IRQ_TYPE_EDGE_RISING>,
+                <GIC_SPI 468 IRQ_TYPE_EDGE_RISING>,
+                <GIC_SPI 359 IRQ_TYPE_EDGE_RISING>,
+                <GIC_SPI 465 IRQ_TYPE_EDGE_RISING>,
+                <GIC_SPI 467 IRQ_TYPE_EDGE_RISING>,
+                <GIC_SPI 641 IRQ_TYPE_EDGE_RISING>,
+                <GIC_SPI 469 IRQ_TYPE_EDGE_RISING>,
+                <GIC_SPI 360 IRQ_TYPE_EDGE_RISING>;
+
+            interrupt-names = "csiphy0",
+                "csiphy1",
+                "csiphy2",
+                "csiphy3",
+                "csiphy4",
+                "csid0",
+                "csid1",
+                "csid2",
+                "csid_lite0",
+                "csid_lite1",
+                "vfe0",
+                "vfe1",
+                "vfe2",
+                "vfe_lite0",
+                "vfe_lite1";
+
+            power-domains = <&camcc CAM_CC_IFE_0_GDSC>,
+                <&camcc CAM_CC_IFE_1_GDSC>,
+                <&camcc CAM_CC_IFE_2_GDSC>,
+                <&camcc CAM_CC_TITAN_TOP_GDSC>;
+
+            clocks = <&gcc GCC_CAMERA_HF_AXI_CLK>,
+                <&camcc CAM_CC_SLOW_AHB_CLK_SRC>,
+                <&camcc CAM_CC_CPAS_AHB_CLK>,
+                <&camcc CAM_CC_CAMNOC_AXI_CLK_SRC>,
+                <&camcc CAM_CC_CAMNOC_AXI_CLK>,
+                <&camcc CAM_CC_CSIPHY0_CLK>,
+                <&camcc CAM_CC_CSI0PHYTIMER_CLK>,
+                <&camcc CAM_CC_CSI0PHYTIMER_CLK_SRC>,
+                <&camcc CAM_CC_CSIPHY1_CLK>,
+                <&camcc CAM_CC_CSI1PHYTIMER_CLK>,
+                <&camcc CAM_CC_CSI1PHYTIMER_CLK_SRC>,
+                <&camcc CAM_CC_CSIPHY2_CLK>,
+                <&camcc CAM_CC_CSI2PHYTIMER_CLK>,
+                <&camcc CAM_CC_CSI2PHYTIMER_CLK_SRC>,
+                <&camcc CAM_CC_CSIPHY3_CLK>,
+                <&camcc CAM_CC_CSI3PHYTIMER_CLK>,
+                <&camcc CAM_CC_CSI3PHYTIMER_CLK_SRC>,
+                <&camcc CAM_CC_CSIPHY4_CLK>,
+                <&camcc CAM_CC_CSI4PHYTIMER_CLK>,
+                <&camcc CAM_CC_CSI4PHYTIMER_CLK_SRC>,
+                <&camcc CAM_CC_IFE_0_CSID_CLK>,
+                <&camcc CAM_CC_IFE_0_CPHY_RX_CLK>,
+                <&camcc CAM_CC_IFE_0_CLK>,
+                <&camcc CAM_CC_IFE_0_AXI_CLK>,
+                <&camcc CAM_CC_CPHY_RX_CLK_SRC>,
+                <&camcc CAM_CC_IFE_1_CSID_CLK>,
+                <&camcc CAM_CC_IFE_1_CPHY_RX_CLK>,
+                <&camcc CAM_CC_IFE_1_CLK>,
+                <&camcc CAM_CC_IFE_1_AXI_CLK>,
+                <&camcc CAM_CC_IFE_2_CSID_CLK>,
+                <&camcc CAM_CC_IFE_2_CPHY_RX_CLK>,
+                <&camcc CAM_CC_IFE_2_CLK>,
+                <&camcc CAM_CC_IFE_2_AXI_CLK>,
+                <&camcc CAM_CC_IFE_LITE_0_CSID_CLK>,
+                <&camcc CAM_CC_IFE_LITE_0_CPHY_RX_CLK>,
+                <&camcc CAM_CC_IFE_LITE_0_CLK>,
+                <&camcc CAM_CC_IFE_LITE_1_CSID_CLK>,
+                <&camcc CAM_CC_IFE_LITE_1_CPHY_RX_CLK>,
+                <&camcc CAM_CC_IFE_LITE_1_CLK>,
+                <&camcc CAM_CC_IFE_LITE_0_CLK>,
+                <&camcc CAM_CC_IFE_LITE_1_CLK>;
+
+            clock-names = "cam_hf_axi",
+                "slow_ahb_src",
+                "cpas_ahb",
+                "camnoc_axi_src",
+                "camnoc_axi",
+                "csiphy0",
+                "csiphy0_timer",
+                "csiphy0_timer_src",
+                "csiphy1",
+                "csiphy1_timer",
+                "csiphy1_timer_src",
+                "csiphy2",
+                "csiphy2_timer",
+                "csiphy2_timer_src",
+                "csiphy3",
+                "csiphy3_timer",
+                "csiphy3_timer_src",
+                "csiphy4",
+                "csiphy4_timer",
+                "csiphy4_timer_src",
+                "vfe0_csid",
+                "vfe0_cphy_rx",
+                "vfe0",
+                "vfe0_axi",
+                "csiphy_rx_src",
+                "vfe1_csid",
+                "vfe1_cphy_rx",
+                "vfe1",
+                "vfe1_axi",
+                "vfe2_csid",
+                "vfe2_cphy_rx",
+                "vfe2",
+                "vfe2_axi",
+                "vfe0_lite_csid",
+                "vfe0_lite_cphy_rx",
+                "vfe0_lite",
+                "vfe1_lite_csid",
+                "vfe1_lite_cphy_rx",
+                "vfe1_lite",
+                "vfe_lite0",
+                "vfe_lite1";
+
+            iommus = <&apps_smmu 0x800 0x4e0>;
+
+            interconnect-names = "cam_ahb", "cam_hf_0";
+
+            interconnects = <&gem_noc  MASTER_APPSS_PROC 0 &cnoc2 SLAVE_CAMERA_CFG 0>,
+                <&mmss_noc MASTER_CAMNOC_HF  0 &mc_virt SLAVE_EBI1     0>;
+
+            ports {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                port@0 {
+                    reg = <0>;
+                };
+
+                port@1 {
+                    reg = <1>;
+                };
+
+                port@2 {
+                    reg = <2>;
+                };
+
+                port@3 {
+                    reg = <3>;
+                };
+
+                port@4 {
+                    reg = <4>;
+                };
+            };
+        };
+    };

-- 
2.25.1

Re: [PATCH 1/6] media: dt-bindings: media: camss: Add qcom,sc7280-camss binding

[Bryan O'Donoghue]

On 28/06/2024 19:32, Vikram Sharma wrote:
> Add bindings for qcom,sc7280-camss in order to support the camera
> subsystem for sc7280.
> 
> Signed-off-by: Suresh Vankadara <quic_svankada@quicinc.com>
> Signed-off-by: Trishansh Bhardwaj <quic_tbhardwa@quicinc.com>
> Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com>
> ---
>   .../bindings/media/qcom,sc7280-camss.yaml          | 477 +++++++++++++++++++++
>   1 file changed, 477 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/qcom,sc7280-camss.yaml b/Documentation/devicetree/bindings/media/qcom,sc7280-camss.yaml
> new file mode 100644
> index 000000000000..588c6fb50e2f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/qcom,sc7280-camss.yaml
> @@ -0,0 +1,477 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +
> +---
> +$id: http://devicetree.org/schemas/media/qcom,sc7280-camss.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Technologies, Inc. SC7280 CAMSS ISP
> +
> +maintainers:
> +  - Azam Sadiq Pasha Kapatrala Syed <akapatra@quicinc.com>
> +  - Hariram Purushothaman <hariramp@quicinc.com>
> +
> +description: |
> +  The CAMSS IP is a CSI decoder and ISP present on
> +  Qualcomm Technologies, Inc. platforms.
> +
> +properties:
> +  compatible:
> +    const: qcom,sc7280-camss
> +
> +  clocks:
> +    minItems: 41
> +    maxItems: 41
> +
> +  clock-names:
> +    items:
> +
> +      - const: cam_hf_axi
> +      - const: slow_ahb_src
> +      - const: cpas_ahb
> +      - const: camnoc_axi_src

You almost certainly don't need to include the "_src" clocks in the list 
of clocks since the CAMCC driver lists "someclock" as having parent 
"someclock_src"

> +      - const: camnoc_axi
> +      - const: csiphy0
> +      - const: csiphy0_timer
> +      - const: csiphy0_timer_src
> +      - const: csiphy1
> +      - const: csiphy1_timer
> +      - const: csiphy1_timer_src
> +      - const: csiphy2
> +      - const: csiphy2_timer
> +      - const: csiphy2_timer_src
> +      - const: csiphy3
> +      - const: csiphy3_timer
> +      - const: csiphy3_timer_src
> +      - const: csiphy4
> +      - const: csiphy4_timer
> +      - const: csiphy4_timer_src
> +      - const: vfe0_csid
> +      - const: vfe0_cphy_rx
> +      - const: vfe0
> +      - const: vfe0_axi
> +      - const: csiphy_rx_src
> +      - const: vfe1_csid
> +      - const: vfe1_cphy_rx
> +      - const: vfe1
> +      - const: vfe1_axi
> +      - const: vfe2_csid
> +      - const: vfe2_cphy_rx
> +      - const: vfe2
> +      - const: vfe2_axi
> +      - const: vfe0_lite_csid
> +      - const: vfe0_lite_cphy_rx
> +      - const: vfe0_lite
> +      - const: vfe1_lite_csid
> +      - const: vfe1_lite_cphy_rx
> +      - const: vfe1_lite
> +      - const: vfe_lite0
> +      - const: vfe_lite1
> +
> +  interrupts:
> +    minItems: 15
> +    maxItems: 15
> +
> +  interrupt-names:
> +    items:
> +      - const: csiphy0
> +      - const: csiphy1
> +      - const: csiphy2
> +      - const: csiphy3
> +      - const: csiphy4
> +      - const: csid0
> +      - const: csid1
> +      - const: csid2
> +      - const: csid_lite0
> +      - const: csid_lite1
> +      - const: vfe0
> +      - const: vfe1
> +      - const: vfe2
> +      - const: vfe_lite0
> +      - const: vfe_lite1
> +
> +  iommus:
> +    maxItems: 1
> +
> +  interconnects:
> +    minItems: 2
> +    maxItems: 2
> +
> +  interconnect-names:
> +    items:
> +      - const: cam_ahb
> +      - const: cam_hf_0
> +
> +  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 GDSC - Titan ISP Block, Global Distributed Switch Controller.

Please name these power domains.

https://lore.kernel.org/linux-arm-msm/fcdb072d-6099-4423-b4b5-21e9052b82cc@linaro.org/

---
bod

Re: [PATCH 1/6] media: dt-bindings: media: camss: Add qcom,sc7280-camss binding

[Krzysztof Kozlowski]

On 28/06/2024 20:32, Vikram Sharma wrote:
> Add bindings for qcom,sc7280-camss in order to support the camera
> subsystem for sc7280.
> 
> Signed-off-by: Suresh Vankadara <quic_svankada@quicinc.com>
> Signed-off-by: Trishansh Bhardwaj <quic_tbhardwa@quicinc.com>
> Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com>
> ---
>  .../bindings/media/qcom,sc7280-camss.yaml          | 477 +++++++++++++++++++++
>  1 file changed, 477 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/qcom,sc7280-camss.yaml b/Documentation/devicetree/bindings/media/qcom,sc7280-camss.yaml
> new file mode 100644
> index 000000000000..588c6fb50e2f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/qcom,sc7280-camss.yaml
> @@ -0,0 +1,477 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +
> +---
> +$id: http://devicetree.org/schemas/media/qcom,sc7280-camss.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Technologies, Inc. SC7280 CAMSS ISP

We write only "Qualcomm", drop "Technologies, Inc.".

> +
> +maintainers:
> +  - Azam Sadiq Pasha Kapatrala Syed <akapatra@quicinc.com>
> +  - Hariram Purushothaman <hariramp@quicinc.com>
> +
> +description: |

Do not need '|' unless you need to preserve formatting.

> +  The CAMSS IP is a CSI decoder and ISP present on
> +  Qualcomm Technologies, Inc. platforms.
> +
> +properties:
> +  compatible:
> +    const: qcom,sc7280-camss
> +
> +  clocks:
> +    minItems: 41


Drop

> +    maxItems: 41
> +
> +  clock-names:
> +    items:
> +


Drop blank line

> +      - const: cam_hf_axi
> +      - const: slow_ahb_src
> +      - const: cpas_ahb
> +      - const: camnoc_axi_src
> +      - const: camnoc_axi

Which file did you use as template? Isn't this supposed to be reversed?

BTW, you *must* work on latest tree. Do not take some ancient code and
use as template. It's really waste of time to point the same issues over
and over again.

> +      - const: csiphy0
> +      - const: csiphy0_timer
> +      - const: csiphy0_timer_src
> +      - const: csiphy1
> +      - const: csiphy1_timer
> +      - const: csiphy1_timer_src
> +      - const: csiphy2
> +      - const: csiphy2_timer
> +      - const: csiphy2_timer_src
> +      - const: csiphy3
> +      - const: csiphy3_timer
> +      - const: csiphy3_timer_src
> +      - const: csiphy4
> +      - const: csiphy4_timer
> +      - const: csiphy4_timer_src
> +      - const: vfe0_csid
> +      - const: vfe0_cphy_rx
> +      - const: vfe0
> +      - const: vfe0_axi
> +      - const: csiphy_rx_src
> +      - const: vfe1_csid
> +      - const: vfe1_cphy_rx
> +      - const: vfe1
> +      - const: vfe1_axi
> +      - const: vfe2_csid
> +      - const: vfe2_cphy_rx
> +      - const: vfe2
> +      - const: vfe2_axi
> +      - const: vfe0_lite_csid
> +      - const: vfe0_lite_cphy_rx

Why these are reversed comparing to other bindings? Srsly, from which
file did you take it?

> +      - const: vfe0_lite
> +      - const: vfe1_lite_csid
> +      - const: vfe1_lite_cphy_rx
> +      - const: vfe1_lite
> +      - const: vfe_lite0
> +      - const: vfe_lite1
> +
> +  interrupts:
> +    minItems: 15

Drop

> +    maxItems: 15
> +
> +  interrupt-names:
> +    items:
> +      - const: csiphy0
> +      - const: csiphy1
> +      - const: csiphy2
> +      - const: csiphy3
> +      - const: csiphy4
> +      - const: csid0
> +      - const: csid1
> +      - const: csid2
> +      - const: csid_lite0
> +      - const: csid_lite1
> +      - const: vfe0
> +      - const: vfe1
> +      - const: vfe2
> +      - const: vfe_lite0
> +      - const: vfe_lite1
> +
> +  iommus:
> +    maxItems: 1
> +
> +  interconnects:
> +    minItems: 2

Drop

> +    maxItems: 2
> +
> +  interconnect-names:
> +    items:
> +      - const: cam_ahb
> +      - const: cam_hf_0

Drop "cam_" prefix in both. Actually everywhere...


> +
> +  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 GDSC - Titan ISP Block, Global Distributed Switch Controller.
> +
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +
> +    description:
> +      CSI input ports.
> +
> +    properties:
> +      port@0:
> +        $ref: /schemas/graph.yaml#/$defs/port-base
> +        unevaluatedProperties: false
> +        description:
> +          Input port for receiving CSI data.
> +
> +        properties:
> +          endpoint:
> +            $ref: video-interfaces.yaml#
> +            unevaluatedProperties: false
> +
> +            properties:
> +              clock-lanes:
> +                maxItems: 1
> +
> +              data-lanes:
> +                minItems: 1
> +                maxItems: 4
> +
> +            required:
> +              - clock-lanes
> +              - data-lanes
> +
> +      port@1:
> +        $ref: /schemas/graph.yaml#/$defs/port-base
> +        unevaluatedProperties: false
> +        description:
> +          Input port for receiving CSI data.
> +
> +        properties:
> +          endpoint:
> +            $ref: video-interfaces.yaml#
> +            unevaluatedProperties: false
> +
> +            properties:
> +              clock-lanes:
> +                maxItems: 1
> +
> +              data-lanes:
> +                minItems: 1
> +                maxItems: 4
> +
> +            required:
> +              - clock-lanes
> +              - data-lanes
> +
> +      port@2:
> +        $ref: /schemas/graph.yaml#/$defs/port-base
> +        unevaluatedProperties: false
> +        description:
> +          Input port for receiving CSI data.
> +
> +        properties:
> +          endpoint:
> +            $ref: video-interfaces.yaml#
> +            unevaluatedProperties: false
> +
> +            properties:
> +              clock-lanes:
> +                maxItems: 1
> +
> +              data-lanes:
> +                minItems: 1
> +                maxItems: 4
> +
> +            required:
> +              - clock-lanes
> +              - data-lanes
> +
> +      port@3:
> +        $ref: /schemas/graph.yaml#/$defs/port-base
> +        unevaluatedProperties: false
> +        description:
> +          Input port for receiving CSI data.
> +
> +        properties:
> +          endpoint:
> +            $ref: video-interfaces.yaml#
> +            unevaluatedProperties: false
> +
> +            properties:
> +              clock-lanes:
> +                maxItems: 1
> +
> +              data-lanes:
> +                minItems: 1
> +                maxItems: 4
> +
> +            required:
> +              - clock-lanes
> +              - data-lanes
> +
> +      port@4:
> +        $ref: /schemas/graph.yaml#/$defs/port-base
> +        unevaluatedProperties: false
> +        description:
> +          Input port for receiving CSI data.
> +
> +        properties:
> +          endpoint:
> +            $ref: video-interfaces.yaml#
> +            unevaluatedProperties: false
> +
> +            properties:
> +              clock-lanes:
> +                maxItems: 1
> +
> +              data-lanes:
> +                minItems: 1
> +                maxItems: 4
> +
> +            required:
> +              - clock-lanes
> +              - data-lanes
> +
> +      port@5:
> +        $ref: /schemas/graph.yaml#/$defs/port-base
> +        unevaluatedProperties: false
> +        description:
> +          Input port for receiving CSI data.
> +
> +        properties:
> +          endpoint:
> +            $ref: video-interfaces.yaml#
> +            unevaluatedProperties: false
> +
> +            properties:
> +              clock-lanes:
> +                maxItems: 1
> +
> +              data-lanes:
> +                minItems: 1
> +                maxItems: 4
> +
> +            required:
> +              - clock-lanes
> +              - data-lanes
> +
> +  reg:
> +    minItems: 10

Drop

> +    maxItems: 10
> +
> +  reg-names:
> +    items:
> +      - const: vfe0
> +      - const: vfe1
> +      - const: vfe2
> +      - const: vfe_lite0
> +      - const: vfe_lite1
> +      - const: csiphy0
> +      - const: csiphy1
> +      - const: csiphy2
> +      - const: csiphy3
> +      - const: csiphy4

Which file did you use as template for these?

> +
> +required:
> +  - clock-names
> +  - clocks
> +  - compatible
> +  - interconnects
> +  - interconnect-names
> +  - interrupts
> +  - interrupt-names
> +  - iommus
> +  - power-domains
> +  - reg
> +  - reg-names
> +
> +additionalProperties: false

Missing blank line

> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/clock/qcom,camcc-sc7280.h>
> +    #include <dt-bindings/interconnect/qcom,sc7280.h>
> +    #include <dt-bindings/clock/qcom,gcc-sc7280.h>
> +    #include <dt-bindings/power/qcom-rpmpd.h>

These should be in alphabetical order, not random.

> +
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        camss: camss@acaf000 {
> +            compatible = "qcom,sc7280-camss";
> +            reg = <0x0 0x0acaf000 0x0 0x5200>,
> +                <0x0 0x0acb6000 0x0 0x5200>,
> +                <0x0 0x0acbd000 0x0 0x5200>,
> +                <0x0 0x0acc4000 0x0 0x5000>,
> +                <0x0 0x0accb000 0x0 0x5000>,
> +                <0x0 0x0ace0000 0x0 0x2000>,
> +                <0x0 0x0ace2000 0x0 0x2000>,
> +                <0x0 0x0ace4000 0x0 0x2000>,
> +                <0x0 0x0ace6000 0x0 0x2000>,
> +                <0x0 0x0ace8000 0x0 0x2000>;
> +
> +            reg-names = "vfe0",
> +                "vfe1",
> +                "vfe2",

Misaligned.

> +                "vfe_lite0",
> +                "vfe_lite1",
> +                "csiphy0",
> +                "csiphy1",
> +                "csiphy2",
> +                "csiphy3",
> +                "csiphy4";
> +
> +            interrupts = <GIC_SPI 477 IRQ_TYPE_EDGE_RISING>,
> +                <GIC_SPI 478 IRQ_TYPE_EDGE_RISING>,

Misaligned.

> +                <GIC_SPI 479 IRQ_TYPE_EDGE_RISING>,
> +                <GIC_SPI 448 IRQ_TYPE_EDGE_RISING>,
> +                <GIC_SPI 122 IRQ_TYPE_EDGE_RISING>,
> +                <GIC_SPI 464 IRQ_TYPE_EDGE_RISING>,
> +                <GIC_SPI 466 IRQ_TYPE_EDGE_RISING>,
> +                <GIC_SPI 640 IRQ_TYPE_EDGE_RISING>,
> +                <GIC_SPI 468 IRQ_TYPE_EDGE_RISING>,
> +                <GIC_SPI 359 IRQ_TYPE_EDGE_RISING>,
> +                <GIC_SPI 465 IRQ_TYPE_EDGE_RISING>,
> +                <GIC_SPI 467 IRQ_TYPE_EDGE_RISING>,
> +                <GIC_SPI 641 IRQ_TYPE_EDGE_RISING>,
> +                <GIC_SPI 469 IRQ_TYPE_EDGE_RISING>,
> +                <GIC_SPI 360 IRQ_TYPE_EDGE_RISING>;
> +
> +            interrupt-names = "csiphy0",
> +                "csiphy1",
> +                "csiphy2",

Misaligned.

> +                "csiphy3",
> +                "csiphy4",
> +                "csid0",
> +                "csid1",
> +                "csid2",
> +                "csid_lite0",
> +                "csid_lite1",
> +                "vfe0",
> +                "vfe1",
> +                "vfe2",
> +                "vfe_lite0",
> +                "vfe_lite1";


Best regards,
Krzysztof

[PATCH 2/6] arm64: dts: qcom: sc7280: Add support for camss

[Vikram Sharma]

This change adds camera subsystem changes.

Signed-off-by: Suresh Vankadara <quic_svankada@quicinc.com>
Signed-off-by: Trishansh Bhardwaj <quic_tbhardwa@quicinc.com>
Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sc7280.dtsi | 182 +++++++++++++++++++++++++++++++++++
 1 file changed, 182 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index f8256d5a8f6b..9ac251fec262 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -4415,6 +4415,188 @@ cci1_i2c1: i2c-bus@1 {
 			};
 		};
 
+		camss: camss@acaf000 {
+			compatible = "qcom,sc7280-camss";
+
+			reg = <0x0 0x0acaf000 0x0 0x5200>,
+			      <0x0 0x0acb6000 0x0 0x5200>,
+			      <0x0 0x0acbd000 0x0 0x5200>,
+			      <0x0 0x0acc4000 0x0 0x5000>,
+			      <0x0 0x0accb000 0x0 0x5000>,
+			      <0x0 0x0ace0000 0x0 0x2000>,
+			      <0x0 0x0ace2000 0x0 0x2000>,
+			      <0x0 0x0ace4000 0x0 0x2000>,
+			      <0x0 0x0ace6000 0x0 0x2000>,
+			      <0x0 0x0ace8000 0x0 0x2000>;
+
+			reg-names = "vfe0",
+				    "vfe1",
+				    "vfe2",
+				    "vfe_lite0",
+				    "vfe_lite1",
+				    "csiphy0",
+				    "csiphy1",
+				    "csiphy2",
+				    "csiphy3",
+				    "csiphy4";
+
+			interrupts = <GIC_SPI 477 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 478 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 479 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 448 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 122 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 464 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 466 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 640 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 468 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 359 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 465 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 467 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 641 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 469 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 360 IRQ_TYPE_EDGE_RISING>;
+
+			interrupt-names = "csiphy0",
+					  "csiphy1",
+					  "csiphy2",
+					  "csiphy3",
+					  "csiphy4",
+					  "csid0",
+					  "csid1",
+					  "csid2",
+					  "csid_lite0",
+					  "csid_lite1",
+					  "vfe0",
+					  "vfe1",
+					  "vfe2",
+					  "vfe_lite0",
+					  "vfe_lite1";
+
+			power-domains = <&camcc CAM_CC_IFE_0_GDSC>,
+					<&camcc CAM_CC_IFE_1_GDSC>,
+					<&camcc CAM_CC_IFE_2_GDSC>,
+					<&camcc CAM_CC_TITAN_TOP_GDSC>;
+
+			clocks = <&gcc GCC_CAMERA_HF_AXI_CLK>,
+				 <&camcc CAM_CC_SLOW_AHB_CLK_SRC>,
+				 <&camcc CAM_CC_CPAS_AHB_CLK>,
+				 <&camcc CAM_CC_CAMNOC_AXI_CLK_SRC>,
+				 <&camcc CAM_CC_CAMNOC_AXI_CLK>,
+				 <&camcc CAM_CC_CSIPHY0_CLK>,
+				 <&camcc CAM_CC_CSI0PHYTIMER_CLK>,
+				 <&camcc CAM_CC_CSI0PHYTIMER_CLK_SRC>,
+				 <&camcc CAM_CC_CSIPHY1_CLK>,
+				 <&camcc CAM_CC_CSI1PHYTIMER_CLK>,
+				 <&camcc CAM_CC_CSI1PHYTIMER_CLK_SRC>,
+				 <&camcc CAM_CC_CSIPHY2_CLK>,
+				 <&camcc CAM_CC_CSI2PHYTIMER_CLK>,
+				 <&camcc CAM_CC_CSI2PHYTIMER_CLK_SRC>,
+				 <&camcc CAM_CC_CSIPHY3_CLK>,
+				 <&camcc CAM_CC_CSI3PHYTIMER_CLK>,
+				 <&camcc CAM_CC_CSI3PHYTIMER_CLK_SRC>,
+				 <&camcc CAM_CC_CSIPHY4_CLK>,
+				 <&camcc CAM_CC_CSI4PHYTIMER_CLK>,
+				 <&camcc CAM_CC_CSI4PHYTIMER_CLK_SRC>,
+				 <&camcc CAM_CC_IFE_0_CSID_CLK>,
+				 <&camcc CAM_CC_IFE_0_CPHY_RX_CLK>,
+				 <&camcc CAM_CC_IFE_0_CLK>,
+				 <&camcc CAM_CC_IFE_0_AXI_CLK>,
+				 <&camcc CAM_CC_CPHY_RX_CLK_SRC>,
+				 <&camcc CAM_CC_IFE_1_CSID_CLK>,
+				 <&camcc CAM_CC_IFE_1_CPHY_RX_CLK>,
+				 <&camcc CAM_CC_IFE_1_CLK>,
+				 <&camcc CAM_CC_IFE_1_AXI_CLK>,
+				 <&camcc CAM_CC_IFE_2_CSID_CLK>,
+				 <&camcc CAM_CC_IFE_2_CPHY_RX_CLK>,
+				 <&camcc CAM_CC_IFE_2_CLK>,
+				 <&camcc CAM_CC_IFE_2_AXI_CLK>,
+				 <&camcc CAM_CC_IFE_LITE_0_CSID_CLK>,
+				 <&camcc CAM_CC_IFE_LITE_0_CPHY_RX_CLK>,
+				 <&camcc CAM_CC_IFE_LITE_0_CLK>,
+				 <&camcc CAM_CC_IFE_LITE_1_CSID_CLK>,
+				 <&camcc CAM_CC_IFE_LITE_1_CPHY_RX_CLK>,
+				 <&camcc CAM_CC_IFE_LITE_1_CLK>,
+				 <&camcc CAM_CC_IFE_LITE_0_CLK>,
+				 <&camcc CAM_CC_IFE_LITE_1_CLK>;
+
+			clock-names = "cam_hf_axi",
+				      "slow_ahb_src",
+				      "cpas_ahb",
+				      "camnoc_axi_src",
+				      "camnoc_axi",
+				      "csiphy0",
+				      "csiphy0_timer",
+				      "csiphy0_timer_src",
+				      "csiphy1",
+				      "csiphy1_timer",
+				      "csiphy1_timer_src",
+				      "csiphy2",
+				      "csiphy2_timer",
+				      "csiphy2_timer_src",
+				      "csiphy3",
+				      "csiphy3_timer",
+				      "csiphy3_timer_src",
+				      "csiphy4",
+				      "csiphy4_timer",
+				      "csiphy4_timer_src",
+				      "vfe0_csid",
+				      "vfe0_cphy_rx",
+				      "vfe0",
+				      "vfe0_axi",
+				      "csiphy_rx_src",
+				      "vfe1_csid",
+				      "vfe1_cphy_rx",
+				      "vfe1",
+				      "vfe1_axi",
+				      "vfe2_csid",
+				      "vfe2_cphy_rx",
+				      "vfe2",
+				      "vfe2_axi",
+				      "vfe0_lite_csid",
+				      "vfe0_lite_cphy_rx",
+				      "vfe0_lite",
+				      "vfe1_lite_csid",
+				      "vfe1_lite_cphy_rx",
+				      "vfe1_lite",
+				      "vfe_lite0",
+				      "vfe_lite1";
+
+			iommus = <&apps_smmu 0x800 0x4e0>;
+
+			interconnect-names = "cam_ahb",
+					     "cam_hf_0";
+
+			interconnects = <&gem_noc  MASTER_APPSS_PROC 0 &cnoc2 SLAVE_CAMERA_CFG 0>,
+					<&mmss_noc MASTER_CAMNOC_HF  0 &mc_virt SLAVE_EBI1     0>;
+
+			status = "disabled";
+
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				port@0 {
+					reg = <0>;
+				};
+
+				port@1 {
+					reg = <1>;
+				};
+
+				port@2 {
+					reg = <2>;
+				};
+
+				port@3 {
+					reg = <3>;
+				};
+
+				port@4 {
+					reg = <4>;
+				};
+			};
+		};
+
 		camcc: clock-controller@ad00000 {
 			compatible = "qcom,sc7280-camcc";
 			reg = <0 0x0ad00000 0 0x10000>;

-- 
2.25.1

Re: [PATCH 2/6] arm64: dts: qcom: sc7280: Add support for camss

[Bryan O'Donoghue]

On 28/06/2024 19:32, Vikram Sharma wrote:
> +			reg = <0x0 0x0acaf000 0x0 0x5200>,
> +			      <0x0 0x0acb6000 0x0 0x5200>,
> +			      <0x0 0x0acbd000 0x0 0x5200>,
> +			      <0x0 0x0acc4000 0x0 0x5000>,
> +			      <0x0 0x0accb000 0x0 0x5000>,
> +			      <0x0 0x0ace0000 0x0 0x2000>,
> +			      <0x0 0x0ace2000 0x0 0x2000>,
> +			      <0x0 0x0ace4000 0x0 0x2000>,
> +			      <0x0 0x0ace6000 0x0 0x2000>,
> +			      <0x0 0x0ace8000 0x0 0x2000>;
> +
> +			reg-names = "vfe0",
> +				    "vfe1",
> +				    "vfe2",
> +				    "vfe_lite0",
> +				    "vfe_lite1",
> +				    "csiphy0",
> +				    "csiphy1",
> +				    "csiphy2",
> +				    "csiphy3",
> +				    "csiphy4";

Per my comment in the last patch for this series.

In V2 we should see csid and csid_lite registers defined in the dts and 
yaml, with the offset enabling code being dropped as a result.

---
bod

[PATCH 3/6] arm64: dts: qcom: sc7280: Add IMX577 camera sensor

[Vikram Sharma]

Add support for IMX577 camera sensor for SC7280 SoC.

Signed-off-by: Hariram Purushothaman <quic_hariramp@quicinc.com>
Signed-off-by: Trishansh Bhardwaj <quic_tbhardwa@quicinc.com>
Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sc7280.dtsi | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index 9ac251fec262..1c99ee09a11a 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -5167,6 +5167,39 @@ cci3_sleep: cci3-sleep-state {
 				bias-pull-down;
 			};
 
+			cam2_default: cam2-default {
+				rst {
+					pins = "gpio78"; /*cam3*/
+					function = "gpio";
+					drive-strength = <2>;
+					bias-disable;
+				};
+
+				mclk {
+					pins = "gpio67"; /*cam3*/
+					function = "cam_mclk";
+					drive-strength = <2>; /*RB5 was 16 and i changed to 2 here*/
+					bias-disable;
+				};
+			};
+
+			cam2_suspend: cam2-suspend {
+				rst {
+					pins = "gpio78"; /*cam3*/
+					function = "gpio";
+					drive-strength = <2>;
+					bias-pull-down;
+					output-low;
+				};
+
+				mclk {
+					pins = "gpio67"; /*cam3*/
+					function = "cam_mclk";
+					drive-strength = <2>;
+					bias-pull-down;
+				};
+			};
+
 			dp_hot_plug_det: dp-hot-plug-det-state {
 				pins = "gpio47";
 				function = "dp_hot";

-- 
2.25.1

Re: [PATCH 3/6] arm64: dts: qcom: sc7280: Add IMX577 camera sensor

[Bryan O'Donoghue]

On 28/06/2024 19:32, Vikram Sharma wrote:
> Add support for IMX577 camera sensor for SC7280 SoC.
> 
> Signed-off-by: Hariram Purushothaman <quic_hariramp@quicinc.com>
> Signed-off-by: Trishansh Bhardwaj <quic_tbhardwa@quicinc.com>
> Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com>
> ---
>   arch/arm64/boot/dts/qcom/sc7280.dtsi | 33 +++++++++++++++++++++++++++++++++
>   1 file changed, 33 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index 9ac251fec262..1c99ee09a11a 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -5167,6 +5167,39 @@ cci3_sleep: cci3-sleep-state {
>   				bias-pull-down;
>   			};
>   
> +			cam2_default: cam2-default {
> +				rst {
> +					pins = "gpio78"; /*cam3*/

I don't think the /* cam3 */ adds much here TBH.

> +					function = "gpio";
> +					drive-strength = <2>;
> +					bias-disable;
> +				};
> +
> +				mclk {
> +					pins = "gpio67"; /*cam3*/
> +					function = "cam_mclk";
> +					drive-strength = <2>; /*RB5 was 16 and i changed to 2 here*/

You can drop that comment too, actually more saliently, what are you 
changing from 16 to 2 since its being mentioned ?

---
bod

Re: [PATCH 3/6] arm64: dts: qcom: sc7280: Add IMX577 camera sensor

[Konrad Dybcio]

On 28.06.2024 8:32 PM, Vikram Sharma wrote:
> Add support for IMX577 camera sensor for SC7280 SoC.
> 
> Signed-off-by: Hariram Purushothaman <quic_hariramp@quicinc.com>
> Signed-off-by: Trishansh Bhardwaj <quic_tbhardwa@quicinc.com>
> Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sc7280.dtsi | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index 9ac251fec262..1c99ee09a11a 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -5167,6 +5167,39 @@ cci3_sleep: cci3-sleep-state {
>  				bias-pull-down;
>  			};
>  
> +			cam2_default: cam2-default {
> +				rst {
> +					pins = "gpio78"; /*cam3*/

You can drop these comments.. the node name and label suggest this is
cam*2* anyway

> +					function = "gpio";
> +					drive-strength = <2>;
> +					bias-disable;
> +				};
> +
> +				mclk {
> +					pins = "gpio67"; /*cam3*/
> +					function = "cam_mclk";
> +					drive-strength = <2>; /*RB5 was 16 and i changed to 2 here*/

/* why? */

Konrad

Re: [PATCH 3/6] arm64: dts: qcom: sc7280: Add IMX577 camera sensor

[Krzysztof Kozlowski]

On 28/06/2024 20:32, Vikram Sharma wrote:
> Add support for IMX577 camera sensor for SC7280 SoC.
> 
> Signed-off-by: Hariram Purushothaman <quic_hariramp@quicinc.com>
> Signed-off-by: Trishansh Bhardwaj <quic_tbhardwa@quicinc.com>
> Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sc7280.dtsi | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index 9ac251fec262..1c99ee09a11a 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -5167,6 +5167,39 @@ cci3_sleep: cci3-sleep-state {
>  				bias-pull-down;
>  			};
>  
> +			cam2_default: cam2-default {

There are tools. Use them, instead of us. Read you internal guideline -
it asks you to do that.

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).



Best regards,
Krzysztof

Re: [PATCH 3/6] arm64: dts: qcom: sc7280: Add IMX577 camera sensor

[Dmitry Baryshkov]

On Sat, Jun 29, 2024 at 12:02:37AM GMT, Vikram Sharma wrote:
> Add support for IMX577 camera sensor for SC7280 SoC.

Note, the subject and commit description do not match patch contents.

> 
> Signed-off-by: Hariram Purushothaman <quic_hariramp@quicinc.com>
> Signed-off-by: Trishansh Bhardwaj <quic_tbhardwa@quicinc.com>
> Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sc7280.dtsi | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index 9ac251fec262..1c99ee09a11a 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -5167,6 +5167,39 @@ cci3_sleep: cci3-sleep-state {
>  				bias-pull-down;
>  			};
>  
> +			cam2_default: cam2-default {
> +				rst {
> +					pins = "gpio78"; /*cam3*/
> +					function = "gpio";
> +					drive-strength = <2>;
> +					bias-disable;
> +				};
> +
> +				mclk {
> +					pins = "gpio67"; /*cam3*/
> +					function = "cam_mclk";
> +					drive-strength = <2>; /*RB5 was 16 and i changed to 2 here*/
> +					bias-disable;
> +				};
> +			};
> +
> +			cam2_suspend: cam2-suspend {
> +				rst {
> +					pins = "gpio78"; /*cam3*/
> +					function = "gpio";
> +					drive-strength = <2>;
> +					bias-pull-down;
> +					output-low;
> +				};
> +
> +				mclk {
> +					pins = "gpio67"; /*cam3*/
> +					function = "cam_mclk";
> +					drive-strength = <2>;
> +					bias-pull-down;
> +				};
> +			};
> +
>  			dp_hot_plug_det: dp-hot-plug-det-state {
>  				pins = "gpio47";
>  				function = "dp_hot";
> 
> -- 
> 2.25.1
> 

-- 
With best wishes
Dmitry

[PATCH 4/6] arm64: dts: qcom: qcs6490-rb3gen2: Enable IMX577 camera sensor

[Vikram Sharma]

Enable IMX577 camera sensor for qcs6490-rb3gen2.

Signed-off-by: Hariram Purushothaman <quic_hariramp@quicinc.com>
Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com>
---
 arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts | 67 ++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
index c4cde4328e3d..237231600dca 100644
--- a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
+++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
@@ -513,6 +513,73 @@ vreg_bob_3p296: bob {
 	};
 };
 
+&camcc {
+	status = "okay";
+};
+
+&camss {
+	status = "disabled";
+	ports {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		/* The port index denotes CSIPHY id i.e. csiphy2 */
+		port@3 {
+			reg = <3>;
+			csiphy3_ep: endpoint {
+				clock-lanes = <7>;
+				data-lanes = <0 1 2 3>;
+				remote-endpoint = <&imx412_ep>;
+			};
+		};
+	};
+};
+
+&cci0 {
+	status = "okay";
+};
+
+&cci1 {
+	status = "okay";
+};
+
+&cci1_i2c1 {
+	camera@1a {
+		/*
+		 * rb3gen2 ships with an imx577. qcom treats imx412
+		 * and imx577 the same way. Absent better data do the same here.
+		 */
+		compatible = "sony,imx412";
+		reg = <0x1a>;
+
+		reset-gpios = <&tlmm 78 GPIO_ACTIVE_LOW>;
+		pinctrl-names = "default", "suspend";
+		pinctrl-0 = <&cam2_default>;
+		pinctrl-1 = <&cam2_suspend>;
+
+		clocks = <&camcc CAM_CC_MCLK3_CLK>,
+				 <&camcc CAM_CC_MCLK2_CLK>;
+		assigned-clocks = <&camcc CAM_CC_MCLK3_CLK>,
+				 <&camcc CAM_CC_MCLK2_CLK>;
+		assigned-clock-rates = <24000000>,
+							   <24000000>;
+
+		dovdd-supply  = <&vreg_l18b_1p8>;
+		/* avdd-supply = <&vdc_5v>;
+		 * dvdd-supply = <&vdc_5v>;
+		 */
+
+		port {
+			imx412_ep: endpoint {
+				clock-lanes = <7>;
+				link-frequencies = /bits/ 64 <600000000>;
+				data-lanes = <0 1 2 3>;
+				remote-endpoint = <&csiphy3_ep>;
+			};
+		};
+	};
+};
+
 &gcc {
 	protected-clocks = <GCC_CFG_NOC_LPASS_CLK>,
 			   <GCC_MSS_CFG_AHB_CLK>,

-- 
2.25.1

Re: [PATCH 4/6] arm64: dts: qcom: qcs6490-rb3gen2: Enable IMX577 camera sensor

[Bryan O'Donoghue]

On 28/06/2024 19:32, Vikram Sharma wrote:
> Enable IMX577 camera sensor for qcs6490-rb3gen2.
> 
> Signed-off-by: Hariram Purushothaman <quic_hariramp@quicinc.com>
> Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com>
> ---
>   arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts | 67 ++++++++++++++++++++++++++++
>   1 file changed, 67 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> index c4cde4328e3d..237231600dca 100644
> --- a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> +++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts

I believe the rb3gen2 can be sold with and without the camera mezzanine 
[1], so the approach we have taken for a similar situation on rb5 was to 
have a separate mezzanine dts which includes the baseboard and extends it.


arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
arch/arm64/boot/dts/qcom/qrb5165-rb5-vision-mezzanine.dts

Please replicate that model here.

[1] https://www.thundercomm.com/product/qualcomm-rb3-gen-2/#versions

> @@ -513,6 +513,73 @@ vreg_bob_3p296: bob {
>   	};
>   };
>   
> +&camcc {
> +	status = "okay";
> +};
> +
> +&camss {
> +	status = "disabled";

once you move this into its own dts the camera should be enabled by 
default for that .dtb

> +	ports {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		/* The port index denotes CSIPHY id i.e. csiphy2 */
> +		port@3 {
> +			reg = <3>;

copy/past splat from the rb5

port@3 means csiphy3

> +			csiphy3_ep: endpoint {
> +				clock-lanes = <7>;
> +				data-lanes = <0 1 2 3>;
> +				remote-endpoint = <&imx412_ep>;
> +			};
> +		};
> +	};
> +};
> +
> +&cci0 {
> +	status = "okay";
> +};

You're enabling cci0 here but not the sensor for it - presumably a 
monochrome sensor attached to one of the other CSIPHYs.

Zap cci0 here until you add that other sensor.

> +
> +&cci1 {
> +	status = "okay";
> +};
> +
> +&cci1_i2c1 {
> +	camera@1a {
> +		/*
> +		 * rb3gen2 ships with an imx577. qcom treats imx412
> +		 * and imx577 the same way. Absent better data do the same here.
> +		 */
> +		compatible = "sony,imx412";
> +		reg = <0x1a>;

The commit log says imx577 but the comapt string says imx412.

Choose which one and maintain the namespace. Its an imx577 right ? 
Upstream kernel has the relevant compat string

Commit: 1251663220d9 ("media: i2c: imx412: Add new compatible strings")

You can just cherry-pick that commit to your kernel and then the 
upstream dts and downstream dts will be compatible.


> +
> +		reset-gpios = <&tlmm 78 GPIO_ACTIVE_LOW>;
> +		pinctrl-names = "default", "suspend";
> +		pinctrl-0 = <&cam2_default>;
> +		pinctrl-1 = <&cam2_suspend>;
> +
> +		clocks = <&camcc CAM_CC_MCLK3_CLK>,
> +				 <&camcc CAM_CC_MCLK2_CLK>;
> +		assigned-clocks = <&camcc CAM_CC_MCLK3_CLK>,
> +				 <&camcc CAM_CC_MCLK2_CLK>;

This looks funny - why do you have MCLK2 ?

One final thing, you appear to be missing some power rails here no ?

e.g. rb5

         vdda-phy-supply = <&vreg_l5a_0p88>;
         vdda-pll-supply = <&vreg_l9a_1p2>;

---
bod

Re: [PATCH 4/6] arm64: dts: qcom: qcs6490-rb3gen2: Enable IMX577 camera sensor

[Krzysztof Kozlowski]

On 28/06/2024 20:32, Vikram Sharma wrote:
> Enable IMX577 camera sensor for qcs6490-rb3gen2.
> 
> Signed-off-by: Hariram Purushothaman <quic_hariramp@quicinc.com>
> Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts | 67 ++++++++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> index c4cde4328e3d..237231600dca 100644
> --- a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> +++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> @@ -513,6 +513,73 @@ vreg_bob_3p296: bob {
>  	};
>  };
>  
> +&camcc {
> +	status = "okay";
> +};
> +
> +&camss {
> +	status = "disabled";
> +	ports {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		/* The port index denotes CSIPHY id i.e. csiphy2 */
> +		port@3 {
> +			reg = <3>;
> +			csiphy3_ep: endpoint {
> +				clock-lanes = <7>;
> +				data-lanes = <0 1 2 3>;
> +				remote-endpoint = <&imx412_ep>;
> +			};
> +		};
> +	};
> +};
> +
> +&cci0 {
> +	status = "okay";
> +};
> +
> +&cci1 {
> +	status = "okay";
> +};
> +
> +&cci1_i2c1 {
> +	camera@1a {
> +		/*
> +		 * rb3gen2 ships with an imx577. qcom treats imx412
> +		 * and imx577 the same way. Absent better data do the same here.

NAK, how qcom treats incorrect code, is not an correct argument.

Use correct compatibles and correct bindings.

> +		 */
> +		compatible = "sony,imx412";
> +		reg = <0x1a>;
> +
> +		reset-gpios = <&tlmm 78 GPIO_ACTIVE_LOW>;
> +		pinctrl-names = "default", "suspend";
> +		pinctrl-0 = <&cam2_default>;
> +		pinctrl-1 = <&cam2_suspend>;
> +
> +		clocks = <&camcc CAM_CC_MCLK3_CLK>,
> +				 <&camcc CAM_CC_MCLK2_CLK>;
> +		assigned-clocks = <&camcc CAM_CC_MCLK3_CLK>,
> +				 <&camcc CAM_CC_MCLK2_CLK>;
> +		assigned-clock-rates = <24000000>,
> +							   <24000000>;
> +
> +		dovdd-supply  = <&vreg_l18b_1p8>;
> +		/* avdd-supply = <&vdc_5v>;
> +		 * dvdd-supply = <&vdc_5v>;

What's this?



Best regards,
Krzysztof

[PATCH 5/6] i2c: Enable IMX577 camera sensor for qcm6490

[Vikram Sharma]

This change enables IMX577 sensor driver for qcm6490.

Signed-off-by: Hariram Purushothaman <quic_hariramp@quicinc.com>
Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com>
---
 drivers/i2c/busses/i2c-qcom-cci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
index 414882c57d7f..10e6df566ae3 100644
--- a/drivers/i2c/busses/i2c-qcom-cci.c
+++ b/drivers/i2c/busses/i2c-qcom-cci.c
@@ -817,6 +817,7 @@ static const struct of_device_id cci_dt_match[] = {
 	 * Do not add any new ones unless they introduce a new config
 	 */
 	{ .compatible = "qcom,msm8916-cci", .data = &cci_v1_data},
+	{ .compatible = "qcom,sc7280-cci", .data = &cci_v2_data},
 	{ .compatible = "qcom,sdm845-cci", .data = &cci_v2_data},
 	{ .compatible = "qcom,sm8250-cci", .data = &cci_v2_data},
 	{ .compatible = "qcom,sm8450-cci", .data = &cci_v2_data},

-- 
2.25.1

Re: [PATCH 5/6] i2c: Enable IMX577 camera sensor for qcm6490

[Luca Weiss]

On Freitag, 28. Juni 2024 20:32:39 MESZ Vikram Sharma wrote:
> This change enables IMX577 sensor driver for qcm6490.
> 
> Signed-off-by: Hariram Purushothaman <quic_hariramp@quicinc.com>
> Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com>
> ---
>  drivers/i2c/busses/i2c-qcom-cci.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> index 414882c57d7f..10e6df566ae3 100644
> --- a/drivers/i2c/busses/i2c-qcom-cci.c
> +++ b/drivers/i2c/busses/i2c-qcom-cci.c
> @@ -817,6 +817,7 @@ static const struct of_device_id cci_dt_match[] = {
>  	 * Do not add any new ones unless they introduce a new config
>  	 */
>  	{ .compatible = "qcom,msm8916-cci", .data = &cci_v1_data},
> +	{ .compatible = "qcom,sc7280-cci", .data = &cci_v2_data},

Please read the comment above qcom,msm8916-cci.

And sc7280.dtsi already uses

  compatible = "qcom,sc7280-cci", "qcom,msm8996-cci";

So qcom,msm8996-cci with the same match data (cci_v2_data) gets used, so
just drop this patch.

Regards
Luca

>  	{ .compatible = "qcom,sdm845-cci", .data = &cci_v2_data},
>  	{ .compatible = "qcom,sm8250-cci", .data = &cci_v2_data},
>  	{ .compatible = "qcom,sm8450-cci", .data = &cci_v2_data},
> 
> 

Re: [PATCH 5/6] i2c: Enable IMX577 camera sensor for qcm6490

[Krzysztof Kozlowski]

On 29/06/2024 10:22, Luca Weiss wrote:
> On Freitag, 28. Juni 2024 20:32:39 MESZ Vikram Sharma wrote:
>> This change enables IMX577 sensor driver for qcm6490.
>>
>> Signed-off-by: Hariram Purushothaman <quic_hariramp@quicinc.com>
>> Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com>
>> ---
>>  drivers/i2c/busses/i2c-qcom-cci.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
>> index 414882c57d7f..10e6df566ae3 100644
>> --- a/drivers/i2c/busses/i2c-qcom-cci.c
>> +++ b/drivers/i2c/busses/i2c-qcom-cci.c
>> @@ -817,6 +817,7 @@ static const struct of_device_id cci_dt_match[] = {
>>  	 * Do not add any new ones unless they introduce a new config
>>  	 */
>>  	{ .compatible = "qcom,msm8916-cci", .data = &cci_v1_data},
>> +	{ .compatible = "qcom,sc7280-cci", .data = &cci_v2_data},
> 
> Please read the comment above qcom,msm8916-cci.
> 
> And sc7280.dtsi already uses
> 
>   compatible = "qcom,sc7280-cci", "qcom,msm8996-cci";
> 
> So qcom,msm8996-cci with the same match data (cci_v2_data) gets used, so
> just drop this patch.
> 

I think we put quite obvious comment, yet it is ignored.

Any ideas how to change the comment so people will read it?

Best regards,
Krzysztof

Re: [PATCH 5/6] i2c: Enable IMX577 camera sensor for qcm6490

[Krzysztof Kozlowski]

On 28/06/2024 20:32, Vikram Sharma wrote:
> This change enables IMX577 sensor driver for qcm6490.
> 
> Signed-off-by: Hariram Purushothaman <quic_hariramp@quicinc.com>
> Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com>
> ---
>  drivers/i2c/busses/i2c-qcom-cci.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> index 414882c57d7f..10e6df566ae3 100644
> --- a/drivers/i2c/busses/i2c-qcom-cci.c
> +++ b/drivers/i2c/busses/i2c-qcom-cci.c
> @@ -817,6 +817,7 @@ static const struct of_device_id cci_dt_match[] = {
>  	 * Do not add any new ones unless they introduce a new config
>  	 */
>  	{ .compatible = "qcom,msm8916-cci", .data = &cci_v1_data},
> +	{ .compatible = "qcom,sc7280-cci", .data = &cci_v2_data},

NAK, ridiculous.

Best regards,
Krzysztof

[PATCH 6/6] media: qcom: camss: support for camss driver for sc7280

[Vikram Sharma]

From: Suresh Vankadara <quic_svankada@quicinc.com>

This change adds support for camss driver for sc7280 soc.

Signed-off-by: Suresh Vankadara <quic_svankada@quicinc.com>
Signed-off-by: Trishansh Bhardwaj <quic_tbhardwa@quicinc.com>
Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com>
---
 drivers/media/platform/qcom/camss/camss-csid.c     |  16 +-
 .../platform/qcom/camss/camss-csiphy-3ph-1-0.c     |   2 +
 drivers/media/platform/qcom/camss/camss-vfe.c      |   2 +
 drivers/media/platform/qcom/camss/camss.c          | 340 +++++++++++++++++++++
 drivers/media/platform/qcom/camss/camss.h          |   2 +
 5 files changed, 359 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c
index 858db5d4ca75..2c622233da6f 100644
--- a/drivers/media/platform/qcom/camss/camss-csid.c
+++ b/drivers/media/platform/qcom/camss/camss-csid.c
@@ -28,6 +28,7 @@
 /* offset of CSID registers in VFE region for VFE 480 */
 #define VFE_480_CSID_OFFSET 0x1200
 #define VFE_480_LITE_CSID_OFFSET 0x200
+#define VFE_165_CSID_OFFSET 0x4000
 
 #define MSM_CSID_NAME "msm_csid"
 
@@ -1028,8 +1029,8 @@ int msm_csid_subdev_init(struct camss *camss, struct csid_device *csid,
 	csid->res->hw_ops->subdev_init(csid);
 
 	/* Memory */
-
-	if (camss->res->version == CAMSS_8250) {
+	switch (camss->res->version) {
+	case CAMSS_8250:
 		/* for titan 480, CSID registers are inside the VFE region,
 		 * between the VFE "top" and "bus" registers. this requires
 		 * VFE to be initialized before CSID
@@ -1040,10 +1041,19 @@ int msm_csid_subdev_init(struct camss *camss, struct csid_device *csid,
 		else
 			csid->base = csid->res->parent_dev_ops->get_base_address(camss, id)
 				 + VFE_480_CSID_OFFSET;
-	} else {
+		break;
+	case CAMSS_7280:
+		/* for titan 165, CSID registers are inside the VFE region,
+		 * between the VFE "top" and "bus" registers. this requires
+		 * VFE to be initialized before CSID
+		 */
+		csid->base = camss->vfe[id].base + VFE_165_CSID_OFFSET;
+		break;
+	default:
 		csid->base = devm_platform_ioremap_resource_byname(pdev, res->reg[0]);
 		if (IS_ERR(csid->base))
 			return PTR_ERR(csid->base);
+		break;
 	}
 
 	/* Interrupt */
diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
index df7e93a5a4f6..c7e507420732 100644
--- a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
+++ b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
@@ -510,6 +510,7 @@ static void csiphy_gen2_config_lanes(struct csiphy_device *csiphy,
 		array_size = ARRAY_SIZE(lane_regs_sdm845[0]);
 		break;
 	case CAMSS_8250:
+	case CAMSS_7280:
 		r = &lane_regs_sm8250[0][0];
 		array_size = ARRAY_SIZE(lane_regs_sm8250[0]);
 		break;
@@ -560,6 +561,7 @@ static bool csiphy_is_gen2(u32 version)
 	case CAMSS_845:
 	case CAMSS_8250:
 	case CAMSS_8280XP:
+	case CAMSS_7280:
 		ret = true;
 		break;
 	}
diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
index 83c5a36d071f..757e872b8eb8 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe.c
@@ -338,6 +338,7 @@ static u32 vfe_src_pad_code(struct vfe_line *line, u32 sink_code,
 	case CAMSS_845:
 	case CAMSS_8250:
 	case CAMSS_8280XP:
+	case CAMSS_7280:
 		switch (sink_code) {
 		case MEDIA_BUS_FMT_YUYV8_1X16:
 		{
@@ -1695,6 +1696,7 @@ static int vfe_bpl_align(struct vfe_device *vfe)
 	case CAMSS_845:
 	case CAMSS_8250:
 	case CAMSS_8280XP:
+	case CAMSS_7280:
 		ret = 16;
 		break;
 	default:
diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index 1f1f44f6fbb2..2b840e2aeb51 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -1480,6 +1480,281 @@ static const struct resources_icc icc_res_sc8280xp[] = {
 	},
 };
 
+static const struct camss_subdev_resources csiphy_res_7280[] = {
+	/* CSIPHY0 */
+	{
+		.regulators = {},
+		.clock = { "csiphy0", "csiphy0_timer", "csiphy0_timer_src"},
+		.clock_rate = { { 300000000 },
+				{ 300000000 },
+				{ 300000000 }},
+		.reg = { "csiphy0" },
+		.interrupt = { "csiphy0" },
+		.csiphy = {
+			.hw_ops = &csiphy_ops_3ph_1_0
+		}
+	},
+	/* CSIPHY1 */
+	{
+		.regulators = {},
+		.clock = { "csiphy1", "csiphy1_timer", "csiphy1_timer_src"},
+		.clock_rate = { { 300000000 },
+				{ 300000000 },
+				{ 300000000 }},
+		.reg = { "csiphy1" },
+		.interrupt = { "csiphy1" },
+		.csiphy = {
+			.hw_ops = &csiphy_ops_3ph_1_0
+		}
+	},
+	/* CSIPHY2 */
+	{
+		.regulators = {},
+		.clock = { "csiphy2", "csiphy2_timer", "csiphy2_timer_src"},
+		.clock_rate = { { 300000000 },
+				{ 300000000 },
+				{ 300000000 }},
+		.reg = { "csiphy2" },
+		.interrupt = { "csiphy2" },
+		.csiphy = {
+			.hw_ops = &csiphy_ops_3ph_1_0
+		}
+	},
+	/* CSIPHY3 */
+	{
+		.regulators = {},
+		.clock = { "csiphy3", "csiphy3_timer", "csiphy3_timer_src"},
+		.clock_rate = { { 300000000 },
+				{ 300000000 },
+				{ 300000000 }},
+		.reg = { "csiphy3" },
+		.interrupt = { "csiphy3" },
+		.csiphy = {
+			.hw_ops = &csiphy_ops_3ph_1_0
+		}
+	},
+	/* CSIPHY4 */
+	{
+		.regulators = {},
+		.clock = { "csiphy4", "csiphy4_timer", "csiphy4_timer_src"},
+		.clock_rate = { { 300000000 },
+				{ 300000000 },
+				{ 300000000 }},
+		.reg = { "csiphy4" },
+		.interrupt = { "csiphy4" },
+		.csiphy = {
+			.hw_ops = &csiphy_ops_3ph_1_0
+		}
+	},
+};
+
+static const struct camss_subdev_resources csid_res_7280[] = {
+	/* CSID0 */
+	{
+		.regulators = { "vdda-phy", "vdda-pll" },
+		.clock = { "vfe0_csid", "vfe0_cphy_rx", "vfe0", "vfe0_axi", "csiphy_rx_src"},
+		.clock_rate = { { 300000000, 0, 380000000, 150000000, 380000000},
+				{ 400000000, 0, 510000000, 140000000, 510000000},
+				{ 400000000, 0, 637000000, 320000000, 637000000},
+				{ 400000000, 0, 760000000, 400000000, 760000000} },
+		.reg = { "csid0" },
+		.interrupt = { "csid0" },
+		.csid = {
+			.is_lite = false,
+			.hw_ops = &csid_ops_gen2,
+			.formats = &csid_formats_gen2
+		}
+	},
+	/* CSID1 */
+	{
+		.regulators = { "vdda-phy", "vdda-pll" },
+		.clock = { "vfe1_csid", "vfe1_cphy_rx", "vfe1", "vfe1_axi", "csiphy_rx_src"},
+		.clock_rate = { { 300000000, 0, 380000000, 300000000, 380000000},
+				{ 400000000, 0, 510000000, 400000000, 510000000},
+				{ 400000000, 0, 637000000, 400000000, 637000000},
+				{ 400000000, 0, 760000000, 400000000, 760000000} },
+		.reg = { "csid1" },
+		.interrupt = { "csid1" },
+		.csid = {
+			.is_lite = false,
+			.hw_ops = &csid_ops_gen2,
+			.formats = &csid_formats_gen2
+		}
+	},
+	/* CSID2 */
+	{
+		.regulators = { "vdda-phy", "vdda-pll" },
+		.clock = { "vfe2_csid", "vfe2_cphy_rx", "vfe2", "vfe2_axi", "csiphy_rx_src"},
+		.clock_rate = { { 300000000, 0, 380000000, 300000000, 380000000},
+				{ 400000000, 0, 510000000, 400000000, 510000000},
+				{ 400000000, 0, 637000000, 400000000, 637000000},
+				{ 400000000, 0, 760000000, 400000000, 760000000} },
+		.reg = { "csid2" },
+		.interrupt = { "csid2" },
+		.csid = {
+			.is_lite = false,
+			.hw_ops = &csid_ops_gen2,
+			.formats = &csid_formats_gen2
+		}
+	},
+	/* CSID3 */
+	{
+		.regulators = { "vdda-phy", "vdda-pll" },
+		.clock = { "vfe0_lite_csid", "vfe0_lite_cphy_rx", "vfe0_lite", "csiphy_rx_src"},
+		.clock_rate = { { 300000000, 0, 320000000, 300000000},
+				{ 400000000, 0, 400000000, 400000000},
+				{ 400000000, 0, 480000000, 400000000},
+				{ 400000000, 0, 600000000, 400000000} },
+		.reg = { "csid_lite0" },
+		.interrupt = { "csid_lite0" },
+		.csid = {
+			.is_lite = true,
+			.hw_ops = &csid_ops_gen2,
+			.formats = &csid_formats_gen2
+		}
+	},
+	/* CSID4 */
+	{
+		.regulators = { "vdda-phy", "vdda-pll" },
+		.clock = { "vfe1_lite_csid", "vfe1_lite_cphy_rx", "vfe1_lite", "csiphy_rx_src"},
+		.clock_rate = { { 300000000, 0, 320000000, 300000000},
+				{ 400000000, 0, 400000000, 400000000},
+				{ 400000000, 0, 480000000, 400000000},
+				{ 400000000, 0, 600000000, 400000000} },
+		.reg = { "csid_lite1" },
+		.interrupt = { "csid_lite1" },
+		.csid = {
+			.is_lite = true,
+			.hw_ops = &csid_ops_gen2,
+			.formats = &csid_formats_gen2
+		}
+	},
+};
+
+static const struct camss_subdev_resources vfe_res_7280[] = {
+	/* VFE0 */
+	{
+		.regulators = {},
+		.clock = { "vfe0", "vfe0_axi", "cam_hf_axi",
+			"slow_ahb_src", "cpas_ahb", "camnoc_axi"},
+		.clock_rate = {
+				{ 380000000, 0, 0, 80000000, 0, 150000000},
+				{ 380000000, 0, 0, 80000000, 0, 150000000},
+				{ 510000000, 0, 0, 80000000, 0, 240000000},
+				{ 637000000, 0, 0, 80000000, 0, 320000000},
+				{ 760000000, 0, 0, 80000000, 0, 400000000},
+				{ 760000000, 0, 0, 80000000, 0, 480000000} },
+		.reg = { "vfe0" },
+		.interrupt = { "vfe0" },
+		.vfe = {
+			.line_num = 3,
+			.is_lite = false,
+			.hw_ops = &vfe_ops_170,
+			.formats_rdi = &vfe_formats_rdi_845,
+			.formats_pix = &vfe_formats_pix_845
+		}
+	},
+	/* VFE1 */
+	{
+		.regulators = {},
+		.clock = { "vfe1", "vfe1_axi", "cam_hf_axi",
+			"slow_ahb_src", "cpas_ahb", "camnoc_axi"},
+		.clock_rate = {
+				{ 380000000, 0, 0, 80000000, 0, 150000000},
+				{ 380000000, 0, 0, 80000000, 0, 150000000 },
+				{ 510000000, 0, 0, 80000000, 0, 240000000 },
+				{ 637000000, 0, 0, 80000000, 0, 320000000 },
+				{ 760000000, 0, 0, 80000000, 0, 400000000 },
+				{ 760000000, 0, 0, 80000000, 0, 480000000 } },
+		.reg = { "vfe1" },
+		.interrupt = { "vfe1" },
+		.vfe = {
+			.line_num = 3,
+			.is_lite = false,
+			.hw_ops = &vfe_ops_170,
+			.formats_rdi = &vfe_formats_rdi_845,
+			.formats_pix = &vfe_formats_pix_845
+		}
+	},
+	/* VFE2 */
+	{
+		.regulators = {},
+		.clock = { "vfe2", "vfe2_axi", "cam_hf_axi",
+			"slow_ahb_src", "cpas_ahb", "camnoc_axi"},
+		.clock_rate = {
+				{ 380000000, 0, 0, 80000000, 0, 150000000},
+				{ 380000000, 0, 0, 80000000, 0, 150000000, },
+				{ 510000000, 0, 0, 80000000, 0, 240000000, },
+				{ 637000000, 0, 0, 80000000, 0, 320000000, },
+				{ 760000000, 0, 0, 80000000, 0, 400000000, },
+				{ 760000000, 0, 0, 80000000, 0, 480000000, } },
+		.reg = { "vfe2" },
+		.interrupt = { "vfe2" },
+		.vfe = {
+			.line_num = 3,
+			.is_lite = false,
+			.hw_ops = &vfe_ops_170,
+			.formats_rdi = &vfe_formats_rdi_845,
+			.formats_pix = &vfe_formats_pix_845
+		}
+	},
+	/* VFE3 (lite) */
+	{
+		.clock = { "vfe_lite0", "cam_hf_axi", "slow_ahb_src", "cpas_ahb", "camnoc_axi"},
+		.clock_rate = {
+				{ 320000000, 0, 80000000, 0, 150000000 },
+				{ 320000000, 0, 80000000, 0, 150000000 },
+				{ 400000000, 0, 80000000, 0, 240000000 },
+				{ 480000000, 0, 80000000, 0, 320000000 },
+				{ 600000000, 0, 80000000, 0, 400000000 },
+				{ 600000000, 0, 80000000, 0, 480000000 } },
+		.regulators = {},
+		.reg = { "vfe_lite0" },
+		.interrupt = { "vfe_lite0" },
+		.vfe = {
+			.line_num = 4,
+			.is_lite = true,
+			.hw_ops = &vfe_ops_170,
+			.formats_rdi = &vfe_formats_rdi_845,
+			.formats_pix = &vfe_formats_pix_845
+		}
+	},
+	/* VFE4 (lite) */
+	{
+		.clock = { "vfe_lite1", "cam_hf_axi", "slow_ahb_src", "cpas_ahb", "camnoc_axi"},
+		.clock_rate = {
+				{ 320000000, 0, 80000000, 0, 150000000 },
+				{ 320000000, 0, 80000000, 0, 150000000 },
+				{ 400000000, 0, 80000000, 0, 240000000 },
+				{ 480000000, 0, 80000000, 0, 320000000 },
+				{ 600000000, 0, 80000000, 0, 400000000 },
+				{ 600000000, 0, 80000000, 0, 480000000 } },
+		.regulators = {},
+		.reg = { "vfe_lite1" },
+		.interrupt = { "vfe_lite1" },
+		.vfe = {
+			.line_num = 4,
+			.is_lite = true,
+			.hw_ops = &vfe_ops_170,
+			.formats_rdi = &vfe_formats_rdi_845,
+			.formats_pix = &vfe_formats_pix_845
+		}
+	},
+};
+
+static const struct resources_icc icc_res_sc7280[] = {
+	{
+		.name = "cam_ahb",
+		.icc_bw_tbl.avg = 38400,
+		.icc_bw_tbl.peak = 76800,
+	},
+	{
+		.name = "cam_hf_0",
+		.icc_bw_tbl.avg = 2097152,
+		.icc_bw_tbl.peak = 2097152,
+	},
+};
+
 /*
  * camss_add_clock_margin - Add margin to clock frequency rate
  * @rate: Clock frequency rate
@@ -1824,6 +2099,57 @@ static int camss_init_subdevices(struct camss *camss)
 	return 0;
 }
 
+/*
+ * camss_link_entities_v2 - Register subdev nodes and create links
+ * @camss: CAMSS device
+ *
+ * Return 0 on success or a negative error code on failure
+ */
+static int camss_link_entities_v2(struct camss *camss)
+{
+	int i, j;
+	int ret;
+
+	for (i = 0; i < camss->res->csiphy_num; i++) {
+		for (j = 0; j < camss->res->csid_num; j++) {
+			ret = media_create_pad_link(&camss->csiphy[i].subdev.entity,
+						    MSM_CSIPHY_PAD_SRC,
+						    &camss->csid[j].subdev.entity,
+						    MSM_CSID_PAD_SINK,
+						    0);
+			if (ret < 0) {
+				dev_err(camss->dev,
+					"Failed to link %s->%s entities: %d\n",
+					camss->csiphy[i].subdev.entity.name,
+					camss->csid[j].subdev.entity.name,
+					ret);
+				return ret;
+			}
+		}
+	}
+
+	for (i = 0; i < camss->res->csid_num; i++)
+		for (j = 0; j < camss->vfe[i].res->line_num; j++) {
+			struct v4l2_subdev *csid = &camss->csid[i].subdev;
+			struct v4l2_subdev *vfe = &camss->vfe[i].line[j].subdev;
+
+			ret = media_create_pad_link(&csid->entity,
+						    MSM_CSID_PAD_FIRST_SRC + j,
+						    &vfe->entity,
+						    MSM_VFE_PAD_SINK,
+						    0);
+			if (ret < 0) {
+				dev_err(camss->dev,
+					"Failed to link %s->%s entities: %d\n",
+					csid->entity.name,
+					vfe->entity.name,
+					ret);
+				return ret;
+			}
+		}
+	return 0;
+}
+
 /*
  * camss_link_entities - Register subdev nodes and create links
  * @camss: CAMSS device
@@ -2440,12 +2766,26 @@ static const struct camss_resources sc8280xp_resources = {
 	.link_entities = camss_link_entities
 };
 
+static const struct camss_resources sc7280_resources = {
+	.version = CAMSS_7280,
+	.csiphy_res = csiphy_res_7280,
+	.csid_res = csid_res_7280,
+	.vfe_res = vfe_res_7280,
+	.icc_res = icc_res_sc7280,
+	.icc_path_num = ARRAY_SIZE(icc_res_sc7280),
+	.csiphy_num = ARRAY_SIZE(csiphy_res_7280),
+	.csid_num = ARRAY_SIZE(csid_res_7280),
+	.vfe_num = 3,
+	.link_entities = camss_link_entities_v2
+};
+
 static const struct of_device_id camss_dt_match[] = {
 	{ .compatible = "qcom,msm8916-camss", .data = &msm8916_resources },
 	{ .compatible = "qcom,msm8996-camss", .data = &msm8996_resources },
 	{ .compatible = "qcom,sdm660-camss", .data = &sdm660_resources },
 	{ .compatible = "qcom,sdm845-camss", .data = &sdm845_resources },
 	{ .compatible = "qcom,sm8250-camss", .data = &sm8250_resources },
+	{ .compatible = "qcom,sc7280-camss", .data = &sc7280_resources },
 	{ .compatible = "qcom,sc8280xp-camss", .data = &sc8280xp_resources },
 	{ }
 };
diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
index 73c47c07fc30..29dbf93ce9c5 100644
--- a/drivers/media/platform/qcom/camss/camss.h
+++ b/drivers/media/platform/qcom/camss/camss.h
@@ -79,11 +79,13 @@ enum camss_version {
 	CAMSS_845,
 	CAMSS_8250,
 	CAMSS_8280XP,
+	CAMSS_7280,
 };
 
 enum icc_count {
 	ICC_DEFAULT_COUNT = 0,
 	ICC_SM8250_COUNT = 4,
+	ICC_SM7280_COUNT = 4,
 };
 
 struct camss_resources {

-- 
2.25.1

Re: [PATCH 6/6] media: qcom: camss: support for camss driver for sc7280

[Bryan O'Donoghue]

On 28/06/2024 19:32, Vikram Sharma wrote:
> From: Suresh Vankadara <quic_svankada@quicinc.com>
> 
> This change adds support for camss driver for sc7280 soc.
> 
> Signed-off-by: Suresh Vankadara <quic_svankada@quicinc.com>
> Signed-off-by: Trishansh Bhardwaj <quic_tbhardwa@quicinc.com>
> Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com>
> ---
>   drivers/media/platform/qcom/camss/camss-csid.c     |  16 +-
>   .../platform/qcom/camss/camss-csiphy-3ph-1-0.c     |   2 +
>   drivers/media/platform/qcom/camss/camss-vfe.c      |   2 +
>   drivers/media/platform/qcom/camss/camss.c          | 340 +++++++++++++++++++++
>   drivers/media/platform/qcom/camss/camss.h          |   2 +
>   5 files changed, 359 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c
> index 858db5d4ca75..2c622233da6f 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid.c
> +++ b/drivers/media/platform/qcom/camss/camss-csid.c
> @@ -28,6 +28,7 @@
>   /* offset of CSID registers in VFE region for VFE 480 */
>   #define VFE_480_CSID_OFFSET 0x1200
>   #define VFE_480_LITE_CSID_OFFSET 0x200
> +#define VFE_165_CSID_OFFSET 0x4000
>   
>   #define MSM_CSID_NAME "msm_csid"
>   
> @@ -1028,8 +1029,8 @@ int msm_csid_subdev_init(struct camss *camss, struct csid_device *csid,
>   	csid->res->hw_ops->subdev_init(csid);
>   
>   	/* Memory */
> -
> -	if (camss->res->version == CAMSS_8250) {
> +	switch (camss->res->version) {
> +	case CAMSS_8250:
>   		/* for titan 480, CSID registers are inside the VFE region,
>   		 * between the VFE "top" and "bus" registers. this requires
>   		 * VFE to be initialized before CSID
> @@ -1040,10 +1041,19 @@ int msm_csid_subdev_init(struct camss *camss, struct csid_device *csid,
>   		else
>   			csid->base = csid->res->parent_dev_ops->get_base_address(camss, id)
>   				 + VFE_480_CSID_OFFSET;
> -	} else {
> +		break;
> +	case CAMSS_7280:
> +		/* for titan 165, CSID registers are inside the VFE region,
> +		 * between the VFE "top" and "bus" registers. this requires
> +		 * VFE to be initialized before CSID
> +		 */
> +		csid->base = camss->vfe[id].base + VFE_165_CSID_OFFSET;


Right but you can just define "csid" registers in your yaml and dts per 
standard definitions.

Looking at what we did for 8250 here there's absolutely no good reason 
to have C code derive offsets like this which can be described in dts.

I'll send a patch to that effect - along with named power-domains for 8250.

Please just define your CSID registers in the yaml/dts - there's no need 
to add executable code to the driver to find an offset.

> +		break;
> +	default:
>   		csid->base = devm_platform_ioremap_resource_byname(pdev, res->reg[0]);
>   		if (IS_ERR(csid->base))
>   			return PTR_ERR(csid->base);
> +		break;
>   	}
>   
>   	/* Interrupt */
> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
> index df7e93a5a4f6..c7e507420732 100644
> --- a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
> +++ b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
> @@ -510,6 +510,7 @@ static void csiphy_gen2_config_lanes(struct csiphy_device *csiphy,
>   		array_size = ARRAY_SIZE(lane_regs_sdm845[0]);
>   		break;
>   	case CAMSS_8250:
> +	case CAMSS_7280:
>   		r = &lane_regs_sm8250[0][0];
>   		array_size = ARRAY_SIZE(lane_regs_sm8250[0]);
>   		break;
> @@ -560,6 +561,7 @@ static bool csiphy_is_gen2(u32 version)
>   	case CAMSS_845:
>   	case CAMSS_8250:
>   	case CAMSS_8280XP:
> +	case CAMSS_7280:

Sort alphanumerically please.


> +	/* CSIPHY0 */
> +	{
> +		.regulators = {},
> +		.clock = { "csiphy0", "csiphy0_timer", "csiphy0_timer_src"},
> +		.clock_rate = { { 300000000 },
> +				{ 300000000 },
> +				{ 300000000 }},

I'll reiterate, I don't believe the _src clocks are required.


> +
> +static const struct resources_icc icc_res_sc7280[] = {
> +	{
> +		.name = "cam_ahb",
> +		.icc_bw_tbl.avg = 38400,
> +		.icc_bw_tbl.peak = 76800,
> +	},
> +	{
> +		.name = "cam_hf_0",
> +		.icc_bw_tbl.avg = 2097152,
> +		.icc_bw_tbl.peak = 2097152,
> +	},
> +};

Good to see this.

> +
>   /*
>    * camss_add_clock_margin - Add margin to clock frequency rate
>    * @rate: Clock frequency rate
> @@ -1824,6 +2099,57 @@ static int camss_init_subdevices(struct camss *camss)
>   	return 0;
>   }
>   
> +/*
> + * camss_link_entities_v2 - Register subdev nodes and create links
> + * @camss: CAMSS device
> + *
> + * Return 0 on success or a negative error code on failure
> + */
> +static int camss_link_entities_v2(struct camss *camss)
> +{
> +	int i, j;
> +	int ret;
> +
> +	for (i = 0; i < camss->res->csiphy_num; i++) {
> +		for (j = 0; j < camss->res->csid_num; j++) {
> +			ret = media_create_pad_link(&camss->csiphy[i].subdev.entity,
> +						    MSM_CSIPHY_PAD_SRC,
> +						    &camss->csid[j].subdev.entity,
> +						    MSM_CSID_PAD_SINK,
> +						    0);
> +			if (ret < 0) {
> +				dev_err(camss->dev,
> +					"Failed to link %s->%s entities: %d\n",
> +					camss->csiphy[i].subdev.entity.name,
> +					camss->csid[j].subdev.entity.name,
> +					ret);
> +				return ret;
> +			}
> +		}
> +	}
> +
> +	for (i = 0; i < camss->res->csid_num; i++)
> +		for (j = 0; j < camss->vfe[i].res->line_num; j++) {
> +			struct v4l2_subdev *csid = &camss->csid[i].subdev;
> +			struct v4l2_subdev *vfe = &camss->vfe[i].line[j].subdev;
> +
> +			ret = media_create_pad_link(&csid->entity,
> +						    MSM_CSID_PAD_FIRST_SRC + j,
> +						    &vfe->entity,
> +						    MSM_VFE_PAD_SINK,
> +						    0);
> +			if (ret < 0) {
> +				dev_err(camss->dev,
> +					"Failed to link %s->%s entities: %d\n",
> +					csid->entity.name,
> +					vfe->entity.name,
> +					ret);
> +				return ret;
> +			}
> +		}
> +	return 0;
> +}

So I see what you're doing here and agree but, I think it should be made 
into its own standalone patch.

We can break up the link_entities function into something for ispif the 
v1 and something for everybody else @ v2, not just 7280.

Either way such a change deserves its own standalone patch.

> +
>   /*
>    * camss_link_entities - Register subdev nodes and create links
>    * @camss: CAMSS device
> @@ -2440,12 +2766,26 @@ static const struct camss_resources sc8280xp_resources = {
>   	.link_entities = camss_link_entities
>   };
>   
> +static const struct camss_resources sc7280_resources = {
> +	.version = CAMSS_7280,
> +	.csiphy_res = csiphy_res_7280,
> +	.csid_res = csid_res_7280,
> +	.vfe_res = vfe_res_7280,
> +	.icc_res = icc_res_sc7280,
> +	.icc_path_num = ARRAY_SIZE(icc_res_sc7280),
> +	.csiphy_num = ARRAY_SIZE(csiphy_res_7280),
> +	.csid_num = ARRAY_SIZE(csid_res_7280),
> +	.vfe_num = 3,
> +	.link_entities = camss_link_entities_v2
> +};
> +
>   static const struct of_device_id camss_dt_match[] = {
>   	{ .compatible = "qcom,msm8916-camss", .data = &msm8916_resources },
>   	{ .compatible = "qcom,msm8996-camss", .data = &msm8996_resources },
>   	{ .compatible = "qcom,sdm660-camss", .data = &sdm660_resources },
>   	{ .compatible = "qcom,sdm845-camss", .data = &sdm845_resources },
>   	{ .compatible = "qcom,sm8250-camss", .data = &sm8250_resources },
> +	{ .compatible = "qcom,sc7280-camss", .data = &sc7280_resources },
>   	{ .compatible = "qcom,sc8280xp-camss", .data = &sc8280xp_resources },

Its just occured to me, this list ought to be sorted alpanumerically too.

I'd be obliged if you could add a patch to this series to sort this list 
prior to adding in your new string - in the appropriate order.

>   	{ }
>   };
> diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/media/platform/qcom/camss/camss.h
> index 73c47c07fc30..29dbf93ce9c5 100644
> --- a/drivers/media/platform/qcom/camss/camss.h
> +++ b/drivers/media/platform/qcom/camss/camss.h
> @@ -79,11 +79,13 @@ enum camss_version {
>   	CAMSS_845,
>   	CAMSS_8250,
>   	CAMSS_8280XP,
> +	CAMSS_7280,
>   };
>   
>   enum icc_count {
>   	ICC_DEFAULT_COUNT = 0,
>   	ICC_SM8250_COUNT = 4,
> +	ICC_SM7280_COUNT = 4,
>   };

Do you even use the SM7280 specific enum ? I didn't see it, SoC name is 
SC7280 anyway.

I think you can drop that.

---
bod

Re: [PATCH 6/6] media: qcom: camss: support for camss driver for sc7280

[Marion & Christophe JAILLET]

Le 28/06/2024 à 20:32, Vikram Sharma a écrit :
> From: Suresh Vankadara <quic_svankada@quicinc.com>
> 
> This change adds support for camss driver for sc7280 soc.
> 
> Signed-off-by: Suresh Vankadara <quic_svankada@quicinc.com>
> Signed-off-by: Trishansh Bhardwaj <quic_tbhardwa@quicinc.com>
> Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com>
> ---

Hi,

...

> -	if (camss->res->version == CAMSS_8250) {
> +	switch (camss->res->version) {
> +	case CAMSS_8250:
>   		/* for titan 480, CSID registers are inside the VFE region,
>   		 * between the VFE "top" and "bus" registers. this requires
>   		 * VFE to be initialized before CSID
> @@ -1040,10 +1041,19 @@ int msm_csid_subdev_init(struct camss *camss, struct csid_device *csid,
>   		else
>   			csid->base = csid->res->parent_dev_ops->get_base_address(camss, id)
>   				 + VFE_480_CSID_OFFSET;
> -	} else {
> +		break;
> +	case CAMSS_7280:

Maybe, as said for other places by Bryan, keep it ordered (CAMSS_7280, 
then CAMSS_8250)?

> +		/* for titan 165, CSID registers are inside the VFE region,
> +		 * between the VFE "top" and "bus" registers. this requires
> +		 * VFE to be initialized before CSID
> +		 */
> +		csid->base = camss->vfe[id].base + VFE_165_CSID_OFFSET;
> +		break;
> +	default:
>   		csid->base = devm_platform_ioremap_resource_byname(pdev, res->reg[0]);
>   		if (IS_ERR(csid->base))
>   			return PTR_ERR(csid->base);
> +		break;
>   	}
>   
>   	/* Interrupt */
> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
> index df7e93a5a4f6..c7e507420732 100644
> --- a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
> +++ b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
> @@ -510,6 +510,7 @@ static void csiphy_gen2_config_lanes(struct csiphy_device *csiphy,
>   		array_size = ARRAY_SIZE(lane_regs_sdm845[0]);
>   		break;
>   	case CAMSS_8250:
> +	case CAMSS_7280:
>   		r = &lane_regs_sm8250[0][0];
>   		array_size = ARRAY_SIZE(lane_regs_sm8250[0]);
>   		break;
> @@ -560,6 +561,7 @@ static bool csiphy_is_gen2(u32 version)
>   	case CAMSS_845:
>   	case CAMSS_8250:
>   	case CAMSS_8280XP:
> +	case CAMSS_7280:
>   		ret = true;
>   		break;
>   	}
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
> index 83c5a36d071f..757e872b8eb8 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
> @@ -338,6 +338,7 @@ static u32 vfe_src_pad_code(struct vfe_line *line, u32 sink_code,
>   	case CAMSS_845:
>   	case CAMSS_8250:
>   	case CAMSS_8280XP:
> +	case CAMSS_7280:

Here as well.

>   		switch (sink_code) {
>   		case MEDIA_BUS_FMT_YUYV8_1X16:
>   		{
> @@ -1695,6 +1696,7 @@ static int vfe_bpl_align(struct vfe_device *vfe)
>   	case CAMSS_845:
>   	case CAMSS_8250:
>   	case CAMSS_8280XP:
> +	case CAMSS_7280:

Here as well.

>   		ret = 16;
>   		break;
>   	default:

...

> +static int camss_link_entities_v2(struct camss *camss)
> +{
> +	int i, j;
> +	int ret;
> +
> +	for (i = 0; i < camss->res->csiphy_num; i++) {
> +		for (j = 0; j < camss->res->csid_num; j++) {
> +			ret = media_create_pad_link(&camss->csiphy[i].subdev.entity,
> +						    MSM_CSIPHY_PAD_SRC,
> +						    &camss->csid[j].subdev.entity,
> +						    MSM_CSID_PAD_SINK,
> +						    0);

Should there be some error handling path here and below to free the 
allocated resources?

.link_entities seems to be new and I can't find it in my -next-20240627, 
so I can't check myself if already handle in a way or another by the 
framework.

CJ


> +			if (ret < 0) {
> +				dev_err(camss->dev,
> +					"Failed to link %s->%s entities: %d\n",
> +					camss->csiphy[i].subdev.entity.name,
> +					camss->csid[j].subdev.entity.name,
> +					ret);
> +				return ret;
> +			}
> +		}
> +	}
> +
> +	for (i = 0; i < camss->res->csid_num; i++)
> +		for (j = 0; j < camss->vfe[i].res->line_num; j++) {
> +			struct v4l2_subdev *csid = &camss->csid[i].subdev;
> +			struct v4l2_subdev *vfe = &camss->vfe[i].line[j].subdev;
> +
> +			ret = media_create_pad_link(&csid->entity,
> +						    MSM_CSID_PAD_FIRST_SRC + j,
> +						    &vfe->entity,
> +						    MSM_VFE_PAD_SINK,
> +						    0);
> +			if (ret < 0) {
> +				dev_err(camss->dev,
> +					"Failed to link %s->%s entities: %d\n",
> +					csid->entity.name,
> +					vfe->entity.name,
> +					ret);
> +				return ret;
> +			}
> +		}
> +	return 0;
> +}

...

Re: [PATCH 0/6] media: qcom: camss: Add sc7280 support

[Luca Weiss]

On Fri Jun 28, 2024 at 8:32 PM CEST, Vikram Sharma wrote:
> SC7280 is a Qualcomm SoC. This series adds support to
> bring up the CSIPHY, CSID, VFE/RDI interfaces in SC7280.
>
> SC7280 provides
>
> - 3 x VFE, 3 RDI per VFE
> - 2 x VFE Lite, 4 RDI per VFE
> - 3 x CSID
> - 2 x CSID Lite
> - 5 x CSI PHY
>
> This series is rebased based on：
> https://lore.kernel.org/linux-arm-msm/20240522154659.510-1-quic_grosikop@quicinc.com/
>
> The changes are verified on SC7280 qcs6490-rb3gen2 board, the base dts for qcs6490-rb3gen2
> is:
> https://lore.kernel.org/all/20231103184655.23555-1-quic_kbajaj@quicinc.com/

Hi Vikram,

Thanks for sending this patch!

I just tried this on QCM6490 Fairphone 5 smartphone but unfortunately
during probe something is not quite right.

[   99.531855] qcom-camss acaf000.camss: Adding to iommu group 11
[   99.533180] ------------[ cut here ]------------
[   99.533187] qcom-camss acaf000.camss: Error: CSID depends on VFE/IFE device ops!
[   99.533219] WARNING: CPU: 2 PID: 6902 at drivers/media/platform/qcom/camss/camss-csid.c:1024 msm_csid_subdev_init+0x41c/0x460 [qcom_camss]
[   99.533248] Modules linked in: qcom_camss(+) videobuf2_dma_sg videobuf2_memops videobuf2_v4l2 videobuf2_common
[   99.533266] CPU: 2 PID: 6902 Comm: modprobe Not tainted 6.10.0-rc5-00087-g1dd25cd60c69 #138
[   99.533272] Hardware name: Fairphone 5 (DT)
[   99.533276] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[   99.533281] pc : msm_csid_subdev_init+0x41c/0x460 [qcom_camss]
[   99.533301] lr : msm_csid_subdev_init+0x41c/0x460 [qcom_camss]
[   99.533321] sp : ffff80008575b760
[   99.533324] x29: ffff80008575b760 x28: ffffae85fd7685d8 x27: ffff80008575bca8
[   99.533334] x26: 0000000000002c28 x25: 0000000000000000 x24: ffff307e4c48a080
[   99.533343] x23: ffff307e46876080 x22: 0000000000000000 x21: ffff307e40bdac10
[   99.533352] x20: ffff307e46876080 x19: 0000000000000000 x18: fffffffffffed520
[   99.533361] x17: 2065636976656420 x16: 4546492f45465620 x15: 6e6f2073646e6570
[   99.533369] x14: ffffae865b46dad0 x13: 2173706f20656369 x12: 766564204546492f
[   99.533377] x11: ffffae865b46dad0 x10: 00000000000002d3 x9 : ffffae865b4c5ad0
[   99.533386] x8 : 0000000000017fe8 x7 : 00000000fffff000 x6 : ffffae865b4c5ad0
[   99.533394] x5 : ffff307fb6f83848 x4 : 0000000000000000 x3 : ffff81f95bd4d000
[   99.533402] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff307e4dfe9000
[   99.533412] Call trace:
[   99.533415]  msm_csid_subdev_init+0x41c/0x460 [qcom_camss]
[   99.533435]  camss_probe+0x310/0x998 [qcom_camss]
[   99.533454]  platform_probe+0x68/0xe0
[   99.533465]  really_probe+0xbc/0x2c0
[   99.533471]  __driver_probe_device+0x78/0x120
[   99.533479]  driver_probe_device+0x3c/0x160
[   99.533485]  __driver_attach+0x90/0x1a0
[   99.533490]  bus_for_each_dev+0x7c/0xf0
[   99.533497]  driver_attach+0x24/0x30
[   99.533503]  bus_add_driver+0xe4/0x208
[   99.533509]  driver_register+0x68/0x124
[   99.533514]  __platform_driver_register+0x28/0x40
[   99.533521]  qcom_camss_driver_init+0x20/0x1000 [qcom_camss]
[   99.533540]  do_one_initcall+0x60/0x1d4
[   99.533547]  do_init_module+0x5c/0x21c
[   99.533555]  load_module+0x18b0/0x1e40
[   99.533562]  init_module_from_file+0x88/0xcc
[   99.533568]  __arm64_sys_finit_module+0x174/0x340
[   99.533575]  invoke_syscall+0x48/0x10c
[   99.533582]  el0_svc_common.constprop.0+0x40/0xe0
[   99.533588]  do_el0_svc+0x1c/0x34
[   99.533594]  el0_svc+0x34/0xe0
[   99.533602]  el0t_64_sync_handler+0x120/0x12c
[   99.533608]  el0t_64_sync+0x190/0x194
[   99.533613] ---[ end trace 0000000000000000 ]---
[   99.533619] qcom-camss acaf000.camss: Failed to init csid0 sub-device: -22
[   99.533828] qcom-camss acaf000.camss: probe with driver qcom-camss failed with error -22

My tree is based on 6.10-rc5 plus v4 of:
https://lore.kernel.org/linux-arm-msm/20240522154659.510-1-quic_grosikop@quicinc.com/
plus your v1 series.

And then some extra patches for my device but nothing touching camss
driver.

Am I missing something?

Regards
Luca

>
> Suresh Vankadara (2):
> media: qcom: camss: support for camss driver on sc7280
> arm64: dts: qcom: sc7280: Add support for camss
>
> Trishansh Bhardwaj (2):
> media: qcom: camss: support for camss driver on sc7280
> arm64: dts: qcom: sc7280: Add support for camss
>
> Vikram Sharma (1):
> media: dt-bindings: media: camss: Add qcom,sc7280-camss binding
>
> Hariram Purshotam (3):
> i2c: Enable IMX577 camera sensor for qcm6490
> arm64: dts: qcom: qcs6490-rb3gen2: Enable IMX577 camera sensor
> arm64: dts: qcom: sc7280: Add IMX577 camera sensor
>
> Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com>
> ---
> Suresh Vankadara (1):
>       media: qcom: camss: support for camss driver for sc7280
>
> Vikram Sharma (5):
>       media: dt-bindings: media: camss: Add qcom,sc7280-camss binding
>       arm64: dts: qcom: sc7280: Add support for camss
>       arm64: dts: qcom: sc7280: Add IMX577 camera sensor
>       arm64: dts: qcom: qcs6490-rb3gen2: Enable IMX577 camera sensor
>       i2c: Enable IMX577 camera sensor for qcm6490
>
>  .../bindings/media/qcom,sc7280-camss.yaml          | 477 +++++++++++++++++++++
>  arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts       |  67 +++
>  arch/arm64/boot/dts/qcom/sc7280.dtsi               | 215 ++++++++++
>  drivers/i2c/busses/i2c-qcom-cci.c                  |   1 +
>  drivers/media/platform/qcom/camss/camss-csid.c     |  16 +-
>  .../platform/qcom/camss/camss-csiphy-3ph-1-0.c     |   2 +
>  drivers/media/platform/qcom/camss/camss-vfe.c      |   2 +
>  drivers/media/platform/qcom/camss/camss.c          | 340 +++++++++++++++
>  drivers/media/platform/qcom/camss/camss.h          |   2 +
>  9 files changed, 1119 insertions(+), 3 deletions(-)
> ---
> base-commit: 18eeb2d92baca167809cd5d48eb60e0a5c036d51
> change-id: 20240627-camss_first_post_linux_next-f4163c90093c
>
> Best regards,