ARM: Orion5x - Restore parts of io.h

PATCH New Andrew Lunn 3 participants 5 messages Jun 20, 2012

Subsystems (3)

Arm (32-bit) Arm sub-architectures Arm/Marvell Dove/MV78xx0/Orion SoC support

lore

Share & tools

lore

Andrew Lunn

author maintainer

2012-06-20 06:06 UTC #0 lore raw

PATCH 1/1 ARM: Orion5x - Restore parts of io.h

Commit 4d5fc58dbe34b78157c05b319669bb3e064ba8bd remove the orion5x io.h. Unfortunetely, this is still needed for the definition of IO_SPACE_LIMIT which overrides the default 64K. All Orion based systems have 1Mbyte of IO space per PCI[e] bus, and try to request_resource() this size.

Signed-off-by Andrew Lunn <[email protected]>

---
 arch/arm/Kconfig                        |    1 +
 arch/arm/mach-orion5x/include/mach/io.h |   14 ++++++++++++++
 2 files changed, 15 insertions(+)
 create mode 100644 arch/arm/mach-orion5x/include/mach/io.h

arch/arm/Kconfig +1 -0

index 84449dd..8fb7e4a 100644
@@ -588,6 +588,7 @@ config ARCH_ORION5X
 	select PCI
 	select ARCH_REQUIRE_GPIOLIB
 	select GENERIC_CLOCKEVENTS
+	select NEED_MACH_IO_H
 	select PLAT_ORION
 	help
 	  Support for the following Marvell Orion 5x series SoCs:

arch/arm/mach-orion5x/include/mach/io.h new file +14 -0

