[question for Rafael below]

On Fri, Apr 15, 2016 at 07:06:36PM +0200, Tomasz Nowicki wrote:

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

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

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

Suggested-by: Lorenzo Pieralisi lorenzo.pieralisi@arm.com Signed-off-by: Tomasz Nowicki tn@semihalf.com Reviewed-by: Lorenzo Pieralisi lorenzo.pieralisi@arm.com Tested-by: Duc Dang dhdang@apm.com Tested-by: Dongdong Liu liudongdong3@huawei.com Tested-by: Hanjun Guo hanjun.guo@linaro.org Tested-by: Graeme Gregory graeme.gregory@linaro.org Tested-by: Sinan Kaya okaya@codeaurora.org

---

arch/ia64/hp/common/sba_iommu.c | 2 +- arch/ia64/include/asm/pci.h | 1 - arch/ia64/pci/pci.c | 16 ---------------- arch/ia64/sn/kernel/io_acpi_init.c | 4 ++-- arch/x86/include/asm/pci.h | 3 --- arch/x86/pci/acpi.c | 17 ----------------- drivers/acpi/pci_root.c | 7 ++++++- drivers/pci/probe.c | 2 ++ 8 files changed, 11 insertions(+), 41 deletions(-)

diff --git a/arch/ia64/hp/common/sba_iommu.c b/arch/ia64/hp/common/sba_iommu.c index a6d6190..78e4444 100644 --- a/arch/ia64/hp/common/sba_iommu.c +++ b/arch/ia64/hp/common/sba_iommu.c @@ -1981,7 +1981,7 @@ sba_connect_bus(struct pci_bus *bus) if (PCI_CONTROLLER(bus)->iommu) return;

• handle = acpi_device_handle(PCI_CONTROLLER(bus)->companion);

• handle = acpi_device_handle(ACPI_COMPANION(bus->bridge)); if (!handle) return;

diff --git a/arch/ia64/include/asm/pci.h b/arch/ia64/include/asm/pci.h index c0835b0..12423f4 100644 --- a/arch/ia64/include/asm/pci.h +++ b/arch/ia64/include/asm/pci.h @@ -63,7 +63,6 @@ extern int pci_mmap_legacy_page_range(struct pci_bus *bus, #define pci_legacy_write platform_pci_legacy_write struct pci_controller {

• struct acpi_device *companion; void *iommu; int segment; int node; /* nearest node with memory or NUMA_NO_NODE for global allocation */

diff --git a/arch/ia64/pci/pci.c b/arch/ia64/pci/pci.c index 8f6ac2f..978d6af 100644 --- a/arch/ia64/pci/pci.c +++ b/arch/ia64/pci/pci.c @@ -301,28 +301,12 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) } info->controller.segment = root->segment;

• info->controller.companion = device; info->controller.node = acpi_get_node(device->handle); INIT_LIST_HEAD(&info->io_resources); return acpi_pci_root_create(root, &pci_acpi_root_ops, &info->common, &info->controller);

} -int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge) -{

• /*

• * We pass NULL as parent to pci_create_root_bus(), so if it is not NULL
  

• * here, pci_create_root_bus() has been called by someone else and
  

• * sysdata is likely to be different from what we expect.  Let it go in
  

• * that case.
  

• */
  

• if (!bridge->dev.parent) {

• struct pci_controller *controller = bridge->bus->sysdata;
  

• ACPI_COMPANION_SET(&bridge->dev, controller->companion);
  

• }
• return 0;

-}

void pcibios_fixup_device_resources(struct pci_dev *dev) { int idx; diff --git a/arch/ia64/sn/kernel/io_acpi_init.c b/arch/ia64/sn/kernel/io_acpi_init.c index 231234c..e454492 100644 --- a/arch/ia64/sn/kernel/io_acpi_init.c +++ b/arch/ia64/sn/kernel/io_acpi_init.c @@ -132,7 +132,7 @@ sn_get_bussoft_ptr(struct pci_bus *bus) struct acpi_resource_vendor_typed *vendor;

• handle = acpi_device_handle(PCI_CONTROLLER(bus)->companion);

• handle = acpi_device_handle(ACPI_COMPANION(bus->bridge)); status = acpi_get_vendor_resource(handle, METHOD_NAME__CRS, &sn_uuid, &buffer); if (ACPI_FAILURE(status)) {

@@ -360,7 +360,7 @@ sn_acpi_get_pcidev_info(struct pci_dev *dev, struct pcidev_info **pcidev_info, acpi_status status; struct acpi_buffer name_buffer = { ACPI_ALLOCATE_BUFFER, NULL };

• rootbus_handle = acpi_device_handle(PCI_CONTROLLER(dev)->companion);

• rootbus_handle = acpi_device_handle(ACPI_COMPANION(dev->bus->bridge)); status = acpi_evaluate_integer(rootbus_handle, METHOD_NAME__SEG, NULL, &segment); if (ACPI_SUCCESS(status)) {

diff --git a/arch/x86/include/asm/pci.h b/arch/x86/include/asm/pci.h index 9ab7507..24de07d 100644 --- a/arch/x86/include/asm/pci.h +++ b/arch/x86/include/asm/pci.h @@ -14,9 +14,6 @@ struct pci_sysdata { int domain; /* PCI domain */ int node; /* NUMA node */ -#ifdef CONFIG_ACPI

• struct acpi_device *companion; /* ACPI companion device */

-#endif #ifdef CONFIG_X86_64 void *iommu; /* IOMMU private data */ #endif diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c index 3cd6983..f4ca17a 100644 --- a/arch/x86/pci/acpi.c +++ b/arch/x86/pci/acpi.c @@ -340,7 +340,6 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) struct pci_sysdata sd = { .domain = domain, .node = node,

• 	.companion = root->device
  

  };

memcpy(bus->sysdata, &sd, sizeof(sd)); @@ -355,7 +354,6 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) else { info->sd.domain = domain; info->sd.node = node;

• 	info->sd.companion = root->device;
  bus = acpi_pci_root_create(root, &acpi_pci_root_ops,
  			   &info->common, &info->sd);
  

  }

@@ -373,21 +371,6 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) return bus; } -int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge) -{

• /*

• * We pass NULL as parent to pci_create_root_bus(), so if it is not NULL
  

• * here, pci_create_root_bus() has been called by someone else and
  

• * sysdata is likely to be different from what we expect.  Let it go in
  

• * that case.
  

• */
  

• if (!bridge->dev.parent) {

• struct pci_sysdata *sd = bridge->bus->sysdata;
  

• ACPI_COMPANION_SET(&bridge->dev, sd->companion);
  

• }
• return 0;

-}

int __init pci_acpi_init(void) { struct pci_dev *dev = NULL; diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c index ae3fe4e..4581e0e 100644 --- a/drivers/acpi/pci_root.c +++ b/drivers/acpi/pci_root.c @@ -564,6 +564,11 @@ static int acpi_pci_root_add(struct acpi_device *device, } }

• /*

• * pci_create_root_bus() needs to detect the parent device type,
  

• * so initialize its companion data accordingly.
  

• */
  

• ACPI_COMPANION_SET(&device->dev, device); root->device = device; root->segment = segment & 0xFFFF; strcpy(acpi_device_name(device), ACPI_PCI_ROOT_DEVICE_NAME);

@@ -846,7 +851,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, pci_acpi_root_add_resources(info); pci_add_resource(&info->resources, &root->secondary);

• bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,

• bus = pci_create_root_bus(&device->dev, busnum, ops->pci_ops, sysdata, &info->resources);

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

if (!bus) goto out_release_info; diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 8004f67..8087297 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -2141,6 +2141,8 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus, bridge->dev.parent = parent; bridge->dev.release = pci_release_host_bridge_dev; dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);

• if (parent)

• ACPI_COMPANION_SET(&bridge->dev, ACPI_COMPANION(parent));
  

  error = pcibios_root_bridge_prepare(bridge); if (error) { kfree(bridge);

-- 1.9.1

-- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html

On Wednesday, May 04, 2016 10:10:58 AM Tomasz Nowicki wrote:

On 27.04.2016 04:45, Bjorn Helgaas wrote:

...

[question for Rafael below]

On Fri, Apr 15, 2016 at 07:06:36PM +0200, Tomasz Nowicki wrote:

...

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

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

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

Suggested-by: Lorenzo Pieralisi lorenzo.pieralisi@arm.com Signed-off-by: Tomasz Nowicki tn@semihalf.com Reviewed-by: Lorenzo Pieralisi lorenzo.pieralisi@arm.com Tested-by: Duc Dang dhdang@apm.com Tested-by: Dongdong Liu liudongdong3@huawei.com Tested-by: Hanjun Guo hanjun.guo@linaro.org Tested-by: Graeme Gregory graeme.gregory@linaro.org Tested-by: Sinan Kaya okaya@codeaurora.org

---

arch/ia64/hp/common/sba_iommu.c | 2 +- arch/ia64/include/asm/pci.h | 1 - arch/ia64/pci/pci.c | 16 ---------------- arch/ia64/sn/kernel/io_acpi_init.c | 4 ++-- arch/x86/include/asm/pci.h | 3 --- arch/x86/pci/acpi.c | 17 ----------------- drivers/acpi/pci_root.c | 7 ++++++- drivers/pci/probe.c | 2 ++ 8 files changed, 11 insertions(+), 41 deletions(-)

diff --git a/arch/ia64/hp/common/sba_iommu.c b/arch/ia64/hp/common/sba_iommu.c index a6d6190..78e4444 100644 --- a/arch/ia64/hp/common/sba_iommu.c +++ b/arch/ia64/hp/common/sba_iommu.c @@ -1981,7 +1981,7 @@ sba_connect_bus(struct pci_bus *bus) if (PCI_CONTROLLER(bus)->iommu) return;

• handle = acpi_device_handle(PCI_CONTROLLER(bus)->companion);

• handle = acpi_device_handle(ACPI_COMPANION(bus->bridge)); if (!handle) return;

diff --git a/arch/ia64/include/asm/pci.h b/arch/ia64/include/asm/pci.h index c0835b0..12423f4 100644 --- a/arch/ia64/include/asm/pci.h +++ b/arch/ia64/include/asm/pci.h @@ -63,7 +63,6 @@ extern int pci_mmap_legacy_page_range(struct pci_bus *bus, #define pci_legacy_write platform_pci_legacy_write

struct pci_controller {

• struct acpi_device *companion; void *iommu; int segment; int node; /* nearest node with memory or NUMA_NO_NODE for global allocation */

diff --git a/arch/ia64/pci/pci.c b/arch/ia64/pci/pci.c index 8f6ac2f..978d6af 100644 --- a/arch/ia64/pci/pci.c +++ b/arch/ia64/pci/pci.c @@ -301,28 +301,12 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) }

info->controller.segment = root->segment;

• info->controller.companion = device; info->controller.node = acpi_get_node(device->handle); INIT_LIST_HEAD(&info->io_resources); return acpi_pci_root_create(root, &pci_acpi_root_ops, &info->common, &info->controller); }

-int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge) -{

• /*

• * We pass NULL as parent to pci_create_root_bus(), so if it is not NULL
  

• * here, pci_create_root_bus() has been called by someone else and
  

• * sysdata is likely to be different from what we expect.  Let it go in
  

• * that case.
  

• */
  

• if (!bridge->dev.parent) {

• struct pci_controller *controller = bridge->bus->sysdata;
  

• ACPI_COMPANION_SET(&bridge->dev, controller->companion);
  

• }
• return 0;

-}

• void pcibios_fixup_device_resources(struct pci_dev *dev) { int idx;

diff --git a/arch/ia64/sn/kernel/io_acpi_init.c b/arch/ia64/sn/kernel/io_acpi_init.c index 231234c..e454492 100644 --- a/arch/ia64/sn/kernel/io_acpi_init.c +++ b/arch/ia64/sn/kernel/io_acpi_init.c @@ -132,7 +132,7 @@ sn_get_bussoft_ptr(struct pci_bus *bus) struct acpi_resource_vendor_typed *vendor;

• handle = acpi_device_handle(PCI_CONTROLLER(bus)->companion);

• handle = acpi_device_handle(ACPI_COMPANION(bus->bridge)); status = acpi_get_vendor_resource(handle, METHOD_NAME__CRS, &sn_uuid, &buffer); if (ACPI_FAILURE(status)) {

@@ -360,7 +360,7 @@ sn_acpi_get_pcidev_info(struct pci_dev *dev, struct pcidev_info **pcidev_info, acpi_status status; struct acpi_buffer name_buffer = { ACPI_ALLOCATE_BUFFER, NULL };

• rootbus_handle = acpi_device_handle(PCI_CONTROLLER(dev)->companion);

• rootbus_handle = acpi_device_handle(ACPI_COMPANION(dev->bus->bridge)); status = acpi_evaluate_integer(rootbus_handle, METHOD_NAME__SEG, NULL, &segment); if (ACPI_SUCCESS(status)) {

diff --git a/arch/x86/include/asm/pci.h b/arch/x86/include/asm/pci.h index 9ab7507..24de07d 100644 --- a/arch/x86/include/asm/pci.h +++ b/arch/x86/include/asm/pci.h @@ -14,9 +14,6 @@ struct pci_sysdata { int domain; /* PCI domain */ int node; /* NUMA node */ -#ifdef CONFIG_ACPI

• struct acpi_device *companion; /* ACPI companion device */

-#endif #ifdef CONFIG_X86_64 void *iommu; /* IOMMU private data */ #endif diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c index 3cd6983..f4ca17a 100644 --- a/arch/x86/pci/acpi.c +++ b/arch/x86/pci/acpi.c @@ -340,7 +340,6 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) struct pci_sysdata sd = { .domain = domain, .node = node,

• 	.companion = root->device
  

  };

  memcpy(bus->sysdata, &sd, sizeof(sd));

@@ -355,7 +354,6 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) else { info->sd.domain = domain; info->sd.node = node;

• 	info->sd.companion = root->device;
  bus = acpi_pci_root_create(root, &acpi_pci_root_ops,
  			   &info->common, &info->sd);
  

  }

@@ -373,21 +371,6 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) return bus; }

-int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge) -{

• /*

• * We pass NULL as parent to pci_create_root_bus(), so if it is not NULL
  

• * here, pci_create_root_bus() has been called by someone else and
  

• * sysdata is likely to be different from what we expect.  Let it go in
  

• * that case.
  

• */
  

• if (!bridge->dev.parent) {

• struct pci_sysdata *sd = bridge->bus->sysdata;
  

• ACPI_COMPANION_SET(&bridge->dev, sd->companion);
  

• }
• return 0;

-}

• int __init pci_acpi_init(void) { struct pci_dev *dev = NULL;

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c index ae3fe4e..4581e0e 100644 --- a/drivers/acpi/pci_root.c +++ b/drivers/acpi/pci_root.c @@ -564,6 +564,11 @@ static int acpi_pci_root_add(struct acpi_device *device, } }

• /*

• * pci_create_root_bus() needs to detect the parent device type,
  

• * so initialize its companion data accordingly.
  

• */
  

• ACPI_COMPANION_SET(&device->dev, device); root->device = device; root->segment = segment & 0xFFFF; strcpy(acpi_device_name(device), ACPI_PCI_ROOT_DEVICE_NAME);

@@ -846,7 +851,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,

pci_acpi_root_add_resources(info); pci_add_resource(&info->resources, &root->secondary);

• bus = pci_create_root_bus(NULL, busnum, ops->pci_ops,

• bus = pci_create_root_bus(&device->dev, busnum, ops->pci_ops, sysdata, &info->resources);

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

Joining Bjorn's question. Rafael, do you recall what was the issue here from the past?

Yes, I do.

Anything struct acpi_device doesn't represent a real device. It represents a firmware object that can be associated with a device. IOW, nothing under LNXSYSTM:00/ should ever be used as the parent or sibling etc with respect to anything outside of that directory. In fact, the entire LNXSYSTM:00/ should be located under /sys/firmware/acpi/ and it was a mistake to put it under /sys/devices/.

One immediate consequence of the above change, AFAICS, would be that the whole PCI hierarchy would now hang under /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/ which would not make any sense whatever, because /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/physical_node already points to /sys/devices/pci0000:00/.

IOW, both PNP0A08:00/ and pci0000:00/ already represent the same thing, ie. the host bridge.

If you want to have a parent for pci0000:00/, you need a physical_node for LNXSYBUS:00 and point to that as the parent from pci0000:00/. That at least will lead to a sysfs hierarchy that makes sense, although it may break user space tools I suppose (which may be assuming that pci0000:00/ will always be present directly under /sys/devices/).

Thanks, Rafael

On Friday, April 15, 2016 07:06:36 PM Tomasz Nowicki wrote:

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

No, this is not relied on for that. It is just NULL, because we don't have a physical parent device representation for the PCI host bridge.

It looks hacky and prevents us from getting some firmware specific info for PCI host controller based on its acpi_device structure in generic pci_create_root_bus() function.

You can't get that info without adding ACPI-specific code to that function anyway.

However, we overcome that blocker by passing down parent device via pci_create_root_bus parameter (as the ACPI device type).

Which is completely wrong.

Then we use ACPI_COMPANION_SET in core code for ACPI boot method only. ACPI_COMPANION_SET is safe to run for all cases DT, ACPI and DT&ACPI.

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

Suggested-by: Lorenzo Pieralisi lorenzo.pieralisi@arm.com Signed-off-by: Tomasz Nowicki tn@semihalf.com Reviewed-by: Lorenzo Pieralisi lorenzo.pieralisi@arm.com Tested-by: Duc Dang dhdang@apm.com Tested-by: Dongdong Liu liudongdong3@huawei.com Tested-by: Hanjun Guo hanjun.guo@linaro.org Tested-by: Graeme Gregory graeme.gregory@linaro.org Tested-by: Sinan Kaya okaya@codeaurora.org

---

arch/ia64/hp/common/sba_iommu.c | 2 +- arch/ia64/include/asm/pci.h | 1 - arch/ia64/pci/pci.c | 16 ---------------- arch/ia64/sn/kernel/io_acpi_init.c | 4 ++-- arch/x86/include/asm/pci.h | 3 --- arch/x86/pci/acpi.c | 17 ----------------- drivers/acpi/pci_root.c | 7 ++++++- drivers/pci/probe.c | 2 ++ 8 files changed, 11 insertions(+), 41 deletions(-)

diff --git a/arch/ia64/hp/common/sba_iommu.c b/arch/ia64/hp/common/sba_iommu.c index a6d6190..78e4444 100644 --- a/arch/ia64/hp/common/sba_iommu.c +++ b/arch/ia64/hp/common/sba_iommu.c @@ -1981,7 +1981,7 @@ sba_connect_bus(struct pci_bus *bus) if (PCI_CONTROLLER(bus)->iommu) return;

• handle = acpi_device_handle(PCI_CONTROLLER(bus)->companion);

• handle = acpi_device_handle(ACPI_COMPANION(bus->bridge)); if (!handle) return;

diff --git a/arch/ia64/include/asm/pci.h b/arch/ia64/include/asm/pci.h index c0835b0..12423f4 100644 --- a/arch/ia64/include/asm/pci.h +++ b/arch/ia64/include/asm/pci.h @@ -63,7 +63,6 @@ extern int pci_mmap_legacy_page_range(struct pci_bus *bus, #define pci_legacy_write platform_pci_legacy_write struct pci_controller {

• struct acpi_device *companion; void *iommu; int segment; int node; /* nearest node with memory or NUMA_NO_NODE for global allocation */

diff --git a/arch/ia64/pci/pci.c b/arch/ia64/pci/pci.c index 8f6ac2f..978d6af 100644 --- a/arch/ia64/pci/pci.c +++ b/arch/ia64/pci/pci.c @@ -301,28 +301,12 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) } info->controller.segment = root->segment;

