[RFC/RFT PATCH 1/2] ARM: kernel: bios32: implement PCI device resources claiming

[RFC/RFT PATCH 1/2] ARM: kernel: bios32: implement PCI device resources claiming

[Lorenzo Pieralisi]

The current ARM pcibios layer prevents PCI devices resources enablement
if the PCI_PROBE_ONLY global flag is set, since on systems that require
immutable PCI BARs set-up (ie probe only) the arm PCI bios layer does
not assign resources, hence resources are not really checked (ie claimed)
and they do not get assigned a parent, which causes pci_enable_resources
to fail in turn.

If the kernel tries to enable the resources for a PCI device through
pci_enable_resources, it would fail on PCI_PROBE_ONLY systems since the
device resources are not claimed (ie they are not assigned) on those
systems and the resource parent is left uninitialized (ie they have a
dangling parent pointer).

This behaviour does not conform with the expected kernel behaviour.

When a PCI device and its resources are scanned, the detected PCI
resources should be claimed in order to validate their values read
from the BARs set-up and should be assigned a parent resource so that the
resource hierarchy is initialized.

Resource claiming must be carried out on PCI_PROBE_ONLY systems
too, since it is a method for validating resources and it has nothing
to do with PCI_PROBE_ONLY flag.

This patch removes the pcibios_enable_device call from the arm bios32
implementation (which means that arm falls back to the generic weak
version for its implementation, that correctly enables a device resources)
and replaces it with an arm pcibios_add_device implementation, that claims
resources for a device, so that the behaviour becomes compliant with the
expected PCI device kernel initialization flow.

This way, resources are claimed and enabled on PCI_PROBE_ONLY systems
too improving the BAR set-up validation and making the arm kernel PCI
behaviour closer to other architectures.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Russell King <linux@arm.linux.org.uk>
---
 arch/arm/kernel/bios32.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index fcbbbb1..aace9ca 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -604,15 +604,25 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
 }
 
 /**
- * pcibios_enable_device - Enable I/O and memory.
- * @dev: PCI device to be enabled
+ * pcibios_add_device - Initialize device before adding it to PCI bus
+ * @dev: PCI device to be added
  */
-int pcibios_enable_device(struct pci_dev *dev, int mask)
+int pcibios_add_device(struct pci_dev *dev)
 {
-	if (pci_has_flag(PCI_PROBE_ONLY))
-		return 0;
+	struct resource *res;
+	int i;
+	/*
+	 * Device resources are claimed to validate
+	 * them and initialize their hierarchy structure
+	 */
+	for (i = 0; i < PCI_NUM_RESOURCES; i++) {
+		res = &dev->resource[i];
+		if (res->parent || !res->flags)
+			continue;
+		pci_claim_resource(dev, i);
+	}
 
-	return pci_enable_resources(dev, mask);
+	return 0;
 }
 
 int pci_mmap_page_range(struct pci_dev *dev, struct vm_area_struct *vma,
-- 
2.2.1

[RFC/RFT PATCH 2/2] ARM64: kernel: pci: implement PCI device resources claiming

[Lorenzo Pieralisi]

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;
 }
 
-- 
2.2.1

Re: [RFC/RFT PATCH 2/2] ARM64: kernel: pci: implement PCI device resources claiming

[Suravee Suthikulanit]

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) &&
+           !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

Re: [RFC/RFT PATCH 2/2] ARM64: kernel: pci: implement PCI device resources claiming

[Lorenzo Pieralisi]

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

Re: [RFC/RFT PATCH 2/2] ARM64: kernel: pci: implement PCI device resources claiming

[Suravee Suthikulanit]

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

Re: [RFC/RFT PATCH 2/2] ARM64: kernel: pci: implement PCI device resources claiming

[Lorenzo Pieralisi]

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

Re: [RFC/RFT PATCH 2/2] ARM64: kernel: pci: implement PCI device resources claiming

[Bjorn Helgaas]

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

Re: [RFC/RFT PATCH 2/2] ARM64: kernel: pci: implement PCI device resources claiming

[Lorenzo Pieralisi]

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

Re: [RFC/RFT PATCH 2/2] ARM64: kernel: pci: implement PCI device resources claiming

[Bjorn Helgaas]

Hi Lorenzo,

On Thu, May 14, 2015 at 03:42:16PM +0100, 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>

Seems like Suravee proposed a tweak?  Can you post a v2 with that
incorporated if necessary?

Bjorn

> ---
>  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;
>  }
>  
> -- 
> 2.2.1
> 

Re: [RFC/RFT PATCH 2/2] ARM64: kernel: pci: implement PCI device resources claiming

[Lorenzo Pieralisi]

Hi Bjorn,

On Wed, May 20, 2015 at 12:25:31AM +0100, Bjorn Helgaas wrote:
> Hi Lorenzo,
> 
> On Thu, May 14, 2015 at 03:42:16PM +0100, 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>
> 
> Seems like Suravee proposed a tweak?  Can you post a v2 with that
> incorporated if necessary?

I can post a v2, but I am not happy about that tweak, I want to
understand why pci_read_bridge_bases has to be executed only on
PCI_PROBE_ONLY systems and why it can't be done in generic code in the
first place - ie pci_scan_child_bus(), I guess as I mentioned that in
some archs this can trigger regressions, if you have an opinion please
let me know, when we agree I will drop a v2 on the lists.

Thanks,
Lorenzo

> Bjorn
> 
> > ---
> >  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;
> >  }
> >  
> > -- 
> > 2.2.1
> > 
>