| Message ID | 20211218141024.500952-5-dmitry.baryshkov@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | qcom: add support for PCIe on SM8450 platform | expand |
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? Is there no vote for the data path? > 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); > + > 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 >
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. > > 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) */ 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. > >> 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); >> + >> 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 >>
On Fri, Feb 04, 2022 at 05:38:33PM +0300, 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. > > > > > 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) */ > > 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. This discussion is gating the series, please let me know if you want me to cherry-pick the other patches or you will resend the series. Lorenzo > > > 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); > > > + > > > 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
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
On Fri 11 Feb 08:12 PST 2022, Lorenzo Pieralisi wrote: > On Fri, Feb 04, 2022 at 05:38:33PM +0300, 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. > > > > > > > > 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) */ > > > > 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. > > This discussion is gating the series, please let me know if you want me > to cherry-pick the other patches or you will resend the series. > Please pick the other patches and I'll work with Dmitry to conclude how this is actually connected to the busses inside the SoC. Thanks, Bjorn > Lorenzo > > > > > 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); > > > > + > > > > 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
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. > > 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; I think it's fair to assume that pretty much all platforms will have a data path to reach the config registers and for the PCI to reach DDR. So how about we place this in the common struct qcom_pcie instead? Regards, Bjorn > }; > > 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"); > + 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); > + > 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 >
On 23/02/2022 02:49, 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. >> >> 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; > > I think it's fair to assume that pretty much all platforms will have a > data path to reach the config registers and for the PCI to reach DDR. > > So how about we place this in the common struct qcom_pcie instead? Sounds logical. I'll think about it. > > Regards, > Bjorn > >> }; >> >> 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"); >> + 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); >> + >> 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 >>
On Tue, Feb 22, 2022 at 03:47:30PM -0800, Bjorn Andersson wrote: > On Fri 11 Feb 08:12 PST 2022, Lorenzo Pieralisi wrote: > > > On Fri, Feb 04, 2022 at 05:38:33PM +0300, 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. > > > > > > > > > > > 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) */ > > > > > > 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. > > > > This discussion is gating the series, please let me know if you want me > > to cherry-pick the other patches or you will resend the series. > > > > Please pick the other patches and I'll work with Dmitry to conclude how > this is actually connected to the busses inside the SoC. Hi, can you resend the series without this patch rebased on top of v5.17-rc1 please ? Thanks, Lorenzo > > Thanks, > Bjorn > > > Lorenzo > > > > > > > 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); > > > > > + > > > > > 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
On 23/02/2022 12:31, Lorenzo Pieralisi wrote: > On Tue, Feb 22, 2022 at 03:47:30PM -0800, Bjorn Andersson wrote: >> On Fri 11 Feb 08:12 PST 2022, Lorenzo Pieralisi wrote: >> >>> On Fri, Feb 04, 2022 at 05:38:33PM +0300, 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. >>>> >>>>> >>>>> 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) */ >>>> >>>> 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. >>> >>> This discussion is gating the series, please let me know if you want me >>> to cherry-pick the other patches or you will resend the series. >>> >> >> Please pick the other patches and I'll work with Dmitry to conclude how >> this is actually connected to the busses inside the SoC. > > Hi, > > can you resend the series without this patch rebased on top of > v5.17-rc1 please ? Done. I've posted v6.
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"); + 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); + 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)
Add optional interconnect support for the 2.7.0/1.9.0 hosts. Set the bandwidth according to the values from the downstream driver. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/pci/controller/dwc/pcie-qcom.c | 11 +++++++++++ 1 file changed, 11 insertions(+)