[PATCH v3 0/3] PCI: qcom: Add support for OPP

[PATCH v3 0/3] PCI: qcom: Add support for OPP

[Krishna chaitanya chundru]

This patch adds support for OPP to vote for the performance state of RPMH
power domain based upon GEN speed it PCIe got enumerated.

Before link up PCIe driver will vote for the maximum performance state.

changes from v3:
	- Instead of using the freq based opp search use level based as suggested
	  by Dmitry Baryshkov.
Changes from v1:
        - Addressed comments from Krzysztof Kozlowski.
        - Added the rpmhpd_opp_xxx phandle as suggested by pavan.
        - Added dev_pm_opp_set_opp API call which was missed on previous patch.

Krishna chaitanya chundru (3):
  dt-bindings: pci: qcom: Add binding for operating-points-v2
  arm64: dts: qcom: sm8450: Add opp table support to PCIe
  PCI: qcom: Add OPP support for speed based performance state of RPMH

 .../devicetree/bindings/pci/qcom,pcie.yaml         |  4 ++
 arch/arm64/boot/dts/qcom/sm8450.dtsi               | 47 ++++++++++++++++++++++
 drivers/pci/controller/dwc/pcie-qcom.c             | 47 ++++++++++++++++++++++
 3 files changed, 98 insertions(+)

-- 
2.7.4

[PATCH v3 1/3] dt-bindings: pci: qcom: Add opp table

[Krishna chaitanya chundru]

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

Adding the Operating Performance Points table allows to adjust power domain
performance state, depending on the PCIe gen speed.

Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 Documentation/devicetree/bindings/pci/qcom,pcie.yaml | 4 ++++
 1 file changed, 4 insertions(+)

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

[PATCH v3 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 up on the PCIe gen speed.

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

Use opp-level for the PCIe gen speed for easier use.

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..3af0cf9 100644
--- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
@@ -1803,7 +1803,28 @@
 			pinctrl-names = "default";
 			pinctrl-0 = <&pcie0_default_state>;
 
+			operating-points-v2 = <&pcie0_opp_table>;
+
 			status = "disabled";
+
+			pcie0_opp_table: opp-table {
+				compatible = "operating-points-v2";
+
+				opp-1 {
+					opp-level = <1>;
+					required-opps = <&rpmhpd_opp_low_svs>;
+				};
+
+				opp-2 {
+					opp-level = <2>;
+					required-opps = <&rpmhpd_opp_low_svs>;
+				};
+
+				opp-3 {
+					opp-level = <3>;
+					required-opps = <&rpmhpd_opp_nom>;
+				};
+			};
 		};
 
 		pcie0_phy: phy@1c06000 {
@@ -1915,7 +1936,33 @@
 			pinctrl-names = "default";
 			pinctrl-0 = <&pcie1_default_state>;
 
+			operating-points-v2 = <&pcie1_opp_table>;
+
 			status = "disabled";
+
+			pcie1_opp_table: opp-table {
+				compatible = "operating-points-v2";
+
+				opp-1 {
+					opp-level = <1>;
+					required-opps = <&rpmhpd_opp_low_svs>;
+				};
+
+				opp-2 {
+					opp-level = <2>;
+					required-opps = <&rpmhpd_opp_low_svs>;
+				};
+
+				opp-3 {
+					opp-level = <3>;
+					required-opps = <&rpmhpd_opp_low_svs>;
+				};
+
+				opp-4 {
+					opp-level = <4>;
+					required-opps = <&rpmhpd_opp_nom>;
+				};
+			};
 		};
 
 		pcie1_phy: phy@1c0f000 {
-- 
2.7.4

[PATCH v3 3/3] PCI: qcom: Add OPP support for speed based performance state of RPMH

[Krishna chaitanya chundru]

Before link training vote for the maximum performance state of RPMH
and once link is up, vote for the performance state based upon the link
speed.

Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 47 ++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 7a87a47..c57ca1a 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -22,6 +22,7 @@
 #include <linux/of_device.h>
 #include <linux/of_gpio.h>
 #include <linux/pci.h>
+#include <linux/pm_opp.h>
 #include <linux/pm_runtime.h>
 #include <linux/platform_device.h>
 #include <linux/phy/pcie.h>
@@ -1357,6 +1358,32 @@ static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
 	return 0;
 }
 
