[PATCH 0/3] ARM: PCI: versatile: fix irqs, I/O

[PATCH 0/3] ARM: PCI: versatile: fix irqs, I/O

[Peter Maydell]

These patches fix a number of issues with the PCI controller
code for mach-versatile:

(1) The irq mapping matched neither hardware nor QEMU; we correct
it to match the hardware, which means it will also work on recent
(1.5 or later) QEMU.

(2) The code was confused between the PCI I/O window (at 0x43000000)
and the first PCI memory window (at 0x44000000), which meant that
PCI devices using PCI PIO rather than MMIO didn't work. This is
fixed (and some variables/labels are renamed to avoid further
confusion in future).

(3) The SMAP register offsets were all off-by-four, though by
fluke this didn't actually have any ill effects.

All these changes have been tested on real hardware (PB926
plus the PCI backplane), as well as on QEMU. I have confirmed
that IRQs and PCI PIO and MMIO work OK. 

PCI bus-master DMA doesn't seem to work on h/w -- as far as I can
tell the device is correctly managing to DMA to the right places
in memory, but every other 32 bit word is corrupt (at least judging
from rtl8139 debug dumps of the frames it's receiving). I'm not
sure what's going on here, but since this is disjoint from
the irq and I/O issues I don't think that applying the
patches that fix those should be stalled on trying to debug
DMA problems. (DMA works fine on QEMU, incidentally.)

Patches based on mainline master.

Peter Maydell (3):
  ARM: PCI: versatile: Fix map_irq function to match hardware
  ARM: PCI: versatile: Fix PCI I/O
  ARM: PCI: versatile: Fix SMAP register offsets

 arch/arm/mach-versatile/include/mach/platform.h |    2 +
 arch/arm/mach-versatile/pci.c                   |   47 +++++++++++++++--------
 2 files changed, 32 insertions(+), 17 deletions(-)

-- 
1.7.9.5

[PATCH 1/3] ARM: PCI: versatile: Fix map_irq function to match hardware

[Peter Maydell]

The PCI controller code for the Versatile board has never had the
correct IRQ mapping for hardware.  For many years it had an odd
mapping ("all interrupts are int 27") which aligned with the
equivalent bug in QEMU.  However as of commit 1bc39ac5dab265
the mapping changed and no longer matched either hardware or QEMU,
with the result that any PCI card beyond the first in QEMU would
not have functioning interrupts; for example a boot with a SCSI
controller would time out as follows:

 ------------
 sym0: <895a> rev 0x0 at pci 0000:00:0d.0 irq 92
 sym0: SCSI BUS has been reset.
 scsi0 : sym-2.2.3
 [...]
 scsi 0:0:0:0: ABORT operation started
 scsi 0:0:0:0: ABORT operation timed-out.
 scsi 0:0:0:0: DEVICE RESET operation started
 scsi 0:0:0:0: DEVICE RESET operation timed-out.
 scsi 0:0:0:0: BUS RESET operation started
 scsi 0:0:0:0: BUS RESET operation timed-out.
 scsi 0:0:0:0: HOST RESET operation started
 sym0: SCSI BUS has been reset
 ------------

Fix the mapping so that it matches real hardware (checked against the
schematics for PB926 and backplane, and tested against the hardware).
This allows PCI cards using interrupts to work on hardware for the
first time; this change will also work with QEMU 1.5 or later, where
the equivalent bugs in the modelling of the hardware have been fixed.

Although QEMU will attempt to autodetect whether the kernel is
expecting the long-standing "everything is int 27" mapping or the one
hardware has, for certainty we force it into "definitely behave like
hardware mode"; this will avoid unexpected surprises later if we
implement sparse irqs. This is harmless on hardware.

Thanks to Paul Gortmaker for bisecting the problem and finding an initial
solution, to Russell King for providing the correct interrupt mapping,
and to Guenter Roeck for providing an initial version of this patch
and prodding me into relocating the hardware and retesting everything.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 arch/arm/mach-versatile/pci.c |   25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-versatile/pci.c b/arch/arm/mach-versatile/pci.c
index e92e5e0..234740d 100644
--- a/arch/arm/mach-versatile/pci.c
+++ b/arch/arm/mach-versatile/pci.c
@@ -295,6 +295,19 @@ int __init pci_versatile_setup(int nr, struct pci_sys_data *sys)
 	__raw_writel(PHYS_OFFSET, local_pci_cfg_base + PCI_BASE_ADDRESS_2);
 
 	/*
+	 * For many years the kernel and QEMU were symbiotically buggy
+	 * in that they both assumed the same broken IRQ mapping.
+	 * QEMU therefore attempts to auto-detect old broken kernels
+	 * so that they still work on newer QEMU as they did on old
+	 * QEMU. Since we now use the correct (ie matching-hardware)
+	 * IRQ mapping we write a definitely different value to a
+	 * PCI_INTERRUPT_LINE register to tell QEMU that we expect
+	 * real hardware behaviour and it need not be backwards
+	 * compatible for us. This write is harmless on real hardware.
+	 */
+	__raw_writel(0, VERSATILE_PCI_VIRT_BASE+PCI_INTERRUPT_LINE);
+
+	/*
 	 * Do not to map Versatile FPGA PCI device into memory space
 	 */
 	pci_slot_ignore |= (1 << myslot);
@@ -327,13 +340,13 @@ static int __init versatile_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
 {
 	int irq;
 
-	/* slot,  pin,	irq
-	 *  24     1     IRQ_SIC_PCI0
-	 *  25     1     IRQ_SIC_PCI1
-	 *  26     1     IRQ_SIC_PCI2
-	 *  27     1     IRQ_SIC_PCI3
+	/*
+	 * Slot	INTA	INTB	INTC	INTD
+	 * 31	PCI1	PCI2	PCI3	PCI0
+	 * 30	PCI0	PCI1	PCI2	PCI3
+	 * 29	PCI3	PCI0	PCI1	PCI2
 	 */
-	irq = IRQ_SIC_PCI0 + ((slot - 24 + pin - 1) & 3);
+	irq = IRQ_SIC_PCI0 + ((slot + 2 + pin - 1) & 3);
 
 	return irq;
 }
-- 
1.7.9.5

[PATCH 1/3] ARM: PCI: versatile: Fix map_irq function to match hardware

[Guenter Roeck]

On Thu, Aug 22, 2013 at 05:47:48PM +0100, Peter Maydell wrote:
> The PCI controller code for the Versatile board has never had the
> correct IRQ mapping for hardware.  For many years it had an odd
> mapping ("all interrupts are int 27") which aligned with the
> equivalent bug in QEMU.  However as of commit 1bc39ac5dab265
> the mapping changed and no longer matched either hardware or QEMU,
> with the result that any PCI card beyond the first in QEMU would
> not have functioning interrupts; for example a boot with a SCSI
> controller would time out as follows:
> 
>  ------------
>  sym0: <895a> rev 0x0 at pci 0000:00:0d.0 irq 92
>  sym0: SCSI BUS has been reset.
>  scsi0 : sym-2.2.3
>  [...]
>  scsi 0:0:0:0: ABORT operation started
>  scsi 0:0:0:0: ABORT operation timed-out.
>  scsi 0:0:0:0: DEVICE RESET operation started
>  scsi 0:0:0:0: DEVICE RESET operation timed-out.
>  scsi 0:0:0:0: BUS RESET operation started
>  scsi 0:0:0:0: BUS RESET operation timed-out.
>  scsi 0:0:0:0: HOST RESET operation started
>  sym0: SCSI BUS has been reset
>  ------------
> 
> Fix the mapping so that it matches real hardware (checked against the
> schematics for PB926 and backplane, and tested against the hardware).
> This allows PCI cards using interrupts to work on hardware for the
> first time; this change will also work with QEMU 1.5 or later, where
> the equivalent bugs in the modelling of the hardware have been fixed.
> 
> Although QEMU will attempt to autodetect whether the kernel is
> expecting the long-standing "everything is int 27" mapping or the one
> hardware has, for certainty we force it into "definitely behave like
> hardware mode"; this will avoid unexpected surprises later if we
> implement sparse irqs. This is harmless on hardware.
> 
> Thanks to Paul Gortmaker for bisecting the problem and finding an initial
> solution, to Russell King for providing the correct interrupt mapping,
> and to Guenter Roeck for providing an initial version of this patch
> and prodding me into relocating the hardware and retesting everything.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Tested-by: Guenter Roeck <linux@roeck-us.net>

Patches applied to 3.11-rc6 and 3.10.9, loaded qemu and verified that
access to SCSI controller is working. qemu version 1.5.2.

Guenter

[PATCH 1/3] ARM: PCI: versatile: Fix map_irq function to match hardware

[Linus Walleij]

On Thu, Aug 22, 2013 at 6:47 PM, Peter Maydell <peter.maydell@linaro.org> wrote:

> The PCI controller code for the Versatile board has never had the
> correct IRQ mapping for hardware.  For many years it had an odd
> mapping ("all interrupts are int 27") which aligned with the
> equivalent bug in QEMU.  However as of commit 1bc39ac5dab265
> the mapping changed and no longer matched either hardware or QEMU,
> with the result that any PCI card beyond the first in QEMU would
> not have functioning interrupts; for example a boot with a SCSI
> controller would time out as follows:
>
>  ------------
>  sym0: <895a> rev 0x0 at pci 0000:00:0d.0 irq 92
>  sym0: SCSI BUS has been reset.
>  scsi0 : sym-2.2.3
>  [...]
>  scsi 0:0:0:0: ABORT operation started
>  scsi 0:0:0:0: ABORT operation timed-out.
>  scsi 0:0:0:0: DEVICE RESET operation started
>  scsi 0:0:0:0: DEVICE RESET operation timed-out.
>  scsi 0:0:0:0: BUS RESET operation started
>  scsi 0:0:0:0: BUS RESET operation timed-out.
>  scsi 0:0:0:0: HOST RESET operation started
>  sym0: SCSI BUS has been reset
>  ------------
>
> Fix the mapping so that it matches real hardware (checked against the
> schematics for PB926 and backplane, and tested against the hardware).
> This allows PCI cards using interrupts to work on hardware for the
> first time; this change will also work with QEMU 1.5 or later, where
> the equivalent bugs in the modelling of the hardware have been fixed.
>
> Although QEMU will attempt to autodetect whether the kernel is
> expecting the long-standing "everything is int 27" mapping or the one
> hardware has, for certainty we force it into "definitely behave like
> hardware mode"; this will avoid unexpected surprises later if we
> implement sparse irqs. This is harmless on hardware.
>
> Thanks to Paul Gortmaker for bisecting the problem and finding an initial
> solution, to Russell King for providing the correct interrupt mapping,
> and to Guenter Roeck for providing an initial version of this patch
> and prodding me into relocating the hardware and retesting everything.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 2/3] ARM: PCI: versatile: Fix PCI I/O

