[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH V3] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips
>>> On 20.11.13 at 01:54, Aravind Gopalakrishnan >>> <Aravind.Gopalakrishnan@xxxxxxx> wrote: > @@ -77,6 +78,37 @@ static struct ns16550 { > #endif > } ns16550_com[2] = { { 0 } }; > > +/* Defining uart config options for MMIO devices */ > +struct ns16550_config_mmio { > + u16 vendor_id; > + u16 dev_id; > + int reg_shift; > + int reg_width; > + int fifo_size; As Andrew had pointed out before, and as I think I explained before too - even if you don't adjust existing code in this patch, you shouldn't introduce further bogus signed integers here. > @@ -546,11 +580,12 @@ static int __init check_existence(struct ns16550 *uart) > } > > #ifdef HAS_PCI > -static int > +static int __init > pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx) > { > uint32_t bar, len; > - int b, d, f, nextf; > + int b, d, f, nextf, i; This "i" is another case of a variable clearly wanting to be unsigned. If making all the other four variables unsigned is okay (which I think it is), then just do it all in one go. Otherwise you'd have to add a new line rather than adding to the existing declaration. > @@ -581,22 +616,67 @@ pci_uart_config (struct ns16550 *uart, int skip_amt, > int bar_idx) > > /* Not IO */ > if ( !(bar & PCI_BASE_ADDRESS_SPACE_IO) ) > - continue; > + { > + vendor = pci_conf_read16(0, b, d, f, PCI_VENDOR_ID); > + device = pci_conf_read16(0, b, d, f, PCI_DEVICE_ID); > + > + /* Check for quirks in uart_config lookup table */ > + for ( i = 0; i < ARRAY_SIZE(uart_config); i++) > + { > + if ( uart_config[i].vendor_id != vendor ) > + continue; > + > + if ( uart_config[i].dev_id != device ) > + continue; > + > + pci_conf_write32(0, b, d, f, PCI_BASE_ADDRESS_0, > ~0u); > + len = pci_conf_read32(0, b, d, f, > PCI_BASE_ADDRESS_0); > + pci_conf_write32(0, b, d, f, > + PCI_BASE_ADDRESS_0 + bar_idx*4, > bar); > + len &= ~(0xf); I'm sure there a manifest constant for this. > + uart->io_size = len & ~(len - 1); Why not just "-len"? Also you're not taking 64-bit BARs into consideration here. Minimally you need to bail upon encountering one. > + > + /* Force length of mmio region to be atleast 8 bytes > */ "at least" > + if ( uart->io_size < 0x8 ) > + continue; > + > + if ( bar_idx >= uart_config[i].max_bars ) > + continue; I suppose this would better be moved up. > + > + if (uart_config[i].fifo_size ) Coding style. > + uart->fifo_size = uart_config[i].fifo_size; > + > + uart->reg_shift = uart_config[i].reg_shift; > + uart->reg_width = uart_config[i].reg_width; > + uart->lsr_mask = uart_config[i].lsr_mask; > + uart->io_base = bar & PCI_BASE_ADDRESS_MEM_MASK; > + break; > + } > + > + /* If we have an io_base, then we succeeded in the > lookup */ > + if ( !uart->io_base ) > + continue; > + } > + /* IO based */ > + else > + { > + pci_conf_write32(0, b, d, f, PCI_BASE_ADDRESS_0, ~0u); > + len = pci_conf_read32(0, b, d, f, PCI_BASE_ADDRESS_0); > + pci_conf_write32(0, b, d, f, > + PCI_BASE_ADDRESS_0 + bar_idx*4, bar); > > - pci_conf_write32(0, b, d, f, PCI_BASE_ADDRESS_0, ~0u); > - len = pci_conf_read32(0, b, d, f, PCI_BASE_ADDRESS_0); > - pci_conf_write32(0, b, d, f, PCI_BASE_ADDRESS_0 + bar_idx*4, > bar); > + /* Not 8 bytes */ > + if ( (len & 0xffff) != 0xfff9 ) If you touch this, please use suitable manifest constants here too. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |