[PATCH v2 0/4] Loadable Module support for PCIe Cadence and J721E

[PATCH v2 0/4] Loadable Module support for PCIe Cadence and J721E

[Siddharth Vadapalli]

Hello,

This series enables support to build the PCIe Cadence Controller drivers
and the PCI J721E Application/Wrapper/Glue driver as Loadable Kernel
Modules. The motivation for this series is that PCIe is not a necessity
for booting the SoC, due to which it doesn't have to be a built-in
module. Additionally, the defconfig doesn't enable the PCIe Cadence
Controller drivers and the PCI J721E driver, due to which PCIe is not
supported by default. Enabling the configs as of now (i.e. without this
series) will result in built-in drivers i.e. a bloated Linux Image for
everyone who doesn't have the PCIe Controller. Therefore, with this
series, after enabling support for building the drivers as loadable
modules, the driver configs can be enabled in the defconfig to build
the drivers as loadable modules, thereby enabling PCIe.

Series is based on linux-next tagged next-20250328.

Series has been tested by loading and unloading the PCI J721E driver
when operating in the Root-Complex mode on J700-EVM with an NVMe SSD
connected to the PCIe Connector. "hdparm" based reads of the NVMe SSD
have been performed to validate functionality before and after a module
unload-load sequence using modprobe. Additionally, the module unload
was performed while running "hdparm" in the background. No crash was
seen and reloading the module enumerated the NVMe SSD and "hdparm" could
be re-run successfully.

v1 of this series is at:
https://lore.kernel.org/r/20250307103128.3287497-1-s-vadapalli@ti.com/
Changes since v1:
- Collected "Reviewed-by" tags from:
  Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
  for the first two patches in this series.
- Based on feedback from Mani on the third patch of the v1 series at:
  https://lore.kernel.org/r/20250318080304.jsmrxqil6pn74nzh@thinkpad/
  pci_epc_deinit_notify() has been included in cdns_pcie_ep_disable().
- Based on feedback from Thomas on the fourth patch of the v1 series at:
  https://lore.kernel.org/r/88b3ecbe-32b6-4310-afb9-da19a2d0506a@bootlin.com/
  the "check" for a non-NULL "pcie-refclk" in j721e_pcie_remove() has been
  dropped.

Regards,
Siddharth.

Kishon Vijay Abraham I (1):
  PCI: cadence: Add support to build pcie-cadence library as a kernel
    module

Siddharth Vadapalli (3):
  PCI: cadence-host: Introduce cdns_pcie_host_disable helper for cleanup
  PCI: cadence-ep: Introduce cdns_pcie_ep_disable helper for cleanup
  PCI: j721e: Add support to build as a loadable module

 drivers/pci/controller/cadence/Kconfig        |  12 +-
 drivers/pci/controller/cadence/pci-j721e.c    |  33 ++++-
 .../pci/controller/cadence/pcie-cadence-ep.c  |  17 +++
 .../controller/cadence/pcie-cadence-host.c    | 113 ++++++++++++++++++
 drivers/pci/controller/cadence/pcie-cadence.c |  12 ++
 drivers/pci/controller/cadence/pcie-cadence.h |  14 ++-
 6 files changed, 192 insertions(+), 9 deletions(-)

-- 
2.34.1

[PATCH v2 1/4] PCI: cadence: Add support to build pcie-cadence library as a kernel module

[Siddharth Vadapalli]

From: Kishon Vijay Abraham I <kishon@ti.com>

Currently, the Cadence PCIe controller driver can be built as a built-in
module only. Since PCIe functionality is not a necessity for booting, add
support to build the Cadence PCIe driver as a loadable module as well.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---

v1:
https://lore.kernel.org/r/20250307103128.3287497-2-s-vadapalli@ti.com/
Changes since v1:
- Collected "Reviewed-by" tag from:
  Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Regards,
Siddharth.

 drivers/pci/controller/cadence/Kconfig             |  6 +++---
 drivers/pci/controller/cadence/pcie-cadence-ep.c   |  6 ++++++
 drivers/pci/controller/cadence/pcie-cadence-host.c |  9 +++++++++
 drivers/pci/controller/cadence/pcie-cadence.c      | 12 ++++++++++++
 drivers/pci/controller/cadence/pcie-cadence.h      |  4 ++--
 5 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/controller/cadence/Kconfig b/drivers/pci/controller/cadence/Kconfig
index 8a0044bb3989..82b58096eea0 100644
--- a/drivers/pci/controller/cadence/Kconfig
+++ b/drivers/pci/controller/cadence/Kconfig
@@ -4,16 +4,16 @@ menu "Cadence-based PCIe controllers"
 	depends on PCI
 
 config PCIE_CADENCE
-	bool
+	tristate
 
 config PCIE_CADENCE_HOST
-	bool
+	tristate
 	depends on OF
 	select IRQ_DOMAIN
 	select PCIE_CADENCE
 
 config PCIE_CADENCE_EP
-	bool
+	tristate
 	depends on OF
 	depends on PCI_ENDPOINT
 	select PCIE_CADENCE
diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c
index 599ec4b1223e..b4bcef2d020a 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-ep.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c
@@ -6,6 +6,7 @@
 #include <linux/bitfield.h>
 #include <linux/delay.h>
 #include <linux/kernel.h>
+#include <linux/module.h>
 #include <linux/of.h>
 #include <linux/pci-epc.h>
 #include <linux/platform_device.h>
@@ -751,3 +752,8 @@ int cdns_pcie_ep_setup(struct cdns_pcie_ep *ep)
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(cdns_pcie_ep_setup);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Cadence PCIe endpoint controller driver");
+MODULE_AUTHOR("Cyrille Pitchen <cyrille.pitchen@free-electrons.com>");
diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
index 8af95e9da7ce..96055edeb099 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-host.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
@@ -5,6 +5,7 @@
 
 #include <linux/delay.h>
 #include <linux/kernel.h>
+#include <linux/module.h>
 #include <linux/list_sort.h>
 #include <linux/of_address.h>
 #include <linux/of_pci.h>
@@ -72,6 +73,7 @@ void __iomem *cdns_pci_map_bus(struct pci_bus *bus, unsigned int devfn,
 
 	return rc->cfg_base + (where & 0xfff);
 }
+EXPORT_SYMBOL_GPL(cdns_pci_map_bus);
 
 static struct pci_ops cdns_pcie_host_ops = {
 	.map_bus	= cdns_pci_map_bus,
@@ -495,6 +497,7 @@ int cdns_pcie_host_init(struct cdns_pcie_rc *rc)
 
 	return cdns_pcie_host_init_address_translation(rc);
 }
