Re: [PATCH] arm64: dts: qcom: sm8650: add description of CCI controllers

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

On 10/04/2024 08:49, Vladimir Zapolskiy wrote:
> Qualcomm SM8650 SoC has three CCI controllers with two I2C busses
> connected to each of them.
> 
> The CCI controllers on SM8650 are compatible with the ones found on
> many other older generations of Qualcomm SoCs.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
> The change is based and depends on a patch series from Jagadeesh Kona:
> 
>    https://lore.kernel.org/linux-arm-msm/20240321092529.13362-1-quic_jkona@quicinc.com/
> 
> It might be an option to add this change right to the series,
> since it anyway requires a respin.
> 
> A new compatible value "qcom,sm8650-cci" is NOT added to
> Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml , because
> the controller IP description and selection is covered by a generic
> compatible value "qcom,msm8996-cci".
> 
>   arch/arm64/boot/dts/qcom/sm8650.dtsi | 315 +++++++++++++++++++++++++++
>   1 file changed, 315 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi
> index b406835b2e71..160b618dff9c 100644
> --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi
> @@ -3122,6 +3122,114 @@ videocc: clock-controller@aaf0000 {
>   			#power-domain-cells = <1>;
>   		};
>   
> +		cci0: cci@ac15000 {
> +			compatible = "qcom,sm8650-cci", "qcom,msm8996-cci";
> +			reg = <0 0x0ac15000 0 0x1000>;
> +			interrupts = <GIC_SPI 426 IRQ_TYPE_EDGE_RISING>;
> +			power-domains = <&camcc CAM_CC_TITAN_TOP_GDSC>;
> +			clocks = <&camcc CAM_CC_CAMNOC_AXI_NRT_CLK>,
> +				 <&camcc CAM_CC_SLOW_AHB_CLK_SRC>,
> +				 <&camcc CAM_CC_CPAS_AHB_CLK>,

If you are taking the CPAS_AHB_CLK you don't need to take 
SLOW_AHB_CLK_SRC since SLOW_AHB_CLK_SRC is the parent clock of CPAS_AHB_CLK.

https://lore.kernel.org/linux-arm-msm/20240321092529.13362-6-quic_jkona@quicinc.com/

static struct clk_branch cam_cc_cpas_ahb_clk = {};

CPAS_AHB_CLK will get you the AHB clock you need on its own.

Same comment for cci1, cci2.

Other than that LGTM, provided you resolve the compat string stuff - add.

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

---
bod