IXP4xx: Indirect PCI MMIO compile failure

IXP4xx: Indirect PCI MMIO compile failure

[Krzysztof Halasa]

Hi,

Guess nobody uses indirect PCI MMIO on IXP4xx - since 2.6.29:
arch/arm/mach-ixp4xx/include/mach/io.h:70: error: 'VMALLOC_START' undeclared

(and so on)

That's a part of commit 4b78a9ffabbb03af4032ff704689912298e19070, dated
2008-11-29. Perhaps we should get rid of this hack (indirect MMIO) if
nobody uses it? 64 MB of MMIO PCI space should be more than enough for
everyone :-)

Since VMALLOC_START wants high_memory, I guess the correct patch is
(attached) and that's what I'm going to use if nobody comes with a
better idea.

--- a/arch/arm/mach-ixp4xx/include/mach/io.h
+++ b/arch/arm/mach-ixp4xx/include/mach/io.h
@@ -14,7 +14,7 @@
 #define __ASM_ARM_ARCH_IO_H
 
 #include <linux/bitops.h>
-
+#include <linux/mm.h>
 #include <mach/hardware.h>
 
 #define IO_SPACE_LIMIT 0x0000ffff

-- 
Krzysztof Halasa

IXP4xx: Indirect PCI MMIO compile failure

[Russell King - ARM Linux]

On Sat, Nov 14, 2009 at 09:15:54PM +0100, Krzysztof Halasa wrote:
> Guess nobody uses indirect PCI MMIO on IXP4xx - since 2.6.29:
> arch/arm/mach-ixp4xx/include/mach/io.h:70: error: 'VMALLOC_START' undeclared
> 
> (and so on)
> 
> That's a part of commit 4b78a9ffabbb03af4032ff704689912298e19070, dated
> 2008-11-29. Perhaps we should get rid of this hack (indirect MMIO) if
> nobody uses it? 64 MB of MMIO PCI space should be more than enough for
> everyone :-)
> 
> Since VMALLOC_START wants high_memory, I guess the correct patch is
> (attached) and that's what I'm going to use if nobody comes with a
> better idea.

This is rather horrible, linux/mm.h is one of those headers which drag
in lots of other headers, and we're risking horrible include loops if we
do this.  (while ifdefs give some protection against those running away,
it does mean that we lose ordering of definitions - which can and does
lead to indeterminent behaviour.)

Probably the best solution is to move __ixp4xx_iounmap out of the header
files.

IXP4xx: Indirect PCI MMIO compile failure

[Krzysztof Halasa]

Russell King - ARM Linux <linux@arm.linux.org.uk> writes:

> This is rather horrible, linux/mm.h is one of those headers which drag
> in lots of other headers, and we're risking horrible include loops if we
> do this.  (while ifdefs give some protection against those running away,
> it does mean that we lose ordering of definitions - which can and does
> lead to indeterminent behaviour.)

Right, that's bad.

I wonder why is this ioremap() etc. code interested in VMALLOC_START at all.
Perhaps reversing the test would make more sense?

Something like:

--- a/arch/arm/mach-ixp4xx/include/mach/io.h
+++ b/arch/arm/mach-ixp4xx/include/mach/io.h
@@ -55,10 +55,16 @@ extern int ixp4xx_pci_write(u32 addr, u32 cmd, u32 data);
  * access registers. If something outside of PCI is ioremap'd, we
  * fallback to the default.
  */
