[PATCH v5 0/5] qcom: add support for PCIe on SM8450 platform

[PATCH v5 0/5] qcom: add support for PCIe on SM8450 platform

[Dmitry Baryshkov]

There are two different PCIe controllers and PHYs on SM8450, one having
one lane and another with two lanes. Add support for both PCIe
controllers

Dependencies:
 - https://lore.kernel.org/linux-arm-msm/20211218140223.500390-1-dmitry.baryshkov@linaro.org/

Changes since v4:
 - Add PCIe1 support
 - Change binding accordingly, to use qcom,pcie-sm8450-pcie0 and
   qcom,pcie-sm8450-pcie1 compatibility strings
 - Rebase on top of (pending) pipe_clock cleanup/rework patchset

Changes since v3:
 - Fix pcie gpios to follow defined schema as noted by Rob
 - Fix commit message according to Bjorn's suggestions

Changes since v2:
 - Remove unnecessary comment in struct qcom_pcie_cfg

Changes since v1:
 - Fix capitalization/wording of PCI patch subjects
 - Add missing gen3x1 specification to PHY table names

----------------------------------------------------------------
Dmitry Baryshkov (5):
      dt-bindings: pci: qcom: Document PCIe bindings for SM8450
      PCI: qcom: Remove redundancy between qcom_pcie and qcom_pcie_cfg
      PCI: qcom: Add ddrss_sf_tbu flag
      PCI: qcom: Add interconnect support to 2.7.0/1.9.0 ops
      PCI: qcom: Add SM8450 PCIe support

 .../devicetree/bindings/pci/qcom,pcie.txt          |  22 ++++-
 drivers/pci/controller/dwc/pcie-qcom.c             | 101 ++++++++++++++-------
 2 files changed, 91 insertions(+), 32 deletions(-)

[PATCH v5 1/5] dt-bindings: pci: qcom: Document PCIe bindings for SM8450

[Dmitry Baryshkov]

Document the PCIe DT bindings for SM8450 SoC. The PCIe IP is similar
to the one used on SM8250, however unlike SM8250, PCIe0 and PCIe1 use
different set of clocks, so two compatible entries are required.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 .../devicetree/bindings/pci/qcom,pcie.txt     | 22 ++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
index a0ae024c2d0c..0adb56d5645e 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
@@ -15,6 +15,8 @@
 			- "qcom,pcie-sc8180x" for sc8180x
 			- "qcom,pcie-sdm845" for sdm845
 			- "qcom,pcie-sm8250" for sm8250
+			- "qcom,pcie-sm8450-pcie0" for PCIe0 on sm8450
+			- "qcom,pcie-sm8450-pcie1" for PCIe1 on sm8450
 			- "qcom,pcie-ipq6018" for ipq6018
 
 - reg:
@@ -169,6 +171,24 @@
 			- "ddrss_sf_tbu" PCIe SF TBU clock
 			- "pipe"	PIPE clock
 
+- clock-names:
+	Usage: required for sm8450-pcie0 and sm8450-pcie1
+	Value type: <stringlist>
+	Definition: Should contain the following entries
+			- "aux"         Auxiliary clock
+			- "cfg"         Configuration clock
+			- "bus_master"  Master AXI clock
+			- "bus_slave"   Slave AXI clock
+			- "slave_q2a"   Slave Q2A clock
+			- "tbu"         PCIe TBU clock
+			- "ddrss_sf_tbu" PCIe SF TBU clock
+			- "pipe"        PIPE clock
+			- "pipe_mux"    PIPE MUX
+			- "phy_pipe"    PIPE output clock
+			- "ref"         REFERENCE clock
+			- "aggre0"	Aggre NoC PCIe0 AXI clock, only for sm8450-pcie0
+			- "aggre1"	Aggre NoC PCIe1 AXI clock
+
 - resets:
 	Usage: required
 	Value type: <prop-encoded-array>
@@ -246,7 +266,7 @@
 			- "ahb"			AHB reset
 
 - reset-names:
-	Usage: required for sc8180x, sdm845 and sm8250
+	Usage: required for sc8180x, sdm845, sm8250 and sm8450
 	Value type: <stringlist>
 	Definition: Should contain the following entries
 			- "pci"			PCIe core reset
-- 
2.34.1

Re: [PATCH v5 1/5] dt-bindings: pci: qcom: Document PCIe bindings for SM8450

[Rob Herring]

On Sat, Dec 18, 2021 at 05:10:20PM +0300, Dmitry Baryshkov wrote:
> Document the PCIe DT bindings for SM8450 SoC. The PCIe IP is similar
> to the one used on SM8250, however unlike SM8250, PCIe0 and PCIe1 use
> different set of clocks, so two compatible entries are required.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  .../devicetree/bindings/pci/qcom,pcie.txt     | 22 ++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> index a0ae024c2d0c..0adb56d5645e 100644
> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> @@ -15,6 +15,8 @@
>  			- "qcom,pcie-sc8180x" for sc8180x
>  			- "qcom,pcie-sdm845" for sdm845
>  			- "qcom,pcie-sm8250" for sm8250
> +			- "qcom,pcie-sm8450-pcie0" for PCIe0 on sm8450
> +			- "qcom,pcie-sm8450-pcie1" for PCIe1 on sm8450

What's the difference between the two?