[Peter Maydell]

The versatile PCI controller code was confused between the
PCI I/O window (at 0x43000000) and the first PCI memory
window (at 0x44000000). Pass the correct base address to
pci_remap_io() so that PCI I/O accesses work.

Since the first PCI memory window isn't used at all (it's
an odd size), rename the associated variables and labels
so that it's clear that it isn't related to the I/O window.

This has been tested and confirmed to fix PCI I/O accesses
both on physical PB926+PCI backplane hardware and on QEMU.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 arch/arm/mach-versatile/include/mach/platform.h |    2 ++
 arch/arm/mach-versatile/pci.c                   |   16 ++++++++--------
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mach-versatile/include/mach/platform.h b/arch/arm/mach-versatile/include/mach/platform.h
index ec08740..6f938cc 100644
--- a/arch/arm/mach-versatile/include/mach/platform.h
+++ b/arch/arm/mach-versatile/include/mach/platform.h
@@ -231,12 +231,14 @@
 /* PCI space */
 #define VERSATILE_PCI_BASE             0x41000000	/* PCI Interface */
 #define VERSATILE_PCI_CFG_BASE	       0x42000000
+#define VERSATILE_PCI_IO_BASE          0x43000000
 #define VERSATILE_PCI_MEM_BASE0        0x44000000
 #define VERSATILE_PCI_MEM_BASE1        0x50000000
 #define VERSATILE_PCI_MEM_BASE2        0x60000000
 /* Sizes of above maps */
 #define VERSATILE_PCI_BASE_SIZE	       0x01000000
 #define VERSATILE_PCI_CFG_BASE_SIZE    0x02000000
