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

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

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.

> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
> ---
> 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.

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

Regards,

-- 
Julien Grall

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