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

[PATCH 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-20250306.

--------------------------
Series has been tested for
--------------------------
[1] Loading and Unloading the PCI J721E driver when operating in the
Root-Complex mode on J721E-EVM with an NVMe SSD connected to the PCIe
Connector. "hdparm" based reads of the NVMe SSD have been performed to
validated functionality before and after a module unload-load sequence
using modprobe. Logs:
https://gist.github.com/Siddharth-Vadapalli-at-TI/085fd24d416bab5dc7d798156ce130b3

[2] Loading and Unloading the PCI J721E driver when operating in the
Endpoint mode on J784S4-EVM with 6 Physical Functions configured in the
Endpoint and connected to the J721E-EVM as the Root-Complex. Logs:
https://gist.github.com/Siddharth-Vadapalli-at-TI/1ec568a76bc3ebc1082d434aab4ab00b

The following changes to arch/arm64/configs/defconfig were made to test
this series and will be posted as a patch after this series gets merged:
diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 3a3706db2982..0ca073141c3e 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -209,6 +209,12 @@ CONFIG_NFC=m
 CONFIG_NFC_NCI=m
 CONFIG_NFC_S3FWRN5_I2C=m
 CONFIG_PCI=y
+CONFIG_PCI_J721E=m
+CONFIG_PCI_J721E_HOST=m
+CONFIG_PCI_J721E_EP=m
+CONFIG_PCIE_CADENCE=m
+CONFIG_PCIE_CADENCE_HOST=m
+CONFIG_PCIE_CADENCE_EP=m
 CONFIG_PCIEPORTBUS=y
 CONFIG_PCIEAER=y
 CONFIG_PCI_IOV=y

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    |  37 +++++-
 .../pci/controller/cadence/pcie-cadence-ep.c  |  16 +++
 .../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, 194 insertions(+), 10 deletions(-)

-- 
2.34.1

[PATCH 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>
---
 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 a4f7ed04d38b..eeb2afdd223e 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>
@@ -752,3 +753,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 f5eeff834ec1..c671e25a481b 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 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>
---
 .../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 c671e25a481b..9068f140596a 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 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>
---
 drivers/pci/controller/cadence/pcie-cadence-ep.c | 10 ++++++++++
 drivers/pci/controller/cadence/pcie-cadence.h    |  5 +++++
 2 files changed, 15 insertions(+)

diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c
index eeb2afdd223e..85bec57fa5d9 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-ep.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c
@@ -646,6 +646,16 @@ 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_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 9068f140596a..c2b9dbb7e1db 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 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>
---
 drivers/pci/controller/cadence/Kconfig     |  6 ++--
 drivers/pci/controller/cadence/pci-j721e.c | 37 ++++++++++++++++++++--
 2 files changed, 38 insertions(+), 5 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 0341d51d6aed..0900e7dd6ac7 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;
@@ -632,9 +643,27 @@ 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);
+	}
+
+	if (pcie->refclk)
+		clk_disable_unprepare(pcie->refclk);
 
-	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);
 }
@@ -729,4 +758,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 1/4] PCI: cadence: Add support to build pcie-cadence library as a kernel module

[Manivannan Sadhasivam]

On Fri, Mar 07, 2025 at 04:01:25PM +0530, Siddharth Vadapalli wrote:
> 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>

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

Even though this was added earlier, looks like not needed.

- Mani

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

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

[Siddharth Vadapalli]

On Thu, Mar 13, 2025 at 11:14:16PM +0530, Manivannan Sadhasivam wrote:

Hello Mani,

> On Fri, Mar 07, 2025 at 04:01:25PM +0530, Siddharth Vadapalli wrote:
> > 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>
> 
> > ---
> >  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
> 
> Even though this was added earlier, looks like not needed.

Thank you for reviewing this patch.

drivers/pci/controller/cadence/Kconfig has the following:
...
	config PCIE_CADENCE_HOST
		bool
		depends on OF
		select IRQ_DOMAIN
		select PCIE_CADENCE
...
	config PCI_J721E_HOST
		bool "TI J721E PCIe controller (host mode)"
		depends on ARCH_K3 || COMPILE_TEST
		depends on OF
		select PCIE_CADENCE_HOST
		select PCI_J721E
...
So PCI_J721E_HOST selects PCIE_CADENCE_HOST which in turn selects
PCIE_CADENCE. As of now, none of these configs are enabled in
arm64-defconfig, and they also will not be accepted as built-in modules
as it will bloat the Linux Image for everyone. For that reason, they are
all being converted to loadable modules, and their configs will eventually
be enabled in arm64-defconfig as loadable modules.

Please let me know if I misunderstood your comment regarding the quoted
change not being required.

Regards,
Siddharth.

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

[Thomas Richard]

> +
> +	if (pcie->reset_gpio) {
> +		msleep(PCIE_T_PVPERL_MS);
> +		gpiod_set_value_cansleep(pcie->reset_gpio, 1);
> +	}
> +
> +	if (pcie->refclk)
> +		clk_disable_unprepare(pcie->refclk);
>  

Hello Siddharth,

I think clk_disable_unprepare() is a no-op if the clock is NULL, so the
if statement is useless.

https://elixir.bootlin.com/linux/v6.14-rc6/source/include/linux/clk.h#L1157
https://elixir.bootlin.com/linux/v6.14-rc6/source/drivers/clk/clk.c#L1237
https://elixir.bootlin.com/linux/v6.14-rc6/source/drivers/clk/clk.c#L1099

Regards,

Thomas

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

[Siddharth Vadapalli]

On Fri, Mar 14, 2025 at 10:03:01AM +0100, Thomas Richard wrote:

Hello Thomas,

> > +
> > +	if (pcie->reset_gpio) {
> > +		msleep(PCIE_T_PVPERL_MS);
> > +		gpiod_set_value_cansleep(pcie->reset_gpio, 1);
> > +	}
> > +
> > +	if (pcie->refclk)
> > +		clk_disable_unprepare(pcie->refclk);
> >  
> 
> Hello Siddharth,
> 
> I think clk_disable_unprepare() is a no-op if the clock is NULL, so the
> if statement is useless.
> 
> https://elixir.bootlin.com/linux/v6.14-rc6/source/include/linux/clk.h#L1157
> https://elixir.bootlin.com/linux/v6.14-rc6/source/drivers/clk/clk.c#L1237
> https://elixir.bootlin.com/linux/v6.14-rc6/source/drivers/clk/clk.c#L1099

Thank you for pointing it out. I will drop the unnecessary check in the
next version. I will wait for feedback on other patches in this series
before I post the next version.

Regards,
Siddharth.

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

[Manivannan Sadhasivam]

On Fri, Mar 14, 2025 at 12:24:44PM +0530, Siddharth Vadapalli wrote:
> On Thu, Mar 13, 2025 at 11:14:16PM +0530, Manivannan Sadhasivam wrote:
> 
> Hello Mani,
> 
> > On Fri, Mar 07, 2025 at 04:01:25PM +0530, Siddharth Vadapalli wrote:
> > > 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>
> > 
> > > ---
> > >  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
> > 
> > Even though this was added earlier, looks like not needed.
> 
> Thank you for reviewing this patch.
> 
> drivers/pci/controller/cadence/Kconfig has the following:
> ...
> 	config PCIE_CADENCE_HOST
> 		bool
> 		depends on OF
> 		select IRQ_DOMAIN
> 		select PCIE_CADENCE
> ...
> 	config PCI_J721E_HOST
> 		bool "TI J721E PCIe controller (host mode)"
> 		depends on ARCH_K3 || COMPILE_TEST
> 		depends on OF
> 		select PCIE_CADENCE_HOST
> 		select PCI_J721E
> ...
> So PCI_J721E_HOST selects PCIE_CADENCE_HOST which in turn selects
> PCIE_CADENCE. As of now, none of these configs are enabled in
> arm64-defconfig, and they also will not be accepted as built-in modules
> as it will bloat the Linux Image for everyone. For that reason, they are
> all being converted to loadable modules, and their configs will eventually
> be enabled in arm64-defconfig as loadable modules.
> 
> Please let me know if I misunderstood your comment regarding the quoted
> change not being required.
> 

