lkml.org 
[lkml]   [2010]   [Nov]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] ARM: Gemini: Add support for PCI BUS
On 11/28/2010 09:56 PM, Arnd Bergmann wrote:
> On Saturday 27 November 2010 13:24:35 Hans Ulli Kroll wrote:
>> +#define PCI_IOSIZE_REG (IO_ADDRESS(GEMINI_PCI_IO_BASE))
>> +#define PCI_PROT_REG (IO_ADDRESS(GEMINI_PCI_IO_BASE) + 0x04)
>> +#define PCI_CTRL_REG (IO_ADDRESS(GEMINI_PCI_IO_BASE) + 0x08)
>> +#define PCI_SOFTRST_REG (IO_ADDRESS(GEMINI_PCI_IO_BASE) + 0x10)
>> +#define PCI_CONFIG_REG (IO_ADDRESS(GEMINI_PCI_IO_BASE) + 0x28)
>> +#define PCI_DATA_REG (IO_ADDRESS(GEMINI_PCI_IO_BASE) + 0x2C)
>
> If you use the virtual address of the mapping instead of
> GEMINI_PCI_IO_BASE, you don't need to repeat the IO_ADDRESS()
> macro everywhere. I have a patch that gets rid of all the
> conflicting definitions of this macro because it breaks
> a multi-platform build once we get there.
>
>> +static DEFINE_SPINLOCK(gemini_pci_lock);
>> +
>> +static struct resource gemini_pci_resource_io = {
>> + .name = "PCI I/O Space",
>> + .start = IO_ADDRESS(GEMINI_PCI_IO_BASE),
>> + .end = IO_ADDRESS(GEMINI_PCI_IO_BASE) + SZ_1M - 1,
>> + .flags = IORESOURCE_IO,
>> +};
>> +
>
> This looks wrong in multiple ways:
>
> * resources are physical addresses, not virtual addresses
> * GEMINI_PCI_IO_BASE is an address in memory space, so it
> needs to be IORESOURCE_MEM, not IORESOURCE_IO. You can
> also register the IORESOURCE_IO resource, but that would
> be .start=PCIBIOS_MIN_IO, .end=IO_SPACE_LIMIT.
> * IO_SPACE_LIMIT is larger than the I/O window, which can
> cause overflows. Setting it to 0xffff is generally enough.
>
>> + spin_lock_irqsave(&gemini_pci_lock, irq_flags);
>> +
>> + __raw_writel(PCI_CONF_BUS(bus->number) |
>> + PCI_CONF_DEVICE(PCI_SLOT(fn)) |
>> + PCI_CONF_FUNCTION(PCI_FUNC(fn)) |
>> + PCI_CONF_WHERE(config) |
>> + PCI_CONF_ENABLE,
>> + PCI_CONFIG_REG);
>> +
>> + switch (size) {
>> + case 4:
>> + __raw_writel(value, PCI_DATA_REG);
>> + break;
>> + case 2:
>> + __raw_writew(value, PCI_DATA_REG + (config& 3));
>> + break;
>> + case 1:
>> + __raw_writeb(value, PCI_DATA_REG + (config& 3));
>> + break;
>> + default:
>> + ret = PCIBIOS_BAD_REGISTER_NUMBER;
>> + }
>> +
>> + spin_unlock_irqrestore(&gemini_pci_lock, irq_flags);
>
> The I/O ordering is probably not what you think it is.
> There is no ordering guarantee between __raw_writel and
> spin_lock/spin_unlock, so you really should be using
> readl/writel.

No he really should NOT use readl/writel. The ONLY difference
between readl/writel and __raw_readl/__raw_writel is endianess
conversion. __raw_*l is not doing it. Which to use depend only
on HW.

> Note that the pci_ops are called under another spinlock, so
> you also don't need to take gemini_pci_lock here.
>
> Arnd



\
 
 \ /
  Last update: 2010-11-29 17:13    [W:0.072 / U:8.264 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site