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

[Shanker Donthineni <shankerd@xxxxxxxxxxxxxx>]

Hi Julien,

On 06/06/2016 11:04 AM, Julien Grall wrote:
> Hi Shanker,
>
> On 03/06/16 18:39, 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
>> (ARM-DEN-0029 Version 3.0, 6 APPENDIX B: GENERIC UART).
>>
>> Signed-off-by: Shanker Donthineni <shankerd@xxxxxxxxxxxxxx>
>> ---
>> Changes since v2:
>>     Edited commit text to include SBSA document version.
>>     Remove setting baudrate code completely as per Julien's suggestion.
>
> This item and ...
>
>>     Support both the SBSA interface types ACPI_DBG2_SBSA &
> ACPI_DBG2_SBSA_32.
>>     Replace MIS references with combination of RIS & IMSC.
>
> this one need some description in the commit message to explain why it is 
> fine for non-SBSA UART to do it.
>
I have not tested this patch on non-SBSA UART platform, let me apply this 
change only for SBSA UART driver
for safe side.

> The later also needs some explanation why MIS could be replaced by RIS & IMSC.
>
> Lastly, IHMO those 2 items should be in separate patch to make the review 
> easier.
>
>>
>> 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 | 76
> ++++++++++++++++++++++++++----------------------
>>   1 file changed, 41 insertions(+), 35 deletions(-)
>>
>> diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c
>> index 1212d5c..3497d61 100644
>> --- a/xen/drivers/char/pl011.c
>> +++ b/xen/drivers/char/pl011.c
>> @@ -31,7 +31,7 @@
>>   #include <asm/io.h>
>>
>>   static struct pl011 {
>> -    unsigned int baud, clock_hz, data_bits, parity, stop_bits;
>> +    unsigned int data_bits, parity, stop_bits;
>>       unsigned int irq;
>>       void __iomem *regs;
>>       /* UART with IRQ line: interrupt-driven I/O. */
>> @@ -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. */
>> @@ -57,7 +58,9 @@ static void pl011_interrupt(int irq, void *data,
> struct cpu_user_regs *regs)
>>   {
>>       struct serial_port *port = data;
>>       struct pl011 *uart = port->uart;
>> -    unsigned int status = pl011_read(uart, MIS);
>> +    unsigned int status = pl011_read(uart, RIS);
>> +
>> +    status &= pl011_read(uart, IMSC);
>>
>>       if ( status )
>>       {
>> @@ -76,7 +79,7 @@ static void pl011_interrupt(int irq, void *data,
> struct cpu_user_regs *regs)
>>               if ( status & (TXI) )
>>                   serial_tx_interrupt(port, regs);
>>
>> -            status = pl011_read(uart, MIS);
>> +            status = pl011_read(uart, RIS) & pl011_read(uart, IMSC);
>
> Please introduce a helper to read the status.
>
>>           } while (status != 0);
>>       }
>>   }
>
> 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