| Message ID | 56CC508B02000078000D52B1@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> + [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?
>>> 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 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
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 >
>>> On 09.03.16 at 17:52, <konrad.wilk@oracle.com> wrote: > 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: >> > 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. Well, for devices not on the list it's undetermined anyway whether they would happen to work - we just can't get it right for all possible cases. Someone truly caring about them working should submit a patch adding them to the list. Jan
On Wed, Mar 09, 2016 at 10:01:08AM -0700, Jan Beulich wrote: > >>> On 09.03.16 at 17:52, <konrad.wilk@oracle.com> wrote: > > 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: > >> > 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. > > Well, for devices not on the list it's undetermined anyway whether > they would happen to work - we just can't get it right for all possible > cases. Someone truly caring about them working should submit a Wouldn't your patch cause a regression compared to how it used to work in earlier version of Xen? Let me take a peek at the card in question and see if I can hook up an extra RS232 connector. > patch adding them to the list. > > Jan > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
>>> On 11.03.16 at 03:31, <konrad@kernel.org> wrote: > On Wed, Mar 09, 2016 at 10:01:08AM -0700, Jan Beulich wrote: >> >>> On 09.03.16 at 17:52, <konrad.wilk@oracle.com> wrote: >> > 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: >> >> > 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. >> >> Well, for devices not on the list it's undetermined anyway whether >> they would happen to work - we just can't get it right for all possible >> cases. Someone truly caring about them working should submit a > > Wouldn't your patch cause a regression compared to how it used > to work in earlier version of Xen? It would, if you want to call such a regression (as I don't think it was determined to behave that way). But okay, I'll simply move the above conditional past the loop, to retain previous behavior. Jan
--- a/xen/drivers/char/ns16550.c +++ b/xen/drivers/char/ns16550.c @@ -78,10 +78,20 @@ static struct ns16550 { #endif } ns16550_com[2] = { { 0 } }; -struct ns16550_config_mmio { +#ifdef CONFIG_HAS_PCI +struct ns16550_config { u16 vendor_id; u16 dev_id; - unsigned int param; + enum { + param_default, /* Must not be referenced by any table entry. */ + param_trumanage, + param_oxford, + param_oxford_2port, + param_pericom_1port, + param_pericom_2port, + param_pericom_4port, + param_pericom_8port, + } param; }; /* Defining uart config options for MMIO devices */ @@ -90,57 +100,91 @@ struct ns16550_config_param { unsigned int reg_width; unsigned int fifo_size; u8 lsr_mask; - unsigned int max_bars; + bool_t mmio; + bool_t bar0; + unsigned int max_ports; unsigned int base_baud; unsigned int uart_offset; unsigned int first_offset; }; - -#ifdef CONFIG_HAS_PCI -enum { - param_default = 0, - param_trumanage, - param_oxford, - param_oxford_2port, -}; /* - * Create lookup tables for specific MMIO devices.. - * It is assumed that if the device found is MMIO, - * then you have indexed it here. Else, the driver - * does nothing. + * Create lookup tables for specific devices. It is assumed that if + * the device found is MMIO, then you have indexed it here. Else, the + * driver does nothing for MMIO based devices. */ static const struct ns16550_config_param __initconst uart_param[] = { - [param_default] = { }, /* Ignored. */ + [param_default] = { + .reg_width = 1, + .lsr_mask = UART_LSR_THRE, + .max_ports = 1, + }, [param_trumanage] = { .reg_shift = 2, .reg_width = 1, .fifo_size = 16, .lsr_mask = (UART_LSR_THRE | UART_LSR_TEMT), - .max_bars = 1, + .mmio = 1, + .max_ports = 1, }, [param_oxford] = { .base_baud = 4000000, .uart_offset = 0x200, .first_offset = 0x1000, .reg_width = 1, - .reg_shift = 0, .fifo_size = 16, .lsr_mask = UART_LSR_THRE, - .max_bars = 1, /* It can do more, but we would need more custom code.*/ + .mmio = 1, + .max_ports = 1, /* It can do more, but we would need more custom code.*/ }, [param_oxford_2port] = { .base_baud = 4000000, .uart_offset = 0x200, .first_offset = 0x1000, .reg_width = 1, - .reg_shift = 0, .fifo_size = 16, .lsr_mask = UART_LSR_THRE, - .max_bars = 2, + .mmio = 1, + .max_ports = 2, + }, + [param_pericom_1port] = { + .base_baud = 921600, + .uart_offset = 8, + .reg_width = 1, + .fifo_size = 16, + .lsr_mask = UART_LSR_THRE, + .bar0 = 1, + .max_ports = 1, + }, + [param_pericom_2port] = { + .base_baud = 921600, + .uart_offset = 8, + .reg_width = 1, + .fifo_size = 16, + .lsr_mask = UART_LSR_THRE, + .bar0 = 1, + .max_ports = 2, + }, + [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, } }; -static const struct ns16550_config_mmio __initconst uart_config[] = +static const struct ns16550_config __initconst uart_config[] = { /* Broadcom TruManage device */ { @@ -339,6 +383,30 @@ static const struct ns16550_config_mmio .vendor_id = PCI_VENDOR_ID_OXSEMI, .dev_id = 0xc4cf, .param = param_oxford, + }, + /* Pericom PI7C9X7951 Uno UART */ + { + .vendor_id = PCI_VENDOR_ID_PERICOM, + .dev_id = 0x7951, + .param = param_pericom_1port + }, + /* Pericom PI7C9X7952 Duo UART */ + { + .vendor_id = PCI_VENDOR_ID_PERICOM, + .dev_id = 0x7952, + .param = param_pericom_2port + }, + /* Pericom PI7C9X7954 Quad UART */ + { + .vendor_id = PCI_VENDOR_ID_PERICOM, + .dev_id = 0x7954, + .param = param_pericom_4port + }, + /* Pericom PI7C9X7958 Octal UART */ + { + .vendor_id = PCI_VENDOR_ID_PERICOM, + .dev_id = 0x7958, + .param = param_pericom_8port } }; #endif @@ -629,7 +697,8 @@ static void __init ns16550_init_postirq( uart->ps_bdf[2])); else { - if ( rangeset_add_range(mmio_ro_ranges, + if ( uart->param->mmio && + rangeset_add_range(mmio_ro_ranges, uart->io_base, uart->io_base + uart->io_size - 1) ) printk(XENLOG_INFO "Error while adding MMIO range of device to mmio_ro_ranges\n"); @@ -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; 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++) + { + 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; + } + + uart->io_base = 0; bar = pci_conf_read32(0, b, d, f, PCI_BASE_ADDRESS_0 + bar_idx*4); /* MMIO based */ - if ( !(bar & PCI_BASE_ADDRESS_SPACE_IO) ) + if ( param->mmio && !(bar & PCI_BASE_ADDRESS_SPACE_IO) ) { - u16 vendor = pci_conf_read16(0, b, d, f, PCI_VENDOR_ID); - u16 device = pci_conf_read16(0, b, d, f, PCI_DEVICE_ID); - pci_conf_write32(0, b, d, f, PCI_BASE_ADDRESS_0 + bar_idx*4, ~0u); len = pci_conf_read32(0, b, d, f, PCI_BASE_ADDRESS_0 + bar_idx*4); @@ -895,56 +988,11 @@ pci_uart_config(struct ns16550 *uart, bo else size = len & PCI_BASE_ADDRESS_MEM_MASK; - size &= -size; - - /* Check for params in uart_config lookup table */ - for ( i = 0; i < ARRAY_SIZE(uart_config); i++) - { - const struct ns16550_config_param *param; - - if ( uart_config[i].vendor_id != vendor ) - continue; - - if ( uart_config[i].dev_id != device ) - continue; - - param = uart_param + uart_config[i].param; - - /* - * Force length of mmio region to be at least - * 8 bytes times (1 << reg_shift) - */ - if ( size < (0x8 * (1 << param->reg_shift)) ) - continue; - - if ( bar_idx >= param->max_bars ) - continue; - - uart->param = param; - - if ( param->fifo_size ) - uart->fifo_size = param->fifo_size; - - uart->reg_shift = param->reg_shift; - uart->reg_width = param->reg_width; - uart->lsr_mask = param->lsr_mask; - uart->io_base = ((u64)bar_64 << 32) | - (bar & PCI_BASE_ADDRESS_MEM_MASK); - uart->io_base += param->first_offset; - uart->io_base += bar_idx * param->uart_offset; - if ( param->base_baud ) - uart->clock_hz = param->base_baud * 16; - size = max(8U << param->reg_shift, - param->uart_offset); - break; - } - - /* If we have an io_base, then we succeeded in the lookup */ - if ( !uart->io_base ) - continue; + uart->io_base = ((u64)bar_64 << 32) | + (bar & PCI_BASE_ADDRESS_MEM_MASK); } /* IO based */ - else + else if ( !param->mmio && (bar & PCI_BASE_ADDRESS_SPACE_IO) ) { pci_conf_write32(0, b, d, f, PCI_BASE_ADDRESS_0 + bar_idx*4, ~0u); @@ -952,22 +1000,45 @@ pci_uart_config(struct ns16550 *uart, bo pci_conf_write32(0, b, d, f, PCI_BASE_ADDRESS_0 + bar_idx*4, bar); size = len & PCI_BASE_ADDRESS_IO_MASK; - size &= -size; - - /* Not at least 8 bytes */ - if ( size < 8 ) - continue; uart->io_base = bar & ~PCI_BASE_ADDRESS_SPACE_IO; } + /* If we have an io_base, then we succeeded in the lookup. */ + if ( !uart->io_base ) + continue; + + size &= -size; + + /* + * Require length of actually used region to be at least + * 8 bytes times (1 << reg_shift). + */ + if ( size < param->first_offset + + port_idx * param->uart_offset + + (8 << param->reg_shift) ) + continue; + + uart->param = param; + + uart->reg_shift = param->reg_shift; + uart->reg_width = param->reg_width; + uart->lsr_mask = param->lsr_mask; + uart->io_base += param->first_offset + + port_idx * param->uart_offset; + if ( param->base_baud ) + uart->clock_hz = param->base_baud * 16; + if ( param->fifo_size ) + uart->fifo_size = param->fifo_size; + uart->ps_bdf[0] = b; uart->ps_bdf[1] = d; uart->ps_bdf[2] = f; uart->bar_idx = bar_idx; uart->bar = bar; uart->bar64 = bar_64; - uart->io_size = size; + uart->io_size = max(8U << param->reg_shift, + param->uart_offset); uart->irq = pci_conf_read8(0, b, d, f, PCI_INTERRUPT_PIN) ? pci_conf_read8(0, b, d, f, PCI_INTERRUPT_LINE) : 0; --- a/xen/include/xen/pci_ids.h +++ b/xen/include/xen/pci_ids.h @@ -2,6 +2,8 @@ #define PCI_VENDOR_ID_NVIDIA 0x10de +#define PCI_VENDOR_ID_PERICOM 0x12d8 + #define PCI_VENDOR_ID_OXSEMI 0x1415 #define PCI_VENDOR_ID_BROADCOM 0x14e4