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

[Julien Grall <julien.grall@linaro.org>]

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