| Message ID | 1692102408-7010-3-git-send-email-quic_krichai@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: qcom: Add support for OPP | expand |
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
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
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 >
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
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
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"; };
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(+)