Re: [Xen-devel] [PATCH] ns16550: Add ACPI support

[Julien Grall <julien@xxxxxxx>]

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,

--
Julien Grall

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