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);
