Re: [PATCH V2] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips

[Aravind Gopalakrioshnan <aravind.gopalakrishnan@amd.com>]

On 11/15/2013 3:31 AM, Jan Beulich wrote:
>>>> On 14.11.13 at 18:40, Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com> wrote:
>>   static struct ns16550 {
>> -    int baud, clock_hz, data_bits, parity, stop_bits, fifo_size, irq;
>> +    int baud, clock_hz, data_bits, parity, stop_bits, irq;
>>       u64 io_base;   /* I/O port or memory-mapped I/O address. */
>> +    u32 fifo_size;
> Is this in any way related to the main purpose of the patch here?
> And if there is (i.e. in response to Andrew's comment on v1), then
> I don't see why most/all of the other fields shouldn't follow suit.

I don't mind changing them all in one patch.. But -

Since it is not directly related to the main purpose of the patch, would 
it be okay
if I did this in a follow up patch and go back to using 'int' for now?


>>       u32 io_size;
>> -    int reg_shift; /* Bits to shift register offset by */
>> -    int reg_width; /* Size of access to use, the registers
>> +    u32 reg_shift; /* Bits to shift register offset by */
>> +    u32 reg_width; /* Size of access to use, the registers
> As much as for fifo_size above, there's nothing here requiring
> them to be fixed-size 32 bits. Please use unsigned int instead.
>
>>                       * themselves are still bytes */
>>       char __iomem *remapped_io_base;  /* Remapped virtual address of MMIO. */
>>       /* UART with IRQ line: interrupt-driven I/O. */
>>       struct irqaction irqaction;
>> +    u32 lsr_mask;
> And this one, if I'm not mistaken, would be u8 if meant to be fixed
> size. Or else once again unsigned int.
>
>> +static struct ns16550_config_mmio uart_config[] =
>> +{
>> +    /* Broadcom TruManage device */
>> +    {
>> +        .vendor_id = 0x14e4,
>> +        .dev_id = 0x160a,
>> +        .reg_shift = 2,
>> +        .reg_width = 1,
>> +        .fifo_size = 64,
>> +        .lsr_mask = (UART_LSR_THRE | UART_LSR_TEMT),
>> +        .max_bars = 1,
>> +    },
>> +};
> Looks like this is used by pci_uart_config() only. Hence it could
> be __initdata (and pci_uart_config() would then for consistency
> also need to be marked __init - I don't know why it isn't already).

Will fix this.

>>                   /* 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;
>> +
>> +                        if ( bar_idx >= uart_config[i].max_bars )
>> +                            break;
> Did you not mean "continue" here?

No.. break is fine here since we only support bar0 for this specific 
device..
(By the time we reach this condition, we have already verified 'vendor' 
and 'device' from
uart_config table anyway.. it is needless to iterate through other 
devices in the table)

>> +
>> +                        uart->reg_shift = uart_config[i].reg_shift;
>> +                        uart->reg_width = uart_config[i].reg_width;
>> +                        uart->fifo_size = uart_config[i].fifo_size;
>> +                        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 )
>> +                        continue;
> I think this size checking actually should also be done in the MMIO
> case.
>
> Jan

This check would not work for this device as the length of region is 64K.

But if we really want to force a length check for MMIO cases as well,
we can define another field in struct uart_config and verify if length 
matches..
Do let me know what you make of it..

Meanwhile, I will fix other issues and have a V3 ready..

Thanks,
-Aravind.