[PATCH v4 0/7] Add SDM670 camera subsystem

[PATCH v4 0/7] Add SDM670 camera subsystem

[Richard Acayan]

This adds support for the camera subsystem on the Snapdragon 670.

As of next-20240902, camss seems to be a bit broken, but the same series
works on stable (although it is much less reliable now that the CCI clock
frequency is not being assigned).

Changes since v3 (20240819221051.31489-7-mailingradian@gmail.com):
- add specific sdm670 compatible for camcc to dt schema and dts (1/7, 6/7)
- pick up patch from Bryan for CCI driver (3/7)
- stop assigning CCI frequency in dts (7/7)
- add maxItems for sdm670 cci clocks (2/7)
- remove empty line at top of camss dt schema (4/7)
- move regs and reg-names up in camss dt schema (4/7)
- move gcc and ahb clocks up in dts and dt schema (4/7, 7/7)
- add reviewed-by from Vladimir for CCI dt-bindings patch (2/7)
- add reviewed-by from Bryan for dts patch (7/7)
- add reviewed-by from Krzysztof for camss dt-bindings patch (4/7)
- add rewiew tags for camss driver patch (5/7)

Changes since v2 (20240813230037.84004-8-mailingradian@gmail.com):
- drop unnecessary assigned AXI clock frequency (5/5)
- drop src clocks from cci (5/5)
- add unit name, remove mmio properties from port in example dts (2/5)
- correct the reg-names order (2/5)
- add parent_dev_ops to csid (3/5)
- remove CSID clocks from VFE (3/5)
- remove AXI clock from CSIPHY (3/5)
- change subsystem part of the commit message summary (3/5)
- add reviewed-by (4/5)

Changes since v1 (20240806224219.71623-7-mailingradian@gmail.com):
- define dedicated resource structs/arrays for sdm670 (3/5)
- separate camcc device tree node into its own patch (4/5)
- specify correct dual license (2/5)
- add include directives in dt-bindings camss example (2/5)
- remove src clocks from dt-bindings (2/5)
- remove src clocks from dtsi (5/5)
- add power-domain-names to camss (5/5)
- specify power domain names (3/5)
- restrict cci-i2c clocks (1/5)
- populate a commit message with hw info (2/5)
- reword commit message (3/5)

Bryan O'Donoghue (1):
  i2c: qcom-cci: Stop complaining about DT set clock rate

Richard Acayan (6):
  dt-bindings: clock: qcom,sdm845-camcc: add sdm670 compatible
  dt-bindings: i2c: qcom-cci: Document SDM670 compatible
  dt-bindings: media: camss: Add qcom,sdm670-camss
  media: qcom: camss: add support for SDM670 camss
  arm64: dts: qcom: sdm670: add camcc
  arm64: dts: qcom: sdm670: add camss and cci

 .../bindings/clock/qcom,sdm845-camcc.yaml     |   6 +-
 .../devicetree/bindings/i2c/qcom,i2c-cci.yaml |  19 ++
 .../bindings/media/qcom,sdm670-camss.yaml     | 318 ++++++++++++++++++
 arch/arm64/boot/dts/qcom/sdm670.dtsi          | 195 +++++++++++
 drivers/i2c/busses/i2c-qcom-cci.c             |   8 -
 drivers/media/platform/qcom/camss/camss.c     | 191 +++++++++++
 6 files changed, 728 insertions(+), 9 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/qcom,sdm670-camss.yaml

-- 
2.46.0

[PATCH v4 1/7] dt-bindings: clock: qcom,sdm845-camcc: add sdm670 compatible

[Richard Acayan]

The camera clocks on SDM670 and SDM845 have no significant differences
that would require a change in the clock controller driver. The only
difference is the clock frequency at each level of the power domains,
which is not specified in the clock driver. There should still be a
compatible specific to the SoC, so add the compatible for SDM670 with
the SDM845 compatible as fallback.

Link: https://android.googlesource.com/kernel/msm/+/d4dc50c0a9291bd99895d4844f973421c047d267/drivers/clk/qcom/camcc-sdm845.c#2048
Suggested-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
Suggested-by: Konrad Dybcio <konradybcio@kernel.org>
Link: https://lore.kernel.org/linux-arm-msm/7d26a62b-b898-4737-bd53-f49821e3b471@linaro.org
Signed-off-by: Richard Acayan <mailingradian@gmail.com>
---
 .../devicetree/bindings/clock/qcom,sdm845-camcc.yaml        | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/clock/qcom,sdm845-camcc.yaml b/Documentation/devicetree/bindings/clock/qcom,sdm845-camcc.yaml
index 810b852ae371..fa95c3a1ba3a 100644
--- a/Documentation/devicetree/bindings/clock/qcom,sdm845-camcc.yaml
+++ b/Documentation/devicetree/bindings/clock/qcom,sdm845-camcc.yaml
@@ -20,7 +20,11 @@ allOf:
 
 properties:
   compatible:
-    const: qcom,sdm845-camcc
+    oneOf:
+      - items:
+          - const: qcom,sdm670-camcc
+          - const: qcom,sdm845-camcc
+      - const: qcom,sdm845-camcc
 
   clocks:
     items:
-- 
2.46.0

[PATCH v4 2/7] dt-bindings: i2c: qcom-cci: Document SDM670 compatible

[Richard Acayan]

The CCI on the Snapdragon 670 is the interface for controlling camera
hardware over I2C. Add the compatible so it can be added to the SDM670
device tree.

Signed-off-by: Richard Acayan <mailingradian@gmail.com>
Reviewed-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 .../devicetree/bindings/i2c/qcom,i2c-cci.yaml | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml b/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml
index c33ae7b63b84..b4450cbba3b2 100644
--- a/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml
+++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml
@@ -27,6 +27,7 @@ properties:
           - enum:
               - qcom,sc7280-cci
               - qcom,sc8280xp-cci
+              - qcom,sdm670-cci
               - qcom,sdm845-cci
               - qcom,sm6350-cci
               - qcom,sm8250-cci
@@ -138,6 +139,24 @@ allOf:
             - const: cci
             - const: camss_ahb
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,sdm670-cci
+    then:
+      properties:
+        clocks:
+          minItems: 4
+          maxItems: 4
+        clock-names:
+          items:
+            - const: camnoc_axi
+            - const: soc_ahb
+            - const: cpas_ahb
+            - const: cci
+
   - if:
       properties:
         compatible:
-- 
2.46.0

[PATCH v4 3/7] i2c: qcom-cci: Stop complaining about DT set clock rate

[Richard Acayan]

From: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

It is common practice in the downstream and upstream CCI dt to set CCI
clock rates to 19.2 MHz. It appears to be fairly common for initial code to
set the CCI clock rate to 37.5 MHz.

Applying the widely used CCI clock rates from downstream ought not to cause
warning messages in the upstream kernel where our general policy is to
usually copy downstream hardware clock rates across the range of Qualcomm
drivers.

Drop the warning it is pervasive across CAMSS users but doesn't add any
information or warrant any changes to the DT to align the DT clock rate to
the bootloader clock rate.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Reviewed-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
Link: https://lore.kernel.org/linux-arm-msm/20240824115900.40702-1-bryan.odonoghue@linaro.org
Signed-off-by: Richard Acayan <mailingradian@gmail.com>
---
 drivers/i2c/busses/i2c-qcom-cci.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
index 414882c57d7f..99e4305a3373 100644
--- a/drivers/i2c/busses/i2c-qcom-cci.c
+++ b/drivers/i2c/busses/i2c-qcom-cci.c
@@ -602,14 +602,6 @@ static int cci_probe(struct platform_device *pdev)
 		}
 	}
 
-	if (cci_clk_rate != cci->data->cci_clk_rate) {
-		/* cci clock set by the bootloader or via assigned clock rate
-		 * in DT.
-		 */
-		dev_warn(dev, "Found %lu cci clk rate while %lu was expected\n",
-			 cci_clk_rate, cci->data->cci_clk_rate);
-	}
-
 	ret = cci_enable_clocks(cci);
 	if (ret < 0)
 		return ret;
-- 
2.46.0

[PATCH v4 4/7] dt-bindings: media: camss: Add qcom,sdm670-camss

[Richard Acayan]

As found in the Pixel 3a, the Snapdragon 670 has a camera subsystem with
3 CSIDs and 3 VFEs (including 1 VFE lite). Add this camera subsystem to
the bindings.

Adapted from SC8280XP camera subsystem.

Signed-off-by: Richard Acayan <mailingradian@gmail.com>
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 .../bindings/media/qcom,sdm670-camss.yaml     | 318 ++++++++++++++++++
 1 file changed, 318 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/qcom,sdm670-camss.yaml

