Re: [PATCH v3 4/4] PCI: qcom: Add Qualcomm SA8255p based PCIe root complex functionality

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 11/15/2024 3:25 AM, Manivannan Sadhasivam wrote:
On Wed, Nov 06, 2024 at 02:13:41PM -0800, Mayank Rana wrote:
On SA8255p ride platform, PCIe root complex is firmware managed as well
configured into ECAM compliant mode. This change adds functionality to
enable resource management (system resource as well PCIe controller and
PHY configuration) through firmware, and enumerating ECAM compliant root
complex.

Signed-off-by: Mayank Rana <quic_mrana@xxxxxxxxxxx>
---
  drivers/pci/controller/dwc/Kconfig     |   1 +
  drivers/pci/controller/dwc/pcie-qcom.c | 116 +++++++++++++++++++++++--
  2 files changed, 108 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index b6d6778b0698..0fe76bd39d69 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -275,6 +275,7 @@ config PCIE_QCOM
  	select PCIE_DW_HOST
  	select CRC8
  	select PCIE_QCOM_COMMON
+	select PCI_HOST_COMMON
  	help
  	  Say Y here to enable PCIe controller support on Qualcomm SoCs. The
  	  PCIe controller uses the DesignWare core plus Qualcomm-specific
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index ef44a82be058..2cb74f902baf 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -21,7 +21,9 @@
  #include <linux/limits.h>
  #include <linux/init.h>
  #include <linux/of.h>
+#include <linux/of_pci.h>
  #include <linux/pci.h>
+#include <linux/pci-ecam.h>
  #include <linux/pm_opp.h>
  #include <linux/pm_runtime.h>
  #include <linux/platform_device.h>
