[PATCH 0/3] PCI: qcom: Add sc8180x support

[PATCH 0/3] PCI: qcom: Add sc8180x support

[Bjorn Andersson]

The SC8180x (8cx) platform is used primarily in Windows laptops, the platform
comes with 4 PCIe controllers so far seen being used for NVME storage and SDX55
5G modem. With the PHY already landed in the QMP driver, this adds the
controller support.

Bjorn Andersson (3):
  PCI: qcom: Introduce enable/disable resource ops
  PCI: qcom: Split init and enable for 1.9.0 and 2.7.0
  PCI: qcom: Add sc8180x compatible

 .../devicetree/bindings/pci/qcom,pcie.txt     |  5 +-
 drivers/pci/controller/dwc/pcie-qcom.c        | 74 +++++++++++++++----
 2 files changed, 62 insertions(+), 17 deletions(-)

-- 
2.29.2

[PATCH 1/3] PCI: qcom: Introduce enable/disable resource ops

[Bjorn Andersson]

The current model of doing resource enablement and controller
initialization in a single "init" function invoked after
dw_pcie_host_init() is invoked might result in clocks not being enabled
at the time the "msi" interrupt fires.

One such case happens reliably on the SC8180x (8cx) Snapdragon laptops,
where it's seems like the bootloader touches PCIe and leaves things in a
state that the "msi" interrupt will fire before we have a change to
enable the clocks, resulting in an access of unclocked hardware.

Introduce a two new callbacks, allowing the individual resource handling
functions to be split between enable/init and deinit/disable.

Helper functions for enable, disable and deinit are introduced to handle
the fact that these functions may now be left without implementation.
init is given a wrapper for symmetry.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 42 +++++++++++++++++++++++---
 1 file changed, 38 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 8a7a300163e5..8a64a126de2b 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -181,9 +181,11 @@ struct qcom_pcie;
 
 struct qcom_pcie_ops {
 	int (*get_resources)(struct qcom_pcie *pcie);
+	int (*enable_resources)(struct qcom_pcie *pcie);
 	int (*init)(struct qcom_pcie *pcie);
 	int (*post_init)(struct qcom_pcie *pcie);
 	void (*deinit)(struct qcom_pcie *pcie);
+	void (*disable_resources)(struct qcom_pcie *pcie);
 	void (*post_deinit)(struct qcom_pcie *pcie);
 	void (*ltssm_enable)(struct qcom_pcie *pcie);
 	int (*config_sid)(struct qcom_pcie *pcie);
@@ -1345,6 +1347,31 @@ static int qcom_pcie_config_sid_sm8250(struct qcom_pcie *pcie)
 	return 0;
 }
 
+static int qcom_pcie_enable_resources(struct qcom_pcie *pcie)
+{
+	if (pcie->ops->enable_resources)
+		return pcie->ops->enable_resources(pcie);
+
+	return 0;
+}
+
+static int qcom_pcie_init(struct qcom_pcie *pcie)
+{
+	return pcie->ops->init(pcie);
+}
+
+static void qcom_pcie_deinit(struct qcom_pcie *pcie)
+{
+	if (pcie->ops->deinit)
+		pcie->ops->deinit(pcie);
+}
+
+static void qcom_pcie_disable_resources(struct qcom_pcie *pcie)
+{
+	if (pcie->ops->disable_resources)
+		pcie->ops->disable_resources(pcie);
+}
+
 static int qcom_pcie_host_init(struct pcie_port *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
@@ -1353,7 +1380,7 @@ static int qcom_pcie_host_init(struct pcie_port *pp)
 
 	qcom_ep_reset_assert(pcie);
 
-	ret = pcie->ops->init(pcie);
+	ret = qcom_pcie_init(pcie);
 	if (ret)
 		return ret;
 
@@ -1384,7 +1411,7 @@ static int qcom_pcie_host_init(struct pcie_port *pp)
 err_disable_phy:
 	phy_power_off(pcie->phy);
 err_deinit:
-	pcie->ops->deinit(pcie);
+	qcom_pcie_deinit(pcie);
 
 	return ret;
 }
@@ -1520,10 +1547,14 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 
 	pp->ops = &qcom_pcie_dw_ops;
 
+	ret = qcom_pcie_enable_resources(pcie);
+	if (ret)
+		goto err_pm_runtime_put;
+
 	ret = phy_init(pcie->phy);
 	if (ret) {
 		pm_runtime_disable(&pdev->dev);
-		goto err_pm_runtime_put;
+		goto err_disable_resources;
 	}
 
 	platform_set_drvdata(pdev, pcie);
@@ -1532,11 +1563,14 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 	if (ret) {
 		dev_err(dev, "cannot initialize host\n");
 		pm_runtime_disable(&pdev->dev);
-		goto err_pm_runtime_put;
+		goto err_disable_resources;
 	}
 
 	return 0;
 
+err_disable_resources:
+	qcom_pcie_disable_resources(pcie);
+
 err_pm_runtime_put:
 	pm_runtime_put(dev);
 	pm_runtime_disable(dev);
-- 
2.29.2

[PATCH 2/3] PCI: qcom: Split init and enable for 1.9.0 and 2.7.0

[Bjorn Andersson]

On the sc8180x platform the "msi" interrupt often fires before init has
a chance to enable the clocks that are necessary for the interrupt
handler to access the hardware.

Split out the resource enablement and disablement into the newly
introduce enable/disable resource operations, to ensure that the
necessary resources are enabled when needed.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 31 +++++++++++++++++---------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 8a64a126de2b..8adcbb718832 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1173,12 +1173,11 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
 	return PTR_ERR_OR_ZERO(res->pipe_clk);
 }
 
-static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
+static int qcom_pcie_enable_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;
-	u32 val;
 	int ret;
 
 	ret = regulator_bulk_enable(ARRAY_SIZE(res->supplies), res->supplies);
@@ -1211,6 +1210,20 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
 		goto err_disable_clocks;
 	}
 
+	return 0;
+
+err_disable_clocks:
+	clk_bulk_disable_unprepare(res->num_clks, res->clks);
+err_disable_regulators:
+	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
+
+	return ret;
+}
+
+static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
+{
+	u32 val;
+
 	/* configure PCIe to RC mode */
 	writel(DEVICE_TYPE_RC, pcie->parf + PCIE20_PARF_DEVICE_TYPE);
 
@@ -1238,15 +1251,9 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
 	}
 
 	return 0;
-err_disable_clocks:
-	clk_bulk_disable_unprepare(res->num_clks, res->clks);
-err_disable_regulators:
-	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
-
-	return ret;
 }
 