diff --git a/Documentation/devicetree/bindings/media/qcom,sdm670-camss.yaml b/Documentation/devicetree/bindings/media/qcom,sdm670-camss.yaml
new file mode 100644
index 000000000000..cc2c811a0aa7
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/qcom,sdm670-camss.yaml
@@ -0,0 +1,318 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/qcom,sdm670-camss.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm SDM670 Camera Subsystem (CAMSS)
+
+maintainers:
+  - Richard Acayan <mailingradian@gmail.com>
+
+description:
+  The CAMSS IP is a CSI decoder and ISP present on Qualcomm platforms.
+
+properties:
+  compatible:
+    const: qcom,sdm670-camss
+
+  reg:
+    maxItems: 9
+
+  reg-names:
+    items:
+      - const: csiphy0
+      - const: csiphy1
+      - const: csiphy2
+      - const: vfe0
+      - const: csid0
+      - const: vfe1
+      - const: csid1
+      - const: vfe_lite
+      - const: csid2
+
+  clocks:
+    maxItems: 22
+
+  clock-names:
+    items:
+      - const: gcc_camera_ahb
+      - const: gcc_camera_axi
+      - const: soc_ahb
+      - const: camnoc_axi
+      - const: cpas_ahb
+      - const: csi0
+      - const: csi1
+      - const: csi2
+      - const: csiphy0
+      - const: csiphy0_timer
+      - const: csiphy1
+      - const: csiphy1_timer
+      - const: csiphy2
+      - const: csiphy2_timer
+      - const: vfe0_axi
+      - const: vfe0
+      - const: vfe0_cphy_rx
+      - const: vfe1_axi
+      - const: vfe1
+      - const: vfe1_cphy_rx
+      - const: vfe_lite
+      - const: vfe_lite_cphy_rx
+
+  interrupts:
+    maxItems: 9
+
+  interrupt-names:
+    items:
+      - const: csid0
+      - const: csid1
+      - const: csid2
+      - const: csiphy0
+      - const: csiphy1
+      - const: csiphy2
+      - const: vfe0
+      - const: vfe1
+      - const: vfe_lite
+
+  iommus:
+    maxItems: 4
+
+  power-domains:
+    items:
+      - description: IFE0 GDSC - Image Front End, Global Distributed Switch Controller.
+      - description: IFE1 GDSC - Image Front End, Global Distributed Switch Controller.
+      - description: Titan Top GDSC - Titan ISP Block, Global Distributed Switch Controller.
+
+  power-domain-names:
+    items:
+      - const: ife0
+      - const: ife1
+      - const: top
+
+  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 from CSIPHY0.
+
+        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 from CSIPHY1.
+
+        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 from CSIPHY2.
+
+        properties:
+          endpoint:
+            $ref: video-interfaces.yaml#
+            unevaluatedProperties: false
+
+            properties:
+              clock-lanes:
+                maxItems: 1
+
+              data-lanes:
+                minItems: 1
+                maxItems: 4
+
+            required:
+              - clock-lanes
+              - data-lanes
+
+  vdda-phy-supply:
+    description:
+      Phandle to a regulator supply to PHY core block.
+
+  vdda-pll-supply:
+    description:
+      Phandle to 1.8V regulator supply to PHY refclk pll block.
+
+required:
+  - reg
+  - reg-names
+  - clock-names
+  - clocks
+  - compatible
+  - interrupts
+  - interrupt-names
+  - iommus
+  - power-domains
+  - power-domain-names
+  - vdda-phy-supply
+  - vdda-pll-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/qcom,camcc-sdm845.h>
+    #include <dt-bindings/clock/qcom,gcc-sdm845.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        camss@ac65000 {
+            compatible = "qcom,sdm670-camss";
+
+            reg = <0 0x0ac65000 0 0x1000>,
+                  <0 0x0ac66000 0 0x1000>,
+                  <0 0x0ac67000 0 0x1000>,
+                  <0 0x0acaf000 0 0x4000>,
+                  <0 0x0acb3000 0 0x1000>,
+                  <0 0x0acb6000 0 0x4000>,
+                  <0 0x0acba000 0 0x1000>,
+                  <0 0x0acc4000 0 0x4000>,
+                  <0 0x0acc8000 0 0x1000>;
+            reg-names = "csiphy0",
+                        "csiphy1",
+                        "csiphy2",
+                        "vfe0",
+                        "csid0",
+                        "vfe1",
+                        "csid1",
+                        "vfe_lite",
+                        "csid2";
+
+            interrupts = <GIC_SPI 464 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 466 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 468 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 477 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 478 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 479 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 465 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 467 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 469 IRQ_TYPE_LEVEL_HIGH>;
+            interrupt-names = "csid0",
+                              "csid1",
+                              "csid2",
+                              "csiphy0",
+                              "csiphy1",
+                              "csiphy2",
+                              "vfe0",
+                              "vfe1",
+                              "vfe_lite";
+
+            clocks = <&camcc CAM_CC_CAMNOC_AXI_CLK>,
+                     <&camcc CAM_CC_CPAS_AHB_CLK>,
+                     <&camcc CAM_CC_IFE_0_CSID_CLK>,
+                     <&camcc CAM_CC_IFE_1_CSID_CLK>,
+                     <&camcc CAM_CC_IFE_LITE_CSID_CLK>,
+                     <&camcc CAM_CC_CSIPHY0_CLK>,
+                     <&camcc CAM_CC_CSI0PHYTIMER_CLK>,
+                     <&camcc CAM_CC_CSIPHY1_CLK>,
+                     <&camcc CAM_CC_CSI1PHYTIMER_CLK>,
+                     <&camcc CAM_CC_CSIPHY2_CLK>,
+                     <&camcc CAM_CC_CSI2PHYTIMER_CLK>,
+                     <&gcc GCC_CAMERA_AHB_CLK>,
+                     <&gcc GCC_CAMERA_AXI_CLK>,
+                     <&camcc CAM_CC_SOC_AHB_CLK>,
+                     <&camcc CAM_CC_IFE_0_AXI_CLK>,
+                     <&camcc CAM_CC_IFE_0_CLK>,
+                     <&camcc CAM_CC_IFE_0_CPHY_RX_CLK>,
+                     <&camcc CAM_CC_IFE_1_AXI_CLK>,
+                     <&camcc CAM_CC_IFE_1_CLK>,
+                     <&camcc CAM_CC_IFE_1_CPHY_RX_CLK>,
+                     <&camcc CAM_CC_IFE_LITE_CLK>,
+                     <&camcc CAM_CC_IFE_LITE_CPHY_RX_CLK>;
+            clock-names = "camnoc_axi",
+                          "cpas_ahb",
+                          "csi0",
+                          "csi1",
+                          "csi2",
+                          "csiphy0",
+                          "csiphy0_timer",
+                          "csiphy1",
+                          "csiphy1_timer",
+                          "csiphy2",
+                          "csiphy2_timer",
+                          "gcc_camera_ahb",
+                          "gcc_camera_axi",
+                          "soc_ahb",
+                          "vfe0_axi",
+                          "vfe0",
+                          "vfe0_cphy_rx",
+                          "vfe1_axi",
+                          "vfe1",
+                          "vfe1_cphy_rx",
+                          "vfe_lite",
+                          "vfe_lite_cphy_rx";
+
+            iommus = <&apps_smmu 0x808 0x0>,
+                     <&apps_smmu 0x810 0x8>,
+                     <&apps_smmu 0xc08 0x0>,
+                     <&apps_smmu 0xc10 0x8>;
+
+            power-domains = <&camcc IFE_0_GDSC>,
+                            <&camcc IFE_1_GDSC>,
+                            <&camcc TITAN_TOP_GDSC>;
+            power-domain-names = "ife0",
+                                 "ife1",
+                                 "top";
+
+            vdda-phy-supply = <&vreg_l1a_1p225>;
+            vdda-pll-supply = <&vreg_l8a_1p8>;
+
+            ports {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                port@0 {
+                    reg = <0>;
+
+                    csiphy_ep0: endpoint {
+                        clock-lanes = <7>;
+                        data-lanes = <0 1 2 3>;
+                        remote-endpoint = <&front_sensor_ep>;
+                    };
+                };
+            };
+        };
+    };
-- 
2.46.0

[PATCH v4 5/7] media: qcom: camss: add support for SDM670 camss

[Richard Acayan]

The camera subsystem for the SDM670 the same as on SDM845 except with
3 CSIPHY ports instead of 4. Add support for the SDM670 camera
subsystem.

Signed-off-by: Richard Acayan <mailingradian@gmail.com>
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Acked-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Reviewed-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 drivers/media/platform/qcom/camss/camss.c | 191 ++++++++++++++++++++++
 1 file changed, 191 insertions(+)

diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index 51b1d3550421..b2f22bfd8692 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -584,6 +584,185 @@ static const struct camss_subdev_resources vfe_res_660[] = {
 	}
 };
 
