From mboxrd@z Thu Jan  1 00:00:00 1970
From: Aravind Gopalakrioshnan <aravind.gopalakrishnan@amd.com>
Subject: Re: [PATCH] ns16550: Add support for UART present in
 Broadcom TruManage capable NetXtreme chips
Date: Thu, 14 Nov 2013 11:16:33 -0600
Message-ID: <52850571.5060301@amd.com>
References: <1384446574-5925-1-git-send-email-Aravind.Gopalakrishnan@amd.com>
	<5284FD79.3000704@citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <5284FD79.3000704@citrix.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Thomas Lendacky <Thomas.Lendacky@amd.com>, keir@xen.org, jbeulich@suse.com, xen-devel@lists.xen.org, Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>, sherry.hurwitz@amd.com, shurd@broadcom.com
List-Id: xen-devel@lists.xenproject.org

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.