[PATCH 0/2]: x86: Honor IO_DELAY IO port settings

[PATCH 0/2]: x86: Honor IO_DELAY IO port settings

[Simon Kagstrom]

Greetings!

The kernel can be configured to use IO port 0xed instead of port 0x80
for IO delay writes and some boards also require this to function
properly. These two patches fix two places where port 0x80 is hardcoded.

* Patch 1: Use native_io_delay for the serial/8250 driver instead of
  always using 0x80.

* Patch 2: Honor CONFIG_IO_DELAY_0XED if set for real-mode boot code


The board we have works fine with using 0x80, but we're debugging a
BIOS issue and are logging writes to port 0x80 (BIOS post codes).
Avoiding the extra port 0x80 writes makes it easier to weed out the
important information.

// Simon

[PATCH 1/2]: serial8250: Use native_io_delay on the x86

[Simon Kagstrom]

Port 0x80 is not safe to use on all x86 boards (see
arch/x86/kernel/io_delay.c), so use native_io_delay instead.

Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
---
 drivers/serial/8250.c |   12 +++++-------
 1 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index c3db16b..b3007a4 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -38,6 +38,7 @@
 #include <linux/serial_8250.h>
 #include <linux/nmi.h>
 #include <linux/mutex.h>
+#include <linux/io.h>
 
 #include <asm/io.h>
 #include <asm/irq.h>
@@ -1109,11 +1110,8 @@ static void autoconfig(struct uart_8250_port *up, unsigned int probeflags)
 		 * Do a simple existence test first; if we fail this,
 		 * there's no point trying anything else.
 		 *
-		 * 0x80 is used as a nonsense port to prevent against
-		 * false positives due to ISA bus float.  The
-		 * assumption is that 0x80 is a non-existent port;
-		 * which should be safe since include/asm/io.h also
-		 * makes this assumption.
+		 * The IO delay is used to prevent against false positives
+		 * due to ISA bus float.
 		 *
 		 * Note: this is safe as long as MCR bit 4 is clear
 		 * and the device is in "PC" mode.
@@ -1121,7 +1119,7 @@ static void autoconfig(struct uart_8250_port *up, unsigned int probeflags)
 		scratch = serial_inp(up, UART_IER);
 		serial_outp(up, UART_IER, 0);
 #ifdef __i386__
-		outb(0xff, 0x080);
+		native_io_delay();
 #endif
 		/*
 		 * Mask out IER[7:4] bits for test as some UARTs (e.g. TL
@@ -1130,7 +1128,7 @@ static void autoconfig(struct uart_8250_port *up, unsigned int probeflags)
 		scratch2 = serial_inp(up, UART_IER) & 0x0f;
 		serial_outp(up, UART_IER, 0x0F);
 #ifdef __i386__
-		outb(0, 0x080);
+		native_io_delay();
 #endif
 		scratch3 = serial_inp(up, UART_IER) & 0x0f;
 		serial_outp(up, UART_IER, scratch);
-- 
1.6.0.4

Re: [PATCH 1/2]: serial8250: Use native_io_delay on the x86

[Alan Cox]

On Wed, 17 Mar 2010 13:30:50 +0100
Simon Kagstrom <simon.kagstrom@netinsight.net> wrote:

> Port 0x80 is not safe to use on all x86 boards (see
> arch/x86/kernel/io_delay.c), so use native_io_delay instead.
> 
> Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>

native_io_delay() won't work if the system is being run with no delays.
The I/O cycle isn't for the delay but to force the bus signals. So in
various modes (paravirt, udelay, no delay) the native_io_delay won't
actually do what is required.

I'm actually surprised you hit this path and if anything the right fix
is to avoid hitting this kind of probing in the first place.

Re: [PATCH 1/2]: serial8250: Use native_io_delay on the x86

[Simon Kagstrom]

On Wed, 17 Mar 2010 13:01:59 +0000
Alan Cox <alan@linux.intel.com> wrote:

> On Wed, 17 Mar 2010 13:30:50 +0100
> Simon Kagstrom <simon.kagstrom@netinsight.net> wrote:
> 
> > Port 0x80 is not safe to use on all x86 boards (see
> > arch/x86/kernel/io_delay.c), so use native_io_delay instead.
> > 
> > Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
> 
> native_io_delay() won't work if the system is being run with no delays.
> The I/O cycle isn't for the delay but to force the bus signals. So in
> various modes (paravirt, udelay, no delay) the native_io_delay won't
> actually do what is required.

You are right, I should have seen that.

Would something similar to the other patch be acceptable, i.e., like
the diff below?

> I'm actually surprised you hit this path and if anything the right fix
> is to avoid hitting this kind of probing in the first place.

But isn't this code path pretty much always being executed? If I read
the code correct, unless we have a buggy UART it will be executed if
UPF_BOOT_AUTOCONF is set.

// Simon

diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index 524f6ab..c5e3f9a 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -38,6 +38,7 @@
 #include <linux/serial_8250.h>
 #include <linux/nmi.h>
 #include <linux/mutex.h>
+#include <linux/io.h>
 
 #include <asm/io.h>
 #include <asm/irq.h>
@@ -1071,6 +1072,19 @@ static void autoconfig_16550a(struct uart_8250_port *up)
        serial_outp(up, UART_IER, iersave);
 }
 
+static void bus_delay(u8 val)
+{
+#ifdef __i386__
+# ifdef CONFIG_IO_DELAY_TYPE_0XED
+       const u16 io_port = 0xed;
+# else
+       const u16 io_port = 0x80;
+#endif
+
+       outb(0xff, io_port);
+#endif
+}
+
 /*
  * This routine is called by rs_init() to initialize a specific serial
  * port.  It determines what type of UART chip this serial port is
@@ -1104,29 +1118,24 @@ static void autoconfig(struct uart_8250_port *up, unsigned int probeflags)
                 * Do a simple existence test first; if we fail this,
                 * there's no point trying anything else.
                 *
-                * 0x80 is used as a nonsense port to prevent against
-                * false positives due to ISA bus float.  The
-                * assumption is that 0x80 is a non-existent port;
-                * which should be safe since include/asm/io.h also
-                * makes this assumption.
+                * The IO delay is used to prevent against false positives
+                * due to ISA bus float.
                 *
                 * Note: this is safe as long as MCR bit 4 is clear
                 * and the device is in "PC" mode.
                 */
                scratch = serial_inp(up, UART_IER);
                serial_outp(up, UART_IER, 0);
