| Message ID | 20241025105728.602310-5-john.ogness@linutronix.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | convert 8250 to nbcon | expand |
On Fri, Oct 25, 2024 at 01:03:26PM +0206, John Ogness wrote: > For RS485 mode, if SER_RS485_RX_DURING_TX is not available, the > console write callback needs to enable/disable TX. It does this > by calling the rs485_start/stop_tx() callbacks. However, these It would be nice if you be consistent across the commit messages and also provide the names of the callbacks in full, because I dunno if we have a local stop_tx() or rs485_start() or whatever the above means. If it is "the rs485_start_tx() / rs485_stop_tx() callbacks.", it's much clearer for the reader. > callbacks will disable/enable interrupts, which is a problem toggle? > for console write, as it must be responsible for > disabling/enabling interrupts. toggling ? > Add an argument @in_con to the rs485_start/stop_tx() callbacks As per above. > to specify if they are being called from console write. If so, > the callbacks will not handle interrupt disabling/enabling. toggling ? > For all call sites other than console write, there is no > functional change. So, why not call the parameter better to emphasize that it's about IRQ toggling? bool toggle_irq ?
On 2024-10-25, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: >> Add an argument @in_con to the rs485_start/stop_tx() callbacks >> to specify if they are being called from console write. If so, >> the callbacks will not handle interrupt disabling/enabling. > > toggling ? > >> For all call sites other than console write, there is no >> functional change. > > So, why not call the parameter better to emphasize that it's about IRQ > toggling? bool toggle_irq ? Currently there are only 2 users: serial8250_em485_stop_tx() bcm2835aux_rs485_stop_tx() The first one toggles the IER bits, the second one does not. I figured it would make more sense to specify the context rather than what needs to be done and let the 8250-variant decide what it should do. But I have no problems renaming it to toggle_irq. It is an 8250-specific callback with few users. And really the IER bits is the only reason that the argument even needs to exist. John
On Fri, Oct 25, 2024 at 04:31:05PM +0206, John Ogness wrote: > On 2024-10-25, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > >> Add an argument @in_con to the rs485_start/stop_tx() callbacks > >> to specify if they are being called from console write. If so, > >> the callbacks will not handle interrupt disabling/enabling. > > > > toggling ? > > > >> For all call sites other than console write, there is no > >> functional change. > > > > So, why not call the parameter better to emphasize that it's about IRQ > > toggling? bool toggle_irq ? > > Currently there are only 2 users: > > serial8250_em485_stop_tx() > bcm2835aux_rs485_stop_tx() > > The first one toggles the IER bits, the second one does not. I figured > it would make more sense to specify the context rather than what needs > to be done and let the 8250-variant decide what it should do. > > But I have no problems renaming it to toggle_irq. It is an 8250-specific > callback with few users. And really the IER bits is the only reason that > the argument even needs to exist. Maybe toggle_ier will be better than? I haven't looked deeply into the implementations, so choose whichever describes better what's behind it.
On 25. 10. 24, 12:57, John Ogness wrote: > For RS485 mode, if SER_RS485_RX_DURING_TX is not available, the > console write callback needs to enable/disable TX. It does this > by calling the rs485_start/stop_tx() callbacks. However, these > callbacks will disable/enable interrupts, which is a problem > for console write, as it must be responsible for > disabling/enabling interrupts. > > Add an argument @in_con to the rs485_start/stop_tx() callbacks > to specify if they are being called from console write. If so, > the callbacks will not handle interrupt disabling/enabling. > > For all call sites other than console write, there is no > functional change. > > Signed-off-by: John Ogness <john.ogness@linutronix.de> ... > @@ -1562,12 +1566,20 @@ static inline void __start_tx(struct uart_port *port) > * stoppable by disabling the UART_IER_RDI interrupt. (Some chips set the > * UART_LSR_DR bit even when UART_IER_RDI is disabled, foiling this approach.) > */ > -void serial8250_em485_start_tx(struct uart_8250_port *up) > +void serial8250_em485_start_tx(struct uart_8250_port *up, bool in_con) > { > unsigned char mcr = serial8250_in_MCR(up); > > - if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX)) > - serial8250_stop_rx(&up->port); > + if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX)) { > + /* > + * In console context, caller handles interrupt disabling. So > + * only LSR_DR masking is needed. > + */ > + if (in_con) > + __serial8250_stop_rx_mask_dr(&up->port); > + else > + serial8250_stop_rx(&up->port); Would it make sense to propagate in_con into serial8250_stop_rx() and do the logic there? That would effectively eliminate patch 2/6. thanks,
On 2024-10-30, Jiri Slaby <jirislaby@kernel.org> wrote: >> -void serial8250_em485_start_tx(struct uart_8250_port *up) >> +void serial8250_em485_start_tx(struct uart_8250_port *up, bool in_con) >> { >> unsigned char mcr = serial8250_in_MCR(up); >> >> - if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX)) >> - serial8250_stop_rx(&up->port); >> + if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX)) { >> + /* >> + * In console context, caller handles interrupt disabling. So >> + * only LSR_DR masking is needed. >> + */ >> + if (in_con) >> + __serial8250_stop_rx_mask_dr(&up->port); >> + else >> + serial8250_stop_rx(&up->port); > > Would it make sense to propagate in_con into serial8250_stop_rx() and do > the logic there? That would effectively eliminate patch 2/6. I considered this, however: 1. The whole idea of stopping RX in order to do TX is an RS485 issue. Modifying the general ->stop_rx() callback for this purpose is kind of out of place. 2. The ->stop_rx() callback is a general uart_ops callback. Changing its prototype would literally affect all serial drivers. OTOH the ->rs485_start_tx() callback is specific to the 8250 driver. (It seems each driver has implemented their own method for handling the RS485 hacks.) So I would prefer to keep the necessary RS485 changes 8250-specific for now. John
On Fri 2024-10-25 13:03:26, John Ogness wrote: > For RS485 mode, if SER_RS485_RX_DURING_TX is not available, the > console write callback needs to enable/disable TX. It does this > by calling the rs485_start/stop_tx() callbacks. However, these > callbacks will disable/enable interrupts, which is a problem > for console write, as it must be responsible for > disabling/enabling interrupts. It is not clear to me what exactly is the problem. Is the main problem calling pm_runtime*() API because it uses extra locks and can cause deadlocks? Or is it more complicated? IMHO, it would deserve some explanation. > Add an argument @in_con to the rs485_start/stop_tx() callbacks > to specify if they are being called from console write. If so, > the callbacks will not handle interrupt disabling/enabling. > > For all call sites other than console write, there is no > functional change. > > Signed-off-by: John Ogness <john.ogness@linutronix.de> It looks like the code does what the description says. And honestly, I do not have any idea how to improve the naming. I would keep it as is after reading John's answers in the thread. IMHO, one thing which makes things comlicated is that serial8250_em485_start_tx() and serial8250_em485_stop_tx() are not completely reversible operations. Especially, the change done by __serial8250_stop_rx_mask_dr() is not reverted in serial8250_em485_stop_tx(). It makes things look tricky. But I think that it is beyond the scope of this patchset to do anything about it. Just 2 my cents. Best Regaards, Petr
diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h index e5310c65cf52..0d8717be0df7 100644 --- a/drivers/tty/serial/8250/8250.h +++ b/drivers/tty/serial/8250/8250.h @@ -231,8 +231,8 @@ void serial8250_rpm_put_tx(struct uart_8250_port *p); int serial8250_em485_config(struct uart_port *port, struct ktermios *termios, struct serial_rs485 *rs485); -void serial8250_em485_start_tx(struct uart_8250_port *p); -void serial8250_em485_stop_tx(struct uart_8250_port *p); +void serial8250_em485_start_tx(struct uart_8250_port *p, bool in_con); +void serial8250_em485_stop_tx(struct uart_8250_port *p, bool in_con); void serial8250_em485_destroy(struct uart_8250_port *p); extern struct serial_rs485 serial8250_em485_supported; diff --git a/drivers/tty/serial/8250/8250_bcm2835aux.c b/drivers/tty/serial/8250/8250_bcm2835aux.c index fdb53b54e99e..c9a106a86b56 100644 --- a/drivers/tty/serial/8250/8250_bcm2835aux.c +++ b/drivers/tty/serial/8250/8250_bcm2835aux.c @@ -46,7 +46,7 @@ struct bcm2835aux_data { u32 cntl; }; -static void bcm2835aux_rs485_start_tx(struct uart_8250_port *up) +static void bcm2835aux_rs485_start_tx(struct uart_8250_port *up, bool in_con) { if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX)) { struct bcm2835aux_data *data = dev_get_drvdata(up->port.dev); @@ -65,7 +65,7 @@ static void bcm2835aux_rs485_start_tx(struct uart_8250_port *up) serial8250_out_MCR(up, UART_MCR_RTS); } -static void bcm2835aux_rs485_stop_tx(struct uart_8250_port *up) +static void bcm2835aux_rs485_stop_tx(struct uart_8250_port *up, bool in_con) { if (up->port.rs485.flags & SER_RS485_RTS_AFTER_SEND) serial8250_out_MCR(up, 0); diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c index b3be0fb184a3..fcbed7e98231 100644 --- a/drivers/tty/serial/8250/8250_omap.c +++ b/drivers/tty/serial/8250/8250_omap.c @@ -365,7 +365,7 @@ static void omap8250_restore_regs(struct uart_8250_port *up) if (up->port.rs485.flags & SER_RS485_ENABLED && up->port.rs485_config == serial8250_em485_config) - serial8250_em485_stop_tx(up); + serial8250_em485_stop_tx(up, false); } /* diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c index 09ac521d232a..7c50387194ad 100644 --- a/drivers/tty/serial/8250/8250_port.c +++ b/drivers/tty/serial/8250/8250_port.c @@ -558,7 +558,7 @@ static int serial8250_em485_init(struct uart_8250_port *p) deassert_rts: if (p->em485->tx_stopped) - p->rs485_stop_tx(p); + p->rs485_stop_tx(p, false); return 0; } @@ -1398,10 +1398,11 @@ static void serial8250_stop_rx(struct uart_port *port) /** * serial8250_em485_stop_tx() - generic ->rs485_stop_tx() callback * @p: uart 8250 port + * @in_con: true if called from console write, otherwise false * * Generic callback usable by 8250 uart drivers to stop rs485 transmission. */ -void serial8250_em485_stop_tx(struct uart_8250_port *p) +void serial8250_em485_stop_tx(struct uart_8250_port *p, bool in_con) { unsigned char mcr = serial8250_in_MCR(p); @@ -1419,7 +1420,9 @@ void serial8250_em485_stop_tx(struct uart_8250_port *p) if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX)) { serial8250_clear_and_reinit_fifos(p); - __serial8250_start_rx_int(p); + /* In console context, caller handles interrupt enabling. */ + if (!in_con) + __serial8250_start_rx_int(p); } } EXPORT_SYMBOL_GPL(serial8250_em485_stop_tx); @@ -1434,7 +1437,7 @@ static enum hrtimer_restart serial8250_em485_handle_stop_tx(struct hrtimer *t) serial8250_rpm_get(p); uart_port_lock_irqsave(&p->port, &flags); if (em485->active_timer == &em485->stop_tx_timer) { - p->rs485_stop_tx(p); + p->rs485_stop_tx(p, false); em485->active_timer = NULL; em485->tx_stopped = true; } @@ -1466,7 +1469,7 @@ static void __stop_tx_rs485(struct uart_8250_port *p, u64 stop_delay) em485->active_timer = &em485->stop_tx_timer; hrtimer_start(&em485->stop_tx_timer, ns_to_ktime(stop_delay), HRTIMER_MODE_REL); } else { - p->rs485_stop_tx(p); + p->rs485_stop_tx(p, false); em485->active_timer = NULL; em485->tx_stopped = true; } @@ -1555,6 +1558,7 @@ static inline void __start_tx(struct uart_port *port) /** * serial8250_em485_start_tx() - generic ->rs485_start_tx() callback * @up: uart 8250 port + * @in_con: true if called from console write, otherwise false * * Generic callback usable by 8250 uart drivers to start rs485 transmission. * Assumes that setting the RTS bit in the MCR register means RTS is high. @@ -1562,12 +1566,20 @@ static inline void __start_tx(struct uart_port *port) * stoppable by disabling the UART_IER_RDI interrupt. (Some chips set the * UART_LSR_DR bit even when UART_IER_RDI is disabled, foiling this approach.) */ -void serial8250_em485_start_tx(struct uart_8250_port *up) +void serial8250_em485_start_tx(struct uart_8250_port *up, bool in_con) { unsigned char mcr = serial8250_in_MCR(up); - if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX)) - serial8250_stop_rx(&up->port); + if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX)) { + /* + * In console context, caller handles interrupt disabling. So + * only LSR_DR masking is needed. + */ + if (in_con) + __serial8250_stop_rx_mask_dr(&up->port); + else + serial8250_stop_rx(&up->port); + } if (up->port.rs485.flags & SER_RS485_RTS_ON_SEND) mcr |= UART_MCR_RTS; @@ -1600,7 +1612,7 @@ static bool start_tx_rs485(struct uart_port *port) if (em485->tx_stopped) { em485->tx_stopped = false; - up->rs485_start_tx(up); + up->rs485_start_tx(up, false); if (up->port.rs485.delay_rts_before_send > 0) { em485->active_timer = &em485->start_tx_timer; @@ -3402,7 +3414,7 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s, if (em485) { if (em485->tx_stopped) - up->rs485_start_tx(up); + up->rs485_start_tx(up, true); mdelay(port->rs485.delay_rts_before_send); } @@ -3440,7 +3452,7 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s, if (em485) { mdelay(port->rs485.delay_rts_after_send); if (em485->tx_stopped) - up->rs485_stop_tx(up); + up->rs485_stop_tx(up, true); } serial_port_out(port, UART_IER, ier); diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h index e0717c8393d7..c25c026d173d 100644 --- a/include/linux/serial_8250.h +++ b/include/linux/serial_8250.h @@ -161,8 +161,8 @@ struct uart_8250_port { void (*dl_write)(struct uart_8250_port *up, u32 value); struct uart_8250_em485 *em485; - void (*rs485_start_tx)(struct uart_8250_port *); - void (*rs485_stop_tx)(struct uart_8250_port *); + void (*rs485_start_tx)(struct uart_8250_port *up, bool in_con); + void (*rs485_stop_tx)(struct uart_8250_port *up, bool in_con); /* Serial port overrun backoff */ struct delayed_work overrun_backoff;
For RS485 mode, if SER_RS485_RX_DURING_TX is not available, the console write callback needs to enable/disable TX. It does this by calling the rs485_start/stop_tx() callbacks. However, these callbacks will disable/enable interrupts, which is a problem for console write, as it must be responsible for disabling/enabling interrupts. Add an argument @in_con to the rs485_start/stop_tx() callbacks to specify if they are being called from console write. If so, the callbacks will not handle interrupt disabling/enabling. For all call sites other than console write, there is no functional change. Signed-off-by: John Ogness <john.ogness@linutronix.de> --- drivers/tty/serial/8250/8250.h | 4 +-- drivers/tty/serial/8250/8250_bcm2835aux.c | 4 +-- drivers/tty/serial/8250/8250_omap.c | 2 +- drivers/tty/serial/8250/8250_port.c | 34 +++++++++++++++-------- include/linux/serial_8250.h | 4 +-- 5 files changed, 30 insertions(+), 18 deletions(-)