[Xen-devel] ns16550: Add ACPI support

Parse the ACPI SPCR table and initialize the 16550 compatible
serial port.

Signed-off-by: Wei Xu <xuwei5@hisilicon.com>
---
 xen/drivers/char/ns16550.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

On 17.01.2020 04:40, Wei Xu wrote:
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -1620,6 +1620,61 @@ DT_DEVICE_START(ns16550, "NS16550 UART", DEVICE_SERIAL)
>  DT_DEVICE_END
>  
>  #endif /* HAS_DEVICE_TREE */
> +
> +#ifdef CONFIG_ACPI
> +#include <xen/acpi.h>
> +
> +static int __init ns16550_acpi_uart_init(const void *data)
> +{
> +    struct acpi_table_spcr *spcr = NULL;

The initializer isn't strictly needed, is it?

> +    acpi_status status;
> +    struct ns16550 *uart;
> +
> +    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 = &ns16550_com[0];

You want to justify the choice of what (on x86 at least= we'd call
com1 in the patch description. Also this could be the initializer
of the variable.

> +    ns16550_init_common(uart);
> +
> +    uart->baud      = BAUD_AUTO;

There's a baud_rate field in the structure. If there's a reason
to ignore it, please add a comment.

There's also an interface_type field - can you really ignore it?

> +    uart->data_bits = 8;
> +    uart->parity    = spcr->parity;
> +    uart->stop_bits = spcr->stop_bits;

There's also a flow_control field, which I think needs checking
that it matches ns16550_setup_preirq() comment:

    /* No flow ctrl: DTR and RTS are both wedged high to keep remote happy. */

Similarly any other fields you don't evaluate at all and which
aren't explained by the spec as possible to be ignored (and the
situation matching the use case, like you not caring about PCI
aspects here) need reasoning about in the description or a code
comment.

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

The field (or perhaps the whole spcr->serial_port) being zero looks
to have special meaning.

> +    uart->io_size = 8;
> +    uart->reg_shift = spcr->serial_port.bit_offset;

spcr->serial_port has other fields which I don't think you should
ignore.

> +    uart->reg_width = 1;

Please use consistent placement of = : Either all of them are
aligned, or all of them are preceded by a single space only.

> +    /* trigger/polarity information is not available in spcr */
> +    irq_set_type(spcr->interrupt, IRQ_TYPE_LEVEL_HIGH);
> +    uart->irq = spcr->interrupt;
> +
> +    uart->vuart.base_addr = uart->io_base;
> +    uart->vuart.size = uart->io_size;
> +    uart->vuart.data_off = UART_THR << uart->reg_shift;
> +    uart->vuart.status_off = UART_LSR << uart->reg_shift;
> +    uart->vuart.status = UART_LSR_THRE | UART_LSR_TEMT;

Style-wise this block should then match whatever the other
block above looks.

> +    /*  Register with generic serial driver. */
> +    serial_register_uart(uart - ns16550_com, &ns16550_driver, uart);
> +
> +    return 0;
> +}
> +
> +ACPI_DEVICE_START(ans16550, "NS16550 UART", DEVICE_SERIAL)
> +    .class_type = ACPI_DBG2_16550_COMPATIBLE,
> +    .init = ns16550_acpi_uart_init,
> +ACPI_DEVICE_END

I don't expect this to build on x86.

Finally, please follow patch submission guidelines: Patches ought to
be sent _to_ the list and maintainers be _cc_-ed.

Jan

Hi Jan,

