| Message ID | d929b43814e6e1a247b954c4be40a35d61b6a45a.1690208840.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] ns16550: add support for polling mode when device tree is used | expand |
On 24.07.2023 16:27, Oleksii Kurochko wrote: > --- a/xen/drivers/char/console.c > +++ b/xen/drivers/char/console.c > @@ -993,6 +993,8 @@ void __init console_init_preirq(void) > #endif > else if ( !strncmp(p, "none", 4) ) > continue; > + else if ( !strncmp(p, "polling", 7 ) > + continue; > else if ( (sh = serial_parse_handle(p)) >= 0 ) > { > sercon_handle = sh; Looks like you mean the new option to go under "console=". Besides this being guesswork because of you not updating the command line doc, this also is wrong, as the property then applies to all consoles. What you mean is a control for a specific instance of a 16550 console, which can only possibly go under "com<N>=". I would suggest to simply extend [<irq>|msi] there to [<irq>|msi|poll]. > @@ -595,7 +601,9 @@ static void __init cf_check ns16550_endboot(struct serial_port *port) > static int __init cf_check ns16550_irq(struct serial_port *port) > { > struct ns16550 *uart = port->uart; > - return ((uart->irq > 0) ? uart->irq : -1); > + > + return ( uart->intr_works != polling ) ? > + uart->irq : -1; > } Please don't corrupt previously correct style. You can keep things almost like they were (albeit there's no strict need for any of the parentheses): return ((uart->intr_works != polling) ? uart->irq : -1); > @@ -1330,9 +1341,12 @@ pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx) > * as special only for X86. > */ > if ( uart->irq == 0xff ) > + { > uart->irq = 0; > + uart->intr_works = polling; > + } > #endif > - if ( !uart->irq ) > + if ( !uart->irq || (uart->intr_works == polling) ) > printk(XENLOG_INFO > "ns16550: %pp: no legacy IRQ, using poll mode\n", > &PCI_SBDF(0, b, d, f)); Message and code (after your addition) continue to be out of sync. As said before, any condition that leads to polling mode wants to find itself expressed by uart->intr_works set to "polling". > @@ -1552,6 +1566,7 @@ static bool __init parse_positional(struct ns16550 *uart, char **str) > conf += 3; > uart->msi = true; > uart->irq = 0; > + uart->intr_works = polling; How that? "msi" is specifically asking for interrupt driven mode, just that the IRQ number isn't known yet. (Seeing this I notice that parse_namevalue_pairs() offers no way of selecting MSI mode. But that's not something you need to sort out.) Jan
On Mon, 2023-07-24 at 16:43 +0200, Jan Beulich wrote: > On 24.07.2023 16:27, Oleksii Kurochko wrote: > > --- a/xen/drivers/char/console.c > > +++ b/xen/drivers/char/console.c > > @@ -993,6 +993,8 @@ void __init console_init_preirq(void) > > #endif > > else if ( !strncmp(p, "none", 4) ) > > continue; > > + else if ( !strncmp(p, "polling", 7 ) > > + continue; > > else if ( (sh = serial_parse_handle(p)) >= 0 ) > > { > > sercon_handle = sh; > > Looks like you mean the new option to go under "console=". Besides > this being guesswork because of you not updating the command line > doc, this also is wrong, as the property then applies to all > consoles. What you mean is a control for a specific instance of a > 16550 console, which can only possibly go under "com<N>=". I would > suggest to simply extend [<irq>|msi] there to [<irq>|msi|poll]. Thanks. It would be definitely better to go under "com<N>" > > > @@ -595,7 +601,9 @@ static void __init cf_check > > ns16550_endboot(struct serial_port *port) > > static int __init cf_check ns16550_irq(struct serial_port *port) > > { > > struct ns16550 *uart = port->uart; > > - return ((uart->irq > 0) ? uart->irq : -1); > > + > > + return ( uart->intr_works != polling ) ? > > + uart->irq : -1; > > } > > Please don't corrupt previously correct style. You can keep things > almost like they were (albeit there's no strict need for any of the > parentheses): > > return ((uart->intr_works != polling) ? uart->irq : -1); Thanks. > > > @@ -1330,9 +1341,12 @@ pci_uart_config(struct ns16550 *uart, bool_t > > skip_amt, unsigned int idx) > > * as special only for X86. > > */ > > if ( uart->irq == 0xff ) > > + { > > uart->irq = 0; > > + uart->intr_works = polling; > > + } > > #endif > > - if ( !uart->irq ) > > + if ( !uart->irq || (uart->intr_works == polling) ) > > printk(XENLOG_INFO > > "ns16550: %pp: no legacy IRQ, using > > poll mode\n", > > &PCI_SBDF(0, b, d, f)); > > Message and code (after your addition) continue to be out of sync. > As said before, any condition that leads to polling mode wants to > find itself expressed by uart->intr_works set to "polling". Well. It looks like it would be better to set 'uart->intr_works = polling' inside if ( !uart->irq ): if ( !uart->irq || (uart->intr_works == polling) ) { uart->intr_works = polling; printk(XENLOG_INFO "ns16550: %pp: using poll mode\n", &PCI_SBDF(0, b, d, f)); } Then "uart->intr_works = polling;" can be removed from "if ( uart->irq == 0xff )" as in that case 'uart->irq = 0' means polling mode for X86. > > > @@ -1552,6 +1566,7 @@ static bool __init parse_positional(struct > > ns16550 *uart, char **str) > > conf += 3; > > uart->msi = true; > > uart->irq = 0; > > + uart->intr_works = polling; > > How that? "msi" is specifically asking for interrupt driven mode, > just that the IRQ number isn't known yet. (Seeing this I notice that > parse_namevalue_pairs() offers no way of selecting MSI mode. But > that's not something you need to sort out.) I was confused by the code from ns16550_init_postirq(): if ( rc ) { uart->irq = 0; if ( msi_desc ) msi_free_irq(msi_desc); else destroy_irq(msi.irq); } where "uart->irq = 0;" means that polling mode should be used because of the following code in the same function: if ( uart->irq > 0 ) { uart->irqaction.handler = ns16550_interrupt; uart->irqaction.name = "ns16550"; uart->irqaction.dev_id = port; if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 ) printk("ERROR: Failed to allocate ns16550 IRQ %d\n", uart- >irq); } ~ Oleksii
On 25.07.2023 10:15, Oleksii wrote: > On Mon, 2023-07-24 at 16:43 +0200, Jan Beulich wrote: >> On 24.07.2023 16:27, Oleksii Kurochko wrote: >>> @@ -1330,9 +1341,12 @@ pci_uart_config(struct ns16550 *uart, bool_t >>> skip_amt, unsigned int idx) >>> * as special only for X86. >>> */ >>> if ( uart->irq == 0xff ) >>> + { >>> uart->irq = 0; >>> + uart->intr_works = polling; >>> + } >>> #endif >>> - if ( !uart->irq ) >>> + if ( !uart->irq || (uart->intr_works == polling) ) >>> printk(XENLOG_INFO >>> "ns16550: %pp: no legacy IRQ, using >>> poll mode\n", >>> &PCI_SBDF(0, b, d, f)); >> >> Message and code (after your addition) continue to be out of sync. >> As said before, any condition that leads to polling mode wants to >> find itself expressed by uart->intr_works set to "polling". > Well. It looks like it would be better to set 'uart->intr_works = > polling' inside if ( !uart->irq ): > if ( !uart->irq || (uart->intr_works == polling) ) > { > uart->intr_works = polling; > printk(XENLOG_INFO > "ns16550: %pp: using poll mode\n", > &PCI_SBDF(0, b, d, f)); > } > Then "uart->intr_works = polling;" can be removed from "if ( uart->irq > == 0xff )" as in that case 'uart->irq = 0' means polling mode for X86. I'm not fully convinced. Care needs to be taken that both the x86 case and the general case continue to function as they did (unless proven to be buggy). Jan
On Tue, 2023-07-25 at 10:18 +0200, Jan Beulich wrote: > On 25.07.2023 10:15, Oleksii wrote: > > On Mon, 2023-07-24 at 16:43 +0200, Jan Beulich wrote: > > > On 24.07.2023 16:27, Oleksii Kurochko wrote: > > > > @@ -1330,9 +1341,12 @@ pci_uart_config(struct ns16550 *uart, > > > > bool_t > > > > skip_amt, unsigned int idx) > > > > * as special only for X86. > > > > */ > > > > if ( uart->irq == 0xff ) > > > > + { > > > > uart->irq = 0; > > > > + uart->intr_works = polling; > > > > + } > > > > #endif > > > > - if ( !uart->irq ) > > > > + if ( !uart->irq || (uart->intr_works == > > > > polling) ) > > > > printk(XENLOG_INFO > > > > "ns16550: %pp: no legacy IRQ, using > > > > poll mode\n", > > > > &PCI_SBDF(0, b, d, f)); > > > > > > Message and code (after your addition) continue to be out of > > > sync. > > > As said before, any condition that leads to polling mode wants to > > > find itself expressed by uart->intr_works set to "polling". > > Well. It looks like it would be better to set 'uart->intr_works = > > polling' inside if ( !uart->irq ): > > if ( !uart->irq || (uart->intr_works == polling) ) > > { > > uart->intr_works = polling; > > printk(XENLOG_INFO > > "ns16550: %pp: using poll mode\n", > > &PCI_SBDF(0, b, d, f)); > > } > > Then "uart->intr_works = polling;" can be removed from "if ( uart- > > >irq > > == 0xff )" as in that case 'uart->irq = 0' means polling mode for > > X86. > > I'm not fully convinced. Care needs to be taken that both the x86 > case > and the general case continue to function as they did (unless proven > to > be buggy). But it continues to work as it worked before ( when uart->intr_works != polling ) otherwise, if uart->intr_works = polling, then polling mode is forced. The only thing that I would probably add it is to force polling mode in case of X86 when polling mode is forced by command line: if ( ( uart->irq == 0xff ) || (uart->intr_works == polling) ) { uart->irq = 0; uart->intr_works = polling; } But this check looks a little bit unnecessary because if the polling mode is forced or not will be checked later in the next if condition. ~ Oleksii
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c index 0e410fa086..07e9610d77 100644 --- a/xen/drivers/char/console.c +++ b/xen/drivers/char/console.c @@ -993,6 +993,8 @@ void __init console_init_preirq(void) #endif else if ( !strncmp(p, "none", 4) ) continue; + else if ( !strncmp(p, "polling", 7 ) + continue; else if ( (sh = serial_parse_handle(p)) >= 0 ) { sercon_handle = sh; diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c index 2aed6ec707..a6eb3b3997 100644 --- a/xen/drivers/char/ns16550.c +++ b/xen/drivers/char/ns16550.c @@ -58,7 +58,11 @@ static struct ns16550 { struct timer timer; struct timer resume_timer; unsigned int timeout_ms; - bool_t intr_works; + enum { + intr_off, + intr_on, + polling, + } intr_works; bool_t dw_usr_bsy; #ifdef NS16550_PCI /* PCI card parameters. */ @@ -181,7 +185,7 @@ static void cf_check ns16550_interrupt( struct serial_port *port = dev_id; struct ns16550 *uart = port->uart; - uart->intr_works = 1; + uart->intr_works = intr_on; while ( !(ns_read_reg(uart, UART_IIR) & UART_IIR_NOINT) ) { @@ -212,7 +216,7 @@ static void cf_check __ns16550_poll(struct cpu_user_regs *regs) struct serial_port *port = this_cpu(poll_port); struct ns16550 *uart = port->uart; - if ( uart->intr_works ) + if ( uart->intr_works == intr_on ) return; /* Interrupts work - no more polling */ while ( ns_read_reg(uart, UART_LSR) & UART_LSR_DR ) @@ -305,7 +309,8 @@ static void ns16550_setup_preirq(struct ns16550 *uart) unsigned char lcr; unsigned int divisor; - uart->intr_works = 0; + if ( uart->intr_works != polling ) + uart->intr_works = intr_off; pci_serial_early_init(uart); @@ -394,7 +399,7 @@ static void __init cf_check ns16550_init_irq(struct serial_port *port) static void ns16550_setup_postirq(struct ns16550 *uart) { - if ( uart->irq > 0 ) + if ( uart->intr_works != polling ) { /* Master interrupt enable; also keep DTR/RTS asserted. */ ns_write_reg(uart, @@ -473,6 +478,7 @@ static void __init cf_check ns16550_init_postirq(struct serial_port *port) if ( rc ) { uart->irq = 0; + uart->intr_works = polling; if ( msi_desc ) msi_free_irq(msi_desc); else @@ -488,7 +494,7 @@ static void __init cf_check ns16550_init_postirq(struct serial_port *port) } #endif - if ( uart->irq > 0 ) + if ( uart->intr_works != polling ) { uart->irqaction.handler = ns16550_interrupt; uart->irqaction.name = "ns16550"; @@ -595,7 +601,9 @@ static void __init cf_check ns16550_endboot(struct serial_port *port) static int __init cf_check ns16550_irq(struct serial_port *port) { struct ns16550 *uart = port->uart; - return ((uart->irq > 0) ? uart->irq : -1); + + return ( uart->intr_works != polling ) ? + uart->irq : -1; } static void cf_check ns16550_start_tx(struct serial_port *port) @@ -654,6 +662,9 @@ static void ns16550_init_common(struct ns16550 *uart) /* Default lsr_mask = UART_LSR_THRE */ uart->lsr_mask = UART_LSR_THRE; + + if ( console_has("polling") ) + uart->intr_works = polling; } #ifdef CONFIG_X86 @@ -1330,9 +1341,12 @@ pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx) * as special only for X86. */ if ( uart->irq == 0xff ) + { uart->irq = 0; + uart->intr_works = polling; + } #endif - if ( !uart->irq ) + if ( !uart->irq || (uart->intr_works == polling) ) printk(XENLOG_INFO "ns16550: %pp: no legacy IRQ, using poll mode\n", &PCI_SBDF(0, b, d, f)); @@ -1552,6 +1566,7 @@ static bool __init parse_positional(struct ns16550 *uart, char **str) conf += 3; uart->msi = true; uart->irq = 0; + uart->intr_works = polling; } else #endif @@ -1791,8 +1806,11 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev, } res = platform_get_irq(dev, 0); - if ( ! res ) - return -EINVAL; + if ( res < 0 ) + { + printk("there is no interrupt property, polling will be used\n"); + uart->intr_works = polling; + } uart->irq = res; uart->dw_usr_bsy = dt_device_is_compatible(dev, "snps,dw-apb-uart");
RISC-V doesn't support interrupts for the time being, so it would be nice to have polling mode. The patch adds new option to console='...,polling' which forces driver to use polling mode. If there is no interrupt property in UART node, then polling will be used. According to 4.2.2 National Semiconductor 16450/16550 Compatible UART Requirements from The Devicetree Specification v0.4 ( https://www.devicetree.org/specifications/ ) the interrupt property is optional. Also, it is possible that interrupt '0' can be used for some architectures as an legal UART interrupt number ( according to dts files in Linux kernel ), so the check of the return value of platform_get_irq() was updated in case of when device tree is used for UART initialization. For example: https://github.com/torvalds/linux/blob/master/arch/powerpc/boot/dts/ebony.dts#L197 Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- xen/drivers/char/console.c | 2 ++ xen/drivers/char/ns16550.c | 38 ++++++++++++++++++++++++++++---------- 2 files changed, 30 insertions(+), 10 deletions(-)