Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1380019 > unrolled thread

[PATCH V6 00/13] Support for generic ACPI based PCI host controller

Back to article view | Back to linux.kernel

Contents

  [PATCH V6 00/13] Support for generic ACPI based PCI host controller Tomasz Nowicki <tn@semihalf.com> - 2016-04-15 19:10 +0200
    [PATCH V6 11/13] pci, acpi: Match PCI config space accessors against platfrom specific quirks. Tomasz Nowicki <tn@semihalf.com> - 2016-04-15 19:10 +0200
      Re: [PATCH V6 11/13] pci, acpi: Match PCI config space accessors  against platfrom specific quirks. "liudongdong (C)" <liudongdong3@huawei.com> - 2016-04-18 14:00 +0200
        Re: [PATCH V6 11/13] pci, acpi: Match PCI config space accessors  against platfrom specific quirks. Tomasz Nowicki <tn@semihalf.com> - 2016-04-18 14:30 +0200
    [PATCH V6 13/13] pci, pci-thunder-pem: Add ACPI support for ThunderX PEM. Tomasz Nowicki <tn@semihalf.com> - 2016-04-15 19:10 +0200
    [PATCH V6 02/13] pci, acpi: Provide generic way to assign bus domain number. Tomasz Nowicki <tn@semihalf.com> - 2016-04-15 19:10 +0200
      Re: [PATCH V6 02/13] pci, acpi: Provide generic way to assign bus  domain number. Bjorn Helgaas <helgaas@kernel.org> - 2016-04-27 04:30 +0200
        Re: [PATCH V6 02/13] pci, acpi: Provide generic way to assign bus  domain number. Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2016-04-27 13:20 +0200
          Re: [PATCH V6 02/13] pci, acpi: Provide generic way to assign bus  domain number. Bjorn Helgaas <helgaas@kernel.org> - 2016-04-27 18:50 +0200
            Re: [PATCH V6 02/13] pci, acpi: Provide generic way to assign bus  domain number. Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2016-04-27 19:40 +0200
              Re: [PATCH V6 02/13] pci, acpi: Provide generic way to assign bus  domain number. Liviu.Dudau@arm.com - 2016-04-28 10:20 +0200
        Re: [PATCH V6 02/13] pci, acpi: Provide generic way to assign bus  domain number. Tomasz Nowicki <tn@semihalf.com> - 2016-04-27 14:10 +0200
    [PATCH V6 12/13] pci, pci-thunder-ecam: Add ACPI support for ThunderX ECAM. Tomasz Nowicki <tn@semihalf.com> - 2016-04-15 19:10 +0200
      Re: [PATCH V6 12/13] pci, pci-thunder-ecam: Add ACPI support for  ThunderX ECAM. Tomasz Nowicki <tn@semihalf.com> - 2016-04-19 12:30 +0200
        Re: [Linaro-acpi] [PATCH V6 12/13] pci, pci-thunder-ecam: Add ACPI  support for ThunderX ECAM. G Gregory <graeme.gregory@linaro.org> - 2016-04-19 12:50 +0200
          Re: [Linaro-acpi] [PATCH V6 12/13] pci, pci-thunder-ecam: Add ACPI  support for ThunderX ECAM. Graeme Gregory <gg@slimlogic.co.uk> - 2016-04-19 13:20 +0200
            Re: [Linaro-acpi] [PATCH V6 12/13] pci, pci-thunder-ecam: Add ACPI  support for ThunderX ECAM. Tomasz Nowicki <tn@semihalf.com> - 2016-04-19 13:30 +0200
              Re: [Linaro-acpi] [PATCH V6 12/13] pci, pci-thunder-ecam: Add ACPI  support for ThunderX ECAM. G Gregory <graeme.gregory@linaro.org> - 2016-04-19 14:30 +0200
    [PATCH V6 08/13] PCI: generic, thunder: update to use generic ECAM API Tomasz Nowicki <tn@semihalf.com> - 2016-04-15 19:10 +0200
      Re: [PATCH V6 08/13] PCI: generic, thunder: update to use generic ECAM API Arnd Bergmann <arnd@arndb.de> - 2016-04-15 20:50 +0200
        Re: [PATCH V6 08/13] PCI: generic, thunder: update to use generic  ECAM API Jayachandran C <jchandra@broadcom.com> - 2016-04-16 09:30 +0200
          Re: [PATCH V6 08/13] PCI: generic, thunder: update to use generic ECAM API Arnd Bergmann <arnd@arndb.de> - 2016-04-16 09:40 +0200
            Re: [PATCH V6 08/13] PCI: generic, thunder: update to use generic  ECAM API Jayachandran C <jchandra@broadcom.com> - 2016-04-16 16:40 +0200
              Re: [PATCH V6 08/13] PCI: generic, thunder: update to use generic  ECAM API Tomasz Nowicki <tn@semihalf.com> - 2016-04-18 15:10 +0200
                Re: [PATCH V6 08/13] PCI: generic, thunder: update to use generic ECAM API Arnd Bergmann <arnd@arndb.de> - 2016-04-18 16:50 +0200
                  Re: [PATCH V6 08/13] PCI: generic, thunder: update to use generic  ECAM API Tomasz Nowicki <tn@semihalf.com> - 2016-04-18 21:40 +0200
                    Re: [PATCH V6 08/13] PCI: generic, thunder: update to use generic ECAM API Arnd Bergmann <arnd@arndb.de> - 2016-04-19 15:10 +0200
                      Re: [PATCH V6 08/13] PCI: generic, thunder: update to use generic  ECAM API Tomasz Nowicki <tn@semihalf.com> - 2016-04-21 11:30 +0200
                        Re: [PATCH V6 08/13] PCI: generic, thunder: update to use generic ECAM API Arnd Bergmann <arnd@arndb.de> - 2016-04-21 11:40 +0200
                          Re: [PATCH V6 08/13] PCI: generic, thunder: update to use generic  ECAM API Tomasz Nowicki <tn@semihalf.com> - 2016-04-21 12:10 +0200
                            Re: [PATCH V6 08/13] PCI: generic, thunder: update to use generic  ECAM API Jon Masters <jcm@redhat.com> - 2016-04-22 16:40 +0200
                              Re: [PATCH V6 08/13] PCI: generic, thunder: update to use generic  ECAM API David Daney <ddaney.cavm@gmail.com> - 2016-04-22 18:10 +0200
      Re: [PATCH V6 08/13] PCI: generic, thunder: update to use generic ECAM API Arnd Bergmann <arnd@arndb.de> - 2016-04-19 23:50 +0200
        Re: [PATCH V6 08/13] PCI: generic, thunder: update to use generic  ECAM API Jayachandran C <jchandra@broadcom.com> - 2016-04-20 02:30 +0200
    [PATCH V6 04/13] pci, of: Move the PCI I/O space management to PCI core code. Tomasz Nowicki <tn@semihalf.com> - 2016-04-15 19:10 +0200
    [PATCH V6 09/13] pci, acpi: Support for ACPI based generic PCI host controller Tomasz Nowicki <tn@semihalf.com> - 2016-04-15 19:10 +0200
      Re: [PATCH V6 09/13] pci, acpi: Support for ACPI based generic PCI  host controller Jayachandran C <jchandra@broadcom.com> - 2016-04-20 21:20 +0200
        Re: [PATCH V6 09/13] pci, acpi: Support for ACPI based generic PCI  host controller Tomasz Nowicki <tn@semihalf.com> - 2016-04-21 11:10 +0200
          Re: [PATCH V6 09/13] pci, acpi: Support for ACPI based generic PCI  host controller Jayachandran C <jchandra@broadcom.com> - 2016-04-22 15:00 +0200
          Re: [PATCH V6 09/13] pci, acpi: Support for ACPI based generic PCI  host controller Jon Masters <jcm@redhat.com> - 2016-04-22 16:50 +0200
            Re: [PATCH V6 09/13] pci, acpi: Support for ACPI based generic PCI  host controller Jon Masters <jcm@redhat.com> - 2016-04-23 17:30 +0200
    [PATCH V6 10/13] arm64, pci, acpi: Start using ACPI based PCI host controller driver for ARM64. Tomasz Nowicki <tn@semihalf.com> - 2016-04-15 19:10 +0200
    [PATCH V6 06/13] arm64, pci, acpi: ACPI support for legacy IRQs parsing and consolidation with DT code. Tomasz Nowicki <tn@semihalf.com> - 2016-04-15 19:20 +0200
      Re: [PATCH V6 06/13] arm64, pci, acpi: ACPI support for legacy IRQs  parsing and consolidation with DT code. Bjorn Helgaas <helgaas@kernel.org> - 2016-04-27 04:50 +0200
        Re: [PATCH V6 06/13] arm64, pci, acpi: ACPI support for legacy IRQs  parsing and consolidation with DT code. Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2016-04-27 13:50 +0200
    [PATCH V6 07/13] PCI: Provide common functions for ECAM mapping Tomasz Nowicki <tn@semihalf.com> - 2016-04-15 19:20 +0200
      Re: [PATCH V6 07/13] PCI: Provide common functions for ECAM mapping Arnd Bergmann <arnd@arndb.de> - 2016-04-15 20:50 +0200
    [PATCH V6 01/13] pci, acpi, x86, ia64: Move ACPI host bridge device companion assignment to core code. Tomasz Nowicki <tn@semihalf.com> - 2016-04-15 19:20 +0200
      Re: [PATCH V6 01/13] pci, acpi, x86, ia64: Move ACPI host bridge  device companion assignment to core code. Bjorn Helgaas <helgaas@kernel.org> - 2016-04-27 00:40 +0200
        Re: [PATCH V6 01/13] pci, acpi, x86, ia64: Move ACPI host bridge  device companion assignment to core code. Tomasz Nowicki <tn@semihalf.com> - 2016-04-27 12:20 +0200
      Re: [PATCH V6 01/13] pci, acpi, x86, ia64: Move ACPI host bridge  device companion assignment to core code. Bjorn Helgaas <helgaas@kernel.org> - 2016-04-27 04:50 +0200
    [PATCH V6 03/13] x86, ia64: Include acpi_pci_{add|remove}_bus to the default pcibios_{add|remove}_bus implementation. Tomasz Nowicki <tn@semihalf.com> - 2016-04-15 19:20 +0200
      Re: [PATCH V6 03/13] x86, ia64: Include acpi_pci_{add|remove}_bus to  the default pcibios_{add|remove}_bus implementation. Bjorn Helgaas <helgaas@kernel.org> - 2016-04-27 04:40 +0200
        Re: [PATCH V6 03/13] x86, ia64: Include acpi_pci_{add|remove}_bus to  the default pcibios_{add|remove}_bus implementation. Tomasz Nowicki <tn@semihalf.com> - 2016-04-27 15:30 +0200
    [PATCH V6 05/13] acpi, pci: Support IO resources when parsing PCI host bridge resources. Tomasz Nowicki <tn@semihalf.com> - 2016-04-15 19:20 +0200
      Re: [PATCH V6 05/13] acpi, pci: Support IO resources when parsing  PCI host bridge resources. Bjorn Helgaas <helgaas@kernel.org> - 2016-04-27 04:40 +0200
        Re: [PATCH V6 05/13] acpi, pci: Support IO resources when parsing PCI  host bridge resources. Jon Masters <jcm@redhat.com> - 2016-04-27 07:40 +0200
        Re: [PATCH V6 05/13] acpi, pci: Support IO resources when parsing  PCI host bridge resources. Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2016-04-27 16:30 +0200
          Re: [PATCH V6 05/13] acpi, pci: Support IO resources when parsing  PCI host bridge resources. Liviu.Dudau@arm.com - 2016-04-27 17:20 +0200
            Re: [PATCH V6 05/13] acpi, pci: Support IO resources when parsing  PCI host bridge resources. Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2016-04-27 18:10 +0200
    Re: [PATCH V6 00/13] Support for generic ACPI based PCI host  controller Jon Masters <jcm@redhat.com> - 2016-04-15 20:20 +0200
      Re: [PATCH V6 00/13] Support for generic ACPI based PCI host controller Jayachandran C <jchandra@broadcom.com> - 2016-04-16 17:40 +0200
        Re: [PATCH V6 00/13] Support for generic ACPI based PCI host  controller Tomasz Nowicki <tn@semihalf.com> - 2016-04-18 15:40 +0200
          Re: [PATCH V6 00/13] Support for generic ACPI based PCI host controller Arnd Bergmann <arnd@arndb.de> - 2016-04-18 16:40 +0200
            Re: [PATCH V6 00/13] Support for generic ACPI based PCI host  controller Tomasz Nowicki <tn@semihalf.com> - 2016-04-18 17:30 +0200
      Re: [PATCH V6 00/13] Support for generic ACPI based PCI host controller Martinez Kristofer <kristofer.s.martinez@gmail.com> - 2016-04-17 11:30 +0200
    Re: [PATCH V6 00/13] Support for generic ACPI based PCI host controller Duc Dang <dhdang@apm.com> - 2016-04-16 20:40 +0200
    Re: [PATCH V6 00/13] Support for generic ACPI based PCI host  controller Sinan Kaya <okaya@codeaurora.org> - 2016-04-17 06:20 +0200
    Re: [PATCH V6 00/13] Support for generic ACPI based PCI host  controller Jeremy Linton <jeremy.linton@arm.com> - 2016-04-25 19:30 +0200

Page 2 of 4 — ← Prev page 1 [2] 3 4  Next page →

#1380495 — Re: [PATCH V6 08/13] PCI: generic, thunder: update to use generic ECAM API