+#define VERSATILE_PCI_IO_BASE_SIZE     0x01000000
 #define VERSATILE_PCI_MEM_BASE0_SIZE   0x0c000000	/* 32Mb */
 #define VERSATILE_PCI_MEM_BASE1_SIZE   0x10000000	/* 256Mb */
 #define VERSATILE_PCI_MEM_BASE2_SIZE   0x10000000	/* 256Mb */
diff --git a/arch/arm/mach-versatile/pci.c b/arch/arm/mach-versatile/pci.c
index 234740d..1e48878 100644
--- a/arch/arm/mach-versatile/pci.c
+++ b/arch/arm/mach-versatile/pci.c
@@ -170,8 +170,8 @@ static struct pci_ops pci_versatile_ops = {
 	.write	= versatile_write_config,
 };
 
-static struct resource io_mem = {
-	.name	= "PCI I/O space",
+static struct resource unused_mem = {
+	.name	= "PCI unused",
 	.start	= VERSATILE_PCI_MEM_BASE0,
 	.end	= VERSATILE_PCI_MEM_BASE0+VERSATILE_PCI_MEM_BASE0_SIZE-1,
 	.flags	= IORESOURCE_MEM,
@@ -195,9 +195,9 @@ static int __init pci_versatile_setup_resources(struct pci_sys_data *sys)
 {
 	int ret = 0;
 
-	ret = request_resource(&iomem_resource, &io_mem);
+	ret = request_resource(&iomem_resource, &unused_mem);
 	if (ret) {
-		printk(KERN_ERR "PCI: unable to allocate I/O "
+		printk(KERN_ERR "PCI: unable to allocate unused "
 		       "memory region (%d)\n", ret);
 		goto out;
 	}
@@ -205,7 +205,7 @@ static int __init pci_versatile_setup_resources(struct pci_sys_data *sys)
 	if (ret) {
 		printk(KERN_ERR "PCI: unable to allocate non-prefetchable "
 		       "memory region (%d)\n", ret);
-		goto release_io_mem;
+		goto release_unused_mem;
 	}
 	ret = request_resource(&iomem_resource, &pre_mem);
 	if (ret) {
@@ -225,8 +225,8 @@ static int __init pci_versatile_setup_resources(struct pci_sys_data *sys)
 
  release_non_mem:
 	release_resource(&non_mem);
- release_io_mem:
-	release_resource(&io_mem);
+ release_unused_mem:
+	release_resource(&unused_mem);
  out:
 	return ret;
 }
@@ -246,7 +246,7 @@ int __init pci_versatile_setup(int nr, struct pci_sys_data *sys)
 		goto out;
 	}
 
-	ret = pci_ioremap_io(0, VERSATILE_PCI_MEM_BASE0);
+	ret = pci_ioremap_io(0, VERSATILE_PCI_IO_BASE);
 	if (ret)
 		goto out;
 
-- 
1.7.9.5

[PATCH 2/3] ARM: PCI: versatile: Fix PCI I/O

[Linus Walleij]

On Thu, Aug 22, 2013 at 6:47 PM, Peter Maydell <peter.maydell@linaro.org> wrote:

> The versatile PCI controller code was confused between the
> PCI I/O window (at 0x43000000) and the first PCI memory
> window (at 0x44000000). Pass the correct base address to
> pci_remap_io() so that PCI I/O accesses work.
>
> Since the first PCI memory window isn't used at all (it's
> an odd size), rename the associated variables and labels
> so that it's clear that it isn't related to the I/O window.
>
> This has been tested and confirmed to fix PCI I/O accesses
> both on physical PB926+PCI backplane hardware and on QEMU.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 3/3] ARM: PCI: versatile: Fix SMAP register offsets

[Peter Maydell]

The SMAP register offsets in the versatile PCI controller code were
all off by four.  (This didn't have any observable bad effects
because on this board PHYS_OFFSET is zero, and (a) writing zero to
the flags register at offset 0x10 has no effect and (b) the reset
value of the SMAP register is zero anyway, so failing to write SMAP2
didn't matter.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 arch/arm/mach-versatile/pci.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-versatile/pci.c b/arch/arm/mach-versatile/pci.c
index 1e48878..c97be4e 100644
--- a/arch/arm/mach-versatile/pci.c
+++ b/arch/arm/mach-versatile/pci.c
@@ -43,9 +43,9 @@
 #define PCI_IMAP0		__IO_ADDRESS(VERSATILE_PCI_CORE_BASE+0x0)
 #define PCI_IMAP1		__IO_ADDRESS(VERSATILE_PCI_CORE_BASE+0x4)
 #define PCI_IMAP2		__IO_ADDRESS(VERSATILE_PCI_CORE_BASE+0x8)
-#define PCI_SMAP0		__IO_ADDRESS(VERSATILE_PCI_CORE_BASE+0x10)
-#define PCI_SMAP1		__IO_ADDRESS(VERSATILE_PCI_CORE_BASE+0x14)
-#define PCI_SMAP2		__IO_ADDRESS(VERSATILE_PCI_CORE_BASE+0x18)
+#define PCI_SMAP0		__IO_ADDRESS(VERSATILE_PCI_CORE_BASE+0x14)
+#define PCI_SMAP1		__IO_ADDRESS(VERSATILE_PCI_CORE_BASE+0x18)
+#define PCI_SMAP2		__IO_ADDRESS(VERSATILE_PCI_CORE_BASE+0x1c)
 #define PCI_SELFID		__IO_ADDRESS(VERSATILE_PCI_CORE_BASE+0xc)
 
 #define DEVICE_ID_OFFSET		0x00
-- 
1.7.9.5

[PATCH 3/3] ARM: PCI: versatile: Fix SMAP register offsets

[Linus Walleij]

On Thu, Aug 22, 2013 at 6:47 PM, Peter Maydell <peter.maydell@linaro.org> wrote:

> The SMAP register offsets in the versatile PCI controller code were
> all off by four.  (This didn't have any observable bad effects
> because on this board PHYS_OFFSET is zero, and (a) writing zero to
> the flags register at offset 0x10 has no effect and (b) the reset
> value of the SMAP register is zero anyway, so failing to write SMAP2
> didn't matter.)
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 0/3] ARM: PCI: versatile: fix irqs, I/O

[Linus Walleij]

On Thu, Aug 22, 2013 at 6:47 PM, Peter Maydell <peter.maydell@linaro.org> wrote:

> These patches fix a number of issues with the PCI controller
> code for mach-versatile:

All seem sound to me.

Please mark all as Cc: stable at vger.kernel.org
and push the ARM SoC maintainers to take these three
patches as fixes.

Yours,
Linus Walleij

[PATCH 0/3] ARM: PCI: versatile: fix irqs, I/O

[Kevin Hilman]

Linus Walleij <linus.walleij@linaro.org> writes:

> On Thu, Aug 22, 2013 at 6:47 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>
>> These patches fix a number of issues with the PCI controller
>> code for mach-versatile:
>
> All seem sound to me.
>
> Please mark all as Cc: stable at vger.kernel.org
> and push the ARM SoC maintainers to take these three
> patches as fixes.

I've picked these up and added the Reviewed-by from Linus and the Cc:
stable.

They will not likely get into the v3.12 merge window, but will get them
in during the v3.12-rc

Thanks for the fixes,

Kevin

[PATCH 0/3] ARM: PCI: versatile: fix irqs, I/O

[Olof Johansson]

On Wed, Sep 4, 2013 at 2:58 PM, Kevin Hilman <khilman@linaro.org> wrote:
> Linus Walleij <linus.walleij@linaro.org> writes:
>
>> On Thu, Aug 22, 2013 at 6:47 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>>> These patches fix a number of issues with the PCI controller
>>> code for mach-versatile:
>>
>> All seem sound to me.
>>
>> Please mark all as Cc: stable at vger.kernel.org
>> and push the ARM SoC maintainers to take these three
>> patches as fixes.
>
> I've picked these up and added the Reviewed-by from Linus and the Cc:
> stable.
>
> They will not likely get into the v3.12 merge window, but will get them
> in during the v3.12-rc

I've merged this into fixes now for 3.12.


-Olof