| 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: <20250715162202.GA2461591@bhelgaas>
Date: Tue, 15 Jul 2025 11:22:02 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Jacky Chou <jacky_chou@...eedtech.com>
Cc: 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,
linux-aspeed@...ts.ozlabs.org, linux-pci@...r.kernel.org,
devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org, openbmc@...ts.ozlabs.org,
linux-gpio@...r.kernel.org, linus.walleij@...aro.org,
p.zabel@...gutronix.de, BMC-SW@...eedtech.com
Subject: Re: [PATCH v2 09/10] PCI: aspeed: Add ASPEED PCIe RC driver
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").
> 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?
> +#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?
> +#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.
> + * @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.
> + 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?
> +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?
> + 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.
> + 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.
> + 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.
> +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
> +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.
> + 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.
> + 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.
> + 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.
> +};
> +
> +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,
> +};
Bjorn
Powered by blists - more mailing lists