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

[Arnd Bergmann <arnd@arndb.de>]

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