+
+static inline int is_pci_memory(u32 addr)
+{
+	return (addr >= PCIBIOS_MIN_MEM) && (addr <= 0x4FFFFFFF);
+}
+
 static inline void __iomem * __indirect_ioremap(unsigned long addr, size_t size,
 						unsigned int mtype)
 {
-	if((addr < PCIBIOS_MIN_MEM) || (addr > 0x4fffffff))
+	if (!is_pci_memory(addr))
 		return __arm_ioremap(addr, size, mtype);
 
 	return (void __iomem *)addr;
@@ -66,7 +72,7 @@ static inline void __iomem * __indirect_ioremap(unsigned long addr, size_t size,
 
 static inline void __indirect_iounmap(void __iomem *addr)
 {
-	if ((__force u32)addr >= VMALLOC_START)
+	if (!is_pci_memory(__force u32)addr)
 		__iounmap(addr);
 }
 

(and so on)
-- 
Krzysztof Halasa

IXP4xx: Indirect PCI MMIO compile failure

[Russell King - ARM Linux]

On Sun, Nov 15, 2009 at 12:02:15AM +0100, Krzysztof Halasa wrote:
> Russell King - ARM Linux <linux@arm.linux.org.uk> writes:
> 
> > This is rather horrible, linux/mm.h is one of those headers which drag
> > in lots of other headers, and we're risking horrible include loops if we
> > do this.  (while ifdefs give some protection against those running away,
> > it does mean that we lose ordering of definitions - which can and does
> > lead to indeterminent behaviour.)
> 
> Right, that's bad.
> 
> I wonder why is this ioremap() etc. code interested in VMALLOC_START at all.
> Perhaps reversing the test would make more sense?

You're confusing virtual addresses with pci addresses, these are not the
same thing.

> +
> +static inline int is_pci_memory(u32 addr)
> +{
> +	return (addr >= PCIBIOS_MIN_MEM) && (addr <= 0x4FFFFFFF);
> +}
> +
> @@ -66,7 +72,7 @@ static inline void __iomem * __indirect_ioremap(unsigned long addr, size_t size,
>  
>  static inline void __indirect_iounmap(void __iomem *addr)
>  {
> -	if ((__force u32)addr >= VMALLOC_START)
> +	if (!is_pci_memory(__force u32)addr)

So here you're testing a virtual address against a pci address.

IXP4xx: Indirect PCI MMIO compile failure

[Krzysztof Halasa]

Russell King - ARM Linux <linux@arm.linux.org.uk> writes:

>>  static inline void __indirect_iounmap(void __iomem *addr)
>>  {
>> -	if ((__force u32)addr >= VMALLOC_START)
>> +	if (!is_pci_memory(__force u32)addr)
>
> So here you're testing a virtual address against a pci address.

There is 1:1 mapping on IXP4xx with indirect PCI MMIO:

static inline void __iomem *
__ixp4xx_ioremap(unsigned long addr, size_t size, unsigned int mtype)
{
        if((addr < PCIBIOS_MIN_MEM) || (addr > 0x4fffffff))
                return __arm_ioremap(addr, size, mtype);

        return (void __iomem *)addr;
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
}

The __iomem *addr is not really a pointer, it's just a cookie which can
only be passed to the indirect routines.
-- 
Krzysztof Halasa

IXP4xx: Indirect PCI MMIO compile failure and fix

[Krzysztof Halasa]

I'm attaching the complete patch, it seems to work on my IXP425 with
indirect PCI, the only PCI device is SIL3512 SATA mini-PCI card.

That #include <linux/mm.h> wasn't a bright idea :-(

Comments?

--- a/arch/arm/mach-ixp4xx/include/mach/io.h
+++ b/arch/arm/mach-ixp4xx/include/mach/io.h
@@ -55,10 +55,16 @@ extern int ixp4xx_pci_write(u32 addr, u32 cmd, u32 data);
  * access registers. If something outside of PCI is ioremap'd, we
  * fallback to the default.
  */
+
+static inline int is_pci_memory(u32 addr)
+{
+	return (addr >= PCIBIOS_MIN_MEM) && (addr <= 0x4FFFFFFF);
+}
+
 static inline void __iomem * __indirect_ioremap(unsigned long addr, size_t size,
 						unsigned int mtype)
 {
-	if((addr < PCIBIOS_MIN_MEM) || (addr > 0x4fffffff))
+	if (!is_pci_memory(addr))
 		return __arm_ioremap(addr, size, mtype);
 
 	return (void __iomem *)addr;
@@ -66,7 +72,7 @@ static inline void __iomem * __indirect_ioremap(unsigned long addr, size_t size,
 
 static inline void __indirect_iounmap(void __iomem *addr)
 {
-	if ((__force u32)addr >= VMALLOC_START)
+	if (!is_pci_memory((__force u32)addr))
 		__iounmap(addr);
 }
 
@@ -94,7 +100,7 @@ static inline void __indirect_writeb(u8 value, volatile void __iomem *p)
 	u32 addr = (u32)p;
 	u32 n, byte_enables, data;
 
-	if (addr >= VMALLOC_START) {
+	if (!is_pci_memory(addr)) {
 		__raw_writeb(value, addr);
 		return;
 	}
@@ -117,7 +123,7 @@ static inline void __indirect_writew(u16 value, volatile void __iomem *p)
 	u32 addr = (u32)p;
 	u32 n, byte_enables, data;
 
-	if (addr >= VMALLOC_START) {
+	if (!is_pci_memory(addr)) {
 		__raw_writew(value, addr);
 		return;
 	}
@@ -138,7 +144,8 @@ static inline void __indirect_writesw(volatile void __iomem *bus_addr,
 static inline void __indirect_writel(u32 value, volatile void __iomem *p)
 {
 	u32 addr = (__force u32)p;
-	if (addr >= VMALLOC_START) {
+
+	if (!is_pci_memory(addr)) {
 		__raw_writel(value, p);
 		return;
 	}
@@ -158,7 +165,7 @@ static inline unsigned char __indirect_readb(const volatile void __iomem *p)
 	u32 addr = (u32)p;
 	u32 n, byte_enables, data;
 
-	if (addr >= VMALLOC_START)
+	if (!is_pci_memory(addr))
 		return __raw_readb(addr);
 
 	n = addr % 4;
@@ -181,7 +188,7 @@ static inline unsigned short __indirect_readw(const volatile void __iomem *p)
 	u32 addr = (u32)p;
 	u32 n, byte_enables, data;
 
-	if (addr >= VMALLOC_START)
+	if (!is_pci_memory(addr))
 		return __raw_readw(addr);
 
 	n = addr % 4;
@@ -204,7 +211,7 @@ static inline unsigned long __indirect_readl(const volatile void __iomem *p)
 	u32 addr = (__force u32)p;
 	u32 data;
 
-	if (addr >= VMALLOC_START)
+	if (!is_pci_memory(addr))
 		return __raw_readl(p);
 
 	if (ixp4xx_pci_read(addr, NP_CMD_MEMREAD, &data))

-- 
Krzysztof Halasa

IXP4xx: Indirect PCI MMIO compile failure

[Krzysztof Halasa]

Krzysztof Halasa <khc@pm.waw.pl> writes:

> There is 1:1 mapping on IXP4xx with indirect PCI MMIO:
>
> static inline void __iomem *
> __ixp4xx_ioremap(unsigned long addr, size_t size, unsigned int mtype)
> {
>         if((addr < PCIBIOS_MIN_MEM) || (addr > 0x4fffffff))
>                 return __arm_ioremap(addr, size, mtype);
>
>         return (void __iomem *)addr;
>         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Well, the 128 MB limit of indirect MMIO addressing seems to be bogus.
I can't find anything about it in the Intel's manual (except that PCI
address space is 0x48000000-0x4FFFFFFF and 0x48000000-0x4BFFFFFF is
actually usable - in direct mode). Added a patch:

--- a/arch/arm/mach-ixp4xx/include/mach/hardware.h
+++ b/arch/arm/mach-ixp4xx/include/mach/hardware.h
@@ -18,7 +18,11 @@
 #define __ASM_ARCH_HARDWARE_H__
 
 #define PCIBIOS_MIN_IO		0x00001000
-#define PCIBIOS_MIN_MEM		(cpu_is_ixp43x() ? 0x40000000 : 0x48000000)
+#ifdef CONFIG_IXP4XX_INDIRECT_PCI
+#define PCIBIOS_MIN_MEM		0x10000000
+#else
+#define PCIBIOS_MIN_MEM		0x48000000
+#endif
 
 /*
  * We override the standard dma-mask routines for bouncing.


And:
# lspci -v
00:0d.0 RAID bus controller: Silicon Image, Inc. SiI 3512 [SATALink/SATARaid] Se
rial ATA Controller (rev 01)
        Subsystem: Silicon Image, Inc. SiI 3512 SATARaid Controller
        Flags: bus master, 66MHz, medium devsel, latency 0, IRQ 29
        I/O ports at 1010 [size=8]
        I/O ports at 1020 [size=4]
        I/O ports at 1018 [size=8]
        I/O ports at 1024 [size=4]
        I/O ports at 1000 [size=16]
        Memory at 10080000 (32-bit, non-prefetchable) [size=512]
                  ^^^^^^^^
        [virtual] Expansion ROM at 10000000 [disabled] [size=512K]

Needless to say, it works.
-- 
Krzysztof Halasa

IXP4xx: Indirect PCI MMIO compile failure

[Krzysztof Halasa]

> Well, the 128 MB limit of indirect MMIO addressing seems to be bogus.
> I can't find anything about it in the Intel's manual (except that PCI
> address space is 0x48000000-0x4FFFFFFF and 0x48000000-0x4BFFFFFF is
> actually usable - in direct mode). Added a patch:

The full patch follows. It seems all IXP4xx CPUs can indirectly access
the whole 4 GB PCI MMIO address space (using the non-prefetch
registers). This is explicitly stated in IXP43x manual, and I've
verified that it works on IXP425.

Picked up 0x10000000 starting address to have 1 GB of PCI address space
available (there is no cost associated with it and since that IXP43x
devel board with its media-something device needed 256 MB or so...)

Probably a mixed direct/indirect access (the first 64 MB direct and the
rest indirect) would be a bit better, but if nobody uses it, I won't
bother.

Comments?

--- a/arch/arm/mach-ixp4xx/Kconfig
+++ b/arch/arm/mach-ixp4xx/Kconfig
@@ -179,7 +179,7 @@ config IXP4XX_INDIRECT_PCI
 	help
           IXP4xx provides two methods of accessing PCI memory space:
 
-          1) A direct mapped window from 0x48000000 to 0x4bffffff (64MB).
+          1) A direct mapped window from 0x48000000 to 0x4BFFFFFF (64MB).
              To access PCI via this space, we simply ioremap() the BAR
              into the kernel and we can use the standard read[bwl]/write[bwl]
              macros. This is the preferred method due to speed but it
@@ -187,13 +187,13 @@ config IXP4XX_INDIRECT_PCI
              problematic if using video cards and other memory-heavy devices.
           
           2) If > 64MB of memory space is required, the IXP4xx can be 
-	     configured to use indirect registers to access PCI This allows 
-	     for up to 128MB (0x48000000 to 0x4fffffff) of memory on the bus. 
-	     The disadvantage of this is that every PCI access requires 
-	     three local register accesses plus a spinlock, but in some 
-	     cases the performance hit is acceptable. In addition, you cannot 
-	     mmap() PCI devices in this case due to the indirect nature
-	     of the PCI window.
+	     configured to use indirect registers to access the whole PCI
+	     memory space. This currently allows for up to 1 GB (0x10000000
+	     to 0x4FFFFFFF) of memory on the bus. The disadvantage of this
+	     is that every PCI access requires three local register accesses
+	     plus a spinlock, but in some cases the performance hit is
+	     acceptable. In addition, you cannot mmap() PCI devices in this
+	     case due to the indirect nature of the PCI window.
 
 	  By default, the direct method is used. Choose this option if you
 	  need to use the indirect method instead. If you don't know
--- a/arch/arm/mach-ixp4xx/common-pci.c
+++ b/arch/arm/mach-ixp4xx/common-pci.c
@@ -481,11 +481,7 @@ int ixp4xx_setup(int nr, struct pci_sys_data *sys)
 
 	res[1].name = "PCI Memory Space";
 	res[1].start = PCIBIOS_MIN_MEM;
-#ifndef CONFIG_IXP4XX_INDIRECT_PCI
-	res[1].end = 0x4bffffff;
-#else
-	res[1].end = 0x4fffffff;
-#endif
+	res[1].end = PCIBIOS_MAX_MEM;
 	res[1].flags = IORESOURCE_MEM;
 
 	request_resource(&ioport_resource, &res[0]);
--- a/arch/arm/mach-ixp4xx/include/mach/hardware.h
+++ b/arch/arm/mach-ixp4xx/include/mach/hardware.h
@@ -18,7 +18,13 @@
 #define __ASM_ARCH_HARDWARE_H__
 
 #define PCIBIOS_MIN_IO		0x00001000
-#define PCIBIOS_MIN_MEM		(cpu_is_ixp43x() ? 0x40000000 : 0x48000000)
+#ifdef CONFIG_IXP4XX_INDIRECT_PCI
+#define PCIBIOS_MIN_MEM		0x10000000 /* 1 GB of indirect PCI MMIO space */
+#define PCIBIOS_MAX_MEM		0x4FFFFFFF
+#else
+#define PCIBIOS_MIN_MEM		0x48000000 /* 64 MB of PCI MMIO space */
+#define PCIBIOS_MAX_MEM		0x4BFFFFFF
+#endif
 
 /*
  * We override the standard dma-mask routines for bouncing.
diff --git a/arch/arm/mach-ixp4xx/include/mach/io.h b/arch/arm/mach-ixp4xx/include/mach/io.h
index c86381b..6547a29 100644
--- a/arch/arm/mach-ixp4xx/include/mach/io.h
+++ b/arch/arm/mach-ixp4xx/include/mach/io.h
@@ -26,22 +26,20 @@ extern int ixp4xx_pci_write(u32 addr, u32 cmd, u32 data);
 /*
  * IXP4xx provides two methods of accessing PCI memory space:
  *
- * 1) A direct mapped window from 0x48000000 to 0x4bffffff (64MB).
+ * 1) A direct mapped window from 0x48000000 to 0x4BFFFFFF (64MB).
  *    To access PCI via this space, we simply ioremap() the BAR
  *    into the kernel and we can use the standard read[bwl]/write[bwl]
  *    macros. This is the preffered method due to speed but it
  *    limits the system to just 64MB of PCI memory. This can be 
- *    problamatic if using video cards and other memory-heavy
- *    targets.
- *
- * 2) If > 64MB of memory space is required, the IXP4xx can be configured
- *    to use indirect registers to access PCI (as we do below for I/O
- *    transactions). This allows for up to 128MB (0x48000000 to 0x4fffffff)
- *    of memory on the bus. The disadvantage of this is that every 
- *    PCI access requires three local register accesses plus a spinlock,
- *    but in some cases the performance hit is acceptable. In addition,
- *    you cannot mmap() PCI devices in this case.
+ *    problematic if using video cards and other memory-heavy targets.
  *
+ * 2) If > 64MB of memory space is required, the IXP4xx can use indirect
+ *    registers to access the whole 4 GB of PCI memory space (as we do below
+ *    for I/O transactions). This allows currently for up to 1 GB (0x10000000
+ *    to 0x4FFFFFFF) of memory on the bus. The disadvantage of this is that
+ *    every PCI access requires three local register accesses plus a spinlock,
+ *    but in some cases the performance hit is acceptable. In addition, you
+ *    cannot mmap() PCI devices in this case.
  */
 #ifndef	CONFIG_IXP4XX_INDIRECT_PCI
 

-- 
Krzysztof Halasa