[PATCH 0/3] Add CCI support for SC7280

[PATCH 0/3] Add CCI support for SC7280

[Luca Weiss]

Add the dts nodes for the camera control interface found on the SC7280
SoC. And then enable the CCI nodes in the Fairphone 5 dts.

Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
---
Luca Weiss (3):
      dt-bindings: i2c: qcom-cci: Document SC7280 compatible
      arm64: dts: qcom: sc7280: Add Camera Control Interface busses
      arm64: dts: qcom: qcm6490-fairphone-fp5: Enable CCI busses

 .../devicetree/bindings/i2c/qcom,i2c-cci.yaml      |   1 +
 arch/arm64/boot/dts/qcom/qcm6490-fairphone-fp5.dts |  20 +++
 arch/arm64/boot/dts/qcom/sc7280.dtsi               | 136 +++++++++++++++++++++
 3 files changed, 157 insertions(+)
---
base-commit: c858197a69efe69e1607f4854af42ec338e54e96
change-id: 20230929-sc7280-cci-4690ef8da107

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

[PATCH 1/3] dt-bindings: i2c: qcom-cci: Document SC7280 compatible

[Luca Weiss]

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

Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
---
 Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml b/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml
index 042d4dc636ee..158588236749 100644
--- a/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml
+++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml
@@ -25,6 +25,7 @@ properties:
 
       - items:
           - enum:
+              - qcom,sc7280-cci
               - qcom,sdm845-cci
               - qcom,sm6350-cci
               - qcom,sm8250-cci

-- 
2.42.0

[PATCH 2/3] arm64: dts: qcom: sc7280: Add Camera Control Interface busses

[Luca Weiss]

Add the CCI busses found on sc7280 and their pinctrl states.

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

diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index 66f1eb83cca7..65550de2e4ff 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -3793,6 +3793,86 @@ videocc: clock-controller@aaf0000 {
 			#power-domain-cells = <1>;
 		};
 
