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

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

On Tue, Mar 08, 2016 at 01:48:05AM -0700, Jan Beulich wrote:
> >>> 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

You mean:

                         param += uart_config[i].param;
+                        if ( !param->bar0 )
+                        {
+                            bar_idx = idx;
+                            port_idx = 0;
+                        }
?

The device in question (NetMos) is not on the uart_config list at all
so we won't get inside this loop.

> 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