diff mbox series

[v2,4/4] arm64: dts: qcom: sm6350: Add video clock controller

Message ID 20250324-sm6350-videocc-v2-4-cc22386433f4@fairphone.com
State New
Headers show
Series Add video clock controller for SM6350 | expand

Commit Message

Luca Weiss March 24, 2025, 8:41 a.m. UTC
Add a node for the videocc found on the SM6350 SoC.

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

Comments

Dmitry Baryshkov March 24, 2025, 10:56 a.m. UTC | #1
On Mon, Mar 24, 2025 at 09:41:04AM +0100, Luca Weiss wrote:
> Add a node for the videocc found on the SM6350 SoC.
> 
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> ---
>  arch/arm64/boot/dts/qcom/sm6350.dtsi | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Konrad Dybcio April 1, 2025, 4:33 p.m. UTC | #2
On 3/24/25 9:41 AM, Luca Weiss wrote:
> Add a node for the videocc found on the SM6350 SoC.
> 
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> ---
>  arch/arm64/boot/dts/qcom/sm6350.dtsi | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm6350.dtsi b/arch/arm64/boot/dts/qcom/sm6350.dtsi
> index 42f9d16c2fa6da66a8bb524a33c2687a1e4b40e0..4498d6dfd61a7e30a050a8654d54dae2d06c220c 100644
> --- a/arch/arm64/boot/dts/qcom/sm6350.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm6350.dtsi
> @@ -1952,6 +1952,20 @@ usb_1_dwc3_ss_out: endpoint {
>  			};
>  		};
>  
> +		videocc: clock-controller@aaf0000 {
> +			compatible = "qcom,sm6350-videocc";
> +			reg = <0x0 0x0aaf0000 0x0 0x10000>;
> +			clocks = <&gcc GCC_VIDEO_AHB_CLK>,
> +				 <&rpmhcc RPMH_CXO_CLK>,
> +				 <&sleep_clk>;
> +			clock-names = "iface",
> +				      "bi_tcxo",
> +				      "sleep_clk";
> +			#clock-cells = <1>;
> +			#reset-cells = <1>;
> +			#power-domain-cells = <1>;
> +		};

You'll probably want to hook up some additional power domains here, see

https://lore.kernel.org/linux-arm-msm/20250327-videocc-pll-multi-pd-voting-v3-0-895fafd62627@quicinc.com/

Konrad
Jagadeesh Kona April 11, 2025, 7:15 a.m. UTC | #3
On 4/1/2025 10:03 PM, Konrad Dybcio wrote:
> On 3/24/25 9:41 AM, Luca Weiss wrote:
>> Add a node for the videocc found on the SM6350 SoC.
>>
>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>> ---
>>  arch/arm64/boot/dts/qcom/sm6350.dtsi | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sm6350.dtsi b/arch/arm64/boot/dts/qcom/sm6350.dtsi
>> index 42f9d16c2fa6da66a8bb524a33c2687a1e4b40e0..4498d6dfd61a7e30a050a8654d54dae2d06c220c 100644
>> --- a/arch/arm64/boot/dts/qcom/sm6350.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sm6350.dtsi
>> @@ -1952,6 +1952,20 @@ usb_1_dwc3_ss_out: endpoint {
>>  			};
>>  		};
>>  
>> +		videocc: clock-controller@aaf0000 {
>> +			compatible = "qcom,sm6350-videocc";
>> +			reg = <0x0 0x0aaf0000 0x0 0x10000>;
>> +			clocks = <&gcc GCC_VIDEO_AHB_CLK>,
>> +				 <&rpmhcc RPMH_CXO_CLK>,
>> +				 <&sleep_clk>;
>> +			clock-names = "iface",
>> +				      "bi_tcxo",
>> +				      "sleep_clk";
>> +			#clock-cells = <1>;
>> +			#reset-cells = <1>;
>> +			#power-domain-cells = <1>;
>> +		};
> 
> You'll probably want to hook up some additional power domains here, see
> 
> https://lore.kernel.org/linux-arm-msm/20250327-videocc-pll-multi-pd-voting-v3-0-895fafd62627@quicinc.com/
> 

On SM6350, videocc doesn't need multiple power domains at HW level, it is only on CX rail which would be ON
when system is active, hence power-domains are not mandatory here.