-static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
+static void qcom_pcie_disable_2_7_0(struct qcom_pcie *pcie)
 {
 	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
 
@@ -1465,8 +1472,9 @@ static const struct qcom_pcie_ops ops_2_3_3 = {
 /* Qcom IP rev.: 2.7.0	Synopsys IP rev.: 4.30a */
 static const struct qcom_pcie_ops ops_2_7_0 = {
 	.get_resources = qcom_pcie_get_resources_2_7_0,
+	.enable_resources = qcom_pcie_enable_2_7_0,
 	.init = qcom_pcie_init_2_7_0,
-	.deinit = qcom_pcie_deinit_2_7_0,
+	.disable_resources = qcom_pcie_disable_2_7_0,
 	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
 	.post_init = qcom_pcie_post_init_2_7_0,
 	.post_deinit = qcom_pcie_post_deinit_2_7_0,
@@ -1475,8 +1483,9 @@ static const struct qcom_pcie_ops ops_2_7_0 = {
 /* Qcom IP rev.: 1.9.0 */
 static const struct qcom_pcie_ops ops_1_9_0 = {
 	.get_resources = qcom_pcie_get_resources_2_7_0,
+	.enable_resources = qcom_pcie_enable_2_7_0,
 	.init = qcom_pcie_init_2_7_0,
-	.deinit = qcom_pcie_deinit_2_7_0,
+	.disable_resources = qcom_pcie_disable_2_7_0,
 	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
 	.post_init = qcom_pcie_post_init_2_7_0,
 	.post_deinit = qcom_pcie_post_deinit_2_7_0,
-- 
2.29.2

[PATCH 3/3] PCI: qcom: Add sc8180x compatible

[Bjorn Andersson]

The SC8180x platform comes with 4 PCIe controllers, typically used for
things such as NVME storage or connecting a SDX55 5G modem. Add a
compatible for this, that just reuses the 1.9.0 ops.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 Documentation/devicetree/bindings/pci/qcom,pcie.txt | 5 +++--
 drivers/pci/controller/dwc/pcie-qcom.c              | 1 +
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
index 3f646875f8c2..a0ae024c2d0c 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
@@ -12,6 +12,7 @@
 			- "qcom,pcie-ipq4019" for ipq4019
 			- "qcom,pcie-ipq8074" for ipq8074
 			- "qcom,pcie-qcs404" for qcs404
+			- "qcom,pcie-sc8180x" for sc8180x
 			- "qcom,pcie-sdm845" for sdm845
 			- "qcom,pcie-sm8250" for sm8250
 			- "qcom,pcie-ipq6018" for ipq6018
@@ -156,7 +157,7 @@
 			- "pipe"	PIPE clock
 
 - clock-names:
-	Usage: required for sm8250
+	Usage: required for sc8180x and sm8250
 	Value type: <stringlist>
 	Definition: Should contain the following entries
 			- "aux"		Auxiliary clock
@@ -245,7 +246,7 @@
 			- "ahb"			AHB reset
 
 - reset-names:
-	Usage: required for sdm845 and sm8250
+	Usage: required for sc8180x, sdm845 and sm8250
 	Value type: <stringlist>
 	Definition: Should contain the following entries
 			- "pci"			PCIe core reset
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 8adcbb718832..3906e975d6db 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1597,6 +1597,7 @@ static const struct of_device_id qcom_pcie_match[] = {
 	{ .compatible = "qcom,pcie-ipq4019", .data = &ops_2_4_0 },
 	{ .compatible = "qcom,pcie-qcs404", .data = &ops_2_4_0 },
 	{ .compatible = "qcom,pcie-sdm845", .data = &ops_2_7_0 },
+	{ .compatible = "qcom,pcie-sc8180x", .data = &ops_1_9_0 },
 	{ .compatible = "qcom,pcie-sm8250", .data = &ops_1_9_0 },
 	{ }
 };
-- 
2.29.2

Re: [PATCH 1/3] PCI: qcom: Introduce enable/disable resource ops

[Rob Herring]

On Sat, Jul 24, 2021 at 09:00:36PM -0700, Bjorn Andersson wrote:
> The current model of doing resource enablement and controller
> initialization in a single "init" function invoked after
> dw_pcie_host_init() is invoked might result in clocks not being enabled
> at the time the "msi" interrupt fires.

This seems like working around DWC ops...

> One such case happens reliably on the SC8180x (8cx) Snapdragon laptops,
> where it's seems like the bootloader touches PCIe and leaves things in a
> state that the "msi" interrupt will fire before we have a change to

s/change/chance/

> enable the clocks, resulting in an access of unclocked hardware.

How does the MSI fire without the clocks or a link? Can't you quiesce 
things?

> Introduce a two new callbacks, allowing the individual resource handling
> functions to be split between enable/init and deinit/disable.
> 
> Helper functions for enable, disable and deinit are introduced to handle
> the fact that these functions may now be left without implementation.
> init is given a wrapper for symmetry.

I think you can simply flip the order the MSI init and host_init() in 
dw_pcie_host_init().

In general, I want to move some of the resource setup (clks, phys, 
perst#, etc.) into the DWC core and make the DWC ops more specific in 
what they do and touch. That should simplify at least the simple cases. 
For Qcom, maybe some of the ops can be moved to new DWC ops.

Rob


> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 42 +++++++++++++++++++++++---
>  1 file changed, 38 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 8a7a300163e5..8a64a126de2b 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -181,9 +181,11 @@ struct qcom_pcie;
>  
>  struct qcom_pcie_ops {
>  	int (*get_resources)(struct qcom_pcie *pcie);
> +	int (*enable_resources)(struct qcom_pcie *pcie);
>  	int (*init)(struct qcom_pcie *pcie);
>  	int (*post_init)(struct qcom_pcie *pcie);
>  	void (*deinit)(struct qcom_pcie *pcie);
> +	void (*disable_resources)(struct qcom_pcie *pcie);
>  	void (*post_deinit)(struct qcom_pcie *pcie);
>  	void (*ltssm_enable)(struct qcom_pcie *pcie);
>  	int (*config_sid)(struct qcom_pcie *pcie);
> @@ -1345,6 +1347,31 @@ static int qcom_pcie_config_sid_sm8250(struct qcom_pcie *pcie)
>  	return 0;
>  }
>  
> +static int qcom_pcie_enable_resources(struct qcom_pcie *pcie)
> +{
> +	if (pcie->ops->enable_resources)
> +		return pcie->ops->enable_resources(pcie);
> +
> +	return 0;
> +}
> +
> +static int qcom_pcie_init(struct qcom_pcie *pcie)
> +{
> +	return pcie->ops->init(pcie);
> +}
> +
> +static void qcom_pcie_deinit(struct qcom_pcie *pcie)
> +{
> +	if (pcie->ops->deinit)
> +		pcie->ops->deinit(pcie);
> +}
> +
> +static void qcom_pcie_disable_resources(struct qcom_pcie *pcie)
> +{
> +	if (pcie->ops->disable_resources)
> +		pcie->ops->disable_resources(pcie);
> +}
> +
>  static int qcom_pcie_host_init(struct pcie_port *pp)
>  {
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> @@ -1353,7 +1380,7 @@ static int qcom_pcie_host_init(struct pcie_port *pp)
>  
>  	qcom_ep_reset_assert(pcie);
>  
> -	ret = pcie->ops->init(pcie);
> +	ret = qcom_pcie_init(pcie);
>  	if (ret)
>  		return ret;
>  
> @@ -1384,7 +1411,7 @@ static int qcom_pcie_host_init(struct pcie_port *pp)
>  err_disable_phy:
>  	phy_power_off(pcie->phy);
>  err_deinit:
> -	pcie->ops->deinit(pcie);
> +	qcom_pcie_deinit(pcie);
>  
>  	return ret;
>  }
> @@ -1520,10 +1547,14 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  
>  	pp->ops = &qcom_pcie_dw_ops;
>  
> +	ret = qcom_pcie_enable_resources(pcie);
> +	if (ret)
> +		goto err_pm_runtime_put;
> +
>  	ret = phy_init(pcie->phy);
>  	if (ret) {
>  		pm_runtime_disable(&pdev->dev);
> -		goto err_pm_runtime_put;
> +		goto err_disable_resources;
>  	}
>  
>  	platform_set_drvdata(pdev, pcie);
> @@ -1532,11 +1563,14 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  	if (ret) {
>  		dev_err(dev, "cannot initialize host\n");
>  		pm_runtime_disable(&pdev->dev);
> -		goto err_pm_runtime_put;
> +		goto err_disable_resources;
>  	}
>  
>  	return 0;
>  
> +err_disable_resources:
> +	qcom_pcie_disable_resources(pcie);
> +
>  err_pm_runtime_put:
>  	pm_runtime_put(dev);
>  	pm_runtime_disable(dev);
> -- 
> 2.29.2
> 
> 

Re: [PATCH 3/3] PCI: qcom: Add sc8180x compatible

[Rob Herring]

On Sat, Jul 24, 2021 at 09:00:38PM -0700, Bjorn Andersson wrote:
> The SC8180x platform comes with 4 PCIe controllers, typically used for
> things such as NVME storage or connecting a SDX55 5G modem. Add a
> compatible for this, that just reuses the 1.9.0 ops.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  Documentation/devicetree/bindings/pci/qcom,pcie.txt | 5 +++--
>  drivers/pci/controller/dwc/pcie-qcom.c              | 1 +
>  2 files changed, 4 insertions(+), 2 deletions(-)

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