[PATCH v2 0/4] Add CCI bus support for SM6350

[PATCH v2 0/4] Add CCI bus support for SM6350

[Luca Weiss]

Add the camera clock controller node and CCI nodes to sm6350 dtsi and enable
the i2c busses on Fairphone 4 dts.

This is tested using PM8008 regulator patches from the lists which power the
cameras, and using i2cdetect/i2cget/i2cset reading the sensor ID registers.

Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
---
Changes in v2:
- Correct ordering of attributes in sm6350.dtsi (#*-cells, pinctrl-names,
  bias-*)
- Link to v1: https://lore.kernel.org/r/20221213-sm6350-cci-v1-0-e5d0c36e0c4f@fairphone.com

---
Luca Weiss (4):
      dt-bindings: i2c: qcom-cci: Document SM6350 compatible
      arm64: dts: qcom: sm6350: Add camera clock controller
      arm64: dts: qcom: sm6350: Add CCI nodes
      arm64: dts: qcom: sm7225-fairphone-fp4: Enable CCI busses

 .../devicetree/bindings/i2c/qcom,i2c-cci.yaml      |   2 +
 arch/arm64/boot/dts/qcom/sm6350.dtsi               | 141 +++++++++++++++++++++
 arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts  |  20 +++
 3 files changed, 163 insertions(+)
---
base-commit: 1578f85d549045aac441821064e7953732460e51
change-id: 20221213-sm6350-cci-38baf19ace3b

Best regards,
-- 
Luca Weiss <luca.weiss@fairphone.com>

[PATCH v2 1/4] dt-bindings: i2c: qcom-cci: Document SM6350 compatible

[Luca Weiss]

Document the compatible for the CCI block found on SM6350 SoC.

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
---
 Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml b/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml
index 87e414f0c39c..ec79b7270437 100644
--- a/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml
+++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml
@@ -26,6 +26,7 @@ properties:
       - items:
           - enum:
               - qcom,sdm845-cci
+              - qcom,sm6350-cci
               - qcom,sm8250-cci
               - qcom,sm8450-cci
           - const: qcom,msm8996-cci # CCI v2
@@ -139,6 +140,7 @@ allOf:
           contains:
             enum:
               - qcom,sdm845-cci
+              - qcom,sm6350-cci
     then:
       properties:
         clocks:

-- 
2.39.1

[PATCH v2 2/4] arm64: dts: qcom: sm6350: Add camera clock controller

[Luca Weiss]

Add a node for the camcc found on SM6350 SoC.

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
---
 arch/arm64/boot/dts/qcom/sm6350.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sm6350.dtsi b/arch/arm64/boot/dts/qcom/sm6350.dtsi
index 8224adb99948..300ced5cda57 100644
--- a/arch/arm64/boot/dts/qcom/sm6350.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm6350.dtsi
@@ -1435,6 +1435,15 @@ usb_1_dwc3: usb@a600000 {
 			};
 		};
 
