On Wednesday 31 October 2012, Pratyush Anand wrote:

On 10/31/2012 3:50 AM, Arnd Bergmann wrote:

quoted

On Monday 29 October 2012, Pratyush Anand wrote:

quoted

SPEAr13xx has dual mode PCIe controller which can be used as Root
Complex as well as Endpoint. This driver supports RC mode of the
controller.

If CONFIG_PCI_MSI is defined then support of MSI interrupt for
downstream  PCIe devices will be enabled.

If CONFIG_PM is defined then it supports suspend/resume functionality.

I think I've seen the same hardware before used with another driver.
IMHO we should really make sure this does not get duplicated again and
move this stuff to a new subdirectory drivers/pci/host. We don't yet have
a completely architecture independent way of defining a PCIe root complex,
but we're getting there and given of how many ARM implementations we have,
we can just as well start with this one and adapt it to another arch if
necesary.

Yes, I too wanted to put it into drivers/pci/host. But, still it would 
be ARCH (arm) dependent, until we have a common solution for all 
architecture. Let me know, if I can put it into drivers/pci/host even if 
it works initially for ARM only.

This is ultimately up to Bjorn Helgaas as the PCI maintainer. I think
everybody would prefer to have an architecture independent interface between
PCI hosts and platform specific code, and we are progressing towards that
goal. I think it would be worthwhile to have this code already in
drivers/pci while it's still ARM specific, and then remove the ARM bits
over time by replacing them with generic interfaces as we get there.

quoted

quoted

+struct pcie_port {
+	u8			controller;
+	u8			root_bus_nr;
+	void __iomem		*dbi_base;
+	void __iomem		*va_dbi_base;
+	void __iomem		*app_base;
+	void __iomem		*va_app_base;
+	void __iomem		*base;
+	void __iomem		*phy_base;
+	void __iomem		*va_phy_base;
+	void __iomem		*cfg0_base;
+	void __iomem		*va_cfg0_base;
+	void __iomem		*cfg1_base;
+	void __iomem		*va_cfg1_base;
+	void __iomem		*mem_base;
+	void __iomem		*io_base;
+	spinlock_t		conf_lock;
+	char			mem_space_name[16];
+	char			io_space_name[16];
+	struct resource		res[2];
+	struct pcie_port_info	config;
+	struct list_head	next;
+	struct clk		*clk;
+	int			irq;
+	int			virt_irq_base;
+	int			susp_state;
+};

Can you explain why you need a total of 13 virtual memory areas?

Ah, its not 13, Its just 5 all va_xxxx.
Probably, I have wrongly put __iomem annotation  with physical addresses 
too. I will correct. Will keep physical address as normal void *.

Physical addresses should not be pointers at all, but they should be
phys_addr_t. If you have a system with LPAE enabled, the physical
address space may be 64 bit long, so pointers don't work on that.

quoted

quoted

+static struct list_head	pcie_port_list;

I'm pretty sure you don't need your own list of ports, since we already enumerate the
ports in the PCI code.

This pcie_port_list is list of private data (struct pcie_port). 
Callbacks defined in struct hw_pci such as setup, scan etc are called 
with first argument as controller number. Now other option could have 
been to use a simple array of struct pcie_port[MAX_PORT_NUM], as other 
platforms has used. But I thought to keep list as better option, so that 
we are not dependent on MAX_PORT_NUM.

A list is indeed better than a static array. However, the preferred way to
do kernel code is to not have any such global lists in a device driver at
all, but keep it inside of the subsystems. I don't remember how this is
done for PCIe ports, but I would assume that you can iterate over them
already. If not, it would be nice to add this feature to the PCI subsystem
so that nobody has to keep arrays or lists like this.

quoted

quoted

+static struct hw_pci pci;

Why is this not statically initialized?

Ok, ll take care in next version.

I noticed later that it does get statically initialized below, so my comment wasn't
actually correct. However, you should try to order the symbols so that you don't
need forward declarations.

quoted

This function looks completely bogus. Subtracting PCIBIOS_MIN_IO means that
everything is shifted by 0x1000 ports, since secondary bridges don't
have this offset. The I/O spaces are just mapped linearly into the virtual
address space anyway, so looking up the I/O base like this is pointless.

Frankly speaking, I do not have clear understanding of PCIe IO 
transaction. I did not test any PCIe device with my host which might 
need IO resource. Since , my host controller does support IO 
transaction, I want to support it in driver. Let me explain my 
requirement, please correct my understanding where I am wrong.

-- I can program one my viewport to generate IO transaction on PCIe bus, 
if CPU writes between AHB address 0x80000000-0x80007FFF.

-- Now, lets say a PCIe device needs 100 io ports, then address range 
0x80000000-0x80000064 will be allocated to this device by pci stack. It 
will also write 0x80000000 to IO_BAR register of its configuration 
address space. Now, if my CPU writes at ahb address 0x80000010 then 16th 
IO port of this device will be written.

The number space for I/O ports is usually 16 bits, which is a hardware
limitation on x86. Practically all PCI implementations on non-x86
architecture have the I/O access directly mapped into the physical
address space, like you have at phys address 0x80000000. However, the
port number for the first port in there is still "0", not "0x80000000",
and the I/O window in you PCI host registers should be set up so that
an access to address 0x80000000 gets translated into a I/O transfer
at address 0 on the PCI bus. This is different from the memory window,
which is typically set up so that the physical addresses are the
same as the bus address in PCI memory space accesses.

The io_offset should be zero on all modern implementations, we just have a few
legacy cases that we carry around mostly because of interactions with older
PCMCIA drivers that can get fixed over time.

If you have multiple root buses, you can choose to make the offset for the
second bus 0x10000.

quoted

quoted

+static void pcie_host_init(struct pcie_port *pp)
+{
+	struct pcie_port_info *config = &pp->config;
+	void __iomem *dbi_base = pp->va_dbi_base;
+	struct pcie_app_reg *app_reg = pp->va_app_base;
+	u32 exp_cap_off = PCI_CAP_ID_EXP_OFFSET;
+	u32 val;
+
+	/* Keep first 64K for IO */
+	pp->io_base = pp->base;
+	pp->mem_base = pp->io_base + config->io_size;
+	pp->cfg0_base = pp->mem_base + config->mem_size;
+	pp->cfg1_base = pp->cfg0_base + config->cfg0_size;

Why is the memory space part of the virtual bus address range here?

Sorry, its not virtual address. I will remove __iomem from all physical 
address pointer.

Ok. I suspect that you don't actually need to store the physicall address
then, because it's not needed after setting up the devices, and you can easily
retrieve it from the resources.

quoted

quoted

+	pp->config.io_size = IO_SIZE_PER_PORT;
+	of_property_read_u32(np, "pcie-host,is_host", &pp->config.is_host);
+	of_property_read_u32(np, "pcie-host,is_gen1", &pp->config.is_gen1);

These should probably be derived from the "compatible" value.

Yes, about is_host I think, I will go with "compatible".

Let me explain why I have provided is_gen1: There are few buggy PCIe 
cards which links up only if host is gen1. My host is by default gen2. 
It does work with gen1 as well as gen2 devices except some buggy gen1 
devices. I provided this flag, to force controller to linkup at gen1 
only so that it can work with those buggy cards too.

Ok, makes sense.

Thanks for the quick replies. I hope I managed to explain the I/O space accesses
now.

	Arnd