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

[Manivannan Sadhasivam]

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@xxxxxxxxxxx>
> ---
>  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
> 

-- 
மணிவண்ணன் சதாசிவம்