| Message ID | 1360686546-24277-7-git-send-email-thomas.petazzoni@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tuesday 12 February 2013, Thomas Petazzoni wrote: > The PCI specifications says that an I/O region must be aligned on a 4 > KB boundary, and a memory region aligned on a 1 MB boundary. > > However, the Marvell PCIe interfaces rely on address decoding windows > (which allow to associate a range of physical addresses with a given > device), and those have special requirements compared to the standard > PCI-to-PCI bridge specifications. I'm not convince that we should add this complexity yet, until everyone agrees on the basic approach taken. Arnd
Dear Arnd Bergmann, On Tue, 12 Feb 2013 18:03:12 +0000, Arnd Bergmann wrote: > On Tuesday 12 February 2013, Thomas Petazzoni wrote: > > The PCI specifications says that an I/O region must be aligned on a > > 4 KB boundary, and a memory region aligned on a 1 MB boundary. > > > > However, the Marvell PCIe interfaces rely on address decoding > > windows (which allow to associate a range of physical addresses > > with a given device), and those have special requirements compared > > to the standard PCI-to-PCI bridge specifications. > > I'm not convince that we should add this complexity yet, until > everyone agrees on the basic approach taken. Regardless of whether we choose to have the emulated PCI-to-PCI bridges or not, we still need this align_resource() hook. In the solution you propose, where each PCIe interface is represented as a separate PCIe domain, we still need the kernel to dynamically assign ranges of address to each memory BAR and I/O BAR of each PCIe device. And those range of address must comply with the address decoding windows requirements, otherwise, we don't be able to create those address decoding windows, and the devices will be unaccessible. Of course, if you have an alternate solution to do a _dynamic_ assignment of address ranges to the different PCIe devices, I'm entirely open to suggestions. Best regards, Thomas
On Tue, Feb 12, 2013 at 08:01:18PM +0100, Thomas Petazzoni wrote: > Dear Arnd Bergmann, > > On Tue, 12 Feb 2013 18:03:12 +0000, Arnd Bergmann wrote: > > On Tuesday 12 February 2013, Thomas Petazzoni wrote: > > > The PCI specifications says that an I/O region must be aligned on a > > > 4 KB boundary, and a memory region aligned on a 1 MB boundary. > > > > > > However, the Marvell PCIe interfaces rely on address decoding > > > windows (which allow to associate a range of physical addresses > > > with a given device), and those have special requirements compared > > > to the standard PCI-to-PCI bridge specifications. > > > > I'm not convince that we should add this complexity yet, until > > everyone agrees on the basic approach taken. > > Regardless of whether we choose to have the emulated PCI-to-PCI bridges > or not, we still need this align_resource() hook. > > In the solution you propose, where each PCIe interface is represented > as a separate PCIe domain, we still need the kernel to dynamically > assign ranges of address to each memory BAR and I/O BAR of each PCIe > device. And those range of address must comply with the address > decoding windows requirements, otherwise, we don't be able to create > those address decoding windows, and the devices will be unaccessible. I think what Arnd is suggesting is that you treat each of your busses as a separate root bus. This then means that you register each bus separately via pci_common_init() (this can take 0..N root buses and scan them.) We can then size each bus (which means, totalling up the amount of space each bus requires). At that point, we know how much memory and IO space each bus requires, and platform code can then setup the appropriate bridge windows. The alignment of the bases is something that platform code can deal with in its allocation mechanism, and the resulting allocated resources can then be assigned to each PCI bus. Once that's done, we can then call pci_bus_assign_resources() to setup the resources for each bus, which will use our allocated resources. There's a downside to this though: the pci core doesn't size root buses, because: (a) it assumes that they're fixed size. (b) root buses don't have a P2P 'bridge' to be sized. I'm wondering if we could come up with a way to refactor __pci_bus_size_bridges() so that we can get this information out of it. That's more a question for PCI people though.
diff --git a/arch/arm/include/asm/mach/pci.h b/arch/arm/include/asm/mach/pci.h index 5cf2e97..7d2c3c8 100644 --- a/arch/arm/include/asm/mach/pci.h +++ b/arch/arm/include/asm/mach/pci.h @@ -30,6 +30,11 @@ struct hw_pci { void (*postinit)(void); u8 (*swizzle)(struct pci_dev *dev, u8 *pin); int (*map_irq)(const struct pci_dev *dev, u8 slot, u8 pin); + resource_size_t (*align_resource)(struct pci_dev *dev, + const struct resource *res, + resource_size_t start, + resource_size_t size, + resource_size_t align); }; /* @@ -51,6 +56,12 @@ struct pci_sys_data { u8 (*swizzle)(struct pci_dev *, u8 *); /* IRQ mapping */ int (*map_irq)(const struct pci_dev *, u8, u8); + /* Resource alignement requirements */ + resource_size_t (*align_resource)(struct pci_dev *dev, + const struct resource *res, + resource_size_t start, + resource_size_t size, + resource_size_t align); void *private_data; /* platform controller private data */ }; diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c index 5401645..be2e6c9 100644 --- a/arch/arm/kernel/bios32.c +++ b/arch/arm/kernel/bios32.c @@ -462,6 +462,7 @@ static void __init pcibios_init_hw(struct hw_pci *hw, struct list_head *head) sys->busnr = busnr; sys->swizzle = hw->swizzle; sys->map_irq = hw->map_irq; + sys->align_resource = hw->align_resource; INIT_LIST_HEAD(&sys->resources); if (hw->private_data) @@ -574,6 +575,8 @@ char * __init pcibios_setup(char *str) resource_size_t pcibios_align_resource(void *data, const struct resource *res, resource_size_t size, resource_size_t align) { + struct pci_dev *dev = data; + struct pci_sys_data *sys = dev->sysdata; resource_size_t start = res->start; if (res->flags & IORESOURCE_IO && start & 0x300) @@ -581,6 +584,9 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res, start = (start + align - 1) & ~(align - 1); + if (sys->align_resource) + return sys->align_resource(dev, res, start, size, align); + return start; }
The PCI specifications says that an I/O region must be aligned on a 4 KB boundary, and a memory region aligned on a 1 MB boundary. However, the Marvell PCIe interfaces rely on address decoding windows (which allow to associate a range of physical addresses with a given device), and those have special requirements compared to the standard PCI-to-PCI bridge specifications. PCIe memory windows must have a power of two size (which matches the PCI spec), be at least of 1 MB (which matches the PCI spec), and be aligned on their size (which does not match the PCI spec, that requires only a 1 MB alignment). PCIe I/O windows must have a power of two size (matches the PCI spec), be at least of 64 KB (doesn't match the PCI spec, which requires only a size of 4 KB), and be aligned on their size (doesn't match the PCI spec). The PCI core already calls pcibios_align_resource() in the ARM PCI core, specifically for such purposes. So this patch extends the ARM PCI core so that it calls a ->align_resource() hook registered by the PCI driver, exactly like the existing ->map_irq() and ->swizzle() hooks. A particular PCI driver can register a align_resource() hook, and do its own specific alignement, depending on the specific constraints of the underlying hardware. Following the earlier reviews of this patch, a few comments: * Using numerical values is not possible since the alignment value is not absolute, but depends on the size of the memory window or I/O window. We therefore really need a function hook. * Using window_alignment() doesn't work, because it doesn't allow to adjust the starting address of the memory or I/O window, and we don't have access to the size. The comment was that ->resource_align() would apply to all resources on all busses, but in the new implementation of the Marvell-specific ->resource_align() hook we are careful to only apply the alignment constraints on the specific bus that hosts the emulated PCI-to-PCI bridges needed to operate the Marvell PCIe interfaces. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> Cc: Russell King <linux@arm.linux.org.uk> --- arch/arm/include/asm/mach/pci.h | 11 +++++++++++ arch/arm/kernel/bios32.c | 6 ++++++ 2 files changed, 17 insertions(+)