>  			- "qcom,pcie-ipq6018" for ipq6018
>  
>  - reg:
> @@ -169,6 +171,24 @@
>  			- "ddrss_sf_tbu" PCIe SF TBU clock
>  			- "pipe"	PIPE clock
>  
> +- clock-names:
> +	Usage: required for sm8450-pcie0 and sm8450-pcie1
> +	Value type: <stringlist>
> +	Definition: Should contain the following entries
> +			- "aux"         Auxiliary clock
> +			- "cfg"         Configuration clock
> +			- "bus_master"  Master AXI clock
> +			- "bus_slave"   Slave AXI clock
> +			- "slave_q2a"   Slave Q2A clock
> +			- "tbu"         PCIe TBU clock
> +			- "ddrss_sf_tbu" PCIe SF TBU clock
> +			- "pipe"        PIPE clock
> +			- "pipe_mux"    PIPE MUX
> +			- "phy_pipe"    PIPE output clock
> +			- "ref"         REFERENCE clock
> +			- "aggre0"	Aggre NoC PCIe0 AXI clock, only for sm8450-pcie0
> +			- "aggre1"	Aggre NoC PCIe1 AXI clock
> +
>  - resets:
>  	Usage: required
>  	Value type: <prop-encoded-array>
> @@ -246,7 +266,7 @@
>  			- "ahb"			AHB reset
>  
>  - reset-names:
> -	Usage: required for sc8180x, sdm845 and sm8250
> +	Usage: required for sc8180x, sdm845, sm8250 and sm8450
>  	Value type: <stringlist>
>  	Definition: Should contain the following entries
>  			- "pci"			PCIe core reset
> -- 
> 2.34.1
> 
> 

Re: [PATCH v5 1/5] dt-bindings: pci: qcom: Document PCIe bindings for SM8450

[Dmitry Baryshkov]

On Tue, 21 Dec 2021 at 17:59, Rob Herring <robh@kernel.org> wrote:
>
> On Sat, Dec 18, 2021 at 05:10:20PM +0300, Dmitry Baryshkov wrote:
> > Document the PCIe DT bindings for SM8450 SoC. The PCIe IP is similar
> > to the one used on SM8250, however unlike SM8250, PCIe0 and PCIe1 use
> > different set of clocks, so two compatible entries are required.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  .../devicetree/bindings/pci/qcom,pcie.txt     | 22 ++++++++++++++++++-
> >  1 file changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> > index a0ae024c2d0c..0adb56d5645e 100644
> > --- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> > +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> > @@ -15,6 +15,8 @@
> >                       - "qcom,pcie-sc8180x" for sc8180x
> >                       - "qcom,pcie-sdm845" for sdm845
> >                       - "qcom,pcie-sm8250" for sm8250
> > +                     - "qcom,pcie-sm8450-pcie0" for PCIe0 on sm8450
> > +                     - "qcom,pcie-sm8450-pcie1" for PCIe1 on sm8450
>
> What's the difference between the two?

Clocks used by these hosts. Quoting the definition:

+                     - "aggre0"      Aggre NoC PCIe0 AXI clock, only
for sm8450-pcie0
+                     - "aggre1"      Aggre NoC PCIe1 AXI clock

aggre1 is used by both pcie0 and pcie1, while aggre0 is used only by pcie0.

>
> >                       - "qcom,pcie-ipq6018" for ipq6018
> >
> >  - reg:
> > @@ -169,6 +171,24 @@
> >                       - "ddrss_sf_tbu" PCIe SF TBU clock
> >                       - "pipe"        PIPE clock
> >
> > +- clock-names:
> > +     Usage: required for sm8450-pcie0 and sm8450-pcie1
> > +     Value type: <stringlist>
> > +     Definition: Should contain the following entries
> > +                     - "aux"         Auxiliary clock
> > +                     - "cfg"         Configuration clock
> > +                     - "bus_master"  Master AXI clock
> > +                     - "bus_slave"   Slave AXI clock
> > +                     - "slave_q2a"   Slave Q2A clock
> > +                     - "tbu"         PCIe TBU clock
> > +                     - "ddrss_sf_tbu" PCIe SF TBU clock
> > +                     - "pipe"        PIPE clock
> > +                     - "pipe_mux"    PIPE MUX
> > +                     - "phy_pipe"    PIPE output clock
> > +                     - "ref"         REFERENCE clock
> > +                     - "aggre0"      Aggre NoC PCIe0 AXI clock, only for sm8450-pcie0
> > +                     - "aggre1"      Aggre NoC PCIe1 AXI clock
> > +
> >  - resets:
> >       Usage: required
> >       Value type: <prop-encoded-array>
> > @@ -246,7 +266,7 @@
> >                       - "ahb"                 AHB reset
> >
> >  - reset-names:
> > -     Usage: required for sc8180x, sdm845 and sm8250
> > +     Usage: required for sc8180x, sdm845, sm8250 and sm8450
> >       Value type: <stringlist>
> >       Definition: Should contain the following entries
> >                       - "pci"                 PCIe core reset
> > --
> > 2.34.1
> >
> >



-- 
With best wishes
Dmitry

Re: [PATCH v5 1/5] dt-bindings: pci: qcom: Document PCIe bindings for SM8450

[Rob Herring]

On Tue, Dec 21, 2021 at 06:43:31PM +0300, Dmitry Baryshkov wrote:
> On Tue, 21 Dec 2021 at 17:59, Rob Herring <robh@kernel.org> wrote:
> >
> > On Sat, Dec 18, 2021 at 05:10:20PM +0300, Dmitry Baryshkov wrote:
> > > Document the PCIe DT bindings for SM8450 SoC. The PCIe IP is similar
> > > to the one used on SM8250, however unlike SM8250, PCIe0 and PCIe1 use
> > > different set of clocks, so two compatible entries are required.
> > >
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > ---
> > >  .../devicetree/bindings/pci/qcom,pcie.txt     | 22 ++++++++++++++++++-
> > >  1 file changed, 21 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> > > index a0ae024c2d0c..0adb56d5645e 100644
> > > --- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> > > +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> > > @@ -15,6 +15,8 @@
> > >                       - "qcom,pcie-sc8180x" for sc8180x
> > >                       - "qcom,pcie-sdm845" for sdm845
> > >                       - "qcom,pcie-sm8250" for sm8250
> > > +                     - "qcom,pcie-sm8450-pcie0" for PCIe0 on sm8450
> > > +                     - "qcom,pcie-sm8450-pcie1" for PCIe1 on sm8450
> >
> > What's the difference between the two?
> 
> Clocks used by these hosts. Quoting the definition:
> 
> +                     - "aggre0"      Aggre NoC PCIe0 AXI clock, only
> for sm8450-pcie0
> +                     - "aggre1"      Aggre NoC PCIe1 AXI clock
> 
> aggre1 is used by both pcie0 and pcie1, while aggre0 is used only by pcie0.

