Re: [PATCH V2 RESEND 6/6] arm64: dts: qcom: sm8650: Add video and camera clock controllers

[Konrad Dybcio <konrad.dybcio@linaro.org>]

On 21.03.2024 14:07, Vladimir Zapolskiy wrote:
> Hello Jagadeesh,
> 
> On 3/21/24 11:25, Jagadeesh Kona wrote:
>> Add device nodes for video and camera clock controllers on Qualcomm
>> SM8650 platform.
>>
>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/sm8650.dtsi | 28 ++++++++++++++++++++++++++++
>>   1 file changed, 28 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi
>> index 32c0a7b9aded..d862aa6be824 100644
>> --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi
>> @@ -4,6 +4,8 @@
>>    */
>>     #include <dt-bindings/clock/qcom,rpmh.h>
>> +#include <dt-bindings/clock/qcom,sm8450-videocc.h>
>> +#include <dt-bindings/clock/qcom,sm8650-camcc.h>
>>   #include <dt-bindings/clock/qcom,sm8650-dispcc.h>
>>   #include <dt-bindings/clock/qcom,sm8650-gcc.h>
>>   #include <dt-bindings/clock/qcom,sm8650-gpucc.h>
>> @@ -3110,6 +3112,32 @@ opp-202000000 {
>>               };
>>           };
>>   +        videocc: clock-controller@aaf0000 {
>> +            compatible = "qcom,sm8650-videocc";
>> +            reg = <0 0x0aaf0000 0 0x10000>;
>> +            clocks = <&bi_tcxo_div2>,
>> +                 <&gcc GCC_VIDEO_AHB_CLK>;
>> +            power-domains = <&rpmhpd RPMHPD_MMCX>;
>> +            required-opps = <&rpmhpd_opp_low_svs>;
> 
> Please add default status = "disabled";
> 
>> +            #clock-cells = <1>;
>> +            #reset-cells = <1>;
>> +            #power-domain-cells = <1>;
>> +        };
>> +
>> +        camcc: clock-controller@ade0000 {
>> +            compatible = "qcom,sm8650-camcc";
>> +            reg = <0 0x0ade0000 0 0x20000>;
>> +            clocks = <&gcc GCC_CAMERA_AHB_CLK>,
>> +                 <&bi_tcxo_div2>,
>> +                 <&bi_tcxo_ao_div2>,
>> +                 <&sleep_clk>;
>> +            power-domains = <&rpmhpd RPMHPD_MMCX>;
>> +            required-opps = <&rpmhpd_opp_low_svs>;
> 
> Please add default status = "disabled";
> 
>> +            #clock-cells = <1>;
>> +            #reset-cells = <1>;
>> +            #power-domain-cells = <1>;
>> +        };
>> +
>>           mdss: display-subsystem@ae00000 {
>>               compatible = "qcom,sm8650-mdss";
>>               reg = <0 0x0ae00000 0 0x1000>;
> 
> After disabling the clock controllers

Clock controllers should never be disabled period, that defeats the
entire point of having unused clk/pd cleanup.

The only reason for them to be disabled is for cases where platform
crashes on access due to stinky "security" settings (like with audio
clocks), or when people are too lazy to upstream panel drivers and
end up partially upstreaming display-related changes and continue
using the bootloader-initialized framebuffer. This takes away from
the very little determinism we have.

Konrad