Yes, you misunderstood indeed :) My earlier comment was about IRQ_DOMAIN symbol
which looked like not needed at all.

- Mani

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

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

[Siddharth Vadapalli]

On Tue, Mar 18, 2025 at 01:19:17PM +0530, Manivannan Sadhasivam wrote:
> On Fri, Mar 14, 2025 at 12:24:44PM +0530, Siddharth Vadapalli wrote:
> > On Thu, Mar 13, 2025 at 11:14:16PM +0530, Manivannan Sadhasivam wrote:
> > 
> > Hello Mani,
> > 
> > > On Fri, Mar 07, 2025 at 04:01:25PM +0530, Siddharth Vadapalli wrote:
> > > > 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>
> > > 
> > > > ---
> > > >  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
> > > 
> > > Even though this was added earlier, looks like not needed.
> > 
> > Thank you for reviewing this patch.
> > 
> > drivers/pci/controller/cadence/Kconfig has the following:
> > ...
> > 	config PCIE_CADENCE_HOST
> > 		bool
> > 		depends on OF
> > 		select IRQ_DOMAIN
> > 		select PCIE_CADENCE
> > ...
> > 	config PCI_J721E_HOST
> > 		bool "TI J721E PCIe controller (host mode)"
> > 		depends on ARCH_K3 || COMPILE_TEST
> > 		depends on OF
> > 		select PCIE_CADENCE_HOST
> > 		select PCI_J721E
> > ...
> > So PCI_J721E_HOST selects PCIE_CADENCE_HOST which in turn selects
> > PCIE_CADENCE. As of now, none of these configs are enabled in
> > arm64-defconfig, and they also will not be accepted as built-in modules
> > as it will bloat the Linux Image for everyone. For that reason, they are
> > all being converted to loadable modules, and their configs will eventually
> > be enabled in arm64-defconfig as loadable modules.
> > 
> > Please let me know if I misunderstood your comment regarding the quoted
> > change not being required.
> > 
> 
> Yes, you misunderstood indeed :) My earlier comment was about IRQ_DOMAIN symbol
> which looked like not needed at all.

Thank you for clarifying. I (mis)interpret your comment in the context of
the entire "config PCIE_CADENCE_HOST" block and the change made in it :)

Regards,
Siddharth.

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

[Manivannan Sadhasivam]

On Fri, Mar 07, 2025 at 04:01:26PM +0530, Siddharth Vadapalli wrote:
> 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>

- Mani

> ---
>  .../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 c671e25a481b..9068f140596a 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
> 

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

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

[Manivannan Sadhasivam]

On Fri, Mar 07, 2025 at 04:01:27PM +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>
> ---
>  drivers/pci/controller/cadence/pcie-cadence-ep.c | 10 ++++++++++
>  drivers/pci/controller/cadence/pcie-cadence.h    |  5 +++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> index eeb2afdd223e..85bec57fa5d9 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-ep.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> @@ -646,6 +646,16 @@ 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()

- Mani

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

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

[Siddharth Vadapalli]

On Tue, Mar 18, 2025 at 01:33:04PM +0530, Manivannan Sadhasivam wrote:

Hello Mani,

> On Fri, Mar 07, 2025 at 04:01:27PM +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>
> > ---
> >  drivers/pci/controller/cadence/pcie-cadence-ep.c | 10 ++++++++++
> >  drivers/pci/controller/cadence/pcie-cadence.h    |  5 +++++
> >  2 files changed, 15 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> > index eeb2afdd223e..85bec57fa5d9 100644
> > --- a/drivers/pci/controller/cadence/pcie-cadence-ep.c
> > +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> > @@ -646,6 +646,16 @@ 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()

I had initially planned to add this, but I noticed that it is not
invoked by dw_pcie_ep_deinit() within
drivers/pci/controller/dwc/pcie-designware-ep.c
Since cdns_pcie_ep_disable() is similar to dw_pcie_ep_deinit(), I
decided to drop it. Current callers of pci_epc_deinit_notify() are:
drivers/pci/controller/dwc/pcie-qcom-ep.c
drivers/pci/controller/dwc/pcie-tegra194.c
while there are many more users of dw_pcie_ep_deinit() that don't invoke
pci_epc_deinit_notify().

While I don't intend to justify dropping pci_epc_deinit_notify() in the
cleanup path, I wanted to check if this should be added to
dw_pcie_ep_deinit() as well. Or is it the case that dw_pcie_ep_deinit()
is different from cdns_pcie_ep_disable()? Please let me know.

Regards,
Siddharth.

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

[Peter Chen]

On 25-03-07 16:01:24, Siddharth Vadapalli wrote:
> EXTERNAL EMAIL
> 
> 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.

If the user doesn't enable PCIe controller device through DTS/ACPI,
that's doesn't matter.

> @@ -209,6 +209,12 @@ CONFIG_NFC=m
>  CONFIG_NFC_NCI=m
>  CONFIG_NFC_S3FWRN5_I2C=m
>  CONFIG_PCI=y
> +CONFIG_PCI_J721E=m
> +CONFIG_PCI_J721E_HOST=m
> +CONFIG_PCI_J721E_EP=m
> +CONFIG_PCIE_CADENCE=m
> +CONFIG_PCIE_CADENCE_HOST=m
> +CONFIG_PCIE_CADENCE_EP=m

The common Cadence configuration will be select if the glue layer's
configuration is select according to Kconfig.

Please do not set common configuration as module, some user may need
it as build-in like dw's. Considering the situation, the rootfs is at
NVMe.

Peter

>  CONFIG_PCIEPORTBUS=y
>  CONFIG_PCIEAER=y
>  CONFIG_PCI_IOV=y
> 
> 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    |  37 +++++-
>  .../pci/controller/cadence/pcie-cadence-ep.c  |  16 +++
>  .../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, 194 insertions(+), 10 deletions(-)
> 
> --
> 2.34.1
> 
> 

-- 

Best regards,
Peter

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

[Siddharth Vadapalli]

On Wed, Mar 19, 2025 at 02:09:03PM +0800, Peter Chen wrote:

Hello Peter,

> On 25-03-07 16:01:24, Siddharth Vadapalli wrote:
> > EXTERNAL EMAIL
> >
> > 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.
> 
> If the user doesn't enable PCIe controller device through DTS/ACPI,
> that's doesn't matter.

The Linux Image for arm64 systems built using:
arch/arm64/configs/defconfig
will not have support for the Cadence PCIe Controller and the PCIe J721e
driver, because these configs aren't enabled.

> 
> > @@ -209,6 +209,12 @@ CONFIG_NFC=m
> >  CONFIG_NFC_NCI=m
> >  CONFIG_NFC_S3FWRN5_I2C=m
> >  CONFIG_PCI=y
> > +CONFIG_PCI_J721E=m
> > +CONFIG_PCI_J721E_HOST=m
> > +CONFIG_PCI_J721E_EP=m
> > +CONFIG_PCIE_CADENCE=m
> > +CONFIG_PCIE_CADENCE_HOST=m
> > +CONFIG_PCIE_CADENCE_EP=m
> 
> The common Cadence configuration will be select if the glue layer's
> configuration is select according to Kconfig.
> 
> Please do not set common configuration as module, some user may need
> it as build-in like dw's. Considering the situation, the rootfs is at
> NVMe.

