| Message ID | 20240112-opp_support-v6-3-77bbf7d0cc37@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: qcom: Add support for OPP | expand |
On Fri, Jan 12, 2024 at 07:52:02PM +0530, Krishna chaitanya chundru wrote: > CPU-PCIe path consits for registers PCIe BAR space, config space. consits? > As there is less access on this path compared to pcie to mem path > add minimum vote i.e GEN1x1 bandwidth always. gen1 bandwidth can't be right. > In suspend remove the cpu vote after register space access is done. > > Fixes: c4860af88d0c ("PCI: qcom: Add basic interconnect support") > cc: stable@vger.kernel.org This does not look like a fix so drop the above. The commit you refer to explicitly left this path unconfigured for now and only added support for the configuring the mem path as needed on sc8280xp which otherwise would crash. > @@ -1573,7 +1588,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; > } > > @@ -1597,6 +1612,12 @@ static int qcom_pcie_suspend_noirq(struct device *dev) > pcie->suspended = true; > } > > + /* Remove cpu path vote after all the register access is done */ > + ret = icc_set_bw(pcie->icc_cpu, 0, 0); I believe you should use icc_disable() here. > + if (ret) { > + dev_err(dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n", ret); > + return ret; And you need to unwind before returning on errors. > + } > return 0; > } > > @@ -1605,6 +1626,12 @@ static int qcom_pcie_resume_noirq(struct device *dev) > struct qcom_pcie *pcie = dev_get_drvdata(dev); > int ret; > > + ret = icc_set_bw(pcie->icc_cpu, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1)); icc_enable() > + if (ret) { > + dev_err(dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n", ret); > + return ret; > + } Johan
Capitalize "ICC" and "CPU" to make the subject easier to read. "Missing" might be superfluous in the subject? It would be nice to have the ICC expansion once in the commit log as a hook for newbies like me :) On Fri, Jan 12, 2024 at 07:52:02PM +0530, Krishna chaitanya chundru wrote: > CPU-PCIe path consits for registers PCIe BAR space, config space. > As there is less access on this path compared to pcie to mem path > add minimum vote i.e GEN1x1 bandwidth always. "GEN1x1" is unnecessarily ambiguous, and the spec recommends avoiding it (PCIe r6.0, sec 1.2). Use the actual bandwidth numbers instead. "PCIe" to match above. Also below in comments and messages. > In suspend remove the cpu vote after register space access is done. "CPU" to match above. > Fixes: c4860af88d0c ("PCI: qcom: Add basic interconnect support") > cc: stable@vger.kernel.org > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> > --- > drivers/pci/controller/dwc/pcie-qcom.c | 31 +++++++++++++++++++++++++++++-- > 1 file changed, 29 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > index 11c80555d975..035953f0b6d8 100644 > --- a/drivers/pci/controller/dwc/pcie-qcom.c > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > @@ -240,6 +240,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; > @@ -1372,6 +1373,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. > @@ -1381,7 +1385,18 @@ 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; > + } > + > + /* > + * The config space, BAR space and registers goes through cpu-pcie path. > + * Set peak bandwidth to single-lane Gen1 for this path all the time. Numbers instead of "Gen1". > + */ > + ret = icc_set_bw(pcie->icc_cpu, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1)); > + if (ret) { > + dev_err(pci->dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n", > ret); > return ret; > } > @@ -1573,7 +1588,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; > } > > @@ -1597,6 +1612,12 @@ static int qcom_pcie_suspend_noirq(struct device *dev) > pcie->suspended = true; > } > > + /* Remove cpu path vote after all the register access is done */ > + ret = icc_set_bw(pcie->icc_cpu, 0, 0); > + if (ret) { > + dev_err(dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n", ret); > + return ret; > + } > return 0; > } > > @@ -1605,6 +1626,12 @@ static int qcom_pcie_resume_noirq(struct device *dev) > struct qcom_pcie *pcie = dev_get_drvdata(dev); > int ret; > > + ret = icc_set_bw(pcie->icc_cpu, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1)); > + if (ret) { > + dev_err(dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n", ret); > + return ret; > + } > + > if (pcie->suspended) { > ret = qcom_pcie_host_init(&pcie->pci->pp); > if (ret) > > -- > 2.42.0 >
On 12.01.2024 16:59, Johan Hovold wrote: > On Fri, Jan 12, 2024 at 07:52:02PM +0530, Krishna chaitanya chundru wrote: >> CPU-PCIe path consits for registers PCIe BAR space, config space. > > consits? > >> As there is less access on this path compared to pcie to mem path >> add minimum vote i.e GEN1x1 bandwidth always. > > gen1 bandwidth can't be right. > >> In suspend remove the cpu vote after register space access is done. >> >> Fixes: c4860af88d0c ("PCI: qcom: Add basic interconnect support") >> cc: stable@vger.kernel.org > > This does not look like a fix so drop the above. > > The commit you refer to explicitly left this path unconfigured for now > and only added support for the configuring the mem path as needed on > sc8280xp which otherwise would crash. I only sorta agree. I'd include a fixes tag but point it to either 8450 addition or original driver introduction, as this is patching up a real hole (see my reply to Bryan). > >> @@ -1573,7 +1588,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; >> } >> >> @@ -1597,6 +1612,12 @@ static int qcom_pcie_suspend_noirq(struct device *dev) >> pcie->suspended = true; >> } >> >> + /* Remove cpu path vote after all the register access is done */ >> + ret = icc_set_bw(pcie->icc_cpu, 0, 0); > > I believe you should use icc_disable() here. Oh, TIL this exists! Konrad
On 1/12/2024 8:47 PM, Bryan O'Donoghue wrote: > On 12/01/2024 14:22, Krishna chaitanya chundru wrote: >> CPU-PCIe path consits for registers PCIe BAR space, config space. >> As there is less access on this path compared to pcie to mem path >> add minimum vote i.e GEN1x1 bandwidth always. >> >> In suspend remove the cpu vote after register space access is done. >> >> Fixes: c4860af88d0c ("PCI: qcom: Add basic interconnect support") > > If this patch is a Fixes then don't you need the accompanying dts change > as a parallel Fixes too ? > > i.e. without the dts update - you won't have the nodes in the dts to > consume => applying this code to the stable kernel absent the dts will > result in no functional change and therefore no bugfix. > > I'm not sure if you are asked to put a Fixes here but it seems to be it > should either be dropped or require a parallel Fixes: tag for the dts > and yaml changes. > > What is the bug this change fixes in the backport ? > There is no change required in the dts because the cpu-pcie path is already present in the dts. So till now driver is ignoring that path, that's why we tagged with fixed. -Krishna Chaitanya >> cc: stable@vger.kernel.org >> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> > > --- > bod
On 1/12/2024 10:17 PM, Bjorn Helgaas wrote: > Capitalize "ICC" and "CPU" to make the subject easier to read. > "Missing" might be superfluous in the subject? It would be nice to > have the ICC expansion once in the commit log as a hook for newbies > like me :) > Sure I will change a suggested in next patch series. > On Fri, Jan 12, 2024 at 07:52:02PM +0530, Krishna chaitanya chundru wrote: >> CPU-PCIe path consits for registers PCIe BAR space, config space. >> As there is less access on this path compared to pcie to mem path >> add minimum vote i.e GEN1x1 bandwidth always. > > "GEN1x1" is unnecessarily ambiguous, and the spec recommends avoiding > it (PCIe r6.0, sec 1.2). Use the actual bandwidth numbers instead. > > "PCIe" to match above. Also below in comments and messages. > ACK. >> In suspend remove the cpu vote after register space access is done. > > "CPU" to match above. > ACK >> Fixes: c4860af88d0c ("PCI: qcom: Add basic interconnect support") >> cc: stable@vger.kernel.org >> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> >> --- >> drivers/pci/controller/dwc/pcie-qcom.c | 31 +++++++++++++++++++++++++++++-- >> 1 file changed, 29 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c >> index 11c80555d975..035953f0b6d8 100644 >> --- a/drivers/pci/controller/dwc/pcie-qcom.c >> +++ b/drivers/pci/controller/dwc/pcie-qcom.c >> @@ -240,6 +240,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; >> @@ -1372,6 +1373,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. >> @@ -1381,7 +1385,18 @@ 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; >> + } >> + >> + /* >> + * The config space, BAR space and registers goes through cpu-pcie path. >> + * Set peak bandwidth to single-lane Gen1 for this path all the time. > > Numbers instead of "Gen1". > ACK -Krishna Chaitanya. >> + */ >> + ret = icc_set_bw(pcie->icc_cpu, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1)); >> + if (ret) { >> + dev_err(pci->dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n", >> ret); >> return ret; >> } >> @@ -1573,7 +1588,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; >> } >> >> @@ -1597,6 +1612,12 @@ static int qcom_pcie_suspend_noirq(struct device *dev) >> pcie->suspended = true; >> } >> >> + /* Remove cpu path vote after all the register access is done */ >> + ret = icc_set_bw(pcie->icc_cpu, 0, 0); >> + if (ret) { >> + dev_err(dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n", ret); >> + return ret; >> + } >> return 0; >> } >> >> @@ -1605,6 +1626,12 @@ static int qcom_pcie_resume_noirq(struct device *dev) >> struct qcom_pcie *pcie = dev_get_drvdata(dev); >> int ret; >> >> + ret = icc_set_bw(pcie->icc_cpu, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1)); >> + if (ret) { >> + dev_err(dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n", ret); >> + return ret; >> + } >> + >> if (pcie->suspended) { >> ret = qcom_pcie_host_init(&pcie->pci->pp); >> if (ret) >> >> -- >> 2.42.0 >>
On 16/01/2024 04:52, Krishna Chaitanya Chundru wrote: >> > There is no change required in the dts because the cpu-pcie path is > already present in the dts. Not at c4860af88d0cb1bb006df12615c5515ae509f73b its not, those dts entries get added later. But anyway re-reading your commit log "vote for minimum bandwidth as at c4860af88d0cb1bb006df12615c5515ae509f73b" makes sense to me. Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
On Tue, Jan 16, 2024 at 10:34:22AM +0530, Krishna Chaitanya Chundru wrote: > > > On 1/12/2024 9:29 PM, Johan Hovold wrote: > > On Fri, Jan 12, 2024 at 07:52:02PM +0530, Krishna chaitanya chundru wrote: > >> CPU-PCIe path consits for registers PCIe BAR space, config space. > > > > consits? > > > >> As there is less access on this path compared to pcie to mem path > >> add minimum vote i.e GEN1x1 bandwidth always. > > > > gen1 bandwidth can't be right. > There is no recommended value we need vote for this path, as there is > BAR and config space in this path we are voting for GEN1x1. I can see that, but that does not explain why you used those seemingly arbitrary numbers or why you think that's correct. > Please suggest a recommended value for this path if the GEN1x1 is high. No, you submitted the patch and you work for Qualcomm. You need to figure out what the value should be. All I can say is that the gen1 value is likely not correct and therefore confusing. > >> In suspend remove the cpu vote after register space access is done. > >> > >> Fixes: c4860af88d0c ("PCI: qcom: Add basic interconnect support") > >> cc: stable@vger.kernel.org > > > > This does not look like a fix so drop the above. > > > > The commit you refer to explicitly left this path unconfigured for now > > and only added support for the configuring the mem path as needed on > > sc8280xp which otherwise would crash. > Without this path vote BAR and config space can result NOC timeout > errors, we are surviving because of other driver vote for this path. > For that reason we added a fix tag. Ok, then mention that in the commit message so that it becomes more clear why this is needed and whether this should be considered a fix. As it stands, the commit message makes this look like a new feature. And the above Fixes tag is incorrect either way as that commit did not introduce any issue. Johan
On Fri, Jan 12, 2024 at 11:37:03PM +0100, Konrad Dybcio wrote: > On 12.01.2024 16:59, Johan Hovold wrote: > > On Fri, Jan 12, 2024 at 07:52:02PM +0530, Krishna chaitanya chundru wrote: > >> CPU-PCIe path consits for registers PCIe BAR space, config space. > > > > consits? > > > >> As there is less access on this path compared to pcie to mem path > >> add minimum vote i.e GEN1x1 bandwidth always. > > > > gen1 bandwidth can't be right. > > > >> In suspend remove the cpu vote after register space access is done. > >> > >> Fixes: c4860af88d0c ("PCI: qcom: Add basic interconnect support") > >> cc: stable@vger.kernel.org > > > > This does not look like a fix so drop the above. > > > > The commit you refer to explicitly left this path unconfigured for now > > and only added support for the configuring the mem path as needed on > > sc8280xp which otherwise would crash. > > I only sorta agree. I'd include a fixes tag but point it to either 8450 > addition or original driver introduction, as this is patching up a real > hole (see my reply to Bryan). Right, the above Fixes tag is not correct in any case. And with a complete commit message it may be possible to tell whether a Fixes tag is warranted or not. Johan
On Tue, Jan 16, 2024 at 10:27:23AM +0530, Krishna Chaitanya Chundru wrote: > > > On 1/12/2024 9:00 PM, Dmitry Baryshkov wrote: > > On Fri, 12 Jan 2024 at 16:24, Krishna chaitanya chundru > > <quic_krichai@quicinc.com> wrote: > > > > > > CPU-PCIe path consits for registers PCIe BAR space, config space. > > > As there is less access on this path compared to pcie to mem path > > > add minimum vote i.e GEN1x1 bandwidth always. > > > > Is this BW amount a real requirement or just a random number? I mean, > > the register space in my opinion consumes much less bandwidth compared > > to Gen1 memory access. > > > Not register space right the BAR space and config space access from CPU > goes through this path only. There is no recommended value we need to > vote for this path. Keeping BAR space and config space we tried to vote > for GEN1x1. > > Please suggest any recommended value, I will change that in the next > series. > You should ask the HW folks on the recommended value to keep the reg access clocking. We cannot suggest a value here. If they say, "there is no recommended value", then ask them what would the minimum value and use it here. - Mani > - Krishna Chaitanya. > > > > > > In suspend remove the cpu vote after register space access is done. > > > > > > Fixes: c4860af88d0c ("PCI: qcom: Add basic interconnect support") > > > cc: stable@vger.kernel.org > > > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> > > > --- > > > drivers/pci/controller/dwc/pcie-qcom.c | 31 +++++++++++++++++++++++++++++-- > > > 1 file changed, 29 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > > > index 11c80555d975..035953f0b6d8 100644 > > > --- a/drivers/pci/controller/dwc/pcie-qcom.c > > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > > > @@ -240,6 +240,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; > > > @@ -1372,6 +1373,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. > > > @@ -1381,7 +1385,18 @@ 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; > > > + } > > > + > > > + /* > > > + * The config space, BAR space and registers goes through cpu-pcie path. > > > + * Set peak bandwidth to single-lane Gen1 for this path all the time. > > > + */ > > > + ret = icc_set_bw(pcie->icc_cpu, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1)); > > > + if (ret) { > > > + dev_err(pci->dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n", > > > ret); > > > return ret; > > > } > > > @@ -1573,7 +1588,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; > > > } > > > > > > @@ -1597,6 +1612,12 @@ static int qcom_pcie_suspend_noirq(struct device *dev) > > > pcie->suspended = true; > > > } > > > > > > + /* Remove cpu path vote after all the register access is done */ > > > + ret = icc_set_bw(pcie->icc_cpu, 0, 0); > > > + if (ret) { > > > + dev_err(dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n", ret); > > > + return ret; > > > + } > > > return 0; > > > } > > > > > > @@ -1605,6 +1626,12 @@ static int qcom_pcie_resume_noirq(struct device *dev) > > > struct qcom_pcie *pcie = dev_get_drvdata(dev); > > > int ret; > > > > > > + ret = icc_set_bw(pcie->icc_cpu, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1)); > > > + if (ret) { > > > + dev_err(dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n", ret); > > > + return ret; > > > + } > > > + > > > if (pcie->suspended) { > > > ret = qcom_pcie_host_init(&pcie->pci->pp); > > > if (ret) > > > > > > -- > > > 2.42.0 > > > > > > > > > > >
On 1/16/24 11:52, Johan Hovold wrote: > On Fri, Jan 12, 2024 at 11:33:15PM +0100, Konrad Dybcio wrote: >> On 12.01.2024 16:17, Bryan O'Donoghue wrote: >>> On 12/01/2024 14:22, Krishna chaitanya chundru wrote: >>>> CPU-PCIe path consits for registers PCIe BAR space, config space. >>>> As there is less access on this path compared to pcie to mem path >>>> add minimum vote i.e GEN1x1 bandwidth always. >>>> >>>> In suspend remove the cpu vote after register space access is done. >>>> >>>> Fixes: c4860af88d0c ("PCI: qcom: Add basic interconnect support") >>> >>> If this patch is a Fixes then don't you need the accompanying dts change as a parallel Fixes too ? >>> >>> i.e. without the dts update - you won't have the nodes in the dts to consume => applying this code to the stable kernel absent the dts will result in no functional change and therefore no bugfix. >> >> The Fixes tag denotes a bug fix, its use for backport autosel is just >> a nice "coincidence". >> >> Fixing a lack of a required icc path and having to rely on BL leftovers >> / keepalive bus settings is definitely worth this tag in my eyes. > > An incomplete implementation can sometimes be considered a bug, but not > always. If this is needed to enable a new use case, then it's hard to > argue that the original omission was a bug. > > And as I just mentioned to Krishna, the above Fixes tag is not correct > as that commit did not *introduce* any issue. It solved the bit that was > strictly needed for sc8280xp, but now it seems you may need something > more for an even newer platform (even if no details and motivation was > included in the commit message as it should have been). The PCIe hardware seems to be piggybacking off of others' bus bandwidth requests and I think it's just been pure luck that it didn't simply refuse to work on previous generations. So indeed, the commit message seems incomplete in explaining where the problem lies Konrad
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c index 11c80555d975..035953f0b6d8 100644 --- a/drivers/pci/controller/dwc/pcie-qcom.c +++ b/drivers/pci/controller/dwc/pcie-qcom.c @@ -240,6 +240,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; @@ -1372,6 +1373,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. @@ -1381,7 +1385,18 @@ 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; + } + + /* + * The config space, BAR space and registers goes through cpu-pcie path. + * Set peak bandwidth to single-lane Gen1 for this path all the time. + */ + ret = icc_set_bw(pcie->icc_cpu, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1)); + if (ret) { + dev_err(pci->dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n", ret); return ret; } @@ -1573,7 +1588,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; } @@ -1597,6 +1612,12 @@ static int qcom_pcie_suspend_noirq(struct device *dev) pcie->suspended = true; } + /* Remove cpu path vote after all the register access is done */ + ret = icc_set_bw(pcie->icc_cpu, 0, 0); + if (ret) { + dev_err(dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n", ret); + return ret; + } return 0; } @@ -1605,6 +1626,12 @@ static int qcom_pcie_resume_noirq(struct device *dev) struct qcom_pcie *pcie = dev_get_drvdata(dev); int ret; + ret = icc_set_bw(pcie->icc_cpu, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1)); + if (ret) { + dev_err(dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n", ret); + return ret; + } + if (pcie->suspended) { ret = qcom_pcie_host_init(&pcie->pci->pp); if (ret)
CPU-PCIe path consits for registers PCIe BAR space, config space. As there is less access on this path compared to pcie to mem path add minimum vote i.e GEN1x1 bandwidth always. In suspend remove the cpu vote after register space access is done. Fixes: c4860af88d0c ("PCI: qcom: Add basic interconnect support") cc: stable@vger.kernel.org Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> --- drivers/pci/controller/dwc/pcie-qcom.c | 31 +++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-)