| 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:
<SEYPR06MB513497D82F1FBBFAC6B9EE309DA0A@SEYPR06MB5134.apcprd06.prod.outlook.com>
Date: Wed, 10 Dec 2025 02:17:31 +0000
From: Jacky Chou <jacky_chou@...eedtech.com>
To: Bjorn Helgaas <helgaas@...nel.org>
CC: Vinod Koul <vkoul@...nel.org>, Kishon Vijay Abraham I <kishon@...nel.org>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Joel Stanley <joel@....id.au>, Andrew
Jeffery <andrew@...econstruct.com.au>, Bjorn Helgaas <bhelgaas@...gle.com>,
Lorenzo Pieralisi <lpieralisi@...nel.org>,
Krzysztof Wilczyński <kwilczynski@...nel.org>, Manivannan
Sadhasivam <mani@...nel.org>, Linus Walleij <linus.walleij@...aro.org>,
Philipp Zabel <p.zabel@...gutronix.de>, "linux-aspeed@...ts.ozlabs.org"
<linux-aspeed@...ts.ozlabs.org>, "linux-pci@...r.kernel.org"
<linux-pci@...r.kernel.org>, "linux-phy@...ts.infradead.org"
<linux-phy@...ts.infradead.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>, Andrew Jeffery <andrew@...id.au>,
"openbmc@...ts.ozlabs.org" <openbmc@...ts.ozlabs.org>,
"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>
Subject: [PATCH v6 6/7] PCI: aspeed: Add ASPEED PCIe RC driver
Hi Bjorn,
Thank you for your reply.
> > + u8 tx_tag;
> > + int host_bus_num;
>
> Only needs a u8.
>
Agreed.
> > +static int aspeed_pcie_port_init(struct aspeed_pcie_port *port) {
> > + struct aspeed_pcie *pcie = port->pcie;
> > + struct device *dev = pcie->dev;
> > + int ret;
> > +
> > + ret = clk_prepare_enable(port->clk);
> > + if (ret)
> > + return dev_err_probe(dev, ret,
> > + "failed to set clock for slot (%d)\n",
> > + port->slot);
> > +
> > + ret = phy_init(port->phy);
> > + if (ret)
> > + return dev_err_probe(dev, ret,
> > + "failed to init phy pcie for slot (%d)\n",
> > + port->slot);
> > +
> > + ret = phy_set_mode_ext(port->phy, PHY_MODE_PCIE,
> PHY_MODE_PCIE_RC);
> > + if (ret)
> > + return dev_err_probe(dev, ret,
> > + "failed to set phy mode for slot (%d)\n",
> > + port->slot);
> > +
> > + reset_control_deassert(port->perst);
> > + mdelay(PCIE_RESET_CONFIG_WAIT_MS);
>
> I think this should use msleep().
>
Agreed.
> > +static int aspeed_ast2600_setup(struct platform_device *pdev) {
> > + struct aspeed_pcie *pcie = platform_get_drvdata(pdev);
> > + struct device *dev = pcie->dev;
> > +
> > + 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");
> > +
> > + aspeed_host_reset(pcie);
> > +
> > + regmap_write(pcie->ahbc, ASPEED_AHBC_KEY,
> ASPEED_AHBC_UNLOCK_KEY);
> > + regmap_update_bits(pcie->ahbc, ASPEED_AHBC_ADDR_MAPPING,
> > + ASPEED_PCIE_RC_MEMORY_EN,
> ASPEED_PCIE_RC_MEMORY_EN);
> > + regmap_write(pcie->ahbc, ASPEED_AHBC_KEY, ASPEED_AHBC_UNLOCK);
> > +
> > + /* Due to the BAR assignment is fixed mapping on 0x6000_0000.*/
> > + writel(ASPEED_AHB_REMAP_LO_ADDR(0x600) |
> ASPEED_AHB_MASK_LO_ADDR(0xe00),
> > + pcie->reg + ASPEED_H2X_AHB_ADDR_CONFIG0);
> > + writel(ASPEED_AHB_REMAP_HI_ADDR(0),
> > + pcie->reg + ASPEED_H2X_AHB_ADDR_CONFIG1);
>
> I assume this ASPEED_H2X_AHB_ADDR_CONFIG is doing basically the same
> thing as aspeed_ast2700_remap_pci_addr() below, so see the comments
> there.
>
Yes.
> > + writel(ASPEED_AHB_MASK_HI_ADDR(~0),
> > + pcie->reg + ASPEED_H2X_AHB_ADDR_CONFIG2);
> > + writel(ASPEED_H2X_BRIDGE_EN, pcie->reg + ASPEED_H2X_CTRL);
> > +
> > + writel(ASPEED_PCIE_RX_DMA_EN | ASPEED_PCIE_RX_LINEAR |
> > + ASPEED_PCIE_RX_MSI_SEL | ASPEED_PCIE_RX_MSI_EN |
> > + ASPEED_PCIE_WAIT_RX_TLP_CLR |
> ASPEED_PCIE_RC_RX_ENABLE |
> > + ASPEED_PCIE_RC_ENABLE,
> > + pcie->reg + ASPEED_H2X_DEV_CTRL);
> > +
> > + writel(ASPEED_RC_TLP_TX_TAG_NUM, pcie->reg +
> ASPEED_H2X_DEV_TX_TAG);
> > +
> > + pcie->host->ops = &aspeed_ast2600_pcie_ops;
> > + pcie->host->child_ops = &aspeed_ast2600_pcie_child_ops;
> > +
> > + return 0;
> > +}
> > +
> > +static int aspeed_ast2700_remap_pci_addr(struct aspeed_pcie *pcie) {
> > + struct device_node *dev_node = pcie->dev->of_node;
> > + struct of_pci_range range;
> > + struct of_pci_range_parser parser;
> > + int ret;
> > +
> > + ret = of_pci_range_parser_init(&parser, dev_node);
> > + if (ret)
> > + return ret;
> > +
> > + for_each_of_pci_range(&parser, &range) {
> > + if ((range.flags & IORESOURCE_TYPE_BITS) == IORESOURCE_MEM) {
> > + writel(ASPEED_REMAP_PCI_ADDR_31_12(range.pci_addr),
> > + pcie->reg + ASPEED_H2X_REMAP_PCI_ADDR_LO);
> > + writel(ASPEED_REMAP_PCI_ADDR_63_32(range.pci_addr),
> > + pcie->reg + ASPEED_H2X_REMAP_PCI_ADDR_HI);
> > + return 0;
>
> It looks like this is essentially hardcoding the offset between the
> parent-bus-address and the child-bus-address in the DT 'ranges'
> property. Since ASPEED_REMAP_PCI_ADDR_31_12() and
> ASPEED_REMAP_PCI_ADDR_63_32() do nothing except mask out the low 12
> bits, I assume that offset is zero.
>
> But this should not be hard-coded at all; it should be extracted from 'ranges'.
> I don't think we really have a consistent way of doing this, but you can take a
> look at how these other drivers program "outbound" mappings like this using
> bridge->windows:
>
> cdns_pcie_hpa_host_init_address_translation()
> dw_pcie_iatu_setup()
> mobiveil_host_init()
> xgene_pcie_map_ranges()
> iproc_pcie_map_ranges()
> rzg3s_pcie_parse_map_ranges()
>
Thank you for providing the information.
We will refer to this information to integrate the PCIe mapping range.
> > +static int aspeed_pcie_parse_dt(struct aspeed_pcie *pcie) {
> > + struct device *dev = pcie->dev;
> > + struct device_node *node = dev->of_node;
> > + int ret;
> > +
> > + for_each_available_child_of_node_scoped(node, child) {
> > + int slot;
> > + const char *type;
> > +
> > + ret = of_property_read_string(child, "device_type", &type);
> > + if (ret || strcmp(type, "pci"))
> > + continue;
> > +
> > + ret = of_pci_get_devfn(child);
> > + if (ret < 0)
> > + return dev_err_probe(dev, ret,
> > + "failed to parse devfn\n");
> > +
> > + slot = PCI_SLOT(ret);
> > +
> > + ret = aspeed_pcie_parse_port(pcie, child, slot);
> > + if (ret)
> > + return ret;
>
> It looks unnecessarily complicated to put each port on a list in
> aspeed_pcie_parse_port() and then iterate over that list in
> aspeed_pcie_init_ports().
>
> I think you could just do something like:
>
> aspeed_pcie_parse_port();
> aspeed_pcie_port_init();
>
> and get rid of the list completely.
>
Agreed.
We will remove the list and initialize the port directly.
> > + }
> > +
> > + if (list_empty(&pcie->ports))
> > + return dev_err_probe(dev, -ENODEV,
> > + "No PCIe port found in DT\n");
> > +
> > + 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 resource_entry *entry;
> > + const struct aspeed_pcie_rc_platform *md;
> > + 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);
> > +
> > + /* Get root bus num for cfg command to decide tlp type 0 or type 1 */
> > + entry = resource_list_first_type(&host->windows, IORESOURCE_BUS);
> > + if (entry)
> > + pcie->host_bus_num = entry->res->start;
>
> s/host_bus_num/root_bus_nr/ to match struct altera_pcie
Agreed.
Thanks,
Jacky
Powered by blists - more mailing lists