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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.11.13 at 01:54, Aravind Gopalakrishnan 
>>> <Aravind.Gopalakrishnan@xxxxxxx> wrote:
> @@ -77,6 +78,37 @@ static struct ns16550 {
>  #endif
>  } ns16550_com[2] = { { 0 } };
>  
> +/* Defining uart config options for MMIO devices */
> +struct ns16550_config_mmio {
> +    u16 vendor_id;
> +    u16 dev_id;
> +    int reg_shift;
> +    int reg_width; 
> +    int fifo_size;

As Andrew had pointed out before, and as I think I explained before
too - even if you don't adjust existing code in this patch, you
shouldn't introduce further bogus signed integers here.

> @@ -546,11 +580,12 @@ static int __init check_existence(struct ns16550 *uart)
>  }
>  
>  #ifdef HAS_PCI
> -static int
> +static int __init
>  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;

This "i" is another case of a variable clearly wanting to be unsigned.
If making all the other four variables unsigned is okay (which I think
it is), then just do it all in one go. Otherwise you'd have to add a
new line rather than adding to the existing declaration.


> @@ -581,22 +616,67 @@ pci_uart_config (struct ns16550 *uart, int skip_amt, 
> int bar_idx)
>  
>                  /* Not IO */
>                  if ( !(bar & PCI_BASE_ADDRESS_SPACE_IO) )
> -                    continue;
> +                {
> +                    vendor = pci_conf_read16(0, b, d, f, PCI_VENDOR_ID);
> +                    device = pci_conf_read16(0, b, d, f, PCI_DEVICE_ID);
> +
> +                    /* Check for quirks in uart_config lookup table */
> +                    for ( i = 0; i < ARRAY_SIZE(uart_config); i++)
> +                    {
> +                        if ( uart_config[i].vendor_id != vendor )
> +                            continue;
> +                       
> +                        if ( uart_config[i].dev_id != device )
> +                            continue;
> +
> +                        pci_conf_write32(0, b, d, f, PCI_BASE_ADDRESS_0, 
> ~0u);
> +                        len = pci_conf_read32(0, b, d, f, 
> PCI_BASE_ADDRESS_0);
> +                        pci_conf_write32(0, b, d, f, 
> +                                         PCI_BASE_ADDRESS_0 + bar_idx*4, 
> bar);
> +                        len &= ~(0xf);

I'm sure there a manifest constant for this.

> +                        uart->io_size = len & ~(len - 1);

Why not just "-len"?

Also you're not taking 64-bit BARs into consideration here. Minimally
you need to bail upon encountering one.

> +
> +                        /* Force length of mmio region to be atleast 8 bytes 
> */

"at least"

> +                        if ( uart->io_size < 0x8 )
> +                            continue;
> +
> +                        if ( bar_idx >= uart_config[i].max_bars )
> +                            continue;

I suppose this would better be moved up.

> +
> +                        if (uart_config[i].fifo_size )

Coding style.

> +                            uart->fifo_size = uart_config[i].fifo_size;
> +
> +                        uart->reg_shift = uart_config[i].reg_shift;
> +                        uart->reg_width = uart_config[i].reg_width;
> +                        uart->lsr_mask = uart_config[i].lsr_mask;
> +                        uart->io_base = bar & PCI_BASE_ADDRESS_MEM_MASK;
> +                        break;
> +                    }
> +
> +                    /* If we have an io_base, then we succeeded in the 
> lookup */
> +                    if ( !uart->io_base )
> +                        continue;
> +                }
> +                /* IO based */
> +                else
> +                {
> +                    pci_conf_write32(0, b, d, f, PCI_BASE_ADDRESS_0, ~0u);
> +                    len = pci_conf_read32(0, b, d, f, PCI_BASE_ADDRESS_0);
> +                    pci_conf_write32(0, b, d, f, 
> +                                     PCI_BASE_ADDRESS_0 + bar_idx*4, bar);
>  
> -                pci_conf_write32(0, b, d, f, PCI_BASE_ADDRESS_0, ~0u);
> -                len = pci_conf_read32(0, b, d, f, PCI_BASE_ADDRESS_0);
> -                pci_conf_write32(0, b, d, f, PCI_BASE_ADDRESS_0 + bar_idx*4, 
> bar);
> +                    /* Not 8 bytes */
> +                    if ( (len & 0xffff) != 0xfff9 )

If you touch this, please use suitable manifest constants here too.

Jan

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