On Fri, Nov 18, 2016 at 10:22 AM, Dongdong Liu <liudongdong3@xxxxxxxxxx> 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@xxxxxxxxxx>
> Signed-off-by: Tomasz Nowicki <tn@xxxxxxxxxxxx>
> ---
> 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?
> +{
> + 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?
> + "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.
> + 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().
> +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.
> + 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.
> + return ret;
> + }
> +
> + return 0;
> +}
It looks like this function could return the resource pointer just
fine. What's the problem with that?
> +#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?
> +
> #endif /* DRIVERS_PCI_H */
> --
Thanks,
Rafael
--
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