+		camcc: clock-controller@ad00000 {
+			compatible = "qcom,sm6350-camcc";
+			reg = <0 0x0ad00000 0 0x16000>;
+			clocks = <&rpmhcc RPMH_CXO_CLK>;
+			#clock-cells = <1>;
+			#reset-cells = <1>;
+			#power-domain-cells = <1>;
+		};
+
 		pdc: interrupt-controller@b220000 {
 			compatible = "qcom,sm6350-pdc", "qcom,pdc";
 			reg = <0 0x0b220000 0 0x30000>, <0 0x17c000f0 0 0x64>;

-- 
2.39.1

[PATCH v2 3/4] arm64: dts: qcom: sm6350: Add CCI nodes

[Luca Weiss]

Add nodes for the two CCI blocks found on SM6350.

The first contains two i2c busses and while the second one might also
contains two busses, the downstream kernel only has one configured, and
some boards use the GPIOs for the potential cci1_i2c1 one other
purposes, so leave that one unconfigured.

Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
---
 arch/arm64/boot/dts/qcom/sm6350.dtsi | 132 +++++++++++++++++++++++++++++++++++
 1 file changed, 132 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sm6350.dtsi b/arch/arm64/boot/dts/qcom/sm6350.dtsi
index 300ced5cda57..802d7f494162 100644
--- a/arch/arm64/boot/dts/qcom/sm6350.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm6350.dtsi
@@ -6,6 +6,7 @@
 
 #include <dt-bindings/clock/qcom,gcc-sm6350.h>
 #include <dt-bindings/clock/qcom,rpmh.h>
+#include <dt-bindings/clock/qcom,sm6350-camcc.h>
 #include <dt-bindings/dma/qcom-gpi.h>
 #include <dt-bindings/gpio/gpio.h>
 #include <dt-bindings/interconnect/qcom,icc.h>
@@ -1435,6 +1436,95 @@ usb_1_dwc3: usb@a600000 {
 			};
 		};
 
