Re: [Xen-devel] [PATCH 2/4] ns16550: enable Pericom controller support

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

> +    [param_pericom_4port] = {
> +        .base_baud = 921600,
> +        .uart_offset = 8,
> +        .reg_width = 1,
> +        .fifo_size = 16,
> +        .lsr_mask = UART_LSR_THRE,
> +        .bar0 = 1,
> +        .max_ports = 4,
> +    },
> +    [param_pericom_8port] = {
> +        .base_baud = 921600,
> +        .uart_offset = 8,
> +        .reg_width = 1,
> +        .fifo_size = 16,
> +        .lsr_mask = UART_LSR_THRE,
> +        .bar0 = 1,
> +        .max_ports = 8,

Perhaps document that Xen can only access two of the ports? Unless we
expand the ns16550_com array of course.

> @@ -830,12 +899,11 @@ static int __init check_existence(struct
>  
>  #ifdef CONFIG_HAS_PCI
>  static int __init
> -pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int bar_idx)
> +pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx)
>  {
>      u64 orig_base = uart->io_base;
>      unsigned int b, d, f, nextf, i;
>  
> -    uart->io_base = 0;
>      /* 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++ )
>      {
> @@ -843,8 +911,10 @@ pci_uart_config(struct ns16550 *uart, bo
>          {
>              for ( f = 0; f < 8; f = nextf )
>              {
> +                unsigned int bar_idx = 0, port_idx = idx;

s/port_idx/port/? or port_nr /?

>                  uint32_t bar, bar_64 = 0, len, len_64;
> -                u64 size;
> +                u64 size = 0;
> +                const struct ns16550_config_param *param = uart_param;
>  
>                  nextf = (f || (pci_conf_read16(0, b, d, f, PCI_HEADER_TYPE) &
>                                 0x80)) ? f + 1 : 8;
> @@ -863,15 +933,38 @@ pci_uart_config(struct ns16550 *uart, bo
>                      continue;
>                  }
>  
> +                /* Check for params in uart_config lookup table */
> +                for ( i = 0; i < ARRAY_SIZE(uart_config); i++)

I am pretty sure I wrote this piece of code - could you fix the
Style on it please? The i++) please?
> +                {
> +                    u16 vendor = pci_conf_read16(0, b, d, f, PCI_VENDOR_ID);
> +                    u16 device = pci_conf_read16(0, b, d, f, PCI_DEVICE_ID);
> +
> +                    if ( uart_config[i].vendor_id == vendor &&
> +                         uart_config[i].dev_id == device )
> +                    {
> +                        param += uart_config[i].param;
> +                        if ( !param->bar0 )
> +                        {
> +                            bar_idx = idx;
> +                            port_idx = 0;
> +                        }
> +                        break;
> +                    }
> +                }
> +
> +                if ( port_idx >= param->max_ports )
> +                {
> +                    idx -= param->max_ports;
> +                    continue;

Could you add a comment about this? I understand it can detect if we are
using an AMT device with the 'com2=115200,8n1,amt' (which would be
invalid - AMT devices only have one IO PORT and there is only one of
them on the machine) we would skip over the found device and continue on..
Thought I don't understand why we want to decrease the idx value from one to 
zero?

Hmm, if it was some other PCI based serial card like:

01:05.0 Serial controller: NetMos Technology PCI 9835 Multi-I/O
Controller (rev 01) (prog-if 02 [16550])
        Subsystem: LSI Logic / Symbios Logic Device 0001
        Flags: medium devsel, IRQ 20
        I/O ports at e050 [size=8]
        I/O ports at e040 [size=8]
        I/O ports at e030 [size=8]
        I/O ports at e020 [size=8]
        I/O ports at e010 [size=8]
        I/O ports at e000 [size=16]

With 'com1=115200,8n1,pci' and 'com2=115200,8n1,pci' then the first loop
would find the device. The second loop would decrement idx (1) by 1 and
continue.. which would make it go search for another device.

I hadn't tested this patch on the above device but I believe it used
to work with the com1 and com2 going throught it - while with the new code
it won't?


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