diff mbox

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

Message ID 563CA9A6.7050309@linux.intel.com
State Not Applicable
Headers show

Commit Message

Jiang Liu Nov. 6, 2015, 1:22 p.m. UTC
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:)

> 
> Thanks,
> Tomasz

Comments

Lorenzo Pieralisi Nov. 6, 2015, 2:45 p.m. UTC | #1
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.

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.

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).

Thoughts ?

Thanks,
Lorenzo
--
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
Jiang Liu Nov. 6, 2015, 3:32 p.m. UTC | #2
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
> 
--
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
Jiang Liu Nov. 6, 2015, 3:44 p.m. UTC | #3
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 Nowicki Nov. 9, 2015, 2:07 p.m. UTC | #4
On 06.11.2015 14:22, 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:)

Based on the further discussion, your draft patch looks good to me. 
Lorenzo, do you agree?

Gerry, what would be the best way to approach with this, extra patch of 
your set? or keep it separately, might be part of my set.

Thanks,
Tomasz
--
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
Lorenzo Pieralisi Nov. 9, 2015, 5:10 p.m. UTC | #5
[CC'ing Arnd]

On Mon, Nov 09, 2015 at 03:07:38PM +0100, Tomasz Nowicki wrote:
> On 06.11.2015 14:22, 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:)
> 
> Based on the further discussion, your draft patch looks good to me.
> Lorenzo, do you agree?

No, because I still do not understand the difference between ia64 and
arm64 (they both drive IO ports cycles through MMIO so the resource
descriptors content must be the same or better they must mean the same
thing). On top of that, this is something that was heavily debated for DT:

http://www.spinics.net/lists/arm-kernel/msg345633.html

and I would like to get Arnd and Bjorn opinion on this because we
should not "interpret" ACPI specifications, we should understand
what they are supposed to describe and write kernel code accordingly.

In particular, I would like to understand, for an eg DWordIO descriptor,
what Range Minimum, Range Maximum and Translation Offset represent,
they can't mean different things depending on the SW parsing them,
this totally defeats the purpose.

By the way, ia64 ioremaps the translation_offset (ie new_space()), so
basically that's the CPU physical address at which the PCI host bridge
map the IO space transactions), I do not think ia64 is any different from
arm64 in this respect, if it is please provide an HW description here from
the PCI bus perspective here (also an example of ia64 ACPI PCI host bridge
tables would help).

Thanks,
Lorenzo
--
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
Arnd Bergmann Nov. 9, 2015, 8:09 p.m. UTC | #6
On Monday 09 November 2015 17:10:43 Lorenzo Pieralisi wrote:
> On Mon, Nov 09, 2015 at 03:07:38PM +0100, Tomasz Nowicki wrote:
> > On 06.11.2015 14:22, 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:)
> > 
> > Based on the further discussion, your draft patch looks good to me.
> > Lorenzo, do you agree?
> 
> No, because I still do not understand the difference between ia64 and
> arm64 (they both drive IO ports cycles through MMIO so the resource
> descriptors content must be the same or better they must mean the same
> thing). On top of that, this is something that was heavily debated for DT:
> 
> http://www.spinics.net/lists/arm-kernel/msg345633.html
> 
> and I would like to get Arnd and Bjorn opinion on this because we
> should not "interpret" ACPI specifications, we should understand
> what they are supposed to describe and write kernel code accordingly.
> 
> In particular, I would like to understand, for an eg DWordIO descriptor,
> what Range Minimum, Range Maximum and Translation Offset represent,
> they can't mean different things depending on the SW parsing them,
> this totally defeats the purpose.

I have no clue about what those mean in ACPI though.

Generally speaking, each PCI domain is expected to have a (normally 64KB)
range of CPU addresses that gets translated into PCI I/O space the same
way that config space and memory space are handled.
This is true for almost every architecture except for x86, which uses
different CPU instructions for I/O space compared to the other spaces.

