[Resend PATCH 0/5] ACPI/PCI: Parse PCI root bridge's ACPI resource via ACPI resource functions

[Resend PATCH 0/5] ACPI/PCI: Parse PCI root bridge's ACPI resource via ACPI resource functions

[tianyu.lan]

From: Lan Tianyu <tianyu.lan@intel.com>

[Patch 1~2 and 4~5 of previous version weren't sent to maillist. So resend the patchset]

This patchset is to add memory prefecth flag setting and address
translation support in the ACPI resource function. Convert x86/iad64
PCI root bridge's ACPI resource to generic resource via ACPI resource
functions.

PATCH 3 is to add new function acpi_dev_resource_address_space_full()
to meet requirement that ACPI address space info is still needed after
being converted to generic resource.


Lan Tianyu (5):
  ACPI/Resource: Add memory prefetch check support
  ACPI/Resource: Add address translation support
  ACPI: Add new acpi_dev_resource_address_space_full() function
  X86/PCI/ACPI: Rework setup_resource() via functions ACPI resource
    functions
  IA64/PCI/ACPI: Rework PCI root bridge ACPI resource conversion

 arch/ia64/pci/pci.c     | 36 ++++++++++++----------
 arch/x86/pci/acpi.c     | 81 ++++++++-----------------------------------------
 drivers/acpi/resource.c | 54 ++++++++++++++++++++++++---------
 include/linux/acpi.h    |  3 ++
 4 files changed, 74 insertions(+), 100 deletions(-)

-- 
1.8.2.1

[Resend PATCH 1/5] ACPI/Resource: Add memory prefetch check support

[tianyu.lan]

From: Lan Tianyu <tianyu.lan@intel.com>

This patch is to check mem address space's acpi resource caching ability
and set prefetch flag of struct resource if it's prefetchable.

Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 drivers/acpi/resource.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
index b7201fc..929f416 100644
--- a/drivers/acpi/resource.c
+++ b/drivers/acpi/resource.c
@@ -202,6 +202,9 @@ bool acpi_dev_resource_address_space(struct acpi_resource *ares,
 		res->flags = acpi_dev_memresource_flags(len,
 						addr.info.mem.write_protect,
 						window);
+
+		if (addr.info.mem.caching == ACPI_PREFETCHABLE_MEMORY)
+			res->flags |= IORESOURCE_PREFETCH;
 		break;
 	case ACPI_IO_RANGE:
 		io_decode = addr.granularity == 0xfff ?
-- 
1.8.2.1

[Resend PATCH 2/5] ACPI/Resource: Add address translation support

[tianyu.lan]

From: Lan Tianyu <tianyu.lan@intel.com>

According ACPI 5.0 spec Section 19.1.8
"For bridges, translate addresses across the bridge, this is the
offset that must be added to the address on the secondary side
to obtain the address on the primary side. Non-bridge devices
must list 0."

This patch is to add address translation offset to the start/end
of struct resource in the acpi_dev_resource_address_space().
Further more, non-bridge device's translation_offset should 0.
So this change will affect other devices.

Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 drivers/acpi/resource.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
index 929f416..84bc3db 100644
--- a/drivers/acpi/resource.c
+++ b/drivers/acpi/resource.c
@@ -192,8 +192,8 @@ bool acpi_dev_resource_address_space(struct acpi_resource *ares,
 	if (ACPI_FAILURE(status))
 		return true;
 
-	res->start = addr.minimum;
-	res->end = addr.maximum;
+	res->start = addr.minimum + addr.translation_offset;
+	res->end = addr.maximum + addr.translation_offset;
 	window = addr.producer_consumer == ACPI_PRODUCER;
 
 	switch(addr.resource_type) {
-- 
1.8.2.1

[Resend PATCH 3/5] ACPI: Add new acpi_dev_resource_address_space_full() function

[tianyu.lan]

From: Lan Tianyu <tianyu.lan@intel.com>

Make acpi_dev_resource_address_space() to accept struct
acpi_resource_address64 as param and rename it to *_full.

This is for some cases that acpi address info is also needed
after convert from acpi resouce to generic resource.

Add acpi_dev_resource_addres_space() again as a wrapper of new
function for original users.

Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 drivers/acpi/resource.c | 53 ++++++++++++++++++++++++++++++++++---------------
 include/linux/acpi.h    |  3 +++
 2 files changed, 40 insertions(+), 16 deletions(-)

diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
index 84bc3db..2e395ba 100644
--- a/drivers/acpi/resource.c
+++ b/drivers/acpi/resource.c
@@ -162,19 +162,21 @@ bool acpi_dev_resource_io(struct acpi_resource *ares, struct resource *res)
 EXPORT_SYMBOL_GPL(acpi_dev_resource_io);
 
 /**
- * acpi_dev_resource_address_space - Extract ACPI address space information.
+ * acpi_dev_resource_address_space_full - Extract ACPI address space information.
  * @ares: Input ACPI resource object.
+ * @addr: Output ACPI resource address64 space object.
  * @res: Output generic resource object.
  *
  * Check if the given ACPI resource object represents an address space resource
- * and if that's the case, use the information in it to populate the generic
- * resource object pointed to by @res.
+ * and if that's the case, convert it to ACPI resource address64 space object
+ * pointed to by @addr and use the information to populate the generic resource
+ * object pointed to by @re.
  */
-bool acpi_dev_resource_address_space(struct acpi_resource *ares,
+bool acpi_dev_resource_address_space_full(struct acpi_resource *ares,
+				     struct acpi_resource_address64 *addr,
 				     struct resource *res)
 {
 	acpi_status status;
-	struct acpi_resource_address64 addr;
 	bool window;
 	u64 len;
 	u8 io_decode;
@@ -188,29 +190,29 @@ bool acpi_dev_resource_address_space(struct acpi_resource *ares,
 		return false;
 	}
 
-	status = acpi_resource_to_address64(ares, &addr);
+	status = acpi_resource_to_address64(ares, addr);
 	if (ACPI_FAILURE(status))
 		return true;
 
-	res->start = addr.minimum + addr.translation_offset;
-	res->end = addr.maximum + addr.translation_offset;
-	window = addr.producer_consumer == ACPI_PRODUCER;
+	res->start = addr->minimum + addr->translation_offset;
+	res->end = addr->maximum + addr->translation_offset;
+	window = addr->producer_consumer == ACPI_PRODUCER;
 
-	switch(addr.resource_type) {
+	switch (addr->resource_type) {
 	case ACPI_MEMORY_RANGE:
-		len = addr.maximum - addr.minimum + 1;
+		len = addr->maximum - addr->minimum + 1;
 		res->flags = acpi_dev_memresource_flags(len,
-						addr.info.mem.write_protect,
+						addr->info.mem.write_protect,
 						window);
 
-		if (addr.info.mem.caching == ACPI_PREFETCHABLE_MEMORY)
+		if (addr->info.mem.caching == ACPI_PREFETCHABLE_MEMORY)
 			res->flags |= IORESOURCE_PREFETCH;
 		break;
 	case ACPI_IO_RANGE:
-		io_decode = addr.granularity == 0xfff ?
+		io_decode = addr->granularity == 0xfff ?
 				ACPI_DECODE_10 : ACPI_DECODE_16;
-		res->flags = acpi_dev_ioresource_flags(addr.minimum,
-						       addr.maximum,
+		res->flags = acpi_dev_ioresource_flags(addr->minimum,
+						       addr->maximum,
 						       io_decode, window);
 		break;
 	case ACPI_BUS_NUMBER_RANGE:
@@ -222,6 +224,25 @@ bool acpi_dev_resource_address_space(struct acpi_resource *ares,
 
 	return true;
 }
+EXPORT_SYMBOL_GPL(acpi_dev_resource_address_space_full);
+
+/**
+ * acpi_dev_resource_address_space - Extract ACPI address space information.
+ * @ares: Input ACPI resource object.
+ * @res: Output generic resource object.
+ *
+ * Check if the given ACPI resource object represents an address space resource
+ * and if that's the case, use the information in it to populate the generic
+ * resource object pointed to by @res.
+ */
+bool acpi_dev_resource_address_space(struct acpi_resource *ares,
+				     struct resource *res)
+{
+	struct acpi_resource_address64 addr;
+
+	return acpi_dev_resource_address_space_full(ares, &addr,
+							 res);
+}
 EXPORT_SYMBOL_GPL(acpi_dev_resource_address_space);
 
 /**
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index a5db4ae..1f6701e 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -260,6 +260,9 @@ bool acpi_dev_resource_memory(struct acpi_resource *ares, struct resource *res);
 bool acpi_dev_resource_io(struct acpi_resource *ares, struct resource *res);
 bool acpi_dev_resource_address_space(struct acpi_resource *ares,
 				     struct resource *res);
+bool acpi_dev_resource_address_space_full(struct acpi_resource *ares,
+				struct acpi_resource_address64 *addr,
+				struct resource *res);
 bool acpi_dev_resource_ext_address_space(struct acpi_resource *ares,
 					 struct resource *res);
 unsigned long acpi_dev_irq_flags(u8 triggering, u8 polarity, u8 shareable);
-- 
1.8.2.1

[Resend PATCH 4/5] X86/PCI/ACPI: Rework setup_resource() via functions ACPI resource functions

[tianyu.lan]

From: Lan Tianyu <tianyu.lan@intel.com>

Using ACPI resource functions to convert ACPI resource to generic resource
instead of resource_to_addr(). Remove resource_to_addr().

Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 arch/x86/pci/acpi.c | 81 ++++++++---------------------------------------------
 1 file changed, 12 insertions(+), 69 deletions(-)

diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
index d641897..f4290a1 100644
--- a/arch/x86/pci/acpi.c
+++ b/arch/x86/pci/acpi.c
@@ -219,62 +219,15 @@ static void teardown_mcfg_map(struct pci_root_info *info)
 #endif
 
 static acpi_status
-resource_to_addr(struct acpi_resource *resource,
-			struct acpi_resource_address64 *addr)
-{
-	acpi_status status;
-	struct acpi_resource_memory24 *memory24;
-	struct acpi_resource_memory32 *memory32;
-	struct acpi_resource_fixed_memory32 *fixed_memory32;
-
-	memset(addr, 0, sizeof(*addr));
-	switch (resource->type) {
-	case ACPI_RESOURCE_TYPE_MEMORY24:
-		memory24 = &resource->data.memory24;
-		addr->resource_type = ACPI_MEMORY_RANGE;
-		addr->minimum = memory24->minimum;
-		addr->address_length = memory24->address_length;
-		addr->maximum = addr->minimum + addr->address_length - 1;
-		return AE_OK;
-	case ACPI_RESOURCE_TYPE_MEMORY32:
-		memory32 = &resource->data.memory32;
-		addr->resource_type = ACPI_MEMORY_RANGE;
-		addr->minimum = memory32->minimum;
-		addr->address_length = memory32->address_length;
-		addr->maximum = addr->minimum + addr->address_length - 1;
-		return AE_OK;
-	case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
-		fixed_memory32 = &resource->data.fixed_memory32;
-		addr->resource_type = ACPI_MEMORY_RANGE;
-		addr->minimum = fixed_memory32->address;
-		addr->address_length = fixed_memory32->address_length;
-		addr->maximum = addr->minimum + addr->address_length - 1;
-		return AE_OK;
-	case ACPI_RESOURCE_TYPE_ADDRESS16:
-	case ACPI_RESOURCE_TYPE_ADDRESS32:
-	case ACPI_RESOURCE_TYPE_ADDRESS64:
-		status = acpi_resource_to_address64(resource, addr);
-		if (ACPI_SUCCESS(status) &&
-		    (addr->resource_type == ACPI_MEMORY_RANGE ||
-		    addr->resource_type == ACPI_IO_RANGE) &&
-		    addr->address_length > 0) {
-			return AE_OK;
-		}
-		break;
-	}
-	return AE_ERROR;
-}
-
-static acpi_status
 count_resource(struct acpi_resource *acpi_res, void *data)
 {
 	struct pci_root_info *info = data;
-	struct acpi_resource_address64 addr;
-	acpi_status status;
+	struct resource res;
 
-	status = resource_to_addr(acpi_res, &addr);
-	if (ACPI_SUCCESS(status))
+	if (acpi_dev_resource_address_space(acpi_res, &res)
+	    || acpi_dev_resource_memory(acpi_res, &res))
 		info->res_num++;
+
 	return AE_OK;
 }
 
@@ -282,27 +235,18 @@ static acpi_status
 setup_resource(struct acpi_resource *acpi_res, void *data)
 {
 	struct pci_root_info *info = data;
-	struct resource *res;
+	struct resource *res = &info->res[info->res_num];
 	struct acpi_resource_address64 addr;
-	acpi_status status;
-	unsigned long flags;
 	u64 start, orig_end, end;
 
-	status = resource_to_addr(acpi_res, &addr);
-	if (!ACPI_SUCCESS(status))
-		return AE_OK;
+	memset(&addr, 0x00, sizeof(addr));
 
-	if (addr.resource_type == ACPI_MEMORY_RANGE) {
-		flags = IORESOURCE_MEM;
-		if (addr.info.mem.caching == ACPI_PREFETCHABLE_MEMORY)
-			flags |= IORESOURCE_PREFETCH;
-	} else if (addr.resource_type == ACPI_IO_RANGE) {
-		flags = IORESOURCE_IO;
-	} else
+	if (!(acpi_dev_resource_address_space_full(acpi_res, &addr, res)
+	    || acpi_dev_resource_memory(acpi_res, res)))
 		return AE_OK;
 
-	start = addr.minimum + addr.translation_offset;
-	orig_end = end = addr.maximum + addr.translation_offset;
+	start = res->start;
+	orig_end = end = res->end;
 
 	/* Exclude non-addressable range or non-addressable portion of range */
 	end = min(end, (u64)iomem_resource.end);
@@ -310,6 +254,7 @@ setup_resource(struct acpi_resource *acpi_res, void *data)
 		dev_info(&info->bridge->dev,
 			"host bridge window [%#llx-%#llx] "
 			"(ignored, not CPU addressable)\n", start, orig_end);
+		memset(&info->res[info->res_num], 0x00, sizeof(*res));
 		return AE_OK;
 	} else if (orig_end != end) {
 		dev_info(&info->bridge->dev,
@@ -318,11 +263,9 @@ setup_resource(struct acpi_resource *acpi_res, void *data)
 			start, orig_end, end + 1, orig_end);
 	}
 
-	res = &info->res[info->res_num];
 	res->name = info->name;
-	res->flags = flags;
-	res->start = start;
 	res->end = end;
+
 	info->res_offset[info->res_num] = addr.translation_offset;
 	info->res_num++;
 
-- 
1.8.2.1

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

[tianyu.lan]

From: Lan Tianyu <tianyu.lan@intel.com>

Using ACPI resource functions to convert ACPI resource to generic resource

Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
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)) {
-- 
1.8.2.1

Re: [Resend PATCH 4/5] X86/PCI/ACPI: Rework setup_resource() via functions ACPI resource functions

[Yinghai Lu]

On Fri, Oct 11, 2013 at 5:19 AM,  <tianyu.lan@intel.com> wrote:
> From: Lan Tianyu <tianyu.lan@intel.com>
>
> Using ACPI resource functions to convert ACPI resource to generic resource
> instead of resource_to_addr(). Remove resource_to_addr().
>
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>

for 1-4

Acked-by: Yinghai Lu <yinghai@kernel.org>

> ---
>  arch/x86/pci/acpi.c | 81 ++++++++---------------------------------------------
>  1 file changed, 12 insertions(+), 69 deletions(-)
>
> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
> index d641897..f4290a1 100644
> --- a/arch/x86/pci/acpi.c
> +++ b/arch/x86/pci/acpi.c
> @@ -219,62 +219,15 @@ static void teardown_mcfg_map(struct pci_root_info *info)
>  #endif
>
>  static acpi_status
> -resource_to_addr(struct acpi_resource *resource,
> -                       struct acpi_resource_address64 *addr)
> -{
> -       acpi_status status;
> -       struct acpi_resource_memory24 *memory24;
> -       struct acpi_resource_memory32 *memory32;
> -       struct acpi_resource_fixed_memory32 *fixed_memory32;
> -
> -       memset(addr, 0, sizeof(*addr));
> -       switch (resource->type) {
> -       case ACPI_RESOURCE_TYPE_MEMORY24:
> -               memory24 = &resource->data.memory24;
> -               addr->resource_type = ACPI_MEMORY_RANGE;
> -               addr->minimum = memory24->minimum;
> -               addr->address_length = memory24->address_length;
> -               addr->maximum = addr->minimum + addr->address_length - 1;
> -               return AE_OK;
> -       case ACPI_RESOURCE_TYPE_MEMORY32:
> -               memory32 = &resource->data.memory32;
> -               addr->resource_type = ACPI_MEMORY_RANGE;
> -               addr->minimum = memory32->minimum;
> -               addr->address_length = memory32->address_length;
> -               addr->maximum = addr->minimum + addr->address_length - 1;
> -               return AE_OK;
> -       case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
> -               fixed_memory32 = &resource->data.fixed_memory32;
> -               addr->resource_type = ACPI_MEMORY_RANGE;
> -               addr->minimum = fixed_memory32->address;
> -               addr->address_length = fixed_memory32->address_length;
> -               addr->maximum = addr->minimum + addr->address_length - 1;
> -               return AE_OK;
> -       case ACPI_RESOURCE_TYPE_ADDRESS16:
> -       case ACPI_RESOURCE_TYPE_ADDRESS32:
> -       case ACPI_RESOURCE_TYPE_ADDRESS64:
> -               status = acpi_resource_to_address64(resource, addr);
> -               if (ACPI_SUCCESS(status) &&
> -                   (addr->resource_type == ACPI_MEMORY_RANGE ||
> -                   addr->resource_type == ACPI_IO_RANGE) &&
> -                   addr->address_length > 0) {
> -                       return AE_OK;
> -               }
> -               break;
> -       }
> -       return AE_ERROR;
> -}
> -
> -static acpi_status
>  count_resource(struct acpi_resource *acpi_res, void *data)
>  {
>         struct pci_root_info *info = data;
> -       struct acpi_resource_address64 addr;
> -       acpi_status status;
> +       struct resource res;
>
> -       status = resource_to_addr(acpi_res, &addr);
> -       if (ACPI_SUCCESS(status))
> +       if (acpi_dev_resource_address_space(acpi_res, &res)
> +           || acpi_dev_resource_memory(acpi_res, &res))
>                 info->res_num++;
> +
>         return AE_OK;
>  }
>
> @@ -282,27 +235,18 @@ static acpi_status
>  setup_resource(struct acpi_resource *acpi_res, void *data)
>  {
>         struct pci_root_info *info = data;
> -       struct resource *res;
> +       struct resource *res = &info->res[info->res_num];
>         struct acpi_resource_address64 addr;
> -       acpi_status status;
> -       unsigned long flags;
>         u64 start, orig_end, end;
>
> -       status = resource_to_addr(acpi_res, &addr);
> -       if (!ACPI_SUCCESS(status))
> -               return AE_OK;
> +       memset(&addr, 0x00, sizeof(addr));
>
> -       if (addr.resource_type == ACPI_MEMORY_RANGE) {
> -               flags = IORESOURCE_MEM;
> -               if (addr.info.mem.caching == ACPI_PREFETCHABLE_MEMORY)
> -                       flags |= IORESOURCE_PREFETCH;
> -       } else if (addr.resource_type == ACPI_IO_RANGE) {
> -               flags = IORESOURCE_IO;
> -       } else
> +       if (!(acpi_dev_resource_address_space_full(acpi_res, &addr, res)
> +           || acpi_dev_resource_memory(acpi_res, res)))
>                 return AE_OK;
>
> -       start = addr.minimum + addr.translation_offset;
> -       orig_end = end = addr.maximum + addr.translation_offset;
> +       start = res->start;
> +       orig_end = end = res->end;
>
>         /* Exclude non-addressable range or non-addressable portion of range */
>         end = min(end, (u64)iomem_resource.end);
> @@ -310,6 +254,7 @@ setup_resource(struct acpi_resource *acpi_res, void *data)
>                 dev_info(&info->bridge->dev,
>                         "host bridge window [%#llx-%#llx] "
>                         "(ignored, not CPU addressable)\n", start, orig_end);
> +               memset(&info->res[info->res_num], 0x00, sizeof(*res));
>                 return AE_OK;
>         } else if (orig_end != end) {
>                 dev_info(&info->bridge->dev,
> @@ -318,11 +263,9 @@ setup_resource(struct acpi_resource *acpi_res, void *data)
>                         start, orig_end, end + 1, orig_end);
>         }
>
> -       res = &info->res[info->res_num];
>         res->name = info->name;
> -       res->flags = flags;
> -       res->start = start;
>         res->end = end;
> +
>         info->res_offset[info->res_num] = addr.translation_offset;
>         info->res_num++;
>
> --
> 1.8.2.1
>

Re: [Resend PATCH 4/5] X86/PCI/ACPI: Rework setup_resource() via functions ACPI resource functions

[Lan Tianyu]

On 10/12/2013 02:30 AM, Yinghai Lu wrote:
> On Fri, Oct 11, 2013 at 5:19 AM,  <tianyu.lan@intel.com> wrote:
>> From: Lan Tianyu <tianyu.lan@intel.com>
>>
>> Using ACPI resource functions to convert ACPI resource to generic resource
>> instead of resource_to_addr(). Remove resource_to_addr().
>>
>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
>
> for 1-4
>
> Acked-by: Yinghai Lu <yinghai@kernel.org>


Thanks for Ack.

>
>> ---
>>   arch/x86/pci/acpi.c | 81 ++++++++---------------------------------------------
>>   1 file changed, 12 insertions(+), 69 deletions(-)
>>
>> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
>> index d641897..f4290a1 100644
>> --- a/arch/x86/pci/acpi.c
>> +++ b/arch/x86/pci/acpi.c
>> @@ -219,62 +219,15 @@ static void teardown_mcfg_map(struct pci_root_info *info)
>>   #endif
>>
>>   static acpi_status
>> -resource_to_addr(struct acpi_resource *resource,
>> -                       struct acpi_resource_address64 *addr)
>> -{
>> -       acpi_status status;
>> -       struct acpi_resource_memory24 *memory24;
>> -       struct acpi_resource_memory32 *memory32;
>> -       struct acpi_resource_fixed_memory32 *fixed_memory32;
>> -
>> -       memset(addr, 0, sizeof(*addr));
>> -       switch (resource->type) {
>> -       case ACPI_RESOURCE_TYPE_MEMORY24:
>> -               memory24 = &resource->data.memory24;
>> -               addr->resource_type = ACPI_MEMORY_RANGE;
>> -               addr->minimum = memory24->minimum;
>> -               addr->address_length = memory24->address_length;
>> -               addr->maximum = addr->minimum + addr->address_length - 1;
>> -               return AE_OK;
>> -       case ACPI_RESOURCE_TYPE_MEMORY32:
>> -               memory32 = &resource->data.memory32;
>> -               addr->resource_type = ACPI_MEMORY_RANGE;
>> -               addr->minimum = memory32->minimum;
>> -               addr->address_length = memory32->address_length;
>> -               addr->maximum = addr->minimum + addr->address_length - 1;
>> -               return AE_OK;
>> -       case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
>> -               fixed_memory32 = &resource->data.fixed_memory32;
>> -               addr->resource_type = ACPI_MEMORY_RANGE;
>> -               addr->minimum = fixed_memory32->address;
>> -               addr->address_length = fixed_memory32->address_length;
>> -               addr->maximum = addr->minimum + addr->address_length - 1;
>> -               return AE_OK;
>> -       case ACPI_RESOURCE_TYPE_ADDRESS16:
>> -       case ACPI_RESOURCE_TYPE_ADDRESS32:
>> -       case ACPI_RESOURCE_TYPE_ADDRESS64:
>> -               status = acpi_resource_to_address64(resource, addr);
>> -               if (ACPI_SUCCESS(status) &&
>> -                   (addr->resource_type == ACPI_MEMORY_RANGE ||
>> -                   addr->resource_type == ACPI_IO_RANGE) &&
>> -                   addr->address_length > 0) {
>> -                       return AE_OK;
>> -               }
>> -               break;
>> -       }
>> -       return AE_ERROR;
>> -}
>> -
>> -static acpi_status
>>   count_resource(struct acpi_resource *acpi_res, void *data)
>>   {
>>          struct pci_root_info *info = data;
>> -       struct acpi_resource_address64 addr;
>> -       acpi_status status;
>> +       struct resource res;
>>
>> -       status = resource_to_addr(acpi_res, &addr);
>> -       if (ACPI_SUCCESS(status))
>> +       if (acpi_dev_resource_address_space(acpi_res, &res)
>> +           || acpi_dev_resource_memory(acpi_res, &res))
>>                  info->res_num++;
>> +
>>          return AE_OK;
>>   }
>>
>> @@ -282,27 +235,18 @@ static acpi_status
>>   setup_resource(struct acpi_resource *acpi_res, void *data)
>>   {
>>          struct pci_root_info *info = data;
>> -       struct resource *res;
>> +       struct resource *res = &info->res[info->res_num];
>>          struct acpi_resource_address64 addr;
>> -       acpi_status status;
>> -       unsigned long flags;
>>          u64 start, orig_end, end;
>>
>> -       status = resource_to_addr(acpi_res, &addr);
>> -       if (!ACPI_SUCCESS(status))
>> -               return AE_OK;
>> +       memset(&addr, 0x00, sizeof(addr));
>>
>> -       if (addr.resource_type == ACPI_MEMORY_RANGE) {
>> -               flags = IORESOURCE_MEM;
>> -               if (addr.info.mem.caching == ACPI_PREFETCHABLE_MEMORY)
>> -                       flags |= IORESOURCE_PREFETCH;
>> -       } else if (addr.resource_type == ACPI_IO_RANGE) {
>> -               flags = IORESOURCE_IO;
>> -       } else
>> +       if (!(acpi_dev_resource_address_space_full(acpi_res, &addr, res)
>> +           || acpi_dev_resource_memory(acpi_res, res)))
>>                  return AE_OK;
>>
>> -       start = addr.minimum + addr.translation_offset;
>> -       orig_end = end = addr.maximum + addr.translation_offset;
>> +       start = res->start;
>> +       orig_end = end = res->end;
>>
>>          /* Exclude non-addressable range or non-addressable portion of range */
>>          end = min(end, (u64)iomem_resource.end);
>> @@ -310,6 +254,7 @@ setup_resource(struct acpi_resource *acpi_res, void *data)
>>                  dev_info(&info->bridge->dev,
>>                          "host bridge window [%#llx-%#llx] "
>>                          "(ignored, not CPU addressable)\n", start, orig_end);
>> +               memset(&info->res[info->res_num], 0x00, sizeof(*res));
>>                  return AE_OK;
>>          } else if (orig_end != end) {
>>                  dev_info(&info->bridge->dev,
>> @@ -318,11 +263,9 @@ setup_resource(struct acpi_resource *acpi_res, void *data)
>>                          start, orig_end, end + 1, orig_end);
>>          }
>>
>> -       res = &info->res[info->res_num];
>>          res->name = info->name;
>> -       res->flags = flags;
>> -       res->start = start;
>>          res->end = end;
>> +
>>          info->res_offset[info->res_num] = addr.translation_offset;
>>          info->res_num++;
>>
>> --
>> 1.8.2.1
>>

Re: [Resend PATCH 4/5] X86/PCI/ACPI: Rework setup_resource() via functions ACPI resource functions

[Rafael J. Wysocki]

On Friday, October 11, 2013 08:19:00 PM tianyu.lan@intel.com wrote:
> From: Lan Tianyu <tianyu.lan@intel.com>
> 
> Using ACPI resource functions to convert ACPI resource to generic resource
> instead of resource_to_addr(). Remove resource_to_addr().
> 
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>

Bjorn, Peter, any objections against this?

> ---
>  arch/x86/pci/acpi.c | 81 ++++++++---------------------------------------------
>  1 file changed, 12 insertions(+), 69 deletions(-)
> 
> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
> index d641897..f4290a1 100644
> --- a/arch/x86/pci/acpi.c
> +++ b/arch/x86/pci/acpi.c
> @@ -219,62 +219,15 @@ static void teardown_mcfg_map(struct pci_root_info *info)
>  #endif
>  
>  static acpi_status
> -resource_to_addr(struct acpi_resource *resource,
> -			struct acpi_resource_address64 *addr)
> -{
> -	acpi_status status;
> -	struct acpi_resource_memory24 *memory24;
> -	struct acpi_resource_memory32 *memory32;
> -	struct acpi_resource_fixed_memory32 *fixed_memory32;
> -
> -	memset(addr, 0, sizeof(*addr));
> -	switch (resource->type) {
> -	case ACPI_RESOURCE_TYPE_MEMORY24:
> -		memory24 = &resource->data.memory24;
> -		addr->resource_type = ACPI_MEMORY_RANGE;
> -		addr->minimum = memory24->minimum;
> -		addr->address_length = memory24->address_length;
> -		addr->maximum = addr->minimum + addr->address_length - 1;
> -		return AE_OK;
> -	case ACPI_RESOURCE_TYPE_MEMORY32:
> -		memory32 = &resource->data.memory32;
> -		addr->resource_type = ACPI_MEMORY_RANGE;
> -		addr->minimum = memory32->minimum;
> -		addr->address_length = memory32->address_length;
> -		addr->maximum = addr->minimum + addr->address_length - 1;
> -		return AE_OK;
> -	case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
> -		fixed_memory32 = &resource->data.fixed_memory32;
> -		addr->resource_type = ACPI_MEMORY_RANGE;
> -		addr->minimum = fixed_memory32->address;
> -		addr->address_length = fixed_memory32->address_length;
> -		addr->maximum = addr->minimum + addr->address_length - 1;
> -		return AE_OK;
> -	case ACPI_RESOURCE_TYPE_ADDRESS16:
> -	case ACPI_RESOURCE_TYPE_ADDRESS32:
> -	case ACPI_RESOURCE_TYPE_ADDRESS64:
> -		status = acpi_resource_to_address64(resource, addr);
> -		if (ACPI_SUCCESS(status) &&
> -		    (addr->resource_type == ACPI_MEMORY_RANGE ||
> -		    addr->resource_type == ACPI_IO_RANGE) &&
> -		    addr->address_length > 0) {
> -			return AE_OK;
> -		}
> -		break;
> -	}
> -	return AE_ERROR;
> -}
> -
> -static acpi_status
>  count_resource(struct acpi_resource *acpi_res, void *data)
>  {
>  	struct pci_root_info *info = data;
> -	struct acpi_resource_address64 addr;
> -	acpi_status status;
> +	struct resource res;
>  
> -	status = resource_to_addr(acpi_res, &addr);
> -	if (ACPI_SUCCESS(status))
> +	if (acpi_dev_resource_address_space(acpi_res, &res)
> +	    || acpi_dev_resource_memory(acpi_res, &res))
>  		info->res_num++;
> +
>  	return AE_OK;
>  }
>  
> @@ -282,27 +235,18 @@ static acpi_status
>  setup_resource(struct acpi_resource *acpi_res, void *data)
>  {
>  	struct pci_root_info *info = data;
> -	struct resource *res;
> +	struct resource *res = &info->res[info->res_num];
>  	struct acpi_resource_address64 addr;
> -	acpi_status status;
> -	unsigned long flags;
>  	u64 start, orig_end, end;
>  
> -	status = resource_to_addr(acpi_res, &addr);
> -	if (!ACPI_SUCCESS(status))
> -		return AE_OK;
> +	memset(&addr, 0x00, sizeof(addr));
>  
> -	if (addr.resource_type == ACPI_MEMORY_RANGE) {
> -		flags = IORESOURCE_MEM;
> -		if (addr.info.mem.caching == ACPI_PREFETCHABLE_MEMORY)
> -			flags |= IORESOURCE_PREFETCH;
> -	} else if (addr.resource_type == ACPI_IO_RANGE) {
> -		flags = IORESOURCE_IO;
> -	} else
> +	if (!(acpi_dev_resource_address_space_full(acpi_res, &addr, res)
> +	    || acpi_dev_resource_memory(acpi_res, res)))
>  		return AE_OK;
>  
> -	start = addr.minimum + addr.translation_offset;
> -	orig_end = end = addr.maximum + addr.translation_offset;
> +	start = res->start;
> +	orig_end = end = res->end;
>  
>  	/* Exclude non-addressable range or non-addressable portion of range */
>  	end = min(end, (u64)iomem_resource.end);
> @@ -310,6 +254,7 @@ setup_resource(struct acpi_resource *acpi_res, void *data)
>  		dev_info(&info->bridge->dev,
>  			"host bridge window [%#llx-%#llx] "
>  			"(ignored, not CPU addressable)\n", start, orig_end);
> +		memset(&info->res[info->res_num], 0x00, sizeof(*res));
>  		return AE_OK;
>  	} else if (orig_end != end) {
>  		dev_info(&info->bridge->dev,
> @@ -318,11 +263,9 @@ setup_resource(struct acpi_resource *acpi_res, void *data)
>  			start, orig_end, end + 1, orig_end);
>  	}
>  
> -	res = &info->res[info->res_num];
>  	res->name = info->name;
> -	res->flags = flags;
> -	res->start = start;
>  	res->end = end;
> +
>  	info->res_offset[info->res_num] = addr.translation_offset;
>  	info->res_num++;
>  
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

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

[Rafael J. Wysocki]

On Friday, October 11, 2013 08:19:01 PM tianyu.lan@intel.com wrote:
> From: Lan Tianyu <tianyu.lan@intel.com>
> 
> Using ACPI resource functions to convert ACPI resource to generic resource
> 
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>

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)) {
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [Resend PATCH 2/5] ACPI/Resource: Add address translation support

[Bjorn Helgaas]

On Fri, Oct 11, 2013 at 08:18:58PM +0800, tianyu.lan@intel.com wrote:
> From: Lan Tianyu <tianyu.lan@intel.com>
> 
> According ACPI 5.0 spec Section 19.1.8

This section reference is wrong.  Table 5-133 (on page 243) does
point to Section 19.1.8, but that section is only the ASL grammar
description and doesn't have any useful information about what
_TRA means.

A more useful reference (and the source of the quote below) is
Section 6.4.3.5.1.

> "For bridges, translate addresses across the bridge, this is the
> offset that must be added to the address on the secondary side
> to obtain the address on the primary side. Non-bridge devices
> must list 0."

You didn't quote this correctly.  Here's a copy/paste from the spec:

    For bridges that translate addresses across the bridge, this is
    the offset that must be added to the address on the secondary side
    to obtain the address on the primary side. Non-bridge devices must
    list 0 for all Address Translation offset bits.

> This patch is to add address translation offset to the start/end
> of struct resource in the acpi_dev_resource_address_space().
> Further more, non-bridge device's translation_offset should 0.
> So this change will affect other devices.
> 
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>

I like the patch, though :)  With the above corrections,

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> ---
>  drivers/acpi/resource.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
> index 929f416..84bc3db 100644
> --- a/drivers/acpi/resource.c
> +++ b/drivers/acpi/resource.c
> @@ -192,8 +192,8 @@ bool acpi_dev_resource_address_space(struct acpi_resource *ares,
>  	if (ACPI_FAILURE(status))
>  		return true;
>  
> -	res->start = addr.minimum;
> -	res->end = addr.maximum;
> +	res->start = addr.minimum + addr.translation_offset;
> +	res->end = addr.maximum + addr.translation_offset;
>  	window = addr.producer_consumer == ACPI_PRODUCER;
>  
>  	switch(addr.resource_type) {
> -- 
> 1.8.2.1
> 

Re: [Resend PATCH 3/5] ACPI: Add new acpi_dev_resource_address_space_full() function

[Bjorn Helgaas]

On Fri, Oct 11, 2013 at 08:18:59PM +0800, tianyu.lan@intel.com wrote:
> From: Lan Tianyu <tianyu.lan@intel.com>
> 
> Make acpi_dev_resource_address_space() to accept struct
> acpi_resource_address64 as param and rename it to *_full.
> 
> This is for some cases that acpi address info is also needed
> after convert from acpi resouce to generic resource.

s/resouce/resource/

> Add acpi_dev_resource_addres_space() again as a wrapper of new

acpi_dev_resource_address_space() (spelled wrong above).

> function for original users.
> 
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> ---
>  drivers/acpi/resource.c | 53 ++++++++++++++++++++++++++++++++++---------------
>  include/linux/acpi.h    |  3 +++
>  2 files changed, 40 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
> index 84bc3db..2e395ba 100644
> --- a/drivers/acpi/resource.c
> +++ b/drivers/acpi/resource.c
> @@ -162,19 +162,21 @@ bool acpi_dev_resource_io(struct acpi_resource *ares, struct resource *res)
>  EXPORT_SYMBOL_GPL(acpi_dev_resource_io);
>  
>  /**
> - * acpi_dev_resource_address_space - Extract ACPI address space information.
> + * acpi_dev_resource_address_space_full - Extract ACPI address space information.
>   * @ares: Input ACPI resource object.
> + * @addr: Output ACPI resource address64 space object.
>   * @res: Output generic resource object.
>   *
>   * Check if the given ACPI resource object represents an address space resource
> - * and if that's the case, use the information in it to populate the generic
> - * resource object pointed to by @res.
> + * and if that's the case, convert it to ACPI resource address64 space object
> + * pointed to by @addr and use the information to populate the generic resource
> + * object pointed to by @re.

s/@re/@res/ or maybe even just use this:

"If @ares is an address space resource, convert it to an
acpi_resource_address64 and use that to populate the generic @res."

>   */
> -bool acpi_dev_resource_address_space(struct acpi_resource *ares,
> +bool acpi_dev_resource_address_space_full(struct acpi_resource *ares,
> +				     struct acpi_resource_address64 *addr,
>  				     struct resource *res)
>  {
>  	acpi_status status;
> -	struct acpi_resource_address64 addr;
>  	bool window;
>  	u64 len;
>  	u8 io_decode;
> @@ -188,29 +190,29 @@ bool acpi_dev_resource_address_space(struct acpi_resource *ares,
>  		return false;
>  	}
>  
> -	status = acpi_resource_to_address64(ares, &addr);
> +	status = acpi_resource_to_address64(ares, addr);
>  	if (ACPI_FAILURE(status))
>  		return true;

You didn't actually change this, but it looks wrong to return "true"
here, because we haven't filled in "res".  Returning "true" will cause
acpi_dev_process_resource() to call acpi_dev_new_resource_entry() with
a struct resource that's full of zeroes.

I think the best fix would be to remove this code just above:

        switch (ares->type) {
        case ACPI_RESOURCE_TYPE_ADDRESS16:
        case ACPI_RESOURCE_TYPE_ADDRESS32:
        case ACPI_RESOURCE_TYPE_ADDRESS64:
                break;
        default:
                return false;
        }

and return "false" if acpi_resource_to_address64() fails.

>  
> -	res->start = addr.minimum + addr.translation_offset;
> -	res->end = addr.maximum + addr.translation_offset;
> -	window = addr.producer_consumer == ACPI_PRODUCER;
> +	res->start = addr->minimum + addr->translation_offset;
> +	res->end = addr->maximum + addr->translation_offset;
> +	window = addr->producer_consumer == ACPI_PRODUCER;
>  
> -	switch(addr.resource_type) {
> +	switch (addr->resource_type) {
>  	case ACPI_MEMORY_RANGE:
> -		len = addr.maximum - addr.minimum + 1;
> +		len = addr->maximum - addr->minimum + 1;
>  		res->flags = acpi_dev_memresource_flags(len,
> -						addr.info.mem.write_protect,
> +						addr->info.mem.write_protect,
>  						window);
>  
> -		if (addr.info.mem.caching == ACPI_PREFETCHABLE_MEMORY)
> +		if (addr->info.mem.caching == ACPI_PREFETCHABLE_MEMORY)
>  			res->flags |= IORESOURCE_PREFETCH;
>  		break;
>  	case ACPI_IO_RANGE:
> -		io_decode = addr.granularity == 0xfff ?
> +		io_decode = addr->granularity == 0xfff ?
>  				ACPI_DECODE_10 : ACPI_DECODE_16;
> -		res->flags = acpi_dev_ioresource_flags(addr.minimum,
> -						       addr.maximum,
> +		res->flags = acpi_dev_ioresource_flags(addr->minimum,
> +						       addr->maximum,
>  						       io_decode, window);
>  		break;
>  	case ACPI_BUS_NUMBER_RANGE:
> @@ -222,6 +224,25 @@ bool acpi_dev_resource_address_space(struct acpi_resource *ares,
>  
>  	return true;
>  }
> +EXPORT_SYMBOL_GPL(acpi_dev_resource_address_space_full);
> +
> +/**
> + * acpi_dev_resource_address_space - Extract ACPI address space information.
> + * @ares: Input ACPI resource object.
> + * @res: Output generic resource object.
> + *
> + * Check if the given ACPI resource object represents an address space resource
> + * and if that's the case, use the information in it to populate the generic
> + * resource object pointed to by @res.

"If @ares is an address space resource, use it to populate the generic @res."

> + */
> +bool acpi_dev_resource_address_space(struct acpi_resource *ares,
> +				     struct resource *res)
> +{
> +	struct acpi_resource_address64 addr;
> +
> +	return acpi_dev_resource_address_space_full(ares, &addr,
> +							 res);
> +}
>  EXPORT_SYMBOL_GPL(acpi_dev_resource_address_space);
>  
>  /**
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index a5db4ae..1f6701e 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -260,6 +260,9 @@ bool acpi_dev_resource_memory(struct acpi_resource *ares, struct resource *res);
>  bool acpi_dev_resource_io(struct acpi_resource *ares, struct resource *res);
>  bool acpi_dev_resource_address_space(struct acpi_resource *ares,
>  				     struct resource *res);
> +bool acpi_dev_resource_address_space_full(struct acpi_resource *ares,
> +				struct acpi_resource_address64 *addr,
> +				struct resource *res);
>  bool acpi_dev_resource_ext_address_space(struct acpi_resource *ares,
>  					 struct resource *res);
>  unsigned long acpi_dev_irq_flags(u8 triggering, u8 polarity, u8 shareable);
> -- 
> 1.8.2.1
> 

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

[Bjorn Helgaas]

On Fri, Oct 11, 2013 at 08:19:01PM +0800, tianyu.lan@intel.com wrote:
> From: Lan Tianyu <tianyu.lan@intel.com>
> 
> Using ACPI resource functions to convert ACPI resource to generic resource
> 
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> ---
> 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
> 

Re: [Resend PATCH 2/5] ACPI/Resource: Add address translation support

[Lan Tianyu]

On 2013年10月17日 07:05, Bjorn Helgaas wrote:
> On Fri, Oct 11, 2013 at 08:18:58PM +0800, tianyu.lan@intel.com wrote:
>> From: Lan Tianyu <tianyu.lan@intel.com>
>>
>> According ACPI 5.0 spec Section 19.1.8
> 
> This section reference is wrong.  Table 5-133 (on page 243) does
> point to Section 19.1.8, but that section is only the ASL grammar
> description and doesn't have any useful information about what
> _TRA means.
> 
> A more useful reference (and the source of the quote below) is
> Section 6.4.3.5.1.
> 
>> "For bridges, translate addresses across the bridge, this is the
>> offset that must be added to the address on the secondary side
>> to obtain the address on the primary side. Non-bridge devices
>> must list 0."
> 
> You didn't quote this correctly.  Here's a copy/paste from the spec:
> 
>     For bridges that translate addresses across the bridge, this is
>     the offset that must be added to the address on the secondary side
>     to obtain the address on the primary side. Non-bridge devices must
>     list 0 for all Address Translation offset bits.
> 
>> This patch is to add address translation offset to the start/end
>> of struct resource in the acpi_dev_resource_address_space().
>> Further more, non-bridge device's translation_offset should 0.
>> So this change will affect other devices.
>>
>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> 
> I like the patch, though :)  With the above corrections,

Hi Bjorn:
	Thanks for review. I will correct them.

> 
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> 
>> ---
>>  drivers/acpi/resource.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
>> index 929f416..84bc3db 100644
>> --- a/drivers/acpi/resource.c
>> +++ b/drivers/acpi/resource.c
>> @@ -192,8 +192,8 @@ bool acpi_dev_resource_address_space(struct acpi_resource *ares,
>>  	if (ACPI_FAILURE(status))
>>  		return true;
>>  
>> -	res->start = addr.minimum;
>> -	res->end = addr.maximum;
>> +	res->start = addr.minimum + addr.translation_offset;
>> +	res->end = addr.maximum + addr.translation_offset;
>>  	window = addr.producer_consumer == ACPI_PRODUCER;
>>  
>>  	switch(addr.resource_type) {
>> -- 
>> 1.8.2.1
>>


-- 
Best regards
Tianyu Lan

Re: [Resend PATCH 3/5] ACPI: Add new acpi_dev_resource_address_space_full() function

[Lan Tianyu]

On 2013年10月17日 07:18, Bjorn Helgaas wrote:
> On Fri, Oct 11, 2013 at 08:18:59PM +0800, tianyu.lan@intel.com wrote:
>> From: Lan Tianyu <tianyu.lan@intel.com>
>>
>> Make acpi_dev_resource_address_space() to accept struct
>> acpi_resource_address64 as param and rename it to *_full.
>>
>> This is for some cases that acpi address info is also needed
>> after convert from acpi resouce to generic resource.
> 
> s/resouce/resource/
> 
>> Add acpi_dev_resource_addres_space() again as a wrapper of new
> 
> acpi_dev_resource_address_space() (spelled wrong above).
> 
>> function for original users.
>>
>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
>> ---
>>  drivers/acpi/resource.c | 53 ++++++++++++++++++++++++++++++++++---------------
>>  include/linux/acpi.h    |  3 +++
>>  2 files changed, 40 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
>> index 84bc3db..2e395ba 100644
>> --- a/drivers/acpi/resource.c
>> +++ b/drivers/acpi/resource.c
>> @@ -162,19 +162,21 @@ bool acpi_dev_resource_io(struct acpi_resource *ares, struct resource *res)
>>  EXPORT_SYMBOL_GPL(acpi_dev_resource_io);
>>  
>>  /**
>> - * acpi_dev_resource_address_space - Extract ACPI address space information.
>> + * acpi_dev_resource_address_space_full - Extract ACPI address space information.
>>   * @ares: Input ACPI resource object.
>> + * @addr: Output ACPI resource address64 space object.
>>   * @res: Output generic resource object.
>>   *
>>   * Check if the given ACPI resource object represents an address space resource
>> - * and if that's the case, use the information in it to populate the generic
>> - * resource object pointed to by @res.
>> + * and if that's the case, convert it to ACPI resource address64 space object
>> + * pointed to by @addr and use the information to populate the generic resource
>> + * object pointed to by @re.
> 
> s/@re/@res/ or maybe even just use this:
> 
> "If @ares is an address space resource, convert it to an
> acpi_resource_address64 and use that to populate the generic @res."
> 
>>   */
>> -bool acpi_dev_resource_address_space(struct acpi_resource *ares,
>> +bool acpi_dev_resource_address_space_full(struct acpi_resource *ares,
>> +				     struct acpi_resource_address64 *addr,
>>  				     struct resource *res)
>>  {
>>  	acpi_status status;
>> -	struct acpi_resource_address64 addr;
>>  	bool window;
>>  	u64 len;
>>  	u8 io_decode;
>> @@ -188,29 +190,29 @@ bool acpi_dev_resource_address_space(struct acpi_resource *ares,
>>  		return false;
>>  	}
>>  
>> -	status = acpi_resource_to_address64(ares, &addr);
>> +	status = acpi_resource_to_address64(ares, addr);
>>  	if (ACPI_FAILURE(status))
>>  		return true;
> 
> You didn't actually change this, but it looks wrong to return "true"
> here, because we haven't filled in "res".  Returning "true" will cause
> acpi_dev_process_resource() to call acpi_dev_new_resource_entry() with
> a struct resource that's full of zeroes.
> 
> I think the best fix would be to remove this code just above:
> 
>         switch (ares->type) {
>         case ACPI_RESOURCE_TYPE_ADDRESS16:
>         case ACPI_RESOURCE_TYPE_ADDRESS32:
>         case ACPI_RESOURCE_TYPE_ADDRESS64:
>                 break;
>         default:
>                 return false;
>         }
> 
> and return "false" if acpi_resource_to_address64() fails.

Yes, Just check acpi_resource_to_address64() and it only processes
ACPI_RESOURCE_TYPE_ADDRESS16, ACPI_RESOURCE_TYPE_ADDRESS32 and
ACPI_RESOURCE_TYPE_ADDRESS64. If other type,  it will return false.
I will change it.

> 
>>  
>> -	res->start = addr.minimum + addr.translation_offset;
>> -	res->end = addr.maximum + addr.translation_offset;
>> -	window = addr.producer_consumer == ACPI_PRODUCER;
>> +	res->start = addr->minimum + addr->translation_offset;
>> +	res->end = addr->maximum + addr->translation_offset;
>> +	window = addr->producer_consumer == ACPI_PRODUCER;
>>  
>> -	switch(addr.resource_type) {
>> +	switch (addr->resource_type) {
>>  	case ACPI_MEMORY_RANGE:
>> -		len = addr.maximum - addr.minimum + 1;
>> +		len = addr->maximum - addr->minimum + 1;
>>  		res->flags = acpi_dev_memresource_flags(len,
>> -						addr.info.mem.write_protect,
>> +						addr->info.mem.write_protect,
>>  						window);
>>  
>> -		if (addr.info.mem.caching == ACPI_PREFETCHABLE_MEMORY)
>> +		if (addr->info.mem.caching == ACPI_PREFETCHABLE_MEMORY)
>>  			res->flags |= IORESOURCE_PREFETCH;
>>  		break;
>>  	case ACPI_IO_RANGE:
>> -		io_decode = addr.granularity == 0xfff ?
>> +		io_decode = addr->granularity == 0xfff ?
>>  				ACPI_DECODE_10 : ACPI_DECODE_16;
>> -		res->flags = acpi_dev_ioresource_flags(addr.minimum,
>> -						       addr.maximum,
>> +		res->flags = acpi_dev_ioresource_flags(addr->minimum,
>> +						       addr->maximum,
>>  						       io_decode, window);
>>  		break;
>>  	case ACPI_BUS_NUMBER_RANGE:
>> @@ -222,6 +224,25 @@ bool acpi_dev_resource_address_space(struct acpi_resource *ares,
>>  
>>  	return true;
>>  }
>> +EXPORT_SYMBOL_GPL(acpi_dev_resource_address_space_full);
>> +
>> +/**
>> + * acpi_dev_resource_address_space - Extract ACPI address space information.
>> + * @ares: Input ACPI resource object.
>> + * @res: Output generic resource object.
>> + *
>> + * Check if the given ACPI resource object represents an address space resource
>> + * and if that's the case, use the information in it to populate the generic
>> + * resource object pointed to by @res.
> 
> "If @ares is an address space resource, use it to populate the generic @res."
> 
>> + */
>> +bool acpi_dev_resource_address_space(struct acpi_resource *ares,
>> +				     struct resource *res)
>> +{
>> +	struct acpi_resource_address64 addr;
>> +
>> +	return acpi_dev_resource_address_space_full(ares, &addr,
>> +							 res);
>> +}
>>  EXPORT_SYMBOL_GPL(acpi_dev_resource_address_space);
>>  
>>  /**
>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>> index a5db4ae..1f6701e 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -260,6 +260,9 @@ bool acpi_dev_resource_memory(struct acpi_resource *ares, struct resource *res);
>>  bool acpi_dev_resource_io(struct acpi_resource *ares, struct resource *res);
>>  bool acpi_dev_resource_address_space(struct acpi_resource *ares,
>>  				     struct resource *res);
>> +bool acpi_dev_resource_address_space_full(struct acpi_resource *ares,
>> +				struct acpi_resource_address64 *addr,
>> +				struct resource *res);
>>  bool acpi_dev_resource_ext_address_space(struct acpi_resource *ares,
>>  					 struct resource *res);
>>  unsigned long acpi_dev_irq_flags(u8 triggering, u8 polarity, u8 shareable);
>> -- 
>> 1.8.2.1
>>


-- 
Best regards
Tianyu Lan

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

[Lan Tianyu]

On 2013年10月17日 07:55, Bjorn Helgaas wrote:
> On Fri, Oct 11, 2013 at 08:19:01PM +0800, tianyu.lan@intel.com wrote:
>> From: Lan Tianyu <tianyu.lan@intel.com>
>>
>> Using ACPI resource functions to convert ACPI resource to generic resource
>>
>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
>> ---
>> 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
>>


-- 
Best regards
Tianyu Lan

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

[Bjorn Helgaas]

[+cc Mike]

On Thu, Oct 17, 2013 at 12:09 AM, Lan Tianyu <tianyu.lan@intel.com> wrote:
> On 2013年10月17日 07:55, Bjorn Helgaas wrote:
>> On Fri, Oct 11, 2013 at 08:19:01PM +0800, tianyu.lan@intel.com wrote:
>>> From: Lan Tianyu <tianyu.lan@intel.com>
>>>
>>> Using ACPI resource functions to convert ACPI resource to generic resource
>>>
>>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
>>> ---
>>> 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

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

[Lan Tianyu]

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@intel.com>
> wrote:
>> On 2013年10月17日 07:55, Bjorn Helgaas wrote:
>>> On Fri, Oct 11, 2013 at 08:19:01PM +0800, tianyu.lan@intel.com
>>> wrote:
>>>> From: Lan Tianyu <tianyu.lan@intel.com>
>>>>
>>>> Using ACPI resource functions to convert ACPI resource to
>>>> generic resource
>>>>
>>>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> --- 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
>

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

[Bjorn Helgaas]

[+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 <tianyu.lan@intel.com> wrote:
>>>On 2013年10月17日 07:55, Bjorn Helgaas wrote:
>>>> On Fri, Oct 11, 2013 at 08:19:01PM +0800, tianyu.lan@intel.com 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

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

[Lan Tianyu]

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 <tianyu.lan@intel.com> wrote:
>>>> On 2013年10月17日 07:55, Bjorn Helgaas wrote:
>>>>> On Fri, Oct 11, 2013 at 08:19:01PM +0800, tianyu.lan@intel.com 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
>

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

[Bjorn Helgaas]

On Sat, Oct 26, 2013 at 10:53 AM, Lan Tianyu <tianyu.lan@intel.com> 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

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

[Lan Tianyu]

On 2013年10月29日 01:32, Bjorn Helgaas wrote:
> On Sat, Oct 26, 2013 at 10:53 AM, Lan Tianyu <tianyu.lan@intel.com> 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
>


-- 
Best regards
Tianyu Lan

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

[Bjorn Helgaas]

On Wed, Oct 30, 2013 at 2:34 AM, Lan Tianyu <tianyu.lan@intel.com> wrote:
> On 2013年10月29日 01:32, Bjorn Helgaas wrote:
>>
>> On Sat, Oct 26, 2013 at 10:53 AM, Lan Tianyu <tianyu.lan@intel.com> 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

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

[Lan Tianyu]

On 2013年10月31日 00:23, Bjorn Helgaas wrote:
> On Wed, Oct 30, 2013 at 2:34 AM, Lan Tianyu <tianyu.lan@intel.com> wrote:
>> On 2013年10月29日 01:32, Bjorn Helgaas wrote:
>>>
>>> On Sat, Oct 26, 2013 at 10:53 AM, Lan Tianyu <tianyu.lan@intel.com> 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
> 


-- 
Best regards
Tianyu Lan

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

[Bjorn Helgaas]

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 <tianyu.lan@intel.com> wrote:
> >> On 2013年10月29日 01:32, Bjorn Helgaas wrote:
> >>>
> >>> On Sat, Oct 26, 2013 at 10:53 AM, Lan Tianyu <tianyu.lan@intel.com> 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