Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1232906 > unrolled thread

Re: [PATCH 05/11] x86, pci, acpi: Move arch-agnostic MMCONFIG (aka ECAM) and ACPI code out of arch/x86/ directory

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 05/11] x86, pci, acpi: Move arch-agnostic MMCONFIG (aka  ECAM) and ACPI code out of arch/x86/ directory Tomasz Nowicki <tomasz.nowicki@linaro.org> - 2015-09-25 18:10 +0200
    Re: [PATCH 05/11] x86, pci, acpi: Move arch-agnostic MMCONFIG (aka  ECAM) and ACPI code out of arch/x86/ directory Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2015-09-25 18:20 +0200

#1232906 — Re: [PATCH 05/11] x86, pci, acpi: Move arch-agnostic MMCONFIG (aka ECAM) and ACPI code out of arch/x86/ directory

Hi Lorenzo,

On 09/14/2015 04:55 PM, Tomasz Nowicki wrote:
> On 11.09.2015 13:20, Lorenzo Pieralisi wrote:
>> On Wed, Sep 09, 2015 at 02:47:55PM +0100, Tomasz Nowicki wrote:
>>
>> [...]
>>
>>>> I think (but I am happy to be corrected) that the map_bus() hook
>>>> (ie that's why struct pci_bus is required in eg
>>>> pci_generic_config_write)
>>>> is there to ensure that when the generic accessors are called
>>>> a) we have a valid bus b) the host controllers implementing it
>>>> has been initialized.
>>>>
>>>> I had another look and I noticed you are trying to solve multiple
>>>> things at once:
>>>>
>>>> 1) ACPICA seems to need PCI config space on bus 0 to be working
>>>>      before PCI enumerates (ie before we have a root bus), we need to
>>>>      countercheck on that, but you can't use the generic PCI accessors
>>>>      for that reasons (ie root bus might not be available, you do not
>>>>      have a pci_bus struct)
>>>> 2) the raw_pci_read/write require _generic_ mmio back-ends, since AMD
>>>>      can't cope with standard x86 read/write{b,w,l}
>>>>
>>>> Overall, it seems to me that we can avoid code duplication by
>>>> shuffling your code a bit.
>>>>
>>>> You could modify the generic accessors in drivers/pci/access.c to
>>>> use your mmio back-end instead of using plain read/write{b,w,l}
>>>> functions (we should check if RobH is ok with that there can be
>>>> reasons that prevent this from happening). This would solve the
>>>> AMD mmio issue.
>>>>
>>>> By factoring out the code that actually carries out the reads
>>>> and writes in the accessors basically you decouple the functions
>>>> requiring the struct pci_bus from the ones that does not require it
>>>> (ie raw_pci_{read/write}.
>>>>
>>>> The generic MMIO layer belongs in the drivers/pci/access.c file, it has
>>>> nothing to do with ECAM.
>>>>
>>>> The mmcfg interface should probably live in pci-acpi.c, I do not think
>>>> you need an extra file in there but that's a detail.
>>>>
>>>> Basically the generic accessors would become something like eg:
>>>>
>>>> int pci_generic_config_write(struct pci_bus *bus, unsigned int devfn,
>>>>                             int where, int size, u32 val)
>>>> {
>>>>        void __iomem *addr;
>>>>
>>>>        addr = bus->ops->map_bus(bus, devfn, where);
>>>>        if (!addr)
>>>>                return PCIBIOS_DEVICE_NOT_FOUND;
>>>>
>>>>        pci_mmio_write(size, addr + where, value);
>>>>
>>>>        return PCIBIOS_SUCCESSFUL;
>>>> }
>>>>
>>>> With that in place using raw_pci_write/read or the generic accessors
>>>> becomes almost identical, with code requiring the pci_bus to be
>>>> created using the generic accessors and ACPICA using the raw version.
>>>>
>>>> I might be missing something, so apologies if that's the case.
>>>>
>>>
>>> Actually, I think you showed me the right direction :) Here are some
>>> conclusions/comments/concerns. Please correct me if I am wrong:
>>>
>>> 1. We need raw_pci_write/read accessors (based on ECAM) for ARM64 too
>>> but only up to the point where buses are enumerated. From that point on,
>>> we should reuse generic accessors from access.c file, right?
>>
>> Well, I still have not figured out whether on arm64 the raw accessors
>> required by ACPICA make sense.
>>
>> So either arm64 relies on the generic MCFG based raw read and writes
>> or we define the global raw read and writes as empty (ie x86 overrides
>> them anyway).
>>
>
> My concerns/ideas related to raw accessors for ARM64, please correct me
> at any point.
>
> ACPI spec - chapter: 19.5.96 OperationRegion (Declare Operation Region)
> defines PCI_Config as one of region types. Every time ASL opcode
> operates on corresponding PCI config space region, ASL interpreter is
> dispatching address space to our raw accessors, please see
> acpi_ex_pci_config_space_handler, acpi_ev_pci_config_region_setup calls.
> What is more important, such operations may happen after (yes after) bus
> enumeration, but always raw accessors are called at the end with the
> {segment, bus, dev, fn} tuple.
>
> Giving above, here are some ideas:
> 1. We force somehow vendors to avoid operations on PCI config regions in
> ASL code. PCI config region definitions still fall into Hardware Reduced
> profile, so new ACPICA special subset for ARM64 is need. Then raw ACPI
> accessors can be empty (and overridden by x86).
> 2. We provide raw accessors which translate {segment, bus, dev, fn}
> tuple to Linux generic accessors (this can be considered only if PCI
> config accesses happened after bus enumeration for HR profile, thus
> tuple to bus structure map is possible).
> 4. We rely on the generic MCFG based raw read and writes.

I will appreciate your opinion on above ideas.

Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1232925

On Fri, Sep 25, 2015 at 05:02:09PM +0100, Tomasz Nowicki wrote:

[...]

> > My concerns/ideas related to raw accessors for ARM64, please correct me
> > at any point.
> >
> > ACPI spec - chapter: 19.5.96 OperationRegion (Declare Operation Region)
> > defines PCI_Config as one of region types. Every time ASL opcode
> > operates on corresponding PCI config space region, ASL interpreter is
> > dispatching address space to our raw accessors, please see
> > acpi_ex_pci_config_space_handler, acpi_ev_pci_config_region_setup calls.
> > What is more important, such operations may happen after (yes after) bus
> > enumeration, but always raw accessors are called at the end with the
> > {segment, bus, dev, fn} tuple.
> >
> > Giving above, here are some ideas:
> > 1. We force somehow vendors to avoid operations on PCI config regions in
> > ASL code. PCI config region definitions still fall into Hardware Reduced
> > profile, so new ACPICA special subset for ARM64 is need. Then raw ACPI
> > accessors can be empty (and overridden by x86).
> > 2. We provide raw accessors which translate {segment, bus, dev, fn}
> > tuple to Linux generic accessors (this can be considered only if PCI
> > config accesses happened after bus enumeration for HR profile, thus
> > tuple to bus structure map is possible).
> > 4. We rely on the generic MCFG based raw read and writes.
> 
> I will appreciate your opinion on above ideas.

Well, (1) does not seem allowed by the ACPI specification, the only
way we can detect that is by leaving the raw accessors empty for
now and see how things will turn out on ARM64, in the meantime
I will start a thread on ASWG to check how that's used on x86, I
do not have any machine to test this and grokking ACPICA is not
trivial, there is lots of history there and it is hard to fathom.

(2) is tempting but I am not sure it works all the time (I still
think that's a quirk of ACPI specs, namely that some OperationRegion
should always be available to ASL, maybe that's just an unused ACPI
spec quirk).

If I read you series correctly (4) can be implemented easily if and
when we deem the raw accessors necessary, on top of the MCFG layer,
by making the MCFG raw accessors the default instead of leaving them
empty.

I pulled your branch and started testing it on AMD Seattle, next week
we should try to get this done.

I think you should target option (1) and in the meantime we should
reach a conclusion on the raw accessors usage on ARM64.

Thanks,
Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web