[PATCH v1 1/3] dt-bindings: pci: qcom: Add binding for operating-points-v2

[PATCH v1 1/3] dt-bindings: pci: qcom: Add binding for operating-points-v2

[Krishna chaitanya chundru]

This adds a binding documenting operating-points-v2.
Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
 Documentation/devicetree/bindings/pci/qcom,pcie.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
index 81971be4..6bc99c5 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
@@ -121,6 +121,8 @@ properties:
     description: GPIO controlled connection to WAKE# signal
     maxItems: 1
 
+  operating-points-v2: true
+
 required:
   - compatible
   - reg
-- 
2.7.4

[PATCH v1 2/3] arm64: dts: qcom: sm8450: Add opp table support to PCIe

[Krishna chaitanya chundru]

PCIe needs to choose the appropriate performance state of RPMH power
domain based upon the PCIe gen speed.

So, let's add the OPP table support to specify RPMH performance states.

Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sm8450.dtsi | 47 ++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
index 595533a..681ea9c 100644
--- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
@@ -381,6 +381,49 @@
 		};
 	};
 
+	pcie0_opp_table: opp-table-pcie0 {
+		compatible = "operating-points-v2";
+
+		opp-2500000 {
+			opp-hz = /bits/ 64 <2500000>;
+			opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
+		};
+
+		opp-5000000 {
+			opp-hz = /bits/ 64 <5000000>;
+			opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
+		};
+
+		opp-8000000 {
+			opp-hz = /bits/ 64 <8000000>;
+			opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
+		};
+	};
+
+	pcie1_opp_table: opp-table-pcie1 {
+		compatible = "operating-points-v2";
+
+		opp-2500000 {
+			opp-hz = /bits/ 64 <2500000>;
+			opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
+		};
+
+		opp-5000000 {
+			opp-hz = /bits/ 64 <5000000>;
+			opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
+		};
+
+		opp-8000000 {
+			opp-hz = /bits/ 64 <8000000>;
+			opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
+		};
+
+		opp-16000000 {
+			opp-hz = /bits/ 64 <16000000>;
+			opp-level = <RPMH_REGULATOR_LEVEL_NOM>;
+		};
+	};
+
 	reserved_memory: reserved-memory {
 		#address-cells = <2>;
 		#size-cells = <2>;
@@ -1803,6 +1846,8 @@
 			pinctrl-names = "default";
 			pinctrl-0 = <&pcie0_default_state>;
 
+			operating-points-v2 = <&pcie0_opp_table>;
+
 			status = "disabled";
 		};
 
@@ -1915,6 +1960,8 @@
 			pinctrl-names = "default";
 			pinctrl-0 = <&pcie1_default_state>;
 
+			operating-points-v2 = <&pcie1_opp_table>;
+
 			status = "disabled";
 		};
 
-- 
2.7.4

Re: [PATCH v1 1/3] dt-bindings: pci: qcom: Add binding for operating-points-v2

[Krzysztof Kozlowski]

On 15/08/2023 14:26, Krishna chaitanya chundru wrote:
> This adds a binding documenting operating-points-v2.

1. Missing blank line. Don't write patches by yourself, but use tools
which create proper commit automatically. Every decent editor does it.

2. Please do not use "This commit/patch", but imperative mood. See
longer explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

3. A nit, subject: drop second/last, redundant "binding for". The
"dt-bindings" prefix is already stating that these are bindings.


> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
>  Documentation/devicetree/bindings/pci/qcom,pcie.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> index 81971be4..6bc99c5 100644
> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> @@ -121,6 +121,8 @@ properties:
>      description: GPIO controlled connection to WAKE# signal
>      maxItems: 1
>  
> +  operating-points-v2: true

phandle without actual table (opp-table) is rather meaningless.

Best regards,
Krzysztof

Re: [PATCH v1 2/3] arm64: dts: qcom: sm8450: Add opp table support to PCIe

[Krzysztof Kozlowski]

On 15/08/2023 14:26, Krishna chaitanya chundru wrote:
> PCIe needs to choose the appropriate performance state of RPMH power
> domain based upon the PCIe gen speed.

This explanation should be also in bindings patch, otherwise why would
we consider the bindings patch?

