Introduce PCIe Root Complex driver for ASPEED SoCs. Support RC
initialization, reset, clock, IRQ domain, and MSI domain setup.
Implement platform-specific setup and register configuration for
ASPEED. And provide PCI config space read/write and INTx/MSI
interrupt handling.

Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
---
 drivers/pci/controller/Kconfig       |   13 +
 drivers/pci/controller/Makefile      |    1 +
 drivers/pci/controller/pcie-aspeed.c | 1039 ++++++++++++++++++++++++++
 3 files changed, 1053 insertions(+)
 create mode 100644 drivers/pci/controller/pcie-aspeed.c

diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index 886f6f43a895..f6b5eea3b570 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -216,6 +216,19 @@ config PCIE_MT7621
 	help
 	  This selects a driver for the MediaTek MT7621 PCIe Controller.
 
+config PCIE_ASPEED
+	bool "ASPEED PCIe controller"
+	depends on PCI
+	depends on OF || COMPILE_TEST
+	select PCI_MSI_ARCH_FALLBACKS
+	help
+	  Enable this option to add support for the PCIe controller
+	  found on ASPEED SoCs.
+	  This driver provides initialization and management for PCIe
+	  Root Complex functionality, including interrupt and MSI support.
+	  Select Y if your platform uses an ASPEED SoC and requires PCIe
+	  connectivity.
+
 config PCI_HYPERV_INTERFACE
 	tristate "Microsoft Hyper-V PCI Interface"
 	depends on ((X86 && X86_64) || ARM64) && HYPERV && PCI_MSI
diff --git a/drivers/pci/controller/Makefile b/drivers/pci/controller/Makefile
index 038ccbd9e3ba..1339f88e153d 100644
--- a/drivers/pci/controller/Makefile
+++ b/drivers/pci/controller/Makefile
@@ -39,6 +39,7 @@ obj-$(CONFIG_PCI_LOONGSON) += pci-loongson.o
 obj-$(CONFIG_PCIE_HISI_ERR) += pcie-hisi-error.o
 obj-$(CONFIG_PCIE_APPLE) += pcie-apple.o
 obj-$(CONFIG_PCIE_MT7621) += pcie-mt7621.o
+obj-$(CONFIG_PCIE_ASPEED) += pcie-aspeed.o
 
 # pcie-hisi.o quirks are needed even without CONFIG_PCIE_DW
 obj-y				+= dwc/
