lkml.org 
[lkml]   [2016]   [Nov]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH V5 1/2] PCI/ACPI: Provide acpi_get_rc_resources() for ARM64 platform
From
Date
Hi Rafael

Many Thanks for your review.

在 2016/11/19 6:00, Rafael J. Wysocki 写道:
> On Fri, Nov 18, 2016 at 10:22 AM, Dongdong Liu <liudongdong3@huawei.com> wrote:
>> The acpi_get_rc_resources() is used to get the RC register address that can
>> not be described in MCFG. It takes the _HID&segment to look for and outputs
>> the RC address resource. Use PNP0C02 devices to describe such RC address
>> resource. Use _UID to match segment to tell which root bus the PNP0C02
>> resource belong to.
>>
>> Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
>> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
>> ---
>> drivers/pci/pci-acpi.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++
>> drivers/pci/pci.h | 4 +++
>> 2 files changed, 75 insertions(+)
>>
>> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
>> index d966d47..3557e3a 100644
>> --- a/drivers/pci/pci-acpi.c
>> +++ b/drivers/pci/pci-acpi.c
>> @@ -29,6 +29,77 @@
>> 0x91, 0x17, 0xea, 0x4d, 0x19, 0xc3, 0x43, 0x4d
>> };
>>
>> +#ifdef CONFIG_ARM64
>> +static int acpi_get_rc_addr(struct acpi_device *adev, struct resource *res)
>
> Why can't it return a resource pointer?

Yes, it can return a resource pointer.
Good catch. will fix.

>
>> +{
>> + struct resource_entry *entry;
>> + struct list_head list;
>> + unsigned long flags;
>> + int ret;
>> +
>> + INIT_LIST_HEAD(&list);
>> + flags = IORESOURCE_MEM;
>> + ret = acpi_dev_get_resources(adev, &list,
>> + acpi_dev_filter_resource_type_cb,
>> + (void *) flags);
>> + if (ret < 0) {
>> + dev_err(&adev->dev,
>
> The dev_err() log level is quite excessive here IMO. Why is the
> message useful to users at all? IOW, what is the user supposed to do
> if this message is present in the log?

Ok, this is not really needed, I will delete it.
>
>> + "failed to parse _CRS method, error code %d\n", ret);
>> + return ret;
>> + } else if (ret == 0) {
>> + dev_err(&adev->dev,
>> + "no IO and memory resources present in _CRS\n");
>
> Same here.
will delete.
>
>> + return -EINVAL;
>> + }
>> +
>> + entry = list_first_entry(&list, struct resource_entry, node);
>> + *res = *entry->res;
>> + acpi_dev_free_resource_list(&list);
>> + return 0;
>> +}
>> +
>> +static acpi_status acpi_match_rc(acpi_handle handle, u32 lvl, void *context,
>> + void **retval)
>> +{
>> + u16 *segment = context;
>> + unsigned long long uid;
>> + acpi_status status;
>> +
>> + status = acpi_evaluate_integer(handle, "_UID", NULL, &uid);
>> + if (ACPI_FAILURE(status) || uid != *segment)
>> + return AE_CTRL_DEPTH;
>> +
>> + *(acpi_handle *)retval = handle;
>> + return AE_CTRL_TERMINATE;
>> +}
>> +
>
> Please add a kerneldoc comment describing acpi_get_rc_resources().
OK, will do.
>
>> +int acpi_get_rc_resources(const char *hid, u16 segment, struct resource *res)
>> +{
>> + struct acpi_device *adev;
>> + acpi_status status;
>> + acpi_handle handle;
>> + int ret;
>> +
>> + status = acpi_get_devices(hid, acpi_match_rc, &segment, &handle);
>> + if (ACPI_FAILURE(status)) {
>> + pr_err("Can't find _HID %s device", hid);
>
> Same here.

will delete
>
>> + return -ENODEV;
>> + }
>> +
>> + ret = acpi_bus_get_device(handle, &adev);
>> + if (ret)
>> + return ret;
>> +
>> + ret = acpi_get_rc_addr(adev, res);
>> + if (ret) {
>> + dev_err(&adev->dev, "can't get RC resource");
>
> Same here.

will delete
>
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>
> It looks like this function could return the resource pointer just
> fine. What's the problem with that?
It is ok to return the resource pointer, will fix it.
>
>> +#endif
>> +
>> phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle)
>> {
>> acpi_status status = AE_NOT_EXIST;
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index 4518562..17ffa38 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -356,4 +356,8 @@ static inline int pci_dev_specific_reset(struct pci_dev *dev, int probe)
>> }
>> #endif
>>
>> +#ifdef CONFIG_ARM64
>> +int acpi_get_rc_resources(const char *hid, u16 segment, struct resource *res);
>> +#endif
>
> Doesn't that depend on ACPI too?

Yes, that depends on ACPI too.
will fix.

Thanks
Dongdong.
>
>> +
>> #endif /* DRIVERS_PCI_H */
>> --
>
> Thanks,
> Rafael
>
> .
>

\
 
 \ /
  Last update: 2016-11-21 09:21    [W:0.139 / U:0.728 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site