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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.02.2020 08:44, Wei Xu wrote:
> On 2020/2/17 21:53, Jan Beulich wrote:
>> On 03.02.2020 12:21, Wei Xu wrote:
>>> +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?

With the "The" preferably dropped, yes.

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

Oh, I see. I should have checked the comment instead of implying
meaning assigned to similarly named things elsewhere. Keep as is.

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

No. I mean you shouldn't ignore other parts of the structure:

struct acpi_generic_address {
        u8 space_id;            /* Address space where struct or register 
exists */
        u8 bit_width;           /* Size in bits of given register */
        u8 bit_offset;          /* Bit offset within the register */
        u8 access_width;        /* Minimum Access size (ACPI 3.0) */
        u64 address;            /* 64-bit address of struct or register */
};

Iirc you now consume all fields except space_id, yet space_id
is a qualifier to the address field (which you obviously use).

Jan

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