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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Wed, 4 Feb 2015, 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

This is bad.  We should have a single uart_init function, that calls
acpi_uart_init #ifdef(CONFIG_ACPI) and/or simply if(!acpi_disabled).


>      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);

It is not appropriate to use DT_IRQ_TYPEs in an acpi init function.
I think you should introduce a new ACPI irq type or at least have an
explicit conversion function.


> +    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");
> +    }
> +}

Given that acpi_uart_init is supposed to be a generic uart initalization
function, is not right to implement it in pl011.c, the source file for
the pl011 driver.

I think that you should rename dt-uart.c into something more generic,
maybe arm_uart, then move acpi_uart_init there.


> +#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
                       ^ typo

> +
>  
> /*******************************************************************************
>   *
>   * 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 *);
> -- 
> 1.9.1
> 

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