Thanks,
Jagadeesh 

> Konrad
>
Konrad Dybcio April 11, 2025, 9:12 a.m. UTC | #4
On 4/11/25 9:15 AM, Jagadeesh Kona wrote:
> 
> 
> On 4/1/2025 10:03 PM, Konrad Dybcio wrote:
>> On 3/24/25 9:41 AM, Luca Weiss wrote:
>>> Add a node for the videocc found on the SM6350 SoC.
>>>
>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>> ---
>>>  arch/arm64/boot/dts/qcom/sm6350.dtsi | 14 ++++++++++++++
>>>  1 file changed, 14 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sm6350.dtsi b/arch/arm64/boot/dts/qcom/sm6350.dtsi
>>> index 42f9d16c2fa6da66a8bb524a33c2687a1e4b40e0..4498d6dfd61a7e30a050a8654d54dae2d06c220c 100644
>>> --- a/arch/arm64/boot/dts/qcom/sm6350.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sm6350.dtsi
>>> @@ -1952,6 +1952,20 @@ usb_1_dwc3_ss_out: endpoint {
>>>  			};
>>>  		};
>>>  
>>> +		videocc: clock-controller@aaf0000 {
>>> +			compatible = "qcom,sm6350-videocc";
>>> +			reg = <0x0 0x0aaf0000 0x0 0x10000>;
>>> +			clocks = <&gcc GCC_VIDEO_AHB_CLK>,
>>> +				 <&rpmhcc RPMH_CXO_CLK>,
>>> +				 <&sleep_clk>;
>>> +			clock-names = "iface",
>>> +				      "bi_tcxo",
>>> +				      "sleep_clk";
>>> +			#clock-cells = <1>;
>>> +			#reset-cells = <1>;
>>> +			#power-domain-cells = <1>;
>>> +		};
>>
>> You'll probably want to hook up some additional power domains here, see
>>
>> https://lore.kernel.org/linux-arm-msm/20250327-videocc-pll-multi-pd-voting-v3-0-895fafd62627@quicinc.com/
>>
> 
> On SM6350, videocc doesn't need multiple power domains at HW level, it is only on CX rail which would be ON
> when system is active, hence power-domains are not mandatory here.

6350 doesn't have either MMCX nor a split MX - shouldn't both normal
CX and MX be in there?

Konrad
Jagadeesh Kona April 11, 2025, 11:37 a.m. UTC | #5
On 4/11/2025 2:42 PM, Konrad Dybcio wrote:
> On 4/11/25 9:15 AM, Jagadeesh Kona wrote:
>>
>>
>> On 4/1/2025 10:03 PM, Konrad Dybcio wrote:
>>> On 3/24/25 9:41 AM, Luca Weiss wrote:
>>>> Add a node for the videocc found on the SM6350 SoC.
>>>>
>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>>> ---
>>>>  arch/arm64/boot/dts/qcom/sm6350.dtsi | 14 ++++++++++++++
>>>>  1 file changed, 14 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/qcom/sm6350.dtsi b/arch/arm64/boot/dts/qcom/sm6350.dtsi
>>>> index 42f9d16c2fa6da66a8bb524a33c2687a1e4b40e0..4498d6dfd61a7e30a050a8654d54dae2d06c220c 100644
>>>> --- a/arch/arm64/boot/dts/qcom/sm6350.dtsi
>>>> +++ b/arch/arm64/boot/dts/qcom/sm6350.dtsi
>>>> @@ -1952,6 +1952,20 @@ usb_1_dwc3_ss_out: endpoint {
>>>>  			};
>>>>  		};
>>>>  
>>>> +		videocc: clock-controller@aaf0000 {
>>>> +			compatible = "qcom,sm6350-videocc";
>>>> +			reg = <0x0 0x0aaf0000 0x0 0x10000>;
>>>> +			clocks = <&gcc GCC_VIDEO_AHB_CLK>,
>>>> +				 <&rpmhcc RPMH_CXO_CLK>,
>>>> +				 <&sleep_clk>;
>>>> +			clock-names = "iface",
>>>> +				      "bi_tcxo",
>>>> +				      "sleep_clk";
>>>> +			#clock-cells = <1>;
>>>> +			#reset-cells = <1>;
>>>> +			#power-domain-cells = <1>;
>>>> +		};
>>>
>>> You'll probably want to hook up some additional power domains here, see
>>>
>>> https://lore.kernel.org/linux-arm-msm/20250327-videocc-pll-multi-pd-voting-v3-0-895fafd62627@quicinc.com/
>>>
>>
>> On SM6350, videocc doesn't need multiple power domains at HW level, it is only on CX rail which would be ON
>> when system is active, hence power-domains are not mandatory here.
> 
> 6350 doesn't have either MMCX nor a split MX - shouldn't both normal
> CX and MX be in there?
> 

