Hi Bjorn,

Thank you for your reply.

v1 posting was
https://lore.kernel.org/r/20250613033001.3153637-1-jacky_chou@aspeedtech
.com
Links to previous postings are helpful in the cover letter.

On Tue, Jul 15, 2025 at 11:43:19AM +0800, Jacky Chou wrote:

quoted

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.

quoted

+config PCIE_ASPEED
+	bool "ASPEED PCIe controller"
+	depends on ARCH_ASPEED || COMPILE_TEST
+	depends on OF
+	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.

Add blank line between paragraphs or reflow into single paragraph (mentioned
before).

Alphabetize this entry by the menu item ("ASPEED PCIe controller", "ARM
Versatile PB PCI controller").

Sorry, I miss it.
I will modify it in next version.

quoted

 config PCI_VERSATILE
 	bool "ARM Versatile PB PCI controller"
 	depends on ARCH_VERSATILE || COMPILE_TEST

quoted

+#define MAX_MSI_HOST_IRQS	64
+#define PCIE_RESET_CONFIG_DEVICE_WAIT_MS	500

Where does this value come from?  Is there a generic value from
drivers/pci/pci.h you can use?

We check the PCIe specification to find these contents.
"With a Downstream Port that supports Link speeds greater than 5.0 GT/s, software must wait a
minimum of 100 ms after Link training completes before sending a Configuration Request to the
device immediately below that Port."
So, we think delay 500ms to let kernel issue the first configuration command is enough
after deassert PERST.

quoted

+#define PCIE_RESET_CONFIG_RC_WAIT_MS		10

Ditto.  If it's an Aspeed-specific value, can you point to the source in the
Aspeed datasheet?

This delay is set to ensure that the RC internal settings are completely reset.
We do not put its usage in our datasheet.

quoted

+#define CRG0_READ_FMTTYPE

\

quoted

+	FIELD_PREP(GENMASK(31, 24),

PCIE_TLP_FMT_TYPE(PCI_TLP_FMT_3DW_NO_DATA, \

quoted

+						      PCI_TLP_TYPE_CFG0_RD))
+#define CRG0_WRITE_FMTTYPE

\

quoted

+	FIELD_PREP(GENMASK(31, 24),

PCIE_TLP_FMT_TYPE(PCI_TLP_FMT_3DW_DATA, \

quoted

+						      PCI_TLP_TYPE_CFG0_WR))
+#define CRG1_READ_FMTTYPE

\

quoted

+	FIELD_PREP(GENMASK(31, 24),

PCIE_TLP_FMT_TYPE(PCI_TLP_FMT_3DW_NO_DATA, \

quoted

+						      PCI_TLP_TYPE_CFG1_RD))
+#define CRG1_WRITE_FMTTYPE

\

quoted

+	FIELD_PREP(GENMASK(31, 24),

PCIE_TLP_FMT_TYPE(PCI_TLP_FMT_3DW_DATA, \

quoted

+						      PCI_TLP_TYPE_CFG1_WR))

Looks like a #define for GENMASK(31, 24) might make sense?

quoted

+ * struct aspeed_pcie_port - PCIe port information
+ * @list: port list
+ * @pcie: pointer to PCIe host info
+ * @clk: pointer to the port clock gate
+ * @phy: pointer to PHY control block
+ * @perst: pointer to port reset control
+ * @slot: port slot
+ */
+struct aspeed_pcie_port {
+	struct list_head list;
+	struct aspeed_pcie *pcie;
+	struct clk *clk;
+	struct regmap *pciephy;
+	struct reset_control *perst;
+	u32 slot;
+};
+
+/**
+ * struct aspeed_pcie - PCIe port information

I think aspeed_pcie_port is for a Root Port; this looks like it might be for the
Root Complex as a whole.

Yes, struct aspeed_pcie is for Root Complex.
I will correct it in next version.

quoted

+ * @host: pointer to pcie host bridge

s/pcie/PCIe/

quoted

+ * @dev: pointer to device structure
+ * @reg: PCIe Host register base address
+ * @ahbc: pointer to AHHC register map
+ * @cfg: pointer to Aspeed PCIe configuration register map
+ * @platform: platform specific information
+ * @ports: list of PCIe ports
+ * @domain: PCI domain number
+ * @tx_tag: current TX tag for the port
+ * @h2xrst: pointer to H2X reset control
+ * @irq_domain: IRQ domain for INTx interrupts
+ * @dev_domain: IRQ domain for device interrupts
+ * @msi_domain: IRQ domain for MSI interrupts
+ * @lock: mutex to protect MSI bitmap variable
+ * @msi_irq_in_use: bitmap to track used MSI host IRQs  */ struct
+aspeed_pcie {
+	struct pci_host_bridge *host;
+	struct device *dev;
+	void __iomem *reg;
+	struct regmap *ahbc;
+	struct regmap *cfg;
+	const struct aspeed_pcie_rc_platform *platform;
+	struct list_head ports;
+
+	int domain;
+	u8 tx_tag;
+
+	struct reset_control *h2xrst;
+
+	struct irq_domain *irq_domain;

Name "intx_domain" to remove ambiguity.

Agreed

quoted

+	struct irq_domain *dev_domain;
+	struct irq_domain *msi_domain;
+	struct mutex lock;		/* Protect MSI bitmap variable */
+	DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_HOST_IRQS); };

