[RFC] arm: versatile: enable PCI I/O space

[RFC] arm: versatile: enable PCI I/O space

[Arnd Bergmann]

I believe I have found out what is actually going on with PCI I/O
space mapping on the versatile platform, thanks to Catalin pointing
me in the right direction.

It seems that I/O space handling on the versatile platform is
currently broken in multiple ways. Most importantly, the ports do
not get mapped into the virtual address space at all.

Also, there is some amount of confusion between PCI I/O
space and other statically mapped MMIO registers in the
platform code:

* The __io_address() macro that is used to access the
  platform register maps to the same __io macro that gets
  used for I/O space.

* The IO_SPACE_LIMIT is set to a value that is much larger
  than the total available space.

* The I/O resource of the PCI bus is set to the physical
  address of the mapping, which is way outside of the
  actual I/O space limit as well as the address range that
  gets decoded by traditional PCI cards.

* No attempt is made to stay outside of the ISA port range
  that some device drivers try access.

* No resource gets requested as a child of ioport_resource,
  but an IORESOURCE_IO type mapping gets requested
  as a child of iomem_resource.

This patch attempts to correct all of the above. This makes
it possible to use virtio-pci based virtual devices as well
as actual PCI cards including those with legacy ISA port
ranges like VGA.

Some of the issues seem to be duplicated on other platforms
and if I got it right, we should probably do similar changes
there.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/arch/arm/mach-versatile/core.c b/arch/arm/mach-versatile/core.c
index 3dff864..27051a3 100644
--- a/arch/arm/mach-versatile/core.c
+++ b/arch/arm/mach-versatile/core.c
@@ -200,26 +200,13 @@ static struct map_desc versatile_io_desc[] __initdata = {
 		.pfn		= __phys_to_pfn(VERSATILE_PCI_CFG_BASE),
 		.length		= VERSATILE_PCI_CFG_BASE_SIZE,
 		.type		= MT_DEVICE
-	},
-#if 0
- 	{
+	}, {
 		.virtual	=  VERSATILE_PCI_VIRT_MEM_BASE0,
 		.pfn		= __phys_to_pfn(VERSATILE_PCI_MEM_BASE0),
 		.length		= SZ_16M,
 		.type		= MT_DEVICE
-	}, {
-		.virtual	=  VERSATILE_PCI_VIRT_MEM_BASE1,
-		.pfn		= __phys_to_pfn(VERSATILE_PCI_MEM_BASE1),
-		.length		= SZ_16M,
-		.type		= MT_DEVICE
-	}, {
-		.virtual	=  VERSATILE_PCI_VIRT_MEM_BASE2,
-		.pfn		= __phys_to_pfn(VERSATILE_PCI_MEM_BASE2),
-		.length		= SZ_16M,
-		.type		= MT_DEVICE
 	},
 #endif
-#endif
 };
 
 void __init versatile_map_io(void)
diff --git a/arch/arm/mach-versatile/include/mach/hardware.h b/arch/arm/mach-versatile/include/mach/hardware.h
index 4f8f99a..9b080ee 100644
--- a/arch/arm/mach-versatile/include/mach/hardware.h
+++ b/arch/arm/mach-versatile/include/mach/hardware.h
@@ -29,25 +29,16 @@
  */
 #define VERSATILE_PCI_VIRT_BASE		(void __iomem *)0xe8000000ul
 #define VERSATILE_PCI_CFG_VIRT_BASE	(void __iomem *)0xe9000000ul
+#define VERSATILE_PCI_VIRT_MEM_BASE0	(void __iomem *)0xeb000000ul
 
-#if 0
-#define VERSATILE_PCI_VIRT_MEM_BASE0	0xf4000000
-#define VERSATILE_PCI_VIRT_MEM_BASE1	0xf5000000
-#define VERSATILE_PCI_VIRT_MEM_BASE2	0xf6000000
-
-#define PCIO_BASE			VERSATILE_PCI_VIRT_MEM_BASE0
-#define PCIMEM_BASE			VERSATILE_PCI_VIRT_MEM_BASE1
-#endif
-
-/* CIK guesswork */
-#define PCIBIOS_MIN_IO			0x44000000
+#define PCIBIOS_MIN_IO			0x00001000
 #define PCIBIOS_MIN_MEM			0x50000000
 
 #define pcibios_assign_all_busses()     1
 
