Re: [PATCH v4 28/28] PCI, x86, ACPI: get ioapic address from acpi device

[Lan Tianyu]

2013/8/27 Yinghai Lu <yinghai@xxxxxxxxxx>:
> 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.
>

Yes, this looks better.

>>
>>  /**
>> @@ -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?
>

For this case,  it's "Yes", . For some other cases(e.g
arch/ia64/pci/pci.c add_window()),
whole addr maybe necessary. To be more general, I hope it can return more infos

> 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



-- 
Best regards
Tianyu Lan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/