On Sat, Apr 16, 2016 at 12:09 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 15 April 2016 19:06:43 Tomasz Nowicki wrote:
>> -MODULE_DEVICE_TABLE(of, thunder_pem_of_match);
>> -
>> -static int thunder_pem_probe(struct platform_device *pdev)
>> +static int thunder_pem_init(struct device *dev, struct pci_config_window *cfg)
>>  {
>> -       struct device *dev = &pdev->dev;
>> -       const struct of_device_id *of_id;
>>         resource_size_t bar4_start;
>>         struct resource *res_pem;
>>         struct thunder_pem_pci *pem_pci;
>> +       struct platform_device *pdev;
>>
>>         pem_pci = devm_kzalloc(dev, sizeof(*pem_pci), GFP_KERNEL);
>>         if (!pem_pci)
>>                 return -ENOMEM;
>>
>> -       of_id = of_match_node(thunder_pem_of_match, dev->of_node);
>> -       pem_pci->gen_pci.cfg.ops = (struct gen_pci_cfg_bus_ops *)of_id->data;
>> +       pdev = to_platform_device(dev);
>>
>>         /*
>>          * The second register range is the PEM bridge to the PCIe
>> @@ -330,7 +298,29 @@ static int thunder_pem_probe(struct platform_device *pdev)
>>         pem_pci->ea_entry[1] = (u32)(res_pem->end - bar4_start) & ~3u;
>>         pem_pci->ea_entry[2] = (u32)(bar4_start >> 32);
>>
>> -       return pci_host_common_probe(pdev, &pem_pci->gen_pci);
>> +       cfg->priv = pem_pci;
>> +       return 0;
>> +}
>> +
>>
>
> I still think it would be better to keep the loadable PCI host drivers
> separate from the ACPI PCI infrastructure. There are a number of
> simplifications that we want to do to the DT based drivers in the long
> run, so it's better if that code is not shared at this level. Abstracting
> out the ECAM code is fine, but at that point you should be able to just
> call it from the ACPI layer.

The issue is not with this patch (in my opinion). This patch is just
re-arranging how thunder specific data is maintained. Earlier it was
a container_of gen_pci, now it is ->priv of pci_config_window.

I can see the issue in patches 12 and 13 of this patchset which adds
ACPI fixups into the thunder OF driver.

The simple approach when doing modular PCI drivers would be to make
pci-thunder-*.c like pci-host-common.c, to be compiled in if configured.
The fie will contain all the Thunder quirks and can export
pci_thunder_ecam_ops.

Then the OF driver part will be trivial and can be merged into
pci-host-generic.c which can be a module. The ACPI hooks can be
moved to the ACPI PCI host driver file.

Would appreciate any suggestions on the way forward.

Thanks,
JC.

[toc] | [prev] | [next] | [standalone]

#1380496 — Re: [PATCH V6 08/13] PCI: generic, thunder: update to use generic ECAM API

On Saturday 16 April 2016 12:50:13 Jayachandran C wrote:
> >
> > I still think it would be better to keep the loadable PCI host drivers
> > separate from the ACPI PCI infrastructure. There are a number of
> > simplifications that we want to do to the DT based drivers in the long
> > run, so it's better if that code is not shared at this level. Abstracting
> > out the ECAM code is fine, but at that point you should be able to just
> > call it from the ACPI layer.
> 
> The issue is not with this patch (in my opinion). This patch is just
> re-arranging how thunder specific data is maintained. Earlier it was
> a container_of gen_pci, now it is ->priv of pci_config_window.
> 
> I can see the issue in patches 12 and 13 of this patchset which adds
> ACPI fixups into the thunder OF driver.

Right, I commented on this one, because it seems to rearrange the code
in order to do the later one.

> The simple approach when doing modular PCI drivers would be to make
> pci-thunder-*.c like pci-host-common.c, to be compiled in if configured.
> The fie will contain all the Thunder quirks and can export
> pci_thunder_ecam_ops.

I would argue that we should not export anything from drivers/pci/host,
those should really be standalone drivers that do not interact with other
subsystems.

How much code would you need to duplicate from thunder-ecam to have
the same functionality available in ACPI? My expectation is that it's
not really that much more compared to the code you need for sharing
a single implementation, but you get a lower complexity here, which
makes it easier to understand and to rework.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1380557 — Re: [PATCH V6 08/13] PCI: generic, thunder: update to use generic ECAM API

On Sat, Apr 16, 2016 at 1:01 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Saturday 16 April 2016 12:50:13 Jayachandran C wrote:
>> >
>> > I still think it would be better to keep the loadable PCI host drivers
>> > separate from the ACPI PCI infrastructure. There are a number of
>> > simplifications that we want to do to the DT based drivers in the long
>> > run, so it's better if that code is not shared at this level. Abstracting
>> > out the ECAM code is fine, but at that point you should be able to just
>> > call it from the ACPI layer.
>>
>> The issue is not with this patch (in my opinion). This patch is just
>> re-arranging how thunder specific data is maintained. Earlier it was
>> a container_of gen_pci, now it is ->priv of pci_config_window.
>>
>> I can see the issue in patches 12 and 13 of this patchset which adds
>> ACPI fixups into the thunder OF driver.
>
> Right, I commented on this one, because it seems to rearrange the code
> in order to do the later one.

Patches 11- 13 are not from me, and I am not completely on board
on the approach of adding the sections.We can look at reworking this.

>> The simple approach when doing modular PCI drivers would be to make
>> pci-thunder-*.c like pci-host-common.c, to be compiled in if configured.
>> The fie will contain all the Thunder quirks and can export
>> pci_thunder_ecam_ops.
>
> I would argue that we should not export anything from drivers/pci/host,
> those should really be standalone drivers that do not interact with other
> subsystems.

pci-host-common.c goes against being standalone. The files calling
pci_host_common_probe() are expected to have custom ECAM ops
the way it is written now. We need to have a reasonable way to share
those ECAM ops if needed by ACPI.

> How much code would you need to duplicate from thunder-ecam to have
> the same functionality available in ACPI? My expectation is that it's
> not really that much more compared to the code you need for sharing
> a single implementation, but you get a lower complexity here, which
> makes it easier to understand and to rework.

Like I wrote above, the sharing is really simple because both generic
ACPI and pci-host-common.c have been written for "ECAM with quirks".

The whole pci-thunder-*.c is to support thunder PCI quirks since the
generic OF is handled by pci-host-common.c and generic ECAM is now
separated -  duplicating the whole file for ACPI will be bad.

Any suggestions on how to do this better would be really welcome.

Thanks,
JC.

[toc] | [prev] | [next] | [standalone]

#1381674 — Re: [PATCH V6 08/13] PCI: generic, thunder: update to use generic ECAM API

On 16.04.2016 16:36, Jayachandran C wrote:
> On Sat, Apr 16, 2016 at 1:01 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Saturday 16 April 2016 12:50:13 Jayachandran C wrote:
>>>>
>>>> I still think it would be better to keep the loadable PCI host drivers
>>>> separate from the ACPI PCI infrastructure. There are a number of
>>>> simplifications that we want to do to the DT based drivers in the long
>>>> run, so it's better if that code is not shared at this level. Abstracting
>>>> out the ECAM code is fine, but at that point you should be able to just
>>>> call it from the ACPI layer.
>>>
>>> The issue is not with this patch (in my opinion). This patch is just
>>> re-arranging how thunder specific data is maintained. Earlier it was
>>> a container_of gen_pci, now it is ->priv of pci_config_window.
>>>
>>> I can see the issue in patches 12 and 13 of this patchset which adds
>>> ACPI fixups into the thunder OF driver.
>>
>> Right, I commented on this one, because it seems to rearrange the code
>> in order to do the later one.
>
> Patches 11- 13 are not from me, and I am not completely on board
> on the approach of adding the sections.We can look at reworking this.
>
>>> The simple approach when doing modular PCI drivers would be to make
>>> pci-thunder-*.c like pci-host-common.c, to be compiled in if configured.
>>> The fie will contain all the Thunder quirks and can export
>>> pci_thunder_ecam_ops.
>>
>> I would argue that we should not export anything from drivers/pci/host,
>> those should really be standalone drivers that do not interact with other
>> subsystems.
>
> pci-host-common.c goes against being standalone. The files calling
> pci_host_common_probe() are expected to have custom ECAM ops
> the way it is written now. We need to have a reasonable way to share
> those ECAM ops if needed by ACPI.
>
>> How much code would you need to duplicate from thunder-ecam to have
>> the same functionality available in ACPI? My expectation is that it's
>> not really that much more compared to the code you need for sharing
>> a single implementation, but you get a lower complexity here, which
>> makes it easier to understand and to rework.
>
> Like I wrote above, the sharing is really simple because both generic
> ACPI and pci-host-common.c have been written for "ECAM with quirks".
>
> The whole pci-thunder-*.c is to support thunder PCI quirks since the
> generic OF is handled by pci-host-common.c and generic ECAM is now
> separated -  duplicating the whole file for ACPI will be bad.

Yes, it would be too much code duplication. Also, we already know 
drivers which need quirks.

We really need to agree on best approach here. Here are requirements 
which came up (please correct me if misunderstood sth):

Arnd:
1. Initial DT driver should be standalone [Arnd]
2. No exported symbols [Arnd]
3. Duplicate necessary code to ACPI framework.

JC:
1. Adding linker section is wrong.
2. Quirks should be exported (pci_thunder_ecam_ops), then no need for 
adding linker section
3. To much duplication to copy code into the ACPI framework.

My opinion:
1. I like linker section because it is easy to maintain and no need to 
export symbols.
2. We need more sophisticated algorithm for matching quirks (DMI is not 
enough and not only for ThunderX drivers). Of course I am open to any 
new suggestions.
3. To much duplication to copy code into the ACPI framework.

Thanks in advance for any pointers.

Thanks,
Tomasz

[toc] | [prev] | [next] | [standalone]

#1381772 — Re: [PATCH V6 08/13] PCI: generic, thunder: update to use generic ECAM API

On Monday 18 April 2016 15:03:51 Tomasz Nowicki wrote:
> On 16.04.2016 16:36, Jayachandran C wrote:
> > On Sat, Apr 16, 2016 at 1:01 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> >> On Saturday 16 April 2016 12:50:13 Jayachandran C wrote:
> > The whole pci-thunder-*.c is to support thunder PCI quirks since the
> > generic OF is handled by pci-host-common.c and generic ECAM is now
> > separated -  duplicating the whole file for ACPI will be bad.
> 
> Yes, it would be too much code duplication. Also, we already know 
> drivers which need quirks.
> 
> We really need to agree on best approach here. Here are requirements 
> which came up (please correct me if misunderstood sth):
> 
> Arnd:
> 1. Initial DT driver should be standalone [Arnd]
> 2. No exported symbols [Arnd]
> 3. Duplicate necessary code to ACPI framework.

Correct.

> JC:
> 1. Adding linker section is wrong.
> 2. Quirks should be exported (pci_thunder_ecam_ops), then no need for 
> adding linker section
> 3. To much duplication to copy code into the ACPI framework.
> 
> My opinion:
> 1. I like linker section because it is easy to maintain and no need to 
> export symbols.
> 2. We need more sophisticated algorithm for matching quirks (DMI is not 
> enough and not only for ThunderX drivers). Of course I am open to any 
> new suggestions.

Agreed.

> 3. To much duplication to copy code into the ACPI framework.
> 
> Thanks in advance for any pointers.

Can you be more specific about what code actually would need to
be duplicated? Anything besides the config space operations?

	Arnd

[toc] | [prev] | [next] | [standalone]

#1382001 — Re: [PATCH V6 08/13] PCI: generic, thunder: update to use generic ECAM API

On 18.04.2016 16:44, Arnd Bergmann wrote:
> On Monday 18 April 2016 15:03:51 Tomasz Nowicki wrote:
>> On 16.04.2016 16:36, Jayachandran C wrote:
>>> On Sat, Apr 16, 2016 at 1:01 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>>> On Saturday 16 April 2016 12:50:13 Jayachandran C wrote:
>>> The whole pci-thunder-*.c is to support thunder PCI quirks since the
>>> generic OF is handled by pci-host-common.c and generic ECAM is now
>>> separated -  duplicating the whole file for ACPI will be bad.
>>
>> Yes, it would be too much code duplication. Also, we already know
>> drivers which need quirks.
>>
>> We really need to agree on best approach here. Here are requirements
>> which came up (please correct me if misunderstood sth):
>>
>> Arnd:
>> 1. Initial DT driver should be standalone [Arnd]
>> 2. No exported symbols [Arnd]
>> 3. Duplicate necessary code to ACPI framework.
>
> Correct.
>
>> JC:
>> 1. Adding linker section is wrong.
>> 2. Quirks should be exported (pci_thunder_ecam_ops), then no need for
>> adding linker section
>> 3. To much duplication to copy code into the ACPI framework.
>>
>> My opinion:
>> 1. I like linker section because it is easy to maintain and no need to
>> export symbols.
>> 2. We need more sophisticated algorithm for matching quirks (DMI is not
>> enough and not only for ThunderX drivers). Of course I am open to any
>> new suggestions.
>
> Agreed.
>
>> 3. To much duplication to copy code into the ACPI framework.
>>
>> Thanks in advance for any pointers.
>
> Can you be more specific about what code actually would need to
> be duplicated? Anything besides the config space operations?
>

Basically the whole content of pci-thunder-ecam.c and pci-thunder-pem.c.

pci-thunder-ecam.c contains config space accessors. Similar for 
pci-thunder-pem.c but it also has extra init call (it is now called 
thunder_pem_init) which finds and maps related registers.

Thanks,
Tomasz

[toc] | [prev] | [next] | [standalone]

#1382491 — Re: [PATCH V6 08/13] PCI: generic, thunder: update to use generic ECAM API

On Monday 18 April 2016 21:31:54 Tomasz Nowicki wrote:
> 
> Basically the whole content of pci-thunder-ecam.c and pci-thunder-pem.c.
> 
> pci-thunder-ecam.c contains config space accessors. Similar for 
> pci-thunder-pem.c but it also has extra init call (it is now called 
> thunder_pem_init) which finds and maps related registers.

They seem to do much more than just override the accessors, they actually
change the contents of the config space as well. Is that really necessary
on ACPI based systems as well?

Another idea: how about moving all of this logic into ACPI and calling
some AML method to access the config space if the devices are that
far out of spec.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1383990 — Re: [PATCH V6 08/13] PCI: generic, thunder: update to use generic ECAM API

On 19.04.2016 15:06, Arnd Bergmann wrote:
> On Monday 18 April 2016 21:31:54 Tomasz Nowicki wrote:
>>
>> Basically the whole content of pci-thunder-ecam.c and pci-thunder-pem.c.
>>
>> pci-thunder-ecam.c contains config space accessors. Similar for
>> pci-thunder-pem.c but it also has extra init call (it is now called
>> thunder_pem_init) which finds and maps related registers.
>
> They seem to do much more than just override the accessors, they actually
> change the contents of the config space as well. Is that really necessary
> on ACPI based systems as well?

Yes, the pci-thunder-ecam.c accessors are meant to emulate config space 
capabilities. They are necessary to synthesize EA capabilities (fixed 
PCI BARs), it wont work without this, for ACPI boot as well.

>
> Another idea: how about moving all of this logic into ACPI and calling
> some AML method to access the config space if the devices are that
> far out of spec.

Do you mean Linux specific way to call non-standard config space 
accessors? Then non-standard accessors are going to AML methods which 
are called from common code which handles quirks via unified API ?

Thanks,
Tomasz

[toc] | [prev] | [next] | [standalone]

#1384012 — Re: [PATCH V6 08/13] PCI: generic, thunder: update to use generic ECAM API

On Thursday 21 April 2016 11:28:15 Tomasz Nowicki wrote:
> On 19.04.2016 15:06, Arnd Bergmann wrote:
> > On Monday 18 April 2016 21:31:54 Tomasz Nowicki wrote:
> >>
> >> Basically the whole content of pci-thunder-ecam.c and pci-thunder-pem.c.
> >>
> >> pci-thunder-ecam.c contains config space accessors. Similar for
> >> pci-thunder-pem.c but it also has extra init call (it is now called
> >> thunder_pem_init) which finds and maps related registers.
> >
> > They seem to do much more than just override the accessors, they actually
> > change the contents of the config space as well. Is that really necessary
> > on ACPI based systems as well?
> 
> Yes, the pci-thunder-ecam.c accessors are meant to emulate config space 
> capabilities. They are necessary to synthesize EA capabilities (fixed 
> PCI BARs), it wont work without this, for ACPI boot as well.

Why is that? I thought the BARs never get reassigned when using ACPI,
so I'm surprised it's actually needed. Maybe I misunderstood what
you mean by fixed PCI BARs.

> > Another idea: how about moving all of this logic into ACPI and calling
> > some AML method to access the config space if the devices are that
> > far out of spec.
> 
> Do you mean Linux specific way to call non-standard config space 
> accessors? Then non-standard accessors are going to AML methods which 
> are called from common code which handles quirks via unified API ?

What I really meant was a standardized way to do handle hardware that
is in some way or another not compliant with PNP0A08: We could have
a different hardware ID for this and let all the first-generation
ARM servers and also anything else using ACPI with nonstandard PCI
use the same method across operating systems.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1384035 — Re: [PATCH V6 08/13] PCI: generic, thunder: update to use generic ECAM API

On 21.04.2016 11:36, Arnd Bergmann wrote:
> On Thursday 21 April 2016 11:28:15 Tomasz Nowicki wrote:
>> On 19.04.2016 15:06, Arnd Bergmann wrote:
>>> On Monday 18 April 2016 21:31:54 Tomasz Nowicki wrote:
>>>>
>>>> Basically the whole content of pci-thunder-ecam.c and pci-thunder-pem.c.
>>>>
>>>> pci-thunder-ecam.c contains config space accessors. Similar for
>>>> pci-thunder-pem.c but it also has extra init call (it is now called
>>>> thunder_pem_init) which finds and maps related registers.
>>>
>>> They seem to do much more than just override the accessors, they actually
>>> change the contents of the config space as well. Is that really necessary
>>> on ACPI based systems as well?
>>
>> Yes, the pci-thunder-ecam.c accessors are meant to emulate config space
>> capabilities. They are necessary to synthesize EA capabilities (fixed
>> PCI BARs), it wont work without this, for ACPI boot as well.
>
> Why is that? I thought the BARs never get reassigned when using ACPI,
> so I'm surprised it's actually needed. Maybe I misunderstood what
> you mean by fixed PCI BARs.

Yes, I meant something else. ThunderX has non-programmable PCI BAR 
addresses. So it uses PCI EA (Extended allocation) capabilities to get 
know PCI BARs addresses. But the early implementation (pass1.x) misses 
EA capabilities hence we need to emulate it in config space accessors.

Tomasz

[toc] | [prev] | [next] | [standalone]

#1385234 — Re: [PATCH V6 08/13] PCI: generic, thunder: update to use generic ECAM API

On 04/21/2016 06:08 AM, Tomasz Nowicki wrote:
> On 21.04.2016 11:36, Arnd Bergmann wrote:
>> On Thursday 21 April 2016 11:28:15 Tomasz Nowicki wrote:
>>> On 19.04.2016 15:06, Arnd Bergmann wrote:
>>>> On Monday 18 April 2016 21:31:54 Tomasz Nowicki wrote:
>>>>>
>>>>> Basically the whole content of pci-thunder-ecam.c and
>>>>> pci-thunder-pem.c.
>>>>>
>>>>> pci-thunder-ecam.c contains config space accessors. Similar for
>>>>> pci-thunder-pem.c but it also has extra init call (it is now called
>>>>> thunder_pem_init) which finds and maps related registers.
>>>>
>>>> They seem to do much more than just override the accessors, they
>>>> actually
>>>> change the contents of the config space as well. Is that really
>>>> necessary
>>>> on ACPI based systems as well?
>>>
>>> Yes, the pci-thunder-ecam.c accessors are meant to emulate config space
>>> capabilities. They are necessary to synthesize EA capabilities (fixed
>>> PCI BARs), it wont work without this, for ACPI boot as well.
>>
>> Why is that? I thought the BARs never get reassigned when using ACPI,
>> so I'm surprised it's actually needed. Maybe I misunderstood what
>> you mean by fixed PCI BARs.
> 
> Yes, I meant something else. ThunderX has non-programmable PCI BAR
> addresses. So it uses PCI EA (Extended allocation) capabilities to get
> know PCI BARs addresses. But the early implementation (pass1.x) misses
> EA capabilities hence we need to emulate it in config space accessors.

Aside: In case it's helpful, at least one enterprise vendor I know of is
only supporting later silicon as a result of this. So IMO there's no
need to worry about this issue on the early preproduction chips.

-- 
Computer Architect | Sent from my Fedora powered laptop

[toc] | [prev] | [next] | [standalone]

#1385328 — Re: [PATCH V6 08/13] PCI: generic, thunder: update to use generic ECAM API

On 04/22/2016 07:30 AM, Jon Masters wrote:
> On 04/21/2016 06:08 AM, Tomasz Nowicki wrote:
>> On 21.04.2016 11:36, Arnd Bergmann wrote:
>>> On Thursday 21 April 2016 11:28:15 Tomasz Nowicki wrote:
>>>> On 19.04.2016 15:06, Arnd Bergmann wrote:
>>>>> On Monday 18 April 2016 21:31:54 Tomasz Nowicki wrote:
>>>>>>
>>>>>> Basically the whole content of pci-thunder-ecam.c and
>>>>>> pci-thunder-pem.c.
>>>>>>
>>>>>> pci-thunder-ecam.c contains config space accessors. Similar for
>>>>>> pci-thunder-pem.c but it also has extra init call (it is now called
>>>>>> thunder_pem_init) which finds and maps related registers.
>>>>>
>>>>> They seem to do much more than just override the accessors, they
>>>>> actually
>>>>> change the contents of the config space as well. Is that really
>>>>> necessary
>>>>> on ACPI based systems as well?
>>>>
>>>> Yes, the pci-thunder-ecam.c accessors are meant to emulate config space
>>>> capabilities. They are necessary to synthesize EA capabilities (fixed
>>>> PCI BARs), it wont work without this, for ACPI boot as well.
>>>
>>> Why is that? I thought the BARs never get reassigned when using ACPI,
>>> so I'm surprised it's actually needed. Maybe I misunderstood what
>>> you mean by fixed PCI BARs.
>>
>> Yes, I meant something else. ThunderX has non-programmable PCI BAR
>> addresses. So it uses PCI EA (Extended allocation) capabilities to get
>> know PCI BARs addresses. But the early implementation (pass1.x) misses
>> EA capabilities hence we need to emulate it in config space accessors.
>
> Aside: In case it's helpful, at least one enterprise vendor I know of is
> only supporting later silicon as a result of this. So IMO there's no
> need to worry about this issue on the early preproduction chips.
>

There are two separate issues that make fixing up the ECAM space necessary:

1) As Jon mentioned, preproduction silicon lacks EA capabilities.

2) On 2-node NUMA systems, the EA capabilities of some devices may be 
incorrect even in production silicon.

In general, the strategy we use for dealing with both of these is to 
hook into the ECAM access methods, and supply corrected config space 
data.  For the case of device-tree provisioned ECAM access, the fix ups 
are done in pci-thunder-ecam.c.  This code is already present and seems 
to be working well.

As we consider ACPI support, supporting case #2 above will be desirable. 
  If we reuse the code in pci-thunder-ecam.c for this, we will probably 
get support for #1 for free.

David Daney

[toc] | [prev] | [next] | [standalone]

#1382859 — Re: [PATCH V6 08/13] PCI: generic, thunder: update to use generic ECAM API

On Friday 15 April 2016 19:06:43 Tomasz Nowicki wrote:
> From: Jayachandran C <jchandra@broadcom.com>
>
> Add config option PCI_GENERIC_ECAM and file drivers/pci/ecam.c to
> provide generic functions for accessing memory mapped PCI config space.
>
> The API is defined in drivers/pci/ecam.h and is written to replace the
> API in drivers/pci/host/pci-host-common.h. The file defines a new
> 'struct pci_config_window' to hold the information related to a PCI
> config area and its mapping. This structure is expected to be used as
> sysdata for controllers that have ECAM based mapping.
>
> Helper functions are provided to setup the mapping, free the mapping
> and to implement the map_bus method in 'struct pci_ops'
>
> Signed-off-by: Jayachandran C <jchandra@broadcom.com>

I've taken a fresh look now at what is going on here.

> @@ -58,4 +58,9 @@ void __iomem *pci_generic_ecam_map_bus(struct pci_bus *bus, unsigned int devfn,
>  /* default ECAM ops, bus shift 20, generic read and write */
>  extern struct pci_generic_ecam_ops pci_generic_ecam_default_ops;
>  
> +#ifdef CONFIG_PCI_HOST_GENERIC
> +/* for DT based pci controllers that support ECAM */
> +int pci_host_common_probe(struct platform_device *pdev,
> +			  struct pci_generic_ecam_ops *ops);
> +#endif
>  #endif

This doesn't seem to belong here: just leave the declaration
in the existing file. 

> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index 7a0780d..31d6eb5 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -82,6 +82,7 @@ config PCI_HOST_GENERIC
>  	bool "Generic PCI host controller"
>  	depends on (ARM || ARM64) && OF
>  	select PCI_HOST_COMMON
> +	select PCI_GENERIC_ECAM
>  	help
>  	  Say Y here if you want to support a simple generic PCI host
>  	  controller, such as the one emulated by kvmtool.
> diff --git a/drivers/pci/host/pci-host-common.c b/drivers/pci/host/pci-host-common.c
> index e9f850f..99d99b3 100644
> --- a/drivers/pci/host/pci-host-common.c
> +++ b/drivers/pci/host/pci-host-common.c
> @@ -22,27 +22,21 @@
>  #include <linux/of_pci.h>
>  #include <linux/platform_device.h>
>  
> -#include "pci-host-common.h"
> +#include "../ecam.h"

As mentioned, don't use headers from parent directories, anything
that needs to be shared must go into include/linux, while the parts
that are only needed in one directory should be declared there.

> -static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
> +static void gen_pci_generic_unmap_cfg(void *ptr)
> +{
> +	pci_generic_ecam_free((struct pci_config_window *)ptr);
> +}

Why the void pointer?

> +static struct pci_generic_ecam_ops pci_thunder_pem_ops = {
> +	.bus_shift	= 24,
> +	.init		= thunder_pem_init,
> +	.pci_ops	= {
> +		.map_bus	= pci_generic_ecam_map_bus,
> +		.read		= thunder_pem_config_read,
> +		.write		= thunder_pem_config_write,
> +	}
> +};

Adding the callback pointer for init here and yet another structure
pci_config_window really seems to go too far with the number of
abstraction levels.

I think here it makes much more sense to just implement ECAM pci_ops
in ACPI separately, as the implementation really trivial to start with,
and all the complexity comes just from trying to share it with other
stuff. Doesn't ACPI already have an ECAM implementation for x86
that you could simply use?

	Arnd

[toc] | [prev] | [next] | [standalone]

#1382924 — Re: [PATCH V6 08/13] PCI: generic, thunder: update to use generic ECAM API

On Wed, Apr 20, 2016 at 3:10 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 15 April 2016 19:06:43 Tomasz Nowicki wrote:
>> From: Jayachandran C <jchandra@broadcom.com>
>>
>> Add config option PCI_GENERIC_ECAM and file drivers/pci/ecam.c to
>> provide generic functions for accessing memory mapped PCI config space.
>>
>> The API is defined in drivers/pci/ecam.h and is written to replace the
>> API in drivers/pci/host/pci-host-common.h. The file defines a new
>> 'struct pci_config_window' to hold the information related to a PCI
>> config area and its mapping. This structure is expected to be used as
>> sysdata for controllers that have ECAM based mapping.
>>
>> Helper functions are provided to setup the mapping, free the mapping
>> and to implement the map_bus method in 'struct pci_ops'
>>
>> Signed-off-by: Jayachandran C <jchandra@broadcom.com>
>
> I've taken a fresh look now at what is going on here.
>
>> @@ -58,4 +58,9 @@ void __iomem *pci_generic_ecam_map_bus(struct pci_bus *bus, unsigned int devfn,
>>  /* default ECAM ops, bus shift 20, generic read and write */
>>  extern struct pci_generic_ecam_ops pci_generic_ecam_default_ops;
>>
>> +#ifdef CONFIG_PCI_HOST_GENERIC
>> +/* for DT based pci controllers that support ECAM */
>> +int pci_host_common_probe(struct platform_device *pdev,
>> +                       struct pci_generic_ecam_ops *ops);
>> +#endif
>>  #endif
>
> This doesn't seem to belong here: just leave the declaration
> in the existing file.

This can be done, the file would just have one line so I thought
it made sense to move it to ecam.h where the struct is defined.

>> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
>> index 7a0780d..31d6eb5 100644
>> --- a/drivers/pci/host/Kconfig
>> +++ b/drivers/pci/host/Kconfig
>> @@ -82,6 +82,7 @@ config PCI_HOST_GENERIC
>>       bool "Generic PCI host controller"
>>       depends on (ARM || ARM64) && OF
>>       select PCI_HOST_COMMON
>> +     select PCI_GENERIC_ECAM
>>       help
>>         Say Y here if you want to support a simple generic PCI host
>>         controller, such as the one emulated by kvmtool.
>> diff --git a/drivers/pci/host/pci-host-common.c b/drivers/pci/host/pci-host-common.c
>> index e9f850f..99d99b3 100644
>> --- a/drivers/pci/host/pci-host-common.c
>> +++ b/drivers/pci/host/pci-host-common.c
>> @@ -22,27 +22,21 @@
>>  #include <linux/of_pci.h>
>>  #include <linux/platform_device.h>
>>
>> -#include "pci-host-common.h"
>> +#include "../ecam.h"
>
> As mentioned, don't use headers from parent directories, anything
> that needs to be shared must go into include/linux, while the parts
> that are only needed in one directory should be declared there.

This is also ok - It can either go to pci.h or a separate pci-ecam.h

>> -static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
>> +static void gen_pci_generic_unmap_cfg(void *ptr)
>> +{
>> +     pci_generic_ecam_free((struct pci_config_window *)ptr);
>> +}
>
> Why the void pointer?

devm_add_action() needs it.

>> +static struct pci_generic_ecam_ops pci_thunder_pem_ops = {
>> +     .bus_shift      = 24,
>> +     .init           = thunder_pem_init,
>> +     .pci_ops        = {
>> +             .map_bus        = pci_generic_ecam_map_bus,
>> +             .read           = thunder_pem_config_read,
>> +             .write          = thunder_pem_config_write,
>> +     }
>> +};
>
> Adding the callback pointer for init here and yet another structure
> pci_config_window really seems to go too far with the number of
> abstraction levels.

The abstraction was already there in pci-host-common.h for
ops for ECAM/CAM based controllers. It made sense to move it
to ecam.h and use it for ECAM based ACPI [1].

We need to pass pci_ops, bus_shift and an additional pointer
for quirks for ECAM based host controllers. Having it as a
structure pci_generic_ecam_ops reduces the function arguments,
and also keeps most of the older API.

> I think here it makes much more sense to just implement ECAM pci_ops
> in ACPI separately, as the implementation really trivial to start with,
> and all the complexity comes just from trying to share it with other
> stuff. Doesn't ACPI already have an ECAM implementation for x86
> that you could simply use?

The implementation is extremely trivial on 64 bit, and slightly more
complex in 32bit (pci-host-common.c per bus mapping and set_pte
based mapping on  x86). The generic ACPI on 64 bit is very simple
if there are no quirks,I have already posted that [2] some time back.

ACPI on x86 also has a 32 bit and a 64 bit version
(arch/x86/pci/mmconfig_{32,64}.c}. The code there is a bit messed
up and it does not make sense to share or reuse that.

There has been suggestions earlier from Bjorn on sharing the
ECAM implementation[1], which was the starting point of
doing this patch.

Overall, this patch improves config window mapping for
pci-host-common.c based drivers on 64 bit and deletes
quite a bit of duplicated code. I would argue that this makes
sense even without ACPI.

JC.

[1] https://lkml.org/lkml/2016/3/3/921
[2] http://article.gmane.org/gmane.linux.kernel.pci/47753

[toc] | [prev] | [next] | [standalone]

#1380030 — [PATCH V6 04/13] pci, of: Move the PCI I/O space management to PCI core code.

No functional changes in this patch.

PCI I/O space mapping code does not depend on OF, therefore it can be
moved to PCI core code. This way we will be able to use it
e.g. in ACPI PCI code.

Suggested-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
CC: Arnd Bergmann <arnd@arndb.de>
CC: Liviu Dudau <Liviu.Dudau@arm.com>
CC: Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>
---
 drivers/of/address.c       | 116 +--------------------------------------------
 drivers/pci/pci.c          | 115 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_address.h |   9 ----
 include/linux/pci.h        |   5 ++
 4 files changed, 121 insertions(+), 124 deletions(-)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index 91a469d..0a553c0 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -4,6 +4,7 @@
 #include <linux/ioport.h>
 #include <linux/module.h>
 #include <linux/of_address.h>
+#include <linux/pci.h>
 #include <linux/pci_regs.h>
 #include <linux/sizes.h>
 #include <linux/slab.h>
@@ -673,121 +674,6 @@ const __be32 *of_get_address(struct device_node *dev, int index, u64 *size,
 }
 EXPORT_SYMBOL(of_get_address);
 
-#ifdef PCI_IOBASE
-struct io_range {
-	struct list_head list;
-	phys_addr_t start;
-	resource_size_t size;
-};
-
-static LIST_HEAD(io_range_list);
-static DEFINE_SPINLOCK(io_range_lock);
-#endif
-
-/*
- * Record the PCI IO range (expressed as CPU physical address + size).
- * Return a negative value if an error has occured, zero otherwise
- */
-int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size)
-{
-	int err = 0;
-
-#ifdef PCI_IOBASE
-	struct io_range *range;
-	resource_size_t allocated_size = 0;
-
-	/* check if the range hasn't been previously recorded */
-	spin_lock(&io_range_lock);
-	list_for_each_entry(range, &io_range_list, list) {
-		if (addr >= range->start && addr + size <= range->start + size) {
-			/* range already registered, bail out */
-			goto end_register;
-		}
-		allocated_size += range->size;
-	}
-
-	/* range not registed yet, check for available space */
-	if (allocated_size + size - 1 > IO_SPACE_LIMIT) {
-		/* if it's too big check if 64K space can be reserved */
-		if (allocated_size + SZ_64K - 1 > IO_SPACE_LIMIT) {
-			err = -E2BIG;
-			goto end_register;
-		}
-
-		size = SZ_64K;
-		pr_warn("Requested IO range too big, new size set to 64K\n");
-	}
-
-	/* add the range to the list */
-	range = kzalloc(sizeof(*range), GFP_ATOMIC);
-	if (!range) {
-		err = -ENOMEM;
-		goto end_register;
-	}
-
-	range->start = addr;
-	range->size = size;
-
-	list_add_tail(&range->list, &io_range_list);
-
-end_register:
-	spin_unlock(&io_range_lock);
-#endif
-
-	return err;
-}
-
-phys_addr_t pci_pio_to_address(unsigned long pio)
-{
-	phys_addr_t address = (phys_addr_t)OF_BAD_ADDR;
-
-#ifdef PCI_IOBASE
-	struct io_range *range;
-	resource_size_t allocated_size = 0;
-
-	if (pio > IO_SPACE_LIMIT)
-		return address;
-
-	spin_lock(&io_range_lock);
-	list_for_each_entry(range, &io_range_list, list) {
-		if (pio >= allocated_size && pio < allocated_size + range->size) {
-			address = range->start + pio - allocated_size;
-			break;
-		}
-		allocated_size += range->size;
-	}
-	spin_unlock(&io_range_lock);
-#endif
-
-	return address;
-}
-
-unsigned long __weak pci_address_to_pio(phys_addr_t address)
-{
-#ifdef PCI_IOBASE
-	struct io_range *res;
-	resource_size_t offset = 0;
-	unsigned long addr = -1;
-
-	spin_lock(&io_range_lock);
-	list_for_each_entry(res, &io_range_list, list) {
-		if (address >= res->start && address < res->start + res->size) {
-			addr = address - res->start + offset;
-			break;
-		}
-		offset += res->size;
-	}
-	spin_unlock(&io_range_lock);
-
-	return addr;
-#else
-	if (address > IO_SPACE_LIMIT)
-		return (unsigned long)-1;
-
-	return (unsigned long) address;
-#endif
-}
-
 static int __of_address_to_resource(struct device_node *dev,
 		const __be32 *addrp, u64 size, unsigned int flags,
 		const char *name, struct resource *r)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 1a74e87..89e9996 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3022,6 +3022,121 @@ int pci_request_regions_exclusive(struct pci_dev *pdev, const char *res_name)
 }
 EXPORT_SYMBOL(pci_request_regions_exclusive);
 
