Hi Hanjun / Mark / Tomasz,
I have tried the following branch on AMD Seattle.
http://git.linaro.org/leg/acpi/acpi.git devel Last commit: f6b94809b273023f675d3504922457c9ffde7e81
I ran info a couple issues. Here are two patches (1 and 2), which should resolve the issues. Please see if they make sense.
Also, patch 3 is needed for parsing _CCA information as a follow up of the CCA patch series here (https://lkml.org/lkml/2015/5/20/1033).
Thanks,
Suravee
Suravee Suthikulpanit (3): ARM64 / PCI / ACPI: Do not return error when a MMCONFIG entry already exist ACPI / PCI: Also look for _PRT in the PCI host bridge device PCI: Generic function for setting up PCI device DMA coherency
arch/arm64/kernel/pci.c | 9 +++++++-- drivers/acpi/pci_irq.c | 46 +++++++++++++++++++++++++++++++++------------- drivers/of/of_pci.c | 20 -------------------- drivers/pci/probe.c | 35 +++++++++++++++++++++++++++++++++-- include/linux/of_pci.h | 3 --- 5 files changed, 73 insertions(+), 40 deletions(-)
During MCFG parsing, the configuration space is added through the following code path:
-- acpi_parse_mcfg() |-- pci_mmconfig_add() |-- pci_mmconfig_alloc()
Then, during PCI root creation, it is trying to re-allocating the config space.
-- acpi_pci_root_add() |-- pci_acpi_scan_root() |-- acpi_pci_root_create() |-- ops->init_info() == pci_acpi_root_init_info() |-- pci_add_mmconfig_region() |-- pci_mmconfig_lookup() |-- pci_mmconfig_alloc()
Therefore, we should be safe to just return if it's already existed.
Signed-off-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com --- arch/arm64/kernel/pci.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c index 5c79f51..3dc82e0 100644 --- a/arch/arm64/kernel/pci.c +++ b/arch/arm64/kernel/pci.c @@ -157,8 +157,13 @@ static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci) info->mcfg_added = false;
cfg = pci_mmconfig_lookup(seg, info->start_bus); - if (cfg) - return -EEXIST; + if (cfg) { + /* Note: if segment already exist. + * This should be okay we should + * simply just return. + */ + return 0; + }
cfg = pci_mmconfig_alloc(seg, info->start_bus, info->end_bus, root->mcfg_addr);
On 2015年05月21日 06:19, Suravee Suthikulpanit wrote:
During MCFG parsing, the configuration space is added through the following code path:
-- acpi_parse_mcfg() |-- pci_mmconfig_add() |-- pci_mmconfig_alloc()Then, during PCI root creation, it is trying to re-allocating the config space.
-- acpi_pci_root_add() |-- pci_acpi_scan_root() |-- acpi_pci_root_create() |-- ops->init_info() == pci_acpi_root_init_info() |-- pci_add_mmconfig_region() |-- pci_mmconfig_lookup() |-- pci_mmconfig_alloc()Therefore, we should be safe to just return if it's already existed.
Signed-off-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com
arch/arm64/kernel/pci.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c index 5c79f51..3dc82e0 100644 --- a/arch/arm64/kernel/pci.c +++ b/arch/arm64/kernel/pci.c @@ -157,8 +157,13 @@ static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci) info->mcfg_added = false;
cfg = pci_mmconfig_lookup(seg, info->start_bus);
- if (cfg)
return -EEXIST;
- if (cfg) {
/* Note: if segment already exist.* This should be okay we should* simply just return.*/return 0;- }
Oh, this is bug, I will squashed your patch to the patch set.
Thanks Hanjun
Hi Suravee,
On 21.05.2015 00:19, Suravee Suthikulpanit wrote:
During MCFG parsing, the configuration space is added through the following code path:
-- acpi_parse_mcfg() |-- pci_mmconfig_add() |-- pci_mmconfig_alloc()Then, during PCI root creation, it is trying to re-allocating the config space.
-- acpi_pci_root_add() |-- pci_acpi_scan_root() |-- acpi_pci_root_create() |-- ops->init_info() == pci_acpi_root_init_info() |-- pci_add_mmconfig_region() |-- pci_mmconfig_lookup() |-- pci_mmconfig_alloc()Therefore, we should be safe to just return if it's already existed.
Signed-off-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com
arch/arm64/kernel/pci.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c index 5c79f51..3dc82e0 100644 --- a/arch/arm64/kernel/pci.c +++ b/arch/arm64/kernel/pci.c @@ -157,8 +157,13 @@ static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci) info->mcfg_added = false;
cfg = pci_mmconfig_lookup(seg, info->start_bus);
- if (cfg)
return -EEXIST;
if (cfg) {
/* Note: if segment already exist.* This should be okay we should* simply just return.*/return 0;}
cfg = pci_mmconfig_alloc(seg, info->start_bus, info->end_bus, root->mcfg_addr);
ACK !
Hanjun,
Any reason why you removed rcu read lock for pci_mmconfig_lookup()? Also, we should do sanity check here for: if (!root->mcfg_addr) return -EINVAL; before new MCFG region injection.
Tomasz
On 2015年05月21日 17:10, Tomasz Nowicki wrote:
Hi Suravee,
On 21.05.2015 00:19, Suravee Suthikulpanit wrote:
During MCFG parsing, the configuration space is added through the following code path:
-- acpi_parse_mcfg() |-- pci_mmconfig_add() |-- pci_mmconfig_alloc()Then, during PCI root creation, it is trying to re-allocating the config space.
-- acpi_pci_root_add() |-- pci_acpi_scan_root() |-- acpi_pci_root_create() |-- ops->init_info() == pci_acpi_root_init_info() |-- pci_add_mmconfig_region() |-- pci_mmconfig_lookup() |-- pci_mmconfig_alloc()Therefore, we should be safe to just return if it's already existed.
Signed-off-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com
arch/arm64/kernel/pci.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c index 5c79f51..3dc82e0 100644 --- a/arch/arm64/kernel/pci.c +++ b/arch/arm64/kernel/pci.c @@ -157,8 +157,13 @@ static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci) info->mcfg_added = false;
cfg = pci_mmconfig_lookup(seg, info->start_bus);
- if (cfg)
return -EEXIST;
if (cfg) {
/* Note: if segment already exist.* This should be okay we should* simply just return.*/return 0;}
cfg = pci_mmconfig_alloc(seg, info->start_bus, info->end_bus, root->mcfg_addr);
ACK !
Hanjun,
Any reason why you removed rcu read lock for pci_mmconfig_lookup()? Also, we should do sanity check here for: if (!root->mcfg_addr) return -EINVAL; before new MCFG region injection.
Just some unknown reason (maybe catathymic amnesia :) ), anyway, I will add them back :)
Thanks Hanjun
On 05/21/2015 08:08 AM, Hanjun Guo wrote:
On 2015年05月21日 17:10, Tomasz Nowicki wrote:
Hi Suravee,
On 21.05.2015 00:19, Suravee Suthikulpanit wrote:
During MCFG parsing, the configuration space is added through the following code path:
-- acpi_parse_mcfg() |-- pci_mmconfig_add() |-- pci_mmconfig_alloc()Then, during PCI root creation, it is trying to re-allocating the config space.
-- acpi_pci_root_add() |-- pci_acpi_scan_root() |-- acpi_pci_root_create() |-- ops->init_info() == pci_acpi_root_init_info() |-- pci_add_mmconfig_region() |-- pci_mmconfig_lookup() |-- pci_mmconfig_alloc()Therefore, we should be safe to just return if it's already existed.
Signed-off-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com
arch/arm64/kernel/pci.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c index 5c79f51..3dc82e0 100644 --- a/arch/arm64/kernel/pci.c +++ b/arch/arm64/kernel/pci.c @@ -157,8 +157,13 @@ static int pci_add_mmconfig_region(struct acpi_pci_root_info *ci) info->mcfg_added = false;
cfg = pci_mmconfig_lookup(seg, info->start_bus);
- if (cfg)
return -EEXIST;
if (cfg) {
/* Note: if segment already exist.* This should be okay we should* simply just return.*/return 0;}
cfg = pci_mmconfig_alloc(seg, info->start_bus, info->end_bus, root->mcfg_addr);
ACK !
Hanjun,
Any reason why you removed rcu read lock for pci_mmconfig_lookup()? Also, we should do sanity check here for: if (!root->mcfg_addr) return -EINVAL; before new MCFG region injection.
Just some unknown reason (maybe catathymic amnesia :) ), anyway, I will add them back :)
Thanks Hanjun
I'm impressed. I actually had to look up "catathymic amnesia". I had never heard the term before. Maybe I suffer from it too :-D.
Current logic does not parse the _PRT object inside the DSDT of PCI host bridge, and result in failing to route PCI legacy interrupt. This patch adds the logic to do so.
Signed-off-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com --- drivers/acpi/pci_irq.c | 46 +++++++++++++++++++++++++++++++++------------- 1 file changed, 33 insertions(+), 13 deletions(-)
diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c index b1def41..671c92c 100644 --- a/drivers/acpi/pci_irq.c +++ b/drivers/acpi/pci_irq.c @@ -157,13 +157,13 @@ static void do_prt_fixups(struct acpi_prt_entry *entry, } }
-static int acpi_pci_irq_check_entry(acpi_handle handle, struct pci_dev *dev, +static int acpi_pci_irq_check_entry(acpi_handle handle, + struct pci_bus *pbus, int device, int pin, struct acpi_pci_routing_table *prt, struct acpi_prt_entry **entry_ptr) { - int segment = pci_domain_nr(dev->bus); - int bus = dev->bus->number; - int device = PCI_SLOT(dev->devfn); + int segment = pci_domain_nr(pbus); + int bus = pbus->number; struct acpi_prt_entry *entry;
if (((prt->address >> 16) & 0xffff) != device || @@ -223,7 +223,7 @@ static int acpi_pci_irq_check_entry(acpi_handle handle, struct pci_dev *dev, return 0; }
-static int acpi_pci_irq_find_prt_entry(struct pci_dev *dev, +static int acpi_pci_irq_find_prt_entry(struct pci_bus *bus, int devid, int pin, struct acpi_prt_entry **entry_ptr) { acpi_status status; @@ -231,8 +231,8 @@ static int acpi_pci_irq_find_prt_entry(struct pci_dev *dev, struct acpi_pci_routing_table *entry; acpi_handle handle = NULL;
- if (dev->bus->bridge) - handle = ACPI_HANDLE(dev->bus->bridge); + if (bus->bridge) + handle = ACPI_HANDLE(bus->bridge);
if (!handle) return -ENODEV; @@ -246,8 +246,8 @@ static int acpi_pci_irq_find_prt_entry(struct pci_dev *dev,
entry = buffer.pointer; while (entry && (entry->length > 0)) { - if (!acpi_pci_irq_check_entry(handle, dev, pin, - entry, entry_ptr)) + if (!acpi_pci_irq_check_entry(handle, bus, devid, + pin, entry, entry_ptr)) break; entry = (struct acpi_pci_routing_table *) ((unsigned long)entry + entry->length); @@ -322,7 +322,7 @@ static struct acpi_prt_entry *acpi_pci_irq_lookup(struct pci_dev *dev, int pin) u8 bridge_pin, orig_pin = pin; int ret;
- ret = acpi_pci_irq_find_prt_entry(dev, pin, &entry); + ret = acpi_pci_irq_find_prt_entry(dev->bus, PCI_SLOT(dev->devfn), pin, &entry); if (!ret && entry) { #ifdef CONFIG_X86_IO_APIC acpi_reroute_boot_interrupt(dev, entry); @@ -336,7 +336,7 @@ static struct acpi_prt_entry *acpi_pci_irq_lookup(struct pci_dev *dev, int pin) * Attempt to derive an IRQ for this device from a parent bridge's * PCI interrupt routing entry (eg. yenta bridge and add-in card bridge). */ - bridge = dev->bus->self; + bridge = pci_upstream_bridge(dev); while (bridge) { pin = pci_swizzle_interrupt_pin(dev, pin);
@@ -352,7 +352,7 @@ static struct acpi_prt_entry *acpi_pci_irq_lookup(struct pci_dev *dev, int pin) pin = bridge_pin; }
- ret = acpi_pci_irq_find_prt_entry(bridge, pin, &entry); + ret = acpi_pci_irq_find_prt_entry(bridge->bus, PCI_SLOT(bridge->devfn), pin, &entry); if (!ret && entry) { ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Derived GSI for %s INT %c from %s\n", @@ -362,7 +362,27 @@ static struct acpi_prt_entry *acpi_pci_irq_lookup(struct pci_dev *dev, int pin) }
dev = bridge; - bridge = dev->bus->self; + bridge = pci_upstream_bridge(dev); + } + + if (!bridge) { + struct device *hb_dev = pci_get_host_bridge_device(dev); + struct pci_host_bridge *h_br = NULL; + + if (hb_dev) + h_br = to_pci_host_bridge(hb_dev); + + if (h_br) + ret = acpi_pci_irq_find_prt_entry(h_br->bus, 0, pin, + &entry); + pci_put_host_bridge_device(hdev); + if (!ret && entry) { + ACPI_DEBUG_PRINT((ACPI_DB_INFO, + "Derived GSI for %s INT %c from %s\n", + pci_name(dev), pin_name(orig_pin), + "host bridge")); + return entry; + } }
dev_warn(&dev->dev, "can't derive routing for PCI INT %c\n",
On 2015年05月21日 06:19, Suravee Suthikulpanit wrote:
Current logic does not parse the _PRT object inside the DSDT of PCI host bridge, and result in failing to route PCI legacy interrupt. This patch adds the logic to do so.
Signed-off-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com
drivers/acpi/pci_irq.c | 46 +++++++++++++++++++++++++++++++++------------- 1 file changed, 33 insertions(+), 13 deletions(-)
diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c index b1def41..671c92c 100644 --- a/drivers/acpi/pci_irq.c +++ b/drivers/acpi/pci_irq.c @@ -157,13 +157,13 @@ static void do_prt_fixups(struct acpi_prt_entry *entry, } }
-static int acpi_pci_irq_check_entry(acpi_handle handle, struct pci_dev *dev, +static int acpi_pci_irq_check_entry(acpi_handle handle,
struct pci_bus *pbus, int device, int pin, struct acpi_pci_routing_table *prt, struct acpi_prt_entry **entry_ptr)
- int segment = pci_domain_nr(dev->bus);
- int bus = dev->bus->number;
- int device = PCI_SLOT(dev->devfn);
int segment = pci_domain_nr(pbus);
int bus = pbus->number; struct acpi_prt_entry *entry;
if (((prt->address >> 16) & 0xffff) != device ||
@@ -223,7 +223,7 @@ static int acpi_pci_irq_check_entry(acpi_handle handle, struct pci_dev *dev, return 0; }
-static int acpi_pci_irq_find_prt_entry(struct pci_dev *dev, +static int acpi_pci_irq_find_prt_entry(struct pci_bus *bus, int devid, int pin, struct acpi_prt_entry **entry_ptr) { acpi_status status; @@ -231,8 +231,8 @@ static int acpi_pci_irq_find_prt_entry(struct pci_dev *dev, struct acpi_pci_routing_table *entry; acpi_handle handle = NULL;
- if (dev->bus->bridge)
handle = ACPI_HANDLE(dev->bus->bridge);
if (bus->bridge)
handle = ACPI_HANDLE(bus->bridge);if (!handle) return -ENODEV;
@@ -246,8 +246,8 @@ static int acpi_pci_irq_find_prt_entry(struct pci_dev *dev,
entry = buffer.pointer; while (entry && (entry->length > 0)) {
if (!acpi_pci_irq_check_entry(handle, dev, pin,entry, entry_ptr))
if (!acpi_pci_irq_check_entry(handle, bus, devid,pin, entry, entry_ptr)) break;@@ -322,7 +322,7 @@ static struct acpi_prt_entry *acpi_pci_irq_lookup(struct pci_dev *dev, int pin) u8 bridge_pin, orig_pin = pin; int ret;
- ret = acpi_pci_irq_find_prt_entry(dev, pin, &entry);
- ret = acpi_pci_irq_find_prt_entry(dev->bus, PCI_SLOT(dev->devfn), pin, &entry); if (!ret && entry) { #ifdef CONFIG_X86_IO_APIC acpi_reroute_boot_interrupt(dev, entry);
@@ -336,7 +336,7 @@ static struct acpi_prt_entry *acpi_pci_irq_lookup(struct pci_dev *dev, int pin) * Attempt to derive an IRQ for this device from a parent bridge's * PCI interrupt routing entry (eg. yenta bridge and add-in card bridge). */
- bridge = dev->bus->self;
- bridge = pci_upstream_bridge(dev); while (bridge) { pin = pci_swizzle_interrupt_pin(dev, pin);
@@ -352,7 +352,7 @@ static struct acpi_prt_entry *acpi_pci_irq_lookup(struct pci_dev *dev, int pin) pin = bridge_pin; }
ret = acpi_pci_irq_find_prt_entry(bridge, pin, &entry);
ret = acpi_pci_irq_find_prt_entry(bridge->bus, PCI_SLOT(bridge->devfn), pin, &entry);@@ -362,7 +362,27 @@ static struct acpi_prt_entry *acpi_pci_irq_lookup(struct pci_dev *dev, int pin) }
dev = bridge;
bridge = dev->bus->self;
bridge = pci_upstream_bridge(dev);- }
It seems that the code above already handle _PRT for root bridge to me, does the PCI bridge topology is special on Seattle?
Thanks Hanjun
if (!bridge) {
struct device *hb_dev = pci_get_host_bridge_device(dev);struct pci_host_bridge *h_br = NULL;if (hb_dev)h_br = to_pci_host_bridge(hb_dev);if (h_br)ret = acpi_pci_irq_find_prt_entry(h_br->bus, 0, pin,&entry);pci_put_host_bridge_device(hdev);if (!ret && entry) {ACPI_DEBUG_PRINT((ACPI_DB_INFO,"Derived GSI for %s INT %c from %s\n",pci_name(dev), pin_name(orig_pin),"host bridge"));return entry;}}
dev_warn(&dev->dev, "can't derive routing for PCI INT %c\n",
Hi Hanjun,
On 5/20/15 22:43, Hanjun Guo wrote:
@@ -322,7 +322,7 @@ static struct acpi_prt_entry *acpi_pci_irq_lookup(struct pci_dev *dev, int pin) u8 bridge_pin, orig_pin = pin; int ret;
- ret = acpi_pci_irq_find_prt_entry(dev, pin, &entry);
- ret = acpi_pci_irq_find_prt_entry(dev->bus, PCI_SLOT(dev->devfn),
pin, &entry); if (!ret && entry) { #ifdef CONFIG_X86_IO_APIC acpi_reroute_boot_interrupt(dev, entry); @@ -336,7 +336,7 @@ static struct acpi_prt_entry *acpi_pci_irq_lookup(struct pci_dev *dev, int pin) * Attempt to derive an IRQ for this device from a parent bridge's * PCI interrupt routing entry (eg. yenta bridge and add-in card bridge). */
- bridge = dev->bus->self;
- bridge = pci_upstream_bridge(dev); while (bridge) { pin = pci_swizzle_interrupt_pin(dev, pin);
@@ -352,7 +352,7 @@ static struct acpi_prt_entry *acpi_pci_irq_lookup(struct pci_dev *dev, int pin) pin = bridge_pin; }
ret = acpi_pci_irq_find_prt_entry(bridge, pin, &entry);
ret = acpi_pci_irq_find_prt_entry(bridge->bus,PCI_SLOT(bridge->devfn), pin, &entry); if (!ret && entry) { ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Derived GSI for %s INT %c from %s\n", @@ -362,7 +362,27 @@ static struct acpi_prt_entry *acpi_pci_irq_lookup(struct pci_dev *dev, int pin) }
dev = bridge;
bridge = dev->bus->self;
bridge = pci_upstream_bridge(dev);- }
It seems that the code above already handle _PRT for root bridge to me, does the PCI bridge topology is special on Seattle?
Thanks Hanjun
I don't thing there is anything special. Here is what the topology looks like:
# lspci -tv -[0000:00]-+-00.0 Advanced Micro Devices, Inc. [AMD] Device 1a00 (Host bridge) +-02.0 Advanced Micro Devices, Inc. [AMD] Device 1a01 -02.1- PCI-PCI bridge - [01]----00.0 Broadcom Corporation NetXtreme BCM5751 Gigabit Ethernet PCI Express
Also, this is what the _PRT looks like:
// // PCIe Root bus // Device (PCI0) { Name (_HID, "PNP0A08") // PCI Express Root Bridge Name (_CID, "PNP0A03") // Compatible PCI Root Bridge Name(_SEG, 0) // Segment of this Root complex Name(_BBN, 0) // Base Bus Number Name (_CCA, 1) // Cache-coherent bus
Name(_PRT, Package(4) { // PCI Routing Table Package(4) {0x000000FFFF, 0, 0, 320}, // A Package(4) {0x000000FFFF, 1, 0, 321}, // B Package(4) {0x000000FFFF, 2, 0, 322}, // C Package(4) {0x000000FFFF, 3, 0, 323} // D })
Thanks, Suravee
On Wed, May 20, 2015 at 05:19:13PM -0500, Suravee Suthikulpanit wrote:
Current logic does not parse the _PRT object inside the DSDT of PCI host bridge, and result in failing to route PCI legacy interrupt. This patch adds the logic to do so.
Signed-off-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com
drivers/acpi/pci_irq.c | 46 +++++++++++++++++++++++++++++++++------------- 1 file changed, 33 insertions(+), 13 deletions(-)
diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c index b1def41..671c92c 100644 --- a/drivers/acpi/pci_irq.c +++ b/drivers/acpi/pci_irq.c @@ -157,13 +157,13 @@ static void do_prt_fixups(struct acpi_prt_entry *entry, } } -static int acpi_pci_irq_check_entry(acpi_handle handle, struct pci_dev *dev, +static int acpi_pci_irq_check_entry(acpi_handle handle,
struct pci_bus *pbus, int device, int pin, struct acpi_pci_routing_table *prt, struct acpi_prt_entry **entry_ptr){
- int segment = pci_domain_nr(dev->bus);
- int bus = dev->bus->number;
- int device = PCI_SLOT(dev->devfn);
- int segment = pci_domain_nr(pbus);
- int bus = pbus->number; struct acpi_prt_entry *entry;
if (((prt->address >> 16) & 0xffff) != device || @@ -223,7 +223,7 @@ static int acpi_pci_irq_check_entry(acpi_handle handle, struct pci_dev *dev, return 0; } -static int acpi_pci_irq_find_prt_entry(struct pci_dev *dev, +static int acpi_pci_irq_find_prt_entry(struct pci_bus *bus, int devid, int pin, struct acpi_prt_entry **entry_ptr) { acpi_status status; @@ -231,8 +231,8 @@ static int acpi_pci_irq_find_prt_entry(struct pci_dev *dev, struct acpi_pci_routing_table *entry; acpi_handle handle = NULL;
- if (dev->bus->bridge)
handle = ACPI_HANDLE(dev->bus->bridge);
- if (bus->bridge)
handle = ACPI_HANDLE(bus->bridge);if (!handle) return -ENODEV; @@ -246,8 +246,8 @@ static int acpi_pci_irq_find_prt_entry(struct pci_dev *dev, entry = buffer.pointer; while (entry && (entry->length > 0)) {
if (!acpi_pci_irq_check_entry(handle, dev, pin,entry, entry_ptr))
if (!acpi_pci_irq_check_entry(handle, bus, devid,pin, entry, entry_ptr)) break;@@ -322,7 +322,7 @@ static struct acpi_prt_entry *acpi_pci_irq_lookup(struct pci_dev *dev, int pin) u8 bridge_pin, orig_pin = pin; int ret;
- ret = acpi_pci_irq_find_prt_entry(dev, pin, &entry);
- ret = acpi_pci_irq_find_prt_entry(dev->bus, PCI_SLOT(dev->devfn), pin, &entry); if (!ret && entry) {
#ifdef CONFIG_X86_IO_APIC acpi_reroute_boot_interrupt(dev, entry); @@ -336,7 +336,7 @@ static struct acpi_prt_entry *acpi_pci_irq_lookup(struct pci_dev *dev, int pin) * Attempt to derive an IRQ for this device from a parent bridge's * PCI interrupt routing entry (eg. yenta bridge and add-in card bridge). */
- bridge = dev->bus->self;
- bridge = pci_upstream_bridge(dev); while (bridge) { pin = pci_swizzle_interrupt_pin(dev, pin);
@@ -352,7 +352,7 @@ static struct acpi_prt_entry *acpi_pci_irq_lookup(struct pci_dev *dev, int pin) pin = bridge_pin; }
ret = acpi_pci_irq_find_prt_entry(bridge, pin, &entry);
ret = acpi_pci_irq_find_prt_entry(bridge->bus, PCI_SLOT(bridge->devfn), pin, &entry);@@ -362,7 +362,27 @@ static struct acpi_prt_entry *acpi_pci_irq_lookup(struct pci_dev *dev, int pin) } dev = bridge;
bridge = dev->bus->self;
bridge = pci_upstream_bridge(dev);- }
- if (!bridge) {
struct device *hb_dev = pci_get_host_bridge_device(dev);struct pci_host_bridge *h_br = NULL;if (hb_dev)h_br = to_pci_host_bridge(hb_dev);if (h_br)ret = acpi_pci_irq_find_prt_entry(h_br->bus, 0, pin,&entry);pci_put_host_bridge_device(hdev);
Was this compile tested? hdev is undefined here according to my compiler
Graeme
if (!ret && entry) {ACPI_DEBUG_PRINT((ACPI_DB_INFO,"Derived GSI for %s INT %c from %s\n",pci_name(dev), pin_name(orig_pin),"host bridge"));return entry;}dev_warn(&dev->dev, "can't derive routing for PCI INT %c\n", -- 2.1.0
Linaro-acpi mailing list Linaro-acpi@lists.linaro.org https://lists.linaro.org/mailman/listinfo/linaro-acpi
Hi Graeme
On 5/21/15 03:03, Graeme Gregory wrote:
- if (!bridge) {
struct device *hb_dev = pci_get_host_bridge_device(dev);struct pci_host_bridge *h_br = NULL;if (hb_dev)h_br = to_pci_host_bridge(hb_dev);if (h_br)ret = acpi_pci_irq_find_prt_entry(h_br->bus, 0, pin,&entry);pci_put_host_bridge_device(hdev);Was this compile tested? hdev is undefined here according to my compiler
Graeme
if (!ret && entry) {ACPI_DEBUG_PRINT((ACPI_DB_INFO,"Derived GSI for %s INT %c from %s\n",pci_name(dev), pin_name(orig_pin),"host bridge"));return entry;}}
dev_warn(&dev->dev, "can't derive routing for PCI INT %c\n",
Sorry, I was cleaning up some code and accidentally missed this part. Here is what I intended to have. This is now retested.
diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c index 671c92c..5658b7d 100644 --- a/drivers/acpi/pci_irq.c +++ b/drivers/acpi/pci_irq.c @@ -375,11 +375,11 @@ static struct acpi_prt_entry *acpi_pci_irq_lookup(struct pci_dev *dev, int pin) if (h_br) ret = acpi_pci_irq_find_prt_entry(h_br->bus, 0, pin, &entry); - pci_put_host_bridge_device(hdev); + pci_put_host_bridge_device(hb_dev); if (!ret && entry) { ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Derived GSI for %s INT %c from %s\n", - pci_name(dev), pin_name(orig_pin), + pci_name(hb_dev), pin_name(orig_pin), "host bridge")); return entry; }
Thanks,
Suravee
On Wed, 2015-05-20 at 17:19 -0500, Suravee Suthikulpanit wrote:
Current logic does not parse the _PRT object inside the DSDT of PCI host bridge, and result in failing to route PCI legacy interrupt. This patch adds the logic to do so.
In what way is it not parsed already? This is all code used by other architectures isn't it? I think the code expects firmware to set up the _PRT for each device/slot. That seems to be the intent in acpi_pci_irq_check_entry() where device id is checked:
if (((prt->address >> 16) & 0xffff) != device || prt->pin + 1 != pin) return -ENODEV;
Signed-off-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com
drivers/acpi/pci_irq.c | 46 +++++++++++++++++++++++++++++++++------------- 1 file changed, 33 insertions(+), 13 deletions(-)
diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c index b1def41..671c92c 100644 --- a/drivers/acpi/pci_irq.c +++ b/drivers/acpi/pci_irq.c @@ -157,13 +157,13 @@ static void do_prt_fixups(struct acpi_prt_entry *entry, } } -static int acpi_pci_irq_check_entry(acpi_handle handle, struct pci_dev *dev, +static int acpi_pci_irq_check_entry(acpi_handle handle,
struct pci_bus *pbus, int device, int pin, struct acpi_pci_routing_table *prt, struct acpi_prt_entry **entry_ptr){
- int segment = pci_domain_nr(dev->bus);
- int bus = dev->bus->number;
- int device = PCI_SLOT(dev->devfn);
- int segment = pci_domain_nr(pbus);
- int bus = pbus->number; struct acpi_prt_entry *entry;
if (((prt->address >> 16) & 0xffff) != device || @@ -223,7 +223,7 @@ static int acpi_pci_irq_check_entry(acpi_handle handle, struct pci_dev *dev, return 0; } -static int acpi_pci_irq_find_prt_entry(struct pci_dev *dev, +static int acpi_pci_irq_find_prt_entry(struct pci_bus *bus, int devid, int pin, struct acpi_prt_entry **entry_ptr) { acpi_status status; @@ -231,8 +231,8 @@ static int acpi_pci_irq_find_prt_entry(struct pci_dev *dev, struct acpi_pci_routing_table *entry; acpi_handle handle = NULL;
- if (dev->bus->bridge)
handle = ACPI_HANDLE(dev->bus->bridge);
- if (bus->bridge)
handle = ACPI_HANDLE(bus->bridge);if (!handle) return -ENODEV; @@ -246,8 +246,8 @@ static int acpi_pci_irq_find_prt_entry(struct pci_dev *dev, entry = buffer.pointer; while (entry && (entry->length > 0)) {
if (!acpi_pci_irq_check_entry(handle, dev, pin,entry, entry_ptr))
if (!acpi_pci_irq_check_entry(handle, bus, devid,pin, entry, entry_ptr)) break;@@ -322,7 +322,7 @@ static struct acpi_prt_entry *acpi_pci_irq_lookup(struct pci_dev *dev, int pin) u8 bridge_pin, orig_pin = pin; int ret;
- ret = acpi_pci_irq_find_prt_entry(dev, pin, &entry);
- ret = acpi_pci_irq_find_prt_entry(dev->bus, PCI_SLOT(dev->devfn), pin, &entry); if (!ret && entry) {
#ifdef CONFIG_X86_IO_APIC acpi_reroute_boot_interrupt(dev, entry); @@ -336,7 +336,7 @@ static struct acpi_prt_entry *acpi_pci_irq_lookup(struct pci_dev *dev, int pin) * Attempt to derive an IRQ for this device from a parent bridge's * PCI interrupt routing entry (eg. yenta bridge and add-in card bridge). */
- bridge = dev->bus->self;
- bridge = pci_upstream_bridge(dev); while (bridge) { pin = pci_swizzle_interrupt_pin(dev, pin);
@@ -352,7 +352,7 @@ static struct acpi_prt_entry *acpi_pci_irq_lookup(struct pci_dev *dev, int pin) pin = bridge_pin; }
ret = acpi_pci_irq_find_prt_entry(bridge, pin, &entry);
ret = acpi_pci_irq_find_prt_entry(bridge->bus, PCI_SLOT(bridge->devfn), pin, &entry);@@ -362,7 +362,27 @@ static struct acpi_prt_entry *acpi_pci_irq_lookup(struct pci_dev *dev, int pin) } dev = bridge;
bridge = dev->bus->self;
bridge = pci_upstream_bridge(dev);- }
- if (!bridge) {
struct device *hb_dev = pci_get_host_bridge_device(dev);struct pci_host_bridge *h_br = NULL;if (hb_dev)h_br = to_pci_host_bridge(hb_dev);if (h_br)ret = acpi_pci_irq_find_prt_entry(h_br->bus, 0, pin,&entry);pci_put_host_bridge_device(hdev);if (!ret && entry) {ACPI_DEBUG_PRINT((ACPI_DB_INFO,"Derived GSI for %s INT %c from %s\n",pci_name(dev), pin_name(orig_pin),"host bridge"));return entry;}dev_warn(&dev->dev, "can't derive routing for PCI INT %c\n",
On 2015年05月21日 21:01, Mark Salter wrote:
On Wed, 2015-05-20 at 17:19 -0500, Suravee Suthikulpanit wrote:
Current logic does not parse the _PRT object inside the DSDT of PCI host bridge, and result in failing to route PCI legacy interrupt. This patch adds the logic to do so.
In what way is it not parsed already? This is all code used by other architectures isn't it? I think the code expects firmware to set up the _PRT for each device/slot. That seems to be the intent in acpi_pci_irq_check_entry() where device id is checked:
if (((prt->address >> 16) & 0xffff) != device || prt->pin + 1 != pin) return -ENODEV;
Yes, I doubt it too, may be the handle of the bridge is NULL?
Could you please try the debug info below and see if I'm right?
Signed-off-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com
drivers/acpi/pci_irq.c | 46 +++++++++++++++++++++++++++++++++------------- 1 file changed, 33 insertions(+), 13 deletions(-)
diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c index b1def41..671c92c 100644 --- a/drivers/acpi/pci_irq.c +++ b/drivers/acpi/pci_irq.c @@ -157,13 +157,13 @@ static void do_prt_fixups(struct acpi_prt_entry *entry, } }
-static int acpi_pci_irq_check_entry(acpi_handle handle, struct pci_dev *dev, +static int acpi_pci_irq_check_entry(acpi_handle handle,
struct pci_bus *pbus, int device, int pin, struct acpi_pci_routing_table *prt, struct acpi_prt_entry **entry_ptr)
- int segment = pci_domain_nr(dev->bus);
- int bus = dev->bus->number;
- int device = PCI_SLOT(dev->devfn);
int segment = pci_domain_nr(pbus);
int bus = pbus->number; struct acpi_prt_entry *entry;
if (((prt->address >> 16) & 0xffff) != device ||
@@ -223,7 +223,7 @@ static int acpi_pci_irq_check_entry(acpi_handle handle, struct pci_dev *dev, return 0; }
-static int acpi_pci_irq_find_prt_entry(struct pci_dev *dev, +static int acpi_pci_irq_find_prt_entry(struct pci_bus *bus, int devid, int pin, struct acpi_prt_entry **entry_ptr) { acpi_status status; @@ -231,8 +231,8 @@ static int acpi_pci_irq_find_prt_entry(struct pci_dev *dev, struct acpi_pci_routing_table *entry; acpi_handle handle = NULL;
- if (dev->bus->bridge)
handle = ACPI_HANDLE(dev->bus->bridge);
- if (bus->bridge)
handle = ACPI_HANDLE(bus->bridge);
pr_info("pci %s: bus %d, bridge %s, handle %p", pci_name(dev), dev->bus->number, dev_name(dev->bus->bridge), handle);
could you please print some info and give me some feedback?
Thanks Hanjun
if (!handle) return -ENODEV; @@ -246,8 +246,8 @@ static int acpi_pci_irq_find_prt_entry(struct pci_dev *dev,
entry = buffer.pointer; while (entry && (entry->length > 0)) {
if (!acpi_pci_irq_check_entry(handle, dev, pin,entry, entry_ptr))
if (!acpi_pci_irq_check_entry(handle, bus, devid,pin, entry, entry_ptr)) break;@@ -322,7 +322,7 @@ static struct acpi_prt_entry *acpi_pci_irq_lookup(struct pci_dev *dev, int pin) u8 bridge_pin, orig_pin = pin; int ret;
- ret = acpi_pci_irq_find_prt_entry(dev, pin, &entry);
- ret = acpi_pci_irq_find_prt_entry(dev->bus, PCI_SLOT(dev->devfn), pin, &entry); if (!ret && entry) { #ifdef CONFIG_X86_IO_APIC acpi_reroute_boot_interrupt(dev, entry);
@@ -336,7 +336,7 @@ static struct acpi_prt_entry *acpi_pci_irq_lookup(struct pci_dev *dev, int pin) * Attempt to derive an IRQ for this device from a parent bridge's * PCI interrupt routing entry (eg. yenta bridge and add-in card bridge). */
- bridge = dev->bus->self;
- bridge = pci_upstream_bridge(dev); while (bridge) { pin = pci_swizzle_interrupt_pin(dev, pin);
@@ -352,7 +352,7 @@ static struct acpi_prt_entry *acpi_pci_irq_lookup(struct pci_dev *dev, int pin) pin = bridge_pin; }
ret = acpi_pci_irq_find_prt_entry(bridge, pin, &entry);
ret = acpi_pci_irq_find_prt_entry(bridge->bus, PCI_SLOT(bridge->devfn), pin, &entry);@@ -362,7 +362,27 @@ static struct acpi_prt_entry *acpi_pci_irq_lookup(struct pci_dev *dev, int pin) }
dev = bridge;
bridge = dev->bus->self;
bridge = pci_upstream_bridge(dev);}
if (!bridge) {
struct device *hb_dev = pci_get_host_bridge_device(dev);struct pci_host_bridge *h_br = NULL;if (hb_dev)h_br = to_pci_host_bridge(hb_dev);if (h_br)ret = acpi_pci_irq_find_prt_entry(h_br->bus, 0, pin,&entry);pci_put_host_bridge_device(hdev);if (!ret && entry) {ACPI_DEBUG_PRINT((ACPI_DB_INFO,"Derived GSI for %s INT %c from %s\n",pci_name(dev), pin_name(orig_pin),"host bridge"));return entry;}}
dev_warn(&dev->dev, "can't derive routing for PCI INT %c\n",
On 5/21/2015 8:57 AM, Hanjun Guo wrote:
On 2015年05月21日 21:01, Mark Salter wrote:
On Wed, 2015-05-20 at 17:19 -0500, Suravee Suthikulpanit wrote:
Current logic does not parse the _PRT object inside the DSDT of PCI host bridge, and result in failing to route PCI legacy interrupt. This patch adds the logic to do so.
In what way is it not parsed already? This is all code used by other architectures isn't it? I think the code expects firmware to set up the _PRT for each device/slot. That seems to be the intent in acpi_pci_irq_check_entry() where device id is checked:
if (((prt->address >> 16) & 0xffff) != device || prt->pin + 1 != pin) return -ENODEV;Yes, I doubt it too, may be the handle of the bridge is NULL?
Could you please try the debug info below and see if I'm right?
So, here is the PCI topology:
# lspci -tv -[0000:00]-+-00.0 Host bridge +-02.0 .... -02.1- PCI-PCI bridge - [01]----00.0 End-point Device
Here is the _PRT from the ACPI table: // // PCIe Root bus // Device (PCI0) { Name (_HID, "PNP0A08") // PCI Express Root Bridge Name (_CID, "PNP0A03") // Compatible PCI Root Bridge Name(_SEG, 0) // Segment of this Root complex Name(_BBN, 0) // Base Bus Number Name (_CCA, 1) // Cache-coherent bus
Name(_PRT, Package(4) { // PCI Routing Table Package(4) {0x000000FFFF, 0, 0, 320}, // A Package(4) {0x000000FFFF, 1, 0, 321}, // B Package(4) {0x000000FFFF, 2, 0, 322}, // C Package(4) {0x000000FFFF, 3, 0, 323} // D })
Here is the debug trace:
[ 7.295381] DEBUG0: acpi_pci_irq_lookup: CHECK DEV : 0000:01:00.0 [ 7.300160] DEBUG: acpi_pci_irq_find_prt_entry: pci=0000:01:00.0: bus=1, bridge=0000:00:02.1, handle=( (null)) [ 7.309716] DEBUG1: acpi_pci_irq_lookup: CHECK BRIDGE : 0000:00:02.1 [ 7.314759] DEBUG: acpi_pci_irq_find_prt_entry: pci=0000:00:02.1: bus=0, bridge=pci0000:00, handle=_SB_.PCI0(fffffe03f3140c80) [ 7.324938] pci 0000:00:02.1: can't derive routing for PCI INT A [ 7.329629] tg3 0000:01:00.0: PCI INT A: no GSI
When the code is calling acpi_pci_irq_find_prt_entry(), we are passing the bridge device (i.e. Device 0:2.1) into the acpi_pci_irq_find_prt_entry(). Here, the logic is trying to match deviceID (2) and pin in the PRT entries above, and it failed to match.
However, the patch I submitted tries to match for device 0 (i.e. the host bridge itself), which now that I think more about it, the _PRT table might not be programmed properly. I am also trying to cross-check if this is how the _PRT is supposed to be programmed. The alternative table would probably be:
Name(_PRT, Package(4) { // PCI Routing Table Package(4) {0x000002FFFF, 0, 0, 320}, // A Package(4) {0x000002FFFF, 1, 0, 321}, // B Package(4) {0x000002FFFF, 2, 0, 322}, // C Package(4) {0x000002FFFF, 3, 0, 323} // D })
Thanks,
Suravee
On 2015年05月22日 01:12, Suravee Suthikulanit wrote:
On 5/21/2015 8:57 AM, Hanjun Guo wrote:
On 2015年05月21日 21:01, Mark Salter wrote:
On Wed, 2015-05-20 at 17:19 -0500, Suravee Suthikulpanit wrote:
Current logic does not parse the _PRT object inside the DSDT of PCI host bridge, and result in failing to route PCI legacy interrupt. This patch adds the logic to do so.
In what way is it not parsed already? This is all code used by other architectures isn't it? I think the code expects firmware to set up the _PRT for each device/slot. That seems to be the intent in acpi_pci_irq_check_entry() where device id is checked:
if (((prt->address >> 16) & 0xffff) != device || prt->pin + 1 != pin) return -ENODEV;Yes, I doubt it too, may be the handle of the bridge is NULL?
Could you please try the debug info below and see if I'm right?
So, here is the PCI topology:
# lspci -tv -[0000:00]-+-00.0 Host bridge +-02.0 .... -02.1- PCI-PCI bridge - [01]----00.0 End-point Device
Here is the _PRT from the ACPI table: // // PCIe Root bus // Device (PCI0) { Name (_HID, "PNP0A08") // PCI Express Root Bridge Name (_CID, "PNP0A03") // Compatible PCI Root Bridge Name(_SEG, 0) // Segment of this Root complex Name(_BBN, 0) // Base Bus Number Name (_CCA, 1) // Cache-coherent bus
Name(_PRT, Package(4) { // PCI Routing Table Package(4) {0x000000FFFF, 0, 0, 320}, // A Package(4) {0x000000FFFF, 1, 0, 321}, // B Package(4) {0x000000FFFF, 2, 0, 322}, // C Package(4) {0x000000FFFF, 3, 0, 323} // D })Here is the debug trace:
[ 7.295381] DEBUG0: acpi_pci_irq_lookup: CHECK DEV : 0000:01:00.0 [ 7.300160] DEBUG: acpi_pci_irq_find_prt_entry: pci=0000:01:00.0: bus=1, bridge=0000:00:02.1, handle=( (null)) [ 7.309716] DEBUG1: acpi_pci_irq_lookup: CHECK BRIDGE : 0000:00:02.1 [ 7.314759] DEBUG: acpi_pci_irq_find_prt_entry: pci=0000:00:02.1: bus=0, bridge=pci0000:00, handle=_SB_.PCI0(fffffe03f3140c80) [ 7.324938] pci 0000:00:02.1: can't derive routing for PCI INT A [ 7.329629] tg3 0000:01:00.0: PCI INT A: no GSI
When the code is calling acpi_pci_irq_find_prt_entry(), we are passing the bridge device (i.e. Device 0:2.1) into the acpi_pci_irq_find_prt_entry(). Here, the logic is trying to match deviceID (2) and pin in the PRT entries above, and it failed to match.
However, the patch I submitted tries to match for device 0 (i.e. the host bridge itself), which now that I think more about it, the _PRT table might not be programmed properly. I am also trying to cross-check if this is how the _PRT is supposed to be programmed. The alternative table would probably be:
Name(_PRT, Package(4) { // PCI Routing Table Package(4) {0x000002FFFF, 0, 0, 320}, // A Package(4) {0x000002FFFF, 1, 0, 321}, // B Package(4) {0x000002FFFF, 2, 0, 322}, // C Package(4) {0x000002FFFF, 3, 0, 323} // D })
Thanks for debugging this, so that's because of the bridge device (parent of endpoint) ID (02) is not match the device ID of host bridge (00), I think the PRT is wrong, not the kernel code itself.
Thanks Hanjun
On Fri, 2015-05-22 at 19:09 +0800, Hanjun Guo wrote:
On 2015年05月22日 01:12, Suravee Suthikulanit wrote:
On 5/21/2015 8:57 AM, Hanjun Guo wrote:
On 2015年05月21日 21:01, Mark Salter wrote:
On Wed, 2015-05-20 at 17:19 -0500, Suravee Suthikulpanit wrote:
Current logic does not parse the _PRT object inside the DSDT of PCI host bridge, and result in failing to route PCI legacy interrupt. This patch adds the logic to do so.
In what way is it not parsed already? This is all code used by other architectures isn't it? I think the code expects firmware to set up the _PRT for each device/slot. That seems to be the intent in acpi_pci_irq_check_entry() where device id is checked:
if (((prt->address >> 16) & 0xffff) != device || prt->pin + 1 != pin) return -ENODEV;Yes, I doubt it too, may be the handle of the bridge is NULL?
Could you please try the debug info below and see if I'm right?
So, here is the PCI topology:
# lspci -tv -[0000:00]-+-00.0 Host bridge +-02.0 .... -02.1- PCI-PCI bridge - [01]----00.0 End-point Device
Here is the _PRT from the ACPI table: // // PCIe Root bus // Device (PCI0) { Name (_HID, "PNP0A08") // PCI Express Root Bridge Name (_CID, "PNP0A03") // Compatible PCI Root Bridge Name(_SEG, 0) // Segment of this Root complex Name(_BBN, 0) // Base Bus Number Name (_CCA, 1) // Cache-coherent bus
Name(_PRT, Package(4) { // PCI Routing Table Package(4) {0x000000FFFF, 0, 0, 320}, // A Package(4) {0x000000FFFF, 1, 0, 321}, // B Package(4) {0x000000FFFF, 2, 0, 322}, // C Package(4) {0x000000FFFF, 3, 0, 323} // D })Here is the debug trace:
[ 7.295381] DEBUG0: acpi_pci_irq_lookup: CHECK DEV : 0000:01:00.0 [ 7.300160] DEBUG: acpi_pci_irq_find_prt_entry: pci=0000:01:00.0: bus=1, bridge=0000:00:02.1, handle=( (null)) [ 7.309716] DEBUG1: acpi_pci_irq_lookup: CHECK BRIDGE : 0000:00:02.1 [ 7.314759] DEBUG: acpi_pci_irq_find_prt_entry: pci=0000:00:02.1: bus=0, bridge=pci0000:00, handle=_SB_.PCI0(fffffe03f3140c80) [ 7.324938] pci 0000:00:02.1: can't derive routing for PCI INT A [ 7.329629] tg3 0000:01:00.0: PCI INT A: no GSI
When the code is calling acpi_pci_irq_find_prt_entry(), we are passing the bridge device (i.e. Device 0:2.1) into the acpi_pci_irq_find_prt_entry(). Here, the logic is trying to match deviceID (2) and pin in the PRT entries above, and it failed to match.
However, the patch I submitted tries to match for device 0 (i.e. the host bridge itself), which now that I think more about it, the _PRT table might not be programmed properly. I am also trying to cross-check if this is how the _PRT is supposed to be programmed. The alternative table would probably be:
Name(_PRT, Package(4) { // PCI Routing Table Package(4) {0x000002FFFF, 0, 0, 320}, // A Package(4) {0x000002FFFF, 1, 0, 321}, // B Package(4) {0x000002FFFF, 2, 0, 322}, // C Package(4) {0x000002FFFF, 3, 0, 323} // D })Thanks for debugging this, so that's because of the bridge device (parent of endpoint) ID (02) is not match the device ID of host bridge (00), I think the PRT is wrong, not the kernel code itself.
And just FYI, on my x86 desktop, the _PRT has entries for every individual PCI device, not just bridges.
On 5/22/15, 08:00, "Mark Salter" msalter@redhat.com wrote:
On Fri, 2015-05-22 at 19:09 +0800, Hanjun Guo wrote:
On 2015年05月22日 01:12, Suravee Suthikulanit wrote:
On 5/21/2015 8:57 AM, Hanjun Guo wrote:
On 2015年05月21日 21:01, Mark Salter wrote:
On Wed, 2015-05-20 at 17:19 -0500, Suravee Suthikulpanit wrote:
Current logic does not parse the _PRT object inside the DSDT of PCI host bridge, and result in failing to route PCI legacy interrupt. This patch
adds
the logic to do so.
In what way is it not parsed already? This is all code used by other architectures isn't it? I think the code expects firmware to set up the _PRT for each device/slot. That seems to be the intent in acpi_pci_irq_check_entry() where device id is checked:
if (((prt->address >> 16) & 0xffff) != device || prt->pin + 1 != pin) return -ENODEV;Yes, I doubt it too, may be the handle of the bridge is NULL?
Could you please try the debug info below and see if I'm right?
So, here is the PCI topology:
# lspci -tv -[0000:00]-+-00.0 Host bridge +-02.0 .... -02.1- PCI-PCI bridge - [01]----00.0 End-point Device
Here is the _PRT from the ACPI table: // // PCIe Root bus // Device (PCI0) { Name (_HID, "PNP0A08") // PCI Express Root Bridge Name (_CID, "PNP0A03") // Compatible PCI Root Bridge Name(_SEG, 0) // Segment of this Root complex Name(_BBN, 0) // Base Bus Number Name (_CCA, 1) // Cache-coherent bus
Name(_PRT, Package(4) { // PCI Routing Table Package(4) {0x000000FFFF, 0, 0, 320}, // A Package(4) {0x000000FFFF, 1, 0, 321}, // B Package(4) {0x000000FFFF, 2, 0, 322}, // C Package(4) {0x000000FFFF, 3, 0, 323} // D })Here is the debug trace:
[ 7.295381] DEBUG0: acpi_pci_irq_lookup: CHECK DEV : 0000:01:00.0 [ 7.300160] DEBUG: acpi_pci_irq_find_prt_entry: pci=0000:01:00.0: bus=1, bridge=0000:00:02.1, handle=( (null)) [ 7.309716] DEBUG1: acpi_pci_irq_lookup: CHECK BRIDGE :
0000:00:02.1
[ 7.314759] DEBUG: acpi_pci_irq_find_prt_entry: pci=0000:00:02.1: bus=0, bridge=pci0000:00, handle=_SB_.PCI0(fffffe03f3140c80) [ 7.324938] pci 0000:00:02.1: can't derive routing for PCI INT A [ 7.329629] tg3 0000:01:00.0: PCI INT A: no GSI
When the code is calling acpi_pci_irq_find_prt_entry(), we are passing the bridge device (i.e. Device 0:2.1) into the acpi_pci_irq_find_prt_entry(). Here, the logic is trying to match deviceID (2) and pin in the PRT entries above, and it failed to match.
However, the patch I submitted tries to match for device 0 (i.e. the host bridge itself), which now that I think more about it, the _PRT table might not be programmed properly. I am also trying to
cross-check
if this is how the _PRT is supposed to be programmed. The alternative table would probably be:
Name(_PRT, Package(4) { // PCI Routing Table Package(4) {0x000002FFFF, 0, 0, 320}, // A Package(4) {0x000002FFFF, 1, 0, 321}, // B Package(4) {0x000002FFFF, 2, 0, 322}, // C Package(4) {0x000002FFFF, 3, 0, 323} // D })Thanks for debugging this, so that's because of the bridge device (parent of endpoint) ID (02) is not match the device ID of host bridge (00), I think the PRT is wrong, not the kernel code itself.
And just FYI, on my x86 desktop, the _PRT has entries for every individual PCI device, not just bridges.
On 5/22/15, 08:00, "Mark Salter" msalter@redhat.com wrote:
On Fri, 2015-05-22 at 19:09 +0800, Hanjun Guo wrote:
On 2015年05月22日 01:12, Suravee Suthikulanit wrote:
On 5/21/2015 8:57 AM, Hanjun Guo wrote:
On 2015年05月21日 21:01, Mark Salter wrote:
On Wed, 2015-05-20 at 17:19 -0500, Suravee Suthikulpanit wrote:
Current logic does not parse the _PRT object inside the DSDT of PCI host bridge, and result in failing to route PCI legacy interrupt. This patch
adds
the logic to do so.
In what way is it not parsed already? This is all code used by other architectures isn't it? I think the code expects firmware to set up the _PRT for each device/slot. That seems to be the intent in acpi_pci_irq_check_entry() where device id is checked:
if (((prt->address >> 16) & 0xffff) != device || prt->pin + 1 != pin) return -ENODEV;Yes, I doubt it too, may be the handle of the bridge is NULL?
Could you please try the debug info below and see if I'm right?
So, here is the PCI topology:
# lspci -tv -[0000:00]-+-00.0 Host bridge +-02.0 .... -02.1- PCI-PCI bridge - [01]----00.0 End-point Device
Here is the _PRT from the ACPI table: // // PCIe Root bus // Device (PCI0) { Name (_HID, "PNP0A08") // PCI Express Root Bridge Name (_CID, "PNP0A03") // Compatible PCI Root Bridge Name(_SEG, 0) // Segment of this Root complex Name(_BBN, 0) // Base Bus Number Name (_CCA, 1) // Cache-coherent bus
Name(_PRT, Package(4) { // PCI Routing Table Package(4) {0x000000FFFF, 0, 0, 320}, // A Package(4) {0x000000FFFF, 1, 0, 321}, // B Package(4) {0x000000FFFF, 2, 0, 322}, // C Package(4) {0x000000FFFF, 3, 0, 323} // D })Here is the debug trace:
[ 7.295381] DEBUG0: acpi_pci_irq_lookup: CHECK DEV : 0000:01:00.0 [ 7.300160] DEBUG: acpi_pci_irq_find_prt_entry: pci=0000:01:00.0: bus=1, bridge=0000:00:02.1, handle=( (null)) [ 7.309716] DEBUG1: acpi_pci_irq_lookup: CHECK BRIDGE :
0000:00:02.1
[ 7.314759] DEBUG: acpi_pci_irq_find_prt_entry: pci=0000:00:02.1: bus=0, bridge=pci0000:00, handle=_SB_.PCI0(fffffe03f3140c80) [ 7.324938] pci 0000:00:02.1: can't derive routing for PCI INT A [ 7.329629] tg3 0000:01:00.0: PCI INT A: no GSI
When the code is calling acpi_pci_irq_find_prt_entry(), we are passing the bridge device (i.e. Device 0:2.1) into the acpi_pci_irq_find_prt_entry(). Here, the logic is trying to match deviceID (2) and pin in the PRT entries above, and it failed to match.
However, the patch I submitted tries to match for device 0 (i.e. the host bridge itself), which now that I think more about it, the _PRT table might not be programmed properly. I am also trying to
cross-check
if this is how the _PRT is supposed to be programmed. The alternative table would probably be:
Name(_PRT, Package(4) { // PCI Routing Table Package(4) {0x000002FFFF, 0, 0, 320}, // A Package(4) {0x000002FFFF, 1, 0, 321}, // B Package(4) {0x000002FFFF, 2, 0, 322}, // C Package(4) {0x000002FFFF, 3, 0, 323} // D })Thanks for debugging this, so that's because of the bridge device (parent of endpoint) ID (02) is not match the device ID of host bridge (00), I think the PRT is wrong, not the kernel code itself.
And just FYI, on my x86 desktop, the _PRT has entries for every individual PCI device, not just bridges.
It is possible and might make sense to specify routing of fixed PCI devices within a system in _PRT if certain routing is preferred. However, for external devices which can be put into a slot, I should be sufficient to just use the routing of the bridge devices.
Thanks,
Suravee
This patch refactors of_pci_dma_configure() into a more generic pci_dma_configure(), which can be reused by non-OF code. Then, it adds support for setting up PCI device DMA coherency from ACPI _CCA object that should normally be specified in the DSDT node of its PCI host bridge..
Signed-off-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com --- drivers/of/of_pci.c | 20 -------------------- drivers/pci/probe.c | 35 +++++++++++++++++++++++++++++++++-- include/linux/of_pci.h | 3 --- 3 files changed, 33 insertions(+), 25 deletions(-)
diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c index 5751dc5..b66ee4e 100644 --- a/drivers/of/of_pci.c +++ b/drivers/of/of_pci.c @@ -117,26 +117,6 @@ int of_get_pci_domain_nr(struct device_node *node) } EXPORT_SYMBOL_GPL(of_get_pci_domain_nr);
-/** - * of_pci_dma_configure - Setup DMA configuration - * @dev: ptr to pci_dev struct of the PCI device - * - * Function to update PCI devices's DMA configuration using the same - * info from the OF node of host bridge's parent (if any). - */ -void of_pci_dma_configure(struct pci_dev *pci_dev) -{ - struct device *dev = &pci_dev->dev; - struct device *bridge = pci_get_host_bridge_device(pci_dev); - - if (!bridge->parent) - return; - - of_dma_configure(dev, bridge->parent->of_node); - pci_put_host_bridge_device(bridge); -} -EXPORT_SYMBOL_GPL(of_pci_dma_configure); - #if defined(CONFIG_OF_ADDRESS) /** * of_pci_get_host_bridge_resources - Parse PCI host bridge resources from DT diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 6675a7a..3d7a78d 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -6,12 +6,14 @@ #include <linux/delay.h> #include <linux/init.h> #include <linux/pci.h> -#include <linux/of_pci.h> +#include <linux/of_device.h> #include <linux/pci_hotplug.h> #include <linux/slab.h> #include <linux/module.h> #include <linux/cpumask.h> #include <linux/pci-aspm.h> +#include <linux/acpi.h> +#include <linux/property.h> #include <asm-generic/pci-bridge.h> #include "pci.h"
@@ -1508,6 +1510,35 @@ static void pci_init_capabilities(struct pci_dev *dev) pci_enable_acs(dev); }
+/** + * pci_dma_configure - Setup DMA configuration + * @pci_dev: ptr to pci_dev struct of the PCI device + * + * Function to update PCI devices's DMA configuration using the same + * info from the OF node or ACPI node of host bridge's parent (if any). + */ +static void pci_dma_configure(struct pci_dev *pci_dev) +{ + struct device *dev = &pci_dev->dev; + struct device *bridge = pci_get_host_bridge_device(pci_dev); + + /* For OF, the bridge->parent point to platform_device.dev. */ + if (IS_ENABLED(CONFIG_OF) && + bridge->parent && + bridge->parent->of_node) { + of_dma_configure(dev, bridge->parent->of_node); + } + /* For ACPI, the bridge point to pci_host_bridge.dev. */ + else { + bool coherent; + + if (acpi_check_dma(ACPI_COMPANION(bridge), &coherent)) + arch_setup_dma_ops(dev, 0, 0, NULL, coherent); + } + + pci_put_host_bridge_device(bridge); +} + void pci_device_add(struct pci_dev *dev, struct pci_bus *bus) { int ret; @@ -1521,7 +1552,7 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus) dev->dev.dma_mask = &dev->dma_mask; dev->dev.dma_parms = &dev->dma_parms; dev->dev.coherent_dma_mask = 0xffffffffull; - of_pci_dma_configure(dev); + pci_dma_configure(dev);
pci_set_dma_max_seg_size(dev, 65536); pci_set_dma_seg_boundary(dev, 0xffffffff); diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h index 29fd3fe..ce0e5ab 100644 --- a/include/linux/of_pci.h +++ b/include/linux/of_pci.h @@ -16,7 +16,6 @@ int of_pci_get_devfn(struct device_node *np); int of_irq_parse_and_map_pci(const struct pci_dev *dev, u8 slot, u8 pin); int of_pci_parse_bus_range(struct device_node *node, struct resource *res); int of_get_pci_domain_nr(struct device_node *node); -void of_pci_dma_configure(struct pci_dev *pci_dev); #else static inline int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq) { @@ -51,8 +50,6 @@ of_get_pci_domain_nr(struct device_node *node) { return -1; } - -static inline void of_pci_dma_configure(struct pci_dev *pci_dev) { } #endif
#if defined(CONFIG_OF_ADDRESS)
On 2015年05月21日 06:19, Suravee Suthikulpanit wrote:
Hi Hanjun / Mark / Tomasz,
Hi Suravee,
I have tried the following branch on AMD Seattle.
http://git.linaro.org/leg/acpi/acpi.git devel Last commit: f6b94809b273023f675d3504922457c9ffde7e81
I ran info a couple issues. Here are two patches (1 and 2), which should resolve the issues. Please see if they make sense.
Also, patch 3 is needed for parsing _CCA information as a follow up of the CCA patch series here (https://lkml.org/lkml/2015/5/20/1033).
Thanks for testing those patches, does PCI work properly on Seattle after applying your 3 patches?
I will review your patches shortly, after that, I will rework the PCI patches then send to upstream for review along with Tomasz's MMCONFIG patches, so can I add your Tested-by: for those patches?
Thanks Hanjun
On 5/20/15 21:43, Hanjun Guo wrote:
On 2015年05月21日 06:19, Suravee Suthikulpanit wrote:
Hi Hanjun / Mark / Tomasz,
Hi Suravee,
I have tried the following branch on AMD Seattle.
http://git.linaro.org/leg/acpi/acpi.git devel Last commit: f6b94809b273023f675d3504922457c9ffde7e81
I ran info a couple issues. Here are two patches (1 and 2), which should resolve the issues. Please see if they make sense.
Also, patch 3 is needed for parsing _CCA information as a follow up of the CCA patch series here (https://lkml.org/lkml/2015/5/20/1033).
Thanks for testing those patches, does PCI work properly on Seattle after applying your 3 patches?
Yes (with legacy interrupts). I'm still doing more testing with different types of devices.
I will review your patches shortly, after that, I will rework the PCI patches then send to upstream for review along with Tomasz's MMCONFIG patches, so can I add your Tested-by: for those patches?
Thanks Hanjun
Sure.
Thanks, Suravee
On 2015年05月21日 19:14, Suravee Suthikulpanit wrote:
On 5/20/15 21:43, Hanjun Guo wrote:
On 2015年05月21日 06:19, Suravee Suthikulpanit wrote:
Hi Hanjun / Mark / Tomasz,
Hi Suravee,
I have tried the following branch on AMD Seattle.
http://git.linaro.org/leg/acpi/acpi.git devel Last commit: f6b94809b273023f675d3504922457c9ffde7e81
I ran info a couple issues. Here are two patches (1 and 2), which should resolve the issues. Please see if they make sense.
Also, patch 3 is needed for parsing _CCA information as a follow up of the CCA patch series here (https://lkml.org/lkml/2015/5/20/1033).
Thanks for testing those patches, does PCI work properly on Seattle after applying your 3 patches?
Yes (with legacy interrupts). I'm still doing more testing with different types of device
OK, thanks. so the conclusion is that Jiang Liu patch is no problem for now, right? if yes, I will reply him it's good to go, and speed up the upstream process.
Thanks Hanjun
On 5/21/2015 9:03 AM, Hanjun Guo wrote:
On 2015年05月21日 19:14, Suravee Suthikulpanit wrote:
On 5/20/15 21:43, Hanjun Guo wrote:
On 2015年05月21日 06:19, Suravee Suthikulpanit wrote:
Hi Hanjun / Mark / Tomasz,
Hi Suravee,
I have tried the following branch on AMD Seattle.
http://git.linaro.org/leg/acpi/acpi.git devel Last commit: f6b94809b273023f675d3504922457c9ffde7e81
I ran info a couple issues. Here are two patches (1 and 2), which should resolve the issues. Please see if they make sense.
Also, patch 3 is needed for parsing _CCA information as a follow up of the CCA patch series here (https://lkml.org/lkml/2015/5/20/1033).
Thanks for testing those patches, does PCI work properly on Seattle after applying your 3 patches?
Yes (with legacy interrupts). I'm still doing more testing with different types of device
OK, thanks. so the conclusion is that Jiang Liu patch is no problem for now, right? if yes, I will reply him it's good to go, and speed up the upstream process.
Thanks Hanjun
Sure.
Thanks, Suravee
-
Al Stone -
Graeme Gregory -
Hanjun Guo -
Mark Salter -
Suravee Suthikulanit -
Suravee Suthikulpanit
-
Suthikulpanit, Suravee
-
Tomasz Nowicki