> 
> So, let's add the OPP table support to specify RPMH performance states.
> 
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sm8450.dtsi | 47 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> index 595533a..681ea9c 100644
> --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> @@ -381,6 +381,49 @@
>  		};
>  	};
>  
> +	pcie0_opp_table: opp-table-pcie0 {
> +		compatible = "operating-points-v2";
> +
> +		opp-2500000 {
> +			opp-hz = /bits/ 64 <2500000>;
> +			opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
> +		};
> +
> +		opp-5000000 {
> +			opp-hz = /bits/ 64 <5000000>;
> +			opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
> +		};
> +
> +		opp-8000000 {
> +			opp-hz = /bits/ 64 <8000000>;
> +			opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
> +		};
> +	};
> +
> +	pcie1_opp_table: opp-table-pcie1 {
> +		compatible = "operating-points-v2";
> +
> +		opp-2500000 {
> +			opp-hz = /bits/ 64 <2500000>;
> +			opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
> +		};
> +
> +		opp-5000000 {
> +			opp-hz = /bits/ 64 <5000000>;
> +			opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
> +		};
> +
> +		opp-8000000 {
> +			opp-hz = /bits/ 64 <8000000>;
> +			opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
> +		};
> +
> +		opp-16000000 {
> +			opp-hz = /bits/ 64 <16000000>;
> +			opp-level = <RPMH_REGULATOR_LEVEL_NOM>;
> +		};
> +	};
> +
>  	reserved_memory: reserved-memory {
>  		#address-cells = <2>;
>  		#size-cells = <2>;
> @@ -1803,6 +1846,8 @@
>  			pinctrl-names = "default";
>  			pinctrl-0 = <&pcie0_default_state>;
>  
> +			operating-points-v2 = <&pcie0_opp_table>;

Why the table is not here? Is it shared with multiple devices?

Best regards,
Krzysztof

Re: [PATCH v1 2/3] arm64: dts: qcom: sm8450: Add opp table support to PCIe

[Pavan Kondeti]

On Tue, Aug 15, 2023 at 05:56:47PM +0530, Krishna chaitanya chundru wrote:
> PCIe needs to choose the appropriate performance state of RPMH power
> domain based upon the PCIe gen speed.
> 
> So, let's add the OPP table support to specify RPMH performance states.
> 
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sm8450.dtsi | 47 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> index 595533a..681ea9c 100644
> --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> @@ -381,6 +381,49 @@
>  		};
>  	};
>  
> +	pcie0_opp_table: opp-table-pcie0 {
> +		compatible = "operating-points-v2";
> +
> +		opp-2500000 {
> +			opp-hz = /bits/ 64 <2500000>;
> +			opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
> +		};
> +
> +		opp-5000000 {
> +			opp-hz = /bits/ 64 <5000000>;
> +			opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
> +		};
> +
> +		opp-8000000 {
> +			opp-hz = /bits/ 64 <8000000>;
> +			opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
> +		};
> +	};
> +
> +	pcie1_opp_table: opp-table-pcie1 {
> +		compatible = "operating-points-v2";
> +
> +		opp-2500000 {
> +			opp-hz = /bits/ 64 <2500000>;
> +			opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
> +		};
> +
> +		opp-5000000 {
> +			opp-hz = /bits/ 64 <5000000>;
> +			opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
> +		};
> +
> +		opp-8000000 {
> +			opp-hz = /bits/ 64 <8000000>;
> +			opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
> +		};
> +
> +		opp-16000000 {
> +			opp-hz = /bits/ 64 <16000000>;
> +			opp-level = <RPMH_REGULATOR_LEVEL_NOM>;
> +		};
> +	};
> +

Should not we using required-opps property to pass the
rpmhpd_opp_xxx phandle so that when this OPP is selected based on your
clock rate, the appropriate OPP (voltage) would be selected on the RPMH side?

Please see SDHCI/MMC voting (sdhc2_opp_table) as an example.

Thanks,
Pavan

Re: [PATCH v1 1/3] dt-bindings: pci: qcom: Add binding for operating-points-v2

[Krishna Chaitanya Chundru]