+#ifdef PCI_IOBASE
+struct io_range {
+	struct list_head list;
+	phys_addr_t start;
+	resource_size_t size;
+};
+
+static LIST_HEAD(io_range_list);
+static DEFINE_SPINLOCK(io_range_lock);
+#endif
+
+/*
+ * Record the PCI IO range (expressed as CPU physical address + size).
+ * Return a negative value if an error has occured, zero otherwise
+ */
+int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size)
+{
+	int err = 0;
+
+#ifdef PCI_IOBASE
+	struct io_range *range;
+	resource_size_t allocated_size = 0;
+
+	/* check if the range hasn't been previously recorded */
+	spin_lock(&io_range_lock);
+	list_for_each_entry(range, &io_range_list, list) {
+		if (addr >= range->start && addr + size <= range->start + size) {
+			/* range already registered, bail out */
+			goto end_register;
+		}
+		allocated_size += range->size;
+	}
+
+	/* range not registed yet, check for available space */
+	if (allocated_size + size - 1 > IO_SPACE_LIMIT) {
+		/* if it's too big check if 64K space can be reserved */
+		if (allocated_size + SZ_64K - 1 > IO_SPACE_LIMIT) {
+			err = -E2BIG;
+			goto end_register;
+		}
+
+		size = SZ_64K;
+		pr_warn("Requested IO range too big, new size set to 64K\n");
+	}
+
+	/* add the range to the list */
+	range = kzalloc(sizeof(*range), GFP_ATOMIC);
+	if (!range) {
+		err = -ENOMEM;
+		goto end_register;
+	}
+
+	range->start = addr;
+	range->size = size;
+
+	list_add_tail(&range->list, &io_range_list);
+
+end_register:
+	spin_unlock(&io_range_lock);
+#endif
+
+	return err;
+}
+
+phys_addr_t pci_pio_to_address(unsigned long pio)
+{
+	phys_addr_t address = (phys_addr_t)OF_BAD_ADDR;
+
+#ifdef PCI_IOBASE
+	struct io_range *range;
+	resource_size_t allocated_size = 0;
+
+	if (pio > IO_SPACE_LIMIT)
+		return address;
+
+	spin_lock(&io_range_lock);
+	list_for_each_entry(range, &io_range_list, list) {
+		if (pio >= allocated_size && pio < allocated_size + range->size) {
+			address = range->start + pio - allocated_size;
+			break;
+		}
+		allocated_size += range->size;
+	}
+	spin_unlock(&io_range_lock);
+#endif
+
+	return address;
+}
+
+unsigned long __weak pci_address_to_pio(phys_addr_t address)
+{
+#ifdef PCI_IOBASE
+	struct io_range *res;
+	resource_size_t offset = 0;
+	unsigned long addr = -1;
+
+	spin_lock(&io_range_lock);
+	list_for_each_entry(res, &io_range_list, list) {
+		if (address >= res->start && address < res->start + res->size) {
+			addr = address - res->start + offset;
+			break;
+		}
+		offset += res->size;
+	}
+	spin_unlock(&io_range_lock);
+
+	return addr;
+#else
+	if (address > IO_SPACE_LIMIT)
+		return (unsigned long)-1;
+
+	return (unsigned long) address;
+#endif
+}
+
 /**
  *	pci_remap_iospace - Remap the memory mapped I/O space
  *	@res: Resource describing the I/O space
diff --git a/include/linux/of_address.h b/include/linux/of_address.h
index 01c0a55..3786473 100644
--- a/include/linux/of_address.h
+++ b/include/linux/of_address.h
@@ -47,10 +47,6 @@ void __iomem *of_io_request_and_map(struct device_node *device,
 extern const __be32 *of_get_address(struct device_node *dev, int index,
 			   u64 *size, unsigned int *flags);
 
-extern int pci_register_io_range(phys_addr_t addr, resource_size_t size);
-extern unsigned long pci_address_to_pio(phys_addr_t addr);
-extern phys_addr_t pci_pio_to_address(unsigned long pio);
-
 extern int of_pci_range_parser_init(struct of_pci_range_parser *parser,
 			struct device_node *node);
 extern struct of_pci_range *of_pci_range_parser_one(
@@ -86,11 +82,6 @@ static inline const __be32 *of_get_address(struct device_node *dev, int index,
 	return NULL;
 }
 
-static inline phys_addr_t pci_pio_to_address(unsigned long pio)
-{
-	return 0;
-}
-
 static inline int of_pci_range_parser_init(struct of_pci_range_parser *parser,
 			struct device_node *node)
 {
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 9b31d48..c28adb4 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1164,6 +1164,9 @@ int __must_check pci_bus_alloc_resource(struct pci_bus *bus,
 			void *alignf_data);
 
 
+int pci_register_io_range(phys_addr_t addr, resource_size_t size);
+unsigned long pci_address_to_pio(phys_addr_t addr);
+phys_addr_t pci_pio_to_address(unsigned long pio);
 int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr);
 
 static inline pci_bus_addr_t pci_bus_address(struct pci_dev *pdev, int bar)
@@ -1480,6 +1483,8 @@ static inline int pci_request_regions(struct pci_dev *dev, const char *res_name)
 { return -EIO; }
 static inline void pci_release_regions(struct pci_dev *dev) { }
 
+static inline unsigned long pci_address_to_pio(phys_addr_t addr) { return -1; }
+
 static inline void pci_block_cfg_access(struct pci_dev *dev) { }
 static inline int pci_block_cfg_access_in_atomic(struct pci_dev *dev)
 { return 0; }
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1380032 — [PATCH V6 09/13] pci, acpi: Support for ACPI based generic PCI host controller

This patch is going to implement generic PCI host controller for
ACPI world, similar to what pci-host-generic.c driver does for DT world.

All such drivers, which we have seen so far, were implemented within
arch/ directory since they had some arch assumptions (x86 and ia64).
However, they all are doing similar thing, so it makes sense to find
some common code and abstract it into the generic driver.

In order to handle PCI config space regions properly, we define new
MCFG interface which parses MCFG table and keep its entries
in a list. New pci_mcfg_init call is defined so that we do not depend
on PCI_MMCONFIG. Regions are not mapped until host bridge ask for it.

The implementation of pci_acpi_scan_root() looks up the saved MCFG entries
and sets up a new mapping. Generic PCI functions are used for
accessing config space. Driver selects PCI_GENERIC_ECAM and uses functions
from drivers/pci/ecam.h to create and access ECAM mappings.

As mentioned in Kconfig help section, ACPI_PCI_HOST_GENERIC choice
should be made on a per-architecture basis.

This patch is heavily based on the updated version from Jayachandran C:
https://lkml.org/lkml/2016/4/11/908
git: https://github.com/jchandra-brcm/linux/ (arm64-acpi-pci-v3)

Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
Signed-off-by: Jayachandran C <jchandra@broadcom.com>
---
 drivers/acpi/Kconfig        |   8 ++
 drivers/acpi/Makefile       |   1 +
 drivers/acpi/bus.c          |   1 +
 drivers/acpi/pci_gen_host.c | 231 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h         |   6 ++
 5 files changed, 247 insertions(+)
 create mode 100644 drivers/acpi/pci_gen_host.c

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 183ffa3..70272c5 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -346,6 +346,14 @@ config ACPI_PCI_SLOT
 	  i.e., segment/bus/device/function tuples, with physical slots in
 	  the system.  If you are unsure, say N.
 
+config ACPI_PCI_HOST_GENERIC
+	bool
+	select PCI_GENERIC_ECAM
+	help
+	  Select this config option from the architecture Kconfig,
+	  if it is preferred to enable ACPI PCI host controller driver which
+	  has no arch-specific assumptions.
+
 config X86_PM_TIMER
 	bool "Power Management Timer Support" if EXPERT
 	depends on X86
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 81e5cbc..b12fa64 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -40,6 +40,7 @@ acpi-$(CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC) += processor_pdc.o
 acpi-y				+= ec.o
 acpi-$(CONFIG_ACPI_DOCK)	+= dock.o
 acpi-y				+= pci_root.o pci_link.o pci_irq.o
+obj-$(CONFIG_ACPI_PCI_HOST_GENERIC)	+= pci_gen_host.o
 acpi-y				+= acpi_lpss.o acpi_apd.o
 acpi-y				+= acpi_platform.o
 acpi-y				+= acpi_pnp.o
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index c068c82..803a1d7 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -1107,6 +1107,7 @@ static int __init acpi_init(void)
 	}
 
 	pci_mmcfg_late_init();
+	pci_mcfg_init();
 	acpi_scan_init();
 	acpi_ec_init();
 	acpi_debugfs_init();
diff --git a/drivers/acpi/pci_gen_host.c b/drivers/acpi/pci_gen_host.c
new file mode 100644
index 0000000..fd360b5
--- /dev/null
+++ b/drivers/acpi/pci_gen_host.c
@@ -0,0 +1,231 @@
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, version 2, as
+ * published by the Free Software Foundation (the "GPL").
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License version 2 (GPLv2) for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * version 2 (GPLv2) along with this source code.
+ */
+#include <linux/kernel.h>
+#include <linux/pci.h>
+#include <linux/pci-acpi.h>
+#include <linux/sfi_acpi.h>
+#include <linux/slab.h>
+
+#include "../pci/ecam.h"
+
+#define PREFIX	"ACPI: "
+
+/* Structure to hold entries from the MCFG table */
+struct mcfg_entry {
+	struct list_head	list;
+	phys_addr_t		addr;
+	u16			segment;
+	u8			bus_start;
+	u8			bus_end;
+};
+
+/* List to save mcfg entries */
+static LIST_HEAD(pci_mcfg_list);
+static DEFINE_MUTEX(pci_mcfg_lock);
+
+/* ACPI info for generic ACPI PCI controller */
+struct acpi_pci_generic_root_info {
+	struct acpi_pci_root_info	common;
+	struct pci_config_window	*cfg;	/* config space mapping */
+};
+
+/* Find the entry in mcfg list which contains range bus_start */
+static struct mcfg_entry *pci_mcfg_lookup(u16 seg, u8 bus_start)
+{
+	struct mcfg_entry *e;
+
+	list_for_each_entry(e, &pci_mcfg_list, list) {
+		if (e->segment == seg &&
+		    e->bus_start <= bus_start && bus_start <= e->bus_end)
+			return e;
+	}
+
+	return NULL;
+}
+
+
+/*
+ * Lookup the bus range for the domain in MCFG, and set up config space
+ * mapping.
+ */
+static int pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root,
+				       struct acpi_pci_generic_root_info *ri)
+{
+	u16 seg = root->segment;
+	u8 bus_start = root->secondary.start;
+	u8 bus_end = root->secondary.end;
+	struct pci_config_window *cfg;
+	struct mcfg_entry *e;
+	phys_addr_t addr;
+	int err = 0;
+
+	mutex_lock(&pci_mcfg_lock);
+	e = pci_mcfg_lookup(seg, bus_start);
+	if (!e) {
+		addr = acpi_pci_root_get_mcfg_addr(root->device->handle);
+		if (addr == 0) {
+			pr_err(PREFIX"%04x:%02x-%02x bus range error\n",
+			       seg, bus_start, bus_end);
+			err = -ENOENT;
+			goto err_out;
+		}
+	} else {
+		if (bus_start != e->bus_start) {
+			pr_err("%04x:%02x-%02x bus range mismatch %02x\n",
+			       seg, bus_start, bus_end, e->bus_start);
+			err = -EINVAL;
+			goto err_out;
+		} else if (bus_end != e->bus_end) {
+			pr_warn("%04x:%02x-%02x bus end mismatch %02x\n",
+				seg, bus_start, bus_end, e->bus_end);
+			bus_end = min(bus_end, e->bus_end);
+		}
+		addr = e->addr;
+	}
+
+	cfg = pci_generic_ecam_create(&root->device->dev, addr, bus_start,
+				      bus_end, &pci_generic_ecam_default_ops);
+	if (IS_ERR(cfg)) {
+		err = PTR_ERR(cfg);
+		pr_err("%04x:%02x-%02x error %d mapping CAM\n", seg,
+			bus_start, bus_end, err);
+		goto err_out;
+	}
+
+	cfg->domain = seg;
+	ri->cfg = cfg;
+err_out:
+	mutex_unlock(&pci_mcfg_lock);
+	return err;
+}
+
+/* release_info: free resrouces allocated by init_info */
+static void pci_acpi_generic_release_info(struct acpi_pci_root_info *ci)
+{
+	struct acpi_pci_generic_root_info *ri;
+
+	ri = container_of(ci, struct acpi_pci_generic_root_info, common);
+	pci_generic_ecam_free(ri->cfg);
+	kfree(ri);
+}
+
+static struct acpi_pci_root_ops acpi_pci_root_ops = {
+	.release_info = pci_acpi_generic_release_info,
+};
+
+/* Interface called from ACPI code to setup PCI host controller */
+struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
+{
+	int node = acpi_get_node(root->device->handle);
+	struct acpi_pci_generic_root_info *ri;
+	struct pci_bus *bus, *child;
+	int err;
+
+	ri = kzalloc_node(sizeof(*ri), GFP_KERNEL, node);
+	if (!ri)
+		return NULL;
+
+	err = pci_acpi_setup_ecam_mapping(root, ri);
+	if (err)
+		return NULL;
+
+	acpi_pci_root_ops.pci_ops = &ri->cfg->ops->pci_ops;
+	bus = acpi_pci_root_create(root, &acpi_pci_root_ops, &ri->common,
+				   ri->cfg);
+	if (!bus)
+		return NULL;
+
+	pci_bus_size_bridges(bus);
+	pci_bus_assign_resources(bus);
+
+	list_for_each_entry(child, &bus->children, node)
+		pcie_bus_configure_settings(child);
+
+	return bus;
+}
+
+/* handle MCFG table entries */
+static __init int pci_mcfg_parse(struct acpi_table_header *header)
+{
+	struct acpi_table_mcfg *mcfg;
+	struct acpi_mcfg_allocation *mptr;
+	struct mcfg_entry *e, *arr;
+	int i, n;
+
+	if (!header)
+		return -EINVAL;
+
+	mcfg = (struct acpi_table_mcfg *)header;
+	mptr = (struct acpi_mcfg_allocation *) &mcfg[1];
+	n = (header->length - sizeof(*mcfg)) / sizeof(*mptr);
+	if (n <= 0 || n > 255) {
+		pr_err(PREFIX " MCFG has incorrect entries (%d).\n", n);
+		return -EINVAL;
+	}
+
+	arr = kcalloc(n, sizeof(*arr), GFP_KERNEL);
+	if (!arr)
+		return -ENOMEM;
+
+	for (i = 0, e = arr; i < n; i++, mptr++, e++) {
+		e->segment = mptr->pci_segment;
+		e->addr =  mptr->address;
+		e->bus_start = mptr->start_bus_number;
+		e->bus_end = mptr->end_bus_number;
+		list_add(&e->list, &pci_mcfg_list);
+		pr_info(PREFIX
+			"MCFG entry for domain %04x [bus %02x-%02x] (base %pa)\n",
+			e->segment, e->bus_start, e->bus_end, &e->addr);
+	}
+
+	return 0;
+}
+
+/* Interface called by ACPI - parse and save MCFG table */
+void __init pci_mcfg_init(void)
+{
+	int err = acpi_table_parse(ACPI_SIG_MCFG, pci_mcfg_parse);
+	if (err)
+		pr_err(PREFIX "Failed to parse MCFG (%d)\n", err);
+	else if (list_empty(&pci_mcfg_list))
+		pr_info(PREFIX "No valid entries in MCFG table.\n");
+	else {
+		struct mcfg_entry *e;
+		int i = 0;
+		list_for_each_entry(e, &pci_mcfg_list, list)
+			i++;
+		pr_info(PREFIX "MCFG table loaded, %d entries\n", i);
+	}
+}
+
+/* Raw operations, works only for MCFG entries with an associated bus */
+int raw_pci_read(unsigned int domain, unsigned int busn, unsigned int devfn,
+		 int reg, int len, u32 *val)
+{
+	struct pci_bus *bus = pci_find_bus(domain, busn);
+
+	if (!bus)
+		return PCIBIOS_DEVICE_NOT_FOUND;
+	return bus->ops->read(bus, devfn, reg, len, val);
+}
+
+int raw_pci_write(unsigned int domain, unsigned int busn, unsigned int devfn,
+		  int reg, int len, u32 val)
+{
+	struct pci_bus *bus = pci_find_bus(domain, busn);
+
+	if (!bus)
+		return PCIBIOS_DEVICE_NOT_FOUND;
+	return bus->ops->write(bus, devfn, reg, len, val);
+}
diff --git a/include/linux/pci.h b/include/linux/pci.h
index df1f33d..c0422ea 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1729,6 +1729,12 @@ static inline void pci_mmcfg_early_init(void) { }
 static inline void pci_mmcfg_late_init(void) { }
 #endif
 
