[PATCH 1/6] Realview PCIX support - add main support module code

[colin.tuckley@arm.com (Colin Tuckley)]

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd at arndb.de]
> Sent: 20 October 2010 22:17

First, I need to point out that this code and the patches are quite old.
They have been used internally since before 2.6.33 and I then forward ported
them for submission. I don't have a lot of time to do a proper
re-validation.

> How about merging this patch with into the last one? No point
> adding code that doesn't even get seen by the compiler.

The idea was to keep the series in logical chunks and since this was already
quite large I kept it separate.

> ptrace.h?

Oops, a left over from some debugging I suspect - removed.

> > +/* PCI-X Configuration registers */
> > +#define PCI_VENID			0x00
> > +#define PCI_DEVID			0x02
> > +#define PCI_COMMAND			0x04
> > ...
> 
> These don't belong here -- just use the common definitions
> from pci_regs.h. The extended config registers starting at 0x40
> are obviously fine here.

Removed, except for the ones that use names that match the chip
documentation.

> How about a temporary
> 	void __iomem *base = PCIX_UNIT_BASE;
> as a shortcut so you don't have to write PCIX_UNIT_BASE every time.

I believe it's clearer the way it is.

> > +#ifdef CONFIG_REALVIEW_HIGH_PHYS_OFFSET
> > +	/* 512MB of DMA capable RAM is available - mapped at 0x70000000
> > +	 * Note :- 0x70000000 is *not* on a 512MB boundary so it
> > +	 * must be mapped in two parts */
> > +	/* Window#0 256MB and enable */
> > +	writel(0x0fff0001, PCIX_UNIT_BASE + PCI_PTMEMMSK0);
> > +	/* Window#1 256MB and enable */
> > +	writel(0x0fff0001, PCIX_UNIT_BASE + PCI_PTMEMMSK1);
> > +
> > +	/* Window base address setting */
> > +	/* Memory Base Address, Ptrefetch Disable, 64bit */
> > +	writel(0x70000004, PCIX_UNIT_BASE + PCI_PTBADDR0L);
> > +	/* Memory Base Address[63:32] */
> > +	writel(0x00000000, PCIX_UNIT_BASE + PCI_PTBADDR0M);
> > +
> > +	/* Memory Base Address, Ptrefetch Disable, 64bit */
> > +	writel(0x80000004, PCIX_UNIT_BASE + PCI_PTBADDR1L);
> > +	/* Memory Base Address[63:32] */
> > +	writel(0x00000000, PCIX_UNIT_BASE + PCI_PTBADDR1M);
> 
> If you write PHYS_OFFSET into PCI_PTBADDR1L, you don't need
> the #ifdef, and the code gets reusable on platforms that have
> yet another phys offset.

That wouldn't allow for the differences in size of DMA capable RAM. I know
it could be computed but I think this makes it clearer.

> > +	realview_pb_pcix_sync();
> > +	udelay(1500); /* Allow hardware to settle */
> 
> You should not need to delay for that long. What is the problem
> here? If the sync operation actually does a sync, you should not
> need to wait again, right?

This is copied from the NEC docs and example code. It isn't really a sync
(as in waiting for something) it's more a device to get the hardware into a
known state. It works and I don't have time to do the testing required if I
start changing things like that which could be critical.

> > +void realview_pb_pcix_set_attr(unsigned int tag)
> > +{
> > +	/* Get Device and Funcion number */
> > +	u32 data = readl(PCIX_UNIT_BASE + PCI_PCIXSTAT) & 0x0ff;
> > +	/* attribute set */
> > +	writel(tag | (data << 8), PCIX_UNIT_BASE + PCI_CFGATTR);
> > +}
> 
> Should this function be static? It appears to be only used in this
> file.
> > +int realview_pb_pcix_set_config(u32 bus, u32 dev, u32 func, int
> offset)

True, I thought I'd found all of those type of cases - fixed.

> > +{
> > +	u32 mode;
> > +	u32 config_addr;
> > +
> > +	writel(0x000307F7, PCIX_UNIT_BASE + PCI_PERRSTAT); /* clear error
> bit */
> > +	writew(0xfb30, PCIX_UNIT_BASE + PCI_STATUS);	   /* error bit
> clear */
> 
> I would guess that you need locking here, two drivers might
> simultaneously be
> accessing config space on different CPUs, or you might have kernel
> preemption
> enabled and get preempted between the register accesses.

Hmm... you could be correct, however we have done a lot of testing on SMP
systems with no problems. I'll have a chat with Catalin about this.

> Why are these functions called realview_pb_pci_* while the others are
> called
> realview_pb_pcix_*? I would expect that the pci name refers to the
> Xilinx
> pci bus while the pcix name is the NEC pci-x.

The original author probably cloned the pci code as a starting point. I've
fixed them.

> > +static int __init pci_realview_pb_setup(int nr, struct pci_sys_data
> *sys)
> > +{
> > +	if (machine_is_realview_pb11mp())
> > +		irq_gic_start = IRQ_PB11MP_GIC_START;
> > +	else
> > +		irq_gic_start = IRQ_PBA8_GIC_START;
> 
> Can you remove the board specific tests from the common file? Just pass
> the
> interrupt number through an init function that can get called by the
> board file, which will make reuse much easier if other platforms use
> the same hardware.

There will not be any more mach-realview board designs, certainly none that
use this root complex.

> > +	sys->io_offset = REALVIEW_PB_PCI_IO_BASE;
> 
> Why do you need to set io_offset?

I haven't investigated this. Is it something that is really wrong? Or just a
different way of doing things?

> Really no swizzling? That is very unusual, almost everyone needs to set
> pci_common_swizzle here to make bridges work.

No generic swizzling, it's done during the bus scan because it's very
non-standard due to the hardware design.

> > +static int __init realview_pb_pci_init(void)
> > +{
> > +	if (machine_is_realview_pb11mp() ||
> > +	    machine_is_realview_pba8() || machine_is_realview_pbx())
> > +		pci_common_init(&realview_pb_pci);
> > +	return 0;
> > +}
> > +
> > +subsys_initcall(realview_pb_pci_init);
> 
> Why not call this from the board file?

Don't know, as I said before I'm not the original author and we changed as
little as possible while getting it to work.

Regards,

Colin