All clocks & GDSC's of SM6350 videocc are only on CX rail, so it requires only CX power domain. But when HLOS
is active, CX rail will be ON and operate at a level above retention, which is sufficient for videocc to operate.
Hence clock driver don't need to explicitly vote on CX rail.

The same is not true for other rails like MMCX and Split MX(MXC), hence clock drivers had to explicitly vote on
those rails.

Thanks,
Jagadeesh 
 
> Konrad
Konrad Dybcio April 11, 2025, 7:26 p.m. UTC | #6
On 4/11/25 1:37 PM, Jagadeesh Kona wrote:
> 
> 
> On 4/11/2025 2:42 PM, Konrad Dybcio wrote:
>> On 4/11/25 9:15 AM, Jagadeesh Kona wrote:
>>>
>>>
>>> On 4/1/2025 10:03 PM, Konrad Dybcio wrote:
>>>> On 3/24/25 9:41 AM, Luca Weiss wrote:
>>>>> Add a node for the videocc found on the SM6350 SoC.
>>>>>
>>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>>>> ---
>>>>>  arch/arm64/boot/dts/qcom/sm6350.dtsi | 14 ++++++++++++++
>>>>>  1 file changed, 14 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/qcom/sm6350.dtsi b/arch/arm64/boot/dts/qcom/sm6350.dtsi
>>>>> index 42f9d16c2fa6da66a8bb524a33c2687a1e4b40e0..4498d6dfd61a7e30a050a8654d54dae2d06c220c 100644
>>>>> --- a/arch/arm64/boot/dts/qcom/sm6350.dtsi
>>>>> +++ b/arch/arm64/boot/dts/qcom/sm6350.dtsi
>>>>> @@ -1952,6 +1952,20 @@ usb_1_dwc3_ss_out: endpoint {
>>>>>  			};
>>>>>  		};
>>>>>  
>>>>> +		videocc: clock-controller@aaf0000 {
>>>>> +			compatible = "qcom,sm6350-videocc";
>>>>> +			reg = <0x0 0x0aaf0000 0x0 0x10000>;
>>>>> +			clocks = <&gcc GCC_VIDEO_AHB_CLK>,
>>>>> +				 <&rpmhcc RPMH_CXO_CLK>,
>>>>> +				 <&sleep_clk>;
>>>>> +			clock-names = "iface",
>>>>> +				      "bi_tcxo",
>>>>> +				      "sleep_clk";
>>>>> +			#clock-cells = <1>;
>>>>> +			#reset-cells = <1>;
>>>>> +			#power-domain-cells = <1>;
>>>>> +		};
>>>>
>>>> You'll probably want to hook up some additional power domains here, see
>>>>
>>>> https://lore.kernel.org/linux-arm-msm/20250327-videocc-pll-multi-pd-voting-v3-0-895fafd62627@quicinc.com/
>>>>
>>>
>>> On SM6350, videocc doesn't need multiple power domains at HW level, it is only on CX rail which would be ON
>>> when system is active, hence power-domains are not mandatory here.
>>
>> 6350 doesn't have either MMCX nor a split MX - shouldn't both normal
>> CX and MX be in there?
>>
> 
> All clocks & GDSC's of SM6350 videocc are only on CX rail, so it requires only CX power domain. But when HLOS
> is active, CX rail will be ON and operate at a level above retention, which is sufficient for videocc to operate.
> Hence clock driver don't need to explicitly vote on CX rail.
> 
> The same is not true for other rails like MMCX and Split MX(MXC), hence clock drivers had to explicitly vote on
> those rails.

I'm worried about MX being undervolted for higher OPPs