+static void qcom_pcie_opp_update(struct qcom_pcie *pcie)
+{
+	struct dw_pcie *pci = pcie->pci;
+	struct dev_pm_opp *opp;
+	u32 offset, status;
+	int speed, ret = 0;
+
+	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
+	status = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
+
+	/* Only update constraints if link is up. */
+	if (!(status & PCI_EXP_LNKSTA_DLLLA))
+		return;
+
+	speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, status);
+
+	opp = dev_pm_opp_find_level_exact(pci->dev, speed);
+	if (!IS_ERR(opp)) {
+		ret = dev_pm_opp_set_opp(pci->dev, opp);
+		if (ret)
+			dev_err(pci->dev, "Failed to set opp: %d\n", ret);
+		dev_pm_opp_put(opp);
+	}
+
+}
+
 static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
 {
 	struct dw_pcie *pci = pcie->pci;
@@ -1439,8 +1466,10 @@ static void qcom_pcie_init_debugfs(struct qcom_pcie *pcie)
 static int qcom_pcie_probe(struct platform_device *pdev)
 {
 	const struct qcom_pcie_cfg *pcie_cfg;
+	unsigned long max_freq = INT_MAX;
 	struct device *dev = &pdev->dev;
 	struct qcom_pcie *pcie;
+	struct dev_pm_opp *opp;
 	struct dw_pcie_rp *pp;
 	struct resource *res;
 	struct dw_pcie *pci;
@@ -1511,6 +1540,22 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_pm_runtime_put;
 
+	/* OPP table is optional */
+	ret = devm_pm_opp_of_add_table(dev);
+	if (ret && ret != -ENODEV) {
+		dev_err(dev, "Invalid OPP table in Device tree\n");
+		goto err_pm_runtime_put;
+	}
+
+	/* vote for max level in the opp table */
+	opp = dev_pm_opp_find_freq_floor(dev, &max_freq);
+	if (!IS_ERR(opp)) {
+		ret = dev_pm_opp_set_opp(dev, opp);
+		if (ret)
+			dev_err(pci->dev, "Failed to set opp: %d\n", ret);
+		dev_pm_opp_put(opp);
+	}
+
 	ret = pcie->cfg->ops->get_resources(pcie);
 	if (ret)
 		goto err_pm_runtime_put;
@@ -1531,6 +1576,8 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 
 	qcom_pcie_icc_update(pcie);
 
+	qcom_pcie_opp_update(pcie);
+
 	if (pcie->mhi)
 		qcom_pcie_init_debugfs(pcie);
 
-- 
2.7.4

Re: [PATCH v3 3/3] PCI: qcom: Add OPP support for speed based performance state of RPMH

[Pavan Kondeti]

On Mon, Aug 21, 2023 at 07:45:43PM +0530, Krishna chaitanya chundru wrote:
> Before link training vote for the maximum performance state of RPMH
> and once link is up, vote for the performance state based upon the link
> speed.
> 
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 47 ++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 7a87a47..c57ca1a 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -22,6 +22,7 @@
>  #include <linux/of_device.h>
>  #include <linux/of_gpio.h>
>  #include <linux/pci.h>
> +#include <linux/pm_opp.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/platform_device.h>
>  #include <linux/phy/pcie.h>
> @@ -1357,6 +1358,32 @@ static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
>  	return 0;
>  }
>  
> +static void qcom_pcie_opp_update(struct qcom_pcie *pcie)
> +{
> +	struct dw_pcie *pci = pcie->pci;
> +	struct dev_pm_opp *opp;
> +	u32 offset, status;
> +	int speed, ret = 0;
> +
> +	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +	status = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
> +
> +	/* Only update constraints if link is up. */
> +	if (!(status & PCI_EXP_LNKSTA_DLLLA))
> +		return;
> +

What happens if link is not up during probe? We set max vote before
this, should not we bring it down in suspend and vote it back again in
resume? 

> +	speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, status);
> +
> +	opp = dev_pm_opp_find_level_exact(pci->dev, speed);
> +	if (!IS_ERR(opp)) {
> +		ret = dev_pm_opp_set_opp(pci->dev, opp);
> +		if (ret)
> +			dev_err(pci->dev, "Failed to set opp: %d\n", ret);
> +		dev_pm_opp_put(opp);
> +	}

Since you added an error message, make it more useful by printing the
opp level also. dev_pm_opp_get_level().

> +
> +}
> +
>  static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
>  {
>  	struct dw_pcie *pci = pcie->pci;
> @@ -1439,8 +1466,10 @@ static void qcom_pcie_init_debugfs(struct qcom_pcie *pcie)
>  static int qcom_pcie_probe(struct platform_device *pdev)
>  {
>  	const struct qcom_pcie_cfg *pcie_cfg;
> +	unsigned long max_freq = INT_MAX;
>  	struct device *dev = &pdev->dev;
>  	struct qcom_pcie *pcie;
> +	struct dev_pm_opp *opp;
>  	struct dw_pcie_rp *pp;
>  	struct resource *res;
>  	struct dw_pcie *pci;
> @@ -1511,6 +1540,22 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_pm_runtime_put;
>  
> +	/* OPP table is optional */
> +	ret = devm_pm_opp_of_add_table(dev);
> +	if (ret && ret != -ENODEV) {
> +		dev_err(dev, "Invalid OPP table in Device tree\n");
> +		goto err_pm_runtime_put;
> +	}
> +
> +	/* vote for max level in the opp table */
> +	opp = dev_pm_opp_find_freq_floor(dev, &max_freq);
> +	if (!IS_ERR(opp)) {
> +		ret = dev_pm_opp_set_opp(dev, opp);
> +		if (ret)
> +			dev_err(pci->dev, "Failed to set opp: %d\n", ret);
> +		dev_pm_opp_put(opp);
> +	}
> +

This needs an update since you moved from frequency based voting to link
speed based voting.

>  	ret = pcie->cfg->ops->get_resources(pcie);
>  	if (ret)
>  		goto err_pm_runtime_put;
> @@ -1531,6 +1576,8 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  
>  	qcom_pcie_icc_update(pcie);
>  
> +	qcom_pcie_opp_update(pcie);
> +
>  	if (pcie->mhi)
>  		qcom_pcie_init_debugfs(pcie);
>  
> 

Thanks,
Pavan

Re: [PATCH v3 3/3] PCI: qcom: Add OPP support for speed based performance state of RPMH

[Krishna Chaitanya Chundru]

On 8/22/2023 9:33 AM, Pavan Kondeti wrote:
> On Mon, Aug 21, 2023 at 07:45:43PM +0530, Krishna chaitanya chundru wrote:
>> Before link training vote for the maximum performance state of RPMH
>> and once link is up, vote for the performance state based upon the link
>> speed.
>>
>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>> ---
>>   drivers/pci/controller/dwc/pcie-qcom.c | 47 ++++++++++++++++++++++++++++++++++
>>   1 file changed, 47 insertions(+)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>> index 7a87a47..c57ca1a 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -22,6 +22,7 @@
>>   #include <linux/of_device.h>
>>   #include <linux/of_gpio.h>
>>   #include <linux/pci.h>
>> +#include <linux/pm_opp.h>
>>   #include <linux/pm_runtime.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/phy/pcie.h>
>> @@ -1357,6 +1358,32 @@ static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
>>   	return 0;
>>   }
>>   
>> +static void qcom_pcie_opp_update(struct qcom_pcie *pcie)
>> +{
>> +	struct dw_pcie *pci = pcie->pci;
>> +	struct dev_pm_opp *opp;
>> +	u32 offset, status;
>> +	int speed, ret = 0;
>> +
>> +	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
>> +	status = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
>> +
>> +	/* Only update constraints if link is up. */
>> +	if (!(status & PCI_EXP_LNKSTA_DLLLA))
>> +		return;
>> +
> What happens if link is not up during probe? We set max vote before
> this, should not we bring it down in suspend and vote it back again in
> resume?

ok, I will set to lower value in the suspend path if the link is not 
up.  If the link is already up driver will not

do any modifications.

>
>> +	speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, status);
>> +
>> +	opp = dev_pm_opp_find_level_exact(pci->dev, speed);
>> +	if (!IS_ERR(opp)) {
>> +		ret = dev_pm_opp_set_opp(pci->dev, opp);
>> +		if (ret)
>> +			dev_err(pci->dev, "Failed to set opp: %d\n", ret);
>> +		dev_pm_opp_put(opp);
>> +	}
> Since you added an error message, make it more useful by printing the
> opp level also. dev_pm_opp_get_level().
Sure I will add this in next patch.
>
>> +
>> +}
>> +
>>   static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
>>   {
>>   	struct dw_pcie *pci = pcie->pci;
>> @@ -1439,8 +1466,10 @@ static void qcom_pcie_init_debugfs(struct qcom_pcie *pcie)
>>   static int qcom_pcie_probe(struct platform_device *pdev)
>>   {
>>   	const struct qcom_pcie_cfg *pcie_cfg;
>> +	unsigned long max_freq = INT_MAX;
>>   	struct device *dev = &pdev->dev;
>>   	struct qcom_pcie *pcie;
>> +	struct dev_pm_opp *opp;
>>   	struct dw_pcie_rp *pp;
>>   	struct resource *res;
>>   	struct dw_pcie *pci;
>> @@ -1511,6 +1540,22 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>>   	if (ret)
>>   		goto err_pm_runtime_put;
>>   
>> +	/* OPP table is optional */
>> +	ret = devm_pm_opp_of_add_table(dev);
>> +	if (ret && ret != -ENODEV) {
>> +		dev_err(dev, "Invalid OPP table in Device tree\n");
>> +		goto err_pm_runtime_put;
>> +	}
>> +
>> +	/* vote for max level in the opp table */
>> +	opp = dev_pm_opp_find_freq_floor(dev, &max_freq);
>> +	if (!IS_ERR(opp)) {
>> +		ret = dev_pm_opp_set_opp(dev, opp);
>> +		if (ret)
>> +			dev_err(pci->dev, "Failed to set opp: %d\n", ret);
>> +		dev_pm_opp_put(opp);
>> +	}
>> +
> This needs an update since you moved from frequency based voting to link
> speed based voting.

dev_pm_opp_find_freq_floor will give us the max the opp level opp we 
don't have a similar API to get max opp-level

For that reason we are using this API.

- KC

>
>>   	ret = pcie->cfg->ops->get_resources(pcie);
>>   	if (ret)
>>   		goto err_pm_runtime_put;
>> @@ -1531,6 +1576,8 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>>   
>>   	qcom_pcie_icc_update(pcie);
>>   
>> +	qcom_pcie_opp_update(pcie);
>> +
>>   	if (pcie->mhi)
>>   		qcom_pcie_init_debugfs(pcie);
>>   
>>
> Thanks,
> Pavan

Re: [PATCH v3 3/3] PCI: qcom: Add OPP support for speed based performance state of RPMH

[Pavan Kondeti]

+linux-pm and OPP maintainers

On Tue, Aug 22, 2023 at 09:57:41AM +0530, Krishna Chaitanya Chundru wrote:
> 
> On 8/22/2023 9:33 AM, Pavan Kondeti wrote:
> > On Mon, Aug 21, 2023 at 07:45:43PM +0530, Krishna chaitanya chundru wrote:
> > > Before link training vote for the maximum performance state of RPMH
> > > and once link is up, vote for the performance state based upon the link
> > > speed.
> > > 
> > > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> > > ---
> > >   drivers/pci/controller/dwc/pcie-qcom.c | 47 ++++++++++++++++++++++++++++++++++
> > >   1 file changed, 47 insertions(+)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > index 7a87a47..c57ca1a 100644
> > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > @@ -22,6 +22,7 @@
> > >   #include <linux/of_device.h>
> > >   #include <linux/of_gpio.h>
> > >   #include <linux/pci.h>
> > > +#include <linux/pm_opp.h>
> > >   #include <linux/pm_runtime.h>
> > >   #include <linux/platform_device.h>
> > >   #include <linux/phy/pcie.h>
> > > @@ -1357,6 +1358,32 @@ static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
> > >   	return 0;
> > >   }
> > > +static void qcom_pcie_opp_update(struct qcom_pcie *pcie)
> > > +{
> > > +	struct dw_pcie *pci = pcie->pci;
> > > +	struct dev_pm_opp *opp;
> > > +	u32 offset, status;
> > > +	int speed, ret = 0;
> > > +
> > > +	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> > > +	status = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
> > > +
> > > +	/* Only update constraints if link is up. */
> > > +	if (!(status & PCI_EXP_LNKSTA_DLLLA))
> > > +		return;
> > > +
> > What happens if link is not up during probe? We set max vote before
> > this, should not we bring it down in suspend and vote it back again in
> > resume?
> 
> ok, I will set to lower value in the suspend path if the link is not up.  If
> the link is already up driver will not
> 
> do any modifications.
> 
> > 
> > > +	speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, status);
> > > +
> > > +	opp = dev_pm_opp_find_level_exact(pci->dev, speed);
> > > +	if (!IS_ERR(opp)) {
> > > +		ret = dev_pm_opp_set_opp(pci->dev, opp);
> > > +		if (ret)
> > > +			dev_err(pci->dev, "Failed to set opp: %d\n", ret);
> > > +		dev_pm_opp_put(opp);
> > > +	}
> > Since you added an error message, make it more useful by printing the
> > opp level also. dev_pm_opp_get_level().
> Sure I will add this in next patch.
> > 
> > > +
> > > +}
> > > +
> > >   static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
> > >   {
> > >   	struct dw_pcie *pci = pcie->pci;
> > > @@ -1439,8 +1466,10 @@ static void qcom_pcie_init_debugfs(struct qcom_pcie *pcie)
> > >   static int qcom_pcie_probe(struct platform_device *pdev)
> > >   {
> > >   	const struct qcom_pcie_cfg *pcie_cfg;
> > > +	unsigned long max_freq = INT_MAX;
> > >   	struct device *dev = &pdev->dev;
> > >   	struct qcom_pcie *pcie;
> > > +	struct dev_pm_opp *opp;
> > >   	struct dw_pcie_rp *pp;
> > >   	struct resource *res;
> > >   	struct dw_pcie *pci;
> > > @@ -1511,6 +1540,22 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> > >   	if (ret)
> > >   		goto err_pm_runtime_put;
> > > +	/* OPP table is optional */
> > > +	ret = devm_pm_opp_of_add_table(dev);
> > > +	if (ret && ret != -ENODEV) {
> > > +		dev_err(dev, "Invalid OPP table in Device tree\n");
> > > +		goto err_pm_runtime_put;
> > > +	}
> > > +
> > > +	/* vote for max level in the opp table */
> > > +	opp = dev_pm_opp_find_freq_floor(dev, &max_freq);
> > > +	if (!IS_ERR(opp)) {
> > > +		ret = dev_pm_opp_set_opp(dev, opp);
> > > +		if (ret)
> > > +			dev_err(pci->dev, "Failed to set opp: %d\n", ret);
> > > +		dev_pm_opp_put(opp);
> > > +	}
> > > +
> > This needs an update since you moved from frequency based voting to link
> > speed based voting.
> 
> dev_pm_opp_find_freq_floor will give us the max the opp level opp we don't
> have a similar API to get max opp-level
> 
> For that reason we are using this API.
> 

Ok, thanks. I get that it is working. Would you be not knowing the exact
level for the max speed supported? if that is unknown, I believe we have
a use case for dev_pm_opp_find_level_floor() API. Adding the best people
on this matter for thei valuable opinion/suggestions.

Thanks,
Pavan

Re: [PATCH v3 3/3] PCI: qcom: Add OPP support for speed based performance state of RPMH

[Viresh Kumar]

On 22-08-23, 10:23, Pavan Kondeti wrote:
> +linux-pm and OPP maintainers
> 
> On Tue, Aug 22, 2023 at 09:57:41AM +0530, Krishna Chaitanya Chundru wrote:
> > 
> > On 8/22/2023 9:33 AM, Pavan Kondeti wrote:
> > > On Mon, Aug 21, 2023 at 07:45:43PM +0530, Krishna chaitanya chundru wrote:
> > > > Before link training vote for the maximum performance state of RPMH
> > > > and once link is up, vote for the performance state based upon the link
> > > > speed.
> > > > 
> > > > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> > > > ---
> > > >   drivers/pci/controller/dwc/pcie-qcom.c | 47 ++++++++++++++++++++++++++++++++++
> > > >   1 file changed, 47 insertions(+)
> > > > 
> > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > index 7a87a47..c57ca1a 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > @@ -22,6 +22,7 @@
> > > >   #include <linux/of_device.h>
> > > >   #include <linux/of_gpio.h>
> > > >   #include <linux/pci.h>
> > > > +#include <linux/pm_opp.h>
> > > >   #include <linux/pm_runtime.h>
> > > >   #include <linux/platform_device.h>
> > > >   #include <linux/phy/pcie.h>
> > > > @@ -1357,6 +1358,32 @@ static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
> > > >   	return 0;
> > > >   }
> > > > +static void qcom_pcie_opp_update(struct qcom_pcie *pcie)
> > > > +{
> > > > +	struct dw_pcie *pci = pcie->pci;
> > > > +	struct dev_pm_opp *opp;
> > > > +	u32 offset, status;
> > > > +	int speed, ret = 0;
> > > > +
> > > > +	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> > > > +	status = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
> > > > +
> > > > +	/* Only update constraints if link is up. */
> > > > +	if (!(status & PCI_EXP_LNKSTA_DLLLA))
> > > > +		return;
> > > > +
> > > What happens if link is not up during probe? We set max vote before
> > > this, should not we bring it down in suspend and vote it back again in
> > > resume?
> > 
> > ok, I will set to lower value in the suspend path if the link is not up.  If
> > the link is already up driver will not
> > 
> > do any modifications.
> > 
> > > 
> > > > +	speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, status);
> > > > +
> > > > +	opp = dev_pm_opp_find_level_exact(pci->dev, speed);
> > > > +	if (!IS_ERR(opp)) {
> > > > +		ret = dev_pm_opp_set_opp(pci->dev, opp);
> > > > +		if (ret)
> > > > +			dev_err(pci->dev, "Failed to set opp: %d\n", ret);
> > > > +		dev_pm_opp_put(opp);
> > > > +	}
> > > Since you added an error message, make it more useful by printing the
> > > opp level also. dev_pm_opp_get_level().
> > Sure I will add this in next patch.
> > > 
> > > > +
> > > > +}
> > > > +
> > > >   static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
> > > >   {
> > > >   	struct dw_pcie *pci = pcie->pci;
> > > > @@ -1439,8 +1466,10 @@ static void qcom_pcie_init_debugfs(struct qcom_pcie *pcie)
> > > >   static int qcom_pcie_probe(struct platform_device *pdev)
> > > >   {
> > > >   	const struct qcom_pcie_cfg *pcie_cfg;
> > > > +	unsigned long max_freq = INT_MAX;
> > > >   	struct device *dev = &pdev->dev;
> > > >   	struct qcom_pcie *pcie;
> > > > +	struct dev_pm_opp *opp;
> > > >   	struct dw_pcie_rp *pp;
> > > >   	struct resource *res;
> > > >   	struct dw_pcie *pci;
> > > > @@ -1511,6 +1540,22 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> > > >   	if (ret)
> > > >   		goto err_pm_runtime_put;
> > > > +	/* OPP table is optional */
> > > > +	ret = devm_pm_opp_of_add_table(dev);
> > > > +	if (ret && ret != -ENODEV) {
> > > > +		dev_err(dev, "Invalid OPP table in Device tree\n");
> > > > +		goto err_pm_runtime_put;
> > > > +	}
> > > > +
> > > > +	/* vote for max level in the opp table */
> > > > +	opp = dev_pm_opp_find_freq_floor(dev, &max_freq);
> > > > +	if (!IS_ERR(opp)) {
> > > > +		ret = dev_pm_opp_set_opp(dev, opp);
> > > > +		if (ret)
> > > > +			dev_err(pci->dev, "Failed to set opp: %d\n", ret);
> > > > +		dev_pm_opp_put(opp);
> > > > +	}
> > > > +
> > > This needs an update since you moved from frequency based voting to link
> > > speed based voting.
> > 
> > dev_pm_opp_find_freq_floor will give us the max the opp level opp we don't
> > have a similar API to get max opp-level
> > 
> > For that reason we are using this API.
> > 
> 
> Ok, thanks. I get that it is working. Would you be not knowing the exact
> level for the max speed supported? if that is unknown, I believe we have
> a use case for dev_pm_opp_find_level_floor() API. Adding the best people
> on this matter for thei valuable opinion/suggestions.

If required feel free to add dev_pm_opp_find_level_floor(), based on
dev_pm_opp_find_level_ceil().

-- 
viresh