Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Jon Masters]

On 12/06/2016 03:18 PM, Bjorn Helgaas wrote:

> I just merged pci/ecam into my "next" branch, so as long as tomorrow's
> linux-next boots out of the box, we should be set.  I made the following
> changes compared to v11:
> 
>   - dropped the x86 change to use acpi_resource_consumer()
>   - added parens around macro args used in arithmetic expressions
>   - renamed "seg" to "node" in THUNDER_PEM_RES and THUNDER_PEM_QUIRK
>   - incorporated (u64) cast and dropped "UL" postfix for THUNDER_PEM_QUIRK
> 
> Let me know if you see any issues.

Just following up. Please find attached a boot log from an HPE ProLiant m400
Moonshot X-Gene based cartridge running next-20161213 with pci/ecam branch.

Here is the /proc/iomem output as well:

# cat /proc/iomem
1052-10523fff : APMC0D18:00
  1052-10523fff : APMC0D18:00
10524000-10527fff : APMC0D17:00
1054-1054a0ff : APMC0D01:00
  10546000-10546fff : APMC0D50:00
  1054a000-1054a00f : APMC0D12:03
1054a000-1054a00f : APMC0D12:02
  1054a000-1054a00f : APMC0D12:01
1054a000-1054a00f : APMC0D12:00
1700-17000fff : APMC0D01:00
17001000-17001fff : APMC0D01:00
  17001000-170013ff : APMC0D15:00
17001000-170013ff : APMC0D15:00
1701c000-1701cfff : APMC0D14:00
1a80-1a800fff : APMC0D0D:00
  1a80-1a800fff : APMC0D0D:00
1c000200-1c0002ff : APMC0D06:00
1c021000-1c0210ff : APMC0D08:00
  1c021000-1c02101f : serial
1c024000-1c024fff : APMC0D07:00
1f23-1f230fff : APMC0D0D:00
  1f23-1f230fff : APMC0D0D:00
1f23d000-1f23dfff : APMC0D0D:00
  1f23d000-1f23dfff : APMC0D0D:00
1f23e000-1f23efff : APMC0D0D:00
  1f23e000-1f23efff : APMC0D0D:00
1f2a-1f31 : APMC0D06:00
1f50-1f50 : PNP0A08:00
7880-78800fff : APMC0D13:00
  7880-78800fff : APMC0D12:03
7880-78800fff : APMC0D12:02
  7880-78800fff : APMC0D12:01
7880-78800fff : APMC0D12:00
  7880-78800fff : APMC0D11:00
  7880-78800fff : APMC0D10:03
  7880-78800fff : APMC0D10:02
  7880-78800fff : APMC0D10:01
  7880-78800fff : APMC0D10:00
7900-798f : APMC0D0E:00
7c00-7c1f : APMC0D12:00
7c20-7c3f : APMC0D12:01
7c40-7c5f : APMC0D12:02
7c60-7c7f : APMC0D12:03
7e00-7e000fff : APMC0D13:00
7e20-7e200fff : APMC0D10:03
  7e20-7e200fff : APMC0D10:02
7e20-7e200fff : APMC0D10:01
  7e20-7e200fff : APMC0D10:00
7e60-7e600fff : APMC0D11:00
7e70-7e700fff : APMC0D10:03
  7e70-7e700fff : APMC0D10:02
7e70-7e700fff : APMC0D10:01
  7e70-7e700fff : APMC0D10:00
7e72-7e720fff : APMC0D10:03
  7e72-7e720fff : APMC0D10:02
7e72-7e720fff : APMC0D10:01
  7e72-7e720fff : APMC0D10:00
7e80-7e800fff : APMC0D10:00
7e84-7e840fff : APMC0D10:01
7e88-7e880fff : APMC0D10:02
7e8c-7e8c0fff : APMC0D10:03
7e93-7e930fff : APMC0D13:00
40-4001ff : System RAM
  48-4000c9 : Kernel code
  4000e2-400171 : Kernel data
40023a-4ff733 : System RAM
4ff734-4ff77c : reserved
4ff77d-4ff79c : System RAM
4ff79d-4ff7e7 : reserved
4ff7e8-4ff7e8 : System RAM
4ff7e9-4ff7ef : reserved
4ff7f1-4ff800 : reserved
4ff801-4f : System RAM
a02000-a03fff : PCI Bus :00
  a02000-a0201f : PCI Bus :01
a02000-a0200f : :01:00.0
  a02000-a0200f : mlx4_core
a02010-a0201f : :01:00.0
a06000-a07fff : PCI Bus :00
a0d000-a0dfff : PCI ECAM
a11000-a14fff : PCI Bus :00
  a11000-a121ff : PCI Bus :01
a11000-a111ff : :01:00.0
  a11000-a111ff : mlx4_core
a11200-a121ff : :01:00.0

Thanks again, Bjorn. Looking forward to seeing this upstream.

Tested-by: Jon Masters 

-- 
Computer Architect | Sent from my Fedora powered laptop

[0.00] Booting Linux on physical CPU 0x0
[0.00] Linux version 4.9.0-next-20161213.next20161213_jcm1 
(r...@hp-moonshot-02-c08.khw.lab.eng.bos.redhat.com) (gcc version 4.8.5 
20150623 (Red Hat 4.8.5-11) (GCC) ) #1 SMP Tue Dec 13 15:42:15 EST 2016
[0.00] Boot CPU: AArch64 Processor [500f0001]
[0.00] earlycon: uart8250 at MMIO32 0x1c021000 (options '')
[0.00] bootconsole [uart8250] enabled
[0.00] efi: Getting EFI parameters from FDT:
[0.00] efi: EFI v2.60 by HPE
[0.00] efi:  ACPI 2.0=0x4ff800  SMBIOS 3.0=0x4ff7a9  
MEMATTR=0x4ff2410698  RNG=0x4ff7e7f518 
[0.00] cma: Reserved 512 MiB at 0x0040e000
[0.00] ACPI: Early table checksum verification disabled
[0.00] ACPI: RSDP 0x004FF800 24 (v02 HP)
[0.00] ACPI: XSDT 0x004FF7FF 84 (v01 HP ProLiant 
0001  0113)
[0.00] ACPI: FACP 0x004FF7FB 000114 (v06 HPEProLiant 
0001 HP   0001)
[0.00] ACPI: DSDT 0x004FF7F8 0023CA (v05 HPE

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Jon Masters]

On 12/06/2016 03:18 PM, Bjorn Helgaas wrote:

> I just merged pci/ecam into my "next" branch, so as long as tomorrow's
> linux-next boots out of the box, we should be set.  I made the following
> changes compared to v11:
> 
>   - dropped the x86 change to use acpi_resource_consumer()
>   - added parens around macro args used in arithmetic expressions
>   - renamed "seg" to "node" in THUNDER_PEM_RES and THUNDER_PEM_QUIRK
>   - incorporated (u64) cast and dropped "UL" postfix for THUNDER_PEM_QUIRK
> 
> Let me know if you see any issues.

Just following up. Please find attached a boot log from an HPE ProLiant m400
Moonshot X-Gene based cartridge running next-20161213 with pci/ecam branch.

Here is the /proc/iomem output as well:

# cat /proc/iomem
1052-10523fff : APMC0D18:00
  1052-10523fff : APMC0D18:00
10524000-10527fff : APMC0D17:00
1054-1054a0ff : APMC0D01:00
  10546000-10546fff : APMC0D50:00
  1054a000-1054a00f : APMC0D12:03
1054a000-1054a00f : APMC0D12:02
  1054a000-1054a00f : APMC0D12:01
1054a000-1054a00f : APMC0D12:00
1700-17000fff : APMC0D01:00
17001000-17001fff : APMC0D01:00
  17001000-170013ff : APMC0D15:00
17001000-170013ff : APMC0D15:00
1701c000-1701cfff : APMC0D14:00
1a80-1a800fff : APMC0D0D:00
  1a80-1a800fff : APMC0D0D:00
1c000200-1c0002ff : APMC0D06:00
1c021000-1c0210ff : APMC0D08:00
  1c021000-1c02101f : serial
1c024000-1c024fff : APMC0D07:00
1f23-1f230fff : APMC0D0D:00
  1f23-1f230fff : APMC0D0D:00
1f23d000-1f23dfff : APMC0D0D:00
  1f23d000-1f23dfff : APMC0D0D:00
1f23e000-1f23efff : APMC0D0D:00
  1f23e000-1f23efff : APMC0D0D:00
1f2a-1f31 : APMC0D06:00
1f50-1f50 : PNP0A08:00
7880-78800fff : APMC0D13:00
  7880-78800fff : APMC0D12:03
7880-78800fff : APMC0D12:02
  7880-78800fff : APMC0D12:01
7880-78800fff : APMC0D12:00
  7880-78800fff : APMC0D11:00
  7880-78800fff : APMC0D10:03
  7880-78800fff : APMC0D10:02
  7880-78800fff : APMC0D10:01
  7880-78800fff : APMC0D10:00
7900-798f : APMC0D0E:00
7c00-7c1f : APMC0D12:00
7c20-7c3f : APMC0D12:01
7c40-7c5f : APMC0D12:02
7c60-7c7f : APMC0D12:03
7e00-7e000fff : APMC0D13:00
7e20-7e200fff : APMC0D10:03
  7e20-7e200fff : APMC0D10:02
7e20-7e200fff : APMC0D10:01
  7e20-7e200fff : APMC0D10:00
7e60-7e600fff : APMC0D11:00
7e70-7e700fff : APMC0D10:03
  7e70-7e700fff : APMC0D10:02
7e70-7e700fff : APMC0D10:01
  7e70-7e700fff : APMC0D10:00
7e72-7e720fff : APMC0D10:03
  7e72-7e720fff : APMC0D10:02
7e72-7e720fff : APMC0D10:01
  7e72-7e720fff : APMC0D10:00
7e80-7e800fff : APMC0D10:00
7e84-7e840fff : APMC0D10:01
7e88-7e880fff : APMC0D10:02
7e8c-7e8c0fff : APMC0D10:03
7e93-7e930fff : APMC0D13:00
40-4001ff : System RAM
  48-4000c9 : Kernel code
  4000e2-400171 : Kernel data
40023a-4ff733 : System RAM
4ff734-4ff77c : reserved
4ff77d-4ff79c : System RAM
4ff79d-4ff7e7 : reserved
4ff7e8-4ff7e8 : System RAM
4ff7e9-4ff7ef : reserved
4ff7f1-4ff800 : reserved
4ff801-4f : System RAM
a02000-a03fff : PCI Bus :00
  a02000-a0201f : PCI Bus :01
a02000-a0200f : :01:00.0
  a02000-a0200f : mlx4_core
a02010-a0201f : :01:00.0
a06000-a07fff : PCI Bus :00
a0d000-a0dfff : PCI ECAM
a11000-a14fff : PCI Bus :00
  a11000-a121ff : PCI Bus :01
a11000-a111ff : :01:00.0
  a11000-a111ff : mlx4_core
a11200-a121ff : :01:00.0

Thanks again, Bjorn. Looking forward to seeing this upstream.

Tested-by: Jon Masters 

-- 
Computer Architect | Sent from my Fedora powered laptop

[0.00] Booting Linux on physical CPU 0x0
[0.00] Linux version 4.9.0-next-20161213.next20161213_jcm1 
(r...@hp-moonshot-02-c08.khw.lab.eng.bos.redhat.com) (gcc version 4.8.5 
20150623 (Red Hat 4.8.5-11) (GCC) ) #1 SMP Tue Dec 13 15:42:15 EST 2016
[0.00] Boot CPU: AArch64 Processor [500f0001]
[0.00] earlycon: uart8250 at MMIO32 0x1c021000 (options '')
[0.00] bootconsole [uart8250] enabled
[0.00] efi: Getting EFI parameters from FDT:
[0.00] efi: EFI v2.60 by HPE
[0.00] efi:  ACPI 2.0=0x4ff800  SMBIOS 3.0=0x4ff7a9  
MEMATTR=0x4ff2410698  RNG=0x4ff7e7f518 
[0.00] cma: Reserved 512 MiB at 0x0040e000
[0.00] ACPI: Early table checksum verification disabled
[0.00] ACPI: RSDP 0x004FF800 24 (v02 HP)
[0.00] ACPI: XSDT 0x004FF7FF 84 (v01 HP ProLiant 
0001  0113)
[0.00] ACPI: FACP 0x004FF7FB 000114 (v06 HPEProLiant 
0001 HP   0001)
[0.00] ACPI: DSDT 0x004FF7F8 0023CA (v05 HPEDSDT 
0001 

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Jon Masters]

On 12/06/2016 03:18 PM, Bjorn Helgaas wrote:
> On Tue, Dec 06, 2016 at 02:46:00PM -0500, Jon Masters wrote:
>> On 12/05/2016 04:21 PM, Bjorn Helgaas wrote:
>>> On Fri, Dec 02, 2016 at 07:33:46PM -0500, Jon Masters wrote:
>>
> Even without this patch, I don't think it's a show-stopper to have
> Linux mistakenly thinking this region is routed to PCI, because the
> driver does reserve it and the PCI core will never try to use it.

 Ok. So are you happy with pulling in Duc's v4 patch and retaining
 status quo on the bridge resources for 4.10?
>>>
>>> Yes, I think it looks good.  I'll finish packaging things up and
>>> repost the current series.
>>
>> Ok, great. So you're still pretty confident we'll have "out of the box"
>> booting on these machines for 4.10? :)
> 
> I just merged pci/ecam into my "next" branch, so as long as tomorrow's
> linux-next boots out of the box, we should be set.  I made the following
> changes compared to v11:
> 
>   - dropped the x86 change to use acpi_resource_consumer()
>   - added parens around macro args used in arithmetic expressions
>   - renamed "seg" to "node" in THUNDER_PEM_RES and THUNDER_PEM_QUIRK
>   - incorporated (u64) cast and dropped "UL" postfix for THUNDER_PEM_QUIRK
> 
> Let me know if you see any issues.

Thanks - I'll test linux-next tomorrow.

Jon.

-- 
Computer Architect | Sent from my Fedora powered laptop

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Jon Masters]

On 12/06/2016 03:18 PM, Bjorn Helgaas wrote:
> On Tue, Dec 06, 2016 at 02:46:00PM -0500, Jon Masters wrote:
>> On 12/05/2016 04:21 PM, Bjorn Helgaas wrote:
>>> On Fri, Dec 02, 2016 at 07:33:46PM -0500, Jon Masters wrote:
>>
> Even without this patch, I don't think it's a show-stopper to have
> Linux mistakenly thinking this region is routed to PCI, because the
> driver does reserve it and the PCI core will never try to use it.

 Ok. So are you happy with pulling in Duc's v4 patch and retaining
 status quo on the bridge resources for 4.10?
>>>
>>> Yes, I think it looks good.  I'll finish packaging things up and
>>> repost the current series.
>>
>> Ok, great. So you're still pretty confident we'll have "out of the box"
>> booting on these machines for 4.10? :)
> 
> I just merged pci/ecam into my "next" branch, so as long as tomorrow's
> linux-next boots out of the box, we should be set.  I made the following
> changes compared to v11:
> 
>   - dropped the x86 change to use acpi_resource_consumer()
>   - added parens around macro args used in arithmetic expressions
>   - renamed "seg" to "node" in THUNDER_PEM_RES and THUNDER_PEM_QUIRK
>   - incorporated (u64) cast and dropped "UL" postfix for THUNDER_PEM_QUIRK
> 
> Let me know if you see any issues.

Thanks - I'll test linux-next tomorrow.

Jon.

-- 
Computer Architect | Sent from my Fedora powered laptop

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Bjorn Helgaas]

On Tue, Dec 06, 2016 at 02:46:00PM -0500, Jon Masters wrote:
> On 12/05/2016 04:21 PM, Bjorn Helgaas wrote:
> > On Fri, Dec 02, 2016 at 07:33:46PM -0500, Jon Masters wrote:
> 
> >>> Even without this patch, I don't think it's a show-stopper to have
> >>> Linux mistakenly thinking this region is routed to PCI, because the
> >>> driver does reserve it and the PCI core will never try to use it.
> >>
> >> Ok. So are you happy with pulling in Duc's v4 patch and retaining
> >> status quo on the bridge resources for 4.10?
> > 
> > Yes, I think it looks good.  I'll finish packaging things up and
> > repost the current series.
> 
> Ok, great. So you're still pretty confident we'll have "out of the box"
> booting on these machines for 4.10? :)

I just merged pci/ecam into my "next" branch, so as long as tomorrow's
linux-next boots out of the box, we should be set.  I made the following
changes compared to v11:

  - dropped the x86 change to use acpi_resource_consumer()
  - added parens around macro args used in arithmetic expressions
  - renamed "seg" to "node" in THUNDER_PEM_RES and THUNDER_PEM_QUIRK
  - incorporated (u64) cast and dropped "UL" postfix for THUNDER_PEM_QUIRK

Let me know if you see any issues.

Bjorn

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Bjorn Helgaas]

On Tue, Dec 06, 2016 at 02:46:00PM -0500, Jon Masters wrote:
> On 12/05/2016 04:21 PM, Bjorn Helgaas wrote:
> > On Fri, Dec 02, 2016 at 07:33:46PM -0500, Jon Masters wrote:
> 
> >>> Even without this patch, I don't think it's a show-stopper to have
> >>> Linux mistakenly thinking this region is routed to PCI, because the
> >>> driver does reserve it and the PCI core will never try to use it.
> >>
> >> Ok. So are you happy with pulling in Duc's v4 patch and retaining
> >> status quo on the bridge resources for 4.10?
> > 
> > Yes, I think it looks good.  I'll finish packaging things up and
> > repost the current series.
> 
> Ok, great. So you're still pretty confident we'll have "out of the box"
> booting on these machines for 4.10? :)

I just merged pci/ecam into my "next" branch, so as long as tomorrow's
linux-next boots out of the box, we should be set.  I made the following
changes compared to v11:

  - dropped the x86 change to use acpi_resource_consumer()
  - added parens around macro args used in arithmetic expressions
  - renamed "seg" to "node" in THUNDER_PEM_RES and THUNDER_PEM_QUIRK
  - incorporated (u64) cast and dropped "UL" postfix for THUNDER_PEM_QUIRK

Let me know if you see any issues.

Bjorn

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Jon Masters]

On 12/05/2016 04:21 PM, Bjorn Helgaas wrote:
> On Fri, Dec 02, 2016 at 07:33:46PM -0500, Jon Masters wrote:

>>> Even without this patch, I don't think it's a show-stopper to have
>>> Linux mistakenly thinking this region is routed to PCI, because the
>>> driver does reserve it and the PCI core will never try to use it.
>>
>> Ok. So are you happy with pulling in Duc's v4 patch and retaining
>> status quo on the bridge resources for 4.10?
> 
> Yes, I think it looks good.  I'll finish packaging things up and
> repost the current series.

Ok, great. So you're still pretty confident we'll have "out of the box"
booting on these machines for 4.10? :)

Jon.

-- 
Computer Architect | Sent from my Fedora powered laptop

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Jon Masters]

On 12/05/2016 04:21 PM, Bjorn Helgaas wrote:
> On Fri, Dec 02, 2016 at 07:33:46PM -0500, Jon Masters wrote:

>>> Even without this patch, I don't think it's a show-stopper to have
>>> Linux mistakenly thinking this region is routed to PCI, because the
>>> driver does reserve it and the PCI core will never try to use it.
>>
>> Ok. So are you happy with pulling in Duc's v4 patch and retaining
>> status quo on the bridge resources for 4.10?
> 
> Yes, I think it looks good.  I'll finish packaging things up and
> repost the current series.

Ok, great. So you're still pretty confident we'll have "out of the box"
booting on these machines for 4.10? :)

Jon.

-- 
Computer Architect | Sent from my Fedora powered laptop

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Jon Masters]