Konrad
Taniya Das April 15, 2025, 4:05 a.m. UTC | #7
On 4/12/2025 12:56 AM, Konrad Dybcio wrote:
> On 4/11/25 1:37 PM, Jagadeesh Kona wrote:
>>
>>
>> On 4/11/2025 2:42 PM, Konrad Dybcio wrote:
>>> On 4/11/25 9:15 AM, Jagadeesh Kona wrote:
>>>>
>>>>
>>>> On 4/1/2025 10:03 PM, Konrad Dybcio wrote:
>>>>> On 3/24/25 9:41 AM, Luca Weiss wrote:
>>>>>> Add a node for the videocc found on the SM6350 SoC.
>>>>>>
>>>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>>>>> ---
>>>>>>  arch/arm64/boot/dts/qcom/sm6350.dtsi | 14 ++++++++++++++
>>>>>>  1 file changed, 14 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/arm64/boot/dts/qcom/sm6350.dtsi b/arch/arm64/boot/dts/qcom/sm6350.dtsi
>>>>>> index 42f9d16c2fa6da66a8bb524a33c2687a1e4b40e0..4498d6dfd61a7e30a050a8654d54dae2d06c220c 100644
>>>>>> --- a/arch/arm64/boot/dts/qcom/sm6350.dtsi
>>>>>> +++ b/arch/arm64/boot/dts/qcom/sm6350.dtsi
>>>>>> @@ -1952,6 +1952,20 @@ usb_1_dwc3_ss_out: endpoint {
>>>>>>  			};
>>>>>>  		};
>>>>>>  
>>>>>> +		videocc: clock-controller@aaf0000 {
>>>>>> +			compatible = "qcom,sm6350-videocc";
>>>>>> +			reg = <0x0 0x0aaf0000 0x0 0x10000>;
>>>>>> +			clocks = <&gcc GCC_VIDEO_AHB_CLK>,
>>>>>> +				 <&rpmhcc RPMH_CXO_CLK>,
>>>>>> +				 <&sleep_clk>;
>>>>>> +			clock-names = "iface",
>>>>>> +				      "bi_tcxo",
>>>>>> +				      "sleep_clk";
>>>>>> +			#clock-cells = <1>;
>>>>>> +			#reset-cells = <1>;
>>>>>> +			#power-domain-cells = <1>;
>>>>>> +		};
>>>>>
>>>>> You'll probably want to hook up some additional power domains here, see
>>>>>
>>>>> https://lore.kernel.org/linux-arm-msm/20250327-videocc-pll-multi-pd-voting-v3-0-895fafd62627@quicinc.com/
>>>>>
>>>>
>>>> On SM6350, videocc doesn't need multiple power domains at HW level, it is only on CX rail which would be ON
>>>> when system is active, hence power-domains are not mandatory here.
>>>
>>> 6350 doesn't have either MMCX nor a split MX - shouldn't both normal
>>> CX and MX be in there?
>>>
>>
>> All clocks & GDSC's of SM6350 videocc are only on CX rail, so it requires only CX power domain. But when HLOS
>> is active, CX rail will be ON and operate at a level above retention, which is sufficient for videocc to operate.
>> Hence clock driver don't need to explicitly vote on CX rail.
>>
>> The same is not true for other rails like MMCX and Split MX(MXC), hence clock drivers had to explicitly vote on
>> those rails.
> 
> I'm worried about MX being undervolted for higher OPPs
>
Konrad Dybcio April 15, 2025, 9:13 a.m. UTC | #8
On 4/15/25 6:05 AM, Taniya Das wrote:
> 
> 
> On 4/12/2025 12:56 AM, Konrad Dybcio wrote:
>> On 4/11/25 1:37 PM, Jagadeesh Kona wrote:
>>>
>>>
>>> On 4/11/2025 2:42 PM, Konrad Dybcio wrote:
>>>> On 4/11/25 9:15 AM, Jagadeesh Kona wrote:
>>>>>
>>>>>
>>>>> On 4/1/2025 10:03 PM, Konrad Dybcio wrote:
>>>>>> On 3/24/25 9:41 AM, Luca Weiss wrote:
>>>>>>> Add a node for the videocc found on the SM6350 SoC.
>>>>>>>
>>>>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>>>>>> ---
>>>>>>>  arch/arm64/boot/dts/qcom/sm6350.dtsi | 14 ++++++++++++++
>>>>>>>  1 file changed, 14 insertions(+)
>>>>>>>
>>>>>>> diff --git a/arch/arm64/boot/dts/qcom/sm6350.dtsi b/arch/arm64/boot/dts/qcom/sm6350.dtsi
>>>>>>> index 42f9d16c2fa6da66a8bb524a33c2687a1e4b40e0..4498d6dfd61a7e30a050a8654d54dae2d06c220c 100644
>>>>>>> --- a/arch/arm64/boot/dts/qcom/sm6350.dtsi
>>>>>>> +++ b/arch/arm64/boot/dts/qcom/sm6350.dtsi
>>>>>>> @@ -1952,6 +1952,20 @@ usb_1_dwc3_ss_out: endpoint {
>>>>>>>  			};
>>>>>>>  		};
>>>>>>>  
>>>>>>> +		videocc: clock-controller@aaf0000 {
>>>>>>> +			compatible = "qcom,sm6350-videocc";
>>>>>>> +			reg = <0x0 0x0aaf0000 0x0 0x10000>;
>>>>>>> +			clocks = <&gcc GCC_VIDEO_AHB_CLK>,
>>>>>>> +				 <&rpmhcc RPMH_CXO_CLK>,
>>>>>>> +				 <&sleep_clk>;
>>>>>>> +			clock-names = "iface",
>>>>>>> +				      "bi_tcxo",
>>>>>>> +				      "sleep_clk";
>>>>>>> +			#clock-cells = <1>;
>>>>>>> +			#reset-cells = <1>;
>>>>>>> +			#power-domain-cells = <1>;
>>>>>>> +		};
>>>>>>
>>>>>> You'll probably want to hook up some additional power domains here, see
>>>>>>
>>>>>> https://lore.kernel.org/linux-arm-msm/20250327-videocc-pll-multi-pd-voting-v3-0-895fafd62627@quicinc.com/
>>>>>>
>>>>>
>>>>> On SM6350, videocc doesn't need multiple power domains at HW level, it is only on CX rail which would be ON
>>>>> when system is active, hence power-domains are not mandatory here.
>>>>
>>>> 6350 doesn't have either MMCX nor a split MX - shouldn't both normal
>>>> CX and MX be in there?
>>>>
>>>
>>> All clocks & GDSC's of SM6350 videocc are only on CX rail, so it requires only CX power domain. But when HLOS
>>> is active, CX rail will be ON and operate at a level above retention, which is sufficient for videocc to operate.
>>> Hence clock driver don't need to explicitly vote on CX rail.
>>>
>>> The same is not true for other rails like MMCX and Split MX(MXC), hence clock drivers had to explicitly vote on
>>> those rails.
>>
>> I'm worried about MX being undervolted for higher OPPs
>>
> 
> From a videocc PoV there is no requirement of Mx on SM6350. The CX
> levels would be taken care by Video SW driver from their defined OPP. Mx
> at system level would be catered via the BW votes.

