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

[Wei Xu <xuwei5@hisilicon.com>]

Hi Jan,

On 2020/2/20 16:33, Jan Beulich wrote:
> 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.
> 

Got it.

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

Got it.

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

I did not know what the space_id means yet.
I will investigate it.
Thanks a lot!

Best Regards,
Wei

> 
> Jan
> 
> .
> 


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