No subject

> +
> +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()".

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

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

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

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

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

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

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

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

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