+#ifdef CONFIG_ACPI_PCI_HOST_GENERIC
+void __init pci_mcfg_init(void);
+#else
+static inline void pci_mcfg_init(void) { return; }
+#endif
+
 int pci_ext_cfg_avail(void);
 
 void __iomem *pci_ioremap_bar(struct pci_dev *pdev, int bar);
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1383658 — Re: [PATCH V6 09/13] pci, acpi: Support for ACPI based generic PCI host controller

On Fri, Apr 15, 2016 at 10:36 PM, Tomasz Nowicki <tn@semihalf.com> wrote:
> This patch is going to implement generic PCI host controller for
> ACPI world, similar to what pci-host-generic.c driver does for DT world.
>
> All such drivers, which we have seen so far, were implemented within
> arch/ directory since they had some arch assumptions (x86 and ia64).
> However, they all are doing similar thing, so it makes sense to find
> some common code and abstract it into the generic driver.
>
> In order to handle PCI config space regions properly, we define new
> MCFG interface which parses MCFG table and keep its entries
> in a list. New pci_mcfg_init call is defined so that we do not depend
> on PCI_MMCONFIG. Regions are not mapped until host bridge ask for it.
>
> The implementation of pci_acpi_scan_root() looks up the saved MCFG entries
> and sets up a new mapping. Generic PCI functions are used for
> accessing config space. Driver selects PCI_GENERIC_ECAM and uses functions
> from drivers/pci/ecam.h to create and access ECAM mappings.
>
> As mentioned in Kconfig help section, ACPI_PCI_HOST_GENERIC choice
> should be made on a per-architecture basis.
>
> This patch is heavily based on the updated version from Jayachandran C:
> https://lkml.org/lkml/2016/4/11/908
> git: https://github.com/jchandra-brcm/linux/ (arm64-acpi-pci-v3)

This is a little bit unusual because I had not posted the v3 patch
to the mailing list yet, but you posted a variant of it The git repository
should not be in the commit comment because it is a temporary location.

There are some changes here I don't agree with. I think it will be
better if you can post a version without the quirk handling and with
some of the suggestions below.

> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> Signed-off-by: Jayachandran C <jchandra@broadcom.com>
> ---
>  drivers/acpi/Kconfig        |   8 ++
>  drivers/acpi/Makefile       |   1 +
>  drivers/acpi/bus.c          |   1 +
>  drivers/acpi/pci_gen_host.c | 231 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci.h         |   6 ++
>  5 files changed, 247 insertions(+)
>  create mode 100644 drivers/acpi/pci_gen_host.c
>
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 183ffa3..70272c5 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -346,6 +346,14 @@ config ACPI_PCI_SLOT
>           i.e., segment/bus/device/function tuples, with physical slots in
>           the system.  If you are unsure, say N.
>
> +config ACPI_PCI_HOST_GENERIC
> +       bool
> +       select PCI_GENERIC_ECAM
> +       help
> +         Select this config option from the architecture Kconfig,
> +         if it is preferred to enable ACPI PCI host controller driver which
> +         has no arch-specific assumptions.
> +
>  config X86_PM_TIMER
>         bool "Power Management Timer Support" if EXPERT
>         depends on X86
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index 81e5cbc..b12fa64 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -40,6 +40,7 @@ acpi-$(CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC) += processor_pdc.o
>  acpi-y                         += ec.o
>  acpi-$(CONFIG_ACPI_DOCK)       += dock.o
>  acpi-y                         += pci_root.o pci_link.o pci_irq.o
> +obj-$(CONFIG_ACPI_PCI_HOST_GENERIC)    += pci_gen_host.o
>  acpi-y                         += acpi_lpss.o acpi_apd.o
>  acpi-y                         += acpi_platform.o
>  acpi-y                         += acpi_pnp.o
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index c068c82..803a1d7 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -1107,6 +1107,7 @@ static int __init acpi_init(void)
>         }
>
>         pci_mmcfg_late_init();
> +       pci_mcfg_init();

Please see below.

>         acpi_scan_init();
>         acpi_ec_init();
>         acpi_debugfs_init();
> diff --git a/drivers/acpi/pci_gen_host.c b/drivers/acpi/pci_gen_host.c
> new file mode 100644
> index 0000000..fd360b5
> --- /dev/null
> +++ b/drivers/acpi/pci_gen_host.c
> @@ -0,0 +1,231 @@
> +/*

You seem to have removed the copyright line, this is not proper, you
should probably add your copyright line if you think your changes are
significant.

> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License, version 2, as
> + * published by the Free Software Foundation (the "GPL").
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License version 2 (GPLv2) for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * version 2 (GPLv2) along with this source code.
> + */
> +#include <linux/kernel.h>
> +#include <linux/pci.h>
> +#include <linux/pci-acpi.h>
> +#include <linux/sfi_acpi.h>
> +#include <linux/slab.h>
> +
> +#include "../pci/ecam.h"
> +
> +#define PREFIX "ACPI: "
> +
> +/* Structure to hold entries from the MCFG table */
> +struct mcfg_entry {
> +       struct list_head        list;
> +       phys_addr_t             addr;
> +       u16                     segment;
> +       u8                      bus_start;
> +       u8                      bus_end;
> +};
> +
> +/* List to save mcfg entries */
> +static LIST_HEAD(pci_mcfg_list);
> +static DEFINE_MUTEX(pci_mcfg_lock);

There is no need to use a list or lock here, I had used an
array and that is sufficient since it is not modified after it
is filled initially.

