| Message ID | 5555553F.9070608@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Suravee, 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@arm.com> > > Cc: Arnd Bergmann <arnd@arndb.de> > > Cc: Will Deacon <will.deacon@arm.com> > > Cc: Liviu Dudau <liviu.dudau@arm.com> > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > --- > > 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. Thanks for testing, I will give it a go on Seattle, if you can drop the log you get on PROBE_ONLY (without your patch below) that would be great so that I can have a look at the issue. Thanks, Lorenzo > ---- 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) && > + !pci_is_root_bus(dev->bus) && > + !pci_bridge_bases_is_read(dev->bus)) > + pci_read_bridge_bases(dev->bus); > + > 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 > >
On 5/18/2015 12:38 PM, Lorenzo Pieralisi wrote: >> 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. > Thanks for testing, I will give it a go on Seattle, if you can drop > the log you get on PROBE_ONLY (without your patch below) that would be > great so that I can have a look at the issue. > > Thanks, > Lorenzo > Lorenzo, Here is the log w/ PROBE_ONLY and w/o the changes I proposed: # dmesg ..... PCI host bridge /smb/pcie@f0000000 ranges: IO 0xefff0000..0xefffffff -> 0x00000000 MEM 0x40000000..0xbfffffff -> 0x40000000 MEM 0x100000000..0x7fffffffff -> 0x100000000 pci-host-generic f0000000.pcie: PCI host bridge to bus 0000:00 pci_bus 0000:00: root bus resource [bus 00-7f] pci_bus 0000:00: root bus resource [io 0x0000-0xffff] pci_bus 0000:00: root bus resource [mem 0x40000000-0xbfffffff] pci_bus 0000:00: root bus resource [mem 0x100000000-0x7fffffffff] pci 0000:00:00.0: of_irq_parse_pci() failed with rc=-19 pci 0000:00:02.0: of_irq_parse_pci() failed with rc=-19 pci 0000:01:00.0: can't claim BAR 0 [mem 0xbfe80000-0xbfefffff 64bit pref]: no compatible bridge window pci 0000:01:00.0: can't claim BAR 2 [io 0x0020-0x003f]: no compatible bridge window pci 0000:01:00.0: can't claim BAR 4 [mem 0xbff04000-0xbff07fff 64bit pref]: no compatible bridge window pci 0000:01:00.1: can't claim BAR 0 [mem 0xbfe00000-0xbfe7ffff 64bit pref]: no compatible bridge window pci 0000:01:00.1: can't claim BAR 2 [io 0x0000-0x001f]: no compatible bridge window pci 0000:01:00.1: can't claim BAR 4 [mem 0xbff00000-0xbff03fff 64bit pref]: no compatible bridge window ..... # dmesg | grep ixgbe [ 7.189232] ixgbe: Intel(R) 10 Gigabit PCI Express Network Driver - version 4.0.1-k [ 7.195580] ixgbe: Copyright (c) 1999-2014 Intel Corporation. [ 7.195638] ixgbe 0000:01:00.0: can't enable device: BAR 0 [mem size 0x00080000 64bit pref] not assigned [ 7.195645] ixgbe: probe of 0000:01:00.0 failed with error -22 [ 7.195660] ixgbe 0000:01:00.1: can't enable device: BAR 0 [mem size 0x00080000 64bit pref] not assigned [ 7.195664] ixgbe: probe of 0000:01:00.1 failed with error -22 Also, here is the bridge configuration. Please note that the change I added tries to setup the bridge resource with information in _Prefetchable memory behind_ region. # lspci -vvv -s 0:2.1 00:02.1 PCI bridge: Advanced Micro Devices, Inc. [AMD] Device 1a02 (prog-if 00 [Normal decode]) Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx- Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx- Latency: 0 Bus: primary=00, secondary=01, subordinate=01, sec-latency=0 I/O behind bridge: 00fff000-00000fff Memory behind bridge: fff00000-000fffff Prefetchable memory behind bridge: 00000000bfe00000-00000000bfffffff Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort+ <SERR- <PERR- BridgeCtl: Parity- SERR- NoISA- VGA- MAbort- >Reset- FastB2B- PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn- Capabilities: [50] Power Management version 3 Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+) Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME- Capabilities: [58] Express (v2) Root Port (Slot+), MSI 00 DevCap: MaxPayload 512 bytes, PhantFunc 0 ExtTag+ RBE+ DevCtl: Report errors: Correctable- Non-Fatal- Fatal- Unsupported- RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop+ MaxPayload 512 bytes, MaxReadReq 512 bytes DevSta: CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr- TransPend- LnkCap: Port #0, Speed 5GT/s, Width x8, ASPM L0s L1, Exit Latency L0s <512ns, L1 <64us ClockPM- Surprise- LLActRep+ BwNot- LnkCtl: ASPM Disabled; RCB 64 bytes Disabled- CommClk+ ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt- LnkSta: Speed 5GT/s, Width x4, TrErr- Train- SlotClk+ DLActive+ BWMgmt- ABWMgmt- SltCap: AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug- Surprise- Slot #0, PowerLimit 0.000W; Interlock- NoCompl+ SltCtl: Enable: AttnBtn- PwrFlt- MRL- PresDet- CmdCplt- HPIrq- LinkChg- Control: AttnInd Unknown, PwrInd Unknown, Power- Interlock- SltSta: Status: AttnBtn- PowerFlt- MRL- CmdCplt- PresDet+ Interlock- Changed: MRL- PresDet- LinkState- RootCtl: ErrCorrectable- ErrNon-Fatal- ErrFatal- PMEIntEna- CRSVisible+ RootCap: CRSVisible- RootSta: PME ReqID 0000, PMEStatus- PMEPending- DevCap2: Completion Timeout: Range ABCD, TimeoutDis+, LTR-, OBFF Not Supported ARIFwd+ DevCtl2: Completion Timeout: 50us to 50ms, TimeoutDis-, LTR-, OBFF Disabled ARIFwd+ LnkCtl2: Target Link Speed: 5GT/s, EnterCompliance- SpeedDis- Transmit Margin: Normal Operating Range, EnterModifiedCompliance- ComplianceSOS- Compliance De-emphasis: -6dB LnkSta2: Current De-emphasis Level: -6dB, EqualizationComplete-, EqualizationPhase1- EqualizationPhase2-, EqualizationPhase3-, LinkEqualizationRequest- Capabilities: [a0] MSI: Enable- Count=1/1 Maskable- 64bit+ Address: 0000000000000000 Data: 0000 Capabilities: [c0] Subsystem: Advanced Micro Devices, Inc. [AMD] Device 1234 Capabilities: [c8] HyperTransport: MSI Mapping Enable+ Fixed+ Capabilities: [100 v1] Vendor Specific Information: ID=0001 Rev=1 Len=010 <?> Capabilities: [270 v1] #19 Thanks, Suravee
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@arm.com> > > Cc: Arnd Bergmann <arnd@arndb.de> > > Cc: Will Deacon <will.deacon@arm.com> > > Cc: Liviu Dudau <liviu.dudau@arm.com> > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > --- > > 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) 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 > >
On Wed, May 20, 2015 at 3:56 AM, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> 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@arm.com> >> > Cc: Arnd Bergmann <arnd@arndb.de> >> > Cc: Will Deacon <will.deacon@arm.com> >> > Cc: Liviu Dudau <liviu.dudau@arm.com> >> > Cc: Bjorn Helgaas <bhelgaas@google.com> >> > --- >> > 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 >> >>
On Wed, May 20, 2015 at 02:02:52PM +0100, Bjorn Helgaas wrote: [...] > >> @@ -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. I put together a patch to move it to pci_scan_child_bus, and to remove it from almost all archs pcibios_fixup_bus implementations, let's see how it goes. > No doubt there are corner cases we'll trip over, but I'm not aware of them yet. > Let's find out :), posting tomorrow. Thanks, Lorenzo
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) && + !pci_is_root_bus(dev->bus) && + !pci_bridge_bases_is_read(dev->bus)) + pci_read_bridge_bases(dev->bus); + 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);