| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250903224820.GA1234878@bhelgaas>
Date: Wed, 3 Sep 2025 17:48:20 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Jacky Chou <jacky_chou@...eedtech.com>
Cc: linux-pci@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, bhelgaas@...gle.com,
lpieralisi@...nel.org, kwilczynski@...nel.org, mani@...nel.org,
robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
joel@....id.au, andrew@...econstruct.com.au, vkoul@...nel.org,
kishon@...nel.org, linus.walleij@...aro.org, p.zabel@...gutronix.de,
linux-aspeed@...ts.ozlabs.org, linux-arm-kernel@...ts.infradead.org,
linux-phy@...ts.infradead.org, openbmc@...ts.ozlabs.org,
linux-gpio@...r.kernel.org
Subject: Re: [PATCH v3 09/10] PCI: aspeed: Add ASPEED PCIe RC driver
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;
> +}
>From comparing with other drivers, I doubt this is needed.
> +
> +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,
> +};
Powered by blists - more mailing lists