> +/* ACPI info for generic ACPI PCI controller */
> +struct acpi_pci_generic_root_info {
> +       struct acpi_pci_root_info       common;
> +       struct pci_config_window        *cfg;   /* config space mapping */
> +};
> +
> +/* Find the entry in mcfg list which contains range bus_start */
> +static struct mcfg_entry *pci_mcfg_lookup(u16 seg, u8 bus_start)
> +{
> +       struct mcfg_entry *e;
> +
> +       list_for_each_entry(e, &pci_mcfg_list, list) {
> +               if (e->segment == seg &&
> +                   e->bus_start <= bus_start && bus_start <= e->bus_end)
> +                       return e;
> +       }
> +
> +       return NULL;
> +}
> +
> +
> +/*
> + * Lookup the bus range for the domain in MCFG, and set up config space
> + * mapping.
> + */
> +static int pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root,
> +                                      struct acpi_pci_generic_root_info *ri)
> +{
> +       u16 seg = root->segment;
> +       u8 bus_start = root->secondary.start;
> +       u8 bus_end = root->secondary.end;
> +       struct pci_config_window *cfg;
> +       struct mcfg_entry *e;
> +       phys_addr_t addr;
> +       int err = 0;
> +
> +       mutex_lock(&pci_mcfg_lock);
> +       e = pci_mcfg_lookup(seg, bus_start);
> +       if (!e) {
> +               addr = acpi_pci_root_get_mcfg_addr(root->device->handle);

The acpi_pci_root_get_mcfg_addr() is already called in pci_root.c, doing
it again here is unnecessary.

I think you can have a function to pick up addr, bus_start, bus_end given
a domain from either MCFG or using _CBA method, but I think that
should be done in pci_root.c in a separate patch.

> +               if (addr == 0) {
> +                       pr_err(PREFIX"%04x:%02x-%02x bus range error\n",
> +                              seg, bus_start, bus_end);
> +                       err = -ENOENT;
> +                       goto err_out;
> +               }
> +       } else {
> +               if (bus_start != e->bus_start) {
> +                       pr_err("%04x:%02x-%02x bus range mismatch %02x\n",
> +                              seg, bus_start, bus_end, e->bus_start);
> +                       err = -EINVAL;
> +                       goto err_out;
> +               } else if (bus_end != e->bus_end) {
> +                       pr_warn("%04x:%02x-%02x bus end mismatch %02x\n",
> +                               seg, bus_start, bus_end, e->bus_end);
> +                       bus_end = min(bus_end, e->bus_end);
> +               }
> +               addr = e->addr;
> +       }
> +
> +       cfg = pci_generic_ecam_create(&root->device->dev, addr, bus_start,
> +                                     bus_end, &pci_generic_ecam_default_ops);
> +       if (IS_ERR(cfg)) {
> +               err = PTR_ERR(cfg);
> +               pr_err("%04x:%02x-%02x error %d mapping CAM\n", seg,
> +                       bus_start, bus_end, err);
> +               goto err_out;
> +       }

You seem to have moved all the config space mapping to this
point. Intel seems to do the mapping when they read MCFG for the
entries, and I had followed that model, and that avoids having another
array/list to save the values.

> +       cfg->domain = seg;
> +       ri->cfg = cfg;
> +err_out:
> +       mutex_unlock(&pci_mcfg_lock);
> +       return err;
> +}
> +
> +/* release_info: free resrouces allocated by init_info */
> +static void pci_acpi_generic_release_info(struct acpi_pci_root_info *ci)
> +{
> +       struct acpi_pci_generic_root_info *ri;
> +
> +       ri = container_of(ci, struct acpi_pci_generic_root_info, common);
> +       pci_generic_ecam_free(ri->cfg);
> +       kfree(ri);
> +}
> +
> +static struct acpi_pci_root_ops acpi_pci_root_ops = {
> +       .release_info = pci_acpi_generic_release_info,
> +};
> +
> +/* Interface called from ACPI code to setup PCI host controller */
> +struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
> +{
> +       int node = acpi_get_node(root->device->handle);
> +       struct acpi_pci_generic_root_info *ri;
> +       struct pci_bus *bus, *child;
> +       int err;
> +
> +       ri = kzalloc_node(sizeof(*ri), GFP_KERNEL, node);
> +       if (!ri)
> +               return NULL;
> +
> +       err = pci_acpi_setup_ecam_mapping(root, ri);
> +       if (err)
> +               return NULL;
> +
> +       acpi_pci_root_ops.pci_ops = &ri->cfg->ops->pci_ops;
> +       bus = acpi_pci_root_create(root, &acpi_pci_root_ops, &ri->common,
> +                                  ri->cfg);
> +       if (!bus)
> +               return NULL;
> +
> +       pci_bus_size_bridges(bus);
> +       pci_bus_assign_resources(bus);
> +
> +       list_for_each_entry(child, &bus->children, node)
> +               pcie_bus_configure_settings(child);
> +
> +       return bus;
> +}
> +
> +/* handle MCFG table entries */
> +static __init int pci_mcfg_parse(struct acpi_table_header *header)
> +{
> +       struct acpi_table_mcfg *mcfg;
> +       struct acpi_mcfg_allocation *mptr;
> +       struct mcfg_entry *e, *arr;
> +       int i, n;
> +
> +       if (!header)
> +               return -EINVAL;
> +
> +       mcfg = (struct acpi_table_mcfg *)header;
> +       mptr = (struct acpi_mcfg_allocation *) &mcfg[1];
> +       n = (header->length - sizeof(*mcfg)) / sizeof(*mptr);
> +       if (n <= 0 || n > 255) {
> +               pr_err(PREFIX " MCFG has incorrect entries (%d).\n", n);
> +               return -EINVAL;
> +       }
> +
> +       arr = kcalloc(n, sizeof(*arr), GFP_KERNEL);
> +       if (!arr)
> +               return -ENOMEM;

Here you already have an array which is also connected as a linked
list which is unnecessary.

> +       for (i = 0, e = arr; i < n; i++, mptr++, e++) {
> +               e->segment = mptr->pci_segment;
> +               e->addr =  mptr->address;
> +               e->bus_start = mptr->start_bus_number;
> +               e->bus_end = mptr->end_bus_number;
> +               list_add(&e->list, &pci_mcfg_list);
> +               pr_info(PREFIX
> +                       "MCFG entry for domain %04x [bus %02x-%02x] (base %pa)\n",
> +                       e->segment, e->bus_start, e->bus_end, &e->addr);
> +       }
> +
> +       return 0;
> +}
> +
> +/* Interface called by ACPI - parse and save MCFG table */
> +void __init pci_mcfg_init(void)
> +{
> +       int err = acpi_table_parse(ACPI_SIG_MCFG, pci_mcfg_parse);
> +       if (err)
> +               pr_err(PREFIX "Failed to parse MCFG (%d)\n", err);
> +       else if (list_empty(&pci_mcfg_list))
> +               pr_info(PREFIX "No valid entries in MCFG table.\n");
> +       else {
> +               struct mcfg_entry *e;
> +               int i = 0;
> +               list_for_each_entry(e, &pci_mcfg_list, list)
> +                       i++;
> +               pr_info(PREFIX "MCFG table loaded, %d entries\n", i);
> +       }
> +}
> +
> +/* Raw operations, works only for MCFG entries with an associated bus */
> +int raw_pci_read(unsigned int domain, unsigned int busn, unsigned int devfn,
> +                int reg, int len, u32 *val)
> +{
> +       struct pci_bus *bus = pci_find_bus(domain, busn);
> +
> +       if (!bus)
> +               return PCIBIOS_DEVICE_NOT_FOUND;
> +       return bus->ops->read(bus, devfn, reg, len, val);
> +}
> +
> +int raw_pci_write(unsigned int domain, unsigned int busn, unsigned int devfn,
> +                 int reg, int len, u32 val)
> +{
> +       struct pci_bus *bus = pci_find_bus(domain, busn);
> +
> +       if (!bus)
> +               return PCIBIOS_DEVICE_NOT_FOUND;
> +       return bus->ops->write(bus, devfn, reg, len, val);
> +}
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index df1f33d..c0422ea 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1729,6 +1729,12 @@ static inline void pci_mmcfg_early_init(void) { }
>  static inline void pci_mmcfg_late_init(void) { }
>  #endif
>
> +#ifdef CONFIG_ACPI_PCI_HOST_GENERIC
> +void __init pci_mcfg_init(void);
> +#else
> +static inline void pci_mcfg_init(void) { return; }
> +#endif

You can still use the function pci_mmcfg_late_init() if
PCI_MMCONFIG or ACPI_PCI_HOST_GENERIC is defined

> +
>  int pci_ext_cfg_avail(void);
>
>  void __iomem *pci_ioremap_bar(struct pci_dev *pdev, int bar);


JC.

[toc] | [prev] | [next] | [standalone]

#1383970 — Re: [PATCH V6 09/13] pci, acpi: Support for ACPI based generic PCI host controller

On 20.04.2016 21:12, Jayachandran C wrote:
> On Fri, Apr 15, 2016 at 10:36 PM, Tomasz Nowicki <tn@semihalf.com> wrote:
>> This patch is going to implement generic PCI host controller for
>> ACPI world, similar to what pci-host-generic.c driver does for DT world.
>>
>> All such drivers, which we have seen so far, were implemented within
>> arch/ directory since they had some arch assumptions (x86 and ia64).
>> However, they all are doing similar thing, so it makes sense to find
>> some common code and abstract it into the generic driver.
>>
>> In order to handle PCI config space regions properly, we define new
>> MCFG interface which parses MCFG table and keep its entries
>> in a list. New pci_mcfg_init call is defined so that we do not depend
>> on PCI_MMCONFIG. Regions are not mapped until host bridge ask for it.
>>
>> The implementation of pci_acpi_scan_root() looks up the saved MCFG entries
>> and sets up a new mapping. Generic PCI functions are used for
>> accessing config space. Driver selects PCI_GENERIC_ECAM and uses functions
>> from drivers/pci/ecam.h to create and access ECAM mappings.
>>
>> As mentioned in Kconfig help section, ACPI_PCI_HOST_GENERIC choice
>> should be made on a per-architecture basis.
>>
>> This patch is heavily based on the updated version from Jayachandran C:
>> https://lkml.org/lkml/2016/4/11/908
>> git: https://github.com/jchandra-brcm/linux/ (arm64-acpi-pci-v3)
>
> This is a little bit unusual because I had not posted the v3 patch
> to the mailing list yet, but you posted a variant of it The git repository
> should not be in the commit comment because it is a temporary location.

We all agree this too important for everybody to delay this series. So 
main motivation is to keep all discussion&patches within one unified 
series. I would like to finally find direction we need to go. Stating 
another discussion based on my previous patch set v5 confused people, 
they do no know who is driving this. Again, lets cooperate to move it 
forward within one patch set.

I agree with you we need maintainers to join this discussion.

>
> There are some changes here I don't agree with. I think it will be
> better if you can post a version without the quirk handling and with
> some of the suggestions below.

The next version will not have quirk handling part. Regarding your 
comments, please see below.

>
>> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
>> Signed-off-by: Jayachandran C <jchandra@broadcom.com>
>> ---
>>   drivers/acpi/Kconfig        |   8 ++
>>   drivers/acpi/Makefile       |   1 +
>>   drivers/acpi/bus.c          |   1 +
>>   drivers/acpi/pci_gen_host.c | 231 ++++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/pci.h         |   6 ++
>>   5 files changed, 247 insertions(+)
>>   create mode 100644 drivers/acpi/pci_gen_host.c
>>
>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>> index 183ffa3..70272c5 100644
>> --- a/drivers/acpi/Kconfig
>> +++ b/drivers/acpi/Kconfig
>> @@ -346,6 +346,14 @@ config ACPI_PCI_SLOT
>>            i.e., segment/bus/device/function tuples, with physical slots in
>>            the system.  If you are unsure, say N.
>>
>> +config ACPI_PCI_HOST_GENERIC
>> +       bool
>> +       select PCI_GENERIC_ECAM
>> +       help
>> +         Select this config option from the architecture Kconfig,
>> +         if it is preferred to enable ACPI PCI host controller driver which
>> +         has no arch-specific assumptions.
>> +
>>   config X86_PM_TIMER
>>          bool "Power Management Timer Support" if EXPERT
>>          depends on X86
>> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
>> index 81e5cbc..b12fa64 100644
>> --- a/drivers/acpi/Makefile
>> +++ b/drivers/acpi/Makefile
>> @@ -40,6 +40,7 @@ acpi-$(CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC) += processor_pdc.o
>>   acpi-y                         += ec.o
>>   acpi-$(CONFIG_ACPI_DOCK)       += dock.o
>>   acpi-y                         += pci_root.o pci_link.o pci_irq.o
>> +obj-$(CONFIG_ACPI_PCI_HOST_GENERIC)    += pci_gen_host.o
>>   acpi-y                         += acpi_lpss.o acpi_apd.o
>>   acpi-y                         += acpi_platform.o
>>   acpi-y                         += acpi_pnp.o
>> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
>> index c068c82..803a1d7 100644
>> --- a/drivers/acpi/bus.c
>> +++ b/drivers/acpi/bus.c
>> @@ -1107,6 +1107,7 @@ static int __init acpi_init(void)
>>          }
>>
>>          pci_mmcfg_late_init();
>> +       pci_mcfg_init();
>
> Please see below.
>
>>          acpi_scan_init();
>>          acpi_ec_init();
>>          acpi_debugfs_init();
>> diff --git a/drivers/acpi/pci_gen_host.c b/drivers/acpi/pci_gen_host.c
>> new file mode 100644
>> index 0000000..fd360b5
>> --- /dev/null
>> +++ b/drivers/acpi/pci_gen_host.c
>> @@ -0,0 +1,231 @@
>> +/*
>
> You seem to have removed the copyright line, this is not proper, you
> should probably add your copyright line if you think your changes are
> significant.

I rather forgot to add copyright here, I will fix it.

>
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License, version 2, as
>> + * published by the Free Software Foundation (the "GPL").
>> + *
>> + * This program is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * General Public License version 2 (GPLv2) for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * version 2 (GPLv2) along with this source code.
>> + */
>> +#include <linux/kernel.h>
>> +#include <linux/pci.h>
>> +#include <linux/pci-acpi.h>
>> +#include <linux/sfi_acpi.h>
>> +#include <linux/slab.h>
>> +
>> +#include "../pci/ecam.h"
>> +
>> +#define PREFIX "ACPI: "
>> +
>> +/* Structure to hold entries from the MCFG table */
>> +struct mcfg_entry {
>> +       struct list_head        list;
>> +       phys_addr_t             addr;
>> +       u16                     segment;
>> +       u8                      bus_start;
>> +       u8                      bus_end;
>> +};
>> +
>> +/* List to save mcfg entries */
>> +static LIST_HEAD(pci_mcfg_list);
>> +static DEFINE_MUTEX(pci_mcfg_lock);
>
> There is no need to use a list or lock here, I had used an
> array and that is sufficient since it is not modified after it
> is filled initially.

ACPI PCI driver supports hot plug/removal, I want to avoid races using 
this lock. I decided to use list because I do ECAM mapping on demand. 
See below for more details.

