| Message ID | 1579232458-26803-1-git-send-email-xuwei5@hisilicon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ns16550: Add ACPI support | expand |
On 17.01.2020 04:40, Wei Xu wrote: > --- a/xen/drivers/char/ns16550.c > +++ b/xen/drivers/char/ns16550.c > @@ -1620,6 +1620,61 @@ DT_DEVICE_START(ns16550, "NS16550 UART", DEVICE_SERIAL) > DT_DEVICE_END > > #endif /* HAS_DEVICE_TREE */ > + > +#ifdef CONFIG_ACPI > +#include <xen/acpi.h> > + > +static int __init ns16550_acpi_uart_init(const void *data) > +{ > + struct acpi_table_spcr *spcr = NULL; The initializer isn't strictly needed, is it? > + acpi_status status; > + struct ns16550 *uart; > + > + 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; > + } > + > + uart = &ns16550_com[0]; You want to justify the choice of what (on x86 at least= we'd call com1 in the patch description. Also this could be the initializer of the variable. > + ns16550_init_common(uart); > + > + uart->baud = BAUD_AUTO; There's a baud_rate field in the structure. If there's a reason to ignore it, please add a comment. There's also an interface_type field - can you really ignore it? > + uart->data_bits = 8; > + uart->parity = spcr->parity; > + uart->stop_bits = spcr->stop_bits; There's also a flow_control field, which I think needs checking that it matches ns16550_setup_preirq() comment: /* No flow ctrl: DTR and RTS are both wedged high to keep remote happy. */ Similarly any other fields you don't evaluate at all and which aren't explained by the spec as possible to be ignored (and the situation matching the use case, like you not caring about PCI aspects here) need reasoning about in the description or a code comment. > + uart->io_base = spcr->serial_port.address; The field (or perhaps the whole spcr->serial_port) being zero looks to have special meaning. > + uart->io_size = 8; > + uart->reg_shift = spcr->serial_port.bit_offset; spcr->serial_port has other fields which I don't think you should ignore. > + uart->reg_width = 1; Please use consistent placement of = : Either all of them are aligned, or all of them are preceded by a single space only. > + /* 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; Style-wise this block should then match whatever the other block above looks. > + /* 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 I don't expect this to build on x86. Finally, please follow patch submission guidelines: Patches ought to be sent _to_ the list and maintainers be _cc_-ed. Jan
Hi Jan, On 17/01/2020 08:33, Jan Beulich wrote: > On 17.01.2020 04:40, Wei Xu wrote: >> --- a/xen/drivers/char/ns16550.c >> +++ b/xen/drivers/char/ns16550.c >> @@ -1620,6 +1620,61 @@ DT_DEVICE_START(ns16550, "NS16550 UART", DEVICE_SERIAL) >> DT_DEVICE_END >> >> #endif /* HAS_DEVICE_TREE */ >> + >> +#ifdef CONFIG_ACPI >> +#include <xen/acpi.h> >> + >> +static int __init ns16550_acpi_uart_init(const void *data) >> +{ >> + struct acpi_table_spcr *spcr = NULL; > > The initializer isn't strictly needed, is it? > >> + acpi_status status; >> + struct ns16550 *uart; >> + >> + 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; >> + } >> + >> + uart = &ns16550_com[0]; > > You want to justify the choice of what (on x86 at least= we'd call > com1 in the patch description. Also this could be the initializer > of the variable. This is the same choice as we made for the DT binding (see ns16550_uart()). We only support one UART on Arm which happen to be ns16550_com[0] (but named diferrently). The code below is actually quite similar to the DT parsing, so maybe we want to provide a common helper here. > >> + ns16550_init_common(uart); >> + >> + uart->baud = BAUD_AUTO; > > There's a baud_rate field in the structure. If there's a reason > to ignore it, please add a comment. Same as for the DT part, we assume the firmware will configure the UART correctly. > > There's also an interface_type field - can you really ignore it? It is not ignored. This is used by the upper layer to detect which uart driver to call (see acpi_uart_init() in arm-uart.c). > >> + uart->data_bits = 8; >> + uart->parity = spcr->parity; >> + uart->stop_bits = spcr->stop_bits; > > There's also a flow_control field, which I think needs checking > that it matches ns16550_setup_preirq() comment: > > /* No flow ctrl: DTR and RTS are both wedged high to keep remote happy. */ > > Similarly any other fields you don't evaluate at all and which > aren't explained by the spec as possible to be ignored (and the > situation matching the use case, like you not caring about PCI > aspects here) need reasoning about in the description or a code > comment. What's missing in the commit message is the fact this is only targeting Arm. So there are a lot we don't care yet (such as PCI). > >> + uart->io_base = spcr->serial_port.address; > > The field (or perhaps the whole spcr->serial_port) being zero looks > to have special meaning. > >> + uart->io_size = 8; >> + uart->reg_shift = spcr->serial_port.bit_offset; > > spcr->serial_port has other fields which I don't think you should > ignore. > >> + uart->reg_width = 1; > > Please use consistent placement of = : Either all of them are > aligned, or all of them are preceded by a single space only. > >> + /* 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; > > Style-wise this block should then match whatever the other > block above looks. > >> + /* 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 > > I don't expect this to build on x86. This is only meant to target Arm. So maybe we want to protect the whole code with defined(CONFIG_ACPI) && defined(CONFIG_ARM). Cheers,
On 18.01.2020 13:32, Julien Grall wrote: > On 17/01/2020 08:33, Jan Beulich wrote: >> On 17.01.2020 04:40, Wei Xu wrote: >>> --- a/xen/drivers/char/ns16550.c >>> +++ b/xen/drivers/char/ns16550.c >>> @@ -1620,6 +1620,61 @@ DT_DEVICE_START(ns16550, "NS16550 UART", DEVICE_SERIAL) >>> DT_DEVICE_END >>> >>> #endif /* HAS_DEVICE_TREE */ >>> + >>> +#ifdef CONFIG_ACPI >>> +#include <xen/acpi.h> >>> + >>> +static int __init ns16550_acpi_uart_init(const void *data) >>> +{ >>> + struct acpi_table_spcr *spcr = NULL; >>> + acpi_status status; >>> + struct ns16550 *uart; >>> + >>> + 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; >>> + } >>> + >>> + uart = &ns16550_com[0]; >> >> You want to justify the choice of what (on x86 at least= we'd call >> com1 in the patch description. Also this could be the initializer >> of the variable. > > This is the same choice as we made for the DT binding (see > ns16550_uart()). We only support one UART on Arm which happen to be > ns16550_com[0] (but named diferrently). > > The code below is actually quite similar to the DT parsing, so maybe we > want to provide a common helper here. That's all fine, but doesn't eliminate the need to say so in the description. >>> + /* 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 >> >> I don't expect this to build on x86. > > This is only meant to target Arm. So maybe we want to protect the whole > code with defined(CONFIG_ACPI) && defined(CONFIG_ARM). Indeed, that's what the remark was aiming at. Jan
Hi Jan, Julien, On 2020/1/20 16:38, Jan Beulich wrote: > On 18.01.2020 13:32, Julien Grall wrote: >> On 17/01/2020 08:33, Jan Beulich wrote: >>> On 17.01.2020 04:40, Wei Xu wrote: >>>> --- a/xen/drivers/char/ns16550.c >>>> +++ b/xen/drivers/char/ns16550.c >>>> @@ -1620,6 +1620,61 @@ DT_DEVICE_START(ns16550, "NS16550 UART", DEVICE_SERIAL) >>>> DT_DEVICE_END >>>> >>>> #endif /* HAS_DEVICE_TREE */ >>>> + >>>> +#ifdef CONFIG_ACPI >>>> +#include <xen/acpi.h> >>>> + >>>> +static int __init ns16550_acpi_uart_init(const void *data) >>>> +{ >>>> + struct acpi_table_spcr *spcr = NULL; >>>> + acpi_status status; >>>> + struct ns16550 *uart; >>>> + >>>> + 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; >>>> + } >>>> + >>>> + uart = &ns16550_com[0]; >>> You want to justify the choice of what (on x86 at least= we'd call >>> com1 in the patch description. Also this could be the initializer >>> of the variable. >> This is the same choice as we made for the DT binding (see >> ns16550_uart()). We only support one UART on Arm which happen to be >> ns16550_com[0] (but named diferrently). >> >> The code below is actually quite similar to the DT parsing, so maybe we >> want to provide a common helper here. > That's all fine, but doesn't eliminate the need to say so in the > description. > >>>> + /* 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 >>> I don't expect this to build on x86. >> This is only meant to target Arm. So maybe we want to protect the whole >> code with defined(CONFIG_ACPI) && defined(CONFIG_ARM). > Indeed, that's what the remark was aiming at. > > Jan > > . > Thanks! I will address the comments in V2. Best Regards, Wei
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c index aa87c57..eb32891 100644 --- a/xen/drivers/char/ns16550.c +++ b/xen/drivers/char/ns16550.c @@ -1620,6 +1620,61 @@ DT_DEVICE_START(ns16550, "NS16550 UART", DEVICE_SERIAL) DT_DEVICE_END #endif /* HAS_DEVICE_TREE */ + +#ifdef CONFIG_ACPI +#include <xen/acpi.h> + +static int __init ns16550_acpi_uart_init(const void *data) +{ + struct acpi_table_spcr *spcr = NULL; + acpi_status status; + struct ns16550 *uart; + + 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; + } + + uart = &ns16550_com[0]; + + ns16550_init_common(uart); + + 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; + + /* 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 */ + /* * Local variables: * mode: C
Parse the ACPI SPCR table and initialize the 16550 compatible serial port. Signed-off-by: Wei Xu <xuwei5@hisilicon.com> --- xen/drivers/char/ns16550.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+)