Re: [Xen-devel] [PATCH 2/3 v3] xen: Add support for initializing 16550 UART using ACPI

[Andre Przywara <andre.przywara@xxxxxxxxxx>]

Hi,

(answering to both Konrad and Bhupinder ...)

On 24/11/17 13:50, Konrad Rzeszutek Wilk wrote:
> On Fri, Nov 24, 2017 at 05:09:11PM +0530, Bhupinder Thakur wrote:
>> Currently, Xen supports only DT based initialization of 16550 UART.
>> This patch adds support for initializing 16550 UART using ACPI SPCR table.
> 
> Can you provide the link to the spec you are using. I am wondering
> if I am looking at some older one.
> 
>>
>> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>
>> ---
>> Changes since v2:
>> - renamed UART_MAX_REG to UART_NUM_REGS
>> - aligned some assignment statements
>> - some coding style changes
>>
>> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> CC: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
>> CC: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
>> CC: Jan Beulich <jbeulich@xxxxxxxx>
>> CC: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
>> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> CC: Tim Deegan <tim@xxxxxxx>
>> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
>> CC: Julien Grall <julien.grall@xxxxxxx>
>>
>>  xen/drivers/char/ns16550.c  | 67 
>> +++++++++++++++++++++++++++++++++++++++++++++
>>  xen/include/xen/8250-uart.h |  1 +
>>  2 files changed, 68 insertions(+)
>>
>> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
>> index c5dfc1e..af4712f 100644
>> --- a/xen/drivers/char/ns16550.c
>> +++ b/xen/drivers/char/ns16550.c
>> @@ -29,6 +29,10 @@
>>  #ifdef CONFIG_X86
>>  #include <asm/fixmap.h>
>>  #endif
>> +#ifdef CONFIG_ACPI
>> +#include <xen/acpi.h>
>> +#endif
>> +
>>  
>>  /*
>>   * Configure serial port with a string:
>> @@ -1565,6 +1569,69 @@ DT_DEVICE_START(ns16550, "NS16550 UART", 
>> DEVICE_SERIAL)
>>  DT_DEVICE_END
>>  
>>  #endif /* HAS_DEVICE_TREE */
>> +
>> +#if defined(CONFIG_ACPI) && defined(CONFIG_ARM)
> 
> I would remove the CONFIG_ARM as the spec says it is possible to use
> this on ARM _and_ x86 hardware.

I was thinking the same. Originally the SPCR was x86 only.

> But I guess you can't as ACPI_DEVICE_START is defined to be only
> on ARM?

We could move the #ifdef there.

>> +
>> +static int ns16550_init_acpi(struct ns16550 **puart)
>> +{
>> +    struct acpi_table_spcr *spcr;
>> +    int status;
> 
> hmm, not acpi_status ?
>> +    struct ns16550 *uart = &ns16550_com[0];
>> +
>> +    ns16550_init_common(uart);
> 
> I would move this below the error check below..
>> +
>> +    status = acpi_get_table(ACPI_SIG_SPCR, 0,
>> +                            (struct acpi_table_header **)&spcr);
>> +
>> +    if ( ACPI_FAILURE(status) )
>> +    {
>> +        printk("ns16550: Failed to get SPCR table\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    uart->baud      = BAUD_AUTO;
>> +    uart->data_bits = 8;
> 
> Are those two assumed from the ACPI spec?

I can't find a field for the number of data bits, so I assume it's
indeed fixed to 8.

> Wait a minute. The
> https://msdn.microsoft.com/en-us/library/windows/hardware/dn639132(v=vs.85).aspx
> has Baud Rate at Offset 58?

Yes, I see the same. We should use that table.
Just wondering what the Moonshot gives you in this table? Is it "7"?

>> +    uart->parity    = spcr->parity;

The Spec is a bit weird there, since it only specifies 0 as "No parity",
every other value is reserved.
Shall we check and bail out if !0 with an error message?

>> +    uart->stop_bits = spcr->stop_bits;

Again if you want to be strict you would need to check against the
specified values, which means only "1" is valid.

>> +    uart->io_base   = spcr->serial_port.address;

I think this needs to be checked against the address type, because we
assume 0 (MMIO) here for ARM, and I guess 1 (PIO) for x86?

If I understand the code correctly, we have some wild heuristics to
guess the address type at the moment?

#ifdef CONFIG_HAS_IOPORTS
    /* I/O ports are distinguished by their size (16 bits). */
    if ( uart->io_base >= 0x10000 )
#endif

I wonder if we should store this explicitly, since ACPI (and DT as well)
give us this information.

>> +    uart->irq       = spcr->interrupt;
> 
> You need to check if the 'Interrupt Type' field is set before
> you look at this?
> 
>> +    uart->reg_width = spcr->serial_port.bit_width / 8;

I am a bit confused about the SPCR field here. Shouldn't this be encoded
in the "Access Size" field instead?
"Register Bit Width: The size in bits of the given register. When
addressing a data structure, this field must be zero."
But I guess the Moonshot gives you 4 in here, but something else in
"Access Size"?

Cheers,
Andre.

>> +    uart->reg_shift = 0;
>> +    uart->io_size   = UART_NUM_REGS << uart->reg_shift;
>> +
>> +    irq_set_type(spcr->interrupt, spcr->interrupt_type);
> 
> Um, the spec has a whole bunch of other bits set in 'interrupt_type'?
> 
>> +
>> +    *puart = uart;
>> +
>> +    return 0;
>> +}
>> +
>> +static int __init ns16550_acpi_uart_init(const void *data)
>> +{
>> +    int ret;
>> +    struct ns16550 *uart;
>> +
>> +    ret = ns16550_init_acpi(&uart);
>> +    if ( ret )
>> +        return ret;
>> +
>> +    ns16550_vuart_init(uart);
>> +
>> +    ns16550_register_uart(uart);
>> +
>> +    return 0;
>> +}
>> +
>> +ACPI_DEVICE_START(ns16550c, "16550 COMPAT UART", DEVICE_SERIAL)
>> +        .class_type = ACPI_DBG2_16550_COMPATIBLE,
>> +        .init = ns16550_acpi_uart_init,
>> +ACPI_DEVICE_END
>> +ACPI_DEVICE_START(ns16550s, "16550 SUBSET UART", DEVICE_SERIAL)
>> +        .class_type = ACPI_DBG2_16550_SUBSET,
>> +        .init = ns16550_acpi_uart_init,
>> +ACPI_DEVICE_END
>> +
>> +#endif
>>  /*
>>   * Local variables:
>>   * mode: C
>> diff --git a/xen/include/xen/8250-uart.h b/xen/include/xen/8250-uart.h
>> index 5c3bac3..849a5c0 100644
>> --- a/xen/include/xen/8250-uart.h
>> +++ b/xen/include/xen/8250-uart.h
>> @@ -35,6 +35,7 @@
>>  #define UART_USR          0x1f    /* Status register (DW) */
>>  #define UART_DLL          0x00    /* divisor latch (ls) (DLAB=1) */
>>  #define UART_DLM          0x01    /* divisor latch (ms) (DLAB=1) */
>> +#define UART_NUM_REGS     (UART_USR + 1)
>>  
>>  /* Interrupt Enable Register */
>>  #define UART_IER_ERDAI    0x01    /* rx data recv'd       */
>> -- 
>> 2.7.4
>>
>>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel