[PATCH 1/6] Realview PCIX support - add main support module code
Colin Tuckley
colin.tuckley at arm.com
Thu Oct 21 06:03:51 EDT 2010
> -----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
More information about the linux-arm-kernel
mailing list