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 3 of 4 — ← Prev page 1 2 [3] 4  Next page →

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

On 04/22/2016 10:40 AM, Jon Masters wrote:
> 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.

As a quick update, since yesterday I have confirmed that several
different microarchitecture implementations (different PCIe) have tested
and validated this patch series. Those minimally include:

* Cavium Networks ThunderX
* Qualcomm Technologies Inc QDF2XXX
* AMD A1100 ("Seattle")

Another is working on testing over the weekend. Still waiting for an ARM
tested-by on Juno I think. I will personally be testing this and future
releases on all of the above mentioned hw.

Jon.

-- 
Computer Architect | Sent from my Fedora powered laptop

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

#1380034 — [PATCH V6 10/13] arm64, pci, acpi: Start using ACPI based PCI host controller driver for ARM64.

It is perfectly fine to use ACPI_PCI_HOST_GENERIC for ARM64,
so lets get rid of PCI init and RAW ACPI accessor empty stubs
and go with full-blown PCI host controller driver.

Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
To: Catalin Marinas <catalin.marinas@arm.com>
To: Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>
To: Will Deacon <will.deacon@arm.com>
To: Arnd Bergmann <arnd@arndb.de>
Cc: Liviu Dudau <Liviu.Dudau@arm.com>
---
 arch/arm64/Kconfig      |  1 +
 arch/arm64/kernel/pci.c | 24 ------------------------
 2 files changed, 1 insertion(+), 24 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 4f43622..1bded87 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -2,6 +2,7 @@ config ARM64
 	def_bool y
 	select ACPI_CCA_REQUIRED if ACPI
 	select ACPI_GENERIC_GSI if ACPI
+	select ACPI_PCI_HOST_GENERIC if ACPI
 	select ACPI_REDUCED_HARDWARE_ONLY if ACPI
 	select ARCH_HAS_DEVMEM_IS_ALLOWED
 	select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE
diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index 15109c11..9a8e6f7 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -63,27 +63,3 @@ int pcibios_alloc_irq(struct pci_dev *dev)
 
 	return 0;
 }
-
-/*
- * raw_pci_read/write - Platform-specific PCI config space access.
- */
-int raw_pci_read(unsigned int domain, unsigned int bus,
-		  unsigned int devfn, int reg, int len, u32 *val)
-{
-	return -ENXIO;
-}
-
-int raw_pci_write(unsigned int domain, unsigned int bus,
-		unsigned int devfn, int reg, int len, u32 val)
-{
-	return -ENXIO;
-}
-
-#ifdef CONFIG_ACPI
-/* Root bridge scanning */
-struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
-{
-	/* TODO: Should be revisited when implementing PCI on ACPI */
-	return NULL;
-}
-#endif
-- 
1.9.1

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

#1380043 — [PATCH V6 06/13] arm64, pci, acpi: ACPI support for legacy IRQs parsing and consolidation with DT code.

To enable PCI legacy IRQs on platforms booting with ACPI, arch code
should include ACPI specific callbacks that parse and set-up the
device IRQ number, equivalent to the DT boot path. Owing to the current
ACPI core scan handlers implementation, ACPI PCI legacy IRQs bindings
cannot be parsed at device add time, since that would trigger ACPI scan
handlers ordering issues depending on how the ACPI tables are defined.

To solve this problem and consolidate FW PCI legacy IRQs parsing in
one single pcibios callback (pending final removal), this patch moves
DT PCI IRQ parsing to the pcibios_alloc_irq() callback (called by
PCI core code at device probe time) and adds ACPI PCI legacy IRQs
parsing to the same callback too, so that FW PCI legacy IRQs parsing
is confined in one single arch callback that can be easily removed
when code parsing PCI legacy IRQs is consolidated and moved to core
PCI code.

Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
Suggested-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
---
 arch/arm64/kernel/pci.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index c72de66..15109c11 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -50,11 +50,16 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
 }
 
 /*
- * Try to assign the IRQ number from DT when adding a new device
+ * Try to assign the IRQ number when probing a new device
  */
-int pcibios_add_device(struct pci_dev *dev)
+int pcibios_alloc_irq(struct pci_dev *dev)
 {
-	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
+	if (acpi_disabled)
+		dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
+#ifdef CONFIG_ACPI
+	else
+		return acpi_pci_irq_enable(dev);
+#endif
 
 	return 0;
 }
-- 
1.9.1

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

#1388608 — Re: [PATCH V6 06/13] arm64, pci, acpi: ACPI support for legacy IRQs parsing and consolidation with DT code.

On Fri, Apr 15, 2016 at 07:06:41PM +0200, Tomasz Nowicki wrote:
> To enable PCI legacy IRQs on platforms booting with ACPI, arch code
> should include ACPI specific callbacks that parse and set-up the
> device IRQ number, equivalent to the DT boot path. Owing to the current
> ACPI core scan handlers implementation, ACPI PCI legacy IRQs bindings
> cannot be parsed at device add time, since that would trigger ACPI scan
> handlers ordering issues depending on how the ACPI tables are defined.

Can you be a little more specific about the issue here?  I think you
mean pci_device_add()-time, because that's where we call
pcibios_add_device.  Which ACPI tables are involved?  _PRT?  Why is
that a problem?  We don't cache those tables any more after
181380b702ee ("PCI/ACPI: Don't cache _PRT, and don't associate them
with bus numbers").

x86 and ia64 both call acpi_pci_irq_enable() from
pcibios_enable_device().  Could you do the same on ARM64?
pcibios_enable_device() happens later than either pci_device_add() or
pci_device_probe().

> To solve this problem and consolidate FW PCI legacy IRQs parsing in
> one single pcibios callback (pending final removal), this patch moves
> DT PCI IRQ parsing to the pcibios_alloc_irq() callback (called by
> PCI core code at device probe time) and adds ACPI PCI legacy IRQs
> parsing to the same callback too, so that FW PCI legacy IRQs parsing
> is confined in one single arch callback that can be easily removed
> when code parsing PCI legacy IRQs is consolidated and moved to core
> PCI code.
> 
> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> Suggested-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> ---
>  arch/arm64/kernel/pci.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index c72de66..15109c11 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -50,11 +50,16 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
>  }
>  
>  /*
> - * Try to assign the IRQ number from DT when adding a new device
> + * Try to assign the IRQ number when probing a new device
>   */
> -int pcibios_add_device(struct pci_dev *dev)
> +int pcibios_alloc_irq(struct pci_dev *dev)
>  {
> -	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
> +	if (acpi_disabled)
> +		dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
> +#ifdef CONFIG_ACPI
> +	else
> +		return acpi_pci_irq_enable(dev);
> +#endif

Not your problem, but your patch makes it obvious: it's ugly that we
set dev->irq to the IRQ returned from of_irq_parse_and_map_pci(), but
acpi_pci_irq_enable() sets dev->irq internally.

x86 also has the situation of calling either acpi_pci_irq_enable() or
of_irq_parse_and_map_pci(), and it looks like they can even decide at
run-time as you can here.  If we're solving the same problem, can we
use a similar mechanism?  x86 sets a pcibios_enable_irq function
pointer.

>  	return 0;
>  }
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

#1388930 — Re: [PATCH V6 06/13] arm64, pci, acpi: ACPI support for legacy IRQs parsing and consolidation with DT code.

On Tue, Apr 26, 2016 at 09:44:30PM -0500, Bjorn Helgaas wrote:
> On Fri, Apr 15, 2016 at 07:06:41PM +0200, Tomasz Nowicki wrote:
> > To enable PCI legacy IRQs on platforms booting with ACPI, arch code
> > should include ACPI specific callbacks that parse and set-up the
> > device IRQ number, equivalent to the DT boot path. Owing to the current
> > ACPI core scan handlers implementation, ACPI PCI legacy IRQs bindings
> > cannot be parsed at device add time, since that would trigger ACPI scan
> > handlers ordering issues depending on how the ACPI tables are defined.
> 
> Can you be a little more specific about the issue here?  I think you
> mean pci_device_add()-time, because that's where we call
> pcibios_add_device.  Which ACPI tables are involved?  _PRT?  Why is
> that a problem?  We don't cache those tables any more after
> 181380b702ee ("PCI/ACPI: Don't cache _PRT, and don't associate them
> with bus numbers").

https://lists.linaro.org/pipermail/linaro-acpi/2015-October/005944.html

I think it is a scan handler ordering issue and probably by caching
_PRT this problem would not exist but I have to read the commit above
in details to understand if that's the case.

> x86 and ia64 both call acpi_pci_irq_enable() from
> pcibios_enable_device().  Could you do the same on ARM64?
> pcibios_enable_device() happens later than either pci_device_add() or
> pci_device_probe().

We could in theory. In practice we have to see if that triggers DT
regressions on PCI host controllers that do not call pci_fixup_irqs(),
but rely on the legacy IRQ routing to be done in arm64
pcibios_add_device().

> > To solve this problem and consolidate FW PCI legacy IRQs parsing in
> > one single pcibios callback (pending final removal), this patch moves
> > DT PCI IRQ parsing to the pcibios_alloc_irq() callback (called by
> > PCI core code at device probe time) and adds ACPI PCI legacy IRQs
> > parsing to the same callback too, so that FW PCI legacy IRQs parsing
> > is confined in one single arch callback that can be easily removed
> > when code parsing PCI legacy IRQs is consolidated and moved to core
> > PCI code.
> > 
> > Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> > Suggested-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > ---
> >  arch/arm64/kernel/pci.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> > index c72de66..15109c11 100644
> > --- a/arch/arm64/kernel/pci.c
> > +++ b/arch/arm64/kernel/pci.c
> > @@ -50,11 +50,16 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
> >  }
> >  
> >  /*
> > - * Try to assign the IRQ number from DT when adding a new device
> > + * Try to assign the IRQ number when probing a new device
> >   */
> > -int pcibios_add_device(struct pci_dev *dev)
> > +int pcibios_alloc_irq(struct pci_dev *dev)
> >  {
> > -	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
> > +	if (acpi_disabled)
> > +		dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
> > +#ifdef CONFIG_ACPI
> > +	else
> > +		return acpi_pci_irq_enable(dev);
> > +#endif
> 
> Not your problem, but your patch makes it obvious: it's ugly that we
> set dev->irq to the IRQ returned from of_irq_parse_and_map_pci(), but
> acpi_pci_irq_enable() sets dev->irq internally.
> 
> x86 also has the situation of calling either acpi_pci_irq_enable() or
> of_irq_parse_and_map_pci(), and it looks like they can even decide at
> run-time as you can here.  If we're solving the same problem, can we
> use a similar mechanism?  x86 sets a pcibios_enable_irq function
> pointer.

Yes we could, but that's orthogonal to this patch, it's basically
rewriting this code in a different way and adding flexibility to the
function mapping irqs.

Thanks,
Lorenzo

> 
> >  	return 0;
> >  }
> > -- 
> > 1.9.1
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

#1380044 — [PATCH V6 07/13] PCI: Provide common functions for ECAM mapping

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>
---
 drivers/pci/Kconfig  |   3 ++
 drivers/pci/Makefile |   2 +
 drivers/pci/ecam.c   | 137 +++++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/ecam.h   |  61 +++++++++++++++++++++++
 4 files changed, 203 insertions(+)
 create mode 100644 drivers/pci/ecam.c
 create mode 100644 drivers/pci/ecam.h

diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
index 209292e..e930d62 100644
--- a/drivers/pci/Kconfig
+++ b/drivers/pci/Kconfig
@@ -83,6 +83,9 @@ config HT_IRQ
 config PCI_ATS
 	bool
 
+config PCI_GENERIC_ECAM
+	bool
+
 config PCI_IOV
 	bool "PCI IOV support"
 	depends on PCI
diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index 2154092..810aec8 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -55,6 +55,8 @@ obj-$(CONFIG_PCI_SYSCALL) += syscall.o
 
 obj-$(CONFIG_PCI_STUB) += pci-stub.o
 
+obj-$(CONFIG_PCI_GENERIC_ECAM) += ecam.o
+
 obj-$(CONFIG_XEN_PCIDEV_FRONTEND) += xen-pcifront.o
 
 obj-$(CONFIG_OF) += of.o
diff --git a/drivers/pci/ecam.c b/drivers/pci/ecam.c
new file mode 100644
index 0000000..ff04c01
--- /dev/null
+++ b/drivers/pci/ecam.c
@@ -0,0 +1,137 @@
+/*
+ * Copyright 2016 Broadcom
+ *
+ * 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/device.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/slab.h>
+
+#include "ecam.h"
+
+/*
+ * On 64 bit systems, we do a single ioremap for the whole config space
+ * since we have enough virtual address range available. On 32 bit, do an
+ * ioremap per bus.
+ */
+static const bool per_bus_mapping = !config_enabled(CONFIG_64BIT);
+
+/*
+ * Create a PCI config space window
+ *  - reserve mem region
+ *  - alloc struct pci_config_window with space for all mappings
+ *  - ioremap the config space
+ */
+struct pci_config_window *pci_generic_ecam_create(struct device *dev,
+				phys_addr_t addr, u8 bus_start, u8 bus_end,
+				struct pci_generic_ecam_ops *ops)
+{
+	struct pci_config_window *cfg;
+	unsigned int bus_shift, bus_range, bsz, mapsz;
+	int i, nidx;
+	int err = -ENOMEM;
+
+	if (bus_end < bus_start)
+		return ERR_PTR(-EINVAL);
+
+	bus_shift = ops->bus_shift;
+	bus_range = bus_end - bus_start + 1;
+	bsz = 1 << bus_shift;
+	nidx = per_bus_mapping ? bus_range : 1;
+	mapsz = per_bus_mapping ? bsz : bus_range * bsz;
+	cfg = kzalloc(sizeof(*cfg) + nidx * sizeof(cfg->win[0]), GFP_KERNEL);
+	if (!cfg)
+		return ERR_PTR(-ENOMEM);
+
+	cfg->bus_start = bus_start;
+	cfg->bus_end = bus_end;
+	cfg->ops = ops;
+
+	if (!request_mem_region(addr, bus_range * bsz, "Configuration Space"))
+		goto err_exit;
+
+	/* cfgaddr has to be set after request_mem_region */
+	cfg->cfgaddr = addr;
+
+	for (i = 0; i < nidx; i++) {
+		cfg->win[i] = ioremap(addr + i * mapsz, mapsz);
+		if (!cfg->win[i])
+			goto err_exit;
+	}
+
+	if (cfg->ops->init) {
+		err = cfg->ops->init(dev, cfg);
+		if (err)
+			goto err_exit;
+	}
+	return cfg;
+
+err_exit:
+	pci_generic_ecam_free(cfg);
+	return ERR_PTR(err);
+}
+
+/*
+ * Free a config space mapping
+ */
+void pci_generic_ecam_free(struct pci_config_window *cfg)
+{
+	unsigned int bus_range;
+	int i, nidx;
+
+	bus_range = cfg->bus_end - cfg->bus_start + 1;
+	nidx = per_bus_mapping ? bus_range : 1;
+	for (i = 0; i < nidx; i++)
+		if (cfg->win[i])
+			iounmap(cfg->win[i]);
+	if (cfg->cfgaddr)
+		release_mem_region(cfg->cfgaddr,
+				   bus_range << cfg->ops->bus_shift);
+	kfree(cfg);
+}
+
+/*
+ * Function to implement the pci_ops ->map_bus method
+ */
+void __iomem *pci_generic_ecam_map_bus(struct pci_bus *bus, unsigned int devfn,
+				       int where)
+{
+	struct pci_config_window *cfg = bus->sysdata;
+	unsigned int devfn_shift = cfg->ops->bus_shift - 8;
+	unsigned int busn = bus->number;
+	void __iomem *base;
+
+	if (busn < cfg->bus_start || busn > cfg->bus_end)
+		return NULL;
+
+	busn -= cfg->bus_start;
+	if (per_bus_mapping)
+		base = cfg->win[busn];
+	else
+		base = cfg->win[0] + (busn << cfg->ops->bus_shift);
+	return base + (devfn << devfn_shift) + where;
+}
+
+/* default ECAM ops */
+struct pci_generic_ecam_ops pci_generic_ecam_default_ops = {
+	.bus_shift	= 20,
+	.pci_ops		= {
+		.map_bus	= pci_generic_ecam_map_bus,
+		.read		= pci_generic_config_read,
+		.write		= pci_generic_config_write,
+	}
+};
diff --git a/drivers/pci/ecam.h b/drivers/pci/ecam.h
new file mode 100644
index 0000000..34c0aba
--- /dev/null
+++ b/drivers/pci/ecam.h
@@ -0,0 +1,61 @@
+/*
+ * Copyright 2016 Broadcom
+ *
+ * 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.
+ */
+#ifndef DRIVERS_PCI_ECAM_H
+#define DRIVERS_PCI_ECAM_H
+
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+
+/*
+ * struct to hold pci ops and bus shift of the config window
+ * for a PCI controller.
+ */
+struct pci_config_window;
+struct pci_generic_ecam_ops {
+	unsigned int			bus_shift;
+	struct pci_ops			pci_ops;
+	int				(*init)(struct device *,
+						struct pci_config_window *);
+};
+
+/*
+ * struct to hold the mappings of a config space window. This
+ * will be allocated with enough entries in win[] to hold all
+ * the mappings for the bus range.
+ */
+struct pci_config_window {
+	phys_addr_t			cfgaddr;
+	u16				domain;
+	u8				bus_start;
+	u8				bus_end;
+	void				*priv;
+	struct pci_generic_ecam_ops	*ops;
+	void __iomem			*win[0];
+};
+
+/* create and free for pci_config_window */
+struct pci_config_window *pci_generic_ecam_create(struct device *dev,
+				phys_addr_t addr, u8 bus_start, u8 bus_end,
+				struct pci_generic_ecam_ops *ops);
+void pci_generic_ecam_free(struct pci_config_window *cfg);
+
+/* map_bus when ->sysdata is an instance of pci_config_window */
+void __iomem *pci_generic_ecam_map_bus(struct pci_bus *bus, unsigned int devfn,
+				       int where);
+/* default ECAM ops, bus shift 20, generic read and write */
+extern struct pci_generic_ecam_ops pci_generic_ecam_default_ops;
+
+#endif
-- 
1.9.1

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

#1380115 — Re: [PATCH V6 07/13] PCI: Provide common functions for ECAM mapping

On Friday 15 April 2016 19:06:42 Tomasz Nowicki wrote:
> diff --git a/drivers/pci/ecam.h b/drivers/pci/ecam.h
> new file mode 100644
> index 0000000..34c0aba
> --- /dev/null
> +++ b/drivers/pci/ecam.h
> 

You are including this file from device drivers and potentially from ACPI 
code, so I think this needs to go into include/linux/pci*.h

	Arnd

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

#1380048 — [PATCH V6 01/13] pci, acpi, x86, ia64: Move ACPI host bridge device companion assignment to core code.

Currently we have two platforms (x86 & ia64) capable of PCI ACPI host
bridge initialization. They both use arch-specific sysdata to pass down
parent device reference and both rely on NULL parent in pci_create_root_bus()
to validate sysdata content.

It looks hacky and prevents us from getting some firmware specific
info for PCI host controller based on its acpi_device structure
in generic pci_create_root_bus() function. However, we overcome that
blocker by passing down parent device via pci_create_root_bus parameter
(as the ACPI device type). Then we use ACPI_COMPANION_SET in core code
for ACPI boot method only. ACPI_COMPANION_SET is safe to run for all
cases DT, ACPI and DT&ACPI.

Since now PCI core code is setting ACPI companion device for us,
x86 & ia64 specific ACPI companion device setting turns out to be dead now.
We can get rid of it, including related companion reference from
PCI sysdata structure. Aslo, PCI_CONTROLLER macro cannot return valid
companion device anymore. Therefore we need to convert its usage to
ACPI_COMPANION.

Suggested-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Tested-by: Duc Dang <dhdang@apm.com>
Tested-by: Dongdong Liu <liudongdong3@huawei.com>
Tested-by: Hanjun Guo <hanjun.guo@linaro.org>
Tested-by: Graeme Gregory <graeme.gregory@linaro.org>
Tested-by: Sinan Kaya <okaya@codeaurora.org>
---
 arch/ia64/hp/common/sba_iommu.c    |  2 +-
 arch/ia64/include/asm/pci.h        |  1 -
 arch/ia64/pci/pci.c                | 16 ----------------
 arch/ia64/sn/kernel/io_acpi_init.c |  4 ++--
 arch/x86/include/asm/pci.h         |  3 ---
 arch/x86/pci/acpi.c                | 17 -----------------
 drivers/acpi/pci_root.c            |  7 ++++++-
 drivers/pci/probe.c                |  2 ++
 8 files changed, 11 insertions(+), 41 deletions(-)

diff --git a/arch/ia64/hp/common/sba_iommu.c b/arch/ia64/hp/common/sba_iommu.c
index a6d6190..78e4444 100644
--- a/arch/ia64/hp/common/sba_iommu.c
+++ b/arch/ia64/hp/common/sba_iommu.c
@@ -1981,7 +1981,7 @@ sba_connect_bus(struct pci_bus *bus)
 	if (PCI_CONTROLLER(bus)->iommu)
 		return;
 
-	handle = acpi_device_handle(PCI_CONTROLLER(bus)->companion);
+	handle = acpi_device_handle(ACPI_COMPANION(bus->bridge));
 	if (!handle)
 		return;
 
diff --git a/arch/ia64/include/asm/pci.h b/arch/ia64/include/asm/pci.h
index c0835b0..12423f4 100644
--- a/arch/ia64/include/asm/pci.h
+++ b/arch/ia64/include/asm/pci.h
@@ -63,7 +63,6 @@ extern int pci_mmap_legacy_page_range(struct pci_bus *bus,
 #define pci_legacy_write platform_pci_legacy_write
 
 struct pci_controller {
-	struct acpi_device *companion;
 	void *iommu;
 	int segment;
 	int node;		/* nearest node with memory or NUMA_NO_NODE for global allocation */
diff --git a/arch/ia64/pci/pci.c b/arch/ia64/pci/pci.c
index 8f6ac2f..978d6af 100644
--- a/arch/ia64/pci/pci.c
+++ b/arch/ia64/pci/pci.c
@@ -301,28 +301,12 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
 	}
 
 	info->controller.segment = root->segment;
-	info->controller.companion = device;
 	info->controller.node = acpi_get_node(device->handle);
 	INIT_LIST_HEAD(&info->io_resources);
 	return acpi_pci_root_create(root, &pci_acpi_root_ops,
 				    &info->common, &info->controller);
 }
 
