diff mbox series

[v4,4/4] PCI: qcom: Add OPP support for speed based performance state of RPMH

Message ID 1692717141-32743-5-git-send-email-quic_krichai@quicinc.com (mailing list archive)
State Superseded
Delegated to: Krzysztof Wilczyński
Headers show
Series PCI: qcom: Add support for OPP | expand

Commit Message

Krishna Chaitanya Chundru Aug. 22, 2023, 3:12 p.m. UTC 
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 | 52 ++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

Comments

Manivannan Sadhasivam Aug. 23, 2023, 7:06 a.m. UTC | #1 
Subject should be, "PCI: qcom: Add OPP support to scale performance state of
power domain"

On Tue, Aug 22, 2023 at 08:42:21PM +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.
> 

Commit message should have the justification on why OPP support should be
addded, not just how you add it. The reasoning should be, "While scaling the
interconnect clocks based on PCIe link speed, it is also mandatory to scale the
power domain performance state so that the SoC can run under optimum power
conditions."

> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 52 ++++++++++++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 7a87a47..161fdad 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,33 @@ 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);
> +

Since icc_update() also queries link status register, this could be moved inside
icc_update() to avoid code duplication and probably the function con be renamed
to "qcom_pcie_icc_opp_update()".

> +	opp = dev_pm_opp_find_level_exact(pci->dev, speed);
> +	if (!IS_ERR(opp)) {

As we decided for pcie-qcom-ep, let's return error from _update() function if
icc paths/opp support were specified in DT.

Use a separate patch for returning error from existing qcom_pcie_icc_update()
function and add opp support on top.

> +		ret = dev_pm_opp_set_opp(pci->dev, opp);
> +		if (ret)
> +			dev_err(pci->dev, "Failed to set opp: level %d ret %d\n",
> +						dev_pm_opp_get_level(opp), ret);
> +		dev_pm_opp_put(opp);
> +	}
> +
> +}
> +
>  static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
>  {
>  	struct dw_pcie *pci = pcie->pci;
> @@ -1439,8 +1467,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_level = 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 +1541,23 @@ 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");

"Failed to add OPP table"

Also, use dev_err_probe() here and below.

> +		goto err_pm_runtime_put;
> +	}
> +
> +	/* vote for max level in the opp table */
> +	opp = dev_pm_opp_find_level_floor(dev, &max_level);

Use a bool flag to check whether opp support is present or not and use that to
decide calling these APIs.

