Re: [PATCH v2 05/10] ARM: tegra: Rewrite PCIe support as a driver

[Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 4739 bytes --]

* Stephen Warren wrote:
> On 06/11/2012 09:05 AM, Thierry Reding wrote:
> > This commit adds a platform device driver for the PCIe controller on
> > Tegra SOCs. Current users of the old code (TrimSlice and Harmony) are
> > converted and now initialize and register a corresponding platform
> > device.
> 
> > diff --git a/arch/arm/mach-tegra/devices.c b/arch/arm/mach-tegra/devices.c
> 
> > +static struct resource tegra_pcie_resources[] = {
> > +	[0] = {
> > +		.start = TEGRA_PCIE_BASE,
> > +		.end = TEGRA_PCIE_BASE + TEGRA_PCIE_SIZE - 1,
> > +		.flags = IORESOURCE_MEM,
> > +	},
> > +	[1] = {
> > +		.start = TEGRA_PCIE_MMIO_BASE,
> > +		.end = TEGRA_PCIE_MMIO_BASE + TEGRA_PCIE_MMIO_SIZE - 1,
> > +		.flags = IORESOURCE_MEM,
> > +	},
> > +	[2] = {
> > +		.start = INT_PCIE_INTR,
> > +		.end = INT_PCIE_INTR,
> > +		.flags = IORESOURCE_IRQ,
> > +	},
> > +};
> 
> I'm not sure those resources either cover all the necessary regions, nor
> are as fine-grained as they should be ...
> 
> In particular, in pcie.c, I see separate afi_writel() and pads_writel()
> implying those are separate regions.
> 
> Also, I see the following hard-code specific addresses, and are still
> used by the driver after conversion:
> 
> > #define MEM_BASE_0              (TEGRA_PCIE_BASE + SZ_256M)
> > #define MEM_SIZE_0              SZ_128M
> > #define MEM_BASE_1              (MEM_BASE_0 + MEM_SIZE_0)
> > #define MEM_SIZE_1              SZ_128M
> > #define PREFETCH_MEM_BASE_0     (MEM_BASE_1 + MEM_SIZE_1)
> > #define PREFETCH_MEM_SIZE_0     SZ_128M
> > #define PREFETCH_MEM_BASE_1     (PREFETCH_MEM_BASE_0 + PREFETCH_MEM_SIZE_0)
> > #define PREFETCH_MEM_SIZE_1     SZ_128M
> 
> Also, there's a comment describing the register layout in terms of a
> number of separate regions:
> 
> > /*
> >  * Tegra2 defines 1GB in the AXI address map for PCIe.
> >  *
> >  * That address space is split into different regions, with sizes and
> >  * offsets as follows:
> >  *
> >  * 0x80000000 - 0x80003fff - PCI controller registers
> >  * 0x80004000 - 0x80103fff - PCI configuration space
> >  * 0x80104000 - 0x80203fff - PCI extended configuration space
> >  * 0x80203fff - 0x803fffff - unused
> >  * 0x80400000 - 0x8040ffff - downstream IO
> >  * 0x80410000 - 0x8fffffff - unused
> >  * 0x90000000 - 0x9fffffff - non-prefetchable memory
> >  * 0xa0000000 - 0xbfffffff - prefetchable memory
> >  */
> 
> (the latter 2 regions at least being also split in half for each of the
> 2 host ports)

I was thinking that maybe each port should be represented as a child node,
but I'm not sure that's going to work too well because the children of the
pci node are supposed to be PCI busses/devices. I'll have to check whether
they could just as well be children of the PCIe port nodes.

That way each port could get an own set of register ranges to better describe
the actual layout. AFAICT the even partitioning of the non-prefetchable and
prefetchable memory regions is arbitrary and it could potentially be useful
to make it configurable via the DT.

> Shouldn't each of these regions be a separate entry in the platform
> device resources.
> 
> This perhaps isn't that relevant for Tegra20 alone, but Tegra30 supports
> 3 host ports instead of 2 (hence I presume alters the internal layout of
> the 1G chunk of physical memory space allocated to PCIe), and moves the
> PCIe area from 2G..3G to 0G..1G (so invalidates the hard-coded
> *_MEM_BASE_* above).

> That said, I'm not sure whether Tegra20's and Tegra30's PCIe controllers
> are similar enough to make a shared driver worthwhile/possible.
> (Although our downstream Android driver appears to handle both with a
> very small number of ifdefs).

> If they are similar, then I think my comments above should be addressed.
> If they are not similar, then I think you can just have a single 1G
> memory region in the resources, and split it up internally, rather than
> needing separate resources for different parts of the address space.

Unfortunately the PCIe controller is very badly documented in the TRM, so
this information is hard to locate (I guess the best source is one of the
downstream drivers).

Last time I looked they seemed to be simple enough to handle the differences
using OF compatible matches. I believe apart from the PCIe area changes there
were some differences in how the MSI were setup.

> While we can easily fix this kind of driver internals so this doesn't
> seem like a big deal, this kind of change would impact the device tree
> binding, so it seems that we need to sort it out before DT conversion.

Yes, we should at least try to get it right from the start.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]