[Resend,5/5] IA64/PCI/ACPI: Rework PCI root bridge ACPI resource conversion

Commit Message

lan,Tianyu Oct. 11, 2013, 12:19 p.m. UTC

From: Lan Tianyu <[e-mail]>

Using ACPI resource functions to convert ACPI resource to generic resource

Signed-off-by: Lan Tianyu <[e-mail]>
---
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(-)

Comments

Rafael Wysocki Oct. 15, 2013, 11:23 p.m. UTC | #1

On Friday, October 11, 2013 08:19:01 PM [e-mail] wrote:
> From: Lan Tianyu <[e-mail]>
> 
> Using ACPI resource functions to convert ACPI resource to generic resource
> 
> Signed-off-by: Lan Tianyu <[e-mail]>

Tony, any objections against this?

> ---
> 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.
> +		 */
> +		resource->start = addr.minimum + offset;
> +		resource->end = resource->start + addr.address_length - 1;
>  	} else
>  		return AE_OK;
>  
> -	resource = &info->res[info->res_num];
>  	resource->name = info->name;
> -	resource->flags = flags;
> -	resource->start = addr.minimum + offset;
> -	resource->end = resource->start + addr.address_length - 1;
>  	info->res_offset[info->res_num] = offset;
>  
>  	if (insert_resource(root, resource)) {
>

Bjorn Helgaas Oct. 16, 2013, 11:55 p.m. UTC | #2

On Fri, Oct 11, 2013 at 08:19:01PM +0800, [e-mail] wrote:
> From: Lan Tianyu <[e-mail]>
> 
> Using ACPI resource functions to convert ACPI resource to generic resource
> 
> Signed-off-by: Lan Tianyu <[e-mail]>
> ---
> 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?

>  	} else
>  		return AE_OK;
>  
> -	resource = &info->res[info->res_num];
>  	resource->name = info->name;
> -	resource->flags = flags;
> -	resource->start = addr.minimum + offset;
> -	resource->end = resource->start + addr.address_length - 1;
>  	info->res_offset[info->res_num] = offset;
>  
>  	if (insert_resource(root, resource)) {
> -- 
> 1.8.2.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

lan,Tianyu Oct. 17, 2013, 6:09 a.m. UTC | #3

On 2013?10?17? 07:55, Bjorn Helgaas wrote:
> On Fri, Oct 11, 2013 at 08:19:01PM +0800, [e-mail] wrote:
>> From: Lan Tianyu <[e-mail]>
>>
>> Using ACPI resource functions to convert ACPI resource to generic resource
>>
>> Signed-off-by: Lan Tianyu <[e-mail]>
>> ---
>> 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().
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.


> 
>>  	} else
>>  		return AE_OK;
>>  
>> -	resource = &info->res[info->res_num];
>>  	resource->name = info->name;
>> -	resource->flags = flags;
>> -	resource->start = addr.minimum + offset;
>> -	resource->end = resource->start + addr.address_length - 1;
>>  	info->res_offset[info->res_num] = offset;
>>  
>>  	if (insert_resource(root, resource)) {
>> -- 
>> 1.8.2.1
>>

Bjorn Helgaas Oct. 17, 2013, 8:33 p.m. UTC | #4

[+cc Mike]

On Thu, Oct 17, 2013 at 12:09 AM, Lan Tianyu <[e-mail]> wrote:
> On 2013?10?17? 07:55, Bjorn Helgaas wrote:
>> On Fri, Oct 11, 2013 at 08:19:01PM +0800, [e-mail] wrote:
>>> From: Lan Tianyu <[e-mail]>
>>>
>>> Using ACPI resource functions to convert ACPI resource to generic resource
>>>
>>> Signed-off-by: Lan Tianyu <[e-mail]>
>>> ---
>>> 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.

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.

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 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-acpi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

lan,Tianyu Oct. 18, 2013, 12:44 p.m. UTC | #5

On 10/18/2013 04:33 AM, Bjorn Helgaas wrote:
> [+cc Mike]
>
> On Thu, Oct 17, 2013 at 12:09 AM, Lan Tianyu <[e-mail]>
> wrote:
>> On 2013?10?17? 07:55, Bjorn Helgaas wrote:
>>> On Fri, Oct 11, 2013 at 08:19:01PM +0800, [e-mail]
>>> wrote:
>>>> From: Lan Tianyu <[e-mail]>
>>>>
>>>> Using ACPI resource functions to convert ACPI resource to
>>>> generic resource
>>>>
>>>> Signed-off-by: Lan Tianyu <[e-mail]> --- 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-acpi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Bjorn Helgaas Oct. 23, 2013, 10:39 p.m. UTC | #6

[+cc Greg]

On Fri, Oct 18, 2013 at 08:44:12PM +0800, Lan Tianyu wrote:
> On 10/18/2013 04:33 AM, Bjorn Helgaas wrote:
>> On Thu, Oct 17, 2013 at 12:09 AM, Lan Tianyu <[e-mail]> wrote:
>>>On 2013?10?17? 07:55, Bjorn Helgaas wrote:
>>>> On Fri, Oct 11, 2013 at 08:19:01PM +0800, [e-mail] wrote:

>>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 it's an oversight, and you should fix it.  I suggest that you
ignore the _TRA value when _TTP is set.  Obviously this only applies
to I/O port resources, since _TTP is only defined in the I/O Resource
Flag (Table 6-185 in ACPI 5.0 spec).

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

Greg collected an acpidump from an HP system that uses these I/O port
ranges.  Unfortunately the system wasn't running Linux, so it's an EFI
dump, not the usual one we get from the "pmtools" package.  But I
think it has the information we want.

It's huge, and I put some of the relevant parts of it here:
https://bugzilla.kernel.org/show_bug.cgi?id=63581  Here's a sample
that shows the _TTP bit is set for the I/O aperture:

    Device E000 (\_SB_.N000.E000)
      Name _SEG (\_SB_.N000.E000._SEG)
        0x01
      Name _CRS (\_SB_.N000.E001._CRS)
        Buffer
          0x0092
          ByteList <0x88 0x0d 0x00 0x02 0x0e 0x00 0x00 0x00
                    0x00 0x00 0xff 0x00 0x00 0x00 0x00 0x01

                    0x8a 0x2b 0x00 0x01 0x0c 0x33 0x00 0x00
                    0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
                    0x00 0x00 0x00 0x00 0x00 0x00 0xff 0xff
                    0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
                    0x00 0xd0 0x00 0x04 0x00 0x00 0x00 0x00
                    0x01 0x00 0x00 0x00 0x00 0x00

Byte 0: 0x8a (QWORD address space descriptor)
Byte 3: Resource Type = 0x01 (I/O range)
Byte 5: Type Specific Flags = 0x33 (_TRS, _TTP, _RNG = 3)

          QWORD Address Space Descriptor:
             Type: I/O
             Flags:  Sparse, Translate, ISA I/O addresses, Non-ISA I/O addresses
             GRA: 0x0000000000000000
             MIN: 0x0000000000000000  MAX: 0x000000000000ffff
             TRA: 0x00000400d0000000  LEN: 0x0000000000010000
             MAX address fixed
             MIN address fixed
             Address positively decoded
             Device produces and consumes this resource

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

lan,Tianyu Oct. 26, 2013, 4:53 p.m. UTC | #7

On 10/24/2013 06:39 AM, Bjorn Helgaas wrote:
> [+cc Greg]
>
> On Fri, Oct 18, 2013 at 08:44:12PM +0800, Lan Tianyu wrote:
>> On 10/18/2013 04:33 AM, Bjorn Helgaas wrote:
>>> On Thu, Oct 17, 2013 at 12:09 AM, Lan Tianyu <[e-mail]> wrote:
>>>> On 2013年10月17日 07:55, Bjorn Helgaas wrote:
>>>>> On Fri, Oct 11, 2013 at 08:19:01PM +0800, [e-mail] wrote:
>
>>> 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 it's an oversight, and you should fix it.  I suggest that you
> ignore the _TRA value when _TTP is set.  Obviously this only applies
> to I/O port resources, since _TTP is only defined in the I/O Resource
> Flag (Table 6-185 in ACPI 5.0 spec).

_TTP is also defined in the Memory Resource flag, Please have a look at
Table 6-184 in the ACPI 5.0 Spec.

I am not sure how to deal with _TTP unsetting io resource? _TTP 
unsetting mean the resource is IO on the primary side and also IO on the 
secondary side.

>
>>> 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.
>
> Greg collected an acpidump from an HP system that uses these I/O port
> ranges.  Unfortunately the system wasn't running Linux, so it's an EFI
> dump, not the usual one we get from the "pmtools" package.  But I
> think it has the information we want.
>
> It's huge, and I put some of the relevant parts of it here:
> https://bugzilla.kernel.org/show_bug.cgi?id=63581  Here's a sample
> that shows the _TTP bit is set for the I/O aperture:
>
>      Device E000 (\_SB_.N000.E000)
>        Name _SEG (\_SB_.N000.E000._SEG)
>          0x01
>        Name _CRS (\_SB_.N000.E001._CRS)
>          Buffer
>            0x0092
>            ByteList <0x88 0x0d 0x00 0x02 0x0e 0x00 0x00 0x00
>                      0x00 0x00 0xff 0x00 0x00 0x00 0x00 0x01
>
>                      0x8a 0x2b 0x00 0x01 0x0c 0x33 0x00 0x00
>                      0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
>                      0x00 0x00 0x00 0x00 0x00 0x00 0xff 0xff
>                      0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
>                      0x00 0xd0 0x00 0x04 0x00 0x00 0x00 0x00
>                      0x01 0x00 0x00 0x00 0x00 0x00
>
> Byte 0: 0x8a (QWORD address space descriptor)
> Byte 3: Resource Type = 0x01 (I/O range)
> Byte 5: Type Specific Flags = 0x33 (_TRS, _TTP, _RNG = 3)
>
>            QWORD Address Space Descriptor:
>               Type: I/O
>               Flags:  Sparse, Translate, ISA I/O addresses, Non-ISA I/O addresses
>               GRA: 0x0000000000000000
>               MIN: 0x0000000000000000  MAX: 0x000000000000ffff
>               TRA: 0x00000400d0000000  LEN: 0x0000000000010000
>               MAX address fixed
>               MIN address fixed
>               Address positively decoded
>               Device produces and consumes this resource
>
> Bjorn
>

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

Bjorn Helgaas Oct. 28, 2013, 5:32 p.m. UTC | #8

On Sat, Oct 26, 2013 at 10:53 AM, Lan Tianyu <[e-mail]> wrote:
> On 10/24/2013 06:39 AM, Bjorn Helgaas wrote:
>> On Fri, Oct 18, 2013 at 08:44:12PM +0800, Lan Tianyu wrote:
>>>
>>> On 10/18/2013 04:33 AM, Bjorn Helgaas wrote:

>>>> 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 it's an oversight, and you should fix it.  I suggest that you
>> ignore the _TRA value when _TTP is set.  Obviously this only applies
>> to I/O port resources, since _TTP is only defined in the I/O Resource
>> Flag (Table 6-185 in ACPI 5.0 spec).
>
> _TTP is also defined in the Memory Resource flag, Please have a look at
> Table 6-184 in the ACPI 5.0 Spec.

Yes, you're right.  That would be for a host bridge that converts I/O
on the primary (upstream) side of the bridge to memory on the PCI
side.  I've never seen such a bridge, and I can't really imagine why
anybody would do that.  But I guess you should be able to safely
ignore _TRA when _TTP is set in either a MEM or IO descriptor, because
the same reasoning should apply to both.

> I am not sure how to deal with _TTP unsetting io resource? _TTP unsetting
> mean the resource is IO on the primary side and also IO on the secondary
> side.

If _TTP is not set, I guess you would apply _TRA.  That's what you
already do for MEM descriptors, and think you should just do the same
for IO descriptors.  I would guess that having _TTP = 0 and _TRA != 0
is rare for IO descriptors, but I suppose it could happen.

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

lan,Tianyu Oct. 30, 2013, 8:34 a.m. UTC | #9

On 2013?10?29? 01:32, Bjorn Helgaas wrote:
> On Sat, Oct 26, 2013 at 10:53 AM, Lan Tianyu <[e-mail]> wrote:
>> On 10/24/2013 06:39 AM, Bjorn Helgaas wrote:
>>> On Fri, Oct 18, 2013 at 08:44:12PM +0800, Lan Tianyu wrote:
>>>>
>>>> On 10/18/2013 04:33 AM, Bjorn Helgaas wrote:
>
>>>>> 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 it's an oversight, and you should fix it.  I suggest that you
>>> ignore the _TRA value when _TTP is set.  Obviously this only applies
>>> to I/O port resources, since _TTP is only defined in the I/O Resource
>>> Flag (Table 6-185 in ACPI 5.0 spec).
>>
>> _TTP is also defined in the Memory Resource flag, Please have a look at
>> Table 6-184 in the ACPI 5.0 Spec.
>
> Yes, you're right.  That would be for a host bridge that converts I/O
> on the primary (upstream) side of the bridge to memory on the PCI
> side.  I've never seen such a bridge, and I can't really imagine why
> anybody would do that.  But I guess you should be able to safely
> ignore _TRA when _TTP is set in either a MEM or IO descriptor, because
> the same reasoning should apply to both.
>
>> I am not sure how to deal with _TTP unsetting io resource? _TTP unsetting
>> mean the resource is IO on the primary side and also IO on the secondary
>> side.
>
> If _TTP is not set, I guess you would apply _TRA.  That's what you
> already do for MEM descriptors, and think you should just do the same
> for IO descriptors.  I would guess that having _TTP = 0 and _TRA != 0
> is rare for IO descriptors, but I suppose it could happen.

Yes, my concern is for the IO resource case of _TTP=0 and _TRA !=0. The
only reason for this case I think of is that the IO resource offsets on
the prime bus and second bus are different. In this case, we still need
to pass _TRA to new_space() and the finial resource->start still should 
be acpi_resource->min + offset returned by add_io_space(), right?

If yes, I think _TRA can't be applied to IO resource in the
acpi_dev_resource_address_space() regardless of the value of _TTP.

BTW, Translation Sparse(_TRS) is only meaningful if _TTP is set.(Table 
6-185). The add_io_space() doesn't check _TTP when set sparse. So this 
should be corrected?

>
> Bjorn
>

Bjorn Helgaas Oct. 30, 2013, 4:23 p.m. UTC | #10

On Wed, Oct 30, 2013 at 2:34 AM, Lan Tianyu <[e-mail]> wrote:
> On 2013?10?29? 01:32, Bjorn Helgaas wrote:
>>
>> On Sat, Oct 26, 2013 at 10:53 AM, Lan Tianyu <[e-mail]> wrote:
>>>
>>> On 10/24/2013 06:39 AM, Bjorn Helgaas wrote:
>>>>
>>>> On Fri, Oct 18, 2013 at 08:44:12PM +0800, Lan Tianyu wrote:
>>>>>
>>>>>
>>>>> On 10/18/2013 04:33 AM, Bjorn Helgaas wrote:
>>
>>
>>>>>> 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 it's an oversight, and you should fix it.  I suggest that you
>>>> ignore the _TRA value when _TTP is set.  Obviously this only applies
>>>> to I/O port resources, since _TTP is only defined in the I/O Resource
>>>> Flag (Table 6-185 in ACPI 5.0 spec).
>>>
>>>
>>> _TTP is also defined in the Memory Resource flag, Please have a look at
>>> Table 6-184 in the ACPI 5.0 Spec.
>>
>>
>> Yes, you're right.  That would be for a host bridge that converts I/O
>> on the primary (upstream) side of the bridge to memory on the PCI
>> side.  I've never seen such a bridge, and I can't really imagine why
>> anybody would do that.  But I guess you should be able to safely
>> ignore _TRA when _TTP is set in either a MEM or IO descriptor, because
>> the same reasoning should apply to both.
>>
>>> I am not sure how to deal with _TTP unsetting io resource? _TTP unsetting
>>> mean the resource is IO on the primary side and also IO on the secondary
>>> side.
>>
>>
>> If _TTP is not set, I guess you would apply _TRA.  That's what you
>> already do for MEM descriptors, and think you should just do the same
>> for IO descriptors.  I would guess that having _TTP = 0 and _TRA != 0
>> is rare for IO descriptors, but I suppose it could happen.
>
>
> Yes, my concern is for the IO resource case of _TTP=0 and _TRA !=0. The
> only reason for this case I think of is that the IO resource offsets on
> the prime bus and second bus are different. In this case, we still need
> to pass _TRA to new_space() and the finial resource->start still should be
> acpi_resource->min + offset returned by add_io_space(), right?

No, I don't think so.  If the "phys_base" argument to new_space() is
non-zero, it is the base of an MMIO region that needs to be
ioremapped.  This is handling the _TTP=1 case, where the MMIO region
is translated by the bridge into an IO region on PCI.

If _TTP=0, the region is IO on both the upstream and downstream sides
of the host bridge, and we don't want to ioremap a new MMIO region for
it.  It might be part of the "legacy I/O port space," but that's
already covered elsewhere.

I don't think we need to add special handling for the _TTP=0 and _TRA
!= 0 case because I don't think it exists in the field.  If and when
it *does* exist, we'll know what to do.  In the meantime, it should
look just like the MEM path.

> If yes, I think _TRA can't be applied to IO resource in the
> acpi_dev_resource_address_space() regardless of the value of _TTP.
>
> BTW, Translation Sparse(_TRS) is only meaningful if _TTP is set.(Table
> 6-185). The add_io_space() doesn't check _TTP when set sparse. So this
> should be corrected?

Sure, I'm OK with this.  It's possible we could trip over a BIOS bug
where _TRS=1 but _TTP=0, but I think the risk is low because only
large ia64 boxes would use this, and there aren't very many of those.

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

lan,Tianyu Oct. 31, 2013, 2:26 a.m. UTC | #11

On 2013?10?31? 00:23, Bjorn Helgaas wrote:
> On Wed, Oct 30, 2013 at 2:34 AM, Lan Tianyu <[e-mail]> wrote:
>> On 2013?10?29? 01:32, Bjorn Helgaas wrote:
>>>
>>> On Sat, Oct 26, 2013 at 10:53 AM, Lan Tianyu <[e-mail]> wrote:
>>>>
>>>> On 10/24/2013 06:39 AM, Bjorn Helgaas wrote:
>>>>>
>>>>> On Fri, Oct 18, 2013 at 08:44:12PM +0800, Lan Tianyu wrote:
>>>>>>
>>>>>>
>>>>>> On 10/18/2013 04:33 AM, Bjorn Helgaas wrote:
>>>
>>>
>>>>>>> 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 it's an oversight, and you should fix it.  I suggest that you
>>>>> ignore the _TRA value when _TTP is set.  Obviously this only applies
>>>>> to I/O port resources, since _TTP is only defined in the I/O Resource
>>>>> Flag (Table 6-185 in ACPI 5.0 spec).
>>>>
>>>>
>>>> _TTP is also defined in the Memory Resource flag, Please have a look at
>>>> Table 6-184 in the ACPI 5.0 Spec.
>>>
>>>
>>> Yes, you're right.  That would be for a host bridge that converts I/O
>>> on the primary (upstream) side of the bridge to memory on the PCI
>>> side.  I've never seen such a bridge, and I can't really imagine why
>>> anybody would do that.  But I guess you should be able to safely
>>> ignore _TRA when _TTP is set in either a MEM or IO descriptor, because
>>> the same reasoning should apply to both.
>>>
>>>> I am not sure how to deal with _TTP unsetting io resource? _TTP unsetting
>>>> mean the resource is IO on the primary side and also IO on the secondary
>>>> side.
>>>
>>>
>>> If _TTP is not set, I guess you would apply _TRA.  That's what you
>>> already do for MEM descriptors, and think you should just do the same
>>> for IO descriptors.  I would guess that having _TTP = 0 and _TRA != 0
>>> is rare for IO descriptors, but I suppose it could happen.
>>
>>
>> Yes, my concern is for the IO resource case of _TTP=0 and _TRA !=0. The
>> only reason for this case I think of is that the IO resource offsets on
>> the prime bus and second bus are different. In this case, we still need
>> to pass _TRA to new_space() and the finial resource->start still should be
>> acpi_resource->min + offset returned by add_io_space(), right?
> 
> No, I don't think so.  If the "phys_base" argument to new_space() is
> non-zero, it is the base of an MMIO region that needs to be
> ioremapped.  This is handling the _TTP=1 case, where the MMIO region
> is translated by the bridge into an IO region on PCI.
> 
> If _TTP=0, the region is IO on both the upstream and downstream sides
> of the host bridge, and we don't want to ioremap a new MMIO region for
> it.  It might be part of the "legacy I/O port space," but that's
> already covered elsewhere.
> 
> I don't think we need to add special handling for the _TTP=0 and _TRA
> != 0 case because I don't think it exists in the field.  If and when
> it *does* exist, we'll know what to do.  In the meantime, it should
> look just like the MEM path.


OK. I get it. acpi_dev_resource_address_space() will only apply _TRA to
resource ->start and ->end for both mem and io resource when _TTP=0. In
the add_window(), the offset returned by add_io_space() will be added
directly to ->start and ->end.

add_window() {
	...
	if (resource->flags & IORESOURCE_MEM) {
		root = &iomem_resource;
		offset = addr.translation_offset;
	} else if (resource->flags & IORESOURCE_IO) {
		root = &ioport_resource;

		offset = add_io_space(info, &addr);
		if (offset == ~0)
			return AE_OK;

		resource->start += offset;
		resource->end += offset;
	} else
		return AE_OK;

	...
}

> 
>> If yes, I think _TRA can't be applied to IO resource in the
>> acpi_dev_resource_address_space() regardless of the value of _TTP.
>>
>> BTW, Translation Sparse(_TRS) is only meaningful if _TTP is set.(Table
>> 6-185). The add_io_space() doesn't check _TTP when set sparse. So this
>> should be corrected?
> 
> Sure, I'm OK with this.  It's possible we could trip over a BIOS bug
> where _TRS=1 but _TTP=0, but I think the risk is low because only
> large ia64 boxes would use this, and there aren't very many of those.
> 

Ok. I will add a check for _TTP before setting sparse. Something likes this.

add_io_space()
{
...
if (addr->info.io.translation == ACPI_TYPE_TRANSLATION &&
    addr->info.io.translation_type == ACPI_SPARSE_TRANSLATION)
	sparse = 1;
...
}



> Bjorn
>

Bjorn Helgaas Oct. 31, 2013, 1 p.m. UTC | #12

On Thu, Oct 31, 2013 at 10:26:03AM +0800, Lan Tianyu wrote:
> On 2013?10?31? 00:23, Bjorn Helgaas wrote:
> > On Wed, Oct 30, 2013 at 2:34 AM, Lan Tianyu <[e-mail]> wrote:
> >> On 2013?10?29? 01:32, Bjorn Helgaas wrote:
> >>>
> >>> On Sat, Oct 26, 2013 at 10:53 AM, Lan Tianyu <[e-mail]> wrote:
> >>>>
> >>>> On 10/24/2013 06:39 AM, Bjorn Helgaas wrote:
> >>>>>
> >>>>> On Fri, Oct 18, 2013 at 08:44:12PM +0800, Lan Tianyu wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 10/18/2013 04:33 AM, Bjorn Helgaas wrote:
> >>>
> >>>
> >>>>>>> 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 it's an oversight, and you should fix it.  I suggest that you
> >>>>> ignore the _TRA value when _TTP is set.  Obviously this only applies
> >>>>> to I/O port resources, since _TTP is only defined in the I/O Resource
> >>>>> Flag (Table 6-185 in ACPI 5.0 spec).
> >>>>
> >>>>
> >>>> _TTP is also defined in the Memory Resource flag, Please have a look at
> >>>> Table 6-184 in the ACPI 5.0 Spec.
> >>>
> >>>
> >>> Yes, you're right.  That would be for a host bridge that converts I/O
> >>> on the primary (upstream) side of the bridge to memory on the PCI
> >>> side.  I've never seen such a bridge, and I can't really imagine why
> >>> anybody would do that.  But I guess you should be able to safely
> >>> ignore _TRA when _TTP is set in either a MEM or IO descriptor, because
> >>> the same reasoning should apply to both.
> >>>
> >>>> I am not sure how to deal with _TTP unsetting io resource? _TTP unsetting
> >>>> mean the resource is IO on the primary side and also IO on the secondary
> >>>> side.
> >>>
> >>>
> >>> If _TTP is not set, I guess you would apply _TRA.  That's what you
> >>> already do for MEM descriptors, and think you should just do the same
> >>> for IO descriptors.  I would guess that having _TTP = 0 and _TRA != 0
> >>> is rare for IO descriptors, but I suppose it could happen.
> >>
> >>
> >> Yes, my concern is for the IO resource case of _TTP=0 and _TRA !=0. The
> >> only reason for this case I think of is that the IO resource offsets on
> >> the prime bus and second bus are different. In this case, we still need
> >> to pass _TRA to new_space() and the finial resource->start still should be
> >> acpi_resource->min + offset returned by add_io_space(), right?
> > 
> > No, I don't think so.  If the "phys_base" argument to new_space() is
> > non-zero, it is the base of an MMIO region that needs to be
> > ioremapped.  This is handling the _TTP=1 case, where the MMIO region
> > is translated by the bridge into an IO region on PCI.
> > 
> > If _TTP=0, the region is IO on both the upstream and downstream sides
> > of the host bridge, and we don't want to ioremap a new MMIO region for
> > it.  It might be part of the "legacy I/O port space," but that's
> > already covered elsewhere.
> > 
> > I don't think we need to add special handling for the _TTP=0 and _TRA
> > != 0 case because I don't think it exists in the field.  If and when
> > it *does* exist, we'll know what to do.  In the meantime, it should
> > look just like the MEM path.
> 
> 
> OK. I get it. acpi_dev_resource_address_space() will only apply _TRA to
> resource ->start and ->end for both mem and io resource when _TTP=0. In
> the add_window(), the offset returned by add_io_space() will be added
> directly to ->start and ->end.
> 
> add_window() {
> 	...
> 	if (resource->flags & IORESOURCE_MEM) {
> 		root = &iomem_resource;
> 		offset = addr.translation_offset;

I can wait for your patch to see the whole thing, but I would expect
"offset = 0" here.  For MEM resources, the arch code should not need to
look inside "addr" at all.

> 	} else if (resource->flags & IORESOURCE_IO) {
> 		root = &ioport_resource;
> 
> 		offset = add_io_space(info, &addr);
> 		if (offset == ~0)
> 			return AE_OK;
> 
> 		resource->start += offset;
> 		resource->end += offset;
> 	} else
> 		return AE_OK;
> 
> 	...
> }
> 
> > 
> >> If yes, I think _TRA can't be applied to IO resource in the
> >> acpi_dev_resource_address_space() regardless of the value of _TTP.
> >>
> >> BTW, Translation Sparse(_TRS) is only meaningful if _TTP is set.(Table
> >> 6-185). The add_io_space() doesn't check _TTP when set sparse. So this
> >> should be corrected?
> > 
> > Sure, I'm OK with this.  It's possible we could trip over a BIOS bug
> > where _TRS=1 but _TTP=0, but I think the risk is low because only
> > large ia64 boxes would use this, and there aren't very many of those.
> > 
> 
> Ok. I will add a check for _TTP before setting sparse. Something likes this.
> 
> add_io_space()
> {
> ...
> if (addr->info.io.translation == ACPI_TYPE_TRANSLATION &&
>     addr->info.io.translation_type == ACPI_SPARSE_TRANSLATION)
> 	sparse = 1;
> ...
> }
> 
> 
> 
> > Bjorn
> > 
> 
> 
> -- 
> Best regards
> Tianyu Lan
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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.
+		 */
+		resource->start = addr.minimum + offset;
+		resource->end = resource->start + addr.address_length - 1;
 	} else
 		return AE_OK;
 
-	resource = &info->res[info->res_num];
 	resource->name = info->name;
-	resource->flags = flags;
-	resource->start = addr.minimum + offset;
-	resource->end = resource->start + addr.address_length - 1;
 	info->res_offset[info->res_num] = offset;
 
 	if (insert_resource(root, resource)) {