The common configuration at the moment is "DISABLED" i.e. no support for
the Cadence Controller at all. Which "user" are you referring to? This
series was introduced since having the drivers built-in was pushed back at:
https://lore.kernel.org/linux-arm-kernel/20250122145822.4ewsmkk6ztbeejzf@slashing/

Regards,
Siddharth.

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

[Peter Chen]

On 25-03-19 14:25:34, Siddharth Vadapalli wrote:
> > >
> > > 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.
> >
> > If the user doesn't enable PCIe controller device through DTS/ACPI,
> > that's doesn't matter.
> 
> The Linux Image for arm64 systems built using:
> arch/arm64/configs/defconfig
> will not have support for the Cadence PCIe Controller and the PCIe J721e
> driver, because these configs aren't enabled.
> 
> >
> > > @@ -209,6 +209,12 @@ CONFIG_NFC=m
> > >  CONFIG_NFC_NCI=m
> > >  CONFIG_NFC_S3FWRN5_I2C=m
> > >  CONFIG_PCI=y
> > > +CONFIG_PCI_J721E=m
> > > +CONFIG_PCI_J721E_HOST=m
> > > +CONFIG_PCI_J721E_EP=m
> > > +CONFIG_PCIE_CADENCE=m
> > > +CONFIG_PCIE_CADENCE_HOST=m
> > > +CONFIG_PCIE_CADENCE_EP=m
> >
> > The common Cadence configuration will be select if the glue layer's
> > configuration is select according to Kconfig.
> >
> > Please do not set common configuration as module, some user may need
> > it as build-in like dw's. Considering the situation, the rootfs is at
> > NVMe.
> 
> The common configuration at the moment is "DISABLED" i.e. no support for
> the Cadence Controller at all. Which "user" are you referring to? This
> series was introduced since having the drivers built-in was pushed back at:

We are using Cadence controller, and prepare upstream radxa-o6 board
whose rootfs is at PCIe NVMe.

You could build driver as module for TI glue layer, but don't force
other vendors using module as well, see dwc as an example please.

-- 

Best regards,
Peter

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

[manivannan.sadhasivam]

On Wed, Mar 19, 2025 at 05:31:01PM +0800, Peter Chen wrote:
> On 25-03-19 14:25:34, Siddharth Vadapalli wrote:
> > > >
> > > > 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.
> > >
> > > If the user doesn't enable PCIe controller device through DTS/ACPI,
> > > that's doesn't matter.
> > 
> > The Linux Image for arm64 systems built using:
> > arch/arm64/configs/defconfig
> > will not have support for the Cadence PCIe Controller and the PCIe J721e
> > driver, because these configs aren't enabled.
> > 
> > >
> > > > @@ -209,6 +209,12 @@ CONFIG_NFC=m
> > > >  CONFIG_NFC_NCI=m
> > > >  CONFIG_NFC_S3FWRN5_I2C=m
> > > >  CONFIG_PCI=y
> > > > +CONFIG_PCI_J721E=m
> > > > +CONFIG_PCI_J721E_HOST=m
> > > > +CONFIG_PCI_J721E_EP=m
> > > > +CONFIG_PCIE_CADENCE=m
> > > > +CONFIG_PCIE_CADENCE_HOST=m
> > > > +CONFIG_PCIE_CADENCE_EP=m
> > >
> > > The common Cadence configuration will be select if the glue layer's
> > > configuration is select according to Kconfig.
> > >
> > > Please do not set common configuration as module, some user may need
> > > it as build-in like dw's. Considering the situation, the rootfs is at
> > > NVMe.
> > 
> > The common configuration at the moment is "DISABLED" i.e. no support for
> > the Cadence Controller at all. Which "user" are you referring to? This
> > series was introduced since having the drivers built-in was pushed back at:
> 
> We are using Cadence controller, and prepare upstream radxa-o6 board
> whose rootfs is at PCIe NVMe.
> 

It doesn't matter. Only criteria AFAIK to build the driver as built-in in
defconfig is that it should be a depedency for console. For some time, storage
was also a dependency, but for sure PCIe is not.

Moreover, CONFIG_BLK_DEV_NVME is built as a module in ARM64 defconfig. So it
doesn't matter if you build PCIe controller driver as a built-in or not. You
need to load the NVMe driver somehow.

So please use initramfs.

> You could build driver as module for TI glue layer, but don't force
> other vendors using module as well, see dwc as an example please.
> 

DWC is a bad example here. Only reason the DWC drivers are not loadable is due
to the in-built MSI controller implementation as irqchip. People tend to build
the irqchip controllers as always built-in for some known issues. Even then some
driver developers prefer to built them as loadable module but suppress unbind to
avoid rmmoding the module.

- Mani

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

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

[Manivannan Sadhasivam]

On Tue, Mar 18, 2025 at 01:42:39PM +0530, Siddharth Vadapalli wrote:
> On Tue, Mar 18, 2025 at 01:33:04PM +0530, Manivannan Sadhasivam wrote:
> 
> Hello Mani,
> 
> > On Fri, Mar 07, 2025 at 04:01:27PM +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>
> > > ---
> > >  drivers/pci/controller/cadence/pcie-cadence-ep.c | 10 ++++++++++
> > >  drivers/pci/controller/cadence/pcie-cadence.h    |  5 +++++
> > >  2 files changed, 15 insertions(+)
> > > 
> > > diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> > > index eeb2afdd223e..85bec57fa5d9 100644
> > > --- a/drivers/pci/controller/cadence/pcie-cadence-ep.c
> > > +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> > > @@ -646,6 +646,16 @@ 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()
> 
> I had initially planned to add this, but I noticed that it is not
> invoked by dw_pcie_ep_deinit() within
> drivers/pci/controller/dwc/pcie-designware-ep.c
> Since cdns_pcie_ep_disable() is similar to dw_pcie_ep_deinit(), I
> decided to drop it. Current callers of pci_epc_deinit_notify() are:
> drivers/pci/controller/dwc/pcie-qcom-ep.c
> drivers/pci/controller/dwc/pcie-tegra194.c
> while there are many more users of dw_pcie_ep_deinit() that don't invoke
> pci_epc_deinit_notify().
> 
> While I don't intend to justify dropping pci_epc_deinit_notify() in the
> cleanup path, I wanted to check if this should be added to
> dw_pcie_ep_deinit() as well. Or is it the case that dw_pcie_ep_deinit()
> is different from cdns_pcie_ep_disable()? Please let me know.
> 

Reason why it was not added to dw_pcie_ep_deinit() because, deinit_notify() is
supposed to be called while performing the resource cleanup with active refclk.

Some plaforms (Tegra, Qcom) depend on refclk from host. So if deinit_notify() is
called when there is no refclk, it will crash the endpoint SoC. But since
cadence endpoint platforms seem to generate their own refclk, you can call
deinit_notify() during deinit phase.

That said, I noticed some issues in the DWC cleanup path while checking the code
now. Will submit fixes for them.

- Mani

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

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

[Siddharth Vadapalli]

On Wed, Mar 19, 2025 at 04:02:17PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Mar 18, 2025 at 01:42:39PM +0530, Siddharth Vadapalli wrote:
> > On Tue, Mar 18, 2025 at 01:33:04PM +0530, Manivannan Sadhasivam wrote:
> > 
> > Hello Mani,
> > 
> > > On Fri, Mar 07, 2025 at 04:01:27PM +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>
> > > > ---
> > > >  drivers/pci/controller/cadence/pcie-cadence-ep.c | 10 ++++++++++
> > > >  drivers/pci/controller/cadence/pcie-cadence.h    |  5 +++++
> > > >  2 files changed, 15 insertions(+)
> > > > 
> > > > diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> > > > index eeb2afdd223e..85bec57fa5d9 100644
> > > > --- a/drivers/pci/controller/cadence/pcie-cadence-ep.c
> > > > +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> > > > @@ -646,6 +646,16 @@ 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()
> > 
> > I had initially planned to add this, but I noticed that it is not
> > invoked by dw_pcie_ep_deinit() within
> > drivers/pci/controller/dwc/pcie-designware-ep.c
> > Since cdns_pcie_ep_disable() is similar to dw_pcie_ep_deinit(), I
> > decided to drop it. Current callers of pci_epc_deinit_notify() are:
> > drivers/pci/controller/dwc/pcie-qcom-ep.c
> > drivers/pci/controller/dwc/pcie-tegra194.c
> > while there are many more users of dw_pcie_ep_deinit() that don't invoke
> > pci_epc_deinit_notify().
> > 
> > While I don't intend to justify dropping pci_epc_deinit_notify() in the
> > cleanup path, I wanted to check if this should be added to
> > dw_pcie_ep_deinit() as well. Or is it the case that dw_pcie_ep_deinit()
> > is different from cdns_pcie_ep_disable()? Please let me know.
> > 
> 
> Reason why it was not added to dw_pcie_ep_deinit() because, deinit_notify() is
> supposed to be called while performing the resource cleanup with active refclk.
> 
> Some plaforms (Tegra, Qcom) depend on refclk from host. So if deinit_notify() is
> called when there is no refclk, it will crash the endpoint SoC. But since
> cadence endpoint platforms seem to generate their own refclk, you can call
> deinit_notify() during deinit phase.

Thank you for the clarification. I will add pci_epc_deinit_notify() to
cdns_pcie_ep_disable() in the v2 series.

Regards,
Siddharth.

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

[hans.zhang]

On 2025/3/19 17:55, manivannan.sadhasivam@linaro.org wrote:
> EXTERNAL EMAIL
> 
> On Wed, Mar 19, 2025 at 05:31:01PM +0800, Peter Chen wrote:
>> On 25-03-19 14:25:34, Siddharth Vadapalli wrote:
>>>>>
>>>>> 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.
>>>>
>>>> If the user doesn't enable PCIe controller device through DTS/ACPI,
>>>> that's doesn't matter.
>>>
>>> The Linux Image for arm64 systems built using:
>>> arch/arm64/configs/defconfig
>>> will not have support for the Cadence PCIe Controller and the PCIe J721e
>>> driver, because these configs aren't enabled.
>>>
>>>>
>>>>> @@ -209,6 +209,12 @@ CONFIG_NFC=m
>>>>>   CONFIG_NFC_NCI=m
>>>>>   CONFIG_NFC_S3FWRN5_I2C=m
>>>>>   CONFIG_PCI=y
>>>>> +CONFIG_PCI_J721E=m
>>>>> +CONFIG_PCI_J721E_HOST=m
>>>>> +CONFIG_PCI_J721E_EP=m
>>>>> +CONFIG_PCIE_CADENCE=m
>>>>> +CONFIG_PCIE_CADENCE_HOST=m
>>>>> +CONFIG_PCIE_CADENCE_EP=m
>>>>
>>>> The common Cadence configuration will be select if the glue layer's
>>>> configuration is select according to Kconfig.
>>>>
>>>> Please do not set common configuration as module, some user may need
>>>> it as build-in like dw's. Considering the situation, the rootfs is at
>>>> NVMe.
>>>
>>> The common configuration at the moment is "DISABLED" i.e. no support for
>>> the Cadence Controller at all. Which "user" are you referring to? This
>>> series was introduced since having the drivers built-in was pushed back at:
>>
>> We are using Cadence controller, and prepare upstream radxa-o6 board
>> whose rootfs is at PCIe NVMe.
>>
> 
> It doesn't matter. Only criteria AFAIK to build the driver as built-in in
> defconfig is that it should be a depedency for console. For some time, storage
> was also a dependency, but for sure PCIe is not.
> 
> Moreover, CONFIG_BLK_DEV_NVME is built as a module in ARM64 defconfig. So it
> doesn't matter if you build PCIe controller driver as a built-in or not. You
> need to load the NVMe driver somehow.
> 
> So please use initramfs.
> 
>> You could build driver as module for TI glue layer, but don't force
>> other vendors using module as well, see dwc as an example please.
>>
> 
> DWC is a bad example here. Only reason the DWC drivers are not loadable is due
> to the in-built MSI controller implementation as irqchip. People tend to build
> the irqchip controllers as always built-in for some known issues. Even then some
> driver developers prefer to built them as loadable module but suppress unbind to
> avoid rmmoding the module.
Hi Mani,

I think the MSI RTL module provided by Synopsys PCIe controller IP is 
not a standard operation. The reason for this MSI module is probably to 
be used by some cpus that do not have ITS(LPI interrupt) designed. Or 
RISC-V SOC, etc. MSI is defined as an MSI/MSIX interrupt that starts 
with a direct write memory access.

There are also SOC vendors that do not use the built-in MSI RTL module. 
Instead, MSI/MSIX interrupts are transmitted directly to the GIC's ITS 
module via the GIC V3/V4 interface. For example, RK3588, they do not use 
the PCIe controller built-in MSI module. Some Qualcomm platforms also 
modify the PCIe controller's built-in MSI modules to connect each of 
them to 32 SPI interrupts to the GIC. I was under the impression that 
the SDM845 was designed that way. The only explanation is that SPI 
interrupts are faster than LPI interrupts without having to look up some 
tables.

So the dwc driver can also compile to ko?

Best regards,
Hans

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

[hans.zhang]

On 2025/3/20 10:14, hans.zhang wrote:
> EXTERNAL EMAIL
> 
> On 2025/3/19 17:55, manivannan.sadhasivam@linaro.org wrote:
>> EXTERNAL EMAIL
>>
>> On Wed, Mar 19, 2025 at 05:31:01PM +0800, Peter Chen wrote:
>>> On 25-03-19 14:25:34, Siddharth Vadapalli wrote:
>>>>>>
>>>>>> 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.
>>>>>
>>>>> If the user doesn't enable PCIe controller device through DTS/ACPI,
>>>>> that's doesn't matter.
>>>>
>>>> The Linux Image for arm64 systems built using:
>>>> arch/arm64/configs/defconfig
>>>> will not have support for the Cadence PCIe Controller and the PCIe 
>>>> J721e
>>>> driver, because these configs aren't enabled.
>>>>
>>>>>
>>>>>> @@ -209,6 +209,12 @@ CONFIG_NFC=m
>>>>>>   CONFIG_NFC_NCI=m
>>>>>>   CONFIG_NFC_S3FWRN5_I2C=m
>>>>>>   CONFIG_PCI=y
>>>>>> +CONFIG_PCI_J721E=m
>>>>>> +CONFIG_PCI_J721E_HOST=m
>>>>>> +CONFIG_PCI_J721E_EP=m
>>>>>> +CONFIG_PCIE_CADENCE=m
>>>>>> +CONFIG_PCIE_CADENCE_HOST=m
>>>>>> +CONFIG_PCIE_CADENCE_EP=m
>>>>>
>>>>> The common Cadence configuration will be select if the glue layer's
>>>>> configuration is select according to Kconfig.
>>>>>
>>>>> Please do not set common configuration as module, some user may need
>>>>> it as build-in like dw's. Considering the situation, the rootfs is at
>>>>> NVMe.
>>>>
>>>> The common configuration at the moment is "DISABLED" i.e. no support 
>>>> for
>>>> the Cadence Controller at all. Which "user" are you referring to? This
>>>> series was introduced since having the drivers built-in was pushed 
>>>> back at:
>>>
>>> We are using Cadence controller, and prepare upstream radxa-o6 board
>>> whose rootfs is at PCIe NVMe.
>>>
>>
>> It doesn't matter. Only criteria AFAIK to build the driver as built-in in
>> defconfig is that it should be a depedency for console. For some time, 
>> storage
>> was also a dependency, but for sure PCIe is not.
>>
>> Moreover, CONFIG_BLK_DEV_NVME is built as a module in ARM64 defconfig. 
>> So it
>> doesn't matter if you build PCIe controller driver as a built-in or 
>> not. You
>> need to load the NVMe driver somehow.
>>
>> So please use initramfs.
>>
>>> You could build driver as module for TI glue layer, but don't force
>>> other vendors using module as well, see dwc as an example please.
>>>
>>
>> DWC is a bad example here. Only reason the DWC drivers are not 
>> loadable is due
>> to the in-built MSI controller implementation as irqchip. People tend 
>> to build
>> the irqchip controllers as always built-in for some known issues. Even 
>> then some
>> driver developers prefer to built them as loadable module but suppress 
>> unbind to
>> avoid rmmoding the module.
> Hi Mani,
> 
> I think the MSI RTL module provided by Synopsys PCIe controller IP is
> not a standard operation. The reason for this MSI module is probably to
> be used by some cpus that do not have ITS(LPI interrupt) designed. Or
> RISC-V SOC, etc. MSI is defined as an MSI/MSIX interrupt that starts
> with a direct write memory access.
> 
> There are also SOC vendors that do not use the built-in MSI RTL module.
> Instead, MSI/MSIX interrupts are transmitted directly to the GIC's ITS
> module via the GIC V3/V4 interface. For example, RK3588, they do not use
> the PCIe controller built-in MSI module. Some Qualcomm platforms also
> modify the PCIe controller's built-in MSI modules to connect each of
> them to 32 SPI interrupts to the GIC. I was under the impression that
> the SDM845 was designed that way. The only explanation is that SPI
> interrupts are faster than LPI interrupts without having to look up some
> tables.
> 
> So the dwc driver can also compile to ko?

Additional reason:

Often in SOC design, the RTL designer does not understand the dwc-driver 
behavior and causes some RTL bugs, and the software needs to workaround. 
  As a result, the dwc driver of build-in cannot be used, and the dwc 
driver needs to be modified, which will make it easier to compile the 
dwc driver to module.

Best regards,
Hans

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

[manivannan.sadhasivam]

On Thu, Mar 20, 2025 at 10:14:02AM +0800, hans.zhang wrote:
> 
> 
> On 2025/3/19 17:55, manivannan.sadhasivam@linaro.org wrote:
> > EXTERNAL EMAIL
> > 
> > On Wed, Mar 19, 2025 at 05:31:01PM +0800, Peter Chen wrote:
> > > On 25-03-19 14:25:34, Siddharth Vadapalli wrote:
> > > > > > 
> > > > > > 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.
> > > > > 
> > > > > If the user doesn't enable PCIe controller device through DTS/ACPI,
> > > > > that's doesn't matter.
> > > > 
> > > > The Linux Image for arm64 systems built using:
> > > > arch/arm64/configs/defconfig
> > > > will not have support for the Cadence PCIe Controller and the PCIe J721e
> > > > driver, because these configs aren't enabled.
> > > > 
> > > > > 
> > > > > > @@ -209,6 +209,12 @@ CONFIG_NFC=m
> > > > > >   CONFIG_NFC_NCI=m
> > > > > >   CONFIG_NFC_S3FWRN5_I2C=m
> > > > > >   CONFIG_PCI=y
> > > > > > +CONFIG_PCI_J721E=m
> > > > > > +CONFIG_PCI_J721E_HOST=m
> > > > > > +CONFIG_PCI_J721E_EP=m
> > > > > > +CONFIG_PCIE_CADENCE=m
> > > > > > +CONFIG_PCIE_CADENCE_HOST=m
> > > > > > +CONFIG_PCIE_CADENCE_EP=m
> > > > > 
> > > > > The common Cadence configuration will be select if the glue layer's
> > > > > configuration is select according to Kconfig.
> > > > > 
> > > > > Please do not set common configuration as module, some user may need
> > > > > it as build-in like dw's. Considering the situation, the rootfs is at
> > > > > NVMe.
> > > > 
> > > > The common configuration at the moment is "DISABLED" i.e. no support for
> > > > the Cadence Controller at all. Which "user" are you referring to? This
> > > > series was introduced since having the drivers built-in was pushed back at:
> > > 
> > > We are using Cadence controller, and prepare upstream radxa-o6 board
> > > whose rootfs is at PCIe NVMe.
> > > 
> > 
> > It doesn't matter. Only criteria AFAIK to build the driver as built-in in
> > defconfig is that it should be a depedency for console. For some time, storage
> > was also a dependency, but for sure PCIe is not.
> > 
> > Moreover, CONFIG_BLK_DEV_NVME is built as a module in ARM64 defconfig. So it
> > doesn't matter if you build PCIe controller driver as a built-in or not. You
> > need to load the NVMe driver somehow.
> > 
> > So please use initramfs.
> > 
> > > You could build driver as module for TI glue layer, but don't force
> > > other vendors using module as well, see dwc as an example please.
> > > 
> > 
> > DWC is a bad example here. Only reason the DWC drivers are not loadable is due
> > to the in-built MSI controller implementation as irqchip. People tend to build
> > the irqchip controllers as always built-in for some known issues. Even then some
> > driver developers prefer to built them as loadable module but suppress unbind to
> > avoid rmmoding the module.
> Hi Mani,
> 
> I think the MSI RTL module provided by Synopsys PCIe controller IP is not a
> standard operation. The reason for this MSI module is probably to be used by
> some cpus that do not have ITS(LPI interrupt) designed. Or RISC-V SOC, etc.
> MSI is defined as an MSI/MSIX interrupt that starts with a direct write
> memory access.
> 

Yeah, DWC MSI controller is not a great design. The older ones are even more
horrible (using SPI interrupts for reporting AERs etc...).

> There are also SOC vendors that do not use the built-in MSI RTL module.
> Instead, MSI/MSIX interrupts are transmitted directly to the GIC's ITS
> module via the GIC V3/V4 interface. For example, RK3588, they do not use the
> PCIe controller built-in MSI module. Some Qualcomm platforms also modify the
> PCIe controller's built-in MSI modules to connect each of them to 32 SPI
> interrupts to the GIC. I was under the impression that the SDM845 was
> designed that way. The only explanation is that SPI interrupts are faster
> than LPI interrupts without having to look up some tables.
> 

If ITS is available, platforms should make use of that. There is no way DWC MSI
is superior than ITS. We are slowly migrating the Qcom platforms to use ITS.

And btw, Qcom DWC MSI controller raise interrupts for AER/PME sent by the
downstream components. So enabling ITS is uncovering AER errors which were
already present :)

> So the dwc driver can also compile to ko?
> 

Only if the MSI support is made as a build time option and there is a guarantee
that the platform will never use it (which is difficult to do as the driver can
only detect it during the runtime based on devicetree).

- Mani

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

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

[Hans Zhang]

