RE: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06

[Gabriele Paoloni]

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@xxxxxxxx]
> Sent: 18 November 2016 16:35
> To: Gabriele Paoloni
> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; mark.rutland@xxxxxxx;
> benh@xxxxxxxxxxxxxxxxxxx; catalin.marinas@xxxxxxx; liviu.dudau@xxxxxxx;
> Linuxarm; lorenzo.pieralisi@xxxxxxx; xuwei (O); Jason Gunthorpe; linux-
> serial@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; minyard@xxxxxxx; will.deacon@xxxxxxx; John
> Garry; zourongrong@xxxxxxxxx; robh+dt@xxxxxxxxxx; bhelgaas@go og
> le.com; kantyzc@xxxxxxx; zhichang.yuan02@xxxxxxxxx; T homas Petazzoni;
> linux-kernel@xxxxxxxxxxxxxxx; Yuanzhichang; olof@xxxxxxxxx
> Subject: Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on
> Hip06
> 
> On Friday, November 18, 2016 4:18:07 PM CET Gabriele Paoloni wrote:
> > From: Arnd Bergmann [mailto:arnd@xxxxxxxx]
> > > On Friday, November 18, 2016 12:53:08 PM CET Gabriele Paoloni
> wrote:
> > > > From: Arnd Bergmann [mailto:arnd@xxxxxxxx]
> > > > > On Friday, November 18, 2016 12:07:28 PM CET Gabriele Paoloni
> > > > > The easiest change compared to the v5 code would be to walk
> > > > > a linked list of 'struct extio_ops' structures rather than
> > > > > assuming there is only ever one of them. I think one of the
> > > > > earlier versions actually did this.
> > > >
> > > > Right but if my understanding is correct if we live in a multi-
> > > > domain I/O space world when you have an input addr in the I/O
> > > > accessors this addr can be duplicated (for example for the
> standard
> > > > PCI IO domain and for our special LPC domain).
> > > > So effectively even if you walk a linked list there is a problem
> > > > of disambiguation...am I right?
> > >
> > > No, unlike the PCI memory space, the PIO addresses are not
> > > usually distinct, i.e. every PCI bus has its own 64K I/O
> > > addresses starting at zero. We linearize them into the
> > > Linux I/O space using the per-domain io_offset value.
> >
> > I am going to summarize my understanding here below:
> >
> > It seems to me that what is linearized is the virtual address
> > space associated to the IO address space. This address space
> > goes from PCI_IOBASE to (PCI_IOBASE + IO_SPACE_LIMIT).
> >
> > The I/O accessors perform rd/wr operation on this address
> > space using a port IO token.
> >
> > Each token map into a cpu physical address range
> > Each cpu physical address range maps to a bus address range
> > if the bus controller specifies a range property.
> >
> > Devices under a bus controller specify the bus addresses that
> > they operate on in their reg property.
> >
> > So each device can use the same bus addresses under two
> > separate bus controllers as long as the bus controller
> > use the range properties to map the same bus range to different
> > cpu address range.
> 
> Sounds all correct to me so far, yes.
> 
> > > For the ISA/LPC spaces there are only 4k of addresses, they
> > > the bus addresses always overlap, but we can trivially
> > > figure out the bus address from Linux I/O port number
> > > by subtracting the start of the range.
> >
> > Are you saying that our LPC controller should specify a
> > range property to map bus addresses into a cpu address range?
> 
> No. There is not CPU address associated with it, because it's
> not memory mapped.
> 
> Instead, we need to associate a bus address with a logical
> Linux port number, both in of_address_to_resource and
> in inb()/outb().

I think this is effectively what we are doing so far with patch 2/3.
The problem with this patch is that we are carving out a "forbidden"
IO tokens range that goes from 0 to PCIBIOS_MIN_IO.

I think that the proper solution would be to have the LPC driver to
set the carveout threshold used in pci_register_io_range(), 
pci_pio_to_address(), pci_address_to_pio(), but this would impose
a probe dependency on the LPC itself that should be probed before
the PCI controller (or before any other devices calling these
functions...) 

> 
> > > > > Another option the IA64 approach mentioned in another subthread
> > > > > today, looking up the operations based on an index from the
> > > > > upper bits of the port number. If we do this, we probably
> > > > > want to do that for all PIO access and replace the entire
> > > > > virtual address remapping logic with that. I think Bjorn
> > > > > in the past argued in favor of such an approach, while I
> > > > > advocated the current scheme for simplicity based on how
> > > > > every I/O space these days is just memory mapped (which now
> > > > > turned out to be false, both on powerpc and arm64).
> > > >
> > > > This seems really complex...I am a bit worried that possibly
> > > > we end up in having the maintainers saying that it is not worth
> > > > to re-invent the world just for this special LPC device...
> > >
> > > It would clearly be a rather invasive change, but the
> > > end-result isn't necessarily more complex than what we
> > > have today, as we'd kill off the crazy pci_pio_to_address()
> > > and pci_address_to_pio() hacks in address translation.
> >
> > I have to look better into this...can you provide me a reference
> > to the Bjorn argument in favor of this approach?
> 
> The thread seems to have been pci: Introduce pci_register_io_range()
> helper function, e.g. in https://lkml.org/lkml/2014/7/8/969

Ok many thanks I am going to look at it

> 
> > > > To be honest with you I would keep things simple for this
> > > > LPC and introduce more complex reworks later if more devices
> > > > need to be introduced.
> > > >
> > > > What if we stick on a single domain now where we introduce a
> > > > reserved threshold for the IO space (say INDIRECT_MAX_IO).
> > >
> > > I said having a single domain is fine, but I still don't
> > > like the idea of reserving low port numbers for this hack,
> > > it would mean that the numbers change for everyone else.
> >
> > I don't get this much...I/O tokens that are passed to the I/O
> > accessors are not fixed anyway and they vary depending on the order
> > of adding ranges to io_range_list...so I don't see a big issue
> > with this...
> 
> On machines with a legacy devices behind the PCI bridge,
> there may still be a reason to have the low I/O port range
> reserved for the primary bus, e.g. to get a VGA text console
> to work.
> 
> On powerpc, this is called the "primary" PCI host, i.e. the
> only one that is allowed to have an ISA bridge.

Yes but
1) isn't the PCI controller range property that defines how IO bus address
   map into physical CPU addresses?
2) How can you guarantee that the cpu range associated with this
   IO bus range is the first to be registered in pci_register_io_range()?
   ( i.e. are you saying that they are just relying on the fact that it is the
     only IO range in the system and by chance the IO tokens and corresponding
     bus addresses are the same? )

Thanks

Gab

> 
> 	Arnd