[PATCH v6 0/5] Add SDM670 camera subsystem

[PATCH v6 0/5] Add SDM670 camera subsystem

[Richard Acayan]

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

Changes since v5 (20241001023520.547271-9-mailingradian@gmail.com):
- sort reg and reg-names alphabetically (2/5, 5/5)
- drop CCI I2C patches since they are applied (formerly 2/7, 3/7)

Changes since v4 (20240904020448.52035-9-mailingradian@gmail.com):
- change camss interrupts to rising edge in dts (7/7)
- change IRQs to rising edge in camss dt-bindings example (4/7)
- move gcc and ahb clocks in camss dt-bindings example (4/7)
- add reviewed-by for camcc dt-bindings patch (1/7)

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)

Richard Acayan (5):
  dt-bindings: clock: qcom,sdm845-camcc: add 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 +-
 .../bindings/media/qcom,sdm670-camss.yaml     | 318 ++++++++++++++++++
 arch/arm64/boot/dts/qcom/sdm670.dtsi          | 195 +++++++++++
 drivers/media/platform/qcom/camss/camss.c     | 191 +++++++++++
 4 files changed, 709 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/media/qcom,sdm670-camss.yaml

-- 
2.47.0

[PATCH v6 1/5] 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>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../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.47.0

[PATCH v6 2/5] 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>
---
 .../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..670502532d28
--- /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: csid0
+      - const: csid1
+      - const: csid2
+      - const: csiphy0
+      - const: csiphy1
+      - const: csiphy2
+      - const: vfe0
+      - const: vfe1
+      - const: vfe_lite
+
+  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 0x0acb3000 0 0x1000>,
+                  <0 0x0acba000 0 0x1000>,
+                  <0 0x0acc8000 0 0x1000>,
+                  <0 0x0ac65000 0 0x1000>,
+                  <0 0x0ac66000 0 0x1000>,
+                  <0 0x0ac67000 0 0x1000>,
+                  <0 0x0acaf000 0 0x4000>,
+                  <0 0x0acb6000 0 0x4000>,
+                  <0 0x0acc4000 0 0x4000>;
+            reg-names = "csid0",
+                        "csid1",
+                        "csid2",
+                        "csiphy0",
+                        "csiphy1",
+                        "csiphy2",
+                        "vfe0",
+                        "vfe1",
+                        "vfe_lite";
+
+            interrupts = <GIC_SPI 464 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 466 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 468 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 477 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 478 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 479 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 465 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 467 IRQ_TYPE_EDGE_RISING>,
+                         <GIC_SPI 469 IRQ_TYPE_EDGE_RISING>;
+            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";
+
+            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.47.0

Re: [PATCH v6 2/5] dt-bindings: media: camss: Add qcom,sdm670-camss

[Vladimir Zapolskiy]

Hello Richard,

On 10/11/24 05:37, 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>
> ---
>   .../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..670502532d28
> --- /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: csid0
> +      - const: csid1
> +      - const: csid2
> +      - const: csiphy0
> +      - const: csiphy1
> +      - const: csiphy2
> +      - const: vfe0
> +      - const: vfe1
> +      - const: vfe_lite
> +
> +  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 0x0acb3000 0 0x1000>,

This is immediately wrong, unit address shall be the same as the address of the
first value of reg property.

I still object to the sorting order of reg values dictated by reg-names property.

There are a few recently added CAMSS device tree binding descriptions, where
reg values are sorted by address values without a connection to another property
values, and I believe this is the correct way to go.

Two most recently added CAMSS IP descriptions (qcom,sm8250-camss.yaml and
qcom,sc8280xp-camss.yaml) do implement sorting by reg values, I believe from now on
it should be assumed that all subsequently added CAMSS IP descriptions to follow
the same established policy.

I vote for it.

> +                  <0 0x0acba000 0 0x1000>,
> +                  <0 0x0acc8000 0 0x1000>,
> +                  <0 0x0ac65000 0 0x1000>,
> +                  <0 0x0ac66000 0 0x1000>,
> +                  <0 0x0ac67000 0 0x1000>,
> +                  <0 0x0acaf000 0 0x4000>,
> +                  <0 0x0acb6000 0 0x4000>,
> +                  <0 0x0acc4000 0 0x4000>;
> +            reg-names = "csid0",
> +                        "csid1",
> +                        "csid2",
> +                        "csiphy0",
> +                        "csiphy1",
> +                        "csiphy2",
> +                        "vfe0",
> +                        "vfe1",
> +                        "vfe_lite";
> +
> +            interrupts = <GIC_SPI 464 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 466 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 468 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 477 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 478 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 479 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 465 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 467 IRQ_TYPE_EDGE_RISING>,
> +                         <GIC_SPI 469 IRQ_TYPE_EDGE_RISING>;
> +            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";
> +
> +            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>;
> +                    };
> +                };
> +            };
> +        };
> +    };

--
Best wishes,
Vladimir

Re: [PATCH v6 2/5] dt-bindings: media: camss: Add qcom,sdm670-camss

[Bryan O'Donoghue]

On 11/10/2024 08:14, Vladimir Zapolskiy wrote:
> 
> Two most recently added CAMSS IP descriptions (qcom,sm8250-camss.yaml and
> qcom,sc8280xp-camss.yaml) do implement sorting by reg values, I believe 
> from now on
> it should be assumed that all subsequently added CAMSS IP descriptions 
> to follow
> the same established policy.

My preference is sort by address not sort by name => we sort the device 
nodes themselves by address so it seems more consistent to sort by 
address inside of the devices too.

Which means sorting reg by address and irq too.

---
bod

Re: [PATCH v6 2/5] dt-bindings: media: camss: Add qcom,sdm670-camss

[Rob Herring]

On Fri, Oct 11, 2024 at 09:31:06AM +0100, Bryan O'Donoghue wrote:
> On 11/10/2024 08:14, Vladimir Zapolskiy wrote:
> > 
> > Two most recently added CAMSS IP descriptions (qcom,sm8250-camss.yaml and
> > qcom,sc8280xp-camss.yaml) do implement sorting by reg values, I believe
> > from now on
> > it should be assumed that all subsequently added CAMSS IP descriptions
> > to follow
> > the same established policy.
> 
> My preference is sort by address not sort by name => we sort the device
> nodes themselves by address so it seems more consistent to sort by address
> inside of the devices too.

Strictly speaking, the values of addresses are unknown to the binding, 
so you can't sort by address. However, if something is truly a single 
block, then the offsets are probably fixed in order by offset makes 
sense. But when a block is changed, any rule on sorting may go out 
the window since we add new regions on the end.

This one in particular I have to wonder why csiphy is not a separate 
node.

> 
> Which means sorting reg by address and irq too.

IRQs make little sense to sort IMO.

Rob

Re: [PATCH v6 2/5] dt-bindings: media: camss: Add qcom,sdm670-camss

[Bryan O'Donoghue]

On 11/10/2024 15:41, Rob Herring wrote:
> But when a block is changed, any rule on sorting may go out
> the window since we add new regions on the end.

Ah I see, I didn't TBH know that.

> This one in particular I have to wonder why csiphy is not a separate
> node.

Hmm, to be honest with you Rob, even though I realise I'm digging myself 
into a hole of yet more work, yes - we should probably structure camss 
along the lines of mdss which has separate nodes for DSI phys.

-> mdss: display-subsystem@ae00000{}

We have 4 SoCs "in flight" at the moment.

sdm670 and sc7280 don't require any real driver update to facilitate.

sm8550 and x1e80100 do require new VFE, CSID etc.

We've been debating how to model the regulators for the CSIPHYs too 
which are on rails supplied by PMICs external to the SoC.

I'm congniscient of the fact 670, 7280 and to an extent sm8550 have been 
on the list for quite some time, so I'd rather not push 670 and 7280 to 
have to wait for a whole new way of doing CSIPHYs - especially because 
these are old hardware with little to no driver change required to support.

OTOH x1e80100 and sm8550 already need development work so we can do the 
CSIPHY transition there.

---
bod

Re: [PATCH v6 2/5] dt-bindings: media: camss: Add qcom,sdm670-camss

[Vladimir Zapolskiy]

Hi Rob.

On 10/11/24 17:41, Rob Herring wrote:
> On Fri, Oct 11, 2024 at 09:31:06AM +0100, Bryan O'Donoghue wrote:
>> On 11/10/2024 08:14, Vladimir Zapolskiy wrote:
>>>
>>> Two most recently added CAMSS IP descriptions (qcom,sm8250-camss.yaml and
>>> qcom,sc8280xp-camss.yaml) do implement sorting by reg values, I believe
>>> from now on
>>> it should be assumed that all subsequently added CAMSS IP descriptions
>>> to follow
>>> the same established policy.
>>
>> My preference is sort by address not sort by name => we sort the device
>> nodes themselves by address so it seems more consistent to sort by address
>> inside of the devices too.
> 
> Strictly speaking, the values of addresses are unknown to the binding,
> so you can't sort by address. However, if something is truly a single
> block, then the offsets are probably fixed in order by offset makes
> sense. But when a block is changed, any rule on sorting may go out
> the window since we add new regions on the end.

Exactly, and this is an argument why the sorting is a subject to a device
driver policy, kind of any sorting order is equally bad. Sorting 'reg'
values by addresses helps to avoid a notorious problem with unit addresses.

> This one in particular I have to wonder why csiphy is not a separate
> node.

There were dicussions about it in the past, and kind of enforced outcome of
the discussions is to keep all CAMSS IP components together under one huge
plain device tree node. I personally dislike this approach, but obedience
is the way to get things merged.

>>
>> Which means sorting reg by address and irq too.
> 
> IRQs make little sense to sort IMO.

For all non-reg properties with a present *-names property the sorting
order should be done by *-names property. Only 'reg' is very special.

--
Best wishes,
Vladimir

Re: [PATCH v6 2/5] dt-bindings: media: camss: Add qcom,sdm670-camss

[Rob Herring]

On Wed, Oct 30, 2024 at 9:20 AM Vladimir Zapolskiy
<vladimir.zapolskiy@linaro.org> wrote:
>
> Hi Rob.
>
> On 10/11/24 17:41, Rob Herring wrote:
> > On Fri, Oct 11, 2024 at 09:31:06AM +0100, Bryan O'Donoghue wrote:
> >> On 11/10/2024 08:14, Vladimir Zapolskiy wrote:
> >>>
> >>> Two most recently added CAMSS IP descriptions (qcom,sm8250-camss.yaml and
> >>> qcom,sc8280xp-camss.yaml) do implement sorting by reg values, I believe
> >>> from now on
> >>> it should be assumed that all subsequently added CAMSS IP descriptions
> >>> to follow
> >>> the same established policy.
> >>
> >> My preference is sort by address not sort by name => we sort the device
> >> nodes themselves by address so it seems more consistent to sort by address
> >> inside of the devices too.
> >
> > Strictly speaking, the values of addresses are unknown to the binding,
> > so you can't sort by address. However, if something is truly a single
> > block, then the offsets are probably fixed in order by offset makes
> > sense. But when a block is changed, any rule on sorting may go out
> > the window since we add new regions on the end.
>
> Exactly, and this is an argument why the sorting is a subject to a device
> driver policy, kind of any sorting order is equally bad. Sorting 'reg'
> values by addresses helps to avoid a notorious problem with unit addresses.

What notorious problem?

>
> > This one in particular I have to wonder why csiphy is not a separate
> > node.
>
> There were dicussions about it in the past, and kind of enforced outcome of
> the discussions is to keep all CAMSS IP components together under one huge
> plain device tree node. I personally dislike this approach, but obedience
> is the way to get things merged.

Who are you saying would be in the way to get things merged? DT
maintainers? I feel certain I would have pushed for separate blocks,
but I'll defer to people that know the h/w. I can't learn the details
of everyone's h/w. If they get it wrong, it's their problem not mine.

> >> Which means sorting reg by address and irq too.
> >
> > IRQs make little sense to sort IMO.
>
> For all non-reg properties with a present *-names property the sorting
> order should be done by *-names property. Only 'reg' is very special.

No. If you had 'main' and 'error', I'd put 'main' first. If they are
somewhat equal (e.g. rx, tx), then sure, sort them however you like
(assuming no existing binding). The only real rules here are how new
entries should be added (on the end). Otherwise, there is no policy.

Rob

Re: [PATCH v6 2/5] dt-bindings: media: camss: Add qcom,sdm670-camss

[Vladimir Zapolskiy]

On 10/30/24 23:06, Rob Herring wrote:
> On Wed, Oct 30, 2024 at 9:20 AM Vladimir Zapolskiy
> <vladimir.zapolskiy@linaro.org> wrote:
>>
>> Hi Rob.
>>
>> On 10/11/24 17:41, Rob Herring wrote:
>>> On Fri, Oct 11, 2024 at 09:31:06AM +0100, Bryan O'Donoghue wrote:
>>>> On 11/10/2024 08:14, Vladimir Zapolskiy wrote:
>>>>>
>>>>> Two most recently added CAMSS IP descriptions (qcom,sm8250-camss.yaml and
>>>>> qcom,sc8280xp-camss.yaml) do implement sorting by reg values, I believe
>>>>> from now on
>>>>> it should be assumed that all subsequently added CAMSS IP descriptions
>>>>> to follow
>>>>> the same established policy.
>>>>
>>>> My preference is sort by address not sort by name => we sort the device
>>>> nodes themselves by address so it seems more consistent to sort by address
>>>> inside of the devices too.
>>>
>>> Strictly speaking, the values of addresses are unknown to the binding,
>>> so you can't sort by address. However, if something is truly a single
>>> block, then the offsets are probably fixed in order by offset makes
>>> sense. But when a block is changed, any rule on sorting may go out
>>> the window since we add new regions on the end.
>>
>> Exactly, and this is an argument why the sorting is a subject to a device
>> driver policy, kind of any sorting order is equally bad. Sorting 'reg'
>> values by addresses helps to avoid a notorious problem with unit addresses.
> 
> What notorious problem?
> 

Here the problem I reference to is the problem of an incorrespondence between
device tree node unit address and the address of the first value of 'reg'
values.

Having a sorting by addresses allows to grasp IO ranges easily, and setting
device tree node unit addresses to some almost arbitrary chosen value from
the middle of IP's IO range is suspicious and confusing in my opinion.

>>
>>> This one in particular I have to wonder why csiphy is not a separate
>>> node.
>>
>> There were dicussions about it in the past, and kind of enforced outcome of
>> the discussions is to keep all CAMSS IP components together under one huge
>> plain device tree node. I personally dislike this approach, but obedience
>> is the way to get things merged.
> 
> Who are you saying would be in the way to get things merged? DT
> maintainers? I feel certain I would have pushed for separate blocks,
> but I'll defer to people that know the h/w. I can't learn the details
> of everyone's h/w. If they get it wrong, it's their problem not mine.

I had this discussion with Qualcomm/CAMSS maintainers long time ago, it
may be restarted, if there is a necessity.

>>>> Which means sorting reg by address and irq too.
>>>
>>> IRQs make little sense to sort IMO.
>>
>> For all non-reg properties with a present *-names property the sorting
>> order should be done by *-names property. Only 'reg' is very special.
> 
> No. If you had 'main' and 'error', I'd put 'main' first. If they are
> somewhat equal (e.g. rx, tx), then sure, sort them however you like
> (assuming no existing binding). The only real rules here are how new
> entries should be added (on the end). Otherwise, there is no policy.
> 

Here in the proposed terms the start of an IO region is 'main', while
some value in the middle of it (the first one in alphabetical sorting)
is too secondary to dictate the device tree node unit address, I believe.

--
Best wishes,
Vladimir

Re: [PATCH v6 2/5] dt-bindings: media: camss: Add qcom,sdm670-camss

[Krzysztof Kozlowski]

On 30/10/2024 23:13, Vladimir Zapolskiy wrote:
> On 10/30/24 23:06, Rob Herring wrote:
>> On Wed, Oct 30, 2024 at 9:20 AM Vladimir Zapolskiy
>> <vladimir.zapolskiy@linaro.org> wrote:
>>>
>>> Hi Rob.
>>>
>>> On 10/11/24 17:41, Rob Herring wrote:
>>>> On Fri, Oct 11, 2024 at 09:31:06AM +0100, Bryan O'Donoghue wrote:
>>>>> On 11/10/2024 08:14, Vladimir Zapolskiy wrote:
>>>>>>
>>>>>> Two most recently added CAMSS IP descriptions (qcom,sm8250-camss.yaml and
>>>>>> qcom,sc8280xp-camss.yaml) do implement sorting by reg values, I believe
>>>>>> from now on
>>>>>> it should be assumed that all subsequently added CAMSS IP descriptions
>>>>>> to follow
>>>>>> the same established policy.
>>>>>
>>>>> My preference is sort by address not sort by name => we sort the device
>>>>> nodes themselves by address so it seems more consistent to sort by address
>>>>> inside of the devices too.
>>>>
>>>> Strictly speaking, the values of addresses are unknown to the binding,
>>>> so you can't sort by address. However, if something is truly a single
>>>> block, then the offsets are probably fixed in order by offset makes
>>>> sense. But when a block is changed, any rule on sorting may go out
>>>> the window since we add new regions on the end.
>>>
>>> Exactly, and this is an argument why the sorting is a subject to a device
>>> driver policy, kind of any sorting order is equally bad. Sorting 'reg'
>>> values by addresses helps to avoid a notorious problem with unit addresses.
>>
>> What notorious problem?
>>
> 
> Here the problem I reference to is the problem of an incorrespondence between
> device tree node unit address and the address of the first value of 'reg'
> values.

Sorting by unit address does not solve it. Sorting by anything else does
not cause it. You repeat this statement multiple times already. Sorry,
it's not true. You can sort by address and still put wrong unit address.

Tools address it already, not sorting.

> 
> Having a sorting by addresses allows to grasp IO ranges easily, and setting
> device tree node unit addresses to some almost arbitrary chosen value from
> the middle of IP's IO range is suspicious and confusing in my opinion.

It's not arbitrary. It's the main, control address space often. At least
it should be. Your sorting rule enforces putting irrelevant address as
unit address, imagine:

isp@1000 {
reg = <0x1000 0x4>, <0x2000, 0x1000>;
reg-names = "reset-control-block-one-unimportant-register",
            "main-control-block-with-99%-of-registers";

Sorry, this is just wrong coding style.

Best regards,
Krzysztof

Re: [PATCH v6 2/5] dt-bindings: media: camss: Add qcom,sdm670-camss

[Krzysztof Kozlowski]

On Fri, Oct 11, 2024 at 10:14:49AM +0300, Vladimir Zapolskiy wrote:
> > +    soc {
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +
> > +        camss@ac65000 {
> > +            compatible = "qcom,sdm670-camss";
> > +
> > +            reg = <0 0x0acb3000 0 0x1000>,
> 
> This is immediately wrong, unit address shall be the same as the address of the
> first value of reg property.
> 
> I still object to the sorting order of reg values dictated by reg-names property.
> 
> There are a few recently added CAMSS device tree binding descriptions, where
> reg values are sorted by address values without a connection to another property
> values, and I believe this is the correct way to go.
> 
> Two most recently added CAMSS IP descriptions (qcom,sm8250-camss.yaml and
> qcom,sc8280xp-camss.yaml) do implement sorting by reg values, I believe from now on
> it should be assumed that all subsequently added CAMSS IP descriptions to follow
> the same established policy.

Heh, sc8280xp introduced entirely different sorting also in interrupt-names.

Just look at interrupts of sm8250 and sc8280xp. Luckily clocks are still
keeping style.

Can you start keeping consistency? All bindings from the same family of
devices, especially if they share something, should have similar order
in lists.

How do you imagine writing drivers and request items by order (not by
name) if the order is different in each flavor?

And such change to randomness in style - sometimes reg sorted that way,
sometimes other, interrupts sometimes like this or like that - should
not come from Linaro.

Best regards,
Krzysztof

Re: [PATCH v6 2/5] dt-bindings: media: camss: Add qcom,sdm670-camss

[Vladimir Zapolskiy]

Hi Krzysztof,

On 10/11/24 17:29, Krzysztof Kozlowski wrote:
> On Fri, Oct 11, 2024 at 10:14:49AM +0300, Vladimir Zapolskiy wrote:
>>> +    soc {
>>> +        #address-cells = <2>;
>>> +        #size-cells = <2>;
>>> +
>>> +        camss@ac65000 {
>>> +            compatible = "qcom,sdm670-camss";
>>> +
>>> +            reg = <0 0x0acb3000 0 0x1000>,
>>
>> This is immediately wrong, unit address shall be the same as the address of the
>> first value of reg property.
>>
>> I still object to the sorting order of reg values dictated by reg-names property.
>>
>> There are a few recently added CAMSS device tree binding descriptions, where
>> reg values are sorted by address values without a connection to another property
>> values, and I believe this is the correct way to go.
>>
>> Two most recently added CAMSS IP descriptions (qcom,sm8250-camss.yaml and
>> qcom,sc8280xp-camss.yaml) do implement sorting by reg values, I believe from now on
>> it should be assumed that all subsequently added CAMSS IP descriptions to follow
>> the same established policy.
> 
> Heh, sc8280xp introduced entirely different sorting also in interrupt-names.
> 
> Just look at interrupts of sm8250 and sc8280xp. Luckily clocks are still
> keeping style.
> 
> Can you start keeping consistency? All bindings from the same family of
> devices, especially if they share something, should have similar order
> in lists.
> 
> How do you imagine writing drivers and request items by order (not by
> name) if the order is different in each flavor?

I don't see a problem here, and I don't remember any reports about this
kind of problem while adding CAMSS support in the driver to new platforms.

While the problem of improper CAMSS unit address appears again and again,
the focus shall be on removing a chance to make a commin mistake here.

As I've already said above, device tree bindings of CAMSS in two most
recently added platforms sm8250 and sc8280xp follow the numerical order
of addresses from reg value. This becomes the policy.

Sorting lists of interrupts or clocks by numerical values makes no sense,
thus the argument of *-names sorting becomes valid here. For clarity, reg
property is very special, also a snippet of its value goes as a unit
address.

--
Best wishes,
Vladimir

Re: [PATCH v6 2/5] dt-bindings: media: camss: Add qcom,sdm670-camss

[Krzysztof Kozlowski]

On 30/10/2024 15:06, Vladimir Zapolskiy wrote:
> Hi Krzysztof,
> 
> On 10/11/24 17:29, Krzysztof Kozlowski wrote:
>> On Fri, Oct 11, 2024 at 10:14:49AM +0300, Vladimir Zapolskiy wrote:
>>>> +    soc {
>>>> +        #address-cells = <2>;
>>>> +        #size-cells = <2>;
>>>> +
>>>> +        camss@ac65000 {
>>>> +            compatible = "qcom,sdm670-camss";
>>>> +
>>>> +            reg = <0 0x0acb3000 0 0x1000>,
>>>
>>> This is immediately wrong, unit address shall be the same as the address of the
>>> first value of reg property.
>>>
>>> I still object to the sorting order of reg values dictated by reg-names property.
>>>
>>> There are a few recently added CAMSS device tree binding descriptions, where
>>> reg values are sorted by address values without a connection to another property
>>> values, and I believe this is the correct way to go.
>>>
>>> Two most recently added CAMSS IP descriptions (qcom,sm8250-camss.yaml and
>>> qcom,sc8280xp-camss.yaml) do implement sorting by reg values, I believe from now on
>>> it should be assumed that all subsequently added CAMSS IP descriptions to follow
>>> the same established policy.
>>
>> Heh, sc8280xp introduced entirely different sorting also in interrupt-names.
>>
>> Just look at interrupts of sm8250 and sc8280xp. Luckily clocks are still
>> keeping style.
>>
>> Can you start keeping consistency? All bindings from the same family of
>> devices, especially if they share something, should have similar order
>> in lists.
>>
>> How do you imagine writing drivers and request items by order (not by
>> name) if the order is different in each flavor?
> 
> I don't see a problem here, and I don't remember any reports about this
> kind of problem while adding CAMSS support in the driver to new platforms.

And I see problem, would create enormous probe code to handle different
variants for clock[0] and then clock[1], etc.

> 
> While the problem of improper CAMSS unit address appears again and again,
> the focus shall be on removing a chance to make a commin mistake here.

This is not a problem. Tools already point it out. Order of reg-names
also does not affect that, you can put fake unit address regardless of
the order of reg-names items.

> 
> As I've already said above, device tree bindings of CAMSS in two most
> recently added platforms sm8250 and sc8280xp follow the numerical order
> of addresses from reg value. This becomes the policy.
> 
> Sorting lists of interrupts or clocks by numerical values makes no sense,
> thus the argument of *-names sorting becomes valid here. For clarity, reg

There is no such argument, no such coding style.

> property is very special, also a snippet of its value goes as a unit
> address.

And order of items does not matter for above "specialness of reg".

Best regards,
Krzysztof

Re: [PATCH v6 2/5] dt-bindings: media: camss: Add qcom,sdm670-camss

[Bryan O'Donoghue]

On 11/10/2024 15:29, Krzysztof Kozlowski wrote:
> How do you imagine writing drivers and request items by order (not by
> name) if the order is different in each flavor?

I don't think I'd be much in favour of relying on declaration order in 
the dts, favouring names to find resources instead, tbh.

The 8250 has regs that sort by address and name in the same order. For 
8280xp we preferred sort by address and you're right the interrupt 
sorting isn't consistent.

However the latest applied dts for CAMSS is sort by address/irq not sort 
by reg-name irq-name.

Unless its a NAK from yourself and Rob, that would certainly be my 
preference for any _new_ additions subsequent.

---
bod

Re: [PATCH v6 2/5] dt-bindings: media: camss: Add qcom,sdm670-camss

[Krzysztof Kozlowski]

On 31/10/2024 16:42, Bryan O'Donoghue wrote:
> On 11/10/2024 15:29, Krzysztof Kozlowski wrote:
>> How do you imagine writing drivers and request items by order (not by
>> name) if the order is different in each flavor?
> 
> I don't think I'd be much in favour of relying on declaration order in 
> the dts, favouring names to find resources instead, tbh.
> 
> The 8250 has regs that sort by address and name in the same order. For 
> 8280xp we preferred sort by address and you're right the interrupt 
> sorting isn't consistent.
> 
> However the latest applied dts for CAMSS is sort by address/irq not sort 
> by reg-name irq-name.
> 
> Unless its a NAK from yourself and Rob, that would certainly be my 
> preference for any _new_ additions subsequent.

It's not a NAK as long you keep the same order in new bindings, which I
think it is not possible. I repeat myself: there is no rule/style that
list should be ordered by values, but there is a rule that all devices
from the same family should have the same order of items in the list. I
don't think it is achievable with your approach - sorting by value.

Best regards,
Krzysztof

Re: [PATCH v6 2/5] dt-bindings: media: camss: Add qcom,sdm670-camss

[Bryan O'Donoghue]

On 01/11/2024 09:17, Krzysztof Kozlowski wrote:
> On 31/10/2024 16:42, Bryan O'Donoghue wrote:
>> On 11/10/2024 15:29, Krzysztof Kozlowski wrote:
>>> How do you imagine writing drivers and request items by order (not by
>>> name) if the order is different in each flavor?
>>
>> I don't think I'd be much in favour of relying on declaration order in
>> the dts, favouring names to find resources instead, tbh.
>>
>> The 8250 has regs that sort by address and name in the same order. For
>> 8280xp we preferred sort by address and you're right the interrupt
>> sorting isn't consistent.
>>
>> However the latest applied dts for CAMSS is sort by address/irq not sort
>> by reg-name irq-name.
>>
>> Unless its a NAK from yourself and Rob, that would certainly be my
>> preference for any _new_ additions subsequent.
> 
> It's not a NAK as long you keep the same order in new bindings, which I
> think it is not possible. I repeat myself: there is no rule/style that
> list should be ordered by values, but there is a rule that all devices
> from the same family should have the same order of items in the list. I
> don't think it is achievable with your approach - sorting by value.

Grand.

I'm happy enough to sort by IP alpha TBH.

---
bod

Re: [PATCH v6 2/5] dt-bindings: media: camss: Add qcom,sdm670-camss

[Krzysztof Kozlowski]

On 01/11/2024 10:36, Bryan O'Donoghue wrote:
> On 01/11/2024 09:17, Krzysztof Kozlowski wrote:
>> On 31/10/2024 16:42, Bryan O'Donoghue wrote:
>>> On 11/10/2024 15:29, Krzysztof Kozlowski wrote:
>>>> How do you imagine writing drivers and request items by order (not by
>>>> name) if the order is different in each flavor?
>>>
>>> I don't think I'd be much in favour of relying on declaration order in
>>> the dts, favouring names to find resources instead, tbh.
>>>
>>> The 8250 has regs that sort by address and name in the same order. For
>>> 8280xp we preferred sort by address and you're right the interrupt
>>> sorting isn't consistent.
>>>
>>> However the latest applied dts for CAMSS is sort by address/irq not sort
>>> by reg-name irq-name.
>>>
>>> Unless its a NAK from yourself and Rob, that would certainly be my
>>> preference for any _new_ additions subsequent.
>>
>> It's not a NAK as long you keep the same order in new bindings, which I
>> think it is not possible. I repeat myself: there is no rule/style that
>> list should be ordered by values, but there is a rule that all devices
>> from the same family should have the same order of items in the list. I
>> don't think it is achievable with your approach - sorting by value.
> 
> Grand.
> 
> I'm happy enough to sort by IP alpha TBH.

I actually missed that Rob responded here clarifying his point of view.
Two of DT maintainers expressed their dislike for such coding style of
sorting by value.

Regardless whether it is helping or not, it is not even possible to
implement. Binding does not know the addresses and one could add a
binding without DTS and drivers for some other user or future
implementation. It's not even possible to implement that coding style.

Best regards,
Krzysztof

[PATCH v6 3/5] 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 d64985ca6e88..4694f5219654 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 */
 	{
@@ -2404,6 +2583,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,
@@ -2448,6 +2638,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.47.0

[PATCH v6 4/5] 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.47.0

[PATCH v6 5/5] 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..229d1c4eb246 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 0x0acb3000 0 0x1000>,
+			      <0 0x0acba000 0 0x1000>,
+			      <0 0x0acc8000 0 0x1000>,
+			      <0 0x0ac65000 0 0x1000>,
+			      <0 0x0ac66000 0 0x1000>,
+			      <0 0x0ac67000 0 0x1000>,
+			      <0 0x0acaf000 0 0x4000>,
+			      <0 0x0acb6000 0 0x4000>,
+			      <0 0x0acc4000 0 0x4000>;
+			reg-names = "csid0",
+				    "csid1",
+				    "csid2",
+				    "csiphy0",
+				    "csiphy1",
+				    "csiphy2",
+				    "vfe0",
+				    "vfe1",
+				    "vfe_lite";
+
+			interrupts = <GIC_SPI 464 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 466 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 468 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 477 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 478 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 479 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 465 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 467 IRQ_TYPE_EDGE_RISING>,
+				     <GIC_SPI 469 IRQ_TYPE_EDGE_RISING>;
+			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.47.0