+EXPORT_SYMBOL_GPL(cdns_pcie_host_init);
 
 int cdns_pcie_host_link_setup(struct cdns_pcie_rc *rc)
 {
@@ -519,6 +522,7 @@ int cdns_pcie_host_link_setup(struct cdns_pcie_rc *rc)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(cdns_pcie_host_link_setup);
 
 int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
 {
@@ -581,3 +585,8 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(cdns_pcie_host_setup);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Cadence PCIe host controller driver");
+MODULE_AUTHOR("Cyrille Pitchen <cyrille.pitchen@free-electrons.com>");
diff --git a/drivers/pci/controller/cadence/pcie-cadence.c b/drivers/pci/controller/cadence/pcie-cadence.c
index 204e045aed8c..70a19573440e 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.c
+++ b/drivers/pci/controller/cadence/pcie-cadence.c
@@ -4,6 +4,7 @@
 // Author: Cyrille Pitchen <cyrille.pitchen@free-electrons.com>
 
 #include <linux/kernel.h>
+#include <linux/module.h>
 #include <linux/of.h>
 
 #include "pcie-cadence.h"
@@ -23,6 +24,7 @@ void cdns_pcie_detect_quiet_min_delay_set(struct cdns_pcie *pcie)
 
 	cdns_pcie_writel(pcie, CDNS_PCIE_LTSSM_CONTROL_CAP, ltssm_control_cap);
 }
+EXPORT_SYMBOL_GPL(cdns_pcie_detect_quiet_min_delay_set);
 
 void cdns_pcie_set_outbound_region(struct cdns_pcie *pcie, u8 busnr, u8 fn,
 				   u32 r, bool is_io,
@@ -100,6 +102,7 @@ void cdns_pcie_set_outbound_region(struct cdns_pcie *pcie, u8 busnr, u8 fn,
 	cdns_pcie_writel(pcie, CDNS_PCIE_AT_OB_REGION_CPU_ADDR0(r), addr0);
 	cdns_pcie_writel(pcie, CDNS_PCIE_AT_OB_REGION_CPU_ADDR1(r), addr1);
 }
+EXPORT_SYMBOL_GPL(cdns_pcie_set_outbound_region);
 
 void cdns_pcie_set_outbound_region_for_normal_msg(struct cdns_pcie *pcie,
 						  u8 busnr, u8 fn,
@@ -134,6 +137,7 @@ void cdns_pcie_set_outbound_region_for_normal_msg(struct cdns_pcie *pcie,
 	cdns_pcie_writel(pcie, CDNS_PCIE_AT_OB_REGION_CPU_ADDR0(r), addr0);
 	cdns_pcie_writel(pcie, CDNS_PCIE_AT_OB_REGION_CPU_ADDR1(r), addr1);
 }
+EXPORT_SYMBOL_GPL(cdns_pcie_set_outbound_region_for_normal_msg);
 
 void cdns_pcie_reset_outbound_region(struct cdns_pcie *pcie, u32 r)
 {
@@ -146,6 +150,7 @@ void cdns_pcie_reset_outbound_region(struct cdns_pcie *pcie, u32 r)
 	cdns_pcie_writel(pcie, CDNS_PCIE_AT_OB_REGION_CPU_ADDR0(r), 0);
 	cdns_pcie_writel(pcie, CDNS_PCIE_AT_OB_REGION_CPU_ADDR1(r), 0);
 }
+EXPORT_SYMBOL_GPL(cdns_pcie_reset_outbound_region);
 
 void cdns_pcie_disable_phy(struct cdns_pcie *pcie)
 {
@@ -156,6 +161,7 @@ void cdns_pcie_disable_phy(struct cdns_pcie *pcie)
 		phy_exit(pcie->phy[i]);
 	}
 }
+EXPORT_SYMBOL_GPL(cdns_pcie_disable_phy);
 
 int cdns_pcie_enable_phy(struct cdns_pcie *pcie)
 {
@@ -184,6 +190,7 @@ int cdns_pcie_enable_phy(struct cdns_pcie *pcie)
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(cdns_pcie_enable_phy);
 
 int cdns_pcie_init_phy(struct device *dev, struct cdns_pcie *pcie)
 {
@@ -243,6 +250,7 @@ int cdns_pcie_init_phy(struct device *dev, struct cdns_pcie *pcie)
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(cdns_pcie_init_phy);
 
 static int cdns_pcie_suspend_noirq(struct device *dev)
 {
@@ -271,3 +279,7 @@ const struct dev_pm_ops cdns_pcie_pm_ops = {
 	NOIRQ_SYSTEM_SLEEP_PM_OPS(cdns_pcie_suspend_noirq,
 				  cdns_pcie_resume_noirq)
 };
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Cadence PCIe controller driver");
+MODULE_AUTHOR("Cyrille Pitchen <cyrille.pitchen@free-electrons.com>");
diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
index 39ee9945c903..4b7f295e24e7 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -519,7 +519,7 @@ static inline bool cdns_pcie_link_up(struct cdns_pcie *pcie)
 	return true;
 }
 
-#ifdef CONFIG_PCIE_CADENCE_HOST
+#if IS_ENABLED(CONFIG_PCIE_CADENCE_HOST)
 int cdns_pcie_host_link_setup(struct cdns_pcie_rc *rc);
 int cdns_pcie_host_init(struct cdns_pcie_rc *rc);
 int cdns_pcie_host_setup(struct cdns_pcie_rc *rc);
@@ -548,7 +548,7 @@ static inline void __iomem *cdns_pci_map_bus(struct pci_bus *bus, unsigned int d
 }
 #endif
 
-#ifdef CONFIG_PCIE_CADENCE_EP
+#if IS_ENABLED(CONFIG_PCIE_CADENCE_EP)
 int cdns_pcie_ep_setup(struct cdns_pcie_ep *ep);
 #else
 static inline int cdns_pcie_ep_setup(struct cdns_pcie_ep *ep)
-- 
2.34.1

[PATCH v2 2/4] PCI: cadence-host: Introduce cdns_pcie_host_disable helper for cleanup

[Siddharth Vadapalli]

Introduce the helper function cdns_pcie_host_disable() which will undo
the configuration performed by cdns_pcie_host_setup(). Also, export it
for use by existing callers of cdns_pcie_host_setup(), thereby allowing
them to cleanup on their exit path.

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---

v1:
https://lore.kernel.org/r/20250307103128.3287497-3-s-vadapalli@ti.com/
Changes since v1:
- Collected "Reviewed-by" tag from:
  Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Regards,
Siddharth.

 .../controller/cadence/pcie-cadence-host.c    | 104 ++++++++++++++++++
 drivers/pci/controller/cadence/pcie-cadence.h |   5 +
 2 files changed, 109 insertions(+)

diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
index 96055edeb099..741508738f88 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-host.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
@@ -152,6 +152,14 @@ static int cdns_pcie_retrain(struct cdns_pcie *pcie)
 	return ret;
 }
 
+static void cdns_pcie_host_disable_ptm_response(struct cdns_pcie *pcie)
+{
+	u32 val;
+
+	val = cdns_pcie_readl(pcie, CDNS_PCIE_LM_PTM_CTRL);
+	cdns_pcie_writel(pcie, CDNS_PCIE_LM_PTM_CTRL, val & ~CDNS_PCIE_LM_TPM_CTRL_PTMRSEN);
+}
+
 static void cdns_pcie_host_enable_ptm_response(struct cdns_pcie *pcie)
 {
 	u32 val;
@@ -177,6 +185,26 @@ static int cdns_pcie_host_start_link(struct cdns_pcie_rc *rc)
 	return ret;
 }
 
+static void cdns_pcie_host_deinit_root_port(struct cdns_pcie_rc *rc)
+{
+	struct cdns_pcie *pcie = &rc->pcie;
+	u32 value, ctrl;
+
+	cdns_pcie_rp_writew(pcie, PCI_CLASS_DEVICE, 0xffff);
+	cdns_pcie_rp_writeb(pcie, PCI_CLASS_PROG, 0xff);
+	cdns_pcie_rp_writeb(pcie, PCI_CLASS_REVISION, 0xff);
+	cdns_pcie_writel(pcie, CDNS_PCIE_LM_ID, 0xffffffff);
+	cdns_pcie_rp_writew(pcie, PCI_DEVICE_ID, 0xffff);
+	ctrl = CDNS_PCIE_LM_BAR_CFG_CTRL_DISABLED;
+	value = ~(CDNS_PCIE_LM_RC_BAR_CFG_BAR0_CTRL(ctrl) |
+		CDNS_PCIE_LM_RC_BAR_CFG_BAR1_CTRL(ctrl) |
+		CDNS_PCIE_LM_RC_BAR_CFG_PREFETCH_MEM_ENABLE |
+		CDNS_PCIE_LM_RC_BAR_CFG_PREFETCH_MEM_64BITS |
+		CDNS_PCIE_LM_RC_BAR_CFG_IO_ENABLE |
+		CDNS_PCIE_LM_RC_BAR_CFG_IO_32BITS);
+	cdns_pcie_writel(pcie, CDNS_PCIE_LM_RC_BAR_CFG, value);
+}
+
 static int cdns_pcie_host_init_root_port(struct cdns_pcie_rc *rc)
 {
 	struct cdns_pcie *pcie = &rc->pcie;
@@ -393,6 +421,32 @@ static int cdns_pcie_host_dma_ranges_cmp(void *priv, const struct list_head *a,
         return resource_size(entry2->res) - resource_size(entry1->res);
 }
 
+static void cdns_pcie_host_unmap_dma_ranges(struct cdns_pcie_rc *rc)
+{
+	struct cdns_pcie *pcie = &rc->pcie;
+	enum cdns_pcie_rp_bar bar;
+	u32 value;
+
+	/* Reset inbound configuration for all BARs which were being used */
+	for (bar = RP_BAR0; bar <= RP_NO_BAR; bar++) {
+		if (rc->avail_ib_bar[bar])
+			continue;
+
+		cdns_pcie_writel(pcie, CDNS_PCIE_AT_IB_RP_BAR_ADDR0(bar), 0);
+		cdns_pcie_writel(pcie, CDNS_PCIE_AT_IB_RP_BAR_ADDR1(bar), 0);
+
+		if (bar == RP_NO_BAR)
+			continue;
+
+		value = ~(LM_RC_BAR_CFG_CTRL_MEM_64BITS(bar) |
+			  LM_RC_BAR_CFG_CTRL_PREF_MEM_64BITS(bar) |
+			  LM_RC_BAR_CFG_CTRL_MEM_32BITS(bar) |
+			  LM_RC_BAR_CFG_CTRL_PREF_MEM_32BITS(bar) |
+			  LM_RC_BAR_CFG_APERTURE(bar, bar_aperture_mask[bar] + 2));
+		cdns_pcie_writel(pcie, CDNS_PCIE_LM_RC_BAR_CFG, value);
+	}
+}
+
 static int cdns_pcie_host_map_dma_ranges(struct cdns_pcie_rc *rc)
 {
 	struct cdns_pcie *pcie = &rc->pcie;
@@ -430,6 +484,29 @@ static int cdns_pcie_host_map_dma_ranges(struct cdns_pcie_rc *rc)
 	return 0;
 }
 
+static void cdns_pcie_host_deinit_address_translation(struct cdns_pcie_rc *rc)
+{
+	struct cdns_pcie *pcie = &rc->pcie;
+	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(rc);
+	struct resource_entry *entry;
+	int r;
+
+	cdns_pcie_host_unmap_dma_ranges(rc);
+
+	/*
+	 * Reset outbound region 0 which was reserved for configuration space
+	 * accesses.
+	 */
+	cdns_pcie_reset_outbound_region(pcie, 0);
+
+	/* Reset rest of the outbound regions */
+	r = 1;
+	resource_list_for_each_entry(entry, &bridge->windows) {
+		cdns_pcie_reset_outbound_region(pcie, r);
+		r++;
+	}
+}
+
 static int cdns_pcie_host_init_address_translation(struct cdns_pcie_rc *rc)
 {
 	struct cdns_pcie *pcie = &rc->pcie;
@@ -487,6 +564,12 @@ static int cdns_pcie_host_init_address_translation(struct cdns_pcie_rc *rc)
 	return cdns_pcie_host_map_dma_ranges(rc);
 }
 
+static void cdns_pcie_host_deinit(struct cdns_pcie_rc *rc)
+{
+	cdns_pcie_host_deinit_address_translation(rc);
+	cdns_pcie_host_deinit_root_port(rc);
+}
+
 int cdns_pcie_host_init(struct cdns_pcie_rc *rc)
 {
 	int err;
@@ -499,6 +582,14 @@ int cdns_pcie_host_init(struct cdns_pcie_rc *rc)
 }
 EXPORT_SYMBOL_GPL(cdns_pcie_host_init);
 
+static void cdns_pcie_host_link_disable(struct cdns_pcie_rc *rc)
+{
+	struct cdns_pcie *pcie = &rc->pcie;
+
+	cdns_pcie_stop_link(pcie);
+	cdns_pcie_host_disable_ptm_response(pcie);
+}
+
 int cdns_pcie_host_link_setup(struct cdns_pcie_rc *rc)
 {
 	struct cdns_pcie *pcie = &rc->pcie;
@@ -524,6 +615,19 @@ int cdns_pcie_host_link_setup(struct cdns_pcie_rc *rc)
 }
 EXPORT_SYMBOL_GPL(cdns_pcie_host_link_setup);
 
+void cdns_pcie_host_disable(struct cdns_pcie_rc *rc)
+{
+	struct pci_host_bridge *bridge;
+
+	bridge = pci_host_bridge_from_priv(rc);
+	pci_stop_root_bus(bridge->bus);
+	pci_remove_root_bus(bridge->bus);
+
+	cdns_pcie_host_deinit(rc);
+	cdns_pcie_host_link_disable(rc);
+}
+EXPORT_SYMBOL_GPL(cdns_pcie_host_disable);
+
 int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
 {
 	struct device *dev = rc->pcie.dev;
diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
index 4b7f295e24e7..0b6bed1ac146 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -523,6 +523,7 @@ static inline bool cdns_pcie_link_up(struct cdns_pcie *pcie)
 int cdns_pcie_host_link_setup(struct cdns_pcie_rc *rc);
 int cdns_pcie_host_init(struct cdns_pcie_rc *rc);
 int cdns_pcie_host_setup(struct cdns_pcie_rc *rc);
+void cdns_pcie_host_disable(struct cdns_pcie_rc *rc);
 void __iomem *cdns_pci_map_bus(struct pci_bus *bus, unsigned int devfn,
 			       int where);
 #else
@@ -541,6 +542,10 @@ static inline int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
 	return 0;
 }
 
+static inline void cdns_pcie_host_disable(struct cdns_pcie_rc *rc)
+{
+}
+
 static inline void __iomem *cdns_pci_map_bus(struct pci_bus *bus, unsigned int devfn,
 					     int where)
 {
-- 
2.34.1

[PATCH v2 3/4] PCI: cadence-ep: Introduce cdns_pcie_ep_disable helper for cleanup

[Siddharth Vadapalli]

Introduce the helper function cdns_pcie_ep_disable() which will undo the
configuration performed by cdns_pcie_ep_setup(). Also, export it for use
by the existing callers of cdns_pcie_ep_setup(), thereby allowing them
to cleanup on their exit path.

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---

v1:
https://lore.kernel.org/r/20250307103128.3287497-4-s-vadapalli@ti.com/
Changes since v1:
- Based on feedback from Mani at:
  https://lore.kernel.org/r/20250318080304.jsmrxqil6pn74nzh@thinkpad/
  pci_epc_deinit_notify() has been included in cdns_pcie_ep_disable().

Regards,
Siddharth.

 drivers/pci/controller/cadence/pcie-cadence-ep.c | 11 +++++++++++
 drivers/pci/controller/cadence/pcie-cadence.h    |  5 +++++
 2 files changed, 16 insertions(+)

diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c
index b4bcef2d020a..ffd19cb25eed 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-ep.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c
@@ -645,6 +645,17 @@ static const struct pci_epc_ops cdns_pcie_epc_ops = {
 	.get_features	= cdns_pcie_ep_get_features,
 };
 
+void cdns_pcie_ep_disable(struct cdns_pcie_ep *ep)
+{
+	struct device *dev = ep->pcie.dev;
+	struct pci_epc *epc = to_pci_epc(dev);
+
+	pci_epc_deinit_notify(epc);
+	pci_epc_mem_free_addr(epc, ep->irq_phys_addr, ep->irq_cpu_addr,
+			      SZ_128K);
+	pci_epc_mem_exit(epc);
+}
+EXPORT_SYMBOL_GPL(cdns_pcie_ep_disable);
 
 int cdns_pcie_ep_setup(struct cdns_pcie_ep *ep)
 {
diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
index 0b6bed1ac146..387174d6e453 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -555,11 +555,16 @@ static inline void __iomem *cdns_pci_map_bus(struct pci_bus *bus, unsigned int d
 
 #if IS_ENABLED(CONFIG_PCIE_CADENCE_EP)
 int cdns_pcie_ep_setup(struct cdns_pcie_ep *ep);
+void cdns_pcie_ep_disable(struct cdns_pcie_ep *ep);
 #else
 static inline int cdns_pcie_ep_setup(struct cdns_pcie_ep *ep)
 {
 	return 0;
 }
+
+static inline void cdns_pcie_ep_disable(struct cdns_pcie_ep *ep)
+{
+}
 #endif
 
 void cdns_pcie_detect_quiet_min_delay_set(struct cdns_pcie *pcie);
-- 
2.34.1

[PATCH v2 4/4] PCI: j721e: Add support to build as a loadable module

[Siddharth Vadapalli]

The 'pci-j721e.c' driver is the application/glue/wrapper driver for the
Cadence PCIe Controllers on TI SoCs. Implement support for building it as a
loadable module.

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---

v1:
https://lore.kernel.org/r/20250307103128.3287497-5-s-vadapalli@ti.com/
Changes since v1:
- Based on feedback from Thomas at:
  https://lore.kernel.org/r/88b3ecbe-32b6-4310-afb9-da19a2d0506a@bootlin.com/
  the "check" for a non-NULL "pcie-refclk" in j721e_pcie_remove() has been
  dropped.

Regards,
Siddharth.

 drivers/pci/controller/cadence/Kconfig     |  6 ++--
 drivers/pci/controller/cadence/pci-j721e.c | 33 +++++++++++++++++++++-
 2 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/cadence/Kconfig b/drivers/pci/controller/cadence/Kconfig
index 82b58096eea0..72d7d264d6c3 100644
--- a/drivers/pci/controller/cadence/Kconfig
+++ b/drivers/pci/controller/cadence/Kconfig
@@ -43,10 +43,10 @@ config PCIE_CADENCE_PLAT_EP
 	  different vendors SoCs.
 
 config PCI_J721E
-	bool
+	tristate
 
 config PCI_J721E_HOST
-	bool "TI J721E PCIe controller (host mode)"
+	tristate "TI J721E PCIe controller (host mode)"
 	depends on ARCH_K3 || COMPILE_TEST
 	depends on OF
 	select PCIE_CADENCE_HOST
@@ -57,7 +57,7 @@ config PCI_J721E_HOST
 	  core.
 
 config PCI_J721E_EP
-	bool "TI J721E PCIe controller (endpoint mode)"
+	tristate "TI J721E PCIe controller (endpoint mode)"
 	depends on ARCH_K3 || COMPILE_TEST
 	depends on OF
 	depends on PCI_ENDPOINT
diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
index ef1cfdae33bb..8bffcd31729c 100644
--- a/drivers/pci/controller/cadence/pci-j721e.c
+++ b/drivers/pci/controller/cadence/pci-j721e.c
@@ -15,6 +15,7 @@
 #include <linux/irqchip/chained_irq.h>
 #include <linux/irqdomain.h>
 #include <linux/mfd/syscon.h>
+#include <linux/module.h>
 #include <linux/of.h>
 #include <linux/pci.h>
 #include <linux/platform_device.h>
@@ -27,6 +28,7 @@
 #define cdns_pcie_to_rc(p) container_of(p, struct cdns_pcie_rc, pcie)
 
 #define ENABLE_REG_SYS_2	0x108
+#define ENABLE_CLR_REG_SYS_2	0x308
 #define STATUS_REG_SYS_2	0x508
 #define STATUS_CLR_REG_SYS_2	0x708
 #define LINK_DOWN		BIT(1)
@@ -116,6 +118,15 @@ static irqreturn_t j721e_pcie_link_irq_handler(int irq, void *priv)
 	return IRQ_HANDLED;
 }
 
+static void j721e_pcie_disable_link_irq(struct j721e_pcie *pcie)
+{
+	u32 reg;
+
+	reg = j721e_pcie_intd_readl(pcie, ENABLE_CLR_REG_SYS_2);
+	reg |= pcie->linkdown_irq_regfield;
+	j721e_pcie_intd_writel(pcie, ENABLE_CLR_REG_SYS_2, reg);
+}
+
 static void j721e_pcie_config_link_irq(struct j721e_pcie *pcie)
 {
 	u32 reg;
@@ -633,9 +644,25 @@ static void j721e_pcie_remove(struct platform_device *pdev)
 	struct j721e_pcie *pcie = platform_get_drvdata(pdev);
 	struct cdns_pcie *cdns_pcie = pcie->cdns_pcie;
 	struct device *dev = &pdev->dev;
+	struct cdns_pcie_ep *ep;
+	struct cdns_pcie_rc *rc;
+
+	if (pcie->mode == PCI_MODE_RC) {
+		rc = container_of(cdns_pcie, struct cdns_pcie_rc, pcie);
+		cdns_pcie_host_disable(rc);
+	} else {
+		ep = container_of(cdns_pcie, struct cdns_pcie_ep, pcie);
+		cdns_pcie_ep_disable(ep);
+	}
+
+	if (pcie->reset_gpio) {
+		msleep(PCIE_T_PVPERL_MS);
+		gpiod_set_value_cansleep(pcie->reset_gpio, 1);
+	}
 
 	clk_disable_unprepare(pcie->refclk);
 	cdns_pcie_disable_phy(cdns_pcie);
+	j721e_pcie_disable_link_irq(pcie);
 	pm_runtime_put(dev);
 	pm_runtime_disable(dev);
 }
@@ -730,4 +757,8 @@ static struct platform_driver j721e_pcie_driver = {
 		.pm	= pm_sleep_ptr(&j721e_pcie_pm_ops),
 	},
 };
-builtin_platform_driver(j721e_pcie_driver);
+module_platform_driver(j721e_pcie_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("PCIe controller driver for TI's J721E and related SoCs");
+MODULE_AUTHOR("Kishon Vijay Abraham I <kishon@ti.com>");
-- 
2.34.1

Re: [PATCH v2 3/4] PCI: cadence-ep: Introduce cdns_pcie_ep_disable helper for cleanup

[Manivannan Sadhasivam]

On Sun, Mar 30, 2025 at 02:09:13PM +0530, Siddharth Vadapalli wrote:
> Introduce the helper function cdns_pcie_ep_disable() which will undo the
> configuration performed by cdns_pcie_ep_setup(). Also, export it for use
> by the existing callers of cdns_pcie_ep_setup(), thereby allowing them
> to cleanup on their exit path.
> 
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
> 
> v1:
> https://lore.kernel.org/r/20250307103128.3287497-4-s-vadapalli@ti.com/
> Changes since v1:
> - Based on feedback from Mani at:
>   https://lore.kernel.org/r/20250318080304.jsmrxqil6pn74nzh@thinkpad/
>   pci_epc_deinit_notify() has been included in cdns_pcie_ep_disable().
> 
> Regards,
> Siddharth.
> 
>  drivers/pci/controller/cadence/pcie-cadence-ep.c | 11 +++++++++++
>  drivers/pci/controller/cadence/pcie-cadence.h    |  5 +++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> index b4bcef2d020a..ffd19cb25eed 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-ep.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> @@ -645,6 +645,17 @@ static const struct pci_epc_ops cdns_pcie_epc_ops = {
>  	.get_features	= cdns_pcie_ep_get_features,
>  };
>  
> +void cdns_pcie_ep_disable(struct cdns_pcie_ep *ep)
> +{
> +	struct device *dev = ep->pcie.dev;
> +	struct pci_epc *epc = to_pci_epc(dev);
> +
> +	pci_epc_deinit_notify(epc);
> +	pci_epc_mem_free_addr(epc, ep->irq_phys_addr, ep->irq_cpu_addr,
> +			      SZ_128K);
> +	pci_epc_mem_exit(epc);
> +}
> +EXPORT_SYMBOL_GPL(cdns_pcie_ep_disable);
>  
>  int cdns_pcie_ep_setup(struct cdns_pcie_ep *ep)
>  {
> diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
> index 0b6bed1ac146..387174d6e453 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence.h
> +++ b/drivers/pci/controller/cadence/pcie-cadence.h
> @@ -555,11 +555,16 @@ static inline void __iomem *cdns_pci_map_bus(struct pci_bus *bus, unsigned int d
>  
>  #if IS_ENABLED(CONFIG_PCIE_CADENCE_EP)
>  int cdns_pcie_ep_setup(struct cdns_pcie_ep *ep);
> +void cdns_pcie_ep_disable(struct cdns_pcie_ep *ep);
>  #else
>  static inline int cdns_pcie_ep_setup(struct cdns_pcie_ep *ep)
>  {
>  	return 0;
>  }
> +
> +static inline void cdns_pcie_ep_disable(struct cdns_pcie_ep *ep)
> +{
> +}
>  #endif
>  
>  void cdns_pcie_detect_quiet_min_delay_set(struct cdns_pcie *pcie);
> -- 
> 2.34.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 4/4] PCI: j721e: Add support to build as a loadable module

[Manivannan Sadhasivam]

On Sun, Mar 30, 2025 at 02:09:14PM +0530, Siddharth Vadapalli wrote:
> The 'pci-j721e.c' driver is the application/glue/wrapper driver for the
> Cadence PCIe Controllers on TI SoCs. Implement support for building it as a
> loadable module.
> 
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> ---
> 
> v1:
> https://lore.kernel.org/r/20250307103128.3287497-5-s-vadapalli@ti.com/
> Changes since v1:
> - Based on feedback from Thomas at:
>   https://lore.kernel.org/r/88b3ecbe-32b6-4310-afb9-da19a2d0506a@bootlin.com/
>   the "check" for a non-NULL "pcie-refclk" in j721e_pcie_remove() has been
>   dropped.
> 
> Regards,
> Siddharth.
> 
>  drivers/pci/controller/cadence/Kconfig     |  6 ++--
>  drivers/pci/controller/cadence/pci-j721e.c | 33 +++++++++++++++++++++-
>  2 files changed, 35 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/cadence/Kconfig b/drivers/pci/controller/cadence/Kconfig
> index 82b58096eea0..72d7d264d6c3 100644
> --- a/drivers/pci/controller/cadence/Kconfig
> +++ b/drivers/pci/controller/cadence/Kconfig
> @@ -43,10 +43,10 @@ config PCIE_CADENCE_PLAT_EP
>  	  different vendors SoCs.
>  
>  config PCI_J721E
> -	bool
> +	tristate
>  
>  config PCI_J721E_HOST
> -	bool "TI J721E PCIe controller (host mode)"
> +	tristate "TI J721E PCIe controller (host mode)"
>  	depends on ARCH_K3 || COMPILE_TEST
>  	depends on OF
>  	select PCIE_CADENCE_HOST
> @@ -57,7 +57,7 @@ config PCI_J721E_HOST
>  	  core.
>  
>  config PCI_J721E_EP
> -	bool "TI J721E PCIe controller (endpoint mode)"
> +	tristate "TI J721E PCIe controller (endpoint mode)"
>  	depends on ARCH_K3 || COMPILE_TEST
>  	depends on OF
>  	depends on PCI_ENDPOINT
> diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
> index ef1cfdae33bb..8bffcd31729c 100644
> --- a/drivers/pci/controller/cadence/pci-j721e.c
> +++ b/drivers/pci/controller/cadence/pci-j721e.c
> @@ -15,6 +15,7 @@
>  #include <linux/irqchip/chained_irq.h>
>  #include <linux/irqdomain.h>
>  #include <linux/mfd/syscon.h>
> +#include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/pci.h>
>  #include <linux/platform_device.h>
> @@ -27,6 +28,7 @@
>  #define cdns_pcie_to_rc(p) container_of(p, struct cdns_pcie_rc, pcie)
>  
>  #define ENABLE_REG_SYS_2	0x108
> +#define ENABLE_CLR_REG_SYS_2	0x308
>  #define STATUS_REG_SYS_2	0x508
>  #define STATUS_CLR_REG_SYS_2	0x708
>  #define LINK_DOWN		BIT(1)
> @@ -116,6 +118,15 @@ static irqreturn_t j721e_pcie_link_irq_handler(int irq, void *priv)
>  	return IRQ_HANDLED;
>  }
>  
> +static void j721e_pcie_disable_link_irq(struct j721e_pcie *pcie)
> +{
> +	u32 reg;
> +
> +	reg = j721e_pcie_intd_readl(pcie, ENABLE_CLR_REG_SYS_2);
> +	reg |= pcie->linkdown_irq_regfield;
> +	j721e_pcie_intd_writel(pcie, ENABLE_CLR_REG_SYS_2, reg);
> +}
> +
>  static void j721e_pcie_config_link_irq(struct j721e_pcie *pcie)
>  {
>  	u32 reg;
> @@ -633,9 +644,25 @@ static void j721e_pcie_remove(struct platform_device *pdev)
>  	struct j721e_pcie *pcie = platform_get_drvdata(pdev);
>  	struct cdns_pcie *cdns_pcie = pcie->cdns_pcie;
>  	struct device *dev = &pdev->dev;
> +	struct cdns_pcie_ep *ep;
> +	struct cdns_pcie_rc *rc;
> +
> +	if (pcie->mode == PCI_MODE_RC) {
> +		rc = container_of(cdns_pcie, struct cdns_pcie_rc, pcie);
> +		cdns_pcie_host_disable(rc);
> +	} else {
> +		ep = container_of(cdns_pcie, struct cdns_pcie_ep, pcie);
> +		cdns_pcie_ep_disable(ep);
> +	}
> +
> +	if (pcie->reset_gpio) {
> +		msleep(PCIE_T_PVPERL_MS);

There is no point in adding a delay before PERST# assertion.

> +		gpiod_set_value_cansleep(pcie->reset_gpio, 1);

This is not PERST# assert, isn't it? Typo?

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 4/4] PCI: j721e: Add support to build as a loadable module

[Siddharth Vadapalli]

On Wed, Apr 09, 2025 at 12:06:35PM +0530, Manivannan Sadhasivam wrote:

Hello Mani,

> On Sun, Mar 30, 2025 at 02:09:14PM +0530, Siddharth Vadapalli wrote:
> > The 'pci-j721e.c' driver is the application/glue/wrapper driver for the
> > Cadence PCIe Controllers on TI SoCs. Implement support for building it as a
> > loadable module.
> > 
> > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> > ---
> > 
> > v1:
> > https://lore.kernel.org/r/20250307103128.3287497-5-s-vadapalli@ti.com/
> > Changes since v1:
> > - Based on feedback from Thomas at:
> >   https://lore.kernel.org/r/88b3ecbe-32b6-4310-afb9-da19a2d0506a@bootlin.com/
> >   the "check" for a non-NULL "pcie-refclk" in j721e_pcie_remove() has been
> >   dropped.
> > 
> > Regards,
> > Siddharth.
> > 
> >  drivers/pci/controller/cadence/Kconfig     |  6 ++--
> >  drivers/pci/controller/cadence/pci-j721e.c | 33 +++++++++++++++++++++-
> >  2 files changed, 35 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/cadence/Kconfig b/drivers/pci/controller/cadence/Kconfig
> > index 82b58096eea0..72d7d264d6c3 100644
> > --- a/drivers/pci/controller/cadence/Kconfig
> > +++ b/drivers/pci/controller/cadence/Kconfig
> > @@ -43,10 +43,10 @@ config PCIE_CADENCE_PLAT_EP
> >  	  different vendors SoCs.
> >  
> >  config PCI_J721E
> > -	bool
> > +	tristate
> >  
> >  config PCI_J721E_HOST
> > -	bool "TI J721E PCIe controller (host mode)"
> > +	tristate "TI J721E PCIe controller (host mode)"
> >  	depends on ARCH_K3 || COMPILE_TEST
> >  	depends on OF
> >  	select PCIE_CADENCE_HOST
> > @@ -57,7 +57,7 @@ config PCI_J721E_HOST
> >  	  core.
> >  
> >  config PCI_J721E_EP
> > -	bool "TI J721E PCIe controller (endpoint mode)"
> > +	tristate "TI J721E PCIe controller (endpoint mode)"
> >  	depends on ARCH_K3 || COMPILE_TEST
> >  	depends on OF
> >  	depends on PCI_ENDPOINT
> > diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
> > index ef1cfdae33bb..8bffcd31729c 100644
> > --- a/drivers/pci/controller/cadence/pci-j721e.c
> > +++ b/drivers/pci/controller/cadence/pci-j721e.c
> > @@ -15,6 +15,7 @@
> >  #include <linux/irqchip/chained_irq.h>
> >  #include <linux/irqdomain.h>
> >  #include <linux/mfd/syscon.h>
> > +#include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/pci.h>
> >  #include <linux/platform_device.h>
> > @@ -27,6 +28,7 @@
> >  #define cdns_pcie_to_rc(p) container_of(p, struct cdns_pcie_rc, pcie)
> >  
> >  #define ENABLE_REG_SYS_2	0x108
> > +#define ENABLE_CLR_REG_SYS_2	0x308
> >  #define STATUS_REG_SYS_2	0x508
> >  #define STATUS_CLR_REG_SYS_2	0x708
> >  #define LINK_DOWN		BIT(1)
> > @@ -116,6 +118,15 @@ static irqreturn_t j721e_pcie_link_irq_handler(int irq, void *priv)
> >  	return IRQ_HANDLED;
> >  }
> >  
> > +static void j721e_pcie_disable_link_irq(struct j721e_pcie *pcie)
> > +{
> > +	u32 reg;
> > +
> > +	reg = j721e_pcie_intd_readl(pcie, ENABLE_CLR_REG_SYS_2);
> > +	reg |= pcie->linkdown_irq_regfield;
> > +	j721e_pcie_intd_writel(pcie, ENABLE_CLR_REG_SYS_2, reg);
> > +}
> > +
> >  static void j721e_pcie_config_link_irq(struct j721e_pcie *pcie)
> >  {
> >  	u32 reg;
> > @@ -633,9 +644,25 @@ static void j721e_pcie_remove(struct platform_device *pdev)
> >  	struct j721e_pcie *pcie = platform_get_drvdata(pdev);
> >  	struct cdns_pcie *cdns_pcie = pcie->cdns_pcie;
> >  	struct device *dev = &pdev->dev;
> > +	struct cdns_pcie_ep *ep;
> > +	struct cdns_pcie_rc *rc;
> > +
> > +	if (pcie->mode == PCI_MODE_RC) {
> > +		rc = container_of(cdns_pcie, struct cdns_pcie_rc, pcie);
> > +		cdns_pcie_host_disable(rc);
> > +	} else {
> > +		ep = container_of(cdns_pcie, struct cdns_pcie_ep, pcie);
> > +		cdns_pcie_ep_disable(ep);
> > +	}
> > +
> > +	if (pcie->reset_gpio) {
> > +		msleep(PCIE_T_PVPERL_MS);
> 
> There is no point in adding a delay before PERST# assertion.

True :)

> 
> > +		gpiod_set_value_cansleep(pcie->reset_gpio, 1);
> 
> This is not PERST# assert, isn't it? Typo?

It is PERST# assert. As you rightly pointed out above, a delay isn't
required when asserting it. It can be dropped. I will fix this in the v3
series. Thank you for reviewing this patch.

Regards,
Siddharth.

Re: [PATCH v2 4/4] PCI: j721e: Add support to build as a loadable module

[Manivannan Sadhasivam]

On Wed, Apr 09, 2025 at 12:12:27PM +0530, Siddharth Vadapalli wrote:
> On Wed, Apr 09, 2025 at 12:06:35PM +0530, Manivannan Sadhasivam wrote:
> 
> Hello Mani,
> 
> > On Sun, Mar 30, 2025 at 02:09:14PM +0530, Siddharth Vadapalli wrote:
> > > The 'pci-j721e.c' driver is the application/glue/wrapper driver for the
> > > Cadence PCIe Controllers on TI SoCs. Implement support for building it as a
> > > loadable module.
> > > 
> > > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> > > ---
> > > 
> > > v1:
> > > https://lore.kernel.org/r/20250307103128.3287497-5-s-vadapalli@ti.com/
> > > Changes since v1:
> > > - Based on feedback from Thomas at:
> > >   https://lore.kernel.org/r/88b3ecbe-32b6-4310-afb9-da19a2d0506a@bootlin.com/
> > >   the "check" for a non-NULL "pcie-refclk" in j721e_pcie_remove() has been
> > >   dropped.
> > > 
> > > Regards,
> > > Siddharth.
> > > 
> > >  drivers/pci/controller/cadence/Kconfig     |  6 ++--
> > >  drivers/pci/controller/cadence/pci-j721e.c | 33 +++++++++++++++++++++-
> > >  2 files changed, 35 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/pci/controller/cadence/Kconfig b/drivers/pci/controller/cadence/Kconfig
> > > index 82b58096eea0..72d7d264d6c3 100644
> > > --- a/drivers/pci/controller/cadence/Kconfig
> > > +++ b/drivers/pci/controller/cadence/Kconfig
> > > @@ -43,10 +43,10 @@ config PCIE_CADENCE_PLAT_EP
> > >  	  different vendors SoCs.
> > >  
> > >  config PCI_J721E
> > > -	bool
> > > +	tristate
> > >  
> > >  config PCI_J721E_HOST
> > > -	bool "TI J721E PCIe controller (host mode)"
> > > +	tristate "TI J721E PCIe controller (host mode)"
> > >  	depends on ARCH_K3 || COMPILE_TEST
> > >  	depends on OF
> > >  	select PCIE_CADENCE_HOST
> > > @@ -57,7 +57,7 @@ config PCI_J721E_HOST
> > >  	  core.
> > >  
> > >  config PCI_J721E_EP
> > > -	bool "TI J721E PCIe controller (endpoint mode)"
> > > +	tristate "TI J721E PCIe controller (endpoint mode)"
> > >  	depends on ARCH_K3 || COMPILE_TEST
> > >  	depends on OF
> > >  	depends on PCI_ENDPOINT
> > > diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
> > > index ef1cfdae33bb..8bffcd31729c 100644
> > > --- a/drivers/pci/controller/cadence/pci-j721e.c
> > > +++ b/drivers/pci/controller/cadence/pci-j721e.c
> > > @@ -15,6 +15,7 @@
> > >  #include <linux/irqchip/chained_irq.h>
> > >  #include <linux/irqdomain.h>
> > >  #include <linux/mfd/syscon.h>
> > > +#include <linux/module.h>
> > >  #include <linux/of.h>
> > >  #include <linux/pci.h>
> > >  #include <linux/platform_device.h>
> > > @@ -27,6 +28,7 @@
> > >  #define cdns_pcie_to_rc(p) container_of(p, struct cdns_pcie_rc, pcie)
> > >  
> > >  #define ENABLE_REG_SYS_2	0x108
> > > +#define ENABLE_CLR_REG_SYS_2	0x308
> > >  #define STATUS_REG_SYS_2	0x508
> > >  #define STATUS_CLR_REG_SYS_2	0x708
> > >  #define LINK_DOWN		BIT(1)
> > > @@ -116,6 +118,15 @@ static irqreturn_t j721e_pcie_link_irq_handler(int irq, void *priv)
> > >  	return IRQ_HANDLED;
> > >  }
> > >  
> > > +static void j721e_pcie_disable_link_irq(struct j721e_pcie *pcie)
> > > +{
> > > +	u32 reg;
> > > +
> > > +	reg = j721e_pcie_intd_readl(pcie, ENABLE_CLR_REG_SYS_2);
> > > +	reg |= pcie->linkdown_irq_regfield;
> > > +	j721e_pcie_intd_writel(pcie, ENABLE_CLR_REG_SYS_2, reg);
> > > +}
> > > +
> > >  static void j721e_pcie_config_link_irq(struct j721e_pcie *pcie)
> > >  {
> > >  	u32 reg;
> > > @@ -633,9 +644,25 @@ static void j721e_pcie_remove(struct platform_device *pdev)
> > >  	struct j721e_pcie *pcie = platform_get_drvdata(pdev);
> > >  	struct cdns_pcie *cdns_pcie = pcie->cdns_pcie;
> > >  	struct device *dev = &pdev->dev;
> > > +	struct cdns_pcie_ep *ep;
> > > +	struct cdns_pcie_rc *rc;
> > > +
> > > +	if (pcie->mode == PCI_MODE_RC) {
> > > +		rc = container_of(cdns_pcie, struct cdns_pcie_rc, pcie);
> > > +		cdns_pcie_host_disable(rc);
> > > +	} else {
> > > +		ep = container_of(cdns_pcie, struct cdns_pcie_ep, pcie);
> > > +		cdns_pcie_ep_disable(ep);
> > > +	}
> > > +
> > > +	if (pcie->reset_gpio) {
> > > +		msleep(PCIE_T_PVPERL_MS);
> > 
> > There is no point in adding a delay before PERST# assertion.
> 
> True :)
> 
> > 
> > > +		gpiod_set_value_cansleep(pcie->reset_gpio, 1);
> > 
> > This is not PERST# assert, isn't it? Typo?
> 
> It is PERST# assert.

Since the reset-gpios polarity is defined as GPIO_ACTIVE_HIGH in DT (which is
wrong unless you have an external component that changes polarity), for PERST#
assert, you need to set 0. If you set 1, then PERST# will be signalled as
deassert.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 4/4] PCI: j721e: Add support to build as a loadable module

[Siddharth Vadapalli]

On Sun, Apr 13, 2025 at 07:43:05PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Apr 09, 2025 at 12:12:27PM +0530, Siddharth Vadapalli wrote:
> > On Wed, Apr 09, 2025 at 12:06:35PM +0530, Manivannan Sadhasivam wrote:
> > 
> > Hello Mani,
> > 
> > > On Sun, Mar 30, 2025 at 02:09:14PM +0530, Siddharth Vadapalli wrote:
> > > > The 'pci-j721e.c' driver is the application/glue/wrapper driver for the
> > > > Cadence PCIe Controllers on TI SoCs. Implement support for building it as a
> > > > loadable module.
> > > > 
> > > > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> > > > ---
> > > > 
> > > > v1:
> > > > https://lore.kernel.org/r/20250307103128.3287497-5-s-vadapalli@ti.com/
> > > > Changes since v1:
> > > > - Based on feedback from Thomas at:
> > > >   https://lore.kernel.org/r/88b3ecbe-32b6-4310-afb9-da19a2d0506a@bootlin.com/
> > > >   the "check" for a non-NULL "pcie-refclk" in j721e_pcie_remove() has been
> > > >   dropped.
> > > > 
> > > > Regards,
> > > > Siddharth.
> > > > 
> > > >  drivers/pci/controller/cadence/Kconfig     |  6 ++--
> > > >  drivers/pci/controller/cadence/pci-j721e.c | 33 +++++++++++++++++++++-
> > > >  2 files changed, 35 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/pci/controller/cadence/Kconfig b/drivers/pci/controller/cadence/Kconfig
> > > > index 82b58096eea0..72d7d264d6c3 100644
> > > > --- a/drivers/pci/controller/cadence/Kconfig
> > > > +++ b/drivers/pci/controller/cadence/Kconfig
> > > > @@ -43,10 +43,10 @@ config PCIE_CADENCE_PLAT_EP
> > > >  	  different vendors SoCs.
> > > >  
> > > >  config PCI_J721E
> > > > -	bool
> > > > +	tristate
> > > >  
> > > >  config PCI_J721E_HOST
> > > > -	bool "TI J721E PCIe controller (host mode)"
> > > > +	tristate "TI J721E PCIe controller (host mode)"
> > > >  	depends on ARCH_K3 || COMPILE_TEST
> > > >  	depends on OF
> > > >  	select PCIE_CADENCE_HOST
> > > > @@ -57,7 +57,7 @@ config PCI_J721E_HOST
> > > >  	  core.
> > > >  
> > > >  config PCI_J721E_EP
> > > > -	bool "TI J721E PCIe controller (endpoint mode)"
> > > > +	tristate "TI J721E PCIe controller (endpoint mode)"
> > > >  	depends on ARCH_K3 || COMPILE_TEST
> > > >  	depends on OF
> > > >  	depends on PCI_ENDPOINT
> > > > diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
> > > > index ef1cfdae33bb..8bffcd31729c 100644
> > > > --- a/drivers/pci/controller/cadence/pci-j721e.c
> > > > +++ b/drivers/pci/controller/cadence/pci-j721e.c
> > > > @@ -15,6 +15,7 @@
> > > >  #include <linux/irqchip/chained_irq.h>
> > > >  #include <linux/irqdomain.h>
> > > >  #include <linux/mfd/syscon.h>
> > > > +#include <linux/module.h>
> > > >  #include <linux/of.h>
> > > >  #include <linux/pci.h>
> > > >  #include <linux/platform_device.h>
> > > > @@ -27,6 +28,7 @@
> > > >  #define cdns_pcie_to_rc(p) container_of(p, struct cdns_pcie_rc, pcie)
> > > >  
> > > >  #define ENABLE_REG_SYS_2	0x108
> > > > +#define ENABLE_CLR_REG_SYS_2	0x308
> > > >  #define STATUS_REG_SYS_2	0x508
> > > >  #define STATUS_CLR_REG_SYS_2	0x708
> > > >  #define LINK_DOWN		BIT(1)
> > > > @@ -116,6 +118,15 @@ static irqreturn_t j721e_pcie_link_irq_handler(int irq, void *priv)
> > > >  	return IRQ_HANDLED;
> > > >  }
> > > >  
> > > > +static void j721e_pcie_disable_link_irq(struct j721e_pcie *pcie)
> > > > +{
> > > > +	u32 reg;
> > > > +
> > > > +	reg = j721e_pcie_intd_readl(pcie, ENABLE_CLR_REG_SYS_2);
> > > > +	reg |= pcie->linkdown_irq_regfield;
> > > > +	j721e_pcie_intd_writel(pcie, ENABLE_CLR_REG_SYS_2, reg);
> > > > +}
> > > > +
> > > >  static void j721e_pcie_config_link_irq(struct j721e_pcie *pcie)
> > > >  {
> > > >  	u32 reg;
> > > > @@ -633,9 +644,25 @@ static void j721e_pcie_remove(struct platform_device *pdev)
> > > >  	struct j721e_pcie *pcie = platform_get_drvdata(pdev);
> > > >  	struct cdns_pcie *cdns_pcie = pcie->cdns_pcie;
> > > >  	struct device *dev = &pdev->dev;
> > > > +	struct cdns_pcie_ep *ep;
> > > > +	struct cdns_pcie_rc *rc;
> > > > +
> > > > +	if (pcie->mode == PCI_MODE_RC) {
> > > > +		rc = container_of(cdns_pcie, struct cdns_pcie_rc, pcie);
> > > > +		cdns_pcie_host_disable(rc);
> > > > +	} else {
> > > > +		ep = container_of(cdns_pcie, struct cdns_pcie_ep, pcie);
> > > > +		cdns_pcie_ep_disable(ep);
> > > > +	}
> > > > +
> > > > +	if (pcie->reset_gpio) {
> > > > +		msleep(PCIE_T_PVPERL_MS);
> > > 
> > > There is no point in adding a delay before PERST# assertion.
> > 
> > True :)
> > 
> > > 
> > > > +		gpiod_set_value_cansleep(pcie->reset_gpio, 1);
> > > 
> > > This is not PERST# assert, isn't it? Typo?
> > 
> > It is PERST# assert.
> 
> Since the reset-gpios polarity is defined as GPIO_ACTIVE_HIGH in DT (which is
> wrong unless you have an external component that changes polarity), for PERST#
> assert, you need to set 0. If you set 1, then PERST# will be signalled as
> deassert.

I realize now that I have accidentally set it to 1 when it should have
been 0. It was meant to be PERST# assert since it is the .remove callback.
The v3 patch at:
https://patchwork.kernel.org/project/linux-pci/patch/20250410104426.463453-5-s-vadapalli@ti.com/
dropped the delay which is not required for PERST# assert, but the
polarity is still incorrect. I will fix it in the v4 series.

Thank you for noticing this and pointing it out.

Regards,
Siddharth.