On Wed, May 20, 2015 at 3:56 AM, Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx> wrote: > On Fri, May 15, 2015 at 03:09:03AM +0100, Suravee Suthikulanit wrote: >> On 5/14/2015 9:42 AM, Lorenzo Pieralisi wrote: >> > When a device is scanned and added to the PCI bus, its resources >> > should be claimed to validate the BARs configuration and to assign >> > them a parent resource so that the resource hierarchy can be sanity >> > checked. >> > >> > This patch adds code that carries out PCI device resources claiming to >> > the ARM64 pcibios_add_device implementation so that device resources >> > are claimed by the core PCI layer upon PCI device initialization on >> > ARM64 systems. >> > >> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx> >> > Cc: Arnd Bergmann <arnd@xxxxxxxx> >> > Cc: Will Deacon <will.deacon@xxxxxxx> >> > Cc: Liviu Dudau <liviu.dudau@xxxxxxx> >> > Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx> >> > --- >> > arch/arm64/kernel/pci.c | 10 ++++++++++ >> > 1 file changed, 10 insertions(+) >> > >> > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c >> > index 4095379..c0a88ca 100644 >> > --- a/arch/arm64/kernel/pci.c >> > +++ b/arch/arm64/kernel/pci.c >> > @@ -43,8 +43,18 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res, >> > */ >> > int pcibios_add_device(struct pci_dev *dev) >> > { >> > + struct resource *res; >> > + int i; >> > + >> > dev->irq = of_irq_parse_and_map_pci(dev, 0, 0); >> > >> > + for (i = 0; i < PCI_NUM_RESOURCES; i++) { >> > + res = &dev->resource[i]; >> > + if (res->parent || !res->flags) >> > + continue; >> > + pci_claim_resource(dev, i); >> > + } >> > + >> > return 0; >> > } >> > >> > >> >> Lorenzo/Bjorn, >> >> I have tested this patch on top of Jayachandran's V2 patch series >> (http://www.spinics.net/lists/linux-pci/msg40811.html) on AMD Seattle >> (w/ PROBE_ONLY and non-PROBE_ONLY mode), and your changes here works >> with additional changes below. >> >> It seems that when booting w/ PROBE_ONLY case, we need to call >> pci_read_bridge_bases() at some point before claiming the resources of >> devices underneath the bridge. This is needed to determine the bridge >> bases (i.e. bridge io, mmio and mmio_pref bases), and update bridge >> resources accordingly. >> >> ---- BEGIN PATCH ----- >> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c >> index c0a88ca..57be6aa 100644 >> --- a/arch/arm64/kernel/pci.c >> +++ b/arch/arm64/kernel/pci.c >> @@ -48,6 +48,11 @@ int pcibios_add_device(struct pci_dev *dev) >> >> dev->irq = of_irq_parse_and_map_pci(dev, 0, 0); >> >> + if (pci_has_flag(PCI_PROBE_ONLY) && > > Does it really matter if PCI_PROBE_ONLY is set ? > >> + !pci_is_root_bus(dev->bus) && > > This check is useless, since pci_read_bridge_bases checks that already. > >> + !pci_bridge_bases_is_read(dev->bus)) >> + pci_read_bridge_bases(dev->bus); >> + > > Ok. Most of the archs do that in pcibios_fixup_bus, I would like to > understand: > > 1) Should we do it on PCI_PROBE_ONLY only > 2) Can we move this to generic code - ie pci_scan_child_bus (I guess answer > is no, because there are corner cases I am not aware of) In my opinion, we should call pci_read_bridge_bases() in all cases, regardless of PCI_PROBE_ONLY. pci_read_bridge_bases() doesn't *change* anything in the hardware; it only reads what's there. (It should attempt writes to learn whether I/O and prefetchable memory apertures are implemented, but those should be done as in pci_bridge_check_ranges(), where the original value is restored.) I also think this should be done in generic code, since there's nothing architecture-specific about bridge operation. I've been hoping to get rid of pcibios_fixup_bus() completely for years, and doing pci_read_bridge_bases() in generic code would be a big step. No doubt there are corner cases we'll trip over, but I'm not aware of them yet. > If I add it to arm64 (in pcibios_fixup_bus) I have to add it to arm too, more > arch specific code that has nothing arch specific in it so I am not really > keen on that. > > Comments appreciated. > > Lorenzo > >> for (i = 0; i < PCI_NUM_RESOURCES; i++) { >> res = &dev->resource[i]; >> if (res->parent || !res->flags) >> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >> index 6675a7a..6cab8be 100644 >> --- a/drivers/pci/probe.c >> +++ b/drivers/pci/probe.c >> @@ -447,6 +447,13 @@ static void pci_read_bridge_mmio_pref(struct >> pci_bus *child) >> } >> } >> >> +bool pci_bridge_bases_is_read(struct pci_bus *bus) >> +{ >> + return (bus->resource[0]->start || >> + bus->resource[1]->start || >> + bus->resource[2]->start); >> +} >> + >> void pci_read_bridge_bases(struct pci_bus *child) >> { >> struct pci_dev *dev = child->self; >> diff --git a/include/linux/pci.h b/include/linux/pci.h >> index 353db8d..11c674d 100644 >> --- a/include/linux/pci.h >> +++ b/include/linux/pci.h >> @@ -798,6 +798,7 @@ void pci_device_add(struct pci_dev *dev, struct >> pci_bus *bus); >> unsigned int pci_scan_child_bus(struct pci_bus *bus); >> void pci_bus_add_device(struct pci_dev *dev); >> void pci_read_bridge_bases(struct pci_bus *child); >> +bool pci_bridge_bases_is_read(struct pci_bus *bus); >> struct resource *pci_find_parent_resource(const struct pci_dev *dev, >> struct resource *res); >> u8 pci_swizzle_interrupt_pin(const struct pci_dev *dev, u8 pin); >> ---- END PATCH ----- >> >> I'm not sure if this is the best place to be reading the bridge bases. >> I guess we should be able to do this when adding bridge devices. >> >> Thanks, >> >> Suravee >> >> -- 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