diff mbox series

[v6,3/6] PCI: qcom: Add missing icc bandwidth vote for cpu to PCIe path

Message ID 20240112-opp_support-v6-3-77bbf7d0cc37@quicinc.com
State Superseded
Headers show
Series PCI: qcom: Add support for OPP | expand

Commit Message

Krishna Chaitanya Chundru Jan. 12, 2024, 2:22 p.m. UTC
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(-)

Comments

Krishna Chaitanya Chundru Jan. 16, 2024, 4:57 a.m. UTC | #1
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.

- 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
>>
>>
> 
>
Krishna Chaitanya Chundru Jan. 16, 2024, 5:04 a.m. UTC | #2
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.
Please suggest a recommended value for this path if the GEN1x1 is high.
>> 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.
>> @@ -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()
> 
I was not aware of these API's, I will add them in next patch.

- Krishna Chaitanya.
>> +	if (ret) {
>> +		dev_err(dev, "failed to set interconnect bandwidth for cpu-pcie: %d\n", ret);
>> +		return ret;
>> +	}
> 
> Johan
Johan Hovold Jan. 16, 2024, 10:46 a.m. UTC | #3
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
Johan Hovold Jan. 16, 2024, 10:54 a.m. UTC | #4
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
Manivannan Sadhasivam Jan. 17, 2024, 6:39 a.m. UTC | #5
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
> > > 
> > > 
> > 
> > 
>
Krishna Chaitanya Chundru Jan. 29, 2024, 2:10 p.m. UTC | #6
On 1/17/2024 12:09 PM, Manivannan Sadhasivam wrote:
> 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
> 
HW team suggested to use minimum value of 1Kbps for this path.
I will update the patches to use 1Kbps in the next series.

- Krishna Chaitanya.
>> - 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
>>>>
>>>>
>>>
>>>
>>
>
diff mbox series

Patch

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)