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

[Julien Grall <julien.grall@arm.com>]

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))
>

[...]

> @@ -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 || ...);

Regards,

-- 
Julien Grall

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