-/* macro to get at IO space when running virtually */
+/* macro to get at MMIO space when running virtually */
 #define IO_ADDRESS(x)		(((x) & 0x0fffffff) + (((x) >> 4) & 0x0f000000) + 0xf0000000)
 
-#define __io_address(n)		__io(IO_ADDRESS(n))
+#define __io_address(n)		__typesafe_io(IO_ADDRESS(n))
 
 #endif
diff --git a/arch/arm/mach-versatile/include/mach/io.h b/arch/arm/mach-versatile/include/mach/io.h
index f067c14..a276171 100644
--- a/arch/arm/mach-versatile/include/mach/io.h
+++ b/arch/arm/mach-versatile/include/mach/io.h
@@ -20,9 +20,12 @@
 #ifndef __ASM_ARM_ARCH_IO_H
 #define __ASM_ARM_ARCH_IO_H
 
-#define IO_SPACE_LIMIT 0xffffffff
+#include <mach/hardware.h>
+#include <mach/platform.h>
 
-#define __io(a)		__typesafe_io(a)
+#define IO_SPACE_LIMIT (VERSATILE_PCI_MEM_BASE0_SIZE - 1)
+
+#define __io(a)		(a + VERSATILE_PCI_VIRT_MEM_BASE0)
 #define __mem_pci(a)	(a)
 
 #endif
diff --git a/arch/arm/mach-versatile/pci.c b/arch/arm/mach-versatile/pci.c
index 334f0df..4630ff1 100644
--- a/arch/arm/mach-versatile/pci.c
+++ b/arch/arm/mach-versatile/pci.c
@@ -170,11 +170,18 @@ static struct pci_ops pci_versatile_ops = {
 	.write	= versatile_write_config,
 };
 
+static struct resource io_port = {
+	.name	= "PCI",
+	.start	= 0,
+	.end	= IO_SPACE_LIMIT,
+	.flags	= IORESOURCE_IO,
+};
+
 static struct resource io_mem = {
 	.name	= "PCI I/O space",
 	.start	= VERSATILE_PCI_MEM_BASE0,
 	.end	= VERSATILE_PCI_MEM_BASE0+VERSATILE_PCI_MEM_BASE0_SIZE-1,
-	.flags	= IORESOURCE_IO,
+	.flags	= IORESOURCE_MEM,
 };
 
 static struct resource non_mem = {
@@ -201,6 +208,12 @@ static int __init pci_versatile_setup_resources(struct resource **resource)
 		       "memory region (%d)\n", ret);
 		goto out;
 	}
