| 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:
<SEYPR06MB51342E52B3C4A7AFD42485039D7CA@SEYPR06MB5134.apcprd06.prod.outlook.com>
Date: Fri, 20 Jun 2025 06:05:20 +0000
From: Jacky Chou <jacky_chou@...eedtech.com>
To: Bjorn Helgaas <helgaas@...nel.org>
CC: "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-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.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>, "elbadrym@...gle.com" <elbadrym@...gle.com>,
"romlem@...gle.com" <romlem@...gle.com>, "anhphan@...gle.com"
<anhphan@...gle.com>, "wak@...gle.com" <wak@...gle.com>,
"yuxiaozhang@...gle.com" <yuxiaozhang@...gle.com>, BMC-SW
<BMC-SW@...eedtech.com>
Subject:
回覆: [PATCH 7/7] pci: aspeed: Add ASPEED PCIe host controller driver
> > 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.
>
> Make the subject match drivers/pci/controller/ style.
Agreed.
>
> > +config PCIE_ASPEED
> > + bool "ASPEED PCIe controller"
> > + depends on PCI
> > + depends on OF || COMPILE_TEST
> > + 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.
>
> Alphabetize this entry by vendor to match the file.
>
> Add blank line between paragraphs.
Agreed.
>
> > + * Copyright 2025 Aspeed Technology Inc.
>
> Settle on "ASPEED" or "Aspeed" in text/comment/etc to match corporate style.
> "aspeed" is good for the driver name, e.g., "PCI: aspeed: ..."
> for the subject.
Agreed.
>
> > +#include <linux/irqchip/chained_irq.h> #include <linux/irqdomain.h>
> > +#include <linux/mfd/syscon.h> #include <linux/kernel.h> #include
> > +<linux/msi.h> #include <linux/module.h> #include
> > +<linux/platform_device.h> #include <linux/of_platform.h> #include
> > +<linux/of_address.h> #include <linux/of_irq.h> #include
> > +<linux/of_pci.h> #include <linux/pci.h> #include <linux/regmap.h>
> > +#include <linux/reset.h> #include <linux/irq.h> #include
> > +<linux/interrupt.h> #include <linux/workqueue.h> #include
> > +<linux/gpio/consumer.h> #include <linux/bitfield.h> #include
> > +<linux/clk.h>
>
> The trend is to alphabetize these #includes.
Agreed.
>
> > +/* AST2600 PCIe Host Controller Registers */
> > +#define PEHR_MISC_10 0x10
> > +#define DATALINK_REPORT_CAPABLE BIT(4)
>
> Name register bits like these in a way that connects them with the register.
>
> > +static struct irq_chip aspeed_intx_irq_chip = {
> > + .name = "ASPEED:IntX",
>
> Usual styling is "INTx".
Agreed.
>
> > + .irq_ack = aspeed_pcie_intx_ack_irq,
> > + .irq_mask = aspeed_pcie_intx_mask_irq,
> > + .irq_unmask = aspeed_pcie_intx_unmask_irq,
>
> Name these functions to match the name of the function pointer, e.g.,
> aspeed_pcie_intx_irq_ack() instead of aspeed_pcie_intx_ack_irq() This makes
> grep/cscope more useful.
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 = readl(pcie->reg + platform->reg_intx_sts) & 0xf;
> > + if (intx) {
>
> Don't need "if (intx)" here; the loop is sufficient.
Agreed.
>
> > + for_each_set_bit(bit, &intx, PCI_NUM_INTX)
> > + generic_handle_domain_irq(pcie->irq_domain, bit);
> > + }
>
> > +static int aspeed_ast2600_rd_conf(struct pci_bus *bus, unsigned int devfn,
> > + int where, int size, u32 *val)
> > +{
> > + struct aspeed_pcie *pcie = bus->sysdata;
> > + u32 bdf_offset;
> > + int rx_done_fail = 0, slot = PCI_SLOT(devfn);
> > + u32 cfg_val, isr, type = 0;
> > + u32 link_sts = 0;
> > + int ret;
> > +
> > + /* Driver may set unlock RX buffere before triggering next TX config
> > +*/
>
> s/buffere/buffer/
>
> > + writel(PCIE_UNLOCK_RX_BUFF | readl(pcie->reg + H2X_DEV_CTRL),
> > + pcie->reg + H2X_DEV_CTRL);
> > +
> > + if (bus->number == 128 && slot != 0 && slot != 8)
> > + return PCIBIOS_DEVICE_NOT_FOUND;
> > + type = (bus->number > 128);
>
> Weird. What's all this? Some kind of device you want to hide?
> Deserves a hint about what's special.
The bus range in our AST2600 design is just starting from 128.
There is no something special.
I will use the child_ops that is in struct pci_host_bridge to distinguish the rc bridge
and the other bus.
>
> > + if (ret) {
> > + dev_err(pcie->dev,
> > + "[%X:%02X:%02X.%02X]CR tx timeout sts: 0x%08x\n",
>
> Conventional format is "%04x:%02x:%02x.%d" (4-digit domain, lower-case
> hex).
Agreed.
>
> > + pcie->domain, bus->number, PCI_SLOT(devfn),
> > + PCI_FUNC(devfn), cfg_val);
> > + ret = PCIBIOS_SET_FAILED;
> > + *val = ~0;
>
> PCI_SET_ERROR_RESPONSE(val)
Agreed.
>
> > +static int aspeed_ast2600_wr_conf(struct pci_bus *bus, unsigned int devfn,
> > + int where, int size, u32 val)
> > +{
> > + u32 type = 0;
> > + u32 shift = 8 * (where & 3);
> > + u32 bdf_offset;
> > + u8 byte_en = 0;
> > + struct aspeed_pcie *pcie = bus->sysdata;
> > + u32 isr, cfg_val;
> > + int ret;
> > +
> > + /* Driver may set unlock RX buffere before triggering next TX config
> > +*/
>
> s/buffere/buffer/
>
> I don't understand this; I suppose it requires hardware knowledge.
>
> > + writel(PCIE_UNLOCK_RX_BUFF | readl(pcie->reg + H2X_DEV_CTRL),
> > + pcie->reg + H2X_DEV_CTRL);
> > +
> > + switch (size) {
> > + case 1:
> > + byte_en = 1 << (where % 4);
> > + val = (val & 0xff) << shift;
> > + break;
> > + case 2:
> > + byte_en = 0x3 << (2 * ((where >> 1) % 2));
> > + val = (val & 0xffff) << shift;
> > + break;
> > + case 4:
> > + default:
> > + byte_en = 0xf;
> > + break;
> > + }
> > +
> > + type = (bus->number > 128);
>
> You're not allowed to *read* bus 128, dev 1, but you can write it?
Agreed.
I think pcie emulation will not find the device that is bus 128, dev1.
Because it cannot get VID/DID on bus 128, dev1.
Therefore, I have not add this check here.
I will add check here.
>
> > +static void aspeed_pcie_port_init(struct aspeed_pcie *pcie) {
> > + u32 link_sts = 0;
> > +
> > + regmap_write(pcie->pciephy, PEHR_LOCK, PCIE_UNLOCK);
> > + regmap_write(pcie->pciephy, PEHR_GLOBAL, ROOT_COMPLEX_ID(0x3));
> > +
> > + reset_control_deassert(pcie->perst);
> > + mdelay(500);
>
> Where did this come from? Should be a #define with reference to a spec.
>
> > +static int aspeed_ast2700_setup(struct platform_device *pdev) {
> > + struct aspeed_pcie *pcie = platform_get_drvdata(pdev);
> > + u32 cfg_val;
> > +
> > + reset_control_assert(pcie->perst);
> > +
> > + regmap_write(pcie->pciephy, PEHR_MISC_70,
> > + POSTED_DATA_CREDITS(0xc0) |
> POSTED_HEADER_CREDITS(0xa));
> > + regmap_write(pcie->pciephy, PEHR_MISC_78,
> > + COMPLETION_DATA_CREDITS(0x30) |
> COMPLETION_HEADER_CREDITS(0x8));
> > + regmap_write(pcie->pciephy, PEHR_MISC_58, LOCAL_SCALE_SUP);
> > +
> > + 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);
> > +
> > + reset_control_assert(pcie->h2xrst);
> > + mdelay(10);
>
> Source?
>
> > + reset_control_deassert(pcie->h2xrst);
> > +
> > + regmap_write(pcie->pciephy, PEHR_MISC_5C, CONFIG_RC_DEVICE);
> > + regmap_read(pcie->pciephy, PEHR_MISC_60, &cfg_val);
> > + regmap_write(pcie->pciephy, PEHR_MISC_60,
> > + (cfg_val & ~PORT_TPYE) | FIELD_PREP(PORT_TPYE,
> > +PORT_TYPE_ROOT));
> > +
> > + writel(0, pcie->reg + H2X_CTRL);
> > + writel(H2X_BRIDGE_EN | H2X_BRIDGE_DIRECT_EN, pcie->reg +
> H2X_CTRL);
> > +
> > + /* The BAR mapping:
> > + * CPU Node0(domain 0): 0x60000000
> > + * CPU Node1(domain 1): 0x80000000
> > + * IO (domain 2): 0xa0000000
>
> Are these addresses or sizes? Should they come from DT? Maybe it's
> something wired into the hardware?
Agreed.
>
> > + writel(REMAP_BAR_BASE(0x60000000 + (0x20000000 * pcie->domain)),
> > + pcie->reg + H2X_REMAP_DIRECT_ADDR);
> > +
> > + /* Prepare for 64-bit BAR pref */
> > + writel(REMAP_PREF_ADDR_63_32(0x3), pcie->reg +
> H2X_REMAP_PREF_ADDR);
> > +
> > + reset_control_deassert(pcie->perst);
> > + mdelay(1000);
>
> Source?
>
> > +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 void *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;
> > +
> > + pcie->reg = devm_platform_ioremap_resource(pdev, 0);
> > +
> > + of_property_read_u32(node, "msi_address", &pcie->msi_address);
> > + of_property_read_u32(node, "linux,pci-domain", &pcie->domain);
> > +
> > + 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->pciephy = syscon_regmap_lookup_by_phandle(node,
> "aspeed,pciephy");
> > + if (IS_ERR(pcie->pciephy))
> > + return dev_err_probe(dev, PTR_ERR(pcie->pciephy), "Failed to map
> > +pciephy 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");
> > +
> > + pcie->perst = devm_reset_control_get_exclusive(dev, "perst");
> > + if (IS_ERR(pcie->perst))
> > + return dev_err_probe(dev, PTR_ERR(pcie->perst), "Failed to get
> > +perst reset\n");
> > +
> > + ret = pcie->platform->setup(pdev);
> > + if (ret)
> > + goto err_setup;
> > +
> > + 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)
> > + goto err_irq;
> > +
> > + ret = devm_request_irq(dev, irq, aspeed_pcie_intr_handler, IRQF_SHARED,
> dev_name(dev),
> > + pcie);
> > + if (ret)
> > + goto err_irq;
> > +
> > + pcie->clock = clk_get(dev, NULL);
> > + if (IS_ERR(pcie->clock))
> > + goto err_clk;
> > + ret = clk_prepare_enable(pcie->clock);
> > + if (ret)
> > + goto err_clk_enable;
>
> We need to observe PCIE_T_RRS_READY_MS (or
> PCIE_RESET_CONFIG_DEVICE_WAIT_MS or whatever name we eventually
> settle
> on) before pci_host_probe() starts issuing config reads. Maybe this is
> accounted for by one of the sleeps above, but we need a generic #define that
> we can look for.
Agreed.
> > + ret = pci_host_probe(host);
> > + if (ret)
> > + goto err_clk_enable;
> > +
> > + return 0;
>
> Sorry, I see there's a lot of duplication with comments from other reviewers :)
>
> Bjorn
Powered by blists - more mailing lists