uart_match_port() question

uart_match_port() question

[Benjamin Herrenschmidt]

Hi Russel, would you accept a patch like that:

Index: linux-work/drivers/serial/serial_core.c
===================================================================
--- linux-work.orig/drivers/serial/serial_core.c	2005-11-14 20:32:16.000000000 +1100
+++ linux-work/drivers/serial/serial_core.c	2005-11-27 11:13:54.000000000 +1100
@@ -2307,7 +2307,8 @@
 		return (port1->iobase == port2->iobase) &&
 		       (port1->hub6   == port2->hub6);
 	case UPIO_MEM:
-		return (port1->membase == port2->membase);
+		return (port1->membase == port2->membase) ||
+			(port1->mapbase && port1->mapbase == port2->mapbase);
 	}
 	return 0;
 }

The reason is a bit complicated, but basically, we have some arch code
that builds a list of available serial ports very early and registers that
as a platform device. It also detects which one is the default firmware port
and what speed it's been configured for and builds a proper config line to
pass to add_preferred_console() so we get the default serial console setup
properly automatically.

This list includes however ports that are on PCI devices on some recent
machines. Thus, we need to make sure that, when 8250_pci.c kicks in, it
property detects that those platform ports are the same it's discovered
and thus properly re-uses the same port & minor. However, while that works
for PIO ports, it doesn't for MMIO since membase is obtained from ioremap,
and thus will be different between the port registered at boot and the
value passed by the PCI code. Only mapbase will be the same.

If we just skipped PCI devices in our early discovery code (thus letting
8250_pci.c alone discover them), we would be unable to use them for very
early console output, and we would be unable to "know" what their minor number
will be, and thus build an appropriate argument string for add_preferred_console(),
which means we would be unable to have the console automatically pick the port
that we set by the firmware and at the right speed, which basically means
console not working for users. 

Cheers,
Ben.

Re: uart_match_port() question

[Russell King]

On Sun, Nov 27, 2005 at 11:21:46AM +1100, Benjamin Herrenschmidt wrote:
> Hi Russel, would you accept a patch like that:

s/l,/l&/

> Index: linux-work/drivers/serial/serial_core.c
> ===================================================================
> --- linux-work.orig/drivers/serial/serial_core.c	2005-11-14 20:32:16.000000000 +1100
> +++ linux-work/drivers/serial/serial_core.c	2005-11-27 11:13:54.000000000 +1100
> @@ -2307,7 +2307,8 @@
>  		return (port1->iobase == port2->iobase) &&
>  		       (port1->hub6   == port2->hub6);
>  	case UPIO_MEM:
> -		return (port1->membase == port2->membase);
> +		return (port1->membase == port2->membase) ||
> +			(port1->mapbase && port1->mapbase == port2->mapbase);
>  	}
>  	return 0;
>  }

I don't think so.  (see below)

> The reason is a bit complicated, but basically, we have some arch code
> that builds a list of available serial ports very early and registers that
> as a platform device. It also detects which one is the default firmware port
> and what speed it's been configured for and builds a proper config line to
> pass to add_preferred_console() so we get the default serial console setup
> properly automatically.
> 
> This list includes however ports that are on PCI devices on some recent
> machines. Thus, we need to make sure that, when 8250_pci.c kicks in, it
> property detects that those platform ports are the same it's discovered
> and thus properly re-uses the same port & minor. However, while that works
> for PIO ports, it doesn't for MMIO since membase is obtained from ioremap,
> and thus will be different between the port registered at boot and the
> value passed by the PCI code. Only mapbase will be the same.
> 
> If we just skipped PCI devices in our early discovery code (thus letting
> 8250_pci.c alone discover them), we would be unable to use them for very
> early console output, and we would be unable to "know" what their minor number
> will be, and thus build an appropriate argument string for add_preferred_console(),
> which means we would be unable to have the console automatically pick the port
> that we set by the firmware and at the right speed, which basically means
> console not working for users. 

Looking at this deeper, I think we should _only_ use mapbase in this
case.  membase is really a indeterminant cookie which bears no real
relationship to whether two ports are identical - in fact, if we are
going to compare two of these cookies, I think arch code should be
involved.

So how about:

-             return (port1->membase == port2->membase);
+             return (port1->mapbase == port2->mapbase);

?

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: uart_match_port() question

[Benjamin Herrenschmidt]

On Mon, 2005-11-28 at 11:30 +0000, Russell King wrote:
> On Sun, Nov 27, 2005 at 11:21:46AM +1100, Benjamin Herrenschmidt wrote:
> > Hi Russel, would you accept a patch like that:

My deepest appologies ! :)

> s/l,/l&/
> 
> > Index: linux-work/drivers/serial/serial_core.c
> > ===================================================================
> > --- linux-work.orig/drivers/serial/serial_core.c	2005-11-14 20:32:16.000000000 +1100
> > +++ linux-work/drivers/serial/serial_core.c	2005-11-27 11:13:54.000000000 +1100
> > @@ -2307,7 +2307,8 @@
> >  		return (port1->iobase == port2->iobase) &&
> >  		       (port1->hub6   == port2->hub6);
> >  	case UPIO_MEM:
> > -		return (port1->membase == port2->membase);
> > +		return (port1->membase == port2->membase) ||
> > +			(port1->mapbase && port1->mapbase == port2->mapbase);
> >  	}
> >  	return 0;
> >  }
> 
> I don't think so.  (see below)

Heh, Ok.

> Looking at this deeper, I think we should _only_ use mapbase in this
> case

Totally agreed.

> .  membase is really a indeterminant cookie which bears no real
> relationship to whether two ports are identical - in fact, if we are
> going to compare two of these cookies, I think arch code should be
> involved.
> 
> So how about:
> 
> -             return (port1->membase == port2->membase);
> +             return (port1->mapbase == port2->mapbase);

Yup, indeed. I did it the above way in case you had good reasons of
comparing membase too, but indeed, comparing mapbase only makes the most
sense.

I'll send a proper patch tomorrow.

Ben.

Re: uart_match_port() question

[Bjorn Helgaas]

On Saturday 26 November 2005 5:21 pm, Benjamin Herrenschmidt wrote:
> The reason is a bit complicated, but basically, we have some arch code
> that builds a list of available serial ports very early and registers that
> as a platform device. It also detects which one is the default firmware port
> and what speed it's been configured for and builds a proper config line to
> pass to add_preferred_console() so we get the default serial console setup
> properly automatically.
> 
> This list includes however ports that are on PCI devices on some recent
> machines. Thus, we need to make sure that, when 8250_pci.c kicks in, it
> property detects that those platform ports are the same it's discovered
> and thus properly re-uses the same port & minor.

[Sorry for the late response, I haven't been keeping up on lkml lately.]

ia64 has basically the same situation.  I decided it was a mistake to
have the arch code register serial ports early, because we only learn
about a few of the ports early, and the firmware console configuration
determines which ones we learn about.

The consequence is that changing the firmware configuration changes the
serial device names, which I thought was a bad thing.

I finally settled on this scheme:
	- discover default firmware port (pcdp.c)
	- set it up as an "early uart" which has no ttyS name
	  and runs in polled mode (early_serial_console_init())
	- register it as a console
	- let 8250_{pci,pnp,etc} discover all the ports and
	  figure out minor numbers (i.e., ttyS names)
	- locate the port that matches the default firmware port,
	  switch console to it, and unregister the "early uart"
	  (early_uart_console_switch())

This all should work for any arch, though I've only really tried it on
ia64.  Of course, the pcdp.c part would have to be replaced.  You can
try it out without the firmware support using the "console=uart"
argument:

        console=        [KNL] Output console device and options.
        ...
                uart,io,<addr>[,options]
                uart,mmio,<addr>[,options]
                        Start an early, polled-mode console on the 8250/16550
                        UART at the specified I/O port or MMIO address,
                        switching to the matching ttyS device later.  The
                        options are the same as for ttyS, above.

Re: uart_match_port() question

[Benjamin Herrenschmidt]

> ia64 has basically the same situation.  I decided it was a mistake to
> have the arch code register serial ports early, because we only learn
> about a few of the ports early, and the firmware console configuration
> determines which ones we learn about.
> 
> The consequence is that changing the firmware configuration changes the
> serial device names, which I thought was a bad thing.
> 
> I finally settled on this scheme:
> 	- discover default firmware port (pcdp.c)
> 	- set it up as an "early uart" which has no ttyS name
> 	  and runs in polled mode (early_serial_console_init())
> 	- register it as a console
> 	- let 8250_{pci,pnp,etc} discover all the ports and
> 	  figure out minor numbers (i.e., ttyS names)
> 	- locate the port that matches the default firmware port,
> 	  switch console to it, and unregister the "early uart"
> 	  (early_uart_console_switch())

Yup, I've been thinking about a similar approach yah. My main issue is
your last step: "locate the port that matches the default firmware
port". Right now, thins works because the early registration allow me to
know in advance what the ttyS number will be. If I go your way,  which I
'm tempted to do, I need to figure out precisely how to properly match
the ports. Part of the problem here is for example PIO. There is no such
thing as PIO on a PowerPC, it's purely a PCI abstraction, thus inX/outX
will only work once the PCI host briges have been discovered and their
IO space mapped (setup_arch() time, but I definitely want my early
console earlier). Thus, for early ports, we use the open firmware tree
to translate all (including PIO) addresses to MMIO in CPU space.

Thus, later on, when the serial driver kicks in, it can't match the PCI
IO resources it's getting from the PCI code to the MMIO physical
addresses or ioremaped addresses that were used at early boot time.

That sort of thing ...

Anyway, things work now with the fix for properly matching MMIO ports
with their physical address and my current mecanism, even if it's not
the nicest solution. I'll still look into reworking it a better way but
I don't have the time right now.

Re: uart_match_port() question

[Bjorn Helgaas]

On Wednesday 07 December 2005 4:13 pm, Benjamin Herrenschmidt wrote:
> ...  Part of the problem here is for example PIO. There is no such
> thing as PIO on a PowerPC, it's purely a PCI abstraction, thus inX/outX
> will only work once the PCI host briges have been discovered and their
> IO space mapped (setup_arch() time, but I definitely want my early
> console earlier).

ia64 has a similar problem, but it's a bit easier because firmware
configures and tells us about the legacy (0-64K) I/O port space.
So we do have to look up the MMIO base where those I/O ports get
mapped, but that's basically the first thing in setup_arch().  We
don't do much before that, so it hasn't been worthwhile to make
the console work earlier.

Re: uart_match_port() question

[Benjamin Herrenschmidt]

On Wed, 2005-12-07 at 16:36 -0700, Bjorn Helgaas wrote:
> On Wednesday 07 December 2005 4:13 pm, Benjamin Herrenschmidt wrote:
> > ...  Part of the problem here is for example PIO. There is no such
> > thing as PIO on a PowerPC, it's purely a PCI abstraction, thus inX/outX
> > will only work once the PCI host briges have been discovered and their
> > IO space mapped (setup_arch() time, but I definitely want my early
> > console earlier).
> 
> ia64 has a similar problem, but it's a bit easier because firmware
> configures and tells us about the legacy (0-64K) I/O port space.
> So we do have to look up the MMIO base where those I/O ports get
> mapped, but that's basically the first thing in setup_arch().  We
> don't do much before that, so it hasn't been worthwhile to make
> the console work earlier.

Oh, that's what I do too, that is, I have a mecanism now to walk the
firmware device-tree and get the MMIO, my problem was with matching that
MMIO address with the PIO one that 8250 gets later on. It seems that you
solved that problem so it looks like I really need to look more closely
at what you are doing :)

Ben.

Re: uart_match_port() question

[Bjorn Helgaas]

On Wednesday 07 December 2005 5:17 pm, Benjamin Herrenschmidt wrote:
> On Wed, 2005-12-07 at 16:36 -0700, Bjorn Helgaas wrote:
> > On Wednesday 07 December 2005 4:13 pm, Benjamin Herrenschmidt wrote:
> > > ...  Part of the problem here is for example PIO. There is no such
> > > thing as PIO on a PowerPC, it's purely a PCI abstraction, thus inX/outX
> > > will only work once the PCI host briges have been discovered and their
> > > IO space mapped (setup_arch() time, but I definitely want my early
> > > console earlier).
> > 
> > ia64 has a similar problem, but it's a bit easier because firmware
> > configures and tells us about the legacy (0-64K) I/O port space.
> > So we do have to look up the MMIO base where those I/O ports get
> > mapped, but that's basically the first thing in setup_arch().  We
> > don't do much before that, so it hasn't been worthwhile to make
> > the console work earlier.
> 
> Oh, that's what I do too, that is, I have a mecanism now to walk the
> firmware device-tree and get the MMIO, my problem was with matching that
> MMIO address with the PIO one that 8250 gets later on. It seems that you
> solved that problem so it looks like I really need to look more closely
> at what you are doing :)

Well, don't get your hopes up too much...  ia64 uses the same addresses
early and late, i.e., if 8250_pci discovers a PIO port, the early code
also uses a PIO address.  We never use an MMIO address early and a PIO
address later.

The PIO -> MMIO translation is hidden inside our inb/outb functions
(see __ia64_mk_io_addr()).  The firmware tells us where the legacy
0-64K I/O ports are mapped, so we can set up io_space[0].mmio_base
very early, without enumerating the PCI bridges.