Re: [PATCH v5 4/5] PCI: qcom: Add interconnect support to 2.7.0/1.9.0 ops

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Fri 04 Feb 06:38 PST 2022, Dmitry Baryshkov wrote:

> On 03/02/2022 18:57, Bjorn Andersson wrote:
> > On Sat 18 Dec 06:10 PST 2021, Dmitry Baryshkov wrote:
> > 
> > > Add optional interconnect support for the 2.7.0/1.9.0 hosts. Set the
> > > bandwidth according to the values from the downstream driver.
> > > 
> > 
> > What memory transactions will travel this path? I would expect there to
> > be two different paths involved, given the rather low bw numbers I
> > presume this is the config path?
> 
> I think so. Downstream votes on this path for most of the known SoCs. Two
> spotted omissions are ipq8074 and qcs404.
> 

Sorry, missed your reply on this one.

> > 
> > Is there no vote for the data path?
> 
> CNSS devices can vote additionally on the MASTER_PCI to memory paths:
> For sm845 (45 = MASTER_PCIE):
>                 qcom,msm-bus,vectors-KBps =
>                         <45 512 0 0>,
>                         <45 512 600000 800000>; /* ~4.6Gbps (MCS12) */
> 
> On sm8150/sm8250 qca bindings do not contain a vote, but wil6210 does (100 =
> MASTER_PCIE_1):
>                 qcom,msm-bus,vectors-KBps =
>                         <100 512 0 0>,
>                         <100 512 600000 800000>; /* ~4.6Gbps (MCS12) */

This is PCIe -> DDR, so I think we should interconnect-names this path
"pci-ddr". I also see that on at least some platforms the value depends
on PCIe Gen. So perhaps we should start by just picking these values for
now and then follow up with something where we add the numbers in an
opp-table based on Gen?

> 
> For sm8450 there are two paths used by cnss:
> 		<&pcie_noc MASTER_PCIE_0 &pcie_noc SLAVE_ANOC_PCIE_GEM_NOC>,
> 		<&gem_noc MASTER_ANOC_PCIE_GEM_NOC &mc_virt SLAVE_EBI1>;
> 
> with multiple entries per each path.
> 
> So, I'm not sure about these values.
> 

That seems to be PCIe to master and then a separate segment for the
memory NoC to DDR. That's odd. I think we should attempt to just do:

 <&pcie_noc MASTER_PCIE_0 &mc_virt SLAVE_EBI1>

As a single path for "pci-ddr"

> > 
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > ---
> > >   drivers/pci/controller/dwc/pcie-qcom.c | 11 +++++++++++
> > >   1 file changed, 11 insertions(+)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > index d8d400423a0a..55ac3caa6d7d 100644
> > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > @@ -12,6 +12,7 @@
> > >   #include <linux/crc8.h>
> > >   #include <linux/delay.h>
> > >   #include <linux/gpio/consumer.h>
> > > +#include <linux/interconnect.h>
> > >   #include <linux/interrupt.h>
> > >   #include <linux/io.h>
> > >   #include <linux/iopoll.h>
> > > @@ -167,6 +168,7 @@ struct qcom_pcie_resources_2_7_0 {
> > >   	struct clk *pipe_clk_src;
> > >   	struct clk *phy_pipe_clk;
> > >   	struct clk *ref_clk_src;
> > > +	struct icc_path *path;
> > >   };
> > >   union qcom_pcie_resources {
> > > @@ -1121,6 +1123,10 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
> > >   	if (IS_ERR(res->pci_reset))
> > >   		return PTR_ERR(res->pci_reset);
> > > +	res->path = devm_of_icc_get(dev, "pci");
> > 
> > The paths are typically identified using a string of the form
> > <source>-<destination>.
> > 
> > 
> > I don't see the related update to the DT binding for the introduction of
> > the interconnect.
> > 
> > Regards,
> > Bjorn
> > 
> > > +	if (IS_ERR(res->path))
> > > +		return PTR_ERR(res->path);
> > > +
> > >   	res->supplies[0].supply = "vdda";
> > >   	res->supplies[1].supply = "vddpe-3v3";
> > >   	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(res->supplies),
> > > @@ -1183,6 +1189,9 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
> > >   	if (pcie->cfg->pipe_clk_need_muxing)
> > >   		clk_set_parent(res->pipe_clk_src, res->phy_pipe_clk);
> > > +	if (res->path)
> > > +		icc_set_bw(res->path, 500, 800);

But that said, these numbers doesn't resemble the numbers you show
above and they don't make sense for the "data path". So perhaps this is
a separate "pci-config" path?

Regards,
Bjorn

> > > +
> > >   	ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
> > >   	if (ret < 0)
> > >   		goto err_disable_regulators;
> > > @@ -1241,6 +1250,8 @@ static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
> > >   	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> > >   	clk_bulk_disable_unprepare(res->num_clks, res->clks);
> > > +	if (res->path)
> > > +		icc_set_bw(res->path, 0, 0);
> > >   	/* Set TCXO as clock source for pcie_pipe_clk_src */
> > >   	if (pcie->cfg->pipe_clk_need_muxing)
> > > -- 
> > > 2.34.1
> > > 
> 
> 
> -- 
> With best wishes
> Dmitry