arch/arm64/boot/dts/qcom/sm8450.dtsi | 17 ++-------------- drivers/opp/core.c | 37 ++++++++++++++++++++++++++++++++-- drivers/opp/opp.h | 2 ++ drivers/pci/controller/dwc/pcie-qcom.c | 8 ++++++-- include/linux/pm_opp.h | 7 +++++++ 5 files changed, 52 insertions(+), 19 deletions(-)
The existing OPP table in the device tree for PCIe is shared across
different link configurations such as data rates 8GT/s x2 and 16GT/s x1.
These configurations often operate at the same frequency, allowing them
to reuse the same OPP entries. However, 8GT/s and 16 GT/s may have
different characteristics beyond frequency—such as RPMh votes in QCOM
case, which cannot be represented accurately when sharing a single OPP.
To avoid conflicts and duplication in the device tree, we now define only
one set of OPP entries per table and introduce a new mechanism to adjust
bandwidth dynamically using a `bw_factor`.
The `bw_factor` is a multiplier applied to the average and peak bandwidth
values of an OPP entry. This allows PCIe drivers to modify the effective
bandwidth at runtime based on the actual link width without needing
separate OPP entries for each configuration.
Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
Krishna Chaitanya Chundru (3):
opp: Add bw_factor support to adjust bandwidth dynamically
PCI: qcom: Use bw_factor to adjust bandwidth based on link width
arm64: dts: qcom: sm8450: Keep only x1 lane PCIe OPP entries
arch/arm64/boot/dts/qcom/sm8450.dtsi | 17 ++--------------
drivers/opp/core.c | 37 ++++++++++++++++++++++++++++++++--
drivers/opp/opp.h | 2 ++
drivers/pci/controller/dwc/pcie-qcom.c | 8 ++++++--
include/linux/pm_opp.h | 7 +++++++
5 files changed, 52 insertions(+), 19 deletions(-)
---
base-commit: e2291551827fe5d2d3758c435c191d32b6d1350e
change-id: 20250717-opp_pcie-793160b2b113
Best regards,
--
Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
Hi Viresh, Can you please review this once. Thanks & Regards, Krishna Chaitanya. On 7/17/2025 7:31 PM, Krishna Chaitanya Chundru wrote: > The existing OPP table in the device tree for PCIe is shared across > different link configurations such as data rates 8GT/s x2 and 16GT/s x1. > These configurations often operate at the same frequency, allowing them > to reuse the same OPP entries. However, 8GT/s and 16 GT/s may have > different characteristics beyond frequency—such as RPMh votes in QCOM > case, which cannot be represented accurately when sharing a single OPP. > > To avoid conflicts and duplication in the device tree, we now define only > one set of OPP entries per table and introduce a new mechanism to adjust > bandwidth dynamically using a `bw_factor`. > > The `bw_factor` is a multiplier applied to the average and peak bandwidth > values of an OPP entry. This allows PCIe drivers to modify the effective > bandwidth at runtime based on the actual link width without needing > separate OPP entries for each configuration. > > Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com> > --- > Krishna Chaitanya Chundru (3): > opp: Add bw_factor support to adjust bandwidth dynamically > PCI: qcom: Use bw_factor to adjust bandwidth based on link width > arm64: dts: qcom: sm8450: Keep only x1 lane PCIe OPP entries > > arch/arm64/boot/dts/qcom/sm8450.dtsi | 17 ++-------------- > drivers/opp/core.c | 37 ++++++++++++++++++++++++++++++++-- > drivers/opp/opp.h | 2 ++ > drivers/pci/controller/dwc/pcie-qcom.c | 8 ++++++-- > include/linux/pm_opp.h | 7 +++++++ > 5 files changed, 52 insertions(+), 19 deletions(-) > --- > base-commit: e2291551827fe5d2d3758c435c191d32b6d1350e > change-id: 20250717-opp_pcie-793160b2b113 > > Best regards,
On 01-08-25, 12:05, Krishna Chaitanya Chundru wrote:
> Can you please review this once.
Sorry about the delay.
> > The existing OPP table in the device tree for PCIe is shared across
> > different link configurations such as data rates 8GT/s x2 and 16GT/s x1.
> > These configurations often operate at the same frequency, allowing them
> > to reuse the same OPP entries. However, 8GT/s and 16 GT/s may have
> > different characteristics beyond frequency—such as RPMh votes in QCOM
> > case, which cannot be represented accurately when sharing a single OPP.
From the looks of it, something like this should also work:
diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
index 54c6d0fdb2af..0a76bc4c4dc9 100644
--- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
@@ -2216,18 +2216,12 @@ opp-2500000 {
opp-peak-kBps = <250000 1>;
};
- /* GEN 1 x2 and GEN 2 x1 */
+ /* GEN 2 x1 */
opp-5000000 {
opp-hz = /bits/ 64 <5000000>;
required-opps = <&rpmhpd_opp_low_svs>;
- opp-peak-kBps = <500000 1>;
- };
-
- /* GEN 2 x2 */
- opp-10000000 {
- opp-hz = /bits/ 64 <10000000>;
- required-opps = <&rpmhpd_opp_low_svs>;
- opp-peak-kBps = <1000000 1>;
+ opp-peak-kBps-x1 = <500000 1>;
+ opp-peak-kBps-x2 = <1000000 1>;
};
/* GEN 3 x1 */
@@ -2237,18 +2231,12 @@ opp-8000000 {
opp-peak-kBps = <984500 1>;
};
- /* GEN 3 x2 and GEN 4 x1 */
+ /* GEN 4 x1 */
opp-16000000 {
opp-hz = /bits/ 64 <16000000>;
required-opps = <&rpmhpd_opp_nom>;
- opp-peak-kBps = <1969000 1>;
- };
-
- /* GEN 4 x2 */
- opp-32000000 {
- opp-hz = /bits/ 64 <32000000>;
- required-opps = <&rpmhpd_opp_nom>;
- opp-peak-kBps = <3938000 1>;
+ opp-peak-kBps-x1 = <1969000 1>;
+ opp-peak-kBps-x2 = <3938000 1>;
};
};
The OPP core supports named properties, which will make this work.
--
viresh
On 8/1/2025 12:58 PM, Viresh Kumar wrote:
> On 01-08-25, 12:05, Krishna Chaitanya Chundru wrote:
>> Can you please review this once.
>
> Sorry about the delay.
>
>>> The existing OPP table in the device tree for PCIe is shared across
>>> different link configurations such as data rates 8GT/s x2 and 16GT/s x1.
>>> These configurations often operate at the same frequency, allowing them
>>> to reuse the same OPP entries. However, 8GT/s and 16 GT/s may have
>>> different characteristics beyond frequency—such as RPMh votes in QCOM
>>> case, which cannot be represented accurately when sharing a single OPP.
>
> From the looks of it, something like this should also work:
>
> diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> index 54c6d0fdb2af..0a76bc4c4dc9 100644
> --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> @@ -2216,18 +2216,12 @@ opp-2500000 {
> opp-peak-kBps = <250000 1>;
> };
>
> - /* GEN 1 x2 and GEN 2 x1 */
> + /* GEN 2 x1 */
> opp-5000000 {
> opp-hz = /bits/ 64 <5000000>;
> required-opps = <&rpmhpd_opp_low_svs>;
> - opp-peak-kBps = <500000 1>;
> - };
> -
> - /* GEN 2 x2 */
> - opp-10000000 {
> - opp-hz = /bits/ 64 <10000000>;
> - required-opps = <&rpmhpd_opp_low_svs>;
> - opp-peak-kBps = <1000000 1>;
> + opp-peak-kBps-x1 = <500000 1>;
> + opp-peak-kBps-x2 = <1000000 1>;
> };
>
> /* GEN 3 x1 */
> @@ -2237,18 +2231,12 @@ opp-8000000 {
> opp-peak-kBps = <984500 1>;
> };
>
> - /* GEN 3 x2 and GEN 4 x1 */
> + /* GEN 4 x1 */
> opp-16000000 {
> opp-hz = /bits/ 64 <16000000>;
> required-opps = <&rpmhpd_opp_nom>;
> - opp-peak-kBps = <1969000 1>;
> - };
> -
> - /* GEN 4 x2 */
> - opp-32000000 {
> - opp-hz = /bits/ 64 <32000000>;
> - required-opps = <&rpmhpd_opp_nom>;
> - opp-peak-kBps = <3938000 1>;
> + opp-peak-kBps-x1 = <1969000 1>;
> + opp-peak-kBps-x2 = <3938000 1>;
> };
> };
>
> The OPP core supports named properties, which will make this work.
Hi Viresh,
When ever PCIe link speed/width changes we need to update the OPP votes,
If we use named properties approach we might not be able to change it
dynamically without removing the OPP table first. For that reason only
we haven't used that approach.
Correct us if our understanding is wrong.
- Krishna Chaitanya.
>
On 01-08-25, 13:58, Krishna Chaitanya Chundru wrote: > When ever PCIe link speed/width changes we need to update the OPP votes, > If we use named properties approach we might not be able to change it > dynamically without removing the OPP table first. For that reason only > we haven't used that approach. I am not sure I understand it fully. I thought this was a one time configuration you were required to do at boot time based on platform's configuration. If you need to change the performance at runtime, won't you switch to a different OPP ? I don't have much knowledge of how PCIe works, maybe that's why the confusion. -- viresh
On 8/1/2025 2:26 PM, Viresh Kumar wrote: > On 01-08-25, 13:58, Krishna Chaitanya Chundru wrote: >> When ever PCIe link speed/width changes we need to update the OPP votes, >> If we use named properties approach we might not be able to change it >> dynamically without removing the OPP table first. For that reason only >> we haven't used that approach. > > I am not sure I understand it fully. I thought this was a one time configuration > you were required to do at boot time based on platform's configuration. > I am not fully familiar with OPP here. > If you need to change the performance at runtime, won't you switch to a > different OPP ? > yes we do set different OPP when we change it. Currently we are fetching the OPP based on the frequency and setting that OPP using dev_pm_opp_set_opp(). As you are suggesting to use dev_pm_opp_set_prop_name() here. This what I understood First set prop name using dev_pm_opp_set_prop_name then set opp dev_pm_opp_set_opp() if you want to change above one we need to first clear using dev_pm_opp_put_prop_name() then again call dev_pm_opp_set_prop_name & dev_pm_opp_set_opp() I was in a impression that once you call dev_pm_opp_put_prop_name() the previous votes will be removed. we don't want that to happen. if this is not correct we can use this approach. If this is not correct assumption can you point me any reference to this I was not able to find any reference on this. - Krishna Chaitanya. > I don't have much knowledge of how PCIe works, maybe that's why the confusion. >
On 01-08-25, 15:05, Krishna Chaitanya Chundru wrote: > Currently we are fetching the OPP based on the frequency and setting > that OPP using dev_pm_opp_set_opp(). > > As you are suggesting to use dev_pm_opp_set_prop_name() here. > This what I understood > > First set prop name using dev_pm_opp_set_prop_name then > set opp dev_pm_opp_set_opp() > > if you want to change above one we need to first clear using > dev_pm_opp_put_prop_name() then again call dev_pm_opp_set_prop_name > & dev_pm_opp_set_opp() dev_pm_opp_set_prop_name() should be called only once at boot time and not again later on. It is there to configure one of the named properties before the OPP table initializes for a device. Basically it is there to select one of the available properties for an OPP, like selecting a voltage applicable for an OPP for a device. -- viresh
On 8/4/2025 4:43 PM, Viresh Kumar wrote: > On 01-08-25, 15:05, Krishna Chaitanya Chundru wrote: >> Currently we are fetching the OPP based on the frequency and setting >> that OPP using dev_pm_opp_set_opp(). >> >> As you are suggesting to use dev_pm_opp_set_prop_name() here. >> This what I understood >> >> First set prop name using dev_pm_opp_set_prop_name then >> set opp dev_pm_opp_set_opp() >> >> if you want to change above one we need to first clear using >> dev_pm_opp_put_prop_name() then again call dev_pm_opp_set_prop_name >> & dev_pm_opp_set_opp() > > dev_pm_opp_set_prop_name() should be called only once at boot time and not > again later on. It is there to configure one of the named properties before the > OPP table initializes for a device. Basically it is there to select one of the > available properties for an OPP, like selecting a voltage applicable for an OPP > for a device. Then we can't use this dev_pm_opp_set_prop_name(), there is possibility link width x1, x2, x4 etc can also change at runtime. - Krishna Chaitanya. >
Sorry for the delay, I was travelling a bit recently. On 06-08-25, 10:35, Krishna Chaitanya Chundru wrote: > On 8/4/2025 4:43 PM, Viresh Kumar wrote: > > On 01-08-25, 15:05, Krishna Chaitanya Chundru wrote: > > > Currently we are fetching the OPP based on the frequency and setting > > > that OPP using dev_pm_opp_set_opp(). > > > > > > As you are suggesting to use dev_pm_opp_set_prop_name() here. > > > This what I understood > > > > > > First set prop name using dev_pm_opp_set_prop_name then > > > set opp dev_pm_opp_set_opp() > > > > > > if you want to change above one we need to first clear using > > > dev_pm_opp_put_prop_name() then again call dev_pm_opp_set_prop_name > > > & dev_pm_opp_set_opp() > > > > dev_pm_opp_set_prop_name() should be called only once at boot time and not > > again later on. It is there to configure one of the named properties before the > > OPP table initializes for a device. Basically it is there to select one of the > > available properties for an OPP, like selecting a voltage applicable for an OPP > > for a device. > > Then we can't use this dev_pm_opp_set_prop_name(), there is possibility > link width x1, x2, x4 etc can also change at runtime. Hmm, looking at the way you have implemented the bw multiplier, you are going to call that every time you need to change the OPP configuration. That doesn't look nice TBH. Such configurations are normally provided via DT or are configured once at boot and not touched after that. What you are basically doing is something like, adding a single OPP in DT and changing the OPP frequency right before setting it at runtime. FWIW, you are allowed to add multiple OPPs with same frequency value but different bandwidths or levels. I think you should use that and correctly describe the hardware first (which is the step in the right direction). And then you can find the right OPP at runtime and send a request to configure it. That way we can avoid adding hacks in the OPP core. -- viresh
On 8/11/2025 2:14 PM, Viresh Kumar wrote:
> Sorry for the delay, I was travelling a bit recently.
>
> On 06-08-25, 10:35, Krishna Chaitanya Chundru wrote:
>> On 8/4/2025 4:43 PM, Viresh Kumar wrote:
>>> On 01-08-25, 15:05, Krishna Chaitanya Chundru wrote:
>>>> Currently we are fetching the OPP based on the frequency and setting
>>>> that OPP using dev_pm_opp_set_opp().
>>>>
>>>> As you are suggesting to use dev_pm_opp_set_prop_name() here.
>>>> This what I understood
>>>>
>>>> First set prop name using dev_pm_opp_set_prop_name then
>>>> set opp dev_pm_opp_set_opp()
>>>>
>>>> if you want to change above one we need to first clear using
>>>> dev_pm_opp_put_prop_name() then again call dev_pm_opp_set_prop_name
>>>> & dev_pm_opp_set_opp()
>>>
>>> dev_pm_opp_set_prop_name() should be called only once at boot time and not
>>> again later on. It is there to configure one of the named properties before the
>>> OPP table initializes for a device. Basically it is there to select one of the
>>> available properties for an OPP, like selecting a voltage applicable for an OPP
>>> for a device.
>>
>> Then we can't use this dev_pm_opp_set_prop_name(), there is possibility
>> link width x1, x2, x4 etc can also change at runtime.
>
> Hmm, looking at the way you have implemented the bw multiplier, you
> are going to call that every time you need to change the OPP
> configuration. That doesn't look nice TBH. Such configurations are
> normally provided via DT or are configured once at boot and not
> touched after that. What you are basically doing is something like,
> adding a single OPP in DT and changing the OPP frequency right before
> setting it at runtime.
>
> FWIW, you are allowed to add multiple OPPs with same frequency value
> but different bandwidths or levels. I think you should use that and
> correctly describe the hardware first (which is the step in the right
> direction). And then you can find the right OPP at runtime and send a
> request to configure it. That way we can avoid adding hacks in the OPP
> core.
Thanks Viresh for the suggestion. We will try this.
Can you confirm this is what you are expecting.
dt change
--- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
@@ -2214,13 +2214,23 @@ opp-2500000 {
opp-hz = /bits/ 64 <2500000>;
required-opps =
<&rpmhpd_opp_low_svs>;
opp-peak-kBps = <250000 1>;
+ opp-level = <1>;
};
- /* GEN 1 x2 and GEN 2 x1 */
+ /* GEN 1 x2 */
opp-5000000 {
opp-hz = /bits/ 64 <5000000>;
required-opps =
<&rpmhpd_opp_low_svs>;
opp-peak-kBps = <500000 1>;
+ opp-level = <1>;
+ };
+
+ /* GEN 2 x1 */
+ opp-5000000 {
+ opp-hz = /bits/ 64 <5000000>;
+ required-opps =
<&rpmhpd_opp_low_svs>;
+ opp-peak-kBps = <500000 1>;
+ opp-level = <2>;
};
/* GEN 2 x2 */
@@ -2228,6 +2238,7 @@ opp-10000000 {
opp-hz = /bits/ 64 <10000000>;
required-opps =
<&rpmhpd_opp_low_svs>;
opp-peak-kBps = <1000000 1>;
+ opp-level = <2>;
};
/* GEN 3 x1 */
@@ -2235,13 +2246,23 @@ opp-8000000 {
opp-hz = /bits/ 64 <8000000>;
required-opps = <&rpmhpd_opp_nom>;
opp-peak-kBps = <984500 1>;
+ opp-level = <3>;
+ };
+
+ /* GEN 3 x2 */
+ opp-16000000 {
+ opp-hz = /bits/ 64 <16000000>;
+ required-opps = <&rpmhpd_opp_nom>;
+ opp-peak-kBps = <1969000 1>;
+ opp-level = <3>;
};
- /* GEN 3 x2 and GEN 4 x1 */
+ /*GEN 4 x1 */
opp-16000000 {
opp-hz = /bits/ 64 <16000000>;
required-opps = <&rpmhpd_opp_nom>;
opp-peak-kBps = <1969000 1>;
+ opp-level = <4>;
};
/* GEN 4 x2 */
@@ -2249,6 +2270,7 @@ opp-32000000 {
opp-hz = /bits/ 64 <32000000>;
required-opps = <&rpmhpd_opp_nom>;
opp-peak-kBps = <3938000 1>;
+ opp-level = <4>;
};
};
And in the driver I need to have a change in OPP framework which
returns OPP based on both frequency and level something like
dev_pm_opp_find_level_freq_exact(struct device *dev,
unsigned int level, unsigned int freq);
Please correct me if this is not suggested approach.
- Krishna Chaitanya.
>
On 11-08-25, 15:35, Krishna Chaitanya Chundru wrote:
> Thanks Viresh for the suggestion. We will try this.
> Can you confirm this is what you are expecting.
>
> dt change
> --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
> @@ -2214,13 +2214,23 @@ opp-2500000 {
> opp-hz = /bits/ 64 <2500000>;
> required-opps =
> <&rpmhpd_opp_low_svs>;
> opp-peak-kBps = <250000 1>;
> + opp-level = <1>;
> };
>
> - /* GEN 1 x2 and GEN 2 x1 */
> + /* GEN 1 x2 */
> opp-5000000 {
> opp-hz = /bits/ 64 <5000000>;
> required-opps =
> <&rpmhpd_opp_low_svs>;
> opp-peak-kBps = <500000 1>;
> + opp-level = <1>;
> + };
> +
> + /* GEN 2 x1 */
> + opp-5000000 {
The node-name has to be different, but freq can be same. Something
like opp-5000000-N, where N = 1, 2, 3.
> + opp-hz = /bits/ 64 <5000000>;
> + required-opps =
> <&rpmhpd_opp_low_svs>;
> + opp-peak-kBps = <500000 1>;
> + opp-level = <2>;
> };
> And in the driver I need to have a change in OPP framework which
> returns OPP based on both frequency and level something like
> dev_pm_opp_find_level_freq_exact(struct device *dev,
> unsigned int level, unsigned int freq);
I thought you wanted OPP based on freq and bandwidth ? But yeah, a new
OPP API like: dev_pm_opp_find_key_exact(dev, *key);
where,
struct dev_pm_opp_key {
unsigned long *freq;
unsigned int *level;
unsigned int *bw;
}
and match all non-NULL values only to begin with.
--
viresh
On 8/11/2025 3:47 PM, Viresh Kumar wrote:
> On 11-08-25, 15:35, Krishna Chaitanya Chundru wrote:
>> Thanks Viresh for the suggestion. We will try this.
>> Can you confirm this is what you are expecting.
>>
>> dt change
>> --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
>> @@ -2214,13 +2214,23 @@ opp-2500000 {
>> opp-hz = /bits/ 64 <2500000>;
>> required-opps =
>> <&rpmhpd_opp_low_svs>;
>> opp-peak-kBps = <250000 1>;
>> + opp-level = <1>;
>> };
>>
>> - /* GEN 1 x2 and GEN 2 x1 */
>> + /* GEN 1 x2 */
>> opp-5000000 {
>> opp-hz = /bits/ 64 <5000000>;
>> required-opps =
>> <&rpmhpd_opp_low_svs>;
>> opp-peak-kBps = <500000 1>;
>> + opp-level = <1>;
>> + };
>> +
>> + /* GEN 2 x1 */
>> + opp-5000000 {
>
> The node-name has to be different, but freq can be same. Something
> like opp-5000000-N, where N = 1, 2, 3.
>
>> + opp-hz = /bits/ 64 <5000000>;
>> + required-opps =
>> <&rpmhpd_opp_low_svs>;
>> + opp-peak-kBps = <500000 1>;
>> + opp-level = <2>;
>> };
>
>> And in the driver I need to have a change in OPP framework which
>> returns OPP based on both frequency and level something like
>> dev_pm_opp_find_level_freq_exact(struct device *dev,
>> unsigned int level, unsigned int freq);
>
> I thought you wanted OPP based on freq and bandwidth ? But yeah, a new
> OPP API like: dev_pm_opp_find_key_exact(dev, *key);
>
As we have can have same frequency entries we will select the OPP based
on the level. level will differentiate between different PCIe data rates.
Mani,
Can you check this once and confirm any issues from PCIe perspective.
- Krishna Chaitanya.
> where,
>
> struct dev_pm_opp_key {
> unsigned long *freq;
> unsigned int *level;
> unsigned int *bw;
> }
>
> and match all non-NULL values only to begin with.
>
© 2016 - 2025 Red Hat, Inc.