Re: [PATCH V8 3/3] arm/acpi: Add Server Base System Architecture UART support

[Shanker Donthineni <shankerd@codeaurora.org>]

On 06/09/2016 05:19 AM, Julien Grall wrote:
> Hello Shanker,
>
> On 08/06/16 14:28, 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@codeaurora.org>
>> Reviewed-by: Julien Grall <julien.grall@arm.com>
>> ---
>> Changes sicne v7:
>>     Moved comment 'To compatible with SBSA v2.x document, all 
>> accesses should be 32-bit' from #2
>>
>> Changes since v3:
>>    Moved non-SBSA related changes to patches 1/3 and 2/3.
>>
>> changes since v2:
>>    Edited commit text to include SBSA document version.
>>    Remove setting baudrate code completely as per Julien's suggestion.
>>    Support both the SBSA interface types ACPI_DBG2_SBSA & 
>> ACPI_DBG2_SBSA_32.
>>    Replace MIS references with combination of RIS & IMSC.
>>
>> 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 | 55 
>> +++++++++++++++++++++++++++++++++++-------------
>>   1 file changed, 40 insertions(+), 15 deletions(-)
>>
>> diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c
>> index a2f929b..d70ec99 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. */
>> @@ -50,6 +51,7 @@ static struct pl011 {
>>   #define PARITY_MARK  (PEN|SPS)
>>   #define PARITY_SPACE (PEN|EPS|SPS)
>>
>> +/* To compatible with SBSA v2.x document, all accesses should be 
>> 32-bit */
>
> The verb is missing. Also please add a full stop at the end of the 
> comment.
>
>>   #define pl011_read(uart, off) readl((uart)->regs + (off))
>>   #define pl011_write(uart, off,val)      writel((val), (uart)->regs 
>> + (off))
>>
>
> [...]
>
Sorry, I didn't understand what is [...]?
>> @@ -313,11 +323,15 @@ static int __init pl011_acpi_uart_init(const 
>> void *data)
>>           return -EINVAL;
>>       }
>>
>> +    if ( (spcr->interface_type == ACPI_DBG2_SBSA) ||
>> +         (spcr->interface_type == ACPI_DBG2_SBSA_32) )
>> +        sbsa = true;
>
> I thought I already mentioned this on a previous version:
>
> sbsa = (spcr->interface_type == ACPI_DBG2_SBSA || ...);
>
You want me change to

sbsa = (spcr->interface_type == ACPI_DBG2_SBSA || spcr->interface_type 
== ACPI_DBG2_SBSA_32)

right?
> 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@lists.xen.org
http://lists.xen.org/xen-devel