quoted

+static void aspeed_pcie_intx_irq_ack(struct irq_data *d) {
+	struct aspeed_pcie *pcie = irq_data_get_irq_chip_data(d);
+	int intx_en = pcie->platform->reg_intx_en;
+	u32 en;
+
+	en = readl(pcie->reg + intx_en);
+	en |= BIT(d->hwirq);
+	writel(en, pcie->reg + intx_en);
+}
+
+static void aspeed_pcie_intx_irq_mask(struct irq_data *d) {
+	struct aspeed_pcie *pcie = irq_data_get_irq_chip_data(d);
+	int intx_en = pcie->platform->reg_intx_en;
+	u32 en;
+
+	en = readl(pcie->reg + intx_en);
+	en |= BIT(d->hwirq);
+	writel(en, pcie->reg + intx_en);
+}
+
+static void aspeed_pcie_intx_irq_unmask(struct irq_data *d) {
+	struct aspeed_pcie *pcie = irq_data_get_irq_chip_data(d);
+	int intx_en = pcie->platform->reg_intx_en;
+	u32 en;
+
+	en = readl(pcie->reg + intx_en);
+	en |= BIT(d->hwirq);
+	writel(en, pcie->reg + intx_en);
+}

aspeed_pcie_intx_irq_ack(), aspeed_pcie_intx_irq_mask(), and
aspeed_pcie_intx_irq_unmask() all look identical.  Am I missing something?

No.
Here is something wrong in aspeed_pcie_intx_irq_mask function.
I will fix it in next version.

quoted

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

"INTx" (mentioned before).

quoted

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

Looks like status and intx should both be u32?

Agreed.
It is 32-bit register, so I will change to u32.

quoted

+	u32 bit;
+	int i;
+
+	intx = readl(pcie->reg + platform->reg_intx_sts) & PCIE_INTX_STS;

Use FIELD_GET() to avoid assumption that PCIE_INTX_STS starts at bit 0.

Agreed.

quoted

+	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;
+}

quoted

+static int aspeed_ast2600_rd_conf(struct pci_bus *bus, unsigned int devfn,
+				  int where, int size, u32 *val)
+{
+	int slot = PCI_SLOT(devfn);
+
+	if (slot != 0 && slot != 8)
+		return PCIBIOS_DEVICE_NOT_FOUND;

I previously asked for a hint about why certain device addresses aren't valid.  I
meant a comment with a hint so the same question doesn't arise every time.

I will add some comments in the same places that need to explain.

quoted

+	return aspeed_ast2600_conf(bus, devfn, where, size, val,
+CRG0_READ_FMTTYPE, false); }

quoted

+static bool aspeed_ast2700_get_link(struct aspeed_pcie_port *port) {
+	u32 reg;
+
+	/* AST2700 has Gen2 and Gen4 RCs.
+	 * Read register to distinguish between Gen2 or Gen4.
+	 * Then read the corresonding register that is from Aspeed
+	 * design to get whether it linked up or not.
+	 */

Comment style is:

  /*
   * Text ...
   */

Add blank line between paragraphs or reflow to a single paragraph.

s/corresonding/corresponding/

"that is from Aspeed design" seems unnecessary.

Agreed.

quoted

+static int aspeed_ast2700_child_config(struct pci_bus *bus, unsigned int

devfn,

quoted

+				       int where, int size, u32 *val,
+				       bool write)
+{
+	struct aspeed_pcie *pcie = bus->sysdata;
+	u32 bdf_offset, status, cfg_val;
+	int ret;
+
+	bdf_offset = aspeed_pcie_get_bdf_offset(bus, devfn, where);
+
+	cfg_val = CRG_PAYLOAD_SIZE;
+	if (write)
+		cfg_val |= (bus->number == 1) ? CRG0_WRITE_FMTTYPE :

CRG1_WRITE_FMTTYPE;

quoted

+	else
+		cfg_val |= (bus->number == 1) ? CRG0_READ_FMTTYPE :
+CRG1_READ_FMTTYPE;

I don't think you should assume that bus 0 is the root bus.  The root bus
number should come from the DT bus-range.

quoted

+static int aspeed_ast2700_rd_conf(struct pci_bus *bus, unsigned int devfn,
+				  int where, int size, u32 *val)
+{
+	if (devfn != 0)
+		return PCIBIOS_DEVICE_NOT_FOUND;

Another good place for a hint about why this restriction exists.

quoted

+static int aspeed_pcie_msi_init(struct aspeed_pcie *pcie) {
+	int ret = 0;
+
+	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);
+
+	pcie->dev_domain =
+		irq_domain_add_linear(NULL, MAX_MSI_HOST_IRQS,

&aspeed_msi_domain_ops, pcie);

quoted

+	if (!pcie->dev_domain)
+		return dev_err_probe(pcie->dev, -ENOMEM, "failed to create IRQ
+domain\n");
+
+	pcie->msi_domain = pci_msi_create_irq_domain(dev_fwnode(pcie->dev),

&aspeed_msi_domain_info,

quoted

+						     pcie->dev_domain);
+	if (!pcie->msi_domain)
+		return dev_err_probe(pcie->dev, -ENOMEM, "failed to create MSI
+domain\n");

Can you please rework this to follow what Nam Cao is doing for existing
drivers:
https://lore.kernel.org/r/cover.1750858083.git.namcao@linutronix.de (local)

Agreed.
I will refactor this part in next version.

quoted

+static int aspeed_ast2700_setup(struct platform_device *pdev) {
+	struct aspeed_pcie *pcie = platform_get_drvdata(pdev);
+	int ret;
+
+	regmap_update_bits(pcie->cfg, SCU_60,
+			   RC_E2M_PATH_EN | RC_H2XS_PATH_EN |

RC_H2XD_PATH_EN | RC_H2XX_PATH_EN |

quoted

+			   RC_UPSTREAM_MEM_EN,
+			   RC_E2M_PATH_EN | RC_H2XS_PATH_EN |

RC_H2XD_PATH_EN | RC_H2XX_PATH_EN |

quoted

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

quoted

+		     RC1_DECODE_DMA_LIMIT(0xFF));
+	regmap_write(pcie->cfg, SCU_70, DISABLE_EP_FUNC);
+
+	aspeed_host_reset(pcie);
+
+	writel(0, pcie->reg + H2X_CTRL);
+	writel(H2X_BRIDGE_EN | H2X_BRIDGE_DIRECT_EN, pcie->reg +

H2X_CTRL);

quoted

+
+	ret = aspeed_ast2700_bar_assign(pcie);
+	if (ret)
+		return dev_err_probe(pcie->dev, ret, "Failed to assign bar\n");

s/bar/BAR/ in the message since it's an acronym.

quoted

+	/* Prepare for 64-bit BAR pref */
+	writel(REMAP_PREF_ADDR_63_32(0x3), pcie->reg +

H2X_REMAP_PREF_ADDR);

quoted

+
+	pcie->host->ops = &aspeed_ast2700_pcie_ops;
+	pcie->host->child_ops = &aspeed_ast2700_pcie_child_ops;
+
+	return 0;
+}

quoted

+static int aspeed_pcie_parse_port(struct aspeed_pcie *pcie,
+				  struct device_node *node,
+				  int slot)
+{
+	struct aspeed_pcie_port *port;
+	struct device *dev = pcie->dev;
+
+	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
+	if (!port)
+		return -ENOMEM;
+
+	port->pciephy = syscon_regmap_lookup_by_phandle(node,

"aspeed,pciephy");

quoted

+	if (IS_ERR(port->pciephy))
+		return dev_err_probe(dev, PTR_ERR(port->pciephy),
+				     "Failed to map pcie%d pciephy base\n", slot);
+
+	port->clk = devm_get_clk_from_child(dev, node, NULL);
+	if (IS_ERR(port->clk))
+		return dev_err_probe(dev, PTR_ERR(port->clk),
+				     "Failed to get pcie%d clock\n", slot);
+
+	port->perst = of_reset_control_get_exclusive(node, "perst");
+	if (IS_ERR(port->perst))
+		return dev_err_probe(dev, PTR_ERR(port->perst),
+				     "Failed to get pcie%d reset control\n", slot);

Driver messages are inconsistently capitalized.

I will confirm all codes.

quoted

+	reset_control_assert(port->perst);
+
+	port->slot = slot;
+	port->pcie = pcie;
+
+	INIT_LIST_HEAD(&port->list);
+	list_add_tail(&port->list, &pcie->ports);
+
+	return 0;
+}

quoted

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

of_device_get_match_data(dev);

quoted

+	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;
+	INIT_LIST_HEAD(&pcie->ports);
+
+	pcie->reg = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(pcie->reg))
+		return PTR_ERR(pcie->reg);
+
+	of_property_read_u32(node, "linux,pci-domain", &pcie->domain);

Normally not needed in a controller driver.  See
of_get_pci_domain_nr() and related interfaces.

Agreed.

quoted

+	pcie->cfg = syscon_regmap_lookup_by_phandle(dev->of_node,

"aspeed,pciecfg");

quoted

+	if (IS_ERR(pcie->cfg))
+		return dev_err_probe(dev, PTR_ERR(pcie->cfg), "Failed to map
+pciecfg 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");
+
+	ret = devm_mutex_init(dev, &pcie->lock);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to init mutex\n");
+
+	ret = pcie->platform->setup(pdev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to setup PCIe RC\n");
+
+	ret = aspeed_pcie_parse_dt(pcie);
+	if (ret)
+		return ret;
+
+	ret = aspeed_pcie_init_ports(pcie);
+	if (ret)
+		goto err_remove_resets;
+
+	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) {
+		ret = irq;
+		goto err_irq;
+	}
+
+	ret = devm_request_irq(dev, irq, aspeed_pcie_intr_handler, IRQF_SHARED,

dev_name(dev),

quoted

+			       pcie);

Most of this file is formatted to fit in 80 columns; would be nice to make the
few outliers also fit.

I will adjust this driver to be consistent in style.

quoted

+	if (ret)
+		goto err_irq;
+
+	ret = pci_host_probe(host);
+	if (ret)
+		goto err_irq;
+
+	return 0;
+err_irq:
+	aspeed_pcie_irq_domain_free(pcie);
+err_irq_init:
+err_remove_resets:
+	list_for_each_entry(port, &pcie->ports, list)
+		reset_control_put(port->perst);
+	dev_err_probe(dev, ret, "Failed to initial RC\n");
+	return ret;
+}
+
+const struct aspeed_pcie_rc_platform pcie_rc_ast2600 = {
+	.setup = aspeed_ast2600_setup,
+	.get_link = aspeed_ast2600_get_link,
+	.port_init = aspeed_ast2600_port_init,
+	.reg_intx_en = 0x04,
+	.reg_intx_sts = 0x08,
+	.reg_msi_en = 0x20,
+	.reg_msi_sts = 0x28,
+	.msi_address = 0x1e77005c,

Capitalize hex numbers consistently.  It appears upper-case is the convention
in this file.

Agreed.
I will adjust this driver to be consistent in style.

quoted

+};
+
+const struct aspeed_pcie_rc_platform pcie_rc_ast2700 = {
+	.setup = aspeed_ast2700_setup,
+	.get_link = aspeed_ast2700_get_link,
+	.port_init = aspeed_ast2700_port_init,
+	.reg_intx_en = 0x40,
+	.reg_intx_sts = 0x48,
+	.reg_msi_en = 0x50,
+	.reg_msi_sts = 0x58,
+	.msi_address = 0x000000F0,
+}

Thanks,
Jacky