On 8/15/2023 6:00 PM, Krzysztof Kozlowski wrote:
> On 15/08/2023 14:26, Krishna chaitanya chundru wrote:
>> This adds a binding documenting operating-points-v2.
> 1. Missing blank line. Don't write patches by yourself, but use tools
> which create proper commit automatically. Every decent editor does it.
>
> 2. Please do not use "This commit/patch", but imperative mood. See
> longer explanation here:
> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
>
> 3. A nit, subject: drop second/last, redundant "binding for". The
> "dt-bindings" prefix is already stating that these are bindings.
>
>
>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>> ---
>>   Documentation/devicetree/bindings/pci/qcom,pcie.yaml | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>> index 81971be4..6bc99c5 100644
>> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>> @@ -121,6 +121,8 @@ properties:
>>       description: GPIO controlled connection to WAKE# signal
>>       maxItems: 1
>>   
>> +  operating-points-v2: true
> phandle without actual table (opp-table) is rather meaningless.
>
> Best regards,
> Krzysztof

I will take all your comments and will send next patch

- KC

Re: [PATCH v1 2/3] arm64: dts: qcom: sm8450: Add opp table support to PCIe

[Krishna Chaitanya Chundru]

On 8/15/2023 6:01 PM, Krzysztof Kozlowski wrote:
> On 15/08/2023 14:26, Krishna chaitanya chundru wrote:
>> PCIe needs to choose the appropriate performance state of RPMH power
>> domain based upon the PCIe gen speed.
> This explanation should be also in bindings patch, otherwise why would
> we consider the bindings patch?
I will update binding patch with this information.
>
>> So, let's add the OPP table support to specify RPMH performance states.
>>
>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/sm8450.dtsi | 47 ++++++++++++++++++++++++++++++++++++
>>   1 file changed, 47 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
>> index 595533a..681ea9c 100644
>> --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
>> @@ -381,6 +381,49 @@
>>   		};
>>   	};
>>   
>> +	pcie0_opp_table: opp-table-pcie0 {
>> +		compatible = "operating-points-v2";
>> +
>> +		opp-2500000 {
>> +			opp-hz = /bits/ 64 <2500000>;
>> +			opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
>> +		};
>> +
>> +		opp-5000000 {
>> +			opp-hz = /bits/ 64 <5000000>;
>> +			opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
>> +		};
>> +
>> +		opp-8000000 {
>> +			opp-hz = /bits/ 64 <8000000>;
>> +			opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
>> +		};
>> +	};
>> +
>> +	pcie1_opp_table: opp-table-pcie1 {
>> +		compatible = "operating-points-v2";
>> +
>> +		opp-2500000 {
>> +			opp-hz = /bits/ 64 <2500000>;
>> +			opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
>> +		};
>> +
>> +		opp-5000000 {
>> +			opp-hz = /bits/ 64 <5000000>;
>> +			opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
>> +		};
>> +
>> +		opp-8000000 {
>> +			opp-hz = /bits/ 64 <8000000>;
>> +			opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
>> +		};
>> +
>> +		opp-16000000 {
>> +			opp-hz = /bits/ 64 <16000000>;
>> +			opp-level = <RPMH_REGULATOR_LEVEL_NOM>;
>> +		};
>> +	};
>> +
>>   	reserved_memory: reserved-memory {
>>   		#address-cells = <2>;
>>   		#size-cells = <2>;
>> @@ -1803,6 +1846,8 @@
>>   			pinctrl-names = "default";
>>   			pinctrl-0 = <&pcie0_default_state>;
>>   
>> +			operating-points-v2 = <&pcie0_opp_table>;
> Why the table is not here? Is it shared with multiple devices?

I will move the table to here in the next patch.

- KC

>
> Best regards,
> Krzysztof
>

Re: [PATCH v1 2/3] arm64: dts: qcom: sm8450: Add opp table support to PCIe

[Krishna Chaitanya Chundru]

On 8/16/2023 12:35 PM, Pavan Kondeti wrote:
> On Tue, Aug 15, 2023 at 05:56:47PM +0530, Krishna chaitanya chundru wrote:
>> PCIe needs to choose the appropriate performance state of RPMH power
>> domain based upon the PCIe gen speed.
>>
>> So, let's add the OPP table support to specify RPMH performance states.
>>
>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/sm8450.dtsi | 47 ++++++++++++++++++++++++++++++++++++
>>   1 file changed, 47 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
>> index 595533a..681ea9c 100644
>> --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
>> @@ -381,6 +381,49 @@
>>   		};
>>   	};
>>   
>> +	pcie0_opp_table: opp-table-pcie0 {
>> +		compatible = "operating-points-v2";
>> +
>> +		opp-2500000 {
>> +			opp-hz = /bits/ 64 <2500000>;
>> +			opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
>> +		};
>> +
>> +		opp-5000000 {
>> +			opp-hz = /bits/ 64 <5000000>;
>> +			opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
>> +		};
>> +
>> +		opp-8000000 {
>> +			opp-hz = /bits/ 64 <8000000>;
>> +			opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
>> +		};
>> +	};
>> +
>> +	pcie1_opp_table: opp-table-pcie1 {
>> +		compatible = "operating-points-v2";
>> +
>> +		opp-2500000 {
>> +			opp-hz = /bits/ 64 <2500000>;
>> +			opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
>> +		};
>> +
>> +		opp-5000000 {
>> +			opp-hz = /bits/ 64 <5000000>;
>> +			opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
>> +		};
>> +
>> +		opp-8000000 {
>> +			opp-hz = /bits/ 64 <8000000>;
>> +			opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
>> +		};
>> +
>> +		opp-16000000 {
>> +			opp-hz = /bits/ 64 <16000000>;
>> +			opp-level = <RPMH_REGULATOR_LEVEL_NOM>;
>> +		};
>> +	};
>> +
> Should not we using required-opps property to pass the
> rpmhpd_opp_xxx phandle so that when this OPP is selected based on your
> clock rate, the appropriate OPP (voltage) would be selected on the RPMH side?
>
> Please see SDHCI/MMC voting (sdhc2_opp_table) as an example.

