From mboxrd@z Thu Jan 1 00:00:00 1970 From: Liviu Dudau Subject: Re: [PATCH] pci: Add support for creating a generic host_bridge from device tree Date: Tue, 4 Feb 2014 12:08:02 +0000 Message-ID: <20140204120801.GB27975@e106497-lin.cambridge.arm.com> References: <1391452428-22917-1-git-send-email-Liviu.Dudau@arm.com> <3523674.i9iUPinOAk@wuerfel> <20140203221743.GB24036@e106497-lin.cambridge.arm.com> <4524393.8AzpATULMB@wuerfel> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: <4524393.8AzpATULMB@wuerfel> Content-Disposition: inline Sender: linux-pci-owner@vger.kernel.org To: Arnd Bergmann Cc: linux-pci , Bjorn Helgaas , Catalin Marinas , Will Deacon , LKML , "devicetree@vger.kernel.org" , LAKML , linaro-kernel List-Id: devicetree@vger.kernel.org On Tue, Feb 04, 2014 at 10:09:44AM +0000, Arnd Bergmann wrote: > On Monday 03 February 2014 22:17:44 Liviu Dudau wrote: > > On Mon, Feb 03, 2014 at 07:31:31PM +0000, Arnd Bergmann wrote: > > > Let's try to come up with nomenclature so we can talk about this = better > > > > > > The ioport_resource is in "logical I/O space", which is a Linux f= iction, > > > it goes from 0 to IO_SPACE_LIMIT (2MB on ARM) and is mapped into = "virtual > > > I/O space", which start at (void __iomem *)PCI_IO_VIRT_BASE. > > > > > > Each PCI domain can have its own "bus I/O aperture", which is typ= ically > > > between 0x1000 and 0xffff and reflects the address that is used i= n PCI > > > transactions and in BARs. > >=20 > > Actually, the bus I/O aperture can start from 0x0000 if you are tal= king about > > PCI bus addresses. >=20 > Right. >=20 > > > The aperture here reflects the subset of the > > > 4GB bus I/O space that is actually mapped into a CPU visible "phy= sical > > > I/O aperture" using an inbound mapping of the host bridge. The ph= ysical > > > I/O aperture in turn gets mapped to the virtual I/O space using > > > pci_ioremap_io. > >=20 > > Agree. > >=20 > > > The difference between a bus I/O address and a logical > > > I/O address is stored in the io_offset. > >=20 > > Not exactly. If that would be true that means that for an I/O range= that > > start at bus I/O address zero but physical I/O apperture starts at > > 0x40000000 the io_offset is zero. For me, the io_offset should be 0= x40000000. >=20 > That's not how we do it on any of the existing host controllers. > Typically the io_offset is zero for the first one, and may be > either zero for all the others (meaning BARs get > 64KB values > for secondary buses) or between 64KB and 2MB (meaning each bus > starts at I/O port number 0). In that case it is probably worth to rename my variable into phys_io_of= fset. I need to go back over my driver code. My assumptions were probably wro= ng wrt to meaning of the io_offset. >=20 > > Let me see if I can summarise this correctly, using only CPU addres= ses: > >=20 > > 0x0000 - IO_SPACE_LIMIT <- logical I/O address > > 0xPPPPPPPP - 0xPPPPPPPP+IO_SIZE <- physical address for PCI I/O = space > > 0xVVVVVVVV - 0xVVVVVVVV+IO_SPACE_LIMIT <- virtual address for I/O > >=20 > > The io_offset then is 0xPPPPPPPP - logical I/O address. At least th= at is > > the intent of the io_offset variable that I introduced in pci_host_= bridge. >=20 > That is highly confusing then, because we already have something call= ed > io_offset with a different meaning. I would call 0xPPPPPPPP the io_ph= ys_base > if I had to come up with a variable name for it. >=20 > > The bus I/O address is generated by the host bridge, I think we can= ignore > > it here as it tends to confuse the message. >=20 > No, it's important because the PCI core code has to transform between > bus I/O address and logical I/O address when accessing the BARs. >=20 > > > So much for basic definitions. When a device driver calls pci_req= uest_region, > > > the port number it sees is the bus I/O port number adjusted using= the > > > io_offset to turn it into a logical I/O port number, which should > > > always be within the host bridge window, which in turn is a subse= t > > > of the ioport_resource. > >=20 > > My understanding is that device drivers all user port numbers that = are logical > > I/O numbers, so no io_offset needs to be applied here. It is only w= hen one > > wants to access the port, that the translation happens. First, inb = or outb > > will add the PCI_IO_VIRT_BASE to generate the virtual address, the = MMU will > > then convert that address to physical address and the host bridge w= ill > > then translate the physical address into bus address. >=20 > This is correct. The bus I/O number is not visible to the device driv= er, > but it is what you put into the 'ranges' property in DT, and it gets > used during PCI resource scanning. >=20 >=20 > > > > And that is why the code in probe.c has been added to deal with= that. It is > > > > too early to do the adjustments here as all we have is the list= of resources > > > > and that might get culled by the architecture fixup code. Remem= bering the > > > > io_offset will happen once the pci_host_bridge gets created, an= d the resources > > > > are then adjusted. > > > > > > So you want to register an incorrect I/O resource first and then > > > have it fixed up later, rather than registering the correct > > > one from the start as everyone else? > >=20 > > The incorrect I/O resource is added to a temporary list of resource= s, it has not > > been attached yet to the list of windows in the bridge. What gets a= dded is the > > I/O resource as described if it would be an ordinary resource. >=20 > I'm not completely sure I'm following here, but let's work out the > other things first, this will probably get clearer then. >=20 > > > > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > > > > > > index 6e34498..16febae 100644 > > > > > > --- a/drivers/pci/probe.c > > > > > > +++ b/drivers/pci/probe.c > > > > > > @@ -1787,6 +1787,17 @@ struct pci_bus *pci_create_root_bus(= struct device *parent, int bus, > > > > > > list_for_each_entry_safe(window, n, resources, list) { > > > > > > list_move_tail(&window->list, &bridge->windows)= ; > > > > > > res =3D window->res; > > > > > > + /* > > > > > > + * IO resources are stored in the kernel with a= CPU start > > > > > > + * address of zero. Adjust the data accordingly= and remember > > > > > > + * the offset > > > > > > + */ > > > > > > + if (resource_type(res) =3D=3D IORESOURCE_IO) { > > > > > > + bridge->io_offset =3D res->start; > > > > > > + res->end -=3D res->start; > > > > > > + window->offset -=3D res->start; > > > > > > + res->start =3D 0; > > > > > > + } > >=20 > > Here, we correct for the fact that IORESOURCE_IO is not a normal re= source, because Linux wants > > a logical I/O as start and end address, not the physical CPU addres= s. We adjust to that and > > remember the offset. >=20 > But the offset (phys_base) doesn't actually matter to the PCI core or > the driver. Why save it? Because I need it later for the host bridge ATR setup. >=20 > > > > > > offset =3D window->offset; > > > > > > if (res->flags & IORESOURCE_BUS) > > > > > > > > > > Won't this break all existing host bridges? > > > > > > > > I am not sure. I believe not, due to what I've explained earlie= r, but you might be right. > > > > > > > > The adjustment happens before the resource is added to the host= bridge windows and translates > > > > it from MMIO range into IO range. > > > > > > AFAICT, the resource_type of the resource you register above shou= ld be > > > IORESOURCE_MEM, so you are not actually matching it here. > >=20 > > No, all resources are added here. For IORESOURCE_IO we do an adjust= ment. >=20 > But there should never be an IORESOURCE_IO resource structure that is > not in IO space, i.e. within ioport_resource. Doing an "adjustment" > is not an operation defined on this structure. What I meant above is = that > the pci range parser gets this right and gives you a resource that lo= oks > like { .flags =3D IORESOURCE_MEM, .start =3D phys_base, .end =3D phys= _base + > size - 1}, while the resource we want to register is { .flags =3D IOR= ESOURCE_IO, > .start =3D log_base, .end =3D log_base + size -1}. In the of_pci_rang= e struct for > the I/O space, the "pci_space" is IORESOURCE_IO (for the pci_addr), w= hile the > "flags" are IORESOURCE_MEM, to go along with the cpu_addr. The pci range parser gives me a range with .flags =3D IORESOURCE_IO for= IO space. It does not convert it to IORESOURCE_MEM. Hence the need for adjustment. Best regards, Liviu >=20 > 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 >=20 >=20 --=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