• info->controller.companion = device; info->controller.node = acpi_get_node(device->handle); INIT_LIST_HEAD(&info->io_resources); return acpi_pci_root_create(root, &pci_acpi_root_ops, &info->common, &info->controller);

} -int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge) -{

• /*

• * We pass NULL as parent to pci_create_root_bus(), so if it is not NULL
  

• * here, pci_create_root_bus() has been called by someone else and
  

• * sysdata is likely to be different from what we expect.  Let it go in
  

• * that case.
  

• */
  

• if (!bridge->dev.parent) {

• struct pci_controller *controller = bridge->bus->sysdata;
  

• ACPI_COMPANION_SET(&bridge->dev, controller->companion);
  

• }
• return 0;

-}

void pcibios_fixup_device_resources(struct pci_dev *dev) { int idx; diff --git a/arch/ia64/sn/kernel/io_acpi_init.c b/arch/ia64/sn/kernel/io_acpi_init.c index 231234c..e454492 100644 --- a/arch/ia64/sn/kernel/io_acpi_init.c +++ b/arch/ia64/sn/kernel/io_acpi_init.c @@ -132,7 +132,7 @@ sn_get_bussoft_ptr(struct pci_bus *bus) struct acpi_resource_vendor_typed *vendor;

• handle = acpi_device_handle(PCI_CONTROLLER(bus)->companion);

• handle = acpi_device_handle(ACPI_COMPANION(bus->bridge)); status = acpi_get_vendor_resource(handle, METHOD_NAME__CRS, &sn_uuid, &buffer); if (ACPI_FAILURE(status)) {

@@ -360,7 +360,7 @@ sn_acpi_get_pcidev_info(struct pci_dev *dev, struct pcidev_info **pcidev_info, acpi_status status; struct acpi_buffer name_buffer = { ACPI_ALLOCATE_BUFFER, NULL };

• rootbus_handle = acpi_device_handle(PCI_CONTROLLER(dev)->companion);

• rootbus_handle = acpi_device_handle(ACPI_COMPANION(dev->bus->bridge)); status = acpi_evaluate_integer(rootbus_handle, METHOD_NAME__SEG, NULL, &segment); if (ACPI_SUCCESS(status)) {

diff --git a/arch/x86/include/asm/pci.h b/arch/x86/include/asm/pci.h index 9ab7507..24de07d 100644 --- a/arch/x86/include/asm/pci.h +++ b/arch/x86/include/asm/pci.h @@ -14,9 +14,6 @@ struct pci_sysdata { int domain; /* PCI domain */ int node; /* NUMA node */ -#ifdef CONFIG_ACPI

• struct acpi_device *companion; /* ACPI companion device */

-#endif #ifdef CONFIG_X86_64 void *iommu; /* IOMMU private data */ #endif diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c index 3cd6983..f4ca17a 100644 --- a/arch/x86/pci/acpi.c +++ b/arch/x86/pci/acpi.c @@ -340,7 +340,6 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) struct pci_sysdata sd = { .domain = domain, .node = node,

• 	.companion = root->device
  

  };

memcpy(bus->sysdata, &sd, sizeof(sd)); @@ -355,7 +354,6 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) else { info->sd.domain = domain; info->sd.node = node;

• 	info->sd.companion = root->device;
  bus = acpi_pci_root_create(root, &acpi_pci_root_ops,
  			   &info->common, &info->sd);
  

  }

@@ -373,21 +371,6 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) return bus; } -int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge) -{

• /*

• * We pass NULL as parent to pci_create_root_bus(), so if it is not NULL
  

• * here, pci_create_root_bus() has been called by someone else and
  

• * sysdata is likely to be different from what we expect.  Let it go in
  

• * that case.
  

• */
  

• if (!bridge->dev.parent) {

• struct pci_sysdata *sd = bridge->bus->sysdata;
  

• ACPI_COMPANION_SET(&bridge->dev, sd->companion);
  

• }
• return 0;

-}

int __init pci_acpi_init(void) { struct pci_dev *dev = NULL; diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c index ae3fe4e..4581e0e 100644 --- a/drivers/acpi/pci_root.c +++ b/drivers/acpi/pci_root.c @@ -564,6 +564,11 @@ static int acpi_pci_root_add(struct acpi_device *device, } }

• /*

• * pci_create_root_bus() needs to detect the parent device type,
  

• * so initialize its companion data accordingly.
  

• */
  

• ACPI_COMPANION_SET(&device->dev, device);

This is a complete nonsense, sorry.

This just means that the ACPI firmware object will now be an ACPI companion of itself.

Thanks, Rafael

On Fri, Apr 15, 2016 at 07:06:37PM +0200, Tomasz Nowicki wrote:

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 tn@semihalf.com Reviewed-by: Liviu Dudau Liviu.Dudau@arm.com Tested-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com Tested-by: Jeremy Linton jeremy.linton@arm.com Tested-by: Duc Dang dhdang@apm.com Tested-by: Dongdong Liu liudongdong3@huawei.com Tested-by: Hanjun Guo hanjun.guo@linaro.org Tested-by: Graeme Gregory graeme.gregory@linaro.org Tested-by: Sinan Kaya okaya@codeaurora.org

---

drivers/acpi/pci_root.c | 18 ++++++++++++++++++ drivers/pci/pci.c | 11 +++++++++-- include/linux/pci-acpi.h | 2 ++ 3 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c index 4581e0e..d9a70c4 100644 --- a/drivers/acpi/pci_root.c +++ b/drivers/acpi/pci_root.c @@ -419,6 +419,24 @@ out: } EXPORT_SYMBOL(acpi_pci_osc_control_set); +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?

• 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; diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 25e0327..1a74e87 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -19,6 +19,7 @@ #include <linux/spinlock.h> #include <linux/string.h> #include <linux/log2.h> +#include <linux/pci-acpi.h> #include <linux/pci-aspm.h> #include <linux/pm_wakeup.h> #include <linux/interrupt.h> @@ -4779,7 +4780,7 @@ int pci_get_new_domain_nr(void) } #ifdef CONFIG_PCI_DOMAINS_GENERIC -void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent) +static int of_pci_bus_domain_nr(struct device *parent) { static int use_dt_domains = -1; int domain = -1; @@ -4823,7 +4824,13 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent) domain = -1; }

• bus->domain_nr = domain;

• return domain;

+}

+void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent) +{

• bus->domain_nr = acpi_disabled ? of_pci_bus_domain_nr(parent) :

• 			 acpi_pci_bus_domain_nr(parent);
  

} #endif #endif diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h index 89ab057..a72e22d 100644 --- a/include/linux/pci-acpi.h +++ b/include/linux/pci-acpi.h @@ -22,6 +22,7 @@ static inline acpi_status pci_acpi_remove_pm_notifier(struct acpi_device *dev) { return acpi_remove_pm_notifier(dev); } +extern int acpi_pci_bus_domain_nr(struct device *parent); extern phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle); static inline acpi_handle acpi_find_root_bridge_handle(struct pci_dev *pdev) @@ -109,6 +110,7 @@ extern const u8 pci_acpi_dsm_uuid[]; #else /* CONFIG_ACPI */ static inline void acpi_pci_add_bus(struct pci_bus *bus) { } static inline void acpi_pci_remove_bus(struct pci_bus *bus) { } +static inline int acpi_pci_bus_domain_nr(struct device *parent) { return -1; } #endif /* CONFIG_ACPI */

#ifdef CONFIG_ACPI_APEI

1.9.1

-- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html

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:

...

On Fri, Apr 15, 2016 at 07:06:37PM +0200, Tomasz Nowicki wrote:

...

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 tn@semihalf.com Reviewed-by: Liviu Dudau Liviu.Dudau@arm.com Tested-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com Tested-by: Jeremy Linton jeremy.linton@arm.com Tested-by: Duc Dang dhdang@apm.com Tested-by: Dongdong Liu liudongdong3@huawei.com Tested-by: Hanjun Guo hanjun.guo@linaro.org Tested-by: Graeme Gregory graeme.gregory@linaro.org Tested-by: Sinan Kaya okaya@codeaurora.org

