| Message ID | 1692627343-4380-4-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 |
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
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
+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
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().
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);
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(+)