Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling for ACPI based PCI host controller

[Christopher Covington]

On 06/17/2016 04:01 AM, Gabriele Paoloni wrote:
> Hi Lorenzo and All
> 
>> -----Original Message-----
>> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@xxxxxxx]
>> Sent: 16 June 2016 18:49
>> To: Christopher Covington
>> Cc: Tomasz Nowicki; Duc Dang; liudongdong (C); Sinan Kaya; Jeff Hugo;
>> Gabriele Paoloni; Jon Masters; Mark Salter; Suravee Suthikulpanit;
>> Jayachandran C; David Daney; Robert Richter; Hanjun Guo; linux-arm-
>> kernel@xxxxxxxxxxxxxxxxxxx; Catalin Marinas; Will Deacon; Bjorn Helgaas;
>> Ganapatrao Kulkarni; linux-kernel@xxxxxxxxxxxxxxx
>> Subject: Re: [RFC PATCH v3 2/2] ARM64/PCI: Start using quirks handling
>> for ACPI based PCI host controller
>>
>> On Wed, Jun 15, 2016 at 11:34:11AM -0400, Christopher Covington wrote:
>>> From: Tomasz Nowicki <tn@xxxxxxxxxxxx>
>>>
>>> pci_generic_ecam_ops is used by default. Since there are platforms
>>> which have non-compliant ECAM space we need to overwrite these
>>> accessors prior to PCI buses enumeration. In order to do that
>>> we call pci_mcfg_get_ops to retrieve pci_ecam_ops structure so that
>>> we can use proper PCI config space accessors and bus_shift.
>>>
>>> pci_generic_ecam_ops is still used for platforms free from quirks.
>>>
>>> Signed-off-by: Tomasz Nowicki <tn@xxxxxxxxxxxx>
>>> ---
>>>  arch/arm64/kernel/pci.c | 7 ++++---
>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
>>> index 94cd43c..a891bda 100644
>>> --- a/arch/arm64/kernel/pci.c
>>> +++ b/arch/arm64/kernel/pci.c
>>> @@ -139,6 +139,7 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root
>> *root)
>>>  	struct pci_config_window *cfg;
>>>  	struct resource cfgres;
>>>  	unsigned int bsz;
>>> +	struct pci_ecam_ops *ops;
>>>
>>>  	/* Use address from _CBA if present, otherwise lookup MCFG */
>>>  	if (!root->mcfg_addr)
>>> @@ -150,12 +151,12 @@ pci_acpi_setup_ecam_mapping(struct
>> acpi_pci_root *root)
>>>  		return NULL;
>>>  	}
>>>
>>> -	bsz = 1 << pci_generic_ecam_ops.bus_shift;
>>> +	ops = pci_mcfg_get_ops(root);
>>> +	bsz = 1 << ops->bus_shift;
>>>  	cfgres.start = root->mcfg_addr + bus_res->start * bsz;
>>>  	cfgres.end = cfgres.start + resource_size(bus_res) * bsz - 1;
>>>  	cfgres.flags = IORESOURCE_MEM;
>>> -	cfg = pci_ecam_create(&root->device->dev, &cfgres, bus_res,
>>> -			      &pci_generic_ecam_ops);
>>> +	cfg = pci_ecam_create(&root->device->dev, &cfgres, bus_res, ops);
>>
>> Arnd pointed this out already, I think that's the only pending question
>> here.
>>
>> pci_ecam_create() maps ECAM space for config regions retrieved from
>> the MCFG, which are *supposed* to be ECAM compliant.
>>
>> Do we think that's *always* correct/safe regardless of the kind
>> of quirk we are currently fixing up ?

> From my perspective I think the easiest solution is to keep this
> quirk mechanism in place and then review vendor by vendor solution
> as they are pushed to the mailing list; if some vendors are abusing
> of some addresses/resources then they can be rejected...

Using pci_ecam_create() works well for QDF2432:

https://us.codeaurora.org/cgit/quic/server/kernel/commit/?h=cov/4.7-rc3-testing&id=ca7439a54ddd8612b435c797ffc54f4e19f03e2b

Cheers,
Cov

-- 
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project