>
>> +/* ACPI info for generic ACPI PCI controller */
>> +struct acpi_pci_generic_root_info {
>> +       struct acpi_pci_root_info       common;
>> +       struct pci_config_window        *cfg;   /* config space mapping */
>> +};
>> +
>> +/* Find the entry in mcfg list which contains range bus_start */
>> +static struct mcfg_entry *pci_mcfg_lookup(u16 seg, u8 bus_start)
>> +{
>> +       struct mcfg_entry *e;
>> +
>> +       list_for_each_entry(e, &pci_mcfg_list, list) {
>> +               if (e->segment == seg &&
>> +                   e->bus_start <= bus_start && bus_start <= e->bus_end)
>> +                       return e;
>> +       }
>> +
>> +       return NULL;
>> +}
>> +
>> +
>> +/*
>> + * Lookup the bus range for the domain in MCFG, and set up config space
>> + * mapping.
>> + */
>> +static int pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root,
>> +                                      struct acpi_pci_generic_root_info *ri)
>> +{
>> +       u16 seg = root->segment;
>> +       u8 bus_start = root->secondary.start;
>> +       u8 bus_end = root->secondary.end;
>> +       struct pci_config_window *cfg;
>> +       struct mcfg_entry *e;
>> +       phys_addr_t addr;
>> +       int err = 0;
>> +
>> +       mutex_lock(&pci_mcfg_lock);
>> +       e = pci_mcfg_lookup(seg, bus_start);
>> +       if (!e) {
>> +               addr = acpi_pci_root_get_mcfg_addr(root->device->handle);
>
> The acpi_pci_root_get_mcfg_addr() is already called in pci_root.c, doing
> it again here is unnecessary.

I put it here as per Bjorn's request, see:
https://lkml.org/lkml/2016/3/4/946

If this is not valid any more, I can easily remove it.

>
> I think you can have a function to pick up addr, bus_start, bus_end given
> a domain from either MCFG or using _CBA method, but I think that
> should be done in pci_root.c in a separate patch.
>
>> +               if (addr == 0) {
>> +                       pr_err(PREFIX"%04x:%02x-%02x bus range error\n",
>> +                              seg, bus_start, bus_end);
>> +                       err = -ENOENT;
>> +                       goto err_out;
>> +               }
>> +       } else {
>> +               if (bus_start != e->bus_start) {
>> +                       pr_err("%04x:%02x-%02x bus range mismatch %02x\n",
>> +                              seg, bus_start, bus_end, e->bus_start);
>> +                       err = -EINVAL;
>> +                       goto err_out;
>> +               } else if (bus_end != e->bus_end) {
>> +                       pr_warn("%04x:%02x-%02x bus end mismatch %02x\n",
>> +                               seg, bus_start, bus_end, e->bus_end);
>> +                       bus_end = min(bus_end, e->bus_end);
>> +               }
>> +               addr = e->addr;
>> +       }
>> +
>> +       cfg = pci_generic_ecam_create(&root->device->dev, addr, bus_start,
>> +                                     bus_end, &pci_generic_ecam_default_ops);
>> +       if (IS_ERR(cfg)) {
>> +               err = PTR_ERR(cfg);
>> +               pr_err("%04x:%02x-%02x error %d mapping CAM\n", seg,
>> +                       bus_start, bus_end, err);
>> +               goto err_out;
>> +       }
>
> You seem to have moved all the config space mapping to this
> point. Intel seems to do the mapping when they read MCFG for the
> entries, and I had followed that model, and that avoids having another
> array/list to save the values.

See:
https://lkml.org/lkml/2016/3/3/921

I agree with Bjorn here, we should map it whenever we need this instead 
of mapping all MCFG entries just in case. Also, I see other advantages:
1. We can always use valid "dev" for pci_generic_ecam_create call, what 
you can't do when you use pci_generic_ecam_create during MCFG parsing.
2. No need for special handling for entries coming from _CBA, the path 
is the same for MCFG and _CBA. We do not have to remember that only _CBA 
related entries have to be unmapped.

>
>> +       cfg->domain = seg;
>> +       ri->cfg = cfg;
>> +err_out:
>> +       mutex_unlock(&pci_mcfg_lock);
>> +       return err;
>> +}
>> +
>> +/* release_info: free resrouces allocated by init_info */
>> +static void pci_acpi_generic_release_info(struct acpi_pci_root_info *ci)
>> +{
>> +       struct acpi_pci_generic_root_info *ri;
>> +
>> +       ri = container_of(ci, struct acpi_pci_generic_root_info, common);
>> +       pci_generic_ecam_free(ri->cfg);
>> +       kfree(ri);
>> +}
>> +
>> +static struct acpi_pci_root_ops acpi_pci_root_ops = {
>> +       .release_info = pci_acpi_generic_release_info,
>> +};
>> +
>> +/* Interface called from ACPI code to setup PCI host controller */
>> +struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
>> +{
>> +       int node = acpi_get_node(root->device->handle);
>> +       struct acpi_pci_generic_root_info *ri;
>> +       struct pci_bus *bus, *child;
>> +       int err;
>> +
>> +       ri = kzalloc_node(sizeof(*ri), GFP_KERNEL, node);
>> +       if (!ri)
>> +               return NULL;
>> +
>> +       err = pci_acpi_setup_ecam_mapping(root, ri);
>> +       if (err)
>> +               return NULL;
>> +
>> +       acpi_pci_root_ops.pci_ops = &ri->cfg->ops->pci_ops;
>> +       bus = acpi_pci_root_create(root, &acpi_pci_root_ops, &ri->common,
>> +                                  ri->cfg);
>> +       if (!bus)
>> +               return NULL;
>> +
>> +       pci_bus_size_bridges(bus);
>> +       pci_bus_assign_resources(bus);
>> +
>> +       list_for_each_entry(child, &bus->children, node)
>> +               pcie_bus_configure_settings(child);
>> +
>> +       return bus;
>> +}
>> +
>> +/* handle MCFG table entries */
>> +static __init int pci_mcfg_parse(struct acpi_table_header *header)
>> +{
>> +       struct acpi_table_mcfg *mcfg;
>> +       struct acpi_mcfg_allocation *mptr;
>> +       struct mcfg_entry *e, *arr;
>> +       int i, n;
>> +
>> +       if (!header)
>> +               return -EINVAL;
>> +
>> +       mcfg = (struct acpi_table_mcfg *)header;
>> +       mptr = (struct acpi_mcfg_allocation *) &mcfg[1];
>> +       n = (header->length - sizeof(*mcfg)) / sizeof(*mptr);
>> +       if (n <= 0 || n > 255) {
>> +               pr_err(PREFIX " MCFG has incorrect entries (%d).\n", n);
>> +               return -EINVAL;
>> +       }
>> +
>> +       arr = kcalloc(n, sizeof(*arr), GFP_KERNEL);
>> +       if (!arr)
>> +               return -ENOMEM;
>
> Here you already have an array which is also connected as a linked
> list which is unnecessary.

The array is to avoid complicated error handling in case the allocation 
for some of element would failed. I can rework this to allocate each 
element separately or to use array but the code is simpler now. As 
explained above (on demand mapping), I would like to keep list/array 
with MCFG entries (I preferred list).

>
>> +       for (i = 0, e = arr; i < n; i++, mptr++, e++) {
>> +               e->segment = mptr->pci_segment;
>> +               e->addr =  mptr->address;
>> +               e->bus_start = mptr->start_bus_number;
>> +               e->bus_end = mptr->end_bus_number;
>> +               list_add(&e->list, &pci_mcfg_list);
>> +               pr_info(PREFIX
>> +                       "MCFG entry for domain %04x [bus %02x-%02x] (base %pa)\n",
>> +                       e->segment, e->bus_start, e->bus_end, &e->addr);
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +/* Interface called by ACPI - parse and save MCFG table */
>> +void __init pci_mcfg_init(void)
>> +{
>> +       int err = acpi_table_parse(ACPI_SIG_MCFG, pci_mcfg_parse);
>> +       if (err)
>> +               pr_err(PREFIX "Failed to parse MCFG (%d)\n", err);
>> +       else if (list_empty(&pci_mcfg_list))
>> +               pr_info(PREFIX "No valid entries in MCFG table.\n");
>> +       else {
>> +               struct mcfg_entry *e;
>> +               int i = 0;
>> +               list_for_each_entry(e, &pci_mcfg_list, list)
>> +                       i++;
>> +               pr_info(PREFIX "MCFG table loaded, %d entries\n", i);
>> +       }
>> +}
>> +
>> +/* Raw operations, works only for MCFG entries with an associated bus */
>> +int raw_pci_read(unsigned int domain, unsigned int busn, unsigned int devfn,
>> +                int reg, int len, u32 *val)
>> +{
>> +       struct pci_bus *bus = pci_find_bus(domain, busn);
>> +
>> +       if (!bus)
>> +               return PCIBIOS_DEVICE_NOT_FOUND;
>> +       return bus->ops->read(bus, devfn, reg, len, val);
>> +}
>> +
>> +int raw_pci_write(unsigned int domain, unsigned int busn, unsigned int devfn,
>> +                 int reg, int len, u32 val)
>> +{
>> +       struct pci_bus *bus = pci_find_bus(domain, busn);
>> +
>> +       if (!bus)
>> +               return PCIBIOS_DEVICE_NOT_FOUND;
>> +       return bus->ops->write(bus, devfn, reg, len, val);
>> +}
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index df1f33d..c0422ea 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -1729,6 +1729,12 @@ static inline void pci_mmcfg_early_init(void) { }
>>   static inline void pci_mmcfg_late_init(void) { }
>>   #endif
>>
>> +#ifdef CONFIG_ACPI_PCI_HOST_GENERIC
>> +void __init pci_mcfg_init(void);
>> +#else
>> +static inline void pci_mcfg_init(void) { return; }
>> +#endif
>
> You can still use the function pci_mmcfg_late_init() if
> PCI_MMCONFIG or ACPI_PCI_HOST_GENERIC is defined
>

OK

-#ifdef CONFIG_PCI_MMCONFIG
+#if defined(CONFIG_PCI_MMCONFIG) || defined(ACPI_PCI_HOST_GENERIC)
void __init pci_mmcfg_early_init(void);
void __init pci_mmcfg_late_init(void);
#else
static inline void pci_mmcfg_early_init(void) { }
static inline void pci_mmcfg_late_init(void) { }
#endif

is that what you mean?

Tomasz

[toc] | [prev] | [next] | [standalone]

#1385140 — Re: [PATCH V6 09/13] pci, acpi: Support for ACPI based generic PCI host controller

On Thu, Apr 21, 2016 at 2:36 PM, Tomasz Nowicki <tn@semihalf.com> wrote:
> On 20.04.2016 21:12, Jayachandran C wrote:
>>
>> On Fri, Apr 15, 2016 at 10:36 PM, Tomasz Nowicki <tn@semihalf.com> wrote:
>>>
>>> This patch is going to implement generic PCI host controller for
>>> ACPI world, similar to what pci-host-generic.c driver does for DT world.
>>>
>>> All such drivers, which we have seen so far, were implemented within
>>> arch/ directory since they had some arch assumptions (x86 and ia64).
>>> However, they all are doing similar thing, so it makes sense to find
>>> some common code and abstract it into the generic driver.
>>>
>>> In order to handle PCI config space regions properly, we define new
>>> MCFG interface which parses MCFG table and keep its entries
>>> in a list. New pci_mcfg_init call is defined so that we do not depend
>>> on PCI_MMCONFIG. Regions are not mapped until host bridge ask for it.
>>>
>>> The implementation of pci_acpi_scan_root() looks up the saved MCFG
>>> entries
>>> and sets up a new mapping. Generic PCI functions are used for
>>> accessing config space. Driver selects PCI_GENERIC_ECAM and uses
>>> functions
>>> from drivers/pci/ecam.h to create and access ECAM mappings.
>>>
>>> As mentioned in Kconfig help section, ACPI_PCI_HOST_GENERIC choice
>>> should be made on a per-architecture basis.
>>>
>>> This patch is heavily based on the updated version from Jayachandran C:
>>> https://lkml.org/lkml/2016/4/11/908
>>> git: https://github.com/jchandra-brcm/linux/ (arm64-acpi-pci-v3)
>>
>>
>> This is a little bit unusual because I had not posted the v3 patch
>> to the mailing list yet, but you posted a variant of it The git repository
>> should not be in the commit comment because it is a temporary location.
>
>
> We all agree this too important for everybody to delay this series. So main
> motivation is to keep all discussion&patches within one unified series. I
> would like to finally find direction we need to go. Stating another
> discussion based on my previous patch set v5 confused people, they do no
> know who is driving this. Again, lets cooperate to move it forward within
> one patch set.
>
> I agree with you we need maintainers to join this discussion.
>
>>
>> There are some changes here I don't agree with. I think it will be
>> better if you can post a version without the quirk handling and with
>> some of the suggestions below.
>
>
> The next version will not have quirk handling part. Regarding your comments,
> please see below.
>
>
>>
>>> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
>>> Signed-off-by: Jayachandran C <jchandra@broadcom.com>
>>> ---
>>>   drivers/acpi/Kconfig        |   8 ++
>>>   drivers/acpi/Makefile       |   1 +
>>>   drivers/acpi/bus.c          |   1 +
>>>   drivers/acpi/pci_gen_host.c | 231
>>> ++++++++++++++++++++++++++++++++++++++++++++
>>>   include/linux/pci.h         |   6 ++
>>>   5 files changed, 247 insertions(+)
>>>   create mode 100644 drivers/acpi/pci_gen_host.c
>>>
>>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>>> index 183ffa3..70272c5 100644
>>> --- a/drivers/acpi/Kconfig
>>> +++ b/drivers/acpi/Kconfig
>>> @@ -346,6 +346,14 @@ config ACPI_PCI_SLOT
>>>            i.e., segment/bus/device/function tuples, with physical slots
>>> in
>>>            the system.  If you are unsure, say N.
>>>
>>> +config ACPI_PCI_HOST_GENERIC
>>> +       bool
>>> +       select PCI_GENERIC_ECAM
>>> +       help
>>> +         Select this config option from the architecture Kconfig,
>>> +         if it is preferred to enable ACPI PCI host controller driver
>>> which
>>> +         has no arch-specific assumptions.
>>> +
>>>   config X86_PM_TIMER
>>>          bool "Power Management Timer Support" if EXPERT
>>>          depends on X86
>>> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
>>> index 81e5cbc..b12fa64 100644
>>> --- a/drivers/acpi/Makefile
>>> +++ b/drivers/acpi/Makefile
>>> @@ -40,6 +40,7 @@ acpi-$(CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC) +=
>>> processor_pdc.o
>>>   acpi-y                         += ec.o
>>>   acpi-$(CONFIG_ACPI_DOCK)       += dock.o
>>>   acpi-y                         += pci_root.o pci_link.o pci_irq.o
>>> +obj-$(CONFIG_ACPI_PCI_HOST_GENERIC)    += pci_gen_host.o
>>>   acpi-y                         += acpi_lpss.o acpi_apd.o
>>>   acpi-y                         += acpi_platform.o
>>>   acpi-y                         += acpi_pnp.o
>>> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
>>> index c068c82..803a1d7 100644
>>> --- a/drivers/acpi/bus.c
>>> +++ b/drivers/acpi/bus.c
>>> @@ -1107,6 +1107,7 @@ static int __init acpi_init(void)
>>>          }
>>>
>>>          pci_mmcfg_late_init();
>>> +       pci_mcfg_init();
>>
>>
>> Please see below.
>>
>>>          acpi_scan_init();
>>>          acpi_ec_init();
>>>          acpi_debugfs_init();
>>> diff --git a/drivers/acpi/pci_gen_host.c b/drivers/acpi/pci_gen_host.c
>>> new file mode 100644
>>> index 0000000..fd360b5
>>> --- /dev/null
>>> +++ b/drivers/acpi/pci_gen_host.c
>>> @@ -0,0 +1,231 @@
>>> +/*
>>
>>
>> You seem to have removed the copyright line, this is not proper, you
>> should probably add your copyright line if you think your changes are
>> significant.
>
>
> I rather forgot to add copyright here, I will fix it.
>
>
>>
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License, version 2, as
>>> + * published by the Free Software Foundation (the "GPL").
>>> + *
>>> + * This program is distributed in the hope that it will be useful, but
>>> + * WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>> + * General Public License version 2 (GPLv2) for more details.
>>> + *
>>> + * You should have received a copy of the GNU General Public License
>>> + * version 2 (GPLv2) along with this source code.
>>> + */
>>> +#include <linux/kernel.h>
>>> +#include <linux/pci.h>
>>> +#include <linux/pci-acpi.h>
>>> +#include <linux/sfi_acpi.h>
>>> +#include <linux/slab.h>
>>> +
>>> +#include "../pci/ecam.h"
>>> +
>>> +#define PREFIX "ACPI: "
>>> +
>>> +/* Structure to hold entries from the MCFG table */
>>> +struct mcfg_entry {
>>> +       struct list_head        list;
>>> +       phys_addr_t             addr;
>>> +       u16                     segment;
>>> +       u8                      bus_start;
>>> +       u8                      bus_end;
>>> +};
>>> +
>>> +/* List to save mcfg entries */
>>> +static LIST_HEAD(pci_mcfg_list);
>>> +static DEFINE_MUTEX(pci_mcfg_lock);
>>
>>
>> There is no need to use a list or lock here, I had used an
>> array and that is sufficient since it is not modified after it
>> is filled initially.
>
>
> ACPI PCI driver supports hot plug/removal, I want to avoid races using this
> lock. I decided to use list because I do ECAM mapping on demand. See below
> for more details.

Yes, there is hotplug. but there is no change to the saved MCFG entries
after you create the array (or list). And, the ECAM mapping is on demand
but it also just does a lookup and does not modify the array/list, so the
locking is not needed.

There may be a locking issue for hotplug in raw_pci_read/write between
find_bus and bus->ops->read/write, but this does not solve that. My
expectation is that since both are ACPI originated this may not be
an issue.

>>
>>> +/* ACPI info for generic ACPI PCI controller */
>>> +struct acpi_pci_generic_root_info {
>>> +       struct acpi_pci_root_info       common;
>>> +       struct pci_config_window        *cfg;   /* config space mapping
>>> */
>>> +};
>>> +
>>> +/* Find the entry in mcfg list which contains range bus_start */
>>> +static struct mcfg_entry *pci_mcfg_lookup(u16 seg, u8 bus_start)
>>> +{
>>> +       struct mcfg_entry *e;
>>> +
>>> +       list_for_each_entry(e, &pci_mcfg_list, list) {
>>> +               if (e->segment == seg &&
>>> +                   e->bus_start <= bus_start && bus_start <= e->bus_end)
>>> +                       return e;
>>> +       }
>>> +
>>> +       return NULL;
>>> +}
>>> +
>>> +
>>> +/*
>>> + * Lookup the bus range for the domain in MCFG, and set up config space
>>> + * mapping.
>>> + */
>>> +static int pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root,
>>> +                                      struct acpi_pci_generic_root_info
>>> *ri)
>>> +{
>>> +       u16 seg = root->segment;
>>> +       u8 bus_start = root->secondary.start;
>>> +       u8 bus_end = root->secondary.end;
>>> +       struct pci_config_window *cfg;
>>> +       struct mcfg_entry *e;
>>> +       phys_addr_t addr;
>>> +       int err = 0;
>>> +
>>> +       mutex_lock(&pci_mcfg_lock);
>>> +       e = pci_mcfg_lookup(seg, bus_start);
>>> +       if (!e) {
>>> +               addr = acpi_pci_root_get_mcfg_addr(root->device->handle);
>>
>>
>> The acpi_pci_root_get_mcfg_addr() is already called in pci_root.c, doing
>> it again here is unnecessary.
>
>
> I put it here as per Bjorn's request, see:
> https://lkml.org/lkml/2016/3/4/946
>
> If this is not valid any more, I can easily remove it.

There are multiple ways in which acpi_pci_root_add() tries get the
bus range and mcfg address for a root bus. I think Bjorn's suggestion
was to add looking up the MCFG there too, and I think it would be
better if it was done in a separate patchset.

>>
>> I think you can have a function to pick up addr, bus_start, bus_end given
>> a domain from either MCFG or using _CBA method, but I think that
>> should be done in pci_root.c in a separate patch.
>>
>>> +               if (addr == 0) {
>>> +                       pr_err(PREFIX"%04x:%02x-%02x bus range error\n",
>>> +                              seg, bus_start, bus_end);
>>> +                       err = -ENOENT;
>>> +                       goto err_out;
>>> +               }
>>> +       } else {
>>> +               if (bus_start != e->bus_start) {
>>> +                       pr_err("%04x:%02x-%02x bus range mismatch
>>> %02x\n",
>>> +                              seg, bus_start, bus_end, e->bus_start);
>>> +                       err = -EINVAL;
>>> +                       goto err_out;
>>> +               } else if (bus_end != e->bus_end) {
>>> +                       pr_warn("%04x:%02x-%02x bus end mismatch %02x\n",
>>> +                               seg, bus_start, bus_end, e->bus_end);
>>> +                       bus_end = min(bus_end, e->bus_end);
>>> +               }
>>> +               addr = e->addr;
>>> +       }
>>> +
>>> +       cfg = pci_generic_ecam_create(&root->device->dev, addr,
>>> bus_start,
>>> +                                     bus_end,
>>> &pci_generic_ecam_default_ops);
>>> +       if (IS_ERR(cfg)) {
>>> +               err = PTR_ERR(cfg);
>>> +               pr_err("%04x:%02x-%02x error %d mapping CAM\n", seg,
>>> +                       bus_start, bus_end, err);
>>> +               goto err_out;
>>> +       }
>>
>>
>> You seem to have moved all the config space mapping to this
>> point. Intel seems to do the mapping when they read MCFG for the
>> entries, and I had followed that model, and that avoids having another
>> array/list to save the values.
>
>
> See:
> https://lkml.org/lkml/2016/3/3/921
>
> I agree with Bjorn here, we should map it whenever we need this instead of
> mapping all MCFG entries just in case. Also, I see other advantages:
> 1. We can always use valid "dev" for pci_generic_ecam_create call, what you
> can't do when you use pci_generic_ecam_create during MCFG parsing.
> 2. No need for special handling for entries coming from _CBA, the path is
> the same for MCFG and _CBA. We do not have to remember that only _CBA
> related entries have to be unmapped.
>
>
>>
>>> +       cfg->domain = seg;
>>> +       ri->cfg = cfg;
>>> +err_out:
>>> +       mutex_unlock(&pci_mcfg_lock);
>>> +       return err;
>>> +}
>>> +
>>> +/* release_info: free resrouces allocated by init_info */
>>> +static void pci_acpi_generic_release_info(struct acpi_pci_root_info *ci)
>>> +{
>>> +       struct acpi_pci_generic_root_info *ri;
>>> +
>>> +       ri = container_of(ci, struct acpi_pci_generic_root_info, common);
>>> +       pci_generic_ecam_free(ri->cfg);
>>> +       kfree(ri);
>>> +}
>>> +
>>> +static struct acpi_pci_root_ops acpi_pci_root_ops = {
>>> +       .release_info = pci_acpi_generic_release_info,
>>> +};
>>> +
>>> +/* Interface called from ACPI code to setup PCI host controller */
>>> +struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
>>> +{
>>> +       int node = acpi_get_node(root->device->handle);
>>> +       struct acpi_pci_generic_root_info *ri;
>>> +       struct pci_bus *bus, *child;
>>> +       int err;
>>> +
>>> +       ri = kzalloc_node(sizeof(*ri), GFP_KERNEL, node);
>>> +       if (!ri)
>>> +               return NULL;
>>> +
>>> +       err = pci_acpi_setup_ecam_mapping(root, ri);
>>> +       if (err)
>>> +               return NULL;
>>> +
>>> +       acpi_pci_root_ops.pci_ops = &ri->cfg->ops->pci_ops;
>>> +       bus = acpi_pci_root_create(root, &acpi_pci_root_ops, &ri->common,
>>> +                                  ri->cfg);
>>> +       if (!bus)
>>> +               return NULL;
>>> +
>>> +       pci_bus_size_bridges(bus);
>>> +       pci_bus_assign_resources(bus);
>>> +
>>> +       list_for_each_entry(child, &bus->children, node)
>>> +               pcie_bus_configure_settings(child);
>>> +
>>> +       return bus;
>>> +}
>>> +
>>> +/* handle MCFG table entries */
>>> +static __init int pci_mcfg_parse(struct acpi_table_header *header)
>>> +{
>>> +       struct acpi_table_mcfg *mcfg;
>>> +       struct acpi_mcfg_allocation *mptr;
>>> +       struct mcfg_entry *e, *arr;
>>> +       int i, n;
>>> +
>>> +       if (!header)
>>> +               return -EINVAL;
>>> +
>>> +       mcfg = (struct acpi_table_mcfg *)header;
>>> +       mptr = (struct acpi_mcfg_allocation *) &mcfg[1];
>>> +       n = (header->length - sizeof(*mcfg)) / sizeof(*mptr);
>>> +       if (n <= 0 || n > 255) {
>>> +               pr_err(PREFIX " MCFG has incorrect entries (%d).\n", n);
>>> +               return -EINVAL;
>>> +       }
>>> +
>>> +       arr = kcalloc(n, sizeof(*arr), GFP_KERNEL);
>>> +       if (!arr)
>>> +               return -ENOMEM;
>>
>>
>> Here you already have an array which is also connected as a linked
>> list which is unnecessary.
>
>
> The array is to avoid complicated error handling in case the allocation for
> some of element would failed. I can rework this to allocate each element
> separately or to use array but the code is simpler now. As explained above
> (on demand mapping), I would like to keep list/array with MCFG entries (I
> preferred list).
>
>
>>
>>> +       for (i = 0, e = arr; i < n; i++, mptr++, e++) {
>>> +               e->segment = mptr->pci_segment;
>>> +               e->addr =  mptr->address;
>>> +               e->bus_start = mptr->start_bus_number;
>>> +               e->bus_end = mptr->end_bus_number;
>>> +               list_add(&e->list, &pci_mcfg_list);
>>> +               pr_info(PREFIX
>>> +                       "MCFG entry for domain %04x [bus %02x-%02x] (base
>>> %pa)\n",
>>> +                       e->segment, e->bus_start, e->bus_end, &e->addr);
>>> +       }
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +/* Interface called by ACPI - parse and save MCFG table */
>>> +void __init pci_mcfg_init(void)
>>> +{
>>> +       int err = acpi_table_parse(ACPI_SIG_MCFG, pci_mcfg_parse);
>>> +       if (err)
>>> +               pr_err(PREFIX "Failed to parse MCFG (%d)\n", err);
>>> +       else if (list_empty(&pci_mcfg_list))
>>> +               pr_info(PREFIX "No valid entries in MCFG table.\n");
>>> +       else {
>>> +               struct mcfg_entry *e;
>>> +               int i = 0;
>>> +               list_for_each_entry(e, &pci_mcfg_list, list)
>>> +                       i++;
>>> +               pr_info(PREFIX "MCFG table loaded, %d entries\n", i);
>>> +       }
>>> +}
>>> +
>>> +/* Raw operations, works only for MCFG entries with an associated bus */
>>> +int raw_pci_read(unsigned int domain, unsigned int busn, unsigned int
>>> devfn,
>>> +                int reg, int len, u32 *val)
>>> +{
>>> +       struct pci_bus *bus = pci_find_bus(domain, busn);
>>> +
>>> +       if (!bus)
>>> +               return PCIBIOS_DEVICE_NOT_FOUND;
>>> +       return bus->ops->read(bus, devfn, reg, len, val);
>>> +}
>>> +
>>> +int raw_pci_write(unsigned int domain, unsigned int busn, unsigned int
>>> devfn,
>>> +                 int reg, int len, u32 val)
>>> +{
>>> +       struct pci_bus *bus = pci_find_bus(domain, busn);
>>> +
>>> +       if (!bus)
>>> +               return PCIBIOS_DEVICE_NOT_FOUND;
>>> +       return bus->ops->write(bus, devfn, reg, len, val);
>>> +}
>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>> index df1f33d..c0422ea 100644
>>> --- a/include/linux/pci.h
>>> +++ b/include/linux/pci.h
>>> @@ -1729,6 +1729,12 @@ static inline void pci_mmcfg_early_init(void) { }
>>>   static inline void pci_mmcfg_late_init(void) { }
>>>   #endif
>>>
>>> +#ifdef CONFIG_ACPI_PCI_HOST_GENERIC
>>> +void __init pci_mcfg_init(void);
>>> +#else
>>> +static inline void pci_mcfg_init(void) { return; }
>>> +#endif
>>
>>
>> You can still use the function pci_mmcfg_late_init() if
>> PCI_MMCONFIG or ACPI_PCI_HOST_GENERIC is defined
>>
>
> OK
>
> -#ifdef CONFIG_PCI_MMCONFIG
> +#if defined(CONFIG_PCI_MMCONFIG) || defined(ACPI_PCI_HOST_GENERIC)
> void __init pci_mmcfg_early_init(void);
> void __init pci_mmcfg_late_init(void);
> #else
> static inline void pci_mmcfg_early_init(void) { }
> static inline void pci_mmcfg_late_init(void) { }
> #endif
>
> is that what you mean?

Yes - this should be sufficient instead of a new function...

JC.

[toc] | [prev] | [next] | [standalone]

#1385240 — Re: [PATCH V6 09/13] pci, acpi: Support for ACPI based generic PCI host controller

On 04/21/2016 05:06 AM, Tomasz Nowicki wrote:
> On 20.04.2016 21:12, Jayachandran C wrote:
>> On Fri, Apr 15, 2016 at 10:36 PM, Tomasz Nowicki <tn@semihalf.com> wrote:

>>> This patch is heavily based on the updated version from Jayachandran C:
>>> https://lkml.org/lkml/2016/4/11/908
>>> git: https://github.com/jchandra-brcm/linux/ (arm64-acpi-pci-v3)
>>
>> This is a little bit unusual because I had not posted the v3 patch
>> to the mailing list yet, but you posted a variant of it The git
>> repository
>> should not be in the commit comment because it is a temporary location.
> 
> We all agree this too important for everybody to delay this series. So
> main motivation is to keep all discussion&patches within one unified
> series. I would like to finally find direction we need to go. Stating
> another discussion based on my previous patch set v5 confused people,
> they do no know who is driving this. Again, lets cooperate to move it
> forward within one patch set.

We need one person in the driver's seat here for this patch series. I
believe the intention is that this is Tomasz, with others cooperating
and assisting. The previous alternative patch series did serve to cause
confusion, and worse, they made it look like the ARM vendors can't work
together. That ends. Right now. I've raised this individually with each
of you (and with all of the other vendors), as well as inside Linaro.
There will be one person driving this, and everyone else will help.

> I agree with you we need maintainers to join this discussion.

Please. I really want to lend support to the sense of urgency of getting
something merged that provides the basic ACPI based ECAM functionality
on ARMv8 systems. Until that happens, there are vendors who are going to
have to delay various other activities around ARM server until it is
done (because of the lack of an upstream patch is a critical blocker -
this isn't embedded, and we don't work that way).

This is incredibly critical and central to the successful adoption of
larger ARMv8 servers over the next year or so, all of which will rely
upon PCIe, using ACPI. We therefore need all hands on deck, and all
vendors getting overwhelmingly excited about making this patch series a
success. As an aside, to help the engineering orgs within these vendors
understand how much I need motion on this thread, I'm buzzing in their
ears multiple times per week specifically on engagement on this thread.
We need this done. If Bjorn or anyone else needs something, tell us.

Jon.

-- 
Computer Architect | Sent from my Fedora powered laptop

[toc] | [prev] | [next] | [standalone]

Page 2 of 4 — ← Prev page 1 [2] 3 4  Next page →

Back to top | Article view | linux.kernel

csiph-web