On 2025/3/25 23:26, manivannan.sadhasivam@linaro.org wrote:
> EXTERNAL EMAIL
> 
> On Thu, Mar 20, 2025 at 10:14:02AM +0800, hans.zhang wrote:
>>
>>
>> On 2025/3/19 17:55, manivannan.sadhasivam@linaro.org wrote:
>>> EXTERNAL EMAIL
>>>
>>> On Wed, Mar 19, 2025 at 05:31:01PM +0800, Peter Chen wrote:
>>>> On 25-03-19 14:25:34, Siddharth Vadapalli wrote:
>>>>>>>
>>>>>>> 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.
>>>>>>
>>>>>> If the user doesn't enable PCIe controller device through DTS/ACPI,
>>>>>> that's doesn't matter.
>>>>>
>>>>> The Linux Image for arm64 systems built using:
>>>>> arch/arm64/configs/defconfig
>>>>> will not have support for the Cadence PCIe Controller and the PCIe J721e
>>>>> driver, because these configs aren't enabled.
>>>>>
>>>>>>
>>>>>>> @@ -209,6 +209,12 @@ CONFIG_NFC=m
>>>>>>>    CONFIG_NFC_NCI=m
>>>>>>>    CONFIG_NFC_S3FWRN5_I2C=m
>>>>>>>    CONFIG_PCI=y
>>>>>>> +CONFIG_PCI_J721E=m
>>>>>>> +CONFIG_PCI_J721E_HOST=m
>>>>>>> +CONFIG_PCI_J721E_EP=m
>>>>>>> +CONFIG_PCIE_CADENCE=m
>>>>>>> +CONFIG_PCIE_CADENCE_HOST=m
>>>>>>> +CONFIG_PCIE_CADENCE_EP=m
>>>>>>
>>>>>> The common Cadence configuration will be select if the glue layer's
>>>>>> configuration is select according to Kconfig.
>>>>>>
>>>>>> Please do not set common configuration as module, some user may need
>>>>>> it as build-in like dw's. Considering the situation, the rootfs is at
>>>>>> NVMe.
>>>>>
>>>>> The common configuration at the moment is "DISABLED" i.e. no support for
>>>>> the Cadence Controller at all. Which "user" are you referring to? This
>>>>> series was introduced since having the drivers built-in was pushed back at:
>>>>
>>>> We are using Cadence controller, and prepare upstream radxa-o6 board
>>>> whose rootfs is at PCIe NVMe.
>>>>
>>>
>>> It doesn't matter. Only criteria AFAIK to build the driver as built-in in
>>> defconfig is that it should be a depedency for console. For some time, storage
>>> was also a dependency, but for sure PCIe is not.
>>>
>>> Moreover, CONFIG_BLK_DEV_NVME is built as a module in ARM64 defconfig. So it
>>> doesn't matter if you build PCIe controller driver as a built-in or not. You
>>> need to load the NVMe driver somehow.
>>>
>>> So please use initramfs.
>>>
>>>> You could build driver as module for TI glue layer, but don't force
>>>> other vendors using module as well, see dwc as an example please.
>>>>
>>>
>>> DWC is a bad example here. Only reason the DWC drivers are not loadable is due
>>> to the in-built MSI controller implementation as irqchip. People tend to build
>>> the irqchip controllers as always built-in for some known issues. Even then some
>>> driver developers prefer to built them as loadable module but suppress unbind to
>>> avoid rmmoding the module.
>> Hi Mani,
>>
>> I think the MSI RTL module provided by Synopsys PCIe controller IP is not a
>> standard operation. The reason for this MSI module is probably to be used by
>> some cpus that do not have ITS(LPI interrupt) designed. Or RISC-V SOC, etc.
>> MSI is defined as an MSI/MSIX interrupt that starts with a direct write
>> memory access.
>>
> 
> Yeah, DWC MSI controller is not a great design. The older ones are even more
> horrible (using SPI interrupts for reporting AERs etc...).

Hi Mani,

Currently Synopsys and Cadence provide SPI interrupts for reporting AERs 
etc... This IP vendor only provides an alternative approach that 
actually requires SOC design companies to design according to PCIe SPEC 
and conform to linux OS software behavior.

I have a way to workaround AER is SPI interrupt. It can also use aer.c 
drivers. However, I have been afraid to submit patch, because this is a 
problem of SOC designers themselves, which does not conform to the port 
driver of linux os (aer.c). So it will certainly not be accepted.


> 
>> There are also SOC vendors that do not use the built-in MSI RTL module.
>> Instead, MSI/MSIX interrupts are transmitted directly to the GIC's ITS
>> module via the GIC V3/V4 interface. For example, RK3588, they do not use the
>> PCIe controller built-in MSI module. Some Qualcomm platforms also modify the
>> PCIe controller's built-in MSI modules to connect each of them to 32 SPI
>> interrupts to the GIC. I was under the impression that the SDM845 was
>> designed that way. The only explanation is that SPI interrupts are faster
>> than LPI interrupts without having to look up some tables.
>>
> 
> If ITS is available, platforms should make use of that. There is no way DWC MSI
> is superior than ITS. We are slowly migrating the Qcom platforms to use ITS.
> 

I agree with you.

> And btw, Qcom DWC MSI controller raise interrupts for AER/PME sent by the
> downstream components. So enabling ITS is uncovering AER errors which were
> already present :)
> 
>> So the dwc driver can also compile to ko?
>>
> 
> Only if the MSI support is made as a build time option and there is a guarantee
> that the platform will never use it (which is difficult to do as the driver can
> only detect it during the runtime based on devicetree).

Anyway, I would still like to request that the Cadence PCIe controller 
driver not be in module mode. Cadence also has a lot of customers, we 
are one of them, it's just that many customers don't have upstream. We 
are about to upstream.

This series was introduced since having the drivers built-in was pushed 
back at:
https://lore.kernel.org/linux-arm-kernel/20250122145822.4ewsmkk6ztbeejzf@slashing/

Hans:
The Cadence PCIe root port driver can not be made into module mode 
because of TI's idea. We should consider the ideas of other customers. 
If you have to make it module mode, I think all peripheral drivers 
should be module mode. Maybe I'm being direct, but that's probably the case.

Many thanks Mani for replying to me.

Best regards,
Hans

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

[manivannan.sadhasivam]