On 17/01/2020 08:33, Jan Beulich wrote:
> On 17.01.2020 04:40, Wei Xu wrote:
>> --- a/xen/drivers/char/ns16550.c
>> +++ b/xen/drivers/char/ns16550.c
>> @@ -1620,6 +1620,61 @@ DT_DEVICE_START(ns16550, "NS16550 UART", DEVICE_SERIAL)
>>   DT_DEVICE_END
>>   
>>   #endif /* HAS_DEVICE_TREE */
>> +
>> +#ifdef CONFIG_ACPI
>> +#include <xen/acpi.h>
>> +
>> +static int __init ns16550_acpi_uart_init(const void *data)
>> +{
>> +    struct acpi_table_spcr *spcr = NULL;
> 
> The initializer isn't strictly needed, is it?
> 
>> +    acpi_status status;
>> +    struct ns16550 *uart;
>> +
>> +    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 = &ns16550_com[0];
> 
> You want to justify the choice of what (on x86 at least= we'd call
> com1 in the patch description. Also this could be the initializer
> of the variable.

This is the same choice as we made for the DT binding (see 
ns16550_uart()). We only support one UART on Arm which happen to be 
ns16550_com[0] (but named diferrently).

The code below is actually quite similar to the DT parsing, so maybe we 
want to provide a common helper here.

> 
>> +    ns16550_init_common(uart);
>> +
>> +    uart->baud      = BAUD_AUTO;
> 
> There's a baud_rate field in the structure. If there's a reason
> to ignore it, please add a comment.

Same as for the DT part, we assume the firmware will configure the UART 
correctly.

> 
> There's also an interface_type field - can you really ignore it?

It is not ignored. This is used by the upper layer to detect which uart 
driver to call (see acpi_uart_init() in arm-uart.c).
> 
>> +    uart->data_bits = 8;
>> +    uart->parity    = spcr->parity;
>> +    uart->stop_bits = spcr->stop_bits;
> 
> There's also a flow_control field, which I think needs checking
> that it matches ns16550_setup_preirq() comment:
> 
>      /* No flow ctrl: DTR and RTS are both wedged high to keep remote happy. */
> 
> Similarly any other fields you don't evaluate at all and which
> aren't explained by the spec as possible to be ignored (and the
> situation matching the use case, like you not caring about PCI
> aspects here) need reasoning about in the description or a code
> comment.
What's missing in the commit message is the fact this is only targeting 
Arm. So there are a lot we don't care yet (such as PCI).

> 
>> +    uart->io_base = spcr->serial_port.address;
> 
> The field (or perhaps the whole spcr->serial_port) being zero looks
> to have special meaning.
> 
>> +    uart->io_size = 8;
>> +    uart->reg_shift = spcr->serial_port.bit_offset;
> 
> spcr->serial_port has other fields which I don't think you should
> ignore.
> 
>> +    uart->reg_width = 1;
> 
> Please use consistent placement of = : Either all of them are
> aligned, or all of them are preceded by a single space only.
> 
>> +    /* trigger/polarity information is not available in spcr */
>> +    irq_set_type(spcr->interrupt, IRQ_TYPE_LEVEL_HIGH);
>> +    uart->irq = spcr->interrupt;
>> +
>> +    uart->vuart.base_addr = uart->io_base;
>> +    uart->vuart.size = uart->io_size;
>> +    uart->vuart.data_off = UART_THR << uart->reg_shift;
>> +    uart->vuart.status_off = UART_LSR << uart->reg_shift;
>> +    uart->vuart.status = UART_LSR_THRE | UART_LSR_TEMT;
> 
> Style-wise this block should then match whatever the other
> block above looks.
> 
>> +    /*  Register with generic serial driver. */
>> +    serial_register_uart(uart - ns16550_com, &ns16550_driver, uart);
>> +
>> +    return 0;
>> +}
>> +
>> +ACPI_DEVICE_START(ans16550, "NS16550 UART", DEVICE_SERIAL)
>> +    .class_type = ACPI_DBG2_16550_COMPATIBLE,
>> +    .init = ns16550_acpi_uart_init,
>> +ACPI_DEVICE_END
> 
> I don't expect this to build on x86.

This is only meant to target Arm. So maybe we want to protect the whole 
code with defined(CONFIG_ACPI) && defined(CONFIG_ARM).

Cheers,