-#ifdef __i386__
-               outb(0xff, 0x080);
-#endif
+               bus_delay(0xff);
+
                /*
                 * Mask out IER[7:4] bits for test as some UARTs (e.g. TL
                 * 16C754B) allow only to modify them if an EFR bit is set.
                 */
                scratch2 = serial_inp(up, UART_IER) & 0x0f;
                serial_outp(up, UART_IER, 0x0F);
-#ifdef __i386__
-               outb(0, 0x080);
-#endif
+               bus_delay(0x0);
+
                scratch3 = serial_inp(up, UART_IER) & 0x0f;
                serial_outp(up, UART_IER, scratch);
                if (scratch2 != 0 || scratch3 != 0x0F) {
[simkag@marrow kernel]$ 

Re: [PATCH 1/2]: serial8250: Use native_io_delay on the x86

[H. Peter Anvin]

On 03/17/2010 05:30 AM, Simon Kagstrom wrote:
>   #ifdef __i386__
> -		outb(0xff, 0x080);
> +		native_io_delay();
>   #endif

There is something a lot more weird about this.  First of all, it's 
#ifdef'd out on all but __i386__ including x86-64; second, it looks like 
this is specific to synchronizing to the IER.

I'm wondering if the right thing isn't to add a dummy write to the SCR 
register (or similar.)

	-hpa

Re: [PATCH 1/2]: serial8250: Use native_io_delay on the x86

[Alan Cox]

On Wed, 17 Mar 2010 11:25:46 -0700
"H. Peter Anvin" <hpa@zytor.com> wrote:

> On 03/17/2010 05:30 AM, Simon Kagstrom wrote:
> >   #ifdef __i386__
> > -		outb(0xff, 0x080);
> > +		native_io_delay();
> >   #endif
> 
> There is something a lot more weird about this.  First of all, it's 
> #ifdef'd out on all but __i386__ including x86-64; second, it looks like 
> this is specific to synchronizing to the IER.
> 
> I'm wondering if the right thing isn't to add a dummy write to the SCR 
> register (or similar.)

You just need a write to something on the ISA bus which is 'safe' so that
you don't end up reading back what you wrote to an non-existant port as
some old chipsets will return the last ISA result when you do this rather
than 0xff.

Re: [PATCH 1/2]: serial8250: Use native_io_delay on the x86

[H. Peter Anvin]

On 03/17/2010 11:36 AM, Alan Cox wrote:
>
> You just need a write to something on the ISA bus which is 'safe' so that
> you don't end up reading back what you wrote to an non-existant port as
> some old chipsets will return the last ISA result when you do this rather
> than 0xff.

Ah... floating bus syndrome.  So how about writing 0xff to the SCR 
register in the same register range?

	-hpa

[PATCH 2/2]: x86 real mode: Set delay port according to kernel config

[Simon Kagstrom]

Port 0x80 is not safe to use on all x86 boards (see
arch/x86/kernel/io_delay.c), so optionally use 0xed from the kernel
config instead.

Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
---
 arch/x86/boot/boot.h |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/arch/x86/boot/boot.h b/arch/x86/boot/boot.h
index 98239d2..79880b1 100644
--- a/arch/x86/boot/boot.h
+++ b/arch/x86/boot/boot.h
@@ -73,7 +73,12 @@ static inline u32 inl(u32 port)
 
 static inline void io_delay(void)
 {
+#ifdef CONFIG_IO_DELAY_0XED
+	const u16 DELAY_PORT = 0xed;
+#else
 	const u16 DELAY_PORT = 0x80;
+#endif
+
 	asm volatile("outb %%al,%0" : : "dN" (DELAY_PORT));
 }
 
-- 
1.6.0.4