That doesn't really seem like you need a different compatible for that. 
Do you need to handle them differently? It seems like abuse of clocks 
putting bus/interconnect clocks here, but sadly that's all too common.

Rob

Re: [PATCH v5 1/5] dt-bindings: pci: qcom: Document PCIe bindings for SM8450

[Dmitry Baryshkov]

On Tue, 21 Dec 2021 at 22:52, Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Dec 21, 2021 at 06:43:31PM +0300, Dmitry Baryshkov wrote:
> > On Tue, 21 Dec 2021 at 17:59, Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Sat, Dec 18, 2021 at 05:10:20PM +0300, Dmitry Baryshkov wrote:
> > > > Document the PCIe DT bindings for SM8450 SoC. The PCIe IP is similar
> > > > to the one used on SM8250, however unlike SM8250, PCIe0 and PCIe1 use
> > > > different set of clocks, so two compatible entries are required.
> > > >
> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > ---
> > > >  .../devicetree/bindings/pci/qcom,pcie.txt     | 22 ++++++++++++++++++-
> > > >  1 file changed, 21 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> > > > index a0ae024c2d0c..0adb56d5645e 100644
> > > > --- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> > > > +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> > > > @@ -15,6 +15,8 @@
> > > >                       - "qcom,pcie-sc8180x" for sc8180x
> > > >                       - "qcom,pcie-sdm845" for sdm845
> > > >                       - "qcom,pcie-sm8250" for sm8250
> > > > +                     - "qcom,pcie-sm8450-pcie0" for PCIe0 on sm8450
> > > > +                     - "qcom,pcie-sm8450-pcie1" for PCIe1 on sm8450
> > >
> > > What's the difference between the two?
> >
> > Clocks used by these hosts. Quoting the definition:
> >
> > +                     - "aggre0"      Aggre NoC PCIe0 AXI clock, only
> > for sm8450-pcie0
> > +                     - "aggre1"      Aggre NoC PCIe1 AXI clock
> >
> > aggre1 is used by both pcie0 and pcie1, while aggre0 is used only by pcie0.
>
> That doesn't really seem like you need a different compatible for that.
> Do you need to handle them differently? It seems like abuse of clocks
> putting bus/interconnect clocks here, but sadly that's all too common.

Unfortunately, yes, it looks like we need to handle them differently.
The clocks are not handled by the interconnect on their own.

-- 
With best wishes
Dmitry

Re: [PATCH v5 1/5] dt-bindings: pci: qcom: Document PCIe bindings for SM8450

[Rob Herring]

On Sat, 18 Dec 2021 17:10:20 +0300, Dmitry Baryshkov wrote:
> Document the PCIe DT bindings for SM8450 SoC. The PCIe IP is similar
> to the one used on SM8250, however unlike SM8250, PCIe0 and PCIe1 use
> different set of clocks, so two compatible entries are required.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  .../devicetree/bindings/pci/qcom,pcie.txt     | 22 ++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v5 1/5] dt-bindings: pci: qcom: Document PCIe bindings for SM8450

[Bjorn Andersson]

On Sat 18 Dec 06:10 PST 2021, Dmitry Baryshkov wrote:

> Document the PCIe DT bindings for SM8450 SoC. The PCIe IP is similar
> to the one used on SM8250, however unlike SM8250, PCIe0 and PCIe1 use
> different set of clocks, so two compatible entries are required.
> 

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  .../devicetree/bindings/pci/qcom,pcie.txt     | 22 ++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> index a0ae024c2d0c..0adb56d5645e 100644
> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> @@ -15,6 +15,8 @@
>  			- "qcom,pcie-sc8180x" for sc8180x
>  			- "qcom,pcie-sdm845" for sdm845
>  			- "qcom,pcie-sm8250" for sm8250
> +			- "qcom,pcie-sm8450-pcie0" for PCIe0 on sm8450
> +			- "qcom,pcie-sm8450-pcie1" for PCIe1 on sm8450
>  			- "qcom,pcie-ipq6018" for ipq6018
>  
>  - reg:
> @@ -169,6 +171,24 @@
>  			- "ddrss_sf_tbu" PCIe SF TBU clock
>  			- "pipe"	PIPE clock
>  
> +- clock-names:
> +	Usage: required for sm8450-pcie0 and sm8450-pcie1
> +	Value type: <stringlist>
> +	Definition: Should contain the following entries
> +			- "aux"         Auxiliary clock
> +			- "cfg"         Configuration clock
> +			- "bus_master"  Master AXI clock
> +			- "bus_slave"   Slave AXI clock
> +			- "slave_q2a"   Slave Q2A clock
> +			- "tbu"         PCIe TBU clock
> +			- "ddrss_sf_tbu" PCIe SF TBU clock
> +			- "pipe"        PIPE clock
> +			- "pipe_mux"    PIPE MUX
> +			- "phy_pipe"    PIPE output clock
> +			- "ref"         REFERENCE clock
> +			- "aggre0"	Aggre NoC PCIe0 AXI clock, only for sm8450-pcie0
> +			- "aggre1"	Aggre NoC PCIe1 AXI clock
> +
>  - resets:
>  	Usage: required
>  	Value type: <prop-encoded-array>
> @@ -246,7 +266,7 @@
>  			- "ahb"			AHB reset
>  
>  - reset-names:
> -	Usage: required for sc8180x, sdm845 and sm8250
> +	Usage: required for sc8180x, sdm845, sm8250 and sm8450
>  	Value type: <stringlist>
>  	Definition: Should contain the following entries
>  			- "pci"			PCIe core reset
> -- 
> 2.34.1
> 

[PATCH v5 2/5] PCI: qcom: Remove redundancy between qcom_pcie and qcom_pcie_cfg

[Dmitry Baryshkov]

In preparation to adding more flags to configuration data, use pointer
to struct qcom_pcie_cfg directly inside struct qcom_pcie, rather than
duplicating all its fields. This would save us from the boilerplate code
that just copies flag values from one struct to another one.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 31 +++++++++++---------------
 1 file changed, 13 insertions(+), 18 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 4e668da96ef4..1204011c96ee 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -200,8 +200,7 @@ struct qcom_pcie {
 	union qcom_pcie_resources res;
 	struct phy *phy;
 	struct gpio_desc *reset;
-	const struct qcom_pcie_ops *ops;
-	unsigned int pipe_clk_need_muxing:1;
+	const struct qcom_pcie_cfg *cfg;
 };
 
 #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)
@@ -225,8 +224,8 @@ static int qcom_pcie_start_link(struct dw_pcie *pci)
 	struct qcom_pcie *pcie = to_qcom_pcie(pci);
 
 	/* Enable Link Training state machine */
-	if (pcie->ops->ltssm_enable)
-		pcie->ops->ltssm_enable(pcie);
+	if (pcie->cfg->ops->ltssm_enable)
+		pcie->cfg->ops->ltssm_enable(pcie);
 
 	return 0;
 }
@@ -1145,7 +1144,7 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
 	if (ret < 0)
 		return ret;
 
-	if (pcie->pipe_clk_need_muxing) {
+	if (pcie->cfg->pipe_clk_need_muxing) {
 		res->pipe_clk_src = devm_clk_get(dev, "pipe_mux");
 		if (IS_ERR(res->pipe_clk_src))
 			return PTR_ERR(res->pipe_clk_src);
@@ -1180,7 +1179,7 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
 	}
 
 	/* Set pipe clock as clock source for pcie_pipe_clk_src */
-	if (pcie->pipe_clk_need_muxing)
+	if (pcie->cfg->pipe_clk_need_muxing)
 		clk_set_parent(res->pipe_clk_src, res->phy_pipe_clk);
 
 	ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
@@ -1243,7 +1242,7 @@ static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
 	clk_bulk_disable_unprepare(res->num_clks, res->clks);
 
 	/* Set TCXO as clock source for pcie_pipe_clk_src */
-	if (pcie->pipe_clk_need_muxing)
+	if (pcie->cfg->pipe_clk_need_muxing)
 		clk_set_parent(res->pipe_clk_src, res->ref_clk_src);
 
 	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
@@ -1336,7 +1335,7 @@ static int qcom_pcie_host_init(struct pcie_port *pp)
 
 	qcom_ep_reset_assert(pcie);
 
-	ret = pcie->ops->init(pcie);
+	ret = pcie->cfg->ops->init(pcie);
 	if (ret)
 		return ret;
 
@@ -1346,8 +1345,8 @@ static int qcom_pcie_host_init(struct pcie_port *pp)
 
 	qcom_ep_reset_deassert(pcie);
 
-	if (pcie->ops->config_sid) {
-		ret = pcie->ops->config_sid(pcie);
+	if (pcie->cfg->ops->config_sid) {
+		ret = pcie->cfg->ops->config_sid(pcie);
 		if (ret)
 			goto err;
 	}
@@ -1358,7 +1357,7 @@ static int qcom_pcie_host_init(struct pcie_port *pp)
 	qcom_ep_reset_assert(pcie);
 	phy_power_off(pcie->phy);
 err_deinit:
-	pcie->ops->deinit(pcie);
+	pcie->cfg->ops->deinit(pcie);
 
 	return ret;
 }
@@ -1468,7 +1467,6 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 	struct pcie_port *pp;
 	struct dw_pcie *pci;
 	struct qcom_pcie *pcie;
-	const struct qcom_pcie_cfg *pcie_cfg;
 	int ret;
 
 	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
@@ -1485,15 +1483,12 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 
 	pcie->pci = pci;
 
-	pcie_cfg = of_device_get_match_data(dev);
-	if (!pcie_cfg || !pcie_cfg->ops) {
+	pcie->cfg = of_device_get_match_data(dev);
+	if (!pcie->cfg || !pcie->cfg->ops) {
 		dev_err(dev, "Invalid platform data\n");
 		return -EINVAL;
 	}
 
-	pcie->ops = pcie_cfg->ops;
-	pcie->pipe_clk_need_muxing = pcie_cfg->pipe_clk_need_muxing;
-
 	pcie->reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
 	if (IS_ERR(pcie->reset))
 		return PTR_ERR(pcie->reset);
@@ -1510,7 +1505,7 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 	if (IS_ERR(pcie->phy))
 		return PTR_ERR(pcie->phy);
 
-	ret = pcie->ops->get_resources(pcie);
+	ret = pcie->cfg->ops->get_resources(pcie);
 	if (ret)
 		return ret;
 
-- 
2.34.1

Re: [PATCH v5 2/5] PCI: qcom: Remove redundancy between qcom_pcie and qcom_pcie_cfg

[Bjorn Andersson]

On Sat 18 Dec 06:10 PST 2021, Dmitry Baryshkov wrote:

> In preparation to adding more flags to configuration data, use pointer
> to struct qcom_pcie_cfg directly inside struct qcom_pcie, rather than
> duplicating all its fields. This would save us from the boilerplate code
> that just copies flag values from one struct to another one.
> 

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 31 +++++++++++---------------
>  1 file changed, 13 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 4e668da96ef4..1204011c96ee 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -200,8 +200,7 @@ struct qcom_pcie {
>  	union qcom_pcie_resources res;
>  	struct phy *phy;
>  	struct gpio_desc *reset;
> -	const struct qcom_pcie_ops *ops;
> -	unsigned int pipe_clk_need_muxing:1;
> +	const struct qcom_pcie_cfg *cfg;
>  };
>  
>  #define to_qcom_pcie(x)		dev_get_drvdata((x)->dev)
> @@ -225,8 +224,8 @@ static int qcom_pcie_start_link(struct dw_pcie *pci)
>  	struct qcom_pcie *pcie = to_qcom_pcie(pci);
>  
>  	/* Enable Link Training state machine */
> -	if (pcie->ops->ltssm_enable)
> -		pcie->ops->ltssm_enable(pcie);
> +	if (pcie->cfg->ops->ltssm_enable)
> +		pcie->cfg->ops->ltssm_enable(pcie);
>  
>  	return 0;
>  }
> @@ -1145,7 +1144,7 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
>  	if (ret < 0)
>  		return ret;
>  
> -	if (pcie->pipe_clk_need_muxing) {
> +	if (pcie->cfg->pipe_clk_need_muxing) {
>  		res->pipe_clk_src = devm_clk_get(dev, "pipe_mux");
>  		if (IS_ERR(res->pipe_clk_src))
>  			return PTR_ERR(res->pipe_clk_src);
> @@ -1180,7 +1179,7 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
>  	}
>  
>  	/* Set pipe clock as clock source for pcie_pipe_clk_src */
> -	if (pcie->pipe_clk_need_muxing)
> +	if (pcie->cfg->pipe_clk_need_muxing)
>  		clk_set_parent(res->pipe_clk_src, res->phy_pipe_clk);
>  
>  	ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
> @@ -1243,7 +1242,7 @@ static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
>  	clk_bulk_disable_unprepare(res->num_clks, res->clks);
>  
>  	/* Set TCXO as clock source for pcie_pipe_clk_src */
> -	if (pcie->pipe_clk_need_muxing)
> +	if (pcie->cfg->pipe_clk_need_muxing)
>  		clk_set_parent(res->pipe_clk_src, res->ref_clk_src);
>  
>  	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
> @@ -1336,7 +1335,7 @@ static int qcom_pcie_host_init(struct pcie_port *pp)
>  
>  	qcom_ep_reset_assert(pcie);
>  
> -	ret = pcie->ops->init(pcie);
> +	ret = pcie->cfg->ops->init(pcie);
>  	if (ret)
>  		return ret;
>  
> @@ -1346,8 +1345,8 @@ static int qcom_pcie_host_init(struct pcie_port *pp)
>  
>  	qcom_ep_reset_deassert(pcie);
>  
> -	if (pcie->ops->config_sid) {
> -		ret = pcie->ops->config_sid(pcie);
> +	if (pcie->cfg->ops->config_sid) {
> +		ret = pcie->cfg->ops->config_sid(pcie);
>  		if (ret)
>  			goto err;
>  	}
> @@ -1358,7 +1357,7 @@ static int qcom_pcie_host_init(struct pcie_port *pp)
>  	qcom_ep_reset_assert(pcie);
>  	phy_power_off(pcie->phy);
>  err_deinit:
> -	pcie->ops->deinit(pcie);
> +	pcie->cfg->ops->deinit(pcie);
>  
>  	return ret;
>  }
> @@ -1468,7 +1467,6 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  	struct pcie_port *pp;
>  	struct dw_pcie *pci;
>  	struct qcom_pcie *pcie;
> -	const struct qcom_pcie_cfg *pcie_cfg;
>  	int ret;
>  
>  	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
> @@ -1485,15 +1483,12 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  
>  	pcie->pci = pci;
>  
> -	pcie_cfg = of_device_get_match_data(dev);
> -	if (!pcie_cfg || !pcie_cfg->ops) {
> +	pcie->cfg = of_device_get_match_data(dev);
> +	if (!pcie->cfg || !pcie->cfg->ops) {
>  		dev_err(dev, "Invalid platform data\n");
>  		return -EINVAL;
>  	}
>  
> -	pcie->ops = pcie_cfg->ops;
> -	pcie->pipe_clk_need_muxing = pcie_cfg->pipe_clk_need_muxing;
> -
>  	pcie->reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
>  	if (IS_ERR(pcie->reset))
>  		return PTR_ERR(pcie->reset);
> @@ -1510,7 +1505,7 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  	if (IS_ERR(pcie->phy))
>  		return PTR_ERR(pcie->phy);
>  
> -	ret = pcie->ops->get_resources(pcie);
> +	ret = pcie->cfg->ops->get_resources(pcie);
>  	if (ret)
>  		return ret;
>  
> -- 
> 2.34.1
> 

[PATCH v5 3/5] PCI: qcom: Add ddrss_sf_tbu flag

[Dmitry Baryshkov]

Qualcomm PCIe driver uses compatible string to check if the ddrss_sf_tbu
clock should be used. Since sc7280 support has added flags, switch to
the new mechanism to check if this clock should be used.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 1204011c96ee..d8d400423a0a 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -191,6 +191,7 @@ struct qcom_pcie_ops {
 struct qcom_pcie_cfg {
 	const struct qcom_pcie_ops *ops;
 	unsigned int pipe_clk_need_muxing:1;
+	unsigned int has_ddrss_sf_tbu_clk:1;
 };
 
 struct qcom_pcie {
@@ -1133,7 +1134,7 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
 	res->clks[3].id = "bus_slave";
 	res->clks[4].id = "slave_q2a";
 	res->clks[5].id = "tbu";
-	if (of_device_is_compatible(dev->of_node, "qcom,pcie-sm8250")) {
+	if (pcie->cfg->has_ddrss_sf_tbu_clk) {
 		res->clks[6].id = "ddrss_sf_tbu";
 		res->num_clks = 7;
 	} else {
@@ -1449,6 +1450,7 @@ static const struct qcom_pcie_cfg sdm845_cfg = {
 
 static const struct qcom_pcie_cfg sm8250_cfg = {
 	.ops = &ops_1_9_0,
+	.has_ddrss_sf_tbu_clk = true,
 };
 
 static const struct qcom_pcie_cfg sc7280_cfg = {
-- 
2.34.1

Re: [PATCH v5 3/5] PCI: qcom: Add ddrss_sf_tbu flag

[Bjorn Andersson]

On Sat 18 Dec 06:10 PST 2021, Dmitry Baryshkov wrote:

> Qualcomm PCIe driver uses compatible string to check if the ddrss_sf_tbu
> clock should be used. Since sc7280 support has added flags, switch to
> the new mechanism to check if this clock should be used.
> 

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 1204011c96ee..d8d400423a0a 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -191,6 +191,7 @@ struct qcom_pcie_ops {
>  struct qcom_pcie_cfg {
>  	const struct qcom_pcie_ops *ops;
>  	unsigned int pipe_clk_need_muxing:1;
> +	unsigned int has_ddrss_sf_tbu_clk:1;
>  };
>  
>  struct qcom_pcie {
> @@ -1133,7 +1134,7 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
>  	res->clks[3].id = "bus_slave";
>  	res->clks[4].id = "slave_q2a";
>  	res->clks[5].id = "tbu";
> -	if (of_device_is_compatible(dev->of_node, "qcom,pcie-sm8250")) {
> +	if (pcie->cfg->has_ddrss_sf_tbu_clk) {
>  		res->clks[6].id = "ddrss_sf_tbu";
>  		res->num_clks = 7;
>  	} else {
> @@ -1449,6 +1450,7 @@ static const struct qcom_pcie_cfg sdm845_cfg = {
>  
>  static const struct qcom_pcie_cfg sm8250_cfg = {
>  	.ops = &ops_1_9_0,
> +	.has_ddrss_sf_tbu_clk = true,
>  };
>  
>  static const struct qcom_pcie_cfg sc7280_cfg = {
> -- 
> 2.34.1
> 

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

[Dmitry Baryshkov]

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;
 };
 
 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

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

[Bjorn Andersson]

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
> 

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

[Dmitry Baryshkov]

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
>>


-- 
With best wishes
Dmitry

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

[Lorenzo Pieralisi]

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

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

[Bjorn Andersson]

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

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

[Lorenzo Pieralisi]

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

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

[Dmitry Baryshkov]

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.

-- 
With best wishes
Dmitry

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

[Bjorn Andersson]

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

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

[Bjorn Andersson]

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
> 

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

[Dmitry Baryshkov]

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
>>


-- 
With best wishes
Dmitry

[PATCH v5 5/5] PCI: qcom: Add SM8450 PCIe support

[Dmitry Baryshkov]

On SM8450 platform PCIe hosts do not use all the clocks (and add several
additional clocks), so expand the driver to handle these requirements.

PCIe0 and PCIe1 hosts use different sets of clocks, so separate entries
are required.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 57 ++++++++++++++++++++------
 1 file changed, 44 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 55ac3caa6d7d..fe6ed1e0415a 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -161,7 +161,7 @@ struct qcom_pcie_resources_2_3_3 {
 
 /* 6 clocks typically, 7 for sm8250 */
 struct qcom_pcie_resources_2_7_0 {
-	struct clk_bulk_data clks[7];
+	struct clk_bulk_data clks[9];
 	int num_clks;
 	struct regulator_bulk_data supplies[2];
 	struct reset_control *pci_reset;
@@ -193,7 +193,10 @@ struct qcom_pcie_ops {
 struct qcom_pcie_cfg {
 	const struct qcom_pcie_ops *ops;
 	unsigned int pipe_clk_need_muxing:1;
+	unsigned int has_tbu_clk:1;
 	unsigned int has_ddrss_sf_tbu_clk:1;
+	unsigned int has_aggre0_clk:1;
+	unsigned int has_aggre1_clk:1;
 };
 
 struct qcom_pcie {
@@ -1117,6 +1120,7 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
 	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
 	struct dw_pcie *pci = pcie->pci;
 	struct device *dev = pci->dev;
+	unsigned int idx;
 	int ret;
 
 	res->pci_reset = devm_reset_control_get_exclusive(dev, "pci");
@@ -1134,18 +1138,22 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
 	if (ret)
 		return ret;
 
-	res->clks[0].id = "aux";
-	res->clks[1].id = "cfg";
-	res->clks[2].id = "bus_master";
-	res->clks[3].id = "bus_slave";
-	res->clks[4].id = "slave_q2a";
-	res->clks[5].id = "tbu";
-	if (pcie->cfg->has_ddrss_sf_tbu_clk) {
-		res->clks[6].id = "ddrss_sf_tbu";
-		res->num_clks = 7;
-	} else {
-		res->num_clks = 6;
-	}
+	idx = 0;
+	res->clks[idx++].id = "aux";
+	res->clks[idx++].id = "cfg";
+	res->clks[idx++].id = "bus_master";
+	res->clks[idx++].id = "bus_slave";
+	res->clks[idx++].id = "slave_q2a";
+	if (pcie->cfg->has_tbu_clk)
+		res->clks[idx++].id = "tbu";
+	if (pcie->cfg->has_ddrss_sf_tbu_clk)
+		res->clks[idx++].id = "ddrss_sf_tbu";
+	if (pcie->cfg->has_aggre0_clk)
+		res->clks[idx++].id = "aggre0";
+	if (pcie->cfg->has_aggre1_clk)
+		res->clks[idx++].id = "aggre1";
+
+	res->num_clks = idx;
 
 	ret = devm_clk_bulk_get(dev, res->num_clks, res->clks);
 	if (ret < 0)
@@ -1210,6 +1218,9 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
 		goto err_disable_clocks;
 	}
 
+	/* Wait for reset to complete, required on SM8450 */
+	usleep_range(1000, 1500);
+
 	/* configure PCIe to RC mode */
 	writel(DEVICE_TYPE_RC, pcie->parf + PCIE20_PARF_DEVICE_TYPE);
 
@@ -1457,15 +1468,33 @@ static const struct qcom_pcie_cfg ipq4019_cfg = {
 
 static const struct qcom_pcie_cfg sdm845_cfg = {
 	.ops = &ops_2_7_0,
+	.has_tbu_clk = true,
 };
 
 static const struct qcom_pcie_cfg sm8250_cfg = {
+	.ops = &ops_1_9_0,
+	.has_tbu_clk = true,
+	.has_ddrss_sf_tbu_clk = true,
+};
+
+static const struct qcom_pcie_cfg sm8450_pcie0_cfg = {
 	.ops = &ops_1_9_0,
 	.has_ddrss_sf_tbu_clk = true,
+	.pipe_clk_need_muxing = true,
+	.has_aggre0_clk = true,
+	.has_aggre1_clk = true,
+};
+
+static const struct qcom_pcie_cfg sm8450_pcie1_cfg = {
+	.ops = &ops_1_9_0,
+	.has_ddrss_sf_tbu_clk = true,
+	.pipe_clk_need_muxing = true,
+	.has_aggre1_clk = true,
 };
 
 static const struct qcom_pcie_cfg sc7280_cfg = {
 	.ops = &ops_1_9_0,
+	.has_tbu_clk = true,
 	.pipe_clk_need_muxing = true,
 };
 
@@ -1564,6 +1593,8 @@ static const struct of_device_id qcom_pcie_match[] = {
 	{ .compatible = "qcom,pcie-sdm845", .data = &sdm845_cfg },
 	{ .compatible = "qcom,pcie-sm8250", .data = &sm8250_cfg },
 	{ .compatible = "qcom,pcie-sc8180x", .data = &sm8250_cfg },
+	{ .compatible = "qcom,pcie-sm8450-pcie0", .data = &sm8450_pcie0_cfg },
+	{ .compatible = "qcom,pcie-sm8450-pcie1", .data = &sm8450_pcie1_cfg },
 	{ .compatible = "qcom,pcie-sc7280", .data = &sc7280_cfg },
 	{ }
 };
-- 
2.34.1

Re: [PATCH v5 5/5] PCI: qcom: Add SM8450 PCIe support

[Bjorn Andersson]

On Sat 18 Dec 06:10 PST 2021, Dmitry Baryshkov wrote:

> On SM8450 platform PCIe hosts do not use all the clocks (and add several
> additional clocks), so expand the driver to handle these requirements.
> 
> PCIe0 and PCIe1 hosts use different sets of clocks, so separate entries
> are required.
> 

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 57 ++++++++++++++++++++------
>  1 file changed, 44 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 55ac3caa6d7d..fe6ed1e0415a 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -161,7 +161,7 @@ struct qcom_pcie_resources_2_3_3 {
>  
>  /* 6 clocks typically, 7 for sm8250 */
>  struct qcom_pcie_resources_2_7_0 {
> -	struct clk_bulk_data clks[7];
> +	struct clk_bulk_data clks[9];
>  	int num_clks;
>  	struct regulator_bulk_data supplies[2];
>  	struct reset_control *pci_reset;
> @@ -193,7 +193,10 @@ struct qcom_pcie_ops {
>  struct qcom_pcie_cfg {
>  	const struct qcom_pcie_ops *ops;
>  	unsigned int pipe_clk_need_muxing:1;
> +	unsigned int has_tbu_clk:1;
>  	unsigned int has_ddrss_sf_tbu_clk:1;
> +	unsigned int has_aggre0_clk:1;
> +	unsigned int has_aggre1_clk:1;
>  };
>  
>  struct qcom_pcie {
> @@ -1117,6 +1120,7 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
>  	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
>  	struct dw_pcie *pci = pcie->pci;
>  	struct device *dev = pci->dev;
> +	unsigned int idx;
>  	int ret;
>  
>  	res->pci_reset = devm_reset_control_get_exclusive(dev, "pci");
> @@ -1134,18 +1138,22 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
>  	if (ret)
>  		return ret;
>  
> -	res->clks[0].id = "aux";
> -	res->clks[1].id = "cfg";
> -	res->clks[2].id = "bus_master";
> -	res->clks[3].id = "bus_slave";
> -	res->clks[4].id = "slave_q2a";
> -	res->clks[5].id = "tbu";
> -	if (pcie->cfg->has_ddrss_sf_tbu_clk) {
> -		res->clks[6].id = "ddrss_sf_tbu";
> -		res->num_clks = 7;
> -	} else {
> -		res->num_clks = 6;
> -	}
> +	idx = 0;
> +	res->clks[idx++].id = "aux";
> +	res->clks[idx++].id = "cfg";
> +	res->clks[idx++].id = "bus_master";
> +	res->clks[idx++].id = "bus_slave";
> +	res->clks[idx++].id = "slave_q2a";
> +	if (pcie->cfg->has_tbu_clk)
> +		res->clks[idx++].id = "tbu";
> +	if (pcie->cfg->has_ddrss_sf_tbu_clk)
> +		res->clks[idx++].id = "ddrss_sf_tbu";
> +	if (pcie->cfg->has_aggre0_clk)
> +		res->clks[idx++].id = "aggre0";
> +	if (pcie->cfg->has_aggre1_clk)
> +		res->clks[idx++].id = "aggre1";
> +
> +	res->num_clks = idx;
>  
>  	ret = devm_clk_bulk_get(dev, res->num_clks, res->clks);
>  	if (ret < 0)
> @@ -1210,6 +1218,9 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
>  		goto err_disable_clocks;
>  	}
>  
> +	/* Wait for reset to complete, required on SM8450 */
> +	usleep_range(1000, 1500);
> +
>  	/* configure PCIe to RC mode */
>  	writel(DEVICE_TYPE_RC, pcie->parf + PCIE20_PARF_DEVICE_TYPE);
>  
> @@ -1457,15 +1468,33 @@ static const struct qcom_pcie_cfg ipq4019_cfg = {
>  
>  static const struct qcom_pcie_cfg sdm845_cfg = {
>  	.ops = &ops_2_7_0,
> +	.has_tbu_clk = true,
>  };
>  
>  static const struct qcom_pcie_cfg sm8250_cfg = {
> +	.ops = &ops_1_9_0,
> +	.has_tbu_clk = true,
> +	.has_ddrss_sf_tbu_clk = true,
> +};
> +
> +static const struct qcom_pcie_cfg sm8450_pcie0_cfg = {
>  	.ops = &ops_1_9_0,
>  	.has_ddrss_sf_tbu_clk = true,
> +	.pipe_clk_need_muxing = true,
> +	.has_aggre0_clk = true,
> +	.has_aggre1_clk = true,
> +};
> +
> +static const struct qcom_pcie_cfg sm8450_pcie1_cfg = {
> +	.ops = &ops_1_9_0,
> +	.has_ddrss_sf_tbu_clk = true,
> +	.pipe_clk_need_muxing = true,
> +	.has_aggre1_clk = true,
>  };
>  
>  static const struct qcom_pcie_cfg sc7280_cfg = {
>  	.ops = &ops_1_9_0,
> +	.has_tbu_clk = true,
>  	.pipe_clk_need_muxing = true,
>  };
>  
> @@ -1564,6 +1593,8 @@ static const struct of_device_id qcom_pcie_match[] = {
>  	{ .compatible = "qcom,pcie-sdm845", .data = &sdm845_cfg },
>  	{ .compatible = "qcom,pcie-sm8250", .data = &sm8250_cfg },
>  	{ .compatible = "qcom,pcie-sc8180x", .data = &sm8250_cfg },
> +	{ .compatible = "qcom,pcie-sm8450-pcie0", .data = &sm8450_pcie0_cfg },
> +	{ .compatible = "qcom,pcie-sm8450-pcie1", .data = &sm8450_pcie1_cfg },
>  	{ .compatible = "qcom,pcie-sc7280", .data = &sc7280_cfg },
>  	{ }
>  };
> -- 
> 2.34.1
> 

Re: [PATCH v5 0/5] qcom: add support for PCIe on SM8450 platform

[Lorenzo Pieralisi]

On Sat, Dec 18, 2021 at 05:10:19PM +0300, Dmitry Baryshkov wrote:
> There are two different PCIe controllers and PHYs on SM8450, one having
> one lane and another with two lanes. Add support for both PCIe
> controllers
> 
> Dependencies:
>  - https://lore.kernel.org/linux-arm-msm/20211218140223.500390-1-dmitry.baryshkov@linaro.org/
> 
> Changes since v4:
>  - Add PCIe1 support
>  - Change binding accordingly, to use qcom,pcie-sm8450-pcie0 and
>    qcom,pcie-sm8450-pcie1 compatibility strings
>  - Rebase on top of (pending) pipe_clock cleanup/rework patchset
> 
> Changes since v3:
>  - Fix pcie gpios to follow defined schema as noted by Rob
>  - Fix commit message according to Bjorn's suggestions
> 
> Changes since v2:
>  - Remove unnecessary comment in struct qcom_pcie_cfg
> 
> Changes since v1:
>  - Fix capitalization/wording of PCI patch subjects
>  - Add missing gen3x1 specification to PHY table names
> 
> ----------------------------------------------------------------
> Dmitry Baryshkov (5):
>       dt-bindings: pci: qcom: Document PCIe bindings for SM8450
>       PCI: qcom: Remove redundancy between qcom_pcie and qcom_pcie_cfg
>       PCI: qcom: Add ddrss_sf_tbu flag
>       PCI: qcom: Add interconnect support to 2.7.0/1.9.0 ops
>       PCI: qcom: Add SM8450 PCIe support
> 
>  .../devicetree/bindings/pci/qcom,pcie.txt          |  22 ++++-
>  drivers/pci/controller/dwc/pcie-qcom.c             | 101 ++++++++++++++-------
>  2 files changed, 91 insertions(+), 32 deletions(-)

Need an ACK from pci-qcom maintainers, thanks.

Lorenzo