Message-ID: <4f3rhkrlp3jypajh77rohqgpoujivpxq6g3o6vrt6u7u5j2atd@gd5o3vtlhapp>
Date: Tue, 6 Jan 2026 18:49:58 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: zhangsenchuan <zhangsenchuan@...incomputing.com>
Cc: Bjorn Helgaas <helgaas@...nel.org>, bhelgaas@...gle.com, 
	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, ningyu@...incomputing.com, 
	linmin@...incomputing.com, pinkesh.vaghela@...fochips.com, ouyanghui@...incomputing.com, 
	Niklas Cassel <cassel@...nel.org>
Subject: Re: [PATCH v9 2/2] PCI: eic7700: Add Eswin PCIe host controller
 driver

On Tue, Jan 06, 2026 at 08:43:11PM +0800, zhangsenchuan wrote:
> > Subject: Re: [PATCH v9 2/2] PCI: eic7700: Add Eswin PCIe host controller driver
> > 
> > [+cc Niklas, list vs array of ports]
> > 
> > On Mon, Dec 29, 2025 at 07:32:07PM +0800, zhangsenchuan@...incomputing.com wrote:
> > > 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.
> > 
> > > +config PCIE_EIC7700
> > > +	tristate "Eswin EIC7700 PCIe controller"
> > 
> > > +/* Vendor and device ID value */
> > > +#define PCI_VENDOR_ID_ESWIN		0x1fe1
> > > +#define PCI_DEVICE_ID_ESWIN		0x2030
> > 
> > Usually the device name is a little more than just the vendor.  What
> > if Eswin ever makes a second device?
> 
> Okey, thanks.
> Perhaps it's a problem. Maybe PCI_DEVICE_ID_EIC7700 is better?
> 
> > 
> > > +static int eic7700_pcie_parse_port(struct eic7700_pcie *pcie,
> > > +				   struct device_node *node)
> > > +{
> > > +	struct device *dev = pcie->pci.dev;
> > > +	struct eic7700_pcie_port *port;
> > > +
> > > +	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> > > +	if (!port)
> > > +		return -ENOMEM;
> > > +
> > > +	port->perst = of_reset_control_get_exclusive(node, "perst");
> > > +	if (IS_ERR(port->perst)) {
> > > +		dev_err(dev, "Failed to get PERST# reset\n");
> > > +		return PTR_ERR(port->perst);
> > > +	}
> > > +
> > > +	/*
> > > +	 * TODO: Since the Root Port node is separated out by pcie devicetree,
> > > +	 * the DWC core initialization code can't parse the num-lanes attribute
> > > +	 * in the Root Port. Before entering the DWC core initialization code,
> > > +	 * the platform driver code parses the Root Port node. The EIC7700 only
> > > +	 * supports one Root Port node, and the num-lanes attribute is suitable
> > > +	 * for the case of one Root Port.
> > > +	 */
> > > +	if (!of_property_read_u32(node, "num-lanes", &port->num_lanes))
> > > +		pcie->pci.num_lanes = port->num_lanes;
> > > +
> > > +	INIT_LIST_HEAD(&port->list);
> > > +	list_add_tail(&port->list, &pcie->ports);
> > 
> > Niklas raised an interesting question about whether a list or an array
> > is the best data structure for the set of Root Ports:
> > 
> >   https://lore.kernel.org/r/aVvkmkd5mWPmxeiS@ryzen
> > 
> > Might have to iterate over the child nodes twice (once to count, again
> > for eic7700_pcie_parse_port()), but otherwise the array is probably
> > simpler code.
> 
> After reading patch's comments, lists and arrays seem to be good choices,
> I don't have any particularly good ideas for the time being. Anyway, this
> is a very good patch that supports multiple Root Ports resolutions.
> 

I still prefer using lists for the reasons mentioned in that thread.

> > 
> > > +	return 0;
> > > +}
> > > +
> > > +static int eic7700_pcie_parse_ports(struct eic7700_pcie *pcie)
> > > +{
> > > +	struct eic7700_pcie_port *port, *tmp;
> > > +	struct device *dev = pcie->pci.dev;
> > > +	int ret;
> > > +
> > > +	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
> > > +		ret = eic7700_pcie_parse_port(pcie, of_port);
> > > +		if (ret)
> > > +			goto err_port;
> > > +	}
> > > +
> > > +	return 0;
> > > +
> > > +err_port:
> > > +	list_for_each_entry_safe(port, tmp, &pcie->ports, list)
> > > +		list_del(&port->list);
> > 
> > Is some kind of reset_control_put() needed to match the
> > of_reset_control_get_exclusive() above?
> 
> I only considered that there is currently only one Root Port. Maybe 
> there will be multiple Root Ports in the future.
> 
> Perhaps this is the best:
> list_for_each_entry_safe(port, tmp, &pcie->ports, list){
>         if (!IS_ERR_OR_NULL(port->perst))

You don't need this check since reset_control_put() does it for you.

>             reset_control_put(port->perst);
>         list_del(&port->list);
> }
> 
> > 
> > > +static struct platform_driver eic7700_pcie_driver = {
> > > +	.probe = eic7700_pcie_probe,
> > 
> > This driver is tristate but has no .remove() callback.  Seems like it
> > should have one?
> 
> In v2 patch, I referred to Mani's comments and removed the .remove()
> callback, as follows:
> "Since this controller implements irqchip using the DWC core driver,
> it is not safe to remove it during runtime."
> https://lore.kernel.org/linux-pci/jghozurjqyhmtunivotitgs67h6xo4sb46qcycnbbwyvjcm4ek@vgq75olazmoi/
> 
> In addition, remove .remove() callback, because this driver has been 
> modified to builtin_platform_driver and does not support HotPlug, 
> therefore, the .remove() callback is not needed. Do you have any
> better suggestions?
> 

Yes, builtin_platform_driver() wouldn't allow the users to remove the module. So
remove() callback will become useless. The reason why this driver is tristate is
that it could be loaded from rootfs and not always statically built to the
kernel image.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Powered by blists - more mailing lists