Re: [PATCH 2/2] PCI: dwc: Add multi-port controller support

[Niklas Cassel <cassel@kernel.org>]

On Tue, Jan 06, 2026 at 10:49:19AM +0530, Manivannan Sadhasivam wrote:
> On Mon, Jan 05, 2026 at 05:19:38PM +0100, Niklas Cassel wrote:
> > On Mon, Jan 05, 2026 at 05:57:55PM +0530, Sumit Kumar wrote:
> > > The current DesignWare PCIe RC implementation supports only the controller
> > > (Host Bridge) node for specifying the Root Port properties in an assumption
> > > that the underlying platform only supports a single root Port per
> > > controller instance. This limits support for multi-port controllers where
> > > different ports may have different lane configurations and speed limits.
> > > 
> > > Introduce a separate dw_pcie_port structure to enable multi-port support.
> > > Each Root Port can have independent lane count, speed limit through pcie@N
> > > child nodes in device tree. Add dw_pcie_parse_root_ports()
> > > API to parse these child nodes.
> > > 
> > > Equalization presets and link width detection currently use common DBI
> > > space for all the root ports. Per-port DBI space assignment for these
> > > features will be added in future.
> > > 
> > > Signed-off-by: Sumit Kumar <sumit.kumar@oss.qualcomm.com>
> > 
> > Hello Sumit,
> > 
> > Is there a reason why you represent this as a list of ports rather than a
> > simple array?
> > 
> > The number of ports is known by parsing the device tree, so it should be
> > static, no?
> > 
> > At least to me, this seem similar to e.g. how a gpio_device has multiple
> > gpio_descriptors "struct gpio_desc *descs":
> > https://github.com/torvalds/linux/blob/master/drivers/gpio/gpiolib.h#L68C1-L68C26
> > 
> > A list is usually used for something that is dynamic.
> > I don't think that the number of ports to a PCIe controller will be dynamic.
> > 
> > I can see that struct qcom_pcie in pcie-qcom.c has struct list_head ports,
> > but that does not necessarily mean that we need to have a list of ports in
> > pcie-designware-host.c. (pcie-qcom could also be modified to have an array
> > of ports if there is a desire for similar design pattern.)
> > 
> 
> Only reason why I went with lists in pcie-qcom is flexibility. There are useful
> helpers available for traversing the lists and they seem much more elegant to
> use rather than manually traversing the array in C.
> 
> But to be frank, I don't really care which one is used as there is going to be
> only a handful of ports at max anyway and there is not much overhead.

Personally, when I see lists, I automatically think of something that is
dynamic, so using lists for something static just looks a little bit out of
place IMHO.

Technically, the difference is speed. If we want a specific element, we
will need to traverse the list. With an array, we can access the element
directly. However, looking at the current patch, it seems like it never
looks for a specific port, it always does an operation for all ports.
So from a speed perspective, it doesn't matter, at least not for now.


One advantage I can see, instead of doing:

+	struct dw_pcie_port *port = list_first_entry(&pci->pp.ports,
+						struct dw_pcie_port, list);
+	return dw_pcie_wait_for_link(pci, port);

for drivers with only one port (most drivers), we could just instead do:

+	return dw_pcie_wait_for_link(pci, pci->pp.port);

To simply get the first element in the array. No need to sprinkle
list_first_entry() everywhere in all the drivers if they just have one port.


For iterating, to avoid manually traversing the array, we could do like
libata and create a simple macro, e.g. ata_qc_for_each():
https://github.com/torvalds/linux/blob/v6.19-rc4/drivers/ata/libata-eh.c#L851-L854
https://github.com/torvalds/linux/blob/v6.19-rc4/include/linux/libata.h#L1657-L1659

And at least personally, I think my brain will parse dw_pcie_port_for_each() { }
faster than it parses list_for_each_entry(port, &pcie->ports, list) { },
since it is more unique, but perhaps I am the weird one here :)


Kind regards,
Niklas