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

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

Hi Shanker,

On 03/06/16 18:39, 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>
> ---
> Changes since v2:
>     Edited commit text to include SBSA document version.
>     Remove setting baudrate code completely as per Julien's suggestion.

This item and ...

>     Support both the SBSA interface types ACPI_DBG2_SBSA & ACPI_DBG2_SBSA_32.
>     Replace MIS references with combination of RIS & IMSC.

this one need some description in the commit message to explain why it 
is fine for non-SBSA UART to do it.

The later also needs some explanation why MIS could be replaced by RIS & 
IMSC.

Lastly, IHMO those 2 items should be in separate patch to make the 
review easier.

>
> 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 | 76 ++++++++++++++++++++++++++----------------------
>   1 file changed, 41 insertions(+), 35 deletions(-)
>
> diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c
> index 1212d5c..3497d61 100644
> --- a/xen/drivers/char/pl011.c
> +++ b/xen/drivers/char/pl011.c
> @@ -31,7 +31,7 @@
>   #include <asm/io.h>
>
>   static struct pl011 {
> -    unsigned int baud, clock_hz, data_bits, parity, stop_bits;
> +    unsigned int data_bits, parity, stop_bits;
>       unsigned int irq;
>       void __iomem *regs;
>       /* UART with IRQ line: interrupt-driven I/O. */
> @@ -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. */
> @@ -57,7 +58,9 @@ static void pl011_interrupt(int irq, void *data, struct cpu_user_regs *regs)
>   {
>       struct serial_port *port = data;
>       struct pl011 *uart = port->uart;
> -    unsigned int status = pl011_read(uart, MIS);
> +    unsigned int status = pl011_read(uart, RIS);
> +
> +    status &= pl011_read(uart, IMSC);
>
>       if ( status )
>       {
> @@ -76,7 +79,7 @@ static void pl011_interrupt(int irq, void *data, struct cpu_user_regs *regs)
>               if ( status & (TXI) )
>                   serial_tx_interrupt(port, regs);
>
> -            status = pl011_read(uart, MIS);
> +            status = pl011_read(uart, RIS) & pl011_read(uart, IMSC);

Please introduce a helper to read the status.

>           } while (status != 0);
>       }
>   }

Regards,

-- 
Julien Grall

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