| Message ID | 1692102408-7010-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 |
Hi Krishna, kernel test robot noticed the following build warnings: [auto build test WARNING on pci/for-linus] [also build test WARNING on robh/for-next linus/master v6.5-rc6 next-20230809] [cannot apply to pci/next] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Krishna-chaitanya-chundru/dt-bindings-pci-qcom-Add-binding-for-operating-points-v2/20230815-203103 base: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git for-linus patch link: https://lore.kernel.org/r/1692102408-7010-4-git-send-email-quic_krichai%40quicinc.com patch subject: [PATCH v1 3/3] PCI: qcom: Add OPP suuport for speed based performance state of RPMH config: loongarch-allyesconfig (https://download.01.org/0day-ci/archive/20230815/202308152125.sU1aQfAd-lkp@intel.com/config) compiler: loongarch64-linux-gcc (GCC) 12.3.0 reproduce: (https://download.01.org/0day-ci/archive/20230815/202308152125.sU1aQfAd-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202308152125.sU1aQfAd-lkp@intel.com/ All warnings (new ones prefixed by >>): drivers/pci/controller/dwc/pcie-qcom.c: In function 'qcom_pcie_opp_update': >> drivers/pci/controller/dwc/pcie-qcom.c:1368:13: warning: variable 'ret' set but not used [-Wunused-but-set-variable] 1368 | int ret = 0; | ^~~ vim +/ret +1368 drivers/pci/controller/dwc/pcie-qcom.c 1360 1361 static void qcom_pcie_opp_update(struct qcom_pcie *pcie) 1362 { 1363 struct dw_pcie *pci = pcie->pci; 1364 struct dev_pm_opp *opp; 1365 u32 offset, status; 1366 uint32_t freq; 1367 int speed; > 1368 int ret = 0; 1369 1370 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP); 1371 status = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA); 1372 1373 /* Only update constraints if link is up. */ 1374 if (!(status & PCI_EXP_LNKSTA_DLLLA)) 1375 return; 1376 1377 speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, status); 1378 1379 switch (speed) { 1380 case 1: 1381 freq = 2500000; 1382 break; 1383 case 2: 1384 freq = 5000000; 1385 break; 1386 case 3: 1387 freq = 8000000; 1388 break; 1389 default: 1390 WARN_ON_ONCE(1); 1391 fallthrough; 1392 case 4: 1393 freq = 16000000; 1394 break; 1395 } 1396 1397 opp = dev_pm_opp_find_freq_exact(pci->dev, freq, true); 1398 1399 if (!IS_ERR(opp)) { 1400 ret = dev_pm_opp_get_voltage(opp); 1401 dev_pm_opp_put(opp); 1402 } 1403
On Tue, Aug 15, 2023 at 05:56:48PM +0530, Krishna chaitanya chundru wrote: > Before link training vote for the maximum performance state of RPMH > and once the 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 | 61 ++++++++++++++++++++++++++++++++++ > 1 file changed, 61 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > index 7a87a47..e29a986 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,51 @@ 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; > + uint32_t freq; > + int speed; > + int 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); > + > + switch (speed) { > + case 1: > + freq = 2500000; > + break; > + case 2: > + freq = 5000000; > + break; > + case 3: > + freq = 8000000; > + break; > + default: > + WARN_ON_ONCE(1); > + fallthrough; > + case 4: > + freq = 16000000; > + break; > + } > + > + opp = dev_pm_opp_find_freq_exact(pci->dev, freq, true); > + > + if (!IS_ERR(opp)) { > + ret = dev_pm_opp_get_voltage(opp); > + dev_pm_opp_put(opp); > + } > + Where are we setting the OPP here? > +} > + > static void qcom_pcie_icc_update(struct qcom_pcie *pcie) > { > struct dw_pcie *pci = pcie->pci; > @@ -1439,8 +1485,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 +1559,17 @@ 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; > + } > + > + opp = dev_pm_opp_find_freq_floor(dev, &max_freq); > + if (!IS_ERR(opp)) > + dev_pm_opp_put(opp); > + This OPP (corresponding to max freq) is not used, so how are we voting for max perf state during probe? > ret = pcie->cfg->ops->get_resources(pcie); > if (ret) > goto err_pm_runtime_put; > @@ -1531,6 +1590,8 @@ static int qcom_pcie_probe(struct platform_device *pdev) > > qcom_pcie_icc_update(pcie); > > + qcom_pcie_opp_update(pcie); > + commit description says, OPP voting is done as per the link speed after probe? I don't see any calls to qcom_pcie_opp_update() outside probe. > if (pcie->mhi) > qcom_pcie_init_debugfs(pcie); > > Thanks, Pavan
On 8/16/2023 11:54 AM, Pavan Kondeti wrote: > On Tue, Aug 15, 2023 at 05:56:48PM +0530, Krishna chaitanya chundru wrote: >> Before link training vote for the maximum performance state of RPMH >> and once the 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 | 61 ++++++++++++++++++++++++++++++++++ >> 1 file changed, 61 insertions(+) >> >> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c >> index 7a87a47..e29a986 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,51 @@ 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; >> + uint32_t freq; >> + int speed; >> + int 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); >> + >> + switch (speed) { >> + case 1: >> + freq = 2500000; >> + break; >> + case 2: >> + freq = 5000000; >> + break; >> + case 3: >> + freq = 8000000; >> + break; >> + default: >> + WARN_ON_ONCE(1); >> + fallthrough; >> + case 4: >> + freq = 16000000; >> + break; >> + } >> + >> + opp = dev_pm_opp_find_freq_exact(pci->dev, freq, true); >> + >> + if (!IS_ERR(opp)) { >> + ret = dev_pm_opp_get_voltage(opp); >> + dev_pm_opp_put(opp); >> + } >> + > Where are we setting the OPP here? > >> +} >> + >> static void qcom_pcie_icc_update(struct qcom_pcie *pcie) >> { >> struct dw_pcie *pci = pcie->pci; >> @@ -1439,8 +1485,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 +1559,17 @@ 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; >> + } >> + >> + opp = dev_pm_opp_find_freq_floor(dev, &max_freq); >> + if (!IS_ERR(opp)) >> + dev_pm_opp_put(opp); >> + > This OPP (corresponding to max freq) is not used, so how are we voting > for max perf state during probe? > >> ret = pcie->cfg->ops->get_resources(pcie); >> if (ret) >> goto err_pm_runtime_put; >> @@ -1531,6 +1590,8 @@ static int qcom_pcie_probe(struct platform_device *pdev) >> >> qcom_pcie_icc_update(pcie); >> >> + qcom_pcie_opp_update(pcie); >> + > commit description says, OPP voting is done as per the link speed after > probe? I don't see any calls to qcom_pcie_opp_update() outside probe. my mistake dev_pm_opp_set_opp somehow missed here I will update in next patch. - KC >> if (pcie->mhi) >> qcom_pcie_init_debugfs(pcie); >> >> > Thanks, > Pavan
On 15.08.2023 14:26, Krishna chaitanya chundru wrote: > Before link training vote for the maximum performance state of RPMH > and once the 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 | 61 ++++++++++++++++++++++++++++++++++ > 1 file changed, 61 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > index 7a87a47..e29a986 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,51 @@ 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; > + uint32_t freq; > + int speed; > + int ret = 0; On top of Krzysztof's comments: ret is effectively unused [...] > + opp = dev_pm_opp_find_freq_exact(pci->dev, freq, true); > + > + if (!IS_ERR(opp)) { Unnecessary newline Konrad
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c index 7a87a47..e29a986 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,51 @@ 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; + uint32_t freq; + int speed; + int 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); + + switch (speed) { + case 1: + freq = 2500000; + break; + case 2: + freq = 5000000; + break; + case 3: + freq = 8000000; + break; + default: + WARN_ON_ONCE(1); + fallthrough; + case 4: + freq = 16000000; + break; + } + + opp = dev_pm_opp_find_freq_exact(pci->dev, freq, true); + + if (!IS_ERR(opp)) { + ret = dev_pm_opp_get_voltage(opp); + dev_pm_opp_put(opp); + } + +} + static void qcom_pcie_icc_update(struct qcom_pcie *pcie) { struct dw_pcie *pci = pcie->pci; @@ -1439,8 +1485,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 +1559,17 @@ 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; + } + + opp = dev_pm_opp_find_freq_floor(dev, &max_freq); + if (!IS_ERR(opp)) + dev_pm_opp_put(opp); + ret = pcie->cfg->ops->get_resources(pcie); if (ret) goto err_pm_runtime_put; @@ -1531,6 +1590,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 the 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 | 61 ++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+)