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

[Aravind Gopalakrioshnan <aravind.gopalakrishnan@amd.com>]

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.