-int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
-{
-	/*
-	 * We pass NULL as parent to pci_create_root_bus(), so if it is not NULL
-	 * here, pci_create_root_bus() has been called by someone else and
-	 * sysdata is likely to be different from what we expect.  Let it go in
-	 * that case.
-	 */
-	if (!bridge->dev.parent) {
-		struct pci_controller *controller = bridge->bus->sysdata;
-		ACPI_COMPANION_SET(&bridge->dev, controller->companion);
-	}
-	return 0;
-}
-
 void pcibios_fixup_device_resources(struct pci_dev *dev)
 {
 	int idx;
diff --git a/arch/ia64/sn/kernel/io_acpi_init.c b/arch/ia64/sn/kernel/io_acpi_init.c
index 231234c..e454492 100644
--- a/arch/ia64/sn/kernel/io_acpi_init.c
+++ b/arch/ia64/sn/kernel/io_acpi_init.c
@@ -132,7 +132,7 @@ sn_get_bussoft_ptr(struct pci_bus *bus)
 	struct acpi_resource_vendor_typed *vendor;
 
 
-	handle = acpi_device_handle(PCI_CONTROLLER(bus)->companion);
+	handle = acpi_device_handle(ACPI_COMPANION(bus->bridge));
 	status = acpi_get_vendor_resource(handle, METHOD_NAME__CRS,
 					  &sn_uuid, &buffer);
 	if (ACPI_FAILURE(status)) {
@@ -360,7 +360,7 @@ sn_acpi_get_pcidev_info(struct pci_dev *dev, struct pcidev_info **pcidev_info,
 	acpi_status status;
 	struct acpi_buffer name_buffer = { ACPI_ALLOCATE_BUFFER, NULL };
 
-	rootbus_handle = acpi_device_handle(PCI_CONTROLLER(dev)->companion);
+	rootbus_handle = acpi_device_handle(ACPI_COMPANION(dev->bus->bridge));
         status = acpi_evaluate_integer(rootbus_handle, METHOD_NAME__SEG, NULL,
                                        &segment);
         if (ACPI_SUCCESS(status)) {
diff --git a/arch/x86/include/asm/pci.h b/arch/x86/include/asm/pci.h
index 9ab7507..24de07d 100644
--- a/arch/x86/include/asm/pci.h
+++ b/arch/x86/include/asm/pci.h
@@ -14,9 +14,6 @@
 struct pci_sysdata {
 	int		domain;		/* PCI domain */
 	int		node;		/* NUMA node */
-#ifdef CONFIG_ACPI
-	struct acpi_device *companion;	/* ACPI companion device */
-#endif
 #ifdef CONFIG_X86_64
 	void		*iommu;		/* IOMMU private data */
 #endif
diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
index 3cd6983..f4ca17a 100644
--- a/arch/x86/pci/acpi.c
+++ b/arch/x86/pci/acpi.c
@@ -340,7 +340,6 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
 		struct pci_sysdata sd = {
 			.domain = domain,
 			.node = node,
-			.companion = root->device
 		};
 
 		memcpy(bus->sysdata, &sd, sizeof(sd));
@@ -355,7 +354,6 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
 		else {
 			info->sd.domain = domain;
 			info->sd.node = node;
-			info->sd.companion = root->device;
 			bus = acpi_pci_root_create(root, &acpi_pci_root_ops,
 						   &info->common, &info->sd);
 		}
@@ -373,21 +371,6 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
 	return bus;
 }
 
-int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
-{
-	/*
-	 * We pass NULL as parent to pci_create_root_bus(), so if it is not NULL
-	 * here, pci_create_root_bus() has been called by someone else and
-	 * sysdata is likely to be different from what we expect.  Let it go in
-	 * that case.
-	 */
-	if (!bridge->dev.parent) {
-		struct pci_sysdata *sd = bridge->bus->sysdata;
-		ACPI_COMPANION_SET(&bridge->dev, sd->companion);
-	}
-	return 0;
-}
-
 int __init pci_acpi_init(void)
 {
 	struct pci_dev *dev = NULL;
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index ae3fe4e..4581e0e 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -564,6 +564,11 @@ static int acpi_pci_root_add(struct acpi_device *device,
 		}
 	}
 
+	/*
+	 * pci_create_root_bus() needs to detect the parent device type,
+	 * so initialize its companion data accordingly.
+	 */
+	ACPI_COMPANION_SET(&device->dev, device);
 	root->device = device;
 	root->segment = segment & 0xFFFF;
 	strcpy(acpi_device_name(device), ACPI_PCI_ROOT_DEVICE_NAME);
@@ -846,7 +851,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
 
 	pci_acpi_root_add_resources(info);
 	pci_add_resource(&info->resources, &root->secondary);
-	bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,
+	bus = pci_create_root_bus(&device->dev, busnum, ops->pci_ops,
 				  sysdata, &info->resources);
 	if (!bus)
 		goto out_release_info;
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 8004f67..8087297 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2141,6 +2141,8 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 	bridge->dev.parent = parent;
 	bridge->dev.release = pci_release_host_bridge_dev;
 	dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
+	if (parent)
+		ACPI_COMPANION_SET(&bridge->dev, ACPI_COMPANION(parent));
 	error = pcibios_root_bridge_prepare(bridge);
 	if (error) {
 		kfree(bridge);
-- 
1.9.1

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

#1388193 — Re: [PATCH V6 01/13] pci, acpi, x86, ia64: Move ACPI host bridge device companion assignment to core code.

On Sat, Apr 16, 2016 at 04:41:45AM +0800, kbuild test robot wrote:
> Hi Tomasz,
> 
> [auto build test ERROR on pci/next]
> [also build test ERROR on v4.6-rc3 next-20160415]
> [if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Tomasz-Nowicki/Support-for-generic-ACPI-based-PCI-host-controller/20160416-011935
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
> config: ia64-allmodconfig (attached as .config)
> reproduce:
>         wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         make.cross ARCH=ia64 
> 
> All errors (new ones prefixed by >>):
> 
>    drivers/pci/hotplug/sgi_hotplug.c: In function 'enable_slot':
> >> drivers/pci/hotplug/sgi_hotplug.c:412:61: error: 'struct pci_controller' has no member named 'companion'
>       phandle = acpi_device_handle(PCI_CONTROLLER(slot->pci_bus)->companion);
>                                                                 ^
>    drivers/pci/hotplug/sgi_hotplug.c: In function 'disable_slot':
>    drivers/pci/hotplug/sgi_hotplug.c:491:32: error: 'struct pci_controller' has no member named 'companion'
>       PCI_CONTROLLER(slot->pci_bus)->companion) {
>                                    ^
>    drivers/pci/hotplug/sgi_hotplug.c:500:61: error: 'struct pci_controller' has no member named 'companion'
>       phandle = acpi_device_handle(PCI_CONTROLLER(slot->pci_bus)->companion);
>                                                                 ^

I assume somebody is fixing this?

> 
> vim +412 drivers/pci/hotplug/sgi_hotplug.c
> 
> 3e643e77 John Keller       2007-01-30  406  		struct acpi_device *pdevice;
> 3e643e77 John Keller       2007-01-30  407  		acpi_handle phandle;
> 3e643e77 John Keller       2007-01-30  408  		acpi_handle chandle = NULL;
> 3e643e77 John Keller       2007-01-30  409  		acpi_handle rethandle;
> 3e643e77 John Keller       2007-01-30  410  		acpi_status ret;
> 3e643e77 John Keller       2007-01-30  411  
> 7b199811 Rafael J. Wysocki 2013-11-11 @412  		phandle = acpi_device_handle(PCI_CONTROLLER(slot->pci_bus)->companion);
> 3e643e77 John Keller       2007-01-30  413  
> 3e643e77 John Keller       2007-01-30  414  		if (acpi_bus_get_device(phandle, &pdevice)) {
> 227f0647 Ryan Desfosses    2014-04-18  415  			dev_dbg(&slot->pci_bus->self->dev, "no parent device, assuming NULL\n");
> 
> :::::: The code at line 412 was first introduced by commit
> :::::: 7b1998116bbb2f3e5dd6cb9a8ee6db479b0b50a9 ACPI / driver core: Store an ACPI device pointer in struct acpi_dev_node
> 
> :::::: TO: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> :::::: CC: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

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

#1388873 — Re: [PATCH V6 01/13] pci, acpi, x86, ia64: Move ACPI host bridge device companion assignment to core code.

On 27.04.2016 00:36, Bjorn Helgaas wrote:
> On Sat, Apr 16, 2016 at 04:41:45AM +0800, kbuild test robot wrote:
>> Hi Tomasz,
>>
>> [auto build test ERROR on pci/next]
>> [also build test ERROR on v4.6-rc3 next-20160415]
>> [if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
>>
>> url:    https://github.com/0day-ci/linux/commits/Tomasz-Nowicki/Support-for-generic-ACPI-based-PCI-host-controller/20160416-011935
>> base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
>> config: ia64-allmodconfig (attached as .config)
>> reproduce:
>>          wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
>>          chmod +x ~/bin/make.cross
>>          # save the attached .config to linux build tree
>>          make.cross ARCH=ia64
>>
>> All errors (new ones prefixed by >>):
>>
>>     drivers/pci/hotplug/sgi_hotplug.c: In function 'enable_slot':
>>>> drivers/pci/hotplug/sgi_hotplug.c:412:61: error: 'struct pci_controller' has no member named 'companion'
>>        phandle = acpi_device_handle(PCI_CONTROLLER(slot->pci_bus)->companion);
>>                                                                  ^
>>     drivers/pci/hotplug/sgi_hotplug.c: In function 'disable_slot':
>>     drivers/pci/hotplug/sgi_hotplug.c:491:32: error: 'struct pci_controller' has no member named 'companion'
>>        PCI_CONTROLLER(slot->pci_bus)->companion) {
>>                                     ^
>>     drivers/pci/hotplug/sgi_hotplug.c:500:61: error: 'struct pci_controller' has no member named 'companion'
>>        phandle = acpi_device_handle(PCI_CONTROLLER(slot->pci_bus)->companion);
>>                                                                  ^
>
> I assume somebody is fixing this?

Yes, it will be fixed in next version also I will ask Hanjun to re-test 
it on IA64 as in the past.

Tomasz

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

#1388606 — Re: [PATCH V6 01/13] pci, acpi, x86, ia64: Move ACPI host bridge device companion assignment to core code.

[question for Rafael below]

On Fri, Apr 15, 2016 at 07:06:36PM +0200, Tomasz Nowicki wrote:
> Currently we have two platforms (x86 & ia64) capable of PCI ACPI host
> bridge initialization. They both use arch-specific sysdata to pass down
> parent device reference and both rely on NULL parent in pci_create_root_bus()
> to validate sysdata content.
> 
> It looks hacky and prevents us from getting some firmware specific
> info for PCI host controller based on its acpi_device structure
> in generic pci_create_root_bus() function. However, we overcome that
> blocker by passing down parent device via pci_create_root_bus parameter
> (as the ACPI device type). Then we use ACPI_COMPANION_SET in core code
> for ACPI boot method only. ACPI_COMPANION_SET is safe to run for all
> cases DT, ACPI and DT&ACPI.
> 
> Since now PCI core code is setting ACPI companion device for us,
> x86 & ia64 specific ACPI companion device setting turns out to be dead now.
> We can get rid of it, including related companion reference from
> PCI sysdata structure. Aslo, PCI_CONTROLLER macro cannot return valid
> companion device anymore. Therefore we need to convert its usage to
> ACPI_COMPANION.
> 
> Suggested-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Tested-by: Duc Dang <dhdang@apm.com>
> Tested-by: Dongdong Liu <liudongdong3@huawei.com>
> Tested-by: Hanjun Guo <hanjun.guo@linaro.org>
> Tested-by: Graeme Gregory <graeme.gregory@linaro.org>
> Tested-by: Sinan Kaya <okaya@codeaurora.org>
> ---
>  arch/ia64/hp/common/sba_iommu.c    |  2 +-
>  arch/ia64/include/asm/pci.h        |  1 -
>  arch/ia64/pci/pci.c                | 16 ----------------
>  arch/ia64/sn/kernel/io_acpi_init.c |  4 ++--
>  arch/x86/include/asm/pci.h         |  3 ---
>  arch/x86/pci/acpi.c                | 17 -----------------
>  drivers/acpi/pci_root.c            |  7 ++++++-
>  drivers/pci/probe.c                |  2 ++
>  8 files changed, 11 insertions(+), 41 deletions(-)
> 
> diff --git a/arch/ia64/hp/common/sba_iommu.c b/arch/ia64/hp/common/sba_iommu.c
> index a6d6190..78e4444 100644
> --- a/arch/ia64/hp/common/sba_iommu.c
> +++ b/arch/ia64/hp/common/sba_iommu.c
> @@ -1981,7 +1981,7 @@ sba_connect_bus(struct pci_bus *bus)
>  	if (PCI_CONTROLLER(bus)->iommu)
>  		return;
>  
> -	handle = acpi_device_handle(PCI_CONTROLLER(bus)->companion);
> +	handle = acpi_device_handle(ACPI_COMPANION(bus->bridge));
>  	if (!handle)
>  		return;
>  
> diff --git a/arch/ia64/include/asm/pci.h b/arch/ia64/include/asm/pci.h
> index c0835b0..12423f4 100644
> --- a/arch/ia64/include/asm/pci.h
> +++ b/arch/ia64/include/asm/pci.h
> @@ -63,7 +63,6 @@ extern int pci_mmap_legacy_page_range(struct pci_bus *bus,
>  #define pci_legacy_write platform_pci_legacy_write
>  
>  struct pci_controller {
> -	struct acpi_device *companion;
>  	void *iommu;
>  	int segment;
>  	int node;		/* nearest node with memory or NUMA_NO_NODE for global allocation */
> diff --git a/arch/ia64/pci/pci.c b/arch/ia64/pci/pci.c
> index 8f6ac2f..978d6af 100644
> --- a/arch/ia64/pci/pci.c
> +++ b/arch/ia64/pci/pci.c
> @@ -301,28 +301,12 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
>  	}
>  
>  	info->controller.segment = root->segment;
> -	info->controller.companion = device;
>  	info->controller.node = acpi_get_node(device->handle);
>  	INIT_LIST_HEAD(&info->io_resources);
>  	return acpi_pci_root_create(root, &pci_acpi_root_ops,
>  				    &info->common, &info->controller);
>  }
>  
> -int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
> -{
> -	/*
> -	 * We pass NULL as parent to pci_create_root_bus(), so if it is not NULL
> -	 * here, pci_create_root_bus() has been called by someone else and
> -	 * sysdata is likely to be different from what we expect.  Let it go in
> -	 * that case.
> -	 */
> -	if (!bridge->dev.parent) {
> -		struct pci_controller *controller = bridge->bus->sysdata;
> -		ACPI_COMPANION_SET(&bridge->dev, controller->companion);
> -	}
> -	return 0;
> -}
> -
>  void pcibios_fixup_device_resources(struct pci_dev *dev)
>  {
>  	int idx;
> diff --git a/arch/ia64/sn/kernel/io_acpi_init.c b/arch/ia64/sn/kernel/io_acpi_init.c
> index 231234c..e454492 100644
> --- a/arch/ia64/sn/kernel/io_acpi_init.c
> +++ b/arch/ia64/sn/kernel/io_acpi_init.c
> @@ -132,7 +132,7 @@ sn_get_bussoft_ptr(struct pci_bus *bus)
>  	struct acpi_resource_vendor_typed *vendor;
>  
>  
> -	handle = acpi_device_handle(PCI_CONTROLLER(bus)->companion);
> +	handle = acpi_device_handle(ACPI_COMPANION(bus->bridge));
>  	status = acpi_get_vendor_resource(handle, METHOD_NAME__CRS,
>  					  &sn_uuid, &buffer);
>  	if (ACPI_FAILURE(status)) {
> @@ -360,7 +360,7 @@ sn_acpi_get_pcidev_info(struct pci_dev *dev, struct pcidev_info **pcidev_info,
>  	acpi_status status;
>  	struct acpi_buffer name_buffer = { ACPI_ALLOCATE_BUFFER, NULL };
>  
> -	rootbus_handle = acpi_device_handle(PCI_CONTROLLER(dev)->companion);
> +	rootbus_handle = acpi_device_handle(ACPI_COMPANION(dev->bus->bridge));
>          status = acpi_evaluate_integer(rootbus_handle, METHOD_NAME__SEG, NULL,
>                                         &segment);
>          if (ACPI_SUCCESS(status)) {
> diff --git a/arch/x86/include/asm/pci.h b/arch/x86/include/asm/pci.h
> index 9ab7507..24de07d 100644
> --- a/arch/x86/include/asm/pci.h
> +++ b/arch/x86/include/asm/pci.h
> @@ -14,9 +14,6 @@
>  struct pci_sysdata {
>  	int		domain;		/* PCI domain */
>  	int		node;		/* NUMA node */
> -#ifdef CONFIG_ACPI
> -	struct acpi_device *companion;	/* ACPI companion device */
> -#endif
>  #ifdef CONFIG_X86_64
>  	void		*iommu;		/* IOMMU private data */
>  #endif
> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
> index 3cd6983..f4ca17a 100644
> --- a/arch/x86/pci/acpi.c
> +++ b/arch/x86/pci/acpi.c
> @@ -340,7 +340,6 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
>  		struct pci_sysdata sd = {
>  			.domain = domain,
>  			.node = node,
> -			.companion = root->device
>  		};
>  
>  		memcpy(bus->sysdata, &sd, sizeof(sd));
> @@ -355,7 +354,6 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
>  		else {
>  			info->sd.domain = domain;
>  			info->sd.node = node;
> -			info->sd.companion = root->device;
>  			bus = acpi_pci_root_create(root, &acpi_pci_root_ops,
>  						   &info->common, &info->sd);
>  		}
> @@ -373,21 +371,6 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
>  	return bus;
>  }
>  
> -int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
> -{
> -	/*
> -	 * We pass NULL as parent to pci_create_root_bus(), so if it is not NULL
> -	 * here, pci_create_root_bus() has been called by someone else and
> -	 * sysdata is likely to be different from what we expect.  Let it go in
> -	 * that case.
> -	 */
> -	if (!bridge->dev.parent) {
> -		struct pci_sysdata *sd = bridge->bus->sysdata;
> -		ACPI_COMPANION_SET(&bridge->dev, sd->companion);
> -	}
> -	return 0;
> -}
> -
>  int __init pci_acpi_init(void)
>  {
>  	struct pci_dev *dev = NULL;
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index ae3fe4e..4581e0e 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -564,6 +564,11 @@ static int acpi_pci_root_add(struct acpi_device *device,
>  		}
>  	}
>  
> +	/*
> +	 * pci_create_root_bus() needs to detect the parent device type,
> +	 * so initialize its companion data accordingly.
> +	 */
> +	ACPI_COMPANION_SET(&device->dev, device);
>  	root->device = device;
>  	root->segment = segment & 0xFFFF;
>  	strcpy(acpi_device_name(device), ACPI_PCI_ROOT_DEVICE_NAME);
> @@ -846,7 +851,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>  
>  	pci_acpi_root_add_resources(info);
>  	pci_add_resource(&info->resources, &root->secondary);
> -	bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,
> +	bus = pci_create_root_bus(&device->dev, busnum, ops->pci_ops,
>  				  sysdata, &info->resources);

"device" here is a struct acpi_device *.  Rafael, is that the right
thing to do?  I dimly recall proposing something similar long ago and
that it turned out to be a bad idea.

>  	if (!bus)
>  		goto out_release_info;
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 8004f67..8087297 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2141,6 +2141,8 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>  	bridge->dev.parent = parent;
>  	bridge->dev.release = pci_release_host_bridge_dev;
>  	dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
> +	if (parent)
> +		ACPI_COMPANION_SET(&bridge->dev, ACPI_COMPANION(parent));
>  	error = pcibios_root_bridge_prepare(bridge);
>  	if (error) {
>  		kfree(bridge);
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

#1380049 — [PATCH V6 03/13] x86, ia64: Include acpi_pci_{add|remove}_bus to the default pcibios_{add|remove}_bus implementation.

x86 and ia64 are the only arches that implement pcibios_{add|remove}_bus hooks
and implement them in the same way. Moreover ARM64 is going to do the same.
So it seems that acpi_pci_{add|remove}_bus is generic enough to be default
option for pcibios_{add|remove}_bus hooks. Also, it is always safe to run
acpi_pci_{add|remove}_bus as they have empty stubs for !ACPI case and
return if ACPI has been switched off in run time.

After all we can remove x86 and ia64 pcibios_{add|remove}_bus
implementation.

Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Tested-by: Duc Dang <dhdang@apm.com>
Tested-by: Dongdong Liu <liudongdong3@huawei.com>
Tested-by: Hanjun Guo <hanjun.guo@linaro.org>
Tested-by: Graeme Gregory <graeme.gregory@linaro.org>
Tested-by: Sinan Kaya <okaya@codeaurora.org>
---
 arch/ia64/pci/pci.c   | 10 ----------
 arch/x86/pci/common.c | 10 ----------
 drivers/pci/probe.c   |  3 +++
 3 files changed, 3 insertions(+), 20 deletions(-)

diff --git a/arch/ia64/pci/pci.c b/arch/ia64/pci/pci.c
index 978d6af..be4c9ef 100644
--- a/arch/ia64/pci/pci.c
+++ b/arch/ia64/pci/pci.c
@@ -358,16 +358,6 @@ void pcibios_fixup_bus(struct pci_bus *b)
 	platform_pci_fixup_bus(b);
 }
 
-void pcibios_add_bus(struct pci_bus *bus)
-{
-	acpi_pci_add_bus(bus);
-}
-
-void pcibios_remove_bus(struct pci_bus *bus)
-{
-	acpi_pci_remove_bus(bus);
-}
-
 void pcibios_set_master (struct pci_dev *dev)
 {
 	/* No special bus mastering setup handling */
diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
index 381a43c..7763a84 100644
--- a/arch/x86/pci/common.c
+++ b/arch/x86/pci/common.c
@@ -170,16 +170,6 @@ void pcibios_fixup_bus(struct pci_bus *b)
 		pcibios_fixup_device_resources(dev);
 }
 
-void pcibios_add_bus(struct pci_bus *bus)
-{
-	acpi_pci_add_bus(bus);
-}
-
-void pcibios_remove_bus(struct pci_bus *bus)
-{
-	acpi_pci_remove_bus(bus);
-}
-
 /*
  * Only use DMI information to set this if nothing was passed
  * on the kernel command line (which was parsed earlier).
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 8087297..ef569e8 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -12,6 +12,7 @@
 #include <linux/slab.h>
 #include <linux/module.h>
 #include <linux/cpumask.h>
+#include <linux/pci-acpi.h>
 #include <linux/pci-aspm.h>
 #include <linux/aer.h>
 #include <linux/acpi.h>
@@ -2101,10 +2102,12 @@ int __weak pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
 
 void __weak pcibios_add_bus(struct pci_bus *bus)
 {
+	acpi_pci_add_bus(bus);
 }
 
 void __weak pcibios_remove_bus(struct pci_bus *bus)
 {
+	acpi_pci_remove_bus(bus);
 }
 
 struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
-- 
1.9.1

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

#1388603 — Re: [PATCH V6 03/13] x86, ia64: Include acpi_pci_{add|remove}_bus to the default pcibios_{add|remove}_bus implementation.

On Fri, Apr 15, 2016 at 07:06:38PM +0200, Tomasz Nowicki wrote:
> x86 and ia64 are the only arches that implement pcibios_{add|remove}_bus hooks
> and implement them in the same way. Moreover ARM64 is going to do the same.
> So it seems that acpi_pci_{add|remove}_bus is generic enough to be default
> option for pcibios_{add|remove}_bus hooks. Also, it is always safe to run
> acpi_pci_{add|remove}_bus as they have empty stubs for !ACPI case and
> return if ACPI has been switched off in run time.
> 
> After all we can remove x86 and ia64 pcibios_{add|remove}_bus
> implementation.
> 
> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Tested-by: Duc Dang <dhdang@apm.com>
> Tested-by: Dongdong Liu <liudongdong3@huawei.com>
> Tested-by: Hanjun Guo <hanjun.guo@linaro.org>
> Tested-by: Graeme Gregory <graeme.gregory@linaro.org>
> Tested-by: Sinan Kaya <okaya@codeaurora.org>
> ---
>  arch/ia64/pci/pci.c   | 10 ----------
>  arch/x86/pci/common.c | 10 ----------
>  drivers/pci/probe.c   |  3 +++
>  3 files changed, 3 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/ia64/pci/pci.c b/arch/ia64/pci/pci.c
> index 978d6af..be4c9ef 100644
> --- a/arch/ia64/pci/pci.c
> +++ b/arch/ia64/pci/pci.c
> @@ -358,16 +358,6 @@ void pcibios_fixup_bus(struct pci_bus *b)
>  	platform_pci_fixup_bus(b);
>  }
>  
> -void pcibios_add_bus(struct pci_bus *bus)
> -{
> -	acpi_pci_add_bus(bus);
> -}
> -
> -void pcibios_remove_bus(struct pci_bus *bus)
> -{
> -	acpi_pci_remove_bus(bus);
> -}
> -
>  void pcibios_set_master (struct pci_dev *dev)
>  {
>  	/* No special bus mastering setup handling */
> diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
> index 381a43c..7763a84 100644
> --- a/arch/x86/pci/common.c
> +++ b/arch/x86/pci/common.c
> @@ -170,16 +170,6 @@ void pcibios_fixup_bus(struct pci_bus *b)
>  		pcibios_fixup_device_resources(dev);
>  }
>  
> -void pcibios_add_bus(struct pci_bus *bus)
> -{
> -	acpi_pci_add_bus(bus);
> -}
> -
> -void pcibios_remove_bus(struct pci_bus *bus)
> -{
> -	acpi_pci_remove_bus(bus);
> -}
> -
>  /*
>   * Only use DMI information to set this if nothing was passed
>   * on the kernel command line (which was parsed earlier).
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 8087297..ef569e8 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -12,6 +12,7 @@
>  #include <linux/slab.h>
>  #include <linux/module.h>
>  #include <linux/cpumask.h>
> +#include <linux/pci-acpi.h>
>  #include <linux/pci-aspm.h>
>  #include <linux/aer.h>
>  #include <linux/acpi.h>
> @@ -2101,10 +2102,12 @@ int __weak pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
>  
>  void __weak pcibios_add_bus(struct pci_bus *bus)
>  {
> +	acpi_pci_add_bus(bus);
>  }
>  
>  void __weak pcibios_remove_bus(struct pci_bus *bus)
>  {
> +	acpi_pci_remove_bus(bus);
>  }

Is this buying us something more than just getting rid of these
pcibios functions in the arches?  The arch-specific pcibios methods
by themselves don't seem too onerous, and I don't really want to add
#includes and calls to every firmware interface under the sun.

I admit it's a net removal of 17 lines, but I'm not sure it's a net
reduction in complexity for the reader, who now has to remember that
this ACPI stuff is a no-op on most arches.

As a tangent, some of the stuff in acpi_pci_add_bus() really belongs
elsewhere anyway.  For example, the _DSM stuff should probably be in
acpi_pci_root_create() since it's a one-per-host bridge kind of thing.

>  struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
> -- 
> 1.9.1
> 

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

#1389045 — Re: [PATCH V6 03/13] x86, ia64: Include acpi_pci_{add|remove}_bus to the default pcibios_{add|remove}_bus implementation.

On 27.04.2016 04:34, Bjorn Helgaas wrote:
> On Fri, Apr 15, 2016 at 07:06:38PM +0200, Tomasz Nowicki wrote:
>> x86 and ia64 are the only arches that implement pcibios_{add|remove}_bus hooks
>> and implement them in the same way. Moreover ARM64 is going to do the same.
>> So it seems that acpi_pci_{add|remove}_bus is generic enough to be default
>> option for pcibios_{add|remove}_bus hooks. Also, it is always safe to run
>> acpi_pci_{add|remove}_bus as they have empty stubs for !ACPI case and
>> return if ACPI has been switched off in run time.
>>
>> After all we can remove x86 and ia64 pcibios_{add|remove}_bus
>> implementation.
>>
>> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
>> Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> Tested-by: Duc Dang <dhdang@apm.com>
>> Tested-by: Dongdong Liu <liudongdong3@huawei.com>
>> Tested-by: Hanjun Guo <hanjun.guo@linaro.org>
>> Tested-by: Graeme Gregory <graeme.gregory@linaro.org>
>> Tested-by: Sinan Kaya <okaya@codeaurora.org>
>> ---
>>   arch/ia64/pci/pci.c   | 10 ----------
>>   arch/x86/pci/common.c | 10 ----------
>>   drivers/pci/probe.c   |  3 +++
>>   3 files changed, 3 insertions(+), 20 deletions(-)
>>
>> diff --git a/arch/ia64/pci/pci.c b/arch/ia64/pci/pci.c
>> index 978d6af..be4c9ef 100644
>> --- a/arch/ia64/pci/pci.c
>> +++ b/arch/ia64/pci/pci.c
>> @@ -358,16 +358,6 @@ void pcibios_fixup_bus(struct pci_bus *b)
>>   	platform_pci_fixup_bus(b);
>>   }
>>
>> -void pcibios_add_bus(struct pci_bus *bus)
>> -{
>> -	acpi_pci_add_bus(bus);
>> -}
>> -
>> -void pcibios_remove_bus(struct pci_bus *bus)
>> -{
>> -	acpi_pci_remove_bus(bus);
>> -}
>> -
>>   void pcibios_set_master (struct pci_dev *dev)
>>   {
>>   	/* No special bus mastering setup handling */
>> diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
>> index 381a43c..7763a84 100644
>> --- a/arch/x86/pci/common.c
>> +++ b/arch/x86/pci/common.c
>> @@ -170,16 +170,6 @@ void pcibios_fixup_bus(struct pci_bus *b)
>>   		pcibios_fixup_device_resources(dev);
>>   }
>>
>> -void pcibios_add_bus(struct pci_bus *bus)
>> -{
>> -	acpi_pci_add_bus(bus);
>> -}
>> -
>> -void pcibios_remove_bus(struct pci_bus *bus)
>> -{
>> -	acpi_pci_remove_bus(bus);
>> -}
>> -
>>   /*
>>    * Only use DMI information to set this if nothing was passed
>>    * on the kernel command line (which was parsed earlier).
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index 8087297..ef569e8 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -12,6 +12,7 @@
>>   #include <linux/slab.h>
>>   #include <linux/module.h>
>>   #include <linux/cpumask.h>
>> +#include <linux/pci-acpi.h>
>>   #include <linux/pci-aspm.h>
>>   #include <linux/aer.h>
>>   #include <linux/acpi.h>
>> @@ -2101,10 +2102,12 @@ int __weak pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
>>
>>   void __weak pcibios_add_bus(struct pci_bus *bus)
>>   {
>> +	acpi_pci_add_bus(bus);
>>   }
>>
>>   void __weak pcibios_remove_bus(struct pci_bus *bus)
>>   {
>> +	acpi_pci_remove_bus(bus);
>>   }
>
> Is this buying us something more than just getting rid of these
> pcibios functions in the arches?  The arch-specific pcibios methods
> by themselves don't seem too onerous, and I don't really want to add
> #includes and calls to every firmware interface under the sun.
>
> I admit it's a net removal of 17 lines, but I'm not sure it's a net
> reduction in complexity for the reader, who now has to remember that
> this ACPI stuff is a no-op on most arches.
>
> As a tangent, some of the stuff in acpi_pci_add_bus() really belongs
> elsewhere anyway.  For example, the _DSM stuff should probably be in
> acpi_pci_root_create() since it's a one-per-host bridge kind of thing.
>

OK, I will add pcibios_add_bus to ARM64 arch code and call 
acpi_pci_remove_bus(bus) from there.

Thanks,
Tomasz

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

#1380050 — [PATCH V6 05/13] acpi, pci: Support IO resources when parsing PCI host bridge resources.

Platforms that have memory mapped IO port (such as ARM64) need special
handling for PCI I/O resources. For host bridge's resource probing case
these resources need to be fixed up with pci_register_io_range/pci_remap_iospace etc.

Furthermore, the same I/O resources need to be released after hotplug
removal so that it can be re-added back by the pci_remap_iospace
function during insertion. Therefore we implement new pci_unmap_iospace call
which unmaps I/O space as the symmetry to pci_remap_iospace.

Signed-off-by: Jayachandran C <jchandra@broadcom.com>
Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
---
 drivers/acpi/pci_root.c | 33 +++++++++++++++++++++++++++++++++
 drivers/pci/pci.c       | 24 ++++++++++++++++++++++++
 include/linux/pci.h     |  1 +
 3 files changed, 58 insertions(+)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index d9a70c4..815b6ca 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -742,6 +742,34 @@ next:
 			resource_list_add_tail(entry, resources);
 	}
 }
+static void acpi_pci_root_remap_iospace(struct resource_entry *entry)
+{
+#ifdef PCI_IOBASE
+	struct resource *res = entry->res;
+	resource_size_t cpu_addr = res->start;
+	resource_size_t pci_addr = cpu_addr - entry->offset;
+	resource_size_t length = resource_size(res);
+	unsigned long port;
+
+	if (pci_register_io_range(cpu_addr, length))
+		goto err;
+
+	port = pci_address_to_pio(cpu_addr);
+	if (port == (unsigned long)-1)
+		goto err;
+
+	res->start = port;
+	res->end = port + length - 1;
+	entry->offset = port - pci_addr;
+
+	if (pci_remap_iospace(res, cpu_addr) < 0)
+		goto err;
+	pr_info("Remapped I/O %pa to %pR\n", &cpu_addr, res);
+	return;
+err:
+	res->flags |= IORESOURCE_DISABLED;
+#endif
+}
 
 int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info)
 {
@@ -763,6 +791,9 @@ int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info)
 			"no IO and memory resources present in _CRS\n");
 	else {
 		resource_list_for_each_entry_safe(entry, tmp, list) {
+			if (entry->res->flags & IORESOURCE_IO)
+				acpi_pci_root_remap_iospace(entry);
+
 			if (entry->res->flags & IORESOURCE_DISABLED)
 				resource_list_destroy_entry(entry);
 			else
@@ -834,6 +865,8 @@ static void acpi_pci_root_release_info(struct pci_host_bridge *bridge)
 
 	resource_list_for_each_entry(entry, &bridge->windows) {
 		res = entry->res;
+		if (res->flags & IORESOURCE_IO)
+			pci_unmap_iospace(res);
 		if (res->parent &&
 		    (res->flags & (IORESOURCE_MEM | IORESOURCE_IO)))
 			release_resource(res);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 89e9996..c0f8a4e 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -26,6 +26,7 @@
 #include <linux/device.h>
 #include <linux/pm_runtime.h>
 #include <linux/pci_hotplug.h>
+#include <linux/vmalloc.h>
 #include <asm/setup.h>
 #include <linux/aer.h>
 #include "pci.h"
@@ -3168,6 +3169,29 @@ int __weak pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
 #endif
 }
 
+/**
+ *	pci_unmap_iospace - Unmap the memory mapped I/O space
+ *	@res: resource to be unmapped
+ *
+ *	Unmap the CPU virtual address @res from virtual address space.
+ *	Only architectures that have memory mapped IO functions defined
+ *	(and the PCI_IOBASE value defined) should call this function.
+ */
+void  pci_unmap_iospace(struct resource *res)
+{
+#if defined(PCI_IOBASE) && defined(CONFIG_MMU)
+	unsigned long vaddr = (unsigned long)PCI_IOBASE + res->start;
+
+	unmap_kernel_range(vaddr, resource_size(res));
+#else
+	/*
+	 * This architecture does not have memory mapped I/O space,
+	 * so this function should never be called.
+	 */
+	WARN_ONCE(1, "This architecture does not support memory mapped I/O\n");
+#endif
+}
+
 static void __pci_set_master(struct pci_dev *dev, bool enable)
 {
 	u16 old_cmd, cmd;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index c28adb4..df1f33d 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1168,6 +1168,7 @@ 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);
+void pci_unmap_iospace(struct resource *res);
 
 static inline pci_bus_addr_t pci_bus_address(struct pci_dev *pdev, int bar)
 {
-- 
1.9.1

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

#1388604 — Re: [PATCH V6 05/13] acpi, pci: Support IO resources when parsing PCI host bridge resources.

On Fri, Apr 15, 2016 at 07:06:40PM +0200, Tomasz Nowicki wrote:
> Platforms that have memory mapped IO port (such as ARM64) need special
> handling for PCI I/O resources. For host bridge's resource probing case
> these resources need to be fixed up with pci_register_io_range/pci_remap_iospace etc.

ia64 also has memory-mapped I/O port space.  It would be ideal to find
some way to handle ia64 and ARM64 similarly.  At the very least, we
have to make sure that this doesn't break ia64.  The ia64 dense/sparse
I/O spaces complicate things; I don't know if ARM64 has something
similar or not.

> Furthermore, the same I/O resources need to be released after hotplug
> removal so that it can be re-added back by the pci_remap_iospace
> function during insertion. Therefore we implement new pci_unmap_iospace call
> which unmaps I/O space as the symmetry to pci_remap_iospace.

"Furthermore" is a hint that we should check to see if this can be
split into two patches.

We already have a pci_remap_iospace(), and you're adding
pci_unmap_iospace(), which will be used for hotplug removal.  So let's 
add pci_unmap_iospace() first in a patch by itself because that's
potentially useful for other callers of pci_remap_iospace(), even if
they don't need the acpi_pci_root_remap_iospace() stuff.

> Signed-off-by: Jayachandran C <jchandra@broadcom.com>
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> ---
>  drivers/acpi/pci_root.c | 33 +++++++++++++++++++++++++++++++++
>  drivers/pci/pci.c       | 24 ++++++++++++++++++++++++
>  include/linux/pci.h     |  1 +
>  3 files changed, 58 insertions(+)
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index d9a70c4..815b6ca 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -742,6 +742,34 @@ next:
>  			resource_list_add_tail(entry, resources);
>  	}
>  }
> +static void acpi_pci_root_remap_iospace(struct resource_entry *entry)
> +{
> +#ifdef PCI_IOBASE
> +	struct resource *res = entry->res;
> +	resource_size_t cpu_addr = res->start;
> +	resource_size_t pci_addr = cpu_addr - entry->offset;
> +	resource_size_t length = resource_size(res);
> +	unsigned long port;
> +
> +	if (pci_register_io_range(cpu_addr, length))
> +		goto err;
> +
> +	port = pci_address_to_pio(cpu_addr);
> +	if (port == (unsigned long)-1)
> +		goto err;
> +
> +	res->start = port;
> +	res->end = port + length - 1;
> +	entry->offset = port - pci_addr;
> +
> +	if (pci_remap_iospace(res, cpu_addr) < 0)
> +		goto err;
> +	pr_info("Remapped I/O %pa to %pR\n", &cpu_addr, res);
> +	return;
> +err:
> +	res->flags |= IORESOURCE_DISABLED;
> +#endif
> +}
>  
>  int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info)
>  {
> @@ -763,6 +791,9 @@ int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info)
>  			"no IO and memory resources present in _CRS\n");
>  	else {
>  		resource_list_for_each_entry_safe(entry, tmp, list) {
> +			if (entry->res->flags & IORESOURCE_IO)
> +				acpi_pci_root_remap_iospace(entry);
> +
>  			if (entry->res->flags & IORESOURCE_DISABLED)
>  				resource_list_destroy_entry(entry);
>  			else
> @@ -834,6 +865,8 @@ static void acpi_pci_root_release_info(struct pci_host_bridge *bridge)
>  
>  	resource_list_for_each_entry(entry, &bridge->windows) {
>  		res = entry->res;
> +		if (res->flags & IORESOURCE_IO)
> +			pci_unmap_iospace(res);
>  		if (res->parent &&
>  		    (res->flags & (IORESOURCE_MEM | IORESOURCE_IO)))
>  			release_resource(res);
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 89e9996..c0f8a4e 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -26,6 +26,7 @@
>  #include <linux/device.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pci_hotplug.h>
> +#include <linux/vmalloc.h>
>  #include <asm/setup.h>
>  #include <linux/aer.h>
>  #include "pci.h"
> @@ -3168,6 +3169,29 @@ int __weak pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
>  #endif
>  }
>  
> +/**
> + *	pci_unmap_iospace - Unmap the memory mapped I/O space
> + *	@res: resource to be unmapped
> + *
> + *	Unmap the CPU virtual address @res from virtual address space.
> + *	Only architectures that have memory mapped IO functions defined
> + *	(and the PCI_IOBASE value defined) should call this function.
> + */
> +void  pci_unmap_iospace(struct resource *res)
> +{
> +#if defined(PCI_IOBASE) && defined(CONFIG_MMU)
> +	unsigned long vaddr = (unsigned long)PCI_IOBASE + res->start;
> +
> +	unmap_kernel_range(vaddr, resource_size(res));
> +#else
> +	/*
> +	 * This architecture does not have memory mapped I/O space,
> +	 * so this function should never be called.
> +	 */
> +	WARN_ONCE(1, "This architecture does not support memory mapped I/O\n");
> +#endif
> +}
> +
>  static void __pci_set_master(struct pci_dev *dev, bool enable)
>  {
>  	u16 old_cmd, cmd;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index c28adb4..df1f33d 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1168,6 +1168,7 @@ 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);
> +void pci_unmap_iospace(struct resource *res);
>  
>  static inline pci_bus_addr_t pci_bus_address(struct pci_dev *pdev, int bar)
>  {
> -- 
> 1.9.1
> 

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

#1388676 — Re: [PATCH V6 05/13] acpi, pci: Support IO resources when parsing PCI host bridge resources.

On 04/26/2016 10:39 PM, Bjorn Helgaas wrote:
> On Fri, Apr 15, 2016 at 07:06:40PM +0200, Tomasz Nowicki wrote:
>> Platforms that have memory mapped IO port (such as ARM64) need special
>> handling for PCI I/O resources. For host bridge's resource probing case
>> these resources need to be fixed up with pci_register_io_range/pci_remap_iospace etc.
> 
> ia64 also has memory-mapped I/O port space.

The specific references of interest to anyone here are:

*). Volume 2, Part 1: Itanium® Architecture-based Operating System
Interaction Model with IA-32 Applications 2:267 section "10.7 I/O Port
Space Model" which describes how they can map 4 "legacy" IO ports on a
virtual page when operating in a "sparse" mode.

*). Page 378 of the ACPI6.1 specification Table 6-213 I/O Resource Flag
(Resource Type = 1) Definitions describes how a "sparse" translation can
exist depending upon bit _TRS. This seems to be implemented in Linux
using the ACPI_SPARSE_TRANSLATION types.

> It would be ideal to find
> some way to handle ia64 and ARM64 similarly.  At the very least, we
> have to make sure that this doesn't break ia64.  The ia64 dense/sparse
> I/O spaces complicate things; I don't know if ARM64 has something
> similar or not.

There's nothing directly similar - it's just regular MMIO.

Jon.

-- 
Computer Architect | Sent from my Fedora powered laptop

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

#1389124 — Re: [PATCH V6 05/13] acpi, pci: Support IO resources when parsing PCI host bridge resources.

On Tue, Apr 26, 2016 at 09:39:16PM -0500, Bjorn Helgaas wrote:
> On Fri, Apr 15, 2016 at 07:06:40PM +0200, Tomasz Nowicki wrote:
> > Platforms that have memory mapped IO port (such as ARM64) need special
> > handling for PCI I/O resources. For host bridge's resource probing case
> > these resources need to be fixed up with pci_register_io_range/pci_remap_iospace etc.
> 
> ia64 also has memory-mapped I/O port space.  It would be ideal to find
> some way to handle ia64 and ARM64 similarly.  At the very least, we
> have to make sure that this doesn't break ia64.  The ia64 dense/sparse
> I/O spaces complicate things; I don't know if ARM64 has something
> similar or not.

No it does not, and that's exactly the same problem we faced with
the DT generic version of of_pci_range_to_resource() which basically
relies on PCI_IOBASE to be defined to add code that creates IO port
resources out of the MMIO resource describing how IO port space is
mapped to MMIO (physical) address space.

IIRC everything hinges on PCI_IOBASE definition to make sure that
of_pci_range_to_resource() *works*, which means that if PCI_IOBASE is
not defined (ie IA64) that code - acpi_pci_root_remap_iospace() in this
case - does nothing.

So acpi_pci_root_remap_iospace() is of_pci_range_to_resource() ACPI
equivalent + the pci_remap_iospace() call (I have to dig into the
logs to check why Liviu did not add a call to pci_remap_iospace()
in of_pci_get_host_bridge_resources() - I want to do that actually).

The point here is: IO space (in DT and ACPI) handling is arch specific.

For DT, by relying on PCI_IOBASE, we left that code in drivers/of and
it works (well, with some niggles - see the thread with Murali on IO
space on TI keystone) for ARM/ARM64.

http://www.spinics.net/lists/linux-pci/msg49725.html

What are we going to do with the ACPI version ?

Do we want to add an arch specific call that takes the raw resource
describing IO space and creates an IO port resource (and the MMIO
equivalent - that's what add_io_space() does in IA64) and use that
in generic ACPI parsing code ?

Or we just do what Tomasz does, which is basically the approach we took
for DT ?

> > Furthermore, the same I/O resources need to be released after hotplug
> > removal so that it can be re-added back by the pci_remap_iospace
> > function during insertion. Therefore we implement new pci_unmap_iospace call
> > which unmaps I/O space as the symmetry to pci_remap_iospace.
> 
> "Furthermore" is a hint that we should check to see if this can be
> split into two patches.
> 
> We already have a pci_remap_iospace(), and you're adding
> pci_unmap_iospace(), which will be used for hotplug removal.  So let's 
> add pci_unmap_iospace() first in a patch by itself because that's
> potentially useful for other callers of pci_remap_iospace(), even if
> they don't need the acpi_pci_root_remap_iospace() stuff.

I agree.

Thanks,
Lorenzo

> > Signed-off-by: Jayachandran C <jchandra@broadcom.com>
> > Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> > Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> > ---
> >  drivers/acpi/pci_root.c | 33 +++++++++++++++++++++++++++++++++
> >  drivers/pci/pci.c       | 24 ++++++++++++++++++++++++
> >  include/linux/pci.h     |  1 +
> >  3 files changed, 58 insertions(+)
> > 
> > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> > index d9a70c4..815b6ca 100644
> > --- a/drivers/acpi/pci_root.c
> > +++ b/drivers/acpi/pci_root.c
> > @@ -742,6 +742,34 @@ next:
> >  			resource_list_add_tail(entry, resources);
> >  	}
> >  }
> > +static void acpi_pci_root_remap_iospace(struct resource_entry *entry)
> > +{
> > +#ifdef PCI_IOBASE
> > +	struct resource *res = entry->res;
> > +	resource_size_t cpu_addr = res->start;
> > +	resource_size_t pci_addr = cpu_addr - entry->offset;
> > +	resource_size_t length = resource_size(res);
> > +	unsigned long port;
> > +
> > +	if (pci_register_io_range(cpu_addr, length))
> > +		goto err;
> > +
> > +	port = pci_address_to_pio(cpu_addr);
> > +	if (port == (unsigned long)-1)
> > +		goto err;
> > +
> > +	res->start = port;
> > +	res->end = port + length - 1;
> > +	entry->offset = port - pci_addr;
> > +
> > +	if (pci_remap_iospace(res, cpu_addr) < 0)
> > +		goto err;
> > +	pr_info("Remapped I/O %pa to %pR\n", &cpu_addr, res);
> > +	return;
> > +err:
> > +	res->flags |= IORESOURCE_DISABLED;
> > +#endif
> > +}
> >  
> >  int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info)
> >  {
> > @@ -763,6 +791,9 @@ int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info)
> >  			"no IO and memory resources present in _CRS\n");
> >  	else {
> >  		resource_list_for_each_entry_safe(entry, tmp, list) {
> > +			if (entry->res->flags & IORESOURCE_IO)
> > +				acpi_pci_root_remap_iospace(entry);
> > +
> >  			if (entry->res->flags & IORESOURCE_DISABLED)
> >  				resource_list_destroy_entry(entry);
> >  			else
> > @@ -834,6 +865,8 @@ static void acpi_pci_root_release_info(struct pci_host_bridge *bridge)
> >  
> >  	resource_list_for_each_entry(entry, &bridge->windows) {
> >  		res = entry->res;
> > +		if (res->flags & IORESOURCE_IO)
> > +			pci_unmap_iospace(res);
> >  		if (res->parent &&
> >  		    (res->flags & (IORESOURCE_MEM | IORESOURCE_IO)))
> >  			release_resource(res);
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 89e9996..c0f8a4e 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -26,6 +26,7 @@
> >  #include <linux/device.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/pci_hotplug.h>
> > +#include <linux/vmalloc.h>
> >  #include <asm/setup.h>
> >  #include <linux/aer.h>
> >  #include "pci.h"
> > @@ -3168,6 +3169,29 @@ int __weak pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
> >  #endif
> >  }
> >  
> > +/**
> > + *	pci_unmap_iospace - Unmap the memory mapped I/O space
> > + *	@res: resource to be unmapped
> > + *
> > + *	Unmap the CPU virtual address @res from virtual address space.
> > + *	Only architectures that have memory mapped IO functions defined
> > + *	(and the PCI_IOBASE value defined) should call this function.
> > + */
> > +void  pci_unmap_iospace(struct resource *res)
> > +{
> > +#if defined(PCI_IOBASE) && defined(CONFIG_MMU)
> > +	unsigned long vaddr = (unsigned long)PCI_IOBASE + res->start;
> > +
> > +	unmap_kernel_range(vaddr, resource_size(res));
> > +#else
> > +	/*
> > +	 * This architecture does not have memory mapped I/O space,
> > +	 * so this function should never be called.
> > +	 */
> > +	WARN_ONCE(1, "This architecture does not support memory mapped I/O\n");
> > +#endif
> > +}
> > +
> >  static void __pci_set_master(struct pci_dev *dev, bool enable)
> >  {
> >  	u16 old_cmd, cmd;
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index c28adb4..df1f33d 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -1168,6 +1168,7 @@ 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);
> > +void pci_unmap_iospace(struct resource *res);
> >  
> >  static inline pci_bus_addr_t pci_bus_address(struct pci_dev *pdev, int bar)
> >  {
> > -- 
> > 1.9.1
> > 
> 

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

#1389221 — Re: [PATCH V6 05/13] acpi, pci: Support IO resources when parsing PCI host bridge resources.

On Wed, Apr 27, 2016 at 03:26:59PM +0100, Lorenzo Pieralisi wrote:
> On Tue, Apr 26, 2016 at 09:39:16PM -0500, Bjorn Helgaas wrote:
> > On Fri, Apr 15, 2016 at 07:06:40PM +0200, Tomasz Nowicki wrote:
> > > Platforms that have memory mapped IO port (such as ARM64) need special
> > > handling for PCI I/O resources. For host bridge's resource probing case
> > > these resources need to be fixed up with pci_register_io_range/pci_remap_iospace etc.
> > 
> > ia64 also has memory-mapped I/O port space.  It would be ideal to find
> > some way to handle ia64 and ARM64 similarly.  At the very least, we
> > have to make sure that this doesn't break ia64.  The ia64 dense/sparse
> > I/O spaces complicate things; I don't know if ARM64 has something
> > similar or not.
> 
> No it does not, and that's exactly the same problem we faced with
> the DT generic version of of_pci_range_to_resource() which basically
> relies on PCI_IOBASE to be defined to add code that creates IO port
> resources out of the MMIO resource describing how IO port space is
> mapped to MMIO (physical) address space.
> 
> IIRC everything hinges on PCI_IOBASE definition to make sure that
> of_pci_range_to_resource() *works*, which means that if PCI_IOBASE is
> not defined (ie IA64) that code - acpi_pci_root_remap_iospace() in this
> case - does nothing.
> 
> So acpi_pci_root_remap_iospace() is of_pci_range_to_resource() ACPI
> equivalent + the pci_remap_iospace() call (I have to dig into the
> logs to check why Liviu did not add a call to pci_remap_iospace()
> in of_pci_get_host_bridge_resources() - I want to do that actually).

Because of_pci_get_host_bridge_resources() only gives you a list of resources,
it doesn't allocate them. An arch or platform could add further filtering
to that list before it gets requested (in our case it is done in pci-host-common.c)

Best regards,
Liviu

> 
> The point here is: IO space (in DT and ACPI) handling is arch specific.
> 
> For DT, by relying on PCI_IOBASE, we left that code in drivers/of and
> it works (well, with some niggles - see the thread with Murali on IO
> space on TI keystone) for ARM/ARM64.
> 
> http://www.spinics.net/lists/linux-pci/msg49725.html
> 
> What are we going to do with the ACPI version ?
> 
> Do we want to add an arch specific call that takes the raw resource
> describing IO space and creates an IO port resource (and the MMIO
> equivalent - that's what add_io_space() does in IA64) and use that
> in generic ACPI parsing code ?
> 
> Or we just do what Tomasz does, which is basically the approach we took
> for DT ?
> 
> > > Furthermore, the same I/O resources need to be released after hotplug
> > > removal so that it can be re-added back by the pci_remap_iospace
> > > function during insertion. Therefore we implement new pci_unmap_iospace call
> > > which unmaps I/O space as the symmetry to pci_remap_iospace.
> > 
> > "Furthermore" is a hint that we should check to see if this can be
> > split into two patches.
> > 
> > We already have a pci_remap_iospace(), and you're adding
> > pci_unmap_iospace(), which will be used for hotplug removal.  So let's 
> > add pci_unmap_iospace() first in a patch by itself because that's
> > potentially useful for other callers of pci_remap_iospace(), even if
> > they don't need the acpi_pci_root_remap_iospace() stuff.
> 
> I agree.
> 
> Thanks,
> Lorenzo
> 
> > > Signed-off-by: Jayachandran C <jchandra@broadcom.com>
> > > Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> > > Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> > > ---
> > >  drivers/acpi/pci_root.c | 33 +++++++++++++++++++++++++++++++++
> > >  drivers/pci/pci.c       | 24 ++++++++++++++++++++++++
> > >  include/linux/pci.h     |  1 +
> > >  3 files changed, 58 insertions(+)
> > > 
> > > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> > > index d9a70c4..815b6ca 100644
> > > --- a/drivers/acpi/pci_root.c
> > > +++ b/drivers/acpi/pci_root.c
> > > @@ -742,6 +742,34 @@ next:
> > >  			resource_list_add_tail(entry, resources);
> > >  	}
> > >  }
> > > +static void acpi_pci_root_remap_iospace(struct resource_entry *entry)
> > > +{
> > > +#ifdef PCI_IOBASE
> > > +	struct resource *res = entry->res;
> > > +	resource_size_t cpu_addr = res->start;
> > > +	resource_size_t pci_addr = cpu_addr - entry->offset;
> > > +	resource_size_t length = resource_size(res);
> > > +	unsigned long port;
> > > +
> > > +	if (pci_register_io_range(cpu_addr, length))
> > > +		goto err;
> > > +
> > > +	port = pci_address_to_pio(cpu_addr);
> > > +	if (port == (unsigned long)-1)
> > > +		goto err;
> > > +
> > > +	res->start = port;
> > > +	res->end = port + length - 1;
> > > +	entry->offset = port - pci_addr;
> > > +
> > > +	if (pci_remap_iospace(res, cpu_addr) < 0)
> > > +		goto err;
> > > +	pr_info("Remapped I/O %pa to %pR\n", &cpu_addr, res);
> > > +	return;
> > > +err:
> > > +	res->flags |= IORESOURCE_DISABLED;
> > > +#endif
> > > +}
> > >  
> > >  int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info)
> > >  {
> > > @@ -763,6 +791,9 @@ int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info)
> > >  			"no IO and memory resources present in _CRS\n");
> > >  	else {
> > >  		resource_list_for_each_entry_safe(entry, tmp, list) {
> > > +			if (entry->res->flags & IORESOURCE_IO)
> > > +				acpi_pci_root_remap_iospace(entry);
> > > +
> > >  			if (entry->res->flags & IORESOURCE_DISABLED)
> > >  				resource_list_destroy_entry(entry);
> > >  			else
> > > @@ -834,6 +865,8 @@ static void acpi_pci_root_release_info(struct pci_host_bridge *bridge)
> > >  
> > >  	resource_list_for_each_entry(entry, &bridge->windows) {
> > >  		res = entry->res;
> > > +		if (res->flags & IORESOURCE_IO)
> > > +			pci_unmap_iospace(res);
> > >  		if (res->parent &&
> > >  		    (res->flags & (IORESOURCE_MEM | IORESOURCE_IO)))
> > >  			release_resource(res);
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index 89e9996..c0f8a4e 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -26,6 +26,7 @@
> > >  #include <linux/device.h>
> > >  #include <linux/pm_runtime.h>
> > >  #include <linux/pci_hotplug.h>
> > > +#include <linux/vmalloc.h>
> > >  #include <asm/setup.h>
> > >  #include <linux/aer.h>
> > >  #include "pci.h"
> > > @@ -3168,6 +3169,29 @@ int __weak pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
> > >  #endif
> > >  }
> > >  
> > > +/**
> > > + *	pci_unmap_iospace - Unmap the memory mapped I/O space
> > > + *	@res: resource to be unmapped
> > > + *
> > > + *	Unmap the CPU virtual address @res from virtual address space.
> > > + *	Only architectures that have memory mapped IO functions defined
> > > + *	(and the PCI_IOBASE value defined) should call this function.
> > > + */
> > > +void  pci_unmap_iospace(struct resource *res)
> > > +{
> > > +#if defined(PCI_IOBASE) && defined(CONFIG_MMU)
> > > +	unsigned long vaddr = (unsigned long)PCI_IOBASE + res->start;
> > > +
> > > +	unmap_kernel_range(vaddr, resource_size(res));
> > > +#else
> > > +	/*
> > > +	 * This architecture does not have memory mapped I/O space,
> > > +	 * so this function should never be called.
> > > +	 */
> > > +	WARN_ONCE(1, "This architecture does not support memory mapped I/O\n");
> > > +#endif
> > > +}
> > > +
> > >  static void __pci_set_master(struct pci_dev *dev, bool enable)
> > >  {
> > >  	u16 old_cmd, cmd;
> > > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > > index c28adb4..df1f33d 100644
> > > --- a/include/linux/pci.h
> > > +++ b/include/linux/pci.h
> > > @@ -1168,6 +1168,7 @@ 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);
> > > +void pci_unmap_iospace(struct resource *res);
> > >  
> > >  static inline pci_bus_addr_t pci_bus_address(struct pci_dev *pdev, int bar)
> > >  {
> > > -- 
> > > 1.9.1
> > > 
> > 
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

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

#1389304 — Re: [PATCH V6 05/13] acpi, pci: Support IO resources when parsing PCI host bridge resources.

On Wed, Apr 27, 2016 at 04:10:36PM +0100, Liviu.Dudau@arm.com wrote:
> On Wed, Apr 27, 2016 at 03:26:59PM +0100, Lorenzo Pieralisi wrote:
> > On Tue, Apr 26, 2016 at 09:39:16PM -0500, Bjorn Helgaas wrote:
> > > On Fri, Apr 15, 2016 at 07:06:40PM +0200, Tomasz Nowicki wrote:
> > > > Platforms that have memory mapped IO port (such as ARM64) need special
> > > > handling for PCI I/O resources. For host bridge's resource probing case
> > > > these resources need to be fixed up with pci_register_io_range/pci_remap_iospace etc.
> > > 
> > > ia64 also has memory-mapped I/O port space.  It would be ideal to find
> > > some way to handle ia64 and ARM64 similarly.  At the very least, we
> > > have to make sure that this doesn't break ia64.  The ia64 dense/sparse
> > > I/O spaces complicate things; I don't know if ARM64 has something
> > > similar or not.
> > 
> > No it does not, and that's exactly the same problem we faced with
> > the DT generic version of of_pci_range_to_resource() which basically
> > relies on PCI_IOBASE to be defined to add code that creates IO port
> > resources out of the MMIO resource describing how IO port space is
> > mapped to MMIO (physical) address space.
> > 
> > IIRC everything hinges on PCI_IOBASE definition to make sure that
> > of_pci_range_to_resource() *works*, which means that if PCI_IOBASE is
> > not defined (ie IA64) that code - acpi_pci_root_remap_iospace() in this
> > case - does nothing.
> > 
> > So acpi_pci_root_remap_iospace() is of_pci_range_to_resource() ACPI
> > equivalent + the pci_remap_iospace() call (I have to dig into the
> > logs to check why Liviu did not add a call to pci_remap_iospace()
> > in of_pci_get_host_bridge_resources() - I want to do that actually).
> 
> Because of_pci_get_host_bridge_resources() only gives you a list of
> resources, it doesn't allocate them. An arch or platform could add
> further filtering to that list before it gets requested (in our case
> it is done in pci-host-common.c)

Well, it does register the IO cpu physical address in pci_register_io_range()
though, if pci_remap_iospace() fails in arch/platform code we can delete the
resource but we must also unregister the corresponding cpu address from
the IO ranges otherwise we end up with stale entries in the io_range_list.

Anyway, it is not related to this thread, I will see what I can do
to improve that API from this standpoint.

Thanks !
Lorenzo

> 
> Best regards,
> Liviu
> 
> > 
> > The point here is: IO space (in DT and ACPI) handling is arch specific.
> > 
> > For DT, by relying on PCI_IOBASE, we left that code in drivers/of and
> > it works (well, with some niggles - see the thread with Murali on IO
> > space on TI keystone) for ARM/ARM64.
> > 
> > http://www.spinics.net/lists/linux-pci/msg49725.html
> > 
> > What are we going to do with the ACPI version ?
> > 
> > Do we want to add an arch specific call that takes the raw resource
> > describing IO space and creates an IO port resource (and the MMIO
> > equivalent - that's what add_io_space() does in IA64) and use that
> > in generic ACPI parsing code ?
> > 
> > Or we just do what Tomasz does, which is basically the approach we took
> > for DT ?
> > 
> > > > Furthermore, the same I/O resources need to be released after hotplug
> > > > removal so that it can be re-added back by the pci_remap_iospace
> > > > function during insertion. Therefore we implement new pci_unmap_iospace call
> > > > which unmaps I/O space as the symmetry to pci_remap_iospace.
> > > 
> > > "Furthermore" is a hint that we should check to see if this can be
> > > split into two patches.
> > > 
> > > We already have a pci_remap_iospace(), and you're adding
> > > pci_unmap_iospace(), which will be used for hotplug removal.  So let's 
> > > add pci_unmap_iospace() first in a patch by itself because that's
> > > potentially useful for other callers of pci_remap_iospace(), even if
> > > they don't need the acpi_pci_root_remap_iospace() stuff.
> > 
> > I agree.
> > 
> > Thanks,
> > Lorenzo
> > 
> > > > Signed-off-by: Jayachandran C <jchandra@broadcom.com>
> > > > Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> > > > Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> > > > ---
> > > >  drivers/acpi/pci_root.c | 33 +++++++++++++++++++++++++++++++++
> > > >  drivers/pci/pci.c       | 24 ++++++++++++++++++++++++
> > > >  include/linux/pci.h     |  1 +
> > > >  3 files changed, 58 insertions(+)
> > > > 
> > > > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> > > > index d9a70c4..815b6ca 100644
> > > > --- a/drivers/acpi/pci_root.c
> > > > +++ b/drivers/acpi/pci_root.c
> > > > @@ -742,6 +742,34 @@ next:
> > > >  			resource_list_add_tail(entry, resources);
> > > >  	}
> > > >  }
> > > > +static void acpi_pci_root_remap_iospace(struct resource_entry *entry)
> > > > +{
> > > > +#ifdef PCI_IOBASE
> > > > +	struct resource *res = entry->res;
> > > > +	resource_size_t cpu_addr = res->start;
> > > > +	resource_size_t pci_addr = cpu_addr - entry->offset;
> > > > +	resource_size_t length = resource_size(res);
> > > > +	unsigned long port;
> > > > +
> > > > +	if (pci_register_io_range(cpu_addr, length))
> > > > +		goto err;
> > > > +
> > > > +	port = pci_address_to_pio(cpu_addr);
> > > > +	if (port == (unsigned long)-1)
> > > > +		goto err;
> > > > +
> > > > +	res->start = port;
> > > > +	res->end = port + length - 1;
> > > > +	entry->offset = port - pci_addr;
> > > > +
> > > > +	if (pci_remap_iospace(res, cpu_addr) < 0)
> > > > +		goto err;
> > > > +	pr_info("Remapped I/O %pa to %pR\n", &cpu_addr, res);
> > > > +	return;
> > > > +err:
> > > > +	res->flags |= IORESOURCE_DISABLED;
> > > > +#endif
> > > > +}
> > > >  
> > > >  int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info)
> > > >  {
> > > > @@ -763,6 +791,9 @@ int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info)
> > > >  			"no IO and memory resources present in _CRS\n");
> > > >  	else {
> > > >  		resource_list_for_each_entry_safe(entry, tmp, list) {
> > > > +			if (entry->res->flags & IORESOURCE_IO)
> > > > +				acpi_pci_root_remap_iospace(entry);
> > > > +
> > > >  			if (entry->res->flags & IORESOURCE_DISABLED)
> > > >  				resource_list_destroy_entry(entry);
> > > >  			else
> > > > @@ -834,6 +865,8 @@ static void acpi_pci_root_release_info(struct pci_host_bridge *bridge)
> > > >  
> > > >  	resource_list_for_each_entry(entry, &bridge->windows) {
> > > >  		res = entry->res;
> > > > +		if (res->flags & IORESOURCE_IO)
> > > > +			pci_unmap_iospace(res);
> > > >  		if (res->parent &&
> > > >  		    (res->flags & (IORESOURCE_MEM | IORESOURCE_IO)))
> > > >  			release_resource(res);
> > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > index 89e9996..c0f8a4e 100644
> > > > --- a/drivers/pci/pci.c
> > > > +++ b/drivers/pci/pci.c
> > > > @@ -26,6 +26,7 @@
> > > >  #include <linux/device.h>
> > > >  #include <linux/pm_runtime.h>
> > > >  #include <linux/pci_hotplug.h>
> > > > +#include <linux/vmalloc.h>
> > > >  #include <asm/setup.h>
> > > >  #include <linux/aer.h>
> > > >  #include "pci.h"
> > > > @@ -3168,6 +3169,29 @@ int __weak pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
> > > >  #endif
> > > >  }
> > > >  
> > > > +/**
> > > > + *	pci_unmap_iospace - Unmap the memory mapped I/O space
> > > > + *	@res: resource to be unmapped
> > > > + *
> > > > + *	Unmap the CPU virtual address @res from virtual address space.
> > > > + *	Only architectures that have memory mapped IO functions defined
> > > > + *	(and the PCI_IOBASE value defined) should call this function.
> > > > + */
> > > > +void  pci_unmap_iospace(struct resource *res)
> > > > +{
> > > > +#if defined(PCI_IOBASE) && defined(CONFIG_MMU)
> > > > +	unsigned long vaddr = (unsigned long)PCI_IOBASE + res->start;
> > > > +
> > > > +	unmap_kernel_range(vaddr, resource_size(res));
> > > > +#else
> > > > +	/*
> > > > +	 * This architecture does not have memory mapped I/O space,
> > > > +	 * so this function should never be called.
> > > > +	 */
> > > > +	WARN_ONCE(1, "This architecture does not support memory mapped I/O\n");
> > > > +#endif
> > > > +}
> > > > +
> > > >  static void __pci_set_master(struct pci_dev *dev, bool enable)
> > > >  {
> > > >  	u16 old_cmd, cmd;
> > > > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > > > index c28adb4..df1f33d 100644
> > > > --- a/include/linux/pci.h
> > > > +++ b/include/linux/pci.h
> > > > @@ -1168,6 +1168,7 @@ 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);
> > > > +void pci_unmap_iospace(struct resource *res);
> > > >  
> > > >  static inline pci_bus_addr_t pci_bus_address(struct pci_dev *pdev, int bar)
> > > >  {
> > > > -- 
> > > > 1.9.1
> > > > 
> > > 
> > 
> 
> -- 
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ??\_(???)_/??

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

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

Back to top | Article view | linux.kernel

csiph-web