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

[Wei Xu <xuwei5@xxxxxxxxxxxxx>]

Hi Jan,

On 2020/2/25 20:25, Jan Beulich wrote:
> On 24.02.2020 02:06, Wei Xu wrote:
>> On 2020/2/21 23:07, Jan Beulich wrote:
>>> On 21.02.2020 15:57, Julien Grall wrote:
>>>> On 21/02/2020 14:02, Jan Beulich wrote:
>>>>> On 21.02.2020 03:22, Wei Xu wrote:
>>>>>> --- a/xen/drivers/char/ns16550.c
>>>>>> +++ b/xen/drivers/char/ns16550.c
>>>>>> @@ -1620,6 +1620,85 @@ 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: Console redirection is disabled\n");
>>>>>> +        return -EINVAL;
>>>>>> +    }
>>>>>> +
>>>>>> +    if ( unlikely(spcr->serial_port.space_id != 
>>>>>> ACPI_ADR_SPACE_SYSTEM_MEMORY) )
>>>>>> +    {
>>>>>> +        printk("ns16550: Address space type is not mmio\n");
>>>>>> +        return -EINVAL;
>>>>>> +    }
>>>>>
>>>>> The space_id field qualifies the address one, i.e. whether a value of
>>>>> zero can sensibly mean "disabled" depends on the address space. Hence
>>>>> logically the address space check should come first.
>>>>>
>>>>> This is the last thing I'd like to see changed. I won't give the
>>>>> patch my ack though, as I think it should be an Arm maintainer to ack
>>>>> it.
>>>>
>>>> Acked-by: Julien Grall <julien@xxxxxxx>
>>>>
>>>> Although, a reviewed-by tag from you would be nice as you did most of 
>>>> the review for this patch.
>>>
>>> Well, to clarify - this is one of the very few (afaict) cases where our
>>> R-b implying A-b (when people are entitled to ack the respective code)
>>> gets in the way. If this wasn't the case, I'd have given the former,
>>> making it clear (also later from just looking at the resulting commit)
>>> that the (only) ack came from an Arm person.
>>
>> I will check the address space in the v5 and add your R-b.
> 
> I guess some misunderstanding has occurred? You're already checking the
> address space in v4 (and I did already commit it this way on Friday).
> And as indicated I intentionally elected to not provide my R-b for this
> patch, for the reason given above.

Sorry, I misunderstood.
Thanks for clarifying!

Best Regards,
Wei

> 
> Jan
> 
> .
> 


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