diff mbox

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

Message ID 1423058539-26403-18-git-send-email-parth.dixit@linaro.org
State New
Headers show

Commit Message

Parth Dixit Feb. 4, 2015, 2:02 p.m. UTC
From: Naresh Bhat <naresh.bhat@linaro.org>

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

Signed-off-by: Naresh Bhat <naresh.bhat@linaro.org>
Signed-off-by: Parth Dixit <parth.dixit@linaro.org>
---
 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(+)

Comments

Julien Grall Feb. 4, 2015, 9:57 p.m. UTC | #1
Hi Parth,

On 04/02/2015 14:02, parth.dixit@linaro.org wrote:
> From: Naresh Bhat <naresh.bhat@linaro.org>
>
> Parse ACPI SPCR (Serial Port Console Redirection table) table and
> initialize the serial port pl011.
>
> Signed-off-by: Naresh Bhat <naresh.bhat@linaro.org>
> Signed-off-by: Parth Dixit <parth.dixit@linaro.org>
> ---
>   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 Feb. 5, 2015, 2:29 p.m. UTC | #2
Hi Ian,

On 05/02/2015 19:42, Ian Campbell wrote:
> On Wed, 2015-02-04 at 21:57 +0000, Julien Grall wrote:
>
>> I believe that most of the SPCR parsing should be generic, so maybe you
>> could extend the DEVICE interface to handle the ACPI case.
>
> Extending DT_DEVICE would be confusing IMHO. The answer is probably a
> similar but parallel ACPI_DEVICE mechanism, perhaps sharing some
> underlying infrastructure with DT_DEVICE.

I was thinking to rename DT_DEVICE into something more meaningful. 
Because infine, DT_DEVICE_START/DT_DEVICE_END doesn't do anything DT 
specific but defined the a printed name and the device class.

So we can extend the device framework without adding too much new code.

Regards,
Julien Grall Feb. 11, 2015, 6:10 a.m. UTC | #3
Hi Ian,

On 05/02/2015 22:52, Ian Campbell wrote:
> On Thu, 2015-02-05 at 22:29 +0800, Julien Grall wrote:
>> On 05/02/2015 19:42, Ian Campbell wrote:
>>> On Wed, 2015-02-04 at 21:57 +0000, Julien Grall wrote:
>>>
>>>> I believe that most of the SPCR parsing should be generic, so maybe you
>>>> could extend the DEVICE interface to handle the ACPI case.
>>>
>>> Extending DT_DEVICE would be confusing IMHO. The answer is probably a
>>> similar but parallel ACPI_DEVICE mechanism, perhaps sharing some
>>> underlying infrastructure with DT_DEVICE.
>>
>> I was thinking to rename DT_DEVICE into something more meaningful.
>> Because infine, DT_DEVICE_START/DT_DEVICE_END doesn't do anything DT
>> specific but defined the a printed name and the device class.
>>
>> So we can extend the device framework without adding too much new code.
>
> Remember that things like the probe function are going to have different
> prototypes. It also contains things like the DT compat list which is DT
> specific.
>
> So, I think the macros should remain boot firmware specific, whether
> they fill in the same array or not is an implementation detail, but it
> would seem less error prone to split them up.

We may duplicate some fields with this. Although we are talking about 
only few bytes.

So I guess it's fine.

Regards,
diff mbox

Patch

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
+
     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)
+
+#include <xen/acpi.h>
+
+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;
+    uart->regs = ioremap_nocache(addr, size);
+
+    acpi_set_irq(uart->irq, DT_IRQ_TYPE_EDGE_BOTH);
+
+    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);
+
+    return 0;
+}
+
+void __init acpi_uart_init(void)
+{
+    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");
+    }
+}
+
+#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 *);