Re: [Xen-devel] [PATCH v3] ns16550: Add ACPI support for ARM only

[Wei Xu <xuwei5@xxxxxxxxxxxxx>]

Hi Jan,

On 2020/2/17 21:53, Jan Beulich wrote:
> On 03.02.2020 12:21, Wei Xu wrote:
>> 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
>> which we do not care yet on ARM are ignored.
>>
>> Signed-off-by: Wei Xu <xuwei5@xxxxxxxxxxxxx>
>>
>> ---
>> Changes in v3:
>> - address the code style comments from Jan
>> - use container_of to do cast
>> - list all fields we ignored
>> - check the console redirection is disabled or not before init the uart
>> - init the uart io_size and width via spcr->serial_port
>>
>> 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 | 75 
>> ++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 75 insertions(+)
>>
>> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
>> index aa87c57..741b510 100644
>> --- a/xen/drivers/char/ns16550.c
>> +++ b/xen/drivers/char/ns16550.c
>> @@ -1620,6 +1620,81 @@ DT_DEVICE_START(ns16550, "NS16550 UART", 
>> DEVICE_SERIAL)
>>  DT_DEVICE_END
>>
>>  #endif /* HAS_DEVICE_TREE */
>> +
>> +#if defined(CONFIG_ACPI) && defined(CONFIG_ARM)
>> +#include <xen/acpi.h>
>> +
>> +static int __init ns16550_acpi_uart_init(const void *data)
>> +{
>> +    struct acpi_table_header *table;
>> +    struct acpi_table_spcr *spcr;
>> +    acpi_status status;
>> +    /*
>> +     * Same as the DT part.
>> +     * 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, &table);
>> +    if ( ACPI_FAILURE(status) )
>> +    {
>> +        printk("ns16550: Failed to get SPCR table\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    spcr = container_of(table, struct acpi_table_spcr, header);
>> +
>> +    /*
>> +     * The serial port address may be 0 for example
>> +     * if the console redirection is disabled.
>> +     */
>> +    if ( unlikely(!spcr->serial_port.address) )
>> +    {
>> +        printk("ns16550: the serial port address is invalid\n");
> 
> Is zero really an invalid address, or is it rather a proper
> indicator of there not being any device?

I will change the msg to "The console redirection is disabled." following the 
description in the spcr.
Is that OK?

> 
>> +        return -EINVAL;
>> +    }
>> +
>> +    ns16550_init_common(uart);
>> +
>> +    /*
>> +     * The baud rate is pre-configured by the firmware.
> 
> But this isn't the same as BAUD_AUTO, is it? If firmware pre-configures
> the baud rate, isn't it this structure which it would use to communicate
> the information?
>

No, the comments of the BAUD_AUTO stated like that
and in fact the baud rate is not changed after the firmmware.
But I can add the baud setting if you prefer to.

>> +     * And currently the ACPI part is only targeting ARM so the following
>> +     * fields pc_interrupt, pci_device_id, pci_vendor_id, pci_bus, 
>> pci_device,
>> +     * pci_function, pci_flags, pci_segment and flow_control which we do not
>> +     * care yet are ignored.
> 
> How come flow control is of no interest?

There is no flow control in the DTS part and same for ACPI on ARM platform.

> 
> I'd also group all the pci_* fields into a simple "and all PCI related
> ones".

OK.

> 
>> +     */
>> +    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 = spcr->serial_port.bit_width;
> 
> Once again: You should not ignore the GAS address space indicator.

Sorry, I did not get the point.
Do you mean check the address is 0 or not?
Thanks!

Best Regards,
Wei

> 
> Jan
> 
> .
> 


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