diff --git a/drivers/pci/controller/pcie-aspeed.c b/drivers/pci/controller/pcie-aspeed.c
new file mode 100644
index 000000000000..c745684a7f9b
--- /dev/null
+++ b/drivers/pci/controller/pcie-aspeed.c
@@ -0,0 +1,1039 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2025 Aspeed Technology Inc.
+ */
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/mfd/syscon.h>
+#include <linux/kernel.h>
+#include <linux/msi.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/of_platform.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_pci.h>
+#include <linux/pci.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+#include <linux/irq.h>
+#include <linux/interrupt.h>
+#include <linux/workqueue.h>
+#include <linux/gpio/consumer.h>
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+
+#define MAX_MSI_HOST_IRQS	64
+
+/* AST2600 AHBC Registers */
+#define AHBC_KEY		0x00
+#define AHBC_UNLOCK_KEY			0xAEED1A03
+#define AHBC_UNLOCK			0x01
+#define AHBC_ADDR_MAPPING	0x8C
+#define PCIE_RC_MEMORY_EN		BIT(5)
+
+/* AST2600 PCIe Host Controller Registers */
+#define PEHR_MISC_10		0x10
+#define DATALINK_REPORT_CAPABLE		BIT(4)
+#define PEHR_GLOBAL		0x30
+#define RC_SYNC_RESET_DISABLE		BIT(20)
+#define PCIE_RC_SLOT_ENABLE		BIT(1)
+#define ROOT_COMPLEX_ID(x)		((x) << 4)
+#define PEHR_LOCK		0x7C
+#define PCIE_UNLOCK			0xa8
+#define PEHR_LINK		0xC0
+#define PCIE_LINK_STS			BIT(5)
+
+/* AST2600 H2X Controller Registers */
+/* Common Registers*/
+#define H2X_INT_STS		0x08
+#define PCIE_TX_IDLE_CLEAR		BIT(0)
+#define H2X_TX_DESC0		0x10
+#define H2X_TX_DESC1		0x14
+#define H2X_TX_DESC2		0x18
+#define H2X_TX_DESC3		0x1C
+#define H2X_TX_DESC_DATA	0x20
+#define H2X_STS			0x24
+#define PCIE_TX_IDLE			BIT(31)
+#define PCIE_STATUS_OF_TX		GENMASK(25, 24)
+#define	PCIE_RC_L_TX_COMPLETE		BIT(24)
+#define	PCIE_RC_H_TX_COMPLETE		BIT(25)
+#define PCIE_TRIGGER_TX			BIT(0)
+#define H2X_AHB_ADDR_CONFIG0	0x60
+#define AHB_REMAP_ADDR_31_20(x)	FIELD_PREP(GENMASK(15, 4), x)
+#define AHB_MASK_ADDR_31_20(x)	FIELD_PREP(GENMASK(31, 20), x)
+#define H2X_AHB_ADDR_CONFIG1	0x64
+#define AHB_REMAP_ADDR_64_32(x)	(x)
+#define H2X_AHB_ADDR_CONFIG2	0x68
+#define AHB_MASK_ADDR_64_32(x)	(x)
+/* Device Registers */
+#define H2X_DEV_CTRL		0x00
+#define PCIE_RX_DMA_EN			BIT(9)
+#define PCIE_RX_LINEAR			BIT(8)
+#define PCIE_RX_MSI_SEL			BIT(7)
+#define PCIE_RX_MSI_EN			BIT(6)
+#define PCIE_UNLOCK_RX_BUFF		BIT(4)
+#define PCIE_Wait_RX_TLP_CLR		BIT(2)
+#define PCIE_RC_RX_ENABLE		BIT(1)
+#define PCIE_RC_ENABLE			BIT(0)
+#define H2X_DEV_STS		0x08
+#define PCIE_RC_RX_DONE_ISR		BIT(4)
+#define H2X_DEV_RX_DESC_DATA	0x0C
+#define H2X_DEV_RX_DESC1	0x14
+#define H2X_DEV_TX_TAG		0x3C
+
+/* AST2700 H2X */
+#define H2X_CTRL		0x00
+#define H2X_BRIDGE_EN			BIT(0)
+#define H2X_BRIDGE_DIRECT_EN		BIT(1)
+#define H2X_CFGE_INT_STS	0x08
+#define CFGE_TX_IDLE			BIT(0)
+#define CFGE_RX_BUSY			BIT(1)
+#define H2X_CFGI_TLP		0x20
+#define H2X_CFGI_WR_DATA	0x24
+#define CFGI_WRITE			BIT(20)
+#define H2X_CFGI_CTRL		0x28
+#define CFGI_TLP_FIRE			BIT(0)
+#define H2X_CFGI_RET_DATA	0x2C
+#define H2X_CFGE_TLP_1ST	0x30
+#define H2X_CFGE_TLP_NEXT	0x34
+#define H2X_CFGE_CTRL		0x38
+#define CFGE_TLP_FIRE			BIT(0)
+#define H2X_CFGE_RET_DATA	0x3C
+#define H2X_REMAP_PREF_ADDR	0x70
+#define REMAP_PREF_ADDR_63_32(x)	(x)
+#define H2X_REMAP_DIRECT_ADDR	0x78
+#define REMAP_BAR_BASE(x)		(x)
+
+/* AST2700 PEHR */
+#define PEHR_VID_DID		0x00
+#define PEHR_MISC_58		0x58
+#define LOCAL_SCALE_SUP			BIT(0)
+#define PEHR_MISC_5C		0x5C
+#define CONFIG_RC_DEVICE		BIT(30)
+#define PEHR_MISC_60		0x60
+#define PORT_TPYE			GENMASK(7, 4)
+#define PORT_TYPE_ROOT			BIT(2)
+#define PEHR_MISC_70		0x70
+#define POSTED_DATA_CREDITS(x)		FIELD_PREP(GENMASK(15, 0), x)
+#define POSTED_HEADER_CREDITS(x)	FIELD_PREP(GENMASK(27, 16), x)
+#define PEHR_MISC_78		0x78
+#define COMPLETION_DATA_CREDITS(x)	FIELD_PREP(GENMASK(15, 0), x)
+#define COMPLETION_HEADER_CREDITS(x)	FIELD_PREP(GENMASK(27, 16), x)
+#define PEHR_MISC_300		0x300
+#define CAPABILITY_GEN2		BIT(0)
+#define PEHR_MISC_344		0x344
+#define LINK_STATUS_GEN2		BIT(18)
+#define PEHR_MISC_358		0x358
+#define LINK_STATUS_GEN4		BIT(8)
+
+/* AST2700 SCU */
+#define SCU_60			0x60
+#define RC_E2M_PATH_EN			BIT(0)
+#define RC_H2XS_PATH_EN			BIT(16)
+#define RC_H2XD_PATH_EN			BIT(17)
+#define RC_H2XX_PATH_EN			BIT(18)
+#define RC_UPSTREAM_MEM_EN		BIT(19)
+#define SCU_64			0x64
+#define RC0_DECODE_DMA_BASE(x)		FIELD_PREP(GENMASK(7, 0), x)
+#define RC0_DECODE_DMA_LIMIT(x)		FIELD_PREP(GENMASK(15, 8), x)
+#define RC1_DECODE_DMA_BASE(x)		FIELD_PREP(GENMASK(23, 16), x)
+#define RC1_DECODE_DMA_LIMIT(x)		FIELD_PREP(GENMASK(31, 24), x)
+#define SCU_70			0x70
+#define DISABLE_EP_FUNC			0x0
+
+/* TLP configuration type 0 and type 1 */
+#define CRG_READ_FMTTYPE(type)		(0x04000000 | (type << 24))
+#define CRG_WRITE_FMTTYPE(type)		(0x44000000 | (type << 24))
+#define CRG_PAYLOAD_SIZE		0x01 /* 1 DWORD */
+#define TLP_COMP_STATUS(s)		(((s) >> 13) & 7)
+#define TLP_BYTE_EN(x)			(x)
+#define AST2600_TX_DESC1_VALUE		0x00002000
+#define AST2700_TX_DESC1_VALUE		0x00401000
+
+struct aspeed_pcie_rc_platform {
+	int (*setup)(struct platform_device *pdev);
+	/* Interrupt Register Offset */
+	int reg_intx_en;
+	int reg_intx_sts;
+	int reg_msi_en;
+	int reg_msi_sts;
+};
+
+struct aspeed_pcie {
+	struct pci_host_bridge *host;
+	struct device *dev;
+	void __iomem *reg;
+	struct regmap *ahbc;
+	struct regmap *cfg;
+	struct regmap *pciephy;
+	struct clk *clock;
+	const struct aspeed_pcie_rc_platform *platform;
+
+	int domain;
+	u32 msi_address;
+	u8 tx_tag;
+
+	struct reset_control *h2xrst;
+	struct reset_control *perst;
+
+	struct irq_domain *irq_domain;
+	struct irq_domain *dev_domain;
+	struct irq_domain *msi_domain;
+	/* Protect bitmap variable */
+	struct mutex lock;
+
+	DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_HOST_IRQS);
+};
+
+static void aspeed_pcie_intx_ack_irq(struct irq_data *d)
+{
+	struct aspeed_pcie *pcie = irq_data_get_irq_chip_data(d);
+	int intx_en = pcie->platform->reg_intx_en;
+
+	writel(readl(pcie->reg + intx_en) | BIT(d->hwirq), pcie->reg + intx_en);
+}
+
+static void aspeed_pcie_intx_mask_irq(struct irq_data *d)
+{
+	struct aspeed_pcie *pcie = irq_data_get_irq_chip_data(d);
+	int intx_en = pcie->platform->reg_intx_en;
+
+	writel(readl(pcie->reg + intx_en) & ~BIT(d->hwirq), pcie->reg + intx_en);
+}
+
+static void aspeed_pcie_intx_unmask_irq(struct irq_data *d)
+{
+	struct aspeed_pcie *pcie = irq_data_get_irq_chip_data(d);
+	int intx_en = pcie->platform->reg_intx_en;
+
+	writel(readl(pcie->reg + intx_en) | BIT(d->hwirq), pcie->reg + intx_en);
+}
+
+static struct irq_chip aspeed_intx_irq_chip = {
+	.name = "ASPEED:IntX",
+	.irq_ack = aspeed_pcie_intx_ack_irq,
+	.irq_mask = aspeed_pcie_intx_mask_irq,
+	.irq_unmask = aspeed_pcie_intx_unmask_irq,
+};
+
+static int aspeed_pcie_intx_map(struct irq_domain *domain, unsigned int irq,
+				irq_hw_number_t hwirq)
+{
+	irq_set_chip_and_handler(irq, &aspeed_intx_irq_chip, handle_level_irq);
+	irq_set_chip_data(irq, domain->host_data);
+	irq_set_status_flags(irq, IRQ_LEVEL);
+
+	return 0;
+}
+
+static const struct irq_domain_ops aspeed_intx_domain_ops = {
+	.map = aspeed_pcie_intx_map,
+};
+
+static irqreturn_t aspeed_pcie_intr_handler(int irq, void *dev_id)
+{
+	struct aspeed_pcie *pcie = dev_id;
+	const struct aspeed_pcie_rc_platform *platform = pcie->platform;
+	unsigned long status;
+	unsigned long intx;
+	u32 bit;
+	int i;
+
+	intx = readl(pcie->reg + platform->reg_intx_sts) & 0xf;
+	if (intx) {
+		for_each_set_bit(bit, &intx, PCI_NUM_INTX)
+			generic_handle_domain_irq(pcie->irq_domain, bit);
+	}
+
+	if (IS_ENABLED(CONFIG_PCI_MSI)) {
+		for (i = 0; i < 2; i++) {
+			status = readl(pcie->reg + platform->reg_msi_sts + (i * 4));
+			writel(status, pcie->reg + platform->reg_msi_sts + (i * 4));
+
+			for_each_set_bit(bit, &status, 32) {
+				bit += (i * 32);
+				generic_handle_domain_irq(pcie->dev_domain, bit);
+			}
+		}
+	}
+
+	return IRQ_HANDLED;
+}
+
+static u32 aspeed_pcie_get_bdf_offset(struct pci_bus *bus, unsigned int devfn,
+				      int where)
+{
+	return ((bus->number) << 24) | (PCI_SLOT(devfn) << 19) |
+		(PCI_FUNC(devfn) << 16) | (where & ~3);
+}
+
+static int aspeed_ast2600_rd_conf(struct pci_bus *bus, unsigned int devfn,
+				  int where, int size, u32 *val)
+{
+	struct aspeed_pcie *pcie = bus->sysdata;
+	u32 bdf_offset;
+	int rx_done_fail = 0, slot = PCI_SLOT(devfn);
+	u32 cfg_val, isr, type = 0;
+	u32 link_sts = 0;
+	int ret;
+
+	/* Driver may set unlock RX buffere before triggering next TX config */
+	writel(PCIE_UNLOCK_RX_BUFF | readl(pcie->reg + H2X_DEV_CTRL),
+	       pcie->reg + H2X_DEV_CTRL);
+
+	if (bus->number == 128 && slot != 0 && slot != 8)
+		return PCIBIOS_DEVICE_NOT_FOUND;
+	type = (bus->number > 128);
+
+	if (type) {
+		regmap_read(pcie->pciephy, PEHR_LINK, &link_sts);
+		if (!(link_sts & PCIE_LINK_STS))
+			return PCIBIOS_DEVICE_NOT_FOUND;
+	}
+
+	bdf_offset = aspeed_pcie_get_bdf_offset(bus, devfn, where);
+
+	regmap_write(pcie->cfg, H2X_TX_DESC0, CRG_READ_FMTTYPE(type) | CRG_PAYLOAD_SIZE);
+	regmap_write(pcie->cfg, H2X_TX_DESC1,
+		     AST2600_TX_DESC1_VALUE | (pcie->tx_tag << 8) | TLP_BYTE_EN(0xf));
+	regmap_write(pcie->cfg, H2X_TX_DESC2, bdf_offset);
+	regmap_write(pcie->cfg, H2X_TX_DESC3, 0);
+
+	regmap_write_bits(pcie->cfg, H2X_STS, PCIE_TRIGGER_TX, PCIE_TRIGGER_TX);
+
+	ret = regmap_read_poll_timeout(pcie->cfg, H2X_STS, cfg_val,
+				       (cfg_val & PCIE_TX_IDLE), 0, 50);
+	if (ret) {
+		dev_err(pcie->dev,
+			"[%X:%02X:%02X.%02X]CR tx timeout sts: 0x%08x\n",
+			pcie->domain, bus->number, PCI_SLOT(devfn),
+			PCI_FUNC(devfn), cfg_val);
+		ret = PCIBIOS_SET_FAILED;
+		*val = ~0;
+		goto out;
+	}
+
+	regmap_write_bits(pcie->cfg, H2X_INT_STS, PCIE_TX_IDLE_CLEAR,
+			  PCIE_TX_IDLE_CLEAR);
+
+	regmap_read(pcie->cfg, H2X_STS, &cfg_val);
+	switch (cfg_val & PCIE_STATUS_OF_TX) {
+	case PCIE_RC_L_TX_COMPLETE:
+	case PCIE_RC_H_TX_COMPLETE:
+		ret = readl_poll_timeout(pcie->reg + H2X_DEV_STS, isr,
+					 (isr & PCIE_RC_RX_DONE_ISR), 0, 50);
+		if (ret) {
+			dev_err(pcie->dev,
+				"[%X:%02X:%02X.%02X]CR rx timeoutsts: 0x%08x\n",
+				pcie->domain, bus->number, PCI_SLOT(devfn),
+				PCI_FUNC(devfn), isr);
+			rx_done_fail = 1;
+			*val = ~0;
+		}
+		if (!rx_done_fail) {
+			if (readl(pcie->reg + H2X_DEV_RX_DESC1) & BIT(13))
+				*val = ~0;
+			else
+				*val = readl(pcie->reg + H2X_DEV_RX_DESC_DATA);
+		}
+
+		writel(PCIE_UNLOCK_RX_BUFF | readl(pcie->reg + H2X_DEV_CTRL),
+		       pcie->reg + H2X_DEV_CTRL);
+		break;
+	case PCIE_STATUS_OF_TX:
+		*val = ~0;
+		break;
+	default:
+		regmap_read(pcie->cfg, H2X_DEV_RX_DESC_DATA, &cfg_val);
+		*val = cfg_val;
+		break;
+	}
+
+	switch (size) {
+	case 1:
+		*val = (*val >> ((where & 3) * 8)) & 0xff;
+		break;
+	case 2:
+		*val = (*val >> ((where & 2) * 8)) & 0xffff;
+		break;
+	case 4:
+	default:
+		break;
+	}
+
+	ret = PCIBIOS_SUCCESSFUL;
+out:
+	writel(readl(pcie->reg + H2X_DEV_STS), pcie->reg + H2X_DEV_STS);
+	pcie->tx_tag = (pcie->tx_tag + 1) % 0x8;
+	return ret;
+}
+
+static int aspeed_ast2600_wr_conf(struct pci_bus *bus, unsigned int devfn,
+				  int where, int size, u32 val)
+{
+	u32 type = 0;
+	u32 shift = 8 * (where & 3);
+	u32 bdf_offset;
+	u8 byte_en = 0;
+	struct aspeed_pcie *pcie = bus->sysdata;
+	u32 isr, cfg_val;
+	int ret;
+
+	/* Driver may set unlock RX buffere before triggering next TX config */
+	writel(PCIE_UNLOCK_RX_BUFF | readl(pcie->reg + H2X_DEV_CTRL),
+	       pcie->reg + H2X_DEV_CTRL);
+
+	switch (size) {
+	case 1:
+		byte_en = 1 << (where % 4);
+		val = (val & 0xff) << shift;
+		break;
+	case 2:
+		byte_en = 0x3 << (2 * ((where >> 1) % 2));
+		val = (val & 0xffff) << shift;
+		break;
+	case 4:
+	default:
+		byte_en = 0xf;
+		break;
+	}
+
+	type = (bus->number > 128);
+
+	bdf_offset = aspeed_pcie_get_bdf_offset(bus, devfn, where);
+
+	regmap_write(pcie->cfg, H2X_TX_DESC0, CRG_WRITE_FMTTYPE(type) | CRG_PAYLOAD_SIZE);
+	regmap_write(pcie->cfg, H2X_TX_DESC1,
+		     AST2600_TX_DESC1_VALUE | (pcie->tx_tag << 8) | TLP_BYTE_EN(byte_en));
+	regmap_write(pcie->cfg, H2X_TX_DESC2, bdf_offset);
+	regmap_write(pcie->cfg, H2X_TX_DESC3, 0);
+	regmap_write(pcie->cfg, H2X_TX_DESC_DATA, val);
+
+	regmap_write_bits(pcie->cfg, H2X_STS, PCIE_TRIGGER_TX, PCIE_TRIGGER_TX);
+
+	ret = regmap_read_poll_timeout(pcie->cfg, H2X_STS, cfg_val,
+				       (cfg_val & PCIE_TX_IDLE), 0, 50);
+	if (ret) {
+		dev_err(pcie->dev,
+			"[%X:%02X:%02X.%02X]CT tx timeout sts: 0x%08x\n",
+			pcie->domain, bus->number, PCI_SLOT(devfn),
+			PCI_FUNC(devfn), cfg_val);
+		ret = PCIBIOS_SET_FAILED;
+		goto out;
+	}
+
+	regmap_write_bits(pcie->cfg, H2X_INT_STS, PCIE_TX_IDLE_CLEAR,
+			  PCIE_TX_IDLE_CLEAR);
+
+	regmap_read(pcie->cfg, H2X_STS, &cfg_val);
+	switch (cfg_val & PCIE_STATUS_OF_TX) {
+	case PCIE_RC_L_TX_COMPLETE:
+	case PCIE_RC_H_TX_COMPLETE:
+		ret = readl_poll_timeout(pcie->reg + H2X_DEV_STS, isr,
+					 (isr & PCIE_RC_RX_DONE_ISR), 0, 50);
+		if (ret) {
+			dev_err(pcie->dev,
+				"[%X:%02X:%02X.%02X]CT rx timeout sts: 0x%08x\n",
+				pcie->domain, bus->number, PCI_SLOT(devfn),
+				PCI_FUNC(devfn), isr);
+			ret = PCIBIOS_SET_FAILED;
+			goto out;
+		}
+		break;
+	}
+
+	ret = PCIBIOS_SUCCESSFUL;
+out:
+	writel(readl(pcie->reg + H2X_DEV_STS), pcie->reg + H2X_DEV_STS);
+	pcie->tx_tag = (pcie->tx_tag + 1) % 0x8;
+	return ret;
+}
+
+static bool aspeed_ast2700_get_link(struct aspeed_pcie *pcie)
+{
+	u32 reg;
+	bool link;
+
+	regmap_read(pcie->pciephy, PEHR_MISC_300, &reg);
+	if (reg & CAPABILITY_GEN2) {
+		regmap_read(pcie->pciephy, PEHR_MISC_344, &reg);
+		link = !!(reg & LINK_STATUS_GEN2);
+	} else {
+		regmap_read(pcie->pciephy, PEHR_MISC_358, &reg);
+		link = !!(reg & LINK_STATUS_GEN4);
+	}
+
+	return link;
+}
+
+static int aspeed_ast2700_rd_conf(struct pci_bus *bus, unsigned int devfn,
+				  int where, int size, u32 *val)
+{
+	struct aspeed_pcie *pcie = bus->sysdata;
+	u32 bdf_offset, status;
+	u8 type;
+	int ret;
+
+	if ((bus->number == 0 && devfn != 0))
+		return PCIBIOS_DEVICE_NOT_FOUND;
+
+	if (bus->number == 0) {
+		/* Internal access to bridge */
+		writel(TLP_BYTE_EN(0xf) << 16 | (where & ~3), pcie->reg + H2X_CFGI_TLP);
+		writel(CFGI_TLP_FIRE, pcie->reg + H2X_CFGI_CTRL);
+		*val = readl(pcie->reg + H2X_CFGI_RET_DATA);
+	} else {
+		if (!aspeed_ast2700_get_link(pcie))
+			return PCIBIOS_DEVICE_NOT_FOUND;
+
+		bdf_offset = aspeed_pcie_get_bdf_offset(bus, devfn, where);
+
+		type = (bus->number == 1) ? PCI_HEADER_TYPE_NORMAL : PCI_HEADER_TYPE_BRIDGE;
+
+		writel(CRG_READ_FMTTYPE(type) | CRG_PAYLOAD_SIZE, pcie->reg + H2X_CFGE_TLP_1ST);
+		writel(AST2700_TX_DESC1_VALUE | (pcie->tx_tag << 8) | TLP_BYTE_EN(0xf),
+		       pcie->reg + H2X_CFGE_TLP_NEXT);
+		writel(bdf_offset, pcie->reg + H2X_CFGE_TLP_NEXT);
+		writel(CFGE_TX_IDLE | CFGE_RX_BUSY, pcie->reg + H2X_CFGE_INT_STS);
+		writel(CFGE_TLP_FIRE, pcie->reg + H2X_CFGE_CTRL);
+
+		ret = readl_poll_timeout(pcie->reg + H2X_CFGE_INT_STS, status,
+					 (status & CFGE_TX_IDLE), 0, 50);
+		if (ret) {
+			dev_err(pcie->dev,
+				"[%X:%02X:%02X.%02X]CR tx timeout sts: 0x%08x\n",
+				pcie->domain, bus->number, PCI_SLOT(devfn),
+				PCI_FUNC(devfn), status);
+			ret = PCIBIOS_SET_FAILED;
+			*val = ~0;
+			goto out;
+		}
+
+		ret = readl_poll_timeout(pcie->reg + H2X_CFGE_INT_STS, status,
+					 (status & CFGE_RX_BUSY), 0, 50000);
+		if (ret) {
+			dev_err(pcie->dev,
+				"[%X:%02X:%02X.%02X]CR rx timeoutsts: 0x%08x\n",
+				pcie->domain, bus->number, PCI_SLOT(devfn),
+				PCI_FUNC(devfn), status);
+			ret = PCIBIOS_SET_FAILED;
+			*val = ~0;
+			goto out;
+		}
+		*val = readl(pcie->reg + H2X_CFGE_RET_DATA);
+	}
+
+	switch (size) {
+	case 1:
+		*val = (*val >> ((where & 3) * 8)) & 0xff;
+		break;
+	case 2:
+		*val = (*val >> ((where & 2) * 8)) & 0xffff;
+		break;
+	case 4:
+	default:
+		break;
+	}
+
+	ret = PCIBIOS_SUCCESSFUL;
+out:
+	writel(status, pcie->reg + H2X_CFGE_INT_STS);
+	pcie->tx_tag = (pcie->tx_tag + 1) % 0xF;
+	return ret;
+}
+
+static int aspeed_ast2700_wr_conf(struct pci_bus *bus, unsigned int devfn,
+				  int where, int size, u32 val)
+{
+	struct aspeed_pcie *pcie = bus->sysdata;
+	u32 shift = 8 * (where & 3);
+	u8 byte_en;
+	u32 bdf_offset, status, type;
+	int ret;
+
+	if ((bus->number == 0 && devfn != 0))
+		return PCIBIOS_DEVICE_NOT_FOUND;
+
+	switch (size) {
+	case 1:
+		byte_en = 1 << (where % 4);
+		val = (val & 0xff) << shift;
+		break;
+	case 2:
+		byte_en = 0x3 << (2 * ((where >> 1) % 2));
+		val = (val & 0xffff) << shift;
+		break;
+	case 4:
+	default:
+		byte_en = 0xf;
+		break;
+	}
+
+	if (bus->number == 0) {
+		/* Internal access to bridge */
+		writel(CFGI_WRITE | TLP_BYTE_EN(byte_en) << 16 | (where & ~3),
+		       pcie->reg + H2X_CFGI_TLP);
+		writel(val, pcie->reg + H2X_CFGI_WR_DATA);
+		writel(CFGI_TLP_FIRE, pcie->reg + H2X_CFGI_CTRL);
+	} else {
+		if (!aspeed_ast2700_get_link(pcie))
+			return PCIBIOS_SET_FAILED;
+
+		bdf_offset = aspeed_pcie_get_bdf_offset(bus, devfn, where);
+
+		type = (bus->number == 1) ? PCI_HEADER_TYPE_NORMAL : PCI_HEADER_TYPE_BRIDGE;
+
+		writel(CRG_WRITE_FMTTYPE(type) | CRG_PAYLOAD_SIZE, pcie->reg + H2X_CFGE_TLP_1ST);
+		writel(AST2700_TX_DESC1_VALUE | (pcie->tx_tag << 8) | byte_en,
+		       pcie->reg + H2X_CFGE_TLP_NEXT);
+		writel(bdf_offset, pcie->reg + H2X_CFGE_TLP_NEXT);
+		writel(val, pcie->reg + H2X_CFGE_TLP_NEXT);
+		writel(CFGE_TX_IDLE | CFGE_RX_BUSY, pcie->reg + H2X_CFGE_INT_STS);
+		writel(CFGE_TLP_FIRE, pcie->reg + H2X_CFGE_CTRL);
+
+		ret = readl_poll_timeout(pcie->reg + H2X_CFGE_INT_STS, status,
+					 (status & CFGE_TX_IDLE), 0, 50);
+		if (ret) {
+			dev_err(pcie->dev,
+				"[%X:%02X:%02X.%02X]CT tx timeout sts: 0x%08x\n",
+				pcie->domain, bus->number, PCI_SLOT(devfn),
+				PCI_FUNC(devfn), status);
+			ret = PCIBIOS_SET_FAILED;
+			goto out;
+		}
+
+		ret = readl_poll_timeout(pcie->reg + H2X_CFGE_INT_STS, status,
+					 (status & CFGE_RX_BUSY), 0, 50000);
+		if (ret) {
+			dev_err(pcie->dev,
+				"[%X:%02X:%02X.%02X]CT rx timeout sts: 0x%08x\n",
+				pcie->domain, bus->number, PCI_SLOT(devfn),
+				PCI_FUNC(devfn), status);
+			ret = PCIBIOS_SET_FAILED;
+			goto out;
+		}
+
+		(void)readl(pcie->reg + H2X_CFGE_RET_DATA);
+	}
+
+	ret = PCIBIOS_SUCCESSFUL;
+out:
+	writel(status, pcie->reg + H2X_CFGE_INT_STS);
+	pcie->tx_tag = (pcie->tx_tag + 1) % 0xF;
+	return ret;
+}
+
+static struct pci_ops aspeed_ast2600_pcie_ops = {
+	.read = aspeed_ast2600_rd_conf,
+	.write = aspeed_ast2600_wr_conf,
+};
+
+static struct pci_ops aspeed_ast2700_pcie_ops = {
+	.read = aspeed_ast2700_rd_conf,
+	.write = aspeed_ast2700_wr_conf,
+};
+
+#ifdef CONFIG_PCI_MSI
+static void aspeed_msi_compose_msi_msg(struct irq_data *data,
+				       struct msi_msg *msg)
+{
+	struct aspeed_pcie *pcie = irq_data_get_irq_chip_data(data);
+
+	msg->address_hi = 0;
+	msg->address_lo = pcie->msi_address;
+	msg->data = data->hwirq;
+}
+
+static int aspeed_msi_set_affinity(struct irq_data *irq_data,
+				   const struct cpumask *mask, bool force)
+{
+	return -EINVAL;
+}
+
+static struct irq_chip aspeed_msi_bottom_irq_chip = {
+	.name = "ASPEED MSI",
+	.irq_compose_msi_msg = aspeed_msi_compose_msi_msg,
+	.irq_set_affinity = aspeed_msi_set_affinity,
+};
+
+static int aspeed_irq_msi_domain_alloc(struct irq_domain *domain,
+				       unsigned int virq, unsigned int nr_irqs,
+				       void *args)
+{
+	struct aspeed_pcie *pcie = domain->host_data;
+	int bit;
+	int i;
+
+	mutex_lock(&pcie->lock);
+
+	bit = bitmap_find_free_region(pcie->msi_irq_in_use, MAX_MSI_HOST_IRQS,
+				      get_count_order(nr_irqs));
+
+	mutex_unlock(&pcie->lock);
+
+	if (bit < 0)
+		return -ENOSPC;
+
+	for (i = 0; i < nr_irqs; i++) {
+		irq_domain_set_info(domain, virq + i, bit + i,
+				    &aspeed_msi_bottom_irq_chip,
+				    domain->host_data, handle_simple_irq, NULL,
+				    NULL);
+	}
+
+	return 0;
+}
+
+static void aspeed_irq_msi_domain_free(struct irq_domain *domain,
+				       unsigned int virq, unsigned int nr_irqs)
+{
+	struct irq_data *data = irq_domain_get_irq_data(domain, virq);
+	struct aspeed_pcie *pcie = irq_data_get_irq_chip_data(data);
+
+	mutex_lock(&pcie->lock);
+
+	bitmap_release_region(pcie->msi_irq_in_use, data->hwirq,
+			      get_count_order(nr_irqs));
+
+	mutex_unlock(&pcie->lock);
+}
+
+static const struct irq_domain_ops aspeed_msi_domain_ops = {
+	.alloc = aspeed_irq_msi_domain_alloc,
+	.free = aspeed_irq_msi_domain_free,
+};
+
+static struct irq_chip aspeed_msi_irq_chip = {
+	.name = "PCIe MSI",
+	.irq_enable = pci_msi_unmask_irq,
+	.irq_disable = pci_msi_mask_irq,
+	.irq_mask = pci_msi_mask_irq,
+	.irq_unmask = pci_msi_unmask_irq,
+};
+
+static struct msi_domain_info aspeed_msi_domain_info = {
+	.flags = (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
+		  MSI_FLAG_MULTI_PCI_MSI | MSI_FLAG_PCI_MSIX),
+	.chip = &aspeed_msi_irq_chip,
+};
+#endif
+
+static void aspeed_pcie_irq_domain_free(struct aspeed_pcie *pcie)
+{
+	if (pcie->irq_domain) {
+		irq_domain_remove(pcie->irq_domain);
+		pcie->irq_domain = NULL;
+	}
+#ifdef CONFIG_PCI_MSI
+	if (pcie->msi_domain) {
+		irq_domain_remove(pcie->msi_domain);
+		pcie->msi_domain = NULL;
+	}
+
+	if (pcie->dev_domain) {
+		irq_domain_remove(pcie->dev_domain);
+		pcie->dev_domain = NULL;
+	}
+#endif
+}
+
+static int aspeed_pcie_init_irq_domain(struct aspeed_pcie *pcie)
+{
+	struct device *dev = pcie->dev;
+	struct device_node *node = dev->of_node;
+	struct device_node *pcie_intc_node;
+	int ret;
+
+	pcie_intc_node = of_get_next_child(node, NULL);
+	if (!pcie_intc_node)
+		return dev_err_probe(dev, -ENODEV, "No PCIe Intc node found\n");
+
+	pcie->irq_domain =
+		irq_domain_add_linear(pcie_intc_node, PCI_NUM_INTX, &aspeed_intx_domain_ops, pcie);
+	of_node_put(pcie_intc_node);
+	if (!pcie->irq_domain) {
+		ret = dev_err_probe(dev, -ENOMEM, "failed to get an INTx IRQ domain\n");
+		goto err;
+	}
+
+	writel(0, pcie->reg + pcie->platform->reg_intx_en);
+	writel(~0, pcie->reg + pcie->platform->reg_intx_sts);
+
+#ifdef CONFIG_PCI_MSI
+	pcie->dev_domain =
+		irq_domain_add_linear(NULL, MAX_MSI_HOST_IRQS, &aspeed_msi_domain_ops, pcie);
+	if (!pcie->dev_domain) {
+		ret = dev_err_probe(pcie->dev, -ENOMEM, "failed to create IRQ domain\n");
+		goto err;
+	}
+
+	pcie->msi_domain = pci_msi_create_irq_domain(dev_fwnode(pcie->dev), &aspeed_msi_domain_info,
+						     pcie->dev_domain);
+	if (!pcie->msi_domain) {
+		ret = dev_err_probe(pcie->dev, -ENOMEM, "failed to create MSI domain\n");
+		goto err;
+	}
+
+	writel(~0, pcie->reg + pcie->platform->reg_msi_en);
+	writel(~0, pcie->reg + pcie->platform->reg_msi_en + 0x04);
+	writel(~0, pcie->reg + pcie->platform->reg_msi_sts);
+	writel(~0, pcie->reg + pcie->platform->reg_msi_sts + 0x04);
+#endif
+	return 0;
+err:
+	aspeed_pcie_irq_domain_free(pcie);
+	return ret;
+}
+
+static void aspeed_pcie_port_init(struct aspeed_pcie *pcie)
+{
+	u32 link_sts = 0;
+
+	regmap_write(pcie->pciephy, PEHR_LOCK, PCIE_UNLOCK);
+	regmap_write(pcie->pciephy, PEHR_GLOBAL, ROOT_COMPLEX_ID(0x3));
+
+	reset_control_deassert(pcie->perst);
+	mdelay(500);
+
+	writel(PCIE_RX_DMA_EN | PCIE_RX_LINEAR | PCIE_RX_MSI_SEL | PCIE_RX_MSI_EN |
+	       PCIE_Wait_RX_TLP_CLR | PCIE_RC_RX_ENABLE | PCIE_RC_ENABLE,
+	       pcie->reg + H2X_DEV_CTRL);
+
+	writel(0x28, pcie->reg + H2X_DEV_TX_TAG);
+
+	regmap_read(pcie->pciephy, PEHR_LINK, &link_sts);
+	if (link_sts & PCIE_LINK_STS)
+		dev_info(pcie->dev, "PCIe Link UP");
+	else
+		dev_info(pcie->dev, "PCIe Link DOWN");
+}
+
+static int aspeed_ast2600_setup(struct platform_device *pdev)
+{
+	struct aspeed_pcie *pcie = platform_get_drvdata(pdev);
+	struct device *dev = pcie->dev;
+
+	pcie->ahbc = syscon_regmap_lookup_by_phandle(dev->of_node, "aspeed,ahbc");
+	if (IS_ERR(pcie->ahbc))
+		return dev_err_probe(dev, PTR_ERR(pcie->ahbc), "failed to map ahbc base\n");
+
+	reset_control_assert(pcie->h2xrst);
+	mdelay(5);
+	reset_control_deassert(pcie->h2xrst);
+
+	regmap_write(pcie->ahbc, AHBC_KEY, AHBC_UNLOCK_KEY);
+	regmap_update_bits(pcie->ahbc, AHBC_ADDR_MAPPING, PCIE_RC_MEMORY_EN, PCIE_RC_MEMORY_EN);
+	regmap_write(pcie->ahbc, AHBC_KEY, AHBC_UNLOCK);
+
+	regmap_write(pcie->cfg, H2X_AHB_ADDR_CONFIG0,
+		     AHB_REMAP_ADDR_31_20(0x600) | AHB_MASK_ADDR_31_20(0xE00));
+	regmap_write(pcie->cfg, H2X_AHB_ADDR_CONFIG1, AHB_REMAP_ADDR_64_32(0));
+	regmap_write(pcie->cfg, H2X_AHB_ADDR_CONFIG2, AHB_MASK_ADDR_64_32(~0));
+
+	regmap_write(pcie->cfg, H2X_CTRL, H2X_BRIDGE_EN);
+
+	aspeed_pcie_port_init(pcie);
+
+	pcie->host->ops = &aspeed_ast2600_pcie_ops;
+
+	return 0;
+}
+
+static int aspeed_ast2700_setup(struct platform_device *pdev)
+{
+	struct aspeed_pcie *pcie = platform_get_drvdata(pdev);
+	u32 cfg_val;
+
+	reset_control_assert(pcie->perst);
+
+	regmap_write(pcie->pciephy, PEHR_MISC_70,
+		     POSTED_DATA_CREDITS(0xc0) | POSTED_HEADER_CREDITS(0xa));
+	regmap_write(pcie->pciephy, PEHR_MISC_78,
+		     COMPLETION_DATA_CREDITS(0x30) | COMPLETION_HEADER_CREDITS(0x8));
+	regmap_write(pcie->pciephy, PEHR_MISC_58, LOCAL_SCALE_SUP);
+
+	regmap_update_bits(pcie->cfg, SCU_60,
+			   RC_E2M_PATH_EN | RC_H2XS_PATH_EN | RC_H2XD_PATH_EN | RC_H2XX_PATH_EN |
+			   RC_UPSTREAM_MEM_EN,
+			   RC_E2M_PATH_EN | RC_H2XS_PATH_EN | RC_H2XD_PATH_EN | RC_H2XX_PATH_EN |
+			   RC_UPSTREAM_MEM_EN);
+	regmap_write(pcie->cfg, SCU_64,
+		     RC0_DECODE_DMA_BASE(0) | RC0_DECODE_DMA_LIMIT(0xFF) | RC1_DECODE_DMA_BASE(0) |
+		     RC1_DECODE_DMA_LIMIT(0xFF));
+	regmap_write(pcie->cfg, SCU_70, DISABLE_EP_FUNC);
+
+	reset_control_assert(pcie->h2xrst);
+	mdelay(10);
+	reset_control_deassert(pcie->h2xrst);
+
+	regmap_write(pcie->pciephy, PEHR_MISC_5C, CONFIG_RC_DEVICE);
+	regmap_read(pcie->pciephy, PEHR_MISC_60, &cfg_val);
+	regmap_write(pcie->pciephy, PEHR_MISC_60,
+		     (cfg_val & ~PORT_TPYE) | FIELD_PREP(PORT_TPYE, PORT_TYPE_ROOT));
+
+	writel(0, pcie->reg + H2X_CTRL);
+	writel(H2X_BRIDGE_EN | H2X_BRIDGE_DIRECT_EN, pcie->reg + H2X_CTRL);
+
+	/* The BAR mapping:
+	 * CPU Node0(domain 0): 0x60000000
+	 * CPU Node1(domain 1): 0x80000000
+	 * IO       (domain 2): 0xa0000000
+	 */
+	writel(REMAP_BAR_BASE(0x60000000 + (0x20000000 * pcie->domain)),
+	       pcie->reg + H2X_REMAP_DIRECT_ADDR);
+
+	/* Prepare for 64-bit BAR pref */
+	writel(REMAP_PREF_ADDR_63_32(0x3), pcie->reg + H2X_REMAP_PREF_ADDR);
+
+	reset_control_deassert(pcie->perst);
+	mdelay(1000);
+
+	pcie->host->ops = &aspeed_ast2700_pcie_ops;
+
+	if (aspeed_ast2700_get_link(pcie))
+		dev_info(pcie->dev, "PCIe Link UP");
+	else
+		dev_info(pcie->dev, "PCIe Link DOWN");
+
+	return 0;
+}
+
+static int aspeed_pcie_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct pci_host_bridge *host;
+	struct aspeed_pcie *pcie;
+	struct device_node *node = dev->of_node;
+	const void *md = of_device_get_match_data(dev);
+	int irq, ret;
+
+	if (!md)
+		return -ENODEV;
+
+	host = devm_pci_alloc_host_bridge(dev, sizeof(*pcie));
+	if (!host)
+		return -ENOMEM;
+
+	pcie = pci_host_bridge_priv(host);
+	pcie->dev = dev;
+	pcie->tx_tag = 0;
+	platform_set_drvdata(pdev, pcie);
+
+	pcie->platform = md;
+	pcie->host = host;
+
+	pcie->reg = devm_platform_ioremap_resource(pdev, 0);
+
+	of_property_read_u32(node, "msi_address", &pcie->msi_address);
+	of_property_read_u32(node, "linux,pci-domain", &pcie->domain);
+
+	pcie->cfg = syscon_regmap_lookup_by_phandle(dev->of_node, "aspeed,pciecfg");
+	if (IS_ERR(pcie->cfg))
+		return dev_err_probe(dev, PTR_ERR(pcie->cfg), "Failed to map pciecfg base\n");
+
+	pcie->pciephy = syscon_regmap_lookup_by_phandle(node, "aspeed,pciephy");
+	if (IS_ERR(pcie->pciephy))
+		return dev_err_probe(dev, PTR_ERR(pcie->pciephy), "Failed to map pciephy base\n");
+
+	pcie->h2xrst = devm_reset_control_get_exclusive(dev, "h2x");
+	if (IS_ERR(pcie->h2xrst))
+		return dev_err_probe(dev, PTR_ERR(pcie->h2xrst), "Failed to get h2x reset\n");
+
+	pcie->perst = devm_reset_control_get_exclusive(dev, "perst");
+	if (IS_ERR(pcie->perst))
+		return dev_err_probe(dev, PTR_ERR(pcie->perst), "Failed to get perst reset\n");
+
+	ret = pcie->platform->setup(pdev);
+	if (ret)
+		goto err_setup;
+
+	host->sysdata = pcie;
+
+	ret = aspeed_pcie_init_irq_domain(pcie);
+	if (ret)
+		goto err_irq_init;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		goto err_irq;
+
+	ret = devm_request_irq(dev, irq, aspeed_pcie_intr_handler, IRQF_SHARED, dev_name(dev),
+			       pcie);
+	if (ret)
+		goto err_irq;
+
+	pcie->clock = clk_get(dev, NULL);
+	if (IS_ERR(pcie->clock))
+		goto err_clk;
+	ret = clk_prepare_enable(pcie->clock);
+	if (ret)
+		goto err_clk_enable;
+
+	ret = pci_host_probe(host);
+	if (ret)
+		goto err_clk_enable;
+
+	return 0;
+
+err_clk_enable:
+	clk_put(pcie->clock);
+err_clk:
+err_irq:
+	aspeed_pcie_irq_domain_free(pcie);
+err_irq_init:
+err_setup:
+	return dev_err_probe(dev, ret, "Failed to setup PCIe RC\n");
+}
+
+static void aspeed_pcie_remove(struct platform_device *pdev)
+{
+	struct aspeed_pcie *pcie = platform_get_drvdata(pdev);
+
+	if (pcie->clock) {
+		clk_disable_unprepare(pcie->clock);
+		clk_put(pcie->clock);
+	}
+
+	pci_stop_root_bus(pcie->host->bus);
+	pci_remove_root_bus(pcie->host->bus);
+	aspeed_pcie_irq_domain_free(pcie);
+}
+
+static struct aspeed_pcie_rc_platform pcie_rc_ast2600 = {
+	.setup = aspeed_ast2600_setup,
+	.reg_intx_en = 0x04,
+	.reg_intx_sts = 0x08,
+	.reg_msi_en = 0x20,
+	.reg_msi_sts = 0x28,
+};
+
+static struct aspeed_pcie_rc_platform pcie_rc_ast2700 = {
+	.setup = aspeed_ast2700_setup,
+	.reg_intx_en = 0x40,
+	.reg_intx_sts = 0x48,
+	.reg_msi_en = 0x50,
+	.reg_msi_sts = 0x58,
+};
+
+static const struct of_device_id aspeed_pcie_of_match[] = {
+	{ .compatible = "aspeed,ast2600-pcie", .data = &pcie_rc_ast2600 },
+	{ .compatible = "aspeed,ast2700-pcie", .data = &pcie_rc_ast2700 },
+	{}
+};
+
+static struct platform_driver aspeed_pcie_driver = {
+	.driver = {
+		.name = "aspeed-pcie",
+		.suppress_bind_attrs = true,
+		.of_match_table = aspeed_pcie_of_match,
+	},
+	.probe = aspeed_pcie_probe,
+	.remove = aspeed_pcie_remove,
+};
+
+module_platform_driver(aspeed_pcie_driver);
+
+MODULE_AUTHOR("Jacky Chou <jacky_chou@aspeedtech.com>");
+MODULE_DESCRIPTION("ASPEED PCIe Root Complex");
+MODULE_LICENSE("GPL");
-- 
2.43.0