> +	if (!IS_ERR(opp)) {
> +		ret = dev_pm_opp_set_opp(dev, opp);
> +		if (ret)
> +			dev_err(pci->dev, "Failed to set opp: level %d ret %d\n",
> +						dev_pm_opp_get_level(opp), ret);
> +		dev_pm_opp_put(opp);
> +	}
> +
>  	ret = pcie->cfg->ops->get_resources(pcie);
>  	if (ret)
>  		goto err_pm_runtime_put;
> @@ -1531,6 +1578,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);
>  
> @@ -1577,6 +1626,7 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
>  	 */
>  	if (!dw_pcie_link_up(pcie->pci)) {
>  		qcom_pcie_host_deinit(&pcie->pci->pp);
> +		dev_pm_opp_set_opp(dev, NULL);

This will print error when OPP table was not specified in DT. So use the flag as
I suggested above.

- Mani

>  		pcie->suspended = true;
>  	}
>  
> @@ -1593,6 +1643,8 @@ static int qcom_pcie_resume_noirq(struct device *dev)
>  		if (ret)
>  			return ret;
>  
> +		qcom_pcie_opp_update(pcie);
> +
>  		pcie->suspended = false;
>  	}
>  
> -- 
> 2.7.4
>
Krishna Chaitanya Chundru Sept. 7, 2023, 6:05 a.m. UTC | #2 
On 8/23/2023 12:36 PM, Manivannan Sadhasivam wrote:
> Subject should be, "PCI: qcom: Add OPP support to scale performance state of
> power domain"
>
> On Tue, Aug 22, 2023 at 08:42:21PM +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.
>>
> Commit message should have the justification on why OPP support should be
> addded, not just how you add it. The reasoning should be, "While scaling the
> interconnect clocks based on PCIe link speed, it is also mandatory to scale the
> power domain performance state so that the SoC can run under optimum power
> conditions."
done
>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>> ---
>>   drivers/pci/controller/dwc/pcie-qcom.c | 52 ++++++++++++++++++++++++++++++++++
>>   1 file changed, 52 insertions(+)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>> index 7a87a47..161fdad 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,33 @@ 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);
>> +
> Since icc_update() also queries link status register, this could be moved inside
> icc_update() to avoid code duplication and probably the function con be renamed
> to "qcom_pcie_icc_opp_update()".
done
>> +	opp = dev_pm_opp_find_level_exact(pci->dev, speed);
>> +	if (!IS_ERR(opp)) {
> As we decided for pcie-qcom-ep, let's return error from _update() function if
> icc paths/opp support were specified in DT.
>
> Use a separate patch for returning error from existing qcom_pcie_icc_update()
> function and add opp support on top.
done
>> +		ret = dev_pm_opp_set_opp(pci->dev, opp);
>> +		if (ret)
>> +			dev_err(pci->dev, "Failed to set opp: level %d ret %d\n",
>> +						dev_pm_opp_get_level(opp), ret);
>> +		dev_pm_opp_put(opp);
>> +	}
>> +
>> +}
>> +
>>   static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
>>   {
>>   	struct dw_pcie *pci = pcie->pci;
>> @@ -1439,8 +1467,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_level = 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 +1541,23 @@ 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");
> "Failed to add OPP table"
>
> Also, use dev_err_probe() here and below.
>
>> +		goto err_pm_runtime_put;
>> +	}
>> +
>> +	/* vote for max level in the opp table */
>> +	opp = dev_pm_opp_find_level_floor(dev, &max_level);
> Use a bool flag to check whether opp support is present or not and use that to
> decide calling these APIs.
done
>> +	if (!IS_ERR(opp)) {
>> +		ret = dev_pm_opp_set_opp(dev, opp);
>> +		if (ret)
>> +			dev_err(pci->dev, "Failed to set opp: level %d ret %d\n",
>> +						dev_pm_opp_get_level(opp), ret);
>> +		dev_pm_opp_put(opp);
>> +	}
>> +
>>   	ret = pcie->cfg->ops->get_resources(pcie);
>>   	if (ret)
>>   		goto err_pm_runtime_put;
>> @@ -1531,6 +1578,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);
>>   
>> @@ -1577,6 +1626,7 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
>>   	 */
>>   	if (!dw_pcie_link_up(pcie->pci)) {
>>   		qcom_pcie_host_deinit(&pcie->pci->pp);
>> +		dev_pm_opp_set_opp(dev, NULL);
> This will print error when OPP table was not specified in DT. So use the flag as
> I suggested above.
>
> - Mani

done

- KC

>>   		pcie->suspended = true;
>>   	}
>>   
>> @@ -1593,6 +1643,8 @@ static int qcom_pcie_resume_noirq(struct device *dev)
>>   		if (ret)
>>   			return ret;
>>   
>> +		qcom_pcie_opp_update(pcie);
>> +
>>   		pcie->suspended = false;
>>   	}
>>   
>> -- 
>> 2.7.4
>>
diff mbox series

Patch

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 7a87a47..161fdad 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,33 @@  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: level %d ret %d\n",
+						dev_pm_opp_get_level(opp), ret);
+		dev_pm_opp_put(opp);
+	}
+
+}
+
 static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
 {
 	struct dw_pcie *pci = pcie->pci;
@@ -1439,8 +1467,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_level = 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 +1541,23 @@  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_level_floor(dev, &max_level);
+	if (!IS_ERR(opp)) {
+		ret = dev_pm_opp_set_opp(dev, opp);
+		if (ret)
+			dev_err(pci->dev, "Failed to set opp: level %d ret %d\n",
+						dev_pm_opp_get_level(opp), ret);
+		dev_pm_opp_put(opp);
+	}
+
 	ret = pcie->cfg->ops->get_resources(pcie);
 	if (ret)
 		goto err_pm_runtime_put;
@@ -1531,6 +1578,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);
 
@@ -1577,6 +1626,7 @@  static int qcom_pcie_suspend_noirq(struct device *dev)
 	 */
 	if (!dw_pcie_link_up(pcie->pci)) {
 		qcom_pcie_host_deinit(&pcie->pci->pp);
+		dev_pm_opp_set_opp(dev, NULL);
 		pcie->suspended = true;
 	}
 
@@ -1593,6 +1643,8 @@  static int qcom_pcie_resume_noirq(struct device *dev)
 		if (ret)
 			return ret;
 
+		qcom_pcie_opp_update(pcie);
+
 		pcie->suspended = false;
 	}