| Message ID | 1410163532-31057-1-git-send-email-lisovy@gmail.com |
|---|---|
| State | New |
| Headers | show |
[+cc Mike] This doesn't touch drivers/pci or conflict with any PCI-related changes, so I'll leave this up to Russell. My comments on this are just kibbitzing. On Mon, Sep 8, 2014 at 2:05 AM, Rostislav Lisovy <lisovy@gmail.com> wrote: > Signed-off-by: Rostislav Lisovy <lisovy@gmail.com> > Reviewed-by: Yijing Wang <wangyijing@huawei.com> > --- > The header file include/linux/pci_ids.h defines > #define PCI_CLASS_BRIDGE_OTHER 0x0680 > #define PCI_CLASS_SYSTEM_DMA 0x0801 > > ((struct pci_dev*)dev)->class > corresponds to the 3 bytes Class code in the PCI Configuration space > header -- 1B Base class, 1B Sub-class, 1B Reg-level interface. > > In that case > (PCI_CLASS_BRIDGE_OTHER << 8) > is equivalent to 0x68000 (imagine the leading zero) > and > ((PCI_CLASS_SYSTEM_DMA << 8) | 0x03)) > is equivalent to 0x80103. This part looks like it's supposed to be the changelog, which should be above the Signed-off-by. > arch/arm/kernel/bios32.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c > index 17a26c1..e511ad1 100644 > --- a/arch/arm/kernel/bios32.c > +++ b/arch/arm/kernel/bios32.c > @@ -255,10 +255,9 @@ static void pci_fixup_it8152(struct pci_dev *dev) > { > int i; > /* fixup for ITE 8152 devices */ > - /* FIXME: add defines for class 0x68000 and 0x80103 */ > if ((dev->class >> 8) == PCI_CLASS_BRIDGE_HOST || > - dev->class == 0x68000 || > - dev->class == 0x80103) { > + dev->class == (PCI_CLASS_BRIDGE_OTHER << 8) || > + dev->class == ((PCI_CLASS_SYSTEM_DMA << 8) | 0x03)) { > for (i = 0; i < PCI_NUM_RESOURCES; i++) { > dev->resource[i].start = 0; > dev->resource[i].end = 0; The interesting part to me is the part you didn't change, i.e., setting the resource start/end/flags to 0. That's all from a8fc0789558d ("[ARM] 4577/1: ITE 8152 PCI bridge support"), which unfortunately doesn't have any more details about *why* this is needed. Presumably these devices have PCI BARs (since that's how dev->resource got filled in), and we want to somehow ignore them, not assign resources to them, and not allow drivers to use whatever is mapped by the BARs. But clearing out the struct resources isn't really safe by itself. The hardware BAR still contains some value, and may still be enabled, which means that it may conflict with other devices. I guess you're not making a functional change, so maybe we don't need to worry about this. But it does leave me scratching my head a bit. Bjorn
On Mon, Sep 08, 2014 at 10:36:36AM -0600, Bjorn Helgaas wrote: > Presumably these devices have PCI BARs (since that's how dev->resource > got filled in), and we want to somehow ignore them, not assign > resources to them, and not allow drivers to use whatever is mapped by > the BARs. But clearing out the struct resources isn't really safe by > itself. The hardware BAR still contains some value, and may still be > enabled, which means that it may conflict with other devices. I've seen this pattern before in the old mvebu stuff. Essentially for that case the mvebu HW is a repurposed end port core, so when working as a root port it presents an end port config space (not a root port bridge config space). The BAR in that config space is not relevant, so the old mvebu used code like this to hide it from the PCI core. Otherwise the core will try to program it from the aperture, and the default BAR size is really big, so it runs out of space. Not sure if this is the same for this driver, but it looks very similar.. Jason
[-cc Mike's old email address] On Mon, Sep 8, 2014 at 12:03 PM, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote: > On Mon, Sep 08, 2014 at 10:36:36AM -0600, Bjorn Helgaas wrote: > >> Presumably these devices have PCI BARs (since that's how dev->resource >> got filled in), and we want to somehow ignore them, not assign >> resources to them, and not allow drivers to use whatever is mapped by >> the BARs. But clearing out the struct resources isn't really safe by >> itself. The hardware BAR still contains some value, and may still be >> enabled, which means that it may conflict with other devices. > > I've seen this pattern before in the old mvebu stuff. > > Essentially for that case the mvebu HW is a repurposed end port core, > so when working as a root port it presents an end port config space > (not a root port bridge config space). The BAR in that config space is > not relevant, so the old mvebu used code like this to hide it from the > PCI core. Otherwise the core will try to program it from the aperture, > and the default BAR size is really big, so it runs out of space. How does mvebu deal with this now? I don't see anything similar in pci-mvebu.c. If the BAR is still a functioning PCI BAR, i.e., it decodes address space and potentially responds to accesses there, we really should pay attention to it. That doesn't mean we have to assign space to it, but for example, we might want to make sure we have PCI_COMMAND_MEM turned off. If we just clear out the resource start/end/flags, the PCI core doesn't have enough information to do that. Bjorn
On Mon, Sep 08, 2014 at 01:39:40PM -0600, Bjorn Helgaas wrote: > > Essentially for that case the mvebu HW is a repurposed end port core, > > so when working as a root port it presents an end port config space > > (not a root port bridge config space). The BAR in that config space is > > not relevant, so the old mvebu used code like this to hide it from the > > PCI core. Otherwise the core will try to program it from the aperture, > > and the default BAR size is really big, so it runs out of space. > > How does mvebu deal with this now? I don't see anything similar in > pci-mvebu.c. The new driver doesn't allow the core directly access to the end point config space registers, the entire config space is managed by the driver directly, exactly how the HW requires it to be set for root port operation. The core sees the config space of a root port bridge that is created by the driver. > If the BAR is still a functioning PCI BAR, i.e., it decodes address > space and potentially responds to accesses there, we really should pay > attention to it. That doesn't mean we have to assign space to it, but > for example, we might want to make sure we have PCI_COMMAND_MEM turned > off. If we just clear out the resource start/end/flags, the PCI core > doesn't have enough information to do that. In mvebu land, the BAR does need to have a proper value. IIRC, with the old driver the bootloader was expected to configure PCI to an operational state, and the kernel was expected not to touch that BAR register at all. For mvebu the new driver explicitly sets that BAR properly like this: /* Setup BAR[1] to all DRAM banks. */ mvebu_writel(port, dram->cs[0].base, PCIE_BAR_LO_OFF(1)); mvebu_writel(port, 0, PCIE_BAR_HI_OFF(1)); mvebu_writel(port, ((size - 1) & 0xffff0000) | 1, PCIE_BAR_CTRL_OFF(1)); [Note: mvebu has a memory mapped alias for the end port config space, which is what PCIE_BAR_LO_OFF is referencing.] Eg, it is matching the DRAM. This is because in an end port the BAR claims incoming MemRd/MemWr TLPs, and for root port operation you want the HW to claim all TLPs related to system memory. The old driver (linux/arch/arm/mach-orion5x/pci.c) did this: static void rc_pci_fixup(struct pci_dev *dev) { /* * Prevent enumeration of root complex. */ if (dev->bus->parent == NULL && dev->devfn == 0) { int i; for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) { dev->resource[i].start = 0; dev->resource[i].end = 0; dev->resource[i].flags = 0; } } } DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_MARVELL, PCI_ANY_ID, rc_pci_fixup); Which seems very similar to what you quoted? Jason
On Mon, Sep 8, 2014 at 2:04 PM, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote: > On Mon, Sep 08, 2014 at 01:39:40PM -0600, Bjorn Helgaas wrote: > >> > Essentially for that case the mvebu HW is a repurposed end port core, >> > so when working as a root port it presents an end port config space >> > (not a root port bridge config space). The BAR in that config space is >> > not relevant, so the old mvebu used code like this to hide it from the >> > PCI core. Otherwise the core will try to program it from the aperture, >> > and the default BAR size is really big, so it runs out of space. This is a bit of a tangent, but an endpoint has a type 0 config header, while a root port is a bridge and has a type 1 config header. I don't understand how the PCI core can operate a root port correctly if that port has a type 0 config header. >> How does mvebu deal with this now? I don't see anything similar in >> pci-mvebu.c. > > The new driver doesn't allow the core directly access to the end point > config space registers, the entire config space is managed by the > driver directly, exactly how the HW requires it to be set for root > port operation. The core sees the config space of a root port bridge > that is created by the driver. > >> If the BAR is still a functioning PCI BAR, i.e., it decodes address >> space and potentially responds to accesses there, we really should pay >> attention to it. That doesn't mean we have to assign space to it, but >> for example, we might want to make sure we have PCI_COMMAND_MEM turned >> off. If we just clear out the resource start/end/flags, the PCI core >> doesn't have enough information to do that. > > In mvebu land, the BAR does need to have a proper value. IIRC, with > the old driver the bootloader was expected to configure PCI to an > operational state, and the kernel was expected not to touch that BAR > register at all. > > For mvebu the new driver explicitly sets that BAR properly like this: > > /* Setup BAR[1] to all DRAM banks. */ > mvebu_writel(port, dram->cs[0].base, PCIE_BAR_LO_OFF(1)); > mvebu_writel(port, 0, PCIE_BAR_HI_OFF(1)); > mvebu_writel(port, ((size - 1) & 0xffff0000) | 1, > PCIE_BAR_CTRL_OFF(1)); > > [Note: mvebu has a memory mapped alias for the end port config space, > which is what PCIE_BAR_LO_OFF is referencing.] > > Eg, it is matching the DRAM. This is because in an end port the BAR > claims incoming MemRd/MemWr TLPs, and for root port operation you want > the HW to claim all TLPs related to system memory. > > The old driver (linux/arch/arm/mach-orion5x/pci.c) did this: > > static void rc_pci_fixup(struct pci_dev *dev) > { > /* > * Prevent enumeration of root complex. > */ > if (dev->bus->parent == NULL && dev->devfn == 0) { > int i; > > for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) { > dev->resource[i].start = 0; > dev->resource[i].end = 0; > dev->resource[i].flags = 0; > } > } > } > DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_MARVELL, PCI_ANY_ID, rc_pci_fixup); > > Which seems very similar to what you quoted? I haven't worked with devices like this before, so pardon my ignorance. It sounds like when these devices are configured as root ports, they have BARs visible to the host, which is how dev->resource[] would get filled in to begin with. We manage normal BARs so the CPU can perform MMIO accesses to the device. In this case, it sounds like the BARs are more for the benefit of devices below the bridge that want to perform DMA. The PCI core certainly wouldn't know what to do about something like that, so I agree the best thing to do is to clear out the resource completely and pretend it doesn't exist. I guess a new (not repurposed) design would probably put stuff like this in device-specific config space, not in architected BARs. Bjorn
On Tue, Sep 09, 2014 at 01:31:46PM -0600, Bjorn Helgaas wrote: > On Mon, Sep 8, 2014 at 2:04 PM, Jason Gunthorpe > <jgunthorpe@obsidianresearch.com> wrote: > > On Mon, Sep 08, 2014 at 01:39:40PM -0600, Bjorn Helgaas wrote: > > > >> > Essentially for that case the mvebu HW is a repurposed end port core, > >> > so when working as a root port it presents an end port config space > >> > (not a root port bridge config space). The BAR in that config space is > >> > not relevant, so the old mvebu used code like this to hide it from the > >> > PCI core. Otherwise the core will try to program it from the aperture, > >> > and the default BAR size is really big, so it runs out of space. > > This is a bit of a tangent, but an endpoint has a type 0 config > header, while a root port is a bridge and has a type 1 config header. > I don't understand how the PCI core can operate a root port correctly > if that port has a type 0 config header. Well, it cannot operate 'correctly' but something gets bodged together enough to pass basic scenarios.. Everything gets flattened down to bus 0, so the end port config space gets to be device 0x10, and then config access to other device numbers are forwarded out as TLPs to whatever is connected, so typically the connected device shows up as device 0x0. > It sounds like when these devices are configured as root ports, they > have BARs visible to the host, which is how dev->resource[] would get > filled in to begin with. We manage normal BARs so the CPU can perform > MMIO accesses to the device. In this case, it sounds like the BARs > are more for the benefit of devices below the bridge that want to > perform DMA. The PCI core certainly wouldn't know what to do about > something like that, so I agree the best thing to do is to clear out > the resource completely and pretend it doesn't exist. I guess a new > (not repurposed) design would probably put stuff like this in > device-specific config space, not in architected BARs. Yes to all of this. Jason
diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c index 17a26c1..e511ad1 100644 --- a/arch/arm/kernel/bios32.c +++ b/arch/arm/kernel/bios32.c @@ -255,10 +255,9 @@ static void pci_fixup_it8152(struct pci_dev *dev) { int i; /* fixup for ITE 8152 devices */ - /* FIXME: add defines for class 0x68000 and 0x80103 */ if ((dev->class >> 8) == PCI_CLASS_BRIDGE_HOST || - dev->class == 0x68000 || - dev->class == 0x80103) { + dev->class == (PCI_CLASS_BRIDGE_OTHER << 8) || + dev->class == ((PCI_CLASS_SYSTEM_DMA << 8) | 0x03)) { for (i = 0; i < PCI_NUM_RESOURCES; i++) { dev->resource[i].start = 0; dev->resource[i].end = 0;