[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 at 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 at arm.com>
>> > Cc: Arnd Bergmann <arnd at arndb.de>
>> > Cc: Will Deacon <will.deacon at arm.com>
>> > Cc: Liviu Dudau <liviu.dudau at arm.com>
>> > Cc: Bjorn Helgaas <bhelgaas at 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
>>
>>