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

[zhangsenchuan <zhangsenchuan@eswincomputing.com>]

> 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@eswincomputing.com wrote:
> > 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.
> 
> > +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.

> 
> > +	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))
            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?

Kind regards,
Senchuan Zhang

> 
> > +	.driver = {
> > +		.name = "eic7700-pcie",
> > +		.of_match_table = eic7700_pcie_of_match,
> > +		.suppress_bind_attrs = true,
> > +		.pm = &eic7700_pcie_pm,
> > +		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
> > +	},
> > +};
> > +builtin_platform_driver(eic7700_pcie_driver);