---

drivers/acpi/pci_root.c | 18 ++++++++++++++++++ drivers/pci/pci.c | 11 +++++++++-- include/linux/pci-acpi.h | 2 ++ 3 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c index 4581e0e..d9a70c4 100644 --- a/drivers/acpi/pci_root.c +++ b/drivers/acpi/pci_root.c @@ -419,6 +419,24 @@ out: } EXPORT_SYMBOL(acpi_pci_osc_control_set); +int acpi_pci_bus_domain_nr(struct device *parent)

It looks like acpi_pci_bus_domain_nr() could be under #ifdef CONFIG_PCI_DOMAINS_GENERIC, right?

...

...

+{

• 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.

http://www.spinics.net/lists/arm-kernel/msg478169.html

I like neither, we need the lesser of two evils though.

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().

...

...

• 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; diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 25e0327..1a74e87 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -19,6 +19,7 @@ #include <linux/spinlock.h> #include <linux/string.h> #include <linux/log2.h> +#include <linux/pci-acpi.h> #include <linux/pci-aspm.h> #include <linux/pm_wakeup.h> #include <linux/interrupt.h> @@ -4779,7 +4780,7 @@ int pci_get_new_domain_nr(void) } #ifdef CONFIG_PCI_DOMAINS_GENERIC -void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent) +static int of_pci_bus_domain_nr(struct device *parent) { static int use_dt_domains = -1; int domain = -1; @@ -4823,7 +4824,13 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent) domain = -1; }

• bus->domain_nr = domain;

• return domain;

+}

+void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent) +{

• bus->domain_nr = acpi_disabled ? of_pci_bus_domain_nr(parent) :

• 			 acpi_pci_bus_domain_nr(parent);
  

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.

...

...

} #endif #endif diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h index 89ab057..a72e22d 100644 --- a/include/linux/pci-acpi.h +++ b/include/linux/pci-acpi.h @@ -22,6 +22,7 @@ static inline acpi_status pci_acpi_remove_pm_notifier(struct acpi_device *dev) { return acpi_remove_pm_notifier(dev); } +extern int acpi_pci_bus_domain_nr(struct device *parent); extern phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle); static inline acpi_handle acpi_find_root_bridge_handle(struct pci_dev *pdev) @@ -109,6 +110,7 @@ extern const u8 pci_acpi_dsm_uuid[]; #else /* CONFIG_ACPI */ static inline void acpi_pci_add_bus(struct pci_bus *bus) { } static inline void acpi_pci_remove_bus(struct pci_bus *bus) { } +static inline int acpi_pci_bus_domain_nr(struct device *parent) { return -1; } #endif /* CONFIG_ACPI */

#ifdef CONFIG_ACPI_APEI

1.9.1

-- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html

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:

...

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:

...

On Fri, Apr 15, 2016 at 07:06:37PM +0200, Tomasz Nowicki wrote:

...

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 tn@semihalf.com Reviewed-by: Liviu Dudau Liviu.Dudau@arm.com Tested-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com Tested-by: Jeremy Linton jeremy.linton@arm.com Tested-by: Duc Dang dhdang@apm.com Tested-by: Dongdong Liu liudongdong3@huawei.com Tested-by: Hanjun Guo hanjun.guo@linaro.org Tested-by: Graeme Gregory graeme.gregory@linaro.org Tested-by: Sinan Kaya okaya@codeaurora.org

---

drivers/acpi/pci_root.c | 18 ++++++++++++++++++ drivers/pci/pci.c | 11 +++++++++-- include/linux/pci-acpi.h | 2 ++ 3 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c index 4581e0e..d9a70c4 100644 --- a/drivers/acpi/pci_root.c +++ b/drivers/acpi/pci_root.c @@ -419,6 +419,24 @@ out: } EXPORT_SYMBOL(acpi_pci_osc_control_set); +int acpi_pci_bus_domain_nr(struct device *parent)

It looks like acpi_pci_bus_domain_nr() could be under #ifdef CONFIG_PCI_DOMAINS_GENERIC, right?

Yes it should.

...

...

...

...

+{

• 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.

...

http://www.spinics.net/lists/arm-kernel/msg478169.html

I like neither, we need the lesser of two evils though.

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

...

...

...

...

• 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; diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 25e0327..1a74e87 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -19,6 +19,7 @@ #include <linux/spinlock.h> #include <linux/string.h> #include <linux/log2.h> +#include <linux/pci-acpi.h> #include <linux/pci-aspm.h> #include <linux/pm_wakeup.h> #include <linux/interrupt.h> @@ -4779,7 +4780,7 @@ int pci_get_new_domain_nr(void) } #ifdef CONFIG_PCI_DOMAINS_GENERIC -void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent) +static int of_pci_bus_domain_nr(struct device *parent) { static int use_dt_domains = -1; int domain = -1; @@ -4823,7 +4824,13 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent) domain = -1; }

• bus->domain_nr = domain;

• return domain;

+}

+void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent) +{

• bus->domain_nr = acpi_disabled ? of_pci_bus_domain_nr(parent) :

• 			 acpi_pci_bus_domain_nr(parent);
  

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

...

...

...

...

} #endif #endif diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h index 89ab057..a72e22d 100644 --- a/include/linux/pci-acpi.h +++ b/include/linux/pci-acpi.h @@ -22,6 +22,7 @@ static inline acpi_status pci_acpi_remove_pm_notifier(struct acpi_device *dev) { return acpi_remove_pm_notifier(dev); } +extern int acpi_pci_bus_domain_nr(struct device *parent); extern phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle); static inline acpi_handle acpi_find_root_bridge_handle(struct pci_dev *pdev) @@ -109,6 +110,7 @@ extern const u8 pci_acpi_dsm_uuid[]; #else /* CONFIG_ACPI */ static inline void acpi_pci_add_bus(struct pci_bus *bus) { } static inline void acpi_pci_remove_bus(struct pci_bus *bus) { } +static inline int acpi_pci_bus_domain_nr(struct device *parent) { return -1; } #endif /* CONFIG_ACPI */

#ifdef CONFIG_ACPI_APEI

1.9.1

-- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html

On 27.04.2016 04:34, Bjorn Helgaas wrote:

On Fri, Apr 15, 2016 at 07:06:38PM +0200, Tomasz Nowicki wrote:

...

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

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

Signed-off-by: Tomasz Nowicki tn@semihalf.com Reviewed-by: Lorenzo Pieralisi lorenzo.pieralisi@arm.com Tested-by: Duc Dang dhdang@apm.com Tested-by: Dongdong Liu liudongdong3@huawei.com Tested-by: Hanjun Guo hanjun.guo@linaro.org Tested-by: Graeme Gregory graeme.gregory@linaro.org Tested-by: Sinan Kaya okaya@codeaurora.org

---

arch/ia64/pci/pci.c | 10 ---------- arch/x86/pci/common.c | 10 ---------- drivers/pci/probe.c | 3 +++ 3 files changed, 3 insertions(+), 20 deletions(-)

diff --git a/arch/ia64/pci/pci.c b/arch/ia64/pci/pci.c index 978d6af..be4c9ef 100644 --- a/arch/ia64/pci/pci.c +++ b/arch/ia64/pci/pci.c @@ -358,16 +358,6 @@ void pcibios_fixup_bus(struct pci_bus *b) platform_pci_fixup_bus(b); }

-void pcibios_add_bus(struct pci_bus *bus) -{

• acpi_pci_add_bus(bus);

-}

-void pcibios_remove_bus(struct pci_bus *bus) -{

• acpi_pci_remove_bus(bus);

-}

• void pcibios_set_master (struct pci_dev *dev) { /* No special bus mastering setup handling */

diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c index 381a43c..7763a84 100644 --- a/arch/x86/pci/common.c +++ b/arch/x86/pci/common.c @@ -170,16 +170,6 @@ void pcibios_fixup_bus(struct pci_bus *b) pcibios_fixup_device_resources(dev); }

-void pcibios_add_bus(struct pci_bus *bus) -{

• acpi_pci_add_bus(bus);

-}

-void pcibios_remove_bus(struct pci_bus *bus) -{

• acpi_pci_remove_bus(bus);

-}