+static const struct camss_subdev_resources csiphy_res_670[] = {
+	/* CSIPHY0 */
+	{
+		.regulators = {},
+		.clock = { "soc_ahb", "cpas_ahb",
+				"csiphy0", "csiphy0_timer" },
+		.clock_rate = { { 0 },
+				{ 0 },
+				{ 0 },
+				{ 19200000, 240000000, 269333333 } },
+		.reg = { "csiphy0" },
+		.interrupt = { "csiphy0" },
+		.csiphy = {
+			.hw_ops = &csiphy_ops_3ph_1_0,
+			.formats = &csiphy_formats_sdm845
+		}
+	},
+
+	/* CSIPHY1 */
+	{
+		.regulators = {},
+		.clock = { "soc_ahb", "cpas_ahb",
+				"csiphy1", "csiphy1_timer" },
+		.clock_rate = { { 0 },
+				{ 0 },
+				{ 0 },
+				{ 19200000, 240000000, 269333333 } },
+		.reg = { "csiphy1" },
+		.interrupt = { "csiphy1" },
+		.csiphy = {
+			.hw_ops = &csiphy_ops_3ph_1_0,
+			.formats = &csiphy_formats_sdm845
+		}
+	},
+
+	/* CSIPHY2 */
+	{
+		.regulators = {},
+		.clock = { "soc_ahb", "cpas_ahb",
+				"csiphy2", "csiphy2_timer" },
+		.clock_rate = { { 0 },
+				{ 0 },
+				{ 0 },
+				{ 19200000, 240000000, 269333333 } },
+		.reg = { "csiphy2" },
+		.interrupt = { "csiphy2" },
+		.csiphy = {
+			.hw_ops = &csiphy_ops_3ph_1_0,
+			.formats = &csiphy_formats_sdm845
+		}
+	}
+};
+
+static const struct camss_subdev_resources csid_res_670[] = {
+	/* CSID0 */
+	{
+		.regulators = { "vdda-phy", "vdda-pll" },
+		.clock = { "cpas_ahb", "soc_ahb", "vfe0",
+				"vfe0_cphy_rx", "csi0" },
+		.clock_rate = { { 0 },
+				{ 0 },
+				{ 100000000, 320000000, 404000000, 480000000, 600000000 },
+				{ 384000000 },
+				{ 19200000, 75000000, 384000000, 538666667 } },
+		.reg = { "csid0" },
+		.interrupt = { "csid0" },
+		.csid = {
+			.hw_ops = &csid_ops_gen2,
+			.parent_dev_ops = &vfe_parent_dev_ops,
+			.formats = &csid_formats_gen2
+		}
+	},
+
+	/* CSID1 */
+	{
+		.regulators = { "vdda-phy", "vdda-pll" },
+		.clock = { "cpas_ahb", "soc_ahb", "vfe1",
+				"vfe1_cphy_rx", "csi1" },
+		.clock_rate = { { 0 },
+				{ 0 },
+				{ 100000000, 320000000, 404000000, 480000000, 600000000 },
+				{ 384000000 },
+				{ 19200000, 75000000, 384000000, 538666667 } },
+		.reg = { "csid1" },
+		.interrupt = { "csid1" },
+		.csid = {
+			.hw_ops = &csid_ops_gen2,
+			.parent_dev_ops = &vfe_parent_dev_ops,
+			.formats = &csid_formats_gen2
+		}
+	},
+
+	/* CSID2 */
+	{
+		.regulators = { "vdda-phy", "vdda-pll" },
+		.clock = { "cpas_ahb", "soc_ahb", "vfe_lite",
+				"vfe_lite_cphy_rx", "csi2" },
+		.clock_rate = { { 0 },
+				{ 0 },
+				{ 100000000, 320000000, 404000000, 480000000, 600000000 },
+				{ 384000000 },
+				{ 19200000, 75000000, 384000000, 538666667 } },
+		.reg = { "csid2" },
+		.interrupt = { "csid2" },
+		.csid = {
+			.is_lite = true,
+			.hw_ops = &csid_ops_gen2,
+			.parent_dev_ops = &vfe_parent_dev_ops,
+			.formats = &csid_formats_gen2
+		}
+	}
+};
+
+static const struct camss_subdev_resources vfe_res_670[] = {
+	/* VFE0 */
+	{
+		.regulators = {},
+		.clock = { "camnoc_axi", "cpas_ahb", "soc_ahb",
+				"vfe0", "vfe0_axi" },
+		.clock_rate = { { 0 },
+				{ 0 },
+				{ 0 },
+				{ 100000000, 320000000, 404000000, 480000000, 600000000 },
+				{ 0 } },
+		.reg = { "vfe0" },
+		.interrupt = { "vfe0" },
+		.vfe = {
+			.line_num = 4,
+			.has_pd = true,
+			.pd_name = "ife0",
+			.hw_ops = &vfe_ops_170,
+			.formats_rdi = &vfe_formats_rdi_845,
+			.formats_pix = &vfe_formats_pix_845
+		}
+	},
+
+	/* VFE1 */
+	{
+		.regulators = {},
+		.clock = { "camnoc_axi", "cpas_ahb", "soc_ahb",
+				"vfe1", "vfe1_axi" },
+		.clock_rate = { { 0 },
+				{ 0 },
+				{ 0 },
+				{ 100000000, 320000000, 404000000, 480000000, 600000000 },
+				{ 0 } },
+		.reg = { "vfe1" },
+		.interrupt = { "vfe1" },
+		.vfe = {
+			.line_num = 4,
+			.has_pd = true,
+			.pd_name = "ife1",
+			.hw_ops = &vfe_ops_170,
+			.formats_rdi = &vfe_formats_rdi_845,
+			.formats_pix = &vfe_formats_pix_845
+		}
+	},
+
+	/* VFE-lite */
+	{
+		.regulators = {},
+		.clock = { "camnoc_axi", "cpas_ahb", "soc_ahb",
+				"vfe_lite" },
+		.clock_rate = { { 0 },
+				{ 0 },
+				{ 0 },
+				{ 100000000, 320000000, 404000000, 480000000, 600000000 } },
+		.reg = { "vfe_lite" },
+		.interrupt = { "vfe_lite" },
+		.vfe = {
+			.is_lite = true,
+			.line_num = 4,
+			.hw_ops = &vfe_ops_170,
+			.formats_rdi = &vfe_formats_rdi_845,
+			.formats_pix = &vfe_formats_pix_845
+		}
+	}
+};
+
 static const struct camss_subdev_resources csiphy_res_845[] = {
 	/* CSIPHY0 */
 	{
@@ -2403,6 +2582,17 @@ static const struct camss_resources sdm660_resources = {
 	.link_entities = camss_link_entities
 };
 
+static const struct camss_resources sdm670_resources = {
+	.version = CAMSS_845,
+	.csiphy_res = csiphy_res_670,
+	.csid_res = csid_res_670,
+	.vfe_res = vfe_res_670,
+	.csiphy_num = ARRAY_SIZE(csiphy_res_670),
+	.csid_num = ARRAY_SIZE(csid_res_670),
+	.vfe_num = ARRAY_SIZE(vfe_res_670),
+	.link_entities = camss_link_entities
+};
+
 static const struct camss_resources sdm845_resources = {
 	.version = CAMSS_845,
 	.csiphy_res = csiphy_res_845,
@@ -2447,6 +2637,7 @@ 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,sdm670-camss", .data = &sdm670_resources },
 	{ .compatible = "qcom,sdm845-camss", .data = &sdm845_resources },
 	{ .compatible = "qcom,sm8250-camss", .data = &sm8250_resources },
 	{ .compatible = "qcom,sc8280xp-camss", .data = &sc8280xp_resources },
-- 
2.46.0

[PATCH v4 6/7] arm64: dts: qcom: sdm670: add camcc

[Richard Acayan]

The camera clock controller on SDM670 controls the clocks that drive the
camera subsystem. The clocks are the same as on SDM845. Add the camera
clock controller for SDM670.

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Richard Acayan <mailingradian@gmail.com>
---
 arch/arm64/boot/dts/qcom/sdm670.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm670.dtsi b/arch/arm64/boot/dts/qcom/sdm670.dtsi
index 187c6698835d..02f87200690a 100644
--- a/arch/arm64/boot/dts/qcom/sdm670.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm670.dtsi
@@ -1400,6 +1400,16 @@ spmi_bus: spmi@c440000 {
 			#interrupt-cells = <4>;
 		};
 
+		camcc: clock-controller@ad00000 {
+			compatible = "qcom,sdm670-camcc", "qcom,sdm845-camcc";
+			reg = <0 0x0ad00000 0 0x10000>;
+			clocks = <&rpmhcc RPMH_CXO_CLK>;
+			clock-names = "bi_tcxo";
+			#clock-cells = <1>;
+			#reset-cells = <1>;
+			#power-domain-cells = <1>;
+		};
+
 		mdss: display-subsystem@ae00000 {
 			compatible = "qcom,sdm670-mdss";
 			reg = <0 0x0ae00000 0 0x1000>;
-- 
2.46.0

[PATCH v4 7/7] arm64: dts: qcom: sdm670: add camss and cci

[Richard Acayan]

Add the camera subsystem and CCI used to interface with cameras on the
Snapdragon 670.

Signed-off-by: Richard Acayan <mailingradian@gmail.com>
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 arch/arm64/boot/dts/qcom/sdm670.dtsi | 185 +++++++++++++++++++++++++++
 1 file changed, 185 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm670.dtsi b/arch/arm64/boot/dts/qcom/sdm670.dtsi
index 02f87200690a..0b0330975702 100644
--- a/arch/arm64/boot/dts/qcom/sdm670.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm670.dtsi
@@ -6,6 +6,7 @@
  * Copyright (c) 2022, Richard Acayan. All rights reserved.
  */
 
+#include <dt-bindings/clock/qcom,camcc-sdm845.h>
 #include <dt-bindings/clock/qcom,dispcc-sdm845.h>
 #include <dt-bindings/clock/qcom,gcc-sdm845.h>
 #include <dt-bindings/clock/qcom,rpmh.h>
@@ -1168,6 +1169,34 @@ tlmm: pinctrl@3400000 {
 			gpio-ranges = <&tlmm 0 0 151>;
 			wakeup-parent = <&pdc>;
 
+			cci0_default: cci0-default-state {
+				pins = "gpio17", "gpio18";
+				function = "cci_i2c";
+				drive-strength = <2>;
+				bias-pull-up;
+			};
+
+			cci0_sleep: cci0-sleep-state {
+				pins = "gpio17", "gpio18";
+				function = "cci_i2c";
+				drive-strength = <2>;
+				bias-pull-down;
+			};
+
+			cci1_default: cci1-default-state {
+				pins = "gpio19", "gpio20";
+				function = "cci_i2c";
+				drive-strength = <2>;
+				bias-pull-up;
+			};
+
+			cci1_sleep: cci1-sleep-state {
+				pins = "gpio19", "gpio20";
+				function = "cci_i2c";
+				drive-strength = <2>;
+				bias-pull-down;
+			};
+
 			qup_i2c0_default: qup-i2c0-default-state {
 				pins = "gpio0", "gpio1";
 				function = "qup0";
@@ -1400,6 +1429,162 @@ spmi_bus: spmi@c440000 {
 			#interrupt-cells = <4>;
 		};
 
+		cci: cci@ac4a000 {
+			compatible = "qcom,sdm670-cci", "qcom,msm8996-cci";
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			reg = <0 0x0ac4a000 0 0x4000>;
+			interrupts = <GIC_SPI 460 IRQ_TYPE_EDGE_RISING>;
+			power-domains = <&camcc TITAN_TOP_GDSC>;
+
+			clocks = <&camcc CAM_CC_CAMNOC_AXI_CLK>,
+				 <&camcc CAM_CC_SOC_AHB_CLK>,
+				 <&camcc CAM_CC_CPAS_AHB_CLK>,
+				 <&camcc CAM_CC_CCI_CLK>;
+			clock-names = "camnoc_axi",
+				      "soc_ahb",
+				      "cpas_ahb",
+				      "cci";
+
+			pinctrl-names = "default", "sleep";
+			pinctrl-0 = <&cci0_default &cci1_default>;
+			pinctrl-1 = <&cci0_sleep &cci1_sleep>;
+
+			status = "disabled";
+
+			cci_i2c0: i2c-bus@0 {
+				reg = <0>;
+				clock-frequency = <1000000>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+			};
+
+			cci_i2c1: i2c-bus@1 {
+				reg = <1>;
+				clock-frequency = <1000000>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+			};
+		};
+
+		camss: camera-controller@ac65000 {
+			compatible = "qcom,sdm670-camss";
+			reg = <0 0x0ac65000 0 0x1000>,
+			      <0 0x0ac66000 0 0x1000>,
+			      <0 0x0ac67000 0 0x1000>,
+			      <0 0x0acaf000 0 0x4000>,
+			      <0 0x0acb3000 0 0x1000>,
+			      <0 0x0acb6000 0 0x4000>,
+			      <0 0x0acba000 0 0x1000>,
+			      <0 0x0acc4000 0 0x4000>,
+			      <0 0x0acc8000 0 0x1000>;
+			reg-names = "csiphy0",
+				    "csiphy1",
+				    "csiphy2",
+				    "vfe0",
+				    "csid0",
+				    "vfe1",
+				    "csid1",
+				    "vfe_lite",
+				    "csid2";
+
+			interrupts = <GIC_SPI 464 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 466 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 468 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 477 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 478 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 479 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 465 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 467 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 469 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "csid0",
+					  "csid1",
+					  "csid2",
+					  "csiphy0",
+					  "csiphy1",
+					  "csiphy2",
+					  "vfe0",
+					  "vfe1",
+					  "vfe_lite";
+
+			clocks = <&gcc GCC_CAMERA_AHB_CLK>,
+				 <&gcc GCC_CAMERA_AXI_CLK>,
+				 <&camcc CAM_CC_SOC_AHB_CLK>,
+				 <&camcc CAM_CC_CAMNOC_AXI_CLK>,
+				 <&camcc CAM_CC_CPAS_AHB_CLK>,
+				 <&camcc CAM_CC_IFE_0_CSID_CLK>,
+				 <&camcc CAM_CC_IFE_1_CSID_CLK>,
+				 <&camcc CAM_CC_IFE_LITE_CSID_CLK>,
+				 <&camcc CAM_CC_CSIPHY0_CLK>,
+				 <&camcc CAM_CC_CSI0PHYTIMER_CLK>,
+				 <&camcc CAM_CC_CSIPHY1_CLK>,
+				 <&camcc CAM_CC_CSI1PHYTIMER_CLK>,
+				 <&camcc CAM_CC_CSIPHY2_CLK>,
+				 <&camcc CAM_CC_CSI2PHYTIMER_CLK>,
+				 <&camcc CAM_CC_IFE_0_AXI_CLK>,
+				 <&camcc CAM_CC_IFE_0_CLK>,
+				 <&camcc CAM_CC_IFE_0_CPHY_RX_CLK>,
+				 <&camcc CAM_CC_IFE_1_AXI_CLK>,
+				 <&camcc CAM_CC_IFE_1_CLK>,
+				 <&camcc CAM_CC_IFE_1_CPHY_RX_CLK>,
+				 <&camcc CAM_CC_IFE_LITE_CLK>,
+				 <&camcc CAM_CC_IFE_LITE_CPHY_RX_CLK>;
+			clock-names = "gcc_camera_ahb",
+				      "gcc_camera_axi",
+				      "soc_ahb",
+				      "camnoc_axi",
+				      "cpas_ahb",
+				      "csi0",
+				      "csi1",
+				      "csi2",
+				      "csiphy0",
+				      "csiphy0_timer",
+				      "csiphy1",
+				      "csiphy1_timer",
+				      "csiphy2",
+				      "csiphy2_timer",
+				      "vfe0_axi",
+				      "vfe0",
+				      "vfe0_cphy_rx",
+				      "vfe1_axi",
+				      "vfe1",
+				      "vfe1_cphy_rx",
+				      "vfe_lite",
+				      "vfe_lite_cphy_rx";
+
+			iommus = <&apps_smmu 0x808 0x0>,
+				 <&apps_smmu 0x810 0x8>,
+				 <&apps_smmu 0xc08 0x0>,
+				 <&apps_smmu 0xc10 0x8>;
+
+			power-domains = <&camcc IFE_0_GDSC>,
+					<&camcc IFE_1_GDSC>,
+					<&camcc TITAN_TOP_GDSC>;
+			power-domain-names = "ife0",
+					     "ife1",
+					     "top";
+
+			status = "disabled";
+
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				camss_port0: port@0 {
+					reg = <0>;
+				};
+
+				camss_port1: port@1 {
+					reg = <1>;
+				};
+
+				camss_port2: port@2 {
+					reg = <2>;
+				};
+			};
+		};
+
 		camcc: clock-controller@ad00000 {
 			compatible = "qcom,sdm670-camcc", "qcom,sdm845-camcc";
 			reg = <0 0x0ad00000 0 0x10000>;
-- 
2.46.0

Re: [PATCH v4 4/7] dt-bindings: media: camss: Add qcom,sdm670-camss

[Rob Herring (Arm)]

On Tue, 03 Sep 2024 22:04:53 -0400, Richard Acayan wrote:
> As found in the Pixel 3a, the Snapdragon 670 has a camera subsystem with
> 3 CSIDs and 3 VFEs (including 1 VFE lite). Add this camera subsystem to
> the bindings.
> 
> Adapted from SC8280XP camera subsystem.
> 
> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---
>  .../bindings/media/qcom,sdm670-camss.yaml     | 318 ++++++++++++++++++
>  1 file changed, 318 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/qcom,sdm670-camss.yaml
> 

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

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sdm670-camss.example.dtb: camss@ac65000: clock-names:0: 'gcc_camera_ahb' was expected
	from schema $id: http://devicetree.org/schemas/media/qcom,sdm670-camss.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sdm670-camss.example.dtb: camss@ac65000: clock-names:1: 'gcc_camera_axi' was expected
	from schema $id: http://devicetree.org/schemas/media/qcom,sdm670-camss.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sdm670-camss.example.dtb: camss@ac65000: clock-names:2: 'soc_ahb' was expected
	from schema $id: http://devicetree.org/schemas/media/qcom,sdm670-camss.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sdm670-camss.example.dtb: camss@ac65000: clock-names:3: 'camnoc_axi' was expected
	from schema $id: http://devicetree.org/schemas/media/qcom,sdm670-camss.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sdm670-camss.example.dtb: camss@ac65000: clock-names:4: 'cpas_ahb' was expected
	from schema $id: http://devicetree.org/schemas/media/qcom,sdm670-camss.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sdm670-camss.example.dtb: camss@ac65000: clock-names:5: 'csi0' was expected
	from schema $id: http://devicetree.org/schemas/media/qcom,sdm670-camss.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sdm670-camss.example.dtb: camss@ac65000: clock-names:6: 'csi1' was expected
	from schema $id: http://devicetree.org/schemas/media/qcom,sdm670-camss.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sdm670-camss.example.dtb: camss@ac65000: clock-names:7: 'csi2' was expected
	from schema $id: http://devicetree.org/schemas/media/qcom,sdm670-camss.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sdm670-camss.example.dtb: camss@ac65000: clock-names:8: 'csiphy0' was expected
	from schema $id: http://devicetree.org/schemas/media/qcom,sdm670-camss.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sdm670-camss.example.dtb: camss@ac65000: clock-names:9: 'csiphy0_timer' was expected
	from schema $id: http://devicetree.org/schemas/media/qcom,sdm670-camss.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sdm670-camss.example.dtb: camss@ac65000: clock-names:10: 'csiphy1' was expected
	from schema $id: http://devicetree.org/schemas/media/qcom,sdm670-camss.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sdm670-camss.example.dtb: camss@ac65000: clock-names:11: 'csiphy1_timer' was expected
	from schema $id: http://devicetree.org/schemas/media/qcom,sdm670-camss.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sdm670-camss.example.dtb: camss@ac65000: clock-names:12: 'csiphy2' was expected
	from schema $id: http://devicetree.org/schemas/media/qcom,sdm670-camss.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,sdm670-camss.example.dtb: camss@ac65000: clock-names:13: 'csiphy2_timer' was expected
	from schema $id: http://devicetree.org/schemas/media/qcom,sdm670-camss.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240904020448.52035-13-mailingradian@gmail.com

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

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

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v4 4/7] dt-bindings: media: camss: Add qcom,sdm670-camss

[Krzysztof Kozlowski]

On Tue, Sep 03, 2024 at 10:04:53PM -0400, Richard Acayan wrote:
> As found in the Pixel 3a, the Snapdragon 670 has a camera subsystem with
> 3 CSIDs and 3 VFEs (including 1 VFE lite). Add this camera subsystem to
> the bindings.
> 
> Adapted from SC8280XP camera subsystem.
> 
> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

This wasn't tested.

Best regards,
Krzysztof

Re: [PATCH v4 1/7] dt-bindings: clock: qcom,sdm845-camcc: add sdm670 compatible

[Krzysztof Kozlowski]

On Tue, Sep 03, 2024 at 10:04:50PM -0400, Richard Acayan wrote:
> The camera clocks on SDM670 and SDM845 have no significant differences
> that would require a change in the clock controller driver. The only
> difference is the clock frequency at each level of the power domains,
> which is not specified in the clock driver. There should still be a
> compatible specific to the SoC, so add the compatible for SDM670 with
> the SDM845 compatible as fallback.
> 
> Link: https://android.googlesource.com/kernel/msm/+/d4dc50c0a9291bd99895d4844f973421c047d267/drivers/clk/qcom/camcc-sdm845.c#2048
> Suggested-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> Suggested-by: Konrad Dybcio <konradybcio@kernel.org>
> Link: https://lore.kernel.org/linux-arm-msm/7d26a62b-b898-4737-bd53-f49821e3b471@linaro.org
> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
> ---

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v4 2/7] dt-bindings: i2c: qcom-cci: Document SDM670 compatible

[Krzysztof Kozlowski]

On Tue, Sep 03, 2024 at 10:04:51PM -0400, Richard Acayan wrote:
> The CCI on the Snapdragon 670 is the interface for controlling camera
> hardware over I2C. Add the compatible so it can be added to the SDM670
> device tree.
> 
> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
> Reviewed-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>  .../devicetree/bindings/i2c/qcom,i2c-cci.yaml | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 

I am going to skip this one, because previous patch was not tested. Let
us know if this one was tested.

Best regards,
Krzysztof

Re: [PATCH v4 3/7] i2c: qcom-cci: Stop complaining about DT set clock rate

[Konrad Dybcio]

On 4.09.2024 4:04 AM, Richard Acayan wrote:
> From: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> 
> It is common practice in the downstream and upstream CCI dt to set CCI
> clock rates to 19.2 MHz. It appears to be fairly common for initial code to
> set the CCI clock rate to 37.5 MHz.
> 
> Applying the widely used CCI clock rates from downstream ought not to cause
> warning messages in the upstream kernel where our general policy is to
> usually copy downstream hardware clock rates across the range of Qualcomm
> drivers.
> 
> Drop the warning it is pervasive across CAMSS users but doesn't add any
> information or warrant any changes to the DT to align the DT clock rate to
> the bootloader clock rate.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Reviewed-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> Link: https://lore.kernel.org/linux-arm-msm/20240824115900.40702-1-bryan.odonoghue@linaro.org
> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
> ---

I.. am not sure this is really a problem? On some platforms the core
clock is only 19.2 Mhz, but e.g. on sdm845 we have:

static const struct freq_tbl ftbl_cam_cc_cci_clk_src[] = {
        F(19200000, P_BI_TCXO, 1, 0, 0),
        F(37500000, P_CAM_CC_PLL0_OUT_EVEN, 16, 0, 0),
        F(50000000, P_CAM_CC_PLL0_OUT_EVEN, 12, 0, 0),
        F(100000000, P_CAM_CC_PLL0_OUT_EVEN, 6, 0, 0),
        { }
};

Shouldn't this be somehow dynamically calculated?

Konrad

Re: [PATCH v4 3/7] i2c: qcom-cci: Stop complaining about DT set clock rate

[Vladimir Zapolskiy]

Hi Konrad,

On 9/5/24 16:57, Konrad Dybcio wrote:
> On 4.09.2024 4:04 AM, Richard Acayan wrote:
>> From: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>
>> It is common practice in the downstream and upstream CCI dt to set CCI
>> clock rates to 19.2 MHz. It appears to be fairly common for initial code to
>> set the CCI clock rate to 37.5 MHz.
>>
>> Applying the widely used CCI clock rates from downstream ought not to cause
>> warning messages in the upstream kernel where our general policy is to
>> usually copy downstream hardware clock rates across the range of Qualcomm
>> drivers.
>>
>> Drop the warning it is pervasive across CAMSS users but doesn't add any
>> information or warrant any changes to the DT to align the DT clock rate to
>> the bootloader clock rate.
>>
>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> Reviewed-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
>> Link: https://lore.kernel.org/linux-arm-msm/20240824115900.40702-1-bryan.odonoghue@linaro.org
>> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
>> ---
> 
> I.. am not sure this is really a problem? On some platforms the core
> clock is only 19.2 Mhz, but e.g. on sdm845 we have:
> 
> static const struct freq_tbl ftbl_cam_cc_cci_clk_src[] = {
>          F(19200000, P_BI_TCXO, 1, 0, 0),
>          F(37500000, P_CAM_CC_PLL0_OUT_EVEN, 16, 0, 0),
>          F(50000000, P_CAM_CC_PLL0_OUT_EVEN, 12, 0, 0),
>          F(100000000, P_CAM_CC_PLL0_OUT_EVEN, 6, 0, 0),
>          { }
> };
> 
> Shouldn't this be somehow dynamically calculated?
> 

I believe the problem fixed by the change is an unnecessary dev_warn(), in
addition it's unclear why the CCI clock rate shall be strictly 37500000 for
all "CCI v2" platforms.

If the latter is a necessity, then it would be better to set the rate
explicitly, however since it's not done for any such platforms, I would say
that it is not needed.

And if it is not needed, or a default and universal 19.2MHz rate is good
enough, then I would suggest to remove everything 'cci_clk_rate' related
from the driver, and this makes the change incomplete...

--
Best wishes,
Vladimir

Re: [PATCH v4 4/7] dt-bindings: media: camss: Add qcom,sdm670-camss

[Vladimir Zapolskiy]

Hello Richard.

On 9/4/24 05:04, Richard Acayan wrote:
> As found in the Pixel 3a, the Snapdragon 670 has a camera subsystem with
> 3 CSIDs and 3 VFEs (including 1 VFE lite). Add this camera subsystem to
> the bindings.
> 
> Adapted from SC8280XP camera subsystem.
> 
> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

<snip>

> +
> +            interrupts = <GIC_SPI 464 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 466 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 468 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 477 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 478 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 479 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 465 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 467 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 469 IRQ_TYPE_LEVEL_HIGH>;

I suppose it should be IRQ_TYPE_EDGE_RISING type for all interrupts,
please correct it here and in the dts change.

Here I rely on the interrupt type requested in the camss driver.

> +            interrupt-names = "csid0",
> +                              "csid1",
> +                              "csid2",
> +                              "csiphy0",
> +                              "csiphy1",
> +                              "csiphy2",
> +                              "vfe0",
> +                              "vfe1",
> +                              "vfe_lite";

--
Best wishes,
Vladimir

Re: [PATCH v4 3/7] i2c: qcom-cci: Stop complaining about DT set clock rate

[Konrad Dybcio]

On 5.09.2024 4:18 PM, Vladimir Zapolskiy wrote:
> Hi Konrad,
> 
> On 9/5/24 16:57, Konrad Dybcio wrote:
>> On 4.09.2024 4:04 AM, Richard Acayan wrote:
>>> From: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>>
>>> It is common practice in the downstream and upstream CCI dt to set CCI
>>> clock rates to 19.2 MHz. It appears to be fairly common for initial code to
>>> set the CCI clock rate to 37.5 MHz.
>>>
>>> Applying the widely used CCI clock rates from downstream ought not to cause
>>> warning messages in the upstream kernel where our general policy is to
>>> usually copy downstream hardware clock rates across the range of Qualcomm
>>> drivers.
>>>
>>> Drop the warning it is pervasive across CAMSS users but doesn't add any
>>> information or warrant any changes to the DT to align the DT clock rate to
>>> the bootloader clock rate.
>>>
>>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>> Reviewed-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
>>> Link: https://lore.kernel.org/linux-arm-msm/20240824115900.40702-1-bryan.odonoghue@linaro.org
>>> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
>>> ---
>>
>> I.. am not sure this is really a problem? On some platforms the core
>> clock is only 19.2 Mhz, but e.g. on sdm845 we have:
>>
>> static const struct freq_tbl ftbl_cam_cc_cci_clk_src[] = {
>>          F(19200000, P_BI_TCXO, 1, 0, 0),
>>          F(37500000, P_CAM_CC_PLL0_OUT_EVEN, 16, 0, 0),
>>          F(50000000, P_CAM_CC_PLL0_OUT_EVEN, 12, 0, 0),
>>          F(100000000, P_CAM_CC_PLL0_OUT_EVEN, 6, 0, 0),
>>          { }
>> };
>>
>> Shouldn't this be somehow dynamically calculated?
>>
> 
> I believe the problem fixed by the change is an unnecessary dev_warn(), in
> addition it's unclear why the CCI clock rate shall be strictly 37500000 for
> all "CCI v2" platforms.
> 
> If the latter is a necessity, then it would be better to set the rate
> explicitly, however since it's not done for any such platforms, I would say
> that it is not needed.
> 
> And if it is not needed, or a default and universal 19.2MHz rate is good
> enough, then I would suggest to remove everything 'cci_clk_rate' related
> from the driver, and this makes the change incomplete...

Downstream seems to set 37500000 for all SoCs with Titan.. but I can't
find a good reason for this.. I'll try to get some answers.

Konrad

Re: [PATCH v4 3/7] i2c: qcom-cci: Stop complaining about DT set clock rate

[Bryan O'Donoghue]

On 05/09/2024 14:57, Konrad Dybcio wrote:
> On 4.09.2024 4:04 AM, Richard Acayan wrote:
>> From: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>
>> It is common practice in the downstream and upstream CCI dt to set CCI
>> clock rates to 19.2 MHz. It appears to be fairly common for initial code to
>> set the CCI clock rate to 37.5 MHz.
>>
>> Applying the widely used CCI clock rates from downstream ought not to cause
>> warning messages in the upstream kernel where our general policy is to
>> usually copy downstream hardware clock rates across the range of Qualcomm
>> drivers.
>>
>> Drop the warning it is pervasive across CAMSS users but doesn't add any
>> information or warrant any changes to the DT to align the DT clock rate to
>> the bootloader clock rate.
>>
>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> Reviewed-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
>> Link: https://lore.kernel.org/linux-arm-msm/20240824115900.40702-1-bryan.odonoghue@linaro.org
>> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
>> ---
> 
> I.. am not sure this is really a problem? On some platforms the core
> clock is only 19.2 Mhz, but e.g. on sdm845 we have:
> 
> static const struct freq_tbl ftbl_cam_cc_cci_clk_src[] = {
>          F(19200000, P_BI_TCXO, 1, 0, 0),
>          F(37500000, P_CAM_CC_PLL0_OUT_EVEN, 16, 0, 0),
>          F(50000000, P_CAM_CC_PLL0_OUT_EVEN, 12, 0, 0),
>          F(100000000, P_CAM_CC_PLL0_OUT_EVEN, 6, 0, 0),
>          { }
> };

CCI latches the code from DT and I assume that people submitting dts 
have actually tested their sensors when they do so.

The complaint about not being 19.2 MHz is surely not valid since, it can 
be any number of frequencies.

Its a redundant and useless warning.

We can do extra work to align to a set of frequencies sure but, the 
warning is not a warning about a real thing.

---
bod

Re: [PATCH v4 0/7] Add SDM670 camera subsystem

[Andi Shyti]

Hi Richard,

On Tue, Sep 03, 2024 at 10:04:49PM GMT, Richard Acayan wrote:
> This adds support for the camera subsystem on the Snapdragon 670.
> 
> As of next-20240902, camss seems to be a bit broken, but the same series
> works on stable (although it is much less reliable now that the CCI clock
> frequency is not being assigned).

I am not understanding this bit: is this series making it better
or not? Can you please clarify what is broken, what is less
reliable and what works?

Besides, I'm reading that this series has not been tested and it
makes it difficult for me to take this in, considering that you
are adding a new support.

Thanks,
Andi

Re: [PATCH v4 0/7] Add SDM670 camera subsystem

[Bryan O'Donoghue]

On 05/09/2024 21:09, Andi Shyti wrote:
> Hi Richard,
> 
> On Tue, Sep 03, 2024 at 10:04:49PM GMT, Richard Acayan wrote:
>> This adds support for the camera subsystem on the Snapdragon 670.
>>
>> As of next-20240902, camss seems to be a bit broken, but the same series
>> works on stable (although it is much less reliable now that the CCI clock
>> frequency is not being assigned).
> 
> I am not understanding this bit: is this series making it better
> or not? Can you please clarify what is broken, what is less
> reliable and what works?
> 
> Besides, I'm reading that this series has not been tested and it
> makes it difficult for me to take this in, considering that you
> are adding a new support.
> 
> Thanks,
> Andi

Actually I completely missed the statement about "much less reliable not 
that the CCI clock frequency is not being assigned"

@Richard - what does that mean ?

---
bod

Re: [PATCH v4 0/7] Add SDM670 camera subsystem

[Vladimir Zapolskiy]

On 9/4/24 05:04, Richard Acayan wrote:
> This adds support for the camera subsystem on the Snapdragon 670.
> 
> As of next-20240902, camss seems to be a bit broken, but the same series
> works on stable (although it is much less reliable now that the CCI clock
> frequency is not being assigned).
> 

Second that, please elaborate on "a bit broken" camss.

Regarding the CCI clock frequency, it's a supply clock and it is kind of
unconnected to the CAMSS driver, here I would expect that some particular
clock frequency setting either works always or does not work at all without
anything in the middle.

--
Best wishes,
Vladimir

Re: [PATCH v4 0/7] Add SDM670 camera subsystem

[Richard Acayan]

On Thu, Sep 05, 2024 at 10:09:34PM +0200, Andi Shyti wrote:
> Hi Richard,
> 
> On Tue, Sep 03, 2024 at 10:04:49PM GMT, Richard Acayan wrote:
> > This adds support for the camera subsystem on the Snapdragon 670.
> > 
> > As of next-20240902, camss seems to be a bit broken, but the same series
> > works on stable (although it is much less reliable now that the CCI clock
> > frequency is not being assigned).
> 
> I am not understanding this bit: is this series making it better
> or not? Can you please clarify what is broken, what is less
> reliable and what works?

When applying this camss series and some camera sensor patches on
linux-next, the Pixel 3a seems to hang when camera capture starts.

When applying the same patches on stable, the camera does not cause the
Pixel 3a to hang.

When these device tree properties from the previous series were removed:

			assigned-clocks = <&camcc CAM_CC_CCI_CLK>;
			assigned-clock-rates = <37500000>;

the CCI would sometimes fail to probe with the error:

	[   51.572732] i2c-qcom-cci ac4a000.cci: deferred probe timeout, ignoring dependency
	[   51.572769] i2c-qcom-cci ac4a000.cci: probe with driver i2c-qcom-cci failed with error -110

On further testing, the rate can be set to 19.2 MHz, and there would be
no failure (or rather, it wouldn't happen often enough for me to witness
it).

> Besides, I'm reading that this series has not been tested and it
> makes it difficult for me to take this in, considering that you
> are adding a new support.

Of course. This revision of the series wasn't submitted to rush into
v6.12-rc1. It can wait until everything is resolved.

When device tree maintainers comment "not tested" on the documentation,
it usually means that `make dt_bindings_check DT_SCHEMA_FILES=...` gives
errors or warnings (even though the device tree and driver may work on
the hardware). It's a separate test and one of the things I haven't
scripted into my workflow, although it's still a responsibility.

Re: [PATCH v4 0/7] Add SDM670 camera subsystem

[Andi Shyti]

Hi Richard,

On Thu, Sep 05, 2024 at 10:36:28PM GMT, Richard Acayan wrote:
> On Thu, Sep 05, 2024 at 10:09:34PM +0200, Andi Shyti wrote:
> > Hi Richard,
> > 
> > On Tue, Sep 03, 2024 at 10:04:49PM GMT, Richard Acayan wrote:
> > > This adds support for the camera subsystem on the Snapdragon 670.
> > > 
> > > As of next-20240902, camss seems to be a bit broken, but the same series
> > > works on stable (although it is much less reliable now that the CCI clock
> > > frequency is not being assigned).
> > 
> > I am not understanding this bit: is this series making it better
> > or not? Can you please clarify what is broken, what is less
> > reliable and what works?
> 
> When applying this camss series and some camera sensor patches on
> linux-next, the Pixel 3a seems to hang when camera capture starts.
> 
> When applying the same patches on stable, the camera does not cause the
> Pixel 3a to hang.
> 
> When these device tree properties from the previous series were removed:
> 
> 			assigned-clocks = <&camcc CAM_CC_CCI_CLK>;
> 			assigned-clock-rates = <37500000>;
> 
> the CCI would sometimes fail to probe with the error:
> 
> 	[   51.572732] i2c-qcom-cci ac4a000.cci: deferred probe timeout, ignoring dependency
> 	[   51.572769] i2c-qcom-cci ac4a000.cci: probe with driver i2c-qcom-cci failed with error -110
> 
> On further testing, the rate can be set to 19.2 MHz, and there would be
> no failure (or rather, it wouldn't happen often enough for me to witness
> it).
> 
> > Besides, I'm reading that this series has not been tested and it
> > makes it difficult for me to take this in, considering that you
> > are adding a new support.
> 
> Of course. This revision of the series wasn't submitted to rush into
> v6.12-rc1. It can wait until everything is resolved.
> 
> When device tree maintainers comment "not tested" on the documentation,
> it usually means that `make dt_bindings_check DT_SCHEMA_FILES=...` gives
> errors or warnings (even though the device tree and driver may work on
> the hardware). It's a separate test and one of the things I haven't
> scripted into my workflow, although it's still a responsibility.

OK, thanks for clarifying. Then, please, next time, to avoid
confusion, make it an RFC; or, if the series is in an advanced
state with little things to improve, state it clearly in the
cover letter or after the '---' section.

For now, thanks a lot, I will keep the R-b's for the time being
(unless the reviewers are against) and I will wait for you to
know the outcome of the tests.

Andi

Re: [PATCH v4 0/7] Add SDM670 camera subsystem

[Bryan O'Donoghue]

On 06/09/2024 03:36, Richard Acayan wrote:
> On Thu, Sep 05, 2024 at 10:09:34PM +0200, Andi Shyti wrote:
>> Hi Richard,
>>
>> On Tue, Sep 03, 2024 at 10:04:49PM GMT, Richard Acayan wrote:
>>> This adds support for the camera subsystem on the Snapdragon 670.
>>>
>>> As of next-20240902, camss seems to be a bit broken, but the same series
>>> works on stable (although it is much less reliable now that the CCI clock
>>> frequency is not being assigned).
>>
>> I am not understanding this bit: is this series making it better
>> or not? Can you please clarify what is broken, what is less
>> reliable and what works?
> 
> When applying this camss series and some camera sensor patches on
> linux-next, the Pixel 3a seems to hang when camera capture starts.
> 
> When applying the same patches on stable, the camera does not cause the
> Pixel 3a to hang.

Right so -next isn't stable that's not exactly a revelation.


> When these device tree properties from the previous series were removed:
> 
> 			assigned-clocks = <&camcc CAM_CC_CCI_CLK>;
> 			assigned-clock-rates = <37500000>;
> 
> the CCI would sometimes fail to probe with the error:

Right, we don't have clk_set_rate in the cci driver.

Maybe just leave the assigned clock for this submission and we can do a 
sweep of fixes to CCI at a later stage including setting the clock 
instead of having it be assigned.

> 
> 	[   51.572732] i2c-qcom-cci ac4a000.cci: deferred probe timeout, ignoring dependency
> 	[   51.572769] i2c-qcom-cci ac4a000.cci: probe with driver i2c-qcom-cci failed with error -110
> 
> On further testing, the rate can be set to 19.2 MHz, and there would be
> no failure (or rather, it wouldn't happen often enough for me to witness
> it).

That's expected 19.2 and 37.5 MHz are supported by CAMCC for your part.

---
bod

Re: [PATCH v4 0/7] Add SDM670 camera subsystem

[Vladimir Zapolskiy]

Hi Bryan, Richard,

On 9/6/24 15:19, Bryan O'Donoghue wrote:
> On 06/09/2024 03:36, Richard Acayan wrote:
>> On Thu, Sep 05, 2024 at 10:09:34PM +0200, Andi Shyti wrote:
>>> Hi Richard,
>>>
>>> On Tue, Sep 03, 2024 at 10:04:49PM GMT, Richard Acayan wrote:
>>>> This adds support for the camera subsystem on the Snapdragon 670.
>>>>
>>>> As of next-20240902, camss seems to be a bit broken, but the same series
>>>> works on stable (although it is much less reliable now that the CCI clock
>>>> frequency is not being assigned).
>>>
>>> I am not understanding this bit: is this series making it better
>>> or not? Can you please clarify what is broken, what is less
>>> reliable and what works?
>>
>> When applying this camss series and some camera sensor patches on
>> linux-next, the Pixel 3a seems to hang when camera capture starts.
>>
>> When applying the same patches on stable, the camera does not cause the
>> Pixel 3a to hang.
> 
> Right so -next isn't stable that's not exactly a revelation.
> 
> 
>> When these device tree properties from the previous series were removed:
>>
>> 			assigned-clocks = <&camcc CAM_CC_CCI_CLK>;
>> 			assigned-clock-rates = <37500000>;
>>
>> the CCI would sometimes fail to probe with the error:
> 
> Right, we don't have clk_set_rate in the cci driver.
> 
> Maybe just leave the assigned clock for this submission and we can do a
> sweep of fixes to CCI at a later stage including setting the clock
> instead of having it be assigned.

first of all it would be nice to confirm that the setting of a particular
clock frequency is actually needed.

Fortunately it's pretty trivial to check it in runtime with a temporary
modification in the board dts file, namely disable CAMSS in board dts file,
but keep CCI enabled, then simply scan the bus with a regular "i2cdetect"
tool in runtime.

If i2cdetect on the CCI bus works only for 37.5MHz clock frequency, then it
is needed, otherwise (and this is my expectation) it is not needed neither
in the dtsi files nor in the driver.

>>
>> 	[   51.572732] i2c-qcom-cci ac4a000.cci: deferred probe timeout, ignoring dependency
>> 	[   51.572769] i2c-qcom-cci ac4a000.cci: probe with driver i2c-qcom-cci failed with error -110
>>
>> On further testing, the rate can be set to 19.2 MHz, and there would be
>> no failure (or rather, it wouldn't happen often enough for me to witness
>> it).
> 
> That's expected 19.2 and 37.5 MHz are supported by CAMCC for your part.
>

I read it as the setting of 37.5MHz clock frequency is not needed, please
correct me.

--
Best wishes,
Vladimir

Re: [PATCH v4 0/7] Add SDM670 camera subsystem

[Richard Acayan]

On Fri, Sep 06, 2024 at 04:00:32PM +0300, Vladimir Zapolskiy wrote:
> Hi Bryan, Richard,
> 
> On 9/6/24 15:19, Bryan O'Donoghue wrote:
> > On 06/09/2024 03:36, Richard Acayan wrote:
> > > On Thu, Sep 05, 2024 at 10:09:34PM +0200, Andi Shyti wrote:
> > > > Hi Richard,
> > > > 
> > > > On Tue, Sep 03, 2024 at 10:04:49PM GMT, Richard Acayan wrote:
> > > > > This adds support for the camera subsystem on the Snapdragon 670.
> > > > > 
> > > > > As of next-20240902, camss seems to be a bit broken, but the same series
> > > > > works on stable (although it is much less reliable now that the CCI clock
> > > > > frequency is not being assigned).
> > > > 
> > > > I am not understanding this bit: is this series making it better
> > > > or not? Can you please clarify what is broken, what is less
> > > > reliable and what works?
> > > 
> > > When applying this camss series and some camera sensor patches on
> > > linux-next, the Pixel 3a seems to hang when camera capture starts.
> > > 
> > > When applying the same patches on stable, the camera does not cause the
> > > Pixel 3a to hang.
> > 
> > Right so -next isn't stable that's not exactly a revelation.
> > 
> > 
> > > When these device tree properties from the previous series were removed:
> > > 
> > > 			assigned-clocks = <&camcc CAM_CC_CCI_CLK>;
> > > 			assigned-clock-rates = <37500000>;
> > > 
> > > the CCI would sometimes fail to probe with the error:
> > 
> > Right, we don't have clk_set_rate in the cci driver.
> > 
> > Maybe just leave the assigned clock for this submission and we can do a
> > sweep of fixes to CCI at a later stage including setting the clock
> > instead of having it be assigned.
> 
> first of all it would be nice to confirm that the setting of a particular
> clock frequency is actually needed.
> 
> Fortunately it's pretty trivial to check it in runtime with a temporary
> modification in the board dts file, namely disable CAMSS in board dts file,
> but keep CCI enabled, then simply scan the bus with a regular "i2cdetect"
> tool in runtime.
> 
> If i2cdetect on the CCI bus works only for 37.5MHz clock frequency, then it
> is needed, otherwise (and this is my expectation) it is not needed neither
> in the dtsi files nor in the driver.
> 
> > > 
> > > 	[   51.572732] i2c-qcom-cci ac4a000.cci: deferred probe timeout, ignoring dependency
> > > 	[   51.572769] i2c-qcom-cci ac4a000.cci: probe with driver i2c-qcom-cci failed with error -110
> > > 
> > > On further testing, the rate can be set to 19.2 MHz, and there would be
> > > no failure (or rather, it wouldn't happen often enough for me to witness
> > > it).
> > 
> > That's expected 19.2 and 37.5 MHz are supported by CAMCC for your part.
> > 
> 
> I read it as the setting of 37.5MHz clock frequency is not needed, please
> correct me.

It is not. My test setup just needs specific EPROBE_DEFER behaviour
(my setup being postmarketOS with a full-disk encryption password prompt
and camcc-sdm845 loaded after mounting the root filesystem).

In drivers/base/platform.c, the platform_probe() function calls
of_clk_set_defaults() then dev_pm_domain_attach() prior to probing the
driver:

	static int platform_probe(struct device *_dev)
	{
		...
		ret = of_clk_set_defaults(_dev->of_node, false);
		if (ret < 0)
			return ret;
	
		ret = dev_pm_domain_attach(_dev, true);
		if (ret)
			goto out;
	
		if (drv->probe) {
			ret = drv->probe(dev);
			if (ret)
				dev_pm_domain_detach(_dev, true);
		}
		...
	}

When handling the assigned-clock-rates property,
of_clk_get_hw_from_clkspec() eventually returns ERR_PTR(-EPROBE_DEFER),
being propagated all the way.

When handling the power-domains property (if not avoided by deferring
with the assigned clock), __genpd_dev_pm_attach() returns a value
returned by driver_deferred_probe_check_state(), which is immediately
-ETIMEDOUT.

Re: [PATCH v4 0/7] Add SDM670 camera subsystem

[Vladimir Zapolskiy]

Hi Richard.

On 9/28/24 01:23, Richard Acayan wrote:
> On Fri, Sep 06, 2024 at 04:00:32PM +0300, Vladimir Zapolskiy wrote:
>> Hi Bryan, Richard,
>>
>> On 9/6/24 15:19, Bryan O'Donoghue wrote:
>>> On 06/09/2024 03:36, Richard Acayan wrote:
>>>> On Thu, Sep 05, 2024 at 10:09:34PM +0200, Andi Shyti wrote:
>>>>> Hi Richard,
>>>>>
>>>>> On Tue, Sep 03, 2024 at 10:04:49PM GMT, Richard Acayan wrote:
>>>>>> This adds support for the camera subsystem on the Snapdragon 670.
>>>>>>
>>>>>> As of next-20240902, camss seems to be a bit broken, but the same series
>>>>>> works on stable (although it is much less reliable now that the CCI clock
>>>>>> frequency is not being assigned).
>>>>>
>>>>> I am not understanding this bit: is this series making it better
>>>>> or not? Can you please clarify what is broken, what is less
>>>>> reliable and what works?
>>>>
>>>> When applying this camss series and some camera sensor patches on
>>>> linux-next, the Pixel 3a seems to hang when camera capture starts.
>>>>
>>>> When applying the same patches on stable, the camera does not cause the
>>>> Pixel 3a to hang.
>>>
>>> Right so -next isn't stable that's not exactly a revelation.
>>>
>>>
>>>> When these device tree properties from the previous series were removed:
>>>>
>>>> 			assigned-clocks = <&camcc CAM_CC_CCI_CLK>;
>>>> 			assigned-clock-rates = <37500000>;
>>>>
>>>> the CCI would sometimes fail to probe with the error:
>>>
>>> Right, we don't have clk_set_rate in the cci driver.
>>>
>>> Maybe just leave the assigned clock for this submission and we can do a
>>> sweep of fixes to CCI at a later stage including setting the clock
>>> instead of having it be assigned.
>>
>> first of all it would be nice to confirm that the setting of a particular
>> clock frequency is actually needed.
>>
>> Fortunately it's pretty trivial to check it in runtime with a temporary
>> modification in the board dts file, namely disable CAMSS in board dts file,
>> but keep CCI enabled, then simply scan the bus with a regular "i2cdetect"
>> tool in runtime.
>>
>> If i2cdetect on the CCI bus works only for 37.5MHz clock frequency, then it
>> is needed, otherwise (and this is my expectation) it is not needed neither
>> in the dtsi files nor in the driver.
>>
>>>>
>>>> 	[   51.572732] i2c-qcom-cci ac4a000.cci: deferred probe timeout, ignoring dependency
>>>> 	[   51.572769] i2c-qcom-cci ac4a000.cci: probe with driver i2c-qcom-cci failed with error -110
>>>>
>>>> On further testing, the rate can be set to 19.2 MHz, and there would be
>>>> no failure (or rather, it wouldn't happen often enough for me to witness
>>>> it).
>>>
>>> That's expected 19.2 and 37.5 MHz are supported by CAMCC for your part.
>>>
>>
>> I read it as the setting of 37.5MHz clock frequency is not needed, please
>> correct me.
> 
> It is not. My test setup just needs specific EPROBE_DEFER behaviour
> (my setup being postmarketOS with a full-disk encryption password prompt
> and camcc-sdm845 loaded after mounting the root filesystem).

Good, let the assigned clock frequency be dropped from the dtsi file then.

> In drivers/base/platform.c, the platform_probe() function calls
> of_clk_set_defaults() then dev_pm_domain_attach() prior to probing the
> driver:
> 
> 	static int platform_probe(struct device *_dev)
> 	{
> 		...
> 		ret = of_clk_set_defaults(_dev->of_node, false);
> 		if (ret < 0)
> 			return ret;
> 	
> 		ret = dev_pm_domain_attach(_dev, true);
> 		if (ret)
> 			goto out;
> 	
> 		if (drv->probe) {
> 			ret = drv->probe(dev);
> 			if (ret)
> 				dev_pm_domain_detach(_dev, true);
> 		}
> 		...
> 	}
> 
> When handling the assigned-clock-rates property,
> of_clk_get_hw_from_clkspec() eventually returns ERR_PTR(-EPROBE_DEFER),
> being propagated all the way.
> 
> When handling the power-domains property (if not avoided by deferring
> with the assigned clock), __genpd_dev_pm_attach() returns a value
> returned by driver_deferred_probe_check_state(), which is immediately
> -ETIMEDOUT.

I grasp it from the problem description, thank you for the explanation.

For sake of simplicity please make camcc-sdm845 as a built-in driver while
testing, it will allow to progress with the platform CAMSS support.

The issue with the observed ETIMEDOUT is generic and it's kind of unrelated
to the CCI/CAMSS support on SDM670.

--
Best wishes,
Vladimir