Re: [PATCH V5 1/2] PCI/ACPI: Provide acpi_get_rc_resources() for ARM64 platform

[Dongdong Liu]

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

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
>
> .