+		cci0: cci@ac4a000 {
+			compatible = "qcom,sm6350-cci", "qcom,msm8996-cci";
+			reg = <0 0x0ac4a000 0 0x1000>;
+			interrupts = <GIC_SPI 468 IRQ_TYPE_EDGE_RISING>;
+			power-domains = <&camcc TITAN_TOP_GDSC>;
+
+			clocks = <&camcc CAMCC_CAMNOC_AXI_CLK>,
+				 <&camcc CAMCC_SOC_AHB_CLK>,
+				 <&camcc CAMCC_SLOW_AHB_CLK_SRC>,
+				 <&camcc CAMCC_CPAS_AHB_CLK>,
+				 <&camcc CAMCC_CCI_0_CLK>,
+				 <&camcc CAMCC_CCI_0_CLK_SRC>;
+			clock-names = "camnoc_axi",
+				      "soc_ahb",
+				      "slow_ahb_src",
+				      "cpas_ahb",
+				      "cci",
+				      "cci_src";
+
+			assigned-clocks = <&camcc CAMCC_CAMNOC_AXI_CLK>,
+					  <&camcc CAMCC_CCI_0_CLK>;
+			assigned-clock-rates = <80000000>, <37500000>;
+
+			pinctrl-0 = <&cci0_default &cci1_default>;
+			pinctrl-1 = <&cci0_sleep &cci1_sleep>;
+			pinctrl-names = "default", "sleep";
+
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			status = "disabled";
+
+			cci0_i2c0: i2c-bus@0 {
+				reg = <0>;
+				clock-frequency = <1000000>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+			};
+
+			cci0_i2c1: i2c-bus@1 {
+				reg = <1>;
+				clock-frequency = <1000000>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+			};
+		};
+
+		cci1: cci@ac4b000 {
+			compatible = "qcom,sm6350-cci", "qcom,msm8996-cci";
+			reg = <0 0x0ac4b000 0 0x1000>;
+			interrupts = <GIC_SPI 462 IRQ_TYPE_EDGE_RISING>;
+			power-domains = <&camcc TITAN_TOP_GDSC>;
+
+			clocks = <&camcc CAMCC_CAMNOC_AXI_CLK>,
+				 <&camcc CAMCC_SOC_AHB_CLK>,
+				 <&camcc CAMCC_SLOW_AHB_CLK_SRC>,
+				 <&camcc CAMCC_CPAS_AHB_CLK>,
+				 <&camcc CAMCC_CCI_1_CLK>,
+				 <&camcc CAMCC_CCI_1_CLK_SRC>;
+			clock-names = "camnoc_axi",
+				      "soc_ahb",
+				      "slow_ahb_src",
+				      "cpas_ahb",
+				      "cci",
+				      "cci_src";
+
+			assigned-clocks = <&camcc CAMCC_CAMNOC_AXI_CLK>,
+					  <&camcc CAMCC_CCI_1_CLK>;
+			assigned-clock-rates = <80000000>, <37500000>;
+
+			pinctrl-0 = <&cci2_default>;
+			pinctrl-1 = <&cci2_sleep>;
+			pinctrl-names = "default", "sleep";
+
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			status = "disabled";
+
+			cci1_i2c0: i2c-bus@0 {
+				reg = <0>;
+				clock-frequency = <1000000>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+			};
+
+			/* SM6350 seems to have cci1_i2c1 on gpio2 & gpio3 but unused downstream */
+		};
+
 		camcc: clock-controller@ad00000 {
 			compatible = "qcom,sm6350-camcc";
 			reg = <0 0x0ad00000 0 0x16000>;
@@ -1522,6 +1612,48 @@ tlmm: pinctrl@f100000 {
 			#interrupt-cells = <2>;
 			gpio-ranges = <&tlmm 0 0 157>;
 
+			cci0_default: cci0-default-state {
+				pins = "gpio39", "gpio40";
+				function = "cci_i2c";
+				drive-strength = <2>;
+				bias-pull-up;
+			};
+
+			cci0_sleep: cci0-sleep-state {
+				pins = "gpio39", "gpio40";
+				function = "cci_i2c";
+				drive-strength = <2>;
+				bias-pull-down;
+			};
+
+			cci1_default: cci1-default-state {
+				pins = "gpio41", "gpio42";
+				function = "cci_i2c";
+				drive-strength = <2>;
+				bias-pull-up;
+			};
+
+			cci1_sleep: cci1-sleep-state {
+				pins = "gpio41", "gpio42";
+				function = "cci_i2c";
+				drive-strength = <2>;
+				bias-pull-down;
+			};
+
+			cci2_default: cci2-default-state {
+				pins = "gpio43", "gpio44";
+				function = "cci_i2c";
+				drive-strength = <2>;
+				bias-pull-up;
+			};
+
+			cci2_sleep: cci2-sleep-state {
+				pins = "gpio43", "gpio44";
+				function = "cci_i2c";
+				drive-strength = <2>;
+				bias-pull-down;
+			};
+
 			sdc2_off_state: sdc2-off-state {
 				clk-pins {
 					pins = "sdc2_clk";

-- 
2.39.1

[PATCH v2 4/4] arm64: dts: qcom: sm7225-fairphone-fp4: Enable CCI busses

[Luca Weiss]

Enable the CCI busses that have cameras connected to them.

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
---
 arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts b/arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts
index f0e7ae630e0c..ed0cb70849d3 100644
--- a/arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts
+++ b/arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts
@@ -363,6 +363,26 @@ vreg_bob: bob {
 	};
 };
 
+&cci0 {
+	status = "okay";
+};
+
+&cci0_i2c0 {
+	/* IMX582 @ 0x1a */
+};
+
+&cci0_i2c1 {
+	/* IMX582 @ 0x1a */
+};
+
+&cci1 {
+	status = "okay";
+};
+
+&cci1_i2c0 {
+	/* IMX576 @ 0x10 */
+};
+
 &cdsp {
 	status = "okay";
 	firmware-name = "qcom/sm7225/fairphone4/cdsp.mdt";

-- 
2.39.1

Re: [PATCH v2 3/4] arm64: dts: qcom: sm6350: Add CCI nodes

[Konrad Dybcio]

On 20.01.2023 14:13, Luca Weiss wrote:
> Add nodes for the two CCI blocks found on SM6350.
> 
> The first contains two i2c busses and while the second one might also
> contains two busses, the downstream kernel only has one configured, and
> some boards use the GPIOs for the potential cci1_i2c1 one other
> purposes, so leave that one unconfigured.
> 
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
>  arch/arm64/boot/dts/qcom/sm6350.dtsi | 132 +++++++++++++++++++++++++++++++++++
>  1 file changed, 132 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm6350.dtsi b/arch/arm64/boot/dts/qcom/sm6350.dtsi
> index 300ced5cda57..802d7f494162 100644
> --- a/arch/arm64/boot/dts/qcom/sm6350.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm6350.dtsi
> @@ -6,6 +6,7 @@
>  
>  #include <dt-bindings/clock/qcom,gcc-sm6350.h>
>  #include <dt-bindings/clock/qcom,rpmh.h>
> +#include <dt-bindings/clock/qcom,sm6350-camcc.h>
>  #include <dt-bindings/dma/qcom-gpi.h>
>  #include <dt-bindings/gpio/gpio.h>
>  #include <dt-bindings/interconnect/qcom,icc.h>
> @@ -1435,6 +1436,95 @@ usb_1_dwc3: usb@a600000 {
>  			};
>  		};
>  
> +		cci0: cci@ac4a000 {
> +			compatible = "qcom,sm6350-cci", "qcom,msm8996-cci";
> +			reg = <0 0x0ac4a000 0 0x1000>;
> +			interrupts = <GIC_SPI 468 IRQ_TYPE_EDGE_RISING>;
> +			power-domains = <&camcc TITAN_TOP_GDSC>;
> +
> +			clocks = <&camcc CAMCC_CAMNOC_AXI_CLK>,
> +				 <&camcc CAMCC_SOC_AHB_CLK>,
> +				 <&camcc CAMCC_SLOW_AHB_CLK_SRC>,
> +				 <&camcc CAMCC_CPAS_AHB_CLK>,
> +				 <&camcc CAMCC_CCI_0_CLK>,
> +				 <&camcc CAMCC_CCI_0_CLK_SRC>;
> +			clock-names = "camnoc_axi",
> +				      "soc_ahb",
> +				      "slow_ahb_src",
> +				      "cpas_ahb",
> +				      "cci",
> +				      "cci_src";
> +
> +			assigned-clocks = <&camcc CAMCC_CAMNOC_AXI_CLK>,
> +					  <&camcc CAMCC_CCI_0_CLK>;
> +			assigned-clock-rates = <80000000>, <37500000>;
> +
> +			pinctrl-0 = <&cci0_default &cci1_default>;
> +			pinctrl-1 = <&cci0_sleep &cci1_sleep>;
> +			pinctrl-names = "default", "sleep";
> +
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			status = "disabled";
> +
> +			cci0_i2c0: i2c-bus@0 {
> +				reg = <0>;
> +				clock-frequency = <1000000>;
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +			};
> +
> +			cci0_i2c1: i2c-bus@1 {
> +				reg = <1>;
> +				clock-frequency = <1000000>;
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +			};
> +		};
> +
> +		cci1: cci@ac4b000 {
> +			compatible = "qcom,sm6350-cci", "qcom,msm8996-cci";
> +			reg = <0 0x0ac4b000 0 0x1000>;
> +			interrupts = <GIC_SPI 462 IRQ_TYPE_EDGE_RISING>;
> +			power-domains = <&camcc TITAN_TOP_GDSC>;
> +
> +			clocks = <&camcc CAMCC_CAMNOC_AXI_CLK>,
> +				 <&camcc CAMCC_SOC_AHB_CLK>,
> +				 <&camcc CAMCC_SLOW_AHB_CLK_SRC>,
> +				 <&camcc CAMCC_CPAS_AHB_CLK>,
> +				 <&camcc CAMCC_CCI_1_CLK>,
> +				 <&camcc CAMCC_CCI_1_CLK_SRC>;
> +			clock-names = "camnoc_axi",
> +				      "soc_ahb",
> +				      "slow_ahb_src",
> +				      "cpas_ahb",
> +				      "cci",
> +				      "cci_src";
> +
> +			assigned-clocks = <&camcc CAMCC_CAMNOC_AXI_CLK>,
> +					  <&camcc CAMCC_CCI_1_CLK>;
> +			assigned-clock-rates = <80000000>, <37500000>;
> +
> +			pinctrl-0 = <&cci2_default>;
> +			pinctrl-1 = <&cci2_sleep>;
> +			pinctrl-names = "default", "sleep";
> +
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			status = "disabled";
> +
> +			cci1_i2c0: i2c-bus@0 {
> +				reg = <0>;
> +				clock-frequency = <1000000>;
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +			};
> +
> +			/* SM6350 seems to have cci1_i2c1 on gpio2 & gpio3 but unused downstream */
> +		};
> +
>  		camcc: clock-controller@ad00000 {
>  			compatible = "qcom,sm6350-camcc";
>  			reg = <0 0x0ad00000 0 0x16000>;
> @@ -1522,6 +1612,48 @@ tlmm: pinctrl@f100000 {
>  			#interrupt-cells = <2>;
>  			gpio-ranges = <&tlmm 0 0 157>;
>  
> +			cci0_default: cci0-default-state {
> +				pins = "gpio39", "gpio40";
> +				function = "cci_i2c";
> +				drive-strength = <2>;
> +				bias-pull-up;
> +			};
> +
> +			cci0_sleep: cci0-sleep-state {
> +				pins = "gpio39", "gpio40";
> +				function = "cci_i2c";
> +				drive-strength = <2>;
> +				bias-pull-down;
> +			};
> +
> +			cci1_default: cci1-default-state {
> +				pins = "gpio41", "gpio42";
> +				function = "cci_i2c";
> +				drive-strength = <2>;
> +				bias-pull-up;
> +			};
> +
> +			cci1_sleep: cci1-sleep-state {
> +				pins = "gpio41", "gpio42";
> +				function = "cci_i2c";
> +				drive-strength = <2>;
> +				bias-pull-down;
> +			};
> +
> +			cci2_default: cci2-default-state {
> +				pins = "gpio43", "gpio44";
> +				function = "cci_i2c";
> +				drive-strength = <2>;
> +				bias-pull-up;
> +			};
> +
> +			cci2_sleep: cci2-sleep-state {
> +				pins = "gpio43", "gpio44";
> +				function = "cci_i2c";
> +				drive-strength = <2>;
> +				bias-pull-down;
> +			};
> +
>  			sdc2_off_state: sdc2-off-state {
>  				clk-pins {
>  					pins = "sdc2_clk";
> 

Re: [PATCH v2 2/4] arm64: dts: qcom: sm6350: Add camera clock controller

[Bryan O'Donoghue]

On 20/01/2023 13:13, Luca Weiss wrote:
> +		camcc: clock-controller@ad00000 {
> +			compatible = "qcom,sm6350-camcc";
> +			reg = <0 0x0ad00000 0 0x16000>;
> +			clocks = <&rpmhcc RPMH_CXO_CLK>;
> +			#clock-cells = <1>;
> +			#reset-cells = <1>;
> +			#power-domain-cells = <1>;
> +		};

Should you include

required-opps = <&rpmhpd_opp_low_svs>;

?

---
bod

Re: [PATCH v2 1/4] dt-bindings: i2c: qcom-cci: Document SM6350 compatible

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 287 bytes --]

On Fri, Jan 20, 2023 at 02:13:44PM +0100, Luca Weiss wrote:
> Document the compatible for the CCI block found on SM6350 SoC.
> 
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 2/4] arm64: dts: qcom: sm6350: Add camera clock controller

[Luca Weiss]

On Fri Jan 20, 2023 at 5:49 PM CET, Bryan O'Donoghue wrote:
> On 20/01/2023 13:13, Luca Weiss wrote:
> > +		camcc: clock-controller@ad00000 {
> > +			compatible = "qcom,sm6350-camcc";
> > +			reg = <0 0x0ad00000 0 0x16000>;
> > +			clocks = <&rpmhcc RPMH_CXO_CLK>;
> > +			#clock-cells = <1>;
> > +			#reset-cells = <1>;
> > +			#power-domain-cells = <1>;
> > +		};
>
> Should you include
>
> required-opps = <&rpmhpd_opp_low_svs>;
>
> ?

I don't know, it works without. But doesn't this property not just
affect power-domains? I haven't passed any here.

>
> ---
> bod

Re: [PATCH v2 2/4] arm64: dts: qcom: sm6350: Add camera clock controller

[Bryan O'Donoghue]

On 24/01/2023 14:48, Luca Weiss wrote:
> On Fri Jan 20, 2023 at 5:49 PM CET, Bryan O'Donoghue wrote:
>> On 20/01/2023 13:13, Luca Weiss wrote:
>>> +		camcc: clock-controller@ad00000 {
>>> +			compatible = "qcom,sm6350-camcc";
>>> +			reg = <0 0x0ad00000 0 0x16000>;
>>> +			clocks = <&rpmhcc RPMH_CXO_CLK>;
>>> +			#clock-cells = <1>;
>>> +			#reset-cells = <1>;
>>> +			#power-domain-cells = <1>;
>>> +		};
>>
>> Should you include
>>
>> required-opps = <&rpmhpd_opp_low_svs>;
>>
>> ?
> 
> I don't know, it works without. But doesn't this property not just
> affect power-domains? I haven't passed any here.
> 

Should you have a TITAN_TOP pd though ?

---
bod

Re: [PATCH v2 2/4] arm64: dts: qcom: sm6350: Add camera clock controller

[Luca Weiss]

On Tue Jan 24, 2023 at 4:25 PM CET, Bryan O'Donoghue wrote:
> On 24/01/2023 14:48, Luca Weiss wrote:
> > On Fri Jan 20, 2023 at 5:49 PM CET, Bryan O'Donoghue wrote:
> >> On 20/01/2023 13:13, Luca Weiss wrote:
> >>> +		camcc: clock-controller@ad00000 {
> >>> +			compatible = "qcom,sm6350-camcc";
> >>> +			reg = <0 0x0ad00000 0 0x16000>;
> >>> +			clocks = <&rpmhcc RPMH_CXO_CLK>;
> >>> +			#clock-cells = <1>;
> >>> +			#reset-cells = <1>;
> >>> +			#power-domain-cells = <1>;
> >>> +		};
> >>
> >> Should you include
> >>
> >> required-opps = <&rpmhpd_opp_low_svs>;
> >>
> >> ?
> > 
> > I don't know, it works without. But doesn't this property not just
> > affect power-domains? I haven't passed any here.
> > 
>
> Should you have a TITAN_TOP pd though ?

Can I reference <&camcc TITAN_TOP_GDSC> from itself? I know that having
it on is required to turn on at least some clocks (maybe all clocks).
But from what I understand how power domains are normally handled, the
driver core enables them before the driver is probed, so self
referencing wouldn't work.

And at least no other SoC upstream references TITAN_TOP_GDSC in camcc.

Regards
Luca

>
> ---
> bod

Re: [PATCH v2 2/4] arm64: dts: qcom: sm6350: Add camera clock controller

[Bryan O'Donoghue]

On 27/01/2023 12:45, Luca Weiss wrote:
> Can I reference <&camcc TITAN_TOP_GDSC> from itself? I know that having
> it on is required to turn on at least some clocks (maybe all clocks).
> But from what I understand how power domains are normally handled, the
> driver core enables them before the driver is probed, so self
> referencing wouldn't work.
> 
> And at least no other SoC upstream references TITAN_TOP_GDSC in camcc.
> 
> Regards
> Luca

Doh I meant to say a power-domain to an mmcx a la

power-domains = <&rpmhpd SM8250_MMCX>;
required-opps = <&rpmhpd_opp_low_svs>;

TITAN_TOP should be in your cci and camss dt nodes.

---
bod

Re: [PATCH v2 2/4] arm64: dts: qcom: sm6350: Add camera clock controller

[Luca Weiss]

On Fri Jan 27, 2023 at 1:49 PM CET, Bryan O'Donoghue wrote:
> On 27/01/2023 12:45, Luca Weiss wrote:
> > Can I reference <&camcc TITAN_TOP_GDSC> from itself? I know that having
> > it on is required to turn on at least some clocks (maybe all clocks).
> > But from what I understand how power domains are normally handled, the
> > driver core enables them before the driver is probed, so self
> > referencing wouldn't work.
> > 
> > And at least no other SoC upstream references TITAN_TOP_GDSC in camcc.
> > 
> > Regards
> > Luca
>
> Doh I meant to say a power-domain to an mmcx a la
>
> power-domains = <&rpmhpd SM8250_MMCX>;
> required-opps = <&rpmhpd_opp_low_svs>;
>
> TITAN_TOP should be in your cci and camss dt nodes.

Okay, that makes more sense.

What I don't quite understand is why sm8250 only has MMCX listed there
since downstream has both vdd_mx-supply = <&VDD_MX_LEVEL> and
vdd_mm-supply = <&VDD_MMCX_LEVEL> and both "supplies" are used for
different clocks using .vdd_class

But back to sm6350, downstream has vdd_mx-supply = <&VDD_MX_LEVEL> and
vdd_cx-supply = <&VDD_CX_LEVEL> and like sm8250 uses cx and mx for
different clocks.
Not sure if I should add both, and I guess mainline also currently
doesn't use higher ops for the power domain when higher clock rate is
needed, from what I understand?

>
> ---
> bod

Re: [PATCH v2 2/4] arm64: dts: qcom: sm6350: Add camera clock controller

[Konrad Dybcio]

On 27.01.2023 14:11, Luca Weiss wrote:
> On Fri Jan 27, 2023 at 1:49 PM CET, Bryan O'Donoghue wrote:
>> On 27/01/2023 12:45, Luca Weiss wrote:
>>> Can I reference <&camcc TITAN_TOP_GDSC> from itself? I know that having
>>> it on is required to turn on at least some clocks (maybe all clocks).
>>> But from what I understand how power domains are normally handled, the
>>> driver core enables them before the driver is probed, so self
>>> referencing wouldn't work.
>>>
>>> And at least no other SoC upstream references TITAN_TOP_GDSC in camcc.
>>>
>>> Regards
>>> Luca
>>
>> Doh I meant to say a power-domain to an mmcx a la
>>
>> power-domains = <&rpmhpd SM8250_MMCX>;
>> required-opps = <&rpmhpd_opp_low_svs>;
>>
>> TITAN_TOP should be in your cci and camss dt nodes.
> 
> Okay, that makes more sense.
> 
> What I don't quite understand is why sm8250 only has MMCX listed there
> since downstream has both vdd_mx-supply = <&VDD_MX_LEVEL> and
> vdd_mm-supply = <&VDD_MMCX_LEVEL> and both "supplies" are used for
> different clocks using .vdd_class
> 
> But back to sm6350, downstream has vdd_mx-supply = <&VDD_MX_LEVEL> and
> vdd_cx-supply = <&VDD_CX_LEVEL> and like sm8250 uses cx and mx for
> different clocks.
> Not sure if I should add both, and I guess mainline also currently
> doesn't use higher ops for the power domain when higher clock rate is
> needed, from what I understand?
Basically if you don't need to power any of these power rails to
have access to the clock controller, you don't need any of them.

What you will need to do however, is make sure that they are scaled with
child devices then.. but that's no bueno since they all need TITAN_GDSC.
That's why Bryan suggests leaving a vote on a power rail in the clock
controller, so that if no other votes are present (as improbable as
that may be), you will still be able to get the clocks going.

That OTOH will require you to add power management support (PM ops)
to the clock controller, as otherwise you can say goodbye to battery
life..

Konrad
> 
>>
>> ---
>> bod
> 

Re: [PATCH v2 2/4] arm64: dts: qcom: sm6350: Add camera clock controller

[Bryan O'Donoghue]

On 27/01/2023 13:11, Luca Weiss wrote:
>> Doh I meant to say a power-domain to an mmcx a la
>>
>> power-domains = <&rpmhpd SM8250_MMCX>;
>> required-opps = <&rpmhpd_opp_low_svs>;
>>
>> TITAN_TOP should be in your cci and camss dt nodes.
> Okay, that makes more sense.
> 
> What I don't quite understand is why sm8250 only has MMCX listed there
> since downstream has both vdd_mx-supply = <&VDD_MX_LEVEL> and
> vdd_mm-supply = <&VDD_MMCX_LEVEL> and both "supplies" are used for
> different clocks using .vdd_class

power-domains = <&rpmhpd SM8250_MMCX>; == MMCX_LEVEL required for camcc
power-domains = <&camcc TITAN_TOP_GDSC>; required for cci/camss

now that you ask the question about MX_LEVEL you're making me doubt we 
have a 100% complete representation upstream TB perfectly honest, 
warrants a deep dive..

I just remember that on 8250 we tripped over MMCX not being switched on 
when - display I think was switched off.

---
bod

Re: [PATCH v2 2/4] arm64: dts: qcom: sm6350: Add camera clock controller

[Konrad Dybcio]

On 27.01.2023 14:54, Bryan O'Donoghue wrote:
> On 27/01/2023 13:11, Luca Weiss wrote:
>>> Doh I meant to say a power-domain to an mmcx a la
>>>
>>> power-domains = <&rpmhpd SM8250_MMCX>;
>>> required-opps = <&rpmhpd_opp_low_svs>;
>>>
>>> TITAN_TOP should be in your cci and camss dt nodes.
>> Okay, that makes more sense.
>>
>> What I don't quite understand is why sm8250 only has MMCX listed there
>> since downstream has both vdd_mx-supply = <&VDD_MX_LEVEL> and
>> vdd_mm-supply = <&VDD_MMCX_LEVEL> and both "supplies" are used for
>> different clocks using .vdd_class
> 
> power-domains = <&rpmhpd SM8250_MMCX>; == MMCX_LEVEL required for camcc
> power-domains = <&camcc TITAN_TOP_GDSC>; required for cci/camss
> 
> now that you ask the question about MX_LEVEL you're making me doubt we have a 100% complete representation upstream TB perfectly honest, warrants a deep dive..
> 
> I just remember that on 8250 we tripped over MMCX not being switched on when - display I think was switched off.
There's no MMCX on 6350 and MX is a parent of CX, so if we just stick
CX here and add the lowest level to required-opps and add corresponding
PM ops to the clk driver, it'll all be taken care of!

Konrad

> 
> ---
> bod
> 

Re: (subset) [PATCH v2 0/4] Add CCI bus support for SM6350

[Bjorn Andersson]

On Fri, 20 Jan 2023 14:13:43 +0100, Luca Weiss wrote:
> Add the camera clock controller node and CCI nodes to sm6350 dtsi and enable
> the i2c busses on Fairphone 4 dts.
> 
> This is tested using PM8008 regulator patches from the lists which power the
> cameras, and using i2cdetect/i2cget/i2cset reading the sensor ID registers.
> 
> 
> [...]

Applied, thanks!

[2/4] arm64: dts: qcom: sm6350: Add camera clock controller
      commit: 4ab96c9c4012770f50f90bda2e61ef774bb63be5
[3/4] arm64: dts: qcom: sm6350: Add CCI nodes
      commit: 033fb15f39b8f092bf4664144784a3e19c834f26
[4/4] arm64: dts: qcom: sm7225-fairphone-fp4: Enable CCI busses
      commit: bd3dc67bbc34d684c7d94865bb10283508d7cd84

Best regards,
-- 
Bjorn Andersson <andersson@kernel.org>