• /*
  • Only use DMI information to set this if nothing was passed
  • on the kernel command line (which was parsed earlier).

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 8087297..ef569e8 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -12,6 +12,7 @@ #include <linux/slab.h> #include <linux/module.h> #include <linux/cpumask.h> +#include <linux/pci-acpi.h> #include <linux/pci-aspm.h> #include <linux/aer.h> #include <linux/acpi.h> @@ -2101,10 +2102,12 @@ int __weak pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)

void __weak pcibios_add_bus(struct pci_bus *bus) {

• acpi_pci_add_bus(bus); }

  void __weak pcibios_remove_bus(struct pci_bus *bus) {

• acpi_pci_remove_bus(bus); }

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

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

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

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

Thanks, Tomasz

On Tue, Apr 26, 2016 at 09:39:16PM -0500, Bjorn Helgaas wrote:

On Fri, Apr 15, 2016 at 07:06:40PM +0200, Tomasz Nowicki wrote:

...

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

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

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

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

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

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

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

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

What are we going to do with the ACPI version ?

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

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

...

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

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

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

I agree.

Thanks, Lorenzo

...

Signed-off-by: Jayachandran C jchandra@broadcom.com Signed-off-by: Sinan Kaya okaya@codeaurora.org Signed-off-by: Tomasz Nowicki tn@semihalf.com

---

drivers/acpi/pci_root.c | 33 +++++++++++++++++++++++++++++++++ drivers/pci/pci.c | 24 ++++++++++++++++++++++++ include/linux/pci.h | 1 + 3 files changed, 58 insertions(+)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c index d9a70c4..815b6ca 100644 --- a/drivers/acpi/pci_root.c +++ b/drivers/acpi/pci_root.c @@ -742,6 +742,34 @@ next: resource_list_add_tail(entry, resources); } } +static void acpi_pci_root_remap_iospace(struct resource_entry *entry) +{ +#ifdef PCI_IOBASE

• struct resource *res = entry->res;
• resource_size_t cpu_addr = res->start;
• resource_size_t pci_addr = cpu_addr - entry->offset;
• resource_size_t length = resource_size(res);
• unsigned long port;
• if (pci_register_io_range(cpu_addr, length))

• goto err;
  

• port = pci_address_to_pio(cpu_addr);
• if (port == (unsigned long)-1)

• goto err;
  

• res->start = port;
• res->end = port + length - 1;
• entry->offset = port - pci_addr;
• if (pci_remap_iospace(res, cpu_addr) < 0)

• goto err;
  

• pr_info("Remapped I/O %pa to %pR\n", &cpu_addr, res);
• return;

+err:

• res->flags |= IORESOURCE_DISABLED;

+#endif +} int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info) { @@ -763,6 +791,9 @@ int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info) "no IO and memory resources present in _CRS\n"); else { resource_list_for_each_entry_safe(entry, tmp, list) {

• 	if (entry->res->flags & IORESOURCE_IO)
  

• 		acpi_pci_root_remap_iospace(entry);
  

• if (entry->res->flags & IORESOURCE_DISABLED)
  	resource_list_destroy_entry(entry);
  else
  

@@ -834,6 +865,8 @@ static void acpi_pci_root_release_info(struct pci_host_bridge *bridge) resource_list_for_each_entry(entry, &bridge->windows) { res = entry->res;

• if (res->flags & IORESOURCE_IO)
  

• 	pci_unmap_iospace(res);
  

  if (res->parent && (res->flags & (IORESOURCE_MEM | IORESOURCE_IO))) release_resource(res);

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 89e9996..c0f8a4e 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -26,6 +26,7 @@ #include <linux/device.h> #include <linux/pm_runtime.h> #include <linux/pci_hotplug.h> +#include <linux/vmalloc.h> #include <asm/setup.h> #include <linux/aer.h> #include "pci.h" @@ -3168,6 +3169,29 @@ int __weak pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr) #endif } +/**

• • pci_unmap_iospace - Unmap the memory mapped I/O space
• • @res: resource to be unmapped
• • Unmap the CPU virtual address @res from virtual address space.
• • Only architectures that have memory mapped IO functions defined
• • (and the PCI_IOBASE value defined) should call this function.
• */

+void pci_unmap_iospace(struct resource *res) +{ +#if defined(PCI_IOBASE) && defined(CONFIG_MMU)

• unsigned long vaddr = (unsigned long)PCI_IOBASE + res->start;
• unmap_kernel_range(vaddr, resource_size(res));

+#else

• /*

• * This architecture does not have memory mapped I/O space,
  

• * so this function should never be called.
  

• */
  

• WARN_ONCE(1, "This architecture does not support memory mapped I/O\n");

+#endif +}

static void __pci_set_master(struct pci_dev *dev, bool enable) { u16 old_cmd, cmd; diff --git a/include/linux/pci.h b/include/linux/pci.h index c28adb4..df1f33d 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1168,6 +1168,7 @@ int pci_register_io_range(phys_addr_t addr, resource_size_t size); unsigned long pci_address_to_pio(phys_addr_t addr); phys_addr_t pci_pio_to_address(unsigned long pio); int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr); +void pci_unmap_iospace(struct resource *res); static inline pci_bus_addr_t pci_bus_address(struct pci_dev *pdev, int bar) { -- 1.9.1

On Wed, Apr 27, 2016 at 04:10:36PM +0100, Liviu.Dudau@arm.com wrote:

On Wed, Apr 27, 2016 at 03:26:59PM +0100, Lorenzo Pieralisi wrote:

...

On Tue, Apr 26, 2016 at 09:39:16PM -0500, Bjorn Helgaas wrote:

...

On Fri, Apr 15, 2016 at 07:06:40PM +0200, Tomasz Nowicki wrote:

...

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

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

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

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

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

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

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

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

Thanks ! Lorenzo

Best regards, Liviu

...

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

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

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

What are we going to do with the ACPI version ?

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

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

...

...

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

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

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

I agree.

Thanks, Lorenzo

...

...

Signed-off-by: Jayachandran C jchandra@broadcom.com Signed-off-by: Sinan Kaya okaya@codeaurora.org Signed-off-by: Tomasz Nowicki tn@semihalf.com

---

drivers/acpi/pci_root.c | 33 +++++++++++++++++++++++++++++++++ drivers/pci/pci.c | 24 ++++++++++++++++++++++++ include/linux/pci.h | 1 + 3 files changed, 58 insertions(+)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c index d9a70c4..815b6ca 100644 --- a/drivers/acpi/pci_root.c +++ b/drivers/acpi/pci_root.c @@ -742,6 +742,34 @@ next: resource_list_add_tail(entry, resources); } } +static void acpi_pci_root_remap_iospace(struct resource_entry *entry) +{ +#ifdef PCI_IOBASE

• struct resource *res = entry->res;
• resource_size_t cpu_addr = res->start;
• resource_size_t pci_addr = cpu_addr - entry->offset;
• resource_size_t length = resource_size(res);
• unsigned long port;
• if (pci_register_io_range(cpu_addr, length))

• goto err;
  

• port = pci_address_to_pio(cpu_addr);
• if (port == (unsigned long)-1)

• goto err;
  

• res->start = port;
• res->end = port + length - 1;
• entry->offset = port - pci_addr;
• if (pci_remap_iospace(res, cpu_addr) < 0)

• goto err;
  

• pr_info("Remapped I/O %pa to %pR\n", &cpu_addr, res);
• return;

+err:

• res->flags |= IORESOURCE_DISABLED;

+#endif +} int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info) { @@ -763,6 +791,9 @@ int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info) "no IO and memory resources present in _CRS\n"); else { resource_list_for_each_entry_safe(entry, tmp, list) {

• 	if (entry->res->flags & IORESOURCE_IO)
  

• 		acpi_pci_root_remap_iospace(entry);
  

• if (entry->res->flags & IORESOURCE_DISABLED)
  	resource_list_destroy_entry(entry);
  else
  

@@ -834,6 +865,8 @@ static void acpi_pci_root_release_info(struct pci_host_bridge *bridge) resource_list_for_each_entry(entry, &bridge->windows) { res = entry->res;

• if (res->flags & IORESOURCE_IO)
  

• 	pci_unmap_iospace(res);
  

  if (res->parent && (res->flags & (IORESOURCE_MEM | IORESOURCE_IO))) release_resource(res);

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 89e9996..c0f8a4e 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -26,6 +26,7 @@ #include <linux/device.h> #include <linux/pm_runtime.h> #include <linux/pci_hotplug.h> +#include <linux/vmalloc.h> #include <asm/setup.h> #include <linux/aer.h> #include "pci.h" @@ -3168,6 +3169,29 @@ int __weak pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr) #endif } +/**

• • pci_unmap_iospace - Unmap the memory mapped I/O space
• • @res: resource to be unmapped
• • Unmap the CPU virtual address @res from virtual address space.
• • Only architectures that have memory mapped IO functions defined
• • (and the PCI_IOBASE value defined) should call this function.
• */

+void pci_unmap_iospace(struct resource *res) +{ +#if defined(PCI_IOBASE) && defined(CONFIG_MMU)

• unsigned long vaddr = (unsigned long)PCI_IOBASE + res->start;
• unmap_kernel_range(vaddr, resource_size(res));

+#else

• /*

• * This architecture does not have memory mapped I/O space,
  

• * so this function should never be called.
  

• */
  

• WARN_ONCE(1, "This architecture does not support memory mapped I/O\n");

+#endif +}

static void __pci_set_master(struct pci_dev *dev, bool enable) { u16 old_cmd, cmd; diff --git a/include/linux/pci.h b/include/linux/pci.h index c28adb4..df1f33d 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1168,6 +1168,7 @@ int pci_register_io_range(phys_addr_t addr, resource_size_t size); unsigned long pci_address_to_pio(phys_addr_t addr); phys_addr_t pci_pio_to_address(unsigned long pio); int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr); +void pci_unmap_iospace(struct resource *res); static inline pci_bus_addr_t pci_bus_address(struct pci_dev *pdev, int bar) { -- 1.9.1

--

| I would like to | | fix the world, | | but they're not | | giving me the | \ source code! /

---

??\_(???)_/??

On Tue, Apr 26, 2016 at 09:44:30PM -0500, Bjorn Helgaas wrote:

On Fri, Apr 15, 2016 at 07:06:41PM +0200, Tomasz Nowicki wrote:

...

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

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

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

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

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

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

...

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

Signed-off-by: Tomasz Nowicki tn@semihalf.com Suggested-by: Lorenzo Pieralisi lorenzo.pieralisi@arm.com

---

arch/arm64/kernel/pci.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c index c72de66..15109c11 100644 --- a/arch/arm64/kernel/pci.c +++ b/arch/arm64/kernel/pci.c @@ -50,11 +50,16 @@ int pcibios_enable_device(struct pci_dev *dev, int mask) } /*

• • Try to assign the IRQ number from DT when adding a new device

• • Try to assign the IRQ number when probing a new device
  */

-int pcibios_add_device(struct pci_dev *dev) +int pcibios_alloc_irq(struct pci_dev *dev) {

• dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);

• if (acpi_disabled)

• dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
  

+#ifdef CONFIG_ACPI

• else

• return acpi_pci_irq_enable(dev);
  

+#endif

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

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

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

Thanks, Lorenzo

...

return 0; } -- 1.9.1

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

On Fri, Apr 29, 2016 at 3:17 AM, Bjorn Helgaas helgaas@kernel.org wrote:

On Fri, Apr 15, 2016 at 07:06:42PM +0200, Tomasz Nowicki wrote:

...

From: Jayachandran C jchandra@broadcom.com

Add config option PCI_GENERIC_ECAM and file drivers/pci/ecam.c to provide generic functions for accessing memory mapped PCI config space.

The API is defined in drivers/pci/ecam.h and is written to replace the API in drivers/pci/host/pci-host-common.h. The file defines a new 'struct pci_config_window' to hold the information related to a PCI config area and its mapping. This structure is expected to be used as sysdata for controllers that have ECAM based mapping.

Helper functions are provided to setup the mapping, free the mapping and to implement the map_bus method in 'struct pci_ops'

Spec reference: PCI Express Base Specification, rev 3.0, sec 7.2.2.

...

Signed-off-by: Jayachandran C jchandra@broadcom.com

drivers/pci/Kconfig | 3 ++ drivers/pci/Makefile | 2 + drivers/pci/ecam.c | 137 +++++++++++++++++++++++++++++++++++++++++++++++++++ drivers/pci/ecam.h | 61 +++++++++++++++++++++++ 4 files changed, 203 insertions(+) create mode 100644 drivers/pci/ecam.c create mode 100644 drivers/pci/ecam.h

diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig index 209292e..e930d62 100644 --- a/drivers/pci/Kconfig +++ b/drivers/pci/Kconfig @@ -83,6 +83,9 @@ config HT_IRQ config PCI_ATS bool

+config PCI_GENERIC_ECAM

• bool
  

"PCI_ECAM" is enough, I think. It's defined by and required by the spec unless there's some arch-specific interface. Plus, if I

Ok. Looking at the comments I think I have to take out generic from all the names - will do this in next version.

understand correctly, this infrastructure supports non-generic ECAM implementations as well, since the caller supplies "struct pci_generic_ecam_ops *ops".

Yes, the idea was to support ECAM with quirks (and CAM) on both 32 and 64 bit, otherwise it would be too trivial to have a separate implementation.

...

config PCI_IOV bool "PCI IOV support" depends on PCI diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile index 2154092..810aec8 100644 --- a/drivers/pci/Makefile +++ b/drivers/pci/Makefile @@ -55,6 +55,8 @@ obj-$(CONFIG_PCI_SYSCALL) += syscall.o

obj-$(CONFIG_PCI_STUB) += pci-stub.o

+obj-$(CONFIG_PCI_GENERIC_ECAM) += ecam.o

obj-$(CONFIG_XEN_PCIDEV_FRONTEND) += xen-pcifront.o

obj-$(CONFIG_OF) += of.o diff --git a/drivers/pci/ecam.c b/drivers/pci/ecam.c new file mode 100644 index 0000000..ff04c01 --- /dev/null +++ b/drivers/pci/ecam.c @@ -0,0 +1,137 @@ +/*

• • Copyright 2016 Broadcom
• • This program is free software; you can redistribute it and/or modify
• • it under the terms of the GNU General Public License, version 2, as
• • published by the Free Software Foundation (the "GPL").
• • This program is distributed in the hope that it will be useful, but
• • WITHOUT ANY WARRANTY; without even the implied warranty of
• • MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
• • General Public License version 2 (GPLv2) for more details.
• • You should have received a copy of the GNU General Public License
• • version 2 (GPLv2) along with this source code.
• */

+#include <linux/device.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/pci.h> +#include <linux/slab.h>

+#include "ecam.h"

+/*

• • On 64 bit systems, we do a single ioremap for the whole config space
• • since we have enough virtual address range available. On 32 bit, do an
• • ioremap per bus.
• */

+static const bool per_bus_mapping = !config_enabled(CONFIG_64BIT);

+/*

• • Create a PCI config space window
• • • reserve mem region
• • • alloc struct pci_config_window with space for all mappings
• • • ioremap the config space
• */

+struct pci_config_window *pci_generic_ecam_create(struct device *dev,

•                         phys_addr_t addr, u8 bus_start, u8 bus_end,
  

Can you take pointers to struct resources here instead of addr, bus_start, and bus_end? The caller probably has them already, and then you could add a useful printk like:

dev_info(dev, "ECAM for %pR at %pR\n", busn_res, mmio_res);

Would have to be careful about the struct resource lifetimes though.

Yes, I had thought of this. The reason I did not go down that path was that we are using request_mem_region() which takes the address and creates a resource .internally.

Beyond that, as you noted, the ownership and lifetime is slightly more complex. Either the calling code has to allocate the resource and handoff, or the ecam code has to make a copy of the resource. I would go with copy since it is much more simple to use.

Since resource based API seems to be preferred, I will switch to passing bus and mmio resource and will use request_resource_conflict() to allocate the ECAM mem region,

If you had the MMIO resource here, you could also do the range checking you currently have in gen_pci_init() here instead, so all callers could benefit.

Ok.

...

•                         struct pci_generic_ecam_ops *ops)
  

+{

• struct pci_config_window *cfg;
  

• unsigned int bus_shift, bus_range, bsz, mapsz;
  

• int i, nidx;
  

• int err = -ENOMEM;
  

• if (bus_end < bus_start)
  

•         return ERR_PTR(-EINVAL);
  

• bus_shift = ops->bus_shift;
  

• bus_range = bus_end - bus_start + 1;
  

• bsz = 1 << bus_shift;
  

• nidx = per_bus_mapping ? bus_range : 1;
  

• mapsz = per_bus_mapping ? bsz : bus_range * bsz;
  

• cfg = kzalloc(sizeof(*cfg) + nidx * sizeof(cfg->win[0]), GFP_KERNEL);
  

• if (!cfg)
  

•         return ERR_PTR(-ENOMEM);
  

• cfg->bus_start = bus_start;
  

• cfg->bus_end = bus_end;
  

• cfg->ops = ops;
  

• if (!request_mem_region(addr, bus_range * bsz, "Configuration Space"))
  

•         goto err_exit;
  

• /* cfgaddr has to be set after request_mem_region */
  

• cfg->cfgaddr = addr;
  

• for (i = 0; i < nidx; i++) {
  

•         cfg->win[i] = ioremap(addr + i * mapsz, mapsz);
  

•         if (!cfg->win[i])
  

•                 goto err_exit;
  

• }
  

• if (cfg->ops->init) {
  

•         err = cfg->ops->init(dev, cfg);
  

•         if (err)
  

•                 goto err_exit;
  

• }
  

• return cfg;
  

+err_exit:

• pci_generic_ecam_free(cfg);
  

• return ERR_PTR(err);
  

+}

+/*

• • Free a config space mapping
• */

Superfluous comment.

Ok, will trim comments to keep the ones that are useful.

...

+void pci_generic_ecam_free(struct pci_config_window *cfg) +{

• unsigned int bus_range;
  

• int i, nidx;
  

• bus_range = cfg->bus_end - cfg->bus_start + 1;
  

• nidx = per_bus_mapping ? bus_range : 1;
  

• for (i = 0; i < nidx; i++)
  

•         if (cfg->win[i])
  

•                 iounmap(cfg->win[i]);
  

• if (cfg->cfgaddr)
  

•         release_mem_region(cfg->cfgaddr,
  

•                            bus_range << cfg->ops->bus_shift);
  

• kfree(cfg);
  

+}

+/*

• • Function to implement the pci_ops ->map_bus method
• */

+void __iomem *pci_generic_ecam_map_bus(struct pci_bus *bus, unsigned int devfn,

•                                int where)
  

+{

• struct pci_config_window *cfg = bus->sysdata;
  

I don't really like the use of bus->sysdata here, because sysdata is explicitly arch-specific.

But I guess we're in a bind right now: it'd be nice to save the cfg pointer in struct pci_host_bridge, but you have to call pci_generic_ecam_create() before the struct pci_host_bridge has been allocated, and you have to pass the pointer into pci_scan_root_bus(), and there's no generic way to do that yet.

So I guess this will have to do for now.

I could call the structure pci_ecam_sysdata instead of pci_config_window to make it clear that it is to be used as sysdata for ECAM based PCI host controllers.

The current push (at least on ARM/ARM64) seems to be to make bus->sysdata controller specific and avoid arch specific sysdata.

...

• unsigned int devfn_shift = cfg->ops->bus_shift - 8;
  

• unsigned int busn = bus->number;
  

• void __iomem *base;
  

• if (busn < cfg->bus_start || busn > cfg->bus_end)
  

•         return NULL;
  

• busn -= cfg->bus_start;
  

• if (per_bus_mapping)
  

•         base = cfg->win[busn];
  

• else
  

•         base = cfg->win[0] + (busn << cfg->ops->bus_shift);
  

• return base + (devfn << devfn_shift) + where;
  

+}

+/* default ECAM ops */ +struct pci_generic_ecam_ops pci_generic_ecam_default_ops = {

Using both "generic" and "default" seems overkill. Maybe just:

struct pci_ecam_ops pci_generic_ecam_ops = { ... ?

Ok.

...

• .bus_shift      = 20,
  

• .pci_ops                = {
  

•         .map_bus        = pci_generic_ecam_map_bus,
  

•         .read           = pci_generic_config_read,
  

•         .write          = pci_generic_config_write,
  

• }
  

+}; diff --git a/drivers/pci/ecam.h b/drivers/pci/ecam.h new file mode 100644 index 0000000..34c0aba --- /dev/null +++ b/drivers/pci/ecam.h @@ -0,0 +1,61 @@ +/*

• • Copyright 2016 Broadcom
• • This program is free software; you can redistribute it and/or modify
• • it under the terms of the GNU General Public License, version 2, as
• • published by the Free Software Foundation (the "GPL").
• • This program is distributed in the hope that it will be useful, but
• • WITHOUT ANY WARRANTY; without even the implied warranty of
• • MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
• • General Public License version 2 (GPLv2) for more details.
• • You should have received a copy of the GNU General Public License
• • version 2 (GPLv2) along with this source code.
• */

+#ifndef DRIVERS_PCI_ECAM_H +#define DRIVERS_PCI_ECAM_H

+#include <linux/kernel.h> +#include <linux/platform_device.h>

+/*

• • struct to hold pci ops and bus shift of the config window
• • for a PCI controller.
• */

+struct pci_config_window; +struct pci_generic_ecam_ops {

"struct pci_ecam_ops"

Ok.

...

• unsigned int                    bus_shift;
  

• struct pci_ops                  pci_ops;
  

• int                             (*init)(struct device *,
  

•                                         struct pci_config_window *);
  

+};

+/*

• • struct to hold the mappings of a config space window. This
• • will be allocated with enough entries in win[] to hold all
• • the mappings for the bus range.
• */

+struct pci_config_window {

• phys_addr_t                     cfgaddr;
  

• u16                             domain;
  

• u8                              bus_start;
  

• u8                              bus_end;
  

• void                            *priv;
  

• struct pci_generic_ecam_ops     *ops;
  

• void __iomem                    *win[0];
  

+};

+/* create and free for pci_config_window */

Superfluous comment.

...

+struct pci_config_window *pci_generic_ecam_create(struct device *dev,

•                         phys_addr_t addr, u8 bus_start, u8 bus_end,
  

•                         struct pci_generic_ecam_ops *ops);
  

+void pci_generic_ecam_free(struct pci_config_window *cfg);

"pci_ecam_create" and "pci_ecam_free"? I suspect you're going to call these for flavors of ECAM that are definitely not "generic".

...

+/* map_bus when ->sysdata is an instance of pci_config_window */ +void __iomem *pci_generic_ecam_map_bus(struct pci_bus *bus, unsigned int devfn,

•                                int where);
  

+/* default ECAM ops, bus shift 20, generic read and write */ +extern struct pci_generic_ecam_ops pci_generic_ecam_default_ops;

+#endif

Thanks for the review. The ECAM patchset can be merged separately if you do not want to tie it the ACPI changes.

Please let me know how you want to proceed. Depending on that, I will either post it as a separate patchset or ask Tomasz to pick up my changes and post the ACPI patchset. again.

JC.

On 05.05.2016 11:24, Jayachandran C wrote:

On Fri, Apr 29, 2016 at 1:31 PM, Jayachandran C jchandra@broadcom.com wrote:

...

On Fri, Apr 29, 2016 at 3:17 AM, Bjorn Helgaas helgaas@kernel.org wrote:

...

On Fri, Apr 15, 2016 at 07:06:42PM +0200, Tomasz Nowicki wrote:

...

From: Jayachandran C jchandra@broadcom.com

Add config option PCI_GENERIC_ECAM and file drivers/pci/ecam.c to provide generic functions for accessing memory mapped PCI config space.

The API is defined in drivers/pci/ecam.h and is written to replace the API in drivers/pci/host/pci-host-common.h. The file defines a new 'struct pci_config_window' to hold the information related to a PCI config area and its mapping. This structure is expected to be used as sysdata for controllers that have ECAM based mapping.

Helper functions are provided to setup the mapping, free the mapping and to implement the map_bus method in 'struct pci_ops'

Spec reference: PCI Express Base Specification, rev 3.0, sec 7.2.2.

...

Signed-off-by: Jayachandran C jchandra@broadcom.com

drivers/pci/Kconfig | 3 ++ drivers/pci/Makefile | 2 + drivers/pci/ecam.c | 137 +++++++++++++++++++++++++++++++++++++++++++++++++++ drivers/pci/ecam.h | 61 +++++++++++++++++++++++ 4 files changed, 203 insertions(+) create mode 100644 drivers/pci/ecam.c create mode 100644 drivers/pci/ecam.h

diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig index 209292e..e930d62 100644 --- a/drivers/pci/Kconfig +++ b/drivers/pci/Kconfig @@ -83,6 +83,9 @@ config HT_IRQ config PCI_ATS bool

+config PCI_GENERIC_ECAM

• bool
  

"PCI_ECAM" is enough, I think. It's defined by and required by the spec unless there's some arch-specific interface. Plus, if I

Ok. Looking at the comments I think I have to take out generic from all the names - will do this in next version.

...

understand correctly, this infrastructure supports non-generic ECAM implementations as well, since the caller supplies "struct pci_generic_ecam_ops *ops".

Yes, the idea was to support ECAM with quirks (and CAM) on both 32 and 64 bit, otherwise it would be too trivial to have a separate implementation.

...

...

config PCI_IOV bool "PCI IOV support" depends on PCI diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile index 2154092..810aec8 100644 --- a/drivers/pci/Makefile +++ b/drivers/pci/Makefile @@ -55,6 +55,8 @@ obj-$(CONFIG_PCI_SYSCALL) += syscall.o

obj-$(CONFIG_PCI_STUB) += pci-stub.o

+obj-$(CONFIG_PCI_GENERIC_ECAM) += ecam.o

• obj-$(CONFIG_XEN_PCIDEV_FRONTEND) += xen-pcifront.o

  obj-$(CONFIG_OF) += of.o

diff --git a/drivers/pci/ecam.c b/drivers/pci/ecam.c new file mode 100644 index 0000000..ff04c01 --- /dev/null +++ b/drivers/pci/ecam.c @@ -0,0 +1,137 @@ +/*

• • Copyright 2016 Broadcom
• • This program is free software; you can redistribute it and/or modify
• • it under the terms of the GNU General Public License, version 2, as
• • published by the Free Software Foundation (the "GPL").
• • This program is distributed in the hope that it will be useful, but
• • WITHOUT ANY WARRANTY; without even the implied warranty of
• • MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
• • General Public License version 2 (GPLv2) for more details.
• • You should have received a copy of the GNU General Public License
• • version 2 (GPLv2) along with this source code.
• */

+#include <linux/device.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/pci.h> +#include <linux/slab.h>

+#include "ecam.h"

+/*

• • On 64 bit systems, we do a single ioremap for the whole config space
• • since we have enough virtual address range available. On 32 bit, do an
• • ioremap per bus.
• */

+static const bool per_bus_mapping = !config_enabled(CONFIG_64BIT);

+/*

• • Create a PCI config space window
• • • reserve mem region
• • • alloc struct pci_config_window with space for all mappings
• • • ioremap the config space
• */

+struct pci_config_window *pci_generic_ecam_create(struct device *dev,

•                         phys_addr_t addr, u8 bus_start, u8 bus_end,
  

Can you take pointers to struct resources here instead of addr, bus_start, and bus_end? The caller probably has them already, and then you could add a useful printk like:

dev_info(dev, "ECAM for %pR at %pR\n", busn_res, mmio_res);

Would have to be careful about the struct resource lifetimes though.

Yes, I had thought of this. The reason I did not go down that path was that we are using request_mem_region() which takes the address and creates a resource .internally.

Beyond that, as you noted, the ownership and lifetime is slightly more complex. Either the calling code has to allocate the resource and handoff, or the ecam code has to make a copy of the resource. I would go with copy since it is much more simple to use.

Since resource based API seems to be preferred, I will switch to passing bus and mmio resource and will use request_resource_conflict() to allocate the ECAM mem region,

...

If you had the MMIO resource here, you could also do the range checking you currently have in gen_pci_init() here instead, so all callers could benefit.

Ok.

...

...

•                         struct pci_generic_ecam_ops *ops)
  

+{

• struct pci_config_window *cfg;
  

• unsigned int bus_shift, bus_range, bsz, mapsz;
  

• int i, nidx;
  

• int err = -ENOMEM;
  

• if (bus_end < bus_start)
  

•         return ERR_PTR(-EINVAL);
  

• bus_shift = ops->bus_shift;
  

• bus_range = bus_end - bus_start + 1;
  

• bsz = 1 << bus_shift;
  

• nidx = per_bus_mapping ? bus_range : 1;
  

• mapsz = per_bus_mapping ? bsz : bus_range * bsz;
  

• cfg = kzalloc(sizeof(*cfg) + nidx * sizeof(cfg->win[0]), GFP_KERNEL);
  

• if (!cfg)
  

•         return ERR_PTR(-ENOMEM);
  

• cfg->bus_start = bus_start;
  

• cfg->bus_end = bus_end;
  

• cfg->ops = ops;
  

• if (!request_mem_region(addr, bus_range * bsz, "Configuration Space"))
  

•         goto err_exit;
  

• /* cfgaddr has to be set after request_mem_region */
  

• cfg->cfgaddr = addr;
  

• for (i = 0; i < nidx; i++) {
  

•         cfg->win[i] = ioremap(addr + i * mapsz, mapsz);
  

•         if (!cfg->win[i])
  

•                 goto err_exit;
  

• }
  

• if (cfg->ops->init) {
  

•         err = cfg->ops->init(dev, cfg);
  

•         if (err)
  

•                 goto err_exit;
  

• }
  

• return cfg;
  

+err_exit:

• pci_generic_ecam_free(cfg);
  

• return ERR_PTR(err);
  

+}

+/*

• • Free a config space mapping
• */

Superfluous comment.

Ok, will trim comments to keep the ones that are useful.

...

...

+void pci_generic_ecam_free(struct pci_config_window *cfg) +{

• unsigned int bus_range;
  

• int i, nidx;
  

• bus_range = cfg->bus_end - cfg->bus_start + 1;
  

• nidx = per_bus_mapping ? bus_range : 1;
  

• for (i = 0; i < nidx; i++)
  

•         if (cfg->win[i])
  

•                 iounmap(cfg->win[i]);
  

• if (cfg->cfgaddr)
  

•         release_mem_region(cfg->cfgaddr,
  

•                            bus_range << cfg->ops->bus_shift);
  

• kfree(cfg);
  

+}

+/*

• • Function to implement the pci_ops ->map_bus method
• */

+void __iomem *pci_generic_ecam_map_bus(struct pci_bus *bus, unsigned int devfn,

•                                int where)
  

+{

• struct pci_config_window *cfg = bus->sysdata;
  

I don't really like the use of bus->sysdata here, because sysdata is explicitly arch-specific.

But I guess we're in a bind right now: it'd be nice to save the cfg pointer in struct pci_host_bridge, but you have to call pci_generic_ecam_create() before the struct pci_host_bridge has been allocated, and you have to pass the pointer into pci_scan_root_bus(), and there's no generic way to do that yet.

So I guess this will have to do for now.

I could call the structure pci_ecam_sysdata instead of pci_config_window to make it clear that it is to be used as sysdata for ECAM based PCI host controllers.

The current push (at least on ARM/ARM64) seems to be to make bus->sysdata controller specific and avoid arch specific sysdata.

...

...

• unsigned int devfn_shift = cfg->ops->bus_shift - 8;
  

• unsigned int busn = bus->number;
  

• void __iomem *base;
  

• if (busn < cfg->bus_start || busn > cfg->bus_end)
  

•         return NULL;
  

• busn -= cfg->bus_start;
  

• if (per_bus_mapping)
  

•         base = cfg->win[busn];
  

• else
  

•         base = cfg->win[0] + (busn << cfg->ops->bus_shift);
  

• return base + (devfn << devfn_shift) + where;
  

+}

+/* default ECAM ops */ +struct pci_generic_ecam_ops pci_generic_ecam_default_ops = {

Using both "generic" and "default" seems overkill. Maybe just:

struct pci_ecam_ops pci_generic_ecam_ops = { ... ?

Ok.

...

...

• .bus_shift      = 20,
  

• .pci_ops                = {
  

•         .map_bus        = pci_generic_ecam_map_bus,
  

•         .read           = pci_generic_config_read,
  

•         .write          = pci_generic_config_write,
  

• }
  

+}; diff --git a/drivers/pci/ecam.h b/drivers/pci/ecam.h new file mode 100644 index 0000000..34c0aba --- /dev/null +++ b/drivers/pci/ecam.h @@ -0,0 +1,61 @@ +/*

• • Copyright 2016 Broadcom
• • This program is free software; you can redistribute it and/or modify
• • it under the terms of the GNU General Public License, version 2, as
• • published by the Free Software Foundation (the "GPL").
• • This program is distributed in the hope that it will be useful, but
• • WITHOUT ANY WARRANTY; without even the implied warranty of
• • MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
• • General Public License version 2 (GPLv2) for more details.
• • You should have received a copy of the GNU General Public License
• • version 2 (GPLv2) along with this source code.
• */

+#ifndef DRIVERS_PCI_ECAM_H +#define DRIVERS_PCI_ECAM_H

+#include <linux/kernel.h> +#include <linux/platform_device.h>

+/*

• • struct to hold pci ops and bus shift of the config window
• • for a PCI controller.
• */

+struct pci_config_window; +struct pci_generic_ecam_ops {

"struct pci_ecam_ops"

Ok.

...

...

• unsigned int                    bus_shift;
  

• struct pci_ops                  pci_ops;
  

• int                             (*init)(struct device *,
  

•                                         struct pci_config_window *);
  

+};

+/*

• • struct to hold the mappings of a config space window. This
• • will be allocated with enough entries in win[] to hold all
• • the mappings for the bus range.
• */

+struct pci_config_window {

• phys_addr_t                     cfgaddr;
  

• u16                             domain;
  

• u8                              bus_start;
  

• u8                              bus_end;
  

• void                            *priv;
  

• struct pci_generic_ecam_ops     *ops;
  

• void __iomem                    *win[0];
  

+};

+/* create and free for pci_config_window */

Superfluous comment.

...

+struct pci_config_window *pci_generic_ecam_create(struct device *dev,

•                         phys_addr_t addr, u8 bus_start, u8 bus_end,
  

•                         struct pci_generic_ecam_ops *ops);
  

+void pci_generic_ecam_free(struct pci_config_window *cfg);

"pci_ecam_create" and "pci_ecam_free"? I suspect you're going to call these for flavors of ECAM that are definitely not "generic".

...

+/* map_bus when ->sysdata is an instance of pci_config_window */ +void __iomem *pci_generic_ecam_map_bus(struct pci_bus *bus, unsigned int devfn,

•                                int where);
  

+/* default ECAM ops, bus shift 20, generic read and write */ +extern struct pci_generic_ecam_ops pci_generic_ecam_default_ops;

+#endif

Thanks for the review. The ECAM patchset can be merged separately if you do not want to tie it the ACPI changes.

Please let me know how you want to proceed. Depending on that, I will either post it as a separate patchset or ask Tomasz to pick up my changes and post the ACPI patchset. again.

I have made the changes suggested here, and added one more change to move from 'void __iomem *win[0]' to a union for pci_config_window so that it can be embedded in other structures[1].

The changes are at https://github.com/jchandra-brcm/linux branch arm64-acpi-pci-v4 so that Tomasz can add it to v7 of the ACPI/PCI patches. I am not posting the changes here to avoid the impression that there are clashing ACPI/PCI patchsets.

The branch also has an example patch for ACPI generic PCI root using the new ECAM code. This also has the simpler domain setting code for ACPI as well as fix for the ECAM address calculation which came up during the earlier discussion.

JC. [1] Minor API changes will be needed for embedding now. This change will make it easier to merge with Arnd's host bridge changes when that goes in.

Thanks JC!

Tomasz

On Friday 15 April 2016 19:06:43 Tomasz Nowicki wrote:

-MODULE_DEVICE_TABLE(of, thunder_pem_of_match);

-static int thunder_pem_probe(struct platform_device *pdev) +static int thunder_pem_init(struct device *dev, struct pci_config_window *cfg) {

•   struct device *dev = &pdev->dev;
  

•   const struct of_device_id *of_id;
    resource_size_t bar4_start;
    struct resource *res_pem;
    struct thunder_pem_pci *pem_pci;
  

•   struct platform_device *pdev;
  

pem_pci = devm_kzalloc(dev, sizeof(*pem_pci), GFP_KERNEL); if (!pem_pci) return -ENOMEM;

•   of_id = of_match_node(thunder_pem_of_match, dev->of_node);
  

•   pem_pci->gen_pci.cfg.ops = (struct gen_pci_cfg_bus_ops *)of_id->data;
  

•   pdev = to_platform_device(dev);
  

/* * The second register range is the PEM bridge to the PCIe @@ -330,7 +298,29 @@ static int thunder_pem_probe(struct platform_device *pdev) pem_pci->ea_entry[1] = (u32)(res_pem->end - bar4_start) & ~3u; pem_pci->ea_entry[2] = (u32)(bar4_start >> 32);

•   return pci_host_common_probe(pdev, &pem_pci->gen_pci);
  

•   cfg->priv = pem_pci;
  

•   return 0;
  

+}

I still think it would be better to keep the loadable PCI host drivers separate from the ACPI PCI infrastructure. There are a number of simplifications that we want to do to the DT based drivers in the long run, so it's better if that code is not shared at this level. Abstracting out the ECAM code is fine, but at that point you should be able to just call it from the ACPI layer.

Arnd