On Wed, Mar 26, 2025 at 12:03:01AM +0800, Hans Zhang wrote:
> 
> 
> On 2025/3/25 23:26, manivannan.sadhasivam@linaro.org wrote:
> > EXTERNAL EMAIL
> > 
> > On Thu, Mar 20, 2025 at 10:14:02AM +0800, hans.zhang wrote:
> > > 
> > > 
> > > On 2025/3/19 17:55, manivannan.sadhasivam@linaro.org wrote:
> > > > EXTERNAL EMAIL
> > > > 
> > > > On Wed, Mar 19, 2025 at 05:31:01PM +0800, Peter Chen wrote:
> > > > > On 25-03-19 14:25:34, Siddharth Vadapalli wrote:
> > > > > > > > 
> > > > > > > > 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.
> > > > > > > 
> > > > > > > If the user doesn't enable PCIe controller device through DTS/ACPI,
> > > > > > > that's doesn't matter.
> > > > > > 
> > > > > > The Linux Image for arm64 systems built using:
> > > > > > arch/arm64/configs/defconfig
> > > > > > will not have support for the Cadence PCIe Controller and the PCIe J721e
> > > > > > driver, because these configs aren't enabled.
> > > > > > 
> > > > > > > 
> > > > > > > > @@ -209,6 +209,12 @@ CONFIG_NFC=m
> > > > > > > >    CONFIG_NFC_NCI=m
> > > > > > > >    CONFIG_NFC_S3FWRN5_I2C=m
> > > > > > > >    CONFIG_PCI=y
> > > > > > > > +CONFIG_PCI_J721E=m
> > > > > > > > +CONFIG_PCI_J721E_HOST=m
> > > > > > > > +CONFIG_PCI_J721E_EP=m
> > > > > > > > +CONFIG_PCIE_CADENCE=m
> > > > > > > > +CONFIG_PCIE_CADENCE_HOST=m
> > > > > > > > +CONFIG_PCIE_CADENCE_EP=m
> > > > > > > 
> > > > > > > The common Cadence configuration will be select if the glue layer's
> > > > > > > configuration is select according to Kconfig.
> > > > > > > 
> > > > > > > Please do not set common configuration as module, some user may need
> > > > > > > it as build-in like dw's. Considering the situation, the rootfs is at
> > > > > > > NVMe.
> > > > > > 
> > > > > > The common configuration at the moment is "DISABLED" i.e. no support for
> > > > > > the Cadence Controller at all. Which "user" are you referring to? This
> > > > > > series was introduced since having the drivers built-in was pushed back at:
> > > > > 
> > > > > We are using Cadence controller, and prepare upstream radxa-o6 board
> > > > > whose rootfs is at PCIe NVMe.
> > > > > 
> > > > 
> > > > It doesn't matter. Only criteria AFAIK to build the driver as built-in in
> > > > defconfig is that it should be a depedency for console. For some time, storage
> > > > was also a dependency, but for sure PCIe is not.
> > > > 
> > > > Moreover, CONFIG_BLK_DEV_NVME is built as a module in ARM64 defconfig. So it
> > > > doesn't matter if you build PCIe controller driver as a built-in or not. You
> > > > need to load the NVMe driver somehow.
> > > > 
> > > > So please use initramfs.
> > > > 
> > > > > You could build driver as module for TI glue layer, but don't force
> > > > > other vendors using module as well, see dwc as an example please.
> > > > > 
> > > > 
> > > > DWC is a bad example here. Only reason the DWC drivers are not loadable is due
> > > > to the in-built MSI controller implementation as irqchip. People tend to build
> > > > the irqchip controllers as always built-in for some known issues. Even then some
> > > > driver developers prefer to built them as loadable module but suppress unbind to
> > > > avoid rmmoding the module.
> > > Hi Mani,
> > > 
> > > I think the MSI RTL module provided by Synopsys PCIe controller IP is not a
> > > standard operation. The reason for this MSI module is probably to be used by
> > > some cpus that do not have ITS(LPI interrupt) designed. Or RISC-V SOC, etc.
> > > MSI is defined as an MSI/MSIX interrupt that starts with a direct write
> > > memory access.
> > > 
> > 
> > Yeah, DWC MSI controller is not a great design. The older ones are even more
> > horrible (using SPI interrupts for reporting AERs etc...).
> 
> Hi Mani,
> 
> Currently Synopsys and Cadence provide SPI interrupts for reporting AERs
> etc... This IP vendor only provides an alternative approach that actually
> requires SOC design companies to design according to PCIe SPEC and conform
> to linux OS software behavior.
> 
> I have a way to workaround AER is SPI interrupt. It can also use aer.c
> drivers. However, I have been afraid to submit patch, because this is a
> problem of SOC designers themselves, which does not conform to the port
> driver of linux os (aer.c). So it will certainly not be accepted.
> 

Right. There is not clean way afaik.

> 
> > 
> > > There are also SOC vendors that do not use the built-in MSI RTL module.
> > > Instead, MSI/MSIX interrupts are transmitted directly to the GIC's ITS
> > > module via the GIC V3/V4 interface. For example, RK3588, they do not use the
> > > PCIe controller built-in MSI module. Some Qualcomm platforms also modify the
> > > PCIe controller's built-in MSI modules to connect each of them to 32 SPI
> > > interrupts to the GIC. I was under the impression that the SDM845 was
> > > designed that way. The only explanation is that SPI interrupts are faster
> > > than LPI interrupts without having to look up some tables.
> > > 
> > 
> > If ITS is available, platforms should make use of that. There is no way DWC MSI
> > is superior than ITS. We are slowly migrating the Qcom platforms to use ITS.
> > 
> 
> I agree with you.
> 
> > And btw, Qcom DWC MSI controller raise interrupts for AER/PME sent by the
> > downstream components. So enabling ITS is uncovering AER errors which were
> > already present :)
> > 
> > > So the dwc driver can also compile to ko?
> > > 
> > 
> > Only if the MSI support is made as a build time option and there is a guarantee
> > that the platform will never use it (which is difficult to do as the driver can
> > only detect it during the runtime based on devicetree).
> 
> Anyway, I would still like to request that the Cadence PCIe controller
> driver not be in module mode. Cadence also has a lot of customers, we are
> one of them, it's just that many customers don't have upstream. We are about
> to upstream.
> 
> This series was introduced since having the drivers built-in was pushed back
> at:
> https://lore.kernel.org/linux-arm-kernel/20250122145822.4ewsmkk6ztbeejzf@slashing/
> 
> Hans:
> The Cadence PCIe root port driver can not be made into module mode because
> of TI's idea. We should consider the ideas of other customers. If you have
> to make it module mode, I think all peripheral drivers should be module
> mode. Maybe I'm being direct, but that's probably the case.
> 

It is not about one company's idea to make the driver as a module. Linux kernel
community in general strongly encourages developers to build the drivers as
module unless there are exceptions (which I have already quoted). If you keep
building the drivers as built-in, it will result in a bloated kernel image. For
sure vendors would want *their* interested drivers to be built-in so that they
do not need to package the drivers in initramfs/rootfs, but that's not a
practice which is encouraged by the Linux community.

So I'm in favor of making the PCIe controllers as module if there are no
technical issues.

- Mani

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

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

[Hans Zhang]

