[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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@xxxxxxxxxxxxxx>
---
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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.