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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

> +        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?

> +     * 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?

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

> +     */
> +    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.

Jan

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