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

[Wei Xu <xuwei5@hisilicon.com>]

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
> 
> .
> 


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