From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Ian Campbell <Ian.Campbell@eu.citrix.com>,
xen-devel <xen-devel@lists.xenproject.org>,
Keir Fraser <keir@xen.org>,
Ian Jackson <Ian.Jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>
Subject: Re: [PATCH 2/4] ns16550: enable Pericom controller support
Date: Wed, 9 Mar 2016 11:52:54 -0500 [thread overview]
Message-ID: <20160309165254.GA25040@char.us.oracle.com> (raw)
In-Reply-To: <56DE9FD502000078000DA417@prv-mh.provo.novell.com>
On Tue, Mar 08, 2016 at 01:48:05AM -0700, Jan Beulich wrote:
> >>> On 07.03.16 at 23:04, <konrad.wilk@oracle.com> 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@lists.xen.org
http://lists.xen.org/xen-devel
next prev parent reply other threads:[~2016-03-09 16:53 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-23 11:22 [PATCH 0/4] ns16550: enable support for Pericom controllers Jan Beulich
2016-02-23 11:28 ` [PATCH 1/4] ns16550: store pointer to config parameters for PCI Jan Beulich
2016-03-07 21:06 ` Konrad Rzeszutek Wilk
2016-02-23 11:28 ` [PATCH 2/4] ns16550: enable Pericom controller support Jan Beulich
2016-03-07 22:04 ` Konrad Rzeszutek Wilk
2016-03-08 8:48 ` Jan Beulich
2016-03-09 16:52 ` Konrad Rzeszutek Wilk [this message]
2016-03-09 17:01 ` Jan Beulich
2016-03-11 2:31 ` Konrad Rzeszutek Wilk
2016-03-11 11:02 ` Jan Beulich
2016-03-22 13:19 ` [PATCH v2 " Jan Beulich
2016-03-28 14:46 ` Konrad Rzeszutek Wilk
2016-02-23 11:30 ` [PATCH 3/4] console: adjust IRQ initialization Jan Beulich
2016-03-07 22:10 ` Konrad Rzeszutek Wilk
2016-02-23 11:30 ` [PATCH RFC 4/4] ns16550: enable use of PCI MSI Jan Beulich
2016-02-29 16:56 ` [PATCH 0/4] ns16550: enable support for Pericom controllers Konrad Rzeszutek Wilk
2016-02-29 17:03 ` Jan Beulich
2016-03-04 13:19 ` Ping: " Jan Beulich
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160309165254.GA25040@char.us.oracle.com \
--to=konrad.wilk@oracle.com \
--cc=Ian.Campbell@eu.citrix.com \
--cc=Ian.Jackson@eu.citrix.com \
--cc=JBeulich@suse.com \
--cc=keir@xen.org \
--cc=tim@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).