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: > > 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. > > > +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. > > config PCI_VERSATILE > > bool "ARM Versatile PB PCI controller" > > depends on ARCH_VERSATILE || COMPILE_TEST > > > +#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. > > +#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. > > +#define CRG0_READ_FMTTYPE > \ > > + FIELD_PREP(GENMASK(31, 24), > PCIE_TLP_FMT_TYPE(PCI_TLP_FMT_3DW_NO_DATA, \ > > + PCI_TLP_TYPE_CFG0_RD)) > > +#define CRG0_WRITE_FMTTYPE > \ > > + FIELD_PREP(GENMASK(31, 24), > PCIE_TLP_FMT_TYPE(PCI_TLP_FMT_3DW_DATA, \ > > + PCI_TLP_TYPE_CFG0_WR)) > > +#define CRG1_READ_FMTTYPE > \ > > + FIELD_PREP(GENMASK(31, 24), > PCIE_TLP_FMT_TYPE(PCI_TLP_FMT_3DW_NO_DATA, \ > > + PCI_TLP_TYPE_CFG1_RD)) > > +#define CRG1_WRITE_FMTTYPE > \ > > + FIELD_PREP(GENMASK(31, 24), > PCIE_TLP_FMT_TYPE(PCI_TLP_FMT_3DW_DATA, \ > > + PCI_TLP_TYPE_CFG1_WR)) > > Looks like a #define for GENMASK(31, 24) might make sense? > > > + * 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. > > + * @host: pointer to pcie host bridge > > s/pcie/PCIe/ > > > + * @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 > > + 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); }; > > > +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. > > +static struct irq_chip aspeed_intx_irq_chip = { > > + .name = "IntX", > > "INTx" (mentioned before). > > > +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. > > + 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. > > + 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 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. > > + return aspeed_ast2600_conf(bus, devfn, where, size, val, > > +CRG0_READ_FMTTYPE, false); } > > > +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. > > +static int aspeed_ast2700_child_config(struct pci_bus *bus, unsigned int > devfn, > > + 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; > > + 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. > > > +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. > > > +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); > > + 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, > > + 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@xxxxxxxxxxxxx > Agreed. I will refactor this part in next version. > > +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 | > > + 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); > > + > > + aspeed_host_reset(pcie); > > + > > + writel(0, pcie->reg + H2X_CTRL); > > + writel(H2X_BRIDGE_EN | H2X_BRIDGE_DIRECT_EN, pcie->reg + > H2X_CTRL); > > + > > + 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. > > > + /* Prepare for 64-bit BAR pref */ > > + writel(REMAP_PREF_ADDR_63_32(0x3), pcie->reg + > H2X_REMAP_PREF_ADDR); > > + > > + pcie->host->ops = &aspeed_ast2700_pcie_ops; > > + pcie->host->child_ops = &aspeed_ast2700_pcie_child_ops; > > + > > + return 0; > > +} > > > +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"); > > + 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. > > + 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; > > +} > > > +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); > > + 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. > > + 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->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), > > + 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. > > + 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. > > +}; > > + > > +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
