From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH RFC 17/35] pl011: Initialize serial from ACPI SPCR table Date: Wed, 04 Feb 2015 21:57:29 +0000 Message-ID: <54D295C9.9040205@linaro.org> References: <1423058539-26403-1-git-send-email-parth.dixit@linaro.org> <1423058539-26403-18-git-send-email-parth.dixit@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1423058539-26403-18-git-send-email-parth.dixit@linaro.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: parth.dixit@linaro.org, xen-devel@lists.xen.org Cc: ian.campbell@citrix.com, Naresh Bhat , tim@xen.org, stefano.stabellini@citrix.com, jbeulich@suse.com, christoffer.dall@linaro.org List-Id: xen-devel@lists.xenproject.org Hi Parth, On 04/02/2015 14:02, parth.dixit@linaro.org wrote: > From: Naresh Bhat > > Parse ACPI SPCR (Serial Port Console Redirection table) table and > initialize the serial port pl011. > > Signed-off-by: Naresh Bhat > Signed-off-by: Parth Dixit > --- > 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 > + 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