Re: [Resend PATCH 5/5] IA64/PCI/ACPI: Rework PCI root bridge ACPIresource conversion
From: Lan Tianyu
Date: Fri Oct 18 2013 - 08:44:24 EST
On 10/18/2013 04:33 AM, Bjorn Helgaas wrote:
[+cc Mike]
On Thu, Oct 17, 2013 at 12:09 AM, Lan Tianyu <tianyu.lan@xxxxxxxxx>
wrote:
On 2013å10æ17æ 07:55, Bjorn Helgaas wrote:
On Fri, Oct 11, 2013 at 08:19:01PM +0800, tianyu.lan@xxxxxxxxx
wrote:
From: Lan Tianyu <tianyu.lan@xxxxxxxxx>
Using ACPI resource functions to convert ACPI resource to
generic resource
Signed-off-by: Lan Tianyu <tianyu.lan@xxxxxxxxx> --- This patch
just passes through compilation test due to no ia64 machine on
hand.
arch/ia64/pci/pci.c | 38
+++++++++++++++++++++----------------- 1 file changed, 21
insertions(+), 17 deletions(-)
diff --git a/arch/ia64/pci/pci.c b/arch/ia64/pci/pci.c index
2326790..14fa175 100644 --- a/arch/ia64/pci/pci.c +++
b/arch/ia64/pci/pci.c @@ -232,8 +232,9 @@ out: return ~0; }
-static acpi_status resource_to_window(struct acpi_resource
*resource, - struct
acpi_resource_address64 *addr) +static acpi_status
resource_to_window(struct acpi_resource *ares, +
struct acpi_resource_address64 *addr, +
struct resource *res) { acpi_status status;
@@ -244,7 +245,7 @@ static acpi_status
resource_to_window(struct acpi_resource *resource, * -
producers, i.e., the address space is routed downstream, *
not consumed by the bridge itself */ - status =
acpi_resource_to_address64(resource, addr); + status =
acpi_dev_resource_address_space_full(ares, addr, res); if
(ACPI_SUCCESS(status) && (addr->resource_type ==
ACPI_MEMORY_RANGE || addr->resource_type == ACPI_IO_RANGE) &&
@@ -255,51 +256,54 @@ static acpi_status
resource_to_window(struct acpi_resource *resource, return
AE_ERROR; }
-static acpi_status count_window(struct acpi_resource
*resource, void *data) +static acpi_status count_window(struct
acpi_resource *ares, void *data) { unsigned int *windows =
(unsigned int *) data; struct acpi_resource_address64 addr; +
struct resource res; acpi_status status;
- status = resource_to_window(resource, &addr); + status
= resource_to_window(ares, &addr, &res); if
(ACPI_SUCCESS(status)) (*windows)++;
return AE_OK; }
-static acpi_status add_window(struct acpi_resource *res, void
*data) +static acpi_status add_window(struct acpi_resource
*ares, void *data) { struct pci_root_info *info = data; -
struct resource *resource; + struct resource *resource =
&info->res[info->res_num]; struct acpi_resource_address64
addr; acpi_status status; - unsigned long flags, offset =
0; + unsigned long offset = 0; struct resource *root;
/* Return AE_OK for non-window resources to keep scanning for
more */ - status = resource_to_window(res, &addr); +
status = resource_to_window(ares, &addr, resource); if
(!ACPI_SUCCESS(status)) return AE_OK;
- if (addr.resource_type == ACPI_MEMORY_RANGE) { -
flags = IORESOURCE_MEM; + if (resource->flags &
IORESOURCE_MEM) { root = &iomem_resource; offset =
addr.translation_offset; - } else if (addr.resource_type ==
ACPI_IO_RANGE) { - flags = IORESOURCE_IO; + }
else if (resource->flags & IORESOURCE_IO) { root =
&ioport_resource; offset = add_io_space(info, &addr); if
(offset == ~0) return AE_OK; + + /* + *
io space address translation offset depends + * on
the return value of add_io_space(). So + *
Repopulate resource->start and end here.
"Repopulate" makes it sound like "resource->start" got clobbered
somewhere. But it didn't. I think it's just that each bridge
can support its own I/O port range, and the PCI port numbers
reported in the acpi_resource may not be distinct, and
add_io_space() adds an offset so all the I/O port ranges fit into
one global I/O port space.
For example, I think these two bridges have the same port
numbers (0x0-0xfff) in their acpi_resource, but the second has an
offset of 0x1000000 in the system I/O port space, and I think
this offset is what add_io_space() returns:
HWP0002:00: host bridge window [io 0x0000-0x0fff] (PCI
[0x0-0xfff]) HWP0002:09: host bridge window [io
0x1000000-0x1000fff] (PCI [0x0-0xfff])
+ */ + resource->start = addr.minimum +
offset; + resource->end = resource->start +
addr.address_length - 1;
Can't we use this:
resource->start += offset; resource->end += offset;
to avoid breaking the encapsulation of struct
acpi_resource_address64?
resource->start has been populated with "addr.minimum +
addr.translation_offset" in the acpi_dev_resource_address_space().
That's true, but this is a change from previous behavior.
Previously, x86 applied addr.translation_offset to both MEM and IO
resources (in setup_resource()), but ia64 applied it only to MEM
resources (in add_window()). With your patch, we apply it to both
types in acpi_dev_resource_address_space(), which is a change for
ia64.
Yes, this is why I repopulate resource->start and ->end after
add_io_space().
I know translation_offset is used on some HP ia64 boxes, but I'm not
aware of it being used for IO resources on any x86 boxes. On those
ia64 boxes, the bridge also does type translation (the resource is
MEM on the primary side but IO on the secondary side). In that case,
I'm not sure it makes sense to add the translation_offset to an IO
address and expect the result to be a MEM address.
On these HP ia64 boxes, the firmware puts the CPU physical address
of the MEM resource in the translation_offset (see the call to
new_space()). The bridge then translates that MEM resource to IO on
the secondary side. It's awfully hard for me to extract this usage
from the ACPI spec, so possibly this is just a quirk of the way HP
encoded these IO resources. But it *is* a precedent, and I'm not
aware of anybody doing anything that conflicts with it.
Thanks for sharing and let me know some background about this. Honestly,
I just change the code just according to the original. It's good to have
a chance to see such kind of machine's acpidump.
I wonder if it would make sense to make
acpi_dev_resource_address_space() ignore addr.translation_offset for
IO resources. Or maybe ignore it if the _TTP (type translation) bit
is set?
I wonder why current code doesn't check _TTP? The code in the
add_io_space() seems to think _TTP is always set, right?
I think the main intent of translation_offset (_TRA) is to map a
smaller address space into part of a larger space of the same type,
e.g., a 32-bit PCI memory space into a 40+ bit CPU memory space.
That doesn't apply directly to IO ports, because I don't think any
CPU has a native IO port address space larger than 16 bits, so
there's no extra space to map into.
Mike, is there any chance you could collect an acpidump from an
rx7620 or similar ia64 system? In particular, I want to see a
multi-node system where we have several PCI domains, and whether it
sets the _TTP bits.
continuing to add the offset to resource->start seems not right.
The add_io_space() accepts translation_offset and then ioremap it
to mmio address. Add the result to io_space array and assign a
space number. Left shift the space number 24 bits as the return
offset of add_io_space().
When one io port address is accessed, __ia64_mk_io_addr() will do
reverse operations and find associated mmio address.
Yep, got it. I wrote all that code originally :) Obviously it
hasn't turned out to be particularly easy to understand.
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/