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

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

Hello Shanker,

On 27/05/16 01:28, Shanker Donthineni wrote:
> The ARM Server Base System Architecture describes a generic UART
> interface. It doesn't support clock control registers to set
> baudrate. So, extend the driver probe() to handle SBSA interface
> types and set the baudrate to 115200 for SBSA interfaces.

I cannot find any mention of the baudrate in the SBSA. Where does it 
come from?

Also the driver is using registers which should not be touch for the 
SBSA UART (see appendix B in the SBSA ARM-DEN-0029 v3.0). So this need 
to be address to get a proper support.

> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
> ---
> https://silver.arm.com/download/download.tm?pv=2950177
>   xen/drivers/char/pl011.c | 25 +++++++++++++++++++++----
>   1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c
> index 1212d5c..81d095f 100644
> --- a/xen/drivers/char/pl011.c
> +++ b/xen/drivers/char/pl011.c
> @@ -226,14 +226,14 @@ static struct uart_driver __read_mostly pl011_driver = {
>       .vuart_info   = pl011_vuart,
>   };
>
> -static int __init pl011_uart_init(int irq, u64 addr, u64 size)
> +static int __init pl011_uart_init(int irq, u64 addr, u64 size, int baud)
>   {
>       struct pl011 *uart;
>
>       uart = &pl011_com;
>       uart->irq       = irq;
>       uart->clock_hz  = 0x16e3600;
> -    uart->baud      = BAUD_AUTO;
> +    uart->baud      = baud;
>       uart->data_bits = 8;
>       uart->parity    = PARITY_NONE;
>       uart->stop_bits = 1;
> @@ -285,7 +285,7 @@ static int __init pl011_dt_uart_init(struct dt_device_node *dev,
>           return -EINVAL;
>       }
>
> -    res = pl011_uart_init(res, addr, size);
> +    res = pl011_uart_init(res, addr, size, BAUD_AUTO);
>       if ( res < 0 )
>       {
>           printk("pl011: Unable to initialize\n");
> @@ -315,6 +315,7 @@ static int __init pl011_acpi_uart_init(const void *data)
>   {
>       acpi_status status;
>       struct acpi_table_spcr *spcr = NULL;
> +    int baud = BAUD_AUTO;
>       int res;
>
>       status = acpi_get_table(ACPI_SIG_SPCR, 0,
> @@ -326,17 +327,23 @@ static int __init pl011_acpi_uart_init(const void *data)
>           return -EINVAL;
>       }
>
> +    if ( (spcr->interface_type == ACPI_DBG2_SBSA_32) ||

Based on the DBG2 spec, this value is deprecated. So I do not think we 
should support it.

> +         (spcr->interface_type == ACPI_DBG2_SBSA) )
> +        baud = 115200;
> +
>       /* trigger/polarity information is not available in spcr */
>       irq_set_type(spcr->interrupt, IRQ_TYPE_LEVEL_HIGH);
>
>       res = pl011_uart_init(spcr->interrupt, spcr->serial_port.address,
> -                          PAGE_SIZE);
> +                          PAGE_SIZE, baud);
>       if ( res < 0 )
>       {
>           printk("pl011: Unable to initialize\n");
>           return res;
>       }
>
> +
> +

Spurious changes.

>       return 0;
>   }
>
> @@ -344,6 +351,16 @@ ACPI_DEVICE_START(apl011, "PL011 UART", DEVICE_SERIAL)
>           .class_type = ACPI_DBG2_PL011,
>           .init = pl011_acpi_uart_init,
>   ACPI_DEVICE_END
> +
> +ACPI_DEVICE_START(asbsa_uart, "SBSA UART", DEVICE_SERIAL)
> +        .class_type = ACPI_DBG2_SBSA,
> +        .init = pl011_acpi_uart_init,
> +ACPI_DEVICE_END
> +
> +ACPI_DEVICE_START(asbsa32_uart, "SBSA32 UART", DEVICE_SERIAL)
> +        .class_type = ACPI_DBG2_SBSA_32,
> +        .init = pl011_acpi_uart_init,
> +ACPI_DEVICE_END
>   #endif
>
>   /*
>

Regards,

-- 
Julien Grall

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