Patchwork [06/32] arm: pci: add a align_resource hook

login
register
mail settings
Submitter Thomas Petazzoni
Date Feb. 12, 2013, 4:28 p.m.
Message ID <1360686546-24277-7-git-send-email-thomas.petazzoni@free-electrons.com>
Download mbox | patch
Permalink /patch/219911/
State Not Applicable
Headers show

Comments

Thomas Petazzoni - Feb. 12, 2013, 4:28 p.m.
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(+)
Arnd Bergmann - Feb. 12, 2013, 6:03 p.m.
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
--
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
Thomas Petazzoni - Feb. 12, 2013, 7:01 p.m.
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
Russell King - ARM Linux - Feb. 12, 2013, 7:49 p.m.
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.
--
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

Patch

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;
 }