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

[Wei Chen <wei.chen@xxxxxxxxxx>]

Hi Shanker,

On 31 May 2016 at 22:02, Shanker Donthineni <shankerd@xxxxxxxxxxxxxx> 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.
>
> Signed-off-by: Shanker Donthineni <shankerd@xxxxxxxxxxxxxx>
> ---
> 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);
> +}

Should we have to move these codes into a separate function?
And stop bit, parity are not baudrate setting code.

> +
> +static void __init pl011_init_preirq(struct serial_port *port)
> +{
> +    struct pl011 *uart = port->uart;
> +    unsigned int cr;
> +
> +    /* No interrupts, please. */
> +    pl011_write(uart, IMSC, 0);
> +
> +    if ( !uart->sbsa )
> +    {
> +        /* Definitely no DMA */
> +        pl011_write(uart, DMACR, 0x0);
> +
> +        /* Configure baudrate */
> +      pl011_init_baudrate(port);
Tab should be replaced by spaces.

> +    }
>      /* Clear errors */
>      pl011_write(uart, RSR, 0);
>
> @@ -121,10 +133,13 @@ static void __init pl011_init_preirq(struct serial_port 
> *port)
>      pl011_write(uart, IMSC, 0);
>      pl011_write(uart, ICR, ALLI);
>
> -    /* Enable the UART for RX and TX; keep RTS and DTR */
> -    cr = pl011_read(uart, CR);
> -    cr &= RTS | DTR;
> -    pl011_write(uart, CR, cr | RXE | TXE | UARTEN);
> +    if ( !uart->sbsa )
> +    {
> +        /* Enable the UART for RX and TX; keep RTS and DTR */
> +        cr = pl011_read(uart, CR);
> +        cr &= RTS | DTR;
> +        pl011_write(uart, CR, cr | RXE | TXE | UARTEN);
> +    }
>  }
>
>  static void __init pl011_init_postirq(struct serial_port *port)
> @@ -226,7 +241,7 @@ 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, bool sbsa)
I would add a type parameter instead of sbsa, and pass
spcr->interface_type to it.
>  {
>      struct pl011 *uart;
>
> @@ -237,6 +252,7 @@ static int __init pl011_uart_init(int irq, u64 addr, u64 
> size)
>      uart->data_bits = 8;
>      uart->parity    = PARITY_NONE;
>      uart->stop_bits = 1;
> +    uart->sbsa = sbsa;
>
>      uart->regs = ioremap_nocache(addr, size);
>      if ( !uart->regs )
> @@ -285,7 +301,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, false);
>      if ( res < 0 )
>      {
>          printk("pl011: Unable to initialize\n");
> @@ -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;
> +

I would move this code to pl011_uart_init.

>      /* 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, sbsa);
>      if ( res < 0 )
>      {
>          printk("pl011: Unable to initialize\n");
>          return res;
>      }
>
> +
> +
These two new lines are spurious changes.
>      return 0;
>  }
>
> @@ -344,6 +365,11 @@ ACPI_DEVICE_START(apl011, "PL011 UART", DEVICE_SERIAL)
>          .class_type = ACPI_DBG2_PL011,
>          .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
>
>  /*
> --
> 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

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