[Xen-devel,v2] ns16550: Add ACPI support for ARM only

Parse the ACPI SPCR table and initialize the 16550 compatible serial port
for ARM only. Currently we only support one UART on ARM. Some fields like
PCI, flow control and so on we do not care yet on ARM are ignored.

Signed-off-by: Wei Xu<xuwei5@hisilicon.com>


---
Changes in v2:
- improve commit message
- remove the spcr initialization
- add comments for the uart initialization and configuration
- adjust the code style issue
- limit the code only built on ACPI and ARM
---
  xen/drivers/char/ns16550.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 60 insertions(+)

-- 2.8.1 .

On 21.01.2020 04:44, Wei Xu wrote:
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -1620,6 +1620,66 @@ DT_DEVICE_START(ns16550, "NS16550 UART", DEVICE_SERIAL)
>   DT_DEVICE_END
>   
>   #endif /* HAS_DEVICE_TREE */
> +#if defined(CONFIG_ACPI) && defined(CONFIG_ARM)

Blank line above here please.

> +#include <xen/acpi.h>
> +
> +static int __init ns16550_acpi_uart_init(const void *data)
> +{
> +    struct acpi_table_spcr *spcr;
> +    acpi_status status;
> +
> +    /* Same as the DT part.

Comment style (again below). Also there shouldn't be a blank line
until after _all_ declarations.

> +     * Only support one UART on ARM which happen to be ns16550_com[0].
> +     */
> +    struct ns16550 *uart = &ns16550_com[0];
> +
> +    status = acpi_get_table(ACPI_SIG_SPCR, 0,
> +                            (struct acpi_table_header **)&spcr);

Please avoid casts like this. Use more type-safe constructs like
container_of() instead.

> +    if ( ACPI_FAILURE(status) )
> +    {
> +        printk("ns16550: Failed to get SPCR table\n");

Is such a message warranted? I.e. wouldn't it trigger on all
systems not having the table, which is hardly what you/we want?

> +        return -EINVAL;

Also, is it really an error if there's no such table?

> +    }
> +
> +    ns16550_init_common(uart);
> +
> +    /* The baud rate is pre-configured by the firmware.
> +     * And currently the ACPI part is only targeting ARM so some fields
> +     * like PCI, flow control and so on we do not care yet are ignored.
> +     */

I'm no convinced though you can ignore some other fields. In
particular on v1 I recall pointing out that the GAS structure
has more fields you should look at. (Overall I'm not happy
with "and so on" here - please list all fields you mean to
ignore so that reviewers as well as future readers can judge
whether this is appropriate.)

> +    uart->baud = BAUD_AUTO;
> +    uart->data_bits = 8;
> +    uart->parity = spcr->parity;
> +    uart->stop_bits = spcr->stop_bits;
> +    uart->io_base = spcr->serial_port.address;
> +    uart->io_size = 8;
> +    uart->reg_shift = spcr->serial_port.bit_offset;
> +    uart->reg_width = 1;
> +
> +    /* The 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;
> +
> +    /*  Register with generic serial driver. */

Stray double blanks at the beginning of the comment.

> +    serial_register_uart(uart - ns16550_com, &ns16550_driver, uart);

I guess it's fine this way, but with "uart = &ns16550_com[0]" above
the construct looks more complicated than it needs to look.

Jan

On 21/01/2020 11:13, Jan Beulich wrote:
> 
>> +    if ( ACPI_FAILURE(status) )
>> +    {
>> +        printk("ns16550: Failed to get SPCR table\n");
> 
> Is such a message warranted? I.e. wouldn't it trigger on all
> systems not having the table, which is hardly what you/we want?
> 
>> +        return -EINVAL;
> 
> Also, is it really an error if there's no such table?

You can only be there if the arm-uart.c found an SPCR table. So this is 
a sanity check. Therefore I think the error message is warrant here.

Cheers,

On 21.01.2020 12:16, Julien Grall wrote:
> On 21/01/2020 11:13, Jan Beulich wrote:
>>
>>> +    if ( ACPI_FAILURE(status) )
>>> +    {
>>> +        printk("ns16550: Failed to get SPCR table\n");
>>
>> Is such a message warranted? I.e. wouldn't it trigger on all
>> systems not having the table, which is hardly what you/we want?
>>
>>> +        return -EINVAL;
>>
>> Also, is it really an error if there's no such table?
> 
> You can only be there if the arm-uart.c found an SPCR table. So this is 
> a sanity check. Therefore I think the error message is warrant here.

If so - fine. But from

ACPI_DEVICE_START(ans16550, "NS16550 UART", DEVICE_SERIAL)
    .class_type = ACPI_DBG2_16550_COMPATIBLE,
    .init = ns16550_acpi_uart_init,
ACPI_DEVICE_END

I can't see why this would be. Would you mind educating me?

Thanks, Jan

Hi Jan,

On 21/01/2020 11:25, Jan Beulich wrote:
> On 21.01.2020 12:16, Julien Grall wrote:
>> On 21/01/2020 11:13, Jan Beulich wrote:
>>>
>>>> +    if ( ACPI_FAILURE(status) )
>>>> +    {
>>>> +        printk("ns16550: Failed to get SPCR table\n");
>>>
>>> Is such a message warranted? I.e. wouldn't it trigger on all
>>> systems not having the table, which is hardly what you/we want?
>>>
>>>> +        return -EINVAL;
>>>
>>> Also, is it really an error if there's no such table?
>>
>> You can only be there if the arm-uart.c found an SPCR table. So this is
>> a sanity check. Therefore I think the error message is warrant here.
> 
> If so - fine. But from
> 
> ACPI_DEVICE_START(ans16550, "NS16550 UART", DEVICE_SERIAL)
>      .class_type = ACPI_DBG2_16550_COMPATIBLE,
>      .init = ns16550_acpi_uart_init,
> ACPI_DEVICE_END
> 
> I can't see why this would be. Would you mind educating me?

The bits you pasted only register a driver for any serial device with 
the class 16550_COMPATIBLE. This is up to an upper layer to decide how 
class_type will be interpreted.

As I mentioned in v1, in the case of serial, the class_type will be 
matches against the field interface_type in the SPCR table. This is done 
in the function acpi_uart_init().

The function will try to retrieve the SPCR table. If there is none, then 
it will bail out. Otherwise, it will call the generic device framework 
to lookup for the driver.

Therefore a serial driver can only be called with the SPCR table 
actually exists.

Cheers,

Hi Jan,

On 2020/1/21 19:13, Jan Beulich wrote:
> On 21.01.2020 04:44, Wei Xu wrote:
>> --- a/xen/drivers/char/ns16550.c
>> +++ b/xen/drivers/char/ns16550.c
>> @@ -1620,6 +1620,66 @@ DT_DEVICE_START(ns16550, "NS16550 UART", DEVICE_SERIAL)
>>   DT_DEVICE_END
>>   
>>   #endif /* HAS_DEVICE_TREE */
>> +#if defined(CONFIG_ACPI) && defined(CONFIG_ARM)
> 
> Blank line above here please.

OK.
I will add it.

> 
>> +#include <xen/acpi.h>
>> +
>> +static int __init ns16550_acpi_uart_init(const void *data)
>> +{
>> +    struct acpi_table_spcr *spcr;
>> +    acpi_status status;
>> +
>> +    /* Same as the DT part.
> 
> Comment style (again below). Also there shouldn't be a blank line
> until after _all_ declarations.

OK.
I will add a separate line leading with '*' as the comment beginning
and remove the blank line in the declarations.

> 
>> +     * Only support one UART on ARM which happen to be ns16550_com[0].
>> +     */
>> +    struct ns16550 *uart = &ns16550_com[0];
>> +
>> +    status = acpi_get_table(ACPI_SIG_SPCR, 0,
>> +                            (struct acpi_table_header **)&spcr);
> 
> Please avoid casts like this. Use more type-safe constructs like
> container_of() instead.
> 
>> +    if ( ACPI_FAILURE(status) )
>> +    {
>> +        printk("ns16550: Failed to get SPCR table\n");
> 
> Is such a message warranted? I.e. wouldn't it trigger on all
> systems not having the table, which is hardly what you/we want?
> 
>> +        return -EINVAL;
> 
> Also, is it really an error if there's no such table?
> 
>> +    }
>> +
>> +    ns16550_init_common(uart);
>> +
>> +    /* The baud rate is pre-configured by the firmware.
>> +     * And currently the ACPI part is only targeting ARM so some fields
>> +     * like PCI, flow control and so on we do not care yet are ignored.
>> +     */
> 
> I'm no convinced though you can ignore some other fields. In
> particular on v1 I recall pointing out that the GAS structure
> has more fields you should look at. (Overall I'm not happy
> with "and so on" here - please list all fields you mean to
> ignore so that reviewers as well as future readers can judge
> whether this is appropriate.)
>

OK.
I will investigate and list all the ignore fields.

>> +    uart->baud = BAUD_AUTO;
>> +    uart->data_bits = 8;
>> +    uart->parity = spcr->parity;
>> +    uart->stop_bits = spcr->stop_bits;
>> +    uart->io_base = spcr->serial_port.address;
>> +    uart->io_size = 8;
>> +    uart->reg_shift = spcr->serial_port.bit_offset;
>> +    uart->reg_width = 1;
>> +
>> +    /* The 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;
>> +
>> +    /*  Register with generic serial driver. */
> 
> Stray double blanks at the beginning of the comment.
>

Sorry, I will remove it.

>> +    serial_register_uart(uart - ns16550_com, &ns16550_driver, uart);
> 
> I guess it's fine this way, but with "uart = &ns16550_com[0]" above
> the construct looks more complicated than it needs to look.

Yes, I can change to use "SERHND_DTUART".
Thanks for you guidance!

Best Regards,
Wei

> 
> Jan
> 
> .
>