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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.03.16 at 23:04, <konrad.wilk@xxxxxxxxxx> wrote:
>>  +    [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.

Done.

>> @@ -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 /?

"port" would be misleading/ambiguous, and I don't see port_nr being
any better than port_idx (or if so, it ought to then also be bar_nr).
In fact, "nr" - other than "idx" - is ambiguous too (commonly
indicating "number of ...").

>> @@ -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?

Sure.

>> +                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?

If we're looking for COM2 and have found a 1-port card, we want to
use the 1st (rather than the 2nd) port on the next card we may find
(if any). This seems pretty obvious behavior to me here, so I'm not
really convinced a comment is warranted.

> 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?

That's the !bar0 case, and hence the code in the loop over
uart_config[] would set port_idx to zero, so the conditional above
won't evaluate to true anyway. I.e. no change in behavior over
the original code (albeit arguably that behavior is not fully correct,
at least if we consider arbitrary bar_idx values - right now it can
only be 0 or 1 -, since some skipping logic would then be needed
too). The question is whether we shouldn't have all single port
cards have their bar0 flag set to true (or extend the conditional
inside the loop to "!param->bar0 && param->max_ports > 1"), to
enable this skipping in all of those cases.

Jan


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