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

Re: [Xen-devel] [PATCH RFC 17/35] pl011: Initialize serial from ACPI SPCR table



Hi Parth,

On 04/02/2015 14:02, parth.dixit@xxxxxxxxxx wrote:
From: Naresh Bhat <naresh.bhat@xxxxxxxxxx>

Parse ACPI SPCR (Serial Port Console Redirection table) table and
initialize the serial port pl011.

Signed-off-by: Naresh Bhat <naresh.bhat@xxxxxxxxxx>
Signed-off-by: Parth Dixit <parth.dixit@xxxxxxxxxx>
---
  xen/arch/arm/setup.c      |  6 +++++
  xen/drivers/char/pl011.c  | 69 +++++++++++++++++++++++++++++++++++++++++++++++
  xen/include/acpi/actbl2.h |  6 +++++
  xen/include/xen/serial.h  |  1 +
  4 files changed, 82 insertions(+)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index af9f429..317b985 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -762,7 +762,13 @@ void __init start_xen(unsigned long boot_phys_offset,

      init_IRQ();

+    /* If ACPI enabled and ARM64 arch then UART initialization from SPCR table 
*/
+#if defined(CONFIG_ACPI) && defined(CONFIG_ARM_64)
+    acpi_uart_init();
+#else
      dt_uart_init();
+#endif
+

Same as the previous patch, a Xen binary should both work on ACPI and DT.

      console_init_preirq();
      console_init_ring();

diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c
index dd19ce8..3499ee3 100644
--- a/xen/drivers/char/pl011.c
+++ b/xen/drivers/char/pl011.c
@@ -280,6 +280,75 @@ DT_DEVICE_START(pl011, "PL011 UART", DEVICE_SERIAL)
          .init = pl011_uart_init,
  DT_DEVICE_END

+/* Parse the SPCR table and initialize the Serial UART */
+#if defined(CONFIG_ARM_64) && defined(CONFIG_ACPI)

#ifdef CONFIG_ACPI

+
+#include <xen/acpi.h>
+

No include in the middle of the file. Please move it a the beginning.

+static int __init acpi_pl011_uart_init(struct acpi_table_spcr *spcr)
+{
+    struct pl011 *uart;
+    u64 addr, size;
+
+    uart = &pl011_com;
+    uart->clock_hz  = 0x16e3600;
+    uart->baud      = spcr->baud_rate;
+    uart->data_bits = 8;
+    uart->parity    = spcr->parity;
+    uart->stop_bits = spcr->stop_bits;
+
+    if ( spcr->interrupt < 0 )
+    {
+        printk("pl011: Unable to retrieve the IRQ\n");
+        return -EINVAL;
+    }
+
+    uart->irq = spcr->interrupt;
+    addr = spcr->serial_port.address;
+    size = 0x1000;

Please explain this size.

+    uart->regs = ioremap_nocache(addr, size);
+
+    acpi_set_irq(uart->irq, DT_IRQ_TYPE_EDGE_BOTH);

Is it always the case? I don't think so... Also, acpi_set_irq is define after this patch. The code should never use code defined later.

+
+    if ( !uart->regs )
+    {
+        printk("pl011: Unable to map the UART memory\n");
+        return -ENOMEM;
+    }
+
+    uart->vuart.base_addr = addr;
+    uart->vuart.size = size;
+    uart->vuart.data_off = DR;
+    uart->vuart.status_off = FR;
+    uart->vuart.status = 0;
+
+    /* Register with generic serial driver. */
+    serial_register_uart(SERHND_DTUART, &pl011_driver, uart);

I don't really like this. Maybe we should define a different serial handler, or redefine it to SERHND_ARM

+
+    return 0;
+}
+
+void __init acpi_uart_init(void)
+{

This function is not part of the PL011. In this case, you are breaking the design.

I believe that most of the SPCR parsing should be generic, so maybe you could extend the DEVICE interface to handle the ACPI case.

+    struct acpi_table_spcr *spcr=NULL;
+
+    printk("ACPI UART Init\n");
+    acpi_get_table(ACPI_SIG_SPCR, 0,(struct acpi_table_header **)&spcr);
+
+    switch(spcr->interface_type) {
+    case ACPI_SPCR_TYPPE_PL011:
+        acpi_pl011_uart_init(spcr);
+        break;
+        /* Not implemented yet */
+    case ACPI_SPCR_TYPE_16550:
+    case ACPI_SPCR_TYPE_16550_SUB:
+    default:
+        printk("iinvalid uart type\n");

invalid

+    }
+}
+
+#endif
+
  /*
   * Local variables:
   * mode: C
diff --git a/xen/include/acpi/actbl2.h b/xen/include/acpi/actbl2.h
index 87bc6b3..6aad200 100644
--- a/xen/include/acpi/actbl2.h
+++ b/xen/include/acpi/actbl2.h
@@ -815,6 +815,12 @@ struct acpi_table_spcr {

  #define ACPI_SPCR_DO_NOT_DISABLE    (1)

+/* UART Interface type */
+#define ACPI_SPCR_TYPE_16550 0
+#define ACPI_SPCR_TYPE_16550_SUB 1
+#define ACPI_SPCR_TYPPE_PL011 3
+
+
  
/*******************************************************************************
   *
   * SPMI - Server Platform Management Interface table
diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h
index 9f4451b..99e53d4 100644
--- a/xen/include/xen/serial.h
+++ b/xen/include/xen/serial.h
@@ -167,6 +167,7 @@ void ns16550_init(int index, struct ns16550_defaults 
*defaults);
  void ehci_dbgp_init(void);

  void __init dt_uart_init(void);
+void __init acpi_uart_init(void);

  struct physdev_dbgp_op;
  int dbgp_op(const struct physdev_dbgp_op *);

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