+		cci0: cci@ac4a000 {
+			compatible = "qcom,sc7280-cci", "qcom,msm8996-cci";
+			reg = <0 0x0ac4a000 0 0x1000>;
+			interrupts = <GIC_SPI 460 IRQ_TYPE_EDGE_RISING>;
+			power-domains = <&camcc CAM_CC_TITAN_TOP_GDSC>;
+
+			clocks = <&camcc CAM_CC_CAMNOC_AXI_CLK>,
+				 <&camcc CAM_CC_SLOW_AHB_CLK_SRC>,
+				 <&camcc CAM_CC_CPAS_AHB_CLK>,
+				 <&camcc CAM_CC_CCI_0_CLK>,
+				 <&camcc CAM_CC_CCI_0_CLK_SRC>;
+			clock-names = "camnoc_axi",
+				      "slow_ahb_src",
+				      "cpas_ahb",
+				      "cci",
+				      "cci_src";
+			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,sc7280-cci", "qcom,msm8996-cci";
+			reg = <0 0x0ac4b000 0 0x1000>;
+			interrupts = <GIC_SPI 271 IRQ_TYPE_EDGE_RISING>;
+			power-domains = <&camcc CAM_CC_TITAN_TOP_GDSC>;
+
+			clocks = <&camcc CAM_CC_CAMNOC_AXI_CLK>,
+				 <&camcc CAM_CC_SLOW_AHB_CLK_SRC>,
+				 <&camcc CAM_CC_CPAS_AHB_CLK>,
+				 <&camcc CAM_CC_CCI_1_CLK>,
+				 <&camcc CAM_CC_CCI_1_CLK_SRC>;
+			clock-names = "camnoc_axi",
+				      "slow_ahb_src",
+				      "cpas_ahb",
+				      "cci",
+				      "cci_src";
+			pinctrl-0 = <&cci2_default &cci3_default>;
+			pinctrl-1 = <&cci2_sleep &cci3_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>;
+			};
+
+			cci1_i2c1: i2c-bus@1 {
+				reg = <1>;
+				clock-frequency = <1000000>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+			};
+		};
+
 		camcc: clock-controller@ad00000 {
 			compatible = "qcom,sc7280-camcc";
 			reg = <0 0x0ad00000 0 0x10000>;
@@ -4298,6 +4378,62 @@ tlmm: pinctrl@f100000 {
 			gpio-ranges = <&tlmm 0 0 175>;
 			wakeup-parent = <&pdc>;
 
+			cci0_default: cci0-default-state {
+				pins = "gpio69", "gpio70";
+				function = "cci_i2c";
+				drive-strength = <2>;
+				bias-pull-up;
+			};
+
+			cci0_sleep: cci0-sleep-state {
+				pins = "gpio69", "gpio70";
+				function = "cci_i2c";
+				drive-strength = <2>;
+				bias-pull-down;
+			};
+
+			cci1_default: cci1-default-state {
+				pins = "gpio71", "gpio72";
+				function = "cci_i2c";
+				drive-strength = <2>;
+				bias-pull-up;
+			};
+
+			cci1_sleep: cci1-sleep-state {
+				pins = "gpio71", "gpio72";
+				function = "cci_i2c";
+				drive-strength = <2>;
+				bias-pull-down;
+			};
+
+			cci2_default: cci2-default-state {
+				pins = "gpio73", "gpio74";
+				function = "cci_i2c";
+				drive-strength = <2>;
+				bias-pull-up;
+			};
+
+			cci2_sleep: cci2-sleep-state {
+				pins = "gpio73", "gpio74";
+				function = "cci_i2c";
+				drive-strength = <2>;
+				bias-pull-down;
+			};
+
+			cci3_default: cci3-default-state {
+				pins = "gpio75", "gpio76";
+				function = "cci_i2c";
+				drive-strength = <2>;
+				bias-pull-up;
+			};
+
+			cci3_sleep: cci3-sleep-state {
+				pins = "gpio75", "gpio76";
+				function = "cci_i2c";
+				drive-strength = <2>;
+				bias-pull-down;
+			};
+
 			dp_hot_plug_det: dp-hot-plug-det-state {
 				pins = "gpio47";
 				function = "dp_hot";

-- 
2.42.0

[PATCH 3/3] arm64: dts: qcom: qcm6490-fairphone-fp5: Enable CCI busses

[Luca Weiss]

Enable the CCI busses where sensors are connected to.

Not covered here is the regulator used for pull-up on the I2C busses.
This would be pm8008_l6 (L6P).

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

diff --git a/arch/arm64/boot/dts/qcom/qcm6490-fairphone-fp5.dts b/arch/arm64/boot/dts/qcom/qcm6490-fairphone-fp5.dts
index c8e7e6689b20..3fc0b3f7dc31 100644
--- a/arch/arm64/boot/dts/qcom/qcm6490-fairphone-fp5.dts
+++ b/arch/arm64/boot/dts/qcom/qcm6490-fairphone-fp5.dts
@@ -388,6 +388,26 @@ vreg_bob: bob {
 	};
 };
 
+&cci0 {
+	status = "okay";
+};
+
+&cci0_i2c0 {
+	/* IMX800 @ 1a */
+};
+
+&cci0_i2c1 {
+	/* IMX858 @ 29 */
+};
+
+&cci1 {
+	status = "okay";
+};
+
+&cci1_i2c1 {
+	/* S5KJN1SQ03 @ 10 */
+};
+
 &dispcc {
 	/* Disable for now so simple-framebuffer continues working */
 	status = "disabled";

-- 
2.42.0

Re: [PATCH 2/3] arm64: dts: qcom: sc7280: Add Camera Control Interface busses

[Konrad Dybcio]

On 9/29/23 10:01, Luca Weiss wrote:
> Add the CCI busses found on sc7280 and their pinctrl states.
> 
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> ---
>   arch/arm64/boot/dts/qcom/sc7280.dtsi | 136 +++++++++++++++++++++++++++++++++++
>   1 file changed, 136 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index 66f1eb83cca7..65550de2e4ff 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -3793,6 +3793,86 @@ videocc: clock-controller@aaf0000 {
>   			#power-domain-cells = <1>;
>   		};
>   
> +		cci0: cci@ac4a000 {
> +			compatible = "qcom,sc7280-cci", "qcom,msm8996-cci";
> +			reg = <0 0x0ac4a000 0 0x1000>;
> +			interrupts = <GIC_SPI 460 IRQ_TYPE_EDGE_RISING>;
> +			power-domains = <&camcc CAM_CC_TITAN_TOP_GDSC>;
> +
> +			clocks = <&camcc CAM_CC_CAMNOC_AXI_CLK>,
> +				 <&camcc CAM_CC_SLOW_AHB_CLK_SRC>,
> +				 <&camcc CAM_CC_CPAS_AHB_CLK>,
> +				 <&camcc CAM_CC_CCI_0_CLK>,
> +				 <&camcc CAM_CC_CCI_0_CLK_SRC>;
> +			clock-names = "camnoc_axi",
> +				      "slow_ahb_src",
> +				      "cpas_ahb",
> +				      "cci",
> +				      "cci_src";
I guess this is more of a question to e.g. Bryan, but are all of these 
clocks actually necessary?

Konrad

Re: [PATCH 3/3] arm64: dts: qcom: qcm6490-fairphone-fp5: Enable CCI busses

[Konrad Dybcio]

On 9/29/23 10:01, Luca Weiss wrote:
> Enable the CCI busses where sensors are connected to.
> 
> Not covered here is the regulator used for pull-up on the I2C busses.
> This would be pm8008_l6 (L6P).
Does it make sense to enable non-functional hw then? Or is it on by default?

Konrad

Re: [PATCH 3/3] arm64: dts: qcom: qcm6490-fairphone-fp5: Enable CCI busses

[Luca Weiss]

On Fri Sep 29, 2023 at 3:36 PM CEST, Konrad Dybcio wrote:
>
>
> On 9/29/23 10:01, Luca Weiss wrote:
> > Enable the CCI busses where sensors are connected to.
> > 
> > Not covered here is the regulator used for pull-up on the I2C busses.
> > This would be pm8008_l6 (L6P).
> Does it make sense to enable non-functional hw then? Or is it on by default?

Without the out-of-tree pm8008 driver you cannot talk to any hardware on
the i2c busses there, so.. I guess you have a point?

A little bird told me this week they're working on a new revision for
pm8008 so hopefully this is coming at some point.

>
> Konrad

Re: [PATCH 2/3] arm64: dts: qcom: sc7280: Add Camera Control Interface busses

[Bryan O'Donoghue]

On 29/09/2023 14:35, Konrad Dybcio wrote:
> 
> 
> On 9/29/23 10:01, Luca Weiss wrote:
>> Add the CCI busses found on sc7280 and their pinctrl states.
>>
>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>> ---
>>   arch/arm64/boot/dts/qcom/sc7280.dtsi | 136 
>> +++++++++++++++++++++++++++++++++++
>>   1 file changed, 136 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi 
>> b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> index 66f1eb83cca7..65550de2e4ff 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> @@ -3793,6 +3793,86 @@ videocc: clock-controller@aaf0000 {
>>               #power-domain-cells = <1>;
>>           };
>> +        cci0: cci@ac4a000 {
>> +            compatible = "qcom,sc7280-cci", "qcom,msm8996-cci";
>> +            reg = <0 0x0ac4a000 0 0x1000>;
>> +            interrupts = <GIC_SPI 460 IRQ_TYPE_EDGE_RISING>;
>> +            power-domains = <&camcc CAM_CC_TITAN_TOP_GDSC>;
>> +
>> +            clocks = <&camcc CAM_CC_CAMNOC_AXI_CLK>,
>> +                 <&camcc CAM_CC_SLOW_AHB_CLK_SRC>,
>> +                 <&camcc CAM_CC_CPAS_AHB_CLK>,
>> +                 <&camcc CAM_CC_CCI_0_CLK>,
>> +                 <&camcc CAM_CC_CCI_0_CLK_SRC>;
>> +            clock-names = "camnoc_axi",
>> +                      "slow_ahb_src",
>> +                      "cpas_ahb",
>> +                      "cci",
>> +                      "cci_src";
> I guess this is more of a question to e.g. Bryan, but are all of these 
> clocks actually necessary?
> 
> Konrad
Hmm its a good question, we generally take the approach of adopting all 
of the downstream clocks for these camera interfaces verbatim.

The clock plan for this part only calls out cci_X_clk and cci_x_clk_src 
for the CCI however we know that to be incomplete since we *absolutely* 
need to have the AXI for the block clocked to access those registers, 
same deal with the AHB bus.

AXI: registers
AHB: data

In the above list the only clock you might conceivably not need is 
CPAS_AHB_CLK.

Let me zap that clock from sdm845 since I have an rb3 right in front of 
me and see what happens.

Crash and reset

--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -4402,13 +4402,11 @@ cci: cci@ac4a000 {
                         clocks = <&clock_camcc CAM_CC_CAMNOC_AXI_CLK>,
                                 <&clock_camcc CAM_CC_SOC_AHB_CLK>,
                                 <&clock_camcc CAM_CC_SLOW_AHB_CLK_SRC>,
-                               <&clock_camcc CAM_CC_CPAS_AHB_CLK>,
                                 <&clock_camcc CAM_CC_CCI_CLK>,
                                 <&clock_camcc CAM_CC_CCI_CLK_SRC>;
                         clock-names = "camnoc_axi",
                                 "soc_ahb",
                                 "slow_ahb_src",
-                               "cpas_ahb",
                                 "cci",
                                 "cci_src";


I think the list is good tbh

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Re: [PATCH 1/3] dt-bindings: i2c: qcom-cci: Document SC7280 compatible

[Bryan O'Donoghue]

On 29/09/2023 09:01, Luca Weiss wrote:
> Document the compatible for the CCI block found on SC7280 SoC.
> 
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> ---
>   Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml b/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml
> index 042d4dc636ee..158588236749 100644
> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml
> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml
> @@ -25,6 +25,7 @@ properties:
>   
>         - items:
>             - enum:
> +              - qcom,sc7280-cci
>                 - qcom,sdm845-cci
>                 - qcom,sm6350-cci
>                 - qcom,sm8250-cci
> 
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Re: [PATCH 1/3] dt-bindings: i2c: qcom-cci: Document SC7280 compatible

[Bryan O'Donoghue]

On 29/09/2023 09:01, Luca Weiss wrote:
> Document the compatible for the CCI block found on SC7280 SoC.
> 
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> ---
>   Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml b/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml
> index 042d4dc636ee..158588236749 100644
> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml
> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml
> @@ -25,6 +25,7 @@ properties:
>   
>         - items:
>             - enum:
> +              - qcom,sc7280-cci
>                 - qcom,sdm845-cci
>                 - qcom,sm6350-cci
>                 - qcom,sm8250-cci
> 
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Re: [PATCH 2/3] arm64: dts: qcom: sc7280: Add Camera Control Interface busses

[Konrad Dybcio]

On 29.09.2023 16:15, Bryan O'Donoghue wrote:
> On 29/09/2023 14:35, Konrad Dybcio wrote:
>>
>>
>> On 9/29/23 10:01, Luca Weiss wrote:
>>> Add the CCI busses found on sc7280 and their pinctrl states.
>>>
>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>> ---
>>>   arch/arm64/boot/dts/qcom/sc7280.dtsi | 136 +++++++++++++++++++++++++++++++++++
>>>   1 file changed, 136 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>>> index 66f1eb83cca7..65550de2e4ff 100644
>>> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>>> @@ -3793,6 +3793,86 @@ videocc: clock-controller@aaf0000 {
>>>               #power-domain-cells = <1>;
>>>           };
>>> +        cci0: cci@ac4a000 {
>>> +            compatible = "qcom,sc7280-cci", "qcom,msm8996-cci";
>>> +            reg = <0 0x0ac4a000 0 0x1000>;
>>> +            interrupts = <GIC_SPI 460 IRQ_TYPE_EDGE_RISING>;
>>> +            power-domains = <&camcc CAM_CC_TITAN_TOP_GDSC>;
>>> +
>>> +            clocks = <&camcc CAM_CC_CAMNOC_AXI_CLK>,
>>> +                 <&camcc CAM_CC_SLOW_AHB_CLK_SRC>,
>>> +                 <&camcc CAM_CC_CPAS_AHB_CLK>,
>>> +                 <&camcc CAM_CC_CCI_0_CLK>,
>>> +                 <&camcc CAM_CC_CCI_0_CLK_SRC>;
>>> +            clock-names = "camnoc_axi",
>>> +                      "slow_ahb_src",
>>> +                      "cpas_ahb",
>>> +                      "cci",
>>> +                      "cci_src";
>> I guess this is more of a question to e.g. Bryan, but are all of these clocks actually necessary?
>>
>> Konrad
> Hmm its a good question, we generally take the approach of adopting all of the downstream clocks for these camera interfaces verbatim.
> 
> The clock plan for this part only calls out cci_X_clk and cci_x_clk_src for the CCI however we know that to be incomplete since we *absolutely* need to have the AXI for the block clocked to access those registers, same deal with the AHB bus.
> 
> AXI: registers
> AHB: data
> 
> In the above list the only clock you might conceivably not need is CPAS_AHB_CLK.
> 
> Let me zap that clock from sdm845 since I have an rb3 right in front of me and see what happens.
> 
> Crash and reset
> 
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -4402,13 +4402,11 @@ cci: cci@ac4a000 {
>                         clocks = <&clock_camcc CAM_CC_CAMNOC_AXI_CLK>,
>                                 <&clock_camcc CAM_CC_SOC_AHB_CLK>,
>                                 <&clock_camcc CAM_CC_SLOW_AHB_CLK_SRC>,
> -                               <&clock_camcc CAM_CC_CPAS_AHB_CLK>,
>                                 <&clock_camcc CAM_CC_CCI_CLK>,
>                                 <&clock_camcc CAM_CC_CCI_CLK_SRC>;
>                         clock-names = "camnoc_axi",
>                                 "soc_ahb",
>                                 "slow_ahb_src",
> -                               "cpas_ahb",
>                                 "cci",
>                                 "cci_src";
> 
> 
> I think the list is good tbh
WDYT about camcc consuming ahb, like dispcc does?
AXI, hmm.. not quite sure what to do with it

Konrad

Re: [PATCH 2/3] arm64: dts: qcom: sc7280: Add Camera Control Interface busses

[Bryan O'Donoghue]

On 29/09/2023 15:18, Konrad Dybcio wrote:
> On 29.09.2023 16:15, Bryan O'Donoghue wrote:
>> On 29/09/2023 14:35, Konrad Dybcio wrote:
>>>
>>>
>>> On 9/29/23 10:01, Luca Weiss wrote:
>>>> Add the CCI busses found on sc7280 and their pinctrl states.
>>>>
>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>>> ---
>>>>    arch/arm64/boot/dts/qcom/sc7280.dtsi | 136 +++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 136 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>>>> index 66f1eb83cca7..65550de2e4ff 100644
>>>> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>>>> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>>>> @@ -3793,6 +3793,86 @@ videocc: clock-controller@aaf0000 {
>>>>                #power-domain-cells = <1>;
>>>>            };
>>>> +        cci0: cci@ac4a000 {
>>>> +            compatible = "qcom,sc7280-cci", "qcom,msm8996-cci";
>>>> +            reg = <0 0x0ac4a000 0 0x1000>;
>>>> +            interrupts = <GIC_SPI 460 IRQ_TYPE_EDGE_RISING>;
>>>> +            power-domains = <&camcc CAM_CC_TITAN_TOP_GDSC>;
>>>> +
>>>> +            clocks = <&camcc CAM_CC_CAMNOC_AXI_CLK>,
>>>> +                 <&camcc CAM_CC_SLOW_AHB_CLK_SRC>,
>>>> +                 <&camcc CAM_CC_CPAS_AHB_CLK>,
>>>> +                 <&camcc CAM_CC_CCI_0_CLK>,
>>>> +                 <&camcc CAM_CC_CCI_0_CLK_SRC>;
>>>> +            clock-names = "camnoc_axi",
>>>> +                      "slow_ahb_src",
>>>> +                      "cpas_ahb",
>>>> +                      "cci",
>>>> +                      "cci_src";
>>> I guess this is more of a question to e.g. Bryan, but are all of these clocks actually necessary?
>>>
>>> Konrad
>> Hmm its a good question, we generally take the approach of adopting all of the downstream clocks for these camera interfaces verbatim.
>>
>> The clock plan for this part only calls out cci_X_clk and cci_x_clk_src for the CCI however we know that to be incomplete since we *absolutely* need to have the AXI for the block clocked to access those registers, same deal with the AHB bus.
>>
>> AXI: registers
>> AHB: data
>>
>> In the above list the only clock you might conceivably not need is CPAS_AHB_CLK.
>>
>> Let me zap that clock from sdm845 since I have an rb3 right in front of me and see what happens.
>>
>> Crash and reset
>>
>> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> @@ -4402,13 +4402,11 @@ cci: cci@ac4a000 {
>>                          clocks = <&clock_camcc CAM_CC_CAMNOC_AXI_CLK>,
>>                                  <&clock_camcc CAM_CC_SOC_AHB_CLK>,
>>                                  <&clock_camcc CAM_CC_SLOW_AHB_CLK_SRC>,
>> -                               <&clock_camcc CAM_CC_CPAS_AHB_CLK>,
>>                                  <&clock_camcc CAM_CC_CCI_CLK>,
>>                                  <&clock_camcc CAM_CC_CCI_CLK_SRC>;
>>                          clock-names = "camnoc_axi",
>>                                  "soc_ahb",
>>                                  "slow_ahb_src",
>> -                               "cpas_ahb",
>>                                  "cci",
>>                                  "cci_src";
>>
>>
>> I think the list is good tbh
> WDYT about camcc consuming ahb, like dispcc does?
> AXI, hmm.. not quite sure what to do with it
> 
> Konrad

Hmm on which platform, camcc clock depds on sdm845 looks very sparse to me.

8550 OTOH

dispcc: clock-controller@af00000 {
        compatible = "qcom,sm8550-dispcc";
        reg = <0 0x0af00000 0 0x20000>;
        clocks = <&bi_tcxo_div2>,
                 <&bi_tcxo_ao_div2>,
                 <&gcc GCC_DISP_AHB_CLK>,

videocc: clock-controller@aaf0000 {
        compatible = "qcom,sm8550-videocc";
        reg = <0 0x0aaf0000 0 0x10000>;
        clocks = <&bi_tcxo_div2>,
                 <&gcc GCC_VIDEO_AHB_CLK>;

sm8250

camcc: clock-controller@ad00000 {
         compatible = "qcom,sm8250-camcc";
         reg = <0 0x0ad00000 0 0x10000>;
         clocks = <&gcc GCC_CAMERA_AHB_CLK>,

I think those DISP_AHB, VIDEO_AHB_CLK, CAMERA_AHB_CLKs should live in 
the display, video and camss blocks i.e. they are not clocks that you 
require to access the clock controller registers themselves...

I'm seeing for the most part these MEDIAIPBLOCK_AHB_CLKs don't come from 
the GCC AHB clock but from another root clock generator.

bi_tcxo ->
cam_cc_pll0_out_main ->
cam_cc_pll0_out_even ->
cam_cc_pll0_out_odd ->
cam_cc_pll2_out_main ->
                         cam_cc_slow_ahb_clk_src ->
                                    camcc_bps_ahb_clk
                                    camcc_ipe_0_ahb_clk
                                    ...
                                    camcc_core_ahb_clk

Lets see what happens to sm8250 if we remove CAMERA_AHB_CLK from the 
camera clock controller @ camcc: clock-controller@ad00000 {} I don't 
believe that is required.

...

Yep.. does SFA

diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi 
b/arch/arm64/boot/dts/qcom/sm8250.dtsi
index 1efa07f2caff4..1e7d9ee25eeae 100644
--- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
@@ -4172,11 +4172,10 @@ port@5 {
                 camcc: clock-controller@ad00000 {
                         compatible = "qcom,sm8250-camcc";
                         reg = <0 0x0ad00000 0 0x10000>;
-                       clocks = <&gcc GCC_CAMERA_AHB_CLK>,
-                                <&rpmhcc RPMH_CXO_CLK>,
+                       clocks = <&rpmhcc RPMH_CXO_CLK>,
                                  <&rpmhcc RPMH_CXO_CLK_A>,
                                  <&sleep_clk>;
-                       clock-names = "iface", "bi_tcxo", "bi_tcxo_ao", 
"sleep_clk";
+                       clock-names = "bi_tcxo", "bi_tcxo_ao", "sleep_clk";
                         power-domains = <&rpmhpd SM8250_MMCX>;
                         required-opps = <&rpmhpd_opp_low_svs>;
                         status = "disabled";

Not actually a required clock for the clock controller.

I suspect the same is true for dispcc and videocc though it would also 
mean the respective drivers would need to switch on <&gcc 
DISPx_CAMERA_AHB_CLK> or <&gcc GCC_VIDEO_AHB_CLK> prior to accessing 
registers inside the ip blocks which may not currently be the case.

Feels like a bit of a contrary answer but my reading is the 
GCC_IPBLOCK_AHB_CLK clocks belong in the drivers not the clock 
controllers..  or at least that's true for sm8250/camcc

ymmv

---
bod

Re: [PATCH 2/3] arm64: dts: qcom: sc7280: Add Camera Control Interface busses

[Konrad Dybcio]

On 29.09.2023 17:22, Bryan O'Donoghue wrote:
> On 29/09/2023 15:18, Konrad Dybcio wrote:
>> On 29.09.2023 16:15, Bryan O'Donoghue wrote:
>>> On 29/09/2023 14:35, Konrad Dybcio wrote:
>>>>
>>>>
>>>> On 9/29/23 10:01, Luca Weiss wrote:
>>>>> Add the CCI busses found on sc7280 and their pinctrl states.
>>>>>
>>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>>>> ---
>>>>>    arch/arm64/boot/dts/qcom/sc7280.dtsi | 136 +++++++++++++++++++++++++++++++++++
>>>>>    1 file changed, 136 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>>>>> index 66f1eb83cca7..65550de2e4ff 100644
>>>>> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>>>>> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>>>>> @@ -3793,6 +3793,86 @@ videocc: clock-controller@aaf0000 {
>>>>>                #power-domain-cells = <1>;
>>>>>            };
>>>>> +        cci0: cci@ac4a000 {
>>>>> +            compatible = "qcom,sc7280-cci", "qcom,msm8996-cci";
>>>>> +            reg = <0 0x0ac4a000 0 0x1000>;
>>>>> +            interrupts = <GIC_SPI 460 IRQ_TYPE_EDGE_RISING>;
>>>>> +            power-domains = <&camcc CAM_CC_TITAN_TOP_GDSC>;
>>>>> +
>>>>> +            clocks = <&camcc CAM_CC_CAMNOC_AXI_CLK>,
>>>>> +                 <&camcc CAM_CC_SLOW_AHB_CLK_SRC>,
>>>>> +                 <&camcc CAM_CC_CPAS_AHB_CLK>,
>>>>> +                 <&camcc CAM_CC_CCI_0_CLK>,
>>>>> +                 <&camcc CAM_CC_CCI_0_CLK_SRC>;
>>>>> +            clock-names = "camnoc_axi",
>>>>> +                      "slow_ahb_src",
>>>>> +                      "cpas_ahb",
>>>>> +                      "cci",
>>>>> +                      "cci_src";
>>>> I guess this is more of a question to e.g. Bryan, but are all of these clocks actually necessary?
>>>>
>>>> Konrad
>>> Hmm its a good question, we generally take the approach of adopting all of the downstream clocks for these camera interfaces verbatim.
>>>
>>> The clock plan for this part only calls out cci_X_clk and cci_x_clk_src for the CCI however we know that to be incomplete since we *absolutely* need to have the AXI for the block clocked to access those registers, same deal with the AHB bus.
>>>
>>> AXI: registers
>>> AHB: data
>>>
>>> In the above list the only clock you might conceivably not need is CPAS_AHB_CLK.
>>>
>>> Let me zap that clock from sdm845 since I have an rb3 right in front of me and see what happens.
>>>
>>> Crash and reset
>>>
>>> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>>> @@ -4402,13 +4402,11 @@ cci: cci@ac4a000 {
>>>                          clocks = <&clock_camcc CAM_CC_CAMNOC_AXI_CLK>,
>>>                                  <&clock_camcc CAM_CC_SOC_AHB_CLK>,
>>>                                  <&clock_camcc CAM_CC_SLOW_AHB_CLK_SRC>,
>>> -                               <&clock_camcc CAM_CC_CPAS_AHB_CLK>,
>>>                                  <&clock_camcc CAM_CC_CCI_CLK>,
>>>                                  <&clock_camcc CAM_CC_CCI_CLK_SRC>;
>>>                          clock-names = "camnoc_axi",
>>>                                  "soc_ahb",
>>>                                  "slow_ahb_src",
>>> -                               "cpas_ahb",
>>>                                  "cci",
>>>                                  "cci_src";
>>>
>>>
>>> I think the list is good tbh
>> WDYT about camcc consuming ahb, like dispcc does?
>> AXI, hmm.. not quite sure what to do with it
>>
>> Konrad
> 
> Hmm on which platform, camcc clock depds on sdm845 looks very sparse to me.
> 
> 8550 OTOH
> 
> dispcc: clock-controller@af00000 {
>        compatible = "qcom,sm8550-dispcc";
>        reg = <0 0x0af00000 0 0x20000>;
>        clocks = <&bi_tcxo_div2>,
>                 <&bi_tcxo_ao_div2>,
>                 <&gcc GCC_DISP_AHB_CLK>,
> 
> videocc: clock-controller@aaf0000 {
>        compatible = "qcom,sm8550-videocc";
>        reg = <0 0x0aaf0000 0 0x10000>;
>        clocks = <&bi_tcxo_div2>,
>                 <&gcc GCC_VIDEO_AHB_CLK>;
> 
> sm8250
> 
> camcc: clock-controller@ad00000 {
>         compatible = "qcom,sm8250-camcc";
>         reg = <0 0x0ad00000 0 0x10000>;
>         clocks = <&gcc GCC_CAMERA_AHB_CLK>,
> 
> I think those DISP_AHB, VIDEO_AHB_CLK, CAMERA_AHB_CLKs should live in the display, video and camss blocks i.e. they are not clocks that you require to access the clock controller registers themselves...
> 
> I'm seeing for the most part these MEDIAIPBLOCK_AHB_CLKs don't come from the GCC AHB clock but from another root clock generator.
> 
> bi_tcxo ->
> cam_cc_pll0_out_main ->
> cam_cc_pll0_out_even ->
> cam_cc_pll0_out_odd ->
> cam_cc_pll2_out_main ->
>                         cam_cc_slow_ahb_clk_src ->
>                                    camcc_bps_ahb_clk
>                                    camcc_ipe_0_ahb_clk
>                                    ...
>                                    camcc_core_ahb_clk
> 
> Lets see what happens to sm8250 if we remove CAMERA_AHB_CLK from the camera clock controller @ camcc: clock-controller@ad00000 {} I don't believe that is required.
> 
> ...
> 
> Yep.. does SFA
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> index 1efa07f2caff4..1e7d9ee25eeae 100644
> --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> @@ -4172,11 +4172,10 @@ port@5 {
>                 camcc: clock-controller@ad00000 {
>                         compatible = "qcom,sm8250-camcc";
>                         reg = <0 0x0ad00000 0 0x10000>;
> -                       clocks = <&gcc GCC_CAMERA_AHB_CLK>,
> -                                <&rpmhcc RPMH_CXO_CLK>,
> +                       clocks = <&rpmhcc RPMH_CXO_CLK>,
>                                  <&rpmhcc RPMH_CXO_CLK_A>,
>                                  <&sleep_clk>;
> -                       clock-names = "iface", "bi_tcxo", "bi_tcxo_ao", "sleep_clk";
> +                       clock-names = "bi_tcxo", "bi_tcxo_ao", "sleep_clk";
>                         power-domains = <&rpmhpd SM8250_MMCX>;
>                         required-opps = <&rpmhpd_opp_low_svs>;
>                         status = "disabled";
> 
> Not actually a required clock for the clock controller.
> 
> I suspect the same is true for dispcc and videocc though it would also mean the respective drivers would need to switch on <&gcc DISPx_CAMERA_AHB_CLK> or <&gcc GCC_VIDEO_AHB_CLK> prior to accessing registers inside the ip blocks which may not currently be the case.
> 
> Feels like a bit of a contrary answer but my reading is the GCC_IPBLOCK_AHB_CLK clocks belong in the drivers not the clock controllers..  or at least that's true for sm8250/camcc
I believe the idea here would be that registering GCC_IP_AHB_CLK
as a pm_clk for the clock controller would make that clock turn
on when IPBLOCK_CC is accessed (e.g. when we turn on
IPBLOCK_CORE_CLK), so that it doesn't need to be duplicated in
each and every end device.

Konrad

Re: [PATCH 2/3] arm64: dts: qcom: sc7280: Add Camera Control Interface busses

[Bryan O'Donoghue]

On 29/09/2023 16:25, Konrad Dybcio wrote:
>> Not actually a required clock for the clock controller.
>>
>> I suspect the same is true for dispcc and videocc though it would also mean the respective drivers would need to switch on <&gcc DISPx_CAMERA_AHB_CLK> or <&gcc GCC_VIDEO_AHB_CLK> prior to accessing registers inside the ip blocks which may not currently be the case.
>>
>> Feels like a bit of a contrary answer but my reading is the GCC_IPBLOCK_AHB_CLK clocks belong in the drivers not the clock controllers..  or at least that's true for sm8250/camcc
> I believe the idea here would be that registering GCC_IP_AHB_CLK
> as a pm_clk for the clock controller would make that clock turn
> on when IPBLOCK_CC is accessed (e.g. when we turn on
> IPBLOCK_CORE_CLK), so that it doesn't need to be duplicated in
> each and every end device.
> 
> Konrad

Yeah I mean I accept the logic - the core AHB clock is effectively gated 
by the ipblockcc even though they originate from different places in 
hardware - and _when_ do you want one clock without the other ? Never 
except at probe() time for the ipblockcc.

Then again if you can show the clock dependency tree of camera or disp 
requires GCC_IP_AHB_CLK you could make the argument the dt requires the 
clock dependency defined in that block.

I'd say we should offline this from Luca's patches tho :) for me anyway 
the first two are fine.

Agree #3 is verboten. No new empty nodes.

---
bod

Re: [PATCH 1/3] dt-bindings: i2c: qcom-cci: Document SC7280 compatible

[Krzysztof Kozlowski]

On 29/09/2023 10:01, Luca Weiss wrote:
> Document the compatible for the CCI block found on SC7280 SoC.
> 
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> ---
>  Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml b/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml
> index 042d4dc636ee..158588236749 100644
> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml
> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml
> @@ -25,6 +25,7 @@ properties:
>  
>        - items:

This is not enough.

You miss constraining clocks.

Best regards,
Krzysztof