| Message ID | 1423058539-26403-18-git-send-email-parth.dixit@linaro.org |
|---|---|
| State | New |
| Headers | show |
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,
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,
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 --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 *);