On Thu, Feb 13, 2014 at 11:47:46AM +0000, Arnd Bergmann wrote:
> On Thursday 13 February 2014 11:04:02 Will Deacon wrote:
> > On Wed, Feb 12, 2014 at 08:59:46PM +0000, Arnd Bergmann wrote:
> > > On Wednesday 12 February 2014 20:16:11 Will Deacon wrote:
> > > > +struct gen_pci_resource {
> > > > + struct list_head list;
> > > > + struct resource cpu_res;
> > > > + resource_size_t offset;
> > > > +};
> > >
> > > Your gen_pci_resource is actually identical to struct pci_host_bridge_window,
> > > which I guess is coincidence, but it would be nice to actually use
> > > the standard structure to make it easier to integrate with common
> > > infrastructure later.
> >
> > Ha, at least I managed to come up with the same struct! I'll move to the
> > generic version.
>
> Hmm, I fear I was speaking too quickly, the pci_host_bridge_window actually
> contains a pointer to the resource rather than the resource itself :(
I can allocate the resources dynamically as I parse them, not a problem at
all.
> There is probably a way to do this better, at least once we unify the
> probe() and setup() functions.
Yes, I fully expect this to be iterative.
> > > > + spin_lock(&pci->cfg.lock);
> > > > + if (pci->cfg.bus != busn) {
> > > > + resource_size_t offset;
> > > > +
> > > > + devm_iounmap(pci->dev, pci->cfg.base);
> > > > + offset = pci->cfg.cpu_phys + (busn << 20);
> > > > + pci->cfg.base = devm_ioremap(pci->dev, offset, SZ_1M);
> > > > + pci->cfg.bus = busn;
> > > > + }
> > >
> > > Nice idea, but unfortunately broken: we need config space access from
> > > atomic context, since there are drivers doing that.
> >
> > That aside, I just took a spin_lock so this needs fixing regardless. The
> > only solution I can think of then is to map all of the buses at setup time
> > (making bus_ranges pretty important) and hope that I don't chew through all
> > of vmalloc.
>
> It's possible we have to go further and only map the buses that are
> actually used, rather than the ones that are defined for the bus,
> but just mapping the entire range is a reasonable start I think.
Okey doke.
> > > > + hw = (struct hw_pci) {
> > > > + .nr_controllers = 1,
> > > > + .private_data = (void **)&pci,
> > > > + .setup = gen_pci_setup,
> > > > + .map_irq = of_irq_parse_and_map_pci,
> > > > + .ops = &gen_pci_ops,
> > > > + };
> > > > +
> > > > + pci_common_init_dev(dev, &hw);
> > > > + return 0;
> > > > +}
> > >
> > > A missing part here seems to be a way to propagate errors from
> > > the pci_common_init_dev or gen_pci_setup back to the caller.
> > >
> > > This seems to be a result of the arm pcibios implementation not
> > > being meant for loadable modules, but I suspect it can be fixed.
> > > The nr_controllers>1 logic gets a bit in the way there, but it's
> > > also a model that seems to be getting out of fashion:
> > > kirkwood/dove/orion5x/mv78xx0 use it at the moment, but are
> > > migrating over to the new pci-mvebu code that doesn't as they
> > > get rid of the non-DT probing. pci-rcar-gen2.c uses it, but it
> > > seems they already ran into problems with that and are changing
> > > it. That pretty much leaves iop13xx as the only user, but at
> > > that point we can probably move the loop into iop13xx specific
> > > code.
> >
> > Makes sense once there are no users of the field.
>
> With the patch I just suggested, we can simply keep
> pci_common_init_dev() for older (non-multiplatform) controllers
> and not change them at all but move on to something else for
> the interesting ones, i.e. those we want to share with arm64.
Yes, that looks sensible. An alternative is to create an hw_pci in
pci_host_bridge_register with nr_controllers = 1 then call
pci_common_init_dev. It would remove slightly more code, but obviously ties
the thing to arm.
Will
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
