Re: [Xen-devel] [PATCH] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips

[Aravind Gopalakrioshnan <aravind.gopalakrishnan@xxxxxxx>]

On 11/14/2013 10:42 AM, Andrew Cooper wrote:
> On 14/11/13 16:29, Aravind Gopalakrishnan wrote:
> >   
> > +/* Defining uart config options for MMIO devices */
> > +struct ns16550_config_mmio {
> > +    u16 vendor_id;
> > +    u16 dev_id;
> > +    int reg_shift;
> Shifts of signed quantities are a recipe for undefined behaviour.  I
> dont see any reason for any of these values to be signed quantities.
You are right.. I will change this..
> > +    int reg_width;
> > +    int fifo_size;
> > +    u32 lsr_mask;
> > +    int max_bars;
> > +};
> > +
> > +/*
> > + * Create lookup tables for specific MMIO devices..
> > + * It is assumed that if the device found is MMIO,
> > + * then you have indexed it here. Else, the driver
> > + * does nothing.
> > + */
> > +static struct ns16550_config_mmio uart_config[] =
> > +{
> > +    /* Broadcom TruManage device */
> > +    {
> > +        .vendor_id = 0x14e4,
> > +        .dev_id = 0x160a,
> > +        .reg_shift = 2,
> > +        .reg_width = 1,
> > +        .fifo_size = 64,
> > +        .lsr_mask = (UART_LSR_THRE | UART_LSR_TEMT),
> > +        .max_bars = 1,
> > +    },
> > +};
> > +
> >   static void ns16550_delayed_resume(void *data);
> >   
> >   static char ns_read_reg(struct ns16550 *uart, int reg)
> > @@ -134,7 +166,7 @@ static void ns16550_interrupt(
> >       while ( !(ns_read_reg(uart, UART_IIR) & UART_IIR_NOINT) )
> >       {
> >           char lsr = ns_read_reg(uart, UART_LSR);
> > -        if ( lsr & UART_LSR_THRE )
> > +        if ( (lsr & uart->lsr_mask) == uart->lsr_mask )
> >               serial_tx_interrupt(port, regs);
> >           if ( lsr & UART_LSR_DR )
> >               serial_rx_interrupt(port, regs);
> > @@ -160,7 +192,7 @@ static void __ns16550_poll(struct cpu_user_regs *regs)
> >           serial_rx_interrupt(port, regs);
> >       }
> >   
> > -    if ( ns_read_reg(uart, UART_LSR) & UART_LSR_THRE )
> > +    if ( ( ns_read_reg(uart, UART_LSR) & uart->lsr_mask ) == uart->lsr_mask )
> >           serial_tx_interrupt(port, regs);
> >   
> >   out:
> > @@ -183,7 +215,9 @@ static int ns16550_tx_ready(struct serial_port *port)
> >   
> >       if ( ns16550_ioport_invalid(uart) )
> >           return -EIO;
> > -    return ns_read_reg(uart, UART_LSR) & UART_LSR_THRE ? uart->fifo_size : 0;
> > +       
> > +    return ( (ns_read_reg(uart, UART_LSR) &
> > +              uart->lsr_mask ) == uart->lsr_mask ) ? uart->fifo_size : 0;
> >   }
> >   
> >   static void ns16550_putc(struct serial_port *port, char c)
> > @@ -550,7 +584,8 @@ static int
> >   pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
> >   {
> >       uint32_t bar, len;
> > -    int b, d, f, nextf;
> > +    int b, d, f, nextf, i;
> > +    u16 vendor, device;
> >   
> >       /* NB. Start at bus 1 to avoid AMT: a plug-in card cannot be on bus 0. */
> >       for ( b = skip_amt ? 1 : 0; b < 0x100; b++ )
> > @@ -579,27 +614,63 @@ pci_uart_config (struct ns16550 *uart, int skip_amt, int 
> > bar_idx)
> >                   bar = pci_conf_read32(0, b, d, f,
> >                                         PCI_BASE_ADDRESS_0 + bar_idx*4);
> >   
> > +                if ( !bar )
> > +                    continue;
> > +
> What is this check supposed to be doing?  This could be a valid
> non-prefechable 32bit memory bar.
Okay, will correct this.
> >         
> >   
> >                   uart->ps_bdf[0] = b;
> >                   uart->ps_bdf[1] = d;
> >                   uart->ps_bdf[2] = f;
> >                   uart->bar = bar;
> >                   uart->bar_idx = bar_idx;
> > -                uart->io_base = bar & ~PCI_BASE_ADDRESS_SPACE_IO;
> >                   uart->irq = pci_conf_read8(0, b, d, f, PCI_INTERRUPT_PIN) ?
> >                       pci_conf_read8(0, b, d, f, PCI_INTERRUPT_LINE) : 0;
> >   
> > +
> Spurious whitespace change.  Please remove.
>
> ~Andrew
Noted.

Will send out changes in a V2.

Thanks,
-Aravind.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel