[PATCH V6 09/13] pci, acpi: Support for ACPI based generic PCI host controller

[Tomasz Nowicki]

On 04/29/2016 07:35 PM, Jayachandran C wrote:
> On Fri, Apr 29, 2016 at 2:07 PM, Lorenzo Pieralisi
> <lorenzo.pieralisi at arm.com> wrote:
>> On Thu, Apr 28, 2016 at 04:48:00PM -0500, Bjorn Helgaas wrote:
>>
>> [...]
>>
>>>> +static int pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root,
>>>> +                                  struct acpi_pci_generic_root_info *ri)
>>>> +{
>>>> +   u16 seg = root->segment;
>>>> +   u8 bus_start = root->secondary.start;
>>>> +   u8 bus_end = root->secondary.end;
>>>> +   struct pci_config_window *cfg;
>>>> +   struct mcfg_entry *e;
>>>> +   phys_addr_t addr;
>>>> +   int err = 0;
>>>> +
>>>> +   mutex_lock(&pci_mcfg_lock);
>>> What does this lock protect?  The pci_mcfg_list should already be
>>> initialized by the time we get there, and it should be immutable for
>>> the life of the system.  In fact, I would prefer if we could just
>>> search the static table itself whenever we need it rather than caching
>>> it in our own list.  But I don't think we can easily do that because
>>> acpi_table_parse() is __init.
>>>
>>>> +   e = pci_mcfg_lookup(seg, bus_start);
>>> I would argue that we should check for _CBA first, and fall back to
>>> MCFG if _CBA doesn't exist.
>>>
>>>> +   if (!e) {
>>>> +           addr = acpi_pci_root_get_mcfg_addr(root->device->handle);
>>> IMO, acpi_pci_root_get_mcfg_addr() is misnamed.  It should be
>>> acpi_pci_config_base_addr() or similar.  It definitely is not related
>>> to MCFG.  Not your fault, obviously.
>>>
>>>> +           if (addr == 0) {
>>>> +                   pr_err(PREFIX"%04x:%02x-%02x bus range error\n",
>>>> +                          seg, bus_start, bus_end);
>>>> +                   err = -ENOENT;
>>>> +                   goto err_out;
>>>> +           }
>>>> +   } else {
>>>> +           if (bus_start != e->bus_start) {
>>>> +                   pr_err("%04x:%02x-%02x bus range mismatch %02x\n",
>>>> +                          seg, bus_start, bus_end, e->bus_start);
>>>> +                   err = -EINVAL;
>>>> +                   goto err_out;
>>>> +           } else if (bus_end != e->bus_end) {
>>>> +                   pr_warn("%04x:%02x-%02x bus end mismatch %02x\n",
>>>> +                           seg, bus_start, bus_end, e->bus_end);
>>>> +                   bus_end = min(bus_end, e->bus_end);
>>>> +           }
>>>> +           addr = e->addr;
>>>> +   }
>>> I really don't think you need a lock around this, so you can factor
>>> out the address lookup into something like:
>>>
>>>    addr = acpi_pci_config_base_addr(...);
>>>    if (addr)
>>>      return addr;
>>>
>>>    return acpi_pci_mcfg_lookup(seg, busn_res);
>>>
>>> You can check inside acpi_pci_mcfg_lookup() to make sure the entry you
>>> find covers the entire [busn_res.start-busn_res.end] range and return
>>> failure if it doesn't.  At this point, I'm not sure it's worth it to
>>> truncate the host bridge bus range to match something we find in MCFG.
>>>
>>> If the MCFG entry covers *more* than the host bridge range from _CRS,
>>> that's fine.  In any case, we have to be careful with the start address,
>>> because the MCFG start address is always based on bus 0, but I think
>>> pci_generic_ecam_create() expects the start address based on the
>>> bus_start you pass to it.
>> Yes, I spotted this too, it is unfortunate but DT and MCFG handle
>> the ECAM regions differently. In DT the reg property is relative
>> to bus_start - ie reg MMIO region maps config space starting at
>> the first bus in bus-range:
>>
>> Documentation/devicetree/bindings/pci/host-generic-pci.txt
>>
>> in ACPI(MCFG) as you said it is always relative to bus 0, it is
>> unfortunate but the address to be mapped should be computed
>> differently in the ECAM layer.
> Can't this be handled by fixing up the address before passing to
> pci_generic_ecam_create?
>
I agree, this should work, IMO.

Tomasz