Re: Re: [PATCH 1 of 3] xen: Automatically find serial port on PCI/PCIe and AMT devices

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Thu, Jul 07, 2011 at 04:56:23PM +0100, Jan Beulich wrote:
> >>> On 07.07.11 at 15:59, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> > # HG changeset patch
> > # User James Mckenzie <jamesmck@bob.uk.xensource.com>
> > # Date 1310047149 14400
> > # Node ID 7cdc5770d13bbd7fc2b958ba7d74787ff4e20eef
> > # Parent  2f63562df1c4230492a81793dce3672f93c93d9a
> > xen: Automatically find serial port on PCI/PCIe and AMT devices.
> > 
> > Instead of having to manually look the right I/O port on the PCI
> > devices or AMT devices, lets probe the card and find that
> > automatically.
> > 
> > This means that you don't have to have this:
> >  com1=115200,8n1,0xd800,0
> > 
> > But instead can have
> >  com1=115200,8n1,magic
> > 
> > Or if you have AMT:
> >  com1=19200,8n1,amt
> > 
> > Signed-off-by: James Mckenzie <jamesmck@bob.uk.xensource.com>
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Signed-off-by: Tom Goetz <tom.goetz@virtualcomputer.com>
> > 
> > diff -r 2f63562df1c4 -r 7cdc5770d13b xen/drivers/char/ns16550.c
> > --- a/xen/drivers/char/ns16550.c	Mon Jun 27 17:37:12 2011 +0100
> > +++ b/xen/drivers/char/ns16550.c	Thu Jul 07 09:59:09 2011 -0400
> > @@ -17,6 +17,8 @@
> >  #include <xen/timer.h>
> >  #include <xen/serial.h>
> >  #include <xen/iocap.h>
> > +#include <xen/pci.h>
> > +#include <xen/pci_regs.h>
> >  #include <asm/io.h>
> >  
> >  /*
> > @@ -433,6 +435,64 @@ static int __init check_existence(struct
> >      return (status == 0x90);
> >  }
> >  
> > +static int
> > +magic_uart_config (struct ns16550 *uart,int skip_amt)
> > +{
> > +  uint16_t class;
> > +  uint32_t bar0, len;
> > +  int b, d, f;
> > +
> > +/*Skanky hack - start at bus 1 to avoid AMT, a plug in card cannot be on bus 
> > 1 */
> > +
> > +  if (skip_amt) b=1;
> > +	else b=0;
> > +
> > +  for (; b < 0x100; ++b)
> > +    {
> > +    for (d = 0; d < 0x20; ++d)
> > +        {
> > +          for (f = 0; f < 0x8; ++f)
> > +            {
> > +
> > +              class = pci_conf_read16 (b, d, f, PCI_CLASS_DEVICE);
> > +              if (class != 0x700)
> > +                continue;;
> > +
> > +              bar0 = pci_conf_read32 (b, d, f, PCI_BASE_ADDRESS_0);
> 
> Why would a serial port only be allowed to be on the port specified
> with BAR0? E.g. if you have a serial card with multiple ports, multiple
> BARs could be candidates.
> 
> Also, why would the first one found be it?

Because that is usually COM1. You can still do com1=115200,8n1,magic
com2=115200,8n1,0xd900,0 to have both of them available.

> 
> Jan
> 
> > +
> > +              /* Not IO */
> > +              if (!(bar0 & 1))
> > +                continue;
> > +
> > +              pci_conf_write32 (b, d, f, PCI_BASE_ADDRESS_0, 0xffffffff);
> > +              len = pci_conf_read32 (b, d, f, PCI_BASE_ADDRESS_0);
> > +              pci_conf_write32 (b, d, f, PCI_BASE_ADDRESS_0, bar0);
> > +
> > +              /* Not 8 bytes */
> > +              if ((len & 0xffff) != 0xfff9)
> > +                continue;
> > +
> > +              uart->io_base = bar0 & 0xfffe;
> > +              uart->irq = 0;
> 
> Hmm, I found that without use of an interrupt the output isn't very
> reliable. But yes, getting the proper number could be pretty difficult,
> especially this early.

Yes. The patch does not remove the option of doing 'com1=115200,8n1,5'
to use IRQ 5 for the standard port. But using the PCI serial interrupt
line then.. not so much.


> 
> Jan
> 
> > +
> > +              return 0;
> > +
> > +            }
> > +
> > +        }
> > +    }
> > +
> > +  if (!skip_amt)
> > +	return -1;
> > +
> > +  uart->io_base = 0x3f8;
> > +  uart->irq = 0;
> > +  uart->clock_hz  = UART_CLOCK_HZ;
> > +
> > +  return 0;
> > +}
> > +
> > +
> >  #define PARSE_ERR(_f, _a...)                 \
> >      do {                                     \
> >          printk( "ERROR: " _f "\n" , ## _a ); \
> > @@ -481,7 +541,18 @@ static void __init ns16550_parse_port_co
> >      if ( *conf == ',' )
> >      {
> >          conf++;
> > -        uart->io_base = simple_strtoul(conf, &conf, 0);
> > +        

Ugh. Looks like a big whitespace issue there
> > +        if ( strncmp(conf,"magic",5) == 0 ) {
> > +	    if (magic_uart_config(uart,1)) 

Ditto
> > +		return;
> > +	    conf+=5;
> > +        } else if ( strncmp(conf,"amt",3) == 0 ) {
> > +	    if (magic_uart_config(uart,0)) 

And here.

I can repost it with those fixed.
> > +		return;
> > +	    conf+=3;
> > +        } else {
> > +            uart->io_base = simple_strtoul(conf, &conf, 0);
> > +        }
> >  
> >          if ( *conf == ',' )
> >          {
> 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel