From mboxrd@z Thu Jan 1 00:00:00 1970 From: Liviu Dudau Subject: Re: [PATCH v8 3/9] pci: Introduce pci_register_io_range() helper function. Date: Tue, 8 Jul 2014 23:45:29 +0100 Message-ID: <20140708224529.GB4980@e106497-lin.cambridge.arm.com> References: <1404240214-9804-1-git-send-email-Liviu.Dudau@arm.com> <1404240214-9804-4-git-send-email-Liviu.Dudau@arm.com> <20140708001418.GB22939@google.com> <201407080900.44882.arnd@arndb.de> <20140708212951.GA4555@google.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: <20140708212951.GA4555@google.com> Content-Disposition: inline Sender: linux-pci-owner@vger.kernel.org To: Bjorn Helgaas Cc: Arnd Bergmann , linux-pci , Catalin Marinas , Will Deacon , Benjamin Herrenschmidt , linaro-kernel , Tanmay Inamdar , Grant Likely , Sinan Kaya , Jingoo Han , Kukjin Kim , Suravee Suthikulanit , LKML , Device Tree ML , LAKML List-Id: devicetree@vger.kernel.org On Tue, Jul 08, 2014 at 10:29:51PM +0100, Bjorn Helgaas wrote: > On Tue, Jul 8, 2014 at 1:00 AM, Arnd Bergmann wrote: > > On Tuesday 08 July 2014, Bjorn Helgaas wrote: > >> On Tue, Jul 01, 2014 at 07:43:28PM +0100, Liviu Dudau wrote: > >> > +static LIST_HEAD(io_range_list); > >> > + > >> > +/* > >> > + * Record the PCI IO range (expressed as CPU physical address += size). > >> > + * Return a negative value if an error has occured, zero otherw= ise > >> > + */ > >> > +int __weak pci_register_io_range(phys_addr_t addr, resource_siz= e_t size) > >> > >> I don't understand the interface here. What's the mapping from CP= U > >> physical address to bus I/O port? For example, I have the followi= ng > >> machine in mind: > >> > >> HWP0002:00: PCI Root Bridge (domain 0000 [bus 00-1b]) > >> HWP0002:00: memory-mapped IO port space [mem 0xf8010000000-0xf80= 10000fff] > >> HWP0002:00: host bridge window [io 0x0000-0x0fff] > >> > >> HWP0002:09: PCI Root Bridge (domain 0001 [bus 00-1b]) > >> HWP0002:09: memory-mapped IO port space [mem 0xf8110000000-0xf81= 10000fff] > >> HWP0002:09: host bridge window [io 0x1000000-0x1000fff] (PCI ad= dress [0x0-0xfff]) > >> > >> The CPU physical memory [mem 0xf8010000000-0xf8010000fff] is trans= lated by > >> the bridge to I/O ports 0x0000-0x0fff on PCI bus 0000:00. Drivers= use, > >> e.g., "inb(0)" to access it. > >> > >> Similarly, [mem 0xf8110000000-0xf8110000fff] is translated by the = second > >> bridge to I/O ports 0x0000-0x0fff on PCI bus 0001:00. Drivers use > >> "inb(0x1000000)" to access it. > > > > I guess you are thinking of the IA64 model here where you keep the = virtual > > I/O port numbers in a per-bus lookup table that gets accessed for e= ach > > inb() call. I've thought about this some more, and I believe there = are good > > reasons for sticking with the model used on arm32 and powerpc for t= he > > generic OF implementation. > > > > The idea is that there is a single virtual memory range for all I/O= port > > mappings and we use the MMU to do the translation rather than compu= ting > > it manually in the inb() implemnetation. The main advantage is that= all > > functions used in device drivers to (potentially) access I/O ports > > become trivial this way, which helps for code size and in some case= s > > (e.g. SoC-internal registers with a low latency) it may even be per= formance > > relevant. >=20 > My example is from ia64, but I'm not advocating for the lookup table. > The point is that the hardware works similarly (at least for dense ia= 64 > I/O port spaces) in terms of mapping CPU physical addresses to PCI I/= O > space. >=20 > I think my confusion is because your pci_register_io_range() and > pci_addess_to_pci() implementations assume that every io_range starts= at > I/O port 0 on PCI (correct me if I'm wrong). I suspect that's why yo= u > don't save the I/O port number in struct io_range. >=20 > Maybe that assumption is guaranteed by OF, but it doesn't hold for AC= PI; > ACPI can describe several I/O port apertures for a single bridge, eac= h > associated with a different CPU physical memory region. That is actually a good catch, I've completely missed the fact that io_range->pci_addr could be non-zero. >=20 > If my speculation here is correct, a comment to the effect that each > io_range corresponds to a PCI I/O space range that starts at 0 might = be > enough. >=20 > If you did add a PCI I/O port number argument to pci_register_io_rang= e(), > we might be able to make an ACPI-based implementation of it. But I g= uess > that could be done if/when anybody ever wants to do that. No, I think you are right, the PCI I/O port number needs to be recorded= =2E I need to add that to pci_register_io_range(). >=20 > >> Here's what these look like in /proc/iomem and /proc/ioports (note= that > >> there are two resource structs for each memory-mapped IO port spac= e: one > >> IORESOURCE_MEM for the memory-mapped area (used only by the host b= ridge > >> driver), and one IORESOURCE_IO for the I/O port space (this become= s the > >> parent of a region used by a regular device driver): > >> > >> /proc/iomem: > >> PCI Bus 0000:00 I/O Ports 00000000-00000fff > >> PCI Bus 0001:00 I/O Ports 01000000-01000fff >=20 > Oops, I forgot the actual physical memory addresses here, but you got > the idea anyway. It should have been something like this: >=20 > /proc/iomem: > f8010000000-f8010000fff PCI Bus 0000:00 I/O Ports 00000000-00000f= ff > f8110000000-f8110000fff PCI Bus 0001:00 I/O Ports 01000000-01000f= ff >=20 > Bjorn >=20 Thanks for being thorough with your review. Best regards, Liviu --=20 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- =C2=AF\_(=E3=83=84)_/=C2=AF