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

[zhangsenchuan <zhangsenchuan@eswincomputing.com>]

> > From: Senchuan Zhang <zhangsenchuan@eswincomputing.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@eswincomputing.com>
> > Signed-off-by: Yanghui Ou <ouyanghui@eswincomputing.com>
> > Signed-off-by: Senchuan Zhang <zhangsenchuan@eswincomputing.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?

Thanks for your suggestion.
There may be cases where memory is allocated but not released. I will fix it
in the next patch.

> 
> 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.

Okey, thanks.

Kind regards,
Senchuan Zhang