Sure I will try to use rpmhpd_opp_xxx phandle in next patch

- KC

>
> Thanks,
> Pavan

Re: [PATCH v1 2/3] arm64: dts: qcom: sm8450: Add opp table support to PCIe

[Konrad Dybcio]

On 16.08.2023 09:05, Pavan Kondeti wrote:
> On Tue, Aug 15, 2023 at 05:56:47PM +0530, Krishna chaitanya chundru wrote:
>> PCIe needs to choose the appropriate performance state of RPMH power
>> domain based upon the PCIe gen speed.
>>
>> So, let's add the OPP table support to specify RPMH performance states.
>>
>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>> ---
>>  arch/arm64/boot/dts/qcom/sm8450.dtsi | 47 ++++++++++++++++++++++++++++++++++++
>>  1 file changed, 47 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
>> index 595533a..681ea9c 100644
>> --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
>> @@ -381,6 +381,49 @@
>>  		};
>>  	};
>>  
>> +	pcie0_opp_table: opp-table-pcie0 {
>> +		compatible = "operating-points-v2";
>> +
>> +		opp-2500000 {
>> +			opp-hz = /bits/ 64 <2500000>;
>> +			opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
>> +		};
>> +
>> +		opp-5000000 {
>> +			opp-hz = /bits/ 64 <5000000>;
>> +			opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
>> +		};
>> +
>> +		opp-8000000 {
>> +			opp-hz = /bits/ 64 <8000000>;
>> +			opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
>> +		};
>> +	};
>> +
>> +	pcie1_opp_table: opp-table-pcie1 {
>> +		compatible = "operating-points-v2";
>> +
>> +		opp-2500000 {
>> +			opp-hz = /bits/ 64 <2500000>;
>> +			opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
>> +		};
>> +
>> +		opp-5000000 {
>> +			opp-hz = /bits/ 64 <5000000>;
>> +			opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
>> +		};
>> +
>> +		opp-8000000 {
>> +			opp-hz = /bits/ 64 <8000000>;
>> +			opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
>> +		};
>> +
>> +		opp-16000000 {
>> +			opp-hz = /bits/ 64 <16000000>;
>> +			opp-level = <RPMH_REGULATOR_LEVEL_NOM>;
>> +		};
>> +	};
>> +
> 
> Should not we using required-opps property to pass the
> rpmhpd_opp_xxx phandle so that when this OPP is selected based on your
> clock rate, the appropriate OPP (voltage) would be selected on the RPMH side?
Yes, opp-level is for opp providers.

Konrad