diff mbox series

[4/8] PCI: dwc: qcom: Update ICC & OPP votes based upon the requested speed

Message ID 20250217-mhi_bw_up-v1-4-9bad1e42bdb1@oss.qualcomm.com (mailing list archive)
State New
Delegated to: Jeff Johnson
Headers show
Series bus: mhi: host: Add support for mhi bus bw | expand

Commit Message

Krishna Chaitanya Chundru Feb. 17, 2025, 6:34 a.m. UTC 
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(-)

Comments

kernel test robot Feb. 18, 2025, 12:36 a.m. UTC | #1 
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
Bjorn Helgaas Feb. 18, 2025, 10:07 p.m. UTC | #2 
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?

> 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.
Krishna Chaitanya Chundru Feb. 19, 2025, 5:58 p.m. UTC | #3 
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.
Bjorn Helgaas Feb. 19, 2025, 6:27 p.m. UTC | #4 
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
diff mbox series

Patch

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)