new file mode 100644
index 0000000..f017fe2
@@ -0,0 +1,14 @@
+/*
+ * arch/arm/mach-orion5x/include/mach/io.h
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#ifndef __ASM_ARCH_IO_H
+#define __ASM_ARCH_IO_H
+
+#define IO_SPACE_LIMIT		0xffffffff
+#define __io(a)			__typesafe_io(a)
+#endif

Signature · 1 line

1.7.10

Sergei Shtylyov

2012-06-20 11:56 UTC #1 lore raw

PATCH 1/1 ARM: Orion5x - Restore parts of io.h

Hello.

Quoted 1 line · Andrew Lunn

Commit 4d5fc58dbe34b78157c05b319669bb3e064ba8bd

Please also specify that commit's summary in parens.

Quoted 7 lines

remove the orion5x
io.h. Unfortunetely, this is still needed for the definition of IO_SPACE_LIMIT which overrides the default 64K. All Orion based systems have 1Mbyte of IO space per PCI[e] bus, and try to request_resource() this size.
Signed-off-by Andrew Lunn<[email protected]>

WBR, Sergei

Arnd Bergmann

2012-06-20 15:14 UTC #2 lore raw

PATCH 1/1 ARM: Orion5x - Restore parts of io.h

In reply to Andrew Lunn

Quoted 16 lines · Andrew Lunn

Commit 4d5fc58dbe34b78157c05b319669bb3e064ba8bd remove the orion5x io.h. Unfortunetely, this is still needed for the definition of IO_SPACE_LIMIT which overrides the default 64K. All Orion based systems have 1Mbyte of IO space per PCI[e] bus, and try to request_resource() this size.
arch/arm/mach-orion5x/include/mach/io.h new file +4 -0
new file mode 100644
index 0000000..f017fe2
@@ -0,0 +1,14 @@
+/*
+ * arch/arm/mach-orion5x/include/mach/io.h
+ *
+ * This file is licensed under the terms of the GNU General Public

arch/arm/mach-orion5x/include/mach/io.h line 14

Quoted 10 lines

arch/arm/mach-orion5x/include/mach/io.h new file +10 -0

+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#ifndef __ASM_ARCH_IO_H
+#define __ASM_ARCH_IO_H
+
+#define IO_SPACE_LIMIT		0xffffffff
+#define __io(a)			__typesafe_io(a)
+#endif

So if you need 1 MB per bus, why do you make the limit 4GB? Also, the __io function does not actually point to the IO window at all, which also appears to be horribly wrong.

My guess is that you actually want this to be

#define IO_SPACE_LIMIT SZ_2MB
#define __io(a) ((void __iomem *)ORION5X_PCI_IO_VIRT_BASE + a)

Your patch otherwise would make the kernel build again, but has no chance of doing the right thing.

Arnd

Andrew Lunn

author maintainer

2012-06-20 15:46 UTC #3 lore raw

PATCH 1/1 ARM: Orion5x - Restore parts of io.h

Quoted 32 lines · Arnd Bergmann

Quoted 16 lines · Andrew Lunn
Commit 4d5fc58dbe34b78157c05b319669bb3e064ba8bd remove the orion5x io.h. Unfortunetely, this is still needed for the definition of IO_SPACE_LIMIT which overrides the default 64K. All Orion based systems have 1Mbyte of IO space per PCI[e] bus, and try to request_resource() this size.
arch/arm/mach-orion5x/include/mach/io.h new file +4 -0
new file mode 100644
index 0000000..f017fe2
@@ -0,0 +1,14 @@
+/*
+ * arch/arm/mach-orion5x/include/mach/io.h
+ *
+ * This file is licensed under the terms of the GNU General Public
arch/arm/mach-orion5x/include/mach/io.h line 14
Quoted 10 lines
arch/arm/mach-orion5x/include/mach/io.h new file +10 -0
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#ifndef __ASM_ARCH_IO_H
+#define __ASM_ARCH_IO_H
+
+#define IO_SPACE_LIMIT		0xffffffff
+#define __io(a)			__typesafe_io(a)
+#endif
So if you need 1 MB per bus, why do you make the limit 4GB? Also, the __io function does not actually point to the IO window at all, which also appears to be horribly wrong.

Quoted 7 lines

My guess is that you actually want this to be
#define IO_SPACE_LIMIT SZ_2MB
#define __io(a) ((void __iomem *)ORION5X_PCI_IO_VIRT_BASE + a)
Your patch otherwise would make the kernel build again, but has no chance of doing the right thing.

My patch simply puts back what was removed. Please see:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=4d5fc58dbe34b78157c05b319669bb3e064ba8bd#patch20

It was probably broken before. It is probably broken now. It probably never did the right thing. However, we don't have any hardware to test with and we think it is probably never used in real life.

Is it worth doing more than putting back the original code?

Andrew

Arnd Bergmann

2012-06-20 15:55 UTC #4 lore raw

PATCH 1/1 ARM: Orion5x - Restore parts of io.h

Quoted 15 lines · Andrew Lunn

Quoted 5 lines
So if you need 1 MB per bus, why do you make the limit 4GB? Also, the __io function does not actually point to the IO window at all, which also appears to be horribly wrong.
Quoted 7 lines
My guess is that you actually want this to be
#define IO_SPACE_LIMIT SZ_2MB
#define __io(a) ((void __iomem *)ORION5X_PCI_IO_VIRT_BASE + a)
Your patch otherwise would make the kernel build again, but has no chance of doing the right thing.
My patch simply puts back what was removed. Please see:

Quoted 7 lines

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=4d5fc58dbe34b78157c05b319669bb3e064ba8bd#patch20
It was probably broken before. It is probably broken now. It probably never did the right thing. However, we don't have any hardware to test with and we think it is probably never used in real life.
Is it worth doing more than putting back the original code?

Well, the broken version has significant side-effects, e.g. you cannot access /dev/port or load a device driver that pokes at the PCI I/O space without crashing the kernel. I think it's better to put something in place that has a chance of working than something that is known to be broken.

That said, we probably should not have done the change that broke the kernel further than it was broken already: The assumption back then was that we'd only remove the files that are known to be incorrect, be we did not consider the side-effects.

Now that we're broken already, we have the choice between either putting it back to the state it was in before, or trying to fix it for real. Right now, I'm leaning towards fixing it because there is still some time before the 3.5 release. If you can try the version that I suggested above and verify that it fixes the problem that was introduced in 4d5fc58dbe3, I'd prefer merging that.

Arnd