QCOM PCIe controllers needs to disable ASPM before initiating link
re-train. So as part of pre_bw_scale() disable ASPM and as part of
post_scale_bus_bw() enable ASPM back.
Update ICC & OPP votes based on the requested speed so that RPMh votes
gets updated based on the speed.
Bring out the core logic from qcom_pcie_icc_opp_update() to new function
qcom_pcie_set_icc_opp().
Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
drivers/pci/controller/dwc/pcie-qcom.c | 105 +++++++++++++++++++++++++--------
1 file changed, 79 insertions(+), 26 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index e4d3366ead1f..a39d4c5d7992 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1294,10 +1294,88 @@ static void qcom_pcie_host_post_init(struct dw_pcie_rp *pp)
pcie->cfg->ops->host_post_init(pcie);
}
+static int qcom_pcie_set_icc_opp(struct qcom_pcie *pcie, int speed, int width)
+{
+ struct dw_pcie *pci = pcie->pci;
+ unsigned long freq_kbps;
+ struct dev_pm_opp *opp;
+ int ret, freq_mbps;
+
+ if (pcie->icc_mem) {
+ ret = icc_set_bw(pcie->icc_mem, 0,
+ width * QCOM_PCIE_LINK_SPEED_TO_BW(speed));
+ if (ret) {
+ dev_err(pci->dev, "Failed to set bandwidth for PCIe-MEM interconnect path: %d\n",
+ ret);
+ }
+ } else if (pcie->use_pm_opp) {
+ freq_mbps = pcie_dev_speed_mbps(pcie_link_speed[speed]);
+ if (freq_mbps < 0)
+ return -EINVAL;
+
+ freq_kbps = freq_mbps * KILO;
+ opp = dev_pm_opp_find_freq_exact(pci->dev, freq_kbps * width,
+ true);
+ if (!IS_ERR(opp)) {
+ ret = dev_pm_opp_set_opp(pci->dev, opp);
+ if (ret)
+ dev_err(pci->dev, "Failed to set OPP for freq (%lu): %d\n",
+ freq_kbps * width, ret);
+ dev_pm_opp_put(opp);
+ }
+ }
+
+ return ret;
+}
+
+static int qcom_pcie_scale_bw(struct dw_pcie_rp *pp, int speed)
+{
+ struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+ struct qcom_pcie *pcie = to_qcom_pcie(pci);
+ u32 offset, status, width;
+
+ offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
+ status = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
+
+ width = FIELD_GET(PCI_EXP_LNKSTA_NLW, status);
+
+ return qcom_pcie_set_icc_opp(pcie, speed, width);
+}
+
+static int qcom_pcie_enable_disable_aspm(struct pci_dev *pdev, void *userdata)
+{
+ bool *enable = userdata;
+
+ if (*enable)
+ pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);
+ else
+ pci_disable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);
+
+ return 0;
+}
+
+static void qcom_pcie_host_post_scale_bus_bw(struct dw_pcie_rp *pp, int current_speed)
+{
+ bool enable = true;
+
+ pci_walk_bus(pp->bridge->bus, qcom_pcie_enable_disable_aspm, &enable);
+ qcom_pcie_scale_bw(pp, current_speed);
+}
+
+static int qcom_pcie_host_pre_scale_bus_bw(struct dw_pcie_rp *pp, int target_speed)
+{
+ bool enable = false;
+
+ pci_walk_bus(pp->bridge->bus, qcom_pcie_enable_disable_aspm, &enable);
+ return qcom_pcie_scale_bw(pp, target_speed);
+}
+
static const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
.init = qcom_pcie_host_init,
.deinit = qcom_pcie_host_deinit,
.post_init = qcom_pcie_host_post_init,
+ .pre_scale_bus_bw = qcom_pcie_host_pre_scale_bus_bw,
+ .post_scale_bus_bw = qcom_pcie_host_post_scale_bus_bw,
};
/* Qcom IP rev.: 2.1.0 Synopsys IP rev.: 4.01a */
@@ -1478,9 +1556,6 @@ static void qcom_pcie_icc_opp_update(struct qcom_pcie *pcie)
{
u32 offset, status, width, speed;
struct dw_pcie *pci = pcie->pci;
- unsigned long freq_kbps;
- struct dev_pm_opp *opp;
- int ret, freq_mbps;
offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
status = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
@@ -1492,29 +1567,7 @@ static void qcom_pcie_icc_opp_update(struct qcom_pcie *pcie)
speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, status);
width = FIELD_GET(PCI_EXP_LNKSTA_NLW, status);
- if (pcie->icc_mem) {
- ret = icc_set_bw(pcie->icc_mem, 0,
- width * QCOM_PCIE_LINK_SPEED_TO_BW(speed));
- if (ret) {
- dev_err(pci->dev, "Failed to set bandwidth for PCIe-MEM interconnect path: %d\n",
- ret);
- }
- } else if (pcie->use_pm_opp) {
- freq_mbps = pcie_dev_speed_mbps(pcie_link_speed[speed]);
- if (freq_mbps < 0)
- return;
-
- freq_kbps = freq_mbps * KILO;
- opp = dev_pm_opp_find_freq_exact(pci->dev, freq_kbps * width,
- true);
- if (!IS_ERR(opp)) {
- ret = dev_pm_opp_set_opp(pci->dev, opp);
- if (ret)
- dev_err(pci->dev, "Failed to set OPP for freq (%lu): %d\n",
- freq_kbps * width, ret);
- dev_pm_opp_put(opp);
- }
- }
+ qcom_pcie_set_icc_opp(pcie, speed, width);
}
static int qcom_pcie_link_transition_count(struct seq_file *s, void *data)
--
2.34.1Make subject line match history for this file.
On Mon, Feb 17, 2025 at 12:04:11PM +0530, Krishna Chaitanya Chundru wrote:
> QCOM PCIe controllers needs to disable ASPM before initiating link
> re-train. So as part of pre_bw_scale() disable ASPM and as part of
> post_scale_bus_bw() enable ASPM back.
s/needs/need/
Why does Qcom need to disable ASPM? Is there a PCIe spec restriction
about this that should be applied to all PCIe host bridges? Or is
this a Qcom defect?
> Update ICC & OPP votes based on the requested speed so that RPMh votes
> gets updated based on the speed.
s/gets/get/
> Bring out the core logic from qcom_pcie_icc_opp_update() to new function
> qcom_pcie_set_icc_opp().
This refactoring possibly could be a separate patch to make the meat
of this change clearer.
> +static int qcom_pcie_set_icc_opp(struct qcom_pcie *pcie, int speed, int width)
> +{
> + struct dw_pcie *pci = pcie->pci;
> + unsigned long freq_kbps;
> + struct dev_pm_opp *opp;
> + int ret, freq_mbps;
> +
> + if (pcie->icc_mem) {
> + ret = icc_set_bw(pcie->icc_mem, 0,
> + width * QCOM_PCIE_LINK_SPEED_TO_BW(speed));
> + if (ret) {
> + dev_err(pci->dev, "Failed to set bandwidth for PCIe-MEM interconnect path: %d\n",
> + ret);
> + }
> + } else if (pcie->use_pm_opp) {
> + freq_mbps = pcie_dev_speed_mbps(pcie_link_speed[speed]);
> + if (freq_mbps < 0)
> + return -EINVAL;
> +
> + freq_kbps = freq_mbps * KILO;
> + opp = dev_pm_opp_find_freq_exact(pci->dev, freq_kbps * width,
> + true);
> + if (!IS_ERR(opp)) {
> + ret = dev_pm_opp_set_opp(pci->dev, opp);
> + if (ret)
> + dev_err(pci->dev, "Failed to set OPP for freq (%lu): %d\n",
> + freq_kbps * width, ret);
> + dev_pm_opp_put(opp);
> + }
> + }
> +
> + return ret;
Looks uninitialized in some paths.
On 2/19/2025 3:37 AM, Bjorn Helgaas wrote:
> Make subject line match history for this file.
>
> On Mon, Feb 17, 2025 at 12:04:11PM +0530, Krishna Chaitanya Chundru wrote:
>> QCOM PCIe controllers needs to disable ASPM before initiating link
>> re-train. So as part of pre_bw_scale() disable ASPM and as part of
>> post_scale_bus_bw() enable ASPM back.
>
> s/needs/need/
>
> Why does Qcom need to disable ASPM? Is there a PCIe spec restriction
> about this that should be applied to all PCIe host bridges? Or is
> this a Qcom defect?
>
It is QCOM controller issue, PCIe spec doesn't mention to disable ASPM.
>> Update ICC & OPP votes based on the requested speed so that RPMh votes
>> gets updated based on the speed.
>
> s/gets/get/
>
>> Bring out the core logic from qcom_pcie_icc_opp_update() to new function
>> qcom_pcie_set_icc_opp().
>
> This refactoring possibly could be a separate patch to make the meat
> of this change clearer.
>
ack.
- Krishna Chaitanya.
>> +static int qcom_pcie_set_icc_opp(struct qcom_pcie *pcie, int speed, int width)
>> +{
>> + struct dw_pcie *pci = pcie->pci;
>> + unsigned long freq_kbps;
>> + struct dev_pm_opp *opp;
>> + int ret, freq_mbps;
>> +
>> + if (pcie->icc_mem) {
>> + ret = icc_set_bw(pcie->icc_mem, 0,
>> + width * QCOM_PCIE_LINK_SPEED_TO_BW(speed));
>> + if (ret) {
>> + dev_err(pci->dev, "Failed to set bandwidth for PCIe-MEM interconnect path: %d\n",
>> + ret);
>> + }
>> + } else if (pcie->use_pm_opp) {
>> + freq_mbps = pcie_dev_speed_mbps(pcie_link_speed[speed]);
>> + if (freq_mbps < 0)
>> + return -EINVAL;
>> +
>> + freq_kbps = freq_mbps * KILO;
>> + opp = dev_pm_opp_find_freq_exact(pci->dev, freq_kbps * width,
>> + true);
>> + if (!IS_ERR(opp)) {
>> + ret = dev_pm_opp_set_opp(pci->dev, opp);
>> + if (ret)
>> + dev_err(pci->dev, "Failed to set OPP for freq (%lu): %d\n",
>> + freq_kbps * width, ret);
>> + dev_pm_opp_put(opp);
>> + }
>> + }
>> +
>> + return ret;
>
> Looks uninitialized in some paths.
On Wed, Feb 19, 2025 at 11:28:47PM +0530, Krishna Chaitanya Chundru wrote: > On 2/19/2025 3:37 AM, Bjorn Helgaas wrote: > > On Mon, Feb 17, 2025 at 12:04:11PM +0530, Krishna Chaitanya Chundru wrote: > > > QCOM PCIe controllers needs to disable ASPM before initiating link > > > re-train. So as part of pre_bw_scale() disable ASPM and as part of > > > post_scale_bus_bw() enable ASPM back. > > > > s/needs/need/ > > > > Why does Qcom need to disable ASPM? Is there a PCIe spec restriction > > about this that should be applied to all PCIe host bridges? Or is > > this a Qcom defect? > > > It is QCOM controller issue, PCIe spec doesn't mention to disable ASPM. OK. I'm a little concerned that disabling/enabling PCIE_LINK_STATE_ALL might clobber preferences set by drivers of downstream devices, but I guess ... Maybe we could add a comment in the code about this being done to work around some Qcom issue so that browsers of pci_enable_link_state_locked() don't get the idea that this needs to be done generically. Bjorn
Hi Krishna,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 0ad2507d5d93f39619fc42372c347d6006b64319]
url: https://github.com/intel-lab-lkp/linux/commits/Krishna-Chaitanya-Chundru/PCI-update-current-bus-speed-as-part-of-pci_bus_add_devices/20250217-144050
base: 0ad2507d5d93f39619fc42372c347d6006b64319
patch link: https://lore.kernel.org/r/20250217-mhi_bw_up-v1-4-9bad1e42bdb1%40oss.qualcomm.com
patch subject: [PATCH 4/8] PCI: dwc: qcom: Update ICC & OPP votes based upon the requested speed
config: powerpc64-randconfig-002-20250218 (https://download.01.org/0day-ci/archive/20250218/202502180859.1y6qqeIk-lkp@intel.com/config)
compiler: clang version 16.0.6 (https://github.com/llvm/llvm-project 7cbf1a2591520c2491aa35339f227775f4d3adf6)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250218/202502180859.1y6qqeIk-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/202502180859.1y6qqeIk-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/pci/controller/dwc/pcie-qcom.c:1319:7: warning: variable 'ret' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
if (!IS_ERR(opp)) {
^~~~~~~~~~~~
drivers/pci/controller/dwc/pcie-qcom.c:1328:9: note: uninitialized use occurs here
return ret;
^~~
drivers/pci/controller/dwc/pcie-qcom.c:1319:3: note: remove the 'if' if its condition is always true
if (!IS_ERR(opp)) {
^~~~~~~~~~~~~~~~~~
drivers/pci/controller/dwc/pcie-qcom.c:1311:13: warning: variable 'ret' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
} else if (pcie->use_pm_opp) {
^~~~~~~~~~~~~~~~
drivers/pci/controller/dwc/pcie-qcom.c:1328:9: note: uninitialized use occurs here
return ret;
^~~
drivers/pci/controller/dwc/pcie-qcom.c:1311:9: note: remove the 'if' if its condition is always true
} else if (pcie->use_pm_opp) {
^~~~~~~~~~~~~~~~~~~~~~
drivers/pci/controller/dwc/pcie-qcom.c:1302:9: note: initialize the variable 'ret' to silence this warning
int ret, freq_mbps;
^
= 0
2 warnings generated.
vim +1319 drivers/pci/controller/dwc/pcie-qcom.c
1296
1297 static int qcom_pcie_set_icc_opp(struct qcom_pcie *pcie, int speed, int width)
1298 {
1299 struct dw_pcie *pci = pcie->pci;
1300 unsigned long freq_kbps;
1301 struct dev_pm_opp *opp;
1302 int ret, freq_mbps;
1303
1304 if (pcie->icc_mem) {
1305 ret = icc_set_bw(pcie->icc_mem, 0,
1306 width * QCOM_PCIE_LINK_SPEED_TO_BW(speed));
1307 if (ret) {
1308 dev_err(pci->dev, "Failed to set bandwidth for PCIe-MEM interconnect path: %d\n",
1309 ret);
1310 }
1311 } else if (pcie->use_pm_opp) {
1312 freq_mbps = pcie_dev_speed_mbps(pcie_link_speed[speed]);
1313 if (freq_mbps < 0)
1314 return -EINVAL;
1315
1316 freq_kbps = freq_mbps * KILO;
1317 opp = dev_pm_opp_find_freq_exact(pci->dev, freq_kbps * width,
1318 true);
> 1319 if (!IS_ERR(opp)) {
1320 ret = dev_pm_opp_set_opp(pci->dev, opp);
1321 if (ret)
1322 dev_err(pci->dev, "Failed to set OPP for freq (%lu): %d\n",
1323 freq_kbps * width, ret);
1324 dev_pm_opp_put(opp);
1325 }
1326 }
1327
1328 return ret;
1329 }
1330
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
© 2016 - 2025 Red Hat, Inc.