| Message ID | 1692717141-32743-5-git-send-email-quic_krichai@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: qcom: Add support for OPP | expand |
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 >
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 --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; }
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(+)