> By the way, ia64 ioremaps the translation_offset (ie new_space()), so
> basically that's the CPU physical address at which the PCI host bridge
> map the IO space transactions), I do not think ia64 is any different from
> arm64 in this respect, if it is please provide an HW description here from
> the PCI bus perspective here (also an example of ia64 ACPI PCI host bridge
> tables would help).

The main difference between ia64 and a lot of the other architectures (e.g.
sparc is different again) is that ia64 defines a logical address range
in terms of having a small number for each I/O space followed by the
offset within that space as a 'port number' and uses a mapping function
that is defined as

static inline void *__ia64_mk_io_addr (unsigned long port)
{
        struct io_space *space = &io_space[IO_SPACE_NR(port)];
        return (space->mmio_base | IO_SPACE_PORT(port););
}
static inline unsigned int inl(unsigned long port)
{
        return *__ia64_mk_io_addr(port);
}

Most architectures allow only one I/O port range and put it at a fixed
virtual address so that inl() simply becomes 

static inline u32 inl(unsigned long addr)
{
        return readl(PCI_IOBASE + addr);
}

which noticeably reduces code size.

On some architectures (powerpc, arm, arm64), we then get the same simplified
definition with a fixed virtual address, and use pci_ioremap_io() or
something like that to to map a physical address range into this virtual
address window at the correct io_offset;

	Arnd
--
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
Sinan Kaya Nov. 23, 2015, 3:23 p.m. UTC | #7
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
diff mbox

Patch

From 51f5cddd8c4301b731805074ebc3e3a6c7dbaf59 Mon Sep 17 00:00:00 2001
From: Liu Jiang <jiang.liu@linux.intel.com>
Date: Fri, 6 Nov 2015 20:01:59 +0800
Subject: [PATCH]


Signed-off-by: Liu Jiang <jiang.liu@linux.intel.com>
---
 drivers/acpi/resource.c      |   25 +++++++++++++++++++++++--
 include/linux/resource_ext.h |    7 +++++++
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
index cdc5c2599beb..1bd3e21f56fe 100644
--- a/drivers/acpi/resource.c
+++ b/drivers/acpi/resource.c
@@ -215,8 +215,29 @@  static bool acpi_decode_space(struct resource_win *win,
 	else if (attr->translation_offset)
 		pr_debug("ACPI: translation_offset(%lld) is invalid for non-bridge device.\n",
 			 attr->translation_offset);
-	start = attr->minimum + offset;
-	end = attr->maximum + offset;
+	start = attr->minimum;
+	end = attr->maximum;
+
+	/*
+	 * Convert bus local address into system global address if it's an
+	 * IO Port->IO Port or MMIO->MMIO translation.
+	 */
+	switch (addr->resource_type) {
+	case ACPI_MEMORY_RANGE:
+		if (addr->info.mem.translation)
+			win->translation_type = RESOURCE_TRANS_MMIO_TO_IOPORT;
+		else
+			start += offset;
+		break;
+	case ACPI_IO_RANGE:
+		if (addr->info.io.translation)
+			win->translation_type = RESOURCE_TRANS_IOPORT_TO_MMIO;
+		else
+			start += offset;
+		break;
+	default:
+		break;
+	}
 
 	win->offset = offset;
 	res->start = start;
diff --git a/include/linux/resource_ext.h b/include/linux/resource_ext.h
index e2bf63d881d4..f06d358c1f22 100644
--- a/include/linux/resource_ext.h
+++ b/include/linux/resource_ext.h
@@ -22,8 +22,15 @@ 
 struct resource_win {
 	struct resource res;		/* In master (CPU) address space */
 	resource_size_t offset;		/* Translation offset for bridge */
+	int translation_type;		/* Translation type for bridge */
 };
 
+#define RESOURCE_TRANS_SAME		0x0
+/* Translate from IO port on slave into MMIO on master */
+#define RESOURCE_TRANS_IOPORT_TO_MMIO	0x1
+/* Translate from MMIO on slave into IO port on master */
+#define RESOURCE_TRANS_MMIO_TO_IOPORT	0x2
+
 /*
  * Common resource list management data structure and interfaces to support
  * ACPI, PNP and PCI host bridge etc.
-- 
1.7.10.4