Re: [PATCH 3/5] PCI: cadence: Add host driver for Cadence PCIe controller

[Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>]

On Tue, Nov 28, 2017 at 02:41:14PM -0600, Bjorn Helgaas wrote:

[...]

> > +static int cdns_pcie_parse_request_of_pci_ranges(struct device *dev,
> > +						 struct list_head *resources,
> > +						 struct resource **bus_range)
> > +{
> > +	int err, res_valid = 0;
> > +	struct device_node *np = dev->of_node;
> > +	resource_size_t iobase;
> > +	struct resource_entry *win, *tmp;
> > +
> > +	err = of_pci_get_host_bridge_resources(np, 0, 0xff, resources, &iobase);
> > +	if (err)
> > +		return err;
> > +
> > +	err = devm_request_pci_bus_resources(dev, resources);
> > +	if (err)
> > +		return err;
> > +
> > +	resource_list_for_each_entry_safe(win, tmp, resources) {
> > +		struct resource *res = win->res;
> > +
> > +		switch (resource_type(res)) {
> > +		case IORESOURCE_IO:
> > +			err = pci_remap_iospace(res, iobase);
> > +			if (err) {
> > +				dev_warn(dev, "error %d: failed to map resource %pR\n",
> > +					 err, res);
> > +				resource_list_destroy_entry(win);
> > +			}
> > +			break;
> > +		case IORESOURCE_MEM:
> > +			res_valid |= !(res->flags & IORESOURCE_PREFETCH);
> > +			break;
> > +		case IORESOURCE_BUS:
> > +			*bus_range = res;
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (res_valid)
> > +		return 0;
> > +
> > +	dev_err(dev, "non-prefetchable memory resource required\n");
> > +	return -EINVAL;
> > +}
> 
> The code above is starting to look awfully familiar.  I wonder if it's
> time to think about some PCI-internal interface that can encapsulate
> this.  In this case, there's really nothing Cadence-specific here.
> There are other callers where there *is* vendor-specific code, but
> possibly that could be handled by returning pointers to bus number,
> I/O port, and MMIO resources so the caller could do the
> vendor-specific stuff?

Yes and that's not the only one, pattern below is duplicated
(with some minor differences across host bridges that I think
can be managed through function parameters), it is probably worth
moving them both into a core code helper.

list_splice_init(&resources, &bridge->windows);
bridge->dev.parent = dev;
bridge->busnr = bus;
bridge->ops = &pci_ops;
bridge->map_irq = of_irq_parse_and_map_pci;
bridge->swizzle_irq = pci_common_swizzle;

ret = pci_scan_root_bus_bridge(bridge);
if (ret < 0) {
	dev_err(dev, "Scanning root bridge failed");
	goto err_init;
}

bus = bridge->bus;
pci_bus_size_bridges(bus);
pci_bus_assign_resources(bus);

list_for_each_entry(child, &bus->children, node)
	pcie_bus_configure_settings(child);

pci_bus_add_devices(bus);