On 18.01.2020 13:32, Julien Grall wrote:
> On 17/01/2020 08:33, Jan Beulich wrote:
>> On 17.01.2020 04:40, Wei Xu wrote:
>>> --- a/xen/drivers/char/ns16550.c
>>> +++ b/xen/drivers/char/ns16550.c
>>> @@ -1620,6 +1620,61 @@ DT_DEVICE_START(ns16550, "NS16550 UART", DEVICE_SERIAL)
>>>   DT_DEVICE_END
>>>   
>>>   #endif /* HAS_DEVICE_TREE */
>>> +
>>> +#ifdef CONFIG_ACPI
>>> +#include <xen/acpi.h>
>>> +
>>> +static int __init ns16550_acpi_uart_init(const void *data)
>>> +{
>>> +    struct acpi_table_spcr *spcr = NULL;
>>> +    acpi_status status;
>>> +    struct ns16550 *uart;
>>> +
>>> +    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 = &ns16550_com[0];
>>
>> You want to justify the choice of what (on x86 at least= we'd call
>> com1 in the patch description. Also this could be the initializer
>> of the variable.
> 
> This is the same choice as we made for the DT binding (see 
> ns16550_uart()). We only support one UART on Arm which happen to be 
> ns16550_com[0] (but named diferrently).
> 
> The code below is actually quite similar to the DT parsing, so maybe we 
> want to provide a common helper here.

That's all fine, but doesn't eliminate the need to say so in the
description.

>>> +    /*  Register with generic serial driver. */
>>> +    serial_register_uart(uart - ns16550_com, &ns16550_driver, uart);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +ACPI_DEVICE_START(ans16550, "NS16550 UART", DEVICE_SERIAL)
>>> +    .class_type = ACPI_DBG2_16550_COMPATIBLE,
>>> +    .init = ns16550_acpi_uart_init,
>>> +ACPI_DEVICE_END
>>
>> I don't expect this to build on x86.
> 
> This is only meant to target Arm. So maybe we want to protect the whole 
> code with defined(CONFIG_ACPI) && defined(CONFIG_ARM).

Indeed, that's what the remark was aiming at.

Jan

Hi Jan, Julien,

On 2020/1/20 16:38, Jan Beulich wrote:
> On 18.01.2020 13:32, Julien Grall wrote:
>> On 17/01/2020 08:33, Jan Beulich wrote:
>>> On 17.01.2020 04:40, Wei Xu wrote:
>>>> --- a/xen/drivers/char/ns16550.c
>>>> +++ b/xen/drivers/char/ns16550.c
>>>> @@ -1620,6 +1620,61 @@ DT_DEVICE_START(ns16550, "NS16550 UART", DEVICE_SERIAL)
>>>>    DT_DEVICE_END
>>>>    
>>>>    #endif /* HAS_DEVICE_TREE */
>>>> +
>>>> +#ifdef CONFIG_ACPI
>>>> +#include <xen/acpi.h>
>>>> +
>>>> +static int __init ns16550_acpi_uart_init(const void *data)
>>>> +{
>>>> +    struct acpi_table_spcr *spcr = NULL;
>>>> +    acpi_status status;
>>>> +    struct ns16550 *uart;
>>>> +
>>>> +    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 = &ns16550_com[0];
>>> You want to justify the choice of what (on x86 at least= we'd call
>>> com1 in the patch description. Also this could be the initializer
>>> of the variable.
>> This is the same choice as we made for the DT binding (see
>> ns16550_uart()). We only support one UART on Arm which happen to be
>> ns16550_com[0] (but named diferrently).
>>
>> The code below is actually quite similar to the DT parsing, so maybe we
>> want to provide a common helper here.
> That's all fine, but doesn't eliminate the need to say so in the
> description.
>
>>>> +    /*  Register with generic serial driver. */
>>>> +    serial_register_uart(uart - ns16550_com, &ns16550_driver, uart);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +ACPI_DEVICE_START(ans16550, "NS16550 UART", DEVICE_SERIAL)
>>>> +    .class_type = ACPI_DBG2_16550_COMPATIBLE,
>>>> +    .init = ns16550_acpi_uart_init,
>>>> +ACPI_DEVICE_END
>>> I don't expect this to build on x86.
>> This is only meant to target Arm. So maybe we want to protect the whole
>> code with defined(CONFIG_ACPI) && defined(CONFIG_ARM).
> Indeed, that's what the remark was aiming at.
>
> Jan
>
> .
>

Thanks!
I will address the comments in V2.

Best Regards,
Wei