[PATCH 0/3] opp: Add bw_factor support to adjust bandwidth dynamically

[PATCH 0/3] opp: Add bw_factor support to adjust bandwidth dynamically

[Krishna Chaitanya Chundru]

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>

[PATCH 1/3] opp: Add bw_factor support to adjust bandwidth dynamically

[Krishna Chaitanya Chundru]

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>
---
 drivers/opp/core.c     | 37 +++++++++++++++++++++++++++++++++++--
 drivers/opp/opp.h      |  2 ++
 include/linux/pm_opp.h |  7 +++++++
 3 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index edbd60501cf00dfd1957f7d19b228d1c61bbbdcc..bd618fd1a36fa9c252408beb35ac2e39bfb17ee5 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1060,8 +1060,8 @@ static int _set_opp_bw(const struct opp_table *opp_table,
 			avg = 0;
 			peak = 0;
 		} else {
-			avg = opp->bandwidth[i].avg;
-			peak = opp->bandwidth[i].peak;
+			avg = opp->bandwidth[i].avg * opp_table->bw_factor;
+			peak = opp->bandwidth[i].peak * opp_table->bw_factor;
 		}
 		ret = icc_set_bw(opp_table->paths[i], avg, peak);
 		if (ret) {
@@ -1461,6 +1461,7 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
 			 __func__, ret);
 	}
 
+	opp_table->bw_factor = 1;
 	BLOCKING_INIT_NOTIFIER_HEAD(&opp_table->head);
 	INIT_LIST_HEAD(&opp_table->opp_list);
 	kref_init(&opp_table->kref);
@@ -2815,6 +2816,38 @@ static int _opp_set_availability(struct device *dev, unsigned long freq,
 	return 0;
 }
 
