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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.11.13 at 18:40, Aravind Gopalakrishnan 
>>> <Aravind.Gopalakrishnan@xxxxxxx> wrote:
>  static struct ns16550 {
> -    int baud, clock_hz, data_bits, parity, stop_bits, fifo_size, irq;
> +    int baud, clock_hz, data_bits, parity, stop_bits, irq;
>      u64 io_base;   /* I/O port or memory-mapped I/O address. */
> +    u32 fifo_size;

Is this in any way related to the main purpose of the patch here?
And if there is (i.e. in response to Andrew's comment on v1), then
I don't see why most/all of the other fields shouldn't follow suit.

>      u32 io_size;
> -    int reg_shift; /* Bits to shift register offset by */
> -    int reg_width; /* Size of access to use, the registers
> +    u32 reg_shift; /* Bits to shift register offset by */
> +    u32 reg_width; /* Size of access to use, the registers

As much as for fifo_size above, there's nothing here requiring
them to be fixed-size 32 bits. Please use unsigned int instead.

>                      * themselves are still bytes */
>      char __iomem *remapped_io_base;  /* Remapped virtual address of MMIO. */
>      /* UART with IRQ line: interrupt-driven I/O. */
>      struct irqaction irqaction;
> +    u32 lsr_mask;

And this one, if I'm not mistaken, would be u8 if meant to be fixed
size. Or else once again unsigned int.

> +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,
> +    },
> +};

Looks like this is used by pci_uart_config() only. Hence it could
be __initdata (and pci_uart_config() would then for consistency
also need to be marked __init - I don't know why it isn't already).

>                  /* 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;
> +
> +                        if ( bar_idx >= uart_config[i].max_bars )
> +                            break;

Did you not mean "continue" here?

> +
> +                        uart->reg_shift = uart_config[i].reg_shift;
> +                        uart->reg_width = uart_config[i].reg_width;
> +                        uart->fifo_size = uart_config[i].fifo_size;
> +                        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 )
> +                        continue;

I think this size checking actually should also be done in the MMIO
case.

Jan

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