On Mon, Aug 26, 2013 at 8:46 AM, Lan Tianyu <lantianyu1986@xxxxxxxxx> wrote:
> 2013/8/24 Yinghai Lu <yinghai@xxxxxxxxxx>:
>> On Fri, Aug 23, 2013 at 12:04 PM, Yinghai Lu <yinghai@xxxxxxxxxx> wrote:
>>> On Fri, Aug 23, 2013 at 11:38 AM, Yinghai Lu <yinghai@xxxxxxxxxx> wrote:
>>>> On Fri, Aug 23, 2013 at 8:34 AM, Lan Tianyu <lantianyu1986@xxxxxxxxx> wrote:
>>>>>> I worked around the problem by replacing acpi_resource_to_address64()
>>>>>> with resource_to_addr(). But resource_to_addr() is a static function
>>>>>> in arch/x86/pci/acpi.c, not very convenient to use. Here's what I did:
>>>>>>
>>>>>
>>>>> Hi Rui&Yinghai:
>>>>> How about using the following code to translate struct
>>>>> acpi_resource to struct resouce in this setup_res()?
>>>>>
>>>>> if (acpi_dev_resource_address_space(...)
>>>>> || acpi_dev_resource_memory(..))
>>>>> return AE_OK;
>>>>
>>>> Yest, that could be better, will update that.
>>>>
>>>> Also can you submit patch that will use that in res_to_addr of
>>>> arch/x86/pci/acpi.c?
>>>
>>> looks acpi_dev_resource_address_space... does not handle
>>> PREFTCH and translation offset.
>>>
>>> So now i have to use res_to_addr alike one.
>>
>> Raphael,
>>
>> Maybe we should move resource_to_addr to acpi generic.
>>
>> Please check if you are ok with attached.
>>
>> Thanks
>>
>> Yinghai
>
> Hi Rafael & Yinghai:
> I wrote 4 proposal patches to try to make acpi resource
> functions to replace
> resource_to_addr() in the arch/x86/pci/acpi.c. From my opinion,
> resource_to_addr()
> does most work which acpi resource functions have done. So please have
> a look. Thanks.
> The following patches just passed through compiling test.
Thanks for doing that.
Overall it's Good to me.
some comments inline.
>
> Patch 1
> -------------------
> commit 1800bc9dda7318314607fbae7afda8be38e056dc
> Author: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> Date: Mon Aug 26 14:40:39 2013 +0800
>
> ACPI/Resource: Add setting PREFETCH flag support
>
> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
> index b7201fc..23a560b 100644
> --- a/drivers/acpi/resource.c
> +++ b/drivers/acpi/resource.c
> @@ -35,7 +35,7 @@
> #endif
>
> static unsigned long acpi_dev_memresource_flags(u64 len, u8 write_protect,
> - bool window)
> + bool window, bool prefetchable)
> {
> unsigned long flags = IORESOURCE_MEM;
>
> @@ -48,6 +48,9 @@ static unsigned long acpi_dev_memresource_flags(u64
> len, u8 write_protect,
> if (window)
> flags |= IORESOURCE_WINDOW;
>
> + if (prefetchable)
> + flags |= IORESOURCE_PREFETCH;
> +
> return flags;
> }
>
> @@ -56,7 +59,8 @@ static void acpi_dev_get_memresource(struct resource
> *res, u64 start, u64 len,
> {
> res->start = start;
> res->end = start + len - 1;
> - res->flags = acpi_dev_memresource_flags(len, write_protect, false);
> + res->flags = acpi_dev_memresource_flags(len, write_protect,
> + false, false);
> }
passing write_protect, window then pref looks silly.
may use | IO_REOURCE_... etc outside of acpi_dev_memresource_flags directly.
>
> /**
> @@ -175,7 +179,7 @@ bool acpi_dev_resource_address_space(struct
> acpi_resource *ares,
> {
> acpi_status status;
> struct acpi_resource_address64 addr;
> - bool window;
> + bool window, prefetchable;
> u64 len;
> u8 io_decode;
>
> @@ -199,9 +203,11 @@ bool acpi_dev_resource_address_space(struct
> acpi_resource *ares,
> switch(addr.resource_type) {
> case ACPI_MEMORY_RANGE:
> len = addr.maximum - addr.minimum + 1;
> + prefetchable =
> + addr.info.mem.caching == ACPI_PREFETCHABLE_MEMORY;
> res->flags = acpi_dev_memresource_flags(len,
> addr.info.mem.write_protect,
> - window);
> + window, prefetchable);
> break;
> case ACPI_IO_RANGE:
> io_decode = addr.granularity == 0xfff ?
> @@ -252,7 +258,7 @@ bool acpi_dev_resource_ext_address_space(struct
> acpi_resource *ares,
> len = ext_addr->maximum - ext_addr->minimum + 1;
> res->flags = acpi_dev_memresource_flags(len,
> ext_addr->info.mem.write_protect,
> - window);
> + window, false);
> break;
> case ACPI_IO_RANGE:
> io_decode = ext_addr->granularity == 0xfff ?
>
> Patch 2
> -----------
> commit a3ff8345ffb51885b216ba0ae231252cd88d4e76
> Author: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> Date: Mon Aug 26 15:57:14 2013 +0800
>
> ACPI/Resource: Add address translation support
>
> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
> index 23a560b..439ee44 100644
> --- a/drivers/acpi/resource.c
> +++ b/drivers/acpi/resource.c
> @@ -196,8 +196,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) {
>
>
> Patch 3
> ---------------
> commit 68bfefe619b49baa5d372e0664a6cb6d5138fad1
> Author: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> Date: Mon Aug 26 21:45:32 2013 +0800
>
> ACPI: Add new acpi_dev_resource_address_space_with_addr() function
>
> Add new function which can return the converted struct
> acpi_resource_address64.
> This will be used to get transaction offset in the setup_resource().
>
> diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
> index 439ee44..98a6c35 100644
> --- a/drivers/acpi/resource.c
> +++ b/drivers/acpi/resource.c
> @@ -174,11 +174,11 @@ EXPORT_SYMBOL_GPL(acpi_dev_resource_io);
> * 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,
> +bool acpi_dev_resource_address_space_with_addr(struct acpi_resource *ares,
> + struct acpi_resource_address64 *addr,
> struct resource *res)
> {
> acpi_status status;
> - struct acpi_resource_address64 addr;
> bool window, prefetchable;
> u64 len;
> u8 io_decode;
> @@ -192,28 +192,28 @@ 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;
> prefetchable =
> - addr.info.mem.caching == ACPI_PREFETCHABLE_MEMORY;
> + addr->info.mem.caching == ACPI_PREFETCHABLE_MEMORY;
> res->flags = acpi_dev_memresource_flags(len,
> - addr.info.mem.write_protect,
> + addr->info.mem.write_protect,
> window, prefetchable);
> 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:
> @@ -225,6 +225,16 @@ bool acpi_dev_resource_address_space(struct
> acpi_resource *ares,
>
> return true;
> }
> +EXPORT_SYMBOL_GPL(acpi_dev_resource_address_space_with_addr);
> +
> +bool acpi_dev_resource_address_space(struct acpi_resource *ares,
> + struct resource *res)
> +{
> + struct acpi_resource_address64 addr;
> +
> + return acpi_dev_resource_address_space_with_addr(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..9f5c0d5 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_with_addr(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);
>
> Patch 4
> --------------
> commit a8733a1da756a257d55e68994268174b84b33670
> Author: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> Date: Mon Aug 26 22:53:38 2013 +0800
>
> X86/PCI/ACPI: Rework setup_resource() via functions acpi resource functions
>
> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
> index d641897..d4f85a1 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_with_addr(acpi_res, &addr, res)
> + || acpi_dev_resource_memory(acpi_res, res)))
acpi_dev_resource_address_space_with_addr(acpi_res, &addr, res)
passing extra addr, just for translation_offset?
maybe pass res_offset directly?
> 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;
Yinghai
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