So I'm specifically thinking about the videocc (and other) PLLs, which
have defined vdd levels downstream - currently we're relying on random
luck rather than ensuring each one of them has its requirements fulfilled

Konrad
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/sm6350.dtsi b/arch/arm64/boot/dts/qcom/sm6350.dtsi
index 42f9d16c2fa6da66a8bb524a33c2687a1e4b40e0..4498d6dfd61a7e30a050a8654d54dae2d06c220c 100644
--- a/arch/arm64/boot/dts/qcom/sm6350.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm6350.dtsi
@@ -1952,6 +1952,20 @@  usb_1_dwc3_ss_out: endpoint {
 			};
 		};
 
+		videocc: clock-controller@aaf0000 {
+			compatible = "qcom,sm6350-videocc";
+			reg = <0x0 0x0aaf0000 0x0 0x10000>;
+			clocks = <&gcc GCC_VIDEO_AHB_CLK>,
+				 <&rpmhcc RPMH_CXO_CLK>,
+				 <&sleep_clk>;
+			clock-names = "iface",
+				      "bi_tcxo",
+				      "sleep_clk";
+			#clock-cells = <1>;
+			#reset-cells = <1>;
+			#power-domain-cells = <1>;
+		};
+
 		cci0: cci@ac4a000 {
 			compatible = "qcom,sm6350-cci", "qcom,msm8996-cci";
 			reg = <0x0 0x0ac4a000 0x0 0x1000>;