linux-kernel - Re: [PATCH v9 2/2] PCI: eic7700: Add Eswin PCIe host controller driver

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <37ef98c2-b13f-4292-8db7-df90237c7ce1@wanadoo.fr>
Date: Mon, 29 Dec 2025 13:28:59 +0100
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: zhangsenchuan@...incomputing.com, bhelgaas@...gle.com, mani@...nel.org,
 krzk+dt@...nel.org, conor+dt@...nel.org, lpieralisi@...nel.org,
 kwilczynski@...nel.org, robh@...nel.org, p.zabel@...gutronix.de,
 jingoohan1@...il.com, gustavo.pimentel@...opsys.com,
 linux-pci@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, christian.bruel@...s.st.com,
 mayank.rana@....qualcomm.com, shradha.t@...sung.com,
 krishna.chundru@....qualcomm.com, thippeswamy.havalige@....com,
 inochiama@...il.com, Frank.li@....com
Cc: ningyu@...incomputing.com, linmin@...incomputing.com,
 pinkesh.vaghela@...fochips.com, ouyanghui@...incomputing.com
Subject: Re: [PATCH v9 2/2] PCI: eic7700: Add Eswin PCIe host controller
 driver

Le 29/12/2025 à 12:32, zhangsenchuan@...incomputing.com a écrit :
> From: Senchuan Zhang <zhangsenchuan@...incomputing.com>
> 
> Add driver for the Eswin EIC7700 PCIe host controller, which is based on
> the DesignWare PCIe core, IP revision 5.96a. The PCIe Gen.3 controller
> supports a data rate of 8 GT/s and 4 channels, support INTx and MSI
> interrupts.
> 
> Signed-off-by: Yu Ning <ningyu@...incomputing.com>
> Signed-off-by: Yanghui Ou <ouyanghui@...incomputing.com>
> Signed-off-by: Senchuan Zhang <zhangsenchuan@...incomputing.com>
> ---

Hi,

> +static int eic7700_pcie_host_init(struct dw_pcie_rp *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct eic7700_pcie *pcie = to_eic7700_pcie(pci);
> +	struct eic7700_pcie_port *port;
> +	u32 val;
> +	int ret;
> +
> +	pcie->num_clks = devm_clk_bulk_get_all_enabled(pci->dev, &pcie->clks);

Is this the correct place to call this function?

eic7700_pcie_host_init() is called from eic7700_pcie_resume_noirq() and 
calling a devm function from a resume function is really unusual and is 
likely to leak memory.

> +	if (pcie->num_clks < 0)
> +		return dev_err_probe(pci->dev, pcie->num_clks,
> +				     "Failed to get pcie clocks\n");
> +
> +	/*
> +	 * The PWR and DBI reset signals are respectively used to reset the
> +	 * PCIe controller and the DBI register.
> +	 *
> +	 * The PERST# signal is a reset signal that simultaneously controls the
> +	 * PCIe controller, PHY, and Endpoint. Before configuring the PHY, the
> +	 * PERST# signal must first be deasserted.
> +	 *
> +	 * The external reference clock is supplied simultaneously to the PHY
> +	 * and EP. When the PHY is configurable, the entire chip already has
> +	 * stable power and reference clock. The PHY will be ready within 20ms
> +	 * after writing app_hold_phy_rst register bit of ELBI register space.
> +	 */
> +	ret = reset_control_bulk_deassert(EIC7700_NUM_RSTS, pcie->resets);
> +	if (ret) {
> +		dev_err(pcie->pci.dev, "Failed to deassert resets\n");
> +		return ret;
> +	}
> +
> +	/* Configure Root Port type */
> +	val = readl_relaxed(pci->elbi_base + PCIEELBI_CTRL0_OFFSET);
> +	val &= ~PCIEELBI_CTRL0_DEV_TYPE;
> +	val |= FIELD_PREP(PCIEELBI_CTRL0_DEV_TYPE, PCI_EXP_TYPE_ROOT_PORT);
> +	writel_relaxed(val, pci->elbi_base + PCIEELBI_CTRL0_OFFSET);
> +
> +	list_for_each_entry(port, &pcie->ports, list) {
> +		ret = eic7700_pcie_perst_reset(port, pcie);
> +		if (ret)
> +			goto err_perst;
> +	}
> +
> +	/* Configure app_hold_phy_rst */
> +	val = readl_relaxed(pci->elbi_base + PCIEELBI_CTRL0_OFFSET);
> +	val &= ~PCIEELBI_APP_HOLD_PHY_RST;
> +	writel_relaxed(val, pci->elbi_base + PCIEELBI_CTRL0_OFFSET);
> +
> +	/* The maximum waiting time for the clock switch lock is 20ms */
> +	ret = readl_poll_timeout(pci->elbi_base + PCIEELBI_STATUS0_OFFSET, val,
> +				 !(val & PCIEELBI_PM_SEL_AUX_CLK), 1000,
> +				 20000);
> +	if (ret) {
> +		dev_err(pci->dev, "Timeout waiting for PM_SEL_AUX_CLK ready\n");
> +		goto err_phy_init;
> +	}
> +
> +	/*
> +	 * Configure ESWIN VID:DID for Root Port as the default values are
> +	 * invalid.
> +	 */
> +	dw_pcie_dbi_ro_wr_en(pci);
> +	dw_pcie_writew_dbi(pci, PCI_VENDOR_ID, PCI_VENDOR_ID_ESWIN);
> +	dw_pcie_writew_dbi(pci, PCI_DEVICE_ID, PCI_DEVICE_ID_ESWIN);
> +	dw_pcie_dbi_ro_wr_dis(pci);
> +
> +	return 0;
> +
> +err_phy_init:
> +	list_for_each_entry(port, &pcie->ports, list)
> +		reset_control_assert(port->perst);
> +err_perst:
> +	reset_control_bulk_assert(EIC7700_NUM_RSTS, pcie->resets);
> +
> +	return ret;
> +}

...

> +DEFINE_NOIRQ_DEV_PM_OPS(eic7700_pcie_pm, eic7700_pcie_suspend_noirq,
> +			eic7700_pcie_resume_noirq);
> +
> +static const struct of_device_id eic7700_pcie_of_match[] = {
> +	{ .compatible = "eswin,eic7700-pcie" },
> +	{},

Nitpick: No need for trailing comma after a terminator.

> +};

CJ