From: Tomasz Nowicki <hidden> Date: 2016-04-15 17:07:14
From the functionality point of view this series might be split into the
following logic parts:
1. Necessary fixes as the preparation for using driver on ARM64.
2. New ECAM API and update for users of the pci-host-common API
3. Use new MCFG interface and implement generic ACPI based PCI host controller driver.
4. Enable above driver on ARM64
Patches has been built on top of 4.6-rc2 and can be found here:
git at github.com:semihalf-nowicki-tomasz/linux.git (pci-acpi-v6)
This has been tested on Cavium ThunderX server. Any help in reviewing and
testing is very appreciated.
v5 -> v6
- dropped idea of x86 MMCONFIG code refactoring
- integrated JC's patches which introduce new ECAM API:
https://lkml.org/lkml/2016/4/11/907
git: https://github.com/jchandra-brcm/linux/ (arm64-acpi-pci-v3)
- integrated Sinan's fix for releasing IO resources, see patch [06/13]
- added ACPI support for ThunderX ECAM and PEM drivers
- rebased to 4.6-rc2
v4 -> v5
- dropped MCFG refactoring group patches 1-6 from series v4 and integrated Jayachandran's patch
https://patchwork.ozlabs.org/patch/575525/
- rewrite PCI legacy IRQs allocation
- squashed two patches 11 and 12 from series v4, fixed bisection issue
- changelog improvements
- rebased to 4.5-rc3
v3 -> v4
- dropped Jiang's fix http://lkml.iu.edu/hypermail/linux/kernel/1601.1/04318.html
- added Lorenzo's fix patch 19/24
- ACPI PCI bus domain number assigning cleanup
- changed resource management, we now claim and reassign resources
- improvements for applying quirks
- dropped Matthew's http://www.spinics.net/lists/linux-pci/msg45950.html dependency
- rebased to 4.5-rc1
v2 -> v3
- fix legacy IRQ assigning and IO ports registration
- remove reference to arch specific companion device for ia64
- move ACPI PCI host controller driver to pci_root.c
- drop generic domain assignment for x86 and ia64 as I am not
able to run all necessary test variants
- drop patch which cleaned legacy IRQ assignment since it belongs to
Mathew's series:
https://patchwork.ozlabs.org/patch/557504/
- extend MCFG quirk code
- rebased to 4.4
v1 -> v2
- moved non-arch specific piece of code to dirver/acpi/ directory
- fixed IO resource handling
- introduced PCI config accessors quirks matching
- moved ACPI_COMPANION_SET to generic code
v1 - https://lkml.org/lkml/2015/10/27/504
v2 - https://lkml.org/lkml/2015/12/16/246
v3 - http://lkml.iu.edu/hypermail/linux/kernel/1601.1/04308.html
v4 - https://lkml.org/lkml/2016/2/4/646
v5 - https://lkml.org/lkml/2016/2/16/426
Jayachandran C (2):
PCI: Provide common functions for ECAM mapping
PCI: generic, thunder: update to use generic ECAM API
Tomasz Nowicki (11):
pci, acpi, x86, ia64: Move ACPI host bridge device companion
assignment to core code.
pci, acpi: Provide generic way to assign bus domain number.
x86, ia64: Include acpi_pci_{add|remove}_bus to the default
pcibios_{add|remove}_bus implementation.
pci, of: Move the PCI I/O space management to PCI core code.
acpi, pci: Support IO resources when parsing PCI host bridge
resources.
arm64, pci, acpi: ACPI support for legacy IRQs parsing and
consolidation with DT code.
pci, acpi: Support for ACPI based generic PCI host controller
arm64, pci, acpi: Start using ACPI based PCI host controller driver
for ARM64.
pci, acpi: Match PCI config space accessors against platfrom specific
quirks.
pci, pci-thunder-ecam: Add ACPI support for ThunderX ECAM.
pci, pci-thunder-pem: Add ACPI support for ThunderX PEM.
arch/arm64/Kconfig | 15 +++
arch/arm64/include/asm/cpufeature.h | 3 +-
arch/arm64/kernel/cpu_errata.c | 8 ++
arch/arm64/kernel/pci.c | 35 ++---
arch/ia64/hp/common/sba_iommu.c | 2 +-
arch/ia64/include/asm/pci.h | 1 -
arch/ia64/pci/pci.c | 26 ----
arch/ia64/sn/kernel/io_acpi_init.c | 4 +-
arch/x86/include/asm/pci.h | 3 -
arch/x86/pci/acpi.c | 17 ---
arch/x86/pci/common.c | 10 --
drivers/acpi/Kconfig | 8 ++
drivers/acpi/Makefile | 1 +
drivers/acpi/bus.c | 1 +
drivers/acpi/pci_gen_host.c | 259 ++++++++++++++++++++++++++++++++++++
drivers/acpi/pci_root.c | 58 +++++++-
drivers/of/address.c | 116 +---------------
drivers/pci/Kconfig | 3 +
drivers/pci/Makefile | 2 +
drivers/pci/ecam.c | 137 +++++++++++++++++++
drivers/pci/ecam.h | 66 +++++++++
drivers/pci/host/Kconfig | 1 +
drivers/pci/host/pci-host-common.c | 119 ++++++++---------
drivers/pci/host/pci-host-common.h | 47 -------
drivers/pci/host/pci-host-generic.c | 52 ++------
drivers/pci/host/pci-thunder-ecam.c | 70 ++++++----
drivers/pci/host/pci-thunder-pem.c | 215 ++++++++++++++++++++++--------
drivers/pci/pci.c | 150 ++++++++++++++++++++-
drivers/pci/probe.c | 5 +
include/asm-generic/vmlinux.lds.h | 7 +
include/linux/of_address.h | 9 --
include/linux/pci-acpi.h | 20 +++
include/linux/pci.h | 12 ++
33 files changed, 1029 insertions(+), 453 deletions(-)
create mode 100644 drivers/acpi/pci_gen_host.c
create mode 100644 drivers/pci/ecam.c
create mode 100644 drivers/pci/ecam.h
delete mode 100644 drivers/pci/host/pci-host-common.h
--
1.9.1
From: Tomasz Nowicki <hidden> Date: 2016-04-15 17:07:20
As we now have valid PCI host bridge device reference we can
introduce code that is going to find its bus domain number using
ACPI _SEG method.
Note that _SEG method is optional, therefore _SEG absence means
that all PCI buses belong to domain 0.
While at it, for the sake of code clarity we put ACPI and DT domain
assign methods into the corresponding helpers.
Signed-off-by: Tomasz Nowicki <redacted>
Reviewed-by: Liviu Dudau <Liviu.Dudau@arm.com>
Tested-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
Tested-by: Jeremy Linton <redacted>
Tested-by: Duc Dang <redacted>
Tested-by: Dongdong Liu <redacted>
Tested-by: Hanjun Guo <redacted>
Tested-by: Graeme Gregory <redacted>
Tested-by: Sinan Kaya <redacted>
---
drivers/acpi/pci_root.c | 18 ++++++++++++++++++
drivers/pci/pci.c | 11 +++++++++--
include/linux/pci-acpi.h | 2 ++
3 files changed, 29 insertions(+), 2 deletions(-)
On Fri, Apr 15, 2016 at 07:06:37PM +0200, Tomasz Nowicki wrote:
quoted hunk
As we now have valid PCI host bridge device reference we can
introduce code that is going to find its bus domain number using
ACPI _SEG method.
Note that _SEG method is optional, therefore _SEG absence means
that all PCI buses belong to domain 0.
While at it, for the sake of code clarity we put ACPI and DT domain
assign methods into the corresponding helpers.
Signed-off-by: Tomasz Nowicki <redacted>
Reviewed-by: Liviu Dudau <Liviu.Dudau@arm.com>
Tested-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
Tested-by: Jeremy Linton <redacted>
Tested-by: Duc Dang <redacted>
Tested-by: Dongdong Liu <redacted>
Tested-by: Hanjun Guo <redacted>
Tested-by: Graeme Gregory <redacted>
Tested-by: Sinan Kaya <redacted>
---
drivers/acpi/pci_root.c | 18 ++++++++++++++++++
drivers/pci/pci.c | 11 +++++++++--
include/linux/pci-acpi.h | 2 ++
3 files changed, 29 insertions(+), 2 deletions(-)
We already have code in acpi_pci_root_add() to evaluate _SEG. We
don't want to evaluate it *twice*, do we?
I was sort of expecting that if you added it here, we'd remove the
existing call, but it looks like you're keeping both?
quoted hunk
+ if (ACPI_FAILURE(status) && status != AE_NOT_FOUND)
+ dev_err(&acpi_dev->dev, "can't evaluate _SEG\n");
+
+ return segment;
+}
+
static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm)
{
u32 support, control, requested;
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Lorenzo Pieralisi <hidden> Date: 2016-04-27 11:18:06
On Tue, Apr 26, 2016 at 09:26:49PM -0500, Bjorn Helgaas wrote:
On Fri, Apr 15, 2016 at 07:06:37PM +0200, Tomasz Nowicki wrote:
quoted
As we now have valid PCI host bridge device reference we can
introduce code that is going to find its bus domain number using
ACPI _SEG method.
Note that _SEG method is optional, therefore _SEG absence means
that all PCI buses belong to domain 0.
While at it, for the sake of code clarity we put ACPI and DT domain
assign methods into the corresponding helpers.
Signed-off-by: Tomasz Nowicki <redacted>
Reviewed-by: Liviu Dudau <Liviu.Dudau@arm.com>
Tested-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
Tested-by: Jeremy Linton <redacted>
Tested-by: Duc Dang <redacted>
Tested-by: Dongdong Liu <redacted>
Tested-by: Hanjun Guo <redacted>
Tested-by: Graeme Gregory <redacted>
Tested-by: Sinan Kaya <redacted>
---
drivers/acpi/pci_root.c | 18 ++++++++++++++++++
drivers/pci/pci.c | 11 +++++++++--
include/linux/pci-acpi.h | 2 ++
3 files changed, 29 insertions(+), 2 deletions(-)
We already have code in acpi_pci_root_add() to evaluate _SEG. We
don't want to evaluate it *twice*, do we?
I was sort of expecting that if you added it here, we'd remove the
existing call, but it looks like you're keeping both?
We can't remove the existing call, since it is used on X86 and IA64
to store the segment number that, in the process, is used in their
pci_domain_nr() arch specific callback to retrieve the domain nr.
On ARM64, that selects PCI_DOMAINS_GENERIC, we have to find a way
to retrieve the domain number that is not arch dependent, since
this is generic code, we can't rely on any bus->sysdata format (unless
we do something like JC did below), therefore the only way is to call
the _SEG method *again* here, which also forced Tomasz to go through
the ACPI_COMPANION setting song and dance and pass the parent pointer
to pci_create_root_bus() (see patch 1), which BTW is a source of
trouble on its own as you noticed.
JC solved it differently, via sysdata and pseudo-generic code:
http://www.spinics.net/lists/arm-kernel/msg478167.htmlhttp://www.spinics.net/lists/arm-kernel/msg478169.html
I like neither, we need the lesser of two evils though.
Lorenzo
quoted
+ if (ACPI_FAILURE(status) && status != AE_NOT_FOUND)
+ dev_err(&acpi_dev->dev, "can't evaluate _SEG\n");
+
+ return segment;
+}
+
static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm)
{
u32 support, control, requested;
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 27, 2016 at 12:17:58PM +0100, Lorenzo Pieralisi wrote:
On Tue, Apr 26, 2016 at 09:26:49PM -0500, Bjorn Helgaas wrote:
quoted
On Fri, Apr 15, 2016 at 07:06:37PM +0200, Tomasz Nowicki wrote:
quoted
As we now have valid PCI host bridge device reference we can
introduce code that is going to find its bus domain number using
ACPI _SEG method.
Note that _SEG method is optional, therefore _SEG absence means
that all PCI buses belong to domain 0.
While at it, for the sake of code clarity we put ACPI and DT domain
assign methods into the corresponding helpers.
Signed-off-by: Tomasz Nowicki <redacted>
Reviewed-by: Liviu Dudau <Liviu.Dudau@arm.com>
Tested-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
Tested-by: Jeremy Linton <redacted>
Tested-by: Duc Dang <redacted>
Tested-by: Dongdong Liu <redacted>
Tested-by: Hanjun Guo <redacted>
Tested-by: Graeme Gregory <redacted>
Tested-by: Sinan Kaya <redacted>
---
drivers/acpi/pci_root.c | 18 ++++++++++++++++++
drivers/pci/pci.c | 11 +++++++++--
include/linux/pci-acpi.h | 2 ++
3 files changed, 29 insertions(+), 2 deletions(-)
It looks like acpi_pci_bus_domain_nr() could be under #ifdef
CONFIG_PCI_DOMAINS_GENERIC, right?
quoted
quoted
+{
+ struct acpi_device *acpi_dev = to_acpi_device(parent);
+ unsigned long long segment = 0;
+ acpi_status status;
+
+ /*
+ * If _SEG method does not exist, following ACPI spec (6.5.6)
+ * all PCI buses belong to domain 0.
+ */
+ status = acpi_evaluate_integer(acpi_dev->handle, METHOD_NAME__SEG, NULL,
+ &segment);
We already have code in acpi_pci_root_add() to evaluate _SEG. We
don't want to evaluate it *twice*, do we?
I was sort of expecting that if you added it here, we'd remove the
existing call, but it looks like you're keeping both?
We can't remove the existing call, since it is used on X86 and IA64
to store the segment number that, in the process, is used in their
pci_domain_nr() arch specific callback to retrieve the domain nr.
On ARM64, that selects PCI_DOMAINS_GENERIC, we have to find a way
to retrieve the domain number that is not arch dependent, since
this is generic code, we can't rely on any bus->sysdata format (unless
we do something like JC did below), therefore the only way is to call
the _SEG method *again* here, which also forced Tomasz to go through
the ACPI_COMPANION setting song and dance and pass the parent pointer
to pci_create_root_bus() (see patch 1), which BTW is a source of
trouble on its own as you noticed.
JC solved it differently, via sysdata and pseudo-generic code:
http://www.spinics.net/lists/arm-kernel/msg478167.html
The thing I don't like about this is the special case of checking
parent and parent->of_node to figure out whether we should use the
segment from ACPI and the fragility of depending on the fact that the
companion hasn't been set yet.
Today we call pci_bus_assign_domain_nr() from the PCI core (from
pci_create_root_bus()). This is only implemented for
PCI_DOMAINS_GENERIC, but even so, it fiddles around to figure out
whether to get the domain from DT or to assign a new one.
That seems backwards to me. The host bridge drivers already know
where the domain should come from (ACPI _SEG, DT, etc.) and in the
long term, I think they should be responsible for looking up or
assigning a domain number *before* they call pci_create_root_bus().
quoted
quoted
+ if (ACPI_FAILURE(status) && status != AE_NOT_FOUND)
+ dev_err(&acpi_dev->dev, "can't evaluate _SEG\n");
+
+ return segment;
+}
+
static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm)
{
u32 support, control, requested;
We have the pci_bus * here, so to_pci_host_bridge(bus->bridge) gives
us the struct pci_host_bridge. I can't remember why we put domain_nr
in the struct pci_bus instead of in the struct pci_host_bridge. It
seems like pci_host_bridge is the more logical place for it, because
every bus below the host bridge must have the same domain by
definition.
Would it be feasible to either (a) move domain_nr to the
pci_host_bridge, or (b) change acpi_pci_bus_domain_nr() so it uses the
struct pci_bus * or the struct device * to find the struct
acpi_pci_root where segment has already been stored by
acpi_pci_root_add()?
Another wrinkle is the quirk added by 1f09b09b4de0 ("x86/PCI: Ignore
_SEG on HP xw9300"). x86 doesn't use PCI_DOMAINS_GENERIC yet, so this
patch wouldn't break it, but I hope x86 can use PCI_DOMAINS_GENERIC in
the future, and then it will be a problem if we evaluate _SEG again.
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Lorenzo Pieralisi <hidden> Date: 2016-04-27 17:31:38
On Wed, Apr 27, 2016 at 11:44:53AM -0500, Bjorn Helgaas wrote:
On Wed, Apr 27, 2016 at 12:17:58PM +0100, Lorenzo Pieralisi wrote:
quoted
On Tue, Apr 26, 2016 at 09:26:49PM -0500, Bjorn Helgaas wrote:
quoted
On Fri, Apr 15, 2016 at 07:06:37PM +0200, Tomasz Nowicki wrote:
quoted
As we now have valid PCI host bridge device reference we can
introduce code that is going to find its bus domain number using
ACPI _SEG method.
Note that _SEG method is optional, therefore _SEG absence means
that all PCI buses belong to domain 0.
While at it, for the sake of code clarity we put ACPI and DT domain
assign methods into the corresponding helpers.
Signed-off-by: Tomasz Nowicki <redacted>
Reviewed-by: Liviu Dudau <Liviu.Dudau@arm.com>
Tested-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
Tested-by: Jeremy Linton <redacted>
Tested-by: Duc Dang <redacted>
Tested-by: Dongdong Liu <redacted>
Tested-by: Hanjun Guo <redacted>
Tested-by: Graeme Gregory <redacted>
Tested-by: Sinan Kaya <redacted>
---
drivers/acpi/pci_root.c | 18 ++++++++++++++++++
drivers/pci/pci.c | 11 +++++++++--
include/linux/pci-acpi.h | 2 ++
3 files changed, 29 insertions(+), 2 deletions(-)
It looks like acpi_pci_bus_domain_nr() could be under #ifdef
CONFIG_PCI_DOMAINS_GENERIC, right?
Yes it should.
quoted
quoted
quoted
+{
+ struct acpi_device *acpi_dev = to_acpi_device(parent);
+ unsigned long long segment = 0;
+ acpi_status status;
+
+ /*
+ * If _SEG method does not exist, following ACPI spec (6.5.6)
+ * all PCI buses belong to domain 0.
+ */
+ status = acpi_evaluate_integer(acpi_dev->handle, METHOD_NAME__SEG, NULL,
+ &segment);
We already have code in acpi_pci_root_add() to evaluate _SEG. We
don't want to evaluate it *twice*, do we?
I was sort of expecting that if you added it here, we'd remove the
existing call, but it looks like you're keeping both?
We can't remove the existing call, since it is used on X86 and IA64
to store the segment number that, in the process, is used in their
pci_domain_nr() arch specific callback to retrieve the domain nr.
On ARM64, that selects PCI_DOMAINS_GENERIC, we have to find a way
to retrieve the domain number that is not arch dependent, since
this is generic code, we can't rely on any bus->sysdata format (unless
we do something like JC did below), therefore the only way is to call
the _SEG method *again* here, which also forced Tomasz to go through
the ACPI_COMPANION setting song and dance and pass the parent pointer
to pci_create_root_bus() (see patch 1), which BTW is a source of
trouble on its own as you noticed.
JC solved it differently, via sysdata and pseudo-generic code:
http://www.spinics.net/lists/arm-kernel/msg478167.html
The thing I don't like about this is the special case of checking
parent and parent->of_node to figure out whether we should use the
segment from ACPI and the fragility of depending on the fact that the
companion hasn't been set yet.
Today we call pci_bus_assign_domain_nr() from the PCI core (from
pci_create_root_bus()). This is only implemented for
PCI_DOMAINS_GENERIC, but even so, it fiddles around to figure out
whether to get the domain from DT or to assign a new one.
That seems backwards to me. The host bridge drivers already know
where the domain should come from (ACPI _SEG, DT, etc.) and in the
long term, I think they should be responsible for looking up or
assigning a domain number *before* they call pci_create_root_bus().
Yes, the question still is how pci_create_root_bus() can get that
value (I am pretty certain this was heavily debated in the past, which
does not mean we can't give it another try).
quoted
quoted
quoted
+ if (ACPI_FAILURE(status) && status != AE_NOT_FOUND)
+ dev_err(&acpi_dev->dev, "can't evaluate _SEG\n");
+
+ return segment;
+}
+
static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm)
{
u32 support, control, requested;
We have the pci_bus * here, so to_pci_host_bridge(bus->bridge) gives
us the struct pci_host_bridge. I can't remember why we put domain_nr
in the struct pci_bus instead of in the struct pci_host_bridge. It
seems like pci_host_bridge is the more logical place for it, because
every bus below the host bridge must have the same domain by
definition.
Would it be feasible to either (a) move domain_nr to the
pci_host_bridge, or (b) change acpi_pci_bus_domain_nr() so it uses the
struct pci_bus * or the struct device * to find the struct
acpi_pci_root where segment has already been stored by
acpi_pci_root_add()?
(b) is what JC implemented even though it works differently for
different hosts since it all depends on what's in bus->sysdata.
It can certainly be done in a generic way (that works on X86 and IA64
too), let's give it more thought.
Another wrinkle is the quirk added by 1f09b09b4de0 ("x86/PCI: Ignore
_SEG on HP xw9300"). x86 doesn't use PCI_DOMAINS_GENERIC yet, so this
patch wouldn't break it, but I hope x86 can use PCI_DOMAINS_GENERIC in
the future, and then it will be a problem if we evaluate _SEG again.
Yes, I share your concern here and I thought about that, if that's the
end goal let's find a solution that works across arches (or we temporarily
use JC's code and we then generalize it).
Thanks,
Lorenzo
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 27, 2016 at 06:31:29PM +0100, Lorenzo Pieralisi wrote:
On Wed, Apr 27, 2016 at 11:44:53AM -0500, Bjorn Helgaas wrote:
quoted
On Wed, Apr 27, 2016 at 12:17:58PM +0100, Lorenzo Pieralisi wrote:
quoted
On Tue, Apr 26, 2016 at 09:26:49PM -0500, Bjorn Helgaas wrote:
quoted
On Fri, Apr 15, 2016 at 07:06:37PM +0200, Tomasz Nowicki wrote:
quoted
As we now have valid PCI host bridge device reference we can
introduce code that is going to find its bus domain number using
ACPI _SEG method.
Note that _SEG method is optional, therefore _SEG absence means
that all PCI buses belong to domain 0.
While at it, for the sake of code clarity we put ACPI and DT domain
assign methods into the corresponding helpers.
Signed-off-by: Tomasz Nowicki <redacted>
Reviewed-by: Liviu Dudau <Liviu.Dudau@arm.com>
Tested-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
Tested-by: Jeremy Linton <redacted>
Tested-by: Duc Dang <redacted>
Tested-by: Dongdong Liu <redacted>
Tested-by: Hanjun Guo <redacted>
Tested-by: Graeme Gregory <redacted>
Tested-by: Sinan Kaya <redacted>
---
drivers/acpi/pci_root.c | 18 ++++++++++++++++++
drivers/pci/pci.c | 11 +++++++++--
include/linux/pci-acpi.h | 2 ++
3 files changed, 29 insertions(+), 2 deletions(-)
It looks like acpi_pci_bus_domain_nr() could be under #ifdef
CONFIG_PCI_DOMAINS_GENERIC, right?
Yes it should.
quoted
quoted
quoted
quoted
+{
+ struct acpi_device *acpi_dev = to_acpi_device(parent);
+ unsigned long long segment = 0;
+ acpi_status status;
+
+ /*
+ * If _SEG method does not exist, following ACPI spec (6.5.6)
+ * all PCI buses belong to domain 0.
+ */
+ status = acpi_evaluate_integer(acpi_dev->handle, METHOD_NAME__SEG, NULL,
+ &segment);
We already have code in acpi_pci_root_add() to evaluate _SEG. We
don't want to evaluate it *twice*, do we?
I was sort of expecting that if you added it here, we'd remove the
existing call, but it looks like you're keeping both?
We can't remove the existing call, since it is used on X86 and IA64
to store the segment number that, in the process, is used in their
pci_domain_nr() arch specific callback to retrieve the domain nr.
On ARM64, that selects PCI_DOMAINS_GENERIC, we have to find a way
to retrieve the domain number that is not arch dependent, since
this is generic code, we can't rely on any bus->sysdata format (unless
we do something like JC did below), therefore the only way is to call
the _SEG method *again* here, which also forced Tomasz to go through
the ACPI_COMPANION setting song and dance and pass the parent pointer
to pci_create_root_bus() (see patch 1), which BTW is a source of
trouble on its own as you noticed.
JC solved it differently, via sysdata and pseudo-generic code:
http://www.spinics.net/lists/arm-kernel/msg478167.html
The thing I don't like about this is the special case of checking
parent and parent->of_node to figure out whether we should use the
segment from ACPI and the fragility of depending on the fact that the
companion hasn't been set yet.
Today we call pci_bus_assign_domain_nr() from the PCI core (from
pci_create_root_bus()). This is only implemented for
PCI_DOMAINS_GENERIC, but even so, it fiddles around to figure out
whether to get the domain from DT or to assign a new one.
That seems backwards to me. The host bridge drivers already know
where the domain should come from (ACPI _SEG, DT, etc.) and in the
long term, I think they should be responsible for looking up or
assigning a domain number *before* they call pci_create_root_bus().
Yes, the question still is how pci_create_root_bus() can get that
value (I am pretty certain this was heavily debated in the past, which
does not mean we can't give it another try).
The main issue is that pci_create_root_bus() does a weird dance trying to
figure out if the root bus hasn't been already allocated. It allocates a
new bus, assigns a domain number and then it tries to find it in the list
of already allocated busses. Because pci_alloc_bus() does not pass any
additional information, pci_bus_assign_domain_nr() needs to try to guess
where the barely initialised bus should live and give you back a number.
Simplifying the creation of root busses to be the job of the host bridges
would greatly simplify the code as well.
Best regards,
Liviu
quoted
quoted
quoted
quoted
+ if (ACPI_FAILURE(status) && status != AE_NOT_FOUND)
+ dev_err(&acpi_dev->dev, "can't evaluate _SEG\n");
+
+ return segment;
+}
+
static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm)
{
u32 support, control, requested;
We have the pci_bus * here, so to_pci_host_bridge(bus->bridge) gives
us the struct pci_host_bridge. I can't remember why we put domain_nr
in the struct pci_bus instead of in the struct pci_host_bridge. It
seems like pci_host_bridge is the more logical place for it, because
every bus below the host bridge must have the same domain by
definition.
Would it be feasible to either (a) move domain_nr to the
pci_host_bridge, or (b) change acpi_pci_bus_domain_nr() so it uses the
struct pci_bus * or the struct device * to find the struct
acpi_pci_root where segment has already been stored by
acpi_pci_root_add()?
(b) is what JC implemented even though it works differently for
different hosts since it all depends on what's in bus->sysdata.
It can certainly be done in a generic way (that works on X86 and IA64
too), let's give it more thought.
quoted
Another wrinkle is the quirk added by 1f09b09b4de0 ("x86/PCI: Ignore
_SEG on HP xw9300"). x86 doesn't use PCI_DOMAINS_GENERIC yet, so this
patch wouldn't break it, but I hope x86 can use PCI_DOMAINS_GENERIC in
the future, and then it will be a problem if we evaluate _SEG again.
Yes, I share your concern here and I thought about that, if that's the
end goal let's find a solution that works across arches (or we temporarily
use JC's code and we then generalize it).
Thanks,
Lorenzo
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
?\_(?)_/?
On Wed, Apr 27, 2016 at 06:31:29PM +0100, Lorenzo Pieralisi wrote:
On Wed, Apr 27, 2016 at 11:44:53AM -0500, Bjorn Helgaas wrote:
quoted
On Wed, Apr 27, 2016 at 12:17:58PM +0100, Lorenzo Pieralisi wrote:
quoted
On Tue, Apr 26, 2016 at 09:26:49PM -0500, Bjorn Helgaas wrote:
quoted
On Fri, Apr 15, 2016 at 07:06:37PM +0200, Tomasz Nowicki wrote:
quoted
Today we call pci_bus_assign_domain_nr() from the PCI core (from
pci_create_root_bus()). This is only implemented for
PCI_DOMAINS_GENERIC, but even so, it fiddles around to figure out
whether to get the domain from DT or to assign a new one.
That seems backwards to me. The host bridge drivers already know
where the domain should come from (ACPI _SEG, DT, etc.) and in the
long term, I think they should be responsible for looking up or
assigning a domain number *before* they call pci_create_root_bus().
Yes, the question still is how pci_create_root_bus() can get that
value (I am pretty certain this was heavily debated in the past, which
does not mean we can't give it another try).
Right, we don't have a good mechanism for passing more info into
pci_create_root_bus(). Maybe the caller could fill in a struct so we
have a chance to extend it without having to change all the existing
callers.
I wonder if there's a design pattern we can copy, e.g., would
something like the scsi_host_alloc(), scsi_add_host(),
scsi_scan_host() model work here?
We have the pci_bus * here, so to_pci_host_bridge(bus->bridge) gives
us the struct pci_host_bridge. I can't remember why we put domain_nr
in the struct pci_bus instead of in the struct pci_host_bridge. It
seems like pci_host_bridge is the more logical place for it, because
every bus below the host bridge must have the same domain by
definition.
Would it be feasible to either (a) move domain_nr to the
pci_host_bridge, or (b) change acpi_pci_bus_domain_nr() so it uses the
struct pci_bus * or the struct device * to find the struct
acpi_pci_root where segment has already been stored by
acpi_pci_root_add()?
(b) is what JC implemented even though it works differently for
different hosts since it all depends on what's in bus->sysdata.
It can certainly be done in a generic way (that works on X86 and IA64
too), let's give it more thought.
quoted
Another wrinkle is the quirk added by 1f09b09b4de0 ("x86/PCI: Ignore
_SEG on HP xw9300"). x86 doesn't use PCI_DOMAINS_GENERIC yet, so this
patch wouldn't break it, but I hope x86 can use PCI_DOMAINS_GENERIC in
the future, and then it will be a problem if we evaluate _SEG again.
Yes, I share your concern here and I thought about that, if that's the
end goal let's find a solution that works across arches (or we temporarily
use JC's code and we then generalize it).
I would ultimately like all arches to use PCI_DOMAINS_GENERIC, because
I don't think there's anything intrisically arch-specific about where
we store the domain number. The means of discovering or assigning a
domain number might be arch-specific, but I think it would be cleanest
if the host bridge driver handled that.
Bjorn
On Thursday 28 April 2016 10:12:12 Bjorn Helgaas wrote:
On Wed, Apr 27, 2016 at 06:31:29PM +0100, Lorenzo Pieralisi wrote:
quoted
On Wed, Apr 27, 2016 at 11:44:53AM -0500, Bjorn Helgaas wrote:
quoted
On Wed, Apr 27, 2016 at 12:17:58PM +0100, Lorenzo Pieralisi wrote:
quoted
On Tue, Apr 26, 2016 at 09:26:49PM -0500, Bjorn Helgaas wrote:
quoted
On Fri, Apr 15, 2016 at 07:06:37PM +0200, Tomasz Nowicki wrote:
quoted
quoted
Today we call pci_bus_assign_domain_nr() from the PCI core (from
pci_create_root_bus()). This is only implemented for
PCI_DOMAINS_GENERIC, but even so, it fiddles around to figure out
whether to get the domain from DT or to assign a new one.
That seems backwards to me. The host bridge drivers already know
where the domain should come from (ACPI _SEG, DT, etc.) and in the
long term, I think they should be responsible for looking up or
assigning a domain number *before* they call pci_create_root_bus().
Yes, the question still is how pci_create_root_bus() can get that
value (I am pretty certain this was heavily debated in the past, which
does not mean we can't give it another try).
Right, we don't have a good mechanism for passing more info into
pci_create_root_bus(). Maybe the caller could fill in a struct so we
have a chance to extend it without having to change all the existing
callers.
I wonder if there's a design pattern we can copy, e.g., would
something like the scsi_host_alloc(), scsi_add_host(),
scsi_scan_host() model work here?
Yes, I think that is a good idea in general. Especially
now that we have separate the ARM code from pci_common_init_dev
and pci_sys_data, that can help with cleanups in the other drivers
as well.
I see two common variations in other subsystems: some use a
special alloc() function that you pass the size of the private
data into, while others just expect you to embed a structure
inside of the driver specific one allocate that separately to
have the generic registration function fill out the common fields.
I have a slight preference for the second, but they are really
the same thing basically.
Arnd
On Thursday 28 April 2016 17:34:10 Arnd Bergmann wrote:
On Thursday 28 April 2016 10:12:12 Bjorn Helgaas wrote:
quoted
Right, we don't have a good mechanism for passing more info into
pci_create_root_bus(). Maybe the caller could fill in a struct so we
have a chance to extend it without having to change all the existing
callers.
I wonder if there's a design pattern we can copy, e.g., would
something like the scsi_host_alloc(), scsi_add_host(),
scsi_scan_host() model work here?
Yes, I think that is a good idea in general. Especially
now that we have separate the ARM code from pci_common_init_dev
and pci_sys_data, that can help with cleanups in the other drivers
as well.
I see two common variations in other subsystems: some use a
special alloc() function that you pass the size of the private
data into, while others just expect you to embed a structure
inside of the driver specific one allocate that separately to
have the generic registration function fill out the common fields.
I have a slight preference for the second, but they are really
the same thing basically.
I've tried this out now, and will follow up with a separate patch
series. Overall, I think it works out well, though I haven't gotten
to the point of actually saving code yet. I've converted two
drivers for demonstration.
Arnd
From: Tomasz Nowicki <hidden> Date: 2016-05-02 12:44:08
On 04/27/2016 01:17 PM, Lorenzo Pieralisi wrote:
On Tue, Apr 26, 2016 at 09:26:49PM -0500, Bjorn Helgaas wrote:
quoted
On Fri, Apr 15, 2016 at 07:06:37PM +0200, Tomasz Nowicki wrote:
[...]
quoted
+int acpi_pci_bus_domain_nr(struct device *parent)
+{
+ struct acpi_device *acpi_dev = to_acpi_device(parent);
+ unsigned long long segment = 0;
+ acpi_status status;
+
+ /*
+ * If _SEG method does not exist, following ACPI spec (6.5.6)
+ * all PCI buses belong to domain 0.
+ */
+ status = acpi_evaluate_integer(acpi_dev->handle, METHOD_NAME__SEG, NULL,
+ &segment);
We already have code in acpi_pci_root_add() to evaluate _SEG. We
don't want to evaluate it *twice*, do we?
I was sort of expecting that if you added it here, we'd remove the
existing call, but it looks like you're keeping both?
We can't remove the existing call, since it is used on X86 and IA64
to store the segment number that, in the process, is used in their
pci_domain_nr() arch specific callback to retrieve the domain nr.
On ARM64, that selects PCI_DOMAINS_GENERIC, we have to find a way
to retrieve the domain number that is not arch dependent, since
this is generic code, we can't rely on any bus->sysdata format (unless
we do something like JC did below), therefore the only way is to call
the _SEG method *again* here, which also forced Tomasz to go through
the ACPI_COMPANION setting song and dance and pass the parent pointer
to pci_create_root_bus() (see patch 1), which BTW is a source of
trouble on its own as you noticed.
What trouble in patch 1 do you mean? I may miss something.
I agree that patch 1 is not necessary if we decide to use sysdata or
rework root bus scanning to move domain to host bridge. Nevertheless,
patch 1 is still a cleanup IMO.
Thanks,
Tomasz
From: Jayachandran C <hidden> Date: 2016-05-02 13:27:03
On Mon, May 2, 2016 at 6:13 PM, Tomasz Nowicki [off-list ref] wrote:
On 04/27/2016 01:17 PM, Lorenzo Pieralisi wrote:
quoted
On Tue, Apr 26, 2016 at 09:26:49PM -0500, Bjorn Helgaas wrote:
quoted
On Fri, Apr 15, 2016 at 07:06:37PM +0200, Tomasz Nowicki wrote:
[...]
quoted
quoted
+int acpi_pci_bus_domain_nr(struct device *parent)
+{
+ struct acpi_device *acpi_dev = to_acpi_device(parent);
+ unsigned long long segment = 0;
+ acpi_status status;
+
+ /*
+ * If _SEG method does not exist, following ACPI spec (6.5.6)
+ * all PCI buses belong to domain 0.
+ */
+ status = acpi_evaluate_integer(acpi_dev->handle,
METHOD_NAME__SEG, NULL,
+ &segment);
We already have code in acpi_pci_root_add() to evaluate _SEG. We
don't want to evaluate it *twice*, do we?
I was sort of expecting that if you added it here, we'd remove the
existing call, but it looks like you're keeping both?
We can't remove the existing call, since it is used on X86 and IA64
to store the segment number that, in the process, is used in their
pci_domain_nr() arch specific callback to retrieve the domain nr.
On ARM64, that selects PCI_DOMAINS_GENERIC, we have to find a way
to retrieve the domain number that is not arch dependent, since
this is generic code, we can't rely on any bus->sysdata format (unless
we do something like JC did below), therefore the only way is to call
the _SEG method *again* here, which also forced Tomasz to go through
the ACPI_COMPANION setting song and dance and pass the parent pointer
to pci_create_root_bus() (see patch 1), which BTW is a source of
trouble on its own as you noticed.
What trouble in patch 1 do you mean? I may miss something.
I agree that patch 1 is not necessary if we decide to use sysdata or rework
root bus scanning to move domain to host bridge. Nevertheless, patch 1 is
still a cleanup IMO.
In this case, getting the domain should be trivial since the ACPI
companion on parent is already set, this should work
int acpi_pci_bus_domain_nr(struct device *parent)
{
struct acpi_device *acpi_dev = to_acpi_device(parent);
struct acpi_pci_root *root = acpi_dev->driver_data;
return root->segment;
}
Or am I missing something here?
JC.
From: Lorenzo Pieralisi <hidden> Date: 2016-05-03 11:02:27
On Mon, May 02, 2016 at 06:56:13PM +0530, Jayachandran C wrote:
On Mon, May 2, 2016 at 6:13 PM, Tomasz Nowicki [off-list ref] wrote:
quoted
On 04/27/2016 01:17 PM, Lorenzo Pieralisi wrote:
quoted
On Tue, Apr 26, 2016 at 09:26:49PM -0500, Bjorn Helgaas wrote:
quoted
On Fri, Apr 15, 2016 at 07:06:37PM +0200, Tomasz Nowicki wrote:
[...]
quoted
quoted
+int acpi_pci_bus_domain_nr(struct device *parent)
+{
+ struct acpi_device *acpi_dev = to_acpi_device(parent);
+ unsigned long long segment = 0;
+ acpi_status status;
+
+ /*
+ * If _SEG method does not exist, following ACPI spec (6.5.6)
+ * all PCI buses belong to domain 0.
+ */
+ status = acpi_evaluate_integer(acpi_dev->handle,
METHOD_NAME__SEG, NULL,
+ &segment);
We already have code in acpi_pci_root_add() to evaluate _SEG. We
don't want to evaluate it *twice*, do we?
I was sort of expecting that if you added it here, we'd remove the
existing call, but it looks like you're keeping both?
We can't remove the existing call, since it is used on X86 and IA64
to store the segment number that, in the process, is used in their
pci_domain_nr() arch specific callback to retrieve the domain nr.
On ARM64, that selects PCI_DOMAINS_GENERIC, we have to find a way
to retrieve the domain number that is not arch dependent, since
this is generic code, we can't rely on any bus->sysdata format (unless
we do something like JC did below), therefore the only way is to call
the _SEG method *again* here, which also forced Tomasz to go through
the ACPI_COMPANION setting song and dance and pass the parent pointer
to pci_create_root_bus() (see patch 1), which BTW is a source of
trouble on its own as you noticed.
What trouble in patch 1 do you mean? I may miss something.
I agree that patch 1 is not necessary if we decide to use sysdata or rework
root bus scanning to move domain to host bridge. Nevertheless, patch 1 is
still a cleanup IMO.
In this case, getting the domain should be trivial since the ACPI
companion on parent is already set, this should work
int acpi_pci_bus_domain_nr(struct device *parent)
{
struct acpi_device *acpi_dev = to_acpi_device(parent);
struct acpi_pci_root *root = acpi_dev->driver_data;
return root->segment;
}
Or am I missing something here?
Well, I thought that the whole idea behind this exercise was to move
the domain number into struct pci_host_bridge (Arnd did not do it with
his first set but this does not mean we can't add it as Bjorn suggested),
so that the domain number could be read from there straight away in an
arch (and FW) independent manner, right ?
Lorenzo
From: Jayachandran C <hidden> Date: 2016-05-03 14:23:36
On Tue, May 3, 2016 at 4:32 PM, Lorenzo Pieralisi
[off-list ref] wrote:
On Mon, May 02, 2016 at 06:56:13PM +0530, Jayachandran C wrote:
quoted
On Mon, May 2, 2016 at 6:13 PM, Tomasz Nowicki [off-list ref] wrote:
quoted
On 04/27/2016 01:17 PM, Lorenzo Pieralisi wrote:
quoted
On Tue, Apr 26, 2016 at 09:26:49PM -0500, Bjorn Helgaas wrote:
quoted
On Fri, Apr 15, 2016 at 07:06:37PM +0200, Tomasz Nowicki wrote:
[...]
quoted
quoted
+int acpi_pci_bus_domain_nr(struct device *parent)
+{
+ struct acpi_device *acpi_dev = to_acpi_device(parent);
+ unsigned long long segment = 0;
+ acpi_status status;
+
+ /*
+ * If _SEG method does not exist, following ACPI spec (6.5.6)
+ * all PCI buses belong to domain 0.
+ */
+ status = acpi_evaluate_integer(acpi_dev->handle,
METHOD_NAME__SEG, NULL,
+ &segment);
We already have code in acpi_pci_root_add() to evaluate _SEG. We
don't want to evaluate it *twice*, do we?
I was sort of expecting that if you added it here, we'd remove the
existing call, but it looks like you're keeping both?
We can't remove the existing call, since it is used on X86 and IA64
to store the segment number that, in the process, is used in their
pci_domain_nr() arch specific callback to retrieve the domain nr.
On ARM64, that selects PCI_DOMAINS_GENERIC, we have to find a way
to retrieve the domain number that is not arch dependent, since
this is generic code, we can't rely on any bus->sysdata format (unless
we do something like JC did below), therefore the only way is to call
the _SEG method *again* here, which also forced Tomasz to go through
the ACPI_COMPANION setting song and dance and pass the parent pointer
to pci_create_root_bus() (see patch 1), which BTW is a source of
trouble on its own as you noticed.
What trouble in patch 1 do you mean? I may miss something.
I agree that patch 1 is not necessary if we decide to use sysdata or rework
root bus scanning to move domain to host bridge. Nevertheless, patch 1 is
still a cleanup IMO.
In this case, getting the domain should be trivial since the ACPI
companion on parent is already set, this should work
int acpi_pci_bus_domain_nr(struct device *parent)
{
struct acpi_device *acpi_dev = to_acpi_device(parent);
struct acpi_pci_root *root = acpi_dev->driver_data;
return root->segment;
}
Or am I missing something here?
Well, I thought that the whole idea behind this exercise was to move
the domain number into struct pci_host_bridge (Arnd did not do it with
his first set but this does not mean we can't add it as Bjorn suggested),
so that the domain number could be read from there straight away in an
arch (and FW) independent manner, right ?
The original issue was using _SEG call again instead of using
the value from acpi_pci_root, and the solution for that problem
is very simple.
The pci host bridge work is of course very useful, but creating a
dependency with that work for an issue that can be so easily solved
is unnecessary.
JC.
From: Lorenzo Pieralisi <hidden> Date: 2016-05-03 14:55:15
On Tue, May 03, 2016 at 07:52:52PM +0530, Jayachandran C wrote:
On Tue, May 3, 2016 at 4:32 PM, Lorenzo Pieralisi
[off-list ref] wrote:
quoted
On Mon, May 02, 2016 at 06:56:13PM +0530, Jayachandran C wrote:
quoted
On Mon, May 2, 2016 at 6:13 PM, Tomasz Nowicki [off-list ref] wrote:
quoted
On 04/27/2016 01:17 PM, Lorenzo Pieralisi wrote:
quoted
On Tue, Apr 26, 2016 at 09:26:49PM -0500, Bjorn Helgaas wrote:
quoted
On Fri, Apr 15, 2016 at 07:06:37PM +0200, Tomasz Nowicki wrote:
[...]
quoted
quoted
+int acpi_pci_bus_domain_nr(struct device *parent)
+{
+ struct acpi_device *acpi_dev = to_acpi_device(parent);
+ unsigned long long segment = 0;
+ acpi_status status;
+
+ /*
+ * If _SEG method does not exist, following ACPI spec (6.5.6)
+ * all PCI buses belong to domain 0.
+ */
+ status = acpi_evaluate_integer(acpi_dev->handle,
METHOD_NAME__SEG, NULL,
+ &segment);
We already have code in acpi_pci_root_add() to evaluate _SEG. We
don't want to evaluate it *twice*, do we?
I was sort of expecting that if you added it here, we'd remove the
existing call, but it looks like you're keeping both?
We can't remove the existing call, since it is used on X86 and IA64
to store the segment number that, in the process, is used in their
pci_domain_nr() arch specific callback to retrieve the domain nr.
On ARM64, that selects PCI_DOMAINS_GENERIC, we have to find a way
to retrieve the domain number that is not arch dependent, since
this is generic code, we can't rely on any bus->sysdata format (unless
we do something like JC did below), therefore the only way is to call
the _SEG method *again* here, which also forced Tomasz to go through
the ACPI_COMPANION setting song and dance and pass the parent pointer
to pci_create_root_bus() (see patch 1), which BTW is a source of
trouble on its own as you noticed.
What trouble in patch 1 do you mean? I may miss something.
I agree that patch 1 is not necessary if we decide to use sysdata or rework
root bus scanning to move domain to host bridge. Nevertheless, patch 1 is
still a cleanup IMO.
In this case, getting the domain should be trivial since the ACPI
companion on parent is already set, this should work
int acpi_pci_bus_domain_nr(struct device *parent)
{
struct acpi_device *acpi_dev = to_acpi_device(parent);
struct acpi_pci_root *root = acpi_dev->driver_data;
return root->segment;
}
Or am I missing something here?
Well, I thought that the whole idea behind this exercise was to move
the domain number into struct pci_host_bridge (Arnd did not do it with
his first set but this does not mean we can't add it as Bjorn suggested),
so that the domain number could be read from there straight away in an
arch (and FW) independent manner, right ?
The original issue was using _SEG call again instead of using
the value from acpi_pci_root, and the solution for that problem
is very simple.
The pci host bridge work is of course very useful, but creating a
dependency with that work for an issue that can be so easily solved
is unnecessary.
It can be easily solved if we do not drop patch 1 (I understood the
additional call to _SEG was only part of the problem). If we keep patch
1 the code above is fine by me, if we have to drop patch 1 (IIRC by
passing the parent pointer to pci_create_root_bus() we are affecting
IA64 and X86 code, if that's acceptable that's fine by me) I do not
think we can use the code above anymore, that's what my comment was
all about because Bjorn was concerned about the fragility of the code
setting the ACPI companion.
Lorenzo
From: Tomasz Nowicki <hidden> Date: 2016-04-27 11:59:58
On 27.04.2016 04:26, Bjorn Helgaas wrote:
On Fri, Apr 15, 2016 at 07:06:37PM +0200, Tomasz Nowicki wrote:
quoted
As we now have valid PCI host bridge device reference we can
introduce code that is going to find its bus domain number using
ACPI _SEG method.
Note that _SEG method is optional, therefore _SEG absence means
that all PCI buses belong to domain 0.
While at it, for the sake of code clarity we put ACPI and DT domain
assign methods into the corresponding helpers.
Signed-off-by: Tomasz Nowicki <redacted>
Reviewed-by: Liviu Dudau <Liviu.Dudau@arm.com>
Tested-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
Tested-by: Jeremy Linton <redacted>
Tested-by: Duc Dang <redacted>
Tested-by: Dongdong Liu <redacted>
Tested-by: Hanjun Guo <redacted>
Tested-by: Graeme Gregory <redacted>
Tested-by: Sinan Kaya <redacted>
---
drivers/acpi/pci_root.c | 18 ++++++++++++++++++
drivers/pci/pci.c | 11 +++++++++--
include/linux/pci-acpi.h | 2 ++
3 files changed, 29 insertions(+), 2 deletions(-)
We already have code in acpi_pci_root_add() to evaluate _SEG. We
don't want to evaluate it *twice*, do we?
Ideally we do not want.
The main intention here was to avoid using "void *sysdata" to retrieve
domain number. sysdata means something different for each architectures.
This would be common way for all arch using PCI_DOMAINS_GENERIC option
such as ARM64.
I was sort of expecting that if you added it here, we'd remove the
existing call, but it looks like you're keeping both?
I leave _SEG evaluation in acpi_pci_root_add to keep support for IA64
and x86 which are still using arch-specific sysdata (struct
pci_sysdata->domain for x86 and struct pci_controller->segment for IA64)
to retrieve domain number.
ARM64 uses PCI_DOMAINS_GENERIC for DT boot method, it would be
consistent to keep it for ACPI too.
I am open to suggestions, do you think we should use sysdata for ARM64?
Thanks,
Tomasz
From: Tomasz Nowicki <hidden> Date: 2016-04-15 17:07:26
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 <redacted>
Signed-off-by: Sinan Kaya <redacted>
Signed-off-by: Tomasz Nowicki <redacted>
---
drivers/acpi/pci_root.c | 33 +++++++++++++++++++++++++++++++++
drivers/pci/pci.c | 24 ++++++++++++++++++++++++
include/linux/pci.h | 1 +
3 files changed, 58 insertions(+)
@@ -742,6 +742,34 @@ next:resource_list_add_tail(entry,resources);}}+staticvoidacpi_pci_root_remap_iospace(structresource_entry*entry)+{+#ifdef PCI_IOBASE+structresource*res=entry->res;+resource_size_tcpu_addr=res->start;+resource_size_tpci_addr=cpu_addr-entry->offset;+resource_size_tlength=resource_size(res);+unsignedlongport;++if(pci_register_io_range(cpu_addr,length))+gotoerr;++port=pci_address_to_pio(cpu_addr);+if(port==(unsignedlong)-1)+gotoerr;++res->start=port;+res->end=port+length-1;+entry->offset=port-pci_addr;++if(pci_remap_iospace(res,cpu_addr)<0)+gotoerr;+pr_info("Remapped I/O %pa to %pR\n",&cpu_addr,res);+return;+err:+res->flags|=IORESOURCE_DISABLED;+#endif+}intacpi_pci_probe_root_resources(structacpi_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
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.
quoted hunk
Signed-off-by: Jayachandran C <redacted>
Signed-off-by: Sinan Kaya <redacted>
Signed-off-by: Tomasz Nowicki <redacted>
---
drivers/acpi/pci_root.c | 33 +++++++++++++++++++++++++++++++++
drivers/pci/pci.c | 24 ++++++++++++++++++++++++
include/linux/pci.h | 1 +
3 files changed, 58 insertions(+)
@@ -742,6 +742,34 @@ next:resource_list_add_tail(entry,resources);}}+staticvoidacpi_pci_root_remap_iospace(structresource_entry*entry)+{+#ifdef PCI_IOBASE+structresource*res=entry->res;+resource_size_tcpu_addr=res->start;+resource_size_tpci_addr=cpu_addr-entry->offset;+resource_size_tlength=resource_size(res);+unsignedlongport;++if(pci_register_io_range(cpu_addr,length))+gotoerr;++port=pci_address_to_pio(cpu_addr);+if(port==(unsignedlong)-1)+gotoerr;++res->start=port;+res->end=port+length-1;+entry->offset=port-pci_addr;++if(pci_remap_iospace(res,cpu_addr)<0)+gotoerr;+pr_info("Remapped I/O %pa to %pR\n",&cpu_addr,res);+return;+err:+res->flags|=IORESOURCE_DISABLED;+#endif+}intacpi_pci_probe_root_resources(structacpi_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
From: Jon Masters <hidden> Date: 2016-04-27 05:36:39
On 04/26/2016 10:39 PM, Bjorn Helgaas wrote:
On Fri, Apr 15, 2016 at 07:06:40PM +0200, Tomasz Nowicki wrote:
quoted
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
From: Jon Masters <hidden> Date: 2016-04-28 21:54:06
On 04/27/2016 01:36 AM, Jon Masters wrote:
On 04/26/2016 10:39 PM, Bjorn Helgaas wrote:
quoted
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.
Just a footnote on the IA64 thing. I'm working on getting access to a
few Itanium systems and running V6 on these (even bought my first
Itanium system today so I can run this at home also). I'm also chatting
with the internal RH QE folks about testing on a few dozen x86 systems.
Will followup when we've any results on that testing.
Jon.
--
Computer Architect | Sent from my Fedora powered laptop
From: Lorenzo Pieralisi <hidden> Date: 2016-04-27 14:27:07
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:
quoted
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 ?
quoted
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
quoted
Signed-off-by: Jayachandran C <redacted>
Signed-off-by: Sinan Kaya <redacted>
Signed-off-by: Tomasz Nowicki <redacted>
---
drivers/acpi/pci_root.c | 33 +++++++++++++++++++++++++++++++++
drivers/pci/pci.c | 24 ++++++++++++++++++++++++
include/linux/pci.h | 1 +
3 files changed, 58 insertions(+)
@@ -742,6 +742,34 @@ next:resource_list_add_tail(entry,resources);}}+staticvoidacpi_pci_root_remap_iospace(structresource_entry*entry)+{+#ifdef PCI_IOBASE+structresource*res=entry->res;+resource_size_tcpu_addr=res->start;+resource_size_tpci_addr=cpu_addr-entry->offset;+resource_size_tlength=resource_size(res);+unsignedlongport;++if(pci_register_io_range(cpu_addr,length))+gotoerr;++port=pci_address_to_pio(cpu_addr);+if(port==(unsignedlong)-1)+gotoerr;++res->start=port;+res->end=port+length-1;+entry->offset=port-pci_addr;++if(pci_remap_iospace(res,cpu_addr)<0)+gotoerr;+pr_info("Remapped I/O %pa to %pR\n",&cpu_addr,res);+return;+err:+res->flags|=IORESOURCE_DISABLED;+#endif+}intacpi_pci_probe_root_resources(structacpi_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
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:
quoted
On Fri, Apr 15, 2016 at 07:06:40PM +0200, Tomasz Nowicki wrote:
quoted
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 ?
quoted
quoted
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
quoted
quoted
Signed-off-by: Jayachandran C <redacted>
Signed-off-by: Sinan Kaya <redacted>
Signed-off-by: Tomasz Nowicki <redacted>
---
drivers/acpi/pci_root.c | 33 +++++++++++++++++++++++++++++++++
drivers/pci/pci.c | 24 ++++++++++++++++++++++++
include/linux/pci.h | 1 +
3 files changed, 58 insertions(+)
@@ -742,6 +742,34 @@ next:resource_list_add_tail(entry,resources);}}+staticvoidacpi_pci_root_remap_iospace(structresource_entry*entry)+{+#ifdef PCI_IOBASE+structresource*res=entry->res;+resource_size_tcpu_addr=res->start;+resource_size_tpci_addr=cpu_addr-entry->offset;+resource_size_tlength=resource_size(res);+unsignedlongport;++if(pci_register_io_range(cpu_addr,length))+gotoerr;++port=pci_address_to_pio(cpu_addr);+if(port==(unsignedlong)-1)+gotoerr;++res->start=port;+res->end=port+length-1;+entry->offset=port-pci_addr;++if(pci_remap_iospace(res,cpu_addr)<0)+gotoerr;+pr_info("Remapped I/O %pa to %pR\n",&cpu_addr,res);+return;+err:+res->flags|=IORESOURCE_DISABLED;+#endif+}intacpi_pci_probe_root_resources(structacpi_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
From: Lorenzo Pieralisi <hidden> Date: 2016-04-27 16:09:31
On Wed, Apr 27, 2016 at 04:10:36PM +0100, Liviu.Dudau at arm.com wrote:
On Wed, Apr 27, 2016 at 03:26:59PM +0100, Lorenzo Pieralisi wrote:
quoted
On Tue, Apr 26, 2016 at 09:39:16PM -0500, Bjorn Helgaas wrote:
quoted
On Fri, Apr 15, 2016 at 07:06:40PM +0200, Tomasz Nowicki wrote:
quoted
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
quoted
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 ?
quoted
quoted
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
quoted
quoted
Signed-off-by: Jayachandran C <redacted>
Signed-off-by: Sinan Kaya <redacted>
Signed-off-by: Tomasz Nowicki <redacted>
---
drivers/acpi/pci_root.c | 33 +++++++++++++++++++++++++++++++++
drivers/pci/pci.c | 24 ++++++++++++++++++++++++
include/linux/pci.h | 1 +
3 files changed, 58 insertions(+)
@@ -742,6 +742,34 @@ next:resource_list_add_tail(entry,resources);}}+staticvoidacpi_pci_root_remap_iospace(structresource_entry*entry)+{+#ifdef PCI_IOBASE+structresource*res=entry->res;+resource_size_tcpu_addr=res->start;+resource_size_tpci_addr=cpu_addr-entry->offset;+resource_size_tlength=resource_size(res);+unsignedlongport;++if(pci_register_io_range(cpu_addr,length))+gotoerr;++port=pci_address_to_pio(cpu_addr);+if(port==(unsignedlong)-1)+gotoerr;++res->start=port;+res->end=port+length-1;+entry->offset=port-pci_addr;++if(pci_remap_iospace(res,cpu_addr)<0)+gotoerr;+pr_info("Remapped I/O %pa to %pR\n",&cpu_addr,res);+return;+err:+res->flags|=IORESOURCE_DISABLED;+#endif+}intacpi_pci_probe_root_resources(structacpi_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
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:
quoted
On Fri, Apr 15, 2016 at 07:06:40PM +0200, Tomasz Nowicki wrote:
quoted
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.
Mapping IO port space into MMIO space is pretty common since most
arches don't have inb/outb instructions like x86 does. Several arches
(at least ia64 and parisc) have some sort of sparse mapping, but my
point is that the "dense" mapping (one byte MMIO space per IO port) is
pretty similar across arches.
There are differences in how we compute the MMIO address from the IO
port number, of course, e.g., on arm64 the CPU virtual MMIO address is
a simple offset from the IO port number, i.e., "vaddr = PCI_IOBASE +
res->start" in pci_remap_iospace(), while on ia64, that CPU address is
less constrained, i.e., "vaddr = space->mmio_base | port" in
__ia64_mk_io_addr().
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.
OK. That's confusing to read, but I see that it probably works.
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 ?
There's a lot of non-arch-specific stuff here, which is what's
bothering me. The arch-specific parts are:
- discovering IO port region (bus address start and size)
- discovering MMIO mapping address and size (CPU "mem" resource)
- discovering or assigning IO port base (CPU "io" resource)
- setting up whatever structures arch uses to implement inb()
Once you have the CPU "mem" and "io" resources, the code to insert
them into iomem_resource and ioport_resource and ioremap() the MMIO
space should be pretty generic. Right now that code is scattered
through the arches and most of them don't do it correctly, e.g.,
typically they don't request the "mem" resource.
From: Tomasz Nowicki <hidden> Date: 2016-04-15 17:07:31
No functional changes in this patch.
PCI I/O space mapping code does not depend on OF, therefore it can be
moved to PCI core code. This way we will be able to use it
e.g. in ACPI PCI code.
Suggested-by: Lorenzo Pieralisi <redacted>
Signed-off-by: Tomasz Nowicki <redacted>
CC: Arnd Bergmann <arnd@arndb.de>
CC: Liviu Dudau <Liviu.Dudau@arm.com>
CC: Lorenzo Pieralisi <redacted>
---
drivers/of/address.c | 116 +--------------------------------------------
drivers/pci/pci.c | 115 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/of_address.h | 9 ----
include/linux/pci.h | 5 ++
4 files changed, 121 insertions(+), 124 deletions(-)
@@ -673,121 +674,6 @@ const __be32 *of_get_address(struct device_node *dev, int index, u64 *size,}EXPORT_SYMBOL(of_get_address);-#ifdef PCI_IOBASE-structio_range{-structlist_headlist;-phys_addr_tstart;-resource_size_tsize;-};--staticLIST_HEAD(io_range_list);-staticDEFINE_SPINLOCK(io_range_lock);-#endif--/*-*RecordthePCIIOrange(expressedasCPUphysicaladdress+size).-*Returnanegativevalueifanerrorhasoccured,zerootherwise-*/-int__weakpci_register_io_range(phys_addr_taddr,resource_size_tsize)-{-interr=0;--#ifdef PCI_IOBASE-structio_range*range;-resource_size_tallocated_size=0;--/* check if the range hasn't been previously recorded */-spin_lock(&io_range_lock);-list_for_each_entry(range,&io_range_list,list){-if(addr>=range->start&&addr+size<=range->start+size){-/* range already registered, bail out */-gotoend_register;-}-allocated_size+=range->size;-}--/* range not registed yet, check for available space */-if(allocated_size+size-1>IO_SPACE_LIMIT){-/* if it's too big check if 64K space can be reserved */-if(allocated_size+SZ_64K-1>IO_SPACE_LIMIT){-err=-E2BIG;-gotoend_register;-}--size=SZ_64K;-pr_warn("Requested IO range too big, new size set to 64K\n");-}--/* add the range to the list */-range=kzalloc(sizeof(*range),GFP_ATOMIC);-if(!range){-err=-ENOMEM;-gotoend_register;-}--range->start=addr;-range->size=size;--list_add_tail(&range->list,&io_range_list);--end_register:-spin_unlock(&io_range_lock);-#endif--returnerr;-}--phys_addr_tpci_pio_to_address(unsignedlongpio)-{-phys_addr_taddress=(phys_addr_t)OF_BAD_ADDR;--#ifdef PCI_IOBASE-structio_range*range;-resource_size_tallocated_size=0;--if(pio>IO_SPACE_LIMIT)-returnaddress;--spin_lock(&io_range_lock);-list_for_each_entry(range,&io_range_list,list){-if(pio>=allocated_size&&pio<allocated_size+range->size){-address=range->start+pio-allocated_size;-break;-}-allocated_size+=range->size;-}-spin_unlock(&io_range_lock);-#endif--returnaddress;-}--unsignedlong__weakpci_address_to_pio(phys_addr_taddress)-{-#ifdef PCI_IOBASE-structio_range*res;-resource_size_toffset=0;-unsignedlongaddr=-1;--spin_lock(&io_range_lock);-list_for_each_entry(res,&io_range_list,list){-if(address>=res->start&&address<res->start+res->size){-addr=address-res->start+offset;-break;-}-offset+=res->size;-}-spin_unlock(&io_range_lock);--returnaddr;-#else-if(address>IO_SPACE_LIMIT)-return(unsignedlong)-1;--return(unsignedlong)address;-#endif-}-staticint__of_address_to_resource(structdevice_node*dev,const__be32*addrp,u64size,unsignedintflags,constchar*name,structresource*r)
@@ -3022,6 +3022,121 @@ int pci_request_regions_exclusive(struct pci_dev *pdev, const char *res_name)}EXPORT_SYMBOL(pci_request_regions_exclusive);+#ifdef PCI_IOBASE+structio_range{+structlist_headlist;+phys_addr_tstart;+resource_size_tsize;+};++staticLIST_HEAD(io_range_list);+staticDEFINE_SPINLOCK(io_range_lock);+#endif++/*+*RecordthePCIIOrange(expressedasCPUphysicaladdress+size).+*Returnanegativevalueifanerrorhasoccured,zerootherwise+*/+int__weakpci_register_io_range(phys_addr_taddr,resource_size_tsize)+{+interr=0;++#ifdef PCI_IOBASE+structio_range*range;+resource_size_tallocated_size=0;++/* check if the range hasn't been previously recorded */+spin_lock(&io_range_lock);+list_for_each_entry(range,&io_range_list,list){+if(addr>=range->start&&addr+size<=range->start+size){+/* range already registered, bail out */+gotoend_register;+}+allocated_size+=range->size;+}++/* range not registed yet, check for available space */+if(allocated_size+size-1>IO_SPACE_LIMIT){+/* if it's too big check if 64K space can be reserved */+if(allocated_size+SZ_64K-1>IO_SPACE_LIMIT){+err=-E2BIG;+gotoend_register;+}++size=SZ_64K;+pr_warn("Requested IO range too big, new size set to 64K\n");+}++/* add the range to the list */+range=kzalloc(sizeof(*range),GFP_ATOMIC);+if(!range){+err=-ENOMEM;+gotoend_register;+}++range->start=addr;+range->size=size;++list_add_tail(&range->list,&io_range_list);++end_register:+spin_unlock(&io_range_lock);+#endif++returnerr;+}++phys_addr_tpci_pio_to_address(unsignedlongpio)+{+phys_addr_taddress=(phys_addr_t)OF_BAD_ADDR;++#ifdef PCI_IOBASE+structio_range*range;+resource_size_tallocated_size=0;++if(pio>IO_SPACE_LIMIT)+returnaddress;++spin_lock(&io_range_lock);+list_for_each_entry(range,&io_range_list,list){+if(pio>=allocated_size&&pio<allocated_size+range->size){+address=range->start+pio-allocated_size;+break;+}+allocated_size+=range->size;+}+spin_unlock(&io_range_lock);+#endif++returnaddress;+}++unsignedlong__weakpci_address_to_pio(phys_addr_taddress)+{+#ifdef PCI_IOBASE+structio_range*res;+resource_size_toffset=0;+unsignedlongaddr=-1;++spin_lock(&io_range_lock);+list_for_each_entry(res,&io_range_list,list){+if(address>=res->start&&address<res->start+res->size){+addr=address-res->start+offset;+break;+}+offset+=res->size;+}+spin_unlock(&io_range_lock);++returnaddr;+#else+if(address>IO_SPACE_LIMIT)+return(unsignedlong)-1;++return(unsignedlong)address;+#endif+}+/***pci_remap_iospace-RemapthememorymappedI/Ospace*@res:ResourcedescribingtheI/Ospace
From: Tomasz Nowicki <hidden> Date: 2016-04-15 17:07:41
This patch is going to implement generic PCI host controller for
ACPI world, similar to what pci-host-generic.c driver does for DT world.
All such drivers, which we have seen so far, were implemented within
arch/ directory since they had some arch assumptions (x86 and ia64).
However, they all are doing similar thing, so it makes sense to find
some common code and abstract it into the generic driver.
In order to handle PCI config space regions properly, we define new
MCFG interface which parses MCFG table and keep its entries
in a list. New pci_mcfg_init call is defined so that we do not depend
on PCI_MMCONFIG. Regions are not mapped until host bridge ask for it.
The implementation of pci_acpi_scan_root() looks up the saved MCFG entries
and sets up a new mapping. Generic PCI functions are used for
accessing config space. Driver selects PCI_GENERIC_ECAM and uses functions
from drivers/pci/ecam.h to create and access ECAM mappings.
As mentioned in Kconfig help section, ACPI_PCI_HOST_GENERIC choice
should be made on a per-architecture basis.
This patch is heavily based on the updated version from Jayachandran C:
https://lkml.org/lkml/2016/4/11/908
git: https://github.com/jchandra-brcm/linux/ (arm64-acpi-pci-v3)
Signed-off-by: Tomasz Nowicki <redacted>
Signed-off-by: Jayachandran C <redacted>
---
drivers/acpi/Kconfig | 8 ++
drivers/acpi/Makefile | 1 +
drivers/acpi/bus.c | 1 +
drivers/acpi/pci_gen_host.c | 231 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/pci.h | 6 ++
5 files changed, 247 insertions(+)
create mode 100644 drivers/acpi/pci_gen_host.c
@@ -0,0 +1,231 @@+/*+*Thisprogramisfreesoftware;youcanredistributeitand/ormodify+*itunderthetermsoftheGNUGeneralPublicLicense,version2,as+*publishedbytheFreeSoftwareFoundation(the"GPL").+*+*Thisprogramisdistributedinthehopethatitwillbeuseful,but+*WITHOUTANYWARRANTY;withouteventheimpliedwarrantyof+*MERCHANTABILITYorFITNESSFORAPARTICULARPURPOSE.SeetheGNU+*GeneralPublicLicenseversion2(GPLv2)formoredetails.+*+*YoushouldhavereceivedacopyoftheGNUGeneralPublicLicense+*version2(GPLv2)alongwiththissourcecode.+*/+#include<linux/kernel.h>+#include<linux/pci.h>+#include<linux/pci-acpi.h>+#include<linux/sfi_acpi.h>+#include<linux/slab.h>++#include"../pci/ecam.h"++#define PREFIX "ACPI: "++/* Structure to hold entries from the MCFG table */+structmcfg_entry{+structlist_headlist;+phys_addr_taddr;+u16segment;+u8bus_start;+u8bus_end;+};++/* List to save mcfg entries */+staticLIST_HEAD(pci_mcfg_list);+staticDEFINE_MUTEX(pci_mcfg_lock);++/* ACPI info for generic ACPI PCI controller */+structacpi_pci_generic_root_info{+structacpi_pci_root_infocommon;+structpci_config_window*cfg;/* config space mapping */+};++/* Find the entry in mcfg list which contains range bus_start */+staticstructmcfg_entry*pci_mcfg_lookup(u16seg,u8bus_start)+{+structmcfg_entry*e;++list_for_each_entry(e,&pci_mcfg_list,list){+if(e->segment==seg&&+e->bus_start<=bus_start&&bus_start<=e->bus_end)+returne;+}++returnNULL;+}+++/*+*LookupthebusrangeforthedomaininMCFG,andsetupconfigspace+*mapping.+*/+staticintpci_acpi_setup_ecam_mapping(structacpi_pci_root*root,+structacpi_pci_generic_root_info*ri)+{+u16seg=root->segment;+u8bus_start=root->secondary.start;+u8bus_end=root->secondary.end;+structpci_config_window*cfg;+structmcfg_entry*e;+phys_addr_taddr;+interr=0;++mutex_lock(&pci_mcfg_lock);+e=pci_mcfg_lookup(seg,bus_start);+if(!e){+addr=acpi_pci_root_get_mcfg_addr(root->device->handle);+if(addr==0){+pr_err(PREFIX"%04x:%02x-%02x bus range error\n",+seg,bus_start,bus_end);+err=-ENOENT;+gotoerr_out;+}+}else{+if(bus_start!=e->bus_start){+pr_err("%04x:%02x-%02x bus range mismatch %02x\n",+seg,bus_start,bus_end,e->bus_start);+err=-EINVAL;+gotoerr_out;+}elseif(bus_end!=e->bus_end){+pr_warn("%04x:%02x-%02x bus end mismatch %02x\n",+seg,bus_start,bus_end,e->bus_end);+bus_end=min(bus_end,e->bus_end);+}+addr=e->addr;+}++cfg=pci_generic_ecam_create(&root->device->dev,addr,bus_start,+bus_end,&pci_generic_ecam_default_ops);+if(IS_ERR(cfg)){+err=PTR_ERR(cfg);+pr_err("%04x:%02x-%02x error %d mapping CAM\n",seg,+bus_start,bus_end,err);+gotoerr_out;+}++cfg->domain=seg;+ri->cfg=cfg;+err_out:+mutex_unlock(&pci_mcfg_lock);+returnerr;+}++/* release_info: free resrouces allocated by init_info */+staticvoidpci_acpi_generic_release_info(structacpi_pci_root_info*ci)+{+structacpi_pci_generic_root_info*ri;++ri=container_of(ci,structacpi_pci_generic_root_info,common);+pci_generic_ecam_free(ri->cfg);+kfree(ri);+}++staticstructacpi_pci_root_opsacpi_pci_root_ops={+.release_info=pci_acpi_generic_release_info,+};++/* Interface called from ACPI code to setup PCI host controller */+structpci_bus*pci_acpi_scan_root(structacpi_pci_root*root)+{+intnode=acpi_get_node(root->device->handle);+structacpi_pci_generic_root_info*ri;+structpci_bus*bus,*child;+interr;++ri=kzalloc_node(sizeof(*ri),GFP_KERNEL,node);+if(!ri)+returnNULL;++err=pci_acpi_setup_ecam_mapping(root,ri);+if(err)+returnNULL;++acpi_pci_root_ops.pci_ops=&ri->cfg->ops->pci_ops;+bus=acpi_pci_root_create(root,&acpi_pci_root_ops,&ri->common,+ri->cfg);+if(!bus)+returnNULL;++pci_bus_size_bridges(bus);+pci_bus_assign_resources(bus);++list_for_each_entry(child,&bus->children,node)+pcie_bus_configure_settings(child);++returnbus;+}++/* handle MCFG table entries */+static__initintpci_mcfg_parse(structacpi_table_header*header)+{+structacpi_table_mcfg*mcfg;+structacpi_mcfg_allocation*mptr;+structmcfg_entry*e,*arr;+inti,n;++if(!header)+return-EINVAL;++mcfg=(structacpi_table_mcfg*)header;+mptr=(structacpi_mcfg_allocation*)&mcfg[1];+n=(header->length-sizeof(*mcfg))/sizeof(*mptr);+if(n<=0||n>255){+pr_err(PREFIX" MCFG has incorrect entries (%d).\n",n);+return-EINVAL;+}++arr=kcalloc(n,sizeof(*arr),GFP_KERNEL);+if(!arr)+return-ENOMEM;++for(i=0,e=arr;i<n;i++,mptr++,e++){+e->segment=mptr->pci_segment;+e->addr=mptr->address;+e->bus_start=mptr->start_bus_number;+e->bus_end=mptr->end_bus_number;+list_add(&e->list,&pci_mcfg_list);+pr_info(PREFIX+"MCFG entry for domain %04x [bus %02x-%02x] (base %pa)\n",+e->segment,e->bus_start,e->bus_end,&e->addr);+}++return0;+}++/* Interface called by ACPI - parse and save MCFG table */+void__initpci_mcfg_init(void)+{+interr=acpi_table_parse(ACPI_SIG_MCFG,pci_mcfg_parse);+if(err)+pr_err(PREFIX"Failed to parse MCFG (%d)\n",err);+elseif(list_empty(&pci_mcfg_list))+pr_info(PREFIX"No valid entries in MCFG table.\n");+else{+structmcfg_entry*e;+inti=0;+list_for_each_entry(e,&pci_mcfg_list,list)+i++;+pr_info(PREFIX"MCFG table loaded, %d entries\n",i);+}+}++/* Raw operations, works only for MCFG entries with an associated bus */+intraw_pci_read(unsignedintdomain,unsignedintbusn,unsignedintdevfn,+intreg,intlen,u32*val)+{+structpci_bus*bus=pci_find_bus(domain,busn);++if(!bus)+returnPCIBIOS_DEVICE_NOT_FOUND;+returnbus->ops->read(bus,devfn,reg,len,val);+}++intraw_pci_write(unsignedintdomain,unsignedintbusn,unsignedintdevfn,+intreg,intlen,u32val)+{+structpci_bus*bus=pci_find_bus(domain,busn);++if(!bus)+returnPCIBIOS_DEVICE_NOT_FOUND;+returnbus->ops->write(bus,devfn,reg,len,val);+}
From: Jayachandran C <hidden> Date: 2016-04-20 19:13:06
On Fri, Apr 15, 2016 at 10:36 PM, Tomasz Nowicki [off-list ref] wrote:
This patch is going to implement generic PCI host controller for
ACPI world, similar to what pci-host-generic.c driver does for DT world.
All such drivers, which we have seen so far, were implemented within
arch/ directory since they had some arch assumptions (x86 and ia64).
However, they all are doing similar thing, so it makes sense to find
some common code and abstract it into the generic driver.
In order to handle PCI config space regions properly, we define new
MCFG interface which parses MCFG table and keep its entries
in a list. New pci_mcfg_init call is defined so that we do not depend
on PCI_MMCONFIG. Regions are not mapped until host bridge ask for it.
The implementation of pci_acpi_scan_root() looks up the saved MCFG entries
and sets up a new mapping. Generic PCI functions are used for
accessing config space. Driver selects PCI_GENERIC_ECAM and uses functions
from drivers/pci/ecam.h to create and access ECAM mappings.
As mentioned in Kconfig help section, ACPI_PCI_HOST_GENERIC choice
should be made on a per-architecture basis.
This patch is heavily based on the updated version from Jayachandran C:
https://lkml.org/lkml/2016/4/11/908
git: https://github.com/jchandra-brcm/linux/ (arm64-acpi-pci-v3)
This is a little bit unusual because I had not posted the v3 patch
to the mailing list yet, but you posted a variant of it The git repository
should not be in the commit comment because it is a temporary location.
There are some changes here I don't agree with. I think it will be
better if you can post a version without the quirk handling and with
some of the suggestions below.
You seem to have removed the copyright line, this is not proper, you
should probably add your copyright line if you think your changes are
significant.
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, version 2, as
+ * published by the Free Software Foundation (the "GPL").
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License version 2 (GPLv2) for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * version 2 (GPLv2) along with this source code.
+ */
+#include <linux/kernel.h>
+#include <linux/pci.h>
+#include <linux/pci-acpi.h>
+#include <linux/sfi_acpi.h>
+#include <linux/slab.h>
+
+#include "../pci/ecam.h"
+
+#define PREFIX "ACPI: "
+
+/* Structure to hold entries from the MCFG table */
+struct mcfg_entry {
+ struct list_head list;
+ phys_addr_t addr;
+ u16 segment;
+ u8 bus_start;
+ u8 bus_end;
+};
+
+/* List to save mcfg entries */
+static LIST_HEAD(pci_mcfg_list);
+static DEFINE_MUTEX(pci_mcfg_lock);
There is no need to use a list or lock here, I had used an
array and that is sufficient since it is not modified after it
is filled initially.
+/* ACPI info for generic ACPI PCI controller */
+struct acpi_pci_generic_root_info {
+ struct acpi_pci_root_info common;
+ struct pci_config_window *cfg; /* config space mapping */
+};
+
+/* Find the entry in mcfg list which contains range bus_start */
+static struct mcfg_entry *pci_mcfg_lookup(u16 seg, u8 bus_start)
+{
+ struct mcfg_entry *e;
+
+ list_for_each_entry(e, &pci_mcfg_list, list) {
+ if (e->segment == seg &&
+ e->bus_start <= bus_start && bus_start <= e->bus_end)
+ return e;
+ }
+
+ return NULL;
+}
+
+
+/*
+ * Lookup the bus range for the domain in MCFG, and set up config space
+ * mapping.
+ */
+static int pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root,
+ struct acpi_pci_generic_root_info *ri)
+{
+ u16 seg = root->segment;
+ u8 bus_start = root->secondary.start;
+ u8 bus_end = root->secondary.end;
+ struct pci_config_window *cfg;
+ struct mcfg_entry *e;
+ phys_addr_t addr;
+ int err = 0;
+
+ mutex_lock(&pci_mcfg_lock);
+ e = pci_mcfg_lookup(seg, bus_start);
+ if (!e) {
+ addr = acpi_pci_root_get_mcfg_addr(root->device->handle);
The acpi_pci_root_get_mcfg_addr() is already called in pci_root.c, doing
it again here is unnecessary.
I think you can have a function to pick up addr, bus_start, bus_end given
a domain from either MCFG or using _CBA method, but I think that
should be done in pci_root.c in a separate patch.
You seem to have moved all the config space mapping to this
point. Intel seems to do the mapping when they read MCFG for the
entries, and I had followed that model, and that avoids having another
array/list to save the values.
From: Tomasz Nowicki <hidden> Date: 2016-04-21 09:07:17
On 20.04.2016 21:12, Jayachandran C wrote:
On Fri, Apr 15, 2016 at 10:36 PM, Tomasz Nowicki [off-list ref] wrote:
quoted
This patch is going to implement generic PCI host controller for
ACPI world, similar to what pci-host-generic.c driver does for DT world.
All such drivers, which we have seen so far, were implemented within
arch/ directory since they had some arch assumptions (x86 and ia64).
However, they all are doing similar thing, so it makes sense to find
some common code and abstract it into the generic driver.
In order to handle PCI config space regions properly, we define new
MCFG interface which parses MCFG table and keep its entries
in a list. New pci_mcfg_init call is defined so that we do not depend
on PCI_MMCONFIG. Regions are not mapped until host bridge ask for it.
The implementation of pci_acpi_scan_root() looks up the saved MCFG entries
and sets up a new mapping. Generic PCI functions are used for
accessing config space. Driver selects PCI_GENERIC_ECAM and uses functions
from drivers/pci/ecam.h to create and access ECAM mappings.
As mentioned in Kconfig help section, ACPI_PCI_HOST_GENERIC choice
should be made on a per-architecture basis.
This patch is heavily based on the updated version from Jayachandran C:
https://lkml.org/lkml/2016/4/11/908
git: https://github.com/jchandra-brcm/linux/ (arm64-acpi-pci-v3)
This is a little bit unusual because I had not posted the v3 patch
to the mailing list yet, but you posted a variant of it The git repository
should not be in the commit comment because it is a temporary location.
We all agree this too important for everybody to delay this series. So
main motivation is to keep all discussion&patches within one unified
series. I would like to finally find direction we need to go. Stating
another discussion based on my previous patch set v5 confused people,
they do no know who is driving this. Again, lets cooperate to move it
forward within one patch set.
I agree with you we need maintainers to join this discussion.
There are some changes here I don't agree with. I think it will be
better if you can post a version without the quirk handling and with
some of the suggestions below.
The next version will not have quirk handling part. Regarding your
comments, please see below.
You seem to have removed the copyright line, this is not proper, you
should probably add your copyright line if you think your changes are
significant.
I rather forgot to add copyright here, I will fix it.
quoted
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, version 2, as
+ * published by the Free Software Foundation (the "GPL").
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License version 2 (GPLv2) for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * version 2 (GPLv2) along with this source code.
+ */
+#include <linux/kernel.h>
+#include <linux/pci.h>
+#include <linux/pci-acpi.h>
+#include <linux/sfi_acpi.h>
+#include <linux/slab.h>
+
+#include "../pci/ecam.h"
+
+#define PREFIX "ACPI: "
+
+/* Structure to hold entries from the MCFG table */
+struct mcfg_entry {
+ struct list_head list;
+ phys_addr_t addr;
+ u16 segment;
+ u8 bus_start;
+ u8 bus_end;
+};
+
+/* List to save mcfg entries */
+static LIST_HEAD(pci_mcfg_list);
+static DEFINE_MUTEX(pci_mcfg_lock);
There is no need to use a list or lock here, I had used an
array and that is sufficient since it is not modified after it
is filled initially.
ACPI PCI driver supports hot plug/removal, I want to avoid races using
this lock. I decided to use list because I do ECAM mapping on demand.
See below for more details.
quoted
+/* ACPI info for generic ACPI PCI controller */
+struct acpi_pci_generic_root_info {
+ struct acpi_pci_root_info common;
+ struct pci_config_window *cfg; /* config space mapping */
+};
+
+/* Find the entry in mcfg list which contains range bus_start */
+static struct mcfg_entry *pci_mcfg_lookup(u16 seg, u8 bus_start)
+{
+ struct mcfg_entry *e;
+
+ list_for_each_entry(e, &pci_mcfg_list, list) {
+ if (e->segment == seg &&
+ e->bus_start <= bus_start && bus_start <= e->bus_end)
+ return e;
+ }
+
+ return NULL;
+}
+
+
+/*
+ * Lookup the bus range for the domain in MCFG, and set up config space
+ * mapping.
+ */
+static int pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root,
+ struct acpi_pci_generic_root_info *ri)
+{
+ u16 seg = root->segment;
+ u8 bus_start = root->secondary.start;
+ u8 bus_end = root->secondary.end;
+ struct pci_config_window *cfg;
+ struct mcfg_entry *e;
+ phys_addr_t addr;
+ int err = 0;
+
+ mutex_lock(&pci_mcfg_lock);
+ e = pci_mcfg_lookup(seg, bus_start);
+ if (!e) {
+ addr = acpi_pci_root_get_mcfg_addr(root->device->handle);
The acpi_pci_root_get_mcfg_addr() is already called in pci_root.c, doing
it again here is unnecessary.
I think you can have a function to pick up addr, bus_start, bus_end given
a domain from either MCFG or using _CBA method, but I think that
should be done in pci_root.c in a separate patch.
You seem to have moved all the config space mapping to this
point. Intel seems to do the mapping when they read MCFG for the
entries, and I had followed that model, and that avoids having another
array/list to save the values.
See:
https://lkml.org/lkml/2016/3/3/921
I agree with Bjorn here, we should map it whenever we need this instead
of mapping all MCFG entries just in case. Also, I see other advantages:
1. We can always use valid "dev" for pci_generic_ecam_create call, what
you can't do when you use pci_generic_ecam_create during MCFG parsing.
2. No need for special handling for entries coming from _CBA, the path
is the same for MCFG and _CBA. We do not have to remember that only _CBA
related entries have to be unmapped.
Here you already have an array which is also connected as a linked
list which is unnecessary.
The array is to avoid complicated error handling in case the allocation
for some of element would failed. I can rework this to allocate each
element separately or to use array but the code is simpler now. As
explained above (on demand mapping), I would like to keep list/array
with MCFG entries (I preferred list).
quoted
+ for (i = 0, e = arr; i < n; i++, mptr++, e++) {
+ e->segment = mptr->pci_segment;
+ e->addr = mptr->address;
+ e->bus_start = mptr->start_bus_number;
+ e->bus_end = mptr->end_bus_number;
+ list_add(&e->list, &pci_mcfg_list);
+ pr_info(PREFIX
+ "MCFG entry for domain %04x [bus %02x-%02x] (base %pa)\n",
+ e->segment, e->bus_start, e->bus_end, &e->addr);
+ }
+
+ return 0;
+}
+
+/* Interface called by ACPI - parse and save MCFG table */
+void __init pci_mcfg_init(void)
+{
+ int err = acpi_table_parse(ACPI_SIG_MCFG, pci_mcfg_parse);
+ if (err)
+ pr_err(PREFIX "Failed to parse MCFG (%d)\n", err);
+ else if (list_empty(&pci_mcfg_list))
+ pr_info(PREFIX "No valid entries in MCFG table.\n");
+ else {
+ struct mcfg_entry *e;
+ int i = 0;
+ list_for_each_entry(e, &pci_mcfg_list, list)
+ i++;
+ pr_info(PREFIX "MCFG table loaded, %d entries\n", i);
+ }
+}
+
+/* Raw operations, works only for MCFG entries with an associated bus */
+int raw_pci_read(unsigned int domain, unsigned int busn, unsigned int devfn,
+ int reg, int len, u32 *val)
+{
+ struct pci_bus *bus = pci_find_bus(domain, busn);
+
+ if (!bus)
+ return PCIBIOS_DEVICE_NOT_FOUND;
+ return bus->ops->read(bus, devfn, reg, len, val);
+}
+
+int raw_pci_write(unsigned int domain, unsigned int busn, unsigned int devfn,
+ int reg, int len, u32 val)
+{
+ struct pci_bus *bus = pci_find_bus(domain, busn);
+
+ if (!bus)
+ return PCIBIOS_DEVICE_NOT_FOUND;
+ return bus->ops->write(bus, devfn, reg, len, val);
+}
From: Jayachandran C <hidden> Date: 2016-04-22 12:49:57
On Thu, Apr 21, 2016 at 2:36 PM, Tomasz Nowicki [off-list ref] wrote:
On 20.04.2016 21:12, Jayachandran C wrote:
quoted
On Fri, Apr 15, 2016 at 10:36 PM, Tomasz Nowicki [off-list ref] wrote:
quoted
This patch is going to implement generic PCI host controller for
ACPI world, similar to what pci-host-generic.c driver does for DT world.
All such drivers, which we have seen so far, were implemented within
arch/ directory since they had some arch assumptions (x86 and ia64).
However, they all are doing similar thing, so it makes sense to find
some common code and abstract it into the generic driver.
In order to handle PCI config space regions properly, we define new
MCFG interface which parses MCFG table and keep its entries
in a list. New pci_mcfg_init call is defined so that we do not depend
on PCI_MMCONFIG. Regions are not mapped until host bridge ask for it.
The implementation of pci_acpi_scan_root() looks up the saved MCFG
entries
and sets up a new mapping. Generic PCI functions are used for
accessing config space. Driver selects PCI_GENERIC_ECAM and uses
functions
from drivers/pci/ecam.h to create and access ECAM mappings.
As mentioned in Kconfig help section, ACPI_PCI_HOST_GENERIC choice
should be made on a per-architecture basis.
This patch is heavily based on the updated version from Jayachandran C:
https://lkml.org/lkml/2016/4/11/908
git: https://github.com/jchandra-brcm/linux/ (arm64-acpi-pci-v3)
This is a little bit unusual because I had not posted the v3 patch
to the mailing list yet, but you posted a variant of it The git repository
should not be in the commit comment because it is a temporary location.
We all agree this too important for everybody to delay this series. So main
motivation is to keep all discussion&patches within one unified series. I
would like to finally find direction we need to go. Stating another
discussion based on my previous patch set v5 confused people, they do no
know who is driving this. Again, lets cooperate to move it forward within
one patch set.
I agree with you we need maintainers to join this discussion.
quoted
There are some changes here I don't agree with. I think it will be
better if you can post a version without the quirk handling and with
some of the suggestions below.
The next version will not have quirk handling part. Regarding your comments,
please see below.
in
the system. If you are unsure, say N.
+config ACPI_PCI_HOST_GENERIC
+ bool
+ select PCI_GENERIC_ECAM
+ help
+ Select this config option from the architecture Kconfig,
+ if it is preferred to enable ACPI PCI host controller driver
which
+ has no arch-specific assumptions.
+
config X86_PM_TIMER
bool "Power Management Timer Support" if EXPERT
depends on X86
You seem to have removed the copyright line, this is not proper, you
should probably add your copyright line if you think your changes are
significant.
I rather forgot to add copyright here, I will fix it.
quoted
quoted
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, version 2, as
+ * published by the Free Software Foundation (the "GPL").
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License version 2 (GPLv2) for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * version 2 (GPLv2) along with this source code.
+ */
+#include <linux/kernel.h>
+#include <linux/pci.h>
+#include <linux/pci-acpi.h>
+#include <linux/sfi_acpi.h>
+#include <linux/slab.h>
+
+#include "../pci/ecam.h"
+
+#define PREFIX "ACPI: "
+
+/* Structure to hold entries from the MCFG table */
+struct mcfg_entry {
+ struct list_head list;
+ phys_addr_t addr;
+ u16 segment;
+ u8 bus_start;
+ u8 bus_end;
+};
+
+/* List to save mcfg entries */
+static LIST_HEAD(pci_mcfg_list);
+static DEFINE_MUTEX(pci_mcfg_lock);
There is no need to use a list or lock here, I had used an
array and that is sufficient since it is not modified after it
is filled initially.
ACPI PCI driver supports hot plug/removal, I want to avoid races using this
lock. I decided to use list because I do ECAM mapping on demand. See below
for more details.
Yes, there is hotplug. but there is no change to the saved MCFG entries
after you create the array (or list). And, the ECAM mapping is on demand
but it also just does a lookup and does not modify the array/list, so the
locking is not needed.
There may be a locking issue for hotplug in raw_pci_read/write between
find_bus and bus->ops->read/write, but this does not solve that. My
expectation is that since both are ACPI originated this may not be
an issue.
quoted
quoted
+/* ACPI info for generic ACPI PCI controller */
+struct acpi_pci_generic_root_info {
+ struct acpi_pci_root_info common;
+ struct pci_config_window *cfg; /* config space mapping
*/
+};
+
+/* Find the entry in mcfg list which contains range bus_start */
+static struct mcfg_entry *pci_mcfg_lookup(u16 seg, u8 bus_start)
+{
+ struct mcfg_entry *e;
+
+ list_for_each_entry(e, &pci_mcfg_list, list) {
+ if (e->segment == seg &&
+ e->bus_start <= bus_start && bus_start <= e->bus_end)
+ return e;
+ }
+
+ return NULL;
+}
+
+
+/*
+ * Lookup the bus range for the domain in MCFG, and set up config space
+ * mapping.
+ */
+static int pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root,
+ struct acpi_pci_generic_root_info
*ri)
+{
+ u16 seg = root->segment;
+ u8 bus_start = root->secondary.start;
+ u8 bus_end = root->secondary.end;
+ struct pci_config_window *cfg;
+ struct mcfg_entry *e;
+ phys_addr_t addr;
+ int err = 0;
+
+ mutex_lock(&pci_mcfg_lock);
+ e = pci_mcfg_lookup(seg, bus_start);
+ if (!e) {
+ addr = acpi_pci_root_get_mcfg_addr(root->device->handle);
The acpi_pci_root_get_mcfg_addr() is already called in pci_root.c, doing
it again here is unnecessary.
There are multiple ways in which acpi_pci_root_add() tries get the
bus range and mcfg address for a root bus. I think Bjorn's suggestion
was to add looking up the MCFG there too, and I think it would be
better if it was done in a separate patchset.
quoted
I think you can have a function to pick up addr, bus_start, bus_end given
a domain from either MCFG or using _CBA method, but I think that
should be done in pci_root.c in a separate patch.
You seem to have moved all the config space mapping to this
point. Intel seems to do the mapping when they read MCFG for the
entries, and I had followed that model, and that avoids having another
array/list to save the values.
See:
https://lkml.org/lkml/2016/3/3/921
I agree with Bjorn here, we should map it whenever we need this instead of
mapping all MCFG entries just in case. Also, I see other advantages:
1. We can always use valid "dev" for pci_generic_ecam_create call, what you
can't do when you use pci_generic_ecam_create during MCFG parsing.
2. No need for special handling for entries coming from _CBA, the path is
the same for MCFG and _CBA. We do not have to remember that only _CBA
related entries have to be unmapped.
Here you already have an array which is also connected as a linked
list which is unnecessary.
The array is to avoid complicated error handling in case the allocation for
some of element would failed. I can rework this to allocate each element
separately or to use array but the code is simpler now. As explained above
(on demand mapping), I would like to keep list/array with MCFG entries (I
preferred list).
quoted
quoted
+ for (i = 0, e = arr; i < n; i++, mptr++, e++) {
+ e->segment = mptr->pci_segment;
+ e->addr = mptr->address;
+ e->bus_start = mptr->start_bus_number;
+ e->bus_end = mptr->end_bus_number;
+ list_add(&e->list, &pci_mcfg_list);
+ pr_info(PREFIX
+ "MCFG entry for domain %04x [bus %02x-%02x] (base
%pa)\n",
+ e->segment, e->bus_start, e->bus_end, &e->addr);
+ }
+
+ return 0;
+}
+
+/* Interface called by ACPI - parse and save MCFG table */
+void __init pci_mcfg_init(void)
+{
+ int err = acpi_table_parse(ACPI_SIG_MCFG, pci_mcfg_parse);
+ if (err)
+ pr_err(PREFIX "Failed to parse MCFG (%d)\n", err);
+ else if (list_empty(&pci_mcfg_list))
+ pr_info(PREFIX "No valid entries in MCFG table.\n");
+ else {
+ struct mcfg_entry *e;
+ int i = 0;
+ list_for_each_entry(e, &pci_mcfg_list, list)
+ i++;
+ pr_info(PREFIX "MCFG table loaded, %d entries\n", i);
+ }
+}
+
+/* Raw operations, works only for MCFG entries with an associated bus */
+int raw_pci_read(unsigned int domain, unsigned int busn, unsigned int
devfn,
+ int reg, int len, u32 *val)
+{
+ struct pci_bus *bus = pci_find_bus(domain, busn);
+
+ if (!bus)
+ return PCIBIOS_DEVICE_NOT_FOUND;
+ return bus->ops->read(bus, devfn, reg, len, val);
+}
+
+int raw_pci_write(unsigned int domain, unsigned int busn, unsigned int
devfn,
+ int reg, int len, u32 val)
+{
+ struct pci_bus *bus = pci_find_bus(domain, busn);
+
+ if (!bus)
+ return PCIBIOS_DEVICE_NOT_FOUND;
+ return bus->ops->write(bus, devfn, reg, len, val);
+}
This is a little bit unusual because I had not posted the v3 patch
to the mailing list yet, but you posted a variant of it The git
repository
should not be in the commit comment because it is a temporary location.
We all agree this too important for everybody to delay this series. So
main motivation is to keep all discussion&patches within one unified
series. I would like to finally find direction we need to go. Stating
another discussion based on my previous patch set v5 confused people,
they do no know who is driving this. Again, lets cooperate to move it
forward within one patch set.
We need one person in the driver's seat here for this patch series. I
believe the intention is that this is Tomasz, with others cooperating
and assisting. The previous alternative patch series did serve to cause
confusion, and worse, they made it look like the ARM vendors can't work
together. That ends. Right now. I've raised this individually with each
of you (and with all of the other vendors), as well as inside Linaro.
There will be one person driving this, and everyone else will help.
I agree with you we need maintainers to join this discussion.
Please. I really want to lend support to the sense of urgency of getting
something merged that provides the basic ACPI based ECAM functionality
on ARMv8 systems. Until that happens, there are vendors who are going to
have to delay various other activities around ARM server until it is
done (because of the lack of an upstream patch is a critical blocker -
this isn't embedded, and we don't work that way).
This is incredibly critical and central to the successful adoption of
larger ARMv8 servers over the next year or so, all of which will rely
upon PCIe, using ACPI. We therefore need all hands on deck, and all
vendors getting overwhelmingly excited about making this patch series a
success. As an aside, to help the engineering orgs within these vendors
understand how much I need motion on this thread, I'm buzzing in their
ears multiple times per week specifically on engagement on this thread.
We need this done. If Bjorn or anyone else needs something, tell us.
Jon.
--
Computer Architect | Sent from my Fedora powered laptop
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
Hey, I really kind of like this. I think this might work out well.
On Fri, Apr 15, 2016 at 07:06:44PM +0200, Tomasz Nowicki wrote:
quoted hunk
This patch is going to implement generic PCI host controller for
ACPI world, similar to what pci-host-generic.c driver does for DT world.
All such drivers, which we have seen so far, were implemented within
arch/ directory since they had some arch assumptions (x86 and ia64).
However, they all are doing similar thing, so it makes sense to find
some common code and abstract it into the generic driver.
In order to handle PCI config space regions properly, we define new
MCFG interface which parses MCFG table and keep its entries
in a list. New pci_mcfg_init call is defined so that we do not depend
on PCI_MMCONFIG. Regions are not mapped until host bridge ask for it.
The implementation of pci_acpi_scan_root() looks up the saved MCFG entries
and sets up a new mapping. Generic PCI functions are used for
accessing config space. Driver selects PCI_GENERIC_ECAM and uses functions
from drivers/pci/ecam.h to create and access ECAM mappings.
As mentioned in Kconfig help section, ACPI_PCI_HOST_GENERIC choice
should be made on a per-architecture basis.
This patch is heavily based on the updated version from Jayachandran C:
https://lkml.org/lkml/2016/4/11/908
git: https://github.com/jchandra-brcm/linux/ (arm64-acpi-pci-v3)
Signed-off-by: Tomasz Nowicki <redacted>
Signed-off-by: Jayachandran C <redacted>
---
drivers/acpi/Kconfig | 8 ++
drivers/acpi/Makefile | 1 +
drivers/acpi/bus.c | 1 +
drivers/acpi/pci_gen_host.c | 231 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/pci.h | 6 ++
5 files changed, 247 insertions(+)
create mode 100644 drivers/acpi/pci_gen_host.c
@@ -0,0 +1,231 @@+/*+*Thisprogramisfreesoftware;youcanredistributeitand/ormodify+*itunderthetermsoftheGNUGeneralPublicLicense,version2,as+*publishedbytheFreeSoftwareFoundation(the"GPL").+*+*Thisprogramisdistributedinthehopethatitwillbeuseful,but+*WITHOUTANYWARRANTY;withouteventheimpliedwarrantyof+*MERCHANTABILITYorFITNESSFORAPARTICULARPURPOSE.SeetheGNU+*GeneralPublicLicenseversion2(GPLv2)formoredetails.+*+*YoushouldhavereceivedacopyoftheGNUGeneralPublicLicense+*version2(GPLv2)alongwiththissourcecode.+*/+#include<linux/kernel.h>+#include<linux/pci.h>+#include<linux/pci-acpi.h>+#include<linux/sfi_acpi.h>+#include<linux/slab.h>++#include"../pci/ecam.h"++#define PREFIX "ACPI: "++/* Structure to hold entries from the MCFG table */+structmcfg_entry{+structlist_headlist;+phys_addr_taddr;+u16segment;+u8bus_start;+u8bus_end;+};++/* List to save mcfg entries */+staticLIST_HEAD(pci_mcfg_list);+staticDEFINE_MUTEX(pci_mcfg_lock);++/* ACPI info for generic ACPI PCI controller */+structacpi_pci_generic_root_info{+structacpi_pci_root_infocommon;+structpci_config_window*cfg;/* config space mapping */+};++/* Find the entry in mcfg list which contains range bus_start */+staticstructmcfg_entry*pci_mcfg_lookup(u16seg,u8bus_start)+{+structmcfg_entry*e;++list_for_each_entry(e,&pci_mcfg_list,list){+if(e->segment==seg&&+e->bus_start<=bus_start&&bus_start<=e->bus_end)+returne;+}++returnNULL;+}
Can you put the MCFG parsing, caching, and searching in a different
file, e.g., drivers/acpi/pci_mcfg.c?
+/*
+ * Lookup the bus range for the domain in MCFG, and set up config space
+ * mapping.
+ */
+static int pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root,
+ struct acpi_pci_generic_root_info *ri)
+{
+ u16 seg = root->segment;
+ u8 bus_start = root->secondary.start;
+ u8 bus_end = root->secondary.end;
+ struct pci_config_window *cfg;
+ struct mcfg_entry *e;
+ phys_addr_t addr;
+ int err = 0;
+
+ mutex_lock(&pci_mcfg_lock);
What does this lock protect? The pci_mcfg_list should already be
initialized by the time we get there, and it should be immutable for
the life of the system. In fact, I would prefer if we could just
search the static table itself whenever we need it rather than caching
it in our own list. But I don't think we can easily do that because
acpi_table_parse() is __init.
+ e = pci_mcfg_lookup(seg, bus_start);
I would argue that we should check for _CBA first, and fall back to
MCFG if _CBA doesn't exist.
+ if (!e) {
+ addr = acpi_pci_root_get_mcfg_addr(root->device->handle);
IMO, acpi_pci_root_get_mcfg_addr() is misnamed. It should be
acpi_pci_config_base_addr() or similar. It definitely is not related
to MCFG. Not your fault, obviously.
+ if (addr == 0) {
+ pr_err(PREFIX"%04x:%02x-%02x bus range error\n",
+ seg, bus_start, bus_end);
+ err = -ENOENT;
+ goto err_out;
+ }
+ } else {
+ if (bus_start != e->bus_start) {
+ pr_err("%04x:%02x-%02x bus range mismatch %02x\n",
+ seg, bus_start, bus_end, e->bus_start);
+ err = -EINVAL;
+ goto err_out;
+ } else if (bus_end != e->bus_end) {
+ pr_warn("%04x:%02x-%02x bus end mismatch %02x\n",
+ seg, bus_start, bus_end, e->bus_end);
+ bus_end = min(bus_end, e->bus_end);
+ }
+ addr = e->addr;
+ }
I really don't think you need a lock around this, so you can factor
out the address lookup into something like:
addr = acpi_pci_config_base_addr(...);
if (addr)
return addr;
return acpi_pci_mcfg_lookup(seg, busn_res);
You can check inside acpi_pci_mcfg_lookup() to make sure the entry you
find covers the entire [busn_res.start-busn_res.end] range and return
failure if it doesn't. At this point, I'm not sure it's worth it to
truncate the host bridge bus range to match something we find in MCFG.
If the MCFG entry covers *more* than the host bridge range from _CRS,
that's fine. In any case, we have to be careful with the start address,
because the MCFG start address is always based on bus 0, but I think
pci_generic_ecam_create() expects the start address based on the
bus_start you pass to it.
Ah, this is information similar to what I suggested we might print in
pci_generic_ecam_create(). Could go either way, but personally I
think I'd put it in pci_generic_ecam_create() instead, because then we
only print the info we're actually using. And I think it'd be nice to
have the actual MMIO resource ("[mem 0x...-0x...]") instead of just
the base.
+ }
+
+ return 0;
+}
+
+/* Interface called by ACPI - parse and save MCFG table */
+void __init pci_mcfg_init(void)
+{
+ int err = acpi_table_parse(ACPI_SIG_MCFG, pci_mcfg_parse);
+ if (err)
+ pr_err(PREFIX "Failed to parse MCFG (%d)\n", err);
+ else if (list_empty(&pci_mcfg_list))
+ pr_info(PREFIX "No valid entries in MCFG table.\n");
+ else {
+ struct mcfg_entry *e;
+ int i = 0;
+ list_for_each_entry(e, &pci_mcfg_list, list)
+ i++;
+ pr_info(PREFIX "MCFG table loaded, %d entries\n", i);
+ }
+}
+
+/* Raw operations, works only for MCFG entries with an associated bus */
Can you elaborate a little on the connection with MCFG? I don't quite
see how they're related. Obviously the device has to be on a bus
we've already enumerated and assigned bus->ops for, but it seems like
we don't really know or care what bus->ops actually is. Given that
these are in this file, acpi_pci_root_ops is the only possibility,
since that's what we pass to acpi_pci_root_create(), but it doesn't
seem worth mentioning specifically in a comment.
quoted hunk
+int raw_pci_read(unsigned int domain, unsigned int busn, unsigned int devfn,
+ int reg, int len, u32 *val)
+{
+ struct pci_bus *bus = pci_find_bus(domain, busn);
+
+ if (!bus)
+ return PCIBIOS_DEVICE_NOT_FOUND;
+ return bus->ops->read(bus, devfn, reg, len, val);
+}
+
+int raw_pci_write(unsigned int domain, unsigned int busn, unsigned int devfn,
+ int reg, int len, u32 val)
+{
+ struct pci_bus *bus = pci_find_bus(domain, busn);
+
+ if (!bus)
+ return PCIBIOS_DEVICE_NOT_FOUND;
+ return bus->ops->write(bus, devfn, reg, len, val);
+}
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
What does this lock protect? The pci_mcfg_list should already be
initialized by the time we get there, and it should be immutable for
the life of the system. In fact, I would prefer if we could just
search the static table itself whenever we need it rather than caching
it in our own list. But I don't think we can easily do that because
acpi_table_parse() is __init.
quoted
+ e = pci_mcfg_lookup(seg, bus_start);
I would argue that we should check for _CBA first, and fall back to
MCFG if _CBA doesn't exist.
quoted
+ if (!e) {
+ addr = acpi_pci_root_get_mcfg_addr(root->device->handle);
IMO, acpi_pci_root_get_mcfg_addr() is misnamed. It should be
acpi_pci_config_base_addr() or similar. It definitely is not related
to MCFG. Not your fault, obviously.
quoted
+ if (addr == 0) {
+ pr_err(PREFIX"%04x:%02x-%02x bus range error\n",
+ seg, bus_start, bus_end);
+ err = -ENOENT;
+ goto err_out;
+ }
+ } else {
+ if (bus_start != e->bus_start) {
+ pr_err("%04x:%02x-%02x bus range mismatch %02x\n",
+ seg, bus_start, bus_end, e->bus_start);
+ err = -EINVAL;
+ goto err_out;
+ } else if (bus_end != e->bus_end) {
+ pr_warn("%04x:%02x-%02x bus end mismatch %02x\n",
+ seg, bus_start, bus_end, e->bus_end);
+ bus_end = min(bus_end, e->bus_end);
+ }
+ addr = e->addr;
+ }
I really don't think you need a lock around this, so you can factor
out the address lookup into something like:
addr = acpi_pci_config_base_addr(...);
if (addr)
return addr;
return acpi_pci_mcfg_lookup(seg, busn_res);
You can check inside acpi_pci_mcfg_lookup() to make sure the entry you
find covers the entire [busn_res.start-busn_res.end] range and return
failure if it doesn't. At this point, I'm not sure it's worth it to
truncate the host bridge bus range to match something we find in MCFG.
If the MCFG entry covers *more* than the host bridge range from _CRS,
that's fine. In any case, we have to be careful with the start address,
because the MCFG start address is always based on bus 0, but I think
pci_generic_ecam_create() expects the start address based on the
bus_start you pass to it.
Yes, I spotted this too, it is unfortunate but DT and MCFG handle
the ECAM regions differently. In DT the reg property is relative
to bus_start - ie reg MMIO region maps config space starting at
the first bus in bus-range:
Documentation/devicetree/bindings/pci/host-generic-pci.txt
in ACPI(MCFG) as you said it is always relative to bus 0, it is
unfortunate but the address to be mapped should be computed
differently in the ECAM layer.
Lorenzo
What does this lock protect? The pci_mcfg_list should already be
initialized by the time we get there, and it should be immutable for
the life of the system. In fact, I would prefer if we could just
search the static table itself whenever we need it rather than caching
it in our own list. But I don't think we can easily do that because
acpi_table_parse() is __init.
quoted
+ e = pci_mcfg_lookup(seg, bus_start);
I would argue that we should check for _CBA first, and fall back to
MCFG if _CBA doesn't exist.
quoted
+ if (!e) {
+ addr = acpi_pci_root_get_mcfg_addr(root->device->handle);
IMO, acpi_pci_root_get_mcfg_addr() is misnamed. It should be
acpi_pci_config_base_addr() or similar. It definitely is not related
to MCFG. Not your fault, obviously.
quoted
+ if (addr == 0) {
+ pr_err(PREFIX"%04x:%02x-%02x bus range error\n",
+ seg, bus_start, bus_end);
+ err = -ENOENT;
+ goto err_out;
+ }
+ } else {
+ if (bus_start != e->bus_start) {
+ pr_err("%04x:%02x-%02x bus range mismatch %02x\n",
+ seg, bus_start, bus_end, e->bus_start);
+ err = -EINVAL;
+ goto err_out;
+ } else if (bus_end != e->bus_end) {
+ pr_warn("%04x:%02x-%02x bus end mismatch %02x\n",
+ seg, bus_start, bus_end, e->bus_end);
+ bus_end = min(bus_end, e->bus_end);
+ }
+ addr = e->addr;
+ }
I really don't think you need a lock around this, so you can factor
out the address lookup into something like:
addr = acpi_pci_config_base_addr(...);
if (addr)
return addr;
return acpi_pci_mcfg_lookup(seg, busn_res);
You can check inside acpi_pci_mcfg_lookup() to make sure the entry you
find covers the entire [busn_res.start-busn_res.end] range and return
failure if it doesn't. At this point, I'm not sure it's worth it to
truncate the host bridge bus range to match something we find in MCFG.
If the MCFG entry covers *more* than the host bridge range from _CRS,
that's fine. In any case, we have to be careful with the start address,
because the MCFG start address is always based on bus 0, but I think
pci_generic_ecam_create() expects the start address based on the
bus_start you pass to it.
Yes, I spotted this too, it is unfortunate but DT and MCFG handle
the ECAM regions differently. In DT the reg property is relative
to bus_start - ie reg MMIO region maps config space starting at
the first bus in bus-range:
Documentation/devicetree/bindings/pci/host-generic-pci.txt
in ACPI(MCFG) as you said it is always relative to bus 0, it is
unfortunate but the address to be mapped should be computed
differently in the ECAM layer.
Can't this be handled by fixing up the address before passing to
pci_generic_ecam_create?
JC.
What does this lock protect? The pci_mcfg_list should already be
initialized by the time we get there, and it should be immutable for
the life of the system. In fact, I would prefer if we could just
search the static table itself whenever we need it rather than caching
it in our own list. But I don't think we can easily do that because
acpi_table_parse() is __init.
quoted
+ e = pci_mcfg_lookup(seg, bus_start);
I would argue that we should check for _CBA first, and fall back to
MCFG if _CBA doesn't exist.
quoted
+ if (!e) {
+ addr = acpi_pci_root_get_mcfg_addr(root->device->handle);
IMO, acpi_pci_root_get_mcfg_addr() is misnamed. It should be
acpi_pci_config_base_addr() or similar. It definitely is not related
to MCFG. Not your fault, obviously.
quoted
+ if (addr == 0) {
+ pr_err(PREFIX"%04x:%02x-%02x bus range error\n",
+ seg, bus_start, bus_end);
+ err = -ENOENT;
+ goto err_out;
+ }
+ } else {
+ if (bus_start != e->bus_start) {
+ pr_err("%04x:%02x-%02x bus range mismatch %02x\n",
+ seg, bus_start, bus_end, e->bus_start);
+ err = -EINVAL;
+ goto err_out;
+ } else if (bus_end != e->bus_end) {
+ pr_warn("%04x:%02x-%02x bus end mismatch %02x\n",
+ seg, bus_start, bus_end, e->bus_end);
+ bus_end = min(bus_end, e->bus_end);
+ }
+ addr = e->addr;
+ }
I really don't think you need a lock around this, so you can factor
out the address lookup into something like:
addr = acpi_pci_config_base_addr(...);
if (addr)
return addr;
return acpi_pci_mcfg_lookup(seg, busn_res);
You can check inside acpi_pci_mcfg_lookup() to make sure the entry you
find covers the entire [busn_res.start-busn_res.end] range and return
failure if it doesn't. At this point, I'm not sure it's worth it to
truncate the host bridge bus range to match something we find in MCFG.
If the MCFG entry covers *more* than the host bridge range from _CRS,
that's fine. In any case, we have to be careful with the start address,
because the MCFG start address is always based on bus 0, but I think
pci_generic_ecam_create() expects the start address based on the
bus_start you pass to it.
Yes, I spotted this too, it is unfortunate but DT and MCFG handle
the ECAM regions differently. In DT the reg property is relative
to bus_start - ie reg MMIO region maps config space starting at
the first bus in bus-range:
Documentation/devicetree/bindings/pci/host-generic-pci.txt
in ACPI(MCFG) as you said it is always relative to bus 0, it is
unfortunate but the address to be mapped should be computed
differently in the ECAM layer.
Can't this be handled by fixing up the address before passing to
pci_generic_ecam_create?
What does this lock protect? The pci_mcfg_list should already be
initialized by the time we get there, and it should be immutable for
the life of the system. In fact, I would prefer if we could just
search the static table itself whenever we need it rather than caching
it in our own list. But I don't think we can easily do that because
acpi_table_parse() is __init.
quoted
+ e = pci_mcfg_lookup(seg, bus_start);
I would argue that we should check for _CBA first, and fall back to
MCFG if _CBA doesn't exist.
quoted
+ if (!e) {
+ addr = acpi_pci_root_get_mcfg_addr(root->device->handle);
IMO, acpi_pci_root_get_mcfg_addr() is misnamed. It should be
acpi_pci_config_base_addr() or similar. It definitely is not related
to MCFG. Not your fault, obviously.
quoted
+ if (addr == 0) {
+ pr_err(PREFIX"%04x:%02x-%02x bus range error\n",
+ seg, bus_start, bus_end);
+ err = -ENOENT;
+ goto err_out;
+ }
+ } else {
+ if (bus_start != e->bus_start) {
+ pr_err("%04x:%02x-%02x bus range mismatch %02x\n",
+ seg, bus_start, bus_end, e->bus_start);
+ err = -EINVAL;
+ goto err_out;
+ } else if (bus_end != e->bus_end) {
+ pr_warn("%04x:%02x-%02x bus end mismatch %02x\n",
+ seg, bus_start, bus_end, e->bus_end);
+ bus_end = min(bus_end, e->bus_end);
+ }
+ addr = e->addr;
+ }
I really don't think you need a lock around this, so you can factor
out the address lookup into something like:
addr = acpi_pci_config_base_addr(...);
if (addr)
return addr;
return acpi_pci_mcfg_lookup(seg, busn_res);
You can check inside acpi_pci_mcfg_lookup() to make sure the entry you
find covers the entire [busn_res.start-busn_res.end] range and return
failure if it doesn't. At this point, I'm not sure it's worth it to
truncate the host bridge bus range to match something we find in MCFG.
If the MCFG entry covers *more* than the host bridge range from _CRS,
that's fine. In any case, we have to be careful with the start address,
because the MCFG start address is always based on bus 0, but I think
pci_generic_ecam_create() expects the start address based on the
bus_start you pass to it.
Yes, I spotted this too, it is unfortunate but DT and MCFG handle
the ECAM regions differently. In DT the reg property is relative
to bus_start - ie reg MMIO region maps config space starting at
the first bus in bus-range:
Documentation/devicetree/bindings/pci/host-generic-pci.txt
in ACPI(MCFG) as you said it is always relative to bus 0, it is
unfortunate but the address to be mapped should be computed
differently in the ECAM layer.
Can't this be handled by fixing up the address before passing to
pci_generic_ecam_create?
Yes it can, you just need to apply the bus shift, given that we know
it is ECAM anyway you can even add a macro in the ecam generic header to
compute it, anyway that's a minor detail, we just should not forget to
fix it.
Lorenzo
From: Tomasz Nowicki <hidden> Date: 2016-05-02 11:04:15
On 04/28/2016 11:48 PM, Bjorn Helgaas wrote:
Hey, I really kind of like this. I think this might work out well.
On Fri, Apr 15, 2016 at 07:06:44PM +0200, Tomasz Nowicki wrote:
quoted
This patch is going to implement generic PCI host controller for
ACPI world, similar to what pci-host-generic.c driver does for DT world.
All such drivers, which we have seen so far, were implemented within
arch/ directory since they had some arch assumptions (x86 and ia64).
However, they all are doing similar thing, so it makes sense to find
some common code and abstract it into the generic driver.
In order to handle PCI config space regions properly, we define new
MCFG interface which parses MCFG table and keep its entries
in a list. New pci_mcfg_init call is defined so that we do not depend
on PCI_MMCONFIG. Regions are not mapped until host bridge ask for it.
The implementation of pci_acpi_scan_root() looks up the saved MCFG entries
and sets up a new mapping. Generic PCI functions are used for
accessing config space. Driver selects PCI_GENERIC_ECAM and uses functions
from drivers/pci/ecam.h to create and access ECAM mappings.
As mentioned in Kconfig help section, ACPI_PCI_HOST_GENERIC choice
should be made on a per-architecture basis.
This patch is heavily based on the updated version from Jayachandran C:
https://lkml.org/lkml/2016/4/11/908
git: https://github.com/jchandra-brcm/linux/ (arm64-acpi-pci-v3)
Signed-off-by: Tomasz Nowicki <redacted>
Signed-off-by: Jayachandran C <redacted>
---
drivers/acpi/Kconfig | 8 ++
drivers/acpi/Makefile | 1 +
drivers/acpi/bus.c | 1 +
drivers/acpi/pci_gen_host.c | 231 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/pci.h | 6 ++
5 files changed, 247 insertions(+)
create mode 100644 drivers/acpi/pci_gen_host.c
@@ -0,0 +1,231 @@+/*+*Thisprogramisfreesoftware;youcanredistributeitand/ormodify+*itunderthetermsoftheGNUGeneralPublicLicense,version2,as+*publishedbytheFreeSoftwareFoundation(the"GPL").+*+*Thisprogramisdistributedinthehopethatitwillbeuseful,but+*WITHOUTANYWARRANTY;withouteventheimpliedwarrantyof+*MERCHANTABILITYorFITNESSFORAPARTICULARPURPOSE.SeetheGNU+*GeneralPublicLicenseversion2(GPLv2)formoredetails.+*+*YoushouldhavereceivedacopyoftheGNUGeneralPublicLicense+*version2(GPLv2)alongwiththissourcecode.+*/+#include<linux/kernel.h>+#include<linux/pci.h>+#include<linux/pci-acpi.h>+#include<linux/sfi_acpi.h>+#include<linux/slab.h>++#include"../pci/ecam.h"++#define PREFIX "ACPI: "++/* Structure to hold entries from the MCFG table */+structmcfg_entry{+structlist_headlist;+phys_addr_taddr;+u16segment;+u8bus_start;+u8bus_end;+};++/* List to save mcfg entries */+staticLIST_HEAD(pci_mcfg_list);+staticDEFINE_MUTEX(pci_mcfg_lock);++/* ACPI info for generic ACPI PCI controller */+structacpi_pci_generic_root_info{+structacpi_pci_root_infocommon;+structpci_config_window*cfg;/* config space mapping */+};++/* Find the entry in mcfg list which contains range bus_start */+staticstructmcfg_entry*pci_mcfg_lookup(u16seg,u8bus_start)+{+structmcfg_entry*e;++list_for_each_entry(e,&pci_mcfg_list,list){+if(e->segment==seg&&+e->bus_start<=bus_start&&bus_start<=e->bus_end)+returne;+}++returnNULL;+}
Can you put the MCFG parsing, caching, and searching in a different
file, e.g., drivers/acpi/pci_mcfg.c?
I will.
quoted
+/*
+ * Lookup the bus range for the domain in MCFG, and set up config space
+ * mapping.
+ */
+static int pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root,
+ struct acpi_pci_generic_root_info *ri)
+{
+ u16 seg = root->segment;
+ u8 bus_start = root->secondary.start;
+ u8 bus_end = root->secondary.end;
+ struct pci_config_window *cfg;
+ struct mcfg_entry *e;
+ phys_addr_t addr;
+ int err = 0;
+
+ mutex_lock(&pci_mcfg_lock);
What does this lock protect? The pci_mcfg_list should already be
initialized by the time we get there, and it should be immutable for
the life of the system.
Right, lock is useless.
In fact, I would prefer if we could just
search the static table itself whenever we need it rather than caching
it in our own list. But I don't think we can easily do that because
acpi_table_parse() is __init.
It is doable. We can implement our MCFG __init parsing handle to do
necessary sanity checks and then store MCFG table root pointer in
pci_mcfg.c. Then lookup call would use that pointer to traverse
available entries on demand.
quoted
+ e = pci_mcfg_lookup(seg, bus_start);
I would argue that we should check for _CBA first, and fall back to
MCFG if _CBA doesn't exist.
Agree.
quoted
+ if (!e) {
+ addr = acpi_pci_root_get_mcfg_addr(root->device->handle);
IMO, acpi_pci_root_get_mcfg_addr() is misnamed. It should be
acpi_pci_config_base_addr() or similar. It definitely is not related
to MCFG. Not your fault, obviously.
quoted
+ if (addr == 0) {
+ pr_err(PREFIX"%04x:%02x-%02x bus range error\n",
+ seg, bus_start, bus_end);
+ err = -ENOENT;
+ goto err_out;
+ }
+ } else {
+ if (bus_start != e->bus_start) {
+ pr_err("%04x:%02x-%02x bus range mismatch %02x\n",
+ seg, bus_start, bus_end, e->bus_start);
+ err = -EINVAL;
+ goto err_out;
+ } else if (bus_end != e->bus_end) {
+ pr_warn("%04x:%02x-%02x bus end mismatch %02x\n",
+ seg, bus_start, bus_end, e->bus_end);
+ bus_end = min(bus_end, e->bus_end);
+ }
+ addr = e->addr;
+ }
I really don't think you need a lock around this, so you can factor
out the address lookup into something like:
addr = acpi_pci_config_base_addr(...);
if (addr)
return addr;
return acpi_pci_mcfg_lookup(seg, busn_res);
You can check inside acpi_pci_mcfg_lookup() to make sure the entry you
find covers the entire [busn_res.start-busn_res.end] range and return
failure if it doesn't. At this point, I'm not sure it's worth it to
truncate the host bridge bus range to match something we find in MCFG.
If the MCFG entry covers *more* than the host bridge range from _CRS,
that's fine.
Makes sense for me.
In any case, we have to be careful with the start address,
because the MCFG start address is always based on bus 0, but I think
pci_generic_ecam_create() expects the start address based on the
bus_start you pass to it.
Ah, this is information similar to what I suggested we might print in
pci_generic_ecam_create(). Could go either way, but personally I
think I'd put it in pci_generic_ecam_create() instead, because then we
only print the info we're actually using. And I think it'd be nice to
have the actual MMIO resource ("[mem 0x...-0x...]") instead of just
the base.
Giving above that I will drop MCFG entries cache, this will be printed
in pci_generic_ecam_create, as you suggested.
quoted
+ }
+
+ return 0;
+}
+
+/* Interface called by ACPI - parse and save MCFG table */
+void __init pci_mcfg_init(void)
+{
+ int err = acpi_table_parse(ACPI_SIG_MCFG, pci_mcfg_parse);
+ if (err)
+ pr_err(PREFIX "Failed to parse MCFG (%d)\n", err);
+ else if (list_empty(&pci_mcfg_list))
+ pr_info(PREFIX "No valid entries in MCFG table.\n");
+ else {
+ struct mcfg_entry *e;
+ int i = 0;
+ list_for_each_entry(e, &pci_mcfg_list, list)
+ i++;
+ pr_info(PREFIX "MCFG table loaded, %d entries\n", i);
+ }
+}
+
+/* Raw operations, works only for MCFG entries with an associated bus */
Can you elaborate a little on the connection with MCFG? I don't quite
see how they're related. Obviously the device has to be on a bus
we've already enumerated and assigned bus->ops for, but it seems like
we don't really know or care what bus->ops actually is. Given that
these are in this file, acpi_pci_root_ops is the only possibility,
since that's what we pass to acpi_pci_root_create(), but it doesn't
seem worth mentioning specifically in a comment.
Yes, you are right. I will drop this comment.
quoted
+int raw_pci_read(unsigned int domain, unsigned int busn, unsigned int devfn,
+ int reg, int len, u32 *val)
+{
+ struct pci_bus *bus = pci_find_bus(domain, busn);
+
+ if (!bus)
+ return PCIBIOS_DEVICE_NOT_FOUND;
+ return bus->ops->read(bus, devfn, reg, len, val);
+}
+
+int raw_pci_write(unsigned int domain, unsigned int busn, unsigned int devfn,
+ int reg, int len, u32 val)
+{
+ struct pci_bus *bus = pci_find_bus(domain, busn);
+
+ if (!bus)
+ return PCIBIOS_DEVICE_NOT_FOUND;
+ return bus->ops->write(bus, devfn, reg, len, val);
+}
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Tomasz
I merged my patchset to branch topci-acpi-v6.
The patchset is used for Hisilicon DO2 PCIe ACPI support.
I found some compile errors. The log as below.
drivers/pci/host/pcie-hisi-acpi.c: In function 'hisi_pcie_init':
drivers/pci/host/pcie-hisi-acpi.c:130:6: error: 'struct acpi_pci_root' has no member named 'sysdata'
root->sysdata = reg_base;
In your PATCH V5, add "sysdata" for strcut acpi_pci_root, but PATCH V6 has not add it.
In my patch, I used root->sysdata which will be available along read/write accessor.
I want to know the reason this v6 patchset does not add "sysdata". I need this.
@@ -556,6 +556,7 @@ struct acpi_pci_root {structpci_bus*bus;u16segment;structresourcesecondary;/* downstream bus range */+void*sysdata;u32osc_support_set;/* _OSC state of support bits */u32osc_control_set;/* _OSC state of control bits */
Thanks
Dongdong
? 2016/4/16 1:06, Tomasz Nowicki ??:
quoted hunk
Some platforms may not be fully compliant with generic set of PCI config
accessors. For these cases we implement the way to overwrite accessors
set prior to PCI buses enumeration. Algorithm traverses available quirk
list, matches against <DMI ID (optional), domain, bus number> tuple and
an extra match call and returns corresponding PCI config ops.
All quirks can be defined using:
DECLARE_ACPI_MCFG_FIXUP() macro and kept self contained. Example:
/* Additional DMI platform identification (optional) */
static const struct dmi_system_id foo_dmi[] = {
{
.ident = "<Platform ident string>",
.matches = {
DMI_MATCH(DMI_SYS_VENDOR, "<system vendor>"),
DMI_MATCH(DMI_PRODUCT_NAME, "<product name>"),
DMI_MATCH(DMI_PRODUCT_VERSION, "product version"),
},
},
{ }
};
/* Custom PCI config ops */
static struct pci_generic_ecam_ops foo_pci_ops = {
.bus_shift = 24,
.pci_ops = {
.map_bus = pci_mcfg_dev_base,
.read = foo_ecam_config_read,
.write = foo_ecam_config_write,
}
};
static int foo_match(struct pci_mcfg_fixup *fixup, struct acpi_pci_root *root)
{
if (additional platform identification)
return true;
return false;
}
DECLARE_ACPI_MCFG_FIXUP(foo_dmi, foo_init, &foo_root_ops, <domain_nr>, <bus_nr>);
Signed-off-by: Tomasz Nowicki <redacted>
---
drivers/acpi/pci_gen_host.c | 30 +++++++++++++++++++++++++++++-
include/asm-generic/vmlinux.lds.h | 7 +++++++
include/linux/pci-acpi.h | 18 ++++++++++++++++++
3 files changed, 54 insertions(+), 1 deletion(-)
From: Tomasz Nowicki <hidden> Date: 2016-04-18 12:22:23
On 18.04.2016 13:37, liudongdong (C) wrote:
Hi Tomasz
I merged my patchset to branch topci-acpi-v6.
The patchset is used for Hisilicon DO2 PCIe ACPI support.
I found some compile errors. The log as below.
drivers/pci/host/pcie-hisi-acpi.c: In function 'hisi_pcie_init':
drivers/pci/host/pcie-hisi-acpi.c:130:6: error: 'struct acpi_pci_root'
has no member named 'sysdata'
root->sysdata = reg_base;
In your PATCH V5, add "sysdata" for strcut acpi_pci_root, but PATCH V6
has not add it.
In my patch, I used root->sysdata which will be available along
read/write accessor.
I want to know the reason this v6 patchset does not add "sysdata". I
need this.
We are handling this different way. You can now use "struct
pci_config_window" -> priv, see pci-thunder-pem.c driver.
Tomasz
From: Tomasz Nowicki <hidden> Date: 2016-04-15 17:07:45
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 <redacted>
To: Catalin Marinas <catalin.marinas@arm.com>
To: Lorenzo Pieralisi <redacted>
To: Will Deacon <redacted>
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(-)
@@ -63,27 +63,3 @@ int pcibios_alloc_irq(struct pci_dev *dev)return0;}--/*-*raw_pci_read/write-Platform-specificPCIconfigspaceaccess.-*/-intraw_pci_read(unsignedintdomain,unsignedintbus,-unsignedintdevfn,intreg,intlen,u32*val)-{-return-ENXIO;-}--intraw_pci_write(unsignedintdomain,unsignedintbus,-unsignedintdevfn,intreg,intlen,u32val)-{-return-ENXIO;-}--#ifdef CONFIG_ACPI-/* Root bridge scanning */-structpci_bus*pci_acpi_scan_root(structacpi_pci_root*root)-{-/* TODO: Should be revisited when implementing PCI on ACPI */-returnNULL;-}-#endif
From: Tomasz Nowicki <hidden> Date: 2016-04-15 17:07:59
Passes 1.x miss PCI enhanced allocation (EA) header for fixed-BARs,
thus these passes should use Cavium-specific config access functions
that synthesize the missing EA capabilities.
We already have DT driver which addresses errata requirements and
allows to use special PCI config accessors. Currently this driver uses
compatible = "cavium,pci-host-thunder-ecam" to mach against the errata.
For ACPI case we need explicit errata number and corresponding
DECLARE_ACPI_MCFG_FIXUP fixup code.
Signed-off-by: Tomasz Nowicki <redacted>
---
arch/arm64/Kconfig | 14 ++++++++++++++
arch/arm64/include/asm/cpufeature.h | 3 ++-
arch/arm64/kernel/cpu_errata.c | 8 ++++++++
drivers/pci/host/pci-thunder-ecam.c | 35 +++++++++++++++++++++++++++++++++++
4 files changed, 59 insertions(+), 1 deletion(-)
From: Tomasz Nowicki <hidden> Date: 2016-04-19 10:26:34
On 15.04.2016 19:06, Tomasz Nowicki wrote:
quoted hunk
Passes 1.x miss PCI enhanced allocation (EA) header for fixed-BARs,
thus these passes should use Cavium-specific config access functions
that synthesize the missing EA capabilities.
We already have DT driver which addresses errata requirements and
allows to use special PCI config accessors. Currently this driver uses
compatible = "cavium,pci-host-thunder-ecam" to mach against the errata.
For ACPI case we need explicit errata number and corresponding
DECLARE_ACPI_MCFG_FIXUP fixup code.
Signed-off-by: Tomasz Nowicki <redacted>
---
arch/arm64/Kconfig | 14 ++++++++++++++
arch/arm64/include/asm/cpufeature.h | 3 ++-
arch/arm64/kernel/cpu_errata.c | 8 ++++++++
drivers/pci/host/pci-thunder-ecam.c | 35 +++++++++++++++++++++++++++++++++++
4 files changed, 59 insertions(+), 1 deletion(-)
I wonder if we can identify quirk based on _DSD properties, like that:
Name (_DSD, Package () {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package () {
Package (2) {"cavium,pci-host-thunder-ecam", 1},
}
})
similar for PEM driver:
Name (_DSD, Package () {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package () {
Package (2) {"cavium,pci-host-thunder-pem", 1},
}
})
Do you think it is right thing to do?
Tomasz
From: G Gregory <hidden> Date: 2016-04-19 10:41:32
On 19 April 2016 at 11:26, Tomasz Nowicki [off-list ref] wrote:
On 15.04.2016 19:06, Tomasz Nowicki wrote:
quoted
Passes 1.x miss PCI enhanced allocation (EA) header for fixed-BARs,
thus these passes should use Cavium-specific config access functions
that synthesize the missing EA capabilities.
We already have DT driver which addresses errata requirements and
allows to use special PCI config accessors. Currently this driver uses
compatible = "cavium,pci-host-thunder-ecam" to mach against the errata.
For ACPI case we need explicit errata number and corresponding
DECLARE_ACPI_MCFG_FIXUP fixup code.
Signed-off-by: Tomasz Nowicki <redacted>
---
arch/arm64/Kconfig | 14 ++++++++++++++
arch/arm64/include/asm/cpufeature.h | 3 ++-
arch/arm64/kernel/cpu_errata.c | 8 ++++++++
drivers/pci/host/pci-thunder-ecam.c | 35
+++++++++++++++++++++++++++++++++++
4 files changed, 59 insertions(+), 1 deletion(-)
missing
+ Enhanced Allocation (EA) capabilities for the fixed BAR
addresses used
+ by the on-SoC hardware blocks. The erratum adds config access
+ functions that synthesize the missing EA capabilities for
versions
+ that are missing that information.
+
+ If unsure, say Y.
+
endmenu
diff --git a/arch/arm64/include/asm/cpufeature.h
b/arch/arm64/include/asm/cpufeature.h
index b9b6494..a78364e 100644
I wonder if we can identify quirk based on _DSD properties, like that:
Name (_DSD, Package () {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package () {
Package (2) {"cavium,pci-host-thunder-ecam", 1},
}
})
similar for PEM driver:
Name (_DSD, Package () {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package () {
Package (2) {"cavium,pci-host-thunder-pem", 1},
}
})
Do you think it is right thing to do?
Well if your adding new properties why not add a Cavium specific _CID?
Graeme
On Tue, Apr 19, 2016 at 11:41:29AM +0100, G Gregory wrote:
On 19 April 2016 at 11:26, Tomasz Nowicki [off-list ref] wrote:
quoted
On 15.04.2016 19:06, Tomasz Nowicki wrote:
quoted
Passes 1.x miss PCI enhanced allocation (EA) header for fixed-BARs,
thus these passes should use Cavium-specific config access functions
that synthesize the missing EA capabilities.
We already have DT driver which addresses errata requirements and
allows to use special PCI config accessors. Currently this driver uses
compatible = "cavium,pci-host-thunder-ecam" to mach against the errata.
For ACPI case we need explicit errata number and corresponding
DECLARE_ACPI_MCFG_FIXUP fixup code.
Signed-off-by: Tomasz Nowicki <redacted>
---
arch/arm64/Kconfig | 14 ++++++++++++++
arch/arm64/include/asm/cpufeature.h | 3 ++-
arch/arm64/kernel/cpu_errata.c | 8 ++++++++
drivers/pci/host/pci-thunder-ecam.c | 35
+++++++++++++++++++++++++++++++++++
4 files changed, 59 insertions(+), 1 deletion(-)
missing
+ Enhanced Allocation (EA) capabilities for the fixed BAR
addresses used
+ by the on-SoC hardware blocks. The erratum adds config access
+ functions that synthesize the missing EA capabilities for
versions
+ that are missing that information.
+
+ If unsure, say Y.
+
endmenu
diff --git a/arch/arm64/include/asm/cpufeature.h
b/arch/arm64/include/asm/cpufeature.h
index b9b6494..a78364e 100644
I wonder if we can identify quirk based on _DSD properties, like that:
Name (_DSD, Package () {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package () {
Package (2) {"cavium,pci-host-thunder-ecam", 1},
}
})
similar for PEM driver:
Name (_DSD, Package () {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package () {
Package (2) {"cavium,pci-host-thunder-pem", 1},
}
})
Do you think it is right thing to do?
Well if your adding new properties why not add a Cavium specific _CID?
For clarity what I mean is something like this allowed.
Device (PCI0)
{
Name (_HID, EisaId ("PNP0A08"))
Name (_CID, EisaId ("PNP0A03"))
Name (_HID, "CAV0666")) // Cavium Quirky Hardware
....
}
Graeme
From: Tomasz Nowicki <hidden> Date: 2016-04-19 11:22:57
On 19.04.2016 13:12, Graeme Gregory wrote:
On Tue, Apr 19, 2016 at 11:41:29AM +0100, G Gregory wrote:
quoted
On 19 April 2016 at 11:26, Tomasz Nowicki [off-list ref] wrote:
quoted
On 15.04.2016 19:06, Tomasz Nowicki wrote:
quoted
Passes 1.x miss PCI enhanced allocation (EA) header for fixed-BARs,
thus these passes should use Cavium-specific config access functions
that synthesize the missing EA capabilities.
We already have DT driver which addresses errata requirements and
allows to use special PCI config accessors. Currently this driver uses
compatible = "cavium,pci-host-thunder-ecam" to mach against the errata.
For ACPI case we need explicit errata number and corresponding
DECLARE_ACPI_MCFG_FIXUP fixup code.
Signed-off-by: Tomasz Nowicki <redacted>
---
arch/arm64/Kconfig | 14 ++++++++++++++
arch/arm64/include/asm/cpufeature.h | 3 ++-
arch/arm64/kernel/cpu_errata.c | 8 ++++++++
drivers/pci/host/pci-thunder-ecam.c | 35
+++++++++++++++++++++++++++++++++++
4 files changed, 59 insertions(+), 1 deletion(-)
missing
+ Enhanced Allocation (EA) capabilities for the fixed BAR
addresses used
+ by the on-SoC hardware blocks. The erratum adds config access
+ functions that synthesize the missing EA capabilities for
versions
+ that are missing that information.
+
+ If unsure, say Y.
+
endmenu
diff --git a/arch/arm64/include/asm/cpufeature.h
b/arch/arm64/include/asm/cpufeature.h
index b9b6494..a78364e 100644
I wonder if we can identify quirk based on _DSD properties, like that:
Name (_DSD, Package () {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package () {
Package (2) {"cavium,pci-host-thunder-ecam", 1},
}
})
similar for PEM driver:
Name (_DSD, Package () {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package () {
Package (2) {"cavium,pci-host-thunder-pem", 1},
}
})
Do you think it is right thing to do?
Well if your adding new properties why not add a Cavium specific _CID?
For clarity what I mean is something like this allowed.
Device (PCI0)
{
Name (_HID, EisaId ("PNP0A08"))
Name (_CID, EisaId ("PNP0A03"))
Name (_HID, "CAV0666")) // Cavium Quirky Hardware
....
}
From: G Gregory <hidden> Date: 2016-04-19 12:29:11
On Tue, Apr 19, 2016 at 01:22:29PM +0200, Tomasz Nowicki wrote:
On 19.04.2016 13:12, Graeme Gregory wrote:
quoted
On Tue, Apr 19, 2016 at 11:41:29AM +0100, G Gregory wrote:
quoted
On 19 April 2016 at 11:26, Tomasz Nowicki [off-list ref] wrote:
quoted
On 15.04.2016 19:06, Tomasz Nowicki wrote:
quoted
Passes 1.x miss PCI enhanced allocation (EA) header for fixed-BARs,
thus these passes should use Cavium-specific config access functions
that synthesize the missing EA capabilities.
We already have DT driver which addresses errata requirements and
allows to use special PCI config accessors. Currently this driver uses
compatible = "cavium,pci-host-thunder-ecam" to mach against the errata.
For ACPI case we need explicit errata number and corresponding
DECLARE_ACPI_MCFG_FIXUP fixup code.
Signed-off-by: Tomasz Nowicki <redacted>
---
arch/arm64/Kconfig | 14 ++++++++++++++
arch/arm64/include/asm/cpufeature.h | 3 ++-
arch/arm64/kernel/cpu_errata.c | 8 ++++++++
drivers/pci/host/pci-thunder-ecam.c | 35
+++++++++++++++++++++++++++++++++++
4 files changed, 59 insertions(+), 1 deletion(-)
missing
+ Enhanced Allocation (EA) capabilities for the fixed BAR
addresses used
+ by the on-SoC hardware blocks. The erratum adds config access
+ functions that synthesize the missing EA capabilities for
versions
+ that are missing that information.
+
+ If unsure, say Y.
+
endmenu
diff --git a/arch/arm64/include/asm/cpufeature.h
b/arch/arm64/include/asm/cpufeature.h
index b9b6494..a78364e 100644
I wonder if we can identify quirk based on _DSD properties, like that:
Name (_DSD, Package () {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package () {
Package (2) {"cavium,pci-host-thunder-ecam", 1},
}
})
similar for PEM driver:
Name (_DSD, Package () {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package () {
Package (2) {"cavium,pci-host-thunder-pem", 1},
}
})
Do you think it is right thing to do?
Well if your adding new properties why not add a Cavium specific _CID?
For clarity what I mean is something like this allowed.
Device (PCI0)
{
Name (_HID, EisaId ("PNP0A08"))
Name (_CID, EisaId ("PNP0A03"))
Name (_HID, "CAV0666")) // Cavium Quirky Hardware
....
}
Rafael, Bjorn, what do you think?
Another option as has been pointed out to me is that devices that are
not compliant don't advertise the IDs PNP0A03/8 and should use hardware
specific IDs.
Obviously in linux the drivers for them would re-use 90% of the generic
code with just their added accessors.
I do not know though what knock-on effects that would have and if there
is stuff that relies on the host bridge being IDed as the standard one.
Graeme
From: Tomasz Nowicki <hidden> Date: 2016-04-15 17:09:52
From: Jayachandran C <redacted>
Use functions provided by drivers/pci/ecam.h for mapping the config
space in drivers/pci/host/pci-host-common.c, and update its users to
use 'struct pci_config_window' and 'struct pci_generic_ecam_ops'
The changes are mostly to use 'struct pci_config_window' in place of
'struct gen_pci'. Some of the fields of gen_pci were only used
temporarily and can be eliminated by using local variables or function
arguments, these are not carried over to struct pci_config_window.
pci-thunder-ecam.c and pci-thunder-pem.c are the only users of the
pci_host_common_probe function and the gen_pci structure, these have
been updated to use the new API as well.
The patch does not introduce any functional changes other than a very
minor one: with the new code, on 64-bit platforms, we do just a single
ioremap for the whole config space.
Signed-off-by: Jayachandran C <redacted>
---
drivers/pci/ecam.h | 5 ++
drivers/pci/host/Kconfig | 1 +
drivers/pci/host/pci-host-common.c | 119 ++++++++++++++++--------------------
drivers/pci/host/pci-host-common.h | 47 --------------
drivers/pci/host/pci-host-generic.c | 52 +++-------------
drivers/pci/host/pci-thunder-ecam.c | 39 +++---------
drivers/pci/host/pci-thunder-pem.c | 88 ++++++++++++--------------
7 files changed, 115 insertions(+), 236 deletions(-)
delete mode 100644 drivers/pci/host/pci-host-common.h
@@ -58,4 +58,9 @@ void __iomem *pci_generic_ecam_map_bus(struct pci_bus *bus, unsigned int devfn,/* default ECAM ops, bus shift 20, generic read and write */externstructpci_generic_ecam_opspci_generic_ecam_default_ops;+#ifdef CONFIG_PCI_HOST_GENERIC+/* for DT based pci controllers that support ECAM */+intpci_host_common_probe(structplatform_device*pdev,+structpci_generic_ecam_ops*ops);+#endif#endif
@@ -60,7 +54,7 @@ static int gen_pci_parse_request_of_pci_ranges(struct gen_pci *pci)res_valid|=!(res->flags&IORESOURCE_PREFETCH);break;caseIORESOURCE_BUS:-pci->cfg.bus_range=res;+*bus_range=res;default:continue;}
@@ -79,65 +73,67 @@ static int gen_pci_parse_request_of_pci_ranges(struct gen_pci *pci)return0;out_release_res:-gen_pci_release_of_pci_ranges(pci);returnerr;}-staticintgen_pci_parse_map_cfg_windows(structgen_pci*pci)+staticvoidgen_pci_generic_unmap_cfg(void*ptr)+{+pci_generic_ecam_free((structpci_config_window*)ptr);+}++staticstructpci_config_window*gen_pci_init(structdevice*dev,+structlist_head*resources,structpci_generic_ecam_ops*ops){interr;-u8bus_max;-resource_size_tbusn;-structresource*bus_range;-structdevice*dev=pci->host.dev.parent;-structdevice_node*np=dev->of_node;-u32sz=1<<pci->cfg.ops->bus_shift;+structresourcecfgres;+structresource*bus_range=NULL;+structpci_config_window*cfg;+unsignedintbus_shift=ops->bus_shift;-err=of_address_to_resource(np,0,&pci->cfg.res);+/* Parse our PCI ranges and request their resources */+err=gen_pci_parse_request_of_pci_ranges(dev,resources,&bus_range);+if(err)+gotoerr_out;++err=of_address_to_resource(dev->of_node,0,&cfgres);if(err){dev_err(dev,"missing \"reg\" property\n");-returnerr;+gotoerr_out;}/* Limit the bus-range to fit within reg */-bus_max=pci->cfg.bus_range->start+-(resource_size(&pci->cfg.res)>>pci->cfg.ops->bus_shift)-1;-pci->cfg.bus_range->end=min_t(resource_size_t,-pci->cfg.bus_range->end,bus_max);--pci->cfg.win=devm_kcalloc(dev,resource_size(pci->cfg.bus_range),-sizeof(*pci->cfg.win),GFP_KERNEL);-if(!pci->cfg.win)-return-ENOMEM;--/* Map our Configuration Space windows */-if(!devm_request_mem_region(dev,pci->cfg.res.start,-resource_size(&pci->cfg.res),-"Configuration Space"))-return-ENOMEM;--bus_range=pci->cfg.bus_range;-for(busn=bus_range->start;busn<=bus_range->end;++busn){-u32idx=busn-bus_range->start;--pci->cfg.win[idx]=devm_ioremap(dev,-pci->cfg.res.start+idx*sz,-sz);-if(!pci->cfg.win[idx])-return-ENOMEM;+bus_range->end=min(bus_range->end,+bus_range->start+(resource_size(&cfgres)>>bus_shift)-1);++cfg=pci_generic_ecam_create(dev,cfgres.start,bus_range->start,+bus_range->end,ops);+if(IS_ERR(cfg)){+err=PTR_ERR(cfg);+gotoerr_out;}-return0;+err=devm_add_action(dev,gen_pci_generic_unmap_cfg,cfg);+if(err){+gen_pci_generic_unmap_cfg(cfg);+gotoerr_out;+}+returncfg;++err_out:+pci_free_resource_list(resources);+returnERR_PTR(err);}intpci_host_common_probe(structplatform_device*pdev,-structgen_pci*pci)+structpci_generic_ecam_ops*ops)+{-interr;constchar*type;structdevice*dev=&pdev->dev;structdevice_node*np=dev->of_node;structpci_bus*bus,*child;+structpci_config_window*cfg;+structlist_headresources;type=of_get_property(np,"device_type",NULL);if(!type||strcmp(type,"pci")){
@@ -147,29 +143,18 @@ int pci_host_common_probe(struct platform_device *pdev,of_pci_check_probe_only();-pci->host.dev.parent=dev;-INIT_LIST_HEAD(&pci->host.windows);-INIT_LIST_HEAD(&pci->resources);--/* Parse our PCI ranges and request their resources */-err=gen_pci_parse_request_of_pci_ranges(pci);-if(err)-returnerr;-/* Parse and map our Configuration Space windows */-err=gen_pci_parse_map_cfg_windows(pci);-if(err){-gen_pci_release_of_pci_ranges(pci);-returnerr;-}+INIT_LIST_HEAD(&resources);+cfg=gen_pci_init(dev,&resources,ops);+if(IS_ERR(cfg))+returnPTR_ERR(cfg);/* Do not reassign resources if probe only */if(!pci_has_flag(PCI_PROBE_ONLY))pci_add_flags(PCI_REASSIGN_ALL_RSRC|PCI_REASSIGN_ALL_BUS);--bus=pci_scan_root_bus(dev,pci->cfg.bus_range->start,-&pci->cfg.ops->ops,pci,&pci->resources);+bus=pci_scan_root_bus(dev,cfg->bus_start,&ops->pci_ops,cfg,+&resources);if(!bus){dev_err(dev,"Scanning rootbus failed");return-ENODEV;
@@ -1,47 +0,0 @@-/*- * 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.- *- * 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 for more details.- *- * You should have received a copy of the GNU General Public License- * along with this program. If not, see <http://www.gnu.org/licenses/>.- *- * Copyright (C) 2014 ARM Limited- *- * Author: Will Deacon <will.deacon@arm.com>- */--#ifndef _PCI_HOST_COMMON_H-#define _PCI_HOST_COMMON_H--#include <linux/kernel.h>-#include <linux/platform_device.h>--struct gen_pci_cfg_bus_ops {- u32 bus_shift;- struct pci_ops ops;-};--struct gen_pci_cfg_windows {- struct resource res;- struct resource *bus_range;- void __iomem **win;-- struct gen_pci_cfg_bus_ops *ops;-};--struct gen_pci {- struct pci_host_bridge host;- struct gen_pci_cfg_windows cfg;- struct list_head resources;-};--int pci_host_common_probe(struct platform_device *pdev,- struct gen_pci *pci);--#endif /* _PCI_HOST_COMMON_H */
@@ -13,18 +13,7 @@#include<linux/of.h>#include<linux/platform_device.h>-#include"pci-host-common.h"--/* Mapping is standard ECAM */-staticvoid__iomem*thunder_ecam_map_bus(structpci_bus*bus,-unsignedintdevfn,-intwhere)-{-structgen_pci*pci=bus->sysdata;-resource_size_tidx=bus->number-pci->cfg.bus_range->start;--returnpci->cfg.win[idx]+((devfn<<12)|where);-}+#include"../ecam.h"staticvoidset_val(u32v,intwhere,intsize,u32*val){
@@ -99,7 +88,7 @@ static int handle_ea_bar(u32 e0, int bar, struct pci_bus *bus,staticintthunder_ecam_p2_config_read(structpci_bus*bus,unsignedintdevfn,intwhere,intsize,u32*val){-structgen_pci*pci=bus->sysdata;+structpci_config_window*cfg=bus->sysdata;intwhere_a=where&~3;void__iomem*addr;u32node_bits;
@@ -129,7 +118,7 @@ static int thunder_ecam_p2_config_read(struct pci_bus *bus, unsigned int devfn,*theconfigspaceaccesswindow.Sinceweareworkingwith*thehigh-order32bits,shifteverythingdownby32bits.*/-node_bits=(pci->cfg.res.start>>32)&(1<<12);+node_bits=(cfg->cfgaddr>>32)&(1<<12);v|=node_bits;set_val(v,where,size,val);
@@ -358,36 +347,24 @@ static int thunder_ecam_config_write(struct pci_bus *bus, unsigned int devfn,returnpci_generic_config_write(bus,devfn,where,size,val);}-staticstructgen_pci_cfg_bus_opsthunder_ecam_bus_ops={+staticstructpci_generic_ecam_opspci_thunder_ecam_ops={.bus_shift=20,-.ops={-.map_bus=thunder_ecam_map_bus,+.pci_ops={+.map_bus=pci_generic_ecam_map_bus,.read=thunder_ecam_config_read,.write=thunder_ecam_config_write,}};staticconststructof_device_idthunder_ecam_of_match[]={-{.compatible="cavium,pci-host-thunder-ecam",-.data=&thunder_ecam_bus_ops},-+{.compatible="cavium,pci-host-thunder-ecam"},{},};MODULE_DEVICE_TABLE(of,thunder_ecam_of_match);staticintthunder_ecam_probe(structplatform_device*pdev){-structdevice*dev=&pdev->dev;-conststructof_device_id*of_id;-structgen_pci*pci=devm_kzalloc(dev,sizeof(*pci),GFP_KERNEL);--if(!pci)-return-ENOMEM;--of_id=of_match_node(thunder_ecam_of_match,dev->of_node);-pci->cfg.ops=(structgen_pci_cfg_bus_ops*)of_id->data;--returnpci_host_common_probe(pdev,pci);+returnpci_host_common_probe(pdev,&pci_thunder_ecam_ops);}staticstructplatform_driverthunder_ecam_driver={
I still think it would be better to keep the loadable PCI host drivers
separate from the ACPI PCI infrastructure. There are a number of
simplifications that we want to do to the DT based drivers in the long
run, so it's better if that code is not shared at this level. Abstracting
out the ECAM code is fine, but at that point you should be able to just
call it from the ACPI layer.
Arnd