+	ret = request_resource(&ioport_resource, &io_port);
+	if (ret) {
+		printk(KERN_ERR "PCI: unable to allocate I/O "
+		       "port region (%d)\n", ret);
+		goto out;
+	}
 	ret = request_resource(&iomem_resource, &non_mem);
 	if (ret) {
 		printk(KERN_ERR "PCI: unable to allocate non-prefetchable "
@@ -219,7 +232,7 @@ static int __init pci_versatile_setup_resources(struct resource **resource)
 	 * bus->resource[1] is the mem resource for this bus
 	 * bus->resource[2] is the prefetch mem resource for this bus
 	 */
-	resource[0] = &io_mem;
+	resource[0] = &io_port;
 	resource[1] = &non_mem;
 	resource[2] = &pre_mem;
 
@@ -250,6 +263,7 @@ int __init pci_versatile_setup(int nr, struct pci_sys_data *sys)
 
 	if (nr == 0) {
 		sys->mem_offset = 0;
+		sys->io_offset = 0;
 		ret = pci_versatile_setup_resources(sys->resource);
 		if (ret < 0) {
 			printk("pci_versatile_setup: resources... oops?\n");

RE: [RFC] arm: versatile: enable PCI I/O space

[Colin Tuckley]

> -----Original Message-----
> From: linux-arm-kernel-bounces at lists.infradead.org [mailto:linux-arm-
> kernel-bounces at lists.infradead.org] On Behalf Of Arnd Bergmann

> I believe I have found out what is actually going on with PCI I/O
> space mapping on the versatile platform, thanks to Catalin pointing
> me in the right direction.

These changes might also be applicable to PB1176 and EB which are built from the mach-realview sub-tree.

Note however that the pcix.c source file in that sub-tree is for the NEC Northbridge based boards (PB11MPCore, PBA8 and PBX). I'm not sure if the pci.c source file even exists in mach-realview.

Colin

--
Colin Tuckley - ARM Ltd.
110 Fulbourn Rd
Cambridge, CB1 9NJ
Tel: +44 1223 400536


-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium.  Thank you.

[RFC] arm: versatile: enable PCI I/O space

[Russell King - ARM Linux]

On Tue, Jul 20, 2010 at 11:23:38AM +0200, Arnd Bergmann wrote:
> -/* macro to get at IO space when running virtually */
> +/* macro to get at MMIO space when running virtually */
>  #define IO_ADDRESS(x)		(((x) & 0x0fffffff) + (((x) >> 4) & 0x0f000000) + 0xf0000000)
>  
> -#define __io_address(n)		__io(IO_ADDRESS(n))
> +#define __io_address(n)		__typesafe_io(IO_ADDRESS(n))

Hmm, that's not a scenario that I forsaw __typesafe_io() being used -
I'd prefer it to be restricted to only aliasing __io() to if it's
appropriate.  Either add an IOMEM() definition or open-code the cast
here.

> diff --git a/arch/arm/mach-versatile/include/mach/io.h b/arch/arm/mach-versatile/include/mach/io.h
> index f067c14..a276171 100644
> --- a/arch/arm/mach-versatile/include/mach/io.h
> +++ b/arch/arm/mach-versatile/include/mach/io.h
> @@ -20,9 +20,12 @@
>  #ifndef __ASM_ARM_ARCH_IO_H
>  #define __ASM_ARM_ARCH_IO_H
>  
> -#define IO_SPACE_LIMIT 0xffffffff
> +#include <mach/hardware.h>
> +#include <mach/platform.h>
>  
> -#define __io(a)		__typesafe_io(a)
> +#define IO_SPACE_LIMIT (VERSATILE_PCI_MEM_BASE0_SIZE - 1)
> +
> +#define __io(a)		(a + VERSATILE_PCI_VIRT_MEM_BASE0)

Not quite.  Have a look at arch/arm/mach-footbridge/include/mach/io.h to
see how __io is defined there.

[RFC] arm: versatile: enable PCI I/O space

[Arnd Bergmann]

On Tuesday 20 July 2010, Russell King - ARM Linux wrote:
> On Tue, Jul 20, 2010 at 11:23:38AM +0200, Arnd Bergmann wrote:
> > -/* macro to get at IO space when running virtually */
> > +/* macro to get at MMIO space when running virtually */
> >  #define IO_ADDRESS(x)		(((x) & 0x0fffffff) + (((x) >> 4) & 0x0f000000) + 0xf0000000)
> >  
> > -#define __io_address(n)		__io(IO_ADDRESS(n))
> > +#define __io_address(n)		__typesafe_io(IO_ADDRESS(n))
> 
> Hmm, that's not a scenario that I forsaw __typesafe_io() being used -
> I'd prefer it to be restricted to only aliasing __io() to if it's
> appropriate.  Either add an IOMEM() definition or open-code the cast
> here.

Yes, that's what I thought, but I didn't want to change too much.
Thanks for the confirmation. The same pattern is also present in
other platforms, I'll follow up with a patch to fix them all.

> > diff --git a/arch/arm/mach-versatile/include/mach/io.h b/arch/arm/mach-versatile/include/mach/io.h
> > index f067c14..a276171 100644
> > --- a/arch/arm/mach-versatile/include/mach/io.h
> > +++ b/arch/arm/mach-versatile/include/mach/io.h
> > @@ -20,9 +20,12 @@
> >  #ifndef __ASM_ARM_ARCH_IO_H
> >  #define __ASM_ARM_ARCH_IO_H
> >  
> > -#define IO_SPACE_LIMIT 0xffffffff
> > +#include <mach/hardware.h>
> > +#include <mach/platform.h>
> >  
> > -#define __io(a)		__typesafe_io(a)
> > +#define IO_SPACE_LIMIT (VERSATILE_PCI_MEM_BASE0_SIZE - 1)
> > +
> > +#define __io(a)		(a + VERSATILE_PCI_VIRT_MEM_BASE0)
> 
> Not quite.  Have a look at arch/arm/mach-footbridge/include/mach/io.h to
> see how __io is defined there.

The result is basically the same. My idea was to only define
VERSATILE_PCI_VIRT_MEM_BASE0 in one place and use it in both the place
where it get mapped and in the place where it gets used.

If I use the same definition as footbridge (which looks sensible,
#define __io(a)     ((void __iomem *)(PCIO_BASE + (a))) ),
should I do 

a)
mach-versatile/include/mach/io.h:
#define PCIO_BASE                ((unsigned long)VERSATILE_PCI_VIRT_MEM_BASE0)
mach-versatile/include/mach/hardware.h:
#define VERSATILE_PCI_VIRT_MEM_BASE0 ((void __iomem *)0xeb000000ul)

or b)
mach-versatile/include/mach/io.h:
#define PCIO_BASE                0xeb000000ul
mach-versatile/include/mach/hardware.h:
#define VERSATILE_PCI_VIRT_MEM_BASE0 ((void __iomem *)PCIO_BASE)


Thanks for the review!

	Arnd

[RFC] arm: versatile: enable PCI I/O space

[Eric Miao]

On Tue, Jul 20, 2010 at 11:23 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> I believe I have found out what is actually going on with PCI I/O
> space mapping on the versatile platform, thanks to Catalin pointing
> me in the right direction.
>
> It seems that I/O space handling on the versatile platform is
> currently broken in multiple ways. Most importantly, the ports do
> not get mapped into the virtual address space at all.
>
> Also, there is some amount of confusion between PCI I/O
> space and other statically mapped MMIO registers in the
> platform code:
>
> * The __io_address() macro that is used to access the
> ?platform register maps to the same __io macro that gets
> ?used for I/O space.
>
> * The IO_SPACE_LIMIT is set to a value that is much larger
> ?than the total available space.
>
> * The I/O resource of the PCI bus is set to the physical
> ?address of the mapping, which is way outside of the
> ?actual I/O space limit as well as the address range that
> ?gets decoded by traditional PCI cards.
>
> * No attempt is made to stay outside of the ISA port range
> ?that some device drivers try access.
>
> * No resource gets requested as a child of ioport_resource,
> ?but an IORESOURCE_IO type mapping gets requested
> ?as a child of iomem_resource.
>
> This patch attempts to correct all of the above. This makes
> it possible to use virtio-pci based virtual devices as well
> as actual PCI cards including those with legacy ISA port
> ranges like VGA.
>
> Some of the issues seem to be duplicated on other platforms
> and if I got it right, we should probably do similar changes
> there.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> diff --git a/arch/arm/mach-versatile/core.c b/arch/arm/mach-versatile/core.c
> index 3dff864..27051a3 100644
> --- a/arch/arm/mach-versatile/core.c
> +++ b/arch/arm/mach-versatile/core.c
> @@ -200,26 +200,13 @@ static struct map_desc versatile_io_desc[] __initdata = {
> ? ? ? ? ? ? ? ?.pfn ? ? ? ? ? ?= __phys_to_pfn(VERSATILE_PCI_CFG_BASE),
> ? ? ? ? ? ? ? ?.length ? ? ? ? = VERSATILE_PCI_CFG_BASE_SIZE,
> ? ? ? ? ? ? ? ?.type ? ? ? ? ? = MT_DEVICE
> - ? ? ? },
> -#if 0
> - ? ? ? {
> + ? ? ? }, {
> ? ? ? ? ? ? ? ?.virtual ? ? ? ?= ?VERSATILE_PCI_VIRT_MEM_BASE0,
> ? ? ? ? ? ? ? ?.pfn ? ? ? ? ? ?= __phys_to_pfn(VERSATILE_PCI_MEM_BASE0),
> ? ? ? ? ? ? ? ?.length ? ? ? ? = SZ_16M,
> ? ? ? ? ? ? ? ?.type ? ? ? ? ? = MT_DEVICE
> - ? ? ? }, {
> - ? ? ? ? ? ? ? .virtual ? ? ? ?= ?VERSATILE_PCI_VIRT_MEM_BASE1,
> - ? ? ? ? ? ? ? .pfn ? ? ? ? ? ?= __phys_to_pfn(VERSATILE_PCI_MEM_BASE1),
> - ? ? ? ? ? ? ? .length ? ? ? ? = SZ_16M,
> - ? ? ? ? ? ? ? .type ? ? ? ? ? = MT_DEVICE
> - ? ? ? }, {
> - ? ? ? ? ? ? ? .virtual ? ? ? ?= ?VERSATILE_PCI_VIRT_MEM_BASE2,
> - ? ? ? ? ? ? ? .pfn ? ? ? ? ? ?= __phys_to_pfn(VERSATILE_PCI_MEM_BASE2),
> - ? ? ? ? ? ? ? .length ? ? ? ? = SZ_16M,
> - ? ? ? ? ? ? ? .type ? ? ? ? ? = MT_DEVICE
> ? ? ? ?},
> ?#endif
> -#endif
> ?};
>
> ?void __init versatile_map_io(void)
> diff --git a/arch/arm/mach-versatile/include/mach/hardware.h b/arch/arm/mach-versatile/include/mach/hardware.h
> index 4f8f99a..9b080ee 100644
> --- a/arch/arm/mach-versatile/include/mach/hardware.h
> +++ b/arch/arm/mach-versatile/include/mach/hardware.h
> @@ -29,25 +29,16 @@
> ?*/
> ?#define VERSATILE_PCI_VIRT_BASE ? ? ? ? ? ? ? ?(void __iomem *)0xe8000000ul
> ?#define VERSATILE_PCI_CFG_VIRT_BASE ? ?(void __iomem *)0xe9000000ul
> +#define VERSATILE_PCI_VIRT_MEM_BASE0 ? (void __iomem *)0xeb000000ul
>
> -#if 0
> -#define VERSATILE_PCI_VIRT_MEM_BASE0 ? 0xf4000000
> -#define VERSATILE_PCI_VIRT_MEM_BASE1 ? 0xf5000000
> -#define VERSATILE_PCI_VIRT_MEM_BASE2 ? 0xf6000000
> -
> -#define PCIO_BASE ? ? ? ? ? ? ? ? ? ? ?VERSATILE_PCI_VIRT_MEM_BASE0
> -#define PCIMEM_BASE ? ? ? ? ? ? ? ? ? ?VERSATILE_PCI_VIRT_MEM_BASE1
> -#endif
> -
> -/* CIK guesswork */
> -#define PCIBIOS_MIN_IO ? ? ? ? ? ? ? ? 0x44000000
> +#define PCIBIOS_MIN_IO ? ? ? ? ? ? ? ? 0x00001000

I guess this is true for other sub-architectures. Currently no idea
why they are all using different values. But 0x1000 seems to be
good value and maybe we should default all of them to this and
move it into some common place instead of defining them every
where.

[RFC] arm: versatile: enable PCI I/O space

[Arnd Bergmann]

On Tuesday 20 July 2010, Eric Miao wrote:

> > -
> > -/* CIK guesswork */
> > -#define PCIBIOS_MIN_IO                 0x44000000
> > +#define PCIBIOS_MIN_IO                 0x00001000
> 
> I guess this is true for other sub-architectures. Currently no idea
> why they are all using different values. But 0x1000 seems to be
> good value and maybe we should default all of them to this and
> move it into some common place instead of defining them every
> where.

Yes. The 0x1000 is really the only sensible way to avoid the
legacy ISA ranges. Most platforms currently set this to zero,
which only works as long as you never try to load things like
an PC keyboard driver (including KDB) or an nvram driver that
probes the well-known ISA ports. The only reason I can see for
using less than 0x1000 is if you have a really small
IO_SPACE_LIMIT, but most platforms seem to set that to
0xffffffff, which is rather unrealistic if you use a memory
mapped IO region.

We also have two platforms (integrator and shark) that use a
higher value of 0x6000 but do not document why they do so.
iop13xx defines it as variable but never sets it to anything
else than zero.

	Arnd

[RFC] arm: versatile: enable PCI I/O space

[Russell King - ARM Linux]

On Tue, Jul 20, 2010 at 11:12:17PM +0200, Arnd Bergmann wrote:
> We also have two platforms (integrator and shark) that use a
> higher value of 0x6000 but do not document why they do so.

On Integrator, it's fairly arbitary, and could be 0x1000 too.