| 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: <20250904160140.GA1263976@bhelgaas>
Date: Thu, 4 Sep 2025 11:01:40 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: zhangsenchuan@...incomputing.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, linux-pci@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
p.zabel@...gutronix.de, johan+linaro@...nel.org,
quic_schintav@...cinc.com, shradha.t@...sung.com, cassel@...nel.org,
thippeswamy.havalige@....com, mayank.rana@....qualcomm.com,
inochiama@...il.com, ningyu@...incomputing.com,
linmin@...incomputing.com, pinkesh.vaghela@...fochips.com
Subject: Re: [PATCH v2 2/2] PCI: eic7700: Add Eswin eic7700 PCIe host
controller driver
On Fri, Aug 29, 2025 at 04:24:05PM +0800, zhangsenchuan@...incomputing.com wrote:
> From: Senchuan Zhang <zhangsenchuan@...incomputing.com>
>
> Add driver for the Eswin EIC7700 PCIe host controller.
> The controller is based on the DesignWare PCIe core.
Wrap to fill 75 columns.
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -492,4 +492,16 @@ config PCIE_VISCONTI_HOST
> Say Y here if you want PCIe controller support on Toshiba Visconti SoC.
> This driver supports TMPV7708 SoC.
>
> +config PCIE_EIC7700
> + tristate "ESWIN PCIe host controller"
Should say "ESWIN PCIe controller" to match other entries.
> + depends on PCI_MSI
> + depends on ARCH_ESWIN || COMPILE_TEST
Reorder these to match other entries, i.e.,
depends on ARCH_ESWIN || COMPILE_TEST
depends on PCI_MSI
> + select PCIE_DW_HOST
> + help
> + Enables support for the PCIe controller in the Eswin SoC
> + The PCI controller on Eswin is based on DesignWare hardware
> + It is a high-speed hardware bus standard used to connect
> + processors with external devices. Say Y here if you want
> + PCIe controller support for the ESWIN.
Alphabetize so the menuconfig entries remain sorted by vendor.
> +++ b/drivers/pci/controller/dwc/pcie-eic7700.c
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +#include <linux/resource.h>
> +#include <linux/types.h>
> +#include <linux/interrupt.h>
> +#include <linux/iopoll.h>
> +#include <linux/reset.h>
> +#include <linux/pm_runtime.h>
Usually people sort these alphabetically.
> +#define PCIE_PM_SEL_AUX_CLK BIT(16)
> +#define PCIEMGMT_APP_LTSSM_ENABLE BIT(5)
Put these under the offset of the register that contains them so we
can tell the connections.
> +#define PCIEMGMT_CTRL0_OFFSET 0x0
> +#define PCIEMGMT_STATUS0_OFFSET 0x100
> +
> +#define PCIE_TYPE_DEV_VEND_ID 0x0
This looks like PCI_VENDOR_ID; use that instead.
> +#define PCIE_DSP_PF0_MSI_CAP 0x50
> +#define PCIE_NEXT_CAP_PTR 0x70
These look like fixed offsets in config space that should be
discovered by the usual method of traversing the capability lists,
e.g., dw_pcie_find_capability().
> +#define DEVICE_CONTROL_DEVICE_STATUS 0x78
I don't think you need this at all (see below). But if you do, the
use below should include PCI_EXP_DEVCTL (e.g., as an offset from the
start of the PCIe Capability) so grep can find it.
> +#define PCIE_MSI_MULTIPLE_MSG_32 (0x5 << 17)
> +#define PCIE_MSI_MULTIPLE_MSG_MASK (0x7 << 17)
Use PCI_MSI_FLAGS_QMASK instead.
> +#define PCIEMGMT_LINKUP_STATE_VALIDATE ((0x11 << 2) | 0x3)
> +#define PCIEMGMT_LINKUP_STATE_MASK 0xff
Line up all the values of these #defines.
> +static int eswin_pcie_host_init(struct dw_pcie_rp *pp)
> +{
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct eswin_pcie *pcie = dev_get_drvdata(pci->dev);
> + int ret;
> + u32 val;
> + u32 retries;
> +
> + /* Fetch clocks */
> + pcie->num_clks = devm_clk_bulk_get_all_enabled(pci->dev, &pcie->clks);
> + if (pcie->num_clks < 0)
> + return dev_err_probe(pci->dev, pcie->num_clks,
> + "failed to get pcie clocks\n");
> +
> + ret = eswin_pcie_power_on(pcie);
> + if (ret)
> + return ret;
> +
> + /* set device type : rc */
> + val = readl_relaxed(pcie->mgmt_base + PCIEMGMT_CTRL0_OFFSET);
> + val &= 0xfffffff0;
> + writel_relaxed(val | 0x4, pcie->mgmt_base + PCIEMGMT_CTRL0_OFFSET);
"rc" is not a device type (a Root Complex is not itself a PCI device
that appears in config space). I think you're talking about a Root
Port, which is a PCIe device, so this should be
PCI_EXP_TYPE_ROOT_PORT.
> + ret = reset_control_assert(pcie->perst);
> + if (ret) {
> + dev_err(pci->dev, "perst assert signal is invalid");
> + goto err_perst;
> + }
> + msleep(100);
This sleep needs a comment (if it's specific to eic7700) or a standard
#define from drivers/pci/pci.h (if something from the PCIe spec).
> + ret = reset_control_deassert(pcie->perst);
> + if (ret) {
> + dev_err(pci->dev, "perst deassert signal is invalid");
> + goto err_perst;
> + }
> +
> + /* app_hold_phy_rst */
> + val = readl_relaxed(pcie->mgmt_base + PCIEMGMT_CTRL0_OFFSET);
> + val &= ~(0x40);
> + writel_relaxed(val, pcie->mgmt_base + PCIEMGMT_CTRL0_OFFSET);
> +
> + /*
> + * It takes at least 20ms to wait for the pcie
> + * status register to be 0.
s/pcie/PCIe/ (follow spec usage in comments, messages, etc)
> + */
> + retries = 30;
> + do {
> + val = readl_relaxed(pcie->mgmt_base + PCIEMGMT_STATUS0_OFFSET);
> + if (!(val & PCIE_PM_SEL_AUX_CLK))
> + break;
> + usleep_range(1000, 1100);
> + retries--;
> + } while (retries);
This delay should also have a citation to eic7700 spec if it came from
there. Suggest fsleep() instead of usleep_range(). The exact delay
doesn't look critical here.
> + if (!retries) {
> + dev_err(pci->dev, "No clock exist.\n");
Drop period at end of messages.
> + ret = -ENODEV;
> + goto err_clock;
> + }
> +
> + /* config eswin vendor id and eic7700 device id */
> + dw_pcie_writel_dbi(pci, PCIE_TYPE_DEV_VEND_ID, 0x20301fe1);
> +
> + /* lane fix config, real driver NOT need, default x4 */
> + val = dw_pcie_readl_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL);
> + val &= 0xffffff80;
> + val |= 0x44;
> + dw_pcie_writel_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL, val);
> +
> + val = dw_pcie_readl_dbi(pci, DEVICE_CONTROL_DEVICE_STATUS);
> + val &= ~(0x7 << 5);
> + val |= (0x2 << 5);
> + dw_pcie_writel_dbi(pci, DEVICE_CONTROL_DEVICE_STATUS, val);
This sets MPS, which should be done by the PCI core, not by the
driver.
> + /* config support 32 msi vectors */
Use "MSI" and "MSI-X" in comments, etc to match spec usage.
> + val = dw_pcie_readl_dbi(pci, PCIE_DSP_PF0_MSI_CAP);
> + val &= ~PCIE_MSI_MULTIPLE_MSG_MASK;
> + val |= PCIE_MSI_MULTIPLE_MSG_32;
Use FIELD_PREP() as in dw_pcie_ep_set_msi().
> + dw_pcie_writel_dbi(pci, PCIE_DSP_PF0_MSI_CAP, val);
> +
> + /* disable msix cap */
> + val = dw_pcie_readl_dbi(pci, PCIE_NEXT_CAP_PTR);
> + val &= 0xffff00ff;
> + dw_pcie_writel_dbi(pci, PCIE_NEXT_CAP_PTR, val);
> +
> + return 0;
> +
> +err_clock:
> + reset_control_assert(pcie->perst);
> +err_perst:
> + eswin_pcie_power_off(pcie);
> + return ret;
> +}
> +
> +static const struct dw_pcie_host_ops eswin_pcie_host_ops = {
> + .init = eswin_pcie_host_init,
> +};
> +
> +static const struct dw_pcie_ops dw_pcie_ops = {
> + .start_link = eswin_pcie_start_link,
> + .link_up = eswin_pcie_link_up,
> +};
> +
> +static int eswin_pcie_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct dw_pcie *pci;
> + struct eswin_pcie *pcie;
> + int ret;
> +
> + pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
> + if (!pcie)
> + return -ENOMEM;
> +
> + pci = &pcie->pci;
> + pci->dev = dev;
> + pci->ops = &dw_pcie_ops;
> + pci->pp.ops = &eswin_pcie_host_ops;
> +
> + /* SiFive specific region: mgmt */
> + pcie->mgmt_base = devm_platform_ioremap_resource_byname(pdev, "mgmt");
> + if (IS_ERR(pcie->mgmt_base))
> + return dev_err_probe(dev, PTR_ERR(pcie->mgmt_base),
> + "failed to map mgmt memory\n");
> +
> + /* Fetch reset */
> + pcie->powerup_rst = devm_reset_control_get_optional(&pdev->dev,
> + "powerup");
> + if (IS_ERR_OR_NULL(pcie->powerup_rst))
> + return dev_err_probe(dev, PTR_ERR(pcie->powerup_rst),
> + "unable to get powerup reset\n");
> +
> + pcie->cfg_rst = devm_reset_control_get_optional(&pdev->dev, "cfg");
> + if (IS_ERR_OR_NULL(pcie->cfg_rst))
> + return dev_err_probe(dev, PTR_ERR(pcie->cfg_rst),
> + "unable to get cfg reset\n");
> +
> + pcie->perst = devm_reset_control_get_optional(&pdev->dev, "pwren");
Why is this not called "perst" in devicetree?
> + if (IS_ERR_OR_NULL(pcie->perst))
> + return dev_err_probe(dev, PTR_ERR(pcie->perst),
> + "unable to get perst reset\n");
> +
> + platform_set_drvdata(pdev, pcie);
> +
> + pm_runtime_set_active(dev);
> + pm_runtime_enable(dev);
> + ret = pm_runtime_get_sync(dev);
> + if (ret < 0) {
> + dev_err(dev, "pm_runtime_get_sync failed: %d\n", ret);
> + goto err_get_sync;
> + }
> +
> + ret = dw_pcie_host_init(&pci->pp);
> + if (ret) {
> + dev_err(dev, "failed to initialize host: %d\n", ret);
> + goto err_host_init;
> + }
> +
> + return ret;
> +
> +err_host_init:
> + pm_runtime_put_sync(dev);
> +err_get_sync:
> + pm_runtime_disable(dev);
> + return ret;
> +}
Powered by blists - more mailing lists