To access PCIe registers, PCIe BAR space, config space the CPU-PCIe
ICC (interconnect consumers) path should be voted otherwise it may
lead to NoC (Network on chip) timeout. We are surviving because of
other driver vote for this path.
As there is less access on this path compared to PCIe to mem path
add minimum vote i.e 1KBps bandwidth always which is recommended
by HW team.
When suspending, disable this path after register space access
is done.
Reviewed-by: Bryan O'Donoghue <[email protected]>
Signed-off-by: Krishna chaitanya chundru <[email protected]>
---
drivers/pci/controller/dwc/pcie-qcom.c | 38 ++++++++++++++++++++++++++++++----
1 file changed, 34 insertions(+), 4 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 14772edcf0d3..b4893214b2d3 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -245,6 +245,7 @@ struct qcom_pcie {
struct phy *phy;
struct gpio_desc *reset;
struct icc_path *icc_mem;
+ struct icc_path *icc_cpu;
const struct qcom_pcie_cfg *cfg;
struct dentry *debugfs;
bool suspended;
@@ -1409,6 +1410,9 @@ static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
if (IS_ERR(pcie->icc_mem))
return PTR_ERR(pcie->icc_mem);
+ pcie->icc_cpu = devm_of_icc_get(pci->dev, "cpu-pcie");
+ if (IS_ERR(pcie->icc_cpu))
+ return PTR_ERR(pcie->icc_cpu);
/*
* Some Qualcomm platforms require interconnect bandwidth constraints
* to be set before enabling interconnect clocks.
@@ -1418,7 +1422,19 @@ static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
*/
ret = icc_set_bw(pcie->icc_mem, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1));
if (ret) {
- dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
+ dev_err(pci->dev, "failed to set interconnect bandwidth for PCIe-MEM: %d\n",
+ ret);
+ return ret;
+ }
+
+ /*
+ * Since the CPU-PCIe path is only used for activities like register
+ * access, Config/BAR space access, HW team has recommended to use a
+ * minimal bandwidth of 1KBps just to keep the link active.
+ */
+ ret = icc_set_bw(pcie->icc_cpu, 0, kBps_to_icc(1));
+ if (ret) {
+ dev_err(pci->dev, "failed to set interconnect bandwidth for CPU-PCIe: %d\n",
ret);
return ret;
}
@@ -1448,7 +1464,7 @@ static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
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 interconnect bandwidth: %d\n",
+ dev_err(pci->dev, "failed to set interconnect bandwidth for PCIe-MEM: %d\n",
ret);
}
}
@@ -1610,7 +1626,7 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
*/
ret = icc_set_bw(pcie->icc_mem, 0, kBps_to_icc(1));
if (ret) {
- dev_err(dev, "Failed to set interconnect bandwidth: %d\n", ret);
+ dev_err(dev, "Failed to set interconnect bandwidth for PCIe-MEM: %d\n", ret);
return ret;
}
@@ -1634,7 +1650,15 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
pcie->suspended = true;
}
- return 0;
+ /*
+ * Remove the vote for CPU-PCIe path now, since at this point onwards,
+ * no register access will be done.
+ */
+ ret = icc_disable(pcie->icc_cpu);
+ if (ret)
+ dev_err(dev, "failed to disable icc path of CPU-PCIe: %d\n", ret);
+
+ return ret;
}
static int qcom_pcie_resume_noirq(struct device *dev)
@@ -1642,6 +1666,12 @@ static int qcom_pcie_resume_noirq(struct device *dev)
struct qcom_pcie *pcie = dev_get_drvdata(dev);
int ret;
+ ret = icc_enable(pcie->icc_cpu);
+ if (ret) {
+ dev_err(dev, "failed to enable icc path of CPU-PCIe: %d\n", ret);
+ return ret;
+ }
+
if (pcie->suspended) {
ret = qcom_pcie_host_init(&pcie->pci->pp);
if (ret)
--
2.42.0
On Sun, Apr 07, 2024 at 10:07:35AM +0530, Krishna chaitanya chundru wrote:
> To access PCIe registers, PCIe BAR space, config space the CPU-PCIe
Please specify whether you are referencing PCIe host controller or endpoint
device or both.
> ICC (interconnect consumers) path should be voted otherwise it may
ICC is just 'Interconnect' unless I misunderstood.
> lead to NoC (Network on chip) timeout. We are surviving because of
> other driver vote for this path.
>
s/vote/voting
> As there is less access on this path compared to PCIe to mem path
> add minimum vote i.e 1KBps bandwidth always which is recommended
> by HW team.
>
'which is sufficient enough to keep the path active.'
> When suspending, disable this path after register space access
> is done.
>
> Reviewed-by: Bryan O'Donoghue <[email protected]>
> Signed-off-by: Krishna chaitanya chundru <[email protected]>
> ---
> drivers/pci/controller/dwc/pcie-qcom.c | 38 ++++++++++++++++++++++++++++++----
> 1 file changed, 34 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 14772edcf0d3..b4893214b2d3 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -245,6 +245,7 @@ struct qcom_pcie {
> struct phy *phy;
> struct gpio_desc *reset;
> struct icc_path *icc_mem;
> + struct icc_path *icc_cpu;
> const struct qcom_pcie_cfg *cfg;
> struct dentry *debugfs;
> bool suspended;
> @@ -1409,6 +1410,9 @@ static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
> if (IS_ERR(pcie->icc_mem))
> return PTR_ERR(pcie->icc_mem);
>
> + pcie->icc_cpu = devm_of_icc_get(pci->dev, "cpu-pcie");
> + if (IS_ERR(pcie->icc_cpu))
> + return PTR_ERR(pcie->icc_cpu);
> /*
> * Some Qualcomm platforms require interconnect bandwidth constraints
> * to be set before enabling interconnect clocks.
> @@ -1418,7 +1422,19 @@ static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
> */
> ret = icc_set_bw(pcie->icc_mem, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1));
> if (ret) {
> - dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
> + dev_err(pci->dev, "failed to set interconnect bandwidth for PCIe-MEM: %d\n",
> + ret);
> + return ret;
> + }
> +
> + /*
> + * Since the CPU-PCIe path is only used for activities like register
Again, differentiate PCIe controller and endpoint device access.
> + * access, Config/BAR space access, HW team has recommended to use a
> + * minimal bandwidth of 1KBps just to keep the link active.
> + */
> + ret = icc_set_bw(pcie->icc_cpu, 0, kBps_to_icc(1));
> + if (ret) {
> + dev_err(pci->dev, "failed to set interconnect bandwidth for CPU-PCIe: %d\n",
> ret);
> return ret;
> }
> @@ -1448,7 +1464,7 @@ static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
>
> 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 interconnect bandwidth: %d\n",
> + dev_err(pci->dev, "failed to set interconnect bandwidth for PCIe-MEM: %d\n",
> ret);
> }
> }
> @@ -1610,7 +1626,7 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
> */
> ret = icc_set_bw(pcie->icc_mem, 0, kBps_to_icc(1));
> if (ret) {
> - dev_err(dev, "Failed to set interconnect bandwidth: %d\n", ret);
> + dev_err(dev, "Failed to set interconnect bandwidth for PCIe-MEM: %d\n", ret);
> return ret;
> }
>
> @@ -1634,7 +1650,15 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
> pcie->suspended = true;
> }
>
> - return 0;
> + /*
> + * Remove the vote for CPU-PCIe path now, since at this point onwards,
> + * no register access will be done.
> + */
Are you sure? Didn't we see late access to DBI registers on sc7280?
> + ret = icc_disable(pcie->icc_cpu);
> + if (ret)
> + dev_err(dev, "failed to disable icc path of CPU-PCIe: %d\n", ret);
s/failed to disable icc path/Failed to disable Interconnect path between CPU-PCIe
> +
> + return ret;
> }
>
> static int qcom_pcie_resume_noirq(struct device *dev)
> @@ -1642,6 +1666,12 @@ static int qcom_pcie_resume_noirq(struct device *dev)
> struct qcom_pcie *pcie = dev_get_drvdata(dev);
> int ret;
>
> + ret = icc_enable(pcie->icc_cpu);
> + if (ret) {
> + dev_err(dev, "failed to enable icc path of CPU-PCIe: %d\n", ret);
Same as above.
- Mani
--
மணிவண்ணன் சதாசிவம்
On 4/7/2024 8:09 PM, Manivannan Sadhasivam wrote:
> On Sun, Apr 07, 2024 at 10:07:35AM +0530, Krishna chaitanya chundru wrote:
>> To access PCIe registers, PCIe BAR space, config space the CPU-PCIe
>
> Please specify whether you are referencing PCIe host controller or endpoint
> device or both.
>
>> ICC (interconnect consumers) path should be voted otherwise it may
>
> ICC is just 'Interconnect' unless I misunderstood.
>
>> lead to NoC (Network on chip) timeout. We are surviving because of
>> other driver vote for this path.
>>
>
> s/vote/voting
>
>> As there is less access on this path compared to PCIe to mem path
>> add minimum vote i.e 1KBps bandwidth always which is recommended
>> by HW team.
>>
>
> 'which is sufficient enough to keep the path active.'
>
>> When suspending, disable this path after register space access
>> is done.
>>
>> Reviewed-by: Bryan O'Donoghue <[email protected]>
>> Signed-off-by: Krishna chaitanya chundru <[email protected]>
>> ---
>> drivers/pci/controller/dwc/pcie-qcom.c | 38 ++++++++++++++++++++++++++++++----
>> 1 file changed, 34 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>> index 14772edcf0d3..b4893214b2d3 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -245,6 +245,7 @@ struct qcom_pcie {
>> struct phy *phy;
>> struct gpio_desc *reset;
>> struct icc_path *icc_mem;
>> + struct icc_path *icc_cpu;
>> const struct qcom_pcie_cfg *cfg;
>> struct dentry *debugfs;
>> bool suspended;
>> @@ -1409,6 +1410,9 @@ static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
>> if (IS_ERR(pcie->icc_mem))
>> return PTR_ERR(pcie->icc_mem);
>>
>> + pcie->icc_cpu = devm_of_icc_get(pci->dev, "cpu-pcie");
>> + if (IS_ERR(pcie->icc_cpu))
>> + return PTR_ERR(pcie->icc_cpu);
>> /*
>> * Some Qualcomm platforms require interconnect bandwidth constraints
>> * to be set before enabling interconnect clocks.
>> @@ -1418,7 +1422,19 @@ static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
>> */
>> ret = icc_set_bw(pcie->icc_mem, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1));
>> if (ret) {
>> - dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
>> + dev_err(pci->dev, "failed to set interconnect bandwidth for PCIe-MEM: %d\n",
>> + ret);
>> + return ret;
>> + }
>> +
>> + /*
>> + * Since the CPU-PCIe path is only used for activities like register
>
> Again, differentiate PCIe controller and endpoint device access.
>
Ack to all comments. I will modify in next patch.
>> + * access, Config/BAR space access, HW team has recommended to use a
>> + * minimal bandwidth of 1KBps just to keep the link active.
>> + */
>> + ret = icc_set_bw(pcie->icc_cpu, 0, kBps_to_icc(1));
>> + if (ret) {
>> + dev_err(pci->dev, "failed to set interconnect bandwidth for CPU-PCIe: %d\n",
>> ret);
>> return ret;
>> }
>> @@ -1448,7 +1464,7 @@ static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
>>
>> 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 interconnect bandwidth: %d\n",
>> + dev_err(pci->dev, "failed to set interconnect bandwidth for PCIe-MEM: %d\n",
>> ret);
>> }
>> }
>> @@ -1610,7 +1626,7 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
>> */
>> ret = icc_set_bw(pcie->icc_mem, 0, kBps_to_icc(1));
>> if (ret) {
>> - dev_err(dev, "Failed to set interconnect bandwidth: %d\n", ret);
>> + dev_err(dev, "Failed to set interconnect bandwidth for PCIe-MEM: %d\n", ret);
>> return ret;
>> }
>>
>> @@ -1634,7 +1650,15 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
>> pcie->suspended = true;
>> }
>>
>> - return 0;
>> + /*
>> + * Remove the vote for CPU-PCIe path now, since at this point onwards,
>> + * no register access will be done.
>> + */
>
> Are you sure? Didn't we see late access to DBI registers on sc7280?
>
yeah you are correct I will add a check to disable icc only in suspend
to idle case. only in suspend to ram case we see the DBI access in sc7280
-Krishna Chaitanya
>> + ret = icc_disable(pcie->icc_cpu);
>> + if (ret)
>> + dev_err(dev, "failed to disable icc path of CPU-PCIe: %d\n", ret);
>
> s/failed to disable icc path/Failed to disable Interconnect path between CPU-PCIe
>
>> +
>> + return ret;
>> }
>>
>> static int qcom_pcie_resume_noirq(struct device *dev)
>> @@ -1642,6 +1666,12 @@ static int qcom_pcie_resume_noirq(struct device *dev)
>> struct qcom_pcie *pcie = dev_get_drvdata(dev);
>> int ret;
>>
>> + ret = icc_enable(pcie->icc_cpu);
>> + if (ret) {
>> + dev_err(dev, "failed to enable icc path of CPU-PCIe: %d\n", ret);
>
> Same as above.
>
> - Mani
>