Re: [PATCH v12 2/6] PCI: qcom: Add ICC bandwidth vote for CPU to PCIe path

[Krishna Chaitanya Chundru <quic_krichai@xxxxxxxxxxx>]

On 5/10/2024 12:04 AM, Mayank Rana wrote:
> Hi Krishna
>
> On 4/26/2024 6:52 PM, Krishna chaitanya chundru wrote:
> > To access the host controller registers of the host controller and the
> > endpoint BAR/config space, the CPU-PCIe ICC (interconnect) path should
> > be voted otherwise it may lead to NoC (Network on chip) timeout.
> > We are surviving because of other driver voting 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 sufficient enough
> > to keep the path active and is recommended by HW team.
> >
> > During S2RAM (Suspend-to-RAM), the DBI access can happen very late (while
> > disabling the boot CPU). So do not disable the CPU-PCIe interconnect path
> > during S2RAM as that may lead to NoC error.
> >
> > Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
> > Signed-off-by: Krishna chaitanya chundru <quic_krichai@xxxxxxxxxxx>
> > ---
> >   drivers/pci/controller/dwc/pcie-qcom.c | 44 
> > ++++++++++++++++++++++++++++++----
> >   1 file changed, 40 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c 
> > b/drivers/pci/controller/dwc/pcie-qcom.c
> > index 14772edcf0d3..465d63b4be1c 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,20 @@ 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 bandwidth for PCIe-MEM 
> > interconnect path: %d\n",
> > +            ret);
> > +        return ret;
> > +    }
> > +
> > +    /*
> > +     * Since the CPU-PCIe path is only used for activities like register
> > +     * access of the host controller and endpoint Config/BAR space 
> > access,
> > +     * HW team has recommended to use a minimal bandwidth of 1KBps 
> > just to
> > +     * keep the path active.
> > +     */
> > +    ret = icc_set_bw(pcie->icc_cpu, 0, kBps_to_icc(1));
> > +    if (ret) {
> > +        dev_err(pci->dev, "Failed to set bandwidth for CPU-PCIe 
> > interconnect path: %d\n",
> >               ret);
> Is it needed to undo icc_mem related bus bandwidth vote here ?
I will add the logic to remove icc_mem path while returning from here,
in the next patch series.

- Krishna Chaitanya.
> >           return ret;
> >       }
> > @@ -1448,7 +1465,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 bandwidth for PCIe-MEM 
> > interconnect path: %d\n",
> >               ret);
> >       }
> >   }