@@ -254,10 +256,12 @@ struct qcom_pcie_ops {
    * @ops: qcom PCIe ops structure
    * @override_no_snoop: Override NO_SNOOP attribute in TLP to enable cache
    * snooping
+  * @firmware_managed: Set if PCIe root complex is firmware managed

ecam_compliant?
I assume you mean to update as Set if ECAM compliant PCIe root complex is firmware manage
    */
  struct qcom_pcie_cfg {
  	const struct qcom_pcie_ops *ops;
  	bool override_no_snoop;
+	bool firmware_managed;
  	bool no_l0s;
  };
@@ -1415,6 +1419,10 @@ static const struct qcom_pcie_cfg cfg_sc8280xp = {
  	.no_l0s = true,
  };
+static const struct qcom_pcie_cfg cfg_fw_managed = {

cfg_ecam?
Putting more emphasize on fw_managed as don't want to conflict with new work in progress (krishna is working on it) to make Qualcomm PCIe root complex as ECAM compliant (non firmware managed one). are you OK with using cfg_ecam_fw_managed ?

+	.firmware_managed = true,
+};
+
  static const struct dw_pcie_ops dw_pcie_ops = {
  	.link_up = qcom_pcie_link_up,
  	.start_link = qcom_pcie_start_link,
@@ -1566,6 +1574,51 @@ static irqreturn_t qcom_pcie_global_irq_thread(int irq, void *data)
  	return IRQ_HANDLED;
  }
+static void qcom_pci_free_msi(void *ptr)
+{
+	struct dw_pcie_rp *pp = (struct dw_pcie_rp *)ptr;
+
+	if (pp && pp->has_msi_ctrl)
+		dw_pcie_free_msi(pp);
+}
+
+static int qcom_pcie_ecam_host_init(struct pci_config_window *cfg)
+{
+	struct device *dev = cfg->parent;
+	struct dw_pcie_rp *pp;
+	struct dw_pcie *pci;
+	int ret;
+
+	pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
+	if (!pci)
+		return -ENOMEM;
+
+	pci->dev = dev;
+	pp = &pci->pp;
+	pci->dbi_base = cfg->win;
+	pp->num_vectors = MSI_DEF_NUM_VECTORS;
+
+	ret = dw_pcie_msi_host_init(pp);
+	if (ret)
+		return ret;
+
+	pp->has_msi_ctrl = true;
+	dw_pcie_msi_init(pp);
+
+	ret = devm_add_action_or_reset(dev, qcom_pci_free_msi, pp);

return devm_add_action_or_reset()...
Done.

+	return ret;
+}
+
+/* ECAM ops */
+const struct pci_ecam_ops pci_qcom_ecam_ops = {
+	.init		= qcom_pcie_ecam_host_init,
+	.pci_ops	= {
+		.map_bus	= pci_ecam_map_bus,
+		.read		= pci_generic_config_read,
+		.write		= pci_generic_config_write,
+	}
+};
+
  static int qcom_pcie_probe(struct platform_device *pdev)
  {
  	const struct qcom_pcie_cfg *pcie_cfg;
@@ -1580,11 +1633,52 @@ static int qcom_pcie_probe(struct platform_device *pdev)
  	char *name;
pcie_cfg = of_device_get_match_data(dev);
-	if (!pcie_cfg || !pcie_cfg->ops) {
-		dev_err(dev, "Invalid platform data\n");
+	if (!pcie_cfg) {
+		dev_err(dev, "No platform data\n");
+		return -EINVAL;
+	}
+
+	if (!pcie_cfg->firmware_managed && !pcie_cfg->ops) {
+		dev_err(dev, "No platform ops\n");
  		return -EINVAL;
  	}
+ pm_runtime_enable(dev);
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0)
+		goto err_pm_runtime_put;
+
+	if (pcie_cfg->firmware_managed) {
+		struct pci_host_bridge *bridge;
+		struct pci_config_window *cfg;
+
+		bridge = devm_pci_alloc_host_bridge(dev, 0);
+		if (!bridge) {
+			ret = -ENOMEM;
+			goto err_pm_runtime_put;
+		}
+
+		of_pci_check_probe_only();

You haven't defined the "linux,pci-probe-only" property in DT. So if everything
works fine, then you can leave this call.
ok will review more and update accordingly.

+		/* Parse and map our Configuration Space windows */
+		cfg = gen_pci_init(dev, bridge, &pci_qcom_ecam_ops);
+		if (IS_ERR(cfg)) {
+			ret = PTR_ERR(cfg);
+			goto err_pm_runtime_put;
+		}
+
+		bridge->sysdata = cfg;
+		bridge->ops = (struct pci_ops *)&pci_qcom_ecam_ops.pci_ops;
+		bridge->msi_domain = true;
+
+		ret = pci_host_probe(bridge);

return pci_host_probe()...
need to perform pm_runtime_put_sync() and pm_runtime_disable() in failure case.
Hence checking error here and doing goto err_pm_runtime_put
+		if (ret) {
+			dev_err(dev, "pci_host_probe() failed:%d\n", ret);
+			goto err_pm_runtime_put;
+		}
+
+		return ret;
+	}
+
  	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
  	if (!pcie)
  		return -ENOMEM;
@@ -1593,11 +1687,6 @@ static int qcom_pcie_probe(struct platform_device *pdev)
  	if (!pci)
  		return -ENOMEM;
- pm_runtime_enable(dev);
-	ret = pm_runtime_get_sync(dev);
-	if (ret < 0)
-		goto err_pm_runtime_put;
-
  	pci->dev = dev;
  	pci->ops = &dw_pcie_ops;
  	pp = &pci->pp;
@@ -1739,9 +1828,13 @@ static int qcom_pcie_probe(struct platform_device *pdev)
static int qcom_pcie_suspend_noirq(struct device *dev)
  {
-	struct qcom_pcie *pcie = dev_get_drvdata(dev);
+	struct qcom_pcie *pcie;
  	int ret = 0;
+ if (of_device_is_compatible(dev->of_node, "qcom,pcie-sa8255p"))
+		return 0;

How about bailing out if dev_get_drvdata() returns NULL?
Done

Regards,
Mayank
- Mani





[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux