Re: [Xen-devel] [PATCH v2] arm/acpi: Add Server Base System Architecture UART support

[Shanker Donthineni <shankerd@xxxxxxxxxxxxxx>]

On 06/01/2016 08:49 AM, Julien Grall wrote:
> Hello Shanker,
>
> On 31/05/16 15:02, Shanker Donthineni wrote:
>> The ARM Server Base System Architecture describes a generic UART
>> interface. It doesn't support clock control registers, modem
>> control, DMA and hardware flow control features. So, extend the
>> driver probe() to handle SBSA interface and skip the accessing
>> PL011 registers that are not described in SBSA document.
>
> Please mention the version of the spec in the commit message.
>
Sure, I'll do.
>> Signed-off-by: Shanker Donthineni <shankerd@xxxxxxxxxxxxxx>
>> ---
>> Changes since v1:
>>     Don't access UART registers that are not part of SBSA document.
>>     Move setting baudrate function to a separate function.
>>
>>   xen/drivers/char/pl011.c | 56
> +++++++++++++++++++++++++++++++++++-------------
>>   1 file changed, 41 insertions(+), 15 deletions(-)
>>
>> diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c
>> index 1212d5c..b57f3b0 100644
>> --- a/xen/drivers/char/pl011.c
>> +++ b/xen/drivers/char/pl011.c
>> @@ -41,6 +41,7 @@ static struct pl011 {
>>       /* struct timer timer; */
>>       /* unsigned int timeout_ms; */
>>       /* bool_t probing, intr_works; */
>> +    bool sbsa;  /* ARM SBSA generic interface */
>>   } pl011_com = {0};
>>
>>   /* These parity settings can be ORed directly into the LCR. */
>> @@ -81,17 +82,10 @@ static void pl011_interrupt(int irq, void *data,
> struct cpu_user_regs *regs)
>>       }
>>   }
>>
>> -static void __init pl011_init_preirq(struct serial_port *port)
>> +static void __init pl011_init_baudrate(struct serial_port *port)
>>   {
>>       struct pl011 *uart = port->uart;
>>       unsigned int divisor;
>> -    unsigned int cr;
>> -
>> -    /* No interrupts, please. */
>> -    pl011_write(uart, IMSC, 0);
>> -
>> -    /* Definitely no DMA */
>> -    pl011_write(uart, DMACR, 0x0);
>>
>>       /* Line control and baud-rate generator. */
>>       if ( uart->baud != BAUD_AUTO )
>> @@ -114,6 +108,24 @@ static void __init pl011_init_preirq(struct
> serial_port *port)
>>                               | FEN
>>                               | ((uart->stop_bits - 1) << 3)
>>                               | uart->parity);
>> +}
>
> As mentioned on the previous version, the code to set/read the baudrate is 
> just wrong. The clock frequency is hardcoded rather than read from the 
> firmware.
>
> However, the baudrate is always set to BAUD_AUTO for this driver, and never 
> used after. So all this code should be dropped.
>
SPCR (non-SBSA) spec has baudrate, stop and parity  information, I think we 
should support setting the correct baudrate according to SPCR table instead of 
removing the code.

I need your opinion on this.

>> @@ -315,6 +331,7 @@ static int __init pl011_acpi_uart_init(const void
> *data)
>>   {
>>       acpi_status status;
>>       struct acpi_table_spcr *spcr = NULL;
>> +    bool sbsa;
>>       int res;
>>
>>       status = acpi_get_table(ACPI_SIG_SPCR, 0,
>> @@ -326,17 +343,21 @@ static int __init pl011_acpi_uart_init(const void
> *data)
>>           return -EINVAL;
>>       }
>>
>> +    sbsa = (spcr->interface_type == ACPI_DBG2_SBSA_32) ? true : false;
>
> sbsa = (spcr->interface_type == ACPI_DBG2_SBSA_32);
>
> However, can you explain why you kept ACPI_DBG2_SBSA_32 and not 
> ACPI_DBG2_SBSA? The former is deprecated whilst the latter is the official 
> one.
>
Qualcomm Technologies QDF2XXX ARM SBSA serial port hardware require registers 
access should be 32-bit, so our firmware sets interface type to  
ACPI_DBG2_SBSA_32.

I would like to support both the interfaces (ACPI_DBG2_SBSA_32/ACPI_DBG2_SBSA) 
If you are okay.

> Regards,
>

-- 
Shanker Donthineni
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel