| Message ID | 5E26738C.1060605@hisilicon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] ns16550: Add ACPI support for ARM only | expand |
On 21.01.2020 04:44, Wei Xu wrote: > --- a/xen/drivers/char/ns16550.c > +++ b/xen/drivers/char/ns16550.c > @@ -1620,6 +1620,66 @@ DT_DEVICE_START(ns16550, "NS16550 UART", DEVICE_SERIAL) > DT_DEVICE_END > > #endif /* HAS_DEVICE_TREE */ > +#if defined(CONFIG_ACPI) && defined(CONFIG_ARM) Blank line above here please. > +#include <xen/acpi.h> > + > +static int __init ns16550_acpi_uart_init(const void *data) > +{ > + struct acpi_table_spcr *spcr; > + acpi_status status; > + > + /* Same as the DT part. Comment style (again below). Also there shouldn't be a blank line until after _all_ declarations. > + * Only support one UART on ARM which happen to be ns16550_com[0]. > + */ > + struct ns16550 *uart = &ns16550_com[0]; > + > + status = acpi_get_table(ACPI_SIG_SPCR, 0, > + (struct acpi_table_header **)&spcr); Please avoid casts like this. Use more type-safe constructs like container_of() instead. > + if ( ACPI_FAILURE(status) ) > + { > + printk("ns16550: Failed to get SPCR table\n"); Is such a message warranted? I.e. wouldn't it trigger on all systems not having the table, which is hardly what you/we want? > + return -EINVAL; Also, is it really an error if there's no such table? > + } > + > + ns16550_init_common(uart); > + > + /* The baud rate is pre-configured by the firmware. > + * And currently the ACPI part is only targeting ARM so some fields > + * like PCI, flow control and so on we do not care yet are ignored. > + */ I'm no convinced though you can ignore some other fields. In particular on v1 I recall pointing out that the GAS structure has more fields you should look at. (Overall I'm not happy with "and so on" here - please list all fields you mean to ignore so that reviewers as well as future readers can judge whether this is appropriate.) > + uart->baud = BAUD_AUTO; > + uart->data_bits = 8; > + uart->parity = spcr->parity; > + uart->stop_bits = spcr->stop_bits; > + uart->io_base = spcr->serial_port.address; > + uart->io_size = 8; > + uart->reg_shift = spcr->serial_port.bit_offset; > + uart->reg_width = 1; > + > + /* The trigger/polarity information is not available in spcr. */ > + irq_set_type(spcr->interrupt, IRQ_TYPE_LEVEL_HIGH); > + uart->irq = spcr->interrupt; > + > + uart->vuart.base_addr = uart->io_base; > + uart->vuart.size = uart->io_size; > + uart->vuart.data_off = UART_THR << uart->reg_shift; > + uart->vuart.status_off = UART_LSR << uart->reg_shift; > + uart->vuart.status = UART_LSR_THRE | UART_LSR_TEMT; > + > + /* Register with generic serial driver. */ Stray double blanks at the beginning of the comment. > + serial_register_uart(uart - ns16550_com, &ns16550_driver, uart); I guess it's fine this way, but with "uart = &ns16550_com[0]" above the construct looks more complicated than it needs to look. Jan
On 21/01/2020 11:13, Jan Beulich wrote: > >> + if ( ACPI_FAILURE(status) ) >> + { >> + printk("ns16550: Failed to get SPCR table\n"); > > Is such a message warranted? I.e. wouldn't it trigger on all > systems not having the table, which is hardly what you/we want? > >> + return -EINVAL; > > Also, is it really an error if there's no such table? You can only be there if the arm-uart.c found an SPCR table. So this is a sanity check. Therefore I think the error message is warrant here. Cheers,
On 21.01.2020 12:16, Julien Grall wrote: > On 21/01/2020 11:13, Jan Beulich wrote: >> >>> + if ( ACPI_FAILURE(status) ) >>> + { >>> + printk("ns16550: Failed to get SPCR table\n"); >> >> Is such a message warranted? I.e. wouldn't it trigger on all >> systems not having the table, which is hardly what you/we want? >> >>> + return -EINVAL; >> >> Also, is it really an error if there's no such table? > > You can only be there if the arm-uart.c found an SPCR table. So this is > a sanity check. Therefore I think the error message is warrant here. If so - fine. But from ACPI_DEVICE_START(ans16550, "NS16550 UART", DEVICE_SERIAL) .class_type = ACPI_DBG2_16550_COMPATIBLE, .init = ns16550_acpi_uart_init, ACPI_DEVICE_END I can't see why this would be. Would you mind educating me? Thanks, Jan
Hi Jan, On 21/01/2020 11:25, Jan Beulich wrote: > On 21.01.2020 12:16, Julien Grall wrote: >> On 21/01/2020 11:13, Jan Beulich wrote: >>> >>>> + if ( ACPI_FAILURE(status) ) >>>> + { >>>> + printk("ns16550: Failed to get SPCR table\n"); >>> >>> Is such a message warranted? I.e. wouldn't it trigger on all >>> systems not having the table, which is hardly what you/we want? >>> >>>> + return -EINVAL; >>> >>> Also, is it really an error if there's no such table? >> >> You can only be there if the arm-uart.c found an SPCR table. So this is >> a sanity check. Therefore I think the error message is warrant here. > > If so - fine. But from > > ACPI_DEVICE_START(ans16550, "NS16550 UART", DEVICE_SERIAL) > .class_type = ACPI_DBG2_16550_COMPATIBLE, > .init = ns16550_acpi_uart_init, > ACPI_DEVICE_END > > I can't see why this would be. Would you mind educating me? The bits you pasted only register a driver for any serial device with the class 16550_COMPATIBLE. This is up to an upper layer to decide how class_type will be interpreted. As I mentioned in v1, in the case of serial, the class_type will be matches against the field interface_type in the SPCR table. This is done in the function acpi_uart_init(). The function will try to retrieve the SPCR table. If there is none, then it will bail out. Otherwise, it will call the generic device framework to lookup for the driver. Therefore a serial driver can only be called with the SPCR table actually exists. Cheers,
Hi Jan, On 2020/1/21 19:13, Jan Beulich wrote: > On 21.01.2020 04:44, Wei Xu wrote: >> --- a/xen/drivers/char/ns16550.c >> +++ b/xen/drivers/char/ns16550.c >> @@ -1620,6 +1620,66 @@ DT_DEVICE_START(ns16550, "NS16550 UART", DEVICE_SERIAL) >> DT_DEVICE_END >> >> #endif /* HAS_DEVICE_TREE */ >> +#if defined(CONFIG_ACPI) && defined(CONFIG_ARM) > > Blank line above here please. OK. I will add it. > >> +#include <xen/acpi.h> >> + >> +static int __init ns16550_acpi_uart_init(const void *data) >> +{ >> + struct acpi_table_spcr *spcr; >> + acpi_status status; >> + >> + /* Same as the DT part. > > Comment style (again below). Also there shouldn't be a blank line > until after _all_ declarations. OK. I will add a separate line leading with '*' as the comment beginning and remove the blank line in the declarations. > >> + * Only support one UART on ARM which happen to be ns16550_com[0]. >> + */ >> + struct ns16550 *uart = &ns16550_com[0]; >> + >> + status = acpi_get_table(ACPI_SIG_SPCR, 0, >> + (struct acpi_table_header **)&spcr); > > Please avoid casts like this. Use more type-safe constructs like > container_of() instead. > >> + if ( ACPI_FAILURE(status) ) >> + { >> + printk("ns16550: Failed to get SPCR table\n"); > > Is such a message warranted? I.e. wouldn't it trigger on all > systems not having the table, which is hardly what you/we want? > >> + return -EINVAL; > > Also, is it really an error if there's no such table? > >> + } >> + >> + ns16550_init_common(uart); >> + >> + /* The baud rate is pre-configured by the firmware. >> + * And currently the ACPI part is only targeting ARM so some fields >> + * like PCI, flow control and so on we do not care yet are ignored. >> + */ > > I'm no convinced though you can ignore some other fields. In > particular on v1 I recall pointing out that the GAS structure > has more fields you should look at. (Overall I'm not happy > with "and so on" here - please list all fields you mean to > ignore so that reviewers as well as future readers can judge > whether this is appropriate.) > OK. I will investigate and list all the ignore fields. >> + uart->baud = BAUD_AUTO; >> + uart->data_bits = 8; >> + uart->parity = spcr->parity; >> + uart->stop_bits = spcr->stop_bits; >> + uart->io_base = spcr->serial_port.address; >> + uart->io_size = 8; >> + uart->reg_shift = spcr->serial_port.bit_offset; >> + uart->reg_width = 1; >> + >> + /* The trigger/polarity information is not available in spcr. */ >> + irq_set_type(spcr->interrupt, IRQ_TYPE_LEVEL_HIGH); >> + uart->irq = spcr->interrupt; >> + >> + uart->vuart.base_addr = uart->io_base; >> + uart->vuart.size = uart->io_size; >> + uart->vuart.data_off = UART_THR << uart->reg_shift; >> + uart->vuart.status_off = UART_LSR << uart->reg_shift; >> + uart->vuart.status = UART_LSR_THRE | UART_LSR_TEMT; >> + >> + /* Register with generic serial driver. */ > > Stray double blanks at the beginning of the comment. > Sorry, I will remove it. >> + serial_register_uart(uart - ns16550_com, &ns16550_driver, uart); > > I guess it's fine this way, but with "uart = &ns16550_com[0]" above > the construct looks more complicated than it needs to look. Yes, I can change to use "SERHND_DTUART". Thanks for you guidance! Best Regards, Wei > > Jan > > . >
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c index aa87c57..a193f74 100644 --- a/xen/drivers/char/ns16550.c +++ b/xen/drivers/char/ns16550.c @@ -1620,6 +1620,66 @@ DT_DEVICE_START(ns16550, "NS16550 UART", DEVICE_SERIAL) DT_DEVICE_END #endif /* HAS_DEVICE_TREE */ +#if defined(CONFIG_ACPI) && defined(CONFIG_ARM) +#include <xen/acpi.h> + +static int __init ns16550_acpi_uart_init(const void *data) +{ + struct acpi_table_spcr *spcr; + acpi_status status; + + /* Same as the DT part. + * Only support one UART on ARM which happen to be ns16550_com[0]. + */ + struct ns16550 *uart = &ns16550_com[0]; + + status = acpi_get_table(ACPI_SIG_SPCR, 0, + (struct acpi_table_header **)&spcr); + + if ( ACPI_FAILURE(status) ) + { + printk("ns16550: Failed to get SPCR table\n"); + return -EINVAL; + } + + ns16550_init_common(uart); + + /* The baud rate is pre-configured by the firmware. + * And currently the ACPI part is only targeting ARM so some fields + * like PCI, flow control and so on we do not care yet are ignored. + */ + uart->baud = BAUD_AUTO; + uart->data_bits = 8; + uart->parity = spcr->parity; + uart->stop_bits = spcr->stop_bits; + uart->io_base = spcr->serial_port.address; + uart->io_size = 8; + uart->reg_shift = spcr->serial_port.bit_offset; + uart->reg_width = 1; + + /* The trigger/polarity information is not available in spcr. */ + irq_set_type(spcr->interrupt, IRQ_TYPE_LEVEL_HIGH); + uart->irq = spcr->interrupt; + + uart->vuart.base_addr = uart->io_base; + uart->vuart.size = uart->io_size; + uart->vuart.data_off = UART_THR << uart->reg_shift; + uart->vuart.status_off = UART_LSR << uart->reg_shift; + uart->vuart.status = UART_LSR_THRE | UART_LSR_TEMT; + + /* Register with generic serial driver. */ + serial_register_uart(uart - ns16550_com, &ns16550_driver, uart); + + return 0; +} + +ACPI_DEVICE_START(ans16550, "NS16550 UART", DEVICE_SERIAL) + .class_type = ACPI_DBG2_16550_COMPATIBLE, + .init = ns16550_acpi_uart_init, +ACPI_DEVICE_END + +#endif /* CONFIG_ACPI && CONFIG_ARM */ + /* * Local variables: * mode: C
Parse the ACPI SPCR table and initialize the 16550 compatible serial port for ARM only. Currently we only support one UART on ARM. Some fields like PCI, flow control and so on we do not care yet on ARM are ignored. Signed-off-by: Wei Xu<xuwei5@hisilicon.com> --- Changes in v2: - improve commit message - remove the spcr initialization - add comments for the uart initialization and configuration - adjust the code style issue - limit the code only built on ACPI and ARM --- xen/drivers/char/ns16550.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) -- 2.8.1 .