Hi Jacky,

kernel test robot noticed the following build errors:

[auto build test ERROR on pci/next]
[also build test ERROR on pci/for-linus robh/for-next linusw-pinctrl/devel linusw-pinctrl/for-next linus/master v6.16-rc2 next-20250618]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jacky-Chou/dt-bindings-phy-Add-document-for-ASPEED-PCIe-PHY/20250613-113331
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link:    https://lore.kernel.org/r/20250613033001.3153637-8-jacky_chou%40aspeedtech.com
patch subject: [PATCH 7/7] pci: aspeed: Add ASPEED PCIe host controller driver
config: x86_64-randconfig-007-20250619 (https://download.01.org/0day-ci/archive/20250619/202506191639.jNEto4NW-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250619/202506191639.jNEto4NW-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506191639.jNEto4NW-lkp@intel.com/

All errors (new ones prefixed by >>):

   ld: vmlinux.o: in function `arch_setup_msi_irqs':
>> drivers/pci/msi/legacy.c:31: undefined reference to `msi_domain_first_desc'
>> ld: drivers/pci/msi/legacy.c:31: undefined reference to `msi_next_desc'
   ld: vmlinux.o: in function `arch_teardown_msi_irqs':
   drivers/pci/msi/legacy.c:45: undefined reference to `msi_domain_first_desc'
   ld: drivers/pci/msi/legacy.c:45: undefined reference to `msi_next_desc'
   ld: vmlinux.o: in function `pci_msi_setup_check_result':
   drivers/pci/msi/legacy.c:60: undefined reference to `msi_domain_first_desc'
   ld: drivers/pci/msi/legacy.c:60: undefined reference to `msi_next_desc'
   ld: vmlinux.o: in function `pci_msi_legacy_setup_msi_irqs':
>> drivers/pci/msi/legacy.c:72: undefined reference to `msi_device_populate_sysfs'
   ld: vmlinux.o: in function `pci_msi_legacy_teardown_msi_irqs':
>> drivers/pci/msi/legacy.c:78: undefined reference to `msi_device_destroy_sysfs'


vim +31 drivers/pci/msi/legacy.c

a01e09ef123789 Thomas Gleixner 2021-12-06  18  
a01e09ef123789 Thomas Gleixner 2021-12-06  19  int __weak arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
a01e09ef123789 Thomas Gleixner 2021-12-06  20  {
a01e09ef123789 Thomas Gleixner 2021-12-06  21  	struct msi_desc *desc;
a01e09ef123789 Thomas Gleixner 2021-12-06  22  	int ret;
a01e09ef123789 Thomas Gleixner 2021-12-06  23  
a01e09ef123789 Thomas Gleixner 2021-12-06  24  	/*
a01e09ef123789 Thomas Gleixner 2021-12-06  25  	 * If an architecture wants to support multiple MSI, it needs to
a01e09ef123789 Thomas Gleixner 2021-12-06  26  	 * override arch_setup_msi_irqs()
a01e09ef123789 Thomas Gleixner 2021-12-06  27  	 */
a01e09ef123789 Thomas Gleixner 2021-12-06  28  	if (type == PCI_CAP_ID_MSI && nvec > 1)
a01e09ef123789 Thomas Gleixner 2021-12-06  29  		return 1;
a01e09ef123789 Thomas Gleixner 2021-12-06  30  
ae24e28fef1468 Thomas Gleixner 2021-12-06 @31  	msi_for_each_desc(desc, &dev->dev, MSI_DESC_NOTASSOCIATED) {
a01e09ef123789 Thomas Gleixner 2021-12-06  32  		ret = arch_setup_msi_irq(dev, desc);
a01e09ef123789 Thomas Gleixner 2021-12-06  33  		if (ret)
a01e09ef123789 Thomas Gleixner 2021-12-06  34  			return ret < 0 ? ret : -ENOSPC;
a01e09ef123789 Thomas Gleixner 2021-12-06  35  	}
a01e09ef123789 Thomas Gleixner 2021-12-06  36  
a01e09ef123789 Thomas Gleixner 2021-12-06  37  	return 0;
a01e09ef123789 Thomas Gleixner 2021-12-06  38  }
a01e09ef123789 Thomas Gleixner 2021-12-06  39  
a01e09ef123789 Thomas Gleixner 2021-12-06  40  void __weak arch_teardown_msi_irqs(struct pci_dev *dev)
a01e09ef123789 Thomas Gleixner 2021-12-06  41  {
a01e09ef123789 Thomas Gleixner 2021-12-06  42  	struct msi_desc *desc;
a01e09ef123789 Thomas Gleixner 2021-12-06  43  	int i;
a01e09ef123789 Thomas Gleixner 2021-12-06  44  
ae24e28fef1468 Thomas Gleixner 2021-12-06  45  	msi_for_each_desc(desc, &dev->dev, MSI_DESC_ASSOCIATED) {
a01e09ef123789 Thomas Gleixner 2021-12-06  46  		for (i = 0; i < desc->nvec_used; i++)
a01e09ef123789 Thomas Gleixner 2021-12-06  47  			arch_teardown_msi_irq(desc->irq + i);
a01e09ef123789 Thomas Gleixner 2021-12-06  48  	}
a01e09ef123789 Thomas Gleixner 2021-12-06  49  }
aa423ac4221abd Thomas Gleixner 2021-12-06  50  
60bf9b33c82c0e Thomas Gleixner 2021-12-06  51  static int pci_msi_setup_check_result(struct pci_dev *dev, int type, int ret)
60bf9b33c82c0e Thomas Gleixner 2021-12-06  52  {
ae24e28fef1468 Thomas Gleixner 2021-12-06  53  	struct msi_desc *desc;
60bf9b33c82c0e Thomas Gleixner 2021-12-06  54  	int avail = 0;
60bf9b33c82c0e Thomas Gleixner 2021-12-06  55  
60bf9b33c82c0e Thomas Gleixner 2021-12-06  56  	if (type != PCI_CAP_ID_MSIX || ret >= 0)
60bf9b33c82c0e Thomas Gleixner 2021-12-06  57  		return ret;
60bf9b33c82c0e Thomas Gleixner 2021-12-06  58  
60bf9b33c82c0e Thomas Gleixner 2021-12-06  59  	/* Scan the MSI descriptors for successfully allocated ones. */
ae24e28fef1468 Thomas Gleixner 2021-12-06  60  	msi_for_each_desc(desc, &dev->dev, MSI_DESC_ASSOCIATED)
60bf9b33c82c0e Thomas Gleixner 2021-12-06  61  		avail++;
ae24e28fef1468 Thomas Gleixner 2021-12-06  62  
60bf9b33c82c0e Thomas Gleixner 2021-12-06  63  	return avail ? avail : ret;
60bf9b33c82c0e Thomas Gleixner 2021-12-06  64  }
60bf9b33c82c0e Thomas Gleixner 2021-12-06  65  
aa423ac4221abd Thomas Gleixner 2021-12-06  66  int pci_msi_legacy_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
aa423ac4221abd Thomas Gleixner 2021-12-06  67  {
60bf9b33c82c0e Thomas Gleixner 2021-12-06  68  	int ret = arch_setup_msi_irqs(dev, nvec, type);
60bf9b33c82c0e Thomas Gleixner 2021-12-06  69  
ffd84485e6beb9 Thomas Gleixner 2021-12-10  70  	ret = pci_msi_setup_check_result(dev, type, ret);
ffd84485e6beb9 Thomas Gleixner 2021-12-10  71  	if (!ret)
ffd84485e6beb9 Thomas Gleixner 2021-12-10 @72  		ret = msi_device_populate_sysfs(&dev->dev);
ffd84485e6beb9 Thomas Gleixner 2021-12-10  73  	return ret;
aa423ac4221abd Thomas Gleixner 2021-12-06  74  }
aa423ac4221abd Thomas Gleixner 2021-12-06  75  
aa423ac4221abd Thomas Gleixner 2021-12-06  76  void pci_msi_legacy_teardown_msi_irqs(struct pci_dev *dev)
aa423ac4221abd Thomas Gleixner 2021-12-06  77  {
ffd84485e6beb9 Thomas Gleixner 2021-12-10 @78  	msi_device_destroy_sysfs(&dev->dev);

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Hi Jacky,

kernel test robot noticed the following build warnings:

[auto build test WARNING on pci/next]
[also build test WARNING on pci/for-linus robh/for-next linusw-pinctrl/devel linusw-pinctrl/for-next linus/master v6.16-rc1 next-20250613]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jacky-Chou/dt-bindings-phy-Add-document-for-ASPEED-PCIe-PHY/20250613-113331
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link:    https://lore.kernel.org/r/20250613033001.3153637-8-jacky_chou%40aspeedtech.com
patch subject: [PATCH 7/7] pci: aspeed: Add ASPEED PCIe host controller driver
config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20250614/202506140931.MWdyPxX1-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250614/202506140931.MWdyPxX1-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506140931.MWdyPxX1-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/pci/controller/pcie-aspeed.c:481:6: warning: variable 'status' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
     481 |         if (bus->number == 0) {
         |             ^~~~~~~~~~~~~~~~
   drivers/pci/controller/pcie-aspeed.c:541:9: note: uninitialized use occurs here
     541 |         writel(status, pcie->reg + H2X_CFGE_INT_STS);
         |                ^~~~~~
   drivers/pci/controller/pcie-aspeed.c:481:2: note: remove the 'if' if its condition is always false
     481 |         if (bus->number == 0) {
         |         ^~~~~~~~~~~~~~~~~~~~~~~
     482 |                 /* Internal access to bridge */
         |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     483 |                 writel(TLP_BYTE_EN(0xf) << 16 | (where & ~3), pcie->reg + H2X_CFGI_TLP);
         |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     484 |                 writel(CFGI_TLP_FIRE, pcie->reg + H2X_CFGI_CTRL);
         |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     485 |                 *val = readl(pcie->reg + H2X_CFGI_RET_DATA);
         |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     486 |         } else {
         |         ~~~~~~
   drivers/pci/controller/pcie-aspeed.c:474:24: note: initialize the variable 'status' to silence this warning
     474 |         u32 bdf_offset, status;
         |                               ^
         |                                = 0
   drivers/pci/controller/pcie-aspeed.c:573:6: warning: variable 'status' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
     573 |         if (bus->number == 0) {
         |             ^~~~~~~~~~~~~~~~
   drivers/pci/controller/pcie-aspeed.c:622:9: note: uninitialized use occurs here
     622 |         writel(status, pcie->reg + H2X_CFGE_INT_STS);
         |                ^~~~~~
   drivers/pci/controller/pcie-aspeed.c:573:2: note: remove the 'if' if its condition is always false
     573 |         if (bus->number == 0) {
         |         ^~~~~~~~~~~~~~~~~~~~~~~
     574 |                 /* Internal access to bridge */
         |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     575 |                 writel(CFGI_WRITE | TLP_BYTE_EN(byte_en) << 16 | (where & ~3),
         |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     576 |                        pcie->reg + H2X_CFGI_TLP);
         |                        ~~~~~~~~~~~~~~~~~~~~~~~~~~
     577 |                 writel(val, pcie->reg + H2X_CFGI_WR_DATA);
         |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     578 |                 writel(CFGI_TLP_FIRE, pcie->reg + H2X_CFGI_CTRL);
         |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     579 |         } else {
         |         ~~~~~~
   drivers/pci/controller/pcie-aspeed.c:552:24: note: initialize the variable 'status' to silence this warning
     552 |         u32 bdf_offset, status, type;
         |                               ^
         |                                = 0
   2 warnings generated.


vim +481 drivers/pci/controller/pcie-aspeed.c

   469	
   470	static int aspeed_ast2700_rd_conf(struct pci_bus *bus, unsigned int devfn,
   471					  int where, int size, u32 *val)
   472	{
   473		struct aspeed_pcie *pcie = bus->sysdata;
   474		u32 bdf_offset, status;
   475		u8 type;
   476		int ret;
   477	
   478		if ((bus->number == 0 && devfn != 0))
   479			return PCIBIOS_DEVICE_NOT_FOUND;
   480	
 > 481		if (bus->number == 0) {
   482			/* Internal access to bridge */
   483			writel(TLP_BYTE_EN(0xf) << 16 | (where & ~3), pcie->reg + H2X_CFGI_TLP);
   484			writel(CFGI_TLP_FIRE, pcie->reg + H2X_CFGI_CTRL);
   485			*val = readl(pcie->reg + H2X_CFGI_RET_DATA);
   486		} else {
   487			if (!aspeed_ast2700_get_link(pcie))
   488				return PCIBIOS_DEVICE_NOT_FOUND;
   489	
   490			bdf_offset = aspeed_pcie_get_bdf_offset(bus, devfn, where);
   491	
   492			type = (bus->number == 1) ? PCI_HEADER_TYPE_NORMAL : PCI_HEADER_TYPE_BRIDGE;
   493	
   494			writel(CRG_READ_FMTTYPE(type) | CRG_PAYLOAD_SIZE, pcie->reg + H2X_CFGE_TLP_1ST);
   495			writel(AST2700_TX_DESC1_VALUE | (pcie->tx_tag << 8) | TLP_BYTE_EN(0xf),
   496			       pcie->reg + H2X_CFGE_TLP_NEXT);
   497			writel(bdf_offset, pcie->reg + H2X_CFGE_TLP_NEXT);
   498			writel(CFGE_TX_IDLE | CFGE_RX_BUSY, pcie->reg + H2X_CFGE_INT_STS);
   499			writel(CFGE_TLP_FIRE, pcie->reg + H2X_CFGE_CTRL);
   500	
   501			ret = readl_poll_timeout(pcie->reg + H2X_CFGE_INT_STS, status,
   502						 (status & CFGE_TX_IDLE), 0, 50);
   503			if (ret) {
   504				dev_err(pcie->dev,
   505					"[%X:%02X:%02X.%02X]CR tx timeout sts: 0x%08x\n",
   506					pcie->domain, bus->number, PCI_SLOT(devfn),
   507					PCI_FUNC(devfn), status);
   508				ret = PCIBIOS_SET_FAILED;
   509				*val = ~0;
   510				goto out;
   511			}
   512	
   513			ret = readl_poll_timeout(pcie->reg + H2X_CFGE_INT_STS, status,
   514						 (status & CFGE_RX_BUSY), 0, 50000);
   515			if (ret) {
   516				dev_err(pcie->dev,
   517					"[%X:%02X:%02X.%02X]CR rx timeoutsts: 0x%08x\n",
   518					pcie->domain, bus->number, PCI_SLOT(devfn),
   519					PCI_FUNC(devfn), status);
   520				ret = PCIBIOS_SET_FAILED;
   521				*val = ~0;
   522				goto out;
   523			}
   524			*val = readl(pcie->reg + H2X_CFGE_RET_DATA);
   525		}
   526	
   527		switch (size) {
   528		case 1:
   529			*val = (*val >> ((where & 3) * 8)) & 0xff;
   530			break;
   531		case 2:
   532			*val = (*val >> ((where & 2) * 8)) & 0xffff;
   533			break;
   534		case 4:
   535		default:
   536			break;
   537		}
   538	
   539		ret = PCIBIOS_SUCCESSFUL;
   540	out:
   541		writel(status, pcie->reg + H2X_CFGE_INT_STS);
   542		pcie->tx_tag = (pcie->tx_tag + 1) % 0xF;
   543		return ret;
   544	}
   545	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

On Fri, Jun 13, 2025 at 11:30:01AM +0800, Jacky Chou wrote:
> Introduce PCIe Root Complex driver for ASPEED SoCs. Support RC
> initialization, reset, clock, IRQ domain, and MSI domain setup.
> Implement platform-specific setup and register configuration for
> ASPEED. And provide PCI config space read/write and INTx/MSI
> interrupt handling.

Make the subject match drivers/pci/controller/ style.

> +config PCIE_ASPEED
> +	bool "ASPEED PCIe controller"
> +	depends on PCI
> +	depends on OF || COMPILE_TEST
> +	select PCI_MSI_ARCH_FALLBACKS
> +	help
> +	  Enable this option to add support for the PCIe controller
> +	  found on ASPEED SoCs.
> +	  This driver provides initialization and management for PCIe
> +	  Root Complex functionality, including interrupt and MSI support.
> +	  Select Y if your platform uses an ASPEED SoC and requires PCIe
> +	  connectivity.

Alphabetize this entry by vendor to match the file.

Add blank line between paragraphs.

> + * Copyright 2025 Aspeed Technology Inc.

Settle on "ASPEED" or "Aspeed" in text/comment/etc to match corporate
style.  "aspeed" is good for the driver name, e.g., "PCI: aspeed: ..."
for the subject.

> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/kernel.h>
> +#include <linux/msi.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_pci.h>
> +#include <linux/pci.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +#include <linux/irq.h>
> +#include <linux/interrupt.h>
> +#include <linux/workqueue.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>

The trend is to alphabetize these #includes.

> +/* AST2600 PCIe Host Controller Registers */
> +#define PEHR_MISC_10		0x10
> +#define DATALINK_REPORT_CAPABLE		BIT(4)

Name register bits like these in a way that connects them with the
register.

> +static struct irq_chip aspeed_intx_irq_chip = {
> +	.name = "ASPEED:IntX",

Usual styling is "INTx".

> +	.irq_ack = aspeed_pcie_intx_ack_irq,
> +	.irq_mask = aspeed_pcie_intx_mask_irq,
> +	.irq_unmask = aspeed_pcie_intx_unmask_irq,

Name these functions to match the name of the function pointer, e.g.,
aspeed_pcie_intx_irq_ack() instead of aspeed_pcie_intx_ack_irq()
This makes grep/cscope more useful.

> +static irqreturn_t aspeed_pcie_intr_handler(int irq, void *dev_id)
> +{
> +	struct aspeed_pcie *pcie = dev_id;
> +	const struct aspeed_pcie_rc_platform *platform = pcie->platform;
> +	unsigned long status;
> +	unsigned long intx;
> +	u32 bit;
> +	int i;
> +
> +	intx = readl(pcie->reg + platform->reg_intx_sts) & 0xf;
> +	if (intx) {

Don't need "if (intx)" here; the loop is sufficient.

> +		for_each_set_bit(bit, &intx, PCI_NUM_INTX)
> +			generic_handle_domain_irq(pcie->irq_domain, bit);
> +	}

> +static int aspeed_ast2600_rd_conf(struct pci_bus *bus, unsigned int devfn,
> +				  int where, int size, u32 *val)
> +{
> +	struct aspeed_pcie *pcie = bus->sysdata;
> +	u32 bdf_offset;
> +	int rx_done_fail = 0, slot = PCI_SLOT(devfn);
> +	u32 cfg_val, isr, type = 0;
> +	u32 link_sts = 0;
> +	int ret;
> +
> +	/* Driver may set unlock RX buffere before triggering next TX config */

s/buffere/buffer/

> +	writel(PCIE_UNLOCK_RX_BUFF | readl(pcie->reg + H2X_DEV_CTRL),
> +	       pcie->reg + H2X_DEV_CTRL);
> +
> +	if (bus->number == 128 && slot != 0 && slot != 8)
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +	type = (bus->number > 128);

Weird.  What's all this?  Some kind of device you want to hide?
Deserves a hint about what's special.

> +	if (ret) {
> +		dev_err(pcie->dev,
> +			"[%X:%02X:%02X.%02X]CR tx timeout sts: 0x%08x\n",

Conventional format is "%04x:%02x:%02x.%d" (4-digit domain, lower-case
hex).

> +			pcie->domain, bus->number, PCI_SLOT(devfn),
> +			PCI_FUNC(devfn), cfg_val);
> +		ret = PCIBIOS_SET_FAILED;
> +		*val = ~0;

PCI_SET_ERROR_RESPONSE(val)

> +static int aspeed_ast2600_wr_conf(struct pci_bus *bus, unsigned int devfn,
> +				  int where, int size, u32 val)
> +{
> +	u32 type = 0;
> +	u32 shift = 8 * (where & 3);
> +	u32 bdf_offset;
> +	u8 byte_en = 0;
> +	struct aspeed_pcie *pcie = bus->sysdata;
> +	u32 isr, cfg_val;
> +	int ret;
> +
> +	/* Driver may set unlock RX buffere before triggering next TX config */

s/buffere/buffer/

I don't understand this; I suppose it requires hardware knowledge.

> +	writel(PCIE_UNLOCK_RX_BUFF | readl(pcie->reg + H2X_DEV_CTRL),
> +	       pcie->reg + H2X_DEV_CTRL);
> +
> +	switch (size) {
> +	case 1:
> +		byte_en = 1 << (where % 4);
> +		val = (val & 0xff) << shift;
> +		break;
> +	case 2:
> +		byte_en = 0x3 << (2 * ((where >> 1) % 2));
> +		val = (val & 0xffff) << shift;
> +		break;
> +	case 4:
> +	default:
> +		byte_en = 0xf;
> +		break;
> +	}
> +
> +	type = (bus->number > 128);

You're not allowed to *read* bus 128, dev 1, but you can write it?

> +static void aspeed_pcie_port_init(struct aspeed_pcie *pcie)
> +{
> +	u32 link_sts = 0;
> +
> +	regmap_write(pcie->pciephy, PEHR_LOCK, PCIE_UNLOCK);
> +	regmap_write(pcie->pciephy, PEHR_GLOBAL, ROOT_COMPLEX_ID(0x3));
> +
> +	reset_control_deassert(pcie->perst);
> +	mdelay(500);

Where did this come from?  Should be a #define with reference to a
spec.

> +static int aspeed_ast2700_setup(struct platform_device *pdev)
> +{
> +	struct aspeed_pcie *pcie = platform_get_drvdata(pdev);
> +	u32 cfg_val;
> +
> +	reset_control_assert(pcie->perst);
> +
> +	regmap_write(pcie->pciephy, PEHR_MISC_70,
> +		     POSTED_DATA_CREDITS(0xc0) | POSTED_HEADER_CREDITS(0xa));
> +	regmap_write(pcie->pciephy, PEHR_MISC_78,
> +		     COMPLETION_DATA_CREDITS(0x30) | COMPLETION_HEADER_CREDITS(0x8));
> +	regmap_write(pcie->pciephy, PEHR_MISC_58, LOCAL_SCALE_SUP);
> +
> +	regmap_update_bits(pcie->cfg, SCU_60,
> +			   RC_E2M_PATH_EN | RC_H2XS_PATH_EN | RC_H2XD_PATH_EN | RC_H2XX_PATH_EN |
> +			   RC_UPSTREAM_MEM_EN,
> +			   RC_E2M_PATH_EN | RC_H2XS_PATH_EN | RC_H2XD_PATH_EN | RC_H2XX_PATH_EN |
> +			   RC_UPSTREAM_MEM_EN);
> +	regmap_write(pcie->cfg, SCU_64,
> +		     RC0_DECODE_DMA_BASE(0) | RC0_DECODE_DMA_LIMIT(0xFF) | RC1_DECODE_DMA_BASE(0) |
> +		     RC1_DECODE_DMA_LIMIT(0xFF));
> +	regmap_write(pcie->cfg, SCU_70, DISABLE_EP_FUNC);
> +
> +	reset_control_assert(pcie->h2xrst);
> +	mdelay(10);

Source?

> +	reset_control_deassert(pcie->h2xrst);
> +
> +	regmap_write(pcie->pciephy, PEHR_MISC_5C, CONFIG_RC_DEVICE);
> +	regmap_read(pcie->pciephy, PEHR_MISC_60, &cfg_val);
> +	regmap_write(pcie->pciephy, PEHR_MISC_60,
> +		     (cfg_val & ~PORT_TPYE) | FIELD_PREP(PORT_TPYE, PORT_TYPE_ROOT));
> +
> +	writel(0, pcie->reg + H2X_CTRL);
> +	writel(H2X_BRIDGE_EN | H2X_BRIDGE_DIRECT_EN, pcie->reg + H2X_CTRL);
> +
> +	/* The BAR mapping:
> +	 * CPU Node0(domain 0): 0x60000000
> +	 * CPU Node1(domain 1): 0x80000000
> +	 * IO       (domain 2): 0xa0000000

Are these addresses or sizes?  Should they come from DT?  Maybe it's
something wired into the hardware?

> +	writel(REMAP_BAR_BASE(0x60000000 + (0x20000000 * pcie->domain)),
> +	       pcie->reg + H2X_REMAP_DIRECT_ADDR);
> +
> +	/* Prepare for 64-bit BAR pref */
> +	writel(REMAP_PREF_ADDR_63_32(0x3), pcie->reg + H2X_REMAP_PREF_ADDR);
> +
> +	reset_control_deassert(pcie->perst);
> +	mdelay(1000);

Source?

> +static int aspeed_pcie_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct pci_host_bridge *host;
> +	struct aspeed_pcie *pcie;
> +	struct device_node *node = dev->of_node;
> +	const void *md = of_device_get_match_data(dev);
> +	int irq, ret;
> +
> +	if (!md)
> +		return -ENODEV;
> +
> +	host = devm_pci_alloc_host_bridge(dev, sizeof(*pcie));
> +	if (!host)
> +		return -ENOMEM;
> +
> +	pcie = pci_host_bridge_priv(host);
> +	pcie->dev = dev;
> +	pcie->tx_tag = 0;
> +	platform_set_drvdata(pdev, pcie);
> +
> +	pcie->platform = md;
> +	pcie->host = host;
> +
> +	pcie->reg = devm_platform_ioremap_resource(pdev, 0);
> +
> +	of_property_read_u32(node, "msi_address", &pcie->msi_address);
> +	of_property_read_u32(node, "linux,pci-domain", &pcie->domain);
> +
> +	pcie->cfg = syscon_regmap_lookup_by_phandle(dev->of_node, "aspeed,pciecfg");
> +	if (IS_ERR(pcie->cfg))
> +		return dev_err_probe(dev, PTR_ERR(pcie->cfg), "Failed to map pciecfg base\n");
> +
> +	pcie->pciephy = syscon_regmap_lookup_by_phandle(node, "aspeed,pciephy");
> +	if (IS_ERR(pcie->pciephy))
> +		return dev_err_probe(dev, PTR_ERR(pcie->pciephy), "Failed to map pciephy base\n");
> +
> +	pcie->h2xrst = devm_reset_control_get_exclusive(dev, "h2x");
> +	if (IS_ERR(pcie->h2xrst))
> +		return dev_err_probe(dev, PTR_ERR(pcie->h2xrst), "Failed to get h2x reset\n");
> +
> +	pcie->perst = devm_reset_control_get_exclusive(dev, "perst");
> +	if (IS_ERR(pcie->perst))
> +		return dev_err_probe(dev, PTR_ERR(pcie->perst), "Failed to get perst reset\n");
> +
> +	ret = pcie->platform->setup(pdev);
> +	if (ret)
> +		goto err_setup;
> +
> +	host->sysdata = pcie;
> +
> +	ret = aspeed_pcie_init_irq_domain(pcie);
> +	if (ret)
> +		goto err_irq_init;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		goto err_irq;
> +
> +	ret = devm_request_irq(dev, irq, aspeed_pcie_intr_handler, IRQF_SHARED, dev_name(dev),
> +			       pcie);
> +	if (ret)
> +		goto err_irq;
> +
> +	pcie->clock = clk_get(dev, NULL);
> +	if (IS_ERR(pcie->clock))
> +		goto err_clk;
> +	ret = clk_prepare_enable(pcie->clock);
> +	if (ret)
> +		goto err_clk_enable;

We need to observe PCIE_T_RRS_READY_MS (or
PCIE_RESET_CONFIG_DEVICE_WAIT_MS or whatever name we eventually settle
on) before pci_host_probe() starts issuing config reads.  Maybe this
is accounted for by one of the sleeps above, but we need a generic
#define that we can look for.

> +	ret = pci_host_probe(host);
> +	if (ret)
> +		goto err_clk_enable;
> +
> +	return 0;

Sorry, I see there's a lot of duplication with comments from other
reviewers :)

Bjorn

On Fri, 13 Jun 2025, Jacky Chou wrote:

> Introduce PCIe Root Complex driver for ASPEED SoCs. Support RC
> initialization, reset, clock, IRQ domain, and MSI domain setup.
> Implement platform-specific setup and register configuration for
> ASPEED. And provide PCI config space read/write and INTx/MSI
> interrupt handling.
> 
> Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
> ---
>  drivers/pci/controller/Kconfig       |   13 +
>  drivers/pci/controller/Makefile      |    1 +
>  drivers/pci/controller/pcie-aspeed.c | 1039 ++++++++++++++++++++++++++
>  3 files changed, 1053 insertions(+)
>  create mode 100644 drivers/pci/controller/pcie-aspeed.c
> 
> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> index 886f6f43a895..f6b5eea3b570 100644
> --- a/drivers/pci/controller/Kconfig
> +++ b/drivers/pci/controller/Kconfig
> @@ -216,6 +216,19 @@ config PCIE_MT7621
>  	help
>  	  This selects a driver for the MediaTek MT7621 PCIe Controller.
>  
> +config PCIE_ASPEED
> +	bool "ASPEED PCIe controller"
> +	depends on PCI
> +	depends on OF || COMPILE_TEST
> +	select PCI_MSI_ARCH_FALLBACKS
> +	help
> +	  Enable this option to add support for the PCIe controller
> +	  found on ASPEED SoCs.
> +	  This driver provides initialization and management for PCIe
> +	  Root Complex functionality, including interrupt and MSI support.
> +	  Select Y if your platform uses an ASPEED SoC and requires PCIe
> +	  connectivity.
> +
>  config PCI_HYPERV_INTERFACE
>  	tristate "Microsoft Hyper-V PCI Interface"
>  	depends on ((X86 && X86_64) || ARM64) && HYPERV && PCI_MSI
> diff --git a/drivers/pci/controller/Makefile b/drivers/pci/controller/Makefile
> index 038ccbd9e3ba..1339f88e153d 100644
> --- a/drivers/pci/controller/Makefile
> +++ b/drivers/pci/controller/Makefile
> @@ -39,6 +39,7 @@ obj-$(CONFIG_PCI_LOONGSON) += pci-loongson.o
>  obj-$(CONFIG_PCIE_HISI_ERR) += pcie-hisi-error.o
>  obj-$(CONFIG_PCIE_APPLE) += pcie-apple.o
>  obj-$(CONFIG_PCIE_MT7621) += pcie-mt7621.o
> +obj-$(CONFIG_PCIE_ASPEED) += pcie-aspeed.o
>  
>  # pcie-hisi.o quirks are needed even without CONFIG_PCIE_DW
>  obj-y				+= dwc/
> diff --git a/drivers/pci/controller/pcie-aspeed.c b/drivers/pci/controller/pcie-aspeed.c
> new file mode 100644
> index 000000000000..c745684a7f9b
> --- /dev/null
> +++ b/drivers/pci/controller/pcie-aspeed.c
> @@ -0,0 +1,1039 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2025 Aspeed Technology Inc.
> + */
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/kernel.h>
> +#include <linux/msi.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_pci.h>
> +#include <linux/pci.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +#include <linux/irq.h>
> +#include <linux/interrupt.h>
> +#include <linux/workqueue.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>

Please order alphabetically.

> +
> +#define MAX_MSI_HOST_IRQS	64
> +
> +/* AST2600 AHBC Registers */
> +#define AHBC_KEY		0x00
> +#define AHBC_UNLOCK_KEY			0xAEED1A03
> +#define AHBC_UNLOCK			0x01
> +#define AHBC_ADDR_MAPPING	0x8C
> +#define PCIE_RC_MEMORY_EN		BIT(5)

Add include for BIT()

> +
> +/* AST2600 PCIe Host Controller Registers */
> +#define PEHR_MISC_10		0x10
> +#define DATALINK_REPORT_CAPABLE		BIT(4)
> +#define PEHR_GLOBAL		0x30
> +#define RC_SYNC_RESET_DISABLE		BIT(20)
> +#define PCIE_RC_SLOT_ENABLE		BIT(1)
> +#define ROOT_COMPLEX_ID(x)		((x) << 4)

Add field define with GENMASK() and use FIELD_PREP() ?

> +#define PEHR_LOCK		0x7C
> +#define PCIE_UNLOCK			0xa8
> +#define PEHR_LINK		0xC0
> +#define PCIE_LINK_STS			BIT(5)
> +
> +/* AST2600 H2X Controller Registers */
> +/* Common Registers*/
> +#define H2X_INT_STS		0x08
> +#define PCIE_TX_IDLE_CLEAR		BIT(0)
> +#define H2X_TX_DESC0		0x10
> +#define H2X_TX_DESC1		0x14
> +#define H2X_TX_DESC2		0x18
> +#define H2X_TX_DESC3		0x1C
> +#define H2X_TX_DESC_DATA	0x20
> +#define H2X_STS			0x24
> +#define PCIE_TX_IDLE			BIT(31)
> +#define PCIE_STATUS_OF_TX		GENMASK(25, 24)
> +#define	PCIE_RC_L_TX_COMPLETE		BIT(24)
> +#define	PCIE_RC_H_TX_COMPLETE		BIT(25)
> +#define PCIE_TRIGGER_TX			BIT(0)
> +#define H2X_AHB_ADDR_CONFIG0	0x60
> +#define AHB_REMAP_ADDR_31_20(x)	FIELD_PREP(GENMASK(15, 4), x)
> +#define AHB_MASK_ADDR_31_20(x)	FIELD_PREP(GENMASK(31, 20), x)

It might make more sense to name these fields with defines and use 
FIELD_PREP at the calling site instead.

> +#define H2X_AHB_ADDR_CONFIG1	0x64
> +#define AHB_REMAP_ADDR_64_32(x)	(x)
> +#define H2X_AHB_ADDR_CONFIG2	0x68
> +#define AHB_MASK_ADDR_64_32(x)	(x)

ADd empty line.

> +/* Device Registers */
> +#define H2X_DEV_CTRL		0x00
> +#define PCIE_RX_DMA_EN			BIT(9)
> +#define PCIE_RX_LINEAR			BIT(8)
> +#define PCIE_RX_MSI_SEL			BIT(7)
> +#define PCIE_RX_MSI_EN			BIT(6)
> +#define PCIE_UNLOCK_RX_BUFF		BIT(4)
> +#define PCIE_Wait_RX_TLP_CLR		BIT(2)

WAIT

> +#define PCIE_RC_RX_ENABLE		BIT(1)
> +#define PCIE_RC_ENABLE			BIT(0)
> +#define H2X_DEV_STS		0x08
> +#define PCIE_RC_RX_DONE_ISR		BIT(4)
> +#define H2X_DEV_RX_DESC_DATA	0x0C
> +#define H2X_DEV_RX_DESC1	0x14
> +#define H2X_DEV_TX_TAG		0x3C
> +
> +/* AST2700 H2X */
> +#define H2X_CTRL		0x00
> +#define H2X_BRIDGE_EN			BIT(0)
> +#define H2X_BRIDGE_DIRECT_EN		BIT(1)
> +#define H2X_CFGE_INT_STS	0x08
> +#define CFGE_TX_IDLE			BIT(0)
> +#define CFGE_RX_BUSY			BIT(1)
> +#define H2X_CFGI_TLP		0x20
> +#define H2X_CFGI_WR_DATA	0x24
> +#define CFGI_WRITE			BIT(20)
> +#define H2X_CFGI_CTRL		0x28
> +#define CFGI_TLP_FIRE			BIT(0)
> +#define H2X_CFGI_RET_DATA	0x2C
> +#define H2X_CFGE_TLP_1ST	0x30
> +#define H2X_CFGE_TLP_NEXT	0x34
> +#define H2X_CFGE_CTRL		0x38
> +#define CFGE_TLP_FIRE			BIT(0)
> +#define H2X_CFGE_RET_DATA	0x3C
> +#define H2X_REMAP_PREF_ADDR	0x70
> +#define REMAP_PREF_ADDR_63_32(x)	(x)
> +#define H2X_REMAP_DIRECT_ADDR	0x78
> +#define REMAP_BAR_BASE(x)		(x)
> +
> +/* AST2700 PEHR */
> +#define PEHR_VID_DID		0x00
> +#define PEHR_MISC_58		0x58
> +#define LOCAL_SCALE_SUP			BIT(0)
> +#define PEHR_MISC_5C		0x5C
> +#define CONFIG_RC_DEVICE		BIT(30)
> +#define PEHR_MISC_60		0x60
> +#define PORT_TPYE			GENMASK(7, 4)
> +#define PORT_TYPE_ROOT			BIT(2)
> +#define PEHR_MISC_70		0x70
> +#define POSTED_DATA_CREDITS(x)		FIELD_PREP(GENMASK(15, 0), x)
> +#define POSTED_HEADER_CREDITS(x)	FIELD_PREP(GENMASK(27, 16), x)
> +#define PEHR_MISC_78		0x78
> +#define COMPLETION_DATA_CREDITS(x)	FIELD_PREP(GENMASK(15, 0), x)
> +#define COMPLETION_HEADER_CREDITS(x)	FIELD_PREP(GENMASK(27, 16), x)
> +#define PEHR_MISC_300		0x300
> +#define CAPABILITY_GEN2		BIT(0)
> +#define PEHR_MISC_344		0x344
> +#define LINK_STATUS_GEN2		BIT(18)
> +#define PEHR_MISC_358		0x358
> +#define LINK_STATUS_GEN4		BIT(8)
> +
> +/* AST2700 SCU */
> +#define SCU_60			0x60
> +#define RC_E2M_PATH_EN			BIT(0)
> +#define RC_H2XS_PATH_EN			BIT(16)
> +#define RC_H2XD_PATH_EN			BIT(17)
> +#define RC_H2XX_PATH_EN			BIT(18)
> +#define RC_UPSTREAM_MEM_EN		BIT(19)
> +#define SCU_64			0x64
> +#define RC0_DECODE_DMA_BASE(x)		FIELD_PREP(GENMASK(7, 0), x)
> +#define RC0_DECODE_DMA_LIMIT(x)		FIELD_PREP(GENMASK(15, 8), x)
> +#define RC1_DECODE_DMA_BASE(x)		FIELD_PREP(GENMASK(23, 16), x)
> +#define RC1_DECODE_DMA_LIMIT(x)		FIELD_PREP(GENMASK(31, 24), x)
> +#define SCU_70			0x70
> +#define DISABLE_EP_FUNC			0x0
> +
> +/* TLP configuration type 0 and type 1 */
> +#define CRG_READ_FMTTYPE(type)		(0x04000000 | (type << 24))
> +#define CRG_WRITE_FMTTYPE(type)		(0x44000000 | (type << 24))

These are straight from PCIe spec, right?

I think those should come from defines inside 
include/uapi/linux/pci_regs.h, there might not be one already, so you 
might have to add them.

I also think you should actually use the type as boolean, and return one 
of the two defines based on it. A helper to do that might be generic PCI 
header material as well.

> +#define CRG_PAYLOAD_SIZE		0x01 /* 1 DWORD */
> +#define TLP_COMP_STATUS(s)		(((s) >> 13) & 7)

Name the field if not yet done with a define and use FIELD_GET().

> +#define TLP_BYTE_EN(x)			(x)
> +#define AST2600_TX_DESC1_VALUE		0x00002000

Is this some flag, which should be using a named define instead of the 
literal?

> +#define AST2700_TX_DESC1_VALUE		0x00401000

Can this be constructed by ORing named defines together?

> +
> +struct aspeed_pcie_rc_platform {
> +	int (*setup)(struct platform_device *pdev);
> +	/* Interrupt Register Offset */
> +	int reg_intx_en;

Really? The variable ends with _en which is a short for "Enable" AFAICT 
but your comment talks nothing about "Enable"??

> +	int reg_intx_sts;
> +	int reg_msi_en;
> +	int reg_msi_sts;
> +};
> +
> +struct aspeed_pcie {
> +	struct pci_host_bridge *host;
> +	struct device *dev;
> +	void __iomem *reg;
> +	struct regmap *ahbc;
> +	struct regmap *cfg;
> +	struct regmap *pciephy;
> +	struct clk *clock;
> +	const struct aspeed_pcie_rc_platform *platform;
> +
> +	int domain;
> +	u32 msi_address;
> +	u8 tx_tag;
> +
> +	struct reset_control *h2xrst;
> +	struct reset_control *perst;
> +
> +	struct irq_domain *irq_domain;
> +	struct irq_domain *dev_domain;
> +	struct irq_domain *msi_domain;
> +	/* Protect bitmap variable */

Protects

Can you group them visually together using empty line before and removing 
the empty line in between them too.

> +	struct mutex lock;
> +
> +	DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_HOST_IRQS);
> +};
> +
> +static void aspeed_pcie_intx_ack_irq(struct irq_data *d)
> +{
> +	struct aspeed_pcie *pcie = irq_data_get_irq_chip_data(d);
> +	int intx_en = pcie->platform->reg_intx_en;
> +
> +	writel(readl(pcie->reg + intx_en) | BIT(d->hwirq), pcie->reg + intx_en);

Please split this kind of operations on 3 (or more) lines (there seem to 
be more below too):

	val = readl(...);
	val |= ...;
	writel();

It will be much easier to read that way. If you need more lines for logic, 
e.g., to clear the field first before ORing the new value in, don't 
hesitate to add them as needed.

(val is just a placeholder, you can pick better name for the variable 
where appropriate.)

> +}
> +
> +static void aspeed_pcie_intx_mask_irq(struct irq_data *d)
> +{
> +	struct aspeed_pcie *pcie = irq_data_get_irq_chip_data(d);
> +	int intx_en = pcie->platform->reg_intx_en;
> +
> +	writel(readl(pcie->reg + intx_en) & ~BIT(d->hwirq), pcie->reg + intx_en);
> +}
> +
> +static void aspeed_pcie_intx_unmask_irq(struct irq_data *d)
> +{
> +	struct aspeed_pcie *pcie = irq_data_get_irq_chip_data(d);
> +	int intx_en = pcie->platform->reg_intx_en;
> +
> +	writel(readl(pcie->reg + intx_en) | BIT(d->hwirq), pcie->reg + intx_en);
> +}
> +
> +static struct irq_chip aspeed_intx_irq_chip = {
> +	.name = "ASPEED:IntX",
> +	.irq_ack = aspeed_pcie_intx_ack_irq,
> +	.irq_mask = aspeed_pcie_intx_mask_irq,
> +	.irq_unmask = aspeed_pcie_intx_unmask_irq,
> +};
> +
> +static int aspeed_pcie_intx_map(struct irq_domain *domain, unsigned int irq,
> +				irq_hw_number_t hwirq)
> +{
> +	irq_set_chip_and_handler(irq, &aspeed_intx_irq_chip, handle_level_irq);
> +	irq_set_chip_data(irq, domain->host_data);
> +	irq_set_status_flags(irq, IRQ_LEVEL);
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops aspeed_intx_domain_ops = {
> +	.map = aspeed_pcie_intx_map,
> +};
> +
> +static irqreturn_t aspeed_pcie_intr_handler(int irq, void *dev_id)
> +{
> +	struct aspeed_pcie *pcie = dev_id;
> +	const struct aspeed_pcie_rc_platform *platform = pcie->platform;
> +	unsigned long status;
> +	unsigned long intx;
> +	u32 bit;
> +	int i;
> +
> +	intx = readl(pcie->reg + platform->reg_intx_sts) & 0xf;

Use a named define for the field of interes instead of the literal.

> +	if (intx) {
> +		for_each_set_bit(bit, &intx, PCI_NUM_INTX)
> +			generic_handle_domain_irq(pcie->irq_domain, bit);
> +	}
> +
> +	if (IS_ENABLED(CONFIG_PCI_MSI)) {
> +		for (i = 0; i < 2; i++) {
> +			status = readl(pcie->reg + platform->reg_msi_sts + (i * 4));
> +			writel(status, pcie->reg + platform->reg_msi_sts + (i * 4));
> +
> +			for_each_set_bit(bit, &status, 32) {
> +				bit += (i * 32);
> +				generic_handle_domain_irq(pcie->dev_domain, bit);
> +			}
> +		}
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static u32 aspeed_pcie_get_bdf_offset(struct pci_bus *bus, unsigned int devfn,
> +				      int where)
> +{
> +	return ((bus->number) << 24) | (PCI_SLOT(devfn) << 19) |
> +		(PCI_FUNC(devfn) << 16) | (where & ~3);
> +}
> +
> +static int aspeed_ast2600_rd_conf(struct pci_bus *bus, unsigned int devfn,
> +				  int where, int size, u32 *val)
> +{
> +	struct aspeed_pcie *pcie = bus->sysdata;
> +	u32 bdf_offset;
> +	int rx_done_fail = 0, slot = PCI_SLOT(devfn);
> +	u32 cfg_val, isr, type = 0;
> +	u32 link_sts = 0;
> +	int ret;
> +
> +	/* Driver may set unlock RX buffere before triggering next TX config */

buffer

> +	writel(PCIE_UNLOCK_RX_BUFF | readl(pcie->reg + H2X_DEV_CTRL),
> +	       pcie->reg + H2X_DEV_CTRL);
> +
> +	if (bus->number == 128 && slot != 0 && slot != 8)
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +	type = (bus->number > 128);
> +
> +	if (type) {
> +		regmap_read(pcie->pciephy, PEHR_LINK, &link_sts);
> +		if (!(link_sts & PCIE_LINK_STS))
> +			return PCIBIOS_DEVICE_NOT_FOUND;
> +	}
> +
> +	bdf_offset = aspeed_pcie_get_bdf_offset(bus, devfn, where);
> +
> +	regmap_write(pcie->cfg, H2X_TX_DESC0, CRG_READ_FMTTYPE(type) | CRG_PAYLOAD_SIZE);
> +	regmap_write(pcie->cfg, H2X_TX_DESC1,
> +		     AST2600_TX_DESC1_VALUE | (pcie->tx_tag << 8) | TLP_BYTE_EN(0xf));

Use FIELD_PREP() instead of the custom shifting. I strongly suggest you 
replace also that TLP_BYTE_EN() with FIELD_PREP() done here. If you feel 
need more space, you can first calculate the value into a local variable 
using a multiline construct:

	val = AST2600_TX_DESC1_VALUE | \
	      FIELD_PREP(..., pcie->tx_tag) | \
	      FIELD_PREP(...);

> +	regmap_write(pcie->cfg, H2X_TX_DESC2, bdf_offset);
> +	regmap_write(pcie->cfg, H2X_TX_DESC3, 0);
> +
> +	regmap_write_bits(pcie->cfg, H2X_STS, PCIE_TRIGGER_TX, PCIE_TRIGGER_TX);
> +
> +	ret = regmap_read_poll_timeout(pcie->cfg, H2X_STS, cfg_val,
> +				       (cfg_val & PCIE_TX_IDLE), 0, 50);
> +	if (ret) {
> +		dev_err(pcie->dev,
> +			"[%X:%02X:%02X.%02X]CR tx timeout sts: 0x%08x\n",
> +			pcie->domain, bus->number, PCI_SLOT(devfn),
> +			PCI_FUNC(devfn), cfg_val);
> +		ret = PCIBIOS_SET_FAILED;
> +		*val = ~0;

PCI_SET_ERROR_RESPONSE()

> +		goto out;
> +	}
> +
> +	regmap_write_bits(pcie->cfg, H2X_INT_STS, PCIE_TX_IDLE_CLEAR,
> +			  PCIE_TX_IDLE_CLEAR);
> +
> +	regmap_read(pcie->cfg, H2X_STS, &cfg_val);
> +	switch (cfg_val & PCIE_STATUS_OF_TX) {
> +	case PCIE_RC_L_TX_COMPLETE:
> +	case PCIE_RC_H_TX_COMPLETE:
> +		ret = readl_poll_timeout(pcie->reg + H2X_DEV_STS, isr,
> +					 (isr & PCIE_RC_RX_DONE_ISR), 0, 50);
> +		if (ret) {
> +			dev_err(pcie->dev,
> +				"[%X:%02X:%02X.%02X]CR rx timeoutsts: 0x%08x\n",

Add space after ]

> +				pcie->domain, bus->number, PCI_SLOT(devfn),
> +				PCI_FUNC(devfn), isr);
> +			rx_done_fail = 1;
> +			*val = ~0;

PCI_SET_ERROR_RESPONSE()

> +		}
> +		if (!rx_done_fail) {
> +			if (readl(pcie->reg + H2X_DEV_RX_DESC1) & BIT(13))

Use a named define instead of BIT() literal.

> +				*val = ~0;

PCI_SET_ERROR_RESPONSE()

> +			else
> +				*val = readl(pcie->reg + H2X_DEV_RX_DESC_DATA);
> +		}
> +
> +		writel(PCIE_UNLOCK_RX_BUFF | readl(pcie->reg + H2X_DEV_CTRL),
> +		       pcie->reg + H2X_DEV_CTRL);
> +		break;
> +	case PCIE_STATUS_OF_TX:
> +		*val = ~0;
> +		break;
> +	default:
> +		regmap_read(pcie->cfg, H2X_DEV_RX_DESC_DATA, &cfg_val);
> +		*val = cfg_val;
> +		break;
> +	}
> +
> +	switch (size) {
> +	case 1:
> +		*val = (*val >> ((where & 3) * 8)) & 0xff;
> +		break;
> +	case 2:
> +		*val = (*val >> ((where & 2) * 8)) & 0xffff;
> +		break;
> +	case 4:
> +	default:
> +		break;
> +	}
> +
> +	ret = PCIBIOS_SUCCESSFUL;
> +out:
> +	writel(readl(pcie->reg + H2X_DEV_STS), pcie->reg + H2X_DEV_STS);
> +	pcie->tx_tag = (pcie->tx_tag + 1) % 0x8;
> +	return ret;
> +}
> +
> +static int aspeed_ast2600_wr_conf(struct pci_bus *bus, unsigned int devfn,
> +				  int where, int size, u32 val)
> +{
> +	u32 type = 0;
> +	u32 shift = 8 * (where & 3);
> +	u32 bdf_offset;
> +	u8 byte_en = 0;
> +	struct aspeed_pcie *pcie = bus->sysdata;
> +	u32 isr, cfg_val;
> +	int ret;
> +
> +	/* Driver may set unlock RX buffere before triggering next TX config */

buffer

Many similar things in this function so please reflect my other comments 
to this.

> +	writel(PCIE_UNLOCK_RX_BUFF | readl(pcie->reg + H2X_DEV_CTRL),
> +	       pcie->reg + H2X_DEV_CTRL);
> +
> +	switch (size) {
> +	case 1:
> +		byte_en = 1 << (where % 4);
> +		val = (val & 0xff) << shift;
> +		break;
> +	case 2:
> +		byte_en = 0x3 << (2 * ((where >> 1) % 2));
> +		val = (val & 0xffff) << shift;
> +		break;
> +	case 4:
> +	default:
> +		byte_en = 0xf;
> +		break;
> +	}
> +
> +	type = (bus->number > 128);
> +
> +	bdf_offset = aspeed_pcie_get_bdf_offset(bus, devfn, where);
> +
> +	regmap_write(pcie->cfg, H2X_TX_DESC0, CRG_WRITE_FMTTYPE(type) | CRG_PAYLOAD_SIZE);
> +	regmap_write(pcie->cfg, H2X_TX_DESC1,
> +		     AST2600_TX_DESC1_VALUE | (pcie->tx_tag << 8) | TLP_BYTE_EN(byte_en));
> +	regmap_write(pcie->cfg, H2X_TX_DESC2, bdf_offset);
> +	regmap_write(pcie->cfg, H2X_TX_DESC3, 0);
> +	regmap_write(pcie->cfg, H2X_TX_DESC_DATA, val);
> +
> +	regmap_write_bits(pcie->cfg, H2X_STS, PCIE_TRIGGER_TX, PCIE_TRIGGER_TX);
> +
> +	ret = regmap_read_poll_timeout(pcie->cfg, H2X_STS, cfg_val,
> +				       (cfg_val & PCIE_TX_IDLE), 0, 50);
> +	if (ret) {
> +		dev_err(pcie->dev,
> +			"[%X:%02X:%02X.%02X]CT tx timeout sts: 0x%08x\n",
> +			pcie->domain, bus->number, PCI_SLOT(devfn),
> +			PCI_FUNC(devfn), cfg_val);
> +		ret = PCIBIOS_SET_FAILED;
> +		goto out;
> +	}
> +
> +	regmap_write_bits(pcie->cfg, H2X_INT_STS, PCIE_TX_IDLE_CLEAR,
> +			  PCIE_TX_IDLE_CLEAR);
> +
> +	regmap_read(pcie->cfg, H2X_STS, &cfg_val);
> +	switch (cfg_val & PCIE_STATUS_OF_TX) {
> +	case PCIE_RC_L_TX_COMPLETE:
> +	case PCIE_RC_H_TX_COMPLETE:
> +		ret = readl_poll_timeout(pcie->reg + H2X_DEV_STS, isr,
> +					 (isr & PCIE_RC_RX_DONE_ISR), 0, 50);
> +		if (ret) {
> +			dev_err(pcie->dev,
> +				"[%X:%02X:%02X.%02X]CT rx timeout sts: 0x%08x\n",
> +				pcie->domain, bus->number, PCI_SLOT(devfn),
> +				PCI_FUNC(devfn), isr);
> +			ret = PCIBIOS_SET_FAILED;
> +			goto out;
> +		}
> +		break;
> +	}
> +
> +	ret = PCIBIOS_SUCCESSFUL;
> +out:
> +	writel(readl(pcie->reg + H2X_DEV_STS), pcie->reg + H2X_DEV_STS);
> +	pcie->tx_tag = (pcie->tx_tag + 1) % 0x8;
> +	return ret;
> +}
> +
> +static bool aspeed_ast2700_get_link(struct aspeed_pcie *pcie)
> +{
> +	u32 reg;
> +	bool link;
> +
> +	regmap_read(pcie->pciephy, PEHR_MISC_300, &reg);
> +	if (reg & CAPABILITY_GEN2) {
> +		regmap_read(pcie->pciephy, PEHR_MISC_344, &reg);
> +		link = !!(reg & LINK_STATUS_GEN2);
> +	} else {
> +		regmap_read(pcie->pciephy, PEHR_MISC_358, &reg);
> +		link = !!(reg & LINK_STATUS_GEN4);

While I don't entirely know the meaning of these bits, what if the link is 
not using maximum speed it is capable of, does this check misbehave?

> +	}
> +
> +	return link;
> +}
> +
> +static int aspeed_ast2700_rd_conf(struct pci_bus *bus, unsigned int devfn,
> +				  int where, int size, u32 *val)
> +{
> +	struct aspeed_pcie *pcie = bus->sysdata;
> +	u32 bdf_offset, status;
> +	u8 type;
> +	int ret;
> +
> +	if ((bus->number == 0 && devfn != 0))
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +
> +	if (bus->number == 0) {
> +		/* Internal access to bridge */
> +		writel(TLP_BYTE_EN(0xf) << 16 | (where & ~3), pcie->reg + H2X_CFGI_TLP);

FIELD_PREP()

> +		writel(CFGI_TLP_FIRE, pcie->reg + H2X_CFGI_CTRL);
> +		*val = readl(pcie->reg + H2X_CFGI_RET_DATA);
> +	} else {
> +		if (!aspeed_ast2700_get_link(pcie))
> +			return PCIBIOS_DEVICE_NOT_FOUND;
> +
> +		bdf_offset = aspeed_pcie_get_bdf_offset(bus, devfn, where);
> +
> +		type = (bus->number == 1) ? PCI_HEADER_TYPE_NORMAL : PCI_HEADER_TYPE_BRIDGE;
> +
> +		writel(CRG_READ_FMTTYPE(type) | CRG_PAYLOAD_SIZE, pcie->reg + H2X_CFGE_TLP_1ST);
> +		writel(AST2700_TX_DESC1_VALUE | (pcie->tx_tag << 8) | TLP_BYTE_EN(0xf),
> +		       pcie->reg + H2X_CFGE_TLP_NEXT);
> +		writel(bdf_offset, pcie->reg + H2X_CFGE_TLP_NEXT);
> +		writel(CFGE_TX_IDLE | CFGE_RX_BUSY, pcie->reg + H2X_CFGE_INT_STS);
> +		writel(CFGE_TLP_FIRE, pcie->reg + H2X_CFGE_CTRL);
> +
> +		ret = readl_poll_timeout(pcie->reg + H2X_CFGE_INT_STS, status,
> +					 (status & CFGE_TX_IDLE), 0, 50);
> +		if (ret) {
> +			dev_err(pcie->dev,
> +				"[%X:%02X:%02X.%02X]CR tx timeout sts: 0x%08x\n",
> +				pcie->domain, bus->number, PCI_SLOT(devfn),
> +				PCI_FUNC(devfn), status);
> +			ret = PCIBIOS_SET_FAILED;
> +			*val = ~0;
> +			goto out;
> +		}
> +
> +		ret = readl_poll_timeout(pcie->reg + H2X_CFGE_INT_STS, status,
> +					 (status & CFGE_RX_BUSY), 0, 50000);
> +		if (ret) {
> +			dev_err(pcie->dev,
> +				"[%X:%02X:%02X.%02X]CR rx timeoutsts: 0x%08x\n",
> +				pcie->domain, bus->number, PCI_SLOT(devfn),
> +				PCI_FUNC(devfn), status);
> +			ret = PCIBIOS_SET_FAILED;
> +			*val = ~0;
> +			goto out;
> +		}
> +		*val = readl(pcie->reg + H2X_CFGE_RET_DATA);
> +	}
> +
> +	switch (size) {
> +	case 1:
> +		*val = (*val >> ((where & 3) * 8)) & 0xff;
> +		break;
> +	case 2:
> +		*val = (*val >> ((where & 2) * 8)) & 0xffff;
> +		break;
> +	case 4:
> +	default:
> +		break;
> +	}
> +
> +	ret = PCIBIOS_SUCCESSFUL;
> +out:
> +	writel(status, pcie->reg + H2X_CFGE_INT_STS);
> +	pcie->tx_tag = (pcie->tx_tag + 1) % 0xF;
> +	return ret;
> +}

On a glance, this (& the wr func) looked very similar to the 2600 one on a 
glance, I didn't check it with diff how similar it really is.

If you need minor variation e.g. in some register value, you could add 
that into the struct pointed by .data. Then you can get it easily here 
using pcie->info->tx_desc_val without duplicating lots of code.

> +static int aspeed_ast2700_wr_conf(struct pci_bus *bus, unsigned int devfn,
> +				  int where, int size, u32 val)
> +{
> +	struct aspeed_pcie *pcie = bus->sysdata;
> +	u32 shift = 8 * (where & 3);
> +	u8 byte_en;
> +	u32 bdf_offset, status, type;
> +	int ret;
> +
> +	if ((bus->number == 0 && devfn != 0))
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +
> +	switch (size) {
> +	case 1:
> +		byte_en = 1 << (where % 4);
> +		val = (val & 0xff) << shift;
> +		break;
> +	case 2:
> +		byte_en = 0x3 << (2 * ((where >> 1) % 2));
> +		val = (val & 0xffff) << shift;
> +		break;
> +	case 4:
> +	default:
> +		byte_en = 0xf;
> +		break;
> +	}
> +
> +	if (bus->number == 0) {
> +		/* Internal access to bridge */
> +		writel(CFGI_WRITE | TLP_BYTE_EN(byte_en) << 16 | (where & ~3),
> +		       pcie->reg + H2X_CFGI_TLP);
> +		writel(val, pcie->reg + H2X_CFGI_WR_DATA);
> +		writel(CFGI_TLP_FIRE, pcie->reg + H2X_CFGI_CTRL);
> +	} else {
> +		if (!aspeed_ast2700_get_link(pcie))
> +			return PCIBIOS_SET_FAILED;
> +
> +		bdf_offset = aspeed_pcie_get_bdf_offset(bus, devfn, where);
> +
> +		type = (bus->number == 1) ? PCI_HEADER_TYPE_NORMAL : PCI_HEADER_TYPE_BRIDGE;
> +
> +		writel(CRG_WRITE_FMTTYPE(type) | CRG_PAYLOAD_SIZE, pcie->reg + H2X_CFGE_TLP_1ST);
> +		writel(AST2700_TX_DESC1_VALUE | (pcie->tx_tag << 8) | byte_en,
> +		       pcie->reg + H2X_CFGE_TLP_NEXT);
> +		writel(bdf_offset, pcie->reg + H2X_CFGE_TLP_NEXT);
> +		writel(val, pcie->reg + H2X_CFGE_TLP_NEXT);
> +		writel(CFGE_TX_IDLE | CFGE_RX_BUSY, pcie->reg + H2X_CFGE_INT_STS);
> +		writel(CFGE_TLP_FIRE, pcie->reg + H2X_CFGE_CTRL);
> +
> +		ret = readl_poll_timeout(pcie->reg + H2X_CFGE_INT_STS, status,
> +					 (status & CFGE_TX_IDLE), 0, 50);
> +		if (ret) {
> +			dev_err(pcie->dev,
> +				"[%X:%02X:%02X.%02X]CT tx timeout sts: 0x%08x\n",
> +				pcie->domain, bus->number, PCI_SLOT(devfn),
> +				PCI_FUNC(devfn), status);
> +			ret = PCIBIOS_SET_FAILED;
> +			goto out;
> +		}
> +
> +		ret = readl_poll_timeout(pcie->reg + H2X_CFGE_INT_STS, status,
> +					 (status & CFGE_RX_BUSY), 0, 50000);
> +		if (ret) {
> +			dev_err(pcie->dev,
> +				"[%X:%02X:%02X.%02X]CT rx timeout sts: 0x%08x\n",
> +				pcie->domain, bus->number, PCI_SLOT(devfn),
> +				PCI_FUNC(devfn), status);
> +			ret = PCIBIOS_SET_FAILED;
> +			goto out;
> +		}
> +
> +		(void)readl(pcie->reg + H2X_CFGE_RET_DATA);
> +	}
> +
> +	ret = PCIBIOS_SUCCESSFUL;
> +out:
> +	writel(status, pcie->reg + H2X_CFGE_INT_STS);
> +	pcie->tx_tag = (pcie->tx_tag + 1) % 0xF;
> +	return ret;
> +}
> +
> +static struct pci_ops aspeed_ast2600_pcie_ops = {
> +	.read = aspeed_ast2600_rd_conf,
> +	.write = aspeed_ast2600_wr_conf,
> +};
> +
> +static struct pci_ops aspeed_ast2700_pcie_ops = {
> +	.read = aspeed_ast2700_rd_conf,
> +	.write = aspeed_ast2700_wr_conf,
> +};
> +
> +#ifdef CONFIG_PCI_MSI
> +static void aspeed_msi_compose_msi_msg(struct irq_data *data,
> +				       struct msi_msg *msg)
> +{
> +	struct aspeed_pcie *pcie = irq_data_get_irq_chip_data(data);
> +
> +	msg->address_hi = 0;
> +	msg->address_lo = pcie->msi_address;
> +	msg->data = data->hwirq;
> +}
> +
> +static int aspeed_msi_set_affinity(struct irq_data *irq_data,
> +				   const struct cpumask *mask, bool force)
> +{
> +	return -EINVAL;
> +}
> +
> +static struct irq_chip aspeed_msi_bottom_irq_chip = {
> +	.name = "ASPEED MSI",
> +	.irq_compose_msi_msg = aspeed_msi_compose_msi_msg,
> +	.irq_set_affinity = aspeed_msi_set_affinity,
> +};
> +
> +static int aspeed_irq_msi_domain_alloc(struct irq_domain *domain,
> +				       unsigned int virq, unsigned int nr_irqs,
> +				       void *args)
> +{
> +	struct aspeed_pcie *pcie = domain->host_data;
> +	int bit;
> +	int i;
> +
> +	mutex_lock(&pcie->lock);
> +
> +	bit = bitmap_find_free_region(pcie->msi_irq_in_use, MAX_MSI_HOST_IRQS,
> +				      get_count_order(nr_irqs));
> +
> +	mutex_unlock(&pcie->lock);
> +
> +	if (bit < 0)
> +		return -ENOSPC;
> +
> +	for (i = 0; i < nr_irqs; i++) {
> +		irq_domain_set_info(domain, virq + i, bit + i,
> +				    &aspeed_msi_bottom_irq_chip,
> +				    domain->host_data, handle_simple_irq, NULL,
> +				    NULL);
> +	}
> +
> +	return 0;
> +}
> +
> +static void aspeed_irq_msi_domain_free(struct irq_domain *domain,
> +				       unsigned int virq, unsigned int nr_irqs)
> +{
> +	struct irq_data *data = irq_domain_get_irq_data(domain, virq);
> +	struct aspeed_pcie *pcie = irq_data_get_irq_chip_data(data);
> +
> +	mutex_lock(&pcie->lock);
> +
> +	bitmap_release_region(pcie->msi_irq_in_use, data->hwirq,
> +			      get_count_order(nr_irqs));
> +
> +	mutex_unlock(&pcie->lock);
> +}
> +
> +static const struct irq_domain_ops aspeed_msi_domain_ops = {
> +	.alloc = aspeed_irq_msi_domain_alloc,
> +	.free = aspeed_irq_msi_domain_free,
> +};
> +
> +static struct irq_chip aspeed_msi_irq_chip = {
> +	.name = "PCIe MSI",
> +	.irq_enable = pci_msi_unmask_irq,
> +	.irq_disable = pci_msi_mask_irq,
> +	.irq_mask = pci_msi_mask_irq,
> +	.irq_unmask = pci_msi_unmask_irq,
> +};
> +
> +static struct msi_domain_info aspeed_msi_domain_info = {
> +	.flags = (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
> +		  MSI_FLAG_MULTI_PCI_MSI | MSI_FLAG_PCI_MSIX),
> +	.chip = &aspeed_msi_irq_chip,
> +};
> +#endif
> +
> +static void aspeed_pcie_irq_domain_free(struct aspeed_pcie *pcie)
> +{
> +	if (pcie->irq_domain) {
> +		irq_domain_remove(pcie->irq_domain);
> +		pcie->irq_domain = NULL;
> +	}
> +#ifdef CONFIG_PCI_MSI
> +	if (pcie->msi_domain) {
> +		irq_domain_remove(pcie->msi_domain);
> +		pcie->msi_domain = NULL;
> +	}
> +
> +	if (pcie->dev_domain) {
> +		irq_domain_remove(pcie->dev_domain);
> +		pcie->dev_domain = NULL;
> +	}
> +#endif

Add aspeed_msi_init() and aspeed_msi_free() helpers inside the large 
#ifdef CONFIG_PCI_MSI block and make them empty on #else side so you don't 
need ifdeffery here at all but can just make one call.

> +}
> +
> +static int aspeed_pcie_init_irq_domain(struct aspeed_pcie *pcie)
> +{
> +	struct device *dev = pcie->dev;
> +	struct device_node *node = dev->of_node;
> +	struct device_node *pcie_intc_node;
> +	int ret;
> +
> +	pcie_intc_node = of_get_next_child(node, NULL);
> +	if (!pcie_intc_node)
> +		return dev_err_probe(dev, -ENODEV, "No PCIe Intc node found\n");
> +
> +	pcie->irq_domain =
> +		irq_domain_add_linear(pcie_intc_node, PCI_NUM_INTX, &aspeed_intx_domain_ops, pcie);
> +	of_node_put(pcie_intc_node);
> +	if (!pcie->irq_domain) {
> +		ret = dev_err_probe(dev, -ENOMEM, "failed to get an INTx IRQ domain\n");
> +		goto err;
> +	}
> +
> +	writel(0, pcie->reg + pcie->platform->reg_intx_en);
> +	writel(~0, pcie->reg + pcie->platform->reg_intx_sts);
> +
> +#ifdef CONFIG_PCI_MSI
> +	pcie->dev_domain =
> +		irq_domain_add_linear(NULL, MAX_MSI_HOST_IRQS, &aspeed_msi_domain_ops, pcie);
> +	if (!pcie->dev_domain) {
> +		ret = dev_err_probe(pcie->dev, -ENOMEM, "failed to create IRQ domain\n");
> +		goto err;
> +	}
> +
> +	pcie->msi_domain = pci_msi_create_irq_domain(dev_fwnode(pcie->dev), &aspeed_msi_domain_info,
> +						     pcie->dev_domain);
> +	if (!pcie->msi_domain) {
> +		ret = dev_err_probe(pcie->dev, -ENOMEM, "failed to create MSI domain\n");
> +		goto err;
> +	}
> +
> +	writel(~0, pcie->reg + pcie->platform->reg_msi_en);
> +	writel(~0, pcie->reg + pcie->platform->reg_msi_en + 0x04);
> +	writel(~0, pcie->reg + pcie->platform->reg_msi_sts);
> +	writel(~0, pcie->reg + pcie->platform->reg_msi_sts + 0x04);
> +#endif

Ditto.

> +	return 0;
> +err:
> +	aspeed_pcie_irq_domain_free(pcie);
> +	return ret;
> +}
> +
> +static void aspeed_pcie_port_init(struct aspeed_pcie *pcie)
> +{
> +	u32 link_sts = 0;
> +
> +	regmap_write(pcie->pciephy, PEHR_LOCK, PCIE_UNLOCK);
> +	regmap_write(pcie->pciephy, PEHR_GLOBAL, ROOT_COMPLEX_ID(0x3));
> +
> +	reset_control_deassert(pcie->perst);
> +	mdelay(500);
> +
> +	writel(PCIE_RX_DMA_EN | PCIE_RX_LINEAR | PCIE_RX_MSI_SEL | PCIE_RX_MSI_EN |
> +	       PCIE_Wait_RX_TLP_CLR | PCIE_RC_RX_ENABLE | PCIE_RC_ENABLE,
> +	       pcie->reg + H2X_DEV_CTRL);
> +
> +	writel(0x28, pcie->reg + H2X_DEV_TX_TAG);

A magic number?

> +
> +	regmap_read(pcie->pciephy, PEHR_LINK, &link_sts);
> +	if (link_sts & PCIE_LINK_STS)
> +		dev_info(pcie->dev, "PCIe Link UP");
> +	else
> +		dev_info(pcie->dev, "PCIe Link DOWN");

No info level prints like that, use _dbg level instead if you feel that 
is necessary information.

> +}
> +
> +static int aspeed_ast2600_setup(struct platform_device *pdev)
> +{
> +	struct aspeed_pcie *pcie = platform_get_drvdata(pdev);
> +	struct device *dev = pcie->dev;
> +
> +	pcie->ahbc = syscon_regmap_lookup_by_phandle(dev->of_node, "aspeed,ahbc");
> +	if (IS_ERR(pcie->ahbc))
> +		return dev_err_probe(dev, PTR_ERR(pcie->ahbc), "failed to map ahbc base\n");
> +
> +	reset_control_assert(pcie->h2xrst);
> +	mdelay(5);
> +	reset_control_deassert(pcie->h2xrst);
> +
> +	regmap_write(pcie->ahbc, AHBC_KEY, AHBC_UNLOCK_KEY);
> +	regmap_update_bits(pcie->ahbc, AHBC_ADDR_MAPPING, PCIE_RC_MEMORY_EN, PCIE_RC_MEMORY_EN);
> +	regmap_write(pcie->ahbc, AHBC_KEY, AHBC_UNLOCK);
> +
> +	regmap_write(pcie->cfg, H2X_AHB_ADDR_CONFIG0,
> +		     AHB_REMAP_ADDR_31_20(0x600) | AHB_MASK_ADDR_31_20(0xE00));
> +	regmap_write(pcie->cfg, H2X_AHB_ADDR_CONFIG1, AHB_REMAP_ADDR_64_32(0));
> +	regmap_write(pcie->cfg, H2X_AHB_ADDR_CONFIG2, AHB_MASK_ADDR_64_32(~0));
> +
> +	regmap_write(pcie->cfg, H2X_CTRL, H2X_BRIDGE_EN);
> +
> +	aspeed_pcie_port_init(pcie);
> +
> +	pcie->host->ops = &aspeed_ast2600_pcie_ops;
> +
> +	return 0;
> +}
> +
> +static int aspeed_ast2700_setup(struct platform_device *pdev)
> +{
> +	struct aspeed_pcie *pcie = platform_get_drvdata(pdev);
> +	u32 cfg_val;
> +
> +	reset_control_assert(pcie->perst);
> +
> +	regmap_write(pcie->pciephy, PEHR_MISC_70,
> +		     POSTED_DATA_CREDITS(0xc0) | POSTED_HEADER_CREDITS(0xa));
> +	regmap_write(pcie->pciephy, PEHR_MISC_78,
> +		     COMPLETION_DATA_CREDITS(0x30) | COMPLETION_HEADER_CREDITS(0x8));
> +	regmap_write(pcie->pciephy, PEHR_MISC_58, LOCAL_SCALE_SUP);
> +
> +	regmap_update_bits(pcie->cfg, SCU_60,
> +			   RC_E2M_PATH_EN | RC_H2XS_PATH_EN | RC_H2XD_PATH_EN | RC_H2XX_PATH_EN |
> +			   RC_UPSTREAM_MEM_EN,
> +			   RC_E2M_PATH_EN | RC_H2XS_PATH_EN | RC_H2XD_PATH_EN | RC_H2XX_PATH_EN |
> +			   RC_UPSTREAM_MEM_EN);
> +	regmap_write(pcie->cfg, SCU_64,
> +		     RC0_DECODE_DMA_BASE(0) | RC0_DECODE_DMA_LIMIT(0xFF) | RC1_DECODE_DMA_BASE(0) |
> +		     RC1_DECODE_DMA_LIMIT(0xFF));
> +	regmap_write(pcie->cfg, SCU_70, DISABLE_EP_FUNC);
> +
> +	reset_control_assert(pcie->h2xrst);
> +	mdelay(10);
> +	reset_control_deassert(pcie->h2xrst);
> +
> +	regmap_write(pcie->pciephy, PEHR_MISC_5C, CONFIG_RC_DEVICE);
> +	regmap_read(pcie->pciephy, PEHR_MISC_60, &cfg_val);
> +	regmap_write(pcie->pciephy, PEHR_MISC_60,
> +		     (cfg_val & ~PORT_TPYE) | FIELD_PREP(PORT_TPYE, PORT_TYPE_ROOT));

TYPE (typo)


Put the logic on separate lines before the write.

> +
> +	writel(0, pcie->reg + H2X_CTRL);
> +	writel(H2X_BRIDGE_EN | H2X_BRIDGE_DIRECT_EN, pcie->reg + H2X_CTRL);
> +
> +	/* The BAR mapping:
> +	 * CPU Node0(domain 0): 0x60000000
> +	 * CPU Node1(domain 1): 0x80000000
> +	 * IO       (domain 2): 0xa0000000
> +	 */
> +	writel(REMAP_BAR_BASE(0x60000000 + (0x20000000 * pcie->domain)),

Please name what is in the comment with defines and use the named defines 
in the code instead of the literals. Also consider using SZ_* for that 
size(?) (remember to add the include if using them).

> +	       pcie->reg + H2X_REMAP_DIRECT_ADDR);
> +
> +	/* Prepare for 64-bit BAR pref */
> +	writel(REMAP_PREF_ADDR_63_32(0x3), pcie->reg + H2X_REMAP_PREF_ADDR);
> +
> +	reset_control_deassert(pcie->perst);
> +	mdelay(1000);
> +
> +	pcie->host->ops = &aspeed_ast2700_pcie_ops;
> +
> +	if (aspeed_ast2700_get_link(pcie))
> +		dev_info(pcie->dev, "PCIe Link UP");
> +	else
> +		dev_info(pcie->dev, "PCIe Link DOWN");
> +
> +	return 0;
> +}
> +
> +static int aspeed_pcie_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct pci_host_bridge *host;
> +	struct aspeed_pcie *pcie;
> +	struct device_node *node = dev->of_node;
> +	const void *md = of_device_get_match_data(dev);
> +	int irq, ret;
> +
> +	if (!md)
> +		return -ENODEV;
> +
> +	host = devm_pci_alloc_host_bridge(dev, sizeof(*pcie));
> +	if (!host)
> +		return -ENOMEM;
> +
> +	pcie = pci_host_bridge_priv(host);
> +	pcie->dev = dev;
> +	pcie->tx_tag = 0;
> +	platform_set_drvdata(pdev, pcie);
> +
> +	pcie->platform = md;
> +	pcie->host = host;
> +
> +	pcie->reg = devm_platform_ioremap_resource(pdev, 0);
> +
> +	of_property_read_u32(node, "msi_address", &pcie->msi_address);
> +	of_property_read_u32(node, "linux,pci-domain", &pcie->domain);
> +
> +	pcie->cfg = syscon_regmap_lookup_by_phandle(dev->of_node, "aspeed,pciecfg");
> +	if (IS_ERR(pcie->cfg))
> +		return dev_err_probe(dev, PTR_ERR(pcie->cfg), "Failed to map pciecfg base\n");
> +
> +	pcie->pciephy = syscon_regmap_lookup_by_phandle(node, "aspeed,pciephy");
> +	if (IS_ERR(pcie->pciephy))
> +		return dev_err_probe(dev, PTR_ERR(pcie->pciephy), "Failed to map pciephy base\n");
> +
> +	pcie->h2xrst = devm_reset_control_get_exclusive(dev, "h2x");
> +	if (IS_ERR(pcie->h2xrst))
> +		return dev_err_probe(dev, PTR_ERR(pcie->h2xrst), "Failed to get h2x reset\n");
> +
> +	pcie->perst = devm_reset_control_get_exclusive(dev, "perst");
> +	if (IS_ERR(pcie->perst))
> +		return dev_err_probe(dev, PTR_ERR(pcie->perst), "Failed to get perst reset\n");
> +
> +	ret = pcie->platform->setup(pdev);
> +	if (ret)
> +		goto err_setup;
> +
> +	host->sysdata = pcie;
> +
> +	ret = aspeed_pcie_init_irq_domain(pcie);
> +	if (ret)
> +		goto err_irq_init;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		goto err_irq;
> +
> +	ret = devm_request_irq(dev, irq, aspeed_pcie_intr_handler, IRQF_SHARED, dev_name(dev),
> +			       pcie);
> +	if (ret)
> +		goto err_irq;
> +
> +	pcie->clock = clk_get(dev, NULL);
> +	if (IS_ERR(pcie->clock))
> +		goto err_clk;
> +	ret = clk_prepare_enable(pcie->clock);
> +	if (ret)
> +		goto err_clk_enable;
> +
> +	ret = pci_host_probe(host);
> +	if (ret)
> +		goto err_clk_enable;
> +
> +	return 0;
> +
> +err_clk_enable:
> +	clk_put(pcie->clock);
> +err_clk:
> +err_irq:

It would be better to name these on what is rolled back, not based on what 
failed.

> +	aspeed_pcie_irq_domain_free(pcie);
> +err_irq_init:
> +err_setup:

These should be removed and the goto replaced with direct return.

> +	return dev_err_probe(dev, ret, "Failed to setup PCIe RC\n");

Common printing is not well suited for the rollback path. It's not much 
value to print a generic message like that, instead print the more 
specific error before gotos.

> +}

Where's the mutex initialized??? Please use devm_mutex_init() and don't 
forget to check errors because devm can fail.

> +
> +static void aspeed_pcie_remove(struct platform_device *pdev)
> +{
> +	struct aspeed_pcie *pcie = platform_get_drvdata(pdev);
> +
> +	if (pcie->clock) {
> +		clk_disable_unprepare(pcie->clock);
> +		clk_put(pcie->clock);
> +	}
> +
> +	pci_stop_root_bus(pcie->host->bus);
> +	pci_remove_root_bus(pcie->host->bus);
> +	aspeed_pcie_irq_domain_free(pcie);
> +}
> +
> +static struct aspeed_pcie_rc_platform pcie_rc_ast2600 = {
> +	.setup = aspeed_ast2600_setup,
> +	.reg_intx_en = 0x04,
> +	.reg_intx_sts = 0x08,
> +	.reg_msi_en = 0x20,
> +	.reg_msi_sts = 0x28,
> +};
> +
> +static struct aspeed_pcie_rc_platform pcie_rc_ast2700 = {
> +	.setup = aspeed_ast2700_setup,
> +	.reg_intx_en = 0x40,
> +	.reg_intx_sts = 0x48,
> +	.reg_msi_en = 0x50,
> +	.reg_msi_sts = 0x58,
> +};
> +
> +static const struct of_device_id aspeed_pcie_of_match[] = {
> +	{ .compatible = "aspeed,ast2600-pcie", .data = &pcie_rc_ast2600 },
> +	{ .compatible = "aspeed,ast2700-pcie", .data = &pcie_rc_ast2700 },
> +	{}
> +};
> +
> +static struct platform_driver aspeed_pcie_driver = {
> +	.driver = {
> +		.name = "aspeed-pcie",
> +		.suppress_bind_attrs = true,
> +		.of_match_table = aspeed_pcie_of_match,
> +	},
> +	.probe = aspeed_pcie_probe,
> +	.remove = aspeed_pcie_remove,
> +};
> +
> +module_platform_driver(aspeed_pcie_driver);
> +
> +MODULE_AUTHOR("Jacky Chou <jacky_chou@aspeedtech.com>");
> +MODULE_DESCRIPTION("ASPEED PCIe Root Complex");
> +MODULE_LICENSE("GPL");
> 

-- 
 i.

On 13/06/2025 05:30, Jacky Chou wrote:
> Introduce PCIe Root Complex driver for ASPEED SoCs. Support RC
> initialization, reset, clock, IRQ domain, and MSI domain setup.
> Implement platform-specific setup and register configuration for
> ASPEED. And provide PCI config space read/write and INTx/MSI
> interrupt handling.
> 
> Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
> ---
>  drivers/pci/controller/Kconfig       |   13 +
>  drivers/pci/controller/Makefile      |    1 +
>  drivers/pci/controller/pcie-aspeed.c | 1039 ++++++++++++++++++++++++++
>  3 files changed, 1053 insertions(+)
>  create mode 100644 drivers/pci/controller/pcie-aspeed.c
> 
> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> index 886f6f43a895..f6b5eea3b570 100644
> --- a/drivers/pci/controller/Kconfig
> +++ b/drivers/pci/controller/Kconfig
> @@ -216,6 +216,19 @@ config PCIE_MT7621
>  	help
>  	  This selects a driver for the MediaTek MT7621 PCIe Controller.
>  
> +config PCIE_ASPEED
> +	bool "ASPEED PCIe controller"
> +	depends on PCI

depends ARCH_ASPEED || COMPILE_TEST

> +	depends on OF || COMPILE_TEST
> +	select PCI_MSI_ARCH_FALLBACKS
> +	help
> +	  Enable this option to add support for the PCIe controller
> +	  found on ASPEED SoCs.
> +	  This driver provides initialization and management for PCIe
> +	  Root Complex functionality, including interrupt and MSI support.
> +	  Select Y if your platform uses an ASPEED SoC and requires PCIe
> +	  connectivity.
> +
>  config PCI_HYPERV_INTERFACE
>  	tristate "Microsoft Hyper-V PCI Interface"
>  	depends on ((X86 && X86_64) || ARM64) && HYPERV && PCI_MSI
> diff --git a/drivers/pci/controller/Makefile b/drivers/pci/controller/Makefile
> index 038ccbd9e3ba..1339f88e153d 100644
> --- a/drivers/pci/controller/Makefile
> +++ b/drivers/pci/controller/Makefile
> @@ -39,6 +39,7 @@ obj-$(CONFIG_PCI_LOONGSON) += pci-loongson.o
>  obj-$(CONFIG_PCIE_HISI_ERR) += pcie-hisi-error.o
>  obj-$(CONFIG_PCIE_APPLE) += pcie-apple.o
>  obj-$(CONFIG_PCIE_MT7621) += pcie-mt7621.o
> +obj-$(CONFIG_PCIE_ASPEED) += pcie-aspeed.o
>  
>  # pcie-hisi.o quirks are needed even without CONFIG_PCIE_DW
>  obj-y				+= dwc/
> diff --git a/drivers/pci/controller/pcie-aspeed.c b/drivers/pci/controller/pcie-aspeed.c
> new file mode 100644
> index 000000000000..c745684a7f9b
> --- /dev/null
> +++ b/drivers/pci/controller/pcie-aspeed.c
> @@ -0,0 +1,1039 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2025 Aspeed Technology Inc.
> + */
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/kernel.h>
> +#include <linux/msi.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_platform.h>

Where do you use it?

> +#include <linux/of_address.h>

Where do you use it?


> +#include <linux/of_irq.h>

Where do you use it?


> +#include <linux/of_pci.h>

Where do you use it?

> +#include <linux/pci.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +#include <linux/irq.h>
> +#include <linux/interrupt.h>
> +#include <linux/workqueue.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +



...

> +
> +static int aspeed_pcie_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct pci_host_bridge *host;
> +	struct aspeed_pcie *pcie;
> +	struct device_node *node = dev->of_node;
> +	const void *md = of_device_get_match_data(dev);

Not void, but specific type. This is not Javascript, we have here types.

> +	int irq, ret;
> +
> +	if (!md)
> +		return -ENODEV;
> +
> +	host = devm_pci_alloc_host_bridge(dev, sizeof(*pcie));
> +	if (!host)
> +		return -ENOMEM;
> +
> +	pcie = pci_host_bridge_priv(host);
> +	pcie->dev = dev;
> +	pcie->tx_tag = 0;
> +	platform_set_drvdata(pdev, pcie);
> +
> +	pcie->platform = md;
> +	pcie->host = host;
> +
> +	pcie->reg = devm_platform_ioremap_resource(pdev, 0);
> +
> +	of_property_read_u32(node, "msi_address", &pcie->msi_address);
> +	of_property_read_u32(node, "linux,pci-domain", &pcie->domain);
> +
> +	pcie->cfg = syscon_regmap_lookup_by_phandle(dev->of_node, "aspeed,pciecfg");
> +	if (IS_ERR(pcie->cfg))
> +		return dev_err_probe(dev, PTR_ERR(pcie->cfg), "Failed to map pciecfg base\n");
> +
> +	pcie->pciephy = syscon_regmap_lookup_by_phandle(node, "aspeed,pciephy");
> +	if (IS_ERR(pcie->pciephy))
> +		return dev_err_probe(dev, PTR_ERR(pcie->pciephy), "Failed to map pciephy base\n");
> +
> +	pcie->h2xrst = devm_reset_control_get_exclusive(dev, "h2x");
> +	if (IS_ERR(pcie->h2xrst))
> +		return dev_err_probe(dev, PTR_ERR(pcie->h2xrst), "Failed to get h2x reset\n");
> +
> +	pcie->perst = devm_reset_control_get_exclusive(dev, "perst");
> +	if (IS_ERR(pcie->perst))
> +		return dev_err_probe(dev, PTR_ERR(pcie->perst), "Failed to get perst reset\n");
> +
> +	ret = pcie->platform->setup(pdev);
> +	if (ret)
> +		goto err_setup;
> +
> +	host->sysdata = pcie;
> +
> +	ret = aspeed_pcie_init_irq_domain(pcie);
> +	if (ret)
> +		goto err_irq_init;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		goto err_irq;
> +
> +	ret = devm_request_irq(dev, irq, aspeed_pcie_intr_handler, IRQF_SHARED, dev_name(dev),
> +			       pcie);
> +	if (ret)
> +		goto err_irq;
> +
> +	pcie->clock = clk_get(dev, NULL);

Huh...

> +	if (IS_ERR(pcie->clock))
> +		goto err_clk;
> +	ret = clk_prepare_enable(pcie->clock);

devm_clk_get_enabled.

> +	if (ret)
> +		goto err_clk_enable;
> +
> +	ret = pci_host_probe(host);
> +	if (ret)
> +		goto err_clk_enable;
> +
> +	return 0;
> +
> +err_clk_enable:
> +	clk_put(pcie->clock);
> +err_clk:
> +err_irq:
> +	aspeed_pcie_irq_domain_free(pcie);
> +err_irq_init:
> +err_setup:
> +	return dev_err_probe(dev, ret, "Failed to setup PCIe RC\n");
> +}
> +
> +static void aspeed_pcie_remove(struct platform_device *pdev)
> +{
> +	struct aspeed_pcie *pcie = platform_get_drvdata(pdev);
> +
> +	if (pcie->clock) {
> +		clk_disable_unprepare(pcie->clock);
> +		clk_put(pcie->clock);
> +	}
> +
> +	pci_stop_root_bus(pcie->host->bus);
> +	pci_remove_root_bus(pcie->host->bus);
> +	aspeed_pcie_irq_domain_free(pcie);
> +}
> +
> +static struct aspeed_pcie_rc_platform pcie_rc_ast2600 = {

This should be const. Why it cannot?

> +	.setup = aspeed_ast2600_setup,
> +	.reg_intx_en = 0x04,
> +	.reg_intx_sts = 0x08,
> +	.reg_msi_en = 0x20,
> +	.reg_msi_sts = 0x28,
> +};
> +
> +static struct aspeed_pcie_rc_platform pcie_rc_ast2700 = {

This should be const. Why it cannot?

> +	.setup = aspeed_ast2700_setup,
> +	.reg_intx_en = 0x40,
> +	.reg_intx_sts = 0x48,
> +	.reg_msi_en = 0x50,
> +	.reg_msi_sts = 0x58,
> +};
> +
> +static const struct of_device_id aspeed_pcie_of_match[] = {
> +	{ .compatible = "aspeed,ast2600-pcie", .data = &pcie_rc_ast2600 },
> +	{ .compatible = "aspeed,ast2700-pcie", .data = &pcie_rc_ast2700 },
> +	{}
> +};
> +
> +static struct platform_driver aspeed_pcie_driver = {
> +	.driver = {
> +		.name = "aspeed-pcie",
> +		.suppress_bind_attrs = true,

Why?

> +		.of_match_table = aspeed_pcie_of_match,
> +	},
> +	.probe = aspeed_pcie_probe,
> +	.remove = aspeed_pcie_remove,

So how exactly remove can be triggered?

> +};
> +
> +module_platform_driver(aspeed_pcie_driver);
> +
> +MODULE_AUTHOR("Jacky Chou <jacky_chou@aspeedtech.com>");
> +MODULE_DESCRIPTION("ASPEED PCIe Root Complex");
> +MODULE_LICENSE("GPL");


Best regards,
Krzysztof