On 2025/3/26 00:36, manivannan.sadhasivam@linaro.org wrote:
> EXTERNAL EMAIL
> 
> On Wed, Mar 26, 2025 at 12:03:01AM +0800, Hans Zhang wrote:
>>
>>
>> On 2025/3/25 23:26, manivannan.sadhasivam@linaro.org wrote:
>>> EXTERNAL EMAIL
>>>
>>> On Thu, Mar 20, 2025 at 10:14:02AM +0800, hans.zhang wrote:
>>>>
>>>>
>>>> On 2025/3/19 17:55, manivannan.sadhasivam@linaro.org wrote:
>>>>> EXTERNAL EMAIL
>>>>>
>>>>> On Wed, Mar 19, 2025 at 05:31:01PM +0800, Peter Chen wrote:
>>>>>> On 25-03-19 14:25:34, Siddharth Vadapalli wrote:
>>>>>>>>>
>>>>>>>>> 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.
>>>>>>>>
>>>>>>>> If the user doesn't enable PCIe controller device through DTS/ACPI,
>>>>>>>> that's doesn't matter.
>>>>>>>
>>>>>>> The Linux Image for arm64 systems built using:
>>>>>>> arch/arm64/configs/defconfig
>>>>>>> will not have support for the Cadence PCIe Controller and the PCIe J721e
>>>>>>> driver, because these configs aren't enabled.
>>>>>>>
>>>>>>>>
>>>>>>>>> @@ -209,6 +209,12 @@ CONFIG_NFC=m
>>>>>>>>>     CONFIG_NFC_NCI=m
>>>>>>>>>     CONFIG_NFC_S3FWRN5_I2C=m
>>>>>>>>>     CONFIG_PCI=y
>>>>>>>>> +CONFIG_PCI_J721E=m
>>>>>>>>> +CONFIG_PCI_J721E_HOST=m
>>>>>>>>> +CONFIG_PCI_J721E_EP=m
>>>>>>>>> +CONFIG_PCIE_CADENCE=m
>>>>>>>>> +CONFIG_PCIE_CADENCE_HOST=m
>>>>>>>>> +CONFIG_PCIE_CADENCE_EP=m
>>>>>>>>
>>>>>>>> The common Cadence configuration will be select if the glue layer's
>>>>>>>> configuration is select according to Kconfig.
>>>>>>>>
>>>>>>>> Please do not set common configuration as module, some user may need
>>>>>>>> it as build-in like dw's. Considering the situation, the rootfs is at
>>>>>>>> NVMe.
>>>>>>>
>>>>>>> The common configuration at the moment is "DISABLED" i.e. no support for
>>>>>>> the Cadence Controller at all. Which "user" are you referring to? This
>>>>>>> series was introduced since having the drivers built-in was pushed back at:
>>>>>>
>>>>>> We are using Cadence controller, and prepare upstream radxa-o6 board
>>>>>> whose rootfs is at PCIe NVMe.
>>>>>>
>>>>>
>>>>> It doesn't matter. Only criteria AFAIK to build the driver as built-in in
>>>>> defconfig is that it should be a depedency for console. For some time, storage
>>>>> was also a dependency, but for sure PCIe is not.
>>>>>
>>>>> Moreover, CONFIG_BLK_DEV_NVME is built as a module in ARM64 defconfig. So it
>>>>> doesn't matter if you build PCIe controller driver as a built-in or not. You
>>>>> need to load the NVMe driver somehow.
>>>>>
>>>>> So please use initramfs.
>>>>>
>>>>>> You could build driver as module for TI glue layer, but don't force
>>>>>> other vendors using module as well, see dwc as an example please.
>>>>>>
>>>>>
>>>>> DWC is a bad example here. Only reason the DWC drivers are not loadable is due
>>>>> to the in-built MSI controller implementation as irqchip. People tend to build
>>>>> the irqchip controllers as always built-in for some known issues. Even then some
>>>>> driver developers prefer to built them as loadable module but suppress unbind to
>>>>> avoid rmmoding the module.
>>>> Hi Mani,
>>>>
>>>> I think the MSI RTL module provided by Synopsys PCIe controller IP is not a
>>>> standard operation. The reason for this MSI module is probably to be used by
>>>> some cpus that do not have ITS(LPI interrupt) designed. Or RISC-V SOC, etc.
>>>> MSI is defined as an MSI/MSIX interrupt that starts with a direct write
>>>> memory access.
>>>>
>>>
>>> Yeah, DWC MSI controller is not a great design. The older ones are even more
>>> horrible (using SPI interrupts for reporting AERs etc...).
>>
>> Hi Mani,
>>
>> Currently Synopsys and Cadence provide SPI interrupts for reporting AERs
>> etc... This IP vendor only provides an alternative approach that actually
>> requires SOC design companies to design according to PCIe SPEC and conform
>> to linux OS software behavior.
>>
>> I have a way to workaround AER is SPI interrupt. It can also use aer.c
>> drivers. However, I have been afraid to submit patch, because this is a
>> problem of SOC designers themselves, which does not conform to the port
>> driver of linux os (aer.c). So it will certainly not be accepted.
>>
> 
> Right. There is not clean way afaik.
> 
>>
>>>
>>>> There are also SOC vendors that do not use the built-in MSI RTL module.
>>>> Instead, MSI/MSIX interrupts are transmitted directly to the GIC's ITS
>>>> module via the GIC V3/V4 interface. For example, RK3588, they do not use the
>>>> PCIe controller built-in MSI module. Some Qualcomm platforms also modify the
>>>> PCIe controller's built-in MSI modules to connect each of them to 32 SPI
>>>> interrupts to the GIC. I was under the impression that the SDM845 was
>>>> designed that way. The only explanation is that SPI interrupts are faster
>>>> than LPI interrupts without having to look up some tables.
>>>>
>>>
>>> If ITS is available, platforms should make use of that. There is no way DWC MSI
>>> is superior than ITS. We are slowly migrating the Qcom platforms to use ITS.
>>>
>>
>> I agree with you.
>>
>>> And btw, Qcom DWC MSI controller raise interrupts for AER/PME sent by the
>>> downstream components. So enabling ITS is uncovering AER errors which were
>>> already present :)
>>>
>>>> So the dwc driver can also compile to ko?
>>>>
>>>
>>> Only if the MSI support is made as a build time option and there is a guarantee
>>> that the platform will never use it (which is difficult to do as the driver can
>>> only detect it during the runtime based on devicetree).
>>
>> Anyway, I would still like to request that the Cadence PCIe controller
>> driver not be in module mode. Cadence also has a lot of customers, we are
>> one of them, it's just that many customers don't have upstream. We are about
>> to upstream.
>>
>> This series was introduced since having the drivers built-in was pushed back
>> at:
>> https://lore.kernel.org/linux-arm-kernel/20250122145822.4ewsmkk6ztbeejzf@slashing/
>>
>> Hans:
>> The Cadence PCIe root port driver can not be made into module mode because
>> of TI's idea. We should consider the ideas of other customers. If you have
>> to make it module mode, I think all peripheral drivers should be module
>> mode. Maybe I'm being direct, but that's probably the case.
>>
> 
> It is not about one company's idea to make the driver as a module. Linux kernel
> community in general strongly encourages developers to build the drivers as
> module unless there are exceptions (which I have already quoted). If you keep
> building the drivers as built-in, it will result in a bloated kernel image. For
> sure vendors would want *their* interested drivers to be built-in so that they
> do not need to package the drivers in initramfs/rootfs, but that's not a
> practice which is encouraged by the Linux community.
> 
> So I'm in favor of making the PCIe controllers as module if there are no
> technical issues.
> 

Hi Mani,

Okay, you must be more thoughtful than I am.

Best regards,
Hans

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

[Niklas Cassel]

Hello Mani,

On Wed, Mar 19, 2025 at 04:02:17PM +0530, Manivannan Sadhasivam wrote:
> > 
> > While I don't intend to justify dropping pci_epc_deinit_notify() in the
> > cleanup path, I wanted to check if this should be added to
> > dw_pcie_ep_deinit() as well. Or is it the case that dw_pcie_ep_deinit()
> > is different from cdns_pcie_ep_disable()? Please let me know.
> > 
> 
> Reason why it was not added to dw_pcie_ep_deinit() because, deinit_notify() is
> supposed to be called while performing the resource cleanup with active refclk.
> 
> Some plaforms (Tegra, Qcom) depend on refclk from host. So if deinit_notify() is
> called when there is no refclk, it will crash the endpoint SoC. But since
> cadence endpoint platforms seem to generate their own refclk, you can call
> deinit_notify() during deinit phase.
> 
> That said, I noticed some issues in the DWC cleanup path while checking the code
> now. Will submit fixes for them.

Could you please elaborate quickly what issues you found?


Kind regards,
Niklas

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

[Manivannan Sadhasivam]

On Tue, Apr 01, 2025 at 01:28:51PM +0200, Niklas Cassel wrote:
> Hello Mani,
> 
> On Wed, Mar 19, 2025 at 04:02:17PM +0530, Manivannan Sadhasivam wrote:
> > > 
> > > While I don't intend to justify dropping pci_epc_deinit_notify() in the
> > > cleanup path, I wanted to check if this should be added to
> > > dw_pcie_ep_deinit() as well. Or is it the case that dw_pcie_ep_deinit()
> > > is different from cdns_pcie_ep_disable()? Please let me know.
> > > 
> > 
> > Reason why it was not added to dw_pcie_ep_deinit() because, deinit_notify() is
> > supposed to be called while performing the resource cleanup with active refclk.
> > 
> > Some plaforms (Tegra, Qcom) depend on refclk from host. So if deinit_notify() is
> > called when there is no refclk, it will crash the endpoint SoC. But since
> > cadence endpoint platforms seem to generate their own refclk, you can call
> > deinit_notify() during deinit phase.
> > 
> > That said, I noticed some issues in the DWC cleanup path while checking the code
> > now. Will submit fixes for them.
> 
> Could you please elaborate quickly what issues you found?
> 

(1)

dw_pcie_ep_deinit()
	-> dw_pcie_ep_cleanup()
		-> dw_pcie_edma_remove()

But dw_pcie_ep_init() is not calling dw_pcie_edma_detect(). It is called by
dw_pcie_ep_init_registers(). So dw_pcie_ep_init() and dw_pcie_ep_deinit() not
symmetrical.

(2)

We are not calling pci_epc_deinit_notify() in non-PREST# supported controller
drivers. I think this could be fixed by moving it inside dw_pcie_ep_cleanup().

- Mani

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