On 12/05/2016 04:20 PM, Bjorn Helgaas wrote:
> On Fri, Dec 02, 2016 at 11:06:30PM -0800, Duc Dang wrote:
>> On Fri, Dec 2, 2016 at 3:39 PM, Bjorn Helgaas  wrote:
> 
>>> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
>>> index 8a177a1..a16fc8e 100644
>>> --- a/arch/arm64/kernel/pci.c
>>> +++ b/arch/arm64/kernel/pci.c
>>> @@ -114,6 +114,19 @@ int pcibios_root_bridge_prepare(struct pci_host_bridge 
>>> *bridge)
>>> return 0;
>>>  }
>>>
>>> +static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
>>> +{
>>> +   struct resource_entry *entry, *tmp;
>>> +   int status;
>>> +
>>> +   status = acpi_pci_probe_root_resources(ci);
>>> +   resource_list_for_each_entry_safe(entry, tmp, >resources) {
>>> +   if (!(entry->res->flags & IORESOURCE_WINDOW))
>>> +   resource_list_destroy_entry(entry);
>>> +   }
>>> +   return status;
>>> +}
>>> +
>>>  /*
>>>   * Lookup the bus range for the domain in MCFG, and set up config space
>>>   * mapping.
>>> @@ -190,6 +203,7 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root 
>>> *root)
>>> }
>>>
>>> root_ops->release_info = pci_acpi_generic_release_info;
>>> +   root_ops->prepare_resources = pci_acpi_root_prepare_resources;
>>> root_ops->pci_ops = >cfg->ops->pci_ops;
>>> bus = acpi_pci_root_create(root, root_ops, >common, ri->cfg);
>>> if (!bus)
>>
>> I tried your patch above with my X-Gene ECAM v4 patch on Mustang, here
>> is the kernel boot log and output of 'cat /proc/iomem'. The PCIe core
>> does not print the MMIO space as a window (which is expected per your
>> patch above).
> 
> Thanks!

...and just for the record, here it is on HPE ProLiant m400 (Moonshot),
with the same result that the region is no longer claimed as PCI space
(it - 1f50 - is now showing as being owned by PNP0A08:00):

# cat /proc/iomem 
1052-10523fff : APMC0D18:00
  1052-10523fff : APMC0D18:00
10524000-10527fff : APMC0D17:00
1054-1054a0ff : APMC0D01:00
  10546000-10546fff : APMC0D50:00
  1054a000-1054a00f : APMC0D12:03
1054a000-1054a00f : APMC0D12:02
  1054a000-1054a00f : APMC0D12:01
1054a000-1054a00f : APMC0D12:00
1700-17000fff : APMC0D01:00
17001000-17001fff : APMC0D01:00
  17001000-170013ff : APMC0D15:00
17001000-170013ff : APMC0D15:00
1701c000-1701cfff : APMC0D14:00
1a80-1a800fff : APMC0D0D:00
  1a80-1a800fff : APMC0D0D:00
1c000200-1c0002ff : APMC0D06:00
1c021000-1c0210ff : APMC0D08:00
  1c021000-1c02101f : serial
1c024000-1c024fff : APMC0D07:00
1f23-1f230fff : APMC0D0D:00
  1f23-1f230fff : APMC0D0D:00
1f23d000-1f23dfff : APMC0D0D:00
  1f23d000-1f23dfff : APMC0D0D:00
1f23e000-1f23efff : APMC0D0D:00
  1f23e000-1f23efff : APMC0D0D:00
1f2a-1f31 : APMC0D06:00
1f50-1f50 : PNP0A08:00
7880-78800fff : APMC0D13:00
  7880-78800fff : APMC0D12:03
7880-78800fff : APMC0D12:02
  7880-78800fff : APMC0D12:01
7880-78800fff : APMC0D12:00
  7880-78800fff : APMC0D11:00
  7880-78800fff : APMC0D10:03
  7880-78800fff : APMC0D10:02
  7880-78800fff : APMC0D10:01
  7880-78800fff : APMC0D10:00
7900-798f : APMC0D0E:00
7c00-7c1f : APMC0D12:00
7c20-7c3f : APMC0D12:01
7c40-7c5f : APMC0D12:02
7c60-7c7f : APMC0D12:03
7e00-7e000fff : APMC0D13:00
7e20-7e200fff : APMC0D10:03
  7e20-7e200fff : APMC0D10:02
7e20-7e200fff : APMC0D10:01
  7e20-7e200fff : APMC0D10:00
7e60-7e600fff : APMC0D11:00
7e70-7e700fff : APMC0D10:03
  7e70-7e700fff : APMC0D10:02
7e70-7e700fff : APMC0D10:01
  7e70-7e700fff : APMC0D10:00
7e72-7e720fff : APMC0D10:03
  7e72-7e720fff : APMC0D10:02
7e72-7e720fff : APMC0D10:01
  7e72-7e720fff : APMC0D10:00
7e80-7e800fff : APMC0D10:00
7e84-7e840fff : APMC0D10:01
7e88-7e880fff : APMC0D10:02
7e8c-7e8c0fff : APMC0D10:03
7e93-7e930fff : APMC0D13:00
40-4001ff : System RAM
  48-4000c3 : Kernel code
  4000db-400165 : Kernel data
40023a-4ff733 : System RAM
4ff734-4ff77c : reserved
4ff77d-4ff79c : System RAM
4ff79d-4ff7e7 : reserved
4ff7e8-4ff7e8 : System RAM
4ff7e9-4ff7ef : reserved
4ff7f1-4ff800 : reserved
4ff801-4f : System RAM
a02000-a03fff : PCI Bus :00
  a02000-a0201f : PCI Bus :01
a02000-a0200f : :01:00.0
  a02000-a0200f : mlx4_core
a02010-a0201f : :01:00.0
a06000-a07fff : PCI Bus :00
a0d000-a0dfff : PCI ECAM
a11000-a14fff : PCI Bus :00
  a11000-a121ff : PCI Bus :01
a11000-a111ff : :01:00.0
  a11000-a111ff : mlx4_core
a11200-a121ff : :01:00.0

Tested-by: Jon Masters 

-- 
Computer Architect | 

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Jon Masters]

On 12/05/2016 04:20 PM, Bjorn Helgaas wrote:
> On Fri, Dec 02, 2016 at 11:06:30PM -0800, Duc Dang wrote:
>> On Fri, Dec 2, 2016 at 3:39 PM, Bjorn Helgaas  wrote:
> 
>>> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
>>> index 8a177a1..a16fc8e 100644
>>> --- a/arch/arm64/kernel/pci.c
>>> +++ b/arch/arm64/kernel/pci.c
>>> @@ -114,6 +114,19 @@ int pcibios_root_bridge_prepare(struct pci_host_bridge 
>>> *bridge)
>>> return 0;
>>>  }
>>>
>>> +static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
>>> +{
>>> +   struct resource_entry *entry, *tmp;
>>> +   int status;
>>> +
>>> +   status = acpi_pci_probe_root_resources(ci);
>>> +   resource_list_for_each_entry_safe(entry, tmp, >resources) {
>>> +   if (!(entry->res->flags & IORESOURCE_WINDOW))
>>> +   resource_list_destroy_entry(entry);
>>> +   }
>>> +   return status;
>>> +}
>>> +
>>>  /*
>>>   * Lookup the bus range for the domain in MCFG, and set up config space
>>>   * mapping.
>>> @@ -190,6 +203,7 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root 
>>> *root)
>>> }
>>>
>>> root_ops->release_info = pci_acpi_generic_release_info;
>>> +   root_ops->prepare_resources = pci_acpi_root_prepare_resources;
>>> root_ops->pci_ops = >cfg->ops->pci_ops;
>>> bus = acpi_pci_root_create(root, root_ops, >common, ri->cfg);
>>> if (!bus)
>>
>> I tried your patch above with my X-Gene ECAM v4 patch on Mustang, here
>> is the kernel boot log and output of 'cat /proc/iomem'. The PCIe core
>> does not print the MMIO space as a window (which is expected per your
>> patch above).
> 
> Thanks!

...and just for the record, here it is on HPE ProLiant m400 (Moonshot),
with the same result that the region is no longer claimed as PCI space
(it - 1f50 - is now showing as being owned by PNP0A08:00):

# cat /proc/iomem 
1052-10523fff : APMC0D18:00
  1052-10523fff : APMC0D18:00
10524000-10527fff : APMC0D17:00
1054-1054a0ff : APMC0D01:00
  10546000-10546fff : APMC0D50:00
  1054a000-1054a00f : APMC0D12:03
1054a000-1054a00f : APMC0D12:02
  1054a000-1054a00f : APMC0D12:01
1054a000-1054a00f : APMC0D12:00
1700-17000fff : APMC0D01:00
17001000-17001fff : APMC0D01:00
  17001000-170013ff : APMC0D15:00
17001000-170013ff : APMC0D15:00
1701c000-1701cfff : APMC0D14:00
1a80-1a800fff : APMC0D0D:00
  1a80-1a800fff : APMC0D0D:00
1c000200-1c0002ff : APMC0D06:00
1c021000-1c0210ff : APMC0D08:00
  1c021000-1c02101f : serial
1c024000-1c024fff : APMC0D07:00
1f23-1f230fff : APMC0D0D:00
  1f23-1f230fff : APMC0D0D:00
1f23d000-1f23dfff : APMC0D0D:00
  1f23d000-1f23dfff : APMC0D0D:00
1f23e000-1f23efff : APMC0D0D:00
  1f23e000-1f23efff : APMC0D0D:00
1f2a-1f31 : APMC0D06:00
1f50-1f50 : PNP0A08:00
7880-78800fff : APMC0D13:00
  7880-78800fff : APMC0D12:03
7880-78800fff : APMC0D12:02
  7880-78800fff : APMC0D12:01
7880-78800fff : APMC0D12:00
  7880-78800fff : APMC0D11:00
  7880-78800fff : APMC0D10:03
  7880-78800fff : APMC0D10:02
  7880-78800fff : APMC0D10:01
  7880-78800fff : APMC0D10:00
7900-798f : APMC0D0E:00
7c00-7c1f : APMC0D12:00
7c20-7c3f : APMC0D12:01
7c40-7c5f : APMC0D12:02
7c60-7c7f : APMC0D12:03
7e00-7e000fff : APMC0D13:00
7e20-7e200fff : APMC0D10:03
  7e20-7e200fff : APMC0D10:02
7e20-7e200fff : APMC0D10:01
  7e20-7e200fff : APMC0D10:00
7e60-7e600fff : APMC0D11:00
7e70-7e700fff : APMC0D10:03
  7e70-7e700fff : APMC0D10:02
7e70-7e700fff : APMC0D10:01
  7e70-7e700fff : APMC0D10:00
7e72-7e720fff : APMC0D10:03
  7e72-7e720fff : APMC0D10:02
7e72-7e720fff : APMC0D10:01
  7e72-7e720fff : APMC0D10:00
7e80-7e800fff : APMC0D10:00
7e84-7e840fff : APMC0D10:01
7e88-7e880fff : APMC0D10:02
7e8c-7e8c0fff : APMC0D10:03
7e93-7e930fff : APMC0D13:00
40-4001ff : System RAM
  48-4000c3 : Kernel code
  4000db-400165 : Kernel data
40023a-4ff733 : System RAM
4ff734-4ff77c : reserved
4ff77d-4ff79c : System RAM
4ff79d-4ff7e7 : reserved
4ff7e8-4ff7e8 : System RAM
4ff7e9-4ff7ef : reserved
4ff7f1-4ff800 : reserved
4ff801-4f : System RAM
a02000-a03fff : PCI Bus :00
  a02000-a0201f : PCI Bus :01
a02000-a0200f : :01:00.0
  a02000-a0200f : mlx4_core
a02010-a0201f : :01:00.0
a06000-a07fff : PCI Bus :00
a0d000-a0dfff : PCI ECAM
a11000-a14fff : PCI Bus :00
  a11000-a121ff : PCI Bus :01
a11000-a111ff : :01:00.0
  a11000-a111ff : mlx4_core
a11200-a121ff : :01:00.0

Tested-by: Jon Masters 

-- 
Computer Architect | Sent from my Fedora powered laptop

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Duc Dang]

On Mon, Dec 5, 2016 at 1:53 PM, Bjorn Helgaas  wrote:
> On Thu, Dec 01, 2016 at 06:52:23PM -0800, Duc Dang wrote:
>> On Thu, Dec 1, 2016 at 10:33 AM, Bjorn Helgaas  wrote:
>
>> I made similar changes in v4 patch. The ECAM quirk will be built when
>> ACPI and PCI_QUIRKS are enabled.
>>
>> When building for DT only, the ECAM quirk won't be compiled.
>
> Perfect.
>
>> >>  #define XGENE_PCIE_IP_VER_UNKN   0
>> >>  #define XGENE_PCIE_IP_VER_1  1
>> >> +#define XGENE_PCIE_IP_VER_2  2
>> >
>> > This isn't used anywhere, which makes me wonder whether it's worth
>> > keeping it.
>>
>> V2 controller will use this XGENE_PCIE_IP_VER_2 (port->version =
>> XGENE_PCIE_IP_VER_2). This will be used to indicate that the
>> controller is V2, and to enable configuration request retry status
>> feature (by not disable it like V1 controller).
>
> OK, I see.  You don't actually need XGENE_PCIE_IP_VER_2, you just need
> port->version to be something other than XGENE_PCIE_IP_VER_1.  So this
> is fine as it is.
>
>> >>  static void __iomem *xgene_pcie_get_cfg_base(struct pci_bus *bus)
>> >>  {
>> >> - struct xgene_pcie_port *port = bus->sysdata;
>> >> + struct pci_config_window *cfg;
>> >> + struct xgene_pcie_port *port;
>> >> +
>> >> + if (acpi_disabled)
>> >> + port = bus->sysdata;
>> >> + else {
>> >> + cfg = bus->sysdata;
>> >> + port = cfg->priv;
>> >> + }
>> >
>> > I would really, really like to figure out a way to get rid of these
>> > "if (acpi_disabled)" checks sprinkled through here.  Is there any way
>> > we can set up bus->sysdata to be the same, regardless of whether we're
>> > using this as a platform driver or an ACPI quirk?
>>
>> Right now, I created a inline function to extract xgene_pcie_port from
>> pci_bus. In order to get rid of acpi_disabled, I will need to make
>> sysdata in DT case also point to pci_config_window structure, which
>> means I will need to convert and test the DT driver to use ecam ops.
>> It is a separate patch itself. So I think I should do it at later time
>> (after this ECAM quirk patch). I hope you are ok with this.
>
> OK.  I did the simple-minded version of leaving the DT ops the same
> but making sysdata point to a dummy pci_config_window.  Your proposal
> of using ECAM for DT would be much better.
>
> It's interesting that you actually already use the same accessors
> except that DT uses the 32-bit pci_generic_config_write32() and ACPI
> uses the regular pci_generic_config_write(). I guess that means the
> hardware actually *does* support sub-32 bit writes?

Yes, the hardware does support sub-32 bit writes (and reads). This is
another item in my TODO list for DT (which does not seem quite urgent
now): switch to use pci_generic_config_write for DT. But, well, I will
need to do that for read as well (for both ACPI and DT).

>
>> I need to define the function (xgene_get_csr_resource()) inside
>> pci-xgene.c to duplicate the code of acpi_get_rc_addr. The reason is
>> X-Gene firmware does not have a dedicate PNP0C02 node to declare the
>> resource, and if I use acpi_get_rc_resources() with "PNP0A08", I got
>> error due to acpi_bus_get_device() returns error.
>
> Looks good.
>
>> > All these init functions are almost identical.  Can we factor this out
>> > by having wrappers that do nothing more than pass in the table and
>> > version, and put the kzalloc and ioremap in a shared back-end?
>>
>> I refactor-ed these .init functions. And as a result, there are only 2
>> ecam ops left: xgene_v1_pcie_ecam_ops and xgene_v2_pcie_ecam_ops.
>
> Looks good.
>
> Bjorn
Regards,
Duc Dang.

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Duc Dang]

On Mon, Dec 5, 2016 at 1:53 PM, Bjorn Helgaas  wrote:
> On Thu, Dec 01, 2016 at 06:52:23PM -0800, Duc Dang wrote:
>> On Thu, Dec 1, 2016 at 10:33 AM, Bjorn Helgaas  wrote:
>
>> I made similar changes in v4 patch. The ECAM quirk will be built when
>> ACPI and PCI_QUIRKS are enabled.
>>
>> When building for DT only, the ECAM quirk won't be compiled.
>
> Perfect.
>
>> >>  #define XGENE_PCIE_IP_VER_UNKN   0
>> >>  #define XGENE_PCIE_IP_VER_1  1
>> >> +#define XGENE_PCIE_IP_VER_2  2
>> >
>> > This isn't used anywhere, which makes me wonder whether it's worth
>> > keeping it.
>>
>> V2 controller will use this XGENE_PCIE_IP_VER_2 (port->version =
>> XGENE_PCIE_IP_VER_2). This will be used to indicate that the
>> controller is V2, and to enable configuration request retry status
>> feature (by not disable it like V1 controller).
>
> OK, I see.  You don't actually need XGENE_PCIE_IP_VER_2, you just need
> port->version to be something other than XGENE_PCIE_IP_VER_1.  So this
> is fine as it is.
>
>> >>  static void __iomem *xgene_pcie_get_cfg_base(struct pci_bus *bus)
>> >>  {
>> >> - struct xgene_pcie_port *port = bus->sysdata;
>> >> + struct pci_config_window *cfg;
>> >> + struct xgene_pcie_port *port;
>> >> +
>> >> + if (acpi_disabled)
>> >> + port = bus->sysdata;
>> >> + else {
>> >> + cfg = bus->sysdata;
>> >> + port = cfg->priv;
>> >> + }
>> >
>> > I would really, really like to figure out a way to get rid of these
>> > "if (acpi_disabled)" checks sprinkled through here.  Is there any way
>> > we can set up bus->sysdata to be the same, regardless of whether we're
>> > using this as a platform driver or an ACPI quirk?
>>
>> Right now, I created a inline function to extract xgene_pcie_port from
>> pci_bus. In order to get rid of acpi_disabled, I will need to make
>> sysdata in DT case also point to pci_config_window structure, which
>> means I will need to convert and test the DT driver to use ecam ops.
>> It is a separate patch itself. So I think I should do it at later time
>> (after this ECAM quirk patch). I hope you are ok with this.
>
> OK.  I did the simple-minded version of leaving the DT ops the same
> but making sysdata point to a dummy pci_config_window.  Your proposal
> of using ECAM for DT would be much better.
>
> It's interesting that you actually already use the same accessors
> except that DT uses the 32-bit pci_generic_config_write32() and ACPI
> uses the regular pci_generic_config_write(). I guess that means the
> hardware actually *does* support sub-32 bit writes?

Yes, the hardware does support sub-32 bit writes (and reads). This is
another item in my TODO list for DT (which does not seem quite urgent
now): switch to use pci_generic_config_write for DT. But, well, I will
need to do that for read as well (for both ACPI and DT).

>
>> I need to define the function (xgene_get_csr_resource()) inside
>> pci-xgene.c to duplicate the code of acpi_get_rc_addr. The reason is
>> X-Gene firmware does not have a dedicate PNP0C02 node to declare the
>> resource, and if I use acpi_get_rc_resources() with "PNP0A08", I got
>> error due to acpi_bus_get_device() returns error.
>
> Looks good.
>
>> > All these init functions are almost identical.  Can we factor this out
>> > by having wrappers that do nothing more than pass in the table and
>> > version, and put the kzalloc and ioremap in a shared back-end?
>>
>> I refactor-ed these .init functions. And as a result, there are only 2
>> ecam ops left: xgene_v1_pcie_ecam_ops and xgene_v2_pcie_ecam_ops.
>
> Looks good.
>
> Bjorn
Regards,
Duc Dang.

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Bjorn Helgaas]

On Thu, Dec 01, 2016 at 06:52:23PM -0800, Duc Dang wrote:
> On Thu, Dec 1, 2016 at 10:33 AM, Bjorn Helgaas  wrote:

> I made similar changes in v4 patch. The ECAM quirk will be built when
> ACPI and PCI_QUIRKS are enabled.
> 
> When building for DT only, the ECAM quirk won't be compiled.

Perfect.

> >>  #define XGENE_PCIE_IP_VER_UNKN   0
> >>  #define XGENE_PCIE_IP_VER_1  1
> >> +#define XGENE_PCIE_IP_VER_2  2
> >
> > This isn't used anywhere, which makes me wonder whether it's worth
> > keeping it.
> 
> V2 controller will use this XGENE_PCIE_IP_VER_2 (port->version =
> XGENE_PCIE_IP_VER_2). This will be used to indicate that the
> controller is V2, and to enable configuration request retry status
> feature (by not disable it like V1 controller).

OK, I see.  You don't actually need XGENE_PCIE_IP_VER_2, you just need
port->version to be something other than XGENE_PCIE_IP_VER_1.  So this
is fine as it is.

> >>  static void __iomem *xgene_pcie_get_cfg_base(struct pci_bus *bus)
> >>  {
> >> - struct xgene_pcie_port *port = bus->sysdata;
> >> + struct pci_config_window *cfg;
> >> + struct xgene_pcie_port *port;
> >> +
> >> + if (acpi_disabled)
> >> + port = bus->sysdata;
> >> + else {
> >> + cfg = bus->sysdata;
> >> + port = cfg->priv;
> >> + }
> >
> > I would really, really like to figure out a way to get rid of these
> > "if (acpi_disabled)" checks sprinkled through here.  Is there any way
> > we can set up bus->sysdata to be the same, regardless of whether we're
> > using this as a platform driver or an ACPI quirk?
> 
> Right now, I created a inline function to extract xgene_pcie_port from
> pci_bus. In order to get rid of acpi_disabled, I will need to make
> sysdata in DT case also point to pci_config_window structure, which
> means I will need to convert and test the DT driver to use ecam ops.
> It is a separate patch itself. So I think I should do it at later time
> (after this ECAM quirk patch). I hope you are ok with this.

OK.  I did the simple-minded version of leaving the DT ops the same
but making sysdata point to a dummy pci_config_window.  Your proposal
of using ECAM for DT would be much better.

It's interesting that you actually already use the same accessors
except that DT uses the 32-bit pci_generic_config_write32() and ACPI
uses the regular pci_generic_config_write(). I guess that means the
hardware actually *does* support sub-32 bit writes?

> I need to define the function (xgene_get_csr_resource()) inside
> pci-xgene.c to duplicate the code of acpi_get_rc_addr. The reason is
> X-Gene firmware does not have a dedicate PNP0C02 node to declare the
> resource, and if I use acpi_get_rc_resources() with "PNP0A08", I got
> error due to acpi_bus_get_device() returns error.

Looks good.

> > All these init functions are almost identical.  Can we factor this out
> > by having wrappers that do nothing more than pass in the table and
> > version, and put the kzalloc and ioremap in a shared back-end?
> 
> I refactor-ed these .init functions. And as a result, there are only 2
> ecam ops left: xgene_v1_pcie_ecam_ops and xgene_v2_pcie_ecam_ops.

Looks good.

Bjorn

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Bjorn Helgaas]

On Thu, Dec 01, 2016 at 06:52:23PM -0800, Duc Dang wrote:
> On Thu, Dec 1, 2016 at 10:33 AM, Bjorn Helgaas  wrote:

> I made similar changes in v4 patch. The ECAM quirk will be built when
> ACPI and PCI_QUIRKS are enabled.
> 
> When building for DT only, the ECAM quirk won't be compiled.

Perfect.

> >>  #define XGENE_PCIE_IP_VER_UNKN   0
> >>  #define XGENE_PCIE_IP_VER_1  1
> >> +#define XGENE_PCIE_IP_VER_2  2
> >
> > This isn't used anywhere, which makes me wonder whether it's worth
> > keeping it.
> 
> V2 controller will use this XGENE_PCIE_IP_VER_2 (port->version =
> XGENE_PCIE_IP_VER_2). This will be used to indicate that the
> controller is V2, and to enable configuration request retry status
> feature (by not disable it like V1 controller).

OK, I see.  You don't actually need XGENE_PCIE_IP_VER_2, you just need
port->version to be something other than XGENE_PCIE_IP_VER_1.  So this
is fine as it is.

> >>  static void __iomem *xgene_pcie_get_cfg_base(struct pci_bus *bus)
> >>  {
> >> - struct xgene_pcie_port *port = bus->sysdata;
> >> + struct pci_config_window *cfg;
> >> + struct xgene_pcie_port *port;
> >> +
> >> + if (acpi_disabled)
> >> + port = bus->sysdata;
> >> + else {
> >> + cfg = bus->sysdata;
> >> + port = cfg->priv;
> >> + }
> >
> > I would really, really like to figure out a way to get rid of these
> > "if (acpi_disabled)" checks sprinkled through here.  Is there any way
> > we can set up bus->sysdata to be the same, regardless of whether we're
> > using this as a platform driver or an ACPI quirk?
> 
> Right now, I created a inline function to extract xgene_pcie_port from
> pci_bus. In order to get rid of acpi_disabled, I will need to make
> sysdata in DT case also point to pci_config_window structure, which
> means I will need to convert and test the DT driver to use ecam ops.
> It is a separate patch itself. So I think I should do it at later time
> (after this ECAM quirk patch). I hope you are ok with this.

OK.  I did the simple-minded version of leaving the DT ops the same
but making sysdata point to a dummy pci_config_window.  Your proposal
of using ECAM for DT would be much better.

It's interesting that you actually already use the same accessors
except that DT uses the 32-bit pci_generic_config_write32() and ACPI
uses the regular pci_generic_config_write(). I guess that means the
hardware actually *does* support sub-32 bit writes?

> I need to define the function (xgene_get_csr_resource()) inside
> pci-xgene.c to duplicate the code of acpi_get_rc_addr. The reason is
> X-Gene firmware does not have a dedicate PNP0C02 node to declare the
> resource, and if I use acpi_get_rc_resources() with "PNP0A08", I got
> error due to acpi_bus_get_device() returns error.

Looks good.

> > All these init functions are almost identical.  Can we factor this out
> > by having wrappers that do nothing more than pass in the table and
> > version, and put the kzalloc and ioremap in a shared back-end?
> 
> I refactor-ed these .init functions. And as a result, there are only 2
> ecam ops left: xgene_v1_pcie_ecam_ops and xgene_v2_pcie_ecam_ops.

Looks good.

Bjorn

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Duc Dang]

On Mon, Dec 5, 2016 at 1:20 PM, Bjorn Helgaas  wrote:
> On Fri, Dec 02, 2016 at 11:06:30PM -0800, Duc Dang wrote:
>> On Fri, Dec 2, 2016 at 3:39 PM, Bjorn Helgaas  wrote:
>
>> > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
>> > index 8a177a1..a16fc8e 100644
>> > --- a/arch/arm64/kernel/pci.c
>> > +++ b/arch/arm64/kernel/pci.c
>> > @@ -114,6 +114,19 @@ int pcibios_root_bridge_prepare(struct 
>> > pci_host_bridge *bridge)
>> > return 0;
>> >  }
>> >
>> > +static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
>> > +{
>> > +   struct resource_entry *entry, *tmp;
>> > +   int status;
>> > +
>> > +   status = acpi_pci_probe_root_resources(ci);
>> > +   resource_list_for_each_entry_safe(entry, tmp, >resources) {
>> > +   if (!(entry->res->flags & IORESOURCE_WINDOW))
>> > +   resource_list_destroy_entry(entry);
>> > +   }
>> > +   return status;
>> > +}
>> > +
>> >  /*
>> >   * Lookup the bus range for the domain in MCFG, and set up config space
>> >   * mapping.
>> > @@ -190,6 +203,7 @@ struct pci_bus *pci_acpi_scan_root(struct 
>> > acpi_pci_root *root)
>> > }
>> >
>> > root_ops->release_info = pci_acpi_generic_release_info;
>> > +   root_ops->prepare_resources = pci_acpi_root_prepare_resources;
>> > root_ops->pci_ops = >cfg->ops->pci_ops;
>> > bus = acpi_pci_root_create(root, root_ops, >common, ri->cfg);
>> > if (!bus)
>>
>> I tried your patch above with my X-Gene ECAM v4 patch on Mustang, here
>> is the kernel boot log and output of 'cat /proc/iomem'. The PCIe core
>> does not print the MMIO space as a window (which is expected per your
>> patch above).
>
> Thanks!
>
>> ACPI: PCI Root Bridge [PCI0] (domain  [bus 00-ff])
>> acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI]
>> acpi PNP0A08:00: _OSC: OS now controls [PCIeHotplug PME AER PCIeCapability]
>> acpi PNP0A08:00: MCFG quirk: ECAM at [mem 0xe0d000-0xe0dfff] for 
>> [bus 00-ff] with xgene_v1_pcie_ecam_ops
>> acpi PNP0A08:00: [Firmware Bug]: ECAM area [mem 0xe0d000-0xe0dfff] 
>> not reserved in ACPI namespace
>> acpi PNP0A08:00: ECAM at [mem 0xe0d000-0xe0dfff] for [bus 00-ff]
>> Remapped I/O 0x00e01000 to [io  0x-0x window]
>> PCI host bridge to bus :00
>> pci_bus :00: root bus resource [io  0x-0x window] (bus address 
>> [0x1000-0x1000])
>> pci_bus :00: root bus resource [mem 0xe04000-0xe07fff window] 
>> (bus address [0x4000-0x7fff])
>> pci_bus :00: root bus resource [mem 0xf0-0xff window]
>> pci_bus :00: root bus resource [bus 00-ff]
>
> Yup, no bridge register space here; that's good.  I assume the bridge
> registers are at [mem 0x1f2b-0x1f2b] as shown in /proc/iomem
> below.

Yes,  the bridge registers are at [mem 0x1f2b-0x1f2b].

>
>> [root@(none) ~]# cat /proc/io mem
>> ...
>> 1900-19007fff : 808622B7:00
>> 1900c100-190f : 808622B7:00
>>   1900c100-190f : 808622B7:00
>> 1980-19807fff : 808622B7:01
>> 1980c100-198f : 808622B7:01
>>   1980c100-198f : 808622B7:01
>> ...
>> 1f28-1f28 : 808622B7:00
>> 1f29-1f29 : 808622B7:01
>
> I'm curious what these "808622B7" devices are.  Per ACPI 6.0, sec
> 6.1.5, that looks like a PCI vendor ID, which I guess is a valid ACPI
> ID.  But these resources don't seem to have any connection with PCI
> (they're not in any of the host bridge apertures).

These are DesignWare USB 3.0 controllers (DWC3). The ACPI ID is
defined in drivers/usb/dwc3/core.c.
>
>> 1f2b-1f2b : PNP0A08:00
>
> Looks like the bridge register space; good.

Yes, it is.

>
>> e04000-e07fff : PCI Bus :00
>>   e04000-e0401f : PCI Bus :01
>> e04000-e0400f : :01:00.0
>>   e04000-e0400f : mlx4_core
>> e04010-e0401f : :01:00.0
>
>> e0d000-e0dfff : PCI ECAM
>
> This region should be described in either a PNP0C02 device or (if we
> decide we can allow "consumer" descriptors) the PNP0A08 device.  I
> assume you'll fix that in a future firmware release.

Yes, future firmware will have PNP0C02 node that describes this ECAM
space (or a new resource in PNP0A08 if we use 'consumer' descriptor).

>
> But I think this reservation from pci_ecam_create() is good enough for
> now.
>
>> f0-ff : PCI Bus :00
>>   f0-f001ff : PCI Bus :01
>> f0-f001ff : :01:00.0
>>   f0-f001ff : mlx4_core
Regards,
Duc Dang.

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Duc Dang]

On Mon, Dec 5, 2016 at 1:20 PM, Bjorn Helgaas  wrote:
> On Fri, Dec 02, 2016 at 11:06:30PM -0800, Duc Dang wrote:
>> On Fri, Dec 2, 2016 at 3:39 PM, Bjorn Helgaas  wrote:
>
>> > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
>> > index 8a177a1..a16fc8e 100644
>> > --- a/arch/arm64/kernel/pci.c
>> > +++ b/arch/arm64/kernel/pci.c
>> > @@ -114,6 +114,19 @@ int pcibios_root_bridge_prepare(struct 
>> > pci_host_bridge *bridge)
>> > return 0;
>> >  }
>> >
>> > +static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
>> > +{
>> > +   struct resource_entry *entry, *tmp;
>> > +   int status;
>> > +
>> > +   status = acpi_pci_probe_root_resources(ci);
>> > +   resource_list_for_each_entry_safe(entry, tmp, >resources) {
>> > +   if (!(entry->res->flags & IORESOURCE_WINDOW))
>> > +   resource_list_destroy_entry(entry);
>> > +   }
>> > +   return status;
>> > +}
>> > +
>> >  /*
>> >   * Lookup the bus range for the domain in MCFG, and set up config space
>> >   * mapping.
>> > @@ -190,6 +203,7 @@ struct pci_bus *pci_acpi_scan_root(struct 
>> > acpi_pci_root *root)
>> > }
>> >
>> > root_ops->release_info = pci_acpi_generic_release_info;
>> > +   root_ops->prepare_resources = pci_acpi_root_prepare_resources;
>> > root_ops->pci_ops = >cfg->ops->pci_ops;
>> > bus = acpi_pci_root_create(root, root_ops, >common, ri->cfg);
>> > if (!bus)
>>
>> I tried your patch above with my X-Gene ECAM v4 patch on Mustang, here
>> is the kernel boot log and output of 'cat /proc/iomem'. The PCIe core
>> does not print the MMIO space as a window (which is expected per your
>> patch above).
>
> Thanks!
>
>> ACPI: PCI Root Bridge [PCI0] (domain  [bus 00-ff])
>> acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI]
>> acpi PNP0A08:00: _OSC: OS now controls [PCIeHotplug PME AER PCIeCapability]
>> acpi PNP0A08:00: MCFG quirk: ECAM at [mem 0xe0d000-0xe0dfff] for 
>> [bus 00-ff] with xgene_v1_pcie_ecam_ops
>> acpi PNP0A08:00: [Firmware Bug]: ECAM area [mem 0xe0d000-0xe0dfff] 
>> not reserved in ACPI namespace
>> acpi PNP0A08:00: ECAM at [mem 0xe0d000-0xe0dfff] for [bus 00-ff]
>> Remapped I/O 0x00e01000 to [io  0x-0x window]
>> PCI host bridge to bus :00
>> pci_bus :00: root bus resource [io  0x-0x window] (bus address 
>> [0x1000-0x1000])
>> pci_bus :00: root bus resource [mem 0xe04000-0xe07fff window] 
>> (bus address [0x4000-0x7fff])
>> pci_bus :00: root bus resource [mem 0xf0-0xff window]
>> pci_bus :00: root bus resource [bus 00-ff]
>
> Yup, no bridge register space here; that's good.  I assume the bridge
> registers are at [mem 0x1f2b-0x1f2b] as shown in /proc/iomem
> below.

Yes,  the bridge registers are at [mem 0x1f2b-0x1f2b].

>
>> [root@(none) ~]# cat /proc/io mem
>> ...
>> 1900-19007fff : 808622B7:00
>> 1900c100-190f : 808622B7:00
>>   1900c100-190f : 808622B7:00
>> 1980-19807fff : 808622B7:01
>> 1980c100-198f : 808622B7:01
>>   1980c100-198f : 808622B7:01
>> ...
>> 1f28-1f28 : 808622B7:00
>> 1f29-1f29 : 808622B7:01
>
> I'm curious what these "808622B7" devices are.  Per ACPI 6.0, sec
> 6.1.5, that looks like a PCI vendor ID, which I guess is a valid ACPI
> ID.  But these resources don't seem to have any connection with PCI
> (they're not in any of the host bridge apertures).

These are DesignWare USB 3.0 controllers (DWC3). The ACPI ID is
defined in drivers/usb/dwc3/core.c.
>
>> 1f2b-1f2b : PNP0A08:00
>
> Looks like the bridge register space; good.

Yes, it is.

>
>> e04000-e07fff : PCI Bus :00
>>   e04000-e0401f : PCI Bus :01
>> e04000-e0400f : :01:00.0
>>   e04000-e0400f : mlx4_core
>> e04010-e0401f : :01:00.0
>
>> e0d000-e0dfff : PCI ECAM
>
> This region should be described in either a PNP0C02 device or (if we
> decide we can allow "consumer" descriptors) the PNP0A08 device.  I
> assume you'll fix that in a future firmware release.

Yes, future firmware will have PNP0C02 node that describes this ECAM
space (or a new resource in PNP0A08 if we use 'consumer' descriptor).

>
> But I think this reservation from pci_ecam_create() is good enough for
> now.
>
>> f0-ff : PCI Bus :00
>>   f0-f001ff : PCI Bus :01
>> f0-f001ff : :01:00.0
>>   f0-f001ff : mlx4_core
Regards,
Duc Dang.

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Bjorn Helgaas]

On Fri, Dec 02, 2016 at 07:33:46PM -0500, Jon Masters wrote:
> On 12/02/2016 06:39 PM, Bjorn Helgaas wrote:
> > On Thu, Dec 01, 2016 at 11:08:23PM -0500, Jon Masters wrote:
> 
> >> Let's see if I summarized this correctly...
> >>
> >> 1. The MMIO registers for the host bridge itself need to be described
> >>somewhere, especially if we need to find those in a quirk and poke
> >>them. Since those registers are very much part of the bridge device,
> >>it makes sense for them to be in the _CRS for PNP0A08/PNP0A03.
> >>
> >> 2. The address space covering these registers MUST be described as a
> >>ResourceConsumer in order to avoid accidentally exposing them as
> >>available for use by downstream devices on the PCI bus.
> >>
> >> 3. The ACPI specification allows for resources of the type "Memory32Fixed".
> >>This is a macro that doesn't have the notion of a producer or consumer.
> >>HOWEVER various interpretations seem to be that this could/should
> >>default to being interpreted as a consumed region.
> > 
> > I agree; I think that per spec, Memory24, Memory32, Memory32Fixed, IO,
> > and FixedIO should all be for consumed resources, not for bridge
> > windows, since they don't have the notion of producer.
> 
> Ok. If we ultimately codify this somewhere as the general Linux kernel
> consensus (Rafael?) then we can also go and get the various ARM server
> specs updated to reflect this in (for e.g.) reference firmware builds.
> 
> > I'm pretty sure there's x86 firmware in the field that uses these for
> > windows, so I think we have to accept that usage, at least on x86.
> 
> Ok. I was pondering how to even go about finding that out, but even if
> I scheduled a job across RH's infra to look, that would be a drop in
> the bucket of possible machines that might be out there doing this.

Hmmm, when researching this, I thought I came across a change
specifically for a machine that used Memory32Fixed this way, but I
can't find it now.

The only thing I did find was some old experiments with Windows that
showed it interpreting a Memory32Fixed region as a window and putting
PCI devices in it: https://bugzilla.kernel.org/show_bug.cgi?id=15817
But that was a synthetic example with qemu, not a real machine in the
field.

> > Even without this patch, I don't think it's a show-stopper to have
> > Linux mistakenly thinking this region is routed to PCI, because the
> > driver does reserve it and the PCI core will never try to use it.
> 
> Ok. So are you happy with pulling in Duc's v4 patch and retaining
> status quo on the bridge resources for 4.10?

Yes, I think it looks good.  I'll finish packaging things up and
repost the current series.

Bjorn

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Bjorn Helgaas]

On Fri, Dec 02, 2016 at 07:33:46PM -0500, Jon Masters wrote:
> On 12/02/2016 06:39 PM, Bjorn Helgaas wrote:
> > On Thu, Dec 01, 2016 at 11:08:23PM -0500, Jon Masters wrote:
> 
> >> Let's see if I summarized this correctly...
> >>
> >> 1. The MMIO registers for the host bridge itself need to be described
> >>somewhere, especially if we need to find those in a quirk and poke
> >>them. Since those registers are very much part of the bridge device,
> >>it makes sense for them to be in the _CRS for PNP0A08/PNP0A03.
> >>
> >> 2. The address space covering these registers MUST be described as a
> >>ResourceConsumer in order to avoid accidentally exposing them as
> >>available for use by downstream devices on the PCI bus.
> >>
> >> 3. The ACPI specification allows for resources of the type "Memory32Fixed".
> >>This is a macro that doesn't have the notion of a producer or consumer.
> >>HOWEVER various interpretations seem to be that this could/should
> >>default to being interpreted as a consumed region.
> > 
> > I agree; I think that per spec, Memory24, Memory32, Memory32Fixed, IO,
> > and FixedIO should all be for consumed resources, not for bridge
> > windows, since they don't have the notion of producer.
> 
> Ok. If we ultimately codify this somewhere as the general Linux kernel
> consensus (Rafael?) then we can also go and get the various ARM server
> specs updated to reflect this in (for e.g.) reference firmware builds.
> 
> > I'm pretty sure there's x86 firmware in the field that uses these for
> > windows, so I think we have to accept that usage, at least on x86.
> 
> Ok. I was pondering how to even go about finding that out, but even if
> I scheduled a job across RH's infra to look, that would be a drop in
> the bucket of possible machines that might be out there doing this.

Hmmm, when researching this, I thought I came across a change
specifically for a machine that used Memory32Fixed this way, but I
can't find it now.

The only thing I did find was some old experiments with Windows that
showed it interpreting a Memory32Fixed region as a window and putting
PCI devices in it: https://bugzilla.kernel.org/show_bug.cgi?id=15817
But that was a synthetic example with qemu, not a real machine in the
field.

> > Even without this patch, I don't think it's a show-stopper to have
> > Linux mistakenly thinking this region is routed to PCI, because the
> > driver does reserve it and the PCI core will never try to use it.
> 
> Ok. So are you happy with pulling in Duc's v4 patch and retaining
> status quo on the bridge resources for 4.10?

Yes, I think it looks good.  I'll finish packaging things up and
repost the current series.

Bjorn

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Bjorn Helgaas]

On Fri, Dec 02, 2016 at 11:06:30PM -0800, Duc Dang wrote:
> On Fri, Dec 2, 2016 at 3:39 PM, Bjorn Helgaas  wrote:

> > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> > index 8a177a1..a16fc8e 100644
> > --- a/arch/arm64/kernel/pci.c
> > +++ b/arch/arm64/kernel/pci.c
> > @@ -114,6 +114,19 @@ int pcibios_root_bridge_prepare(struct pci_host_bridge 
> > *bridge)
> > return 0;
> >  }
> >
> > +static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
> > +{
> > +   struct resource_entry *entry, *tmp;
> > +   int status;
> > +
> > +   status = acpi_pci_probe_root_resources(ci);
> > +   resource_list_for_each_entry_safe(entry, tmp, >resources) {
> > +   if (!(entry->res->flags & IORESOURCE_WINDOW))
> > +   resource_list_destroy_entry(entry);
> > +   }
> > +   return status;
> > +}
> > +
> >  /*
> >   * Lookup the bus range for the domain in MCFG, and set up config space
> >   * mapping.
> > @@ -190,6 +203,7 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root 
> > *root)
> > }
> >
> > root_ops->release_info = pci_acpi_generic_release_info;
> > +   root_ops->prepare_resources = pci_acpi_root_prepare_resources;
> > root_ops->pci_ops = >cfg->ops->pci_ops;
> > bus = acpi_pci_root_create(root, root_ops, >common, ri->cfg);
> > if (!bus)
> 
> I tried your patch above with my X-Gene ECAM v4 patch on Mustang, here
> is the kernel boot log and output of 'cat /proc/iomem'. The PCIe core
> does not print the MMIO space as a window (which is expected per your
> patch above).

Thanks!

> ACPI: PCI Root Bridge [PCI0] (domain  [bus 00-ff])
> acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI]
> acpi PNP0A08:00: _OSC: OS now controls [PCIeHotplug PME AER PCIeCapability]
> acpi PNP0A08:00: MCFG quirk: ECAM at [mem 0xe0d000-0xe0dfff] for [bus 
> 00-ff] with xgene_v1_pcie_ecam_ops
> acpi PNP0A08:00: [Firmware Bug]: ECAM area [mem 0xe0d000-0xe0dfff] 
> not reserved in ACPI namespace
> acpi PNP0A08:00: ECAM at [mem 0xe0d000-0xe0dfff] for [bus 00-ff]
> Remapped I/O 0x00e01000 to [io  0x-0x window]
> PCI host bridge to bus :00
> pci_bus :00: root bus resource [io  0x-0x window] (bus address 
> [0x1000-0x1000])
> pci_bus :00: root bus resource [mem 0xe04000-0xe07fff window] 
> (bus address [0x4000-0x7fff])
> pci_bus :00: root bus resource [mem 0xf0-0xff window]
> pci_bus :00: root bus resource [bus 00-ff]

Yup, no bridge register space here; that's good.  I assume the bridge
registers are at [mem 0x1f2b-0x1f2b] as shown in /proc/iomem
below.

> [root@(none) ~]# cat /proc/io mem
> ...
> 1900-19007fff : 808622B7:00
> 1900c100-190f : 808622B7:00
>   1900c100-190f : 808622B7:00
> 1980-19807fff : 808622B7:01
> 1980c100-198f : 808622B7:01
>   1980c100-198f : 808622B7:01
> ...
> 1f28-1f28 : 808622B7:00
> 1f29-1f29 : 808622B7:01

I'm curious what these "808622B7" devices are.  Per ACPI 6.0, sec
6.1.5, that looks like a PCI vendor ID, which I guess is a valid ACPI
ID.  But these resources don't seem to have any connection with PCI
(they're not in any of the host bridge apertures).

> 1f2b-1f2b : PNP0A08:00

Looks like the bridge register space; good.

> e04000-e07fff : PCI Bus :00
>   e04000-e0401f : PCI Bus :01
> e04000-e0400f : :01:00.0
>   e04000-e0400f : mlx4_core
> e04010-e0401f : :01:00.0

> e0d000-e0dfff : PCI ECAM

This region should be described in either a PNP0C02 device or (if we
decide we can allow "consumer" descriptors) the PNP0A08 device.  I
assume you'll fix that in a future firmware release.

But I think this reservation from pci_ecam_create() is good enough for
now.

> f0-ff : PCI Bus :00
>   f0-f001ff : PCI Bus :01
> f0-f001ff : :01:00.0
>   f0-f001ff : mlx4_core

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Bjorn Helgaas]

On Fri, Dec 02, 2016 at 11:06:30PM -0800, Duc Dang wrote:
> On Fri, Dec 2, 2016 at 3:39 PM, Bjorn Helgaas  wrote:

> > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> > index 8a177a1..a16fc8e 100644
> > --- a/arch/arm64/kernel/pci.c
> > +++ b/arch/arm64/kernel/pci.c
> > @@ -114,6 +114,19 @@ int pcibios_root_bridge_prepare(struct pci_host_bridge 
> > *bridge)
> > return 0;
> >  }
> >
> > +static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci)
> > +{
> > +   struct resource_entry *entry, *tmp;
> > +   int status;
> > +
> > +   status = acpi_pci_probe_root_resources(ci);
> > +   resource_list_for_each_entry_safe(entry, tmp, >resources) {
> > +   if (!(entry->res->flags & IORESOURCE_WINDOW))
> > +   resource_list_destroy_entry(entry);
> > +   }
> > +   return status;
> > +}
> > +
> >  /*
> >   * Lookup the bus range for the domain in MCFG, and set up config space
> >   * mapping.
> > @@ -190,6 +203,7 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root 
> > *root)
> > }
> >
> > root_ops->release_info = pci_acpi_generic_release_info;
> > +   root_ops->prepare_resources = pci_acpi_root_prepare_resources;
> > root_ops->pci_ops = >cfg->ops->pci_ops;
> > bus = acpi_pci_root_create(root, root_ops, >common, ri->cfg);
> > if (!bus)
> 
> I tried your patch above with my X-Gene ECAM v4 patch on Mustang, here
> is the kernel boot log and output of 'cat /proc/iomem'. The PCIe core
> does not print the MMIO space as a window (which is expected per your
> patch above).

Thanks!

> ACPI: PCI Root Bridge [PCI0] (domain  [bus 00-ff])
> acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI]
> acpi PNP0A08:00: _OSC: OS now controls [PCIeHotplug PME AER PCIeCapability]
> acpi PNP0A08:00: MCFG quirk: ECAM at [mem 0xe0d000-0xe0dfff] for [bus 
> 00-ff] with xgene_v1_pcie_ecam_ops
> acpi PNP0A08:00: [Firmware Bug]: ECAM area [mem 0xe0d000-0xe0dfff] 
> not reserved in ACPI namespace
> acpi PNP0A08:00: ECAM at [mem 0xe0d000-0xe0dfff] for [bus 00-ff]
> Remapped I/O 0x00e01000 to [io  0x-0x window]
> PCI host bridge to bus :00
> pci_bus :00: root bus resource [io  0x-0x window] (bus address 
> [0x1000-0x1000])
> pci_bus :00: root bus resource [mem 0xe04000-0xe07fff window] 
> (bus address [0x4000-0x7fff])
> pci_bus :00: root bus resource [mem 0xf0-0xff window]
> pci_bus :00: root bus resource [bus 00-ff]

Yup, no bridge register space here; that's good.  I assume the bridge
registers are at [mem 0x1f2b-0x1f2b] as shown in /proc/iomem
below.

> [root@(none) ~]# cat /proc/io mem
> ...
> 1900-19007fff : 808622B7:00
> 1900c100-190f : 808622B7:00
>   1900c100-190f : 808622B7:00
> 1980-19807fff : 808622B7:01
> 1980c100-198f : 808622B7:01
>   1980c100-198f : 808622B7:01
> ...
> 1f28-1f28 : 808622B7:00
> 1f29-1f29 : 808622B7:01

I'm curious what these "808622B7" devices are.  Per ACPI 6.0, sec
6.1.5, that looks like a PCI vendor ID, which I guess is a valid ACPI
ID.  But these resources don't seem to have any connection with PCI
(they're not in any of the host bridge apertures).

> 1f2b-1f2b : PNP0A08:00

Looks like the bridge register space; good.

> e04000-e07fff : PCI Bus :00
>   e04000-e0401f : PCI Bus :01
> e04000-e0400f : :01:00.0
>   e04000-e0400f : mlx4_core
> e04010-e0401f : :01:00.0

> e0d000-e0dfff : PCI ECAM

This region should be described in either a PNP0C02 device or (if we
decide we can allow "consumer" descriptors) the PNP0A08 device.  I
assume you'll fix that in a future firmware release.

But I think this reservation from pci_ecam_create() is good enough for
now.

> f0-ff : PCI Bus :00
>   f0-f001ff : PCI Bus :01
> f0-f001ff : :01:00.0
>   f0-f001ff : mlx4_core

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Duc Dang]

On Fri, Dec 2, 2016 at 3:39 PM, Bjorn Helgaas  wrote:
>
> On Thu, Dec 01, 2016 at 11:08:23PM -0500, Jon Masters wrote:
> > Hi Bjorn, Duc, Mark,
> >
> > I switched my brain to the on mode and went and read some specs, and a few
> > tables, so here's my 2 cents on this...
> >
> > On 12/01/2016 06:22 PM, Duc Dang wrote:
> > > On Thu, Dec 1, 2016 at 3:07 PM, Bjorn Helgaas  wrote:
> > >> On Thu, Dec 01, 2016 at 02:10:10PM -0800, Duc Dang wrote:
> >
> > > The SoC provide some number of RC bridges, each with a different base
> > > for some mmio registers. Even if segment is legitimate in MCFG, there
> > > is still a problem if a platform doesn't use the segment ordering
> > > implied by the code. But the PNP0A03 _CRS does have this base address
> > > as the first memory resource, so we could get it from there and not
> > > have hard-coded addresses and implied ording in the quirk code.
> > 
> >  I'm confused.  Doesn't the current code treat every item in PNP0A03
> >  _CRS as a window?  Do you mean the first resource is handled
> >  differently somehow?  The Consumer/Producer bit could allow us to do
> >  this by marking the RC MMIO space as "Consumer", but I didn't think
> >  that strategy was quite working yet.
> >
> > Let's see if I summarized this correctly...
> >
> > 1. The MMIO registers for the host bridge itself need to be described
> >somewhere, especially if we need to find those in a quirk and poke
> >them. Since those registers are very much part of the bridge device,
> >it makes sense for them to be in the _CRS for PNP0A08/PNP0A03.
> >
> > 2. The address space covering these registers MUST be described as a
> >ResourceConsumer in order to avoid accidentally exposing them as
> >available for use by downstream devices on the PCI bus.
> >
> > 3. The ACPI specification allows for resources of the type "Memory32Fixed".
> >This is a macro that doesn't have the notion of a producer or consumer.
> >HOWEVER various interpretations seem to be that this could/should
> >default to being interpreted as a consumed region.
>
> I agree; I think that per spec, Memory24, Memory32, Memory32Fixed, IO,
> and FixedIO should all be for consumed resources, not for bridge
> windows, since they don't have the notion of producer.
>
> I'm pretty sure there's x86 firmware in the field that uses these for
> windows, so I think we have to accept that usage, at least on x86.
>
> > 4. At one point, a regression was added to the kernel:
> >
> >63f1789ec716 ("x86/PCI/ACPI: Ignore resources consumed by
> >host bridge itself")
> >
> >Which lead to a series on conversations about what should happen
> >for bridge resources (e.g. https://lkml.org/lkml/2015/3/24/962 )
> >
> > 5. This resulted in the following commit reverting point 4:
> >
> >2c62e8492ed7 ("x86/PCI/ACPI: Make all resources except [io 0xcf8-0xcff]
> >available on PCI bus")
> >
> >Which also stated that:
> >
> >"This solution will also ease the way to consolidate ACPI PCI host
> > bridge common code from x86, ia64 and ARM64"
> >
> > End of summary.
> >
> > So it seems that generally there is an aversion to having bridge resources
> > be described in this manner and you would like to require that they be
> > described e.g. using QWordMemory with a ResourceConsumer type?
>
> Per spec, we should ignore the Consumer/Producer bit in Word/DWord/QWord
> descriptors.  In bridge devices on x86, I think we have to treat them as
> producers (windows) because that's how they've been typically used.
>
> > BUT if we were to do that, it would break existing shipping systems since
> > there are quirks out there that use this form to find the base CSR:
> >
> >if (acpi_res->type == ACPI_RESOURCE_TYPE_FIXED_MEMORY32) {
> > fixed32 = _res->data.fixed_memory32;
> > port->csr_base = ioremap(fixed32->address,
> >  fixed32->address_length);
> > return AE_CTRL_TERMINATE;
> > }
>
> I think this is a valid usage of FixedMemory32 in terms of the spec.
> Linux currently handles this as a window if it appears in a PNP0A03
> device because some x86 firmware used it that way.
>
> We might be able to handle it differently on arm64, e.g., by making an
> arm64 version of pci_acpi_root_prepare_resources() that checks for
> IORESOURCE_WINDOW.
>
> > 2. What would happen if we had a difference policy on arm64 for such
> >resources. x86 has an "exception" for accessing the config space
> >using IO port 0xCF8-0xCFF (a fairly reasonable exception!) and
> >we can make the rules for a new platform (i.e. actually prescribe
> >exactly what the behavior is, rather than have it not be defined).
> >This is of course terrible in that existing BIOS vendors and so on
> >won't necessarily know this when working on ARM ACPI later on.
>
> > Indeed. And 

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Duc Dang]

On Fri, Dec 2, 2016 at 3:39 PM, Bjorn Helgaas  wrote:
>
> On Thu, Dec 01, 2016 at 11:08:23PM -0500, Jon Masters wrote:
> > Hi Bjorn, Duc, Mark,
> >
> > I switched my brain to the on mode and went and read some specs, and a few
> > tables, so here's my 2 cents on this...
> >
> > On 12/01/2016 06:22 PM, Duc Dang wrote:
> > > On Thu, Dec 1, 2016 at 3:07 PM, Bjorn Helgaas  wrote:
> > >> On Thu, Dec 01, 2016 at 02:10:10PM -0800, Duc Dang wrote:
> >
> > > The SoC provide some number of RC bridges, each with a different base
> > > for some mmio registers. Even if segment is legitimate in MCFG, there
> > > is still a problem if a platform doesn't use the segment ordering
> > > implied by the code. But the PNP0A03 _CRS does have this base address
> > > as the first memory resource, so we could get it from there and not
> > > have hard-coded addresses and implied ording in the quirk code.
> > 
> >  I'm confused.  Doesn't the current code treat every item in PNP0A03
> >  _CRS as a window?  Do you mean the first resource is handled
> >  differently somehow?  The Consumer/Producer bit could allow us to do
> >  this by marking the RC MMIO space as "Consumer", but I didn't think
> >  that strategy was quite working yet.
> >
> > Let's see if I summarized this correctly...
> >
> > 1. The MMIO registers for the host bridge itself need to be described
> >somewhere, especially if we need to find those in a quirk and poke
> >them. Since those registers are very much part of the bridge device,
> >it makes sense for them to be in the _CRS for PNP0A08/PNP0A03.
> >
> > 2. The address space covering these registers MUST be described as a
> >ResourceConsumer in order to avoid accidentally exposing them as
> >available for use by downstream devices on the PCI bus.
> >
> > 3. The ACPI specification allows for resources of the type "Memory32Fixed".
> >This is a macro that doesn't have the notion of a producer or consumer.
> >HOWEVER various interpretations seem to be that this could/should
> >default to being interpreted as a consumed region.
>
> I agree; I think that per spec, Memory24, Memory32, Memory32Fixed, IO,
> and FixedIO should all be for consumed resources, not for bridge
> windows, since they don't have the notion of producer.
>
> I'm pretty sure there's x86 firmware in the field that uses these for
> windows, so I think we have to accept that usage, at least on x86.
>
> > 4. At one point, a regression was added to the kernel:
> >
> >63f1789ec716 ("x86/PCI/ACPI: Ignore resources consumed by
> >host bridge itself")
> >
> >Which lead to a series on conversations about what should happen
> >for bridge resources (e.g. https://lkml.org/lkml/2015/3/24/962 )
> >
> > 5. This resulted in the following commit reverting point 4:
> >
> >2c62e8492ed7 ("x86/PCI/ACPI: Make all resources except [io 0xcf8-0xcff]
> >available on PCI bus")
> >
> >Which also stated that:
> >
> >"This solution will also ease the way to consolidate ACPI PCI host
> > bridge common code from x86, ia64 and ARM64"
> >
> > End of summary.
> >
> > So it seems that generally there is an aversion to having bridge resources
> > be described in this manner and you would like to require that they be
> > described e.g. using QWordMemory with a ResourceConsumer type?
>
> Per spec, we should ignore the Consumer/Producer bit in Word/DWord/QWord
> descriptors.  In bridge devices on x86, I think we have to treat them as
> producers (windows) because that's how they've been typically used.
>
> > BUT if we were to do that, it would break existing shipping systems since
> > there are quirks out there that use this form to find the base CSR:
> >
> >if (acpi_res->type == ACPI_RESOURCE_TYPE_FIXED_MEMORY32) {
> > fixed32 = _res->data.fixed_memory32;
> > port->csr_base = ioremap(fixed32->address,
> >  fixed32->address_length);
> > return AE_CTRL_TERMINATE;
> > }
>
> I think this is a valid usage of FixedMemory32 in terms of the spec.
> Linux currently handles this as a window if it appears in a PNP0A03
> device because some x86 firmware used it that way.
>
> We might be able to handle it differently on arm64, e.g., by making an
> arm64 version of pci_acpi_root_prepare_resources() that checks for
> IORESOURCE_WINDOW.
>
> > 2. What would happen if we had a difference policy on arm64 for such
> >resources. x86 has an "exception" for accessing the config space
> >using IO port 0xCF8-0xCFF (a fairly reasonable exception!) and
> >we can make the rules for a new platform (i.e. actually prescribe
> >exactly what the behavior is, rather than have it not be defined).
> >This is of course terrible in that existing BIOS vendors and so on
> >won't necessarily know this when working on ARM ACPI later on.
>
> > Indeed. And in the case of m400, it is currently 

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Jon Masters]

On 12/02/2016 06:39 PM, Bjorn Helgaas wrote:
> On Thu, Dec 01, 2016 at 11:08:23PM -0500, Jon Masters wrote:

>> Let's see if I summarized this correctly...
>>
>> 1. The MMIO registers for the host bridge itself need to be described
>>somewhere, especially if we need to find those in a quirk and poke
>>them. Since those registers are very much part of the bridge device,
>>it makes sense for them to be in the _CRS for PNP0A08/PNP0A03.
>>
>> 2. The address space covering these registers MUST be described as a
>>ResourceConsumer in order to avoid accidentally exposing them as
>>available for use by downstream devices on the PCI bus.
>>
>> 3. The ACPI specification allows for resources of the type "Memory32Fixed".
>>This is a macro that doesn't have the notion of a producer or consumer.
>>HOWEVER various interpretations seem to be that this could/should
>>default to being interpreted as a consumed region.
> 
> I agree; I think that per spec, Memory24, Memory32, Memory32Fixed, IO,
> and FixedIO should all be for consumed resources, not for bridge
> windows, since they don't have the notion of producer.

Ok. If we ultimately codify this somewhere as the general Linux kernel
consensus (Rafael?) then we can also go and get the various ARM server
specs updated to reflect this in (for e.g.) reference firmware builds.

> I'm pretty sure there's x86 firmware in the field that uses these for
> windows, so I think we have to accept that usage, at least on x86.

Ok. I was pondering how to even go about finding that out, but even if
I scheduled a job across RH's infra to look, that would be a drop in
the bucket of possible machines that might be out there doing this.



> Per spec, we should ignore the Consumer/Producer bit in Word/DWord/QWord
> descriptors.  In bridge devices on x86, I think we have to treat them as
> producers (windows) because that's how they've been typically used.  

Ok.

>> BUT if we were to do that, it would break existing shipping systems since
>> there are quirks out there that use this form to find the base CSR:
>>
>>if (acpi_res->type == ACPI_RESOURCE_TYPE_FIXED_MEMORY32) {
>> fixed32 = _res->data.fixed_memory32;
>> port->csr_base = ioremap(fixed32->address,
>>  fixed32->address_length);
>> return AE_CTRL_TERMINATE;
>> }
> 
> I think this is a valid usage of FixedMemory32 in terms of the spec.
> Linux currently handles this as a window if it appears in a PNP0A03
> device because some x86 firmware used it that way.
> 
> We might be able to handle it differently on arm64, e.g., by making an
> arm64 version of pci_acpi_root_prepare_resources() that checks for
> IORESOURCE_WINDOW.

This is something we should figure out the consensus on and codify.

>> 2. What would happen if we had a difference policy on arm64 for such
>>resources. x86 has an "exception" for accessing the config space
>>using IO port 0xCF8-0xCFF (a fairly reasonable exception!) and
>>we can make the rules for a new platform (i.e. actually prescribe
>>exactly what the behavior is, rather than have it not be defined).
>>This is of course terrible in that existing BIOS vendors and so on
>>won't necessarily know this when working on ARM ACPI later on.
> 
>> Indeed. And in the case of m400, it is currently this in shipping systems:
>>
>>Memory32Fixed (ReadWrite,
>> 0x1F50, // Address Base
>> 0x0001, // Address Length
>> )
> 
> [0.822990] pci_bus :00: root bus resource [mem 
> 0x1f2b-0x1f2b]

 I think this is wrong.  The PCI core thinks [mem 0x1f2b-0x1f2b]
 is available for use by devices on bus :00, but I think you're
 saying it is consumed by the bridge itself, not forwarded down to PCI.
> 
> I think this ASL is perfectly spec-compliant, and what's wrong is the
> way Linux is interpreting it.
> 
> I'm not clear on what's terrible about idea 2.  I think it's basically
> what I suggested above, i.e., something like the patch below, which I
> think (hope) would keep us from thinking that region is a window.

I was guarded because I like harmony between architectures (where it
makes sense). But that said, there is nothing to prevent having a
different interpretation on ARM, as long as everyone agrees on it.

> Even without this patch, I don't think it's a show-stopper to have
> Linux mistakenly thinking this region is routed to PCI, because the
> driver does reserve it and the PCI core will never try to use it.

Ok. So are you happy with pulling in Duc's v4 patch and retaining
status quo on the bridge resources for 4.10? We can continue to
discuss this and ultimately set a direction for the spec, as well
as clean up existing and future designs (certainly the latter) to
ensure all possible resources used by a platform are described

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Jon Masters]

On 12/02/2016 06:39 PM, Bjorn Helgaas wrote:
> On Thu, Dec 01, 2016 at 11:08:23PM -0500, Jon Masters wrote:

>> Let's see if I summarized this correctly...
>>
>> 1. The MMIO registers for the host bridge itself need to be described
>>somewhere, especially if we need to find those in a quirk and poke
>>them. Since those registers are very much part of the bridge device,
>>it makes sense for them to be in the _CRS for PNP0A08/PNP0A03.
>>
>> 2. The address space covering these registers MUST be described as a
>>ResourceConsumer in order to avoid accidentally exposing them as
>>available for use by downstream devices on the PCI bus.
>>
>> 3. The ACPI specification allows for resources of the type "Memory32Fixed".
>>This is a macro that doesn't have the notion of a producer or consumer.
>>HOWEVER various interpretations seem to be that this could/should
>>default to being interpreted as a consumed region.
> 
> I agree; I think that per spec, Memory24, Memory32, Memory32Fixed, IO,
> and FixedIO should all be for consumed resources, not for bridge
> windows, since they don't have the notion of producer.

Ok. If we ultimately codify this somewhere as the general Linux kernel
consensus (Rafael?) then we can also go and get the various ARM server
specs updated to reflect this in (for e.g.) reference firmware builds.

> I'm pretty sure there's x86 firmware in the field that uses these for
> windows, so I think we have to accept that usage, at least on x86.

Ok. I was pondering how to even go about finding that out, but even if
I scheduled a job across RH's infra to look, that would be a drop in
the bucket of possible machines that might be out there doing this.



> Per spec, we should ignore the Consumer/Producer bit in Word/DWord/QWord
> descriptors.  In bridge devices on x86, I think we have to treat them as
> producers (windows) because that's how they've been typically used.  

Ok.

>> BUT if we were to do that, it would break existing shipping systems since
>> there are quirks out there that use this form to find the base CSR:
>>
>>if (acpi_res->type == ACPI_RESOURCE_TYPE_FIXED_MEMORY32) {
>> fixed32 = _res->data.fixed_memory32;
>> port->csr_base = ioremap(fixed32->address,
>>  fixed32->address_length);
>> return AE_CTRL_TERMINATE;
>> }
> 
> I think this is a valid usage of FixedMemory32 in terms of the spec.
> Linux currently handles this as a window if it appears in a PNP0A03
> device because some x86 firmware used it that way.
> 
> We might be able to handle it differently on arm64, e.g., by making an
> arm64 version of pci_acpi_root_prepare_resources() that checks for
> IORESOURCE_WINDOW.

This is something we should figure out the consensus on and codify.

>> 2. What would happen if we had a difference policy on arm64 for such
>>resources. x86 has an "exception" for accessing the config space
>>using IO port 0xCF8-0xCFF (a fairly reasonable exception!) and
>>we can make the rules for a new platform (i.e. actually prescribe
>>exactly what the behavior is, rather than have it not be defined).
>>This is of course terrible in that existing BIOS vendors and so on
>>won't necessarily know this when working on ARM ACPI later on.
> 
>> Indeed. And in the case of m400, it is currently this in shipping systems:
>>
>>Memory32Fixed (ReadWrite,
>> 0x1F50, // Address Base
>> 0x0001, // Address Length
>> )
> 
> [0.822990] pci_bus :00: root bus resource [mem 
> 0x1f2b-0x1f2b]

 I think this is wrong.  The PCI core thinks [mem 0x1f2b-0x1f2b]
 is available for use by devices on bus :00, but I think you're
 saying it is consumed by the bridge itself, not forwarded down to PCI.
> 
> I think this ASL is perfectly spec-compliant, and what's wrong is the
> way Linux is interpreting it.
> 
> I'm not clear on what's terrible about idea 2.  I think it's basically
> what I suggested above, i.e., something like the patch below, which I
> think (hope) would keep us from thinking that region is a window.

I was guarded because I like harmony between architectures (where it
makes sense). But that said, there is nothing to prevent having a
different interpretation on ARM, as long as everyone agrees on it.

> Even without this patch, I don't think it's a show-stopper to have
> Linux mistakenly thinking this region is routed to PCI, because the
> driver does reserve it and the PCI core will never try to use it.

Ok. So are you happy with pulling in Duc's v4 patch and retaining
status quo on the bridge resources for 4.10? We can continue to
discuss this and ultimately set a direction for the spec, as well
as clean up existing and future designs (certainly the latter) to
ensure all possible resources used by a platform are described

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Bjorn Helgaas]

On Thu, Dec 01, 2016 at 11:08:23PM -0500, Jon Masters wrote:
> Hi Bjorn, Duc, Mark,
> 
> I switched my brain to the on mode and went and read some specs, and a few
> tables, so here's my 2 cents on this...
> 
> On 12/01/2016 06:22 PM, Duc Dang wrote:
> > On Thu, Dec 1, 2016 at 3:07 PM, Bjorn Helgaas  wrote:
> >> On Thu, Dec 01, 2016 at 02:10:10PM -0800, Duc Dang wrote:
> 
> > The SoC provide some number of RC bridges, each with a different base
> > for some mmio registers. Even if segment is legitimate in MCFG, there
> > is still a problem if a platform doesn't use the segment ordering
> > implied by the code. But the PNP0A03 _CRS does have this base address
> > as the first memory resource, so we could get it from there and not
> > have hard-coded addresses and implied ording in the quirk code.
> 
>  I'm confused.  Doesn't the current code treat every item in PNP0A03
>  _CRS as a window?  Do you mean the first resource is handled
>  differently somehow?  The Consumer/Producer bit could allow us to do
>  this by marking the RC MMIO space as "Consumer", but I didn't think
>  that strategy was quite working yet.
> 
> Let's see if I summarized this correctly...
> 
> 1. The MMIO registers for the host bridge itself need to be described
>somewhere, especially if we need to find those in a quirk and poke
>them. Since those registers are very much part of the bridge device,
>it makes sense for them to be in the _CRS for PNP0A08/PNP0A03.
> 
> 2. The address space covering these registers MUST be described as a
>ResourceConsumer in order to avoid accidentally exposing them as
>available for use by downstream devices on the PCI bus.
> 
> 3. The ACPI specification allows for resources of the type "Memory32Fixed".
>This is a macro that doesn't have the notion of a producer or consumer.
>HOWEVER various interpretations seem to be that this could/should
>default to being interpreted as a consumed region.

I agree; I think that per spec, Memory24, Memory32, Memory32Fixed, IO,
and FixedIO should all be for consumed resources, not for bridge
windows, since they don't have the notion of producer.

I'm pretty sure there's x86 firmware in the field that uses these for
windows, so I think we have to accept that usage, at least on x86.

> 4. At one point, a regression was added to the kernel:
> 
>63f1789ec716 ("x86/PCI/ACPI: Ignore resources consumed by
>host bridge itself")
> 
>Which lead to a series on conversations about what should happen
>for bridge resources (e.g. https://lkml.org/lkml/2015/3/24/962 )
> 
> 5. This resulted in the following commit reverting point 4:
> 
>2c62e8492ed7 ("x86/PCI/ACPI: Make all resources except [io 0xcf8-0xcff]
>available on PCI bus")
> 
>Which also stated that:
> 
>"This solution will also ease the way to consolidate ACPI PCI host
> bridge common code from x86, ia64 and ARM64"
> 
> End of summary.
> 
> So it seems that generally there is an aversion to having bridge resources
> be described in this manner and you would like to require that they be
> described e.g. using QWordMemory with a ResourceConsumer type?

Per spec, we should ignore the Consumer/Producer bit in Word/DWord/QWord
descriptors.  In bridge devices on x86, I think we have to treat them as
producers (windows) because that's how they've been typically used.  

> BUT if we were to do that, it would break existing shipping systems since
> there are quirks out there that use this form to find the base CSR:
> 
>if (acpi_res->type == ACPI_RESOURCE_TYPE_FIXED_MEMORY32) {
> fixed32 = _res->data.fixed_memory32;
> port->csr_base = ioremap(fixed32->address,
>  fixed32->address_length);
> return AE_CTRL_TERMINATE;
> }

I think this is a valid usage of FixedMemory32 in terms of the spec.
Linux currently handles this as a window if it appears in a PNP0A03
device because some x86 firmware used it that way.

We might be able to handle it differently on arm64, e.g., by making an
arm64 version of pci_acpi_root_prepare_resources() that checks for
IORESOURCE_WINDOW.

> 2. What would happen if we had a difference policy on arm64 for such
>resources. x86 has an "exception" for accessing the config space
>using IO port 0xCF8-0xCFF (a fairly reasonable exception!) and
>we can make the rules for a new platform (i.e. actually prescribe
>exactly what the behavior is, rather than have it not be defined).
>This is of course terrible in that existing BIOS vendors and so on
>won't necessarily know this when working on ARM ACPI later on.

> Indeed. And in the case of m400, it is currently this in shipping systems:
> 
>Memory32Fixed (ReadWrite,
> 0x1F50, // Address Base
> 0x0001, // Address Length
> )

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Bjorn Helgaas]

On Thu, Dec 01, 2016 at 11:08:23PM -0500, Jon Masters wrote:
> Hi Bjorn, Duc, Mark,
> 
> I switched my brain to the on mode and went and read some specs, and a few
> tables, so here's my 2 cents on this...
> 
> On 12/01/2016 06:22 PM, Duc Dang wrote:
> > On Thu, Dec 1, 2016 at 3:07 PM, Bjorn Helgaas  wrote:
> >> On Thu, Dec 01, 2016 at 02:10:10PM -0800, Duc Dang wrote:
> 
> > The SoC provide some number of RC bridges, each with a different base
> > for some mmio registers. Even if segment is legitimate in MCFG, there
> > is still a problem if a platform doesn't use the segment ordering
> > implied by the code. But the PNP0A03 _CRS does have this base address
> > as the first memory resource, so we could get it from there and not
> > have hard-coded addresses and implied ording in the quirk code.
> 
>  I'm confused.  Doesn't the current code treat every item in PNP0A03
>  _CRS as a window?  Do you mean the first resource is handled
>  differently somehow?  The Consumer/Producer bit could allow us to do
>  this by marking the RC MMIO space as "Consumer", but I didn't think
>  that strategy was quite working yet.
> 
> Let's see if I summarized this correctly...
> 
> 1. The MMIO registers for the host bridge itself need to be described
>somewhere, especially if we need to find those in a quirk and poke
>them. Since those registers are very much part of the bridge device,
>it makes sense for them to be in the _CRS for PNP0A08/PNP0A03.
> 
> 2. The address space covering these registers MUST be described as a
>ResourceConsumer in order to avoid accidentally exposing them as
>available for use by downstream devices on the PCI bus.
> 
> 3. The ACPI specification allows for resources of the type "Memory32Fixed".
>This is a macro that doesn't have the notion of a producer or consumer.
>HOWEVER various interpretations seem to be that this could/should
>default to being interpreted as a consumed region.

I agree; I think that per spec, Memory24, Memory32, Memory32Fixed, IO,
and FixedIO should all be for consumed resources, not for bridge
windows, since they don't have the notion of producer.

I'm pretty sure there's x86 firmware in the field that uses these for
windows, so I think we have to accept that usage, at least on x86.

> 4. At one point, a regression was added to the kernel:
> 
>63f1789ec716 ("x86/PCI/ACPI: Ignore resources consumed by
>host bridge itself")
> 
>Which lead to a series on conversations about what should happen
>for bridge resources (e.g. https://lkml.org/lkml/2015/3/24/962 )
> 
> 5. This resulted in the following commit reverting point 4:
> 
>2c62e8492ed7 ("x86/PCI/ACPI: Make all resources except [io 0xcf8-0xcff]
>available on PCI bus")
> 
>Which also stated that:
> 
>"This solution will also ease the way to consolidate ACPI PCI host
> bridge common code from x86, ia64 and ARM64"
> 
> End of summary.
> 
> So it seems that generally there is an aversion to having bridge resources
> be described in this manner and you would like to require that they be
> described e.g. using QWordMemory with a ResourceConsumer type?

Per spec, we should ignore the Consumer/Producer bit in Word/DWord/QWord
descriptors.  In bridge devices on x86, I think we have to treat them as
producers (windows) because that's how they've been typically used.  

> BUT if we were to do that, it would break existing shipping systems since
> there are quirks out there that use this form to find the base CSR:
> 
>if (acpi_res->type == ACPI_RESOURCE_TYPE_FIXED_MEMORY32) {
> fixed32 = _res->data.fixed_memory32;
> port->csr_base = ioremap(fixed32->address,
>  fixed32->address_length);
> return AE_CTRL_TERMINATE;
> }

I think this is a valid usage of FixedMemory32 in terms of the spec.
Linux currently handles this as a window if it appears in a PNP0A03
device because some x86 firmware used it that way.

We might be able to handle it differently on arm64, e.g., by making an
arm64 version of pci_acpi_root_prepare_resources() that checks for
IORESOURCE_WINDOW.

> 2. What would happen if we had a difference policy on arm64 for such
>resources. x86 has an "exception" for accessing the config space
>using IO port 0xCF8-0xCFF (a fairly reasonable exception!) and
>we can make the rules for a new platform (i.e. actually prescribe
>exactly what the behavior is, rather than have it not be defined).
>This is of course terrible in that existing BIOS vendors and so on
>won't necessarily know this when working on ARM ACPI later on.

> Indeed. And in the case of m400, it is currently this in shipping systems:
> 
>Memory32Fixed (ReadWrite,
> 0x1F50, // Address Base
> 0x0001, // Address Length
> )

> >>> [

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Jon Masters]

Quick reply - sorry for top posting (it's 3am...) - I would favor keeping the 
existing Fixed32Memory _CRS but switching over to prefer the PNP entry as a 
good citizen. The trouble is that it would be unfortunate if existing distros 
stopped working on newer firmware and it would lead to IMO more pain than it is 
worth. Hopefully for this reason Bjorn will take your v4 as-is for now and let 
us all figure out the cleanest long term cleanup later.

-- 
Computer Architect | Sent from my 64-bit #ARM Powered phone

> On Dec 2, 2016, at 02:34, Duc Dang  wrote:
> 
>> On Thu, Dec 1, 2016 at 10:31 PM, Jon Masters  wrote:
>> Bjorn,
>> 
>> Although I think the below still applies (that we need to leave that
>> Memory32Fixed for existing deployments, and this is going to result
>> in /proc/iomem polution), I've done some more reading of your ecam
>> tree and the implementation of acpi_get_rc_resources you mentioned,
>> and in particular how the PNP0C02 devices actually get wired up.
>> 
>> I would like to be able to boot upstream on existing shipping and
>> deployed machines that are in the field (not to mention our labs), but
>> there's no reason we can't *also* get APM to add a new vendor specific
>> PNP0C02 to the ACPI namespace in future firmware updates (for at least
>> their own Mustang reference boards) matching segment to CSR, as in the
>> case of the HiSi patches. That might then allow for some later
>> preference to use that for the CSR rather than getting it from the RC
>> device. Still, it would be ideal to boot on machines that are shipping
>> from HPE and others at this moment, so I am still hopeful you'll
>> at least allow the approach from Duc's v4 for now (4.10).
> 
> APM X-Gene 1 and X-Gene 2 ACPI tables will absolutely have PNP0C02
> nodes (in upcoming firmware release). I hope to have a solution that
> works for both old buggy firmware and the future improved firmware. So
> I am thinking the CSR discovery will be like this:
> 
> (1) Use  acpi_get_rc_resources() to discover CSR resource by checking
> PNP0C02 nodes
> (2) (1) should succeed with the new firmware
> (3) If (1) fails, we can fall back to approach on v4 patch: calling
> xgene_get_csr_resource() to discover the CSR described by
> Memory32Fixed macro.
> 
> How do you feel about this? The drawback is the new firmware that does
> not have the CSR space described with Memory32Fixed macro will fail on
> the distro version that uses the old quirk (that relies on this
> Memory32Fixed macro).
> 
>> 
>> Another nasty option for later consideration could then be having
>> the kernel fake up any missing PNP0C02 on existing machines, but
>> it would need special knowledge of the platform to generate that
>> so as to handle the problem Mark flagged earlier (segment vs
>> controller mismatch on some platforms). That could be done with a
>> DMI quirk that matched on a specific (e.g. HPE) machine. It would
>> only be needed on "broken" existing machines, and could be added
>> post-4.10 to clean this up if you really want to do that.
> 
> Bjorn suggested similar approach (have a PNP quirk to fabricate a
> PNP0C02 device and decleare all the required resources there) on
> another thread. But as you said, this approach does not scale, it can
> only applicable for a specific machine (by checking DMI information to
> apply the PNP quirk).
> 
>> 
>> That's all very nasty...
>> 
>> Jon.
>> 
>>> On 12/01/2016 11:08 PM, Jon Masters wrote:
>>> Hi Bjorn, Duc, Mark,
>>> 
>>> I switched my brain to the on mode and went and read some specs, and a few
>>> tables, so here's my 2 cents on this...
>>> 
 On 12/01/2016 06:22 PM, Duc Dang wrote:
> On Thu, Dec 1, 2016 at 3:07 PM, Bjorn Helgaas  wrote:
> On Thu, Dec 01, 2016 at 02:10:10PM -0800, Duc Dang wrote:
>>> 
 The SoC provide some number of RC bridges, each with a different base
 for some mmio registers. Even if segment is legitimate in MCFG, there
 is still a problem if a platform doesn't use the segment ordering
 implied by the code. But the PNP0A03 _CRS does have this base address
 as the first memory resource, so we could get it from there and not
 have hard-coded addresses and implied ording in the quirk code.
>>> 
>>> I'm confused.  Doesn't the current code treat every item in PNP0A03
>>> _CRS as a window?  Do you mean the first resource is handled
>>> differently somehow?  The Consumer/Producer bit could allow us to do
>>> this by marking the RC MMIO space as "Consumer", but I didn't think
>>> that strategy was quite working yet.
>>> 
>>> Let's see if I summarized this correctly...
>>> 
>>> 1. The MMIO registers for the host bridge itself need to be described
>>>   somewhere, especially if we need to find those in a quirk and poke
>>>   them. Since those registers are very much part of the bridge device,
>>>   it makes sense for them to be in the _CRS for PNP0A08/PNP0A03.
>>> 

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Jon Masters]

Quick reply - sorry for top posting (it's 3am...) - I would favor keeping the 
existing Fixed32Memory _CRS but switching over to prefer the PNP entry as a 
good citizen. The trouble is that it would be unfortunate if existing distros 
stopped working on newer firmware and it would lead to IMO more pain than it is 
worth. Hopefully for this reason Bjorn will take your v4 as-is for now and let 
us all figure out the cleanest long term cleanup later.

-- 
Computer Architect | Sent from my 64-bit #ARM Powered phone

> On Dec 2, 2016, at 02:34, Duc Dang  wrote:
> 
>> On Thu, Dec 1, 2016 at 10:31 PM, Jon Masters  wrote:
>> Bjorn,
>> 
>> Although I think the below still applies (that we need to leave that
>> Memory32Fixed for existing deployments, and this is going to result
>> in /proc/iomem polution), I've done some more reading of your ecam
>> tree and the implementation of acpi_get_rc_resources you mentioned,
>> and in particular how the PNP0C02 devices actually get wired up.
>> 
>> I would like to be able to boot upstream on existing shipping and
>> deployed machines that are in the field (not to mention our labs), but
>> there's no reason we can't *also* get APM to add a new vendor specific
>> PNP0C02 to the ACPI namespace in future firmware updates (for at least
>> their own Mustang reference boards) matching segment to CSR, as in the
>> case of the HiSi patches. That might then allow for some later
>> preference to use that for the CSR rather than getting it from the RC
>> device. Still, it would be ideal to boot on machines that are shipping
>> from HPE and others at this moment, so I am still hopeful you'll
>> at least allow the approach from Duc's v4 for now (4.10).
> 
> APM X-Gene 1 and X-Gene 2 ACPI tables will absolutely have PNP0C02
> nodes (in upcoming firmware release). I hope to have a solution that
> works for both old buggy firmware and the future improved firmware. So
> I am thinking the CSR discovery will be like this:
> 
> (1) Use  acpi_get_rc_resources() to discover CSR resource by checking
> PNP0C02 nodes
> (2) (1) should succeed with the new firmware
> (3) If (1) fails, we can fall back to approach on v4 patch: calling
> xgene_get_csr_resource() to discover the CSR described by
> Memory32Fixed macro.
> 
> How do you feel about this? The drawback is the new firmware that does
> not have the CSR space described with Memory32Fixed macro will fail on
> the distro version that uses the old quirk (that relies on this
> Memory32Fixed macro).
> 
>> 
>> Another nasty option for later consideration could then be having
>> the kernel fake up any missing PNP0C02 on existing machines, but
>> it would need special knowledge of the platform to generate that
>> so as to handle the problem Mark flagged earlier (segment vs
>> controller mismatch on some platforms). That could be done with a
>> DMI quirk that matched on a specific (e.g. HPE) machine. It would
>> only be needed on "broken" existing machines, and could be added
>> post-4.10 to clean this up if you really want to do that.
> 
> Bjorn suggested similar approach (have a PNP quirk to fabricate a
> PNP0C02 device and decleare all the required resources there) on
> another thread. But as you said, this approach does not scale, it can
> only applicable for a specific machine (by checking DMI information to
> apply the PNP quirk).
> 
>> 
>> That's all very nasty...
>> 
>> Jon.
>> 
>>> On 12/01/2016 11:08 PM, Jon Masters wrote:
>>> Hi Bjorn, Duc, Mark,
>>> 
>>> I switched my brain to the on mode and went and read some specs, and a few
>>> tables, so here's my 2 cents on this...
>>> 
 On 12/01/2016 06:22 PM, Duc Dang wrote:
> On Thu, Dec 1, 2016 at 3:07 PM, Bjorn Helgaas  wrote:
> On Thu, Dec 01, 2016 at 02:10:10PM -0800, Duc Dang wrote:
>>> 
 The SoC provide some number of RC bridges, each with a different base
 for some mmio registers. Even if segment is legitimate in MCFG, there
 is still a problem if a platform doesn't use the segment ordering
 implied by the code. But the PNP0A03 _CRS does have this base address
 as the first memory resource, so we could get it from there and not
 have hard-coded addresses and implied ording in the quirk code.
>>> 
>>> I'm confused.  Doesn't the current code treat every item in PNP0A03
>>> _CRS as a window?  Do you mean the first resource is handled
>>> differently somehow?  The Consumer/Producer bit could allow us to do
>>> this by marking the RC MMIO space as "Consumer", but I didn't think
>>> that strategy was quite working yet.
>>> 
>>> Let's see if I summarized this correctly...
>>> 
>>> 1. The MMIO registers for the host bridge itself need to be described
>>>   somewhere, especially if we need to find those in a quirk and poke
>>>   them. Since those registers are very much part of the bridge device,
>>>   it makes sense for them to be in the _CRS for PNP0A08/PNP0A03.
>>> 
>>> 2. The address space covering these registers 

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Duc Dang]

On Thu, Dec 1, 2016 at 10:31 PM, Jon Masters  wrote:
> Bjorn,
>
> Although I think the below still applies (that we need to leave that
> Memory32Fixed for existing deployments, and this is going to result
> in /proc/iomem polution), I've done some more reading of your ecam
> tree and the implementation of acpi_get_rc_resources you mentioned,
> and in particular how the PNP0C02 devices actually get wired up.
>
> I would like to be able to boot upstream on existing shipping and
> deployed machines that are in the field (not to mention our labs), but
> there's no reason we can't *also* get APM to add a new vendor specific
> PNP0C02 to the ACPI namespace in future firmware updates (for at least
> their own Mustang reference boards) matching segment to CSR, as in the
> case of the HiSi patches. That might then allow for some later
> preference to use that for the CSR rather than getting it from the RC
> device. Still, it would be ideal to boot on machines that are shipping
> from HPE and others at this moment, so I am still hopeful you'll
> at least allow the approach from Duc's v4 for now (4.10).

APM X-Gene 1 and X-Gene 2 ACPI tables will absolutely have PNP0C02
nodes (in upcoming firmware release). I hope to have a solution that
works for both old buggy firmware and the future improved firmware. So
I am thinking the CSR discovery will be like this:

(1) Use  acpi_get_rc_resources() to discover CSR resource by checking
PNP0C02 nodes
(2) (1) should succeed with the new firmware
(3) If (1) fails, we can fall back to approach on v4 patch: calling
xgene_get_csr_resource() to discover the CSR described by
Memory32Fixed macro.

How do you feel about this? The drawback is the new firmware that does
not have the CSR space described with Memory32Fixed macro will fail on
the distro version that uses the old quirk (that relies on this
Memory32Fixed macro).

>
> Another nasty option for later consideration could then be having
> the kernel fake up any missing PNP0C02 on existing machines, but
> it would need special knowledge of the platform to generate that
> so as to handle the problem Mark flagged earlier (segment vs
> controller mismatch on some platforms). That could be done with a
> DMI quirk that matched on a specific (e.g. HPE) machine. It would
> only be needed on "broken" existing machines, and could be added
> post-4.10 to clean this up if you really want to do that.

Bjorn suggested similar approach (have a PNP quirk to fabricate a
PNP0C02 device and decleare all the required resources there) on
another thread. But as you said, this approach does not scale, it can
only applicable for a specific machine (by checking DMI information to
apply the PNP quirk).

>
> That's all very nasty...
>
> Jon.
>
> On 12/01/2016 11:08 PM, Jon Masters wrote:
>> Hi Bjorn, Duc, Mark,
>>
>> I switched my brain to the on mode and went and read some specs, and a few
>> tables, so here's my 2 cents on this...
>>
>> On 12/01/2016 06:22 PM, Duc Dang wrote:
>>> On Thu, Dec 1, 2016 at 3:07 PM, Bjorn Helgaas  wrote:
 On Thu, Dec 01, 2016 at 02:10:10PM -0800, Duc Dang wrote:
>>
>>> The SoC provide some number of RC bridges, each with a different base
>>> for some mmio registers. Even if segment is legitimate in MCFG, there
>>> is still a problem if a platform doesn't use the segment ordering
>>> implied by the code. But the PNP0A03 _CRS does have this base address
>>> as the first memory resource, so we could get it from there and not
>>> have hard-coded addresses and implied ording in the quirk code.
>>
>> I'm confused.  Doesn't the current code treat every item in PNP0A03
>> _CRS as a window?  Do you mean the first resource is handled
>> differently somehow?  The Consumer/Producer bit could allow us to do
>> this by marking the RC MMIO space as "Consumer", but I didn't think
>> that strategy was quite working yet.
>>
>> Let's see if I summarized this correctly...
>>
>> 1. The MMIO registers for the host bridge itself need to be described
>>somewhere, especially if we need to find those in a quirk and poke
>>them. Since those registers are very much part of the bridge device,
>>it makes sense for them to be in the _CRS for PNP0A08/PNP0A03.
>>
>> 2. The address space covering these registers MUST be described as a
>>ResourceConsumer in order to avoid accidentally exposing them as
>>available for use by downstream devices on the PCI bus.
>>
>> 3. The ACPI specification allows for resources of the type "Memory32Fixed".
>>This is a macro that doesn't have the notion of a producer or consumer.
>>HOWEVER various interpretations seem to be that this could/should
>>default to being interpreted as a consumed region.
>>
>> 4. At one point, a regression was added to the kernel:
>>
>>63f1789ec716 ("x86/PCI/ACPI: Ignore resources consumed by
>>host bridge itself")
>>
>>Which lead to a series on conversations about what 

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Duc Dang]

On Thu, Dec 1, 2016 at 10:31 PM, Jon Masters  wrote:
> Bjorn,
>
> Although I think the below still applies (that we need to leave that
> Memory32Fixed for existing deployments, and this is going to result
> in /proc/iomem polution), I've done some more reading of your ecam
> tree and the implementation of acpi_get_rc_resources you mentioned,
> and in particular how the PNP0C02 devices actually get wired up.
>
> I would like to be able to boot upstream on existing shipping and
> deployed machines that are in the field (not to mention our labs), but
> there's no reason we can't *also* get APM to add a new vendor specific
> PNP0C02 to the ACPI namespace in future firmware updates (for at least
> their own Mustang reference boards) matching segment to CSR, as in the
> case of the HiSi patches. That might then allow for some later
> preference to use that for the CSR rather than getting it from the RC
> device. Still, it would be ideal to boot on machines that are shipping
> from HPE and others at this moment, so I am still hopeful you'll
> at least allow the approach from Duc's v4 for now (4.10).

APM X-Gene 1 and X-Gene 2 ACPI tables will absolutely have PNP0C02
nodes (in upcoming firmware release). I hope to have a solution that
works for both old buggy firmware and the future improved firmware. So
I am thinking the CSR discovery will be like this:

(1) Use  acpi_get_rc_resources() to discover CSR resource by checking
PNP0C02 nodes
(2) (1) should succeed with the new firmware
(3) If (1) fails, we can fall back to approach on v4 patch: calling
xgene_get_csr_resource() to discover the CSR described by
Memory32Fixed macro.

How do you feel about this? The drawback is the new firmware that does
not have the CSR space described with Memory32Fixed macro will fail on
the distro version that uses the old quirk (that relies on this
Memory32Fixed macro).

>
> Another nasty option for later consideration could then be having
> the kernel fake up any missing PNP0C02 on existing machines, but
> it would need special knowledge of the platform to generate that
> so as to handle the problem Mark flagged earlier (segment vs
> controller mismatch on some platforms). That could be done with a
> DMI quirk that matched on a specific (e.g. HPE) machine. It would
> only be needed on "broken" existing machines, and could be added
> post-4.10 to clean this up if you really want to do that.

Bjorn suggested similar approach (have a PNP quirk to fabricate a
PNP0C02 device and decleare all the required resources there) on
another thread. But as you said, this approach does not scale, it can
only applicable for a specific machine (by checking DMI information to
apply the PNP quirk).

>
> That's all very nasty...
>
> Jon.
>
> On 12/01/2016 11:08 PM, Jon Masters wrote:
>> Hi Bjorn, Duc, Mark,
>>
>> I switched my brain to the on mode and went and read some specs, and a few
>> tables, so here's my 2 cents on this...
>>
>> On 12/01/2016 06:22 PM, Duc Dang wrote:
>>> On Thu, Dec 1, 2016 at 3:07 PM, Bjorn Helgaas  wrote:
 On Thu, Dec 01, 2016 at 02:10:10PM -0800, Duc Dang wrote:
>>
>>> The SoC provide some number of RC bridges, each with a different base
>>> for some mmio registers. Even if segment is legitimate in MCFG, there
>>> is still a problem if a platform doesn't use the segment ordering
>>> implied by the code. But the PNP0A03 _CRS does have this base address
>>> as the first memory resource, so we could get it from there and not
>>> have hard-coded addresses and implied ording in the quirk code.
>>
>> I'm confused.  Doesn't the current code treat every item in PNP0A03
>> _CRS as a window?  Do you mean the first resource is handled
>> differently somehow?  The Consumer/Producer bit could allow us to do
>> this by marking the RC MMIO space as "Consumer", but I didn't think
>> that strategy was quite working yet.
>>
>> Let's see if I summarized this correctly...
>>
>> 1. The MMIO registers for the host bridge itself need to be described
>>somewhere, especially if we need to find those in a quirk and poke
>>them. Since those registers are very much part of the bridge device,
>>it makes sense for them to be in the _CRS for PNP0A08/PNP0A03.
>>
>> 2. The address space covering these registers MUST be described as a
>>ResourceConsumer in order to avoid accidentally exposing them as
>>available for use by downstream devices on the PCI bus.
>>
>> 3. The ACPI specification allows for resources of the type "Memory32Fixed".
>>This is a macro that doesn't have the notion of a producer or consumer.
>>HOWEVER various interpretations seem to be that this could/should
>>default to being interpreted as a consumed region.
>>
>> 4. At one point, a regression was added to the kernel:
>>
>>63f1789ec716 ("x86/PCI/ACPI: Ignore resources consumed by
>>host bridge itself")
>>
>>Which lead to a series on conversations about what should happen
>>for bridge 

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Jon Masters]

Bjorn,

Although I think the below still applies (that we need to leave that
Memory32Fixed for existing deployments, and this is going to result
in /proc/iomem polution), I've done some more reading of your ecam
tree and the implementation of acpi_get_rc_resources you mentioned,
and in particular how the PNP0C02 devices actually get wired up.

I would like to be able to boot upstream on existing shipping and
deployed machines that are in the field (not to mention our labs), but
there's no reason we can't *also* get APM to add a new vendor specific
PNP0C02 to the ACPI namespace in future firmware updates (for at least
their own Mustang reference boards) matching segment to CSR, as in the
case of the HiSi patches. That might then allow for some later
preference to use that for the CSR rather than getting it from the RC
device. Still, it would be ideal to boot on machines that are shipping
from HPE and others at this moment, so I am still hopeful you'll
at least allow the approach from Duc's v4 for now (4.10).

Another nasty option for later consideration could then be having
the kernel fake up any missing PNP0C02 on existing machines, but
it would need special knowledge of the platform to generate that
so as to handle the problem Mark flagged earlier (segment vs
controller mismatch on some platforms). That could be done with a
DMI quirk that matched on a specific (e.g. HPE) machine. It would
only be needed on "broken" existing machines, and could be added
post-4.10 to clean this up if you really want to do that.

That's all very nasty...

Jon.

On 12/01/2016 11:08 PM, Jon Masters wrote:
> Hi Bjorn, Duc, Mark,
> 
> I switched my brain to the on mode and went and read some specs, and a few
> tables, so here's my 2 cents on this...
> 
> On 12/01/2016 06:22 PM, Duc Dang wrote:
>> On Thu, Dec 1, 2016 at 3:07 PM, Bjorn Helgaas  wrote:
>>> On Thu, Dec 01, 2016 at 02:10:10PM -0800, Duc Dang wrote:
> 
>> The SoC provide some number of RC bridges, each with a different base
>> for some mmio registers. Even if segment is legitimate in MCFG, there
>> is still a problem if a platform doesn't use the segment ordering
>> implied by the code. But the PNP0A03 _CRS does have this base address
>> as the first memory resource, so we could get it from there and not
>> have hard-coded addresses and implied ording in the quirk code.
>
> I'm confused.  Doesn't the current code treat every item in PNP0A03
> _CRS as a window?  Do you mean the first resource is handled
> differently somehow?  The Consumer/Producer bit could allow us to do
> this by marking the RC MMIO space as "Consumer", but I didn't think
> that strategy was quite working yet.
> 
> Let's see if I summarized this correctly...
> 
> 1. The MMIO registers for the host bridge itself need to be described
>somewhere, especially if we need to find those in a quirk and poke
>them. Since those registers are very much part of the bridge device,
>it makes sense for them to be in the _CRS for PNP0A08/PNP0A03.
> 
> 2. The address space covering these registers MUST be described as a
>ResourceConsumer in order to avoid accidentally exposing them as
>available for use by downstream devices on the PCI bus.
> 
> 3. The ACPI specification allows for resources of the type "Memory32Fixed".
>This is a macro that doesn't have the notion of a producer or consumer.
>HOWEVER various interpretations seem to be that this could/should
>default to being interpreted as a consumed region.
> 
> 4. At one point, a regression was added to the kernel:
> 
>63f1789ec716 ("x86/PCI/ACPI: Ignore resources consumed by
>host bridge itself")
> 
>Which lead to a series on conversations about what should happen
>for bridge resources (e.g. https://lkml.org/lkml/2015/3/24/962 )
> 
> 5. This resulted in the following commit reverting point 4:
> 
>2c62e8492ed7 ("x86/PCI/ACPI: Make all resources except [io 0xcf8-0xcff]
>available on PCI bus")
> 
>Which also stated that:
> 
>"This solution will also ease the way to consolidate ACPI PCI host
> bridge common code from x86, ia64 and ARM64"
> 
> End of summary.
> 
> So it seems that generally there is an aversion to having bridge resources
> be described in this manner and you would like to require that they be
> described e.g. using QWordMemory with a ResourceConsumer type?
> 
> BUT if we were to do that, it would break existing shipping systems since
> there are quirks out there that use this form to find the base CSR:
> 
>if (acpi_res->type == ACPI_RESOURCE_TYPE_FIXED_MEMORY32) {
> fixed32 = _res->data.fixed_memory32;
> port->csr_base = ioremap(fixed32->address,
>  fixed32->address_length);
> return AE_CTRL_TERMINATE;
> }
> 
> That's what's shipping in at least RHEL(SA) today, and probably in other
> distros. So if we get vendors 

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Jon Masters]

Bjorn,

Although I think the below still applies (that we need to leave that
Memory32Fixed for existing deployments, and this is going to result
in /proc/iomem polution), I've done some more reading of your ecam
tree and the implementation of acpi_get_rc_resources you mentioned,
and in particular how the PNP0C02 devices actually get wired up.

I would like to be able to boot upstream on existing shipping and
deployed machines that are in the field (not to mention our labs), but
there's no reason we can't *also* get APM to add a new vendor specific
PNP0C02 to the ACPI namespace in future firmware updates (for at least
their own Mustang reference boards) matching segment to CSR, as in the
case of the HiSi patches. That might then allow for some later
preference to use that for the CSR rather than getting it from the RC
device. Still, it would be ideal to boot on machines that are shipping
from HPE and others at this moment, so I am still hopeful you'll
at least allow the approach from Duc's v4 for now (4.10).

Another nasty option for later consideration could then be having
the kernel fake up any missing PNP0C02 on existing machines, but
it would need special knowledge of the platform to generate that
so as to handle the problem Mark flagged earlier (segment vs
controller mismatch on some platforms). That could be done with a
DMI quirk that matched on a specific (e.g. HPE) machine. It would
only be needed on "broken" existing machines, and could be added
post-4.10 to clean this up if you really want to do that.

That's all very nasty...

Jon.

On 12/01/2016 11:08 PM, Jon Masters wrote:
> Hi Bjorn, Duc, Mark,
> 
> I switched my brain to the on mode and went and read some specs, and a few
> tables, so here's my 2 cents on this...
> 
> On 12/01/2016 06:22 PM, Duc Dang wrote:
>> On Thu, Dec 1, 2016 at 3:07 PM, Bjorn Helgaas  wrote:
>>> On Thu, Dec 01, 2016 at 02:10:10PM -0800, Duc Dang wrote:
> 
>> The SoC provide some number of RC bridges, each with a different base
>> for some mmio registers. Even if segment is legitimate in MCFG, there
>> is still a problem if a platform doesn't use the segment ordering
>> implied by the code. But the PNP0A03 _CRS does have this base address
>> as the first memory resource, so we could get it from there and not
>> have hard-coded addresses and implied ording in the quirk code.
>
> I'm confused.  Doesn't the current code treat every item in PNP0A03
> _CRS as a window?  Do you mean the first resource is handled
> differently somehow?  The Consumer/Producer bit could allow us to do
> this by marking the RC MMIO space as "Consumer", but I didn't think
> that strategy was quite working yet.
> 
> Let's see if I summarized this correctly...
> 
> 1. The MMIO registers for the host bridge itself need to be described
>somewhere, especially if we need to find those in a quirk and poke
>them. Since those registers are very much part of the bridge device,
>it makes sense for them to be in the _CRS for PNP0A08/PNP0A03.
> 
> 2. The address space covering these registers MUST be described as a
>ResourceConsumer in order to avoid accidentally exposing them as
>available for use by downstream devices on the PCI bus.
> 
> 3. The ACPI specification allows for resources of the type "Memory32Fixed".
>This is a macro that doesn't have the notion of a producer or consumer.
>HOWEVER various interpretations seem to be that this could/should
>default to being interpreted as a consumed region.
> 
> 4. At one point, a regression was added to the kernel:
> 
>63f1789ec716 ("x86/PCI/ACPI: Ignore resources consumed by
>host bridge itself")
> 
>Which lead to a series on conversations about what should happen
>for bridge resources (e.g. https://lkml.org/lkml/2015/3/24/962 )
> 
> 5. This resulted in the following commit reverting point 4:
> 
>2c62e8492ed7 ("x86/PCI/ACPI: Make all resources except [io 0xcf8-0xcff]
>available on PCI bus")
> 
>Which also stated that:
> 
>"This solution will also ease the way to consolidate ACPI PCI host
> bridge common code from x86, ia64 and ARM64"
> 
> End of summary.
> 
> So it seems that generally there is an aversion to having bridge resources
> be described in this manner and you would like to require that they be
> described e.g. using QWordMemory with a ResourceConsumer type?
> 
> BUT if we were to do that, it would break existing shipping systems since
> there are quirks out there that use this form to find the base CSR:
> 
>if (acpi_res->type == ACPI_RESOURCE_TYPE_FIXED_MEMORY32) {
> fixed32 = _res->data.fixed_memory32;
> port->csr_base = ioremap(fixed32->address,
>  fixed32->address_length);
> return AE_CTRL_TERMINATE;
> }
> 
> That's what's shipping in at least RHEL(SA) today, and probably in other
> distros. So if we get vendors to take that out, 

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Jon Masters]

Hi Bjorn, Duc, Mark,

I switched my brain to the on mode and went and read some specs, and a few
tables, so here's my 2 cents on this...

On 12/01/2016 06:22 PM, Duc Dang wrote:
> On Thu, Dec 1, 2016 at 3:07 PM, Bjorn Helgaas  wrote:
>> On Thu, Dec 01, 2016 at 02:10:10PM -0800, Duc Dang wrote:

> The SoC provide some number of RC bridges, each with a different base
> for some mmio registers. Even if segment is legitimate in MCFG, there
> is still a problem if a platform doesn't use the segment ordering
> implied by the code. But the PNP0A03 _CRS does have this base address
> as the first memory resource, so we could get it from there and not
> have hard-coded addresses and implied ording in the quirk code.

 I'm confused.  Doesn't the current code treat every item in PNP0A03
 _CRS as a window?  Do you mean the first resource is handled
 differently somehow?  The Consumer/Producer bit could allow us to do
 this by marking the RC MMIO space as "Consumer", but I didn't think
 that strategy was quite working yet.

Let's see if I summarized this correctly...

1. The MMIO registers for the host bridge itself need to be described
   somewhere, especially if we need to find those in a quirk and poke
   them. Since those registers are very much part of the bridge device,
   it makes sense for them to be in the _CRS for PNP0A08/PNP0A03.

2. The address space covering these registers MUST be described as a
   ResourceConsumer in order to avoid accidentally exposing them as
   available for use by downstream devices on the PCI bus.

3. The ACPI specification allows for resources of the type "Memory32Fixed".
   This is a macro that doesn't have the notion of a producer or consumer.
   HOWEVER various interpretations seem to be that this could/should
   default to being interpreted as a consumed region.

4. At one point, a regression was added to the kernel:

   63f1789ec716 ("x86/PCI/ACPI: Ignore resources consumed by
   host bridge itself")

   Which lead to a series on conversations about what should happen
   for bridge resources (e.g. https://lkml.org/lkml/2015/3/24/962 )

5. This resulted in the following commit reverting point 4:

   2c62e8492ed7 ("x86/PCI/ACPI: Make all resources except [io 0xcf8-0xcff]
   available on PCI bus")

   Which also stated that:

   "This solution will also ease the way to consolidate ACPI PCI host
bridge common code from x86, ia64 and ARM64"

End of summary.

So it seems that generally there is an aversion to having bridge resources
be described in this manner and you would like to require that they be
described e.g. using QWordMemory with a ResourceConsumer type?

BUT if we were to do that, it would break existing shipping systems since
there are quirks out there that use this form to find the base CSR:

   if (acpi_res->type == ACPI_RESOURCE_TYPE_FIXED_MEMORY32) {
fixed32 = _res->data.fixed_memory32;
port->csr_base = ioremap(fixed32->address,
 fixed32->address_length);
return AE_CTRL_TERMINATE;
}

That's what's shipping in at least RHEL(SA) today, and probably in other
distros. So if we get vendors to take that out, existing stuff will break,
which will have the downside that customers will have to choose between
whether to run a given distro or be able to use upstream kernels. In
that sense, to me, there are shipping platforms out there, which may well
be doing the "wrong" thing, but they are deployed and they are doing it.

Which makes me wonder a couple of things (I think should NOT be done):

1. What would happen if we had both. A FixedMemory32 and the same region
   described again using the longer form as a consumed region. I doubt
   that's legal, and the current code would still add the region if it
   saw the FixedMemory32 first when walking the tree. I don't like it,
   but I'm mentioning it in case that leads to some helpful thinking.

2. What would happen if we had a difference policy on arm64 for such
   resources. x86 has an "exception" for accessing the config space
   using IO port 0xCF8-0xCFF (a fairly reasonable exception!) and
   we can make the rules for a new platform (i.e. actually prescribe
   exactly what the behavior is, rather than have it not be defined).
   This is of course terrible in that existing BIOS vendors and so on
   won't necessarily know this when working on ARM ACPI later on.

I don't like either of these obviously. I'm hoping there's some way we
can say that this is tolerated in this one quirk (allow the use of
FixedMemory32 in this case) on the grounds that the driver claims
this bridge region and can be annotated to explain such.

Once you let us know what you prefer, we will go and update the ARM
SBBR to spell out that future platforms should not make this mistake
again. We can prescribe whatever you'd like in terms of how bridge
resources consumed by the bridge are 

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Jon Masters]

Hi Bjorn, Duc, Mark,

I switched my brain to the on mode and went and read some specs, and a few
tables, so here's my 2 cents on this...

On 12/01/2016 06:22 PM, Duc Dang wrote:
> On Thu, Dec 1, 2016 at 3:07 PM, Bjorn Helgaas  wrote:
>> On Thu, Dec 01, 2016 at 02:10:10PM -0800, Duc Dang wrote:

> The SoC provide some number of RC bridges, each with a different base
> for some mmio registers. Even if segment is legitimate in MCFG, there
> is still a problem if a platform doesn't use the segment ordering
> implied by the code. But the PNP0A03 _CRS does have this base address
> as the first memory resource, so we could get it from there and not
> have hard-coded addresses and implied ording in the quirk code.

 I'm confused.  Doesn't the current code treat every item in PNP0A03
 _CRS as a window?  Do you mean the first resource is handled
 differently somehow?  The Consumer/Producer bit could allow us to do
 this by marking the RC MMIO space as "Consumer", but I didn't think
 that strategy was quite working yet.

Let's see if I summarized this correctly...

1. The MMIO registers for the host bridge itself need to be described
   somewhere, especially if we need to find those in a quirk and poke
   them. Since those registers are very much part of the bridge device,
   it makes sense for them to be in the _CRS for PNP0A08/PNP0A03.

2. The address space covering these registers MUST be described as a
   ResourceConsumer in order to avoid accidentally exposing them as
   available for use by downstream devices on the PCI bus.

3. The ACPI specification allows for resources of the type "Memory32Fixed".
   This is a macro that doesn't have the notion of a producer or consumer.
   HOWEVER various interpretations seem to be that this could/should
   default to being interpreted as a consumed region.

4. At one point, a regression was added to the kernel:

   63f1789ec716 ("x86/PCI/ACPI: Ignore resources consumed by
   host bridge itself")

   Which lead to a series on conversations about what should happen
   for bridge resources (e.g. https://lkml.org/lkml/2015/3/24/962 )

5. This resulted in the following commit reverting point 4:

   2c62e8492ed7 ("x86/PCI/ACPI: Make all resources except [io 0xcf8-0xcff]
   available on PCI bus")

   Which also stated that:

   "This solution will also ease the way to consolidate ACPI PCI host
bridge common code from x86, ia64 and ARM64"

End of summary.

So it seems that generally there is an aversion to having bridge resources
be described in this manner and you would like to require that they be
described e.g. using QWordMemory with a ResourceConsumer type?

BUT if we were to do that, it would break existing shipping systems since
there are quirks out there that use this form to find the base CSR:

   if (acpi_res->type == ACPI_RESOURCE_TYPE_FIXED_MEMORY32) {
fixed32 = _res->data.fixed_memory32;
port->csr_base = ioremap(fixed32->address,
 fixed32->address_length);
return AE_CTRL_TERMINATE;
}

That's what's shipping in at least RHEL(SA) today, and probably in other
distros. So if we get vendors to take that out, existing stuff will break,
which will have the downside that customers will have to choose between
whether to run a given distro or be able to use upstream kernels. In
that sense, to me, there are shipping platforms out there, which may well
be doing the "wrong" thing, but they are deployed and they are doing it.

Which makes me wonder a couple of things (I think should NOT be done):

1. What would happen if we had both. A FixedMemory32 and the same region
   described again using the longer form as a consumed region. I doubt
   that's legal, and the current code would still add the region if it
   saw the FixedMemory32 first when walking the tree. I don't like it,
   but I'm mentioning it in case that leads to some helpful thinking.

2. What would happen if we had a difference policy on arm64 for such
   resources. x86 has an "exception" for accessing the config space
   using IO port 0xCF8-0xCFF (a fairly reasonable exception!) and
   we can make the rules for a new platform (i.e. actually prescribe
   exactly what the behavior is, rather than have it not be defined).
   This is of course terrible in that existing BIOS vendors and so on
   won't necessarily know this when working on ARM ACPI later on.

I don't like either of these obviously. I'm hoping there's some way we
can say that this is tolerated in this one quirk (allow the use of
FixedMemory32 in this case) on the grounds that the driver claims
this bridge region and can be annotated to explain such.

Once you let us know what you prefer, we will go and update the ARM
SBBR to spell out that future platforms should not make this mistake
again. We can prescribe whatever you'd like in terms of how bridge
resources consumed by the bridge are exposed. I have spoken 

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Duc Dang]

On Thu, Dec 1, 2016 at 10:33 AM, Bjorn Helgaas  wrote:
> Hi Duc,
>
> On Wed, Nov 30, 2016 at 03:42:53PM -0800, Duc Dang wrote:
>> PCIe controllers in X-Gene SoCs is not ECAM compliant: software
>> needs to configure additional controller's register to address
>> device at bus:dev:function.
>>
>> The quirk will only be applied for X-Gene PCIe MCFG table with
>> OEM revison 1, 2, 3 or 4 (PCIe controller v1 and v2 on X-Gene SoCs).
>>
>> The quirk declares the X-Gene PCIe controller register space as 64KB
>> fixed memory resource with name "PCIe CSR". This name will be showed
>> next to the resource range in the output of "cat /proc/iomem".
>>
>> Signed-off-by: Duc Dang 
>> ---
>> v3:
>>   - Rebase on top of pci/ecam-v6 tree.
>>   - Use DEFINE_RES_MEM_NAMED to declare controller register space
>>   with name "PCIe CSR"
>> v2:
>>   RFC v2: https://patchwork.ozlabs.org/patch/686846/
>> v1:
>>   RFC v1: https://patchwork.kernel.org/patch/9337115/
>>
>>  drivers/acpi/pci_mcfg.c  |  31 
>>  drivers/pci/host/pci-xgene.c | 165 
>> ++-
>>  include/linux/pci-ecam.h |   7 ++
>>  3 files changed, 200 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
>> index ac21db3..eb6125b 100644
>> --- a/drivers/acpi/pci_mcfg.c
>> +++ b/drivers/acpi/pci_mcfg.c
>> @@ -57,6 +57,37 @@ struct mcfg_fixup {
>>   { "QCOM  ", "QDF2432 ", 1, 5, MCFG_BUS_ANY, _32b_ops },
>>   { "QCOM  ", "QDF2432 ", 1, 6, MCFG_BUS_ANY, _32b_ops },
>>   { "QCOM  ", "QDF2432 ", 1, 7, MCFG_BUS_ANY, _32b_ops },
>> +
>> +#ifdef CONFIG_PCI_XGENE
>
> As you've no doubt noticed, I'm proposing to add these quirks without
> CONFIG_PCI_XGENE, so we don't have to select each device when building
> a generic ACPI kernel.
>
> I'm also proposing some Kconfig and Makefile changes so we don't build
> the platform driver part in a generic ACPI kernel (unless we *also*
> explicitly select the platform driver).
>
> Here's an example:
> https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=pci/ecam=f80edf4d6c05

I made similar changes in v4 patch. The ECAM quirk will be built when
ACPI and PCI_QUIRKS are enabled.

When building for DT only, the ECAM quirk won't be compiled.
>
>> +#define XGENE_V1_ECAM_MCFG(rev, seg) \
>> + {"APM   ", "XGENE   ", rev, seg, MCFG_BUS_ANY, \
>> + _v1_pcie_ecam_ops }
>> +#define XGENE_V2_1_ECAM_MCFG(rev, seg) \
>> + {"APM   ", "XGENE   ", rev, seg, MCFG_BUS_ANY, \
>> + _v2_1_pcie_ecam_ops }
>> +#define XGENE_V2_2_ECAM_MCFG(rev, seg) \
>> + {"APM   ", "XGENE   ", rev, seg, MCFG_BUS_ANY, \
>> + _v2_2_pcie_ecam_ops }
>> +
>> + /* X-Gene SoC with v1 PCIe controller */
>> + XGENE_V1_ECAM_MCFG(1, 0),
>> + XGENE_V1_ECAM_MCFG(1, 1),
>
>> @@ -64,6 +66,7 @@
>>  /* PCIe IP version */
>>  #define XGENE_PCIE_IP_VER_UNKN   0
>>  #define XGENE_PCIE_IP_VER_1  1
>> +#define XGENE_PCIE_IP_VER_2  2
>
> This isn't used anywhere, which makes me wonder whether it's worth
> keeping it.

