Message-ID:
 <SEYPR06MB5134692DCCFD55F5ABD812F39D3DA@SEYPR06MB5134.apcprd06.prod.outlook.com>
Date: Fri, 22 Aug 2025 07:00:25 +0000
From: Jacky Chou <jacky_chou@...eedtech.com>
To: Bjorn Helgaas <helgaas@...nel.org>
CC: "bhelgaas@...gle.com" <bhelgaas@...gle.com>, "lpieralisi@...nel.org"
	<lpieralisi@...nel.org>, "kwilczynski@...nel.org" <kwilczynski@...nel.org>,
	"mani@...nel.org" <mani@...nel.org>, "robh@...nel.org" <robh@...nel.org>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>, "conor+dt@...nel.org"
	<conor+dt@...nel.org>, "joel@....id.au" <joel@....id.au>,
	"andrew@...econstruct.com.au" <andrew@...econstruct.com.au>,
	"linux-aspeed@...ts.ozlabs.org" <linux-aspeed@...ts.ozlabs.org>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "openbmc@...ts.ozlabs.org"
	<openbmc@...ts.ozlabs.org>, "linux-gpio@...r.kernel.org"
	<linux-gpio@...r.kernel.org>, "linus.walleij@...aro.org"
	<linus.walleij@...aro.org>, "p.zabel@...gutronix.de"
	<p.zabel@...gutronix.de>, BMC-SW <BMC-SW@...eedtech.com>
Subject:
 回覆: [PATCH v2 09/10] PCI: aspeed: Add ASPEED PCIe RC driver

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@linutronix.de
> 

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

Powered by blists - more mailing lists