+/**
+ * dev_pm_opp_set_bw_factor() - helper to change the bw factor
+ * @dev:		device for which we do this operation
+ * @bw_factor:		bw factor which multiples the supplied bw
+ *
+ * Return: -EINVAL for bad pointers, -ENOMEM if no memory available for the
+ * copy operation, returns 0 if no modifcation was done OR modification was
+ * successful.
+ */
+int dev_pm_opp_set_bw_factor(struct device *dev, u8 bw_factor)
+{
+	struct opp_table *opp_table __free(put_opp_table);
+	int r;
+
+	/* Find the opp_table */
+	opp_table = _find_opp_table(dev);
+	if (IS_ERR(opp_table)) {
+		r = PTR_ERR(opp_table);
+		dev_warn(dev, "%s: Device OPP not found (%d)\n", __func__, r);
+		return r;
+	}
+
+	if (opp_table->bw_factor == bw_factor)
+		return 0;
+
+	scoped_guard(mutex, &opp_table->lock)
+		opp_table->bw_factor = bw_factor;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_set_bw_factor);
+
 /**
  * dev_pm_opp_adjust_voltage() - helper to change the voltage of an OPP
  * @dev:		device for which we do this operation
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 9eba63e01a9e7650cf2e49515b70ba73f72210fc..f52d8582b705f1dcf8b5c8279716d38acb273a6c 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -192,6 +192,7 @@ enum opp_table_access {
  * property).
  * @paths: Interconnect path handles
  * @path_count: Number of interconnect paths
+ * @bw_factor: Multiplier to the supplied bw
  * @enabled: Set to true if the device's resources are enabled/configured.
  * @is_genpd: Marks if the OPP table belongs to a genpd.
  * @dentry:	debugfs dentry pointer of the real device directory (not links).
@@ -240,6 +241,7 @@ struct opp_table {
 	int regulator_count;
 	struct icc_path **paths;
 	unsigned int path_count;
+	u8 bw_factor;
 	bool enabled;
 	bool is_genpd;
 
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index cf477beae4bbede88223566df5f43d85adc5a816..4b090fd7391975ab3fa9a94e939325de946cadfa 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -170,6 +170,8 @@ int dev_pm_opp_add_dynamic(struct device *dev, struct dev_pm_opp_data *opp);
 void dev_pm_opp_remove(struct device *dev, unsigned long freq);
 void dev_pm_opp_remove_all_dynamic(struct device *dev);
 
+int dev_pm_opp_set_bw_factor(struct device *dev, u8 bw_factor);
+
 int dev_pm_opp_adjust_voltage(struct device *dev, unsigned long freq,
 			      unsigned long u_volt, unsigned long u_volt_min,
 			      unsigned long u_volt_max);
@@ -371,6 +373,11 @@ static inline void dev_pm_opp_remove_all_dynamic(struct device *dev)
 {
 }
 
+static inline int dev_pm_opp_set_bw_factor(struct device *dev, u8 bw_factor)
+{
+	return 0;
+}
+
 static inline int
 dev_pm_opp_adjust_voltage(struct device *dev, unsigned long freq,
 			  unsigned long u_volt, unsigned long u_volt_min,

-- 
2.34.1

[PATCH 2/3] PCI: qcom: Use bw_factor to adjust bandwidth based on link width

[Krishna Chaitanya Chundru]

Data rates 8GT/s x2 and Data rates 16GT/s x1 have same frequency so using
same OPP entry in the OPP table.  QCOM controllers may have different RPMh
votes for different rates. So we can't use shared entries in the OPP.

Use only data rate freqiency and remove width in it and use bw_factor
to multiply bandwidth based up on the link width through OPP.

Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index c789e3f856550bcfa1ce09962ba9c086d117de05..fde9fd3fff6bdcec0c9618d3f4b003a3d823307f 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1505,13 +1505,17 @@ static void qcom_pcie_icc_opp_update(struct qcom_pcie *pcie)
 			return;
 
 		freq_kbps = freq_mbps * KILO;
-		opp = dev_pm_opp_find_freq_exact(pci->dev, freq_kbps * width,
+		opp = dev_pm_opp_find_freq_exact(pci->dev, freq_kbps,
 						 true);
 		if (!IS_ERR(opp)) {
+			ret = dev_pm_opp_set_bw_factor(pci->dev, width);
+			if (ret)
+				dev_err(pci->dev, "Failed to set OPP scale: %d\n", ret);
+
 			ret = dev_pm_opp_set_opp(pci->dev, opp);
 			if (ret)
 				dev_err(pci->dev, "Failed to set OPP for freq (%lu): %d\n",
-					freq_kbps * width, ret);
+					freq_kbps, ret);
 			dev_pm_opp_put(opp);
 		}
 	}

-- 
2.34.1

[PATCH 3/3] arm64: dts: qcom: sm8450: Keep only x1 lane PCIe OPP entries

[Krishna Chaitanya Chundru]

Currently the PCIe OPP table is included entries for multiple lanes
configurations like x2. Since the driver now uses bw_factor to adjust
bandwidth based on link width, we only need to define OPPs for x1
lane configurations.

Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
 arch/arm64/boot/dts/qcom/sm8450.dtsi | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
index 54c6d0fdb2afa51084c510eddc341d6087189611..d752dc2b17f03284fada7584b5fbdf7672e06142 100644
--- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
@@ -2216,20 +2216,13 @@ 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>;
-				};
-
 				/* GEN 3 x1 */
 				opp-8000000 {
 					opp-hz = /bits/ 64 <8000000>;
@@ -2237,19 +2230,13 @@ 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>;
-				};
 			};
 
 			pcie@0 {

-- 
2.34.1

Re: [PATCH 0/3] opp: Add bw_factor support to adjust bandwidth dynamically

[Krishna Chaitanya Chundru]

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,

Re: [PATCH 0/3] opp: Add bw_factor support to adjust bandwidth dynamically

[Viresh Kumar]

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

Re: [PATCH 0/3] opp: Add bw_factor support to adjust bandwidth dynamically

[Krishna Chaitanya Chundru]

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.
> 

Re: [PATCH 0/3] opp: Add bw_factor support to adjust bandwidth dynamically

[Viresh Kumar]

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

Re: [PATCH 0/3] opp: Add bw_factor support to adjust bandwidth dynamically

[Krishna Chaitanya Chundru]

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.
> 

Re: [PATCH 0/3] opp: Add bw_factor support to adjust bandwidth dynamically

[Viresh Kumar]

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

Re: [PATCH 0/3] opp: Add bw_factor support to adjust bandwidth dynamically

[Krishna Chaitanya Chundru]

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.
> 

Re: [PATCH 0/3] opp: Add bw_factor support to adjust bandwidth dynamically

[Viresh Kumar]

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

Re: [PATCH 0/3] opp: Add bw_factor support to adjust bandwidth dynamically

[Krishna Chaitanya Chundru]

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.
> 

Re: [PATCH 0/3] opp: Add bw_factor support to adjust bandwidth dynamically

[Viresh Kumar]

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

Re: [PATCH 0/3] opp: Add bw_factor support to adjust bandwidth dynamically

[Krishna Chaitanya Chundru]

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.
>