[PATCH v3 09/10] PCI: aspeed: Add ASPEED PCIe RC driver

[Bjorn Helgaas]

On Mon, Sep 01, 2025 at 01:59:21PM +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.

> +/* TLP configuration type 0 and type 1 */
> +#define CRG0_READ_FMTTYPE                                        \
> +	FIELD_PREP(ASPEED_TLP_COMMON_FIELDS,                     \
> +		   ASPEED_TLP_FMT_TYPE(PCIE_TLP_FMT_3DW_NO_DATA, \
> +				       PCIE_TLP_TYPE_CFG0_RD))
> +#define CRG0_WRITE_FMTTYPE                                    \
> +	FIELD_PREP(ASPEED_TLP_COMMON_FIELDS,                  \
> +		   ASPEED_TLP_FMT_TYPE(PCIE_TLP_FMT_3DW_DATA, \
> +				       PCIE_TLP_TYPE_CFG0_WR))
> +#define CRG1_READ_FMTTYPE                                        \
> +	FIELD_PREP(ASPEED_TLP_COMMON_FIELDS,                     \
> +		   ASPEED_TLP_FMT_TYPE(PCIE_TLP_FMT_3DW_NO_DATA, \
> +				       PCIE_TLP_TYPE_CFG1_RD))
> +#define CRG1_WRITE_FMTTYPE                                    \
> +	FIELD_PREP(ASPEED_TLP_COMMON_FIELDS,                  \
> +		   ASPEED_TLP_FMT_TYPE(PCIE_TLP_FMT_3DW_DATA, \
> +				       PCIE_TLP_TYPE_CFG1_WR))
> +#define CRG_PAYLOAD_SIZE		0x01 /* 1 DWORD */

What does "CRG" in the above mean?  If it means the same as "CFG",
i.e., an abbreviation for "configuration", can you use "CFG" instead?
It it's to match an internal spec, go ahead and keep "CRG".

> + * struct aspeed_pcie_rc_platform - Platform information
> + * @setup: initialization function
> + * @reg_intx_en: INTx enable register offset
> + * @reg_intx_sts: INTx status register offset
> + * @reg_msi_en: MSI enable register offset
> + * @reg_msi_sts: MSI enable register offset
> + * @msi_address: HW fixed MSI address
> + */
> +struct aspeed_pcie_rc_platform {
> +	int (*setup)(struct platform_device *pdev);
> +	int reg_intx_en;
> +	int reg_intx_sts;
> +	int reg_msi_en;
> +	int reg_msi_sts;
> +	int msi_address;

I think this should be u32 to match struct msi_msg.address_lo.

> +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 = FIELD_GET(PCIE_INTX_STS,
> +			 readl(pcie->reg + platform->reg_intx_sts));
> +	for_each_set_bit(bit, &intx, PCI_NUM_INTX)
> +		generic_handle_domain_irq(pcie->intx_domain, bit);
> +
> +	if (IS_ENABLED(CONFIG_PCI_MSI)) {
> +		for (i = 0; i < 2; i++) {
> +			int msi_sts_reg = platform->reg_msi_sts + (i * 4);
> +
> +			status = readl(pcie->reg + msi_sts_reg);
> +			writel(status, pcie->reg + msi_sts_reg);
> +
> +			/*
> +			 * AST2700 A1 workaround:
> +			 * The MSI status needs to clear one more time.
> +			 */
> +			if (of_device_is_compatible(pcie->dev->of_node,
> +						    "aspeed,ast2700-pcie"))

It looks pretty expensive to look this up for every interrupt.  It's
constant for the life of the driver, so you only need to do it once at
probe time.

> +				writel(status, pcie->reg + msi_sts_reg);
> +
> +			for_each_set_bit(bit, &status, 32) {
> +				bit += (i * 32);
> +				generic_handle_domain_irq(pcie->msi_domain,
> +							  bit);
> +			}
> +		}
> +	}
> +
> +	return IRQ_HANDLED;
> +}

> +static int aspeed_msi_set_affinity(struct irq_data *irq_data,
> +				   const struct cpumask *mask, bool force)
> +{
> +	return -EINVAL;
> +}