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

[Sinan Kaya <okaya@codeaurora.org>]

On 11/6/2015 10:44 AM, Jiang Liu wrote:
> On 2015/11/6 23:32, Jiang Liu wrote:
>> 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.
> One note for the TypeTranslation case, the arch code needs to reset
> resource_win->offset to zero after setting up the MMIO map. Sample
> code as below:
> va = ioremap(resource_win->offset + res->start, resource_size(res));
> resource_win->offset = 0;
> 
> Otherwise it will break pcibios_resource_to_bus() etc.
> 
>>
>> 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
>>>
>> --
>> 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/
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Tomasz, Lorenzo;
I have an ARMv8 platform with IO address support where I can test this
implementation. Let me know if there is any patch that I can try.
Sinan


-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
Linux Foundation Collaborative Project