V2 controller will use this XGENE_PCIE_IP_VER_2 (port->version =
XGENE_PCIE_IP_VER_2). This will be used to indicate that the
controller is V2, and to enable configuration request retry status
feature (by not disable it like V1 controller).

>
>>  static void __iomem *xgene_pcie_get_cfg_base(struct pci_bus *bus)
>>  {
>> - struct xgene_pcie_port *port = bus->sysdata;
>> + struct pci_config_window *cfg;
>> + struct xgene_pcie_port *port;
>> +
>> + if (acpi_disabled)
>> + port = bus->sysdata;
>> + else {
>> + cfg = bus->sysdata;
>> + port = cfg->priv;
>> + }
>
> I would really, really like to figure out a way to get rid of these
> "if (acpi_disabled)" checks sprinkled through here.  Is there any way
> we can set up bus->sysdata to be the same, regardless of whether we're
> using this as a platform driver or an ACPI quirk?

Right now, I created a inline function to extract xgene_pcie_port from
pci_bus. In order to get rid of acpi_disabled, I will need to make
sysdata in DT case also point to pci_config_window structure, which
means I will need to convert and test the DT driver to use ecam ops.
It is a separate patch itself. So I think I should do it at later time
(after this ECAM quirk patch). I hope you are ok with this.

>
>> +#ifdef CONFIG_ACPI
>
> You've probably noticed that I've been using
>
>   #if defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS)
>
> in this situation, mostly to make it clear that this is part of a
> workaround.  I don't want people to blindly copy this stuff without
> realizing that it's a workaround for a hardware issue.

Yes, I used defined(CONFIG_PCI_QUIRKS) in v4 patch as well.
>
>> +static struct resource xgene_v1_csr_res[] = {
>> + [0] = DEFINE_RES_MEM_NAMED(0x1f2bUL, SZ_64K, "PCIe CSR"),
>> + [1] = DEFINE_RES_MEM_NAMED(0x1f2cUL, 

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Duc Dang]

On Thu, Dec 1, 2016 at 10:33 AM, Bjorn Helgaas  wrote:
> Hi Duc,
>
> On Wed, Nov 30, 2016 at 03:42:53PM -0800, Duc Dang wrote:
>> PCIe controllers in X-Gene SoCs is not ECAM compliant: software
>> needs to configure additional controller's register to address
>> device at bus:dev:function.
>>
>> The quirk will only be applied for X-Gene PCIe MCFG table with
>> OEM revison 1, 2, 3 or 4 (PCIe controller v1 and v2 on X-Gene SoCs).
>>
>> The quirk declares the X-Gene PCIe controller register space as 64KB
>> fixed memory resource with name "PCIe CSR". This name will be showed
>> next to the resource range in the output of "cat /proc/iomem".
>>
>> Signed-off-by: Duc Dang 
>> ---
>> v3:
>>   - Rebase on top of pci/ecam-v6 tree.
>>   - Use DEFINE_RES_MEM_NAMED to declare controller register space
>>   with name "PCIe CSR"
>> v2:
>>   RFC v2: https://patchwork.ozlabs.org/patch/686846/
>> v1:
>>   RFC v1: https://patchwork.kernel.org/patch/9337115/
>>
>>  drivers/acpi/pci_mcfg.c  |  31 
>>  drivers/pci/host/pci-xgene.c | 165 
>> ++-
>>  include/linux/pci-ecam.h |   7 ++
>>  3 files changed, 200 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
>> index ac21db3..eb6125b 100644
>> --- a/drivers/acpi/pci_mcfg.c
>> +++ b/drivers/acpi/pci_mcfg.c
>> @@ -57,6 +57,37 @@ struct mcfg_fixup {
>>   { "QCOM  ", "QDF2432 ", 1, 5, MCFG_BUS_ANY, _32b_ops },
>>   { "QCOM  ", "QDF2432 ", 1, 6, MCFG_BUS_ANY, _32b_ops },
>>   { "QCOM  ", "QDF2432 ", 1, 7, MCFG_BUS_ANY, _32b_ops },
>> +
>> +#ifdef CONFIG_PCI_XGENE
>
> As you've no doubt noticed, I'm proposing to add these quirks without
> CONFIG_PCI_XGENE, so we don't have to select each device when building
> a generic ACPI kernel.
>
> I'm also proposing some Kconfig and Makefile changes so we don't build
> the platform driver part in a generic ACPI kernel (unless we *also*
> explicitly select the platform driver).
>
> Here's an example:
> https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=pci/ecam=f80edf4d6c05

I made similar changes in v4 patch. The ECAM quirk will be built when
ACPI and PCI_QUIRKS are enabled.

When building for DT only, the ECAM quirk won't be compiled.
>
>> +#define XGENE_V1_ECAM_MCFG(rev, seg) \
>> + {"APM   ", "XGENE   ", rev, seg, MCFG_BUS_ANY, \
>> + _v1_pcie_ecam_ops }
>> +#define XGENE_V2_1_ECAM_MCFG(rev, seg) \
>> + {"APM   ", "XGENE   ", rev, seg, MCFG_BUS_ANY, \
>> + _v2_1_pcie_ecam_ops }
>> +#define XGENE_V2_2_ECAM_MCFG(rev, seg) \
>> + {"APM   ", "XGENE   ", rev, seg, MCFG_BUS_ANY, \
>> + _v2_2_pcie_ecam_ops }
>> +
>> + /* X-Gene SoC with v1 PCIe controller */
>> + XGENE_V1_ECAM_MCFG(1, 0),
>> + XGENE_V1_ECAM_MCFG(1, 1),
>
>> @@ -64,6 +66,7 @@
>>  /* PCIe IP version */
>>  #define XGENE_PCIE_IP_VER_UNKN   0
>>  #define XGENE_PCIE_IP_VER_1  1
>> +#define XGENE_PCIE_IP_VER_2  2
>
> This isn't used anywhere, which makes me wonder whether it's worth
> keeping it.

V2 controller will use this XGENE_PCIE_IP_VER_2 (port->version =
XGENE_PCIE_IP_VER_2). This will be used to indicate that the
controller is V2, and to enable configuration request retry status
feature (by not disable it like V1 controller).

>
>>  static void __iomem *xgene_pcie_get_cfg_base(struct pci_bus *bus)
>>  {
>> - struct xgene_pcie_port *port = bus->sysdata;
>> + struct pci_config_window *cfg;
>> + struct xgene_pcie_port *port;
>> +
>> + if (acpi_disabled)
>> + port = bus->sysdata;
>> + else {
>> + cfg = bus->sysdata;
>> + port = cfg->priv;
>> + }
>
> I would really, really like to figure out a way to get rid of these
> "if (acpi_disabled)" checks sprinkled through here.  Is there any way
> we can set up bus->sysdata to be the same, regardless of whether we're
> using this as a platform driver or an ACPI quirk?

Right now, I created a inline function to extract xgene_pcie_port from
pci_bus. In order to get rid of acpi_disabled, I will need to make
sysdata in DT case also point to pci_config_window structure, which
means I will need to convert and test the DT driver to use ecam ops.
It is a separate patch itself. So I think I should do it at later time
(after this ECAM quirk patch). I hope you are ok with this.

>
>> +#ifdef CONFIG_ACPI
>
> You've probably noticed that I've been using
>
>   #if defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS)
>
> in this situation, mostly to make it clear that this is part of a
> workaround.  I don't want people to blindly copy this stuff without
> realizing that it's a workaround for a hardware issue.

Yes, I used defined(CONFIG_PCI_QUIRKS) in v4 patch as well.
>
>> +static struct resource xgene_v1_csr_res[] = {
>> + [0] = DEFINE_RES_MEM_NAMED(0x1f2bUL, SZ_64K, "PCIe CSR"),
>> + [1] = DEFINE_RES_MEM_NAMED(0x1f2cUL, SZ_64K, "PCIe CSR"),
>> + [2] = 

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Duc Dang]

On Thu, Dec 1, 2016 at 3:07 PM, Bjorn Helgaas  wrote:
> On Thu, Dec 01, 2016 at 02:10:10PM -0800, Duc Dang wrote:
>> On Thu, Dec 1, 2016 at 11:41 AM, Bjorn Helgaas  wrote:
>> > On Thu, Dec 01, 2016 at 02:20:53PM -0500, Mark Salter wrote:
>> >> On Thu, 2016-12-01 at 12:33 -0600, Bjorn Helgaas wrote:
>> >> > On Wed, Nov 30, 2016 at 03:42:53PM -0800, Duc Dang wrote:
>> >
>> >> > > +static struct resource xgene_v1_csr_res[] = {
>> >> > > + [0] = DEFINE_RES_MEM_NAMED(0x1f2bUL, SZ_64K, "PCIe CSR"),
>> >> > > + [1] = DEFINE_RES_MEM_NAMED(0x1f2cUL, SZ_64K, "PCIe CSR"),
>> >> > > + [2] = DEFINE_RES_MEM_NAMED(0x1f2dUL, SZ_64K, "PCIe CSR"),
>> >> > > + [3] = DEFINE_RES_MEM_NAMED(0x1f50UL, SZ_64K, "PCIe CSR"),
>> >> > > + [4] = DEFINE_RES_MEM_NAMED(0x1f51UL, SZ_64K, "PCIe CSR"),
>> >> > I assume these ranges are not the actual ECAM space, right?
>> >> > If they *were* ECAM, I assume you would have included them in the
>> >> > quirk itself in the mcfg_quirks[] table.
>> >>
>> >> These are base addresses for some RC mmio registers.
>> >> >
>> >> > >
>> >> > > +static int xgene_v1_pcie_ecam_init(struct pci_config_window *cfg)
>> >> > > +{
>> >> > > + struct acpi_device *adev = to_acpi_device(cfg->parent);
>> >> > > + struct acpi_pci_root *root = acpi_driver_data(adev);
>> >> > > + struct device *dev = cfg->parent;
>> >> > > + struct xgene_pcie_port *port;
>> >> > > + struct resource *csr;
>> >> > > +
>> >> > > + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
>> >> > > + if (!port)
>> >> > > + return -ENOMEM;
>> >> > > +
>> >> > > + csr = _v1_csr_res[root->segment];
>> >> > This makes me nervous because root->segment comes from the ACPI _SEG,
>> >> > and if firmware gives us junk in _SEG, we will reference something in
>> >> > the weeds.
>> >>
>> >> The SoC provide some number of RC bridges, each with a different base
>> >> for some mmio registers. Even if segment is legitimate in MCFG, there
>> >> is still a problem if a platform doesn't use the segment ordering
>> >> implied by the code. But the PNP0A03 _CRS does have this base address
>> >> as the first memory resource, so we could get it from there and not
>> >> have hard-coded addresses and implied ording in the quirk code.
>> >
>> > I'm confused.  Doesn't the current code treat every item in PNP0A03
>> > _CRS as a window?  Do you mean the first resource is handled
>> > differently somehow?  The Consumer/Producer bit could allow us to do
>> > this by marking the RC MMIO space as "Consumer", but I didn't think
>> > that strategy was quite working yet.
>>
>> The first resource is defined like below. It was introduced long time
>> ago to use with older version of X-Gene ECAM quirks.
>> Memory32Fixed(ReadWrite, 0x1F2B, 0x1, )
>
>> [0.822990] pci_bus :00: root bus resource [mem 0x1f2b-0x1f2b]
>
> I think this is wrong.  The PCI core thinks [mem 0x1f2b-0x1f2b]
> is available for use by devices on bus :00, but I think you're
> saying it is consumed by the bridge itself, not forwarded down to PCI.
>
> What's in your /proc/iomem?  I see that your quirks do call
> devm_ioremap_resource(), which calls devm_request_mem_region()
> internally, so the driver does at least request that region, which
> should keep us from assigning it to PCI devices.
>
> But it still isn't quite right to tell the PCI core that the region is
> available on the root bus.

This is /proc/iomem output on my Mustang board. The 64K "PCIe CSR"
region is consumed completely.
1f2b-1f2b : PCI Bus :00
  1f2b-1f2b : PCIe CSR

e04000-e07fff : PCI Bus :00
  e04000-e0401f : PCI Bus :01
e04000-e0400f : :01:00.0
  e04000-e0400f : mlx4_core
e04010-e0401f : :01:00.0
e0d000-e0dfff : PCI ECAM
f0-ff : PCI Bus :00
  f0-f001ff : PCI Bus :01
f0-f001ff : :01:00.0
  f0-f001ff : mlx4_core

Using hard-coded resources for mmio space make the quirk rely on the
segment number passing from the firmware. Using Mark's method or
acpi_get_rc_resource can discover the mmio space and consume all of
the space, but as you mentioned, it leaves the defect that PCI core
considers the mmio space as available resource for secondary devices
although it will never allocate the mmio space to secondary devices as
the RC already reserves and consumes all of the space.

>
> Bjorn
>
Regards,
Duc Dang.

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Duc Dang]

On Thu, Dec 1, 2016 at 3:07 PM, Bjorn Helgaas  wrote:
> On Thu, Dec 01, 2016 at 02:10:10PM -0800, Duc Dang wrote:
>> On Thu, Dec 1, 2016 at 11:41 AM, Bjorn Helgaas  wrote:
>> > On Thu, Dec 01, 2016 at 02:20:53PM -0500, Mark Salter wrote:
>> >> On Thu, 2016-12-01 at 12:33 -0600, Bjorn Helgaas wrote:
>> >> > On Wed, Nov 30, 2016 at 03:42:53PM -0800, Duc Dang wrote:
>> >
>> >> > > +static struct resource xgene_v1_csr_res[] = {
>> >> > > + [0] = DEFINE_RES_MEM_NAMED(0x1f2bUL, SZ_64K, "PCIe CSR"),
>> >> > > + [1] = DEFINE_RES_MEM_NAMED(0x1f2cUL, SZ_64K, "PCIe CSR"),
>> >> > > + [2] = DEFINE_RES_MEM_NAMED(0x1f2dUL, SZ_64K, "PCIe CSR"),
>> >> > > + [3] = DEFINE_RES_MEM_NAMED(0x1f50UL, SZ_64K, "PCIe CSR"),
>> >> > > + [4] = DEFINE_RES_MEM_NAMED(0x1f51UL, SZ_64K, "PCIe CSR"),
>> >> > I assume these ranges are not the actual ECAM space, right?
>> >> > If they *were* ECAM, I assume you would have included them in the
>> >> > quirk itself in the mcfg_quirks[] table.
>> >>
>> >> These are base addresses for some RC mmio registers.
>> >> >
>> >> > >
>> >> > > +static int xgene_v1_pcie_ecam_init(struct pci_config_window *cfg)
>> >> > > +{
>> >> > > + struct acpi_device *adev = to_acpi_device(cfg->parent);
>> >> > > + struct acpi_pci_root *root = acpi_driver_data(adev);
>> >> > > + struct device *dev = cfg->parent;
>> >> > > + struct xgene_pcie_port *port;
>> >> > > + struct resource *csr;
>> >> > > +
>> >> > > + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
>> >> > > + if (!port)
>> >> > > + return -ENOMEM;
>> >> > > +
>> >> > > + csr = _v1_csr_res[root->segment];
>> >> > This makes me nervous because root->segment comes from the ACPI _SEG,
>> >> > and if firmware gives us junk in _SEG, we will reference something in
>> >> > the weeds.
>> >>
>> >> The SoC provide some number of RC bridges, each with a different base
>> >> for some mmio registers. Even if segment is legitimate in MCFG, there
>> >> is still a problem if a platform doesn't use the segment ordering
>> >> implied by the code. But the PNP0A03 _CRS does have this base address
>> >> as the first memory resource, so we could get it from there and not
>> >> have hard-coded addresses and implied ording in the quirk code.
>> >
>> > I'm confused.  Doesn't the current code treat every item in PNP0A03
>> > _CRS as a window?  Do you mean the first resource is handled
>> > differently somehow?  The Consumer/Producer bit could allow us to do
>> > this by marking the RC MMIO space as "Consumer", but I didn't think
>> > that strategy was quite working yet.
>>
>> The first resource is defined like below. It was introduced long time
>> ago to use with older version of X-Gene ECAM quirks.
>> Memory32Fixed(ReadWrite, 0x1F2B, 0x1, )
>
>> [0.822990] pci_bus :00: root bus resource [mem 0x1f2b-0x1f2b]
>
> I think this is wrong.  The PCI core thinks [mem 0x1f2b-0x1f2b]
> is available for use by devices on bus :00, but I think you're
> saying it is consumed by the bridge itself, not forwarded down to PCI.
>
> What's in your /proc/iomem?  I see that your quirks do call
> devm_ioremap_resource(), which calls devm_request_mem_region()
> internally, so the driver does at least request that region, which
> should keep us from assigning it to PCI devices.
>
> But it still isn't quite right to tell the PCI core that the region is
> available on the root bus.

This is /proc/iomem output on my Mustang board. The 64K "PCIe CSR"
region is consumed completely.
1f2b-1f2b : PCI Bus :00
  1f2b-1f2b : PCIe CSR

e04000-e07fff : PCI Bus :00
  e04000-e0401f : PCI Bus :01
e04000-e0400f : :01:00.0
  e04000-e0400f : mlx4_core
e04010-e0401f : :01:00.0
e0d000-e0dfff : PCI ECAM
f0-ff : PCI Bus :00
  f0-f001ff : PCI Bus :01
f0-f001ff : :01:00.0
  f0-f001ff : mlx4_core

Using hard-coded resources for mmio space make the quirk rely on the
segment number passing from the firmware. Using Mark's method or
acpi_get_rc_resource can discover the mmio space and consume all of
the space, but as you mentioned, it leaves the defect that PCI core
considers the mmio space as available resource for secondary devices
although it will never allocate the mmio space to secondary devices as
the RC already reserves and consumes all of the space.

>
> Bjorn
>
Regards,
Duc Dang.

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Bjorn Helgaas]

On Thu, Dec 01, 2016 at 02:10:10PM -0800, Duc Dang wrote:
> On Thu, Dec 1, 2016 at 11:41 AM, Bjorn Helgaas  wrote:
> > On Thu, Dec 01, 2016 at 02:20:53PM -0500, Mark Salter wrote:
> >> On Thu, 2016-12-01 at 12:33 -0600, Bjorn Helgaas wrote:
> >> > On Wed, Nov 30, 2016 at 03:42:53PM -0800, Duc Dang wrote:
> >
> >> > > +static struct resource xgene_v1_csr_res[] = {
> >> > > + [0] = DEFINE_RES_MEM_NAMED(0x1f2bUL, SZ_64K, "PCIe CSR"),
> >> > > + [1] = DEFINE_RES_MEM_NAMED(0x1f2cUL, SZ_64K, "PCIe CSR"),
> >> > > + [2] = DEFINE_RES_MEM_NAMED(0x1f2dUL, SZ_64K, "PCIe CSR"),
> >> > > + [3] = DEFINE_RES_MEM_NAMED(0x1f50UL, SZ_64K, "PCIe CSR"),
> >> > > + [4] = DEFINE_RES_MEM_NAMED(0x1f51UL, SZ_64K, "PCIe CSR"),
> >> > I assume these ranges are not the actual ECAM space, right?
> >> > If they *were* ECAM, I assume you would have included them in the
> >> > quirk itself in the mcfg_quirks[] table.
> >>
> >> These are base addresses for some RC mmio registers.
> >> >
> >> > >
> >> > > +static int xgene_v1_pcie_ecam_init(struct pci_config_window *cfg)
> >> > > +{
> >> > > + struct acpi_device *adev = to_acpi_device(cfg->parent);
> >> > > + struct acpi_pci_root *root = acpi_driver_data(adev);
> >> > > + struct device *dev = cfg->parent;
> >> > > + struct xgene_pcie_port *port;
> >> > > + struct resource *csr;
> >> > > +
> >> > > + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> >> > > + if (!port)
> >> > > + return -ENOMEM;
> >> > > +
> >> > > + csr = _v1_csr_res[root->segment];
> >> > This makes me nervous because root->segment comes from the ACPI _SEG,
> >> > and if firmware gives us junk in _SEG, we will reference something in
> >> > the weeds.
> >>
> >> The SoC provide some number of RC bridges, each with a different base
> >> for some mmio registers. Even if segment is legitimate in MCFG, there
> >> is still a problem if a platform doesn't use the segment ordering
> >> implied by the code. But the PNP0A03 _CRS does have this base address
> >> as the first memory resource, so we could get it from there and not
> >> have hard-coded addresses and implied ording in the quirk code.
> >
> > I'm confused.  Doesn't the current code treat every item in PNP0A03
> > _CRS as a window?  Do you mean the first resource is handled
> > differently somehow?  The Consumer/Producer bit could allow us to do
> > this by marking the RC MMIO space as "Consumer", but I didn't think
> > that strategy was quite working yet.
> 
> The first resource is defined like below. It was introduced long time
> ago to use with older version of X-Gene ECAM quirks.
> Memory32Fixed(ReadWrite, 0x1F2B, 0x1, )

> [0.822990] pci_bus :00: root bus resource [mem 0x1f2b-0x1f2b]

I think this is wrong.  The PCI core thinks [mem 0x1f2b-0x1f2b]
is available for use by devices on bus :00, but I think you're
saying it is consumed by the bridge itself, not forwarded down to PCI.

What's in your /proc/iomem?  I see that your quirks do call
devm_ioremap_resource(), which calls devm_request_mem_region()
internally, so the driver does at least request that region, which
should keep us from assigning it to PCI devices.

But it still isn't quite right to tell the PCI core that the region is
available on the root bus.

Bjorn

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Bjorn Helgaas]

On Thu, Dec 01, 2016 at 02:10:10PM -0800, Duc Dang wrote:
> On Thu, Dec 1, 2016 at 11:41 AM, Bjorn Helgaas  wrote:
> > On Thu, Dec 01, 2016 at 02:20:53PM -0500, Mark Salter wrote:
> >> On Thu, 2016-12-01 at 12:33 -0600, Bjorn Helgaas wrote:
> >> > On Wed, Nov 30, 2016 at 03:42:53PM -0800, Duc Dang wrote:
> >
> >> > > +static struct resource xgene_v1_csr_res[] = {
> >> > > + [0] = DEFINE_RES_MEM_NAMED(0x1f2bUL, SZ_64K, "PCIe CSR"),
> >> > > + [1] = DEFINE_RES_MEM_NAMED(0x1f2cUL, SZ_64K, "PCIe CSR"),
> >> > > + [2] = DEFINE_RES_MEM_NAMED(0x1f2dUL, SZ_64K, "PCIe CSR"),
> >> > > + [3] = DEFINE_RES_MEM_NAMED(0x1f50UL, SZ_64K, "PCIe CSR"),
> >> > > + [4] = DEFINE_RES_MEM_NAMED(0x1f51UL, SZ_64K, "PCIe CSR"),
> >> > I assume these ranges are not the actual ECAM space, right?
> >> > If they *were* ECAM, I assume you would have included them in the
> >> > quirk itself in the mcfg_quirks[] table.
> >>
> >> These are base addresses for some RC mmio registers.
> >> >
> >> > >
> >> > > +static int xgene_v1_pcie_ecam_init(struct pci_config_window *cfg)
> >> > > +{
> >> > > + struct acpi_device *adev = to_acpi_device(cfg->parent);
> >> > > + struct acpi_pci_root *root = acpi_driver_data(adev);
> >> > > + struct device *dev = cfg->parent;
> >> > > + struct xgene_pcie_port *port;
> >> > > + struct resource *csr;
> >> > > +
> >> > > + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> >> > > + if (!port)
> >> > > + return -ENOMEM;
> >> > > +
> >> > > + csr = _v1_csr_res[root->segment];
> >> > This makes me nervous because root->segment comes from the ACPI _SEG,
> >> > and if firmware gives us junk in _SEG, we will reference something in
> >> > the weeds.
> >>
> >> The SoC provide some number of RC bridges, each with a different base
> >> for some mmio registers. Even if segment is legitimate in MCFG, there
> >> is still a problem if a platform doesn't use the segment ordering
> >> implied by the code. But the PNP0A03 _CRS does have this base address
> >> as the first memory resource, so we could get it from there and not
> >> have hard-coded addresses and implied ording in the quirk code.
> >
> > I'm confused.  Doesn't the current code treat every item in PNP0A03
> > _CRS as a window?  Do you mean the first resource is handled
> > differently somehow?  The Consumer/Producer bit could allow us to do
> > this by marking the RC MMIO space as "Consumer", but I didn't think
> > that strategy was quite working yet.
> 
> The first resource is defined like below. It was introduced long time
> ago to use with older version of X-Gene ECAM quirks.
> Memory32Fixed(ReadWrite, 0x1F2B, 0x1, )

> [0.822990] pci_bus :00: root bus resource [mem 0x1f2b-0x1f2b]

I think this is wrong.  The PCI core thinks [mem 0x1f2b-0x1f2b]
is available for use by devices on bus :00, but I think you're
saying it is consumed by the bridge itself, not forwarded down to PCI.

What's in your /proc/iomem?  I see that your quirks do call
devm_ioremap_resource(), which calls devm_request_mem_region()
internally, so the driver does at least request that region, which
should keep us from assigning it to PCI devices.

But it still isn't quite right to tell the PCI core that the region is
available on the root bus.

Bjorn

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Jon Masters]

Thanks Duc! I will test quickly if you have it today :)

-- 
Computer Architect | Sent from my 64-bit #ARM Powered phone

> On Dec 1, 2016, at 17:10, Duc Dang  wrote:
> 
>> On Thu, Dec 1, 2016 at 11:41 AM, Bjorn Helgaas  wrote:
>>> On Thu, Dec 01, 2016 at 02:20:53PM -0500, Mark Salter wrote:
 On Thu, 2016-12-01 at 12:33 -0600, Bjorn Helgaas wrote:
 On Wed, Nov 30, 2016 at 03:42:53PM -0800, Duc Dang wrote:
>> 
> +static struct resource xgene_v1_csr_res[] = {
> + [0] = DEFINE_RES_MEM_NAMED(0x1f2bUL, SZ_64K, "PCIe CSR"),
> + [1] = DEFINE_RES_MEM_NAMED(0x1f2cUL, SZ_64K, "PCIe CSR"),
> + [2] = DEFINE_RES_MEM_NAMED(0x1f2dUL, SZ_64K, "PCIe CSR"),
> + [3] = DEFINE_RES_MEM_NAMED(0x1f50UL, SZ_64K, "PCIe CSR"),
> + [4] = DEFINE_RES_MEM_NAMED(0x1f51UL, SZ_64K, "PCIe CSR"),
 I assume these ranges are not the actual ECAM space, right?
 If they *were* ECAM, I assume you would have included them in the
 quirk itself in the mcfg_quirks[] table.
>>> 
>>> These are base addresses for some RC mmio registers.
 
> 
> +static int xgene_v1_pcie_ecam_init(struct pci_config_window *cfg)
> +{
> + struct acpi_device *adev = to_acpi_device(cfg->parent);
> + struct acpi_pci_root *root = acpi_driver_data(adev);
> + struct device *dev = cfg->parent;
> + struct xgene_pcie_port *port;
> + struct resource *csr;
> +
> + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> + if (!port)
> + return -ENOMEM;
> +
> + csr = _v1_csr_res[root->segment];
 This makes me nervous because root->segment comes from the ACPI _SEG,
 and if firmware gives us junk in _SEG, we will reference something in
 the weeds.
>>> 
>>> The SoC provide some number of RC bridges, each with a different base
>>> for some mmio registers. Even if segment is legitimate in MCFG, there
>>> is still a problem if a platform doesn't use the segment ordering
>>> implied by the code. But the PNP0A03 _CRS does have this base address
>>> as the first memory resource, so we could get it from there and not
>>> have hard-coded addresses and implied ording in the quirk code.
>> 
>> I'm confused.  Doesn't the current code treat every item in PNP0A03
>> _CRS as a window?  Do you mean the first resource is handled
>> differently somehow?  The Consumer/Producer bit could allow us to do
>> this by marking the RC MMIO space as "Consumer", but I didn't think
>> that strategy was quite working yet.
> 
> The first resource is defined like below. It was introduced long time
> ago to use with older version of X-Gene ECAM quirks.
> Memory32Fixed(ReadWrite, 0x1F2B, 0x1, )
> 
> The resource discovered during booting will be like following:
> [0.728117] ACPI: MCFG table detected, 1 entries
> [0.735330] ACPI: Power Resource [SCVR] (on)
> [0.767478] ACPI: PCI Root Bridge [PCI0] (domain  [bus 00-ff])
> [0.774013] acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM
> ClockPM Segments MSI]
> [0.782864] acpi PNP0A08:00: _OSC: OS now controls [PCIeHotplug PME
> AER PCIeCapability]
> [0.791331] acpi PNP0A08:00: MCFG quirk: ECAM at [mem
> 0xe0d000-0xe0dfff] for [bus 00-ff] with xgene_v1_pcie_ecam_ops
> [0.803207] acpi PNP0A08:00: ECAM at [mem
> 0xe0d000-0xe0dfff] for [bus 00-ff]
> [0.811399] Remapped I/O 0x00e01000 to [io  0x-0x window]
> [0.818678] PCI host bridge to bus :00
> [0.822990] pci_bus :00: root bus resource [mem 0x1f2b-0x1f2b]
> [0.830257] pci_bus :00: root bus resource [io  0x-0x
> window] (bus address [0x1000-0x1000])
> [0.840917] pci_bus :00: root bus resource [mem
> 0xe04000-0xe07fff window] (bus address
> [0x4000-0x7fff])
> [0.852675] pci_bus :00: root bus resource [mem
> 0xf0-0xff window]
> [0.860950] pci_bus :00: root bus resource [bus 00-ff]
> [0.866761] pci :00:00.0: [10e8:e004] type 01 class 0x060400
> [0.873175] pci :00:00.0: supports D1 D2
> [0.877980] pci :01:00.0: [15b3:1003] type 00 class 0x02
> [0.884597] pci :01:00.0: reg 0x10: [mem 0xe04000-0xe0400f 
> 64bit]
> [0.892337] pci :01:00.0: reg 0x18: [mem
> 0xe04200-0xe043ff 64bit pref]
> [0.900694] pci :01:00.0: reg 0x30: [mem 0xfff0-0x pref]
> [0.923853] pci_bus :00: on NUMA node 0
> [0.928269] pci :00:00.0: BAR 15: assigned [mem
> 0xf0-0xf001ff 64bit pref]
> [0.936908] pci :00:00.0: BAR 14: assigned [mem
> 0xe04000-0xe0401f]
> [0.944539] pci :01:00.0: BAR 2: assigned [mem
> 0xf0-0xf001ff 64bit pref]
> [0.953210] pci :01:00.0: BAR 0: assigned [mem
> 0xe04000-0xe0400f 64bit]
> [0.961430] pci :01:00.0: BAR 6: assigned [mem
> 0xe04010-0xe0401f pref]
> [0.969438] pci :00:00.0: PCI bridge to [bus 01]
> 

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Jon Masters]

Thanks Duc! I will test quickly if you have it today :)

-- 
Computer Architect | Sent from my 64-bit #ARM Powered phone

> On Dec 1, 2016, at 17:10, Duc Dang  wrote:
> 
>> On Thu, Dec 1, 2016 at 11:41 AM, Bjorn Helgaas  wrote:
>>> On Thu, Dec 01, 2016 at 02:20:53PM -0500, Mark Salter wrote:
 On Thu, 2016-12-01 at 12:33 -0600, Bjorn Helgaas wrote:
 On Wed, Nov 30, 2016 at 03:42:53PM -0800, Duc Dang wrote:
>> 
> +static struct resource xgene_v1_csr_res[] = {
> + [0] = DEFINE_RES_MEM_NAMED(0x1f2bUL, SZ_64K, "PCIe CSR"),
> + [1] = DEFINE_RES_MEM_NAMED(0x1f2cUL, SZ_64K, "PCIe CSR"),
> + [2] = DEFINE_RES_MEM_NAMED(0x1f2dUL, SZ_64K, "PCIe CSR"),
> + [3] = DEFINE_RES_MEM_NAMED(0x1f50UL, SZ_64K, "PCIe CSR"),
> + [4] = DEFINE_RES_MEM_NAMED(0x1f51UL, SZ_64K, "PCIe CSR"),
 I assume these ranges are not the actual ECAM space, right?
 If they *were* ECAM, I assume you would have included them in the
 quirk itself in the mcfg_quirks[] table.
>>> 
>>> These are base addresses for some RC mmio registers.
 
> 
> +static int xgene_v1_pcie_ecam_init(struct pci_config_window *cfg)
> +{
> + struct acpi_device *adev = to_acpi_device(cfg->parent);
> + struct acpi_pci_root *root = acpi_driver_data(adev);
> + struct device *dev = cfg->parent;
> + struct xgene_pcie_port *port;
> + struct resource *csr;
> +
> + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> + if (!port)
> + return -ENOMEM;
> +
> + csr = _v1_csr_res[root->segment];
 This makes me nervous because root->segment comes from the ACPI _SEG,
 and if firmware gives us junk in _SEG, we will reference something in
 the weeds.
>>> 
>>> The SoC provide some number of RC bridges, each with a different base
>>> for some mmio registers. Even if segment is legitimate in MCFG, there
>>> is still a problem if a platform doesn't use the segment ordering
>>> implied by the code. But the PNP0A03 _CRS does have this base address
>>> as the first memory resource, so we could get it from there and not
>>> have hard-coded addresses and implied ording in the quirk code.
>> 
>> I'm confused.  Doesn't the current code treat every item in PNP0A03
>> _CRS as a window?  Do you mean the first resource is handled
>> differently somehow?  The Consumer/Producer bit could allow us to do
>> this by marking the RC MMIO space as "Consumer", but I didn't think
>> that strategy was quite working yet.
> 
> The first resource is defined like below. It was introduced long time
> ago to use with older version of X-Gene ECAM quirks.
> Memory32Fixed(ReadWrite, 0x1F2B, 0x1, )
> 
> The resource discovered during booting will be like following:
> [0.728117] ACPI: MCFG table detected, 1 entries
> [0.735330] ACPI: Power Resource [SCVR] (on)
> [0.767478] ACPI: PCI Root Bridge [PCI0] (domain  [bus 00-ff])
> [0.774013] acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM
> ClockPM Segments MSI]
> [0.782864] acpi PNP0A08:00: _OSC: OS now controls [PCIeHotplug PME
> AER PCIeCapability]
> [0.791331] acpi PNP0A08:00: MCFG quirk: ECAM at [mem
> 0xe0d000-0xe0dfff] for [bus 00-ff] with xgene_v1_pcie_ecam_ops
> [0.803207] acpi PNP0A08:00: ECAM at [mem
> 0xe0d000-0xe0dfff] for [bus 00-ff]
> [0.811399] Remapped I/O 0x00e01000 to [io  0x-0x window]
> [0.818678] PCI host bridge to bus :00
> [0.822990] pci_bus :00: root bus resource [mem 0x1f2b-0x1f2b]
> [0.830257] pci_bus :00: root bus resource [io  0x-0x
> window] (bus address [0x1000-0x1000])
> [0.840917] pci_bus :00: root bus resource [mem
> 0xe04000-0xe07fff window] (bus address
> [0x4000-0x7fff])
> [0.852675] pci_bus :00: root bus resource [mem
> 0xf0-0xff window]
> [0.860950] pci_bus :00: root bus resource [bus 00-ff]
> [0.866761] pci :00:00.0: [10e8:e004] type 01 class 0x060400
> [0.873175] pci :00:00.0: supports D1 D2
> [0.877980] pci :01:00.0: [15b3:1003] type 00 class 0x02
> [0.884597] pci :01:00.0: reg 0x10: [mem 0xe04000-0xe0400f 
> 64bit]
> [0.892337] pci :01:00.0: reg 0x18: [mem
> 0xe04200-0xe043ff 64bit pref]
> [0.900694] pci :01:00.0: reg 0x30: [mem 0xfff0-0x pref]
> [0.923853] pci_bus :00: on NUMA node 0
> [0.928269] pci :00:00.0: BAR 15: assigned [mem
> 0xf0-0xf001ff 64bit pref]
> [0.936908] pci :00:00.0: BAR 14: assigned [mem
> 0xe04000-0xe0401f]
> [0.944539] pci :01:00.0: BAR 2: assigned [mem
> 0xf0-0xf001ff 64bit pref]
> [0.953210] pci :01:00.0: BAR 0: assigned [mem
> 0xe04000-0xe0400f 64bit]
> [0.961430] pci :01:00.0: BAR 6: assigned [mem
> 0xe04010-0xe0401f pref]
> [0.969438] pci :00:00.0: PCI bridge to [bus 01]
> [0.974690] pci :00:00.0:   

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Duc Dang]

On Thu, Dec 1, 2016 at 11:41 AM, Bjorn Helgaas  wrote:
> On Thu, Dec 01, 2016 at 02:20:53PM -0500, Mark Salter wrote:
>> On Thu, 2016-12-01 at 12:33 -0600, Bjorn Helgaas wrote:
>> > On Wed, Nov 30, 2016 at 03:42:53PM -0800, Duc Dang wrote:
>
>> > > +static struct resource xgene_v1_csr_res[] = {
>> > > + [0] = DEFINE_RES_MEM_NAMED(0x1f2bUL, SZ_64K, "PCIe CSR"),
>> > > + [1] = DEFINE_RES_MEM_NAMED(0x1f2cUL, SZ_64K, "PCIe CSR"),
>> > > + [2] = DEFINE_RES_MEM_NAMED(0x1f2dUL, SZ_64K, "PCIe CSR"),
>> > > + [3] = DEFINE_RES_MEM_NAMED(0x1f50UL, SZ_64K, "PCIe CSR"),
>> > > + [4] = DEFINE_RES_MEM_NAMED(0x1f51UL, SZ_64K, "PCIe CSR"),
>> > I assume these ranges are not the actual ECAM space, right?
>> > If they *were* ECAM, I assume you would have included them in the
>> > quirk itself in the mcfg_quirks[] table.
>>
>> These are base addresses for some RC mmio registers.
>> >
>> > >
>> > > +static int xgene_v1_pcie_ecam_init(struct pci_config_window *cfg)
>> > > +{
>> > > + struct acpi_device *adev = to_acpi_device(cfg->parent);
>> > > + struct acpi_pci_root *root = acpi_driver_data(adev);
>> > > + struct device *dev = cfg->parent;
>> > > + struct xgene_pcie_port *port;
>> > > + struct resource *csr;
>> > > +
>> > > + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
>> > > + if (!port)
>> > > + return -ENOMEM;
>> > > +
>> > > + csr = _v1_csr_res[root->segment];
>> > This makes me nervous because root->segment comes from the ACPI _SEG,
>> > and if firmware gives us junk in _SEG, we will reference something in
>> > the weeds.
>>
>> The SoC provide some number of RC bridges, each with a different base
>> for some mmio registers. Even if segment is legitimate in MCFG, there
>> is still a problem if a platform doesn't use the segment ordering
>> implied by the code. But the PNP0A03 _CRS does have this base address
>> as the first memory resource, so we could get it from there and not
>> have hard-coded addresses and implied ording in the quirk code.
>
> I'm confused.  Doesn't the current code treat every item in PNP0A03
> _CRS as a window?  Do you mean the first resource is handled
> differently somehow?  The Consumer/Producer bit could allow us to do
> this by marking the RC MMIO space as "Consumer", but I didn't think
> that strategy was quite working yet.

The first resource is defined like below. It was introduced long time
ago to use with older version of X-Gene ECAM quirks.
Memory32Fixed(ReadWrite, 0x1F2B, 0x1, )

The resource discovered during booting will be like following:
[0.728117] ACPI: MCFG table detected, 1 entries
[0.735330] ACPI: Power Resource [SCVR] (on)
[0.767478] ACPI: PCI Root Bridge [PCI0] (domain  [bus 00-ff])
[0.774013] acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM
ClockPM Segments MSI]
[0.782864] acpi PNP0A08:00: _OSC: OS now controls [PCIeHotplug PME
AER PCIeCapability]
[0.791331] acpi PNP0A08:00: MCFG quirk: ECAM at [mem
0xe0d000-0xe0dfff] for [bus 00-ff] with xgene_v1_pcie_ecam_ops
[0.803207] acpi PNP0A08:00: ECAM at [mem
0xe0d000-0xe0dfff] for [bus 00-ff]
[0.811399] Remapped I/O 0x00e01000 to [io  0x-0x window]
[0.818678] PCI host bridge to bus :00
[0.822990] pci_bus :00: root bus resource [mem 0x1f2b-0x1f2b]
[0.830257] pci_bus :00: root bus resource [io  0x-0x
window] (bus address [0x1000-0x1000])
[0.840917] pci_bus :00: root bus resource [mem
0xe04000-0xe07fff window] (bus address
[0x4000-0x7fff])
[0.852675] pci_bus :00: root bus resource [mem
0xf0-0xff window]
[0.860950] pci_bus :00: root bus resource [bus 00-ff]
[0.866761] pci :00:00.0: [10e8:e004] type 01 class 0x060400
[0.873175] pci :00:00.0: supports D1 D2
[0.877980] pci :01:00.0: [15b3:1003] type 00 class 0x02
[0.884597] pci :01:00.0: reg 0x10: [mem 0xe04000-0xe0400f 64bit]
[0.892337] pci :01:00.0: reg 0x18: [mem
0xe04200-0xe043ff 64bit pref]
[0.900694] pci :01:00.0: reg 0x30: [mem 0xfff0-0x pref]
[0.923853] pci_bus :00: on NUMA node 0
[0.928269] pci :00:00.0: BAR 15: assigned [mem
0xf0-0xf001ff 64bit pref]
[0.936908] pci :00:00.0: BAR 14: assigned [mem
0xe04000-0xe0401f]
[0.944539] pci :01:00.0: BAR 2: assigned [mem
0xf0-0xf001ff 64bit pref]
[0.953210] pci :01:00.0: BAR 0: assigned [mem
0xe04000-0xe0400f 64bit]
[0.961430] pci :01:00.0: BAR 6: assigned [mem
0xe04010-0xe0401f pref]
[0.969438] pci :00:00.0: PCI bridge to [bus 01]
[0.974690] pci :00:00.0:   bridge window [mem 0xe04000-0xe0401f]
[0.982231] pci :00:00.0:   bridge window [mem
0xf0-0xf001ff 64bit pref]

>
>> I have tested a modified version of these quirks using this to
>> get the CSR base and it works on the 3 different 

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Duc Dang]

On Thu, Dec 1, 2016 at 11:41 AM, Bjorn Helgaas  wrote:
> On Thu, Dec 01, 2016 at 02:20:53PM -0500, Mark Salter wrote:
>> On Thu, 2016-12-01 at 12:33 -0600, Bjorn Helgaas wrote:
>> > On Wed, Nov 30, 2016 at 03:42:53PM -0800, Duc Dang wrote:
>
>> > > +static struct resource xgene_v1_csr_res[] = {
>> > > + [0] = DEFINE_RES_MEM_NAMED(0x1f2bUL, SZ_64K, "PCIe CSR"),
>> > > + [1] = DEFINE_RES_MEM_NAMED(0x1f2cUL, SZ_64K, "PCIe CSR"),
>> > > + [2] = DEFINE_RES_MEM_NAMED(0x1f2dUL, SZ_64K, "PCIe CSR"),
>> > > + [3] = DEFINE_RES_MEM_NAMED(0x1f50UL, SZ_64K, "PCIe CSR"),
>> > > + [4] = DEFINE_RES_MEM_NAMED(0x1f51UL, SZ_64K, "PCIe CSR"),
>> > I assume these ranges are not the actual ECAM space, right?
>> > If they *were* ECAM, I assume you would have included them in the
>> > quirk itself in the mcfg_quirks[] table.
>>
>> These are base addresses for some RC mmio registers.
>> >
>> > >
>> > > +static int xgene_v1_pcie_ecam_init(struct pci_config_window *cfg)
>> > > +{
>> > > + struct acpi_device *adev = to_acpi_device(cfg->parent);
>> > > + struct acpi_pci_root *root = acpi_driver_data(adev);
>> > > + struct device *dev = cfg->parent;
>> > > + struct xgene_pcie_port *port;
>> > > + struct resource *csr;
>> > > +
>> > > + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
>> > > + if (!port)
>> > > + return -ENOMEM;
>> > > +
>> > > + csr = _v1_csr_res[root->segment];
>> > This makes me nervous because root->segment comes from the ACPI _SEG,
>> > and if firmware gives us junk in _SEG, we will reference something in
>> > the weeds.
>>
>> The SoC provide some number of RC bridges, each with a different base
>> for some mmio registers. Even if segment is legitimate in MCFG, there
>> is still a problem if a platform doesn't use the segment ordering
>> implied by the code. But the PNP0A03 _CRS does have this base address
>> as the first memory resource, so we could get it from there and not
>> have hard-coded addresses and implied ording in the quirk code.
>
> I'm confused.  Doesn't the current code treat every item in PNP0A03
> _CRS as a window?  Do you mean the first resource is handled
> differently somehow?  The Consumer/Producer bit could allow us to do
> this by marking the RC MMIO space as "Consumer", but I didn't think
> that strategy was quite working yet.

The first resource is defined like below. It was introduced long time
ago to use with older version of X-Gene ECAM quirks.
Memory32Fixed(ReadWrite, 0x1F2B, 0x1, )

The resource discovered during booting will be like following:
[0.728117] ACPI: MCFG table detected, 1 entries
[0.735330] ACPI: Power Resource [SCVR] (on)
[0.767478] ACPI: PCI Root Bridge [PCI0] (domain  [bus 00-ff])
[0.774013] acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM
ClockPM Segments MSI]
[0.782864] acpi PNP0A08:00: _OSC: OS now controls [PCIeHotplug PME
AER PCIeCapability]
[0.791331] acpi PNP0A08:00: MCFG quirk: ECAM at [mem
0xe0d000-0xe0dfff] for [bus 00-ff] with xgene_v1_pcie_ecam_ops
[0.803207] acpi PNP0A08:00: ECAM at [mem
0xe0d000-0xe0dfff] for [bus 00-ff]
[0.811399] Remapped I/O 0x00e01000 to [io  0x-0x window]
[0.818678] PCI host bridge to bus :00
[0.822990] pci_bus :00: root bus resource [mem 0x1f2b-0x1f2b]
[0.830257] pci_bus :00: root bus resource [io  0x-0x
window] (bus address [0x1000-0x1000])
[0.840917] pci_bus :00: root bus resource [mem
0xe04000-0xe07fff window] (bus address
[0x4000-0x7fff])
[0.852675] pci_bus :00: root bus resource [mem
0xf0-0xff window]
[0.860950] pci_bus :00: root bus resource [bus 00-ff]
[0.866761] pci :00:00.0: [10e8:e004] type 01 class 0x060400
[0.873175] pci :00:00.0: supports D1 D2
[0.877980] pci :01:00.0: [15b3:1003] type 00 class 0x02
[0.884597] pci :01:00.0: reg 0x10: [mem 0xe04000-0xe0400f 64bit]
[0.892337] pci :01:00.0: reg 0x18: [mem
0xe04200-0xe043ff 64bit pref]
[0.900694] pci :01:00.0: reg 0x30: [mem 0xfff0-0x pref]
[0.923853] pci_bus :00: on NUMA node 0
[0.928269] pci :00:00.0: BAR 15: assigned [mem
0xf0-0xf001ff 64bit pref]
[0.936908] pci :00:00.0: BAR 14: assigned [mem
0xe04000-0xe0401f]
[0.944539] pci :01:00.0: BAR 2: assigned [mem
0xf0-0xf001ff 64bit pref]
[0.953210] pci :01:00.0: BAR 0: assigned [mem
0xe04000-0xe0400f 64bit]
[0.961430] pci :01:00.0: BAR 6: assigned [mem
0xe04010-0xe0401f pref]
[0.969438] pci :00:00.0: PCI bridge to [bus 01]
[0.974690] pci :00:00.0:   bridge window [mem 0xe04000-0xe0401f]
[0.982231] pci :00:00.0:   bridge window [mem
0xf0-0xf001ff 64bit pref]

>
>> I have tested a modified version of these quirks using this to
>> get the CSR base and it works on the 3 different platforms I have
>> 

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Duc Dang]

On Thu, Dec 1, 2016 at 11:17 AM, Jon Masters  wrote:
> On 12/01/2016 10:08 AM, Mark Salter wrote:
>> On Wed, 2016-11-30 at 15:42 -0800, Duc Dang wrote:
>
>>> +static int xgene_v1_pcie_ecam_init(struct pci_config_window *cfg)
>>> +{
>>> +struct acpi_device *adev = to_acpi_device(cfg->parent);
>>> +struct acpi_pci_root *root = acpi_driver_data(adev);
>>> +struct device *dev = cfg->parent;
>>> +struct xgene_pcie_port *port;
>>> +struct resource *csr;
>>> +
>>> +port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
>>> +if (!port)
>>> +return -ENOMEM;
>>> +
>>> +csr = _v1_csr_res[root->segment];
>>
>> This hard-coded assumption that segment N uses controller N breaks
>> for m400 where segment 0 is using controller 3.

I think the latest firmware released from us a few months back use
segment 3 for PCIe controller 3 in MCFG table.
>
> This seems very fragile. So in addition to Bjorn's comment about not
> trusting firmware provided data for the segment offset in the CSR list,
> you will want to also determine the controller from the ACPI tree. The
> existing code walks the ACPI hierarchy and finds the CSR that way.
> Obviously, the goal is to avoid that in the latest incarnation, but you
> could still determine which controller matches a given device.

Yes, I will look into that more.

>
> Jon.
>
> --
> Computer Architect | Sent from my Fedora powered laptop
>
Regards,
Duc Dang.

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Duc Dang]

On Thu, Dec 1, 2016 at 11:17 AM, Jon Masters  wrote:
> On 12/01/2016 10:08 AM, Mark Salter wrote:
>> On Wed, 2016-11-30 at 15:42 -0800, Duc Dang wrote:
>
>>> +static int xgene_v1_pcie_ecam_init(struct pci_config_window *cfg)
>>> +{
>>> +struct acpi_device *adev = to_acpi_device(cfg->parent);
>>> +struct acpi_pci_root *root = acpi_driver_data(adev);
>>> +struct device *dev = cfg->parent;
>>> +struct xgene_pcie_port *port;
>>> +struct resource *csr;
>>> +
>>> +port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
>>> +if (!port)
>>> +return -ENOMEM;
>>> +
>>> +csr = _v1_csr_res[root->segment];
>>
>> This hard-coded assumption that segment N uses controller N breaks
>> for m400 where segment 0 is using controller 3.

I think the latest firmware released from us a few months back use
segment 3 for PCIe controller 3 in MCFG table.
>
> This seems very fragile. So in addition to Bjorn's comment about not
> trusting firmware provided data for the segment offset in the CSR list,
> you will want to also determine the controller from the ACPI tree. The
> existing code walks the ACPI hierarchy and finds the CSR that way.
> Obviously, the goal is to avoid that in the latest incarnation, but you
> could still determine which controller matches a given device.

Yes, I will look into that more.

>
> Jon.
>
> --
> Computer Architect | Sent from my Fedora powered laptop
>
Regards,
Duc Dang.

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Bjorn Helgaas]

On Thu, Dec 01, 2016 at 02:20:53PM -0500, Mark Salter wrote:
> On Thu, 2016-12-01 at 12:33 -0600, Bjorn Helgaas wrote:
> > On Wed, Nov 30, 2016 at 03:42:53PM -0800, Duc Dang wrote:

> > > +static struct resource xgene_v1_csr_res[] = {
> > > + [0] = DEFINE_RES_MEM_NAMED(0x1f2bUL, SZ_64K, "PCIe CSR"),
> > > + [1] = DEFINE_RES_MEM_NAMED(0x1f2cUL, SZ_64K, "PCIe CSR"),
> > > + [2] = DEFINE_RES_MEM_NAMED(0x1f2dUL, SZ_64K, "PCIe CSR"),
> > > + [3] = DEFINE_RES_MEM_NAMED(0x1f50UL, SZ_64K, "PCIe CSR"),
> > > + [4] = DEFINE_RES_MEM_NAMED(0x1f51UL, SZ_64K, "PCIe CSR"),
> > I assume these ranges are not the actual ECAM space, right?
> > If they *were* ECAM, I assume you would have included them in the
> > quirk itself in the mcfg_quirks[] table.
> 
> These are base addresses for some RC mmio registers.
> > 
> > > 
> > > +static int xgene_v1_pcie_ecam_init(struct pci_config_window *cfg)
> > > +{
> > > + struct acpi_device *adev = to_acpi_device(cfg->parent);
> > > + struct acpi_pci_root *root = acpi_driver_data(adev);
> > > + struct device *dev = cfg->parent;
> > > + struct xgene_pcie_port *port;
> > > + struct resource *csr;
> > > +
> > > + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> > > + if (!port)
> > > + return -ENOMEM;
> > > +
> > > + csr = _v1_csr_res[root->segment];
> > This makes me nervous because root->segment comes from the ACPI _SEG,
> > and if firmware gives us junk in _SEG, we will reference something in
> > the weeds.
> 
> The SoC provide some number of RC bridges, each with a different base
> for some mmio registers. Even if segment is legitimate in MCFG, there
> is still a problem if a platform doesn't use the segment ordering
> implied by the code. But the PNP0A03 _CRS does have this base address
> as the first memory resource, so we could get it from there and not
> have hard-coded addresses and implied ording in the quirk code.

I'm confused.  Doesn't the current code treat every item in PNP0A03
_CRS as a window?  Do you mean the first resource is handled
differently somehow?  The Consumer/Producer bit could allow us to do
this by marking the RC MMIO space as "Consumer", but I didn't think
that strategy was quite working yet.

> I have tested a modified version of these quirks using this to
> get the CSR base and it works on the 3 different platforms I have
> access to.
> 
> static int xgene_pcie_get_csr(struct device *dev, struct resource *r)
> {
>   struct acpi_device *adev = to_acpi_device(dev);
>   unsigned long flags;
>   struct list_head list;
>   struct resource_entry *entry;
>   int ret;
> 
>   INIT_LIST_HEAD();
>   flags = IORESOURCE_MEM;
>   ret = acpi_dev_get_resources(adev, ,
>    acpi_dev_filter_resource_type_cb,
>    (void *)flags);
>   if (ret < 0) {
>   dev_err(dev, "failed to parse _CRS, error: %d\n", ret);
>   return ret;
>   } else if (ret == 0) {
>   dev_err(dev, "no memory resources present in _CRS\n");
>   return -EINVAL;
>   }
> 
>   entry = list_first_entry(, struct resource_entry, node);
>   *r = *entry->res;
>   acpi_dev_free_resource_list();
>   return 0;
> }

The code above is identical to acpi_get_rc_addr(), which is used in
the acpi_get_rc_resources() path by the other quirks.  Can you use
that path, too, instead of reimplementing it here?

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Bjorn Helgaas]

On Thu, Dec 01, 2016 at 02:20:53PM -0500, Mark Salter wrote:
> On Thu, 2016-12-01 at 12:33 -0600, Bjorn Helgaas wrote:
> > On Wed, Nov 30, 2016 at 03:42:53PM -0800, Duc Dang wrote:

> > > +static struct resource xgene_v1_csr_res[] = {
> > > + [0] = DEFINE_RES_MEM_NAMED(0x1f2bUL, SZ_64K, "PCIe CSR"),
> > > + [1] = DEFINE_RES_MEM_NAMED(0x1f2cUL, SZ_64K, "PCIe CSR"),
> > > + [2] = DEFINE_RES_MEM_NAMED(0x1f2dUL, SZ_64K, "PCIe CSR"),
> > > + [3] = DEFINE_RES_MEM_NAMED(0x1f50UL, SZ_64K, "PCIe CSR"),
> > > + [4] = DEFINE_RES_MEM_NAMED(0x1f51UL, SZ_64K, "PCIe CSR"),
> > I assume these ranges are not the actual ECAM space, right?
> > If they *were* ECAM, I assume you would have included them in the
> > quirk itself in the mcfg_quirks[] table.
> 
> These are base addresses for some RC mmio registers.
> > 
> > > 
> > > +static int xgene_v1_pcie_ecam_init(struct pci_config_window *cfg)
> > > +{
> > > + struct acpi_device *adev = to_acpi_device(cfg->parent);
> > > + struct acpi_pci_root *root = acpi_driver_data(adev);
> > > + struct device *dev = cfg->parent;
> > > + struct xgene_pcie_port *port;
> > > + struct resource *csr;
> > > +
> > > + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> > > + if (!port)
> > > + return -ENOMEM;
> > > +
> > > + csr = _v1_csr_res[root->segment];
> > This makes me nervous because root->segment comes from the ACPI _SEG,
> > and if firmware gives us junk in _SEG, we will reference something in
> > the weeds.
> 
> The SoC provide some number of RC bridges, each with a different base
> for some mmio registers. Even if segment is legitimate in MCFG, there
> is still a problem if a platform doesn't use the segment ordering
> implied by the code. But the PNP0A03 _CRS does have this base address
> as the first memory resource, so we could get it from there and not
> have hard-coded addresses and implied ording in the quirk code.

I'm confused.  Doesn't the current code treat every item in PNP0A03
_CRS as a window?  Do you mean the first resource is handled
differently somehow?  The Consumer/Producer bit could allow us to do
this by marking the RC MMIO space as "Consumer", but I didn't think
that strategy was quite working yet.

> I have tested a modified version of these quirks using this to
> get the CSR base and it works on the 3 different platforms I have
> access to.
> 
> static int xgene_pcie_get_csr(struct device *dev, struct resource *r)
> {
>   struct acpi_device *adev = to_acpi_device(dev);
>   unsigned long flags;
>   struct list_head list;
>   struct resource_entry *entry;
>   int ret;
> 
>   INIT_LIST_HEAD();
>   flags = IORESOURCE_MEM;
>   ret = acpi_dev_get_resources(adev, ,
>    acpi_dev_filter_resource_type_cb,
>    (void *)flags);
>   if (ret < 0) {
>   dev_err(dev, "failed to parse _CRS, error: %d\n", ret);
>   return ret;
>   } else if (ret == 0) {
>   dev_err(dev, "no memory resources present in _CRS\n");
>   return -EINVAL;
>   }
> 
>   entry = list_first_entry(, struct resource_entry, node);
>   *r = *entry->res;
>   acpi_dev_free_resource_list();
>   return 0;
> }

The code above is identical to acpi_get_rc_addr(), which is used in
the acpi_get_rc_resources() path by the other quirks.  Can you use
that path, too, instead of reimplementing it here?

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Jon Masters]

On 12/01/2016 02:20 PM, Mark Salter wrote:
> On Thu, 2016-12-01 at 12:33 -0600, Bjorn Helgaas wrote:

>>> +   csr = _v1_csr_res[root->segment];
>> This makes me nervous because root->segment comes from the ACPI _SEG,
>> and if firmware gives us junk in _SEG, we will reference something in
>> the weeds.
> 
> The SoC provide some number of RC bridges, each with a different base
> for some mmio registers. Even if segment is legitimate in MCFG, there
> is still a problem if a platform doesn't use the segment ordering
> implied by the code. But the PNP0A03 _CRS does have this base address
> as the first memory resource, so we could get it from there and not
> have hard-coded addresses and implied ording in the quirk code.
> 
> I have tested a modified version of these quirks using this to
> get the CSR base and it works on the 3 different platforms I have
> access to.
> 
> static int xgene_pcie_get_csr(struct device *dev, struct resource *r)
> {
>   struct acpi_device *adev = to_acpi_device(dev);
>   unsigned long flags;
>   struct list_head list;
>   struct resource_entry *entry;
>   int ret;
> 
>   INIT_LIST_HEAD();
>   flags = IORESOURCE_MEM;
>   ret = acpi_dev_get_resources(adev, ,
>acpi_dev_filter_resource_type_cb,
>(void *)flags);
>   if (ret < 0) {
>   dev_err(dev, "failed to parse _CRS, error: %d\n", ret);
>   return ret;
>   } else if (ret == 0) {
>   dev_err(dev, "no memory resources present in _CRS\n");
>   return -EINVAL;
>   }
> 
>   entry = list_first_entry(, struct resource_entry, node);
>   *r = *entry->res;
>   acpi_dev_free_resource_list();
>   return 0;
> }

This seems a lot safer. At some point trusting firmware to provide the
correct _CRS for the RC in use is better than hard coding for every
possible implementation configuration of an X-Gene SoC.

Jon.

-- 
Computer Architect | Sent from my Fedora powered laptop

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Jon Masters]

On 12/01/2016 02:20 PM, Mark Salter wrote:
> On Thu, 2016-12-01 at 12:33 -0600, Bjorn Helgaas wrote:

>>> +   csr = _v1_csr_res[root->segment];
>> This makes me nervous because root->segment comes from the ACPI _SEG,
>> and if firmware gives us junk in _SEG, we will reference something in
>> the weeds.
> 
> The SoC provide some number of RC bridges, each with a different base
> for some mmio registers. Even if segment is legitimate in MCFG, there
> is still a problem if a platform doesn't use the segment ordering
> implied by the code. But the PNP0A03 _CRS does have this base address
> as the first memory resource, so we could get it from there and not
> have hard-coded addresses and implied ording in the quirk code.
> 
> I have tested a modified version of these quirks using this to
> get the CSR base and it works on the 3 different platforms I have
> access to.
> 
> static int xgene_pcie_get_csr(struct device *dev, struct resource *r)
> {
>   struct acpi_device *adev = to_acpi_device(dev);
>   unsigned long flags;
>   struct list_head list;
>   struct resource_entry *entry;
>   int ret;
> 
>   INIT_LIST_HEAD();
>   flags = IORESOURCE_MEM;
>   ret = acpi_dev_get_resources(adev, ,
>acpi_dev_filter_resource_type_cb,
>(void *)flags);
>   if (ret < 0) {
>   dev_err(dev, "failed to parse _CRS, error: %d\n", ret);
>   return ret;
>   } else if (ret == 0) {
>   dev_err(dev, "no memory resources present in _CRS\n");
>   return -EINVAL;
>   }
> 
>   entry = list_first_entry(, struct resource_entry, node);
>   *r = *entry->res;
>   acpi_dev_free_resource_list();
>   return 0;
> }

This seems a lot safer. At some point trusting firmware to provide the
correct _CRS for the RC in use is better than hard coding for every
possible implementation configuration of an X-Gene SoC.

Jon.

-- 
Computer Architect | Sent from my Fedora powered laptop

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Mark Salter]

On Thu, 2016-12-01 at 12:33 -0600, Bjorn Helgaas wrote:
> Hi Duc,
> 
> On Wed, Nov 30, 2016 at 03:42:53PM -0800, Duc Dang wrote:
> > 
> > PCIe controllers in X-Gene SoCs is not ECAM compliant: software
> > needs to configure additional controller's register to address
> > device at bus:dev:function.
> > 
> > The quirk will only be applied for X-Gene PCIe MCFG table with
> > OEM revison 1, 2, 3 or 4 (PCIe controller v1 and v2 on X-Gene SoCs).
> > 
> > The quirk declares the X-Gene PCIe controller register space as 64KB
> > fixed memory resource with name "PCIe CSR". This name will be showed
> > next to the resource range in the output of "cat /proc/iomem".
> > 
> > Signed-off-by: Duc Dang 
> > ---
> > v3:
> >   - Rebase on top of pci/ecam-v6 tree.
> >   - Use DEFINE_RES_MEM_NAMED to declare controller register space
> >   with name "PCIe CSR"
> > v2:
> >   RFC v2: https://patchwork.ozlabs.org/patch/686846/
> > v1:
> >   RFC v1: https://patchwork.kernel.org/patch/9337115/
> > 
> >  drivers/acpi/pci_mcfg.c  |  31 
> >  drivers/pci/host/pci-xgene.c | 165 
> > ++-
> >  include/linux/pci-ecam.h |   7 ++
> >  3 files changed, 200 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> > index ac21db3..eb6125b 100644
> > --- a/drivers/acpi/pci_mcfg.c
> > +++ b/drivers/acpi/pci_mcfg.c
> > @@ -57,6 +57,37 @@ struct mcfg_fixup {
> >     { "QCOM  ", "QDF2432 ", 1, 5, MCFG_BUS_ANY, _32b_ops },
> >     { "QCOM  ", "QDF2432 ", 1, 6, MCFG_BUS_ANY, _32b_ops },
> >     { "QCOM  ", "QDF2432 ", 1, 7, MCFG_BUS_ANY, _32b_ops },
> > +
> > +#ifdef CONFIG_PCI_XGENE
> As you've no doubt noticed, I'm proposing to add these quirks without
> CONFIG_PCI_XGENE, so we don't have to select each device when building
> a generic ACPI kernel.
> 
> I'm also proposing some Kconfig and Makefile changes so we don't build
> the platform driver part in a generic ACPI kernel (unless we *also*
> explicitly select the platform driver).
> 
> Here's an example:
> https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=pci/ecam=f80edf4d6c05
> 
> > 
> > +#define XGENE_V1_ECAM_MCFG(rev, seg) \
> > +   {"APM   ", "XGENE   ", rev, seg, MCFG_BUS_ANY, \
> > +   _v1_pcie_ecam_ops }
> > +#define XGENE_V2_1_ECAM_MCFG(rev, seg) \
> > +   {"APM   ", "XGENE   ", rev, seg, MCFG_BUS_ANY, \
> > +   _v2_1_pcie_ecam_ops }
> > +#define XGENE_V2_2_ECAM_MCFG(rev, seg) \
> > +   {"APM   ", "XGENE   ", rev, seg, MCFG_BUS_ANY, \
> > +   _v2_2_pcie_ecam_ops }
> > +
> > +   /* X-Gene SoC with v1 PCIe controller */
> > +   XGENE_V1_ECAM_MCFG(1, 0),
> > +   XGENE_V1_ECAM_MCFG(1, 1),
> > 
> > @@ -64,6 +66,7 @@
> >  /* PCIe IP version */
> >  #define XGENE_PCIE_IP_VER_UNKN 0
> >  #define XGENE_PCIE_IP_VER_11
> > +#define XGENE_PCIE_IP_VER_22
> This isn't used anywhere, which makes me wonder whether it's worth
> keeping it.
> 
> > 
> >  static void __iomem *xgene_pcie_get_cfg_base(struct pci_bus *bus)
> >  {
> > -   struct xgene_pcie_port *port = bus->sysdata;
> > +   struct pci_config_window *cfg;
> > +   struct xgene_pcie_port *port;
> > +
> > +   if (acpi_disabled)
> > +   port = bus->sysdata;
> > +   else {
> > +   cfg = bus->sysdata;
> > +   port = cfg->priv;
> > +   }
> I would really, really like to figure out a way to get rid of these
> "if (acpi_disabled)" checks sprinkled through here.  Is there any way
> we can set up bus->sysdata to be the same, regardless of whether we're
> using this as a platform driver or an ACPI quirk?
> 
> > 
> > +#ifdef CONFIG_ACPI
> You've probably noticed that I've been using
> 
>   #if defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS)
> 
> in this situation, mostly to make it clear that this is part of a
> workaround.  I don't want people to blindly copy this stuff without
> realizing that it's a workaround for a hardware issue.
> 
> > 
> > +static struct resource xgene_v1_csr_res[] = {
> > +   [0] = DEFINE_RES_MEM_NAMED(0x1f2bUL, SZ_64K, "PCIe CSR"),
> > +   [1] = DEFINE_RES_MEM_NAMED(0x1f2cUL, SZ_64K, "PCIe CSR"),
> > +   [2] = DEFINE_RES_MEM_NAMED(0x1f2dUL, SZ_64K, "PCIe CSR"),
> > +   [3] = DEFINE_RES_MEM_NAMED(0x1f50UL, SZ_64K, "PCIe CSR"),
> > +   [4] = DEFINE_RES_MEM_NAMED(0x1f51UL, SZ_64K, "PCIe CSR"),
> I assume these ranges are not the actual ECAM space, right?
> If they *were* ECAM, I assume you would have included them in the
> quirk itself in the mcfg_quirks[] table.

These are base addresses for some RC mmio registers.
> 
> > 
> > +static int xgene_v1_pcie_ecam_init(struct pci_config_window *cfg)
> > +{
> > +   struct acpi_device *adev = to_acpi_device(cfg->parent);
> > +   struct acpi_pci_root *root = acpi_driver_data(adev);
> > +   struct device *dev = cfg->parent;
> > +   struct xgene_pcie_port *port;
> > +   struct resource *csr;
> > +
> > +   port = devm_kzalloc(dev, sizeof(*port), 

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Mark Salter]

On Thu, 2016-12-01 at 12:33 -0600, Bjorn Helgaas wrote:
> Hi Duc,
> 
> On Wed, Nov 30, 2016 at 03:42:53PM -0800, Duc Dang wrote:
> > 
> > PCIe controllers in X-Gene SoCs is not ECAM compliant: software
> > needs to configure additional controller's register to address
> > device at bus:dev:function.
> > 
> > The quirk will only be applied for X-Gene PCIe MCFG table with
> > OEM revison 1, 2, 3 or 4 (PCIe controller v1 and v2 on X-Gene SoCs).
> > 
> > The quirk declares the X-Gene PCIe controller register space as 64KB
> > fixed memory resource with name "PCIe CSR". This name will be showed
> > next to the resource range in the output of "cat /proc/iomem".
> > 
> > Signed-off-by: Duc Dang 
> > ---
> > v3:
> >   - Rebase on top of pci/ecam-v6 tree.
> >   - Use DEFINE_RES_MEM_NAMED to declare controller register space
> >   with name "PCIe CSR"
> > v2:
> >   RFC v2: https://patchwork.ozlabs.org/patch/686846/
> > v1:
> >   RFC v1: https://patchwork.kernel.org/patch/9337115/
> > 
> >  drivers/acpi/pci_mcfg.c  |  31 
> >  drivers/pci/host/pci-xgene.c | 165 
> > ++-
> >  include/linux/pci-ecam.h |   7 ++
> >  3 files changed, 200 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> > index ac21db3..eb6125b 100644
> > --- a/drivers/acpi/pci_mcfg.c
> > +++ b/drivers/acpi/pci_mcfg.c
> > @@ -57,6 +57,37 @@ struct mcfg_fixup {
> >     { "QCOM  ", "QDF2432 ", 1, 5, MCFG_BUS_ANY, _32b_ops },
> >     { "QCOM  ", "QDF2432 ", 1, 6, MCFG_BUS_ANY, _32b_ops },
> >     { "QCOM  ", "QDF2432 ", 1, 7, MCFG_BUS_ANY, _32b_ops },
> > +
> > +#ifdef CONFIG_PCI_XGENE
> As you've no doubt noticed, I'm proposing to add these quirks without
> CONFIG_PCI_XGENE, so we don't have to select each device when building
> a generic ACPI kernel.
> 
> I'm also proposing some Kconfig and Makefile changes so we don't build
> the platform driver part in a generic ACPI kernel (unless we *also*
> explicitly select the platform driver).
> 
> Here's an example:
> https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=pci/ecam=f80edf4d6c05
> 
> > 
> > +#define XGENE_V1_ECAM_MCFG(rev, seg) \
> > +   {"APM   ", "XGENE   ", rev, seg, MCFG_BUS_ANY, \
> > +   _v1_pcie_ecam_ops }
> > +#define XGENE_V2_1_ECAM_MCFG(rev, seg) \
> > +   {"APM   ", "XGENE   ", rev, seg, MCFG_BUS_ANY, \
> > +   _v2_1_pcie_ecam_ops }
> > +#define XGENE_V2_2_ECAM_MCFG(rev, seg) \
> > +   {"APM   ", "XGENE   ", rev, seg, MCFG_BUS_ANY, \
> > +   _v2_2_pcie_ecam_ops }
> > +
> > +   /* X-Gene SoC with v1 PCIe controller */
> > +   XGENE_V1_ECAM_MCFG(1, 0),
> > +   XGENE_V1_ECAM_MCFG(1, 1),
> > 
> > @@ -64,6 +66,7 @@
> >  /* PCIe IP version */
> >  #define XGENE_PCIE_IP_VER_UNKN 0
> >  #define XGENE_PCIE_IP_VER_11
> > +#define XGENE_PCIE_IP_VER_22
> This isn't used anywhere, which makes me wonder whether it's worth
> keeping it.
> 
> > 
> >  static void __iomem *xgene_pcie_get_cfg_base(struct pci_bus *bus)
> >  {
> > -   struct xgene_pcie_port *port = bus->sysdata;
> > +   struct pci_config_window *cfg;
> > +   struct xgene_pcie_port *port;
> > +
> > +   if (acpi_disabled)
> > +   port = bus->sysdata;
> > +   else {
> > +   cfg = bus->sysdata;
> > +   port = cfg->priv;
> > +   }
> I would really, really like to figure out a way to get rid of these
> "if (acpi_disabled)" checks sprinkled through here.  Is there any way
> we can set up bus->sysdata to be the same, regardless of whether we're
> using this as a platform driver or an ACPI quirk?
> 
> > 
> > +#ifdef CONFIG_ACPI
> You've probably noticed that I've been using
> 
>   #if defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS)
> 
> in this situation, mostly to make it clear that this is part of a
> workaround.  I don't want people to blindly copy this stuff without
> realizing that it's a workaround for a hardware issue.
> 
> > 
> > +static struct resource xgene_v1_csr_res[] = {
> > +   [0] = DEFINE_RES_MEM_NAMED(0x1f2bUL, SZ_64K, "PCIe CSR"),
> > +   [1] = DEFINE_RES_MEM_NAMED(0x1f2cUL, SZ_64K, "PCIe CSR"),
> > +   [2] = DEFINE_RES_MEM_NAMED(0x1f2dUL, SZ_64K, "PCIe CSR"),
> > +   [3] = DEFINE_RES_MEM_NAMED(0x1f50UL, SZ_64K, "PCIe CSR"),
> > +   [4] = DEFINE_RES_MEM_NAMED(0x1f51UL, SZ_64K, "PCIe CSR"),
> I assume these ranges are not the actual ECAM space, right?
> If they *were* ECAM, I assume you would have included them in the
> quirk itself in the mcfg_quirks[] table.

These are base addresses for some RC mmio registers.
> 
> > 
> > +static int xgene_v1_pcie_ecam_init(struct pci_config_window *cfg)
> > +{
> > +   struct acpi_device *adev = to_acpi_device(cfg->parent);
> > +   struct acpi_pci_root *root = acpi_driver_data(adev);
> > +   struct device *dev = cfg->parent;
> > +   struct xgene_pcie_port *port;
> > +   struct resource *csr;
> > +
> > +   port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> 

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Jon Masters]

On 12/01/2016 10:08 AM, Mark Salter wrote:
> On Wed, 2016-11-30 at 15:42 -0800, Duc Dang wrote:

>> +static int xgene_v1_pcie_ecam_init(struct pci_config_window *cfg)
>> +{
>> +struct acpi_device *adev = to_acpi_device(cfg->parent);
>> +struct acpi_pci_root *root = acpi_driver_data(adev);
>> +struct device *dev = cfg->parent;
>> +struct xgene_pcie_port *port;
>> +struct resource *csr;
>> +
>> +port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
>> +if (!port)
>> +return -ENOMEM;
>> +
>> +csr = _v1_csr_res[root->segment];
> 
> This hard-coded assumption that segment N uses controller N breaks
> for m400 where segment 0 is using controller 3.

This seems very fragile. So in addition to Bjorn's comment about not
trusting firmware provided data for the segment offset in the CSR list,
you will want to also determine the controller from the ACPI tree. The
existing code walks the ACPI hierarchy and finds the CSR that way.
Obviously, the goal is to avoid that in the latest incarnation, but you
could still determine which controller matches a given device.

Jon.

-- 
Computer Architect | Sent from my Fedora powered laptop

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Jon Masters]

On 12/01/2016 10:08 AM, Mark Salter wrote:
> On Wed, 2016-11-30 at 15:42 -0800, Duc Dang wrote:

>> +static int xgene_v1_pcie_ecam_init(struct pci_config_window *cfg)
>> +{
>> +struct acpi_device *adev = to_acpi_device(cfg->parent);
>> +struct acpi_pci_root *root = acpi_driver_data(adev);
>> +struct device *dev = cfg->parent;
>> +struct xgene_pcie_port *port;
>> +struct resource *csr;
>> +
>> +port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
>> +if (!port)
>> +return -ENOMEM;
>> +
>> +csr = _v1_csr_res[root->segment];
> 
> This hard-coded assumption that segment N uses controller N breaks
> for m400 where segment 0 is using controller 3.

This seems very fragile. So in addition to Bjorn's comment about not
trusting firmware provided data for the segment offset in the CSR list,
you will want to also determine the controller from the ACPI tree. The
existing code walks the ACPI hierarchy and finds the CSR that way.
Obviously, the goal is to avoid that in the latest incarnation, but you
could still determine which controller matches a given device.

Jon.

-- 
Computer Architect | Sent from my Fedora powered laptop

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Bjorn Helgaas]

Hi Duc,

On Wed, Nov 30, 2016 at 03:42:53PM -0800, Duc Dang wrote:
> PCIe controllers in X-Gene SoCs is not ECAM compliant: software
> needs to configure additional controller's register to address
> device at bus:dev:function.
> 
> The quirk will only be applied for X-Gene PCIe MCFG table with
> OEM revison 1, 2, 3 or 4 (PCIe controller v1 and v2 on X-Gene SoCs).
> 
> The quirk declares the X-Gene PCIe controller register space as 64KB
> fixed memory resource with name "PCIe CSR". This name will be showed
> next to the resource range in the output of "cat /proc/iomem".
> 
> Signed-off-by: Duc Dang 
> ---
> v3:
>   - Rebase on top of pci/ecam-v6 tree.
>   - Use DEFINE_RES_MEM_NAMED to declare controller register space
>   with name "PCIe CSR"
> v2:
>   RFC v2: https://patchwork.ozlabs.org/patch/686846/
> v1:
>   RFC v1: https://patchwork.kernel.org/patch/9337115/
> 
>  drivers/acpi/pci_mcfg.c  |  31 
>  drivers/pci/host/pci-xgene.c | 165 
> ++-
>  include/linux/pci-ecam.h |   7 ++
>  3 files changed, 200 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> index ac21db3..eb6125b 100644
> --- a/drivers/acpi/pci_mcfg.c
> +++ b/drivers/acpi/pci_mcfg.c
> @@ -57,6 +57,37 @@ struct mcfg_fixup {
>   { "QCOM  ", "QDF2432 ", 1, 5, MCFG_BUS_ANY, _32b_ops },
>   { "QCOM  ", "QDF2432 ", 1, 6, MCFG_BUS_ANY, _32b_ops },
>   { "QCOM  ", "QDF2432 ", 1, 7, MCFG_BUS_ANY, _32b_ops },
> +
> +#ifdef CONFIG_PCI_XGENE

As you've no doubt noticed, I'm proposing to add these quirks without
CONFIG_PCI_XGENE, so we don't have to select each device when building
a generic ACPI kernel.

I'm also proposing some Kconfig and Makefile changes so we don't build
the platform driver part in a generic ACPI kernel (unless we *also*
explicitly select the platform driver).

Here's an example:
https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=pci/ecam=f80edf4d6c05

> +#define XGENE_V1_ECAM_MCFG(rev, seg) \
> + {"APM   ", "XGENE   ", rev, seg, MCFG_BUS_ANY, \
> + _v1_pcie_ecam_ops }
> +#define XGENE_V2_1_ECAM_MCFG(rev, seg) \
> + {"APM   ", "XGENE   ", rev, seg, MCFG_BUS_ANY, \
> + _v2_1_pcie_ecam_ops }
> +#define XGENE_V2_2_ECAM_MCFG(rev, seg) \
> + {"APM   ", "XGENE   ", rev, seg, MCFG_BUS_ANY, \
> + _v2_2_pcie_ecam_ops }
> +
> + /* X-Gene SoC with v1 PCIe controller */
> + XGENE_V1_ECAM_MCFG(1, 0),
> + XGENE_V1_ECAM_MCFG(1, 1),

> @@ -64,6 +66,7 @@
>  /* PCIe IP version */
>  #define XGENE_PCIE_IP_VER_UNKN   0
>  #define XGENE_PCIE_IP_VER_1  1
> +#define XGENE_PCIE_IP_VER_2  2

This isn't used anywhere, which makes me wonder whether it's worth
keeping it.

>  static void __iomem *xgene_pcie_get_cfg_base(struct pci_bus *bus)
>  {
> - struct xgene_pcie_port *port = bus->sysdata;
> + struct pci_config_window *cfg;
> + struct xgene_pcie_port *port;
> +
> + if (acpi_disabled)
> + port = bus->sysdata;
> + else {
> + cfg = bus->sysdata;
> + port = cfg->priv;
> + }

I would really, really like to figure out a way to get rid of these
"if (acpi_disabled)" checks sprinkled through here.  Is there any way
we can set up bus->sysdata to be the same, regardless of whether we're
using this as a platform driver or an ACPI quirk?

> +#ifdef CONFIG_ACPI

You've probably noticed that I've been using

  #if defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS)

in this situation, mostly to make it clear that this is part of a
workaround.  I don't want people to blindly copy this stuff without
realizing that it's a workaround for a hardware issue.

> +static struct resource xgene_v1_csr_res[] = {
> + [0] = DEFINE_RES_MEM_NAMED(0x1f2bUL, SZ_64K, "PCIe CSR"),
> + [1] = DEFINE_RES_MEM_NAMED(0x1f2cUL, SZ_64K, "PCIe CSR"),
> + [2] = DEFINE_RES_MEM_NAMED(0x1f2dUL, SZ_64K, "PCIe CSR"),
> + [3] = DEFINE_RES_MEM_NAMED(0x1f50UL, SZ_64K, "PCIe CSR"),
> + [4] = DEFINE_RES_MEM_NAMED(0x1f51UL, SZ_64K, "PCIe CSR"),

I assume these ranges are not the actual ECAM space, right?
If they *were* ECAM, I assume you would have included them in the
quirk itself in the mcfg_quirks[] table.

> +static int xgene_v1_pcie_ecam_init(struct pci_config_window *cfg)
> +{
> + struct acpi_device *adev = to_acpi_device(cfg->parent);
> + struct acpi_pci_root *root = acpi_driver_data(adev);
> + struct device *dev = cfg->parent;
> + struct xgene_pcie_port *port;
> + struct resource *csr;
> +
> + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> + if (!port)
> + return -ENOMEM;
> +
> + csr = _v1_csr_res[root->segment];

This makes me nervous because root->segment comes from the ACPI _SEG,
and if firmware gives us junk in _SEG, we will reference something in
the weeds.

> + port->csr_base = devm_ioremap_resource(dev, csr);

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Bjorn Helgaas]

Hi Duc,

On Wed, Nov 30, 2016 at 03:42:53PM -0800, Duc Dang wrote:
> PCIe controllers in X-Gene SoCs is not ECAM compliant: software
> needs to configure additional controller's register to address
> device at bus:dev:function.
> 
> The quirk will only be applied for X-Gene PCIe MCFG table with
> OEM revison 1, 2, 3 or 4 (PCIe controller v1 and v2 on X-Gene SoCs).
> 
> The quirk declares the X-Gene PCIe controller register space as 64KB
> fixed memory resource with name "PCIe CSR". This name will be showed
> next to the resource range in the output of "cat /proc/iomem".
> 
> Signed-off-by: Duc Dang 
> ---
> v3:
>   - Rebase on top of pci/ecam-v6 tree.
>   - Use DEFINE_RES_MEM_NAMED to declare controller register space
>   with name "PCIe CSR"
> v2:
>   RFC v2: https://patchwork.ozlabs.org/patch/686846/
> v1:
>   RFC v1: https://patchwork.kernel.org/patch/9337115/
> 
>  drivers/acpi/pci_mcfg.c  |  31 
>  drivers/pci/host/pci-xgene.c | 165 
> ++-
>  include/linux/pci-ecam.h |   7 ++
>  3 files changed, 200 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> index ac21db3..eb6125b 100644
> --- a/drivers/acpi/pci_mcfg.c
> +++ b/drivers/acpi/pci_mcfg.c
> @@ -57,6 +57,37 @@ struct mcfg_fixup {
>   { "QCOM  ", "QDF2432 ", 1, 5, MCFG_BUS_ANY, _32b_ops },
>   { "QCOM  ", "QDF2432 ", 1, 6, MCFG_BUS_ANY, _32b_ops },
>   { "QCOM  ", "QDF2432 ", 1, 7, MCFG_BUS_ANY, _32b_ops },
> +
> +#ifdef CONFIG_PCI_XGENE

As you've no doubt noticed, I'm proposing to add these quirks without
CONFIG_PCI_XGENE, so we don't have to select each device when building
a generic ACPI kernel.

I'm also proposing some Kconfig and Makefile changes so we don't build
the platform driver part in a generic ACPI kernel (unless we *also*
explicitly select the platform driver).

Here's an example:
https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=pci/ecam=f80edf4d6c05

> +#define XGENE_V1_ECAM_MCFG(rev, seg) \
> + {"APM   ", "XGENE   ", rev, seg, MCFG_BUS_ANY, \
> + _v1_pcie_ecam_ops }
> +#define XGENE_V2_1_ECAM_MCFG(rev, seg) \
> + {"APM   ", "XGENE   ", rev, seg, MCFG_BUS_ANY, \
> + _v2_1_pcie_ecam_ops }
> +#define XGENE_V2_2_ECAM_MCFG(rev, seg) \
> + {"APM   ", "XGENE   ", rev, seg, MCFG_BUS_ANY, \
> + _v2_2_pcie_ecam_ops }
> +
> + /* X-Gene SoC with v1 PCIe controller */
> + XGENE_V1_ECAM_MCFG(1, 0),
> + XGENE_V1_ECAM_MCFG(1, 1),

> @@ -64,6 +66,7 @@
>  /* PCIe IP version */
>  #define XGENE_PCIE_IP_VER_UNKN   0
>  #define XGENE_PCIE_IP_VER_1  1
> +#define XGENE_PCIE_IP_VER_2  2

This isn't used anywhere, which makes me wonder whether it's worth
keeping it.

>  static void __iomem *xgene_pcie_get_cfg_base(struct pci_bus *bus)
>  {
> - struct xgene_pcie_port *port = bus->sysdata;
> + struct pci_config_window *cfg;
> + struct xgene_pcie_port *port;
> +
> + if (acpi_disabled)
> + port = bus->sysdata;
> + else {
> + cfg = bus->sysdata;
> + port = cfg->priv;
> + }

I would really, really like to figure out a way to get rid of these
"if (acpi_disabled)" checks sprinkled through here.  Is there any way
we can set up bus->sysdata to be the same, regardless of whether we're
using this as a platform driver or an ACPI quirk?

> +#ifdef CONFIG_ACPI

You've probably noticed that I've been using

  #if defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS)

in this situation, mostly to make it clear that this is part of a
workaround.  I don't want people to blindly copy this stuff without
realizing that it's a workaround for a hardware issue.

> +static struct resource xgene_v1_csr_res[] = {
> + [0] = DEFINE_RES_MEM_NAMED(0x1f2bUL, SZ_64K, "PCIe CSR"),
> + [1] = DEFINE_RES_MEM_NAMED(0x1f2cUL, SZ_64K, "PCIe CSR"),
> + [2] = DEFINE_RES_MEM_NAMED(0x1f2dUL, SZ_64K, "PCIe CSR"),
> + [3] = DEFINE_RES_MEM_NAMED(0x1f50UL, SZ_64K, "PCIe CSR"),
> + [4] = DEFINE_RES_MEM_NAMED(0x1f51UL, SZ_64K, "PCIe CSR"),

I assume these ranges are not the actual ECAM space, right?
If they *were* ECAM, I assume you would have included them in the
quirk itself in the mcfg_quirks[] table.

> +static int xgene_v1_pcie_ecam_init(struct pci_config_window *cfg)
> +{
> + struct acpi_device *adev = to_acpi_device(cfg->parent);
> + struct acpi_pci_root *root = acpi_driver_data(adev);
> + struct device *dev = cfg->parent;
> + struct xgene_pcie_port *port;
> + struct resource *csr;
> +
> + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> + if (!port)
> + return -ENOMEM;
> +
> + csr = _v1_csr_res[root->segment];

This makes me nervous because root->segment comes from the ACPI _SEG,
and if firmware gives us junk in _SEG, we will reference something in
the weeds.

> + port->csr_base = devm_ioremap_resource(dev, csr);
> + if 

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Mark Salter]

On Wed, 2016-11-30 at 15:42 -0800, Duc Dang wrote:
> PCIe controllers in X-Gene SoCs is not ECAM compliant: software
> needs to configure additional controller's register to address
> device at bus:dev:function.
> 
> The quirk will only be applied for X-Gene PCIe MCFG table with
> OEM revison 1, 2, 3 or 4 (PCIe controller v1 and v2 on X-Gene SoCs).
> 
> The quirk declares the X-Gene PCIe controller register space as 64KB
> fixed memory resource with name "PCIe CSR". This name will be showed
> next to the resource range in the output of "cat /proc/iomem".
> 
> Signed-off-by: Duc Dang 
> ---
> v3:
>   - Rebase on top of pci/ecam-v6 tree.
>   - Use DEFINE_RES_MEM_NAMED to declare controller register space
>   with name "PCIe CSR"
> v2:
>   RFC v2: https://patchwork.ozlabs.org/patch/686846/
> v1:
>   RFC v1: https://patchwork.kernel.org/patch/9337115/
> 
>  drivers/acpi/pci_mcfg.c  |  31 
>  drivers/pci/host/pci-xgene.c | 165 
> ++-
>  include/linux/pci-ecam.h |   7 ++
>  3 files changed, 200 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> index ac21db3..eb6125b 100644
> --- a/drivers/acpi/pci_mcfg.c
> +++ b/drivers/acpi/pci_mcfg.c
> @@ -57,6 +57,37 @@ struct mcfg_fixup {
>   { "QCOM  ", "QDF2432 ", 1, 5, MCFG_BUS_ANY, _32b_ops },
>   { "QCOM  ", "QDF2432 ", 1, 6, MCFG_BUS_ANY, _32b_ops },
>   { "QCOM  ", "QDF2432 ", 1, 7, MCFG_BUS_ANY, _32b_ops },
> +
> +#ifdef CONFIG_PCI_XGENE
> +#define XGENE_V1_ECAM_MCFG(rev, seg) \
> + {"APM   ", "XGENE   ", rev, seg, MCFG_BUS_ANY, \
> + _v1_pcie_ecam_ops }
> +#define XGENE_V2_1_ECAM_MCFG(rev, seg) \
> + {"APM   ", "XGENE   ", rev, seg, MCFG_BUS_ANY, \
> + _v2_1_pcie_ecam_ops }
> +#define XGENE_V2_2_ECAM_MCFG(rev, seg) \
> + {"APM   ", "XGENE   ", rev, seg, MCFG_BUS_ANY, \
> + _v2_2_pcie_ecam_ops }
> +
> + /* X-Gene SoC with v1 PCIe controller */
> + XGENE_V1_ECAM_MCFG(1, 0),
> + XGENE_V1_ECAM_MCFG(1, 1),
> + XGENE_V1_ECAM_MCFG(1, 2),
> + XGENE_V1_ECAM_MCFG(1, 3),
> + XGENE_V1_ECAM_MCFG(1, 4),
> + XGENE_V1_ECAM_MCFG(2, 0),
> + XGENE_V1_ECAM_MCFG(2, 1),
> + XGENE_V1_ECAM_MCFG(2, 2),
> + XGENE_V1_ECAM_MCFG(2, 3),
> + XGENE_V1_ECAM_MCFG(2, 4),
> + /* X-Gene SoC with v2.1 PCIe controller */
> + XGENE_V2_1_ECAM_MCFG(3, 0),
> + XGENE_V2_1_ECAM_MCFG(3, 1),
> + /* X-Gene SoC with v2.2 PCIe controller */
> + XGENE_V2_2_ECAM_MCFG(4, 0),
> + XGENE_V2_2_ECAM_MCFG(4, 1),
> + XGENE_V2_2_ECAM_MCFG(4, 2),
> +#endif
>  };
>  
>  static char mcfg_oem_id[ACPI_OEM_ID_SIZE];
> diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
> index 1de23d7..43d7c69 100644
> --- a/drivers/pci/host/pci-xgene.c
> +++ b/drivers/pci/host/pci-xgene.c
> @@ -27,6 +27,8 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
>  #include 
>  #include 
>  
> @@ -64,6 +66,7 @@
>  /* PCIe IP version */
>  #define XGENE_PCIE_IP_VER_UNKN   0
>  #define XGENE_PCIE_IP_VER_1  1
> +#define XGENE_PCIE_IP_VER_2  2
>  
>  struct xgene_pcie_port {
>   struct device_node  *node;
> @@ -97,7 +100,15 @@ static inline u32 pcie_bar_low_val(u32 addr, u32 flags)
>   */
>  static void __iomem *xgene_pcie_get_cfg_base(struct pci_bus *bus)
>  {
> - struct xgene_pcie_port *port = bus->sysdata;
> + struct pci_config_window *cfg;
> + struct xgene_pcie_port *port;
> +
> + if (acpi_disabled)
> + port = bus->sysdata;
> + else {
> + cfg = bus->sysdata;
> + port = cfg->priv;
> + }
>  
>   if (bus->number >= (bus->primary + 1))
>   return port->cfg_base + AXI_EP_CFG_ACCESS;
> @@ -111,10 +122,18 @@ static void __iomem *xgene_pcie_get_cfg_base(struct 
> pci_bus *bus)
>   */
>  static void xgene_pcie_set_rtdid_reg(struct pci_bus *bus, uint devfn)
>  {
> - struct xgene_pcie_port *port = bus->sysdata;
> + struct pci_config_window *cfg;
> + struct xgene_pcie_port *port;
>   unsigned int b, d, f;
>   u32 rtdid_val = 0;
>  
> + if (acpi_disabled)
> + port = bus->sysdata;
> + else {
> + cfg = bus->sysdata;
> + port = cfg->priv;
> + }
> +
>   b = bus->number;
>   d = PCI_SLOT(devfn);
>   f = PCI_FUNC(devfn);
> @@ -158,7 +177,15 @@ static void __iomem *xgene_pcie_map_bus(struct pci_bus 
> *bus, unsigned int devfn,
>  static int xgene_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
>   int where, int size, u32 *val)
>  {
> - struct xgene_pcie_port *port = bus->sysdata;
> + struct pci_config_window *cfg;
> + struct xgene_pcie_port *port;
> +
> + if (acpi_disabled)
> + port = bus->sysdata;
> + else {
> + cfg = bus->sysdata;
> + port = cfg->priv;
> + }
>  
>   if 

Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Mark Salter]

On Wed, 2016-11-30 at 15:42 -0800, Duc Dang wrote:
> PCIe controllers in X-Gene SoCs is not ECAM compliant: software
> needs to configure additional controller's register to address
> device at bus:dev:function.
> 
> The quirk will only be applied for X-Gene PCIe MCFG table with
> OEM revison 1, 2, 3 or 4 (PCIe controller v1 and v2 on X-Gene SoCs).
> 
> The quirk declares the X-Gene PCIe controller register space as 64KB
> fixed memory resource with name "PCIe CSR". This name will be showed
> next to the resource range in the output of "cat /proc/iomem".
> 
> Signed-off-by: Duc Dang 
> ---
> v3:
>   - Rebase on top of pci/ecam-v6 tree.
>   - Use DEFINE_RES_MEM_NAMED to declare controller register space
>   with name "PCIe CSR"
> v2:
>   RFC v2: https://patchwork.ozlabs.org/patch/686846/
> v1:
>   RFC v1: https://patchwork.kernel.org/patch/9337115/
> 
>  drivers/acpi/pci_mcfg.c  |  31 
>  drivers/pci/host/pci-xgene.c | 165 
> ++-
>  include/linux/pci-ecam.h |   7 ++
>  3 files changed, 200 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> index ac21db3..eb6125b 100644
> --- a/drivers/acpi/pci_mcfg.c
> +++ b/drivers/acpi/pci_mcfg.c
> @@ -57,6 +57,37 @@ struct mcfg_fixup {
>   { "QCOM  ", "QDF2432 ", 1, 5, MCFG_BUS_ANY, _32b_ops },
>   { "QCOM  ", "QDF2432 ", 1, 6, MCFG_BUS_ANY, _32b_ops },
>   { "QCOM  ", "QDF2432 ", 1, 7, MCFG_BUS_ANY, _32b_ops },
> +
> +#ifdef CONFIG_PCI_XGENE
> +#define XGENE_V1_ECAM_MCFG(rev, seg) \
> + {"APM   ", "XGENE   ", rev, seg, MCFG_BUS_ANY, \
> + _v1_pcie_ecam_ops }
> +#define XGENE_V2_1_ECAM_MCFG(rev, seg) \
> + {"APM   ", "XGENE   ", rev, seg, MCFG_BUS_ANY, \
> + _v2_1_pcie_ecam_ops }
> +#define XGENE_V2_2_ECAM_MCFG(rev, seg) \
> + {"APM   ", "XGENE   ", rev, seg, MCFG_BUS_ANY, \
> + _v2_2_pcie_ecam_ops }
> +
> + /* X-Gene SoC with v1 PCIe controller */
> + XGENE_V1_ECAM_MCFG(1, 0),
> + XGENE_V1_ECAM_MCFG(1, 1),
> + XGENE_V1_ECAM_MCFG(1, 2),
> + XGENE_V1_ECAM_MCFG(1, 3),
> + XGENE_V1_ECAM_MCFG(1, 4),
> + XGENE_V1_ECAM_MCFG(2, 0),
> + XGENE_V1_ECAM_MCFG(2, 1),
> + XGENE_V1_ECAM_MCFG(2, 2),
> + XGENE_V1_ECAM_MCFG(2, 3),
> + XGENE_V1_ECAM_MCFG(2, 4),
> + /* X-Gene SoC with v2.1 PCIe controller */
> + XGENE_V2_1_ECAM_MCFG(3, 0),
> + XGENE_V2_1_ECAM_MCFG(3, 1),
> + /* X-Gene SoC with v2.2 PCIe controller */
> + XGENE_V2_2_ECAM_MCFG(4, 0),
> + XGENE_V2_2_ECAM_MCFG(4, 1),
> + XGENE_V2_2_ECAM_MCFG(4, 2),
> +#endif
>  };
>  
>  static char mcfg_oem_id[ACPI_OEM_ID_SIZE];
> diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
> index 1de23d7..43d7c69 100644
> --- a/drivers/pci/host/pci-xgene.c
> +++ b/drivers/pci/host/pci-xgene.c
> @@ -27,6 +27,8 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
>  #include 
>  #include 
>  
> @@ -64,6 +66,7 @@
>  /* PCIe IP version */
>  #define XGENE_PCIE_IP_VER_UNKN   0
>  #define XGENE_PCIE_IP_VER_1  1
> +#define XGENE_PCIE_IP_VER_2  2
>  
>  struct xgene_pcie_port {
>   struct device_node  *node;
> @@ -97,7 +100,15 @@ static inline u32 pcie_bar_low_val(u32 addr, u32 flags)
>   */
>  static void __iomem *xgene_pcie_get_cfg_base(struct pci_bus *bus)
>  {
> - struct xgene_pcie_port *port = bus->sysdata;
> + struct pci_config_window *cfg;
> + struct xgene_pcie_port *port;
> +
> + if (acpi_disabled)
> + port = bus->sysdata;
> + else {
> + cfg = bus->sysdata;
> + port = cfg->priv;
> + }
>  
>   if (bus->number >= (bus->primary + 1))
>   return port->cfg_base + AXI_EP_CFG_ACCESS;
> @@ -111,10 +122,18 @@ static void __iomem *xgene_pcie_get_cfg_base(struct 
> pci_bus *bus)
>   */
>  static void xgene_pcie_set_rtdid_reg(struct pci_bus *bus, uint devfn)
>  {
> - struct xgene_pcie_port *port = bus->sysdata;
> + struct pci_config_window *cfg;
> + struct xgene_pcie_port *port;
>   unsigned int b, d, f;
>   u32 rtdid_val = 0;
>  
> + if (acpi_disabled)
> + port = bus->sysdata;
> + else {
> + cfg = bus->sysdata;
> + port = cfg->priv;
> + }
> +
>   b = bus->number;
>   d = PCI_SLOT(devfn);
>   f = PCI_FUNC(devfn);
> @@ -158,7 +177,15 @@ static void __iomem *xgene_pcie_map_bus(struct pci_bus 
> *bus, unsigned int devfn,
>  static int xgene_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
>   int where, int size, u32 *val)
>  {
> - struct xgene_pcie_port *port = bus->sysdata;
> + struct pci_config_window *cfg;
> + struct xgene_pcie_port *port;
> +
> + if (acpi_disabled)
> + port = bus->sysdata;
> + else {
> + cfg = bus->sysdata;
> + port = cfg->priv;
> + }
>  
>   if (pci_generic_config_read32(bus, 

[PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Duc Dang]

PCIe controllers in X-Gene SoCs is not ECAM compliant: software
needs to configure additional controller's register to address
device at bus:dev:function.

The quirk will only be applied for X-Gene PCIe MCFG table with
OEM revison 1, 2, 3 or 4 (PCIe controller v1 and v2 on X-Gene SoCs).

The quirk declares the X-Gene PCIe controller register space as 64KB
fixed memory resource with name "PCIe CSR". This name will be showed
next to the resource range in the output of "cat /proc/iomem".

Signed-off-by: Duc Dang 
---
v3:
  - Rebase on top of pci/ecam-v6 tree.
  - Use DEFINE_RES_MEM_NAMED to declare controller register space
  with name "PCIe CSR"
v2:
  RFC v2: https://patchwork.ozlabs.org/patch/686846/
v1:
  RFC v1: https://patchwork.kernel.org/patch/9337115/

 drivers/acpi/pci_mcfg.c  |  31 
 drivers/pci/host/pci-xgene.c | 165 ++-
 include/linux/pci-ecam.h |   7 ++
 3 files changed, 200 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
index ac21db3..eb6125b 100644
--- a/drivers/acpi/pci_mcfg.c
+++ b/drivers/acpi/pci_mcfg.c
@@ -57,6 +57,37 @@ struct mcfg_fixup {
{ "QCOM  ", "QDF2432 ", 1, 5, MCFG_BUS_ANY, _32b_ops },
{ "QCOM  ", "QDF2432 ", 1, 6, MCFG_BUS_ANY, _32b_ops },
{ "QCOM  ", "QDF2432 ", 1, 7, MCFG_BUS_ANY, _32b_ops },
+
+#ifdef CONFIG_PCI_XGENE
+#define XGENE_V1_ECAM_MCFG(rev, seg) \
+   {"APM   ", "XGENE   ", rev, seg, MCFG_BUS_ANY, \
+   _v1_pcie_ecam_ops }
+#define XGENE_V2_1_ECAM_MCFG(rev, seg) \
+   {"APM   ", "XGENE   ", rev, seg, MCFG_BUS_ANY, \
+   _v2_1_pcie_ecam_ops }
+#define XGENE_V2_2_ECAM_MCFG(rev, seg) \
+   {"APM   ", "XGENE   ", rev, seg, MCFG_BUS_ANY, \
+   _v2_2_pcie_ecam_ops }
+
+   /* X-Gene SoC with v1 PCIe controller */
+   XGENE_V1_ECAM_MCFG(1, 0),
+   XGENE_V1_ECAM_MCFG(1, 1),
+   XGENE_V1_ECAM_MCFG(1, 2),
+   XGENE_V1_ECAM_MCFG(1, 3),
+   XGENE_V1_ECAM_MCFG(1, 4),
+   XGENE_V1_ECAM_MCFG(2, 0),
+   XGENE_V1_ECAM_MCFG(2, 1),
+   XGENE_V1_ECAM_MCFG(2, 2),
+   XGENE_V1_ECAM_MCFG(2, 3),
+   XGENE_V1_ECAM_MCFG(2, 4),
+   /* X-Gene SoC with v2.1 PCIe controller */
+   XGENE_V2_1_ECAM_MCFG(3, 0),
+   XGENE_V2_1_ECAM_MCFG(3, 1),
+   /* X-Gene SoC with v2.2 PCIe controller */
+   XGENE_V2_2_ECAM_MCFG(4, 0),
+   XGENE_V2_2_ECAM_MCFG(4, 1),
+   XGENE_V2_2_ECAM_MCFG(4, 2),
+#endif
 };
 
 static char mcfg_oem_id[ACPI_OEM_ID_SIZE];
diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
index 1de23d7..43d7c69 100644
--- a/drivers/pci/host/pci-xgene.c
+++ b/drivers/pci/host/pci-xgene.c
@@ -27,6 +27,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 #include 
 #include 
 
@@ -64,6 +66,7 @@
 /* PCIe IP version */
 #define XGENE_PCIE_IP_VER_UNKN 0
 #define XGENE_PCIE_IP_VER_11
+#define XGENE_PCIE_IP_VER_22
 
 struct xgene_pcie_port {
struct device_node  *node;
@@ -97,7 +100,15 @@ static inline u32 pcie_bar_low_val(u32 addr, u32 flags)
  */
 static void __iomem *xgene_pcie_get_cfg_base(struct pci_bus *bus)
 {
-   struct xgene_pcie_port *port = bus->sysdata;
+   struct pci_config_window *cfg;
+   struct xgene_pcie_port *port;
+
+   if (acpi_disabled)
+   port = bus->sysdata;
+   else {
+   cfg = bus->sysdata;
+   port = cfg->priv;
+   }
 
if (bus->number >= (bus->primary + 1))
return port->cfg_base + AXI_EP_CFG_ACCESS;
@@ -111,10 +122,18 @@ static void __iomem *xgene_pcie_get_cfg_base(struct 
pci_bus *bus)
  */
 static void xgene_pcie_set_rtdid_reg(struct pci_bus *bus, uint devfn)
 {
-   struct xgene_pcie_port *port = bus->sysdata;
+   struct pci_config_window *cfg;
+   struct xgene_pcie_port *port;
unsigned int b, d, f;
u32 rtdid_val = 0;
 
+   if (acpi_disabled)
+   port = bus->sysdata;
+   else {
+   cfg = bus->sysdata;
+   port = cfg->priv;
+   }
+
b = bus->number;
d = PCI_SLOT(devfn);
f = PCI_FUNC(devfn);
@@ -158,7 +177,15 @@ static void __iomem *xgene_pcie_map_bus(struct pci_bus 
*bus, unsigned int devfn,
 static int xgene_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
int where, int size, u32 *val)
 {
-   struct xgene_pcie_port *port = bus->sysdata;
+   struct pci_config_window *cfg;
+   struct xgene_pcie_port *port;
+
+   if (acpi_disabled)
+   port = bus->sysdata;
+   else {
+   cfg = bus->sysdata;
+   port = cfg->priv;
+   }
 
if (pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val) !=
PCIBIOS_SUCCESSFUL)
@@ -189,6 +216,138 @@ static int xgene_pcie_config_read32(struct pci_bus *bus, 
unsigned int devfn,
.write = 

[PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

[Duc Dang]

PCIe controllers in X-Gene SoCs is not ECAM compliant: software
needs to configure additional controller's register to address
device at bus:dev:function.

The quirk will only be applied for X-Gene PCIe MCFG table with
OEM revison 1, 2, 3 or 4 (PCIe controller v1 and v2 on X-Gene SoCs).

The quirk declares the X-Gene PCIe controller register space as 64KB
fixed memory resource with name "PCIe CSR". This name will be showed
next to the resource range in the output of "cat /proc/iomem".

Signed-off-by: Duc Dang 
---
v3:
  - Rebase on top of pci/ecam-v6 tree.
  - Use DEFINE_RES_MEM_NAMED to declare controller register space
  with name "PCIe CSR"
v2:
  RFC v2: https://patchwork.ozlabs.org/patch/686846/
v1:
  RFC v1: https://patchwork.kernel.org/patch/9337115/

 drivers/acpi/pci_mcfg.c  |  31 
 drivers/pci/host/pci-xgene.c | 165 ++-
 include/linux/pci-ecam.h |   7 ++
 3 files changed, 200 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
index ac21db3..eb6125b 100644
--- a/drivers/acpi/pci_mcfg.c
+++ b/drivers/acpi/pci_mcfg.c
@@ -57,6 +57,37 @@ struct mcfg_fixup {
{ "QCOM  ", "QDF2432 ", 1, 5, MCFG_BUS_ANY, _32b_ops },
{ "QCOM  ", "QDF2432 ", 1, 6, MCFG_BUS_ANY, _32b_ops },
{ "QCOM  ", "QDF2432 ", 1, 7, MCFG_BUS_ANY, _32b_ops },
+
+#ifdef CONFIG_PCI_XGENE
+#define XGENE_V1_ECAM_MCFG(rev, seg) \
+   {"APM   ", "XGENE   ", rev, seg, MCFG_BUS_ANY, \
+   _v1_pcie_ecam_ops }
+#define XGENE_V2_1_ECAM_MCFG(rev, seg) \
+   {"APM   ", "XGENE   ", rev, seg, MCFG_BUS_ANY, \
+   _v2_1_pcie_ecam_ops }
+#define XGENE_V2_2_ECAM_MCFG(rev, seg) \
+   {"APM   ", "XGENE   ", rev, seg, MCFG_BUS_ANY, \
+   _v2_2_pcie_ecam_ops }
+
+   /* X-Gene SoC with v1 PCIe controller */
+   XGENE_V1_ECAM_MCFG(1, 0),
+   XGENE_V1_ECAM_MCFG(1, 1),
+   XGENE_V1_ECAM_MCFG(1, 2),
+   XGENE_V1_ECAM_MCFG(1, 3),
+   XGENE_V1_ECAM_MCFG(1, 4),
+   XGENE_V1_ECAM_MCFG(2, 0),
+   XGENE_V1_ECAM_MCFG(2, 1),
+   XGENE_V1_ECAM_MCFG(2, 2),
+   XGENE_V1_ECAM_MCFG(2, 3),
+   XGENE_V1_ECAM_MCFG(2, 4),
+   /* X-Gene SoC with v2.1 PCIe controller */
+   XGENE_V2_1_ECAM_MCFG(3, 0),
+   XGENE_V2_1_ECAM_MCFG(3, 1),
+   /* X-Gene SoC with v2.2 PCIe controller */
+   XGENE_V2_2_ECAM_MCFG(4, 0),
+   XGENE_V2_2_ECAM_MCFG(4, 1),
+   XGENE_V2_2_ECAM_MCFG(4, 2),
+#endif
 };
 
 static char mcfg_oem_id[ACPI_OEM_ID_SIZE];
diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
index 1de23d7..43d7c69 100644
--- a/drivers/pci/host/pci-xgene.c
+++ b/drivers/pci/host/pci-xgene.c
@@ -27,6 +27,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 #include 
 #include 
 
@@ -64,6 +66,7 @@
 /* PCIe IP version */
 #define XGENE_PCIE_IP_VER_UNKN 0
 #define XGENE_PCIE_IP_VER_11
+#define XGENE_PCIE_IP_VER_22
 
 struct xgene_pcie_port {
struct device_node  *node;
@@ -97,7 +100,15 @@ static inline u32 pcie_bar_low_val(u32 addr, u32 flags)
  */
 static void __iomem *xgene_pcie_get_cfg_base(struct pci_bus *bus)
 {
-   struct xgene_pcie_port *port = bus->sysdata;
+   struct pci_config_window *cfg;
+   struct xgene_pcie_port *port;
+
+   if (acpi_disabled)
+   port = bus->sysdata;
+   else {
+   cfg = bus->sysdata;
+   port = cfg->priv;
+   }
 
if (bus->number >= (bus->primary + 1))
return port->cfg_base + AXI_EP_CFG_ACCESS;
@@ -111,10 +122,18 @@ static void __iomem *xgene_pcie_get_cfg_base(struct 
pci_bus *bus)
  */
 static void xgene_pcie_set_rtdid_reg(struct pci_bus *bus, uint devfn)
 {
-   struct xgene_pcie_port *port = bus->sysdata;
+   struct pci_config_window *cfg;
+   struct xgene_pcie_port *port;
unsigned int b, d, f;
u32 rtdid_val = 0;
 
+   if (acpi_disabled)
+   port = bus->sysdata;
+   else {
+   cfg = bus->sysdata;
+   port = cfg->priv;
+   }
+
b = bus->number;
d = PCI_SLOT(devfn);
f = PCI_FUNC(devfn);
@@ -158,7 +177,15 @@ static void __iomem *xgene_pcie_map_bus(struct pci_bus 
*bus, unsigned int devfn,
 static int xgene_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
int where, int size, u32 *val)
 {
-   struct xgene_pcie_port *port = bus->sysdata;
+   struct pci_config_window *cfg;
+   struct xgene_pcie_port *port;
+
+   if (acpi_disabled)
+   port = bus->sysdata;
+   else {
+   cfg = bus->sysdata;
+   port = cfg->priv;
+   }
 
if (pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val) !=
PCIBIOS_SUCCESSFUL)
@@ -189,6 +216,138 @@ static int xgene_pcie_config_read32(struct pci_bus *bus, 
unsigned int devfn,
.write = pci_generic_config_write32,
 };
 
+#ifdef