Message-ID:
 <SEYPR06MB5134E916ABFBF7FBB8744F189D03A@SEYPR06MB5134.apcprd06.prod.outlook.com>
Date: Fri, 5 Sep 2025 01:41:13 +0000
From: Jacky Chou <jacky_chou@...eedtech.com>
To: Bjorn Helgaas <helgaas@...nel.org>
CC: "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"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>,
	"vkoul@...nel.org" <vkoul@...nel.org>, "kishon@...nel.org"
	<kishon@...nel.org>, "linus.walleij@...aro.org" <linus.walleij@...aro.org>,
	"p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
	"linux-aspeed@...ts.ozlabs.org" <linux-aspeed@...ts.ozlabs.org>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-phy@...ts.infradead.org"
	<linux-phy@...ts.infradead.org>, "openbmc@...ts.ozlabs.org"
	<openbmc@...ts.ozlabs.org>, "linux-gpio@...r.kernel.org"
	<linux-gpio@...r.kernel.org>
Subject: [PATCH v3 09/10] PCI: aspeed: Add ASPEED PCIe RC driver

Hi Bjorn,

Thank you for your reply.

> > +/* 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".
> 

It is "configuration". I will change "CRG" to "CFG" in next version.

> > + * 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.
> 

Agreed.

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

Calling of_device_is_compatible() in the interrupt path is unnecessary.
In next version, I will add a variable in private at probe and 
have the ISR check the variable instead.

> > +				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;
> > +}
> 
> From comparing with other drivers, I doubt this is needed.
> 

Agreed.
It seems unnecessary. I will remove in next version.

> > +
> > +static struct irq_chip aspeed_msi_bottom_irq_chip = {
> > +	.name = "ASPEED MSI",
> > +	.irq_compose_msi_msg = aspeed_msi_compose_msi_msg,
> 
> I would prefer a name that matches irq_chip.irq_compose_msi_msg, e.g.,
> "aspeed_irq_compose_msi_msg()".
> 

Agreed.

> > +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);
> > +
> > +	struct irq_domain_info info = {
> > +		.fwnode		= dev_fwnode(pcie->dev),
> > +		.ops		= &aspeed_msi_domain_ops,
> > +		.host_data	= pcie,
> > +		.size		= MAX_MSI_HOST_IRQS,
> > +	};
> > +
> > +	pcie->msi_domain = msi_create_parent_irq_domain(&info,
> > +							&aspeed_msi_parent_ops);
> > +	if (!pcie->msi_domain)
> > +		return dev_err_probe(pcie->dev, -ENOMEM,
> > +				     "failed to create MSI domain\n");
> > +
> > +	return ret;
> 
> Useless "ret".  Remove it and just "return 0;"
> 

Agreed.

> > +static int aspeed_ast2600_setup(struct platform_device *pdev) {
> > +	struct aspeed_pcie *pcie = platform_get_drvdata(pdev);
> > +	struct device *dev = pcie->dev;
> > +
> > +	if (pcie->host_bus_num != 0x80)
> > +		return dev_err_probe(dev, -EINVAL,
> > +				     "The host bus must be 0x80\n");
> 
> Why not check this at the point you read it from the devicetree?
> 

Sorry.
Do you mean this check put in aspeed_pcie_parse_dt()?

> > +	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");
> 
> Same here.  Looks like a devicetree validation check.
> 

And this also puts to aspeed_pcie_parse_dt()?

> > +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;
> > +	int ret;
> > +
> > +	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> > +	if (!port)
> > +		return -ENOMEM;
> > +
> > +	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->phy = devm_of_phy_get(dev, node, NULL);
> > +	if (IS_ERR(port->phy))
> > +		return dev_err_probe(dev, PTR_ERR(port->phy),
> > +				     "failed to get phy pcie%d\n",
> > +				     port->slot);
> 
> port->slot hasn't been set yet.
> 

Agreed.
I will change it to "slot" in next version.

> > +	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);
> > +	ret = devm_add_action_or_reset(dev, aspeed_pcie_reset_release,
> > +				       port->perst);
> > +	if (ret)
> > +		return ret;
> > +	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 device_node *node = dev->of_node;
> > +	const struct aspeed_pcie_rc_platform *md;
> > +	u32 bus_range[2];
> > +	int irq, ret;
> > +
> > +	md = of_device_get_match_data(dev);
> > +	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);
> > +
> > +	ret = of_property_read_u32_array(node, "bus-range", bus_range,
> > +					 ARRAY_SIZE(bus_range));
> 
> No other drivers do this; why do you need it?
> 

I need to get the number of root bus.
At 2700 configuration command, it needs to use the next number of root bus
to send the header type of cfg command of TLP.

static int aspeed_ast2700_child_config(struct pci_bus *bus, unsigned int devfn,
				       int where, int size, u32 *val,
				       bool write)
{

.....

	if (write)
		cfg_val |= (bus->number == (pcie->host_bus_num + 1)) ?
				   CFG0_WRITE_FMTTYPE :
				   CFG1_WRITE_FMTTYPE;
	else
		cfg_val |= (bus->number == (pcie->host_bus_num + 1)) ?
				   CFG0_READ_FMTTYPE :
				   CFG1_READ_FMTTYPE;
	writel(cfg_val, pcie->reg + H2X_CFGE_TLP_1ST);

....

> > +	if (ret) {
> > +		dev_warn(dev, "failed to get bus range, assuming bus is 0\n");
> > +		pcie->host_bus_num = 0;
> > +	}
> > +	pcie->host_bus_num = bus_range[0];
> > +
> > +	pcie->reg = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(pcie->reg))
> > +		return PTR_ERR(pcie->reg);
> > +
> > +	pcie->domain = of_get_pci_domain_nr(node);
> 
> Almost no drivers use this; why do you need it?
> 

It is just used for message, must not be requirement.
I will remove it in next version.

> > +	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)
> > +		return ret;
> > +
> > +	host->sysdata = pcie;
> > +
> > +	ret = aspeed_pcie_init_irq_domain(pcie);
> > +	if (ret)
> > +		return ret;
> > +
> > +	irq = platform_get_irq(pdev, 0);
> > +	if (irq < 0)
> > +		return irq;
> > +
> > +	ret = devm_add_action_or_reset(dev, aspeed_pcie_irq_domain_free,
> pcie);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = devm_request_irq(dev, irq, aspeed_pcie_intr_handler,
> IRQF_SHARED,
> > +			       dev_name(dev), pcie);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = pci_host_probe(host);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> 
> This is the same as:
> 
>   return pci_host_probe(hoste);
> 

Agreed.

> > +}
> > +
> > +const struct aspeed_pcie_rc_platform pcie_rc_ast2600 = {
> > +	.setup = aspeed_ast2600_setup,
> > +	.reg_intx_en = 0xc4,
> > +	.reg_intx_sts = 0xc8,
> > +	.reg_msi_en = 0xe0,
> > +	.reg_msi_sts = 0xe8,
> > +	.msi_address = 0x1e77005c,
> 
> Where does this .msi_address come from?  Does this depend on an address
> map that could vary based on the platform?  Should it come from devicetree?
> 

The .msi_address is a SoC-fixed MSI doorbell target address used by the RC
to capture MSI writes. The memory write of MSI will not be mapped to RAM.
I think it should be keep in the SoC match data rather than Devicetree.

> > +};
> > +
> > +const 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,
> > +	.msi_address = 0x000000f0,
> > +};

Thanks,
Jacky

Powered by blists - more mailing lists