Re: [Patch v7 4/7] PCI/ACPI: Add interface acpi_pci_root_create()

[Jiang Liu <jiang.liu@linux.intel.com>]

On 2015/11/6 22:45, Lorenzo Pieralisi wrote:
> On Fri, Nov 06, 2015 at 09:22:46PM +0800, Jiang Liu wrote:
>> On 2015/11/6 20:40, Tomasz Nowicki wrote:
>>> On 06.11.2015 12:46, Jiang Liu wrote:
>>>> On 2015/11/6 18:37, Tomasz Nowicki wrote:
>>>>> On 06.11.2015 09:52, Jiang Liu wrote:
>>>>> Sure, ARM64 (0-16M IO space) QEMU example:
>>>>> DWordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange,
>>>>>           0x00000000,         // Granularity
>>>>>           0x00000000,         // Range Minimum
>>>>>           0x0000FFFF,         // Range Maximum
>>>>>           0x3EFF0000,         // Translation Offset
>>>>>           0x00010000,         // Length
>>>>>           ,, , TypeStatic)
>>>> The above DWordIO resource descriptor doesn't confirm to the ACPI spec.
>>>> According to my understanding, ARM/ARM64 has no concept of IO port
>>>> address space, so the PCI host bridge will map IO port on PCI side
>>>> onto MMIO on host side. In other words, PCI host bridge on ARM64
>>>> implement a IO Port->MMIO translation instead of a IO Port->IO Port
>>>> translation. If that's true, it should use 'TypeTranslation' instead
>>>> of 'TypeStatic'. And kernel ACPI resource parsing interface doesn't
>>>> support 'TypeTranslation' yet, so we need to find a solution for it.
>>>
>>> I think you are right, we need TypeTranslation flag for ARM64 DWordIO
>>> descriptors and an extra kernel patch to support it.
>> How about the attached to patch to support TypeTranslation?
>> It only passes compilation:)
> 
> Eh, hopefully there are not any ACPI tables out there with that bit
> set that work _today_ and would not work with the patch attached :)
> 
> My question is still there: do we want to handle the same problem
> as ia64 has in a different manner ? Certainly we won't be able
> to update ia64 platforms ACPI tables, so we would end up with
> two platforms handling IO resources in different ways unless I am
> missing something here.
There are some difference between IA64 and ARM64.
On IA64, it supports 16M IO address space per PCI domain and 256 PCI
domains at max. So the system IO address space is 16M * 256 = 4G.
So it does two level translations to support IO port
1) translate PCI bus local IO port address into system global IO port
   address by adding acpi_des->translation_offset.
2) translate the 4G system IO port address space into MMIO address.
   IA64 has reserved a 4G space for IO port mapping. This translation
   is done by arch specific method.
In other word, IA64 needs two level translation, but ACPI only provides
on (trans_type, trans_offset) pair for encoding, so it's used for step 1).

For ARM64, I think currently it only needs step 2).

> 
> BTW, why would we add offset to res->start only if TypeTranslation is
> clear ? Is not that something we would do just to make things "work" ?
> That flag has no bearing on the offset, only on the resource type AFAIK.
It's not a hack, but a way to interpret ACPI spec:)

With current linux resource management framework, we need to allocate
both MMIO and IO port address space range for an ACPI resource of type
'TypeTranslation'. And struct resource could be either IO port or MMIO,
not both. So the choice is to keep the resource as IO port, and let
arch code to build the special MMIO mapping for it. Otherwise it will
break too many things if we convert the resource as MMIO.

That said, we need to add translation_offset to convert bus local
IO port address into system global IO port address if it's type of
TypeStatic, because ioresource_ioport uses system global IO port
address.

For an ACPI resource of type TypeTranslation, system global IO port
address equals bus local IO port address, and the translation_offset
is used to translate IO port address into MMIO address, so we shouldn't
add translation_offset to the IO port resource descriptor.

Thanks,
Gerry

> 
> This without taking into account ARM64 systems shipping with ACPI
> tables that does not set the TypeTranslation at present.
> 
> On top of that, I noticed that core ACPI code handles Sparse
> Translation (ie _TRS), that should be considered meaningful only if _TTP
> is set (and that's not checked).
Yes, that's a flaw:(

> 
> Thoughts ?
> 
> Thanks,
> Lorenzo
>