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

[Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>]

On Tue, Nov 22, 2016 at 04:09:57PM +0000, Gabriele Paoloni wrote:

[...]

> > On Tue, Nov 22, 2016 at 08:08:48PM +0800, Dongdong Liu 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
> > returns
> > > 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.
> > 
> > Here (ie interpreting the _UID as a segment identifier) is where we
> > start interpreting the specs instead of relying on them, if you want to
> > go ahead with this feel free but I do not like this attitude at all.
> 
> Obviously there are no specs to specify the segment id of a quirk.

That's the reason why _UID (or any other standard object) should not be
(ab)used to do that, again, that's just my opinion.

> Looking at the ACPI specs it seems to me that the _UID is no worse
> than other device identification objects...

There are no bindings in the specs describing what you need. If you want
to say "for this _HID, _UID means something completely different
- ie root bridge segment identifier - go ahead if that's fine with Bjorn
and Rafael; another solution would be adding a _DSM/DSD to the PNP0c02
device to retrieve the segment number, other than that I have run out
of ideas.

Lorenzo

> 
> > 
> > > Signed-off-by: Dongdong Liu <liudongdong3@huawei.com>
> > > Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> > > ---
> > >  drivers/pci/pci-acpi.c | 69
> > ++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  drivers/pci/pci.h      |  4 +++
> > >  2 files changed, 73 insertions(+)
> > >
> > > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> > > index d966d47..76fd6f4 100644
> > > --- a/drivers/pci/pci-acpi.c
> > > +++ b/drivers/pci/pci-acpi.c
> > > @@ -29,6 +29,75 @@
> > >  	0x91, 0x17, 0xea, 0x4d, 0x19, 0xc3, 0x43, 0x4d
> > >  };
> > >
> > > +#ifdef CONFIG_ARM64
> > 
> > CONFIG_PCI_ECAM_QUIRKS (that you will introduce earlier in your
> > series).
> > 
> > The way you did it seems to imply that on ARM64 to get RC address we
> > need to (and are allowed to) use this function (and related, *Linux
> > specific*, ACPI bindings) and that's a generic way of getting config
> > space. It is not and it must not be and I consider it a bit of a
> > stretch
> > to have it in generic code but I can live with that as long as it comes
> > with its own config option that describes that this stuff is
> > non-compliant code used by non-compliant platform drivers.
> 
> Yes probably better to have
> 
> #ifdef CONFIG_PCI_ECAM_QUIRKS
> 
> Thanks
> 
> Gab
> 
> > 
> > Thanks,
> > Lorenzo
> > 
> > > +static struct resource *acpi_get_rc_addr(struct acpi_device *adev)
> > > +{
> > > +	struct resource_entry *entry;
> > > +	struct list_head list;
> > > +	unsigned long flags;
> > > +	int ret;
> > > +	struct resource *res;
> > > +
> > > +	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)
> > > +		return NULL;
> > > +
> > > +	entry = list_first_entry(&list, struct resource_entry, node);
> > > +	res = entry->res;
> > > +	acpi_dev_free_resource_list(&list);
> > > +	return res;
> > > +}
> > > +
> > > +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;
> > > +}
> > > +
> > > +/**
> > > + * acpi_get_rc_resources() - get the RC address resource.
> > > + * @hid:	HID to search for.
> > > + * @segment:	PCI Segment number.
> > > + *
> > > + * Get the RC address resource that can not be described in MCFG. It
> > takes
> > > + * the _HID&segment to look for and returns the RC address resource.
> > Use
> > > + * _CRS of PNP0C02 devices to describe such RC address resource. Use
> > _UID
> > > + * to match segment to tell which root bus the PNP0C02 resource
> > belong to.
> > > + *
> > > + * Return: RC address resource.
> > > + */
> > > +struct resource *acpi_get_rc_resources(const char *hid, u16 segment)
> > > +{
> > > +	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))
> > > +		return -ENODEV;
> > > +
> > > +	ret = acpi_bus_get_device(handle, &adev);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	return acpi_get_rc_addr(adev);
> > > +}
> > > +#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..bf1dbfe 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
> > >
> > > +#if defined(CONFIG_ARM64) && defined(CONFIG_ACPI)
> > > +struct resource *acpi_get_rc_resources(const char *hid, u16
> > segment);
> > > +#endif
> > > +
> > >  #endif /* DRIVERS_PCI_H */
> > > --
> > > 1.9.1
> > >